fix(api): sanitize createdByRunId on comment insert to prevent 500s (#9489)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The issue-comments API (`POST /api/issues/:id/comments`) attributes each comment to the run that created it via `created_by_run_id`, a foreign key into `heartbeat_runs` > - In multi-agent local control-plane usage, board/session clients sometimes forward an `X-Paperclip-Run-Id` that is not a real run row — a non-UUID client request id, a synthetic string, or a since-deleted run > - That value was written straight to the FK column, so the insert died with a Postgres FK violation and the endpoint returned HTTP 500, breaking agent coordination > - This PR resolves the run id defensively before insert: reject non-UUID shapes, verify the row exists for the company, and null out anything unresolvable while logging a warning > - The benefit is that a bad run-id header degrades gracefully to an unattributed comment (201) instead of a 500, so comment creation stays up ## Linked Issues or Issue Description No public issue exists; describing inline (bug): **What happened:** `POST /api/issues/:id/comments` returns HTTP 500 when the request carries an `X-Paperclip-Run-Id` that does not correspond to a row in `heartbeat_runs` (non-UUID value, synthetic client id, or deleted run). The value is written to the `created_by_run_id` FK, and Postgres rejects the insert with a foreign-key violation (SQLSTATE 23503). **Expected:** the comment is created (HTTP 201); an unresolvable run id is dropped to `null` rather than failing the request. **Impact:** in multi-agent usage, comment creation — and the agent coordination that depends on it — fails whenever a client forwards a run id that isn't a live run. ## What Changed - Add `resolveCommentCreatedByRunId(dbOrTx, companyId, runId)` — trims and validates UUID shape, then checks existence in `heartbeat_runs` scoped to the company; returns `null` for missing/invalid ids. - `addComment` now resolves the run id through that helper before insert and logs a warning when a supplied run id is dropped. - Add embedded-Postgres regression tests for the three cases (non-UUID header, unknown UUID, valid run id). ## Verification - `pnpm --filter server test issues-service` — the new `issueService.addComment createdByRunId` block passes. - Cases covered: non-UUID header → 201, `createdByRunId: null`; UUID absent from `heartbeat_runs` → 201, `null`; valid run id present for the company → preserved. ## Risks Low. Purely defensive — valid run ids are still preserved, only unresolvable ones are nulled. Adds one indexed, tenant-scoped `SELECT` per comment insert. ## Model Used Claude Opus 4.8 (extended thinking), via the Paperclip PR-triage cockpit, produced the added regression tests and this description. The original implementation is by @digitalflanker-ux; the author's model is unspecified. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [ ] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above (related: #4795 same fix; #8065 sibling FK-guard on the activity-log path) - [x] I have either (a) linked existing issues OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links - [ ] My branch name describes the change and contains no internal Paperclip ticket id - [ ] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com> Co-authored-by: Andrew Aymeloglu <aaymeloglu@gmail.com>
This commit is contained in:
openclaw-fmag
GitHub
parent
156830006b
commit
a59aa128a3
|
|
@ -6063,3 +6063,91 @@ describeEmbeddedPostgres("issueService.assertCheckoutOwner stale checkout adopti
|
|||
});
|
||||
|
||||
});
|
||||
|
||||
describeEmbeddedPostgres("issueService.addComment createdByRunId", () => {
|
||||
let db!: ReturnType<typeof createDb>;
|
||||
let svc!: ReturnType<typeof issueService>;
|
||||
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | null = null;
|
||||
let companyId!: string;
|
||||
let agentId!: string;
|
||||
let issueId!: string;
|
||||
|
||||
beforeAll(async () => {
|
||||
tempDb = await startEmbeddedPostgresTestDatabase("paperclip-issues-comment-runid-");
|
||||
db = createDb(tempDb.connectionString);
|
||||
svc = issueService(db);
|
||||
|
||||
companyId = randomUUID();
|
||||
agentId = randomUUID();
|
||||
issueId = randomUUID();
|
||||
|
||||
await db.insert(companies).values({
|
||||
id: companyId,
|
||||
name: "Paperclip",
|
||||
issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`,
|
||||
requireBoardApprovalForNewAgents: false,
|
||||
});
|
||||
await db.insert(agents).values({
|
||||
id: agentId,
|
||||
companyId,
|
||||
name: "TestAgent",
|
||||
role: "engineer",
|
||||
status: "active",
|
||||
adapterType: "codex_local",
|
||||
adapterConfig: {},
|
||||
runtimeConfig: {},
|
||||
permissions: {},
|
||||
});
|
||||
await db.insert(issues).values({
|
||||
id: issueId,
|
||||
companyId,
|
||||
title: "Test issue",
|
||||
status: "todo",
|
||||
priority: "medium",
|
||||
});
|
||||
}, 20_000);
|
||||
|
||||
afterAll(async () => {
|
||||
await tempDb?.cleanup();
|
||||
});
|
||||
|
||||
async function createdByRunIdFor(commentId: string) {
|
||||
return db
|
||||
.select({ createdByRunId: issueComments.createdByRunId })
|
||||
.from(issueComments)
|
||||
.where(eq(issueComments.id, commentId))
|
||||
.then((rows) => rows[0]?.createdByRunId ?? null);
|
||||
}
|
||||
|
||||
it("nulls out a non-UUID x-paperclip-run-id instead of 500-ing", async () => {
|
||||
const comment = await svc.addComment(issueId, "hello from a synthetic run id", {
|
||||
runId: "client-request-abc123",
|
||||
});
|
||||
|
||||
expect(comment.id).toBeTruthy();
|
||||
expect(await createdByRunIdFor(comment.id)).toBeNull();
|
||||
});
|
||||
|
||||
it("nulls out a UUID runId absent from heartbeat_runs instead of 500-ing", async () => {
|
||||
const comment = await svc.addComment(issueId, "hello from a stale run", {
|
||||
runId: randomUUID(),
|
||||
});
|
||||
|
||||
expect(comment.id).toBeTruthy();
|
||||
expect(await createdByRunIdFor(comment.id)).toBeNull();
|
||||
});
|
||||
|
||||
it("preserves a valid runId that exists in heartbeat_runs for the company", async () => {
|
||||
const runId = randomUUID();
|
||||
await db.insert(heartbeatRuns).values({
|
||||
id: runId,
|
||||
companyId,
|
||||
agentId,
|
||||
status: "running",
|
||||
});
|
||||
|
||||
const comment = await svc.addComment(issueId, "hello from a live run", { runId });
|
||||
|
||||
expect(await createdByRunIdFor(comment.id)).toBe(runId);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -355,6 +355,25 @@ type DerivedIssueCommentAttribution = {
|
|||
derivedAuthorSource: IssueCommentDerivedAuthorSource;
|
||||
};
|
||||
|
||||
/**
|
||||
* Resolve a `created_by_run_id` safe for the heartbeat_runs FK; returns null for
|
||||
* missing/invalid ids so an unknown run id never 500s a comment insert.
|
||||
*/
|
||||
async function resolveCommentCreatedByRunId(
|
||||
dbOrTx: any,
|
||||
companyId: string,
|
||||
runId: string | null | undefined,
|
||||
): Promise<string | null> {
|
||||
const normalized = typeof runId === "string" ? runId.trim() : "";
|
||||
if (!normalized || !isUuidLike(normalized)) return null;
|
||||
const existing = await dbOrTx
|
||||
.select({ id: heartbeatRuns.id })
|
||||
.from(heartbeatRuns)
|
||||
.where(and(eq(heartbeatRuns.id, normalized), eq(heartbeatRuns.companyId, companyId)))
|
||||
.then((rows: Array<{ id: string }>) => rows[0] ?? null);
|
||||
return existing?.id ?? null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Best-effort agent attribution for comments whose stored author is a non-human
|
||||
* sentinel (e.g. `local-board`). Callers MUST pre-filter `comments` to drop any
|
||||
|
|
@ -7428,6 +7447,14 @@ export function issueService(db: Db) {
|
|||
const presentation = issueCommentPresentationSchema.nullable().parse(options?.presentation ?? null);
|
||||
const metadata = issueCommentMetadataSchema.nullable().parse(options?.metadata ?? null);
|
||||
const createdAt = options?.createdAt ? new Date(options.createdAt) : null;
|
||||
// Invalid/stale run ids must not 500 the insert — null out unknowns.
|
||||
const createdByRunId = await resolveCommentCreatedByRunId(dbOrTx, issue.companyId, actor.runId);
|
||||
if (actor.runId && !createdByRunId) {
|
||||
logger.warn(
|
||||
{ issueId, companyId: issue.companyId, runId: actor.runId },
|
||||
"dropping invalid createdByRunId for issue comment insert",
|
||||
);
|
||||
}
|
||||
const [comment] = await dbOrTx
|
||||
.insert(issueComments)
|
||||
.values({
|
||||
|
|
@ -7436,7 +7463,7 @@ export function issueService(db: Db) {
|
|||
authorAgentId: actor.agentId ?? null,
|
||||
authorUserId: actor.userId ?? null,
|
||||
authorType,
|
||||
createdByRunId: actor.runId ?? null,
|
||||
createdByRunId,
|
||||
body: redactedBody,
|
||||
presentation,
|
||||
metadata,
|
||||
|
|
|
|||
Loading…
Reference in New Issue