Skip to content

feat(server): durable feedback archive for every submitted review - #1438

Merged
backnotprop merged 3 commits into
mainfrom
feat/feedback-archive
Sep 1, 2026
Merged

backnotprop merged 3 commits into
mainfrom
feat/feedback-archive

Conversation

@backnotprop

Copy link
Copy Markdown
Owner

What this does

Plannotator hands submitted feedback to the invoking agent and then forgets it. This adds a durable local archive of every review a user submits, on every surface, in both server runtimes.

The gaps this closes:

  • Code review persisted nothing. POST /api/feedback deleted the draft, settled the decision promise, and wrote no record. If the invoking agent had already timed out, the review existed nowhere. That is exactly the failure Annotate feedback can be lost when submit auto-closes localhost session after CLI timeout on Windows/Codex #678 fixed for annotate.
  • Plan decisions depended on a user setting. plans/ snapshots are written only while the client-sent planSave setting is on, and they are keyed by slug plus status, so approve then deny then approve on one plan keeps one file per status instead of a timeline.
  • Annotate only covered single local files. URL, folder, live-app and annotate-last submissions wrote nothing.

Storage

${PLANNOTATOR_DATA_DIR}/feedback/{project}/
  index.jsonl                                            # append-only, authoritative, schema v1
  records/2026-08-31T14-22-07-511Z-review-feedback.md    # human-readable sidecar

The JSONL line is self-contained so an analyzer never has to open a sidecar (jq over index.jsonl); the markdown sidecar exists because the rest of the data dir is greppable markdown, and it is written only for records that carry content. Bare approvals, LGTMs and dismissals are decision-only lines.

Sample record (code review):

{"v":1,"ts":"2026-08-31T14:22:07.511Z","client":"plannotator","project":"plannotator","origin":"claude-code","surface":"review","decision":"feedback","target":{"review":{"diffType":"since-base","base":"origin/main","gitRef":"HEAD","snapshotId":"a1b2c3:since-base","cwd":"/abs/repo","vcsType":"git","changedFiles":14,"patchBytes":48211}},"feedback":"## Review\n\nparse() still drops the null case.","annotations":[{"id":"a1","type":"comment","text":"null case","originalText":"parse(input)","file":"src/parse.ts","lineStart":12,"lineEnd":12,"side":"new","author":"ramos"}],"counts":{"annotations":1,"external":0,"images":0},"recordFile":"records/2026-08-31T14-22-07-511Z-review-feedback.md"}

Records are cheap by design:

  • Code review carries diff identity (vcsType, diffType, base, gitRef, snapshotId, cwd, PR metadata, changed-file count, patch byte count) and never the patch bytes. Guide history already showed what uncapped patch copies cost on disk.
  • Plan records carry the decision text plus a reference to the exact history/{project}/{slug}/NNN.md version the decision was made on, so an analyzer joins the record to the plan content already on disk instead of storing a second copy.
  • Annotation provenance (source, author) is preserved. External, review-agent and WebMCP annotations are included, because the submitted feedback text already embeds them, but they stay tagged, so source == null selects the reviewer's own comments. Agent job outputs (guides, tours) are not archived.

Where it is wired

One shared module, packages/shared/feedback-archive.ts, vendored to Pi through vendor.sh. It resolves the data dir per call (not at module load) so tests can redirect PLANNOTATOR_DATA_DIR inside the test body under Bun's one-process rule, and it never throws: a failure is logged, degrades silently for the user, and keeps the annotation draft as the recovery copy. The append happens before deleteDraft, generalizing the #678 ordering.

Surface Bun Pi
Plan approve / deny packages/server/index.ts serverPlan.ts
Review feedback / approve / LGTM / close packages/server/review.ts serverReview.ts
Annotate submit / approve / close packages/server/annotate.ts serverAnnotate.ts

Controls

  • PLANNOTATOR_FEEDBACK_HISTORY env plus feedbackHistory config key, resolver beside resolveAnnotateHistory, default on.
  • PLANNOTATOR_ANNOTATE_HISTORY=0 continues to suppress all annotate-surface writes including the new records, so the documented fully-stateless annotate session stays literally true.
  • "feedback" is added to PURGE_OWNED_TOP_LEVEL, so uninstall purge removes it and dry-run does not report it as an unrecognized leftover.
  • No retention or pruning, matching plans/, history/ and guides/. The privacy note (feedback text and quoted excerpts land on disk, kept indefinitely) is documented in the env table.

Behavior change worth release-noting

With defaults, URL, annotate-last, live-app and folder annotate submissions leave a durable record for the first time, and code review leaves one at all for the first time. Legacy stores are untouched: plans/ snapshots and the #678 history/.../submissions/ records behave exactly as before.

Deferred follow-ups (named, not in this PR)

  1. Structured plan annotations. The plan client submits only rendered feedback text, so plan records are text plus the version-file reference. Capturing per-annotation structure needs a client payload addition that touches the UI and both built HTMLs; it is additive and can land later without changing the record schema.
  2. POST /api/pr-action records (platform review submissions), an additive sixth severable increment from the design.
  3. Read-path increments: a plannotator feedback list CLI reader, Archive-tab integration, image attachment copying, and opt-in full-patch retention.

Tests

New: packages/shared/feedback-archive.test.ts (record shape, append durability, sidecar collisions, torn-line tolerance, never-throws, project bucketing), packages/server/feedback-archive.test.ts (all three Bun surfaces end to end over HTTP), apps/pi-extension/server/feedback-archive.test.ts (Pi parity). packages/server/uninstall.test.ts gains the purge fixture. Each test names the regression it guards; all writes are sandboxed under a temp PLANNOTATOR_DATA_DIR set inside the test body.

No UI changes, no new endpoints, no client changes, no build-order implications.

AI-assisted (Claude) under maintainer direction.

…ack store

Submitted feedback was only as durable as the agent session that asked for
it. Code review persisted nothing at all: /api/feedback deleted the draft,
settled the decision promise, and if the invoking agent had already timed
out the review existed nowhere (the failure #678 fixed for annotate). Plan
decisions only reached plans/ while the client-side planSave setting was on,
and repeat decisions on one plan overwrote each other. Annotate kept the
#678 record for single local files only.

Every submission now appends one record to
${PLANNOTATOR_DATA_DIR}/feedback/{project}/index.jsonl, plus a
records/{stamp}-{surface}-{decision}.md sidecar when it carries content,
written at decision settlement time inside the servers so all nine agent
frontends are covered by two implementations.

Surfaces wired in both runtimes: plan approve and deny, code review
/api/feedback (Send Feedback, Approve, LGTM) and /api/exit, annotate submit,
approve and exit. Bare approvals, LGTMs and dismissals are decision-only
JSONL lines with no sidecar.

Records are cheap by design. Code review carries diff identity (vcsType,
diffType, base, gitRef, snapshotId, cwd, PR metadata, changed-file count,
patch byte count) and never the patch bytes; plan records carry the decision
text plus a reference to the history/{project}/{slug}/NNN.md version the
decision was made on rather than a second copy of the plan. Annotation
provenance (source, author) is preserved, so external, review-agent and
WebMCP findings stay tagged and source == null selects the reviewer's own
comments.

The shared module never throws: an archive failure is logged, degrades
silently for the user, and keeps the annotation draft as the recovery copy.
The append happens before deleteDraft, generalizing the #678 ordering.

Controlled by PLANNOTATOR_FEEDBACK_HISTORY / feedbackHistory (default on).
PLANNOTATOR_ANNOTATE_HISTORY=0 additionally suppresses records for every
annotate surface, so the documented stateless-annotate promise still holds.
"feedback" is added to PURGE_OWNED_TOP_LEVEL so uninstall purge removes it.

AI-assisted (Claude) under maintainer direction.
…a dir in tests

Review findings on the durable feedback archive.

1. The archive is default-on, and most server tests boot a real plan, review,
   or annotate server without redirecting PLANNOTATOR_DATA_DIR, so `bun test`
   deposited records in the contributor's own ~/.plannotator/feedback (24 files
   across 12 buckets from two test files alone) on CI and every machine. A new
   bunfig test preload, tests/setup/feedback-archive-off.ts, turns the archive
   off for the suite; the archive's own tests opt back in inside their test
   bodies, which is also how they exercise the opt-out. Those tests now use
   distinctive project names and remove the annotate history they leave in the
   real data dir, since storage.ts fixes its data directory at import time.

2. PR reviews bucketed under feedback/pr-<n>/. PR mode never sets gitContext
   and --local points agentCwd at a pool/pr-<n> checkout, so deriving the
   project from the review cwd was wrong. ReviewServerOptions now takes a
   `project` option, mirroring the annotate server, preferred over the cwd
   derivation on both runtimes; the Claude Code, OpenCode, and Pi entry points
   pass their already-computed detectProjectName() result.

3. changedFiles overcounted renames: extractChangedFiles unions the a/ and b/
   sides so a reader can resolve either path. The record now counts b-side
   paths through countChangedFiles, so a rename is one file.

4. Docs: the feedback archive is added to the privacy page and
   PLANNOTATOR_FEEDBACK_HISTORY (plus PLANNOTATOR_ANNOTATE_HISTORY) to the
   environment variables reference. The overclaim that every submitted review
   is archived is corrected: a review posted straight to GitHub or GitLab
   through /api/pr-action is not archived locally yet. Three behaviors are now
   written down: O_APPEND is not atomic on NFS or SMB and a genuine interleave
   damages both records that raced, folder-session records carry the folder
   path rather than the open document, and URL-session records store the full
   URL including its query string.

5. Pi parity: the Node mirror now has the failed-archive-write test (the one
   invariant its handler copies by hand) and the PR-mode bucketing test.

Comments only, no behavior change: the pool checkout recorded in
target.review.cwd can be cleaned up before anyone reads the record, and
getPlanVersionPath resolves the data directory storage.ts captured at import
while the archive resolves it per call.

AI-assisted (Claude) under maintainer direction.
plannotator-tui will append to the same feedback/{project}/index.jsonl with
client "plannotator-tui", so the module's stance of "a client tool may emit
this shape under its own clients/ namespace" is out of date. The index is one
shared source of records, labeled by client.

1. The module docstring and the FEEDBACK_RECORD_CLIENT comment now describe the
   shared index: several tools append to the same file, separated by `client`;
   plannotator-tui is a known second writer, herdr-annotate is reserved, and
   `client` is an open set rather than an enum to validate against.

2. Two optional fields are declared so v1 reserves their names across clients:
   target.agent ({ host, session, transcript }) for surfaces whose subject is
   an agent session rather than a file or a diff, and top-level clientVersion.
   Neither is populated here. clientVersion stays unset deliberately: there is
   no runtime-agnostic version constant in packages/shared, and reading
   package.json from a vendored module would be a new filesystem dependency
   for cosmetic data.

3. Sidecar naming is documented at the naming site and in AGENTS.md: other
   clients suffix their id ({stamp}-{surface}-{decision}-plannotator-tui.md),
   so recordFile values carrying such suffixes are valid and nothing may parse
   a sidecar name. Nothing in this repo did: every consumer treats recordFile
   as an opaque handle and no test pins a filename pattern. A new test appends
   a foreign line (unknown client, unknown fields, suffixed recordFile) and
   pins that the reader keeps it.

4. Honesty fix to the atomicity comments, in code and in AGENTS.md:
   appendFileSync loops internally, so "one write syscall" was wrong even on a
   local filesystem. The real model is that a line-sized buffer handed to a
   single append-mode write completes without interleaving in practice
   locally, with the reader's skip-unparsable tolerance as the backstop and
   the NFS/SMB caveat unchanged.

5. Exhausting the sidecar collision counter now throws a named error instead of
   re-throwing a bare EEXIST, so the server log says what actually happened:
   100 taken names in one millisecond means a stopped clock or a runaway
   writer, not a transient disk problem.

6. AGENTS.md and the parseFeedbackIndex doc state the reader contract: lines
   are gated on a numeric `v` and unparsable ones are skipped, so analyzers
   that depend on v1 semantics should filter v <= 1 themselves. Fields are
   added, never repurposed, so a v2 would mean a real shape change.

AI-assisted (Claude) under maintainer direction.
@backnotprop
backnotprop merged commit 990f3e8 into main Sep 1, 2026
27 of 28 checks passed
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.

1 participant