Skip to content

review: real metering on salvage paths; gate lastText on a delivered result record #377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jwbron merged 2 commits into from
Aug 25, 2026
Merged

review: real metering on salvage paths; gate lastText on a delivered result record #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe5b3d8
review: real metering on salvage paths; gate lastText on a delivered …
jwbron Aug 24, 2026
9ca0d43
review: branch the Stop reason on an attempt counter; score the band …
jwbron Aug 24, 2026
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/runner-metering-and-riders.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"review": patch
---

Sub-agent salvage paths stop reporting `usd: 0, turns: 0` for sessions that really ran. A non-success result record (`error_max_turns` et al.) still carries `total_cost_usd`/`num_turns`; the runner now captures them before throwing, so the captured/provisional and lastText salvages report real metering, and the Stop hook's common max-turns ending no longer systematically undercounts dispatch's `perAgent` entries and the `totalUsd` summed over them (zeros remain when the stream dies with no result record at all, and on the rethrow path, where dispatch.ts's run-failed entry has no channel for a cost carried on an error). The lastText salvage is also gated on a delivered result record: a hard failure mid-stream leaves narration, not a final, and salvaging it burned the malformed-output re-dispatch. The Stop hook's block reason now distinguishes "never called submit_result" from "your submission was bounced, correct and resubmit", branching on an attempt counter so both bounce kinds (contract and prose gate) are covered (an agent mid-bounce was being told it had not delivered). Riders from merged-PR review folds: the adjudicated cross-file hard-negative fixture asserts its calibration band (bigram and overlap floors cleared, jaccard rejecting) instead of only documenting it, the acknowledgment guard's dead `isBotLogin` clause is documented as belt-and-suspenders (staged threads carry GraphQL's bare logins) with the test spelling annotated, and the consumer-bump skill stops describing the now-guarded `--repo` footgun as silent (Khan/actions#372 made the checker fail loudly).
5 changes: 3 additions & 2 deletions .claude/skills/review-consumer-bump/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,9 @@ effects, each observed on v0.85.4:
whatever tag you had lying around: a version-skewed checker produces
phantom warnings (checking 1.17.0 pins with the 1.16.0 checker added one
spurious warning per repo). `--repo` takes a **path** to the consumer
checkout, not a repo name; a name silently resolves as a nonexistent path
and reports everything missing:
checkout, not a repo name; the checker fails loudly on a nonexistent path
(Khan/actions#372), so a name like `Khan/webapp` dies with an error naming
the path instead of reporting everything missing:

```sh
git -C <consumer> ls-files | node -r @swc-node/register \
Expand Down
2 changes: 1 addition & 1 deletion workflows/review/lib/check-consumer-config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ describe("the repo root", () => {
content,
]),
);
const report = checkConsumerConfig(fakeFs(inputs), {
const report = check(inputs, {
repoRoot: "../consumer",
checkerVersion: "1.11.0",
});
Expand Down
18 changes: 17 additions & 1 deletion workflows/review/lib/dedup-adjudicated.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ import {
suppressAdjudicatedDuplicates,
suppressTrackedDuplicates,
} from "./dedup-adjudicated";
import {suppressOpenThreadDuplicates} from "./dedup-threads";
import {OTHER_LINE_FLOOR} from "./dedup-text";
import {openThreadScore, suppressOpenThreadDuplicates} from "./dedup-threads";
import type {Claim} from "./dispatch-contracts";

/**
Expand Down Expand Up @@ -341,6 +342,21 @@ describe("cross-file adjudicated suppression (the path key dropped)", () => {
);
expect(kept).toEqual([negative]);
expect(suppressed).toEqual([]);
// Assert the calibration band, not just the outcome: a fixture that
// quietly drifted out of the band (rejected on bigrams instead of
// jaccard) would still pass the kept assertion while pinning
// nothing. Scored through openThreadScore itself, so the assertion
// measures exactly what production measures (threadProse strip
// included).
const {jaccard, overlap, sharedBigrams} = openThreadScore(
negative,
adjudicatedThread(),
);
expect(sharedBigrams).toBeGreaterThanOrEqual(
OTHER_LINE_FLOOR.sharedBigrams,
);
expect(overlap).toBeGreaterThanOrEqual(OTHER_LINE_FLOOR.overlap);
expect(jaccard).toBeLessThan(OTHER_LINE_FLOOR.jaccard);
});

it("picks the best-scoring adjudicated thread across files, independent of staging order", () => {
Expand Down
11 changes: 10 additions & 1 deletion workflows/review/lib/dedup-threads.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,16 @@ type OpenThreadScore = {
sharedBigrams: number;
};

const openThreadScore = (claim: Claim, thread: OpenThread): OpenThreadScore => {
/**
* Exported for the calibration tests: the adjudicated hard-negative fixture
* asserts its band (bigrams/overlap clearing, jaccard rejecting) against
* THIS function, so the assertion scores exactly what production scores
* (threadProse strip included) instead of re-deriving the formula.
*/
export const openThreadScore = (
claim: Claim,
thread: OpenThread,
): OpenThreadScore => {
const tokensA = contentTokens(
`${claim.subject} ${claim.discussion} ${claim.failure_scenario}`,
);
Expand Down
130 changes: 125 additions & 5 deletions workflows/review/lib/dispatch-runner.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,28 +125,104 @@ describe("createSdkRunner submit_result (trial suggestion h)", () => {
const result = await (await createSdkRunner())(request());
expect(result.structured).toBe(true);
expect(JSON.parse(result.output)).toEqual({findings: []});
// Cost fields are best-effort zero: the SDK never delivered its
// result record.
// Cost fields are best-effort zero ONLY here: the stream died with
// no result record of any subtype, so there is nothing to report.
expect(result.usd).toBe(0);
});

it("salvages the last assistant text when the session dies without success", async () => {
// The Stop hook pushes a free-text agent to keep going, so it can
// burn its last turns being redirected and end on error_max_turns
// with a usable final already written; that text is the output.
// with a usable final already written; that text is the output, and
// the metering comes from the non-success result record rather than
// a systematic zero (the record still carries cost and turns).
session = async function* () {
yield {
type: "assistant",
message: {
content: [{type: "text", text: "the free-text findings"}],
},
};
yield {type: "result", subtype: "error_max_turns"};
yield {
type: "result",
subtype: "error_max_turns",
total_cost_usd: 1.5,
num_turns: 100,
};
};
const result = await (await createSdkRunner())(request());
expect(result.structured).toBeUndefined();
expect(result.output).toBe("the free-text findings");
expect(result.usd).toBe(0);
expect(result.usd).toBe(1.5);
expect(result.turns).toBe(100);
});

it("the LAST assistant text wins when several were emitted", async () => {
// "last" was previously unasserted: an agent narrates before its
// final, and salvaging the narration instead of the final would
// silently ship the wrong text.
session = async function* () {
for (const text of ["narration", "the real final"]) {
yield {
type: "assistant",
message: {content: [{type: "text", text}]},
};
}
yield {type: "result", subtype: "error_max_turns"};
};
const result = await (await createSdkRunner())(request());
expect(result.output).toBe("the real final");
});

it("does not salvage lastText on a hard failure with no result record", async () => {
// The gate on the lastText salvage: a session that dies mid-stream
// (no result record of any subtype) left narration, not a final;
// returning it would fail the contract parse and burn the
// malformed-output re-dispatch, exactly like the timeout case.
session = async function* () {
yield {
type: "assistant",
message: {content: [{type: "text", text: "narration"}]},
};
throw new Error("stream died");
};
await expect((await createSdkRunner())(request())).rejects.toThrow(
/stream died/,
);
});

it("a bounced provisional payload outranks lastText and reports the run's metering", async () => {
// provisional-beats-lastText in the catch path: a contract-valid
// submission the prose gate was still bouncing salvages as the
// structured output even though a later free-text final exists.
session = async function* (tools) {
const bounced = await tools[0].handler(
{result: {findings: [{id: "styled"}]}},
undefined,
);
expect(bounced.isError).toBe(true);
yield {
type: "assistant",
message: {content: [{type: "text", text: "free-text final"}]},
};
yield {
type: "result",
subtype: "error_max_turns",
total_cost_usd: 2.25,
num_turns: 42,
};
};
const result = await (
await createSdkRunner()
)(
request({
judgeProse: async () => "prose rejected: too poetic",
}),
);
expect(result.structured).toBe(true);
expect(JSON.parse(result.output)).toEqual({findings: [{id: "styled"}]});
expect(result.usd).toBe(2.25);
expect(result.turns).toBe(42);
});

it("reports a timeout instead of salvaging mid-investigation narration", async () => {
Expand Down Expand Up @@ -391,6 +467,50 @@ describe("createSdkRunner Stop hook (the free-text fallback funnel)", () => {
expect(await hook!()).toEqual({});
});

it("names the mid-bounce state when a prose-gate-rejected submission exists", async () => {
session = async function* (tools) {
const bounced = await tools[0].handler(
{result: {findings: [{id: "pre-style"}]}},
undefined,
);
expect(bounced.isError).toBe(true);
yield success("free text");
};
await (
await createSdkRunner()
)(
request({
judgeProse: () => Promise.resolve("Result rejected: style"),
}),
);
const blocked = await stopHook()!();
expect(blocked).toMatchObject({decision: "block"});
// The contract-valid payload is mid-bounce: telling the agent it
// never delivered is the falsehood this branch removes.
expect(String(blocked["reason"])).toContain("was rejected");
expect(String(blocked["reason"])).not.toContain("have not delivered");
});

it("names the mid-bounce state for a CONTRACT bounce too (no provisional exists)", async () => {
// A contract bounce sets neither captured nor provisional; only the
// attempt counter knows the agent already called the tool.
session = async function* (tools) {
const bounced = await tools[0].handler(
{result: {finding: "singular, drifted"}},
undefined,
);
expect(bounced.isError).toBe(true);
yield success("free text");
};
await (
await createSdkRunner()
)(request());
const blocked = await stopHook()!();
expect(blocked).toMatchObject({decision: "block"});
expect(String(blocked["reason"])).toContain("was rejected");
expect(String(blocked["reason"])).not.toContain("have not delivered");
});

it("lets a stop through once a payload was accepted", async () => {
session = async function* (tools) {
await tools[0].handler({result: {findings: []}}, undefined);
Expand Down
Loading
Loading