Persist Guided Reviews across sessions#1115
Conversation
Runtime-agnostic guide persistence for #1112: packages/shared/guide-store.ts writes validated guides to ${PLANNOTATOR_DATA_DIR}/guides/{repo-key}/{id}.json with atomic tmp+rename writes and graceful corrupt-file handling. The repo key is a sanitized host__owner__repo from the origin remote (or the PR url), with a dir-name+hash8 fallback when no remote parses, so PR and branch sessions of one repository share a shelf and same-named branches in different repos never collide. Includes the session glue (repo-key/headSha/label resolution plus the jobId-to-savedId map) shared by both server runtimes, the guideHistory config key with resolveGuideHistory (PLANNOTATOR_GUIDE_HISTORY, coerced booleans), and the browser-safe SavedGuideListEntry/CodeGuideData extensions.
…imes
Both packages/server/review.ts and the Pi mirror serverReview.ts now: autosave
a guide the moment it passes the existing validateGuideOutput gate (including
manual-repair submits); write reviewed-state changes on a live job id through
to that job's saved file; serve persisted guides through the existing guide
endpoints as saved:{id} pseudo job ids (GET guide + PUT reviewed); and expose
GET /api/guides (repo-scoped list with progress and a moved flag comparing the
stored head sha to the current head) and DELETE /api/guides/:id. guide-store
joins vendor.sh's flat copy list; cross-runtime endpoint wiring is covered by
packages/server/guide-persistence.test.ts against both servers, including
reviewed-state persistence across a server restart and traversal-id rejection.
…e hint
GuideEmptyState grows a Previous guides section under the Generate controls:
rows show the target label chip, title, age, reviewed progress, a quiet diff
changed flag when the stored head no longer matches, and a per-row delete;
clicking a row loads the guide via its saved:{id} pseudo job id (the existing
useGuideData/GuideScreen id plumbing already treats ids as opaque). GuideView
shows a small Saved chip once the active guide is persisted and, for an
outdated saved guide, one muted hint line whose Regenerate action launches a
fresh guide with the persisted defaults. The engine/model resolution and
launch-param shapes move into the shared useGuideLaunch hook so the empty
state and the hint stay in lockstep. DOM tests cover the new GuideView states.
Live visual verificationLive visual proofs of every state, recorded against this branch's build ( One caveat, stated up front: real guide generation needs agent tokens, so the two saved guides were seeded by calling this PR's Assets live on the orphan branch 1. Guide launch screen with the Previous guides listBoth rows visible: the fresh guide with 2. Row hoverHovering a row shifts the title to the primary color. 3. Loaded saved guide with the Saved chipClicking the fresh row loads it via 4. Stale guide with the regenerate hintThe moved guide shows the muted hint line "Generated on a different version of this branch · Regenerate" under the Saved chip. 5. Reviewed state survives a server restartBefore: section 2 toggled reviewed through the UI, now The server process was then killed and restarted, and the browser session recreated from scratch. After: the list shows 6. Per-row deleteClicking the row's × removes it optimistically; the list updates. A full page reload confirms the delete hit the server ( 7. Empty stateAfter deleting the last guide (and reloading), the Previous guides section hides entirely. 8. Opt-out sanity (PLANNOTATOR_GUIDE_HISTORY=0)API-level terminal proof: with the server restarted under VideoOne continuous session at human pace (27s, H.264 mp4): launch screen with the list, open the fresh guide, Saved chip, toggle a reviewed checkbox, back to the list, open the moved guide, the regenerate hint, delete a row, list shrinks. |
…etion-time state Review finding on #1115: saveForJob read the live session getters when the job COMPLETED, but guide jobs run for minutes while the session supports mid-generation PR switching (/api/pr-switch) and diff switches. Launch on PR A, switch to B, complete: the envelope permanently carried A's content labeled with B's PR label/url/headSha (and could even land on B's repo shelf). The review-target context (pr url/label/head, branch label, head sha) is now snapshotted at job LAUNCH via guideStore.captureLaunchContext() in the guide buildCommand branch of both runtimes and carried on the job itself as AgentJobInfo.guideContext, the same discipline as changedFilesSnapshot, so it is garbage-collected with the job and needs no separate cleanup. saveForJob prefers the snapshot (falling back to the live getters only for jobs launched without one), derives the shelf from the launch-time PR url, and records the shelf alongside the saved id so reviewed write-through follows the file wherever it landed. Repair jobs reuse the FAILED job's own snapshot. Covered by new session tests that mutate the injected getters between launch capture and completion.
Review finding on #1115: a saved:{id} guide has no AgentJobInfo, so GuideScreen's context match passes trivially (unknown ids are tolerated for the demo path). Switching PRs or worktrees while a saved guide was open left it mounted over the new context's diff with a stale moved flag. App.tsx now clears activeGuideJobId on any prMetadata.url / activeWorktreePath change when it points at a saved: id; the user reopens it from the Previous guides list. Live job ids are untouched, GuideScreen's own matching handles those.
|
Addressed the two findings from the maintainer's Plannotator-based review of this PR.
Full bun test, DOM tests, typecheck, and both builds are green after the changes. |










Closes #1112.
Problem
Guided Reviews lived only in the running review server's memory. Closing Plannotator and reopening the same PR or branch lost the generated guide and its per-section Reviewed state, forcing another agent run to recreate the same guide.
What this does
The deliberately simplified design agreed for v1: autosave plus a previous-guides list plus one hint line.
validateGuideOutputgate (or a manual repair submit passes the same gate), the guide is written to${PLANNOTATOR_DATA_DIR}/guides/{repo-key}/{id}.json. Failed or invalid guides never write. Reviewed-state changes on a live job write through to that job's saved file.packages/shared/guide-store.ts(node:fs, atomic tmp+rename writes, corrupt files load as "no saved guide"). The repo key is a sanitizedhost__owner__repofrom the origin remote, or the PR url in PR mode, so a PR review and a local review of the same repository share one shelf and same-named branches in different repositories never collide. No-remote fallback: repo-root dir name plus an 8-char path hash, matching the annotate history slug pattern. The envelope records label ("PR SEO: publish the new default social card #1082" or the branch name), title, engine/model, the head sha at generation, and the reviewed array.saved:{id}pseudo job ids; the client's guide id plumbing already treats ids as opaque, so opening one is just setting the active guide id.GET /api/guides(repo-scoped list with progress and amovedflag comparing the stored head sha to the current head) andDELETE /api/guides/:id.useGuideLaunchhook so both launch surfaces stay identical. No banner machinery.PLANNOTATOR_GUIDE_HISTORY=0or{ "guideHistory": false }in config.json disables writes; reads keep working and already-saved guides remain listed. Uses the same resolver pattern asannotateHistory, including quoted-boolean coercion.packages/server/review.tsand the Pi mirrorapps/pi-extension/server/serverReview.ts;guide-storewas added tovendor.sh's copy list (exact.tsspecifiers preserved by the existing normalization).Deliberately deferred, per the maintainer discussion: PR-to-branch identity unification, staleness banners with commit counts, retention/LRU, and switch-and-open. The
movedflag is a single head-sha comparison computed at list/read time, nothing more.Verification
bun testfully green (2263 pass, 0 fail, 193 files), including new unit tests for the store (save/load/list/delete, corrupt files, repo-key derivation incl. no-remote fallback, atomic replace, opt-out, PR-mode keys) and a cross-runtime endpoint test that starts both the Bun and Pi review servers and exercises list/get/reviewed-persists-across-restart/delete plus traversal-id rejection.DOM_TESTS=1 bun test packages/review-editorgreen, including new GuideView tests for the Saved chip and the outdated hint's Regenerate wiring.bun run typecheckclean;bun run --cwd apps/review buildandbun run build:hookcompile clean (dist not committed).GET /api/guideslists it (moved: truefor a stale sha),GET /api/guide/saved:{id}serves it with saved/moved flags,PUT .../reviewedpersists across a full server restart,DELETEremoves it, andPLANNOTATOR_GUIDE_HISTORY=0blocks writes while the enabled control writes.