From a59aa128a3ed82aea3102ab5e7e7ce82ed4dee9f Mon Sep 17 00:00:00 2001 From: openclaw-fmag Date: Mon, 20 Jul 2026 17:31:42 -0500 Subject: [PATCH] fix(api): sanitize createdByRunId on comment insert to prevent 500s (#9489) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 Co-authored-by: Andrew Aymeloglu --- server/src/__tests__/issues-service.test.ts | 88 +++++++++++++++++++++ server/src/services/issues.ts | 29 ++++++- 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index 0d6675133e..a60da3e442 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -6063,3 +6063,91 @@ describeEmbeddedPostgres("issueService.assertCheckoutOwner stale checkout adopti }); }); + +describeEmbeddedPostgres("issueService.addComment createdByRunId", () => { + let db!: ReturnType; + let svc!: ReturnType; + let tempDb: Awaited> | 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); + }); +}); diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 64df23809b..66e974649a 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -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 { + 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,