From 3cc4813da97ccc4f1dec169ab44cee0302d57a72 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 17:41:40 +0000 Subject: [PATCH] docs(review): trim the rebuild's module headers to their invariants The browser-review rebuild's new modules shared one rhetorical template: an aphoristic title, a paragraph of prototype autopsy carrying an audit finding id, and a "deliberately X rather than Y" closer. The same anecdote was retold in up to eight files, which buried the invariants a maintainer actually needs and left claims no reader can verify. Each header now keeps the rule and drops the postmortem. Audit findings are still cited where the surrounding sentence earns it, and each prototype telling stays only in the module that owns the rule it explains: generationOrder for C1, noteSize for D1, reviewCommands for D3. Also documents the review model's remaining undocumented public types. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4 --- .changeset/review-header-cleanup.md | 2 ++ src/app/review/producer.ts | 20 ++++++++-------- src/app/review/publication.ts | 8 +++---- src/app/review/resourceStore.ts | 2 +- src/core/commandCatalog.ts | 14 +++++------ src/core/review/actions.ts | 2 +- src/core/review/address.ts | 7 ++++++ src/core/review/anchors.ts | 12 ++++++---- src/core/review/annotations.ts | 6 ++--- src/core/review/canonicalFile.ts | 6 +---- src/core/review/contentManifest.ts | 10 ++++---- src/core/review/document.ts | 5 ++-- src/core/review/generationOrder.ts | 12 +++++----- src/core/review/geometry.ts | 6 ++--- src/core/review/identity.ts | 8 +++---- src/core/review/intents.ts | 23 ++++++++++++++----- src/core/review/navigation.ts | 8 +++---- src/core/review/noteSize.ts | 20 +++++----------- src/core/review/reducer.ts | 6 ++--- src/core/review/resourceAssembly.ts | 6 +++++ src/core/review/state.ts | 5 ++++ src/core/review/types.ts | 4 ++-- src/core/review/validation.ts | 12 +++++----- src/session/broker/reviewMirror.ts | 13 ++++------- src/session/reviewProtocol.ts | 5 ++-- .../consumers/brokerMirror.ts | 7 ++---- .../consumers/reviewProducer.ts | 3 +-- .../consumers/reviewWire.ts | 2 +- test/review-conformance/noteSize.ts | 10 ++++---- test/review-conformance/orderingFixtures.ts | 9 +++----- test/review-conformance/types.ts | 6 ++--- test/review-conformance/wireFixtures.ts | 8 +++---- 32 files changed, 135 insertions(+), 132 deletions(-) create mode 100644 .changeset/review-header-cleanup.md diff --git a/.changeset/review-header-cleanup.md b/.changeset/review-header-cleanup.md new file mode 100644 index 000000000..a845151cc --- /dev/null +++ b/.changeset/review-header-cleanup.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/src/app/review/producer.ts b/src/app/review/producer.ts index 5f3fb3094..88c65b220 100644 --- a/src/app/review/producer.ts +++ b/src/app/review/producer.ts @@ -8,16 +8,14 @@ * against the shared contract (`core/review/generationOrder.ts`) before anything is * published, so a producer bug fails here rather than desynchronizing a reader. * - * It deliberately does *not* own the review's live state. The terminal's controller owns - * the store today, and moving that is a behavior change rather than a seam extraction; the - * producer attaches to whichever store the host mounted and plans intents against it, - * supplying the caller-owned facts core refuses to invent — identity, time, and the - * annotation index (`ReviewIntentFacts.annotations`) — through the same derivation the - * terminal uses. + * It does *not* own the review's live state. The terminal's controller owns the store + * today, and moving that is a behavior change rather than a seam extraction; the producer + * attaches to whichever store the host mounted and plans intents against it, supplying the + * caller-owned facts core refuses to invent (identity, time, and the annotation index, + * `ReviewIntentFacts.annotations`) through the same derivation the terminal uses. * * No transport lives here. Serving the session surface means answering method calls; HTTP, - * SSE, and a browser client are later phases, and their absence is what keeps this module - * about the review rather than about a protocol. + * SSE, and a browser client are later phases. */ import { assertReviewPublicationAdvance, @@ -158,9 +156,9 @@ export class ReviewProducer { /** * The review state this producer plans against, when a host has attached one. * - * Read-only, and deliberately the *store's* state rather than a copy: a caller - * validating a request against the current review — does this file exist, is this the - * draft I opened — must see exactly what the next intent will be planned against. + * Read-only, and the *store's* state rather than a copy: a caller validating a request + * against the current review — does this file exist, is this the draft I opened — must + * see exactly what the next intent will be planned against. */ getReviewState() { return this.store?.getSnapshot(); diff --git a/src/app/review/publication.ts b/src/app/review/publication.ts index 292d93ba5..515d07d7a 100644 --- a/src/app/review/publication.ts +++ b/src/app/review/publication.ts @@ -7,10 +7,10 @@ * mutating this one — which is what makes "which generation is this?" answerable rather * than a matter of timing. * - * The document itself deliberately knows nothing about any of this - * (`src/core/review/document.ts`); publication is layered on top so the shared model stays - * a description of a review rather than of a transport. The vocabulary for addressing and - * ordering those generations is `src/core/review/generationOrder.ts`. + * The document itself knows nothing about any of this (`src/core/review/document.ts`); + * publication is layered on top so the shared model stays a description of a review rather + * than of a transport. The vocabulary for addressing and ordering those generations is + * `src/core/review/generationOrder.ts`. */ import { buildReviewContentManifest, diff --git a/src/app/review/resourceStore.ts b/src/app/review/resourceStore.ts index ee34e1f33..236d95861 100644 --- a/src/app/review/resourceStore.ts +++ b/src/app/review/resourceStore.ts @@ -1,7 +1,7 @@ /** * Materializing and serving one generation's resources. * - * Three rules shape this module, each of them a defect the prototype shipped: + * Three rules shape this module: * * - **Single flight per resource.** A resource is produced at most once per generation, * and concurrent readers share that one production. It is not a cache bolted on top: a diff --git a/src/core/commandCatalog.ts b/src/core/commandCatalog.ts index 91d0cc015..2eaa7ffcc 100644 --- a/src/core/commandCatalog.ts +++ b/src/core/commandCatalog.ts @@ -13,17 +13,17 @@ * attached surface sees the result. Its effect is declared as data rather than as a * function, which is what lets the same declaration drive the terminal's handler, an * agent command, and later a wire action. - * - `client-local` — deliberately per-client view state (scrolling, layout, theme, help). + * - `client-local` — per-client view state (scrolling, layout, theme, help). * Each client implements its own handler; sharing identity is what keeps help screens * and palettes agreeing about what the command is called and what it is bound to. * - `host-only` — it runs where the review is hosted (quitting, reloading the source, - * opening `$EDITOR`). Not invocable from a remote client without an explicit allowlist, - * which is a scope boundary rather than a missing feature (audit F4). + * opening `$EDITOR`). Not invocable from a remote client without an explicit allowlist + * (audit F4). * - * This module is deliberately renderer-neutral and dependency-light: no OpenTUI, no React, - * no Node builtins, chords as plain strings. It is not part of `src/core/review` because - * it describes UI vocabulary rather than review semantics, and that module stays purely - * about what a review *is*. + * This module is renderer-neutral and dependency-light: no OpenTUI, no React, no Node + * builtins, chords as plain strings. It is not part of `src/core/review` because it + * describes UI vocabulary rather than review semantics, and that module stays purely about + * what a review *is*. */ import type { ReviewIntent } from "./review/intents"; import type { ReviewSelectionScope } from "./review/navigation"; diff --git a/src/core/review/actions.ts b/src/core/review/actions.ts index 830b35fc5..cb2345a75 100644 --- a/src/core/review/actions.ts +++ b/src/core/review/actions.ts @@ -1,5 +1,5 @@ /** - * Declares the actions the reducer executes — decided state transitions only. + * Declares the actions the reducer executes: decided state transitions only. * * An action states what changes, not whether it should: lifecycle code and intent plans * decide that first. `reduceReviewState` applies one without further validation beyond diff --git a/src/core/review/address.ts b/src/core/review/address.ts index a0a65fd1f..9df0ebd95 100644 --- a/src/core/review/address.ts +++ b/src/core/review/address.ts @@ -16,6 +16,13 @@ */ import type { ReviewSide } from "./types"; +/** + * One addressable thing in a review, at the granularity the caller means. + * + * `file` and `hunk` name structure; `line` names a position in the content by side, which + * exists whether or not anything is anchored there; `note` names one anchored note by its + * id, which outlives the line it currently hangs from. + */ export type ReviewAddress = | { kind: "file"; fileKey: string } | { kind: "hunk"; fileKey: string; hunkIndex: number } diff --git a/src/core/review/anchors.ts b/src/core/review/anchors.ts index d06303b94..ddfd09332 100644 --- a/src/core/review/anchors.ts +++ b/src/core/review/anchors.ts @@ -6,10 +6,10 @@ * this resolver placed through its fallback is not silently dropped by a consumer that * re-derives placement (`docs/browser-review-seam-audit.md`, D3/B8). * - * Resolution is deliberately permissive: an imported note may name a line the current - * patch no longer shows, and losing it entirely would be worse than hanging it from the - * nearest real hunk. Callers that must reject an unbacked target validate before - * anchoring rather than reading a verdict out of the anchor. + * Resolution is permissive: an imported note may name a line the current patch no longer + * shows, and losing it entirely would be worse than hanging it from the nearest real hunk. + * Callers that must reject an unbacked target validate before anchoring rather than + * reading a verdict out of the anchor. */ import { reviewHunkRange, reviewRangesOverlap, type ReviewHunkSpan } from "./geometry"; import type { ReviewLineRange, ReviewRangeAnchorV1, ReviewSide } from "./types"; @@ -17,6 +17,10 @@ import type { ReviewLineRange, ReviewRangeAnchorV1, ReviewSide } from "./types"; export interface ReviewNoteAnchorInput { oldRange?: ReviewLineRange; newRange?: ReviewLineRange; + /** + * The line the note was placed on. When it lands inside a hunk it decides ownership, + * ahead of any range that also intersects one. + */ preferred?: { side: ReviewSide; line: number }; /** * The hunk that owns the note when no range intersects one — an expanded-gap line, or diff --git a/src/core/review/annotations.ts b/src/core/review/annotations.ts index ac3c62504..622508154 100644 --- a/src/core/review/annotations.ts +++ b/src/core/review/annotations.ts @@ -5,12 +5,12 @@ * index, and core cannot compute it: notes arrive from sources the semantic document does * not carry (a sidecar loaded with the changeset, live agent comments, the reviewer's own * notes), and only the consumer that merged them onto the diff model knows the full set. - * It is therefore a caller-supplied fact (`ReviewIntentFacts.annotations`) — but the + * It is therefore a caller-supplied fact (`ReviewIntentFacts.annotations`), but the * *derivation* is shared, so the terminal and the producer hand the planner the same * answer instead of two that agree by coincidence. * - * File membership is deliberately broader than hunk membership: a file carrying review - * context but no note inside any hunk is still a stop on the annotated-file tour. + * File membership is broader than hunk membership: a file carrying review context but no + * note inside any hunk is still a stop on the annotated-file tour. */ import { reviewHunkRanges, reviewRangesOverlap, type ReviewHunkSpan } from "./geometry"; import type { ReviewAnnotationIndex } from "./navigation"; diff --git a/src/core/review/canonicalFile.ts b/src/core/review/canonicalFile.ts index 7b461ae54..92ddc2fc1 100644 --- a/src/core/review/canonicalFile.ts +++ b/src/core/review/canonicalFile.ts @@ -2,11 +2,7 @@ * Does this serialized file still describe the review it came from? * * A producer serves each reviewed file as a canonical JSON resource, and a reader has to - * be able to check that what it received matches the review it was published with. The - * prototype checked this three times with three different field lists — seventeen fields - * at the producer, twelve in the browser, ten in the broker — and the browser's compared - * two of them by `JSON.stringify`, so a lazily inserted key could spuriously fail a file - * that had not changed at all. None of the three compared hunk *content* + * be able to check that what it received matches the review it was published with * (`docs/browser-review-seam-audit.md`, D4). * * There is one check here, and it does not carry a field list of its own: it projects the diff --git a/src/core/review/contentManifest.ts b/src/core/review/contentManifest.ts index 36a326eb3..172c519d0 100644 --- a/src/core/review/contentManifest.ts +++ b/src/core/review/contentManifest.ts @@ -1,15 +1,13 @@ /** * A deterministic semantic snapshot of one review document. * - * The manifest is a parity instrument, not a validator: every consumer of the shared - * model can be driven through the same fixture and compared against the same manifest, - * so a renderer that re-derives geometry instead of consuming core fails visibly rather - * than drifting quietly. + * The manifest exists so every consumer of the shared model can be driven through the same + * fixture and compared against the same snapshot, making a renderer that re-derives + * geometry instead of consuming core fail visibly rather than drift quietly. * * It therefore records *derived* geometry — hunk extents, gap addresses, default note * targets, the reason a file renders nothing — alongside the content those derivations - * read. Renderer identity (runtime ids, rows, widths) is deliberately absent, since two - * consumers agreeing on it would prove nothing. + * read. Renderer identity (runtime ids, rows, widths) is left out. */ import { reviewExpansionSide, diff --git a/src/core/review/document.ts b/src/core/review/document.ts index 31f1a82ca..29dc87743 100644 --- a/src/core/review/document.ts +++ b/src/core/review/document.ts @@ -7,9 +7,8 @@ * the store, the terminal's note projection, later a transport — reads the result rather * than the diff model behind it. * - * Publication concerns (generations, resource descriptors, byte digests) deliberately do - * not appear: they belong to the producer runtime that serves a document, not to the - * document itself. + * Publication concerns (generations, resource descriptors, byte digests) do not appear: + * they belong to the producer runtime that serves a document, not to the document itself. */ import type { DiffFile } from "../types"; import { diff --git a/src/core/review/generationOrder.ts b/src/core/review/generationOrder.ts index 0b693421e..90e914b4b 100644 --- a/src/core/review/generationOrder.ts +++ b/src/core/review/generationOrder.ts @@ -19,14 +19,14 @@ * - Within one generation, state revisions strictly increase but need **not** be * contiguous: a receiver that joined late, replayed a log, or took a fresh snapshot * legitimately sees jumps. A revision that repeats is a replay, not an update. - * - Across generations, revisions are not comparable at all — a new generation may restart - * them — which is why a generation change is classified as its own verdict rather than - * folded into revision comparison. + * - Across generations, revisions are not comparable at all, since a new generation may + * restart them. A generation change is therefore classified as its own verdict rather + * than folded into revision comparison. * * Non-semantic republication (a renderer width changed, nothing about the review did) is - * deliberately *not* modelled here. It carries no new position, so it classifies as a - * replay; whoever needs to re-emit it decides that on its own publication key rather than - * by loosening this comparison. + * *not* modelled here. It carries no new position, so it classifies as a replay; whoever + * needs to re-emit it decides that on its own publication key rather than by loosening + * this comparison. */ export interface ReviewGenerationIdentity { diff --git a/src/core/review/geometry.ts b/src/core/review/geometry.ts index 5b49da746..2f2f06831 100644 --- a/src/core/review/geometry.ts +++ b/src/core/review/geometry.ts @@ -7,9 +7,9 @@ * once, because a renderer that re-derives one silently disagrees with the state store * that validates against it (`docs/browser-review-seam-audit.md`, A3/A4/A6/A10). * - * The inputs are deliberately structural rather than `ReviewFileV1`: a parsed diff hunk - * and a projected `ReviewHunkV1` both satisfy them, so the terminal can call these - * primitives from its render path without projecting a whole semantic document first. + * The inputs are structural rather than `ReviewFileV1`: a parsed diff hunk and a projected + * `ReviewHunkV1` both satisfy them, so the terminal can call these primitives from its + * render path without projecting a whole semantic document first. */ import type { ReviewLineAddressV1, ReviewLineRange, ReviewSide } from "./types"; diff --git a/src/core/review/identity.ts b/src/core/review/identity.ts index 5dd3fbf48..e7f83e40d 100644 --- a/src/core/review/identity.ts +++ b/src/core/review/identity.ts @@ -6,10 +6,10 @@ * positions or renderer object identity. Every identity here is a pure function of the * facts it names, so two processes projecting the same content agree. * - * The digest is an identity hash, not an integrity check: it is deliberately - * platform-neutral arithmetic rather than a crypto primitive, so the shared model stays - * importable from a browser bundle without a hashing runtime. Wire-integrity digests - * belong beside the transport that verifies bytes, not here. + * The digest is an identity hash, not an integrity check: it is platform-neutral + * arithmetic rather than a crypto primitive, so the shared model stays importable from a + * browser bundle without a hashing runtime. Wire-integrity digests belong beside the + * transport that verifies bytes, not here. */ /** Four independent 32-bit FNV-1a lanes, giving a 128-bit identity from one pass. */ diff --git a/src/core/review/intents.ts b/src/core/review/intents.ts index 30e988003..26e38b6f7 100644 --- a/src/core/review/intents.ts +++ b/src/core/review/intents.ts @@ -38,6 +38,13 @@ import { import type { ReviewStore } from "./store"; import type { ReviewFileV1, ReviewLineAddressV1, ReviewLineRange, ReviewSide } from "./types"; +/** + * The facts core refuses to invent, supplied by whoever submits an intent. + * + * Identity, time, and the annotation index all depend on the runtime a review is hosted + * in, so planning reads them from here instead of reaching for a clock, a UUID source, or + * a note set the semantic document does not carry. + */ export interface ReviewIntentFacts { /** Caller-allocated identity for a newly persisted note. */ noteId?: string; @@ -62,6 +69,7 @@ export interface ReviewIntentFacts { } export type ReviewIntent = + /** Select one hunk outright, revealing it the way the caller asks. */ | { type: "selection/select"; fileKey: string; hunkIndex: number; reveal: ReviewRevealRequest } /** Step the selection through one navigable scope; the scope decides wrap and reveal. */ | { type: "selection/move"; scope: ReviewSelectionScope; delta: number } @@ -69,7 +77,9 @@ export type ReviewIntent = | { type: "selection/select-file"; fileKey: string; reveal?: ReviewRevealRequest } /** Adopt the position a renderer's viewport settled on, without moving any viewport. */ | { type: "selection/anchor"; fileKey: string; hunkIndex: number } + /** Replace the review's file filter, which decides the visible stream. */ | { type: "filter/set"; filter: string } + /** Set whether agent notes are shown; reviewer-authored notes stay visible either way. */ | { type: "notes/set-visibility"; visible: boolean } /** Open a draft at one hunk, defaulting to the line a whole-hunk note hangs from. */ | { @@ -81,7 +91,9 @@ export type ReviewIntent = } /** Persist the active draft; a blank body retires the draft instead. */ | { type: "notes/create-user"; consumeDraft: true } + /** Delete one reviewer-authored note by id. */ | { type: "notes/remove-user"; noteId: string } + /** Dismiss one live agent note by id, leaving reviewer notes untouched. */ | { type: "notes/remove-live"; noteId: string } | { type: "notes/clear"; fileKey?: string; includeUser?: boolean } /** Flip one addressable collapsed gap between collapsed and expanded. */ @@ -90,12 +102,11 @@ export type ReviewIntent = /** * Every intent type, as a value rather than only as a type. * - * The wire vocabulary is derived from this list instead of restated beside it: the - * prototype hand-copied the action union into three more places, so an intent added to - * one was silently unreachable from the others (`docs/browser-review-seam-audit.md`, - * B12). The assertion below makes the list total — adding a member to `ReviewIntent` - * without naming it here fails to typecheck — and `src/session/reviewProtocol.ts` - * subtracts a named exclusion list from it rather than writing its own. + * The wire vocabulary is derived from this list instead of restated beside it + * (`docs/browser-review-seam-audit.md`, B12). The assertion below makes the list total — + * adding a member to `ReviewIntent` without naming it here fails to typecheck — and + * `src/session/reviewProtocol.ts` subtracts a named exclusion list from it rather than + * writing its own. */ export const REVIEW_INTENT_TYPES = [ "selection/select", diff --git a/src/core/review/navigation.ts b/src/core/review/navigation.ts index 8cfe4e544..0c32934f9 100644 --- a/src/core/review/navigation.ts +++ b/src/core/review/navigation.ts @@ -18,10 +18,10 @@ * another file puts that file's header on screen; crossing backward reveals the hunk * itself; annotated-hunk navigation asks for the note. Callers do not re-decide this. * - * The model this plans over is deliberately structural — file keys and hunk counts, plus - * an annotation index the consumer supplies. Which hunks count as annotated depends on - * note sources the semantic document does not carry (an imported sidecar, a renderer's - * merged live comments), so it arrives as a caller-owned fact rather than being guessed. + * The model this plans over is structural — file keys and hunk counts, plus an annotation + * index the consumer supplies. Which hunks count as annotated depends on note sources the + * semantic document does not carry (an imported sidecar, a renderer's merged live + * comments), so it arrives as a caller-owned fact rather than being guessed. */ import type { ReviewRevealRequest, ReviewSemanticSelection } from "./state"; diff --git a/src/core/review/noteSize.ts b/src/core/review/noteSize.ts index 76910ce3d..a33e3c939 100644 --- a/src/core/review/noteSize.ts +++ b/src/core/review/noteSize.ts @@ -1,16 +1,9 @@ /** * The one size a review note is measured against. * - * The prototype measured notes in two units: action validation checked `body` and - * `markup` separately, while the producer and broker checked the whole serialized note. - * A note could therefore pass the check that admitted it and then fail the check that - * published it — poisoning an entire snapshot with a capacity error rather than rejecting - * one note (`docs/browser-review-seam-audit.md`, D1). - * - * So there is one measurement, and it is the whole note: everything that will be - * serialized, counted together, in the unit the transport actually pays. A composer - * checking a note it is about to create and a producer checking a note it is about to - * publish call the same function and get the same answer. + * A note is measured whole — its serialized JSON, including framing — never field by + * field, so the check that admits a note and the check that publishes it cannot disagree. + * Composers and producers call the same function (`docs/browser-review-seam-audit.md`, D1). */ import type { ReviewNoteV1 } from "./types"; import { utf8ByteLength } from "./validation"; @@ -21,10 +14,9 @@ export const MAX_REVIEW_NOTE_BYTES = 256 * 1024; /** * The serialized size of one note. * - * Measured over the note's JSON form, because that is what a snapshot carries — summing - * the text fields alone would undercount the framing every one of them is wrapped in, and - * undercounting is how the per-field check let an oversized note through. Key order does - * not affect the total, so two encoders that order fields differently still agree. + * Measured over the note's JSON form, because that is what a snapshot carries: summing + * the text fields alone would undercount the framing every one of them is wrapped in. Key + * order does not affect the total, so two encoders that order fields differently agree. */ export function reviewNoteByteLength(note: ReviewNoteV1) { return utf8ByteLength(JSON.stringify(note)); diff --git a/src/core/review/reducer.ts b/src/core/review/reducer.ts index cef586121..7bfddbe2e 100644 --- a/src/core/review/reducer.ts +++ b/src/core/review/reducer.ts @@ -52,9 +52,9 @@ export function reduceReviewState(state: ReviewState, action: ReviewAction): Rev if (action.document === state.document) { return state; } - // Selection reconciliation is deliberately not done here: which file becomes - // selected when the current one disappears depends on the consumer's visible - // stream, so consumers dispatch the follow-up selection they want. + // Selection reconciliation is not done here: which file becomes selected when the + // current one disappears depends on the consumer's visible stream, so consumers + // dispatch the follow-up selection they want. const retired = reviewFileKeysWithRetiredContent(state.document, action.document); const expandedGaps = state.expandedGaps.filter((gap) => !retired.has(gap.fileKey)); // Loaded text is a cache of what a reader returned, not a fact of the diff: it diff --git a/src/core/review/resourceAssembly.ts b/src/core/review/resourceAssembly.ts index 14e70e3b1..1e4ac5aaf 100644 --- a/src/core/review/resourceAssembly.ts +++ b/src/core/review/resourceAssembly.ts @@ -37,6 +37,12 @@ export interface ReviewResourceChunkBytes { bytes: Uint8Array; } +/** + * What one assembler needs to verify a read it did not perform. + * + * Everything platform-shaped enters here rather than being reached for, which is why + * hashing is the injected `ReviewDigestFn` seam below. + */ export interface ReviewChunkAssemblerOptions { /** The resource being read; a chunk about anything else is a routing failure. */ resourceId: string; diff --git a/src/core/review/state.ts b/src/core/review/state.ts index 791267329..21c27510c 100644 --- a/src/core/review/state.ts +++ b/src/core/review/state.ts @@ -149,6 +149,11 @@ export interface ReviewExpandedGapState { expanded: boolean; } +/** + * Everything a review is, semantically, at one moment: the document plus what the reviewer + * has done to it. Rows, scroll offsets, widths, and themes belong to the surface drawing + * it, not here. + */ export interface ReviewState { document: ReviewDocumentV1; /** Monotonic counter advanced by every state-changing dispatch. */ diff --git a/src/core/review/types.ts b/src/core/review/types.ts index b8c84adfa..0e5804841 100644 --- a/src/core/review/types.ts +++ b/src/core/review/types.ts @@ -6,8 +6,8 @@ * transported. * * The document carries what every consumer reads. Publication addresses — generations, - * resource descriptors, digests — belong to the producer runtime and are deliberately - * absent, so nothing here implies a transport. + * resource descriptors, digests — belong to the producer runtime and are absent here, so + * nothing in this file implies a transport. */ import type { ReviewNoteSource } from "../types"; diff --git a/src/core/review/validation.ts b/src/core/review/validation.ts index 9ba63248f..cc0d36668 100644 --- a/src/core/review/validation.ts +++ b/src/core/review/validation.ts @@ -9,9 +9,9 @@ * agree — and because none of them may reach for a platform encoder or a hashing runtime * to answer these questions. * - * Hashing itself is deliberately *not* here: computing a SHA-256 needs a platform - * primitive, so it arrives as an injected `ReviewDigestFn` from whichever tier owns bytes. - * Core only names the algorithm, validates the shape, and compares two values. + * Hashing itself is *not* here: computing a SHA-256 needs a platform primitive, so it + * arrives as an injected `ReviewDigestFn` from whichever tier owns bytes. Core only names + * the algorithm, validates the shape, and compares two values. */ /** @@ -86,9 +86,9 @@ const REVIEW_SHA256_DIGEST_PATTERN = /^[0-9a-f]{64}$/; /** * Whether one value is a digest in canonical form. * - * Deliberately case-sensitive: there is one canonical spelling, and accepting both is - * what let the prototype's writer and reader disagree about whether two digests matched. - * Anything from outside is normalized on the way in rather than validated leniently. + * Case-sensitive: there is one canonical spelling, and accepting both spellings is how a + * writer and a reader come to disagree about whether two digests matched. Anything from + * outside is normalized on the way in rather than validated leniently. */ export function isReviewSha256Digest(value: unknown): value is string { return typeof value === "string" && REVIEW_SHA256_DIGEST_PATTERN.test(value); diff --git a/src/session/broker/reviewMirror.ts b/src/session/broker/reviewMirror.ts index cf8e8762c..3179f969e 100644 --- a/src/session/broker/reviewMirror.ts +++ b/src/session/broker/reviewMirror.ts @@ -6,15 +6,12 @@ * actions to it. That is the whole of what is mirrored — a position and a catalog. The * review itself stays with the session that owns it. * - * The one rule this module implements is *ordering*, and it implements it by asking - * `classifyReviewPublication` and doing what it says. The prototype's mirror had its own - * comparison — accept a lower revision never, an equal revision sometimes, a new - * generation always — which is one of the five disagreeing acceptance rules the audit - * found (`docs/browser-review-seam-audit.md`, C1). There is exactly one call to the - * classifier here and no other comparison of two publications anywhere in the daemon. + * The one rule this module implements is *ordering*, and ordering is what + * `classifyReviewPublication` says, applied verbatim — the mirror has no comparison of its + * own (`docs/browser-review-seam-audit.md`, C1). * - * A session that publishes nothing — one built before the mirror existed — is mirrored as - * nothing. That is deliberately not an error: the daemon still lists it, still brokers its + * A session that publishes nothing, such as one built before the mirror existed, is + * mirrored as nothing. That is not an error: the daemon still lists it, still brokers its * comment commands, and simply has no resources to offer on its behalf. */ import { diff --git a/src/session/reviewProtocol.ts b/src/session/reviewProtocol.ts index 2f41d9ae2..983acb81e 100644 --- a/src/session/reviewProtocol.ts +++ b/src/session/reviewProtocol.ts @@ -17,9 +17,8 @@ * of a bound that exists elsewhere. * - **Nothing is re-derived.** A caller addressing a line inside an expanded gap sends * the proof it holds (B10); the producer resolves it through `resolveReviewExpandedLine` - * and the shared anchor path. The wire never computes hunk intersections or ownership, - * which is exactly what the prototype's broker copy got wrong — its re-derivation - * omitted the fallback branch and rejected legal notes (D3). + * and the shared anchor path. The wire never computes hunk intersections or ownership + * (D3). * * The module is browser-safe by construction and gated as such: it imports from * `src/core/review/` and nothing else — no Node builtins, no broker package, no diff --git a/test/review-conformance/consumers/brokerMirror.ts b/test/review-conformance/consumers/brokerMirror.ts index 5fa763285..09b51e707 100644 --- a/test/review-conformance/consumers/brokerMirror.ts +++ b/test/review-conformance/consumers/brokerMirror.ts @@ -7,11 +7,8 @@ * the resnapshot a generation change forces, and `ignored` is everything the mirror * declined to act on. * - * That indirection is the point. The prototype's mirror compared publications itself — - * lower revisions rejected, equal ones sometimes accepted, new generations always taken — - * and the disagreement with the rest of the system only ever surfaced as a client that - * silently stopped updating (`docs/browser-review-seam-audit.md`, C1). Driving the same - * fixtures through the mirror is what proves it has no rules of its own. + * Driving the shared fixtures through the mirror's own update path is what proves it has + * no comparison rules of its own (`docs/browser-review-seam-audit.md`, C1). */ import type { ReviewPublicationAddress } from "../../../src/core/review/generationOrder"; import { ReviewMirror } from "../../../src/session/broker/reviewMirror"; diff --git a/test/review-conformance/consumers/reviewProducer.ts b/test/review-conformance/consumers/reviewProducer.ts index b66f90496..fe28a595f 100644 --- a/test/review-conformance/consumers/reviewProducer.ts +++ b/test/review-conformance/consumers/reviewProducer.ts @@ -8,8 +8,7 @@ * that addressed a different span than the manifest advertises, fails here. * * Each fixture is also self-checked at the boundary the producer actually serves: every - * canonical file it would hand out is compared against the manifest entry for it, which is - * the check three prototype implementations did three different ways (D4). + * canonical file it would hand out is compared against the manifest entry for it (D4). */ import { ReviewProducer } from "../../../src/app/review/producer"; import { assertCanonicalFileMatchesManifest } from "../../../src/core/review/canonicalFile"; diff --git a/test/review-conformance/consumers/reviewWire.ts b/test/review-conformance/consumers/reviewWire.ts index 7a7ac473e..aff99be75 100644 --- a/test/review-conformance/consumers/reviewWire.ts +++ b/test/review-conformance/consumers/reviewWire.ts @@ -8,7 +8,7 @@ * client (`docs/browser-review-seam-audit.md`, B12/B10). * * It also runs the note-size corpus, because "may this note cross a boundary" is a wire - * question the prototype answered differently from the producer (D1). + * question as much as a producer one, and both must answer it the same way (D1). */ import { reviewNoteWithinSizeLimit } from "../../../src/core/review/noteSize"; import { parseHunkReviewAction, toReviewIntent } from "../../../src/session/reviewProtocol"; diff --git a/test/review-conformance/noteSize.ts b/test/review-conformance/noteSize.ts index 2f2851050..c39628aa2 100644 --- a/test/review-conformance/noteSize.ts +++ b/test/review-conformance/noteSize.ts @@ -1,12 +1,10 @@ /** * The note-size corpus: one note, one measurement, at the boundary. * - * The prototype measured a note twice — `body` and `markup` checked separately when the - * note was admitted, the whole serialized note checked when it was published — so a note - * could pass the first check and then fail the second, taking the entire snapshot with it - * (`docs/browser-review-seam-audit.md`, D1). These fixtures are the cases that split the - * two rules apart, written from the semantics: each states the field sizes and whether the - * whole note fits. + * A note is measured whole, once, by `src/core/review/noteSize.ts` + * (`docs/browser-review-seam-audit.md`, D1). These fixtures are the cases that would split + * a whole-note measurement apart from a per-field one, written from the semantics: each + * states the field sizes and whether the whole note fits. * * Sizes are stated relative to the shared bound rather than as literals, so the corpus * still means the same thing if the bound moves. diff --git a/test/review-conformance/orderingFixtures.ts b/test/review-conformance/orderingFixtures.ts index ae8662e46..d00e4bf96 100644 --- a/test/review-conformance/orderingFixtures.ts +++ b/test/review-conformance/orderingFixtures.ts @@ -1,12 +1,9 @@ /** * The publication-ordering corpus: what is ahead, what is behind, and what needs a resnap. * - * The prototype answered this five times with three rules — one client demanded contiguous - * `+1` revisions the server never promised, another accepted equal revisions, a third - * required exact equality — and the disagreements only surfaced as a browser that silently - * stopped updating (`docs/browser-review-seam-audit.md`, C1). These fixtures pin the one - * rule from both ends: the classification itself, and the transitions a real producer - * actually emits. + * The rule these fixtures pin lives in `src/core/review/generationOrder.ts` + * (`docs/browser-review-seam-audit.md`, C1). They pin it from both ends: the + * classification itself, and the transitions a real producer actually emits. * * Verdicts are written by hand from the invariant, never captured from the classifier. */ diff --git a/test/review-conformance/types.ts b/test/review-conformance/types.ts index 08a3447f5..73563547b 100644 --- a/test/review-conformance/types.ts +++ b/test/review-conformance/types.ts @@ -203,9 +203,9 @@ export interface ReviewWireParseOutcome { /** * One consumer of the wire schema. * - * Two questions, both of which the prototype answered differently at different tiers: what - * an action means once parsed (B12/B10), and whether a note may cross a boundary at all - * (D1). A consumer joins by driving the code path it really uses. + * Two questions every tier must answer the same way: what an action means once parsed + * (B12/B10), and whether a note may cross a boundary at all (D1). A consumer joins by + * driving the code path it really uses. */ export interface ReviewWireConsumer { name: string; diff --git a/test/review-conformance/wireFixtures.ts b/test/review-conformance/wireFixtures.ts index 4464c4968..2a1fc7bf1 100644 --- a/test/review-conformance/wireFixtures.ts +++ b/test/review-conformance/wireFixtures.ts @@ -3,13 +3,11 @@ * * Every action in the vocabulary appears here with the intent it lowers to, written by * hand. That makes the corpus a statement of the round trip rather than a snapshot of the - * parser: an action that stopped lowering to the intent it derives from — the B12 failure - * mode, where a wire type drifts away from the semantics it is supposed to carry — fails - * here. + * parser: an action that stopped lowering to the intent it derives from fails here, which + * is the B12 failure mode of a wire type drifting away from the semantics it carries. * * The adversarial cases are the two the audit contributed. B10: a line inside an expanded - * gap is addressable at all, because the action carries the proof for it — the prototype's - * browser could not express one and had its clicks rejected or mis-sided. D1 is covered by + * gap is addressable at all, because the action carries the proof for it. D1 is covered by * the note-size corpus, which the wire now runs as a consumer. */ import type { ReviewWireFixture } from "./types";