feat(providers): preserve prompt cache affinity - #622
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProvider API keys now use deterministic session affinity by default, with an opt-out for strict round-robin rotation. Anthropic request translation now preserves supported ChangesProvider key affinity
Anthropic cache-control preservation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant Provider
participant Keyring
participant Upstream
Request->>Provider: request with session context
Provider->>Keyring: NextForContext(ctx)
Keyring-->>Provider: pinned or round-robin key
Provider->>Upstream: authenticated request
sequenceDiagram
participant CanonicalRequest
participant Router
participant AnthropicTranslation
participant AnthropicAPI
CanonicalRequest->>Router: chat request with cache-control
Router->>AnthropicTranslation: provider-specific adaptation
AnthropicTranslation->>AnthropicAPI: translated request with supported cache-control
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/providers/anthropic/request_translation.go (1)
571-604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the tail validation between
anthropicCacheControlFromExtraandanthropicCacheControlFromValue.Both functions perform the same "empty/null → nil, else must start with
{, else clone" check after producing ajson.RawMessagefrom different sources. Extract that shared tail into one helper to avoid the two implementations drifting apart.♻️ Proposed refactor
+func validatedCacheControlJSON(raw json.RawMessage) (json.RawMessage, error) { + trimmed := bytes.TrimSpace(raw) + if len(trimmed) == 0 || core.IsJSONNull(trimmed) { + return nil, nil + } + if trimmed[0] != '{' { + return nil, core.NewInvalidRequestError("anthropic cache_control must be an object", nil) + } + return core.CloneRawJSON(trimmed), nil +} + func anthropicCacheControlFromExtra(extraFields core.UnknownJSONFields) (json.RawMessage, error) { raw := extraFields.Lookup("cache_control") if len(raw) == 0 { return nil, nil } - trimmed := bytes.TrimSpace(raw) - if core.IsJSONNull(trimmed) { - return nil, nil - } - if trimmed[0] != '{' { - return nil, core.NewInvalidRequestError("anthropic cache_control must be an object", nil) - } - return core.CloneRawJSON(trimmed), nil + return validatedCacheControlJSON(raw) } func anthropicCacheControlFromValue(value any) (json.RawMessage, error) { if value == nil { return nil, nil } raw, err := json.Marshal(value) if err != nil { return nil, core.NewInvalidRequestError("anthropic cache_control must be an object", err) } - trimmed := bytes.TrimSpace(raw) - if len(trimmed) == 0 || core.IsJSONNull(trimmed) { - return nil, nil - } - if trimmed[0] != '{' { - return nil, core.NewInvalidRequestError("anthropic cache_control must be an object", nil) - } - return core.CloneRawJSON(trimmed), nil + return validatedCacheControlJSON(raw) }🤖 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 `@internal/providers/anthropic/request_translation.go` around lines 571 - 604, Extract the shared post-processing logic from anthropicCacheControlFromExtra and anthropicCacheControlFromValue into a helper that accepts the produced raw JSON, returns nil for empty or JSON null values, rejects non-object JSON with the existing invalid-request error, and clones valid objects. Update both functions to delegate to this helper while preserving their source-specific handling and error behavior.internal/anthropicapi/request.go (1)
534-544: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDrop
cache_controlbefore routing to non-Anthropic providers.
buildExtraFieldsunconditionally storesreq.CacheControlincore.ChatRequest.ExtraFields, and OpenAI-style providers emitExtraFieldsin the final request body. If a request withcache_controlroutes to OpenAI, the unknown top-level field can produce a400instead of the intended graceful drop. Add an Anthropic-only gate/strip path for this extra field.🤖 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 `@internal/anthropicapi/request.go` around lines 534 - 544, Ensure the cache_control field from buildExtraFields is retained only for Anthropic requests. Before routing or serializing requests for non-Anthropic providers, remove req.CacheControl or its cache_control entry from core.ChatRequest.ExtraFields, while preserving stop and other provider-neutral fields.
🤖 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 `@internal/anthropicapi/request.go`:
- Around line 361-372: Extract the shared cache-control validation from
anthropicCacheControlExtra and convertTools into a helper that trims the raw
JSON, accepts empty or null values, and rejects non-object or invalid JSON with
the established error behavior. Update both callers to use this helper while
preserving anthropicCacheControlExtra’s unknown-field construction and
convertTools’ existing flow.
In `@internal/embedding/embedding.go`:
- Line 134: Update NewEmbedder’s providers.NewKeyring construction to pass the
provider’s raw.SessionStickyKeys setting, defaulting it to true when
unspecified, so embedding requests honor session_sticky_keys=false while
preserving sticky behavior by default.
In `@internal/providers/openrouter/openrouter_test.go`:
- Around line 59-61: Synchronize the shared gotSessionID access between the
httptest handler and the test assertions. Replace the unsynchronized write/read
with a channel or mutex, ensuring the test waits for the handler’s captured
X-Session-Id before asserting it and remains race-free under go test -race.
In `@web/dashboard/src/pages/providers-config/providersConfigLogic.js`:
- Around line 460-465: Update the rendered-field loop in
buildProviderCredentialPayload to skip FIELD_SESSION_STICKY_KEYS when schema is
unavailable, ensuring it is not marked rendered or included in the payload;
preserve existing behavior when schema is present, and add a regression test
covering buildProviderCredentialPayload(form, null).
---
Outside diff comments:
In `@internal/anthropicapi/request.go`:
- Around line 534-544: Ensure the cache_control field from buildExtraFields is
retained only for Anthropic requests. Before routing or serializing requests for
non-Anthropic providers, remove req.CacheControl or its cache_control entry from
core.ChatRequest.ExtraFields, while preserving stop and other provider-neutral
fields.
In `@internal/providers/anthropic/request_translation.go`:
- Around line 571-604: Extract the shared post-processing logic from
anthropicCacheControlFromExtra and anthropicCacheControlFromValue into a helper
that accepts the produced raw JSON, returns nil for empty or JSON null values,
rejects non-object JSON with the existing invalid-request error, and clones
valid objects. Update both functions to delegate to this helper while preserving
their source-specific handling and error behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e9dbd72d-2ca6-4d34-9c1c-4248330b0909
⛔ Files ignored due to path filters (3)
internal/admin/dashboard/static/dist/assets/index-ClpBYKZR.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-DNWYS-jJ.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (47)
CLAUDE.mdconfig/config.example.yamlconfig/providers.godocs/adr/0007-anthropic-messages-ingress.mddocs/advanced/anthropic-messages-api.mdxdocs/dev/2026-07-17_anthropic-sdk-compat-findings.mddocs/features/session-keeping.mdxdocs/providers/key-rotation.mdxdocs/providers/overview.mdxinternal/admin/handler_provider_credentials.gointernal/admin/handler_provider_credentials_test.gointernal/anthropicapi/request.gointernal/anthropicapi/request_test.gointernal/anthropicapi/types.gointernal/embedding/embedding.gointernal/providers/anthropic/anthropic.gointernal/providers/anthropic/anthropic_test.gointernal/providers/anthropic/request_translation.gointernal/providers/anthropic/types.gointernal/providers/azure/realtime.gointernal/providers/bailian/realtime.gointernal/providers/bedrockmantle/auth.gointernal/providers/cohere/cohere.gointernal/providers/config.gointernal/providers/config_test.gointernal/providers/credential_schema.gointernal/providers/credential_schema_test.gointernal/providers/credentials.gointernal/providers/credentials_store_mongodb.gointernal/providers/credentials_store_sql.gointernal/providers/credentials_store_sql_test.gointernal/providers/factory.gointernal/providers/gemini/gemini.gointernal/providers/keyring.gointernal/providers/keyring_test.gointernal/providers/ollama/ollama.gointernal/providers/openai/compatible_provider.gointernal/providers/openai/realtime.gointernal/providers/openrouter/openrouter.gointernal/providers/openrouter/openrouter_test.gointernal/providers/provider_status.gointernal/providers/xai/realtime.gointernal/providers/zai/realtime.gorun/providers_test.goweb/dashboard/src/pages/providers-config/ProviderCredentialField.svelteweb/dashboard/src/pages/providers-config/providersConfigLogic.jsweb/dashboard/tests/providers-config.test.js
Confidence Score: 5/5Safe to merge based on the exercised translation, routing, and credential-selection paths. The final defect set is empty. The exercised request path preserved cache controls only where supported, and the key-selection path behaved deterministically for identified sessions while preserving configured round-robin behavior elsewhere. Files Needing Attention: No files require corrective action. The primary exercised areas were internal/anthropicapi/request.go, internal/providers/cache_control.go, internal/providers/router.go, and internal/providers/keyring.go.
 T-Rex LogsWhat T-Rex did
Reviews (3): Last reviewed commit: "fix(providers): adapt message batch cach..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed the review pass in 55cf7a9:
Validated by the full pre-commit suite: race tests, dashboard tests/dist check, performance guard, modernization check, lint, and docs validation. |
|
Added environment-only configuration parity in ce1bbe3: |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/dashboard/src/pages/providers-config/providersConfigLogic.js (1)
453-471: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the same schema-availability check for payload construction.
providerCredentialFormFieldstreats missing or emptyschema.fieldsas the compatibility fallback. This guard checks onlyschema == null. If a matched schema hasfields: []or omitsfields, the fallback includessession_sticky_keys, and this loop sendssession_sticky_keys: trueeven though the gateway did not advertise it. Base the skip on the explicit schema field list and add a regression test for an empty or missingfieldsarray.Proposed fix
const { primary, advanced } = providerCredentialFormFields(schema); + const hasAdvertisedFields = Boolean( + schema && Array.isArray(schema.fields) && schema.fields.length > 0, + ); const rendered = new Set(); for (const field of [...primary, ...advanced]) { - if (!schema && field.name === FIELD_SESSION_STICKY_KEYS) { + if (!hasAdvertisedFields && field.name === FIELD_SESSION_STICKY_KEYS) { continue; }🤖 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 `@web/dashboard/src/pages/providers-config/providersConfigLogic.js` around lines 453 - 471, Update the `session_sticky_keys` skip logic in the payload-construction loops to use the same explicit schema-field availability check as `providerCredentialFormFields`, treating missing or empty `schema.fields` as unsupported rather than checking only `schema`. Ensure `FIELD_SESSION_STICKY_KEYS` is omitted unless it is advertised in the schema, and add a regression test covering an empty or missing `fields` array.
🤖 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 `@internal/anthropicapi/request.go`:
- Around line 374-383: Extract the duplicated trim/empty-null/object-shape/clone
logic from validatedCacheControlJSON in internal/anthropicapi/request.go lines
374-383 and validatedAnthropicCacheControlJSON in
internal/providers/anthropic/request_translation.go lines 586-594 into a shared
core helper returning cloned JSON and a plain error; update both callers to use
it, with validatedAnthropicCacheControlJSON wrapping failures in
core.NewInvalidRequestError("anthropic cache_control must be an object", nil)
while preserving each function’s existing behavior.
In `@internal/embedding/embedding_test.go`:
- Around line 55-83: Replace TestNewEmbedder_HonorsSessionStickyKeysOptOut with
table-driven HTTP tests that exercise the public embedding request path,
covering default session-sticky behavior and SessionStickyKeys set to false. Use
a test server to record authorization keys from successive embedding requests
made with the same session context, and assert the observed key sequences
without accessing apiEmbedder or its keyring directly.
In `@internal/providers/config_test.go`:
- Around line 473-487: Refactor TestApplyProviderEnvVars_SessionStickyKeys into
a table-driven test covering unsuffixed and suffixed discovery,
environment-over-YAML precedence, valid true/false values, default
configuration, and invalid environment values. Include a case with an existing
YAML SessionStickyKeys value and assert that an invalid environment value
preserves it, while retaining provider-specific mappings for openai and
openai-eu.
In `@internal/providers/router_test.go`:
- Around line 563-645: Expand
TestRouterChatCompletion_AdaptsAnthropicCacheControlAfterRouting into a
table-driven routing test covering provider types and cache-control behavior at
request, content, tool, tool-use, and tool-result locations. Assert forwarded
fields match each provider’s expectation, including removal for unsupported
providers and preservation for supported ones, while verifying every original
nested field remains unchanged; retain the existing x_keep preservation checks
where applicable.
---
Outside diff comments:
In `@web/dashboard/src/pages/providers-config/providersConfigLogic.js`:
- Around line 453-471: Update the `session_sticky_keys` skip logic in the
payload-construction loops to use the same explicit schema-field availability
check as `providerCredentialFormFields`, treating missing or empty
`schema.fields` as unsupported rather than checking only `schema`. Ensure
`FIELD_SESSION_STICKY_KEYS` is omitted unless it is advertised in the schema,
and add a regression test covering an empty or missing `fields` array.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5e4efa91-4d03-402e-89ab-e88a53fdf95f
⛔ Files ignored due to path filters (2)
internal/admin/dashboard/static/dist/assets/index-BdnxAytg.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (18)
.env.templatedocs/features/session-keeping.mdxdocs/providers/key-rotation.mdxinternal/anthropicapi/request.gointernal/core/json_fields.gointernal/core/json_fields_test.gointernal/embedding/embedding.gointernal/embedding/embedding_test.gointernal/providers/anthropic/request_translation.gointernal/providers/cache_control.gointernal/providers/config.gointernal/providers/config_test.gointernal/providers/keyring.gointernal/providers/openrouter/openrouter_test.gointernal/providers/router.gointernal/providers/router_test.goweb/dashboard/src/pages/providers-config/providersConfigLogic.jsweb/dashboard/tests/providers-config.test.js
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 57 changed files in this pull request and generated no new comments.
Suppressed comments (2)
internal/providers/router.go:558
forwardChatRequestcallsr.GetProviderType(selector.QualifiedModel()), which re-runs model resolution even thoughselectoris already resolved. This adds avoidable work on the hot path (especially for cache_control-heavy traffic) and risks subtle divergence ifResolveModelapplies additional rewriting.
internal/providers/cache_control.go:17adaptAnthropicCacheControltreats the generic field namecache_controlas “Anthropic-only” and strips it for most provider types. Elsewhere in the codebase,cache_controlis used as an opaque/unknown nested field that is preserved for OpenAI-compatible chat payloads (e.g. OpenAI provider tests), so this routing-time stripping is a backwards-incompatible behavior change for/v1/chat/completionscallers relying on that passthrough. Consider scoping Anthropic/v1/messagescache controls under a distinct internal metadata key (or otherwise gating this stripping to the Anthropic-ingress path) so OpenAI-compatiblecache_controlusage is not unintentionally removed.
const anthropicCacheControlField = "cache_control"
// adaptAnthropicCacheControl removes Anthropic-only cache directives after the
// route is known unless the selected provider accepts Anthropic request shapes.
// The caller's request remains unchanged.
func adaptAnthropicCacheControl(req *core.ChatRequest, providerType string) *core.ChatRequest {
if req == nil || providerAcceptsAnthropicCacheControl(providerType) || !hasAnthropicCacheControl(req) {
return req
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 57 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/providers/anthropic/request_translation.go:290
cache_controlis only read fromtoolCall.ExtraFieldswhen building Anthropictool_useblocks. However, the rest of this PR treatstoolCall.functionas another possible location forcache_control(e.g. cache stripping and router tests). If a caller sendscache_controlundertool_calls[].function, it will currently be dropped during Anthropic translation, defeating the goal of preserving prompt-cache breakpoints at tool-use locations.
cacheControl, err := anthropicCacheControlFromExtra(toolCall.ExtraFields)
if err != nil {
return nil, err
}
|
Completed an exhaustive pass over all review surfaces (issue comments, review summaries, inline threads, and suppressed Copilot comments). The remaining three actionable suppressed findings are fixed in 2b9d150:
The full pre-commit gate passed: race tests, gofmt/imports, module tidiness, hot-path performance thresholds, fix-check, and lint. The other comments are resolved, obsolete after earlier fixes, informational/status-only, or repository/org configuration requests rather than code changes. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/providers/router_test.go (1)
624-669: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winVerify caller
ExtraFieldsdirectly after routing.Lines 624-669 serialize
ChatRequestwithjson.Marshal.ExtraFieldsusesjson:"-", so this comparison cannot detect mutations to request, message, content-part, tool-call, function-call, or tool-result cache metadata.After routing, assert that each original
UnknownJSONFieldsvalue still containscache_controlandx_keep. Keep the serialized comparison for fields that are part of the JSON request shape.🤖 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 `@internal/providers/router_test.go` around lines 624 - 669, Update the caller-mutation assertions in the ChatCompletion routing test to inspect the original request’s ExtraFields directly after routing, rather than relying on json.Marshal for metadata tagged json:"-". Apply the existing assertFields helper to the request, message, content-part, tool-call, function-call, and tool-result ExtraFields, while retaining the serialized before/after comparison for JSON-visible fields.
🤖 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 `@internal/providers/router_test.go`:
- Around line 624-669: Update the caller-mutation assertions in the
ChatCompletion routing test to inspect the original request’s ExtraFields
directly after routing, rather than relying on json.Marshal for metadata tagged
json:"-". Apply the existing assertFields helper to the request, message,
content-part, tool-call, function-call, and tool-result ExtraFields, while
retaining the serialized before/after comparison for JSON-visible fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4844036c-05e7-4279-a561-821f2e00dfdf
📒 Files selected for processing (7)
internal/anthropicapi/request.gointernal/anthropicapi/request_test.gointernal/core/types.gointernal/providers/anthropic/anthropic_test.gointernal/providers/anthropic/request_translation.gointernal/providers/router.gointernal/providers/router_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 58 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/providers/router.go:560
AnthropicMessagesRequestis gateway-only routing metadata (likeProvider). After using it to decide whether to adapt cache_control, it should be cleared before dispatching to providers to avoid leaking internal flags into downstream logic (even if it’s JSON-ignored today).
|
Addressed the two findings from the latest review pass in 671e5cc:
The full pre-commit gate passes again. PR #624 has also been rebased onto this commit and its focused provider/cache suites pass. |
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 `@internal/providers/router_test.go`:
- Around line 646-648: Update the cache_control assertions in assertFields and
the forwarded.Tools[0] checks to compare the canonical JSON value against the
expected preservation value, rather than only checking key presence. For
stripping cases, explicitly assert that cache_control is absent; preserve the
existing table-driven provider translation coverage.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e86d4c6b-e512-44b4-ab6c-359f36a06f7d
📒 Files selected for processing (2)
internal/providers/router.gointernal/providers/router_test.go
|
Fixed the batch-path cache-control gap and exact-value assertion in dda4362.
The full pre-commit suite passes. PR #624 was rebased onto this commit at 89b23f8, and its focused provider/cache/server suites pass. |
Summary
cache_controlthrough managed/v1/messagestranslation at request, system/content, tool, tool-use, and tool-result locationssession_sticky_keysopt-out in YAML, admin persistence, migrations, and the dashboardValidation
go test ./...make test-race, lint, performance guard, dashboard tests/dist check, Mintlify validation)npm run checkand production dashboard buildSummary by CodeRabbit
New Features
Bug Fixes
cache_controlsettings are preserved across supported messages, tools, batches, and content.Documentation