feat(routing): cache weight via length-based estimate - #2104
Merged
Merged
Conversation
Re-applies #2095 (cache-support routing weight) using a chars/4 length-based prompt estimate instead of gpt-tokenizer. Replaces all in-flight tokenizer usage in the gateway with the same cheap heuristic, since the encoder caused a measured throughput regression. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
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:
WalkthroughThis pull request removes the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Pull request overview
This PR reintroduces cache-aware routing for large prompts while removing gpt-tokenizer from the gateway hot path by switching token counting to a chars/4 heuristic, improving throughput at the cost of estimation precision.
Changes:
- Add cache-support weighting to provider selection when estimated prompt tokens exceed a 5k threshold, and expose
cacheSupportedin routing score metadata. - Replace
gpt-tokenizerusage across gateway routing/cost/usage estimation paths with length-based token estimates. - Remove
gpt-tokenizerfromapps/gatewaydependencies and update tests/docs accordingly.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Drops gpt-tokenizer and updates lockfile graph accordingly. |
| packages/actions/src/get-cheapest-from-available-providers.ts | Adds cache-support scoring weight gated by estimated prompt tokens; surfaces cacheSupported in provider scores. |
| packages/actions/src/models.spec.ts | Adds unit tests covering cache-support weighting behavior above/below threshold. |
| apps/gateway/src/chat/chat.ts | Computes a single prompt-token estimate per request and plumbs it into routing; removes tokenizer calls in streaming/non-streaming usage estimation. |
| apps/gateway/src/chat/tools/tokenizer.ts | Replaces chat token counting with a chars/4 length-based estimator. |
| apps/gateway/src/chat/tools/estimate-tokens.ts | Switches completion-token fallback to length-based estimation. |
| apps/gateway/src/chat/tools/types.ts | Removes tokenizer-specific constants/types tied to gpt-tokenizer. |
| apps/gateway/src/lib/costs.ts | Replaces cost-time tokenization with length-based estimates for missing usage. |
| apps/gateway/src/lib/prompt-tokens.spec.ts | Updates tests to reflect length-based estimation semantics (including empty-input behavior). |
| apps/gateway/package.json | Removes gpt-tokenizer dependency from the gateway app. |
| apps/docs/content/features/routing.mdx | Documents cache-support weighting behavior for large prompts. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
apps/gateway/src/lib/costs.ts:193
calculateCostsnow uses a length-based estimator that can return0for empty prompts/messages, but the early-return checkif (!calculatedPromptTokens)treats0the same asnull/undefinedand returns all-null costs. This can incorrectly suppress costs for legitimately empty prompts (should be $0 with 0 tokens) and can also affect any caller that passespromptTokens = 0intentionally. Consider switching these truthiness checks to explicit nullish checks (e.g.,calculatedPromptTokens == null) and similarly usingpromptTokens == null/completionTokens == nullin the estimation gate so0is treated as a real value.
if ((!promptTokens || !completionTokens) && fullOutput) {
// We're going to estimate at least some of the tokens
isEstimated = true;
// Calculate prompt tokens using a cheap length-based estimate.
// Accuracy is intentionally traded for throughput so we never run
// gpt-tokenizer on the gateway hot path.
if (!promptTokens && fullOutput) {
if (fullOutput.messages) {
calculatedPromptTokens = encodeChatMessages(fullOutput.messages);
} else if (fullOutput.prompt) {
calculatedPromptTokens = estimateTokensFromContent(
JSON.stringify(fullOutput.prompt),
);
}
}
// Calculate completion tokens
if (!completionTokens && fullOutput) {
let completionText = "";
// Include main completion content
if (fullOutput.completion) {
completionText += fullOutput.completion;
}
// Include tool results if available
if (fullOutput.toolResults && Array.isArray(fullOutput.toolResults)) {
for (const toolResult of fullOutput.toolResults) {
if (toolResult.function?.name) {
completionText += toolResult.function.name;
}
if (toolResult.function?.arguments) {
completionText += JSON.stringify(toolResult.function.arguments);
}
}
}
if (completionText) {
calculatedCompletionTokens = estimateTokensFromContent(completionText);
}
}
}
// If we don't have prompt tokens, we can't calculate any costs
if (!calculatedPromptTokens) {
return {
inputCost: null,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| **Cache Support for Large Prompts**: | ||
|
|
||
| When the estimated prompt is at least 5,000 tokens, an additional 20% weight is added to the score for whether each provider supports prompt caching (advertised via a cached input price). Providers that support caching score better than ones that do not, since caching can substantially reduce the cost of large or repeated prompts. Below the 5k threshold, this weight is dropped entirely — caching has little impact on small prompts, so cache support is ignored. The selected provider's cache support is exposed as `cacheSupported` on the routing metadata. |
There was a problem hiding this comment.
The docs say cache support is exposed as cacheSupported "on the routing metadata", but in code it’s added on each entry of metadata.providerScores (and not as a top-level RoutingMetadata.cacheSupported). Please clarify the exact field path (e.g. routingMetadata.providerScores[].cacheSupported) to avoid consumers looking for a non-existent top-level property.
Suggested change
| When the estimated prompt is at least 5,000 tokens, an additional 20% weight is added to the score for whether each provider supports prompt caching (advertised via a cached input price). Providers that support caching score better than ones that do not, since caching can substantially reduce the cost of large or repeated prompts. Below the 5k threshold, this weight is dropped entirely — caching has little impact on small prompts, so cache support is ignored. The selected provider's cache support is exposed as `cacheSupported` on the routing metadata. | |
| When the estimated prompt is at least 5,000 tokens, an additional 20% weight is added to the score for whether each provider supports prompt caching (advertised via a cached input price). Providers that support caching score better than ones that do not, since caching can substantially reduce the cost of large or repeated prompts. Below the 5k threshold, this weight is dropped entirely — caching has little impact on small prompts, so cache support is ignored. Cache support is exposed on each provider score entry in the routing metadata as `routingMetadata.providerScores[].cacheSupported`. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
apps/gateway/src/chat/tools/tokenizer.ts (1)
17-28: Optional: consolidate the chars/4 heuristic in one place.
estimateTokensFromLengthhere andestimateTokensFromContentinestimate-tokens-from-content.tsimplement the sameMath.max(1, Math.round(length / 4))logic with the sameCHARS_PER_TOKENfactor (currently hardcoded as/ 4there). If the heuristic ever changes (e.g. different ratio, BPE-aware adjustment), both call sites must be kept in sync. Consider havingestimateTokensFromContentdelegate toestimateTokensFromLength(content.length)so the constant lives in a single module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/tokenizer.ts` around lines 17 - 28, Consolidate the chars-per-token heuristic by having estimateTokensFromContent delegate to estimateTokensFromLength so the CHARS_PER_TOKEN constant is maintained in one place: remove the duplicate hardcoded `/ 4` logic from estimateTokensFromContent and replace its implementation with a call to estimateTokensFromLength(content.length), ensuring estimateTokensFromLength (and its CHARS_PER_TOKEN constant) remain the single source of truth for the heuristic.packages/actions/src/get-cheapest-from-available-providers.ts (3)
586-597:selectByPriceOnlyfallback doesn't surfacecacheSupported.When
metricsMapis missing/empty (e.g., metrics layer down), routing falls through toselectByPriceOnly, and the resultingproviderScoresentries omit the newcacheSupportedfield even for large prompts. This isn't a correctness bug for selection (price-only intentionally ignores cache), but it makes the routing-metadata schema inconsistent and hides whether the selected provider supports caching from downstream consumers (logs, dashboard, thecacheSupportedsignal documented inrouting.mdx).Consider populating
cacheSupportedhere as well so the field is always present:♻️ Proposed fix
for (const provider of stableProviders) { const providerInfo = modelWithPricing.providers.find( (p) => p.providerId === provider.providerId && p.region === provider.region, ); const totalPrice = getProviderSelectionPrice(providerInfo, videoPricing); // Apply provider priority: lower priority = effectively higher price const providerDef = getProviderDefinition(provider.providerId); const priority = providerDef?.priority ?? 1; const effectivePrice = priority > 0 ? totalPrice / priority : totalPrice; providerPrices.push({ providerId: provider.providerId, region: provider.region, price: totalPrice, effectivePrice, priority, + cacheSupported: providerSupportsCaching(providerInfo), });(plus a corresponding field on the local array type and on the mapped
providerScoresentry).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/actions/src/get-cheapest-from-available-providers.ts` around lines 586 - 597, The RoutingMetadata produced in get-cheapest-from-available-providers.ts (within the select-by-price-only fallback) omits the cacheSupported flag on providerScores entries; update the local providerPrices entry type and the mapped providerScores construction (the array built from providerPrices.map(...) that feeds RoutingMetadata) to include cacheSupported (set appropriately from each provider's capabilities, e.g., provider.cacheSupported or a boolean derived from provider info) and ensure the RoutingMetadata type/shape includes cacheSupported so downstream consumers always see the field even when metricsMap is missing.
165-199:providerSupportsCachingtriggers off anycachedInputPrice, including a 0 placeholder.The check
providerInfo.cachedInputPrice !== undefinedreturnstruewhenever the field exists, even if it's0or equal toinputPrice. In practicecachedInputPrice: 0would mean "free cached input", which is still cache support — fine. But if a provider mapping ever setscachedInputPriceequal to or higher thaninputPriceas a placeholder (or sets it on a region but not at the top level for matching), it will get the cache-routing bonus without any actual cost benefit. Worth tightening the check tocachedInputPricestrictly less thaninputPrice(and similarly for tiers/regions) if you want the routing weight to track an actual savings rather than the mere presence of the field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/actions/src/get-cheapest-from-available-providers.ts` around lines 165 - 199, The function providerSupportsCaching currently treats any present cachedInputPrice (including 0 or placeholders >= inputPrice) as support; update providerSupportsCaching to accept an inputPrice parameter and change all checks so they only return true when cachedInputPrice is strictly less than inputPrice (i.e., providerInfo.cachedInputPrice < inputPrice, tier.cachedInputPrice < inputPrice, region.cachedInputPrice < inputPrice, and region.pricingTiers items similarly) so the routing bonus only applies when there is an actual cost savings; keep the same nested checks and early-return logic but compare values rather than just presence.
304-307: Threshold compares estimated prompt tokens to a real-token threshold.
promptTokensflowing in here comes from the chars/4 heuristic inencodeChatMessages(per PR objectives). For English prose, chars/4 ≈ true gpt-tokenizer count, so the 5k threshold is roughly preserved. For code-heavy or non-Latin prompts (CJK, etc.), the heuristic systematically under-counts tokens, so the cache-support weight will under-trigger on exactly the prompts where caching would help most. This is a known trade-off mentioned in the PR description, but worth a brief code comment so future readers understand the threshold is approximate, and consider biasing the threshold downward (e.g. 4000) to compensate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/actions/src/get-cheapest-from-available-providers.ts` around lines 304 - 307, The comparison using promptTokens (from encodeChatMessages' chars/4 heuristic) to CACHE_PROMPT_TOKEN_THRESHOLD can under-count tokens for code-heavy or non-Latin input, so update the comment near the cacheSupportRelevant calculation to note the heuristic is approximate, may under-count for CJK/code, and recommend a conservative bias (e.g. lower the effective threshold to ~4000) or revisit CACHE_PROMPT_TOKEN_THRESHOLD; reference encodeChatMessages and CACHE_PROMPT_TOKEN_THRESHOLD and the cacheSupportRelevant variable so maintainers know where to adjust the threshold or heuristic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 1090-1100: routingPromptTokens is computed before
applyRedactions() can change messages, causing routing (cache-weight threshold
and auto-routing) to be based on pre-redaction content; move or recompute the
token estimate after redactions so selection uses the final payload.
Specifically, after applyRedactions() finishes (the code path that may mutate
messages/tools), call encodeChatMessages(messages) and re-measure tools
(JSON.stringify(tools).length / 4) to set routingPromptTokens, replacing the
earlier pre-redaction calculation in the same scope where routing decisions are
made.
- Around line 6887-6890: calculatedReasoningTokens is computed as a fallback but
never propagated; update downstream logic to use calculatedReasoningTokens
wherever reasoningTokens is currently referenced so the fallback is honoured:
pass calculatedReasoningTokens into calculateCosts(...), include it in
total-token math and any usage chunk computations (e.g. usageChunks/usage
totals), and ensure transformResponseToOpenai(...) receives the
calculatedReasoningTokens for response metadata. Locate uses of reasoningTokens
in the same scope and replace or augment them to prefer
calculatedReasoningTokens when reasoningTokens is falsy so billing and metadata
reflect the estimated reasoning usage (also apply the same change near the other
occurrence around the 8940–8942 region).
In `@apps/gateway/src/chat/tools/estimate-tokens.ts`:
- Around line 26-30: The code is unnecessarily calling JSON.stringify on content
before token estimation, which wraps the string in quotes and inflates the token
count; in the branch that checks if (!completionTokens && content) pass content
directly to estimateTokensFromContent (replace JSON.stringify(content) with
content) so calculatedCompletionTokens is computed from the raw string, matching
the behavior used elsewhere (see costs.ts usage) and keeping estimates
consistent.
In `@apps/gateway/src/lib/costs.ts`:
- Around line 156-160: The token estimation inflates prompt length because
fullOutput.prompt (typed as string) is being wrapped with JSON.stringify before
calling estimateTokensFromContent; instead, call estimateTokensFromContent with
the raw fullOutput.prompt value (no JSON.stringify) so it matches the raw-string
path used for completionText and yields consistent token estimates; update the
branch in costs.ts where fullOutput.prompt is handled to pass fullOutput.prompt
directly to estimateTokensFromContent.
In `@packages/actions/src/models.spec.ts`:
- Around line 948-963: The test relies on implicit provider priority defaults
(openai/deepseek) which makes it brittle; update the test in
packages/actions/src/models.spec.ts that calls getCheapestFromAvailableProviders
with cacheTestModel to explicitly assert cache-related behavior instead of equal
scores—e.g., check result.metadata.providerScores entries for a cacheSupported
(or equivalent) flag for each provider and assert that cacheSupported is
true/false as expected for cache-supporting providers, or run two calls (one
with providers modified to disable cache support) and assert the score delta
between runs; reference getCheapestFromAvailableProviders, cacheTestModel, and
the providerScores/openai and deepseek entries to locate and change the
assertions.
---
Nitpick comments:
In `@apps/gateway/src/chat/tools/tokenizer.ts`:
- Around line 17-28: Consolidate the chars-per-token heuristic by having
estimateTokensFromContent delegate to estimateTokensFromLength so the
CHARS_PER_TOKEN constant is maintained in one place: remove the duplicate
hardcoded `/ 4` logic from estimateTokensFromContent and replace its
implementation with a call to estimateTokensFromLength(content.length), ensuring
estimateTokensFromLength (and its CHARS_PER_TOKEN constant) remain the single
source of truth for the heuristic.
In `@packages/actions/src/get-cheapest-from-available-providers.ts`:
- Around line 586-597: The RoutingMetadata produced in
get-cheapest-from-available-providers.ts (within the select-by-price-only
fallback) omits the cacheSupported flag on providerScores entries; update the
local providerPrices entry type and the mapped providerScores construction (the
array built from providerPrices.map(...) that feeds RoutingMetadata) to include
cacheSupported (set appropriately from each provider's capabilities, e.g.,
provider.cacheSupported or a boolean derived from provider info) and ensure the
RoutingMetadata type/shape includes cacheSupported so downstream consumers
always see the field even when metricsMap is missing.
- Around line 165-199: The function providerSupportsCaching currently treats any
present cachedInputPrice (including 0 or placeholders >= inputPrice) as support;
update providerSupportsCaching to accept an inputPrice parameter and change all
checks so they only return true when cachedInputPrice is strictly less than
inputPrice (i.e., providerInfo.cachedInputPrice < inputPrice,
tier.cachedInputPrice < inputPrice, region.cachedInputPrice < inputPrice, and
region.pricingTiers items similarly) so the routing bonus only applies when
there is an actual cost savings; keep the same nested checks and early-return
logic but compare values rather than just presence.
- Around line 304-307: The comparison using promptTokens (from
encodeChatMessages' chars/4 heuristic) to CACHE_PROMPT_TOKEN_THRESHOLD can
under-count tokens for code-heavy or non-Latin input, so update the comment near
the cacheSupportRelevant calculation to note the heuristic is approximate, may
under-count for CJK/code, and recommend a conservative bias (e.g. lower the
effective threshold to ~4000) or revisit CACHE_PROMPT_TOKEN_THRESHOLD; reference
encodeChatMessages and CACHE_PROMPT_TOKEN_THRESHOLD and the cacheSupportRelevant
variable so maintainers know where to adjust the threshold or heuristic.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 26804f55-5c79-4f0c-b647-d3d353443e20
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/docs/content/features/routing.mdxapps/gateway/package.jsonapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/estimate-tokens.tsapps/gateway/src/chat/tools/tokenizer.tsapps/gateway/src/chat/tools/types.tsapps/gateway/src/lib/costs.tsapps/gateway/src/lib/prompt-tokens.spec.tspackages/actions/src/get-cheapest-from-available-providers.tspackages/actions/src/models.spec.ts
💤 Files with no reviewable changes (2)
- apps/gateway/package.json
- apps/gateway/src/chat/tools/types.ts
Comment on lines
6887
to
+6890
| let calculatedReasoningTokens = reasoningTokens; | ||
| if (!reasoningTokens && fullReasoningContent) { | ||
| try { | ||
| calculatedReasoningTokens = encode(fullReasoningContent).length; | ||
| } catch (error) { | ||
| // Fallback to simple estimation if encoding fails | ||
| logger.error( | ||
| "Failed to encode reasoning text in streaming", | ||
| error instanceof Error ? error : new Error(String(error)), | ||
| ); | ||
| calculatedReasoningTokens = | ||
| estimateTokensFromContent(fullReasoningContent); | ||
| } | ||
| calculatedReasoningTokens = | ||
| estimateTokensFromContent(fullReasoningContent); |
Contributor
There was a problem hiding this comment.
Thread the estimated reasoning tokens through the downstream accounting.
calculatedReasoningTokens is populated here, but the later calculateCosts(...), total-token math, usage chunks, and transformResponseToOpenai(...) still use raw reasoningTokens. When upstream omits reasoning usage, the new fallback only affects some log fields while response metadata and billing-related totals still undercount reasoning.
Also applies to: 8940-8942
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 6887 - 6890,
calculatedReasoningTokens is computed as a fallback but never propagated; update
downstream logic to use calculatedReasoningTokens wherever reasoningTokens is
currently referenced so the fallback is honoured: pass calculatedReasoningTokens
into calculateCosts(...), include it in total-token math and any usage chunk
computations (e.g. usageChunks/usage totals), and ensure
transformResponseToOpenai(...) receives the calculatedReasoningTokens for
response metadata. Locate uses of reasoningTokens in the same scope and replace
or augment them to prefer calculatedReasoningTokens when reasoningTokens is
falsy so billing and metadata reflect the estimated reasoning usage (also apply
the same change near the other occurrence around the 8940–8942 region).
Comment on lines
156
to
160
| } else if (fullOutput.prompt) { | ||
| // For text prompt | ||
| try { | ||
| calculatedPromptTokens = encode( | ||
| JSON.stringify(fullOutput.prompt), | ||
| ).length; | ||
| } catch (error) { | ||
| // If encoding fails, leave as null | ||
| logger.error(`Failed to encode prompt text: ${error}`); | ||
| } | ||
| calculatedPromptTokens = estimateTokensFromContent( | ||
| JSON.stringify(fullOutput.prompt), | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Drop the JSON.stringify on fullOutput.prompt.
fullOutput.prompt is typed as string (line 93). JSON.stringify wraps it in quotes and escapes specials, inflating the chars/4 estimate by at least 2 characters compared to the consistent raw-string path used at line 185 for completionText. Pass the prompt directly.
♻️ Proposed fix
} else if (fullOutput.prompt) {
- calculatedPromptTokens = estimateTokensFromContent(
- JSON.stringify(fullOutput.prompt),
- );
+ calculatedPromptTokens = estimateTokensFromContent(fullOutput.prompt);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/lib/costs.ts` around lines 156 - 160, The token estimation
inflates prompt length because fullOutput.prompt (typed as string) is being
wrapped with JSON.stringify before calling estimateTokensFromContent; instead,
call estimateTokensFromContent with the raw fullOutput.prompt value (no
JSON.stringify) so it matches the raw-string path used for completionText and
yields consistent token estimates; update the branch in costs.ts where
fullOutput.prompt is handled to pass fullOutput.prompt directly to
estimateTokensFromContent.
Comment on lines
+948
to
+963
| it("does not factor cache support when prompt is below the threshold", () => { | ||
| const result = getCheapestFromAvailableProviders( | ||
| cacheTestModel.providers, | ||
| cacheTestModel, | ||
| { metricsMap: equalMetrics, promptTokens: 1000 }, | ||
| ); | ||
|
|
||
| const openai = result?.metadata.providerScores.find( | ||
| (p) => p.providerId === "openai", | ||
| ); | ||
| const deepseek = result?.metadata.providerScores.find( | ||
| (p) => p.providerId === "deepseek", | ||
| ); | ||
|
|
||
| expect(openai?.score).toBe(deepseek?.score); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify openai and deepseek share the same priority in the provider catalog
rg -nP --type=ts -C2 '(providerId|id)\s*:\s*"(openai|deepseek)"\b' packages/models/src
rg -nP --type=ts -C5 '\bpriority\s*:' packages/models/src | rg -nP -B5 '"(openai|deepseek)"'Repository: theopenco/llmgateway
Length of output: 46
🏁 Script executed:
# First, explore the repo structure to understand where provider definitions are
find . -type f -name "*.ts" | grep -E "(provider|model)" | head -20Repository: theopenco/llmgateway
Length of output: 1111
🏁 Script executed:
# Look at the test file itself to understand cacheTestModel and the function being tested
find . -type f -name "models.spec.ts" | head -5Repository: theopenco/llmgateway
Length of output: 143
🏁 Script executed:
# Find getCheapestFromAvailableProviders function location
rg -l "getCheapestFromAvailableProviders" --type=ts | head -10Repository: theopenco/llmgateway
Length of output: 226
🏁 Script executed:
# Read the test file around lines 948-963 to see cacheTestModel setup and test context
sed -n '900,1000p' packages/actions/src/models.spec.tsRepository: theopenco/llmgateway
Length of output: 2684
🏁 Script executed:
# Find the getCheapestFromAvailableProviders implementation and look at priorityPenalty logic
sed -n '490,520p' packages/actions/src/get-cheapest-from-available-providers.tsRepository: theopenco/llmgateway
Length of output: 1315
🏁 Script executed:
# Search for provider definitions/priorities
rg -n "priority" packages/actions/src/models.spec.ts | head -20Repository: theopenco/llmgateway
Length of output: 376
🏁 Script executed:
# Search for provider registry or definitions
rg -n "getProviderDefinition\|provider.*priority" --type=ts | head -20Repository: theopenco/llmgateway
Length of output: 46
🏁 Script executed:
# Find where provider definitions are stored and their priorities
rg -n "getProviderDefinition\|export.*provider.*definition" --type=ts -A5 | head -40Repository: theopenco/llmgateway
Length of output: 46
🏁 Script executed:
# Look for provider definitions or a provider registry
find . -type f -name "*.ts" | xargs grep -l "getProviderDefinition" | head -10Repository: theopenco/llmgateway
Length of output: 400
🏁 Script executed:
# Check if there's a provider catalog or registry with priority values
rg -n "openai.*priority\|deepseek.*priority" --type=ts | head -20Repository: theopenco/llmgateway
Length of output: 46
🏁 Script executed:
# Check the provider definitions in packages/models/src
cat packages/models/src/providers.ts | head -150Repository: theopenco/llmgateway
Length of output: 4078
🏁 Script executed:
# Look for getProviderDefinition function implementation
rg -n "getProviderDefinition" packages/models/src --type=ts -A10 | head -50Repository: theopenco/llmgateway
Length of output: 2090
🏁 Script executed:
# Check the provider.ts file for priority definitions
cat packages/models/src/provider.tsRepository: theopenco/llmgateway
Length of output: 3907
🏁 Script executed:
# Search for openai and deepseek provider definitions with priority info
rg -n "id.*openai\|id.*deepseek" packages/models/src/providers.ts -A15 | head -60Repository: theopenco/llmgateway
Length of output: 46
🏁 Script executed:
# Get all provider definitions from providers.ts to see openai and deepseek entries
cat packages/models/src/providers.ts | grep -A10 "id.*:.*openai\|id.*:.*deepseek"Repository: theopenco/llmgateway
Length of output: 611
🏁 Script executed:
# Alternative: extract the entire providers array
sed -n '/export const providers = \[/,/^\]/p' packages/models/src/providers.ts | head -300Repository: theopenco/llmgateway
Length of output: 6640
🏁 Script executed:
# Verify the exact priority values for openai and deepseek by extracting their full definitions
sed -n '/id.*:.*"openai"/,/^[[:space:]]*},/p' packages/models/src/providers.ts
sed -n '/id.*:.*"deepseek"/,/^[[:space:]]*},/p' packages/models/src/providers.tsRepository: theopenco/llmgateway
Length of output: 516
🏁 Script executed:
# Get openai provider definition with more context
rg -n "id.*:.*\"openai\"" packages/models/src/providers.ts -A20Repository: theopenco/llmgateway
Length of output: 668
🏁 Script executed:
# Get deepseek provider definition with more context
rg -n "id.*:.*\"deepseek\"" packages/models/src/providers.ts -A20Repository: theopenco/llmgateway
Length of output: 604
The test assertion is correct as written. Both openai and deepseek provider definitions in packages/models/src/providers.ts (lines 72–86 and 249–263 respectively) lack an explicit priority field, so both default to priority = 1. With equal metrics and pricing, their final scores will indeed be equal.
However, the test could be more resilient to future provider catalog changes. Consider explicitly asserting on cache-logic behavior alone—e.g., verify that cacheSupported is reported correctly or compare score deltas between a run with cache-supporting providers and one without, rather than relying on priority field defaults to remain unchanged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/actions/src/models.spec.ts` around lines 948 - 963, The test relies
on implicit provider priority defaults (openai/deepseek) which makes it brittle;
update the test in packages/actions/src/models.spec.ts that calls
getCheapestFromAvailableProviders with cacheTestModel to explicitly assert
cache-related behavior instead of equal scores—e.g., check
result.metadata.providerScores entries for a cacheSupported (or equivalent) flag
for each provider and assert that cacheSupported is true/false as expected for
cache-supporting providers, or run two calls (one with providers modified to
disable cache support) and assert the score delta between runs; reference
getCheapestFromAvailableProviders, cacheTestModel, and the providerScores/openai
and deepseek entries to locate and change the assertions.
content is already a non-empty string at the call site; wrapping it in JSON.stringify just adds ~2 chars of quote/escape overhead and diverged from the equivalent path in costs.ts. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Surfaces the new cacheSupported routing-score field on the log detail card alongside uptime/throughput/latency/price. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
## Summary - Add `canopywave` provider entry for the `deepseek-v4-flash` model - Prices: $0.14/Mt input, $0.28/Mt output, $0.03/Mt cached input; 1M context; 30% discount - Capabilities aligned with canopywave's listed features (function-calling, structured-outputs, no reasoning) ## Test plan - [x] `TEST_MODELS="canopywave/deepseek-v4-flash" pnpm test:e2e` for basic, streaming, tool calls, JSON, reasoning, and prompt-caching suites — all pass individually - [x] `pnpm build` - [x] `pnpm format` 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Introduced canopywave as a new provider option for the deepseek-v4-flash model, featuring custom pricing and capability configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Luca Steeb (bot) <contact@luca-steeb.com>
Adds the cacheSupported badge to the shared log-card component used in log lists, mirroring the detail page. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
apps/gateway/src/chat/chat.ts (2)
1090-1100:⚠️ Potential issue | 🟠 MajorRecompute
routingPromptTokensafter guardrail redactions.
routingPromptTokensis still captured beforeapplyRedactions()can rewritemessages(Line 1403), so the 5k cache-weight threshold and the 10k auto-routing cutoff can be evaluated against a different prompt than the one we actually send upstream.Suggested fix
- let routingPromptTokens = 0; - if (messages && messages.length > 0) { - routingPromptTokens = encodeChatMessages(messages); - } - if (tools && tools.length > 0) { - routingPromptTokens += Math.round(JSON.stringify(tools).length / 4); - } + const calculateRoutingPromptTokens = () => { + let promptTokens = messages.length > 0 ? encodeChatMessages(messages) : 0; + if (tools && tools.length > 0) { + promptTokens += Math.round(JSON.stringify(tools).length / 4); + } + return promptTokens; + }; + let routingPromptTokens = calculateRoutingPromptTokens(); ... if (guardrailResult.redactions.length > 0) { messages = applyRedactions( messages as Parameters<typeof applyRedactions>[0], guardrailResult.redactions, ) as typeof messages; + routingPromptTokens = calculateRoutingPromptTokens(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 1090 - 1100, routingPromptTokens is computed from messages and tools before applyRedactions() mutates messages, so downstream routing thresholds (cache-weight and auto-routing) may use stale token counts; update the code to recompute routingPromptTokens after applyRedactions() runs (or call encodeChatMessages(messages) again on the redacted messages and re-add the tools heuristic), ensuring the token estimate used for decisions reflects the actual messages sent upstream and still uses encodeChatMessages and the existing tools length/4 heuristic.
6889-6892:⚠️ Potential issue | 🟠 MajorUse
calculatedReasoningTokensin downstream usage and cost accounting.These fallback blocks still only populate a local variable. Line 7141, Line 7200, Line 7419, Line 8961, and Lines 9004-9007 continue to read
reasoningTokens, soreasoning_tokens,total_tokens, and cost metadata are still undercounted whenever the provider omits reasoning usage.Also applies to: 8946-8948
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 6889 - 6892, The fallback logic that sets calculatedReasoningTokens (via estimateTokensFromContent) is correct but never used downstream; update all downstream usages that currently read reasoningTokens to use calculatedReasoningTokens when reasoningTokens is falsy—specifically wherever you compute reasoning_tokens, total_tokens, and cost metadata ensure you default to calculatedReasoningTokens (e.g., use (reasoningTokens ?? calculatedReasoningTokens) or similar). Make the change in the code paths that assemble usage/cost objects so reasoning_tokens reflects the fallback value and total_tokens/cost calculations include it, leaving existing behavior unchanged when reasoningTokens is already present.
🧹 Nitpick comments (3)
apps/playground/src/components/playground/chat-ui.tsx (1)
457-468: Prefer shared image config over local GPT-image option duplication.
isGptImage/size/quality logic is now split between this file andapps/playground/src/lib/image-gen.ts; consolidating on the shared config would reduce drift risk.Also applies to: 486-486
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/playground/src/components/playground/chat-ui.tsx` around lines 457 - 468, The duplicate image-model logic in chat-ui.tsx (variables isGptImage and usesPixelDimensions derived from selectedModel and used for size/quality UI) should be removed and replaced with a call to the shared image config in apps/playground/src/lib/image-gen.ts; locate the isGptImage and usesPixelDimensions definitions in chat-ui.tsx and replace them with imports and usage of the exported helpers or config (e.g., functions or flags from image-gen.ts that determine pixel-based dimensions and quality support) so the UI consumes the single source of truth for image model detection and size/quality rules.apps/gateway/src/chat/tools/transform-response-to-openai.ts (1)
463-468: Consider passing image tokens through in existing-response branches for consistency.The
applyExtendedUsageFieldscalls in branches that modify existing responses (e.g., lines 463-468, 749-754, and similar patterns for alibaba, bytedance, xai, zai, and the default case) don't passimageInputTokens/imageOutputTokens.Currently this is fine because image tokens are only relevant for OpenAI gpt-image models, which take the
buildUsageObjectpath. However, for consistency and future-proofing (e.g., if other providers add image token reporting), you could pass them through:applyExtendedUsageFields(transformedResponse.usage, { costs, cachedTokens, cacheCreationTokens, reasoningTokens, + imageInputTokens, + imageOutputTokens, });Also applies to: 749-754
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/transform-response-to-openai.ts` around lines 463 - 468, The applyExtendedUsageFields call in branches that update an existing response (e.g., where transformedResponse.usage is passed) omits imageInputTokens and imageOutputTokens, so add these tokens to all such calls for consistency and future-proofing; update each invocation of applyExtendedUsageFields (including the OpenAI-existing-response branch shown and the analogous alibaba, bytedance, xai, zai, and default branches) to include imageInputTokens and imageOutputTokens alongside costs, cachedTokens, cacheCreationTokens, and reasoningTokens, ensuring the values come from the same buildUsageObject or upstream variables used when image tokens are available.apps/playground/src/components/playground/chat-page-client.tsx (1)
1296-1398: Consider extracting duplicated logic into shared hooks.The model configuration logic (
isGptImagedetection,usesPixelDimensionschecks,sendMessageWithHeaderscallback, and the useEffect reset logic) is nearly identical between the main component andExtraChatPanel.Consider extracting this into:
- A custom hook like
useImageConfig(selectedModel)for state and reset logic- A helper function for building the
imageConfigobjectThis would reduce maintenance burden when adding new model types or changing behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/playground/src/components/playground/chat-page-client.tsx` around lines 1296 - 1398, The duplicated model/image-detection and reset logic (isGptImage, usesPixelDimensions, the useEffect that resets sizes/quality, and the sendMessageWithHeaders imageConfig construction) should be extracted so both this component and ExtraChatPanel share it; create a custom hook useImageConfig(selectedModel) that encapsulates the useEffect reset behavior and exports computed flags (isGptImage, usesPixelDimensions, supportsImageGen, supportsImages, default sizes/qualities and setters), and refactor the imageConfig building into a helper buildImageConfig({isGptImage, usesPixelDimensions, alibabaImageSize, imageSize, imageAspectRatio, imageQuality, imageCount, useImageGen}) which returns the image_config object used in sendMessageWithHeaders, then replace the inline logic in sendMessageWithHeaders and the component’s useEffect with calls to the new hook and helper so both components consume the shared behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/lib/costs.ts`:
- Around line 151-157: The prompt token estimation path overcounts multimodal
content because encodeChatMessages serializes arrays (via JSON.stringify) and
treats image blocks/data URLs as text; update the branch that handles
fullOutput.messages before calling encodeChatMessages to strip or replace
non-text message parts (e.g., objects with a type like "image", items with data:
URLs, Buffers, or known multimodal markers) with empty strings or a placeholder
text-only representation so encodeChatMessages only sees textual content; refer
to encodeChatMessages and the fullOutput.messages handling to locate the change
and ensure you do the same sanitization logic used by the tokenizer for image
detection so inputCost and promptTokens are not double-counted.
In `@apps/playground/src/components/playground/image-page-client.tsx`:
- Around line 204-232: The reset and request payload logic currently derives
config from selectedModels[0]; instead, call getModelImageConfig(modelId) for
each model when in comparison mode so each model's reset and imageConfigBody is
built from its own config (e.g., inside the loop that sends requests over
selectedModels), and only include fields like image_quality or pixel-dimension
settings when that model's config.supportsQuality or config.usesPixelDimensions
respectively; update the useEffect reset logic and the imageConfigBody
construction to compute config per model (referencing selectedModels,
getModelImageConfig, imageConfigBody, and the
imageQuality/imageSize/alibabaImageSize setters) to avoid leaking unsupported
fields to secondary models.
In `@apps/ui/src/components/models/model-card.tsx`:
- Around line 114-118: hasEstimatedImageCost currently returns true when
imageOutputTokensByResolution is an empty object; change the guard in
hasEstimatedImageCost to require both mapping.imageOutputPrice truthy and
mapping.imageOutputTokensByResolution to have at least one key (e.g.,
Object.keys(mapping.imageOutputTokensByResolution || {}).length > 0). Update the
same pattern wherever the same logic appears (the other occurrences that check
imageOutputPrice and imageOutputTokensByResolution) so empty maps do not count
as providing an estimated image cost.
In `@packages/actions/src/prepare-request-body.spec.ts`:
- Around line 207-235: Remove the unnecessary "as any" test casts: update the
three occurrences where requestBody is declared (calls to
prepareOpenAIImageRequest in the image tests) to use the actual return type
instead of "as any" (e.g., const requestBody = await
prepareOpenAIImageRequest({...})); this keeps type safety and lets existing
expect assertions work—adjust the variable declarations for the tests "should
not derive size from aspect_ratio", "should drop unsupported quality values",
and the earlier image-size test to drop the "as any" suffix.
In `@packages/actions/src/prepare-request-body.ts`:
- Around line 685-695: The code currently ignores image_config.aspect_ratio when
image_size is not set, causing silent fallback; update the image request
construction logic (involving image_config, openaiImageRequest, image_size,
aspect_ratio, normalizeImageQuality) to either map supported aspect_ratio values
to concrete OpenAI size strings or reject/throw a clear error when
image_config.aspect_ratio is present but image_config.image_size is not;
validate aspect_ratio against an explicit whitelist of supported ratios and
ensure the rejection occurs before building openaiImageRequest so callers
receive a fast, descriptive failure instead of silently using OpenAI defaults.
In `@scripts/image-edit.sh`:
- Around line 20-21: Validate the N variable immediately after it is set and
before any payload construction: check that N contains only digits (and
optionally is >=1) and exit with a clear error message if not; update the script
near where N is defined (reference variable N and TIMESTAMP/RESPONSE_FILE usage)
and also add the same validation before the payload-building section referenced
around the later payload block (the code near line ~84) so the script fails fast
with a helpful message instead of letting Python raise a traceback.
---
Duplicate comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 1090-1100: routingPromptTokens is computed from messages and tools
before applyRedactions() mutates messages, so downstream routing thresholds
(cache-weight and auto-routing) may use stale token counts; update the code to
recompute routingPromptTokens after applyRedactions() runs (or call
encodeChatMessages(messages) again on the redacted messages and re-add the tools
heuristic), ensuring the token estimate used for decisions reflects the actual
messages sent upstream and still uses encodeChatMessages and the existing tools
length/4 heuristic.
- Around line 6889-6892: The fallback logic that sets calculatedReasoningTokens
(via estimateTokensFromContent) is correct but never used downstream; update all
downstream usages that currently read reasoningTokens to use
calculatedReasoningTokens when reasoningTokens is falsy—specifically wherever
you compute reasoning_tokens, total_tokens, and cost metadata ensure you default
to calculatedReasoningTokens (e.g., use (reasoningTokens ??
calculatedReasoningTokens) or similar). Make the change in the code paths that
assemble usage/cost objects so reasoning_tokens reflects the fallback value and
total_tokens/cost calculations include it, leaving existing behavior unchanged
when reasoningTokens is already present.
---
Nitpick comments:
In `@apps/gateway/src/chat/tools/transform-response-to-openai.ts`:
- Around line 463-468: The applyExtendedUsageFields call in branches that update
an existing response (e.g., where transformedResponse.usage is passed) omits
imageInputTokens and imageOutputTokens, so add these tokens to all such calls
for consistency and future-proofing; update each invocation of
applyExtendedUsageFields (including the OpenAI-existing-response branch shown
and the analogous alibaba, bytedance, xai, zai, and default branches) to include
imageInputTokens and imageOutputTokens alongside costs, cachedTokens,
cacheCreationTokens, and reasoningTokens, ensuring the values come from the same
buildUsageObject or upstream variables used when image tokens are available.
In `@apps/playground/src/components/playground/chat-page-client.tsx`:
- Around line 1296-1398: The duplicated model/image-detection and reset logic
(isGptImage, usesPixelDimensions, the useEffect that resets sizes/quality, and
the sendMessageWithHeaders imageConfig construction) should be extracted so both
this component and ExtraChatPanel share it; create a custom hook
useImageConfig(selectedModel) that encapsulates the useEffect reset behavior and
exports computed flags (isGptImage, usesPixelDimensions, supportsImageGen,
supportsImages, default sizes/qualities and setters), and refactor the
imageConfig building into a helper buildImageConfig({isGptImage,
usesPixelDimensions, alibabaImageSize, imageSize, imageAspectRatio,
imageQuality, imageCount, useImageGen}) which returns the image_config object
used in sendMessageWithHeaders, then replace the inline logic in
sendMessageWithHeaders and the component’s useEffect with calls to the new hook
and helper so both components consume the shared behavior.
In `@apps/playground/src/components/playground/chat-ui.tsx`:
- Around line 457-468: The duplicate image-model logic in chat-ui.tsx (variables
isGptImage and usesPixelDimensions derived from selectedModel and used for
size/quality UI) should be removed and replaced with a call to the shared image
config in apps/playground/src/lib/image-gen.ts; locate the isGptImage and
usesPixelDimensions definitions in chat-ui.tsx and replace them with imports and
usage of the exported helpers or config (e.g., functions or flags from
image-gen.ts that determine pixel-based dimensions and quality support) so the
UI consumes the single source of truth for image model detection and
size/quality rules.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 26ac9f60-fef4-49fa-9054-2f4cef47191f
⛔ Files ignored due to path filters (4)
apps/code/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/playground/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsee/admin/src/lib/api/v1.d.tsis excluded by!**/v1.d.tspnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
.github/workflows/images.ymlapps/api/package.jsonapps/docs/content/features/image-generation.mdxapps/gateway/src/chat/chat.tsapps/gateway/src/chat/schemas/completions.tsapps/gateway/src/chat/tools/create-log-entry.tsapps/gateway/src/chat/tools/parse-provider-response.tsapps/gateway/src/chat/tools/resolve-provider-context.tsapps/gateway/src/chat/tools/transform-response-to-openai.spec.tsapps/gateway/src/chat/tools/transform-response-to-openai.tsapps/gateway/src/images/images.tsapps/gateway/src/lib/costs.spec.tsapps/gateway/src/lib/costs.tsapps/playground/package.jsonapps/playground/src/app/api/chat/route.tsapps/playground/src/app/api/image/route.tsapps/playground/src/components/model-selector.tsxapps/playground/src/components/playground/chat-page-client.tsxapps/playground/src/components/playground/chat-ui.tsxapps/playground/src/components/playground/image-controls.tsxapps/playground/src/components/playground/image-page-client.tsxapps/playground/src/lib/image-gen.tsapps/ui/src/app/dashboard/[orgId]/[projectId]/activity/[logId]/log-detail-client.tsxapps/ui/src/components/models/model-card.tsxdocs/providers/openai/gpt-image-2/openai-gpt-image-2.mdee/admin/package.jsonpackages/actions/src/prepare-request-body.spec.tspackages/actions/src/prepare-request-body.tspackages/cache/src/swr.tspackages/models/src/models/deepseek.tspackages/models/src/models/openai.tspackages/models/src/types.tspackages/shared/src/components/log-card.tsxscripts/image-edit.sh
✅ Files skipped from review due to trivial changes (8)
- apps/playground/package.json
- ee/admin/package.json
- apps/gateway/src/chat/schemas/completions.ts
- apps/api/package.json
- .github/workflows/images.yml
- apps/gateway/src/chat/tools/transform-response-to-openai.spec.ts
- apps/gateway/src/chat/tools/resolve-provider-context.ts
- packages/cache/src/swr.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/app/dashboard/[orgId]/[projectId]/activity/[logId]/log-detail-client.tsx
Comment on lines
+204
to
+232
| // Reset image size/quality when the selected model changes and the current | ||
| // value isn't valid for the new model. Including the value itself in deps | ||
| // would clobber the user's explicit selection on every re-render. | ||
| useEffect(() => { | ||
| const primaryModel = selectedModels[0] ?? ""; | ||
| const config = getModelImageConfig(primaryModel); | ||
| if (!config.availableSizes.includes(imageSize as never)) { | ||
| if (config.usesPixelDimensions) { | ||
| if (config.isGptImage && alibabaImageSize === "1024x1024") { | ||
| setAlibabaImageSize(config.defaultSize); | ||
| } else if ( | ||
| !(config.availableSizes as readonly string[]).includes(alibabaImageSize) | ||
| ) { | ||
| setAlibabaImageSize(config.defaultSize); | ||
| } | ||
| } else if ( | ||
| !(config.availableSizes as readonly string[]).includes(imageSize) | ||
| ) { | ||
| setImageSize(config.defaultSize); | ||
| } | ||
| if ( | ||
| !config.supportsQuality || | ||
| !(config.availableQualities as readonly string[]).includes(imageQuality) | ||
| ) { | ||
| setImageQuality(config.defaultQuality ?? "auto"); | ||
| } | ||
| if (!isEditModel) { | ||
| setInputImages([]); | ||
| } | ||
| }, [selectedModels, imageSize, imageGenModels, isEditModel]); | ||
| }, [selectedModels, isEditModel]); |
Contributor
There was a problem hiding this comment.
Build image config per compared model, not from selectedModels[0] only.
In comparison mode, both the reset logic and imageConfigBody are keyed off the primary model, but that same payload is sent to every selected model. With the new image_quality field, comparing a quality-capable model with one that doesn’t support quality will leak image_quality into the secondary request; the same applies to pixel-dimension defaults. Compute getModelImageConfig(modelId) inside the request loop or restrict comparison mode to the intersection of supported settings.
Possible fix
- const primaryModel = selectedModels[0] ?? "";
- const config = getModelImageConfig(primaryModel);
- // Always forward the user's quality choice (including "auto") so it
- // shows up in the activity log; the gateway / model treat "auto" the
- // same as omitting the field upstream.
- const includeQuality = config.supportsQuality && !!imageQuality;
- const imageConfigBody = config.usesPixelDimensions
- ? {
- ...(config.isGptImage
- ? alibabaImageSize !== "auto" && {
- image_size: alibabaImageSize,
- }
- : alibabaImageSize !== "1024x1024" && {
- image_size: alibabaImageSize,
- }),
- ...(includeQuality && { image_quality: imageQuality }),
- n: imageCount,
- }
- : {
- ...(imageAspectRatio !== "auto" && {
- aspect_ratio: imageAspectRatio,
- }),
- ...(imageSize !== "1K" && { image_size: imageSize }),
- n: imageCount,
- };
-
// Fire requests independently — each updates gallery as images stream in
pendingRef.current = selectedModels.length;
for (const modelId of selectedModels) {
+ const config = getModelImageConfig(modelId);
+ const includeQuality = config.supportsQuality && !!imageQuality;
+ const imageConfigBody = config.usesPixelDimensions
+ ? {
+ ...(config.isGptImage
+ ? alibabaImageSize !== "auto" && {
+ image_size: alibabaImageSize,
+ }
+ : alibabaImageSize !== "1024x1024" && {
+ image_size: alibabaImageSize,
+ }),
+ ...(includeQuality && { image_quality: imageQuality }),
+ n: imageCount,
+ }
+ : {
+ ...(imageAspectRatio !== "auto" && {
+ aspect_ratio: imageAspectRatio,
+ }),
+ ...(imageSize !== "1K" && { image_size: imageSize }),
+ n: imageCount,
+ };
+
const noFallback = shouldDisableFallback(modelId);Also applies to: 301-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/playground/src/components/playground/image-page-client.tsx` around lines
204 - 232, The reset and request payload logic currently derives config from
selectedModels[0]; instead, call getModelImageConfig(modelId) for each model
when in comparison mode so each model's reset and imageConfigBody is built from
its own config (e.g., inside the loop that sends requests over selectedModels),
and only include fields like image_quality or pixel-dimension settings when that
model's config.supportsQuality or config.usesPixelDimensions respectively;
update the useEffect reset logic and the imageConfigBody construction to compute
config per model (referencing selectedModels, getModelImageConfig,
imageConfigBody, and the imageQuality/imageSize/alibabaImageSize setters) to
avoid leaking unsupported fields to secondary models.
Comment on lines
+114
to
+118
| function hasEstimatedImageCost(mapping: ApiModelProviderMapping): boolean { | ||
| return Boolean( | ||
| mapping.imageOutputPrice && mapping.imageOutputTokensByResolution, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard hasEstimatedImageCost against empty resolution maps.
An empty {} map currently evaluates as estimated, which can suppress default token pricing until expand, despite no usable estimate rows.
Suggested fix
function hasEstimatedImageCost(mapping: ApiModelProviderMapping): boolean {
- return Boolean(
- mapping.imageOutputPrice && mapping.imageOutputTokensByResolution,
- );
+ const hasPrice =
+ mapping.imageOutputPrice !== null &&
+ mapping.imageOutputPrice !== undefined;
+ const byResolution = mapping.imageOutputTokensByResolution;
+ const hasEstimateRows =
+ !!byResolution && Object.keys(byResolution).length > 0;
+ return hasPrice && hasEstimateRows;
}Also applies to: 516-519, 958-958
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/models/model-card.tsx` around lines 114 - 118,
hasEstimatedImageCost currently returns true when imageOutputTokensByResolution
is an empty object; change the guard in hasEstimatedImageCost to require both
mapping.imageOutputPrice truthy and mapping.imageOutputTokensByResolution to
have at least one key (e.g., Object.keys(mapping.imageOutputTokensByResolution
|| {}).length > 0). Update the same pattern wherever the same logic appears (the
other occurrences that check imageOutputPrice and imageOutputTokensByResolution)
so empty maps do not count as providing an estimated image cost.
Comment on lines
+207
to
+235
| const requestBody = (await prepareOpenAIImageRequest({ | ||
| image_size: size, | ||
| image_quality: "high", | ||
| n: 1, | ||
| })) as any; | ||
|
|
||
| expect(requestBody).toMatchObject({ | ||
| model: "gpt-image-2", | ||
| prompt: "Generate a cinematic landscape", | ||
| size, | ||
| quality: "high", | ||
| n: 1, | ||
| }); | ||
| }); | ||
|
|
||
| test("should not derive size from aspect_ratio", async () => { | ||
| const requestBody = (await prepareOpenAIImageRequest({ | ||
| aspect_ratio: "16:9", | ||
| })) as any; | ||
|
|
||
| expect(requestBody.size).toBeUndefined(); | ||
| }); | ||
|
|
||
| test("should drop unsupported quality values", async () => { | ||
| const requestBody = (await prepareOpenAIImageRequest({ | ||
| image_size: "1024x1024", | ||
| image_quality: "standard", | ||
| })) as any; | ||
|
|
Contributor
There was a problem hiding this comment.
Remove new as any casts in these tests.
Line 211, Line 225, and Line 234 introduce new as any casts that are not necessary here. A typed helper return keeps assertions readable without dropping type safety.
♻️ Suggested fix
+type OpenAIImagePreparedBody = {
+ model: string;
+ prompt: string;
+ size?: string;
+ quality?: "low" | "medium" | "high" | "auto";
+ n?: number;
+};
+
async function prepareOpenAIImageRequest(imageConfig: {
aspect_ratio?: string;
image_size?: string;
image_quality?: string;
n?: number;
-}) {
- return await prepareRequestBody(
+}): Promise<OpenAIImagePreparedBody> {
+ return (await prepareRequestBody(
"openai",
"gpt-image-2",
[{ role: "user", content: "Generate a cinematic landscape" }],
false,
undefined,
@@
imageConfig,
undefined,
true,
- );
+ )) as OpenAIImagePreparedBody;
}
@@
- const requestBody = (await prepareOpenAIImageRequest({
+ const requestBody = await prepareOpenAIImageRequest({
image_size: size,
image_quality: "high",
n: 1,
- })) as any;
+ });
@@
- const requestBody = (await prepareOpenAIImageRequest({
+ const requestBody = await prepareOpenAIImageRequest({
aspect_ratio: "16:9",
- })) as any;
+ });
@@
- const requestBody = (await prepareOpenAIImageRequest({
+ const requestBody = await prepareOpenAIImageRequest({
image_size: "1024x1024",
image_quality: "standard",
- })) as any;
+ });As per coding guidelines, **/*.{ts,tsx}: Never use any or as any in TypeScript unless absolutely necessary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/actions/src/prepare-request-body.spec.ts` around lines 207 - 235,
Remove the unnecessary "as any" test casts: update the three occurrences where
requestBody is declared (calls to prepareOpenAIImageRequest in the image tests)
to use the actual return type instead of "as any" (e.g., const requestBody =
await prepareOpenAIImageRequest({...})); this keeps type safety and lets
existing expect assertions work—adjust the variable declarations for the tests
"should not derive size from aspect_ratio", "should drop unsupported quality
values", and the earlier image-size test to drop the "as any" suffix.
Comment on lines
+685
to
695
| // Pass image_size straight through to OpenAI as `WxH` (or `auto`). | ||
| // OpenAI returns a 4xx for unsupported sizes, which we propagate. | ||
| const openaiSize = image_config?.image_size; | ||
| const openaiQuality = normalizeImageQuality(image_config?.image_quality); | ||
|
|
||
| const openaiImageRequest: OpenAIImageRequest = { | ||
| model: usedModel, | ||
| prompt, | ||
| ...(openaiSize && { size: openaiSize }), | ||
| ...(openaiQuality && { quality: openaiQuality }), | ||
| ...(image_config?.n && { n: image_config.n }), |
Contributor
There was a problem hiding this comment.
Reject unsupported OpenAI aspect_ratio values instead of silently dropping them.
apps/gateway/src/images/images.ts still populates image_config.aspect_ratio, but this branch now forwards only image_size/quality. A request that sets only aspect_ratio will quietly fall back to OpenAI’s default size, which is a behavior regression and very hard for callers to detect. Either map the supported ratios here or fail fast when aspect_ratio is present without a concrete image_size.
Possible fix
// Pass image_size straight through to OpenAI as `WxH` (or `auto`).
// OpenAI returns a 4xx for unsupported sizes, which we propagate.
const openaiSize = image_config?.image_size;
+ if (image_config?.aspect_ratio && !openaiSize) {
+ throw new Error(
+ "OpenAI image generation requires image_size; aspect_ratio alone is not supported for this provider.",
+ );
+ }
const openaiQuality = normalizeImageQuality(image_config?.image_quality);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/actions/src/prepare-request-body.ts` around lines 685 - 695, The
code currently ignores image_config.aspect_ratio when image_size is not set,
causing silent fallback; update the image request construction logic (involving
image_config, openaiImageRequest, image_size, aspect_ratio,
normalizeImageQuality) to either map supported aspect_ratio values to concrete
OpenAI size strings or reject/throw a clear error when image_config.aspect_ratio
is present but image_config.image_size is not; validate aspect_ratio against an
explicit whitelist of supported ratios and ensure the rejection occurs before
building openaiImageRequest so callers receive a fast, descriptive failure
instead of silently using OpenAI defaults.
Comment on lines
+20
to
21
| N=${N:-1} | ||
| RESPONSE_FILE=${RESPONSE_FILE:-.context/image-edit-response-${TIMESTAMP}.json} |
Contributor
There was a problem hiding this comment.
Validate N before building the payload.
N is converted with int(...) later, so non-numeric values fail late with a Python traceback. Fail fast in bash with a clearer message.
Suggested fix
N=${N:-1}
+if ! [[ "$N" =~ ^[1-9][0-9]*$ ]]; then
+ echo "N must be a positive integer (got: $N)" >&2
+ exit 1
+fi
RESPONSE_FILE=${RESPONSE_FILE:-.context/image-edit-response-${TIMESTAMP}.json}Also applies to: 84-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/image-edit.sh` around lines 20 - 21, Validate the N variable
immediately after it is set and before any payload construction: check that N
contains only digits (and optionally is >=1) and exit with a clear error message
if not; update the script near where N is defined (reference variable N and
TIMESTAMP/RESPONSE_FILE usage) and also add the same validation before the
payload-building section referenced around the later payload block (the code
near line ~84) so the script fails fast with a helpful message instead of
letting Python raise a traceback.
# Conflicts: # apps/docs/content/features/image-generation.mdx # apps/playground/src/components/playground/chat-page-client.tsx
Move the chars/4 estimator into @llmgateway/shared with explicit multimodal handling: only text content is counted; image/file/audio parts are skipped. The previous JSON.stringify path on multimodal content double-counted image bytes against costs.ts image-input billing for vision/image requests when upstream usage was missing. Tracked follow-up for multimodal-aware estimation: #2112. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
steebchen
enabled auto-merge
April 29, 2026 11:54
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/shared/src/token-estimate.spec.ts (1)
26-91: Add boundary tests around the 5k-token routing cutoff.Given cache-weight routing depends on a 5k threshold, please add explicit cases near the boundary (for example, 19,999 vs 20,000 chars) to lock expected behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/token-estimate.spec.ts` around lines 26 - 91, Add explicit boundary tests in token-estimate.spec.ts around the 5k-token cutoff using estimateChatMessageTokens: create two tests that build messages with a single string content of length 19,999 and 20,000 characters respectively, call estimateChatMessageTokens([{ content: longString }]) for each, and assert the token estimate is below 5000 for the 19,999-char case and at or above 5000 for the 20,000-char case so the routing cutoff behavior is locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/tools/tokenizer.ts`:
- Around line 28-30: Replace the use of any[] in encodeChatMessages with a
concrete message type that matches what estimateChatMessageTokens expects:
define or import a ChatMessage type/interface where each message includes a
content field typed as string | Array<{ type?: string; text?: string }> | null
(and any other optional fields your codebase requires), update the function
signature to encodeChatMessages(messages: ChatMessage[]): number, and pass that
typed array into estimateChatMessageTokens so the compiler enforces the correct
structure; reference the encodeChatMessages function and the
estimateChatMessageTokens call when making the change.
---
Nitpick comments:
In `@packages/shared/src/token-estimate.spec.ts`:
- Around line 26-91: Add explicit boundary tests in token-estimate.spec.ts
around the 5k-token cutoff using estimateChatMessageTokens: create two tests
that build messages with a single string content of length 19,999 and 20,000
characters respectively, call estimateChatMessageTokens([{ content: longString
}]) for each, and assert the token estimate is below 5000 for the 19,999-char
case and at or above 5000 for the 20,000-char case so the routing cutoff
behavior is locked in.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a2228502-e2e9-4740-bb05-d42ad6a18f83
⛔ Files ignored due to path filters (4)
apps/code/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/playground/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/ui/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsee/admin/src/lib/api/v1.d.tsis excluded by!**/v1.d.ts
📒 Files selected for processing (6)
apps/gateway/src/chat/tools/estimate-tokens-from-content.tsapps/gateway/src/chat/tools/tokenizer.tsapps/gateway/src/lib/prompt-tokens.spec.tspackages/shared/src/index.tspackages/shared/src/token-estimate.spec.tspackages/shared/src/token-estimate.ts
✅ Files skipped from review due to trivial changes (1)
- packages/shared/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/gateway/src/lib/prompt-tokens.spec.ts
Comment on lines
28
to
30
| export function encodeChatMessages(messages: any[]): number { | ||
| try { | ||
| const chatMessages = messages.map((m) => ({ | ||
| role: m.role as "user" | "assistant" | "system" | undefined, | ||
| content: messageContentToString(m.content), | ||
| ...(m.name !== null && m.name !== undefined && { name: m.name }), | ||
| })); | ||
| return encodeChat(chatMessages, DEFAULT_TOKENIZER_MODEL).length; | ||
| } catch (error) { | ||
| logger.error("Failed to encode chat messages", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| messageCount: messages.length, | ||
| messageRoles: messages.map((m) => m.role), | ||
| messageContentTypes: messages.map((m) => typeof m.content), | ||
| }); | ||
| // Fallback: rough 4-chars-per-token estimate | ||
| return Math.max( | ||
| 1, | ||
| Math.round( | ||
| messages.reduce( | ||
| (acc: number, m: any) => | ||
| acc + messageContentToString(m.content).length, | ||
| 0, | ||
| ) / 4, | ||
| ), | ||
| ); | ||
| } | ||
| return estimateChatMessageTokens(messages); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current `any[]` usage and inspect encodeChatMessages call sites before tightening types.
rg -n --type=ts -C2 'export function encodeChatMessages\(messages:\s*any\[\]\)'
rg -n --type=ts -C2 '\bencodeChatMessages\('Repository: theopenco/llmgateway
Length of output: 2749
🏁 Script executed:
#!/bin/bash
# Check the actual types of messages at call sites
# 1. Check costs.ts to see type of fullOutput.messages
head -160 apps/gateway/src/lib/costs.ts | tail -20
# 2. Check chat.ts to see type of messages parameter where encodeChatMessages is called
grep -B20 "encodeChatMessages(messages)" apps/gateway/src/chat/chat.ts | head -40
# 3. Look for existing message type definitions
rg -n "type.*Message|interface.*Message" --type=ts | grep -v node_modules | head -20
# 4. Check estimate-tokens.ts
rg -B5 "encodeChatMessages" apps/gateway/src/chat/tools/estimate-tokens.ts
# 5. Check the estimateChatMessageTokens function signature
rg -A10 "function estimateChatMessageTokens" --type=tsRepository: theopenco/llmgateway
Length of output: 5181
🏁 Script executed:
#!/bin/bash
# Get the MessageLike interface definition
cat -n packages/shared/src/token-estimate.ts | head -50
# Check if encodeChatMessages is defined in tokenizer.ts with more context
cat -n apps/gateway/src/chat/tools/tokenizer.ts
# Check the types imported/used in chat.ts for the messages parameter
grep -B10 "function.*messages" apps/gateway/src/chat/chat.ts | head -30
# Check if there are type definitions for fullOutput in costs.ts
rg -B5 "fullOutput" apps/gateway/src/lib/costs.ts | head -30Repository: theopenco/llmgateway
Length of output: 4083
🏁 Script executed:
#!/bin/bash
# Check if MessageLike is exported from the shared package
rg -n "export.*MessageLike|export.*interface.*MessageLike" packages/shared/src/
# Check the exports in token-estimate.ts
grep -n "export" packages/shared/src/token-estimate.ts
# Check what's currently imported in tokenizer.ts
head -10 apps/gateway/src/chat/tools/tokenizer.ts
# Check if `@llmgateway/shared` is available as a dependency in gateway
grep -n "@llmgateway/shared" apps/gateway/package.json
# Check the full token-estimate.ts to see if MessageLike is exported
cat packages/shared/src/token-estimate.tsRepository: theopenco/llmgateway
Length of output: 2476
Replace any[] in encodeChatMessages with a concrete message type.
Line 28 uses any[], which violates the coding guidelines and drops type safety on a routing/cost-critical path. The function passes messages directly to estimateChatMessageTokens from @llmgateway/shared, which expects a concrete structure with content (string, array of parts, or null) and parts with optional type and text fields.
Suggested type-safe change
+type EstimationContentPart = {
+ type?: string;
+ text?: string;
+};
+
+type EstimationMessage = {
+ content?: string | EstimationContentPart[] | null;
+};
+
-export function encodeChatMessages(messages: any[]): number {
+export function encodeChatMessages(messages: EstimationMessage[]): number {
return estimateChatMessageTokens(messages);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/tools/tokenizer.ts` around lines 28 - 30, Replace the
use of any[] in encodeChatMessages with a concrete message type that matches
what estimateChatMessageTokens expects: define or import a ChatMessage
type/interface where each message includes a content field typed as string |
Array<{ type?: string; text?: string }> | null (and any other optional fields
your codebase requires), update the function signature to
encodeChatMessages(messages: ChatMessage[]): number, and pass that typed array
into estimateChatMessageTokens so the compiler enforces the correct structure;
reference the encodeChatMessages function and the estimateChatMessageTokens call
when making the change.
This was referenced Jun 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gpt-tokenizercall regressed gateway throughput on every chat request.encodeChatMessages/encodewith a cheap chars/4 length-based estimate everywhere on the gateway hot path. Accuracy is intentionally traded for throughput; routing only needs a rough threshold check, and post-call usage estimation only matters when the upstream omits token counts.gpt-tokenizeris dropped fromapps/gateway/package.json.Code paths switched off the tokenizer
apps/gateway/src/chat/chat.ts— routing prompt-token estimate at the top of the chat handler (the call site that caused the regression), the streaming-response prompt/completion/reasoning estimates around line 6857, and the non-streaming reasoning estimate near line 8944.apps/gateway/src/chat/tools/tokenizer.ts—encodeChatMessagesnow sumsmessageContentToString(...).lengthand divides by 4.apps/gateway/src/chat/tools/estimate-tokens.ts— completion-token fallback now usesestimateTokensFromContent.apps/gateway/src/lib/costs.ts—calculateCostsno longer runsencodeChat/encodeto fill in missing prompt/completion tokens; it reusesencodeChatMessages(now length-based) andestimateTokensFromContent.apps/gateway/src/chat/tools/calculate-prompt-tokens.ts— unchanged signature, but now backed by the length-basedencodeChatMessages.Test plan
pnpm formatpnpm build(turbo, all apps)npx vitest run --no-file-parallelism packages/actions packages/models packages/db apps/gateway/src/lib apps/gateway/src/chat(485 unit tests passing)Notes / implications
calculateCostsonly estimates prompt/completion tokens when the upstream response didn't include them. For mainstream providers (OpenAI, Anthropic, Google, etc.) the upstreamusageis honored and the heuristic isn't used. Where it is used (e.g. some streaming paths or older providers), prompt token counts may now be a few percent off, which propagates into the displayed input/output cost.cacheSupportedrouting weight only kicks in aboveCACHE_PROMPT_TOKEN_THRESHOLD(5k tokens). Length-based estimation is more than accurate enough to gate that decision.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation