Skip to content

fix(annotate): persist submitted feedback before deleting the draft (#678) #1237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
backnotprop wants to merge 1 commit into
base: main
Choose a base branch
from
+644 −9
Open

fix(annotate): persist submitted feedback before deleting the draft (#678) #1237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 190 additions & 0 deletions apps/pi-extension/server/annotate-submission.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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: "<html></html>",
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 });
}
});
});
63 changes: 60 additions & 3 deletions apps/pi-extension/server/serverAnnotate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
Loading