Skip to content

fix(loop): max-mode hands the drive the failure that killed every candidate (#425) - #429

Merged
mabry1985 merged 6 commits into
mainfrom
fix/max-mode-provider-425
Sep 10, 2026
Merged

mabry1985 merged 6 commits into
mainfrom
fix/max-mode-provider-425

Conversation

@mabry1985

@mabry1985 mabry1985 commented Sep 10, 2026

Copy link
Copy Markdown
Member

Fixes #425.

What was wrong

_dispatch_max_mode swallows each candidate's exception. When every candidate died, the drive only ever saw NoChangesError("max-mode: all N candidates produced no diff"). That is a capability verdict, so the card climbed a rung, no matter what had actually killed the candidates.

Reproduced on origin/main through _drive, with rung [codex, sonnet], max_mode_n: 2 and codex refusing its model:

dispatched: codex, codex, opus, opus   escalated ×2   blocked terminal   codex marked: False

That's the trace from the #421 review. The same swallow:

The fix

When every candidate raised, no candidate returned anything for the selector to judge. So max-mode re-raises the one error that speaks for them, and from there the drive's own handling applies exactly as for a single dispatch. representative_failure() in loop/_common.py picks it, most specific edge first:

rank failure the drive then…
0 a refused model (also beside a quota: the provider is at least partly broken) marks it, rotates within the rung, or blocks dispatch-infra (#420)
1 a spent quota rotates, or backs off (#362)
2 a CoderTimeout counts it for #378, and climbs with #146's note (fanning out again), or blocks pre-first-token as infra
3 a dispatch failure the drive does not retry #339 blocks it as pre-model unless a candidate reached the model
4 any other dispatch failure (retryable) backs off and re-runs the fan-out
5 any other WorktreeError as raised

Ties go to the earliest candidate. Only a WorktreeError can speak. A raw error (say a BrokenPipeError from an untapped host) keeps the old no-diff verdict, which the drive's shutdown and cancel checks see first. Only a fan-out where at least one candidate RETURNED (ran and came back with nothing) is guaranteed to be a capability climb. A candidate that left a diff still wins exactly as before. provider_failure_category() is the one shared definition of a provider failure, used by the drive's rotation and by the ranking.

Found in review, also fixed

  • One all-timeout fan-out switched max-mode off for the card for good. The timeout climb put its "prior attempt timed out" note (Retry paths don't propagate failure context — the search repeats the same failure N times, and a timeout escalation upgrades the model while telling it nothing #146) in _ci_feedback. That slot marks a carried-forward fix, which disables fan-out, and it outlives the drive. So the climbed rung ran ONE dispatch, and every later drive of the card also ran single, opening with "a PREVIOUS attempt TIMED OUT". The note is now drive-local: the climb is a fresh build and the new rung fans out again (codex×2 → opus×2). It's spent once the dispatch that carried it has run, and dropped on any other climb.
  • The note described the wrong candidate. It was mined from the last gen, which in a fan-out may be a sibling that failed fast ("ran ~0.0s", no tool). The tap now stamps the stop reason of the gen its watchdog killed, on every dispatch path, and the note is mined from that gen.
  • A raw error during shutdown blocked the card as unexpected. It's no longer re-raised (see above).

Merge order with #435. #429 on its own blocks a card whose fan-outs keep timing out as transient, and the sweep then rebuilds it twice more. #435 closes that by parking it for a split. The drive-level test of the merged behaviour lives here (test_a_fan_out_that_keeps_timing_out_is_parked_for_its_split). It skips, naming #435, until the park is on the branch, and passes on the three-way merge.

Tests (tests/test_max_mode_provider_425.py)

Covers each precedence boundary, the fallback, and the review items:

  • refused on every candidate → same-rung switch, codex marked. One-provider board → dispatch-infra with the evidence. Refusal + quota → the refusal edge.
  • quota on every candidate → switch with no backoff, not marked. Quota + timeout → the quota edge; the timeout is not counted.
  • timeout + seam failureA large card can still time out forever: the design gate is not a breadth cap, and a repeated timeout should decompose rather than park #378's counter moves and the climb carries "TIMED OUT".
  • seam failure + network blip → blocked as pre-model, one fan-out, no backoff. Network blip on every candidate → re-run after backoff max_attempts times, then transient, never climbs.
  • fallback: one candidate refused and one returned empty climbs. Tested both with a stand-in tap and through the real dispatch_coder_tapped.
  • review:
    • a climb after an all-timeout fan-out fans out again, asserting the full sequence codex×2 → opus×2;
    • a later drive of the card fans out again with no stale note;
    • the note is spent once the climbed dispatch has run;
    • through the real tap, the note describes the candidate that timed out;
    • every candidate is reaped before the drive handles the failure;
    • a CancelledError child is neither returned nor raised;
    • a raw error on every candidate keeps the shutdown edge.
  • unit: the full precedence (every rank beats every later one; ties go to the first; a raw error never speaks), and provider_failure_category matches the drive's definition.

One existing test changes: test_drive_carries_timeout_context_into_the_escalated_prompt pinned the note inside _ci_feedback, which was the bug. It now asserts the note did not land there.

Red-checks: against origin/main, the precedence tests fail with the reproduction above. Against the previous head (6d7a24f), the six review tests that change behaviour fail; the three guards pass there by design. All 10 of the reviewer's reproducers pass on this head.

Mutation checks (8 review mutations plus the earlier precedence ones), each caught:

  • the note back in _ci_feedback;
  • the last gen again;
  • no stamp on the tap;
  • raw errors speaking;
  • reaping only on the no-diff path;
  • a cancelled child counted as raised;
  • any raised candidate speaking;
  • a note that is never spent;
  • each adjacent pair of ranks swapped.

Gates: ruff format --check, ruff check, full suite 1922 passed / 14 skipped (the +1 skip is the merge-only test).

Note: this conflicts textually with #434 in _dispatch_max_mode. Keep #434's discard-by-path and this PR's re-raise.

🤖 Generated with Claude Code

https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av

mabry1985 and others added 2 commits September 10, 2026 14:54
Max-mode builds a card N ways at once and swallows each candidate's
error. With every candidate dead the drive saw only NoChangesError("max-
mode: all N candidates produced no diff"), a capability verdict, and
climbed a rung. When the candidates died because their PROVIDER did (a
spent quota, #362, or a model it can't serve, #420), that is the wrong
edge: neither says anything about the model, and the single-dispatch and
coder.solve paths already rotate within the rung on both. The review of
#421 reproduced it: rung [codex, ...] with max_mode_n: 2 and codex
refusing its model dispatched codex, codex, escalated, then opus, opus,
and escalated again. No mark, no rotation.

When every candidate failed on its provider, and in the same way, max-
mode now re-raises one of those dispatch errors, so the drive rotates,
marks, backs off and blocks exactly as it does for a single dispatch. One
error can only stand for all of them when they agree. A candidate that
came back with nothing reached the model and failed it. A mix of provider
classes has no single truthful edge (a mark versus a backoff). Both stay
the capability failure they always were.

"Provider failure" now has one definition, provider_failure_category(),
which the drive's rotation and max-mode both ask.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av

@protoreview protoreview Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

QA panel review — PASS

code-review-structural · head 80b96469439f · formal

⚠️ PR advanced 1 commit(s) during this round (80b96469439f21e1b85f1896); 0 finding(s) in the delta were demoted to possibly addressed.

Overall risk is HIGH: 4 of 5 finders failed (timeouts, truncation, gateway auth), so the production-code change — extracting provider_failure_category into _common.py and wiring it into _drive and _dispatch_max_mode — went effectively unreviewed by the panel. The single surviving finder produced no findings, and the verifier had an empty array to verify. Fix-first: re-run the failed finders (and the skipped structural pass) before merging; do not treat this round as a clean pass. The panel did not disagree on anything because it produced nothing. Coverage gap: the heavily-changed production files (_common.py, the _drive call site, the _dispatch_max_mode re-raise path) received zero substantive review; the test file and changelog likewise went unexamined.

No findings — the review came back clean.

findings JSON (machine-readable)
[]

2 panel step(s) hit their time budget and were skipped this round: find_correctness, find_removed_behavior. The verdict stands on the remaining angles; a finding only that step would have caught could be missed — the next push re-runs the full panel.

mabry1985 and others added 2 commits September 10, 2026 15:25
…didate (#425)

The first cut re-raised only when every candidate failed on its provider
the same way. But the swallow hid more than provider failures. A timeout
on every candidate never reached #378's timeout counter or its decompose
ask, and a pre-model seam failure never reached #339's block. Both became
a "no diff" capability verdict and climbed a rung, and so did a mix of a
refusal and a quota.

Now, whenever EVERY candidate raised, nothing returned for the selector to
judge, so max-mode re-raises the one error that speaks for them and the
drive's own handling applies as for a single dispatch. The choice is
representative_failure(), most specific edge first:
- a refused model (beside a quota too: the provider is at least partly
  broken, and a climb is wrong either way)
- a spent quota
- a timeout (so #378 sees it)
- a dispatch failure the drive does not retry (#339 blocks it as pre-model
  unless a candidate reached the model)
- any other dispatch failure (retryable: back off and re-run)
- anything else as raised

Ties go to the earliest candidate. Only when a candidate RETURNED, ran and
came back with nothing, is it the capability failure it always was.
provider_failure_category() stays the shared predicate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av
@mabry1985 mabry1985 changed the title fix(loop): max-mode hands the drive a shared provider failure (#425) fix(loop): max-mode hands the drive the failure that killed every candidate (#425) Sep 10, 2026
@protoreview

protoreview Bot commented Sep 10, 2026

Copy link
Copy Markdown

⚠️ QA panel exhausted — this PR has not been reviewed.
The review panel failed after 2 attempt(s) on head 6d7a24f48a96. No verdict was posted.
A new push will re-trigger the review.

mabry1985 and others added 2 commits September 10, 2026 16:24
…r a fan-out (#425)

Review of the generalised fan-out edge found five issues, each reproduced.

1. One all-timeout fan-out switched max-mode off for the card for good.
   The climb put its "prior attempt timed out" note (#146) in
   _ci_feedback. That slot marks a carried-forward FIX, which disables
   fan-out, and it outlives the drive. So the climbed rung ran ONE
   dispatch, and every later drive of the card also ran single, opening
   with "a PREVIOUS attempt TIMED OUT". The note is now drive-local: a
   timeout climb is a fresh build, so the new rung fans out again
   (codex x2 -> opus x2). The note is spent once the dispatch that
   carried it has run, and dropped on any other climb.
2. The note was mined from the LAST gen. In a fan-out, that is just the
   last candidate, which may have failed fast on something else ("ran
   ~0.0s", no tool). The tap now stamps the stop reason of a gen its
   watchdog killed, on every dispatch path, and the note is mined from
   that gen.
3. A raw (non-WorktreeError) error re-raised from the fan-out skipped the
   drive's shutdown and cancel checks. A BrokenPipeError mid-shutdown
   blocked the card as `unexpected`. Only a WorktreeError can now speak
   for a fan-out; anything else keeps the old no-diff verdict, which
   those checks see first.
4. Untested behaviour, now pinned: every candidate is reaped before the
   drive handles the representative; a CancelledError child is neither
   returned nor raised; and the "a candidate RETURNED -> capability
   climb" fallback is exercised through the real dispatch_coder_tapped,
   not a stand-in.
5. The docs now state the actual rule and the full precedence. A
   drive-level test of the merged #429 x #435 behaviour (a fan-out that
   keeps timing out is parked) lives here; it skips until #435's park is
   on the branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av
@mabry1985
mabry1985 merged commit 7a720e6 into main Sep 10, 2026
4 checks passed
@mabry1985
mabry1985 deleted the fix/max-mode-provider-425 branch September 10, 2026 23:29
@protoreview

protoreview Bot commented Sep 11, 2026

Copy link
Copy Markdown

⚠️ QA panel exhausted — this PR has not been reviewed.
The review panel failed after 2 attempt(s) on head ea19792cd52c. No verdict was posted.
A new push will re-trigger the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: max-mode swallows each candidate's dispatch error, so a quota or dead provider climbs the ladder instead of rotating

1 participant