fix(annotate): persist submitted feedback before deleting the draft (#678) - #1237
Conversation
Adversarial verification found the durable record had no mode gate: an annotate-last or URL session, which was completely stateless before, would persist submitted feedback quoting the agent's message or the fetched page under history/, widening the documented annotateHistory contract without a docs change. The record now shares the exact eligibility gate the version history uses (mode annotate, non-URL path), so previously-stateless modes stay stateless. Also makes persistSubmittedDecision defensive about body types: /api/feedback does no validation (unlike /api/approve), and a non-string feedback previously flowed through settle() untouched with a 200; the new .trim() guard turned that into a thrown 500 after the decision had already settled. Malformed values now degrade to the exact legacy behavior (settle, delete draft, 200) instead of throwing. Both changes mirrored in the Pi server, with regression tests in both runtimes: stateless modes write no record, and a malformed feedback body returns 200 with the draft deleted and nothing persisted.
|
TLDR: An adversarial verification pass tried to break every user-reachable path of this PR. The core mechanism held (no data loss, no availability regressions, exit-code contract intact, Bun/Pi parity confirmed), but it proved two unintended behavior changes, both now fixed in 095f7e5. 1552 tests pass. This verification and the fixes were AI-assisted. Fixed:
Verified and could not be broken: version scans ignore the submissions/ subdir in every listing path, 50 sub-millisecond submits produce 50 distinct records, concurrent submits write exactly one record for the winner, write failures never escape as errors on an otherwise-successful submit, the record lands on disk before the awaiting CLI can observe the decision, the strict-gate exit-code contract is untouched, and vendor.sh already covers both shared modules so the published Pi package resolves cleanly. Known and accepted: a failed durable write keeps the draft, so a later identical session can show a restore banner holding already-submitted annotations. That is the intended recovery tradeoff. |
#1245) * chore: fold in pre-release QA findings (print overlays, doc alignment) From the 25-item QA sweep over the v0.26.4..main range (all items passed; these were the three real minors worth folding into the release): - The raw-HTML iframe's injected CSS had no print rules, so pin badges (and, in a narrow window, the pinpoint outline box) printed into hard copies of annotated HTML pages. The overlay elements now carry an explicit @media print hide inside the iframe document, where the outer print.css cannot reach. Inline annotation marks stay printable on purpose, matching markdown documents. - AGENTS.md/CLAUDE.md now state that PLANNOTATOR_ANNOTATE_HISTORY also gates the durable submitted-feedback records from #1237, and the Annotation interface listing includes the htmlAnchor field from #1243. - apps/codex/README.md aligns with the #1241 top-level wording (Windows Codex hooks are experimental with printed manual steps, not disabled). - Marketing docs: annotate page documents the pinpoint-first default and minimal-first chrome for raw-HTML sessions; installation page notes the old-git plain-clone fallback from #1239. * chore: drop deprecated marketing-docs edits (canonical docs are Mintlify)
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