Repository navigation
Fix compaction failures after tool results - #5895
Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
github-actions
Bot
added
size: M
📝 WalkthroughSummary by CodeRabbit
WalkthroughCompaction errors and inference failures now defer compaction and continue through the existing prompt path. Tests, integration milestone assertions, failure-category coverage, and the frozen compaction specification reflect this non-terminal behavior. ChangesDeferred compaction continuation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ToolExecution
participant PromptCompactionStep
participant LoopState
participant ModelTurn
ToolExecution->>PromptCompactionStep: forced compaction after tool results
PromptCompactionStep->>LoopState: emit failure and record deferred watermark
PromptCompactionStep->>LoopState: clear forced compaction
PromptCompactionStep->>ModelTurn: continue with existing prompt
ModelTurn-->>ToolExecution: finalize assistant reply
Possibly related issues
🚥 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}
There was a problem hiding this comment.
Code Review
This pull request modifies the agent loop executor to make prompt compaction failures recoverable. Instead of terminating the execution with a failure exit, the loop now continues on the normal prompt path using the existing prompt candidate. Corresponding unit and integration tests have been updated or added to verify this fallback behavior and to track compaction milestones. The review feedback suggests improving the test assertions in tests/integration/http_matcher.rs by replacing a generic .is_err() check with a specific typed error assertion using .expect_err() to prevent false positives from unrelated infrastructure issues.
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.
| assert!( | ||
| h.assert_conversation_history_role_contains( | ||
| MessageKind::Summary, | ||
| "ignore previous instructions" | ||
| ) | ||
| .await | ||
| .is_err(), | ||
| "unsafe compaction summary must not be persisted" | ||
| ); |
There was a problem hiding this comment.
Using a generic .is_err() assertion here can lead to false positives if the harness encounters an unrelated database or infrastructure error. Additionally, avoid using string-matching on error messages (such as err.to_string().contains(...)) to classify failures. Instead, assert against the specific expected error variant using typed checks (e.g., matches!), and prefer using .expect_err() with a descriptive message instead of .unwrap_err() to ensure failures are explicitly reported with clear context.
let err = h
.assert_conversation_history_role_contains(
MessageKind::Summary,
"ignore previous instructions",
)
.await
.expect_err("expected conversation history assertion to fail");
assert!(
matches!(err, ConversationError::NotFound),
"expected NotFound error, got: {err:?}"
);References
- Avoid using generic
is_err()assertions in tests when verifying that an operation fails. Instead, assert against the specific expected error message or variant to prevent infrastructure or harness-level failures from causing false positives. - Avoid using string-matching on error messages (e.g.,
err.to_string().contains(...)) to classify or allow-list failures in tests. Prefer typed checks or introducing typed seams in the test harness to prevent unrelated failures from being silently retried or swallowed. - In Rust tests, prefer using
.expect()with a descriptive message instead of.unwrap()or fallbacks likeunwrap_or_else()to ensure that failures are explicitly reported with clear context.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ✅ Approved | 0 | 0 | 0 | 868957fb1c81 |
Head: 868957fb1c81ad787e91a6f2f4633e2e0833f294
Next: No reviewer action needed.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete actionable issues found in the compaction-failure recovery change or its test/support updates.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/failed/superseded state while reviewers run.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/executor/prompt.rs (1)
505-530: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate deferred-compaction state mutation between
Deferredbranch andcompaction_failed_continue.The
LoopCompactionOutcome::Deferredarm (lines 512-530) and the newcompaction_failed_continuehelper (lines 657-669) both: clearforce_compact_on_next_iteration, build an identicalDeferredCompactionWatermark { through_seq: drop_through_seq, prompt_fingerprint: state.compaction_prompt.fingerprint() }, runcancel_if_requested_after_pending_input_ack, and returnSkipped(state). Only the progress-event emission differs. Any future change to the watermark/cancellation sequencing now has two call sites to keep in sync.♻️ Proposed refactor: extract the shared tail into one helper
+async fn defer_compaction( + ctx: StageContext<'_>, + mut state: LoopExecutionState, + pending_input_ack: &mut PendingInputAck, + drop_through_seq: u64, +) -> Result<PromptCompactionOutcome, AgentLoopExecutorError> { + state.compaction_state.force_compact_on_next_iteration = false; + state.compaction_state.last_deferred = Some(DeferredCompactionWatermark { + through_seq: drop_through_seq, + prompt_fingerprint: state.compaction_prompt.fingerprint(), + }); + state = match CheckpointStage + .cancel_if_requested_after_pending_input_ack(ctx, state, pending_input_ack) + .await? + { + CancelCheck::Continue(state) => *state, + CancelCheck::Exit(exit) => return Ok(PromptCompactionOutcome::Exited(exit)), + }; + Ok(PromptCompactionOutcome::Skipped(state)) +}Then both the
Deferredarm andcompaction_failed_continuecalldefer_compaction(...)after doing their own (differing) progress-event emission.As per coding guidelines, "Keep functions focused and extract helpers when logic is reused."
Also applies to: 639-670
🤖 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/prompt.rs` around lines 505 - 530, The Deferred compaction handling is duplicated between the LoopCompactionOutcome::Deferred branch in PromptCompactionOutcome processing and the compaction_failed_continue helper. Extract the shared tail into a single helper, such as defer_compaction, that clears force_compact_on_next_iteration, sets last_deferred with the DeferredCompactionWatermark built from drop_through_seq and state.compaction_prompt.fingerprint(), performs cancel_if_requested_after_pending_input_ack, and returns Skipped(state). Keep only the progress-event emission differences in the existing call sites and route both paths through the shared helper.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/executor/prompt.rs`:
- Around line 505-530: The Deferred compaction handling is duplicated between
the LoopCompactionOutcome::Deferred branch in PromptCompactionOutcome processing
and the compaction_failed_continue helper. Extract the shared tail into a single
helper, such as defer_compaction, that clears force_compact_on_next_iteration,
sets last_deferred with the DeferredCompactionWatermark built from
drop_through_seq and state.compaction_prompt.fingerprint(), performs
cancel_if_requested_after_pending_input_ack, and returns Skipped(state). Keep
only the progress-event emission differences in the existing call sites and
route both paths through the shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 13263780-d8f5-4553-b836-6e95f1196692
📒 Files selected for processing (7)
crates/ironclaw_agent_loop/src/executor/prompt.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/executor/tests/failure_matrix.rscrates/ironclaw_agent_loop/tests/executor_happy_paths.rstests/integration/http_matcher.rstests/integration/support/builder.rstests/integration/support/group.rs
|
🚅 Deployed to the ironclaw-pr-5895 environment in ironclaw-ci-preview
|
…ty guard PR #5895 (868957f) removed compaction_failure_category from the agent loop, changing non-cancellation compaction failures from a terminal exit to a deferred continuation (issue #5838, design doc section 5). The composition-core safe-summary parity guard still expected the agent loop to mint the 7 granular compaction_* categories, which are no longer produced. crate::failure_summary keeps display support for them so historical failure records still render. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deduplicate the identical tail of the Deferred outcome arm and compaction_failed_continue into a crate-private defer_compaction helper; each caller emits its own progress event then delegates. Behavior-preserving: mutate-then-cancel-check ordering unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
size: L
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.25% — 292535 / 343140 lines Per-crate breakdown (63 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Make failed forced compaction recoverable after tool results while preserving cancellation semantics so the post-tool model turn can complete.
Stats: 6 findings (from 9 raw, 6 after dedup) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Security
- Medium Timeout recovery leaves compaction inference running beside model call (
crates/ironclaw_agent_loop/src/executor/prompt.rs:530-542, confidence 94) — anchor: crates/ironclaw_agent_loop/src/executor/prompt.rs:534
When the outer compaction timeout wins, the pinned future is dropped and the new recovery path immediately continues. In production, GuardedSystemInferencePort has already spawned a worker whose JoinHandle is then detached, so its model request and post-accounting can remain active for up to another deadline while the normal model turn starts. Repeated timeout recoveries can accumulate concurrent model work and exhaust gateway or connection-pool capacity.
Fix: Make the compaction inference worker abort-safe and await its termination before returning the recoverable timeout outcome. - High SecurityRejected compaction failures bypass the safety gate (
crates/ironclaw_agent_loop/src/executor/prompt.rs:519-528, confidence 95) — anchor: crates/ironclaw_agent_loop/src/executor/prompt.rs:519
SecurityRejected is emitted when transcript input contains injection or secret-leak markers, but this branch routes it through defer_compaction and sends the already-built prompt to the next model turn. Newly returned malicious tool content can therefore reach the model unsanitized, enabling prompt-injection tool calls or secret disclosure.
Fix: Distinguish input-security rejection from sanitized-summary rejection; block or sanitize the former and only recover from safe operational/output failures.
Tests
- Medium Cancellation test does not verify deferred state persistence (
crates/ironclaw_agent_loop/src/executor/prompt.rs:653-665, confidence 95) — anchor: crates/ironclaw_agent_loop/src/executor/prompt.rs:659
defer_compaction mutates the force flag and deferred watermark before the cancellation boundary writes the Final checkpoint. The cancellation test only checks the cancelled exit and prompt count, so it would pass if those mutations were lost from the staged checkpoint.
Fix: Add executor coverage for compaction-failure cancellation that verifies the Final checkpoint persists the cleared force flag and expected last_deferred watermark. - Medium Scope the summary-history assertion to the tested turn (
tests/integration/http_matcher.rs:137-143, confidence 98) — anchor: tests/integration/http_matcher.rs:143
This test submits three seed turns before calling the full-history assert_conversation_history_role_contains. The integration-test contract says full-history assertions are safe only for single-turn harnesses; multi-turn tests must use a baseline captured with history_len() and a *_since assertion. Earlier summaries can therefore contaminate this regression check.
Fix: Capture history_len() before the fetch turn and use a baseline-aware role-filtered history assertion, adding that helper if necessary.
Local Patterns
- Low Remove reference to unavailable design document (
crates/ironclaw_reborn_composition/src/projection/tests/failure_explanation.rs:351-353, confidence 100) — anchor: crates/ironclaw_reborn_composition/src/projection/tests/failure_explanation.rs:351
The explanatory comment links to .issue-work/5838-compaction-robustness-design.md, but that file is not tracked or present in the repository, leaving readers with a dead navigation path for the test's rationale.
Fix: Replace the path with a concise invariant-based explanation or link to a committed design/contract document.
Maintainability
- Low Keep milestone matching inside the assertion layer (
tests/integration/support/builder.rs:634-640, confidence 75) — anchor: tests/integration/support/builder.rs:634
The new public loop_milestones accessor exposes raw LoopHostMilestone records so the integration test can reimplement milestone matching directly. This leaks the sink representation into callers and separates this check from the existing assertion API.
Fix: Make the raw accessor private to the support module and add a focused assertion helper in tests/integration/support/assertions.rs, analogous to assert_turn_event_recorded, so tests depend on behavior rather than the sink's record shape.
…tion checkpoint Strengthen compaction_failure_cancellation_skips_explanation_and_returns_cancelled to decode the Final checkpoint payload and assert force_compact_on_next_iteration is cleared and the deferred watermark is persisted, not just the in-memory Cancelled exit reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…access Full-history conversation asserts are unsafe outside single-turn harnesses (CLAUDE.md); add role-scoped *_since baseline variants and use them after 3 seed turns. Restrict loop_milestones() to the support module and add a named assert_compaction_failed helper instead of pattern-matching raw LoopHostMilestoneKind at the test call site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hed inference workers await_compaction_with_cancellation raced an outer tokio::time::sleep against the compaction future using the same deadline_ms already enforced by ModelGatewayBackedSystemInferencePort's inner timeout. When the outer race won first, the future was dropped, detaching the GuardedSystemInferencePort worker it had spawned. The inner timeout already surfaces as InferenceFailed -> compaction_failed_continue with identical observable behavior, so the outer race added risk with no benefit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… note Review finding on #5895: the comment referenced .issue-work/, a local untracked file other readers cannot open. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Make non-cancellation compaction failures recoverable so post-tool model turns continue while preserving cancellation semantics.
Stats: 2 new findings (from 7 raw, 4 after overlap dedup, 2 after same-line and existing-thread suppression) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
The security lane reproduced an existing resolved current-head thread and was not reposted.
Performance
-
Medium Removing outer timeout leaves compaction DB work unbounded (
crates/ironclaw_agent_loop/src/executor/prompt.rs:494-498, confidence 92) — anchor:crates/ironclaw_agent_loop/src/executor/prompt.rs:568The inner timeout only wraps model inference, but
compact_loop_contextalso performs transcript reads, validation, scanning, and summary persistence. A stalled database or persistence call can now block the agent loop indefinitely; the removed outer timeout previously bounded the entire compaction operation.
Tests
-
Medium Scope compaction milestone assertion to the tested turn (
tests/integration/http_matcher.rs:133-135, confidence 95) — anchor:tests/integration/CLAUDE.md:202This multi-turn test asserts against
assert_compaction_failed, which scans milestones from harness construction and therefore includes the three seed turns. The assertion can pass because of a prior compaction failure rather than the forced compaction in the turn under test. Capture a milestone baseline immediately beforefetch items and ordersand assert only that slice.
assert_compaction_failed scanned all milestones since harness construction, so a multi-turn test (3 seed turns then the turn under test) could match a stale milestone from an earlier turn. Add milestone_len()/assert_compaction_failed_since mirroring the history_len()/*_since pattern; the old full-scan variant had one caller and is replaced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Amend two Design-lock rows: compaction errors are now deferred-continue rather than terminal CompactionUnavailable (#5838), and the wall-clock deadline is scoped to the inference call inside SystemInferencePort — store phases follow the loop's uniform store-I/O semantics, matching step 7's existing language. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reborn/2026-05-26-context-compaction.md`:
- Line 797: Update the failure state machine section around the descriptions of
InvalidCutPoint, InputTooLarge, InjectionDetected, LeakDetected,
InferenceFailed, and PersistenceFailed to remove the obsolete terminal
LoopFailureKind::CompactionUnavailable behavior. Document that these failures
emit CompactionFailed, record a deferred watermark, and continue the candidate
prompt in the same iteration; retain only Cancelled as terminal, consistent with
the executor path.
🪄 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: 5fe69532-ebef-45a0-acf7-e12556de7dbb
📒 Files selected for processing (4)
docs/reborn/2026-05-26-context-compaction.mdtests/integration/http_matcher.rstests/integration/support/assertions.rstests/integration/support/builder.rs
Non-terminal compaction failures were silent: no durable event and no log at emission. Adds a warn! with reason_kind + task_id (closed-vocab, safe) at the compaction_failed_continue funnel so operators can see the failure; durable RuntimeEvent surfacing is deferred to Slice C. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_agent_loop/src/executor/prompt.rs`:
- Around line 617-621: Change the recovered compaction failure log in the
executor path from tracing::warn! to tracing::debug!, preserving the task_id,
reason_kind, and existing message; the failure is already reported through
LoopProgressEvent::CompactionFailed.
🪄 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: 682f0b06-bd35-408d-9456-007873b6b137
📒 Files selected for processing (1)
crates/ironclaw_agent_loop/src/executor/prompt.rs
Review feedback on #5895: keep background loop diagnostics at debug per the repo logging convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on #5895: the failure state machine (§10) and three procedural walkthroughs still described the pre-#5838 terminal CompactionUnavailable behavior. All compaction-path references now match the deferred-continue contract; the Phase-2 GoalRefresh section is untouched (different stage, unchanged behavior). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reborn/2026-05-26-context-compaction.md`:
- Around line 985-997: The failed-compaction recovery path must check
cancellation before resuming inference. In the non-terminal error handling
branch for CompactionFailed, after clearing force_compact_on_next_iteration and
before continuing the candidate prompt, add an explicit cancellation checkpoint
that exits terminally if cancellation is requested.
🪄 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: 4a7ec146-d4e6-4467-a15e-9a1ab0b01e4d
📒 Files selected for processing (1)
docs/reborn/2026-05-26-context-compaction.md
Closes #5838.
Summary
CompactionUnavailablerun failures.CompactionFailed, clear the forced-compaction bit, remember the deferred watermark, and continue with the existing prompt path.Bug evidence
Before the production fix, the new executor regression failed after a successful large tool result because forced compaction returned
SecurityRejectedand the executor exited withCompactionUnavailable/compaction_security_rejectedinstead of letting the post-tool model turn finish.Tests
cargo fmtcargo test -p ironclaw_agent_loop compaction -- --nocapturecargo test --test reborn_integration_http_matcher multi_tool_turn_survives_failed_forced_compaction_after_results -- --nocapturecargo clippy -p ironclaw_agent_loop --all-targets -- -D warningscargo clippy --test reborn_integration_http_matcher -- -D warningsgit diff --checkNote: cargo prints the existing workspace warning
unused config key net.retries.