Skip to content

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

Open
backnotprop wants to merge 1 commit into
mainfrom
fix/annotate-submit-durable
Open

fix(annotate): persist submitted feedback before deleting the draft (#678)#1237
backnotprop wants to merge 1 commit into
mainfrom
fix/annotate-submit-durable

Conversation

@backnotprop

Copy link
Copy Markdown
Owner

TLDR

An annotate submit could silently lose all feedback when the invoking CLI/agent had already timed out: the server settled the decision promise with nobody listening, deleted the draft, and the submitted annotations then existed nowhere. Both runtimes now write a durable record of the submitted feedback to ~/.plannotator/history/{project}/{slug}/submissions/{timestamp}.md before the draft is deleted. If that write fails, the draft is kept as the recovery copy. With annotate history disabled, nothing new is written and the legacy submit behavior is preserved unchanged.

The loss window

plannotator annotate hands the browser's decision back through a promise the invoking CLI/agent awaits. When the agent-side shell command times out (the Codex-on-Windows repro in #678), the user can keep reviewing in the still-open browser and click Send Feedback. The server then:

  1. settles the decision promise (nobody is listening anymore),
  2. deletes the annotation draft,
  3. responds OK and the page closes.

At that point the feedback exists nowhere: not in drafts (just deleted), not in annotate history (that stores versions of the source file, not annotations), not anywhere. The plan server does not have this hole because it persists a decision snapshot on approve/deny (saveAnnotations / saveFinalSnapshot); the annotate server had no equivalent. Adjacent work does not cover it either: #1091's --result-file applies only to strict --gate --json invocations, and #1143 preserves the draft on abandonment, not on a successful submit.

The fix

New shared machinery, reusing the existing history storage layout rather than inventing a parallel scheme:

  • saveAnnotateSubmission() in packages/shared/storage.ts: writes one markdown file per submit to {DATA_DIR}/history/{project}/{slug}/submissions/{timestamp}.md, right next to the file's annotate version history (slug is the same deriveAnnotateHistorySlug slug the version snapshots use). The submissions/ subdirectory keeps records out of the numeric NNN.md version scans, and filenames carry a collision counter so rapid submits never overwrite.
  • persistAnnotateSubmission() in packages/shared/annotate-history.ts: composes the record (source path, decision kind, timestamp, and the exported feedback text, which already embeds every annotation in human-readable form; raw annotations JSON only as a defensive fallback when the text is empty). Never throws; returns null on storage failure.

Both annotate servers (Bun packages/server/annotate.ts and Pi apps/pi-extension/server/serverAnnotate.ts, re-vendored via vendor.sh) wire it into /api/feedback and /api/approve after the decision settler wins and before deleteDraft. Ordering matters twice:

  • persist happens after decision.settle() wins, so a 409 losing producer never writes a phantom record, and
  • deleteDraft only runs when the record was written (or persistence was legitimately skipped). If the durable write fails, the draft stays behind as the only remaining copy of the reviewer's work; the decision itself still succeeds because persistence is an enhancement, never a gate.

/api/approve is not contentless: approve-with-notes carries feedback/annotations, so it persists under the same rule. A bare approve carries no user content and writes nothing. /api/exit is untouched.

annotateHistory-disabled policy

PLANNOTATOR_ANNOTATE_HISTORY=0 / { "annotateHistory": false } means "do not write copies of annotated content to the data dir", and submitted feedback quotes that content (annotation originalText excerpts). So with history disabled, no submission record is written, and the submit path behaves exactly as before (draft deleted, response OK). The task's alternative of skipping the draft delete only when the decision promise has no live consumer is not implementable: the server cannot detect in-process that its caller stopped reading the decision. And keeping the draft on every opted-out submit would resurface already-delivered feedback on the next session for the same content, which contradicts the normal submit contract everywhere else (plan server included). #1143's precedent keeps the draft only when no decision was delivered (abandonment); here a decision was made, so the opt-out user gets the pre-existing, explicitly chosen stateless behavior. The failed-write path (history enabled but the data dir is unwritable) is the one place the draft is deliberately kept, since the durable record was expected and did not happen.

Recovery discoverability

plannotator sessions lists live server processes from ~/.plannotator/sessions/{pid}.json, so the new records are not discoverable from it (and per scope, no new CLI surface is added here). Records are plain markdown under ~/.plannotator/history/{project}/{slug}/submissions/ and sit next to the version snapshots for the same file.

Tests

  • Bun (packages/server/annotate.test.ts, new describe): record written and draft gone after /api/feedback; approve-with-notes persists while bare approve writes nothing; disabled history writes no content while still deleting the draft; a failed durable write keeps the draft.
  • Pi (apps/pi-extension/server/annotate-submission.test.ts): mirrors the feedback, approve-with-notes, and disabled-history cases against the Node server.
  • bun test packages/server packages/shared: 1362 pass, 0 fail (82 files).
  • bun test apps/pi-extension: 186 pass, 0 fail (21 files).
  • bun run typecheck (includes vendor.sh): clean.

Closes #678

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Annotate feedback can be lost when submit auto-closes localhost session after CLI timeout on Windows/Codex

1 participant