diff --git a/.changeset/consistent-file-stat-badges.md b/.changeset/consistent-file-stat-badges.md new file mode 100644 index 000000000..6a7969c23 --- /dev/null +++ b/.changeset/consistent-file-stat-badges.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +File change stats now render consistently across the sidebar, diff header, and pager, hiding zero counts everywhere. diff --git a/AGENTS.md b/AGENTS.md index 4d70dee37..2f28ce884 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,16 +43,19 @@ ReviewIntent + caller facts -> planReviewIntent -> ReviewAction[] -> reducer -> `selectors.ts` shared policies; `store.ts` synchronous observable storage. New cross-surface operations start as intents. Callers supply mutable-note IDs/timestamps; core derives identities. - **Surfaces/publishers:** `useTerminalReview.ts` is the TUI adapter and - `reviewNoteMapping.ts` is terminal-only. Rows, measurement, scrolling, layout, themes, DOM - mechanics, and source I/O stay local. `useHunkSessionBridge.ts` publishes the current terminal + `reviewNoteMapping.ts` is terminal-only; `src/web/` is the browser client — read-only today + — where `browserReviewApiClient`/`browserReviewMirror` speak the Phase 4 HTTP contracts and + `browserPierreDocument`/`BrowserReviewStream` render with Pierre. Rows, measurement, scrolling, layout, + themes, DOM mechanics, and source I/O stay local to each surface. `useHunkSessionBridge.ts` publishes the current terminal session export; `registration.ts` builds its metadata/initial snapshot and `bridge.ts` receives agent commands. This broker export is not a full `ReviewState` mirror. - **Future consumers:** Web/API consumers reuse the model, derivations, state, intents, and then the producer/protocol tier (see Phases 2–3 in `docs/browser-review-rebuild.md` for modules and status). Never build a parallel protocol. Keep presentation/client-local state local; host/extension commands need explicit remote capabilities. -- **Conformance:** `test/review-conformance/` has hand-authored semantic fixtures and currently - covers core plus terminal render planning. Every new semantic consumer registers its real +- **Conformance:** `test/review-conformance/` has hand-authored semantic fixtures and covers + core, terminal render planning, the producer, the broker mirror, the wire, the HTTP + surface, and the browser client's projection, mirror, and event reader. Every new semantic consumer registers its real projection and runs the whole corpus. `scripts/source-boundaries.test.ts` keeps the seam renderer/platform-free; its Node-debt list is shrink-only and tombstone lists append-only. A repaid seam finding deletes copies, adds a file or banned-symbol tombstone and adversarial @@ -65,7 +68,7 @@ ReviewIntent + caller facts -> planReviewIntent -> ReviewAction[] -> reducer -> core VCS catalog. Do not add provider commands, spawning, or source readers under `src/core`. - Pager mode has two paths: full diff UI for patch-like stdin, plain-text fallback for non-diff pager content. - View defaults are layered through built-ins, user config, repo `.hunk/config.toml`, command sections, pager sections, and CLI flags. -- `hunk daemon serve` runs one loopback daemon that brokers agent commands to many live Hunk sessions. Normal Hunk sessions should auto-start and register with that daemon when session brokering is enabled. Keep it local-only and session-brokered rather than opening per-TUI ports. The daemon also mirrors each session's current review publication (generation plus resource catalog) and reads bulky content — patch text, canonical files, source — back as bounded, digest-verified resource chunks instead of holding it in the registration. Order publications with `classifyReviewPublication` and assemble chunks with `ReviewChunkAssembler`; do not add a second acceptance rule or a second assembly loop. The same daemon serves each session's review over HTTP (`src/session/broker/browserReviewServer.ts`): loopback and same-origin only, no CORS, and every route authorized by a per-session capability the session mints and publishes only the digest of. That surface is transport and authorization — its routes, capability grammar, SSE event contract, and error messages are the browser-safe modules `src/session/review{HttpProtocol,EventProtocol,ErrorCatalog}.ts`, and its semantic answers come from the producer through the existing intent path. +- `hunk daemon serve` runs one loopback daemon that brokers agent commands to many live Hunk sessions. Normal Hunk sessions should auto-start and register with that daemon when session brokering is enabled. Keep it local-only and session-brokered rather than opening per-TUI ports. The daemon also mirrors each session's current review publication (generation plus resource catalog) and reads bulky content — patch text, canonical files, source — back as bounded, digest-verified resource chunks instead of holding it in the registration. Order publications with `classifyReviewPublication` and assemble chunks with `ReviewChunkAssembler`; do not add a second acceptance rule or a second assembly loop. The same daemon serves each session's review over HTTP (`src/session/broker/webReviewServer.ts`): loopback and same-origin only, no CORS, and every route authorized by a per-session capability the session mints and publishes only the digest of. That surface is transport and authorization — its routes, capability grammar, SSE event contract, and error messages are the browser-safe modules `src/session/review{HttpProtocol,EventProtocol,ErrorCatalog}.ts`, and its semantic answers come from the producer through the existing intent path. - Extensions come in two tiers — user TypeScript extensions and the bundled tier in `src/extensions/default/` — running through one per-extension API object and registry (`src/extensions/runExtension.ts`, resolved via `src/extensions/apply.ts`). Every shipped VCS backend and the built-in sidebar are bundled extensions registering through the public API; that dogfooding keeps `hunkdiff/extension` honest. Hard rules: `src/extension-api/types.ts` stays import-free (declaration emission publishes whatever it reaches; `scripts/check-pack.ts` gates it); `src/extensions/default/vcs/` loads from VCS adapter resolution and must stay renderer-free (the sidebar loads separately via `getBundledSidebarView`); repo-local `.hunk/extensions/` never executes without the trust prompt; bundled extensions stay loaded under `--no-extensions`. The full architecture — host-served runtime modules, sidebar pane model, command dispatch, VCS detection ordering, conversion boundaries — is mapped in `docs/extension-architecture.md` and documented in depth by the module headers it names; the authoring guide is `docs/extensions.md`, and `skills/hunk-extensions/SKILL.md` is the agent-facing map of those touchpoints. - Agent rationale is optional sidecar JSON matched onto files/hunks. - The order of `files` in the sidecar is intentional. Hunk uses that order for the sidebar and main review stream. @@ -108,6 +111,7 @@ ReviewIntent + caller facts -> planReviewIntent -> ReviewAction[] -> reducer -> - `test/session/` for daemon/session integration and end-to-end flows. - `test/pty/` for PTY-backed live UI integration tests. - `test/review-conformance/` for the shared review model's golden fixtures and per-consumer conformance suites. + - Browser client code lives in `src/web/` with colocated unit tests; its end-to-end flow against a real session is `test/session/browserReviewClient.integration.test.ts`. - `test/smoke/` for opt-in terminal transcript smoke coverage. ## code comments diff --git a/bun.lock b/bun.lock index 884b9bcd0..36b745167 100644 --- a/bun.lock +++ b/bun.lock @@ -25,6 +25,7 @@ "@opentui/react": "^0.5.1", "@types/bun": "1.3.14", "@types/react": "^19.2.14", + "@types/react-dom": "^19.2.3", "@types/ws": "^8.18.1", "knip": "^6.32.0", "lint-staged": "^16.4.0", @@ -32,6 +33,7 @@ "oxfmt": "^0.41.0", "oxlint": "^1.56.0", "react": "^19.2.4", + "react-dom": "^19.2.4", "simple-git-hooks": "^2.13.1", "tuistory": "^0.0.16", "typescript": "^5.9.3", @@ -363,6 +365,8 @@ "@types/react": ["@types/react@19.2.14", "", { "dependencies": { "csstype": "^3.2.2" } }, "sha512-ilcTH/UniCkMdtexkoCN0bI7pMcJDvmQFPvuPvmEaYA/NSfFTAgdUSLAoVjaRJm7+6PvcM+q1zYOwS4wTYMF9w=="], + "@types/react-dom": ["@types/react-dom@19.2.4", "", { "peerDependencies": { "@types/react": "^19.2.0" } }, "sha512-Bsc+QHgp+P/F02XDzNCY9jnZNCUuLki36KT7VKrTXXLdHf+vHMNZnW1rVu5DNW/rCK+fya3DATySbLM4yhtKUw=="], + "@types/unist": ["@types/unist@3.0.3", "", {}, "sha512-ko/gIFJRv177XgZsZcBwnqJN5x/Gien8qNOn0D5bQU/zAzVf9Zt3BlcUiLqhV9y4ARk0GbT3tnUiPNgnTXzc/Q=="], "@types/ws": ["@types/ws@8.18.1", "", { "dependencies": { "@types/node": "*" } }, "sha512-ThVF6DCVhA8kUGy+aazFQ4kXQ7E1Ty7A3ypFOe0IcJV8O/M511G99AW24irKrW56Wt44yG9+ij8FaqoBGkuBXg=="], diff --git a/docs/browser-review-rebuild.md b/docs/browser-review-rebuild.md index ec4ce6af3..c32bd3f62 100644 --- a/docs/browser-review-rebuild.md +++ b/docs/browser-review-rebuild.md @@ -73,7 +73,7 @@ parallel-load test; vocabulary derivation checks active (rung 5). ## Phase 4 — HTTP surface, no client (landed) -`browserReviewServer` + capability auth + SSE, loopback-only, tested with plain `fetch`. +`webReviewServer` + capability auth + SSE, loopback-only, tested with plain `fetch`. Four routes per live session, mounted inside the existing daemon rather than on a port per terminal: the current publication (position plus resource catalog), bounded digest-verified resource reads through the existing mirror and cache, an SSE stream, and action submission @@ -97,10 +97,21 @@ this PR alone. ## Phase 5 — browser client (two PRs) -1. **Read-only mirror**: `apiClient` / `mirror` / `pierreDocument` / review stream rendering a - snapshot with Pierre — built on the shared geometry/selector/ordering primitives from day - one (no `sideRange`, no local acceptance rules, no bare `split("\n")`). No actions, no note - editing. +1. **Read-only mirror (landed)**: `src/web/` — `browserReviewApiClient` / `browserReviewMirror` / + `browserPierreDocument` / `BrowserReviewStream` rendering a publication with Pierre, built on the + shared geometry/selector/ordering primitives from day one (no `sideRange`, no local + acceptance rules, no bare `split("\n")`). No actions, no note editing. + + Two findings the PR settled rather than inherited. First, the question the Phase 4 run + boundary left open: a publication is a position plus a resource catalog, and the + catalog's content resources carry no selection, filter, expansion, or notes — so a + read-only client mirrors a review's _content_ and nothing about its semantic position, + and every note-shaped browser finding waits on PR 2 putting review state on the wire. + Second, a gap in the Phase 4 surface only a client could find: catalog descriptors are + unmeasured until the producer materializes them, so a resource response now states the + whole resource's size and digest in headers declared beside the routes, and the daemon + keeps the digest its own assembly verified against. + 2. **Interactivity**: action dispatch through the broker, selection sync (G2 policy decided before this PR), note editing, watch/reload generation swaps, browser key bindings and the command palette rendered from the shared catalog. @@ -108,10 +119,20 @@ this PR alone. Repays: A11, C3, C5, E1, G1; the browser sites of A/B/C/D findings left open in earlier phases; F browser bindings; G3/G4 browser adoption. E2 and the G2 selection policy must be decided (not necessarily built) before PR 2. +PR 1 closed: A6, A7, A11, C1's browser site, C2's browser site, C3, C4's client half, C5, +D4's browser site, E1, G4's browser adoption, and G1 part (a); A3/A4/A5/A8/A10 and B6's +geometry half are answered by the browser projection beside the terminal. Left for PR 2: +B3–B6's state halves, B7/B8, B10's client, D1's composers, D3, A9's parser relocation, E2, +F1–F3's browser halves, G1's persistence half, G2's policy, and G3's deep-link navigation. Gate: browser projection joins the conformance harness — the same fixtures every other consumer runs, closing the renderer-parity loop (note placement, gap addressing, reveal targets, default note targets); web boundary gates and the browser-closure node-free gate active; command-parity check (both clients render command surfaces from the shared catalog). +PR 1's half of that gate is met: `browser review projection` (geometry), `browser review +mirror` (ordering), and `browser review client reader` (events) are registered and run the +whole corpus, and `scripts/review-vocabulary.test.ts` now scans `src/web` for re-declared +constants. Navigation and wire consumers stay unregistered rather than hollow — a read-only +client plans no moves and sends no actions — and join with PR 2. ## Phase 6 — entry points and packaging @@ -218,7 +239,7 @@ re-derive. Every phase therefore passes the same five-rung ladder, and each rung one golden fixture corpus under `test/review-conformance/`, with every consumer registering a suite against the _same_ fixtures as it lands — terminal render planning (Phase 1), producer projection (Phase 2), broker mirror and wire round-trip (Phase 3), HTTP surface - (Phase 4), browser projection (Phase 5). A phase's gate is that all previously registered + (Phase 4), browser projection, mirror, and event reader (Phase 5 PR 1). A phase's gate is that all previously registered suites still pass plus its own joins. 3. **Adversarial fixtures from the audit**: every audit finding that documented a divergence contributes the fixture its old copy got wrong — pure-insertion hunks and zero-count sides diff --git a/docs/browser-review-seam-audit.md b/docs/browser-review-seam-audit.md index 41751b57a..35d3b166a 100644 --- a/docs/browser-review-seam-audit.md +++ b/docs/browser-review-seam-audit.md @@ -7,45 +7,59 @@ references are against the prototype branch (`feat/browser-review` at merge comm and will drift as that branch changes; treat them as locators, not anchors. Each extraction PR should delete the copies its primitive replaces and check off the finding here. -## Run boundary — after Phase 4 +## Run boundary — after Phase 5 PR 1 -Phases 0–4 have landed: the seam contract and its gates, the shared review model with the -terminal on it, the producer runtime, the wire protocol with the daemon's review mirror and -resource path, and now the HTTP surface — capability authorization, publication and resource -reads, the SSE event stream, and action submission — with no browser client. What remains +Phases 0–4 and Phase 5's first PR have landed: the seam contract and its gates, the shared +review model with the terminal on it, the producer runtime, the wire protocol with the +daemon's review mirror and resource path, the HTTP surface, and now a read-only browser +mirror that reads a publication over that surface and renders it with Pierre. What remains open, and where the plan puts it: -- **C3, C5** — the epoch/supersede queue and one reconnect scheduler, both browser-client - work (Phase 5). C4 is repaid on the server side; its client half closes with the reader - that imports the same module. -- **The event stream needs a `fetch` reader, not `EventSource`.** The capability is - presented in a header, which `EventSource` cannot set, so Phase 5's client consumes the - stream with `fetch` and a streaming reader. That is also what makes C5's "one reconnect - scheduler" reachable — there is no built-in reconnect to interleave with. -- **No replay buffer.** The surface answers any `Last-Event-ID` with a fresh publication - rather than retained frames. The stream carries no deltas, so a publication _is_ a - complete resynchronization and a history would be an optimization; if Phase 5 measures a - need for one, it is additive. -- **Browser sites of A, B, C, D** — every finding whose fix landed in core with only the - terminal, producer, broker, and wire converted keeps its browser half open: A6, A7, A11, - B3–B8, B10's client, C1 and C2's client sites, D1's composer sites, D3's - `pierreNoteAnchor`, D4's `parseCanonicalReviewFile`. -- **E1, E2** — the shared stat-badge formatter, and the product decision about whether the - browser mirrors the terminal theme. Both Phase 5; E2 must be decided before its second PR. +- **Phase 5 PR 1 answered the question the last run left open, and the answer is no.** A + publication is a position plus a resource catalog, and the catalog's three resource + kinds — canonical file, patch, source — carry the review's _content_ and nothing about + its _semantic position_. Selection, filter, expansion, and notes live in the producer's + `ReviewState`, and no resource contains them, so a client cannot mirror them by reading + harder. A read-only client therefore renders content only, and the browser sites of every + note-shaped finding (B7, B8, D1's composers, D3, A9's STML parsing) cannot be repaid + until PR 2 puts a review's semantic state on the wire. That is a scope fact, not an + oversight: the alternative — a client deriving a selection of its own — is exactly the + duplication this seam exists to prevent. +- **C3 is closed by there being one machine, not by unifying two.** The finding paired the + prototype's browser snapshot recovery with a runtime reload queue that never landed in + the rebuild. The mirror's supersede rule (`src/web/browserReviewMirror.ts`) is now the only one + in the repo: a load belongs to the generation that started it and checks that it is still + current before publishing anything, so a newer generation makes an older load's result + unwanted rather than something to cancel and unwind. Nothing to extract; a primitive with + one consumer would be the thing the seam rules warn about. +- **No replay buffer, still.** The surface answers any `Last-Event-ID` with a fresh + publication rather than retained frames, and the client is built for exactly that: a + publication is a complete resynchronization, so a reconnect needs no history. +- **Browser sites still open after PR 1** — B3–B6 (file-jump, selection fallback, filter + matching, reveal-target resolution) all need a selection to exist, which is PR 2; B10's + client, D1's composers, and D3's `pierreNoteAnchor` need note composition; A9's parser + relocation is still the prerequisite for rendering notes at all. +- **E2** — the theme decision is still open, and PR 1 deliberately did not make it: the + browser renders in Pierre's own palette and imports nothing from `src/ui/themes`. Due + before PR 2. - **F browser bindings** — F1–F3's browser halves (palette, keymap resolution onto DOM events). F4 stays a scope boundary, not work. -- **G1, G2 policy** — view-option classification and persistence, and the multi-client - selection and authorship policy. G2's _wire fields_ are done (see G2); what a receiver - should do with an actor tag is the decision, due before Phase 5 PR 2. -- **G3 adoption** — the address grammar exists with no consumers; browser deep links - (Phase 5) and opener fragments (Phase 6) close it. +- **G1 part (b), G2 policy** — the classification is done and the client consumes it; how a + client _persists_ its own overrides is PR 2's, alongside the multi-client selection and + authorship policy G2 is waiting on. +- **G3 adoption** — the grammar now has its first consumer (browser anchors); deep-link + _navigation_ and opener fragments close it in PR 2 and Phase 6. - **G5** — a placement rule for undo, if undo is ever built. Not work. -Two residuals earlier runs created rather than inherited: remote note _composition_ has no -draft-body intent yet (recorded under B12), and the publication a client reads over HTTP is -a position plus a resource catalog rather than a serialized `ReviewState` — selection, -filter, and notes reach a client through the resources and actions it already has, and -whether a client needs more than that is Phase 5's first question. +One residual earlier runs created rather than inherited: remote note _composition_ has no +draft-body intent yet (recorded under B12). + +PR 1 also closed a gap in the Phase 4 surface that only a client could find. A published +catalog describes resources the producer has not measured yet — measuring one means +producing its bytes — so a reader had nothing to verify a read against. Every resource +response now carries the whole resource's size and digest in headers declared beside the +routes (`reviewContentMeasurementHeaders`), and the daemon keeps the digest its own +assembly verified against rather than rehashing per request. ## A. Diff geometry @@ -82,6 +96,9 @@ whether a client needs more than that is Phase 5's first question. _Repaid (Phase 1 PR 2)_: `reviewHunkRange`/`reviewHunkRanges` in `core/review/geometry.ts`; `hunkLineRange` deleted and all six terminal/session sites converted; fixture `hunk-with-leading-context`. + _Closed (Phase 5 PR 1, browser site)_: the browser's render model reads its per-hunk + extents from `reviewHunkRanges`; there is no `sideRange`, and the `browser review +projection` consumer answers `hunk-with-leading-context` beside the terminal. - **A4. Source-line splitting for expanded context — 3 implementations, browser skips normalization.** Terminal `expandCollapsedRows.ts` `sliceLines` and core `anchors.ts` `normalizedReviewSourceLines` agree (CRLF-normalize, strip one trailing newline); web @@ -92,24 +109,39 @@ whether a client needs more than that is Phase 5's first question. _Repaid (Phase 1 PR 2)_: `normalizedReviewSourceLines` in `core/review/geometry.ts`; `expandCollapsedRows.ts` `sliceLines` deleted; `splitSourceLines` carries the comment saying why it is legitimately different; fixtures `crlf-source` and `source-without-trailing-newline`. + _Closed (Phase 5 PR 1, browser site)_: `browserReviewExpandedGapRows` splits the source it read + with the shared splitter, and the `browser review projection` consumer answers both + fixtures — the bare `split("\n")` the finding names never existed in this client. - **A5. Expansion side policy `deleted ? "old" : "new"` — 3 copies.** Core `intents.ts` (authoritative), terminal `diffSectionRowPlan.ts` (recomputed instead of reading `gap.side`), web `pierreDocument.ts` fallback ordering. Fix: thread `gap.side`; export `reviewExpansionSide(file)`. _Repaid (Phase 1 PR 2)_: `reviewExpansionSide` in `core/review/expansion.ts`; both terminal recomputations (`diffSectionRowPlan.ts`, `useReviewController.ts`) deleted. + _Closed (Phase 5 PR 1, browser site)_: the render model carries `expansionSide` from the + same function, and the client reads the source resource for that side rather than deciding + by fallback ordering. - **A6. Hunk content-index rebasing — 2 copies, opposite `isPartial` conclusions.** Web `pierreDocument.ts` `isolatePierreHunk` vs terminal `sourceBackedHighlight.ts` (~:108-199). Fix: one `rebaseReviewHunk(hunk, origins)` in core. _Repaid (Phase 1 PR 2, terminal site)_: `rebaseReviewHunk` in `core/review/geometry.ts`, adopted by `sourceBackedHighlight.ts`. It returns the per-side end indices so a caller can slice or - validate without re-walking; the browser's isolate-one-hunk use lands on it in Phase 5. + validate without re-walking. + _Closed (Phase 5 PR 1, browser site)_: `isolateBrowserReviewHunk` in `src/web/browserPierreDocument.ts` is + the shared walk plus two slices taken with the indices it reports, so the browser cannot + disagree with the terminal about where a hunk's lines end. The stream renders one Pierre + view per hunk — which is what makes a collapsed-region strip land between the right two + hunks — and `pierreDocument.test.ts` pins that each isolated render carries exactly its own + hunk's lines and stays partial. - **A7. File split/unified line totals — web guesses.** `pierreDocument.ts` reconstructs `splitLineCount`/`unifiedLineCount` by reducing over hunks; terminal reads Pierre's authoritative values. Mis-sizes browser virtualization when the parser counts rows outside hunk spans. Fix: carry both on `ReviewFileV1`. _Repaid (Phase 1 PR 2, model side)_: `splitLineCount`/`unifiedLineCount` carried on - `ReviewFileV1` by `core/review/document.ts`; the browser consumes them in Phase 5. + `ReviewFileV1` by `core/review/document.ts`. + _Closed (Phase 5 PR 1, browser site)_: `buildBrowserReviewFileRenderModel` reads both off the file. + `pierreDocument.test.ts` asserts they differ from the sum over hunks on a real parse, which + is the mis-sizing the prototype's reduction produced. - **A8. Empty-diff explanation — 3 variants with different precedence.** Terminal `renderRows.tsx` `diffMessage` (rename-pure first), web `ReviewStream.tsx` (binary first), `staticDiffPager.ts` (extra cases). Same file can explain itself differently per client. @@ -120,6 +152,9 @@ whether a client needs more than that is Phase 5's first question. `renderRows.tsx` and `staticDiffPager.ts` keep their own wording and share the reason; fixture `binary-rename-with-no-rows`. The static pager's own order put storage first, so a renamed binary or oversized rename now reports as a rename there too. + _Closed (Phase 5 PR 1, browser site)_: the browser keeps its own wording too and shares the + reason, so the binary-first precedence the prototype used is gone; the projection consumer + answers `binary-rename-with-no-rows`. - **A9. STML tag vocabulary — parse shared, tag semantics forked.** Terminal `ui/lib/stml/layout.ts` handles the full vocabulary; web `ReviewNote.tsx` handles a subset (everything else flattens to ``) and accepts a `` alias the terminal lacks. Fix: @@ -136,10 +171,19 @@ whether a client needs more than that is Phase 5's first question. _Repaid (Phase 1 PR 2)_: `reviewDefaultHunkLineTarget` in `core/review/geometry.ts`; `firstCommentTargetForHunk` deleted and both terminal callers converted; every geometry fixture pins the target, `pure-deletion-hunk` and `hunk-with-leading-context` adversarially. + _Closed (Phase 5 PR 1, browser site)_: the render model carries the shared target per hunk + and the projection consumer answers every fixture with it, so the prototype's "hunk's first + line" rule cannot come back. Nothing renders a note there yet — a publication carries none + — which is why the target is model rather than pixels until PR 2. - **A11. Language registration side effect missing in browser.** `core/fileLanguage.ts` registers `.mts`/`.cts`; the web bundle never imports it, so Pierre's own inference runs unregistered for files without an explicit `language`. Fix: side-effect import in `src/web/main.tsx` (or fold registration into the shared model). + _Closed (Phase 5 PR 1)_: `src/web/main.tsx` imports `../core/fileLanguage` for its side + effect before anything renders. The boundary gate refused the import until + `BROWSER_SAFE_CORE_MODULES` named the module and the reason, which is deliberately a list + rather than a directory — `src/core` also holds config resolution and file I/O, which a + browser has no business reaching. Renderer-specific, do not unify: terminal row construction and measured-cell windowing (`rowWindowing.ts`, `diffSectionGeometry.ts`) vs browser IntersectionObserver windowing and @@ -216,8 +260,12 @@ duplication); hunk header text (browser delegates to Pierre separators); platfor (preferred side first, backed sides only), behind `selectRevealTarget`. Fixture `pure-deletion-reveal-target` pins the case the prototype browser got wrong, and pins that a hunk's position is its first row while a note about the whole hunk hangs from its first change. - The terminal's reveal is row geometry it measures itself and stays renderer-local; the browser - consumes the selector in Phase 5. + The terminal's reveal is row geometry it measures itself and stays renderer-local. + _Partly repaid (Phase 5 PR 1, browser geometry)_: the browser's render model carries each + hunk's reveal target from `reviewCanonicalHunkLine`, so the `newRange ? "new" : "old"` + recomputation the finding names does not exist here. Reading `selectRevealTarget` — the + selector over review _state_ — needs a selection to reveal, so the client half of that + closes with PR 2. - **B7. "Jump to note" target — terminal geometry decides, web ignores.** The active-note choice lives in `DiffPane.tsx` row scanning; web never reads `reveal.scrollToNote`. Fix: `selectActiveRevealNoteId(state)` in core. @@ -334,7 +382,15 @@ path suffixes, expansion retention, git-status badges). call for an action's `expectedStateRevision`, so "has the review moved past what this caller decided from" is the same question as "is this publication ahead". The mirror is registered against the Phase 2 fixtures as the `broker review mirror` ordering consumer, - which is what proves it has no rules of its own. The browser site closes in Phase 5. + which is what proves it has no rules of its own. + _Closed (Phase 5 PR 1, browser site)_: `src/web/browserReviewMirror.ts` makes one + `classifyReviewPublication` call and acts on the verdict — `accepted` advances the + position and reads nothing (a generation's document is immutable, so there is nothing to + re-read), `gap` resyncs, `stale` is ignored. The prototype's contiguous `+1` revision + rule is gone with it, and `browser review mirror` is registered as an ordering consumer + whose verdict is _inferred from what the mirror did_ rather than reported by it: a client + with a comparison of its own disagrees with the reference on the C1 fixtures. Five + implementations with three policies are now one. - **C2. Chunk assembly + verification — 4 copies, 2 in one file.** Web `apiClient.ts` range loop; broker `state.ts` materializing and pre-sized loops (which already disagree on progress/eof rules); SSE reassembly in `mirror.ts`. Three in-flight dedupe key formats; @@ -365,14 +421,31 @@ path suffixes, expansion retention, git-status badges). reserving its kind's ceiling, which is what let a handful of ordinary patches serialize the parallel loads. Single flight is one map keyed by session, generation, and resource id; concurrent callers await the same assembly. `src/session/broker/reviewResources.integration.test.ts` - drives the whole path with only the socket replaced. The browser's `apiClient` range loop - closes in Phase 5. + drives the whole path with only the socket replaced. + _Closed (Phase 5 PR 1, browser site)_: `BrowserReviewApiClient.readResource` asks for windows and + hands each one to a `ReviewChunkAssembler`; it has no loop of its own beyond "ask for the + next offset", and the four copies with three in-flight key formats are now one class. + Two transport facts stay at the edge, as the finding says they should: the first window + asks for no `Range` at all — a zero-length resource has no satisfiable range, so asking + for one would refuse a resource that is merely empty — and end-of-stream is the window + that reaches the size the response states, since HTTP has no eof marker. Bulk loads run + under the shared `REVIEW_RESOURCE_LOAD_CONCURRENCY` rather than an unbounded + `Promise.all`. The measurement to verify against is the response's own + (`HUNK_REVIEW_CONTENT_SIZE_HEADER` / `HUNK_REVIEW_CONTENT_DIGEST_HEADER`), because a + published catalog's descriptors are unmeasured until the producer materializes them. - **C3. Epoch/supersede/trailing-retry — 2 parallel machines.** Runtime reload queue (`reloadEpochSequence`/`supersededReloads`) vs web snapshot recovery (`recoveryEpoch`/trailing while-loop), plus three unrelated anti-spin timing constants. Fix: one epoch-queue primitive; keep DOM/React wiring local. + _Closed (Phase 5 PR 1), by there being one machine_: the runtime reload queue never landed + in the rebuild — the producer publishes generations and `assertReviewPublicationAdvance` + checks them — so the only supersede rule in the repo is the mirror's, and it is a + generation comparison rather than a counter: a load records the generation it is for and + publishes only if that is still the current one. There is no trailing while-loop and no + anti-spin constant, because a superseded load is abandoned rather than retried. + Extracting a primitive for a single consumer is the thing this seam's rules warn about. - **C4. SSE event contract defined on both ends.** Frame names (`${type}-begin/-chunk/-end`), - begin/end envelopes, and the event-id grammar are built in `browserReviewServer.ts` and + begin/end envelopes, and the event-id grammar are built in `webReviewServer.ts` and re-declared/regex-parsed in `mirror.ts`/`apiClient.ts`; client bounds (12 MiB / 1024 chunks) are unlinked from server bounds and only coincidentally compatible. Fix: `src/session/reviewEventProtocol.ts` owning names, envelopes, id grammar, and bounds derived @@ -383,7 +456,7 @@ path suffixes, expansion retention, git-status badges). protocol's envelope bound, `REVIEW_EVENT_CHUNK_BYTES` is the shared resource chunk size, and `MAX_REVIEW_EVENT_CHUNKS` is the quotient, so a sender asking for smaller windows is clamped to the ceiling a reader is allowed to hold rather than emitting frames the reader - will refuse. `browserReviewServer.ts` imports all of it and declares none of it; the + will refuse. `webReviewServer.ts` imports all of it and declares none of it; the browser client imports the same module unchanged in Phase 5, which `scripts/source-boundaries.test.ts` keeps possible by gating the module's transitive closure platform-free. Two decisions differ from the prototype deliberately: a chunked @@ -393,13 +466,27 @@ path suffixes, expansion retention, git-status badges). name a position inside a half-delivered payload. Fixtures `publication-exactly-one-window` and `publication-one-byte-over-a-window` in `test/review-conformance/eventFixtures.ts` pin the boundary the two ends must agree on, - and both the protocol and the real HTTP surface are registered as event consumers. The - browser reader joins in Phase 5, which is when this finding closes. + and both the protocol and the real HTTP surface are registered as event consumers. + _Closed (Phase 5 PR 1, client side)_: `BrowserReviewApiClient.streamEvents` reads the stream with + `fetch` and parses it with `parseReviewEventFrameName`, `parseReviewEventFrame`, + `parseReviewEventBegin`/`Chunk`/`End`, and `ReviewEventAssembler` — it declares no frame + name, no envelope, no id pattern, and no bound. `browser review client reader` is + registered as an event consumer against the same fixtures the surface answers, with the + response body teed so the frames can be counted without asking the client to report its + own framing. Sender and reader now answer one corpus, which is what the finding was for. - **C5. Reconnect/backoff — 4 schedulers, 1 verbatim duplicate.** `apiClient.ts` (exp/4 s), web `App.tsx` (exp/4 s + anti-spin), `brokerClient.ts` (fixed 3 s — re-implementing the scheduler of the connection it already configures), `session-broker/connection.ts`. Fix: one `createReconnectScheduler` in `@hunk/session-broker-core`; EventSource's built-in reconnect interplay stays client-side. + _Closed (Phase 5 PR 1)_: `createReconnectScheduler` in `@hunk/session-broker-core` owns + "one pending attempt, a delay that may grow, a stop that cannot be restarted, a timer that + does not hold the process open". `SessionBrokerConnection` and `HunkSessionBrokerClient` + both dropped their copies with their timing unchanged (the default factor is 1, so a fixed + three seconds stays a fixed three seconds), and the browser mirror asks the same scheduler + for backoff with jitter — jitter because one daemon restart drops every open tab at once. + There is no `EventSource` reconnect to interleave with, which is the other half of why the + client reads the stream with `fetch`. ## D. Notes and validation @@ -456,11 +543,18 @@ path suffixes, expansion retention, git-status badges). from it. The manifest gained the content that requires (patch text, hunk blocks, source identity) and stays a snapshot rather than becoming the validator. The producer self-checks with it before serving any canonical file, and the conformance harness runs that check over - every fixture. The browser and broker copies go when those tiers land. + every fixture. + _Closed (Phase 5 PR 1, browser site)_: the browser has no field list and no second + consistency check. It recomputes the file's own content identity with + `reviewFileContentIdentityOf` — the same function projection uses, so producer and reader + cannot compute it differently — and refuses a file that does not hash to what it declares + or that arrives under another key. A field list cannot drift from the model when there is + no field list. Projection now hashes the file it built rather than a parallel description + of it, which is what made the reader's half expressible at all. - **D5. Validator/constant hygiene.** `isReviewSha256Digest` exists but is bypassed by five inline regexes with case-sensitivity drift; raw `createHash("sha256")` at seven sites instead of `reviewDigest`; `hasExactKeys` private while the pattern is inlined ~10×; - action-envelope parsing duplicated between `reviewProtocol.ts` and `browserReviewServer.ts`; + action-envelope parsing duplicated between `reviewProtocol.ts` and `webReviewServer.ts`; wire constants re-declared as literals (client range size vs server response cap, snapshot bounds, filter length cap) and re-typed in tests. Fix: export the validators/helpers from `reviewProtocol`/`brokerWireParsers`; import constants everywhere, and derive coupled bounds @@ -499,9 +593,27 @@ path suffixes, expansion retention, git-status badges). - **E1. File stat badges.** Terminal `ui/lib/files.ts` `formatSidebarStat` (zero-hiding, truncation marker) vs web inline `+${additions} −${deletions}` in `treeSource.ts`. One shared formatter. + _Closed (Phase 5 PR 1)_: `reviewFileStatBadges` in `core/review/presentation.ts` decides + the text, and states the two policies rather than leaving them implicit — a zero count is + hidden, and truncation is marked once, on the additions badge, because one marker per file + is enough to say both numbers are lower bounds. The sidebar's private `formatSidebarStat` + is deleted and tombstoned; the browser's file list and file headers call the same + function, so churn cannot read one way in a terminal and another in a browser. - **E2. Theme.** Web hardcodes two standalone palettes disconnected from `src/ui/themes` and the `AppTheme` mapping; whether the browser mirrors the terminal theme is an open product decision — decide before Phase 5, don't unify by default. + _Still open after Phase 5 PR 1, deliberately_: the read-only client renders in Pierre's own + palette and imports nothing from `src/ui/themes`, so no default has been set by accident. + `theme` is classified as a per-client option in `REVIEW_VIEW_OPTION_LOCUS`, which is the + part that was mechanical; whether a browser should adopt the terminal's chosen theme is + still the product decision, due before PR 2. + _Decided (before Phase 5 PR 2)_: the browser follows the terminal's theme, delivered as a + host default over the wire through the same locus mechanism view options already use — the + session publishes its active theme, the client renders it, and a client-side override is + per-client state under G1's persistence rules. Shared by wire, not by import: `src/web` + still imports nothing from `src/ui/themes`, so there is no second palette to drift; the + served theme arrives as semantic tokens, and Pierre's own palette remains the fallback for + a client that has not yet received a default. ## F. Commands and keybindings (preemptive — the prototype browser had none) @@ -547,6 +659,8 @@ here so the extraction happens before the duplication exists. Design detail in `selectReviewGapForSelection` in core, replacing the terminal's `selectGapForKeyboardToggle`. `SEMANTIC_COMMANDS_WITHOUT_REVIEW_EFFECT` is now empty, and every semantic command lowers to an intent a remote client could fire. + Still open after Phase 5 PR 1: the read-only client fires no commands, so the lowering's + second consumer is still the PR 2 palette. Residual (found in review): `lowerAppCommandToReviewIntent` still has no production caller — the terminal's handlers read the catalog's declared scope/direction but build their intents inline, so the lowering and the terminal closures can diverge with only @@ -584,6 +698,19 @@ implementation does. schema, not implied by where code happens to read them; (b) whether the browser receives the host's resolved view defaults as its starting point (it should — the host already computed them) with per-client overrides persisted client-side. Phase 5. + _Repaid (Phase 5 PR 1, part (a) and half of (b))_: `REVIEW_VIEW_OPTION_LOCUS` in + `core/review/viewOptions.ts` classifies every option as `review` or `client` over the + option schema, with totality as a `Record` over its keys — so an option added without a + locus fails to typecheck rather than being treated as per-client by whichever surface + reads it first. The rule behind the table is stated with it: an option is the review's + when it changes what the review is _about_, and a client's when it changes only how one + screen draws it. `src/web/browserViewOptions.ts` resolves the client-locus options only, over the + host's resolved defaults when the page carries them, and asks the predicate rather than + assuming — a host default for `showAgentNotes` or the filter is ignored rather than copied + into per-client state. Part (b)'s remaining half — how a client persists its own + overrides, and how the host's defaults reach the served page — is PR 2 and Phase 6: they + belong with the document that serves the page, not with a publication, because a window's + size is not review state. - **G2. Actor identity and multi-client selection policy.** Notes carry an optional `author`, but wire actions carry no actor/client identity — with a terminal, a browser, and agents attached to one session, nothing distinguishes who moved the selection or wrote a note, and @@ -609,6 +736,14 @@ implementation does. minting a per-client identity belongs with the client that needs one — Phase 5, beside the selection policy that is the only thing which will read it. Parts (2) and (3) are unchanged. + _Decided (before Phase 5 PR 2, part 2)_: selection is per-client with optional follow. + Each client owns its own selection; the terminal session's selection travels in the + published review state as it already does, and the browser offers a follow-the-terminal + mode that tracks it without ever writing it back. No client's navigation moves another + client's cursor — the fight B11 documented cannot recur — and nothing needs to arbitrate + concurrent moves because every action already carries its explicit target rather than + reading a shared cursor. Part (3) is unchanged by this: note authorship still defaults + from the actor tag on the action that composed the note. - **G3. Semantic addressing / permalinks.** The prototype's URL fragment carries only the capability token — there is no grammar for addressing a file/hunk/line/note. Three consumers will need one: browser deep links and back/forward history, a terminal "copy link" command, @@ -619,8 +754,12 @@ implementation does. _Repaid (Phase 1 PR 3, core primitive)_: `core/review/address.ts` serializes and parses the four address kinds over percent-encoded semantic identifiers, with round-trip coverage for keys carrying separators, percent signs, and non-ASCII characters, and strict rejection of anything - outside the grammar. No consumers yet, by design — browser deep links are Phase 5 and opener - fragments Phase 6, which is when this finding closes. + outside the grammar. + _First consumer (Phase 5 PR 1)_: the browser stream gives every file and hunk a DOM anchor + built by `formatReviewAddress`, and the file list links to them — so the addresses a page + offers are the grammar's rather than strings this client invented. Reading an address back + (deep-link navigation on load and on `hashchange`) needs a selection to move, which is PR + 2; opener fragments are Phase 6, which is when this finding closes. - **G4. User-facing error catalog.** The repo already solves this once for agents: `src/session/agent/errors.ts` single-sources every message the generated skill quotes, with contract tests. The browser has no equivalent — action rejections (`invalid-action`, @@ -637,11 +776,16 @@ implementation does. of the thing under test. Messages carry no interpolated caller input, so they can be rendered anywhere, including where echoing a request back would be wrong; the HTTP surface uses a catalog message unless the producer supplied a more specific one. The status map - lives with the transport (`browserReviewServer.ts`) rather than in the catalog, because a + lives with the transport (`webReviewServer.ts`) rather than in the catalog, because a client reads codes and an HTTP status is not something to tell a person. The agent surface keeps its own wording — its codes are `hunk session` CLI failures rather than these, and - the two vocabularies do not yet overlap. Browser adoption is Phase 5, which is when this - finding closes. + the two vocabularies do not yet overlap. + _Closed (Phase 5 PR 1, browser adoption)_: the client invents no wording. Every failure it + reports is `reviewClientFailure(code)`, whose message is the catalog's unless the surface + supplied a more specific one, and the page renders that message as-is. The one place the + client chooses a code rather than reading one is a refusal with no body — an unsatisfiable + range, which the surface answers as a bare 416 — and that choice is stated where it is + made. - **G5. Undo, if it ever arrives.** Note editing today has no undo. If it is added, the history/undo semantics belong in the shared reducer (which client undoes what, across actors), never in one client's keyboard handler. Recorded as a placement rule, not work. diff --git a/knip.json b/knip.json index 355f83665..db348a636 100644 --- a/knip.json +++ b/knip.json @@ -5,6 +5,7 @@ "entry": [ "src/extension-api/index.ts", "src/opentui/index.ts", + "src/web/main.tsx", "src/**/*.test.{ts,tsx}", "scripts/**/*.test.ts", "test/**/*.test.{ts,tsx}", diff --git a/nix/bun.lock.nix b/nix/bun.lock.nix index 45fe41499..19150dd8e 100644 --- a/nix/bun.lock.nix +++ b/nix/bun.lock.nix @@ -566,6 +566,10 @@ url = "https://registry.npmjs.org/@types/node/-/node-25.5.0.tgz"; hash = "sha512-jp2P3tQMSxWugkCUKLRPVUpGaL5MVFwF8RDuSRztfwgN1wmqJeMSbKlnEtQqU8UrhTmzEmZdu2I6v2dpp7XIxw=="; }; + "@types/react-dom@19.2.4" = fetchurl { + url = "https://registry.npmjs.org/@types/react-dom/-/react-dom-19.2.4.tgz"; + hash = "sha512-Bsc+QHgp+P/F02XDzNCY9jnZNCUuLki36KT7VKrTXXLdHf+vHMNZnW1rVu5DNW/rCK+fya3DATySbLM4yhtKUw=="; + }; "@types/react@19.2.14" = fetchurl { url = "https://registry.npmjs.org/@types/react/-/react-19.2.14.tgz"; hash = "sha512-ilcTH/UniCkMdtexkoCN0bI7pMcJDvmQFPvuPvmEaYA/NSfFTAgdUSLAoVjaRJm7+6PvcM+q1zYOwS4wTYMF9w=="; diff --git a/package.json b/package.json index 2615d69fb..9366c464f 100644 --- a/package.json +++ b/package.json @@ -130,6 +130,7 @@ "@opentui/react": "^0.5.1", "@types/bun": "1.3.14", "@types/react": "^19.2.14", + "@types/react-dom": "^19.2.3", "@types/ws": "^8.18.1", "knip": "^6.32.0", "lint-staged": "^16.4.0", @@ -137,6 +138,7 @@ "oxfmt": "^0.41.0", "oxlint": "^1.56.0", "react": "^19.2.4", + "react-dom": "^19.2.4", "simple-git-hooks": "^2.13.1", "tuistory": "^0.0.16", "typescript": "^5.9.3" diff --git a/packages/session-broker-core/src/concurrency.test.ts b/packages/session-broker-core/src/concurrency.test.ts new file mode 100644 index 000000000..5a86057f6 --- /dev/null +++ b/packages/session-broker-core/src/concurrency.test.ts @@ -0,0 +1,142 @@ +import { describe, expect, test } from "bun:test"; +import { ConcurrencyGate, inBoundedParallel } from "./concurrency"; + +/** A promise a test resolves by hand, so concurrency can be observed rather than timed. */ +function deferred() { + let resolve!: () => void; + const promise = new Promise((settle) => { + resolve = settle; + }); + return { promise, resolve }; +} + +describe("inBoundedParallel", () => { + test("returns results in input order however they finish", async () => { + const results = await inBoundedParallel([5, 1, 4, 2, 3], 3, async (item) => { + await new Promise((resolve) => setTimeout(resolve, item)); + return item * 10; + }); + + expect(results).toEqual([50, 10, 40, 20, 30]); + }); + + test("passes each item's own index to the work", async () => { + const results = await inBoundedParallel(["a", "b", "c"], 2, async (item, index) => + Promise.resolve(`${index}:${item}`), + ); + + expect(results).toEqual(["0:a", "1:b", "2:c"]); + }); + + test("never has more than the limit in flight", async () => { + const gates = Array.from({ length: 6 }, deferred); + let inFlight = 0; + let peak = 0; + + const run = inBoundedParallel(gates, 2, async (gate) => { + inFlight += 1; + peak = Math.max(peak, inFlight); + await gate.promise; + inFlight -= 1; + return true; + }); + for (const gate of gates) { + gate.resolve(); + await Promise.resolve(); + } + await run; + + expect(peak).toBe(2); + }); + + test("starts no more workers than there is work", async () => { + let started = 0; + + await inBoundedParallel([1, 2], 10, async (item) => { + started += 1; + return item; + }); + + expect(started).toBe(2); + }); + + // The clamp that had already drifted: an unclamped `Math.min(limit, n)` starts zero + // workers for a limit of zero and never settles. One is the floor. + test("runs serially rather than never when the limit is below one", async () => { + for (const limit of [0, -3, Number.NaN]) { + let inFlight = 0; + let peak = 0; + + const results = await inBoundedParallel([1, 2, 3], limit, async (item) => { + inFlight += 1; + peak = Math.max(peak, inFlight); + await Promise.resolve(); + inFlight -= 1; + return item; + }); + + expect(results).toEqual([1, 2, 3]); + expect(peak).toBe(1); + } + }); + + test("answers an empty list without running anything", async () => { + let started = 0; + + expect(await inBoundedParallel([], 4, async () => (started += 1))).toEqual([]); + expect(started).toBe(0); + }); +}); + +describe("ConcurrencyGate", () => { + test("hands out its slots at once and holds the rest", async () => { + const gate = new ConcurrencyGate(2); + let third = false; + + await gate.acquire(); + await gate.acquire(); + void gate.acquire().then(() => { + third = true; + }); + await Promise.resolve(); + + expect(gate.inFlight).toBe(2); + expect(third).toBe(false); + + gate.release(); + await Promise.resolve(); + expect(third).toBe(true); + expect(gate.inFlight).toBe(2); + }); + + test("releases waiters in the order they queued", async () => { + const gate = new ConcurrencyGate(1); + const order: number[] = []; + + await gate.acquire(); + const waiters = [1, 2, 3].map((id) => gate.acquire().then(() => order.push(id))); + for (const _ of waiters) { + gate.release(); + await Promise.resolve(); + } + await Promise.all(waiters); + + expect(order).toEqual([1, 2, 3]); + }); + + test("keeps a single slot when the limit is below one", async () => { + const gate = new ConcurrencyGate(0); + let second = false; + + await gate.acquire(); + void gate.acquire().then(() => { + second = true; + }); + await Promise.resolve(); + + expect(second).toBe(false); + gate.release(); + await Promise.resolve(); + expect(second).toBe(true); + }); +}); diff --git a/packages/session-broker-core/src/concurrency.ts b/packages/session-broker-core/src/concurrency.ts new file mode 100644 index 000000000..50036ad20 --- /dev/null +++ b/packages/session-broker-core/src/concurrency.ts @@ -0,0 +1,87 @@ +/** + * Running bounded work, for every caller that must not start everything at once. + * + * Reading many resources, hashing many files, fetching many sources: the shape is always + * the same — a limit, work that must not exceed it, and results that must come back in the + * order they were asked for. The repo had three copies of it, with the clamps already + * disagreeing: one guarded `Math.max(1, limit)` and one did not, so a limit of zero meant + * "no workers, hang forever" on one side and "one worker" on the other + * (`docs/browser-review-seam-audit.md`, C2). The clamp here is the guarded one, and a + * limit below one runs the work serially rather than never. + * + * Two entry points because callers arrive two ways. `inBoundedParallel` runs a list that is + * known up front; `ConcurrencyGate` bounds work that arrives over time and has no list at + * all. Both count the same slots the same way, and neither knows what the work is — this + * module has no I/O, no timers, and no platform, which is what lets a browser client + * import it alongside the daemon. + */ + +/** Hold one limit to at least a single worker, so a zero never means "never run". */ +function boundedLimit(limit: number) { + return Math.max(1, Math.floor(limit) || 1); +} + +/** + * Run one bounded-parallel pass over a work list, preserving the order of the results. + * + * At most `limit` calls to `run` are in flight; results are returned by input position + * regardless of the order they finished in. Never starts more workers than there is work. + */ +export async function inBoundedParallel( + items: readonly Item[], + limit: number, + run: (item: Item, index: number) => Promise, +): Promise { + const results = Array.from({ length: items.length }) as Result[]; + let next = 0; + const workers = Array.from({ length: Math.min(boundedLimit(limit), items.length) }, async () => { + for (let index = next++; index < items.length; index = next++) { + results[index] = await run(items[index]!, index); + } + }); + await Promise.all(workers); + return results; +} + +/** + * A fixed number of slots, taken and given back as work arrives. + * + * For the callers `inBoundedParallel` cannot serve: work that is requested one call at a + * time, where the limit has to hold across calls rather than within one pass. Waiters are + * released in the order they queued, so a burst of requests is served first-come rather + * than by whichever promise the runtime happens to resume. + */ +export class ConcurrencyGate { + private readonly limit: number; + private active = 0; + private readonly waiting: Array<() => void> = []; + + constructor(limit: number) { + this.limit = boundedLimit(limit); + } + + /** How many slots are in use right now. */ + get inFlight() { + return this.active; + } + + /** Take one slot, waiting when they are all in use. */ + acquire(): Promise { + if (this.active < this.limit) { + this.active += 1; + return Promise.resolve(); + } + return new Promise((resolve) => { + this.waiting.push(() => { + this.active += 1; + resolve(); + }); + }); + } + + /** Give one slot back to the next waiter. */ + release() { + this.active -= 1; + this.waiting.shift()?.(); + } +} diff --git a/packages/session-broker-core/src/index.ts b/packages/session-broker-core/src/index.ts index c75d74644..db4b02ff4 100644 --- a/packages/session-broker-core/src/index.ts +++ b/packages/session-broker-core/src/index.ts @@ -1,6 +1,8 @@ export * from "./types"; export * from "./brokerWire"; +export * from "./concurrency"; export * from "./limits"; +export * from "./reconnect"; export * from "./brokerState"; export * from "./selectors"; export * from "./sessionTerminalMetadata"; diff --git a/packages/session-broker-core/src/reconnect.test.ts b/packages/session-broker-core/src/reconnect.test.ts new file mode 100644 index 000000000..8da47b73b --- /dev/null +++ b/packages/session-broker-core/src/reconnect.test.ts @@ -0,0 +1,138 @@ +import { describe, expect, test } from "bun:test"; +import { createReconnectScheduler, type ReconnectSchedulerOptions } from "./reconnect"; + +/** A timer table a test drives by hand, so no scheduler assertion waits on real time. */ +function createFakeTimers() { + const due = new Map void>(); + let nextHandle = 1; + const delays: number[] = []; + return { + delays, + timers: { + setTimeout(handler: () => void, delayMs: number) { + delays.push(delayMs); + const handle = nextHandle; + nextHandle += 1; + due.set(handle, handler); + return handle; + }, + clearTimeout(handle: unknown) { + due.delete(handle as number); + }, + } satisfies NonNullable, + /** Fire every armed timer, as the runtime would when each delay elapses. */ + fire() { + const pending = [...due.values()]; + due.clear(); + for (const handler of pending) { + handler(); + } + }, + }; +} + +describe("createReconnectScheduler", () => { + test("arms one attempt at a time and reports which call armed it", () => { + const { timers, delays, fire } = createFakeTimers(); + let attempts = 0; + const scheduler = createReconnectScheduler({ + delayMs: 3_000, + onDue: () => { + attempts += 1; + }, + timers, + }); + + expect(scheduler.schedule()).toBe(true); + expect(scheduler.schedule()).toBe(false); + expect(scheduler.pending).toBe(true); + expect(delays).toEqual([3_000]); + + fire(); + expect(attempts).toBe(1); + expect(scheduler.pending).toBe(false); + // The timer is free again once it has run, which is what lets a failed attempt ask for + // the next one from inside its own callback. + expect(scheduler.schedule()).toBe(true); + }); + + test("keeps a fixed delay unless a factor asks otherwise", () => { + const { timers, delays, fire } = createFakeTimers(); + const scheduler = createReconnectScheduler({ delayMs: 3_000, onDue: () => undefined, timers }); + + for (let attempt = 0; attempt < 3; attempt += 1) { + scheduler.schedule(); + fire(); + } + + expect(delays).toEqual([3_000, 3_000, 3_000]); + }); + + test("grows by the factor, stops at the ceiling, and forgets on reset", () => { + const { timers, delays, fire } = createFakeTimers(); + const scheduler = createReconnectScheduler({ + delayMs: 1_000, + factor: 2, + maxDelayMs: 4_000, + onDue: () => undefined, + timers, + }); + + for (let attempt = 0; attempt < 4; attempt += 1) { + scheduler.schedule(); + fire(); + } + expect(delays).toEqual([1_000, 2_000, 4_000, 4_000]); + + scheduler.reset(); + scheduler.schedule(); + expect(delays.at(-1)).toBe(1_000); + }); + + test("spreads the delay by the jitter fraction", () => { + const { timers, delays } = createFakeTimers(); + const scheduler = createReconnectScheduler({ + delayMs: 1_000, + jitter: 0.5, + random: () => 0, + onDue: () => undefined, + timers, + }); + + scheduler.schedule(); + // Jitter subtracts rather than adds, so a jittered delay is never longer than the + // delay a caller asked for. + expect(delays).toEqual([500]); + }); + + test("takes an explicit delay over the computed one", () => { + const { timers, delays } = createFakeTimers(); + const scheduler = createReconnectScheduler({ delayMs: 3_000, onDue: () => undefined, timers }); + + scheduler.schedule(50); + expect(delays).toEqual([50]); + }); + + test("cancel keeps the scheduler usable and stop does not", () => { + const { timers, fire } = createFakeTimers(); + let attempts = 0; + const scheduler = createReconnectScheduler({ + delayMs: 3_000, + onDue: () => { + attempts += 1; + }, + timers, + }); + + scheduler.schedule(); + scheduler.cancel(); + fire(); + expect(attempts).toBe(0); + expect(scheduler.schedule()).toBe(true); + + scheduler.stop(); + fire(); + expect(attempts).toBe(0); + expect(scheduler.schedule()).toBe(false); + }); +}); diff --git a/packages/session-broker-core/src/reconnect.ts b/packages/session-broker-core/src/reconnect.ts new file mode 100644 index 000000000..eff037a98 --- /dev/null +++ b/packages/session-broker-core/src/reconnect.ts @@ -0,0 +1,119 @@ +/** + * One reconnect timer, for every client that has to come back after a dropped link. + * + * The pieces are always the same — at most one pending attempt, a delay that may grow with + * consecutive failures, a stop that cannot be restarted, and a timer that must not hold the + * process open — and the prototype wrote them four times with three different policies, + * including one client re-implementing the scheduler of the connection it already + * configured (`docs/browser-review-seam-audit.md`, C5). It is written once here, in the + * package both the session-side connections and a browser client can import. + * + * What stays with each caller is what should: which failures are worth retrying, and what + * to do when the timer comes due. This module owns the timing and nothing else — it never + * touches a socket, so it is as usable over `fetch` as over a websocket. + */ + +export interface ReconnectSchedulerOptions { + /** Delay before a first retry, and the whole delay while `factor` is 1. */ + delayMs: number; + /** What the timer does when it comes due. */ + onDue: () => void; + /** + * Growth per consecutive attempt. The default of 1 keeps a fixed delay, so a caller that + * wants "try again in three seconds, forever" gets exactly that rather than a backoff it + * did not ask for. + */ + factor?: number; + /** Ceiling the delay never exceeds, however many attempts have failed. */ + maxDelayMs?: number; + /** + * Fraction of each delay spread randomly, as a number in `[0, 1]`. + * + * Several clients dropped by one restarting daemon otherwise retry in lockstep and + * arrive as one burst; jitter turns that into a spread. + */ + jitter?: number; + /** Randomness source, injected so a test can pin the jitter. */ + random?: () => number; + /** Timer functions, injected so a test can drive time. */ + timers?: { + setTimeout: (handler: () => void, delayMs: number) => unknown; + clearTimeout: (handle: unknown) => void; + }; +} + +export interface ReconnectScheduler { + /** + * Arrange one attempt, unless one is already pending or the scheduler is stopped. + * + * Returns whether this call is the one that armed the timer, which is what a caller + * needs to avoid logging a reconnect per failed message. + */ + schedule: (delayMs?: number) => boolean; + /** Cancel any pending attempt without ending the scheduler. */ + cancel: () => void; + /** Forget the failure count, so the next delay is the first one again. */ + reset: () => void; + /** Cancel and refuse every later `schedule`. */ + stop: () => void; + /** How many attempts have been scheduled since the last `reset`. */ + readonly attempts: number; + /** Whether an attempt is currently armed. */ + readonly pending: boolean; +} + +/** Build one reconnect scheduler. */ +export function createReconnectScheduler(options: ReconnectSchedulerOptions): ReconnectScheduler { + const factor = options.factor ?? 1; + const jitter = Math.min(Math.max(options.jitter ?? 0, 0), 1); + const random = options.random ?? Math.random; + const setTimer = options.timers?.setTimeout ?? ((handler, ms) => setTimeout(handler, ms)); + const clearTimer = options.timers?.clearTimeout ?? ((handle) => clearTimeout(handle as never)); + + let handle: unknown = null; + let attempts = 0; + let stopped = false; + + /** The delay this attempt waits: growth, then ceiling, then jitter. */ + function delayFor(attempt: number) { + const grown = options.delayMs * factor ** Math.max(attempt, 0); + const bounded = Math.min(grown, options.maxDelayMs ?? grown); + return jitter === 0 ? bounded : bounded * (1 - jitter + jitter * random()); + } + + return { + schedule(delayMs?: number) { + if (handle !== null || stopped) { + return false; + } + const wait = delayMs ?? delayFor(attempts); + attempts += 1; + handle = setTimer(() => { + handle = null; + options.onDue(); + }, wait); + // Node's timers can hold a process open; a pending reconnect never should. + (handle as { unref?: () => void })?.unref?.(); + return true; + }, + cancel() { + if (handle !== null) { + clearTimer(handle); + handle = null; + } + }, + reset() { + attempts = 0; + }, + stop() { + stopped = true; + this.cancel(); + }, + get attempts() { + return attempts; + }, + get pending() { + return handle !== null; + }, + }; +} diff --git a/packages/session-broker/src/connection.ts b/packages/session-broker/src/connection.ts index 7fffe8416..4e3d79f21 100644 --- a/packages/session-broker/src/connection.ts +++ b/packages/session-broker/src/connection.ts @@ -1,8 +1,10 @@ -import type { - SessionClientMessage, - SessionRegistration, - SessionServerMessage, - SessionSnapshot, +import { + createReconnectScheduler, + type ReconnectScheduler, + type SessionClientMessage, + type SessionRegistration, + type SessionServerMessage, + type SessionSnapshot, } from "@hunk/session-broker-core"; import type { SessionBrokerConnectionCloseDirective, @@ -54,7 +56,7 @@ export class SessionBrokerConnection< private socket: Socket | null = null; private bridge: SessionBrokerConnectionBridge | null; private queuedMessages: ServerMessage[] = []; - private reconnectTimer: ReturnType | null = null; + private readonly reconnect: ReconnectScheduler; private heartbeatTimer: ReturnType | null = null; private stopped = false; private registration: SessionRegistration; @@ -72,6 +74,12 @@ export class SessionBrokerConnection< this.bridge = options.bridge ?? null; this.registration = options.registration; this.snapshot = options.snapshot; + // Timing is the shared scheduler's; this connection only decides when to ask for a + // retry and what one does (`docs/browser-review-seam-audit.md`, C5). + this.reconnect = createReconnectScheduler({ + delayMs: options.reconnectDelayMs ?? DEFAULT_RECONNECT_DELAY_MS, + onDue: () => this.connect(), + }); } start() { @@ -84,10 +92,7 @@ export class SessionBrokerConnection< stop() { this.stopped = true; - if (this.reconnectTimer) { - clearTimeout(this.reconnectTimer); - this.reconnectTimer = null; - } + this.reconnect.stop(); this.stopHeartbeat(); this.socket?.close(); @@ -186,17 +191,10 @@ export class SessionBrokerConnection< }; } - private scheduleReconnect(delayMs = this.options.reconnectDelayMs ?? DEFAULT_RECONNECT_DELAY_MS) { - if (this.reconnectTimer || this.stopped) { - return; + private scheduleReconnect(delayMs?: number) { + if (!this.stopped) { + this.reconnect.schedule(delayMs); } - - this.reconnectTimer = setTimeout(() => { - this.reconnectTimer = null; - this.connect(); - }, delayMs); - - this.reconnectTimer.unref?.(); } private startHeartbeat() { diff --git a/scripts/review-vocabulary.test.ts b/scripts/review-vocabulary.test.ts index 0ad57fab9..abd113397 100644 --- a/scripts/review-vocabulary.test.ts +++ b/scripts/review-vocabulary.test.ts @@ -22,7 +22,7 @@ * gate's tombstone and debt lists stay easy to audit. */ import { describe, expect, test } from "bun:test"; -import { readdirSync, readFileSync } from "node:fs"; +import { existsSync, readdirSync, readFileSync } from "node:fs"; import { join, resolve, sep } from "node:path"; import { MAX_WS_MESSAGE_BYTES } from "@hunk/session-broker-core"; import { REVIEW_INTENT_TYPES } from "../src/core/review/intents"; @@ -42,9 +42,13 @@ const REPO_ROOT = resolve(import.meta.dir, ".."); const REVIEW_MODEL_ROOT = join(REPO_ROOT, "src", "core", "review"); const SESSION_ROOT = join(REPO_ROOT, "src", "session"); const PRODUCER_ROOT = join(REPO_ROOT, "src", "app"); +const WEB_CLIENT_ROOT = join(REPO_ROOT, "src", "web"); -/** Every production TypeScript file below one directory. */ +/** Every production TypeScript file below one directory, tolerating an absent tree. */ function sourceFiles(directory: string): string[] { + if (!existsSync(directory)) { + return []; + } return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { const path = join(directory, entry.name); if (entry.isDirectory()) { @@ -112,6 +116,24 @@ describe("review constant derivation", () => { expect(collisions).toEqual([]); }); + // Phase 5's half of the same rule: a browser client is the tier furthest from the + // definitions and the one the audit found re-declaring the most — frame names, byte + // bounds, range sizes. It may import every shared constant, so it never needs to + // declare one that already exists. + test("no browser module re-declares a constant the shared tiers export", () => { + const shared = new Set([ + ...sourceFiles(REVIEW_MODEL_ROOT).flatMap(exportedConstants), + ...sourceFiles(SESSION_ROOT).flatMap(exportedConstants), + ]); + const collisions = sourceFiles(WEB_CLIENT_ROOT).flatMap((path) => + exportedConstants(path) + .filter((name) => shared.has(name)) + .map((name) => `${repoPath(path)} -> ${name}`), + ); + + expect(collisions).toEqual([]); + }); + // The canonical digest check lives in `core/review/validation.ts`; five inline patterns // with differing case sensitivity are what let a writer and a reader disagree about // whether two digests matched. The producer tier is scanned too, because it is the side @@ -122,6 +144,7 @@ describe("review constant derivation", () => { ...sourceFiles(SESSION_ROOT), ...sourceFiles(REVIEW_MODEL_ROOT), ...sourceFiles(PRODUCER_ROOT), + ...sourceFiles(WEB_CLIENT_ROOT), ] .filter((path) => repoPath(path) !== "src/core/review/validation.ts") .filter((path) => pattern.test(readFileSync(path, "utf8"))) diff --git a/scripts/source-boundaries.test.ts b/scripts/source-boundaries.test.ts index f22bffe3a..f7cf297ce 100644 --- a/scripts/source-boundaries.test.ts +++ b/scripts/source-boundaries.test.ts @@ -8,6 +8,17 @@ const CORE_ROOT = join(SRC_ROOT, "core"); const EXTENSIONS_ROOT = join(SRC_ROOT, "extensions"); const BUNDLED_PROVIDER_ROOT = join(EXTENSIONS_ROOT, "default", "vcs"); const REVIEW_MODEL_ROOT = join(CORE_ROOT, "review"); + +// Core modules outside the review model that a browser bundle may import, each for a +// stated reason. Deliberately a list rather than a directory: `src/core` also holds config +// resolution, VCS catalogs, and file I/O, none of which a browser has any business +// reaching. Every entry is walked by the platform-free closure check below like any other +// browser-reachable module. +const BROWSER_SAFE_CORE_MODULES: readonly string[] = [ + // A11: Hunk registers file extensions Pierre's own inference lacks, and a browser that + // never imports the registration highlights the same file differently from a terminal. + join(CORE_ROOT, "fileLanguage.ts"), +]; const REVIEW_PROTOCOL_PATH = join(SRC_ROOT, "session", "reviewProtocol.ts"); const WEB_CLIENT_ROOT = join(SRC_ROOT, "web"); @@ -194,6 +205,7 @@ const EXTRACTED_DUPLICATE_SYMBOLS: ReadonlyArray<{ { file: "src/ui/lib/agentAnnotations.ts", symbol: "annotationOverlapsHunk", finding: "B1" }, { file: "src/ui/lib/agentAnnotations.ts", symbol: "getAnnotatedHunkIndices", finding: "B1" }, { file: "src/ui/lib/reviewState.ts", symbol: "buildReviewAnnotationIndex", finding: "B1" }, + { file: "src/ui/lib/files.ts", symbol: "formatSidebarStat", finding: "E1" }, ]; describe("source architecture boundaries", () => { @@ -327,6 +339,7 @@ describe("shared review primitives seam", () => { escapingImports(WEB_CLIENT_ROOT, [ WEB_CLIENT_ROOT, REVIEW_MODEL_ROOT, + ...BROWSER_SAFE_CORE_MODULES, ...BROWSER_SAFE_SESSION_MODULES, ]), ).toEqual([]); @@ -337,7 +350,11 @@ describe("shared review primitives seam", () => { // the node-debt map's "a browser bundle must never import these until repaid" clause // mechanical rather than aspirational. test("keeps the browser-reachable module closure free of platform runtimes", () => { - const roots = [...sourceFiles(WEB_CLIENT_ROOT), ...BROWSER_SAFE_SESSION_MODULES]; + const roots = [ + ...sourceFiles(WEB_CLIENT_ROOT), + ...BROWSER_SAFE_CORE_MODULES, + ...BROWSER_SAFE_SESSION_MODULES, + ]; const violations = [...reachableSourceFiles(roots)].flatMap((path) => valueImportSpecifiers(path) .filter((specifier) => specifier.startsWith("node:") || specifier.startsWith("bun:")) diff --git a/src/app/review/resourceStore.ts b/src/app/review/resourceStore.ts index 236d95861..b38ee1a3c 100644 --- a/src/app/review/resourceStore.ts +++ b/src/app/review/resourceStore.ts @@ -18,6 +18,7 @@ * The store belongs to one publication, so a new generation starts with nothing cached and * the previous generation's bytes become collectable as soon as it is retired. */ +import { ConcurrencyGate } from "@hunk/session-broker-core"; import { SourceTextTooLargeError } from "../../core/fileSource"; import { isMaterializedReviewResource, @@ -79,15 +80,14 @@ export interface ReviewResourceStoreOptions { export class ReviewResourceStore { private readonly publication: ReviewPublication; private readonly digest: ReviewDigestFn; - private readonly concurrency: number; + /** The store's load slots, bounded by the shared gate rather than a local counter. */ + private readonly loadSlots: ConcurrencyGate; private readonly maxCacheBytes: number; /** Settled bytes, oldest first, so eviction is a plain iteration order. */ private readonly materialized = new Map(); /** One production per resource id; concurrent readers await this exact promise. */ private readonly inFlight = new Map>(); private cachedBytes = 0; - private active = 0; - private readonly waiting: Array<() => void> = []; constructor({ publication, @@ -97,7 +97,7 @@ export class ReviewResourceStore { }: ReviewResourceStoreOptions) { this.publication = publication; this.digest = digest; - this.concurrency = Math.max(1, concurrency); + this.loadSlots = new ConcurrencyGate(concurrency); this.maxCacheBytes = maxCacheBytes; } @@ -162,11 +162,11 @@ export class ReviewResourceStore { const results = new Map(); await Promise.all( [...new Set(resourceIds)].map(async (resourceId) => { - await this.acquire(); + await this.loadSlots.acquire(); try { results.set(resourceId, await this.materialize(resourceId)); } finally { - this.release(); + this.loadSlots.release(); } }), ); @@ -334,24 +334,4 @@ export class ReviewResourceStore { this.materialized.delete(resourceId); this.materialized.set(resourceId, resource); } - - /** Take one of the concurrency limit's slots, waiting when they are all in use. */ - private acquire(): Promise { - if (this.active < this.concurrency) { - this.active += 1; - return Promise.resolve(); - } - return new Promise((resolve) => { - this.waiting.push(() => { - this.active += 1; - resolve(); - }); - }); - } - - /** Give one slot back to the next waiting load. */ - private release() { - this.active -= 1; - this.waiting.shift()?.(); - } } diff --git a/src/core/review/document.ts b/src/core/review/document.ts index 29dc87743..1a53068fd 100644 --- a/src/core/review/document.ts +++ b/src/core/review/document.ts @@ -11,12 +11,7 @@ * they belong to the producer runtime that serves a document, not to the document itself. */ import type { DiffFile } from "../types"; -import { - reviewFileContentIdentity, - reviewFileKey, - reviewSourceIdentity, - type ReviewFileContentIdentityInput, -} from "./identity"; +import { reviewFileContentIdentity, reviewFileKey, reviewSourceIdentity } from "./identity"; import type { ReviewDocumentV1, ReviewFileChangeKind, @@ -86,32 +81,34 @@ function hunkSignature(hunks: readonly ReviewHunkV1[]) { .join(";"); } -/** Collect the renderer-neutral facts one file's content identity is hashed from. */ -function contentIdentityInput( - file: DiffFile, - hunks: readonly ReviewHunkV1[], -): ReviewFileContentIdentityInput { - return { +/** + * Recompute one already-projected file's content identity from the file itself. + * + * A reader that received a serialized `ReviewFileV1` needs to know that what arrived still + * describes the content it claims to. It cannot ask the manifest — that stays with the + * producer — but it can rehash the file and compare, and rehashing reads exactly the fields + * the model says a file's content is. That is the point: a reader with a field list of its + * own drifts from the producer's, which is how the prototype's `parseCanonicalReviewFile` + * came to check twelve fields against the producer's seventeen + * (`docs/browser-review-seam-audit.md`, D4). + * + * Projection uses the same function, so producer and reader cannot compute it differently. + */ +export function reviewFileContentIdentityOf( + file: Omit, +): string { + return reviewFileContentIdentity({ path: file.path, ...(file.previousPath !== undefined ? { previousPath: file.previousPath } : {}), - changeKind: file.metadata.type, + changeKind: file.changeKind, ...(file.language !== undefined ? { language: file.language } : {}), patch: file.patch, - stats: { - additions: file.stats.additions, - deletions: file.stats.deletions, - truncated: Boolean(file.statsTruncated), - }, - flags: { - untracked: Boolean(file.isUntracked), - binary: Boolean(file.isBinary), - tooLarge: Boolean(file.isTooLarge), - partial: Boolean(file.metadata.isPartial), - }, - hunkSignature: hunkSignature(hunks), - additionLines: file.metadata.additionLines, - deletionLines: file.metadata.deletionLines, - }; + stats: file.stats, + flags: file.flags, + hunkSignature: hunkSignature(file.hunks), + additionLines: file.additionLines, + deletionLines: file.deletionLines, + }); } export interface ProjectReviewDocumentOptions { @@ -125,8 +122,44 @@ export interface ProjectReviewDocumentOptions { /** Project one diff file into the semantic file the review model addresses. */ function projectReviewFile(file: DiffFile, sourceLabel: string, duplicateIndex: number) { const hunks = file.metadata.hunks.map(projectReviewHunk); - const identityInput = contentIdentityInput(file, hunks); - const contentIdentity = reviewFileContentIdentity(identityInput); + // The content half of the file is assembled first and then hashed, so the identity is a + // function of the projected file rather than of a parallel description of it — which is + // what lets a reader recompute it from what arrived (D4). + const content = { + path: file.path, + ...(file.previousPath !== undefined ? { previousPath: file.previousPath } : {}), + changeKind: file.metadata.type as ReviewFileChangeKind, + ...(file.language !== undefined ? { language: file.language } : {}), + ...(file.agent?.summary !== undefined ? { agentSummary: file.agent.summary } : {}), + stats: { + additions: file.stats.additions, + deletions: file.stats.deletions, + truncated: Boolean(file.statsTruncated), + }, + flags: { + untracked: Boolean(file.isUntracked), + binary: Boolean(file.isBinary), + tooLarge: Boolean(file.isTooLarge), + partial: Boolean(file.metadata.isPartial), + }, + patch: file.patch, + splitLineCount: file.metadata.splitLineCount, + unifiedLineCount: file.metadata.unifiedLineCount, + // Copied, not referenced: an extension that retains and later mutates the model it + // returned must not change a document the store already published. + additionLines: [...file.metadata.additionLines], + deletionLines: [...file.metadata.deletionLines], + ...(file.lineMoveKinds + ? { + lineMoveKinds: { + additionLines: file.lineMoveKinds.additionLines.map((kind) => kind ?? null), + deletionLines: file.lineMoveKinds.deletionLines.map((kind) => kind ?? null), + }, + } + : {}), + hunks, + }; + const contentIdentity = reviewFileContentIdentityOf(content); const sourceIdentity = file.sourceFetcher ? reviewSourceIdentity({ path: file.path, @@ -149,29 +182,7 @@ function projectReviewFile(file: DiffFile, sourceLabel: string, duplicateIndex: duplicateIndex, }), runtimeId: file.id, - path: file.path, - ...(file.previousPath !== undefined ? { previousPath: file.previousPath } : {}), - changeKind: identityInput.changeKind as ReviewFileChangeKind, - ...(file.language !== undefined ? { language: file.language } : {}), - ...(file.agent?.summary !== undefined ? { agentSummary: file.agent.summary } : {}), - stats: identityInput.stats, - flags: identityInput.flags, - patch: file.patch, - splitLineCount: file.metadata.splitLineCount, - unifiedLineCount: file.metadata.unifiedLineCount, - // Copied, not referenced: an extension that retains and later mutates the model it - // returned must not change a document the store already published. - additionLines: [...file.metadata.additionLines], - deletionLines: [...file.metadata.deletionLines], - ...(file.lineMoveKinds - ? { - lineMoveKinds: { - additionLines: file.lineMoveKinds.additionLines.map((kind) => kind ?? null), - deletionLines: file.lineMoveKinds.deletionLines.map((kind) => kind ?? null), - }, - } - : {}), - hunks, + ...content, contentIdentity, ...(sourceIdentity !== undefined ? { sourceIdentity, sourceAttested } : {}), } satisfies ReviewFileV1; diff --git a/src/core/review/expansion.test.ts b/src/core/review/expansion.test.ts index 4f262bd9d..f03be00b0 100644 --- a/src/core/review/expansion.test.ts +++ b/src/core/review/expansion.test.ts @@ -3,9 +3,11 @@ import { createTestReviewFile } from "../../../test/helpers/review-store-helpers import { parseReviewGapId, resolveReviewExpandedLine, + reviewExpandedGapLines, reviewExpansionSide, reviewGapAddress, reviewGapId, + reviewGapSourceForFile, reviewLeadingGap, reviewTrailingGap, type ReviewGapHunk, @@ -312,3 +314,55 @@ describe("resolveReviewExpandedLine", () => { ).toBeUndefined(); }); }); + +describe("reviewExpandedGapLines", () => { + const address = { + oldRange: [4, 8] as [number, number], + newRange: [6, 10] as [number, number], + lineCount: 5, + }; + + test("pairs both sides' lines and reads the source from the expansion side", () => { + expect(reviewExpandedGapLines(address, "new")).toEqual([ + { oldLine: 4, newLine: 6, sourceLine: 6 }, + { oldLine: 5, newLine: 7, sourceLine: 7 }, + { oldLine: 6, newLine: 8, sourceLine: 8 }, + { oldLine: 7, newLine: 9, sourceLine: 9 }, + { oldLine: 8, newLine: 10, sourceLine: 10 }, + ]); + expect(reviewExpandedGapLines(address, "old").map((line) => line.sourceLine)).toEqual([ + 4, 5, 6, 7, 8, + ]); + }); + + test("reveals nothing for a gap of no lines", () => { + expect(reviewExpandedGapLines({ ...address, lineCount: 0 }, "new")).toEqual([]); + }); + + // The two directions have to agree, or a surface expands lines it cannot then address: + // every line this reveals must resolve back to the gap it came from, and the line on + // either side of the gap must not. + test("reveals exactly the lines resolveReviewExpandedLine accepts", () => { + const file = createTestReviewFile({ key: "alpha", sourceIdentity: "src:1" }); + const gapId = "before:1"; + const resolved = reviewGapAddress(reviewGapSourceForFile(file), gapId)!; + + for (const side of ["old", "new"] as const) { + const lines = reviewExpandedGapLines(resolved, side); + const range = side === "old" ? resolved.oldRange : resolved.newRange; + + expect(lines).toHaveLength(resolved.lineCount); + for (const line of lines) { + const claim = { gapId, side, line: side === "old" ? line.oldLine : line.newLine }; + expect(resolveReviewExpandedLine(file, { ...claim, sourceIdentity: "src:1" })).toEqual( + resolved, + ); + } + for (const outside of [range[0] - 1, range[1] + 1]) { + expect( + resolveReviewExpandedLine(file, { gapId, side, line: outside, sourceIdentity: "src:1" }), + ).toBeUndefined(); + } + } + }); +}); diff --git a/src/core/review/expansion.ts b/src/core/review/expansion.ts index 721e288d1..aa04965f9 100644 --- a/src/core/review/expansion.ts +++ b/src/core/review/expansion.ts @@ -209,6 +209,44 @@ export function resolveReviewExpandedLine( return claim.line >= start && claim.line <= end ? address : undefined; } +/** + * One line an expanded gap reveals. + * + * Both sides are named because a gap renders as paired rows, and `sourceLine` says which + * line of the file's full text fills them — the same 1-based numbering + * `normalizedReviewSourceLines` produces. + */ +export interface ReviewExpandedGapLine { + oldLine: number; + newLine: number; + /** 1-based line number in the side's full source text. */ + sourceLine: number; +} + +/** + * Which source line each row of an expanded gap shows. + * + * The forward direction of `resolveReviewExpandedLine`: that answers "which gap does this + * line belong to", this answers "which lines does this gap reveal". Three renderers had + * derived it themselves, each pairing the two sides' ranges with the expansion side's by + * hand (`docs/browser-review-seam-audit.md`, A6), which is one offset error away from + * labelling every expanded line wrongly on one side. + * + * Deliberately text-free: it reports line numbers, and slicing the source with them — + * including what an out-of-range line means — stays with the surface that holds the text. + */ +export function reviewExpandedGapLines( + address: Pick, + side: ReviewSide, +): ReviewExpandedGapLine[] { + const [sourceStart] = side === "old" ? address.oldRange : address.newRange; + return Array.from({ length: Math.max(0, address.lineCount) }, (_unused, offset) => ({ + oldLine: address.oldRange[0] + offset, + newLine: address.newRange[0] + offset, + sourceLine: sourceStart + offset, + })); +} + /** * Which side's full source text fills this file's expanded gaps. * diff --git a/src/core/review/presentation.test.ts b/src/core/review/presentation.test.ts new file mode 100644 index 000000000..e8eb6ca24 --- /dev/null +++ b/src/core/review/presentation.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, test } from "bun:test"; +import { reviewFileStatBadges } from "./presentation"; + +describe("reviewFileStatBadges", () => { + test("states both deltas", () => { + expect(reviewFileStatBadges({ additions: 12, deletions: 3 })).toEqual({ + additionsText: "+12", + deletionsText: "-3", + }); + }); + + test("hides a zero rather than printing it", () => { + expect(reviewFileStatBadges({ additions: 0, deletions: 4 })).toEqual({ + additionsText: null, + deletionsText: "-4", + }); + expect(reviewFileStatBadges({ additions: 4, deletions: 0 })).toEqual({ + additionsText: "+4", + deletionsText: null, + }); + expect(reviewFileStatBadges({ additions: 0, deletions: 0 })).toEqual({ + additionsText: null, + deletionsText: null, + }); + }); + + test("marks truncation once, on the additions badge", () => { + expect(reviewFileStatBadges({ additions: 900, deletions: 800, truncated: true })).toEqual({ + additionsText: "+900+", + deletionsText: "-800", + }); + }); + + test("does not mark truncation on a file with nothing added", () => { + expect(reviewFileStatBadges({ additions: 0, deletions: 800, truncated: true })).toEqual({ + additionsText: null, + deletionsText: "-800", + }); + }); +}); diff --git a/src/core/review/presentation.ts b/src/core/review/presentation.ts new file mode 100644 index 000000000..a82445ce7 --- /dev/null +++ b/src/core/review/presentation.ts @@ -0,0 +1,47 @@ +/** + * Renderer-neutral formatting of the facts a review carries. + * + * Some presentation is not renderer-specific at all: how a file's line churn reads as a + * badge is the same string in a terminal row and in a browser sidebar, and the prototype + * had them differ — the terminal hid zero counts and marked truncation, the browser always + * printed `+n −n` (`docs/browser-review-seam-audit.md`, E1). The text is decided here so + * one review cannot describe the same file two ways depending on which surface is looking. + * + * Only text belongs in this module. Colors, widths, glyph choice, and placement are the + * renderer's, and nothing here reaches for a theme or measures a cell. + */ +/** The stats one file's badges are built from; `ReviewFileStatsV1` satisfies it. */ +export interface ReviewStatSubject { + additions: number; + deletions: number; + /** The producer counted more than it rendered, so the numbers are a lower bound. */ + truncated?: boolean; +} + +export interface ReviewFileStatBadges { + /** `+12`, or `+12+` when the count is a lower bound; null when nothing was added. */ + additionsText: string | null; + /** `-3`; null when nothing was deleted. */ + deletionsText: string | null; +} + +/** Render one signed count, hiding a zero so a badge only ever states a real delta. */ +function formatReviewStat(prefix: "+" | "-", value: number, truncated: boolean) { + return value > 0 ? `${prefix}${value}${truncated ? "+" : ""}` : null; +} + +/** + * The line-churn badges one reviewed file shows. + * + * Two policies, stated rather than implied by where the code is read from: a zero count is + * hidden entirely, and truncation is reported once — on the additions badge — because one + * marker per file is enough to say both numbers are lower bounds, and repeating it reads + * as two separate truncations. + */ +export function reviewFileStatBadges(stats: ReviewStatSubject): ReviewFileStatBadges { + const truncated = stats.truncated ?? false; + return { + additionsText: formatReviewStat("+", stats.additions, truncated), + deletionsText: formatReviewStat("-", stats.deletions, false), + }; +} diff --git a/src/core/review/resourceAssembly.ts b/src/core/review/resourceAssembly.ts index 1e4ac5aaf..5df05369b 100644 --- a/src/core/review/resourceAssembly.ts +++ b/src/core/review/resourceAssembly.ts @@ -109,6 +109,16 @@ export class ReviewChunkAssembler { return this.contentSize; } + /** + * The whole-resource digest the writer declared, once any chunk has declared one. + * + * Read by a caller that has to pass the measurement on to a reader of its own — the + * daemon serving assembled bytes over HTTP — rather than measuring them again. + */ + get declaredDigest() { + return this.contentDigest; + } + /** * Accept one chunk. * diff --git a/src/core/review/viewOptions.test.ts b/src/core/review/viewOptions.test.ts new file mode 100644 index 000000000..f47a31281 --- /dev/null +++ b/src/core/review/viewOptions.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, test } from "bun:test"; +import { + isClientReviewViewOption, + REVIEW_VIEW_OPTION_LOCUS, + type ReviewViewOptionLocus, +} from "./viewOptions"; + +describe("REVIEW_VIEW_OPTION_LOCUS", () => { + // Stated by hand rather than read back out of the table, so a classification changed by + // accident fails here instead of being re-asserted from itself. + const EXPECTED: Record = { + filter: "review", + showAgentNotes: "review", + mode: "client", + theme: "client", + showLineNumbers: "client", + wrapLines: "client", + showHunkHeaders: "client", + showMenuBar: "client", + copyDecorations: "client", + cursorLine: "client", + }; + + test("classifies every option exactly once", () => { + expect(REVIEW_VIEW_OPTION_LOCUS).toEqual(EXPECTED as typeof REVIEW_VIEW_OPTION_LOCUS); + }); + + test("agrees with the predicate clients gate their own overrides on", () => { + for (const [option, locus] of Object.entries(REVIEW_VIEW_OPTION_LOCUS)) { + expect(isClientReviewViewOption(option as keyof typeof REVIEW_VIEW_OPTION_LOCUS)).toBe( + locus === "client", + ); + } + }); +}); diff --git a/src/core/review/viewOptions.ts b/src/core/review/viewOptions.ts new file mode 100644 index 000000000..5408d049f --- /dev/null +++ b/src/core/review/viewOptions.ts @@ -0,0 +1,82 @@ +/** + * Which view options belong to the review and which belong to one client. + * + * With a terminal, a browser, and agents attached to one session, every option has to + * answer a question nobody had to ask while there was only a terminal: if I change this, + * does it change for everyone? The prototype answered it by accident — an option was + * shared if it happened to be read from shared state and per-client if it happened to be + * read from a local hook — and the browser then hardcoded its own defaults for the rest + * (`docs/browser-review-seam-audit.md`, G1). The classification is stated here instead, + * once, so both clients agree on what an option means before either renders it. + * + * The rule behind the table: an option is `review` when it changes what the review *is + * about* — which files are in view, whether their agent context is part of the reading — + * and `client` when it changes only how one screen draws it. Two people reading one review + * on two screens expect to see the same files; they do not expect one to be forced into + * the other's colors, column layout, or window size. + * + * This module classifies. It resolves nothing: reading user config, repo config, and CLI + * flags is the host's layered chain (`src/core/config.ts`), and a client's own overrides + * are its own storage. + */ +import type { PersistedViewPreferences } from "../types"; + +/** + * How a surface lays a diff out. + * + * One vocabulary for both tiers: `auto` is responsive — two columns when there is room for + * them — and an explicit choice overrides it. It lives with the option classification + * because a layout is one of the options being classified, and a browser that declared its + * own three-word union would be free to drift from the terminal's on what `auto` means. + */ +export type LayoutMode = "auto" | "split" | "stack"; + +/** + * Where one option's value lives. + * + * - `review` — part of the review every attached surface sees; changing it is a review + * action, and the producer broadcasts the result. + * - `client` — one surface's own view of that review; changing it reaches nobody else. + */ +export type ReviewViewOptionLocus = "review" | "client"; + +/** + * Every option a review surface carries, and where its value lives. + * + * Keyed over `PersistedViewPreferences` plus `filter`, which is review state rather than a + * persisted preference but is exactly the kind of option this table exists to classify. + * Totality is mechanical: an option added to the preferences without a locus fails to + * typecheck here rather than being silently treated as per-client by whichever surface + * reads it first. + */ +export const REVIEW_VIEW_OPTION_LOCUS: Record< + keyof PersistedViewPreferences | "filter", + ReviewViewOptionLocus +> = { + // What the review is about. + filter: "review", + showAgentNotes: "review", + // How one screen draws it. + mode: "client", + theme: "client", + showLineNumbers: "client", + wrapLines: "client", + showHunkHeaders: "client", + showMenuBar: "client", + copyDecorations: "client", + cursorLine: "client", +}; + +/** The options one client may set for itself without changing anyone else's review. */ +export type ClientReviewViewOption = { + [Key in keyof typeof REVIEW_VIEW_OPTION_LOCUS]: (typeof REVIEW_VIEW_OPTION_LOCUS)[Key] extends "client" + ? Key + : never; +}[keyof typeof REVIEW_VIEW_OPTION_LOCUS]; + +/** Whether one option is this client's own to change. */ +export function isClientReviewViewOption( + option: keyof typeof REVIEW_VIEW_OPTION_LOCUS, +): option is ClientReviewViewOption { + return REVIEW_VIEW_OPTION_LOCUS[option] === "client"; +} diff --git a/src/core/types.ts b/src/core/types.ts index ba35a5c25..2ff895f69 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -7,6 +7,7 @@ import type { NamedCustomThemeConfig, } from "../extension-api/types"; import type { FileSourceFetcher } from "./fileSource"; +import type { LayoutMode } from "./review/viewOptions"; import type { StartupNotice } from "./startupNotice"; import type { VcsCatalog } from "./vcs/types"; @@ -25,7 +26,8 @@ export type { NamedCustomThemeConfig, } from "../extension-api/types"; -export type LayoutMode = "auto" | "split" | "stack"; +// Declared with the shared view-option classification so both tiers name one union. +export type { LayoutMode }; export type CursorLine = "row" | "number" | "off"; export type VcsMode = string; export type TerminalThemeMode = "light" | "dark"; diff --git a/src/session/broker/brokerClient.ts b/src/session/broker/brokerClient.ts index 5cba7b01f..45e12a3bf 100644 --- a/src/session/broker/brokerClient.ts +++ b/src/session/broker/brokerClient.ts @@ -4,10 +4,12 @@ import { type SessionBrokerConnectionBridge, type SessionBrokerSocketLike, } from "@hunk/session-broker"; -import type { - SessionRegistration, - SessionServerMessage, - SessionSnapshot, +import { + createReconnectScheduler, + type ReconnectScheduler, + type SessionRegistration, + type SessionServerMessage, + type SessionSnapshot, } from "@hunk/session-broker-core"; import { SESSION_BROKER_SOCKET_PATH, @@ -57,7 +59,7 @@ export class SessionBrokerClient< Result > | null = null; private bridge: SessionAppBridge | null = null; - private reconnectTimer: ReturnType | null = null; + private readonly reconnect: ReconnectScheduler; private stopped = false; private startupPromise: Promise | null = null; private lastConnectionWarning: string | null = null; @@ -66,7 +68,15 @@ export class SessionBrokerClient< private registration: SessionRegistration, private snapshot: SessionSnapshot, private timing: SessionBrokerClientTiming = {}, - ) {} + ) { + // The daemon may not be up yet, which is a different failure from a dropped socket — + // but it is retried by the same shared timer, so this client no longer re-implements + // the scheduler of the connection it configures (C5). + this.reconnect = createReconnectScheduler({ + delayMs: timing.reconnectDelayMs ?? RECONNECT_DELAY_MS, + onDue: () => void this.start(), + }); + } start() { if (process.env.HUNK_MCP_DISABLE === "1") { @@ -95,10 +105,7 @@ export class SessionBrokerClient< stop() { this.stopped = true; - if (this.reconnectTimer) { - clearTimeout(this.reconnectTimer); - this.reconnectTimer = null; - } + this.reconnect.stop(); this.connection?.stop(); this.connection = null; @@ -224,16 +231,10 @@ export class SessionBrokerClient< this.connection.start(); } - private scheduleReconnect(delayMs = this.timing.reconnectDelayMs ?? RECONNECT_DELAY_MS) { - if (this.reconnectTimer || this.stopped) { - return; + private scheduleReconnect(delayMs?: number) { + if (!this.stopped) { + this.reconnect.schedule(delayMs); } - - this.reconnectTimer = setTimeout(() => { - this.reconnectTimer = null; - this.start(); - }, delayMs); - this.reconnectTimer.unref?.(); } /** Return whether the daemon explicitly rejected this session as incompatible after an upgrade. */ diff --git a/src/session/broker/brokerServer.ts b/src/session/broker/brokerServer.ts index dfe11c800..32c9cf0c0 100644 --- a/src/session/broker/brokerServer.ts +++ b/src/session/broker/brokerServer.ts @@ -10,7 +10,7 @@ import { isLoopbackHost, resolveSessionBrokerConfig, } from "./brokerConfig"; -import { BrowserReviewServer } from "./browserReviewServer"; +import { WebReviewServer } from "./webReviewServer"; import { createHunkSessionBrokerState, type HunkSessionBrokerState } from "./state"; import type { AppliedCommentBatchResult, @@ -466,7 +466,7 @@ export function serveSessionBrokerDaemon( options.staleSessionSweepIntervalMs ?? DEFAULT_STALE_SESSION_SWEEP_INTERVAL_MS; const state = createHunkSessionBrokerState(); // One loopback process serves every attached review, rather than a port per terminal. - const browserReview = new BrowserReviewServer(state); + const browserReview = new WebReviewServer(state); const daemon = createSessionBrokerDaemon({ broker: createHunkBrokerController(state), capabilities: { diff --git a/src/session/broker/reviewResourceCache.test.ts b/src/session/broker/reviewResourceCache.test.ts index 1b07690a1..435f9249a 100644 --- a/src/session/broker/reviewResourceCache.test.ts +++ b/src/session/broker/reviewResourceCache.test.ts @@ -7,17 +7,22 @@ const KEY = { resourceId: "resource:patch:file:a", }; const OTHER = { ...KEY, resourceId: "resource:patch:file:b" }; - -function bytes(length: number) { - return new Uint8Array(length); +const SHA256_HEX_LENGTH = 64; + +/** One loaded resource of the given size, with the digest its assembly verified against. */ +function resource(length: number) { + return { + bytes: new Uint8Array(length), + digest: "0".repeat(SHA256_HEX_LENGTH), + }; } describe("ReviewResourceCache", () => { test("returns what it stored", () => { const cache = new ReviewResourceCache(); - cache.store(KEY, bytes(4)); + cache.store(KEY, resource(4)); - expect(cache.get(KEY)).toEqual(bytes(4)); + expect(cache.get(KEY)).toEqual(resource(4)); expect(cache.get(OTHER)).toBeUndefined(); expect(cache.getCachedBytes()).toBe(4); }); @@ -25,20 +30,20 @@ describe("ReviewResourceCache", () => { // Intent: the daemon holds many sessions, so retained bytes are capped across all of them. test("evicts the least recently used entry to stay inside the byte budget", () => { const cache = new ReviewResourceCache({ cacheBytes: 10 }); - cache.store(KEY, bytes(6)); - cache.store(OTHER, bytes(6)); + cache.store(KEY, resource(6)); + cache.store(OTHER, resource(6)); expect(cache.get(KEY)).toBeUndefined(); - expect(cache.get(OTHER)).toEqual(bytes(6)); + expect(cache.get(OTHER)).toEqual(resource(6)); expect(cache.getCachedBytes()).toBe(6); }); test("promotes a read entry so it is not the next evicted", () => { const cache = new ReviewResourceCache({ cacheBytes: 10 }); - cache.store(KEY, bytes(4)); - cache.store(OTHER, bytes(4)); + cache.store(KEY, resource(4)); + cache.store(OTHER, resource(4)); cache.get(KEY); - cache.store({ ...KEY, resourceId: "resource:patch:file:c" }, bytes(4)); + cache.store({ ...KEY, resourceId: "resource:patch:file:c" }, resource(4)); expect(cache.get(KEY)).toBeDefined(); expect(cache.get(OTHER)).toBeUndefined(); @@ -91,9 +96,9 @@ describe("ReviewResourceCache", () => { test("drops everything belonging to a retired generation", () => { const cache = new ReviewResourceCache(); - cache.store(KEY, bytes(4)); + cache.store(KEY, resource(4)); const reservation = cache.reserve(OTHER, 4); - cache.store({ ...KEY, generation: "generation:p1:1" }, bytes(4)); + cache.store({ ...KEY, generation: "generation:p1:1" }, resource(4)); cache.evictGeneration("s-1", "generation:p1:0"); expect(cache.get(KEY)).toBeUndefined(); @@ -106,8 +111,8 @@ describe("ReviewResourceCache", () => { test("drops everything belonging to a departed session", () => { const cache = new ReviewResourceCache(); - cache.store(KEY, bytes(4)); - cache.store({ ...KEY, sessionId: "s-2" }, bytes(4)); + cache.store(KEY, resource(4)); + cache.store({ ...KEY, sessionId: "s-2" }, resource(4)); cache.evictSession("s-1"); expect(cache.getEntryCount()).toBe(1); @@ -116,7 +121,7 @@ describe("ReviewResourceCache", () => { test("clears completely on shutdown", () => { const cache = new ReviewResourceCache(); - cache.store(KEY, bytes(4)); + cache.store(KEY, resource(4)); cache.reserve(OTHER, 4); cache.clear(); diff --git a/src/session/broker/reviewResourceCache.ts b/src/session/broker/reviewResourceCache.ts index 906b333ea..5a55a60db 100644 --- a/src/session/broker/reviewResourceCache.ts +++ b/src/session/broker/reviewResourceCache.ts @@ -55,7 +55,19 @@ export class ReviewResourceBudgetError extends Error { interface CacheEntry { key: ReviewResourceKey; + resource: LoadedReviewResource; +} + +/** + * One assembled resource and the measurement it was verified against. + * + * The digest is kept with the bytes rather than recomputed by whoever serves them: the + * assembly already held the stream to it, and a second hash of a thirty-megabyte file per + * range request would be the cost of forgetting it. + */ +export interface LoadedReviewResource { bytes: Uint8Array; + digest: string; } /** Build one map key that cannot collide between two sessions or two generations. */ @@ -80,7 +92,7 @@ export class ReviewResourceCache { } /** Return one completed resource, promoting it to the young end of the eviction order. */ - get(key: ReviewResourceKey): Uint8Array | undefined { + get(key: ReviewResourceKey): LoadedReviewResource | undefined { const mapKey = cacheKey(key); const entry = this.entries.get(mapKey); if (!entry) { @@ -88,7 +100,7 @@ export class ReviewResourceCache { } this.entries.delete(mapKey); this.entries.set(mapKey, entry); - return entry.bytes; + return entry.resource; } /** @@ -169,22 +181,22 @@ export class ReviewResourceCache { * The resource just admitted is never the one evicted: a caller that asked for it is * about to use it, and dropping it to make room for itself would guarantee a re-read. */ - store(key: ReviewResourceKey, bytes: Uint8Array) { + store(key: ReviewResourceKey, resource: LoadedReviewResource) { const mapKey = cacheKey(key); const existing = this.entries.get(mapKey); if (existing) { - this.cachedBytes -= existing.bytes.byteLength; + this.cachedBytes -= existing.resource.bytes.byteLength; this.entries.delete(mapKey); } - this.entries.set(mapKey, { key, bytes }); - this.cachedBytes += bytes.byteLength; + this.entries.set(mapKey, { key, resource }); + this.cachedBytes += resource.bytes.byteLength; for (const [oldestKey, oldest] of this.entries) { if (this.cachedBytes <= this.limits.cacheBytes || oldestKey === mapKey) { break; } this.entries.delete(oldestKey); - this.cachedBytes -= oldest.bytes.byteLength; + this.cachedBytes -= oldest.resource.bytes.byteLength; } } @@ -232,7 +244,7 @@ export class ReviewResourceCache { for (const [mapKey, entry] of this.entries) { if (matches(entry.key)) { this.entries.delete(mapKey); - this.cachedBytes -= entry.bytes.byteLength; + this.cachedBytes -= entry.resource.bytes.byteLength; } } for (const [mapKey, reservation] of this.reservations) { diff --git a/src/session/broker/state.ts b/src/session/broker/state.ts index e4ed9d05b..df990e22f 100644 --- a/src/session/broker/state.ts +++ b/src/session/broker/state.ts @@ -42,7 +42,11 @@ import { readSnapshotReviewPublication, type MirroredReviewPublication, } from "./reviewMirror"; -import { ReviewResourceCache, type ReviewResourceReservation } from "./reviewResourceCache"; +import { + ReviewResourceCache, + type LoadedReviewResource, + type ReviewResourceReservation, +} from "./reviewResourceCache"; import { ReviewChunkAssembler } from "../../core/review/resourceAssembly"; import { isMaterializedReviewResource, @@ -63,6 +67,7 @@ import { type HunkReviewResourceReadResultV1, } from "../reviewProtocol"; import { + inBoundedParallel, SessionBrokerState, type SessionBrokerViewAdapter, type SessionTargetInput, @@ -140,23 +145,6 @@ function readRegistrationReviewCapabilityDigest(registrationInput: unknown): str : undefined; } -/** Run one bounded-parallel pass over a work list, in the shared load concurrency. */ -async function inBoundedParallel( - items: readonly Item[], - limit: number, - run: (item: Item) => Promise, -): Promise { - const results = Array.from({ length: items.length }) as Result[]; - let next = 0; - const workers = Array.from({ length: Math.min(Math.max(1, limit), items.length) }, async () => { - for (let index = next++; index < items.length; index = next++) { - results[index] = await run(items[index]!); - } - }); - await Promise.all(workers); - return results; -} - /** The generic broker this state specializes, named once so the overrides stay readable. */ type HunkBrokerBase = SessionBrokerState< HunkSessionInfo, @@ -185,7 +173,7 @@ export class HunkSessionBrokerState extends SessionBrokerState< private readonly mirror = new ReviewMirror(); private readonly resources: ReviewResourceCache; /** One load per resource; concurrent callers await the same assembly. */ - private readonly loads = new Map>(); + private readonly loads = new Map>(); /** Capability verifiers by session, as their registrations declared them. */ private readonly capabilityDigests = new Map(); private readonly publicationWatchers = new Set<(event: ReviewPublicationEvent) => void>(); @@ -349,7 +337,7 @@ export class HunkSessionBrokerState extends SessionBrokerState< sessionId: string, generation: string, resourceId: string, - ): Promise { + ): Promise { const descriptor = this.requireDescriptor(sessionId, generation, resourceId); const key = { sessionId, generation, resourceId }; const cached = this.resources.get(key); @@ -481,7 +469,7 @@ export class HunkSessionBrokerState extends SessionBrokerState< private async assembleResource( key: { sessionId: string; generation: string; resourceId: string }, descriptor: ReviewResourceDescriptorV1, - ): Promise { + ): Promise { const measured = isMaterializedReviewResource(descriptor); const reservation: ReviewResourceReservation = this.resources.reserve( key, @@ -526,8 +514,11 @@ export class HunkSessionBrokerState extends SessionBrokerState< throw new ReviewResourceReadError(assembled.code, assembled.message); } this.assertGenerationActive(key.sessionId, key.generation); - this.resources.store(key, assembled.bytes); - return assembled.bytes; + // The digest the stream declared is what the bytes were just verified against, so it + // travels with them instead of being recomputed by whoever serves them next. + const loaded = { bytes: assembled.bytes, digest: assembler.declaredDigest! }; + this.resources.store(key, loaded); + return loaded; } finally { this.resources.release(reservation); } @@ -567,7 +558,7 @@ export class HunkSessionBrokerState extends SessionBrokerState< if (!fileKey) { throw new Error(reviewResourceUnavailableMessage(file.path)); } - const bytes = await this.loadReviewResource( + const { bytes } = await this.loadReviewResource( sessionId, publication.address.generation, reviewResourceId({ kind: "patch", fileKey }), diff --git a/src/session/broker/browserReviewServer.integration.test.ts b/src/session/broker/webReviewServer.integration.test.ts similarity index 95% rename from src/session/broker/browserReviewServer.integration.test.ts rename to src/session/broker/webReviewServer.integration.test.ts index 4abb079a2..ad13ae23d 100644 --- a/src/session/broker/browserReviewServer.integration.test.ts +++ b/src/session/broker/webReviewServer.integration.test.ts @@ -26,6 +26,7 @@ import { parseReviewEventFrame, parseReviewEventFrameName, ReviewEventAssembler, + ReviewEventSseDecoder, reviewEventId, } from "../reviewEventProtocol"; import { @@ -35,13 +36,12 @@ import { type HunkReviewHttpRoute, } from "../reviewHttpProtocol"; import { HUNK_REVIEW_PROTOCOL_VERSION } from "../reviewProtocol"; -import { BrowserReviewServer, type BrowserReviewServerOptions } from "./browserReviewServer"; +import { WebReviewServer, type WebReviewServerOptions } from "./webReviewServer"; const SESSION_ID = "session-http-1"; const ACTOR = { clientId: "test-client", kind: "browser" } as const; -const running: Array<{ review: BrowserReviewServer; server: { stop: (force?: boolean) => void } }> = - []; +const running: Array<{ review: WebReviewServer; server: { stop: (force?: boolean) => void } }> = []; afterEach(() => { for (const entry of running.splice(0)) { @@ -53,9 +53,9 @@ afterEach(() => { /** Mount the review surface on a real loopback listener. */ function serve( harness: ReturnType, - options: BrowserReviewServerOptions = {}, + options: WebReviewServerOptions = {}, ) { - const review = new BrowserReviewServer(harness.state, options); + const review = new WebReviewServer(harness.state, options); const server = Bun.serve({ hostname: "127.0.0.1", port: 0, @@ -67,10 +67,7 @@ function serve( } /** Connect a session, register it, and mount the surface over it. */ -function start( - files = [createTestPatchFile("alpha", 4)], - options: BrowserReviewServerOptions = {}, -) { +function start(files = [createTestPatchFile("alpha", 4)], options: WebReviewServerOptions = {}) { const harness = connectReviewSession(files, { sessionId: SESSION_ID }); harness.register(); return { harness, ...serve(harness, options) }; @@ -491,29 +488,22 @@ describe("browser review surface: actions", () => { */ async function readEvents(response: Response, events: number) { const reader = response.body!.getReader(); - const decoder = new TextDecoder(); + const textDecoder = new TextDecoder(); + const records = new ReviewEventSseDecoder(); const frames: Array<{ id?: string; event: string; data: unknown }> = []; let complete = 0; - let buffer = ""; while (complete < events) { const { value, done } = await reader.read(); if (done) { break; } - buffer += decoder.decode(value, { stream: true }); - let boundary = buffer.indexOf("\n\n"); - while (boundary >= 0) { - const record = buffer.slice(0, boundary); - buffer = buffer.slice(boundary + 2); - const lines = record.split("\n"); - const id = lines.find((line) => line.startsWith("id: "))?.slice(4); - const event = lines.find((line) => line.startsWith("event: "))?.slice(7); - const data = lines.find((line) => line.startsWith("data: "))?.slice(6); - if (event && data !== undefined) { - frames.push({ ...(id ? { id } : {}), event, data: JSON.parse(data) as unknown }); - complete += id ? 1 : 0; - } - boundary = buffer.indexOf("\n\n"); + for (const record of records.push(textDecoder.decode(value, { stream: true }))) { + frames.push({ + ...(record.id === undefined ? {} : { id: record.id }), + event: record.event, + data: JSON.parse(record.data) as unknown, + }); + complete += record.id === undefined ? 0 : 1; } } await reader.cancel(); diff --git a/src/session/broker/browserReviewServer.ts b/src/session/broker/webReviewServer.ts similarity index 94% rename from src/session/broker/browserReviewServer.ts rename to src/session/broker/webReviewServer.ts index eb10d59a9..0bb735ba9 100644 --- a/src/session/broker/browserReviewServer.ts +++ b/src/session/broker/webReviewServer.ts @@ -42,7 +42,6 @@ import { nodeReviewDigest } from "../../lib/reviewDigest"; import { REVIEW_RESOURCE_CHUNK_BYTES } from "../../core/review/resources"; import type { ReviewPublicationAddress } from "../../core/review/generationOrder"; import { isReviewSha256Digest } from "../../core/review/validation"; -import { reviewErrorMessage } from "../reviewErrorCatalog"; import { encodeReviewEventFrame, planReviewEventFrames, @@ -57,8 +56,10 @@ import { HUNK_REVIEW_HTTP_PATH_PREFIX, isReviewCapabilityToken, parseReviewHttpPath, + reviewContentMeasurementHeaders, + REVIEW_ERROR_STATUS, + reviewHttpFailure, type HunkReviewClientErrorCodeV1, - type HunkReviewHttpFailureV1, type HunkReviewHttpRoute, type HunkReviewPublicationBodyV1, } from "../reviewHttpProtocol"; @@ -68,6 +69,7 @@ import { parseHunkReviewActionEnvelope, } from "../reviewProtocol"; import { allowsUnsafeRemoteSessionBroker, isLoopbackHost } from "./brokerConfig"; +import type { LoadedReviewResource } from "./reviewResourceCache"; import { ReviewGenerationRetiredError, ReviewResourceReadError, @@ -75,36 +77,6 @@ import { type ReviewPublicationEvent, } from "./state"; -/** - * HTTP status each failure is reported with. - * - * Total over the code union, so a code added to any tier's vocabulary cannot reach this - * surface without someone deciding what it means to a client. - */ -const REVIEW_ERROR_STATUS: Record = { - "stale-generation": 409, - "invalid-request": 400, - "unsupported-action": 400, - "file-not-found": 404, - "hunk-not-found": 404, - "gap-not-found": 404, - "draft-missing": 409, - "note-not-found": 404, - "missing-fact": 400, - "unknown-resource": 404, - "resource-unavailable": 502, - "resource-too-large": 413, - "resource-integrity": 502, - "invalid-range": 416, - unauthorized: 401, - "no-publication": 409, - "payload-too-large": 413, - "method-not-allowed": 405, - "unsupported-media-type": 415, - "forbidden-origin": 403, - "too-many-streams": 503, -}; - /** * Headers every review response carries. * @@ -136,7 +108,7 @@ const DEFAULT_MAX_STREAMS_PER_SESSION = 8; */ const DEFAULT_MAX_STREAM_BUFFER_BYTES = 2 * MAX_HUNK_REVIEW_ENVELOPE_BYTES; -export interface BrowserReviewServerOptions { +export interface WebReviewServerOptions { heartbeatMs?: number; maxStreams?: number; maxStreamsPerSession?: number; @@ -185,7 +157,7 @@ function digestsMatchInConstantTime(presented: string, expected: string | undefi } /** Serve one daemon's review surface: publication, resources, events, and actions. */ -export class BrowserReviewServer { +export class WebReviewServer { private readonly streams = new Set(); private readonly unsubscribe: () => void; private readonly heartbeat: ReturnType; @@ -193,7 +165,7 @@ export class BrowserReviewServer { constructor( private readonly state: HunkSessionBrokerState, - private readonly options: BrowserReviewServerOptions = {}, + private readonly options: WebReviewServerOptions = {}, ) { this.unsubscribe = state.subscribeReviewPublications((event) => this.observe(event)); this.heartbeat = setInterval( @@ -346,9 +318,9 @@ export class BrowserReviewServer { return this.rangeNotSatisfiable(undefined); } - let bytes: Uint8Array; + let loaded: LoadedReviewResource; try { - bytes = await this.state.loadReviewResource( + loaded = await this.state.loadReviewResource( route.sessionId, route.generation, route.resourceId, @@ -356,6 +328,7 @@ export class BrowserReviewServer { } catch (error) { return this.resourceFailure(error); } + const bytes = loaded.bytes; const range = rangeHeader === null ? undefined : parseByteRange(rangeHeader); if (range && (range.start >= bytes.byteLength || range.start > (range.end ?? Infinity))) { @@ -381,6 +354,12 @@ export class BrowserReviewServer { headers: this.headers({ "accept-ranges": "bytes", "content-type": descriptor?.contentType ?? "application/octet-stream", + // Every window states the measurement of the whole resource, so a reader + // assembling several of them holds them all to one size and digest. + ...reviewContentMeasurementHeaders({ + byteLength: bytes.byteLength, + digest: loaded.digest, + }), ...(partial ? { "content-range": `bytes ${start}-${Math.max(start, end)}/${bytes.byteLength}` } : {}), @@ -663,20 +642,14 @@ export class BrowserReviewServer { /** * Answer one failure in the shape every review route uses. * - * The message comes from the shared catalog unless the producer supplied a more specific - * one, so a client never has to invent wording for a code (G4). + * Both the body and the status are the HTTP contract's, so the client rebuilding this + * refusal and the surface writing it cannot disagree about either (G4). */ private failure( code: HunkReviewClientErrorCodeV1, details: { message?: string; currentGeneration?: string } = {}, ) { - const body: HunkReviewHttpFailureV1 = { - ok: false, - code, - message: details.message ?? reviewErrorMessage(code), - ...(details.currentGeneration ? { currentGeneration: details.currentGeneration } : {}), - }; - return this.json(body, REVIEW_ERROR_STATUS[code]); + return this.json(reviewHttpFailure(code, details), REVIEW_ERROR_STATUS[code]); } /** Report one resource read failure with the code that says how it failed. */ diff --git a/src/session/reviewEventProtocol.test.ts b/src/session/reviewEventProtocol.test.ts index 03fd3204f..73f4618fd 100644 --- a/src/session/reviewEventProtocol.test.ts +++ b/src/session/reviewEventProtocol.test.ts @@ -12,6 +12,8 @@ import { parseReviewEventFrameName, parseReviewEventId, planReviewEventFrames, + REVIEW_EVENT_HEARTBEAT_FRAME, + ReviewEventSseDecoder, ReviewEventAssembler, ReviewEventTooLargeError, reviewEventChunkCount, @@ -299,3 +301,85 @@ describe("review event envelope parsing", () => { expect(parseReviewEventEnd(withoutCount)).toBeUndefined(); }); }); + +describe("review event record decoding", () => { + /** Feed one text stream through the decoder in fixed-size reads, as a transport does. */ + function decodeInReads(text: string, readSize: number) { + const decoder = new ReviewEventSseDecoder(); + const records = []; + for (let offset = 0; offset < text.length; offset += readSize) { + records.push(...decoder.push(text.slice(offset, offset + readSize))); + } + return records; + } + + test("reads back exactly the fields one encoded frame carries", () => { + const written = { id: "revent:x", event: "publication", data: { a: 1, b: "two" } }; + const [record] = new ReviewEventSseDecoder().push(encodeReviewEventFrame(written)); + + expect(record).toEqual({ id: "revent:x", event: "publication", data: '{"a":1,"b":"two"}' }); + expect(JSON.parse(record!.data)).toEqual(written.data); + }); + + test("leaves off the id when the frame that was written had none", () => { + const [record] = new ReviewEventSseDecoder().push( + encodeReviewEventFrame({ event: "publication-chunk", data: { offset: 0 } }), + ); + + expect(record).toEqual({ event: "publication-chunk", data: '{"offset":0}' }); + expect("id" in record!).toBe(false); + }); + + test("reads every frame of a chunked event out of one write", () => { + const frames = frame({ value: "z".repeat(200) }, 32); + const text = frames.map(encodeReviewEventFrame).join(""); + + const records = new ReviewEventSseDecoder().push(text); + + expect(records.map((record) => record.event)).toEqual(frames.map((entry) => entry.event)); + expect(records.map((record) => record.id)).toEqual(frames.map((entry) => entry.id)); + }); + + // A transport hands over whatever bytes arrived, so every record boundary has to survive + // landing in the middle of a read — the split that a re-derived grammar gets wrong. + test("holds a record until the read that completes it", () => { + const frames = frame({ value: "w".repeat(200) }, 48); + const text = frames.map(encodeReviewEventFrame).join(""); + const whole = new ReviewEventSseDecoder().push(text); + + for (const readSize of [1, 3, 7, 64, text.length]) { + expect(decodeInReads(text, readSize)).toEqual(whole); + } + }); + + test("emits nothing for a record whose separator has not arrived", () => { + const decoder = new ReviewEventSseDecoder(); + const text = encodeReviewEventFrame({ id: "revent:y", event: "publication", data: 1 }); + + expect(decoder.push(text.slice(0, -1))).toEqual([]); + expect(decoder.push(text.slice(-1))).toEqual([ + { id: "revent:y", event: "publication", data: "1" }, + ]); + }); + + test("drops the heartbeat, which carries no event or data of its own", () => { + const decoder = new ReviewEventSseDecoder(); + const framed = encodeReviewEventFrame({ event: "disconnect", data: {} }); + + expect(decoder.push(`${REVIEW_EVENT_HEARTBEAT_FRAME}${framed}`)).toEqual([ + { event: "disconnect", data: "{}" }, + ]); + }); + + // The writer escapes every newline into the `data:` line, so a payload containing a + // record separator must still read back as exactly one record. + test("keeps a payload that contains a record separator in one record", () => { + const data = { note: "one\n\ntwo" }; + const records = new ReviewEventSseDecoder().push( + encodeReviewEventFrame({ event: "publication", data }), + ); + + expect(records).toHaveLength(1); + expect(JSON.parse(records[0]!.data)).toEqual(data); + }); +}); diff --git a/src/session/reviewEventProtocol.ts b/src/session/reviewEventProtocol.ts index ad6de51ce..ead906496 100644 --- a/src/session/reviewEventProtocol.ts +++ b/src/session/reviewEventProtocol.ts @@ -325,6 +325,75 @@ export function encodeReviewEventFrame(frame: ReviewEventSseFrame): string { return `${frame.id ? `id: ${frame.id}\n` : ""}event: ${frame.event}\ndata: ${JSON.stringify(frame.data)}\n\n`; } +/** One record read back off the wire: the three fields this protocol writes, and nothing else. */ +export interface ReviewEventSseRecord { + /** Present only on a record that completes an event, exactly as `encodeReviewEventFrame` writes it. */ + id?: string; + event: string; + /** The `data:` line as text; parsing it is the reader's, since only it knows the frame. */ + data: string; +} + +/** The record separator and per-field prefixes `encodeReviewEventFrame` writes. */ +const SSE_RECORD_SEPARATOR = "\n\n"; +const SSE_FIELD_PREFIXES = { id: "id: ", event: "event: ", data: "data: " } as const; + +/** Read one field off a record's lines, or undefined when the record does not carry it. */ +function readSseField(lines: readonly string[], field: keyof typeof SSE_FIELD_PREFIXES) { + const prefix = SSE_FIELD_PREFIXES[field]; + return lines.find((line) => line.startsWith(prefix))?.slice(prefix.length); +} + +/** + * Read one complete record's text into the fields it carries. + * + * A record without both an `event` and a `data` is not one this protocol wrote — the + * heartbeat is a bare comment line — so it is dropped rather than half-read. + */ +function parseReviewEventSseRecord(text: string): ReviewEventSseRecord | undefined { + const lines = text.split("\n"); + const event = readSseField(lines, "event"); + const data = readSseField(lines, "data"); + if (event === undefined || data === undefined) { + return undefined; + } + const id = readSseField(lines, "id"); + return { ...(id === undefined ? {} : { id }), event, data }; +} + +/** + * Split a stream of text into the records it carries, across as many reads as it takes. + * + * The counterpart of `encodeReviewEventFrame`, and the reason it lives beside it: a + * transport reads whatever bytes arrive, so a record can be split across two reads and two + * records can arrive in one. The splitter keeps the tail that has no separator yet and + * emits nothing until it does, which is what makes a partial `data:` line impossible to + * mistake for a whole one. Every consumer that reads this stream — the browser client and + * the suites that watch the server — decodes with this rather than re-deriving the + * prefixes and offsets (`docs/browser-review-seam-audit.md`, C4). + */ +export class ReviewEventSseDecoder { + private buffer = ""; + + /** Take one more piece of stream text, returning every record it completed. */ + push(text: string): ReviewEventSseRecord[] { + this.buffer += text; + const records: ReviewEventSseRecord[] = []; + for ( + let boundary = this.buffer.indexOf(SSE_RECORD_SEPARATOR); + boundary >= 0; + boundary = this.buffer.indexOf(SSE_RECORD_SEPARATOR) + ) { + const record = parseReviewEventSseRecord(this.buffer.slice(0, boundary)); + this.buffer = this.buffer.slice(boundary + SSE_RECORD_SEPARATOR.length); + if (record) { + records.push(record); + } + } + return records; + } +} + /** How many chunks one payload of the given size is sent as. */ export function reviewEventChunkCount(contentSize: number) { return Math.max(1, Math.ceil(contentSize / REVIEW_EVENT_CHUNK_BYTES)); diff --git a/src/session/reviewHttpProtocol.test.ts b/src/session/reviewHttpProtocol.test.ts index e7d89a8b4..bd0ce2cd4 100644 --- a/src/session/reviewHttpProtocol.test.ts +++ b/src/session/reviewHttpProtocol.test.ts @@ -2,7 +2,11 @@ import { describe, expect, test } from "bun:test"; import { createReviewCapability } from "../app/review/capability"; import { HUNK_REVIEW_CAPABILITY_FRAGMENT_KEY, + HUNK_REVIEW_CONTENT_DIGEST_HEADER, + HUNK_REVIEW_CONTENT_SIZE_HEADER, isReviewCapabilityToken, + parseReviewContentMeasurementHeaders, + reviewContentMeasurementHeaders, parseReviewCapabilityFragment, parseReviewHttpPath, REVIEW_CAPABILITY_TOKEN_LENGTH, @@ -109,3 +113,47 @@ describe("review url", () => { ).toBeUndefined(); }); }); + +describe("resource measurement headers", () => { + const MEASUREMENT = { byteLength: 4_096, digest: "a".repeat(64) }; + + test("round-trips one whole-resource measurement", () => { + const headers = new Headers(reviewContentMeasurementHeaders(MEASUREMENT)); + + expect(parseReviewContentMeasurementHeaders(headers)).toEqual(MEASUREMENT); + }); + + test("reads a zero-length resource, which has no satisfiable range at all", () => { + const headers = new Headers( + reviewContentMeasurementHeaders({ byteLength: 0, digest: MEASUREMENT.digest }), + ); + + expect(parseReviewContentMeasurementHeaders(headers)).toEqual({ + byteLength: 0, + digest: MEASUREMENT.digest, + }); + }); + + test("refuses a response that states no measurement, or an unusable one", () => { + expect(parseReviewContentMeasurementHeaders(new Headers())).toBeUndefined(); + expect( + parseReviewContentMeasurementHeaders( + new Headers({ [HUNK_REVIEW_CONTENT_DIGEST_HEADER]: MEASUREMENT.digest }), + ), + ).toBeUndefined(); + // Uppercase hex is exactly the drift the canonical form exists to refuse. + expect( + parseReviewContentMeasurementHeaders( + new Headers(reviewContentMeasurementHeaders({ ...MEASUREMENT, digest: "A".repeat(64) })), + ), + ).toBeUndefined(); + expect( + parseReviewContentMeasurementHeaders( + new Headers({ + ...reviewContentMeasurementHeaders(MEASUREMENT), + [HUNK_REVIEW_CONTENT_SIZE_HEADER]: "not-a-number", + }), + ), + ).toBeUndefined(); + }); +}); diff --git a/src/session/reviewHttpProtocol.ts b/src/session/reviewHttpProtocol.ts index 405e2e3d9..c6a0a7b02 100644 --- a/src/session/reviewHttpProtocol.ts +++ b/src/session/reviewHttpProtocol.ts @@ -28,7 +28,8 @@ import { HUNK_REVIEW_PROTOCOL_VERSION, MAX_HUNK_REVIEW_IDENTIFIER_BYTES } from "./reviewProtocol"; import type { HunkReviewFailureCodeV1, HunkReviewResourceCatalogV1 } from "./reviewProtocol"; import type { ReviewPublicationAddress } from "../core/review/generationOrder"; -import { utf8ByteLength } from "../core/review/validation"; +import { isReviewSha256Digest, utf8ByteLength } from "../core/review/validation"; +import { reviewErrorMessage } from "./reviewErrorCatalog"; /** Path every review route hangs from, so a client never assembles route strings itself. */ export const HUNK_REVIEW_HTTP_PATH_PREFIX = "/review-api"; @@ -74,6 +75,60 @@ export function isReviewCapabilityToken(value: unknown): value is string { ); } +/** + * Headers a resource response states its whole-resource measurement in. + * + * A published catalog describes resources the producer has not measured yet — measuring + * one means producing its bytes — so a reader cannot get the size and digest to verify a + * read from the catalog it addressed the read with. They travel with the bytes instead, + * on every window of them, which is what lets a client hold a multi-window read to one + * measurement through the shared `ReviewChunkAssembler` rather than trusting whatever + * arrives (`docs/browser-review-seam-audit.md`, C2). + * + * Deliberately about the whole resource, never about the slice: `content-range` and + * `content-length` already describe the slice, and a per-window digest would let a + * truncated read verify. + */ +export const HUNK_REVIEW_CONTENT_SIZE_HEADER = "hunk-review-content-size"; +export const HUNK_REVIEW_CONTENT_DIGEST_HEADER = "hunk-review-content-digest"; + +/** One resource's whole-content measurement, as the surface states and a reader parses it. */ +export interface HunkReviewContentMeasurement { + byteLength: number; + digest: string; +} + +/** Render one measurement as the headers every window of a resource carries. */ +export function reviewContentMeasurementHeaders({ + byteLength, + digest, +}: HunkReviewContentMeasurement): Record { + return { + [HUNK_REVIEW_CONTENT_SIZE_HEADER]: String(byteLength), + [HUNK_REVIEW_CONTENT_DIGEST_HEADER]: digest, + }; +} + +/** + * Read one measurement back off a response, or nothing when it does not state one. + * + * Validated rather than trusted: the digest must be in the canonical form the shared + * validator accepts, so a reader cannot adopt a measurement it would later be unable to + * compare against (D5). + */ +export function parseReviewContentMeasurementHeaders( + headers: Headers, +): HunkReviewContentMeasurement | undefined { + const digest = headers.get(HUNK_REVIEW_CONTENT_DIGEST_HEADER); + const size = headers.get(HUNK_REVIEW_CONTENT_SIZE_HEADER); + // Read as a decimal integer rather than through `Number`, which reads an absent header + // as zero — a size a zero-length resource legitimately has. + const byteLength = size !== null && /^\d{1,15}$/.test(size) ? Number(size) : Number.NaN; + return isReviewSha256Digest(digest) && Number.isSafeInteger(byteLength) + ? { byteLength, digest } + : undefined; +} + /** One route on the review surface, named by what it serves rather than by its path. */ export type HunkReviewHttpRoute = | { kind: "publication"; sessionId: string } @@ -220,6 +275,82 @@ export interface HunkReviewHttpFailureV1 { currentGeneration?: string; } +/** + * HTTP status each failure is reported with. + * + * Total over the code union, so a code added to any tier's vocabulary cannot reach this + * surface without someone deciding what it means to a client. It lives with the rest of + * the HTTP contract rather than in the server, because a client reading a body-less + * refusal has to answer the same question in the other direction. + */ +export const REVIEW_ERROR_STATUS: Record = { + "stale-generation": 409, + "invalid-request": 400, + "unsupported-action": 400, + "file-not-found": 404, + "hunk-not-found": 404, + "gap-not-found": 404, + "draft-missing": 409, + "note-not-found": 404, + "missing-fact": 400, + "unknown-resource": 404, + "resource-unavailable": 502, + "resource-too-large": 413, + "resource-integrity": 502, + "invalid-range": 416, + unauthorized: 401, + "no-publication": 409, + "payload-too-large": 413, + "method-not-allowed": 405, + "unsupported-media-type": 415, + "forbidden-origin": 403, + "too-many-streams": 503, +}; + +/** Statuses exactly one code claims, derived from the table rather than listed again. */ +const REVIEW_ERROR_STATUS_INVERSE: ReadonlyMap = (() => { + const claims = new Map(); + for (const [code, status] of Object.entries(REVIEW_ERROR_STATUS) as Array< + [HunkReviewClientErrorCodeV1, number] + >) { + claims.set(status, [...(claims.get(status) ?? []), code]); + } + return new Map( + [...claims].flatMap(([status, codes]) => (codes.length === 1 ? [[status, codes[0]!]] : [])), + ); +})(); + +/** + * The one code a status stands for, when the table gives it only one. + * + * A route that refuses without a body — an unsatisfiable range is answered with a bare 416 + * — leaves a client nothing but the status to read. Several codes share 400, 404, and 409, + * and picking one of those would be inventing an answer, so this reports only where the + * table is unambiguous and leaves the caller to say what an ambiguous status means to it. + */ +export function reviewErrorCodeForStatus(status: number): HunkReviewClientErrorCodeV1 | undefined { + return REVIEW_ERROR_STATUS_INVERSE.get(status); +} + +/** + * Build one refusal in the shape every review route answers with. + * + * The message comes from the shared catalog unless a tier supplied a more specific one, so + * no consumer — the surface answering, or a client rebuilding what it was told — has to + * invent wording for a code (`docs/browser-review-seam-audit.md`, G4). + */ +export function reviewHttpFailure( + code: HunkReviewClientErrorCodeV1, + details: { message?: string; currentGeneration?: string } = {}, +): HunkReviewHttpFailureV1 { + return { + ok: false, + code, + message: details.message ?? reviewErrorMessage(code), + ...(details.currentGeneration ? { currentGeneration: details.currentGeneration } : {}), + }; +} + /** * One publication as the surface serves it: where the review is, and what it offers there. * diff --git a/src/session/reviewProtocol.ts b/src/session/reviewProtocol.ts index 983acb81e..073242a26 100644 --- a/src/session/reviewProtocol.ts +++ b/src/session/reviewProtocol.ts @@ -590,6 +590,13 @@ export interface HunkReviewResourceCatalogV1 { * without guessing; it retires when the surface itself adopts semantic keys. */ fileKeysByRuntimeId: Record; + /** + * Every resource this generation offers, in review order. + * + * The order is part of the contract, not an accident of how the producer built the list: + * a review's file order is its sidebar and stream order, and a client reading a document + * out of this catalog has nowhere else to learn it from. + */ resources: ReviewResourceDescriptorV1[]; } diff --git a/src/ui/components/panes/DiffFileHeaderRow.tsx b/src/ui/components/panes/DiffFileHeaderRow.tsx index 80aea2c8c..2db9e661d 100644 --- a/src/ui/components/panes/DiffFileHeaderRow.tsx +++ b/src/ui/components/panes/DiffFileHeaderRow.tsx @@ -48,10 +48,20 @@ export function DiffFileHeaderRow({ justifyContent: "flex-end", }} > - {additionsText} - - {deletionsText} - + {/* A badge is drawn only when it states a real delta, each trailed by the one + space `fileHeaderStats` measured, so the row and the copied text agree. */} + {additionsText !== null && ( + <> + {additionsText} + + + )} + {deletionsText !== null && ( + <> + {deletionsText} + + + )} ); diff --git a/src/ui/diff/expandCollapsedRows.ts b/src/ui/diff/expandCollapsedRows.ts index e02f70727..0f6b7e4a7 100644 --- a/src/ui/diff/expandCollapsedRows.ts +++ b/src/ui/diff/expandCollapsedRows.ts @@ -1,4 +1,4 @@ -import { reviewGapId } from "../../core/review/expansion"; +import { reviewExpandedGapLines, reviewGapId } from "../../core/review/expansion"; import { normalizedReviewSourceLines } from "../../core/review/geometry"; import { DEFAULT_TAB_WIDTH } from "../../core/tabWidth"; import { sanitizeTerminalLine, sanitizeTerminalSpans } from "../../lib/terminalText"; @@ -185,10 +185,16 @@ export function expandCollapsedRows( text: expandedRowText(lineCount), }); - for (let offset = 0; offset < lineCount; offset += 1) { - const oldLineNumber = row.oldRange[0] + offset; - const newLineNumber = row.newRange[0] + offset; - const sourceLineNumber = (side === "old" ? oldLineNumber : newLineNumber) - 1; + // Which line each revealed row shows is the shared derivation's, not this renderer's: + // pairing the two sides against the expansion side by hand is what let three copies of + // this loop label expanded lines differently (A6). + const revealed = reviewExpandedGapLines( + { oldRange: row.oldRange, newRange: row.newRange, lineCount }, + side, + ); + for (const [offset, line] of revealed.entries()) { + const { oldLine: oldLineNumber, newLine: newLineNumber } = line; + const sourceLineNumber = line.sourceLine - 1; if (sourceLineNumber < 0 || sourceLineNumber >= sourceLines.length) { break; } diff --git a/src/ui/lib/fileHeader.test.ts b/src/ui/lib/fileHeader.test.ts index 1121377ff..bf72d335f 100644 --- a/src/ui/lib/fileHeader.test.ts +++ b/src/ui/lib/fileHeader.test.ts @@ -16,11 +16,37 @@ describe("file header layout", () => { large.stats = { additions: 1234, deletions: 56 }; large.statsTruncated = true; - expect(fileHeaderStats(small)).toMatchObject({ text: "+1 -0 ", width: 6 }); + expect(fileHeaderStats(small)).toMatchObject({ text: "+1 ", width: 3 }); expect(fileHeaderStats(large)).toMatchObject({ text: "+1234+ -56 ", width: 11 }); expect(maxFileHeaderStatsWidth([small, large])).toBe(11); }); + test("states the churn the shared formatter states, so a zero count is not a badge", () => { + const added = createTestDiffFile({ id: "added", path: "added.ts" }); + const removed = createTestDiffFile({ id: "removed", path: "removed.ts" }); + const unchanged = createTestDiffFile({ id: "unchanged", path: "unchanged.ts" }); + added.stats = { additions: 3, deletions: 0 }; + removed.stats = { additions: 0, deletions: 7 }; + unchanged.stats = { additions: 0, deletions: 0 }; + + expect(fileHeaderStats(added)).toMatchObject({ + additionsText: "+3", + deletionsText: null, + text: "+3 ", + }); + expect(fileHeaderStats(removed)).toMatchObject({ + additionsText: null, + deletionsText: "-7", + text: "-7 ", + }); + expect(fileHeaderStats(unchanged)).toMatchObject({ + additionsText: null, + deletionsText: null, + text: "", + width: 0, + }); + }); + test("fits long paths with three dots while preserving terminal-cell width", () => { const file = createTestDiffFile({ id: "long-path", diff --git a/src/ui/lib/fileHeader.ts b/src/ui/lib/fileHeader.ts index bde64defc..c0602db9b 100644 --- a/src/ui/lib/fileHeader.ts +++ b/src/ui/lib/fileHeader.ts @@ -1,3 +1,4 @@ +import { reviewFileStatBadges } from "../../core/review/presentation"; import type { DiffFile } from "../../core/types"; import { fileLabelParts } from "./files"; import { fitText, measureTextWidth } from "./text"; @@ -5,11 +6,24 @@ import { fitText, measureTextWidth } from "./text"; /** The explicit overflow marker used for file paths in review headers. */ export const FILE_HEADER_OVERFLOW_MARKER = "..."; -/** Build the styled text fragments and measured width for one file's line counts. */ +/** + * Build the styled text fragments and measured width for one file's line counts. + * + * The badge text is the shared review formatter's, so the diff header states the same + * churn as the sidebar and a browser reading the same file (E1) — a zero count is absent + * rather than printed as `+0`. Each present badge is trailed by one space, which is what + * the header row draws and what the copied text has to reproduce cell for cell. + */ export function fileHeaderStats(file: Pick) { - const additionsText = `+${file.stats.additions}${file.statsTruncated ? "+" : ""}`; - const deletionsText = `-${file.stats.deletions}`; - const text = `${additionsText} ${deletionsText} `; + const { additionsText, deletionsText } = reviewFileStatBadges({ + additions: file.stats.additions, + deletions: file.stats.deletions, + ...(file.statsTruncated !== undefined ? { truncated: file.statsTruncated } : {}), + }); + const text = [additionsText, deletionsText] + .filter((badge): badge is string => badge !== null) + .map((badge) => `${badge} `) + .join(""); return { additionsText, diff --git a/src/ui/lib/files.ts b/src/ui/lib/files.ts index 38cc77b79..1b5368bd3 100644 --- a/src/ui/lib/files.ts +++ b/src/ui/lib/files.ts @@ -1,6 +1,7 @@ import { basename, dirname } from "node:path/posix"; import type { FileDiffMetadata } from "@pierre/diffs"; import { normalizeDiffPath } from "../../core/diffPaths"; +import { reviewFileStatBadges } from "../../core/review/presentation"; import type { AgentAnnotation, DiffFile } from "../../core/types"; import { readMetadataChangeType } from "../../extensions/events"; import { formatTerminalPath } from "../../lib/terminalText"; @@ -61,11 +62,6 @@ function sidebarFileName(file: SidebarFileSource) { return previousName === nextName ? nextName : `${previousName} -> ${nextName}`; } -/** Hide zero-value file stats so the sidebar only shows real line deltas. */ -function formatSidebarStat(prefix: "+" | "-", value: number, truncated = false) { - return value > 0 ? `${prefix}${value}${truncated ? "+" : ""}` : null; -} - /** Build the visible stats badges for one sidebar row. * Keep the agent-note badge first so it reads as review context before line churn. */ @@ -140,14 +136,21 @@ export function buildSidebarEntries(files: readonly SidebarFileSource[]): Sideba } const agentCommentCount = file.agent?.annotations.length ?? 0; + // Badge text is the shared review formatter's, not the sidebar's, so a browser + // rendering the same file states the same churn (E1). + const { additionsText, deletionsText } = reviewFileStatBadges({ + additions: file.stats.additions, + deletions: file.stats.deletions, + ...(file.statsTruncated !== undefined ? { truncated: file.statsTruncated } : {}), + }); entries.push({ kind: "file", id: file.id, name: sidebarFileName(file), agentCommentsText: agentCommentCount > 0 ? `*${agentCommentCount}` : null, - additionsText: formatSidebarStat("+", file.stats.additions, file.statsTruncated), - deletionsText: formatSidebarStat("-", file.stats.deletions), + additionsText, + deletionsText, changeType: file.changeType ?? readMetadataChangeType(file.metadata) ?? "change", isUntracked: file.isUntracked ?? false, }); diff --git a/src/ui/staticDiffPager.test.ts b/src/ui/staticDiffPager.test.ts index d0f924027..9c3d60921 100644 --- a/src/ui/staticDiffPager.test.ts +++ b/src/ui/staticDiffPager.test.ts @@ -210,7 +210,9 @@ describe("static diff pager", () => { const plain = stripAnsi(await renderStaticDiffPager(patchText)); - expect(plain).toContain("new.txt new file 100644 +1 -0"); + // A zero count is not a badge here either: the shared review formatter states the + // churn the sidebar and the diff header state, so a pure addition says only `+1`. + expect(plain.split("\n")[0]).toBe("new.txt new file 100644 +1"); expect(plain).not.toContain("diff --git"); expect(plain).not.toContain("index 0000000"); }); diff --git a/src/ui/staticDiffPager.ts b/src/ui/staticDiffPager.ts index a58edb871..f62e41aa9 100644 --- a/src/ui/staticDiffPager.ts +++ b/src/ui/staticDiffPager.ts @@ -16,6 +16,7 @@ */ import { loadAppBootstrap } from "../core/changesetLoaders"; import { reviewEmptyDiffReason, type ReviewEmptyDiffReason } from "../core/review/document"; +import { reviewFileStatBadges } from "../core/review/presentation"; import { DEFAULT_TAB_WIDTH } from "../core/tabWidth"; import type { CommonOptions, DiffFile, NamedCustomThemeConfig } from "../core/types"; import { @@ -360,9 +361,23 @@ async function renderStaticFile( ? buildSplitRows(file, highlighted, theme, tabWidth) : buildStackRows(file, highlighted, theme, tabWidth); const lineNumberWidth = maxLineNumberWidth(file, rows); - const stats = `${colorText(`+${file.stats.additions}${file.statsTruncated ? "+" : ""}`, theme.badgeAdded)} ${colorText(`-${file.stats.deletions}`, theme.badgeRemoved)}`; + // Badge text is the shared review formatter's, so a paged file states the same churn as + // the sidebar and the diff header, zero counts included — that is, omitted (E1). + const badges = reviewFileStatBadges({ + additions: file.stats.additions, + deletions: file.stats.deletions, + ...(file.statsTruncated !== undefined ? { truncated: file.statsTruncated } : {}), + }); + const stats = [ + badges.additionsText === null ? null : colorText(badges.additionsText, theme.badgeAdded), + badges.deletionsText === null ? null : colorText(badges.deletionsText, theme.badgeRemoved), + ] + .filter((badge): badge is string => badge !== null) + .join(" "); const status = colorText(`${fileStatusLabel(file)}${fileModeText(file)}`, theme.muted); - const header = `${colorText(fileDisplayPath(file), theme.text)} ${status} ${stats}`; + const header = [colorText(fileDisplayPath(file), theme.text), status, stats] + .filter((part) => part.length > 0) + .join(" "); if (rows.length === 0) { return [header, colorText(` ${staticEmptyDiffMessage(file)}`, theme.muted)].join("\n"); diff --git a/src/web/BrowserReviewApp.test.tsx b/src/web/BrowserReviewApp.test.tsx new file mode 100644 index 000000000..efe240458 --- /dev/null +++ b/src/web/BrowserReviewApp.test.tsx @@ -0,0 +1,161 @@ +/** @jsxImportSource react */ +import { describe, expect, test } from "bun:test"; +import { renderToStaticMarkup } from "react-dom/server"; +import { formatReviewAddress } from "../core/review/address"; +import { projectReviewDocument } from "../core/review/document"; +import { reviewResourceId } from "../core/review/resources"; +import type { ReviewFileV1 } from "../core/review/types"; +import { createTestDiffFile } from "../../test/helpers/diff-helpers"; +import { reviewErrorMessage } from "../session/reviewErrorCatalog"; +import { reviewHttpFailure } from "../session/reviewHttpProtocol"; +import { HUNK_REVIEW_PROTOCOL_VERSION } from "../session/reviewProtocol"; +import type { BrowserReviewApiClient, BrowserReviewEventHandlers } from "./browserReviewApiClient"; +import { BrowserReviewApp } from "./BrowserReviewApp"; +import { BrowserReviewMirror, type BrowserReviewMirrorSource } from "./browserReviewMirror"; + +const SESSION_ID = "session-1"; +const GENERATION = "generation:p1:0"; +const BASE = `${Array.from({ length: 12 }, (_unused, index) => `line ${index + 1}`).join("\n")}\n`; + +/** Two files a review would publish, in this order. */ +function documentFiles(): ReviewFileV1[] { + return projectReviewDocument( + [ + createTestDiffFile({ + id: "alpha", + path: "src/alpha.ts", + before: BASE, + after: BASE.replace("line 4", "line 4 changed"), + context: 3, + }), + createTestDiffFile({ + id: "beta", + path: "src/beta.ts", + before: BASE, + after: BASE.replace("line 9", "line 9 changed"), + context: 3, + }), + ], + { sourceLabel: "/repo" }, + ).files; +} + +/** A mirror driven through its real transport seam until it has settled. */ +async function settledMirror( + files: ReviewFileV1[], + serve: BrowserReviewMirrorSource["readResource"], +) { + let handlers: BrowserReviewEventHandlers | undefined; + const source: BrowserReviewMirrorSource = { + readResource: serve, + streamEvents(next) { + handlers = next; + return new Promise(() => undefined); + }, + }; + const mirror = new BrowserReviewMirror(source, { + timers: { setTimeout: () => 1, clearTimeout: () => undefined }, + }); + mirror.start(); + handlers!.onPublication({ + protocolVersion: HUNK_REVIEW_PROTOCOL_VERSION, + sessionId: SESSION_ID, + publication: { generation: GENERATION, stateRevision: 1 }, + catalog: { + generation: GENERATION, + fileKeysByRuntimeId: Object.fromEntries(files.map((file) => [file.runtimeId, file.key])), + resources: files.map((file) => ({ + id: reviewResourceId({ kind: "canonical-file", fileKey: file.key }), + generation: GENERATION, + fileKey: file.key, + kind: "canonical-file" as const, + contentType: "application/vnd.hunk.review-file+json; charset=utf-8" as const, + })), + }, + }); + for (let turn = 0; turn < 8; turn += 1) { + await Promise.resolve(); + } + return { mirror, handlers: handlers! }; +} + +/** Serve each file's canonical form, as the surface would. */ +function serveFiles(files: ReviewFileV1[]): BrowserReviewMirrorSource["readResource"] { + const encoder = new TextEncoder(); + return async (descriptor) => { + const file = files.find( + (candidate) => + reviewResourceId({ kind: "canonical-file", fileKey: candidate.key }) === descriptor.id, + ); + return file + ? { ok: true, value: encoder.encode(JSON.stringify(file)) } + : { ok: false, code: "unknown-resource", message: "not in this test" }; + }; +} + +/** The client the page would fetch source with; nothing static-rendered reaches it. */ +const UNUSED_CLIENT = {} as BrowserReviewApiClient; + +describe("BrowserReviewApp", () => { + test("lists every file in review order, linking to its place in the stream", async () => { + const files = documentFiles(); + const { mirror } = await settledMirror(files, serveFiles(files)); + + const markup = renderToStaticMarkup( + , + ); + + const [first, second] = files; + expect(markup).toContain( + `href="#${formatReviewAddress({ kind: "file", fileKey: first!.key })}"`, + ); + expect(markup.indexOf("src/alpha.ts")).toBeLessThan(markup.indexOf("src/beta.ts")); + expect(markup).toContain(formatReviewAddress({ kind: "file", fileKey: second!.key })); + }); + + test("says nothing about a review it has not loaded yet", () => { + const idle = new BrowserReviewMirror( + { + readResource: async () => ({ ok: false, code: "unknown-resource", message: "idle" }), + streamEvents: () => new Promise(() => undefined), + }, + { timers: { setTimeout: () => 1, clearTimeout: () => undefined } }, + ); + + const markup = renderToStaticMarkup(); + + expect(markup).toContain('data-status="idle"'); + expect(markup).not.toContain("review-file-list"); + }); + + test("shows a failure in the words the mirror was given, not its own", async () => { + const files = documentFiles(); + const failure = reviewErrorMessage("resource-unavailable"); + const { mirror } = await settledMirror(files, async () => ({ + ok: false, + code: "resource-unavailable", + message: failure, + })); + + const markup = renderToStaticMarkup( + , + ); + + expect(markup).toContain('data-status="failed"'); + expect(markup).toContain(failure); + }); + + test("keeps the diff on screen while the dropped stream is reconnecting", async () => { + const files = documentFiles(); + const { mirror, handlers } = await settledMirror(files, serveFiles(files)); + + handlers.onError?.(reviewHttpFailure("resource-unavailable")); + const markup = renderToStaticMarkup( + , + ); + + expect(markup).toContain('data-status="reconnecting"'); + expect(markup).toContain("Reconnecting to the review…"); + expect(markup).toContain("src/alpha.ts"); + }); +}); diff --git a/src/web/BrowserReviewApp.tsx b/src/web/BrowserReviewApp.tsx new file mode 100644 index 000000000..78f2a0c08 --- /dev/null +++ b/src/web/BrowserReviewApp.tsx @@ -0,0 +1,155 @@ +/** @jsxImportSource react */ +/** + * The browser review page: a file list, the review stream, and what the connection is doing. + * + * Read-only by construction — nothing here dispatches an action, edits a note, or publishes + * a selection. It subscribes to the mirror, renders whatever document the mirror holds, and + * says in the shared catalog's words when something failed + * (`docs/browser-review-seam-audit.md`, G4). + * + * The sidebar is navigation only, as it is in the terminal: selecting a file moves the + * stream to that file rather than collapsing the stream to it. + */ +import { useCallback, useEffect, useMemo, useState, useSyncExternalStore } from "react"; +import { formatReviewAddress } from "../core/review/address"; +import { reviewFileStatBadges } from "../core/review/presentation"; +import type { ReviewFileV1 } from "../core/review/types"; +import type { BrowserReviewApiClient } from "./browserReviewApiClient"; +import type { BrowserReviewMirror, BrowserReviewMirrorSnapshot } from "./browserReviewMirror"; +import { + BrowserReviewSourceStore, + type BrowserReviewSourceEntries, + type BrowserReviewSourceSnapshot, +} from "./browserReviewSources"; +import { BrowserReviewStream } from "./BrowserReviewStream"; +import { + resolveBrowserViewOptions, + type BrowserViewOptions, + type BrowserHostViewDefaults, +} from "./browserViewOptions"; + +export interface BrowserReviewAppProps { + mirror: BrowserReviewMirror; + client: BrowserReviewApiClient; + /** The host's resolved view defaults, when the page was served with them (G1). */ + hostViewDefaults?: BrowserHostViewDefaults; +} + +/** Watch one mirror as a React store, without copying its state into component state. */ +function useReviewMirror(mirror: BrowserReviewMirror): BrowserReviewMirrorSnapshot { + return useSyncExternalStore( + useCallback((notify) => mirror.subscribe(notify), [mirror]), + useCallback(() => mirror.getSnapshot(), [mirror]), + useCallback(() => mirror.getSnapshot(), [mirror]), + ); +} + +/** No source read yet, and a stable identity so an empty render is not a new object. */ +const NO_SOURCES: BrowserReviewSourceEntries = {}; + +/** Watch one source store the same way, so a read that lands re-renders the gaps it fills. */ +function useReviewSources(sources: BrowserReviewSourceStore): BrowserReviewSourceSnapshot { + return useSyncExternalStore( + useCallback((notify) => sources.subscribe(notify), [sources]), + useCallback(() => sources.getSnapshot(), [sources]), + useCallback(() => sources.getSnapshot(), [sources]), + ); +} + +/** Track the viewport width the responsive layout decides from. */ +function useViewportWidth() { + const [width, setWidth] = useState(() => (typeof window === "undefined" ? 0 : window.innerWidth)); + useEffect(() => { + const onResize = () => setWidth(window.innerWidth); + window.addEventListener("resize", onResize); + return () => window.removeEventListener("resize", onResize); + }, []); + return width; +} + +export function BrowserReviewApp({ mirror, client, hostViewDefaults }: BrowserReviewAppProps) { + const snapshot = useReviewMirror(mirror); + const viewportWidth = useViewportWidth(); + const [view] = useState(() => resolveBrowserViewOptions(hostViewDefaults)); + const sources = useMemo(() => new BrowserReviewSourceStore(client), [client]); + const sourceSnapshot = useReviewSources(sources); + + // The page owns the mirror's attachment for as long as it is on screen; the mirror can be + // attached again, so a remount picks the same review back up rather than going dark. + useEffect(() => { + mirror.start(); + return () => mirror.stop(); + }, [mirror]); + + // A generation change invalidates every source it was read for: the same file key over + // new content is different text. The store is pointed at the new generation in an effect, + // which runs after this render, so what is drawn is guarded on the generation the entries + // were read for rather than on the effect having caught up. + const generation = snapshot.publication?.generation; + useEffect(() => { + sources.setGeneration(generation); + }, [sources, generation]); + const sourceByFileKey = + sourceSnapshot.generation === generation ? sourceSnapshot.entries : NO_SOURCES; + + const requestSource = useCallback((file: ReviewFileV1) => sources.request(file), [sources]); + + return ( +
+ + {snapshot.document ? ( +
+ + +
+ ) : null} +
+ ); +} + +/** What the connection is doing, in the shared catalog's words when it went wrong. */ +function ReviewStatus({ snapshot }: { snapshot: BrowserReviewMirrorSnapshot }) { + return ( +
+ {snapshot.status === "loading" ? Loading the review… : null} + {/* The diff below is still the one this review published; only the link dropped. */} + {snapshot.status === "reconnecting" ? Reconnecting to the review… : null} + {snapshot.status === "disconnected" ? This review session has ended. : null} + {snapshot.failure ? ( + {snapshot.failure.message} + ) : null} +
+ ); +} + +/** The review's files, in review order, each linking to its place in the stream. */ +function ReviewFileList({ files }: { files: readonly ReviewFileV1[] }) { + return ( + + ); +} diff --git a/src/web/BrowserReviewStream.test.tsx b/src/web/BrowserReviewStream.test.tsx new file mode 100644 index 000000000..57dd8a1d6 --- /dev/null +++ b/src/web/BrowserReviewStream.test.tsx @@ -0,0 +1,148 @@ +/** @jsxImportSource react */ +import { describe, expect, test } from "bun:test"; +import { renderToStaticMarkup } from "react-dom/server"; +import { formatReviewAddress } from "../core/review/address"; +import { projectReviewDocument } from "../core/review/document"; +import { createTestDiffFile, createTestSourceFetcher } from "../../test/helpers/diff-helpers"; +import { reviewErrorMessage } from "../session/reviewErrorCatalog"; +import { buildBrowserReviewFileRenderModel } from "./browserPierreDocument"; +import { reviewHttpFailure } from "../session/reviewHttpProtocol"; +import type { BrowserReviewSourceEntry } from "./browserReviewSources"; +import { BrowserGapStrip, BrowserReviewStream } from "./BrowserReviewStream"; +import { DEFAULT_BROWSER_VIEW_OPTIONS } from "./browserViewOptions"; + +const BASE = `${Array.from({ length: 24 }, (_unused, index) => `line ${index + 1}`).join("\n")}\n`; +const CHANGED = BASE.replace("line 4", "line 4 changed"); + +/** The document a review of these two files publishes, in this order. */ +function documentFor() { + return projectReviewDocument( + [ + createTestDiffFile({ + id: "alpha", + path: "src/alpha.ts", + before: BASE, + after: CHANGED, + context: 3, + sourceFetcher: createTestSourceFetcher(() => BASE), + }), + createTestDiffFile({ id: "beta", path: "src/beta.ts", before: BASE, after: BASE }), + ], + { sourceLabel: "/repo" }, + ); +} + +/** Render the stream to markup, which is as far as a static render can take it. */ +function render(width = 1_400) { + const document = documentFor(); + return { + document, + markup: renderToStaticMarkup( + , + ), + }; +} + +/** One opened collapsed region, rendered the way the stream places it around a hunk. */ +function renderOpenGap(options: { + source: BrowserReviewSourceEntry; + showHeader?: boolean; + showLineNumbers?: boolean; +}) { + const file = documentFor().files[0]!; + const gap = buildBrowserReviewFileRenderModel(file).gaps[0]!; + return renderToStaticMarkup( + undefined} + showHeader={options.showHeader ?? false} + showLineNumbers={options.showLineNumbers ?? false} + />, + ); +} + +describe("BrowserReviewStream", () => { + test("renders every file, in the document's order", () => { + const { markup } = render(); + + expect(markup.indexOf("src/alpha.ts")).toBeGreaterThanOrEqual(0); + expect(markup.indexOf("src/beta.ts")).toBeGreaterThan(markup.indexOf("src/alpha.ts")); + }); + + test("addresses each file and hunk through the shared address grammar", () => { + const { document, markup } = render(); + const file = document.files[0]!; + + expect(markup).toContain(formatReviewAddress({ kind: "file", fileKey: file.key })); + expect(markup).toContain( + formatReviewAddress({ kind: "hunk", fileKey: file.key, hunkIndex: 0 }), + ); + }); + + test("shows the churn badges the shared formatter produced", () => { + const { document, markup } = render(); + const file = document.files[0]!; + + expect(markup).toContain(`+${file.stats.additions}`); + expect(markup).toContain(`-${file.stats.deletions}`); + }); + + test("explains a file with nothing to render instead of drawing an empty diff", () => { + const { markup } = render(); + + expect(markup).toContain("No changes to show."); + }); + + test("offers each collapsed region by the line count core addressed it with", () => { + const { document, markup } = render(); + const gaps = buildBrowserReviewFileRenderModel(document.files[0]!).gaps; + + expect(gaps).not.toHaveLength(0); + for (const gap of gaps) { + expect(markup).toContain(`${gap.lineCount} unchanged`); + // The label is the gap's own addresses, not a count this component computed. + expect(markup).toContain(`-${gap.oldRange[0]},${gap.lineCount}`); + } + // Closed until a reader opens it: nothing is fetched for a region nobody looked at. + expect(markup).toContain('aria-expanded="false"'); + }); + + test("numbers the lines an opened gap reveals when line numbers are on", () => { + const markup = renderOpenGap({ + source: { status: "ready", text: BASE }, + showLineNumbers: true, + showHeader: false, + }); + + expect(markup).toContain("review-gap-line-numbers"); + // The two options are separate: the range label belongs to the hunk-header option. + expect(markup).not.toContain("review-gap-range"); + }); + + test("leaves them off when line numbers are off, whatever the hunk headers say", () => { + const markup = renderOpenGap({ + source: { status: "ready", text: BASE }, + showLineNumbers: false, + showHeader: true, + }); + + expect(markup).not.toContain("review-gap-line-numbers"); + expect(markup).toContain("review-gap-range"); + }); + + test("says why an opened gap has no lines instead of loading them forever", () => { + const failure = reviewHttpFailure("resource-unavailable"); + + const markup = renderOpenGap({ source: { status: "failed", failure } }); + + expect(markup).toContain(reviewErrorMessage("resource-unavailable")); + expect(markup).not.toContain("Loading unchanged lines…"); + }); +}); diff --git a/src/web/BrowserReviewStream.tsx b/src/web/BrowserReviewStream.tsx new file mode 100644 index 000000000..43be20d59 --- /dev/null +++ b/src/web/BrowserReviewStream.tsx @@ -0,0 +1,311 @@ +/** @jsxImportSource react */ +/** + * The read-only review stream: every visible file's diff, top to bottom, in review order. + * + * This is the browser's half of the product rule the terminal already follows — one + * continuous stream rather than one file at a time — and it draws nothing it derives + * itself. What to draw comes from `pierreDocument`'s render model, which reads geometry + * from `src/core/review/`; how a diff looks is Pierre's; this file places them. + * + * Two structural decisions: + * + * - **One Pierre render per hunk, not per file.** A collapsed region sits *between* two + * hunks, so drawing hunks separately is what lets the strip that describes one appear + * where it belongs. The isolation is the shared re-basing walk (A6), not a second one. + * - **Presentation state stays here.** Which gaps a reader has opened is this client's + * business in a read-only mirror; when actions land it becomes the shared + * `expansion/toggle` intent instead, and this component reads the answer rather than + * holding it. + */ +import { useCallback, useEffect, useMemo, useState, type ReactNode } from "react"; +import { FileDiff } from "@pierre/diffs/react"; +import type { ReviewDocumentV1, ReviewFileV1 } from "../core/review/types"; +import { formatReviewAddress } from "../core/review/address"; +import { + buildBrowserReviewFileRenderModel, + browserReviewExpandedGapRows, + type BrowserReviewExpandedRow, + type BrowserReviewFileRenderModel, + type BrowserReviewRenderGap, +} from "./browserPierreDocument"; +import type { BrowserReviewSourceEntry } from "./browserReviewSources"; +import { resolveBrowserDiffStyle, type BrowserViewOptions } from "./browserViewOptions"; + +/** What a file with no rows says about itself, in this surface's wording. */ +const EMPTY_DIFF_MESSAGES = { + "rename-only": "Renamed with no content changes.", + binary: "Binary file.", + "too-large": "File too large to render.", + "new-file": "New empty file.", + "deleted-file": "File deleted.", + "no-hunks": "No changes to show.", +} as const; + +export interface BrowserReviewStreamProps { + document: ReviewDocumentV1; + view: BrowserViewOptions; + /** Width the responsive layout decides from; the window's, in a real page. */ + viewportWidth: number; + /** Each file's source text, or why it could not be read, for the gaps a reader opened. */ + sourceByFileKey?: Record; + /** Asked for the source behind one file whenever a gap in it is opened. */ + onRequestSource?: (file: ReviewFileV1) => void; +} + +/** The whole review, in the order the document lists it. */ +export function BrowserReviewStream({ + document, + view, + viewportWidth, + sourceByFileKey = {}, + onRequestSource, +}: BrowserReviewStreamProps) { + const diffStyle = resolveBrowserDiffStyle(view.layout, viewportWidth); + return ( +
+ {document.files.map((file) => ( + + ))} +
+ ); +} + +interface ReviewFileSectionProps { + file: ReviewFileV1; + view: BrowserViewOptions; + diffStyle: "split" | "unified"; + source?: BrowserReviewSourceEntry; + onRequestSource?: (file: ReviewFileV1) => void; +} + +/** One file: its header, its collapsed regions, and its hunks between them. */ +function ReviewFileSection({ + file, + view, + diffStyle, + source, + onRequestSource, +}: ReviewFileSectionProps) { + // Built from the file's content and nothing else — not from the width, which only reaches + // Pierre — so a resize re-renders the stream without rebuilding every file's model and + // handing Pierre fresh object identities to re-highlight. + const model = useMemo(() => buildBrowserReviewFileRenderModel(file), [file]); + const [openGaps, setOpenGaps] = useState>(() => new Set()); + + // A reload replaces the file behind this section; gap ids address the geometry that was + // published with it, so what was open cannot be carried over. + useEffect(() => { + setOpenGaps(new Set()); + }, [file.contentIdentity]); + + const toggleGap = useCallback( + (gapId: string) => { + const opening = !openGaps.has(gapId); + setOpenGaps((open) => { + const next = new Set(open); + if (next.delete(gapId)) { + return next; + } + next.add(gapId); + return next; + }); + // Asked outside the updater, which React may run more than once. The request is the + // reader's ask for this file's text and also the retry after one that failed; the + // store is what decides whether it costs a read. + if (opening) { + onRequestSource?.(file); + } + }, + [file, onRequestSource, openGaps], + ); + + return ( +
+ + {model.emptyDiffReason ? ( +

{EMPTY_DIFF_MESSAGES[model.emptyDiffReason]}

+ ) : ( + model.hunks.map((hunk) => ( +
+ + +
+ )) + )} + gap.position === "trailing")} + open={openGaps} + file={file} + source={source} + onToggle={toggleGap} + showHeader={view.showHunkHeaders} + showLineNumbers={view.showLineNumbers} + /> +
+ ); +} + +/** The collapsed region immediately before one hunk, when the file has one. */ +function gapBefore( + model: BrowserReviewFileRenderModel, + hunkIndex: number, +): BrowserReviewRenderGap | undefined { + return model.gaps.find((gap) => gap.position === "before" && gap.hunkIndex === hunkIndex); +} + +/** One file's identity row: where it is, where it came from, and how much it changed. */ +function ReviewFileHeader({ model }: { model: BrowserReviewFileRenderModel }) { + return ( +
+ {model.path} + {model.previousPath ? ( + renamed from {model.previousPath} + ) : null} + + {model.statBadges.additionsText ? ( + {model.statBadges.additionsText} + ) : null} + {model.statBadges.deletionsText ? ( + {model.statBadges.deletionsText} + ) : null} + +
+ ); +} + +export interface BrowserGapStripProps { + gap: BrowserReviewRenderGap | undefined; + open: ReadonlySet; + file: ReviewFileV1; + /** This file's source, once it has been read, or why it could not be. */ + source: BrowserReviewSourceEntry | undefined; + onToggle: (gapId: string) => void; + /** Whether the strip states the range it covers, as a hunk header would. */ + showHeader: boolean; + /** Whether the lines it reveals carry their line numbers, as the diff rows do. */ + showLineNumbers: boolean; +} + +/** + * One collapsed region: how many lines it hides, and the lines themselves once opened. + * + * The line labels are the gap's own addresses and the text is the shared source splitter's, + * so an expanded line here is the same line the producer would accept a note on. + */ +export function BrowserGapStrip({ + gap, + open, + file, + source, + onToggle, + showHeader, + showLineNumbers, +}: BrowserGapStripProps) { + if (!gap) { + return null; + } + const isOpen = open.has(gap.gapId); + const rows = + isOpen && source?.text !== undefined + ? browserReviewExpandedGapRows(file, gap.gapId, source.text) + : undefined; + return ( +
+ + {isOpen ? ( + + ) : null} +
+ ); +} + +/** The `-old +new` label one gap covers, in the ranges core addressed it by. */ +function rangeLabel(gap: BrowserReviewRenderGap) { + return `-${gap.oldRange[0]},${gap.lineCount} +${gap.newRange[0]},${gap.lineCount}`; +} + +/** The lines an opened gap reveals, or why they are not there yet. */ +function ExpandedRows({ + rows, + showLineNumbers, + failure, +}: { + rows: BrowserReviewExpandedRow[] | undefined; + showLineNumbers: boolean; + /** Why the source behind these lines could not be read, in the catalog's words. */ + failure?: string; +}): ReactNode { + if (failure) { + // Said once, in the refusal's own wording: a read that will not come back must not look + // like one that is still on its way. Opening the gap again asks for it again. + return

{failure}

; + } + if (!rows) { + return

Loading unchanged lines…

; + } + return ( +
+      {rows.map((row) => (
+        
+ {showLineNumbers ? ( + + {row.oldLine} {row.newLine} + + ) : null} + {row.text} +
+ ))} +
+ ); +} diff --git a/src/web/browserPierreDocument.test.ts b/src/web/browserPierreDocument.test.ts new file mode 100644 index 000000000..7b0d16f71 --- /dev/null +++ b/src/web/browserPierreDocument.test.ts @@ -0,0 +1,136 @@ +import { describe, expect, test } from "bun:test"; +import { projectReviewDocument } from "../core/review/document"; +import { createTestDiffFile } from "../../test/helpers/diff-helpers"; +import { + buildBrowserReviewFileRenderModel, + isolateBrowserReviewHunk, + browserReviewExpandedGapRows, +} from "./browserPierreDocument"; + +const BASE = Array.from({ length: 24 }, (_unused, index) => `line ${index + 1}`).join("\n") + "\n"; +/** Two changes far enough apart to parse as two hunks, with context between them. */ +const CHANGED = BASE.replace("line 4", "line 4 changed").replace("line 20", "line 20 changed"); + +/** One real parse, so the model is built from geometry a parser produced. */ +function modelFor(before: string, after: string) { + const document = projectReviewDocument( + [createTestDiffFile({ id: "alpha", path: "src/alpha.ts", before, after, context: 3 })], + { sourceLabel: "/repo" }, + ); + return { file: document.files[0]!, model: buildBrowserReviewFileRenderModel(document.files[0]!) }; +} + +describe("buildBrowserReviewFileRenderModel", () => { + test("carries the row totals the parser measured rather than reducing over hunks", () => { + const { file, model } = modelFor(BASE, CHANGED); + const reduced = file.hunks.reduce((total, hunk) => total + hunk.splitLineCount, 0); + + // A7: the file's totals include rows outside every hunk span, which is exactly why a + // renderer that reduces over hunks mis-sizes its virtualization. + expect(model.splitLineCount).toBe(file.splitLineCount); + expect(model.unifiedLineCount).toBe(file.unifiedLineCount); + expect(model.splitLineCount).not.toBe(reduced); + }); + + test("reads the expansion side from the file's change kind", () => { + expect(modelFor(BASE, CHANGED).model.expansionSide).toBe("new"); + expect(modelFor(BASE, "").model.expansionSide).toBe("old"); + }); + + test("offers one hunk per hunk, addressed by the review's own index", () => { + const { file, model } = modelFor(BASE, CHANGED); + + expect(model.hunks.map((hunk) => hunk.index)).toEqual(file.hunks.map((_hunk, index) => index)); + expect(model.hunks.length).toBeGreaterThan(1); + }); + + test("reports why a file with no rows has none", () => { + const document = projectReviewDocument( + [ + { + ...createTestDiffFile({ id: "bin", path: "logo.png", before: BASE, after: BASE }), + isBinary: true, + }, + ], + { sourceLabel: "/repo" }, + ); + + expect(buildBrowserReviewFileRenderModel(document.files[0]!).emptyDiffReason).toBe("binary"); + }); +}); + +describe("isolateBrowserReviewHunk", () => { + test("slices exactly the lines the shared re-basing walk consumed", () => { + const { file } = modelFor(BASE, CHANGED); + + for (const hunk of file.hunks) { + const isolated = isolateBrowserReviewHunk(file, hunk); + const only = isolated.hunks[0]!; + + // A6: origins are zero and the sliced arrays are exactly as long as the hunk's own + // content, so a renderer never reads a neighbouring hunk's lines. + expect(only.additionLineIndex).toBe(0); + expect(only.deletionLineIndex).toBe(0); + expect(only.collapsedBefore).toBe(0); + expect(isolated.additionLines).toEqual( + file.additionLines.slice( + hunk.additionLineIndex, + hunk.additionLineIndex + isolated.additionLines.length, + ), + ); + expect(isolated.additionLines.length).toBe(hunk.additionCount); + expect(isolated.deletionLines.length).toBe(hunk.deletionCount); + } + }); + + test("keeps the file's identity in the highlight cache key", () => { + const { file } = modelFor(BASE, CHANGED); + + expect(isolateBrowserReviewHunk(file, file.hunks[0]!).cacheKey).toBe( + `${file.contentIdentity}:0`, + ); + }); + + test("stays partial: a review file carries the patch's lines, not the file's", () => { + const { file } = modelFor(BASE, CHANGED); + + expect(isolateBrowserReviewHunk(file, file.hunks[0]!).isPartial).toBe(true); + }); +}); + +describe("browserReviewExpandedGapRows", () => { + test("labels each revealed line with the gap's own addresses", () => { + const { file, model } = modelFor(BASE, CHANGED); + const gap = model.gaps[0]!; + + const rows = browserReviewExpandedGapRows(file, gap.gapId, CHANGED)!; + + expect(rows).toHaveLength(gap.lineCount); + expect(rows[0]).toEqual({ + oldLine: gap.oldRange[0], + newLine: gap.newRange[0], + text: `line ${gap.newRange[0]}`, + }); + }); + + test("normalizes CRLF rather than leaking carriage returns into a row", () => { + const { file, model } = modelFor( + BASE.replaceAll("\n", "\r\n"), + CHANGED.replaceAll("\n", "\r\n"), + ); + + const rows = browserReviewExpandedGapRows( + file, + model.gaps[0]!.gapId, + CHANGED.replaceAll("\n", "\r\n"), + )!; + + expect(rows.every((row) => !row.text.includes("\r"))).toBe(true); + }); + + test("reveals nothing for a gap this file does not have", () => { + const { file } = modelFor(BASE, CHANGED); + + expect(browserReviewExpandedGapRows(file, "before:99", CHANGED)).toBeUndefined(); + }); +}); diff --git a/src/web/browserPierreDocument.ts b/src/web/browserPierreDocument.ts new file mode 100644 index 000000000..cda37d47b --- /dev/null +++ b/src/web/browserPierreDocument.ts @@ -0,0 +1,240 @@ +/** + * Turning one semantic file into what a browser draws it from. + * + * Pierre renders a diff from its own `FileDiffMetadata`, and the review model carries every + * fact that shape needs — a review hunk *is* a Pierre hunk plus an index, and the row totals + * a renderer sizes with are carried on the file rather than reduced from its hunks. So this + * adapter mostly renames, and its value is in what it refuses to compute: gap ranges, hunk + * extents, note targets, reveal targets, expansion side, and source splitting all come from + * `src/core/review/`, because a renderer that derives one of them disagrees with the state + * store that validates against it. + * + * Each of those is an audit finding with a browser site + * (`docs/browser-review-seam-audit.md`): the prototype's own adapter measured hunk extents + * from changed-line counts (A3), guessed the file's row totals (A7), re-derived the + * expansion side (A5), split source text with a bare `split("\n")` (A4), and rebased an + * isolated hunk's indices with a second walk that disagreed with the terminal's about + * whether the result was partial (A6). None of that happens here. + * + * The module is renderer-shaped but DOM-free: it produces values, not elements, so every + * geometry question the conformance corpus asks can be answered without a browser. + */ +import type { FileDiffMetadata, Hunk, SupportedLanguages } from "@pierre/diffs"; +import { reviewEmptyDiffReason, type ReviewEmptyDiffReason } from "../core/review/document"; +import { + reviewExpandedGapLines, + reviewExpansionSide, + reviewGapAddress, + reviewGapId, + reviewGapSourceForFile, + reviewLeadingGap, + reviewTrailingGap, + type ReviewGapAddress, +} from "../core/review/expansion"; +import { + normalizedReviewSourceLines, + rebaseReviewHunk, + reviewCanonicalHunkLine, + reviewDefaultHunkLineTarget, + reviewHunkRanges, +} from "../core/review/geometry"; +import { reviewFileStatBadges, type ReviewFileStatBadges } from "../core/review/presentation"; +import type { + ReviewFileV1, + ReviewHunkV1, + ReviewLineAddressV1, + ReviewLineRange, + ReviewSide, +} from "../core/review/types"; + +/** One hunk, as the stream needs to place it and address it. */ +export interface BrowserReviewRenderHunk { + index: number; + /** Inclusive per-side extents, so a note or a highlight lands inside its own hunk. */ + oldRange: ReviewLineRange; + newRange: ReviewLineRange; + /** Where a note about this whole hunk hangs (A10). */ + noteTarget: ReviewLineAddressV1; + /** + * The line this hunk is scrolled to, on a side that really has rows (B6). + * + * Absent for a hunk with rows on neither side, which is not something to scroll to. + */ + revealTarget?: ReviewLineAddressV1; + /** Pierre's description of this hunk alone, for rendering it between two gap strips. */ + fileDiff: FileDiffMetadata; +} + +/** One collapsed region, addressed the way every consumer addresses it. */ +export interface BrowserReviewRenderGap extends ReviewGapAddress { + gapId: string; +} + +/** Everything the stream needs to draw one file, and nothing it has to derive itself. */ +export interface BrowserReviewFileRenderModel { + fileKey: string; + path: string; + previousPath?: string; + language?: string; + statBadges: ReviewFileStatBadges; + /** Row totals the parser measured, carried rather than reduced from hunks (A7). */ + splitLineCount: number; + unifiedLineCount: number; + /** The side whose full source text fills this file's gaps (A5). */ + expansionSide: ReviewSide; + /** Identity of that source, so an expansion knows which text it read. */ + sourceIdentity?: string; + gaps: BrowserReviewRenderGap[]; + hunks: BrowserReviewRenderHunk[]; + /** Why there is nothing to draw, for a file with no rows (A8). */ + emptyDiffReason?: ReviewEmptyDiffReason; +} + +/** One row an expanded gap reveals: the labels on each side, and the text between them. */ +export interface BrowserReviewExpandedRow { + oldLine: number; + newLine: number; + text: string; +} + +/** The Pierre change type one review change kind is; the two vocabularies are the same. */ +function pierreChangeType(file: ReviewFileV1): FileDiffMetadata["type"] { + return file.changeKind; +} + +/** The parts of Pierre's file metadata that describe the file rather than its rows. */ +function pierreFileHeader(file: ReviewFileV1) { + return { + name: file.path, + ...(file.previousPath !== undefined ? { prevName: file.previousPath } : {}), + ...(file.language !== undefined ? { lang: file.language as SupportedLanguages } : {}), + type: pierreChangeType(file), + // Always partial: a review file carries the patch's lines, not the whole file's, and + // telling Pierre otherwise would offer expansion it has no content for. Expansion is + // the review's own, through the gap addresses below. + isPartial: true, + }; +} + +/** One review hunk as Pierre's, which is the same record without the review's index. */ +function pierreHunk(hunk: ReviewHunkV1): Hunk { + const { index: _index, ...rest } = hunk; + return rest as unknown as Hunk; +} + +/** + * Pierre metadata for one hunk on its own. + * + * Rendering hunk by hunk is what lets the stream put a collapsed-region strip where the + * collapsed region actually is, rather than around the whole file. The hunk's line indices + * are re-based onto a zero origin by the shared walk, which also reports where each side + * ends — so the lines are sliced with the same numbers the re-basing used, instead of by a + * second count that could disagree with it (A6). + */ +export function isolateBrowserReviewHunk(file: ReviewFileV1, hunk: ReviewHunkV1): FileDiffMetadata { + const rebased = rebaseReviewHunk(hunk, { additionLineIndex: 0, deletionLineIndex: 0 }); + return { + ...pierreFileHeader(file), + hunks: [ + pierreHunk({ + ...rebased.hunk, + // Drawn on its own, so it starts at the top of its own render and reports no + // collapsed region: the strip beside it is what says lines were omitted. + collapsedBefore: 0, + splitLineStart: 0, + unifiedLineStart: 0, + }), + ], + additionLines: file.additionLines.slice( + hunk.additionLineIndex, + hunk.additionLineIndex + rebased.additionEndIndex, + ), + deletionLines: file.deletionLines.slice( + hunk.deletionLineIndex, + hunk.deletionLineIndex + rebased.deletionEndIndex, + ), + splitLineCount: hunk.splitLineCount, + unifiedLineCount: hunk.unifiedLineCount, + // Identity of the content, so Pierre's highlight cache keys on what changed rather + // than on object identity. + cacheKey: `${file.contentIdentity}:${hunk.index}`, + }; +} + +/** Every collapsed region in one file, in the order a top-to-bottom reader meets them. */ +export function browserReviewRenderGaps(file: ReviewFileV1): BrowserReviewRenderGap[] { + const source = reviewGapSourceForFile(file); + const leading = file.hunks.flatMap((_hunk, index) => { + const gap = reviewLeadingGap(source, index); + return gap ? [{ ...gap, gapId: reviewGapId("before", index) }] : []; + }); + const trailing = reviewTrailingGap(source); + return trailing + ? [...leading, { ...trailing, gapId: reviewGapId("trailing", trailing.hunkIndex) }] + : leading; +} + +/** Build everything the stream draws one file from. */ +export function buildBrowserReviewFileRenderModel( + file: ReviewFileV1, +): BrowserReviewFileRenderModel { + return { + fileKey: file.key, + path: file.path, + ...(file.previousPath !== undefined ? { previousPath: file.previousPath } : {}), + ...(file.language !== undefined ? { language: file.language } : {}), + statBadges: reviewFileStatBadges(file.stats), + splitLineCount: file.splitLineCount, + unifiedLineCount: file.unifiedLineCount, + expansionSide: reviewExpansionSide(file.changeKind), + ...(file.sourceIdentity !== undefined ? { sourceIdentity: file.sourceIdentity } : {}), + gaps: browserReviewRenderGaps(file), + hunks: file.hunks.map((hunk) => { + const revealTarget = reviewCanonicalHunkLine(hunk); + return { + index: hunk.index, + ...reviewHunkRanges(hunk), + noteTarget: reviewDefaultHunkLineTarget(hunk), + ...(revealTarget ? { revealTarget } : {}), + fileDiff: isolateBrowserReviewHunk(file, hunk), + }; + }), + ...(file.hunks.length === 0 + ? { + emptyDiffReason: reviewEmptyDiffReason({ + changeKind: file.changeKind, + binary: file.flags.binary, + tooLarge: file.flags.tooLarge, + }), + } + : {}), + }; +} + +/** + * The rows one expanded gap reveals, given the file's full source text. + * + * The gap is resolved by id against the file's current geometry, and the text is split by + * the shared splitter — CRLF collapsed, one trailing newline dropped — so line N of the + * source is the line the gap's range calls N. A bare `split("\n")` here is what put `\r` + * glyphs and a phantom last line into the prototype's browser (A4). + * + * Undefined when the gap addresses nothing in this file, which is what a reload that moved + * the diff looks like from a client still holding the old gap id. + */ +export function browserReviewExpandedGapRows( + file: ReviewFileV1, + gapId: string, + sourceText: string, +): BrowserReviewExpandedRow[] | undefined { + const address = reviewGapAddress(reviewGapSourceForFile(file), gapId); + if (!address) { + return undefined; + } + const lines = normalizedReviewSourceLines(sourceText); + return reviewExpandedGapLines(address, reviewExpansionSide(file.changeKind)).map((line) => ({ + oldLine: line.oldLine, + newLine: line.newLine, + text: lines[line.sourceLine - 1] ?? "", + })); +} diff --git a/src/web/browserReviewApiClient.test.ts b/src/web/browserReviewApiClient.test.ts new file mode 100644 index 000000000..b335b8956 --- /dev/null +++ b/src/web/browserReviewApiClient.test.ts @@ -0,0 +1,351 @@ +import { describe, expect, test } from "bun:test"; +import { REVIEW_RESOURCE_CHUNK_BYTES, reviewResourceId } from "../core/review/resources"; +import { reviewErrorMessage } from "../session/reviewErrorCatalog"; +import { + encodeReviewEventFrame, + planReviewEventFrames, + reviewEventId, +} from "../session/reviewEventProtocol"; +import { + HUNK_REVIEW_CAPABILITY_HEADER, + reviewContentMeasurementHeaders, + reviewHttpPath, + reviewUrl, + type HunkReviewPublicationBodyV1, +} from "../session/reviewHttpProtocol"; +import { HUNK_REVIEW_PROTOCOL_VERSION } from "../session/reviewProtocol"; +import { parseBrowserReviewLocation, BrowserReviewApiClient } from "./browserReviewApiClient"; +import { browserReviewDigest } from "./browserReviewDigest"; + +const ORIGIN = "http://127.0.0.1:4300"; +const SESSION_ID = "session-1"; +const GENERATION = "generation:p1:3"; +const CAPABILITY = "c".repeat(43); +const FILE_KEY = "file:00000001"; +const RESOURCE_ID = reviewResourceId({ kind: "patch", fileKey: FILE_KEY }); + +const PUBLICATION: HunkReviewPublicationBodyV1 = { + protocolVersion: HUNK_REVIEW_PROTOCOL_VERSION, + sessionId: SESSION_ID, + publication: { generation: GENERATION, stateRevision: 7 }, + catalog: { + generation: GENERATION, + fileKeysByRuntimeId: { "file-1": FILE_KEY }, + resources: [ + { + id: RESOURCE_ID, + generation: GENERATION, + fileKey: FILE_KEY, + kind: "patch", + contentType: "text/x-diff; charset=utf-8", + }, + ], + }, +}; + +/** Build a client over a `fetch` that answers from a table of route handlers. */ +function clientOver(handle: (request: Request) => Response | Promise) { + const requests: Request[] = []; + const client = new BrowserReviewApiClient({ + origin: ORIGIN, + sessionId: SESSION_ID, + capability: CAPABILITY, + digest: browserReviewDigest, + fetch: Object.assign( + async (input: unknown, init: RequestInit | undefined) => { + const request = new Request(input as string, init); + requests.push(request); + return await handle(request); + }, + { preconnect: () => undefined }, + ) as unknown as typeof globalThis.fetch, + }); + return { client, requests }; +} + +/** Serve one resource the way the surface does: capped windows plus the measurement. */ +function serveResource(bytes: Uint8Array, options: { measure?: boolean } = {}) { + return (request: Request) => { + const range = /^bytes=(\d+)-(\d+)$/.exec(request.headers.get("range") ?? ""); + const start = range ? Number(range[1]) : 0; + const end = Math.min( + range ? Number(range[2]) : bytes.byteLength - 1, + bytes.byteLength - 1, + start + REVIEW_RESOURCE_CHUNK_BYTES - 1, + ); + return new Response(bytes.slice(start, end + 1), { + status: range ? 206 : 200, + headers: + options.measure === false + ? {} + : reviewContentMeasurementHeaders({ + byteLength: bytes.byteLength, + digest: browserReviewDigest(bytes), + }), + }); + }; +} + +describe("parseBrowserReviewLocation", () => { + test("reads the session and capability out of a review URL", () => { + const url = new URL(reviewUrl(ORIGIN, SESSION_ID, CAPABILITY)); + + expect(parseBrowserReviewLocation(url)).toEqual({ + origin: ORIGIN, + sessionId: SESSION_ID, + capability: CAPABILITY, + }); + }); + + test("refuses a URL carrying no capability", () => { + expect(parseBrowserReviewLocation(new URL(`${ORIGIN}/review/${SESSION_ID}/`))).toBeUndefined(); + }); + + test("refuses a malformed path as an answer, not an exception", () => { + // "%E0%A4%A" is truncated percent-encoding: decodeURIComponent throws on it. A + // hand-edited link must land on the invalid-link message, so the parser answers + // undefined instead of aborting whoever mounted the page. + const url = new URL(reviewUrl(ORIGIN, SESSION_ID, CAPABILITY)); + const malformed = { ...url, origin: url.origin, hash: url.hash, pathname: "/review/%E0%A4%A/" }; + + expect(parseBrowserReviewLocation(malformed)).toBeUndefined(); + }); +}); + +describe("BrowserReviewApiClient.readPublication", () => { + test("presents the capability in a header and nowhere else", async () => { + const { client, requests } = clientOver(() => Response.json(PUBLICATION)); + + await client.readPublication(); + + const request = requests[0]!; + expect(request.headers.get(HUNK_REVIEW_CAPABILITY_HEADER)).toBe(CAPABILITY); + expect(request.url).not.toContain(CAPABILITY); + expect(request.url).toBe( + `${ORIGIN}${reviewHttpPath({ kind: "publication", sessionId: SESSION_ID })}`, + ); + }); + + test("accepts a publication the wire protocol would accept", async () => { + const { client } = clientOver(() => Response.json(PUBLICATION)); + + expect(await client.readPublication()).toEqual({ ok: true, value: PUBLICATION }); + }); + + test("refuses a publication whose catalog belongs to another generation", async () => { + const { client } = clientOver(() => + Response.json({ + ...PUBLICATION, + catalog: { ...PUBLICATION.catalog, generation: "generation:p1:4" }, + }), + ); + + expect(await client.readPublication()).toMatchObject({ ok: false, code: "invalid-request" }); + }); + + test("reports the code and message the surface sent", async () => { + const { client } = clientOver(() => + Response.json( + { ok: false, code: "no-publication", message: "not yet", currentGeneration: GENERATION }, + { status: 409 }, + ), + ); + + expect(await client.readPublication()).toEqual({ + ok: false, + code: "no-publication", + message: "not yet", + currentGeneration: GENERATION, + }); + }); + + test("falls back to the shared catalog when a refusal carries no body", async () => { + const { client } = clientOver(() => new Response(null, { status: 416 })); + + expect(await client.readPublication()).toEqual({ + ok: false, + code: "invalid-range", + message: reviewErrorMessage("invalid-range"), + }); + }); +}); + +describe("BrowserReviewApiClient.readResource", () => { + const descriptor = { id: RESOURCE_ID, generation: GENERATION, kind: "patch" } as const; + + test("joins several windows into one verified resource", async () => { + const bytes = new Uint8Array(REVIEW_RESOURCE_CHUNK_BYTES * 2 + 17); + crypto.getRandomValues(bytes); + const { client, requests } = clientOver(serveResource(bytes)); + + const result = await client.readResource(descriptor); + + expect(result).toEqual({ ok: true, value: bytes }); + expect(requests).toHaveLength(3); + // The first window asks for no range at all, so an empty resource is readable. + expect(requests[0]!.headers.get("range")).toBeNull(); + expect(requests[1]!.headers.get("range")).toBe( + `bytes=${REVIEW_RESOURCE_CHUNK_BYTES}-${REVIEW_RESOURCE_CHUNK_BYTES * 2 - 1}`, + ); + }); + + test("reads a zero-length resource, which has no satisfiable range", async () => { + const { client, requests } = clientOver(serveResource(new Uint8Array(0))); + + expect(await client.readResource(descriptor)).toEqual({ ok: true, value: new Uint8Array(0) }); + expect(requests).toHaveLength(1); + }); + + test("refuses bytes that do not hash to the digest they were served with", async () => { + const bytes = new TextEncoder().encode("the patch"); + // The same length, so what fails is the digest rather than the arithmetic before it. + const { client } = clientOver( + () => + new Response(new TextEncoder().encode("the p4tch"), { + headers: reviewContentMeasurementHeaders({ + byteLength: bytes.byteLength, + digest: browserReviewDigest(bytes), + }), + }), + ); + + expect(await client.readResource(descriptor)).toMatchObject({ + ok: false, + code: "resource-integrity", + }); + }); + + test("refuses a window that changes the measurement mid-stream", async () => { + const first = new Uint8Array(REVIEW_RESOURCE_CHUNK_BYTES); + const whole = new Uint8Array(REVIEW_RESOURCE_CHUNK_BYTES * 2); + let window = 0; + const { client } = clientOver(() => { + const bytes = whole.slice(0, REVIEW_RESOURCE_CHUNK_BYTES); + const measurement = + window === 0 + ? { byteLength: whole.byteLength, digest: browserReviewDigest(whole) } + : { byteLength: whole.byteLength, digest: browserReviewDigest(first) }; + window += 1; + return new Response(bytes, { + status: 206, + headers: reviewContentMeasurementHeaders(measurement), + }); + }); + + expect(await client.readResource(descriptor)).toMatchObject({ + ok: false, + code: "resource-integrity", + }); + }); + + test("refuses bytes served without a measurement to verify them against", async () => { + const { client } = clientOver(serveResource(new TextEncoder().encode("x"), { measure: false })); + + expect(await client.readResource(descriptor)).toMatchObject({ + ok: false, + code: "resource-integrity", + }); + }); +}); + +describe("BrowserReviewApiClient.streamEvents", () => { + /** Serve one event stream built by the shared framer, as the surface builds it. */ + function serveEvents(body: unknown, chunkBytes?: number) { + const payload = new TextEncoder().encode(JSON.stringify(body)); + const frames = planReviewEventFrames({ + type: "publication", + address: PUBLICATION.publication, + body, + payload, + contentDigest: browserReviewDigest(payload), + encodeChunk: (bytes) => btoa(String.fromCharCode(...bytes)), + ...(chunkBytes === undefined ? {} : { chunkBytes }), + }); + return new Response(new TextEncoder().encode(frames.map(encodeReviewEventFrame).join("")), { + headers: { "content-type": "text/event-stream" }, + }); + } + + test("delivers one whole publication from a single-frame event", async () => { + const { client } = clientOver(() => serveEvents(PUBLICATION)); + const seen: unknown[] = []; + + await client.streamEvents({ onPublication: (body) => seen.push(body) }); + + expect(seen).toEqual([PUBLICATION]); + }); + + test("reassembles a chunked publication through the shared assembler", async () => { + const { client } = clientOver(() => serveEvents(PUBLICATION, 32)); + const seen: unknown[] = []; + + await client.streamEvents({ onPublication: (body) => seen.push(body) }); + + expect(seen).toEqual([PUBLICATION]); + }); + + test("reports a chunked payload whose bytes were tampered with", async () => { + const { client } = clientOver(() => { + const original = serveEvents(PUBLICATION, 32); + return original.text().then( + (text) => + new Response(text.replace(/"data":"([A-Za-z0-9+/=]{4})/, '"data":"AAAA'), { + headers: { "content-type": "text/event-stream" }, + }), + ); + }); + const failures: string[] = []; + + await client.streamEvents({ + onPublication: () => undefined, + onError: (failure) => failures.push(failure.code), + }); + + expect(failures).toContain("resource-integrity"); + }); + + test("ignores the heartbeat, which is a comment rather than an event", async () => { + const { client } = clientOver( + () => + new Response( + new TextEncoder().encode( + `: hunk-review-heartbeat\n\n${encodeReviewEventFrame({ + id: reviewEventId("disconnect", PUBLICATION.publication), + event: "disconnect", + data: { + eventId: reviewEventId("disconnect", PUBLICATION.publication), + generation: GENERATION, + stateRevision: PUBLICATION.publication.stateRevision, + payload: { sessionId: SESSION_ID }, + }, + })}`, + ), + { headers: { "content-type": "text/event-stream" } }, + ), + ); + let disconnected = false; + + await client.streamEvents({ + onPublication: () => undefined, + onDisconnect: () => { + disconnected = true; + }, + }); + + expect(disconnected).toBe(true); + }); + + test("reports a stream the surface refused to open", async () => { + const { client } = clientOver(() => + Response.json({ ok: false, code: "too-many-streams", message: "full" }, { status: 503 }), + ); + const failures: string[] = []; + + await client.streamEvents({ + onPublication: () => undefined, + onError: (failure) => failures.push(failure.code), + }); + + expect(failures).toEqual(["too-many-streams"]); + }); +}); diff --git a/src/web/browserReviewApiClient.ts b/src/web/browserReviewApiClient.ts new file mode 100644 index 000000000..d23477208 --- /dev/null +++ b/src/web/browserReviewApiClient.ts @@ -0,0 +1,547 @@ +/** + * Talking to one live review over the daemon's HTTP surface. + * + * Four things a browser client needs: read where the review is, read the content behind + * it, watch it move, and be told why a request was refused. Every rule about how those + * happen already exists — routes and authorization in `reviewHttpProtocol`, frame names and + * envelopes in `reviewEventProtocol`, chunk verification in the shared + * `ReviewChunkAssembler`, wording in `reviewErrorCatalog` — so this module composes them + * and owns only what a transport owns: requests, ranges, streams, and abort. + * + * That is the whole point of the C4/C2/G4 findings. The prototype's client re-declared the + * server's frame names, regex-parsed its event ids, wrote its own range loop with its own + * digest handling, and invented its own wording for failures + * (`docs/browser-review-seam-audit.md`). Nothing here declares any of that; the boundary + * gate in `scripts/source-boundaries.test.ts` keeps it that way by restricting what this + * tree may import at all. + * + * Two transport decisions worth stating: + * + * - **The stream is read with `fetch`, not `EventSource`.** The capability is presented in + * a request header and `EventSource` cannot set one. That also removes the built-in + * reconnect an `EventSource` would bring, so reconnect timing is the caller's, through + * the one shared scheduler (C5). + * - **A resource is read in windows and verified as one stream.** The surface caps every + * response at the shared chunk size, so a read is several requests; each response states + * the whole resource's size and digest, and the shared assembler holds every window to + * the first one's declaration and hashes the result. + */ +import { + REVIEW_RESOURCE_CHUNK_BYTES, + reviewResourceCeiling, + type ReviewResourceDescriptorV1, +} from "../core/review/resources"; +import { ReviewChunkAssembler } from "../core/review/resourceAssembly"; +import type { ReviewDigestFn } from "../core/review/validation"; +import { reviewErrorMessage } from "../session/reviewErrorCatalog"; +import { + parseReviewEventBegin, + parseReviewEventChunk, + parseReviewEventEnd, + parseReviewEventFrame, + parseReviewEventFrameName, + REVIEW_EVENT_STREAM_CONTENT_TYPE, + ReviewEventAssembler, + ReviewEventSseDecoder, + type ReviewEventSseRecord, + type ReviewEventTypeV1, +} from "../session/reviewEventProtocol"; +import { + HUNK_REVIEW_CAPABILITY_HEADER, + parseReviewContentMeasurementHeaders, + parseReviewCapabilityFragment, + reviewErrorCodeForStatus, + reviewHttpFailure, + reviewHttpPath, + reviewPagePath, + type HunkReviewClientErrorCodeV1, + type HunkReviewHttpFailureV1, + type HunkReviewHttpRoute, + type HunkReviewPublicationBodyV1, +} from "../session/reviewHttpProtocol"; +import { + HUNK_REVIEW_PROTOCOL_VERSION, + parseHunkReviewPublicationAddress, + parseHunkReviewResourceCatalog, +} from "../session/reviewProtocol"; +import { browserReviewDigest } from "./browserReviewDigest"; + +/** + * One refusal, exactly as the surface answers it. + * + * The wire type itself rather than a client-side restatement of its fields: a refusal this + * client reports is one the review surface either sent or would have sent, so there is no + * second shape to keep in step (G4). `reviewHttpFailure` builds them, on both ends. + */ +export type BrowserReviewFailure = HunkReviewHttpFailureV1; + +export type BrowserReviewResult = { ok: true; value: Value } | BrowserReviewFailure; + +/** + * The catalog's sentence for one code, with what this client can add about this instance. + * + * The catalog states what happened and what to do about it; a client knows which read or + * which frame it was. Appending rather than replacing is what keeps the browser and the + * terminal explaining the same failure the same way (G4). + */ +function withDetail(code: HunkReviewClientErrorCodeV1, detail: string) { + return detail ? `${reviewErrorMessage(code)} (${detail})` : reviewErrorMessage(code); +} + +/** What the client was told, or the fact that it could not be told anything. */ +function transportFailure(error: unknown): BrowserReviewFailure { + return reviewHttpFailure("resource-unavailable", { + message: withDetail("resource-unavailable", error instanceof Error ? error.message : ""), + }); +} + +/** Where this client is talking, and who it says it is. */ +export interface BrowserReviewApiClientOptions { + /** Origin the review surface is served from, e.g. `http://127.0.0.1:4300`. */ + origin: string; + sessionId: string; + /** The capability read from the review URL's fragment. */ + capability: string; + /** Injected so a test can drive a real server without a global. */ + fetch?: typeof globalThis.fetch; + /** Injected so the same client can be exercised against the session's own hashing. */ + digest?: ReviewDigestFn; +} + +/** What one review event stream reports to whoever opened it. */ +export interface BrowserReviewEventHandlers { + /** The review's current position and catalog, complete every time. */ + onPublication: (body: HunkReviewPublicationBodyV1) => void; + /** The session behind the stream is gone; no further event is coming. */ + onDisconnect?: () => void; + /** The stream ended or could not be read. Reconnecting is the caller's decision. */ + onError?: (failure: BrowserReviewFailure) => void; +} + +/** + * Read one review URL into the client that talks to it. + * + * The session id comes from the path and the capability from the fragment, both through + * the shared grammar, so a URL this cannot read is one the session never wrote. + */ +export function parseBrowserReviewLocation( + location: Pick, +): { origin: string; sessionId: string; capability: string } | undefined { + const capability = parseReviewCapabilityFragment(location.hash); + if (!capability) { + return undefined; + } + // Recognized by rebuilding the page path from each candidate segment rather than by + // matching a pattern this module would then own a second copy of. A segment whose + // percent-encoding does not decode is not a candidate rather than an error: this + // function answers "is this a review URL?", and a malformed one is not. + const segments = location.pathname.split("/").filter((segment) => segment.length > 0); + const sessionId = segments + .flatMap((segment) => { + try { + return [decodeURIComponent(segment)]; + } catch { + return []; + } + }) + .find((candidate) => location.pathname.startsWith(reviewPagePath(candidate))); + return sessionId ? { origin: location.origin, sessionId, capability } : undefined; +} + +export class BrowserReviewApiClient { + private readonly fetch: typeof globalThis.fetch; + private readonly digest: ReviewDigestFn; + + constructor(private readonly options: BrowserReviewApiClientOptions) { + this.fetch = options.fetch ?? globalThis.fetch.bind(globalThis); + this.digest = options.digest ?? browserReviewDigest; + } + + get sessionId() { + return this.options.sessionId; + } + + /** + * Read where the review is and what it offers there. + * + * The body is parsed through the wire protocol's own parsers rather than cast, so a + * publication this client accepts is one the daemon would also have accepted — the + * catalog especially, since every later read is addressed from it. + */ + async readPublication( + signal?: AbortSignal, + ): Promise> { + let response: Response; + try { + response = await this.request({ kind: "publication", sessionId: this.sessionId }, { signal }); + } catch (error) { + return transportFailure(error); + } + if (!response.ok) { + return this.readFailure(response); + } + return this.parsePublication(await response.json().catch(() => undefined)); + } + + /** + * Read one whole resource, verified against the measurement it is served with. + * + * Windows are requested at the shared chunk size and handed to the shared assembler, + * which is what refuses a stream that overlaps, skips, changes its declared size, or + * ends at a digest other than the one it opened with. + */ + async readResource( + descriptor: Pick, + signal?: AbortSignal, + ): Promise> { + const assembler = new ReviewChunkAssembler({ + resourceId: descriptor.id, + generation: descriptor.generation, + digest: this.digest, + maxBytes: reviewResourceCeiling(descriptor.kind), + }); + + for (;;) { + const window = await this.readResourceWindow(descriptor, assembler.nextOffset, signal); + if (!window.ok) { + return window; + } + const { bytes, measurement } = window.value; + const step = assembler.accept({ + chunk: { + generation: descriptor.generation, + resourceId: descriptor.id, + offset: assembler.nextOffset, + byteLength: bytes.byteLength, + // The record describes the window; the bytes ride in the response body rather + // than inside it, so the assembler is handed them decoded and `data` is empty. + encoding: "base64", + data: "", + contentDigest: measurement.digest, + contentSize: measurement.byteLength, + // HTTP has no end-of-stream marker of its own: a window ends the resource when + // it reaches the size every response states. The assembler still checks that + // claim against what it actually received. + eof: assembler.nextOffset + bytes.byteLength >= measurement.byteLength, + }, + bytes, + }); + if (!step.ok) { + return reviewHttpFailure(step.code, { message: step.message }); + } + if (step.done) { + break; + } + } + + const assembled = assembler.finish(); + return assembled.ok + ? { ok: true, value: assembled.bytes } + : reviewHttpFailure(assembled.code, { message: assembled.message }); + } + + /** + * Follow one review's event stream until it ends or the caller aborts. + * + * Resolves when the stream is over — normally, because the session disconnected or the + * caller aborted, or because reading failed. Whether and when to open another one is the + * caller's, which is what keeps reconnect policy in one place instead of two. + */ + async streamEvents(handlers: BrowserReviewEventHandlers, signal?: AbortSignal): Promise { + let response: Response; + try { + response = await this.request( + { kind: "events", sessionId: this.sessionId }, + { signal, headers: { accept: REVIEW_EVENT_STREAM_CONTENT_TYPE } }, + ); + } catch (error) { + handlers.onError?.(transportFailure(error)); + return; + } + if (!response.ok || !response.body) { + handlers.onError?.(await this.readFailure(response)); + return; + } + + const reader = new ReviewEventStreamReader(this.digest, handlers); + try { + await readServerSentRecords(response.body, (record) => reader.accept(record)); + reader.finishStream(); + } catch (error) { + // An abort is the caller ending the stream, not a failure to report. + if (!signal?.aborted) { + handlers.onError?.(transportFailure(error)); + } + } + } + + /** Narrow one publication body, refusing anything the wire protocol would not carry. */ + private parsePublication(value: unknown): BrowserReviewResult { + const record = + value && typeof value === "object" ? (value as Record) : undefined; + const publication = parseHunkReviewPublicationAddress(record?.publication); + const catalog = parseHunkReviewResourceCatalog(record?.catalog); + if ( + !record || + record.protocolVersion !== HUNK_REVIEW_PROTOCOL_VERSION || + record.sessionId !== this.sessionId || + !publication || + !catalog || + catalog.generation !== publication.generation + ) { + return reviewHttpFailure("invalid-request"); + } + return { + ok: true, + value: { + protocolVersion: HUNK_REVIEW_PROTOCOL_VERSION, + sessionId: this.sessionId, + publication, + catalog, + }, + }; + } + + /** Fetch one window of one resource, with the measurement the response states. */ + private async readResourceWindow( + descriptor: Pick, + offset: number, + signal: AbortSignal | undefined, + ): Promise< + BrowserReviewResult<{ bytes: Uint8Array; measurement: { byteLength: number; digest: string } }> + > { + let response: Response; + try { + response = await this.request( + { + kind: "resource", + sessionId: this.sessionId, + generation: descriptor.generation, + resourceId: descriptor.id, + }, + { + signal, + // The first window asks for no range at all: the surface caps every response at + // the shared chunk size anyway, and a zero-length resource has no satisfiable + // range, so asking for one would refuse a resource that is merely empty. + ...(offset === 0 + ? {} + : { + headers: { + range: `bytes=${offset}-${offset + REVIEW_RESOURCE_CHUNK_BYTES - 1}`, + }, + }), + }, + ); + } catch (error) { + return transportFailure(error); + } + if (!response.ok) { + return this.readFailure(response); + } + + const measurement = parseReviewContentMeasurementHeaders(response.headers); + if (!measurement) { + // Without a measurement there is nothing to verify against, and serving bytes that + // cannot be checked is worse than refusing them. + return reviewHttpFailure("resource-integrity"); + } + return { + ok: true, + value: { bytes: new Uint8Array(await response.arrayBuffer()), measurement }, + }; + } + + /** One request to one route, carrying the capability and nothing else identifying. */ + private request( + route: HunkReviewHttpRoute, + init: { signal?: AbortSignal; headers?: Record } = {}, + ) { + return this.fetch(`${this.options.origin}${reviewHttpPath(route)}`, { + headers: { + [HUNK_REVIEW_CAPABILITY_HEADER]: this.options.capability, + ...init.headers, + }, + ...(init.signal ? { signal: init.signal } : {}), + }); + } + + /** + * Read one refusal out of a response. + * + * The surface answers failures in one shape with a code from the shared vocabulary; a + * response that is not in that shape is reported by its status rather than guessed at, + * because a body this client cannot read is not one it should quote. + */ + private async readFailure(response: Response): Promise { + const body = (await response.json().catch(() => undefined)) as + | Partial + | undefined; + if (body?.ok === false && typeof body.code === "string") { + return reviewHttpFailure(body.code, { + ...(typeof body.message === "string" ? { message: body.message } : {}), + ...(typeof body.currentGeneration === "string" + ? { currentGeneration: body.currentGeneration } + : {}), + }); + } + // One route answers without a body: an unsatisfiable range is refused with a bare + // 416 and a `content-range` stating the size that should have been asked within. The + // status is read through the shared table, so this client holds no opinion about + // which code a status means; a status several codes share is not guessed at. + return reviewHttpFailure(reviewErrorCodeForStatus(response.status) ?? "invalid-request"); + } +} + +/** + * Read one byte stream as the records the review event protocol wrote. + * + * The record grammar itself is the protocol's, beside the writer that produces it; what + * this adds is the browser's half — decoding bytes to text and releasing the reader when + * the caller is done with the stream. + */ +async function readServerSentRecords( + body: ReadableStream, + onRecord: (record: ReviewEventSseRecord) => void, +) { + const reader = body.getReader(); + const textDecoder = new TextDecoder(); + const records = new ReviewEventSseDecoder(); + try { + for (;;) { + const { value, done } = await reader.read(); + if (done) { + break; + } + for (const record of records.push(textDecoder.decode(value, { stream: true }))) { + onRecord(record); + } + } + } finally { + await reader.cancel().catch(() => undefined); + } +} + +/** + * Turn a stream of records into the events they describe. + * + * Frame names, envelopes, and payload reassembly are all the shared protocol's — this + * holds the little state a reader needs between frames: which chunked event is open, and + * what to do when one completes. + */ +class ReviewEventStreamReader { + private assembler: ReviewEventAssembler | undefined; + private assemblingType: ReviewEventTypeV1 | undefined; + private disconnected = false; + + constructor( + private readonly digest: ReviewDigestFn, + private readonly handlers: BrowserReviewEventHandlers, + ) {} + + /** Take one record, dispatching whatever it completes. */ + accept(record: ReviewEventSseRecord) { + const frame = parseReviewEventFrameName(record.event); + if (!frame) { + return; + } + let data: unknown; + try { + data = JSON.parse(record.data); + } catch { + this.fail("a review event arrived that could not be read"); + return; + } + + if (frame.phase === undefined) { + const parsed = parseReviewEventFrame(data); + if (!parsed) { + this.fail("a review event arrived in a shape this client does not accept"); + return; + } + this.dispatch(frame.type, parsed.payload); + return; + } + if (frame.phase === "begin") { + const begin = parseReviewEventBegin(data); + if (!begin) { + this.fail("a chunked review event began in a shape this client does not accept"); + return; + } + this.assembler = new ReviewEventAssembler({ begin, digest: this.digest }); + this.assemblingType = frame.type; + return; + } + if (frame.phase === "chunk") { + const chunk = parseReviewEventChunk(data); + if (!chunk || !this.assembler) { + this.fail("a review event chunk arrived without a payload to belong to"); + return; + } + const step = this.assembler.accept(chunk, decodeBase64(chunk.data)); + if (!step.ok) { + this.fail(step.message); + } + return; + } + + const end = parseReviewEventEnd(data); + if (!end || !this.assembler || this.assemblingType !== frame.type) { + this.fail("a review event ended without a payload to complete"); + return; + } + const assembled = this.assembler.finish(end); + this.assembler = undefined; + if (!assembled.ok) { + this.fail(assembled.message); + return; + } + try { + this.dispatch(frame.type, JSON.parse(new TextDecoder().decode(assembled.bytes))); + } catch { + this.fail("a review event carried a payload that could not be read"); + } + } + + /** Report a stream that ended without the session saying goodbye. */ + finishStream() { + if (!this.disconnected) { + this.handlers.onError?.( + reviewHttpFailure("resource-unavailable", { + message: withDetail("resource-unavailable", "the review event stream ended"), + }), + ); + } + } + + /** Hand one completed event to the caller. */ + private dispatch(type: ReviewEventTypeV1, payload: unknown) { + if (type === "disconnect") { + this.disconnected = true; + this.handlers.onDisconnect?.(); + return; + } + // The stream carries no deltas: every publication is complete, so a client applies it + // whole rather than reconciling it against what it already had. + this.handlers.onPublication(payload as HunkReviewPublicationBodyV1); + } + + private fail(detail: string) { + this.assembler = undefined; + this.handlers.onError?.( + reviewHttpFailure("resource-integrity", { + message: withDetail("resource-integrity", detail), + }), + ); + } +} + +/** Decode one base64 chunk with the browser's own decoder. */ +function decodeBase64(data: string) { + const binary = atob(data); + const bytes = new Uint8Array(binary.length); + for (let index = 0; index < binary.length; index += 1) { + bytes[index] = binary.charCodeAt(index); + } + return bytes; +} diff --git a/src/web/browserReviewDigest.test.ts b/src/web/browserReviewDigest.test.ts new file mode 100644 index 000000000..cff649e7e --- /dev/null +++ b/src/web/browserReviewDigest.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, test } from "bun:test"; +import { createHash } from "node:crypto"; +import { isReviewSha256Digest } from "../core/review/validation"; +import { browserReviewDigest } from "./browserReviewDigest"; + +/** What the session's own edge would compute for the same bytes. */ +function nodeDigest(bytes: Uint8Array) { + return createHash("sha256").update(bytes).digest("hex"); +} + +describe("browserReviewDigest", () => { + test("matches the published vector for the empty input", () => { + expect(browserReviewDigest(new Uint8Array(0))).toBe( + "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + ); + }); + + test("matches the published vector for 'abc'", () => { + expect(browserReviewDigest(new TextEncoder().encode("abc"))).toBe( + "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad", + ); + }); + + // Block boundaries are where a padding bug hides: a message that fills a block exactly + // needs a whole extra block for its length, and one byte short of that does not. + test.each([1, 55, 56, 57, 63, 64, 65, 119, 120, 127, 128, 129, 1_000, 100_000])( + "agrees with the session's digest at %i bytes", + (byteLength) => { + const bytes = new Uint8Array(byteLength); + for (let index = 0; index < byteLength; index += 1) { + bytes[index] = (index * 31 + 7) & 0xff; + } + expect(browserReviewDigest(bytes)).toBe(nodeDigest(bytes)); + }, + ); + + test("agrees with the session's digest on random inputs", () => { + for (let round = 0; round < 64; round += 1) { + const bytes = new Uint8Array(Math.floor(Math.random() * 500)); + crypto.getRandomValues(bytes); + expect(browserReviewDigest(bytes)).toBe(nodeDigest(bytes)); + } + }); + + test("produces the canonical form the shared validator accepts", () => { + expect(isReviewSha256Digest(browserReviewDigest(new TextEncoder().encode("hunk")))).toBe(true); + }); +}); diff --git a/src/web/browserReviewDigest.ts b/src/web/browserReviewDigest.ts new file mode 100644 index 000000000..8e4ca32b1 --- /dev/null +++ b/src/web/browserReviewDigest.ts @@ -0,0 +1,121 @@ +/** + * SHA-256 for the browser review client, computed synchronously. + * + * The shared chunk assembler verifies a resource with an injected `ReviewDigestFn` + * (`src/core/review/validation.ts`), which is synchronous because verification happens + * inline as chunks arrive — a reader that had to await a hash could accept a chunk it has + * not yet checked. The platform hash a browser is handed, `crypto.subtle.digest`, is + * async-only, so the browser tier brings its own implementation rather than reshaping the + * seam around one runtime's API. + * + * That is why this file is here and not in `src/core/review/`: the audit lists platform + * hashing (`node:crypto` versus Web Crypto) among the things deliberately *not* unified + * (`docs/browser-review-seam-audit.md`, § A). The session's edge does the same thing on the + * other side in `src/lib/reviewDigest.ts`, and `reviewDigest.test.ts` holds this one to + * that implementation's output. + * + * It is used for integrity, never for secrecy: nothing here hashes a capability, and the + * capability the client holds is presented as-is over a loopback connection. + */ +import type { ReviewDigestFn } from "../core/review/validation"; + +/** The first thirty-two bits of the fractional parts of the cube roots of the first primes. */ +const ROUND_CONSTANTS = new Uint32Array([ + 0x428a2f98, 0x71374491, 0xb5c0fbcf, 0xe9b5dba5, 0x3956c25b, 0x59f111f1, 0x923f82a4, 0xab1c5ed5, + 0xd807aa98, 0x12835b01, 0x243185be, 0x550c7dc3, 0x72be5d74, 0x80deb1fe, 0x9bdc06a7, 0xc19bf174, + 0xe49b69c1, 0xefbe4786, 0x0fc19dc6, 0x240ca1cc, 0x2de92c6f, 0x4a7484aa, 0x5cb0a9dc, 0x76f988da, + 0x983e5152, 0xa831c66d, 0xb00327c8, 0xbf597fc7, 0xc6e00bf3, 0xd5a79147, 0x06ca6351, 0x14292967, + 0x27b70a85, 0x2e1b2138, 0x4d2c6dfc, 0x53380d13, 0x650a7354, 0x766a0abb, 0x81c2c92e, 0x92722c85, + 0xa2bfe8a1, 0xa81a664b, 0xc24b8b70, 0xc76c51a3, 0xd192e819, 0xd6990624, 0xf40e3585, 0x106aa070, + 0x19a4c116, 0x1e376c08, 0x2748774c, 0x34b0bcb5, 0x391c0cb3, 0x4ed8aa4a, 0x5b9cca4f, 0x682e6ff3, + 0x748f82ee, 0x78a5636f, 0x84c87814, 0x8cc70208, 0x90befffa, 0xa4506ceb, 0xbef9a3f7, 0xc67178f2, +]); + +/** The first thirty-two bits of the fractional parts of the square roots of the first primes. */ +const INITIAL_STATE = new Uint32Array([ + 0x6a09e667, 0xbb67ae85, 0x3c6ef372, 0xa54ff53a, 0x510e527f, 0x9b05688c, 0x1f83d9ab, 0x5be0cd19, +]); + +const BLOCK_BYTES = 64; +const ROUNDS = 64; + +/** Rotate one 32-bit word right, which is the only operation this hash needs beyond adds. */ +function rotateRight(value: number, bits: number) { + return (value >>> bits) | (value << (32 - bits)); +} + +/** Hash one message into eight 32-bit words of state. */ +function sha256State(message: Uint8Array) { + // One padded copy: the message, a 0x80 terminator, zeroes, and a 64-bit big-endian bit + // length, rounded up to whole blocks. + // Exactly the minimum: padding longer than the standard's is a different message. + const paddedLength = Math.ceil((message.byteLength + 9) / BLOCK_BYTES) * BLOCK_BYTES; + const padded = new Uint8Array(paddedLength); + padded.set(message); + padded[message.byteLength] = 0x80; + + const bitLength = message.byteLength * 8; + const view = new DataView(padded.buffer); + // Written as two 32-bit halves because a JavaScript number cannot hold the whole 64-bit + // length exactly; the high half is the byte count's top bits. + view.setUint32(paddedLength - 8, Math.floor(bitLength / 2 ** 32), false); + view.setUint32(paddedLength - 4, bitLength >>> 0, false); + + const state = new Uint32Array(INITIAL_STATE); + const schedule = new Uint32Array(ROUNDS); + + for (let offset = 0; offset < paddedLength; offset += BLOCK_BYTES) { + for (let index = 0; index < 16; index += 1) { + schedule[index] = view.getUint32(offset + index * 4, false); + } + for (let index = 16; index < ROUNDS; index += 1) { + const previous = schedule[index - 15]!; + const recent = schedule[index - 2]!; + const s0 = rotateRight(previous, 7) ^ rotateRight(previous, 18) ^ (previous >>> 3); + const s1 = rotateRight(recent, 17) ^ rotateRight(recent, 19) ^ (recent >>> 10); + schedule[index] = (schedule[index - 16]! + s0 + schedule[index - 7]! + s1) >>> 0; + } + + let [a, b, c, d, e, f, g, h] = state as unknown as number[]; + for (let index = 0; index < ROUNDS; index += 1) { + const s1 = rotateRight(e!, 6) ^ rotateRight(e!, 11) ^ rotateRight(e!, 25); + const choice = (e! & f!) ^ (~e! & g!); + const temp1 = (h! + s1 + choice + ROUND_CONSTANTS[index]! + schedule[index]!) >>> 0; + const s0 = rotateRight(a!, 2) ^ rotateRight(a!, 13) ^ rotateRight(a!, 22); + const majority = (a! & b!) ^ (a! & c!) ^ (b! & c!); + const temp2 = (s0 + majority) >>> 0; + + h = g; + g = f; + f = e; + e = (d! + temp1) >>> 0; + d = c; + c = b; + b = a; + a = (temp1 + temp2) >>> 0; + } + + const round = [a!, b!, c!, d!, e!, f!, g!, h!]; + for (let index = 0; index < state.length; index += 1) { + state[index] = (state[index]! + round[index]!) >>> 0; + } + } + + return state; +} + +/** + * Hash one byte array to the canonical lowercase hex digest the review model compares. + * + * Lowercase without exception: `isReviewSha256Digest` accepts only that form, because a + * case-insensitive comparison is what let a writer and a reader disagree about whether two + * digests matched (`docs/browser-review-seam-audit.md`, D5). + */ +export const browserReviewDigest: ReviewDigestFn = (bytes: Uint8Array) => { + const state = sha256State(bytes); + let hex = ""; + for (const word of state) { + hex += word.toString(16).padStart(8, "0"); + } + return hex; +}; diff --git a/src/web/browserReviewMirror.test.ts b/src/web/browserReviewMirror.test.ts new file mode 100644 index 000000000..f54037363 --- /dev/null +++ b/src/web/browserReviewMirror.test.ts @@ -0,0 +1,403 @@ +import { describe, expect, test } from "bun:test"; +import { projectReviewDocument } from "../core/review/document"; +import type { ReviewPublicationAddress } from "../core/review/generationOrder"; +import { reviewResourceId } from "../core/review/resources"; +import type { ReviewFileV1 } from "../core/review/types"; +import { createTestDiffFile } from "../../test/helpers/diff-helpers"; +import { + HUNK_REVIEW_PROTOCOL_VERSION, + type HunkReviewResourceCatalogV1, +} from "../session/reviewProtocol"; +import { reviewHttpFailure, type HunkReviewPublicationBodyV1 } from "../session/reviewHttpProtocol"; +import type { BrowserReviewResult, BrowserReviewEventHandlers } from "./browserReviewApiClient"; +import { + BrowserReviewMirror, + type BrowserReviewMirrorSnapshot, + type BrowserReviewMirrorSource, +} from "./browserReviewMirror"; + +const SESSION_ID = "session-1"; + +/** Project real files, so what the mirror parses is what a producer would have served. */ +function documentFor(paths: string[]): ReviewFileV1[] { + return projectReviewDocument( + paths.map((path, index) => createTestDiffFile({ id: `file-${index}`, path })), + { sourceLabel: "/repo" }, + ).files; +} + +/** The catalog one generation of those files publishes, in review order. */ +function catalogFor(generation: string, files: ReviewFileV1[]): HunkReviewResourceCatalogV1 { + return { + generation, + fileKeysByRuntimeId: Object.fromEntries(files.map((file) => [file.runtimeId, file.key])), + resources: files.flatMap((file) => [ + { + id: reviewResourceId({ kind: "canonical-file", fileKey: file.key }), + generation, + fileKey: file.key, + kind: "canonical-file" as const, + contentType: "application/vnd.hunk.review-file+json; charset=utf-8" as const, + }, + { + id: reviewResourceId({ kind: "patch", fileKey: file.key }), + generation, + fileKey: file.key, + kind: "patch" as const, + contentType: "text/x-diff; charset=utf-8" as const, + }, + ]), + }; +} + +function publicationFor( + address: ReviewPublicationAddress, + files: ReviewFileV1[], +): HunkReviewPublicationBodyV1 { + return { + protocolVersion: HUNK_REVIEW_PROTOCOL_VERSION, + sessionId: SESSION_ID, + publication: address, + catalog: catalogFor(address.generation, files), + }; +} + +/** + * A transport a test drives by hand. + * + * The stream is a handle rather than a promise so a test can deliver publications in the + * order it wants to reason about, which is what the ordering rules are all about. + */ +function createTestSource() { + const encoder = new TextEncoder(); + const filesByKey = new Map(); + const reads: string[] = []; + let handlers: BrowserReviewEventHandlers | undefined; + let endStream: (() => void) | undefined; + let pendingRead: (() => void) | undefined; + let holdReads = false; + let streams = 0; + + const source: BrowserReviewMirrorSource = { + async readResource(descriptor): Promise> { + reads.push(descriptor.id); + if (holdReads) { + await new Promise((resolve) => { + pendingRead = resolve; + }); + } + const file = [...filesByKey.values()].find( + (candidate) => + reviewResourceId({ kind: "canonical-file", fileKey: candidate.key }) === descriptor.id, + ); + return file + ? { ok: true, value: encoder.encode(JSON.stringify(file)) } + : reviewHttpFailure("unknown-resource"); + }, + streamEvents(next, signal) { + streams += 1; + handlers = next; + return new Promise((resolve) => { + endStream = resolve; + // A detached stream really ends, the way an aborted `fetch` body does, so a test + // cannot keep delivering events to a mirror that stopped listening. + signal?.addEventListener("abort", () => { + handlers = undefined; + resolve(); + }); + }); + }, + }; + + return { + source, + reads, + /** How many streams the mirror has opened. */ + streamCount: () => streams, + /** Make one generation's files readable. */ + offer(files: ReviewFileV1[]) { + for (const file of files) { + filesByKey.set(file.key, file); + } + }, + publish(body: HunkReviewPublicationBodyV1) { + handlers?.onPublication(body); + }, + disconnect() { + handlers?.onDisconnect?.(); + }, + /** Drop the stream the way the real client does: an error, then the read ending. */ + dropStream() { + handlers?.onError?.(reviewHttpFailure("resource-unavailable")); + handlers = undefined; + endStream?.(); + endStream = undefined; + }, + /** Stall every resource read until `releaseReads` is called. */ + holdReads() { + holdReads = true; + }, + releaseReads() { + holdReads = false; + pendingRead?.(); + pendingRead = undefined; + }, + end() { + endStream?.(); + }, + }; +} + +/** Let every already-resolved promise settle. */ +async function settle() { + for (let turn = 0; turn < 8; turn += 1) { + await Promise.resolve(); + } +} + +/** + * Timers a test runs by hand. + * + * A reconnect never fires on its own — no ordering assertion has one running underneath it — + * and a test that is about reconnecting calls `reconnectNow` to run the one that is pending. + */ +function createTestTimers() { + let due: (() => void) | undefined; + return { + timers: { + setTimeout: (handler: () => void) => { + due = handler; + return 1; + }, + clearTimeout: () => { + due = undefined; + }, + }, + reconnectNow() { + const handler = due; + due = undefined; + handler?.(); + }, + }; +} + +function createMirror() { + const transport = createTestSource(); + const { timers, reconnectNow } = createTestTimers(); + const mirror = new BrowserReviewMirror(transport.source, { timers }); + const seen: BrowserReviewMirrorSnapshot[] = []; + mirror.subscribe((snapshot) => seen.push(snapshot)); + return { ...transport, mirror, seen, reconnectNow }; +} + +describe("BrowserReviewMirror", () => { + test("loads the document the first publication describes, in review order", async () => { + const files = documentFor(["src/alpha.ts", "src/beta.ts"]); + const harness = createMirror(); + harness.offer(files); + + harness.mirror.start(); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, files)); + await settle(); + + expect(harness.mirror.getSnapshot().status).toBe("ready"); + expect(harness.mirror.getSnapshot().document?.files.map((file) => file.path)).toEqual([ + "src/alpha.ts", + "src/beta.ts", + ]); + // Only canonical files are read: the patch is carried inside one. + expect(harness.reads.every((id) => id.startsWith("resource:canonical-file:"))).toBe(true); + }); + + test("advances the position without re-reading a generation it already holds", async () => { + const files = documentFor(["src/alpha.ts"]); + const harness = createMirror(); + harness.offer(files); + harness.mirror.start(); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, files)); + await settle(); + const readsAfterLoad = harness.reads.length; + + // Revisions need not be contiguous — a receiver legitimately sees jumps — which is the + // rule the prototype's client got wrong. + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 9 }, files)); + await settle(); + + expect(harness.mirror.getSnapshot().publication).toEqual({ + generation: "generation:p1:0", + stateRevision: 9, + }); + expect(harness.reads).toHaveLength(readsAfterLoad); + }); + + test("ignores a publication behind the one it holds", async () => { + const files = documentFor(["src/alpha.ts"]); + const harness = createMirror(); + harness.offer(files); + harness.mirror.start(); + harness.publish(publicationFor({ generation: "generation:p1:1", stateRevision: 4 }, files)); + await settle(); + + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 9 }, files)); + harness.publish(publicationFor({ generation: "generation:p1:1", stateRevision: 4 }, files)); + await settle(); + + expect(harness.mirror.getSnapshot().publication).toEqual({ + generation: "generation:p1:1", + stateRevision: 4, + }); + }); + + test("resyncs onto a later generation and drops what the older one was loading", async () => { + const first = documentFor(["src/alpha.ts"]); + const second = documentFor(["src/gamma.ts"]); + const harness = createMirror(); + harness.offer([...first, ...second]); + harness.mirror.start(); + harness.holdReads(); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, first)); + await settle(); + + // The newer generation arrives while the older one is still reading its files. + harness.publish(publicationFor({ generation: "generation:p1:1", stateRevision: 1 }, second)); + harness.releaseReads(); + await settle(); + + expect(harness.mirror.getSnapshot().status).toBe("ready"); + expect(harness.mirror.getSnapshot().publication?.generation).toBe("generation:p1:1"); + expect(harness.mirror.getSnapshot().document?.files.map((file) => file.path)).toEqual([ + "src/gamma.ts", + ]); + }); + + test("reports a file it cannot read, keeping the failure's own code", async () => { + const files = documentFor(["src/alpha.ts"]); + const harness = createMirror(); + + harness.mirror.start(); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, files)); + await settle(); + + expect(harness.mirror.getSnapshot()).toMatchObject({ + status: "failed", + failure: { code: "unknown-resource" }, + }); + }); + + test("refuses a file whose content does not hash to the identity it declares", async () => { + const files = documentFor(["src/alpha.ts"]); + const harness = createMirror(); + // Exactly the D4 failure: a file that arrives describing content it is not. + harness.offer([{ ...files[0]!, path: "src/tampered.ts" }]); + + harness.mirror.start(); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, files)); + await settle(); + + expect(harness.mirror.getSnapshot()).toMatchObject({ + status: "failed", + failure: { code: "resource-integrity" }, + }); + }); + + test("reads the document again when the publication that failed is delivered again", async () => { + const files = documentFor(["src/alpha.ts"]); + const harness = createMirror(); + // Nothing is readable yet, so the first load fails the way a transient refusal does. + harness.mirror.start(); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, files)); + await settle(); + expect(harness.mirror.getSnapshot().status).toBe("failed"); + + harness.offer(files); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, files)); + await settle(); + + expect(harness.mirror.getSnapshot().status).toBe("ready"); + expect(harness.mirror.getSnapshot().document?.files).toHaveLength(1); + expect(harness.mirror.getSnapshot().failure).toBeUndefined(); + }); + + test("keeps the document a dropped stream did not invalidate, and recovers when it resyncs", async () => { + const files = documentFor(["src/alpha.ts"]); + const harness = createMirror(); + harness.offer(files); + harness.mirror.start(); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, files)); + await settle(); + const readsAfterLoad = harness.reads.length; + + harness.dropStream(); + await settle(); + + // The link is gone, not the review: the diff stays on screen while the retry is pending. + expect(harness.mirror.getSnapshot().status).toBe("reconnecting"); + expect(harness.mirror.getSnapshot().document?.files).toHaveLength(1); + + harness.reconnectNow(); + await settle(); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, files)); + await settle(); + + expect(harness.mirror.getSnapshot().status).toBe("ready"); + expect(harness.mirror.getSnapshot().failure).toBeUndefined(); + // The generation is immutable, so the resync confirmed the document rather than re-read it. + expect(harness.reads).toHaveLength(readsAfterLoad); + }); + + test("never moves the position back to where a finished load started", async () => { + const files = documentFor(["src/alpha.ts"]); + const harness = createMirror(); + harness.offer(files); + harness.mirror.start(); + harness.holdReads(); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, files)); + await settle(); + + // The review moves on within the generation while its document is still being read. + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 2 }, files)); + harness.releaseReads(); + await settle(); + + expect(harness.mirror.getSnapshot().status).toBe("ready"); + expect(harness.mirror.getSnapshot().publication).toEqual({ + generation: "generation:p1:0", + stateRevision: 2, + }); + }); + + test("attaches again after it was detached, so a remounted view is not left dead", async () => { + const files = documentFor(["src/alpha.ts"]); + const harness = createMirror(); + harness.offer(files); + + harness.mirror.start(); + harness.mirror.stop(); + await settle(); + harness.mirror.start(); + await settle(); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, files)); + await settle(); + + expect(harness.streamCount()).toBe(2); + expect(harness.mirror.getSnapshot().status).toBe("ready"); + expect(harness.mirror.getSnapshot().document?.files).toHaveLength(1); + }); + + test("stops for good when the session says goodbye", async () => { + const files = documentFor(["src/alpha.ts"]); + const harness = createMirror(); + harness.offer(files); + harness.mirror.start(); + harness.publish(publicationFor({ generation: "generation:p1:0", stateRevision: 1 }, files)); + await settle(); + + harness.disconnect(); + harness.end(); + await settle(); + + expect(harness.mirror.getSnapshot().status).toBe("disconnected"); + // The document stays on screen: the session is gone, not the review it published. + expect(harness.mirror.getSnapshot().document?.files).toHaveLength(1); + }); +}); diff --git a/src/web/browserReviewMirror.ts b/src/web/browserReviewMirror.ts new file mode 100644 index 000000000..2258e2f69 --- /dev/null +++ b/src/web/browserReviewMirror.ts @@ -0,0 +1,428 @@ +/** + * The browser's copy of one live review, and what it does when the review moves. + * + * A publication is a position plus a resource catalog, never a diff — so mirroring one + * means ordering it, and then reading the document behind it out of the catalog it names. + * Both halves are where the prototype went wrong: its client required contiguous state + * revisions the server never promised, and recovered from a missed snapshot with a + * trailing while-loop racing its own epoch counter + * (`docs/browser-review-seam-audit.md`, C1/C3). + * + * Three rules shape this module: + * + * - **Ordering is `classifyReviewPublication` and nothing else.** This mirror has no + * comparison of its own, which is what the ordering conformance suite proves rather than + * trusts. + * - **A generation is immutable, so a resync happens exactly on `gap`.** Within one + * generation the document cannot change — only the review's position in it — so an + * `accepted` publication advances the position and reads nothing. A new generation + * replaces everything derived from the old one. + * - **One load at a time, and the newest wins.** A resync belongs to the attachment and + * generation that started it; when a later generation arrives, or the mirror is detached, + * the in-flight load is abandoned rather than allowed to finish and overwrite what came + * after it. + * + * Ordering is only half of what the mirror tracks, and conflating the two halves is what + * left the first version with no way back from a failure: what publication is *current* is + * the classifier's answer, and what the document on screen was *read for* is load state + * (`loadedGeneration`). Read together they say when a publication has to be read again — + * a new generation, or a generation whose document the mirror does not have — and when a + * publication is merely news that the link is working, which is how a dropped stream comes + * back without re-reading a document that was never invalidated. + * + * What a publication does *not* carry is worth stating, because it is the question Phase 5 + * was told to answer first: selection, filter, expansion, and notes live in the producer's + * `ReviewState`, and no resource in the catalog contains them. A read-only mirror is + * therefore a mirror of the review's *content*; sharing its semantic position needs more + * on the wire than a publication has, which is Phase 5 PR 2's work. + */ +import { createReconnectScheduler, inBoundedParallel } from "@hunk/session-broker-core"; +import { + classifyReviewPublication, + type ReviewPublicationAddress, +} from "../core/review/generationOrder"; +import { reviewFileContentIdentityOf } from "../core/review/document"; +import { REVIEW_RESOURCE_LOAD_CONCURRENCY } from "../core/review/resources"; +import type { ReviewDocumentV1, ReviewFileV1 } from "../core/review/types"; +import { reviewErrorMessage } from "../session/reviewErrorCatalog"; +import { reviewHttpFailure, type HunkReviewPublicationBodyV1 } from "../session/reviewHttpProtocol"; +import type { HunkReviewResourceCatalogV1 } from "../session/reviewProtocol"; +import type { BrowserReviewApiClient, BrowserReviewFailure } from "./browserReviewApiClient"; + +/** + * How soon a dropped stream is retried, and how far apart retries grow. + * + * Deliberately short at first — a daemon restarting between two keystrokes should not cost + * a reviewer four seconds of blank screen — and bounded well below any human's patience. + * Jitter matters because one daemon restart drops every tab at once. + */ +const RECONNECT_DELAY_MS = 500; +const RECONNECT_FACTOR = 2; +const RECONNECT_MAX_DELAY_MS = 15_000; +const RECONNECT_JITTER = 0.25; + +/** What this mirror is doing, in the terms a screen has to say something about. */ +export type BrowserReviewMirrorStatus = + /** Nothing has been asked for yet. */ + | "idle" + /** Attached, and reading the document one publication describes. */ + | "loading" + /** Holding a complete document for the publication below. */ + | "ready" + /** The stream dropped over a document that is still good; a retry is pending. */ + | "reconnecting" + /** The last attempt failed, and there is no document to show; a retry may be pending. */ + | "failed" + /** The session ended. Nothing further is coming, and no retry will help. */ + | "disconnected"; + +export interface BrowserReviewMirrorSnapshot { + status: BrowserReviewMirrorStatus; + /** Where the review is, as of the last publication accepted. */ + publication?: ReviewPublicationAddress; + /** The document behind that publication, once every file has been read and verified. */ + document?: ReviewDocumentV1; + /** Why the last attempt failed, in the shared vocabulary and wording. */ + failure?: BrowserReviewFailure; +} + +/** + * What the mirror needs from a transport: content, and news that the review moved. + * + * Named as the two methods rather than as the client class so the ordering rules can be + * driven without a listener, which is how the conformance harness asks them. + */ +export type BrowserReviewMirrorSource = Pick< + BrowserReviewApiClient, + "readResource" | "streamEvents" +>; + +export interface BrowserReviewMirrorOptions { + /** Injected so a test can drive time instead of waiting for it. */ + timers?: Parameters[0]["timers"]; +} + +export class BrowserReviewMirror { + private snapshot: BrowserReviewMirrorSnapshot = { status: "idle" }; + private readonly listeners = new Set<(snapshot: BrowserReviewMirrorSnapshot) => void>(); + private readonly reconnect: ReturnType; + private streamAbort: AbortController | undefined; + /** + * The load in flight, if any, and the token that says it is still the one wanted. + * + * This is the whole supersede rule: a load checks that its token is still the pending one + * before it publishes anything, so a newer load or a `stop` simply makes the older load's + * result unwanted rather than something to cancel and unwind. A token rather than a + * generation, because a failed load is retried for the generation it already had. + */ + private pendingLoad: { token: number; publication: ReviewPublicationAddress } | undefined; + private loadCount = 0; + /** + * The generation the document on screen was read for. + * + * Load state, not ordering: it says which publications the mirror can answer without + * reading anything, and is absent whenever there is no document to answer with. + */ + private loadedGeneration: string | undefined; + private stopped = false; + + constructor( + private readonly client: BrowserReviewMirrorSource, + options: BrowserReviewMirrorOptions = {}, + ) { + this.reconnect = createReconnectScheduler({ + delayMs: RECONNECT_DELAY_MS, + factor: RECONNECT_FACTOR, + maxDelayMs: RECONNECT_MAX_DELAY_MS, + jitter: RECONNECT_JITTER, + onDue: () => void this.connect(), + ...(options.timers ? { timers: options.timers } : {}), + }); + } + + /** The current view of the review, safe to render directly. */ + getSnapshot(): BrowserReviewMirrorSnapshot { + return this.snapshot; + } + + /** Watch the mirror. The listener is not called for the state it already sees. */ + subscribe(listener: (snapshot: BrowserReviewMirrorSnapshot) => void) { + this.listeners.add(listener); + return () => { + this.listeners.delete(listener); + }; + } + + /** + * Attach to the review. + * + * Only the event stream is opened: its first frame is always a complete publication, so + * a separate initial fetch would ask the same question twice and give the two answers a + * chance to disagree. + * + * Attaching again after `stop` is deliberate. A view that mounts twice — React's + * development double-mount, a route left and returned to — asks the same mirror to start + * twice, and a mirror that could only be detached would leave the second mount blank. A + * session that said goodbye is the exception: there is nothing left to attach to. + */ + start() { + if (this.streamAbort || this.snapshot.status === "disconnected") { + return; + } + this.stopped = false; + this.reconnect.reset(); + void this.connect(); + } + + /** Detach: end the stream, abandon any load in flight, and retry nothing until `start`. */ + stop() { + this.stopped = true; + // Cancelled rather than stopped: the shared scheduler cannot be restarted and this + // mirror can, so refusing later retries is this flag's job rather than the timer's. + this.reconnect.cancel(); + this.streamAbort?.abort(); + this.streamAbort = undefined; + // A load belongs to the attachment that started it; whatever it returns after this + // describes a review nobody is watching. + this.pendingLoad = undefined; + } + + /** Open one event stream and follow it until it ends. */ + private async connect() { + if (this.stopped) { + return; + } + const abort = new AbortController(); + this.streamAbort = abort; + await this.client.streamEvents( + { + onPublication: (body) => { + if (abort.signal.aborted) { + return; + } + // A stream that delivers anything is a working stream; the next failure starts + // its backoff from the beginning rather than from where the last one left off. + this.reconnect.reset(); + this.observe(body); + }, + onDisconnect: () => { + if (abort.signal.aborted) { + return; + } + this.reconnect.cancel(); + this.publish({ ...this.snapshot, status: "disconnected" }); + }, + onError: (failure) => { + if (!abort.signal.aborted) { + this.fail(failure); + } + }, + }, + abort.signal, + ); + if (this.streamAbort !== abort) { + // Detached, or replaced by a later attachment: this stream ending says nothing about + // the one that took its place, and scheduling from here would open a second. + return; + } + this.streamAbort = undefined; + if (!this.stopped && this.snapshot.status !== "disconnected") { + this.reconnect.schedule(); + } + } + + /** + * Take one arriving publication. + * + * The first one has nothing to be ordered against, so it is adopted; every later one is + * classified, and only the classifier decides where it sits. What that verdict means for + * the *document* is then load state's question: a publication whose generation the mirror + * has no document for has to be read, whether it advances the position or merely repeats + * it, and that is the retry after a load that failed. + */ + private observe(body: HunkReviewPublicationBodyV1) { + const current = this.snapshot.publication; + if (!current) { + void this.load(body); + return; + } + const order = classifyReviewPublication(current, body.publication); + if (order === "gap") { + // A later generation: nothing derived from the old one carries over, document first. + void this.load(body); + return; + } + if (order === "stale" && body.publication.generation !== current.generation) { + // Behind, replayed from another producer, or an identity that does not parse. Not a + // position this mirror is on, and not a document it should adopt. + return; + } + const document = + this.loadedGeneration === body.publication.generation ? this.snapshot.document : undefined; + if (!document && !this.pendingLoad) { + void this.load(body); + return; + } + if (document) { + // The document is the one this generation names, so the publication is a position and + // proof the link works: a stream that dropped and came back recovers here rather than + // staying failed, and a replay of a position already held only does that much. + if (order === "accepted" || this.snapshot.status !== "ready") { + this.publish({ + status: "ready", + publication: order === "accepted" ? body.publication : current, + document, + }); + } + return; + } + if (order === "accepted") { + // A load for this generation is already reading; this only moves the position it will + // publish at when it finishes. + this.publish({ ...this.snapshot, publication: body.publication }); + } + } + + /** Read the whole document one publication describes, unless a newer one arrives first. */ + private async load(body: HunkReviewPublicationBodyV1) { + this.loadCount += 1; + const token = this.loadCount; + this.pendingLoad = { token, publication: body.publication }; + this.loadedGeneration = undefined; + this.publish({ status: "loading", publication: body.publication }); + + const files = await this.readDocumentFiles(body.catalog); + if (this.pendingLoad?.token !== token) { + // Superseded while it was reading — by a later load, or by a `stop` — so this + // document describes a review nobody is looking at any more. + return; + } + this.pendingLoad = undefined; + if (!files.ok) { + this.fail(files); + return; + } + this.loadedGeneration = body.publication.generation; + this.publish({ + status: "ready", + publication: this.furthestPublication(body.publication), + document: { files: files.value }, + }); + } + + /** + * The further along of where this load started and where the mirror has moved since. + * + * A load takes time, and the review may advance within its generation while it reads. The + * document is the same either way, so finishing must not carry the position back to where + * the read began — and which of the two is further along is the shared classifier's + * answer, not a comparison of this module's own. + */ + private furthestPublication(loaded: ReviewPublicationAddress): ReviewPublicationAddress { + const held = this.snapshot.publication; + return held && classifyReviewPublication(loaded, held) === "accepted" ? held : loaded; + } + + /** + * Read every file in one catalog, in review order. + * + * Order is the catalog's, which is the review's — the producer lists each file's + * resources in document order, and document order is sidebar and stream order. Loads run + * under the shared concurrency bound rather than as one unbounded `Promise.all` (C2). + */ + private async readDocumentFiles( + catalog: HunkReviewResourceCatalogV1, + ): Promise<{ ok: true; value: ReviewFileV1[] } | BrowserReviewFailure> { + const descriptors = catalog.resources.filter( + (resource): resource is Extract => + resource.kind === "canonical-file", + ); + const loaded = await inBoundedParallel( + descriptors, + REVIEW_RESOURCE_LOAD_CONCURRENCY, + (descriptor) => this.readCanonicalFile(descriptor), + ); + + const files: ReviewFileV1[] = []; + for (const result of loaded) { + if (!result.ok) { + return result; + } + files.push(result.value); + } + return { ok: true, value: files }; + } + + /** + * Read one canonical file and check that it still describes what it claims to. + * + * The bytes are already verified against the digest they were served with; what this + * adds is that they parse as the file the model expects, which is asked by recomputing + * the file's own content identity rather than by checking a list of fields this client + * would then own a copy of (D4). + */ + private async readCanonicalFile( + descriptor: HunkReviewResourceCatalogV1["resources"][number], + ): Promise<{ ok: true; value: ReviewFileV1 } | BrowserReviewFailure> { + const bytes = await this.client.readResource(descriptor); + if (!bytes.ok) { + return bytes; + } + let file: ReviewFileV1; + try { + file = JSON.parse(new TextDecoder().decode(bytes.value)) as ReviewFileV1; + if (file.key !== descriptor.fileKey) { + throw new Error(`it describes ${file.key}`); + } + if (reviewFileContentIdentityOf(file) !== file.contentIdentity) { + throw new Error("its content does not hash to the identity it declares"); + } + } catch (error) { + // The catalog says what a mismatched read means and what to do about it; the file + // and the reason it failed are what this mirror can add (G4). + return reviewHttpFailure("resource-integrity", { + message: `${reviewErrorMessage("resource-integrity")} (the review served a file for ${ + descriptor.fileKey + } that could not be read${error instanceof Error ? `: ${error.message}` : ""})`, + }); + } + return { ok: true, value: file }; + } + + /** + * Record one failure, keeping whatever document is still on screen. + * + * A dropped stream over a document the review never invalidated is a link problem rather + * than a lost review, so it degrades to `reconnecting` — the diff stays readable while + * the retry is pending, and the next publication over the new stream restores `ready`. + * Without a document there is nothing to keep reading, and that is `failed`. + */ + private fail(failure: BrowserReviewFailure) { + if (this.snapshot.status === "disconnected") { + return; + } + this.publish({ + ...this.snapshot, + status: this.holdsLoadedDocument() ? "reconnecting" : "failed", + failure, + }); + } + + /** Whether the document on screen is the one the current publication's generation names. */ + private holdsLoadedDocument() { + return ( + this.snapshot.document !== undefined && + this.loadedGeneration !== undefined && + this.loadedGeneration === this.snapshot.publication?.generation + ); + } + + /** Move to one snapshot and tell everyone watching. */ + private publish(snapshot: BrowserReviewMirrorSnapshot) { + this.snapshot = snapshot; + // A copy, so a listener that unsubscribes while being told does not skip the next one. + for (const listener of Array.from(this.listeners)) { + listener(snapshot); + } + } +} diff --git a/src/web/browserReviewSources.test.ts b/src/web/browserReviewSources.test.ts new file mode 100644 index 000000000..b0020b70f --- /dev/null +++ b/src/web/browserReviewSources.test.ts @@ -0,0 +1,177 @@ +import { describe, expect, test } from "bun:test"; +import { projectReviewDocument } from "../core/review/document"; +import type { ReviewFileV1 } from "../core/review/types"; +import { createTestDiffFile, createTestSourceFetcher } from "../../test/helpers/diff-helpers"; +import { reviewErrorMessage } from "../session/reviewErrorCatalog"; +import { reviewHttpFailure } from "../session/reviewHttpProtocol"; +import type { BrowserReviewResult } from "./browserReviewApiClient"; +import { BrowserReviewSourceStore, type BrowserReviewSourceReader } from "./browserReviewSources"; + +const BASE = `${Array.from({ length: 12 }, (_unused, index) => `line ${index + 1}`).join("\n")}\n`; + +/** One file a review would publish, with source the page can expand gaps from. */ +function testFile(): ReviewFileV1 { + return projectReviewDocument( + [ + createTestDiffFile({ + id: "alpha", + path: "src/alpha.ts", + before: BASE, + after: BASE.replace("line 4", "line 4 changed"), + context: 1, + // A file with a fetcher is one the review offers a source resource for. + sourceFetcher: createTestSourceFetcher(() => BASE), + }), + ], + { sourceLabel: "/repo" }, + ).files[0]!; +} + +/** A reader a test resolves by hand, so a read can be left in flight. */ +function createTestReader() { + const requests: string[] = []; + const pending: Array<(result: BrowserReviewResult) => void> = []; + const reader: BrowserReviewSourceReader = { + readResource(descriptor) { + requests.push(descriptor.id); + return new Promise>((resolve) => { + pending.push(resolve); + }); + }, + }; + return { + reader, + requests, + /** Answer the oldest read still waiting. */ + resolve(result: BrowserReviewResult) { + pending.shift()?.(result); + }, + }; +} + +/** Let every already-resolved promise settle. */ +async function settle() { + for (let turn = 0; turn < 8; turn += 1) { + await Promise.resolve(); + } +} + +describe("BrowserReviewSourceStore", () => { + test("keeps the text one read returned, for the file it was read for", async () => { + const file = testFile(); + const { reader, resolve } = createTestReader(); + const store = new BrowserReviewSourceStore(reader); + store.setGeneration("generation:p1:0"); + + store.request(file); + expect(store.getSnapshot().entries[file.key]?.status).toBe("loading"); + resolve({ ok: true, value: new TextEncoder().encode(BASE) }); + await settle(); + + expect(store.getSnapshot().entries[file.key]).toEqual({ status: "ready", text: BASE }); + }); + + test("drops a read that lands after the review moved to another generation", async () => { + const file = testFile(); + const { reader, resolve } = createTestReader(); + const store = new BrowserReviewSourceStore(reader); + store.setGeneration("generation:p1:0"); + store.request(file); + + // The review reloaded while the read was in flight: the same key over new content is + // different text, and drawing it in the new generation's gaps would be wrong lines. + store.setGeneration("generation:p1:1"); + resolve({ ok: true, value: new TextEncoder().encode(BASE) }); + await settle(); + + expect(store.getSnapshot().entries[file.key]).toBeUndefined(); + }); + + test("says which generation the text it holds was read for", async () => { + const file = testFile(); + const { reader, resolve } = createTestReader(); + const store = new BrowserReviewSourceStore(reader); + store.setGeneration("generation:p1:0"); + store.request(file); + resolve({ ok: true, value: new TextEncoder().encode(BASE) }); + await settle(); + + expect(store.getSnapshot().generation).toBe("generation:p1:0"); + + // A page renders the new generation's document before any effect can clear this store, + // so what it holds has to say which review it belongs to. + store.setGeneration("generation:p1:1"); + + expect(store.getSnapshot()).toEqual({ generation: "generation:p1:1", entries: {} }); + }); + + test("reads a file's source once, however many gaps in it are opened", async () => { + const file = testFile(); + const { reader, requests, resolve } = createTestReader(); + const store = new BrowserReviewSourceStore(reader); + store.setGeneration("generation:p1:0"); + + store.request(file); + // Asked again while the first read is still in flight, then again once it is held. + store.request(file); + resolve({ ok: true, value: new TextEncoder().encode(BASE) }); + await settle(); + store.request(file); + await settle(); + + expect(requests).toHaveLength(1); + }); + + test("remembers why a read was refused, in the catalog's words", async () => { + const file = testFile(); + const { reader, resolve } = createTestReader(); + const store = new BrowserReviewSourceStore(reader); + store.setGeneration("generation:p1:0"); + + store.request(file); + resolve(reviewHttpFailure("resource-unavailable")); + await settle(); + + expect(store.getSnapshot().entries[file.key]).toMatchObject({ + status: "failed", + failure: { + code: "resource-unavailable", + message: reviewErrorMessage("resource-unavailable"), + }, + }); + }); + + test("reads again after a failure, since asking again is the reader's retry", async () => { + const file = testFile(); + const { reader, requests, resolve } = createTestReader(); + const store = new BrowserReviewSourceStore(reader); + store.setGeneration("generation:p1:0"); + + store.request(file); + resolve(reviewHttpFailure("resource-unavailable")); + await settle(); + store.request(file); + resolve({ ok: true, value: new TextEncoder().encode(BASE) }); + await settle(); + + expect(requests).toHaveLength(2); + expect(store.getSnapshot().entries[file.key]).toEqual({ status: "ready", text: BASE }); + }); + + test("tells watchers when a file's source state changes", async () => { + const file = testFile(); + const { reader, resolve } = createTestReader(); + const store = new BrowserReviewSourceStore(reader); + store.setGeneration("generation:p1:0"); + let notices = 0; + store.subscribe(() => { + notices += 1; + }); + + store.request(file); + resolve({ ok: true, value: new TextEncoder().encode(BASE) }); + await settle(); + + expect(notices).toBe(2); + }); +}); diff --git a/src/web/browserReviewSources.ts b/src/web/browserReviewSources.ts new file mode 100644 index 000000000..b4eaf1e3c --- /dev/null +++ b/src/web/browserReviewSources.ts @@ -0,0 +1,141 @@ +/** + * Holds the full source text behind each file, for the collapsed regions a reader opens. + * + * Expanding a gap needs the whole file, not the patch, so the page reads one `source` + * resource per file and keeps it. Three facts shape what that means: + * + * - **A read belongs to the generation that asked for it.** The same file key over new + * content is different text, so a read that lands after a reload is dropped rather than + * stored — otherwise a gap in the new generation draws the old file's lines. + * - **One read per file.** Source text cannot change within a generation, and every read + * re-assembles and re-hashes megabytes on the main thread, so opening a second gap in a + * file the page already read asks for nothing. + * - **A failure is a state, not a silence.** A read that was refused is remembered with the + * catalog's own wording so the gap can say what happened, instead of claiming forever + * that it is still loading. + * + * Presentation state, so it lives with this client: a read-only mirror expands gaps for + * itself, and when expansion becomes a shared intent the answer arrives with the review + * instead. + */ +import { reviewExpansionSide } from "../core/review/expansion"; +import { reviewResourceId } from "../core/review/resources"; +import type { ReviewFileV1 } from "../core/review/types"; +import type { BrowserReviewApiClient, BrowserReviewFailure } from "./browserReviewApiClient"; + +/** What the page knows about one file's source text. */ +export interface BrowserReviewSourceEntry { + status: "loading" | "ready" | "failed"; + /** The file's whole source text, once it has been read. */ + text?: string; + /** Why the read was refused, in the shared vocabulary and wording. */ + failure?: BrowserReviewFailure; +} + +/** Every file's source state, keyed the way the review addresses files. */ +export type BrowserReviewSourceEntries = Readonly>; + +/** What the store holds, and the generation every entry in it was read for. */ +export interface BrowserReviewSourceSnapshot { + /** Absent until the page knows which generation it is showing. */ + generation: string | undefined; + entries: BrowserReviewSourceEntries; +} + +/** What this store needs from a transport: one resource read. */ +export type BrowserReviewSourceReader = Pick; + +export class BrowserReviewSourceStore { + private snapshot: BrowserReviewSourceSnapshot = { generation: undefined, entries: {} }; + private readonly listeners = new Set<() => void>(); + + constructor(private readonly client: BrowserReviewSourceReader) {} + + /** + * The current source state, safe to render directly. + * + * The generation rides with the entries so a caller can tell at render time whether they + * belong to the review it is drawing — a document arrives one render before any effect + * clearing this store could run, and stale text under a live file key is wrong lines + * rather than a missing one. + */ + getSnapshot(): BrowserReviewSourceSnapshot { + return this.snapshot; + } + + /** Watch the store. The listener is not called for the state it already sees. */ + subscribe(listener: () => void) { + this.listeners.add(listener); + return () => { + this.listeners.delete(listener); + }; + } + + /** + * Point the store at one generation, forgetting what was read for another. + * + * Reads already in flight are not cancelled — there is nothing to unwind — but what they + * return is dropped, because they were asked of a review that has since moved on. + */ + setGeneration(generation: string | undefined) { + if (this.snapshot.generation === generation) { + return; + } + this.snapshot = { generation, entries: {} }; + this.notify(); + } + + /** + * Read one file's source, unless it is already read or already being read. + * + * A failed entry is read again, which is the retry policy in full: the reader opening the + * gap again is the only retry gesture a read-only page has, and nothing retries on its own. + */ + request(file: ReviewFileV1) { + const { generation, entries } = this.snapshot; + if (!generation || file.sourceIdentity === undefined) { + return; + } + if (entries[file.key]?.status === "loading" || entries[file.key]?.status === "ready") { + return; + } + this.put(file.key, { status: "loading" }); + void this.client + .readResource({ + id: reviewResourceId({ + kind: "source", + fileKey: file.key, + side: reviewExpansionSide(file.changeKind), + }), + generation, + kind: "source", + }) + .then((result) => { + if (this.snapshot.generation !== generation) { + return; + } + this.put( + file.key, + result.ok + ? { status: "ready", text: new TextDecoder().decode(result.value) } + : { status: "failed", failure: result }, + ); + }); + } + + /** Record one file's state and tell everyone watching. */ + private put(fileKey: string, entry: BrowserReviewSourceEntry) { + this.snapshot = { + ...this.snapshot, + entries: { ...this.snapshot.entries, [fileKey]: entry }, + }; + this.notify(); + } + + private notify() { + // A copy, so a listener that unsubscribes while being told does not skip the next one. + for (const listener of Array.from(this.listeners)) { + listener(); + } + } +} diff --git a/src/web/browserViewOptions.test.ts b/src/web/browserViewOptions.test.ts new file mode 100644 index 000000000..b5fb724aa --- /dev/null +++ b/src/web/browserViewOptions.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, test } from "bun:test"; +import { + BROWSER_SPLIT_LAYOUT_MIN_WIDTH, + DEFAULT_BROWSER_VIEW_OPTIONS, + resolveBrowserDiffStyle, + resolveBrowserViewOptions, +} from "./browserViewOptions"; + +describe("resolveBrowserViewOptions", () => { + test("starts from the built-in defaults when the host supplied none", () => { + expect(resolveBrowserViewOptions()).toEqual(DEFAULT_BROWSER_VIEW_OPTIONS); + }); + + test("adopts the host's resolved defaults for the options that are this client's", () => { + expect( + resolveBrowserViewOptions({ mode: "stack", wrapLines: true, showLineNumbers: false }), + ).toEqual({ + layout: "stack", + wrapLines: true, + showLineNumbers: false, + showHunkHeaders: true, + }); + }); + + test("lets this client override what the host suggested", () => { + expect(resolveBrowserViewOptions({ mode: "stack" }, { layout: "split" }).layout).toBe("split"); + }); + + test("keeps review-wide options out of the client's view options entirely", () => { + // G1: `showAgentNotes` and the filter are the review's, and a per-client copy of one + // is how two surfaces come to show different reviews. + const resolved = resolveBrowserViewOptions({ showAgentNotes: true, filter: "src/" }); + + expect(Object.keys(resolved).sort()).toEqual([ + "layout", + "showHunkHeaders", + "showLineNumbers", + "wrapLines", + ]); + }); +}); + +describe("resolveBrowserDiffStyle", () => { + test("chooses by width only when the layout is auto", () => { + expect(resolveBrowserDiffStyle("auto", BROWSER_SPLIT_LAYOUT_MIN_WIDTH)).toBe("split"); + expect(resolveBrowserDiffStyle("auto", BROWSER_SPLIT_LAYOUT_MIN_WIDTH - 1)).toBe("unified"); + }); + + test("an explicit choice overrides the responsive one", () => { + expect(resolveBrowserDiffStyle("split", 320)).toBe("split"); + expect(resolveBrowserDiffStyle("stack", 4_000)).toBe("unified"); + }); +}); diff --git a/src/web/browserViewOptions.ts b/src/web/browserViewOptions.ts new file mode 100644 index 000000000..5a56bce69 --- /dev/null +++ b/src/web/browserViewOptions.ts @@ -0,0 +1,110 @@ +/** + * The view options one browser client draws with, and where each one came from. + * + * The terminal resolves view options through a layered chain — built-ins, user config, repo + * config, command sections, CLI flags — and the prototype browser had none of that: it + * hardcoded a layout and a theme and persisted nothing (`docs/browser-review-seam-audit.md`, + * G1). What this module fixes is the half a read-only client can fix: the host's resolved + * defaults are the starting point when it supplies them, and only the options the shared + * classification calls per-client are resolved here at all. + * + * That filter is the point. `showAgentNotes` and the filter are the review's, shared by + * every attached surface, and a client that quietly kept its own copy of one would be + * showing a different review from the terminal beside it. Asking + * `REVIEW_VIEW_OPTION_LOCUS` rather than assuming is what keeps that honest. + * + * Theme is deliberately absent: whether the browser mirrors the terminal's theme is an open + * product decision (E2), so this client renders in Pierre's own palette and adopts nothing + * from `src/ui/themes` by default. + */ +import { + isClientReviewViewOption, + REVIEW_VIEW_OPTION_LOCUS, + type LayoutMode, +} from "../core/review/viewOptions"; + +export interface BrowserViewOptions { + layout: LayoutMode; + showLineNumbers: boolean; + wrapLines: boolean; + showHunkHeaders: boolean; +} + +/** + * What a browser draws with before anyone says otherwise. + * + * Chosen to match the terminal's own built-in defaults, so opening the same review in two + * places does not look like two different products. + */ +export const DEFAULT_BROWSER_VIEW_OPTIONS: BrowserViewOptions = { + layout: "auto", + showLineNumbers: true, + wrapLines: false, + showHunkHeaders: true, +}; + +/** The host's resolved defaults, as far as they are this client's to adopt. */ +export interface BrowserHostViewDefaults { + mode?: LayoutMode; + showLineNumbers?: boolean; + wrapLines?: boolean; + showHunkHeaders?: boolean; + /** Shared review state; accepted so a caller may pass the host's whole record. */ + showAgentNotes?: boolean; + filter?: string; +} + +/** Which host default fills which client option, named once for both directions. */ +const CLIENT_OPTION_SOURCES = { + layout: "mode", + showLineNumbers: "showLineNumbers", + wrapLines: "wrapLines", + showHunkHeaders: "showHunkHeaders", +} as const satisfies Record; + +/** + * Resolve what this client draws with: built-in defaults, then the host's, then its own. + * + * A host default for an option the classification calls part of the review is ignored + * rather than adopted locally — not because it is wrong, but because a per-client copy of + * shared state is how two surfaces come to disagree about one review. + */ +export function resolveBrowserViewOptions( + hostDefaults: BrowserHostViewDefaults = {}, + overrides: Partial = {}, +): BrowserViewOptions { + const resolved = { ...DEFAULT_BROWSER_VIEW_OPTIONS }; + for (const [option, source] of Object.entries(CLIENT_OPTION_SOURCES) as Array< + [keyof BrowserViewOptions, keyof typeof REVIEW_VIEW_OPTION_LOCUS] + >) { + if (!isClientReviewViewOption(source)) { + continue; + } + const hostValue = hostDefaults[source as keyof BrowserHostViewDefaults]; + const override = overrides[option]; + const value = override ?? hostValue ?? resolved[option]; + Object.assign(resolved, { [option]: value }); + } + return resolved; +} + +/** + * Which layout a viewport of this width draws. + * + * `auto` is responsive exactly as the terminal's is — wide enough for two columns means + * two columns — and an explicit choice overrides it. + */ +export const BROWSER_SPLIT_LAYOUT_MIN_WIDTH = 1_000; + +export function resolveBrowserDiffStyle( + layout: LayoutMode, + viewportWidth: number, +): "split" | "unified" { + if (layout === "split") { + return "split"; + } + if (layout === "stack") { + return "unified"; + } + return viewportWidth >= BROWSER_SPLIT_LAYOUT_MIN_WIDTH ? "split" : "unified"; +} diff --git a/src/web/main.tsx b/src/web/main.tsx new file mode 100644 index 000000000..8288a6a5b --- /dev/null +++ b/src/web/main.tsx @@ -0,0 +1,61 @@ +/** @jsxImportSource react */ +/** + * The browser review page's entry point. + * + * Three things happen here and nowhere else: the review URL is read, the client and mirror + * are built from it, and the page is mounted. Everything after that is the mirror's. + * + * The language registration import is a side effect, and a deliberate one: Hunk registers + * file extensions Pierre's own inference does not know (`.mts`, `.cts`), and the prototype + * browser never imported the module that does it — so the same file highlighted one way in + * a terminal and another in a browser (`docs/browser-review-seam-audit.md`, A11). Importing + * it before anything renders is what makes the two agree. + */ +import { createRoot } from "react-dom/client"; +import "../core/fileLanguage"; +import { reviewErrorMessage } from "../session/reviewErrorCatalog"; +import { BrowserReviewApiClient, parseBrowserReviewLocation } from "./browserReviewApiClient"; +import { BrowserReviewMirror } from "./browserReviewMirror"; +import { BrowserReviewApp } from "./BrowserReviewApp"; +import type { BrowserHostViewDefaults } from "./browserViewOptions"; + +/** + * The host's resolved view defaults, when the page was served with them. + * + * Delivered with the document rather than over the wire: they are a fact about the session + * that served the page, not about the review it publishes, and a publication that carried + * them would make one client's window size look like review state (G1). + */ +declare global { + interface Window { + __hunkReviewViewDefaults?: BrowserHostViewDefaults; + } +} + +/** Mount the review page, or say why there is nothing to mount. */ +export function mountBrowserReviewPage(container: HTMLElement) { + const location = parseBrowserReviewLocation(new URL(window.location.href)); + if (!location) { + // A link this client cannot read is one no session would honor, so it is the + // catalog's `unauthorized` wording rather than a sentence written here (G4). + container.textContent = reviewErrorMessage("unauthorized"); + return; + } + + const client = new BrowserReviewApiClient(location); + const mirror = new BrowserReviewMirror(client); + createRoot(container).render( + , + ); +} + +const container = document.getElementById("hunk-review"); +if (container) { + mountBrowserReviewPage(container); +} diff --git a/test/review-conformance/conformance.test.ts b/test/review-conformance/conformance.test.ts index 5114e64d1..ee75d5a2e 100644 --- a/test/review-conformance/conformance.test.ts +++ b/test/review-conformance/conformance.test.ts @@ -54,6 +54,7 @@ describe("review conformance corpus", () => { "core review model", "terminal render planning", "review producer", + "browser review projection", ]); expect(REVIEW_NAVIGATION_CONSUMERS.map((consumer) => consumer.name)).toEqual([ "core intent planner", @@ -62,13 +63,15 @@ describe("review conformance corpus", () => { expect(REVIEW_ORDERING_CONSUMERS.map((consumer) => consumer.name)).toEqual([ "core publication ordering", "broker review mirror", + "browser review mirror", ]); expect(REVIEW_WIRE_CONSUMERS.map((consumer) => consumer.name)).toEqual([ "review wire protocol", ]); expect(REVIEW_EVENT_CONSUMERS.map((consumer) => consumer.name)).toEqual([ "review event protocol", - "browser review HTTP surface", + "web review HTTP surface", + "browser review client reader", ]); }); diff --git a/test/review-conformance/consumers.ts b/test/review-conformance/consumers.ts index 41a307dd7..c5b630bd0 100644 --- a/test/review-conformance/consumers.ts +++ b/test/review-conformance/consumers.ts @@ -7,7 +7,10 @@ * own has joined (`docs/browser-review-rebuild.md` § "Per-phase seam verification"). */ import { brokerMirrorOrderingConsumer } from "./consumers/brokerMirror"; -import { browserReviewSurfaceEventConsumer } from "./consumers/browserReviewSurface"; +import { browserMirrorOrderingConsumer } from "./consumers/browserReviewMirror"; +import { browserReviewProjectionConsumer } from "./consumers/browserReviewProjection"; +import { browserReviewReaderEventConsumer } from "./consumers/browserReviewReader"; +import { webReviewSurfaceEventConsumer } from "./consumers/webReviewSurface"; import { reviewEventProtocolConsumer } from "./consumers/reviewEventProtocol"; import { coreModelConsumer } from "./consumers/coreModel"; import { coreOrderingConsumer } from "./consumers/coreOrdering"; @@ -28,6 +31,7 @@ export const REVIEW_GEOMETRY_CONSUMERS: readonly ReviewGeometryConsumer[] = [ coreModelConsumer, terminalRenderPlanConsumer, reviewProducerConsumer, + browserReviewProjectionConsumer, ]; /** @@ -46,12 +50,13 @@ export const REVIEW_NAVIGATION_CONSUMERS: readonly ReviewNavigationConsumer[] = * Consumers of the publication-ordering contract. * * The contract itself answers first, and every tier that orders publications joins beside - * it: the broker's mirror here, a browser client's in Phase 5. A tier with a rule of its - * own disagrees with the reference on the fixtures the C1 finding contributed. + * it: the broker's mirror and the browser client's. A tier with a rule of its own + * disagrees with the reference on the fixtures the C1 finding contributed. */ export const REVIEW_ORDERING_CONSUMERS: readonly ReviewOrderingConsumer[] = [ coreOrderingConsumer, brokerMirrorOrderingConsumer, + browserMirrorOrderingConsumer, ]; /** @@ -66,11 +71,12 @@ export const REVIEW_WIRE_CONSUMERS: readonly ReviewWireConsumer[] = [reviewWireC /** * Consumers of the event contract. * - * The shared protocol answers first and the HTTP surface answers beside it, over a real - * listener — which is what proves the surface has no framing of its own. A browser - * client's reader joins in Phase 5, closing the loop the C4 finding is about. + * The shared protocol answers first, the HTTP surface answers beside it over a real + * listener, and the browser client reads the same stream back — the sender and the reader + * on one corpus, which is the loop the C4 finding is about. */ export const REVIEW_EVENT_CONSUMERS: readonly ReviewEventConsumer[] = [ reviewEventProtocolConsumer, - browserReviewSurfaceEventConsumer, + webReviewSurfaceEventConsumer, + browserReviewReaderEventConsumer, ]; diff --git a/test/review-conformance/consumers/browserReviewMirror.ts b/test/review-conformance/consumers/browserReviewMirror.ts new file mode 100644 index 000000000..fa9cc0652 --- /dev/null +++ b/test/review-conformance/consumers/browserReviewMirror.ts @@ -0,0 +1,97 @@ +/** + * The browser client's mirror as an ordering consumer. + * + * The mirror decides what to do with an arriving publication, and the only way it is + * allowed to decide is `classifyReviewPublication`. This adapter drives the real mirror — + * a real event handler delivering real publication bodies — and reads the verdict back out + * of what the mirror did with them, so a client that had grown a comparison of its own + * (the prototype's contiguous `+1` revision rule) disagrees with the reference consumer + * here (`docs/browser-review-seam-audit.md`, C1). + * + * The verdict is inferred from behavior rather than asked for: `gap` is the only case that + * reads the catalog again, `accepted` moves the position without reading, and `stale` + * changes nothing. That is exactly the difference a client's users would see. + */ +import type { + ReviewPublicationAddress, + ReviewPublicationOrder, +} from "../../../src/core/review/generationOrder"; +import { + REVIEW_CANONICAL_FILE_CONTENT_TYPE, + reviewResourceId, +} from "../../../src/core/review/resources"; +import { HUNK_REVIEW_PROTOCOL_VERSION } from "../../../src/session/reviewProtocol"; +import type { HunkReviewPublicationBodyV1 } from "../../../src/session/reviewHttpProtocol"; +import type { BrowserReviewEventHandlers } from "../../../src/web/browserReviewApiClient"; +import { + BrowserReviewMirror, + type BrowserReviewMirrorSource, +} from "../../../src/web/browserReviewMirror"; +import type { ReviewOrderingConsumer } from "../types"; + +const SESSION_ID = "session-conformance"; +const FILE_KEY = "file:00000001"; + +/** One publication body for a position, with a catalog naming a single readable file. */ +function publicationFor(address: ReviewPublicationAddress): HunkReviewPublicationBodyV1 { + return { + protocolVersion: HUNK_REVIEW_PROTOCOL_VERSION, + sessionId: SESSION_ID, + publication: address, + catalog: { + generation: address.generation, + fileKeysByRuntimeId: { "file-1": FILE_KEY }, + resources: [ + { + id: reviewResourceId({ kind: "canonical-file", fileKey: FILE_KEY }), + generation: address.generation, + fileKey: FILE_KEY, + kind: "canonical-file", + contentType: REVIEW_CANONICAL_FILE_CONTENT_TYPE, + }, + ], + }, + }; +} + +export const browserMirrorOrderingConsumer: ReviewOrderingConsumer = { + name: "browser review mirror", + phase: "Phase 5 PR 1", + classify(current: ReviewPublicationAddress, incoming: ReviewPublicationAddress) { + let handlers: BrowserReviewEventHandlers | undefined; + const readGenerations: string[] = []; + const source: BrowserReviewMirrorSource = { + async readResource(descriptor) { + readGenerations.push(descriptor.generation); + // The bytes are irrelevant to ordering; refusing them keeps the adapter from + // needing a document while still recording that a read was attempted. + return { ok: false, code: "unknown-resource", message: "not part of this corpus" }; + }, + streamEvents(next) { + handlers = next; + return new Promise(() => undefined); + }, + }; + const mirror = new BrowserReviewMirror(source, { + timers: { setTimeout: () => 1, clearTimeout: () => undefined }, + }); + + mirror.start(); + handlers!.onPublication(publicationFor(current)); + const readsBefore = readGenerations.length; + // Delivered without settling the first read, deliberately: the mirror reads a + // generation again when it holds no document for it and none is on the way, which is + // how a failed load is retried. Letting the refusal above land first would make that + // retry look like a resync, so the second publication arrives while the first read is + // still in flight and only a real `gap` reads anything. + handlers!.onPublication(publicationFor(incoming)); + mirror.stop(); + + const reread = readGenerations.length > readsBefore; + const moved = mirror.getSnapshot().publication?.stateRevision !== current.stateRevision; + if (reread) { + return "gap" satisfies ReviewPublicationOrder; + } + return moved ? "accepted" : "stale"; + }, +}; diff --git a/test/review-conformance/consumers/browserReviewProjection.ts b/test/review-conformance/consumers/browserReviewProjection.ts new file mode 100644 index 000000000..f50b80c11 --- /dev/null +++ b/test/review-conformance/consumers/browserReviewProjection.ts @@ -0,0 +1,62 @@ +/** + * The browser client's projection as a conformance consumer. + * + * This is the loop the Phase 5 gate is about: the same fixtures the terminal's row builder + * answers, answered by what the browser actually draws from. Every value below is read out + * of the render model `pierreDocument` produces — the gaps the stream draws strips for, the + * per-hunk extents it addresses, the note target it hangs a note from, the rows an expanded + * gap reveals — rather than by calling core a second time. A browser that re-derived any of + * them would disagree with the terminal here rather than on a reviewer's screen + * (`docs/browser-review-seam-audit.md`, A1–A10). + * + * The fixtures build parsed diff files, which is the terminal's input; the browser's input + * is the projected document those files publish, so the adapter projects first and then + * asks the browser's own code the questions. + */ +import { projectReviewDocument } from "../../../src/core/review/document"; +import { + buildBrowserReviewFileRenderModel, + browserReviewExpandedGapRows, +} from "../../../src/web/browserPierreDocument"; +import type { ConformanceGap, ReviewGeometryConsumer, ReviewGeometryFixture } from "../types"; + +/** Read the gaps the stream would draw, in the order it would draw them. */ +function gapsOf(model: ReturnType): ConformanceGap[] { + return model.gaps.map((gap) => ({ + gapId: gap.gapId, + oldRange: [...gap.oldRange] as [number, number], + newRange: [...gap.newRange] as [number, number], + lineCount: gap.lineCount, + })); +} + +export const browserReviewProjectionConsumer: ReviewGeometryConsumer = { + name: "browser review projection", + phase: "Phase 5 PR 1", + project(fixture: ReviewGeometryFixture) { + const document = projectReviewDocument(fixture.build()); + return { + files: document.files.map((file, fileIndex) => { + const model = buildBrowserReviewFileRenderModel(file); + const expansion = + fixture.expansion?.fileIndex === fileIndex ? fixture.expansion : undefined; + return { + path: model.path, + gaps: gapsOf(model), + hunkRanges: model.hunks.map((hunk) => ({ + oldRange: [...hunk.oldRange] as [number, number], + newRange: [...hunk.newRange] as [number, number], + })), + defaultNoteTargets: model.hunks.map((hunk) => hunk.noteTarget), + ...(model.emptyDiffReason ? { emptyDiffReason: model.emptyDiffReason } : {}), + ...(expansion + ? { + expandedRows: + browserReviewExpandedGapRows(file, expansion.gapId, expansion.sourceText) ?? [], + } + : {}), + }; + }), + }; + }, +}; diff --git a/test/review-conformance/consumers/browserReviewReader.ts b/test/review-conformance/consumers/browserReviewReader.ts new file mode 100644 index 000000000..e37dca099 --- /dev/null +++ b/test/review-conformance/consumers/browserReviewReader.ts @@ -0,0 +1,139 @@ +/** + * The browser client's event reader as an event consumer. + * + * The surface answers this corpus by framing a publication; the client answers it by + * reading one back. Running both against the same fixtures is what closes C4: a client + * that had re-declared frame names, envelopes, or which frame is resumable would disagree + * with the protocol here rather than in a browser + * (`docs/browser-review-seam-audit.md`, C4). + * + * The client is the real one, reading a real stream from a real listener. What the adapter + * adds is an observer: the response body is duplicated on its way into the client, so the + * frames can be counted without the client being asked to report them — it has no reason + * to, and a reader that reported its own framing would be describing itself. + */ +import { SESSION_BROKER_REGISTRATION_VERSION } from "@hunk/session-broker-core"; +import { reviewProcessCapability } from "../../../src/app/review/capability"; +import { WebReviewServer } from "../../../src/session/broker/webReviewServer"; +import { HunkSessionBrokerState } from "../../../src/session/broker/state"; +import { ReviewEventSseDecoder } from "../../../src/session/reviewEventProtocol"; +import { BrowserReviewApiClient } from "../../../src/web/browserReviewApiClient"; +import { EVENT_FIXTURE_SESSION_ID } from "../eventFixtures"; +import { collapseChunkRun, resolveFixtureChunkBytes } from "../eventFraming"; +import type { ReviewEventConsumer, ReviewEventFixture } from "../types"; + +/** Register the fixture's publication with a real broker state, through its own parsers. */ +function mirrorFixture(state: HunkSessionBrokerState, fixture: ReviewEventFixture) { + state.registerSession( + { send: () => undefined }, + { + registrationVersion: SESSION_BROKER_REGISTRATION_VERSION, + sessionId: EVENT_FIXTURE_SESSION_ID, + pid: process.pid, + cwd: "/repo", + launchedAt: new Date().toISOString(), + info: { + inputKind: "vcs", + title: "conformance", + sourceLabel: "/repo", + files: [], + reviewCatalog: fixture.body.catalog, + reviewCapabilityDigest: reviewProcessCapability().digest, + }, + }, + { + updatedAt: new Date().toISOString(), + state: { + selectedHunkIndex: 0, + showAgentNotes: false, + liveCommentCount: 0, + liveComments: [], + reviewPublication: fixture.body.publication, + }, + }, + ); +} + +/** Read the observed stream text with the protocol's own record decoder. */ +function observedRecords(text: string) { + return new ReviewEventSseDecoder().push(text).map((record) => ({ + event: record.event, + // Only a record that completes an event carries an id, which is the protocol's rule + // rather than this adapter's reading of one. + resumable: record.id !== undefined, + })); +} + +export const browserReviewReaderEventConsumer: ReviewEventConsumer = { + name: "browser review client reader", + phase: "Phase 5 PR 1", + async frame(fixture: ReviewEventFixture) { + const serialized = JSON.stringify(fixture.body); + const state = new HunkSessionBrokerState(); + mirrorFixture(state, fixture); + const review = new WebReviewServer(state, { + eventChunkBytes: resolveFixtureChunkBytes( + fixture, + new TextEncoder().encode(serialized).byteLength, + ), + }); + const server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + idleTimeout: 0, + fetch: async (request) => + (await review.handle(request)) ?? new Response(null, { status: 404 }), + }); + + let observed = ""; + const client = new BrowserReviewApiClient({ + origin: `http://127.0.0.1:${server.port}`, + sessionId: EVENT_FIXTURE_SESSION_ID, + capability: reviewProcessCapability().token, + fetch: (async (input: string, init: RequestInit) => { + const response = await fetch(input, init); + if (!response.body) { + return response; + } + // Tee rather than intercept: the client reads one branch and the adapter the + // other, so what is counted is exactly what the client parsed. + const [toClient, toObserver] = response.body.tee(); + void (async () => { + const decoder = new TextDecoder(); + for await (const chunk of toObserver as unknown as AsyncIterable) { + observed += decoder.decode(chunk, { stream: true }); + } + })().catch(() => { + // The observer branch ends when the client aborts the stream, which is how a + // reader that has what it came for stops reading. + }); + return new Response(toClient, { headers: response.headers, status: response.status }); + }) as unknown as typeof globalThis.fetch, + }); + + try { + const abort = new AbortController(); + let delivered: unknown; + await client.streamEvents( + { + onPublication: (body) => { + delivered = body; + // One complete event is the whole question; the stream stays open otherwise. + abort.abort(); + }, + }, + abort.signal, + ); + const records = observedRecords(observed); + return { + frames: collapseChunkRun(records.map((record) => record.event)), + resumableFrames: records.filter((record) => record.resumable).length, + roundTrips: JSON.stringify(delivered) === serialized, + }; + } finally { + review.close(); + server.stop(true); + state.shutdown(); + } + }, +}; diff --git a/test/review-conformance/consumers/coreModel.ts b/test/review-conformance/consumers/coreModel.ts index 600ccfe02..8d8ce9b3d 100644 --- a/test/review-conformance/consumers/coreModel.ts +++ b/test/review-conformance/consumers/coreModel.ts @@ -8,6 +8,7 @@ */ import { projectReviewDocument, reviewEmptyDiffReason } from "../../../src/core/review/document"; import { + reviewExpandedGapLines, reviewExpansionSide, reviewGapAddress, reviewGapId, @@ -68,13 +69,11 @@ function expandedRowsOf( if (!address) { return undefined; } - const side = reviewExpansionSide(file.changeKind); const sourceLines = normalizedReviewSourceLines(expansion.sourceText); - const range = side === "old" ? address.oldRange : address.newRange; - return Array.from({ length: address.lineCount }, (_unused, offset) => ({ - oldLine: address.oldRange[0] + offset, - newLine: address.newRange[0] + offset, - text: sourceLines[range[0] + offset - 1] ?? "", + return reviewExpandedGapLines(address, reviewExpansionSide(file.changeKind)).map((line) => ({ + oldLine: line.oldLine, + newLine: line.newLine, + text: sourceLines[line.sourceLine - 1] ?? "", })); } diff --git a/test/review-conformance/consumers/browserReviewSurface.ts b/test/review-conformance/consumers/webReviewSurface.ts similarity index 81% rename from test/review-conformance/consumers/browserReviewSurface.ts rename to test/review-conformance/consumers/webReviewSurface.ts index 4ee6c8ce8..460b75e39 100644 --- a/test/review-conformance/consumers/browserReviewSurface.ts +++ b/test/review-conformance/consumers/webReviewSurface.ts @@ -12,7 +12,7 @@ import { SESSION_BROKER_REGISTRATION_VERSION } from "@hunk/session-broker-core"; import { reviewProcessCapability } from "../../../src/app/review/capability"; import { nodeReviewDigest } from "../../../src/lib/reviewDigest"; -import { BrowserReviewServer } from "../../../src/session/broker/browserReviewServer"; +import { WebReviewServer } from "../../../src/session/broker/webReviewServer"; import { HunkSessionBrokerState } from "../../../src/session/broker/state"; import { parseReviewEventBegin, @@ -20,6 +20,7 @@ import { parseReviewEventEnd, parseReviewEventFrame, ReviewEventAssembler, + ReviewEventSseDecoder, } from "../../../src/session/reviewEventProtocol"; import { HUNK_REVIEW_CAPABILITY_HEADER, @@ -77,30 +78,24 @@ function mirrorFixture(state: HunkSessionBrokerState, fixture: ReviewEventFixtur /** Read SSE records until one complete event has arrived, then stop. */ async function readOneEvent(response: Response) { const reader = response.body!.getReader(); - const decoder = new TextDecoder(); + const textDecoder = new TextDecoder(); + const records = new ReviewEventSseDecoder(); const frames: DecodedFrame[] = []; - let buffer = ""; let complete = false; while (!complete) { const { value, done } = await reader.read(); if (done) { break; } - buffer += decoder.decode(value, { stream: true }); - let boundary = buffer.indexOf("\n\n"); - while (boundary >= 0) { - const lines = buffer.slice(0, boundary).split("\n"); - buffer = buffer.slice(boundary + 2); - const id = lines.find((line) => line.startsWith("id: "))?.slice(4); - const event = lines.find((line) => line.startsWith("event: "))?.slice(7); - const data = lines.find((line) => line.startsWith("data: "))?.slice(6); - if (event && data !== undefined) { - frames.push({ ...(id ? { id } : {}), event, data: JSON.parse(data) as unknown }); - // Only a frame that completes an event carries an id, which is how a reader knows - // where an event ends without knowing whether it was chunked. - complete ||= id !== undefined; - } - boundary = buffer.indexOf("\n\n"); + for (const record of records.push(textDecoder.decode(value, { stream: true }))) { + frames.push({ + ...(record.id === undefined ? {} : { id: record.id }), + event: record.event, + data: JSON.parse(record.data) as unknown, + }); + // Only a frame that completes an event carries an id, which is how a reader knows + // where an event ends without knowing whether it was chunked. + complete ||= record.id !== undefined; } } await reader.cancel(); @@ -128,14 +123,14 @@ function roundTripsTo(frames: DecodedFrame[], expected: string) { return assembled.ok && new TextDecoder().decode(assembled.bytes) === expected; } -export const browserReviewSurfaceEventConsumer: ReviewEventConsumer = { - name: "browser review HTTP surface", +export const webReviewSurfaceEventConsumer: ReviewEventConsumer = { + name: "web review HTTP surface", phase: "Phase 4", async frame(fixture: ReviewEventFixture) { const serialized = JSON.stringify(fixture.body); const state = new HunkSessionBrokerState(); mirrorFixture(state, fixture); - const review = new BrowserReviewServer(state, { + const review = new WebReviewServer(state, { eventChunkBytes: resolveFixtureChunkBytes( fixture, new TextEncoder().encode(serialized).byteLength, diff --git a/test/session/browserReviewClient.integration.test.ts b/test/session/browserReviewClient.integration.test.ts new file mode 100644 index 000000000..12824797b --- /dev/null +++ b/test/session/browserReviewClient.integration.test.ts @@ -0,0 +1,231 @@ +/** + * The browser client against a real review, with only the browser missing. + * + * A real producer publishes a real generation, a real broker state mirrors it, the review + * surface is mounted on a real loopback listener, and the client under test is the one the + * browser bundle runs — same `BrowserReviewApiClient`, same `BrowserReviewMirror`, same synchronous + * digest. Nothing between the two ends is stubbed, so what is asserted here is the loop the + * whole phase is about: a publication read over HTTP, a document read out of the catalog it + * names, and a new publication arriving on the event stream. + */ +import { afterEach, describe, expect, test } from "bun:test"; +import { reviewProcessCapability } from "../../src/app/review/capability"; +import { reviewResourceId } from "../../src/core/review/resources"; +import { WebReviewServer } from "../../src/session/broker/webReviewServer"; +import { reviewHttpPath, reviewUrl } from "../../src/session/reviewHttpProtocol"; +import { + BrowserReviewApiClient, + parseBrowserReviewLocation, +} from "../../src/web/browserReviewApiClient"; +import { + BrowserReviewMirror, + type BrowserReviewMirrorSnapshot, +} from "../../src/web/browserReviewMirror"; +import { connectReviewSession, createTestPatchFile } from "../helpers/review-session-harness"; + +const SESSION_ID = "session-web-1"; + +const running: Array<{ review: WebReviewServer; server: { stop: (force?: boolean) => void } }> = []; + +afterEach(() => { + for (const entry of running.splice(0)) { + entry.review.close(); + entry.server.stop(true); + } +}); + +/** Connect a session, mount the review surface over it, and build the client for it. */ +function start(files = [createTestPatchFile("alpha", 4), createTestPatchFile("beta", 2)]) { + const harness = connectReviewSession(files, { sessionId: SESSION_ID }); + const registration = harness.register(); + // The registration the session first published, so a reload can update it in place the + // way a live session does. + const review = new WebReviewServer(harness.state); + const server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + idleTimeout: 0, + fetch: async (request) => (await review.handle(request)) ?? new Response(null, { status: 404 }), + }); + running.push({ review, server }); + + const origin = `http://127.0.0.1:${server.port}`; + // Built the way a browser gets it: parse the URL the session would print. + const location = parseBrowserReviewLocation( + new URL(reviewUrl(origin, SESSION_ID, reviewProcessCapability().token)), + )!; + return { harness, registration, origin, client: new BrowserReviewApiClient(location) }; +} + +/** Wait for the mirror to reach a snapshot, failing loudly rather than hanging forever. */ +async function waitFor( + mirror: BrowserReviewMirror, + describeWanted: string, + wanted: (snapshot: BrowserReviewMirrorSnapshot) => boolean, +) { + return await new Promise((resolve, reject) => { + const timer = setTimeout(() => { + unsubscribe(); + reject( + new Error( + `The mirror stayed at ${mirror.getSnapshot().status}, waiting for ${describeWanted}.`, + ), + ); + }, 5_000); + const settle = (snapshot: BrowserReviewMirrorSnapshot) => { + if (!wanted(snapshot)) { + return; + } + clearTimeout(timer); + unsubscribe(); + resolve(snapshot); + }; + const unsubscribe = mirror.subscribe(settle); + settle(mirror.getSnapshot()); + }); +} + +/** Wait for the mirror to hold a complete document. */ +function waitForReady(mirror: BrowserReviewMirror) { + return waitFor(mirror, "a complete document", (snapshot) => snapshot.status === "ready"); +} + +describe("browser review client", () => { + test("reads the publication the session is serving", async () => { + const { harness, client } = start(); + + const result = await client.readPublication(); + + expect(result.ok).toBe(true); + if (!result.ok) { + return; + } + expect(result.value.publication).toEqual(harness.producer.getPublicationAddress()); + expect(result.value.catalog.generation).toBe(harness.producer.getPublication().generation); + }); + + test("refuses a publication read without the capability", async () => { + const { origin } = start(); + const anonymous = new BrowserReviewApiClient({ + origin, + sessionId: SESSION_ID, + capability: "x", + }); + + const result = await anonymous.readPublication(); + + expect(result).toMatchObject({ ok: false, code: "unauthorized" }); + }); + + test("reads and verifies one resource larger than a single response", async () => { + // Well over the shared chunk size, so the read is several windows the assembler has to + // join — the case the prototype's own range loop got wrong. + const { harness, client } = start([createTestPatchFile("wide", 20_000)]); + const publication = harness.producer.getPublication(); + const descriptor = publication.resources.find((resource) => resource.kind === "patch")!; + + const result = await client.readResource(descriptor); + + expect(result.ok).toBe(true); + if (!result.ok) { + return; + } + expect(new TextDecoder().decode(result.value)).toBe(publication.document.files[0]!.patch); + expect(result.value.byteLength).toBeGreaterThan(256 * 1024); + }); + + test("reports a resource the generation does not offer", async () => { + const { harness, client } = start(); + + const result = await client.readResource({ + id: reviewResourceId({ kind: "patch", fileKey: "file:deadbeef" }), + generation: harness.producer.getPublication().generation, + kind: "patch", + }); + + expect(result).toMatchObject({ ok: false, code: "unknown-resource" }); + }); + + test("mirrors the whole document, in review order, from the stream's first event", async () => { + const { harness, client } = start(); + const mirror = new BrowserReviewMirror(client); + + mirror.start(); + const ready = await waitForReady(mirror); + mirror.stop(); + + const published = harness.producer.getPublication(); + expect(ready.publication).toEqual(harness.producer.getPublicationAddress()); + expect(ready.document?.files.map((file) => file.path)).toEqual( + published.document.files.map((file) => file.path), + ); + expect(ready.document?.files.map((file) => file.contentIdentity)).toEqual( + published.document.files.map((file) => file.contentIdentity), + ); + }); + + test("resyncs onto a new generation when the session reloads", async () => { + const { harness, client, registration } = start(); + const mirror = new BrowserReviewMirror(client); + + mirror.start(); + const first = await waitForReady(mirror); + + harness.producer.publish({ + files: [createTestPatchFile("gamma", 3)], + sourceLabel: harness.bootstrap.changeset.sourceLabel, + }); + harness.publishSnapshot(); + harness.register(registration); + + const second = await waitFor( + mirror, + "a document for the next generation", + (snapshot) => + snapshot.status === "ready" && + snapshot.publication?.generation !== first.publication?.generation, + ); + mirror.stop(); + + expect(second.publication?.generation).not.toBe(first.publication?.generation); + expect(second.document?.files.map((file) => file.path)).toEqual(["src/gamma.ts"]); + }); + + test("refuses a resource whose bytes were corrupted in transit", async () => { + const harness = connectReviewSession([createTestPatchFile("alpha", 4)], { + sessionId: SESSION_ID, + corruptResourceChunks: true, + }); + harness.register(); + const review = new WebReviewServer(harness.state); + const server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + idleTimeout: 0, + fetch: async (request) => + (await review.handle(request)) ?? new Response(null, { status: 404 }), + }); + running.push({ review, server }); + const client = new BrowserReviewApiClient({ + origin: `http://127.0.0.1:${server.port}`, + sessionId: SESSION_ID, + capability: reviewProcessCapability().token, + }); + + const result = await client.readResource( + harness.producer.getPublication().resources.find((resource) => resource.kind === "patch")!, + ); + + // The daemon's own assembler catches it first, which is the point: corruption is + // reported as corruption at whichever tier sees it, never as a missing resource. + expect(result).toMatchObject({ ok: false, code: "resource-integrity" }); + }); + + test("addresses every route through the shared path grammar", () => { + // The client never assembles a review path itself; this pins that the grammar it uses + // is the one the surface parses. + expect(reviewHttpPath({ kind: "publication", sessionId: SESSION_ID })).toBe( + `/review-api/${SESSION_ID}/publication`, + ); + }); +});