Harden document annotation revision guards

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,

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<T>() {
+  let resolve!: (value: T | PromiseLike<T>) => void;
+  let reject!: (reason?: unknown) => void;
+  const promise = new Promise<T>((promiseResolve, promiseReject) => {
+    resolve = promiseResolve;
+    reject = promiseReject;
+  });
+  return { promise, resolve, reject };
+}
+
+describeEmbeddedPostgres("documentAnnotationService", () => {
+  let db!: ReturnType<typeof createDb>;
+  let annotations!: ReturnType<typeof documentAnnotationService>;
+  let docs!: ReturnType<typeof documentService>;
+  let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | 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<void>();
+    const concurrentUpdateHasWritten = deferred<void>();
+
+    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);
+  });
+});

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 });
   });

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 (