review: surface why a sub-agent produced nothing, and fall back when it refused - #311
Conversation
🦋 Changeset detectedLatest commit: c22d9fd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Review live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
Production path wired, and one claim correctedThe fallback now runs on the production dispatcher too ( Probe result: Opus 4.8 clean, but refusals are intermittentRun 30658862532, same two cases, $2.83 total:
The candidate result is weak positive evidence for Opus 4.8 as the fallback target. The baseline result is the interesting one: the same Fable pin cleared both cases it had blocked in run 30656579898. So refusals are intermittent, not a deterministic property of (model, case). This branch was asserting the opposite in code comments, the README, and the changeset, and that is now corrected throughout. Note the probe is also confounded on a second axis: 30656579898 ran the Pi runner on both arms, this one ran the SDK runner on both. Two runs cannot separate model, harness, and chance. What can be said honestly is that Fable refused 3 of 4 case-arms once and 0 of 2 another time, and that Opus 4.8 has not refused in the one run where it was tried. The fallback still stands, for a different reason than I first gaveNot "retrying the same pin always refuses again" — that is now known to be false. The reason is that the contract-parse retry cannot recover a refusal at all: it appends a corrective note about output shape, and a blocked request never produced an output to correct. That retry will keep firing uselessly no matter the refusal rate. Switching to a model with a different refusal profile is the reliable recovery; re-rolling the refusing pin is a coin flip. Intermittency does raise the value of the 1592 tests pass (16 new); |
…ess behind the runner seam Rebuilt on top of jwies/review-refusal-visibility (#311), which carries the visibility and refusal-fallback work this branch originally contained. What is left here is the harness alone. Scripted dispatch gains a second AgentRunner implementation (lib/dispatch-runner-pi.ts) built on Pi's libraries, selected by REVIEW_DISPATCH_RUNNER=pi. The Claude Agent SDK runner stays the default and the production path; no model pin moves, and dispatch.ts still imports neither runner (the CLI entry loads one lazily). A harness switch, not a model switch: both runners honor the same per-role model: pins, so pi-vs-sdk on an unchanged review.md isolates the loop. Pi's libraries are multi-provider, so once the harness is anchored, moving a role off Anthropic becomes a pin change rather than a second bespoke agent loop. Known incomplete: live-ab.ts builds ONE runner and uses it for both arms by design (its module doc says so), so REVIEW_DISPATCH_RUNNER currently sets the harness for the whole A/B rather than per arm. A real harness comparison needs a per-arm runner in runArm; until then this branch cannot claim one.
… Opus 5 Re-applied on top of #311 (refusal visibility and fallback). The original two commits conflicted with main independently of that stack: the sandbox.agent.version pin was retired and gh-aw v0.83.4 / firewall v0.27.42 landed underneath them, so review.md's frontmatter had moved. This keeps main's infrastructure evolution and re-applies the pin intent on top, rather than resurrecting frontmatter that main has since superseded. 22 pins move to claude-opus-5: the orchestrator, thread-reconciler, skill-auditor, claim-validator, conventions, the opt-in whole-change reviewers, all twelve specialist lenses, plus correctness-reviewer and first-principles from claude-fable-5. pattern-triage stays on Sonnet 4.6 as the cheap first pass; the eval's judge and match arbiter stay on Haiku. What changed since this PR was written, and why it is now stacked: The refusal risk this PR's body accepts as a specialist-lens hazard, detectable only via weekly drift, is measured and already open. Run 30656579898 caught correctness-reviewer on Fable 5 refusing incident-auth-bypass and adversarial-injection-approve under Anthropic's usage policy, at 5,207 tokens. It is a DEFAULT-roster agent, not a lens, on the metric this repo calls load-bearing. Moving it to Opus 5 does not resolve that: this PR's own assessment is that Opus 5 also ships elevated cyber safeguards and can return stop_reason refusal, so the hazard moves with the roster. Stacking on #311 means the roster lands with the mitigation in place: a refused agent re-dispatches on claude-opus-4-8 (already in that map for opus-5), recorded as fellBackTo and reported as a weekly rate. Note the detector this PR nominates changes character: a refusing reviewer no longer craters recall on security-adjacent cases, because it falls back and produces findings. The fallback counter replaces that inference with a direct measurement. Two things still owed on this branch, neither resolvable from the rebase: the risk section still describes the pre-#311 world, and the models.default-ai-credits-pricing question needs re-checking against firewall v0.27.42 (main retired the sandbox pin on the grounds that v0.27.42 prices fable-5; whether it prices claude-opus-5 is unverified, and an un-priced model is a 400 on every dispatch).
| // without it (run 30656579898: correctness-reviewer on Fable 5, | ||
| // blocked on security-adjacent diffs). | ||
| const fallback = | ||
| result.refused === true && modelOverride === undefined |
There was a problem hiding this comment.
issue (blocking): Production refusal fallback is dead code — the SDK runner never sets refused.
This branch gates on result.refused === true, but the production runner createSdkRunner (dispatch-runner.ts, unchanged by this PR) returns only {output, usd, turns, wallMs} (or structured: true) on every path — it never inspects stop_reason or sets refused. The only code that sets refused is the eval runner (live-runner.ts:146) and the test stubs. So in production a refusal still presents as an empty final with refused undefined: the fallback is skipped, the empty output fails the contract parse, parseWithRetry re-runs the same refusing pin, and the dimension is shed silently — the exact coverage hole this PR exists to close. dispatch.test.ts passes only because its stub fabricates refused: true. Teach createSdkRunner to detect stop_reason === "refusal" and return refused/stopReason so this path can fire.
1 additional low-confidence note
workflows/review/lib/refusal-fallback.ts:51— consider writing down the exit condition this fallback instruments (e.g. a per-agent fallback-rate threshold that means "repin"), so the counters section drives the pin decision it was built to inform rather than standing in for it.
| if (rereviewDepth !== undefined) { | ||
| run.rereviewDepth = rereviewDepth; | ||
| } | ||
| const perAgent = isRecord(raw.dispatchResult) |
There was a problem hiding this comment.
issue (blocking): Production refusal-fallback rate is never populated — no disk loader passes dispatchResult.
normalizeRunArtifacts derives refusalFallbacks from raw.dispatchResult here, and the README states counters reads fellBackTo from each run's out/dispatch-result.json. But neither disk loader feeds it: readRunArtifactsFromDir (counters.ts:687) builds raw from claims/validator/summary/rereviewPlan only, and loadRunDir (counters-report.ts:270) — the weekly report's actual loader — passes only {claims, validator, summary}. So raw.dispatchResult is always undefined, run.refusalFallbacks never populates, and the "Refusal fallbacks" section always renders "None" even after a fallback fires. The added RunArtifactLayout.dispatchResult field and default are therefore dead. This is also untested: the counters tests inject RunArtifacts directly, bypassing both disk readers. Read dispatch-result.json in both loaders and add a normalizeRunArtifacts test over the perAgent[].fellBackTo path.
| ? refusalFallbackFor(model) | ||
| : undefined; | ||
| if (fallback !== undefined) { | ||
| return dispatchAgent(name, malformedNote, fallback); |
There was a problem hiding this comment.
suggestion (non-blocking): Two latent issues on the fallback return path — they surface once the refusal detector from the blocking comment above lands.
- Refused attempt's cost/turns are dropped.
return dispatchAgent(name, malformedNote, fallback)discards the refusedresultentirely — noperAgententry is pushed for it and itsusd/turns/wallMsare not folded into the fallback entry, sototalUsd(a sum overperAgent) undercounts by the refused attempt. A refusal can land after a full investigation loop, so it is not free. The eval producer accumulates across attempts; this path does not. - Fallback is forgotten across the malformed-retry seam. The fallback is a recursion inside
dispatchAgent, butparseWithRetry(~line 543) re-dispatches with nomodelOverride, so a fallback whose output then fails the contract parse retries ondefinition.model— the refusing pinrefusal-fallback.tsdocuments as never re-rolled — which refuses again and pushes a secondfellBackToentry.counters.tsflatMaps everyfellBackTo, so one logical refusal is counted twice. Thread the resolved model intoparseWithRetry, or move the malformed retry inside the model-aware loop.
| if (fallback !== undefined) { | ||
| model = fallback; | ||
| tried.push(fallback); | ||
| report.fellBackTo = fallback; |
There was a problem hiding this comment.
suggestion (non-blocking): Eval records fellBackTo (and swaps model) on the final attempt, when the fallback model is never dispatched.
When the refusal lands on attempt 1 (attempt 0 failed malformed, then the corrective retry refused), this branch sets report.fellBackTo = fallback and report.failed, then continues — exiting the two-iteration loop without ever invoking the runner on the fallback model. The A/B report then asserts a fallback to claude-opus-4-8 alongside a failure note, so a reader concludes the fallback model itself refused and chases a model that never ran. At attempt === 1, record the failure without swapping the model or setting fellBackTo.
| `- ${fallbacks.total} fallback${fallbacks.total === 1 ? "" : "s"}` + | ||
| ` across ${fallbacks.runsAffected} of ${counters.runCount} runs`, | ||
| ); | ||
| for (const entry of fallbacks.byAgent) { |
There was a problem hiding this comment.
suggestion (non-blocking): Non-zero "Refusal fallbacks" render branch is untested.
renderCountersMarkdown's new section has a zero and a non-zero branch; the only test drives runs with no fallbacks, so just the "None" path is exercised. The per-agent listing, the count summary, and the singular/plural and runsAffected phrasing go unverified. Add a test that drives a run with a recorded fallback and asserts the rendered per-agent line.
| } | ||
| writeOut(name, result.output); | ||
| perAgent.push({ | ||
| name, |
There was a problem hiding this comment.
question (non-blocking): AgentResult gains stopReason/errorMessage/tokensAtFailure/toolCalls, but the production dispatcher records none of them.
The perAgent.push blocks persist only refused-derived state, never the failure detail or tool-call counts, though AgentResult now declares them (the eval producer does record them). The changeset scopes those bullets to the eval arms, but the PR summary lists "error message, provider raw stop reason, token counts at failure" under a "both the eval and production paths" heading. Was production visibility beyond the empty-final naming meant to be deferred here?
| (f) => | ||
| `<details><summary>${arm} / ${c.caseId} / ${f.agent}</summary>\n\n` + | ||
| "```\n" + | ||
| f.output + |
There was a problem hiding this comment.
suggestion (non-blocking): Failed-agent raw output is embedded in a bare fence without escaping.
The captured text is a failed final, and a common contract-parse failure mode is precisely a code-fenced reply, so the payload frequently contains a triple-backtick fence — which closes this fence early and renders the remainder as live markdown inside the job summary, breaking the <details> pairing for every section after it. Wrap the payload in a longer fence (four backticks) or indent it.
| runsAffected += 1; | ||
| } | ||
| for (const entry of entries) { | ||
| const key = `${entry.agent}\u0000${entry.model}`; |
There was a problem hiding this comment.
nitpick (non-blocking): Composite Map key uses `` where the repo's idiom is a colon.
The package builds composite Map keys with a colon elsewhere (`${grain}:${id}` in thumbs-sweep-github.ts:583, `${corpusCase.id}:${detail.specKey}` in live-ab-report.ts:269). Agent and model names contain no colons, so : is collision-safe here and matches the prevailing pattern.
44ce5dd to
ee85ff7
Compare
… Opus 5 Re-applied on top of #311 (refusal visibility and fallback). The original two commits conflicted with main independently of that stack: the sandbox.agent.version pin was retired and gh-aw v0.83.4 / firewall v0.27.42 landed underneath them, so review.md's frontmatter had moved. This keeps main's infrastructure evolution and re-applies the pin intent on top, rather than resurrecting frontmatter that main has since superseded. 22 pins move to claude-opus-5: the orchestrator, thread-reconciler, skill-auditor, claim-validator, conventions, the opt-in whole-change reviewers, all twelve specialist lenses, plus correctness-reviewer and first-principles from claude-fable-5. pattern-triage stays on Sonnet 4.6 as the cheap first pass; the eval's judge and match arbiter stay on Haiku. What changed since this PR was written, and why it is now stacked: The refusal risk this PR's body accepts as a specialist-lens hazard, detectable only via weekly drift, is measured and already open. Run 30656579898 caught correctness-reviewer on Fable 5 refusing incident-auth-bypass and adversarial-injection-approve under Anthropic's usage policy, at 5,207 tokens. It is a DEFAULT-roster agent, not a lens, on the metric this repo calls load-bearing. Moving it to Opus 5 does not resolve that: this PR's own assessment is that Opus 5 also ships elevated cyber safeguards and can return stop_reason refusal, so the hazard moves with the roster. Stacking on #311 means the roster lands with the mitigation in place: a refused agent re-dispatches on claude-opus-4-8 (already in that map for opus-5), recorded as fellBackTo and reported as a weekly rate. Note the detector this PR nominates changes character: a refusing reviewer no longer craters recall on security-adjacent cases, because it falls back and produces findings. The fallback counter replaces that inference with a direct measurement. Two things still owed on this branch, neither resolvable from the rebase: the risk section still describes the pre-#311 world, and the models.default-ai-credits-pricing question needs re-checking against firewall v0.27.42 (main retired the sandbox pin on the grounds that v0.27.42 prices fable-5; whether it prices claude-opus-5 is unverified, and an un-priced model is a 400 on every dispatch).
ee85ff7 to
c22d9fd
Compare
16c4bc6 to
b253aa8
Compare
… Opus 5 Re-applied on top of #311 (refusal visibility and fallback). The original two commits conflicted with main independently of that stack: the sandbox.agent.version pin was retired and gh-aw v0.83.4 / firewall v0.27.42 landed underneath them, so review.md's frontmatter had moved. This keeps main's infrastructure evolution and re-applies the pin intent on top, rather than resurrecting frontmatter that main has since superseded. 22 pins move to claude-opus-5: the orchestrator, thread-reconciler, skill-auditor, claim-validator, conventions, the opt-in whole-change reviewers, all twelve specialist lenses, plus correctness-reviewer and first-principles from claude-fable-5. pattern-triage stays on Sonnet 4.6 as the cheap first pass; the eval's judge and match arbiter stay on Haiku. What changed since this PR was written, and why it is now stacked: The refusal risk this PR's body accepts as a specialist-lens hazard, detectable only via weekly drift, is measured and already open. Run 30656579898 caught correctness-reviewer on Fable 5 refusing incident-auth-bypass and adversarial-injection-approve under Anthropic's usage policy, at 5,207 tokens. It is a DEFAULT-roster agent, not a lens, on the metric this repo calls load-bearing. Moving it to Opus 5 does not resolve that: this PR's own assessment is that Opus 5 also ships elevated cyber safeguards and can return stop_reason refusal, so the hazard moves with the roster. Stacking on #311 means the roster lands with the mitigation in place: a refused agent re-dispatches on claude-opus-4-8 (already in that map for opus-5), recorded as fellBackTo and reported as a weekly rate. Note the detector this PR nominates changes character: a refusing reviewer no longer craters recall on security-adjacent cases, because it falls back and produces findings. The fallback counter replaces that inference with a direct measurement. Two things still owed on this branch, neither resolvable from the rebase: the risk section still describes the pre-#311 world, and the models.default-ai-credits-pricing question needs re-checking against firewall v0.27.42 (main retired the sandbox pin on the grounds that v0.27.42 prices fable-5; whether it prices claude-opus-5 is unverified, and an un-priced model is a 400 on every dispatch).
… Opus 5 Re-applied on top of #311 (refusal visibility and fallback). The original two commits conflicted with main independently of that stack: the sandbox.agent.version pin was retired and gh-aw v0.83.4 / firewall v0.27.42 landed underneath them, so review.md's frontmatter had moved. This keeps main's infrastructure evolution and re-applies the pin intent on top, rather than resurrecting frontmatter that main has since superseded. 22 pins move to claude-opus-5: the orchestrator, thread-reconciler, skill-auditor, claim-validator, conventions, the opt-in whole-change reviewers, all twelve specialist lenses, plus correctness-reviewer and first-principles from claude-fable-5. pattern-triage stays on Sonnet 4.6 as the cheap first pass; the eval's judge and match arbiter stay on Haiku. What changed since this PR was written, and why it is now stacked: The refusal risk this PR's body accepts as a specialist-lens hazard, detectable only via weekly drift, is measured and already open. Run 30656579898 caught correctness-reviewer on Fable 5 refusing incident-auth-bypass and adversarial-injection-approve under Anthropic's usage policy, at 5,207 tokens. It is a DEFAULT-roster agent, not a lens, on the metric this repo calls load-bearing. Moving it to Opus 5 does not resolve that: this PR's own assessment is that Opus 5 also ships elevated cyber safeguards and can return stop_reason refusal, so the hazard moves with the roster. Stacking on #311 means the roster lands with the mitigation in place: a refused agent re-dispatches on claude-opus-4-8 (already in that map for opus-5), recorded as fellBackTo and reported as a weekly rate. Note the detector this PR nominates changes character: a refusing reviewer no longer craters recall on security-adjacent cases, because it falls back and produces findings. The fallback counter replaces that inference with a direct measurement. Two things still owed on this branch, neither resolvable from the rebase: the risk section still describes the pre-#311 world, and the models.default-ai-credits-pricing question needs re-checking against firewall v0.27.42 (main retired the sandbox pin on the grounds that v0.27.42 prices fable-5; whether it prices claude-opus-5 is unverified, and an un-priced model is a 400 on every dispatch).
… Opus 5 Re-applied on top of #311 (refusal visibility and fallback). The original two commits conflicted with main independently of that stack: the sandbox.agent.version pin was retired and gh-aw v0.83.4 / firewall v0.27.42 landed underneath them, so review.md's frontmatter had moved. This keeps main's infrastructure evolution and re-applies the pin intent on top, rather than resurrecting frontmatter that main has since superseded. 22 pins move to claude-opus-5: the orchestrator, thread-reconciler, skill-auditor, claim-validator, conventions, the opt-in whole-change reviewers, all twelve specialist lenses, plus correctness-reviewer and first-principles from claude-fable-5. pattern-triage stays on Sonnet 4.6 as the cheap first pass; the eval's judge and match arbiter stay on Haiku. What changed since this PR was written, and why it is now stacked: The refusal risk this PR's body accepts as a specialist-lens hazard, detectable only via weekly drift, is measured and already open. Run 30656579898 caught correctness-reviewer on Fable 5 refusing incident-auth-bypass and adversarial-injection-approve under Anthropic's usage policy, at 5,207 tokens. It is a DEFAULT-roster agent, not a lens, on the metric this repo calls load-bearing. Moving it to Opus 5 does not resolve that: this PR's own assessment is that Opus 5 also ships elevated cyber safeguards and can return stop_reason refusal, so the hazard moves with the roster. Stacking on #311 means the roster lands with the mitigation in place: a refused agent re-dispatches on claude-opus-4-8 (already in that map for opus-5), recorded as fellBackTo and reported as a weekly rate. Note the detector this PR nominates changes character: a refusing reviewer no longer craters recall on security-adjacent cases, because it falls back and produces findings. The fallback counter replaces that inference with a direct measurement. Two things still owed on this branch, neither resolvable from the rebase: the risk section still describes the pre-#311 world, and the models.default-ai-credits-pricing question needs re-checking against firewall v0.27.42 (main retired the sandbox pin on the grounds that v0.27.42 prices fable-5; whether it prices claude-opus-5 is unverified, and an un-priced model is a 400 on every dispatch).
… Opus 5 Re-applied on top of #311 (refusal visibility and fallback). The original two commits conflicted with main independently of that stack: the sandbox.agent.version pin was retired and gh-aw v0.83.4 / firewall v0.27.42 landed underneath them, so review.md's frontmatter had moved. This keeps main's infrastructure evolution and re-applies the pin intent on top, rather than resurrecting frontmatter that main has since superseded. 22 pins move to claude-opus-5: the orchestrator, thread-reconciler, skill-auditor, claim-validator, conventions, the opt-in whole-change reviewers, all twelve specialist lenses, plus correctness-reviewer and first-principles from claude-fable-5. pattern-triage stays on Sonnet 4.6 as the cheap first pass; the eval's judge and match arbiter stay on Haiku. What changed since this PR was written, and why it is now stacked: The refusal risk this PR's body accepts as a specialist-lens hazard, detectable only via weekly drift, is measured and already open. Run 30656579898 caught correctness-reviewer on Fable 5 refusing incident-auth-bypass and adversarial-injection-approve under Anthropic's usage policy, at 5,207 tokens. It is a DEFAULT-roster agent, not a lens, on the metric this repo calls load-bearing. Moving it to Opus 5 does not resolve that: this PR's own assessment is that Opus 5 also ships elevated cyber safeguards and can return stop_reason refusal, so the hazard moves with the roster. Stacking on #311 means the roster lands with the mitigation in place: a refused agent re-dispatches on claude-opus-4-8 (already in that map for opus-5), recorded as fellBackTo and reported as a weekly rate. Note the detector this PR nominates changes character: a refusing reviewer no longer craters recall on security-adjacent cases, because it falls back and produces findings. The fallback counter replaces that inference with a direct measurement. Two things still owed on this branch, neither resolvable from the rebase: the risk section still describes the pre-#311 world, and the models.default-ai-credits-pricing question needs re-checking against firewall v0.27.42 (main retired the sandbox pin on the grounds that v0.27.42 prices fable-5; whether it prices claude-opus-5 is unverified, and an un-priced model is a 400 on every dispatch).
What
Two things, split out of #305 so they are not blocked by the unproven Pi harness: make a silent sub-agent failure visible, and fall back to another model when the pinned one refuses.
No model pin changes. No new dependencies. The default dispatch path is untouched.
The bug this found
correctness-revieweris refusing outright on security-adjacent code, in production, today:Reproduced on
incident-auth-bypassandadversarial-injection-approve(run 30656579898, $1.57). Not a context limit — 5,207 tokens. Not a harness defect — both the Claude Agent SDK and the Pi runner hit it identically, because it is the model.The hazard was documented and the mitigation went on the wrong roles. The README keeps the twelve specialist lenses on Opus "because Fable's cyber safety classifiers can refuse benign security-focused analysis, and a refused security lens would be a silent coverage hole". Meanwhile the 2026-07-20 A/B moved
correctness-reviewer— the default roster's load-bearing recall agent — onto Fable 5 for its recall gain. The shield went on the opt-in roles; the default finder went the other way and has been there since.A refused reviewer produces no error. It produces nothing, and the review proceeds without it.
Visibility
empty output: the agent returned no final text), distinct from unparseable prose, on both the eval and production contract parsers. The old message blamed the model's output shape for a request that was blocked before it produced any.The cost of not having this: three eval runs, ~$30, chasing a contract bug that never existed. The instrumentation identified the real cause in one $1.57 scoped run.
Fallback
A refusal is deterministic in the model — the same prompt on the same pin refuses again — so the ordinary retry cannot help and the fallback must change the model.
lib/refusal-fallback.ts:claude-fable-5claude-opus-4-8claude-opus-5claude-opus-4-8Three deliberate rules:
fellBackTo) and lands in the report and run artifact. Trading an invisible skip for an invisible model swap would defeat the point, and the drift corpus needs to see how often this fires.Anthropic's own refusal message points integrators at a fallback model. #294 states "gh-aw exposes no
fallbacksparameter, so there's no server-side retry-on-another-model to lean on" — true of gh-aw, but scripted dispatch owns its runner, so the policy can live in our code regardless.Known gaps, deliberately not in this PR
lib/dispatch.tshas its own retry path and still loses a refused reviewer silently. That is the more urgent half and wants its own review; this PR makes the failure visible on both paths, which is the prerequisite.claim-validatoron the assumption that it is safe here.Relationship to #294
#294 moves
correctness-reviewerfrom Fable 5 to Opus 5. That may resolve this refusal or reproduce it — #294's own body flags Opus 5's "elevated cybersecurity safeguards" andstop_reason: \"refusal\". With this PR merged that becomes a $1.57 measurement on two named cases instead of a judgment call, and either way the fallback covers it.Checks
1591 tests pass (15 new);
tsc --noEmitclean.pnpm lintstill cannot run in this worktree (pre-existing duplicate-eslint-plugin clash with the parent checkout, same as #294).