fix(vertex): enable prompt caching on native Claude paths - #1113
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds Anthropic prompt-cache breakpoint support for Vertex+Claude calls. New types define ChangesVertex Anthropic Prompt-Cache Breakpoints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. 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 |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/providers/googleVertex.ts`:
- Around line 3403-3411: The cache usage tokens (cacheReadTokens and
cacheCreationTokens) are being added to result.usage but are being stripped out
before cost calculation. Ensure that when attachUsageAndCostAttributes() is
called, it includes the full usage object with the cache token fields intact,
and verify that calculateCost() receives these cache usage values so it can
properly calculate and include cache read and write charges in the final cost
calculation instead of treating cached content as full input rate billing.
🪄 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: CHILL
Plan: Pro
Run ID: 9abfb3db-4313-46b4-b30e-a9adae8769fc
📒 Files selected for processing (5)
package.jsonsrc/lib/providers/googleVertex.tssrc/lib/types/providers.tssrc/lib/utils/anthropicCacheBreakpoints.tstest/continuous-test-suite-cache-breakpoints.ts
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed: 5
New issues raised: 5
Issues by Severity
| Severity | Count | Description |
|---|---|---|
| 🔒 CRITICAL | 0 | None |
| 1 | Cache metrics accumulation bug in streaming path | |
| 💡 MINOR | 1 | Shallow clone documentation |
| 💬 SUGGESTION | 3 | Type safety, test coverage, naming |
Blocking Issue (MAJOR)
Cache metrics incorrectly handled in streaming path (googleVertex.ts)
In executeNativeAnthropicStream, the cache metrics are being assigned directly to usage.cacheReadTokens and usage.cacheCreationTokens inside the agentic loop. While turnCacheUsage correctly accumulates across steps, the assignment pattern overwrites these values each iteration. This means only the last step's cache metrics survive, not the running total.
Fix required: Accumulate into usage using += pattern (like the generate path does), or assign final turnCacheUsage values after the loop completes.
Non-blocking Observations
- Type definitions are well-structured and maintain backward compatibility
- Test coverage is good for the main scenarios; edge cases could be expanded
- Documentation clearly explains the Anthropic 4-breakpoint limit and allocation strategy
- Code organization follows existing patterns with the new utility properly isolated
Next Steps
Please address the MAJOR issue with cache metric accumulation in the streaming path. Once fixed, this PR should be ready for merge - the implementation correctly enables prompt caching on Vertex+Claude paths as described.
| turnCacheUsage.creation5m += stepCacheCreation5m; | ||
| turnCacheUsage.creation1h += stepCacheCreation1h; | ||
|
|
||
| usage.input += response.usage?.input_tokens || 0; |
There was a problem hiding this comment.
In the streaming path (executeNativeAnthropicStream), the cache metrics are being overwritten on each loop iteration instead of accumulated:
// Lines ~3400-3409 (in the loop)
if (turnCacheUsage.read > 0) {
usage.cacheReadTokens = turnCacheUsage.read; // Overwrites!
}
if (turnCacheUsage.creation > 0) {
usage.cacheCreationTokens = turnCacheUsage.creation; // Overwrites!
}Problem: Unlike the generate path which uses += to accumulate, the streaming path assigns directly. While turnCacheUsage correctly aggregates across steps, the assignment pattern overwrites usage.cacheReadTokens each iteration, so only the last step's values survive.
Compare with generate path (correct):
totalCacheReadTokens += response.usage?.cache_read_input_tokens || 0;
totalCacheCreationTokens += response.usage?.cache_creation_input_tokens || 0;Fix: Either accumulate directly into usage:
usage.cacheReadTokens = (usage.cacheReadTokens || 0) + stepCacheRead;
usage.cacheCreationTokens = (usage.cacheCreationTokens || 0) + stepCacheCreation;Or move the assignment outside the loop to use final turnCacheUsage values.
| return false; | ||
| } | ||
| messages[i] = { | ||
| ...message, |
There was a problem hiding this comment.
💡 MINOR: Shallow clone may not preserve deep structure integrity
The markLastContentBlock function performs shallow clones of content blocks:
const content = message.content.map((block) => ({ ...block }));While this is fine for the current type definitions (which don't have nested objects in blocks), if future content block types add nested properties (e.g., source in image/document blocks with nested metadata), this shallow clone could lead to unintended mutations of the original input.
Suggestion: Consider adding a comment documenting this assumption, or use a deep clone utility if the type definitions evolve to include deeply nested structures.
This is currently safe given the type definitions in providers.ts, but worth noting for maintainability.
| * block / tool / system block to make the rendered prefix up to that point a | ||
| * cache breakpoint. Vertex has NO automatic caching, so these explicit markers | ||
| * are the only way the conversation prefix is cached across turns. | ||
| */ |
There was a problem hiding this comment.
✅ Good: Proper type definitions for cache control
The type definitions correctly extend the Anthropic message/content block types with optional cache_control fields. This maintains backward compatibility while enabling the new caching functionality.
One minor observation: The VertexAnthropicCacheControl type is defined as { type: "ephemeral" } which matches Anthropic's API specification. Consider exporting this as a const assertion if the API guarantees this is the only valid value:
export const EPHEMERAL_CACHE_CONTROL = { type: "ephemeral" } as const;
export type VertexAnthropicCacheControl = typeof EPHEMERAL_CACHE_CONTROL;This would provide stronger type safety and prevent accidental mutation of the constant.
| testNoSystemMarksLastTool(); | ||
| testPurity(); | ||
| testHistoryBreakpointCap(); | ||
| }); |
There was a problem hiding this comment.
💡 SUGGESTION: Missing edge case tests
The test suite covers the main scenarios well, but consider adding tests for:
- Empty messages array - What happens when
messages: []? - Messages with empty content arrays -
content: []should skip that message - Very long message history (>4 messages) - Verify only the last 3 get history breakpoints when system is present
- Mixed content types - Test with image/document blocks to ensure cache_control is placed correctly
Also, the purity test verifies inputs aren't mutated, but doesn't verify that the output is a new object (not a reference to input). Consider adding:
recordTest("output is new object", out.messages !== inputMessages);
recordTest("output messages are new objects", out.messages[0] !== inputMessages[0]);| "test": "npx tsx test/continuous-test-suite.ts", | ||
| "test:client": "npx tsx test/continuous-test-suite-client.ts", | ||
| "test:context": "npx tsx test/continuous-test-suite-context.ts", | ||
| "test:cache": "npx tsx test/continuous-test-suite-cache-breakpoints.ts", |
There was a problem hiding this comment.
✅ Good: Test script properly added
The new test:cache script follows the existing naming convention and correctly references the new test file. This maintains consistency with the existing test suite structure.
|
Thanks for the thorough reviews — addressed in the latest commit. Per finding:
|
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Code Review Summary
I've completed a thorough review of this PR that enables Anthropic prompt caching on native Vertex+Claude request paths.
Files Reviewed
package.json- Test script addition ✅src/lib/providers/googleVertex.ts- Cache metrics integration ✅src/lib/types/providers.ts- Cache control type definitions ✅src/lib/utils/anthropicCacheBreakpoints.ts- New utility (pure, well-documented) ✅test/continuous-test-suite-cache-breakpoints.ts- Comprehensive unit tests ✅
Verification of Previous Review Comments
All 6 existing review threads have been addressed:
- Cache metrics accumulation - Confirmed fixed: streaming path assigns cumulative values once after the agentic loop (not per-iteration)
- Cost path integration - Confirmed fixed:
attachUsageAndCostAttributes()passes cache tokens tocalculateCost() - Type definitions - Properly extend Anthropic types with optional
cache_controlfields - Test coverage - 14 tests covering system/tools breakpoints, rolling history, purity, caps, and edge cases
Key Implementation Highlights
- Correct accumulation pattern: Generate path uses
+=, streaming path accumulates inturnCacheUsagethen assigns once - Cost calculation wired: Cache read (~0.1x) and creation (~1.25x) tiers properly passed to pricing
- Pure utility:
applyVertexAnthropicCacheBreakpointsclones inputs, never mutates - Budget-aware: Respects Anthropic's 4-breakpoint limit with smart allocation (system/tools + rolling history)
Compliance Check
- ✅ No hardcoded secrets
- ✅ No breaking changes to public SDK API
- ✅ Follows CLAUDE.md architectural rules
- ✅ Type-safe with proper TypeScript strict mode compliance
- ✅ Error handling via ErrorFactory patterns
- ✅ Observability: cache metrics exposed on spans for analytics
Approved - This is a well-implemented performance fix that restores caching parity with the Claude Agent SDK on Vertex.
d4e7241 to
42b9287
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
I've completed a comprehensive review of PR #1113 which enables Anthropic prompt caching on the native Vertex+Claude request paths.
Files Reviewed (5)
src/lib/utils/anthropicCacheBreakpoints.ts(new - 125 lines)src/lib/types/providers.ts(modifications - 71 additions)src/lib/providers/googleVertex.ts(modifications - 105 additions, 4 deletions)test/continuous-test-suite-cache-breakpoints.ts(new - 256 lines)package.json(1 new test script)
Verification Against Project Standards
| Criteria | Status |
|---|---|
| Security - No hardcoded secrets | ✅ Pass |
| CLAUDE.md Rule 1 - Dynamic imports only in registry | ✅ Pass |
CLAUDE.md Rule 6 - formatProviderError returns, never throws |
✅ Pass |
Type Safety - No any papering |
✅ Pass |
| Backward Compatibility - Public API unchanged | ✅ Pass |
Cache Metrics Accumulation - Correct += pattern |
✅ Pass |
Key Implementation Details Verified
-
Cache Breakpoint Logic: The
applyVertexAnthropicCacheBreakpoints()function correctly implements Anthropic's 4-breakpoint limit with proper budget allocation (1 for system/tools prefix, up to 3 for rolling history). -
Immutability: Inputs are cloned (shallow copy sufficient for current type definitions) and never mutated - verified in both implementation and tests.
-
Cache Metrics Flow:
- Streaming path:
turnCacheUsageaccumulates via+=during the agentic loop, assigned tousageonce after completion - Generate path:
totalCacheReadTokens/totalCacheCreationTokensaccumulate via+=across steps - Both paths pass cache tokens to
calculateCost()for proper pricing (~0.1x read / ~1.25x write tiers)
- Streaming path:
-
Test Coverage: 14 tests covering system+history breakpoints, no-system fallback to tools, purity, history cap, and edge cases (empty arrays, overflow, output identity).
Existing Review Comments
All blocking issues previously raised have been resolved:
- Cache metrics overwriting issue (Tara-ag) - ✅ Fixed
- Cost path integration (CodeRabbit) - ✅ Fixed
Non-blocking suggestions (shallow clone documentation, additional edge case tests) are minor and can be addressed in follow-ups.
Decision
APPROVE - This PR is ready for merge. The implementation correctly fixes the Vertex+Claude prompt caching regression, follows all architectural rules, maintains backward compatibility, and includes adequate test coverage.
42b9287 to
c765c0d
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
🎉 This PR is included in version 9.79.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Complete ✅
Files reviewed: 5 (package.json, googleVertex.ts, providers.ts, anthropicCacheBreakpoints.ts, continuous-test-suite-cache-breakpoints.ts)
New issues found: 0
Assessment
This PR correctly implements prompt caching for the native Vertex+Claude paths:
- Cache metrics accumulation properly uses
+=pattern across agentic loop iterations, with final values assigned once after loop completion (lines 3694-3705) - Cache breakpoints are correctly placed on system/tools (stable prefix) and rolling history messages
- Type safety maintained with backward-compatible optional fields
- No security issues - no hardcoded secrets or injection risks
- CLAUDE.md compliance verified - no architectural rule violations
Existing Open Comments Acknowledged
Two minor items from prior review remain open but non-blocking:
- Thread 2 (MINOR): Shallow clone documentation - acceptable with existing comment
- Thread 4 (SUGGESTION): Additional edge case tests - nice to have, not required
The PR is approved for merge (already merged). The cache fix will significantly reduce token costs for Vertex Claude users by enabling the ~0.1x cache-read pricing tier.
This branch was successfully deployed
3 participants
Description
Enables Anthropic prompt caching on the native Vertex+Claude request paths. The conversation history was never given a
cache_controlbreakpoint, so every turn re-sent the full (growing, tool-result-heavy) prompt as fresh input —cache_read_input_tokenswas0on every turn, including 600K+ token turns. This restores caching parity with the Claude Agent SDK on the same Vertex project.Type of Change
Motivation and Context
Vertex does not support automatic prompt caching — caching only activates from explicit
cache_controlbreakpoints in the request. The nativeexecuteNativeAnthropicGenerate/executeNativeAnthropicStreampaths set none, so the conversation prefix fell after the last breakpoint and was billed at full input price every turn (cost scaled with conversation length). The cost math (pricing.ts) was already correct — this was a missing-breakpoint bug, not a mispricing.Changes Made
applyVertexAnthropicCacheBreakpoints()(src/lib/utils/anthropicCacheBreakpoints.ts) — pure helper placing ≤4cache_controlbreakpoints: one on the system block (cachestools + system, since system renders after tools), plus a rolling breakpoint on the trailing history messages (resilient to Anthropic's 20-block lookback on tool-heavy turns). Falls back to the last tool when there is no system prompt.googleVertex.ts, re-applied per agentic step so the stable prefix stays byte-identical while the history breakpoint rolls forward.cache_read_input_tokens/cache_creation_input_tokensand expose them ascacheReadTokens/cacheCreationTokensonresult.usage, so analytics can see caching working andcalculateCostprices the ~0.1x read / ~1.25x write tiers.cache_controladded to the Vertex-Anthropic message/tool/system types (canonicalsrc/lib/types/).test/continuous-test-suite-cache-breakpoints.ts+pnpm run test:cache.Breaking Changes
Testing
pnpm run test:cache— 14/14 pass, no API key)pnpm build(0 errors, publint clean)Verification after deploy: on consecutive main-flow turns,
usage.cacheReadTokensgoes from0→ nonzero (turn 1 writes the cache, turns 2+ read at 0.1x);input_tokens/call drops toward the newest-turn size; the cache-read SKU appears in the GCP billing export.Code Quality
Commit Message Format
fix(vertex): enable prompt caching on native Claude pathsDeployment Notes
Additional Notes
Scope here is the caching fix (Vertex+Claude). The complementary history-growth track (summarize large tool results at ingest + a history token budget) is intentionally out of scope for this PR and will follow separately; caching makes the large history cheap to re-send, summarization makes it smaller.
Summary by CodeRabbit
Release Notes
test:cachescript for running the cache-breakpoint test suite.