Repository navigation
feat(loop): derive the prompt context budget from the model's advertised window - #8053
henrypark133 wants to merge 29 commits into
Conversation
…ed window Adds PromptContextTokenBudget::from_advertised_window, which turns a provider-advertised total context window into a usable budget: 90% of the window to absorb chars/4 estimate error, with the flat response reserve clamped so a small-window model still has room for transcript. None (or zero) reproduces the compiled-in 128k/20k exactly, so a provider that advertises nothing behaves as it does today. No caller yet. Also adds serde::Deserialize, which the LoopRunContext field needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
Adds resolved_context_budget: Option<PromptContextTokenBudget>, mirroring the existing resolved_model_route field and builder exactly: serde-default so runs recorded before this change still replay, and one builder for the single writer that will set it. No producer or consumer yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
Structural only. can_evaluate and trigger_at take the budget instead of reading self.prompt_context_budget, and all 7 call sites pass exactly the value the field read produced, so every compaction decision is identical. Zero test edits: no test calls either helper directly (the test named can_evaluate_skips_when_visible_threshold_equals_preserve_tail asserts on should_compact). That zero-churn result is the proof this is internal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
should_compact now reads ctx.resolved_context_budget via effective_budget, falling back to the strategy's own budget when the run resolved none. Both CompactionStrategy implementors go through the same helper. The compaction ceiling is no longer a fixed property of the family, so the four replay fingerprints say context_limit=run_context and all four ComponentDigest constants are recomputed from failing-test output. The digest now stays stable across models rather than encoding one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
… gateway Adds HostManagedModelGateway::advertised_context_window_tokens, a defaulted async trait method copying diagnostic_effective_model's shape, so every other implementor keeps today's behavior unchanged. LlmProviderModelGateway overrides it by reading ModelMetadata.context_length -- a field that has had no consumer outside ironclaw_llm until now. It only trusts the window when the model the provider describes is the model this run will actually be served: request_model_override lets a route override the model, and borrowing a larger model's window is exactly the provider rejection this work exists to prevent. A mismatch falls back to the default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
Structural only. ThreadResolvingLoopModelGatewayParts gains a thirteenth field, carried onto the gateway and applied to ThreadBackedLoopModelPort via the builder that until now had only test callers. That port's resolve_model_messages is what calls select_prompt_context_messages -- the call deciding which transcript messages actually reach the provider. It was the one budget consumer the earlier draft of this work left on the compiled-in default, which would have shipped a loop that compacts against one ceiling and sizes requests against another. Both construction arms pass self.config.prompt_context_budget, which is PromptContextTokenBudget::default() today -- byte-identical to the port's own default -- so request sizing is unchanged. Task 7 changes the value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
…odel build_text_only_host_with_capabilities now asks the run's gateway for the model's advertised context window, derives a budget from it, and carries it on LoopRunContext. All four consumers read the one resolved local: the compaction strategy (via the run context), the prompt context port, both model-gateway construction arms, and structured finalization. Two details worth keeping: - The gateway is resolved once via resolve_for_scope and that same object answers the window query and serves the run. Asking self.model_gateway while a scope override is active would let the budget describe a different gateway than the one issuing the request. - The await sits after the three advisory prefetch kickoffs, not before them, so it cannot serialize work the surrounding code deliberately runs in parallel. A gateway that advertises nothing leaves the run on the configured default, which is exactly today's behavior. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
TraceLlm -- the SDK-seam fake the in-process integration tier mocks at -- had no model_metadata impl, so it inherited the trait default and reported no window. That made the model-derived budget unreachable from any integration scenario. Adds an advertised_context_window field with a with_* setter and a model_metadata override, plus an advertised_context_window(tokens) method on the harness thread builder that applies it. Unset stays None, so every existing scenario is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
…ext budget ThreadResolvingLoopModelGateway is what the driver host hands the loop, so a budget that stops at the wrapper never sizes the outbound request. The port-level test already proved the port honors a budget; this pins that the wrapper forwards its own budget instead of the port's compiled-in default. Fails with all five seeded messages forwarded when the builder call in stream_model_inner is removed. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
…und request The existing tests proved a resolved budget lands on the run context; this one drives an empty model request through the built host and asserts the gateway receives only the transcript tail the derived budget admits. Fails with all five seeded messages forwarded when the model port is handed the config default instead of the run-derived budget. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
…eal turn The scripted provider advertises a 40k context window; through the real LlmProviderModelGateway (route-identity check included) the run derives a 36k/9k budget and the model is sent fewer transcript messages than an unadvertised run, which keeps the compiled-in 128k/20k ceiling. Fails with '13 vs 13' when the production gateway stops reporting a window. Compaction and request sizing share one threshold on this path, so this tier does not separate them; each link of the request-sizing chain is pinned by mutation-verified crate tests in ironclaw_loop_host and ironclaw_turn_runner. Adds the harness builder knob and captured_request_message_count, and the tests/AGENTS.md coverage row. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
- ironclaw_llm CONTRACT: model_metadata().context_length now drives the per-run prompt budget via LlmProviderModelGateway::advertised_context_window_tokens. - model_gateway: tag the advisory .ok()? with the silent-ok marker the error-handling rule greps for. - integration support: restore script()'s doc comment, displaced by the advertised_context_window builder. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
Matches the crate's host/run_context/tests.rs and runtime_context/tests.rs layout. No behavior change; same eight tests. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
…13773 Genuine contract growth from the model-derived prompt budget: the from_advertised_window derivation on the type this crate owns and the optional resolved budget on LoopRunContext (~41 lines). main already sat 3 lines under the effective ceiling. Count read from the test's own failure message; reason recorded in the ladder comment. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
…dget Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
TokenRefreshingProvider::model_metadata ran ensure_fresh_token first — a possible HTTP POST to the OAuth token endpoint under the renewal lock shared with in-flight complete() calls — before delegating to an inner that builds a static struct needing no credential. With model_metadata() now awaited once per turn-run host build (advertised_context_window_tokens), every turn on the OpenAI Codex chain could pay a network round trip, or wait behind an in-flight refresh, to learn nothing. Delete the refresh; pin it with a test whose counting local endpoint saw one connection before the fix and none after. Make the rule explicit in the ironclaw_llm CONTRACT (model_metadata is a static, I/O-free description; decorators delegate it unchanged) and point the driver host's critical-path comment at that contract instead of at an observation. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
|
🚅 Deployed to the ironclaw-pr-8053 environment in ironclaw-ci-preview
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe change derives per-run prompt budgets from provider-advertised context windows. It stores budgets on the run context and applies them to transcript sizing and compaction. It preserves compiled-in defaults when metadata is absent or mismatched. ChangesModel-derived prompt context budget
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change adapts agent prompt budgets to matching provider-advertised model windows while preserving existing defaults when metadata is unavailable. No current merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant TurnRunner
participant LlmProviderModelGateway
participant LlmProvider
participant LoopRunContext
participant CompactionStrategy
participant ThreadBackedLoopModelPort
TurnRunner->>LlmProviderModelGateway: query advertised context window
LlmProviderModelGateway->>LlmProvider: model_metadata()
LlmProvider-->>LlmProviderModelGateway: context_length and model id
LlmProviderModelGateway-->>TurnRunner: matching window or None
TurnRunner->>LoopRunContext: store resolved_context_budget
TurnRunner->>CompactionStrategy: evaluate with effective budget
TurnRunner->>ThreadBackedLoopModelPort: apply prompt context token budget
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description covers the required sections and provides detailed validation evidence. However, the new feature has no linked approved issue, despite the template requiring one. The description also states that pr-shepherd has not yet been run after a coding-agent change.
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 120402e9ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
The new integration test relies on a fixed captured-request index that can shift when compaction adds extra model calls, making the assertion potentially brittle/flaky.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes the agent loop’s prompt context budget model-aware by deriving it from the provider-advertised context window (when available) and threading the resolved budget through the turn-runner host build into all relevant loop consumers (compaction + message selection), while preserving today’s 128k/20k behavior when no window is advertised.
Changes:
- Add
PromptContextTokenBudget::from_advertised_window(..)and carry an optional resolved budget onLoopRunContext. - Extend the loop-host gateway surface to report an advertised context window (with route/model-id verification) and wire the resolved budget through turn-runner host construction and model/message selection.
- Update compaction strategies and replay fingerprints/digests to treat the compaction ceiling as run-scoped, and add/adjust contract + unit + integration tests (including SDK-seam fakes) to pin the end-to-end behavior.
File summaries
| File | Description |
|---|---|
| Cargo.toml | Registers new integration test target for context-budget scenario. |
| crates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rs | Re-pins ironclaw_loop_contracts size ceiling with rationale for the new lines. |
| crates/contracts/ironclaw_loop_contracts/src/context_budget.rs | Adds Deserialize + from_advertised_window derivation; moves tests out-of-line. |
| crates/contracts/ironclaw_loop_contracts/src/context_budget/tests.rs | Adds coverage for advertised-window budget derivation behavior. |
| crates/contracts/ironclaw_loop_contracts/src/host/run_context.rs | Adds resolved_context_budget: Option<_> + builder on LoopRunContext. |
| crates/contracts/ironclaw_loop_contracts/src/host/run_context/tests.rs | Adds serde/default and builder/round-trip tests for the new run-context field. |
| crates/domains/ironclaw_llm/CONTRACT.md | Documents model_metadata() invariants + the new runtime consumption of context_length. |
| crates/domains/ironclaw_llm/src/token_refreshing.rs | Removes token refresh from model_metadata() and adds a regression test to pin I/O-free behavior. |
| crates/loop/ironclaw_agent_loop/src/families/mod.rs | Updates default family fingerprint/digest for run-scoped compaction budget. |
| crates/loop/ironclaw_agent_loop/src/families/subagent.rs | Updates subagent family fingerprint/digest for run-scoped compaction budget. |
| crates/loop/ironclaw_agent_loop/src/families/unbound.rs | Updates unbound family fingerprints/digests for run-scoped compaction budget. |
| crates/loop/ironclaw_agent_loop/src/strategies/active_task_compaction.rs | Uses run-context resolved budget for compaction; adds tests for override/fallback behavior. |
| crates/loop/ironclaw_agent_loop/src/strategies/compaction.rs | Uses run-context resolved budget for compaction; refactors helper signatures; adds tests. |
| crates/loop/ironclaw_loop_host/src/lib.rs | Adds defaulted advertised_context_window_tokens to HostManagedModelGateway. |
| crates/loop/ironclaw_loop_host/src/model_gateway.rs | Implements advertised-window lookup on provider-backed gateway with route/model-id check; adds tests. |
| crates/loop/ironclaw_loop_host/src/thread_resolving_model_gateway.rs | Threads a prompt-context budget into the wrapper so outbound request sizing uses the resolved budget. |
| crates/loop/ironclaw_loop_host/tests/thread_loop_host_contract.rs | Adds contract test proving the wrapper applies its injected budget to message selection. |
| crates/loop/ironclaw_turn_runner/src/loop_driver_host.rs | Resolves advertised window once per host build, sets run-context budget, and wires the effective budget to ports/gateway. |
| crates/loop/ironclaw_turn_runner/src/loop_driver_host/context_budget_tests.rs | Adds driver-host tests pinning run-context budget resolution and request sizing behavior. |
| docs/internal/reborn/design/model-derived-context-budget.md | Adds design/spec doc describing the end-to-end shape and invariants. |
| docs/internal/superpowers/plans/2026-09-01-model-derived-context-budget.md | Adds detailed implementation plan/playbook for the change. |
| tests/AGENTS.md | Updates scenario coverage map to include the new integration test. |
| tests/integration/context_budget.rs | Adds integration scenario asserting advertised-window runs send fewer messages than unadvertised runs. |
| tests/integration/support/assertions.rs | Adds harness helper for inspecting captured request message counts. |
| tests/integration/support/builder.rs | Plumbs advertised context window through integration harness builder to scripted provider. |
| tests/integration/support/group.rs | Plumbs advertised context window through grouped-thread builder path. |
| tests/support/trace_llm.rs | Extends TraceLlm to optionally advertise a context window via model_metadata(). |
| tests/trace_llm_tests.rs | Adds tests verifying TraceLlm’s default vs configured advertised context window. |
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review · Status🟩 CompletedIronLoop completed the review and posted it to GitHub. ResultRun detailsAutomatic trigger · attempt 1 of 3 · completed in 21m 55s |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core loop budgeting behavior across multiple crates and relies on cross-component invariants (provider metadata + host build + compaction + request sizing) that warrant final human review despite strong test coverage.
Review details
- Files reviewed: 34/34 changed files
- Comments generated: 0 new
- Review effort level: Lite
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Derive per-run prompt context budgets from the served model’s advertised window while preserving defaults and removing unnecessary metadata token-refresh I/O.
Stats: 3 findings (from 6 raw, 6 after filter, 3 after dedup) across 3 files. Reviewers run: correctness, security, performance, design, coverage. Reviewers failed: none. Body-only: 0
Unresolved review threads at emission: 0
Mechanical
- Medium New plan file exceeds the repository’s 1,000-line file-size ceiling (
docs/internal/superpowers/plans/2026-09-01-model-derived-context-budget.md:1-1332, confidence 90) — anchor: docs/internal/superpowers/plans/2026-09-01-model-derived-context-budget.md:1
This change adds a 1,332-line plan document, crossing the repository’s stated 1,000-line file-size ceiling.
Correctness / bugs
- High Budget ignores smaller failover provider windows (
crates/loop/ironclaw_loop_host/src/model_gateway.rs:431-451, confidence 95) — anchor: crates/loop/ironclaw_loop_host/src/model_gateway.rs:433
When the gateway wraps FailoverProvider, model_metadata() describes only its current last_used provider, initially the primary. If a large-window primary fails and fallback advances to a smaller-window provider, the run retains the primary-derived budget and can send a prompt larger than the fallback accepts, causing repeated context-overflow failure instead of recovery.
Also flagged by: coverage/High, design/Medium
Coverage / tests
- Medium Token-refresh regression is not tested through its production caller (
crates/domains/ironclaw_llm/src/token_refreshing.rs:620-632, confidence 91) — anchor: crates/domains/ironclaw_llm/src/token_refreshing.rs:620
model_metadata_does_not_refresh_the_token calls TokenRefreshingProvider::model_metadata() directly. The side effect matters because LlmProviderModelGateway::advertised_context_window_tokens invokes that decorator during host construction; a future gateway/decorator wiring change could reintroduce the HTTP call while the leaf test remains green.
Also flagged by: coverage/Medium
…chain model_metadata() returned providers[last_used]'s metadata. The gateway reads it once per run at host build, while last_used is still the primary, and the derived budget lives on the run context for the whole run; a later in-run failure (or the gateway's fallback_index walk) serves a smaller-window fallback with the primary's budget and overflows instead of recovering. Composition builds the chain from a primary and a fallback_model it expects to differ. Advertise min(context_length) across every chain member, None if any is unknown, keeping id on last_used so the gateway's identity check stays consistent with active_model_name(). Same shape as SmartRoutingProvider; CONTRACT names failover chains alongside routing. Swappable delegates live to its current inner and is operator-swapped, not failure-driven — unchanged. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
…h the gateway The token-refresh regression test called TokenRefreshingProvider directly; the production caller is LlmProviderModelGateway::advertised_context_window_tokens during host build, and a decorator re-wiring could reintroduce the HTTP call with the leaf test still green. Drive the real gateway over the real decorator with a session loaded from an expired on-disk session file (no new seam) and a counting local endpoint: 1 connection with the refresh reinserted, 0 with the fix. Lives in tests/llm_gateway.rs beside the other real-gateway tests: the reborn_product_api_crates_do_not_bind_http_ingress gate scans this crate's src/ (test modules included) for TcpListener::bind, and the loopback counting endpoint is exactly that. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
There was a problem hiding this comment.
🟡 Changes recommended
There are still correctness/perf issues on the host-build-critical model_metadata() path (redundant metadata awaits in FailoverProvider) and a semantic mismatch where “unknown” advertised windows can incorrectly populate resolved_context_budget instead of leaving it None.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/loop/ironclaw_turn_runner/src/loop_driver_host.rs:1687
PromptContextTokenBudget::from_advertised_window(Some(window))intentionally treatsNone/0/too-small windows as “unknown” by returning the compiled-in default. But the host-build currently stores that default back intorun_context.resolved_context_budgetwhenever the gateway returnsSome(window)(including0or other values that derive to the default), which contradicts theLoopRunContextfield docs that sayNonerepresents “provider advertises no window / fall back to the default”. Consider only settingresolved_context_budgetwhen the derived budget actually differs from the configured fallback, and filter outwindow == 0up front.
- Files reviewed: 36/36 changed files
- Comments generated: 1
- Review effort level: Lite
…indow Review nit: model_metadata() awaited the last_used member twice and kept querying after the aggregate window had already become None. One call per member, id captured for last_used, early exit once the aggregate is None. Same result. Pinned by two tests counting model_metadata() calls per member: on the old body the last_used member is queried twice (left: 2, right: 1). Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core loop runtime budgeting/compaction semantics across multiple crates and relies on subtle critical-path contract guarantees that merit a final human review despite strong test coverage.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/loop/ironclaw_turn_runner/src/loop_driver_host.rs:1664
- The comment here says the await resolves immediately because
model_metadata()is I/O-free, but the awaited call isadvertised_context_window_tokens(), which is a gateway method that can be overridden independently ofLlmProvider::model_metadata(). As written, this reads like a stronger guarantee than the trait provides and could mislead future implementors into adding a slow/I/O implementation without noticing it lands on the host-build critical path.
Consider rewording to explicitly tie the “fast/no-I/O” assumption to the provider-backed gateway’s implementation (which delegates to model_metadata()), and keep the ponytail note as the fallback if that assumption changes.
- Files reviewed: 36/36 changed files
- Comments generated: 1
- Review effort level: Lite
…room The three reborn_*_location_scan binaries each carry one scan of the entire crates/ tree. On the last green run they finished at 176.8s against the ci profile's 60s x 3 = 180s hard kill; the next run was terminated at 180.008s with no code change that touches their runtime. reborn_sealed_evidence_mint_ratchet rides the same ladder to 136-141s. One override for that family at 60s x 6 keeps the SLOW cadence and stops every PR's architecture bucket being a coin flip; the default stays as is for every other binary. Match verified locally: with the override's period set to 150s no SLOW marker fires (tests take ~110s); with 60s the markers return at 60s. Rollback: delete the override block. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
There was a problem hiding this comment.
🔵 Needs a closer look
It changes critical-path prompt sizing/compaction behavior across multiple crates and provider decorators, so it needs a final human review despite strong test coverage.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tests/integration/support/assertions.rs:591
captured_last_request_message_count()callsTraceLlm::captured_requests()(which clones every captured request and message) and then reads.last(). With large transcripts (as in the new context-budget integration tests), this does unnecessary O(total captured size) cloning just to get the last request length and can significantly slow tests / increase memory.
Prefer a dedicated TraceLlm accessor that reads the last captured call under the lock (e.g. captured_last_request_len() / captured_last_request_messages()), cloning only what’s needed (or just returning the length), and use that here (and in captured_last_request_contents()).
This issue also appears on line 599 of the same file.
tests/integration/support/assertions.rs:603
captured_last_request_contents()also callsTraceLlm::captured_requests()and then takes.last(), which clones all captured requests/messages even though only the last request is needed. This is especially expensive when messages contain largecontentstrings.
Consider adding a TraceLlm API that returns a clone of only the last request’s messages (or iterates them under the lock) so this helper doesn’t duplicate the entire capture history.
- Files reviewed: 37/37 changed files
- Comments generated: 0 new
- Review effort level: Lite
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Derive prompt context budgets from the model’s advertised context window and remove token-refresh I/O.
Stats: 3 findings (from 3 raw, 3 after filter, 3 after dedup) across 2 files. Reviewers run: correctness, security, performance, design, coverage. Reviewers failed: none. Body-only: 0
Mechanical
- Medium New plan document exceeds the 1,000-line file-size ceiling (
docs/internal/superpowers/plans/2026-09-01-model-derived-context-budget.md:1-1332, confidence 90) — anchor:docs/internal/superpowers/plans/2026-09-01-model-derived-context-budget.md:1
This change adds a 1,332-line plan document. The repository change contract caps touched source files at roughly 1,000 lines; large planning artifacts are harder to review and maintain.
Regression escape
- Medium Route-override model identity is not regression-tested (
crates/loop/ironclaw_loop_host/src/model_gateway.rs:443-450, confidence 94) — anchor:crates/loop/ironclaw_loop_host/src/model_gateway.rs:443
The metadata path is tested only withresolved_model_route = Noneand a policy-level override. Production runs can carry aHostManagedModelRouteSnapshot; a regression could derive a budget from the wrong model.
Tests
- Medium Metadata-error fallback is not regression-tested (
crates/loop/ironclaw_loop_host/src/model_gateway.rs:436, confidence 87) — anchor:crates/loop/ironclaw_loop_host/src/model_gateway.rs:436
The advisory fallback whenprovider.model_metadata()returns an error is untested. A provider/decorator failure must leave the run on the compiled-in budget rather than fail host construction or use partial metadata.
…a-error paths Review round three: the resolved-route-snapshot branch of the identity check and the model_metadata() error fallback had no tests. Snapshot naming another model -> None (fails with Some(200000) when the identity check is removed); snapshot naming the served model -> the window; metadata error -> None (the double's error flag is what flips the outcome). The trait doc now states the probe is awaited on the host-build critical path and must be cheap. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
There was a problem hiding this comment.
🔵 Needs a closer look
It changes critical-path turn-run host construction and cross-crate loop budgeting behavior, so it warrants final human review despite strong test coverage.
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The derivation is consistently wired end-to-end with strong multi-tier regression coverage, and the model_metadata() critical-path I/O removal is enforced by both code and contract tests.
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
PromptContextTokenBudgetregardless of model. Runs now derive it from the provider-advertised context window:PromptContextTokenBudget::from_advertised_windowtakes 90% of the window as the limit and keeps the flat 20k response reserve, clamped to a quarter of the limit so small-window models keep a usable transcript.None/0reproduce today's default exactly.HostManagedModelGateway::advertised_context_window_tokensand carries the result onLoopRunContext.resolved_context_budget(not persisted). The productionLlmProviderModelGatewayreadsModelMetadata.context_lengthonly when the metadata'sidmatches the model the request will actually be served by, so a route override never borrows another model's window.ironclaw_agent_loop, which stays contracts-only), the context port, the model port viaThreadResolvingLoopModelGateway, and structured finalization. The four family replay fingerprints drop the hard-codedcontext_limit=128000,reserve=20000literals (digests recomputed) so the replay identity stays stable across models.TokenRefreshingProvider::model_metadata()ran a token-refresh HTTP call (under the lock shared with in-flightcomplete()) before delegating to a static answer. Removed, pinned by test, and made a CONTRACT rule:model_metadata()is a static, I/O-free description.ironclaw_loop_contractsgrew by ~41 genuine lines against a ceilingmainalready sat 3 lines under; tests moved to the crate's out-of-linetests.rslayout and the ceiling re-pinned at the measured 13,773 with the reason in the ladder comment.Design note:
docs/internal/reborn/design/model-derived-context-budget.md. Plan:docs/internal/superpowers/plans/2026-09-01-model-derived-context-budget.md.Change Type
TokenRefreshingProvider::model_metadatatoken-refresh I/O)context_budgettests out of line; compaction budget passed as an argument)Linked Issue
None — no tracking issue exists for this; opened from a direct request. Flagging for the maintainer whether one should be filed and linked.
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings(zero warnings, run twice: pre- and post-fixup tree)cargo build(as part of the test runs)cargo test -p ironclaw_llm -p ironclaw_loop_contracts -p ironclaw_agent_loop -p ironclaw_loop_host -p ironclaw_turn_runner -p ironclaw_architecture_tests;cargo test -p ironclaw_integration_tests --test reborn_integration_context_budget --test reborn_integration_model_recovery --test reborn_integration_greeting; fullcargo test -p ironclaw_integration_testsand-p ironclaw_compositionwithRUST_MIN_STACK=67108864.cargo test -p <owning-crate> --features integration— Not applicable: no database-backed behavior changed.pr-shepherd— not yet run; will run on CI feedback.Pre-existing failures observed and confirmed identical on
mainat99457e152, none in files this branch touches:ironclaw_turn_runnertrace_capture::tests::capture_skips_when_policy_missing_or_disabled; five trace-commons tests that read a stray real~/.ironclaw/trace_contributions/policy.jsonon the dev machine;ironclaw_compositioncapability_port_omits_host_disclosure_without_confirmed_host_mount(FilesystemDeniedvsInputEncode);reborn_user_submit_completes_while_another_turn_state_write_is_blockedflakes 2–4 of 8 isolated runs on both branches on its 5 s window (its harness never reaches this branch's code path).Test Strategy
User behavior: a run served by a model that advertises a context window gets a prompt budget sized to that model — a 40k-window model is sent a smaller transcript and compacts earlier than the 128k default; a model that advertises nothing behaves exactly as before.
Risk areas:
Tests added or updated:
ironclaw_loop_contractscontext_budget/tests.rs(derivation: none/zero → default, large window keeps flat reserve, small window clamps reserve, today's constant is reduced by the margin) andrun_context/tests.rs(field default, builder, wire round trip).ironclaw_agent_loopcompaction.rs/active_task_compaction.rs(run-context budget overrides the strategy default; absent budget falls back) plus the four digest self-consistency tests.ironclaw_loop_hostmodel_gateway.rs(gateway reports the provider window / none / none on route override) andtests/thread_loop_host_contract.rs::thread_resolving_gateway_applies_its_prompt_context_budget_to_message_selection.ironclaw_turn_runnerloop_driver_host/context_budget_tests.rs(resolved budget reaches the run context; nothing advertised leaves itNone;derived_budget_sizes_the_request_the_host_sends_to_the_gateway).ironclaw_llmtoken_refreshing.rs::model_metadata_does_not_refresh_the_token.tests/integration/context_budget.rs(small_advertised_window_shrinks_what_the_model_receives,unadvertised_window_keeps_the_compiled_in_ceiling) through the realLlmProviderModelGatewayandironclaw_llmdecorator chain; harness gainsadvertised_context_window(tokens)andcaptured_request_message_count(index). Coverage row added totests/AGENTS.md.What the tests prove: every link of the wiring chain is mutation-verified — each new test was shown failing with its link removed (gateway override returning
None→ integration13 vs 13; wrapper builder call removed → all 5 seeded messages forwarded; driver-host derivation replaced by the config default → all 5 forwarded; token refresh restored → 1 connection attempt on the counting endpoint). The integration test deliberately does not separate compaction from request sizing (they sharevisible_transcript_tokens, so compaction always fires first); that separation is what the two crate-tier tests above pin.Commands run: see Validation. All with
RUST_MIN_STACK=67108864(what CI andscripts/ci/quality_gate.shset; without it ~46 full-runtime test binaries overflow libtest's default stack on this machine).Security Impact
None. The advertised window is advisory: a provider that cannot report one, or reports one for a different model than the route serves, leaves the run on the compiled-in budget (
// silent-okmarked). No new network calls — one existing unnecessary network call (token refresh insidemodel_metadata) is removed.Reborn Trust-Boundary Checklist
PromptContextTokenBudgetand the newOptionfield are plain DTOs constructed by the turn runner.ComponentDigestBLAKE3 replay fingerprints changed content, not purpose; self-consistency tests pin them.HostManagedModelGateway; all implementors inheritNone. Command:rg -n "impl.*HostManagedModelGateway for" crates tests— onlyLlmProviderModelGatewayoverrides.serde(default)field:resolved_context_budget: Option<_>with#[serde(default, skip_serializing_if = "Option::is_none")]— missing meansNonemeans today's default (fails to the conservative pre-existing behavior). Round-trip test inrun_context/tests.rs.saturating_mul/ integer division in the derivation;saturating_subinvisible_transcript_tokens. Reserve clamped so visible transcript is never zero for a non-zero window.Database Impact
None.
resolved_context_budgetis per-run, in-memory only — deliberately not persisted (design note §5).Blast Radius
ironclaw_loop_contracts(type + run-context field),ironclaw_agent_loop(compaction reads the run-context budget; replay digests),ironclaw_loop_host(gateway method + wrapper threading),ironclaw_turn_runner(per-run resolution in host build),ironclaw_llm(TokenRefreshingProvider::model_metadatano longer refreshes). Behavior changes only for runs whose provider populatescontext_length(today: Gemini's static table, and any provider later wired). Everything else is on the identical default, pinned by theNonetests at each tier.What could break: a provider that advertises a window smaller than the transcript the product expects (e.g. a stale table entry) would compact earlier than before; the 90% margin and reserve clamp bound how aggressive that gets, and the route-identity check prevents borrowing the wrong model's window.
Rollback Plan
Revert the branch (16 commits, no schema, no persisted state). A partial rollback is also safe: reverting only
b963204bc(turn-runner resolution) returns every run to the compiled-in default while leaving the plumbing inert.Review Follow-Through
ponytail:inloop_driver_host.rsrecords the one deliberate shortcut: the window probe awaits on the host-build critical path, justified by the new CONTRACT rule thatmodel_metadata()is I/O-free; if that rule is ever relaxed, promote it to atokio::spawnbesideuser_profile_fetch.context_lengthfor more providers (static table + free catalog fields), which is what makes this budget bite for them.Review track: B (feature; touches loop runtime behavior but no security, DB, or CI surface)
🤖 Generated with Claude Code