From 7df36ef61589a58996ec548b12a9594207004878 Mon Sep 17 00:00:00 2001 From: Dotta Date: Sun, 7 Jun 2026 03:43:33 +0000 Subject: [PATCH] Stop agent doc writes 500ing on a run-id unknown to the local DB (PAP-10583) An agent presents its live X-Paperclip-Run-Id, which can reference a heartbeat run that does not exist in this server's DB (e.g. a worktree dev environment whose DB never recorded the run). That run-id is FK-constrained on issues.checkout_run_id/execution_run_id, document_revisions.created_by_run_id and activity_log.run_id, so persisting it raised a FK violation that surfaced as a 500. Board users were unaffected because they carry no run-id. Resolve the run-id to null at each FK-bearing write site when the run is unknown: checkout adoption still grants the assigned agent ownership (without a lock), document provenance and activity logging degrade run_id to null. In real deployments the run exists, so the value is persisted unchanged. Co-Authored-By: Claude Opus 4.8 --- .../issue-document-unknown-run-routes.test.ts | 201 ++++++++++++++++++ server/src/services/activity-log.ts | 21 +- server/src/services/documents.ts | 26 ++- server/src/services/issues.ts | 26 ++- 4 files changed, 265 insertions(+), 9 deletions(-) create mode 100644 server/src/__tests__/issue-document-unknown-run-routes.test.ts diff --git a/server/src/__tests__/issue-document-unknown-run-routes.test.ts b/server/src/__tests__/issue-document-unknown-run-routes.test.ts new file mode 100644 index 00000000000..cc350738256 --- /dev/null +++ b/server/src/__tests__/issue-document-unknown-run-routes.test.ts @@ -0,0 +1,201 @@ +import { randomUUID } from "node:crypto"; +import express from "express"; +import request from "supertest"; +import { eq } from "drizzle-orm"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { + activityLog, + agents, + companies, + createDb, + documentRevisions, + documents, + heartbeatRuns, + issueComments, + issueDocuments, + issueRelations, + issues, +} from "@paperclipai/db"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { errorHandler } from "../middleware/index.js"; +import { issueRoutes } from "../routes/issues.js"; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +if (!embeddedPostgresSupport.supported) { + console.warn( + `Skipping embedded Postgres unknown-run document route tests on this host: ${ + embeddedPostgresSupport.reason ?? "unsupported environment" + }`, + ); +} + +// Regression coverage for PAP-10583: an agent presents its live X-Paperclip-Run-Id, which +// can reference a heartbeat run that does not exist in this server's DB (e.g. a worktree +// dev environment). The run-id is FK-constrained on issues.checkout_run_id, +// document_revisions.created_by_run_id and activity_log.run_id, so persisting it raised a +// FK violation that surfaced as a 500. Board users were unaffected because they carry no +// run-id. Writes must now succeed (the agent path is the design contract) with run +// provenance degraded to null, while a known run-id is still persisted unchanged. +describeEmbeddedPostgres("issue document writes with an unknown agent run-id", () => { + let db!: ReturnType; + let tempDb: Awaited> | null = null; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-issue-document-unknown-run-"); + db = createDb(tempDb.connectionString); + }, 20_000); + + afterEach(async () => { + await db.delete(documentRevisions); + await db.delete(issueDocuments); + await db.delete(documents); + await db.delete(issueComments); + await db.delete(issueRelations); + await db.delete(activityLog); + await db.delete(issues); + await db.delete(heartbeatRuns); + await db.delete(agents); + await db.delete(companies); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + function createApp(actor: Express.Request["actor"]) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.actor = actor; + next(); + }); + app.use("/api", issueRoutes(db, {} as any)); + app.use(errorHandler); + return app; + } + + function agentActor(companyId: string, agentId: string, runId: string): Express.Request["actor"] { + return { type: "agent", agentId, companyId, runId, source: "agent_jwt" }; + } + + async function seed(options: { recordRun: boolean }) { + const companyId = randomUUID(); + const agentId = randomUUID(); + const runId = randomUUID(); + const 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: "ClaudeCoder", + role: "engineer", + status: "active", + adapterType: "codex_local", + adapterConfig: {}, + runtimeConfig: {}, + permissions: {}, + }); + if (options.recordRun) { + await db.insert(heartbeatRuns).values({ + id: runId, + companyId, + agentId, + status: "running", + invocationSource: "manual", + startedAt: new Date(), + }); + } + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Doc write", + status: "in_progress", + priority: "high", + assigneeAgentId: agentId, + checkoutRunId: null, + executionRunId: null, + }); + + return { companyId, agentId, runId, issueId }; + } + + it("returns 201 and degrades run provenance to null when the run is unknown to this DB", async () => { + const { companyId, agentId, runId, issueId } = await seed({ recordRun: false }); + const app = createApp(agentActor(companyId, agentId, runId)); + + const created = await request(app) + .put(`/api/issues/${issueId}/documents/qa-test-doc`) + .send({ title: "x", format: "markdown", body: "# x" }); + expect(created.status, JSON.stringify(created.body)).toBe(201); + + const revision = await db + .select({ createdByRunId: documentRevisions.createdByRunId, createdByAgentId: documentRevisions.createdByAgentId }) + .from(documentRevisions) + .then((rows) => rows[0]); + // FK-bearing run-id is dropped, but the agent is still recorded as the author. + expect(revision).toEqual({ createdByRunId: null, createdByAgentId: agentId }); + + // Checkout adoption cannot persist a missing run, so ownership is granted without a lock. + const issueRow = await db + .select({ checkoutRunId: issues.checkoutRunId, executionRunId: issues.executionRunId }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]); + expect(issueRow).toEqual({ checkoutRunId: null, executionRunId: null }); + + // Activity is still logged, with run_id nulled rather than FK-violating. + const activity = await db + .select({ action: activityLog.action, runId: activityLog.runId, agentId: activityLog.agentId }) + .from(activityLog) + .where(eq(activityLog.action, "issue.document_created")) + .then((rows) => rows[0]); + expect(activity).toMatchObject({ action: "issue.document_created", runId: null, agentId }); + + // A follow-up update on the same doc must also succeed (the existing-document path). + const updated = await request(app) + .put(`/api/issues/${issueId}/documents/qa-test-doc`) + .send({ title: "y", format: "markdown", body: "# y", baseRevisionId: created.body.latestRevisionId }); + expect(updated.status, JSON.stringify(updated.body)).toBe(200); + }); + + it("persists the run-id unchanged when the run exists in this DB", async () => { + const { companyId, agentId, runId, issueId } = await seed({ recordRun: true }); + const app = createApp(agentActor(companyId, agentId, runId)); + + const created = await request(app) + .put(`/api/issues/${issueId}/documents/qa-test-doc`) + .send({ title: "x", format: "markdown", body: "# x" }); + expect(created.status, JSON.stringify(created.body)).toBe(201); + + const revision = await db + .select({ createdByRunId: documentRevisions.createdByRunId }) + .from(documentRevisions) + .then((rows) => rows[0]); + expect(revision).toEqual({ createdByRunId: runId }); + + const issueRow = await db + .select({ checkoutRunId: issues.checkoutRunId, executionRunId: issues.executionRunId }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]); + expect(issueRow).toEqual({ checkoutRunId: runId, executionRunId: runId }); + + const activity = await db + .select({ runId: activityLog.runId }) + .from(activityLog) + .where(eq(activityLog.action, "issue.document_created")) + .then((rows) => rows[0]); + expect(activity).toEqual({ runId }); + }); +}); diff --git a/server/src/services/activity-log.ts b/server/src/services/activity-log.ts index 473a1d787bf..37d01d7bca6 100644 --- a/server/src/services/activity-log.ts +++ b/server/src/services/activity-log.ts @@ -1,6 +1,7 @@ import { randomUUID } from "node:crypto"; +import { eq } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; -import { activityLog } from "@paperclipai/db"; +import { activityLog, heartbeatRuns } from "@paperclipai/db"; import { PLUGIN_EVENT_TYPES, type PluginEventType } from "@paperclipai/shared"; import type { PluginEvent } from "@paperclipai/plugin-sdk"; import { publishLiveEvent } from "./live-events.js"; @@ -62,6 +63,21 @@ export interface LogActivityInput { details?: Record | null; } +// activity_log.run_id is FK-constrained to heartbeat_runs. An agent presents its live +// run-id, which may not be recorded in this server's DB (e.g. a worktree dev environment +// whose DB never saw the run). Persisting a missing run-id raises a FK violation and turns +// every agent-token write that logs activity into a 500. Resolve provenance to null when +// the run is unknown; in real deployments the run exists and the value is unchanged. +async function persistableActivityRunId(db: Db, runId: string | null | undefined): Promise { + if (!runId) return null; + const run = await db + .select({ id: heartbeatRuns.id }) + .from(heartbeatRuns) + .where(eq(heartbeatRuns.id, runId)) + .then((rows) => rows[0] ?? null); + return run ? runId : null; +} + export async function logActivity(db: Db, input: LogActivityInput) { const currentUserRedactionOptions = { enabled: (await instanceSettingsService(db).getGeneral()).censorUsernameInLogs, @@ -70,6 +86,7 @@ export async function logActivity(db: Db, input: LogActivityInput) { const redactedDetails = sanitizedDetails ? redactCurrentUserValue(sanitizedDetails, currentUserRedactionOptions) : null; + const runId = await persistableActivityRunId(db, input.runId); await db.insert(activityLog).values({ companyId: input.companyId, actorType: input.actorType, @@ -78,7 +95,7 @@ export async function logActivity(db: Db, input: LogActivityInput) { entityType: input.entityType, entityId: input.entityId, agentId: input.agentId ?? null, - runId: input.runId ?? null, + runId, details: redactedDetails, }); diff --git a/server/src/services/documents.ts b/server/src/services/documents.ts index d379fb35959..7851903b08d 100644 --- a/server/src/services/documents.ts +++ b/server/src/services/documents.ts @@ -42,6 +42,19 @@ function isUniqueViolation(error: unknown): boolean { return !!error && typeof error === "object" && "code" in error && (error as { code?: string }).code === "23505"; } +// Resolve a document-revision run-id to null when it does not reference a known +// heartbeat_runs row, avoiding a FK violation when an agent's live run-id is unknown to +// this server's DB. Returns the run-id unchanged when it exists. +async function persistableDocumentRunId(db: Db, runId: string | null): Promise { + if (!runId) return null; + const run = await db + .select({ id: heartbeatRuns.id }) + .from(heartbeatRuns) + .where(eq(heartbeatRuns.id, runId)) + .then((rows) => rows[0] ?? null); + return run ? runId : null; +} + function nextAvailableDocumentKey(sourceKey: string, existingKeys: string[]) { const usedKeys = new Set(existingKeys); for (let index = 2; index < 1000; index += 1) { @@ -817,6 +830,13 @@ export function documentService(db: Db) { .then((rows) => rows[0] ?? null); if (!issue) throw notFound("Issue not found"); + // document_revisions.created_by_run_id is FK-constrained to heartbeat_runs. An agent + // presents its live run-id, which may not exist in this server's DB (e.g. a worktree + // dev environment whose DB never recorded the run). Persisting a missing run-id would + // raise a FK violation and surface as a 500, so resolve provenance to null when the + // run is unknown. In real deployments the run exists and the value is unchanged. + const createdByRunId = await persistableDocumentRunId(db, input.createdByRunId ?? null); + const maxAttempts = input.lockedDocumentStrategy === "create_new_document" ? 3 : 1; for (let attempt = 0; attempt < maxAttempts; attempt += 1) { try { @@ -893,7 +913,7 @@ export function documentService(db: Db) { changeSummary: input.changeSummary ?? null, createdByAgentId: input.createdByAgentId ?? null, createdByUserId: input.createdByUserId ?? null, - createdByRunId: input.createdByRunId ?? null, + createdByRunId, createdAt: now, }) .returning(); @@ -994,7 +1014,7 @@ export function documentService(db: Db) { changeSummary: input.changeSummary ?? null, createdByAgentId: input.createdByAgentId ?? null, createdByUserId: input.createdByUserId ?? null, - createdByRunId: input.createdByRunId ?? null, + createdByRunId, createdAt: now, }) .returning(); @@ -1078,7 +1098,7 @@ export function documentService(db: Db) { changeSummary: input.changeSummary ?? null, createdByAgentId: input.createdByAgentId ?? null, createdByUserId: input.createdByUserId ?? null, - createdByRunId: input.createdByRunId ?? null, + createdByRunId, createdAt: now, }) .returning(); diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index e4c01818ee7..bbe2e200aa5 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -3679,6 +3679,22 @@ export function issueService(db: Db) { return TERMINAL_HEARTBEAT_RUN_STATUSES.has(run.status); } + // The checkout run-id columns (issues.checkout_run_id / execution_run_id) carry a + // foreign key to heartbeat_runs. An agent presents its live X-Paperclip-Run-Id, which + // may not be recorded in this server's DB (e.g. a worktree dev environment whose DB + // never saw the run). Persisting that run-id would raise a FK violation and surface as a + // 500. Resolve the value to null when the run is unknown so the assigned agent still + // gains/keeps ownership instead of crashing. In real deployments the run exists, so this + // returns the run-id unchanged. + async function persistableLockRunId(runId: string): Promise { + const run = await db + .select({ id: heartbeatRuns.id }) + .from(heartbeatRuns) + .where(eq(heartbeatRuns.id, runId)) + .then((rows) => rows[0] ?? null); + return run ? runId : null; + } + async function adoptStaleCheckoutRun(input: { issueId: string; actorAgentId: string; @@ -3689,11 +3705,12 @@ export function issueService(db: Db) { if (!stale) return null; const now = new Date(); + const lockRunId = await persistableLockRunId(input.actorRunId); const adopted = await db .update(issues) .set({ - checkoutRunId: input.actorRunId, - executionRunId: input.actorRunId, + checkoutRunId: lockRunId, + executionRunId: lockRunId, executionLockedAt: now, updatedAt: now, }) @@ -3723,11 +3740,12 @@ export function issueService(db: Db) { actorRunId: string; }) { const now = new Date(); + const lockRunId = await persistableLockRunId(input.actorRunId); const adopted = await db .update(issues) .set({ - checkoutRunId: input.actorRunId, - executionRunId: input.actorRunId, + checkoutRunId: lockRunId, + executionRunId: lockRunId, executionLockedAt: now, updatedAt: now, })