From cd5b73563162d8486e0ac6f4d92a98a3dc7135c8 Mon Sep 17 00:00:00 2001 From: Dotta Date: Thu, 14 May 2026 21:23:29 -0500 Subject: [PATCH] Harden document annotation revision guards --- .../document-annotation-routes.test.ts | 25 ++- .../document-annotations-service.test.ts | 183 ++++++++++++++++++ server/src/routes/issues.ts | 6 +- server/src/services/document-annotations.ts | 9 +- 4 files changed, 219 insertions(+), 4 deletions(-) create mode 100644 server/src/__tests__/document-annotations-service.test.ts diff --git a/server/src/__tests__/document-annotation-routes.test.ts b/server/src/__tests__/document-annotation-routes.test.ts index c23f1f973aa..86c9d687c1f 100644 --- a/server/src/__tests__/document-annotation-routes.test.ts +++ b/server/src/__tests__/document-annotation-routes.test.ts @@ -187,7 +187,15 @@ describe("document annotation routes", () => { }); mockIssueService.assertCheckoutOwner.mockResolvedValue({}); mockDocumentService.getIssueDocumentByKey.mockResolvedValue(documentPayload); - mockAnnotationService.listThreadsForIssueDocument.mockResolvedValue([{ ...annotationThread, comments: [annotationComment] }]); + mockAnnotationService.listThreadsForIssueDocument.mockImplementation(async ( + _issueId: string, + _key: string, + options?: { includeComments?: boolean }, + ) => ( + options?.includeComments + ? [{ ...annotationThread, comments: [annotationComment] }] + : [annotationThread] + )); mockAnnotationService.getThreadForIssueDocument.mockResolvedValue({ ...annotationThread, comments: [annotationComment] }); mockAnnotationService.createThread.mockResolvedValue({ ...annotationThread, comments: [annotationComment] }); mockAnnotationService.addComment.mockResolvedValue(annotationComment); @@ -195,12 +203,25 @@ describe("document annotation routes", () => { mockAnnotationService.remapOpenThreadsForDocument.mockResolvedValue([]); }); - it("includes compact open annotations by default for agent document reads", async () => { + it("includes compact open annotations without comment bodies by default for agent document reads", async () => { const res = await request(await createApp("agent")) .get(`/api/issues/${issueId}/documents/plan`) .expect(200); expect(res.body.annotations).toHaveLength(1); + expect(res.body.annotations[0].comments).toBeUndefined(); + expect(mockAnnotationService.listThreadsForIssueDocument).toHaveBeenCalledWith(issueId, "plan", { + status: "open", + includeComments: false, + }); + }); + + it("includes annotation comment bodies on document reads only when explicitly requested", async () => { + const res = await request(await createApp("agent")) + .get(`/api/issues/${issueId}/documents/plan?includeAnnotationComments=true`) + .expect(200); + + expect(res.body.annotations[0].comments[0].body).toBe("Please review PAP-1"); expect(mockAnnotationService.listThreadsForIssueDocument).toHaveBeenCalledWith(issueId, "plan", { status: "open", includeComments: true, diff --git a/server/src/__tests__/document-annotations-service.test.ts b/server/src/__tests__/document-annotations-service.test.ts new file mode 100644 index 00000000000..ff6a23d7f3e --- /dev/null +++ b/server/src/__tests__/document-annotations-service.test.ts @@ -0,0 +1,183 @@ +import { randomUUID } from "node:crypto"; +import { eq } from "drizzle-orm"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { + companies, + createDb, + documentAnnotationAnchorSnapshots, + documentAnnotationComments, + documentAnnotationThreads, + documentRevisions, + documents, + issueDocuments, + issues, +} from "@paperclipai/db"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { documentAnnotationService } from "../services/document-annotations.js"; +import { documentService } from "../services/documents.js"; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +if (!embeddedPostgresSupport.supported) { + console.warn( + `Skipping embedded Postgres document annotation service tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`, + ); +} + +function deferred() { + let resolve!: (value: T | PromiseLike) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((promiseResolve, promiseReject) => { + resolve = promiseResolve; + reject = promiseReject; + }); + return { promise, resolve, reject }; +} + +describeEmbeddedPostgres("documentAnnotationService", () => { + let db!: ReturnType; + let annotations!: ReturnType; + let docs!: ReturnType; + let tempDb: Awaited> | null = null; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-document-annotations-"); + db = createDb(tempDb.connectionString); + annotations = documentAnnotationService(db); + docs = documentService(db); + }, 20_000); + + afterEach(async () => { + await db.delete(documentAnnotationAnchorSnapshots); + await db.delete(documentAnnotationComments); + await db.delete(documentAnnotationThreads); + await db.delete(documentRevisions); + await db.delete(issueDocuments); + await db.delete(documents); + await db.delete(issues); + await db.delete(companies); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + async function createIssueWithDocument() { + const companyId = 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(issues).values({ + id: issueId, + companyId, + identifier: "PAP-9442", + title: "Annotation race", + description: "Validate annotation revision guards", + status: "in_progress", + priority: "high", + }); + + const created = await docs.upsertIssueDocument({ + issueId, + key: "plan", + title: "Plan", + format: "markdown", + body: "Alpha selected text omega", + }); + + return { companyId, issueId, document: created.document }; + } + + it("fails closed when a concurrent document update wins before annotation thread creation commits", async () => { + const { companyId, issueId, document } = await createIssueWithDocument(); + const concurrentUpdateCanCommit = deferred(); + const concurrentUpdateHasWritten = deferred(); + + const concurrentUpdate = db.transaction(async (tx) => { + const now = new Date(); + const [revision] = await tx + .insert(documentRevisions) + .values({ + companyId, + documentId: document.id, + revisionNumber: document.latestRevisionNumber + 1, + title: "Plan", + format: "markdown", + body: "Alpha changed text omega", + changeSummary: "Concurrent edit", + createdAt: now, + }) + .returning(); + + await tx + .update(documents) + .set({ + latestBody: "Alpha changed text omega", + latestRevisionId: revision.id, + latestRevisionNumber: document.latestRevisionNumber + 1, + updatedAt: now, + }) + .where(eq(documents.id, document.id)); + + concurrentUpdateHasWritten.resolve(); + await concurrentUpdateCanCommit.promise; + }); + + await concurrentUpdateHasWritten.promise; + + let annotationSettled = false; + const annotationResult = annotations + .createThread( + issueId, + "plan", + { + baseRevisionId: document.latestRevisionId!, + baseRevisionNumber: document.latestRevisionNumber, + selector: { + quote: { exact: "selected text", prefix: "Alpha ", suffix: " omega" }, + position: { normalizedStart: 6, normalizedEnd: 19, markdownStart: 6, markdownEnd: 19 }, + }, + body: "Please review this text", + }, + { actorType: "user", actorId: "board-user", userId: "board-user" }, + ) + .then( + () => ({ status: "fulfilled" as const }), + (error: unknown) => ({ status: "rejected" as const, error }), + ) + .finally(() => { + annotationSettled = true; + }); + + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(annotationSettled).toBe(false); + + concurrentUpdateCanCommit.resolve(); + await concurrentUpdate; + + const result = await annotationResult; + expect(result.status).toBe("rejected"); + if (result.status === "rejected") { + expect(result.error).toMatchObject({ + status: 409, + message: "Annotation anchor requires the current document revision", + details: { + currentRevisionNumber: 2, + }, + }); + } + + const threads = await db.select().from(documentAnnotationThreads); + expect(threads).toHaveLength(0); + }); +}); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index fb7376a91da..49cbd611e6c 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -1116,6 +1116,10 @@ export function issueRoutes( return req.actor.type === "agent" || parseBooleanQuery(req.query.includeAnnotations); } + function shouldIncludeDocumentAnnotationComments(req: Request) { + return parseBooleanQuery(req.query.includeAnnotationComments); + } + function annotationActorInput(req: Request) { const actor = getActorInfo(req); return { @@ -2518,7 +2522,7 @@ export function issueRoutes( } const annotations = await documentAnnotationsSvc.listThreadsForIssueDocument(issue.id, keyParsed.data, { status: "open", - includeComments: true, + includeComments: shouldIncludeDocumentAnnotationComments(req), }); res.json({ ...doc, annotations }); }); diff --git a/server/src/services/document-annotations.ts b/server/src/services/document-annotations.ts index 4ee69c95852..a308e19e067 100644 --- a/server/src/services/document-annotations.ts +++ b/server/src/services/document-annotations.ts @@ -1,4 +1,4 @@ -import { and, asc, desc, eq, inArray } from "drizzle-orm"; +import { and, asc, desc, eq, inArray, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { documentAnnotationAnchorSnapshots, @@ -187,6 +187,13 @@ export function documentAnnotationService(db: Db) { input: CreateDocumentAnnotationThread, actor: ActorInput, ) => db.transaction(async (tx) => { + await tx.execute(sql` + select ${documents.id} + from ${issueDocuments} + inner join ${documents} on ${issueDocuments.documentId} = ${documents.id} + where ${and(eq(issueDocuments.issueId, issueId), eq(issueDocuments.key, key))} + for update of ${documents} + `); const doc = await getIssueDocument(issueId, key, tx); if (!doc) throw notFound("Document not found"); if (