feat(llm): explicit Anthropic cache_control breakpoints on both transports - #6997
Conversation
…ports Closes #6984 (P0 of the pi-harness adoption program, docs/research/ pi-agent-deep-dive.md §7.3). The rig transport previously relied solely on Anthropic automatic caching via a top-level cache_control field, and the OAuth transport emitted no cache markers at all. Now both place explicit breakpoints so the tool/system prefix and the growing conversation cache independently: - OAuth transport: apply_cache_breakpoints marks the system prompt block, the last tool definition, and the last content block of the last message, all carrying the retention TTL. Retention None keeps the legacy wire shape (plain-string system, no markers). - rig transport: build_rig_request marks the last tool by moving it into rig's raw additional_params.tools (appended after typed tools, order preserved, Anthropic-native input_schema shape) and keeps the top-level automatic marker; Short retention additionally enables rig's typed system/last-message breakpoints. Long must not enable the typed breakpoints: rig markers cannot carry a TTL and a 5m block marker beside a 1h automatic marker is an API error. All markers in a request share one TTL, satisfying Anthropic's longer-TTL-first ordering rule. Unsupported models downgrade to None via supports_prompt_cache on both paths. Wire shape is pinned by loopback capture-server tests in both files (three per transport: short, long, none), plus build_rig_request seam tests for the tool move. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🚅 Deployed to the ironclaw-pr-6997 environment in ironclaw-ci-preview
|
github-actions
Bot
added
scope: docs
📝 WalkthroughSummary by CodeRabbit
WalkthroughAnthropic OAuth and rig requests now support configurable prompt caching. Retention markers apply to system prompts, tools, and final message blocks. Unsupported models downgrade to uncached requests. Dedicated tests cover wire formats, conversions, errors, tokens, and streaming. ChangesAnthropic prompt-cache retention
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant RigAdapter
participant OAuthTransport
participant AnthropicAPI
Provider->>RigAdapter: effective cache retention
RigAdapter->>RigAdapter: build cache markers and tool parameters
Provider->>OAuthTransport: cache retention
OAuthTransport->>OAuthTransport: apply system, tool, and message markers
RigAdapter->>AnthropicAPI: serialized rig request
OAuthTransport->>AnthropicAPI: serialized OAuth request
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_llm/src/anthropic_oauth.rs`:
- Around line 706-712: Update the system-prompt conversion around request.system
to match through a mutable borrow, such as request.system.as_mut(), instead of
calling take(). Convert only the AnthropicSystem::Text variant into cached
blocks and leave AnthropicSystem::Blocks unchanged, removing any unreachable
handling that is no longer needed.
In `@crates/ironclaw_llm/src/lib.rs`:
- Around line 505-514: Centralize model-specific retention resolution by adding
CacheRetention::for_model(&self, model: &str) -> CacheRetention, reusing the
existing prompt-cache capability check. In crates/ironclaw_llm/src/lib.rs lines
505-514 and crates/ironclaw_llm/src/anthropic_oauth.rs lines 133-139, replace
the duplicated conditional logic with this resolver before configuring prompt
caching; update RigAdapter::with_cache_retention to use the same method while
preserving its warning behavior.
In `@crates/ironclaw_llm/src/rig_adapter.rs`:
- Line 1086: Above the #[allow(clippy::too_many_arguments)] associated with
build_rig_request, add an immediately preceding // arch-exempt: comment naming
the missing request-shape aggregation and referencing the applicable plan
number. Keep the existing attribute and function behavior unchanged.
🪄 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: e700c3a2-d9b7-4bdc-b94c-a7943aa3fa67
📒 Files selected for processing (5)
crates/ironclaw_llm/CLAUDE.mdcrates/ironclaw_llm/src/anthropic_oauth.rscrates/ironclaw_llm/src/config.rscrates/ironclaw_llm/src/lib.rscrates/ironclaw_llm/src/rig_adapter.rs
| /// only: rig's typed markers are always plain 5m ephemeral, and a 5m block | ||
| /// marker combined with a 1h automatic marker is rejected by the API (TTL | ||
| /// conflict on the last block), so `Long` relies on the automatic marker for | ||
| /// the conversation tail. | ||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required // arch-exempt: line above this #[allow].
Repo policy requires every #[allow(clippy::too_many_arguments)] to carry an immediately preceding exemption comment naming the missing aggregation and a plan link. This call site is a good candidate for a small request-shape struct, because build_rig_request now takes seven inputs including cache_retention.
♻️ Proposed annotation
+// arch-exempt: too_many_args, no RigRequestSpec aggregation for preamble/history/tools/tool_choice/sampling/cache_retention, plan `#6984`
#[allow(clippy::too_many_arguments)]
fn build_rig_request(Based on learnings, #[allow(clippy::too_many_arguments)] requires an architecture-exemption comment explaining the justification, and as per coding guidelines "Do not add #[allow(clippy::too_many_arguments)] without an immediately preceding // arch-exempt: too_many_args, <specific missing aggregation>, plan #NNNN`` comment."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[allow(clippy::too_many_arguments)] | |
| // arch-exempt: too_many_args, no RigRequestSpec aggregation for preamble/history/tools/tool_choice/sampling/cache_retention, plan `#6984` | |
| #[allow(clippy::too_many_arguments)] |
🤖 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_llm/src/rig_adapter.rs` at line 1086, Above the
#[allow(clippy::too_many_arguments)] associated with build_rig_request, add an
immediately preceding // arch-exempt: comment naming the missing request-shape
aggregation and referencing the applicable plan number. Keep the existing
attribute and function behavior unchanged.
Sources: Coding guidelines, Learnings
There was a problem hiding this comment.
Pull request overview
This PR updates the ironclaw_llm Anthropic integrations to emit explicit cache_control prompt-caching breakpoints on both transports (rig/API-key and OAuth), aligning with the pi-harness caching layout and ensuring subscription-auth users also benefit from prompt caching.
Changes:
- Add explicit Anthropic cache breakpoints: system prompt, last tool definition, and the last content block of the last message.
- Extend the rig adapter to move the last tool into
additional_params.tools(Anthropic-native shape) so it can carrycache_control, while retaining the request-level automatic caching marker. - Add capture-server wire-shape tests for short/long/none retention and document the caching layout in
crates/ironclaw_llm/CLAUDE.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ironclaw_llm/src/rig_adapter.rs | Emits cache markers via request root + moved last-tool entry; adds seam + wire-capture tests. |
| crates/ironclaw_llm/src/lib.rs | Aligns rig model prompt_caching with retention and pre-downgrades unsupported models. |
| crates/ironclaw_llm/src/config.rs | Adds CacheRetention::cache_control_json() helper for consistent marker emission. |
| crates/ironclaw_llm/src/anthropic_oauth.rs | Adds apply_cache_breakpoints and updates request encoding to support per-block cache_control; adds wire-capture tests. |
| crates/ironclaw_llm/CLAUDE.md | Documents Anthropic prompt caching breakpoint layout and TTL constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let cache_retention = if config.cache_retention != CacheRetention::None | ||
| && !rig_adapter::supports_prompt_cache(&config.model) | ||
| { | ||
| CacheRetention::None | ||
| } else { | ||
| config.cache_retention | ||
| }; |
| let cache_retention = if config.cache_retention != crate::config::CacheRetention::None | ||
| && !crate::rig_adapter::supports_prompt_cache(&config.model) | ||
| { | ||
| crate::config::CacheRetention::None | ||
| } else { | ||
| config.cache_retention | ||
| }; |
🔎 Review · PR #6997
1 actionable findings →Reviewed the complete trusted base-to-head comparison. The breakpoint layouts are well covered, but cache eligibility is determined from the configured model rather than the effective request model, allowing unsupported model overrides to receive invalid cache markers. Automatic · PR opened · attempt 1 of 3 · completed in 1m 30s Run details
|
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Review complete · PR #6997
Reviewed the complete trusted base-to-head comparison. The breakpoint layouts are well covered, but cache eligibility is determined from the configured model rather than the effective request model, allowing unsupported model overrides to receive invalid cache markers.
Findings
- 🟠 Medium · Revalidate cache support after selecting the effective model —
crates/ironclaw_llm/src/anthropic_oauth.rs:352
Details are attached to the relevant diff.
Validation and technical details
- Inspected the complete diff from refs/ironloop/base (a50ad06) to refs/ironloop/head (1227a74), including all five changed files and surrounding provider/request code.
- Traced cache retention through provider construction, OAuth complete/complete_with_tools/set_model, rig complete and streaming variants, request model overrides, tool conversion, and additional-parameter merging.
git diff --check refs/ironloop/base..refs/ironloop/headcompleted without errors.- Focused Rust tests could not be executed because
cargois unavailable in the review environment (/bin/bash: cargo: command not found). - Base:
main - Head:
feat/anthropic-cache-breakpointsat1227a74 - Run:
9b17c485-107a-404f-81e8-6608e205abfa
| max_tokens, | ||
| temperature: req.temperature, | ||
| tools: None, | ||
| tool_choice: None, | ||
| }; | ||
|
|
||
| apply_cache_breakpoints(&mut request, self.cache_retention); |
There was a problem hiding this comment.
🟠 Medium · Revalidate cache support after selecting the effective model
cache_retention is fixed when the provider is constructed, but this method first selects a per-request model override (and OAuth also supports changing active_model through set_model) and then applies the fixed retention unconditionally. Thus a provider configured for a cache-capable Claude model can later send cache_control markers to an unsupported model such as claude-2, despite the documented downgrade, causing Anthropic to reject the request. The rig path has the same issue because build_rig_request emits markers before the typed model override is injected. Determine effective retention from the selected model for every request, and add override/set-model wire tests for unsupported models.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.95% — 323586 / 376495 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 (19 entry/entries excluded from the accounting above)
|
The Reborn integration-tier changed-coverage gate flagged uncovered branches in #6997: the unsupported-model retention downgrade on both transports, the Image/ToolUse marker arms, the empty-text guard, and the create_anthropic_from_registry wiring. - Extract the duplicated downgrade logic into rig_adapter::effective_cache_retention, shared by lib.rs and the OAuth constructor, with a direct unit test over all branches. - Wire test: an unsupported model (claude-2.1) with Short retention keeps the legacy no-caching shape end-to-end. - Direct apply_cache_breakpoints tests: tool_use tail without system/tools, image tail, empty-text tail, empty transcript. - Construction test driving create_anthropic_from_registry across all retention modes including the downgrade path. - Move the OAuth transport test suite to src/anthropic_oauth/tests.rs (same idiom as rig_adapter/tests/) to stay inside the file-size budget, with the matching coverage exemption entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
crates/ironclaw_llm/src/anthropic_oauth.rs:434
- Same as
complete(): cache breakpoint stamping should be downgraded based on the actualrequest.modelused aftertake_model_override(), otherwise a model override to a non-caching model can still emitcache_controlmarkers and fail the request.
apply_cache_breakpoints(&mut request, self.cache_retention);
crates/ironclaw_llm/src/anthropic_oauth.rs:347
apply_cache_breakpointsis always driven byself.cache_retention, which was computed from the configured model at construction. If a per-request model override selects an unsupported model (e.g. claude-2), we can still emit cache markers and risk an API 400. Consider downgrading retention at request time based on the actualrequest.modelbeing sent.
This issue also appears on line 434 of the same file.
apply_cache_breakpoints(&mut request, self.cache_retention);
The changed-coverage gate flagged three remnants: the Text arm of set_cache_control, the empty-blocks tail, and the non-Text side of the system take-and-rebuild — which was also a latent drop: a system value already in block form was taken and never restored. Restore it untouched and pin all three paths with direct tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_llm/src/anthropic_oauth.rs (1)
732-735: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not mark an empty trailing text block.
Line 733 applies
cache_controltoText { text: "" }. This contradicts Lines 722-724 and sends a request that Anthropic rejects when a multimodal message ends with an empty text block. Skip empty trailing text blocks. Add a captured-wire regression throughcomplete()orcomplete_with_tools().As per path instructions, “Test through the caller: when a helper gates a side effect, require a test driving the real call site.”
🤖 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_llm/src/anthropic_oauth.rs` around lines 732 - 735, Update the AnthropicContent::Blocks handling in the cache-control marker helper to leave a trailing Text block with empty text unmarked, while preserving marking for non-empty trailing blocks. Add a captured-wire regression that drives the real complete() or complete_with_tools() caller and verifies the empty trailing text block is not sent with cache_control.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ironclaw_llm/src/anthropic_oauth.rs`:
- Around line 732-735: Update the AnthropicContent::Blocks handling in the
cache-control marker helper to leave a trailing Text block with empty text
unmarked, while preserving marking for non-empty trailing blocks. Add a
captured-wire regression that drives the real complete() or
complete_with_tools() caller and verifies the empty trailing text block is not
sent with cache_control.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71043475-d160-43e8-812b-1ae24cd6303d
📒 Files selected for processing (2)
crates/ironclaw_llm/src/anthropic_oauth.rscrates/ironclaw_llm/src/anthropic_oauth/tests.rs
The last uncovered branch on the changed-coverage gate: Some(tools) with an empty vec (only constructible directly — complete_with_tools maps empty to None). Extend the empty-cases test to pin the no-op. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
crates/ironclaw_llm/src/anthropic_oauth.rs:434
- Same as
complete(): the resolved model can differ from the construction-time model, but breakpoints are applied usingself.cache_retentionwithout re-validating against the actual model. This can emitcache_controlfor unsupported claude-2-era models whenmodel_override/set_modelis used.
apply_cache_breakpoints(&mut request, self.cache_retention);
crates/ironclaw_llm/src/lib.rs:509
effective_cache_retentiondowngrades unsupported models toNone, but this path no longer emits the warning thatRigAdapter::with_cache_retentionwould have produced (because the adapter now receives the already-downgraded value). That makes a misconfiguredANTHROPIC_CACHE_RETENTIONsilent in production logs.
// Downgrade retention up front for models without prompt-cache support so
// the rig `prompt_caching` flag below agrees with the adapter's own
// `with_cache_retention` validation.
let cache_retention =
rig_adapter::effective_cache_retention(config.cache_retention, &config.model);
crates/ironclaw_llm/src/anthropic_oauth.rs:347
apply_cache_breakpointsis driven byself.cache_retentioncomputed at construction, but the request’smodelcan differ viatake_model_override()/set_model(). If the active/override model is a claude-2-era model, this can still emitcache_controlmarkers for an unsupported model (likely 400). Downgrade retention against the resolved model before applying breakpoints.
This issue also appears on line 434 of the same file.
apply_cache_breakpoints(&mut request, self.cache_retention);
|
Reviewed alongside #7001, #6992 and #5981. One thing I'd treat as blocking, then some test-coverage notes. I've also included one concern I chased down and refuted, so nobody else spends time on it. Blocking: retention is frozen at construction, but the model is resolved per request
let model = req.take_model_override().unwrap_or_else(|| self.active_model_name()); // 329-331
...
apply_cache_breakpoints(&mut request, self.cache_retention); // 347
Two failure modes:
What makes this a regression rather than a pre-existing wart: before this PR the OAuth path emitted no markers at all, so a stale retention was harmless. Now it's load-bearing. Suggest evaluating Should fixEmpty
Test coverage
Refuted — no action neededI suspected the Cross-PR#7001 makes the final message a host block carrying the minute-precision clock. This PR places its third breakpoint on "the last content block of the last message". Landed together, that breakpoint sits on host boilerplate that turns over on a clock rather than on conversation content — worth checking |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add explicit Anthropic prompt-caching breakpoints across OAuth and API-key transports while preserving compatible behavior for retention modes and unsupported models.
Mode: normal — preflight classified this as an XL but contained 7-file handwritten change, with no generated/vendor or stacked-PR routing.
Coverage: diff source github; production 5, tests 1, docs 1, generated/vendor 0, CI 0, config 0; diff_truncated=true. The reviewer prompt carried the mandated 40,000-character diff prefix, while every reviewer had the exact detached head worktree for full-file and sibling-code inspection. The codebase graph was unavailable, so review used crate guidance and targeted live-code search.
Stats: 4 findings (from 6 raw, 4 after dedup) across 3 files. Reviewers run: security, bugs, performance/concurrency, tests, conventions, local patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Security
- Medium Prompt cache is not partitioned by Ironclaw tenant (
crates/ironclaw_llm/src/anthropic_oauth.rs:720-729, confidence 78) — anchor:crates/ironclaw_llm/src/anthropic_oauth.rs:720
The OAuth path now caches the message tail inside the shared Anthropic workspace, but this provider has no tenant namespace. With a shared OAuth credential, an exact-prefix probe can use returned cache-read counts to infer that a guessed sensitive prompt occurred during the TTL. Anthropic documents cache sharing/isolation at workspace granularity, not at the application-tenant boundary.
Bugs
- Medium Revalidate cache retention against the request's actual model (
crates/ironclaw_llm/src/anthropic_oauth.rs:347, confidence 98) — anchor:crates/ironclaw_llm/src/anthropic_oauth.rs:347
Retention support is frozen from the startup model even though both request paths support per-request or mutable active-model selection. Switching model families can therefore emit unsupported markers or leave caching disabled. Also flagged by Approach/Medium.
Tests
- Medium Factory cache-mode wiring is not exercised on the wire (
crates/ironclaw_llm/src/lib.rs:508-519, confidence 100) — anchor:crates/ironclaw_llm/src/lib.rs:519
Wire tests manually reproduce the factory'sprompt_cachingassignment, while the factory test asserts only construction and model name. A regression in the production Short/Long assignment can leave all current wire tests green. Also flagged by Maintainability/Medium.
Local Patterns
- Medium Unsupported cache requests are now silently downgraded (
crates/ironclaw_llm/src/rig_adapter.rs:923-924, confidence 98) — anchor:crates/ironclaw_llm/src/rig_adapter.rs:923
Pre-normalizing retention toNonebypasses the adapter's established construction-time warning, so an explicitly requested cache setting can be ignored silently.
| max_tokens, | ||
| temperature: req.temperature, | ||
| tools: None, | ||
| tool_choice: None, | ||
| }; | ||
|
|
||
| apply_cache_breakpoints(&mut request, self.cache_retention); |
There was a problem hiding this comment.
Medium — Revalidate cache retention against the request's actual model.
cache_retention is frozen from config.model at construction, but complete() and complete_with_tools() resolve the actual model per request via an override or mutable active_model. Switching from a cache-capable model to a Claude 2-era model still emits cache_control markers and can make Anthropic reject the request; switching the other direction silently leaves caching disabled. This contradicts the stated unsupported-model downgrade guarantee.
Fix: Store the configured retention unchanged, then compute effective_cache_retention(self.cache_retention, &model) after resolving the request model in both completion paths before calling apply_cache_breakpoints.
Also flagged by: approach/Medium
| // retention must NOT set this — rig's markers cannot carry a TTL, and a | ||
| // 5m block marker alongside a 1h automatic marker is an API error | ||
| // (TTL conflict on the last block). See issue #6984. | ||
| model.prompt_caching = cache_retention == CacheRetention::Short; |
There was a problem hiding this comment.
Medium — Factory cache-mode wiring is not exercised on the wire.
The wire tests construct the rig model directly and manually set model.prompt_caching, while the only create_anthropic_from_registry test checks construction and model_name(). Removing or inverting the production factory assignment can therefore leave every current cache-layout test green even though Short loses its system/message breakpoints or Long emits conflicting 5m markers beside the 1h marker.
Fix: Add tests::anthropic_registry_factory_emits_retention_specific_breakpoints covering Short, Long, None, and an unsupported model by capturing request JSON from a provider built through create_anthropic_from_registry.
Also flagged by: maintainability/Medium
| last.cache_control = Some(marker.clone()); | ||
| } | ||
|
|
||
| if let Some(last_message) = request.messages.last_mut() { |
There was a problem hiding this comment.
Medium — Prompt cache is not partitioned by Ironclaw tenant.
The OAuth path now caches the last message, including user text and tool results, under the shared Anthropic workspace. Anthropic isolates caches by workspace rather than by this application's tenant, while this provider receives no actor/tenant namespace. In a deployment sharing one OAuth credential, another tenant can submit a guessed identical prefix and use returned cache-read token counts to infer that the sensitive prompt occurred during the cache TTL. OAuth requests previously emitted no cache markers, so this exposure is introduced here.
Fix: Partition cached prefixes with an opaque tenant-scoped namespace or per-tenant provider credentials; until tenant scope reaches this layer, disable message-tail caching for shared multi-tenant OAuth providers.
| retention: CacheRetention, | ||
| model_name: &str, | ||
| ) -> CacheRetention { | ||
| if retention != CacheRetention::None && !supports_prompt_cache(model_name) { |
There was a problem hiding this comment.
Medium — Unsupported cache requests are now silently downgraded.
effective_cache_retention converts an explicitly requested retention to None before either factory reaches with_cache_retention. The established construction-time warning is therefore bypassed on the API-key path, and OAuth also downgrades silently, so a user's cache setting can be ignored without the diagnostic promised by the adapter's method documentation.
Fix: Emit the existing model-scoped warning when effective_cache_retention performs the downgrade, or preserve the requested retention for validation while using only the effective value for marker configuration, ensuring each transport warns exactly once.
Resolve conflicts from the crates/ family reorganization (#7037 etc.): - anthropic_oauth.rs: keep the PR's cache-breakpoint logic alongside main's stream field, convert_anthropic_tools/convert_anthropic_tool_choice helpers, and streaming paths (now typed to AnthropicSystem). - Move the PR's tests to crates/domains/ironclaw_llm/src/anthropic_oauth/ and merge main's streaming tests into the suite. - Register anthropic_oauth/tests.rs in CONTRACT.md sub-owner map and repoint the coverage exemption at the new crate path.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/domains/ironclaw_llm/src/anthropic_oauth.rs (1)
528-539: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply cache breakpoints on both OAuth streaming paths.
Both streaming paths build
AnthropicRequestand send it withoutapply_cache_breakpoints. Configured caching therefore works only for buffered OAuth calls. Streaming requests emit the legacy uncached wire shape.
crates/domains/ironclaw_llm/src/anthropic_oauth.rs#L528-L539: apply the effective retention after request construction and beforesend_streaming_request.crates/domains/ironclaw_llm/src/anthropic_oauth.rs#L633-L648: apply the effective retention after tool request construction and beforesend_streaming_request.Add streaming caller-level wire tests for
Short,Long, andNone.As per path instructions, “Test through the caller” requires tests through the streaming provider entry points.
🤖 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/domains/ironclaw_llm/src/anthropic_oauth.rs` around lines 528 - 539, Update both streaming request paths in crates/domains/ironclaw_llm/src/anthropic_oauth.rs:528-539 and :633-648 to apply the effective cache retention to each constructed AnthropicRequest before send_streaming_request. Add caller-level wire tests through the streaming provider entry points covering Short, Long, and None retention, ensuring each path emits the configured cache breakpoints.Source: Path instructions
🤖 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/domains/ironclaw_llm/src/anthropic_oauth.rs`:
- Around line 1137-1152: Update the trailing-block handling in convert_messages
so AnthropicContentBlock::ToolResult receives no cache marker when its content
is empty, while preserving existing marking behavior for non-empty blocks and
other content types. Add a regression test covering a request whose final
message is an empty tool result.
---
Outside diff comments:
In `@crates/domains/ironclaw_llm/src/anthropic_oauth.rs`:
- Around line 528-539: Update both streaming request paths in
crates/domains/ironclaw_llm/src/anthropic_oauth.rs:528-539 and :633-648 to apply
the effective cache retention to each constructed AnthropicRequest before
send_streaming_request. Add caller-level wire tests through the streaming
provider entry points covering Short, Long, and None retention, ensuring each
path emits the configured cache breakpoints.
🪄 Autofix
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: dc1f4e97-5169-44e4-a3a1-fa198470f130
📒 Files selected for processing (7)
crates/domains/ironclaw_llm/CONTRACT.mdcrates/domains/ironclaw_llm/src/anthropic_oauth.rscrates/domains/ironclaw_llm/src/anthropic_oauth/tests.rscrates/domains/ironclaw_llm/src/config.rscrates/domains/ironclaw_llm/src/lib.rscrates/domains/ironclaw_llm/src/rig_adapter.rstests/integration/coverage-exemptions.toml
| if let Some(last_message) = request.messages.last_mut() { | ||
| match &mut last_message.content { | ||
| // Empty text blocks cannot carry cache_control (API rejects | ||
| // them), so an empty trailing message keeps the string form. | ||
| AnthropicContent::Text(text) if !text.is_empty() => { | ||
| last_message.content = | ||
| AnthropicContent::Blocks(vec![AnthropicContentBlock::Text { | ||
| text: std::mem::take(text), | ||
| cache_control: Some(marker), | ||
| }]); | ||
| } | ||
| AnthropicContent::Text(_) => {} | ||
| AnthropicContent::Blocks(blocks) => { | ||
| if let Some(last_block) = blocks.last_mut() { | ||
| last_block.set_cache_control(marker); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skip cache markers on empty tool_result blocks.
convert_messages preserves empty tool-result content. This branch then marks that empty tail block. Anthropic rejects cache markers on empty content blocks, so a valid empty tool result can fail the request.
Do not mark an empty AnthropicContentBlock::ToolResult. Add a regression test for an empty final tool result.
🤖 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/domains/ironclaw_llm/src/anthropic_oauth.rs` around lines 1137 - 1152,
Update the trailing-block handling in convert_messages so
AnthropicContentBlock::ToolResult receives no cache marker when its content is
empty, while preserving existing marking behavior for non-empty blocks and other
content types. Add a regression test covering a request whose final message is
an empty tool result.
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Merge conflicts resolved and full CI green (clippy, tests, buckets, coverage). Approving to unblock the merge queue.
…ports (nearai#6997) * feat(llm): explicit Anthropic cache_control breakpoints on both transports Closes nearai#6984 (P0 of the pi-harness adoption program, docs/research/ pi-agent-deep-dive.md §7.3). The rig transport previously relied solely on Anthropic automatic caching via a top-level cache_control field, and the OAuth transport emitted no cache markers at all. Now both place explicit breakpoints so the tool/system prefix and the growing conversation cache independently: - OAuth transport: apply_cache_breakpoints marks the system prompt block, the last tool definition, and the last content block of the last message, all carrying the retention TTL. Retention None keeps the legacy wire shape (plain-string system, no markers). - rig transport: build_rig_request marks the last tool by moving it into rig's raw additional_params.tools (appended after typed tools, order preserved, Anthropic-native input_schema shape) and keeps the top-level automatic marker; Short retention additionally enables rig's typed system/last-message breakpoints. Long must not enable the typed breakpoints: rig markers cannot carry a TTL and a 5m block marker beside a 1h automatic marker is an API error. All markers in a request share one TTL, satisfying Anthropic's longer-TTL-first ordering rule. Unsupported models downgrade to None via supports_prompt_cache on both paths. Wire shape is pinned by loopback capture-server tests in both files (three per transport: short, long, none), plus build_rig_request seam tests for the tool move. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(llm): close changed-line coverage gaps on cache breakpoints The Reborn integration-tier changed-coverage gate flagged uncovered branches in nearai#6997: the unsupported-model retention downgrade on both transports, the Image/ToolUse marker arms, the empty-text guard, and the create_anthropic_from_registry wiring. - Extract the duplicated downgrade logic into rig_adapter::effective_cache_retention, shared by lib.rs and the OAuth constructor, with a direct unit test over all branches. - Wire test: an unsupported model (claude-2.1) with Short retention keeps the legacy no-caching shape end-to-end. - Direct apply_cache_breakpoints tests: tool_use tail without system/tools, image tail, empty-text tail, empty transcript. - Construction test driving create_anthropic_from_registry across all retention modes including the downgrade path. - Move the OAuth transport test suite to src/anthropic_oauth/tests.rs (same idiom as rig_adapter/tests/) to stay inside the file-size budget, with the matching coverage exemption entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(llm): cover remaining cache-breakpoint branch arms The changed-coverage gate flagged three remnants: the Text arm of set_cache_control, the empty-blocks tail, and the non-Text side of the system take-and-rebuild — which was also a latent drop: a system value already in block form was taken and never restored. Restore it untouched and pin all three paths with direct tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(llm): cover the empty-tools arm of apply_cache_breakpoints The last uncovered branch on the changed-coverage gate: Some(tools) with an empty vec (only constructible directly — complete_with_tools maps empty to None). Extend the empty-cases test to pin the no-op. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: serrrfirat <f@nuff.tech>
Closes #6984 — P0 #1 of the pi-harness adoption program (
docs/research/pi-agent-deep-dive.md§7.3, PR #6991).Summary
Both Anthropic transports now place explicit
cache_controlbreakpoints instead of relying on automatic caching alone (rig/API-key path) or emitting nothing (OAuth path — previously no cache markers at all, so subscription-auth users got zero prompt caching).OAuth transport (
anthropic_oauth.rs): newapply_cache_breakpointsmarks the three pi-style boundaries — system prompt block, last tool definition, last content block of the last message (including the tool_result tail common in agent loops) — each carrying the retention TTL. Retentionnonepreserves the legacy wire shape exactly (plain-string system, no markers).API-key transport (
rig_adapter.rs+create_anthropic_from_registry):ToolDefinitioncan't carrycache_control, so the last tool moves into rig's rawadditional_params.tools(appended after typed tools, order preserved, Anthropic-nativeinput_schemashape),shortretention additionally enables rig's typed system/last-message breakpoints (CompletionModel::prompt_caching).longretention deliberately does not enable rig's typed breakpoints: they're always plain 5m ephemeral, and per Anthropic's rules a 5m block marker beside a 1h automatic marker on the last block is a 400 (TTL conflict), and 1h entries must precede 5m entries. All markers within a request therefore share one TTL. Verified against the current prompt-caching docs (request-level + block-level compatibility, 4-breakpoint budget, longer-TTL-first ordering).Unsupported models (claude-2 era) downgrade to
noneon both paths viasupports_prompt_cache.Tests (written first, red → green)
build_rig_requestfor the tool move (marker +input_schemakey + typed-tools untouched when caching is off).ironclaw_llmsuite: 893 passed; crate clippy-D warningsclean.Note on the issue's second acceptance item (sustained cache reads visible in
prompt_cache_activity): cache-read counts come from the live API and can't be asserted hermetically; this PR pins the emitting side. The realized hit rate also depends on the rest of the P0 program (#6985 prefix stability, #6986 tool-array stability, #6987 regression test).Spec
crates/ironclaw_llm/CLAUDE.mdgains an "Anthropic Prompt Caching" section documenting the layout and the TTL-ordering constraint.🤖 Generated with Claude Code