fix: [#1009] activate Snowflake Cortex prompt caching for Claude models#1020
Conversation
Cortex's Chat Completions API only honors prompt caching for Claude models via block-level markers (`messages[].content[].cache_control`, `ephemeral`, max 4 breakpoints). `@ai-sdk/openai-compatible` serializes our cache providerOptions as message-level fields — on `system` messages, on single-text-part `user` messages (collapsed to string content), on `tool` messages, and into assistant `tool_calls` entries — all silently ignored by Cortex, so every request billed the full input rate (`cache_read_input`/`cache_write_input` stayed NULL in `TOKENS_GRANULAR`). - `relocateCacheControl`: move message-level markers into content blocks (string content is wrapped in a text block; array content attaches to the last non-empty block), strip stray markers from `tool_calls` entries, leave pre-existing block-level markers untouched, and enforce the 4-breakpoint limit keeping the trailing markers (longest prefixes). - Fallback: if Cortex rejects a cache-marked request with a 400, retry once with markers stripped via `stripCacheControl`; when the stripped retry succeeds, disable caching for the rest of the session (`cacheControl` param on `transformSnowflakeBody` strips all markers when disabled). - Contract test drives `ProviderTransform.applyCaching` through the real `@ai-sdk/openai-compatible` serialization and the plugin transform. - Env-gated live e2e tests: cache activity via `usage.prompt_tokens_details.cached_tokens`, and acceptance of `role:"tool"` messages with array content carrying markers. - Docs + CHANGELOG entries. Closes #1009 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
full receipts (2 sessions)
orchestrator ·
|
| subagent | cost |
|---|---|
| agent-aglm5-reviewer-81e1a2d9cfc161f3 · claude-sonnet-5 | ≥ $3.0816 |
| agent-aminimax-reviewer-af3ab9c88cd88d75 · claude-sonnet-5 | ≥ $2.6444 |
| agent-adeepseek-reviewer-7f791f93b26cccb3 · claude-sonnet-5 | ≥ $2.3129 |
| agent-agemini-reviewer-1f1f416419a90b2e · claude-sonnet-5 | ≥ $1.9977 |
| agent-amimo-reviewer-c05561467a6dfb7f · claude-sonnet-5 | ≥ $1.8187 |
| agent-agrok-reviewer-3684d935dddf1a29 · claude-sonnet-5 | ≥ $1.7811 |
| agent-aqwen-reviewer-680478472935ab7a · claude-sonnet-5 | ≥ $1.7811 |
| agent-agpt-reviewer-0517b09cc8290db1 · claude-sonnet-5 | ≥ $1.7437 |
| agent-akimi-reviewer-a21643382d43872f · claude-sonnet-5 | ≥ $1.0077 |
codex · a7762cee
- - - - - - - - - - - - - - - - - - - - - - - - -
AIRECEIPTS
“Review this git diff for the fix to GitHub is…”
Codex · Jul 20 2026 18:34:20 UTC · 12m 54s
gpt-5.6-sol 100%
cache served 93% of input tokens
pre-edit: no named edit tool observed
(share before the first named edit tool)
exec.........................≥ $2.6576 (25 calls)
caveat: Codex trace omits GPT-5.6 cache-write tokens — floor excludes any write premium
--------------------------------------------------
KNOWN PRICED SUBTOTAL....................≥ $2.6576
standard API-equivalent floor; not an invoice
partial pricing coverage; invoice total unknown
same tokens on gpt-5.4-mini..............≥ $0.3986
(85% lower observable floor)
(arithmetic, not a prediction)
- - - - - - - - - - - - - - - - - - - - - - - - -
npx aireceipts-cli
github.com/anandgupta42/receipts
- - - - - - - - - - - - - - - - - - - - - - - - -
handoff — flagged pattern cost ≈ 4,788,862 tok
FLAGGED PATTERN COST...............≈ 4,788,862 tok
heuristic pattern subtotal · not proven savings
≈ re-priced eligible trivial spans.......≈ $0.4945
(9 tiny turns, priced at claude-haiku-4-5)
→ route short replies to a cheaper model
covers: 2 sessions · 256 turns · 1 flagged-pattern line
Generated by aireceipts
|
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:
📝 WalkthroughWalkthroughSnowflake Cortex now supports Claude prompt-cache marker relocation with retry fallback, and its model catalog and documentation are refreshed. Tests cover cache transformations, session behavior, current model availability, and updated Claude model identifiers. ChangesSnowflake Cortex Claude prompt caching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SnowflakeCortexAuthPlugin
participant transformSnowflakeBody
participant Cortex
Client->>SnowflakeCortexAuthPlugin: Send request
SnowflakeCortexAuthPlugin->>transformSnowflakeBody: Transform with cache control
transformSnowflakeBody-->>SnowflakeCortexAuthPlugin: Return body and cacheApplied
SnowflakeCortexAuthPlugin->>Cortex: Submit marked request
Cortex-->>SnowflakeCortexAuthPlugin: Return HTTP 400
SnowflakeCortexAuthPlugin->>Cortex: Retry without cache markers
Cortex-->>SnowflakeCortexAuthPlugin: Return retry response
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opencode/test/altimate/cortex-snowflake-e2e.test.ts (2)
401-402: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsume the first response body.
It is a good practice to consume the response body of the first
fetchto ensure the underlying connection can be returned to the pool or gracefully closed before the second request is dispatched.♻️ Proposed refactor
const first = await request() expect(first.status).toBe(200) + await first.text()🤖 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 `@packages/opencode/test/altimate/cortex-snowflake-e2e.test.ts` around lines 401 - 402, Consume the body of the first response returned by request() before dispatching the second request, while preserving the existing status assertion in the test flow.
378-379: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the token estimate.
The comment estimates
~1,400 tokens, but 700 iterations of this filler string actually produce around 7,000 tokens (which still safely exceeds the 4,096 minimum for Opus/Haiku).📝 Proposed update
- // ~1,400 tokens of deterministic filler — above the 1,024-token minimum + // ~7,000 tokens of deterministic filler — above the 1,024-token minimum // for claude-3-5-sonnet cache entries (Opus/Haiku models need 4,096).🤖 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 `@packages/opencode/test/altimate/cortex-snowflake-e2e.test.ts` around lines 378 - 379, Update the filler-token estimate comment near the deterministic filler generation to state approximately 7,000 tokens, while preserving the note that this exceeds the 4,096-token minimum required by Opus/Haiku models.
🤖 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.
Nitpick comments:
In `@packages/opencode/test/altimate/cortex-snowflake-e2e.test.ts`:
- Around line 401-402: Consume the body of the first response returned by
request() before dispatching the second request, while preserving the existing
status assertion in the test flow.
- Around line 378-379: Update the filler-token estimate comment near the
deterministic filler generation to state approximately 7,000 tokens, while
preserving the note that this exceeds the 4,096-token minimum required by
Opus/Haiku models.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dcb98848-160e-4283-b6c6-37aa7107b073
📒 Files selected for processing (5)
CHANGELOG.mddocs/docs/configure/providers.mdpackages/opencode/src/altimate/plugin/snowflake.tspackages/opencode/test/altimate/cortex-snowflake-e2e.test.tspackages/opencode/test/provider/snowflake.test.ts
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if (stripped !== body) { | ||
| const retry = await fetch(requestInput, { ...init, headers, body: stripped }) | ||
| if (retry.ok) { | ||
| cacheControlSupported = false |
There was a problem hiding this comment.
WARNING: cacheControlSupported race under concurrent fetches multiplies retries.
The flag is read at transform time (line 290) but only written here after the stripped retry succeeds. Between those points, concurrent in-flight requests all observe the stale true value, so each independently fires with cache markers, 400s, and retries stripped before the session-wide disable propagates. On parallel-agent workloads against an account that rejects markers, this multiplies Cortex request load N-fold during the window (each retry is billable on Snowflake credits) and can compound rate-limit backpressure. An in-flight retry guard or setting the flag before the retry would bound the fan-out.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 84bc9fa0cf (after the sticky flag became a 5-minute cooldown in 1912781896): the cooldown is now set eagerly when the cache-marked 400 is detected — before the probe retry runs — so concurrent in-flight requests stop sending the rejected shape immediately. It rolls back if the probe shows the markers weren't the cause.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous Review Summaries (4 snapshots, latest commit 10c67e5)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 10c67e5)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 39597d7)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit 04b0faf)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 40234c7)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Reviewed by glm-5.2 · Input: 23.4K · Output: 2.4K · Cached: 195.6K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Live verification against a real Cortex account (claude-sonnet-4-5) showed Cortex honors block-level `cache_control` on system/user/assistant messages (cache activity confirmed via `usage.prompt_tokens_details.cached_tokens`), but **silently ignores** markers on `role:"tool"` messages (accepted, zero cache). Relocation now walks a tool-message marker back to the nearest earlier cacheable message — in agent loops the trailing breakpoint lands on the last assistant/user message, which caches the full conversation prefix except the newest tool result. Markers already sitting on tool-message blocks are stripped so they don't burn the 4-breakpoint budget. Also verified live: assistant messages with text blocks + `tool_calls` + marker cache correctly (the exact mid-loop shape the plugin now emits), and array content on `role:"tool"` never 400s (the fallback retry stays as insurance only). E2E suite updates: swap `claude-3-5-sonnet` (now "unknown model" on current Cortex accounts) for `claude-sonnet-4-5` in live-network tests, and add an agent-loop caching test. Full live run: 40 pass / 0 fail via key-pair JWT. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
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)
packages/opencode/test/altimate/cortex-snowflake-e2e.test.ts (1)
35-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
claude-sonnet-4-5toTOOLCAPABLE_E2E_FIXTURE.
transformSnowflakeBody()keepstoolsonly for allowlisted models, and this file now exercisesclaude-sonnet-4-5in the tool-calling paths. Leaving the fixture onclaude-3-5-sonnet/claude-sonnet-4-6makes the new model look non-tool-capable in these checks.🤖 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 `@packages/opencode/test/altimate/cortex-snowflake-e2e.test.ts` around lines 35 - 38, Add "claude-sonnet-4-5" to the TOOLCAPABLE_E2E_FIXTURE allowlist so tool-calling checks preserve tools for this model.
🤖 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 `@packages/opencode/test/altimate/cortex-snowflake-e2e.test.ts`:
- Around line 35-38: Add "claude-sonnet-4-5" to the TOOLCAPABLE_E2E_FIXTURE
allowlist so tool-calling checks preserve tools for this model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eb22836c-2995-4173-95d1-336e2e4c0048
📒 Files selected for processing (3)
packages/opencode/src/altimate/plugin/snowflake.tspackages/opencode/test/altimate/cortex-snowflake-e2e.test.tspackages/opencode/test/provider/snowflake.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/altimate/plugin/snowflake.ts
…per family
Catalog probed live against a Cortex account (2026-07-20, every ID via
/chat/completions):
Added:
- `claude-sonnet-5` (1M ctx / 64K out) — tool calling and prompt caching
verified live (cache markers honored, 8,965 cached tokens)
- `claude-opus-4-8` (1M / 128K, public preview) — caching verified
(10,369 cached; agent-loop shape 20,385)
- `openai-gpt-5.4` (gpt-5 family defaults), `openai-gpt-5.4-mini` and
`openai-gpt-5.4-nano` (400K / 128K per Snowflake's restrictions table)
Removed (requests hard-fail on Cortex now):
- Deprecated by Snowflake on 2026-07-08: `deepseek-r1`, `mistral-large`,
`llama3.1-405b`, `snowflake-llama-3.3-70b`
- Delisted ("unknown model"): `claude-3-7-sonnet`, `claude-3-5-sonnet`,
`openai-gpt-5-chat`, `llama4-scout`, `snowflake-llama-3.1-405b`,
`mixtral-8x7b`, `gemini-3.1-pro`
Caching verified per family on the live account:
- Claude (all 9 picker models): explicit block markers cache on every model
- OpenAI: automatic caching confirmed on gpt-4.1 / gpt-5 / gpt-5.4 /
gpt-5.4-mini (second request shows 11-13K cached tokens); gpt-5.2 showed
no cache activity in this account/region
- Llama/Mistral: markers tolerated (no 400), no caching, as documented
`claude-fable-5` is recognized by Cortex (maps to claude-fable-5-global)
but unavailable on tested accounts — noted in the map for later.
Catalog tests updated to lock in the verified list, including assertions
that removed models are gone from the picker. Live e2e: 37 pass / 0 fail.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
| const TOOLCAPABLE_FIXTURE: ReadonlySet<string> = new Set([ | ||
| "claude-opus-4-7", "claude-sonnet-4-6", "claude-opus-4-6", "claude-sonnet-4-5", | ||
| "claude-opus-4-8", "claude-opus-4-7", "claude-sonnet-4-6", "claude-opus-4-6", "claude-sonnet-4-5", | ||
| "claude-opus-4-5", "claude-haiku-4-5", "claude-4-sonnet", "claude-3-7-sonnet", |
There was a problem hiding this comment.
SUGGESTION: Fixture retains models this PR removed from the catalog.
TOOLCAPABLE_FIXTURE still lists claude-3-7-sonnet, claude-3-5-sonnet (lines 54-55), and openai-gpt-5-chat (line 57) — all removed from provider.ts in this same PR — and is missing the newly added claude-sonnet-5 and openai-gpt-5.4 / -mini / -nano. The fixture header (lines 48-49) states it "reflects what Snowflake Cortex actually accepts tools for today," which is no longer accurate. The e2e counterpart TOOLCAPABLE_E2E_FIXTURE was fully synced with the new catalog; this unit-test fixture only had claude-opus-4-8 added. Syncing it keeps both fixtures consistent and avoids misleading future maintainers.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 10c67e5918: TOOLCAPABLE_FIXTURE is synced with the refreshed catalog — removed claude-3-7-sonnet/claude-3-5-sonnet/openai-gpt-5-chat, added claude-sonnet-5 and the openai-gpt-5.4 family.
![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: 39597d74de
ℹ️ 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".
| if (response.status === 400 && cacheApplied && typeof body === "string") { | ||
| const stripped = stripCacheControl(body) | ||
| if (stripped !== body) { | ||
| const retry = await fetch(requestInput, { ...init, headers, body: stripped }) |
There was a problem hiding this comment.
When a cache-marked Cortex request returns 400 but the marker-free retry encounters a transport failure or an already-aborted signal, this unguarded await fetch(...) rejects and discards the valid original response. The Snowflake fetch interceptor then surfaces a network exception instead of the original Cortex diagnostic, contradicting the fallback's intended fail-safe behavior; catch retry failures and return response in that case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 84bc9fa0cf: the probe retry is wrapped in try/catch — on a transport failure the eager cooldown is rolled back and the original Cortex 400 (with its diagnostic body) is returned. Covered by a new unit test ("returns the original 400 and keeps caching enabled when the retry fetch throws").
| // Cortex documents block-level cache_control for Claude models, but not | ||
| // for every message role. If an account rejects a cache-marked request, | ||
| // fall back once and stop injecting markers for the rest of the session. | ||
| let cacheControlSupported = true |
There was a problem hiding this comment.
When any request-specific cache marker shape receives a 400 and succeeds after stripping, this flag is set to false in the auth-loader closure. The loader output is retained by the instance-scoped Provider state, so the disable applies to every later Snowflake conversation and Claude model in that workspace—not merely the affected session as documented—and remains until provider state is rebuilt. A single unsupported block shape can therefore silently restore full uncached billing for unrelated sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1912781896: the disable is no longer sticky at any scope — it's a 5-minute cooldown (matching Cortex's ephemeral cache TTL), so a trip affects at most one cold-cache window across the provider instance and self-heals. Comments/docs updated to say "pause" rather than "session".
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 3 finding(s)
- 3 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/altimate/plugin/snowflake.ts (L337-L346)
[🟠 MEDIUM] High Severity Issue: Missing Cleanup on Fetch Exception
If the retry fetch call throws an exception (e.g., a network error or DNS failure), the original response.body stream is never cancelled. This can cause the original connection to stay open, leading to memory leaks and socket exhaustion.
Consider wrapping the retry logic in a try...catch block to ensure cleanup on error.
Suggested change:
if (stripped !== body) {
try {
const retry = await fetch(requestInput, { ...init, headers, body: stripped })
if (retry.ok) {
cacheControlSupported = false
void response.body?.cancel().catch(() => {})
return retry
}
void retry.body?.cancel().catch(() => {})
return response
} catch (error) {
void response.body?.cancel().catch(() => {})
throw error
}
}
2. packages/opencode/src/altimate/plugin/snowflake.ts (L205-L207)
[🔴 HIGH] High Severity Issue: Bypassed Cache Stripping for Non-Claude Models
When cacheControl is true but the model does not include "claude" (e.g., Llama or Mistral), relocateCacheControl returns false early and leaves the original payload untouched. This causes stripParsedCacheControl to be bypassed, leaving cache_control markers intact on non-Claude requests, which Snowflake Cortex rejects with a 400 Bad Request error.
If relocation fails or isn't applicable, we should strip the markers to ensure compatibility.
Suggested change:
let cacheApplied = false
if (cacheControl) {
cacheApplied = relocateCacheControl(parsed)
if (!cacheApplied) stripParsedCacheControl(parsed)
} else {
stripParsedCacheControl(parsed)
}
3. packages/opencode/src/altimate/plugin/snowflake.ts (L46-L48)
[🔵 LOW] Code Quality Issue: Missing Comment for any Type
According to the TypeScript type standards, any type should be avoided. If its use is strictly necessary, an explanatory comment should be provided to clarify why Record<string, any> is needed here (e.g., to easily read and mutate arbitrary JSON properties like content or cache_control later).
Suggested change:
// Using `any` is necessary here to allow flexible reading and modification of
// arbitrary JSON properties (e.g., `content`, `cache_control`, `tool_calls`).
function isRecord(value: unknown): value is Record<string, any> {
return typeof value === "object" && value !== null && !Array.isArray(value)
}
| if (stripped !== body) { | ||
| const retry = await fetch(requestInput, { ...init, headers, body: stripped }) | ||
| if (retry.ok) { | ||
| cacheControlSupported = false | ||
| void response.body?.cancel().catch(() => {}) | ||
| return retry | ||
| } | ||
| void retry.body?.cancel().catch(() => {}) | ||
| return response | ||
| } |
There was a problem hiding this comment.
[🟠 MEDIUM] High Severity Issue: Missing Cleanup on Fetch Exception
If the retry fetch call throws an exception (e.g., a network error or DNS failure), the original response.body stream is never cancelled. This can cause the original connection to stay open, leading to memory leaks and socket exhaustion.
Consider wrapping the retry logic in a try...catch block to ensure cleanup on error.
Suggested change:
| if (stripped !== body) { | |
| const retry = await fetch(requestInput, { ...init, headers, body: stripped }) | |
| if (retry.ok) { | |
| cacheControlSupported = false | |
| void response.body?.cancel().catch(() => {}) | |
| return retry | |
| } | |
| void retry.body?.cancel().catch(() => {}) | |
| return response | |
| } | |
| if (stripped !== body) { | |
| try { | |
| const retry = await fetch(requestInput, { ...init, headers, body: stripped }) | |
| if (retry.ok) { | |
| cacheControlSupported = false | |
| void response.body?.cancel().catch(() => {}) | |
| return retry | |
| } | |
| void retry.body?.cancel().catch(() => {}) | |
| return response | |
| } catch (error) { | |
| void response.body?.cancel().catch(() => {}) | |
| throw error | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in 84bc9fa0cf: the retry fetch is wrapped in try/catch and the original response is returned on failure, so the original connection is handed back to the caller rather than abandoned. (When the retry succeeds, the original body was already being cancelled.)
| let cacheApplied = false | ||
| if (cacheControl) cacheApplied = relocateCacheControl(parsed) | ||
| else stripParsedCacheControl(parsed) |
There was a problem hiding this comment.
[🔴 HIGH] High Severity Issue: Bypassed Cache Stripping for Non-Claude Models
When cacheControl is true but the model does not include "claude" (e.g., Llama or Mistral), relocateCacheControl returns false early and leaves the original payload untouched. This causes stripParsedCacheControl to be bypassed, leaving cache_control markers intact on non-Claude requests, which Snowflake Cortex rejects with a 400 Bad Request error.
If relocation fails or isn't applicable, we should strip the markers to ensure compatibility.
Suggested change:
| let cacheApplied = false | |
| if (cacheControl) cacheApplied = relocateCacheControl(parsed) | |
| else stripParsedCacheControl(parsed) | |
| let cacheApplied = false | |
| if (cacheControl) { | |
| cacheApplied = relocateCacheControl(parsed) | |
| if (!cacheApplied) stripParsedCacheControl(parsed) | |
| } else { | |
| stripParsedCacheControl(parsed) | |
| } |
There was a problem hiding this comment.
This finding is factually incorrect: Snowflake Cortex does NOT 400 on cache markers for non-Claude models — it silently ignores them, per Snowflake's compatibility chart ("ignored for OpenAI and others") and live verification on a real account (llama4-maverick / llama3.1-8b / mistral-large2 / mistral-7b all returned 200 with block-level markers present, cached_tokens 0). Leaving non-Claude payloads untouched is deliberate; stripping them would alter requests for no benefit. No change made — though 84bc9fa0cf does tighten the opposite direction (disabled-mode stripping is now Claude-scoped).
| function isRecord(value: unknown): value is Record<string, any> { | ||
| return typeof value === "object" && value !== null && !Array.isArray(value) | ||
| } |
There was a problem hiding this comment.
[🔵 LOW] Code Quality Issue: Missing Comment for any Type
According to the TypeScript type standards, any type should be avoided. If its use is strictly necessary, an explanatory comment should be provided to clarify why Record<string, any> is needed here (e.g., to easily read and mutate arbitrary JSON properties like content or cache_control later).
Suggested change:
| function isRecord(value: unknown): value is Record<string, any> { | |
| return typeof value === "object" && value !== null && !Array.isArray(value) | |
| } | |
| // Using `any` is necessary here to allow flexible reading and modification of | |
| // arbitrary JSON properties (e.g., `content`, `cache_control`, `tool_calls`). | |
| function isRecord(value: unknown): value is Record<string, any> { | |
| return typeof value === "object" && value !== null && !Array.isArray(value) | |
| } |
There was a problem hiding this comment.
Fixed in 84bc9fa0cf: added a comment on isRecord explaining the deliberate Record<string, any> (arbitrary request-body JSON navigated and mutated without a fixed schema).
Addresses the converged 10-reviewer consensus review (2 majors, 6 minors):
- Sync `TOOLCAPABLE_FIXTURE` with the live catalog (drop `claude-3-7-sonnet`,
`claude-3-5-sonnet`, `openai-gpt-5-chat`; add `claude-sonnet-5` and the
`openai-gpt-5.4` family).
- Replace the dead `DeepSeek R1 Reasoning` e2e block (passed vacuously via a
status guard against the removed model) with a negative test asserting
Cortex rejects the deprecated `deepseek-r1`.
- Tighten the caching gate from `includes("claude")` to
`startsWith("claude-")` — no false positives on user aliases.
- Narrow the interceptor's catch to `SyntaxError`: non-JSON bodies still pass
through, but transform bugs now surface instead of silently disabling both
caching and the `max_tokens` rename.
- Build the 400-retry body via `transformSnowflakeBody(text, toolCapable,
false)` so the fallback request matches the clean disabled-mode shape;
remove the now-unused `stripCacheControl` export.
- Log a warning when caching is sticky-disabled (previously silent loss of
the cache discount). The retry remains unscoped by design — Cortex's
marker-rejection error text is undocumented, and a deterministic
non-marker 400 recurs identically on the retry, never flipping the flag.
- Add `openai-gpt-5.4-mini`/`-nano` to the e2e availability list; retarget
the removed-model tool-strip unit test to `mistral-7b`.
- Document `cacheApplied` in the `transformSnowflakeBody` JSDoc; add tests
for the prefix gate and for a marker attaching to a trailing non-text
block (previously untested path).
Verified: 76 unit tests, live e2e 39 pass / 0 fail (key-pair JWT), typecheck
and upstream marker check clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Multi-model consensus code review completed (Claude + GPT 5.4, Gemini 3.1 Pro, Kimi K2.6, Grok 4.20, MiniMax M2.7, GLM-5.1, Qwen 3.6 Plus, MiMo V2.5 Pro, DeepSeek V4 Pro; 1 convergence round). Verdict was REQUEST CHANGES — 0 critical, 2 majors (both test hygiene), 6 minors, all now addressed in
The core caching implementation passed all ten reviewers. Follow-up notes (non-blocking, tracked in the review artifacts): Post-fix verification: 76 unit / 0 fail · live e2e 39 / 0 fail (key-pair JWT against a real account) · typecheck + marker check clean. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
The 400-fallback previously disabled marker injection for the REST of the session — a false trip (transient 400 whose stripped retry happened to succeed) would silently bill every subsequent request at the uncached rate, reintroducing the exact cost blowout this fix exists to prevent. Replace the session-permanent flag with `cacheDisabledUntil`, a cooldown deadline set to `CACHE_DISABLE_COOLDOWN_MS` (5 minutes — Cortex's ephemeral cache TTL, so by expiry the cache would be cold anyway). This bounds both failure modes: - False trip: caching self-heals after <= 5 minutes instead of never. - Account that genuinely rejects markers (hypothetical — every live probe accepted them): one wasted marked-attempt + retry per cooldown period. Cooldown test uses `setSystemTime` to verify markers resume after expiry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Follow-up to the review batch: the 400-fallback is no longer session-permanent. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
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)
packages/opencode/src/altimate/plugin/snowflake.ts (1)
351-365: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnsure cleanup of original response on retry fetch exception.
If the retry
fetchthrows an exception (e.g., a network error or DNS failure), the originalresponse.bodystream is never cancelled. This leaves the original connection open, which can lead to memory leaks and socket exhaustion. Wrap the retryfetchin atry...catchblock to guarantee cleanup on error.🐛 Proposed fix
- const stripped = transformSnowflakeBody(text, toolCapable, false).body - if (stripped !== body) { - const retry = await fetch(requestInput, { ...init, headers, body: stripped }) - if (retry.ok) { - cacheDisabledUntil = Date.now() + CACHE_DISABLE_COOLDOWN_MS - log.warn("Cortex rejected cache-marked request; pausing prompt caching", { - status: response.status, - cooldownMs: CACHE_DISABLE_COOLDOWN_MS, - }) - void response.body?.cancel().catch(() => {}) - return retry - } - void retry.body?.cancel().catch(() => {}) - return response - } + const stripped = transformSnowflakeBody(text, toolCapable, false).body + if (stripped !== body) { + let retry: Response + try { + retry = await fetch(requestInput, { ...init, headers, body: stripped }) + } catch (error) { + void response.body?.cancel().catch(() => {}) + throw error + } + + if (retry.ok) { + cacheDisabledUntil = Date.now() + CACHE_DISABLE_COOLDOWN_MS + log.warn("Cortex rejected cache-marked request; pausing prompt caching", { + status: response.status, + cooldownMs: CACHE_DISABLE_COOLDOWN_MS, + }) + void response.body?.cancel().catch(() => {}) + return retry + } + void retry.body?.cancel().catch(() => {}) + return response + }🤖 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 `@packages/opencode/src/altimate/plugin/snowflake.ts` around lines 351 - 365, Update the retry fetch flow in the transformSnowflakeBody handling block to wrap fetch(requestInput, { ...init, headers, body: stripped }) in try/catch. If the retry throws, cancel the original response.body stream before propagating the exception, while preserving the existing successful retry and failed retry response behavior.
🤖 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 `@packages/opencode/src/altimate/plugin/snowflake.ts`:
- Around line 351-365: Update the retry fetch flow in the transformSnowflakeBody
handling block to wrap fetch(requestInput, { ...init, headers, body: stripped })
in try/catch. If the retry throws, cancel the original response.body stream
before propagating the exception, while preserving the existing successful retry
and failed retry response behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 34058fae-52a0-4845-84a5-d5cdd7fb75df
📒 Files selected for processing (3)
CHANGELOG.mdpackages/opencode/src/altimate/plugin/snowflake.tspackages/opencode/test/provider/snowflake.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- packages/opencode/test/provider/snowflake.test.ts
- Close the concurrency window (kilo-code-bot, cubic): the cooldown is now set eagerly when a cache-marked 400 is detected, before the probe retry runs, so concurrent in-flight requests stop sending the rejected shape immediately; it is rolled back if the retry shows markers weren't the cause (retry also fails) or the probe hits a transport error. - Preserve the original 400 when the probe retry fetch throws (Codex, OpenCodeReview): the unguarded `await fetch` could replace Cortex's diagnostic with a network exception; the retry is now wrapped and the original response returned on transport failure. - Scope disabled-mode stripping to Claude models (cubic): non-Claude payloads pass through untouched during a cooldown instead of having their (Cortex-ignored) markers removed. - Consume the first response body in the caching e2e test (CodeRabbit); document the deliberate `any` in `isRecord` (OpenCodeReview). New tests: disabled-mode non-Claude passthrough; retry-fetch-throws returns the original 400 and rolls back the cooldown. 78 unit / 0 fail; live e2e 39 / 0 fail; typecheck + marker check clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Review-bot findings addressed in
Verified after changes: 78 unit / 0 fail · live e2e 39 / 0 fail (two runs, key-pair JWT) · typecheck + marker check clean. |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/plugin/snowflake.ts">
<violation number="1" location="packages/opencode/src/altimate/plugin/snowflake.ts:364">
P2: Concurrent 400 probes can clear the cooldown after another probe succeeds, so subsequent requests resume sending the rejected cache-marker shape. Associate restoration with this probe's deadline (or serialize probes) so a stale failed retry cannot overwrite a newer in-flight pause.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // sending the rejected shape while this probe retry runs; | ||
| // restored below if the retry shows markers weren't the cause. | ||
| const previousDisabledUntil = cacheDisabledUntil | ||
| cacheDisabledUntil = Date.now() + CACHE_DISABLE_COOLDOWN_MS |
There was a problem hiding this comment.
P2: Concurrent 400 probes can clear the cooldown after another probe succeeds, so subsequent requests resume sending the rejected cache-marker shape. Associate restoration with this probe's deadline (or serialize probes) so a stale failed retry cannot overwrite a newer in-flight pause.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/plugin/snowflake.ts, line 364:
<comment>Concurrent 400 probes can clear the cooldown after another probe succeeds, so subsequent requests resume sending the rejected cache-marker shape. Associate restoration with this probe's deadline (or serialize probes) so a stale failed retry cannot overwrite a newer in-flight pause.</comment>
<file context>
@@ -350,16 +357,29 @@ export async function SnowflakeCortexAuthPlugin(_input: PluginInput): Promise<Ho
+ // sending the rejected shape while this probe retry runs;
+ // restored below if the retry shows markers weren't the cause.
+ const previousDisabledUntil = cacheDisabledUntil
+ cacheDisabledUntil = Date.now() + CACHE_DISABLE_COOLDOWN_MS
+ let retry: Response
+ try {
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84bc9fa0cf
ℹ️ 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".
| retry = await fetch(requestInput, { ...init, headers, body: stripped }) | ||
| } catch { | ||
| // Transport failure on the probe — surface the original 400. | ||
| cacheDisabledUntil = previousDisabledUntil |
There was a problem hiding this comment.
When two cache-marked requests probe concurrently, one stripped retry can succeed and establish a confirmed cooldown after the other request captured previousDisabledUntil; if the latter retry then throws—or reaches the equivalent restoration after a non-OK response—this unconditional assignment restores the stale timestamp and erases the successful request's cooldown. Subsequent requests resume the rejected marker shape and incur another 400 plus retry, so only roll back if cacheDisabledUntil still equals the value installed by this probe.
Useful? React with 👍 / 👎.
2 participants
PINEAPPLE
Summary
Fixes #1009 — Snowflake Cortex users on Claude models were getting zero prompt caching (
cache_read_input/cache_write_inputNULL inTOKENS_GRANULAR), billing every request's full ~300K-token input at the undiscounted rate.Root cause: Cortex's Chat Completions API only honors caching for Claude models via block-level markers —
messages[].content[].cache_control: {"type":"ephemeral"}(max 4 breakpoints; docs). Our pipeline sets the markers asproviderOptions.openaiCompatible.cache_control, but@ai-sdk/openai-compatible@2.0.41serializes them as message-level fields — onsystemmessages, on single-text-partusermessages (collapsed to string content), ontoolmessages — and into assistanttool_callsentries. Cortex silently ignores all of those. This has been the case since the provider shipped (#349); the cost explosion in the issue coincides with Opus 4.7's 1M context making uncached prefixes much larger.Fix (in the registered plugin
packages/opencode/src/altimate/plugin/snowflake.ts, alongside the existingmax_tokensbody rewrite):relocateCacheControlmoves message-level markers into content blocks (string content wrapped in a text block, array content attaches to the last non-empty block), strips stray markers fromtool_callsentries, and enforces the 4-breakpoint limit keeping the trailing markers.system/user/assistantmessages but silently ignores them onrole:"tool"(accepted, zero cache activity). A marker destined for a tool message therefore walks back to the nearest earlier cacheable message — in agent loops the trailing breakpoint lands on the last assistant/user message, caching the whole conversation prefix except the newest tool result. Useless markers on tool-message blocks are stripped so they can't burn the breakpoint budget.Test Plan
Live verification (account
EJJKBKO-FUB20041, key-pair JWT,claude-sonnet-4-5):cached_tokens: 12466on both write and read requests ✅cached_tokens: 7563✅ · Assistant-block marker:cached_tokens: 8273✅cached_tokens: 0→ drove the walk-back designtool_calls+ marker):cached_tokens: 18474✅Automated:
ProviderTransform.applyCachingthrough the real@ai-sdk/openai-compatibleserialization (captured fetch) and thentransformSnowflakeBody, asserting final block placement — guards against SDK upgrades moving the serialization.bun test test/provider/snowflake.test.ts→ 75 pass / 0 fail; typecheck clean; upstream marker check clean.claude-3-5-sonnet(nowunknown modelon current Cortex accounts) toclaude-sonnet-4-5.test/plugin/snowflake-cortex.test.ts("single-flight refresh") fails onmaintoo — it tests the unregistered OAuth plugin variant, untouched here.Model catalog refresh + per-family caching verification (follow-up commits)
Probed every candidate model ID live against the Cortex account (2026-07-20):
Added to the picker:
claude-sonnet-5(1M/64K — tools + caching verified live),claude-opus-4-8(1M/128K — caching verified: 10,369 cached; agent-loop shape 20,385),openai-gpt-5.4,openai-gpt-5.4-mini,openai-gpt-5.4-nano.Removed: Snowflake-deprecated on 2026-07-08 (
deepseek-r1,mistral-large,llama3.1-405b,snowflake-llama-3.3-70b) and delisted/"unknown model" (claude-3-7-sonnet,claude-3-5-sonnet,openai-gpt-5-chat,llama4-scout,snowflake-llama-3.1-405b,mixtral-8x7b,gemini-3.1-pro). Requests to all of these hard-fail on Cortex today. (claude-fable-5is recognized but gated — noted in the map.)Caching verified per family (live):
Catalog tests now lock in the verified list and assert removed models are gone. Final runs: 117 unit / 0 fail, live e2e 37 / 0 fail, typecheck clean.
Checklist
Summary by CodeRabbit
#1009) and refreshed Snowflake model availability expectations.