You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: respect max_completion_tokens and guard missing model capabilities - #275
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_tokens field
instead of legacy max_tokens. handleCompletion unconditionally
injects max_tokens from the model's advertised limits whenever max_tokens is unset, so upstream receives BOTH parameters and
rejects the request: max_tokens and max_completion_tokens cannot both be set.
500 for models without capabilities.
GitHub's /models endpoint lists some entries with NO capabilities object (observed: gpt-41-copilot, trajectory-compaction). Requesting one crashes the proxy: undefined is not an object (evaluating 'selectedModel?.capabilities.limits.max_output_tokens').
Changes
File
Change
src/routes/chat-completions/handler.ts
skip max_tokens injection when max_completion_tokens is present; optional-chain the capabilities lookup
src/services/copilot/create-chat-completions.ts
add max_completion_tokens?: number | null to ChatCompletionsPayload
src/services/copilot/get-models.ts
Model.capabilities made optional, matching the real API
src/lib/tokenizer.ts
guard capabilities?.tokenizer lookup
tests/chat-completions.test.ts
regression tests for both fixes
Relation to #220: that PR solves the same max_completion_tokens
interop by normalizing the parameter into max_tokens; this PR takes
the passthrough approach (the Copilot API accepts max_completion_tokens natively, so forwarding it preserves client
intent). 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 test all green
New regression tests:
payload carrying only max_completion_tokens is forwarded without
an injected max_tokens
requesting a capability-less model no longer throws
Live-tested against a Copilot individual-plan endpoint:
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.
Note
Reviews paused
It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
Walkthrough
The change adds model-session support and adapters for Responses and legacy Completions. It adds POST routes for both protocols and registers standard and /v1 endpoints. Responses-only models can use the Responses API through chat completion creation. The change also updates token-limit handling, tokenizer fallback behavior, and Copilot and API version values.
Priority: ⚪ Not assessed
Merge Risk:🟡 Moderate · up to 64a30
Some Responses clients may treat token-truncated output as complete; malformed null-body requests return 500, and multi-choice proxy requests return merged choices. These are bounded API failures, but the streaming contract issue should be corrected before merge.
Security Architecture Review
Security architecture risk:🟡 Moderate · up to 64a30
The new endpoints expand what callers can do through the server’s shared upstream credentials. Existing request safeguards are applied, but access depends on how the server is exposed, and a translated stream can give clients an inconsistent completion signal.
Retained concerns
Medium · security · inferred: New Responses and Completions endpoints extend the operations reachable with the server’s shared Copilot credential without adding caller authentication at the server router. The actual exposure depends on listener and external access controls; existing Chat Completions and Embeddings routes establish that this is an expansion of an existing trust pattern, not a newly removed control.
Medium · reliability · observed: For a Chat Completions stream terminated by its length limit, the Responses adapter emits a response.completed event whose response status is incomplete. A consumer relying on the terminal event to contain or recover from truncated output receives a contradictory signal.
Security review details
Security Blast Radius
inferred — Any caller able to reach the newly mounted endpoints can request additional operations against Copilot using this process’s credential. The maximum independently attackable scope is the reachable server instance and its configured upstream account; external exposure and caller identity controls remain unverified.
Security Findings and Attack Paths
inferred — A reachable caller can select a model and supply a request to either new protocol; its route then invokes a credentialed upstream service. Rate limiting and conditional approval precede dispatch, and comparable credentialed routes existed before this PR. No verified authentication bypass or cross-origin credential leak is established.
Trust Boundaries and Controls
observed — The new routes apply the shared rate limit and optional operator approval. Responses input markers can influence the upstream X-Initiator value, but Chat Completions already derives that value from caller-supplied message roles; whether it conveys privileged authority rather than protocol metadata is not established.
Resilience and Maintainability Implications
inferred — Wait-mode callers are not serialized by the shared timestamp, limiting how strongly that control can contain concurrent demand on the newly reachable operations. This behavior is inherited from the existing helper, rather than introduced in its implementation by this PR.
Hardening Proposals
proposed — If the listener can be reached by callers outside the intended trusted boundary, enforce caller authorization before credentialed dispatch; define whether the shared rate limit is intended to serialize concurrent requests. Bind cached model sessions to the upstream identity if credentials can rotate while the process runs.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 21 files.
Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name
Status
Explanation
Title check
✅ Passed
The title clearly summarizes both primary fixes: preserving max_completion_tokens and safely handling missing model capabilities.
Description check
✅ Passed
The description directly explains the two fixes, affected files, regression tests, and reported validation results.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
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.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 11
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: c588bb46-3afe-46bc-89e9-85f7d04be650
📥 Commits
Reviewing files that changed from the base of the PR and between 6e3b966 and c2c2cdb.
📒 Files selected for processing (17)
src/lib/api-config.ts
src/lib/state.ts
src/routes/completions/route.ts
src/routes/responses/route.ts
src/server.ts
src/services/copilot/completions-adapter.ts
src/services/copilot/create-chat-completions.ts
src/services/copilot/create-model-session.ts
src/services/copilot/create-proxy-completions.ts
src/services/copilot/create-responses.ts
src/services/copilot/get-models.ts
src/services/copilot/responses-adapter.ts
src/services/github/get-copilot-usage.ts
tests/chat-completions.test.ts
tests/create-chat-completions.test.ts
tests/model-session.test.ts
tests/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 Correctness
Do not flag 2026-01-09 based on the public REST version list.
githubHeaders is used only for GitHub's internal Copilot endpoints: /copilot_internal/v2/token and /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 reject 2026-01-09. No version change is supported by this finding.
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
🟠 Major · Propagate Responses stream failures instead of emitting… · responses-adapter.ts:386-389
src/services/copilot/responses-adapter.ts:386-389 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate Responses stream failures instead of emitting content_filter.
A response.failed or error SSE event can pass through the Responses-only Chat Completions path after an initially successful HTTP response. The adapter currently emits a terminal finish_reason: "content_filter" chunk. The Chat Completions handler writes that chunk as a successful SSE response, so clients can treat an upstream failure as a completed request.
Reviewing files that changed from the base of the PR and between c2c2cdb and 2e64394.
📒 Files selected for processing (12)
src/routes/chat-completions/handler.ts
src/routes/completions/route.ts
src/routes/responses/route.ts
src/services/copilot/completions-adapter.ts
src/services/copilot/create-chat-completions.ts
src/services/copilot/create-model-session.ts
src/services/copilot/create-proxy-completions.ts
src/services/copilot/create-responses.ts
src/services/copilot/responses-adapter.ts
tests/chat-completions.test.ts
tests/completions-adapter.test.ts
tests/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)
src/routes/chat-completions/handler.ts (1)
42-54: LGTM!
tests/chat-completions.test.ts (1)
114-125: LGTM!
src/services/copilot/completions-adapter.ts (1)
22-27: LGTM!
Also applies to: 40-40, 47-66, 84-89
src/routes/completions/route.ts (1)
43-53: LGTM!
tests/completions-adapter.test.ts (1)
1-66: LGTM!
src/services/copilot/responses-adapter.ts (2)
732-732: Guard call.index against non-array-index values.
call.index < toolCalls.length is true for a negative index. toolCalls[-1] is then undefined, so the code falls through. The while loop is skipped, and a new call is pushed. This case is unlikely from a well-behaved upstream. The behavior is safe but wrong for that input. No fix is required.
46-77: LGTM!
Also applies to: 102-108, 245-246, 261-261, 429-430, 548-559, 571-584, 630-647, 729-750
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 3a988130-f9be-4080-a478-c7ba810f878f
📥 Commits
Reviewing files that changed from the base of the PR and between 2e64394 and 23778b3.
📒 Files selected for processing (4)
src/routes/completions/route.ts
src/routes/responses/route.ts
src/services/copilot/responses-adapter.ts
tests/responses-adapter.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
When the upstream emits multiple choices for n > 1, this loop appends every choice's text into one string and the returned object always exposes only one choice at index 0. Because n is accepted and forwarded by CompletionsPayload, non-streaming legacy requests lose independent completions; accumulate by choice.index and return one result per choice.
Response stream omits the OpenAI DONE sentinel
src/services/copilot/responses-adapter.ts:499
The response-only chat stream never emits the OpenAI [DONE] sentinel: after response.completed this return exits after the JSON chunks, and the EOF fallback below also emits only a final JSON chunk. handleCompletion writes these events directly, so clients that wait for [DONE] can hang or fail to mark the completion finished. Emit the sentinel on both the normal-completion and fallback paths, and update the stream regression test to ignore it.
This issue also appears on line 713 of the same file.
PR scope omits substantial API and adapter changes
src/server.ts:10
This change adds two public API routes plus the Responses/completions adapters, proxy integration, model-session flow, and protocol translations, while the PR description scopes the work to two chat-completion fixes and lists only five changed files. Please split this unrelated feature work or update the PR scope and test/compatibility documentation before approval; otherwise the stated review scope does not cover these new behaviors.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Unresolved protocol adapter, validation, streaming, and session-path issues remain.
Review effort: Lite Findings: None
Previously missed (3)
In code that hasn't changed since last review
Preserve response_format in Responses translation
src/services/copilot/responses-adapter.ts:94
The chat payload's response_format is silently dropped on this translation path. A client requesting response_format: { type: "json_object" } for a Responses-only model will therefore receive no JSON-mode constraint, unlike the normal chat path. Map it to the Responses API's text.format (or reject it explicitly) instead of forwarding a request that changes the client's requested behavior.
Emit streamed function-call events
src/services/copilot/responses-adapter.ts:800
Tool calls are collected in state.toolCalls, but the streamed Responses event sequence never emits them: this only closes the message item and then puts function calls inside response.completed. Responses clients consume function calls through response.output_item.added and response.function_call_arguments.* events, so a streamed tool-use request can finish without any tool call being delivered incrementally. Emit the corresponding function-call item/argument events (and its output_item.done) before response.completed.
Split unrelated API feature work or expand PR scope
src/server.ts:25
The PR is described as two targeted fixes and lists only the handler/model/tokenizer changes, but this hunk also introduces public /completions and /responses APIs, protocol adapters, proxy-engine routing, model-session state, and an upstream API-version change. Please split this unrelated feature work or update the PR scope and validation plan before approval; otherwise the stated review/test scope does not cover the behavior being shipped.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
🟡 Minor · Preserve each indexed choice when aggregating proxy… · create-proxy-completions.ts:41-71
src/services/copilot/create-proxy-completions.ts:41-71 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve each indexed choice when aggregating proxy completions.
When a non-streaming request for a -copilot model sets n greater than one, the route forwards n to the proxy and aggregates the streaming response. The aggregator appends text from every indexed choice to one string, then returns only choice index: 0. Clients receive concatenated completions instead of the separate choices required by the legacy Completions response.
Suggested fix
- let text = ""- let finishReason: string | null = null+ const choices = new Map<+ number,+ { text: string; finishReason: string | null }+ >()
for await (const event of events(response)) {
if (!event.data) continue
if (event.data === "[DONE]") break
const chunk = JSON.parse(event.data) as LegacyCompletionChunk
if (chunk.id) id = chunk.id
for (const choice of chunk.choices ?? []) {
- if (typeof choice.text === "string") text += choice.text- if (choice.finish_reason) finishReason = choice.finish_reason+ const aggregate = choices.get(choice.index) ?? {+ text: "",+ finishReason: null,+ }+ if (typeof choice.text === "string") aggregate.text += choice.text+ if (choice.finish_reason) aggregate.finishReason = choice.finish_reason+ choices.set(choice.index, aggregate)
}
}
return {
@@
- choices: [- { text, index: 0, finish_reason: finishReason ?? "stop", logprobs: null },- ],+ choices:+ choices.size > 0 ?+ [...choices.entries()]+ .sort(([left], [right]) => left - right)+ .map(([index, choice]) => ({+ text: choice.text,+ index,+ finish_reason: choice.finishReason ?? "stop",+ logprobs: null,+ }))+ : [{ text: "", index: 0, finish_reason: "stop", logprobs: null }],
}
}
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 33fda2ba-6c5a-4e7f-b6a2-451121024460
📥 Commits
Reviewing files that changed from the base of the PR and between 23778b3 and 64a302c.
📒 Files selected for processing (5)
src/routes/completions/route.ts
src/routes/embeddings/route.ts
src/routes/responses/route.ts
src/services/copilot/responses-adapter.ts
tests/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.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Unresolved moderate issues affect token forwarding and Responses API correctness.
Review effort: Lite Findings: None
Previously missed (2)
In code that hasn't changed since last review
Preserve response.incomplete as a length finish reason
src/services/copilot/responses-adapter.ts:420
response.incomplete is the Responses API terminal event for max-output truncation, but only response.completed is converted here. When a Responses-only model hits its output limit, this event is ignored and the fallback at the end emits finish_reason: "stop", so Chat Completions clients see a successful completion instead of length. Handle response.incomplete with the same conversion path so the status is preserved.
Align PR scope with added APIs and protocol changes
src/server.ts:34
The description presents this as two targeted chat-completions fixes and lists only five affected files, but this diff also adds and registers full /completions and /responses APIs, model-session handling, and protocol/header changes. That is a materially broader behavioral and operational change than the stated scope; please either update the PR description and test/rollout plan or split the unrelated additions into a separate PR.
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)