From 5fcd9a67e0ac6c7127c3b6d856a19a5a4548c11e Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 21 Jul 2026 14:40:52 -0700 Subject: [PATCH 1/6] [jwies/review-dispatch-gate] review: block review submission when the dispatched sub-agent outputs are missing (dispatch-conformance gate) --- .changeset/review-dispatch-gate.md | 5 + workflows/review/README.md | 15 + workflows/review/lib/dispatch-gate.test.ts | 611 +++++++++++++++++++++ workflows/review/lib/dispatch-gate.ts | 570 +++++++++++++++++++ workflows/review/review.md | 21 + 5 files changed, 1222 insertions(+) create mode 100644 .changeset/review-dispatch-gate.md create mode 100644 workflows/review/lib/dispatch-gate.test.ts create mode 100644 workflows/review/lib/dispatch-gate.ts diff --git a/.changeset/review-dispatch-gate.md b/.changeset/review-dispatch-gate.md new file mode 100644 index 00000000..83fe5326 --- /dev/null +++ b/.changeset/review-dispatch-gate.md @@ -0,0 +1,5 @@ +--- +"review": minor +--- + +The dispatch-conformance gate: a review verdict can no longer be submitted unless the sub-agent outputs it is supposed to summarize actually exist. On the v1.7.0 acceptance trial (Khan/webapp#40992, run 29865480728) the orchestrator skipped its own protocol in production: it ran no router, dispatched zero sub-agents, did no claim validation, reviewed the diff itself, labeled its audit record "streamlined direct review", and submitted a REQUEST_CHANGES that disclosed none of it; the previous day's review of Khan/actions#272 dispatched correctly and disclosed its sheds, so this is stochastic non-conformance the eval suite cannot see by construction (the harness dispatches sub-agents from a script). The gate is code at the submit chokepoint, same family as v1.6.1's non-empty-body rule: a new `post-steps:` step in the agent job (`lib/dispatch-gate.ts`) runs after gh-aw finalizes the safe-output queue and before the queue ships to the `safe_outputs` job that calls the GitHub API. It checks the queued verdict and findings against the staged `out/` files per re-review depth (the correctness pass wherever the depth dispatches one, with the pattern-triage empty-`reviewFiles` waiver; a parseable `claim-validator.json` or its disclosed skipped-dimension note whenever inline comments post; a disclosure note for every reviewer routing planned that never dispatched) and, on violation, strips every posting item from the queue and fails the job: the submission is blocked rather than detected, the run goes red, and the original queue plus the gate report ride the agent artifact for diagnosis. Fail-open only for the gate's own bugs (loud warning, review unblocked); a detected violation never passes silently. diff --git a/workflows/review/README.md b/workflows/review/README.md index f87557cb..965b7cd8 100644 --- a/workflows/review/README.md +++ b/workflows/review/README.md @@ -76,6 +76,21 @@ sheds remaining work (each shed reviewer becomes a skipped-dimension note) and submits the verdict from the findings validated so far, so a run never dies at a ceiling with everything spent and nothing posted. +One more gate sits after the agent itself: the **dispatch-conformance gate** +(`lib/dispatch-gate.ts`, a `post-steps:` step in the agent job). gh-aw queues +every safe output during the agent run and executes the queue from a separate +`safe_outputs` job, so the gate runs at the hand-off: it checks the queued +verdict and findings against the staged `out/` sub-agent outputs (per re-review +depth: the correctness pass wherever the depth dispatches one, the +claim-validator whenever findings post, a disclosure note for every planned +shed) and, on violation, strips the posting items from the queue and fails the +job. A run that skipped its own dispatch protocol (observed in production: +zero sub-agents dispatched, verdict submitted, nothing disclosed) becomes a +red run that posts nothing instead of a normal-looking review; the run +artifact keeps the original queue and the gate report for diagnosis. The gate +proves the reviewer outputs were staged, not that a model authored them; +script-driven dispatch (the next migration slice) is what closes that. + ## Install ```sh diff --git a/workflows/review/lib/dispatch-gate.test.ts b/workflows/review/lib/dispatch-gate.test.ts new file mode 100644 index 00000000..54325342 --- /dev/null +++ b/workflows/review/lib/dispatch-gate.test.ts @@ -0,0 +1,611 @@ +import {describe, it, expect} from "vitest"; + +import { + disclosesSkippedDimension, + evaluateDispatchConformance, + renderGateSummary, + runDispatchGateCli, + KEEP_ITEM_TYPES, + type DispatchGateFs, + type DispatchGateInput, + type SafeOutputItem, +} from "./dispatch-gate"; + +/** + * Dispatch-conformance gate tests. + * + * The production failure this gate exists for (Khan/webapp#40992, run + * 29865480728): the orchestrator dispatched zero sub-agents, wrote a single + * self-authored `out/orchestrator-findings.json` calling the run a + * "streamlined direct review", and queued a REQUEST_CHANGES with three inline + * comments and a bare body. The contrasting conforming run (Khan/actions#272, + * 2026-07-20) staged one `out/.json` per dispatched reviewer and + * disclosed its missing validator with exactly "Note: claim validation not + * assessed this run (claim-validator output unavailable)." Both shapes are + * reproduced below verbatim from the downloaded run artifacts. + * + * NOTE: the eval suite cannot cover this failure class by construction (the + * harness dispatches sub-agents from a script, so protocol fidelity is + * exactly what it never exercises); these deterministic tests are the whole + * coverage story for the gate. + */ + +const submitItem = (event: string, body = ""): SafeOutputItem => ({ + type: "submit_pull_request_review", + event, + body, +}); + +const commentItem = (line = 1): SafeOutputItem => ({ + type: "create_pull_request_review_comment", + path: "a.ts", + line, + body: "**issue (blocking):** x", +}); + +const uploadItem: SafeOutputItem = {type: "upload_artifact", path: "out"}; + +/** A fully conforming full-depth staging (the Khan/actions#272 shape). */ +const conformingOutFiles = (): Record => ({ + "pattern-triage.json": JSON.stringify({ + patterns: [], + reviewFiles: ["a.ts"], + }), + "correctness-reviewer.json": JSON.stringify({findings: [], files: []}), + "claim-validator.json": JSON.stringify({verifications: []}), + "rereview-plan.json": JSON.stringify({depth: "full"}), +}); + +const evaluate = (overrides: Partial) => + evaluateDispatchConformance({ + items: [], + plan: undefined, + routing: undefined, + outFiles: {}, + ...overrides, + }); + +describe("evaluateDispatchConformance", () => { + it("flags the webapp#40992 freelance shape: verdict and findings with zero dispatches", () => { + // Reproduced from the run 29865480728 artifacts: three comments, a + // REQUEST_CHANGES with the bare non-empty-body line, no routing, no + // plan, and only the self-authored findings file in out/. + const result = evaluate({ + items: [ + commentItem(39), + commentItem(70), + commentItem(13), + submitItem( + "REQUEST_CHANGES", + "Changes requested — see inline comments.", + ), + uploadItem, + ], + outFiles: { + "orchestrator-findings.json": JSON.stringify({ + process: "streamlined direct review", + }), + }, + }); + expect(result.conformant).toBe(false); + expect(result.depth).toBe("full"); + expect(result.violations.map((v) => v.code)).toEqual([ + "correctness-missing", + "validator-missing-with-findings", + ]); + expect(result.verdictEvent).toBe("REQUEST_CHANGES"); + expect(result.commentCount).toBe(3); + }); + + it("passes the Khan/actions#272 conforming shape: dispatched roster, validator gap disclosed", () => { + const outFiles = conformingOutFiles(); + delete outFiles["claim-validator.json"]; + const result = evaluate({ + items: [ + commentItem(), + submitItem( + "APPROVE", + "Note: claim validation not assessed this run (claim-validator output unavailable).", + ), + uploadItem, + ], + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles, + }); + expect(result.violations).toEqual([]); + expect(result.conformant).toBe(true); + }); + + it("passes a fully-staged conforming run with findings", () => { + const result = evaluate({ + items: [ + commentItem(), + submitItem( + "REQUEST_CHANGES", + "Changes requested — see inline comments.", + ), + ], + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles: conformingOutFiles(), + }); + expect(result.conformant).toBe(true); + }); + + it("is trivially conformant when nothing posting is queued", () => { + // The redundant-approval skip (Step 6) queues no submission at all. + const result = evaluate({items: [uploadItem]}); + expect(result.conformant).toBe(true); + expect(result.verdictEvent).toBeNull(); + expect(result.commentCount).toBe(0); + }); + + describe("per depth mode", () => { + it("requires the correctness pass at full, scoped, and flip-gated", () => { + for (const depth of ["full", "scoped", "flip-gated"]) { + const result = evaluate({ + items: [ + submitItem( + "APPROVE", + "Approved — no blocking issues found.", + ), + ], + plan: {depth}, + outFiles: {}, + }); + expect( + result.violations.map((v) => v.code), + `depth ${depth}`, + ).toEqual(["correctness-missing"]); + } + }); + + it("carries no correctness requirement at fast depth (reconcile-only roster)", () => { + const result = evaluate({ + items: [ + submitItem( + "APPROVE", + "Approved — no blocking issues found.", + ), + ], + plan: {depth: "fast"}, + outFiles: {"thread-reconciler.json": "{}"}, + }); + expect(result.conformant).toBe(true); + expect(result.depth).toBe("fast"); + }); + + it("still requires the validator when findings post at fast depth (no producer ran)", () => { + const result = evaluate({ + items: [commentItem(), submitItem("APPROVE")], + plan: {depth: "fast"}, + outFiles: {}, + }); + expect(result.violations.map((v) => v.code)).toEqual([ + "validator-missing-with-findings", + ]); + }); + + it("defaults a missing or unrecognized plan to full depth (the strictest)", () => { + const missing = evaluate({ + items: [submitItem("APPROVE")], + outFiles: {}, + }); + expect(missing.depth).toBe("full"); + expect(missing.notes).toContain( + "rereview plan not staged: rules ran at full depth", + ); + const garbled = evaluate({ + items: [submitItem("APPROVE")], + plan: {depth: "turbo"}, + outFiles: {}, + }); + expect(garbled.depth).toBe("full"); + expect(garbled.violations.map((v) => v.code)).toEqual([ + "correctness-missing", + ]); + }); + }); + + describe("the pattern-triage empty-reviewFiles waiver", () => { + it("waives the correctness requirement when triage emptied the review set", () => { + const result = evaluate({ + items: [ + submitItem( + "APPROVE", + "Approved — no blocking issues found.", + ), + ], + plan: {depth: "full"}, + outFiles: { + "pattern-triage.json": JSON.stringify({ + patterns: ["rename"], + reviewFiles: [], + }), + }, + }); + expect(result.conformant).toBe(true); + expect(result.notes.join(" ")).toContain("waived"); + }); + + it("does not waive when reviewFiles is non-empty or triage output is unparseable", () => { + for (const triage of [ + JSON.stringify({reviewFiles: ["a.ts"]}), + "not json", + JSON.stringify({}), + ]) { + const result = evaluate({ + items: [submitItem("APPROVE")], + plan: {depth: "full"}, + outFiles: {"pattern-triage.json": triage}, + }); + expect(result.violations.map((v) => v.code)).toEqual([ + "correctness-missing", + ]); + } + }); + + it("does not apply at flip-gated depth (triage never runs there)", () => { + const result = evaluate({ + items: [submitItem("APPROVE")], + plan: {depth: "flip-gated"}, + outFiles: { + "pattern-triage.json": JSON.stringify({reviewFiles: []}), + }, + }); + expect(result.violations.map((v) => v.code)).toEqual([ + "correctness-missing", + ]); + }); + }); + + describe("unparseable dispatched output", () => { + it("accepts an unparseable correctness output when the body discloses it", () => { + // Step 3 allows staging the raw (possibly non-JSON) text of a + // failed sub-agent; the Step 6 note is the required disclosure. + const result = evaluate({ + items: [ + submitItem( + "APPROVE", + "Note: correctness not assessed this run (correctness-reviewer output unavailable).", + ), + ], + plan: {depth: "full"}, + outFiles: {"correctness-reviewer.json": "raw model text"}, + }); + expect(result.conformant).toBe(true); + }); + + it("flags an unparseable correctness output with no disclosure", () => { + const result = evaluate({ + items: [submitItem("APPROVE")], + plan: {depth: "full"}, + outFiles: {"correctness-reviewer.json": "raw model text"}, + }); + expect(result.violations.map((v) => v.code)).toEqual([ + "correctness-unparseable-undisclosed", + ]); + }); + }); + + describe("planned-shed disclosure (rule 3)", () => { + const routing = { + enabledReviewers: ["holistic", "test-adequacy"], + lensesToSpawn: ["security-auth"], + }; + + it("passes when every planned-but-undispatched name is disclosed", () => { + const result = evaluate({ + items: [ + submitItem( + "APPROVE", + [ + "Note: holistic not assessed this run (shed under the High-tier run budget).", + "Note: test-adequacy not assessed this run (shed under the High-tier run budget).", + "Note: security-auth not assessed this run (shed under the High-tier run budget).", + ].join("\n"), + ), + ], + plan: {depth: "full"}, + routing, + outFiles: conformingOutFiles(), + }); + expect(result.conformant).toBe(true); + }); + + it("flags each undisclosed planned shed by name", () => { + const outFiles = { + ...conformingOutFiles(), + "holistic.json": JSON.stringify({findings: []}), + }; + const result = evaluate({ + items: [ + submitItem( + "APPROVE", + "Approved — no blocking issues found.", + ), + ], + plan: {depth: "scoped"}, + routing, + outFiles, + }); + expect(result.violations).toEqual([ + expect.objectContaining({ + code: "shed-undisclosed", + dimension: "test-adequacy", + }), + expect.objectContaining({ + code: "shed-undisclosed", + dimension: "security-auth", + }), + ]); + }); + + it("skips the rule (with a note) when routing was never staged", () => { + const result = evaluate({ + items: [submitItem("APPROVE")], + plan: {depth: "full"}, + outFiles: conformingOutFiles(), + }); + expect(result.conformant).toBe(true); + expect(result.notes.join(" ")).toContain( + "planned-roster rule skipped", + ); + }); + }); +}); + +describe("disclosesSkippedDimension", () => { + it("matches the observed production validator wording (Khan/actions#272)", () => { + expect( + disclosesSkippedDimension( + "Note: claim validation not assessed this run (claim-validator output unavailable).", + "claim-validator", + ), + ).toBe(true); + }); + + it("matches the planned-shed wording and separator variants", () => { + expect( + disclosesSkippedDimension( + "Note: test adequacy not assessed this run (shed under the Low-tier run budget).", + "test-adequacy", + ), + ).toBe(true); + expect( + disclosesSkippedDimension( + "Note: security/auth not assessed this run (shed under the Low-tier run budget).", + "security-auth", + ), + ).toBe(true); + }); + + it("requires the not-assessed phrasing, not a bare name mention", () => { + expect( + disclosesSkippedDimension( + "The holistic reviewer found nothing.", + "holistic", + ), + ).toBe(false); + expect( + disclosesSkippedDimension( + "Note: holistic not assessed this run.", + "security-auth", + ), + ).toBe(false); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* CLI */ +/* -------------------------------------------------------------------------- */ + +/** Minimal in-memory fs honoring the paths the CLI touches. */ +const makeFakeFs = ( + files: Record, +): DispatchGateFs & { + files: Record; +} => { + const state = {...files}; + return { + files: state, + readFileSync: (p: string) => { + if (!(p in state)) { + throw new Error(`ENOENT: ${p}`); + } + return state[p]; + }, + writeFileSync: (p: string, data: string) => { + state[p] = data; + }, + existsSync: (p: string) => + p in state || Object.keys(state).some((f) => f.startsWith(`${p}/`)), + mkdirSync: () => {}, + readdirSync: (p: string) => { + const prefix = `${p}/`; + return [ + ...new Set( + Object.keys(state) + .filter((f) => f.startsWith(prefix)) + .map((f) => f.slice(prefix.length).split("/")[0]), + ), + ]; + }, + }; +}; + +const AGENT_OUTPUT = "/tmp/gh-aw/agent_output.json"; +const OUT = "/tmp/gh-aw/review/out"; + +describe("runDispatchGateCli", () => { + it("blocks a synthetic violation: strips posting items, keeps evidence, preserves the original queue", () => { + // The fabricated out/ directory is missing correctness-reviewer.json; + // the queue carries a verdict, comments, a thread resolution, and the + // artifact upload (the definition-of-done repro). + const queue = { + items: [ + { + type: "create_pull_request_review_comment", + path: "a.ts", + line: 1, + body: "x", + }, + { + type: "submit_pull_request_review", + event: "REQUEST_CHANGES", + body: "Changes requested — see inline comments.", + }, + { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_1", + }, + {type: "add_comment", body: "risks"}, + {type: "upload_artifact", path: "out"}, + {type: "missing_tool", tool: "x"}, + ], + }; + const fs = makeFakeFs({ + [AGENT_OUTPUT]: JSON.stringify(queue), + [`${OUT}/orchestrator-findings.json`]: "{}", + }); + const report = runDispatchGateCli(fs); + + expect(report.blocked).toBe(true); + expect(report.violations.map((v) => v.code)).toEqual([ + "correctness-missing", + "validator-missing-with-findings", + ]); + // The rewritten queue keeps only the KEEP_ITEM_TYPES survivors. + const rewritten = JSON.parse(fs.files[AGENT_OUTPUT]) as { + items: {type: string}[]; + }; + expect(rewritten.items.map((i) => i.type)).toEqual([ + "upload_artifact", + "missing_tool", + ]); + expect(rewritten.items.every((i) => KEEP_ITEM_TYPES.has(i.type))).toBe( + true, + ); + // Forensics: the original queue and the report ride the agent artifact. + expect( + JSON.parse(fs.files["/tmp/gh-aw/agent/agent_output.pre-gate.json"]), + ).toEqual(queue); + const report2 = JSON.parse( + fs.files["/tmp/gh-aw/agent/dispatch-gate.json"], + ) as {blocked: boolean; strippedItemTypes: Record}; + expect(report2.blocked).toBe(true); + expect(report2.strippedItemTypes).toEqual({ + create_pull_request_review_comment: 1, + submit_pull_request_review: 1, + resolve_pull_request_review_thread: 1, + add_comment: 1, + }); + }); + + it("leaves a conforming run's queue untouched and reports conformant", () => { + const queueText = JSON.stringify({ + items: [ + { + type: "create_pull_request_review_comment", + path: "a.ts", + line: 1, + body: "x", + }, + { + type: "submit_pull_request_review", + event: "APPROVE", + body: "", + }, + ], + }); + const fs = makeFakeFs({ + [AGENT_OUTPUT]: queueText, + "/tmp/gh-aw/review/routing.json": JSON.stringify({ + enabledReviewers: [], + lensesToSpawn: [], + }), + "/tmp/gh-aw/review/rereview-plan.json": JSON.stringify({ + depth: "full", + }), + [`${OUT}/pattern-triage.json`]: JSON.stringify({ + reviewFiles: ["a.ts"], + }), + [`${OUT}/correctness-reviewer.json`]: "{}", + [`${OUT}/claim-validator.json`]: "{}", + }); + const report = runDispatchGateCli(fs); + expect(report.blocked).toBe(false); + expect(report.violations).toEqual([]); + expect(fs.files[AGENT_OUTPUT]).toBe(queueText); + expect(fs.files["/tmp/gh-aw/agent/agent_output.pre-gate.json"]).toBe( + undefined, + ); + // The report still lands for the conformance-rate measurement. + expect( + ( + JSON.parse(fs.files["/tmp/gh-aw/agent/dispatch-gate.json"]) as { + blocked: boolean; + } + ).blocked, + ).toBe(false); + }); + + it("reads the plan from the out/ copy when the review-dir original is gone", () => { + const fs = makeFakeFs({ + [AGENT_OUTPUT]: JSON.stringify({ + items: [ + { + type: "submit_pull_request_review", + event: "APPROVE", + body: "", + }, + ], + }), + [`${OUT}/rereview-plan.json`]: JSON.stringify({depth: "fast"}), + }); + const report = runDispatchGateCli(fs); + expect(report.depth).toBe("fast"); + expect(report.blocked).toBe(false); + }); + + it("gates nothing when the queue is missing or unparseable (placeholder runs)", () => { + for (const files of [ + {}, + {[AGENT_OUTPUT]: "not json"}, + {[AGENT_OUTPUT]: JSON.stringify({items: []})}, + ]) { + const fs = makeFakeFs(files); + const report = runDispatchGateCli(fs); + expect(report.blocked).toBe(false); + expect(report.conformant).toBe(true); + } + }); + + it("renders a summary a human can read at a glance", () => { + const fs = makeFakeFs({ + [AGENT_OUTPUT]: JSON.stringify({ + items: [ + { + type: "submit_pull_request_review", + event: "REQUEST_CHANGES", + body: "Changes requested — see inline comments.", + }, + ], + }), + }); + const report = runDispatchGateCli(fs); + const summary = renderGateSummary(report); + expect(summary).toContain("## Dispatch-conformance gate"); + expect(summary).toContain("**BLOCKED**"); + expect(summary).toContain("correctness-missing"); + const okSummary = renderGateSummary({ + ...report, + blocked: false, + violations: [], + verdictEvent: null, + commentCount: 0, + }); + expect(okSummary).toContain("Nothing to gate"); + }); +}); diff --git a/workflows/review/lib/dispatch-gate.ts b/workflows/review/lib/dispatch-gate.ts new file mode 100644 index 00000000..32cace9f --- /dev/null +++ b/workflows/review/lib/dispatch-gate.ts @@ -0,0 +1,570 @@ +/** + * The dispatch-conformance gate: a review verdict cannot leave the agent job + * unless the sub-agent outputs it claims to summarize actually exist. + * + * The production failure this module exists for (the review-v1.7.0 acceptance + * trial, Khan/webapp#40992, run 29865480728): the orchestrator ran no router, + * dispatched zero sub-agents, did no claim validation, reviewed the diff + * itself, labeled its audit record "streamlined direct review", and submitted + * a REQUEST_CHANGES whose body disclosed none of it. Nothing on the PR + * distinguishes such a run from a conforming one (the previous day's review + * of Khan/actions#272 dispatched correctly and disclosed its sheds), the eval + * harness dispatches sub-agents from a script so it can never observe + * protocol fidelity, and an orchestrator that skips dispatch also skips + * prompt-mandated self-checks; code at the submission chokepoint is the only + * lever. Same design family as v1.6.1's non-empty-body rule. + * + * Where it runs: a gh-aw `post-steps:` step in the agent job. gh-aw v0.81.6 + * compiles post-steps after "Ingest agent output" (which finalizes + * `/tmp/gh-aw/agent_output.json`, the validated safe-output queue) and before + * "Upload agent artifacts" (which ships that file to the `safe_outputs` job, + * the separate job that actually calls the GitHub API). The gate therefore + * sees the exact queue the API-calling job will execute, plus the real + * `/tmp/gh-aw/review/` staging on the same runner, and a rewrite here BLOCKS + * the submission rather than detecting it after the fact. + * + * What it enforces (per re-review depth; `rereview-plan.json` is the staged + * source of truth, missing plan defaults to `full`, the strictest): + * + * 1. A queued review verdict requires `out/correctness-reviewer.json` to + * exist at every depth that dispatches the correctness pass (`full`, + * `scoped`, `flip-gated`). The one waiver: `pattern-triage` returned an + * empty `reviewFiles` (nothing needed review), proven by its own staged + * output. `fast` dispatches no finding producers, so it carries no + * correctness requirement. + * 2. Queued inline review comments require a parseable + * `out/claim-validator.json`, or the disclosed skipped-dimension note + * ("claim validation not assessed this run ...") in the verdict body + * (the #258 shed rules allow shedding the validator near a hard + * ceiling, but never silently). + * 3. Planned-but-undispatched reviewers must be disclosed: every name in + * `routing.json`'s `enabledReviewers`/`lensesToSpawn` with no `out/` + * file needs its "not assessed this run" note in the verdict body. + * + * Violation behavior: strip every posting/mutating item from the queue + * (keeping the diagnostics and the `out/` artifact upload so the evidence + * still lands), preserve the original queue beside the agent artifact, and + * exit non-zero, which fails the agent job and files gh-aw's failure issue. + * A violated run is a red run that posts nothing, never a silently-passing + * one. Existence is the contract, not authenticity: the gate proves the + * orchestrator staged reviewer outputs, not that a model produced them + * (script-driven dispatch, the next migration slice, closes that residual). + * + * Deliberately NOT enforced, to keep the false-positive rate at zero: + * `thread-reconciler` and `skill-auditor` existence (production shows a + * conforming first review with no prior threads dispatches no reconciler, + * e.g. Khan/actions#272), `pattern-triage` itself, and the router having run + * (a routerless freelancing run is already caught by rule 1). + * + * Determinism boundary: pure functions of the queued items and the staged + * files; no model call, no clock, no prose about the code under review. + */ + +/* -------------------------------------------------------------------------- */ +/* Types */ +/* -------------------------------------------------------------------------- */ + +/** One queued safe-output item (only the fields the gate reads are typed). */ +export type SafeOutputItem = { + type?: unknown; + event?: unknown; + body?: unknown; +} & Record; + +/** Mirrors `ReReviewDepth` (rereview-mode.ts); parsed defensively here. */ +export type DispatchGateDepth = "full" | "scoped" | "flip-gated" | "fast"; + +const GATE_DEPTHS: readonly DispatchGateDepth[] = [ + "full", + "scoped", + "flip-gated", + "fast", +]; + +export type DispatchGateViolationCode = + | "correctness-missing" + | "correctness-unparseable-undisclosed" + | "validator-missing-with-findings" + | "shed-undisclosed"; + +export type DispatchGateViolation = { + /** Fixed-format code (never prose). */ + code: DispatchGateViolationCode; + /** The reviewer / lens / dimension the violation is about. */ + dimension: string; + /** One sentence for the step log and the failure issue. */ + detail: string; +}; + +export type DispatchGateEvaluation = { + conformant: boolean; + violations: DispatchGateViolation[]; + /** The queued verdict event; null when no review submission is queued. */ + verdictEvent: string | null; + /** Queued inline review comment count. */ + commentCount: number; + /** The depth the rules ran under (defaulted to `full` when unstaged). */ + depth: DispatchGateDepth; + /** Non-blocking observations (unstaged inputs, applied waivers). */ + notes: string[]; +}; + +export type DispatchGateInput = { + /** The validated safe-output queue (`agent_output.json` `items`). */ + items: SafeOutputItem[]; + /** Parsed `rereview-plan.json`; undefined when not staged. */ + plan: unknown; + /** Parsed `routing.json`; undefined when not staged. */ + routing: unknown; + /** `out/` basename → raw file text, e.g. `correctness-reviewer.json`. */ + outFiles: Record; +}; + +/* -------------------------------------------------------------------------- */ +/* Evaluation */ +/* -------------------------------------------------------------------------- */ + +const SUBMIT_TYPE = "submit_pull_request_review"; +const COMMENT_TYPE = "create_pull_request_review_comment"; + +const CORRECTNESS_OUT = "correctness-reviewer.json"; +const VALIDATOR_OUT = "claim-validator.json"; +const TRIAGE_OUT = "pattern-triage.json"; + +/** The Step 6 skipped-dimension phrasing shared by both note wordings. */ +const NOT_ASSESSED_PHRASE = "not assessed this run"; + +const parseJson = (text: string): unknown => { + try { + return JSON.parse(text); + } catch { + return undefined; + } +}; + +/** + * Lowercase and collapse the separator variants (`-`, `_`, `/`) note authors + * use, so `test-adequacy` matches "test adequacy" and `security-auth` + * matches "security/auth". + */ +const normalize = (text: string): string => + text + .toLowerCase() + .replace(/[-_/]+/g, " ") + .replace(/\s+/g, " "); + +/** + * Note aliases where the Step 6 dimension wording diverges from the + * sub-agent name. Values are normalized substrings; the default alias is the + * normalized name itself. `claim valid` covers both the observed production + * wording ("claim validation not assessed this run (claim-validator output + * unavailable)", Khan/actions#272) and the planned-shed variant. + */ +const DIMENSION_ALIASES: Record = { + "correctness-reviewer": ["correctness"], + "claim-validator": ["claim valid"], +}; + +/** + * Does the review body disclose this dimension as skipped? True when the + * body carries the Step 6 "not assessed this run" phrasing and names the + * dimension (either note wording: planned shed or output unavailable). + */ +export const disclosesSkippedDimension = ( + body: string, + dimension: string, +): boolean => { + const normBody = normalize(body); + if (!normBody.includes(NOT_ASSESSED_PHRASE)) { + return false; + } + const aliases = DIMENSION_ALIASES[dimension] ?? [normalize(dimension)]; + return aliases.some((alias) => normBody.includes(alias)); +}; + +const resolveDepth = (plan: unknown, notes: string[]): DispatchGateDepth => { + const depth = (plan as {depth?: unknown} | undefined)?.depth; + if ( + typeof depth === "string" && + (GATE_DEPTHS as readonly string[]).includes(depth) + ) { + return depth as DispatchGateDepth; + } + notes.push( + plan === undefined + ? "rereview plan not staged: rules ran at full depth" + : "rereview plan depth unrecognized: rules ran at full depth", + ); + return "full"; +}; + +/** The names routing planned beyond the defaults (strings only, deduped). */ +const plannedExtras = (routing: unknown): string[] => { + const r = routing as + | {enabledReviewers?: unknown; lensesToSpawn?: unknown} + | undefined; + const names = [ + ...(Array.isArray(r?.enabledReviewers) ? r.enabledReviewers : []), + ...(Array.isArray(r?.lensesToSpawn) ? r.lensesToSpawn : []), + ].filter((name): name is string => typeof name === "string"); + return [...new Set(names)]; +}; + +/** + * The one legitimate way a `full`/`scoped` run submits a verdict with no + * correctness pass: `pattern-triage` ran and returned an empty `reviewFiles` + * (every changed file was generated / formatting-only / pattern-only), proven + * by its own staged output. + */ +const triageEmptiedReview = (outFiles: Record): boolean => { + const raw = outFiles[TRIAGE_OUT]; + if (raw === undefined) { + return false; + } + const parsed = parseJson(raw) as {reviewFiles?: unknown} | undefined; + return ( + parsed !== undefined && + Array.isArray(parsed.reviewFiles) && + parsed.reviewFiles.length === 0 + ); +}; + +/** Pure conformance evaluation; the CLI below is its only production caller. */ +export const evaluateDispatchConformance = ( + input: DispatchGateInput, +): DispatchGateEvaluation => { + const notes: string[] = []; + const violations: DispatchGateViolation[] = []; + + const submit = input.items.find((item) => item.type === SUBMIT_TYPE); + const verdictEvent = + submit === undefined + ? null + : typeof submit.event === "string" + ? submit.event + : ""; + const body = + submit !== undefined && typeof submit.body === "string" + ? submit.body + : ""; + const commentCount = input.items.filter( + (item) => item.type === COMMENT_TYPE, + ).length; + + const depth = resolveDepth(input.plan, notes); + const emptiedByTriage = + (depth === "full" || depth === "scoped") && + triageEmptiedReview(input.outFiles); + if (emptiedByTriage) { + notes.push( + "pattern-triage returned an empty reviewFiles: correctness and planned-roster rules waived", + ); + } + + // Rule 1: a verdict requires the correctness pass at every depth that + // dispatches one. + if (submit !== undefined && depth !== "fast" && !emptiedByTriage) { + const raw = input.outFiles[CORRECTNESS_OUT]; + if (raw === undefined) { + violations.push({ + code: "correctness-missing", + dimension: "correctness-reviewer", + detail: + `verdict ${ + verdictEvent || "(no event)" + } queued but out/${CORRECTNESS_OUT} does not exist ` + + `(depth ${depth} dispatches the correctness pass; even a failed dispatch stages an error note)`, + }); + } else if ( + parseJson(raw) === undefined && + !disclosesSkippedDimension(body, "correctness-reviewer") + ) { + violations.push({ + code: "correctness-unparseable-undisclosed", + dimension: "correctness-reviewer", + detail: + `out/${CORRECTNESS_OUT} is not valid JSON and the review body carries no ` + + `"correctness ${NOT_ASSESSED_PHRASE}" note disclosing the gap`, + }); + } + } + + // Rule 2: posted findings require the precision gate, or its disclosed + // shed (the #258 shed rules permit shedding the validator only near a + // hard ceiling, and never silently). + if (commentCount > 0) { + const raw = input.outFiles[VALIDATOR_OUT]; + const validated = raw !== undefined && parseJson(raw) !== undefined; + if (!validated && !disclosesSkippedDimension(body, "claim-validator")) { + violations.push({ + code: "validator-missing-with-findings", + dimension: "claim-validator", + detail: + `${commentCount} inline review comment(s) queued but out/${VALIDATOR_OUT} is ` + + `${ + raw === undefined ? "missing" : "unparseable" + } and the review body carries no ` + + `"claim validation ${NOT_ASSESSED_PHRASE}" note`, + }); + } + } + + // Rule 3: dispatched < planned requires a disclosure note per shed name. + // Only full/scoped plan the extras (flip-gated and fast dispatch fixed + // rosters, already covered by rule 1). + if ( + submit !== undefined && + (depth === "full" || depth === "scoped") && + !emptiedByTriage + ) { + if (input.routing === undefined) { + notes.push( + "routing not staged: planned-roster rule skipped (the missing correctness pass, rule 1, is what catches a routerless run)", + ); + } else { + for (const name of plannedExtras(input.routing)) { + const dispatched = `${name}.json` in input.outFiles; + if (!dispatched && !disclosesSkippedDimension(body, name)) { + violations.push({ + code: "shed-undisclosed", + dimension: name, + detail: + `routing planned ${name} but out/${name}.json does not exist and the review body ` + + `carries no "${name} ${NOT_ASSESSED_PHRASE}" note`, + }); + } + } + } + } + + return { + conformant: violations.length === 0, + violations, + verdictEvent, + commentCount, + depth, + notes, + }; +}; + +/* -------------------------------------------------------------------------- */ +/* CLI: the post-agent gate step */ +/* -------------------------------------------------------------------------- */ + +/** + * Fixed gh-aw paths (agent job). `AGENT_OUTPUT_PATH` is written by gh-aw's + * "Ingest agent output" step (a placeholder `{"items":[]}` is guaranteed by + * "Write agent output placeholder if missing", which precedes post-steps) + * and uploaded afterwards as the `agent` artifact the `safe_outputs` job + * executes from. `REPORT_DIR` is `/tmp/gh-aw/agent/`, which the "Upload + * agent artifacts" step already includes, so the gate report and the + * pre-gate queue copy ride the run artifact for free. + */ +const AGENT_OUTPUT_PATH = "/tmp/gh-aw/agent_output.json"; +const REVIEW_DIR = "/tmp/gh-aw/review"; +const OUT_DIR = `${REVIEW_DIR}/out`; +const ROUTING_PATH = `${REVIEW_DIR}/routing.json`; +const PLAN_PATHS = [ + `${REVIEW_DIR}/rereview-plan.json`, + `${OUT_DIR}/rereview-plan.json`, +]; +const REPORT_DIR = "/tmp/gh-aw/agent"; +const REPORT_PATH = `${REPORT_DIR}/dispatch-gate.json`; +const PRE_GATE_QUEUE_PATH = `${REPORT_DIR}/agent_output.pre-gate.json`; + +/** + * Item types a violated run may still execute: the artifact upload (the + * evidence a human needs to diagnose the violation) and the non-posting + * diagnostics. Everything else (the review submission, inline comments, + * thread resolutions, the risks/patterns comment, reviewer requests, and any + * type this list has never seen) is stripped: default-deny. + */ +export const KEEP_ITEM_TYPES: ReadonlySet = new Set([ + "upload_artifact", + "missing_tool", + "missing_data", + "noop", +]); + +export type DispatchGateFs = { + readFileSync: (p: string, enc: "utf8") => string; + writeFileSync: (p: string, data: string) => void; + existsSync: (p: string) => boolean; + mkdirSync: (p: string, opts: {recursive: boolean}) => void; + readdirSync: (p: string) => string[]; +}; + +export type DispatchGateReport = DispatchGateEvaluation & { + gateVersion: 1; + /** True when the queue was rewritten and the job should fail. */ + blocked: boolean; + /** `out/` basenames the gate saw (the dispatch evidence). */ + outFilesSeen: string[]; + /** Item types stripped from the queue, with counts (blocked runs). */ + strippedItemTypes: Record; +}; + +const readJsonIfPresent = (fs: DispatchGateFs, path: string): unknown => { + if (!fs.existsSync(path)) { + return undefined; + } + return parseJson(fs.readFileSync(path, "utf8")); +}; + +/** + * Run the gate over the staged run. Factored out (fs injected) so it is + * testable without touching the real filesystem. Writes the report always; + * rewrites the queue only on violation. Returns what it decided. + */ +export const runDispatchGateCli = (fs: DispatchGateFs): DispatchGateReport => { + const notes: string[] = []; + + const rawQueue = fs.existsSync(AGENT_OUTPUT_PATH) + ? fs.readFileSync(AGENT_OUTPUT_PATH, "utf8") + : undefined; + const queue = rawQueue === undefined ? undefined : parseJson(rawQueue); + const items: SafeOutputItem[] = Array.isArray( + (queue as {items?: unknown} | undefined)?.items, + ) + ? ((queue as {items: unknown[]}).items.filter( + (item): item is SafeOutputItem => + typeof item === "object" && item !== null, + ) as SafeOutputItem[]) + : []; + if (queue === undefined) { + notes.push( + `agent output queue missing or unparseable (${AGENT_OUTPUT_PATH}): nothing to gate`, + ); + } + + const outFiles: Record = {}; + if (fs.existsSync(OUT_DIR)) { + for (const name of fs.readdirSync(OUT_DIR)) { + try { + outFiles[name] = fs.readFileSync(`${OUT_DIR}/${name}`, "utf8"); + } catch { + // A subdirectory or unreadable entry is not dispatch evidence. + } + } + } + + const plan = PLAN_PATHS.map((path) => readJsonIfPresent(fs, path)).find( + (parsed) => parsed !== undefined, + ); + const routing = readJsonIfPresent(fs, ROUTING_PATH); + + const evaluation = evaluateDispatchConformance({ + items, + plan, + routing, + outFiles, + }); + evaluation.notes.unshift(...notes); + + const strippedItemTypes: Record = {}; + const blocked = !evaluation.conformant; + if (blocked && rawQueue !== undefined) { + const kept = items.filter( + (item) => + typeof item.type === "string" && KEEP_ITEM_TYPES.has(item.type), + ); + for (const item of items) { + if (!kept.includes(item)) { + const type = + typeof item.type === "string" ? item.type : "(untyped)"; + strippedItemTypes[type] = (strippedItemTypes[type] ?? 0) + 1; + } + } + fs.mkdirSync(REPORT_DIR, {recursive: true}); + fs.writeFileSync(PRE_GATE_QUEUE_PATH, rawQueue); + fs.writeFileSync( + AGENT_OUTPUT_PATH, + JSON.stringify( + {...(queue as Record), items: kept}, + null, + 2, + ), + ); + } + + const report: DispatchGateReport = { + gateVersion: 1, + blocked, + outFilesSeen: Object.keys(outFiles).sort(), + strippedItemTypes, + ...evaluation, + }; + fs.mkdirSync(REPORT_DIR, {recursive: true}); + fs.writeFileSync(REPORT_PATH, JSON.stringify(report, null, 2)); + return report; +}; + +/** Markdown for the job step summary; one glance says what happened. */ +export const renderGateSummary = (report: DispatchGateReport): string => { + const lines = [ + "## Dispatch-conformance gate", + "", + report.blocked + ? "**BLOCKED**: the queued review does not conform to the dispatch protocol; every posting safe output was stripped and this job fails." + : report.verdictEvent === null && report.commentCount === 0 + ? "Nothing to gate (no review submission or inline comments queued)." + : "Conformant.", + "", + `- depth: \`${report.depth}\``, + `- verdict queued: \`${report.verdictEvent ?? "none"}\``, + `- inline comments queued: ${report.commentCount}`, + `- out/ files seen: ${ + report.outFilesSeen.length > 0 + ? report.outFilesSeen.map((name) => `\`${name}\``).join(", ") + : "none" + }`, + ]; + for (const violation of report.violations) { + lines.push( + `- **${violation.code}** (${violation.dimension}): ${violation.detail}`, + ); + } + for (const note of report.notes) { + lines.push(`- note: ${note}`); + } + return `${lines.join("\n")}\n`; +}; + +// Run only when executed directly (review.md post-steps), never on import +// (tests). Fail-open on the gate's own errors: a gate bug must not block +// reviews, but it announces itself in the log and the step summary. +if (typeof require !== "undefined" && require.main === module) { + const nodeFs = require("node:fs") as DispatchGateFs & { + appendFileSync: (p: string, data: string) => void; + }; + try { + const report = runDispatchGateCli(nodeFs); + // eslint-disable-next-line no-console + console.log(JSON.stringify(report, null, 2)); + const summaryPath = process.env.GITHUB_STEP_SUMMARY; + if (summaryPath !== undefined && summaryPath !== "") { + nodeFs.appendFileSync(summaryPath, renderGateSummary(report)); + } + if (report.blocked) { + for (const violation of report.violations) { + // eslint-disable-next-line no-console + console.error( + `::error title=dispatch-conformance gate::${violation.code} (${violation.dimension}): ${violation.detail}`, + ); + } + // eslint-disable-next-line no-console + console.error( + `::error title=dispatch-conformance gate::review submission blocked; original queue preserved at ${PRE_GATE_QUEUE_PATH} (agent artifact), report at ${REPORT_PATH}`, + ); + process.exit(1); + } + } catch (error) { + // eslint-disable-next-line no-console + console.log( + `::warning title=dispatch-conformance gate::gate errored (fail-open, review not blocked): ${ + error instanceof Error ? error.message : String(error) + }`, + ); + process.exit(0); + } +} diff --git a/workflows/review/review.md b/workflows/review/review.md index 02bce5f9..d110c3fb 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -233,6 +233,27 @@ pre-agent-steps: path: gh-aw-review-lib persist-credentials: false +# The dispatch-conformance gate (workflows/review/lib/dispatch-gate.ts): a code +# chokepoint between the agent and the review submission. gh-aw compiles +# `post-steps` into the agent job after "Ingest agent output" (which finalizes +# /tmp/gh-aw/agent_output.json, the validated safe-output queue) and before +# "Upload agent artifacts" (which ships that queue to the separate safe_outputs +# job that actually calls the GitHub API). The gate reads the queue plus the +# /tmp/gh-aw/review/ staging on the same runner and, when a queued verdict or +# queued findings lack the sub-agent outputs the protocol requires (Step 3; +# per re-review depth, sheds must be disclosed), strips every posting item +# from the queue and exits non-zero: the submission is BLOCKED (not detected +# after the fact), the run goes red, and the evidence (the out/ artifact, the +# original queue beside the agent artifact, the gate report) still lands. +# Exists because run 29865480728 (Khan/webapp#40992) submitted a verdict with +# zero sub-agent dispatches and no disclosure; a prompt rule cannot gate an +# orchestrator that is already ignoring the prompt. `if: always()` because the +# safe_outputs job executes the queue even when the agent job fails partway. +post-steps: + - name: Dispatch-conformance gate + if: always() + run: cd gh-aw-review-lib && npx -y tsx workflows/review/lib/dispatch-gate.ts + # Cost guardrails (AI credits; 1 credit = $0.01). gh-aw >= v0.79 bakes in # defaults of 1000/run ($10) and 5000/day ($50). Disable the daily ceiling # (-1) so reviews are never skipped on a busy PR day; the per-run cap below From 9e4cf671634ad590f17bce0d2df0cd7a8badd6b2 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 21 Jul 2026 15:50:47 -0700 Subject: [PATCH 2/6] [jwies/review-dispatch-gate] review: harden the gate's fail-open ordering; sentinel-gated job failure; per-line disclosure match (review feedback) --- workflows/review/lib/dispatch-gate.test.ts | 131 +++++++++++++++++++ workflows/review/lib/dispatch-gate.ts | 140 +++++++++++++++------ workflows/review/review.md | 19 ++- 3 files changed, 248 insertions(+), 42 deletions(-) diff --git a/workflows/review/lib/dispatch-gate.test.ts b/workflows/review/lib/dispatch-gate.test.ts index 54325342..12e918e7 100644 --- a/workflows/review/lib/dispatch-gate.test.ts +++ b/workflows/review/lib/dispatch-gate.test.ts @@ -1,6 +1,7 @@ import {describe, it, expect} from "vitest"; import { + BLOCKED_SENTINEL_PATH, disclosesSkippedDimension, evaluateDispatchConformance, renderGateSummary, @@ -609,3 +610,133 @@ describe("runDispatchGateCli", () => { expect(okSummary).toContain("Nothing to gate"); }); }); + +describe("fail-open ordering and disclosure precision (review feedback)", () => { + it("requires the phrase and the dimension on the same line", () => { + // One legitimate note plus a prose mention of another dimension: the + // mention must not read as that dimension's disclosure. + const body = [ + "Note: holistic not assessed this run (shed under the High-tier run budget).", + "No security/auth concerns in this change.", + ].join("\n"); + expect(disclosesSkippedDimension(body, "holistic")).toBe(true); + expect(disclosesSkippedDimension(body, "security-auth")).toBe(false); + }); + + it("flags a present-but-unparseable validator output with findings queued", () => { + const result = evaluate({ + items: [commentItem(), submitItem("APPROVE", "")], + plan: {depth: "fast"}, + outFiles: {"claim-validator.json": "not json"}, + }); + expect(result.violations.map((v) => v.code)).toEqual([ + "validator-missing-with-findings", + ]); + expect(result.violations[0].detail).toContain("unparseable"); + }); + + it("strips and counts a typeless item under (untyped)", () => { + const fs = makeFakeFs({ + [AGENT_OUTPUT]: JSON.stringify({ + items: [ + {event: "REQUEST_CHANGES"}, + { + type: "submit_pull_request_review", + event: "REQUEST_CHANGES", + body: "x", + }, + ], + }), + }); + const report = runDispatchGateCli(fs); + expect(report.blocked).toBe(true); + expect(report.strippedItemTypes["(untyped)"]).toBe(1); + expect(JSON.parse(fs.files[AGENT_OUTPUT]).items).toEqual([]); + }); + + it("writes the violation sentinel only on a real block", () => { + const blockedFs = makeFakeFs({ + [AGENT_OUTPUT]: JSON.stringify({ + items: [ + { + type: "submit_pull_request_review", + event: "REQUEST_CHANGES", + body: "x", + }, + ], + }), + }); + runDispatchGateCli(blockedFs); + expect(blockedFs.files[BLOCKED_SENTINEL_PATH]).toBeDefined(); + + const cleanFs = makeFakeFs({ + [AGENT_OUTPUT]: JSON.stringify({items: []}), + }); + runDispatchGateCli(cleanFs); + expect(cleanFs.files[BLOCKED_SENTINEL_PATH]).toBe(undefined); + }); + + it("leaves the queue intact when a pre-decision write throws (fail-open path)", () => { + const queueText = JSON.stringify({ + items: [ + { + type: "submit_pull_request_review", + event: "REQUEST_CHANGES", + body: "x", + }, + ], + }); + const fs = makeFakeFs({[AGENT_OUTPUT]: queueText}); + const failingFs = { + ...fs, + writeFileSync: (p: string, data: string) => { + if (p.endsWith("dispatch-gate.json")) { + throw new Error("disk full"); + } + fs.writeFileSync(p, data); + }, + }; + expect(() => runDispatchGateCli(failingFs)).toThrow("disk full"); + // The report write precedes every queue mutation, so the original + // queue is untouched and the CLI entry fails open. + expect(fs.files[AGENT_OUTPUT]).toBe(queueText); + expect(fs.files[BLOCKED_SENTINEL_PATH]).toBe(undefined); + }); + + it("degrades block to detect (still red) when only the queue rewrite fails", () => { + const queueText = JSON.stringify({ + items: [ + { + type: "submit_pull_request_review", + event: "REQUEST_CHANGES", + body: "x", + }, + ], + }); + const fs = makeFakeFs({[AGENT_OUTPUT]: queueText}); + const failingFs = { + ...fs, + writeFileSync: (p: string, data: string) => { + if (p === AGENT_OUTPUT) { + throw new Error("read-only queue"); + } + fs.writeFileSync(p, data); + }, + }; + const report = runDispatchGateCli(failingFs); + expect(report.blocked).toBe(true); + expect(report.notes.join(" ")).toContain("queue rewrite failed"); + // The sentinel landed, so the step still fails the job. + expect(fs.files[BLOCKED_SENTINEL_PATH]).toBeDefined(); + expect(fs.files[AGENT_OUTPUT]).toBe(queueText); + }); + + it("notes a present-but-unparseable routing.json distinctly from a missing one", () => { + const fs = makeFakeFs({ + [AGENT_OUTPUT]: JSON.stringify({items: []}), + "/tmp/gh-aw/review/routing.json": "corrupt {", + }); + const report = runDispatchGateCli(fs); + expect(report.notes.join(" ")).toContain("present but unparseable"); + }); +}); diff --git a/workflows/review/lib/dispatch-gate.ts b/workflows/review/lib/dispatch-gate.ts index 32cace9f..d9a46ddd 100644 --- a/workflows/review/lib/dispatch-gate.ts +++ b/workflows/review/lib/dispatch-gate.ts @@ -174,12 +174,18 @@ export const disclosesSkippedDimension = ( body: string, dimension: string, ): boolean => { - const normBody = normalize(body); - if (!normBody.includes(NOT_ASSESSED_PHRASE)) { - return false; - } const aliases = DIMENSION_ALIASES[dimension] ?? [normalize(dimension)]; - return aliases.some((alias) => normBody.includes(alias)); + // The phrase and the dimension must co-occur on one line (the Step 6 + // notes are one line each): matched independently across the whole body, + // one legitimate note would satisfy the phrase globally and any prose + // mention of another dimension would then read as its disclosure. + return body.split("\n").some((line) => { + const norm = normalize(line); + return ( + norm.includes(NOT_ASSESSED_PHRASE) && + aliases.some((alias) => norm.includes(alias)) + ); + }); }; const resolveDepth = (plan: unknown, notes: string[]): DispatchGateDepth => { @@ -371,6 +377,12 @@ const PLAN_PATHS = [ const REPORT_DIR = "/tmp/gh-aw/agent"; const REPORT_PATH = `${REPORT_DIR}/dispatch-gate.json`; const PRE_GATE_QUEUE_PATH = `${REPORT_DIR}/agent_output.pre-gate.json`; +/** + * Written only when a violation was decided. The workflow step fails the job + * only when this file exists, so an infra failure (npx bootstrap, a crash + * before the decision) fails open instead of reading as a block. + */ +export const BLOCKED_SENTINEL_PATH = "/tmp/gh-aw/dispatch-gate.blocked"; /** * Item types a violated run may still execute: the artifact upload (the @@ -452,6 +464,11 @@ export const runDispatchGateCli = (fs: DispatchGateFs): DispatchGateReport => { (parsed) => parsed !== undefined, ); const routing = readJsonIfPresent(fs, ROUTING_PATH); + if (routing === undefined && fs.existsSync(ROUTING_PATH)) { + notes.push( + `routing.json is present but unparseable (${ROUTING_PATH}): treated as not staged`, + ); + } const evaluation = evaluateDispatchConformance({ items, @@ -463,6 +480,23 @@ export const runDispatchGateCli = (fs: DispatchGateFs): DispatchGateReport => { const strippedItemTypes: Record = {}; const blocked = !evaluation.conformant; + + // Ordering is the fail-open invariant (module doc): every fallible write + // before the queue rewrite may throw and leave the original queue intact + // (the CLI entry then fails open); the queue rewrite comes LAST among the + // mutating writes, and once `blocked` is decided the CLI's exit code no + // longer depends on any write succeeding, so a post-rewrite error can + // never turn a blocked run green. + const report: DispatchGateReport = { + gateVersion: 1, + blocked, + outFilesSeen: Object.keys(outFiles).sort(), + strippedItemTypes, + ...evaluation, + }; + fs.mkdirSync(REPORT_DIR, {recursive: true}); + fs.writeFileSync(REPORT_PATH, JSON.stringify(report, null, 2)); + if (blocked && rawQueue !== undefined) { const kept = items.filter( (item) => @@ -475,27 +509,40 @@ export const runDispatchGateCli = (fs: DispatchGateFs): DispatchGateReport => { strippedItemTypes[type] = (strippedItemTypes[type] ?? 0) + 1; } } - fs.mkdirSync(REPORT_DIR, {recursive: true}); + // The violation sentinel: the workflow step fails the job only when + // this file exists, so a crash of the gate BEFORE this point (or of + // the `npx tsx` bootstrap before the script runs at all) reads as an + // infra failure and fails open instead of red-flagging the run. + fs.writeFileSync(BLOCKED_SENTINEL_PATH, "blocked\n"); fs.writeFileSync(PRE_GATE_QUEUE_PATH, rawQueue); - fs.writeFileSync( - AGENT_OUTPUT_PATH, - JSON.stringify( - {...(queue as Record), items: kept}, - null, - 2, - ), - ); + try { + fs.writeFileSync( + AGENT_OUTPUT_PATH, + JSON.stringify( + {...(queue as Record), items: kept}, + null, + 2, + ), + ); + } catch (error) { + // A failed rewrite degrades block to detect: the run still goes + // red (blocked is already decided), but the untouched queue may + // post. Recorded so the forensics say which mode this run got. + report.notes.push( + `queue rewrite failed (${ + error instanceof Error ? error.message : String(error) + }): violation detected but the original queue may still post`, + ); + } + // Refresh the report with the strip counts and any rewrite note; + // best-effort (the pre-rewrite report above already persisted). + try { + fs.writeFileSync(REPORT_PATH, JSON.stringify(report, null, 2)); + } catch { + // The earlier report write already landed. + } } - const report: DispatchGateReport = { - gateVersion: 1, - blocked, - outFilesSeen: Object.keys(outFiles).sort(), - strippedItemTypes, - ...evaluation, - }; - fs.mkdirSync(REPORT_DIR, {recursive: true}); - fs.writeFileSync(REPORT_PATH, JSON.stringify(report, null, 2)); return report; }; @@ -531,40 +578,51 @@ export const renderGateSummary = (report: DispatchGateReport): string => { }; // Run only when executed directly (review.md post-steps), never on import -// (tests). Fail-open on the gate's own errors: a gate bug must not block -// reviews, but it announces itself in the log and the step summary. +// (tests). Fail-open ONLY on errors thrown before the gate decided (the +// queue is then untouched, by the write ordering in runDispatchGateCli); +// once `blocked` is decided, the exit code depends on nothing else — a +// failed report write or step-summary append can never turn a blocked run +// green. if (typeof require !== "undefined" && require.main === module) { const nodeFs = require("node:fs") as DispatchGateFs & { appendFileSync: (p: string, data: string) => void; }; + let report: DispatchGateReport; + try { + report = runDispatchGateCli(nodeFs); + } catch (error) { + // eslint-disable-next-line no-console + console.log( + `::warning title=dispatch-conformance gate::gate errored before deciding (fail-open, review not blocked): ${ + error instanceof Error ? error.message : String(error) + }`, + ); + process.exit(0); + } + // Reporting is best-effort and must not affect the exit code in either + // direction. try { - const report = runDispatchGateCli(nodeFs); // eslint-disable-next-line no-console console.log(JSON.stringify(report, null, 2)); const summaryPath = process.env.GITHUB_STEP_SUMMARY; if (summaryPath !== undefined && summaryPath !== "") { nodeFs.appendFileSync(summaryPath, renderGateSummary(report)); } - if (report.blocked) { - for (const violation of report.violations) { - // eslint-disable-next-line no-console - console.error( - `::error title=dispatch-conformance gate::${violation.code} (${violation.dimension}): ${violation.detail}`, - ); - } + } catch { + // The report file and stdout above are redundant surfaces; losing + // one changes nothing about the verdict on this run. + } + if (report.blocked) { + for (const violation of report.violations) { // eslint-disable-next-line no-console console.error( - `::error title=dispatch-conformance gate::review submission blocked; original queue preserved at ${PRE_GATE_QUEUE_PATH} (agent artifact), report at ${REPORT_PATH}`, + `::error title=dispatch-conformance gate::${violation.code} (${violation.dimension}): ${violation.detail}`, ); - process.exit(1); } - } catch (error) { // eslint-disable-next-line no-console - console.log( - `::warning title=dispatch-conformance gate::gate errored (fail-open, review not blocked): ${ - error instanceof Error ? error.message : String(error) - }`, + console.error( + `::error title=dispatch-conformance gate::review submission blocked; original queue preserved at ${PRE_GATE_QUEUE_PATH} (agent artifact), report at ${REPORT_PATH}`, ); - process.exit(0); + process.exit(1); } } diff --git a/workflows/review/review.md b/workflows/review/review.md index d110c3fb..59a35839 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -249,10 +249,27 @@ pre-agent-steps: # zero sub-agent dispatches and no disclosure; a prompt rule cannot gate an # orchestrator that is already ignoring the prompt. `if: always()` because the # safe_outputs job executes the queue even when the agent job fails partway. +# The step fails the job ONLY on the gate's violation sentinel, never on an +# infra failure: `npx` resolving `tsx` from the registry (or any crash before +# the gate decides) exits non-zero without the sentinel, and since the +# safe_outputs job runs regardless of this job's result, red-flagging such a +# run would file a spurious failure issue while the untouched queue posts +# anyway. The gate writes the sentinel only after deciding a real violation +# (and it strips the queue in the same code path). post-steps: - name: Dispatch-conformance gate if: always() - run: cd gh-aw-review-lib && npx -y tsx workflows/review/lib/dispatch-gate.ts + run: | + rm -f /tmp/gh-aw/dispatch-gate.blocked + if (cd gh-aw-review-lib && npx -y tsx workflows/review/lib/dispatch-gate.ts); then + exit 0 + fi + if [ -f /tmp/gh-aw/dispatch-gate.blocked ]; then + echo "::error title=dispatch-conformance gate::submission blocked; failing the job" + exit 1 + fi + echo "::warning title=dispatch-conformance gate::gate could not run (infra failure; review not blocked)" + exit 0 # Cost guardrails (AI credits; 1 credit = $0.01). gh-aw >= v0.79 bakes in # defaults of 1000/run ($10) and 5000/day ($50). Disable the daily ceiling From 4f8a83df299f993f9db31f9c64a67073479884b5 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 21 Jul 2026 16:22:18 -0700 Subject: [PATCH 3/6] [jwies/review-dispatch-gate] review: protect the sentinel/pre-gate writes so the rewrite always runs once blocked (re-review feedback) --- workflows/review/lib/dispatch-gate.test.ts | 50 ++++++++++++++++++++++ workflows/review/lib/dispatch-gate.ts | 17 +++++++- 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/workflows/review/lib/dispatch-gate.test.ts b/workflows/review/lib/dispatch-gate.test.ts index 12e918e7..9a9a1d87 100644 --- a/workflows/review/lib/dispatch-gate.test.ts +++ b/workflows/review/lib/dispatch-gate.test.ts @@ -740,3 +740,53 @@ describe("fail-open ordering and disclosure precision (review feedback)", () => expect(report.notes.join(" ")).toContain("present but unparseable"); }); }); + +describe("re-review hardening (second feedback round)", () => { + it("a disclosure note does not waive a fully-missing correctness output", () => { + const result = evaluate({ + items: [ + submitItem( + "APPROVE", + "Note: correctness not assessed this run (correctness-reviewer output unavailable).", + ), + ], + plan: {depth: "full"}, + outFiles: {}, + }); + expect(result.violations.map((v) => v.code)).toEqual([ + "correctness-missing", + ]); + }); + + it("still strips the queue when the sentinel/pre-gate writes fail", () => { + const queueText = JSON.stringify({ + items: [ + { + type: "submit_pull_request_review", + event: "REQUEST_CHANGES", + body: "x", + }, + ], + }); + const fs = makeFakeFs({[AGENT_OUTPUT]: queueText}); + const failingFs = { + ...fs, + writeFileSync: (p: string, data: string) => { + if ( + p === BLOCKED_SENTINEL_PATH || + p.endsWith("agent_output.pre-gate.json") + ) { + throw new Error("disk full"); + } + fs.writeFileSync(p, data); + }, + }; + const report = runDispatchGateCli(failingFs); + expect(report.blocked).toBe(true); + expect(report.notes.join(" ")).toContain( + "sentinel/pre-gate write failed", + ); + // The queue rewrite still ran: the violating queue can never post. + expect(JSON.parse(fs.files[AGENT_OUTPUT]).items).toEqual([]); + }); +}); diff --git a/workflows/review/lib/dispatch-gate.ts b/workflows/review/lib/dispatch-gate.ts index d9a46ddd..8b142bd5 100644 --- a/workflows/review/lib/dispatch-gate.ts +++ b/workflows/review/lib/dispatch-gate.ts @@ -513,8 +513,21 @@ export const runDispatchGateCli = (fs: DispatchGateFs): DispatchGateReport => { // this file exists, so a crash of the gate BEFORE this point (or of // the `npx tsx` bootstrap before the script runs at all) reads as an // infra failure and fails open instead of red-flagging the run. - fs.writeFileSync(BLOCKED_SENTINEL_PATH, "blocked\n"); - fs.writeFileSync(PRE_GATE_QUEUE_PATH, rawQueue); + // Wrapped so a failed write here cannot escape to the entry's + // fail-open catch with the queue still unstripped: the rewrite below + // must run whenever `blocked` was decided (worst case is a blocked + // run whose job stays green — quiet, but the violating queue never + // posts). + try { + fs.writeFileSync(BLOCKED_SENTINEL_PATH, "blocked\n"); + fs.writeFileSync(PRE_GATE_QUEUE_PATH, rawQueue); + } catch (error) { + report.notes.push( + `sentinel/pre-gate write failed (${ + error instanceof Error ? error.message : String(error) + }): block still enforced via the queue rewrite`, + ); + } try { fs.writeFileSync( AGENT_OUTPUT_PATH, From 4b96616c5e1626e9cc503bcdd37733303aaa201f Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 21 Jul 2026 16:46:59 -0700 Subject: [PATCH 4/6] [jwies/review-dispatch-gate] review: pin keep-list survivors, Step 6 template coupling, and the Conformant summary branch (third-round nits) --- workflows/review/lib/dispatch-gate.test.ts | 64 ++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/workflows/review/lib/dispatch-gate.test.ts b/workflows/review/lib/dispatch-gate.test.ts index 9a9a1d87..b5bb094c 100644 --- a/workflows/review/lib/dispatch-gate.test.ts +++ b/workflows/review/lib/dispatch-gate.test.ts @@ -1,3 +1,6 @@ +import {readFileSync} from "node:fs"; +import {join} from "node:path"; + import {describe, it, expect} from "vitest"; import { @@ -790,3 +793,64 @@ describe("re-review hardening (second feedback round)", () => { expect(JSON.parse(fs.files[AGENT_OUTPUT]).items).toEqual([]); }); }); + +describe("third-round nits: keep-list survivors, template coupling, summary", () => { + it("keeps noop and missing_data through a strip", () => { + const fs = makeFakeFs({ + [AGENT_OUTPUT]: JSON.stringify({ + items: [ + { + type: "submit_pull_request_review", + event: "REQUEST_CHANGES", + body: "x", + }, + {type: "noop", message: "m"}, + {type: "missing_data", data: "d"}, + ], + }), + }); + const report = runDispatchGateCli(fs); + expect(report.blocked).toBe(true); + expect( + JSON.parse(fs.files[AGENT_OUTPUT]).items.map( + (i: {type: string}) => i.type, + ), + ).toEqual(["noop", "missing_data"]); + }); + + it("review.md's Step 6 note templates still carry the phrase the gate matches", () => { + // Couples the disclosure matcher to the prompt templates: a Step 6 + // reword that drops the phrase must fail here, not silently break + // rules 2/3 in production. + const reviewMd = readFileSync( + join(__dirname, "..", "review.md"), + "utf8", + ); + expect(reviewMd).toContain( + "not assessed this run (shed under the -tier run budget)", + ); + expect(reviewMd).toContain( + "not assessed this run ( output unavailable)", + ); + }); + + it("renders the Conformant summary branch", () => { + const fs = makeFakeFs({ + [AGENT_OUTPUT]: JSON.stringify({ + items: [ + { + type: "submit_pull_request_review", + event: "APPROVE", + body: "Approved — no blocking issues found.", + }, + ], + }), + "/tmp/gh-aw/review/rereview-plan.json": JSON.stringify({ + depth: "fast", + }), + }); + const report = runDispatchGateCli(fs); + expect(report.blocked).toBe(false); + expect(renderGateSummary(report)).toContain("Conformant."); + }); +}); From 5a96e690d48cee61c2844540885ab36be09c6c5d Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 21 Jul 2026 23:03:59 -0700 Subject: [PATCH 5/6] [jwies/review-stamp-carrier] review: re-review fingerprint falls back to cache memory (the body stamp never survives gh-aw ingest) --- .changeset/stamp-carrier-cache-memory.md | 7 + workflows/review/lib/rereview-mode.test.ts | 153 +++++++++++++++++++++ workflows/review/lib/rereview-mode.ts | 137 ++++++++++++++++-- workflows/review/review.md | 19 ++- 4 files changed, 298 insertions(+), 18 deletions(-) create mode 100644 .changeset/stamp-carrier-cache-memory.md diff --git a/.changeset/stamp-carrier-cache-memory.md b/.changeset/stamp-carrier-cache-memory.md new file mode 100644 index 00000000..703450dc --- /dev/null +++ b/.changeset/stamp-carrier-cache-memory.md @@ -0,0 +1,7 @@ +--- +"review": patch +--- + +review: the re-review fingerprint anchors on cache memory; the body stamp never survives gh-aw ingest + +gh-aw's safe-output sanitizer strips all XML/HTML comments (`removeXmlComments`), so the hidden fingerprint stamp a review body carries never reaches the PR: every production re-review planned `no-prior-fingerprint` and silently escalated to full depth, making the `re-review` ROUTING dial (scoped/flip-gated/fast) inert. The plan CLI now falls back to the Step 9 cache-memory record (`verdict`, `stampHunks`/`reviewedHunks`, `wasDraft`) when no prior-review body carries a stamp, and records which carrier anchored the plan as `stampSource` in `rereview-plan.json`. Step 9 gains a `stampHunks` field copied verbatim from the plan CLI's own hash computation so hash regimes are never mixed. Cache eviction still degrades to a full review, never a cheaper one. diff --git a/workflows/review/lib/rereview-mode.test.ts b/workflows/review/lib/rereview-mode.test.ts index 2523d137..72608eb4 100644 --- a/workflows/review/lib/rereview-mode.test.ts +++ b/workflows/review/lib/rereview-mode.test.ts @@ -13,6 +13,7 @@ import { runRereviewPlanCli, runRereviewStampCli, STAMP_SCHEMA_VERSION, + stampFromCacheMemory, } from "./rereview-mode"; import type {HunkSignature, ReReviewStamp} from "./rereview-mode"; @@ -575,6 +576,158 @@ describe("runRereviewPlanCli", () => { }); }); +/* -------------------------------------------------------------------------- */ +/* The cache-memory fingerprint carrier */ +/* -------------------------------------------------------------------------- */ + +/** A Step 9 cache record whose fields reconstruct a usable stamp. */ +const cacheRecord = (over: Record = {}): string => + JSON.stringify({ + verdict: "APPROVE", + reviewedHunks: CURRENT, + wasDraft: false, + ...over, + }); + +describe("stampFromCacheMemory", () => { + it("reconstructs a stamp from a valid Step 9 record", () => { + const stamp = stampFromCacheMemory(JSON.parse(cacheRecord())); + expect(stamp).toEqual({ + schemaVersion: STAMP_SCHEMA_VERSION, + depth: "full", + verdict: "APPROVE", + anchorDraft: false, + anchorHunks: CURRENT, + }); + }); + + it.each([ + ["missing verdict", {verdict: undefined}], + ["unknown verdict", {verdict: "COMMENTED"}], + ["missing wasDraft", {wasDraft: undefined}], + ["non-boolean wasDraft", {wasDraft: "false"}], + ["missing hunks", {reviewedHunks: undefined}], + ["array hunks", {reviewedHunks: ["abc"]}], + ["non-string hash", {reviewedHunks: {"a.ts": [42]}}], + ["empty hash", {reviewedHunks: {"a.ts": [""]}}], + ["empty hunk map", {reviewedHunks: {}}], + ])("returns null on %s (fail toward full)", (_label, over) => { + expect(stampFromCacheMemory(JSON.parse(cacheRecord(over)))).toBeNull(); + }); + + it("returns null on a non-object record", () => { + expect(stampFromCacheMemory(null)).toBeNull(); + expect(stampFromCacheMemory("{}")).toBeNull(); + expect(stampFromCacheMemory([])).toBeNull(); + }); + + it("prefers stampHunks (the plan CLI's own hash regime) over reviewedHunks", () => { + const other: HunkSignature = {"other.ts": ["deadbeef"]}; + const stamp = stampFromCacheMemory( + JSON.parse(cacheRecord({stampHunks: other})), + ); + expect(stamp?.anchorHunks).toEqual(other); + }); + + it("falls back to reviewedHunks when stampHunks is invalid", () => { + const stamp = stampFromCacheMemory( + JSON.parse(cacheRecord({stampHunks: {"a.ts": [42]}})), + ); + expect(stamp?.anchorHunks).toEqual(CURRENT); + }); +}); + +describe("runRereviewPlanCli cache-memory fallback", () => { + const CACHE_PATH = "/tmp/gh-aw/cache-memory/pr-41007.json"; + const contextWithNumber = JSON.stringify({isDraft: false, number: 41007}); + + it("anchors on the cache record when no prior-review body carries a stamp (the production shape: the ingest sanitizer strips the body stamp)", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/pr-context.json`]: contextWithNumber, + // What production prior reviews actually look like: bodies + // present, stamps sanitized away. + [`${REVIEW_DIR}/prior-reviews.json`]: JSON.stringify([ + {body: "Changes requested — see inline comments."}, + ]), + [CACHE_PATH]: cacheRecord(), + }), + ); + const {plan, stampSource} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("fast"); + expect(plan.reasons).toEqual(["mode-fast"]); + expect(stampSource).toBe("cache-memory"); + const written = JSON.parse( + fs.files.get(`${REVIEW_DIR}/rereview-plan.json`) ?? "{}", + ); + expect(written.stampSource).toBe("cache-memory"); + }); + + it("prefers a review-body stamp over the cache record", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/pr-context.json`]: contextWithNumber, + [CACHE_PATH]: cacheRecord({verdict: "REQUEST_CHANGES"}), + }), + ); + const {stampSource} = runRereviewPlanCli(fs); + expect(stampSource).toBe("review-body"); + }); + + it("plans full with no stamp in either carrier", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/pr-context.json`]: contextWithNumber, + [`${REVIEW_DIR}/prior-reviews.json`]: JSON.stringify([ + {body: "no stamp here"}, + ]), + }), + ); + const {plan, stampSource} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("full"); + expect(plan.reasons).toEqual(["no-prior-fingerprint"]); + expect(stampSource).toBeNull(); + }); + + it("applies the ready-for-review guard to a cache anchor taken on a draft", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/pr-context.json`]: contextWithNumber, + [`${REVIEW_DIR}/prior-reviews.json`]: "[]", + [CACHE_PATH]: cacheRecord({wasDraft: true}), + }), + ); + const {plan} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("full"); + expect(plan.reasons).toEqual(["ready-for-review-anchor"]); + }); + + it("ignores the cache when pr-context carries no number", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/prior-reviews.json`]: "[]", + [CACHE_PATH]: cacheRecord(), + }), + ); + const {plan, stampSource} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("full"); + expect(stampSource).toBeNull(); + }); + + it("treats an unparseable cache record as no anchor", () => { + const fs = fakeFs( + stagedInputs({ + [`${REVIEW_DIR}/pr-context.json`]: contextWithNumber, + [`${REVIEW_DIR}/prior-reviews.json`]: "[]", + [CACHE_PATH]: "{not json", + }), + ); + const {plan, stampSource} = runRereviewPlanCli(fs); + expect(plan.depth).toBe("full"); + expect(stampSource).toBeNull(); + }); +}); + describe("runRereviewStampCli", () => { it("renders this run's stamp from the staged plan and the decided verdict", () => { const fs = fakeFs(stagedInputs()); diff --git a/workflows/review/lib/rereview-mode.ts b/workflows/review/lib/rereview-mode.ts index 4e61e9da..7e9f7408 100644 --- a/workflows/review/lib/rereview-mode.ts +++ b/workflows/review/lib/rereview-mode.ts @@ -21,14 +21,34 @@ * reconciliation, and a REQUEST_CHANGES→APPROVE flip is vetoed by any * validated blocking finding from that pass; the findings gate the * flip instead of being discarded. - * 3. **Divergence tripwire.** Every full-depth review stamps a - * content-hashed hunk signature into its review body as a hidden - * comment (so it survives cache eviction AND branch protection's - * dismiss-stale-approvals; a dismissed review keeps its body). Each - * later push compares its current signature against that last - * fully-reviewed fingerprint; when the unreviewed share crosses - * {@link DEFAULT_TRIPWIRE_THRESHOLD}, full-review mode re-arms and the - * divergent push gets the whole roster. + * 3. **Divergence tripwire.** Every full-depth review records a + * content-hashed hunk signature. Each later push compares its current + * signature against that last fully-reviewed fingerprint; when the + * unreviewed share crosses {@link DEFAULT_TRIPWIRE_THRESHOLD}, + * full-review mode re-arms and the divergent push gets the whole + * roster. + * + * **Fingerprint carriers.** The signature is written to two places and read + * back in priority order: + * + * 1. The hidden-comment stamp in the review body. This was designed as the + * durable carrier (it would survive cache eviction and branch + * protection's dismiss-stale-approvals), but gh-aw's safe-output ingest + * sanitizer strips ALL XML/HTML comments (`removeXmlComments` in + * gh-aw-actions `sanitize_content_core.cjs`), so a stamp posted through + * `submit_pull_request_review` never reaches the PR. Measured in + * production 2026-07-21 (Khan/webapp#40996: every re-review planned + * `no-prior-fingerprint` and escalated to full). The stamp is still + * emitted and still parsed first: it costs nothing, it documents the + * run, and it becomes load-bearing again the day the sanitizer allows + * it through or another submission path posts it verbatim. + * 2. The cache-memory record (`/tmp/gh-aw/cache-memory/pr-.json`), + * whose Step 9 fields (`verdict`, `stampHunks` — falling back to + * `reviewedHunks` where a consumer's Step 9 wrote the code-computed + * signature there — and `wasDraft`) carry the same information. This + * is the carrier that works today. Cache eviction degrades to `full` + * (more review, never less), which is exactly the pre-fix steady + * state. * * Two interactions are handled by construction: * @@ -343,6 +363,71 @@ export const findLatestStamp = ( return null; }; +/** + * Reconstruct a stamp from the Step 9 cache-memory record (the fallback + * fingerprint carrier; see the module header). The record is model-written + * in task mode, so every field is validated and any gap returns null: a + * fingerprint we cannot trust anchors nothing, and the depth decision + * degrades to `full`. The executed depth is not recorded there, so the + * reconstructed stamp carries `full` (the field is informational; no + * consumer branches on it). + */ +export const stampFromCacheMemory = (raw: unknown): ReReviewStamp | null => { + if (typeof raw !== "object" || raw === null || Array.isArray(raw)) { + return null; + } + const record = raw as { + verdict?: unknown; + stampHunks?: unknown; + reviewedHunks?: unknown; + wasDraft?: unknown; + }; + if (record.verdict !== "APPROVE" && record.verdict !== "REQUEST_CHANGES") { + return null; + } + if (typeof record.wasDraft !== "boolean") { + return null; + } + const validSignature = (hunks: unknown): HunkSignature | null => { + if ( + typeof hunks !== "object" || + hunks === null || + Array.isArray(hunks) + ) { + return null; + } + const signature: HunkSignature = {}; + for (const [path, hashes] of Object.entries(hunks)) { + if ( + !Array.isArray(hashes) || + hashes.some((hash) => typeof hash !== "string" || hash === "") + ) { + return null; + } + signature[path] = hashes as string[]; + } + return Object.keys(signature).length === 0 ? null : signature; + }; + // `stampHunks` is the field Step 9 copies verbatim from the plan CLI's + // own computation; `reviewedHunks` is accepted for consumers whose + // Step 9 wrote the code-computed signature there (the scripted-mode + // staging layer does). A hash-regime mismatch inside either one cannot + // be detected here; it surfaces as full divergence, i.e. a full review. + const signature = + validSignature(record.stampHunks) ?? + validSignature(record.reviewedHunks); + if (signature === null) { + return null; + } + return { + schemaVersion: STAMP_SCHEMA_VERSION, + depth: "full", + verdict: record.verdict, + anchorDraft: record.wasDraft, + anchorHunks: signature, + }; +}; + /* -------------------------------------------------------------------------- */ /* The depth decision */ /* -------------------------------------------------------------------------- */ @@ -520,10 +605,16 @@ export const buildScopedDiff = ( * `full-stripped.diff` (the provenance CLI's generated-stripped diff) over * `full.diff`, so generated churn (a lockfile push) neither enters the * fingerprint nor counts as divergence. It also reads `routing.json` (for - * `reReviewMode`), `pr-context.json` (for `isDraft`), and + * `reReviewMode`), `pr-context.json` (for `isDraft` and `number`), and * `prior-reviews.json` (the bot's prior reviews of this PR, each * `{body, submittedAt?}`, every state included, DISMISSED and COMMENTED - * too), and writes `rereview-plan.json` (the {@link ReReviewPlan}). When the + * too). When no prior-review body carries a stamp (in production none ever + * does; the ingest sanitizer strips it, see the module header), the anchor + * falls back to `/tmp/gh-aw/cache-memory/pr-.json` via + * {@link stampFromCacheMemory}. It writes `rereview-plan.json` (the + * {@link ReReviewPlan}, plus `stampSource`: + * `"review-body" | "cache-memory" | null`, recording which carrier + * anchored the plan). When the * plan stages `new-hunks` it also writes `scoped.diff` (generated-stripped * whenever the stripped diff was the input). A missing or unreadable input * degrades the plan to `full` with a fixed-format reason, never to a crash @@ -539,6 +630,7 @@ const STRIPPED_DIFF_PATH = `${REVIEW_DIR}/full-stripped.diff`; const ROUTING_PATH = `${REVIEW_DIR}/routing.json`; const PR_CONTEXT_PATH = `${REVIEW_DIR}/pr-context.json`; const PRIOR_REVIEWS_PATH = `${REVIEW_DIR}/prior-reviews.json`; +const CACHE_MEMORY_DIR = "/tmp/gh-aw/cache-memory"; const PLAN_OUT = `${REVIEW_DIR}/rereview-plan.json`; const SCOPED_DIFF_OUT = `${REVIEW_DIR}/scoped.diff`; @@ -560,10 +652,14 @@ const readJsonIfPresent = (fs: RereviewCliFs, path: string): unknown => { } }; +/** Which carrier anchored the plan's prior fingerprint. */ +export type StampSource = "review-body" | "cache-memory" | null; + export type RereviewPlanCliResult = { plan: ReReviewPlan; /** Fixed-format staging problems (each also forced the plan to full). */ warnings: string[]; + stampSource: StampSource; }; /** @@ -590,7 +686,7 @@ export const runRereviewPlanCli = ( } const prContext = readJsonIfPresent(fs, PR_CONTEXT_PATH) as - | {isDraft?: unknown} + | {isDraft?: unknown; number?: unknown} | undefined; let isDraft = false; if (prContext !== undefined && typeof prContext.isDraft === "boolean") { @@ -633,7 +729,19 @@ export const runRereviewPlanCli = ( })) : []; - const priorStamp = findLatestStamp(priorReviews); + let priorStamp = findLatestStamp(priorReviews); + let stampSource: StampSource = priorStamp === null ? null : "review-body"; + if (priorStamp === null && typeof prContext?.number === "number") { + priorStamp = stampFromCacheMemory( + readJsonIfPresent( + fs, + `${CACHE_MEMORY_DIR}/pr-${prContext.number}.json`, + ), + ); + if (priorStamp !== null) { + stampSource = "cache-memory"; + } + } const plan = decideReReviewDepth({ mode, isDraft, @@ -642,7 +750,7 @@ export const runRereviewPlanCli = ( }); fs.mkdirSync(REVIEW_DIR, {recursive: true}); - fs.writeFileSync(PLAN_OUT, JSON.stringify(plan, null, 2)); + fs.writeFileSync(PLAN_OUT, JSON.stringify({...plan, stampSource}, null, 2)); // A `new-hunks` plan implies a usable anchor (every guard that loses the // anchor resolves to full, whose staging is the whole diff). if ( @@ -657,7 +765,7 @@ export const runRereviewPlanCli = ( ); } - return {plan, warnings}; + return {plan, warnings, stampSource}; }; /** @@ -714,6 +822,7 @@ if (typeof require !== "undefined" && require.main === module) { tripwireRearmed: result.plan.tripwireRearmed, unreviewedShare: result.plan.divergence?.unreviewedShare ?? null, + stampSource: result.stampSource, warnings: result.warnings, }), ); diff --git a/workflows/review/review.md b/workflows/review/review.md index 59a35839..f288b395 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -418,7 +418,10 @@ CHANGES_REQUESTED, COMMENTED, DISMISSED), each `{"body": "...", "submittedAt": ""}`. The re-review plan CLI (Step 3) reads the hidden fingerprint stamp from these bodies; a review that branch protection dismissed, or that was submitted comment-only, still carries its stamp, which is exactly why the -state is ignored here. Do not filter or truncate the bodies. +state is ignored here. Do not filter or truncate the bodies. (In practice gh-aw's +safe-output sanitizer strips the stamp comment before the review posts, so these +bodies usually carry none; the CLI then falls back to the Step 9 cache-memory +record. Stage them anyway: the body stamp is read first whenever it exists.) ## Step 2: Early-Exit Check @@ -1651,9 +1654,17 @@ Save to `/tmp/gh-aw/cache-memory/pr-${{ github.event.pull_request.number || gith comments to hunks whose content is new since this review (Step 1 → Step 3). Record the full current signature, not just the hunks you commented on — "already reviewed" means every hunk you looked at this run. (This cache entry serves comment scoping - only; the divergence tripwire's authoritative fingerprint is the hidden stamp in - the review body, Step 6, which is exactly why the stamp exists: cache memory can - be evicted, the review body cannot.) + only; both sides of that comparison are Step 1's own added-lines hash.) +- `stampHunks`: copy **verbatim** from `rereview-plan.json`'s `stampHunks` field (the + plan CLI wrote it in Step 3). This, with `verdict` and `wasDraft`, is the divergence + tripwire's working fingerprint carrier: gh-aw's safe-output sanitizer strips the + hidden body stamp before the review posts, so the Step 6 stamp (still emitted, and + still read first if ever present) never survives to the PR today, and the next + run's plan CLI anchors on this cache record instead. Never hand-compute it: the + CLI compares it hash-for-hash against its own computation, which hashes added AND + removed lines (Step 1's added-lines hash is a different regime and must not be + mixed in). Cache eviction degrades the next run to a full review, never a cheaper + one. - `wasDraft`: whether the PR was a draft at this review (its `draft` field). Record it on every review so Step 2 can compare it against the current draft status to detect the draft→ready transition and bypass the early-exit check From db17815326afb8470b1f6586f3e8b7a3c6e71944 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 21 Jul 2026 23:08:38 -0700 Subject: [PATCH 6/6] [jwies/review-dispatch-gate-local] review: shared lenient sub-agent JSON extraction; the gate reads out-files with the dispatcher's leniency (trial run 29893634730) --- workflows/review/lib/agent-json.test.ts | 103 ++++++++++++++ workflows/review/lib/agent-json.ts | 153 +++++++++++++++++++++ workflows/review/lib/dispatch-gate.test.ts | 72 ++++++++++ workflows/review/lib/dispatch-gate.ts | 21 ++- 4 files changed, 346 insertions(+), 3 deletions(-) create mode 100644 workflows/review/lib/agent-json.test.ts create mode 100644 workflows/review/lib/agent-json.ts diff --git a/workflows/review/lib/agent-json.test.ts b/workflows/review/lib/agent-json.test.ts new file mode 100644 index 00000000..08503e05 --- /dev/null +++ b/workflows/review/lib/agent-json.test.ts @@ -0,0 +1,103 @@ +import {describe, it, expect} from "vitest"; + +import {extractJsonObject, extractJsonValue} from "./agent-json"; + +const PAYLOAD = {findings: [], hunts: [{hunt: "h1", state: "ran"}]}; + +describe("extractJsonValue", () => { + it("parses a bare JSON object", () => { + expect(extractJsonValue(JSON.stringify(PAYLOAD))).toEqual(PAYLOAD); + }); + + it("parses a bare JSON array", () => { + expect(extractJsonValue('[{"a": 1}]')).toEqual([{a: 1}]); + }); + + it("tolerates surrounding whitespace", () => { + expect( + extractJsonValue(`\n\n ${JSON.stringify(PAYLOAD)} \n`), + ).toEqual(PAYLOAD); + }); + + it("extracts the payload from prose followed by a json fence (the production correctness-reviewer shape)", () => { + const text = [ + "Investigation complete. The wrapper batches at 500, so the", + "commit-limit concern is refuted.", + "", + "```json", + JSON.stringify(PAYLOAD, null, 2), + "```", + ].join("\n"); + expect(extractJsonValue(text)).toEqual(PAYLOAD); + }); + + it("extracts unfenced trailing JSON after prose (the production claim-validator shape)", () => { + const text = [ + "All four claims are factually accurate and non-blocking:", + "- **test-adequacy-1**: Confirmed.", + "", + JSON.stringify({claims: [{id: "x", verification: "confirmed"}]}), + ].join("\n"); + expect(extractJsonValue(text)).toEqual({ + claims: [{id: "x", verification: "confirmed"}], + }); + }); + + it("prefers the last fence over an earlier quoted example", () => { + const text = [ + "Per the contract:", + "```json", + '{"example": true}', + "```", + "Here is my actual result:", + "```json", + JSON.stringify(PAYLOAD), + "```", + ].join("\n"); + expect(extractJsonValue(text)).toEqual(PAYLOAD); + }); + + it("survives prose braces before the payload", () => { + const text = `The {} literal and {"tiny": 1} appear in prose. ${JSON.stringify( + PAYLOAD, + )}`; + // The longest parseable span wins, not the first. + expect(extractJsonValue(text)).toEqual(PAYLOAD); + }); + + it("handles braces inside JSON strings", () => { + const tricky = {note: 'a "}" inside a string { should not confuse'}; + expect(extractJsonValue(`prose ${JSON.stringify(tricky)}`)).toEqual( + tricky, + ); + }); + + it("returns undefined on pure prose", () => { + expect(extractJsonValue("no JSON here, just words")).toBeUndefined(); + }); + + it("returns undefined on a bare primitive (no contract is a primitive)", () => { + expect(extractJsonValue("42")).toBeUndefined(); + expect(extractJsonValue('"ok"')).toBeUndefined(); + }); + + it("returns undefined on an unbalanced fragment", () => { + expect(extractJsonValue('{"findings": [')).toBeUndefined(); + }); +}); + +describe("extractJsonObject", () => { + it("narrows to a plain object", () => { + expect(extractJsonObject(JSON.stringify(PAYLOAD))).toEqual(PAYLOAD); + }); + + it("rejects a top-level array", () => { + expect(extractJsonObject('[{"a": 1}]')).toBeUndefined(); + }); + + it("finds the object when prose precedes it", () => { + expect( + extractJsonObject(`Result follows. ${JSON.stringify(PAYLOAD)}`), + ).toEqual(PAYLOAD); + }); +}); diff --git a/workflows/review/lib/agent-json.ts b/workflows/review/lib/agent-json.ts new file mode 100644 index 00000000..e24706f0 --- /dev/null +++ b/workflows/review/lib/agent-json.ts @@ -0,0 +1,153 @@ +/** + * Lenient JSON extraction from a sub-agent's final text. + * + * Sub-agent output contracts say "return ONLY the JSON object", but models + * routinely prefix prose or wrap the payload in a code fence (measured in + * production: run 29893634730's `out/correctness-reviewer.json` and + * `out/claim-validator.json` both carried prose before a valid payload). + * Every consumer of a sub-agent's raw text — the dispatcher parsing a + * contract, the conformance gate checking that an output exists and parses — + * must apply the SAME leniency, or the two disagree about the same file: + * the dispatcher accepts what the gate calls unparseable, and a conforming + * run fails the gate. This module is that single shared rule. + * + * Extraction order: + * 1. The whole text, strictly. + * 2. Fenced code blocks (``` with or without a language tag), last first: + * an agent's real payload is its final fence; earlier fences are + * usually quoted examples. + * 3. Balanced `{...}` / `[...]` spans (string- and escape-aware), longest + * parseable span first: prose braces produce tiny false candidates + * (`{}` in a sentence), and the contract payload is with overwhelming + * likelihood the longest valid span. + * + * Determinism boundary: pure function of the text; no model call, no + * filesystem. + */ + +/** One fenced code block's inner text, in document order. */ +const fencedBlocks = (text: string): string[] => { + const blocks: string[] = []; + const fence = /```[^\n]*\n([\s\S]*?)```/g; + for (let m = fence.exec(text); m !== null; m = fence.exec(text)) { + blocks.push(m[1]); + } + return blocks; +}; + +/** + * Every balanced top-level `{...}` or `[...]` span in the text, found by a + * string-aware depth scan. Spans nested inside a larger balanced span are + * not re-reported (the outer span is the candidate; if it fails to parse, + * the scan continues after its opening character, so inner spans still get + * their turn). + */ +const balancedSpans = (text: string, cap = 200): string[] => { + const spans: string[] = []; + let i = 0; + while (i < text.length && spans.length < cap) { + const ch = text[i]; + if (ch !== "{" && ch !== "[") { + i++; + continue; + } + const close = ch === "{" ? "}" : "]"; + let depth = 0; + let inString = false; + let escaped = false; + let end = -1; + for (let j = i; j < text.length; j++) { + const c = text[j]; + if (inString) { + if (escaped) { + escaped = false; + } else if (c === "\\") { + escaped = true; + } else if (c === '"') { + inString = false; + } + continue; + } + if (c === '"') { + inString = true; + } else if (c === "{" || c === "[") { + depth++; + } else if (c === "}" || c === "]") { + depth--; + if (depth === 0) { + end = c === close ? j : -1; + break; + } + } + } + if (end === -1) { + i++; + continue; + } + spans.push(text.slice(i, end + 1)); + // Continue INSIDE the span too: if the outer candidate fails to + // parse, an inner one may be the real payload. + i++; + } + return spans; +}; + +const tryParse = (candidate: string): unknown => { + try { + return JSON.parse(candidate) as unknown; + } catch { + return undefined; + } +}; + +/** + * Extract the JSON value (object or array) from an agent's final text, per + * the module rule. Returns undefined when no candidate parses. A bare + * primitive (`"ok"`, `42`) is deliberately NOT extracted: no sub-agent + * contract is a primitive, and prose fragments parse as primitives far too + * easily. + */ +export const extractJsonValue = (text: string): unknown => { + const whole = tryParse(text.trim()); + if (whole !== undefined && typeof whole === "object" && whole !== null) { + return whole; + } + const fences = fencedBlocks(text); + for (let i = fences.length - 1; i >= 0; i--) { + const parsed = tryParse(fences[i].trim()); + if ( + parsed !== undefined && + typeof parsed === "object" && + parsed !== null + ) { + return parsed; + } + } + const spans = balancedSpans(text).sort((a, b) => b.length - a.length); + for (const span of spans) { + const parsed = tryParse(span); + if ( + parsed !== undefined && + typeof parsed === "object" && + parsed !== null + ) { + return parsed; + } + } + return undefined; +}; + +/** + * {@link extractJsonValue} narrowed to a plain object, the shape every + * sub-agent contract uses at the top level. Arrays and null return + * undefined. + */ +export const extractJsonObject = ( + text: string, +): Record | undefined => { + const value = extractJsonValue(text); + if (typeof value !== "object" || value === null || Array.isArray(value)) { + return undefined; + } + return value as Record; +}; diff --git a/workflows/review/lib/dispatch-gate.test.ts b/workflows/review/lib/dispatch-gate.test.ts index b5bb094c..dd645106 100644 --- a/workflows/review/lib/dispatch-gate.test.ts +++ b/workflows/review/lib/dispatch-gate.test.ts @@ -854,3 +854,75 @@ describe("third-round nits: keep-list survivors, template coupling, summary", () expect(renderGateSummary(report)).toContain("Conformant."); }); }); + +/* -------------------------------------------------------------------------- */ +/* Lenient out-file parsing (run 29893634730) */ +/* -------------------------------------------------------------------------- */ + +describe("prose-tolerant out-file parsing", () => { + // The production shape that falsely blocked a conforming scripted run: + // sub-agents prefix prose (and fence the payload) despite the "JSON + // only" contract, and the dispatcher stages their final text verbatim. + const prosePrefixedValidator = [ + "All four claims are factually accurate and non-blocking:", + "- **test-adequacy-1**: Confirmed.", + "", + JSON.stringify({claims: [{id: "x", verification: "confirmed"}]}), + ].join("\n"); + + const fencedCorrectness = [ + "Investigation complete. The commit-limit concern is refuted.", + "", + "```json", + JSON.stringify({files: [], findings: []}), + "```", + ].join("\n"); + + it("accepts a prose-prefixed validator output (rule 2)", () => { + const outFiles = conformingOutFiles(); + outFiles["claim-validator.json"] = prosePrefixedValidator; + const result = evaluate({ + items: [commentItem(), submitItem("APPROVE", "ok")], + outFiles, + }); + expect(result.violations.map((v) => v.code)).toEqual([]); + }); + + it("accepts a fence-wrapped correctness output (rule 1)", () => { + const outFiles = conformingOutFiles(); + outFiles["correctness-reviewer.json"] = fencedCorrectness; + const result = evaluate({ + items: [submitItem("APPROVE", "ok")], + outFiles, + }); + expect(result.violations.map((v) => v.code)).toEqual([]); + }); + + it("still flags a validator file with no JSON payload at all", () => { + const outFiles = conformingOutFiles(); + outFiles["claim-validator.json"] = "I could not finish the audit."; + const result = evaluate({ + items: [commentItem(), submitItem("APPROVE", "ok")], + outFiles, + }); + expect(result.violations.map((v) => v.code)).toEqual([ + "validator-missing-with-findings", + ]); + }); + + it("reads a prose-wrapped triage empty-reviewFiles waiver", () => { + const result = evaluate({ + items: [submitItem("APPROVE", "ok")], + outFiles: { + "pattern-triage.json": [ + "Everything in this diff is generated.", + "```json", + JSON.stringify({patterns: [], reviewFiles: []}), + "```", + ].join("\n"), + }, + }); + expect(result.conformant).toBe(true); + expect(result.notes.join(" ")).toContain("empty reviewFiles"); + }); +}); diff --git a/workflows/review/lib/dispatch-gate.ts b/workflows/review/lib/dispatch-gate.ts index 8b142bd5..d7fcbb62 100644 --- a/workflows/review/lib/dispatch-gate.ts +++ b/workflows/review/lib/dispatch-gate.ts @@ -60,6 +60,8 @@ * files; no model call, no clock, no prose about the code under review. */ +import {extractJsonValue} from "./agent-json"; + /* -------------------------------------------------------------------------- */ /* Types */ /* -------------------------------------------------------------------------- */ @@ -142,6 +144,16 @@ const parseJson = (text: string): unknown => { } }; +/** + * Sub-agent OUT-FILE parses use the shared lenient extraction + * (`agent-json.ts`), the same rule the dispatcher applies: a prose-prefixed + * or fence-wrapped payload is an output that exists, and the gate must not + * call unparseable what the dispatcher parsed (run 29893634730 blocked a + * conforming submission exactly that way). Code-written inputs (the agent + * output queue, staged routing) stay on strict {@link parseJson}. + */ +const parseAgentOutFile = (text: string): unknown => extractJsonValue(text); + /** * Lowercase and collapse the separator variants (`-`, `_`, `/`) note authors * use, so `test-adequacy` matches "test adequacy" and `security-auth` @@ -227,7 +239,9 @@ const triageEmptiedReview = (outFiles: Record): boolean => { if (raw === undefined) { return false; } - const parsed = parseJson(raw) as {reviewFiles?: unknown} | undefined; + const parsed = parseAgentOutFile(raw) as + | {reviewFiles?: unknown} + | undefined; return ( parsed !== undefined && Array.isArray(parsed.reviewFiles) && @@ -282,7 +296,7 @@ export const evaluateDispatchConformance = ( `(depth ${depth} dispatches the correctness pass; even a failed dispatch stages an error note)`, }); } else if ( - parseJson(raw) === undefined && + parseAgentOutFile(raw) === undefined && !disclosesSkippedDimension(body, "correctness-reviewer") ) { violations.push({ @@ -300,7 +314,8 @@ export const evaluateDispatchConformance = ( // hard ceiling, and never silently). if (commentCount > 0) { const raw = input.outFiles[VALIDATOR_OUT]; - const validated = raw !== undefined && parseJson(raw) !== undefined; + const validated = + raw !== undefined && parseAgentOutFile(raw) !== undefined; if (!validated && !disclosesSkippedDimension(body, "claim-validator")) { violations.push({ code: "validator-missing-with-findings",