feat(chat): native chat->chat for openai-chat providers (#1467) - #1569
feat(chat): native chat->chat for openai-chat providers (#1467)#1569dbc-hbin wants to merge 8 commits into
Conversation
github-actions
Bot
added
the
intake: hygiene-blocked
|
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEligible ChangesChat-native routing
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🟠 High · up to The new native chat path can violate the Chat wire contract, turn upstream errors or malformed payloads into successful responses, skip required provider credential handling, expose sensitive error content, and lose streaming usage accounting. These current-head correctness, security, and observability risks should be fixed before merge. Sequence Diagram(s)sequenceDiagram
participant ChatClient
participant ChatCompletionsHandler
participant ChatNativeProvider
participant UpstreamChatEndpoint
participant RequestLogger
ChatClient->>ChatCompletionsHandler: POST /v1/chat/completions
ChatCompletionsHandler->>ChatNativeProvider: resolve eligible native route
ChatNativeProvider->>UpstreamChatEndpoint: send normalized Chat request
UpstreamChatEndpoint-->>ChatNativeProvider: JSON or SSE response
ChatNativeProvider->>RequestLogger: record attempts and outcome
ChatNativeProvider-->>ChatCompletionsHandler: normalized Chat response
ChatCompletionsHandler-->>ChatClient: JSON or SSE response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🤖 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 `@src/server/chat-completions.ts`:
- Around line 551-556: Remove the eager recordFirstOutput call from the native
response-construction path. Add a noteFirstOutput callback to the normalizer and
invoke it immediately before each controller.enqueue, and wrap the outStream =
upstreamBody passthrough so its first read chunk triggers the same callback;
preserve first-output deduplication and existing cleanup/final logging behavior.
- Around line 565-573: Update the native non-streaming response handling around
response.text() for both SSE and JSON bodies to use an incremental,
translatorBudget-aware reader, charging each chunk and stopping when the budget
is exceeded. Reuse the existing shared collection/budget mechanism where
possible, and catch isTranslatorBudgetExceededError so the handler returns HTTP
413 with translation_buffer_limit, matching request-side behavior.
- Around line 339-346: Update the retry-delay logic around the inline Promise to
remove the `abort` listeners from both `ac.signal` and `req.signal` on every
exit path, including timer resolution and abort rejection. Prefer reusing
`sleepWithAbort` with a combined `AbortSignal.any` signal if compatible with the
existing retry behavior.
- Around line 205-210: In the route selection flow around
isChatNativeEligibleProvider and shouldBridgeChatNative, remove the as unknown
as ChatNativeRoute double cast and make the assigned route structurally
type-safe. Ensure the local ChatNativeRoute shape exposes a mutable provider
field so the later routeInfo.provider reassignment no longer needs its separate
cast, while preserving the existing native routing behavior.
- Around line 263-274: Update the native provider request header construction
around upstreamHeaders to start from a clean Headers instance rather than
cloning the Responses-bridge headers. Preserve only explicitly approved inbound
headers needed by Chat providers, never forwarding authorization or
chatgpt-account-id, then apply the configured provider API key and
providerConfig.headers as before.
- Around line 488-503: Update the ReadableStream pull handler around
reader.read(), buffer, and the per-frame processing logic to flush
decoder.decode() and process any remaining buffer when done is true before
enqueueing [DONE] and closing. Extract or reuse the existing frame-normalization
logic so the trailing SSE frame receives the same handling as regular frames,
including usage and finish_reason data.
- Around line 575-597: Replace the lossy native SSE fold in the surrounding
chat-completion handler with the existing collectChatCompletion accumulator,
adapting the normalized ReadableStream<Uint8Array> as needed. Preserve
tool_calls, reasoning_content, refusal, role, usage, and upstream finish_reason
values in the returned response, while retaining the existing 502 handling when
stream collection fails.
- Around line 139-157: Remove the unconditional raw Chat body charge before the
routing branch, and move it into the native path after `bodyJson` is created in
the native handler. Charge the native serialized body once using
`bodyJson.length`, preserving the existing overflow response shape while leaving
bridge handling and its `internalBodyJson` charge unchanged.
- Around line 288-289: Align URL construction in the native chat-completions
flow with the rule used by openai-chat: preserve the configured base URL’s
trailing slash when appending /chat/completions, or otherwise update both
construction paths consistently. Modify the URL-building logic near
providerConfig.baseUrl and the corresponding openai-chat implementation so
identical baseUrl values produce identical request paths.
- Around line 363-395: Update the 429 failover loop around
rotateProviderTransportOn429 to track the currently attempted API key and count
rotations. Limit rotations to at most apiKeyPool.length, pass the current key as
attemptedKey, and update it to nextProvider.apiKey after each successful
rotation so the same key is not retried indefinitely.
- Around line 94-96: Update isChatNativeEligibleProvider to return true only
when the adapter is "openai-chat" and authMode is undefined, "key", or "local";
ensure OAuth providers are excluded from the native path and routed through the
bridge.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65f142cf-e608-453b-9be9-494aa5376dc3
📒 Files selected for processing (1)
src/server/chat-completions.ts
github-actions
Bot
added
review-ready
and removed
intake: hygiene-blocked
|
Tip For best results, initiate chat on the files or code changes.
I am replying to the latest available comment. The earlier comment chain is not available in this context. 🧠 Learnings usedYou are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedYou are interacting with an AI system. |
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 `@src/server/chat-completions.ts`:
- Around line 679-713: In the bounded JSON read around
response.body.getReader(), retain the reader outside the try block and
explicitly cancel it in the catch handler before cleanup() and every return,
including translator-budget overflow and read failures. Preserve the existing
status responses and cleanup behavior while ensuring cancellation is attempted
when a reader was acquired.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bb4d73d1-a9bf-4496-94c2-069c93e51115
📒 Files selected for processing (2)
src/server/chat-completions.tstests/chat-completions-endpoint.test.ts
|
🧠 Learnings used
|
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The direction is valuable, but the current exact head c89a2fdf5352f4aa5fe4742dc744cb1376c2776b still has one concrete resource-lifecycle blocker in src/server/chat-completions.ts:679-713: after response.body.getReader(), a translator-budget exception or reader.read() rejection reaches the catch path without cancelling the locked reader/body. Repeated oversized or failed JSON responses can therefore retain upstream sockets until GC. Keep the reader outside the try, await reader.cancel() best-effort before cleanup on every catch return, and add a regression proving cancellation on budget overflow/read failure. Also rerun full exact-head CI; the current head only has lightweight hygiene/target checks, not the cross-platform suite. Once that is fixed and exact-head CI is green, this remains a strong merge candidate for #1467.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files and diff stat ==='
git diff --stat HEAD^ HEAD
git diff --name-status HEAD^ HEAD
echo '=== Outline: server implementation ==='
ast-grep outline src/server/chat-completions.ts --items all --view expanded
echo '=== Changed hunks: server implementation ==='
git diff --unified=80 HEAD^ HEAD -- src/server/chat-completions.ts
echo '=== Changed hunks: endpoint tests ==='
git diff --unified=50 HEAD^ HEAD -- tests/chat-completions-endpoint.test.tsLength of output: 20791 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Native branch and response handling ==='
sed -n '117,690p' src/server/chat-completions.ts
echo '=== Retry and rotation helper definitions and call sites ==='
rg -n -C 5 \
'function (rotateProviderTransportOn429|hasKeyPoolFailover|rateLimitRetryPolicyFor|rateLimitRetryDelayMs|sleepWithAbort|linkAbortSignal)|const (rotateProviderTransportOn429|hasKeyPoolFailover|rateLimitRetryPolicyFor|rateLimitRetryDelayMs)|\brotateProviderTransportOn429\s*\(|\bhasKeyPoolFailover\s*\(' \
src tests
echo '=== Relevant test definitions and 429 coverage ==='
ast-grep outline tests/chat-completions-endpoint.test.ts --items all --view expanded
rg -n -C 8 \
'429|rate.?limit|apiKeyPool|key.?pool|abort|non.?JSON|translatorBudget|tool_calls|finish_reason' \
tests/chat-completions-endpoint.test.ts testsLength of output: 201357 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Imports and exact native path ==='
sed -n '1,80p' src/server/chat-completions.ts
sed -n '220,470p' src/server/chat-completions.ts
echo '=== Definitions in source files only ==='
rg -n -C 12 \
'export (function|const) (rotateProviderTransportOn429|hasKeyPoolFailover|rateLimitRetryPolicyFor|rateLimitRetryDelayMs|sleepWithAbort|linkAbortSignal)|function (rotateProviderTransportOn429|hasKeyPoolFailover|rateLimitRetryPolicyFor|rateLimitRetryDelayMs|sleepWithAbort|linkAbortSignal)' \
src
echo '=== Initial header construction and native eligibility ==='
sed -n '80,250p' src/server/chat-completions.ts
echo '=== Focused endpoint test range ==='
sed -n '600,735p' tests/chat-completions-endpoint.test.tsLength of output: 37886 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Key-failover helper ==='
sed -n '130,285p' src/providers/key-failover.ts
echo '=== Responses core key rotation call sites ==='
rg -n -C 35 \
'rotateProviderTransportOn429\(|maxRotations|key-429|rate-limit-429|sameTargetRetries' \
src/server/responses/core.ts
echo '=== Chat-native line numbers for changed behavior ==='
nl -ba src/server/chat-completions.ts | sed -n '300,445p'
echo '=== Recent commit metadata ==='
git show --format=fuller --no-ext-diff --stat HEADLength of output: 32585
[P1] Preserve transient-retry and transport-failure handling for 429 replays The initial request uses If the retry connection fails, for example due to a socket reset or a transient 5xx recovery condition,
Wrap both native replay fetches in 🧠 Learnings usedYou are interacting with an AI system. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head eb925fa04074275dde2b64041f43a2524082eafa: the reader is now retained and cancelled best-effort on the bounded JSON catch path, so the resource-lifecycle blocker is resolved. I am not approving yet because this new head still lacks a focused regression that proves cancellation on translator-budget overflow/read failure, and the exact-head cross-platform CI suite has not run. Add that regression and obtain green exact-head CI; no further product-code blocker is identified in this update.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/server/chat-completions.ts (10)
583-586: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecord first output for already-normalized chunks.
The proper
chat.completion.chunkbranch enqueues directly without callingnoteSseFirstOutput(). The common upstream path uses this envelope, so native streaming TTFT can remain unset.Proposed fix
if (rec.object === "chat.completion.chunk") { + noteSseFirstOutput(); controller.enqueue(encoder.encode(`data: ${JSON.stringify(rec)}\n\n`)); continue; }🤖 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 `@src/server/chat-completions.ts` around lines 583 - 586, Update the chat.completion.chunk branch in the record-forwarding flow to call noteSseFirstOutput() before enqueueing the normalized chunk, ensuring native streaming records their first output timestamp while preserving the existing direct forwarding behavior.
277-289: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winFinish the request attempt when body charging fails.
When
translatorBudget.chargeRetained()throws, this return writes the final request log but never callsfinishRequestAttempt(). The attempt created at Line 242 remains active without a status or duration.Finish the attempt before returning the 413 or 500 response.
Proposed fix
} catch (err) { const overflow = isTranslatorBudgetExceededError(err); const status = overflow ? 413 : 500; + finishRequestAttempt( + attempt, + status, + Date.now() - (logCtx.activeAttemptStartedAt ?? Date.now()), + ); if (logIds) addFinalRequestLog(logIds.requestId, logIds.start, logCtx, status, { closeReason: "non_stream" });🤖 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 `@src/server/chat-completions.ts` around lines 277 - 289, In the catch block handling translatorBudget.chargeRetained() within the request flow, call finishRequestAttempt() with the computed 413 or 500 status before returning chatCompletionsErrorResponse. Keep the existing final request logging and response behavior unchanged.
716-731: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn an error for malformed native JSON.
If
JSON.parse()fails,parsedJsonbecomesnull, but the handler still returns HTTP 200 with the raw body and marks the attempt successful. A non-JSON, empty, or invalid Chat payload therefore appears to be a successful completion.Reject parse failures and invalid success shapes with a 502 response. Finish the attempt and request log with the error status.
🤖 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 `@src/server/chat-completions.ts` around lines 716 - 731, Update the native JSON response handling around parsedJson so JSON.parse failures and invalid success payloads are rejected with HTTP 502 instead of returning the raw body with status 200. Validate the parsed result against the expected success shape before usage extraction, and ensure finishRequestAttempt and request logging use the error status while preserving cleanup.
579-604: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTreat upstream SSE error envelopes as errors.
A payload such as
{ "error": { ... } }is not achat.completion.chunk, so this code treats it as a minimal delta with empty choices. The handler then emits[DONE], making the client observe a successful empty completion.Detect
rec.errorbefore normalizing. Reuse the repository’s error-SSE behavior and terminate the stream without reporting a successful completion.🤖 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 `@src/server/chat-completions.ts` around lines 579 - 604, In the SSE payload handling near the existing chat.completion.chunk check, detect rec.error before constructing the normalized chunk. Reuse the repository’s established error-SSE behavior to enqueue the appropriate error event and terminate the stream, ensuring noteSseFirstOutput and [DONE] are not emitted for upstream errors.
508-564: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse a bounded, CRLF-tolerant SSE frame parser.
The local buffer has no maximum and splits only on
\n\n. A valid\r\n\r\nstream remains inbuffer; at EOF,tailcontains multiple frames,JSON.parse()fails, and the client receives only[DONE]. A delimiter-free or oversized event can also grow the buffer without a translator-budget charge.Reuse the repository’s bounded SSE framing implementation, or normalize line endings and enforce a maximum frame size before enqueueing.
Based on learnings:
BoundedSseFrameBuffer.takeCandidate()releases each completed frame to avoid retaining rare multi-MiB allocations.🤖 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 `@src/server/chat-completions.ts` around lines 508 - 564, Replace the ad hoc buffer splitting in the ReadableStream pull logic with the repository’s bounded SSE framing implementation, using takeCandidate() to release each completed frame. Ensure parsing accepts both LF and CRLF delimiters, enforces the maximum frame size before enqueueing, and drains all complete frames at EOF so multiple trailing events are forwarded correctly before the final [DONE].Source: Learnings
437-449: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact parsed provider messages before returning them.
The fallback raw body is redacted, but
nested?.messageandflatare assigned directly tomessage. Line 473 then serializes the value into the client response.If a provider echoes an API key or token in its error message, this path exposes it to the Chat client.
Proposed fix
- message = nested?.message || flat || fallback; + message = redactSecretString(nested?.message || flat || fallback);As per path instructions: tokens and OAuth material must never be logged or serialized into responses.
🤖 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 `@src/server/chat-completions.ts` around lines 437 - 449, Redact provider-supplied error messages before assigning them to the client-facing message in the response parsing flow around nested and flat error extraction. Apply redactSecretString to nested?.message and flat, while preserving the existing fallback redaction and upstreamType/upstreamCode handling in the surrounding response error logic.Source: Path instructions
425-451: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUse a bounded, abort-aware reader for non-2xx bodies.
The non-OK path calls
response.text()without a translator-budget limit. It also callscleanup()before the read, so client cancellation no longer reaches this body read.If an upstream error body is large or never completes, this path can consume excessive memory or hang. Reuse the bounded reader used at Lines 677-700, cancel on overflow or read failure, and call
cleanup()after body consumption.🤖 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 `@src/server/chat-completions.ts` around lines 425 - 451, Update the non-OK response handling around the response.text() call to use the existing bounded, abort-aware reader from the later response path, including its translator-budget limit and cancellation on overflow or read failure. Move cleanup() until after body consumption completes, while preserving the current error parsing and fallback behavior.
481-487: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftClassify the upstream body before selecting the response format.
shouldStreambecomes true for any response body when the client requested streaming, but normalization depends only ontext/event-stream.If an upstream returns JSON with
stream: true, the handler labels raw JSON astext/event-stream. If an upstream sends SSE without that header, streaming bypasses normalization and non-streaming requests bypasscollectChatCompletion().Use a replayable body peek or enforce the upstream media-type contract. Handle JSON responses like the existing bridge path and send headerless SSE through the same normalizer.
Also applies to: 638-640
🤖 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 `@src/server/chat-completions.ts` around lines 481 - 487, Update the response-format selection around shouldStream to classify the upstream body before choosing streaming or JSON handling. Use a replayable body peek or enforce the upstream media-type contract so JSON responses with stream=true follow the existing JSON/bridge normalization, while headerless SSE is passed through collectChatCompletion(); ensure content-type labeling matches the actual selected format.
236-277: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse the
openai-chatprovider builder for native requests.The native branch serializes
rawChatdirectly and only rewrites the model, stream flag, store flag, and one structured-output case. It bypassesbuildRequestinsrc/adapters/openai-chat.ts, Lines 824-1005.That builder applies message and tool normalization, model-specific limits, reasoning fields, structured-output policy,
parallel_tool_calls, provider headers, andstream_options.include_usage. Native requests can therefore miss required provider fields or send fields that a provider rejects.Create a shared Chat wire builder and use it here. Keep the request Chat-native, but do not bypass provider-specific normalization.
As per path instructions:
src/**changes must not bypass provider/adapter contracts or shared routing/config layers.🤖 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 `@src/server/chat-completions.ts` around lines 236 - 277, The chat-native branch must stop constructing its payload directly from rawChat and instead reuse the normalization performed by the openai-chat adapter’s buildRequest flow. Extract the applicable Chat wire-building logic from buildRequest into a shared builder, preserving the native protocol while applying message/tool normalization, model limits, reasoning and structured-output policies, parallel_tool_calls, provider headers, and stream usage options; then invoke that builder from the chatNativeRoute branch and retain only native-specific routing behavior.Source: Path instructions
546-601: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd
stream_options.include_usageto native Chat requests.The native path in
src/server/chat-completions.ts:276-277bypassessrc/adapters/openai-chat.ts:976and does not request usage frames. Preserve existingstream_optionsfields and setinclude_usage: true; otherwise streamed requests can finish without actual usage data for the request log.🤖 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 `@src/server/chat-completions.ts` around lines 546 - 601, Update the native Chat request construction near the native path to preserve all existing stream_options fields while setting include_usage to true. Ensure the resulting request always asks for a usage frame so the existing response parsing and request logging receive actual usage data.
🤖 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.
Outside diff comments:
In `@src/server/chat-completions.ts`:
- Around line 583-586: Update the chat.completion.chunk branch in the
record-forwarding flow to call noteSseFirstOutput() before enqueueing the
normalized chunk, ensuring native streaming records their first output timestamp
while preserving the existing direct forwarding behavior.
- Around line 277-289: In the catch block handling
translatorBudget.chargeRetained() within the request flow, call
finishRequestAttempt() with the computed 413 or 500 status before returning
chatCompletionsErrorResponse. Keep the existing final request logging and
response behavior unchanged.
- Around line 716-731: Update the native JSON response handling around
parsedJson so JSON.parse failures and invalid success payloads are rejected with
HTTP 502 instead of returning the raw body with status 200. Validate the parsed
result against the expected success shape before usage extraction, and ensure
finishRequestAttempt and request logging use the error status while preserving
cleanup.
- Around line 579-604: In the SSE payload handling near the existing
chat.completion.chunk check, detect rec.error before constructing the normalized
chunk. Reuse the repository’s established error-SSE behavior to enqueue the
appropriate error event and terminate the stream, ensuring noteSseFirstOutput
and [DONE] are not emitted for upstream errors.
- Around line 508-564: Replace the ad hoc buffer splitting in the ReadableStream
pull logic with the repository’s bounded SSE framing implementation, using
takeCandidate() to release each completed frame. Ensure parsing accepts both LF
and CRLF delimiters, enforces the maximum frame size before enqueueing, and
drains all complete frames at EOF so multiple trailing events are forwarded
correctly before the final [DONE].
- Around line 437-449: Redact provider-supplied error messages before assigning
them to the client-facing message in the response parsing flow around nested and
flat error extraction. Apply redactSecretString to nested?.message and flat,
while preserving the existing fallback redaction and upstreamType/upstreamCode
handling in the surrounding response error logic.
- Around line 425-451: Update the non-OK response handling around the
response.text() call to use the existing bounded, abort-aware reader from the
later response path, including its translator-budget limit and cancellation on
overflow or read failure. Move cleanup() until after body consumption completes,
while preserving the current error parsing and fallback behavior.
- Around line 481-487: Update the response-format selection around shouldStream
to classify the upstream body before choosing streaming or JSON handling. Use a
replayable body peek or enforce the upstream media-type contract so JSON
responses with stream=true follow the existing JSON/bridge normalization, while
headerless SSE is passed through collectChatCompletion(); ensure content-type
labeling matches the actual selected format.
- Around line 236-277: The chat-native branch must stop constructing its payload
directly from rawChat and instead reuse the normalization performed by the
openai-chat adapter’s buildRequest flow. Extract the applicable Chat
wire-building logic from buildRequest into a shared builder, preserving the
native protocol while applying message/tool normalization, model limits,
reasoning and structured-output policies, parallel_tool_calls, provider headers,
and stream usage options; then invoke that builder from the chatNativeRoute
branch and retain only native-specific routing behavior.
- Around line 546-601: Update the native Chat request construction near the
native path to preserve all existing stream_options fields while setting
include_usage to true. Ensure the resulting request always asks for a usage
frame so the existing response parsing and request logging receive actual usage
data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 575c5fd7-4bd8-4f21-af2d-2ef6b2b49b38
📒 Files selected for processing (1)
src/server/chat-completions.ts
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current PR head ==='
git rev-parse HEAD
git show --no-patch --format='%H%n%s' HEAD
echo '=== Native 429 replay implementation ==='
nl -ba src/server/chat-completions.ts | sed -n '330,450p'
echo '=== Relevant helpers and initial request error handling ==='
rg -n -C 8 \
'fetchWithTransientRetry\s*\(|fetchWithHeaderTimeout\s*\(|transportFailureResponse\s*\(|sameTargetRetries|rotateProviderTransportOn429' \
src/server/chat-completions.ts
echo '=== Focused regression tests ==='
rg -n -C 12 \
'same-target|sameTarget|rotated-key|rotate|429|socket|ECONNRESET|transportFailureResponse|fetchWithTransientRetry' \
tests/chat-completions-endpoint.test.ts
echo '=== Changed test hunks at current head ==='
git diff --unified=50 HEAD^ HEAD -- tests/chat-completions-endpoint.test.tsLength of output: 1389 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Native 429 replay implementation (source lines 330-450) ==='
sed -n '330,450p' src/server/chat-completions.ts
echo '=== Relevant helper calls and error handling ==='
rg -n -C 8 \
'fetchWithTransientRetry\s*\(|fetchWithHeaderTimeout\s*\(|transportFailureResponse\s*\(|sameTargetRetries|rotateProviderTransportOn429' \
src/server/chat-completions.ts
echo '=== Focused endpoint test coverage ==='
rg -n -C 12 \
'same-target|sameTarget|rotated-key|rotate|429|socket|ECONNRESET|transportFailureResponse|fetchWithTransientRetry' \
tests/chat-completions-endpoint.test.ts
echo '=== Current-head endpoint-test change ==='
git diff --unified=50 HEAD^ HEAD -- tests/chat-completions-endpoint.test.tsLength of output: 12440
[P1] Handle transport failures during native 429 replays Both the same-target 429 replay and the rotated-key 429 replay still call If either replay fails due to a connection error, Use 🧠 Learnings usedYou are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current commit ==='
git rev-parse HEAD
git show --no-patch --format='%H%n%s' HEAD
echo '=== Native 429 replay paths ==='
nl -ba src/server/chat-completions.ts | sed -n '300,430p'
echo '=== Retry and transport-failure helper use in the native handler ==='
rg -n -C 7 \
'fetchWithTransientRetry\s*\(|fetchWithHeaderTimeout\s*\(|transportFailureResponse\s*\(|chatCompletionsErrorResponse\s*\(|sameTargetRetries|rotateProviderTransportOn429' \
src/server/chat-completions.ts
echo '=== Endpoint tests for 429 replay failures ==='
rg -n -C 10 \
'same-target|sameTarget|rotated-key|rotate|429|ECONNRESET|socket|transport failure|fetchWithTransientRetry' \
tests/chat-completions-endpoint.test.tsLength of output: 1292 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Native 429 replay paths, numbered ==='
sed -n '300,430p' src/server/chat-completions.ts | awk '{ printf "%6d\t%s\n", NR + 299, $0 }'
echo '=== Retry and transport-failure helper use in the native handler ==='
rg -n -C 7 \
'fetchWithTransientRetry\s*\(|fetchWithHeaderTimeout\s*\(|transportFailureResponse\s*\(|chatCompletionsErrorResponse\s*\(|sameTargetRetries|rotateProviderTransportOn429' \
src/server/chat-completions.ts || true
echo '=== Endpoint tests for 429 replay failures ==='
rg -n -C 10 \
'same-target|sameTarget|rotated-key|rotate|429|ECONNRESET|socket|transport failure|fetchWithTransientRetry' \
tests/chat-completions-endpoint.test.ts || true
echo '=== Current-head changes in the affected files ==='
git diff --unified=30 HEAD^ HEAD -- src/server/chat-completions.ts tests/chat-completions-endpoint.test.tsLength of output: 27511
In If either replay has a transport failure, Use 🧠 Learnings usedYou are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/chat-completions-endpoint.test.ts`:
- Around line 751-758: Update the second request assertions in the chat
completions test to consume response2’s body and assert its expected content
before checking calls. Keep the status assertion, ensure the body is fully read
through the response API, then verify calls equals 2 to validate cleanup.
- Line 740: Update the upstream URL normalization in the mockConfig setup at
both occurrences to use a regex matching an actual trailing slash, ensuring the
slash is removed before appending /v1 and preventing a double slash.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1dc05603-0be4-407d-a6c0-765cc8494466
📒 Files selected for processing (1)
tests/chat-completions-endpoint.test.ts
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current PR head ==='
git rev-parse HEAD
git show --no-patch --format='%H%n%s' HEAD
echo '=== Native 429 replay paths ==='
nl -ba src/server/chat-completions.ts | sed -n '285,430p'
echo '=== Retry and transport-failure handling in native path ==='
rg -n -C 8 \
'fetchWithTransientRetry\s*\(|fetchWithHeaderTimeout\s*\(|transportFailureResponse\s*\(|chatCompletionsErrorResponse\s*\(|sameTargetRetries|rotateProviderTransportOn429' \
src/server/chat-completions.ts
echo '=== Focused endpoint tests for 429 replay transport failures ==='
rg -n -C 12 \
'same-target|sameTarget|rotated-key|rotate|429|ECONNRESET|socket|transport failure|fetchWithTransientRetry' \
tests/chat-completions-endpoint.test.ts || trueLength of output: 1311 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Native 429 replay paths ==='
sed -n '285,430p' src/server/chat-completions.ts | awk '{ printf "%6d\t%s\n", NR + 284, $0 }'
echo '=== Retry and transport-failure handling in native path ==='
rg -n -C 8 \
'fetchWithTransientRetry\s*\(|fetchWithHeaderTimeout\s*\(|transportFailureResponse\s*\(|chatCompletionsErrorResponse\s*\(|sameTargetRetries|rotateProviderTransportOn429' \
src/server/chat-completions.ts || true
echo '=== Focused endpoint tests for 429 replay transport failures ==='
rg -n -C 12 \
'same-target|sameTarget|rotated-key|rotate|429|ECONNRESET|socket|transport failure|fetchWithTransientRetry' \
tests/chat-completions-endpoint.test.ts || trueLength of output: 23461
However, the 429 replay paths do not use this handling:
If either replay has a transport failure, Use No endpoint test for a transport failure after either 429 replay exists in 🧠 Learnings usedYou are interacting with an AI system. |
|
I rechecked the new head |
…un#1467) Chat-form providers are the majority, so keep the change minimal: when a Chat Completions request routes to an openai-chat target, preserve the Chat wire end-to-end instead of round-tripping through the Responses pipeline. - Resolve the wire after routing (resolveWireProtocolOverride) and take the chat-native path only when adapter is openai-chat and the payload needs no Responses-only features (store/background/ previous_response_id/compaction/hosted tools). - Keep the existing translate-and-replay bridge for every other target so the shared admission/routing/key-pool/retry/usage/logging lifecycle stays on one path. - Wire policy, rate-limit retry, and usage accounting are preserved on the chat-native path; no raw byte relay. Refs lidge-jun#1467
…1467) - Restrict native eligibility to key/local auth (OAuth stays on bridge) - Stop leaking ChatGPT OAuth headers to third-party hosts - Move translator budget charge into native branch only - Type-safe route assignment, URL align with openai-chat adapter - Use sleepWithAbort + AbortSignal.any for retry delays - Bound key-pool rotations and track attempted key - Flush trailing SSE buffer, fix TTFT recording, reuse collectChatCompletion for non-stream fold to preserve tool_calls, and bound JSON reads via translatorBudget. Refs lidge-jun#1467
Remove dead acAbort assignment in native retry delay and cancel the bounded JSON reader on failure so upstream body does not leak. Refs lidge-jun#1467
…un#1467) The non-streaming chat-native path now charges upstream bytes incrementally and cancels the locked reader on budget overflow. Cover overflow returning 413 and that the server remains responsive for the next turn, and document the invalid-JSON passthrough contract. No further product-code blocker per Ingwannu review. Refs lidge-jun#1467
…jun#1467) Same-target and rotated-key 429 replays now mirror the initial fetch + Responses core: wrap with fetchWithTransientRetry and return transport failure (redacted 502) instead of stale 429. Refs lidge-jun#1467
e6dbd8d to
fbd9fa8
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Checked-out commit ==='
git rev-parse HEAD
git show --no-patch --format='%H%n%s%n%ci' HEAD
echo '=== Local refs related to dev ==='
git branch -a --contains HEAD | sed -n '1,80p'
git show-ref --heads --remotes | rg 'refs/(heads|remotes/[^/]+)/(dev|main)$' || true
echo '=== PR metadata and check status ==='
gh pr view 1569 --repo lidge-jun/opencodex \
--json number,headRefName,headRefOid,baseRefName,baseRefOid,mergeable,reviewDecision,statusCheckRollup \
--jq '{number,headRefName,headRefOid,baseRefName,baseRefOid,mergeable,reviewDecision,statusCheckRollup: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}]}'
echo '=== Compare PR head with origin/dev when available ==='
if git rev-parse --verify origin/dev >/dev/null 2>&1; then
git merge-base HEAD origin/dev
git rev-list --left-right --count HEAD...origin/dev
git log --oneline HEAD..origin/dev | sed -n '1,20p'
else
echo 'origin/dev is unavailable in this checkout'
fi
echo '=== Affected endpoint test outline ==='
ast-grep outline tests/chat-completions-endpoint.test.ts --items all --view expanded
echo '=== Relevant test fixtures and cleanup assertions ==='
rg -n -C 16 \
'second.*response|response.*second|cleanup|base.?URL|baseUrl|trailing slash|replace\(|mock.*URL|429|reader|cancel' \
tests/chat-completions-endpoint.test.tsLength of output: 49799 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="lidge-jun/opencodex"
head="fbd9fa8c0894e69076e72ff8e12f187603f039f6"
echo '=== GitHub comparison: current dev to PR head ==='
gh api "repos/${repo}/compare/dev...${head}" \
--jq '{status, ahead_by, behind_by, merge_base_commit: .merge_base_commit.sha, base_commit: .base_commit.sha, head_commit: .head_commit.sha}'
echo '=== Check suites and runs for the exact PR head ==='
gh api "repos/${repo}/commits/${head}/check-suites" --paginate \
--jq '.check_suites[] | {id, status, conclusion, app: .app.slug, head_sha, url}' || true
gh api "repos/${repo}/commits/${head}/check-runs" --paginate \
--jq '.check_runs[] | {name, status, conclusion, details_url}' || true
echo '=== Native 429 replay handling ==='
nl -ba src/server/chat-completions.ts | sed -n '290,435p'
echo '=== Current-head patch ==='
git diff --unified=35 HEAD^ HEAD -- \
src/server/chat-completions.ts \
tests/chat-completions-endpoint.test.tsLength of output: 4329
The gate state names However, exact-head CI is not fully green. Two GitHub Actions check suites for Two reported test defects also remain:
The 🧠 Learnings usedYou are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/server/chat-completions.ts`:
- Around line 505-517: Update the success response handling around shouldStream
and the passthrough branch to stream directly only when the client requests
streaming and the upstream content type is SSE. For wantsStream with a non-SSE
response, consume the JSON through the existing bounded-read path and synthesize
the same Chat SSE frame sequence used by the bridge logic near Lines 960-967,
including the proper data framing and [DONE] sentinel; preserve normal
passthrough for non-streaming requests.
- Around line 385-405: Remove the unknown double casts in the maxRotations
calculation and read the declared providerConfig.apiKeyPool directly, using its
length and preserving the appropriate zero-value behavior when absent. Remove
the unknown cast from rotateProviderTransportOn429 and align
activeProvider/nextProvider with the function’s OcxProviderTransport return
type, updating the surrounding provider typing if necessary so the rotation loop
remains type-checked.
- Around line 606-611: Update the canonical-chunk forwarding branch in the
stream normalizer to call noteSseFirstOutput() before enqueueing the chunk,
matching the other enqueue paths so first-output timing is recorded. Also apply
the same call to the [DONE] forwarding branches if terminal-only streams are
expected to report first output, while preserving their existing forwarding
behavior.
- Around line 449-475: Bound the non-OK response body read in the
chat-completions error path instead of calling response.text() unconditionally.
Update the logic around the response error handling and its JSON parsing so it
consumes only the needed limited bytes, preserves the existing 400-character
redacted fallback and parsed error fields, and follows the bounded-read approach
used by the success path.
- Around line 94-99: Update the native chat request flow in the function
containing fetchWithTransientRetry to validate key-auth providers before
sending: when authMode is "key", keyOptional is false, and the credential is
empty or missing, return the same proxy error produced by openai-chat rather
than omitting Authorization and fetching. Align isChatNativeEligibleProvider or
its surrounding validation with the adapter’s non-empty-credential guard while
preserving valid key, local, and unauthenticated paths.
- Around line 236-293: Update the chat-native request construction around
chatBodyForWire so streaming requests default stream_options.include_usage to
true when the client did not provide stream_options. Preserve any
client-supplied stream_options unchanged, and apply this default only for native
streaming turns.
Apply the same fix in `@src/server/chat-completions.ts` around lines 615 - 651:
The streamed usage chunk is forwarded but not recorded in request accounting.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: db7468cb-9b80-4d4c-ad11-54f274738282
📒 Files selected for processing (1)
src/server/chat-completions.ts
Wrap native 429 replays in transient retry, validate key credential, default stream_options.include_usage, synthesize streaming for non-SSE JSON, bound error body read, record usage and TTFT for streamed chunks, and clean up casts. Refs lidge-jun#1467
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="lidge-jun/opencodex"
reported_head="b10379ecf87b5d0febed933e8a0b89fa4919fa18"
echo '=== PR metadata ==='
gh pr view 1569 --repo "$repo" \
--json number,headRefOid,baseRefName,baseRefOid,mergeable,reviewDecision,statusCheckRollup \
--jq '{number,headRefOid,baseRefName,baseRefOid,mergeable,reviewDecision,checks:[.statusCheckRollup[] | {name,status,conclusion,detailsUrl}]}'
echo '=== Comparison with dev ==='
gh api "repos/${repo}/compare/dev...${reported_head}" \
--jq '{status,ahead_by,behind_by,merge_base_commit:.merge_base_commit.sha,head_commit:.head_commit.sha}'
echo '=== Exact-head check suites ==='
gh api "repos/${repo}/commits/${reported_head}/check-suites" --paginate \
--jq '.check_suites[] | {app:.app.slug,status,conclusion,head_sha}'
echo '=== Exact-head check runs ==='
gh api "repos/${repo}/commits/${reported_head}/check-runs" --paginate \
--jq '.check_runs[] | {name,status,conclusion,details_url}'
echo '=== Native 429 replay handling ==='
nl -ba src/server/chat-completions.ts | sed -n '285,435p'
echo '=== Replay transport-failure test coverage ==='
rg -n -C 10 \
'same-target|sameTarget|rotated-key|rotate|429|ECONNRESET|socket|transport failure|fetchWithTransientRetry|response2\.text|replace\(/\\/\$' \
tests/chat-completions-endpoint.test.ts || true
echo '=== Current changes in affected files ==='
git diff --unified=35 HEAD^ HEAD -- \
src/server/chat-completions.ts \
tests/chat-completions-endpoint.test.tsLength of output: 6762 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="lidge-jun/opencodex"
head="b10379ecf87b5d0febed933e8a0b89fa4919fa18"
echo '=== Exact-head workflow runs ==='
gh api "repos/${repo}/actions/runs?head_sha=${head}&per_page=100" \
--jq '.workflow_runs[] | {
id, name, event, status, conclusion, html_url,
workflow_id, run_attempt, created_at, updated_at
}'
echo '=== Action-required check-suite details ==='
gh api "repos/${repo}/commits/${head}/check-suites" --paginate \
--jq '.check_suites[]
| select(.conclusion == "action_required")
| {id,app:.app.slug,status,conclusion,head_sha,url,latest_check_runs_count}'
echo '=== Native 429 replay implementation ==='
awk 'NR >= 285 && NR <= 435 { printf "%6d\t%s\n", NR, $0 }' \
src/server/chat-completions.ts
echo '=== Endpoint regression-test implementation ==='
awk 'NR >= 700 && NR <= 810 { printf "%6d\t%s\n", NR, $0 }' \
tests/chat-completions-endpoint.test.ts
echo '=== Focused current-head test coverage ==='
rg -n -C 8 \
'fetchWithTransientRetry|fetchWithHeaderTimeout|chatCompletionsErrorResponse|sameTargetRetries|rotateProviderTransportOn429|ECONNRESET|socket|transport failure|response2\.text|replace\(/\\/\$' \
src/server/chat-completions.ts tests/chat-completions-endpoint.test.ts || trueLength of output: 73612
The PR is current with
The current [P1] Return the 502 response after a same-target replay transport failure The same-target 429 replay now uses When the replay fetch fails, Replace the return chatCompletionsErrorResponse(
502,
redactSecretString(msg).slice(0, 500),
"server_error",
);Add an endpoint regression test for a transport failure after a same-target 429 replay. The rotated-key replay now returns 502 correctly, but it also needs its requested regression test. The earlier response-consumption defect is fixed. The trailing-slash test defect remains:
Both lines use Obtain successful exact-head results for 🧠 Learnings usedYou are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="lidge-jun/opencodex"
echo '=== Current PR metadata ==='
gh pr view 1569 --repo "$repo" \
--json number,headRefOid,baseRefName,baseRefOid,mergeable,reviewDecision,statusCheckRollup \
--jq '{
number,
headRefOid,
baseRefName,
baseRefOid,
mergeable,
reviewDecision,
checks: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}]
}'
head="$(gh pr view 1569 --repo "$repo" --json headRefOid --jq .headRefOid)"
echo "=== Current head: ${head} ==="
echo '=== Comparison with dev ==='
gh api "repos/${repo}/compare/dev...${head}" \
--jq '{status, ahead_by, behind_by, merge_base_commit: .merge_base_commit.sha, head_commit: .head_commit.sha}'
echo '=== Exact-head check suites and runs ==='
gh api "repos/${repo}/commits/${head}/check-suites" --paginate \
--jq '.check_suites[] | {app: .app.slug, status, conclusion, head_sha}' || true
gh api "repos/${repo}/commits/${head}/check-runs" --paginate \
--jq '.check_runs[] | {name, status, conclusion, details_url}' || true
echo '=== Native 429 replay implementation ==='
nl -ba src/server/chat-completions.ts | sed -n '300,450p'
echo '=== Regression tests and mock URL normalization ==='
nl -ba tests/chat-completions-endpoint.test.ts | sed -n '710,825p'
rg -n -C 6 \
'replace\(/\\/\$|same-target|sameTarget|rotated-key|rotate|ECONNRESET|transport failure|response2\.(text|json)' \
tests/chat-completions-endpoint.test.ts || trueLength of output: 6782 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="lidge-jun/opencodex"
head="b10379ecf87b5d0febed933e8a0b89fa4919fa18"
echo '=== Exact-head workflow runs ==='
gh api "repos/${repo}/actions/runs?head_sha=${head}&per_page=100" \
--jq '.workflow_runs[] | {
id, name, event, status, conclusion, html_url, workflow_id, run_attempt
}'
echo '=== Action-required check suites ==='
gh api "repos/${repo}/commits/${head}/check-suites" --paginate \
--jq '.check_suites[]
| select(.conclusion == "action_required")
| {id, app: .app.slug, status, conclusion, url, latest_check_runs_count}' || true
echo '=== Native 429 replay implementation (Lines 300-450) ==='
awk 'NR >= 300 && NR <= 450 { printf "%6d %s\n", NR, $0 }' \
src/server/chat-completions.ts
echo '=== Regression tests and URL normalization (Lines 710-825) ==='
awk 'NR >= 710 && NR <= 825 { printf "%6d %s\n", NR, $0 }' \
tests/chat-completions-endpoint.test.ts
echo '=== Focused test coverage ==='
rg -n -C 6 \
'replace\(/\\/\$|same-target|sameTarget|rotated-key|rotate|ECONNRESET|transport failure|response2\.(text|json)' \
tests/chat-completions-endpoint.test.ts || trueLength of output: 27371
The PR is current with
The PR also has unresolved defects. [P1] Return the 502 response after a same-target 429 replay transport failure The same-target replay now uses At that point, Replace the return chatCompletionsErrorResponse(
502,
redactSecretString(msg).slice(0, 500),
"server_error",
);[P1] Correct the trailing-slash normalization in the new endpoint tests Both tests use: .replace(/\/\$/, "")This matches the literal text .replace(/\/$/, "")The second response body is now consumed at Lines 757-758. That earlier defect is fixed. No regression test covers a transport failure after either type of 429 replay. Add one endpoint test for the same-target replay and one endpoint test for the rotated-key replay. Each test should assert the redacted 502 response after the replay transport failure. Keep the 🧠 Learnings usedYou are interacting with an AI system. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head b10379ecf87b5d0febed933e8a0b89fa4919fa18. Some prior blockers were addressed, but this update is still not mergeable and introduced new failure semantics. Local focused validation remains green (39/39, typecheck, privacy scan), but there is no regression coverage for the new paths below.
- In
src/server/chat-completions.ts, a same-target replay fetch exception now records/finalizes 502 and thenbreaks instead of returning.responseremains the previous 429 whose body was already cancelled; the function can then enter key rotation or the non-OK response path, finalize again, and return/log 429 rather than the transport 502. Return the 502 immediately (as the previous head did), with a regression proving one final log and no rotation/error-body read after replay transport failure. - The non-OK “8 KiB” reader is still not byte-bounded. It decodes each entire provider-controlled chunk before checking
chunk.length, so one large first chunk allocates/decodes the whole body. Count and sliceUint8Arraybytes before decoding, await best-effort cancellation when the byte cap is reached or a read fails, and test a single oversized chunk plus read rejection. - The JSON-despite-stream synthesis treats invalid JSON or a JSON object without a valid Chat completion choice as a successful empty
stopcompletion followed by[DONE]. That hides upstream protocol failures as HTTP/SSE success. Parse and validate through a shared bounded Chat-completion normalizer; malformed/failed JSON must emit the established error contract, not fabricate a successful turn. Add text, tool-call, malformed, and failed-response regressions. - Stream usage accounting is still incomplete for the common canonical
object: "chat.completion.chunk"branch (including its EOF-tail branch): those chunks are forwarded without copyingusageintologCtx.usage/ attempt usage. Centralize usage extraction so every forwarded/synthesized chunk uses it, and assert the finalized request log.
The new head also moved the PR out of draft while exact-head cross-platform CI has not run. Please return it to draft until these protocol/logging/resource boundaries have direct tests and the full exact-head suite is green. The overall native Chat direction remains worth pursuing; the request is to narrow and prove this implementation before merge.
…dge-jun#1467) Same-target 429 replay now returns 502 immediately; non-OK error read is byte-bounded; JSON synthesis validates and rejects invalid chat completions via error contract; stream usage centralized and non-stream invalid JSON returns 502; body-charge attempt finalized. Add regressions for replay, synthesized streaming, and validation. Refs lidge-jun#1467
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="lidge-jun/opencodex"
echo '=== PR metadata ==='
gh pr view 1569 --repo "$repo" \
--json number,headRefOid,baseRefName,baseRefOid,mergeable,reviewDecision,statusCheckRollup \
--jq '{
number,
headRefOid,
baseRefName,
baseRefOid,
mergeable,
reviewDecision,
checks: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}]
}'
head="$(gh pr view 1569 --repo "$repo" --json headRefOid --jq .headRefOid)"
echo "=== Current head: ${head} ==="
echo '=== Comparison with dev ==='
gh api "repos/${repo}/compare/dev...${head}" \
--jq '{status, ahead_by, behind_by, merge_base_commit: .merge_base_commit.sha, head_commit: .head_commit.sha}'
echo '=== Exact-head check suites ==='
gh api "repos/${repo}/commits/${head}/check-suites" --paginate \
--jq '.check_suites[] | {app: .app.slug, status, conclusion, head_sha}' || true
echo '=== Exact-head check runs ==='
gh api "repos/${repo}/commits/${head}/check-runs" --paginate \
--jq '.check_runs[] | {name, status, conclusion, details_url}' || true
echo '=== Native 429 replay paths ==='
nl -ba src/server/chat-completions.ts | sed -n '300,455p'
echo '=== Endpoint replay regression tests and URL normalization ==='
rg -n -C 8 \
'same-target|sameTarget|rotated-key|rotate|429|ECONNRESET|socket|transport failure|replace\(/\\/\$|response2\.(text|json)' \
tests/chat-completions-endpoint.test.ts || trueLength of output: 5534 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="lidge-jun/opencodex"
head="fd7333540b8b9ccb936d36fd4638e425e28f2bfa"
echo '=== Native 429 replay paths ==='
awk 'NR >= 300 && NR <= 455 { printf "%6d %s\n", NR, $0 }' \
src/server/chat-completions.ts
echo '=== Endpoint replay tests and URL normalization ==='
awk 'NR >= 700 && NR <= 860 { printf "%6d %s\n", NR, $0 }' \
tests/chat-completions-endpoint.test.ts
echo '=== Focused source and test matches ==='
rg -n -C 8 \
'fetchWithTransientRetry|fetchWithHeaderTimeout|chatCompletionsErrorResponse|sameTargetRetries|rotateProviderTransportOn429|ECONNRESET|socket|transport failure|replace\(/\\/\$|response2\.(text|json)' \
src/server/chat-completions.ts tests/chat-completions-endpoint.test.ts || true
echo '=== Exact-head workflow runs ==='
gh api "repos/${repo}/actions/runs?head_sha=${head}&per_page=100" \
--jq '.workflow_runs[] | {
id,
name,
event,
status,
conclusion,
html_url,
run_attempt,
created_at,
updated_at
}'
echo '=== Action-required suite metadata ==='
gh api "repos/${repo}/commits/${head}/check-suites" --paginate \
--jq '.check_suites[]
| select(.conclusion == "action_required")
| {id, app: .app.slug, status, conclusion, url, latest_check_runs_count}' || trueLength of output: 90142
The gate state names Exact-head CI is not green:
The prior implementation defects are fixed:
[P2] Add regression tests for 429 replay transport failures The test file has no endpoint test for either required failure path:
Add one test for each path. Each test must make the upstream return 429 first, then fail the replay request. Assert that the client receives the redacted 502 Keep 🧠 Learnings usedYou are interacting with an AI system. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Exact-head re-review at fd7333540b8b9ccb936d36fd4638e425e28f2bfa: the four blockers from my prior review are addressed. Replay transport failures now return immediately with one 502 path; the non-OK reader bounds bytes before decoding and settles the reader; JSON-despite-stream validates Chat completion shape instead of fabricating success; and usage extraction covers canonical and synthesized chunks. The new focused regressions cover valid JSON/SSE synthesis, tool calls, malformed/missing-choice responses, overflow, and replay failure. Local exact-head validation is green: 60/60 endpoint tests, typecheck, privacy scan, and diff check.
I found no remaining blocker in the reviewed native Chat paths. Keep the PR draft until the full exact-head cross-platform suite is run and green; the current head only has lightweight PR gates. Once that matrix is green and an independent maintainer is comfortable with the new direct-wire surface, this remains a strong merge candidate for #1467.
2 participants
Summary
Chat-form providers are the majority, so this keeps the change minimal: when a
POST /v1/chat/completionsrequest routes to anopenai-chattarget, preserve the Chat wire end-to-end instead of round-tripping through the Responses pipeline (chat -> responses -> chat).chat -> chatis native in this PR. Every other target (Responses-native, Anthropic, Cursor/Kiro, hosted-tool/previous_response paths) stays on the existing translate-and-replay bridge.Closes #1467 (follow-up to #357).
Why this scope
The current
/v1/chat/completionshandler always converts the request into the internal Responses shape and then converts it back to Chat after theopenai-chatadapter renders it again. For a Chat client routed to a Chat provider that is a wasted round-trip and can alter protocol-faithful fields (tool-call history,reasoning_content,response_format, streaming frames). Keeping it minimal on the dominant wire avoids touching the rest of the pipelines.Behavior matrix
chatopenai-chatchatresponsesmessages(Claude)Chat-native is taken only when the resolved provider is
openai-chat(authMode != forward) and the payload needs no Responses-only feature (store/background/previous_response_id/compaction_trigger/hostedweb_search/image_generation). Otherwise it falls through to the bridge.Change
Single-file, minimal:
src/server/chat-completions.tsresolveWireProtocolOverride(..., "chat")) and branch on the result.POST /chat/completionsdirectly, send viafetchWithHeaderTimeout+fetchWithTransientRetry, with pre-stream 429 same-target wait and key-poolrotateProviderTransportOn429, bearer fromprovider.apiKey/provider.headers, andmodelSuffixBracketStriphandling.chat.completion.chunkenvelope (mock omits it) and relay viatrackStreamLifetimeso drain/cancel stay correct.chat.completionJSON for non-stream callers (matching the legacy bridge'scollectChatCompletionfolding).noStructuredOutputModelsper-model opt-out on the chat-native path.translation_buffer_limit(413) still holds without a Responses stringify.inboundProtocol:"chat",beginRequestAttempt("openai-chat"),usage.prompt_tokens/completion_tokensparsed intoOcxUsage.No raw byte relay; no duplicated lifecycle/drain/logging.
Verification
Notable coverage that passed: streaming
chat.completion.chunkenvelope, non-streamingchat.completionJSON, translator overflow413 translation_buffer_limit, and per-modelresponse_formatopt-out.Checklist
bun x tsc --noEmitgreenchat-completionssuite green (52)chat -> chatpass-throughReview readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Bug Fixes