From 92d1971acb9ba00f9728133e3db065dccf66deae Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 15:14:20 +0000 Subject: [PATCH 1/4] refactor(review): delete review wire and validation code nothing reaches The browser-review rebuild left four unreached paths behind: an identity function over the action type, an alias for `reviewNoteWithinBounds`, a lenient digest normalizer that contradicts the module's case-sensitive policy, and two empty exclusion lists whose only caller subtracted nothing. Each one is a second name for something core already answers, so the next reader has to work out which of the two is authoritative. The exclusion points survive as comments where a real exclusion would go, and the gate assertions that would catch one keep their tripwires without depending on the deleted machinery. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4 --- docs/browser-review-seam-audit.md | 7 +- scripts/review-vocabulary.test.ts | 27 +++---- src/core/commandCatalog.test.ts | 6 +- src/core/commandCatalog.ts | 11 --- src/core/review/validation.test.ts | 7 -- src/core/review/validation.ts | 9 --- src/session/reviewProtocol.test.ts | 28 ++----- src/session/reviewProtocol.ts | 76 ++++--------------- .../consumers/reviewWire.ts | 9 +-- 9 files changed, 41 insertions(+), 139 deletions(-) diff --git a/docs/browser-review-seam-audit.md b/docs/browser-review-seam-audit.md index 02c6d15d5..9774138d8 100644 --- a/docs/browser-review-seam-audit.md +++ b/docs/browser-review-seam-audit.md @@ -270,9 +270,10 @@ duplication); hunk header text (browser delegates to Pierre separators); platfor _Repaid (Phase 3)_: `REVIEW_INTENT_TYPES` in `core/review/intents.ts` is the vocabulary, made total in both directions by type assertions — a member added to `ReviewIntent` and not listed fails to typecheck, and a listed name that is not an intent fails too. - `HUNK_REVIEW_ACTION_TYPES` is that list minus `WIRE_UNREACHABLE_REVIEW_INTENT_TYPES`, which - is empty and says why: a semantic intent resolves at the producer and is broadcast to every - attached surface, so every one of them belongs to every surface. The wire _type_ is derived + `HUNK_REVIEW_ACTION_TYPES` _is_ that list, and nothing is withheld: a semantic intent + resolves at the producer and is broadcast to every attached surface, so every one of them + belongs to every surface. Withholding one would mean subtracting it by name, with the reason + it is not shareable. The wire _type_ is derived the same way — `HunkReviewActionV1` is `ReviewIntent` with the two wire-only fields added to the members that need them — and `toReviewIntent` strips them again, so an action is validated and narrowed rather than restated. The action-type-to-parser table is keyed by the diff --git a/scripts/review-vocabulary.test.ts b/scripts/review-vocabulary.test.ts index 2ac355823..95fe6e037 100644 --- a/scripts/review-vocabulary.test.ts +++ b/scripts/review-vocabulary.test.ts @@ -9,10 +9,9 @@ * * Two mechanical claims: * - * - The wire action vocabulary **is** the intent vocabulary minus a named exclusion list, - * so an intent added in a later phase becomes wire-reachable automatically and one - * deliberately withheld has to be named and justified (`browser-review-seam-audit.md`, - * B12). + * - The wire action vocabulary **is** the intent vocabulary, so an intent added in a later + * phase becomes wire-reachable automatically and one deliberately withheld has to be + * subtracted by name and justified (`browser-review-seam-audit.md`, B12). * - Coupled constants are imported, not re-declared: no session module re-declares a name * the shared review model already exports, no digest check is written as an inline * pattern beside the shared validator, and the transport bound the browser-safe protocol @@ -26,12 +25,11 @@ import { describe, expect, test } from "bun:test"; import { 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, type ReviewIntentType } from "../src/core/review/intents"; +import { REVIEW_INTENT_TYPES } from "../src/core/review/intents"; import { HUNK_REVIEW_ACTION_TYPES, MAX_HUNK_REVIEW_ENVELOPE_BYTES, parseHunkReviewAction, - WIRE_UNREACHABLE_REVIEW_INTENT_TYPES, } from "../src/session/reviewProtocol"; const REPO_ROOT = resolve(import.meta.dir, ".."); @@ -65,17 +63,12 @@ function exportedConstants(path: string) { } describe("review wire vocabulary derivation", () => { - test("is the intent vocabulary minus the named exclusions", () => { - const excluded = new Set(WIRE_UNREACHABLE_REVIEW_INTENT_TYPES); - expect([...HUNK_REVIEW_ACTION_TYPES]).toEqual( - REVIEW_INTENT_TYPES.filter((type) => !excluded.has(type)), - ); - }); - - test("names only real intents as unreachable, once each", () => { - const excluded = [...WIRE_UNREACHABLE_REVIEW_INTENT_TYPES] as ReviewIntentType[]; - expect(new Set(excluded).size).toBe(excluded.length); - expect(excluded.filter((type) => !REVIEW_INTENT_TYPES.includes(type))).toEqual([]); + // Withholding an intent would land here as a named subtraction from the intent + // vocabulary; until one is justified, a type missing from the wire is a silent drop. + test("is the intent vocabulary, whole and once each", () => { + const actionTypes = [...HUNK_REVIEW_ACTION_TYPES]; + expect(actionTypes).toEqual([...REVIEW_INTENT_TYPES]); + expect(new Set(actionTypes).size).toBe(actionTypes.length); }); // A type in the vocabulary with no parser would fail open: the action would be reported diff --git a/src/core/commandCatalog.test.ts b/src/core/commandCatalog.test.ts index 54c2bf18c..edcc1ec83 100644 --- a/src/core/commandCatalog.test.ts +++ b/src/core/commandCatalog.test.ts @@ -4,7 +4,6 @@ import { APP_COMMAND_CATALOG, appCommandCatalogEntry, lowerAppCommandToReviewIntent, - SEMANTIC_COMMANDS_WITHOUT_REVIEW_EFFECT, type AppCommandCatalogEntry, } from "./commandCatalog"; @@ -29,7 +28,8 @@ describe("app command catalog", () => { }); // Intent: the resolution locus is what tells a remote client whether it may invoke a - // command at all, so only semantic commands may carry a review effect. + // command at all, so only semantic commands may carry a review effect — and every one of + // them carries one, so a semantic command added without an effect is caught here. test("declares a review effect for semantic commands and nothing else", () => { const missingEffect = APP_COMMAND_CATALOG.filter( (command) => command.locus === "semantic" && command.review === undefined, @@ -38,7 +38,7 @@ describe("app command catalog", () => { (command) => command.locus !== "semantic" && command.review !== undefined, ).map((command) => command.id); - expect(missingEffect).toEqual([...SEMANTIC_COMMANDS_WITHOUT_REVIEW_EFFECT]); + expect(missingEffect).toEqual([]); expect(strayEffect).toEqual([]); }); diff --git a/src/core/commandCatalog.ts b/src/core/commandCatalog.ts index e8b1e1f75..91d0cc015 100644 --- a/src/core/commandCatalog.ts +++ b/src/core/commandCatalog.ts @@ -493,17 +493,6 @@ export type AppCommandId = (typeof BUILTIN_COMMANDS)[number]["id"]; export const APP_COMMAND_CATALOG: readonly AppCommandCatalogEntry[] = BUILTIN_COMMANDS; -/** - * Semantic commands whose review effect is not modelled as an intent yet. - * - * Empty, and meant to stay that way: the last two entries — starting a note and toggling - * gap expansion — became the `notes/start-draft` and `expansion/toggle` intents in the - * producer-runtime phase, so every semantic command now lowers to something every attached - * surface can resolve. The list survives as the place a deliberate exception would be - * named, rather than being left implicit in a command with no declared effect. - */ -export const SEMANTIC_COMMANDS_WITHOUT_REVIEW_EFFECT: readonly AppCommandId[] = []; - /** Look one command up by id. */ export function appCommandCatalogEntry(id: string): AppCommandCatalogEntry | undefined { return APP_COMMAND_CATALOG.find((entry) => entry.id === id); diff --git a/src/core/review/validation.test.ts b/src/core/review/validation.test.ts index 2a98b1917..765027b66 100644 --- a/src/core/review/validation.test.ts +++ b/src/core/review/validation.test.ts @@ -2,7 +2,6 @@ import { describe, expect, test } from "bun:test"; import { hasExactKeys, isReviewSha256Digest, - normalizeReviewDigest, reviewDigestsEqual, utf8ByteLength, } from "./validation"; @@ -53,12 +52,6 @@ describe("review digests", () => { expect(isReviewSha256Digest(undefined)).toBe(false); }); - test("normalizes an external digest into canonical form or rejects it", () => { - expect(normalizeReviewDigest(HEX.toUpperCase())).toBe(HEX); - expect(normalizeReviewDigest("zz")).toBeUndefined(); - expect(normalizeReviewDigest(42)).toBeUndefined(); - }); - test("compares with both operands normalized", () => { expect(reviewDigestsEqual(HEX, HEX.toUpperCase())).toBe(true); expect(reviewDigestsEqual(HEX.toUpperCase(), HEX)).toBe(true); diff --git a/src/core/review/validation.ts b/src/core/review/validation.ts index ddf6e9ecf..b9907741e 100644 --- a/src/core/review/validation.ts +++ b/src/core/review/validation.ts @@ -82,15 +82,6 @@ export function isReviewSha256Digest(value: unknown): value is string { return typeof value === "string" && REVIEW_SHA256_DIGEST_PATTERN.test(value); } -/** Put one externally supplied digest into canonical form, or undefined when it is not one. */ -export function normalizeReviewDigest(value: unknown): string | undefined { - if (typeof value !== "string") { - return undefined; - } - const normalized = value.toLowerCase(); - return isReviewSha256Digest(normalized) ? normalized : undefined; -} - /** Compare two digests with both operands normalized, never just one. */ export function reviewDigestsEqual(left: string, right: string) { return left.toLowerCase() === right.toLowerCase(); diff --git a/src/session/reviewProtocol.test.ts b/src/session/reviewProtocol.test.ts index 4ea48c7fe..621a11a09 100644 --- a/src/session/reviewProtocol.test.ts +++ b/src/session/reviewProtocol.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import { REVIEW_INTENT_TYPES, type ReviewIntent } from "../core/review/intents"; -import { MAX_REVIEW_NOTE_BYTES } from "../core/review/noteBounds"; +import { MAX_REVIEW_NOTE_BYTES, reviewNoteWithinBounds } from "../core/review/noteBounds"; import { REVIEW_CANONICAL_FILE_CONTENT_TYPE, REVIEW_PATCH_CONTENT_TYPE, @@ -9,10 +9,8 @@ import { } from "../core/review/resources"; import type { ReviewNoteV1 } from "../core/review/types"; import { - deriveReviewActionTypes, HUNK_REVIEW_ACTION_TYPES, HUNK_REVIEW_PROTOCOL_VERSION, - isTransportableReviewNote, parseHunkReviewAction, parseHunkReviewActionEnvelope, parseHunkReviewActor, @@ -22,7 +20,6 @@ import { parseHunkReviewResourceReadEnvelope, parseHunkReviewPublicationAddress, toReviewIntent, - WIRE_UNREACHABLE_REVIEW_INTENT_TYPES, } from "./reviewProtocol"; const GENERATION = "generation:p1:3"; @@ -41,19 +38,8 @@ function envelope(action: unknown, overrides: Record = {}) { describe("review action vocabulary", () => { // Intent: B12 — the wire cannot forget an intent, because it does not list them. - test("is the intent vocabulary minus the named exclusions", () => { - expect([...HUNK_REVIEW_ACTION_TYPES]).toEqual( - REVIEW_INTENT_TYPES.filter( - (type) => !(WIRE_UNREACHABLE_REVIEW_INTENT_TYPES as readonly string[]).includes(type), - ), - ); - }); - - test("subtracts exactly what an exclusion list names", () => { - expect(deriveReviewActionTypes(["filter/set", "notes/clear"], ["notes/clear"])).toEqual([ - "filter/set", - ]); - expect(deriveReviewActionTypes(["filter/set"], [])).toEqual(["filter/set"]); + test("is the intent vocabulary", () => { + expect([...HUNK_REVIEW_ACTION_TYPES]).toEqual([...REVIEW_INTENT_TYPES]); }); test("names an action outside the vocabulary as unsupported, not malformed", () => { @@ -413,19 +399,19 @@ describe("note transport bounds", () => { } // Intent: D1 — the case the prototype's two rules disagreed at. Every field passes a - // per-field check; the note is three times the bound and must be refused here, at the - // wire, rather than admitted and then poisoning the snapshot that publishes it. + // per-field check; the note is three times the bound, and the wire tier answers with the + // shared bound rather than admitting it and poisoning the snapshot that publishes it. test("refuses a note whose fields each fit but whose whole does not", () => { const note = oversizedNote(); for (const field of [note.summary, note.rationale!, note.markup!]) { expect(field.length).toBeLessThanOrEqual(MAX_REVIEW_NOTE_BYTES); } - expect(isTransportableReviewNote(note)).toBe(false); + expect(reviewNoteWithinBounds(note)).toBe(false); }); test("accepts a note within the shared bound", () => { expect( - isTransportableReviewNote({ + reviewNoteWithinBounds({ id: "user:1", source: "user", fileKey: FILE_KEY, diff --git a/src/session/reviewProtocol.ts b/src/session/reviewProtocol.ts index a03f60f82..1b86536b8 100644 --- a/src/session/reviewProtocol.ts +++ b/src/session/reviewProtocol.ts @@ -8,12 +8,11 @@ * - **The action vocabulary is `ReviewIntent`.** The prototype hand-copied that union * into a wire type, a capability list, and a validator, so an intent added to one was * silently unreachable from the others (`docs/browser-review-seam-audit.md`, B12). - * Here the vocabulary is `REVIEW_INTENT_TYPES` minus a named exclusion list, and each - * action's payload is the intent's own shape plus the few fields only a remote caller - * needs. - * - **Validation is the shared validators.** Note size is measured once, over the whole - * note, by `reviewNoteWithinBounds` (D1); exact-key checking is `hasExactKeys`; digests - * are `isReviewSha256Digest`; the resource read request and chunk are core's own types + * Here the vocabulary *is* `REVIEW_INTENT_TYPES`, and each action's payload is the + * intent's own shape plus the few fields only a remote caller needs. + * - **Validation is the shared validators.** Exact-key checking is `hasExactKeys`; digests + * are `isReviewSha256Digest`; note size is core's `reviewNoteWithinBounds`, measured once + * over the whole note (D1); the resource read request and chunk are core's own types * (D5). This module declares no digest regex, no second byte measurement, and no copy * of a bound that exists elsewhere. * - **Nothing is re-derived.** A caller addressing a line inside an expanded gap sends @@ -36,7 +35,6 @@ import { type ReviewIntentType, } from "../core/review/intents"; import { REVIEW_SELECTION_WRAP_POLICY, type ReviewSelectionScope } from "../core/review/navigation"; -import { reviewNoteWithinBounds } from "../core/review/noteBounds"; import { parseReadReviewResourceRequest, parseReviewResourceId, @@ -55,7 +53,7 @@ import { type ReviewPublicationAddress, } from "../core/review/generationOrder"; import type { ReviewRevealAnchor, ReviewRevealRequest } from "../core/review/state"; -import type { ReviewLineAddressV1, ReviewNoteV1, ReviewSide } from "../core/review/types"; +import type { ReviewLineAddressV1, ReviewSide } from "../core/review/types"; import { hasExactKeys, isReviewSha256Digest, utf8ByteLength } from "../core/review/validation"; export const HUNK_REVIEW_PROTOCOL_VERSION = 1 as const; @@ -132,36 +130,18 @@ export type HunkReviewExpandedLineProofV1 = ReviewExpandedLineClaim; // -- Action vocabulary (B12) ------------------------------------------------------------ -/** - * Intents deliberately not reachable over the wire. - * - * Empty, and that is a statement rather than an oversight: a semantic intent is by - * definition one that resolves at the producer and is broadcast to every attached - * surface, so every one of them belongs to every surface. Host-only effects — quitting, - * editing in `$EDITOR`, extension commands — are not intents at all and never enter this - * vocabulary (F4). An entry added here must carry the reason it is not shareable. - */ -export const WIRE_UNREACHABLE_REVIEW_INTENT_TYPES = - [] as const satisfies readonly ReviewIntentType[]; - -/** Subtract one named exclusion list from the intent vocabulary. */ -export function deriveReviewActionTypes( - intentTypes: readonly ReviewIntentType[], - excluded: readonly ReviewIntentType[], -): readonly ReviewIntentType[] { - return intentTypes.filter((type) => !excluded.includes(type)); -} - /** * Every action type a review client may send. * - * Derived, never listed: adding an intent makes it wire-reachable automatically, and - * withholding one requires naming it above. + * The intent vocabulary itself, never a list: adding an intent makes it wire-reachable + * automatically, and a wire-reachable type with no parser in `ACTION_PARSERS` fails to + * compile. Nothing is withheld, and that is a statement rather than an oversight — a + * semantic intent resolves at the producer and is broadcast to every attached surface, so + * every one of them belongs to every surface, while host-only effects (quitting, editing + * in `$EDITOR`, extension commands) are not intents at all (F4). Withholding one would + * mean subtracting a named exclusion list here, with the reason it is not shareable. */ -export const HUNK_REVIEW_ACTION_TYPES: readonly ReviewIntentType[] = deriveReviewActionTypes( - REVIEW_INTENT_TYPES, - WIRE_UNREACHABLE_REVIEW_INTENT_TYPES, -); +export const HUNK_REVIEW_ACTION_TYPES: readonly ReviewIntentType[] = REVIEW_INTENT_TYPES; /** * Fields a remote caller needs that a locally planned intent does not. @@ -489,18 +469,6 @@ export function toReviewIntent(action: HunkReviewActionV1): ReviewIntent { return action; } -/** - * Render one intent as the action that carries it. - * - * The inverse of `toReviewIntent` for an intent that needs no wire-only fields, which is - * every intent a local surface can plan. Round-tripping through - * `parseHunkReviewAction` must return the same value, and the conformance harness checks - * exactly that for the whole vocabulary. - */ -export function toHunkReviewAction(intent: ReviewIntent): HunkReviewActionV1 { - return intent; -} - /** Parse one action envelope, then the action inside it. */ export function parseHunkReviewActionEnvelope( value: unknown, @@ -542,22 +510,6 @@ export function parseHunkReviewResourceReadEnvelope( return { ok: true, value: record as unknown as HunkReviewResourceReadEnvelopeV1 }; } -// -- Note transport --------------------------------------------------------------------- - -/** - * Whether one note may cross a review boundary. - * - * One measurement, over the whole note, in the unit a transport pays. The prototype - * checked `body` and `markup` separately here and the whole serialized note at the - * producer, so a note could pass the check that admitted it and then fail the check that - * published it — poisoning a whole snapshot with a capacity error rather than rejecting - * one note (D1). There is no per-field check on this side any more, and there is no - * second bound: `reviewNoteWithinBounds` is the answer at every tier. - */ -export function isTransportableReviewNote(note: ReviewNoteV1) { - return reviewNoteWithinBounds(note); -} - // -- Resource catalog transport --------------------------------------------------------- /** The content type each resource kind is served as, so a parser cannot invent one. */ diff --git a/test/review-conformance/consumers/reviewWire.ts b/test/review-conformance/consumers/reviewWire.ts index f06dee1e0..732eb9c8f 100644 --- a/test/review-conformance/consumers/reviewWire.ts +++ b/test/review-conformance/consumers/reviewWire.ts @@ -10,11 +10,8 @@ * It also runs the note-bounds corpus, because "may this note cross a boundary" is a wire * question the prototype answered differently from the producer (D1). */ -import { - isTransportableReviewNote, - parseHunkReviewAction, - toReviewIntent, -} from "../../../src/session/reviewProtocol"; +import { reviewNoteWithinBounds } from "../../../src/core/review/noteBounds"; +import { parseHunkReviewAction, toReviewIntent } from "../../../src/session/reviewProtocol"; import type { ReviewWireConsumer } from "../types"; export const reviewWireConsumer: ReviewWireConsumer = { @@ -26,5 +23,5 @@ export const reviewWireConsumer: ReviewWireConsumer = { ? { accepted: true, intent: toReviewIntent(parsed.value) } : { accepted: false }; }, - acceptsNote: isTransportableReviewNote, + acceptsNote: reviewNoteWithinBounds, }; From e85443e61e095326b08a32922fcdf42336e7ffba Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 15:19:55 +0000 Subject: [PATCH 2/4] refactor(review): hoist the shared resource, record, and file helpers into core Eight small facts were stated twice across the review tiers: the resource failure shape and its constructor, which ceiling a resource kind is held to, the plain-record guard, the "file does not exist" rejection and its wording, UTF-8 byte measurement, an index clamp, and the gap-id format. Each copy is a place the two can drift, and the audit's own findings are about exactly that kind of drift going unnoticed. Core now owns one of each and both tiers import it. The producer's resource-too-large messages keep their existing wording, including the path-bearing one a reviewer sees when a source is too large to expand. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4 --- src/app/review/resourceStore.ts | 54 ++++++++++++----------- src/core/review/contentManifest.ts | 3 +- src/core/review/intents.ts | 22 ++++++---- src/core/review/navigation.ts | 2 +- src/core/review/reducer.ts | 6 +-- src/core/review/resourceAssembly.ts | 16 +++---- src/core/review/resources.ts | 40 ++++++++++++++--- src/core/review/validation.ts | 12 ++++++ src/session/app/reviewCommands.ts | 66 +++++++++-------------------- src/session/broker/state.ts | 14 ++---- src/session/reviewProtocol.ts | 14 +++--- src/ui/lib/stml/parse.ts | 5 +-- 12 files changed, 127 insertions(+), 127 deletions(-) diff --git a/src/app/review/resourceStore.ts b/src/app/review/resourceStore.ts index c942a4963..0f7b7df91 100644 --- a/src/app/review/resourceStore.ts +++ b/src/app/review/resourceStore.ts @@ -22,12 +22,12 @@ import { SourceTextTooLargeError } from "../../core/fileSource"; import { isMaterializedReviewResource, isReviewResourceRange, - MAX_REVIEW_RESOURCE_BYTES, - MAX_REVIEW_SOURCE_RESOURCE_BYTES, REVIEW_RESOURCE_LOAD_CONCURRENCY, + reviewResourceCeiling, + reviewResourceFailure, type ReviewResourceChunkV1, type ReviewResourceDescriptorV1, - type ReviewResourceErrorCode, + type ReviewResourceFailure, type ReviewResourceRange, } from "../../core/review/resources"; import { reviewDigestsEqual, type ReviewDigestFn } from "../../core/review/validation"; @@ -49,11 +49,7 @@ export interface MaterializedReviewResource { digest: string; } -export interface ReviewResourceFailure { - ok: false; - code: ReviewResourceErrorCode; - message: string; -} +export type { ReviewResourceFailure }; export type ReviewResourceLoad = | { ok: true; resource: MaterializedReviewResource } @@ -61,11 +57,6 @@ export type ReviewResourceLoad = export type ReviewResourceRead = { ok: true; chunk: ReviewResourceChunkV1 } | ReviewResourceFailure; -/** Build one typed failure without inventing a transport string for it. */ -function failure(code: ReviewResourceErrorCode, message: string): ReviewResourceFailure { - return { ok: false, code, message }; -} - /** * Serialize one canonical file, self-checking it against the manifest first. * @@ -185,7 +176,7 @@ export class ReviewResourceStore { /** Read one verified byte window of a resource. */ async readChunk(resourceId: string, range: ReviewResourceRange): Promise { if (!isReviewResourceRange(range)) { - return failure( + return reviewResourceFailure( "invalid-range", `Resource reads take a non-negative offset and a length within the shared chunk bound.`, ); @@ -198,7 +189,7 @@ export class ReviewResourceStore { const { bytes, byteLength, digest } = load.resource; if (range.offset > byteLength) { - return failure( + return reviewResourceFailure( "invalid-range", `Resource ${resourceId} has ${byteLength} bytes; offset ${range.offset} is past its end.`, ); @@ -225,14 +216,14 @@ export class ReviewResourceStore { private async produce(resourceId: string): Promise { const descriptor = reviewPublicationResource(this.publication, resourceId); if (!descriptor) { - return failure( + return reviewResourceFailure( "unknown-resource", `Review resource ${resourceId} is not part of generation ${this.publication.generation}.`, ); } const file = reviewPublicationFile(this.publication, descriptor.fileKey); if (!file) { - return failure( + return reviewResourceFailure( "unknown-resource", `Review resource ${resourceId} names a file this generation does not have.`, ); @@ -252,7 +243,7 @@ export class ReviewResourceStore { ): Promise { const fetcher = this.publication.diffFilesByKey.get(descriptor.fileKey)?.sourceFetcher; if (!fetcher) { - return failure( + return reviewResourceFailure( "resource-unavailable", `Review file ${file.path} has no source reader in this generation.`, ); @@ -266,8 +257,11 @@ export class ReviewResourceStore { // reader that failed, and a caller offering to expand context needs to tell them // apart. return error instanceof SourceTextTooLargeError - ? failure("resource-too-large", `Source for ${file.path} exceeds the readable limit.`) - : failure( + ? reviewResourceFailure( + "resource-too-large", + `Source for ${file.path} exceeds the readable limit.`, + ) + : reviewResourceFailure( "resource-unavailable", `Could not read ${descriptor.side} source for ${file.path}: ${ error instanceof Error ? error.message : String(error) @@ -275,17 +269,20 @@ export class ReviewResourceStore { ); } if (text === null) { - return failure( + return reviewResourceFailure( "resource-unavailable", `Review file ${file.path} has no ${descriptor.side} source to read.`, ); } + // Reported against the path rather than the resource id, because a reviewer asking to + // expand context needs to know which file refused before the generic measure does. + const ceiling = reviewResourceCeiling(descriptor.kind); const bytes = Buffer.from(text, "utf8"); - return bytes.byteLength > MAX_REVIEW_SOURCE_RESOURCE_BYTES - ? failure( + return bytes.byteLength > ceiling + ? reviewResourceFailure( "resource-too-large", - `Source for ${file.path} is ${bytes.byteLength} bytes, over the ${MAX_REVIEW_SOURCE_RESOURCE_BYTES}-byte source limit.`, + `Source for ${file.path} is ${bytes.byteLength} bytes, over the ${ceiling}-byte source limit.`, ) : this.measure(descriptor, bytes); } @@ -298,10 +295,11 @@ export class ReviewResourceStore { * what was produced, and disagreeing is an integrity failure rather than a miss. */ private measure(descriptor: ReviewResourceDescriptorV1, bytes: Buffer): ReviewResourceLoad { - if (bytes.byteLength > MAX_REVIEW_RESOURCE_BYTES) { - return failure( + const ceiling = reviewResourceCeiling(descriptor.kind); + if (bytes.byteLength > ceiling) { + return reviewResourceFailure( "resource-too-large", - `Review resource ${descriptor.id} is ${bytes.byteLength} bytes, over the ${MAX_REVIEW_RESOURCE_BYTES}-byte resource limit.`, + `Review resource ${descriptor.id} is ${bytes.byteLength} bytes, over the ${ceiling}-byte resource limit.`, ); } const digest = this.digest(bytes); @@ -310,7 +308,7 @@ export class ReviewResourceStore { (descriptor.byteLength !== bytes.byteLength || !reviewDigestsEqual(descriptor.digest!, digest)) ) { - return failure( + return reviewResourceFailure( "resource-integrity", `Review resource ${descriptor.id} does not match the length and digest it was published with.`, ); diff --git a/src/core/review/contentManifest.ts b/src/core/review/contentManifest.ts index fbf336dfb..36a326eb3 100644 --- a/src/core/review/contentManifest.ts +++ b/src/core/review/contentManifest.ts @@ -13,6 +13,7 @@ */ import { reviewExpansionSide, + reviewGapId, reviewGapSourceForFile, reviewLeadingGap, reviewTrailingGap, @@ -91,7 +92,7 @@ export interface ReviewContentManifest { /** Record one resolved gap address, keyed by the id every consumer addresses it with. */ function manifestGap(address: ReviewGapAddress): ReviewContentManifestGap { return { - gapId: `${address.position}:${address.hunkIndex}`, + gapId: reviewGapId(address.position, address.hunkIndex), oldRange: [...address.oldRange] as ReviewLineRange, newRange: [...address.newRange] as ReviewLineRange, lineCount: address.lineCount, diff --git a/src/core/review/intents.ts b/src/core/review/intents.ts index 8080edb50..30e988003 100644 --- a/src/core/review/intents.ts +++ b/src/core/review/intents.ts @@ -249,8 +249,14 @@ function requireFact(value: string | undefined, label: "noteId" | "draftId" | "t return value; } -/** Resolve one current semantic file or reject the target. */ -function requireFile(state: ReviewState, fileKey: string): ReviewFileV1 { +/** + * Resolve one current semantic file or reject the target. + * + * Exported because a caller validating an action before it plans one asks the same + * question, and the rejection a caller reports must be the rejection planning would have + * produced — the same code and the same words, not a second phrasing of them. + */ +export function requireReviewFile(state: ReviewState, fileKey: string): ReviewFileV1 { const file = selectReviewFileByKey(state, fileKey); if (!file) { throw new ReviewIntentPlanningError( @@ -334,7 +340,7 @@ function planDraftStart( intent: Extract, facts: ReviewIntentFacts, ): ReviewIntentPlan { - const file = requireFile(state, intent.fileKey); + const file = requireReviewFile(state, intent.fileKey); requireHunk(file, intent.hunkIndex); const hunk = file.hunks[intent.hunkIndex]!; // Where a note about the whole hunk belongs is one shared answer; a caller that @@ -367,7 +373,7 @@ function planExpansionToggle( state: ReviewState, intent: Extract, ): ReviewIntentPlan { - const file = requireFile(state, intent.fileKey); + const file = requireReviewFile(state, intent.fileKey); // Validated against the same addressing every renderer draws and every note-line check // accepts, so a gap a surface can offer is exactly a gap this intent can expand (A1). const address = reviewGapAddress(reviewGapSourceForFile(file), intent.gapId); @@ -401,7 +407,7 @@ function planUserNoteCreation(state: ReviewState, facts: ReviewIntentFacts): Rev if (!draft) { throw new ReviewIntentPlanningError("draft-missing", "No user note draft is active."); } - const file = requireFile(state, draft.fileKey); + const file = requireReviewFile(state, draft.fileKey); requireHunk(file, draft.hunkIndex); if (isBlankReviewNoteBody(draft.body)) { return { actions: [{ type: "draft/cancel" }] }; @@ -486,7 +492,7 @@ export function planReviewIntent( case "selection/select": { // Only the file is required: an out-of-range hunk clamps rather than rejecting, so // a stale index from a reloaded file still lands the reviewer somewhere real. - const file = requireFile(state, intent.fileKey); + const file = requireReviewFile(state, intent.fileKey); return { actions: [ { @@ -503,7 +509,7 @@ export function planReviewIntent( case "selection/select-file": { // The file-jump rule, owned here rather than restated per surface: selecting a file // means its first hunk, and the reveal defaults to the file's own header. - const file = requireFile(state, intent.fileKey); + const file = requireReviewFile(state, intent.fileKey); return planSelection( file.key, REVIEW_FILE_JUMP_HUNK_INDEX, @@ -511,7 +517,7 @@ export function planReviewIntent( ); } case "selection/anchor": { - const file = requireFile(state, intent.fileKey); + const file = requireReviewFile(state, intent.fileKey); return { actions: [ { diff --git a/src/core/review/navigation.ts b/src/core/review/navigation.ts index cacc05793..8cfe4e544 100644 --- a/src/core/review/navigation.ts +++ b/src/core/review/navigation.ts @@ -112,7 +112,7 @@ export const REVIEW_FILE_JUMP_REVEAL: ReviewRevealRequest = Object.freeze({ export const REVIEW_FILE_JUMP_HUNK_INDEX = 0; /** Clamp one index into an inclusive range. */ -function clamp(value: number, min: number, max: number) { +export function clamp(value: number, min: number, max: number) { return Math.min(Math.max(value, min), max); } diff --git a/src/core/review/reducer.ts b/src/core/review/reducer.ts index 2b3b62457..cef586121 100644 --- a/src/core/review/reducer.ts +++ b/src/core/review/reducer.ts @@ -6,6 +6,7 @@ * Timestamps and ids never originate here — callers put them on the action. */ import type { ReviewAction } from "./actions"; +import { clamp } from "./navigation"; import { isReviewNoteWithinClearScope, reviewFileKeysWithRetiredContent, @@ -19,11 +20,6 @@ import { type ReviewStoredNote, } from "./state"; -/** Clamp one index into an inclusive range. */ -function clamp(value: number, min: number, max: number) { - return Math.min(Math.max(value, min), max); -} - /** Compare renderer-neutral source statuses by semantic value. */ function sourceStatusesEqual(left: ReviewSourceStatus | undefined, right: ReviewSourceStatus) { if (!left || left.kind !== right.kind) { diff --git a/src/core/review/resourceAssembly.ts b/src/core/review/resourceAssembly.ts index eac55f67b..14e70e3b1 100644 --- a/src/core/review/resourceAssembly.ts +++ b/src/core/review/resourceAssembly.ts @@ -17,16 +17,15 @@ */ import { REVIEW_RESOURCE_CHUNK_BYTES, + reviewResourceFailure, type ReviewResourceChunkV1, type ReviewResourceErrorCode, + type ReviewResourceFailure, } from "./resources"; import { isReviewSha256Digest, reviewDigestsEqual, type ReviewDigestFn } from "./validation"; -export interface ReviewAssemblyFailure { - ok: false; - code: ReviewResourceErrorCode; - message: string; -} +/** A refused assembly, in the shared resource vocabulary a caller already handles. */ +export type ReviewAssemblyFailure = ReviewResourceFailure; export type ReviewAssemblyStep = { ok: true; done: boolean } | ReviewAssemblyFailure; @@ -63,11 +62,6 @@ export interface ReviewChunkAssemblerOptions { expected?: { byteLength: number; digest: string }; } -/** Build one typed failure without inventing a transport string for it. */ -function failure(code: ReviewResourceErrorCode, message: string): ReviewAssemblyFailure { - return { ok: false, code, message }; -} - /** * Assemble one resource from bounded chunks, verifying as it goes. * @@ -249,7 +243,7 @@ export class ReviewChunkAssembler { /** Record one failure so every later call reports the first cause rather than a symptom. */ private fail(code: ReviewResourceErrorCode, message: string): ReviewAssemblyFailure { - this.failed ??= failure(code, message); + this.failed ??= reviewResourceFailure(code, message); return this.failed; } } diff --git a/src/core/review/resources.ts b/src/core/review/resources.ts index cfee00689..9da096f4d 100644 --- a/src/core/review/resources.ts +++ b/src/core/review/resources.ts @@ -14,7 +14,7 @@ */ import { parseReviewGeneration } from "./generationOrder"; import type { ReviewSide } from "./types"; -import { hasExactKeys } from "./validation"; +import { asRecord, hasExactKeys } from "./validation"; export type ReviewResourceKind = "canonical-file" | "patch" | "source"; @@ -36,6 +36,17 @@ export const REVIEW_RESOURCE_LOAD_CONCURRENCY = 4; /** Largest single resource of any kind a producer will materialize. */ export const MAX_REVIEW_RESOURCE_BYTES = 32 * 1024 * 1024; +/** + * The largest a resource of one kind may be, which is what a read reserves against. + * + * Stated once because both tiers reserve: the producer before it retains bytes, the mirror + * before it accepts a stream. Two copies of "source is held to a smaller limit" would be + * two chances for one of them to keep the general bound after the source bound moved. + */ +export function reviewResourceCeiling(kind: ReviewResourceKind) { + return kind === "source" ? MAX_REVIEW_SOURCE_RESOURCE_BYTES : MAX_REVIEW_RESOURCE_BYTES; +} + export const REVIEW_CANONICAL_FILE_CONTENT_TYPE = "application/vnd.hunk.review-file+json; charset=utf-8" as const; export const REVIEW_PATCH_CONTENT_TYPE = "text/x-diff; charset=utf-8" as const; @@ -169,11 +180,9 @@ const READ_RESOURCE_FIELDS = ["generation", "resourceId", "offset", "length"] as export function parseReadReviewResourceRequest( value: unknown, ): ReadReviewResourceRequest | undefined { - if (!value || typeof value !== "object" || Array.isArray(value)) { - return undefined; - } - const record = value as Record; + const record = asRecord(value); if ( + !record || !hasExactKeys(record, READ_RESOURCE_FIELDS) || parseReviewGeneration(record.generation) === undefined || typeof record.resourceId !== "string" || @@ -232,3 +241,24 @@ export type ReviewRequestErrorCode = | "stale-generation" /** The request was not expressible: missing, extra, or wrongly typed fields. */ | "invalid-request"; + +/** + * One refused resource operation, in the vocabulary above. + * + * Shared by everything that produces or consumes resource bytes — the producer's store, + * the reader's chunk assembler — so a caller handling one of them handles both, and a code + * added here reaches every tier at once. + */ +export interface ReviewResourceFailure { + ok: false; + code: ReviewResourceErrorCode; + message: string; +} + +/** Build one typed failure without inventing a transport string for it. */ +export function reviewResourceFailure( + code: ReviewResourceErrorCode, + message: string, +): ReviewResourceFailure { + return { ok: false, code, message }; +} diff --git a/src/core/review/validation.ts b/src/core/review/validation.ts index b9907741e..9ba63248f 100644 --- a/src/core/review/validation.ts +++ b/src/core/review/validation.ts @@ -45,6 +45,18 @@ export function utf8ByteLength(value: string): number { return bytes; } +/** + * Narrows one untrusted value to a plain object, or undefined when it is not one. + * + * Arrays and `null` are excluded because both pass a bare `typeof value === "object"`, and + * a parser that accepted either would then read named keys off a value that has none. + */ +export function asRecord(value: unknown): Record | undefined { + return value && typeof value === "object" && !Array.isArray(value) + ? (value as Record) + : undefined; +} + /** * Whether one parsed record carries exactly the allowed keys — none missing, none extra. * diff --git a/src/session/app/reviewCommands.ts b/src/session/app/reviewCommands.ts index 2e90f8e4c..48dbf6155 100644 --- a/src/session/app/reviewCommands.ts +++ b/src/session/app/reviewCommands.ts @@ -20,11 +20,10 @@ * the caller here, so note and draft ids and timestamps are allocated at this edge. */ import { randomUUID } from "node:crypto"; -import type { ReviewProducer, ReviewProducerFailure } from "../../app/review/producer"; +import type { ReviewProducer } from "../../app/review/producer"; import { classifyReviewPublication } from "../../core/review/generationOrder"; import { resolveReviewExpandedLine } from "../../core/review/expansion"; -import { ReviewIntentPlanningError } from "../../core/review/intents"; -import { selectReviewFileByKey } from "../../core/review/selectors"; +import { requireReviewFile, ReviewIntentPlanningError } from "../../core/review/intents"; import type { ReviewState } from "../../core/review/state"; import type { ReviewFileV1, ReviewLineAddressV1 } from "../../core/review/types"; import { @@ -53,28 +52,19 @@ function fail( }; } -/** Lift one producer failure onto the wire with its code and message intact. */ -function fromProducerFailure(failure: ReviewProducerFailure): HunkReviewFailureV1 { - return { - ok: false, - code: failure.code, - message: failure.message, - currentGeneration: failure.currentGeneration, - }; -} - /** * Read one bounded, digest-verified window of one published resource. * * Everything about the read — strict request parsing, generation checking, single-flight * materialization, chunk bounds — already belongs to the producer; this only routes to it. + * Its answer is already a wire answer: the producer's failure codes are a subset of the + * wire's, so copying the fields across would only be a second shape of one rejection. */ export async function readSessionReviewResource( producer: ReviewProducer, envelope: HunkReviewResourceReadEnvelopeV1, ): Promise { - const read = await producer.readResource(envelope.request); - return read.ok ? read : fromProducerFailure(read); + return producer.readResource(envelope.request); } /** @@ -114,26 +104,6 @@ function checkPosition( ); } -/** Resolve one file the action names, or the failure that says it is not there. */ -function requireFile( - producer: ReviewProducer, - state: ReviewState, - fileKey: string, -): - | { file: ReviewFileV1; failure?: undefined } - | { file?: undefined; failure: HunkReviewFailureV1 } { - const file = selectReviewFileByKey(state, fileKey); - return file - ? { file } - : { - failure: fail( - producer, - "file-not-found", - `Review file ${fileKey} does not exist in the current review.`, - ), - }; -} - /** * Check one expanded-line proof against the file it claims to be about. * @@ -164,7 +134,13 @@ function checkExpandedLine( ); } -/** Validate everything about one action that needs the current review to be known. */ +/** + * Validate everything about one action that needs the current review to be known. + * + * Resolving a file the action names is core's `requireReviewFile`, so it throws a + * `ReviewIntentPlanningError` the caller converts — the same rejection, in the same words, + * a caller would have received had planning reached it. + */ function checkAgainstReview( producer: ReviewProducer, state: ReviewState, @@ -174,8 +150,8 @@ function checkAgainstReview( if (!action.expandedLineProof || !action.target) { return undefined; } - const { file, failure } = requireFile(producer, state, action.fileKey); - return failure ?? checkExpandedLine(producer, file, action.target, action.expandedLineProof); + const file = requireReviewFile(state, action.fileKey); + return checkExpandedLine(producer, file, action.target, action.expandedLineProof); } if (action.type === "notes/create-user" && action.target) { @@ -192,8 +168,8 @@ function checkAgainstReview( if (!action.expandedLineProof) { return undefined; } - const { file, failure } = requireFile(producer, state, draft.fileKey); - return failure ?? checkExpandedLine(producer, file, action.target, action.expandedLineProof); + const file = requireReviewFile(state, draft.fileKey); + return checkExpandedLine(producer, file, action.target, action.expandedLineProof); } return undefined; @@ -225,12 +201,12 @@ export function applySessionReviewAction( ); } - const rejected = checkAgainstReview(producer, state, envelope.action); - if (rejected) { - return rejected; - } - try { + const rejected = checkAgainstReview(producer, state, envelope.action); + if (rejected) { + return rejected; + } + // Identity and time are the facts core refuses to invent, and this edge is the caller // that owns them for a remote action. producer.applyIntent(toReviewIntent(envelope.action), { diff --git a/src/session/broker/state.ts b/src/session/broker/state.ts index 43da435a9..820ec9a17 100644 --- a/src/session/broker/state.ts +++ b/src/session/broker/state.ts @@ -46,10 +46,9 @@ import { ReviewResourceCache, type ReviewResourceReservation } from "./reviewRes import { ReviewChunkAssembler } from "../../core/review/resourceAssembly"; import { isMaterializedReviewResource, - MAX_REVIEW_RESOURCE_BYTES, - MAX_REVIEW_SOURCE_RESOURCE_BYTES, REVIEW_RESOURCE_CHUNK_BYTES, REVIEW_RESOURCE_LOAD_CONCURRENCY, + reviewResourceCeiling, reviewResourceId, type ReviewResourceDescriptorV1, } from "../../core/review/resources"; @@ -100,13 +99,6 @@ export class ReviewGenerationRetiredError extends Error { } } -/** The largest a resource of one kind may be, which is what a load reserves against. */ -function resourceCeiling(descriptor: ReviewResourceDescriptorV1) { - return descriptor.kind === "source" - ? MAX_REVIEW_SOURCE_RESOURCE_BYTES - : MAX_REVIEW_RESOURCE_BYTES; -} - /** Run one bounded-parallel pass over a work list, in the shared load concurrency. */ async function inBoundedParallel( items: readonly Item[], @@ -392,14 +384,14 @@ export class HunkSessionBrokerState extends SessionBrokerState< key, measured ? descriptor.byteLength! - : Math.min(REVIEW_RESOURCE_CHUNK_BYTES, resourceCeiling(descriptor)), + : Math.min(REVIEW_RESOURCE_CHUNK_BYTES, reviewResourceCeiling(descriptor.kind)), ); try { const assembler = new ReviewChunkAssembler({ resourceId: key.resourceId, generation: key.generation, digest: nodeReviewDigest, - maxBytes: resourceCeiling(descriptor), + maxBytes: reviewResourceCeiling(descriptor.kind), ...(measured ? { expected: { byteLength: descriptor.byteLength!, digest: descriptor.digest! } } : {}), diff --git a/src/session/reviewProtocol.ts b/src/session/reviewProtocol.ts index 1b86536b8..058d52019 100644 --- a/src/session/reviewProtocol.ts +++ b/src/session/reviewProtocol.ts @@ -54,7 +54,12 @@ import { } from "../core/review/generationOrder"; import type { ReviewRevealAnchor, ReviewRevealRequest } from "../core/review/state"; import type { ReviewLineAddressV1, ReviewSide } from "../core/review/types"; -import { hasExactKeys, isReviewSha256Digest, utf8ByteLength } from "../core/review/validation"; +import { + asRecord, + hasExactKeys, + isReviewSha256Digest, + utf8ByteLength, +} from "../core/review/validation"; export const HUNK_REVIEW_PROTOCOL_VERSION = 1 as const; @@ -253,13 +258,6 @@ export type HunkReviewParseResult = { ok: true; value: Value } | HunkRevi const INVALID: HunkReviewParseFailure = { ok: false, reason: "invalid" }; const UNSUPPORTED: HunkReviewParseFailure = { ok: false, reason: "unsupported" }; -/** Whether one value is a plain object rather than an array or null. */ -function asRecord(value: unknown): Record | undefined { - return value && typeof value === "object" && !Array.isArray(value) - ? (value as Record) - : undefined; -} - /** Whether one value is a non-empty identifier within the shared identifier bound. */ function isIdentifier(value: unknown): value is string { return ( diff --git a/src/ui/lib/stml/parse.ts b/src/ui/lib/stml/parse.ts index 9588bcfa8..8d66056bb 100644 --- a/src/ui/lib/stml/parse.ts +++ b/src/ui/lib/stml/parse.ts @@ -5,6 +5,7 @@ // human-readable `errors`, so a sloppy note still renders something useful. import { isRawTextStmlTag, isVoidStmlTag } from "../../../core/review/stml"; +import { utf8ByteLength } from "../../../core/review/validation"; import { sanitizeTerminalText } from "../../../lib/terminalText"; export interface StmlText { @@ -263,10 +264,6 @@ function limitedErrorCollector(errors: string[], maxErrors: number): (message: s }; } -function utf8ByteLength(text: string): number { - return new TextEncoder().encode(text).length; -} - function truncateUtf8(text: string, maxBytes: number): string { const bytes = new TextEncoder().encode(text).slice(0, maxBytes); // Lossy decode, then strip the single replacement char a mid-codepoint cut leaves. From e44f99c199d8f31789ba151f4e8cd416748efb0e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 15:20:56 +0000 Subject: [PATCH 3/4] fix(review): parse every generation sequence the formatter accepts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parser hardcoded a 15-digit sequence while the formatter accepts any non-negative safe integer, so a producer that passed 10^15 would publish generations nothing could read back — and a generation that fails to parse is treated as stale, which would strand every attached surface. Building the parser's pattern from the prefix and producer-id rule the formatter writes by removes the twin that let them disagree; the safe-integer check stays the real gate on the value. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4 --- src/core/review/generationOrder.test.ts | 19 +++++++++++++++++++ src/core/review/generationOrder.ts | 17 +++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/core/review/generationOrder.test.ts b/src/core/review/generationOrder.test.ts index a147d7c95..251dec7cf 100644 --- a/src/core/review/generationOrder.test.ts +++ b/src/core/review/generationOrder.test.ts @@ -20,6 +20,25 @@ describe("review generation identity", () => { expect(parseReviewGeneration(generation(7))).toEqual({ producerId: producer, sequence: 7 }); }); + // Intent: the parser's digit bound was narrower than the range the formatter accepts, so + // a producer that lived long enough to pass 10^15 would publish generations nothing could + // read back. Every sequence the formatter writes must parse. + test("round-trips every sequence the formatter accepts", () => { + for (const sequence of [ + 0, + 999_999_999_999_999, + 1_000_000_000_000_000, + Number.MAX_SAFE_INTEGER, + ]) { + const formatted = formatReviewGeneration({ producerId: producer, sequence }); + expect(parseReviewGeneration(formatted)).toEqual({ producerId: producer, sequence }); + } + }); + + test("rejects a sequence past the safe-integer range", () => { + expect(parseReviewGeneration(`generation:${producer}:9007199254740993`)).toBeUndefined(); + }); + test("rejects identities outside the grammar", () => { for (const value of [ "generation:p1", diff --git a/src/core/review/generationOrder.ts b/src/core/review/generationOrder.ts index f83cba7e8..0b693421e 100644 --- a/src/core/review/generationOrder.ts +++ b/src/core/review/generationOrder.ts @@ -40,7 +40,20 @@ export interface ReviewGenerationIdentity { const REVIEW_GENERATION_PREFIX = "generation"; /** Producer ids are opaque but may not contain the separator the serialized form uses. */ -const REVIEW_PRODUCER_ID_PATTERN = /^[A-Za-z0-9._-]+$/; +const REVIEW_PRODUCER_ID_BODY = "[A-Za-z0-9._-]+"; +const REVIEW_PRODUCER_ID_PATTERN = new RegExp(`^${REVIEW_PRODUCER_ID_BODY}$`); + +/** + * The serialized form, built from the prefix and producer-id rule the formatter writes by. + * + * A hand-written twin of this is what let the two disagree: a sequence the formatter + * accepted could be one the parser refused. The digit bound is only wide enough to keep + * the match cheap — every safe integer fits in sixteen digits — and `Number.isSafeInteger` + * below remains the actual gate on the value. + */ +const REVIEW_GENERATION_PATTERN = new RegExp( + `^${REVIEW_GENERATION_PREFIX}:(${REVIEW_PRODUCER_ID_BODY}):(\\d{1,16})$`, +); /** Render one generation identity as the opaque string every surface passes around. */ export function formatReviewGeneration({ producerId, sequence }: ReviewGenerationIdentity) { @@ -58,7 +71,7 @@ export function parseReviewGeneration(value: unknown): ReviewGenerationIdentity if (typeof value !== "string") { return undefined; } - const match = /^generation:([A-Za-z0-9._-]+):(\d{1,15})$/.exec(value); + const match = REVIEW_GENERATION_PATTERN.exec(value); if (!match) { return undefined; } From 04ddf82714656bbe921736c9526c2aee2faf3d24 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 15:21:31 +0000 Subject: [PATCH 4/4] test(review): scan the producer tier for hand-written digest patterns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The digest-pattern gate covered the wire and the shared model but not `src/app`, which is the tier that computes the digests the other two compare — the one place an inline pattern would do the most damage. There is no live violation, so widening the scan only closes the hole before someone falls into it. Adds the maintenance-only changeset for this cleanup. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4 --- .changeset/review-cleanup-dedup.md | 2 ++ scripts/review-vocabulary.test.ts | 10 ++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 .changeset/review-cleanup-dedup.md diff --git a/.changeset/review-cleanup-dedup.md b/.changeset/review-cleanup-dedup.md new file mode 100644 index 000000000..a845151cc --- /dev/null +++ b/.changeset/review-cleanup-dedup.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/scripts/review-vocabulary.test.ts b/scripts/review-vocabulary.test.ts index 95fe6e037..9337629c3 100644 --- a/scripts/review-vocabulary.test.ts +++ b/scripts/review-vocabulary.test.ts @@ -35,6 +35,7 @@ import { 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"); /** Every production TypeScript file below one directory. */ function sourceFiles(directory: string): string[] { @@ -107,10 +108,15 @@ describe("review constant derivation", () => { // 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. + // whether two digests matched. The producer tier is scanned too, because it is the side + // that computes the digests the other tiers compare. test("no module writes its own SHA-256 digest pattern", () => { const pattern = /\{\s*64\s*\}/; - const offenders = [...sourceFiles(SESSION_ROOT), ...sourceFiles(REVIEW_MODEL_ROOT)] + const offenders = [ + ...sourceFiles(SESSION_ROOT), + ...sourceFiles(REVIEW_MODEL_ROOT), + ...sourceFiles(PRODUCER_ROOT), + ] .filter((path) => repoPath(path) !== "src/core/review/validation.ts") .filter((path) => pattern.test(readFileSync(path, "utf8"))) .map(repoPath);