diff --git a/.changeset/runner-metering-and-riders.md b/.changeset/runner-metering-and-riders.md new file mode 100644 index 00000000..afe72029 --- /dev/null +++ b/.changeset/runner-metering-and-riders.md @@ -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). diff --git a/.claude/skills/review-consumer-bump/SKILL.md b/.claude/skills/review-consumer-bump/SKILL.md index 2b46f9d9..b7e14eab 100644 --- a/.claude/skills/review-consumer-bump/SKILL.md +++ b/.claude/skills/review-consumer-bump/SKILL.md @@ -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 ls-files | node -r @swc-node/register \ diff --git a/workflows/review/lib/check-consumer-config.test.ts b/workflows/review/lib/check-consumer-config.test.ts index ca219cef..a55c6c31 100644 --- a/workflows/review/lib/check-consumer-config.test.ts +++ b/workflows/review/lib/check-consumer-config.test.ts @@ -706,7 +706,7 @@ describe("the repo root", () => { content, ]), ); - const report = checkConsumerConfig(fakeFs(inputs), { + const report = check(inputs, { repoRoot: "../consumer", checkerVersion: "1.11.0", }); diff --git a/workflows/review/lib/dedup-adjudicated.test.ts b/workflows/review/lib/dedup-adjudicated.test.ts index ccd38a69..2f65a70b 100644 --- a/workflows/review/lib/dedup-adjudicated.test.ts +++ b/workflows/review/lib/dedup-adjudicated.test.ts @@ -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"; /** @@ -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", () => { diff --git a/workflows/review/lib/dedup-threads.ts b/workflows/review/lib/dedup-threads.ts index 0d2884f4..55757c0d 100644 --- a/workflows/review/lib/dedup-threads.ts +++ b/workflows/review/lib/dedup-threads.ts @@ -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}`, ); diff --git a/workflows/review/lib/dispatch-runner.test.ts b/workflows/review/lib/dispatch-runner.test.ts index 9ff278bd..3866b8f6 100644 --- a/workflows/review/lib/dispatch-runner.test.ts +++ b/workflows/review/lib/dispatch-runner.test.ts @@ -125,15 +125,17 @@ 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", @@ -141,12 +143,86 @@ describe("createSdkRunner submit_result (trial suggestion h)", () => { 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 () => { @@ -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); diff --git a/workflows/review/lib/dispatch-runner.ts b/workflows/review/lib/dispatch-runner.ts index 6dcb05af..a8595f83 100644 --- a/workflows/review/lib/dispatch-runner.ts +++ b/workflows/review/lib/dispatch-runner.ts @@ -98,6 +98,12 @@ export const createSdkRunner = async (): Promise => { // `captured ?? provisional` — the styled acceptance when one // happened, else the best contract-valid submission seen. let provisional: Record | undefined; + // How many times the agent called submit_result at all, counted + // BEFORE the contract check: the Stop-hook reason branches on this, + // and a contract-bounced agent (validate non-null, so neither + // `captured` nor `provisional` is set) is still mid-correction, not + // an agent that never delivered. + let submitAttempts = 0; const validate = request.validate; if (validate !== undefined) { options.mcpServers = { @@ -109,6 +115,7 @@ export const createSdkRunner = async (): Promise => { "Deliver your final structured result. Pass the entire output-contract JSON object as `result`.", {result: z.record(z.string(), z.unknown())}, async (args) => { + submitAttempts += 1; const payload = args["result"] as Record< string, unknown @@ -133,11 +140,12 @@ export const createSdkRunner = async (): Promise => { // same way a contract rejection does (the // plain-prose loop: the pinned judge model // scores, the author rewrites in-session - // with its repo context intact). The gate caps its own bounces and - // fails open, and `provisional` above keeps - // the pre-style payload salvageable, so this - // await can slow a submission or cost prose - // quality, never lose one. + // with its repo context intact). The gate + // caps its own bounces and fails open, and + // `provisional` above keeps the pre-style + // payload salvageable, so this await can + // slow a submission or cost prose quality, + // never lose one. if (request.judgeProse !== undefined) { const styleRejection = await request.judgeProse(payload); @@ -168,30 +176,37 @@ export const createSdkRunner = async (): Promise => { }), }; allowedTools.push("mcp__review__submit_result"); - // The Stop hook: an agent ending its turn WITHOUT having called - // submit_result is heading for the free-text fallback, which - // skips both the in-session contract bounce and the prose gate. - // Block the stop (the model sees `reason` and continues) and - // point it back at the tool, at most twice: past the cap a - // confused agent gets its genuine fallback rather than a loop, - // and dispatch.ts records its findings as skipped by the gate. + // The Stop hook: an agent ending its turn WITHOUT an accepted + // submission is heading for the free-text fallback, which skips + // both the in-session contract bounce and the prose gate. Block + // the stop (the model sees `reason` and continues) and point it + // back at the tool, at most twice: past the cap a confused agent + // gets its genuine fallback rather than a loop, and dispatch.ts + // records its findings as skipped by the gate. `captured` empty + // does NOT mean "never called": a submission the contract or + // prose gate bounced leaves it empty too, so the reason branches + // on whether submit_result was ever called (either bounce kind + // leaves the agent mid-correction, not undelivered). let stopBlocks = 0; options.hooks = { Stop: [ { hooks: [ - () => { + async () => { if ( captured === undefined && stopBlocks < MAX_STOP_BLOCKS ) { stopBlocks += 1; - return Promise.resolve({ + return { decision: "block" as const, - reason: "You have not delivered your result yet. Call the submit_result tool ONCE now, passing the ENTIRE JSON object your output contract specifies as its `result` argument; do not paste the JSON as a message.", - }); + reason: + submitAttempts === 0 + ? "You have not delivered your result yet. Call the submit_result tool ONCE now, passing the ENTIRE JSON object your output contract specifies as its `result` argument; do not paste the JSON as a message." + : "Your submission was rejected and must be corrected. Rewrite what the rejection message named and call submit_result again with the full corrected result object; do not paste the JSON as a message.", + }; } - return Promise.resolve({}); + return {}; }, ], }, @@ -204,6 +219,16 @@ export const createSdkRunner = async (): Promise => { // a usable final already written; the catch below salvages this text // so the redirect can cost turns, never the output. let lastText: string | undefined; + // Metering from a NON-success result record (error_max_turns et al. + // still carry total_cost_usd/num_turns): captured before the throw + // so the salvage paths report what the run really cost instead of a + // systematic zero (the Stop hook makes non-success endings common + // for free-text agents, so the zeros were not a rare best-effort + // case but a standing undercount in dispatch's perAgent entries and + // the totalUsd summed over them). + let endedUsd = 0; + let endedTurns = 0; + let ended = false; try { const run = sdk.query({prompt: request.prompt, options}); let output = ""; @@ -245,6 +270,9 @@ export const createSdkRunner = async (): Promise => { continue; } if (message["subtype"] !== "success") { + endedUsd = Number(message["total_cost_usd"] ?? 0); + endedTurns = Number(message["num_turns"] ?? 0); + ended = true; throw new Error( `sub-agent ended without success: ${String( message["subtype"], @@ -278,17 +306,18 @@ export const createSdkRunner = async (): Promise => { wallMs: Date.now() - started, }; } catch (error) { - // A payload the tool already accepted is complete and validated: - // salvage it even when the session then dies (a hang after - // submission, a max-turns overrun). Cost fields are best-effort - // zero here; the metered proxy still charged the run, but the - // SDK never delivered its result record. + // A payload the tool accepted (or a contract-valid one the prose + // gate was still bouncing) is complete: salvage it even when the + // session then dies (a hang after submission, a max-turns + // overrun). Cost fields come from the non-success result record + // when the SDK delivered one; they are best-effort zero only + // when the stream died with no record at all. const salvage = captured ?? provisional; if (salvage !== undefined) { return { output: JSON.stringify(salvage), - usd: 0, - turns: 0, + usd: endedUsd, + turns: endedTurns, wallMs: Date.now() - started, structured: true, }; @@ -305,15 +334,19 @@ export const createSdkRunner = async (): Promise => { `sub-agent timed out after ${request.timeoutMs}ms`, ); } - // No structured payload, but the agent did write a final: the - // free-text fallback path. Return it instead of discarding a - // usable output because the session then died (the Stop hook - // makes that ending common for free-text agents). - if (lastText !== undefined) { + // No structured payload, but the agent did write a final AND + // the session ended with a delivered result record (the + // max-turns shape the Stop hook makes common): the free-text + // fallback path. Gated on `ended`, not on any error: a hard + // failure mid-stream has no result record and its lastText is + // mid-investigation narration, which would fail the contract + // parse and burn the malformed-output re-dispatch, exactly like + // the timeout case above. + if (ended && lastText !== undefined) { return { output: lastText, - usd: 0, - turns: 0, + usd: endedUsd, + turns: endedTurns, wallMs: Date.now() - started, }; } diff --git a/workflows/review/lib/rereview.test.ts b/workflows/review/lib/rereview.test.ts index 0f153c74..36e5d891 100644 --- a/workflows/review/lib/rereview.test.ts +++ b/workflows/review/lib/rereview.test.ts @@ -725,9 +725,13 @@ describe("acknowledged threads (the will-fix signal, webapp#41290)", () => { }); it("never counts bot replies (thumbs follow-ups, autofix)", () => { - // The sweep's follow-up and autofix's replies sit on exactly - // these threads; a reconciler hallucinating a concession out of - // one must contribute nothing. + // Autofix's replies (and retired sweep follow-ups on older + // threads) sit on exactly these threads; a reconciler + // hallucinating a concession out of one must contribute nothing. + // t1 is the spelling staged threads actually carry (GraphQL's + // bare login, caught by isReviewBotAuthor); t2's `[bot]` suffix + // is the REST spelling, unreachable from today's staging and + // kept only to pin the isBotLogin belt-and-suspenders clause. const ids = verifiedAcknowledgedIds( {resolve: [], keep: ["t1", "t2"], acknowledged: ["t1", "t2"]}, [ diff --git a/workflows/review/lib/rereview.ts b/workflows/review/lib/rereview.ts index fd71588a..99986c8b 100644 --- a/workflows/review/lib/rereview.ts +++ b/workflows/review/lib/rereview.ts @@ -108,15 +108,19 @@ export const verifiedAcknowledgedIds = ( continue; } const thread = threads.find((t) => t.thread_id === id); - const authorReplied = thread?.comments - .slice(1) - .some( - (comment) => - comment.author !== "" && - !isReviewBotAuthor(comment.author) && - !isBotLogin(comment.author) && - sameLogin(comment.author, prAuthor), - ); + const authorReplied = thread?.comments.slice(1).some( + (comment) => + comment.author !== "" && + !isReviewBotAuthor(comment.author) && + // Belt-and-suspenders only: staged threads carry + // GraphQL's bare logins, which never end in `[bot]`, so + // this clause fires only if staging ever switches to the + // REST spelling. The guards that actually hold are the + // bot-PR-author early return above and the sameLogin + // comparison below. + !isBotLogin(comment.author) && + sameLogin(comment.author, prAuthor), + ); if (authorReplied === true) { verified.add(id); }