feat(review): document projection, diff geometry primitives, and the conformance harness (Phase 1 PR 2)

[benvinegar]

Phase 1 PR 2 of the browser-review rebuild (docs/browser-review-rebuild.md). Stacked on PR 1 (#716).

What

Lands the shared diff-geometry and document-projection primitives in src/core/review/ (geometry / expansion / anchors / identity / document / stml / contentManifest), moves every terminal derivation the audit flagged onto them (deleting the local copies), and builds the conformance harness (test/review-conformance/) with hand-written adversarial fixtures. Repays audit findings A1–A10 and D2 (terminal/model sites; browser halves close in Phase 5). Three commits: core primitives → terminal adoption → harness.

What changed, in diagrams

Derivation ownership: before

Each geometric fact about a diff was computed independently wherever it was needed — the audit found up to four copies per derivation, at least one of them wrong:

flowchart TB
    subgraph before["One derivation, many owners (copy count from the audit)"]
        HR["per-side hunk range — 4 copies, 1 wrong (A3)"]
        GAP["collapsed-gap line ranges — 3 copies, terminal off by one (A1)"]
        TRAIL["trailing-gap existence — 3 formulations (A2)"]
        SPLIT["source-line splitting — 3 copies, 1 unnormalized (A4)"]
        SIDE["expansion side — 3 copies (A5)"]
        MSG["empty-diff reason — 3 precedence orders (A8)"]
        TGT["default note target — 2 divergent rules (A10)"]
        REB["hunk rebasing — 2 copies, opposite isPartial (A6)"]
    end
    P["ui/diff/pierre.ts"] --- GAP & TRAIL
    E["ui/diff/expandCollapsedRows.ts"] --- SPLIT
    DSR["ui/diff/diffSectionRowPlan.ts"] --- SIDE
    SBH["ui/diff/sourceBackedHighlight.ts"] --- REB
    RR["ui/diff/renderRows.tsx"] --- MSG
    SDP["ui/staticDiffPager.ts"] --- MSG
    LC["core/liveComments.ts"] --- HR & TGT
    REG["session/app/registration.ts"] --- TRAIL

Loading

Derivation ownership: after

One primitive per derivation, in the boundary-gated core; consumers import, never re-derive. The deleted copies are gone (leadingCollapsedRanges, trailingCollapsedRanges, trailingCollapsedLines, sliceLines, hunkLineRange, firstCommentTargetForHunk, the expansion-side recomputations, the per-surface empty-diff precedence):

flowchart LR
    subgraph core["src/core/review — one owner per derivation"]
        GEO["geometry.ts<br/>reviewHunkRange · rebaseReviewHunk<br/>normalizedReviewSourceLines · reviewDefaultHunkLineTarget"]
        EXP["expansion.ts<br/>reviewLeadingGap · reviewTrailingGap<br/>reviewGapAddress · reviewExpansionSide"]
        DOC["document.ts<br/>projectReviewDocument · reviewEmptyDiffReason<br/>split/unified line totals (A7)"]
        ANC["anchors.ts<br/>resolveReviewNoteAnchor"]
        STML["stml.ts — stmlTagRole (A9)"]
        IDN["identity.ts — content identity, no node:crypto"]
    end
    P2["pierre.ts"] --> EXP
    E2["expandCollapsedRows.ts"] --> GEO
    DSR2["diffSectionRowPlan.ts"] --> EXP
    SBH2["sourceBackedHighlight.ts"] --> GEO
    RR2["renderRows.tsx"] --> DOC
    SDP2["staticDiffPager.ts"] --> DOC
    LC2["liveComments.ts"] --> GEO
    REG2["registration.ts"] --> EXP
    LAY["ui/lib/stml/layout.ts"] --> STML
    RP2["ui/lib/reviewProjection.ts"] --> IDN & ANC & DOC

Loading

The A1 bug fix, concretely

For a hunk with a zero-count side (reachable via git diff -U0 | hunk pager), the terminal's local gap math labeled expanded context off by one. Fixed once in reviewLeadingGap, adopted by the terminal:

flowchart TB
    H["pure-insertion hunk  @@ -6,0 +7,1 @@<br/>old side contributes zero lines"]
    B["before — pierre.ts local math<br/>leading gap labeled old 0–5<br/>(line 0 does not exist)"]
    A["after — core reviewLeadingGap<br/>leading gap labeled old 1–6<br/>labels match the source file"]
    H --> B
    H --> A

Loading

The symmetric pure-deletion case had the new-side range one low, revealing text one line early. Row counts are unchanged in both cases — only labels/content alignment corrected.

The conformance harness

Every fixture's expectations are hand-written from unified-diff semantics (never generated by calling the primitive), and each adversarial case is precisely an input a deleted copy got wrong. Consumers register as phases land:

flowchart LR
    subgraph fx["test/review-conformance — golden corpus"]
        F1["pure-insertion-hunk · pure-deletion-hunk (A1/A2)"]
        F2["hunk-with-leading-context (A3)"]
        F3["crlf-source · source-without-trailing-newline (A4)"]
        F4["binary-rename-with-no-rows (A8)"]
        F5["note-body cases ×8 (D2)"]
    end
    subgraph now["Registered now"]
        C1["core model"]
        C2["terminal render planning"]
    end
    fx --> C1
    fx --> C2
    fx -.-> P2ph["producer (Phase 2)"]
    fx -.-> P3ph["broker + wire (Phase 3)"]
    fx -.-> P5ph["browser projection (Phase 5)"]

Loading

Seam probe run: reverting the A1 formula inside core made the terminal's conformance suite fail alongside core's — mechanical proof the terminal consumes the primitive rather than keeping a shadow copy.

Disclosed behavior changes (2)

1. A1 gap labeling (bug fix, above): expanded-gap line labels for hunks with a zero-count side are now correct; pure-deletion reveals the right source lines.
2. Source identity became content-derived: a reload that changed nothing now keeps expanded gaps open and loaded source cached (previously identity was the fetcher object, so every reload retired them); changed content still retires both. Three useReviewController unit tests were rewritten to reload with changed content and one new test pins the preservation contract — this is the rung-4 "test edit = behavior change" red flag, raised deliberately: the change is the fix the identity primitive exists for.

Deliberate residuals (recorded in the audit doc)

• A2: reviewTrailingGap keeps the terminal's existing existence rule rather than the "correct" math — the correction would add trailing gaps to files whose last hunk has a zero-count side, an unsanctioned behavior change staged separately. Every consumer now agrees on one rule, which is the finding's point.
• A9: the STML parser stays in ui/lib/stml (it sanitizes via lib/terminalText, which core may not import); core owns the tag→role table and the parser derives its void/raw-text sets from it. Parser relocation is Phase 5 work.
• A6/A7: browser halves land in Phase 5 (rebaseReviewHunk returns per-side end indices so both use cases fit without either copy's isPartial opinion).

Design notes (fresh implementation, not a port)

• Primitives take minimal structural inputs (ReviewHunkSpan, ReviewGapSource) that Pierre metadata satisfies directly — the prototype's primitives demanded a fully projected file, which is why its terminal kept local copies.
• identity.ts is platform-neutral (no node:crypto) — the node-debt map shrank from 4 entries to 1 (jsonStream.ts only).
• File keys are addresses (path/rename/occurrence), not content hashes — content-derived keys (the prototype's choice) orphan every note on reload; contentIdentity separately answers "did the content move".

Gates

• bun run typecheck, bun run lint (deny-warnings), bun run format — clean.
• bun test: 2414 pass / 5 fail — every failure pre-existing and reproduced on the base branch (dev-dep imports ×2, one baseline ui-components failure, PTY load-flakes).
• bun run test:integration (PTY): 106/106, untouched.
• Boundary suite green; no test/pty / test/cli / test/session edits.
• AGENTS.md gains the test/review-conformance/ directory entry; per-finding repayment recorded inline in docs/browser-review-seam-audit.md.

---

Generated by Claude Code

[greptile-apps]

Too many files changed for review (53 files, 50 file limit).

Bypass the limit by tagging @greptile-apps to review.

[vercel]

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:49am