Repository navigation
Conversation
…l continuation - Abort upstream Copilot requests when the client disconnects during streaming - Configure undici connection pool (16 connections per origin) - Skip debug JSON serialization unless debug logging is enabled - Append a 'Continue.' user message when the conversation ends with an assistant turn Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughDirect connections use a shared agent configuration when environment proxying is disabled. The chat-completions and messages handlers select streaming or non-streaming behavior from the request setting, pass abort signals for streamed requests, and gate debug logs by log level. Copilot requests retry once after a 401 or 403 response by refreshing the token. Message translation appends a user message containing "Continue." for qualifying assistant-ending conversations. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
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 8 functions across 9 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the main changes: aborting upstream requests, pooling connections, and handling assistant prefill. Description check The description directly explains the implemented changes, affected behavior, testing, and known limitations. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 ESLint
-
If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
src/lib/token.ts
-
ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.
src/services/copilot/create-chat-completions.ts
-
ESLint 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 @coderabbitai help to get the list of available commands.
mingqi
changed the title
![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
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
0dc4141b-7209-46b7-83f7-6f91c112e282
📒 Files selected for processing (8)
src/lib/proxy.tssrc/routes/chat-completions/handler.tssrc/routes/messages/handler.tssrc/routes/messages/non-stream-translation.tssrc/services/copilot/create-chat-completions.tssrc/start.tstests/anthropic-request.test.tstests/create-chat-completions.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 (6)
src/lib/proxy.ts (1)
5-21: LGTM!Also applies to: 27-27
src/start.ts (1)
10-10: LGTM!Also applies to: 33-34
src/routes/messages/non-stream-translation.ts (1)
71-78: LGTM!tests/anthropic-request.test.ts (1)
157-161: LGTM!Also applies to: 163-182
src/services/copilot/create-chat-completions.ts (1)
3-3: LGTM!Also applies to: 11-11, 33-33, 37-43, 51-51
tests/create-chat-completions.test.ts (1)
58-69: LGTM!
| controller.abort() | ||
| }) | ||
|
|
||
| const response = await createChatCompletions(payload, controller.signal) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve upstream failures before returning an SSE response. Both routes now await the Copilot request inside the streamSSE callback. If the token is missing or Copilot rejects the request, Hono has already returned the SSE response. Hono then writes an error event instead of allowing an HTTP error response. (github.com)
src/routes/chat-completions/handler.ts#L62-L62: await the upstream connection before callingstreamSSE; retain the controller for disconnect cancellation.src/routes/messages/handler.ts#L58-L61: make the same change before starting the translated SSE response.
📍 Affects 2 files
src/routes/chat-completions/handler.ts#L62-L62(this comment)src/routes/messages/handler.ts#L58-L61
The Copilot token can be revoked server-side before its scheduled refresh, leaving the proxy returning 403 forbidden until restart. On 401/403 from chat/completions, fetch a fresh Copilot token (deduplicated across concurrent requests) and retry once. Also stop the periodic refresh from rethrowing inside setInterval. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
7229c57d-9ad2-41d6-b1b5-1d81efa9aea1
📒 Files selected for processing: 2
src/lib/token.tssrc/services/copilot/create-chat-completions.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 context used
🪛 ast-grep (0.45.3)
src/lib/token.ts
[warning] 53-53: Avoid logging sensitive data
Context: consola.info("Refreshed Copilot token:", token)
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data-typescript)
🔇 Additional comments: 2
src/lib/token.ts: 1
31-37: LGTM!Also applies to: 39-59
src/services/copilot/create-chat-completions.ts: 1
8-8: LGTM!Also applies to: 28-44
| try { | ||
| await refreshCopilotToken() | ||
| await response.body?.cancel() | ||
| response = await send() | ||
| } catch (error) { | ||
| consola.error("Failed to refresh Copilot token:", error) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Limit the try/catch to refreshCopilotToken(). Let errors from the retried send() propagate.
The catch block also handles errors from response.body?.cancel() and from the retried send(). Here is the failure sequence:
- The refresh succeeds.
- Line 56 cancels the body of the first 401/403
response. - The retried
send()rejects. This happens on a network error, or with anAbortErrorwhen the client disconnects during the refresh. - Line 59 logs "Failed to refresh Copilot token", which is wrong because the refresh succeeded.
responsestill points to the first 401/403 response, whose body is already cancelled.- Line 65 throws
HTTPErrorwith that response. A downstream error handler that readserror.response.text()or.json()then fails on the cancelled body.
The result is that the real error (network failure or abort) is lost. The client gets a misleading 401/403 or an error-handler failure. The route handlers check for error.name !== "AbortError" to skip logging aborts, so an AbortError swallowed here also gets logged as a token-refresh failure.
Catch only the refresh failure. If the refresh fails, keep the first response and do not cancel its body.
🐛 Proposed fix
--- "a/src/services/copilot/create-chat-completions.ts"
+++ "b/src/services/copilot/create-chat-completions.ts"
@@ -51,13 +51,17 @@
consola.warn(
`Copilot returned ${response.status}, refreshing Copilot token and retrying`,
)
- try {
- await refreshCopilotToken()
- await response.body?.cancel()
- response = await send()
- } catch (error) {
- consola.error("Failed to refresh Copilot token:", error)
- }
+ let refreshed = false
+ try {
+ await refreshCopilotToken()
+ refreshed = true
+ } catch (error) {
+ consola.error("Failed to refresh Copilot token:", error)
+ }
+ if (refreshed) {
+ await response.body?.cancel()
+ response = await send()
+ }
}
if (!response.ok) {📝 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.
| try { | |
| await refreshCopilotToken() | |
| await response.body?.cancel() | |
| response = await send() | |
| } catch (error) { | |
| consola.error("Failed to refresh Copilot token:", error) | |
| } | |
| let refreshed = false | |
| try { | |
| await refreshCopilotToken() | |
| refreshed = true | |
| } catch (error) { | |
| consola.error("Failed to refresh Copilot token:", error) | |
| } | |
| if (refreshed) { | |
| await response.body?.cancel() | |
| response = await send() | |
| } |
Summary
This PR fixes three problems in how the proxy handles requests to Copilot, and adds one small performance change.
1. Upstream requests are aborted when the client disconnects
Problem: If a client cancelled a streaming request (for example, pressed Esc in Claude Code), the proxy kept reading the Copilot stream until it finished. That wasted quota and kept connections open.
Fix:
createChatCompletionsnow accepts an optionalAbortSignaland passes it to bothfetchandevents()./v1/chat/completionsand/v1/messagescreate anAbortControllerinsidestreamSSE. It is aborted fromstream.onAbort()and in afinallyblock.AbortErroris no longer logged as an error. Other streaming errors are still logged.2. HTTP connection pooling (Node only)
Problem: The default undici
Agentwas used without explicit limits.Fix: Added
initDirectConnectionPool(), which sets a global undiciAgentwith 16 connections per origin andpipelining: 1. It runs when--proxy-envis not set. The direct agent insideinitProxyFromEnvuses the same settings. This is skipped under Bun, as before.3. Assistant prefill / conversations that end on an assistant turn
Problem: Anthropic clients can send a conversation whose last message is from the assistant (a "prefill"). Copilot's OpenAI-style endpoint does not handle that, so the request either fails or returns an empty or unexpected completion.
Fix: When the translated conversation ends with an assistant message that has no tool calls, a
{ role: "user", content: "Continue." }message is added at the end.4. Debug logging is cheaper
JSON.stringifyof payloads, chunks, and responses now runs only whenconsola.level >= 4(debug). Before this, every streamed chunk was serialized even when debug logging was off.Testing
tests/anthropic-request.test.ts: added a test for assistant prefill, and updated the existing thinking-block test to expect the trailingContinue.message.tests/create-chat-completions.test.ts: added a test that checks the abort signal is passed to the upstreamfetch.bun test,bun run lintNotes
🤖 Generated with Claude Code