fix(proxy): route builtin responses through chat hot path#207
fix(proxy): route builtin responses through chat hot path#207awsl233777 wants to merge 11 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 6 minutes and 48 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe proxy now converts ChangesResponses to chat conversion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/builtin-proxy.js (1)
143-151: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetry the proxy’s own timeout errors.
Line 223 and Line 1632 emit
error: 'timeout', but this classifier does not match that value, soretryTransientRequeststops after the first timed-out/chat/completionsattempt.Suggested fix
function isTransientNetworkError(error) { const text = String(error || '').trim(); if (!text) return false; + if (/timeout|timed out/i.test(text)) return true; if (/socket hang up/i.test(text)) return true;🤖 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 `@cli/builtin-proxy.js` around lines 143 - 151, The transient error classifier in isTransientNetworkError does not recognize the proxy’s own timeout marker, so retryTransientRequest treats timed-out /chat/completions failures as non-retryable. Update the timeout detection logic in isTransientNetworkError in builtin-proxy.js to return true when the error text is the proxy’s emitted timeout value (error: 'timeout'), alongside the existing socket/TLS/network checks, so timeout failures are retried consistently.
🤖 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 `@cli/builtin-proxy.js`:
- Around line 143-151: The transient error classifier in isTransientNetworkError
does not recognize the proxy’s own timeout marker, so retryTransientRequest
treats timed-out /chat/completions failures as non-retryable. Update the timeout
detection logic in isTransientNetworkError in builtin-proxy.js to return true
when the error text is the proxy’s emitted timeout value (error: 'timeout'),
alongside the existing socket/TLS/network checks, so timeout failures are
retried consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50a6be28-37d9-4f28-aeb1-ff1504bc3e36
📒 Files selected for processing (4)
cli/builtin-proxy.jstests/e2e/run.jstests/e2e/test-builtin-proxy-codex-conversion.jstests/unit/builtin-proxy-responses-shim.test.mjs
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.0)
tests/e2e/run.js
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: require('child_process')
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: require('child_process')
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
tests/e2e/test-builtin-proxy-codex-conversion.js
[warning] 37-130: Use https protocol over http
Context: http.createServer((req, res) => {
const requestPath = String(req.url || '').split('?')[0];
let body = '';
req.setEncoding('utf-8');
req.on('data', chunk => body += chunk);
req.on('end', () => {
let parsedBody = null;
if (body.trim()) {
try { parsedBody = JSON.parse(body); } catch (_) { parsedBody = null; }
}
requests.push({
method: req.method,
path: requestPath,
headers: req.headers,
body: parsedBody
});
if (req.method === 'GET' && requestPath === '/nested/api/models') {
const payload = JSON.stringify({ data: [{ id: 'gpt-sub2api-e2e' }] });
res.writeHead(200, {
'Content-Type': 'application/json; charset=utf-8',
'Content-Length': Buffer.byteLength(payload, 'utf-8')
});
res.end(payload, 'utf-8');
return;
}
if (req.method === 'POST' && requestPath === '/nested/api/responses') {
const payload = JSON.stringify({ error: { message: 'responses endpoint must not be used' } });
res.writeHead(500, {
'Content-Type': 'application/json; charset=utf-8',
'Content-Length': Buffer.byteLength(payload, 'utf-8')
});
res.end(payload, 'utf-8');
return;
}
if (req.method === 'POST' && requestPath === '/nested/api/chat/completions') {
if (parsedBody && parsedBody.model === 'error-model') {
const payload = JSON.stringify({ error: { message: 'upstream rate limited' } });
res.writeHead(429, {
'Content-Type': 'application/json; charset=utf-8',
'Content-Length': Buffer.byteLength(payload, 'utf-8')
});
res.end(payload, 'utf-8');
return;
}
if (parsedBody && parsedBody.stream === true) {
res.writeHead(200, { 'Content-Type': 'text/event-stream; charset=utf-8' });
res.write('data: {"id":"chatcmpl_sub2api_stream","model":"gpt-sub2api-e2e","choices":[{"delta":{"role":"assistant"}}]}\n\n');
res.write('data: {"id":"chatcmpl_sub2api_stream","model":"gpt-sub2api-e2e","choices":[{"delta":{"content":"stream-"}}]}\n\n');
res.write('data: {"id":"chatcmpl_sub2api_stream","model":"gpt-sub2api-e2e","choices":[{"delta":{"content":"ok"}}]}\n\n');
res.end('data: [DONE]\n\n');
return;
}
const hasTools = parsedBody && Array.isArray(parsedBody.tools) && parsedBody.tools.length > 0;
const payload = JSON.stringify({
id: hasTools ? 'chatcmpl_sub2api_tool' : 'chatcmpl_sub2api_text',
model: parsedBody && parsedBody.model ? parsedBody.model : 'unknown-model',
choices: [{
finish_reason: hasTools ? 'tool_calls' : 'stop',
message: hasTools ? {
role: 'assistant',
content: null,
tool_calls: [{
id: 'call_lookup_e2e',
type: 'function',
function: { name: 'lookup', arguments: '{"q":"codexmate"}' }
}]
} : {
role: 'assistant',
content: 'direct-chat-ok'
}
}],
usage: { prompt_tokens: 5, completion_tokens: 7, total_tokens: 12 }
});
res.writeHead(200, {
'Content-Type': 'application/json; charset=utf-8',
'Content-Length': Buffer.byteLength(payload, 'utf-8')
});
res.end(payload, 'utf-8');
return;
}
const notFound = JSON.stringify({ error: { message: 'not found' } });
res.writeHead(404, {
'Content-Type': 'application/json; charset=utf-8',
'Content-Length': Buffer.byteLength(notFound, 'utf-8')
});
res.end(notFound, 'utf-8');
});
})
Note: [CWE-319] Cleartext Transmission of Sensitive Information. Security best practice.
(https-protocol-missing)
🔇 Additional comments (4)
tests/unit/builtin-proxy-responses-shim.test.mjs (1)
131-148: LGTM!Also applies to: 168-169, 496-507, 542-551, 1390-1410, 1436-1440
tests/e2e/run.js (1)
19-19: LGTM!Also applies to: 155-155
tests/e2e/test-builtin-proxy-codex-conversion.js (1)
178-179: 🎯 Functional CorrectnessNo change needed for the non-stream lookup The non-stream body already sets
stream: false, soreq.body.stream === falseis safe.cli/builtin-proxy.js (1)
957-959: LGTM!Also applies to: 1642-1644, 2008-2012, 2037-2059, 2061-2070
|
Updated after CodeRabbit feedback:
Validation after the update:
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
|
Updated the builtin Responses streaming path for the stability issue reported in chat:
Validation after this update:
@coderabbitai re-review |
|
✅ Action performedReview finished.
|
|
Updated the streaming bridge to align with sub2api's Chat Completions → Responses lifecycle instead of synthesizing a custom stream shape. What changed:
Validation after this update:
@coderabbitai re-review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/test-builtin-proxy-codex-conversion.js (1)
212-212: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid picking the proxy port from a fixed random range.
20000 + Math.floor(Math.random() * 1000)can collide with another process in CI, so this test can fail before it exercises the proxy at all. Prefer an OS-assigned free port or reserve one explicitly first.🤖 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 `@tests/e2e/test-builtin-proxy-codex-conversion.js` at line 212, The proxy port selection in the e2e test is using a fixed random range, which can still collide in CI. Update the test setup around the proxyPort assignment to obtain an OS-assigned free port or reserve a port explicitly before starting the proxy, and keep the change localized to the proxy test flow so the rest of the test continues to use the chosen port.
🧹 Nitpick comments (2)
cli/builtin-proxy.js (1)
2244-2254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnreachable dead code: this
if (wantsStream)block can never execute.
wantsStreamisconst(Line 2185) and the streaming case already returns inside the earlierif (wantsStream) { ... return; }block (Lines 2193-2215). By the time execution reaches Line 2244,wantsStreamis alwaysfalse, so this branch (and itssendResponsesSse/event-stream write) is dead. Drop it and keep only the JSON response.♻️ Proposed cleanup
const responsesPayload = buildResponsesPayloadFromChatCompletion(chatJson.value, model, { toolTypesByName }); - - if (wantsStream) { - res.writeHead(200, { - 'Content-Type': 'text/event-stream; charset=utf-8', - 'Cache-Control': 'no-cache', - 'Connection': 'keep-alive', - 'X-Accel-Buffering': 'no' - }); - sendResponsesSse(res, responsesPayload); - res.end(); - return; - } res.writeHead(200, { 'Content-Type': 'application/json; charset=utf-8' }); res.end(JSON.stringify(responsesPayload));🤖 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 `@cli/builtin-proxy.js` around lines 2244 - 2254, Remove the unreachable streaming branch in builtin-proxy.js: the later if (wantsStream) block around the response handling is dead because wantsStream is a const and the earlier streaming path already returns after sendResponsesSse. Update the response flow so only the JSON response path remains there, and keep the logic centered around the existing request handling code that computes wantsStream and sendResponsesSse.tests/e2e/test-builtin-proxy-codex-conversion.js (1)
276-285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe slow-stream regression stops before it verifies delayed output.
requestUntilText(..., 'event: response.created', ...)resolves on the first marker andrequestUntilTextdestroys the request immediately afterward, so a proxy that emitsresponse.createdand then stalls forever would still pass here. Consider following this with a full-body read (or a helper that waits for both the early marker and[DONE]) so the delayed-content path is actually covered.🤖 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 `@tests/e2e/test-builtin-proxy-codex-conversion.js` around lines 276 - 285, The slow-stream test in requestUntilText only proves the early response.created marker arrives and then cancels the request, so it never validates delayed output. Update the test around earlyStreamResponse to keep reading after the first marker or use a helper that waits for both the early SSE event and the eventual [DONE] terminator, so the slow-content path is actually exercised and verified.
🤖 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/e2e/test-builtin-proxy-codex-conversion.js`:
- Line 49: The timed-out probe path in the E2E helper leaves the probe request
open, which can leak the SSE socket and flake later steps. Update the timeout
branch in the probe setup (the setTimeout that calls finish in
test-builtin-proxy-codex-conversion.js) to explicitly cancel/close the in-flight
req before resolving the timeout result, and make sure the cleanup happens for
the timeout path as well as normal completion.
In `@tests/unit/builtin-proxy-responses-shim.test.mjs`:
- Around line 1697-1699: The SSE assertions in the builtin proxy responses shim
test are too specific about how function-call arguments are chunked, so update
the test around the response.function_call_arguments.delta checks to verify the
full reconstructed arguments content instead of a particular delta split
pattern. Keep the event-name assertion in the relevant test case, but replace
the two delta-partition regex matches with assertions that tolerate any valid
re-chunking while still confirming the final JSON arguments for the function
call are correct.
- Around line 1222-1251: Loosen the timeout gate in the slow-stream test so it
matches the intended contract instead of failing too early. In the test around
the earlySse/proxyPort request flow, increase the setTimeout used to finish the
probe and keep the assertions focused on “response.created arrives before
delayed content” by checking the first upstream chat chunk behavior in
builtin-proxy-responses-shim.test.mjs. Update the elapsed-time expectation to
allow normal CI jitter while still ensuring the SSE is emitted before the
delayed content from the proxy path.
---
Outside diff comments:
In `@tests/e2e/test-builtin-proxy-codex-conversion.js`:
- Line 212: The proxy port selection in the e2e test is using a fixed random
range, which can still collide in CI. Update the test setup around the proxyPort
assignment to obtain an OS-assigned free port or reserve a port explicitly
before starting the proxy, and keep the change localized to the proxy test flow
so the rest of the test continues to use the chosen port.
---
Nitpick comments:
In `@cli/builtin-proxy.js`:
- Around line 2244-2254: Remove the unreachable streaming branch in
builtin-proxy.js: the later if (wantsStream) block around the response handling
is dead because wantsStream is a const and the earlier streaming path already
returns after sendResponsesSse. Update the response flow so only the JSON
response path remains there, and keep the logic centered around the existing
request handling code that computes wantsStream and sendResponsesSse.
In `@tests/e2e/test-builtin-proxy-codex-conversion.js`:
- Around line 276-285: The slow-stream test in requestUntilText only proves the
early response.created marker arrives and then cancels the request, so it never
validates delayed output. Update the test around earlyStreamResponse to keep
reading after the first marker or use a helper that waits for both the early SSE
event and the eventual [DONE] terminator, so the slow-content path is actually
exercised and verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac286d6d-4360-408b-b256-a34f15b5c8d0
📒 Files selected for processing (3)
cli/builtin-proxy.jstests/e2e/test-builtin-proxy-codex-conversion.jstests/unit/builtin-proxy-responses-shim.test.mjs
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.0)
tests/e2e/test-builtin-proxy-codex-conversion.js
[warning] 48-48: Avoid using the initial state variable in setState
Context: setTimeout(() => finish({ statusCode: 0, body: text, firstDataElapsedMs, timedOut: true }), timeoutMs)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🔇 Additional comments (3)
cli/builtin-proxy.js (3)
143-153: LGTM!
1342-1419: LGTM!
1487-1533: LGTM!
|
Stability follow-up pushed in What changed:
Validation actually run:
@coderabbitai re-review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/builtin-proxy-responses-shim.test.mjs (1)
1311-1315: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert this idle-stream path never probes
/v1/responses.The handler returns 404 but does not record/assert usage, so this test could still pass if the proxy probes
/v1/responsesand then falls back to chat.Proposed test tightening
+ let responsesHit = false; const upstream = http.createServer((req, res) => { if (req.url === '/v1/responses' && req.method === 'POST') { + responsesHit = true; res.writeHead(404, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ error: 'responses endpoint unavailable' })); return; @@ assert.match(early.text, /: keepalive/); assert.doesNotMatch(early.text, /event: response\.created/); assert.doesNotMatch(early.text, /event: response\.in_progress/); + assert.equal(responsesHit, false, 'proxy should not probe upstream /v1/responses before chat streaming');Also applies to: 1376-1381
🤖 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 `@tests/unit/builtin-proxy-responses-shim.test.mjs` around lines 1311 - 1315, The idle-stream fallback test is not verifying that the proxy avoids probing /v1/responses, so it can pass even if the proxy still makes that request before falling back. Update the upstream test server in builtin-proxy-responses-shim.test.mjs to explicitly record or assert any /v1/responses POST attempts in the relevant idle-stream cases, and then fail the test if that path is hit; use the existing upstream request handler around the idle-stream scenarios so the assertion is tied to the proxy behavior being tested.
🤖 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/unit/builtin-proxy-responses-shim.test.mjs`:
- Around line 1350-1351: The early probe timeout path in the keepalive test
leaves the HTTP request open, so update the timeout handling around the
timer/http.request probe to explicitly destroy or abort the request before
calling finish when the 500ms timeout fires. Make the fix in the probe setup
logic so the request lifecycle is cleaned up on timeout, and ensure the existing
finish callback still reports the timedOut result only after the request has
been torn down.
---
Nitpick comments:
In `@tests/unit/builtin-proxy-responses-shim.test.mjs`:
- Around line 1311-1315: The idle-stream fallback test is not verifying that the
proxy avoids probing /v1/responses, so it can pass even if the proxy still makes
that request before falling back. Update the upstream test server in
builtin-proxy-responses-shim.test.mjs to explicitly record or assert any
/v1/responses POST attempts in the relevant idle-stream cases, and then fail the
test if that path is hit; use the existing upstream request handler around the
idle-stream scenarios so the assertion is tied to the proxy behavior being
tested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4acf102-9f71-40ae-a612-a442249b8c62
📒 Files selected for processing (3)
cli/builtin-proxy.jstests/e2e/test-builtin-proxy-codex-conversion.jstests/unit/builtin-proxy-responses-shim.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/builtin-proxy.js
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.0)
tests/e2e/test-builtin-proxy-codex-conversion.js
[warning] 83-83: Avoid using the initial state variable in setState
Context: setTimeout(resolve, ms)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🔇 Additional comments (2)
tests/e2e/test-builtin-proxy-codex-conversion.js (1)
83-117: LGTM!Also applies to: 170-191, 245-245, 333-390
tests/unit/builtin-proxy-responses-shim.test.mjs (1)
1310-1310: LGTM!Also applies to: 1316-1349, 1352-1388
|
Addressed the latest CodeRabbit test-stability feedback in What changed:
Validation actually run:
@coderabbitai re-review |
|
(•ᴗ•)🐇 ✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/test-builtin-proxy-codex-conversion.js (2)
372-407: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAssert the CPU guard instead of only logging it.
This computes
process.cpuUsage()but never fails on a busy-loop regression, so the claimed CPU stability coverage is observational only. Add a generous ceiling or remove the metric from the test contract.🤖 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 `@tests/e2e/test-builtin-proxy-codex-conversion.js` around lines 372 - 407, The stability test in the idle stream flow currently records CPU usage but only logs it, so it won’t fail on a busy-loop regression. In the test block around the existing process.cpuUsage() measurement and the final console.log, add an assertion on the CPU delta (using the stabilityCpu values from the idle stream run) with a generous ceiling, or otherwise drop CPU usage from the test contract entirely. Keep the check aligned with the existing idle-stream assertions in this test so the CPU guard is enforced by the same stability scenario.
86-99: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid the port handoff race
getAvailableTcpPort()closes the socket beforeproxy-startbinds it, leaving a TOCTOU window in both uses here and at 286-294.proxy-startalso rejectsport: 0, so the fix needs to move to the startup path if you want to remove the flake.🤖 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 `@tests/e2e/test-builtin-proxy-codex-conversion.js` around lines 86 - 99, The race is in getAvailableTcpPort and the proxy-start setup: the helper closes the temporary server before proxy-start can bind, creating a TOCTOU window, and proxy-start currently rejects port 0. Update the startup path around proxy-start so it acquires and binds a real available port directly instead of handing off a released socket, and adjust the getAvailableTcpPort usage so the selected port stays reserved until the server starts. Use the getAvailableTcpPort helper and proxy-start port handling as the main symbols to locate the changes.
🤖 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.
Nitpick comments:
In `@tests/e2e/test-builtin-proxy-codex-conversion.js`:
- Around line 372-407: The stability test in the idle stream flow currently
records CPU usage but only logs it, so it won’t fail on a busy-loop regression.
In the test block around the existing process.cpuUsage() measurement and the
final console.log, add an assertion on the CPU delta (using the stabilityCpu
values from the idle stream run) with a generous ceiling, or otherwise drop CPU
usage from the test contract entirely. Keep the check aligned with the existing
idle-stream assertions in this test so the CPU guard is enforced by the same
stability scenario.
- Around line 86-99: The race is in getAvailableTcpPort and the proxy-start
setup: the helper closes the temporary server before proxy-start can bind,
creating a TOCTOU window, and proxy-start currently rejects port 0. Update the
startup path around proxy-start so it acquires and binds a real available port
directly instead of handing off a released socket, and adjust the
getAvailableTcpPort usage so the selected port stays reserved until the server
starts. Use the getAvailableTcpPort helper and proxy-start port handling as the
main symbols to locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad0a9d66-9dac-4e57-b135-5900a23935fe
📒 Files selected for processing (2)
tests/e2e/test-builtin-proxy-codex-conversion.jstests/unit/builtin-proxy-responses-shim.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/builtin-proxy-responses-shim.test.mjs
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.0)
tests/e2e/test-builtin-proxy-codex-conversion.js
[warning] 87-87: Use https protocol over http
Context: http.createServer()
Note: [CWE-319] Cleartext Transmission of Sensitive Information. Security best practice.
(https-protocol-missing)
🔇 Additional comments (1)
tests/e2e/test-builtin-proxy-codex-conversion.js (1)
49-52: LGTM!
|
事故复现链路这轮又补了一刀，不按 review nitpick 走：
已验证： node --check cli/builtin-proxy.js
node --check tests/unit/builtin-proxy-responses-shim.test.mjs
node --test tests/unit/builtin-proxy-responses-shim.test.mjs # 25/25, TARGET_UNIT_EXIT=0
npm run test:e2e # E2E_EXIT=0
npm run test:unit # 716 tests passed, UNIT_EXIT=0
npm run lint # LINT_EXIT=0
git diff --check # DIFF_CHECK_EXIT=0说明：这次修的是用户现场看到的 “DSML/tool_calls 内脏直接出现在正文 + Working 卡住” 这一类真实事故路径，不是只为了让 CI 变绿。 |
|
Update: fixed the real Changes in this round:
Validation actually run:
Real provider re-test boundary:
|
|
Follow-up after real provider UX review: added bounded bridge-side retries for early streaming failures. Why:
Changes in this round:
Regression coverage added/updated:
Validation actually run:
Boundary:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cli/builtin-proxy.js (3)
1123-1133: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
response.createdin-progress and empty.This emits the completed
statusand fulloutputinresponse.created, before the laterresponse.output_item.addedevents. That breaks the streamed lifecycle ordering this shim is trying to preserve; mirrorbeginChatStreamResponsesSseand only expose final output inresponse.completed.Proposed fix
if (!options.skipCreated) { writeSse(res, 'response.created', { type: 'response.created', response: { id: responseId, model, created_at: response.created_at, - status: response.status || 'in_progress', - output: Array.isArray(response.output) ? response.output : [] + status: 'in_progress', + output: [] } }); }🤖 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 `@cli/builtin-proxy.js` around lines 1123 - 1133, The `response.created` event in the proxy stream is leaking completed state too early; adjust the `writeSse` payload in `cli/builtin-proxy.js` so `response.created` stays in-progress and empty, matching `beginChatStreamResponsesSse`. Keep `status` fixed to the in-progress value and omit the full `output` here, then reserve the final status and output population for `response.completed` only.
1684-1691: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAttach the client-close abort handler before waiting for upstream headers.
The close listener is only installed after upstream SSE is accepted. If the client disconnects during DNS/TLS/header wait, the upstream request remains alive until timeout.
Proposed fix
}); sharedState.upstreamReq = req; + if (res && typeof res.on === 'function' && !sharedState.closeListenerAttached) { + sharedState.closeListenerAttached = true; + res.on('close', () => { + sharedState.clientClosed = true; + stopChatStreamHeartbeat(sharedState); + if (!sharedState.finished && sharedState.upstreamReq) { + try { sharedState.upstreamReq.destroy(new Error('client aborted')); } catch (_) {} + } + }); + } req.setTimeout(timeoutMs, () => { if (streamAccepted) return; try { req.destroy(new Error('timeout')); } catch (_) {} finish({ ok: false, error: 'timeout' }); }); - req.on('error', (err) => finish({ ok: false, error: err && err.message ? err.message : 'request failed' })); + req.on('error', (err) => { + if (sharedState.clientClosed || res.writableEnded || res.destroyed) { + finish({ ok: true }); + return; + } + finish({ ok: false, error: err && err.message ? err.message : 'request failed' }); + });Also applies to: 1922-1929
🤖 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 `@cli/builtin-proxy.js` around lines 1684 - 1691, The client-close abort handler is attached too late in the streaming flow, so disconnects during DNS/TLS/header waiting can leave the upstream request running. Move the res.close listener setup in the relevant streaming path(s) in cli/builtin-proxy.js so it is installed before awaiting upstream headers, and ensure the existing state.clientClosed, stopChatStreamHeartbeat, and state.upstreamReq.destroy('client aborted') logic still runs when the client disconnects.
2335-2343: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetry retryable upstream HTTP failures before surfacing the stream error.
The streaming route now relies on this chat-completions path, but
status >= 400results are returned withstatus, andretryTransientRequeststops on any positive status. That means 429/5xx and quota-style 403 failures before output are not retried.Proposed fix
+ function isRetryableUpstreamStatus(status, bodyText = '') { + if (status === 408 || status === 409 || status === 425 || status === 429) return true; + if (status >= 500 && status <= 599) return true; + if (status === 403 && /quota|balance|credit|capacity|rate/i.test(String(bodyText || ''))) return true; + return false; + } + async function retryTransientRequest(executor) { let lastResult = null; for (let attempt = 0; attempt <= TRANSIENT_RETRY_DELAYS_MS.length; attempt += 1) { @@ if (!result) return result; if (result.ok) return result; if (result.retry) return result; - if (result.status && result.status > 0) return result; - if (!result.retryTransient && !isTransientNetworkError(result.error)) return result; + if (result.retryTransient || isTransientNetworkError(result.error)) continue; + if (result.status && result.status > 0) return result; } return lastResult; }if (status >= 400) { upstreamRes.on('data', (chunk) => chunk && chunks.push(chunk)); - upstreamRes.on('end', () => finish({ ok: false, status, bodyText: chunks.length ? Buffer.concat(chunks).toString('utf-8') : '' })); + upstreamRes.on('end', () => { + const bodyText = chunks.length ? Buffer.concat(chunks).toString('utf-8') : ''; + finish({ + ok: false, + status, + bodyText, + ...(isRetryableUpstreamStatus(status, bodyText) ? { retryTransient: true } : {}) + }); + }); return; }🤖 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 `@cli/builtin-proxy.js` around lines 2335 - 2343, The streaming chat-completions path is surfacing retryable upstream errors too early because `streamChatCompletionsAsResponsesSseWithFallbackUrls` returns positive `status` values that cause `retryTransientRequest` to stop immediately. Update the retry handling around the `streamChatCompletionsAsResponsesSseWithFallbackUrls` call in `builtin-proxy` so 429, 5xx, and quota-style 403 responses before any streamed output are treated as transient and retried before failing the request. Keep the fix localized to the streaming/chat-completions flow and ensure the retry decision uses the upstream response status and body/error details appropriately.
🤖 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 `@cli/openai-bridge.js`:
- Around line 1539-1574: The DSML buffering in
flushChatStreamContentBuffer/writeChatStreamContentSegment can grow without
bound when DSML_TOOL_CALLS_END never arrives, so add a maxBytes-based guard for
pending content. In flushChatStreamContentBuffer, track the buffered DSML block
size and trim or reject oversized incomplete blocks instead of keeping the full
buffer in state.contentBuffer; ensure the guard applies both when start/end
markers are missing and when buffering through writeChatStreamContentSegment.
Use the existing state fields and DSML marker handling to locate the change.
In `@tests/unit/openai-bridge-upstream-responses.test.mjs`:
- Around line 168-170: Wrap the shutdown logic in the
openai-bridge-upstream-responses test so it waits for server close completion
instead of just calling http.Server.close() directly. Update the cleanup around
bridge.close() and upstream.close() to use promise-based shutdown handling (for
example via a helper that resolves on the close callback) before removing
tmpDir. This should be done in the test cleanup path where bridge and upstream
are torn down.
---
Outside diff comments:
In `@cli/builtin-proxy.js`:
- Around line 1123-1133: The `response.created` event in the proxy stream is
leaking completed state too early; adjust the `writeSse` payload in
`cli/builtin-proxy.js` so `response.created` stays in-progress and empty,
matching `beginChatStreamResponsesSse`. Keep `status` fixed to the in-progress
value and omit the full `output` here, then reserve the final status and output
population for `response.completed` only.
- Around line 1684-1691: The client-close abort handler is attached too late in
the streaming flow, so disconnects during DNS/TLS/header waiting can leave the
upstream request running. Move the res.close listener setup in the relevant
streaming path(s) in cli/builtin-proxy.js so it is installed before awaiting
upstream headers, and ensure the existing state.clientClosed,
stopChatStreamHeartbeat, and state.upstreamReq.destroy('client aborted') logic
still runs when the client disconnects.
- Around line 2335-2343: The streaming chat-completions path is surfacing
retryable upstream errors too early because
`streamChatCompletionsAsResponsesSseWithFallbackUrls` returns positive `status`
values that cause `retryTransientRequest` to stop immediately. Update the retry
handling around the `streamChatCompletionsAsResponsesSseWithFallbackUrls` call
in `builtin-proxy` so 429, 5xx, and quota-style 403 responses before any
streamed output are treated as transient and retried before failing the request.
Keep the fix localized to the streaming/chat-completions flow and ensure the
retry decision uses the upstream response status and body/error details
appropriately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fac4490-dbfd-4522-a47c-1301015e22ed
📒 Files selected for processing (4)
cli/builtin-proxy.jscli/openai-bridge.jstests/unit/builtin-proxy-responses-shim.test.mjstests/unit/openai-bridge-upstream-responses.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/builtin-proxy-responses-shim.test.mjs
📜 Review details
🧰 Additional context used
🪛 OpenGrep (1.23.0)
cli/openai-bridge.js
[ERROR] 1451-1451: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 1476-1476: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 1484-1484: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
cli/builtin-proxy.js
[ERROR] 1259-1259: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 1284-1284: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 1292-1292: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (5)
tests/unit/openai-bridge-upstream-responses.test.mjs (1)
156-166: LGTM!Also applies to: 173-227, 234-290, 297-351, 610-680
cli/openai-bridge.js (3)
1134-1151: LGTM!Also applies to: 1251-1304
1413-1537: LGTM!
1618-1623: LGTM!Also applies to: 1641-1641, 1865-1865, 2402-2402, 2413-2413, 2440-2442
cli/builtin-proxy.js (1)
143-153: LGTM!Also applies to: 1057-1078, 1221-1239, 1242-1301, 1403-1497, 1519-1538, 1550-1563, 1568-1650, 1668-1717, 1805-1819, 1855-1872, 1880-1902, 2304-2308
| function flushChatStreamContentBuffer(state, force = false) { | ||
| if (!state || typeof state.contentBuffer !== 'string' || !state.contentBuffer) return; | ||
| let buffer = state.contentBuffer; | ||
| state.contentBuffer = ''; | ||
| while (buffer) { | ||
| const start = buffer.indexOf(DSML_TOOL_CALLS_START); | ||
| if (start < 0) { | ||
| const keep = force ? 0 : longestDsmlStartPrefixSuffix(buffer); | ||
| const text = keep > 0 ? buffer.slice(0, buffer.length - keep) : buffer; | ||
| emitChatStreamTextDelta(state, text); | ||
| state.contentBuffer = keep > 0 ? buffer.slice(buffer.length - keep) : ''; | ||
| return; | ||
| } | ||
| if (start > 0) { | ||
| emitChatStreamTextDelta(state, buffer.slice(0, start)); | ||
| buffer = buffer.slice(start); | ||
| } | ||
| const end = buffer.indexOf(DSML_TOOL_CALLS_END, DSML_TOOL_CALLS_START.length); | ||
| if (end < 0) { | ||
| if (force) { | ||
| state.contentBuffer = ''; | ||
| } else { | ||
| state.contentBuffer = buffer; | ||
| } | ||
| return; | ||
| } | ||
| const blockText = buffer.slice(DSML_TOOL_CALLS_START.length, end); | ||
| appendDsmlToolCallsToChatStreamState(state, blockText); | ||
| buffer = buffer.slice(end + DSML_TOOL_CALLS_END.length); | ||
| } | ||
| } | ||
|
|
||
| function writeChatStreamContentSegment(state, segment) { | ||
| if (!state || typeof segment !== 'string' || !segment) return; | ||
| state.contentBuffer = (typeof state.contentBuffer === 'string' ? state.contentBuffer : '') + segment; | ||
| flushChatStreamContentBuffer(state, false); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound incomplete DSML buffers.
Line 1561 retains the entire pending DSML block until the end marker arrives. Since successful streaming chunks are not capped by maxBytes, a malformed upstream stream can grow state.contentBuffer indefinitely and stall output.
Proposed guard
const DSML_TOOL_CALLS_START = '<｜DSML｜tool_calls>';
const DSML_TOOL_CALLS_END = '</｜DSML｜tool_calls>';
+const DSML_CONTENT_BUFFER_MAX_BYTES = 1024 * 1024;
@@
if (end < 0) {
if (force) {
state.contentBuffer = '';
} else {
+ if (Buffer.byteLength(buffer, 'utf8') > DSML_CONTENT_BUFFER_MAX_BYTES) {
+ state.contentBuffer = '';
+ failChatStreamResponsesSse(state, 'DSML tool-call block exceeded buffer limit');
+ return;
+ }
state.contentBuffer = buffer;
}
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function flushChatStreamContentBuffer(state, force = false) { | |
| if (!state || typeof state.contentBuffer !== 'string' || !state.contentBuffer) return; | |
| let buffer = state.contentBuffer; | |
| state.contentBuffer = ''; | |
| while (buffer) { | |
| const start = buffer.indexOf(DSML_TOOL_CALLS_START); | |
| if (start < 0) { | |
| const keep = force ? 0 : longestDsmlStartPrefixSuffix(buffer); | |
| const text = keep > 0 ? buffer.slice(0, buffer.length - keep) : buffer; | |
| emitChatStreamTextDelta(state, text); | |
| state.contentBuffer = keep > 0 ? buffer.slice(buffer.length - keep) : ''; | |
| return; | |
| } | |
| if (start > 0) { | |
| emitChatStreamTextDelta(state, buffer.slice(0, start)); | |
| buffer = buffer.slice(start); | |
| } | |
| const end = buffer.indexOf(DSML_TOOL_CALLS_END, DSML_TOOL_CALLS_START.length); | |
| if (end < 0) { | |
| if (force) { | |
| state.contentBuffer = ''; | |
| } else { | |
| state.contentBuffer = buffer; | |
| } | |
| return; | |
| } | |
| const blockText = buffer.slice(DSML_TOOL_CALLS_START.length, end); | |
| appendDsmlToolCallsToChatStreamState(state, blockText); | |
| buffer = buffer.slice(end + DSML_TOOL_CALLS_END.length); | |
| } | |
| } | |
| function writeChatStreamContentSegment(state, segment) { | |
| if (!state || typeof segment !== 'string' || !segment) return; | |
| state.contentBuffer = (typeof state.contentBuffer === 'string' ? state.contentBuffer : '') + segment; | |
| flushChatStreamContentBuffer(state, false); | |
| const DSML_CONTENT_BUFFER_MAX_BYTES = 1024 * 1024; | |
| function flushChatStreamContentBuffer(state, force = false) { | |
| if (!state || typeof state.contentBuffer !== 'string' || !state.contentBuffer) return; | |
| let buffer = state.contentBuffer; | |
| state.contentBuffer = ''; | |
| while (buffer) { | |
| const start = buffer.indexOf(DSML_TOOL_CALLS_START); | |
| if (start < 0) { | |
| const keep = force ? 0 : longestDsmlStartPrefixSuffix(buffer); | |
| const text = keep > 0 ? buffer.slice(0, buffer.length - keep) : buffer; | |
| emitChatStreamTextDelta(state, text); | |
| state.contentBuffer = keep > 0 ? buffer.slice(buffer.length - keep) : ''; | |
| return; | |
| } | |
| if (start > 0) { | |
| emitChatStreamTextDelta(state, buffer.slice(0, start)); | |
| buffer = buffer.slice(start); | |
| } | |
| const end = buffer.indexOf(DSML_TOOL_CALLS_END, DSML_TOOL_CALLS_START.length); | |
| if (end < 0) { | |
| if (force) { | |
| state.contentBuffer = ''; | |
| } else { | |
| if (Buffer.byteLength(buffer, 'utf8') > DSML_CONTENT_BUFFER_MAX_BYTES) { | |
| state.contentBuffer = ''; | |
| failChatStreamResponsesSse(state, 'DSML tool-call block exceeded buffer limit'); | |
| return; | |
| } | |
| state.contentBuffer = buffer; | |
| } | |
| return; | |
| } | |
| const blockText = buffer.slice(DSML_TOOL_CALLS_START.length, end); | |
| appendDsmlToolCallsToChatStreamState(state, blockText); | |
| buffer = buffer.slice(end + DSML_TOOL_CALLS_END.length); | |
| } | |
| } | |
| function writeChatStreamContentSegment(state, segment) { | |
| if (!state || typeof segment !== 'string' || !segment) return; | |
| state.contentBuffer = (typeof state.contentBuffer === 'string' ? state.contentBuffer : '') + segment; | |
| flushChatStreamContentBuffer(state, false); |
🤖 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 `@cli/openai-bridge.js` around lines 1539 - 1574, The DSML buffering in
flushChatStreamContentBuffer/writeChatStreamContentSegment can grow without
bound when DSML_TOOL_CALLS_END never arrives, so add a maxBytes-based guard for
pending content. In flushChatStreamContentBuffer, track the buffered DSML block
size and trim or reject oversized incomplete blocks instead of keeping the full
buffer in state.contentBuffer; ensure the guard applies both when start/end
markers are missing and when buffering through writeChatStreamContentSegment.
Use the existing state fields and DSML marker handling to locate the change.
|
Updated the Codex streaming bridge to match dedicated Codex Responses→Chat proxy behavior instead of guessing via upstream Changes:
Validation run locally:
@coderabbitai re-review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
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 (1)
cli/openai-bridge.js (1)
2395-2408: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftStop retrying once SSE output has started.
streamChatCompletionsAsResponsesSsewritesresponse.createdbefore the upstream stream can time out, but the timeout handler still returns{ ok: false, error: 'timeout' }. That makesretryTransientRequestre-run the stream on the sameresafter the response has already been committed. Guard the timeout path with a started flag and finalize the SSE stream instead of retrying once output has begun.🤖 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 `@cli/openai-bridge.js` around lines 2395 - 2408, The retry loop around retryTransientRequest in the streamChatCompletionsAsResponsesSse call is re-running after SSE output has already started, which can write to the same res twice. Add a started/output-begun guard in streamChatCompletionsAsResponsesSse so the timeout path returns a non-retryable finalization once response.created or any SSE data has been sent, and only allow { ok: false, error: 'timeout' } before streaming begins. Use the existing streamChatCompletionsAsResponsesSse and retryTransientRequest flow to stop retries after the first committed chunk and finalize the SSE stream instead.
🤖 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 `@cli/openai-bridge.js`:
- Around line 2395-2408: The retry loop around retryTransientRequest in the
streamChatCompletionsAsResponsesSse call is re-running after SSE output has
already started, which can write to the same res twice. Add a
started/output-begun guard in streamChatCompletionsAsResponsesSse so the timeout
path returns a non-retryable finalization once response.created or any SSE data
has been sent, and only allow { ok: false, error: 'timeout' } before streaming
begins. Use the existing streamChatCompletionsAsResponsesSse and
retryTransientRequest flow to stop retries after the first committed chunk and
finalize the SSE stream instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 167615e1-e04e-470c-9643-c984a3f64146
📒 Files selected for processing (2)
cli/openai-bridge.jstests/unit/openai-bridge-upstream-responses.test.mjs
📜 Review details
🔇 Additional comments (6)
tests/unit/openai-bridge-upstream-responses.test.mjs (3)
177-179:await bridge.close()/await upstream.close()don't await socket drain (http.Server.closeis callback-based, not a promise), so this teardown can leave open handles and make the suite flaky. This is the same issue previously raised, and it now recurs in the new tests (Lines 177-179, 308-311, 439-442).
182-367: LGTM!
376-753: LGTM!cli/openai-bridge.js (3)
1727-1734: LGTM!
2382-2394: LGTM!
1134-1163: 🩺 Stability & AvailabilityNo missing SSE terminator here.
sendResponsesSsealready emitsdata: [DONE]afterresponse.completed, so this error path still closes the stream correctly.> Likely an incorrect or invalid review comment.
|
Addressed the latest CodeRabbit feedback in Fixes:
Validation run locally:
@coderabbitai re-review |
|
(•ᴗ•)🐇 ✏️ Learnings added
✅ Action performedReview finished.
|
1 participant
Summary
/v1/responsesrequests directly through the sub2api-style/v1/chat/completionshot pathresponse.createdon first chat chunk, no syntheticresponse.in_progressevent, reasoning/content/tool item lifecycles closed correctly): keepalivewhile an upstream chat stream is idle before the first chunk, without faking Responses eventsValidation
node --test tests/unit/builtin-proxy-responses-shim.test.mjs— 24/24 passed,TARGET_UNIT_EXIT=0npm run test:e2e—E2E_EXIT=0; stability run loggedelapsed_ms=808 cpu_user_us=10912 cpu_system_us=3683 metrics={"opened":14,"closed":14,"active":0,"maxActive":9}npm run test:unit— All 715 tests passed,UNIT_EXIT=0npm run lint— lint passed for 229 files,LINT_EXIT=0git diff --check—DIFF_CHECK_EXIT=0Notes
/responses; the test asserts no requests hit that endpoint and all conversion traffic goes to/chat/completions.response.createduntil the first upstream chat chunk arrives, preserving the sub2api event ordering.Summary by CodeRabbit
New Features
/v1/responseshandling via chat-completions conversion, including tool/function-call mapping and DSML tool-call extraction for streaming SSE.Bug Fixes
/v1/responsesprobing; improved streaming transport hardening (keepalive/teardown/header safety) and retry eligibility for transient network/timeout cases.response.created/reasoning/tool/output deltas; improved handling of DSML/text buffering and error completion behavior.Tests