refactor: consolidate and harden Anthropic prompt-cache checkpoint handling#3712
Conversation
…points The SDK's ContentBlockParamUnion.GetCacheControl (and its Beta twin) returns a pointer to the variant's CacheControl field for exactly the block kinds we handled by hand (text, tool_use, tool_result, image, document), so both hand-rolled switches collapse to a single assignment.
…nverters Both the standard and Beta message converters duplicated the same comment and 4-breakpoint budgeting logic; hoist it into messageCacheBreakpoints.
…trol.go Pure move: messageCacheBreakpoints, applyMessageCacheControl, and applyBetaMessageCacheControl relocate unchanged so the prompt-caching breakpoint logic lives in one file.
… the limit The 4-breakpoint budget was an implicit cross-layer contract: the session marks up to two system messages and the provider spends the rest, but nothing enforced the global invariant \u2014 a third upstream mark (e.g. from a hook-rewritten message list) would produce a request Anthropic rejects outright (the failure mode of docker#2236). Own the whole budget in cache_control.go: document the ledger and cap honored system marks at maxSystemCacheBreakpoints, dropping the excess instead of forwarding it. Behavior is unchanged for compliant input.
Cost and CacheControl clearing lived in two mirrored copies (the compactor's CompactionInput wrapper and the compaction benchmark), each a forget-to-clear hazard for the next consumer \u2014 exactly how where the message copies are made: every consumer now gets clean input by construction, and the compactor and benchmark no longer handle cache control at all.
…control.go Pure move of extractSystemBlocks and extractBetaSystemBlocks so the entire checkpoint mechanism (budget ledger, system-block marking with cap, message-tail breakpoints) lives in one file, and document the CacheControl contract on chat.Message: request-assembly state, set by the session on assembled copies, JSON-serialized only because marks must survive the before_llm_call hook rewrite round-trip. Note: deleting the field in favor of out-of-band boundary indices was considered and rejected \u2014 hooks.Input.Messages / updated_messages round-trip assembled messages through external hook processes as JSON, and per-message marks are the only representation that survives hook-driven message-list rewrites (index-based boundaries would be silently misplaced by an insert/delete).
… accessor contract Two hardening follow-ups to the breakpoint-budget work: - log (debug) when an over-budget system mark is dropped, so upstream misbehavior is diagnosable instead of silent - pin GetCacheControl coverage for every block kind the converters produce, on both the standard and Beta unions, so an SDK bump that changes accessor coverage fails tests instead of silently changing which requests get cached
…marks The contract on chat.Message.CacheControl said stored transcript items never carry marks, but nothing enforced it \u2014 an API client echoing a mark back could persist it and have it resurface on every future prompt assembly. Strip the flag at AddMessage ingestion (alongside the existing capToolResultContent normalization) and reword the CompactionInput clearing rationale in session terms rather than compactor internals.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This refactor cleanly consolidates the Anthropic prompt-cache checkpoint handling. The budget arithmetic is correct (tool + system + message ≤ 4 in all combinations), the AddMessage ingestion strip and CompactionInput clearing are correctly implemented, and the GetCacheControl() accessor swap is sound (verified by the SDK coverage test that pins all block kinds). One low-confidence edge case in extractSystemBlocks is noted inline.
| if len(msg.MultiContent) > 0 { | ||
| for _, part := range msg.MultiContent { | ||
| if part.Type == chat.MessagePartTypeText { | ||
| if txt := strings.TrimSpace(part.Text); txt != "" { | ||
| systemBlocks = append(systemBlocks, anthropic.TextBlockParam{Text: txt}) | ||
| } | ||
| } | ||
| } | ||
| } else if txt := strings.TrimSpace(msg.Content); txt != "" { | ||
| // Trim system-message content: YAML literal blocks (instruction: |) always | ||
| // append a trailing newline, and we don't want that in API payloads. | ||
| systemBlocks = append(systemBlocks, anthropic.TextBlockParam{ | ||
| Text: txt, | ||
| }) | ||
| } | ||
|
|
||
| if msg.CacheControl && len(systemBlocks) > 0 { | ||
| marked = markSystemBlockCacheControl(&systemBlocks[len(systemBlocks)-1], marked) |
There was a problem hiding this comment.
[low] extractSystemBlocks: CacheControl=true system message with all-whitespace MultiContent silently mismarks the previous message's last block
When a system message has CacheControl=true and MultiContent composed entirely of whitespace-only text parts, it contributes zero blocks to systemBlocks. The guard len(systemBlocks) > 0 is satisfied by earlier messages, so markSystemBlockCacheControl fires on the previous message's last block instead of the intended boundary — silently attributing the cache checkpoint to the wrong location.
// Problematic path: msg contributes no blocks but still triggers mark
if msg.CacheControl && len(systemBlocks) > 0 {
marked = markSystemBlockCacheControl(&systemBlocks[len(systemBlocks)-1], marked)
}A minimal guard that checks whether this message contributed any blocks would prevent the misattribution:
| if len(msg.MultiContent) > 0 { | |
| for _, part := range msg.MultiContent { | |
| if part.Type == chat.MessagePartTypeText { | |
| if txt := strings.TrimSpace(part.Text); txt != "" { | |
| systemBlocks = append(systemBlocks, anthropic.TextBlockParam{Text: txt}) | |
| } | |
| } | |
| } | |
| } else if txt := strings.TrimSpace(msg.Content); txt != "" { | |
| // Trim system-message content: YAML literal blocks (instruction: |) always | |
| // append a trailing newline, and we don't want that in API payloads. | |
| systemBlocks = append(systemBlocks, anthropic.TextBlockParam{ | |
| Text: txt, | |
| }) | |
| } | |
| if msg.CacheControl && len(systemBlocks) > 0 { | |
| marked = markSystemBlockCacheControl(&systemBlocks[len(systemBlocks)-1], marked) | |
| blocksBeforeThisMsg := len(systemBlocks) | |
| if len(msg.MultiContent) > 0 { | |
| for _, part := range msg.MultiContent { | |
| if part.Type == chat.MessagePartTypeText { | |
| if txt := strings.TrimSpace(part.Text); txt != "" { | |
| systemBlocks = append(systemBlocks, anthropic.TextBlockParam{Text: txt}) | |
| } | |
| } | |
| } | |
| } else if txt := strings.TrimSpace(msg.Content); txt != "" { | |
| // Trim system-message content: YAML literal blocks (instruction: |) always | |
| // append a trailing newline, and we don't want that in API payloads. | |
| systemBlocks = append(systemBlocks, anthropic.TextBlockParam{ | |
| Text: txt, | |
| }) | |
| } | |
| if msg.CacheControl && len(systemBlocks) > blocksBeforeThisMsg { | |
| marked = markSystemBlockCacheControl(&systemBlocks[len(systemBlocks)-1], marked) | |
| } |
In practice this combination (CacheControl=true + fully whitespace MultiContent) is unlikely because such a system message carries no useful content. The checkpoint would still be consumed (burning one of the 2 system budget slots) on a block that has nothing to do with the intended message boundary.
| Confidence | Score |
|---|---|
| 🟡 moderate | 62/100 |
chat.Message.CacheControl(Anthropic prompt-cache checkpoints) was handled in seven places with implicit cross-layer contracts, which had already produced two production bugs: markers surviving compaction and exceeding Anthropic's 4-breakpoint limit, and a panic inextractSystemBlockson empty flagged messages. This PR consolidates the handling to three logical owners and makes the known failure modes structurally impossible.On the deduplication side, the two hand-rolled 5-case cache-control switches are replaced with the SDK's
GetCacheControlunion accessor (verified to cover every block kind the converters produce, on both standard and Beta unions — this test already caught a regression whenanthropic-sdk-gov1.57→v1.58 landed on main mid-branch). The shared message-breakpoint budget helper is extracted, and the entire mechanism — budget ledger, system-block marking, message-tail breakpoints, system-block extraction — is consolidated intopkg/model/provider/anthropic/cache_control.go.On the robustness side, system cache breakpoints are now capped so a request can never exceed Anthropic's 4-breakpoint limit regardless of upstream marks; excess marks are dropped and debug-logged, and the worst case is pinned by test.
session.CompactionInputclearsCost/CacheControlon its own copies, removing the compactor's mirrored clearing loops.session.AddMessagestripsCacheControlat ingestion, enforcing the documented contract that transcripts never carry marks — guarding against API clients that echo marks back.chat.Message.CacheControlnow documents its full contract, including why the field must stay JSON-serialized (marks must survivebefore_llm_callhook rewrite round-trips; out-of-band boundary indices would be silently misplaced by hook message inserts/deletes).Behavior changes only affect inputs that previously caused API rejections or panics. All other paths are byte-identical, confirmed by e2e cassette replay.