fix(ai): retain Anthropic cache checkpoints across tool loops - #1075
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32ee10114a
ℹ️ 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".
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-1 review of commit 5437b99d432965f3b7ad92fec5243ea3101fbe6c. This is not mergeable or releasable as submitted.
B1. The PR is currently CONFLICTING / mergeable=false against main. gh pr checks exposes only the neutral Cubic reviewer and GitGuardian; the required Check and test and Changelog gate checks are absent, so the author’s local GREEN claims are not CI evidence for this head. Rebase onto current main, resolve the conflict, and rerun both required checks before this can be reconsidered.
B2. Open PR #1126 is a semantic collision, not a harmless textual overlap: it changes this same convertMessages() region to coalesce a toolResult followed by user text into one user message. If that change is applied without deliberately preserving this PR’s final checkpoint pass, the cache marker can be dropped; if it is applied on top without a combined regression, the marker moves from the tool-result tail to the appended text block. Reconcile both designs on current main and add a test for the actual interrupted shape [assistant(tool_use), toolResult, user text], asserting the exact wire block order, marker locations, and a maximum of four markers.
B3. The new pass is unconditional: whenever the final serialized message is cacheable, it marks the previous cacheable user message even when there was no tool loop at all. A normal long history such as user -> assistant -> user now gets two dynamic breakpoints where the old implementation got one. On a cold cache this can create two cache writes (Anthropic cache writes are premium-priced), and the implementation has no check that the earlier checkpoint was actually created or that this request is a tool-loop continuation. Constrain the rolling behavior to the intended checkpoint shape, or provide a measured cost/behavior justification and regression coverage for ordinary multi-turn histories.
B4. The added tests do not cover the merge-conflict shape from #1126, empty history, a compaction/truncation boundary, or a long non-tool conversation, so they cannot establish that the four-breakpoint budget and cache cost remain correct outside the hand-built three-loop fixture. The test suite also only runs in the author’s stated environment; this checkout has no installed Vitest binary, and the required remote checks are not present.
The release CHANGELOG and packages/ai/src/changes.md entries are present, and the local changelog gate passes against the stale PR base; that does not clear the conflict, missing CI, collision, or cost issues above.
|
Addressed review 5058011465 in pushed commit |
|
WORKING: review-1075-r2 - inspecting round-1 blockers, coalescing interaction, cache budget, and regression coverage. |
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-2 review of c5d1f9c.
B1 - VERIFIED. The branch is no longer conflicting: c5d1f9c has f97db40 (the current PR base) as its second parent, and GitHub reports mergeable=true. The required remote checks subsequently completed successfully: Check and test and Changelog gate both pass. The HUSKY=0 commit bypassed the local hook invocation only after substantive checks; the hook's failure was its restaging attempt around the ignored .omo path, not a source/test failure. The same relevant gates are now independently evidenced by GitHub CI.
B2 - VERIFIED. The final convertMessages implementation contains the #1126 coalescing behavior and preserves standalone string user content. User text is kept as content:string unless it is appended to an existing wire user turn; tool results are appended through the same helper. The cache pass runs only after this coalescing, so [assistant(tool_use), toolResult, user text] serializes as assistant followed by one user message whose blocks remain tool_result then text, with the marker on the final text block. The added regression asserts the exact roles and block order; the #1126 diff shows the same coalescing/standalone-string design, with no unresolved ordering conflict.
B3 - VERIFIED. retainPrecedingCheckpoint is computed from the post-coalesce params. Its predicate requires the final wire message to be a user array containing a tool_result and the immediately preceding wire message to be an assistant array containing a tool_use. A normal user -> assistant(text) -> user history therefore takes only the normal tail marker. A tool result before a later assistant/user tail and an assistant without tool_use also fail the predicate. When the predicate is true, the backward scan adds at most one preceding user checkpoint; it does not add checkpoints to ordinary histories. The new ordinary multi-turn and long non-tool tests, plus the 11-test stability file run, verify this behavior.
B4 - VERIFIED. Coverage now includes empty history, the compaction boundary, ordinary and long non-tool histories, the coalesced interrupted turn, OAuth with and without a context system prompt, cacheRetention:none, unpaired results, and three consecutive tool loops. The focused stability suite passed 11/11; related Anthropic coalescing/provider-native/deferred-tool suites passed 41/41, and the coalescing plus cache-retention suites passed 24/24 with four expected skips. The required CI fan-in and changelog gate also passed.
Additional checks: for a non-OAuth request the possible generated markers are system + last tool + preceding user + tail user; for OAuth the identity marker is omitted when a system prompt exists, so the analogous maximum is also four. The three-loop tests assert four markers on the final wire payload. Cache usage accounting remains unchanged and continues to report cache_creation_input_tokens as cacheWrite, so the new rolling marker does not create an accounting mismatch. I found no new defensible blocker in the #1126 interaction, long-loop budget, or HUSKY=0 history.
Correcting an accidental approval: final verification found a standalone Anthropic string-shape regression.
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-2 adversarial review of c5d1f9c.
B1 - VERIFIED. The merge commit has f97db40 (the PR base) as its second parent and GitHub reports the branch mergeable. The required Check and test and Changelog gate checks both completed successfully. HUSKY=0 skipped the local hook invocation after the substantive checks, and the reported failure was the hook's restaging helper touching ignored .omo state; the same static/build/test gates are independently green in CI.
B2 - CORE SHAPE VERIFIED. The cache pass is after convertMessages coalescing. For [assistant(tool_use), toolResult, user text], the wire roles are assistant/user and the coalesced user blocks are tool_result followed by text; the new test asserts that exact order and marker placement. The post-coalesce predicate is therefore inspecting the shape that is actually sent.
B3 - VERIFIED. retainPrecedingCheckpoint requires the final wire user array to contain a tool_result and the immediately preceding assistant array to contain a tool_use. Plain user -> assistant(text) -> user histories, tool results before a later non-tool assistant, and assistants without tool_use do not enter the rolling path. The backward scan adds at most one preceding user marker. The ordinary and long non-tool tests passed, and the three-loop fixture shows the intended rolling behavior.
B4 - VERIFIED. The added suite covers empty history, compaction boundary, long non-tool history, coalesced interrupted turns, OAuth with/without context system prompts, cacheRetention:none, unpaired results, and three consecutive tool loops. Focused stability passed 11/11; related Anthropic suites passed 41/41 and the coalescing/cache-retention run passed 24/24 (4 skipped). CI's full required fan-in also passed.
B5 - NEW BLOCKER: standalone string user content is still regressed in this head. In convertMessages(), the plain-string user branch at packages/ai/src/api/anthropic-messages.ts:2269 always calls appendUserBlocks(params, [{ type: "text", ... }]). When no adjacent user turn exists, that helper creates an array anyway. I reproduced this directly with cacheRetention:none: a standalone input {role:"user", content:"standalone"} is sent as {role:"user", content:[{type:"text", text:"standalone"}]} instead of the prior {role:"user", content:"standalone"} wire shape. This is exactly the compatibility regression fixed by #1126's latest 0c7ee98 commit, whose diff preserves strings unless appending to an existing user turn; c5d1f9c does not contain that fix. The current PR's new tests do not assert standalone string preservation, so a merge/conflict resolution that kept the coalescing half has silently dropped #1126's final compatibility fix. Apply the #1126 standalone branch while retaining this PR's post-coalesce checkpoint pass, and add the standalone cacheRetention:none wire-shape assertion to this PR's coverage.
I found no separate blocker in the rolling cache-write accounting or long-loop breakpoint budget: the generated dynamic markers are bounded to system/identity + last tool + one preceding user + tail user (maximum four), and the adapter continues to report provider cache_creation_input_tokens as cacheWrite.
|
Fixed in 93e4e73.
Verification:
|
|
WORKING: review-1075-r3 - inspecting the B5 branch, coalescing boundaries, metadata behavior, and edge-case coverage. |
code-yeongyu
left a comment
There was a problem hiding this comment.
Round-3 adversarial re-review of 93e4e73.
B5 - VERIFIED. In convertMessages(), a nonempty string user message is preserved as content:string only when the last emitted wire parameter is not a user turn. If the preceding emitted turn is user content produced by a consecutive user message or a tool-result turn, appendUserBlocks() is used and the resulting content is an array. An assistant turn in between leaves the string standalone. The new cacheRetention:none assertion is an exact wire-shape equality, so reverting to the prior unconditional appendUserBlocks() behavior would fail it; the author also provided failing-first evidence.
The post-coalesce checkpoint pass still sees the final wire shape. For [assistant(tool_use), toolResult, user text], the tool_result and text remain in that order, the tail text receives the new checkpoint, and the backward scan retains the preceding user checkpoint only for the genuine tool-loop predicate. A preserved standalone string with cache retention enabled is not given cache_control as a string: markUserMessageCacheCheckpoint() first promotes it to a text block and then attaches the metadata. With cacheRetention:none, the pass is disabled and the string remains unchanged. Empty and whitespace-only user strings are skipped before either branch, so they create neither malformed blocks nor checkpoints.
I also checked the branch/pass interaction and edge shapes directly: standalone cached and uncached strings, consecutive user strings, interrupted tool-result plus user text, separated assistant turns, empty strings, and cache-disabled coalescing. The targeted stability suite passed 12/12; related cache-retention, provider-native replay, and deferred-tools suites passed 65 tests with 4 expected skips. LSP diagnostics and git diff --check are clean, and the worktree is unchanged.
No new defensible blockers found.
Summary
Fixes direct
anthropic-messagesprompt-cache replay during tool loops by retaining the prior dynamic cache checkpoint while marking the newest tool-result tail.Evidence
./node_modules/.bin/vitest --run packages/ai/test/anthropic-cache-checkpoint-stability.test.ts packages/ai/test/warm-prompt-cache.test.ts(8 passed)./node_modules/.bin/tsc --noEmit --pretty falsepassed.Related: #1005 (goal-continuation cache prefix stability); this PR addresses the remaining serializer-level checkpoint movement.
Summary by cubic
Retains Anthropic prompt-cache checkpoints across tool loops so prefix reprocessing is avoided, and preserves standalone string user content when cache retention is disabled.
anthropic-messagesrequests.packages/ai/test/anthropic-cache-checkpoint-stability.test.ts.Written for commit 93e4e73. Summary will update on new commits.