Repository navigation
⚡ feat: Single Tail Prompt-Cache Breakpoint - #245
Conversation
Replace the rolling "last two user messages" prompt-cache strategy with a single breakpoint anchored on the conversation tail, mirroring the approach used by Claude Code. Anthropic/OpenRouter now place exactly one ephemeral cache_control marker on the last cacheable block of the final non-synthetic message; Bedrock places a single cachePoint via the new addBedrockTailCacheControl. Because the marker always rides the true tail, the whole prefix is written once and read back as history grows append-only, instead of re-writing large spans every step. - Add addTailCacheControl / addBedrockTailCacheControl (single tail marker), skipping thinking blocks and synthetic skill/meta messages as anchors and stripping all stale markers in one pass. - Wire Graph (Anthropic, OpenRouter, Bedrock), AgentContext system-runnable body path, and summarization to the tail strategy by default. - Keep legacy addCacheControl / addBedrockCacheControl exported for compatibility; update affected tests and add cache.tail.test.ts.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b1eaa2bd5
ℹ️ 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".
| const canPlaceCachePoint = | ||
| !cachePointPlaced && | ||
| !isEmptyString && | ||
| !isSyntheticMetaMessage(originalMessage) && | ||
| (typeof content === 'string' || hasArrayContent); |
There was a problem hiding this comment.
When the latest turn is a ToolMessage, this predicate is true, so a string tool result is rewritten to [{ text }, { cachePoint }]. convertToolMessageToConverseMessage later places every one of those blocks inside toolResult.content (src/llm/bedrock/utils/message_inputs.ts:821-842), but Bedrock only allows cachePoint as a top-level message content block, not inside tool result content. Any Bedrock tool-follow-up with promptCache: true will therefore send an invalid Converse request instead of reaching the model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed real, and fixed in 6ef0166 — thanks. One correction to the mechanism: Bedrock does not reject the request. It silently accepts it and drops the breakpoint (verified live against Converse, us-east-1, claude-sonnet-4-5: tool-result tail → turn1 cache_creation=0, turn2 cache_read=0).
Rather than excluding ToolMessages from the tail (which would leave the tool output — often the largest block — uncached), the fix hoists the cachePoint out of toolResult.content to a message-level sibling after the toolResult block in convertToolMessageToConverseMessage. Live after the fix: turn1 cache_creation=9437, turn2 cache_read=9437 — append-only caching on the tool tail, matching Anthropic-direct.
| const canPlaceMarker = | ||
| !markerPlaced && !isSyntheticMetaMessage(originalMessage); |
There was a problem hiding this comment.
Because this only filters synthetic messages, a trailing ToolMessage can receive the new cache_control marker. For the common string tool result case, that first converts the tool result into an array text block, and _ensureMessageContents then wraps the whole array as the content of a new top-level tool_result block (src/llm/anthropic/utils/message_inputs.ts:182-197). The marker is left on nested tool-result content rather than on the top-level tool_result block, so tool-loop follow-up calls do not create the usable tail breakpoint this change is trying to add.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified live against the Anthropic API — this one does not reproduce. The marker does land on the nested text block inside tool_result.content (as you describe), but Anthropic honors cache_control there: the tail breakpoint on a string tool result is effective.
Live (claude-sonnet-4-5, promptCache: true, single tail marker on a string ToolMessage tail, ~9.4k-token prefix):
- turn1 →
cache_creation=9437,cache_read=0(fresh write) - turn2 →
cache_read=9437,cache_creation=103(prefix read back, append-only)
So tool-loop follow-ups do get a usable breakpoint on Anthropic-direct; no change needed here. (The same nesting on Bedrock Converse is not honored — that's the P1 fix above.)
…point The single tail prompt-cache breakpoint frequently anchors on a tool result, since agent-loop conversations end with a tool turn before the next model call. addBedrockTailCacheControl writes the cachePoint into the tool message content, but the Converse converter wrapped the entire content (cachePoint included) inside toolResult.content. A cachePoint is a message-level ContentBlock, not a ToolResultContentBlock. Bedrock does not reject the nested form — it silently drops the breakpoint (verified live: cache_creation/cache_read both stay 0), so the tail strategy produced ZERO caching for the most common agent-loop shape. Hoist any cachePoint out of toolResult.content to a message-level sibling after the toolResult block — the only position Bedrock honors. Live Bedrock Converse now shows the tool-result tail writing the prefix on turn 1 (cache_creation) and reading it back on turn 2 (cache_read), matching the Anthropic-direct behavior. - Hoist cachePoint(s) in convertToolMessageToConverseMessage. - Add toolResultCachePoint.test.ts (converter hoist + end-to-end). - Add cache.tail.test.ts case for a trailing string tool-result tail.
|
@codex review Pushed a follow-up fix (6ef0166) for a Bedrock-specific bug surfaced while validating the tail strategy against live Bedrock Converse. Bug: The single tail breakpoint frequently anchors on a tool result (agent loops end with a tool turn before the next model call). Live evidence (us-east-1,
Fix: Hoist any Tests: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ef0166902
ℹ️ 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".
| workingContent = [ | ||
| { type: 'text', text: content, cache_control: { type: 'ephemeral' } }, | ||
| ] as unknown as MessageContentComplex[]; |
There was a problem hiding this comment.
When the tail is a normal ToolMessage with string content, this branch turns it into a text block carrying cache_control. The Anthropic serializer then sees a non-string tool message and wraps _formatContent(message) inside a new top-level tool_result block (src/llm/anthropic/utils/message_inputs.ts:182-193), so the marker is nested under tool_result.content instead of being on the tool_result block itself. Anthropic only honors cache breakpoints on top-level content blocks, so agent-loop calls whose tail is a tool result lose the single valid breakpoint and get no tail caching; the marker needs to be created/hoisted onto the outer tool_result during conversion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified live against the Anthropic API — this one does not reproduce, so no change here. The marker does land on the nested text block inside tool_result.content (as described), but Anthropic honors cache_control there.
Live (claude-sonnet-4-5, promptCache: true, single tail marker on a string ToolMessage tail, ~9.4k-token prefix):
- turn1 →
cache_creation=9437,cache_read=0(fresh write) - turn2 →
cache_read=9437,cache_creation=103(prefix read back, append-only)
So tool-loop follow-ups DO get a usable breakpoint on Anthropic-direct. (Bedrock's nested form is not honored — that's the separate converter hoist in 6ef0166.) Tracking this as a false positive; happy to add a hoist defensively if you have a counter-repro on a specific model.
| !agentContext.systemRunnable | ||
| ) { | ||
| finalMessages = addCacheControl<BaseMessage>(finalMessages); | ||
| finalMessages = addTailCacheControl<BaseMessage>(finalMessages); |
There was a problem hiding this comment.
When thinking is enabled and the prompt tail is an older non-thinking AI→Tool chain, this new tail marker is added before the later ensureThinkingBlockInMessages call folds that chain into a [Previous agent context] HumanMessage. That fold uses appendMessageContent, which copies the tool text but not the cache_control/cachePoint metadata, so the only tail breakpoint disappears before the model call; the same ordering affects the Bedrock branch below. Insert the tail marker after thinking normalization, or preserve cache metadata during the fold, so these handoff/history prompts do not regress to zero message caching.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real — fixed in cc0bc1d by moving the provider-specific tail cache insertion (Anthropic, Bedrock, OpenRouter) to run last, after ensureThinkingBlockInMessages and sanitizeOrphanToolBlocks, so the marker anchors on the final message list.
Confirmed by inspecting _convertMessagesToAnthropicPayload output for a historical non-thinking AI→Tool tail: marking before the fold loses the breakpoint (the [Previous agent context] builder copies text but not cache_control), marking after preserves it. tailCacheConversion.test.ts includes a guard asserting the old order drops it. Legacy addCacheControl dodged this because it only marked human messages (never folded).
| if (type == null || THINKING_BLOCK_TYPES.has(type)) { | ||
| return false; |
There was a problem hiding this comment.
For Anthropic/OpenRouter calls that include cross-provider assistant history ending in reasoning_content, reasoning, or think, this predicate treats that block as cacheable because only Anthropic thinking types are excluded. The marker is then placed on a block that _convertMessagesToAnthropicPayload explicitly drops on assistant turns (src/llm/anthropic/utils/message_inputs.ts:745-748), so the request loses its only tail breakpoint instead of marking the preceding text/tool block. Exclude these foreign reasoning types when selecting tailIndex so handoffs from Bedrock/Google keep message caching.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real — fixed in cc0bc1d. isTailCacheableBlock now excludes reasoning_content / reasoning / think (alongside native thinking / redacted_thinking), so on a cross-provider handoff the marker anchors on the preceding surviving text/tool block instead of a foreign reasoning block that _convertMessagesToAnthropicPayload drops.
Verified end-to-end: with a reasoning_content tail, the pre-fix payload had zero cache_control; post-fix the breakpoint lands on the text block and survives conversion (tailCacheConversion.test.ts + parametrized cases in cache.tail.test.ts).
Two edge cases dropped the single tail breakpoint before the model call, silently regressing to zero message caching (legacy marked human messages, which avoided both paths): 1. Foreign reasoning tail (Anthropic/OpenRouter): isTailCacheableBlock only excluded native `thinking`/`redacted_thinking`, so on a cross-provider handoff the marker could anchor on a `reasoning_content`/`reasoning`/ `think` block — which _convertMessagesToAnthropicPayload drops on assistant turns. The only breakpoint vanished. Now exclude foreign reasoning types from tail anchoring so the marker lands on a surviving text/tool block. 2. Thinking-fold ordering: the tail marker was placed before ensureThinkingBlockInMessages, which folds a trailing non-thinking AI→Tool chain into a `[Previous agent context]` HumanMessage whose builder copies text but not cache_control/cachePoint. Move the provider-specific tail cache insertion (Anthropic, Bedrock, OpenRouter) to run LAST — after thinking normalization and orphan sanitization — so it anchors on the final message list. Verified by inspecting the final _convertMessagesToAnthropicPayload output: the breakpoint now survives in both cases (and a guard test asserts the old mark-before-fold order loses it). - Exclude reasoning_content/reasoning/think in isTailCacheableBlock. - Reorder tail cache insertion after ensureThinkingBlock/sanitizeOrphan in Graph. - Add tailCacheConversion.test.ts and foreign-reasoning cases in cache.tail.test.ts.
|
@codex review Addressed the latest round in cc0bc1d:
Regression coverage: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc0bc1ded4
ℹ️ 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".
| const canPlaceMarker = | ||
| !markerPlaced && !isSyntheticMetaMessage(originalMessage); |
There was a problem hiding this comment.
With Claude 4.6+ requests that include an assistant prefill (the last message is assistant), this helper now marks that prefill because canPlaceMarker only filters synthetic messages. The Anthropic wrapper calls stripUnsupportedAssistantPrefill immediately before both streaming and non-streaming requests and removes trailing assistant messages, so the only tail/body cache_control is removed before the API call; the previous last-user strategy still left a surviving breakpoint. Please skip assistant-prefill tails for models that are stripped, or anchor on the previous surviving message.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real — fixed in 6d2cc3e. stripUnsupportedAssistantPrefill (Claude 4.6+) pops a trailing assistant prefill right before the API call; if the only tail breakpoint rode it, message caching was lost. It now re-anchors the breakpoint onto the new tail — but only when one was actually removed (messagesHaveCacheControl(before) && !messagesHaveCacheControl(after)), so caching-off requests stay untouched and a surviving breakpoint isn't double-anchored. Reuses addTailCacheControl so the re-anchor honors the same block exclusions. Covered by stripPrefillCache.test.ts. (Bedrock has no equivalent prefill strip, so this is Anthropic-direct only.)
| const fullMessages = [...messages, new HumanMessage(instruction)]; | ||
| const invokeMessages = | ||
| usePromptCache === true ? addCacheControl(fullMessages) : fullMessages; | ||
| usePromptCache === true ? addTailCacheControl(fullMessages) : fullMessages; |
There was a problem hiding this comment.
When self-summarization has a prior summary, buildSummarizationInstruction appends that previous summary to the final HumanMessage, so this tail instruction changes on every compaction. Switching this path to a single tail marker puts the only breakpoint on that volatile instruction; Anthropic/OpenRouter cannot read the unchanged conversation prefix when the instruction changes, whereas the previous two-user-message strategy still left an earlier breakpoint on the preceding user turn. Please keep an earlier stable marker here or keep the summary instruction outside the sole breakpoint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not reproducible — left as-is. The summarization cache read of the conversation prefix doesn't depend on this call's own breakpoint: Anthropic/OpenRouter read the longest matching previously-cached prefix automatically, regardless of where the new request's breakpoints sit. The conversation prefix is cached during normal agent turns (tail strategy); the summarization call reads it back whether its own marker is on the volatile instruction or on the preceding user turn.
Verified live earlier in this PR: turn 2 read a prefix that turn 1 cached at a different, earlier breakpoint position (cache_read=9437 with the turn-2 marker further along). So moving the marker to the instruction tail doesn't prevent the prefix read. The only difference vs. the old strategy is what the summarization call itself writes (conversation+instruction vs. conversation-only), which is negligible since compactions are infrequent and far apart relative to the 5-min TTL.
| workingContent = [ | ||
| { type: 'text', text: content, cache_control: { type: 'ephemeral' } }, | ||
| ] as unknown as MessageContentComplex[]; |
There was a problem hiding this comment.
When prompt caching is enabled and the tail is a string ToolMessage, this branch converts it to array content before attemptInvoke calls annotateMessagesForLLM; that annotator only adds live reference annotations for ToolMessages whose content is still a string, so toolOutputReferences-enabled runs lose the reference marker on the common tool-result tail. The old user-only marker never rewrote ToolMessages, so preserve string tool content or annotate cached text arrays.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real (functional regression) — fixed in 6d2cc3e. Prompt caching rewrites a string ToolMessage tail into a text-block array (to host the marker), keeping _refKey on additional_kwargs. annotateMessagesForLLM previously applied the live [ref: …] annotation only to string content, so the cached tool-result tail lost its reference marker. It now projects the live ref (and any unresolved warning) onto array tool content as leading text blocks, preserving the original blocks and their cache marker. Added array+live-ref regression cases to annotateMessagesForLLM.test.ts.
| if (type == null || NON_ANCHORABLE_REASONING_TYPES.has(type)) { | ||
| return false; |
There was a problem hiding this comment.
When the tail assistant content contains persisted streaming input_json_delta siblings, this check treats them as cacheable and the later loop can move the only cache_control marker onto that block. _convertMessagesToAnthropicPayload drops input_json_delta blocks before sending, so the request loses its sole marker. Exclude dropped input deltas, or keep the previous tool/text block as the anchor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real — fixed in 6d2cc3e. isTailCacheableBlock now also excludes input_json_delta (the set is renamed NON_ANCHORABLE_BLOCK_TYPES), so the marker can't anchor on a persisted partial-input delta that _convertMessagesToAnthropicPayload drops. Same class as the foreign-reasoning exclusion.
Three more cases where the single tail breakpoint failed to reach the model; all stem from anchoring on a volatile tail that a later stage drops/rewrites. 1. input_json_delta anchor (Anthropic/OpenRouter): persisted partial tool-input deltas are dropped by _convertMessagesToAnthropicPayload (input is restored onto the tool_use block). Anchoring the marker there lost it. Excluded input_json_delta from tail anchoring (joins the reasoning types), renaming the set to NON_ANCHORABLE_BLOCK_TYPES. 2. toolOutputReferences annotation (functional regression): prompt caching rewrites a string ToolMessage tail into a text-block array to host its marker; annotateMessagesForLLM only applied the live `[ref: …]` annotation to STRING tool content, so the common tool-result tail silently lost its reference marker once cached. annotateMessagesForLLM now projects the live ref (and unresolved warning) onto array tool content too. 3. assistant-prefill strip (Claude 4.6+): stripUnsupportedAssistantPrefill pops a trailing assistant prefill right before the API call; if the only tail breakpoint rode it, message caching was lost. It now re-anchors the breakpoint onto the new tail (only when one was actually removed, so caching-off requests stay untouched), reusing addTailCacheControl to honor the same exclusions. Tests: stripPrefillCache.test.ts (re-anchor); array live-ref cases in annotateMessagesForLLM.test.ts; input_json_delta is covered by the NON_ANCHORABLE_BLOCK_TYPES exclusion. tsc + lint clean.
|
@codex review Round 3 addressed in 6d2cc3e:
New/updated tests: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d2cc3ec56
ℹ️ 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".
| workingContent = [ | ||
| { type: 'text', text: content, cache_control: { type: 'ephemeral' } }, | ||
| ] as unknown as MessageContentComplex[]; |
There was a problem hiding this comment.
When the tail is a ToolMessage with string content, this rewrites it to a text-block array carrying cache_control; _ensureMessageContents then treats it as non-string and nests that text block inside a top-level tool_result.content array. Anthropic documents tool results as cacheable at the top-level messages.content block and says sub-content blocks cannot be cached directly (https://platform.claude.com/docs/en/build-with-claude/prompt-caching), so the common agent-loop tail either loses the intended breakpoint or risks provider rejection. Please hoist the marker onto the generated tool_result block instead of the nested text block for tool messages.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in 4e2bbc6 — thanks for the persistence here.
To be precise about the current behavior: I isolated it live (no system prompt, large tool result, so the only cacheable content sits behind the tail tool_result marker; CustomAnthropic adds no caching of its own — control no-marker run = cache_creation 0). The nested marker was in fact honored — cache_creation=10232 written, then cache_read=10232 — so it isn't broken today. But you're right that the docs only document the top-level messages.content block as cacheable, and relying on the nested form is fragile.
So I hoist the cache_control off the inner content onto the generated tool_result block in _ensureMessageContents (mirrors the Bedrock cachePoint hoist). Re-verified live on the documented position: control no-marker = cache_creation 0; hoisted marker = cache_creation=12354 turn 1, cache_read=12354 turn 2. tailCacheConversion.test.ts now asserts the marker is on the tool_result block and not nested.
…lock The single tail breakpoint frequently anchors on a tool result. For a string ToolMessage tail, addTailCacheControl rewrites it to a text-block array carrying cache_control, and _ensureMessageContents nests that block inside tool_result.content. The Anthropic API currently honors that nested marker — verified live with an isolated, system-prompt-free large tool result (control no-marker => cache_creation 0; nested marker => 10232 written then read) — so it is not broken today. But Anthropic documents the top-level messages.content block as the cacheable position and does not document sub-content caching, so relying on the nested form is fragile. Hoist any cache_control off the inner tool-result content onto the generated tool_result block itself (mirrors the Bedrock cachePoint hoist). Live-verified end to end: control no-marker => cache_creation 0; hoisted marker => 12354 written on turn 1, read on turn 2. - Add hoistToolResultCacheControl; apply it in _ensureMessageContents. - tailCacheConversion.test.ts now asserts the marker lands on the tool_result block, not nested.
|
@codex review Round 4 addressed in 4e2bbc6: hoist Anthropic Note on behavior: I isolated the nested form live (no system prompt + large tool result; CustomAnthropic adds no caching itself) — the API actually does honor the nested marker (
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e2bbc62ae
ℹ️ 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".
| // Place the single tail prompt-cache breakpoint LAST, after thinking | ||
| // normalization and orphan sanitization. ensureThinkingBlockInMessages can | ||
| // fold a trailing non-thinking AI→Tool chain into a `[Previous agent | ||
| // context]` HumanMessage whose builder copies text but not cache_control / | ||
| // cachePoint, and sanitizeOrphanToolBlocks can drop the anchored block — so | ||
| // marking earlier would let the only breakpoint vanish before the model | ||
| // call (zero message caching). Anchoring on the final message list keeps | ||
| // the marker on a block that actually ships. |
There was a problem hiding this comment.
Because the prompt-cache marker is now added only after needsOrphanSanitize has already been evaluated, prompt-cached Anthropic/Bedrock sends no longer force finalMessages !== messagesToUse before this guard runs. In runs where agentContext.pruneMessages exists but createPruneMessages returns the full context unchanged, the guard stays false, so orphaned AI/tool pairs from persisted history can reach the provider and fail structural validation; before this move, addCacheControl/addBedrockCacheControl reassigned first and caused the cleanup to run. Include prompt-cache sends in the sanitize condition, then place the tail marker after the cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — real regression from the round-2 reorder. Fixed in 924c6e5.
I compute the prompt-cache strategy up front (anthropicPromptCache / openRouterPromptCache / bedrockPromptCache → willAddTailCache) and add willAddTailCache to the needsOrphanSanitize gate, so cached sends are sanitized BEFORE the marker is applied. This restores the pre-move guarantee (previously the marker reassigned finalMessages ahead of the gate, forcing cleanup). The tail marker still runs last, after the cleanup. The same booleans now drive the insertion branch, so the gate and the insertion can't drift.
Moving the tail cache marker to run after sanitizeOrphanToolBlocks (so the marker survives the thinking fold) had a side effect: the marker no longer reassigns finalMessages before the `needsOrphanSanitize` gate is evaluated. For a prompt-cached Anthropic/Bedrock send whose pruner returned the context unchanged (finalMessages === messagesToUse), the gate went false and orphaned AI/tool pairs from persisted history could reach the provider and fail structural validation — whereas the pre-move code always reassigned first. Compute the prompt-cache strategy up front and add `willAddTailCache` to the sanitize gate, so cached sends are cleaned before the marker is applied (restoring the pre-move guarantee). Collapses the cache-insertion branch to the same up-front booleans.
|
@codex review Round 5 fix in 924c6e5: keep orphan sanitization enabled for prompt-cached sends. The round-2 reorder (mark after cleanup) meant the marker no longer reassigned |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 924c6e514f
ℹ️ 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".
| const anthropicPromptCache = | ||
| agentContext.provider === Providers.ANTHROPIC && | ||
| (agentContext.clientOptions as t.AnthropicClientOptions | undefined) | ||
| ?.promptCache === true && | ||
| !agentContext.systemRunnable; |
There was a problem hiding this comment.
When Anthropic/OpenRouter prompt caching is enabled with a system runnable (the normal path for agents with system instructions), this flag is false because of !agentContext.systemRunnable, so willAddTailCache stays false and the sanitize gate can still skip when the pruner returns messagesToUse unchanged. The system runnable later calls addTailCacheControl on the body, so cached requests in that configuration can still ship orphaned AI/tool pairs from persisted history and fail provider validation—the exact case this gate is intended to prevent. Track “prompt cache enabled for this provider” separately from “Graph will add the marker itself” so system-runnable cached sends also force sanitizeOrphanToolBlocks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 805e1e1 — you're right, the system-runnable path was still uncovered.
I now track two separate facts:
providerPromptCacheEnabled— caching is on for the provider at all (Anthropic/OpenRouter/Bedrock, regardless of systemRunnable). This drives the orphan-sanitize gate, so system-runnable cached sends (where AgentContext adds the body marker) are cleaned too.- the node-adds-the-marker condition (Anthropic/OpenRouter
&& !systemRunnable, or Bedrock) — drives the actual insertion here.
So the gate fires for every cached send, while the marker is still only added here when this node owns it. (OpenRouter is moot for the gate since it isn't anthropicLike, but it's tracked for completeness.)
The previous gate used "this node will add the marker" (which excludes the system-runnable path via !systemRunnable). But when a system runnable owns the system prompt, AgentContext still adds the body cache marker — so those are cached sends that must be orphan-sanitized as well. With prompt caching + system runnable + a pruner that returned the context unchanged, orphaned AI/tool pairs from persisted history could still reach the provider. Track two separate facts: `providerPromptCacheEnabled` (caching is on for the provider at all — drives orphan cleanup, system-runnable included) vs. the node-adds-the-marker condition (Anthropic/OpenRouter minus systemRunnable, or Bedrock — drives the insertion). The sanitize gate now uses the former.
|
@codex review Round 6 fix in 805e1e1: orphan-sanitize system-runnable prompt-cached sends. Split the concern into |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
The P3-1 re-anchor imported addTailCacheControl from @/messages/cache into the
Anthropic converter, closing a cycle:
messages/format.ts -> llm/anthropic/utils/message_inputs.ts
-> messages/cache.ts -> messages/format.ts
which the bundler's circular-dependency check (npm run build:dev) flags.
Replace the cross-module reuse with a small local re-anchor that operates on the
already-converted Anthropic payload. This is also more correct: at that stage
the converter has already dropped foreign-reasoning / input_json_delta blocks,
so only native thinking blocks need excluding, and the post-strip tail is always
a user message. Live-reverified: turn1 cache_creation=6264, turn2 read=6264.
|
@codex review Round 7 fix in 0cb03c9: break the import cycle the P3-1 re-anchor introduced ( |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Add a committed, live benchmark that empirically justifies the single tail breakpoint over the legacy "last two user messages" strategy, plus a doc with representative results. bench-prompt-cache.ts replays three realistic harness shapes (agent tool loop, multi-turn chat, realistic agent) under BOTH strategies over the same conversations in separate cache namespaces, against a real provider, and reports per-call cache token breakdowns. `fresh` (uncached, full-price input) is derived provider-agnostically from total_tokens-output_tokens minus the cache buckets, since Anthropic folds cache tokens into input_tokens while Bedrock reports them separately. Result (live, claude-sonnet-4-5): the tail strategy is cheaper in every scenario on both Anthropic and Bedrock. Legacy reprocesses tens of thousands of full-price tokens in any tool-bearing conversation (its lone user-message marker leaves the growing transcript uncached); tail reduces that to ~0 and reads the prefix back. Effective cost −30..−38% (Anthropic), −9..−15% (Bedrock); even legacy's best case (frequent user messages) ties-or-wins. - src/scripts/bench-prompt-cache.ts (excluded from build/CI; real paid calls) - npm run bench:cache [-- --provider bedrock|anthropic --rounds N --model id] - docs/prompt-cache-benchmark.md
Covers the two transcript-mutating harness behaviors raised in review: - Tool truncation: a non-issue for caching — applied once at tool-exec with a model-fixed (turn-invariant) cap by the already-tested, deterministic truncateToolResultContent, so a truncated result is a stable prefix block. Documented; no separate scenario needed (existing tool-loop already exercises tool results in the cached prefix). - Compaction (summarization): add a post-compaction scenario — a few pre- compaction tool rounds, a head→summary swap (one-time cache miss for any strategy), then continued tool rounds. Confirms the tail strategy re-establishes append-only caching on the new summary-headed prefix. Live result (claude-sonnet-4-5): tail wins 4/4 scenarios on BOTH Anthropic and Bedrock. Post-compaction is among the largest wins (Anthropic effective −41%, read +76%) because after compaction the summary is the only user message, so legacy re-sends all continued tool work uncached (fresh 63k → 42). docs/prompt-cache-benchmark.md updated with the 4-scenario tables and a truncation/compaction section.
1 participant
Summary
Replaces the rolling "last two user messages" prompt-cache strategy with a single breakpoint anchored on the conversation tail, mirroring the approach used by Claude Code. This is now the default for all prompt-cached providers — no flag.
Motivation: on real multi-turn agent traffic the two-marker strategy was writing tens of thousands of cache tokens every turn while reading almost none back (cache
write≫read), because freshly-appended assistant/tool turns fell outside the cached prefix and the prefix kept re-writing. A single tail breakpoint keeps the prefix append-only: the whole conversation is written once and read back as history grows.Changes
addTailCacheControl(Anthropic/OpenRouter): strips all stale markers in one backward pass, then places exactly onecache_control: { type: 'ephemeral' }on the last cacheable block of the final non-synthetic message. Skipsthinking/redacted_thinkingblocks (API rejectscache_controlthere) and synthetic skill/meta messages as anchors.addBedrockTailCacheControl(Bedrock Converse): the counterpart — inserts a single{ cachePoint: { type: 'default' } }after the last non-empty text block of the tail; sanitizes (but never anchors) system messages.Graph(Anthropic, OpenRouter, Bedrock),AgentContextsystem-runnable body path, andsummarizationnow use the tail strategy by default.addCacheControl/addBedrockCacheControlremain exported for backward compatibility.AgentContexttests; addedsrc/messages/cache.tail.test.ts(13 cases).Test plan
cache.tail.test.ts— 13 new cases (single marker on tail, anchors on trailing tool_result, strips stale markers, skips thinking blocks, skips synthetic-meta tail, string tails, immutability, Bedrock variants)cache.test.ts— 63 existing pass (legacy fns unchanged)AgentContext.test.ts— 96 pass (two body-cache assertions updated to single-tail)summarization.test.ts/summarize-prune.test.ts/token-accounting-pipeline.test.ts— passtsc -p tsconfig.build.jsoncleancache_read/cache_creationtoken counts old-vs-new on a real Anthropic/Bedrock multi-turn sessionNotes / follow-ups
ttl: "1h"for interactive sessions.