fix(bedrock): keep the cache breakpoint when inlining a mid-conversation system reminder

[anduril-rvanderzee]

Summary

A mid-conversation role:"system" message loses its cache_control when inlined for Bedrock, so the client's conversation-level cache breakpoint never reaches the provider: 3 cache_control breakpoints in, 2 cachePoints out. With that anchor gone the cacheable prefix is pinned at the system/tools floor and the entire conversation body is re-read uncached on every turn.

role:"system" inside messages is a supported Anthropic API feature (mid-conversation system messages, Opus 4.8+ — SupportsMidConversationSystem returns false for Bedrock, correctly). Claude Code uses it for <system-reminder> turns, so this fires on ordinary Claude Code traffic through Bedrock.

Reproduction

claude-opus-5 on Bedrock via /anthropic/v1/messages. Both arms send identical content and an identical third breakpoint; the only difference is whether the tail breakpoint rides on a role:"user" or role:"system" turn. Each arm is called twice — once to warm the prefix, once to measure.

Tail breakpoint rides on	uncached input_tokens	cache_read_input_tokens	Real hit rate
role:"user" (control)	2	36,115	100.0%
role:"system"	18,116	18,018	49.9%

Roughly half the input goes uncached on an otherwise identical, already-warm prefix.

Wire-level, same pair of calls — both received cache_control × 3:

Arm	cachePoint emitted	Placement
control	3	2 in system + 1 in messages
affected	2	2 in system, none in messages

Also reproduces via /openai/v1/responses with claude-sonnet-5 (100.0% → 66.5%), so it is in the Bedrock converter rather than one inbound translation layer.

The reproduction is self-contained: any Claude-Code-shaped payload with two system breakpoints plus a third on a trailing role:"system" turn shows it, and the arms differ by ~18K cached tokens — far outside noise. Sampling large real requests and inspecting the emitted Converse body separated cleanly on the same marker: affected requests emit 2 cachePoints and end in a role:system item, unaffected ones emit 3.

A representative sequence of three consecutive requests over one growing conversation, seconds apart, same model and key:

#	cache read ÷ prompt tokens	cache write
1	99.9%	small
2	8.0%	0
3	99.4%	small

Request 2 reads 8% of a prefix that was warm one second earlier and writes nothing — not a cold start, not TTL expiry, not a routing change. Its only distinguishing feature is a trailing role:system item carrying the breakpoint.

On the previous behavior

TestSystemReminderDoesNotCarryCachePoint pinned the omission deliberately, reasoning that a breakpoint at the moving conversation tail shifts every turn and defeats prefix caching. I think the premise is right and the conclusion inverted:

1. A breakpoint that advances each turn is how incremental conversation caching is meant to work — it extends the cached prefix by one turn for the cost of one write.
2. Dropping it doesn't fall back to a safe state; it removes the only conversation-level breakpoint, which is the collapse inlineSystemReminders exists to prevent.
3. Measured: preserving it is 100% vs 49.9%.

The rationale would hold if the reminder were appended after the last breakpoint. Here the reminder carries the breakpoint. That test is replaced by TestSystemReminderCarriesCachePoint; happy to revisit if there's a case I'm not seeing.

Changes

• convertBifrostSystemReminderToBedrockUserMessage carries the block's CacheControl through and appends a cachePoint after the text block it terminates (Converse semantics).
• Only the last breakpoint within a single reminder is emitted, so a pathological input can't exceed Bedrock's per-request checkpoint budget.
• Reminders with no cache_control are unchanged — no invented cachePoint.
• The 1h TTL is preserved rather than silently downgraded to 5m.

Tests

Six cases in cache_points_test.go covering the contract: breakpoint preserved, ordering after text, user-role control unchanged, multiple breakpoints collapse to one, absent cache_control stays absent, 1h TTL preserved. Verified to fail against unpatched dev (3 of 6) and pass with the change. Full core/providers/... and core/schemas/... suites pass; gofmt and go vet clean.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Preserved cache-control breakpoints when system reminders appear mid-conversation.
  • Ensured the final cache point is placed after reminder text.
  • Maintained cache-point ordering, counts, user-role behavior, and one-hour expiration settings.

Walkthrough

Changes

Bedrock cache-point preservation

Layer / File(s)	Summary
Preserve reminder cache points core/providers/bedrock/responses.go	Mid-conversation system reminders retain the final text block’s cache-control metadata. The converter emits one Bedrock cache point after the reminder text and preserves its TTL.
Validate cache-point behavior core/providers/bedrock/cache_points_test.go, core/providers/bedrock/bedrock_test.go	Tests verify cache-point count, ordering, roles, limits, absent metadata, TTL values, and the renamed reminder test contract.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• maximhq/bifrost#5811: Modifies Bedrock cache-control propagation and cache-point placement.
• maximhq/bifrost#5824: Modifies cache-control conversion in responses.go.

Suggested reviewers: tejasghatte

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the Bedrock cache breakpoint fix.
Description check	✅ Passed	The description clearly explains the bug, reproduction, implementation, design rationale, tests, and validation results.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

core/providers/bedrock/responses.go (1)

3919-3942: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Preserve the cache point position.

If a cache-controlled text block is followed by an unmarked text block, this code retains the earlier CacheControl but appends its CachePoint after the later block. That changes the cache boundary.

Insert the single retained cache point immediately after the final cache-controlled wrapped text block. Add a regression case with a cache-controlled text block followed by unmarked text.

Proposed fix

 var lastCacheControl *schemas.CacheControl
+lastCachePointIndex := -1
 ...
 if block.Text != nil {
   wrap(*block.Text)
   if block.CacheControl != nil {
     lastCacheControl = block.CacheControl
+    lastCachePointIndex = len(contentBlocks)
   }
 }
 ...
 if lastCacheControl != nil {
-  contentBlocks = append(contentBlocks, BedrockContentBlock{
+  cachePoint := BedrockContentBlock{
     CachePoint: newBedrockCachePoint(lastCacheControl.TTL),
-  })
+  }
+  contentBlocks = append(contentBlocks, BedrockContentBlock{})
+  copy(contentBlocks[lastCachePointIndex+1:], contentBlocks[lastCachePointIndex:])
+  contentBlocks[lastCachePointIndex] = cachePoint
 }

🤖 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 `@core/providers/bedrock/responses.go` around lines 3919 - 3942, Update the
content-block assembly around wrap and lastCacheControl so the single retained
cache point is inserted immediately after the final cache-controlled wrapped
text block, rather than appended after later unmarked text. Preserve the
existing Bedrock checkpoint budget behavior, and add a regression case covering
a cache-controlled text block followed by unmarked text.

🤖 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 `@core/providers/bedrock/responses.go`:
- Around line 3919-3942: Update the content-block assembly around wrap and
lastCacheControl so the single retained cache point is inserted immediately
after the final cache-controlled wrapped text block, rather than appended after
later unmarked text. Preserve the existing Bedrock checkpoint budget behavior,
and add a regression case covering a cache-controlled text block followed by
unmarked text.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e5946f39-2d57-4bbe-9803-8c9f2f938d75

📥 Commits

Reviewing files that changed from the base of the PR and between 9fbbacf and 04dd423.

📒 Files selected for processing (3)

• core/providers/bedrock/bedrock_test.go
• core/providers/bedrock/cache_points_test.go
• core/providers/bedrock/responses.go

[akshaydeo]

its covered in broader change #5931