skills/backnotprop__plannotator
1
0
Fork 0
mirror of https://github.com/backnotprop/plannotator.git synced 2026-09-14 14:17:26 +08:00
Code Issues Packages Projects Releases Wiki Activity
1,191 Commits 375 Branches 154 Tags
main
Commit Graph

5 Commits

Author SHA1 Message Date
Michael Ramos 64062af9a1 feat: Portable Guided Reviews — export, share links, agent-authored guides, guides.show (#1324) 
A Guided Review can now leave Plannotator: as a single self-contained HTML file that renders exactly like the in-app guide, as an encrypted-by-default share link on guides.show, or authored by any agent through the new guide CLI.

Highlights: packages/guide-viewer extracted from review-editor at the injection seam (read-only host, no third renderer); guides.show Worker with R2-backed share storage, per-IP rate limiting on creation, delete tokens hashed at rest, and 128-bit ids; portable exports pin the viewer by SRI hash with budget and manifest gates in PR CI and at deploy; two-runtime parity across Bun and Pi verified; v0.27.x saved guides load unchanged. Retention is indefinite by explicit decision, to revisit with the lean sharing refactor.

Decision record: adr/decisions/007-portable-guided-reviews-20260815.md
 2026-08-16 12:17:13 -07:00
Michael Ramos 75e8b78cc7 fix(review): stop stubbing files whose worktree content the size probe cannot find (#1220) 
The oversized preflight in buildBoundedTrackedDiff mapped every object the
cat-file batch could not size to infinity. `missing` is routine: for
tree-vs-worktree diffs git hashes the WORKING-TREE content of any path pulled
into rename/copy detection and prints that hash in --raw output without ever
writing the blob, and partial clones report it for unfetched blobs. Those
files were excluded by pathspec and replaced with a contents-free binary stub,
so a renamed-and-edited file rendered as a silently empty card and
/api/file-content refused it as binary.

Missing now means unknown, not oversized, and an unreadable new side is bounded
by the working-tree file's stat size instead. Every rendered diff stays bounded
git-side by core.bigFileThreshold (#1205), genuinely oversized files still stub
via real probe sizes plus the stat door, and a blob git truly cannot read now
fails loudly through assertGitSuccess instead of blanking a file.

Client side, a chunk with a binary marker and no hunks now renders an explicit
placeholder in both the all-files view and the single-file viewer, so an empty
card can never again pass for "no changes here".

AI-assisted.
 2026-08-06 00:47:19 -07:00
Michael Ramos 7ba4e3b3e4 fix(review): mint content-derived diff cache keys so single-file tabs render fully (#1219) 
* fix(review): mint content-derived diff cache keys so single-file tabs render fully

Single-file diff tabs have not rendered their full-content diff since
v0.26.0: the expansion gap bars show no chevrons and clicking them does
nothing, at every file size.

@pierre/diffs 1.3.2 (the 1.2.8 -> 1.3.2 bump, upstream "Fix diff rerender
in edit mode (#878)") added name-based cacheKey defaulting in
FileDiff.render: an unset `fileDiff.cacheKey` becomes the file's name.
`areDiffTargetsEqual` — the only identity check its render and highlight
caches make — compares nothing but that key.

DiffViewer renders each file twice on one surviving FileDiff instance
(key={filePath}): first the PARTIAL diff from getSingularPatch, then the
AUGMENTED full-content diff from processFile once /api/file-content
lands. Neither set a cacheKey, so both defaulted to the filename and
Pierre served the stale partial render forever. Only the augmented diff
is expandable, hence the dead gap bars.

Both diffs now mint content-derived keys (`<path>#<hash>` and
`<path>#full#<hash>`), matching how AllFilesCodeView already keys its
items — which is why the all-files view was never affected. The hash
(not patch.length) matters because Pierre's worker highlight cache is a
singleton that outlives remounts. The partial diff needs its own key too:
with key={filePath} the instance also survives diff-type and base
switches, where a same-named new patch would otherwise hit the same
name-keyed stale cache.

hashString moves from AllFilesCodeView to utils/hashString.ts so both
surfaces mint keys the same way.

Covered by a new DOM test that mounts DiffViewer against the real
@pierre/diffs renderer, holds the /api/file-content response until the
non-expandable partial baseline is asserted, then requires the expansion
affordances to reach the pixels. It fails against the unfixed tree.

* fix(review): explain why an oversized file's card has no diff

Files over the 5 MB review limit are replaced by a contents-free stub
(buildOversizedTrackedStub, plus the untracked equivalent), which renders
as a header-only card with no counts and no explanation. Users read that
as a broken diff.

The stub now carries an explicit marker line in its extended header
(OVERSIZED_REVIEW_STUB_MARKER). A marker rather than a client heuristic
because the only other signal, `Binary files ... differ`, is exactly what
a genuine binary file emits, so a heuristic would put a false size-cap
explanation on every image in the diff. The marker lives in
shared/diff-paths so the browser bundle can detect it without pulling in
the node-facing review core; both server runtimes pick it up from
review-core, which vendor.sh already copies to Pi. Git ignores unknown
extended-header lines and @pierre/diffs parses the stub identically with
or without it, so nothing else moves. Which files get stubbed is
unchanged.

Both review surfaces now render one line under the file header saying the
file is over the limit and only a stub is shown.

* test(review): make the diff-swap proof machine independent, not stopwatch based

CI failed two tests that pass locally. Both were timing races, neither was
an app bug.

1. DiffViewer.fullContentSwap: the swap assertion carried a 15s internal
   wall-clock budget, which a cold, contended CI runner blows and a warm
   laptop clears. Two changes, both aimed at the clock rather than the
   symptom:

   - The waits are now budgeted in SCHEDULER TURNS, not milliseconds. A
     slower box spends longer inside each turn but needs no more of them,
     so the budget never has to be retuned for CI hardware.
   - Pierre's shared Shiki highlighter is preloaded before mounting. It is
     a module singleton, and building it was the entire multi-second cost
     the old budget was accidentally measuring; warming it moves that work
     into an unbounded await OUTSIDE the observed window. Disposed in
     afterAll, because packages/ui/utils/codeHighlight.test.ts asserts the
     pre-attachment behaviour of that same singleton.

   Verified against an artificially stalled clock: forcing 20s of dead time
   into every wait (41s total, far past the old 15s budget) still passes,
   and with the cacheKey fix removed it still fails on the assertion (not
   as an opaque timeout) in ~12s. A 20-turn budget with the preload removed
   and every core saturated also passed 10/10, so 400 turns is a wide
   margin rather than a guess.

2. App.archiveReadOnly compared the fenced block's innerHTML before and
   after a click. Since #1218, applyHighlight writes plain text first and
   swaps in Shiki markup when the grammar attaches, so that MARKUP changes
   on its own schedule and the assertion was racing the swap. The test is
   checking that the click opened no mutation entry point, which textContent
   plus the absence of an annotation <mark> says exactly, and which no
   highlight swap can perturb. Latent on main; the branch's run happened to
   lose the race.

Also fixed while confirming the above: codeHighlight.test.ts asserted a
GLOBAL precondition ("no grammar attached yet") that any earlier file
attaching a typescript fence invalidates, so
`DOM_TESTS=1 bun test packages/ui packages/editor` failed by file order
alone. It now resets the attachment cache through the existing
__resetCodeHighlightCacheForTests seam and asserts the contract instead of
the run order. Not currently reachable from CI (that file is not in the DOM
list), but one list edit away.

* test(review): drop the highlighter preload, harden the swap proof, report why a paint is missing

The preload added in the previous commit made CI strictly worse, so it is
gone. Before it, CI's partial diff painted and only the swap was missing;
with it, CI never painted at all. It was an optimization for a theory the
evidence has since killed, and it mutated a process-wide singleton to buy
it, so it is not worth keeping while the real failure is unexplained. The
afterAll dispose that existed only to undo the preload goes with it.

What the CI log actually shows:

  - The "WorkerPoolManager: operation canceled because the pool terminated"
    error is inside discardRestoreRender.test.tsx's own group, ~0.3s BEFORE
    this file's group opens. It is that file's provider unmounting and
    terminating the pool singleton it created: end-of-file teardown, the
    same benign noise documented on #1209. It also prints on every local
    run, where the whole list passes. It is not a mid-test terminator, and
    nothing in this file uses the worker pool (no WorkerPoolContextProvider
    is mounted, so useWorkerPool() is undefined and rendering takes the
    main-thread path).
  - This file's group prints NOTHING for its whole 10.3s: no console.warn
    from the stale-content guard, no error. Pierre simply painted nothing.

Not reproducible locally: the exact DOM list from test.yml, one bun
process, forward and reverse order, 13 runs with every core saturated, all
green. So the remaining difference is the environment, which cannot be
reasoned out from here. Three changes make the next CI run answer it
instead of costing another guess:

  - renderDiagnostics() dumps what Pierre actually painted (container /
    separator / chevron / line-number counts plus a markup fragment) when a
    wait gives up. Prints only on failure, so it is worth keeping.
  - The precondition is asserted rather than assumed: the REAL
    getSingularPatch and processFile must produce partial-then-full on
    these fixtures. Bun's mock.module is process global and an earlier file
    in this very list mocks '@pierre/diffs', so a leaked mock now fails in
    milliseconds with a clear message instead of as a render that never
    arrives.
  - The first paint is now REPORTED, not asserted. The verdict belongs to
    the swap; gating on the partial paint let a slow or absent first paint
    mask the result the test exists for. Removing the cacheKey fix still
    fails it (verified), because that tree paints no chevrons at any point.

Also fixed a real trap in the fixture: the hunk header said @@ -61 while
its context lines start at line 59 of both contents. Pierre realigns a
misaligned header rather than rejecting it, so it was silently tolerated.

* test(review): stop a leaked module mock from silently unrendering the diff tests

Root cause, and it was never a timing problem.

AllFilesCodeView.lifecycle.test.tsx calls
`mock.module('@pierre/diffs', ...)` with a hunk-less `getSingularPatch` and
`processFile: () => null`. Bun's module mocks are process global and are not
unwound at file boundaries, and that file sits immediately before
DiffViewer.fullContentSwap.test.tsx in the DOM step's list. On the Linux
runner the stub reached this file; on macOS it did not, which is why 13
local runs of the exact list, both orders, cores saturated, stayed green.

It explains both CI symptoms exactly, including the one that looked like a
contradiction: `processFile: () => null` means the augmented diff never
exists, so no chevrons ever (the failure before the preload); the stub
`getSingularPatch` has `hunks: []`, so nothing paints at all (the failure
after it). The "WorkerPoolManager: operation canceled because the pool
terminated" line was a red herring throughout: it is inside
discardRestoreRender's own group, ~0.3s BEFORE this file's group opens, is
that file's provider unmounting the pool it created, and prints on every
local run too.

The precondition assertion added in the previous commit is what proved it,
turning a 10.3s mystery into a 0.45ms verdict:

  228 |       expect(expected?.isPartial).toBe(false);
  error: expect(received).toBe(expected)
  Expected: false
  Received: undefined

Fixed at both ends:

  - Source: the mocking file now captures the real modules before it stubs
    them and restores both library specifiers in afterAll, so no later file
    in any run inherits its stubs. This fixes the class for every future
    DOM test that needs the real renderer, which was the actual leak.
  - Consumer: the two tests that render against the real @pierre/diffs get
    their own CI step, the same isolation (and for the same kind of reason)
    this workflow already gives useFileBrowser.test.tsx. They are removed
    from the shared list so that step is their single source of truth. The
    restore above should make this unnecessary; it is not something to bet
    a green build on from a machine that cannot reproduce the platform
    behaviour.

Verified with a CI-faithful harness: one bun process per step, the exact
lists from test.yml, isolated + shared-forward + shared-reverse, 10
iterations with every core saturated, then 6 more after the final split.
All green, plus the full suite and typecheck.

* docs(test): point the diff-renderer DOM tests at the CI step that actually runs them
 2026-08-06 00:31:25 -07:00
Michael Ramos 795f381ebe feat(review): "All changes" git-status review view (#990) 
* feat(review): "Since main" git-status review view + correctness fixes

Adds a composite `since-base` diff — merge-base(origin/main, HEAD) vs the
working tree, plus untracked — as the default code-review view, rendered as a
three-section "git status" panel (Committed / Changes / Untracked) with a
Sections|Tree toggle, a first-run setup chooser, and a "baseline behind GitHub"
fetch banner. Everything normalizes to one git patch, so the diff viewer,
annotations, and feedback path are unchanged.

New diff type wired through runGitDiff / fingerprint / file-content / context /
staging / agent-context in packages/shared + both server runtimes (Bun +
Pi mirror). Default flipped from `unstaged` to `since-base`; the old
DiffTypeSetupDialog is replaced by ReviewSetupDialog (view + default-diff
chooser, screenshots, Settings access).

Includes a reviewed batch of correctness fixes:
- P0: baseBehindRemote was permanently true (rev-parse missing --verify)
- fingerprint blind to quoted/unicode untracked paths (unquote)
- sections parser: record both sides of a rename; staged-delete wins over
  untracked (rm --cached collision)
- graceful degrade when merge-base can't resolve (trunk/no-remote repos)
- /api/fetch-base re-queries remote (narrow-refspec honesty)
- /api/diff/switch concurrency guard (diffSwitchEpoch) + draft rekey
- decouple remote-staleness probe from initialBase (Pi parity)
- keyboard file nav in the sections view; banner gated to base-relative modes

See adr/decisions/005-since-base-github-view-default-20260701-223706.md

* fix(review): since-base review-round hardening (11 fixes, both runtimes)

Addresses the multi-agent + PR-990 review findings:

- Sections rename parser split ` -> ` on the raw quoted porcelain token, so a
  filename containing ` -> ` tore into garbage and a dirty file could render as
  "Committed". Now quote-aware (splitPorcelainRename + unit tests).
- /api/diff/switch captured the epoch AFTER await req.json(), letting a
  slow-body older switch overwrite a newer confirmed one. Epoch now captured
  before any await; hideWhitespace committed only on win. Both runtimes.
- Unresolvable base (trunk / no origin/HEAD) no longer auto-defaults to a
  degraded since-base that hides committed work — getGitContext only offers
  since-base when the base ref resolves, so the default falls through to
  uncommitted. Fingerprint + file-content degrade to HEAD to match the diff.
- git rm --cached no longer yields two diff entries for one path (untracked
  files already in the tracked patch are dropped) — fixes wrong-file-open and
  j/k nav looping in the Sections panel; also fixes latent uncommitted/unstaged.
- Settings' "Default Diff View" list now preserves the sections<->since-base
  coupling (can't leave an invalid sections+classic pair).
- "Behind GitHub" banner only treats origin/* as fetchable; a bare local base
  ("main") is upgraded to its tracking ref at startup so Fetch can clear it.
- hashUntracked resolves untracked paths against the repo toplevel, not cwd, so
  a review launched from a subdirectory isn't blind to untracked edits.
- Agent review instruction now tells agents to enumerate/inspect untracked files.
- Terminology: one "Committed changes" label; the live "Since <base>" label is
  dynamic (matches the header); "Since main" kept only as product copy.
- Docs: CLAUDE.md endpoints/fields + since-base view section; ADR recap.

Verified: typecheck (all projects), bun test 1793 pass, and empirical repro of
the rm --cached dedup, trunk->uncommitted fallback, and rename parser.

* fix(review): restore "(PR view)" on the Committed-changes label (unify toward it, not away)

* fix(review): close since-base coverage gaps from PR-990 review round 2

- First-run setup no longer forces since-base on repos where it isn't available
  (base ref can't resolve). getGitContext omits since-base there; the first-run
  block now checks the same availability before resetting the default or showing
  the chooser, so committed work isn't silently hidden on trunk/no-origin repos.
- reviewBase now includes 'since-base', so changing the base while in the
  git-status view passes the selected base to /api/file-content — expandable
  context is fetched from the right merge-base (was falling back to default).
- Settings now mirror ReviewSetupDialog's coupling exactly: Sections ⇒ force
  since-base; Tree ⇒ leave the diff (Tree + since-base is valid and now
  saveable); classic diff ⇒ Tree; since-base diff ⇒ leave the view. Removes both
  coercions that previously discarded a supported preference.

* fix(review): PR-990 review round 3 — fingerprint/dedup/base-canonicalization

- Fingerprint now uses `git status --porcelain -uall`, so editing a file inside
  a brand-new untracked directory changes the fingerprint and the "Diff out of
  date" banner fires (was collapsed to `?? dir/` and hashed as unreadable).
- extractTrackedPatchPaths pairs `---`/`+++` and excludes only the file's KEYED
  path (new side, or old side for a pure deletion) — no longer drops a recreated
  rename-source file (`git mv a b && touch a`). Verified rm --cached still
  dedups to one entry and the recreated file still shows.
- resolveReviewBase canonicalizes a bare local default name ("main") to its
  tracking ref ("origin/main") on every call, so a client that loaded before the
  startup upgrade resolved can't revert the server to the stale local base on the
  next refresh/switch. Both runtimes (new Pi resolveReviewBase helper).
- Removed the dead activeDiffLabel prop chain (FileTree -> DiffTypePicker).
- ADR recap: documented the staleness-banner scope limitation (default base only).

Verified: typecheck (all projects), bun test 23/23 review-core+fingerprint pass,
empirical repro of the untracked-dir fingerprint, rm --cached dedup, and
rename-recreate cases.

* fix(review): PR-990 review round 4 — dedup content, staging gate, banner timing

- Dedup rewrite: instead of dropping the untracked side of a same-path collision
  (which hid content), drop the tracked DELETION block and keep the untracked
  working-tree content. `git rm --cached f` + edit now shows the new content, not
  a phantom deletion — still one entry per path (no dock/nav collision). New
  stripHeaderPath also strips git's trailing-tab metadata, so space-named files
  dedup correctly. Verified: rm --cached+modify shows content (1 entry),
  space-named (1 entry), rename+recreate still shows both.
- All-files (and the `a` shortcut on the focused file) now gate staging per-file:
  committed files in since-base mode are not stageable, matching SectionsPanel /
  FileTreeNode. Shared isPathStageable helper threaded via ReviewStateContext →
  AllFilesCodeView. Stops the confusing `git add` no-op that still flipped local
  staged/viewed state.
- /api/diff/switch now awaits recomputeBaseBehindRemote before building the
  response, so the "Baseline behind GitHub" banner reflects the new base
  immediately (no ~5s lag switching in, no stale banner switching away). Both
  runtimes.
- ReviewSetupDialog re-applies the recommended default only on first-run dismiss,
  not when reopened from the header menu (was snapping a mid-session diff back).
- SectionsPanel "N added" header now counts sidecar-staged files too, so it
  matches the staged dots on rows.

Verified: typecheck (all projects), bun test 1830 pass, empirical dedup repro,
both bundles build.

* refactor(review): reuse parsePatchPathToken in removeTrackedDeletions (self-review)

Drop the duplicated stripHeaderPath helper — the shared diff-paths
parsePatchPathToken already strips a/|b/ prefix + C-quoting + git trailing-tab,
and verifies the prefix instead of blindly slicing two chars. Documents the
binary-deletion edge (no --- line, so a binary rm --cached is not deduped).

* fix(review): PR-990 review round 5 — mixed-base + staging/sort/flicker nits

- Atomic base upgrade: the startup origin/* canonicalization swapped currentBase
  without rebuilding the patch, so /api/diff could advertise origin/main while
  the served hunks came from local main (mixed-base review on origin/HEAD-absent
  repos). Now rebuilds the diff for the new base and commits base+patch+ref+
  fingerprint together (only if no user switch happened); the fingerprint change
  makes the client's freshness poll pick it up. Both runtimes.
- isPathStageable: gate staging OFF when since-base is active but the sidecar
  hasn't loaded (was falling through to true, allowing a git-add no-op on a
  committed file).
- SectionsPanel: session-staged files now float to the top of Changes — the sort
  key was `false ?? stagedFiles.has(...)` which short-circuits; now ORs them.
- /api/diff/fresh: early returns now carry baseBehindRemote, so a snapshot change
  mid-probe no longer clears the "behind GitHub" banner for one poll. Both runtimes.
- Removed the now-dead activeLabelFallback prop from DiffTypePicker (FileTree
  stopped forwarding it in round 3).

Skipped (agreed): the fetch-base input-validation nit (no realistic attacker)
and the diff-switch TOCTOU (concurrent switches aren't UI-reachable).

Verified: typecheck (all projects), bun test 1830 pass, both bundles build.

* refactor(review): drop redundant staged-OR in SectionsRow (self-review)

item.staged (the grouping key) already ORs in stagedFiles as of the round-5
sort fix, so the row-level `|| stagedFiles.has(...)` is dead weight.

* fix(review): surface the startup base upgrade to already-loaded clients

Review round 6 fixes:

- The startup main -> origin/main upgrade re-baselined the freshness
  fingerprint, so a client that fetched /api/diff before the rebuild kept
  the old patch and every /api/diff/fresh probe reported fresh (the probe
  compares server state to itself, never to what the client renders). Now
  the fingerprint is only re-baselined when no client has loaded the
  pre-upgrade snapshot; otherwise the stale baseline trips the normal
  "Diff out of date - Refresh" banner. Bun + Pi.
- preserveFile refreshes (staleness Refresh, post-Fetch) now adopt the
  server's returned base — exactly the paths where the server may have
  canonicalized main -> origin/main; keeping the old name sent
  /api/file-content and Ask AI context against the wrong base.
- ReviewSetupDialog: clamp the fixed 800px height to the viewport so the
  dialog fits on small laptop screens.
- Docs: /api/diff/fresh response also carries baseBehindRemote/agentCwd.
- Documented the committed-deletion + untracked-recreation dedupe edge as
  accepted (code comment + ADR) — fixing it needs two same-path diff
  entries, which the path-keyed UI cannot represent.

* fix(review): stop the Git-status default from silently reverting to tree

Users with reviewPanelView=sections could keep opening in the tree view on
a stale diff type despite their cookie saying Git status. Three causes:

- The header Sections toggle persisted the view but not defaultDiffType,
  creating a conflicted pair (sections + non-since-base default) that every
  UI writer is supposed to prevent. It now couples the diff default like
  the setup dialog and Settings do.
- configStore's debounced POST /api/config could be lost when a session
  closed within 300ms of a change, leaving cookie and config.json split.
  Pending writes now flush on pagehide with a keepalive fetch.
- configStore.init() applies config.json over the cookie without the UI
  coupling, so a stale server value re-corrupted the pair on every load.
  The app now self-heals at load: if the view says sections but the diff
  default isn't since-base, it repairs the default (cookie + config.json)
  and switches the live session to since-base.

* fix(ui): keep the stage (+) button border visible on the active file row

The button's --border border has no contrast against the active row's 30%
primary tint, so it vanished on the selected row until hovered. Tint the
border with the row's primary color on active rows (both sections and tree
views); the button's own hover border still applies.

* ui(review): spell out the panel view toggle — 'Git status | Tree' text instead of icons

* fix(review): PR-990 review round 7 — export label, diff snapshot race, unicode paths

- exportFeedback describeDiff(): add the missing since-base case — every
  feedback export in the new default mode read "**Diff:** since-base".
- /api/diff GET: snapshot patch/base/ref/error BEFORE the sections-sidecar
  await and pass the pinned base into buildSectionsSidecar. The startup base
  upgrade landing mid-await could pair a rebuilt patch with sections grouped
  against the old base, with initialDiffServed still false so no refresh
  banner ever came. Bun + Pi.
- SectionsPanel: a session-staged untracked file now moves to the Changes
  section immediately (anticipating the server's next sidecar) instead of
  sitting in Untracked with a staged dot until refresh.
- unquoteGitPath: real C-style unquoting with octal (UTF-8 byte) escape
  decoding. JSON.parse rejects octal escapes, so non-ASCII names kept their
  literal \303\251 form — an untracked "café.txt" was silently ABSENT from
  the review (git diff --no-index could not access the quoted name) and the
  deletion dedupe Set lookup could never match. getUntrackedFileDiffs now
  unquotes ls-files output. Unit tests: octal decoding + end-to-end unicode
  untracked file in a real repo.
- review-core.test: worktree subtype round-trip now covers since-base and
  all (was 6 of 8).

Parked (deliberate): SectionsPanel/FileTree keyboard-nav dedup refactor.

* fix(shared): unquoteGitPath keeps literal unicode intact (self-review)

The byte-collector treated literal non-ASCII code units as single bytes,
which would mojibake headers synthesized by our own quoteGitPath
(JSON.stringify leaves unicode unescaped inside quotes — workspace-mode
prefixed headers round-trip through the same parser). Literal chars now
append as string code units (surrogate-safe); only octal escapes go
through the UTF-8 byte decoder.

* fix(review): PR-990 review round 8 — pre-staged toggles, explicit base, AI context race

- useGitAdd: session Set replaced with a tri-state override map folded over
  the sections sidecar; stagedFiles is now the EFFECTIVE staged set (sidecar
  + session stages - session unstages). Fixes pre-staged files: the first
  `a` press actually unstages (was a git-add no-op), the sidebar dot clears
  after a real unstage (was stuck via sidecar OR), and the All-files header
  agrees with the sidebar on load. Overrides reset whenever a fresh sidecar
  arrives (switch, preserveFile refresh, PR response) so stale session
  intent can't fight new porcelain truth. SectionsPanel drops its own
  sidecar OR; stagedCount = effective size.
- Explicit base picks are honored verbatim: the picker sends explicitBase,
  and the server permanently disables local-name -> origin/* canonicalization
  once set (the local/remote groups are distinct choices). The behind-GitHub
  banner also exempts an explicitly-picked local name — Fetch advances
  origin/*, so the banner would be un-clearable nagging. Bun + Pi.
- buildCurrentAiReviewContext(patch, base): GET /api/diff builds Ask AI
  context from the same served snapshot as the patch — the startup base
  upgrade could hand Ask AI a different changeset than the screen. Bun + Pi.
- recomputeBaseBehindRemote: capture remoteDefaultInfo once — a concurrent
  refresh nulling it mid-await threw. Bun + Pi.
- Revert stray "Status Update / Updated!" edit to adr/0001 (test debris
  swept into the round-2 commit).
- splitPorcelainRename comment corrected (porcelain v1 does NOT quote plain
  spaces; a name containing " -> " is ambiguous without -z) + ADR notes for
  the index-only-changes semantics and the rename edge.

* fix(review): FileTreeNode uses the effective staged set — round 9

Round 8 made stagedFiles the effective set (sidecar + session overrides)
and removed SectionsPanel's sidecar OR, but missed the same OR in
FileTreeNode's since-base row (sectionStaged). In the Tree fallback a
pre-staged file unstaged this session kept its dot and the next toggle
re-staged it. Grep-swept: this was the last surviving sidecar-staged OR.

* refactor(review): make the staged-display invariant unrepresentable

The round-8/9 bug class (sidecar staged flag ORed over the effective set)
existed because surfaces had a second staging source to reach for. Remove
it: stagedFiles is now a REQUIRED prop on SectionsPanel/FileTree/
FileTreeNode and the optional-prop fallback branches reading
sectionEntry.staged for display are deleted — a future surface cannot
reintroduce the OR because the pattern no longer exists to copy. The
sidecar type's staged field and AGENTS.md now document the invariant at
the point of temptation. Grep for display reads of .staged now hits only
useGitAdd (owner) and the sidecar builder (producer).

* refactor(review): self-review cleanups on the staged-invariant hardening

- orderFilesBySections: document why its snapshot .staged read is safe
  (only called at sidecar-fresh moments where snapshot = effective) and
  that it must not be reused mid-session — the one remaining display-side
  snapshot read the invariant sweep surfaced.
- FileTreeNode: drop the now-pure sectionStaged alias; use isStaged.

* fix(review): PR-990 review round 10 — header staging gate, fingerprint cap, single-writer coupling

- Single-file diff header now uses the per-path staging gate (canStagePath),
  closing the last ungated staging trigger: committed-only files in
  since-base offered a no-op Git Add that flipped local staged/viewed state.
  Full trigger inventory swept: App `a` shortcut, all-files `a` + header,
  SectionsPanel rows, FileTreeNode rows, single-file header — all six now
  per-path gated or group-gated; no context-menu staging exists.
- Fingerprint circuit-breaker: the freshness poll's `git status --porcelain
  -uall` degrades permanently (per cwd, per process) to collapsed -unormal
  once its output exceeds 2MB — a forgotten node_modules/ no longer burns
  CPU every 5s for the whole session. Costs untracked-dir edit sensitivity
  only on such repos; one possibly-spurious staleness banner at the switch.
- The sections ⟺ since-base coupling now has a single writer:
  setReviewPanelView/setReviewDefaultDiffType in @plannotator/ui/config.
  All five call sites (setup dialog x2, Settings x2, header toggle,
  first-run reset, self-heal) converted; grep for direct writes of either
  setting now hits only reviewView.ts.
- /api/diff/switch responses pass snapshot args to the AI context builder
  (both branches, Bun + Pi) — correct today, now robust against future
  awaits between the epoch check and the response.
- Docs: explicitBase in the /api/diff/switch body.

Parked per discussion: SectionsPanel/FileTree nav+search+footer dedup
(extract when the commit-list view adds a third panel), querySelector row
measurement, fetch-base stderr passthrough (standing decision), Pi
hasAgentLocalAccess (pre-existing follow-up list).

* fix(review): PR-990 review round 11 — subdirectory launches, escape decode, settings note

- Repo-root-relative patch paths now resolve against the git toplevel
  everywhere they meet a filesystem path or pathspec, via a shared
  resolveRepoToplevel helper: file-content working-tree reads (hunk
  expansion returned null from a subdirectory launch) and gitAddFile/
  gitResetFile (stage/unstage failed with pathspec errors). Both bugs
  pre-existed for uncommitted/unstaged; since-base made them the default
  experience. The two existing inline toplevel resolutions (untracked
  diffs, fingerprint) now use the same helper. Shared code — Pi inherits
  via vendoring. Real-repo subdirectory tests for both.
- unquoteGitPath decodes \uXXXX (JSON.stringify emits it for control
  chars without a short escape; our synthesized workspace headers
  round-trip through this decoder). Malformed \u stays literal. Tests.
- Pi explicitBase guard matches Bun byte-for-byte on empty-string base.
- Settings Git tab notes when the CURRENT repo can't serve the Git-status
  view (base ref unresolvable) instead of letting the preference look
  silently broken — it's a global preference, so the options stay.

Same-class items left parked (pre-existing, untouched by this PR):
open-in root resolution and code-nav file reads from subdirectory
launches — on the follow-up list with the Pi divergences.

* fix(review): PR-990 review round 12 — per-client freshness, keyboard-operable row controls

- Freshness is now judged PER CLIENT: every patch-carrying response
  (/api/diff, /api/diff/switch, pr-switch, pr-diff-scope) includes
  snapshotId (the server's draftKey), and the client echoes it on
  /api/diff/fresh probes. A mismatch reports stale for THAT client
  regardless of the VCS fingerprint. This fixes the round-12 finding —
  reloads/second tabs after the startup base upgrade got a permanently
  bogus staleness banner from the shared pre-upgrade baseline — and
  deletes the round-6 conditional re-baseline hack entirely (the
  fingerprint recaptures unconditionally again; the old client's banner
  now comes from its snapshot mismatch, not a deliberately stale
  baseline). Also gives unfingerprintable modes (P4, PR layer) snapshot-
  level staleness for free. Bun + Pi + client hook.
- StageControl and ViewedControl (which had the identical gap) are
  keyboard-operable: tabIndex + Enter/Space activation + focus outline.
  They're spans inside the row <button> (nested real buttons are invalid
  HTML), so they need their own focus stop; the a/v shortcuts remain the
  power path.
- Docs: snapshotId on /api/diff, ?snapshot= on /api/diff/fresh.

* fix(review): PR-990 review round 13 — re-key snapshots on scope switch, PR-tab refresh

- The PR scope switch now re-keys draftKey (= snapshotId + draft storage
  key) at BOTH commit points, unconditionally — matching every other
  snapshot commit site. The full-stack branch previously kept the layer
  patch's key: stale layer tabs never got the banner after a cross-tab
  scope switch (the exact case snapshotId exists for), and full-stack
  drafts collided with layer drafts (pre-existing). The layer branch's
  !layerPatchIncomplete conditional is gone — it only stayed consistent
  because full-stack never re-keyed. Invariant now: every currentPatch
  commit is followed by a re-key. Bun + Pi.
- Refresh works for any stale PR tab: re-selects the CURRENT scope
  instead of no-opping for layer (only full-stack could go stale in the
  fingerprint-only world; snapshot mismatch changed that). Accepted
  residual (documented in code): after a cross-tab PR switch, refresh
  updates the patch but not prMetadata — the scope endpoint doesn't
  carry it, and the full-rehydrate refactor isn't worth the two-tab edge.

* fix(review): stale incomplete-layer Refresh uses the non-blocking upgrade path (self-review)

Round 13 made Refresh re-select the current scope for stale PR tabs; for
an INCOMPLETE layer patch that POST triggers the server's local recompute,
which can park for minutes behind checkout warmup — and
handlePRDiffScopeSelect renders the full-screen switch overlay the whole
time. Route that case through handleLoadFullDiff (same POST, progress
notice instead of modal), which exists for exactly this slow path.

* fix(review): round 14 — composite snapshot id, fully-pinned /api/diff, "All changes" label

- snapshotId is now content hash + diff type (+ PR scope), built by a
  single currentSnapshotId() helper used at every response site and the
  freshness compare. A cross-tab MODE switch with a byte-identical patch
  (layer vs full-stack on a single-PR stack) now flags old tabs; the base
  is deliberately excluded so a same-commit main -> origin/main
  canonicalization stays banner-silent (round-12 noise-avoidance kept).
  draftKey stays a pure content hash — drafts survive content-identical
  round-trips. Bun + Pi.
- GET /api/diff pins ALL served fields (diffType, hideWhitespace,
  prDiffScope join patch/base/ref/error/snapshotId) and the sidecar + AI
  context builders take the pinned type instead of reading globals — a
  concurrent tab switch during the sidecar await can no longer produce a
  since-base patch labeled with another mode. Bun + Pi.
- First-user feedback: the since-base label is now plain English —
  "All changes since origin/main" (dynamic, follows the picked base);
  dialog/Settings short form "All changes"; feedback exports match.
  uncommitted reverts to "Uncommitted" where it had borrowed "All
  changes", so the two stay distinguishable side by side.

* docs(review): finish the Since-main -> All-changes terminology sweep in comments (self-review)

* fix(review): PR-990 review round 15 — base-revert race, freshness reset, unstage regroup

- resolveReviewBase gains a second, probe-independent rule: a non-explicit
  echo of the bare local name of the CURRENT origin/* base stays on the
  tracking ref. The existing rule keys off remoteDefaultInfo, which comes
  from a second network probe that can lag the startup upgrade by seconds;
  in that window a diff-type/whitespace switch echoing "main" committed
  the session back onto the stale local branch and set baseEverSwitched,
  permanently blocking the upgrade. Bun + Pi.
- useDiffFreshness resets stale/dismissed state on snapshotId too — since
  round 14 a new snapshot can reuse identical patch text with a different
  id, and the old banner state wrongly carried over until the next poll.
- SectionsPanel: unstaging a PRE-staged add moves it back to Untracked
  (mirror of the round-7 stage regroup). Detection deliberately uses the
  sidecar's snapshot flag to recognize "was pre-staged"; staged
  modifications stay in Changes, staged renames remain a refresh-heals
  edge.

* fix(review): PR-990 review round 16 — rename staged count, guarded fetch-base replay

- "N added" no longer double-counts staged renames: the sidecar truthfully
  marks BOTH porcelain sides staged, but an above-threshold rename renders
  as ONE file — the hidden old path inflated the count and left a phantom
  effective-staged entry that unstaging the visible row couldn't clear.
  The client now filters sidecar-staged paths to rendered files, which is
  correct in both patch shapes (below-threshold renames render delete+add
  as two rows and both sides pass the filter). Client-only; the sidecar
  stays faithful to porcelain.
- The fetch-base completion only replays the diff refresh if the user's
  diff type/base selection is unchanged since the click — a slow fetch no
  longer yanks the review back to the view captured at click time.

Declined with reasons (in review thread): first-run persist-before-confirm
(approved forced default, second flagging), explicit-base flag ordering
(early-set preserves user intent; commit-after-win would leave picker and
server disagreeing), base-picker revert target (near-unreachable compound
race, deferred).

* fix(review): PR-990 review round 17 — find origin/main on feature-only clones

getDefaultBranch's chain (origin/HEAD -> local main -> blind "master")
skipped the fetched remote-tracking ref entirely. On checkouts with no
origin/HEAD symref and no local main/master — CI checkouts, `clone
--branch feature`, extra worktrees — it guessed "master", the base
didn't resolve, getGitContext suppressed since-base, and the flagship
"All changes" view silently disabled itself for the whole session even
though origin/main was fetched and diffable. The startup upgrade can't
rescue it either (its guard reads the bogus "master" as a deliberate
non-default base).

Chain is now: origin/HEAD (verified) -> origin/main -> local main ->
origin/master -> "master" — remote-tracking refs preferred, matching the
function's stated prefer-upstream intent. Shared core (Pi inherits);
real-repo test reproducing the exact clone shape.
 2026-07-04 09:53:49 -07:00
Oscar Silva f08764063d feat(review): support multi-repo workspace reviews (#543) 
* feat(review): support multi-repo workspace reviews (#527)

* fix(workspace): address critical issues from deep review

- Fix race condition in label generation by pre-computing labels sequentially
- Fix rewritePatchLine to support quoted paths and rename/copy headers
- Add separator between aggregated patches to avoid invalid diffs
- Normalize input paths in resolveWorkspaceFilePath
- Add timeout to PR discovery (15s) to prevent server hangs
- Fix PATCH /api/workspace/repo to rollback state on failure via applyRepoMutation
- Validate body.source runtime (must be 'local' or 'pr')
- Snapshot active repo in agent jobs at launch to prevent race in onJobComplete
- Prevent double-prefixing of agent findings when paths are already prefixed
- Fix frontend findWorkspaceRepoForPath to use longest-prefix matching
- Fix shared types: diffType uses DiffType, platformUser is string | null

* fix: remove duplicate gitRuntime export in vcs.ts

* fix: resolve remaining merge conflict in local review mode, remove stale detectManagedVcs import

* Add local multi-repo workspace review support

* Fix workspace review edge cases

* Remove session query from browser launch

* Add switchable workspace review modes

* fix(review): cover opencode workspace bridge

* fix(review): clean workspace review plumbing

* fix(review): clarify workspace agent finding paths

* fix(review): preserve diff paths with spaces

---------

Co-authored-by: Michael Ramos <mdramos8@gmail.com>
 2026-06-05 14:29:38 -07:00