Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/review-dispatch-gate.md
Original file line number Diff line number Diff line change
@@ -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.
7 changes: 7 additions & 0 deletions .changeset/stamp-carrier-cache-memory.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"review": patch
---

review: the re-review fingerprint anchors on cache memory; the body stamp never survives gh-aw ingest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (non-blocking): This changeset body opens with a review: prefix, unlike every other changeset in the repo — its sibling .changeset/review-dispatch-gate.md opens "The dispatch-conformance gate: ...". The prefix duplicates the "review" scope already declared in the frontmatter and renders straight into the published CHANGELOG. Consider dropping it and capitalizing: 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.
15 changes: 15 additions & 0 deletions workflows/review/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
103 changes: 103 additions & 0 deletions workflows/review/lib/agent-json.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
153 changes: 153 additions & 0 deletions workflows/review/lib/agent-json.ts
Original file line number Diff line number Diff line change
@@ -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<string, unknown> | undefined => {
const value = extractJsonValue(text);
if (typeof value !== "object" || value === null || Array.isArray(value)) {
return undefined;
}
return value as Record<string, unknown>;
};
Loading
Loading