Repository navigation
Detect prompt-cache breaks and stop doomed compaction loops - #5975
Merged
Merged
Conversation
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesCompaction now tracks trigger-specific effectiveness, opens a one-way circuit after repeated ineffective rebuilds, and preserves forced-compaction bypasses. Model gateways now record per-run prompt-cache usage and classify cache breaks using token and prompt-signature changes. Tests and path references were updated. Compaction effectiveness
Prompt-cache telemetry
Path contract updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)Compaction rebuild accountingsequenceDiagram
participant PromptPlanningPipeline
participant PromptCompactionStep
participant CompactionStrategyState
PromptPlanningPipeline->>PromptCompactionStep: run compaction decision
PromptCompactionStep->>CompactionStrategyState: store pending baseline
PromptPlanningPipeline->>PromptPlanningPipeline: rebuild prompt bundle
PromptPlanningPipeline->>CompactionStrategyState: observe rebuilt token estimate
CompactionStrategyState-->>PromptPlanningPipeline: update circuit state
Prompt-cache recordingsequenceDiagram
participant ModelGateway
participant CompleteModelRequest
participant PromptCacheCallScope
participant PromptCacheActivityLog
ModelGateway->>PromptCacheCallScope: create run scope
ModelGateway->>CompleteModelRequest: pass cache scope
CompleteModelRequest->>PromptCacheCallScope: record response usage
PromptCacheCallScope->>PromptCacheActivityLog: classify cache continuity or break
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
github-actions
Bot
added
size: XL
![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
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | deee8a2d5806 |
Head: deee8a2d58064ad6908d6dfeb67404936c4f380a
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found a blocking regression in the new compaction breaker: once it opens, explicit shrink/forced compactions are skipped too, which can make context-overflow recovery retry the same oversized prompt until abort.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Do not let the circuit breaker suppress forced shrink compactions
Location: crates/ironclaw_agent_loop/src/strategies/compaction.rs:52-54
The new early return runs before force_compact_on_next_iteration, so once compaction_circuit_open is true all explicit compaction requests become no-ops. That flag is how RetryAlteration::ShrinkContext handles provider ContextOverflow, and it is also used by byte-cap overflow handling. After the breaker trips, a context-overflow retry can enter the prompt stage, skip compaction here, rebuild the same oversized prompt, and burn through the model retry budget until the run aborts. Please limit the breaker to automatic compaction decisions, or make forced/recovery compaction fail fast through a distinct path instead of silently retrying unchanged.
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.
| pub const DEFAULT_DEADLINE_MS: u64 = 30_000; | ||
|
|
||
| pub(super) fn can_evaluate(&self, state: &LoopExecutionState) -> bool { | ||
| if state.compaction_state.compaction_circuit_open { |
Contributor
There was a problem hiding this comment.
This check also blocks force_compact_on_next_iteration. That flag is how RetryAlteration::ShrinkContext handles context-overflow recovery, so after the breaker opens a provider context overflow can retry with the same oversized prompt until the retry budget aborts. Please let explicit forced/recovery compactions bypass the auto-compaction breaker, or fail fast through a distinct recovery path.
serrrfirat
requested changes
Jul 11, 2026
serrrfirat
left a comment
serrrfirat
left a comment
Collaborator
There was a problem hiding this comment.
Multi-agent + thermo-nuclear review
Disposition: Request changes
Five specialist reviewers, an intent analyzer, and an independent thermo-nuclear structural pass reviewed stack PR #5975 against base #5959. After confidence filtering and overlap deduplication, 9 findings remain.
- Security: 0
- Bugs: 1
- Performance/concurrency: 1
- Tests: 4 raw, 3 standalone (one overlaps the core breaker bug)
- Conventions/contracts: 4
- Thermo-nuclear: 1 structural finding, deduplicated with conventions
- Reviewer failures: 0
The two blocking correctness findings show that the circuit breaker can miss both summary-driven and forced/byte-cap compact-recompact loops. The structural pass also rejects adding another cohesive subsystem directly to the 3.3K-line gateway.
| // Code measured ~250K wasted API calls/day from exactly this | ||
| // compact-recompact doom loop before adding a breaker. | ||
| let circuit_was_open = state.compaction_state.compaction_circuit_open; | ||
| state.compaction_state = state |
Collaborator
There was a problem hiding this comment.
[High · Performance/Correctness · 96%] Circuit breaker measures before the summary is added
The breaker classifies compaction immediately after retain_after_sequence, when the estimate contains only the retained tail and explicitly excludes the newly injected summary. A compactor can return a summary large enough that the rebuilt prompt stays above threshold, yet every attempt is recorded as effective because the tail alone is below threshold. The next iteration compacts again and resets the counter, so the doomed loop can continue through the 1,024-iteration backstop.
This was also flagged by the Tests lens: the caller test seeds the counter at limit - 1 and never proves three real prompt-stage compactions trip the breaker.
Fix: Measure after rebuilding the bundle including the summary, or carry a pending observation into the next iteration and compare the refreshed total before incrementing/resetting. Add a caller test driving three actual ineffective compactions and suppressing the fourth.
| drop_through_seq, | ||
| preserve_tail_tokens: self.preserve_tail_tokens, | ||
| deadline_ms: self.deadline_ms, | ||
| trigger_threshold_tokens: self.prompt_context_budget.visible_transcript_tokens(), |
Collaborator
There was a problem hiding this comment.
[High · Bug · 95%] Forced compactions use the wrong effectiveness threshold
Every trigger carries the normal visible-transcript threshold, including force_compact_on_next_iteration raised by ByteCapStrategy. A forced compaction can be completely ineffective while the retained prompt remains below that unrelated threshold; repeated ~32K capability-result overflows under the default ~108K transcript threshold will reset the counter rather than advance it. This contradicts the stated requirement that the breaker cover forced/byte-cap compactions.
Fix: Carry trigger-specific effectiveness data. For forced compactions compare against pre-compaction size or a byte-cap-specific target; keep the transcript threshold only for threshold-triggered compaction.
| return Err(map_provider_error(error)); | ||
| } | ||
| }; | ||
| if let Some(scope) = cache_scope.as_ref() { |
Collaborator
There was a problem hiding this comment.
[Medium · Tests · 90%] Tool-capable cache telemetry path is untested
The gateway-level cache-break test exercises only the text CompletionResponse branch. No integration test drives stream_model_with_capabilities through this distinct ToolCompletionResponse branch, so tool-definition hashing, recording, and attribution can regress unnoticed.
Fix: Add tests::llm_gateway::gateway_warns_and_attributes_prompt_cache_break_on_tool_capable_calls, using two same-run capability calls with a cache collapse and changed tool surface.
| return Err(map_provider_error(error)); | ||
| } | ||
| }; | ||
| if let Some(scope) = cache_scope.as_ref() { |
Collaborator
There was a problem hiding this comment.
[Medium · Tests · 85%] Repair retry cache recording has no test
A repairable tool-output failure performs a second provider call and records it as a separate cache observation, but existing repair tests do not assert this telemetry path.
Fix: Add tests::llm_gateway::gateway_records_prompt_cache_usage_for_tool_repair_retry, asserting both calls share the run scope and the retry emits a break on a large cache-read drop.
| usage.cache_read_input_tokens, | ||
| ) => | ||
| { | ||
| PromptCacheObservation::Break { |
Collaborator
There was a problem hiding this comment.
[Medium · Tests · 80%] System-prompt attribution branch is untested
Tests cover changed tools with an unchanged system prompt, but not the complementary branch. system_prompt_changed could remain permanently false without detection.
Fix: Add tests::model_gateway::prompt_cache_activity_log_attributes_system_prompt_only_break with unchanged tools and a changed system prompt.
| system_prompt_changed, | ||
| } = observation | ||
| { | ||
| tracing::warn!( |
Collaborator
There was a problem hiding this comment.
[Medium · Conventions · 100%] Internal cache diagnostics must not log at warn
This is internal prompt-cache diagnostic attribution, but tracing::warn! appears in and corrupts the REPL/TUI. The repository logging rule directs internal trace analysis to debug! (CLAUDE.md:84).
Fix: Emit at debug!, or use a diagnostics-only target excluded from interactive output.
| trigger_threshold_tokens, | ||
| ); | ||
| if !circuit_was_open && state.compaction_state.compaction_circuit_open { | ||
| tracing::warn!( |
Collaborator
There was a problem hiding this comment.
[Medium · Conventions · 100%] Compaction breaker diagnostics must not log at warn
Opening an internal circuit breaker emits tracing::warn!, violating the same repository rule that internal engine diagnostics belong at debug!; the structured progress path already carries lifecycle information.
Fix: Change this diagnostic to debug!, or route it through a diagnostics-only target.
| } | ||
| } | ||
|
|
||
| /// Relative drop factor for cache-break detection: the current call must read |
Collaborator
There was a problem hiding this comment.
[Medium · Thermo/Conventions · 90%] Extract cache telemetry from the 3.3K-line gateway
This adds roughly 350 lines of cache-break policy, state, hashing, eviction, logging, and tests directly to model_gateway.rs, growing it from 2,948 to 3,296 lines. Prompt-cache activity is a cohesive new runtime-adapter concern, and crates/ironclaw_runner/AGENTS.md explicitly says to add a new file for a new concern. This makes an existing god-file materially broader.
Fix: Move the tracker, observation types, thresholds, signatures, eviction, logging, and unit tests into a private prompt_cache_activity submodule; leave only scope construction and recording calls in the gateway.
| /// detects each break as it happens and attributes it to cheap request-shape | ||
| /// signals (tool surface or system prompt changed). | ||
| #[derive(Debug, Default)] | ||
| pub struct PromptCacheActivityLog { |
Collaborator
There was a problem hiding this comment.
[Medium · Conventions · 95%] Keep the cache activity log private
PromptCacheActivityLog is pub, but every use and all methods are internal to model_gateway.rs; related state and observation types are private. This expands the downstream API without providing a usable external contract, and the PR gives no rationale for exporting it.
Fix: Remove pub, or use pub(crate) only if the extracted internal module requires crate-level visibility.
… loop Two failure modes discovered via claw-swe-bench-lite run 9ca133e5 (30% vs hermes 65% on the same model) discarded hours of agent work: - A transient provider 5xx storm aborted the whole run after 2 quick retries (max_attempts_per_class=2, backoff capped at 5s). Availability- class model errors (transient/unavailable/internal) now retry on their own deeper budget: max_model_availability_attempts=12 with a 1s..60s exponential backoff, riding out ~7 minutes of sustained provider failure. MAX_MODEL_RETRIES raised 8 -> 16 to let the strategy govern. - DefaultBudgetStrategy's iteration_limit=32 failed closed mid-task with no synthesis (llm_calls in failed bench tasks clustered at exactly 63/64/127/128). The default is now DEFAULT_ITERATION_BACKSTOP=1024 (subagent 16 -> 256), documented as a runaway backstop: operational bounds are the resource budget system and stop-condition strategy. New seam mirroring IRONCLAW_REBORN_PLANNED_DEFAULT_ITERATION_LIMIT: IRONCLAW_REBORN_MODEL_AVAILABILITY_RETRY_ATTEMPTS -> DefaultPlannedRuntimeConfig.planned_model_availability_retry_attempts -> families::default_with_overrides. The integration group harness pins attempts=1 so scenarios that deliberately script provider failures (failure_category_demasked) reach Failed in seconds, not minutes; the availability-retry tests run under paused tokio time. Family fingerprint digests regenerated for the new strategy parameters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Benchmark traces showed the model retrying identical failing calls blind:
builtin.shell parameter errors and coding-tool path rejections reached it
as a bare category ("the tool input could not be encoded") because the
handlers built FirstPartyCapabilityError/CodingCapabilityError with no
safe_summary — the model-visible Diagnostic detail channel downstream was
already wired but starved (one agent burned 13 apply_patch calls against
an out-of-scope /testbed path with empty errors).
- shell.rs: shell_error/process_error now carry the concrete reason
("missing 'command' parameter", timeout duration, spawn failure),
bounded to 512 chars. The strict safe-summary validator still falls
back to the fixed category string; the reason always survives on the
secret-scrubbed diagnostic channel.
- coding/paths.rs: scoped-path rejections name the offending path and
the available scoped roots; permission rejections say the operation is
not permitted on that mount.
Covered at the dispatch tier (coding state dispatch, host-runtime
invoke_capability) per test-through-the-caller.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The deep availability-retry backoff added for provider-outage ride-out made outage-scripting scenario tests sleep for real: safety_nets alone took ~423s (the exact cumulative backoff schedule) because scripted or script-exhausted model errors now retry for minutes. Pause the clock on all executor scenario targets — they drive the in-process mock host exclusively, so timers auto-advance and the suites return to seconds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bility retries The placeholder unconfigured provider's RequestFailed was mapped through the catch-all Unavailable arm, so the new deep availability retry budget rode a permanent configuration fault through ~7 minutes of exponential backoff. Users with no LLM configured waited minutes for an error that retrying can never fix, and the Reborn CLI smoke tests that pin fast nonzero exits timed out (the 4 failures on CI run 29136954176). Map errors carrying the shared UNCONFIGURED_PROVIDER_ID to CredentialUnavailable, which is unclassified in loop recovery and therefore terminal on first sight; the Settings → Inference hint travels on the scrubbed detail channel. The provider id moves to a shared constant in ironclaw_llm so the composition placeholder and the runner mapping cannot drift. Regression tests: unconfigured_provider_error_maps_to_credential_unavailable_ not_availability and unconfigured_provider_detection_requires_the_placeholder_ provider_id in model_gateway.rs; the existing smoke tests (*_exits_nonzero_when_runtime_does_not_produce_reply) pin the fast-fail at the caller tier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nfig families::default_with_overrides swapped budget/recovery strategies but reused the planner's static version digest, so an overridden composition carried the pure-default replay identity — violating the component-identity contract (family.rs: the digest identifies replay-relevant configuration). - Turn the cfg(test) fingerprint const into a runtime default_family_fingerprint(iteration_limit, model_availability_attempts) builder; override-built families hash it with their resolved values at composition time (BLAKE3, same path as the pinned const). The pure-default composition keeps the static DEFAULT_FAMILY_DIGEST, and overrides spelling out the production defaults hash to that same digest. - Collapse the two Option args into a FamilyOverrides struct and drop the now-dead (None, None) branch in the runner's registry factory. - Tests: digest differs per override knob and is deterministic; explicit production defaults reproduce the static digest; an attempts=1 override reaches the composed recovery strategy (one retry then abort). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cancel Two model-stage fixes from the PR 5959 review: MAX_MODEL_RETRIES=16 silently capped any configured availability budget of 16+: the retry loop fell through to a generic ModelError exit with FailedExitDetails::default() — no failure category, no diagnostic ref — before the strategy could reach its own Abort. The executor now derives the loop bound from the composed strategy via RecoveryStrategy::max_total_model_attempts() (DefaultRecoveryStrategy computes it from its per-class + availability budgets with margin), so every accepted override reaches the strategy's abort boundary. The contract-bug fall-through now carries the last observed model error's category and diagnostic ref instead of empty details. The availability backoff sleep (up to 60s per attempt) was not cancellation-aware: a cancel request could wait out the full delay. The sleep now selects over the host's cancellation_requested() future (same pattern as the prompt-compaction and failure-explanation waits), and a boundary cancel check right after the alteration turns the wake into a checkpointed Cancelled exit without issuing another model call. Tests (paused tokio time): an availability budget of 20 — past the old executor cap — fails with the strategy's model_unavailable category and diagnostic ref after exactly 21 model calls; cancellation requested during the first 1s backoff exits Cancelled without riding out the sleep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…enforced The doc claimed operational bounds come from the resource budget system, but ResourceBudgetPolicy.max_model_calls and the wall-clock cap are defined and not applied; until they are, this backstop and the stop-condition strategy are the only live ceilings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ator
PR 5959's headline feature (model-visible tool-failure reasons) never
reached the model for path-bearing reasons: LoopSafeSummary rejects
path/payload delimiters and newlines, and dispatch_failure_message
silently degraded every such reason to the generic category sentence
before it could reach the diagnostic channel.
- production.rs (failure_from): a host-authored safe_summary that fails
LoopSafeSummary validation is preserved as the new
DispatchFailureDetail::Diagnostic instead of being dropped; the
message keeps the fixed category sentence (host-authored, Invariant 2).
- capability_port.rs: maps the Diagnostic detail into the model-visible
CapabilityFailureDetail::Diagnostic, scrubbing secret values and
normalizing control characters the observation validator rejects (so
one stray escape byte cannot drop the whole observation); newlines
are preserved. The RetrySameCall arm now forwards structured detail
too.
- coding/paths.rs: scoped-path rejection summaries render the path and
available roots delimiter-free ("path testbed replacer.go",
"available roots: workspace") so they pass the strict validator —
FilesystemDenied surfaces as a Denied loop outcome whose only
model-visible channel is the summary itself.
- shell.rs: bounded_failure_reason documents the (now real) diagnostic
flow; truncation remains char-based (no byte-boundary panics).
Regression tests:
- coding/mod.rs: out-of-scope rejection summary passes LoopSafeSummary
and names path + roots; new read-only-mount write denial test pins
FilesystemDenied with an actionable validated reason.
- host_runtime caller tier (first_party_coding_tools.rs): out-of-scope
read and read-only write driven through invoke_capability, asserting
the loop-boundary message survives validation and names path/root.
- host_runtime caller tier (first_party_builtin_tools.rs): shell
sensitive-path rejection reason rides the Diagnostic detail while the
strict summary channel stays path-free.
- production.rs: failure_from preserves rejected summaries (path +
newline) on the Diagnostic detail and leaves validator-safe summaries
on the message alone.
- capability_port.rs: Diagnostic detail reaches the model scrubbed
(path kept, newline kept, control char normalized, secret redacted);
empty-after-normalization diagnostics are dropped.
- shell.rs: bounded_failure_reason boundary tests (exactly 512 kept,
513 truncated with ellipsis, multibyte truncation on char
boundaries) and producer pin that paths/newlines are not
pre-sanitized away.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The reborn_parity_qa harness scripts deliberately-failing replay gateways (exhausted steps, mismatched requests) whose errors are availability-class; the production 12-attempt backoff budget rode them for minutes and reborn_trace_error_path_parity timed out waiting for Failed. Pin planned_model_availability_retry_attempts=1 in the harness runtime config, mirroring the integration group harness's env pin. The parity suite passes in ~1s again; the pinned test is the regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
force-pushed
the
reborn-loop-resilience
branch
from
July 12, 2026 21:29
c8a43b8 to
d1d249a
Compare
ilblackdragon
force-pushed
the
cache-first-loop
branch
from
July 12, 2026 21:29
deee8a2 to
fea8159
Compare
Member
Author
|
All nine findings addressed across three commits (branch restacked on the updated #5959): Breaker vs forced compactions (ironloopai blocking) — fixed ( Effectiveness measured before the summary lands (serrrfirat High·96%) — fixed ( Forced compactions judged against the wrong threshold (serrrfirat High·95%) — fixed ( Test gaps (N1–N3) — added ( Log levels (N4/N5) — downgraded to Structure (N6/N7) — extracted ( |
Three more real-time test paths rode the new 12-attempt availability backoff for minutes (composition-core / reborn-core CI buckets): - The substrate-only StubGateway returned Unavailable for a missing LLM gateway — a build-configuration fault retrying can never fix. It now returns CredentialUnavailable (unclassified in recovery, terminal), matching the placeholder-provider fail-fast. - send_user_message_preserves_model_unavailable_after_retry_budget scripts a deliberate outage; RebornRuntimeInput gains a test-support with_model_availability_retry_attempts override (wins over the env knob) and the test pins attempts=2, keeping its retry-then-fail pin. - turn_runner_worker_full_reborn_fails_cleanly_when_model_provider_is_offline now builds its registry via build_loop_family_registry_with_overrides with attempts=2 for the same reason. The three tests pass in ~3s each again; they are the regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
force-pushed
the
cache-first-loop
branch
from
July 12, 2026 22:25
fea8159 to
6bcc93f
Compare
The stub gateway now reports a configuration fault instead of an availability blip; the two runtime.rs pins (and one comment) tracking its failure category move from model_unavailable to model_credentials_unavailable accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Benchmark analysis showed the provider KV cache hit rate collapsing from ~82% to 29% as reborn agent runs grow past ~200 model calls, roughly 3.5x input cost. Two defenses: ironclaw_runner model gateway: track per-run cache_read continuity across completed provider calls. Every call emits a debug-level per-call cache series (cache_read / cache_creation / input tokens); a drop of BOTH >5% relative and >10K tokens absolute against the previous call in the same run warns "prompt cache break detected" with cheap attribution hints: tool-definition list hash changed, system prompt hash changed, seconds since last call. The pure threshold decision (is_prompt_cache_break) is unit-tested at its edges; per-run state lives in a bounded Mutex<HashMap> keyed by TurnRunId on each gateway. ironclaw_agent_loop compaction circuit breaker: when 3 consecutive compactions complete without the post-compaction prompt estimate dropping below the trigger threshold (compaction fired and didn't help), a one-way breaker in the CompactionStrategyState slot opens and compaction stays disabled for the remainder of the run, with a single tracing::warn!. Claude Code measured ~250K wasted API calls/day from exactly this compact-recompact doom loop before adding a breaker. State follows the typed-slot pattern (strategy reads, executor swaps); CompactionDecision::Trigger now carries the trigger threshold so the executor can score effectiveness. All compaction defaults (128000/20000/8000/...) are unchanged; the family fingerprints gain ineffective_trip_limit=3 and their BLAKE3 digests are regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review findings N6/N7 on #5975: the ~350 lines of prompt-cache continuity tracking (thresholds, break detection, observation types, per-run scope eviction, logging) lived inline in the 3,300-line model_gateway.rs and PromptCacheActivityLog was needlessly pub. Pure move into model_gateway/prompt_cache_activity.rs (unit tests included); the gateway now only constructs PromptCacheCallScope and records completed calls. All moved types are pub(super) at most, so PromptCacheActivityLog is no longer part of the crate's public API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rding Review findings N1-N4 on #5975: - N4: the cache-break record logged at warn!, but repo logging rules reserve info!/warn! for user-facing REPL output — internal diagnostics must use debug!. Downgraded, and renamed the gateway test to gateway_records_prompt_cache_break_within_a_run with a logs_assert that pins the record to a non-warn level. - N1: added a gateway integration test driving stream_model_with_capabilities with two same-run tool-capable calls, a 200K->50K cached-read collapse, and a changed advertised tool surface — pinning ModelCallCacheUsage::from_tool_response recording and tool-surface break attribution on the tool-capable path. - N2: extended gateway_repairs_oversized_provider_tool_arguments_ before_registration with scripted cache usage so the repair-retry recording seam is covered: both calls must record, and the collapse across the retry is a break with an unchanged request shape. - N3: extended the classification unit test with a changed-system- prompt break so system_prompt_changed=true attribution is pinned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eness, trigger-kind baselines Fixes the three blocking review findings on #5975 (B1/B2/B3): - B1: an open breaker suppressed forced/recovery compactions. force_compact_on_next_iteration is how ContextOverflow recovery (RetryAlteration::ShrinkContext) and byte-cap overflow request a shrink; with the breaker open they silently no-oped, the same oversized prompt was rebuilt, and the retry budget burned to abort. DefaultCompactionStrategy::can_evaluate now checks the force flag before the breaker — the breaker only gates automatic threshold-triggered compaction. - B2: effectiveness was measured right after retain_after_sequence, before the injected summary landed in the prompt, so a huge summary that kept the prompt oversized still counted every compaction as effective and the doom loop persisted to the iteration backstop. PromptCompactionStep now stashes the baseline on the new compaction_state.pending_effectiveness_baseline slot; the executor consumes it via observe_pending_compaction_effectiveness once the prompt bundle is next rebuilt (immediately after the in-pipeline rebuild, or on the next iteration's candidate build for compaction-only SkipModel turns), so the comparison sees the summary's tokens. - B3: forced compactions were judged against the unrelated visible-transcript threshold, letting a completely ineffective forced compaction reset the counter. CompactionDecision::Trigger now carries a trigger-kind-specific CompactionEffectivenessBaseline: TriggerThresholdTokens for automatic triggers, PreCompactionPromptTokens (did the prompt actually shrink?) for forced ones. Also downgrades the breaker-opened log from warn! to debug! (N5): internal loop diagnostics must not render into the REPL/TUI. Tests (red-first where the old behavior was pinned): - prompt_stage_circuit_breaker_disables_compaction_after_repeated_ ineffective_runs now drives three REAL ineffective compactions through the prompt stage (no seeded counter) with a retained tail below the threshold so only the summary-aware measurement trips the breaker, and asserts the fourth threshold overflow is suppressed. - prompt_stage_forced_compaction_bypasses_open_circuit_breaker pins B1+B3 at the executor tier: forced compaction runs with the breaker open and an effective shrink (260 -> 140) resets the counter even though 140 is still over the 90-token threshold. - Strategy tests updated: forced-bypass triggers for both DefaultCompactionStrategy and ActiveTaskPreservingCompactionStrategy, threshold-gated skip kept, slots-level forced-baseline coverage, and checkpoint back-compat for the new pending slot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
force-pushed
the
cache-first-loop
branch
from
July 12, 2026 23:00
6bcc93f to
2a388ef
Compare
Member
Author
|
@serrrfirat CI green across the stack; all 9 findings addressed (detailed response above) — please re-review. |
Resolves conflicts from the #5959 squash and later main changes: - executor/model.rs, production.rs, shell.rs, coding/paths.rs: take main's refined error mapping (failure summary with detail, PolicyDenied kind, debug-logged causes, bounded diagnostics + regression test) - families/mod.rs + subagent.rs: keep this branch's compaction ineffective_trip_limit=3 fingerprint and matching digests; adopt main's FamilyOverrides builder setters - composition runtime.rs/runtime_input.rs: take main's lazy env resolution and the ironclaw_loop_host rename (also fixed two stale ironclaw_loop_support mentions this branch carried); dedupe the with_model_availability_retry_attempts hook both sides added Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.55% — 302249 / 353301 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)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_runner/src/model_gateway.rs (1)
1108-1127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
cache_scoperequired here
All eight call sites incrates/ironclaw_runner/src/model_gateway.rspassSome(self.prompt_cache_scope(run_id)), socache_scopecan be a plainPromptCacheCallScopeand theif let Some(scope)branches can be removed. This matches the repo rule against optional runtime dependencies when wiring always supplies them.🤖 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_runner/src/model_gateway.rs` around lines 1108 - 1127, The complete_model_request function should require cache_scope as a plain PromptCacheCallScope instead of Option<PromptCacheCallScope>. Update all eight call sites to pass self.prompt_cache_scope(run_id) directly, then remove the if-let Some(scope) branching and use cache_scope directly wherever it is consumed.
🤖 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_runner/src/model_gateway.rs`:
- Around line 711-714: Add tests covering the RoutedLlmProviderModelGateway
request paths that pass prompt_cache_activity and prompt_cache_scope, including
the run_id forwarded to prompt_cache_scope. Mirror the existing
LlmProviderModelGateway cache assertions while exercising the routed gateway
methods around complete_model_request, and verify the expected cache activity
and scope are recorded.
In `@crates/ironclaw_runner/src/model_gateway/prompt_cache_activity.rs`:
- Around line 101-153: Add a regression test for
PromptCacheActivityLog::observe_model_call that records
PROMPT_CACHE_MAX_TRACKED_SCOPES + 1 distinct run IDs, then calls the oldest run
ID again and asserts it returns PromptCacheObservation::FirstCall. Ensure the
test distinguishes the eviction case from continuity for retained scopes.
---
Outside diff comments:
In `@crates/ironclaw_runner/src/model_gateway.rs`:
- Around line 1108-1127: The complete_model_request function should require
cache_scope as a plain PromptCacheCallScope instead of
Option<PromptCacheCallScope>. Update all eight call sites to pass
self.prompt_cache_scope(run_id) directly, then remove the if-let Some(scope)
branching and use cache_scope directly wherever it is consumed.
🪄 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: 96dc0bcb-1122-4b27-b133-580417a7ead0
📒 Files selected for processing (14)
crates/ironclaw_agent_loop/src/default_planner.rscrates/ironclaw_agent_loop/src/executor/prompt.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/families/mod.rscrates/ironclaw_agent_loop/src/families/subagent.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_agent_loop/src/state/slots.rscrates/ironclaw_agent_loop/src/strategies/active_task_compaction.rscrates/ironclaw_agent_loop/src/strategies/compaction.rscrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_runner/src/model_gateway.rscrates/ironclaw_runner/src/model_gateway/prompt_cache_activity.rscrates/ironclaw_runner/tests/llm_gateway.rscrates/ironclaw_threads/tests/filesystem_session_thread_contract.rs
Comment on lines
+711
to
714
| Some(self.prompt_cache_scope(run_id)), | ||
| ) | ||
| .await | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
RoutedLlmProviderModelGateway cache wiring is untested.
The routed gateway gets the identical prompt_cache_activity/prompt_cache_scope wiring as LlmProviderModelGateway, but llm_gateway.rs's new cache tests only exercise LlmProviderModelGateway. The underlying complete_model_request recording logic is covered through that sibling, but a wiring regression specific to the routed variant (e.g., wrong run_id threading) wouldn't be caught.
Also applies to: 750-753, 793-796, 837-840
🤖 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_runner/src/model_gateway.rs` around lines 711 - 714, Add
tests covering the RoutedLlmProviderModelGateway request paths that pass
prompt_cache_activity and prompt_cache_scope, including the run_id forwarded to
prompt_cache_scope. Mirror the existing LlmProviderModelGateway cache assertions
while exercising the routed gateway methods around complete_model_request, and
verify the expected cache activity and scope are recorded.
Comment on lines
+101
to
+153
| impl PromptCacheActivityLog { | ||
| /// Classifies the call against the previous call in the same run scope | ||
| /// and stores it as the new last-call state. | ||
| fn observe_model_call( | ||
| &self, | ||
| run_id: TurnRunId, | ||
| usage: ModelCallCacheUsage, | ||
| tool_definitions_hash: u64, | ||
| system_prompt_hash: u64, | ||
| ) -> PromptCacheObservation { | ||
| let mut scopes = self | ||
| .scopes | ||
| .lock() | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()); | ||
| let observation = match scopes.get(&run_id) { | ||
| None => PromptCacheObservation::FirstCall, | ||
| Some(previous) | ||
| if is_prompt_cache_break( | ||
| previous.cache_read_input_tokens, | ||
| usage.cache_read_input_tokens, | ||
| ) => | ||
| { | ||
| PromptCacheObservation::Break { | ||
| previous_cache_read_tokens: previous.cache_read_input_tokens, | ||
| tool_definitions_changed: previous.tool_definitions_hash | ||
| != tool_definitions_hash, | ||
| system_prompt_changed: previous.system_prompt_hash != system_prompt_hash, | ||
| } | ||
| } | ||
| Some(_) => PromptCacheObservation::Continuity, | ||
| }; | ||
| let seconds_since_last_call = scopes | ||
| .get(&run_id) | ||
| .map(|previous| previous.observed_at.elapsed().as_secs_f64()); | ||
| if scopes.len() >= PROMPT_CACHE_MAX_TRACKED_SCOPES && !scopes.contains_key(&run_id) { | ||
| let oldest = scopes | ||
| .iter() | ||
| .min_by_key(|(_, state)| state.observed_at) | ||
| .map(|(scope_run_id, _)| *scope_run_id); | ||
| if let Some(oldest) = oldest { | ||
| scopes.remove(&oldest); | ||
| } | ||
| } | ||
| scopes.insert( | ||
| run_id, | ||
| LastCallCacheState { | ||
| cache_read_input_tokens: usage.cache_read_input_tokens, | ||
| tool_definitions_hash, | ||
| system_prompt_hash, | ||
| observed_at: Instant::now(), | ||
| }, | ||
| ); | ||
| drop(scopes); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Eviction bound (PROMPT_CACHE_MAX_TRACKED_SCOPES) has no regression test.
observe_model_call correctly caps scopes by evicting the oldest entry before insert, but no test in this file (or the gateway tests) drives more than a couple of run ids through it. This is the one branch in the function not covered by the otherwise-thorough test suite below.
As per coding guidelines, "Every new feature and bug fix must begin with a test that demonstrates the expected behavior." Add a test that inserts PROMPT_CACHE_MAX_TRACKED_SCOPES + 1 distinct run ids and asserts the oldest scope was evicted (e.g., a subsequent call for the evicted run id classifies as FirstCall again).
🤖 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_runner/src/model_gateway/prompt_cache_activity.rs` around
lines 101 - 153, Add a regression test for
PromptCacheActivityLog::observe_model_call that records
PROMPT_CACHE_MAX_TRACKED_SCOPES + 1 distinct run IDs, then calls the oldest run
ID again and asserts it returns PromptCacheObservation::FirstCall. Ensure the
test distinguishes the eviction case from continuity for retained scopes.
Source: Coding guidelines
serrrfirat
previously approved these changes
Jul 14, 2026
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Jul 15, 2026
…ion (#5977) * Ride out provider outages and drop the 32-call turn cap in the reborn loop Two failure modes discovered via claw-swe-bench-lite run 9ca133e5 (30% vs hermes 65% on the same model) discarded hours of agent work: - A transient provider 5xx storm aborted the whole run after 2 quick retries (max_attempts_per_class=2, backoff capped at 5s). Availability- class model errors (transient/unavailable/internal) now retry on their own deeper budget: max_model_availability_attempts=12 with a 1s..60s exponential backoff, riding out ~7 minutes of sustained provider failure. MAX_MODEL_RETRIES raised 8 -> 16 to let the strategy govern. - DefaultBudgetStrategy's iteration_limit=32 failed closed mid-task with no synthesis (llm_calls in failed bench tasks clustered at exactly 63/64/127/128). The default is now DEFAULT_ITERATION_BACKSTOP=1024 (subagent 16 -> 256), documented as a runaway backstop: operational bounds are the resource budget system and stop-condition strategy. New seam mirroring IRONCLAW_REBORN_PLANNED_DEFAULT_ITERATION_LIMIT: IRONCLAW_REBORN_MODEL_AVAILABILITY_RETRY_ATTEMPTS -> DefaultPlannedRuntimeConfig.planned_model_availability_retry_attempts -> families::default_with_overrides. The integration group harness pins attempts=1 so scenarios that deliberately script provider failures (failure_category_demasked) reach Failed in seconds, not minutes; the availability-retry tests run under paused tokio time. Family fingerprint digests regenerated for the new strategy parameters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Surface tool-failure reasons to the model for shell and coding tools Benchmark traces showed the model retrying identical failing calls blind: builtin.shell parameter errors and coding-tool path rejections reached it as a bare category ("the tool input could not be encoded") because the handlers built FirstPartyCapabilityError/CodingCapabilityError with no safe_summary — the model-visible Diagnostic detail channel downstream was already wired but starved (one agent burned 13 apply_patch calls against an out-of-scope /testbed path with empty errors). - shell.rs: shell_error/process_error now carry the concrete reason ("missing 'command' parameter", timeout duration, spawn failure), bounded to 512 chars. The strict safe-summary validator still falls back to the fixed category string; the reason always survives on the secret-scrubbed diagnostic channel. - coding/paths.rs: scoped-path rejections name the offending path and the available scoped roots; permission rejections say the operation is not permitted on that mount. Covered at the dispatch tier (coding state dispatch, host-runtime invoke_capability) per test-through-the-caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Run agent_loop scenario test targets under paused tokio time The deep availability-retry backoff added for provider-outage ride-out made outage-scripting scenario tests sleep for real: safety_nets alone took ~423s (the exact cumulative backoff schedule) because scripted or script-exhausted model errors now retry for minutes. Pause the clock on all executor scenario targets — they drive the in-process mock host exclusively, so timers auto-advance and the suites return to seconds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fail fast when no LLM provider is configured instead of riding availability retries The placeholder unconfigured provider's RequestFailed was mapped through the catch-all Unavailable arm, so the new deep availability retry budget rode a permanent configuration fault through ~7 minutes of exponential backoff. Users with no LLM configured waited minutes for an error that retrying can never fix, and the Reborn CLI smoke tests that pin fast nonzero exits timed out (the 4 failures on CI run 29136954176). Map errors carrying the shared UNCONFIGURED_PROVIDER_ID to CredentialUnavailable, which is unclassified in loop recovery and therefore terminal on first sight; the Settings → Inference hint travels on the scrubbed detail channel. The provider id moves to a shared constant in ironclaw_llm so the composition placeholder and the runner mapping cannot drift. Regression tests: unconfigured_provider_error_maps_to_credential_unavailable_ not_availability and unconfigured_provider_detection_requires_the_placeholder_ provider_id in model_gateway.rs; the existing smoke tests (*_exits_nonzero_when_runtime_does_not_produce_reply) pin the fast-fail at the caller tier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Derive override-built default-family replay identity from resolved config families::default_with_overrides swapped budget/recovery strategies but reused the planner's static version digest, so an overridden composition carried the pure-default replay identity — violating the component-identity contract (family.rs: the digest identifies replay-relevant configuration). - Turn the cfg(test) fingerprint const into a runtime default_family_fingerprint(iteration_limit, model_availability_attempts) builder; override-built families hash it with their resolved values at composition time (BLAKE3, same path as the pinned const). The pure-default composition keeps the static DEFAULT_FAMILY_DIGEST, and overrides spelling out the production defaults hash to that same digest. - Collapse the two Option args into a FamilyOverrides struct and drop the now-dead (None, None) branch in the runner's registry factory. - Tests: digest differs per override knob and is deterministic; explicit production defaults reproduce the static digest; an attempts=1 override reaches the composed recovery strategy (one retry then abort). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Let the recovery strategy own the model retry guard; wake backoff on cancel Two model-stage fixes from the PR 5959 review: MAX_MODEL_RETRIES=16 silently capped any configured availability budget of 16+: the retry loop fell through to a generic ModelError exit with FailedExitDetails::default() — no failure category, no diagnostic ref — before the strategy could reach its own Abort. The executor now derives the loop bound from the composed strategy via RecoveryStrategy::max_total_model_attempts() (DefaultRecoveryStrategy computes it from its per-class + availability budgets with margin), so every accepted override reaches the strategy's abort boundary. The contract-bug fall-through now carries the last observed model error's category and diagnostic ref instead of empty details. The availability backoff sleep (up to 60s per attempt) was not cancellation-aware: a cancel request could wait out the full delay. The sleep now selects over the host's cancellation_requested() future (same pattern as the prompt-compaction and failure-explanation waits), and a boundary cancel check right after the alteration turns the wake into a checkpointed Cancelled exit without issuing another model call. Tests (paused tokio time): an availability budget of 20 — past the old executor cap — fails with the strategy's model_unavailable category and diagnostic ref after exactly 21 model calls; cancellation requested during the first 1s backoff exits Cancelled without riding out the sleep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Clarify DEFAULT_ITERATION_BACKSTOP doc: resource budgets are not yet enforced The doc claimed operational bounds come from the resource budget system, but ResourceBudgetPolicy.max_model_calls and the wall-clock cap are defined and not applied; until they are, this backstop and the stop-condition strategy are the only live ceilings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Carry tool-failure reasons to the model past the strict summary validator PR 5959's headline feature (model-visible tool-failure reasons) never reached the model for path-bearing reasons: LoopSafeSummary rejects path/payload delimiters and newlines, and dispatch_failure_message silently degraded every such reason to the generic category sentence before it could reach the diagnostic channel. - production.rs (failure_from): a host-authored safe_summary that fails LoopSafeSummary validation is preserved as the new DispatchFailureDetail::Diagnostic instead of being dropped; the message keeps the fixed category sentence (host-authored, Invariant 2). - capability_port.rs: maps the Diagnostic detail into the model-visible CapabilityFailureDetail::Diagnostic, scrubbing secret values and normalizing control characters the observation validator rejects (so one stray escape byte cannot drop the whole observation); newlines are preserved. The RetrySameCall arm now forwards structured detail too. - coding/paths.rs: scoped-path rejection summaries render the path and available roots delimiter-free ("path testbed replacer.go", "available roots: workspace") so they pass the strict validator — FilesystemDenied surfaces as a Denied loop outcome whose only model-visible channel is the summary itself. - shell.rs: bounded_failure_reason documents the (now real) diagnostic flow; truncation remains char-based (no byte-boundary panics). Regression tests: - coding/mod.rs: out-of-scope rejection summary passes LoopSafeSummary and names path + roots; new read-only-mount write denial test pins FilesystemDenied with an actionable validated reason. - host_runtime caller tier (first_party_coding_tools.rs): out-of-scope read and read-only write driven through invoke_capability, asserting the loop-boundary message survives validation and names path/root. - host_runtime caller tier (first_party_builtin_tools.rs): shell sensitive-path rejection reason rides the Diagnostic detail while the strict summary channel stays path-free. - production.rs: failure_from preserves rejected summaries (path + newline) on the Diagnostic detail and leaves validator-safe summaries on the message alone. - capability_port.rs: Diagnostic detail reaches the model scrubbed (path kept, newline kept, control char normalized, secret redacted); empty-after-normalization diagnostics are dropped. - shell.rs: bounded_failure_reason boundary tests (exactly 512 kept, 513 truncated with ellipsis, multibyte truncation on char boundaries) and producer pin that paths/newlines are not pre-sanitized away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Pin availability retries to 1 in the parity-QA binary E2E harness The reborn_parity_qa harness scripts deliberately-failing replay gateways (exhausted steps, mismatched requests) whose errors are availability-class; the production 12-attempt backoff budget rode them for minutes and reborn_trace_error_path_parity timed out waiting for Failed. Pin planned_model_availability_retry_attempts=1 in the harness runtime config, mirroring the integration group harness's env pin. The parity suite passes in ~1s again; the pinned test is the regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fail fast on the stub gateway and pin retries in scripted-outage tests Three more real-time test paths rode the new 12-attempt availability backoff for minutes (composition-core / reborn-core CI buckets): - The substrate-only StubGateway returned Unavailable for a missing LLM gateway — a build-configuration fault retrying can never fix. It now returns CredentialUnavailable (unclassified in recovery, terminal), matching the placeholder-provider fail-fast. - send_user_message_preserves_model_unavailable_after_retry_budget scripts a deliberate outage; RebornRuntimeInput gains a test-support with_model_availability_retry_attempts override (wins over the env knob) and the test pins attempts=2, keeping its retry-then-fail pin. - turn_runner_worker_full_reborn_fails_cleanly_when_model_provider_is_offline now builds its registry via build_loop_family_registry_with_overrides with attempts=2 for the same reason. The three tests pass in ~3s each again; they are the regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Update stub-gateway failure pins to model_credentials_unavailable The stub gateway now reports a configuration fault instead of an availability blip; the two runtime.rs pins (and one comment) tracking its failure category move from model_unavailable to model_credentials_unavailable accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Detect prompt-cache breaks and stop doomed compaction loops Benchmark analysis showed the provider KV cache hit rate collapsing from ~82% to 29% as reborn agent runs grow past ~200 model calls, roughly 3.5x input cost. Two defenses: ironclaw_runner model gateway: track per-run cache_read continuity across completed provider calls. Every call emits a debug-level per-call cache series (cache_read / cache_creation / input tokens); a drop of BOTH >5% relative and >10K tokens absolute against the previous call in the same run warns "prompt cache break detected" with cheap attribution hints: tool-definition list hash changed, system prompt hash changed, seconds since last call. The pure threshold decision (is_prompt_cache_break) is unit-tested at its edges; per-run state lives in a bounded Mutex<HashMap> keyed by TurnRunId on each gateway. ironclaw_agent_loop compaction circuit breaker: when 3 consecutive compactions complete without the post-compaction prompt estimate dropping below the trigger threshold (compaction fired and didn't help), a one-way breaker in the CompactionStrategyState slot opens and compaction stays disabled for the remainder of the run, with a single tracing::warn!. Claude Code measured ~250K wasted API calls/day from exactly this compact-recompact doom loop before adding a breaker. State follows the typed-slot pattern (strategy reads, executor swaps); CompactionDecision::Trigger now carries the trigger threshold so the executor can score effectiveness. All compaction defaults (128000/20000/8000/...) are unchanged; the family fingerprints gain ineffective_trip_limit=3 and their BLAKE3 digests are regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Extract prompt-cache telemetry into a private model_gateway submodule Review findings N6/N7 on #5975: the ~350 lines of prompt-cache continuity tracking (thresholds, break detection, observation types, per-run scope eviction, logging) lived inline in the 3,300-line model_gateway.rs and PromptCacheActivityLog was needlessly pub. Pure move into model_gateway/prompt_cache_activity.rs (unit tests included); the gateway now only constructs PromptCacheCallScope and records completed calls. All moved types are pub(super) at most, so PromptCacheActivityLog is no longer part of the crate's public API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Record prompt-cache breaks at debug level and cover tool-capable recording Review findings N1-N4 on #5975: - N4: the cache-break record logged at warn!, but repo logging rules reserve info!/warn! for user-facing REPL output — internal diagnostics must use debug!. Downgraded, and renamed the gateway test to gateway_records_prompt_cache_break_within_a_run with a logs_assert that pins the record to a non-warn level. - N1: added a gateway integration test driving stream_model_with_capabilities with two same-run tool-capable calls, a 200K->50K cached-read collapse, and a changed advertised tool surface — pinning ModelCallCacheUsage::from_tool_response recording and tool-surface break attribution on the tool-capable path. - N2: extended gateway_repairs_oversized_provider_tool_arguments_ before_registration with scripted cache usage so the repair-retry recording seam is covered: both calls must record, and the collapse across the retry is a break with an unchanged request shape. - N3: extended the classification unit test with a changed-system- prompt break so system_prompt_changed=true attribution is pinned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix compaction circuit breaker: forced bypass, summary-aware effectiveness, trigger-kind baselines Fixes the three blocking review findings on #5975 (B1/B2/B3): - B1: an open breaker suppressed forced/recovery compactions. force_compact_on_next_iteration is how ContextOverflow recovery (RetryAlteration::ShrinkContext) and byte-cap overflow request a shrink; with the breaker open they silently no-oped, the same oversized prompt was rebuilt, and the retry budget burned to abort. DefaultCompactionStrategy::can_evaluate now checks the force flag before the breaker — the breaker only gates automatic threshold-triggered compaction. - B2: effectiveness was measured right after retain_after_sequence, before the injected summary landed in the prompt, so a huge summary that kept the prompt oversized still counted every compaction as effective and the doom loop persisted to the iteration backstop. PromptCompactionStep now stashes the baseline on the new compaction_state.pending_effectiveness_baseline slot; the executor consumes it via observe_pending_compaction_effectiveness once the prompt bundle is next rebuilt (immediately after the in-pipeline rebuild, or on the next iteration's candidate build for compaction-only SkipModel turns), so the comparison sees the summary's tokens. - B3: forced compactions were judged against the unrelated visible-transcript threshold, letting a completely ineffective forced compaction reset the counter. CompactionDecision::Trigger now carries a trigger-kind-specific CompactionEffectivenessBaseline: TriggerThresholdTokens for automatic triggers, PreCompactionPromptTokens (did the prompt actually shrink?) for forced ones. Also downgrades the breaker-opened log from warn! to debug! (N5): internal loop diagnostics must not render into the REPL/TUI. Tests (red-first where the old behavior was pinned): - prompt_stage_circuit_breaker_disables_compaction_after_repeated_ ineffective_runs now drives three REAL ineffective compactions through the prompt stage (no seeded counter) with a retained tail below the threshold so only the summary-aware measurement trips the breaker, and asserts the fourth threshold overflow is suppressed. - prompt_stage_forced_compaction_bypasses_open_circuit_breaker pins B1+B3 at the executor tier: forced compaction runs with the breaker open and an effective shrink (260 -> 140) resets the counter even though 140 is still over the 90-token threshold. - Strategy tests updated: forced-bypass triggers for both DefaultCompactionStrategy and ActiveTaskPreservingCompactionStrategy, threshold-gated skip kept, slots-level forced-baseline coverage, and checkpoint back-compat for the new pending slot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Advertise Reborn skills as a one-line listing; load bodies on activation Reborn injected the FULL SKILL.md body of every keyword-scored skill into the prompt on every run — ~7K tokens of mostly irrelevant instructions per turn (measured in claw-swe-bench traces) — and the skill section churned whenever scores shifted, defeating prompt-cache stability. Flip to the Claude Code model: advertise skills as one `- name: description` line each and load a body only when the model invokes it via builtin.skill_activate. - `SelectableSkillContextSource` gains `SkillInjectionMode`: * `Listing` (Reborn default, `IRONCLAW_REBORN_SKILL_INJECTION` overridable to `full`): non-activated skills collapse into ONE discoverable "available-skills" candidate — a fixed header pointing at builtin.skill_activate plus one line per skill (250-char description cap, 100-entry cap). Full bodies inject only for explicit $name mentions and model-selected activations. Keyword scoring still runs and RANKS the listing; criteria selections no longer consume the model's activation budget. * `Full` stays the library default so non-Reborn consumers and the skill-execution capture plan keep their existing semantics. - Prompt build and by-ref model-message resolution rebuild the snippet set independently, so both now derive body-eligibility and listing ranking from the merged active plan — identical sets, stable msg:snippet refs. - `SkillContextService` safe summaries are now the bounded first line of the description instead of the full text: the multi-skill listing (5.6KB) blew past MODEL_SAFE_SUMMARY_MAX_BYTES and killed runs at the prompt stage — a latent bug for any long skill description. v1 (`src/`) skill behavior is untouched. Tests: reborn_integration_skill_activate pins the one-liner listing (body absent pre-activation, present post-activation) at the captured model-request seam; new selector listing-mode tests in ironclaw_first_party_extension_ports; safe-summary regression in skill_context_service_contract; env parse + config propagation tests in ironclaw_reborn_composition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Upgrade criteria-listed skills to ModelSelected on later skill_activate merge_active_plan deduped merged activations by bundle id alone, so a skill already in the run plan as a listing-only ActivationCriteria entry silently dropped a later ModelSelected activation for the same bundle: the mode never upgraded, body_eligible_bundle_ids never included it, and the SKILL.md body never injected — while the skill_activate tool still reported "activated 1 skill(s)" because the merged plan matched by name. Mode-priority merge: when the incoming activation is body-eligible (ModelSelected/ExplicitMention) and the existing entry for the same bundle is ActivationCriteria, upgrade the existing entry's mode in place; never downgrade. Regression test drives record_user_message (criteria match) then activate_skills_for_run and asserts the body injects on the next prompt build and leaves the listing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Hash full model content into skill snippet model-message refs skill_snippet_model_message_ref hashed only [snippet_ref, safe_summary, ordinal], and the safe summary is first-line-only (capped 256 chars). The available-skills listing's first line is a constant header, so adding, removing, or reordering listed skills changed the listing body without changing the ref — a stale ref resolved against the new content instead of failing closed. Include the full model_content in the hashed fields. Refs are per-run ephemeral (built at prompt time, resolved live by instruction_snippet_messages_by_ref in the same run; never persisted — the executor's message index stores only sequence/kind/token counts), so the deliberate one-time hash rotation is safe; pinned ref literals in the turns/loop-support contract tests are updated to the new layout. Regression tests pin that listings differing only after the first line produce different refs and identical content produces a stable ref. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Jul 17, 2026
#5978) * Ride out provider outages and drop the 32-call turn cap in the reborn loop Two failure modes discovered via claw-swe-bench-lite run 9ca133e5 (30% vs hermes 65% on the same model) discarded hours of agent work: - A transient provider 5xx storm aborted the whole run after 2 quick retries (max_attempts_per_class=2, backoff capped at 5s). Availability- class model errors (transient/unavailable/internal) now retry on their own deeper budget: max_model_availability_attempts=12 with a 1s..60s exponential backoff, riding out ~7 minutes of sustained provider failure. MAX_MODEL_RETRIES raised 8 -> 16 to let the strategy govern. - DefaultBudgetStrategy's iteration_limit=32 failed closed mid-task with no synthesis (llm_calls in failed bench tasks clustered at exactly 63/64/127/128). The default is now DEFAULT_ITERATION_BACKSTOP=1024 (subagent 16 -> 256), documented as a runaway backstop: operational bounds are the resource budget system and stop-condition strategy. New seam mirroring IRONCLAW_REBORN_PLANNED_DEFAULT_ITERATION_LIMIT: IRONCLAW_REBORN_MODEL_AVAILABILITY_RETRY_ATTEMPTS -> DefaultPlannedRuntimeConfig.planned_model_availability_retry_attempts -> families::default_with_overrides. The integration group harness pins attempts=1 so scenarios that deliberately script provider failures (failure_category_demasked) reach Failed in seconds, not minutes; the availability-retry tests run under paused tokio time. Family fingerprint digests regenerated for the new strategy parameters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Surface tool-failure reasons to the model for shell and coding tools Benchmark traces showed the model retrying identical failing calls blind: builtin.shell parameter errors and coding-tool path rejections reached it as a bare category ("the tool input could not be encoded") because the handlers built FirstPartyCapabilityError/CodingCapabilityError with no safe_summary — the model-visible Diagnostic detail channel downstream was already wired but starved (one agent burned 13 apply_patch calls against an out-of-scope /testbed path with empty errors). - shell.rs: shell_error/process_error now carry the concrete reason ("missing 'command' parameter", timeout duration, spawn failure), bounded to 512 chars. The strict safe-summary validator still falls back to the fixed category string; the reason always survives on the secret-scrubbed diagnostic channel. - coding/paths.rs: scoped-path rejections name the offending path and the available scoped roots; permission rejections say the operation is not permitted on that mount. Covered at the dispatch tier (coding state dispatch, host-runtime invoke_capability) per test-through-the-caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Run agent_loop scenario test targets under paused tokio time The deep availability-retry backoff added for provider-outage ride-out made outage-scripting scenario tests sleep for real: safety_nets alone took ~423s (the exact cumulative backoff schedule) because scripted or script-exhausted model errors now retry for minutes. Pause the clock on all executor scenario targets — they drive the in-process mock host exclusively, so timers auto-advance and the suites return to seconds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fail fast when no LLM provider is configured instead of riding availability retries The placeholder unconfigured provider's RequestFailed was mapped through the catch-all Unavailable arm, so the new deep availability retry budget rode a permanent configuration fault through ~7 minutes of exponential backoff. Users with no LLM configured waited minutes for an error that retrying can never fix, and the Reborn CLI smoke tests that pin fast nonzero exits timed out (the 4 failures on CI run 29136954176). Map errors carrying the shared UNCONFIGURED_PROVIDER_ID to CredentialUnavailable, which is unclassified in loop recovery and therefore terminal on first sight; the Settings → Inference hint travels on the scrubbed detail channel. The provider id moves to a shared constant in ironclaw_llm so the composition placeholder and the runner mapping cannot drift. Regression tests: unconfigured_provider_error_maps_to_credential_unavailable_ not_availability and unconfigured_provider_detection_requires_the_placeholder_ provider_id in model_gateway.rs; the existing smoke tests (*_exits_nonzero_when_runtime_does_not_produce_reply) pin the fast-fail at the caller tier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Derive override-built default-family replay identity from resolved config families::default_with_overrides swapped budget/recovery strategies but reused the planner's static version digest, so an overridden composition carried the pure-default replay identity — violating the component-identity contract (family.rs: the digest identifies replay-relevant configuration). - Turn the cfg(test) fingerprint const into a runtime default_family_fingerprint(iteration_limit, model_availability_attempts) builder; override-built families hash it with their resolved values at composition time (BLAKE3, same path as the pinned const). The pure-default composition keeps the static DEFAULT_FAMILY_DIGEST, and overrides spelling out the production defaults hash to that same digest. - Collapse the two Option args into a FamilyOverrides struct and drop the now-dead (None, None) branch in the runner's registry factory. - Tests: digest differs per override knob and is deterministic; explicit production defaults reproduce the static digest; an attempts=1 override reaches the composed recovery strategy (one retry then abort). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Let the recovery strategy own the model retry guard; wake backoff on cancel Two model-stage fixes from the PR 5959 review: MAX_MODEL_RETRIES=16 silently capped any configured availability budget of 16+: the retry loop fell through to a generic ModelError exit with FailedExitDetails::default() — no failure category, no diagnostic ref — before the strategy could reach its own Abort. The executor now derives the loop bound from the composed strategy via RecoveryStrategy::max_total_model_attempts() (DefaultRecoveryStrategy computes it from its per-class + availability budgets with margin), so every accepted override reaches the strategy's abort boundary. The contract-bug fall-through now carries the last observed model error's category and diagnostic ref instead of empty details. The availability backoff sleep (up to 60s per attempt) was not cancellation-aware: a cancel request could wait out the full delay. The sleep now selects over the host's cancellation_requested() future (same pattern as the prompt-compaction and failure-explanation waits), and a boundary cancel check right after the alteration turns the wake into a checkpointed Cancelled exit without issuing another model call. Tests (paused tokio time): an availability budget of 20 — past the old executor cap — fails with the strategy's model_unavailable category and diagnostic ref after exactly 21 model calls; cancellation requested during the first 1s backoff exits Cancelled without riding out the sleep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Clarify DEFAULT_ITERATION_BACKSTOP doc: resource budgets are not yet enforced The doc claimed operational bounds come from the resource budget system, but ResourceBudgetPolicy.max_model_calls and the wall-clock cap are defined and not applied; until they are, this backstop and the stop-condition strategy are the only live ceilings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Carry tool-failure reasons to the model past the strict summary validator PR 5959's headline feature (model-visible tool-failure reasons) never reached the model for path-bearing reasons: LoopSafeSummary rejects path/payload delimiters and newlines, and dispatch_failure_message silently degraded every such reason to the generic category sentence before it could reach the diagnostic channel. - production.rs (failure_from): a host-authored safe_summary that fails LoopSafeSummary validation is preserved as the new DispatchFailureDetail::Diagnostic instead of being dropped; the message keeps the fixed category sentence (host-authored, Invariant 2). - capability_port.rs: maps the Diagnostic detail into the model-visible CapabilityFailureDetail::Diagnostic, scrubbing secret values and normalizing control characters the observation validator rejects (so one stray escape byte cannot drop the whole observation); newlines are preserved. The RetrySameCall arm now forwards structured detail too. - coding/paths.rs: scoped-path rejection summaries render the path and available roots delimiter-free ("path testbed replacer.go", "available roots: workspace") so they pass the strict validator — FilesystemDenied surfaces as a Denied loop outcome whose only model-visible channel is the summary itself. - shell.rs: bounded_failure_reason documents the (now real) diagnostic flow; truncation remains char-based (no byte-boundary panics). Regression tests: - coding/mod.rs: out-of-scope rejection summary passes LoopSafeSummary and names path + roots; new read-only-mount write denial test pins FilesystemDenied with an actionable validated reason. - host_runtime caller tier (first_party_coding_tools.rs): out-of-scope read and read-only write driven through invoke_capability, asserting the loop-boundary message survives validation and names path/root. - host_runtime caller tier (first_party_builtin_tools.rs): shell sensitive-path rejection reason rides the Diagnostic detail while the strict summary channel stays path-free. - production.rs: failure_from preserves rejected summaries (path + newline) on the Diagnostic detail and leaves validator-safe summaries on the message alone. - capability_port.rs: Diagnostic detail reaches the model scrubbed (path kept, newline kept, control char normalized, secret redacted); empty-after-normalization diagnostics are dropped. - shell.rs: bounded_failure_reason boundary tests (exactly 512 kept, 513 truncated with ellipsis, multibyte truncation on char boundaries) and producer pin that paths/newlines are not pre-sanitized away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Pin availability retries to 1 in the parity-QA binary E2E harness The reborn_parity_qa harness scripts deliberately-failing replay gateways (exhausted steps, mismatched requests) whose errors are availability-class; the production 12-attempt backoff budget rode them for minutes and reborn_trace_error_path_parity timed out waiting for Failed. Pin planned_model_availability_retry_attempts=1 in the harness runtime config, mirroring the integration group harness's env pin. The parity suite passes in ~1s again; the pinned test is the regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fail fast on the stub gateway and pin retries in scripted-outage tests Three more real-time test paths rode the new 12-attempt availability backoff for minutes (composition-core / reborn-core CI buckets): - The substrate-only StubGateway returned Unavailable for a missing LLM gateway — a build-configuration fault retrying can never fix. It now returns CredentialUnavailable (unclassified in recovery, terminal), matching the placeholder-provider fail-fast. - send_user_message_preserves_model_unavailable_after_retry_budget scripts a deliberate outage; RebornRuntimeInput gains a test-support with_model_availability_retry_attempts override (wins over the env knob) and the test pins attempts=2, keeping its retry-then-fail pin. - turn_runner_worker_full_reborn_fails_cleanly_when_model_provider_is_offline now builds its registry via build_loop_family_registry_with_overrides with attempts=2 for the same reason. The three tests pass in ~3s each again; they are the regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Update stub-gateway failure pins to model_credentials_unavailable The stub gateway now reports a configuration fault instead of an availability blip; the two runtime.rs pins (and one comment) tracking its failure category move from model_unavailable to model_credentials_unavailable accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Detect prompt-cache breaks and stop doomed compaction loops Benchmark analysis showed the provider KV cache hit rate collapsing from ~82% to 29% as reborn agent runs grow past ~200 model calls, roughly 3.5x input cost. Two defenses: ironclaw_runner model gateway: track per-run cache_read continuity across completed provider calls. Every call emits a debug-level per-call cache series (cache_read / cache_creation / input tokens); a drop of BOTH >5% relative and >10K tokens absolute against the previous call in the same run warns "prompt cache break detected" with cheap attribution hints: tool-definition list hash changed, system prompt hash changed, seconds since last call. The pure threshold decision (is_prompt_cache_break) is unit-tested at its edges; per-run state lives in a bounded Mutex<HashMap> keyed by TurnRunId on each gateway. ironclaw_agent_loop compaction circuit breaker: when 3 consecutive compactions complete without the post-compaction prompt estimate dropping below the trigger threshold (compaction fired and didn't help), a one-way breaker in the CompactionStrategyState slot opens and compaction stays disabled for the remainder of the run, with a single tracing::warn!. Claude Code measured ~250K wasted API calls/day from exactly this compact-recompact doom loop before adding a breaker. State follows the typed-slot pattern (strategy reads, executor swaps); CompactionDecision::Trigger now carries the trigger threshold so the executor can score effectiveness. All compaction defaults (128000/20000/8000/...) are unchanged; the family fingerprints gain ineffective_trip_limit=3 and their BLAKE3 digests are regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Extract prompt-cache telemetry into a private model_gateway submodule Review findings N6/N7 on #5975: the ~350 lines of prompt-cache continuity tracking (thresholds, break detection, observation types, per-run scope eviction, logging) lived inline in the 3,300-line model_gateway.rs and PromptCacheActivityLog was needlessly pub. Pure move into model_gateway/prompt_cache_activity.rs (unit tests included); the gateway now only constructs PromptCacheCallScope and records completed calls. All moved types are pub(super) at most, so PromptCacheActivityLog is no longer part of the crate's public API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Record prompt-cache breaks at debug level and cover tool-capable recording Review findings N1-N4 on #5975: - N4: the cache-break record logged at warn!, but repo logging rules reserve info!/warn! for user-facing REPL output — internal diagnostics must use debug!. Downgraded, and renamed the gateway test to gateway_records_prompt_cache_break_within_a_run with a logs_assert that pins the record to a non-warn level. - N1: added a gateway integration test driving stream_model_with_capabilities with two same-run tool-capable calls, a 200K->50K cached-read collapse, and a changed advertised tool surface — pinning ModelCallCacheUsage::from_tool_response recording and tool-surface break attribution on the tool-capable path. - N2: extended gateway_repairs_oversized_provider_tool_arguments_ before_registration with scripted cache usage so the repair-retry recording seam is covered: both calls must record, and the collapse across the retry is a break with an unchanged request shape. - N3: extended the classification unit test with a changed-system- prompt break so system_prompt_changed=true attribution is pinned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix compaction circuit breaker: forced bypass, summary-aware effectiveness, trigger-kind baselines Fixes the three blocking review findings on #5975 (B1/B2/B3): - B1: an open breaker suppressed forced/recovery compactions. force_compact_on_next_iteration is how ContextOverflow recovery (RetryAlteration::ShrinkContext) and byte-cap overflow request a shrink; with the breaker open they silently no-oped, the same oversized prompt was rebuilt, and the retry budget burned to abort. DefaultCompactionStrategy::can_evaluate now checks the force flag before the breaker — the breaker only gates automatic threshold-triggered compaction. - B2: effectiveness was measured right after retain_after_sequence, before the injected summary landed in the prompt, so a huge summary that kept the prompt oversized still counted every compaction as effective and the doom loop persisted to the iteration backstop. PromptCompactionStep now stashes the baseline on the new compaction_state.pending_effectiveness_baseline slot; the executor consumes it via observe_pending_compaction_effectiveness once the prompt bundle is next rebuilt (immediately after the in-pipeline rebuild, or on the next iteration's candidate build for compaction-only SkipModel turns), so the comparison sees the summary's tokens. - B3: forced compactions were judged against the unrelated visible-transcript threshold, letting a completely ineffective forced compaction reset the counter. CompactionDecision::Trigger now carries a trigger-kind-specific CompactionEffectivenessBaseline: TriggerThresholdTokens for automatic triggers, PreCompactionPromptTokens (did the prompt actually shrink?) for forced ones. Also downgrades the breaker-opened log from warn! to debug! (N5): internal loop diagnostics must not render into the REPL/TUI. Tests (red-first where the old behavior was pinned): - prompt_stage_circuit_breaker_disables_compaction_after_repeated_ ineffective_runs now drives three REAL ineffective compactions through the prompt stage (no seeded counter) with a retained tail below the threshold so only the summary-aware measurement trips the breaker, and asserts the fourth threshold overflow is suppressed. - prompt_stage_forced_compaction_bypasses_open_circuit_breaker pins B1+B3 at the executor tier: forced compaction runs with the breaker open and an effective shrink (260 -> 140) resets the counter even though 140 is still over the 90-token threshold. - Strategy tests updated: forced-bypass triggers for both DefaultCompactionStrategy and ActiveTaskPreservingCompactionStrategy, threshold-gated skip kept, slots-level forced-baseline coverage, and checkpoint back-compat for the new pending slot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Advertise Reborn skills as a one-line listing; load bodies on activation Reborn injected the FULL SKILL.md body of every keyword-scored skill into the prompt on every run — ~7K tokens of mostly irrelevant instructions per turn (measured in claw-swe-bench traces) — and the skill section churned whenever scores shifted, defeating prompt-cache stability. Flip to the Claude Code model: advertise skills as one `- name: description` line each and load a body only when the model invokes it via builtin.skill_activate. - `SelectableSkillContextSource` gains `SkillInjectionMode`: * `Listing` (Reborn default, `IRONCLAW_REBORN_SKILL_INJECTION` overridable to `full`): non-activated skills collapse into ONE discoverable "available-skills" candidate — a fixed header pointing at builtin.skill_activate plus one line per skill (250-char description cap, 100-entry cap). Full bodies inject only for explicit $name mentions and model-selected activations. Keyword scoring still runs and RANKS the listing; criteria selections no longer consume the model's activation budget. * `Full` stays the library default so non-Reborn consumers and the skill-execution capture plan keep their existing semantics. - Prompt build and by-ref model-message resolution rebuild the snippet set independently, so both now derive body-eligibility and listing ranking from the merged active plan — identical sets, stable msg:snippet refs. - `SkillContextService` safe summaries are now the bounded first line of the description instead of the full text: the multi-skill listing (5.6KB) blew past MODEL_SAFE_SUMMARY_MAX_BYTES and killed runs at the prompt stage — a latent bug for any long skill description. v1 (`src/`) skill behavior is untouched. Tests: reborn_integration_skill_activate pins the one-liner listing (body absent pre-activation, present post-activation) at the captured model-request seam; new selector listing-mode tests in ironclaw_first_party_extension_ports; safe-summary regression in skill_context_service_contract; env parse + config propagation tests in ironclaw_reborn_composition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Upgrade criteria-listed skills to ModelSelected on later skill_activate merge_active_plan deduped merged activations by bundle id alone, so a skill already in the run plan as a listing-only ActivationCriteria entry silently dropped a later ModelSelected activation for the same bundle: the mode never upgraded, body_eligible_bundle_ids never included it, and the SKILL.md body never injected — while the skill_activate tool still reported "activated 1 skill(s)" because the merged plan matched by name. Mode-priority merge: when the incoming activation is body-eligible (ModelSelected/ExplicitMention) and the existing entry for the same bundle is ActivationCriteria, upgrade the existing entry's mode in place; never downgrade. Regression test drives record_user_message (criteria match) then activate_skills_for_run and asserts the body injects on the next prompt build and leaves the listing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Hash full model content into skill snippet model-message refs skill_snippet_model_message_ref hashed only [snippet_ref, safe_summary, ordinal], and the safe summary is first-line-only (capped 256 chars). The available-skills listing's first line is a constant header, so adding, removing, or reordering listed skills changed the listing body without changing the ref — a stale ref resolved against the new content instead of failing closed. Include the full model_content in the hashed fields. Refs are per-run ephemeral (built at prompt time, resolved live by instruction_snippet_messages_by_ref in the same run; never persisted — the executor's message index stores only sequence/kind/token counts), so the deliberate one-time hash rotation is safe; pinned ref literals in the turns/loop-support contract tests are updated to the new layout. Regression tests pin that listings differing only after the first line produce different refs and identical content produces a stable ref. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Require read-before-edit and reject stale edits in reborn coding tools Benchmark traces show models editing files from a stale in-context view and blindly overwriting files they never read: builtin.apply_patch and builtin.write_file had no guard against either, unlike Claude Code's Edit tool (read-before-edit + mid-air collision detection). This brings the reborn coding tools to parity. read_file now records a content fingerprint (cheap std hash of the raw bytes it already holds) per scope + resolved virtual path in a bounded registry on CodingCapabilityState, shared across concurrent runs exactly like edit_locks. Only a full read (no offset/limit window) records state. write_file and apply_patch on an EXISTING file now reject with a recovery-worded, model-visible safe summary when: - no read was recorded ("read it with read_file before editing it"), or - the file's current fingerprint no longer matches the recorded one ("the file changed since it was last read; read it again ..."). Creating a new file needs no prior read, a successful edit refreshes the fingerprint so chained edits don't re-trip, and write-only mounts are exempt (the model cannot read there, so blind overwrite is the intended mode). Eviction from the bounded registry fails safe: a missing entry only forces a fresh read_file. Regression tests: dispatch-tier tests in coding/mod.rs pin all five behaviors (unread-write rejection, read-then-patch, out-of-band staleness, new-file write, chained edits); host_runtime caller-tier tests updated to seed read state through the public read path, and the former builtin_apply_patch_accepts_unique_match_without_prior_read pin is inverted to builtin_apply_patch_requires_prior_read_of_existing_file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Only record read-before-edit state for complete, untruncated reads PR 5978 review finding 2 (ironloopai, MEDIUM): read_file recorded the read-before-edit fingerprint whenever the call had no explicit offset/limit, BEFORE the default output truncation was applied. A default read of a file over the 2,000-line or 64 KiB output cap returned partial content with truncated=true, yet still unlocked whole-file write_file/apply_patch - violating the guard's own stated invariant that only a full read proves the model has seen the file. read_file now computes the rendered output first and records the fingerprint only when the read had no explicit range AND was not truncated at the line or byte cap (missing flag fails safe as truncated). The read-before-edit rejection message now also explains that ranged and truncated reads do not count, so the model understands why its earlier read did not unlock the edit and that a file too large to read in full cannot be blind-edited with these tools. Regression tests (caller-tier, driven through the composed host runtime in first_party_coding_tools.rs): - default read of a >2,000-line file (line truncation) does not unlock write_file, and an explicit offset/limit read wide enough to cover the file still does not unlock it - default read of a >64 KiB-rendered file (byte truncation) does not unlock apply_patch, even when the patch anchor was inside the visible window - rejected edits leave the file untouched and carry the model-visible full-read guidance Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Scope coding read-before-edit state to the loop turn-run PR 5978 review finding 1 (ironloopai, HIGH): CodingReadScopeKey keyed read-before-edit state by tenant/user/agent/project/mission/thread only, and CodingCapabilityState lives in the long-lived BuiltinFirstPartyTools handler. A read_file recorded in one run could therefore authorize write_file/apply_patch in a LATER run that never read the file, as long as the content fingerprint still matched. Security framing: the fingerprint check already guarantees content equality at edit time, so this is a policy tightening - the model must have SEEN the file within the current run - not a data-corruption fix. Why a new wire field: the reviewer suggested keying by request.scope.invocation_id, but invocation_id is minted per tool call (InvocationId::from_uuid(activity_id) in invocation_context_from_visible), so keying on it would break the feature entirely - the read and the subsequent edit are always different tool calls. Nothing between thread_id (spans many runs) and invocation_id (one tool call) existed on the wire, so this adds the prompt-visible turn-run identity that the loop host already owns (LoopRunContext::run_id) as an optional, host-stamped RunId, mirroring the existing authenticated_actor_user_id plumbing hop for hop: ExecutionContext.run_id (stamped in invocation_context_from_visible) -> CapabilityDispatchRequest.run_id (capability host) -> RuntimeAdapterRequest.run_id (dispatcher) -> FirstPartyCapabilityRequest.run_id (first-party adapter) -> CodingCapabilityRequest.run_id -> CodingReadScopeKey.run_id run_id is serde(default) so persisted legacy contexts still deserialize; non-loop callers (system services, spawned processes, one-shot product invocations) carry None, which is its own key bucket, never a wildcard. Reads recorded in run A neither unlock run B nor the None bucket, and vice versa; a read in tool-call N still unlocks an edit in tool-call N+2 of the same run. Regression tests: - caller-tier (first_party_coding_tools.rs, through the composed host runtime): read in run A, write_file/apply_patch with a same-identity scope carrying a different run_id are rejected with the read-before-edit error and leave the file untouched; a later tool call (fresh invocation_id) of the SAME run still edits, pinning run-not-invocation granularity - loop_support: invocation_context_from_visible stamps run_id from LoopRunContext::run_id, so a dropped stamp cannot silently collapse every run into the shared None bucket - host_api: legacy ExecutionContext payloads without run_id (and authenticated_actor_user_id) still deserialize Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Surface new post-edit check diagnostics on reborn coding edits (#5979) * Surface new post-edit check diagnostics on reborn coding edits Benchmark failure analysis shows agents breaking adjacent code without noticing: a fix passes its own test while a sibling test starts failing, and nothing in the tool loop tells the model. Mirror Claude Code's baseline-diff diagnostics push with a minimal reborn seam: after a SUCCESSFUL builtin.write_file / builtin.apply_patch dispatch, run an operator-configured check command through the invocation's RuntimeProcessPort and append only the NEW diagnostic lines to the edit's model-visible output as a post_edit_check field. Config seam: IRONCLAW_POST_EDIT_CHECK (shell command; feature off when unset/blank) and IRONCLAW_POST_EDIT_CHECK_TIMEOUT_SECS (default 30). Composition resolves it once via the module-owned PostEditCheckConfig::from_env (build_local_runtime -> HostRuntimeServices::with_post_edit_check) and threads it through LocalInvocationServicesResolver into InvocationServices, mirroring unsafe_raw_diagnostics_allowed - per-call handlers never read env. Execution seam: the coding fallback arm of BuiltinFirstPartyTools::dispatch, where the already-resolved services.process port sits next to the dispatch result. The check runs with the first read+write mount alias as workdir so the process port resolves it exactly like a shell workdir (/workspace -> the host workspace root); read-only coding tools never trigger it. New-only filtering: a bounded per-scope seen-line registry (same scope key dimensions as the coding read-state registry; 500 lines/scope FIFO, 512 scopes) reports up to 30 lines / 4000 bytes per edit with a "+N more new lines" note, marking only reported lines as seen so trimmed lines surface on a later check. The check is advisory and never fails the edit: no new findings -> {"exit_code": N} only; timeout -> {"timed_out": true}; a check that cannot run at all is debug-logged and omitted. v1 limitation (documented in the module): the seen-set is global per scope with no per-file clearing, so a diagnostic that disappears and reappears unchanged is not re-reported until evicted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Gate the post-edit check behind the process policy and account its spawn The IRONCLAW_POST_EDIT_CHECK command ran through the invocation's default process port from write_file/apply_patch plans that declare only filesystem effects, so under ProcessBackendKind::None or a tenant-sandbox policy the check spawned a host process outside process-backend selection, and the spawn was never accounted. LocalInvocationServicesResolver now populates InvocationServices::post_edit_check only when the plan's process policy resolves local host execution (ProcessBackendKind::LocalHost under DeploymentMode::LocalSingleUser — the same arm that hands the local port to process-requiring plans like builtin.shell, now shared via one predicate). Under any other backend the advisory check is withheld, not rerouted. A check that does run is accounted as process_count=1 on the edit's ResourceUsage, mirroring builtin.shell. Default local-dev behavior is unchanged. Regression tests: resolver-tier coverage of the LocalHost/None/ TenantSandbox/hosted-LocalHost gating, caller-tier write_file dispatch under a no-process policy and a tenant-sandbox policy (asserting zero local-host and zero sandbox-port invocations), and process accounting asserted in the existing post-edit-check findings test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): PR #5978 review + isolate post-edit check per tenant Read-before-edit guardrail fixes: - Split CodingEditLockKey (run-agnostic) from CodingReadScopeKey (run-scoped): concurrent runs editing the same path now contend on one edit-lock stripe. Folding run_id into the lock key sent them to different stripes, so both could pass their fingerprint check and overwrite each other (lost update). (ironloop) - apply_patch reuses verify_read_before_edit, sharing the recorded-read / size-limit / re-read / fingerprint checks with write_file. (coderabbit) - The post-edit check runs in the writable mount backing the edited file, not an arbitrary first writable mount. (coderabbit) Post-edit check multi-tenant isolation (ironloop/coderabbit #4): - The check no longer runs on the deployment-blind local process port. The invocation-services resolver now routes it to the port matching the plan's process backend -- the tenant sandbox under HostedMultiTenant -- so a tenant's command runs isolated in that tenant's own sandbox, never on the shared provider host, and is disabled when no backend can run it in isolation. - Wired the operator check into production composition (previously local-dev only). Regression tests: cross-run edit-lock serialization, run-agnostic lock key vs run-scoped read key, edited-mount selection + alias-boundary match, and resolver isolation routing (local host vs tenant sandbox vs disabled). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): reconcile post-edit-check merge with main (#5978) Merging main (PR #5979, the post-edit-check diagnostics PR) into this branch surfaced two integration gaps that were failing CI: - main's new `first_party_tools/http.rs` save-request test constructed `FirstPartyCapabilityRequest` / `InvocationServices` without this branch's added `run_id` / `post_edit_check` fields, breaking the default-feature build. GitHub's `pull_request` CI builds the PR merged into base, so it hit this before a local merge did. - `builtin_edit_tools_disable_post_edit_check_under_tenant_sandbox_policy` (introduced by #5979) encoded the pre-rework behavior where the check was withheld outright under a tenant-sandbox policy. This branch's rework (82d7148) instead routes the check to the tenant-sandbox process port so it runs ISOLATED in the tenant's own sandbox — the behavior the authoritative resolver test `local_resolver_routes_post_edit_check_to_the_deployment_isolated_process_port` pins. #5979's test couldn't be updated in that commit because it only existed on main. Rewrote it as `builtin_edit_tools_run_post_edit_check_in_tenant_sandbox_not_on_local_host`, asserting the check runs through the sandbox port (surfacing its diagnostics) and never escapes onto the local host port. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 29, 2026
15 of 29 tasks
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
2 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.
Stack 1/4 on #5959. Lessons from studying Claude Code's execution engine, applied to the measured KV-cache collapse (82% hit rate at ~120 calls → 29% at 425 calls on claw-swe-bench run
799636ce, a ~3.5× cost multiplier on long agentic turns).Part 1 — prompt-cache break detector (
ironclaw_runner::model_gateway)Every completed model call now emits a per-call
tracing::debug!cache-usage series (cache_read / cache_creation / input tokens), and atracing::warn!"prompt cache break detected" when the turn-over-turn cache read drops >5% AND >10K tokens — with attribution hints: seconds since last call, whether the tool-definition list changed (name-list hash), whether the system prompt changed (content hash). Per-run scoping via a bounded activity log on both gateway flavors. This is the diagnostic that tells us why the hit rate collapses (compaction rewrite vs provider routing vs surface churn) instead of guessing from run totals.Part 2 — compaction circuit breaker (
ironclaw_agent_loop)Claude Code added the same guard after measuring ~250K wasted API calls/day from doomed auto-compaction loops. If compaction runs 3 consecutive times without bringing the token estimate back under the trigger threshold, the circuit opens for the rest of the run (one warn, covers forced/byte-cap compaction too; old checkpoints rehydrate with the breaker closed). State lives in the typed
CompactionStrategyStateslot per the strategies contract; no default parameter values changed. Family fingerprints gainedineffective_trip_limit=3and digests were regenerated red→green.Testing
Red-then-green throughout: pure-function tests for the break predicate (boundary cases), activity-log scope isolation + attribution flags, slot transition tests, strategy skip tests, a through-the-caller
PromptStageexecutor test, and a gateway-tier#[traced_test]asserting the warn fires within a run (and stays quiet at exact thresholds).cargo test -p ironclaw_agent_loopfully green in seconds (this stack's base also pauses tokio time in the scenario test targets, which the deep availability backoff from #5959 had slowed to ~423s). fmt + clippy clean.Deferred (documented in code): message-prefix divergence attribution (prior-call messages aren't retained at the gateway), tool schema hashing (names+count only), eviction-on-run-completion (no lifecycle signal at this layer).
🤖 Generated with Claude Code