fix: respect max_completion_tokens and guard missing model capabilities - #275
laizhengbao wants to merge 2 commits into
Conversation
Two fixes in the chat completions handler: 1. Clients that send the newer `max_completion_tokens` parameter (instead of legacy `max_tokens`) receive a 400 from the Copilot API: `max_tokens and max_completion_tokens cannot both be set`. The handler injects its own `max_tokens` whenever the legacy field is unset, and upstream rejects requests carrying both parameters. Injection is now skipped when either parameter is present, and the payload type gains an optional `max_completion_tokens` field so the value is forwarded upstream as intended. Related: ericc-ch#220 addresses the same interop by normalizing `max_completion_tokens` into `max_tokens` instead; this commit takes the passthrough approach. Happy to drop this half if ericc-ch#220 lands first. 2. GitHub's /models endpoint lists some entries WITHOUT a `capabilities` object (observed: `gpt-41-copilot`, `trajectory-compaction`). Requesting one crashed the proxy with a 500: `undefined is not an object (evaluating 'selectedModel?.capabilities.limits.max_output_tokens')`. `Model.capabilities` is now optional to mirror the real API, and both the handler and the tokenizer lookup guard against its absence. Tested against a live Copilot endpoint (individual plan): - `max_completion_tokens`-only request -> 200 (previously 500) - capability-less model without max_tokens -> clean upstream error, no crash - plain request without either parameter -> `max_tokens` still injected - streaming requests unaffected
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. WalkthroughThe change adds model-session support and adapters for Responses and legacy Completions. It adds POST routes for both protocols and registers standard and 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
src/lib/api-config.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/lib/state.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). src/routes/completions/route.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency).
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 |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
A moderate issue remains when max_completion_tokens is explicitly null, potentially forwarding both token-limit parameters.
Review effort: Lite
Findings: None
What changed in this PR
Updates chat completion handling for modern token limits and models with incomplete capability metadata.
Changes:
- Supports
max_completion_tokenswithout conflicting injection. - Makes capability and tokenizer lookups safe.
- Adds regression tests.
| File | Summary |
|---|---|
tests/chat-completions.test.ts |
Adds regression coverage. |
src/services/copilot/get-models.ts |
Makes model capabilities optional. |
src/services/copilot/create-chat-completions.ts |
Adds max_completion_tokens support. |
src/routes/chat-completions/handler.ts |
Adjusts token-limit injection and capability access. |
src/lib/tokenizer.ts |
Adds tokenizer fallback handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: c588bb46-3afe-46bc-89e9-85f7d04be650
📒 Files selected for processing (17)
src/lib/api-config.tssrc/lib/state.tssrc/routes/completions/route.tssrc/routes/responses/route.tssrc/server.tssrc/services/copilot/completions-adapter.tssrc/services/copilot/create-chat-completions.tssrc/services/copilot/create-model-session.tssrc/services/copilot/create-proxy-completions.tssrc/services/copilot/create-responses.tssrc/services/copilot/get-models.tssrc/services/copilot/responses-adapter.tssrc/services/github/get-copilot-usage.tstests/chat-completions.test.tstests/create-chat-completions.test.tstests/model-session.test.tstests/responses-adapter.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
📜 Review details
🔇 Additional comments (13)
tests/chat-completions.test.ts (1)
72-77: LGTM!src/lib/api-config.ts (1)
14-14: 🎯 Functional CorrectnessDo not flag
2026-01-09based on the public REST version list.
githubHeadersis used only for GitHub's internal Copilot endpoints:/copilot_internal/v2/tokenand/copilot_internal/user. The public REST version documentation does not establish the version contract for these endpoints, and the supplied evidence does not show that they reject2026-01-09. No version change is supported by this finding.src/services/copilot/get-models.ts (1)
49-49: LGTM!src/lib/state.ts (1)
1-1: LGTM!Also applies to: 10-11
src/services/github/get-copilot-usage.ts (1)
41-41: LGTM!src/services/copilot/create-model-session.ts (1)
82-88: LGTM!tests/model-session.test.ts (1)
1-116: LGTM!tests/create-chat-completions.test.ts (1)
12-18: LGTM!src/services/copilot/create-chat-completions.ts (1)
7-15: LGTM!Also applies to: 53-55, 61-61
tests/responses-adapter.test.ts (1)
1-202: LGTM!src/routes/responses/route.ts (1)
1-68: LGTM!src/routes/completions/route.ts (1)
1-66: LGTM!src/server.ts (1)
6-6: LGTM!Also applies to: 10-10, 22-22, 25-25, 31-31, 34-34
| const prompt = | ||
| Array.isArray(payload.prompt) ? payload.prompt.join("\n") : payload.prompt |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve separate array prompts.
If a legacy request supplies prompt: ["first", "second"], this code sends one prompt containing "first\nsecond". The Completions contract treats array elements as separate prompts, so the route returns a completion for a different input and cannot return results for each prompt. Process each prompt separately and retain its result index, or reject array prompts rather than silently merging them. (github.com)
| const result: ChatCompletionsPayload = { | ||
| model: payload.model, | ||
| messages: [{ role: "user", content: prompt }], | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve n and every completion choice.
If a client requests n: 2, this conversion drops n, so the chat request uses its single-choice default. The non-streaming and streaming converters also select choices[0]; forwarding n alone would still discard additional choices. Transfer n, then convert every choice with its original index in both result paths. (github.com)
| return { | ||
| id: chat.id, | ||
| object: "text_completion", | ||
| created: chat.created, | ||
| model: chat.model, | ||
| choices: [ | ||
| { | ||
| text: choice.message.content ?? "", | ||
| index: 0, | ||
| finish_reason: choice.finish_reason, | ||
| logprobs: null, | ||
| }, | ||
| ], | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Carry token usage into non-streaming completion results.
When the chat response contains usage, this conversion drops it. Legacy completion clients therefore cannot obtain the token counts from a successful chat-backed request. Add optional usage to ProxyCompletionResult and copy it here. (github.com)
| try { | ||
| const response = await fetch(`${copilotBaseUrl(state)}/models/session`, { | ||
| method: "POST", | ||
| headers: copilotHeaders(state), | ||
| body: JSON.stringify({ auto_mode: { model_hints: modelHints() } }), | ||
| }) | ||
|
|
||
| if (!response.ok) { | ||
| consola.warn("Model session creation failed with status", response.status) | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Cache session-creation failures and share one in-flight request.
createChatCompletions and createResponses await modelSessionHeaders on every request. When POST /models/session fails, for example because the account or SKU does not support it, ensureModelSession returns undefined and stores nothing. Every later chat request then makes one more upstream round trip before its real request. The fetch also has no timeout. A slow session endpoint therefore delays every chat request. Parallel first requests each start their own POST /models/session.
Store a short failure backoff. Share one pending promise. Add an abort timeout.
♻️ Proposed fix
+const FAILURE_BACKOFF_MS = 5 * 60 * 1000
+let retryAfter = 0
+let pending: Promise<ModelSession | undefined> | undefined
+
export const ensureModelSession = async (): Promise<
ModelSession | undefined
> => {
if (!state.copilotToken) return
const current = state.modelSession
if (current && current.expiresAt > Date.now()) return current
+ if (Date.now() < retryAfter) return undefined
+ pending ??= createSession().finally(() => {
+ pending = undefined
+ })
+ return pending
+}In createSession, pass signal: AbortSignal.timeout(10_000) to fetch. On a non-OK status or an error, set retryAfter = Date.now() + FAILURE_BACKOFF_MS.
| const response = await fetch( | ||
| `${base}/v1/engines/${payload.model}/completions`, | ||
| { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟡 Minor | ⚡ Quick win
Path Traversal
Reachability: External
Exploitability: Moderate
CWE: CWE-22 — Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Encode payload.model before inserting it into the proxy URL path.
payload.model comes from the request body, and the route checks only endsWith("-copilot"). A value such as ../../other/path?x=-copilot passes that check. URL parsing then normalizes the traversal and sends the Copilot bearer token to another path on the same proxy host. Encode the model as one path segment:
🔒️ Proposed fix
const response = await fetch(
- `${base}/v1/engines/${payload.model}/completions`,
+ `${base}/v1/engines/${encodeURIComponent(payload.model)}/completions`,The impact depends on whether callers already have full use of the Copilot token.
📝 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.
| const response = await fetch( | |
| `${base}/v1/engines/${payload.model}/completions`, | |
| { | |
| const response = await fetch( | |
| `${base}/v1/engines/${encodeURIComponent(payload.model)}/completions`, | |
| { |
| if (!isNullish(payload.tool_choice)) { | ||
| result.tool_choice = payload.tool_choice | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the shape of an object-form tool_choice.
Chat Completions uses { type: "function", function: { name } }. The Responses API uses { type: "function", name }. Both conversion functions copy tool_choice unchanged. In both directions, a client that forces a specific tool sends the wrong shape upstream, and the request fails. The string values "none", "auto" and "required" are the same in both APIs.
🐛 Proposed fix
- if (!isNullish(payload.tool_choice)) {
- result.tool_choice = payload.tool_choice
- }
+ const choice = payload.tool_choice
+ if (!isNullish(choice)) {
+ result.tool_choice =
+ typeof choice === "string" ? choice
+ : { type: "function", name: choice.function.name }
+ }- if (!isNullish(payload.tool_choice)) {
- result.tool_choice =
- payload.tool_choice as ChatCompletionsPayload["tool_choice"]
- }
+ const choice = payload.tool_choice
+ if (typeof choice === "string") {
+ result.tool_choice = choice as ChatCompletionsPayload["tool_choice"]
+ } else if (
+ choice && typeof (choice as { name?: unknown }).name === "string"
+ ) {
+ result.tool_choice = {
+ type: "function",
+ function: { name: (choice as { name: string }).name },
+ }
+ }Also applies to: 506-509
| ...(toolCalls.length > 0 ? { tool_calls: toolCalls } : {}), | ||
| }, | ||
| logprobs: null, | ||
| finish_reason: toolCalls.length > 0 ? "tool_calls" : "stop", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Map status: "incomplete" to finish_reason: "length".
A Responses result that stops at max_output_tokens has status: "incomplete". This code reports "stop" in that case. A chat client then treats a truncated reply as complete. completedEventToChunks also sets "stop" on the streaming path, so both paths need the same mapping.
🐛 Proposed fix
- finish_reason: toolCalls.length > 0 ? "tool_calls" : "stop",
+ finish_reason:
+ toolCalls.length > 0 ? "tool_calls"
+ : result.status === "incomplete" ? "length"
+ : "stop",📝 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.
| finish_reason: toolCalls.length > 0 ? "tool_calls" : "stop", | |
| finish_reason: | |
| toolCalls.length > 0 ? "tool_calls" | |
| : result.status === "incomplete" ? "length" | |
| : "stop", |
| if (item.type === "function_call") { | ||
| messages.push({ | ||
| role: "assistant", | ||
| content: null, | ||
| tool_calls: [ | ||
| { | ||
| id: item.call_id ?? "call_0", | ||
| type: "function", | ||
| function: { name: item.name ?? "", arguments: item.arguments ?? "" }, | ||
| }, | ||
| ], | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Group consecutive function_call items into one assistant message.
Parallel tool calls reach this code as consecutive function_call items, followed by their function_call_output items. Each function_call becomes its own assistant message here. The result is assistant(call_1), assistant(call_2), tool(call_1), tool(call_2). Chat Completions requires the tool messages for all of an assistant message's tool_calls to come right after that message. The upstream rejects this sequence.
🐛 Proposed fix
if (item.type === "function_call") {
- messages.push({
- role: "assistant",
- content: null,
- tool_calls: [
- {
- id: item.call_id ?? "call_0",
- type: "function",
- function: { name: item.name ?? "", arguments: item.arguments ?? "" },
- },
- ],
- })
+ const call: ToolCall = {
+ id: item.call_id ?? `call_${messages.length}`,
+ type: "function",
+ function: { name: item.name ?? "", arguments: item.arguments ?? "" },
+ }
+ const last = messages.at(-1)
+ if (last?.role === "assistant") {
+ last.tool_calls = [...(last.tool_calls ?? []), call]
+ } else {
+ messages.push({ role: "assistant", content: null, tool_calls: [call] })
+ }
return
}📝 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.
| if (item.type === "function_call") { | |
| messages.push({ | |
| role: "assistant", | |
| content: null, | |
| tool_calls: [ | |
| { | |
| id: item.call_id ?? "call_0", | |
| type: "function", | |
| function: { name: item.name ?? "", arguments: item.arguments ?? "" }, | |
| }, | |
| ], | |
| }) | |
| return | |
| } | |
| if (item.type === "function_call") { | |
| const call: ToolCall = { | |
| id: item.call_id ?? `call_${messages.length}`, | |
| type: "function", | |
| function: { name: item.name ?? "", arguments: item.arguments ?? "" }, | |
| } | |
| const last = messages.at(-1) | |
| if (last?.role === "assistant") { | |
| last.tool_calls = [...(last.tool_calls ?? []), call] | |
| } else { | |
| messages.push({ role: "assistant", content: null, tool_calls: [call] }) | |
| } | |
| return | |
| } |
| const responsesToolsToChatTools = (tools: unknown): Array<Tool> | undefined => { | ||
| if (!Array.isArray(tools)) return undefined | ||
| return tools.map((tool) => { | ||
| const record = tool as Record<string, unknown> | ||
| return { | ||
| type: "function", | ||
| function: { | ||
| name: String(record.name), | ||
| description: | ||
| typeof record.description === "string" ? | ||
| record.description | ||
| : undefined, | ||
| parameters: (record.parameters ?? {}) as Record<string, unknown>, | ||
| }, | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Convert only type: "function" tools.
Responses clients can send built-in or custom tools, such as web_search or local_shell, that have no top-level name. responsesToolsToChatTools turns each of these into a function tool named "undefined", via String(record.name), with empty parameters. The upstream then rejects the request, or the model sees a false tool. Skip tools that are not functions.
🐛 Proposed fix
const responsesToolsToChatTools = (tools: unknown): Array<Tool> | undefined => {
if (!Array.isArray(tools)) return undefined
- return tools.map((tool) => {
- const record = tool as Record<string, unknown>
+ const converted = (tools as Array<Record<string, unknown>>)
+ .filter((record) => record.type === "function" && typeof record.name === "string")
+ .map((record) => {
return {
type: "function",
function: {
- name: String(record.name),
+ name: record.name as string,After the map, return converted.length > 0 ? converted : undefined.
| const collectToolCalls = ( | ||
| choice: ChatCompletionChunk["choices"][0], | ||
| toolCalls: Array<ToolCall>, | ||
| ) => { | ||
| for (const call of choice.delta.tool_calls ?? []) { | ||
| toolCalls.push({ | ||
| id: call.id ?? `call_${toolCalls.length}`, | ||
| type: "function", | ||
| function: { | ||
| name: call.function?.name ?? "", | ||
| arguments: call.function?.arguments ?? "", | ||
| }, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Merge streamed tool-call deltas by index.
Chat Completions streams one tool call across many chunks. The first delta holds id and function.name. Each later delta holds only an arguments fragment with the same index. collectToolCalls pushes a new ToolCall for every delta. response.completed therefore holds one function call with the name and empty arguments, plus many nameless calls that each hold one argument fragment. Streaming tool use from Responses clients breaks on chat-only models.
🐛 Proposed fix
const collectToolCalls = (
choice: ChatCompletionChunk["choices"][0],
toolCalls: Array<ToolCall>,
) => {
for (const call of choice.delta.tool_calls ?? []) {
- toolCalls.push({
- id: call.id ?? `call_${toolCalls.length}`,
+ const existing = toolCalls[call.index]
+ if (existing) {
+ if (call.id) existing.id = call.id
+ if (call.function?.name) existing.function.name += call.function.name
+ existing.function.arguments += call.function?.arguments ?? ""
+ continue
+ }
+ toolCalls[call.index] = {
+ id: call.id ?? `call_${call.index}`,
type: "function",
function: {
name: call.function?.name ?? "",
arguments: call.function?.arguments ?? "",
},
- })
+ }
}
}📝 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.
| const collectToolCalls = ( | |
| choice: ChatCompletionChunk["choices"][0], | |
| toolCalls: Array<ToolCall>, | |
| ) => { | |
| for (const call of choice.delta.tool_calls ?? []) { | |
| toolCalls.push({ | |
| id: call.id ?? `call_${toolCalls.length}`, | |
| type: "function", | |
| function: { | |
| name: call.function?.name ?? "", | |
| arguments: call.function?.arguments ?? "", | |
| }, | |
| }) | |
| } | |
| } | |
| const collectToolCalls = ( | |
| choice: ChatCompletionChunk["choices"][0], | |
| toolCalls: Array<ToolCall>, | |
| ) => { | |
| for (const call of choice.delta.tool_calls ?? []) { | |
| const existing = toolCalls[call.index] | |
| if (existing) { | |
| if (call.id) existing.id = call.id | |
| if (call.function?.name) existing.function.name += call.function.name | |
| existing.function.arguments += call.function?.arguments ?? "" | |
| continue | |
| } | |
| toolCalls[call.index] = { | |
| id: call.id ?? `call_${call.index}`, | |
| type: "function", | |
| function: { | |
| name: call.function?.name ?? "", | |
| arguments: call.function?.arguments ?? "", | |
| }, | |
| } | |
| } | |
| } |
fix: respect max_completion_tokens and guard missing model capabilities
Summary
Two independent bugs in the chat completions handler, both hit by
real-world clients (observed behind Cherry Studio / gpt-load):
Upstream 400 when clients use
max_completion_tokens.Modern OpenAI clients send the newer
max_completion_tokensfieldinstead of legacy
max_tokens.handleCompletionunconditionallyinjects
max_tokensfrom the model's advertised limits whenevermax_tokensis unset, so upstream receives BOTH parameters andrejects the request:
max_tokens and max_completion_tokens cannot both be set.500 for models without
capabilities.GitHub's
/modelsendpoint lists some entries with NOcapabilitiesobject (observed:gpt-41-copilot,trajectory-compaction). Requesting one crashes the proxy:undefined is not an object (evaluating 'selectedModel?.capabilities.limits.max_output_tokens').Changes
src/routes/chat-completions/handler.tsmax_tokensinjection whenmax_completion_tokensis present; optional-chain the capabilities lookupsrc/services/copilot/create-chat-completions.tsmax_completion_tokens?: number | nulltoChatCompletionsPayloadsrc/services/copilot/get-models.tsModel.capabilitiesmade optional, matching the real APIsrc/lib/tokenizer.tscapabilities?.tokenizerlookuptests/chat-completions.test.tsRelation to #220: that PR solves the same
max_completion_tokensinterop by normalizing the parameter into
max_tokens; this PR takesthe passthrough approach (the Copilot API accepts
max_completion_tokensnatively, so forwarding it preserves clientintent). Happy to drop that half if #220 lands first — the
capabilities fix is orthogonal and not covered by any open PR.
Test plan
bun run typecheck/bun run lint/bun testall greenmax_completion_tokensis forwarded withoutan injected
max_tokensmax_completion_tokens-only request -> 200 (previously 500)gpt-41-copilotwithoutmax_tokens-> clean upstream 4xx, no crashgpt-4.1without either parameter ->max_tokens: 16384still injected (existing behavior preserved)