feat(session): review wire protocol and daemon mirror derived from the intent vocabulary (Phase 3)

.changeset/review-wire-protocol.md

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

AGENTS.md

@@ -29,7 +29,7 @@ CLI input
   core VCS catalog. Do not add provider commands, spawning, or source readers under `src/core`.
 - Pager mode has two paths: full diff UI for patch-like stdin, plain-text fallback for non-diff pager content.
 - View defaults are layered through built-ins, user config, repo `.hunk/config.toml`, command sections, pager sections, and CLI flags.
-- `hunk daemon serve` runs one loopback daemon that brokers agent commands to many live Hunk sessions. Normal Hunk sessions should auto-start and register with that daemon when session brokering is enabled. Keep it local-only and session-brokered rather than opening per-TUI ports.
+- `hunk daemon serve` runs one loopback daemon that brokers agent commands to many live Hunk sessions. Normal Hunk sessions should auto-start and register with that daemon when session brokering is enabled. Keep it local-only and session-brokered rather than opening per-TUI ports. The daemon also mirrors each session's current review publication (generation plus resource catalog) and reads bulky content — patch text, canonical files, source — back as bounded, digest-verified resource chunks instead of holding it in the registration. Order publications with `classifyReviewPublication` and assemble chunks with `ReviewChunkAssembler`; do not add a second acceptance rule or a second assembly loop.
 - Extensions come in two tiers — user TypeScript extensions and the bundled tier in `src/extensions/default/` — running through one per-extension API object and registry (`src/extensions/runExtension.ts`, resolved via `src/extensions/apply.ts`). Every shipped VCS backend and the built-in sidebar are bundled extensions registering through the public API; that dogfooding keeps `hunkdiff/extension` honest. Hard rules: `src/extension-api/types.ts` stays import-free (declaration emission publishes whatever it reaches; `scripts/check-pack.ts` gates it); `src/extensions/default/vcs/` loads from VCS adapter resolution and must stay renderer-free (the sidebar loads separately via `getBundledSidebarView`); repo-local `.hunk/extensions/` never executes without the trust prompt; bundled extensions stay loaded under `--no-extensions`. The full architecture — host-served runtime modules, sidebar pane model, command dispatch, VCS detection ordering, conversion boundaries — is mapped in `docs/extension-architecture.md` and documented in depth by the module headers it names; the authoring guide is `docs/extensions.md`, and `skills/hunk-extensions/SKILL.md` is the agent-facing map of those touchpoints.
 - Agent rationale is optional sidecar JSON matched onto files/hunks.
 - The order of `files` in the sidecar is intentional. Hunk uses that order for the sidebar and main review stream.

scripts/review-vocabulary.test.ts

@@ -0,0 +1,134 @@
+/**
+ * Rung 5 of the per-phase seam ladder: vocabulary and constant derivation.
+ *
+ * Import gates prove a module *may* use a shared definition; the conformance harness
+ * proves consumers agree on answers. Neither catches the failure this suite exists for:
+ * a wire schema that quietly stops covering the semantics it is supposed to carry, or
+ * re-states a bound that already exists somewhere else
+ * (`docs/browser-review-rebuild.md` § "Per-phase seam verification", rung 5).
+ *
+ * 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).
+ * - 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
+ *   deliberately does not import still accommodates it (D5).
+ *
+ * It lives in `scripts/` beside the boundary gate because it is the same kind of thing —
+ * a mechanical check over the source tree — and is kept in its own file so the boundary
+ * gate's tombstone and debt lists stay easy to audit.
+ */
+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 {
+  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");
+
+/** Every production TypeScript file below one directory. */
+function sourceFiles(directory: string): string[] {
+  return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => {
+    const path = join(directory, entry.name);
+    if (entry.isDirectory()) {
+      return sourceFiles(path);
+    }
+    return /\.tsx?$/.test(entry.name) && !/\.test\.tsx?$/.test(entry.name) ? [path] : [];
+  });
+}
+
+/** Repo-relative path with forward slashes, for stable assertions on every platform. */
+function repoPath(path: string) {
+  return path
+    .slice(REPO_ROOT.length + 1)
+    .split(sep)
+    .join("/");
+}
+
+/** Every `export const NAME` one file declares. */
+function exportedConstants(path: string) {
+  return [...readFileSync(path, "utf8").matchAll(/export const ([A-Z][A-Z0-9_]*)\b/g)].map(
+    (match) => match[1]!,
+  );
+}
+
+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([]);
+  });
+
+  // A type in the vocabulary with no parser would fail open: the action would be reported
+  // as unknown rather than validated. Probing each type with a field no intent has proves
+  // a parser really ran — an unrouted type would answer `unsupported` instead.
+  test("routes every action in the vocabulary to a parser", () => {
+    for (const type of HUNK_REVIEW_ACTION_TYPES) {
+      expect(parseHunkReviewAction({ type, unexpectedField: 1 })).toEqual({
+        ok: false,
+        reason: "invalid",
+      });
+    }
+  });
+
+  test("reports a type outside the vocabulary as unsupported", () => {
+    expect(parseHunkReviewAction({ type: "notes/update-user", noteId: "n" })).toEqual({
+      ok: false,
+      reason: "unsupported",
+    });
+  });
+});
+
+describe("review constant derivation", () => {
+  // A session module that re-declares a shared name is the drift the audit found: two
+  // constants with one name, changed independently.
+  test("no session module re-declares a constant the review model exports", () => {
+    const modelConstants = new Set(sourceFiles(REVIEW_MODEL_ROOT).flatMap(exportedConstants));
+    const collisions = sourceFiles(SESSION_ROOT).flatMap((path) =>
+      exportedConstants(path)
+        .filter((name) => modelConstants.has(name))
+        .map((name) => `${repoPath(path)} -> ${name}`),
+    );
+
+    expect(collisions).toEqual([]);
+  });
+
+  // The canonical digest check lives in `core/review/validation.ts`; five inline patterns
+  // with differing case sensitivity are what let a writer and a reader disagree about
+  // whether two digests matched.
+  test("no module writes its own SHA-256 digest pattern", () => {
+    const pattern = /\{\s*64\s*\}/;
+    const offenders = [...sourceFiles(SESSION_ROOT), ...sourceFiles(REVIEW_MODEL_ROOT)]
+      .filter((path) => repoPath(path) !== "src/core/review/validation.ts")
+      .filter((path) => pattern.test(readFileSync(path, "utf8")))
+      .map(repoPath);
+
+    expect(offenders).toEqual([]);
+  });
+
+  // The wire protocol stays browser-safe by not importing the broker package, so the one
+  // coupling it cannot express as an import is asserted here instead: whatever frame the
+  // session transport carries must still fit a complete review envelope.
+  test("the session transport can carry a complete review envelope", () => {
+    expect(MAX_WS_MESSAGE_BYTES).toBeGreaterThanOrEqual(MAX_HUNK_REVIEW_ENVELOPE_BYTES);
+  });
+});

skills/hunk-review/SKILL.md

@@ -182,3 +182,4 @@ Guidelines:
 - **"Specify exactly one navigation target"** -- pick one of `--hunk`, `--old-line`, or `--new-line`.
 - **"Specify exactly one comment target"** -- pass `comment add` one of `--old-line` or `--new-line`.
 - **"Specify either --next-comment or --prev-comment, not both."** -- choose one comment-navigation direction.
+- **"Could not read the raw diff for ..."** -- the session reloaded or closed while `--include-patch` was reading it. Re-run `review`; drop `--include-patch` if you only need file and hunk structure.

src/app/review/producer.test.ts

@@ -9,7 +9,8 @@ import {
 } from "../../core/review/resources";
 import { createReviewStore } from "../../core/review/store";
 import type { DiffFile } from "../../core/types";
-import { ReviewProducer, parseReadReviewResourceRequest } from "./producer";
+import { parseReadReviewResourceRequest } from "../../core/review/resources";
+import { ReviewProducer } from "./producer";
 
 const BEFORE = lines("alpha", "beta", "gamma", "delta");
 const AFTER = lines("alpha", "BETA", "gamma", "delta");

src/app/review/producer.ts

@@ -23,7 +23,6 @@ import {
   assertReviewPublicationAdvance,
   formatReviewGeneration,
   nextReviewGeneration,
-  parseReviewGeneration,
   type ReviewGenerationIdentity,
   type ReviewPublicationAddress,
 } from "../../core/review/generationOrder";
@@ -35,13 +34,14 @@ import {
   type ReviewIntentOutcomeByType,
 } from "../../core/review/intents";
 import {
-  isReviewResourceRange,
+  parseReadReviewResourceRequest,
   REVIEW_RESOURCE_CHUNK_BYTES,
   type ReviewResourceChunkV1,
   type ReviewResourceDescriptorV1,
   type ReviewResourceErrorCode,
+  type ReviewRequestErrorCode,
 } from "../../core/review/resources";
-import { hasExactKeys, type ReviewDigestFn } from "../../core/review/validation";
+import type { ReviewDigestFn } from "../../core/review/validation";
 import type { ReviewStore } from "../../core/review/store";
 import type { DiffFile } from "../../core/types";
 import { nodeReviewDigest } from "./digest";
@@ -51,16 +51,10 @@ import { ReviewResourceStore, type ReviewResourceFailure } from "./resourceStore
 /**
  * Everything that can go wrong answering a producer request.
  *
- * The resource codes are the shared ones, so a reader classifies a failure the same way
- * whichever tier reported it; the two added here are about the request rather than the
- * resource.
+ * Composed from the shared vocabularies rather than restated, so a reader classifies a
+ * failure the same way whichever tier reported it.
  */
-export type ReviewProducerErrorCode =
-  | ReviewResourceErrorCode
-  /** The request named a generation this producer is no longer serving. */
-  | "stale-generation"
-  /** The request was not expressible: missing, extra, or wrongly typed fields. */
-  | "invalid-request";
+export type ReviewProducerErrorCode = ReviewResourceErrorCode | ReviewRequestErrorCode;
 
 export interface ReviewProducerFailure {
   ok: false;
@@ -74,36 +68,6 @@ export type ReviewProducerChunkResult =
   | { ok: true; chunk: ReviewResourceChunkV1 }
   | ReviewProducerFailure;
 
-/** One resource read, as an untrusted caller states it. */
-export interface ReadReviewResourceRequest {
-  generation: string;
-  resourceId: string;
-  offset: number;
-  length: number;
-}
-
-const READ_RESOURCE_FIELDS = ["generation", "resourceId", "offset", "length"] as const;
-
-/** Parse one resource-read request strictly, rejecting omitted and unknown fields alike. */
-export function parseReadReviewResourceRequest(
-  value: unknown,
-): ReadReviewResourceRequest | undefined {
-  if (!value || typeof value !== "object" || Array.isArray(value)) {
-    return undefined;
-  }
-  const record = value as Record<string, unknown>;
-  if (
-    !hasExactKeys(record, READ_RESOURCE_FIELDS) ||
-    parseReviewGeneration(record.generation) === undefined ||
-    typeof record.resourceId !== "string" ||
-    record.resourceId.length === 0 ||
-    !isReviewResourceRange({ offset: record.offset, length: record.length })
-  ) {
-    return undefined;
-  }
-  return record as unknown as ReadReviewResourceRequest;
-}
-
 export interface ReviewProducerOptions {
   /** Identity of this producer, part of every generation it mints. */
   producerId?: string;
@@ -191,6 +155,17 @@ export class ReviewProducer {
     this.store = store;
   }
 
+  /**
+   * 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.
+   */
+  getReviewState() {
+    return this.store?.getSnapshot();
+  }
+
   /**
    * Plan and commit one semantic intent on behalf of a caller.
    *
@@ -268,12 +243,22 @@ export class ReviewProducer {
     });
   }
 
-  /** The caller-owned facts every intent planned here is given. */
+  /**
+   * The caller-owned facts every intent planned here is given.
+   *
+   * The annotation index is keyed by the file keys of the document the plan will run
+   * against — the attached store's, when there is one — rather than by this publication's
+   * own. A host that projected its document separately from the producer would otherwise
+   * hand the planner an index addressed in a vocabulary the state does not use, and
+   * annotated navigation would silently find nothing.
+   */
   private intentFacts(): ReviewIntentFacts {
+    const document = this.store?.getSnapshot().document ?? this.publication.document;
+    const keyByRuntimeId = new Map(document.files.map((file) => [file.runtimeId, file.key]));
     return {
       annotations: buildReviewAnnotationIndex(
         [...this.publication.diffFilesByKey.values()],
-        new Map([...this.publication.diffFilesByKey].map(([fileKey, file]) => [file.id, fileKey])),
+        keyByRuntimeId,
       ),
     };
   }

src/app/review/resourceStore.ts

@@ -24,6 +24,7 @@ import {
   isReviewResourceRange,
   MAX_REVIEW_RESOURCE_BYTES,
   MAX_REVIEW_SOURCE_RESOURCE_BYTES,
+  REVIEW_RESOURCE_LOAD_CONCURRENCY,
   type ReviewResourceChunkV1,
   type ReviewResourceDescriptorV1,
   type ReviewResourceErrorCode,
@@ -39,9 +40,6 @@ import {
   type ReviewPublication,
 } from "./publication";
 
-/** How many resources one bulk load produces at a time. */
-export const REVIEW_RESOURCE_LOAD_CONCURRENCY = 4;
-
 /** How many materialized bytes one generation retains before evicting its oldest. */
 export const MAX_REVIEW_RESOURCE_CACHE_BYTES = 64 * 1024 * 1024;
 

src/core/review/expansion.test.ts

@@ -1,6 +1,8 @@
 import { describe, expect, test } from "bun:test";
+import { createTestReviewFile } from "../../../test/helpers/review-store-helpers";
 import {
   parseReviewGapId,
+  resolveReviewExpandedLine,
   reviewExpansionSide,
   reviewGapAddress,
   reviewGapId,
@@ -278,3 +280,35 @@ describe("reviewExpansionSide", () => {
     }
   });
 });
+
+describe("resolveReviewExpandedLine", () => {
+  const file = createTestReviewFile({ key: "alpha", sourceIdentity: "src:1" });
+  const claim = { gapId: "before:1", side: "new" as const, line: 5, sourceIdentity: "src:1" };
+
+  // Intent: a line the patch never showed is addressable exactly while its gap is.
+  test("accepts a line inside the gap it names", () => {
+    expect(resolveReviewExpandedLine(file, claim)?.hunkIndex).toBe(1);
+    // The gap covers lines 2..10 on both sides; its ends are inside it.
+    expect(resolveReviewExpandedLine(file, { ...claim, line: 2 })).toBeDefined();
+    expect(resolveReviewExpandedLine(file, { ...claim, line: 10 })).toBeDefined();
+  });
+
+  test("rejects a line outside the gap it names", () => {
+    expect(resolveReviewExpandedLine(file, { ...claim, line: 1 })).toBeUndefined();
+    expect(resolveReviewExpandedLine(file, { ...claim, line: 11 })).toBeUndefined();
+  });
+
+  test("rejects a gap the file does not have", () => {
+    expect(resolveReviewExpandedLine(file, { ...claim, gapId: "before:0" })).toBeUndefined();
+    expect(resolveReviewExpandedLine(file, { ...claim, gapId: "nonsense" })).toBeUndefined();
+  });
+
+  // Intent: the same gap over replaced source text is a different set of lines, so a claim
+  // about the old content must not resolve against the new.
+  test("rejects a claim about source the file no longer has", () => {
+    expect(resolveReviewExpandedLine(file, { ...claim, sourceIdentity: "src:2" })).toBeUndefined();
+    expect(
+      resolveReviewExpandedLine(createTestReviewFile({ key: "alpha" }), claim),
+    ).toBeUndefined();
+  });
+});

src/core/review/expansion.ts

@@ -162,6 +162,46 @@ export function reviewGapAddress(
   return trailing?.hunkIndex === parsed.hunkIndex ? trailing : undefined;
 }
 
+/**
+ * A caller's claim that one line it is addressing came from an expanded gap.
+ *
+ * A line inside a gap is not in the patch at all, so nothing about the file proves it
+ * exists: a surface that expanded a gap and then addressed a line in it has to say which
+ * gap, and which content it was reading. The identity is what makes the claim checkable
+ * across a reload — the same gap over different source text is a different set of lines.
+ */
+export interface ReviewExpandedLineClaim {
+  gapId: string;
+  side: ReviewSide;
+  line: number;
+  /** Identity of the source the caller expanded; must still be the file's own. */
+  sourceIdentity: string;
+}
+
+/**
+ * Resolve one expanded-line claim against the file's current geometry.
+ *
+ * Returns the gap the line belongs to, or undefined when the claim does not hold — the
+ * gap is gone, the line falls outside it, or the source behind it has been replaced. The
+ * gap's `hunkIndex` is what an anchor uses as the owning hunk, so a note on an expanded
+ * line stays attached to the hunk the reviewer was reading (`docs/browser-review-seam-
+ * audit.md`, B10/D3).
+ */
+export function resolveReviewExpandedLine(
+  file: ReviewFileV1,
+  claim: ReviewExpandedLineClaim,
+): ReviewGapAddress | undefined {
+  if (file.sourceIdentity === undefined || file.sourceIdentity !== claim.sourceIdentity) {
+    return undefined;
+  }
+  const address = reviewGapAddress(reviewGapSourceForFile(file), claim.gapId);
+  if (!address) {
+    return undefined;
+  }
+  const [start, end] = claim.side === "old" ? address.oldRange : address.newRange;
+  return claim.line >= start && claim.line <= end ? address : undefined;
+}
+
 /**
  * Which side's full source text fills this file's expanded gaps.
  *