Repository navigation
fix(runner): stop silently retrying model-stage failures that cannot succeed - #6824
Merged
Merged
Conversation
…succeed #6284 WS1's remaining mapping box. Went in expecting a naming tidy-up; it is a live retry-burn. `model_stage_failure_category` returned `None` for `InvalidInvocation`, `Invalid`, `ScopeMismatch` and `PolicyDenied`, so all four fell through to `host_stage_unavailable_model` — which `is_auto_retriable_category` lists among "transient host / lease / store / provider / tool faults where re-running the *identical* request from the checkpoint is likely to succeed". None of the four can succeed on an identical retry. Policy does not change between attempts, a malformed request stays malformed, and a scope mismatch is configuration-shaped. So the run silently re-drove a call that could never work, and reported "host stage unavailable" to the operator instead of the real cause. That same list's doc comment says "Conservative by design — anything not clearly transient falls to `UserInitiated`"; the fallthrough defeated exactly that intent. `executor/mapping.rs` documented this as already handled — "the runner preserves the original kind when categorizing the failure." It did not. Comment corrected in place rather than left to mislead the next reader. Three categories, none auto-retriable (an unlisted category falls to `UserInitiated`, which is the wanted behavior): - `model_stage_request_invalid` (InvalidInvocation | Invalid) - `model_stage_policy_denied` (PolicyDenied) - `model_stage_scope_mismatch` (ScopeMismatch) Two follow-on sites the compiler and the suite caught, both worth naming: - `failure_lane`'s canonical category list needed the three additions; its doc says "keep this in lockstep" and nothing enforces that but the test. - `every_failure_category_is_explainable_and_classified` rejected them for having no user-facing explanation. The generic fallback they would have used reads "Retry the run, and contact support if it keeps happening" — advice that can never work for a refused or malformed request. Each new category now states plainly that retrying will not help and names what has to change. That is the same defect this epic fixes for the model, one layer up: the *user* was also being told to retry something that could not succeed. Two tests inverted, each with the reason recorded beside it: - `model_stage_host_error_kind_category_matrix_is_exhaustive` pinned all four kinds at `None` — the epic anticipated this ("a runner test pins the wrong behavior as expected — fix the mapping and invert the test"). - `text_only_model_reply_driver_sanitizes_model_failures_...` pinned the generic `model_error` reason kind for a model-stage `PolicyDenied`. New regression test `permanent_model_stage_failures_are_not_categorized_ as_transient_outages` asserts each kind gets a category AND that the category is not auto-retriable — the second half is the part that actually matters, and it fails against the old code. Gate: runner, agent_loop, turns — 1,826 tests pass, clippy clean, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6824
July 28, 2026 19:44
Destroyed
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe runner now classifies permanent model-stage invalid, policy-denied, and scope-mismatch failures with dedicated non-retriable categories and summaries. Registries, mapping tests, product summary tests, host-loop expectations, and an explanatory comment were updated. ChangesModel failure classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size: M
Closed
59 tasks done
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_runner/src/model_failure_mapping.rs`:
- Around line 65-107: Extend the driver-level integration test that currently
covers PolicyDenied to exercise InvalidInvocation, Invalid, and ScopeMismatch
through the production caller path rather than calling
model_stage_failure_category directly. For each case, assert the emitted driver
failure category matches its specific non-transient category and verify
is_auto_retriable_category returns false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71e239f9-d455-4f6f-a389-1cbbdd145dbd
📒 Files selected for processing (7)
crates/ironclaw_agent_loop/src/executor/mapping.rscrates/ironclaw_runner/src/failure_categories.rscrates/ironclaw_runner/src/failure_lane.rscrates/ironclaw_runner/src/failure_summary.rscrates/ironclaw_runner/src/model_failure_mapping.rscrates/ironclaw_runner/src/retry_disposition.rscrates/ironclaw_runner/tests/loop_driver_host.rs
Comment on lines
+65
to
+107
| fn permanent_model_stage_failures_are_not_categorized_as_transient_outages() { | ||
| use crate::retry_disposition::is_auto_retriable_category; | ||
| use AgentLoopHostErrorKind as K; | ||
|
|
||
| for kind in [ | ||
| K::InvalidInvocation, | ||
| K::Invalid, | ||
| K::ScopeMismatch, | ||
| K::PolicyDenied, | ||
| ] { | ||
| let category = model_stage_failure_category(true, kind, None).unwrap_or_else(|| { | ||
| panic!( | ||
| "{kind:?} has no model-stage category, so it falls through to the generic \ | ||
| host-stage outage and is silently auto-retried" | ||
| ) | ||
| }); | ||
| assert!( | ||
| !is_auto_retriable_category(category), | ||
| "{kind:?} -> {category:?} is auto-retriable, but an identical retry cannot succeed" | ||
| ); | ||
| assert_ne!( | ||
| category, | ||
| crate::failure_categories::HOST_STAGE_UNAVAILABLE_MODEL_CATEGORY, | ||
| "{kind:?} must name its own cause, not a generic host outage" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn model_stage_host_error_kind_category_matrix_is_exhaustive() { | ||
| use AgentLoopHostErrorKind as K; | ||
|
|
||
| let expected_without_reason = |kind| match kind { | ||
| K::CredentialUnavailable => Some(MODEL_CREDENTIALS_UNAVAILABLE_CATEGORY), | ||
| K::BudgetAccountingFailed => Some(BUDGET_ACCOUNTING_FAILED_CATEGORY), | ||
| // Permanent for an identical retry — each names its own cause | ||
| // instead of falling through to the auto-retriable generic | ||
| // host-stage outage. Inverted from `None`, which pinned the bug | ||
| // `permanent_model_stage_failures_are_not_categorized_as_transient_outages` | ||
| // now guards. | ||
| K::InvalidInvocation | K::Invalid => Some(MODEL_STAGE_REQUEST_INVALID_CATEGORY), | ||
| K::PolicyDenied => Some(MODEL_STAGE_POLICY_DENIED_CATEGORY), | ||
| K::ScopeMismatch => Some(MODEL_STAGE_SCOPE_MISMATCH_CATEGORY), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover the remaining categories through the driver path.
This test calls the classifier directly; the integration test covers only PolicyDenied. Add caller-level cases for InvalidInvocation, Invalid, and ScopeMismatch that assert the emitted driver failure category and non-auto-retry behavior.
As per coding guidelines, “New or changed production-wired behavior must have a caller-level test”; as per path instructions, “Test through the caller” when a classifier gates a side effect.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_runner/src/model_failure_mapping.rs` around lines 65 - 107,
Extend the driver-level integration test that currently covers PolicyDenied to
exercise InvalidInvocation, Invalid, and ScopeMismatch through the production
caller path rather than calling model_stage_failure_category directly. For each
case, assert the emitted driver failure category matches its specific
non-transient category and verify is_auto_retriable_category returns false.
Sources: Coding guidelines, Path instructions
Contributor
🔎 Review · PR #6824
Execution result is invalid The structured result could not be verified. Automatic · PR opened · attempt 1 of 3 · failed after 1m 56s Failure details
|
9 of 10 tasks
…ary table CI caught what my local gate missed: I ran the crates I edited (`ironclaw_runner`, `ironclaw_agent_loop`, `ironclaw_turns`) rather than the crates that CONSUME the value I changed. `ironclaw_product` keeps a SECOND table of user-facing failure text, and `failure_summary_covers_reborn_failure_category_constants` asserts it covers exactly the public constants in `ironclaw_runner::failure_ categories`. Adding three constants without adding three entries fails that test — correctly. Added with the same wording used in the runner's table, so the two agree for these categories. Worth naming for whoever touches this next: the category CONSTANTS have a single home (`ironclaw_product` imports them from `ironclaw_runner`), so there is no vocabulary drift here — unlike the recovery-hint allowlist fixed in #6792. What is duplicated is the user-facing TEXT, across `runner::failure_summary` and `product::projection`. That duplication is guarded by this test, which is why it surfaced in minutes instead of silently. Whether the two tables should be one is a real question, but a separate one: they may be deliberately different registers (operator vs end-user), and I have not verified that either way. Gate: ironclaw_product + ironclaw_event_projections — 526 tests pass, including the guard that failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6824
July 28, 2026 20:36
Destroyed
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.66% — 312506 / 364802 lines Per-crate breakdown (60 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
|
🚅 Deployed to the ironclaw-pr-6824 environment in ironclaw-ci-preview
|
Review feedback on #6824: the classifier had a direct unit test, but only `PolicyDenied` was proven through the caller. That distinction matters here rather than being a formality. `map_host_error` reaches the category through an early return that bypasses the kind match below it, so the classifier being correct does not prove the driver emits what the classifier returns -- and the emitted `reason_kind` is what `retry_disposition` keys on. Covers all four permanent kinds -- `InvalidInvocation`, `Invalid`, `ScopeMismatch`, `PolicyDenied` -- asserting each reaches the driver as its own category and that `is_auto_retriable_category` rejects it. Before the fix all four routed through `host_stage_unavailable_model`, which IS auto-retriable, so a permanently-failing call was silently re-driven. Sabotage-verified: removing the `InvalidInvocation | Invalid` arm from the classifier now fails at the driver seam. Refs #6524 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
|
Fixed in 2ecb43d. The distinction matters here rather than being a formality: Now covers all four permanent kinds ( Sabotage-verified: removing the |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6824
July 28, 2026 21:29
Destroyed
serrrfirat
added a commit
that referenced
this pull request
Jul 29, 2026
…g missing models (#6826) * fix(llm): stop reading rate limits as auth failures, and stop retrying missing models #6284 WS5's two live bugs. Both are permanent-vs-transient misclassification, the same shape as #6824. **1. A number containing 401 or 403 was read as an auth failure.** `is_auth_error_message` matched `"401"`/`"403"` with a bare `contains`, so any number holding those digits classified the failure as auth: "rate limited, retry after 4013 ms" -> AuthFailed A rate limit — the most common transient provider error there is — ended the run immediately telling the user to fix their API key. `AuthFailed` is deliberately neither retried nor circuit-broken, so the run had no path back from a condition that would have cleared on its own. Status codes now match on digit boundaries (`contains_status_code`). Genuine `401`/`403` still classify; `4013`, `14030`, `1403`, `24019` and `4010` no longer do. **2. `LlmError::ModelNotAvailable` had zero producers.** Every 404 / model-not-found fell into `RequestFailed`, which `retry::is_retryable` treats as retryable, so a typo'd or decommissioned model id burned the full 12-attempt budget before failing. Its consumers were all dead: `retry::is_retryable`, `circuit_breaker::is_transient`, and the gateway's `=> PolicyDenied` arm. `is_model_not_available_message` now produces it. Deliberately conservative: explicit phrases ("model not found", "does not exist", "unknown model", ...) always match, but a bare `404` counts only when the message also mentions the model — an unrelated 404 (a proxy path, a health endpoint) still falls through to `RequestFailed`. Ordering matters and is pinned by placement: context-length is checked first (so a 413 is never read as auth), then auth (so a 403 naming a model stays a permission problem), then missing-model. Regression coverage, both red-verified against the old code: - `a_number_containing_401_is_not_an_auth_failure` fails with "rate limited, retry after 4013 ms" ... "the run would terminate telling the user to fix their API key". - `a_standalone_401_or_403_is_still_an_auth_failure` guards the fix from over-correcting. - `a_missing_model_is_not_retried_as_a_transient_failure` asserts the mapping AND that the result is not retryable — the second half is the part that fixes the burn. - `an_unrelated_404_still_falls_through` pins the conservative bound. Gate: ironclaw_llm + ironclaw_runner — 1,671 tests pass, clippy clean, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(llm): restore the test attribute my insertion displaced Clippy caught `duplicated attribute` at rig_adapter.rs. My test insertion anchored on the `fn` line rather than its attribute, so the pre-existing `#[test]` on `map_rig_error_unrelated_still_request_failed` ended up stacked above my new doc comment, and that function lost its own. Consequence worth naming: `map_rig_error_unrelated_still_request_failed` STOPPED RUNNING. It compiled, the suite was green, and a test had quietly been switched off. Only `-D warnings` on the full workspace caught it — my local `cargo clippy -p ironclaw_llm` did not, because I scoped it to the crate instead of running CI's actual command. All five tests in that module now run and pass; clippy clean under `cargo clippy --all --tests --examples --all-features -- -D warnings`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(llm): assert the auth boundary through map_rig_error, not the predicate Review feedback on #6826: both boundary tests stopped at `is_auth_error_message`, which is not what the run acts on. `map_rig_error` turns the predicate into the error variant, and `retry::is_retryable` keys off that variant -- so a predicate fix that failed to change the classification would leave the bug exactly where it was. Each case now also drives `map_rig_error` and asserts the contract: - `4013 ms` and friends map to something retryable and NOT `AuthFailed` -- this is the rate limit the fix exists for, and it would have cleared on its own - a standalone `401`/`403` maps to `AuthFailed` and is not retried, because retrying a bad credential cannot help Sabotage-verified: reverting `contains_status_code` to the bare `contains` fails on "rate limited, retry after 4013 ms". Refs #6524 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
22 of 29 tasks
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
…succeed (nearai#6824) * fix(runner): stop silently retrying model-stage failures that cannot succeed nearai#6284 WS1's remaining mapping box. Went in expecting a naming tidy-up; it is a live retry-burn. `model_stage_failure_category` returned `None` for `InvalidInvocation`, `Invalid`, `ScopeMismatch` and `PolicyDenied`, so all four fell through to `host_stage_unavailable_model` — which `is_auto_retriable_category` lists among "transient host / lease / store / provider / tool faults where re-running the *identical* request from the checkpoint is likely to succeed". None of the four can succeed on an identical retry. Policy does not change between attempts, a malformed request stays malformed, and a scope mismatch is configuration-shaped. So the run silently re-drove a call that could never work, and reported "host stage unavailable" to the operator instead of the real cause. That same list's doc comment says "Conservative by design — anything not clearly transient falls to `UserInitiated`"; the fallthrough defeated exactly that intent. `executor/mapping.rs` documented this as already handled — "the runner preserves the original kind when categorizing the failure." It did not. Comment corrected in place rather than left to mislead the next reader. Three categories, none auto-retriable (an unlisted category falls to `UserInitiated`, which is the wanted behavior): - `model_stage_request_invalid` (InvalidInvocation | Invalid) - `model_stage_policy_denied` (PolicyDenied) - `model_stage_scope_mismatch` (ScopeMismatch) Two follow-on sites the compiler and the suite caught, both worth naming: - `failure_lane`'s canonical category list needed the three additions; its doc says "keep this in lockstep" and nothing enforces that but the test. - `every_failure_category_is_explainable_and_classified` rejected them for having no user-facing explanation. The generic fallback they would have used reads "Retry the run, and contact support if it keeps happening" — advice that can never work for a refused or malformed request. Each new category now states plainly that retrying will not help and names what has to change. That is the same defect this epic fixes for the model, one layer up: the *user* was also being told to retry something that could not succeed. Two tests inverted, each with the reason recorded beside it: - `model_stage_host_error_kind_category_matrix_is_exhaustive` pinned all four kinds at `None` — the epic anticipated this ("a runner test pins the wrong behavior as expected — fix the mapping and invert the test"). - `text_only_model_reply_driver_sanitizes_model_failures_...` pinned the generic `model_error` reason kind for a model-stage `PolicyDenied`. New regression test `permanent_model_stage_failures_are_not_categorized_ as_transient_outages` asserts each kind gets a category AND that the category is not auto-retriable — the second half is the part that actually matters, and it fails against the old code. Gate: runner, agent_loop, turns — 1,826 tests pass, clippy clean, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(product): cover the new model-stage categories in the Tier-2 summary table CI caught what my local gate missed: I ran the crates I edited (`ironclaw_runner`, `ironclaw_agent_loop`, `ironclaw_turns`) rather than the crates that CONSUME the value I changed. `ironclaw_product` keeps a SECOND table of user-facing failure text, and `failure_summary_covers_reborn_failure_category_constants` asserts it covers exactly the public constants in `ironclaw_runner::failure_ categories`. Adding three constants without adding three entries fails that test — correctly. Added with the same wording used in the runner's table, so the two agree for these categories. Worth naming for whoever touches this next: the category CONSTANTS have a single home (`ironclaw_product` imports them from `ironclaw_runner`), so there is no vocabulary drift here — unlike the recovery-hint allowlist fixed in nearai#6792. What is duplicated is the user-facing TEXT, across `runner::failure_summary` and `product::projection`. That duplication is guarded by this test, which is why it surfaced in minutes instead of silently. Whether the two tables should be one is a real question, but a separate one: they may be deliberately different registers (operator vs end-user), and I have not verified that either way. Gate: ironclaw_product + ironclaw_event_projections — 526 tests pass, including the guard that failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(runner): cover every permanent model-stage kind through the driver Review feedback on nearai#6824: the classifier had a direct unit test, but only `PolicyDenied` was proven through the caller. That distinction matters here rather than being a formality. `map_host_error` reaches the category through an early return that bypasses the kind match below it, so the classifier being correct does not prove the driver emits what the classifier returns -- and the emitted `reason_kind` is what `retry_disposition` keys on. Covers all four permanent kinds -- `InvalidInvocation`, `Invalid`, `ScopeMismatch`, `PolicyDenied` -- asserting each reaches the driver as its own category and that `is_auto_retriable_category` rejects it. Before the fix all four routed through `host_stage_unavailable_model`, which IS auto-retriable, so a permanently-failing call was silently re-driven. Sabotage-verified: removing the `InvalidInvocation | Invalid` arm from the classifier now fails at the driver seam. Refs nearai#6524 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
…g missing models (nearai#6826) * fix(llm): stop reading rate limits as auth failures, and stop retrying missing models nearai#6284 WS5's two live bugs. Both are permanent-vs-transient misclassification, the same shape as nearai#6824. **1. A number containing 401 or 403 was read as an auth failure.** `is_auth_error_message` matched `"401"`/`"403"` with a bare `contains`, so any number holding those digits classified the failure as auth: "rate limited, retry after 4013 ms" -> AuthFailed A rate limit — the most common transient provider error there is — ended the run immediately telling the user to fix their API key. `AuthFailed` is deliberately neither retried nor circuit-broken, so the run had no path back from a condition that would have cleared on its own. Status codes now match on digit boundaries (`contains_status_code`). Genuine `401`/`403` still classify; `4013`, `14030`, `1403`, `24019` and `4010` no longer do. **2. `LlmError::ModelNotAvailable` had zero producers.** Every 404 / model-not-found fell into `RequestFailed`, which `retry::is_retryable` treats as retryable, so a typo'd or decommissioned model id burned the full 12-attempt budget before failing. Its consumers were all dead: `retry::is_retryable`, `circuit_breaker::is_transient`, and the gateway's `=> PolicyDenied` arm. `is_model_not_available_message` now produces it. Deliberately conservative: explicit phrases ("model not found", "does not exist", "unknown model", ...) always match, but a bare `404` counts only when the message also mentions the model — an unrelated 404 (a proxy path, a health endpoint) still falls through to `RequestFailed`. Ordering matters and is pinned by placement: context-length is checked first (so a 413 is never read as auth), then auth (so a 403 naming a model stays a permission problem), then missing-model. Regression coverage, both red-verified against the old code: - `a_number_containing_401_is_not_an_auth_failure` fails with "rate limited, retry after 4013 ms" ... "the run would terminate telling the user to fix their API key". - `a_standalone_401_or_403_is_still_an_auth_failure` guards the fix from over-correcting. - `a_missing_model_is_not_retried_as_a_transient_failure` asserts the mapping AND that the result is not retryable — the second half is the part that fixes the burn. - `an_unrelated_404_still_falls_through` pins the conservative bound. Gate: ironclaw_llm + ironclaw_runner — 1,671 tests pass, clippy clean, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(llm): restore the test attribute my insertion displaced Clippy caught `duplicated attribute` at rig_adapter.rs. My test insertion anchored on the `fn` line rather than its attribute, so the pre-existing `#[test]` on `map_rig_error_unrelated_still_request_failed` ended up stacked above my new doc comment, and that function lost its own. Consequence worth naming: `map_rig_error_unrelated_still_request_failed` STOPPED RUNNING. It compiled, the suite was green, and a test had quietly been switched off. Only `-D warnings` on the full workspace caught it — my local `cargo clippy -p ironclaw_llm` did not, because I scoped it to the crate instead of running CI's actual command. All five tests in that module now run and pass; clippy clean under `cargo clippy --all --tests --examples --all-features -- -D warnings`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(llm): assert the auth boundary through map_rig_error, not the predicate Review feedback on nearai#6826: both boundary tests stopped at `is_auth_error_message`, which is not what the run acts on. `map_rig_error` turns the predicate into the error variant, and `retry::is_retryable` keys off that variant -- so a predicate fix that failed to change the classification would leave the bug exactly where it was. Each case now also drives `map_rig_error` and asserts the contract: - `4013 ms` and friends map to something retryable and NOT `AuthFailed` -- this is the rate limit the fix exists for, and it would have cleared on its own - a standalone `401`/`403` maps to `AuthFailed` and is not retried, because retrying a bad credential cannot help Sabotage-verified: reverting `contains_status_code` to the bare `contains` fails on "rate limited, retry after 4013 ms". Refs nearai#6524 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closing #6284 WS1's remaining mapping box. I went in expecting a naming tidy-up and found a live retry-burn.
model_stage_failure_categoryreturnedNoneforInvalidInvocation,Invalid,ScopeMismatchandPolicyDenied. All four fell through tohost_stage_unavailable_model, whichis_auto_retriable_categorylists among "transient host / lease / store / provider / tool faults where re-running the identical request from the checkpoint is likely to succeed."None of the four can succeed on an identical retry. Policy does not change between attempts, a malformed request stays malformed, a scope mismatch is configuration-shaped. So the run silently re-drove a call that could never work, and told the operator "host stage unavailable" instead of the real cause.
That list's own doc comment says "Conservative by design — anything not clearly transient falls to
UserInitiated." The fallthrough defeated exactly that intent.executor/mapping.rsdocumented this as handled — "the runner preserves the original kind when categorizing the failure." It did not. Corrected in place rather than left to mislead the next reader.Change Type
Linked Issue
Related #6284 (WS1). Closes its
model_failure_mappingbox.Validation
cargo fmt --all -- --checkcargo clippy --all-targets --all-features— zero warningscargo buildironclaw_runner,ironclaw_agent_loop,ironclaw_turns— 1,826 passed, 0 failedcargo test --features integration— not applicable: no persistence or DB behavior changed. The routing this affects is covered by the runner's own suite plus the two conformance tests below.The fix
Three categories, none auto-retriable — an unlisted category falls to
UserInitiated, which is the behavior we want:InvalidInvocation,Invalidmodel_stage_request_invalidPolicyDeniedmodel_stage_policy_deniedScopeMismatchmodel_stage_scope_mismatchTwo follow-on sites the suite caught, both worth naming
failure_lane's canonical category list needed the additions. Its doc says "keep this in lockstep" and nothing enforces that except the test — worth knowing if you add categories.every_failure_category_is_explainable_and_classifiedrejected them for having no user-facing explanation. The generic fallback they would have used reads:That advice can never work for a refused or malformed request. Each new category now says plainly that retrying will not help and names what has to change.
This is the same defect the epic fixes for the model, one layer up. The user was also being told to retry something that could not succeed. Good test — it caught a real gap rather than just a missing table entry.
Test Strategy
User behavior: a model call refused by policy, or malformed, now fails once with an accurate reason and an explanation that does not tell the user to retry. Previously it burned silent retries and reported a generic host outage.
Risk areas:
reason_kind→ retry disposition → failure lane → user summary)New:
permanent_model_stage_failures_are_not_categorized_as_transient_outagesasserts each kind gets a category and that the category is not auto-retriable. The second half is the part that matters — a category alone would not have fixed the retry burn.Two tests inverted, each with the reason recorded beside it:
model_stage_host_error_kind_category_matrix_is_exhaustivepinned all four atNone. The epic anticipated this exactly: "a runner test pins the wrong behavior as expected — fix the mapping and invert the test."text_only_model_reply_driver_sanitizes_model_failures_and_skips_transcript_writepinned the genericmodel_errorreason kind for a model-stagePolicyDenied.Red verification: the new test fails against the old mapping with "
PolicyDeniedhas no model-stage category, so it falls through to the generic host-stage outage and is silently auto-retried."Security Impact
None directly. A refused call is now reported as refused rather than as a host outage, which is more accurate to the operator and does not disclose anything the failure did not already carry.
Reborn Trust-Boundary Checklist
text_loop_driver(reason_kind),retry_disposition(falls toUserInitiated— intended),failure_lane(canonical list updated),failure_summary(explanations added). That chain is exactly what the two failing tests walked me through.serde(default): N/A.Database Impact
None.
Blast Radius
ironclaw_runnerfailure categorization. A model-stage failure of these four kinds now surfaces a differentreason_kindand is no longer auto-retried. Anything keying onmodel_errorfor these specific kinds will see the more specific value — that is the intent, and the audit above lists every consumer.Worth flagging to whoever watches run-failure dashboards: some failures previously counted as
host_stage_unavailable_model/model_errorwill move to the three new categories, and their auto-retry volume will drop. Same events, honest labels, fewer wasted calls.Rollback Plan
Revert the commit. No migration and no persisted shape change; the categories are runtime strings.
Review Follow-Through
One judgment call worth a look: I mapped
InvalidInvocationandInvalidto a singlemodel_stage_request_invalidbecause the remediation is identical (fix the request). If they warrant separate operator-facing messages, splitting is a one-line change.Scope note: I sized the other WS1/WS3 tails while here and deliberately left them out.
LoopDiagnosticRef's deletion is ~70 references across four crates including public struct fields — a refactor, not a tail. Thedetail: Noneproducer sweep is unbounded until someone surveys it. Neither belongs in this PR.Review track: C (runtime)
🤖 Generated with Claude Code