refactor(review): delete dead wire helpers and hoist duplicated ones into core

.changeset/review-cleanup-dedup.md

@@ -0,0 +1,2 @@
+---
+---

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

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,17 +25,17 @@ 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, "..");
 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[] {
@@ -65,17 +64,12 @@ function exportedConstants(path: string) {
 }
 
 describe("review wire vocabulary derivation", () => {
-  test("is the intent vocabulary minus the named exclusions", () => {
-    const excluded = new Set<string>(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
@@ -114,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);

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,23 +49,14 @@ export interface MaterializedReviewResource {
   digest: string;
 }
 
-export interface ReviewResourceFailure {
-  ok: false;
-  code: ReviewResourceErrorCode;
-  message: string;
-}
+export type { ReviewResourceFailure };
 
 export type ReviewResourceLoad =
   | { ok: true; resource: MaterializedReviewResource }
   | ReviewResourceFailure;
 
 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<ReviewResourceRead> {
     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<ReviewResourceLoad> {
     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<ReviewResourceLoad> {
     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,26 +257,32 @@ 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)
             }`,
           );
     }
     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.`,
       );

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([]);
   });
 

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);

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,

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",

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;
   }