Repository navigation
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe gateway now validates requested reasoning-effort tiers against provider metadata, documents the behavior in schemas and model definitions, tests supported and unsupported cases, and returns structured capability errors with matching activity-feed entries. ChangesReasoning effort validation and error reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant CompletionsRoute
participant ValidateModelCapabilities
participant ActivityFeed
Client->>CompletionsRoute: submit reasoning_effort
CompletionsRoute->>ValidateModelCapabilities: validate model capabilities
ValidateModelCapabilities-->>CompletionsRoute: unsupported_reasoning_effort
CompletionsRoute->>ActivityFeed: write client_error
CompletionsRoute-->>Client: HTTP 400 with capability message
🚥 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 |
Reject a requested reasoning_effort with a 400 (and a client_error log) when it falls outside a mapping's declared reasoningEfforts list. When a mapping declares no list, all efforts pass through unchanged so the gateway never 400s a request that could have succeeded upstream. With multiple candidate providers, only reject when every reasoning-capable mapping declares a list that excludes the value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
85f57f6 to
12934b0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/gateway/src/chat/tools/validate-model-capabilities.spec.ts (1)
250-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider self-contained fixtures instead of scanning the live
modelscatalog.
declaredModel/undeclaredModelare derived from production model metadata at module-evaluation time, so these tests silently change meaning as mappings evolve, and if the predicates ever stop matching the suite fails during collection with an opaqueErrorrather than as a test failure. The rest of this spec uses localModelDefinitionliterals (noVisionModel,verbosityModel), which keeps the allowlist assertions deterministic.🤖 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 `@apps/gateway/src/chat/tools/validate-model-capabilities.spec.ts` around lines 250 - 309, Replace the live-catalog scans used to initialize declaredModel and undeclaredModel with explicit local ModelDefinition fixtures, mirroring the existing noVisionModel and verbosityModel patterns. Ensure one fixture has reasoning-capable providers with non-empty reasoningEfforts and a missing global tier, while the other includes a reasoning-capable provider without a declared list; derive supportedEffort, unsupportedEffort, and pinnedProvider from those fixtures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 2679-2684: Update the capability-rejection path around
clientErrorCause and the HTTPException throw to preserve the structured error
code in the HTTP response. Match the existing service-tier/guardrail response
handling by returning the structured OpenAI error directly, or propagate
clientErrorCause through the global handler, while retaining the current
rejection status and message.
---
Nitpick comments:
In `@apps/gateway/src/chat/tools/validate-model-capabilities.spec.ts`:
- Around line 250-309: Replace the live-catalog scans used to initialize
declaredModel and undeclaredModel with explicit local ModelDefinition fixtures,
mirroring the existing noVisionModel and verbosityModel patterns. Ensure one
fixture has reasoning-capable providers with non-empty reasoningEfforts and a
missing global tier, while the other includes a reasoning-capable provider
without a declared list; derive supportedEffort, unsupportedEffort, and
pinnedProvider from those fixtures.
🪄 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 Plus
Run ID: d5bdb52a-46e2-4e9d-b152-69f258edb570
📒 Files selected for processing (5)
apps/gateway/src/chat/chat.tsapps/gateway/src/chat/schemas/completions.tsapps/gateway/src/chat/tools/validate-model-capabilities.spec.tsapps/gateway/src/chat/tools/validate-model-capabilities.tspackages/models/src/models.ts
| logger.error("Failed to log capability rejection", { | ||
| error: toError(error), | ||
| cause: clientErrorCause, | ||
| }); | ||
| } | ||
| throw new HTTPException(400, { message }); | ||
| throw new HTTPException(400, { message: clientErrorMessage }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How does the global error handler build the client error body (code/param) from HTTPException?
fd -t f 'error' apps/gateway/src --extension ts | head -50
rg -nP --type=ts -C6 'invalid_request_error' apps/gateway/src | head -120
rg -nP --type=ts -C8 '\bcause\b' apps/gateway/src --glob '!**/*.spec.ts' | rg -nC6 'HTTPException|onError' | head -120Repository: theopenco/llmgateway
Length of output: 17351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== app.ts error handler section ==="
sed -n '200,255p' apps/gateway/src/app.ts | cat -n
echo "=== error-response.ts ==="
cat -n apps/gateway/src/lib/error-response.ts
echo "=== app.ts renderGatewayError references/typescript usages ==="
rg -nP --type=ts -C8 'function renderGatewayError|renderGatewayError|onError|if \(error instanceof HTTPException\)' apps/gateway/src | head -200
echo "=== relevant chat.ts validation and capability catch ==="
sed -n '2390,2445p' apps/gateway/src/chat/chat.ts | cat -n
sed -n '2550,2690p' apps/gateway/src/chat/chat.ts | cat -n
echo "=== app.ts imports ==="
sed -n '1,80p' apps/gateway/src/app.ts | cat -n
echo "=== package references to hono/http-exception ==="
rg -n 'hono/http-exception|HTTPException' apps/gateway/package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | head -80 || true
echo "=== app.spec relevant lines ==="
sed -n '1060,1125p' apps/gateway/src/api.spec.ts | cat -nRepository: theopenco/llmgateway
Length of output: 34539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== renderGatewayError implementation ==="
rg -n --type=ts -C8 'function renderGatewayError|const renderGatewayError|export .*renderGatewayError' apps/gateway/src
echo "=== broader renderGatewayError usages near app errors ==="
rg -n --type=ts -C4 'renderGatewayError\(c, error' apps/gateway/src
echo "=== app.ts all lines around onError start ==="
rg -n --type=ts -C6 'onError:|onError' apps/gateway/src/app.ts
sed -n '1,80p' apps/gateway/src/app.ts | cat -nRepository: theopenco/llmgateway
Length of output: 5372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== service tier validation throw location ==="
rg -n --type=ts -C6 'unsupported_service_tier|service_tier' apps/gateway/src --glob '**/*.ts'
echo "=== app.ts renderGatewayError body ==="
sed -n '139,200p' apps/gateway/src/app.ts | cat -nRepository: theopenco/llmgateway
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== exact chat service_tier throw block ==="
sed -n '2099,2168p' apps/gateway/src/chat/chat.ts | cat -n
echo "=== exact chat reasoning_effort throw block ==="
sed -n '2280,2345p' apps/gateway/src/chat/chat.ts | cat -n
echo "=== exact app renderGatewayError body ==="
sed -n '168,199p' apps/gateway/src/app.ts | cat -n
echo "=== exact client response parsing for code/param ==="
rg -n --type=ts -C4 'unsupported_service_tier|code.*param|error.code|error.code\(' apps/gateway/src/api.spec.ts apps/gateway/src -g '*.ts' | head -240Repository: theopenco/llmgateway
Length of output: 22097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== service tier throw complete ==="
sed -n '2107,2182p' apps/gateway/src/chat/chat.ts | cat -n
echo "=== reasoning effort throw complete ==="
sed -n '2320,2380p' apps/gateway/src/chat/chat.ts | cat -n
echo "=== read-only probe: global handler body for service_tier/effort code values ==="
python3 - <<'PY'
from pathlib import Path
app = Path("apps/gateway/src/app.ts").read_text()
error_response = Path("apps/gateway/src/lib/error-response.ts").read_text()
chat = Path("apps/gateway/src/chat/chat.ts").read_text()
for needle in ["new HTTPException(400, {", ".cause", "BuildOpenAIErrorBody", "buildOpenAIErrorBody"]:
print(f"{needle}: {needle in error_response or needle in app or needle in chat}")
print("app onError uses renderGatewayError for HTTPException:", "return renderGatewayError(c, status, error.message)" in app or "renderGatewayError(c, error.statusCode, error.message)" in app)
print("renderGatewayError params:", "function renderGatewayError(" in app and "message: string" in app)
print("buildOpenAIErrorBody ignores cause:", "cause" not in error_response or "function buildOpenAIErrorBody(opts" in error_response and "code: opts.code !== undefined ? opts.code : meta.code" in error_response)
start = chat.find("if (serviceTierSupportedProviders.length === 0)")
end = chat.find("throw new HTTPException", start)
print("\nservice_tier throw snippet:")
for i, line in enumerate(chat[start:end+400].splitlines(), 1):
if i <= 90:
print(f"{i}: {line}")
PYRepository: theopenco/llmgateway
Length of output: 7934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '2170,2192p' apps/gateway/src/chat/chat.ts | cat -n
sed -n '2370,2436p' apps/gateway/src/chat/chat.ts | cat -nRepository: theopenco/llmgateway
Length of output: 2971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '2180,2198p' apps/gateway/src/chat/chat.ts | cat -n
sed -n '2420,2442p' apps/gateway/src/chat/chat.ts | cat -n
sed -n '2674,2690p' apps/gateway/src/chat/chat.ts | cat -nRepository: theopenco/llmgateway
Length of output: 1997
Preserve the capability-rejection code in the HTTP exception.
This code path records clientErrorCause but rethrows a plain 400 body, so OpenAI clients see error.code: null while Anthropic clients already have no code in their envelope. Return the structured OpenAI error directly, like the service-tier/guardrail paths, or carry the cause through the global handler.
🔧 Proposed fix
- throw new HTTPException(400, { message: clientErrorMessage });
+ return c.json({
+ error: {
+ message: clientErrorMessage,
+ type: clientErrorCause === "unsupported_service_tier"
+ ? "invalid_request_error"
+ : clientErrorCause,
+ param: clientErrorCause === "unsupported_service_tier" ? "service_tier" : null,
+ code: clientErrorCause === "unsupported_service_tier" ? "unsupported_service_tier" : null,
+ },
+ }, 400);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.error("Failed to log capability rejection", { | |
| error: toError(error), | |
| cause: clientErrorCause, | |
| }); | |
| } | |
| throw new HTTPException(400, { message }); | |
| throw new HTTPException(400, { message: clientErrorMessage }); | |
| logger.error("Failed to log capability rejection", { | |
| error: toError(error), | |
| cause: clientErrorCause, | |
| }); | |
| } | |
| return c.json({ | |
| error: { | |
| message: clientErrorMessage, | |
| type: clientErrorCause === "unsupported_service_tier" | |
| ? "invalid_request_error" | |
| : clientErrorCause, | |
| param: clientErrorCause === "unsupported_service_tier" ? "service_tier" : null, | |
| code: clientErrorCause === "unsupported_service_tier" ? "unsupported_service_tier" : null, | |
| }, | |
| }, 400); |
🤖 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 `@apps/gateway/src/chat/chat.ts` around lines 2679 - 2684, Update the
capability-rejection path around clientErrorCause and the HTTPException throw to
preserve the structured error code in the HTTP response. Match the existing
service-tier/guardrail response handling by returning the structured OpenAI
error directly, or propagate clientErrorCause through the global handler, while
retaining the current rejection status and message.
|
Closing for now — not necessary at the moment. Branch and work preserved; can reopen if we decide to enforce the reasoning-effort allowlist later. |
What
When a request specifies a
reasoning_effort(orreasoning.effort) that is not in a model mapping's declaredreasoningEffortslist, the gateway now rejects it with a 400 and records aclient_errorlog so the rejected request shows up in the activity feed. Previously the value was forwarded blindly and only failed upstream (for enum-based providers) or was silently translated (for budget-based providers) — thereasoningEffortsfield was purely informational metadata.Behavior
["low","medium","high"]) and user requests"xhigh"→ 400 +client_errorlog (causeunsupported_reasoning_effort).provider/model) → checked against that provider's list only.auto/custommodels and non-reasoning models keep their existing behavior.Changes
validate-model-capabilities.ts— new allowlist check after the reasoning-support gate; throwsHTTPException(400, { cause: "unsupported_reasoning_effort" }).chat.ts— generalized the existing capability-error catch so both the Anthropic budget-thinking case and the new effort case insert aclient_errorlog via one shared_insertLogcall.completions.ts/models.ts— updated thereasoning_effortschema docs and thereasoningEffortsfield doc to describe the new enforcement.pnpm formatand a fullpnpm buildpass.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation