feat(agent-loop): output-aware no-progress detection (PR3) - #5022
Conversation
…mpletion When the runaway-loop safety guard fires (StopKind::NoProgressDetected), the executor finalized a canned "I stopped because I was repeating the same step" assistant reply and returned the run as Completed — a runtime control decision leaking into the conversation as a fake successful turn. This hid real blockers (auth, broken connector, empty search) and marked incomplete tasks as done. The ExitStage NoProgressDetected arm now: - keeps the #4837 final-answer-nudge path bit-for-bit: when the gate is enabled and the model synthesizes a real closing answer, complete with that answer (PinchBench path unchanged); - otherwise writes the Final checkpoint and returns a typed failed_exit(LoopFailureKind::NoProgressDetected) instead of the canned reply. The product layer already maps "no_progress_detected" to deterministic copy, so the user sees an honest failure on every channel. Deletes NO_PROGRESS_FALLBACK_REPLY and finalize_no_progress_fallback. PR1 of the output-aware no-progress redesign (exit honesty). PR2 (content-digest progress signal) and PR3 (failures off the no-progress axis) follow. Wire behavior: gate-off no-progress runs now arrive as Failed{no_progress_detected} instead of Completed with a canned reply. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…PR2) Inert plumbing for output-aware no-progress detection (PR3 will consume it). Adds a ContentDigest of each completed capability output so a later change can treat "the output moved" as real progress. This PR does NOT change any detection/stop behavior — it is observably inert. - ContentDigest newtype (keyed-blake3, mirrors ArgsHash) in ironclaw_turns; normalize_for_hash moved there and re-exported from agent_loop::strategies so CapabilityCallSignature hashing is unchanged (no checkpoint-signature break). - output_digest: Option<ContentDigest> on CapabilityResultMessage (serde default None = back-compat); computed host-side in the write_capability_result chokepoint alongside byte_len. Best-effort: a digest-compute failure degrades to None and never fails an otherwise-successful capability write. - seen_capability_output_digests: BoundedRing<_, 64> on LoopExecutionState, populated at append_completed_capability_result but NOT consumed. Additive #[serde(default)] checkpoint field (legacy checkpoints decode to an empty ring; round-trip + legacy-decode tests added). - record_result still receives the host's progress unchanged — detection is byte-identical (inertness test drives the executor and asserts no behavior change for repeated identical-output completed calls). Review fixes folded in (multi-agent review): from_output is fail-open; a caller-level test asserts the digest is recorded into the ring through a real run; boundary note documents why the digest impl lives in ironclaw_turns. PR2 of the no-progress redesign. PR1 (#4993) = honest typed failure; PR3 = consume the digest (NoChange when output repeats) + take failures off the no-progress axis. Benchmark-gated at PR3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Consumes the PR2 content-digest plumbing to make "no progress" mean the tool OUTPUT actually repeated, and takes failures off the no-progress axis. - append_completed_capability_result now DERIVES progress from the digest: if the same call (signature) produced an output already seen this run -> NoChange; a first-seen output -> MadeProgress; no digest -> host-reported progress. The membership check runs BEFORE recording the observation. This flips PR2 from inert population to active output-aware detection. - stop.rs counts ONLY NoChange toward the no-progress escape (Blocked dropped), and the repeated-call warning terminalizes only on a genuine NoChange repeat (new no_change_signatures track). Failing/blocked tools therefore route through recovery and the budget/iteration limit, never the no-progress give-up. Net (PR1+PR2+PR3): no-progress fires on genuine output-stagnation (same call, same output, repeated) as an honest typed failure; it no longer fires on a model progressing with changing output (polling/pagination) or on failing tools, and it never fakes a completion. Tests: flipped PR2's inertness test (identical output now trips); added the polling counterpart (changing output does not trip); blocked-failure tests now assert the run recovers instead of a no-progress escape; Unknown progress no longer terminalizes the repeated-call warning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 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
WalkthroughRefines no-progress detection so only capability results with identical output digests (mapped to ChangesNo-progress detection via output-digest deduplication
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
🚅 Deployed to the ironclaw-pr-5022 environment in ironclaw-ci-preview
|
There was a problem hiding this comment.
Code Review
This pull request refactors the agent loop's progress tracking to be output-aware, ensuring that only genuine output stagnation (repeated identical output digests) triggers the no-progress guard, while tool failures and advancing outputs are allowed to continue or recover. The feedback suggests utilizing exhaustive enum matching instead of conditional checks when handling the capability progress in stop.rs to leverage compiler-enforced safety and avoid redundant cloning.
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.
| if progress == CapabilityProgress::NoChange { | ||
| self.no_progress_count = self.no_progress_count.saturating_add(1); | ||
| push_unique_signature(&mut self.no_change_signatures, signature.clone()); | ||
| } | ||
| if progress == CapabilityProgress::MadeProgress { | ||
| push_unique_signature(&mut self.made_progress_signatures, signature); |
There was a problem hiding this comment.
Since CapabilityProgress is an enum, we should prefer exhaustive enum matching over runtime property-based checks (like if / else if chains) to leverage compiler-enforced safety. By using a match statement, we can safely move signature in both branches without needing to clone it, avoiding redundant allocations while keeping the code robust.
match progress {
CapabilityProgress::NoChange => {
self.no_progress_count = self.no_progress_count.saturating_add(1);
push_unique_signature(&mut self.no_change_signatures, signature);
}
CapabilityProgress::MadeProgress => {
push_unique_signature(&mut self.made_progress_signatures, signature);
}
}References
- Prefer exhaustive enum matching over runtime property-based checks for classifying variants when the set is small and known, as it leverages compiler-enforced safety and prevents loosening type constraints.
…nto firat/no-progress-pr2-content-digest # Conflicts: # crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs # crates/ironclaw_reborn/tests/loop_driver_host.rs # crates/ironclaw_reborn_composition/tests/product_live_adapters.rs # tests/support/reborn/harness.rs
…gress-pr3-output-aware
|
/benchmark pinchbench26 --framework ironclaw-reborn |
|
🧪 Started |
🧪 nearai-bench
|
|
/benchmark pinchbench --framework ironclaw-reborn |
|
🧪 Started |
🧪 nearai-bench
|
…i-fix # Conflicts: # crates/ironclaw_agent_loop/src/executor/capabilities.rs # crates/ironclaw_agent_loop/src/executor/loop_exit.rs # crates/ironclaw_agent_loop/src/executor/tests.rs # crates/ironclaw_agent_loop/tests/safety_nets.rs # crates/ironclaw_reborn_composition/src/product_live_adapters.rs # crates/ironclaw_reborn_composition/src/runtime/local_dev.rs
* fix(agent-loop): no-progress stop fails honestly instead of faking completion When the runaway-loop safety guard fires (StopKind::NoProgressDetected), the executor finalized a canned "I stopped because I was repeating the same step" assistant reply and returned the run as Completed — a runtime control decision leaking into the conversation as a fake successful turn. This hid real blockers (auth, broken connector, empty search) and marked incomplete tasks as done. The ExitStage NoProgressDetected arm now: - keeps the nearai#4837 final-answer-nudge path bit-for-bit: when the gate is enabled and the model synthesizes a real closing answer, complete with that answer (PinchBench path unchanged); - otherwise writes the Final checkpoint and returns a typed failed_exit(LoopFailureKind::NoProgressDetected) instead of the canned reply. The product layer already maps "no_progress_detected" to deterministic copy, so the user sees an honest failure on every channel. Deletes NO_PROGRESS_FALLBACK_REPLY and finalize_no_progress_fallback. PR1 of the output-aware no-progress redesign (exit honesty). PR2 (content-digest progress signal) and PR3 (failures off the no-progress axis) follow. Wire behavior: gate-off no-progress runs now arrive as Failed{no_progress_detected} instead of Completed with a canned reply. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(agent-loop): content-digest plumbing for output-aware progress (PR2) Inert plumbing for output-aware no-progress detection (PR3 will consume it). Adds a ContentDigest of each completed capability output so a later change can treat "the output moved" as real progress. This PR does NOT change any detection/stop behavior — it is observably inert. - ContentDigest newtype (keyed-blake3, mirrors ArgsHash) in ironclaw_turns; normalize_for_hash moved there and re-exported from agent_loop::strategies so CapabilityCallSignature hashing is unchanged (no checkpoint-signature break). - output_digest: Option<ContentDigest> on CapabilityResultMessage (serde default None = back-compat); computed host-side in the write_capability_result chokepoint alongside byte_len. Best-effort: a digest-compute failure degrades to None and never fails an otherwise-successful capability write. - seen_capability_output_digests: BoundedRing<_, 64> on LoopExecutionState, populated at append_completed_capability_result but NOT consumed. Additive #[serde(default)] checkpoint field (legacy checkpoints decode to an empty ring; round-trip + legacy-decode tests added). - record_result still receives the host's progress unchanged — detection is byte-identical (inertness test drives the executor and asserts no behavior change for repeated identical-output completed calls). Review fixes folded in (multi-agent review): from_output is fail-open; a caller-level test asserts the digest is recorded into the ring through a real run; boundary note documents why the digest impl lives in ironclaw_turns. PR2 of the no-progress redesign. PR1 (nearai#4993) = honest typed failure; PR3 = consume the digest (NoChange when output repeats) + take failures off the no-progress axis. Benchmark-gated at PR3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(agent-loop): output-aware no-progress detection (PR3) Consumes the PR2 content-digest plumbing to make "no progress" mean the tool OUTPUT actually repeated, and takes failures off the no-progress axis. - append_completed_capability_result now DERIVES progress from the digest: if the same call (signature) produced an output already seen this run -> NoChange; a first-seen output -> MadeProgress; no digest -> host-reported progress. The membership check runs BEFORE recording the observation. This flips PR2 from inert population to active output-aware detection. - stop.rs counts ONLY NoChange toward the no-progress escape (Blocked dropped), and the repeated-call warning terminalizes only on a genuine NoChange repeat (new no_change_signatures track). Failing/blocked tools therefore route through recovery and the budget/iteration limit, never the no-progress give-up. Net (PR1+PR2+PR3): no-progress fires on genuine output-stagnation (same call, same output, repeated) as an honest typed failure; it no longer fires on a model progressing with changing output (polling/pagination) or on failing tools, and it never fakes a completion. Tests: flipped PR2's inertness test (identical output now trips); added the polling counterpart (changing output does not trip); blocked-failure tests now assert the run recovers instead of a no-progress escape; Unknown progress no longer terminalizes the repeated-call warning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Stacked on #5000 (PR2), which is stacked on #4993 (PR1). Review against base
firat/no-progress-pr2-content-digestso the diff shows only this change; retarget down the stack as the lower PRs merge.What
The keystone of the no-progress redesign. PR2 added an inert
ContentDigestof each completed capability output; this PR consumes it so "no progress" finally means the output actually repeated, and it takes failures off the no-progress axis.append_completed_capability_result: if the same call (signature) produced an output we've already seen this run →NoChange; a first-seen output →MadeProgress; no digest (synthetic/older host) → the host-reported progress. The membership check runs before recording the observation (or a first occurrence would look "seen"). This flips PR2 from inert population to active output-aware detection.stop.rs: onlyNoChangecounts toward the no-progress escape (Blockeddropped), and the repeated-call warning terminalizes only on a genuineNoChangerepeat (newno_change_signaturestrack). Failing/blocked tools route through recovery and the budget/iteration limit — never the no-progress give-up.Why this is the keystone
Forensics on the PinchBench taxonomy run showed the no-progress give-up killed tasks that were making real progress (e.g.
task_selector_fix: repeatedapply_patchwith changing output, 10/19 rubric checks already green) and tasks where tools were failing (apply_patch/http errors mislabeled as "looping"). After PR3:Net effect (PR1 + PR2 + PR3)
No-progress now fires only on genuine output-stagnation, as an honest
Failed(NoProgressDetected)(or a real nudged answer where #4837 is enabled) — never a faked completion, never on a progressing model, never on failing tools.Tests
repeated_identical_output_digest_trips_no_progress(identical output now trips).changing_output_digests_do_not_trip_no_progress— the load-bearing polling counterpart (same call, advancing output, completes normally).trailing_no_progress_results == 0.Unknownprogress no longer terminalizes the repeated-call warning.Validation
fmt clean ·
cargo clippy -p ironclaw_agent_loop --all-targets --all-features0 warnings ·cargo test -p ironclaw_agent_loopall green (318 lib + integration binaries). Changes are confined toironclaw_agent_loopinternals (no public API/struct change), so the full-workspace gate + the PinchBench benchmark on the PR1+PR2+PR3 stack run in CI.🤖 Generated with Claude Code