feat(review): browser-review rebuild Phase 0 guardrails + review store (Phase 1 PR 1) by benvinegar · Pull Request #723 · modem-dev/hunk

benvinegar · 2026-08-13T00:37:15Z

First main landing of the browser-review rebuild. This branch carries two already-reviewed units:

Phase 0 — seam contract and guardrails: the source-boundary seam gates for src/core/review/, src/session/reviewProtocol.ts, and src/web/ (tolerating absent trees, so they gate code before it exists), the extracted-duplicate tombstone list, the shrink-only node-debt map, the phased rebuild plan (docs/browser-review-rebuild.md), and the per-finding audit work-list (docs/browser-review-seam-audit.md).
Phase 1 PR 1 — the review store (refactor(review): extract a renderer-neutral review store into core (Phase 1 PR 1) #716, reviewed and squash-merged into this branch): renderer-neutral src/core/review/ store (types/state/actions/reducer/store/selectors/intents), the terminal refactored onto it (14 useState hooks → 3), and the active-voice header-comment rule in AGENTS.md.

The remaining stack (#719 → #720 → #721 → #722) lands on main one PR at a time after this: I'll rebase and retarget each to main in sequence as its predecessor merges.

Gates at this tip: typecheck/lint/format clean; boundary suite green; bun test failures a strict subset of baseline (pre-existing dev-dep + known flakes); PTY suite passing with the terminal behavior-neutral.

Generated by Claude Code

…he browser-review rebuild Phase 0 of the piecewise browser-review rebuild: make the seam between shared review primitives and their consumers mechanical before any code lands on it. - Add seam gates to the source-boundary suite: the shared review model (src/core/review) stays contained in core with Pierre types as its only external dependency, the wire protocol (src/session/reviewProtocol.ts) stays browser-safe, and the browser client (src/web) imports only shared primitives, React, and Pierre. Gates tolerate absent trees so they land ahead of the code they constrain. - Track the prototype's remaining Node-only model primitives as a shrink-only debt map that must be repaid with platform-neutral implementations before a browser bundle may import those files. - Document the staged rebuild plan in docs/browser-review-rebuild.md and the seam rule in AGENTS.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4

…ication audit A file-level audit of the browser-review prototype found roughly thirty duplicated derivations across terminal, browser, agent runtime, and broker — including four inconsistent hunk-range implementations, five generation/revision acceptance state machines, and three independent hunk-navigation walks. Widen the rebuild plan's seam beyond the review model and wire schema: diff geometry, navigation semantics, ordering and transfer, validation single-sourcing, and presentation helpers, each assigned to a rebuild phase, plus renderer parity tests as a Phase 5 gate and an explicit do-not-unify list for legitimately renderer- specific code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4

…on work-list The seam inventory in the rebuild plan summarized the prototype duplication audit at cluster level; the per-finding detail (sites on each side, observed divergences, replacing primitive) existed only in session context. Capture all ~30 findings in docs/browser-review-seam-audit.md so extraction PRs have a durable check-off list, and patch the inventory summary with the items it had not individually named (note-visibility predicate, wire vocabulary derivation, expandedLineProof, language-registration side effect). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4

…per-locus resolution Commands should work in the browser without transporting the terminal's dispatch table: split each command into shared catalog data (id, title, chords, resolution locus), per-client bindings, and effects. Semantic commands lower to ReviewIntents and resolve at the producer through the existing apply-action path; client-local view commands resolve in each client; host-only and extension commands stay uninvocable from the browser until an explicit allowlist design exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4

Record the command/keybinding seam as preemptive findings: the prototype browser shipped no command system, so unlike sections A-E these are the duplications the browser would grow the moment shortcuts are added - catalog fused with terminal bindings and effects, semantic effects as closures instead of intent dispatches, terminal-owned keymap resolution, and the host-only/extension command scope boundary. Adds a command-parity verification hook so a command added to one client without catalog registration fails tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4

Beyond commands, four more capabilities where the browser would invent its own semantics without a shared primitive: view-default resolution and shared-vs-per-client option classification, actor identity on wire actions and the multi-client selection policy, a semantic address grammar for deep links / copy-link / agent targeting, and a user-facing error catalog following the existing agent-errors precedent. Plus a placement rule for undo should it ever be added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4

…n protocol Define how each rebuild phase proves it used the shared primitives instead of re-deriving them: a five-rung mechanical ladder (boundary gates, a conformance harness built in Phase 1 that every consumer joins as it lands, adversarial fixtures derived from each audit finding's documented divergence with hand-written expectations, behavior- invariance suites, and wire-vocabulary derivation checks), plus a seam- probe spot check for the identical-reimplementation residual risk. Land the tombstone mechanism now: an append-only list of deleted duplicate modules the boundary suite keeps deleted, and a four-part mechanical definition of "finding repaid" shared by the plan and audit docs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4

…er-closure gate Last pass before execution. Link the plan to the audit mechanically: every phase now carries a Repays line naming the finding ids it converts, with a spanning rule (a finding closes when its last site converts). Restructure Phase 1 into three PRs (store adoption; document projection + geometry with the conformance harness; navigation intents + command catalog + address grammar) and align each phase gate with the verification ladder. Close a real gate gap: the node-debt map's "browser must never import these" clause was unenforced - add a transitive browser-closure check asserting the module graph reachable from src/web is free of node:/bun: imports, and allow the browser-safe broker-core parsers the audit tells the web client to reuse. Move the feature changeset to Phase 6, where the browser becomes user-reachable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4

…Phase 1 PR 1) (#716) Co-authored-by: Claude <noreply@anthropic.com>

vercel · 2026-08-13T00:37:21Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
hunk-web	Ignored	Preview	Aug 13, 2026 12:42am

greptile-apps · 2026-08-13T00:40:05Z

Greptile Summary

The PR establishes browser-review seam guardrails and introduces a renderer-neutral review store adopted by the terminal UI.

Adds review state, actions, reducer, selectors, intents, store, and comprehensive unit tests.
Refactors useReviewController, App, and DiffPane to project terminal behavior through the shared store.
Adds phased rebuild and seam-audit documentation plus source-boundary checks for core, protocol, and browser code.
The boundary scanner has a non-blocking syntax blind spot for static re-exports and side-effect imports.

Confidence Score: 4/5

The PR appears safe to merge, with a non-blocking gap in the new boundary scanner’s coverage of valid module syntaxes.

The review-store adoption preserves the checked terminal behaviors and duplicate-save protections; the remaining concern is that static re-exports and side-effect imports can bypass the architectural guardrails.

Files Needing Attention: scripts/source-boundaries.test.ts

Important Files Changed

Filename	Overview
scripts/source-boundaries.test.ts	Adds seam containment and browser-runtime checks, but its dependency scanner misses several valid static module syntaxes.
src/core/review/reducer.ts	Implements synchronous immutable review-state transitions with reconciliation, selection, notes, drafts, filters, and expansion handling.
src/core/review/intents.ts	Centralizes semantic validation and action planning, including idempotent draft persistence through synchronous store dispatch.
src/core/review/store.ts	Adds a small external store that publishes reducer snapshots synchronously to subscribers.
src/ui/hooks/useReviewController.ts	Refactors terminal review behavior onto the shared store while preserving existing controller and session-bridge interfaces.
src/ui/lib/reviewProjection.ts	Converts between terminal-specific files/comments and renderer-neutral review documents and stored notes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Terminal["Terminal UI"] --> Controller["useReviewController"]
  Controller --> Store["Review Store"]
  Store --> Reducer["Reducer"]
  Controller --> Intents["Intent Planner"]
  Intents --> Reducer
  Reducer --> State["Review State"]
  State --> Selectors["Selectors / Projection"]
  Selectors --> Terminal
  Browser["Future Browser Client"] -. shared seam .-> Store
  Session["Future Wire Protocol"] -. shared seam .-> Intents
  Guards["Source Boundary Guards"] -. constrain .-> Store
  Guards -. constrain .-> Browser
  Guards -. constrain .-> Session

Comments Outside Diff (1)

scripts/source-boundaries.test.ts, line 36 (link)

Scanner misses static module edges

The new dependency scanner does not recognize static re-exports such as export * from or side-effect-only imports, so those edges bypass every containment and transitive-closure check. This weakens the guardrail because a forbidden renderer or platform dependency can pass the boundary suite unnoticed.

Prompt To Fix With AI

This is a comment left during a code review.
Path: scripts/source-boundaries.test.ts
Line: 36

Comment:
**Scanner misses static module edges**

The new dependency scanner does not recognize static re-exports such as `export * from` or side-effect-only imports, so those edges bypass every containment and transitive-closure check. This weakens the guardrail because a forbidden renderer or platform dependency can pass the boundary suite unnoticed.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Prompt To Fix All With AI

### Issue 1
scripts/source-boundaries.test.ts:36
**Scanner misses static module edges**

The new dependency scanner does not recognize static re-exports such as `export * from` or side-effect-only imports, so those edges bypass every containment and transitive-closure check. This weakens the guardrail because a forbidden renderer or platform dependency can pass the boundary suite unnoticed.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{Reviews (1): Last reviewed commit: "refactor(review): extract a renderer-neu..." | Re-trigger Greptile}

The import scanner matched static/re-export `from` clauses and dynamic import() calls but not side-effect-only `import "..."` statements, so a renderer or platform dependency pulled in purely for its side effects would bypass every containment and closure check. Match them too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4

benvinegar · 2026-08-13T00:42:15Z

Half-confirmed and fixed in 6c40c89: static re-exports were already matched (they carry from), but side-effect-only import "..." statements were genuinely missed — and those are exactly how a renderer/platform dependency could slip past the containment and closure checks unnamed. The scanner now matches them; all 10 boundary gates still pass with no new violations surfaced by the widened scan.

Generated by Claude Code

claude and others added 9 commits August 12, 2026 14:15

refactor(review): extract a renderer-neutral review store into core (…

da5a1e2

…Phase 1 PR 1) (#716) Co-authored-by: Claude <noreply@anthropic.com>

benvinegar enabled auto-merge (squash) August 13, 2026 00:43

benvinegar merged commit 1995019 into main Aug 13, 2026
12 checks passed

andrewloux mentioned this pull request Sep 29, 2026

fix(session): keep a session connected when a reload leaves out a noted file #1131

Open

Navigation Menu

Repository navigation

feat(review): browser-review rebuild Phase 0 guardrails + review store (Phase 1 PR 1) - #723

feat(review): browser-review rebuild Phase 0 guardrails + review store (Phase 1 PR 1)#723
benvinegar merged 10 commits into
mainfrom
claude/review-rebuild-phase-0

benvinegar commented Aug 13, 2026

Uh oh!

vercel Bot commented Aug 13, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Aug 13, 2026 •

edited

Loading

Comments Outside Diff (1)

Uh oh!

benvinegar commented Aug 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

benvinegar commented Aug 13, 2026

Uh oh!

vercel Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Flowchart

Comments Outside Diff (1)

Uh oh!

benvinegar commented Aug 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel Bot commented Aug 13, 2026 •

edited

Loading

greptile-apps Bot commented Aug 13, 2026 •

edited

Loading