fix: stop imposing catalog assumptions on custom providers - #2667
Merged
Merged
Conversation
Custom providers synthesized a mock model definition with a hardcoded maxOutput of 4096, which made the gateway reject requests whose max_tokens exceeded 4096 even when the upstream model supports far more. The gateway has no catalog knowledge of a custom model's real limits, so leave maxOutput uncapped and let the upstream provider enforce it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughSkip synthesizing catalog-derived limits and capability validation for custom providers; retain only type-required ChangesCustom provider handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Comment |
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/gateway/src/chat-custom-provider.e2e.ts`:
- Around line 200-221: The test "should not cap max_tokens for custom providers"
currently only checks for a 200 and response body; update it to assert the
upstream request actually received max_tokens: 32000 so the value isn't being
rewritten/omitted. After sending the POST to "/v1/chat/completions" (the
app.request call that sets max_tokens: 32000), read the mock upstream's captured
request (e.g., the recorded request array or spy used by your test harness) and
add an expectation that the parsed upstream request body has max_tokens ===
32000; keep the existing response assertions intact.
🪄 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: 4334ae5e-8a51-4ac7-a622-ed6870bd73cc
📒 Files selected for processing (3)
apps/gateway/src/chat-custom-provider.e2e.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/resolve-model-info.ts
Comment on lines
+200
to
+221
| test("should not cap max_tokens for custom providers", async () => { | ||
| await setupTestData({ mode: "api-keys", includeProviderKey: true }); | ||
|
|
||
| const res = await app.request("/v1/chat/completions", { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: "Bearer real-token", | ||
| }, | ||
| body: JSON.stringify({ | ||
| model: "my-custom/qwen3.6-plus", | ||
| max_tokens: 32000, | ||
| messages: [{ role: "user", content: "hello" }], | ||
| }), | ||
| }); | ||
|
|
||
| const json = await res.json(); | ||
| expect(res.status).toBe(200); | ||
| expect(json.choices[0].message.content).toBe( | ||
| "Hello from custom provider!", | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
This regression test doesn’t verify the token value is actually uncapped/forwarded.
Right now it only asserts a 200 response. Since the mock server always returns success, this can still pass if max_tokens is silently rewritten or omitted before upstream dispatch. Add an assertion on the received upstream request body (e.g., max_tokens === 32000).
Suggested test hardening
+let lastCustomProviderRequestBody: unknown = null;
+
mockServer.post("/v1/chat/completions", async (c) => {
+ lastCustomProviderRequestBody = await c.req.json();
return c.json({
id: "chatcmpl-mock-custom",
object: "chat.completion",
created: Math.floor(Date.now() / 1000),
@@
test("should not cap max_tokens for custom providers", async () => {
await setupTestData({ mode: "api-keys", includeProviderKey: true });
@@
const json = await res.json();
expect(res.status).toBe(200);
+ expect((lastCustomProviderRequestBody as { max_tokens?: number }).max_tokens).toBe(32000);
expect(json.choices[0].message.content).toBe(
"Hello from custom provider!",
);
});🤖 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-custom-provider.e2e.ts` around lines 200 - 221, The
test "should not cap max_tokens for custom providers" currently only checks for
a 200 and response body; update it to assert the upstream request actually
received max_tokens: 32000 so the value isn't being rewritten/omitted. After
sending the POST to "/v1/chat/completions" (the app.request call that sets
max_tokens: 32000), read the mock upstream's captured request (e.g., the
recorded request array or spy used by your test harness) and add an expectation
that the parsed upstream request body has max_tokens === 32000; keep the
existing response assertions intact.
contextSize, maxOutput, and vision cannot be known for a custom provider since it has no catalog entry. Leave them unset instead of guessing placeholder values; the upstream provider enforces its own limits and capabilities. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
steebchen
changed the title
Custom providers have no catalog entry, so the gateway cannot know their capabilities (vision, jsonOutput, etc). The synthesized model definition previously guessed these flags, which both asserted false limits and risked rejecting valid requests. Skip capability validation entirely for custom providers and let the upstream provider be the authority. Drop the now-unused jsonOutput flag from the synthesized mapping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
steebchen
changed the title
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
1 participant
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.
Problem
Requests to a custom provider were rejected when
max_tokensexceeded 4096, even when the underlying model supports far more:Custom providers have no entry in the model catalog, so the gateway synthesizes a mock
ModelDefinition. That mock hardcoded placeholder values it cannot actually know —maxOutput: 4096,contextSize: 8192,vision: false,jsonOutput: true. These guesses both imposed false limits (themaxOutputcap above) and gated capability rejections (vision/JSON/tools) on values the gateway has no way to determine.Fix
Treat custom providers as fully opaque — the upstream provider is the authority on limits and capabilities:
maxOutput,contextSize,vision,jsonOutput) from the synthesized custom-provider mapping in both places it is built (resolve-model-info.tsandchat.ts). Themax_tokensvalidation guards onmaxOutput !== undefined, so it is now skipped for custom providers;contextSizeonly feeds auto-routing (which never applies to a pinned custom provider) and has a fallback.validateModelCapabilities(early return whenrequestedProvider === "custom"), consistent with how it already skips the bareauto/custommodel strings. This covers vision, documents, JSON output, JSON schema, reasoning, and tools — none of which the gateway can know for a custom endpoint.streaming: trueis kept only because the type requires it; it is never read for custom providers (streaming support is computed from the catalog viagetModelStreamingSupport, which returnsnullfor a non-catalog model and therefore never rejects). Pricing stays"0"(BYOK, not billed by the gateway).Tests
chat-custom-provider.e2e.ts: added regression tests formax_tokens: 32000andresponse_format: json_objectagainst a custom provider, both asserting200. Full suite (8 tests) passes.validate-model-capabilities.spec.ts: added a test asserting all capability checks are skipped when the provider is custom (12 tests pass).pnpm formatandpnpm buildare green.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests