diff --git a/apps/pi-extension/server/annotate-submission.test.ts b/apps/pi-extension/server/annotate-submission.test.ts new file mode 100644 index 000000000..9a5d5f317 --- /dev/null +++ b/apps/pi-extension/server/annotate-submission.test.ts @@ -0,0 +1,190 @@ +/** + * Annotate server (Pi/Node): durable submit records (#678) + * + * Node mirror of packages/server/annotate.test.ts's "durable submit records" + * describe block. The decision promise's consumer (the invoking agent) can + * time out before the reviewer submits; the submit then settled the promise + * with nobody listening, deleted the draft, and the feedback existed nowhere. + * These tests pin the fix in serverAnnotate.ts: a durable record is written + * to history/{project}/{slug}/submissions/ BEFORE the draft is deleted, and + * the annotate-history opt-out suppresses the record while keeping the + * legacy submit behavior. + * + * History writes go to the real ~/.plannotator data dir (generated/storage + * caches its data directory at import time — see annotate-history.test.ts), + * so each test uses a unique project namespace cleaned up in afterAll. + */ + +import { afterAll, afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { startAnnotateServer } from "./serverAnnotate.ts"; +import { deriveAnnotateHistorySlug } from "../generated/annotate-history.ts"; +import { getPlannotatorDataDir } from "../generated/data-dir.ts"; + +describe("pi annotate server: durable submit records (#678)", () => { + let savedPort: string | undefined; + let savedRemote: string | undefined; + let savedHistoryFlag: string | undefined; + + beforeEach(() => { + savedPort = process.env.PLANNOTATOR_PORT; + savedRemote = process.env.PLANNOTATOR_REMOTE; + savedHistoryFlag = process.env.PLANNOTATOR_ANNOTATE_HISTORY; + delete process.env.PLANNOTATOR_PORT; + process.env.PLANNOTATOR_REMOTE = "0"; + // Force the toggle on unless a test explicitly flips it off — a real + // ~/.plannotator/config.json must never change the outcome. + process.env.PLANNOTATOR_ANNOTATE_HISTORY = "1"; + }); + + afterEach(() => { + if (savedPort === undefined) delete process.env.PLANNOTATOR_PORT; + else process.env.PLANNOTATOR_PORT = savedPort; + if (savedRemote === undefined) delete process.env.PLANNOTATOR_REMOTE; + else process.env.PLANNOTATOR_REMOTE = savedRemote; + if (savedHistoryFlag === undefined) delete process.env.PLANNOTATOR_ANNOTATE_HISTORY; + else process.env.PLANNOTATOR_ANNOTATE_HISTORY = savedHistoryFlag; + }); + + const mintedProjects: string[] = []; + function uniqueProject(label: string): string { + const project = `_pi_annotate_submission_test_${label}_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`; + mintedProjects.push(project); + return project; + } + + afterAll(() => { + const historyDir = join(getPlannotatorDataDir(), "history"); + for (const project of mintedProjects) { + rmSync(join(historyDir, project), { recursive: true, force: true }); + } + }); + + function submissionsDir(project: string, docPath: string): string { + return join( + getPlannotatorDataDir(), + "history", + project, + deriveAnnotateHistorySlug(resolve(docPath)), + "submissions", + ); + } + + // The project name is baked into the markdown so every test gets a unique + // content-hashed draft key — drafts live in the real data dir and identical + // markdown across tests would collide on one draft file. + async function startServer(project: string, docPath: string) { + const markdown = `# Doc ${project}\n\nBody\n`; + writeFileSync(docPath, markdown, "utf-8"); + return startAnnotateServer({ + markdown, + filePath: docPath, + htmlContent: "", + project, + }); + } + + test("feedback submit writes a durable record and only then deletes the draft", async () => { + const dir = mkdtempSync(join(tmpdir(), "plannotator-pi-submit-durable-")); + const docPath = join(dir, "doc.md"); + const project = uniqueProject("feedback"); + const server = await startServer(project, docPath); + + try { + // Auto-saved draft exists before submit (the recovery copy). + const saved = await fetch(`${server.url}/api/draft`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ annotations: [{ id: "a1" }] }), + }); + expect(saved.status).toBe(200); + + const response = await fetch(`${server.url}/api/feedback`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + feedback: "## Feedback\n\nPlease fix X in the second paragraph.", + annotations: [{ id: "a1" }], + }), + }); + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ ok: true }); + + // Durable record: one markdown file next to the file's version history. + const recordDir = submissionsDir(project, docPath); + const records = readdirSync(recordDir).filter((f) => f.endsWith(".md")); + expect(records.length).toBe(1); + const content = readFileSync(join(recordDir, records[0]), "utf-8"); + expect(content).toContain("Please fix X in the second paragraph."); + expect(content).toContain("- Decision: feedback"); + expect(content).toContain(`- Source: ${resolve(docPath)}`); + + // Draft is gone AFTER the record exists. + const draft = await fetch(`${server.url}/api/draft`); + expect(draft.status).toBe(404); + } finally { + server.stop(); + rmSync(dir, { recursive: true, force: true }); + } + }); + + test("approve with notes persists a record", async () => { + const dir = mkdtempSync(join(tmpdir(), "plannotator-pi-submit-approve-")); + const docPath = join(dir, "notes.md"); + const project = uniqueProject("approve-notes"); + const server = await startServer(project, docPath); + + try { + const response = await fetch(`${server.url}/api/approve`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ feedback: "LGTM, but rename the helper.", annotations: [] }), + }); + expect(response.status).toBe(200); + const recordDir = submissionsDir(project, docPath); + const records = readdirSync(recordDir).filter((f) => f.endsWith(".md")); + expect(records.length).toBe(1); + const content = readFileSync(join(recordDir, records[0]), "utf-8"); + expect(content).toContain("LGTM, but rename the helper."); + expect(content).toContain("- Decision: approved (with notes)"); + } finally { + server.stop(); + rmSync(dir, { recursive: true, force: true }); + } + }); + + test("annotateHistory disabled: no content is written and the draft is deleted (legacy behavior)", async () => { + process.env.PLANNOTATOR_ANNOTATE_HISTORY = "0"; + const dir = mkdtempSync(join(tmpdir(), "plannotator-pi-submit-optout-")); + const docPath = join(dir, "doc.md"); + const project = uniqueProject("opt-out"); + const server = await startServer(project, docPath); + + try { + await fetch(`${server.url}/api/draft`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ annotations: [{ id: "a1" }] }), + }); + + const response = await fetch(`${server.url}/api/feedback`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ feedback: "Secret excerpt", annotations: [{ id: "a1" }] }), + }); + expect(response.status).toBe(200); + + // The opt-out means "no annotate content in the data dir": no version + // snapshot AND no submission record — the project dir never appears. + expect(existsSync(join(getPlannotatorDataDir(), "history", project))).toBe(false); + // Legacy behavior preserved: the draft is still deleted on submit. + const draft = await fetch(`${server.url}/api/draft`); + expect(draft.status).toBe(404); + } finally { + server.stop(); + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/apps/pi-extension/server/serverAnnotate.ts b/apps/pi-extension/server/serverAnnotate.ts index c3d524206..2675cbf56 100644 --- a/apps/pi-extension/server/serverAnnotate.ts +++ b/apps/pi-extension/server/serverAnnotate.ts @@ -6,7 +6,7 @@ import { randomUUID } from "node:crypto"; import { contentHash, deleteDraft } from "../generated/draft.ts"; import { getPlanVersion, getVersionCount, listVersions } from "../generated/storage.ts"; -import { computeAnnotateHistory, deriveAnnotateHistorySlug, type AnnotateHistoryResult } from "../generated/annotate-history.ts"; +import { computeAnnotateHistory, deriveAnnotateHistorySlug, persistAnnotateSubmission, type AnnotateHistoryResult } from "../generated/annotate-history.ts"; import { htmlDiff } from "../generated/html-diff.ts"; import { saveConfig, detectGitUser, getServerConfig, loadConfig, resolveAIEnabled, resolveSharingEnabled, resolveAnnotateHistory, type PromptRuntime } from "../generated/config.ts"; import { getAnnotateFileFeedbackTemplate, getAnnotateMessageFeedbackTemplate } from "../generated/prompts.ts"; @@ -324,6 +324,48 @@ export async function startAnnotateServer(options: { return result; } + // Durable submit records (#678): the caller consuming waitForDecision() may + // be gone (agent-side timeout) by the time the reviewer clicks submit — + // settling the promise then deleting the draft would leave the submitted + // feedback existing nowhere. persistAnnotateSubmission writes the record to + // {DATA_DIR}/history/{project}/{slug}/submissions/{timestamp}.md (next to + // the file's annotate version history) BEFORE the draft delete. + // + // annotateHistory opt-out policy: PLANNOTATOR_ANNOTATE_HISTORY=0 means "do + // not write annotated content to the data dir", and submitted feedback + // quotes that content, so the record is skipped and the legacy submit + // behavior (draft deleted) is preserved unchanged. A missing/timed-out + // consumer is not detectable in-process (the server cannot know its caller + // stopped reading), so there is no narrower condition to key off. + // + // Returns whether the draft delete may proceed: true when the record was + // written, when there was no user content to lose, or when the user opted + // out of persistence; false only when a durable write was expected and + // failed — the draft then stays behind as the recovery copy. + const submissionSessionPath = + options.mode === "annotate-folder" && options.folderPath + ? resolvePath(options.folderPath) + : /^https?:\/\//i.test(options.filePath) + ? options.filePath + : resolvePath(options.filePath); + const persistSubmittedDecision = ( + feedback: string, + annotations: unknown[], + approved: boolean, + ): boolean => { + if (!feedback.trim() && annotations.length === 0) return true; // contentless (e.g. bare approve) + if (!annotateHistoryEnabled) return true; // opt-out: stateless annotate sessions + return ( + persistAnnotateSubmission({ + project: annotateProjectName, + sessionPath: submissionSessionPath, + feedback, + annotations, + approved, + }) !== null + ); + }; + // Detect repo info (cached for this session) const repoInfo = getRepoInfo(); @@ -807,7 +849,14 @@ export async function startAnnotateServer(options: { sendAlreadyDecided(res); return; } - deleteDraft(draftKey, readDraftGenerationFromBody(body)); + // Approve-with-notes carries user content — make it durable before + // the draft (the reviewer's only other copy) is deleted (#678). + const approvalDurable = persistSubmittedDecision( + (body.feedback as string | undefined) || "", + (body.annotations as unknown[] | undefined) || [], + true, + ); + if (approvalDurable) deleteDraft(draftKey, readDraftGenerationFromBody(body)); clientLease.cancel(); json(res, { ok: true }); } catch (err) { @@ -826,7 +875,15 @@ export async function startAnnotateServer(options: { sendAlreadyDecided(res); return; } - deleteDraft(draftKey, readDraftGenerationFromBody(body)); + // Make the submitted feedback durable BEFORE deleting the draft: + // the decision promise's consumer may have timed out, and this + // record is then the only surviving copy (#678). + const feedbackDurable = persistSubmittedDecision( + (body.feedback as string) || "", + (body.annotations as unknown[]) || [], + false, + ); + if (feedbackDurable) deleteDraft(draftKey, readDraftGenerationFromBody(body)); clientLease.cancel(); json(res, { ok: true }); } catch (err) { diff --git a/packages/server/annotate.test.ts b/packages/server/annotate.test.ts index 354eb76ac..020a3946c 100644 --- a/packages/server/annotate.test.ts +++ b/packages/server/annotate.test.ts @@ -15,9 +15,9 @@ */ import { afterAll, afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, symlinkSync, unlinkSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, realpathSync, rmSync, symlinkSync, unlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "os"; -import { join } from "path"; +import { join, resolve } from "path"; import { startAnnotateServer } from "./annotate"; import { deriveAnnotateHistorySlug } from "@plannotator/shared/annotate-history"; import { getPlannotatorDataDir } from "@plannotator/shared/data-dir"; @@ -1419,3 +1419,236 @@ describe("annotate server: client lease", () => { expect(next.done).toBe(true); }); }); + +describe("annotate server: durable submit records (#678)", () => { + // The decision promise's consumer (the invoking CLI/agent) can time out + // before the reviewer submits; the submit then settled the promise with + // nobody listening, deleted the draft, and the feedback existed nowhere. + // These tests pin the fix: a durable record is written to + // history/{project}/{slug}/submissions/ BEFORE the draft is deleted, the + // annotate-history opt-out suppresses the record (stateless sessions keep + // legacy behavior), and a failed durable write keeps the draft behind as + // the recovery copy. + let savedPort: string | undefined; + let savedRemote: string | undefined; + let savedHistoryFlag: string | undefined; + + beforeEach(() => { + savedPort = process.env.PLANNOTATOR_PORT; + savedRemote = process.env.PLANNOTATOR_REMOTE; + savedHistoryFlag = process.env.PLANNOTATOR_ANNOTATE_HISTORY; + delete process.env.PLANNOTATOR_PORT; + process.env.PLANNOTATOR_REMOTE = "0"; + // Force the toggle on unless a test explicitly flips it off — a real + // ~/.plannotator/config.json must never change the outcome. + process.env.PLANNOTATOR_ANNOTATE_HISTORY = "1"; + }); + + afterEach(() => { + if (savedPort === undefined) delete process.env.PLANNOTATOR_PORT; + else process.env.PLANNOTATOR_PORT = savedPort; + if (savedRemote === undefined) delete process.env.PLANNOTATOR_REMOTE; + else process.env.PLANNOTATOR_REMOTE = savedRemote; + if (savedHistoryFlag === undefined) delete process.env.PLANNOTATOR_ANNOTATE_HISTORY; + else process.env.PLANNOTATOR_ANNOTATE_HISTORY = savedHistoryFlag; + }); + + // History lives in the real data dir (DATA_DIR is cached at module import), + // so each test uses a unique project namespace and afterAll removes it. + const mintedProjects: string[] = []; + function uniqueProject(label: string): string { + const project = `_annotate_submission_test_${label}_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`; + mintedProjects.push(project); + return project; + } + + afterAll(() => { + const historyDir = join(getPlannotatorDataDir(), "history"); + for (const project of mintedProjects) { + rmSync(join(historyDir, project), { recursive: true, force: true }); + } + }); + + function submissionsDir(project: string, docPath: string): string { + return join( + getPlannotatorDataDir(), + "history", + project, + deriveAnnotateHistorySlug(resolve(docPath)), + "submissions", + ); + } + + // The project name is baked into the markdown so every test gets a unique + // content-hashed draft key — drafts live in the real data dir and identical + // markdown across tests would collide on one draft file. + async function startServer(project: string, docPath: string) { + const markdown = `# Doc ${project}\n\nBody\n`; + writeFileSync(docPath, markdown, "utf-8"); + return startAnnotateServer({ + markdown, + filePath: docPath, + htmlContent: MINIMAL_HTML, + project, + }); + } + + test("feedback submit writes a durable record and only then deletes the draft", async () => { + const dir = mkdtempSync(join(tmpdir(), "plannotator-submit-durable-")); + const docPath = join(dir, "doc.md"); + const project = uniqueProject("feedback"); + const server = await startServer(project, docPath); + + try { + // Auto-saved draft exists before submit (the recovery copy). + const saved = await fetch(`${server.url}/api/draft`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ annotations: [{ id: "a1" }] }), + }); + expect(saved.status).toBe(200); + + const response = await fetch(`${server.url}/api/feedback`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + feedback: "## Feedback\n\nPlease fix X in the second paragraph.", + annotations: [{ id: "a1" }], + }), + }); + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ ok: true }); + + // Durable record: one markdown file next to the file's version history. + const recordDir = submissionsDir(project, docPath); + const records = readdirSync(recordDir).filter((f) => f.endsWith(".md")); + expect(records.length).toBe(1); + const content = readFileSync(join(recordDir, records[0]), "utf-8"); + expect(content).toContain("Please fix X in the second paragraph."); + expect(content).toContain("- Decision: feedback"); + expect(content).toContain(`- Source: ${resolve(docPath)}`); + + // Draft is gone AFTER the record exists. + const draft = await fetch(`${server.url}/api/draft`); + expect(draft.status).toBe(404); + } finally { + server.stop(); + rmSync(dir, { recursive: true, force: true }); + } + }); + + test("approve with notes persists a record; a bare approve writes nothing", async () => { + const dir = mkdtempSync(join(tmpdir(), "plannotator-submit-approve-")); + + // Approve-with-notes carries user content -> record. + const notesDoc = join(dir, "notes.md"); + const notesProject = uniqueProject("approve-notes"); + const notesServer = await startServer(notesProject, notesDoc); + try { + const response = await fetch(`${notesServer.url}/api/approve`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ feedback: "LGTM, but rename the helper.", annotations: [] }), + }); + expect(response.status).toBe(200); + const recordDir = submissionsDir(notesProject, notesDoc); + const records = readdirSync(recordDir).filter((f) => f.endsWith(".md")); + expect(records.length).toBe(1); + const content = readFileSync(join(recordDir, records[0]), "utf-8"); + expect(content).toContain("LGTM, but rename the helper."); + expect(content).toContain("- Decision: approved (with notes)"); + } finally { + notesServer.stop(); + } + + // Bare approve is contentless -> nothing to persist. + const bareDoc = join(dir, "bare.md"); + const bareProject = uniqueProject("approve-bare"); + const bareServer = await startServer(bareProject, bareDoc); + try { + const response = await fetch(`${bareServer.url}/api/approve`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({}), + }); + expect(response.status).toBe(200); + expect(existsSync(submissionsDir(bareProject, bareDoc))).toBe(false); + } finally { + bareServer.stop(); + rmSync(dir, { recursive: true, force: true }); + } + }); + + test("annotateHistory disabled: no content is written and the draft is deleted (legacy behavior)", async () => { + process.env.PLANNOTATOR_ANNOTATE_HISTORY = "0"; + const dir = mkdtempSync(join(tmpdir(), "plannotator-submit-optout-")); + const docPath = join(dir, "doc.md"); + const project = uniqueProject("opt-out"); + const server = await startServer(project, docPath); + + try { + await fetch(`${server.url}/api/draft`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ annotations: [{ id: "a1" }] }), + }); + + const response = await fetch(`${server.url}/api/feedback`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ feedback: "Secret excerpt", annotations: [{ id: "a1" }] }), + }); + expect(response.status).toBe(200); + + // The opt-out means "no annotate content in the data dir": no version + // snapshot AND no submission record — the project dir never appears. + expect(existsSync(join(getPlannotatorDataDir(), "history", project))).toBe(false); + // Legacy behavior preserved: the draft is still deleted on submit. + const draft = await fetch(`${server.url}/api/draft`); + expect(draft.status).toBe(404); + } finally { + server.stop(); + rmSync(dir, { recursive: true, force: true }); + } + }); + + test("a failed durable write keeps the draft as the recovery copy", async () => { + const dir = mkdtempSync(join(tmpdir(), "plannotator-submit-unwritable-")); + const docPath = join(dir, "doc.md"); + const project = uniqueProject("unwritable"); + // Plant a FILE where the project's history directory must go: every + // mkdir under it fails, so both the startup snapshot and the submission + // write degrade. (afterAll's recursive+force rm removes the file too.) + const historyRoot = join(getPlannotatorDataDir(), "history"); + mkdirSync(historyRoot, { recursive: true }); + writeFileSync(join(historyRoot, project), "not a directory", "utf-8"); + const server = await startServer(project, docPath); + + try { + await fetch(`${server.url}/api/draft`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ annotations: [{ id: "a1" }] }), + }); + + const response = await fetch(`${server.url}/api/feedback`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ feedback: "Please fix X", annotations: [{ id: "a1" }] }), + }); + // The decision itself still succeeds — persistence is an enhancement. + expect(response.status).toBe(200); + + // But the draft survives: with no durable record written, it is the + // only remaining copy of the reviewer's work. + const draft = await fetch(`${server.url}/api/draft`); + expect(draft.status).toBe(200); + + // Cleanup: don't leave this test's draft behind in the real data dir. + await fetch(`${server.url}/api/draft`, { method: "DELETE" }); + } finally { + server.stop(); + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/server/annotate.ts b/packages/server/annotate.ts index fb1b9c5c3..d6aa14f4c 100644 --- a/packages/server/annotate.ts +++ b/packages/server/annotate.ts @@ -20,7 +20,7 @@ import { handleFileBrowserFilesStream } from "./reference-watch"; import { resolveUserPath, warmFileListCache } from "@plannotator/shared/resolve-file"; import { contentHash, deleteDraft } from "./draft"; import { getPlanVersion, getVersionCount, listVersions } from "@plannotator/shared/storage"; -import { computeAnnotateHistory, deriveAnnotateHistorySlug, type AnnotateHistoryResult } from "@plannotator/shared/annotate-history"; +import { computeAnnotateHistory, deriveAnnotateHistorySlug, persistAnnotateSubmission, type AnnotateHistoryResult } from "@plannotator/shared/annotate-history"; import { htmlDiff } from "@plannotator/shared/html-diff"; import { disabledSourceSave, type SourceSaveRequest } from "@plannotator/shared/source-save"; import { getAnnotateReferenceRootPaths } from "@plannotator/shared/annotate-reference-roots-node"; @@ -234,6 +234,48 @@ export async function startAnnotateServer( ? `folder:${resolvePath(folderPath)}` : renderHtml && rawHtml ? rawHtml : markdown; const draftKey = contentHash(draftSource); + + // Durable submit records (#678): the caller consuming waitForDecision() may + // be gone (agent-side timeout) by the time the reviewer clicks submit — + // settling the promise then deleting the draft would leave the submitted + // feedback existing nowhere. persistAnnotateSubmission writes the record to + // {DATA_DIR}/history/{project}/{slug}/submissions/{timestamp}.md (next to + // the file's annotate version history) BEFORE the draft delete. + // + // annotateHistory opt-out policy: PLANNOTATOR_ANNOTATE_HISTORY=0 means "do + // not write annotated content to the data dir", and submitted feedback + // quotes that content, so the record is skipped and the legacy submit + // behavior (draft deleted) is preserved unchanged. A missing/timed-out + // consumer is not detectable in-process (the server cannot know its caller + // stopped reading), so there is no narrower condition to key off. + // + // Returns whether the draft delete may proceed: true when the record was + // written, when there was no user content to lose, or when the user opted + // out of persistence; false only when a durable write was expected and + // failed — the draft then stays behind as the recovery copy. + const submissionSessionPath = + mode === "annotate-folder" && folderPath + ? resolvePath(folderPath) + : /^https?:\/\//i.test(filePath) + ? filePath + : resolvePath(filePath); + const persistSubmittedDecision = ( + feedback: string, + annotations: unknown[], + approved: boolean, + ): boolean => { + if (!feedback.trim() && annotations.length === 0) return true; // contentless (e.g. bare approve) + if (!annotateHistoryEnabled) return true; // opt-out: stateless annotate sessions + return ( + persistAnnotateSubmission({ + project: annotateProjectName, + sessionPath: submissionSessionPath, + feedback, + annotations, + approved, + }) !== null + ); + }; const externalAnnotations = createExternalAnnotationHandler("plan"); const aiRuntime = resolveAIEnabled() ? await createAIRuntime() : null; const htmlAssets = createHtmlAssetRegistry(); @@ -845,7 +887,14 @@ export async function startAnnotateServer( : undefined, }); if (!approvalWon) return alreadyDecided(); - deleteDraft(draftKey, readDraftGenerationFromBody(body)); + // Approve-with-notes carries user content — make it durable before + // the draft (the reviewer's only other copy) is deleted (#678). + const approvalDurable = persistSubmittedDecision( + (body.feedback as string | undefined) || "", + (body.annotations as unknown[] | undefined) || [], + true, + ); + if (approvalDurable) deleteDraft(draftKey, readDraftGenerationFromBody(body)); clientLease.cancel(); return Response.json({ ok: true }); } @@ -868,7 +917,15 @@ export async function startAnnotateServer( feedbackScope: body.feedbackScope, }); if (!feedbackWon) return alreadyDecided(); - deleteDraft(draftKey, readDraftGenerationFromBody(body)); + // Make the submitted feedback durable BEFORE deleting the draft: + // the decision promise's consumer may have timed out, and this + // record is then the only surviving copy (#678). + const feedbackDurable = persistSubmittedDecision( + body.feedback || "", + body.annotations || [], + false, + ); + if (feedbackDurable) deleteDraft(draftKey, readDraftGenerationFromBody(body)); clientLease.cancel(); return Response.json({ ok: true }); diff --git a/packages/shared/annotate-history.ts b/packages/shared/annotate-history.ts index 05f0c70f5..77a2ab023 100644 --- a/packages/shared/annotate-history.ts +++ b/packages/shared/annotate-history.ts @@ -21,7 +21,7 @@ * non-Bun runtimes can vendor it unmodified. */ -import { saveToHistory, getPlanVersion, getVersionCount } from "./storage"; +import { saveToHistory, getPlanVersion, getVersionCount, saveAnnotateSubmission } from "./storage"; import { contentHash } from "./draft"; export interface AnnotateVersionInfo { @@ -94,3 +94,69 @@ export function computeAnnotateHistory( return null; } } + +// --- Durable submit records (#678) --- + +export interface AnnotateSubmissionInput { + /** Project namespace, same one used for the session's version history. */ + project: string; + /** + * Stable identity of what was annotated: the resolved file path for + * single-file sessions, the resolved folder path for folder sessions, the + * URL for URL sessions, or the session's filePath label for message + * sessions. Runs through deriveAnnotateHistorySlug, so single-file records + * land in the SAME history slug directory as the file's version snapshots. + */ + sessionPath: string; + /** Exported human-readable feedback text (what the agent would receive). */ + feedback: string; + /** Raw annotations payload from the submit body. */ + annotations: unknown[]; + /** True for approve-with-notes, false/absent for plain feedback. */ + approved?: boolean; +} + +/** + * Persist a durable record of a submitted annotate decision BEFORE the + * reviewer's draft is deleted (#678). + * + * The annotate decision promise's consumer is the invoking CLI/agent, which + * may have timed out by the time the reviewer clicks submit. Without this + * record, a successful submit settles the promise (nobody listening), deletes + * the draft, and the feedback then exists nowhere. The record is written to + * `{DATA_DIR}/history/{project}/{slug}/submissions/{timestamp}.md`, alongside + * the file's annotate version history. + * + * Never throws: any storage failure is logged and `null` is returned so the + * caller can react (the servers keep the draft as the recovery copy when this + * returns null). + */ +export function persistAnnotateSubmission(input: AnnotateSubmissionInput): string | null { + try { + const slug = deriveAnnotateHistorySlug(input.sessionPath); + // The exported feedback text already embeds every annotation in + // human-readable form; the raw annotations JSON is only recorded when + // there is no text to fall back on (defensive — the UI always exports). + const body = input.feedback.trim() + ? input.feedback + : "```json\n" + JSON.stringify(input.annotations, null, 2) + "\n```"; + const content = [ + "# Annotate feedback", + "", + `- Source: ${input.sessionPath}`, + `- Decision: ${input.approved ? "approved (with notes)" : "feedback"}`, + `- Submitted: ${new Date().toISOString()}`, + "", + "---", + "", + body, + "", + ].join("\n"); + return saveAnnotateSubmission(input.project, slug, content); + } catch (error) { + console.error( + `[plannotator] warning: could not persist submitted annotate feedback (${error instanceof Error ? error.message : String(error)}); keeping the annotation draft as the recovery copy`, + ); + return null; + } +} diff --git a/packages/shared/storage.ts b/packages/shared/storage.ts index 431198d93..cc4e12f66 100644 --- a/packages/shared/storage.ts +++ b/packages/shared/storage.ts @@ -253,6 +253,38 @@ export function saveToHistory( return { version: nextVersion, path: filePath, isNew: true }; } +/** + * Save a durable record of submitted annotate feedback (#678). + * + * Annotate submissions settle a decision promise whose consumer (the invoking + * CLI/agent) may have timed out and stopped listening. The plan flow persists + * its decisions via saveFinalSnapshot; annotate had no equivalent, so a submit + * whose caller was gone deleted the draft and left the feedback nowhere. This + * writes the record next to the file's annotate version history: + * + * {DATA_DIR}/history/{project}/{slug}/submissions/{timestamp}.md + * + * The `submissions/` subdirectory keeps these out of the numeric NNN.md + * version scans above (getNextVersionNumber / listVersions / listProjectPlans + * all match files directly in the slug directory only). Filenames are + * filesystem-safe ISO timestamps (colons/dots replaced) with a collision + * counter so rapid successive submits never overwrite each other. + * + * Returns the full path to the saved file. Throws on write failure — callers + * (persistAnnotateSubmission) catch and degrade. + */ +export function saveAnnotateSubmission(project: string, slug: string, content: string): string { + const submissionsDir = join(getHistoryDir(project, slug), "submissions"); + mkdirSync(submissionsDir, { recursive: true }); + const stamp = new Date().toISOString().replace(/[:.]/g, "-"); + let filePath = join(submissionsDir, `${stamp}.md`); + for (let n = 2; existsSync(filePath); n++) { + filePath = join(submissionsDir, `${stamp}-${n}.md`); + } + writeFileSync(filePath, content, "utf-8"); + return filePath; +} + /** * Read a specific version's content from history. * Returns null if the version doesn't exist or on read error.