feat(turns): RunFailureReason funnel foundation (phase 1 of 4) - #5954
ilblackdragon wants to merge 1 commit into
Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
github-actions
Bot
added
size: XL
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a public ChangesRun failure classification
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new run_failure module to provide structured, single-funnel classification for terminal run failures, mapping them to specific recovery lanes, retry policies, and user-facing messages. The review feedback highlights a critical issue in RunFailureReason::from_category_str where unrecognized or newer category strings are mapped to UnknownFailure, causing the original category string to be lost and overwritten with 'unknown_failure'. A code suggestion is provided to preserve the original category string while maintaining the fallback classification behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| /// Classify from a raw persisted/wire category string (recompute on read). | ||
| pub fn from_category_str( | ||
| category: &str, | ||
| source: FailureSource, | ||
| correlation_id: TurnRunId, | ||
| ) -> Self { | ||
| Self::classify( | ||
| RunFailureCategory::from_category_str(category), | ||
| source, | ||
| correlation_id, | ||
| ) | ||
| } |
There was a problem hiding this comment.
When reconstructing a RunFailureReason from a persisted category string using from_category_str, any unrecognized/newer category string (e.g., "some_future_category") is mapped to RunFailureCategory::UnknownFailure. This causes Self::classify to set the internal category field to "unknown_failure", completely losing the original category string.
Since the PR design aims for byte-identical persistence (where the category is recomputed on read and written back as-is), this loss of fidelity violates that guarantee and mutates newer categories to "unknown_failure" upon write-back.
We should preserve the original category string (if it is a valid SanitizedFailure) while still classifying it under the UnknownFailure rules.
/// Classify from a raw persisted/wire category string (recompute on read).
pub fn from_category_str(
category: &str,
source: FailureSource,
correlation_id: TurnRunId,
) -> Self {
let parsed = RunFailureCategory::from_category_str(category);
let (lane, retry_policy) = parsed.lane_and_policy();
let sanitized_category = SanitizedFailure::new(category)
.unwrap_or_else(|_| SanitizedFailure::from_trusted_static(parsed.as_str()));
Self {
lane,
retry_policy,
user_message: parsed.user_message(),
correlation_id,
category: sanitized_category,
}
}![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | a14ab587380d |
Head: a14ab587380d147f66313a03d34f1525c20d5f05
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found two blocking issues in the new run-failure funnel API: the safety-stop evidence token is publicly mintable, and persisted security stops cannot round-trip back to the SecurityStop lane.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [HIGH] Safety-stop evidence can be minted by any caller
Location: crates/ironclaw_turns/src/run_failure.rs:393
SafetyStopEvidence::from_safety_layer is a public constructor on a public type that is also re-exported from ironclaw_turns, so any crate that can depend on ironclaw_turns can mint this token and call RunFailureReason::security_stop. That makes the promised safety-only origin false and lets non-safety code classify arbitrary terminal failures as SecurityStop. Move the proof/token construction to a safety-owned boundary, restrict the constructor, or otherwise enforce this with a boundary/compile-fail test before exposing the lane.
2. ❌ [MEDIUM] Persisted security stops rehydrate as ordinary unknown failures
Location: crates/ironclaw_turns/src/run_failure.rs:440-449
security_stop persists evidence.category() as the only stored category, but from_category_str maps any category outside RunFailureCategory to UnknownFailure and then classify rewrites it to unknown_failure with the Explainable lane. A real safety category such as prompt_injection_blocked would therefore lose both its original category and its SecurityStop lane when recomputed from persisted state. Store the lane, reserve/parse security-stop categories or prefixes, or add a security-aware persisted form and cover the round trip in tests.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/failed/superseded state while reviewers run.
| /// Mint security-stop evidence from the safety layer. The caller is | ||
| /// `ironclaw_safety` at a real block decision (prompt-injection detection or | ||
| /// a confirmed secret leak); `category` is the sanitized block category. | ||
| pub fn from_safety_layer(category: SanitizedFailure) -> Self { |
There was a problem hiding this comment.
This constructor is public on a public, re-exported type, so every ironclaw_turns consumer can mint SafetyStopEvidence and call RunFailureReason::security_stop. That does not enforce the documented safety-layer-only origin; please move/restrict the minting boundary and add a boundary/compile-fail guard.
| } | ||
|
|
||
| /// Classify from a raw persisted/wire category string (recompute on read). | ||
| pub fn from_category_str( |
There was a problem hiding this comment.
This read-side recomputation cannot reconstruct SecurityStop: security_stop stores an arbitrary safety category, while unknown strings here become UnknownFailure and are re-persisted as unknown_failure with the Explainable lane. Please add a security-aware persisted representation or parser and a round-trip test.
There was a problem hiding this comment.
Pull request overview
Adds the initial “single-funnel” data model for terminal run failures in ironclaw_turns, introducing a typed classification layer intended to make unclassified terminal failures structurally unrepresentable once wired into run-exit contracts in later phases.
Changes:
- Introduces
run_failuremodule withRunFailureReason,RunFailureCategory, lane/policy enums, and a non-emptyUserMessagenewtype. - Adds parsing/round-trip helpers for wire-stable failure category strings and a baseline per-category user message table.
- Exposes the new types from
ironclaw_turns’ public API vialib.rsre-exports.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/ironclaw_turns/src/run_failure.rs | Adds the failure funnel foundation: categories, classification into lane/retry/message, security-stop pathway, and tests. |
| crates/ironclaw_turns/src/lib.rs | Exposes the new run_failure module and re-exports its public types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl SafetyStopEvidence { | ||
| /// Mint security-stop evidence from the safety layer. The caller is | ||
| /// `ironclaw_safety` at a real block decision (prompt-injection detection or | ||
| /// a confirmed secret leak); `category` is the sanitized block category. | ||
| pub fn from_safety_layer(category: SanitizedFailure) -> Self { | ||
| Self { category } | ||
| } |
| /// Which of the three terminal outcomes a run failure resolves to. | ||
| /// | ||
| /// This is the two-bucket end state: a security-related failure stops the run; | ||
| /// everything else is user-explainable and, where safe, retriable. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] |
| /// Provenance of a terminal failure — which recording path produced it. | ||
| /// | ||
| /// Carried alongside the category so a future classifier can nuance lane/retry | ||
| /// by source without re-deriving it. Today the category alone determines the | ||
| /// classification; `source` is retained for diagnostics and forward flexibility. |
![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_turns/src/run_failure.rs`:
- Around line 389-400: Make the minting guarantee accurate: either introduce an
unforgeable/private capability required by SafetyStopEvidence::from_safety_layer
and ensure only the safety layer can obtain it, or soften the method and related
documentation to explicitly describe this as a convention rather than an
enforced boundary. Review SanitizedFailure::new, the SafetyStopEvidence
re-export, and RunFailureReason::security_stop so the public API does not claim
downstream crates are unable to forge tokens.
🪄 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: 6a8ff907-2c41-4252-a5cc-09a4053a2187
📒 Files selected for processing (2)
crates/ironclaw_turns/src/lib.rscrates/ironclaw_turns/src/run_failure.rs
| impl SafetyStopEvidence { | ||
| /// Mint security-stop evidence from the safety layer. The caller is | ||
| /// `ironclaw_safety` at a real block decision (prompt-injection detection or | ||
| /// a confirmed secret leak); `category` is the sanitized block category. | ||
| pub fn from_safety_layer(category: SanitizedFailure) -> Self { | ||
| Self { category } | ||
| } | ||
|
|
||
| fn category(&self) -> SanitizedFailure { | ||
| self.category.clone() | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How is SafetyStopEvidence minted across the workspace, and is there a real seal?
rg -nP '\bfrom_safety_layer\s*\(' --type=rust
rg -nP 'SafetyStopEvidence' --type=rust -C2
# Any sealed-trait / crate-private mechanism claimed by the "boundary test"?
rg -nP 'security_stop|single.?origin|boundary test' --type=rust -C2Repository: nearai/ironclaw
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- run_failure.rs outline ---'
ast-grep outline crates/ironclaw_turns/src/run_failure.rs --view expanded || true
echo '--- run_failure.rs lines 340-430 ---'
sed -n '340,430p' crates/ironclaw_turns/src/run_failure.rs | cat -n
echo '--- search SafetyStopEvidence / SanitizedFailure / security_stop ---'
rg -n 'SafetyStopEvidence|SanitizedFailure|security_stop|from_safety_layer|single.?origin|boundary test|seal|sealed' crates/ironclaw_turns/src -C 3 || true
echo '--- find SanitizedFailure definition ---'
rg -n 'struct SanitizedFailure|enum SanitizedFailure|impl SanitizedFailure' crates/ironclaw_turns/src -C 3 || trueRepository: nearai/ironclaw
Length of output: 38529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- call sites of RunFailureReason::security_stop / SafetyStopEvidence::from_safety_layer ---'
rg -n 'RunFailureReason::security_stop|SafetyStopEvidence::from_safety_layer|security_stop\(' crates src tests -C 2 || true
echo '--- workspace members / published surface clues ---'
sed -n '1,220p' Cargo.toml | cat -n
echo '--- ironclaw_turns lib visibility ---'
sed -n '1,160p' crates/ironclaw_turns/src/lib.rs | cat -nRepository: nearai/ironclaw
Length of output: 22409
SafetyStopEvidence is forgeable from downstream crates
from_safety_layer is pub, SanitizedFailure::new is pub, and SafetyStopEvidence is re-exported from crates/ironclaw_turns/src/lib.rs, so any crate that can import ironclaw_turns can mint a security-stop token and call RunFailureReason::security_stop(...). The doc’s “only ironclaw_safety can mint” claim is too strong unless there’s a real seal elsewhere. Either add an unforgeable guard or soften the API/docs to make this a convention, not a compile-time boundary.
🤖 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_turns/src/run_failure.rs` around lines 389 - 400, Make the
minting guarantee accurate: either introduce an unforgeable/private capability
required by SafetyStopEvidence::from_safety_layer and ensure only the safety
layer can obtain it, or soften the method and related documentation to
explicitly describe this as a convention rather than an enforced boundary.
Review SanitizedFailure::new, the SafetyStopEvidence re-export, and
RunFailureReason::security_stop so the public API does not claim downstream
crates are unable to forge tokens.
Sources: Coding guidelines, Path instructions
|
🚅 Deployed to the ironclaw-pr-5954 environment in ironclaw-ci-preview
|
Add `ironclaw_turns::run_failure` — the single classification funnel every
terminal run failure will route through (target architecture from the
error-recoverability audit; follows the exhaustive-classification keystone
Phase 1 is additive and consumed by nothing yet (no behavior change):
- `RunFailureReason` with ALL-private fields and only two constructors,
`classify()` and `security_stop()`. Once the run-exit contracts carry it
(phase 2), a terminal failure cannot be recorded without passing a funnel.
- `RunFailureCategory`: a wildcard-free enum over every terminal category
(LoopFailureKind, protocol violations, driver/scheduler paths, model
provider categories). `classify` matches it exhaustively, so a new
category can't be added without deciding its lane/retry/message.
- `FailureLane { SecurityStop, Retriable, Explainable }` + `RetryPolicy`.
`classify` never yields SecurityStop; the only path to it is
`security_stop`, gated behind `SafetyStopEvidence` (mintable only by the
safety layer — wired in phase 4).
- `UserMessage`: non-empty newtype, so "a terminal failure with no
user-facing explanation" is unrepresentable. The per-category message
table is moved in from `reborn_composition::failure_summary`.
- `category()` returns the wire-stable `SanitizedFailure` so persistence
stays byte-identical (lane/retry/message are recomputable on read).
Tests: exhaustive per-category classification (non-empty message, never
SecurityStop), category-string round-trip, lane/retry agreement, and the
security_stop path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a14ab58 to
805cb13
Compare
| //! 3. **`SecurityStop` originates only in the safety layer.** `classify` never | ||
| //! yields [`FailureLane::SecurityStop`]; the only path to it is | ||
| //! [`RunFailureReason::security_stop`], gated behind a [`SafetyStopEvidence`] | ||
| //! token that only `ironclaw_safety` can mint. |
| /// Which of the three terminal outcomes a run failure resolves to. | ||
| /// | ||
| /// This is the two-bucket end state: a security-related failure stops the run; | ||
| /// everything else is user-explainable and, where safe, retriable. |
| /// Provenance of a terminal failure — which recording path produced it. | ||
| /// | ||
| /// Carried alongside the category so a future classifier can nuance lane/retry | ||
| /// by source without re-deriving it. Today the category alone determines the | ||
| /// classification; `source` is retained for diagnostics and forward flexibility. |
| /// reachable only from the safety layer. The single-origin property is | ||
| /// additionally locked by a boundary test. | ||
| #[derive(Debug, Clone)] | ||
| pub struct SafetyStopEvidence { |
| /// Mint security-stop evidence from the safety layer. The caller is | ||
| /// `ironclaw_safety` at a real block decision (prompt-injection detection or | ||
| /// a confirmed secret leak); `category` is the sanitized block category. | ||
| pub fn from_safety_layer(category: SanitizedFailure) -> Self { |
|
|
||
| /// The security-stop funnel: the only path to [`FailureLane::SecurityStop`]. | ||
| /// Requires [`SafetyStopEvidence`], which only the safety layer can mint. | ||
| pub fn security_stop(evidence: SafetyStopEvidence, correlation_id: TurnRunId) -> Self { |
| pub use run_failure::{ | ||
| FailureLane, FailureSource, RetryPolicy, RunFailureCategory, RunFailureReason, | ||
| SafetyStopEvidence, UserMessage, UserMessageError, | ||
| }; |
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.18% — 290621 / 341200 lines Per-crate breakdown (63 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)
|
|
Closing — superseded by already-merged #5692 ("reborn: no run-borking failures — collapsed recoverability stack"). This PR was Phase 1 of a Continuing would introduce a duplicate The static no-swallow enforcement that motivated this whole effort is delivered by the merged #5651 (exhaustive capability-error classification) and #5652 ( |
What
Phase 1 of the "every terminal run failure passes one classifier" funnel — the target architecture from the error-recoverability audit, and the run-boundary companion to the exhaustive-classification keystone (#5651). Additive and consumed by nothing yet — zero behavior change.
Adds
ironclaw_turns::run_failure:RunFailureReason— all-private fields, only two constructors:classify()andsecurity_stop(). Once the run-exit contracts carry it (phase 2), a terminal failure cannot be recorded without passing a funnel — an unsurfaced terminal error becomes structurally unrepresentable.RunFailureCategory— a wildcard-free enum over every terminal category (LoopFailureKind, protocol violations, driver/scheduler paths, model-provider categories).classifymatches it exhaustively, so a new category can't be added without deciding its lane/retry/message (compile error otherwise — ties into refactor(errors): static enforcement that failures surface, not swallow #5651).FailureLane { SecurityStop, Retriable, Explainable }+RetryPolicy.classifynever yieldsSecurityStop; the only path to it issecurity_stop, gated behindSafetyStopEvidence(mintable only by the safety layer — wired in phase 4).UserMessage— a non-empty newtype, so "a terminal failure with no user-facing explanation" is unrepresentable. The per-category message table is moved in fromreborn_composition::failure_summary.category()returns the wire-stableSanitizedFailureso persistence stays byte-identical (lane/retry/message are recomputable from the category on read).Tests
cargo test -p ironclaw_turns— the module's 5 tests pass: exhaustive per-category classification (non-empty message, never SecurityStop), category-string round-trip, lane/retry agreement, and thesecurity_stoppath.Follow-up PRs (this is 1 of 4)
failurefields (FailRunRequest,RecordRunnerFailureRequest,TurnRunnerOutcome::Failed,LoopExitMapping::RecoveryRequired) toRunFailureReason; extract.category()whereTurnRunState.failureis written (durable persistence stays byte-identical); route the three producer paths throughclassify. Preserves the exactly-oncefail_runinvariant.user_messagefromclassify()(keepFailureExplanationProvideras the model-enrichment override); addlane/retry_policytoProductProjectionItem::RunStatus(the UI retry signal).ironclaw_safety → ironclaw_turnsedge, mintSafetyStopEvidenceat the leak-block site, and a grep-based origin test lockingSecurityStopto the safety layer.🤖 Generated with Claude Code