reborn: no run-borking failures — failure explanation + retryable failed runs - #4841
Closed
serrrfirat wants to merge 15 commits into
Closed
serrrfirat wants to merge 15 commits into
serrrfirat wants to merge 15 commits into
Conversation
github-actions
Bot
added
scope: docs
|
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 (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds failure-explanation plumbing, retry-turn and resumable-checkpoint support, invalid-output repair retrying, retryable lifecycle/projection fields, and a WebUI v2 retry-run endpoint. ChangesExecutor failure explanations and repair flow
Turn retry contracts and storage
WebUI retry-run endpoint and facade
Runtime projection and host mappings
Reborn host, driver, and local-dev support
Estimated code review effort: 5 (Critical) | ~150 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements failure explanations and retryable failed runs. It introduces a best-effort model-based failure explanation mechanism before terminal exits, extends failed exits to carry verified explanation references and safe summaries, and adds a retry mechanism (retry_turn) to spawn new runs from the last resumable checkpoint. Additionally, it exposes a retry endpoint in the WebUI and surfaces retryability flags and actionable failure summaries in projections. Feedback on the changes highlights a critical issue in crates/ironclaw_turns/src/memory.rs, where clearing the checkpoint ID on failed runs prevents lease-expired or runner-failed runs from being retried, contradicting the core design goals.
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.
serrrfirat
marked this pull request as draft
June 13, 2026 09:51
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rs (1)
141-176:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin the new
retryablefield in the schema contract.The fixture now carries
retryable, but nothing asserts the serialized value. Add explicit assertions for the running and failed run-status entries so a serializer regression can’t drop or rename the field without failing CI.🤖 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_webui_v2/tests/webui_v2_schema_contract.rs` around lines 141 - 176, The test fixture projection_state() adds a new retryable field on ProductProjectionItem::RunStatus but the test doesn't assert its serialized value; update the test that serializes/deserializes ProductProjectionState (the code exercising ProductProjectionState::new / projection_state()) to explicitly assert the running entry has retryable == None (or serialized "retryable": null) and the failed entry has retryable == Some(true) (or serialized "retryable": true), so a serializer regression that drops/renames the field will fail CI.
🤖 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_agent_loop/src/executor/failure_explanation.rs`:
- Around line 166-171: The inline prompt string in function final_instruction
(returning a formatted String using LoopFailureKind::as_str) must be moved into
a prompt file per project convention; create
crates/ironclaw_engine/prompts/failure_explanation_instruction.md containing the
template ("The run is ending due to {}. Write a short honest message to the user
explaining what happened what was completed so far and what they can do next. Do
not invent details."), then update final_instruction to load that template via
include_str!("../../ironclaw_engine/prompts/failure_explanation_instruction.md")
(or the correct relative path) and call format!(included_str,
reason_kind.as_str()), leaving LoopFailureKind and final_instruction signature
unchanged.
In `@crates/ironclaw_agent_loop/src/test_support/mod.rs`:
- Around line 818-820: The field cancel_after_capability_batch is being cloned
instead of consumed so the cancellation keeps re-firing; change the code to take
the Option once and forward the inner value to set_cancellation_signal (e.g.
replace self.cancel_after_capability_batch.clone() with
self.cancel_after_capability_batch.take() and call set_cancellation_signal only
if take() returns Some) so the cancellation is one-shot; reference the
cancel_after_capability_batch field and the set_cancellation_signal method when
making this change.
In `@crates/ironclaw_product_adapters/src/outbound.rs`:
- Around line 951-954: The run-status field retryable is documented as valid
only for failed runs but the current validation allows retryable: Some(...) for
non-failed statuses; update the run-status validation logic that
constructs/validates the outbound run-status item to reject or clear retryable
when the run's status is not a failure (e.g., require that retryable is Some
only if status == Failed/RunOutcome::Failed and otherwise return a validation
error or set retryable to None). Locate the code that validates or serializes
run-status items (the struct with the retryable field and its
validation/serialization path) and enforce this invariant so wire contracts
never expose retryable for non-failed statuses.
In `@crates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rs`:
- Around line 375-378: Add a regression test that constructs a
TurnError::RunNotRetryable and asserts the code path classifies it as
TriggerError::InvalidMaterialization (the mapping exercised by the match arm
that calls rejected_trigger_materialization("trusted trigger submit rejected")).
Implement a #[test] or #[tokio::test] that creates the minimal inputs/mocks to
invoke the classification logic (or call the helper that converts TurnError to
TriggerError if exposed), feed it TurnError::RunNotRetryable, and assert the
resulting TriggerError equals TriggerError::InvalidMaterialization; keep the
test focused and deterministic so it fails if the mapping changes.
In `@crates/ironclaw_reborn/src/loop_driver_host.rs`:
- Around line 2231-2238: Add a regression case to the existing test suite (the
test function turn_error_to_host_error_tests) that constructs a
TurnError::RunNotRetryable { .. } instance and asserts that it is
converted/mapped to AgentLoopHostErrorKind::CheckpointRejected via the same
conversion/mapping path exercised by the other cases in that test (the code that
exercises the mapping in loop_driver_host.rs around the "checkpoint_state" write
branch). The test should mirror the existing assertions for TurnError::Conflict,
using TurnError::RunNotRetryable as the input and expecting
AgentLoopHostErrorKind::CheckpointRejected as the output so the contract is
locked in.
In `@crates/ironclaw_reborn/src/loop_driver_host/port_adapters.rs`:
- Around line 271-295: The function checkpoint_state_store_ref_and_run_id
currently strips any parseable "checkpoint:{run_id}:{token}" refs, allowing
foreign-run refs to be treated as local; update
checkpoint_state_store_ref_and_run_id to reject refs whose embedded run_id does
not match the current run_context.run_id: keep the existing branch that accepts
the exact run-scoped prefix (run_scoped_prefix = format!("checkpoint:{}:",
run_context.run_id)) and returns store_checkpoint_state_ref(token) with
run_context.run_id, but when handling the general "checkpoint:{run_id}:{token}"
case (the rest.split_once(':') branch), parse run_id and if it does not equal
run_context.run_id return Err(AgentLoopHostError::new(...)) indicating a foreign
run-scoped checkpoint ref instead of returning Ok; leave the branch that already
returns the original state_ref for non-checkpoint refs unchanged.
In `@crates/ironclaw_reborn/src/turn_runner.rs`:
- Around line 678-699: The current code silently falls back when
latest_resumable_checkpoint fails and then only logs if
transition_port.apply_validated_loop_exit(...) fails; change this to "fail
loud": make retry_checkpoint_for_claimed_run return a
Result<Option<CheckpointId>, E> (or otherwise propagate the checkpoint lookup
error) and at the call sites that currently do let resume_checkpoint_id =
self.retry_checkpoint_for_claimed_run(claimed).await handle Err by calling
self.record_runner_failure(...) with the same Failure payload (so the run gets a
terminal failure recorded) and returning early; likewise, if
transition_port.apply_validated_loop_exit(request).await returns Err, replace
the current log_runner_failure_record_error-only path with a call to
self.record_runner_failure(...) (passing runner_id, run_id, failure/explanation
as appropriate) so the terminal failure is recorded immediately instead of
leaving the run claimed.
In `@crates/ironclaw_turns/src/lifecycle.rs`:
- Around line 512-518: The lifecycle module's top-level contract
comment/documentation was not updated to reflect that coordinator-origin
publications now include retry_turn; update the module docstring or the
"lifecycle contract" comment adjacent to the lifecycle implementation to add
retry_turn (or equivalent wording) to the list of coordinator-origin publication
paths so it matches the behavior in the retry_turn method (which calls
publish_event_once_deferred(retry_event(...)) and delegates to
inner.retry_turn). Ensure the updated comment mentions retry_turn alongside
submit/resume/request_cancel/submit_child and keep phrasing consistent with
existing contract language.
In `@crates/ironclaw_turns/src/memory.rs`:
- Around line 3018-3044: Fix retry_idempotency_record so transient ThreadBusy
errors are not recorded as a permanent Error replay: in
retry_idempotency_record, when matching Err(error) detect TurnError::ThreadBusy
and map it to the same "thread-busy" variant used by the submit path (e.g., set
replay to TurnIdempotencyReplay::SubmitThreadBusy and outcome to the
corresponding outcome kind instead of TurnIdempotencyReplay::Error(...)); leave
other errors unchanged (keep TurnIdempotencyOutcomeKind::from_error(error) and
TurnIdempotencyReplay::Error(...) for non-ThreadBusy cases). Add a regression
test (#[test] or #[tokio::test]) asserting that a retry that yields
TurnError::ThreadBusy produces a record with the SubmitThreadBusy replay/outcome
and not an Error replay.
In `@crates/ironclaw_turns/src/runner.rs`:
- Around line 140-147: EventPublishingTurnRunTransitionPort currently fails to
delegate TurnRunTransitionPort::latest_resumable_checkpoint, dropping retry
checkpoint propagation; implement an async method latest_resumable_checkpoint on
EventPublishingTurnRunTransitionPort that simply calls and returns
self.inner.latest_resumable_checkpoint(scope, turn_id, run_id) to preserve the
decorator delegation invariant, and add/extend a caller-driven regression test
that exercises TurnRunnerWorker retry behavior through the
EventPublishingTurnRunTransitionPort wrapper to ensure checkpoint propagation is
preserved.
In `@crates/ironclaw_turns/tests/retry_failed_turn_store_contract.rs`:
- Around line 23-32: The engine_filesystem function currently calls .keep() on
tempfile::tempdir() which leaks the TempDir; change engine_filesystem to return
a tuple (LocalFilesystem, tempfile::TempDir) (or a struct wrapper) so the
TempDir guard outlives the test and is dropped to auto-cleanup, e.g., create let
storage = tempfile::tempdir().unwrap(); mount it into LocalFilesystem as before
but return (fs, storage) instead of fs; then update call sites that invoke
engine_filesystem to accept the tuple and hold the TempDir guard (keeping the
LocalFilesystem usage the same) so the temporary directory is removed when the
test scope ends.
In `@crates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rs`:
- Around line 497-517: The test stub's retry_run currently returns a synthetic
success when next_retry_run is empty, which can hide missing test setup; change
retry_run (and its use of next_retry_run.pop_front().unwrap_or_else(...)) to
fail loudly instead—e.g., replace the unwrap_or_else default with a panic or
explicit test-error path that reports "no scripted response for retry_run" so
unexpected calls surface during tests; leave the push to retry_run_calls
unchanged so the recorded call is still available for assertions.
---
Outside diff comments:
In `@crates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rs`:
- Around line 141-176: The test fixture projection_state() adds a new retryable
field on ProductProjectionItem::RunStatus but the test doesn't assert its
serialized value; update the test that serializes/deserializes
ProductProjectionState (the code exercising ProductProjectionState::new /
projection_state()) to explicitly assert the running entry has retryable == None
(or serialized "retryable": null) and the failed entry has retryable ==
Some(true) (or serialized "retryable": true), so a serializer regression that
drops/renames the field will fail CI.
🪄 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: d363683c-3046-4681-8220-8ed020150792
📒 Files selected for processing (90)
crates/ironclaw_agent_loop/src/executor.rscrates/ironclaw_agent_loop/src/executor/budget.rscrates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/exit_helpers.rscrates/ironclaw_agent_loop/src/executor/failure_explanation.rscrates/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/model.rscrates/ironclaw_agent_loop/src/executor/prompt.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/test_support/mod.rscrates/ironclaw_conversations/src/inbound.rscrates/ironclaw_conversations/src/trusted_trigger.rscrates/ironclaw_event_projections/src/pending_gate_projection/tests/support.rscrates/ironclaw_host_runtime/tests/turn_scheduler_contract.rscrates/ironclaw_loop_support/src/cancellation_port.rscrates/ironclaw_loop_support/src/subagent_spawn_port/tests.rscrates/ironclaw_loop_support/src/turn_event_publisher.rscrates/ironclaw_product_adapters/src/outbound.rscrates/ironclaw_product_workflow/src/auth_continuation.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/reborn_services/types.rscrates/ironclaw_product_workflow/src/webui_inbound.rscrates/ironclaw_product_workflow/tests/approval_interaction_contract.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_product_workflow/tests/inbound_turn_contract.rscrates/ironclaw_product_workflow/tests/product_workflow_contract.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_product_workflow/tests/webui_inbound_contract.rscrates/ironclaw_reborn/src/failure_categories.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_driver_host/port_adapters.rscrates/ironclaw_reborn/src/loop_driver_host/tests.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/mod.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/support.rscrates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn/src/turn_runner.rscrates/ironclaw_reborn/src/turn_runner/tests/mod.rscrates/ironclaw_reborn/tests/hooks_integration.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn/tests/loop_milestone_event_projection.rscrates/ironclaw_reborn_composition/src/factory/auth_tests.rscrates/ironclaw_reborn_composition/src/failure_summary.rscrates/ironclaw_reborn_composition/src/openai_compat_serve/tests.rscrates/ironclaw_reborn_composition/src/projection.rscrates/ironclaw_reborn_composition/src/projection/tests.rscrates/ironclaw_reborn_composition/src/projection/tests/failure_explanation.rscrates/ironclaw_reborn_composition/src/projection/tests/turn_stream.rscrates/ironclaw_reborn_composition/src/projection/tests/turn_stream_auth.rscrates/ironclaw_reborn_composition/src/projection/turn_events.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rscrates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rscrates/ironclaw_reborn_composition/tests/webui_v2_product_auth.rscrates/ironclaw_reborn_composition/tests/webui_v2_product_auth_4201.rscrates/ironclaw_reborn_composition/tests/webui_v2_serve.rscrates/ironclaw_reborn_openai_compat/tests/streaming_handlers_contract.rscrates/ironclaw_reborn_webui_ingress/tests/session_round_trip.rscrates/ironclaw_reborn_webui_ingress/tests/signed_session_multi_user.rscrates/ironclaw_reborn_webui_ingress/tests/support/harness.rscrates/ironclaw_turns/src/coordinator.rscrates/ironclaw_turns/src/events.rscrates/ironclaw_turns/src/filesystem_store.rscrates/ironclaw_turns/src/lib.rscrates/ironclaw_turns/src/lifecycle.rscrates/ironclaw_turns/src/loop_exit.rscrates/ironclaw_turns/src/loop_exit/tests/mod.rscrates/ironclaw_turns/src/memory.rscrates/ironclaw_turns/src/request.rscrates/ironclaw_turns/src/response.rscrates/ironclaw_turns/src/runner.rscrates/ironclaw_turns/src/status.rscrates/ironclaw_turns/src/store.rscrates/ironclaw_turns/tests/active_run_ref_state_contract.rscrates/ironclaw_turns/tests/checkpoint_state_store_contract.rscrates/ironclaw_turns/tests/retry_failed_turn_store_contract.rscrates/ironclaw_turns/tests/turn_coordinator_contract.rscrates/ironclaw_webui_v2/src/descriptors.rscrates/ironclaw_webui_v2/src/handlers.rscrates/ironclaw_webui_v2/src/lib.rscrates/ironclaw_webui_v2/src/router.rscrates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_operator_config_key_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rsdocs/plans/2026-06-12-reborn-no-borking-failures.mddocs/reborn/contracts/loop-exit.mddocs/reborn/contracts/turn-runner.md
serrrfirat
added a commit
that referenced
this pull request
Jun 13, 2026
terminal_transition and the runner-failure path cleared checkpoint_id on Failed, making lease-expired and externally-failed runs non-retryable — contradicting their "Retry the run." failure summary. Resolve to the latest resumable checkpoint instead (None when none exists, keeping the projected `retryable` flag consistent with retry_turn validation). Regression test covers both store backends, both the retryable and non-resumable branches. Addresses gemini-code-assist review on PR #4841. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
serrrfirat
added a commit
that referenced
this pull request
Jun 13, 2026
…ings Codex /thermo-nuclear-code-quality-review pass 2 (gpt-5.5 xhigh) resolving CodeRabbit findings on PR #4841: - test_support: cancel_after_capability_batch is now consume-once (was re-firing every batch). - product_adapters: RunStatus rejects `retryable: Some` for non-failed statuses (failed-only wire invariant) + tests. - turns: retry idempotency no longer persists transient ThreadBusy as a permanent Error replay (mirrors submit path) + regression test. - loop_support: EventPublishingTurnRunTransitionPort delegates latest_resumable_checkpoint (was dropping retry-checkpoint propagation). - reborn/composition: regression tests locking RunNotRetryable -> CheckpointRejected and -> trusted-submit InvalidMaterialization. - lifecycle: doc lists retry among coordinator-origin publications. - webui_v2 tests: retry_run stub fails loud on unqueued call; schema contract asserts the retryable wire field. - retry store contract: temp dirs auto-clean via retained guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
serrrfirat
added a commit
that referenced
this pull request
Jun 13, 2026
The checkpoint WRITE path (checkpoint_state_store_ref) accepted any
parseable checkpoint:{run_id}:{token} ref, so a foreign-run ref could be
staged and then fail to load. Cross-run links remain a read-only
retry-resume affordance in load_checkpoint_payload; the write path now
rejects refs not scoped to the current run with CheckpointRejected.
Regression test added.
Addresses CodeRabbit review on PR #4841.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
serrrfirat
commented
Jun 13, 2026
serrrfirat
left a comment
serrrfirat
left a comment
Collaborator
Author
There was a problem hiding this comment.
Forced code-review-multi pass on draft PR #4841. Actionable findings were fixed directly in 82b0fca.
Fixed findings:
- Security: driver-supplied failure categories are now allowlisted, with unknown values falling back to driver_failed.
- Correctness: cancelled driver failures now use the runner failure transition so CancelRequested runs reach the cancel-or-fail state path.
- Performance: failure-explanation model calls now have a timeout/cancel path, failed-run retry checkpoint selection moved into the store transition, and durable reply verification indexes assistant refs once per history.
- Tests/docs: added regression coverage for explanation degradation, retry source immutability, transition checkpoint preservation, malformed retry IDs, and updated retry docs/routes.
Local verification:
- cargo fmt --all
- cargo test -p ironclaw_agent_loop explanation_prompt_bundle_error_degrades_to_original_failed_exit
- cargo test -p ironclaw_turns --test retry_failed_turn_store_contract
- cargo test -p ironclaw_product_workflow --test reborn_services_contract retry_run_rejects_invalid_run_id_without_turn_facade
- cargo test -p ironclaw_reborn turn_runner::tests::
- cargo test -p ironclaw_reborn loop_exit_applier::tests::thread_checkpoint_evidence
GitHub checks were pending immediately after the push.
serrrfirat
marked this pull request as ready for review
June 14, 2026 11:02
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
crates/ironclaw_turns/src/store.rs (1)
295-307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
RunNotRetryablein persisted retry replays.
RunNotRetryable { run_id }gets flattened into genericConflicthere.Inner::from_persistence_snapshot()later rehydrates retry idempotency throughTurnIdempotencyRecord::replay_retry(), so the same duplicateretry_turnrequest changes shape after a restart/filesystem round-trip. Please persist a retry-specific replay variant, or enough subtype data to rebuildRunNotRetryable, and add a snapshot round-trip regression for it.As per coding guidelines, "Every bug fix must include a regression test using
#[test]or#[tokio::test]."🤖 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/store.rs` around lines 295 - 307, The from_error function in the TurnStore/TurnError conversion is mapping TurnError::RunNotRetryable to the generic Self::Conflict variant, which loses critical retry-specific information during persistence. Create a new Self::RunNotRetryable variant (or similar retry-specific variant) in the persisted error type that captures and preserves the run_id from the original error, then update the from_error match arm to map TurnError::RunNotRetryable { run_id } to this new variant instead of Conflict. Update Inner::from_persistence_snapshot() to properly rehydrate this persisted retry variant back into a TurnError::RunNotRetryable with the correct run_id, ensuring TurnIdempotencyRecord::replay_retry() receives the correct error type after a filesystem round-trip. Finally, add a regression test (using #[test] or #[tokio::test]) that verifies a RunNotRetryable error can be persisted to snapshot and rehydrated back with the same variant and run_id intact.Source: Coding guidelines
crates/ironclaw_agent_loop/src/executor/capabilities.rs (1)
754-778:⚠️ Potential issue | 🟠 MajorCall
attach_failure_explanationfor DriverBug to populate explanation ref.Line 763 manually pushes
LoopFailureKind::DriverBugwithout invoking the explanation provider. Lines 662 and 795 in the same file callattach_failure_explanation(ctx, &mut state, failure_kind).await?and propagate the result intoFailedExitDetails.explanation_message_ref. DriverBug has a deterministic template inFailureExplanationProvider(verified in test evidence). Replace the manual push with a call toattach_failure_explanation, remove the manual push, and use the returnedOption<LoopMessageRef>in theFailedExitDetailsstruct instead ofNone.🤖 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_agent_loop/src/executor/capabilities.rs` around lines 754 - 778, Replace the manual push of LoopFailureKind::DriverBug with a call to attach_failure_explanation(ctx, &mut state, LoopFailureKind::DriverBug).await? to ensure the explanation provider is invoked and returns the appropriate explanation message reference. Remove the state.recent_failure_kinds.push(LoopFailureKind::DriverBug) line and instead capture the Option<LoopMessageRef> returned from attach_failure_explanation, then pass that returned value to the explanation_message_ref field in the FailedExitDetails struct instead of None. This aligns with the pattern already established in lines 662 and 795 of the same file.crates/ironclaw_agent_loop/src/executor/failure_explanation.rs (1)
18-32:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake failure explanation + terminal failure persistence atomic.
attach_failure_explanation()now durably finalizes the assistant message before the caller writes the terminal checkpoint/outcome. If that later write fails, the run keeps a durable explanation message that is absent from the trusted failed-exit evidence, and this repo does not allow deleting LLM data as compensation. Please stage the explanation until the terminal checkpoint succeeds, or move the durable finalize into the same host-owned commit path that records the failed exit. As per coding guidelines, “Multi-step database operations ... MUST be wrapped in a transaction and never assume sequential calls are atomic” and “LLM data is never deleted”.🤖 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_agent_loop/src/executor/failure_explanation.rs` around lines 18 - 32, The attach_failure_explanation() function currently durably finalizes the assistant message from explain_failure() before the terminal checkpoint is written by the caller, creating an atomicity issue where an orphaned explanation message can persist if the checkpoint write fails. Either defer the durable finalization of the explanation message until after the terminal checkpoint succeeds, or refactor so that the explanation finalization and terminal checkpoint are written together in the same transaction/commit path to ensure they remain consistent. The coding guidelines require that multi-step database operations be atomic and LLM data is never deleted, so these operations must not be separated across different persistence boundaries.Source: Coding guidelines
crates/ironclaw_turns/src/loop_exit.rs (1)
146-155:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
resume_checkpoint_idnever gets populated on the production failed-run path.
validate_failed_exit()now reads the retry checkpoint exclusively fromLoopExitValidationPolicy.failure_resume_checkpoint_id, butderive_policy()initializes that field toNoneand never assigns it in theLoopExit::Failedbranch. The only setter is test-only, so every trustedTurnRunnerOutcome::Failedcurrently shipsresume_checkpoint_id: Noneand the retry path never becomes available. Please derive the resume checkpoint in the host path here and lock it with a regression throughLoopExitApplier::apply. Based on learnings from the PR objectives, failed runs are supposed to be “retryable from the last good checkpoint when one exists”. As per coding guidelines, “Test through the caller”.Also applies to: 205-224, 850-859
🤖 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/loop_exit.rs` around lines 146 - 155, The failure_resume_checkpoint_id field in LoopExitValidationPolicy is initialized to None in derive_policy() at lines 146-155 and never populated in the LoopExit::Failed branch, causing the retry path to be unavailable in production. Extract the resume checkpoint ID from the failed loop exit data and assign it to failure_resume_checkpoint_id in the LoopExit::Failed handling within derive_policy(). Additionally, ensure the same fix is applied at lines 205-224 and 850-859 where similar initialization or assignment occurs. Add a regression test through LoopExitApplier::apply (the caller of derive_policy()) that verifies a failed run with an available checkpoint correctly populates resume_checkpoint_id in the resulting TurnRunnerOutcome.Source: Coding guidelines
crates/ironclaw_turns/src/loop_exit/tests/mod.rs (2)
77-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
failure_resume_checkpoint_idto the wire-forgery matrix.
failure_resume_checkpoint_idis new host-verified state, but this deserialization test still omits it. A serde regression could now mint retry checkpoints from untrusted wire data without tripping the fail-closed coverage here.As per coding guidelines, "Fail closed for auth, approvals, trust..." and "Every bug fix must include a regression test using #[test] or #[tokio::test] that reproduces the original failure."
🤖 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/loop_exit/tests/mod.rs` around lines 77 - 128, The test `loop_exit_validation_policy_deserialization_cannot_mint_host_verified_evidence` is missing `failure_resume_checkpoint_id` from its wire-forgery validation matrix. Add `"failure_resume_checkpoint_id"` to the `trusted_field` array at the start of the function, and include `"failure_resume_checkpoint_id": false` in all three JSON objects (`forged`, `forged_terminal`, and `strict_fail_closed`) to ensure the deserialization test properly prevents this host-verified field from being mintable from untrusted wire data.Source: Coding guidelines
590-631:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis strict-final test never exercises the resume-checkpoint path.
Both branches set
failure_resume_checkpoint_id: None, so the bug this test name describes would still pass if broken. Seed a real resume checkpoint id here and assert it is withheld beforefinal_checkpoint_verifiedand surfaced after it.As per coding guidelines, "Every bug fix must include a regression test using #[test] or #[tokio::test] that reproduces the original failure."
🤖 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/loop_exit/tests/mod.rs` around lines 590 - 631, The test `strict_final_checkpoint_policy_admits_failed_resume_checkpoint_only_after_verification` never actually tests the resume checkpoint path because both validation calls set `failure_resume_checkpoint_id: None`. To fix this, create a real resume checkpoint ID (similar to how `checkpoint_id` is created with `TurnCheckpointId::new()`), then update the first rejection branch to pass this resume checkpoint ID and verify that it is withheld from the resulting `TurnRunnerOutcome::Failed` when `final_checkpoint_verified` is false. Update the second accepted branch to also pass this same resume checkpoint ID and verify that it appears in the `resume_checkpoint_id` field of the resulting `TurnRunnerOutcome::Failed` when `final_checkpoint_verified` is true. This creates a proper regression test that exercises both the withholding and surfacing of the resume checkpoint based on verification status.Source: Coding guidelines
crates/ironclaw_turns/src/status.rs (1)
204-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve backward compatibility for persisted failure categories.
Line 220 now rejects
:, and Line 204 deserializes throughSanitizedFailure::new; that can make historical rows with legacy colon-delimited categories unreadable after upgrade.Suggested compatibility-only read-path fix
impl<'de> Deserialize<'de> for SanitizedFailure { fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: serde::Deserializer<'de>, { #[derive(Deserialize)] struct WireFailure { category: String, } let wire = WireFailure::deserialize(deserializer)?; - Self::new(wire.category).map_err(serde::de::Error::custom) + let normalized = if wire.category.contains(':') { + wire.category.replace(':', "_") + } else { + wire.category + }; + Self::new(normalized).map_err(serde::de::Error::custom) } }Also applies to: 220-224
🤖 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/status.rs` around lines 204 - 205, The deserialization path at line 204 in the `deserialize` implementation calls `SanitizedFailure::new(wire.category)` which enforces strict validation that rejects colons, but historically persisted data may contain colon-delimited categories. Modify the deserialization logic to detect and handle legacy colon-delimited category formats before passing them to `SanitizedFailure::new`, either by converting them to the new format or providing a compatibility layer that normalizes the input during reads while keeping the write path (lines 220-224) strict about rejecting colons. This ensures old persisted rows remain readable after upgrade without loosening validation on new writes.
🤖 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_agent_loop/src/state.rs`:
- Around line 193-197: The rebase_for_run function clears input_cursor,
assistant_refs, and result_refs but leaves stale gate-bound resume state fields
(last_gate, pending_approval_resume, and pending_auth_resume) from the source
run intact, which can leak into the new retry run and violate fail-closed gate
evidence semantics. In the rebase_for_run method, also clear the last_gate,
pending_approval_resume, and pending_auth_resume fields in addition to the
fields already being cleared, to ensure that gate-bound resume state does not
leak across run boundaries when crossing trust boundaries.
In `@crates/ironclaw_product_workflow/tests/reborn_services_contract.rs`:
- Around line 8193-8219: The RecordingLander::land method currently ignores the
_thread_scope parameter (indicated by the underscore prefix). To comply with the
coding guideline for mocking multi-arg runtime APIs, modify the mock to capture
thread_scope in the self.landed state storage along with the existing message_id
and attachments. Update the data structure that stores the landed information to
include the thread_scope argument so that all production inputs are captured and
can be validated in the contract test.
In
`@crates/ironclaw_reborn_composition/src/projection/tests/failure_explanation.rs`:
- Around line 252-338: The test
`failure_summary_covers_agent_loop_safe_summary_categories` uses a hardcoded
list of expected categories without verifying it against the agent-loop source,
so new categories added to the source will silently pass this test instead of
failing loudly. Implement a parity guard similar to the
`include_str!`/`BTreeSet` approach mentioned in the review comment to verify
that the expected categories list is exhaustive and matches all sanitized
safe-summary categories from the agent-loop source. This way, when new
categories are added to the source, the test will fail until the expected list
is updated.
In `@crates/ironclaw_turns/src/memory.rs`:
- Around line 2502-2506: In the `fail_claimed_record()` method, the
`resume_checkpoint_id` parameter is being trusted without validation, which can
allow invalid checkpoints (final or foreign-run) to be marked as retryable.
Validate the explicit `resume_checkpoint_id` through the
`retryable_loop_checkpoint()` method first before using it; only if that returns
None should you fall back to calling `latest_resumable_loop_checkpoint()`.
Additionally, add a regression test using `#[tokio::test]` to verify that
passing an invalid checkpoint to `fail_claimed_record()` does not incorrectly
mark the run as retryable.
In `@crates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rs`:
- Around line 286-287: The test uses hardcoded array indices (items[1] and
items[2]) to access run_status values, which makes it brittle to fixture
reordering unrelated to schema changes. Instead of relying on indices, refactor
the code to find the running and failed status entries by filtering the items
array based on their "status" field values. Replace the direct index-based
assignments with filtered lookups that match on the appropriate status value,
ensuring the test validates the schema contract without coupling to fixture
order.
---
Outside diff comments:
In `@crates/ironclaw_agent_loop/src/executor/capabilities.rs`:
- Around line 754-778: Replace the manual push of LoopFailureKind::DriverBug
with a call to attach_failure_explanation(ctx, &mut state,
LoopFailureKind::DriverBug).await? to ensure the explanation provider is invoked
and returns the appropriate explanation message reference. Remove the
state.recent_failure_kinds.push(LoopFailureKind::DriverBug) line and instead
capture the Option<LoopMessageRef> returned from attach_failure_explanation,
then pass that returned value to the explanation_message_ref field in the
FailedExitDetails struct instead of None. This aligns with the pattern already
established in lines 662 and 795 of the same file.
In `@crates/ironclaw_agent_loop/src/executor/failure_explanation.rs`:
- Around line 18-32: The attach_failure_explanation() function currently durably
finalizes the assistant message from explain_failure() before the terminal
checkpoint is written by the caller, creating an atomicity issue where an
orphaned explanation message can persist if the checkpoint write fails. Either
defer the durable finalization of the explanation message until after the
terminal checkpoint succeeds, or refactor so that the explanation finalization
and terminal checkpoint are written together in the same transaction/commit path
to ensure they remain consistent. The coding guidelines require that multi-step
database operations be atomic and LLM data is never deleted, so these operations
must not be separated across different persistence boundaries.
In `@crates/ironclaw_turns/src/loop_exit.rs`:
- Around line 146-155: The failure_resume_checkpoint_id field in
LoopExitValidationPolicy is initialized to None in derive_policy() at lines
146-155 and never populated in the LoopExit::Failed branch, causing the retry
path to be unavailable in production. Extract the resume checkpoint ID from the
failed loop exit data and assign it to failure_resume_checkpoint_id in the
LoopExit::Failed handling within derive_policy(). Additionally, ensure the same
fix is applied at lines 205-224 and 850-859 where similar initialization or
assignment occurs. Add a regression test through LoopExitApplier::apply (the
caller of derive_policy()) that verifies a failed run with an available
checkpoint correctly populates resume_checkpoint_id in the resulting
TurnRunnerOutcome.
In `@crates/ironclaw_turns/src/loop_exit/tests/mod.rs`:
- Around line 77-128: The test
`loop_exit_validation_policy_deserialization_cannot_mint_host_verified_evidence`
is missing `failure_resume_checkpoint_id` from its wire-forgery validation
matrix. Add `"failure_resume_checkpoint_id"` to the `trusted_field` array at the
start of the function, and include `"failure_resume_checkpoint_id": false` in
all three JSON objects (`forged`, `forged_terminal`, and `strict_fail_closed`)
to ensure the deserialization test properly prevents this host-verified field
from being mintable from untrusted wire data.
- Around line 590-631: The test
`strict_final_checkpoint_policy_admits_failed_resume_checkpoint_only_after_verification`
never actually tests the resume checkpoint path because both validation calls
set `failure_resume_checkpoint_id: None`. To fix this, create a real resume
checkpoint ID (similar to how `checkpoint_id` is created with
`TurnCheckpointId::new()`), then update the first rejection branch to pass this
resume checkpoint ID and verify that it is withheld from the resulting
`TurnRunnerOutcome::Failed` when `final_checkpoint_verified` is false. Update
the second accepted branch to also pass this same resume checkpoint ID and
verify that it appears in the `resume_checkpoint_id` field of the resulting
`TurnRunnerOutcome::Failed` when `final_checkpoint_verified` is true. This
creates a proper regression test that exercises both the withholding and
surfacing of the resume checkpoint based on verification status.
In `@crates/ironclaw_turns/src/status.rs`:
- Around line 204-205: The deserialization path at line 204 in the `deserialize`
implementation calls `SanitizedFailure::new(wire.category)` which enforces
strict validation that rejects colons, but historically persisted data may
contain colon-delimited categories. Modify the deserialization logic to detect
and handle legacy colon-delimited category formats before passing them to
`SanitizedFailure::new`, either by converting them to the new format or
providing a compatibility layer that normalizes the input during reads while
keeping the write path (lines 220-224) strict about rejecting colons. This
ensures old persisted rows remain readable after upgrade without loosening
validation on new writes.
In `@crates/ironclaw_turns/src/store.rs`:
- Around line 295-307: The from_error function in the TurnStore/TurnError
conversion is mapping TurnError::RunNotRetryable to the generic Self::Conflict
variant, which loses critical retry-specific information during persistence.
Create a new Self::RunNotRetryable variant (or similar retry-specific variant)
in the persisted error type that captures and preserves the run_id from the
original error, then update the from_error match arm to map
TurnError::RunNotRetryable { run_id } to this new variant instead of Conflict.
Update Inner::from_persistence_snapshot() to properly rehydrate this persisted
retry variant back into a TurnError::RunNotRetryable with the correct run_id,
ensuring TurnIdempotencyRecord::replay_retry() receives the correct error type
after a filesystem round-trip. Finally, add a regression test (using #[test] or
#[tokio::test]) that verifies a RunNotRetryable error can be persisted to
snapshot and rehydrated back with the same variant and run_id intact.
🪄 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: c0fcffdd-3cba-4ff7-bb26-c01b646116e6
📒 Files selected for processing (56)
crates/ironclaw_agent_loop/src/executor/budget.rscrates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/failure_explanation.rscrates/ironclaw_agent_loop/src/executor/loop_exit.rscrates/ironclaw_agent_loop/src/executor/model.rscrates/ironclaw_agent_loop/src/executor/prompt.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/executor/tests/support/compaction.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_agent_loop/src/test_support/mod.rscrates/ironclaw_agent_loop/tests/safety_nets.rscrates/ironclaw_conversations/src/inbound.rscrates/ironclaw_conversations/tests/inbound_contract.rscrates/ironclaw_loop_support/src/subagent_spawn_port/tests.rscrates/ironclaw_loop_support/src/turn_event_publisher.rscrates/ironclaw_product_adapters/src/outbound.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/webui_inbound.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_product_workflow/tests/webui_inbound_contract.rscrates/ironclaw_reborn/src/failure_categories.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_driver_host/port_adapters.rscrates/ironclaw_reborn/src/loop_driver_host/tests.rscrates/ironclaw_reborn/src/loop_exit_applier.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/mod.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/support.rscrates/ironclaw_reborn/src/planned_driver.rscrates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn/src/turn_runner.rscrates/ironclaw_reborn/src/turn_runner/tests/mod.rscrates/ironclaw_reborn_composition/src/factory/auth_tests.rscrates/ironclaw_reborn_composition/src/failure_summary.rscrates/ironclaw_reborn_composition/src/projection/tests/failure_explanation.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rscrates/ironclaw_turns/src/coordinator.rscrates/ironclaw_turns/src/lifecycle.rscrates/ironclaw_turns/src/loop_exit.rscrates/ironclaw_turns/src/loop_exit/tests/mod.rscrates/ironclaw_turns/src/memory.rscrates/ironclaw_turns/src/run_profile/host.rscrates/ironclaw_turns/src/run_profile/prompt.rscrates/ironclaw_turns/src/status.rscrates/ironclaw_turns/src/store.rscrates/ironclaw_turns/tests/retry_failed_turn_store_contract.rscrates/ironclaw_webui_v2/CLAUDE.mdcrates/ironclaw_webui_v2/src/descriptors.rscrates/ironclaw_webui_v2/src/descriptors/run_action_descriptors.rscrates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rsdocs/reborn/contracts/turn-runner.mdtests/reborn_trace_error_path_parity.rstests/support_unit_tests.rs
💤 Files with no reviewable changes (1)
- crates/ironclaw_reborn_composition/src/slack_delivery.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
crates/ironclaw_turns/src/store.rs (1)
295-307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
RunNotRetryablein persisted retry replays.
RunNotRetryable { run_id }gets flattened into genericConflicthere.Inner::from_persistence_snapshot()later rehydrates retry idempotency throughTurnIdempotencyRecord::replay_retry(), so the same duplicateretry_turnrequest changes shape after a restart/filesystem round-trip. Please persist a retry-specific replay variant, or enough subtype data to rebuildRunNotRetryable, and add a snapshot round-trip regression for it.As per coding guidelines, "Every bug fix must include a regression test using
#[test]or#[tokio::test]."🤖 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/store.rs` around lines 295 - 307, The from_error function in the TurnStore/TurnError conversion is mapping TurnError::RunNotRetryable to the generic Self::Conflict variant, which loses critical retry-specific information during persistence. Create a new Self::RunNotRetryable variant (or similar retry-specific variant) in the persisted error type that captures and preserves the run_id from the original error, then update the from_error match arm to map TurnError::RunNotRetryable { run_id } to this new variant instead of Conflict. Update Inner::from_persistence_snapshot() to properly rehydrate this persisted retry variant back into a TurnError::RunNotRetryable with the correct run_id, ensuring TurnIdempotencyRecord::replay_retry() receives the correct error type after a filesystem round-trip. Finally, add a regression test (using #[test] or #[tokio::test]) that verifies a RunNotRetryable error can be persisted to snapshot and rehydrated back with the same variant and run_id intact.Source: Coding guidelines
crates/ironclaw_agent_loop/src/executor/capabilities.rs (1)
754-778:⚠️ Potential issue | 🟠 MajorCall
attach_failure_explanationfor DriverBug to populate explanation ref.Line 763 manually pushes
LoopFailureKind::DriverBugwithout invoking the explanation provider. Lines 662 and 795 in the same file callattach_failure_explanation(ctx, &mut state, failure_kind).await?and propagate the result intoFailedExitDetails.explanation_message_ref. DriverBug has a deterministic template inFailureExplanationProvider(verified in test evidence). Replace the manual push with a call toattach_failure_explanation, remove the manual push, and use the returnedOption<LoopMessageRef>in theFailedExitDetailsstruct instead ofNone.🤖 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_agent_loop/src/executor/capabilities.rs` around lines 754 - 778, Replace the manual push of LoopFailureKind::DriverBug with a call to attach_failure_explanation(ctx, &mut state, LoopFailureKind::DriverBug).await? to ensure the explanation provider is invoked and returns the appropriate explanation message reference. Remove the state.recent_failure_kinds.push(LoopFailureKind::DriverBug) line and instead capture the Option<LoopMessageRef> returned from attach_failure_explanation, then pass that returned value to the explanation_message_ref field in the FailedExitDetails struct instead of None. This aligns with the pattern already established in lines 662 and 795 of the same file.crates/ironclaw_agent_loop/src/executor/failure_explanation.rs (1)
18-32:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake failure explanation + terminal failure persistence atomic.
attach_failure_explanation()now durably finalizes the assistant message before the caller writes the terminal checkpoint/outcome. If that later write fails, the run keeps a durable explanation message that is absent from the trusted failed-exit evidence, and this repo does not allow deleting LLM data as compensation. Please stage the explanation until the terminal checkpoint succeeds, or move the durable finalize into the same host-owned commit path that records the failed exit. As per coding guidelines, “Multi-step database operations ... MUST be wrapped in a transaction and never assume sequential calls are atomic” and “LLM data is never deleted”.🤖 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_agent_loop/src/executor/failure_explanation.rs` around lines 18 - 32, The attach_failure_explanation() function currently durably finalizes the assistant message from explain_failure() before the terminal checkpoint is written by the caller, creating an atomicity issue where an orphaned explanation message can persist if the checkpoint write fails. Either defer the durable finalization of the explanation message until after the terminal checkpoint succeeds, or refactor so that the explanation finalization and terminal checkpoint are written together in the same transaction/commit path to ensure they remain consistent. The coding guidelines require that multi-step database operations be atomic and LLM data is never deleted, so these operations must not be separated across different persistence boundaries.Source: Coding guidelines
crates/ironclaw_turns/src/loop_exit.rs (1)
146-155:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
resume_checkpoint_idnever gets populated on the production failed-run path.
validate_failed_exit()now reads the retry checkpoint exclusively fromLoopExitValidationPolicy.failure_resume_checkpoint_id, butderive_policy()initializes that field toNoneand never assigns it in theLoopExit::Failedbranch. The only setter is test-only, so every trustedTurnRunnerOutcome::Failedcurrently shipsresume_checkpoint_id: Noneand the retry path never becomes available. Please derive the resume checkpoint in the host path here and lock it with a regression throughLoopExitApplier::apply. Based on learnings from the PR objectives, failed runs are supposed to be “retryable from the last good checkpoint when one exists”. As per coding guidelines, “Test through the caller”.Also applies to: 205-224, 850-859
🤖 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/loop_exit.rs` around lines 146 - 155, The failure_resume_checkpoint_id field in LoopExitValidationPolicy is initialized to None in derive_policy() at lines 146-155 and never populated in the LoopExit::Failed branch, causing the retry path to be unavailable in production. Extract the resume checkpoint ID from the failed loop exit data and assign it to failure_resume_checkpoint_id in the LoopExit::Failed handling within derive_policy(). Additionally, ensure the same fix is applied at lines 205-224 and 850-859 where similar initialization or assignment occurs. Add a regression test through LoopExitApplier::apply (the caller of derive_policy()) that verifies a failed run with an available checkpoint correctly populates resume_checkpoint_id in the resulting TurnRunnerOutcome.Source: Coding guidelines
crates/ironclaw_turns/src/loop_exit/tests/mod.rs (2)
77-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
failure_resume_checkpoint_idto the wire-forgery matrix.
failure_resume_checkpoint_idis new host-verified state, but this deserialization test still omits it. A serde regression could now mint retry checkpoints from untrusted wire data without tripping the fail-closed coverage here.As per coding guidelines, "Fail closed for auth, approvals, trust..." and "Every bug fix must include a regression test using #[test] or #[tokio::test] that reproduces the original failure."
🤖 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/loop_exit/tests/mod.rs` around lines 77 - 128, The test `loop_exit_validation_policy_deserialization_cannot_mint_host_verified_evidence` is missing `failure_resume_checkpoint_id` from its wire-forgery validation matrix. Add `"failure_resume_checkpoint_id"` to the `trusted_field` array at the start of the function, and include `"failure_resume_checkpoint_id": false` in all three JSON objects (`forged`, `forged_terminal`, and `strict_fail_closed`) to ensure the deserialization test properly prevents this host-verified field from being mintable from untrusted wire data.Source: Coding guidelines
590-631:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis strict-final test never exercises the resume-checkpoint path.
Both branches set
failure_resume_checkpoint_id: None, so the bug this test name describes would still pass if broken. Seed a real resume checkpoint id here and assert it is withheld beforefinal_checkpoint_verifiedand surfaced after it.As per coding guidelines, "Every bug fix must include a regression test using #[test] or #[tokio::test] that reproduces the original failure."
🤖 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/loop_exit/tests/mod.rs` around lines 590 - 631, The test `strict_final_checkpoint_policy_admits_failed_resume_checkpoint_only_after_verification` never actually tests the resume checkpoint path because both validation calls set `failure_resume_checkpoint_id: None`. To fix this, create a real resume checkpoint ID (similar to how `checkpoint_id` is created with `TurnCheckpointId::new()`), then update the first rejection branch to pass this resume checkpoint ID and verify that it is withheld from the resulting `TurnRunnerOutcome::Failed` when `final_checkpoint_verified` is false. Update the second accepted branch to also pass this same resume checkpoint ID and verify that it appears in the `resume_checkpoint_id` field of the resulting `TurnRunnerOutcome::Failed` when `final_checkpoint_verified` is true. This creates a proper regression test that exercises both the withholding and surfacing of the resume checkpoint based on verification status.Source: Coding guidelines
crates/ironclaw_turns/src/status.rs (1)
204-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve backward compatibility for persisted failure categories.
Line 220 now rejects
:, and Line 204 deserializes throughSanitizedFailure::new; that can make historical rows with legacy colon-delimited categories unreadable after upgrade.Suggested compatibility-only read-path fix
impl<'de> Deserialize<'de> for SanitizedFailure { fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: serde::Deserializer<'de>, { #[derive(Deserialize)] struct WireFailure { category: String, } let wire = WireFailure::deserialize(deserializer)?; - Self::new(wire.category).map_err(serde::de::Error::custom) + let normalized = if wire.category.contains(':') { + wire.category.replace(':', "_") + } else { + wire.category + }; + Self::new(normalized).map_err(serde::de::Error::custom) } }Also applies to: 220-224
🤖 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/status.rs` around lines 204 - 205, The deserialization path at line 204 in the `deserialize` implementation calls `SanitizedFailure::new(wire.category)` which enforces strict validation that rejects colons, but historically persisted data may contain colon-delimited categories. Modify the deserialization logic to detect and handle legacy colon-delimited category formats before passing them to `SanitizedFailure::new`, either by converting them to the new format or providing a compatibility layer that normalizes the input during reads while keeping the write path (lines 220-224) strict about rejecting colons. This ensures old persisted rows remain readable after upgrade without loosening validation on new writes.
🤖 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_agent_loop/src/state.rs`:
- Around line 193-197: The rebase_for_run function clears input_cursor,
assistant_refs, and result_refs but leaves stale gate-bound resume state fields
(last_gate, pending_approval_resume, and pending_auth_resume) from the source
run intact, which can leak into the new retry run and violate fail-closed gate
evidence semantics. In the rebase_for_run method, also clear the last_gate,
pending_approval_resume, and pending_auth_resume fields in addition to the
fields already being cleared, to ensure that gate-bound resume state does not
leak across run boundaries when crossing trust boundaries.
In `@crates/ironclaw_product_workflow/tests/reborn_services_contract.rs`:
- Around line 8193-8219: The RecordingLander::land method currently ignores the
_thread_scope parameter (indicated by the underscore prefix). To comply with the
coding guideline for mocking multi-arg runtime APIs, modify the mock to capture
thread_scope in the self.landed state storage along with the existing message_id
and attachments. Update the data structure that stores the landed information to
include the thread_scope argument so that all production inputs are captured and
can be validated in the contract test.
In
`@crates/ironclaw_reborn_composition/src/projection/tests/failure_explanation.rs`:
- Around line 252-338: The test
`failure_summary_covers_agent_loop_safe_summary_categories` uses a hardcoded
list of expected categories without verifying it against the agent-loop source,
so new categories added to the source will silently pass this test instead of
failing loudly. Implement a parity guard similar to the
`include_str!`/`BTreeSet` approach mentioned in the review comment to verify
that the expected categories list is exhaustive and matches all sanitized
safe-summary categories from the agent-loop source. This way, when new
categories are added to the source, the test will fail until the expected list
is updated.
In `@crates/ironclaw_turns/src/memory.rs`:
- Around line 2502-2506: In the `fail_claimed_record()` method, the
`resume_checkpoint_id` parameter is being trusted without validation, which can
allow invalid checkpoints (final or foreign-run) to be marked as retryable.
Validate the explicit `resume_checkpoint_id` through the
`retryable_loop_checkpoint()` method first before using it; only if that returns
None should you fall back to calling `latest_resumable_loop_checkpoint()`.
Additionally, add a regression test using `#[tokio::test]` to verify that
passing an invalid checkpoint to `fail_claimed_record()` does not incorrectly
mark the run as retryable.
In `@crates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rs`:
- Around line 286-287: The test uses hardcoded array indices (items[1] and
items[2]) to access run_status values, which makes it brittle to fixture
reordering unrelated to schema changes. Instead of relying on indices, refactor
the code to find the running and failed status entries by filtering the items
array based on their "status" field values. Replace the direct index-based
assignments with filtered lookups that match on the appropriate status value,
ensuring the test validates the schema contract without coupling to fixture
order.
---
Outside diff comments:
In `@crates/ironclaw_agent_loop/src/executor/capabilities.rs`:
- Around line 754-778: Replace the manual push of LoopFailureKind::DriverBug
with a call to attach_failure_explanation(ctx, &mut state,
LoopFailureKind::DriverBug).await? to ensure the explanation provider is invoked
and returns the appropriate explanation message reference. Remove the
state.recent_failure_kinds.push(LoopFailureKind::DriverBug) line and instead
capture the Option<LoopMessageRef> returned from attach_failure_explanation,
then pass that returned value to the explanation_message_ref field in the
FailedExitDetails struct instead of None. This aligns with the pattern already
established in lines 662 and 795 of the same file.
In `@crates/ironclaw_agent_loop/src/executor/failure_explanation.rs`:
- Around line 18-32: The attach_failure_explanation() function currently durably
finalizes the assistant message from explain_failure() before the terminal
checkpoint is written by the caller, creating an atomicity issue where an
orphaned explanation message can persist if the checkpoint write fails. Either
defer the durable finalization of the explanation message until after the
terminal checkpoint succeeds, or refactor so that the explanation finalization
and terminal checkpoint are written together in the same transaction/commit path
to ensure they remain consistent. The coding guidelines require that multi-step
database operations be atomic and LLM data is never deleted, so these operations
must not be separated across different persistence boundaries.
In `@crates/ironclaw_turns/src/loop_exit.rs`:
- Around line 146-155: The failure_resume_checkpoint_id field in
LoopExitValidationPolicy is initialized to None in derive_policy() at lines
146-155 and never populated in the LoopExit::Failed branch, causing the retry
path to be unavailable in production. Extract the resume checkpoint ID from the
failed loop exit data and assign it to failure_resume_checkpoint_id in the
LoopExit::Failed handling within derive_policy(). Additionally, ensure the same
fix is applied at lines 205-224 and 850-859 where similar initialization or
assignment occurs. Add a regression test through LoopExitApplier::apply (the
caller of derive_policy()) that verifies a failed run with an available
checkpoint correctly populates resume_checkpoint_id in the resulting
TurnRunnerOutcome.
In `@crates/ironclaw_turns/src/loop_exit/tests/mod.rs`:
- Around line 77-128: The test
`loop_exit_validation_policy_deserialization_cannot_mint_host_verified_evidence`
is missing `failure_resume_checkpoint_id` from its wire-forgery validation
matrix. Add `"failure_resume_checkpoint_id"` to the `trusted_field` array at the
start of the function, and include `"failure_resume_checkpoint_id": false` in
all three JSON objects (`forged`, `forged_terminal`, and `strict_fail_closed`)
to ensure the deserialization test properly prevents this host-verified field
from being mintable from untrusted wire data.
- Around line 590-631: The test
`strict_final_checkpoint_policy_admits_failed_resume_checkpoint_only_after_verification`
never actually tests the resume checkpoint path because both validation calls
set `failure_resume_checkpoint_id: None`. To fix this, create a real resume
checkpoint ID (similar to how `checkpoint_id` is created with
`TurnCheckpointId::new()`), then update the first rejection branch to pass this
resume checkpoint ID and verify that it is withheld from the resulting
`TurnRunnerOutcome::Failed` when `final_checkpoint_verified` is false. Update
the second accepted branch to also pass this same resume checkpoint ID and
verify that it appears in the `resume_checkpoint_id` field of the resulting
`TurnRunnerOutcome::Failed` when `final_checkpoint_verified` is true. This
creates a proper regression test that exercises both the withholding and
surfacing of the resume checkpoint based on verification status.
In `@crates/ironclaw_turns/src/status.rs`:
- Around line 204-205: The deserialization path at line 204 in the `deserialize`
implementation calls `SanitizedFailure::new(wire.category)` which enforces
strict validation that rejects colons, but historically persisted data may
contain colon-delimited categories. Modify the deserialization logic to detect
and handle legacy colon-delimited category formats before passing them to
`SanitizedFailure::new`, either by converting them to the new format or
providing a compatibility layer that normalizes the input during reads while
keeping the write path (lines 220-224) strict about rejecting colons. This
ensures old persisted rows remain readable after upgrade without loosening
validation on new writes.
In `@crates/ironclaw_turns/src/store.rs`:
- Around line 295-307: The from_error function in the TurnStore/TurnError
conversion is mapping TurnError::RunNotRetryable to the generic Self::Conflict
variant, which loses critical retry-specific information during persistence.
Create a new Self::RunNotRetryable variant (or similar retry-specific variant)
in the persisted error type that captures and preserves the run_id from the
original error, then update the from_error match arm to map
TurnError::RunNotRetryable { run_id } to this new variant instead of Conflict.
Update Inner::from_persistence_snapshot() to properly rehydrate this persisted
retry variant back into a TurnError::RunNotRetryable with the correct run_id,
ensuring TurnIdempotencyRecord::replay_retry() receives the correct error type
after a filesystem round-trip. Finally, add a regression test (using #[test] or
#[tokio::test]) that verifies a RunNotRetryable error can be persisted to
snapshot and rehydrated back with the same variant and run_id intact.
🪄 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: c0fcffdd-3cba-4ff7-bb26-c01b646116e6
📒 Files selected for processing (56)
crates/ironclaw_agent_loop/src/executor/budget.rscrates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/failure_explanation.rscrates/ironclaw_agent_loop/src/executor/loop_exit.rscrates/ironclaw_agent_loop/src/executor/model.rscrates/ironclaw_agent_loop/src/executor/prompt.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/executor/tests/support/compaction.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_agent_loop/src/test_support/mod.rscrates/ironclaw_agent_loop/tests/safety_nets.rscrates/ironclaw_conversations/src/inbound.rscrates/ironclaw_conversations/tests/inbound_contract.rscrates/ironclaw_loop_support/src/subagent_spawn_port/tests.rscrates/ironclaw_loop_support/src/turn_event_publisher.rscrates/ironclaw_product_adapters/src/outbound.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/webui_inbound.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_product_workflow/tests/webui_inbound_contract.rscrates/ironclaw_reborn/src/failure_categories.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_driver_host/port_adapters.rscrates/ironclaw_reborn/src/loop_driver_host/tests.rscrates/ironclaw_reborn/src/loop_exit_applier.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/mod.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/support.rscrates/ironclaw_reborn/src/planned_driver.rscrates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn/src/turn_runner.rscrates/ironclaw_reborn/src/turn_runner/tests/mod.rscrates/ironclaw_reborn_composition/src/factory/auth_tests.rscrates/ironclaw_reborn_composition/src/failure_summary.rscrates/ironclaw_reborn_composition/src/projection/tests/failure_explanation.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rscrates/ironclaw_turns/src/coordinator.rscrates/ironclaw_turns/src/lifecycle.rscrates/ironclaw_turns/src/loop_exit.rscrates/ironclaw_turns/src/loop_exit/tests/mod.rscrates/ironclaw_turns/src/memory.rscrates/ironclaw_turns/src/run_profile/host.rscrates/ironclaw_turns/src/run_profile/prompt.rscrates/ironclaw_turns/src/status.rscrates/ironclaw_turns/src/store.rscrates/ironclaw_turns/tests/retry_failed_turn_store_contract.rscrates/ironclaw_webui_v2/CLAUDE.mdcrates/ironclaw_webui_v2/src/descriptors.rscrates/ironclaw_webui_v2/src/descriptors/run_action_descriptors.rscrates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rsdocs/reborn/contracts/turn-runner.mdtests/reborn_trace_error_path_parity.rstests/support_unit_tests.rs
💤 Files with no reviewable changes (1)
- crates/ironclaw_reborn_composition/src/slack_delivery.rs
🛑 Comments failed to post (5)
crates/ironclaw_agent_loop/src/state.rs (1)
193-197:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear gate-bound resume state during retry rebase.
Line 193 resets cursor/transcript refs but leaves
last_gate,pending_approval_resume, andpending_auth_resumeintact. Because retry resume acceptsBeforeBlockcheckpoints, stale source-run gate refs/tokens can leak into the new run and break fail-closed gate evidence semantics.Proposed fix
pub fn rebase_for_run(mut self, context: &LoopRunContext) -> Self { self.input_cursor = LoopInputCursor::origin_for_run(context); self.assistant_refs.clear(); self.result_refs.clear(); + self.last_gate = None; + self.pending_approval_resume = None; + self.pending_auth_resume = None; self }As per coding guidelines: "Preserve tenant/user/agent/project/mission/thread scope..." and "Fail closed for auth, approvals..." when crossing trust/state boundaries.
🤖 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_agent_loop/src/state.rs` around lines 193 - 197, The rebase_for_run function clears input_cursor, assistant_refs, and result_refs but leaves stale gate-bound resume state fields (last_gate, pending_approval_resume, and pending_auth_resume) from the source run intact, which can leak into the new retry run and violate fail-closed gate evidence semantics. In the rebase_for_run method, also clear the last_gate, pending_approval_resume, and pending_auth_resume fields in addition to the fields already being cleared, to ensure that gate-bound resume state does not leak across run boundaries when crossing trust boundaries.Source: Coding guidelines
crates/ironclaw_product_workflow/tests/reborn_services_contract.rs (1)
8193-8219:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCapture
thread_scopeinRecordingLandermock state.
RecordingLander::landignores_thread_scope, so this multi-arg runtime mock is not capturing all production inputs. That weakens this contract test’s ability to catch scope regressions.Suggested patch
#[derive(Default)] struct RecordingLander { - landed: Mutex<Vec<(String, Vec<InboundAttachment>)>>, + landed: Mutex<Vec<(ThreadScope, String, Vec<InboundAttachment>)>>, } #[async_trait] impl InboundAttachmentLander for RecordingLander { async fn land( &self, - _thread_scope: &ThreadScope, + thread_scope: &ThreadScope, message_id: &str, attachments: Vec<InboundAttachment>, ) -> Result<Vec<AttachmentRef>, RebornServicesError> { @@ self.landed .lock() .expect("lander mutex") - .push((message_id.to_string(), attachments)); + .push((thread_scope.clone(), message_id.to_string(), attachments)); Ok(refs) } }As per coding guidelines, "When mocking a multi-arg runtime API, the mock must capture every argument the production caller passes."
Also applies to: 8253-8261
🤖 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_product_workflow/tests/reborn_services_contract.rs` around lines 8193 - 8219, The RecordingLander::land method currently ignores the _thread_scope parameter (indicated by the underscore prefix). To comply with the coding guideline for mocking multi-arg runtime APIs, modify the mock to capture thread_scope in the self.landed state storage along with the existing message_id and attachments. Update the data structure that stores the landed information to include the thread_scope argument so that all production inputs are captured and can be validated in the contract test.Source: Coding guidelines
crates/ironclaw_reborn_composition/src/projection/tests/failure_explanation.rs (1)
252-338:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis completeness test can silently miss new agent-loop categories.
Unlike the two source-driven coverage checks above, this table never verifies its
expectedset against the owning agent-loop source. Ifironclaw_agent_loopadds another sanitized safe-summary category, this test stays green and the new path falls through toGENERIC_FAILURE_SUMMARY. Mirror theinclude_str!/BTreeSetparity guard here so category additions fail loudly.As per coding guidelines, "Fail loud: flag silent-failure patterns …".
🤖 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_reborn_composition/src/projection/tests/failure_explanation.rs` around lines 252 - 338, The test `failure_summary_covers_agent_loop_safe_summary_categories` uses a hardcoded list of expected categories without verifying it against the agent-loop source, so new categories added to the source will silently pass this test instead of failing loudly. Implement a parity guard similar to the `include_str!`/`BTreeSet` approach mentioned in the review comment to verify that the expected categories list is exhaustive and matches all sanitized safe-summary categories from the agent-loop source. This way, when new categories are added to the source, the test will fail until the expected list is updated.Source: Coding guidelines
crates/ironclaw_turns/src/memory.rs (1)
2502-2506:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
resume_checkpoint_idbefore marking the run retryable.
fail_claimed_record()currently trusts any non-Noneresume_checkpoint_id. If the runner hands back a final or foreign-run checkpoint,record.checkpoint_idstays populated,push_event()reports the failed run as retryable, andretry_turn()later rejects the same run becauseretryable_loop_checkpoint()only accepts same-runBeforeModel/BeforeBlockcheckpoints. Filter the explicit id throughretryable_loop_checkpoint()first, then fall back tolatest_resumable_loop_checkpoint().Suggested shape
- let retry_checkpoint_id = resume_checkpoint_id.or_else(|| { - self.latest_resumable_loop_checkpoint(&record.scope, record.turn_id, record.run_id) - }); + let retry_checkpoint_id = resume_checkpoint_id + .filter(|checkpoint_id| self.retryable_loop_checkpoint(&record, *checkpoint_id).is_some()) + .or_else(|| { + self.latest_resumable_loop_checkpoint(&record.scope, record.turn_id, record.run_id) + });As per coding guidelines, "Fail loud" applies here, and "Every bug fix must include a regression test using
#[test]or#[tokio::test]."🤖 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/memory.rs` around lines 2502 - 2506, In the `fail_claimed_record()` method, the `resume_checkpoint_id` parameter is being trusted without validation, which can allow invalid checkpoints (final or foreign-run) to be marked as retryable. Validate the explicit `resume_checkpoint_id` through the `retryable_loop_checkpoint()` method first before using it; only if that returns None should you fall back to calling `latest_resumable_loop_checkpoint()`. Additionally, add a regression test using `#[tokio::test]` to verify that passing an invalid checkpoint to `fail_claimed_record()` does not incorrectly mark the run as retryable.Source: Coding guidelines
crates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rs (1)
286-287: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Avoid index-coupled schema assertions.
This test binds to
items[1]/items[2], so unrelated fixture reordering will fail it without any wire-schema regression. Selectrun_statusentries by"status"instead.Suggested refactor
- let running_status = &items[1]["run_status"]; - let failed_status = &items[2]["run_status"]; + let find_run_status = |status: &str| { + items.iter().find_map(|item| { + let run_status = item.get("run_status")?; + (run_status.get("status")?.as_str() == Some(status)).then_some(run_status) + }) + }; + let running_status = find_run_status("running").expect("running run_status"); + let failed_status = find_run_status("failed").expect("failed run_status");🤖 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_webui_v2/tests/webui_v2_schema_contract.rs` around lines 286 - 287, The test uses hardcoded array indices (items[1] and items[2]) to access run_status values, which makes it brittle to fixture reordering unrelated to schema changes. Instead of relying on indices, refactor the code to find the running and failed status entries by filtering the items array based on their "status" field values. Replace the direct index-based assignments with filtered lookups that match on the appropriate status value, ensuring the test validates the schema contract without coupling to fixture order.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_agent_loop/src/executor/capabilities.rs (1)
682-697:⚠️ Potential issue | 🟠 MajorModel output from failure explanation must be scanned for secrets before finalization.
The explanation response from the model (line 107,
ParentLoopOutput::AssistantReply) flows directly intofinalize_assistant_message()without passing throughLeakDetectororSanitizer. Per the safety guideline: "Every new ingress point...that reaches...a DB write without calling a safety_layer.* function is a review-blocker." Add a leak-detection scan on the reply content before callingfinalize_assistant_message()at line 119.🤖 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_agent_loop/src/executor/capabilities.rs` around lines 682 - 697, The failure explanation response from the model (ParentLoopOutput::AssistantReply) is being passed directly to finalize_assistant_message() without undergoing leak detection or sanitization, which violates the safety guideline requiring all ingress points reaching DB writes to call safety layer functions. Add a leak-detection scan by passing the reply content through LeakDetector or Sanitizer before the reply is processed in finalize_assistant_message(), ensuring that any secrets in the model-generated explanation are detected and removed before the message is finalized.
🤖 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_reborn_composition/src/slack_delivery.rs`:
- Around line 1112-1132: The single-flight guard logic in the guard.contains
check causes an early return at the already_delivering branch that bypasses the
auth-denial message posting logic around line 200. To fix this, check for and
handle AuthResolution::Denied acks before the single-flight guard check so that
cancellation messages are posted regardless of whether a delivery loop is
already active for that run_id. Additionally, add a caller-level regression test
to observe_workflow_ack that verifies when an in-flight watcher exists for a
run_id and an accepted AuthResolution::Denied ack arrives for the same run_id,
the Authentication canceled message is still posted exactly once.
---
Outside diff comments:
In `@crates/ironclaw_agent_loop/src/executor/capabilities.rs`:
- Around line 682-697: The failure explanation response from the model
(ParentLoopOutput::AssistantReply) is being passed directly to
finalize_assistant_message() without undergoing leak detection or sanitization,
which violates the safety guideline requiring all ingress points reaching DB
writes to call safety layer functions. Add a leak-detection scan by passing the
reply content through LeakDetector or Sanitizer before the reply is processed in
finalize_assistant_message(), ensuring that any secrets in the model-generated
explanation are detected and removed before the message is finalized.
🪄 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: 41ba17a1-5976-4253-91f3-8c1f48749751
📒 Files selected for processing (14)
crates/ironclaw_agent_loop/src/executor.rscrates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/gates.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_loop_support/src/subagent_spawn_port/tests.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/tests/hooks_integration.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rscrates/ironclaw_turns/src/run_profile/host.rstests/support_unit_tests.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_agent_loop/src/executor/capabilities.rs (1)
682-697:⚠️ Potential issue | 🟠 MajorModel output from failure explanation must be scanned for secrets before finalization.
The explanation response from the model (line 107,
ParentLoopOutput::AssistantReply) flows directly intofinalize_assistant_message()without passing throughLeakDetectororSanitizer. Per the safety guideline: "Every new ingress point...that reaches...a DB write without calling a safety_layer.* function is a review-blocker." Add a leak-detection scan on the reply content before callingfinalize_assistant_message()at line 119.🤖 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_agent_loop/src/executor/capabilities.rs` around lines 682 - 697, The failure explanation response from the model (ParentLoopOutput::AssistantReply) is being passed directly to finalize_assistant_message() without undergoing leak detection or sanitization, which violates the safety guideline requiring all ingress points reaching DB writes to call safety layer functions. Add a leak-detection scan by passing the reply content through LeakDetector or Sanitizer before the reply is processed in finalize_assistant_message(), ensuring that any secrets in the model-generated explanation are detected and removed before the message is finalized.
🤖 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_reborn_composition/src/slack_delivery.rs`:
- Around line 1112-1132: The single-flight guard logic in the guard.contains
check causes an early return at the already_delivering branch that bypasses the
auth-denial message posting logic around line 200. To fix this, check for and
handle AuthResolution::Denied acks before the single-flight guard check so that
cancellation messages are posted regardless of whether a delivery loop is
already active for that run_id. Additionally, add a caller-level regression test
to observe_workflow_ack that verifies when an in-flight watcher exists for a
run_id and an accepted AuthResolution::Denied ack arrives for the same run_id,
the Authentication canceled message is still posted exactly once.
---
Outside diff comments:
In `@crates/ironclaw_agent_loop/src/executor/capabilities.rs`:
- Around line 682-697: The failure explanation response from the model
(ParentLoopOutput::AssistantReply) is being passed directly to
finalize_assistant_message() without undergoing leak detection or sanitization,
which violates the safety guideline requiring all ingress points reaching DB
writes to call safety layer functions. Add a leak-detection scan by passing the
reply content through LeakDetector or Sanitizer before the reply is processed in
finalize_assistant_message(), ensuring that any secrets in the model-generated
explanation are detected and removed before the message is finalized.
🪄 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: 41ba17a1-5976-4253-91f3-8c1f48749751
📒 Files selected for processing (14)
crates/ironclaw_agent_loop/src/executor.rscrates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/gates.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_loop_support/src/subagent_spawn_port/tests.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/tests/hooks_integration.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rscrates/ironclaw_turns/src/run_profile/host.rstests/support_unit_tests.rs
🛑 Comments failed to post (1)
crates/ironclaw_reborn_composition/src/slack_delivery.rs (1)
1112-1132:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winSingle-flight guard drops accepted auth-denial feedback under contention
At Line 1125, the early return for an already-active
run_idbypasses the auth-denial branch at Line 200 that posts"Authentication canceled.".
So if an auth-denial ack arrives while the original watcher is still active, the user-facing denial message is silently skipped.Use the guard only for acks that actually enter long polling (
should_deliver_after_ack == true), or handle accepted auth-denial before the single-flight check.Suggested fix sketch
- let _delivery_guard = if let Some(run_id) = submitted_run_id(&ack) { + let _delivery_guard = if should_deliver_after_ack(&envelope, &ack) + && let Some(run_id) = submitted_run_id(&ack) + { let already_delivering = { let mut guard = self .active_delivery_run_ids .lock() .unwrap_or_else(|e| e.into_inner()); if guard.contains(&run_id) { true } else { guard.insert(run_id); false } }; if already_delivering { tracing::debug!( target = "ironclaw::reborn::slack_delivery", %run_id, "skipping redundant delivery loop: a loop is already watching this run" ); return; } Some(RunDeliveryGuard { set: &self.active_delivery_run_ids, run_id, }) } else { None };Please add a caller-level regression test that drives
observe_workflow_ackwith (1) in-flight watcher forrun_id, then (2) acceptedAuthResolution::Deniedfor samerun_id, and asserts the cancellation message is still posted once.As per coding guidelines,
"Fail loud"and"Test through the caller"apply here because this guard now gates a user-visible side effect.Also applies to: 200-208
🤖 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_reborn_composition/src/slack_delivery.rs` around lines 1112 - 1132, The single-flight guard logic in the guard.contains check causes an early return at the already_delivering branch that bypasses the auth-denial message posting logic around line 200. To fix this, check for and handle AuthResolution::Denied acks before the single-flight guard check so that cancellation messages are posted regardless of whether a delivery loop is already active for that run_id. Additionally, add a caller-level regression test to observe_workflow_ack that verifies when an in-flight watcher exists for a run_id and an accepted AuthResolution::Denied ack arrives for the same run_id, the Authentication canceled message is still posted exactly once.Source: Coding guidelines
Collaborator
Author
CodeRabbit review triage (pushed in c0578ee, 5e3b9b2, c1723c1)CodeRabbit's latest comments landed as "failed to post / outside-diff range" (no inline threads to reply on), so summarizing the triage here. Each finding was verified against current code before acting. Fixed (with regression tests where behavior changed):
Reviewed and not changed (reasoning):
Also: fixed two |
Collaborator
Author
Triage of latest CodeRabbit findings (fix pushed in 860c645)
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_agent_loop/tests/safety_nets.rs (1)
57-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis bypasses the behavior the test says it pins.
Line 57 mutates the latched cancellation state directly, so Lines 58-59 still pass even if the second
invoke_capability_batchnever consults or consumes cancellation at all. It also blesses replacing the first observed signal, which conflicts with theLoopCancellationPort::observe_cancellation()stable-snapshot contract. Use a fresh host for the second scenario, or keep the second signal on the same scripted batch path instead of mutating observable state out-of-band.🤖 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_agent_loop/tests/safety_nets.rs` around lines 57 - 59, The test mutates the cancellation state directly via set_cancellation_signal() on line 57, which allows the assertion on line 59 to pass even if invoke_capability_batch() never actually consumes the second signal, and violates the stable-snapshot contract of observe_cancellation(). Either create a fresh host instance for testing the second signal scenario instead of mutating the same host's state out-of-band, or integrate the second signal into the scripted batch execution path so the test verifies that invoke_capability_batch() genuinely consults the new cancellation signal during its execution rather than just observing a manually-set value.
🤖 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/status.rs`:
- Around line 204-215: The current normalization logic using `replace(':', "_")`
indiscriminately converts all colons to underscores, which allows malformed data
like `a::b`, `:model`, and `:` to become valid `SanitizedFailure` values that
the write path could never produce. Instead of replacing all colons, restrict
normalization to only the historical single-colon format (exactly one colon
separating two parts, like `host_stage_unavailable:model`). Check if the wire
category contains exactly one colon using a pattern match or split check, and
only then replace that single colon with an underscore; for any other colon
patterns (multiple colons, leading colon, trailing colon), either pass the
category through unchanged to let `Self::new()` reject it, or explicitly reject
it. Additionally, add regression tests that verify malformed categories with
multiple colons, leading colons, or trailing colons are properly rejected during
deserialization.
In `@crates/ironclaw_turns/tests/retry_failed_turn_store_contract.rs`:
- Around line 340-378: Add a new regression test function (similar to
assert_explicit_nonresumable_resume_checkpoint_is_not_retryable) that verifies
the fallback to latest_resumable_loop_checkpoint when an explicit non-resumable
checkpoint is supplied. The new test should create both a resumable checkpoint
(like BeforeModel or BeforeBlock) and a Final checkpoint on the same run, pass
the Final checkpoint as the explicit resume_checkpoint_id to fail_claimed_run,
assert that the failed run still advertises retryability (checkpoint_id is
Some), and then verify that retry_turn succeeds and actually retries from the
latest resumable checkpoint rather than failing with RunNotRetryable. This
ensures the fallback mechanism in memory.rs that filters out invalid explicit
checkpoints continues to work correctly.
---
Outside diff comments:
In `@crates/ironclaw_agent_loop/tests/safety_nets.rs`:
- Around line 57-59: The test mutates the cancellation state directly via
set_cancellation_signal() on line 57, which allows the assertion on line 59 to
pass even if invoke_capability_batch() never actually consumes the second
signal, and violates the stable-snapshot contract of observe_cancellation().
Either create a fresh host instance for testing the second signal scenario
instead of mutating the same host's state out-of-band, or integrate the second
signal into the scripted batch execution path so the test verifies that
invoke_capability_batch() genuinely consults the new cancellation signal during
its execution rather than just observing a manually-set value.
🪄 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: 9cf8e841-541c-4472-95dc-6679bc9bb36b
📒 Files selected for processing (14)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_agent_loop/tests/safety_nets.rscrates/ironclaw_hooks/src/middleware/tests/capability_port.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn_composition/src/projection/tests/failure_explanation.rscrates/ironclaw_turns/src/loop_exit/tests/mod.rscrates/ironclaw_turns/src/memory.rscrates/ironclaw_turns/src/status.rscrates/ironclaw_turns/tests/retry_failed_turn_store_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rstests/reborn_failure_retry_resume_e2e.rstests/support/reborn/harness.rstests/support/reborn/model_replay.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_agent_loop/src/state.rs (1)
642-670:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRegression test misses
pending_approval_resumepreservation pathLine 642 and Line 667-670 document/pin preservation of gate-bound resume state, but this test only seeds/asserts
pending_auth_resume. Please also seed and assertpending_approval_resumeso the full rebase contract is regression-locked.Suggested test delta
@@ state.last_gate = Some(LoopGateRef::new("gate:source-run").unwrap()); + state.pending_approval_resume = Some(PendingApprovalResume { + gate_ref: LoopGateRef::new("gate:source-approval").unwrap(), + capability_id: CapabilityId::new("gsuite.calendar.list_events").unwrap(), + approval_request_id: ApprovalRequestId::new(), + resume_token: CapabilityResumeToken::new( + "00000000-0000-0000-0000-000000000002", + ) + .unwrap(), + correlation_id: CorrelationId::new(), + surface_version: CapabilitySurfaceVersion::new("surface-v1").unwrap(), + input_ref: CapabilityInputRef::new("input:source-approval").unwrap(), + effective_capability_ids: vec![], + provider_replay: None, + input: json!({"k":"v"}), + estimate: ResourceEstimate::default(), + }); @@ assert_eq!(rebased.last_gate, state.last_gate); + assert_eq!( + rebased.pending_approval_resume, + state.pending_approval_resume + ); assert_eq!(rebased.pending_auth_resume, state.pending_auth_resume);As per coding guidelines: “Every bug fix must include a regression test using #[test] or #[tokio::test] that reproduces the original failure.”
🤖 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_agent_loop/src/state.rs` around lines 642 - 670, The regression test for the rebase_for_run method in the state module is incomplete. Currently, it only seeds and asserts preservation of pending_auth_resume, but per the comment on lines 642 and 667-670, the test should also cover the preservation of pending_approval_resume to fully lock down the gate-bound resume state rebase contract. Add seed logic for pending_approval_resume (similar to the existing pending_auth_resume setup) after the current pending_auth_resume assignment, and then add a corresponding assertion after the existing pending_auth_resume assertion to verify that rebased.pending_approval_resume equals state.pending_approval_resume.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@crates/ironclaw_agent_loop/src/state.rs`:
- Around line 642-670: The regression test for the rebase_for_run method in the
state module is incomplete. Currently, it only seeds and asserts preservation of
pending_auth_resume, but per the comment on lines 642 and 667-670, the test
should also cover the preservation of pending_approval_resume to fully lock down
the gate-bound resume state rebase contract. Add seed logic for
pending_approval_resume (similar to the existing pending_auth_resume setup)
after the current pending_auth_resume assignment, and then add a corresponding
assertion after the existing pending_auth_resume assertion to verify that
rebased.pending_approval_resume equals state.pending_approval_resume.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 48910314-b60b-43e4-ab94-7e282b0fb344
📒 Files selected for processing (1)
crates/ironclaw_agent_loop/src/state.rs
Collaborator
Author
Reborn root tests failure — fixed in a7c3f99Root cause: my earlier change to Reverted the gate-state clearing. The run-boundary stays fail-closed because the resume host re-validates the gate before honoring the pending-resume record — wiping the record was never what enforced that. The isolated Verified locally (78 tests, 0 failures): |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/ironclaw_loop_support/src/capability_port.rs (3)
1208-1231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap provider-origin sandbox conversion failures to tool failures.
Line 1216 catches schema-prep
InvalidInvocation, buthost_runtime_input_for_capability(...) ?can still returnInvalidInvocationfor provider-supplied process-sandbox plans after schema prep. That bypasses the provider error-result path and reintroduces a terminal tool-argument failure, contrary to this PR’s no run-borking failure objective.Proposed fix
- ( - host_runtime_input_for_capability(&request.capability_id, input)?, - capability.estimate.clone(), - ) + let input = match host_runtime_input_for_capability(&request.capability_id, input) { + Ok(input) => input, + Err(error) + if error.kind == AgentLoopHostErrorKind::InvalidInvocation + && request.is_provider_call => + { + let result = Ok(CapabilityOutcome::Failed(CapabilityFailure { + error_kind: CapabilityFailureKind::InvalidInput, + safe_summary: error.safe_summary, + detail: None, + })); + guard.commit(); + self.record_loop_completed(&idempotency_key, result.clone())?; + return result; + } + Err(error) => return Err(error), + }; + (input, capability.estimate.clone())🤖 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_loop_support/src/capability_port.rs` around lines 1208 - 1231, The error handling after `prepare_provider_arguments_with_detail` only catches `InvalidInvocation` errors from schema prep, but the subsequent call to `host_runtime_input_for_capability(...)` can also return `InvalidInvocation` errors for provider-supplied process-sandbox plans. Replace the `?` operator at the call to `host_runtime_input_for_capability(...)` with a match expression that catches `InvalidInvocation` errors and maps them to a `CapabilityOutcome::Failed` result with `CapabilityFailureKind::InvalidInput`, following the same pattern used for the schema prep error handling (lines 1208-1223), while letting other errors propagate normally. This ensures provider-origin sandbox conversion failures are treated as tool failures rather than terminal errors.
5051-5059:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMark these provider-staged invocations as provider calls.
Both candidates are created by
register_provider_tool_call, but the invocations setis_provider_call: false. That bypasses Line 1216, so schema-invalid input returnsErr(InvalidInvocation)instead of theCapabilityOutcome::Failedthese tests assert.Proposed fix
- is_provider_call: false, + is_provider_call: true,Apply the same change to both provider-staged schema-failure invocations.
Also applies to: 5147-5155
🤖 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_loop_support/src/capability_port.rs` around lines 5051 - 5059, The `CapabilityInvocation` objects for provider-staged schema-failure invocations are incorrectly setting `is_provider_call: false` when they should be set to `is_provider_call: true`. This causes schema-invalid input to bypass the validation check at Line 1216 and return `Err(InvalidInvocation)` instead of the expected `CapabilityOutcome::Failed`. Change `is_provider_call: false` to `is_provider_call: true` in the `CapabilityInvocation` struct at lines 5051-5059 (in the first `invoke_capability` call) and at lines 5147-5155 (in the second `invoke_capability` call).
66-75: 🧹 Nitpick | 🔵 TrivialTest fixtures do not exercise provider input registration.
The trait default at lines 66–75 returns
InvalidInvocationforregister_provider_tool_call_input. All production resolvers (LocalDevCapabilityIo,ProductLiveCapabilityIo,SubagentSpawnInputCodec) properly override this method. However, test doubles inloop_driver_host.rs(StubInputResolver,InMemoryCapabilityIo) do not—though they are not exercised for provider tool call paths. Consider adding explicit overrides to test fixtures for consistency, or document that they are scoped to non-provider workflows.🤖 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_loop_support/src/capability_port.rs` around lines 66 - 75, The test fixture classes StubInputResolver and InMemoryCapabilityIo do not provide explicit overrides for the register_provider_tool_call_input method, relying instead on the trait default that returns InvalidInvocation. To improve consistency with production resolvers (LocalDevCapabilityIo, ProductLiveCapabilityIo, SubagentSpawnInputCodec) that all override this method, either add explicit implementations of register_provider_tool_call_input to both test fixture classes that align with their intended behavior, or add clear documentation comments to these test classes explaining that they are scoped to non-provider workflows and intentionally do not override this method.crates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs (1)
790-797:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
is_provider_callis inverted on provider-origin test invocations.Invocations created from
register_provider_tool_call(...)candidates are being taggedis_provider_call: false. That breaks provenance fidelity and can hide regressions in provider-call branches while tests still pass.
crates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs#L790-L797: setis_provider_call: truefor candidate-derived invocation helpers/usages.crates/ironclaw_loop_support/src/capability_surface_filter.rs#L1067-L1074: setis_provider_call: truefor invocation built fromcandidatereturned by provider registration.crates/ironclaw_loop_support/src/subagent_spawn_port/tests.rs#L1613-L1620: setis_provider_call: truefor candidate-based invocation after provider registration.As per coding guidelines, the “Everything Goes Through Tools” invariant requires preserving tool-origin semantics through the dispatch path.
🤖 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_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs` around lines 790 - 797, The `is_provider_call` field is incorrectly set to `false` when creating `CapabilityInvocation` instances from provider-originated tool call candidates, but should be `true` to preserve tool-origin semantics and maintain provenance fidelity. Fix this across three locations: (1) in crates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs lines 790-797, change `is_provider_call: false` to `is_provider_call: true` in the test invocation helper, (2) in crates/ironclaw_loop_support/src/capability_surface_filter.rs lines 1067-1074, change `is_provider_call: false` to `is_provider_call: true` when building the invocation from the provider registration candidate, and (3) in crates/ironclaw_loop_support/src/subagent_spawn_port/tests.rs lines 1613-1620, change `is_provider_call: false` to `is_provider_call: true` for the candidate-based invocation after provider registration.Source: Coding guidelines
crates/ironclaw_loop_support/tests/host_capability_port_composition.rs (1)
44-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail loud on invariant-scan file reads
Line 44 silently converts filesystem read failures into empty content via
unwrap_or_default(). That can hide offenders and let the construction-invariant test pass incorrectly. Return/record a test failure on read errors instead of defaulting.Suggested fix
- let src = std::fs::read_to_string(path).unwrap_or_default(); + let src = std::fs::read_to_string(path) + .unwrap_or_else(|e| panic!("failed to read {}: {e}", path.display()));As per coding guidelines, fail-loud patterns must not collapse IO errors into empty state.
🤖 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_loop_support/tests/host_capability_port_composition.rs` at line 44, The read_to_string call with unwrap_or_default() on line 44 silently converts file read failures into empty content, which can hide errors and allow the test to incorrectly pass. Replace the unwrap_or_default() pattern with proper error handling that fails the test when the filesystem read operation encounters an error. Use either expect() with a descriptive error message, or use the ? operator if within a function that returns a Result, ensuring IO errors are propagated and not silently converted to empty state.Source: Coding guidelines
🤖 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_agent_loop/src/executor/capabilities.rs`:
- Around line 638-679: The `is_resume_origin` flag derived from
`state.pending_approval_resume` and `state.pending_auth_resume` at
error-handling time in the executor/capabilities.rs file (around line 670) is
vulnerable to being cleared by concurrent outcome processing before the deferred
failure is handled, allowing a resumed failure to be treated as non-resume and
permit unsafe retry dispatch. Instead of deriving resume origin from mutable
shared state at error-handling time, capture the resume origin as immutable
per-invocation provenance when the capability invocation is created, then
reference that immutable captured flag throughout the error-handling path. This
same fix must be applied at the secondary locations (lines 747-768) where
`is_resume_origin` is used to guard retry eligibility, ensuring all retry guard
checks use the immutable per-invocation provenance rather than querying mutable
state.
In `@crates/ironclaw_agent_loop/src/executor/tests.rs`:
- Around line 5491-5497: Add assertions to each of the four test cases (at
crates/ironclaw_agent_loop/src/executor/tests.rs lines 5491-5497, 5541-5568,
5658-5666, and 5711-5738) to verify that the Phase 2 batch invocation actually
executed and that the Backend failure was properly appended as a tool-error
result. These assertions must confirm the second batch call ran with the correct
parameters and that the resulting tool error appears in the final state,
ensuring the test will fail if resume dispatch logic is skipped or the failure
handling is not exercised.
- Around line 5395-5739: The file tests.rs now exceeds 5,739 lines and this PR
adds 345 lines (>200-line threshold), requiring justification per CLAUDE.md. Add
an inline arch-exempt comment immediately before the first test function
(resume_origin_backend_failure_does_not_die_as_scope_mismatch) in the format:
`// arch-exempt: [reason], [tracking issue]`. In the reason, briefly explain
that these are regression tests for the Part C-sub-A fix addressing
scope_mismatch failures when approval-resume and auth-resume dispatches return
Backend errors, and that they test distinct observable behavior requiring two
separate test functions. Reference a decomposition tracking issue number if one
exists, or use a placeholder like `plan `#TBD`` if filing will happen separately.
In `@crates/ironclaw_reborn/src/loop_driver_host.rs`:
- Around line 2320-2326: Add a regression test case in the mapping test module
that verifies the new error branch for TurnError::InvalidRunOriginAdapter
correctly maps to AgentLoopHostErrorKind::InvalidInvocation. The test should
assert that when a TurnError::InvalidRunOriginAdapter is encountered, it
produces the expected AgentLoopHostErrorKind::InvalidInvocation output through
the ironclaw_loop_support::raw_agent_loop_host_error function. Use the standard
#[test] or #[tokio::test] attribute as appropriate for the test module
structure.
In `@crates/ironclaw_turns/src/run_profile/host.rs`:
- Around line 1393-1394: Replace the boolean field `is_provider_call` in the
struct at the anchor location with a typed `CapabilityInvocationOrigin` enum
that represents the semantic modes of provider tool call replay versus direct
invocation. Define the enum with appropriate variants (e.g., one for direct
invocation and one for provider replay), then update all construction sites that
currently hardcode `false` or derive the value from
`call.provider_replay.is_some()` to instead construct the appropriate enum
variant. Finally, update the match guard branching logic in
`crates/ironclaw_loop_support/src/capability_port.rs:1216` to pattern match on
the enum variants instead of the boolean condition.
In `@crates/ironclaw_turns/tests/agent_loop_host_contract.rs`:
- Around line 3723-3724: Before the test block comment at line 3723 that marks
the start of the ProductTurnContext serde round-trips tests, add an inline
comment block that includes a reference to the decomposition tracking issue for
this file (which exceeds 3,000 lines) and a clear justification explaining why
adding this block of >200 lines is necessary. The justification should explain
the purpose and importance of these new tests to provide context for the file
size increase.
- Around line 3851-3908: The test function
instruction_bundle_runtime_communication_none_is_byte_identical_to_4795_baseline
claims to verify byte and fingerprint parity with a baseline but only validates
the presence or absence of specific strings. To fix this, define or reference
the expected baseline content and fingerprint values for the `#4795` baseline
case, then add explicit equality assertions comparing the actual rendered
runtime content (from runtime_msg.model_content) and the bundle's fingerprint to
these baseline values. This ensures the test actually guards against unintended
drift from the baseline rather than just checking for specific substrings.
In `@docs/plans/2026-06-15-provider-tool-input-scope-mismatch.md`:
- Line 3: The document contains contradictory implementation status statements:
line 3 states the design is ACCEPTED and ready to implement, while line 130
states that final signoff is still pending. Review both locations in the
document and choose one canonical implementation status, then update both line 3
and line 130 to consistently reflect that single state throughout the document
so that execution ownership is unambiguous.
---
Outside diff comments:
In `@crates/ironclaw_loop_support/src/capability_port.rs`:
- Around line 1208-1231: The error handling after
`prepare_provider_arguments_with_detail` only catches `InvalidInvocation` errors
from schema prep, but the subsequent call to
`host_runtime_input_for_capability(...)` can also return `InvalidInvocation`
errors for provider-supplied process-sandbox plans. Replace the `?` operator at
the call to `host_runtime_input_for_capability(...)` with a match expression
that catches `InvalidInvocation` errors and maps them to a
`CapabilityOutcome::Failed` result with `CapabilityFailureKind::InvalidInput`,
following the same pattern used for the schema prep error handling (lines
1208-1223), while letting other errors propagate normally. This ensures
provider-origin sandbox conversion failures are treated as tool failures rather
than terminal errors.
- Around line 5051-5059: The `CapabilityInvocation` objects for provider-staged
schema-failure invocations are incorrectly setting `is_provider_call: false`
when they should be set to `is_provider_call: true`. This causes schema-invalid
input to bypass the validation check at Line 1216 and return
`Err(InvalidInvocation)` instead of the expected `CapabilityOutcome::Failed`.
Change `is_provider_call: false` to `is_provider_call: true` in the
`CapabilityInvocation` struct at lines 5051-5059 (in the first
`invoke_capability` call) and at lines 5147-5155 (in the second
`invoke_capability` call).
- Around line 66-75: The test fixture classes StubInputResolver and
InMemoryCapabilityIo do not provide explicit overrides for the
register_provider_tool_call_input method, relying instead on the trait default
that returns InvalidInvocation. To improve consistency with production resolvers
(LocalDevCapabilityIo, ProductLiveCapabilityIo, SubagentSpawnInputCodec) that
all override this method, either add explicit implementations of
register_provider_tool_call_input to both test fixture classes that align with
their intended behavior, or add clear documentation comments to these test
classes explaining that they are scoped to non-provider workflows and
intentionally do not override this method.
In
`@crates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs`:
- Around line 790-797: The `is_provider_call` field is incorrectly set to
`false` when creating `CapabilityInvocation` instances from provider-originated
tool call candidates, but should be `true` to preserve tool-origin semantics and
maintain provenance fidelity. Fix this across three locations: (1) in
crates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs
lines 790-797, change `is_provider_call: false` to `is_provider_call: true` in
the test invocation helper, (2) in
crates/ironclaw_loop_support/src/capability_surface_filter.rs lines 1067-1074,
change `is_provider_call: false` to `is_provider_call: true` when building the
invocation from the provider registration candidate, and (3) in
crates/ironclaw_loop_support/src/subagent_spawn_port/tests.rs lines 1613-1620,
change `is_provider_call: false` to `is_provider_call: true` for the
candidate-based invocation after provider registration.
In `@crates/ironclaw_loop_support/tests/host_capability_port_composition.rs`:
- Line 44: The read_to_string call with unwrap_or_default() on line 44 silently
converts file read failures into empty content, which can hide errors and allow
the test to incorrectly pass. Replace the unwrap_or_default() pattern with
proper error handling that fails the test when the filesystem read operation
encounters an error. Use either expect() with a descriptive error message, or
use the ? operator if within a function that returns a Result, ensuring IO
errors are propagated and not silently converted to empty state.
🪄 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: 9837fcc9-ebeb-4fc5-a45a-4548aadf0402
📒 Files selected for processing (60)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/capability_helpers.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/tests/safety_nets.rscrates/ironclaw_conversations/src/inbound.rscrates/ironclaw_conversations/src/trusted_trigger.rscrates/ironclaw_hooks/src/middleware/capability_port.rscrates/ironclaw_hooks/src/middleware/resolver.rscrates/ironclaw_hooks/src/middleware/tests/capability_port.rscrates/ironclaw_host_runtime/tests/turn_scheduler_contract.rscrates/ironclaw_loop_support/src/cancellation_port.rscrates/ironclaw_loop_support/src/capability_port.rscrates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rscrates/ironclaw_loop_support/src/capability_surface_filter.rscrates/ironclaw_loop_support/src/subagent_spawn_port/tests.rscrates/ironclaw_loop_support/tests/host_capability_port_composition.rscrates/ironclaw_loop_support/tests/thread_loop_support_contract.rscrates/ironclaw_product_workflow/src/auth_continuation.rscrates/ironclaw_product_workflow/src/inbound_turn.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/webui_inbound.rscrates/ironclaw_product_workflow/tests/approval_interaction_contract.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_product_workflow/tests/inbound_turn_contract.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/support.rscrates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn/src/subagent/flavors.rscrates/ironclaw_reborn/src/turn_runner/tests/mod.rscrates/ironclaw_reborn/tests/hooks_integration.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn/tests/loop_milestone_event_projection.rscrates/ironclaw_reborn_composition/src/factory/auth_tests.rscrates/ironclaw_reborn_composition/src/projection/tests.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/shell_tests.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rscrates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rscrates/ironclaw_reborn_composition/tests/product_live_adapters.rscrates/ironclaw_turns/src/events.rscrates/ironclaw_turns/src/lib.rscrates/ironclaw_turns/src/memory.rscrates/ironclaw_turns/src/request.rscrates/ironclaw_turns/src/run_profile/host.rscrates/ironclaw_turns/src/run_profile/prompt.rscrates/ironclaw_turns/src/status.rscrates/ironclaw_turns/src/store.rscrates/ironclaw_turns/tests/active_run_ref_state_contract.rscrates/ironclaw_turns/tests/agent_loop_host_contract.rscrates/ironclaw_turns/tests/checkpoint_state_store_contract.rscrates/ironclaw_turns/tests/retry_failed_turn_store_contract.rscrates/ironclaw_turns/tests/turn_coordinator_contract.rscrates/ironclaw_webui_v2/src/handlers.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rsdocs/plans/2026-06-15-provider-tool-input-scope-mismatch.mdtests/support/reborn/harness.rstests/support_unit_tests.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/ironclaw_loop_support/src/capability_port.rs (3)
1208-1231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap provider-origin sandbox conversion failures to tool failures.
Line 1216 catches schema-prep
InvalidInvocation, buthost_runtime_input_for_capability(...) ?can still returnInvalidInvocationfor provider-supplied process-sandbox plans after schema prep. That bypasses the provider error-result path and reintroduces a terminal tool-argument failure, contrary to this PR’s no run-borking failure objective.Proposed fix
- ( - host_runtime_input_for_capability(&request.capability_id, input)?, - capability.estimate.clone(), - ) + let input = match host_runtime_input_for_capability(&request.capability_id, input) { + Ok(input) => input, + Err(error) + if error.kind == AgentLoopHostErrorKind::InvalidInvocation + && request.is_provider_call => + { + let result = Ok(CapabilityOutcome::Failed(CapabilityFailure { + error_kind: CapabilityFailureKind::InvalidInput, + safe_summary: error.safe_summary, + detail: None, + })); + guard.commit(); + self.record_loop_completed(&idempotency_key, result.clone())?; + return result; + } + Err(error) => return Err(error), + }; + (input, capability.estimate.clone())🤖 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_loop_support/src/capability_port.rs` around lines 1208 - 1231, The error handling after `prepare_provider_arguments_with_detail` only catches `InvalidInvocation` errors from schema prep, but the subsequent call to `host_runtime_input_for_capability(...)` can also return `InvalidInvocation` errors for provider-supplied process-sandbox plans. Replace the `?` operator at the call to `host_runtime_input_for_capability(...)` with a match expression that catches `InvalidInvocation` errors and maps them to a `CapabilityOutcome::Failed` result with `CapabilityFailureKind::InvalidInput`, following the same pattern used for the schema prep error handling (lines 1208-1223), while letting other errors propagate normally. This ensures provider-origin sandbox conversion failures are treated as tool failures rather than terminal errors.
5051-5059:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMark these provider-staged invocations as provider calls.
Both candidates are created by
register_provider_tool_call, but the invocations setis_provider_call: false. That bypasses Line 1216, so schema-invalid input returnsErr(InvalidInvocation)instead of theCapabilityOutcome::Failedthese tests assert.Proposed fix
- is_provider_call: false, + is_provider_call: true,Apply the same change to both provider-staged schema-failure invocations.
Also applies to: 5147-5155
🤖 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_loop_support/src/capability_port.rs` around lines 5051 - 5059, The `CapabilityInvocation` objects for provider-staged schema-failure invocations are incorrectly setting `is_provider_call: false` when they should be set to `is_provider_call: true`. This causes schema-invalid input to bypass the validation check at Line 1216 and return `Err(InvalidInvocation)` instead of the expected `CapabilityOutcome::Failed`. Change `is_provider_call: false` to `is_provider_call: true` in the `CapabilityInvocation` struct at lines 5051-5059 (in the first `invoke_capability` call) and at lines 5147-5155 (in the second `invoke_capability` call).
66-75: 🧹 Nitpick | 🔵 TrivialTest fixtures do not exercise provider input registration.
The trait default at lines 66–75 returns
InvalidInvocationforregister_provider_tool_call_input. All production resolvers (LocalDevCapabilityIo,ProductLiveCapabilityIo,SubagentSpawnInputCodec) properly override this method. However, test doubles inloop_driver_host.rs(StubInputResolver,InMemoryCapabilityIo) do not—though they are not exercised for provider tool call paths. Consider adding explicit overrides to test fixtures for consistency, or document that they are scoped to non-provider workflows.🤖 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_loop_support/src/capability_port.rs` around lines 66 - 75, The test fixture classes StubInputResolver and InMemoryCapabilityIo do not provide explicit overrides for the register_provider_tool_call_input method, relying instead on the trait default that returns InvalidInvocation. To improve consistency with production resolvers (LocalDevCapabilityIo, ProductLiveCapabilityIo, SubagentSpawnInputCodec) that all override this method, either add explicit implementations of register_provider_tool_call_input to both test fixture classes that align with their intended behavior, or add clear documentation comments to these test classes explaining that they are scoped to non-provider workflows and intentionally do not override this method.crates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs (1)
790-797:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
is_provider_callis inverted on provider-origin test invocations.Invocations created from
register_provider_tool_call(...)candidates are being taggedis_provider_call: false. That breaks provenance fidelity and can hide regressions in provider-call branches while tests still pass.
crates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs#L790-L797: setis_provider_call: truefor candidate-derived invocation helpers/usages.crates/ironclaw_loop_support/src/capability_surface_filter.rs#L1067-L1074: setis_provider_call: truefor invocation built fromcandidatereturned by provider registration.crates/ironclaw_loop_support/src/subagent_spawn_port/tests.rs#L1613-L1620: setis_provider_call: truefor candidate-based invocation after provider registration.As per coding guidelines, the “Everything Goes Through Tools” invariant requires preserving tool-origin semantics through the dispatch path.
🤖 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_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs` around lines 790 - 797, The `is_provider_call` field is incorrectly set to `false` when creating `CapabilityInvocation` instances from provider-originated tool call candidates, but should be `true` to preserve tool-origin semantics and maintain provenance fidelity. Fix this across three locations: (1) in crates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs lines 790-797, change `is_provider_call: false` to `is_provider_call: true` in the test invocation helper, (2) in crates/ironclaw_loop_support/src/capability_surface_filter.rs lines 1067-1074, change `is_provider_call: false` to `is_provider_call: true` when building the invocation from the provider registration candidate, and (3) in crates/ironclaw_loop_support/src/subagent_spawn_port/tests.rs lines 1613-1620, change `is_provider_call: false` to `is_provider_call: true` for the candidate-based invocation after provider registration.Source: Coding guidelines
crates/ironclaw_loop_support/tests/host_capability_port_composition.rs (1)
44-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail loud on invariant-scan file reads
Line 44 silently converts filesystem read failures into empty content via
unwrap_or_default(). That can hide offenders and let the construction-invariant test pass incorrectly. Return/record a test failure on read errors instead of defaulting.Suggested fix
- let src = std::fs::read_to_string(path).unwrap_or_default(); + let src = std::fs::read_to_string(path) + .unwrap_or_else(|e| panic!("failed to read {}: {e}", path.display()));As per coding guidelines, fail-loud patterns must not collapse IO errors into empty state.
🤖 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_loop_support/tests/host_capability_port_composition.rs` at line 44, The read_to_string call with unwrap_or_default() on line 44 silently converts file read failures into empty content, which can hide errors and allow the test to incorrectly pass. Replace the unwrap_or_default() pattern with proper error handling that fails the test when the filesystem read operation encounters an error. Use either expect() with a descriptive error message, or use the ? operator if within a function that returns a Result, ensuring IO errors are propagated and not silently converted to empty state.Source: Coding guidelines
🤖 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_agent_loop/src/executor/capabilities.rs`:
- Around line 638-679: The `is_resume_origin` flag derived from
`state.pending_approval_resume` and `state.pending_auth_resume` at
error-handling time in the executor/capabilities.rs file (around line 670) is
vulnerable to being cleared by concurrent outcome processing before the deferred
failure is handled, allowing a resumed failure to be treated as non-resume and
permit unsafe retry dispatch. Instead of deriving resume origin from mutable
shared state at error-handling time, capture the resume origin as immutable
per-invocation provenance when the capability invocation is created, then
reference that immutable captured flag throughout the error-handling path. This
same fix must be applied at the secondary locations (lines 747-768) where
`is_resume_origin` is used to guard retry eligibility, ensuring all retry guard
checks use the immutable per-invocation provenance rather than querying mutable
state.
In `@crates/ironclaw_agent_loop/src/executor/tests.rs`:
- Around line 5491-5497: Add assertions to each of the four test cases (at
crates/ironclaw_agent_loop/src/executor/tests.rs lines 5491-5497, 5541-5568,
5658-5666, and 5711-5738) to verify that the Phase 2 batch invocation actually
executed and that the Backend failure was properly appended as a tool-error
result. These assertions must confirm the second batch call ran with the correct
parameters and that the resulting tool error appears in the final state,
ensuring the test will fail if resume dispatch logic is skipped or the failure
handling is not exercised.
- Around line 5395-5739: The file tests.rs now exceeds 5,739 lines and this PR
adds 345 lines (>200-line threshold), requiring justification per CLAUDE.md. Add
an inline arch-exempt comment immediately before the first test function
(resume_origin_backend_failure_does_not_die_as_scope_mismatch) in the format:
`// arch-exempt: [reason], [tracking issue]`. In the reason, briefly explain
that these are regression tests for the Part C-sub-A fix addressing
scope_mismatch failures when approval-resume and auth-resume dispatches return
Backend errors, and that they test distinct observable behavior requiring two
separate test functions. Reference a decomposition tracking issue number if one
exists, or use a placeholder like `plan `#TBD`` if filing will happen separately.
In `@crates/ironclaw_reborn/src/loop_driver_host.rs`:
- Around line 2320-2326: Add a regression test case in the mapping test module
that verifies the new error branch for TurnError::InvalidRunOriginAdapter
correctly maps to AgentLoopHostErrorKind::InvalidInvocation. The test should
assert that when a TurnError::InvalidRunOriginAdapter is encountered, it
produces the expected AgentLoopHostErrorKind::InvalidInvocation output through
the ironclaw_loop_support::raw_agent_loop_host_error function. Use the standard
#[test] or #[tokio::test] attribute as appropriate for the test module
structure.
In `@crates/ironclaw_turns/src/run_profile/host.rs`:
- Around line 1393-1394: Replace the boolean field `is_provider_call` in the
struct at the anchor location with a typed `CapabilityInvocationOrigin` enum
that represents the semantic modes of provider tool call replay versus direct
invocation. Define the enum with appropriate variants (e.g., one for direct
invocation and one for provider replay), then update all construction sites that
currently hardcode `false` or derive the value from
`call.provider_replay.is_some()` to instead construct the appropriate enum
variant. Finally, update the match guard branching logic in
`crates/ironclaw_loop_support/src/capability_port.rs:1216` to pattern match on
the enum variants instead of the boolean condition.
In `@crates/ironclaw_turns/tests/agent_loop_host_contract.rs`:
- Around line 3723-3724: Before the test block comment at line 3723 that marks
the start of the ProductTurnContext serde round-trips tests, add an inline
comment block that includes a reference to the decomposition tracking issue for
this file (which exceeds 3,000 lines) and a clear justification explaining why
adding this block of >200 lines is necessary. The justification should explain
the purpose and importance of these new tests to provide context for the file
size increase.
- Around line 3851-3908: The test function
instruction_bundle_runtime_communication_none_is_byte_identical_to_4795_baseline
claims to verify byte and fingerprint parity with a baseline but only validates
the presence or absence of specific strings. To fix this, define or reference
the expected baseline content and fingerprint values for the `#4795` baseline
case, then add explicit equality assertions comparing the actual rendered
runtime content (from runtime_msg.model_content) and the bundle's fingerprint to
these baseline values. This ensures the test actually guards against unintended
drift from the baseline rather than just checking for specific substrings.
In `@docs/plans/2026-06-15-provider-tool-input-scope-mismatch.md`:
- Line 3: The document contains contradictory implementation status statements:
line 3 states the design is ACCEPTED and ready to implement, while line 130
states that final signoff is still pending. Review both locations in the
document and choose one canonical implementation status, then update both line 3
and line 130 to consistently reflect that single state throughout the document
so that execution ownership is unambiguous.
---
Outside diff comments:
In `@crates/ironclaw_loop_support/src/capability_port.rs`:
- Around line 1208-1231: The error handling after
`prepare_provider_arguments_with_detail` only catches `InvalidInvocation` errors
from schema prep, but the subsequent call to
`host_runtime_input_for_capability(...)` can also return `InvalidInvocation`
errors for provider-supplied process-sandbox plans. Replace the `?` operator at
the call to `host_runtime_input_for_capability(...)` with a match expression
that catches `InvalidInvocation` errors and maps them to a
`CapabilityOutcome::Failed` result with `CapabilityFailureKind::InvalidInput`,
following the same pattern used for the schema prep error handling (lines
1208-1223), while letting other errors propagate normally. This ensures
provider-origin sandbox conversion failures are treated as tool failures rather
than terminal errors.
- Around line 5051-5059: The `CapabilityInvocation` objects for provider-staged
schema-failure invocations are incorrectly setting `is_provider_call: false`
when they should be set to `is_provider_call: true`. This causes schema-invalid
input to bypass the validation check at Line 1216 and return
`Err(InvalidInvocation)` instead of the expected `CapabilityOutcome::Failed`.
Change `is_provider_call: false` to `is_provider_call: true` in the
`CapabilityInvocation` struct at lines 5051-5059 (in the first
`invoke_capability` call) and at lines 5147-5155 (in the second
`invoke_capability` call).
- Around line 66-75: The test fixture classes StubInputResolver and
InMemoryCapabilityIo do not provide explicit overrides for the
register_provider_tool_call_input method, relying instead on the trait default
that returns InvalidInvocation. To improve consistency with production resolvers
(LocalDevCapabilityIo, ProductLiveCapabilityIo, SubagentSpawnInputCodec) that
all override this method, either add explicit implementations of
register_provider_tool_call_input to both test fixture classes that align with
their intended behavior, or add clear documentation comments to these test
classes explaining that they are scoped to non-provider workflows and
intentionally do not override this method.
In
`@crates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs`:
- Around line 790-797: The `is_provider_call` field is incorrectly set to
`false` when creating `CapabilityInvocation` instances from provider-originated
tool call candidates, but should be `true` to preserve tool-origin semantics and
maintain provenance fidelity. Fix this across three locations: (1) in
crates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rs
lines 790-797, change `is_provider_call: false` to `is_provider_call: true` in
the test invocation helper, (2) in
crates/ironclaw_loop_support/src/capability_surface_filter.rs lines 1067-1074,
change `is_provider_call: false` to `is_provider_call: true` when building the
invocation from the provider registration candidate, and (3) in
crates/ironclaw_loop_support/src/subagent_spawn_port/tests.rs lines 1613-1620,
change `is_provider_call: false` to `is_provider_call: true` for the
candidate-based invocation after provider registration.
In `@crates/ironclaw_loop_support/tests/host_capability_port_composition.rs`:
- Line 44: The read_to_string call with unwrap_or_default() on line 44 silently
converts file read failures into empty content, which can hide errors and allow
the test to incorrectly pass. Replace the unwrap_or_default() pattern with
proper error handling that fails the test when the filesystem read operation
encounters an error. Use either expect() with a descriptive error message, or
use the ? operator if within a function that returns a Result, ensuring IO
errors are propagated and not silently converted to empty state.
🪄 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: 9837fcc9-ebeb-4fc5-a45a-4548aadf0402
📒 Files selected for processing (60)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/capability_helpers.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/tests/safety_nets.rscrates/ironclaw_conversations/src/inbound.rscrates/ironclaw_conversations/src/trusted_trigger.rscrates/ironclaw_hooks/src/middleware/capability_port.rscrates/ironclaw_hooks/src/middleware/resolver.rscrates/ironclaw_hooks/src/middleware/tests/capability_port.rscrates/ironclaw_host_runtime/tests/turn_scheduler_contract.rscrates/ironclaw_loop_support/src/cancellation_port.rscrates/ironclaw_loop_support/src/capability_port.rscrates/ironclaw_loop_support/src/capability_port/tests/runtime_lifecycle_tests.rscrates/ironclaw_loop_support/src/capability_surface_filter.rscrates/ironclaw_loop_support/src/subagent_spawn_port/tests.rscrates/ironclaw_loop_support/tests/host_capability_port_composition.rscrates/ironclaw_loop_support/tests/thread_loop_support_contract.rscrates/ironclaw_product_workflow/src/auth_continuation.rscrates/ironclaw_product_workflow/src/inbound_turn.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/webui_inbound.rscrates/ironclaw_product_workflow/tests/approval_interaction_contract.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_product_workflow/tests/inbound_turn_contract.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/support.rscrates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn/src/subagent/flavors.rscrates/ironclaw_reborn/src/turn_runner/tests/mod.rscrates/ironclaw_reborn/tests/hooks_integration.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn/tests/loop_milestone_event_projection.rscrates/ironclaw_reborn_composition/src/factory/auth_tests.rscrates/ironclaw_reborn_composition/src/projection/tests.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/shell_tests.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rscrates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rscrates/ironclaw_reborn_composition/tests/product_live_adapters.rscrates/ironclaw_turns/src/events.rscrates/ironclaw_turns/src/lib.rscrates/ironclaw_turns/src/memory.rscrates/ironclaw_turns/src/request.rscrates/ironclaw_turns/src/run_profile/host.rscrates/ironclaw_turns/src/run_profile/prompt.rscrates/ironclaw_turns/src/status.rscrates/ironclaw_turns/src/store.rscrates/ironclaw_turns/tests/active_run_ref_state_contract.rscrates/ironclaw_turns/tests/agent_loop_host_contract.rscrates/ironclaw_turns/tests/checkpoint_state_store_contract.rscrates/ironclaw_turns/tests/retry_failed_turn_store_contract.rscrates/ironclaw_turns/tests/turn_coordinator_contract.rscrates/ironclaw_webui_v2/src/handlers.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rsdocs/plans/2026-06-15-provider-tool-input-scope-mismatch.mdtests/support/reborn/harness.rstests/support_unit_tests.rs
🛑 Comments failed to post (8)
crates/ironclaw_agent_loop/src/executor/capabilities.rs (1)
638-679:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResume-origin retry guard can be bypassed after deferred outcome reordering.
Line 670 derives
is_resume_originfromstate.pending_*at error-handling time. In the non-suspended path, failed outcomes are deferred first (Line 313), while later completed outcomes can clear those markers for the same capability (Lines 271-272 / 294-295) before the deferred failure is processed. That makes a resumed failure look non-resume, allowing retry dispatch at Line 794 and reintroducing the exact side-effect re-exec / scope-mismatch hazard this guard is meant to block.Suggested fix (capture resume origin per invocation index, not from mutable state later)
- let outcomes = batch.outcomes; + let outcomes = batch.outcomes; + // Build alongside invocation creation: true iff that invocation consumed + // approval/auth resume context. + // Example shape: Vec<bool> aligned to visible_calls order. + let resume_origin_by_index: Vec<bool> = ...; - for (call, outcome) in visible_calls.into_iter().zip(outcomes) { + for (idx, (call, outcome)) in visible_calls.into_iter().zip(outcomes).enumerate() { match outcome { ... - other => pending_outcomes.push((call, other)), + other => pending_outcomes.push((idx, call, other)), } } - for (call, outcome) in pending_outcomes { + for (idx, call, outcome) in pending_outcomes { + let is_resume_origin = resume_origin_by_index[idx]; match self - .handle_capability_outcome(ctx, state, call, outcome, &mut capability_batch) + .handle_capability_outcome( + ctx, + state, + call, + outcome, + is_resume_origin, + &mut capability_batch, + ) .await?As per coding guidelines, auth/approval paths must fail closed; retry eligibility here must be derived from immutable per-invocation provenance, not mutable shared state.
Also applies to: 747-768
🤖 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_agent_loop/src/executor/capabilities.rs` around lines 638 - 679, The `is_resume_origin` flag derived from `state.pending_approval_resume` and `state.pending_auth_resume` at error-handling time in the executor/capabilities.rs file (around line 670) is vulnerable to being cleared by concurrent outcome processing before the deferred failure is handled, allowing a resumed failure to be treated as non-resume and permit unsafe retry dispatch. Instead of deriving resume origin from mutable shared state at error-handling time, capture the resume origin as immutable per-invocation provenance when the capability invocation is created, then reference that immutable captured flag throughout the error-handling path. This same fix must be applied at the secondary locations (lines 747-768) where `is_resume_origin` is used to guard retry eligibility, ensuring all retry guard checks use the immutable per-invocation provenance rather than querying mutable state.Source: Coding guidelines
crates/ironclaw_agent_loop/src/executor/tests.rs (2)
5395-5739:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
# Check the file size and line count wc -l crates/ironclaw_agent_loop/src/executor/tests.rs # Check if there's already an inline justification or decomposition issue comment grep -A5 -B5 "resume_origin_backend_failure_does_not_die_as_scope_mismatch\|decomposition\|arch-exempt" crates/ironclaw_agent_loop/src/executor/tests.rs | head -50 # Count exact lines being added (from 5395 to 5739 inclusive) sed -n '5395,5739p' crates/ironclaw_agent_loop/src/executor/tests.rs | wc -l # Check for any tracking issue or justification comment near the start of the added region sed -n '5390,5410p' crates/ironclaw_agent_loop/src/executor/tests.rsRepository: nearai/ironclaw
Length of output: 2232
🏁 Script executed:
# Also check the CLAUDE.md or coding guidelines repo structure to see the exact decomposition requirement find . -name "CLAUDE.md" -o -name "claude.md" | head -5Repository: nearai/ironclaw
Length of output: 265
🏁 Script executed:
# Search for how other test files handle this decomposition requirement grep -r "arch-exempt\|decomposition" crates/ --include="*.rs" | grep -i "test" | head -10Repository: nearai/ironclaw
Length of output: 1452
Add inline justification or file the decomposition issue for tests.rs expansion.
File now 5,739 lines; this PR adds 345 lines (>200-line threshold). Per CLAUDE.md: "Existing files > 3,000 lines must have a tracking issue filed for decomposition; PRs adding > 200 lines need inline justification."
Either add
// arch-exempt: large-test-file, regression tests for approval/auth-resume Backend failures (Part C-sub-A), plan#NNNN`` near the first test, or link the filed decomposition issue for this file.🤖 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_agent_loop/src/executor/tests.rs` around lines 5395 - 5739, The file tests.rs now exceeds 5,739 lines and this PR adds 345 lines (>200-line threshold), requiring justification per CLAUDE.md. Add an inline arch-exempt comment immediately before the first test function (resume_origin_backend_failure_does_not_die_as_scope_mismatch) in the format: `// arch-exempt: [reason], [tracking issue]`. In the reason, briefly explain that these are regression tests for the Part C-sub-A fix addressing scope_mismatch failures when approval-resume and auth-resume dispatches return Backend errors, and that they test distinct observable behavior requiring two separate test functions. Reference a decomposition tracking issue number if one exists, or use a placeholder like `plan `#TBD`` if filing will happen separately.Source: Coding guidelines
5491-5497:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the regression fail if resume dispatch is skipped.
Both tests can pass if Phase 2 ignores the pending resume, consumes the canned
reply_response(), and completes without exercising the Backend failure batch outcome. Assert the second batch invocation ran and that the Backend failure was appended as a tool-error result.Suggested assertion shape
assert!( matches!(phase2_exit, LoopExit::Completed(_)), "phase 2 must complete the run after Backend→ToolErrorResult; got {phase2_exit:?}" ); + let batch_invocations = host.batch_invocations(); + assert_eq!( + batch_invocations.len(), + 2, + "phase 2 must consume the approval-resume batch outcome" + ); + assert!( + batch_invocations[1].invocations[0].approval_resume.is_some(), + "phase 2 must dispatch with approval-resume context" + ); + assert!( + host.appended_result_refs().iter().any(|entry| entry + .safe_summary + .contains("transient backend error during cap1 resume")), + "backend failure must be surfaced as a tool-error result before the final reply" + ); // No single invoke_capability calls should have been made: the C-sub-A guard // prevents the retry dispatch entirely for resume-origin failures. assert!( host.single_invocations().is_empty(), @@ assert!( matches!(phase2_exit, LoopExit::Completed(_)), "phase 2 must complete the run after Backend→ToolErrorResult; got {phase2_exit:?}" ); + let batch_invocations = host.batch_invocations(); + assert_eq!( + batch_invocations.len(), + 2, + "phase 2 must consume the auth-resume batch outcome" + ); + assert!( + host.appended_result_refs().iter().any(|entry| entry + .safe_summary + .contains("transient backend error during cap1 auth-resume")), + "backend failure must be surfaced as a tool-error result before the final reply" + ); // No single invoke_capability calls should have been made: the C-sub-A guard // prevents the retry dispatch entirely for auth-resume-origin failures. assert!( host.single_invocations().is_empty(),As per coding guidelines, “Add a regression test with every bug fix” and “Test through the caller when a helper gates a side effect.”
Also applies to: 5541-5568, 5658-5666, 5711-5738
🤖 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_agent_loop/src/executor/tests.rs` around lines 5491 - 5497, Add assertions to each of the four test cases (at crates/ironclaw_agent_loop/src/executor/tests.rs lines 5491-5497, 5541-5568, 5658-5666, and 5711-5738) to verify that the Phase 2 batch invocation actually executed and that the Backend failure was properly appended as a tool-error result. These assertions must confirm the second batch call ran with the correct parameters and that the resulting tool error appears in the final state, ensuring the test will fail if resume dispatch logic is skipped or the failure handling is not exercised.Source: Coding guidelines
crates/ironclaw_reborn/src/loop_driver_host.rs (1)
2320-2326: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a regression test for
InvalidRunOriginAdaptermapping.Line 2320 adds a new host-error mapping branch, but the local mapping test module doesn’t lock this contract yet. Please add a case asserting
TurnError::InvalidRunOriginAdaptermaps toAgentLoopHostErrorKind::InvalidInvocation.Proposed test diff
#[cfg(test)] mod turn_error_to_host_error_tests { use super::*; use ironclaw_turns::{TurnCapacityResource, TurnError, TurnRunId}; + #[test] + fn invalid_run_origin_adapter_maps_to_invalid_invocation() { + let error = turn_error_to_host_error(TurnError::InvalidRunOriginAdapter); + assert_eq!(error.kind, AgentLoopHostErrorKind::InvalidInvocation); + } + #[test] fn capacity_exceeded_maps_to_unavailable() { let error = turn_error_to_host_error(TurnError::capacity_exceeded( TurnCapacityResource::SpawnTreeDescendants, 3,As per coding guidelines, “Every bug fix must include a regression test (
#[test]or#[tokio::test]) that reproduces the original failure.”Also applies to: 3233-3278
🤖 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_reborn/src/loop_driver_host.rs` around lines 2320 - 2326, Add a regression test case in the mapping test module that verifies the new error branch for TurnError::InvalidRunOriginAdapter correctly maps to AgentLoopHostErrorKind::InvalidInvocation. The test should assert that when a TurnError::InvalidRunOriginAdapter is encountered, it produces the expected AgentLoopHostErrorKind::InvalidInvocation output through the ironclaw_loop_support::raw_agent_loop_host_error function. Use the standard #[test] or #[tokio::test] attribute as appropriate for the test module structure.Source: Coding guidelines
crates/ironclaw_turns/src/run_profile/host.rs (1)
1393-1394:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Locate all construction and branching sites for the new provider-call marker. # Expectation: every `CapabilityInvocation` construction and every marker read is migrated # to the typed origin enum if this refactor is accepted. rg -n -C3 --type=rust '\bCapabilityInvocation\s*\{|\.is_provider_call\b|is_provider_call\s*:'Repository: nearai/ironclaw
Length of output: 50372
🏁 Script executed:
# Check if is_provider_call is ever read/matched/branched on, not just set rg -n '\bis_provider_call\b' --type=rust -A2 -B2 | grep -E '(if|match|==|!=|assert)' | head -30Repository: nearai/ironclaw
Length of output: 3892
🏁 Script executed:
# More targeted: find any code that reads the field (not construction) rg -n '\.is_provider_call\s*(?!:)' --type=rust | grep -v 'is_provider_call:' | head -20Repository: nearai/ironclaw
Length of output: 310
🏁 Script executed:
# Look for any conditional logic or reads on is_provider_call (not just struct field assignments) rg -n 'is_provider_call' --type=rust -B3 -A3 | grep -E '(if|match|==|!=|\?|assert|unwrap|expect|>|<)' | head -30Repository: nearai/ironclaw
Length of output: 4315
🏁 Script executed:
# Direct search: find lines where is_provider_call appears but NOT in struct init context rg -n 'is_provider_call' --type=rust | grep -v 'is_provider_call:' | grep -v 'is_provider_call: false,' | head -20Repository: nearai/ironclaw
Length of output: 601
🏁 Script executed:
# Get context around the conditional use at capability_port.rs:1216 sed -n '1210,1225p' crates/ironclaw_loop_support/src/capability_port.rsRepository: nearai/ironclaw
Length of output: 823
🏁 Script executed:
# Get context around the assignment in capability_helpers.rs sed -n '30,80p' crates/ironclaw_agent_loop/src/executor/capability_helpers.rsRepository: nearai/ironclaw
Length of output: 1938
Replace
is_provider_callboolean with typedCapabilityInvocationOriginenum.The field is branched on in
crates/ironclaw_loop_support/src/capability_port.rs:1216(match guard), and encodes a semantic mode (provider tool call replay vs. direct invocation). Per coding guidelines, use enums for modes instead of booleans. All construction sites either hardcodefalseor derive fromcall.provider_replay.is_some()— migration is straightforward.Suggested contract shape
+#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum CapabilityInvocationOrigin { + #[default] + IronclawCapability, + ProviderToolCall, +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CapabilityInvocation { pub surface_version: CapabilitySurfaceVersion, pub capability_id: CapabilityId, pub input_ref: CapabilityInputRef, - #[serde(default)] - pub is_provider_call: bool, + #[serde(default)] + pub origin: CapabilityInvocationOrigin,🤖 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_profile/host.rs` around lines 1393 - 1394, Replace the boolean field `is_provider_call` in the struct at the anchor location with a typed `CapabilityInvocationOrigin` enum that represents the semantic modes of provider tool call replay versus direct invocation. Define the enum with appropriate variants (e.g., one for direct invocation and one for provider replay), then update all construction sites that currently hardcode `false` or derive the value from `call.provider_replay.is_some()` to instead construct the appropriate enum variant. Finally, update the match guard branching logic in `crates/ironclaw_loop_support/src/capability_port.rs:1216` to pattern match on the enum variants instead of the boolean condition.Source: Coding guidelines
crates/ironclaw_turns/tests/agent_loop_host_contract.rs (2)
3723-3724:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd decomposition tracking/justification for this added block.
Line [3723] starts a large test expansion in a file already above 3,000 lines. Please add the required decomposition tracking issue reference and inline justification for adding >200 lines in this file.
As per coding guidelines, “Existing files > 3,000 lines must have a tracking issue filed for decomposition; PRs adding > 200 lines need inline justification.”
🤖 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/tests/agent_loop_host_contract.rs` around lines 3723 - 3724, Before the test block comment at line 3723 that marks the start of the ProductTurnContext serde round-trips tests, add an inline comment block that includes a reference to the decomposition tracking issue for this file (which exceeds 3,000 lines) and a clear justification explaining why adding this block of >200 lines is necessary. The justification should explain the purpose and importance of these new tests to provide context for the file size increase.Source: Coding guidelines
3851-3908:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe “byte_identical” regression does not assert byte/fingerprint identity.
Line [3851] claims byte-identical/fingerprint parity with the baseline, but the test only checks presence/absence of runtime lines. Add an explicit equality assertion for rendered runtime content and fingerprint against the baseline shape to prevent silent drift.
🤖 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/tests/agent_loop_host_contract.rs` around lines 3851 - 3908, The test function instruction_bundle_runtime_communication_none_is_byte_identical_to_4795_baseline claims to verify byte and fingerprint parity with a baseline but only validates the presence or absence of specific strings. To fix this, define or reference the expected baseline content and fingerprint values for the `#4795` baseline case, then add explicit equality assertions comparing the actual rendered runtime content (from runtime_msg.model_content) and the bundle's fingerprint to these baseline values. This ensures the test actually guards against unintended drift from the baseline rather than just checking for specific substrings.docs/plans/2026-06-15-provider-tool-input-scope-mismatch.md (1)
3-3:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve contradictory implementation status in the same plan.
Line 3 says the design is accepted and ready to implement, while Line 130 says final signoff is still pending. Pick one canonical state so execution ownership is unambiguous.
Also applies to: 130-130
🤖 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 `@docs/plans/2026-06-15-provider-tool-input-scope-mismatch.md` at line 3, The document contains contradictory implementation status statements: line 3 states the design is ACCEPTED and ready to implement, while line 130 states that final signoff is still pending. Review both locations in the document and choose one canonical implementation status, then update both line 3 and line 130 to consistently reflect that single state throughout the document so that execution ownership is unambiguous.Source: Coding guidelines
Collaborator
Author
Re: latest CodeRabbit review (Run ID
|
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_agent_loop/src/executor/loop_exit.rs`:
- Around line 73-77: The nudge infrastructure failures at build_prompt_bundle
(lines 73-77), the model call (lines 96-100), and transcript building (lines
125-129) in crates/ironclaw_agent_loop/src/executor/loop_exit.rs are currently
mapped with `map_err(|_| ...)` which erases error context, then bubbled up with
`?` which terminates execution fatally. These host errors should be non-fatal
and return Ok(None) instead, while preserving error details in logging and
explicitly propagating only cancellation signals. At each of the three affected
locations, replace the error mapping that uses `|_|` with explicit error
handling that logs the actual error context, checks for explicit cancellation to
propagate, and returns Ok(None) for non-fatal host unavailability rather than
using the `?` operator.
In `@crates/ironclaw_agent_loop/src/executor/tests.rs`:
- Around line 1652-1797: The existing nudge tests
(no_progress_nudge_synthesizes_reply_when_gate_enabled,
no_progress_skips_nudge_when_gate_disabled,
budget_iteration_limit_nudges_to_completed_when_gate_enabled,
nudge_respects_one_shot_cap) cover the happy path and gate/cap scenarios but do
not test failure modes. Add new test cases that verify the executor's behavior
when nudge internals fail (such as prompt generation, model call, or
finalization failures). These tests should exercise the real caller paths
(ExitStage and BudgetStage) with MockHost configured to return errors, and
assert that the executor falls back to normal exit behavior (using the canned
fallback) instead of propagating the nudge failure, while also verifying that
cancellation is still properly propagated when applicable.
🪄 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: 6a9b1798-db0b-448a-97a5-dd242c262204
📒 Files selected for processing (6)
crates/ironclaw_agent_loop/src/executor/budget.rscrates/ironclaw_agent_loop/src/executor/loop_exit.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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_agent_loop/src/executor/loop_exit.rs`:
- Around line 73-77: The nudge infrastructure failures at build_prompt_bundle
(lines 73-77), the model call (lines 96-100), and transcript building (lines
125-129) in crates/ironclaw_agent_loop/src/executor/loop_exit.rs are currently
mapped with `map_err(|_| ...)` which erases error context, then bubbled up with
`?` which terminates execution fatally. These host errors should be non-fatal
and return Ok(None) instead, while preserving error details in logging and
explicitly propagating only cancellation signals. At each of the three affected
locations, replace the error mapping that uses `|_|` with explicit error
handling that logs the actual error context, checks for explicit cancellation to
propagate, and returns Ok(None) for non-fatal host unavailability rather than
using the `?` operator.
In `@crates/ironclaw_agent_loop/src/executor/tests.rs`:
- Around line 1652-1797: The existing nudge tests
(no_progress_nudge_synthesizes_reply_when_gate_enabled,
no_progress_skips_nudge_when_gate_disabled,
budget_iteration_limit_nudges_to_completed_when_gate_enabled,
nudge_respects_one_shot_cap) cover the happy path and gate/cap scenarios but do
not test failure modes. Add new test cases that verify the executor's behavior
when nudge internals fail (such as prompt generation, model call, or
finalization failures). These tests should exercise the real caller paths
(ExitStage and BudgetStage) with MockHost configured to return errors, and
assert that the executor falls back to normal exit behavior (using the canned
fallback) instead of propagating the nudge failure, while also verifying that
cancellation is still properly propagated when applicable.
🪄 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: 6a9b1798-db0b-448a-97a5-dd242c262204
📒 Files selected for processing (6)
crates/ironclaw_agent_loop/src/executor/budget.rscrates/ironclaw_agent_loop/src/executor/loop_exit.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rs
🛑 Comments failed to post (2)
crates/ironclaw_agent_loop/src/executor/loop_exit.rs (1)
73-77:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winBest-effort nudge path can still hard-fail the run.
At Line 73, Line 96, and Line 125, host errors are mapped then bubbled, and callers use
?, so nudge infrastructure failures can terminate execution instead of falling back to normal no-progress / iteration-limit exits. This also drops root cause detail viamap_err(|_| ...).Treat nudge prompt/model/transcript failures as non-fatal (
Ok(None)) while preserving explicit cancellation propagation, and keep underlying error context when mapping.As per coding guidelines, fail-loud handling must preserve causes and explicitly forbids
.map_err(|_| OtherError)cause erasure; this also conflicts with the PR objective to avoid run-borking terminal failures.Also applies to: 96-100, 125-129
🤖 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_agent_loop/src/executor/loop_exit.rs` around lines 73 - 77, The nudge infrastructure failures at build_prompt_bundle (lines 73-77), the model call (lines 96-100), and transcript building (lines 125-129) in crates/ironclaw_agent_loop/src/executor/loop_exit.rs are currently mapped with `map_err(|_| ...)` which erases error context, then bubbled up with `?` which terminates execution fatally. These host errors should be non-fatal and return Ok(None) instead, while preserving error details in logging and explicitly propagating only cancellation signals. At each of the three affected locations, replace the error mapping that uses `|_|` with explicit error handling that logs the actual error context, checks for explicit cancellation to propagate, and returns Ok(None) for non-fatal host unavailability rather than using the `?` operator.Source: Coding guidelines
crates/ironclaw_agent_loop/src/executor/tests.rs (1)
1652-1797: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add nudge failure-path regressions at caller level.
These tests cover enable/disable/cap paths well, but they don’t assert fail-open behavior when nudge prompt/model/finalize fails. Please add cases proving executor falls back to normal exit behavior (and still propagates cancellation) when nudge internals fail.
As per coding guidelines, every bug fix should carry a regression test and helper-gated behavior should be tested through the real caller path.
🤖 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_agent_loop/src/executor/tests.rs` around lines 1652 - 1797, The existing nudge tests (no_progress_nudge_synthesizes_reply_when_gate_enabled, no_progress_skips_nudge_when_gate_disabled, budget_iteration_limit_nudges_to_completed_when_gate_enabled, nudge_respects_one_shot_cap) cover the happy path and gate/cap scenarios but do not test failure modes. Add new test cases that verify the executor's behavior when nudge internals fail (such as prompt generation, model call, or finalization failures). These tests should exercise the real caller paths (ExitStage and BudgetStage) with MockHost configured to return errors, and assert that the executor falls back to normal exit behavior (using the canned fallback) instead of propagating the nudge failure, while also verifying that cancellation is still properly propagated when applicable.Source: Coding guidelines
serrrfirat
added a commit
that referenced
this pull request
Jun 15, 2026
CodeRabbit (PR #4841): the merged-in final-answer nudge (#4837) mapped its prompt/model/transcript host calls with `map_err(|_| HostUnavailable)?`, which both erased the error cause AND hard-failed the run. That defeats the nudge's purpose (rescue an empty turn ending) and reintroduces exactly the run-borking this PR targets — especially with a flaky provider, where the nudge's own model call is the thing failing. - route the three nudge host calls through a `nudge_bail` helper: propagate only cancellation, otherwise log the actual cause (kind + safe_summary, never erased) and return Ok(None) so the caller keeps its normal exit - count the nudge attempt before any host call so a failing nudge can't retry - add three caller-level regression tests (ExitStage + BudgetStage): model failure falls back to canned/failed exit; cancellation still propagates Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
BenKurrek
reviewed
Jul 3, 2026
| } | ||
| } | ||
|
|
||
| state.recent_failure_kinds.push(LoopFailureKind::DriverBug); |
Collaborator
There was a problem hiding this comment.
we push DriverBug here but exit as ModelError at :247. verify_failure_evidence (loop_exit_applier.rs:413) checks that recent_failure_kinds contains the exit kind — and since only the first model error's kind is recorded, if that first error was InvalidOutput the list won't contain ModelError, so the failure gets downgraded to driver_protocol_violation and loses its category + resume checkpoint.
reachable at the 4×2=8 boundary (8 varied errors, InvalidOutput first) — uncommon but possible. fix is just to make the pushed kind and the exit kind agree. same pattern as capabilities.rs:1039.
BenKurrek
reviewed
Jul 3, 2026
| Failed { | ||
| failure: SanitizedFailure, | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| explanation_message_refs: Vec<LoopMessageRef>, |
Collaborator
There was a problem hiding this comment.
both of these new fields are dead in prod:
explanation_message_refsis dropped with..at both readers (memory/mod.rs:2836, filesystem_store.rs:1262), never read. the refs already reach the UI via the LoopFailed exit + thread history.resume_checkpoint_idis only set by a#[cfg(test)]builder, so it's alwaysNoneand retry always falls back tolatest_resumable_loop_checkpoint.
either wire them up or drop them. (safe_summary is fine, it's used.)
BenKurrek
reviewed
Jul 3, 2026
| /// for that key is lost until the operation is re-recorded. Surface it instead | ||
| /// of dropping silently. Logs metadata only — never the replay payload. | ||
| fn warn_malformed_idempotency_record(record: &TurnIdempotencyRecord) { | ||
| tracing::warn!( |
Collaborator
There was a problem hiding this comment.
this runs during snapshot load (engine-internal), and warn! corrupts the REPL per CLAUDE.md — should be debug!.
BenKurrek
reviewed
Jul 3, 2026
| result: Result<RetryTurnResponse, TurnError>, | ||
| created_at: crate::TurnTimestamp, | ||
| ) { | ||
| let replayable = !matches!(result, Err(TurnError::ThreadBusy(_))); |
Collaborator
There was a problem hiding this comment.
only ThreadBusy is excluded here, so a transient AdmissionRejected gets cached + replayed for the same idempotency key indefinitely. in practice the frontend mints a fresh client_action_id per click so a normal retry self-heals — this only bites a client that reuses the same key (which the 429 retryable:true invites). it's consistent with submit, so more of a consistency call: either exclude it here like ThreadBusy, or stop marking it retryable.
BenKurrek
reviewed
Jul 3, 2026
| thread_scope = ?thread_scope, | ||
| thread_id = ?scope.thread_id, | ||
| error = %err, | ||
| "webui submit ownership probe failed: thread not resolvable under caller scope; no run will be submitted" |
Collaborator
There was a problem hiding this comment.
nit: these debug logs look like they're for the separate "thread stops responding" issue, not the failure/retry work — could split them out to keep this PR scoped. non-blocking.
serrrfirat
added a commit
that referenced
this pull request
Jul 4, 2026
Maps every reborn run error to recoverable / run-borking, analyzes PR #4841 coverage, and lays out the path to the two-bucket end state (SecurityStop | Retriable | Explainable). Headline finding: the host_runtime disposition layer intends no capability failure to abort, but the recovery strategy aborts on Dispatcher/InvalidOutput/Unknown — re-bucketing that class makes "model called a nonexistent tool" and malformed-output failures recoverable. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
✅ IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Verdict: ❌ Changes requested
Findings: 1 blocking / 0 notes
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Head: 704a9e4367ed87db5df2abe28f90290d3f7e0796
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found one blocking regression in the new retry resume path: the rebase helper is applied to every checkpoint resume, including normal approval/auth gate resumes, and it clears same-run transcript/result refs.
Findings
1. ❌ [MEDIUM] Normal gate resumes lose accumulated refs
Location: crates/ironclaw_reborn/src/planned_driver.rs:152
PlannedDriver::resume now calls rebase_for_run unconditionally after loading any resumable checkpoint. That helper resets the input cursor and clears assistant_refs/result_refs, which is only appropriate for the new retry case where a different run is reusing a source run's checkpoint. The same resume path is also used for ordinary approval/auth gate resolution on the original run; in that case the refs are still valid and represent work completed before the gate. Clearing them means a run that blocks after earlier tool results resumes without those refs, so subsequent prompt context/final evidence can omit already-completed work. Gate resumes should preserve same-run state and only rebase/clear when the loaded checkpoint is actually from a different source run.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
| checkpoint_kind, | ||
| ) { | ||
| Ok(initial) => initial, | ||
| Ok(initial) => initial.rebase_for_run(run_context), |
Contributor
There was a problem hiding this comment.
This rebases every checkpoint resume, not just failed-run retry resumes. Ordinary approval/auth gate resumes on the same run also come through here, and rebase_for_run clears assistant_refs/result_refs, dropping pre-gate work from the resumed state. Please only clear/rebase for cross-run retry checkpoints, or make the helper preserve refs when the checkpoint is already scoped to run_context.run_id.
Contributor
🗂️ Archived IronLoop Review: reviewerThis result is from an older PR head and is no longer the active review.
Archived summaryFound a compile-blocking issue: the new required RebornServicesApi::retry_run method was not added to an existing test implementation. Archived findings
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_agent_loop/src/state.rs (1)
304-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment doesn't mention the new same-run early-return branch.
The doc comment (Lines 304-323) explains the reset-and-preserve-gate-state path for cross-run retries but says nothing about the early return at Line 325-327 that now short-circuits entirely when
input_cursor.is_for_run(context)— i.e. the intra-run gate-resume case where evenassistant_refs/result_refsare preserved. Given this exact branch was previously added/reverted per the PR history (broke approval/auth resume, then fixed), documenting why the two cases diverge here will save the next reviewer from re-litigating it.📝 Suggested doc addition
/// Gate-bound resume state (`last_gate`, `pending_approval_resume`, /// `pending_auth_resume`) is deliberately NOT cleared here: this same path /// (`PlannedDriver::resume` -> `from_checkpoint_payload().rebase_for_run()`) /// is what resumes a run after an approval/auth gate is resolved, and the /// pending-resume record is exactly the evidence that tells the loop to /// re-dispatch the gated capability. Clearing it drops the resumed /// invocation (regression: only the pre-gate call runs). The resume host /// re-validates the gate before honoring the record, so this is not a /// trust-boundary leak. + /// + /// When `input_cursor` already belongs to the target run (same-run + /// gate/auth resume, not a cross-run retry), this is a no-op: the whole + /// state — including `assistant_refs`/`result_refs` — is left untouched, + /// since those refs are genuinely owned by this run rather than a + /// foreign source run. pub fn rebase_for_run(mut self, context: &LoopRunContext) -> Self {🤖 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_agent_loop/src/state.rs` around lines 304 - 332, Update the doc comment on rebase_for_run to describe the new same-run early-return path handled by input_cursor.is_for_run(context). Clarify that this branch is for intra-run gate-resume and returns without rebasing or clearing assistant_refs/result_refs, while the existing reset logic applies only to cross-run retry rebases. Keep the explanation tied to rebase_for_run and the input_cursor.is_for_run(context) check so future readers understand why the two paths intentionally diverge.
🤖 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.
Outside diff comments:
In `@crates/ironclaw_agent_loop/src/state.rs`:
- Around line 304-332: Update the doc comment on rebase_for_run to describe the
new same-run early-return path handled by input_cursor.is_for_run(context).
Clarify that this branch is for intra-run gate-resume and returns without
rebasing or clearing assistant_refs/result_refs, while the existing reset logic
applies only to cross-run retry rebases. Keep the explanation tied to
rebase_for_run and the input_cursor.is_for_run(context) check so future readers
understand why the two paths intentionally diverge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0ba2ccc4-5ba4-49c5-911b-bc71fe800117
📒 Files selected for processing (1)
crates/ironclaw_agent_loop/src/state.rs
Contributor
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Verdict: ❌ Changes requested
Findings: 1 blocking / 0 notes
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Head: 84e4896fe65af55ab3bf931e650577dabd2dda5b
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found one concrete validation regression in the new failure-category compatibility path. I could not run Rust tests because cargo is not installed in this review environment.
Findings
1. ❌ [MEDIUM] SanitizedFailure deserialization accepts arbitrary colon categories
Location: crates/ironclaw_turns/src/status.rs:171-175
The compatibility branch says it normalizes only the historical host_stage_unavailable:model shape, but the guard accepts any single-colon value with non-empty sides and rewrites it to snake_case. That means JSON/persisted input like model:credentials_unavailable or foo:bar bypasses the stricter SanitizedFailure::new write-path validation and is minted as a valid category. Please restrict this branch to the exact legacy prefix/shape that needs migration, or keep rejecting other colon-delimited categories.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
Inline review fallback
Inline comment projection fell back to a body-only PR Review because GitHub rejected the inline payload.
Reason: Unprocessable Entity: "Line could not be resolved" - https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request
IronLoop preserved the inline review comment payloads below instead of dropping them.
Inline fallback 1: crates/ironclaw_turns/src/status.rs:171
This compatibility guard currently accepts any single-colon category and rewrites it to snake_case, even though the comment says only the historical host_stage_unavailable:model shape should be normalized. That lets malformed JSON/persisted input such as foo:bar bypass the strict category validator as foo_bar. Please narrow this to the exact legacy shape/prefix that needs migration.
railway-app
Bot
temporarily deployed
to
ironclaw-rebornneari / preview-4841
July 6, 2026 09:34
Inactive
Contributor
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Verdict: ❌ Changes requested
Findings: 1 blocking / 0 notes
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Head: bfc72b3a58075edaebf5f4d12bed39817c509c00
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found a compile-blocking issue: the new required RebornServicesApi::retry_run method was not added to an existing test implementation.
Findings
1. ❌ [HIGH] Update all RebornServicesApi implementors for retry_run
Location: crates/ironclaw_product_workflow/src/reborn_services.rs:1770-1774
Adding retry_run as a required trait method leaves tests/integration/webui_v2_router_smoke.rs's impl RebornServicesApi for MinimalWebuiServices without an implementation, so the root integration test target will fail to compile with a missing trait item. Add a rejecting retry_run stub there, as was done for the other test fakes.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
| request: WebUiResolveGateRequest, | ||
| ) -> Result<RebornResolveGateResponse, RebornServicesError>; | ||
|
|
||
| async fn retry_run( |
Contributor
There was a problem hiding this comment.
This new required method also needs to be added to the existing MinimalWebuiServices implementation in tests/integration/webui_v2_router_smoke.rs; otherwise that integration test target fails to compile with a missing retry_run trait item.
railway-app
Bot
temporarily deployed
to
ironclaw-rebornneari / preview-4841
July 6, 2026 09:49
Inactive
Contributor
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: cdd7551a21737c62745e21ddd2bcfed30ea82fbb
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete blocking issues found in the reviewed diff. The changes add failed-run retry plumbing, retryability projection, failure explanation summaries, and checkpoint rebasing with coverage across the affected turn, Reborn, product workflow, and WebUI layers.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
ironloopai
Bot
dismissed stale reviews from themself
July 6, 2026 09:54
Superseded by a later IronLoop approved review for this reviewer.
This was referenced Jul 6, 2026
serrrfirat
added a commit
that referenced
this pull request
Jul 7, 2026
… + #5389/#5390/#5403/#5613) (#5692) * reborn: add failure explanations and retryable failed runs * test(loop_support): set inline_messages on the contract-test LoopModelRequest #4841 added the `inline_messages` field (serde default) to LoopModelRequest but missed this one construction in the thread_loop_support_contract integration test, breaking that test target's compile. Production builds default it to Vec::new(); match that. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(reborn): resolve main-merge CI breaks on #4841 (retry_turn stub + finer failure categories) The main→#4841 merge surfaced two semantic conflicts the auto-merge missed: - Clippy: main added `TurnCoordinator::retry_turn`; the StaticTurnCoordinator test stub in openai_compat_serve/tests.rs didn't implement it. Add the stub (returns Unavailable, matching its other methods). - Test ironclaw_reborn: main's chaos tests (#5296) assert the coarse failure categories "driver_unavailable"/"model_error", but #4841 refined production to finer, accurate categories — a full checkpoint-state disk now yields "host_stage_unavailable_checkpoint" and an offline model provider yields "model_unavailable" (both deliberate named categories with dedicated failure_summary messages). Update the two stale assertions to match #4841's intended categorization. Verified: the two turn_runner_worker_full_reborn_fails_* tests pass; clippy ironclaw_reborn_composition --all-features clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Persist retry busy idempotency records * test(reborn): expect specific model unavailable failure * fix(reborn): preserve same-run checkpoint refs * fix(ci): update reborn compile drift * fix(ci): update composition test drift * feat(reborn): make model-fixable capability failures recoverable (batch 1) Turns recoverable→bork mis-mappings into model-visible tool errors so the agent self-corrects instead of the run dying. On top of #4841. - agent_loop keystone: capability_error_class re-buckets Dispatcher / InvalidOutput / Unknown(_) / non-exhaustive default from Permanent (Abort) to OperationFailed (ToolErrorResult), aligning with the host_runtime disposition layer (which never intends a capability failure to abort). Cancelled / Permanent stay terminal. This makes "model called a nonexistent tool" (UnknownCapability/UnknownProvider -> InvalidOutput) recoverable. - outbound_delivery: outbound_delivery_outcome routes recoverable RebornServicesErrorCode to Ok(Failed/Denied) (only Internal -> Err); fixed the safe_summary that interpolated the model-supplied target_id (Invariant 2); expired/not-yet-approved approval-lease arms -> Ok(Denied) instead of terminal. - host_runtime: malformed model-supplied SandboxProcessPlan -> recoverable Failed{InvalidInput} outcome (defense-in-depth; the live gate in loop_support's host_runtime_input_for_capability is fixed in a follow-up). Lib tests green: agent_loop 354, host_runtime 297, loop_support 337, reborn 259; outbound_delivery 26. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(loop_support): malformed sandbox plan is recoverable, not run-ending Completes the sandbox-plan fix. The live terminal gate is loop_support's host_runtime_input_for_capability: a malformed/invalid model-supplied SandboxProcessPlan returned AgentLoopHostError::InvalidInvocation, which capability_host_error maps to terminal HostUnavailable{Capability} (run dies). Now the invoke path downgrades that InvalidInvocation to a model-visible Ok(CapabilityOutcome::Failed{InvalidInput}) so the agent can correct the plan and the run continues. The helper only emits InvalidInvocation for the sandbox-plan parse/validation case; its host-internal serialization failure keeps its Internal Err. Updated both locked tests to assert the recoverable outcome. loop_support lib: 337 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(llm): provider error fidelity for accurate recover/explain (batch 2) Maps provider failures to the right LlmError variant so model-call errors are explained accurately and context overflow recovers via context-shrink instead of borking. - rig_adapter (OpenAI/Anthropic/Ollama/Tinfoil/openai_compatible): map_rig_error now detects auth failures (401/403/invalid key) -> AuthFailed (non-retryable, non-breaker-tripping) instead of generic RequestFailed, so a bad key surfaces as a credentials problem rather than wasted retries + opaque run-bork. - Codex (openai_codex_provider + codex_chatgpt): a stream ending without response.completed is now a retryable InvalidResponse/EmptyResponse instead of a silent successful Stop; codex_chatgpt now maps SSE error/response.failed events; both detect 413/context-overflow -> ContextLengthExceeded. - github_copilot + anthropic_oauth: detect 413 (and 400+context body) -> ContextLengthExceeded so context-shrink recovery fires (401/429/5xx untouched). ironclaw_llm lib: 915 passed, 0 failed. clippy + fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(host_runtime): unknown method/capability is immediate model-visible error (batch 3) Sweep finding: a method/capability the model named that does not exist (RuntimeDispatchErrorKind::MethodMissing / UndeclaredCapability) mapped to RuntimeFailureKind::Backend -> RetrySameCall, so it burned the retry budget before becoming model-visible. Retrying never resolves a nonexistent target. Now maps to InvalidInput -> ModelVisibleToolError: the model gets an immediate "no such method/capability" tool error and self-corrects. Updated the pinning table entries. host_runtime lib: 297 passed. Batch-3 sweep conclusion: after the keystone + batches 1-2, no remaining recoverable->bork CORRECTNESS defects exist (tool backends fully clean; no hard-Err bypass on model-fixable conditions; nothing mapped to terminal Cancelled/Permanent). This was the last shape-#3 quality nit worth fixing now. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(reborn): FailureLane classifier + two-bucket enforcement test Item #2 foundation, built ON TOP of #4841's failure-surfacing machinery (reuses category + FailureExplanationProvider + retryable rather than a parallel RunFailureReason taxonomy). - FailureLane enum (Retriable | Explainable | Security), wire-stable snake_case. - failure_lane(category, retryable): retryable -> Retriable, else Explainable. Security is reserved for the ingress safety/leak refusal path (minimal security-stop policy) and is never produced at the run boundary; the match on category is the seam for a future mid-run safety-abort category. - ALL_RUN_FAILURE_CATEGORIES: canonical list of every category the run boundary can produce. - ENFORCEMENT TEST (every_failure_category_is_explainable_and_classified): locks the two-bucket invariant — every failure category resolves to a SPECIFIC user explanation (never the generic fallback) AND a definite lane. A new category that forgets its sentence, or regresses to the generic fallback, fails here. Plus canonical_list_covers_loop_failure_kinds guards against list drift. reborn_composition failure_lane: 5 passed. clippy clean (the one pre-existing needless_return in local_runtime_profile.rs is unrelated). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(reborn): retry-disposition policy (hybrid retry core) Operationalizes the hybrid retry decision on top of the FailureLane classifier. Pure decision function; the auto-redrive scheduler is its consumer. - RetryDisposition { Auto | UserInitiated | NoRetry }, wire-stable snake_case. - retry_disposition(category, retryable): no checkpoint -> NoRetry; transient host/lease/store/provider/tool faults -> Auto (silent re-drive from checkpoint, bounded by the scheduler); model/provider/config/model-fixable faults -> UserInitiated (retry affordance; a silent re-drive would just re-fail). Conservative Auto allowlist (anything not clearly transient -> UserInitiated). - RetryDisposition::failure_lane() ties it back to FailureLane; a test asserts the two layers agree for every category in ALL_RUN_FAILURE_CATEGORIES. reborn_composition retry_disposition: 5 passed. clippy + fmt clean. Follow-up: the scheduler wiring that calls retry_disposition() to auto-requeue (the behavior-flipping "Auto" half) — this is its tested decision core. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(reborn): carry secret-scrubbed raw cause to the model Reborn over-sanitized capability/host failures: a real cause like `missing input_schema_ref at /system/extensions/.../list_calendars.input.v1.json` was collapsed to the generic "host runtime rejected capability request" because there was no model-visible field to carry the raw cause and the summary validator rejected any string containing `/`. Policy shift: redact secret VALUES only; let paths, codes, schema refs, and raw error text reach the model so it can retry or explain. Foundation + Tier-1 vertical: - AgentLoopHostError gains an optional model-visible `detail: Option<String>` channel (+ `with_detail`). - CapabilityFailureDetail gains a free-text `Diagnostic { text }` variant. - ToolObservationDetail::GenericFailure gains a bounded, leniently-validated `detail` (allows `/ { } [ ] < >`, rejects NUL/control + caps length) — the channel that already reaches the model and bypasses the strict summary validator. - Relax ONLY the false-positive word bans in validate_loop_safe_summary and validate_tool_result_safe_summary (drop "provider error", "stack trace", "tool input", "traceback", "host path", "raw runtime", "invalid api key"); keep the delimiter ban, control-char ban, length cap, and credential markers. - Tier-1 producers stop dropping the cause: raw_agent_loop_host_error threads the value-scrubbed raw_detail into AgentLoopHostError.detail; the runtime model-visible failure path carries a value-scrubbed Diagnostic when the strict summary validator drops the reason; capability_helpers forwards the diagnostic into the model-visible observation. - Boxed ProviderArgumentError.error to keep result_large_err quiet after the AgentLoopHostError/CapabilityFailureDetail size growth. Tests cover the anchor (path string reaches the model-visible detail), secret value redaction, the relaxed/retained summary markers, and legacy GenericFailure JSON round-trip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(reborn): MCP per-cause error tokens + explainer detail plumbing (tiers 2a, 3.2) - ironclaw_mcp: replace the flat "response_error"/"request_denied" literals with per-cause diagnostic tokens (mcp_http_status_<code>, mcp_jsonrpc_error code=..., mcp_parse_failed, ...), bounded + control-char-stripped, no public signature change. The model now learns the real HTTP status / JSON-RPC code. - reborn_composition: FailureExplanationInput gains a `detail` field rendered into the failure-explanation prompt (secret-scrubbed via sanitize_model_visible_text). Wired end-to-end in the projection; sourced once TurnLifecycleEvent carries detail (upstream chain in a follow-up commit). mcp --lib 18 passed; reborn_composition --lib failure_explanation tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(reborn): thread secret-scrubbed failure detail to the explainer (tiers 2b, 3.1) Complete the model-error-detail chain so the failure explainer (and the model) receive the real cause of a model/provider/driver fault instead of only a sanitized category. Only secret VALUES are withheld (scrubbed via the existing value-level redactors); the descriptive cause now flows end-to-end. Carrier `detail: Option<String>` (serde default + skip_serializing_if, so pre-detail persisted rows rehydrate as None) added and threaded through: - ironclaw_loop_support: HostManagedModelError.detail + with_detail; threaded in model_gateway_error into AgentLoopHostError.detail. - ironclaw_agent_loop: AgentLoopExecutorError::HostUnavailableWithDiagnostics gains detail; model-stage construction carries error.detail. - ironclaw_turns: AgentLoopDriverError::Failed.detail; TurnLifecycleEvent.detail (Failed events only, via failure_detail_for_event in the runner/memory path). - ironclaw_reborn: map_provider_error puts the scrubbed provider reason into HostManagedModelError.detail; planned_driver carries HostUnavailable detail into AgentLoopDriverError::Failed; turn_runner/turn_run_executor carry it into the failure record. - ironclaw_reborn_composition: detail_for_turn_event sources from event.detail, feeding the FailureExplanationInput.detail already rendered in the explainer prompt. Construction-site churn: detail added to TurnLifecycleEvent / AgentLoopDriverError test fixtures and the event_projections pending-gate test support. Verified per crate (--lib): turns 355, loop_support 342, agent_loop 261, reborn 175, event_projections 26 — all green; composition --lib 1042 passed (1 pre-existing live_progress_stream failure, unrelated). turns integration contracts compile; clippy clean across the chain. Pre-existing base-branch breakage in loop_support thread_loop_support_contract (inline_messages) is unrelated to this change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(turns): allow secret-scrubbed model-visible detail on Failed events The detail channel (TurnLifecycleEvent.detail) intentionally carries a secret-scrubbed description of the real failure cause to the model/explainer; update the guardrail so the spec matches the behavior. Only secret values are withheld; raw unscrubbed backend strings still stay behind host adapters. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): carry detail on AgentLoopDriverError::Failed in integration tests The tier-2b/3.1 detail field on AgentLoopDriverError::Failed broke construction and pattern sites in reborn's integration test targets (concurrent_workers, loop_driver_host) that the original --lib gate never compiled. Constructions get detail: None; the driver_host_error helper carries error.detail; exhaustive match patterns bind detail: _. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): update integration contracts for per-cause MCP tokens + detail channel Two integration test targets asserted pre-refinement model-visible strings that the --lib gate never compiled (test-through-the-caller gap): - mcp_adapter_contract: 5 assertions expected the flat "response_error"/ "request_denied" tokens; update them to the per-cause tokens the Tier 2a change now emits (mcp_invalid_protocol_version, mcp_jsonrpc_id_mismatch, mcp_invalid_session_id, mcp_http_status_500, mcp_denied_credential_source). - llm_gateway: the offline-provider test asserted the error Debug leaked NO provider detail at all. Tier 2b deliberately surfaces the secret-scrubbed non-secret cause on the detail channel. Rewrite (and rename) the test to the current policy: assert the non-secret reason ("connection refused", endpoint URL) reaches the model via `detail`, while the credential token (sk-provider-secret) is scrubbed from both `detail` and the full Debug. This strengthens the secret-scrubbing guard. mcp full suite green; llm_gateway full suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(host_runtime): missing first-party handler is InvalidInput, not Backend first_party_missing_handler_fails_closed_without_side_effect_handler asserted the dispatch failure kind was Backend, but #5389 deliberately reclassified an UndeclaredCapability/MethodMissing dispatch failure (a capability the model named that has no registered handler) to InvalidInput — a model-fixable, model-visible tool error that must not burn the retry budget on a call that can never resolve by retrying (see the From<DispatchFailureKind> mapping in production.rs). The test still fails closed (Failed outcome) and still carries "dispatch failed: UndeclaredCapability"; only the kind assertion is updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): LoopFailureKind fault matrix — executor layer + exhaustive category table Phase 1 of the per-error coverage harness (docs/plans/2026-07-03-loop-failure-matrix.md): - turns: all_failure_kinds category table now exhaustive (13/13 — adds the previously-missing CheckpointUnavailable + CompactionUnavailable) with a same-crate exhaustive-match guard so a new variant breaks compilation. - agent_loop: new table-driven executor failure matrix (executor/tests/failure_matrix.rs) driving every executor-reachable LoopFailureKind at its real origin via MockHost seams, asserting per row: P1 (reason_kind + sanitized category/safe_summary), P3 (no fabricated final assistant reply), and explanation_message_refs presence per the explainable set. New fail_transcript_with test knob on MockHost + DriverMockHost (test code only). - Four divergences found and documented (doc §5a), asserted as actual behavior, none silently fixed: Approval+SkipAndContinue completes (gate enforcement gap), NoProgressDetected missing its explanation attach, single Denied recovers-and-completes (no-borking working as designed), TranscriptWriteFailed/CheckpointRejected legacy-only enum origins. Validated (bounded): cargo test -p ironclaw_turns all_failure_kinds; cargo test -p ironclaw_agent_loop failure_matrix; check/clippy -D warnings/ fmt on ironclaw_agent_loop — all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): LoopFailureKind fault matrix — binary/driver layer rows Phase 2 of the per-error coverage harness (docs/plans/2026-07-03-loop-failure-matrix.md): - planned_driver: resume with missing checkpoint payload asserts LoopFailureKind::CheckpointUnavailable + "checkpoint_unavailable"; in-flight model Cancelled (no cooperative cancel signal) asserts map_executor_error yields "interrupted_unexpectedly". - e2e: binary-level divergence lock — the same in-flight Cancelled run projects "driver_failed" at the runner boundary (category overwritten; doc §5a.5, candidate follow-up to preserve the driver-mapped category). - e2e: non-model P4 row — capability-stage invocation failure is retryable ("host_stage_unavailable_capability", checkpoint preserved, no fabricated reply) and retry_run resumes to completion, so P4 is no longer proven only through the model stage. New scripted capability-invocation-error mode in the test harness (test support only). Validated (bounded): targeted planned_driver tests + full reborn_failure_retry_resume_e2e (15 passed) + clippy -D warnings + fmt. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): reconcile fault matrix to the recoverability stack (#5389/#5390) This matrix PR sits on top of the recoverability stack (main←#4841←#5389←#5390←#5403), so it validates the FIXED + classified system rather than #4841's base behavior: - Add executor rows for #5389's model-fixable capability failures that are now RECOVERABLE (InvalidInput / InvalidOutput / PolicyDenied): where the base branch terminated the run, the stack now surfaces a model-visible tool error and the loop completes. Rows assert the recovered outcome. - Add FailureLane / RetryDisposition alignment (binary/e2e layer, since ironclaw_agent_loop can't depend on ironclaw_reborn_composition): each real failure path's (category, retryable) is asserted to map to the expected #5390 FailureLane bucket + RetryDisposition — proving the real paths feed the classifier correctly. Complements #5390's classifier unit tests (which test the functions directly) rather than duplicating them. - Doc: §6 relationship to #5390; §5a marks divergences RESOLVED by the stack (capability recoverability) vs still-open (Approval SkipAndContinue completes; NoProgressDetected lacks explanation; Transcript/Checkpoint are planned-executor host errors; interrupted_unexpectedly projected as driver_failed at the runner boundary). The cherry-picked base matrix assertions still passed unchanged on the stack — they assert actual behavior, so the additive fixes did not break them; this commit adds the stack-specific coverage on top. Validated (bounded, on the stack): cargo test ironclaw_turns all_failure_kinds; cargo test ironclaw_agent_loop failure_matrix; cargo test --test reborn_failure_retry_resume_e2e (19 passed); clippy -D warnings; fmt --check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): address gemini failure matrix comments (#5613) * style: cargo fmt on batch-1 recoverable-error changes Reproduces the stack's skipped fmt commit (c19db45) against the restacked base. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tests): restack reconciliation — retry_run stub + superseded IssueCode import - webui_v2_router_smoke's MinimalWebuiServices gained the retry_run rejecting stub the trait now requires (base #4841 break: #5633's smoke fake predates #4841's retry_run addition; every other RebornServicesApi fake already has it). - drop CapabilityInputIssueCode from the ironclaw_turns re-export and ironclaw_loop_support import: the restacked base's CapabilityInputIssue carries DispatchInputIssueCode instead, and the stack's name was import-only on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(restack): outbound set-target routes through outbound_delivery_outcome; MCP tests use Option error info - set-target handler: replace the superseded #5445 NotFound special-case + outbound_delivery_host_error (deleted by the recoverability batch) with the outbound_delivery_outcome disposition the stack pins in unit tests; matches the list handler. - parse_mcp_response framing tests (base-side, written against the old `error: bool`): assert against the stack's richer `Option<JsonRpcErrorInfo>` — same intent, error presence still pinned. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(restack): turn_stream_auth fake event carries the new detail field Base-side projection test predates the stack's TurnLifecycleEvent.detail addition; None matches the auth-gate fixture's intent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): align failure_category_demasked pin to the fidelity taxonomy TraceLlm exhaustion (gateway cannot serve the call) maps to ModelErrorClass::Unavailable -> "model_unavailable" under the batch-2 provider-error fidelity mapping; the scenario's "model_error" pin predated it. The scenario's intent — the de-masked TRUE category survives, never the "driver_protocol_violation" sentinel — is unchanged and still asserted exactly. Capability-surface direction (not a security boundary): both categories are classified, retriable lanes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(agent_loop): run fault-matrix rows on 16MiB threads The restacked executor's future (failure explanations + digests + the recovery re-entry) outgrows the 2MiB default test-thread stack in debug on the in-run recovery rows. Production loop threads run 8MiB stacks (ironclaw_reborn_cli serve runtime); mirror the repo's big-stack test-thread pattern (traces/tests.rs, process_port.rs) per row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(host_runtime): egress contract pins the per-cause MCP denial token The SecretStoreLease-over-production-egress denial now surfaces mcp_denied_credential_source (McpRequestDeniedCause::DeniedCredentialSource) instead of the flat request_denied. Deny-before-transport is unchanged and still asserted (zero recorded requests); ironclaw_mcp's own adapter contract pins the same token. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(review): strip model-visible failure detail from public run-state + align feature-gated tests Security (IronLoop HIGH): RebornGetRunStateResponse forwarded SanitizedFailure.detail — free-form, model-visible backend cause text, scrubbed only for secret VALUES — straight to the browser. Add SanitizedFailure::public_projection() (keeps category, drops detail) and project the public WebUI shape through it. Regression tests: the strip helper (status.rs) and the caller (get_run_state contract test now programs a detail-bearing failure and asserts the DTO omits it). Inherent to the stack, not the reconciliation (original top-of-stack forwarded it raw too). Feature-gated test alignments (only run under --all-features/libsql, so the default-feature local sweep missed them; CI crate buckets caught them): - factory web-access: missing first-party handler is InvalidInput, not Backend (#5389 reclassify; capability still fails closed, only disposition changed). - outbound local_dev (x2): set-target routes through outbound_delivery_outcome (matches original top-of-stack), so the missing-target summary is the fixed "invalid outbound delivery request"; error_kind stays recoverable InvalidInput. - ironclaw_mcp parse_mcp_response_rejects_empty: per-cause tokens replace the flat "response_error" (mcp_parse_failed / mcp_no_payload). Also: cargo fmt (capability_port import reflow from the restack edit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(review): drop untrusted MCP server error message from model-visible reason + align tool_call category Security (IronLoop HIGH): parse_json_rpc_error_info copied the untrusted MCP server JSON-RPC error.message into the McpClientError::Client reason (model-visible, 'stable sanitized reason') with only length-bounding — not redaction. MCP servers can echo request args, paths, provider diagnostics, or credential-shaped values. Remove message end-to-end (JsonRpcErrorInfo field, parse, cause variant, render); keep only the standardized protocol code=<n>, which is the safe diagnostic. No redaction util is reachable from this leaf crate, and the stable-reason surface should carry stable tokens, not free text. Regression: the former 'reason carries message' test now asserts the message does NOT leak. Feature-gated integration test (ran only under --features libsql): tests/integration/tool_call.rs disabled-spawn-subagent-called-anyway now asserts 'model_unavailable' (InvalidOutput -> Unavailable fidelity category), not the stale 'model_error'. The security property (disabled capability never dispatched) is unchanged and still asserted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): cancel-path provider error is model_context_overflow, not model_error fail_model() -> ErrLlm -> LlmError::ContextLengthExceeded, which the batch-2 provider fidelity mapping now categorizes as the accurate model_context_overflow (was the generic model_error). Both cancel tests still pin the load-bearing behavior: reaches Failed after bounded context-shrink recovery (no retry-forever), and the per-thread busy lock releases on Failed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(merge): implement retry_turn on TurnCoordinator doubles added by main Merging origin/main brought two new TurnCoordinator test doubles (UnusedTurnCoordinator in src/runtime.rs, SpyTurnCoordinator in tests/runtime.rs) that predate this stack's retry_turn addition to the TurnCoordinator trait. Add the impls (unimplemented!/delegate, matching each double's existing method style) so the merged tree compiles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(deny): ignore RUSTSEC-2026-0204 (crossbeam Debug-fmt invalid deref) Newly published advisory (after this branch and main), transitive via crossbeam-epoch. The affected path is the `fmt::Pointer`/`Debug` impl for `Atomic`/`Shared` when the pointer is already invalid — a formatting path we do not exercise. Ignore with justification per the existing advisories convention; remove when the fixed crossbeam-utils release propagates. Verified `cargo deny check advisories` = ok locally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(reborn): preserve scrubbed failure detail into TurnRunExecutorError (IronLoop) The driver-failed Err path in execute_claimed_run converted the computed SanitizedFailure back to TurnRunExecutorError::new(category), dropping the scrubbed model-visible detail. The scheduler records error.failure(), so production driver failures persisted only the category and TurnLifecycleEvent.detail stayed empty — the failure explainer got the fallback summary instead of the real provider/model cause. Add TurnRunExecutorError::from_failure(SanitizedFailure) (the struct already holds a full SanitizedFailure) and use it at the call site so detail survives across the host-runtime boundary. Caller regression test: driver Failed{detail: Some(..)} -> execute_claimed_run -> err.failure().detail() is preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(reborn): JSON-frame untrusted failure detail in the explainer prompt (IronLoop) detail is untrusted provider/tool/runtime error text (e.g. MCP server or provider bodies). sanitize_model_visible_text redacts credential tokens but keeps newlines/instructions, so appending it raw let a crafted error inject extra prompt fields or directives (a fake fallback_summary:, an 'ignore previous instructions') into the failure explainer — whose output becomes the public failure_summary. That is a prompt-injection path into user-visible messaging, widened by the detail-preservation fix. Frame detail as data: JSON-string-escape it so newlines/quotes are escaped and it stays a single quoted 'detail: "..."' value. failure_category and fallback_summary are host-authored (category-derived) and unchanged. Regression test: a detail embedding newline+fallback_summary+directive is neutralized (exactly one real fallback_summary line, no directive line, detail present as an escaped quoted value). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This branch was previously 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
3 participants
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.
Goal
Eliminate "run-borking" terminal errors in the reborn binary. Today a run that hits
HostUnavailable, a model failure, or a capability protocol error dies with an opaque code and no recovery path. This PR moves the stack toward: every run-terminal error is either recovered, explained to the user by the model, or explained by a deterministic template — and is retryable from the last good checkpoint when one exists.Design doc:
docs/plans/2026-06-12-reborn-no-borking-failures.md.How this was built (multi-agent, Codex CLI)
The design doc decomposes Part 1 into 5 crate-disjoint workstreams (WS-1 → {WS-2, WS-3, WS-4} → WS-5). WS-2/3/4 were implemented in parallel by Codex CLI sub-agents:
one agent per workstream, each scoped to its own crates, each TDD-first. Their reports were reviewed, committed per-workstream, then a max-effort
/code-reviewpass over the workstream commits drove a final fix wave.Workstreams
ironclaw_turns):LoopFailedgainsexplanation_message_refs+safe_summary(serde-default, legacy round-trip tested);TurnRunnerOutcome::Failedcarries explanation refs +resume_checkpoint_id;RetryTurnRequest/TurnCoordinator::retry_turncontract +TurnError::RunNotRetryable.ironclaw_agent_loop): best-effort Tier-1 explanation — one constrained, no-capability model call finalized to the transcript before explainableLoopExit::Failedconstructions (capability abort, iteration limit, gate aborts, compaction, stop-aborted). Failed exits now carry partial assistant refs + diagnostics instead of discarding state. Model-unreachable kinds skip Tier 1 by design.ironclaw_turns/ironclaw_reborn/ironclaw_loop_support):retry_turnin both store backends — validates failed/latest/checkpointed runs, spawns a new claimable run seeded via a metadata-only checkpoint link (payload shared, failed run untouched), reacquires the thread lock, idempotent replay. Runner maps driverUnavailable/Failedinto retryable failures withhost_stage_unavailable:<stage>categories.ironclaw_reborn_composition/ironclaw_product_workflow/ironclaw_webui_v2):FailureExplanationProvidercovers everyLoopFailureKind, reborn failure category, andhost_stage_unavailable:*with actionable sentences + a safe generic fallback (completeness-tested). Product-workflowretry_runfacade +webui_v2retry endpoint with idempotent replay.Code-review findings fixed
Failedinstead ofCancelled. Now propagates hostCancelled; regression test added.SanitizedFailurevalidation (a::b,:x,x:) — now rejected; charset tests added.attach_failure_explanationchokepoint.retryablethreaded end-to-end (TurnLifecycleEvent→ composition projection →ProductProjectionItem::RunStatuswire), closing the WS-4 deviation; cross-crate projection test +host_stage_unavailable:unknowncompleteness coverage added.Tests / regression coverage
TDD at every layer: WS-2 executor tests (explanation finalized; model-error degrades cleanly;
ModelErrormakes no extra call; cancellation pre-empts/propagates), WS-3 store-contract + runner tests (retry spawns claimable run; not-retryable typed errors; driver-unavailable → categorized retryable failure), WS-4 projection completeness + facade/endpoint tests, plus a cross-crate retryable-projection test (turns event → composition → wire item).Gate:
cargo fmtclean ·cargo clippy --all --tests --all-featureszero warnings · per-crate suites green (ironclaw_turns303,ironclaw_agent_loop135, composition projection, product_adapters/workflow/event_projections/loop_support, reborn, webui_v2). One pre-existing parallel-execution flake inironclaw_reborn/tests/secrets.rs(libsql/keychain contention; passes in isolation; untouched by this branch).🤖 Generated with Claude Code