From d4ad0bf22932b784984add4ae9fa65f936c456bd Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Mon, 27 Jul 2026 23:02:56 +0200 Subject: [PATCH 01/11] =?UTF-8?q?fix(panel):=20key=20the=20verdict=20cache?= =?UTF-8?q?=20by=20the=20full=20review=20request=20=E2=80=94=20no=20cross-?= =?UTF-8?q?request=20reuse=20(P4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Observable-gates plan P4. The panel's verdict cache keyed only on {treeHash, panelHash, rubricVersion, cacheVersion}, so a cached verdict was reused for a DIFFERENT review request on the same tree: a different --base (different diff), a different --intent (different context), or --quick (a REDUCED 1-reviewer roster, whose panelHash is the FULL panel's) all hit the same key. A quick 1-reviewer PASS could thus satisfy a full review — a panel-side false-green. - verdictCacheKey now also mixes in base, intent, and mode (quick|full), so any change in the review request MISSES the cache and forces a fresh review. - The key is now JSON-serialized (unforgeable — escapes + array delimiting) instead of a space-join, so a value containing a space can't slide across a field boundary and forge a collision (same lesson as the db:push fingerprint). NOT re-validating on a cache hit: only a real verdict is cached (⟹ validate passed), and an identical treeHash ⟹ identical code ⟹ same validate result — re-running it would just defeat the cache without adding safety. Tests: base/intent/mode each change the key; unforgeable-key (a slid space can't collide two distinct requests); existing tree-hash + cacheVersion coverage updated. Full suite green. --- packages/core/src/cli/harness-review-mode.ts | 6 ++ packages/core/src/reviewers/harness-review.ts | 22 ++++++- .../core/tests/reviewers-artifact.test.ts | 62 ++++++++++++------- 3 files changed, 65 insertions(+), 25 deletions(-) diff --git a/packages/core/src/cli/harness-review-mode.ts b/packages/core/src/cli/harness-review-mode.ts index 68aaeff4..8ea34d3f 100644 --- a/packages/core/src/cli/harness-review-mode.ts +++ b/packages/core/src/cli/harness-review-mode.ts @@ -309,6 +309,12 @@ export async function harnessReviewMode(argv: string[]): Promise { panelHash, rubricVersion: RUBRIC_VERSION, cacheVersion: CACHE_VERSION, + // The verdict is only valid for THIS review request: a different base (different diff), + // intent (different context), or mode (quick = reduced roster) must MISS the cache and + // force a fresh review — otherwise a verdict from one request false-reuses for another. + base: args.base ?? "", + intent: args.intent ?? "", + mode: args.quick ? "quick" : "full", }); let verdict: IVerdict; diff --git a/packages/core/src/reviewers/harness-review.ts b/packages/core/src/reviewers/harness-review.ts index c42e9a59..d6451512 100644 --- a/packages/core/src/reviewers/harness-review.ts +++ b/packages/core/src/reviewers/harness-review.ts @@ -228,10 +228,30 @@ export function verdictCacheKey(input: { panelHash: string; rubricVersion: string; cacheVersion: string; + /** The diff base ref — a review vs a DIFFERENT base is a different diff, so it must + * not reuse this verdict. */ + base: string; + /** The review intent — different context ⇒ a different review. */ + intent: string; + /** "quick" | "full". `quick` reviews with a REDUCED roster (1 reviewer); its verdict + * must never satisfy a full review, and vice versa. */ + mode: string; }): string { + // JSON-serialize the fields (unforgeable: escapes + array delimiting) rather than a + // space-join — a value containing a space (e.g. an intent string) could otherwise slide + // across the boundary and forge a key collision, reusing a verdict for a different + // request. Same lesson as the db:push fingerprint. return createHash("sha256") .update( - `${input.treeHash} ${input.panelHash} ${input.rubricVersion} ${input.cacheVersion}` + JSON.stringify([ + input.treeHash, + input.panelHash, + input.rubricVersion, + input.cacheVersion, + input.base, + input.intent, + input.mode, + ]) ) .digest("hex"); } diff --git a/packages/core/tests/reviewers-artifact.test.ts b/packages/core/tests/reviewers-artifact.test.ts index 8064f132..1f945762 100644 --- a/packages/core/tests/reviewers-artifact.test.ts +++ b/packages/core/tests/reviewers-artifact.test.ts @@ -16,28 +16,21 @@ const v: IVerdict = { }; describe("artifact + cache", () => { - test("cache key is stable for the same inputs and changes with the tree hash", () => { - const a = verdictCacheKey({ - treeHash: "t1", - panelHash: "p1", - rubricVersion: "1", - cacheVersion: "2", - }); - const b = verdictCacheKey({ - treeHash: "t1", - panelHash: "p1", - rubricVersion: "1", - cacheVersion: "2", - }); - const c = verdictCacheKey({ - treeHash: "t2", - panelHash: "p1", - rubricVersion: "1", - cacheVersion: "2", - }); + const key = { + treeHash: "t1", + panelHash: "p1", + rubricVersion: "1", + cacheVersion: "2", + base: "main", + intent: "fix the widget", + mode: "full", + }; - expect(a).toBe(b); - expect(a).not.toBe(c); + test("cache key is stable for the same inputs and changes with the tree hash", () => { + expect(verdictCacheKey({ ...key })).toBe(verdictCacheKey({ ...key })); + expect(verdictCacheKey({ ...key })).not.toBe( + verdictCacheKey({ ...key, treeHash: "t2" }) + ); }); test("cacheVersion is mixed into the key — bumping it retires ALL legacy artifacts", () => { @@ -45,10 +38,31 @@ describe("artifact + cache", () => { // (they carry no preReview flag, so the read-side guard can't reject them). If a // regression dropped it from the hash, legacy poison would be re-served and every // other test would still pass — so pin it here. - const base = { treeHash: "t1", panelHash: "p1", rubricVersion: "1" }; + expect(verdictCacheKey({ ...key, cacheVersion: "1" })).not.toBe( + verdictCacheKey({ ...key, cacheVersion: "2" }) + ); + }); + + test("base, intent, and mode each change the key — a verdict can't be reused across a different review request (P4)", () => { + // The whole request identity, not just the tree, keys the cache: a review vs a + // different base (different diff), a different intent (different context), or quick + // vs full (reduced roster) MUST miss the cache and force a fresh review. + expect(verdictCacheKey({ ...key })).not.toBe( + verdictCacheKey({ ...key, base: "HEAD~3" }) + ); + expect(verdictCacheKey({ ...key })).not.toBe( + verdictCacheKey({ ...key, intent: "something else" }) + ); + expect(verdictCacheKey({ ...key, mode: "quick" })).not.toBe( + verdictCacheKey({ ...key, mode: "full" }) + ); + }); - expect(verdictCacheKey({ ...base, cacheVersion: "1" })).not.toBe( - verdictCacheKey({ ...base, cacheVersion: "2" }) + test("unforgeable key: a space slid between fields can't collide two distinct requests", () => { + // A space-join would make (base 'a', intent 'b c') and (base 'a b', intent 'c') + // collide; the JSON serialization keeps them distinct. + expect(verdictCacheKey({ ...key, base: "a", intent: "b c" })).not.toBe( + verdictCacheKey({ ...key, base: "a b", intent: "c" }) ); }); From 6589eb8392b33542b3bd14ee4dc3b602506edcd5 Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Mon, 27 Jul 2026 23:14:02 +0200 Subject: [PATCH 02/11] fix(panel): key the verdict cache on RESOLVED base sha + intent, not raw flags (P4 round-2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel BLOCK (4/0, critical): keying on args.base/args.intent was still unsound — an omitted --base resolves to `merge-base main HEAD` and a named ref like `main` is resolved at review time, and an omitted --intent comes from the commit subject. So the SAME flags denote a DIFFERENT diff after main moves / a rebase, or a DIFFERENT intent after an amend — all with an unchanged treeHash → a stale PASS still shared one key and false-hit. New resolveReviewInputs(git, base, intent) resolves base→concrete sha (rev-parse, so a moved ref changes it) and intent→text (commit subject when omitted, so an amend changes it). The cache key uses those resolved values, and the SAME resolved base/intent are handed to runHarnessReview so the key and the review can never diverge. Tests: a moved base ref → different baseSha → different key; omitted intent → commit subject (amend → different key); plus the earlier base/intent/mode + unforgeable-key coverage. Full suite green. --- packages/core/src/cli/harness-review-mode.ts | 22 ++++--- packages/core/src/reviewers/harness-review.ts | 26 ++++++++ .../core/tests/reviewers-artifact.test.ts | 62 +++++++++++++++++++ 3 files changed, 103 insertions(+), 7 deletions(-) diff --git a/packages/core/src/cli/harness-review-mode.ts b/packages/core/src/cli/harness-review-mode.ts index 8ea34d3f..7967094b 100644 --- a/packages/core/src/cli/harness-review-mode.ts +++ b/packages/core/src/cli/harness-review-mode.ts @@ -20,6 +20,7 @@ import { artifactBody, shouldCacheVerdict, honorCachedVerdict, + resolveReviewInputs, CACHE_VERSION, } from "../reviewers/harness-review"; import { RUBRIC_VERSION } from "../reviewers/schema"; @@ -304,16 +305,22 @@ export async function harnessReviewMode(argv: string[]): Promise { const treeHashRes = await gitRunner(["write-tree"]); const treeHash = treeHashRes.stdout.trim(); const panelHash = computePanelHash(cfg.reviewPanel ?? {}); + // Resolve base + intent to their CONCRETE values (base ref → sha, omitted intent → + // commit subject) BEFORE keying — keying on the raw flags is unsound: a moving `main`, + // a rebase, or an amended message changes the real diff/intent with an unchanged + // treeHash and unchanged flags (4-model panel finding). The SAME resolved values feed + // the review below, so the key and the review can never diverge. + const resolved = await resolveReviewInputs(gitRunner, args.base, args.intent); const cacheKey = verdictCacheKey({ treeHash, panelHash, rubricVersion: RUBRIC_VERSION, cacheVersion: CACHE_VERSION, - // The verdict is only valid for THIS review request: a different base (different diff), - // intent (different context), or mode (quick = reduced roster) must MISS the cache and - // force a fresh review — otherwise a verdict from one request false-reuses for another. - base: args.base ?? "", - intent: args.intent ?? "", + // The verdict is only valid for THIS review request: a different resolved base + // (different diff), intent (different context), or mode (quick = reduced roster) must + // MISS the cache and force a fresh review — otherwise one request's verdict false-reuses. + base: resolved.baseSha, + intent: resolved.intent ?? "", mode: args.quick ? "quick" : "full", }); @@ -336,8 +343,9 @@ export async function harnessReviewMode(argv: string[]): Promise { identity: `${active.name}/${active.entry.model}`, }, { - base: args.base, - intent: args.intent, + // Same resolved inputs the cache key used — the review can't diverge from the key. + base: resolved.baseSha, + intent: resolved.intent ?? undefined, maxFiles: DEFAULT_MAX_FILES, maxChars: DEFAULT_MAX_CHARS, } diff --git a/packages/core/src/reviewers/harness-review.ts b/packages/core/src/reviewers/harness-review.ts index d6451512..4a44c9df 100644 --- a/packages/core/src/reviewers/harness-review.ts +++ b/packages/core/src/reviewers/harness-review.ts @@ -68,6 +68,32 @@ async function resolveIntent( return GENERIC_INTENTS.has(subject.toLowerCase()) ? null : subject; } +/** + * Resolve the review's base and intent to their CONCRETE values — what the diff is + * actually taken against and the actual intent text — so the verdict cache can key on + * them. Keying on the raw flags is unsound: an omitted `--base` resolves to + * `merge-base main HEAD` and a named ref like `main` is resolved at review time, so the + * SAME flags can denote a DIFFERENT diff after `main` moves or a rebase; an omitted + * `--intent` comes from the commit subject, which an amend changes — all with an + * unchanged treeHash. Resolving the base to a SHA (via rev-parse) and the intent to its + * text makes the cache key reflect the real review inputs, so those cases MISS the cache. + * The same resolved values are then handed to the review, so the key and the review can + * never diverge. + */ +export async function resolveReviewInputs( + git: IGitRunner, + base: string | undefined, + intent: string | undefined +): Promise<{ baseSha: string; intent: string | null }> { + const baseRef = await resolveBase(git, base); + const revParsed = (await git(["rev-parse", baseRef])).stdout.trim(); + + return { + baseSha: revParsed.length > 0 ? revParsed : baseRef, + intent: await resolveIntent(git, intent), + }; +} + async function changedFiles(git: IGitRunner, base: string): Promise { const res = await git(["diff", "--name-only", `${base}...HEAD`]); diff --git a/packages/core/tests/reviewers-artifact.test.ts b/packages/core/tests/reviewers-artifact.test.ts index 1f945762..764a1976 100644 --- a/packages/core/tests/reviewers-artifact.test.ts +++ b/packages/core/tests/reviewers-artifact.test.ts @@ -3,7 +3,9 @@ import { verdictCacheKey, artifactBody, honorCachedVerdict, + resolveReviewInputs, } from "../src/reviewers/harness-review"; +import type { IGitRunner } from "../src/reviewers/harness-review"; import type { IVerdict } from "../src/reviewers/aggregate"; const v: IVerdict = { @@ -66,6 +68,66 @@ describe("artifact + cache", () => { ); }); + // A git runner where `rev-parse ` returns `mainSha` (simulating a moved ref), + // merge-base returns a fixed sha, and the commit subject is `subject`. + const gitWith = + (mainSha: string, subject: string): IGitRunner => + async (argv) => { + if (argv[0] === "rev-parse") { + return { code: 0, stdout: `${mainSha}\n` }; + } + + if (argv[0] === "merge-base") { + return { code: 0, stdout: "mergebasesha\n" }; + } + + if (argv[0] === "log") { + return { code: 0, stdout: `${subject}\n` }; + } + + return { code: 0, stdout: "" }; + }; + + test("resolveReviewInputs: a named base ref resolves to a concrete sha → a moved ref changes the key (P4 panel finding)", async () => { + // `--base main` with `main` moved: same flags, same treeHash, but a DIFFERENT diff. + // Resolving the ref to a sha makes the key change so it can't reuse the stale verdict. + const before = await resolveReviewInputs( + gitWith("sha_A", "fix"), + "main", + "fix" + ); + const after = await resolveReviewInputs( + gitWith("sha_B", "fix"), + "main", + "fix" + ); + + expect(before.baseSha).toBe("sha_A"); + expect(after.baseSha).toBe("sha_B"); + + const keyFor = (baseSha: string): string => + verdictCacheKey({ ...key, base: baseSha }); + + expect(keyFor(before.baseSha)).not.toBe(keyFor(after.baseSha)); + }); + + test("resolveReviewInputs: omitted intent falls back to the commit subject (so an amend changes the key)", async () => { + const a = await resolveReviewInputs( + gitWith("sha", "first subject"), + undefined, + undefined + ); + const b = await resolveReviewInputs( + gitWith("sha", "amended subject"), + undefined, + undefined + ); + + expect(a.intent).toBe("first subject"); + expect(b.intent).toBe("amended subject"); + expect(a.intent).not.toBe(b.intent); + }); + test("honorCachedVerdict drops a cached pre-review block, passes a real verdict through", () => { // The read-side defense in depth: a pre-review block that somehow reached disk // must force a fresh live review (null), while a genuine panel verdict is honored. From 0047f1377fd32e658dd05b2cced3e14fa3b1318e Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Mon, 27 Jul 2026 23:41:32 +0200 Subject: [PATCH 03/11] fix(panel): key verdict cache on the reviewed-diff hash + unify CI/non-CI review path (P4 round-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel round-2 found the base-sha key still can't capture a three-dot merge-base diff: a rebase shifts merge-base(base,HEAD) while the ref/tree/subject stay fixed, so the reviewed diff changes but the key doesn't. It also found the --ci branch still reviewed raw args while persisting under the resolved key, and a git-failure fallback to the movable ref. - resolveReviewInputs now hashes the ACTUAL reviewed diff (${base}...HEAD); a git failure returns diffHash=null so the caller skips the cache (never falls back to a movable ref). - verdictCacheKey keys on diffHash (+intent+mode+panel+versions), dropping treeHash+base. - new reviewPlan() binds the cache key AND the review request to ONE resolved object, unifying the --ci and interactive paths so key and review can't diverge; --ci just never reads the cache. - tests: rebase-changes-diff, git-failure→null, reviewPlan CI-parity/quick/null-intent. --- .../plans/2026-07-27-observable-gates-plan.md | 114 ++++++++++++ packages/core/src/cli/harness-review-mode.ts | 86 ++++----- packages/core/src/reviewers/harness-review.ts | 114 +++++++++--- .../core/tests/reviewers-artifact.test.ts | 164 ++++++++++++++---- 4 files changed, 365 insertions(+), 113 deletions(-) create mode 100644 docs/plans/2026-07-27-observable-gates-plan.md diff --git a/docs/plans/2026-07-27-observable-gates-plan.md b/docs/plans/2026-07-27-observable-gates-plan.md new file mode 100644 index 00000000..d72bae17 --- /dev/null +++ b/docs/plans/2026-07-27-observable-gates-plan.md @@ -0,0 +1,114 @@ +# Observable-Gates Plan — killing the "trust the proxy, not the outcome" false-green class + +Source: 2-round consultation with the 4-model panel (deepseek-pro, glm, grok, codex), +seeded with the Reddit/DeepSeek thesis and validated against the tsforge harness. All +four converged; disagreements adjudicated below. Two of my own code-verifications anchor it. + +## Diagnosis (unanimous) + +The dominant class of **runtime/product** false-greens (the jump from "well-formed code" +to "shipped behavior") is: the harness scores that an action **ran** (exit 0 / substring +present / tests green / judge said pass) instead of asserting the final **observable +state**. It's structural, not a few incidents — the same shape recurs at db:push (#200, +#204), reachability (#202), resume baseline (#198), hollow-UI/wiring, and the review panel +(META: the panel passed a fix by inheriting its framing; only a live build falsified it). +Corollary the panel corrected me on: **most per-cycle reds are honest** (lint/type/knip) — +the lie is specifically at the code→behavior boundary. So the fix is targeted, not a rewrite. + +## Verified facts (I checked the code) + +1. **`noE2eAcceptance` is real gate-relaxation.** In `build.ts runFinalAcceptance`, a failed + FULL gate (validate/build/size/root-drift) only flips to `stuck` when e2e is *enabled*; + with the flag set it emits a "stuck" event but `return result` keeps the passed status. + The `boringstack-final-acceptance.test.ts` "FIX 5" case codifies this as intended. +2. **`settleGate` does NOT green-cache** — it re-runs `evaluateGate` every cycle. The only + real caches are the panel verdict cache (`harness-review-mode.ts`) and the differential + baseline. (Kills a round-1 red herring.) +3. **Polish-recheck failure rolls back to the pre-polish green** (a genuine green), so it's + defensible; residual risk is only an incomplete rollback (files outside snapshot / DB). + +## Plan (ordered; per-cycle cheap, expensive stuff to CI/live-qualification) + +### P0a — Make the full gate terminal regardless of `noE2eAcceptance` (cheap, ~0 cost) +- Seam: `build.ts runFinalAcceptance` (the `!finalPassed && !e2eAcceptanceDisabled` branch, + ~line 873); invert `boringstack-final-acceptance.test.ts` "FIX 5". The flag skips only the + browser/chain e2e — a failed validate/build/size/root-drift is ALWAYS terminal. +- Observable: full gate red ⇒ status `stuck` and headless exits nonzero, flag or not. +- Proof: unit test both flag states against a failing full gate → both `stuck`; live repro: + set the flag, inject deterministic root-drift/size failure, assert exit 1 (was exit 0/done). + +### P0b — DB boundary oracle after every successful `db:push` (per-cycle, ~1–2s/entity) +- Seam: new `boringstack/db-oracle.ts`, invoked right after `dbPushForce` in + `gate-stages.ts` (thread the entity spec already derived in `build.ts`); query the same + `DATABASE_URL`. +- Expected columns are **harness-derived from the PLAN, never the model's schema file**: + table = `toCamelCase(entity.id)` in schema `app`; required set = + `{id, user_id, created_at, updated_at}` ∪ plan fields ∪ `{relEntity}_id` for each non-User + belongs-to (reuse the relationship derivation in `acceptance-spec.ts`; `belongs to User` → + existing `user_id`). **SUBSET, not exact** (scaffold/model may add extra columns — don't + fail on extras). Assert column **presence** (SQL type families are too coarse for the + current plan types → at most a soft check). Match names **tolerant of camel/snake** + (verified: the live DB uses `user_id`/`created_at`; domain cols use the field name as + written) — normalize (strip `_`, lowercase) both sides. Stub `name` required only if the + plan declares `name`. +- Two-phase: the pre-model scaffold push can't have domain fields yet → assert only the + fixed infra columns there; apply the full contract after model edits. +- Proof: exit-0 push whose DB lacks `title` for `Bookmark{title,url}` ⇒ `db-schema-mismatch` + BEFORE validate runs; extra columns pass; replay valbuild27/#204 + #200 → red same cycle. + +### P1 — Reachability = real HTTP + DB-derived sentinel (per-cycle, one fetch + one SELECT) +- Seam: replace the source-substring path in `reachability.ts` with an authenticated request + after the command gate (keep static as a cheap prefilter if useful). `fetch`, NOT Playwright. +- Observable: seed a unique sentinel row via the real create path, `GET` the collection/detail, + assert the response body carries that sentinel (tri-point: plan → seed → DB row → HTTP body). + A 200 with a template/stub body fails. +- Proof: route registered but handler returns `[]`/stub, or route unmounted, or API-only/DB-only + → each red. Live: hollow-API greens that pass today. + +### P2 — Sentinel identity in the per-slice e2e (finish the partial) (CI/final acceptance) +- Seam: `e2e-generator.ts` / `e2e-runner.ts`. The one harness identity must appear in the UI + row AND an authenticated API GET AND a direct DB SELECT (extend the existing unique-value row + check). Keep create/edit/delete→reload orderings. Add persisted-FK verification to the chain. +- Proof: valbuild19 hollow Contact/Deal, build49 dead hooks → red; a usable Company → green. + +### P2 — Adversarial orderings that are actually ours (CI/live-qualification only) +- Keep: **schema-evolve** (stub `name` → domain cols; the #204 trigger) and **reload/restart-persist** + (sentinel survives). **Drop generic concurrent-rewrite** — single-user CRUD has no conflict semantics. + +### P3 — Harness-honesty "disable-must-fail" suite (CI only, NOT per-cycle) +- Seam: a mutant runner under `packages/core/scripts/` wired into `core-ci.yml`. +- Observable: deliberately break each control (drop a column, unmount a route, strip a testid, + mock an exit-0 no-op db:push, disable the P0a terminal branch) → the suite MUST go red. +- Require this artifact only for PRs that touch behavioral-control seams (not every PR). +- Proof: mutants replay #204, the disabled-e2e/full-red repro, and 200-without-sentinel. + +### P4 — Panel cache + framing integrity (pre-push/CI) +- Seam: `harness-review-mode.ts`. Always run current `validate` before trusting a cached + verdict; key the cache by diff+base+intent+reviewer-roster+quick/full mode (agreed win). +- Framing control: adopt the cheap **fails-when-disabled artifact** for harness-behavioral PRs + now. Treat glm's **framing-free counterfactual reviewer** (one reviewer sees only the DB + schema diff + plan field list, never the narrative) as an EXPERIMENT — A/B it on planted + defects before making it blocking (codex/grok caution: could be bureaucratic/proxy). + +## Explicitly DROPPED (low-ROI / over-correction — unanimous) +- Generic concurrent-rewrite / race suites (not our failure mode). +- `settleGate` green-cache work (falsified — it re-runs every cycle). +- Per-cycle Playwright or full 4-model panel. +- Blanket secret/logging sink sweep — plan-gated only, if a plan ever declares logging/secrets. +- Auto-classifying Postgres error text as red/green (unwinnable, already a known tar-pit). +- Deriving oracle expected-columns from the model's migration/schema file. +- deepseek's reverse/exact-equality column check (majority: subset-presence only). +- Polish-rollback rework as a P0 (defensible; residual partly caught by P0b anyway). + +## Guardrails (unanimous) +Never relax the gate; every new check is ADDITIVE and turns the gate RED (never redefines +green). Deterministic cheap oracles (SQL/HTTP) run per-cycle; browser + mutation + adversarial +run at final acceptance / CI only (per-cycle Playwright latency+flakiness trains the model to +ignore reds). Boundary asserts derive from the PLAN, not the model's own output. Flakiness is +browser/env, not SQL — fix isolation, don't retreat to mocks. + +## Sequencing recommendation +P0a first (1-line-ish, restores gate integrity, ~0 cost), then P0b (the primary runtime +truth-source), each shipped as its own PR with an observable test + a live-build repro, panel- +reviewed. Then P1. P2–P4 as follow-ons. One boundary check at a time, each proven by making +the OLD behavior go red. diff --git a/packages/core/src/cli/harness-review-mode.ts b/packages/core/src/cli/harness-review-mode.ts index 7967094b..c9aaf9a1 100644 --- a/packages/core/src/cli/harness-review-mode.ts +++ b/packages/core/src/cli/harness-review-mode.ts @@ -16,14 +16,12 @@ import { runHarnessReview, DEFAULT_MAX_FILES, DEFAULT_MAX_CHARS, - verdictCacheKey, artifactBody, shouldCacheVerdict, honorCachedVerdict, resolveReviewInputs, - CACHE_VERSION, + reviewPlan, } from "../reviewers/harness-review"; -import { RUBRIC_VERSION } from "../reviewers/schema"; import { parseVerdict, type IVerdict } from "../reviewers/aggregate"; interface IArgs { @@ -305,57 +303,29 @@ export async function harnessReviewMode(argv: string[]): Promise { const treeHashRes = await gitRunner(["write-tree"]); const treeHash = treeHashRes.stdout.trim(); const panelHash = computePanelHash(cfg.reviewPanel ?? {}); - // Resolve base + intent to their CONCRETE values (base ref → sha, omitted intent → - // commit subject) BEFORE keying — keying on the raw flags is unsound: a moving `main`, - // a rebase, or an amended message changes the real diff/intent with an unchanged - // treeHash and unchanged flags (4-model panel finding). The SAME resolved values feed - // the review below, so the key and the review can never diverge. + // Resolve base + intent AND fingerprint the exact reviewed diff BEFORE keying. Keying on + // the raw flags — or even a base SHA — is unsound: a moving `main`, an amended message, + // or a REBASE that shifts merge-base(base, HEAD) all change the reviewed diff + // (`${base}...HEAD`, three-dot) with the flags, tree, and subject unchanged. Hashing the + // diff bytes the reviewers actually see captures every one of those (4-model panel + // finding). The SAME resolved base feeds the review below, so key and review can't diverge. const resolved = await resolveReviewInputs(gitRunner, args.base, args.intent); - const cacheKey = verdictCacheKey({ - treeHash, - panelHash, - rubricVersion: RUBRIC_VERSION, - cacheVersion: CACHE_VERSION, - // The verdict is only valid for THIS review request: a different resolved base - // (different diff), intent (different context), or mode (quick = reduced roster) must - // MISS the cache and force a fresh review — otherwise one request's verdict false-reuses. - base: resolved.baseSha, - intent: resolved.intent ?? "", - mode: args.quick ? "quick" : "full", - }); - - let verdict: IVerdict; - - if (!args.ci) { - const cached = await readCachedVerdict(cacheKey); - - if (cached !== null) { - process.stdout.write("harness-review: cache hit, reusing verdict\n"); - verdict = cached; - } else { - verdict = await runHarnessReview( - { - git: gitRunner, - validate: validateRunner, - makeProvider, - runBinary, - panel: effective, - identity: `${active.name}/${active.entry.model}`, - }, - { - // Same resolved inputs the cache key used — the review can't diverge from the key. - base: resolved.baseSha, - intent: resolved.intent ?? undefined, - maxFiles: DEFAULT_MAX_FILES, - maxChars: DEFAULT_MAX_CHARS, - } - ); - - // persistVerdict caches ONLY a real panel verdict. A pre-review gate block - // (validate flake, empty intent, diff too large) is transient — caching one - // poisons the tree-hash so a flaky validate under load blocks every future push. - await persistVerdict(verdict, cacheKey, treeHash, panelHash); - } + // reviewPlan binds the cache key AND the review request to the SAME resolved inputs — so + // the diff the key fingerprints and the diff the review reads can't diverge, on either the + // --ci or the interactive path (the CI-parity hole the panel found). A null cacheKey means + // the diff was unfingerprintable (git failed) → neither read nor write the cache. + const plan = reviewPlan(resolved, { quick: args.quick, panelHash }); + + // ONE review path for both --ci and interactive runs. --ci never READS the cache (CI + // always re-reviews) but still WRITES it, so a later interactive run with an identical + // diff can reuse it. + let verdict: IVerdict | null = + !args.ci && plan.cacheKey !== null + ? await readCachedVerdict(plan.cacheKey) + : null; + + if (verdict !== null) { + process.stdout.write("harness-review: cache hit, reusing verdict\n"); } else { verdict = await runHarnessReview( { @@ -367,14 +337,18 @@ export async function harnessReviewMode(argv: string[]): Promise { identity: `${active.name}/${active.entry.model}`, }, { - base: args.base, - intent: args.intent, + base: plan.reviewBase, + intent: plan.reviewIntent, maxFiles: DEFAULT_MAX_FILES, maxChars: DEFAULT_MAX_CHARS, } ); - await persistVerdict(verdict, cacheKey, treeHash, panelHash); + // persistVerdict caches ONLY a real panel verdict (a pre-review gate block is transient). + // Skip persistence when the diff was unfingerprintable — there is no sound key to store under. + if (plan.cacheKey !== null) { + await persistVerdict(verdict, plan.cacheKey, treeHash, panelHash); + } } process.stdout.write(`${formatVerdict(verdict)}\n`); diff --git a/packages/core/src/reviewers/harness-review.ts b/packages/core/src/reviewers/harness-review.ts index 4a44c9df..47bd41b1 100644 --- a/packages/core/src/reviewers/harness-review.ts +++ b/packages/core/src/reviewers/harness-review.ts @@ -68,30 +68,51 @@ async function resolveIntent( return GENERIC_INTENTS.has(subject.toLowerCase()) ? null : subject; } +export interface IResolvedReviewInputs { + /** The base to hand `runHarnessReview` so its diff matches what `diffHash` hashed. */ + base: string; + /** The resolved intent text (commit subject when the flag is omitted), or null when + * empty/generic (the review will block on it). */ + intent: string | null; + /** sha256 of the ACTUAL reviewed diff (`${base}...HEAD`), or null when the diff could + * NOT be computed (git failed). A null hash means the caller MUST skip the cache + * entirely — never key on a movable ref as a fallback (that is the very bug this + * resolver exists to kill). */ + diffHash: string | null; +} + /** - * Resolve the review's base and intent to their CONCRETE values — what the diff is - * actually taken against and the actual intent text — so the verdict cache can key on - * them. Keying on the raw flags is unsound: an omitted `--base` resolves to - * `merge-base main HEAD` and a named ref like `main` is resolved at review time, so the - * SAME flags can denote a DIFFERENT diff after `main` moves or a rebase; an omitted - * `--intent` comes from the commit subject, which an amend changes — all with an - * unchanged treeHash. Resolving the base to a SHA (via rev-parse) and the intent to its - * text makes the cache key reflect the real review inputs, so those cases MISS the cache. - * The same resolved values are then handed to the review, so the key and the review can - * never diverge. + * Resolve the review's base + intent AND fingerprint the exact diff the reviewers will + * see, so the verdict cache can key on the real review inputs. Keying on the raw flags is + * unsound: an omitted `--base` resolves to `merge-base main HEAD` and a named ref like + * `main` is resolved at review time, so the SAME flags can denote a DIFFERENT diff after + * `main` moves; an omitted `--intent` comes from the commit subject, which an amend + * changes. + * + * Keying on a base SHA is NOT enough either: the diff is `${base}...HEAD` (three-dot, a + * merge-base diff), so a rebase that shifts `merge-base(base, HEAD)` changes the reviewed + * diff while the base ref, the final tree, and the subject all stay identical. The only + * fingerprint that captures every one of those is a hash of the reviewed diff itself — + * exactly the bytes the reviewers judge. The same resolved `base` is handed to the review, + * so the diff the key hashed and the diff the review reads are the same command. + * + * If the diff cannot be computed (git error), `diffHash` is null and the caller skips the + * cache — a failure must never silently fall back to keying on the movable ref. */ export async function resolveReviewInputs( git: IGitRunner, base: string | undefined, intent: string | undefined -): Promise<{ baseSha: string; intent: string | null }> { +): Promise { const baseRef = await resolveBase(git, base); - const revParsed = (await git(["rev-parse", baseRef])).stdout.trim(); - - return { - baseSha: revParsed.length > 0 ? revParsed : baseRef, - intent: await resolveIntent(git, intent), - }; + const intentText = await resolveIntent(git, intent); + const diff = await git(["diff", `${baseRef}...HEAD`]); + const diffHash = + diff.code === 0 + ? createHash("sha256").update(diff.stdout).digest("hex") + : null; + + return { base: baseRef, intent: intentText, diffHash }; } async function changedFiles(git: IGitRunner, base: string): Promise { @@ -250,14 +271,17 @@ export async function runHarnessReview( export const CACHE_VERSION = "2"; export function verdictCacheKey(input: { - treeHash: string; + /** sha256 of the ACTUAL reviewed diff (`${base}...HEAD`, three-dot). This is what the + * reviewers see, so it captures the true merge-base: a moved base ref, a rebase that + * shifts the merge-base, or any tree change all produce a different diff → a different + * key. (Keying on a base SHA alone cannot — a rebase moves the merge-base with the ref + * and tree unchanged.) */ + diffHash: string; panelHash: string; rubricVersion: string; cacheVersion: string; - /** The diff base ref — a review vs a DIFFERENT base is a different diff, so it must - * not reuse this verdict. */ - base: string; - /** The review intent — different context ⇒ a different review. */ + /** The resolved review intent (commit subject when the flag is omitted) — different + * context ⇒ a different review; an amend that changes the subject changes this. */ intent: string; /** "quick" | "full". `quick` reviews with a REDUCED roster (1 reviewer); its verdict * must never satisfy a full review, and vice versa. */ @@ -270,11 +294,10 @@ export function verdictCacheKey(input: { return createHash("sha256") .update( JSON.stringify([ - input.treeHash, + input.diffHash, input.panelHash, input.rubricVersion, input.cacheVersion, - input.base, input.intent, input.mode, ]) @@ -282,6 +305,49 @@ export function verdictCacheKey(input: { .digest("hex"); } +export interface IReviewPlan { + /** Cache key, or null when the diff was unfingerprintable (git failed) — a null key means + * the caller must NEITHER read nor write the cache (no sound key exists). */ + cacheKey: string | null; + /** Base to hand `runHarnessReview` — identical to the base the cache key's diff was taken + * against, so the review reads exactly the diff the key fingerprinted. */ + reviewBase: string; + /** Intent to hand `runHarnessReview` — the same resolved intent the key used. */ + reviewIntent: string | undefined; +} + +/** + * Bind the resolved review inputs to BOTH the cache key and the review request from a + * SINGLE source, so the diff the key fingerprints and the diff the review reads can never + * diverge. This closes the CI-parity hole the panel found: previously the `--ci` path + * reviewed the raw flags while the key was built from the resolved values, so a verdict + * could be stored under one request's key for another request's diff. Now both paths take + * `reviewBase`/`reviewIntent` from here (the `--ci` path simply never reads the cache). + * Pure and injectable so this wiring is unit-tested without spawning the CLI. + */ +export function reviewPlan( + resolved: IResolvedReviewInputs, + opts: { quick: boolean; panelHash: string } +): IReviewPlan { + const cacheKey = + resolved.diffHash === null + ? null + : verdictCacheKey({ + diffHash: resolved.diffHash, + panelHash: opts.panelHash, + rubricVersion: RUBRIC_VERSION, + cacheVersion: CACHE_VERSION, + intent: resolved.intent ?? "", + mode: opts.quick ? "quick" : "full", + }); + + return { + cacheKey, + reviewBase: resolved.base, + reviewIntent: resolved.intent ?? undefined, + }; +} + /** The caching decision, isolated so it is unit-testable without the filesystem. * ONLY a real panel verdict is cached. A pre-review gate/precondition block * (validate failed, empty intent, diff too large) is transient — caching one diff --git a/packages/core/tests/reviewers-artifact.test.ts b/packages/core/tests/reviewers-artifact.test.ts index 764a1976..17d3c4c5 100644 --- a/packages/core/tests/reviewers-artifact.test.ts +++ b/packages/core/tests/reviewers-artifact.test.ts @@ -1,11 +1,18 @@ import { test, expect, describe } from "bun:test"; +import { createHash } from "node:crypto"; import { verdictCacheKey, artifactBody, honorCachedVerdict, resolveReviewInputs, + reviewPlan, + CACHE_VERSION, +} from "../src/reviewers/harness-review"; +import { RUBRIC_VERSION } from "../src/reviewers/schema"; +import type { + IGitRunner, + IResolvedReviewInputs, } from "../src/reviewers/harness-review"; -import type { IGitRunner } from "../src/reviewers/harness-review"; import type { IVerdict } from "../src/reviewers/aggregate"; const v: IVerdict = { @@ -19,19 +26,18 @@ const v: IVerdict = { describe("artifact + cache", () => { const key = { - treeHash: "t1", + diffHash: "d1", panelHash: "p1", rubricVersion: "1", cacheVersion: "2", - base: "main", intent: "fix the widget", mode: "full", }; - test("cache key is stable for the same inputs and changes with the tree hash", () => { + test("cache key is stable for the same inputs and changes with the reviewed-diff hash", () => { expect(verdictCacheKey({ ...key })).toBe(verdictCacheKey({ ...key })); expect(verdictCacheKey({ ...key })).not.toBe( - verdictCacheKey({ ...key, treeHash: "t2" }) + verdictCacheKey({ ...key, diffHash: "d2" }) ); }); @@ -45,12 +51,12 @@ describe("artifact + cache", () => { ); }); - test("base, intent, and mode each change the key — a verdict can't be reused across a different review request (P4)", () => { - // The whole request identity, not just the tree, keys the cache: a review vs a - // different base (different diff), a different intent (different context), or quick - // vs full (reduced roster) MUST miss the cache and force a fresh review. + test("diffHash, intent, and mode each change the key — a verdict can't be reused across a different review request (P4)", () => { + // The whole request identity keys the cache: a different reviewed diff (different + // diffHash), a different intent (different context), or quick vs full (reduced roster) + // MUST miss the cache and force a fresh review. expect(verdictCacheKey({ ...key })).not.toBe( - verdictCacheKey({ ...key, base: "HEAD~3" }) + verdictCacheKey({ ...key, diffHash: "other-diff" }) ); expect(verdictCacheKey({ ...key })).not.toBe( verdictCacheKey({ ...key, intent: "something else" }) @@ -61,20 +67,27 @@ describe("artifact + cache", () => { }); test("unforgeable key: a space slid between fields can't collide two distinct requests", () => { - // A space-join would make (base 'a', intent 'b c') and (base 'a b', intent 'c') + // A space-join would make (diffHash 'a', intent 'b c') and (diffHash 'a b', intent 'c') // collide; the JSON serialization keeps them distinct. - expect(verdictCacheKey({ ...key, base: "a", intent: "b c" })).not.toBe( - verdictCacheKey({ ...key, base: "a b", intent: "c" }) + expect(verdictCacheKey({ ...key, diffHash: "a", intent: "b c" })).not.toBe( + verdictCacheKey({ ...key, diffHash: "a b", intent: "c" }) ); }); - // A git runner where `rev-parse ` returns `mainSha` (simulating a moved ref), - // merge-base returns a fixed sha, and the commit subject is `subject`. - const gitWith = - (mainSha: string, subject: string): IGitRunner => - async (argv) => { - if (argv[0] === "rev-parse") { - return { code: 0, stdout: `${mainSha}\n` }; + // A git runner scripted per command. `diff` returns `diffOut` (with `diffCode`), the + // commit subject is `subject`. It also RECORDS the argv of every `git diff` it served so + // a test can assert WHICH range was hashed. + const gitScript = (opts: { + diffOut: string; + diffCode?: number; + subject?: string; + seenDiffArgs?: string[][]; + }): IGitRunner => { + return async (argv) => { + if (argv[0] === "diff") { + opts.seenDiffArgs?.push(argv); + + return { code: opts.diffCode ?? 0, stdout: opts.diffOut }; } if (argv[0] === "merge-base") { @@ -82,43 +95,70 @@ describe("artifact + cache", () => { } if (argv[0] === "log") { - return { code: 0, stdout: `${subject}\n` }; + return { code: 0, stdout: `${opts.subject ?? "a subject"}\n` }; } return { code: 0, stdout: "" }; }; + }; - test("resolveReviewInputs: a named base ref resolves to a concrete sha → a moved ref changes the key (P4 panel finding)", async () => { - // `--base main` with `main` moved: same flags, same treeHash, but a DIFFERENT diff. - // Resolving the ref to a sha makes the key change so it can't reuse the stale verdict. + test("resolveReviewInputs hashes the ACTUAL `${base}...HEAD` diff and returns the same base for the review", async () => { + // The invariant that makes key and review inseparable: the hash is taken over exactly + // the range the review will diff, and the SAME base is returned to hand to the review. + const seen: string[][] = []; + const r = await resolveReviewInputs( + gitScript({ diffOut: "some diff", seenDiffArgs: seen, subject: "fix" }), + "main", + "fix" + ); + + expect(seen).toContainEqual(["diff", "main...HEAD"]); + expect(r.base).toBe("main"); + expect(r.diffHash).toBe( + createHash("sha256").update("some diff").digest("hex") + ); + }); + + test("resolveReviewInputs: a rebase that changes the diff — same base ref, same subject, same tree — changes the key (P4 three-dot merge-base finding)", async () => { + // `--base main` with `main`, the subject, and the final tree ALL unchanged, but a + // rebase shifted merge-base(main, HEAD) so `main...HEAD` now yields a DIFFERENT diff. + // Keying on a base sha would collide here; hashing the diff bytes does not. const before = await resolveReviewInputs( - gitWith("sha_A", "fix"), + gitScript({ diffOut: "diff BEFORE rebase", subject: "fix" }), "main", "fix" ); const after = await resolveReviewInputs( - gitWith("sha_B", "fix"), + gitScript({ diffOut: "diff AFTER rebase", subject: "fix" }), "main", "fix" ); - expect(before.baseSha).toBe("sha_A"); - expect(after.baseSha).toBe("sha_B"); + // Same named ref, but the reviewed diff (and thus its fingerprint) differs — which the + // "diffHash changes the key" test above proves yields a different cache key. + expect(before.base).toBe(after.base); + expect(before.diffHash).not.toBeNull(); + expect(before.diffHash).not.toBe(after.diffHash); + }); - const keyFor = (baseSha: string): string => - verdictCacheKey({ ...key, base: baseSha }); + test("resolveReviewInputs: a git diff failure yields diffHash null (caller MUST skip the cache, never fall back to the movable ref)", async () => { + const r = await resolveReviewInputs( + gitScript({ diffOut: "", diffCode: 128, subject: "fix" }), + "main", + "fix" + ); - expect(keyFor(before.baseSha)).not.toBe(keyFor(after.baseSha)); + expect(r.diffHash).toBeNull(); }); test("resolveReviewInputs: omitted intent falls back to the commit subject (so an amend changes the key)", async () => { const a = await resolveReviewInputs( - gitWith("sha", "first subject"), + gitScript({ diffOut: "d", subject: "first subject" }), undefined, undefined ); const b = await resolveReviewInputs( - gitWith("sha", "amended subject"), + gitScript({ diffOut: "d", subject: "amended subject" }), undefined, undefined ); @@ -128,6 +168,64 @@ describe("artifact + cache", () => { expect(a.intent).not.toBe(b.intent); }); + const resolvedInputs = ( + over: Partial + ): IResolvedReviewInputs => ({ + base: "main", + intent: "fix the widget", + diffHash: "d1", + ...over, + }); + + test("reviewPlan binds the cache key AND the review request to the SAME resolved inputs (the review can't diverge from the key — CI-parity)", () => { + // The wiring guarantee the panel demanded: the base/intent the key fingerprints are the + // exact base/intent handed to the review. There is no separate raw-flag path. + const resolved = resolvedInputs({ base: "abc123", intent: "do a thing" }); + const plan = reviewPlan(resolved, { quick: false, panelHash: "p1" }); + + expect(plan.reviewBase).toBe(resolved.base); + expect(plan.reviewIntent).toBe(resolved.intent ?? undefined); + // The key is the same one verdictCacheKey would produce for these resolved inputs. + expect(plan.cacheKey).toBe( + verdictCacheKey({ + diffHash: "d1", + panelHash: "p1", + rubricVersion: RUBRIC_VERSION, + cacheVersion: CACHE_VERSION, + intent: "do a thing", + mode: "full", + }) + ); + }); + + test("reviewPlan: quick mode keys and reviews as 'quick' (reduced roster can't satisfy a full review)", () => { + const resolved = resolvedInputs({}); + const full = reviewPlan(resolved, { quick: false, panelHash: "p1" }); + const quick = reviewPlan(resolved, { quick: true, panelHash: "p1" }); + + expect(quick.cacheKey).not.toBe(full.cacheKey); + }); + + test("reviewPlan: a null diffHash yields a null cacheKey — caller neither reads nor writes the cache (git-failure fail-safe), but STILL reviews (base/intent set)", () => { + const plan = reviewPlan(resolvedInputs({ diffHash: null }), { + quick: false, + panelHash: "p1", + }); + + expect(plan.cacheKey).toBeNull(); + expect(plan.reviewBase).toBe("main"); + expect(plan.reviewIntent).toBe("fix the widget"); + }); + + test("reviewPlan: a null intent is passed to the review as undefined (not the string 'null')", () => { + const plan = reviewPlan(resolvedInputs({ intent: null }), { + quick: false, + panelHash: "p1", + }); + + expect(plan.reviewIntent).toBeUndefined(); + }); + test("honorCachedVerdict drops a cached pre-review block, passes a real verdict through", () => { // The read-side defense in depth: a pre-review block that somehow reached disk // must force a fresh live review (null), while a genuine panel verdict is honored. From b5b868b2f71784929bd965cfff0ad1595bb65b49 Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Mon, 27 Jul 2026 23:57:32 +0200 Subject: [PATCH 04/11] fix(panel): re-run validate before trusting the verdict cache + testable orchestration (P4 round-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel round-3 found dropping treeHash removed the live-state invalidation: keying purely on the committed ${base}...HEAD diff let a cache hit reuse a green verdict even when the current worktree would fail validate. P4's actual requirement was to re-run validate before trusting a cached verdict. - new resolveVerdict() orchestrator: runs validate FRESH (caller), trusts a cached verdict ONLY when the current gate is green; the summary is threaded into the review (IGatherOptions.validateSummary) so validate runs exactly once. The cache short-circuits only the expensive model panel, never the gate. - extracted from the CLI so the wiring is unit-tested (cache-hit reuse, keyed write, --ci writes-but-never-reads, red-validate-skips-cache, null-key no-op) — the integration coverage the panel required. - resolveBase now pins the base to merge-base(ref, HEAD) as an immutable SHA, so the fingerprinted bytes and the reviewed bytes are one snapshot even if the ref moves. - tests: resolveVerdict scenarios + gatherChange reuses a provided summary. Full suite 3178/0. --- packages/core/src/cli/harness-review-mode.ts | 70 +++++---- packages/core/src/reviewers/harness-review.ts | 83 +++++++++- .../core/tests/reviewers-artifact.test.ts | 142 +++++++++++++++++- .../tests/reviewers-harness-review.test.ts | 22 +++ 4 files changed, 273 insertions(+), 44 deletions(-) diff --git a/packages/core/src/cli/harness-review-mode.ts b/packages/core/src/cli/harness-review-mode.ts index c9aaf9a1..fb5a13fa 100644 --- a/packages/core/src/cli/harness-review-mode.ts +++ b/packages/core/src/cli/harness-review-mode.ts @@ -21,6 +21,7 @@ import { honorCachedVerdict, resolveReviewInputs, reviewPlan, + resolveVerdict, } from "../reviewers/harness-review"; import { parseVerdict, type IVerdict } from "../reviewers/aggregate"; @@ -316,39 +317,44 @@ export async function harnessReviewMode(argv: string[]): Promise { // the diff was unfingerprintable (git failed) → neither read nor write the cache. const plan = reviewPlan(resolved, { quick: args.quick, panelHash }); - // ONE review path for both --ci and interactive runs. --ci never READS the cache (CI - // always re-reviews) but still WRITES it, so a later interactive run with an identical - // diff can reuse it. - let verdict: IVerdict | null = - !args.ci && plan.cacheKey !== null - ? await readCachedVerdict(plan.cacheKey) - : null; - - if (verdict !== null) { - process.stdout.write("harness-review: cache hit, reusing verdict\n"); - } else { - verdict = await runHarnessReview( - { - git: gitRunner, - validate: validateRunner, - makeProvider, - runBinary, - panel: effective, - identity: `${active.name}/${active.entry.model}`, - }, - { - base: plan.reviewBase, - intent: plan.reviewIntent, - maxFiles: DEFAULT_MAX_FILES, - maxChars: DEFAULT_MAX_CHARS, - } + // Run validate FRESH, once, before consulting the cache. The verdict cache short-circuits + // only the expensive model panel — never the gate: a cached PASS is trusted only when the + // current worktree is still green (resolveVerdict gates the cache read on this summary), + // and the summary is threaded into the review so validate never runs twice. This is P4's + // core requirement — re-run validate before trusting a cached verdict. + const validateSummary = await validateRunner(); + + const { verdict, cacheHit } = await resolveVerdict( + { + readCache: readCachedVerdict, + runReview: (base, intent, summary) => + runHarnessReview( + { + git: gitRunner, + validate: validateRunner, + makeProvider, + runBinary, + panel: effective, + identity: `${active.name}/${active.entry.model}`, + }, + { + base, + intent, + maxFiles: DEFAULT_MAX_FILES, + maxChars: DEFAULT_MAX_CHARS, + validateSummary: summary, + } + ), + persist: (v, cacheKey) => + persistVerdict(v, cacheKey, treeHash, panelHash), + }, + { ci: args.ci, plan, validateSummary } + ); + + if (cacheHit) { + process.stdout.write( + "harness-review: cache hit (validate re-run green), reusing verdict\n" ); - - // persistVerdict caches ONLY a real panel verdict (a pre-review gate block is transient). - // Skip persistence when the diff was unfingerprintable — there is no sound key to store under. - if (plan.cacheKey !== null) { - await persistVerdict(verdict, plan.cacheKey, treeHash, panelHash); - } } process.stdout.write(`${formatVerdict(verdict)}\n`); diff --git a/packages/core/src/reviewers/harness-review.ts b/packages/core/src/reviewers/harness-review.ts index 47bd41b1..82165bd7 100644 --- a/packages/core/src/reviewers/harness-review.ts +++ b/packages/core/src/reviewers/harness-review.ts @@ -35,6 +35,10 @@ export interface IGatherOptions { intent?: string; maxFiles: number; maxChars: number; + /** A validate summary already computed by the caller. When present, gatherChange REUSES + * it instead of running validate again — so validate runs exactly once per invocation + * even when the caller re-ran it to decide whether to trust the verdict cache. */ + validateSummary?: IValidateSummary; } export type GatherResult = @@ -45,14 +49,21 @@ async function resolveBase( git: IGitRunner, explicit: string | undefined ): Promise { - if (explicit !== undefined && explicit.length > 0) { - return explicit; - } - - const res = await git(["merge-base", "main", "HEAD"]); + const ref = explicit !== undefined && explicit.length > 0 ? explicit : "main"; + // Pin to the merge-base SHA, not the ref. The diff is `${base}...HEAD` (three-dot), whose + // true origin is merge-base(ref, HEAD); returning that immutable SHA means the bytes the + // fingerprint hashes and the bytes the review diffs are ONE snapshot even if `ref` moves + // between the two in-process git calls (and a rebase that shifts the merge-base changes it). + const res = await git(["merge-base", ref, "HEAD"]); const base = res.stdout.trim(); - return base.length > 0 ? base : "HEAD~1"; + if (base.length > 0) { + return base; + } + + // merge-base failed (e.g. no `main`): fall back to the ref itself (or HEAD~1 when omitted). + // Cache safety does not rest on this — an unresolvable diff yields diffHash=null downstream. + return explicit !== undefined && explicit.length > 0 ? explicit : "HEAD~1"; } async function resolveIntent( @@ -128,7 +139,7 @@ export async function gatherChange( deps: IGatherDeps, opts: IGatherOptions ): Promise { - const validateSummary = await deps.validate(); + const validateSummary = opts.validateSummary ?? (await deps.validate()); if (!validateSummary.passed) { return { @@ -348,6 +359,64 @@ export function reviewPlan( }; } +export interface IVerdictDecisionDeps { + /** Read a cached verdict by key (already applies the pre-review guard); null = miss. */ + readCache: (cacheKey: string) => Promise; + /** Run the live panel review for the resolved base/intent, REUSING the given validate + * summary so validate is not run a second time. */ + runReview: ( + base: string, + intent: string | undefined, + validateSummary: IValidateSummary + ) => Promise; + /** Persist a verdict under the key (the real impl guards against caching pre-review blocks). */ + persist: (verdict: IVerdict, cacheKey: string) => Promise; +} + +/** + * Decide the verdict for one review invocation, orchestrating the cache against a FRESH + * validate. Extracted from the CLI so the wiring is unit-testable (the panel required an + * integration test for cache-hit reuse, keyed writes, and the CI write-not-read path). + * + * The cache short-circuits ONLY the expensive model panel — NEVER validate. The caller runs + * validate fresh and passes the summary in; a cached verdict is trusted only when the + * current gate is green (`validateSummary.passed`). This closes the staleness gap the panel + * found: keying purely on the committed `${base}...HEAD` diff (dropping treeHash) removed + * the accidental index-invalidation that used to force validate to re-run, so without a live + * validate check a cached PASS could be reused over a currently-red worktree. Validate is now + * live every time, so the key only needs to identify the reviewed diff. + * + * `--ci` never READS the cache (CI always re-reviews) but still WRITES it, so a later + * interactive run with an identical diff can reuse it. A null `cacheKey` (unfingerprintable + * diff — git failed) neither reads nor writes: there is no sound key. + */ +export async function resolveVerdict( + deps: IVerdictDecisionDeps, + opts: { ci: boolean; plan: IReviewPlan; validateSummary: IValidateSummary } +): Promise<{ verdict: IVerdict; cacheHit: boolean }> { + const { plan } = opts; + + if (!opts.ci && plan.cacheKey !== null && opts.validateSummary.passed) { + const cached = await deps.readCache(plan.cacheKey); + + if (cached !== null) { + return { verdict: cached, cacheHit: true }; + } + } + + const verdict = await deps.runReview( + plan.reviewBase, + plan.reviewIntent, + opts.validateSummary + ); + + if (plan.cacheKey !== null) { + await deps.persist(verdict, plan.cacheKey); + } + + return { verdict, cacheHit: false }; +} + /** The caching decision, isolated so it is unit-testable without the filesystem. * ONLY a real panel verdict is cached. A pre-review gate/precondition block * (validate failed, empty intent, diff too large) is transient — caching one diff --git a/packages/core/tests/reviewers-artifact.test.ts b/packages/core/tests/reviewers-artifact.test.ts index 17d3c4c5..0edba0ca 100644 --- a/packages/core/tests/reviewers-artifact.test.ts +++ b/packages/core/tests/reviewers-artifact.test.ts @@ -6,6 +6,7 @@ import { honorCachedVerdict, resolveReviewInputs, reviewPlan, + resolveVerdict, CACHE_VERSION, } from "../src/reviewers/harness-review"; import { RUBRIC_VERSION } from "../src/reviewers/schema"; @@ -102,9 +103,11 @@ describe("artifact + cache", () => { }; }; - test("resolveReviewInputs hashes the ACTUAL `${base}...HEAD` diff and returns the same base for the review", async () => { - // The invariant that makes key and review inseparable: the hash is taken over exactly - // the range the review will diff, and the SAME base is returned to hand to the review. + test("resolveReviewInputs pins the base to the merge-base SHA and hashes the ACTUAL `${sha}...HEAD` diff (named ref → immutable snapshot)", async () => { + // The invariant that makes key and review inseparable: the named ref `main` is resolved + // to merge-base(main, HEAD) — an immutable SHA — so the range the hash covers and the + // range the review diffs are one snapshot even if `main` moves mid-run, and that SAME + // base is returned to hand to the review. const seen: string[][] = []; const r = await resolveReviewInputs( gitScript({ diffOut: "some diff", seenDiffArgs: seen, subject: "fix" }), @@ -112,8 +115,8 @@ describe("artifact + cache", () => { "fix" ); - expect(seen).toContainEqual(["diff", "main...HEAD"]); - expect(r.base).toBe("main"); + expect(seen).toContainEqual(["diff", "mergebasesha...HEAD"]); + expect(r.base).toBe("mergebasesha"); expect(r.diffHash).toBe( createHash("sha256").update("some diff").digest("hex") ); @@ -226,6 +229,135 @@ describe("artifact + cache", () => { expect(plan.reviewIntent).toBeUndefined(); }); + // A resolveVerdict harness: records which seams fired so the CLI wiring can be asserted + // without spawning the process. runReview returns a distinct verdict so a cache hit + // (which must NOT run the review) is distinguishable from a miss. + const reviewVerdict: IVerdict = { ...v, reason: "fresh review" }; + const cachedVerdict: IVerdict = { ...v, reason: "from cache" }; + + const decisionHarness = (opts: { + cached: IVerdict | null; + passed?: boolean; + }) => { + const calls = { read: 0, review: 0, persist: [] as string[] }; + const deps = { + readCache: async (key: string) => { + calls.read += 1; + void key; + + return opts.cached; + }, + runReview: async () => { + calls.review += 1; + + return reviewVerdict; + }, + persist: async (_verdict: IVerdict, key: string) => { + calls.persist.push(key); + }, + }; + const summary = { + passed: opts.passed ?? true, + failCount: 0, + firstErrors: [], + }; + + return { calls, deps, summary }; + }; + + const planWith = (cacheKey: string | null): IResolvedReviewInputs => + resolvedInputs({ diffHash: cacheKey === null ? null : "d1" }); + + test("resolveVerdict: cache HIT (interactive, keyed, validate green) reuses the cached verdict — no review, no write", async () => { + const { calls, deps, summary } = decisionHarness({ cached: cachedVerdict }); + const plan = reviewPlan(planWith("k"), { quick: false, panelHash: "p1" }); + + const r = await resolveVerdict(deps, { + ci: false, + plan, + validateSummary: summary, + }); + + expect(r.cacheHit).toBe(true); + expect(r.verdict.reason).toBe("from cache"); + expect(calls.review).toBe(0); + expect(calls.persist).toHaveLength(0); + }); + + test("resolveVerdict: cache MISS runs the review and WRITES under the plan's key", async () => { + const { calls, deps, summary } = decisionHarness({ cached: null }); + const plan = reviewPlan(planWith("k"), { quick: false, panelHash: "p1" }); + + const r = await resolveVerdict(deps, { + ci: false, + plan, + validateSummary: summary, + }); + + expect(r.cacheHit).toBe(false); + expect(r.verdict.reason).toBe("fresh review"); + expect(calls.review).toBe(1); + expect(calls.persist).toEqual( + plan.cacheKey === null ? [] : [plan.cacheKey] + ); + }); + + test("resolveVerdict: --ci WRITES but never READS the cache (CI always re-reviews)", async () => { + const { calls, deps, summary } = decisionHarness({ cached: cachedVerdict }); + const plan = reviewPlan(planWith("k"), { quick: false, panelHash: "p1" }); + + const r = await resolveVerdict(deps, { + ci: true, + plan, + validateSummary: summary, + }); + + expect(calls.read).toBe(0); // never reads on CI, even though a cache entry exists + expect(calls.review).toBe(1); + expect(calls.persist).toEqual( + plan.cacheKey === null ? [] : [plan.cacheKey] + ); + expect(r.cacheHit).toBe(false); + }); + + test("resolveVerdict: a RED validate skips the cache read and re-reviews (cache never short-circuits the gate — P4 core)", async () => { + // The staleness fix: with the current worktree failing validate, a cached PASS for the + // same committed diff must NOT be reused. The review is re-run with the failing summary. + const { calls, deps, summary } = decisionHarness({ + cached: cachedVerdict, + passed: false, + }); + const plan = reviewPlan(planWith("k"), { quick: false, panelHash: "p1" }); + + const r = await resolveVerdict(deps, { + ci: false, + plan, + validateSummary: summary, + }); + + expect(calls.read).toBe(0); // validate red → cache not even consulted + expect(calls.review).toBe(1); + expect(r.cacheHit).toBe(false); + }); + + test("resolveVerdict: a null cacheKey (unfingerprintable diff) neither reads nor writes, but still reviews", async () => { + const { calls, deps, summary } = decisionHarness({ cached: cachedVerdict }); + const plan = reviewPlan(planWith(null), { quick: false, panelHash: "p1" }); + + expect(plan.cacheKey).toBeNull(); + + const r = await resolveVerdict(deps, { + ci: false, + plan, + validateSummary: summary, + }); + + expect(calls.read).toBe(0); + expect(calls.persist).toHaveLength(0); + expect(calls.review).toBe(1); + expect(r.verdict.reason).toBe("fresh review"); + }); + test("honorCachedVerdict drops a cached pre-review block, passes a real verdict through", () => { // The read-side defense in depth: a pre-review block that somehow reached disk // must force a fresh live review (null), while a genuine panel verdict is honored. diff --git a/packages/core/tests/reviewers-harness-review.test.ts b/packages/core/tests/reviewers-harness-review.test.ts index 174194e0..d52789d7 100644 --- a/packages/core/tests/reviewers-harness-review.test.ts +++ b/packages/core/tests/reviewers-harness-review.test.ts @@ -95,6 +95,28 @@ describe("gatherChange", () => { } }); + test("reuses a provided validateSummary instead of running validate again (validate runs once per invocation)", async () => { + let validateCalls = 0; + const deps: IGatherDeps = { + git: git({ + "diff --name-only": "x.ts", + diff: "diff --git a/x b/x\n+code", + }), + validate: async () => { + validateCalls += 1; + + return { passed: true, failCount: 0, firstErrors: [] }; + }, + }; + const r = await gatherChange(deps, { + ...opts, + validateSummary: { passed: true, failCount: 0, firstErrors: [] }, + }); + + expect(r.kind).toBe("request"); + expect(validateCalls).toBe(0); // the caller's fresh summary was reused + }); + test("attaches the changed files' current (HEAD) contents as review context", async () => { const deps: IGatherDeps = { git: git({ From b9f4ef65f0371ede093c6e464df2241cf65d5094 Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Tue, 28 Jul 2026 00:21:56 +0200 Subject: [PATCH 05/11] fix(panel): key the verdict cache on a fingerprint of the ACTUAL review request (P4 round-6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel round-5 (full run, 4/4) converged on one root flaw: the key must fingerprint the exact request reviewers judge — diff AND full contextFiles AND the effective roster — computed from the same bytes the review uses. Incremental keys (diffHash, then base, then treeHash) each missed a dimension. Redesign — gather-then-key-then-cache: - gatherChange runs validate FRESH and builds the request first; a cache hit is only reached when the gate currently passes (P4's re-validate requirement, by construction). It now BLOCKS on a git-diff exit≠0 or an empty diff instead of building a vacuous green review (the ignored-exit false-green the panel flagged). - reviewRequestKey hashes the gathered request (diff + contextFiles + intent + rubric) + panelIdentityHash(EFFECTIVE roster + builder) + mode + CACHE_VERSION(→3). The key and the review hash the SAME object — no second git read to diverge from. This kills the contextFiles-omission, effective-vs-cfg-roster, and fingerprint/payload findings. - reviewRequest (review an already-gathered request) + decideVerdict (cache read/write, --ci writes-not-reads) extracted and unit-tested; runHarnessReview reuses them. - resolveBase pins base to merge-base(ref,HEAD); fallback + git-exit guards tested. - removed the as-cast in the test. Full suite 3173/0, typecheck+lint+format clean. --- packages/core/src/cli/harness-review-mode.ts | 111 ++--- packages/core/src/reviewers/harness-review.ts | 315 ++++++-------- .../core/tests/reviewers-artifact.test.ts | 384 +++++------------- .../tests/reviewers-harness-review.test.ts | 97 ++++- 4 files changed, 368 insertions(+), 539 deletions(-) diff --git a/packages/core/src/cli/harness-review-mode.ts b/packages/core/src/cli/harness-review-mode.ts index fb5a13fa..b7fcf224 100644 --- a/packages/core/src/cli/harness-review-mode.ts +++ b/packages/core/src/cli/harness-review-mode.ts @@ -1,6 +1,6 @@ import { mkdir, mkdtemp, readFile, writeFile, rm } from "node:fs/promises"; import { join } from "node:path"; -import { createHash, randomUUID } from "node:crypto"; +import { randomUUID } from "node:crypto"; import { tmpdir } from "node:os"; import { isRecord } from "../lib/guards"; import { OpenAICompatibleProvider, type IProvider } from "../inference"; @@ -13,15 +13,17 @@ import { } from "../models-config"; import { resolvePanel } from "../reviewers/registry"; import { - runHarnessReview, + gatherChange, + reviewRequest, + blockedVerdict, + reviewRequestKey, + panelIdentityHash, + decideVerdict, DEFAULT_MAX_FILES, DEFAULT_MAX_CHARS, artifactBody, shouldCacheVerdict, honorCachedVerdict, - resolveReviewInputs, - reviewPlan, - resolveVerdict, } from "../reviewers/harness-review"; import { parseVerdict, type IVerdict } from "../reviewers/aggregate"; @@ -200,10 +202,6 @@ async function validateRunner(): Promise<{ return { passed: code === 0, failCount: firstErrors.length, firstErrors }; } -function computePanelHash(panel: object): string { - return createHash("sha256").update(JSON.stringify(panel)).digest("hex"); -} - export const CACHE_DIR = join(".tsforge", "harness-review"); async function readCachedVerdict( @@ -303,58 +301,61 @@ export async function harnessReviewMode(argv: string[]): Promise { const treeHashRes = await gitRunner(["write-tree"]); const treeHash = treeHashRes.stdout.trim(); - const panelHash = computePanelHash(cfg.reviewPanel ?? {}); - // Resolve base + intent AND fingerprint the exact reviewed diff BEFORE keying. Keying on - // the raw flags — or even a base SHA — is unsound: a moving `main`, an amended message, - // or a REBASE that shifts merge-base(base, HEAD) all change the reviewed diff - // (`${base}...HEAD`, three-dot) with the flags, tree, and subject unchanged. Hashing the - // diff bytes the reviewers actually see captures every one of those (4-model panel - // finding). The SAME resolved base feeds the review below, so key and review can't diverge. - const resolved = await resolveReviewInputs(gitRunner, args.base, args.intent); - // reviewPlan binds the cache key AND the review request to the SAME resolved inputs — so - // the diff the key fingerprints and the diff the review reads can't diverge, on either the - // --ci or the interactive path (the CI-parity hole the panel found). A null cacheKey means - // the diff was unfingerprintable (git failed) → neither read nor write the cache. - const plan = reviewPlan(resolved, { quick: args.quick, panelHash }); - - // Run validate FRESH, once, before consulting the cache. The verdict cache short-circuits - // only the expensive model panel — never the gate: a cached PASS is trusted only when the - // current worktree is still green (resolveVerdict gates the cache read on this summary), - // and the summary is threaded into the review so validate never runs twice. This is P4's - // core requirement — re-run validate before trusting a cached verdict. - const validateSummary = await validateRunner(); - - const { verdict, cacheHit } = await resolveVerdict( + const identity = `${active.name}/${active.entry.model}`; + + // GATHER the review request FIRST — gatherChange runs validate FRESH (blocking, never + // cached, when the gate is red), resolves the base to an immutable merge-base SHA, and + // builds the exact request (diff + contextFiles) the reviewers will judge. A cache hit is + // therefore only ever reached when the gate currently passes — P4's "re-run validate + // before trusting the cache" requirement, satisfied by construction. + const gathered = await gatherChange( + { git: gitRunner, validate: validateRunner }, { - readCache: readCachedVerdict, - runReview: (base, intent, summary) => - runHarnessReview( - { - git: gitRunner, - validate: validateRunner, + base: args.base, + intent: args.intent, + maxFiles: DEFAULT_MAX_FILES, + maxChars: DEFAULT_MAX_CHARS, + } + ); + + let verdict: IVerdict; + let cacheHit = false; + + if (gathered.kind === "block") { + // A pre-review gate/precondition block — never cached (a transient block must not reject + // every later push). + verdict = blockedVerdict(gathered.reason, identity); + } else { + // Key on a fingerprint of the ACTUAL gathered request (diff + full contextFiles + intent + // + rubric) plus the EFFECTIVE roster (the reviewers actually used, after skips/quick- + // slice/active-model exclusion) and mode. The same request object is handed to the + // review, so key and review hash the same bytes — no second read to diverge from. + const rosterHash = panelIdentityHash(effective, identity); + const key = reviewRequestKey(gathered.request, { + rosterHash, + mode: args.quick ? "quick" : "full", + }); + const decided = await decideVerdict( + { + readCache: readCachedVerdict, + review: () => + reviewRequest(gathered.request, { makeProvider, runBinary, panel: effective, - identity: `${active.name}/${active.entry.model}`, - }, - { - base, - intent, - maxFiles: DEFAULT_MAX_FILES, - maxChars: DEFAULT_MAX_CHARS, - validateSummary: summary, - } - ), - persist: (v, cacheKey) => - persistVerdict(v, cacheKey, treeHash, panelHash), - }, - { ci: args.ci, plan, validateSummary } - ); + identity, + }), + persist: (v, k) => persistVerdict(v, k, treeHash, rosterHash), + }, + { ci: args.ci, key } + ); + + verdict = decided.verdict; + cacheHit = decided.cacheHit; + } if (cacheHit) { - process.stdout.write( - "harness-review: cache hit (validate re-run green), reusing verdict\n" - ); + process.stdout.write("harness-review: cache hit, reusing verdict\n"); } process.stdout.write(`${formatVerdict(verdict)}\n`); diff --git a/packages/core/src/reviewers/harness-review.ts b/packages/core/src/reviewers/harness-review.ts index 82165bd7..3a27dd9e 100644 --- a/packages/core/src/reviewers/harness-review.ts +++ b/packages/core/src/reviewers/harness-review.ts @@ -35,10 +35,6 @@ export interface IGatherOptions { intent?: string; maxFiles: number; maxChars: number; - /** A validate summary already computed by the caller. When present, gatherChange REUSES - * it instead of running validate again — so validate runs exactly once per invocation - * even when the caller re-ran it to decide whether to trust the verdict cache. */ - validateSummary?: IValidateSummary; } export type GatherResult = @@ -79,67 +75,11 @@ async function resolveIntent( return GENERIC_INTENTS.has(subject.toLowerCase()) ? null : subject; } -export interface IResolvedReviewInputs { - /** The base to hand `runHarnessReview` so its diff matches what `diffHash` hashed. */ - base: string; - /** The resolved intent text (commit subject when the flag is omitted), or null when - * empty/generic (the review will block on it). */ - intent: string | null; - /** sha256 of the ACTUAL reviewed diff (`${base}...HEAD`), or null when the diff could - * NOT be computed (git failed). A null hash means the caller MUST skip the cache - * entirely — never key on a movable ref as a fallback (that is the very bug this - * resolver exists to kill). */ - diffHash: string | null; -} - -/** - * Resolve the review's base + intent AND fingerprint the exact diff the reviewers will - * see, so the verdict cache can key on the real review inputs. Keying on the raw flags is - * unsound: an omitted `--base` resolves to `merge-base main HEAD` and a named ref like - * `main` is resolved at review time, so the SAME flags can denote a DIFFERENT diff after - * `main` moves; an omitted `--intent` comes from the commit subject, which an amend - * changes. - * - * Keying on a base SHA is NOT enough either: the diff is `${base}...HEAD` (three-dot, a - * merge-base diff), so a rebase that shifts `merge-base(base, HEAD)` changes the reviewed - * diff while the base ref, the final tree, and the subject all stay identical. The only - * fingerprint that captures every one of those is a hash of the reviewed diff itself — - * exactly the bytes the reviewers judge. The same resolved `base` is handed to the review, - * so the diff the key hashed and the diff the review reads are the same command. - * - * If the diff cannot be computed (git error), `diffHash` is null and the caller skips the - * cache — a failure must never silently fall back to keying on the movable ref. - */ -export async function resolveReviewInputs( - git: IGitRunner, - base: string | undefined, - intent: string | undefined -): Promise { - const baseRef = await resolveBase(git, base); - const intentText = await resolveIntent(git, intent); - const diff = await git(["diff", `${baseRef}...HEAD`]); - const diffHash = - diff.code === 0 - ? createHash("sha256").update(diff.stdout).digest("hex") - : null; - - return { base: baseRef, intent: intentText, diffHash }; -} - -async function changedFiles(git: IGitRunner, base: string): Promise { - const res = await git(["diff", "--name-only", `${base}...HEAD`]); - - return res.stdout - .split("\n") - .map((l) => l.trim()) - .filter((l) => l.length > 0); -} - export async function gatherChange( deps: IGatherDeps, opts: IGatherOptions ): Promise { - const validateSummary = opts.validateSummary ?? (await deps.validate()); + const validateSummary = await deps.validate(); if (!validateSummary.passed) { return { @@ -159,7 +99,29 @@ export async function gatherChange( }; } - const files = await changedFiles(deps.git, base); + // Read the changed-file list AND check git's exit code. Ignoring it lets a failed diff + // (empty stdout on error) build an EMPTY review that the panel green-lights, then caches + // under the real request's key — a false green. A failure blocks instead. + const namesRes = await deps.git(["diff", "--name-only", `${base}...HEAD`]); + + if (namesRes.code !== 0) { + return { + kind: "block", + reason: `could not compute the changed-file list (git diff exited ${String(namesRes.code)}) — cannot review`, + }; + } + + const files = namesRes.stdout + .split("\n") + .map((l) => l.trim()) + .filter((l) => l.length > 0); + + if (files.length === 0) { + return { + kind: "block", + reason: `no changes between ${base} and HEAD to review`, + }; + } if (files.length > opts.maxFiles) { return { @@ -168,7 +130,16 @@ export async function gatherChange( }; } - const diff = (await deps.git(["diff", `${base}...HEAD`])).stdout; + const diffRes = await deps.git(["diff", `${base}...HEAD`]); + + if (diffRes.code !== 0) { + return { + kind: "block", + reason: `could not compute the diff (git diff exited ${String(diffRes.code)}) — cannot review`, + }; + } + + const diff = diffRes.stdout; if (diff.length > opts.maxChars) { return { @@ -238,7 +209,11 @@ export interface IRunDeps extends IGatherDeps, IInvokeDeps { identity: string; } -function blockedVerdict(reason: string, identity: string): IVerdict { +/** A pre-review gate/precondition block (validate red, empty intent, diff too large, + * git failure). The panel did NOT run. Marked `preReview` so it is never cached — a + * transient block must not poison the cache and reject every later push. Exported so the + * CLI produces it directly (it gathers the request itself, then reviews). */ +export function blockedVerdict(reason: string, identity: string): IVerdict { return { blocked: true, reason, @@ -246,13 +221,34 @@ function blockedVerdict(reason: string, identity: string): IVerdict { ranked: [], perReviewer: [], identity, - // Pre-review gate/precondition block — the panel did not run. Marked so the - // caller never caches it (a transient validate flake must not poison the - // tree-hash and block every later push). preReview: true, }; } +export interface IReviewDeps extends IInvokeDeps { + panel: IPanel; + identity: string; +} + +/** Run the panel on an ALREADY-GATHERED request (validate + diff + context already built). + * Split out from runHarnessReview so the CLI can fingerprint the exact request for the + * cache key BEFORE deciding whether to invoke the models — key and review then hash the + * same bytes by construction. */ +export async function reviewRequest( + request: IReviewRequest, + deps: IReviewDeps +): Promise { + const outcomes = await reviewerInvoke(deps.panel, request, { + makeProvider: deps.makeProvider, + runBinary: deps.runBinary, + }); + + return aggregate(outcomes, { + minReviewers: deps.panel.minReviewers, + identity: deps.identity, + }); +} + export async function runHarnessReview( deps: IRunDeps, opts: IGatherOptions @@ -263,156 +259,101 @@ export async function runHarnessReview( return blockedVerdict(gathered.reason, deps.identity); } - const outcomes = await reviewerInvoke(deps.panel, gathered.request, { - makeProvider: deps.makeProvider, - runBinary: deps.runBinary, - }); - - return aggregate(outcomes, { - minReviewers: deps.panel.minReviewers, - identity: deps.identity, - }); + return reviewRequest(gathered.request, deps); } /** Verdict-cache schema version. Bump to invalidate ALL previously written cache - * artifacts in one shot. Bumped to "2" when pre-review gate blocks stopped being - * cached: legacy "1" artifacts can hold a poisoned "validate failed" block with no - * `preReview` marker, and without a version change readCachedVerdict would keep - * serving them and block every push of that tree. */ -export const CACHE_VERSION = "2"; - -export function verdictCacheKey(input: { - /** sha256 of the ACTUAL reviewed diff (`${base}...HEAD`, three-dot). This is what the - * reviewers see, so it captures the true merge-base: a moved base ref, a rebase that - * shifts the merge-base, or any tree change all produce a different diff → a different - * key. (Keying on a base SHA alone cannot — a rebase moves the merge-base with the ref - * and tree unchanged.) */ - diffHash: string; - panelHash: string; - rubricVersion: string; - cacheVersion: string; - /** The resolved review intent (commit subject when the flag is omitted) — different - * context ⇒ a different review; an amend that changes the subject changes this. */ - intent: string; - /** "quick" | "full". `quick` reviews with a REDUCED roster (1 reviewer); its verdict - * must never satisfy a full review, and vice versa. */ - mode: string; -}): string { - // JSON-serialize the fields (unforgeable: escapes + array delimiting) rather than a - // space-join — a value containing a space (e.g. an intent string) could otherwise slide - // across the boundary and forge a key collision, reusing a verdict for a different - // request. Same lesson as the db:push fingerprint. + * artifacts in one shot. Bumped to "3" when the key changed from a diff-hash to a full + * request fingerprint (below): legacy artifacts key on incomparable inputs, so the bump + * retires them rather than risk a stale-input collision. */ +export const CACHE_VERSION = "3"; + +/** + * Fingerprint the EXACT review request the reviewers will judge — the diff AND the full + * `contextFiles` (the changed files' HEAD contents) AND the intent AND the rubric — plus + * the roster identity and mode. This is the cache key. + * + * Keying on a diff hash alone is unsound: reviewers also see `contextFiles`, so a rebase + * onto a different base can yield identical patch bytes while the surrounding file contents + * (and thus the review input) differ — a false reuse. Hashing the request object itself, + * the same object handed to `reviewRequest`, makes the key and the review provably one + * input: there is no second git read to diverge from. + */ +export function reviewRequestKey( + request: IReviewRequest, + opts: { rosterHash: string; mode: string } +): string { + // JSON array (unforgeable: escaped + delimited) over the reviewer-visible content. The + // validate summary is NOT keyed — validate is re-run fresh every invocation (a request + // only exists when it currently passes), so it is a live precondition, not a cache axis. return createHash("sha256") .update( JSON.stringify([ - input.diffHash, - input.panelHash, - input.rubricVersion, - input.cacheVersion, - input.intent, - input.mode, + request.diff, + request.contextFiles ?? [], + request.intent, + request.rubricVersion, + opts.rosterHash, + opts.mode, + CACHE_VERSION, ]) ) .digest("hex"); } -export interface IReviewPlan { - /** Cache key, or null when the diff was unfingerprintable (git failed) — a null key means - * the caller must NEITHER read nor write the cache (no sound key exists). */ - cacheKey: string | null; - /** Base to hand `runHarnessReview` — identical to the base the cache key's diff was taken - * against, so the review reads exactly the diff the key fingerprinted. */ - reviewBase: string; - /** Intent to hand `runHarnessReview` — the same resolved intent the key used. */ - reviewIntent: string | undefined; -} - /** - * Bind the resolved review inputs to BOTH the cache key and the review request from a - * SINGLE source, so the diff the key fingerprints and the diff the review reads can never - * diverge. This closes the CI-parity hole the panel found: previously the `--ci` path - * reviewed the raw flags while the key was built from the resolved values, so a verdict - * could be stored under one request's key for another request's diff. Now both paths take - * `reviewBase`/`reviewIntent` from here (the `--ci` path simply never reads the cache). - * Pure and injectable so this wiring is unit-tested without spawning the CLI. + * Identity of the roster that ACTUALLY reviewed — the resolved reviewer ids (after skips, + * quick-slicing, and active-model exclusion), the quorum, and the builder. A verdict from a + * different roster (a reviewer added/dropped, a different builder whose independence differs) + * must not be reused, so this feeds the cache key. Keying on the raw config instead would + * miss all of those, which the panel flagged. */ -export function reviewPlan( - resolved: IResolvedReviewInputs, - opts: { quick: boolean; panelHash: string } -): IReviewPlan { - const cacheKey = - resolved.diffHash === null - ? null - : verdictCacheKey({ - diffHash: resolved.diffHash, - panelHash: opts.panelHash, - rubricVersion: RUBRIC_VERSION, - cacheVersion: CACHE_VERSION, - intent: resolved.intent ?? "", - mode: opts.quick ? "quick" : "full", - }); +export function panelIdentityHash( + panel: { reviewers: readonly { id: string }[]; minReviewers: number }, + builderIdentity: string +): string { + const roster = panel.reviewers.map((r) => r.id).sort(); - return { - cacheKey, - reviewBase: resolved.base, - reviewIntent: resolved.intent ?? undefined, - }; + return createHash("sha256") + .update(JSON.stringify([roster, panel.minReviewers, builderIdentity])) + .digest("hex"); } -export interface IVerdictDecisionDeps { +export interface IDecideDeps { /** Read a cached verdict by key (already applies the pre-review guard); null = miss. */ - readCache: (cacheKey: string) => Promise; - /** Run the live panel review for the resolved base/intent, REUSING the given validate - * summary so validate is not run a second time. */ - runReview: ( - base: string, - intent: string | undefined, - validateSummary: IValidateSummary - ) => Promise; - /** Persist a verdict under the key (the real impl guards against caching pre-review blocks). */ - persist: (verdict: IVerdict, cacheKey: string) => Promise; + readCache: (key: string) => Promise; + /** Run the live panel (only reached on a miss or --ci). */ + review: () => Promise; + /** Persist the verdict under the key (the real impl guards against caching a pre-review block). */ + persist: (verdict: IVerdict, key: string) => Promise; } /** - * Decide the verdict for one review invocation, orchestrating the cache against a FRESH - * validate. Extracted from the CLI so the wiring is unit-testable (the panel required an - * integration test for cache-hit reuse, keyed writes, and the CI write-not-read path). - * - * The cache short-circuits ONLY the expensive model panel — NEVER validate. The caller runs - * validate fresh and passes the summary in; a cached verdict is trusted only when the - * current gate is green (`validateSummary.passed`). This closes the staleness gap the panel - * found: keying purely on the committed `${base}...HEAD` diff (dropping treeHash) removed - * the accidental index-invalidation that used to force validate to re-run, so without a live - * validate check a cached PASS could be reused over a currently-red worktree. Validate is now - * live every time, so the key only needs to identify the reviewed diff. + * Decide a verdict for an already-gathered, already-keyed request: reuse the cache when + * possible, else run the panel and persist. Extracted from the CLI so the wiring is + * unit-testable (the panel required proof that a cache hit is reused, a miss writes under + * the key, and --ci writes but never reads). * - * `--ci` never READS the cache (CI always re-reviews) but still WRITES it, so a later - * interactive run with an identical diff can reuse it. A null `cacheKey` (unfingerprintable - * diff — git failed) neither reads nor writes: there is no sound key. + * Validate freshness is NOT this function's concern: the caller gathers the request first + * (gatherChange runs validate fresh and blocks — never reaching here — when the gate is red), + * so a cache hit implies the gate currently passes. --ci never READS the cache (CI always + * re-reviews) but still WRITES it, seeding a later interactive run with the identical request. */ -export async function resolveVerdict( - deps: IVerdictDecisionDeps, - opts: { ci: boolean; plan: IReviewPlan; validateSummary: IValidateSummary } +export async function decideVerdict( + deps: IDecideDeps, + opts: { ci: boolean; key: string } ): Promise<{ verdict: IVerdict; cacheHit: boolean }> { - const { plan } = opts; - - if (!opts.ci && plan.cacheKey !== null && opts.validateSummary.passed) { - const cached = await deps.readCache(plan.cacheKey); + if (!opts.ci) { + const cached = await deps.readCache(opts.key); if (cached !== null) { return { verdict: cached, cacheHit: true }; } } - const verdict = await deps.runReview( - plan.reviewBase, - plan.reviewIntent, - opts.validateSummary - ); + const verdict = await deps.review(); - if (plan.cacheKey !== null) { - await deps.persist(verdict, plan.cacheKey); - } + await deps.persist(verdict, opts.key); return { verdict, cacheHit: false }; } diff --git a/packages/core/tests/reviewers-artifact.test.ts b/packages/core/tests/reviewers-artifact.test.ts index 0edba0ca..eae4ceec 100644 --- a/packages/core/tests/reviewers-artifact.test.ts +++ b/packages/core/tests/reviewers-artifact.test.ts @@ -1,19 +1,12 @@ import { test, expect, describe } from "bun:test"; -import { createHash } from "node:crypto"; import { - verdictCacheKey, + reviewRequestKey, + panelIdentityHash, + decideVerdict, artifactBody, honorCachedVerdict, - resolveReviewInputs, - reviewPlan, - resolveVerdict, - CACHE_VERSION, -} from "../src/reviewers/harness-review"; -import { RUBRIC_VERSION } from "../src/reviewers/schema"; -import type { - IGitRunner, - IResolvedReviewInputs, } from "../src/reviewers/harness-review"; +import type { IReviewRequest } from "../src/reviewers/schema"; import type { IVerdict } from "../src/reviewers/aggregate"; const v: IVerdict = { @@ -25,229 +18,121 @@ const v: IVerdict = { identity: "local/flash", }; -describe("artifact + cache", () => { - const key = { - diffHash: "d1", - panelHash: "p1", - rubricVersion: "1", - cacheVersion: "2", - intent: "fix the widget", - mode: "full", - }; - - test("cache key is stable for the same inputs and changes with the reviewed-diff hash", () => { - expect(verdictCacheKey({ ...key })).toBe(verdictCacheKey({ ...key })); - expect(verdictCacheKey({ ...key })).not.toBe( - verdictCacheKey({ ...key, diffHash: "d2" }) - ); - }); - - test("cacheVersion is mixed into the key — bumping it retires ALL legacy artifacts", () => { - // CACHE_VERSION is the ONLY thing that retires already-on-disk poisoned v1 blocks - // (they carry no preReview flag, so the read-side guard can't reject them). If a - // regression dropped it from the hash, legacy poison would be re-served and every - // other test would still pass — so pin it here. - expect(verdictCacheKey({ ...key, cacheVersion: "1" })).not.toBe( - verdictCacheKey({ ...key, cacheVersion: "2" }) - ); - }); +const request: IReviewRequest = { + title: "add the widget", + intent: "add the widget", + diff: "diff --git a/x b/x\n+code", + validateSummary: { passed: true, failCount: 0, firstErrors: [] }, + contextFiles: ["=== x.ts ===\nexport const x = 1;"], + rubricVersion: "1", +}; - test("diffHash, intent, and mode each change the key — a verdict can't be reused across a different review request (P4)", () => { - // The whole request identity keys the cache: a different reviewed diff (different - // diffHash), a different intent (different context), or quick vs full (reduced roster) - // MUST miss the cache and force a fresh review. - expect(verdictCacheKey({ ...key })).not.toBe( - verdictCacheKey({ ...key, diffHash: "other-diff" }) - ); - expect(verdictCacheKey({ ...key })).not.toBe( - verdictCacheKey({ ...key, intent: "something else" }) - ); - expect(verdictCacheKey({ ...key, mode: "quick" })).not.toBe( - verdictCacheKey({ ...key, mode: "full" }) - ); - }); +const rosterOpts = { rosterHash: "r1", mode: "full" }; - test("unforgeable key: a space slid between fields can't collide two distinct requests", () => { - // A space-join would make (diffHash 'a', intent 'b c') and (diffHash 'a b', intent 'c') - // collide; the JSON serialization keeps them distinct. - expect(verdictCacheKey({ ...key, diffHash: "a", intent: "b c" })).not.toBe( - verdictCacheKey({ ...key, diffHash: "a b", intent: "c" }) +describe("reviewRequestKey (cache key = fingerprint of the ACTUAL review request)", () => { + test("stable for the same request + roster + mode", () => { + expect(reviewRequestKey(request, rosterOpts)).toBe( + reviewRequestKey(request, rosterOpts) ); }); - // A git runner scripted per command. `diff` returns `diffOut` (with `diffCode`), the - // commit subject is `subject`. It also RECORDS the argv of every `git diff` it served so - // a test can assert WHICH range was hashed. - const gitScript = (opts: { - diffOut: string; - diffCode?: number; - subject?: string; - seenDiffArgs?: string[][]; - }): IGitRunner => { - return async (argv) => { - if (argv[0] === "diff") { - opts.seenDiffArgs?.push(argv); - - return { code: opts.diffCode ?? 0, stdout: opts.diffOut }; - } - - if (argv[0] === "merge-base") { - return { code: 0, stdout: "mergebasesha\n" }; - } - - if (argv[0] === "log") { - return { code: 0, stdout: `${opts.subject ?? "a subject"}\n` }; - } - - return { code: 0, stdout: "" }; - }; - }; - - test("resolveReviewInputs pins the base to the merge-base SHA and hashes the ACTUAL `${sha}...HEAD` diff (named ref → immutable snapshot)", async () => { - // The invariant that makes key and review inseparable: the named ref `main` is resolved - // to merge-base(main, HEAD) — an immutable SHA — so the range the hash covers and the - // range the review diffs are one snapshot even if `main` moves mid-run, and that SAME - // base is returned to hand to the review. - const seen: string[][] = []; - const r = await resolveReviewInputs( - gitScript({ diffOut: "some diff", seenDiffArgs: seen, subject: "fix" }), - "main", - "fix" - ); + test("every reviewer-visible dimension changes the key — diff, contextFiles, intent, rubric, roster, mode", () => { + const base = reviewRequestKey(request, rosterOpts); - expect(seen).toContainEqual(["diff", "mergebasesha...HEAD"]); - expect(r.base).toBe("mergebasesha"); - expect(r.diffHash).toBe( - createHash("sha256").update("some diff").digest("hex") - ); + expect( + reviewRequestKey({ ...request, diff: "different" }, rosterOpts) + ).not.toBe(base); + // contextFiles: a rebase can yield an identical diff but different surrounding file + // content — the reviewers see this, so it MUST change the key. + expect( + reviewRequestKey( + { ...request, contextFiles: ["=== x.ts ===\nother"] }, + rosterOpts + ) + ).not.toBe(base); + expect( + reviewRequestKey({ ...request, intent: "different" }, rosterOpts) + ).not.toBe(base); + expect( + reviewRequestKey({ ...request, rubricVersion: "2" }, rosterOpts) + ).not.toBe(base); + expect( + reviewRequestKey(request, { ...rosterOpts, rosterHash: "r2" }) + ).not.toBe(base); + expect( + reviewRequestKey(request, { ...rosterOpts, mode: "quick" }) + ).not.toBe(base); }); - test("resolveReviewInputs: a rebase that changes the diff — same base ref, same subject, same tree — changes the key (P4 three-dot merge-base finding)", async () => { - // `--base main` with `main`, the subject, and the final tree ALL unchanged, but a - // rebase shifted merge-base(main, HEAD) so `main...HEAD` now yields a DIFFERENT diff. - // Keying on a base sha would collide here; hashing the diff bytes does not. - const before = await resolveReviewInputs( - gitScript({ diffOut: "diff BEFORE rebase", subject: "fix" }), - "main", - "fix" - ); - const after = await resolveReviewInputs( - gitScript({ diffOut: "diff AFTER rebase", subject: "fix" }), - "main", - "fix" + test("unforgeable: a value sliding across a field boundary can't collide two distinct requests", () => { + // JSON serialization keeps ('a','b c') distinct from ('a b','c'). + expect( + reviewRequestKey({ ...request, diff: "a", intent: "b c" }, rosterOpts) + ).not.toBe( + reviewRequestKey({ ...request, diff: "a b", intent: "c" }, rosterOpts) ); - - // Same named ref, but the reviewed diff (and thus its fingerprint) differs — which the - // "diffHash changes the key" test above proves yields a different cache key. - expect(before.base).toBe(after.base); - expect(before.diffHash).not.toBeNull(); - expect(before.diffHash).not.toBe(after.diffHash); }); - test("resolveReviewInputs: a git diff failure yields diffHash null (caller MUST skip the cache, never fall back to the movable ref)", async () => { - const r = await resolveReviewInputs( - gitScript({ diffOut: "", diffCode: 128, subject: "fix" }), - "main", - "fix" - ); + test("a missing contextFiles hashes the same as an explicit empty list (no undefined/[] ambiguity)", () => { + const { contextFiles: _drop, ...noCtx } = request; - expect(r.diffHash).toBeNull(); - }); - - test("resolveReviewInputs: omitted intent falls back to the commit subject (so an amend changes the key)", async () => { - const a = await resolveReviewInputs( - gitScript({ diffOut: "d", subject: "first subject" }), - undefined, - undefined - ); - const b = await resolveReviewInputs( - gitScript({ diffOut: "d", subject: "amended subject" }), - undefined, - undefined + expect(reviewRequestKey(noCtx, rosterOpts)).toBe( + reviewRequestKey({ ...noCtx, contextFiles: [] }, rosterOpts) ); - - expect(a.intent).toBe("first subject"); - expect(b.intent).toBe("amended subject"); - expect(a.intent).not.toBe(b.intent); - }); - - const resolvedInputs = ( - over: Partial - ): IResolvedReviewInputs => ({ - base: "main", - intent: "fix the widget", - diffHash: "d1", - ...over, }); +}); - test("reviewPlan binds the cache key AND the review request to the SAME resolved inputs (the review can't diverge from the key — CI-parity)", () => { - // The wiring guarantee the panel demanded: the base/intent the key fingerprints are the - // exact base/intent handed to the review. There is no separate raw-flag path. - const resolved = resolvedInputs({ base: "abc123", intent: "do a thing" }); - const plan = reviewPlan(resolved, { quick: false, panelHash: "p1" }); +describe("panelIdentityHash (the roster that ACTUALLY reviewed keys the cache)", () => { + const panel = { + reviewers: [{ id: "grok" }, { id: "codex" }], + minReviewers: 2, + }; - expect(plan.reviewBase).toBe(resolved.base); - expect(plan.reviewIntent).toBe(resolved.intent ?? undefined); - // The key is the same one verdictCacheKey would produce for these resolved inputs. - expect(plan.cacheKey).toBe( - verdictCacheKey({ - diffHash: "d1", - panelHash: "p1", - rubricVersion: RUBRIC_VERSION, - cacheVersion: CACHE_VERSION, - intent: "do a thing", - mode: "full", - }) + test("stable and order-independent (roster is sorted before hashing)", () => { + expect(panelIdentityHash(panel, "local/flash")).toBe( + panelIdentityHash( + { reviewers: [{ id: "codex" }, { id: "grok" }], minReviewers: 2 }, + "local/flash" + ) ); }); - test("reviewPlan: quick mode keys and reviews as 'quick' (reduced roster can't satisfy a full review)", () => { - const resolved = resolvedInputs({}); - const full = reviewPlan(resolved, { quick: false, panelHash: "p1" }); - const quick = reviewPlan(resolved, { quick: true, panelHash: "p1" }); + test("adding/dropping a reviewer, changing the quorum, or changing the builder all change the key", () => { + const base = panelIdentityHash(panel, "local/flash"); - expect(quick.cacheKey).not.toBe(full.cacheKey); - }); - - test("reviewPlan: a null diffHash yields a null cacheKey — caller neither reads nor writes the cache (git-failure fail-safe), but STILL reviews (base/intent set)", () => { - const plan = reviewPlan(resolvedInputs({ diffHash: null }), { - quick: false, - panelHash: "p1", - }); - - expect(plan.cacheKey).toBeNull(); - expect(plan.reviewBase).toBe("main"); - expect(plan.reviewIntent).toBe("fix the widget"); - }); - - test("reviewPlan: a null intent is passed to the review as undefined (not the string 'null')", () => { - const plan = reviewPlan(resolvedInputs({ intent: null }), { - quick: false, - panelHash: "p1", - }); - - expect(plan.reviewIntent).toBeUndefined(); + expect( + panelIdentityHash( + { reviewers: [{ id: "grok" }], minReviewers: 2 }, + "local/flash" + ) + ).not.toBe(base); // a dropped reviewer (effective roster ≠ configured) must not reuse + expect( + panelIdentityHash({ ...panel, minReviewers: 1 }, "local/flash") + ).not.toBe(base); + expect(panelIdentityHash(panel, "other/model")).not.toBe(base); // different builder }); +}); - // A resolveVerdict harness: records which seams fired so the CLI wiring can be asserted - // without spawning the process. runReview returns a distinct verdict so a cache hit - // (which must NOT run the review) is distinguishable from a miss. +describe("decideVerdict (cache orchestration for an already-gathered, already-keyed request)", () => { const reviewVerdict: IVerdict = { ...v, reason: "fresh review" }; const cachedVerdict: IVerdict = { ...v, reason: "from cache" }; - const decisionHarness = (opts: { - cached: IVerdict | null; - passed?: boolean; - }) => { - const calls = { read: 0, review: 0, persist: [] as string[] }; + interface ICalls { + read: number; + review: number; + persist: string[]; + } + + const harness = (cached: IVerdict | null) => { + const calls: ICalls = { read: 0, review: 0, persist: [] }; const deps = { readCache: async (key: string) => { calls.read += 1; void key; - return opts.cached; + return cached; }, - runReview: async () => { + review: async () => { calls.review += 1; return reviewVerdict; @@ -256,27 +141,14 @@ describe("artifact + cache", () => { calls.persist.push(key); }, }; - const summary = { - passed: opts.passed ?? true, - failCount: 0, - firstErrors: [], - }; - return { calls, deps, summary }; + return { calls, deps }; }; - const planWith = (cacheKey: string | null): IResolvedReviewInputs => - resolvedInputs({ diffHash: cacheKey === null ? null : "d1" }); + test("cache HIT (interactive) reuses the cached verdict — no review, no write", async () => { + const { calls, deps } = harness(cachedVerdict); - test("resolveVerdict: cache HIT (interactive, keyed, validate green) reuses the cached verdict — no review, no write", async () => { - const { calls, deps, summary } = decisionHarness({ cached: cachedVerdict }); - const plan = reviewPlan(planWith("k"), { quick: false, panelHash: "p1" }); - - const r = await resolveVerdict(deps, { - ci: false, - plan, - validateSummary: summary, - }); + const r = await decideVerdict(deps, { ci: false, key: "k" }); expect(r.cacheHit).toBe(true); expect(r.verdict.reason).toBe("from cache"); @@ -284,83 +156,31 @@ describe("artifact + cache", () => { expect(calls.persist).toHaveLength(0); }); - test("resolveVerdict: cache MISS runs the review and WRITES under the plan's key", async () => { - const { calls, deps, summary } = decisionHarness({ cached: null }); - const plan = reviewPlan(planWith("k"), { quick: false, panelHash: "p1" }); + test("cache MISS runs the review and WRITES under the SAME key", async () => { + const { calls, deps } = harness(null); - const r = await resolveVerdict(deps, { - ci: false, - plan, - validateSummary: summary, - }); + const r = await decideVerdict(deps, { ci: false, key: "k" }); expect(r.cacheHit).toBe(false); expect(r.verdict.reason).toBe("fresh review"); expect(calls.review).toBe(1); - expect(calls.persist).toEqual( - plan.cacheKey === null ? [] : [plan.cacheKey] - ); + expect(calls.persist).toEqual(["k"]); }); - test("resolveVerdict: --ci WRITES but never READS the cache (CI always re-reviews)", async () => { - const { calls, deps, summary } = decisionHarness({ cached: cachedVerdict }); - const plan = reviewPlan(planWith("k"), { quick: false, panelHash: "p1" }); + test("--ci WRITES but never READS the cache (CI always re-reviews)", async () => { + const { calls, deps } = harness(cachedVerdict); - const r = await resolveVerdict(deps, { - ci: true, - plan, - validateSummary: summary, - }); + const r = await decideVerdict(deps, { ci: true, key: "k" }); expect(calls.read).toBe(0); // never reads on CI, even though a cache entry exists expect(calls.review).toBe(1); - expect(calls.persist).toEqual( - plan.cacheKey === null ? [] : [plan.cacheKey] - ); - expect(r.cacheHit).toBe(false); - }); - - test("resolveVerdict: a RED validate skips the cache read and re-reviews (cache never short-circuits the gate — P4 core)", async () => { - // The staleness fix: with the current worktree failing validate, a cached PASS for the - // same committed diff must NOT be reused. The review is re-run with the failing summary. - const { calls, deps, summary } = decisionHarness({ - cached: cachedVerdict, - passed: false, - }); - const plan = reviewPlan(planWith("k"), { quick: false, panelHash: "p1" }); - - const r = await resolveVerdict(deps, { - ci: false, - plan, - validateSummary: summary, - }); - - expect(calls.read).toBe(0); // validate red → cache not even consulted - expect(calls.review).toBe(1); + expect(calls.persist).toEqual(["k"]); expect(r.cacheHit).toBe(false); }); +}); - test("resolveVerdict: a null cacheKey (unfingerprintable diff) neither reads nor writes, but still reviews", async () => { - const { calls, deps, summary } = decisionHarness({ cached: cachedVerdict }); - const plan = reviewPlan(planWith(null), { quick: false, panelHash: "p1" }); - - expect(plan.cacheKey).toBeNull(); - - const r = await resolveVerdict(deps, { - ci: false, - plan, - validateSummary: summary, - }); - - expect(calls.read).toBe(0); - expect(calls.persist).toHaveLength(0); - expect(calls.review).toBe(1); - expect(r.verdict.reason).toBe("fresh review"); - }); - +describe("read-side + artifact", () => { test("honorCachedVerdict drops a cached pre-review block, passes a real verdict through", () => { - // The read-side defense in depth: a pre-review block that somehow reached disk - // must force a fresh live review (null), while a genuine panel verdict is honored. expect(honorCachedVerdict(null)).toBeNull(); expect( honorCachedVerdict({ ...v, blocked: true, preReview: true }) diff --git a/packages/core/tests/reviewers-harness-review.test.ts b/packages/core/tests/reviewers-harness-review.test.ts index d52789d7..6b5c3009 100644 --- a/packages/core/tests/reviewers-harness-review.test.ts +++ b/packages/core/tests/reviewers-harness-review.test.ts @@ -95,26 +95,93 @@ describe("gatherChange", () => { } }); - test("reuses a provided validateSummary instead of running validate again (validate runs once per invocation)", async () => { - let validateCalls = 0; + test("a failing `git diff --name-only` BLOCKS instead of building an empty review (exit code honored)", async () => { + // The false-green the panel flagged: if git errors and returns empty stdout, an + // unguarded gather would build a 0-file, empty-diff request that the panel green-lights + // and caches. Honoring the exit code turns that into a block. + const failingGit: IGatherDeps["git"] = async (args) => + args.includes("--name-only") + ? { stdout: "", code: 128 } + : { stdout: "", code: 0 }; + const r = await gatherChange( + { git: failingGit, validate: cleanValidate }, + opts + ); + + expect(r.kind).toBe("block"); + + if (r.kind === "block") { + expect(r.reason).toMatch(/could not compute the changed-file list/iu); + } + }); + + test("a failing `git diff` (content) BLOCKS even when the name list succeeded", async () => { + const git2: IGatherDeps["git"] = async (args) => { + if (args.includes("--name-only")) { + return { stdout: "x.ts", code: 0 }; + } + + if (args[0] === "diff") { + return { stdout: "", code: 129 }; // the content diff fails + } + + return { stdout: "", code: 0 }; + }; + + const r = await gatherChange({ git: git2, validate: cleanValidate }, opts); + + expect(r.kind).toBe("block"); + + if (r.kind === "block") { + expect(r.reason).toMatch(/could not compute the diff/iu); + } + }); + + test("no changed files between base and HEAD → block (nothing to review, never a vacuous green)", async () => { const deps: IGatherDeps = { - git: git({ - "diff --name-only": "x.ts", - diff: "diff --git a/x b/x\n+code", - }), - validate: async () => { - validateCalls += 1; + git: git({ "diff --name-only": "", diff: "" }), + validate: cleanValidate, + }; + const r = await gatherChange(deps, opts); - return { passed: true, failCount: 0, firstErrors: [] }; - }, + expect(r.kind).toBe("block"); + + if (r.kind === "block") { + expect(r.reason).toMatch(/no changes/iu); + } + }); + + test("resolveBase merge-base failure falls back to the ref and still diffs (shallow/odd repos)", async () => { + // merge-base returns empty (failure); gather must still resolve a range and produce a + // request rather than throw. Records the range actually diffed. + const seen: string[] = []; + + const fallbackGit: IGatherDeps["git"] = async (args) => { + const key = args.join(" "); + + if (args[0] === "merge-base") { + return { stdout: "", code: 1 }; // no merge-base + } + + if (args[0] === "diff") { + seen.push(key); + + return key.includes("--name-only") + ? { stdout: "x.ts", code: 0 } + : { stdout: "diff --git a/x b/x\n+code", code: 0 }; + } + + return { stdout: "", code: 0 }; }; - const r = await gatherChange(deps, { - ...opts, - validateSummary: { passed: true, failCount: 0, firstErrors: [] }, - }); + + const r = await gatherChange( + { git: fallbackGit, validate: cleanValidate }, + { ...opts, base: "featureX" } + ); expect(r.kind).toBe("request"); - expect(validateCalls).toBe(0); // the caller's fresh summary was reused + // explicit ref given → falls back to that ref (not HEAD~1) for the diff range + expect(seen.some((k) => k.includes("featureX...HEAD"))).toBe(true); }); test("attaches the changed files' current (HEAD) contents as review context", async () => { From 91663bed1bbf9540d63c9273bd65831c919a72e8 Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Tue, 28 Jul 2026 00:33:42 +0200 Subject: [PATCH 06/11] fix(panel): hash the WHOLE review request as the cache key + guard empty-content diff (P4 round-7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-6 findings (all real): - reviewRequestKey omitted validateSummary though it IS in the request reviewers read (validateRunner can return passed:true with non-empty firstErrors) → false reuse across differing diagnostics. Now hashes the ENTIRE request object (same bytes reviewRequest gets), no field selection — key ≡ request literally. - gatherChange only blocked an empty FILE LIST, not an empty CONTENT diff (files listed, git diff exit 0, empty stdout — rename/mode-only). Now blocks the empty diff too. - restored the CACHE_VERSION-in-key pin test (deleted in round-6) via a recompute-mirror. - fixed a stale resolveBase comment referencing the removed diffHash. Full suite 3174/0, typecheck+lint+format clean. --- packages/core/src/reviewers/harness-review.ts | 48 +++++++++-------- .../core/tests/reviewers-artifact.test.ts | 54 ++++++++++++++++--- .../tests/reviewers-harness-review.test.ts | 24 +++++++++ 3 files changed, 99 insertions(+), 27 deletions(-) diff --git a/packages/core/src/reviewers/harness-review.ts b/packages/core/src/reviewers/harness-review.ts index 3a27dd9e..a4f3ce28 100644 --- a/packages/core/src/reviewers/harness-review.ts +++ b/packages/core/src/reviewers/harness-review.ts @@ -58,7 +58,9 @@ async function resolveBase( } // merge-base failed (e.g. no `main`): fall back to the ref itself (or HEAD~1 when omitted). - // Cache safety does not rest on this — an unresolvable diff yields diffHash=null downstream. + // Cache safety does not rest on this — if the resulting `${base}...HEAD` diff can't be + // computed, gatherChange blocks (git-exit / empty-diff guards) rather than caching a + // vacuous review. return explicit !== undefined && explicit.length > 0 ? explicit : "HEAD~1"; } @@ -141,6 +143,16 @@ export async function gatherChange( const diff = diffRes.stdout; + if (diff.length === 0) { + // Files are listed but the CONTENT diff is empty (a rename- or mode-only change, or a + // git quirk): there is nothing textual for the reviewers to judge, so a PASS would be + // vacuous. Block rather than build+cache an empty review. + return { + kind: "block", + reason: `the diff between ${base} and HEAD is empty (rename/mode-only change?) — nothing to review`, + }; + } + if (diff.length > opts.maxChars) { return { kind: "block", @@ -269,34 +281,28 @@ export async function runHarnessReview( export const CACHE_VERSION = "3"; /** - * Fingerprint the EXACT review request the reviewers will judge — the diff AND the full - * `contextFiles` (the changed files' HEAD contents) AND the intent AND the rubric — plus - * the roster identity and mode. This is the cache key. + * Fingerprint the EXACT review request the reviewers will judge, plus the roster identity + * and mode. This is the cache key. * - * Keying on a diff hash alone is unsound: reviewers also see `contextFiles`, so a rebase - * onto a different base can yield identical patch bytes while the surrounding file contents - * (and thus the review input) differ — a false reuse. Hashing the request object itself, - * the same object handed to `reviewRequest`, makes the key and the review provably one - * input: there is no second git read to diverge from. + * It hashes the WHOLE `request` object — the same object handed to `reviewRequest` — not a + * hand-picked subset. That makes key and review provably one input: any byte a reviewer + * sees (diff, the full `contextFiles`, intent, rubricVersion, AND the `validateSummary` + * including its `firstErrors`/`failCount`, which the panel reads) is in the key. Selecting a + * subset was unsound — e.g. `validateRunner` can return `passed:true` with a non-empty + * `firstErrors`, so two runs with an identical diff but different validate diagnostics feed + * the reviewers different bytes; omitting the summary from the key would false-reuse across + * them. Keying on a diff hash alone likewise missed `contextFiles` (a rebase can yield + * identical patch bytes over different surrounding file contents). */ export function reviewRequestKey( request: IReviewRequest, opts: { rosterHash: string; mode: string } ): string { - // JSON array (unforgeable: escaped + delimited) over the reviewer-visible content. The - // validate summary is NOT keyed — validate is re-run fresh every invocation (a request - // only exists when it currently passes), so it is a live precondition, not a cache axis. + // JSON array (unforgeable: escaped + delimited). `request` is built with a fixed field + // order in gatherChange, so its serialization is deterministic for identical content. return createHash("sha256") .update( - JSON.stringify([ - request.diff, - request.contextFiles ?? [], - request.intent, - request.rubricVersion, - opts.rosterHash, - opts.mode, - CACHE_VERSION, - ]) + JSON.stringify([request, opts.rosterHash, opts.mode, CACHE_VERSION]) ) .digest("hex"); } diff --git a/packages/core/tests/reviewers-artifact.test.ts b/packages/core/tests/reviewers-artifact.test.ts index eae4ceec..c142c8a8 100644 --- a/packages/core/tests/reviewers-artifact.test.ts +++ b/packages/core/tests/reviewers-artifact.test.ts @@ -1,10 +1,12 @@ import { test, expect, describe } from "bun:test"; +import { createHash } from "node:crypto"; import { reviewRequestKey, panelIdentityHash, decideVerdict, artifactBody, honorCachedVerdict, + CACHE_VERSION, } from "../src/reviewers/harness-review"; import type { IReviewRequest } from "../src/reviewers/schema"; import type { IVerdict } from "../src/reviewers/aggregate"; @@ -36,7 +38,7 @@ describe("reviewRequestKey (cache key = fingerprint of the ACTUAL review request ); }); - test("every reviewer-visible dimension changes the key — diff, contextFiles, intent, rubric, roster, mode", () => { + test("every reviewer-visible dimension changes the key — diff, contextFiles, intent, rubric, validateSummary, roster, mode", () => { const base = reviewRequestKey(request, rosterOpts); expect( @@ -56,6 +58,21 @@ describe("reviewRequestKey (cache key = fingerprint of the ACTUAL review request expect( reviewRequestKey({ ...request, rubricVersion: "2" }, rosterOpts) ).not.toBe(base); + // validateSummary is part of the request the reviewers read (firstErrors can differ even + // on a passing run), so a different summary MUST change the key — no false reuse. + expect( + reviewRequestKey( + { + ...request, + validateSummary: { + passed: true, + failCount: 0, + firstErrors: ["a stray 'error' line"], + }, + }, + rosterOpts + ) + ).not.toBe(base); expect( reviewRequestKey(request, { ...rosterOpts, rosterHash: "r2" }) ).not.toBe(base); @@ -73,12 +90,37 @@ describe("reviewRequestKey (cache key = fingerprint of the ACTUAL review request ); }); - test("a missing contextFiles hashes the same as an explicit empty list (no undefined/[] ambiguity)", () => { - const { contextFiles: _drop, ...noCtx } = request; + test("CACHE_VERSION is mixed into the key — bumping it retires ALL legacy artifacts in one shot", () => { + // The ONLY lever that invalidates every already-on-disk artifact (e.g. legacy diff-hash + // keys, or a poisoned pre-review block). If a regression dropped CACHE_VERSION from the + // hashed array, one-shot invalidation would silently break while every other test stayed + // green — so pin it by recomputing the exact key WITH it and asserting equality. + const expected = createHash("sha256") + .update( + JSON.stringify([ + request, + rosterOpts.rosterHash, + rosterOpts.mode, + CACHE_VERSION, + ]) + ) + .digest("hex"); + + expect(reviewRequestKey(request, rosterOpts)).toBe(expected); + + // And a different CACHE_VERSION would produce a different key (the invalidation itself). + const otherVersion = createHash("sha256") + .update( + JSON.stringify([ + request, + rosterOpts.rosterHash, + rosterOpts.mode, + "OTHER", + ]) + ) + .digest("hex"); - expect(reviewRequestKey(noCtx, rosterOpts)).toBe( - reviewRequestKey({ ...noCtx, contextFiles: [] }, rosterOpts) - ); + expect(reviewRequestKey(request, rosterOpts)).not.toBe(otherVersion); }); }); diff --git a/packages/core/tests/reviewers-harness-review.test.ts b/packages/core/tests/reviewers-harness-review.test.ts index 6b5c3009..d9d1f909 100644 --- a/packages/core/tests/reviewers-harness-review.test.ts +++ b/packages/core/tests/reviewers-harness-review.test.ts @@ -137,6 +137,30 @@ describe("gatherChange", () => { } }); + test("files listed but an EMPTY content diff (rename/mode-only) → block, not a vacuous cached green", async () => { + // The false-green the panel flagged: `--name-only` reports a file, but `git diff` exits 0 + // with empty stdout. Without the empty-diff guard a 0-byte review would be built + cached. + const git2: IGatherDeps["git"] = async (args) => { + if (args.includes("--name-only")) { + return { stdout: "renamed.ts", code: 0 }; + } + + if (args[0] === "diff") { + return { stdout: "", code: 0 }; // exit 0, but empty content + } + + return { stdout: "", code: 0 }; + }; + + const r = await gatherChange({ git: git2, validate: cleanValidate }, opts); + + expect(r.kind).toBe("block"); + + if (r.kind === "block") { + expect(r.reason).toMatch(/empty/iu); + } + }); + test("no changed files between base and HEAD → block (nothing to review, never a vacuous green)", async () => { const deps: IGatherDeps = { git: git({ "diff --name-only": "", diff: "" }), From 6a92ab7be8f8b822dfd6b03f59aa24ebc0680bcb Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Tue, 28 Jul 2026 00:50:18 +0200 Subject: [PATCH 07/11] fix(panel): testable runReviewFlow + full-config roster hash + canonical key (P4 round-8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-7 (4/4) findings, all addressed: - BLOCK: the CLI wiring had no test proving gather-before-cache / validate-fail-prevents- reuse. Extracted runReviewFlow (injectable) — gather runs BEFORE any cache access and a gather block never reads/writes the cache; the key is derived from the gathered request. Unit-tested for block-never-caches, gather-before-cache, hit/miss, --ci write-not-read. - panelIdentityHash now hashes the FULL resolved reviewer objects (model/endpoint/binary argv/timeout), not just ids — retargeting an id to a different implementation no longer reuses a verdict. Tested. - empty-content-diff guard kept but its rename justification corrected (real renames aren't empty; it's defense-in-depth) in both comment and test. - removed dead runHarnessReview + IRunDeps (CLI no longer calls them). - added a direct reviewRequest success-path test. - reviewRequestKey serializes via canonicalJson (recursive key-sort) so equal content can't thrash/diverge on object key order. - restored the CACHE_VERSION-in-key pin (recompute-mirror) + fixed stale tree-hash comments. Full suite 3177/0; typecheck+lint+format clean. --- packages/core/src/cli/harness-review-mode.ts | 87 ++++------ packages/core/src/reviewers/harness-review.ts | 156 +++++++++++------- .../core/tests/reviewers-artifact.test.ts | 120 +++++++++++--- .../tests/reviewers-harness-review.test.ts | 69 ++++---- 4 files changed, 261 insertions(+), 171 deletions(-) diff --git a/packages/core/src/cli/harness-review-mode.ts b/packages/core/src/cli/harness-review-mode.ts index b7fcf224..6d93ca97 100644 --- a/packages/core/src/cli/harness-review-mode.ts +++ b/packages/core/src/cli/harness-review-mode.ts @@ -15,10 +15,8 @@ import { resolvePanel } from "../reviewers/registry"; import { gatherChange, reviewRequest, - blockedVerdict, - reviewRequestKey, + runReviewFlow, panelIdentityHash, - decideVerdict, DEFAULT_MAX_FILES, DEFAULT_MAX_CHARS, artifactBody, @@ -302,57 +300,38 @@ export async function harnessReviewMode(argv: string[]): Promise { const treeHashRes = await gitRunner(["write-tree"]); const treeHash = treeHashRes.stdout.trim(); const identity = `${active.name}/${active.entry.model}`; - - // GATHER the review request FIRST — gatherChange runs validate FRESH (blocking, never - // cached, when the gate is red), resolves the base to an immutable merge-base SHA, and - // builds the exact request (diff + contextFiles) the reviewers will judge. A cache hit is - // therefore only ever reached when the gate currently passes — P4's "re-run validate - // before trusting the cache" requirement, satisfied by construction. - const gathered = await gatherChange( - { git: gitRunner, validate: validateRunner }, - { - base: args.base, - intent: args.intent, - maxFiles: DEFAULT_MAX_FILES, - maxChars: DEFAULT_MAX_CHARS, - } - ); - - let verdict: IVerdict; - let cacheHit = false; - - if (gathered.kind === "block") { - // A pre-review gate/precondition block — never cached (a transient block must not reject - // every later push). - verdict = blockedVerdict(gathered.reason, identity); - } else { - // Key on a fingerprint of the ACTUAL gathered request (diff + full contextFiles + intent - // + rubric) plus the EFFECTIVE roster (the reviewers actually used, after skips/quick- - // slice/active-model exclusion) and mode. The same request object is handed to the - // review, so key and review hash the same bytes — no second read to diverge from. - const rosterHash = panelIdentityHash(effective, identity); - const key = reviewRequestKey(gathered.request, { - rosterHash, - mode: args.quick ? "quick" : "full", - }); - const decided = await decideVerdict( - { - readCache: readCachedVerdict, - review: () => - reviewRequest(gathered.request, { - makeProvider, - runBinary, - panel: effective, - identity, - }), - persist: (v, k) => persistVerdict(v, k, treeHash, rosterHash), - }, - { ci: args.ci, key } - ); - - verdict = decided.verdict; - cacheHit = decided.cacheHit; - } + // Fingerprint the EFFECTIVE roster (the reviewers actually used after skips/quick-slice/ + // active-model exclusion) + the builder — reused for both the cache key and the artifact. + const rosterHash = panelIdentityHash(effective, identity); + + // runReviewFlow enforces the wiring invariant: GATHER (validate runs fresh inside) BEFORE + // any cache access, and a gather block never touches the cache. The gathered request is + // keyed from its OWN bytes, so key and review can't diverge. --ci writes but never reads. + const { verdict, cacheHit } = await runReviewFlow({ + gather: () => + gatherChange( + { git: gitRunner, validate: validateRunner }, + { + base: args.base, + intent: args.intent, + maxFiles: DEFAULT_MAX_FILES, + maxChars: DEFAULT_MAX_CHARS, + } + ), + identity, + rosterHash, + mode: args.quick ? "quick" : "full", + ci: args.ci, + readCache: readCachedVerdict, + review: (request) => + reviewRequest(request, { + makeProvider, + runBinary, + panel: effective, + identity, + }), + persist: (v, key) => persistVerdict(v, key, treeHash, rosterHash), + }); if (cacheHit) { process.stdout.write("harness-review: cache hit, reusing verdict\n"); diff --git a/packages/core/src/reviewers/harness-review.ts b/packages/core/src/reviewers/harness-review.ts index a4f3ce28..5db87f4c 100644 --- a/packages/core/src/reviewers/harness-review.ts +++ b/packages/core/src/reviewers/harness-review.ts @@ -144,12 +144,13 @@ export async function gatherChange( const diff = diffRes.stdout; if (diff.length === 0) { - // Files are listed but the CONTENT diff is empty (a rename- or mode-only change, or a - // git quirk): there is nothing textual for the reviewers to judge, so a PASS would be - // vacuous. Block rather than build+cache an empty review. + // Defense in depth: files were listed but the CONTENT diff came back empty. (A real + // rename/mode-only change is NOT empty — git emits `similarity index` / `old mode` etc. + // — so this is an anomaly: a git quirk, or a `--name-only`/`diff` disagreement.) There is + // nothing for the reviewers to judge, so block rather than build+cache a vacuous review. return { kind: "block", - reason: `the diff between ${base} and HEAD is empty (rename/mode-only change?) — nothing to review`, + reason: `the diff between ${base} and HEAD is empty despite listed changes — nothing to review`, }; } @@ -216,12 +217,7 @@ async function gatherContext( return blocks; } -export interface IRunDeps extends IGatherDeps, IInvokeDeps { - panel: IPanel; - identity: string; -} - -/** A pre-review gate/precondition block (validate red, empty intent, diff too large, +/** A pre-review gate/precondition block (validate red, empty intent, empty/oversized diff, * git failure). The panel did NOT run. Marked `preReview` so it is never cached — a * transient block must not poison the cache and reject every later push. Exported so the * CLI produces it directly (it gathers the request itself, then reviews). */ @@ -243,9 +239,9 @@ export interface IReviewDeps extends IInvokeDeps { } /** Run the panel on an ALREADY-GATHERED request (validate + diff + context already built). - * Split out from runHarnessReview so the CLI can fingerprint the exact request for the - * cache key BEFORE deciding whether to invoke the models — key and review then hash the - * same bytes by construction. */ + * Split from the gather step so the CLI can fingerprint the exact request for the cache + * key BEFORE deciding whether to invoke the models — key and review then hash the same + * bytes by construction. */ export async function reviewRequest( request: IReviewRequest, deps: IReviewDeps @@ -261,25 +257,41 @@ export async function reviewRequest( }); } -export async function runHarnessReview( - deps: IRunDeps, - opts: IGatherOptions -): Promise { - const gathered = await gatherChange(deps, opts); - - if (gathered.kind === "block") { - return blockedVerdict(gathered.reason, deps.identity); - } - - return reviewRequest(gathered.request, deps); -} - /** Verdict-cache schema version. Bump to invalidate ALL previously written cache * artifacts in one shot. Bumped to "3" when the key changed from a diff-hash to a full * request fingerprint (below): legacy artifacts key on incomparable inputs, so the bump * retires them rather than risk a stale-input collision. */ export const CACHE_VERSION = "3"; +/** Deterministic JSON: recursively sort object keys so equal CONTENT hashes equally + * regardless of construction/insertion order. A cache key must not thrash — or, if a + * second request-construction path ever appears, diverge — because two equal requests + * serialized their keys in a different order. Exported so the cache-key pin test can + * recompute the exact digest (and catch an accidental drop of CACHE_VERSION from the key). */ +export function canonicalJson(value: unknown): string { + const canon = (v: unknown): unknown => { + if (Array.isArray(v)) { + return v.map(canon); + } + + if (typeof v === "object" && v !== null) { + const out: Record = {}; + + for (const [k, val] of Object.entries(v).sort(([a], [b]) => + a < b ? -1 : a > b ? 1 : 0 + )) { + out[k] = canon(val); + } + + return out; + } + + return v; + }; + + return JSON.stringify(canon(value)); +} + /** * Fingerprint the EXACT review request the reviewers will judge, plus the roster identity * and mode. This is the cache key. @@ -290,84 +302,106 @@ export const CACHE_VERSION = "3"; * including its `firstErrors`/`failCount`, which the panel reads) is in the key. Selecting a * subset was unsound — e.g. `validateRunner` can return `passed:true` with a non-empty * `firstErrors`, so two runs with an identical diff but different validate diagnostics feed - * the reviewers different bytes; omitting the summary from the key would false-reuse across - * them. Keying on a diff hash alone likewise missed `contextFiles` (a rebase can yield - * identical patch bytes over different surrounding file contents). + * the reviewers different bytes; omitting the summary would false-reuse across them. + * Serialized canonically (key-sorted) so equal content always yields the same digest. */ export function reviewRequestKey( request: IReviewRequest, opts: { rosterHash: string; mode: string } ): string { - // JSON array (unforgeable: escaped + delimited). `request` is built with a fixed field - // order in gatherChange, so its serialization is deterministic for identical content. return createHash("sha256") - .update( - JSON.stringify([request, opts.rosterHash, opts.mode, CACHE_VERSION]) - ) + .update(canonicalJson([request, opts.rosterHash, opts.mode, CACHE_VERSION])) .digest("hex"); } /** - * Identity of the roster that ACTUALLY reviewed — the resolved reviewer ids (after skips, - * quick-slicing, and active-model exclusion), the quorum, and the builder. A verdict from a - * different roster (a reviewer added/dropped, a different builder whose independence differs) - * must not be reused, so this feeds the cache key. Keying on the raw config instead would - * miss all of those, which the panel flagged. + * Identity of the roster that ACTUALLY reviewed. Hashes the FULL resolved reviewer objects + * (kind, id, model `entry`, binary argv/input mode, endpoint, timeout — every field), not + * just their ids: retargeting the SAME id to a different model/endpoint/binary yields a + * different reviewer IMPLEMENTATION whose verdict must not be reused. Plus the quorum and + * the builder identity (independence differs per builder). Sorted by id so resolution order + * doesn't churn the key. This feeds the cache key; keying on ids alone (or the raw config) + * missed reviewer-implementation changes, which the panel flagged. */ export function panelIdentityHash( panel: { reviewers: readonly { id: string }[]; minReviewers: number }, builderIdentity: string ): string { - const roster = panel.reviewers.map((r) => r.id).sort(); + const roster = [...panel.reviewers].sort((a, b) => + a.id < b.id ? -1 : a.id > b.id ? 1 : 0 + ); return createHash("sha256") - .update(JSON.stringify([roster, panel.minReviewers, builderIdentity])) + .update(canonicalJson([roster, panel.minReviewers, builderIdentity])) .digest("hex"); } -export interface IDecideDeps { +export interface IReviewFlowDeps { + /** Gather the review request — runs validate FRESH and builds the request (or blocks). */ + gather: () => Promise; + identity: string; + /** Fingerprint of the effective roster + builder (panelIdentityHash). */ + rosterHash: string; + /** "quick" | "full". */ + mode: string; + ci: boolean; /** Read a cached verdict by key (already applies the pre-review guard); null = miss. */ readCache: (key: string) => Promise; - /** Run the live panel (only reached on a miss or --ci). */ - review: () => Promise; + /** Run the live panel on the gathered request (only reached on a miss or --ci). */ + review: (request: IReviewRequest) => Promise; /** Persist the verdict under the key (the real impl guards against caching a pre-review block). */ persist: (verdict: IVerdict, key: string) => Promise; } /** - * Decide a verdict for an already-gathered, already-keyed request: reuse the cache when - * possible, else run the panel and persist. Extracted from the CLI so the wiring is - * unit-testable (the panel required proof that a cache hit is reused, a miss writes under - * the key, and --ci writes but never reads). + * The end-to-end review flow, injectable so the CLI wiring's CENTRAL INVARIANT is directly + * testable rather than only implied by the units: the request is GATHERED (validate runs + * inside gather) BEFORE any cache access, and a gather block (validate red / precondition) + * yields a blocked verdict WITHOUT reading OR writing the cache. Only a gathered request is + * keyed — from its own bytes — then reused-or-reviewed. A future reordering or bypass in the + * CLI that read the cache before validating, or reused a verdict across a red gate, breaks a + * test here (which unit tests for gather + the cache decision separately cannot catch). * - * Validate freshness is NOT this function's concern: the caller gathers the request first - * (gatherChange runs validate fresh and blocks — never reaching here — when the gate is red), - * so a cache hit implies the gate currently passes. --ci never READS the cache (CI always - * re-reviews) but still WRITES it, seeding a later interactive run with the identical request. + * `--ci` never READS the cache (CI always re-reviews) but still WRITES it, seeding a later + * interactive run with the identical request. */ -export async function decideVerdict( - deps: IDecideDeps, - opts: { ci: boolean; key: string } +export async function runReviewFlow( + deps: IReviewFlowDeps ): Promise<{ verdict: IVerdict; cacheHit: boolean }> { - if (!opts.ci) { - const cached = await deps.readCache(opts.key); + const gathered = await deps.gather(); + + if (gathered.kind === "block") { + // Validate red / precondition — never touch the cache. + return { + verdict: blockedVerdict(gathered.reason, deps.identity), + cacheHit: false, + }; + } + + const key = reviewRequestKey(gathered.request, { + rosterHash: deps.rosterHash, + mode: deps.mode, + }); + + if (!deps.ci) { + const cached = await deps.readCache(key); if (cached !== null) { return { verdict: cached, cacheHit: true }; } } - const verdict = await deps.review(); + const verdict = await deps.review(gathered.request); - await deps.persist(verdict, opts.key); + await deps.persist(verdict, key); return { verdict, cacheHit: false }; } /** The caching decision, isolated so it is unit-testable without the filesystem. * ONLY a real panel verdict is cached. A pre-review gate/precondition block - * (validate failed, empty intent, diff too large) is transient — caching one - * poisons the tree-hash so a flaky validate under load blocks every later push. */ + * (validate failed, empty intent, empty/oversized diff) is transient — caching one + * would re-serve as a permanent block for that request, so it is skipped. */ export function shouldCacheVerdict(verdict: IVerdict): boolean { return verdict.preReview !== true; } diff --git a/packages/core/tests/reviewers-artifact.test.ts b/packages/core/tests/reviewers-artifact.test.ts index c142c8a8..4e0cf002 100644 --- a/packages/core/tests/reviewers-artifact.test.ts +++ b/packages/core/tests/reviewers-artifact.test.ts @@ -3,11 +3,13 @@ import { createHash } from "node:crypto"; import { reviewRequestKey, panelIdentityHash, - decideVerdict, + runReviewFlow, + canonicalJson, artifactBody, honorCachedVerdict, CACHE_VERSION, } from "../src/reviewers/harness-review"; +import type { GatherResult } from "../src/reviewers/harness-review"; import type { IReviewRequest } from "../src/reviewers/schema"; import type { IVerdict } from "../src/reviewers/aggregate"; @@ -97,7 +99,7 @@ describe("reviewRequestKey (cache key = fingerprint of the ACTUAL review request // green — so pin it by recomputing the exact key WITH it and asserting equality. const expected = createHash("sha256") .update( - JSON.stringify([ + canonicalJson([ request, rosterOpts.rosterHash, rosterOpts.mode, @@ -111,7 +113,7 @@ describe("reviewRequestKey (cache key = fingerprint of the ACTUAL review request // And a different CACHE_VERSION would produce a different key (the invalidation itself). const otherVersion = createHash("sha256") .update( - JSON.stringify([ + canonicalJson([ request, rosterOpts.rosterHash, rosterOpts.mode, @@ -125,15 +127,18 @@ describe("reviewRequestKey (cache key = fingerprint of the ACTUAL review request }); describe("panelIdentityHash (the roster that ACTUALLY reviewed keys the cache)", () => { - const panel = { - reviewers: [{ id: "grok" }, { id: "codex" }], - minReviewers: 2, + const grok = { + kind: "model", + id: "grok", + entry: { baseUrl: "http://a", model: "g1" }, }; + const codex = { kind: "binary", id: "codex", argv: ["codex"] }; + const panel = { reviewers: [grok, codex], minReviewers: 2 }; - test("stable and order-independent (roster is sorted before hashing)", () => { + test("stable and order-independent (roster is sorted by id before hashing)", () => { expect(panelIdentityHash(panel, "local/flash")).toBe( panelIdentityHash( - { reviewers: [{ id: "codex" }, { id: "grok" }], minReviewers: 2 }, + { reviewers: [codex, grok], minReviewers: 2 }, "local/flash" ) ); @@ -143,38 +148,65 @@ describe("panelIdentityHash (the roster that ACTUALLY reviewed keys the cache)", const base = panelIdentityHash(panel, "local/flash"); expect( - panelIdentityHash( - { reviewers: [{ id: "grok" }], minReviewers: 2 }, - "local/flash" - ) + panelIdentityHash({ reviewers: [grok], minReviewers: 2 }, "local/flash") ).not.toBe(base); // a dropped reviewer (effective roster ≠ configured) must not reuse expect( panelIdentityHash({ ...panel, minReviewers: 1 }, "local/flash") ).not.toBe(base); expect(panelIdentityHash(panel, "other/model")).not.toBe(base); // different builder }); + + test("RETARGETING the same id to a different model/endpoint/binary changes the key (reviewer implementation is pinned, not just its id)", () => { + // The panel finding: hashing ids alone would reuse a verdict produced by a DIFFERENT + // reviewer implementation. Full config is hashed, so same-id-different-model differs. + const grokRetargeted = { + kind: "model", + id: "grok", + entry: { baseUrl: "http://a", model: "g2-DIFFERENT" }, + }; + + expect( + panelIdentityHash( + { reviewers: [grokRetargeted, codex], minReviewers: 2 }, + "local/flash" + ) + ).not.toBe(panelIdentityHash(panel, "local/flash")); + }); }); -describe("decideVerdict (cache orchestration for an already-gathered, already-keyed request)", () => { +describe("runReviewFlow (the CLI wiring invariant: gather-before-cache, block-never-caches)", () => { const reviewVerdict: IVerdict = { ...v, reason: "fresh review" }; const cachedVerdict: IVerdict = { ...v, reason: "from cache" }; interface ICalls { + gather: number; read: number; review: number; persist: string[]; } - const harness = (cached: IVerdict | null) => { - const calls: ICalls = { read: 0, review: 0, persist: [] }; + // A flow harness: `gathered` is what gather() returns (block or request); records the + // order/counts so the wiring invariant can be asserted, and captures the key each cache + // op received (to prove it derives from the gathered request). + const harness = (gathered: GatherResult, cached: IVerdict | null) => { + const calls: ICalls = { gather: 0, read: 0, review: 0, persist: [] }; const deps = { + gather: async () => { + calls.gather += 1; + + return gathered; + }, + identity: "local/flash", + rosterHash: "roster-1", + mode: "full", + ci: false, readCache: async (key: string) => { calls.read += 1; void key; return cached; }, - review: async () => { + review: async (_request: IReviewRequest) => { calls.review += 1; return reviewVerdict; @@ -187,36 +219,72 @@ describe("decideVerdict (cache orchestration for an already-gathered, already-ke return { calls, deps }; }; - test("cache HIT (interactive) reuses the cached verdict — no review, no write", async () => { - const { calls, deps } = harness(cachedVerdict); + const requestResult: GatherResult = { kind: "request", request }; + + test("a gather BLOCK (validate red / precondition) returns a blocked verdict WITHOUT touching the cache", async () => { + // The central invariant: no cache read AND no write when the gate is red — a verdict is + // never reused across a failing validate, and a transient block is never persisted. + const { calls, deps } = harness( + { kind: "block", reason: "validate failed (3 errors)" }, + cachedVerdict + ); + + const r = await runReviewFlow(deps); + + expect(r.verdict.blocked).toBe(true); + expect(r.verdict.preReview).toBe(true); + expect(calls.gather).toBe(1); + expect(calls.read).toBe(0); + expect(calls.review).toBe(0); + expect(calls.persist).toHaveLength(0); + }); + + test("gather runs BEFORE any cache access, and the cache key is derived from the gathered request", async () => { + const { calls, deps } = harness(requestResult, null); + + await runReviewFlow(deps); + + // The key the cache saw is exactly reviewRequestKey(gathered.request, roster/mode). + const expectedKey = reviewRequestKey(request, { + rosterHash: "roster-1", + mode: "full", + }); + + expect(calls.gather).toBe(1); + expect(calls.persist).toEqual([expectedKey]); + }); + + test("cache HIT reuses the cached verdict — gather ran, but no review, no write", async () => { + const { calls, deps } = harness(requestResult, cachedVerdict); - const r = await decideVerdict(deps, { ci: false, key: "k" }); + const r = await runReviewFlow(deps); expect(r.cacheHit).toBe(true); expect(r.verdict.reason).toBe("from cache"); + expect(calls.gather).toBe(1); expect(calls.review).toBe(0); expect(calls.persist).toHaveLength(0); }); - test("cache MISS runs the review and WRITES under the SAME key", async () => { - const { calls, deps } = harness(null); + test("cache MISS runs the review and writes under the derived key", async () => { + const { calls, deps } = harness(requestResult, null); - const r = await decideVerdict(deps, { ci: false, key: "k" }); + const r = await runReviewFlow(deps); expect(r.cacheHit).toBe(false); expect(r.verdict.reason).toBe("fresh review"); expect(calls.review).toBe(1); - expect(calls.persist).toEqual(["k"]); + expect(calls.persist).toHaveLength(1); }); test("--ci WRITES but never READS the cache (CI always re-reviews)", async () => { - const { calls, deps } = harness(cachedVerdict); + const { calls, deps } = harness(requestResult, cachedVerdict); - const r = await decideVerdict(deps, { ci: true, key: "k" }); + const r = await runReviewFlow({ ...deps, ci: true }); expect(calls.read).toBe(0); // never reads on CI, even though a cache entry exists expect(calls.review).toBe(1); - expect(calls.persist).toEqual(["k"]); + expect(calls.persist).toHaveLength(1); expect(r.cacheHit).toBe(false); }); }); diff --git a/packages/core/tests/reviewers-harness-review.test.ts b/packages/core/tests/reviewers-harness-review.test.ts index d9d1f909..731ab059 100644 --- a/packages/core/tests/reviewers-harness-review.test.ts +++ b/packages/core/tests/reviewers-harness-review.test.ts @@ -1,10 +1,9 @@ import { test, expect, describe } from "bun:test"; import { gatherChange, - runHarnessReview, + reviewRequest, shouldCacheVerdict, type IGatherDeps, - type IRunDeps, } from "../src/reviewers/harness-review"; import type { IPanel } from "../src/reviewers/registry"; @@ -137,9 +136,10 @@ describe("gatherChange", () => { } }); - test("files listed but an EMPTY content diff (rename/mode-only) → block, not a vacuous cached green", async () => { - // The false-green the panel flagged: `--name-only` reports a file, but `git diff` exits 0 - // with empty stdout. Without the empty-diff guard a 0-byte review would be built + cached. + test("files listed but an EMPTY content diff → block (defense in depth), not a vacuous cached green", async () => { + // `--name-only` reports a file, but the content `git diff` exits 0 with empty stdout — a + // git anomaly / name-only↔diff disagreement (a REAL rename/mode change is non-empty). The + // guard blocks it rather than build+cache a 0-byte review the panel would green-light. const git2: IGatherDeps["git"] = async (args) => { if (args.includes("--name-only")) { return { stdout: "renamed.ts", code: 0 }; @@ -231,41 +231,50 @@ describe("gatherChange", () => { }); }); -describe("runHarnessReview", () => { - test("a blocked gather short-circuits to a blocked verdict without invoking reviewers", async () => { - let invoked = false; - const panel: IPanel = { reviewers: [], minReviewers: 2, skipped: [] }; - const deps: IRunDeps = { - git: git({ "diff --name-only": "x.ts", diff: "+x" }), - validate: async () => ({ - passed: false, - failCount: 1, - firstErrors: ["boom"], - }), +describe("reviewRequest (success path: invoke the panel on a gathered request → aggregate)", () => { + test("invokes the reviewers and returns their aggregated verdict", async () => { + const request = { + title: "add x", + intent: "add x", + diff: "diff --git a/x b/x\n+code", + validateSummary: { passed: true, failCount: 0, firstErrors: [] }, + contextFiles: [], + rubricVersion: "1", + }; + // A single approving model reviewer; aggregate should return an unblocked verdict. + const panel: IPanel = { + reviewers: [ + { + kind: "model", + id: "r1", + entry: { baseUrl: "http://x/v1", model: "m" }, + }, + ], + minReviewers: 1, + skipped: [], + }; + let invoked = 0; + const v = await reviewRequest(request, { makeProvider: () => { - invoked = true; + invoked += 1; return { async complete() { - return { content: "", toolCalls: [] }; + return { + content: '{"decision":"approve","findings":[]}', + toolCalls: [], + }; }, }; }, - runBinary: async () => { - invoked = true; - - return { ok: true, stdout: "" }; - }, + runBinary: async () => ({ ok: true, stdout: "" }), panel, identity: "local/flash", - }; - const v = await runHarnessReview(deps, opts); + }); - expect(v.blocked).toBe(true); - expect(invoked).toBe(false); - // Marked as a PRE-REVIEW gate block so the caller never caches it — a transient - // validate flake under load must not poison the tree-hash for every later push. - expect(v.preReview).toBe(true); + expect(invoked).toBe(1); // the panel actually ran (not short-circuited) + expect(v.identity).toBe("local/flash"); + expect(v.preReview).not.toBe(true); // a real panel verdict, cacheable }); }); From 1aa274db962cc58eb4a85c5643130106adf9cfa3 Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Tue, 28 Jul 2026 01:09:22 +0200 Subject: [PATCH 08/11] fix(panel): pin HEAD snapshot + CLI-wiring test + typed roster (P4 round-9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-8 (4/4) findings, all addressed: - TOCTOU: pin HEAD to a SHA once (resolveHead) and read the file list, diff, subject, and context all against that snapshot — a HEAD move mid-gather can no longer mix commits into one request. resolveBase/resolveIntent/gatherContext take the pinned head. - CLI wiring now has a mirrored test: extracted buildReviewFlowDeps (in harness-review-mode) and unit-tested it — rosterHash derives from the EFFECTIVE panel, review targets it, mode/ci map from args, gather validates fresh + diffs the pinned base…HEAD, persist carries the effective rosterHash. The runReviewFlow test now asserts real call ORDER (gather→read→review→persist) and the exact key readCache/persist receive. - panelIdentityHash typed to require full ResolvedReviewer[] (no id-only caller) — the compile-time reintroduction of the false-reuse bug is closed. - added the resolveBase merge-base SUCCESS-path test (diffs MBSHA...HEADSHA). - added a canonicalJson order-independence test (same content, reordered keys → same digest). Full suite 3184/0; typecheck+lint+format clean. --- packages/core/src/cli/harness-review-mode.ts | 107 ++++++++--- packages/core/src/reviewers/harness-review.ts | 63 +++++-- .../core/tests/harness-review-mode.test.ts | 175 ++++++++++++++++++ .../core/tests/reviewers-artifact.test.ts | 63 ++++++- .../tests/reviewers-harness-review.test.ts | 45 +++++ 5 files changed, 394 insertions(+), 59 deletions(-) create mode 100644 packages/core/tests/harness-review-mode.test.ts diff --git a/packages/core/src/cli/harness-review-mode.ts b/packages/core/src/cli/harness-review-mode.ts index 6d93ca97..f9fc4f08 100644 --- a/packages/core/src/cli/harness-review-mode.ts +++ b/packages/core/src/cli/harness-review-mode.ts @@ -11,7 +11,7 @@ import { type IModelEntry, type BinaryInputMode, } from "../models-config"; -import { resolvePanel } from "../reviewers/registry"; +import { resolvePanel, type IPanel } from "../reviewers/registry"; import { gatherChange, reviewRequest, @@ -22,6 +22,10 @@ import { artifactBody, shouldCacheVerdict, honorCachedVerdict, + type IReviewFlowDeps, + type IReviewDeps, + type IGitRunner, + type IValidateRunner, } from "../reviewers/harness-review"; import { parseVerdict, type IVerdict } from "../reviewers/aggregate"; @@ -274,6 +278,60 @@ export function formatVerdict(v: IVerdict): string { return lines.join("\n"); } +/** + * Wire the resolved CLI pieces (effective roster, args, git/validate, providers, cache + * seams) into the runReviewFlow deps — exported so the CLI's central wiring is unit-tested: + * the cache key's rosterHash is derived from the EFFECTIVE roster (not cfg), the review + * targets the effective panel, mode/ci come from the args, and gather reads args.base/intent. + * A miswire (cfg roster, hardcoded ci, wrong panel) is caught here rather than only in prod. + */ +export function buildReviewFlowDeps(input: { + effective: IPanel; + identity: string; + quick: boolean; + ci: boolean; + base: string | undefined; + intent: string | undefined; + git: IGitRunner; + validate: IValidateRunner; + makeProvider: IReviewDeps["makeProvider"]; + runBinary: IReviewDeps["runBinary"]; + readCache: (key: string) => Promise; + persistArtifact: ( + verdict: IVerdict, + key: string, + rosterHash: string + ) => Promise; +}): IReviewFlowDeps { + const rosterHash = panelIdentityHash(input.effective, input.identity); + + return { + gather: () => + gatherChange( + { git: input.git, validate: input.validate }, + { + base: input.base, + intent: input.intent, + maxFiles: DEFAULT_MAX_FILES, + maxChars: DEFAULT_MAX_CHARS, + } + ), + identity: input.identity, + rosterHash, + mode: input.quick ? "quick" : "full", + ci: input.ci, + readCache: input.readCache, + review: (request) => + reviewRequest(request, { + makeProvider: input.makeProvider, + runBinary: input.runBinary, + panel: input.effective, + identity: input.identity, + }), + persist: (v, key) => input.persistArtifact(v, key, rosterHash), + }; +} + export async function harnessReviewMode(argv: string[]): Promise { const args = parse(argv); @@ -300,38 +358,29 @@ export async function harnessReviewMode(argv: string[]): Promise { const treeHashRes = await gitRunner(["write-tree"]); const treeHash = treeHashRes.stdout.trim(); const identity = `${active.name}/${active.entry.model}`; - // Fingerprint the EFFECTIVE roster (the reviewers actually used after skips/quick-slice/ - // active-model exclusion) + the builder — reused for both the cache key and the artifact. - const rosterHash = panelIdentityHash(effective, identity); // runReviewFlow enforces the wiring invariant: GATHER (validate runs fresh inside) BEFORE // any cache access, and a gather block never touches the cache. The gathered request is // keyed from its OWN bytes, so key and review can't diverge. --ci writes but never reads. - const { verdict, cacheHit } = await runReviewFlow({ - gather: () => - gatherChange( - { git: gitRunner, validate: validateRunner }, - { - base: args.base, - intent: args.intent, - maxFiles: DEFAULT_MAX_FILES, - maxChars: DEFAULT_MAX_CHARS, - } - ), - identity, - rosterHash, - mode: args.quick ? "quick" : "full", - ci: args.ci, - readCache: readCachedVerdict, - review: (request) => - reviewRequest(request, { - makeProvider, - runBinary, - panel: effective, - identity, - }), - persist: (v, key) => persistVerdict(v, key, treeHash, rosterHash), - }); + // buildReviewFlowDeps constructs the deps (roster hash from the EFFECTIVE panel etc.) and + // is unit-tested for that wiring. + const { verdict, cacheHit } = await runReviewFlow( + buildReviewFlowDeps({ + effective, + identity, + quick: args.quick, + ci: args.ci, + base: args.base, + intent: args.intent, + git: gitRunner, + validate: validateRunner, + makeProvider, + runBinary, + readCache: readCachedVerdict, + persistArtifact: (v, key, rosterHash) => + persistVerdict(v, key, treeHash, rosterHash), + }) + ); if (cacheHit) { process.stdout.write("harness-review: cache hit, reusing verdict\n"); diff --git a/packages/core/src/reviewers/harness-review.ts b/packages/core/src/reviewers/harness-review.ts index 5db87f4c..2d7cf379 100644 --- a/packages/core/src/reviewers/harness-review.ts +++ b/packages/core/src/reviewers/harness-review.ts @@ -1,5 +1,5 @@ import { createHash } from "node:crypto"; -import type { IPanel } from "./registry"; +import type { IPanel, ResolvedReviewer } from "./registry"; import { reviewerInvoke, type IInvokeDeps } from "./invoke"; import { aggregate, type IVerdict } from "./aggregate"; import { @@ -41,16 +41,29 @@ export type GatherResult = | { kind: "request"; request: IReviewRequest } | { kind: "block"; reason: string }; +/** Pin HEAD to an immutable commit SHA once, so every subsequent read in a gather (the diff, + * the file list, the context `show`, the subject `log`) references the SAME snapshot. If + * HEAD moves mid-gather, an unpinned gather would combine file names, diff, and context from + * DIFFERENT commits — the gate would then approve a scope that isn't the commit being pushed + * (a TOCTOU the panel flagged). Falls back to the literal "HEAD" only if rev-parse fails. */ +async function resolveHead(git: IGitRunner): Promise { + const res = await git(["rev-parse", "HEAD"]); + const head = res.stdout.trim(); + + return head.length > 0 ? head : "HEAD"; +} + async function resolveBase( git: IGitRunner, - explicit: string | undefined + explicit: string | undefined, + head: string ): Promise { const ref = explicit !== undefined && explicit.length > 0 ? explicit : "main"; - // Pin to the merge-base SHA, not the ref. The diff is `${base}...HEAD` (three-dot), whose - // true origin is merge-base(ref, HEAD); returning that immutable SHA means the bytes the - // fingerprint hashes and the bytes the review diffs are ONE snapshot even if `ref` moves - // between the two in-process git calls (and a rebase that shifts the merge-base changes it). - const res = await git(["merge-base", ref, "HEAD"]); + // Pin to the merge-base SHA against the PINNED head, not the ref. The diff is + // `${base}...${head}` (three-dot), whose true origin is merge-base(ref, head); returning + // that immutable SHA means the bytes the fingerprint hashes and the bytes the review diffs + // are ONE snapshot even if `ref` moves (and a rebase that shifts the merge-base changes it). + const res = await git(["merge-base", ref, head]); const base = res.stdout.trim(); if (base.length > 0) { @@ -58,7 +71,7 @@ async function resolveBase( } // merge-base failed (e.g. no `main`): fall back to the ref itself (or HEAD~1 when omitted). - // Cache safety does not rest on this — if the resulting `${base}...HEAD` diff can't be + // Cache safety does not rest on this — if the resulting `${base}...${head}` diff can't be // computed, gatherChange blocks (git-exit / empty-diff guards) rather than caching a // vacuous review. return explicit !== undefined && explicit.length > 0 ? explicit : "HEAD~1"; @@ -66,13 +79,14 @@ async function resolveBase( async function resolveIntent( git: IGitRunner, - explicit: string | undefined + explicit: string | undefined, + head: string ): Promise { if (explicit !== undefined && explicit.trim().length > 0) { return explicit.trim(); } - const subject = (await git(["log", "-1", "--format=%s"])).stdout.trim(); + const subject = (await git(["log", "-1", "--format=%s", head])).stdout.trim(); return GENERIC_INTENTS.has(subject.toLowerCase()) ? null : subject; } @@ -90,8 +104,11 @@ export async function gatherChange( }; } - const base = await resolveBase(deps.git, opts.base); - const intent = await resolveIntent(deps.git, opts.intent); + // Pin HEAD to a SHA ONCE, then read the file list, diff, subject, and context all against + // that same snapshot — so a HEAD move mid-gather can't mix commits into one request. + const head = await resolveHead(deps.git); + const base = await resolveBase(deps.git, opts.base, head); + const intent = await resolveIntent(deps.git, opts.intent, head); if (intent === null) { return { @@ -104,7 +121,7 @@ export async function gatherChange( // Read the changed-file list AND check git's exit code. Ignoring it lets a failed diff // (empty stdout on error) build an EMPTY review that the panel green-lights, then caches // under the real request's key — a false green. A failure blocks instead. - const namesRes = await deps.git(["diff", "--name-only", `${base}...HEAD`]); + const namesRes = await deps.git(["diff", "--name-only", `${base}...${head}`]); if (namesRes.code !== 0) { return { @@ -121,7 +138,7 @@ export async function gatherChange( if (files.length === 0) { return { kind: "block", - reason: `no changes between ${base} and HEAD to review`, + reason: `no changes between ${base} and ${head} to review`, }; } @@ -132,7 +149,7 @@ export async function gatherChange( }; } - const diffRes = await deps.git(["diff", `${base}...HEAD`]); + const diffRes = await deps.git(["diff", `${base}...${head}`]); if (diffRes.code !== 0) { return { @@ -150,7 +167,7 @@ export async function gatherChange( // nothing for the reviewers to judge, so block rather than build+cache a vacuous review. return { kind: "block", - reason: `the diff between ${base} and HEAD is empty despite listed changes — nothing to review`, + reason: `the diff between ${base} and ${head} is empty despite listed changes — nothing to review`, }; } @@ -161,7 +178,12 @@ export async function gatherChange( }; } - const contextFiles = await gatherContext(deps.git, files, opts.maxChars); + const contextFiles = await gatherContext( + deps.git, + files, + head, + opts.maxChars + ); return { kind: "request", @@ -184,6 +206,7 @@ export async function gatherChange( async function gatherContext( git: IGatherDeps["git"], files: string[], + head: string, budget: number ): Promise { const blocks: string[] = []; @@ -191,10 +214,10 @@ async function gatherContext( let omitted = 0; for (const file of files) { - const res = await git(["show", `HEAD:${file}`]); + const res = await git(["show", `${head}:${file}`]); if (res.code !== 0) { - continue; // deleted/renamed/binary — not readable at HEAD, skip + continue; // deleted/renamed/binary — not readable at this commit, skip } const block = `=== ${file} ===\n${res.stdout}`; @@ -324,7 +347,7 @@ export function reviewRequestKey( * missed reviewer-implementation changes, which the panel flagged. */ export function panelIdentityHash( - panel: { reviewers: readonly { id: string }[]; minReviewers: number }, + panel: { reviewers: readonly ResolvedReviewer[]; minReviewers: number }, builderIdentity: string ): string { const roster = [...panel.reviewers].sort((a, b) => diff --git a/packages/core/tests/harness-review-mode.test.ts b/packages/core/tests/harness-review-mode.test.ts new file mode 100644 index 00000000..3c46b8f7 --- /dev/null +++ b/packages/core/tests/harness-review-mode.test.ts @@ -0,0 +1,175 @@ +import { test, expect, describe } from "bun:test"; +import { buildReviewFlowDeps } from "../src/cli/harness-review-mode"; +import { panelIdentityHash } from "../src/reviewers/harness-review"; +import type { IReviewRequest } from "../src/reviewers/schema"; +import type { IPanel } from "../src/reviewers/registry"; +import type { IVerdict } from "../src/reviewers/aggregate"; + +/** + * The CLI wiring (harness-review-mode) is exercised through buildReviewFlowDeps: it must + * derive the cache key's rosterHash from the EFFECTIVE panel, target the review at the + * effective panel, map mode/ci from the args, gather with args.base/intent, and thread the + * effective rosterHash into persistence. A miswire (cfg roster, hardcoded ci, wrong panel) + * is caught here rather than only in production. + */ + +const effective: IPanel = { + reviewers: [ + { + kind: "model", + id: "r1", + entry: { baseUrl: "http://x/v1", model: "MODEL-X" }, + }, + ], + minReviewers: 1, + skipped: [], +}; + +const request: IReviewRequest = { + title: "t", + intent: "t", + diff: "d", + validateSummary: { passed: true, failCount: 0, firstErrors: [] }, + contextFiles: [], + rubricVersion: "1", +}; + +function makeInput(over: { quick?: boolean; ci?: boolean }) { + const log = { + validated: 0, + gitArgs: [] as string[][], + providerModels: [] as string[], + readKeys: [] as string[], + persisted: [] as { key: string; rosterHash: string }[], + }; + + return { + log, + input: { + effective, + identity: "local/flash", + quick: over.quick ?? false, + ci: over.ci ?? false, + base: "main", + intent: "do the thing", + git: async (args: string[]) => { + log.gitArgs.push(args); + + // Enough for gatherChange to build a request: rev-parse/merge-base SHAs, a file + + // a non-empty diff, readable context. + if (args[0] === "rev-parse") { + return { stdout: "HEADSHA\n", code: 0 }; + } + + if (args[0] === "merge-base") { + return { stdout: "MBSHA\n", code: 0 }; + } + + if (args[0] === "diff") { + return args.includes("--name-only") + ? { stdout: "x.ts", code: 0 } + : { stdout: "diff --git a/x b/x\n+code", code: 0 }; + } + + if (args[0] === "show") { + return { stdout: "content", code: 0 }; + } + + return { stdout: "", code: 0 }; + }, + validate: async () => { + log.validated += 1; + + return { passed: true, failCount: 0, firstErrors: [] }; + }, + makeProvider: (entry: { model: string }) => { + log.providerModels.push(entry.model); + + return { + async complete() { + return { + content: '{"decision":"approve","findings":[]}', + toolCalls: [], + }; + }, + }; + }, + runBinary: async () => ({ ok: true, stdout: "" }), + readCache: async (key: string) => { + log.readKeys.push(key); + + return null; + }, + persistArtifact: async ( + _v: IVerdict, + key: string, + rosterHash: string + ) => { + log.persisted.push({ key, rosterHash }); + }, + }, + }; +} + +describe("buildReviewFlowDeps (CLI wiring)", () => { + test("rosterHash is derived from the EFFECTIVE panel + builder (not the raw config)", () => { + const { input } = makeInput({}); + const deps = buildReviewFlowDeps(input); + + expect(deps.rosterHash).toBe(panelIdentityHash(effective, "local/flash")); + }); + + test("mode maps from --quick and ci maps from --ci", () => { + expect(buildReviewFlowDeps(makeInput({ quick: false }).input).mode).toBe( + "full" + ); + expect(buildReviewFlowDeps(makeInput({ quick: true }).input).mode).toBe( + "quick" + ); + expect(buildReviewFlowDeps(makeInput({ ci: true }).input).ci).toBe(true); + expect(buildReviewFlowDeps(makeInput({ ci: false }).input).ci).toBe(false); + }); + + test("gather runs validate FRESH and diffs the pinned base against HEAD", async () => { + const { log, input } = makeInput({}); + const deps = buildReviewFlowDeps(input); + + const gathered = await deps.gather(); + + expect(gathered.kind).toBe("request"); + expect(log.validated).toBe(1); // validate ran inside gather + expect( + log.gitArgs.some((a) => a.join(" ").includes("MBSHA...HEADSHA")) + ).toBe(true); + }); + + test("review targets the EFFECTIVE panel (the effective reviewer's model provider is invoked)", async () => { + const { log, input } = makeInput({}); + const deps = buildReviewFlowDeps(input); + + await deps.review(request); + + expect(log.providerModels).toContain("MODEL-X"); + }); + + test("persist carries the EFFECTIVE rosterHash (the artifact records the roster that actually reviewed)", async () => { + const { log, input } = makeInput({}); + const deps = buildReviewFlowDeps(input); + const verdict: IVerdict = { + blocked: false, + reason: "ok", + reviewers: { ok: 1, errored: 0 }, + ranked: [], + perReviewer: [], + identity: "local/flash", + }; + + await deps.persist(verdict, "KEY"); + + expect(log.persisted).toHaveLength(1); + expect(log.persisted[0]?.key).toBe("KEY"); + expect(log.persisted[0]?.rosterHash).toBe( + panelIdentityHash(effective, "local/flash") + ); + }); +}); diff --git a/packages/core/tests/reviewers-artifact.test.ts b/packages/core/tests/reviewers-artifact.test.ts index 4e0cf002..343b50a4 100644 --- a/packages/core/tests/reviewers-artifact.test.ts +++ b/packages/core/tests/reviewers-artifact.test.ts @@ -10,6 +10,7 @@ import { CACHE_VERSION, } from "../src/reviewers/harness-review"; import type { GatherResult } from "../src/reviewers/harness-review"; +import type { ResolvedReviewer } from "../src/reviewers/registry"; import type { IReviewRequest } from "../src/reviewers/schema"; import type { IVerdict } from "../src/reviewers/aggregate"; @@ -92,6 +93,24 @@ describe("reviewRequestKey (cache key = fingerprint of the ACTUAL review request ); }); + test("canonical (key-sorted) serialization: SAME content in a different key insertion order hashes equally (no thrash / divergence)", () => { + // A second construction path (or a JS engine detail) that emits the request's keys in a + // different order must not change the digest, or the cache would thrash / a re-gather + // could miss. The recursive key-sort guarantees content-equality ⇒ digest-equality. + const reordered: IReviewRequest = { + rubricVersion: request.rubricVersion, + contextFiles: request.contextFiles, + validateSummary: request.validateSummary, + diff: request.diff, + intent: request.intent, + title: request.title, + }; + + expect(reviewRequestKey(reordered, rosterOpts)).toBe( + reviewRequestKey(request, rosterOpts) + ); + }); + test("CACHE_VERSION is mixed into the key — bumping it retires ALL legacy artifacts in one shot", () => { // The ONLY lever that invalidates every already-on-disk artifact (e.g. legacy diff-hash // keys, or a poisoned pre-review block). If a regression dropped CACHE_VERSION from the @@ -127,12 +146,19 @@ describe("reviewRequestKey (cache key = fingerprint of the ACTUAL review request }); describe("panelIdentityHash (the roster that ACTUALLY reviewed keys the cache)", () => { - const grok = { + const grok: ResolvedReviewer = { kind: "model", id: "grok", entry: { baseUrl: "http://a", model: "g1" }, }; - const codex = { kind: "binary", id: "codex", argv: ["codex"] }; + const codex: ResolvedReviewer = { + kind: "binary", + id: "codex", + argv: ["codex"], + input: "stdin", + timeoutMs: 1000, + parse: "raw", + }; const panel = { reviewers: [grok, codex], minReviewers: 2 }; test("stable and order-independent (roster is sorted by id before hashing)", () => { @@ -159,7 +185,7 @@ describe("panelIdentityHash (the roster that ACTUALLY reviewed keys the cache)", test("RETARGETING the same id to a different model/endpoint/binary changes the key (reviewer implementation is pinned, not just its id)", () => { // The panel finding: hashing ids alone would reuse a verdict produced by a DIFFERENT // reviewer implementation. Full config is hashed, so same-id-different-model differs. - const grokRetargeted = { + const grokRetargeted: ResolvedReviewer = { kind: "model", id: "grok", entry: { baseUrl: "http://a", model: "g2-DIFFERENT" }, @@ -183,16 +209,28 @@ describe("runReviewFlow (the CLI wiring invariant: gather-before-cache, block-ne read: number; review: number; persist: string[]; + /** Ordered log of every seam as it fires, so call ORDER (not just counts) is asserted. */ + order: string[]; + /** The key readCache was called with (null if never). */ + readKey: string | null; } // A flow harness: `gathered` is what gather() returns (block or request); records the // order/counts so the wiring invariant can be asserted, and captures the key each cache // op received (to prove it derives from the gathered request). const harness = (gathered: GatherResult, cached: IVerdict | null) => { - const calls: ICalls = { gather: 0, read: 0, review: 0, persist: [] }; + const calls: ICalls = { + gather: 0, + read: 0, + review: 0, + persist: [], + order: [], + readKey: null, + }; const deps = { gather: async () => { calls.gather += 1; + calls.order.push("gather"); return gathered; }, @@ -202,16 +240,19 @@ describe("runReviewFlow (the CLI wiring invariant: gather-before-cache, block-ne ci: false, readCache: async (key: string) => { calls.read += 1; - void key; + calls.order.push("read"); + calls.readKey = key; return cached; }, review: async (_request: IReviewRequest) => { calls.review += 1; + calls.order.push("review"); return reviewVerdict; }, persist: async (_verdict: IVerdict, key: string) => { + calls.order.push("persist"); calls.persist.push(key); }, }; @@ -239,29 +280,31 @@ describe("runReviewFlow (the CLI wiring invariant: gather-before-cache, block-ne expect(calls.persist).toHaveLength(0); }); - test("gather runs BEFORE any cache access, and the cache key is derived from the gathered request", async () => { + test("gather runs strictly BEFORE any cache access, and the READ + WRITE keys are exactly reviewRequestKey(gathered request)", async () => { const { calls, deps } = harness(requestResult, null); await runReviewFlow(deps); - // The key the cache saw is exactly reviewRequestKey(gathered.request, roster/mode). const expectedKey = reviewRequestKey(request, { rosterHash: "roster-1", mode: "full", }); - expect(calls.gather).toBe(1); + // ORDER, not just counts: gather precedes the read, which precedes review+persist. + expect(calls.order).toEqual(["gather", "read", "review", "persist"]); + // Both cache ops keyed the gathered request (not the raw flags / a different base). + expect(calls.readKey).toBe(expectedKey); expect(calls.persist).toEqual([expectedKey]); }); - test("cache HIT reuses the cached verdict — gather ran, but no review, no write", async () => { + test("cache HIT reuses the cached verdict — gather ran BEFORE the read, then no review, no write", async () => { const { calls, deps } = harness(requestResult, cachedVerdict); const r = await runReviewFlow(deps); expect(r.cacheHit).toBe(true); expect(r.verdict.reason).toBe("from cache"); - expect(calls.gather).toBe(1); + expect(calls.order).toEqual(["gather", "read"]); // gathered first, then read, then stop expect(calls.review).toBe(0); expect(calls.persist).toHaveLength(0); }); diff --git a/packages/core/tests/reviewers-harness-review.test.ts b/packages/core/tests/reviewers-harness-review.test.ts index 731ab059..1f7e1099 100644 --- a/packages/core/tests/reviewers-harness-review.test.ts +++ b/packages/core/tests/reviewers-harness-review.test.ts @@ -175,6 +175,51 @@ describe("gatherChange", () => { } }); + test("resolveBase SUCCESS path: diffs merge-base-SHA...HEAD-SHA (both pinned to immutable commits, not movable refs)", async () => { + // The happy path the panel flagged as untested: merge-base returns a real SHA and HEAD is + // pinned via rev-parse, so every read is against one snapshot. Deleting the pin would make + // this fail (the range would reference `main`/`HEAD` refs, not the SHAs). + const seen: string[] = []; + + const pinnedGit: IGatherDeps["git"] = async (args) => { + const key = args.join(" "); + + if (args[0] === "rev-parse") { + return { stdout: "HEADSHA\n", code: 0 }; + } + + if (args[0] === "merge-base") { + return { stdout: "MBSHA\n", code: 0 }; + } + + if (args[0] === "diff") { + seen.push(key); + + return key.includes("--name-only") + ? { stdout: "x.ts", code: 0 } + : { stdout: "diff --git a/x b/x\n+code", code: 0 }; + } + + if (args[0] === "show") { + seen.push(key); + + return { stdout: "file content", code: 0 }; + } + + return { stdout: "", code: 0 }; + }; + + const r = await gatherChange( + { git: pinnedGit, validate: cleanValidate }, + { ...opts, base: "main" } + ); + + expect(r.kind).toBe("request"); + // The diff range and the context `show` both reference the pinned SHAs. + expect(seen.some((k) => k.includes("MBSHA...HEADSHA"))).toBe(true); + expect(seen.some((k) => k.includes("show HEADSHA:x.ts"))).toBe(true); + }); + test("resolveBase merge-base failure falls back to the ref and still diffs (shallow/odd repos)", async () => { // merge-base returns empty (failure); gather must still resolve a range and produce a // request rather than throw. Records the range actually diffed. From 47f15ed96bbc4e4b8aabb2c8ce098971de79fb7b Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Tue, 28 Jul 2026 01:28:15 +0200 Subject: [PATCH 09/11] fix(panel): review the pushed OID via --head + honest pin + test hardening (P4 round-10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-9 (4/4) findings, all addressed: - CRITICAL: pre-push reviewed HEAD, not the pushed commit — an explicit refspec (feature:main) could push harness code the panel never saw. Added a --head target to harness-review (default HEAD); the pre-push hook now reads git's stdin and reviews each pushed local OID with --head, skipping deletes (falls back to HEAD for manual runs). - the review target is now pinned to a SHA BEFORE validate (not after), and rev-parse's exit code is honored: a failed pin BLOCKS rather than soft-falling-back to the movable HEAD ref (which would silently re-open the TOCTOU). - house rule: removed the forbidden `as` casts from the CLI wiring test (typed ILog). - added a rev-parse-failure block test; strengthened the buildReviewFlowDeps gather test so a dropped base or frozen intent is actually caught (merge-base echoes its ref; intent asserted). Full suite 3185/0; typecheck+lint+format clean; hook: bash -n OK + executable. --- .githooks/pre-push | 43 +++++++++++++++---- packages/core/src/cli/harness-review-mode.ts | 9 ++++ packages/core/src/reviewers/harness-review.ts | 38 +++++++++------- .../core/tests/harness-review-mode.test.ts | 41 +++++++++++++----- .../tests/reviewers-harness-review.test.ts | 19 ++++++++ 5 files changed, 115 insertions(+), 35 deletions(-) diff --git a/.githooks/pre-push b/.githooks/pre-push index 2ceff2c3..dc38f8c6 100755 --- a/.githooks/pre-push +++ b/.githooks/pre-push @@ -3,15 +3,40 @@ # touches harness code. The panel uses local binaries (grok, codex) and # local/keyed model endpoints, so it runs HERE (not in CI). Bypass with # --no-verify. (core-ci.yml still enforces the code gate — typecheck/lint/test.) +# +# git feeds the pushed refs on stdin as: . +# Review the ACTUAL pushed commit(s) via `--head ` — NOT whatever HEAD happens to be — +# so an explicit refspec (e.g. `git push origin feature:main`) can't push harness code the +# panel never saw. Deletes (all-zero local oid) are skipped. set -euo pipefail -base="$(git merge-base main HEAD 2>/dev/null || echo HEAD~1)" -if git diff --name-only "$base"...HEAD | grep -q '^packages/core/'; then - echo "harness-review: harness code changed — running the review panel..." - bun run packages/core/src/cli.ts harness-review || { - echo "harness-review BLOCKED the push. Fix findings or --no-verify." >&2 - exit 1 - } -else - echo "harness-review: no packages/core changes — skipping panel." +zero="0000000000000000000000000000000000000000" + +review_target() { + local sha="$1" + local base + base="$(git merge-base main "$sha" 2>/dev/null || echo "${sha}~1")" + if git diff --name-only "$base"..."$sha" | grep -q '^packages/core/'; then + echo "harness-review: harness code changed in ${sha} — running the review panel..." + bun run packages/core/src/cli.ts harness-review --head "$sha" || { + echo "harness-review BLOCKED the push. Fix findings or --no-verify." >&2 + exit 1 + } + else + echo "harness-review: no packages/core changes in ${sha} — skipping panel." + fi +} + +reviewed_any=0 +while read -r _local_ref local_oid _remote_ref _remote_oid; do + if [ "$local_oid" = "$zero" ]; then + continue # a branch/tag delete — nothing to review + fi + review_target "$local_oid" + reviewed_any=1 +done + +# No stdin (a manual invocation, not a real pre-push) — fall back to reviewing HEAD. +if [ "$reviewed_any" -eq 0 ]; then + review_target "HEAD" fi diff --git a/packages/core/src/cli/harness-review-mode.ts b/packages/core/src/cli/harness-review-mode.ts index f9fc4f08..0aefd588 100644 --- a/packages/core/src/cli/harness-review-mode.ts +++ b/packages/core/src/cli/harness-review-mode.ts @@ -32,6 +32,8 @@ import { parseVerdict, type IVerdict } from "../reviewers/aggregate"; interface IArgs { base: string | undefined; intent: string | undefined; + /** The commit to review — the pre-push hook passes the actual pushed local OID. Default HEAD. */ + head: string | undefined; quick: boolean; ci: boolean; installHook: boolean; @@ -41,6 +43,7 @@ function parse(argv: string[]): IArgs { const out: IArgs = { base: undefined, intent: undefined, + head: undefined, quick: false, ci: false, installHook: false, @@ -61,6 +64,9 @@ function parse(argv: string[]): IArgs { } else if (a === "--base") { i += 1; out.base = argv[i]; + } else if (a === "--head") { + i += 1; + out.head = argv[i]; } } @@ -292,6 +298,7 @@ export function buildReviewFlowDeps(input: { ci: boolean; base: string | undefined; intent: string | undefined; + head: string | undefined; git: IGitRunner; validate: IValidateRunner; makeProvider: IReviewDeps["makeProvider"]; @@ -312,6 +319,7 @@ export function buildReviewFlowDeps(input: { { base: input.base, intent: input.intent, + head: input.head, maxFiles: DEFAULT_MAX_FILES, maxChars: DEFAULT_MAX_CHARS, } @@ -372,6 +380,7 @@ export async function harnessReviewMode(argv: string[]): Promise { ci: args.ci, base: args.base, intent: args.intent, + head: args.head, git: gitRunner, validate: validateRunner, makeProvider, diff --git a/packages/core/src/reviewers/harness-review.ts b/packages/core/src/reviewers/harness-review.ts index 2d7cf379..7093c595 100644 --- a/packages/core/src/reviewers/harness-review.ts +++ b/packages/core/src/reviewers/harness-review.ts @@ -33,6 +33,10 @@ export interface IGatherDeps { export interface IGatherOptions { base?: string; intent?: string; + /** The commit to review (a SHA/ref). Defaults to HEAD. The pre-push hook passes the ACTUAL + * pushed local OID here, so an explicit refspec pushing a non-HEAD ref is reviewed as what + * is being pushed — not whatever HEAD happens to be. */ + head?: string; maxFiles: number; maxChars: number; } @@ -41,18 +45,6 @@ export type GatherResult = | { kind: "request"; request: IReviewRequest } | { kind: "block"; reason: string }; -/** Pin HEAD to an immutable commit SHA once, so every subsequent read in a gather (the diff, - * the file list, the context `show`, the subject `log`) references the SAME snapshot. If - * HEAD moves mid-gather, an unpinned gather would combine file names, diff, and context from - * DIFFERENT commits — the gate would then approve a scope that isn't the commit being pushed - * (a TOCTOU the panel flagged). Falls back to the literal "HEAD" only if rev-parse fails. */ -async function resolveHead(git: IGitRunner): Promise { - const res = await git(["rev-parse", "HEAD"]); - const head = res.stdout.trim(); - - return head.length > 0 ? head : "HEAD"; -} - async function resolveBase( git: IGitRunner, explicit: string | undefined, @@ -95,6 +87,25 @@ export async function gatherChange( deps: IGatherDeps, opts: IGatherOptions ): Promise { + // Pin the review target (opts.head, default HEAD) to an immutable SHA FIRST — before + // validate and every diff — so the whole gather references one snapshot and validate's + // result is attributed to a fixed commit. Guard the exit code like every other git step: a + // failed pin BLOCKS (a soft fallback to the movable "HEAD" would silently re-open the + // TOCTOU). (Validate still runs against the working tree; for the pre-push gate that tree is + // the pinned commit — an unstaged divergence from it is the caller's responsibility.) + const target = opts.head ?? "HEAD"; + const headRes = await deps.git(["rev-parse", target]); + + if (headRes.code !== 0) { + return { + kind: "block", + reason: `could not resolve the review target (git rev-parse ${target} exited ${String(headRes.code)}) — cannot review`, + }; + } + + const head = + headRes.stdout.trim().length > 0 ? headRes.stdout.trim() : target; + const validateSummary = await deps.validate(); if (!validateSummary.passed) { @@ -104,9 +115,6 @@ export async function gatherChange( }; } - // Pin HEAD to a SHA ONCE, then read the file list, diff, subject, and context all against - // that same snapshot — so a HEAD move mid-gather can't mix commits into one request. - const head = await resolveHead(deps.git); const base = await resolveBase(deps.git, opts.base, head); const intent = await resolveIntent(deps.git, opts.intent, head); diff --git a/packages/core/tests/harness-review-mode.test.ts b/packages/core/tests/harness-review-mode.test.ts index 3c46b8f7..78acaa4e 100644 --- a/packages/core/tests/harness-review-mode.test.ts +++ b/packages/core/tests/harness-review-mode.test.ts @@ -34,13 +34,21 @@ const request: IReviewRequest = { rubricVersion: "1", }; +interface ILog { + validated: number; + gitArgs: string[][]; + providerModels: string[]; + readKeys: string[]; + persisted: { key: string; rosterHash: string }[]; +} + function makeInput(over: { quick?: boolean; ci?: boolean }) { - const log = { + const log: ILog = { validated: 0, - gitArgs: [] as string[][], - providerModels: [] as string[], - readKeys: [] as string[], - persisted: [] as { key: string; rosterHash: string }[], + gitArgs: [], + providerModels: [], + readKeys: [], + persisted: [], }; return { @@ -50,19 +58,22 @@ function makeInput(over: { quick?: boolean; ci?: boolean }) { identity: "local/flash", quick: over.quick ?? false, ci: over.ci ?? false, - base: "main", - intent: "do the thing", + // Distinctive base/intent so a wiring that drops or overrides them is caught. + base: "customBase", + intent: "DISTINCT-INTENT", + head: undefined as string | undefined, git: async (args: string[]) => { log.gitArgs.push(args); // Enough for gatherChange to build a request: rev-parse/merge-base SHAs, a file + - // a non-empty diff, readable context. + // a non-empty diff, readable context. merge-base ECHOES its ref arg so the diff range + // proves WHICH base was used (a dropped base would echo "main", not "customBase"). if (args[0] === "rev-parse") { return { stdout: "HEADSHA\n", code: 0 }; } if (args[0] === "merge-base") { - return { stdout: "MBSHA\n", code: 0 }; + return { stdout: `mb-${String(args[1])}\n`, code: 0 }; } if (args[0] === "diff") { @@ -130,7 +141,7 @@ describe("buildReviewFlowDeps (CLI wiring)", () => { expect(buildReviewFlowDeps(makeInput({ ci: false }).input).ci).toBe(false); }); - test("gather runs validate FRESH and diffs the pinned base against HEAD", async () => { + test("gather runs validate FRESH and uses args.base + args.intent (a dropped base or frozen intent is caught)", async () => { const { log, input } = makeInput({}); const deps = buildReviewFlowDeps(input); @@ -138,8 +149,16 @@ describe("buildReviewFlowDeps (CLI wiring)", () => { expect(gathered.kind).toBe("request"); expect(log.validated).toBe(1); // validate ran inside gather + + if (gathered.kind === "request") { + // args.intent flowed through (not frozen to some other string). + expect(gathered.request.intent).toBe("DISTINCT-INTENT"); + } + + // The diff range proves the CONFIGURED base ("customBase") was used — merge-base echoes + // its ref, so a dropped base would show "mb-main...HEADSHA" and fail this. expect( - log.gitArgs.some((a) => a.join(" ").includes("MBSHA...HEADSHA")) + log.gitArgs.some((a) => a.join(" ").includes("mb-customBase...HEADSHA")) ).toBe(true); }); diff --git a/packages/core/tests/reviewers-harness-review.test.ts b/packages/core/tests/reviewers-harness-review.test.ts index 1f7e1099..47eec345 100644 --- a/packages/core/tests/reviewers-harness-review.test.ts +++ b/packages/core/tests/reviewers-harness-review.test.ts @@ -94,6 +94,25 @@ describe("gatherChange", () => { } }); + test("a failing `git rev-parse` of the review target BLOCKS (no soft fallback to a moving HEAD)", async () => { + // The pin must be honest: on rev-parse failure, block — do NOT silently review the movable + // "HEAD" ref (which would re-open the TOCTOU the pin exists to close). + const failingHead: IGatherDeps["git"] = async (args) => + args[0] === "rev-parse" + ? { stdout: "", code: 128 } + : { stdout: "", code: 0 }; + const r = await gatherChange( + { git: failingHead, validate: cleanValidate }, + opts + ); + + expect(r.kind).toBe("block"); + + if (r.kind === "block") { + expect(r.reason).toMatch(/could not resolve the review target/iu); + } + }); + test("a failing `git diff --name-only` BLOCKS instead of building an empty review (exit code honored)", async () => { // The false-green the panel flagged: if git errors and returns empty stdout, an // unguarded gather would build a 0-file, empty-diff request that the panel green-lights From 919cf5385173777db7778f3e63b0d4356c6c1c93 Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Tue, 28 Jul 2026 01:40:31 +0200 Subject: [PATCH 10/11] =?UTF-8?q?fix(panel):=20scope=20P4=20to=20cache=20i?= =?UTF-8?q?ntegrity=20=E2=80=94=20revert=20the=20pushed-OID=20targeting=20?= =?UTF-8?q?feature=20(round-11)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-10 showed the round-10 --head/pushed-OID feature was a scope error: reviewing an arbitrary pushed commit requires validating THAT commit's tree (not the working tree) — a checkout/worktree-level change with its own base=remote_oid, artifact-tree, and hook robustness concerns. That is review-TARGETING integrity, a separate feature from P4's verdict-cache integrity, and can't be bolted on correctly here. - reverted the --head flag (harness-review + parse + buildReviewFlowDeps) and restored the pre-push hook to its original HEAD review (now identical to main → off the review surface). - KEPT the in-scope, cache-relevant HEAD-snapshot fix: gatherChange pins HEAD to a SHA once (rev-parse, exit-code-guarded → blocks on failure) and reads the file list/diff/subject/ context all against it, so the fingerprint is of one consistent commit (the round-8 TOCTOU). - KEPT all cache-integrity work: whole-request key (canonical), effective-roster hash (typed to ResolvedReviewer), gather-before-cache runReviewFlow, git-exit/empty-diff guards, and the CLI-wiring + order + merge-base-success + canonical-order tests. Review-targeting integrity (validate the pushed commit's tree; review the pushed OID, not HEAD) is recorded as a separate follow-up. Full suite 3185/0; typecheck+lint+format clean. --- .githooks/pre-push | 43 ++++--------------- packages/core/src/cli/harness-review-mode.ts | 9 ---- packages/core/src/reviewers/harness-review.ts | 23 ++++------ .../core/tests/harness-review-mode.test.ts | 1 - .../tests/reviewers-harness-review.test.ts | 4 +- 5 files changed, 20 insertions(+), 60 deletions(-) diff --git a/.githooks/pre-push b/.githooks/pre-push index dc38f8c6..2ceff2c3 100755 --- a/.githooks/pre-push +++ b/.githooks/pre-push @@ -3,40 +3,15 @@ # touches harness code. The panel uses local binaries (grok, codex) and # local/keyed model endpoints, so it runs HERE (not in CI). Bypass with # --no-verify. (core-ci.yml still enforces the code gate — typecheck/lint/test.) -# -# git feeds the pushed refs on stdin as: . -# Review the ACTUAL pushed commit(s) via `--head ` — NOT whatever HEAD happens to be — -# so an explicit refspec (e.g. `git push origin feature:main`) can't push harness code the -# panel never saw. Deletes (all-zero local oid) are skipped. set -euo pipefail -zero="0000000000000000000000000000000000000000" - -review_target() { - local sha="$1" - local base - base="$(git merge-base main "$sha" 2>/dev/null || echo "${sha}~1")" - if git diff --name-only "$base"..."$sha" | grep -q '^packages/core/'; then - echo "harness-review: harness code changed in ${sha} — running the review panel..." - bun run packages/core/src/cli.ts harness-review --head "$sha" || { - echo "harness-review BLOCKED the push. Fix findings or --no-verify." >&2 - exit 1 - } - else - echo "harness-review: no packages/core changes in ${sha} — skipping panel." - fi -} - -reviewed_any=0 -while read -r _local_ref local_oid _remote_ref _remote_oid; do - if [ "$local_oid" = "$zero" ]; then - continue # a branch/tag delete — nothing to review - fi - review_target "$local_oid" - reviewed_any=1 -done - -# No stdin (a manual invocation, not a real pre-push) — fall back to reviewing HEAD. -if [ "$reviewed_any" -eq 0 ]; then - review_target "HEAD" +base="$(git merge-base main HEAD 2>/dev/null || echo HEAD~1)" +if git diff --name-only "$base"...HEAD | grep -q '^packages/core/'; then + echo "harness-review: harness code changed — running the review panel..." + bun run packages/core/src/cli.ts harness-review || { + echo "harness-review BLOCKED the push. Fix findings or --no-verify." >&2 + exit 1 + } +else + echo "harness-review: no packages/core changes — skipping panel." fi diff --git a/packages/core/src/cli/harness-review-mode.ts b/packages/core/src/cli/harness-review-mode.ts index 0aefd588..f9fc4f08 100644 --- a/packages/core/src/cli/harness-review-mode.ts +++ b/packages/core/src/cli/harness-review-mode.ts @@ -32,8 +32,6 @@ import { parseVerdict, type IVerdict } from "../reviewers/aggregate"; interface IArgs { base: string | undefined; intent: string | undefined; - /** The commit to review — the pre-push hook passes the actual pushed local OID. Default HEAD. */ - head: string | undefined; quick: boolean; ci: boolean; installHook: boolean; @@ -43,7 +41,6 @@ function parse(argv: string[]): IArgs { const out: IArgs = { base: undefined, intent: undefined, - head: undefined, quick: false, ci: false, installHook: false, @@ -64,9 +61,6 @@ function parse(argv: string[]): IArgs { } else if (a === "--base") { i += 1; out.base = argv[i]; - } else if (a === "--head") { - i += 1; - out.head = argv[i]; } } @@ -298,7 +292,6 @@ export function buildReviewFlowDeps(input: { ci: boolean; base: string | undefined; intent: string | undefined; - head: string | undefined; git: IGitRunner; validate: IValidateRunner; makeProvider: IReviewDeps["makeProvider"]; @@ -319,7 +312,6 @@ export function buildReviewFlowDeps(input: { { base: input.base, intent: input.intent, - head: input.head, maxFiles: DEFAULT_MAX_FILES, maxChars: DEFAULT_MAX_CHARS, } @@ -380,7 +372,6 @@ export async function harnessReviewMode(argv: string[]): Promise { ci: args.ci, base: args.base, intent: args.intent, - head: args.head, git: gitRunner, validate: validateRunner, makeProvider, diff --git a/packages/core/src/reviewers/harness-review.ts b/packages/core/src/reviewers/harness-review.ts index 7093c595..514730a0 100644 --- a/packages/core/src/reviewers/harness-review.ts +++ b/packages/core/src/reviewers/harness-review.ts @@ -33,10 +33,6 @@ export interface IGatherDeps { export interface IGatherOptions { base?: string; intent?: string; - /** The commit to review (a SHA/ref). Defaults to HEAD. The pre-push hook passes the ACTUAL - * pushed local OID here, so an explicit refspec pushing a non-HEAD ref is reviewed as what - * is being pushed — not whatever HEAD happens to be. */ - head?: string; maxFiles: number; maxChars: number; } @@ -87,24 +83,23 @@ export async function gatherChange( deps: IGatherDeps, opts: IGatherOptions ): Promise { - // Pin the review target (opts.head, default HEAD) to an immutable SHA FIRST — before - // validate and every diff — so the whole gather references one snapshot and validate's - // result is attributed to a fixed commit. Guard the exit code like every other git step: a - // failed pin BLOCKS (a soft fallback to the movable "HEAD" would silently re-open the - // TOCTOU). (Validate still runs against the working tree; for the pre-push gate that tree is - // the pinned commit — an unstaged divergence from it is the caller's responsibility.) - const target = opts.head ?? "HEAD"; - const headRes = await deps.git(["rev-parse", target]); + // Pin HEAD to an immutable SHA FIRST — before validate and every diff — so the whole gather + // references one snapshot (a HEAD move mid-gather can't mix file names, diff, and context + // from different commits into one request). Honor rev-parse's exit code like every other git + // step: a failed pin BLOCKS (a soft fallback to the movable "HEAD" ref would re-open the + // TOCTOU). (Reviewing an arbitrary pushed OID — validating THAT commit's tree rather than the + // working tree — is a separate review-targeting concern, tracked apart from cache integrity.) + const headRes = await deps.git(["rev-parse", "HEAD"]); if (headRes.code !== 0) { return { kind: "block", - reason: `could not resolve the review target (git rev-parse ${target} exited ${String(headRes.code)}) — cannot review`, + reason: `could not resolve HEAD (git rev-parse exited ${String(headRes.code)}) — cannot review`, }; } const head = - headRes.stdout.trim().length > 0 ? headRes.stdout.trim() : target; + headRes.stdout.trim().length > 0 ? headRes.stdout.trim() : "HEAD"; const validateSummary = await deps.validate(); diff --git a/packages/core/tests/harness-review-mode.test.ts b/packages/core/tests/harness-review-mode.test.ts index 78acaa4e..a1c877d5 100644 --- a/packages/core/tests/harness-review-mode.test.ts +++ b/packages/core/tests/harness-review-mode.test.ts @@ -61,7 +61,6 @@ function makeInput(over: { quick?: boolean; ci?: boolean }) { // Distinctive base/intent so a wiring that drops or overrides them is caught. base: "customBase", intent: "DISTINCT-INTENT", - head: undefined as string | undefined, git: async (args: string[]) => { log.gitArgs.push(args); diff --git a/packages/core/tests/reviewers-harness-review.test.ts b/packages/core/tests/reviewers-harness-review.test.ts index 47eec345..aa353edc 100644 --- a/packages/core/tests/reviewers-harness-review.test.ts +++ b/packages/core/tests/reviewers-harness-review.test.ts @@ -94,7 +94,7 @@ describe("gatherChange", () => { } }); - test("a failing `git rev-parse` of the review target BLOCKS (no soft fallback to a moving HEAD)", async () => { + test("a failing `git rev-parse HEAD` BLOCKS (no soft fallback to a moving HEAD ref)", async () => { // The pin must be honest: on rev-parse failure, block — do NOT silently review the movable // "HEAD" ref (which would re-open the TOCTOU the pin exists to close). const failingHead: IGatherDeps["git"] = async (args) => @@ -109,7 +109,7 @@ describe("gatherChange", () => { expect(r.kind).toBe("block"); if (r.kind === "block") { - expect(r.reason).toMatch(/could not resolve the review target/iu); + expect(r.reason).toMatch(/could not resolve HEAD/iu); } }); From ee48e13838c477d1145f866636d65c3213df5ec6 Mon Sep 17 00:00:00 2001 From: Aleksandar Grbic Date: Tue, 28 Jul 2026 01:56:40 +0200 Subject: [PATCH 11/11] =?UTF-8?q?harden(panel):=20complete=20the=20HEAD/ba?= =?UTF-8?q?se=20pin=20=E2=80=94=20always=20a=20SHA=20or=20block=20(P4=20ro?= =?UTF-8?q?und-11=20advisory)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-11 PASSED but 6 unanimous advisory findings converged on one real gap: the pin soft-fell-back to a MOVABLE ref (rev-parse empty-on-success → 'HEAD'; merge-base failure → the ref / HEAD~1), which the code's own comments say re-opens the mid-gather TOCTOU. Closed it: - new resolveSha(): a pin must be a real SHA — rev-parse with a nonzero exit OR empty stdout yields null. gatherChange blocks on a null HEAD (no soft fallback to the movable ref). - resolveBase honors merge-base's exit code and, on failure, pins the fallback ref to a SHA via resolveSha (or blocks) — the diff never runs against a movable ref, so name-only/content observe ONE snapshot. - moved the quick-mode roster slice INTO buildReviewFlowDeps so the effective-roster wiring (rosterHash + review target the sliced roster) is unit-tested. - tests: pinned-fallback range, unpinnable-fallback blocks, empty-rev-parse blocks, quick-slice roster hash; git test helper defaults rev-parse/merge-base to SHAs. Full suite 3188/0; typecheck+lint+format clean. --- packages/core/src/cli/harness-review-mode.ts | 32 ++--- packages/core/src/reviewers/harness-review.ts | 65 ++++++---- .../core/tests/harness-review-mode.test.ts | 39 +++++- .../tests/reviewers-harness-review.test.ts | 115 ++++++++++++++++-- 4 files changed, 204 insertions(+), 47 deletions(-) diff --git a/packages/core/src/cli/harness-review-mode.ts b/packages/core/src/cli/harness-review-mode.ts index f9fc4f08..1ba7a216 100644 --- a/packages/core/src/cli/harness-review-mode.ts +++ b/packages/core/src/cli/harness-review-mode.ts @@ -279,14 +279,15 @@ export function formatVerdict(v: IVerdict): string { } /** - * Wire the resolved CLI pieces (effective roster, args, git/validate, providers, cache - * seams) into the runReviewFlow deps — exported so the CLI's central wiring is unit-tested: - * the cache key's rosterHash is derived from the EFFECTIVE roster (not cfg), the review - * targets the effective panel, mode/ci come from the args, and gather reads args.base/intent. - * A miswire (cfg roster, hardcoded ci, wrong panel) is caught here rather than only in prod. + * Wire the resolved CLI pieces (full panel + quick flag, args, git/validate, providers, cache + * seams) into the runReviewFlow deps — exported so the CLI's central wiring is unit-tested. + * The EFFECTIVE roster is derived HERE (quick mode → a 1-reviewer slice), so the cache key's + * rosterHash and the review both target that effective roster — a `quick` run can't reuse a + * full-panel verdict, or vice versa. mode/ci come from the args; gather reads args.base/intent. + * A miswire (cfg roster, hardcoded ci, wrong panel, un-sliced quick roster) is caught here. */ export function buildReviewFlowDeps(input: { - effective: IPanel; + panel: IPanel; identity: string; quick: boolean; ci: boolean; @@ -303,7 +304,12 @@ export function buildReviewFlowDeps(input: { rosterHash: string ) => Promise; }): IReviewFlowDeps { - const rosterHash = panelIdentityHash(input.effective, input.identity); + // `quick` reviews with a REDUCED roster (the first reviewer only). The effective roster + // feeds BOTH the cache key and the review, so its verdict never satisfies a full review. + const effective: IPanel = input.quick + ? { ...input.panel, reviewers: input.panel.reviewers.slice(0, 1) } + : input.panel; + const rosterHash = panelIdentityHash(effective, input.identity); return { gather: () => @@ -325,7 +331,7 @@ export function buildReviewFlowDeps(input: { reviewRequest(request, { makeProvider: input.makeProvider, runBinary: input.runBinary, - panel: input.effective, + panel: effective, identity: input.identity, }), persist: (v, key) => input.persistArtifact(v, key, rosterHash), @@ -351,10 +357,6 @@ export async function harnessReviewMode(argv: string[]): Promise { process.stdout.write(`skipped reviewer ${s.id}: ${s.reason}\n`); } - const effective = args.quick - ? { ...panel, reviewers: panel.reviewers.slice(0, 1) } - : panel; - const treeHashRes = await gitRunner(["write-tree"]); const treeHash = treeHashRes.stdout.trim(); const identity = `${active.name}/${active.entry.model}`; @@ -362,11 +364,11 @@ export async function harnessReviewMode(argv: string[]): Promise { // runReviewFlow enforces the wiring invariant: GATHER (validate runs fresh inside) BEFORE // any cache access, and a gather block never touches the cache. The gathered request is // keyed from its OWN bytes, so key and review can't diverge. --ci writes but never reads. - // buildReviewFlowDeps constructs the deps (roster hash from the EFFECTIVE panel etc.) and - // is unit-tested for that wiring. + // buildReviewFlowDeps derives the EFFECTIVE roster (quick-slice) and the roster hash, and is + // unit-tested for that wiring. const { verdict, cacheHit } = await runReviewFlow( buildReviewFlowDeps({ - effective, + panel, identity, quick: args.quick, ci: args.ci, diff --git a/packages/core/src/reviewers/harness-review.ts b/packages/core/src/reviewers/harness-review.ts index 514730a0..8dcc93dd 100644 --- a/packages/core/src/reviewers/harness-review.ts +++ b/packages/core/src/reviewers/harness-review.ts @@ -41,28 +41,42 @@ export type GatherResult = | { kind: "request"; request: IReviewRequest } | { kind: "block"; reason: string }; +/** Resolve a commit-ish to an immutable SHA, or null if it can't be resolved. A pin must be a + * real SHA: rev-parse succeeding with EMPTY stdout (or a nonzero exit) is NOT a valid pin — + * returning the movable ref instead would re-open the mid-gather TOCTOU this exists to close. */ +async function resolveSha( + git: IGitRunner, + ref: string +): Promise { + const res = await git(["rev-parse", ref]); + const sha = res.stdout.trim(); + + return res.code === 0 && sha.length > 0 ? sha : null; +} + async function resolveBase( git: IGitRunner, explicit: string | undefined, head: string -): Promise { +): Promise { const ref = explicit !== undefined && explicit.length > 0 ? explicit : "main"; - // Pin to the merge-base SHA against the PINNED head, not the ref. The diff is - // `${base}...${head}` (three-dot), whose true origin is merge-base(ref, head); returning - // that immutable SHA means the bytes the fingerprint hashes and the bytes the review diffs - // are ONE snapshot even if `ref` moves (and a rebase that shifts the merge-base changes it). + // Pin to the merge-base SHA against the PINNED head. The diff is `${base}...${head}` + // (three-dot), whose true origin is merge-base(ref, head); an immutable SHA means the bytes + // the fingerprint hashes and the bytes the review diffs are ONE snapshot even if `ref` moves. const res = await git(["merge-base", ref, head]); const base = res.stdout.trim(); - if (base.length > 0) { - return base; + if (res.code === 0 && base.length > 0) { + return base; // merge-base output IS a SHA } - // merge-base failed (e.g. no `main`): fall back to the ref itself (or HEAD~1 when omitted). - // Cache safety does not rest on this — if the resulting `${base}...${head}` diff can't be - // computed, gatherChange blocks (git-exit / empty-diff guards) rather than caching a - // vacuous review. - return explicit !== undefined && explicit.length > 0 ? explicit : "HEAD~1"; + // merge-base failed (e.g. shallow clone, no `main`): pin the fallback ref to a SHA too — + // NEVER diff against a movable ref, or the name-only / content diffs could observe different + // commits if the ref moves mid-gather. `head` is a SHA, so `${head}~1` is a stable target. + return resolveSha( + git, + explicit !== undefined && explicit.length > 0 ? explicit : `${head}~1` + ); } async function resolveIntent( @@ -85,22 +99,20 @@ export async function gatherChange( ): Promise { // Pin HEAD to an immutable SHA FIRST — before validate and every diff — so the whole gather // references one snapshot (a HEAD move mid-gather can't mix file names, diff, and context - // from different commits into one request). Honor rev-parse's exit code like every other git - // step: a failed pin BLOCKS (a soft fallback to the movable "HEAD" ref would re-open the - // TOCTOU). (Reviewing an arbitrary pushed OID — validating THAT commit's tree rather than the - // working tree — is a separate review-targeting concern, tracked apart from cache integrity.) - const headRes = await deps.git(["rev-parse", "HEAD"]); + // from different commits into one request). A pin that isn't a real SHA BLOCKS: no soft + // fallback to the movable "HEAD" ref, which would re-open the TOCTOU. (Reviewing an arbitrary + // pushed OID — validating THAT commit's tree, not the working tree — is a separate + // review-targeting concern, tracked apart from cache integrity.) + const head = await resolveSha(deps.git, "HEAD"); - if (headRes.code !== 0) { + if (head === null) { return { kind: "block", - reason: `could not resolve HEAD (git rev-parse exited ${String(headRes.code)}) — cannot review`, + reason: + "could not resolve HEAD to a commit (git rev-parse) — cannot review", }; } - const head = - headRes.stdout.trim().length > 0 ? headRes.stdout.trim() : "HEAD"; - const validateSummary = await deps.validate(); if (!validateSummary.passed) { @@ -111,6 +123,15 @@ export async function gatherChange( } const base = await resolveBase(deps.git, opts.base, head); + + if (base === null) { + return { + kind: "block", + reason: + "could not resolve the diff base to a commit (git merge-base/rev-parse) — cannot review", + }; + } + const intent = await resolveIntent(deps.git, opts.intent, head); if (intent === null) { diff --git a/packages/core/tests/harness-review-mode.test.ts b/packages/core/tests/harness-review-mode.test.ts index a1c877d5..a6fc1a12 100644 --- a/packages/core/tests/harness-review-mode.test.ts +++ b/packages/core/tests/harness-review-mode.test.ts @@ -54,7 +54,7 @@ function makeInput(over: { quick?: boolean; ci?: boolean }) { return { log, input: { - effective, + panel: effective, identity: "local/flash", quick: over.quick ?? false, ci: over.ci ?? false, @@ -129,6 +129,43 @@ describe("buildReviewFlowDeps (CLI wiring)", () => { expect(deps.rosterHash).toBe(panelIdentityHash(effective, "local/flash")); }); + test("quick mode slices the roster to 1 reviewer — rosterHash uses the SLICED roster (a quick verdict can't satisfy a full review)", () => { + const twoReviewers: IPanel = { + reviewers: [ + { + kind: "model", + id: "r1", + entry: { baseUrl: "http://x/v1", model: "MODEL-X" }, + }, + { + kind: "model", + id: "r2", + entry: { baseUrl: "http://y/v1", model: "MODEL-Y" }, + }, + ], + minReviewers: 1, + skipped: [], + }; + const full = buildReviewFlowDeps({ + ...makeInput({}).input, + panel: twoReviewers, + quick: false, + }); + const quick = buildReviewFlowDeps({ + ...makeInput({}).input, + panel: twoReviewers, + quick: true, + }); + + expect(quick.rosterHash).not.toBe(full.rosterHash); + expect(quick.rosterHash).toBe( + panelIdentityHash( + { ...twoReviewers, reviewers: twoReviewers.reviewers.slice(0, 1) }, + "local/flash" + ) + ); + }); + test("mode maps from --quick and ci maps from --ci", () => { expect(buildReviewFlowDeps(makeInput({ quick: false }).input).mode).toBe( "full" diff --git a/packages/core/tests/reviewers-harness-review.test.ts b/packages/core/tests/reviewers-harness-review.test.ts index aa353edc..5905fa17 100644 --- a/packages/core/tests/reviewers-harness-review.test.ts +++ b/packages/core/tests/reviewers-harness-review.test.ts @@ -12,7 +12,21 @@ function git(map: Record): IGatherDeps["git"] { const key = args.join(" "); const hit = Object.entries(map).find(([k]) => key.includes(k)); - return { stdout: hit?.[1] ?? "", code: 0 }; + if (hit) { + return { stdout: hit[1], code: 0 }; + } + + // Defaults so the HEAD pin + base resolve to SHAs unless a test overrides them (the pin + // now REQUIRES a real SHA — an empty rev-parse would block). + if (args[0] === "rev-parse") { + return { stdout: "HEADSHA", code: 0 }; + } + + if (args[0] === "merge-base") { + return { stdout: "MBSHA", code: 0 }; + } + + return { stdout: "", code: 0 }; }; } @@ -117,10 +131,20 @@ describe("gatherChange", () => { // The false-green the panel flagged: if git errors and returns empty stdout, an // unguarded gather would build a 0-file, empty-diff request that the panel green-lights // and caches. Honoring the exit code turns that into a block. - const failingGit: IGatherDeps["git"] = async (args) => - args.includes("--name-only") + const failingGit: IGatherDeps["git"] = async (args) => { + if (args[0] === "rev-parse") { + return { stdout: "HEADSHA", code: 0 }; + } + + if (args[0] === "merge-base") { + return { stdout: "MBSHA", code: 0 }; + } + + return args.includes("--name-only") ? { stdout: "", code: 128 } : { stdout: "", code: 0 }; + }; + const r = await gatherChange( { git: failingGit, validate: cleanValidate }, opts @@ -135,6 +159,14 @@ describe("gatherChange", () => { test("a failing `git diff` (content) BLOCKS even when the name list succeeded", async () => { const git2: IGatherDeps["git"] = async (args) => { + if (args[0] === "rev-parse") { + return { stdout: "HEADSHA", code: 0 }; + } + + if (args[0] === "merge-base") { + return { stdout: "MBSHA", code: 0 }; + } + if (args.includes("--name-only")) { return { stdout: "x.ts", code: 0 }; } @@ -160,6 +192,14 @@ describe("gatherChange", () => { // git anomaly / name-only↔diff disagreement (a REAL rename/mode change is non-empty). The // guard blocks it rather than build+cache a 0-byte review the panel would green-light. const git2: IGatherDeps["git"] = async (args) => { + if (args[0] === "rev-parse") { + return { stdout: "HEADSHA", code: 0 }; + } + + if (args[0] === "merge-base") { + return { stdout: "MBSHA", code: 0 }; + } + if (args.includes("--name-only")) { return { stdout: "renamed.ts", code: 0 }; } @@ -239,14 +279,22 @@ describe("gatherChange", () => { expect(seen.some((k) => k.includes("show HEADSHA:x.ts"))).toBe(true); }); - test("resolveBase merge-base failure falls back to the ref and still diffs (shallow/odd repos)", async () => { - // merge-base returns empty (failure); gather must still resolve a range and produce a - // request rather than throw. Records the range actually diffed. + test("resolveBase merge-base failure falls back to the ref resolved to a SHA (shallow/odd repos), still one snapshot", async () => { + // merge-base fails; the fallback base must be PINNED (rev-parse'd to a SHA), NOT the movable + // ref — else the name-only/content diffs could observe different commits mid-gather. Prove + // the range is ..., both immutable. const seen: string[] = []; const fallbackGit: IGatherDeps["git"] = async (args) => { const key = args.join(" "); + if (args[0] === "rev-parse") { + // HEAD → HEADSHA; the fallback base ref "featureX" → FEATURESHA. + return args[1] === "HEAD" + ? { stdout: "HEADSHA", code: 0 } + : { stdout: "FEATURESHA", code: 0 }; + } + if (args[0] === "merge-base") { return { stdout: "", code: 1 }; // no merge-base } @@ -268,8 +316,56 @@ describe("gatherChange", () => { ); expect(r.kind).toBe("request"); - // explicit ref given → falls back to that ref (not HEAD~1) for the diff range - expect(seen.some((k) => k.includes("featureX...HEAD"))).toBe(true); + // Both ends are pinned SHAs — a moving ref would have shown "featureX...HEAD" (and would + // still pass on a broken pin), which this now rejects. + expect(seen.some((k) => k.includes("FEATURESHA...HEADSHA"))).toBe(true); + }); + + test("a merge-base failure whose fallback ref ALSO can't be pinned BLOCKS (never diff a movable ref)", async () => { + const unpinnable: IGatherDeps["git"] = async (args) => { + if (args[0] === "rev-parse") { + // HEAD pins, but the fallback base ref cannot be resolved. + return args[1] === "HEAD" + ? { stdout: "HEADSHA", code: 0 } + : { stdout: "", code: 128 }; + } + + if (args[0] === "merge-base") { + return { stdout: "", code: 1 }; + } + + return { stdout: "", code: 0 }; + }; + + const r = await gatherChange( + { git: unpinnable, validate: cleanValidate }, + { ...opts, base: "ghost" } + ); + + expect(r.kind).toBe("block"); + + if (r.kind === "block") { + expect(r.reason).toMatch(/could not resolve the diff base/iu); + } + }); + + test("rev-parse HEAD succeeds but returns EMPTY stdout → still BLOCKS (an empty SHA is not a valid pin)", async () => { + // The subtle soft-fallback the panel flagged: code 0 + empty output must NOT become the + // movable "HEAD" ref. It blocks like a hard rev-parse failure. + const emptyHead: IGatherDeps["git"] = async (args) => + args[0] === "rev-parse" + ? { stdout: "\n", code: 0 } + : { stdout: "", code: 0 }; + const r = await gatherChange( + { git: emptyHead, validate: cleanValidate }, + opts + ); + + expect(r.kind).toBe("block"); + + if (r.kind === "block") { + expect(r.reason).toMatch(/could not resolve HEAD/iu); + } }); test("attaches the changed files' current (HEAD) contents as review context", async () => { @@ -277,7 +373,8 @@ describe("gatherChange", () => { git: git({ "diff --name-only": "src/x.ts", diff: "diff --git a/src/x.ts b/src/x.ts\n+code", - "show HEAD:src/x.ts": + // head is pinned to a SHA (default HEADSHA), so context reads `show :file`. + "show HEADSHA:src/x.ts": "export function realCode(): number {\n return 42;\n}", }), validate: cleanValidate,