Skip to content

Persist Guided Reviews across sessions#1115

Merged
backnotprop merged 6 commits into
mainfrom
feat/guide-persistence
Jul 23, 2026
Merged

Persist Guided Reviews across sessions#1115
backnotprop merged 6 commits into
mainfrom
feat/guide-persistence

Conversation

@backnotprop

Copy link
Copy Markdown
Owner

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.

  • Autosave. When a guide job completes and passes the existing validateGuideOutput gate (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.
  • Storage. New runtime-agnostic packages/shared/guide-store.ts (node:fs, atomic tmp+rename writes, corrupt files load as "no saved guide"). The repo key is a sanitized host__owner__repo from 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.
  • Loading. Saved guides flow through the existing guide endpoints as 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.
  • Two new endpoints only. 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.
  • Client UX. A "Previous guides" list at the bottom of the guide landing page (label chip, title, age, progress, a quiet "diff changed" flag, per-row delete; hidden when empty). A small "Saved" chip in the guide header once persisted, and, only for an outdated saved guide, one muted hint line "Generated on a different version of this branch · Regenerate" that launches a fresh guide with the persisted defaults. The engine/model launch shapes were extracted into a shared useGuideLaunch hook so both launch surfaces stay identical. No banner machinery.
  • Opt-out. PLANNOTATOR_GUIDE_HISTORY=0 or { "guideHistory": false } in config.json disables writes; reads keep working and already-saved guides remain listed. Uses the same resolver pattern as annotateHistory, including quoted-boolean coercion.
  • Both runtimes. Everything server-side lands in packages/server/review.ts and the Pi mirror apps/pi-extension/server/serverReview.ts; guide-store was added to vendor.sh's copy list (exact .ts specifiers 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 moved flag is a single head-sha comparison computed at list/read time, nothing more.

Verification

  • bun test fully 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-editor green, including new GuideView tests for the Saved chip and the outdated hint's Regenerate wiring.
  • bun run typecheck clean; bun run --cwd apps/review build and bun run build:hook compile clean (dist not committed).
  • Live smoke against a scratch git repo with sandboxed HOME/data dir: seeded a guide through the store, verified GET /api/guides lists it (moved: true for a stale sha), GET /api/guide/saved:{id} serves it with saved/moved flags, PUT .../reviewed persists across a full server restart, DELETE removes it, and PLANNOTATOR_GUIDE_HISTORY=0 blocks writes while the enabled control writes.

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.
@backnotprop

backnotprop commented Jul 23, 2026

Copy link
Copy Markdown
Owner Author

Live visual verification

Live visual proofs of every state, recorded against this branch's build (33cc7ba5, bun run --cwd apps/review build && bun run build:hook, server run via bun apps/hook/server/index.ts review). The review server ran against a scratch git repo (2 commits, 3 modified files, 1 untracked file) with a sandboxed HOME and PLANNOTATOR_DATA_DIR. Captures are Playwright Chromium, 1920x1080 at 2x, dark theme.

One caveat, stated up front: real guide generation needs agent tokens, so the two saved guides were seeded by calling this PR's packages/shared/guide-store.ts (saveGuide) directly, under the same PLANNOTATOR_DATA_DIR and repo key the server derives. Everything after seeding (list, saved:{id} load, reviewed write-through, delete, restart survival) went through the running server's endpoints, so the identical read path is what's on screen. The reviewed-state change in step 5 was made through the UI itself.

Assets live on the orphan branch assets/pr-1115-visuals (no changes to this PR's code); the raw URLs above point at this branch, so it must stay for the images to keep rendering.

1. Guide launch screen with the Previous guides list

Both rows visible: the fresh guide with 1/3 progress, and the stale one flagged diff changed (stored headSha differs from current HEAD).

Launch screen with previous guides

2. Row hover

Hovering a row shifts the title to the primary color.

Row hover state

3. Loaded saved guide with the Saved chip

Clicking the fresh row loads it via saved:{id} through the existing guide endpoints. The header reads 3 sections · 1/3 reviewed with the Saved chip; the already-reviewed section is collapsed with its checkbox set.

Saved guide loaded

4. Stale guide with the regenerate hint

The moved guide shows the muted hint line "Generated on a different version of this branch · Regenerate" under the Saved chip.

Moved guide hint

5. Reviewed state survives a server restart

Before: section 2 toggled reviewed through the UI, now 2/3.

Before restart

The server process was then killed and restarted, and the browser session recreated from scratch. After: the list shows 2/3, and re-opening the guide shows both checkmarks still set.

List after restart

Reviewed state after restart

6. Per-row delete

Clicking the row's × removes it optimistically; the list updates.

After delete

A full page reload confirms the delete hit the server (DELETE /api/guides/:id); the row does not come back.

After delete and reload

7. Empty state

After deleting the last guide (and reloading), the Previous guides section hides entirely.

Empty launch screen

8. Opt-out sanity (PLANNOTATOR_GUIDE_HISTORY=0)

API-level terminal proof: with the server restarted under PLANNOTATOR_GUIDE_HISTORY=0, GET /api/guides still lists existing guides, but a new save through the PR's own writesEnabled gate (resolveGuideHistory(loadConfig()), wired exactly as packages/server/review.ts does) writes nothing:

$ PLANNOTATOR_GUIDE_HISTORY=0  # server restarted with guide history disabled

$ curl -s http://localhost:19571/api/guides   # listing still works
[
    {
        "id": "1784660806748-regional-tax-rules-for-checkout-totals",
        "label": "feature/tax-rules",
        "title": "Regional tax rules for checkout totals",
        "savedAt": 1784660806748,
        "progress": { "reviewed": 2, "total": 3 },
        "moved": false
    }
]

$ bun optout-proof.ts   # but a new save is blocked by the writesEnabled gate
PLANNOTATOR_GUIDE_HISTORY = 0
resolveGuideHistory(loadConfig()) = false
guides on disk before save attempt: 1
guides on disk after save attempt:  1
new save blocked: true

Video

One 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.

Download the walkthrough video (mp4, 2.2 MB)

…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.
@backnotprop

Copy link
Copy Markdown
Owner Author

Addressed the two findings from the maintainer's Plannotator-based review of this PR.

  1. Envelope labeled with completion-time context: the review-target context (PR url/label/head, branch label, head sha) is now snapshotted at job launch in both runtimes via guideStore.captureLaunchContext() and carried on the job itself as AgentJobInfo.guideContext, the same discipline as changedFilesSnapshot, so it is garbage collected with the job. saveForJob prefers the snapshot over the live getters, derives the storage shelf from the launch-time PR url, and reviewed write-through follows the file wherever it landed. Repair jobs reuse the failed job's own snapshot. New store tests mutate the injected getters between launch capture and completion to prove a guide launched on PR A and completed after a switch to PR B keeps A's label, url, head sha, and shelf.

  2. Saved guide surviving context switches: App.tsx now clears activeGuideJobId on any PR url or worktree-path change when it points at a saved: id, so a persisted guide never stays mounted over a different context's diff with a stale moved flag; it is reopened from the Previous guides list. There is no test seam around that App-level effect, so this one is verified by inspection: the effect's deps are exactly the two values GuideScreen's own context matching keys on, and the functional update leaves live job ids untouched.

Full bun test, DOM tests, typecheck, and both builds are green after the changes.

@backnotprop
backnotprop merged commit 99d11dc into main Jul 23, 2026
13 checks passed
@backnotprop
backnotprop deleted the feat/guide-persistence branch July 23, 2026 22:32
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.

Persist Guided Reviews across sessions for PRs and branches

1 participant