fix(annotate): persist submitted feedback before deleting the draft (#678) - #1237
Open
backnotprop wants to merge 1 commit into
Open
fix(annotate): persist submitted feedback before deleting the draft (#678)#1237backnotprop wants to merge 1 commit into
backnotprop wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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}.mdbefore 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 annotatehands 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: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-fileapplies only to strict--gate --jsoninvocations, 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()inpackages/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 (slugis the samederiveAnnotateHistorySlugslug the version snapshots use). Thesubmissions/subdirectory keeps records out of the numericNNN.mdversion scans, and filenames carry a collision counter so rapid submits never overwrite.persistAnnotateSubmission()inpackages/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.tsand Piapps/pi-extension/server/serverAnnotate.ts, re-vendored viavendor.sh) wire it into/api/feedbackand/api/approveafter the decision settler wins and beforedeleteDraft. Ordering matters twice:decision.settle()wins, so a 409 losing producer never writes a phantom record, anddeleteDraftonly 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/approveis not contentless: approve-with-notes carriesfeedback/annotations, so it persists under the same rule. A bare approve carries no user content and writes nothing./api/exitis 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 (annotationoriginalTextexcerpts). 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 sessionslists 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
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.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(includesvendor.sh): clean.Closes #678