Repository navigation
fix(reborn): make model-visible failures recoverable - #6437
Conversation
…ons + mapping re-bucket (checkpoint) Checkpoint of in-flight subagent work; will be reorganized into reviewed commits before any PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017n7vtfDLvAD9KUZLMTxWVm
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017n7vtfDLvAD9KUZLMTxWVm
…odel-visible operation failures A guest trap and a script timeout are driven by the composed request, not host infrastructure — an identical retry fails identically. Both previously classified into the retryable-infra Backend bucket, burning the availability retry budget before the model ever saw the error. Re-bucket both to OperationFailed so they surface as immediate model-visible tool errors (#6284 item 1; follows the audit's §6.1 re-bucket pattern). Red-first: flipped the pinned mapping expectations (dispatch_kind_to_failure_pins_every_runtime_dispatch_error_kind, new script_timeout_classifies_as_model_visible_operation_failure), watched both fail on the old mapping, then changed the two production arms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017n7vtfDLvAD9KUZLMTxWVm
…checkpoint 3) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017n7vtfDLvAD9KUZLMTxWVm
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
🔎 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates executor recovery and gate validation, adds stale-request classification and retry handling, preserves failure details, improves sandbox diagnostics, bounds WASM blocking work, and expands integration and replay coverage. ChangesExecutor recovery and gate enforcement
Sandbox and WASM runtime handling
Runner failure records
Integration and replay contracts
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
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
scope: docs
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
⏭️ IronLoop Review Declined: reviewer
Review at a glance
| Disposition | Head |
|---|---|
| ⏭️ Review declined | 277223b701ac |
Head: 277223b701ac3e7cfdedeb27d8731f13de1dd2f8
Reason: The stated base commit is not an ancestor of head (merge base: b9f86de). The supplied comparison spans 159 files and 3,583 additions across recovery semantics plus unrelated composition, migration, CLI, CI, Docker, harness, and documentation areas, so the intended PR layer cannot be isolated reliably.
Next: Rebase or provide the intended stack-layer base so it is an ancestor of head, then request review with unrelated composition/migration/CLI/CI changes split into separate PRs or explicitly identify the cumulative-stack review target.
Run details
Status: Current
Trustworthy review produced: no
Summary
Skipped: the supplied base-to-head comparison is a mega, cross-cutting diff that cannot be reviewed reliably within the configured focused-review scope.
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Make model-correctable Reborn failures recoverable and preserve precise failure evidence across execution, retry, projection, replay, and checkpoint boundaries.
Snapshot: 99cb2dc8f8094c160636448cad90c5b5005ba3e0 (refreshed after the PR merged current main)
Coverage: complete — 28 files, 6 packets, 8 specialist reviewers, 0 reviewer failures.
Stats: 9 deduplicated findings (1 High, 7 Medium, 1 Low) from 16 raw findings.
Findings
-
High — Sandbox diagnostics bypass the required secret and injection scrub (
crates/ironclaw_loop_host/src/capability_port.rs:2845-2864, confidence 100)
safe_sandbox_plan_cause converts raw serde/ProcessSandboxPlanError text into a model-visible diagnostic after stripping only delimiters and control characters. It bypasses this module's existing model_visible_diagnostic_text path, which applies the full secret registry and prompt-injection fencing. Several sandbox errors interpolate model-supplied host, path, and environment fields, so credential-shaped text or injected instructions can survive into the model-visible detail. The repository error-boundary rule forbids paths and credential material crossing the boundary without the established redaction contract. -
Medium — External-tool invalid skip outcome lacks stage coverage (
crates/ironclaw_agent_loop/src/executor/gates.rs:33-45, confidence 100)
The new enforcement contract explicitly rejects SkipAndContinue for Approval, AwaitDependentRun, and ExternalTool gates. The changed tests exercise Approval and AwaitDependentRun, but no GateStage test drives ExternalTool through this enforcement. A regression that exempts or misroutes ExternalTool would therefore pass while silently skipping a gated external call. -
Medium — No-progress explanation cancellation path is untested (
crates/ironclaw_agent_loop/src/executor/loop_exit.rs:284-288, confidence 100)
The new NoProgressDetected branch propagates the only error from attach_failure_explanation via await?: cancellation during prompt/model/finalization. Existing cancellation tests exercise the older Aborted explanation path, while the new no-progress tests cover successful and fail-soft explanations only. They would not catch this branch writing a Final checkpoint or returning Failed instead of Cancelled after cancellation. -
Medium — Spawn callers do not verify the new model-visible cause (
crates/ironclaw_host_runtime/src/production.rs:1587-1619, confidence 100)
Helper tests assert that malformed and invalid plans initially receive model_visible_cause, but the existing public spawn_capability and resume_spawn_capability contract tests assert only failure kind, disposition, and summary. They do not prove the newly added cause survives the production caller's failure finalization and scrubbing path, so wiring that drops the corrective diagnostic would still pass. -
Medium — WASM host failures are mislabeled as guest operation failures (
crates/ironclaw_host_runtime/src/production.rs:1878-1884, confidence 100)
RuntimeDispatchErrorKind::Guest is not limited to guest traps. wasm_error_kind maps every WasmError::ExecutionFailed to it, while run_wasm_execution_blocking and run_wasm_prepare_blocking construct that variant for a closed execution/preparation gate and for blocking-task panics. This new blanket mapping turns those host-runtime failures into model-visible OperationFailed results, removing the backend retry path and incorrectly asking the model to change its tool call. -
Medium — Deterministic invalid model requests are treated as stale (
crates/ironclaw_agent_loop/src/executor/mapping.rs:116-118, confidence 98)
InvalidInvocation is the loop-host mapping for every HostManagedModelErrorKind::InvalidRequest, whose contract includes malformed requests and unknown tools. Production producers also use it for invalid routes, provider/model identity mismatches, invalid replay metadata, and missing replay content. Rebuilding the same iteration cannot repair those deterministic faults, but this mapping performs repeated model calls, labels exhaustion model_stale_request, and assigns the Auto retry disposition. Only the genuinely stale-surface case is safe to retry this way. -
Medium — Delete the duplicate sandbox-plan validation path (
crates/ironclaw_loop_host/src/capability_port.rs:2791-2807, confidence 96)
The loop-host adapter now parses and validates SandboxProcessPlan, wraps failures in a provider-argument error, and sanitizes its own diagnostic even though DefaultHostRuntime::spawn_capability performs the same parse and validation and already returns a model-visible RuntimeCapabilityFailure. Production spawns therefore validate and serialize the plan twice, while validation rules, summaries, and diagnostic handling must remain synchronized across two crates. This also places runtime-specific request-shape logic in the upper adapter instead of the runtime owner. -
Medium — New stale-request attempt class lacks checkpoint round-trip coverage (
crates/ironclaw_agent_loop/src/state/slots.rs:427-436, confidence 75)
ModelStaleRequest is added to the serialized RecoveryAttemptClass used as a persistent BTreeMap key. Existing checkpoint lifecycle coverage round-trips ModelInvalidOutput and the generic state builder uses ModelTransient; no test serializes and reloads the new wire value model_stale_request. Replay/checkpoint compatibility for the new retry counter is therefore unverified. -
Low — Gate outcome documentation still recommends a now-invalid skip (
crates/ironclaw_agent_loop/src/executor/gates.rs:26-32, confidence 100)
The new enforcement makes SkipAndContinue invalid for Approval, AwaitDependentRun, and ExternalTool gates, but the owning GateOutcome documentation still says this variant is intended for tools where a missing approval is non-fatal. A strategy author following that local documentation will now produce a DriverBug abort. The new comment also calls the conversion to the more severe Abort outcome a downgrade, which obscures the actual behavior.
Posted from the validated local multi-agent review; detailed fixes are attached inline.
| /// is bounded. Returns `None` when nothing legible remains. | ||
| fn safe_sandbox_plan_cause(raw: &str) -> Option<String> { | ||
| const MAX_BYTES: usize = 400; | ||
| let sanitized: String = raw |
There was a problem hiding this comment.
High — Sandbox diagnostics bypass the required secret and injection scrub
safe_sandbox_plan_cause converts raw serde/ProcessSandboxPlanError text into a model-visible diagnostic after stripping only delimiters and control characters. It bypasses this module's existing model_visible_diagnostic_text path, which applies the full secret registry and prompt-injection fencing. Several sandbox errors interpolate model-supplied host, path, and environment fields, so credential-shaped text or injected instructions can survive into the model-visible detail. The repository error-boundary rule forbids paths and credential material crossing the boundary without the established redaction contract.
Fix: Pass the cause through model_visible_diagnostic_text before applying any additional SafeSummary-compatible delimiter normalization, and add a regression case containing credential-shaped/injection text.
Also flagged by: tests/Medium, approach/Medium, bugs/Medium, security/Medium
There was a problem hiding this comment.
Fixed in 16f9955. Sandbox validation causes now pass through the canonical leak detector, model-visible sanitizer, and injection scanner before a compact untrusted-data fence, SafeSummary delimiter normalization, and the existing UTF-8-safe 400-byte bound. The caller test proves credential-shaped text is redacted while corrective detail remains model-visible. Verified by the full loop-host suite and workspace Clippy.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — sandbox validation causes use the canonical compact scrubber, secret redaction, and injection fence on the current head. Verification: full ironclaw_loop_host suite and workspace clippy passed.
| /// mask a driver bug behind a normal completion. The invalid outcome is | ||
| /// downgraded to `Abort` with the validator's failure kind (`DriverBug`) so | ||
| /// the run fails through the standard abort path. | ||
| fn enforce_gate_outcome_contract(outcome: GateOutcome, kind: GateKind) -> GateOutcome { |
There was a problem hiding this comment.
Medium — External-tool invalid skip outcome lacks stage coverage
The new enforcement contract explicitly rejects SkipAndContinue for Approval, AwaitDependentRun, and ExternalTool gates. The changed tests exercise Approval and AwaitDependentRun, but no GateStage test drives ExternalTool through this enforcement. A regression that exempts or misroutes ExternalTool would therefore pass while silently skipping a gated external call.
Fix: tests::executor::external_tool_gate_skip_and_continue_fails_as_driver_bug covering an ExternalTool GateStage outcome of SkipAndContinue and asserting Failed(DriverBug) with no subsequent model turn
There was a problem hiding this comment.
Fixed in 16f9955. Added a full executor caller-path test for ExternalTool plus SkipAndContinue; it asserts Failed(DriverBug), a checkpoint, and no subsequent model turn. The agent-loop suite passes.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — the full executor test covers ExternalTool plus SkipAndContinue and asserts DriverBug with no subsequent model turn. Verification: current-head test audit and workspace clippy passed.
| let explanation_message_ref = if completed { | ||
| None | ||
| } else { | ||
| attach_failure_explanation(ctx, &mut state, LoopFailureKind::NoProgressDetected) |
There was a problem hiding this comment.
Medium — No-progress explanation cancellation path is untested
The new NoProgressDetected branch propagates the only error from attach_failure_explanation via await?: cancellation during prompt/model/finalization. Existing cancellation tests exercise the older Aborted explanation path, while the new no-progress tests cover successful and fail-soft explanations only. They would not catch this branch writing a Final checkpoint or returning Failed instead of Cancelled after cancellation.
Fix: tests::executor::no_progress_explanation_cancellation_returns_cancelled_before_final_checkpoint covering cancellation during the NoProgressDetected explanation call and asserting AgentLoopExecutorError::Cancelled without a final failed checkpoint
Also flagged by: conventions/Medium
There was a problem hiding this comment.
Fixed in 16f9955. Added the no-progress explanation cancellation test and asserted AgentLoopExecutorError::Cancelled, no final checkpoint, and no finalized assistant message. The agent-loop suite passes.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — the no-progress cancellation test asserts Cancelled, no final checkpoint, and no finalized assistant message. Verification: current-head test audit and workspace clippy passed.
| // The parse cause ("missing field `run`", …) rides the | ||
| // model-visible Diagnostic channel — scrubbed at the loop | ||
| // seam — so the model can correct the plan shape on retry. | ||
| .with_model_visible_cause(error.to_string()), |
There was a problem hiding this comment.
Medium — Spawn callers do not verify the new model-visible cause
Helper tests assert that malformed and invalid plans initially receive model_visible_cause, but the existing public spawn_capability and resume_spawn_capability contract tests assert only failure kind, disposition, and summary. They do not prove the newly added cause survives the production caller's failure finalization and scrubbing path, so wiring that drops the corrective diagnostic would still pass.
Fix: tests::host_runtime_services_contract::host_runtime_spawn_and_resume_invalid_plan_preserve_model_visible_cause covering both public spawn paths and asserting the returned failure cause names the missing field or failed validation rule
There was a problem hiding this comment.
Fixed in 16f9955. Both public spawn_capability and resume_spawn_capability contract tests now assert that the returned model-visible cause retains run command must not be empty. The host-runtime services contract passes 114/114.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — both public spawn and resume contracts assert the corrective sandbox cause survives. Verification: current-head contract audit and workspace clippy passed.
| // same guest invocation as host infrastructure cannot repair it. | ||
| // Surface it as an operation failure so the model can change | ||
| // approach or report the broken extension. | ||
| DispatchFailureKind::Runtime(RuntimeDispatchErrorKind::Guest) => { |
There was a problem hiding this comment.
Medium — WASM host failures are mislabeled as guest operation failures
RuntimeDispatchErrorKind::Guest is not limited to guest traps. wasm_error_kind maps every WasmError::ExecutionFailed to it, while run_wasm_execution_blocking and run_wasm_prepare_blocking construct that variant for a closed execution/preparation gate and for blocking-task panics. This new blanket mapping turns those host-runtime failures into model-visible OperationFailed results, removing the backend retry path and incorrectly asking the model to change its tool call.
Fix: Give gate/join failures an executor/backend-specific WASM error kind, or classify them before this mapping, and map only confirmed guest execution traps to OperationFailed.
Also flagged by: tests/Medium
There was a problem hiding this comment.
Fixed in 16f9955. Added a typed WasmBlockingError provenance boundary: semaphore acquire and blocking-task join failures map to Executor, while runtime-returned guest execution failures retain Guest. The boundary was extracted into wasm_blocking.rs; focused WASM tests pass 16/16 and host-runtime Clippy is clean.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — WasmBlockingError preserves Executor provenance for host gate/join failures and runtime provenance for guest failures. Verification: current-head code/test audit and workspace clippy passed.
| | AgentLoopHostErrorKind::PolicyDenied | ||
| | AgentLoopHostErrorKind::CheckpointRejected | ||
| | AgentLoopHostErrorKind::TranscriptWriteFailed => None, | ||
| | AgentLoopHostErrorKind::Invalid => Some(ModelErrorClass::StaleRequest), |
There was a problem hiding this comment.
Medium — Deterministic invalid model requests are treated as stale
InvalidInvocation is the loop-host mapping for every HostManagedModelErrorKind::InvalidRequest, whose contract includes malformed requests and unknown tools. Production producers also use it for invalid routes, provider/model identity mismatches, invalid replay metadata, and missing replay content. Rebuilding the same iteration cannot repair those deterministic faults, but this mapping performs repeated model calls, labels exhaustion model_stale_request, and assigns the Auto retry disposition. Only the genuinely stale-surface case is safe to retry this way.
Fix: Introduce a distinct stale-request error kind across the model gateway boundary and retry only that kind; leave generic InvalidInvocation/Invalid terminal with their accurate category.
Also flagged by: performance/Medium
There was a problem hiding this comment.
Fixed in 16f9955, with replay-double alignment in 7e64d5d. Added the distinct serialized StaleRequest kind and retry only StaleSurface; generic InvalidRequest/InvalidInvocation is terminal and does not consume another response. Provider output outside the capability surface remains correctly typed InvalidOutput, so the model receives the bounded repair instruction without broadening invalid-request retries. The retry/resume E2E and trace parity targets pass.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — only typed StaleRequest maps to StaleSurface retry; deterministic invalid requests remain terminal. Verification: retry/resume E2E passed 21 tests with 1 credential-gated test ignored.
| let plan = serde_json::from_value::<SandboxProcessPlan>(input).map_err(|_| { | ||
| AgentLoopHostError::new( | ||
| AgentLoopHostErrorKind::InvalidInvocation, | ||
| let plan = serde_json::from_value::<SandboxProcessPlan>(input).map_err(|error| { |
There was a problem hiding this comment.
Medium — Delete the duplicate sandbox-plan validation path
The loop-host adapter now parses and validates SandboxProcessPlan, wraps failures in a provider-argument error, and sanitizes its own diagnostic even though DefaultHostRuntime::spawn_capability performs the same parse and validation and already returns a model-visible RuntimeCapabilityFailure. Production spawns therefore validate and serialize the plan twice, while validation rules, summaries, and diagnostic handling must remain synchronized across two crates. This also places runtime-specific request-shape logic in the upper adapter instead of the runtime owner.
Fix: Remove host_runtime_input_for_capability, sandbox_plan_input_error, and safe_sandbox_plan_cause; pass the provider-normalized JSON directly to HostRuntime::spawn_capability and let the existing runtime-failure mapper turn the host runtime's InvalidInput outcome and model-visible cause into the loop result. Test doubles should implement that HostRuntime contract rather than requiring a second production preflight.
There was a problem hiding this comment.
Fixed in 16f9955. Removed loop-host's duplicate sandbox parse/validation helpers and now pass provider-normalized JSON directly to host-runtime. The loop-host test double implements the runtime contract and asserts one spawn attempt, preserving caller coverage without a second production validator.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — duplicate loop-host sandbox validation helpers are absent and runtime remains the validation owner. Verification: full ironclaw_loop_host suite and workspace clippy passed.
| ModelInvalidOutput, | ||
| ModelUnavailable, | ||
| ModelInternal, | ||
| ModelStaleRequest, |
There was a problem hiding this comment.
Medium — New stale-request attempt class lacks checkpoint round-trip coverage
ModelStaleRequest is added to the serialized RecoveryAttemptClass used as a persistent BTreeMap key. Existing checkpoint lifecycle coverage round-trips ModelInvalidOutput and the generic state builder uses ModelTransient; no test serializes and reloads the new wire value model_stale_request. Replay/checkpoint compatibility for the new retry counter is therefore unverified.
Fix: tests::state_lifecycle::model_stale_request_recovery_attempts_survive_checkpoint_reload covering a BeforeModel checkpoint containing RecoveryAttemptClass::ModelStaleRequest and asserting its count after deserialization
There was a problem hiding this comment.
Fixed in 16f9955. Added a BeforeModel checkpoint serialization/reload test with two ModelStaleRequest attempts and asserted the exact count after restoration. The agent-loop lifecycle and full crate suites pass.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — checkpoint lifecycle coverage round-trips ModelStaleRequest attempts and asserts the restored count. Verification: current-head test audit and workspace clippy passed.
| /// strategy outcome (§5a.1, docs/plans/2026-07-03-loop-failure-matrix.md): a | ||
| /// `SkipAndContinue` on an Approval / AwaitDependentRun / ExternalTool gate is | ||
| /// a strategy-contract violation, and silently skipping the gated call would | ||
| /// mask a driver bug behind a normal completion. The invalid outcome is |
There was a problem hiding this comment.
Low — Gate outcome documentation still recommends a now-invalid skip
The new enforcement makes SkipAndContinue invalid for Approval, AwaitDependentRun, and ExternalTool gates, but the owning GateOutcome documentation still says this variant is intended for tools where a missing approval is non-fatal. A strategy author following that local documentation will now produce a DriverBug abort. The new comment also calls the conversion to the more severe Abort outcome a downgrade, which obscures the actual behavior.
Fix: Update the GateOutcome::SkipAndContinue documentation in strategies/gate.rs to name Auth and Resource as the valid skip kinds and explicitly state that Approval, AwaitDependentRun, and ExternalTool skips are converted to Abort DriverBug; describe the conversion here as enforcement rather than a downgrade.
There was a problem hiding this comment.
Fixed in 16f9955. The owning docs now state that SkipAndContinue is valid only for Auth and Resource gates, name the three enforced Abort(DriverBug) cases, and describe the conversion as enforcement rather than a downgrade.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — GateOutcome documentation names Auth and Resource as valid skip kinds and the three enforced DriverBug cases. Verification: current-head source audit.
|
🚅 Deployed to the ironclaw-pr-6437 environment in ironclaw-ci-preview
|
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 99cb2dc8f809 |
Head: 99cb2dc8f8094c160636448cad90c5b5005ba3e0
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
Changes requested: sandbox validation diagnostics bypass the canonical secret and prompt-injection scrubber before becoming model-visible.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Scrub sandbox validation causes before exposing them to the model
Location: crates/ironclaw_loop_host/src/capability_port.rs:2847-2854
ProcessSandboxPlanError embeds model-supplied host/path/env strings, but this sanitizer only replaces delimiters before passing the text to CapabilityFailureDetail::Diagnostic. That bypasses scrub_model_visible_detail's leak-detector and injection fencing. A hostile or credential-shaped invalid value can reach model context unredacted, or cause the later transcript validator to drop the entire observation and lose the corrective detail. Route this through the canonical scrubber before applying SafeSummary formatting, and add hostile host/path coverage.
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.
| /// is bounded. Returns `None` when nothing legible remains. | ||
| fn safe_sandbox_plan_cause(raw: &str) -> Option<String> { | ||
| const MAX_BYTES: usize = 400; | ||
| let sanitized: String = raw |
There was a problem hiding this comment.
This turns a ProcessSandboxPlanError containing model-supplied host/path/env values into a model-visible diagnostic with only character replacement. It bypasses the canonical secret/prompt-injection scrubber, so hostile or credential-shaped input can be exposed or make the later transcript validator drop the recovery detail. Please route the cause through scrub_model_visible_detail before publishing it.
There was a problem hiding this comment.
Fixed in 16f9955. The character-only sandbox helper was removed with the duplicate loop-host validator. Runtime-owned validation causes now flow through the canonical secret scrubber and injection scanner, then a compact explicit untrusted-data fence and the legacy SafeSummary bound. A regression test covers both an api_key token and injection-shaped text.
There was a problem hiding this comment.
Reviewed; no code change: DUPLICATE/ALREADY ADDRESSED — runtime-owned sandbox causes cross the canonical secret scrubber and injection fence, with hostile-input caller coverage. Verification: full ironclaw_loop_host suite passed.
![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 `@tests/reborn_trace_error_path_parity.rs`:
- Around line 126-136: Update the assertions in the completed-run test to
inspect the second entry from harness.model_requests() and verify it contains a
ToolResult carrying the unadvertised-capability rejection diagnostic. Keep the
existing invocation, response-count, and request-count assertions, matching the
malformed-input test’s assertion style.
🪄 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: f43b57ac-1d01-4eb7-9d43-7cb1264f4a0e
📒 Files selected for processing (28)
crates/ironclaw_agent_loop/src/executor/gates.rscrates/ironclaw_agent_loop/src/executor/loop_exit.rscrates/ironclaw_agent_loop/src/executor/mapping.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/executor/tests/failure_matrix.rscrates/ironclaw_agent_loop/src/families/mod.rscrates/ironclaw_agent_loop/src/families/subagent.rscrates/ironclaw_agent_loop/src/state/slots.rscrates/ironclaw_agent_loop/src/strategies/recovery.rscrates/ironclaw_agent_loop/tests/safety_nets.rscrates/ironclaw_host_runtime/src/production.rscrates/ironclaw_host_runtime/src/services/runtime_adapters.rscrates/ironclaw_loop_host/src/capability_port.rscrates/ironclaw_runner/src/failure_lane.rscrates/ironclaw_runner/src/failure_summary.rscrates/ironclaw_runner/src/loop_exit_applier/tests/mod.rscrates/ironclaw_runner/src/retry_disposition.rscrates/ironclaw_runner/src/turn_runner.rscrates/ironclaw_turns/src/loop_exit.rscrates/ironclaw_turns/src/loop_exit/tests/mod.rsdocs/plans/2026-07-03-loop-failure-matrix.mdtests/integration/cancel.rstests/integration/support/builder.rstests/integration/support/group.rstests/integration/support/scripted_provider.rstests/reborn_failure_retry_resume_e2e.rstests/reborn_trace_error_path_parity.rstests/support/reborn_parity_qa/binary_e2e.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_host_runtime/src/services/wasm_blocking.rs`:
- Around line 24-30: Update the WASM execution and preparation admission flow
around WASM_EXEC_SEMAPHORE, WASM_PREPARE_SEMAPHORE, and their
acquire_owned().await calls to prevent indefinite process-wide queueing: add a
bounded acquire timeout and map timeout failures to the existing retryable
dispatch error, or implement equivalent per-scope fairness using the existing
ResourceGovernor scope context. Preserve the current concurrency limits while
ensuring one tenant cannot indefinitely starve other scopes.
In `@crates/ironclaw_runner/src/model_gateway.rs`:
- Around line 2761-2785: The existing test only validates
map_capability_host_error, so add a regression test through the caller that
invokes issue_host_prompt_bundle with a surface-version mismatch and asserts the
resulting gateway error is StaleRequest. Extend the
capability_model_request_errors_preserve_stale_distinction table with
ScopeMismatch if it should continue mapping to InvalidRequest.
🪄 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: 3d995348-a804-4f6c-a55e-485444dff7b8
📒 Files selected for processing (15)
crates/ironclaw_agent_loop/src/executor/gates.rscrates/ironclaw_agent_loop/src/executor/mapping.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/strategies/gate.rscrates/ironclaw_agent_loop/tests/state_lifecycle.rscrates/ironclaw_host_runtime/src/services.rscrates/ironclaw_host_runtime/src/services/runtime_adapters.rscrates/ironclaw_host_runtime/src/services/wasm_blocking.rscrates/ironclaw_host_runtime/src/services/wasm_execution.rscrates/ironclaw_host_runtime/tests/host_runtime_services_contract.rscrates/ironclaw_loop_host/src/capability_port.rscrates/ironclaw_loop_host/src/lib.rscrates/ironclaw_loop_host/src/model_visible_scrub.rscrates/ironclaw_runner/src/model_gateway.rstests/reborn_failure_retry_resume_e2e.rs
| /// Process-wide gate over concurrent native WASM execution. | ||
| pub(super) static WASM_EXEC_SEMAPHORE: std::sync::LazyLock<Arc<Semaphore>> = | ||
| std::sync::LazyLock::new(|| Arc::new(Semaphore::new(MAX_CONCURRENT_WASM_EXEC))); | ||
|
|
||
| /// Process-wide gate over concurrent WASM component compilation. | ||
| pub(super) static WASM_PREPARE_SEMAPHORE: std::sync::LazyLock<Arc<Semaphore>> = | ||
| std::sync::LazyLock::new(|| Arc::new(Semaphore::new(MAX_CONCURRENT_WASM_PREPARE))); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Global WASM concurrency gates are process-wide, not tenant-scoped, and have no acquire timeout.
WASM_EXEC_SEMAPHORE/WASM_PREPARE_SEMAPHORE bound total concurrency well, but a single tenant's burst of long-running WASM calls can exhaust all 64/16 slots process-wide and starve every other tenant queued on acquire_owned().await indefinitely — there's no fairness partitioning and no deadline, so the failure mode under sustained overload is unbounded queuing rather than load-shedding. Worth considering per-scope fairness (or at least a bounded acquire timeout that maps to a retryable dispatch error) given the existing scope-aware ResourceGovernor reservation already tracks tenant/scope for the same call.
Also applies to: 66-89
🤖 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_host_runtime/src/services/wasm_blocking.rs` around lines 24 -
30, Update the WASM execution and preparation admission flow around
WASM_EXEC_SEMAPHORE, WASM_PREPARE_SEMAPHORE, and their acquire_owned().await
calls to prevent indefinite process-wide queueing: add a bounded acquire timeout
and map timeout failures to the existing retryable dispatch error, or implement
equivalent per-scope fairness using the existing ResourceGovernor scope context.
Preserve the current concurrency limits while ensuring one tenant cannot
indefinitely starve other scopes.
There was a problem hiding this comment.
Skipped for this PR after verification. The process-wide semaphores and unbounded acquire_owned() waits are real, but there is no existing retryable dispatch error that can represent an admission timeout without changing runtime contracts and recovery semantics across layers. This PR intentionally keeps timeout provenance in the executor lane until that typed contract exists; adding per-scope fairness or load-shedding belongs in a dedicated resource-governance change. The existing 64/16 concurrency bounds remain unchanged.
There was a problem hiding this comment.
@serrrfirat, agreed—this remains a valid capacity-isolation concern, but a timeout without a typed retryable admission-failure contract would be an incomplete cross-layer change. Deferring it keeps this PR focused and preserves the existing 64/16 bounds.
Would you like me to open a follow-up GitHub issue for tenant-aware WASM admission control / bounded load-shedding?
There was a problem hiding this comment.
Reviewed; no code change: QUESTION/NON-ACTIONABLE for this PR — bounded admission or tenant fairness requires a typed cross-layer load-shedding contract and would materially expand the change; the thread already agreed to defer it. Existing 64/16 concurrency bounds are unchanged.
There was a problem hiding this comment.
@serrrfirat, acknowledged. The concern is deferred as non-actionable for #6437: the current process-wide 64/16 bounds are preserved, and introducing timeout/load-shedding or tenant-aware admission correctly requires a dedicated typed cross-layer contract.
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 `@tests/reborn_failure_retry_resume_e2e.rs`:
- Around line 32-34: Revise the documentation comment above the affected test to
describe recovery from a stale model request injected at the model gateway,
rather than claiming recovery of a stale capability-surface mismatch. Remove
assertions about cross-layer capability recovery unless the test is expanded to
exercise an actual surface change.
🪄 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: d237626c-5a11-4092-aa53-ddaba8a08f5e
📒 Files selected for processing (10)
crates/ironclaw_runner/src/model_gateway.rstests/integration/support/doubles/recording_network_http_egress.rstests/integration/support/doubles/recording_test_capability_port.rstests/integration/support/harness/mod.rstests/integration/support/harness/recorder.rstests/integration/web_access.rstests/reborn_failure_retry_resume_e2e.rstests/reborn_trace_error_path_parity.rstests/reborn_trace_wasm_github_fixture_parity.rstests/support/reborn_parity_qa/binary_e2e.rs
…coverability-item1 # Conflicts: # crates/ironclaw_host_runtime/src/services.rs # crates/ironclaw_host_runtime/src/services/runtime_adapters.rs # crates/ironclaw_host_runtime/src/services/wasm_execution.rs # crates/ironclaw_loop_host/src/capability_port.rs # tests/integration/support/doubles/recording_network_http_egress.rs # tests/integration/support/doubles/recording_test_capability_port.rs
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.26% — 305059 / 353671 lines Per-crate breakdown (62 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)
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_loop_host/src/capability_port.rs`:
- Around line 3437-3464: Add a focused unit test for
sandbox_model_visible_diagnostic_text that supplies a diagnostic longer than 400
bytes containing multi-byte UTF-8 characters, then verifies the result is
truncated to at most 400 bytes and remains valid UTF-8 without splitting a
character. Keep the test targeted to the function’s byte-limit and
is_char_boundary walk-back behavior.
In `@crates/ironclaw_loop_host/src/lib.rs`:
- Around line 2343-2354: Replace or supplement
model_gateway_error_preserves_stale_request_kind with a caller-driven regression
test that invokes the production executor path using a stale model request.
Verify that the resulting StaleSurface triggers iteration-scoped retries and is
classified as terminal after retry exhaustion, rather than testing
model_gateway_error directly.
🪄 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: 76d8065d-e7b8-4868-a48c-a4fe66590335
📒 Files selected for processing (8)
crates/ironclaw_agent_loop/tests/safety_nets.rscrates/ironclaw_host_runtime/src/production.rscrates/ironclaw_host_runtime/src/services.rscrates/ironclaw_host_runtime/src/services/runtime_adapters.rscrates/ironclaw_host_runtime/src/services/wasm_execution.rscrates/ironclaw_host_runtime/tests/host_runtime_services_contract.rscrates/ironclaw_loop_host/src/capability_port.rscrates/ironclaw_loop_host/src/lib.rs
|
/canary |
|
Started Reborn WebUI v2 live canary for |
|
/canary |
|
Started Reborn WebUI v2 live canary for |
This branch was successfully deployed
2 participants
Summary
Change Type
Linked Issue
Related #6284 (item 1 only; this PR does not close the epic).
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings— ran the stronger workspace-wide command, plus the host-runtime command after the final module extraction.cargo build— not run separately; all changed crates and the workspace were compiled by tests and clippy.cargo test --features integration— not applicable: no database-backed behavior or schema changed.Test Strategy
User behavior: Model-correctable failures remain model-visible and can lead to a changed action in the same run. The new deterministic scenarios prove recovery for typed stale model requests, generic invalid capability input, Exa content-fetch failure with fallback search, and a real GitHub WASM guest
operation_failedresponse. Corrective sandbox validation details still reach the model only after canonical secret scrubbing and an explicit untrusted-data fence.Risk areas:
Tests added or updated:
get_contentfailure reaches the model as typed corrective context and recovers throughweb-access.search; a real GitHub WASM guest 422 reaches the model asoperation_failedand recovers throughgithub.get_repo.What the tests prove: Eligible failures cross the real loop/runner/checkpoint seams with stable categories, appear in the next model request as safe typed context, permit the model to choose a different action, and can still persist a final reply. Invalid gate outcomes fail closed; sandbox diagnostics remain useful, secret-scrubbed, and fenced; and host infrastructure failures cannot be mislabeled as guest tool failures.
Commands run:
cargo fmt --all -- --checkcargo test -p ironclaw_agent_loop --no-fail-fast -qcargo test -p ironclaw_loop_host --no-fail-fastcargo test -p ironclaw_runner --no-fail-fast -qcargo test -p ironclaw_host_runtime --no-fail-fastcargo test -p ironclaw_host_runtime services::wasm_execution::tests --lib -qcargo test -p ironclaw_host_runtime --test host_runtime_services_contract -qcargo test --test reborn_failure_retry_resume_e2ecargo test --test reborn_integration_web_accesscargo test --test reborn_trace_wasm_github_fixture_paritycargo test --test reborn_trace_error_path_parity reborn_trace_invalid_input_recovers_with_changed_action -- --nocapturecargo test -p ironclaw_loop_host process_sandbox_capability_maps_runtime_invalid_plan_failure_to_model -- --nocapturecargo test -p ironclaw_loop_host process_sandbox_rejection_keeps_scrubbed_fenced_diagnostic_model_visible -- --nocapturecargo test -p ironclaw_host_runtime --test github_wasm_runtime_contract host_runtime_services_keeps_github_non_validation_422_as_operation_failed -- --nocapturecargo test -p ironclaw_runner capability_model_request_errors_preserve_stale_distinction -- --nocapturecargo test -p ironclaw_architectureCARGO_TEST_ARGS='-q' scripts/reborn-e2e-rust.sh architecturecargo clippy --workspace --all-targets --all-features -- -D warningscargo clippy -p ironclaw_host_runtime --all-targets --all-features -- -D warningsscripts/pre-commit-safety.shgit diff --checkLocal-only limitations:
reborn_trace_error_path_paritybinary was stopped after multiple existing host-runtime-backed cases hung while building Reborn services; an untouched existing process-profile test reproduced the same harness issue. The new recording-seam invalid-input recovery test passes when run directly, and the other three affected binaries pass in full.system.process_sandbox.runyieldsSuspension::Process, and the canonical executor currently fails closed on unsupported process-wait resume. This PR therefore pairs the exact sandbox producer/scrubber contracts with a whole-turn capability-neutralInvalidInputrecovery test instead of adding a test-only adapter that would misrepresent production behavior.NetworkDenied. Every host-runtime integration target passed, including 114/114 host-runtime service contracts; the focused final WASM module run passed 16/16.Security Impact
This changes error handling at model, capability, sandbox, and gate boundaries. Process-sandbox request validation now has one production owner in host-runtime; its corrective cause is passed through the existing leak detector, model-visible text sanitizer, injection scanner, compact untrusted-data fence, legacy delimiter normalization, and a UTF-8-safe 400-byte bound before reaching the model. Authorization, approvals, network mediation, credential handling, and capability visibility are unchanged. Invalid gate outcomes continue to fail closed as
DriverBug.Reborn Trust-Boundary Checklist
HostManagedModelErrorKindmappings were audited;StaleRequestis additive and maps only toStaleSurface.serde(default)fields fail closed or have migration tests: stale-request recovery state survives checkpoint serialization and reload.Database Impact
None. No migration, schema, PostgreSQL, or libSQL behavior changed.
Blast Radius
Agent-loop recovery, host-managed model error serialization/mapping, runner retry disposition, process-sandbox failure diagnostics, WASM blocking error provenance, gate contract enforcement, and recovery checkpoint tests. Compatibility risk is limited to the additive
stale_requestserialized variant and the intentional behavior change that generic invalid model requests no longer retry.Rollback Plan
Revert this PR; there is no database or persistence-schema migration. Reverting restores generic invalid-request retries, the prior WASM error classification, and the previous sandbox diagnostic path. If rollback occurs during mixed-version operation, older readers will not understand newly persisted
stale_requestvalues, so drain or complete affected in-flight runs first.Review Follow-Through
Review track: C (runtime and trust-boundary error semantics)