mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-07 16:35:48 +01:00
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 <noreply@anthropic.com>
This commit is contained in:
1 parent
eac2752227
commit
7df36ef615
4 files changed
+265
-9
No files matched your search
@@ -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<typeof createDb>;
|
||||
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | 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 });
|
||||
});
|
||||
});
|
||||
@@ -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<string, unknown> | 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<string | null> {
|
||||
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,
|
||||
});
|
||||
|
||||
|
||||
@@ -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<string | null> {
|
||||
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();
|
||||
|
||||
@@ -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<string | 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;
|
||||
}
|
||||
|
||||
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,
|
||||
})
|
||||
|
||||
Reference in new issue
Block a user