Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion crates/ironclaw_agent_loop/src/executor/mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,16 @@ pub(super) fn model_error_class(error: &AgentLoopHostError) -> Option<ModelError
// Deliberately unclassified (terminal with diagnostics): deterministic
// request-invalid errors must not masquerade as stale/retryable, while
// policy denial and scope mismatch remain host/config-shaped. The
// runner preserves the original kind when categorizing the failure.
// runner names each of these with its own failure category
// (`model_stage_request_invalid` / `_policy_denied` / `_scope_mismatch`
// in `ironclaw_runner::failure_categories`), none of which is
// auto-retriable.
//
// This comment previously claimed the runner "preserves the original
// kind" — it did not. All four fell through to
// `host_stage_unavailable_model`, which the runner lists as a transient
// outage that re-drives cleanly, so a permanently-failing call was
// silently retried and reported as a generic host outage.
AgentLoopHostErrorKind::InvalidInvocation
| AgentLoopHostErrorKind::Invalid
| AgentLoopHostErrorKind::ScopeMismatch
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ use ironclaw_runner::failure_categories::{
HOST_STAGE_UNAVAILABLE_MODEL_CATEGORY, HOST_STAGE_UNAVAILABLE_PROMPT_CATEGORY,
HOST_STAGE_UNAVAILABLE_TRANSCRIPT_CATEGORY, HOST_STAGE_UNAVAILABLE_UNKNOWN_CATEGORY,
MODEL_CREDENTIALS_UNAVAILABLE_CATEGORY, MODEL_CREDITS_EXHAUSTED_CATEGORY,
MODEL_STAGE_POLICY_DENIED_CATEGORY, MODEL_STAGE_REQUEST_INVALID_CATEGORY,
MODEL_STAGE_SCOPE_MISMATCH_CATEGORY,
};
use ironclaw_turns::LoopFailureKind;

Expand Down Expand Up @@ -226,6 +228,22 @@ fn failure_summary_covers_reborn_failure_category_constants() {
HOST_STAGE_UNAVAILABLE_UNKNOWN_CATEGORY,
"The run failed because a required host stage was unavailable. Retry the run, and contact support if it keeps happening.",
),
// Permanent model-stage failures. Each says retrying will not help,
// because these previously inherited the generic "Retry the run"
// summary — advice that can never work for a refused or malformed
// request.
(
MODEL_STAGE_REQUEST_INVALID_CATEGORY,
"The model request was rejected as invalid. Retrying it unchanged will not help; the request itself needs to change.",
),
(
MODEL_STAGE_POLICY_DENIED_CATEGORY,
"Policy does not permit this model request. Retrying will not help; the policy or the model profile needs to change.",
),
(
MODEL_STAGE_SCOPE_MISMATCH_CATEGORY,
"The model request fell outside the granted scope. Retrying will not help; the scope or configuration needs to change.",
),
];
let source_values = reborn_failure_category_constant_values_from_source();
let expected_values = expected
Expand Down
18 changes: 18 additions & 0 deletions crates/ironclaw_runner/src/failure_categories.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,24 @@ pub const MODEL_CREDENTIALS_UNAVAILABLE_CATEGORY: &str = "model_credentials_unav
/// This must not be presented as a provider balance or configured-budget outcome.
pub const BUDGET_ACCOUNTING_FAILED_CATEGORY: &str = "budget_accounting_failed";

/// Model-stage failures that are PERMANENT for an identical retry.
///
/// These four kinds previously returned no category and fell through to
/// `host_stage_unavailable_model`, which `is_auto_retriable_category` treats as
/// a transient outage that re-drives cleanly. None of them can succeed on an
/// identical retry — policy does not change between attempts and a malformed
/// request stays malformed — so the run burned retries on a call that could not
/// work and reported a generic host outage instead of the real cause.
///
/// Deliberately NOT in `is_auto_retriable_category`.
pub const MODEL_STAGE_REQUEST_INVALID_CATEGORY: &str = "model_stage_request_invalid";
/// Model-stage call refused by policy. Permanent; see
/// [`MODEL_STAGE_REQUEST_INVALID_CATEGORY`].
pub const MODEL_STAGE_POLICY_DENIED_CATEGORY: &str = "model_stage_policy_denied";
/// Model-stage call outside the granted scope — configuration-shaped, not
/// transient. See [`MODEL_STAGE_REQUEST_INVALID_CATEGORY`].
pub const MODEL_STAGE_SCOPE_MISMATCH_CATEGORY: &str = "model_stage_scope_mismatch";

pub const HOST_STAGE_UNAVAILABLE_PROMPT_CATEGORY: &str = "host_stage_unavailable_prompt";
pub const HOST_STAGE_UNAVAILABLE_MODEL_CATEGORY: &str = "host_stage_unavailable_model";
pub const HOST_STAGE_UNAVAILABLE_CAPABILITY_CATEGORY: &str = "host_stage_unavailable_capability";
Expand Down
5 changes: 5 additions & 0 deletions crates/ironclaw_runner/src/failure_lane.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ pub const ALL_RUN_FAILURE_CATEGORIES: &[&str] = &[
"lease_expired",
// LoopFailureKind (ironclaw_turns)
"model_error",
// Permanent model-stage failures (see `failure_categories.rs`): named
// separately so they are not auto-retried as generic host outages.
"model_stage_request_invalid",
"model_stage_policy_denied",
"model_stage_scope_mismatch",
"context_build_failed",
"capability_protocol_error",
"iteration_limit",
Expand Down
13 changes: 13 additions & 0 deletions crates/ironclaw_runner/src/failure_summary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,19 @@ pub fn reborn_failure_summary_for_category(category: Option<&str>) -> &'static s
}

match category {
// Permanent model-stage failures. Each states plainly that retrying
// will not help, because these previously routed through the generic
// host-outage summary that told the user to "retry the run" — advice
// that could never work for a refused or malformed request.
"model_stage_request_invalid" => {
"The model request was rejected as invalid. Retrying it unchanged will not help; the request itself needs to change."
}
"model_stage_policy_denied" => {
"Policy does not permit this model request. Retrying will not help; the policy or the model profile needs to change."
}
"model_stage_scope_mismatch" => {
"The model request fell outside the granted scope. Retrying will not help; the scope or configuration needs to change."
}
"driver_not_found" => {
"The run could not start because the configured agent runtime was unavailable."
}
Expand Down
76 changes: 70 additions & 6 deletions crates/ironclaw_runner/src/model_failure_mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ use ironclaw_turns::run_profile::{AgentLoopHostErrorKind, AgentLoopHostErrorReas
use crate::failure_categories::{
BUDGET_ACCOUNTING_FAILED_CATEGORY, MODEL_CREDENTIALS_UNAVAILABLE_CATEGORY,
MODEL_CREDITS_EXHAUSTED_CATEGORY, MODEL_CREDITS_EXHAUSTED_REASON_KIND,
MODEL_STAGE_POLICY_DENIED_CATEGORY, MODEL_STAGE_REQUEST_INVALID_CATEGORY,
MODEL_STAGE_SCOPE_MISMATCH_CATEGORY,
};

pub(crate) fn model_stage_failure_category(
Expand All @@ -22,29 +24,91 @@ pub(crate) fn model_stage_failure_category(
return Some(BUDGET_ACCOUNTING_FAILED_CATEGORY);
}

(kind == AgentLoopHostErrorKind::CredentialUnavailable)
.then_some(MODEL_CREDENTIALS_UNAVAILABLE_CATEGORY)
if kind == AgentLoopHostErrorKind::CredentialUnavailable {
return Some(MODEL_CREDENTIALS_UNAVAILABLE_CATEGORY);
}

// Permanent for an identical retry. Without these, all four fell through to
// `host_stage_unavailable_model` — an auto-retriable transient outage — so
// the run re-drove a call that could not succeed and named the wrong cause.
// `executor/mapping.rs` already documented this as handled; it was not.
match kind {
AgentLoopHostErrorKind::InvalidInvocation | AgentLoopHostErrorKind::Invalid => {
Some(MODEL_STAGE_REQUEST_INVALID_CATEGORY)
}
AgentLoopHostErrorKind::PolicyDenied => Some(MODEL_STAGE_POLICY_DENIED_CATEGORY),
AgentLoopHostErrorKind::ScopeMismatch => Some(MODEL_STAGE_SCOPE_MISMATCH_CATEGORY),
_ => None,
}
}

#[cfg(test)]
mod tests {
use super::*;

/// A permanent model-stage failure must not be retried as a transient
/// host outage.
///
/// `InvalidInvocation`, `Invalid`, `ScopeMismatch` and `PolicyDenied` all
/// returned `None` here, so they fell through to
/// `host_stage_unavailable_model` — which `is_auto_retriable_category`
/// lists as a transient outage that "re-drives cleanly on a silent retry".
/// None of the four can succeed on an identical retry: policy does not
/// change between attempts, and a malformed request stays malformed. The
/// run burned retries on a call that could not work and told the operator
/// "host stage unavailable" instead of naming the real cause.
///
/// `executor/mapping.rs` already claimed this was handled — "the runner
/// preserves the original kind when categorizing the failure". It did not.
/// This pins the claim.
#[test]
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),
Comment on lines +65 to +107

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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

K::Unauthorized
| K::ScopeMismatch
| K::StaleSurface
| K::InvalidInvocation
| K::Invalid
| K::InvalidOutput
| K::ContentFiltered
| K::PolicyDenied
| K::BudgetExceeded
| K::BudgetApprovalRequired
| K::Unavailable
Expand Down
2 changes: 1 addition & 1 deletion crates/ironclaw_runner/src/retry_disposition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ impl RetryDisposition {
/// store / provider / tool faults where re-running the *identical* request from
/// the checkpoint is likely to succeed without any change. Conservative by
/// design — anything not clearly transient falls to `UserInitiated`.
fn is_auto_retriable_category(category: &str) -> bool {
pub(crate) fn is_auto_retriable_category(category: &str) -> bool {
matches!(
category,
// Host-stage transient outages
Expand Down
59 changes: 59 additions & 0 deletions crates/ironclaw_runner/src/text_loop_driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,65 @@ mod tests {
);
}

/// All four permanent model-stage kinds, through the driver path.
///
/// `permanent_model_stage_failures_are_not_categorized_as_transient_outages`
/// pins the classifier; this pins the CALLER. `map_host_error` reaches the
/// category via an early return that bypasses the whole kind match below
/// it, so the classifier being right does not prove the driver emits it —
/// and the emitted `reason_kind` is what `retry_disposition` keys on.
///
/// Before the fix all four produced a generic reason kind that routed
/// through `host_stage_unavailable_model`, which IS auto-retriable, so a
/// permanently-failing call was silently re-driven.
#[test]
fn permanent_model_stage_kinds_reach_the_driver_as_non_retriable_categories() {
use crate::failure_categories::{
MODEL_STAGE_POLICY_DENIED_CATEGORY, MODEL_STAGE_REQUEST_INVALID_CATEGORY,
MODEL_STAGE_SCOPE_MISMATCH_CATEGORY,
};
use crate::retry_disposition::is_auto_retriable_category;

let cases = [
(
AgentLoopHostErrorKind::InvalidInvocation,
MODEL_STAGE_REQUEST_INVALID_CATEGORY,
),
(
AgentLoopHostErrorKind::Invalid,
MODEL_STAGE_REQUEST_INVALID_CATEGORY,
),
(
AgentLoopHostErrorKind::ScopeMismatch,
MODEL_STAGE_SCOPE_MISMATCH_CATEGORY,
),
(
AgentLoopHostErrorKind::PolicyDenied,
MODEL_STAGE_POLICY_DENIED_CATEGORY,
),
];

for (kind, expected_category) in cases {
let mapped = map_host_error(
"model",
AgentLoopHostError::new(kind, "model stage rejected the request"),
);

let AgentLoopDriverError::Failed { reason_kind, .. } = &mapped else {
panic!("{kind:?} must surface as a Failed driver error, got {mapped:?}");
};
assert_eq!(
reason_kind, expected_category,
"{kind:?} must reach the driver as its own category, not a generic one"
);
assert!(
!is_auto_retriable_category(reason_kind),
"{kind:?} -> {reason_kind} is auto-retriable at the driver seam, so the run \
would silently re-drive a call that cannot succeed"
);
}
}

#[test]
fn non_model_stage_with_credential_unavailable_maps_to_model_error() {
let mapped = map_host_error(
Expand Down
9 changes: 8 additions & 1 deletion crates/ironclaw_runner/tests/loop_driver_host.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1594,9 +1594,16 @@ async fn text_only_model_reply_driver_sanitizes_model_failures_and_skips_transcr
.await
.unwrap_err();

// A model-stage `PolicyDenied` now names its own cause instead of the
// generic `model_error`. That generic label routed through
// `host_stage_unavailable_model`, which `is_auto_retriable_category` treats
// as a transient outage — so a permanently-refused call was silently
// re-driven. `model_stage_policy_denied` is not auto-retriable, which is
// the point of the change.
assert!(matches!(
error,
AgentLoopDriverError::Failed { ref reason_kind, detail: _ } if reason_kind == "model_error"
AgentLoopDriverError::Failed { ref reason_kind, detail: _ }
if reason_kind == "model_stage_policy_denied"
));
assert_driver_error_hides_raw_payloads(&error);
assert_no_assistant_message(&fixture).await;
Expand Down
Loading