fix(codexmate): harden bridge retries and OpenClaw UX#207
Conversation
📝 WalkthroughWalkthroughThe OpenAI bridge now retries transient upstream failures by default and safely handles post-response errors. Provider retry-count configuration was removed from bridge data and UI. OpenClaw summaries and quick-file controls were added, with related editor styling and collapsed sidebar sizing updates. ChangesOpenAI bridge resilience
Provider transform retry removal
OpenClaw configuration summaries
Collapsed sidebar layout
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BridgeClient
participant OpenAIBridge
participant UpstreamEndpoint
BridgeClient->>OpenAIBridge: request models or responses
OpenAIBridge->>UpstreamEndpoint: send upstream request
UpstreamEndpoint-->>OpenAIBridge: return transient status or network failure
OpenAIBridge->>UpstreamEndpoint: retry request
UpstreamEndpoint-->>OpenAIBridge: return successful response
OpenAIBridge-->>BridgeClient: return JSON or converted SSE
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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.
|
awsl233777
force-pushed
the
fix/builtin-proxy-sub2api-hot-path
branch
from
ec07f51 to
a9c9fd6
Compare
awsl233777
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
web-ui/partials/index/modals-basic.html (1)
56-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
max="10"to the retry input for UI consistency with the clamping range.
normalizeBridgeMaxRetriesclamps to a maximum of 10, but the input only specifiesmin="2". A user entering 15 will see it silently clamped to 10 on blur with no visual hint of the upper bound. Addingmax="10"aligns the HTML constraint with the normalization logic and improves discoverability.♻️ Proposed addition
v-model.number="newProvider.openaiBridgeMaxRetries" type="number" min="2" + max="10" step="1"Also applies to: 106-117
🤖 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 `@web-ui/partials/index/modals-basic.html` around lines 56 - 67, Add max="10" to the retry number inputs bound to newProvider.openaiBridgeMaxRetries, including both the add and edit provider form sections, so their HTML constraints match normalizeBridgeMaxRetries clamping.web-ui/modules/app.methods.providers.mjs (1)
44-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
normalizeBridgeMaxRetriesis duplicated incli/openai-bridge.js.The identical function appears at
cli/openai-bridge.js:44-49. If this is an intentional UI/CLI boundary separation, consider adding a shared utility module to prevent the implementations from diverging over time. If the duplication is acceptable for now, a brief comment noting the mirror would help future maintainers.🤖 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 `@web-ui/modules/app.methods.providers.mjs` around lines 44 - 49, normalizeBridgeMaxRetries is duplicated between the UI module and cli/openai-bridge.js, risking divergent behavior. Prefer extracting it into a shared utility and update both callers to import it; if the boundary requires duplication, add a brief comment in both implementations identifying the mirrored function.tests/unit/providers-validation.test.mjs (1)
183-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood test for minimum clamping — consider adding a parallel test for maximum clamping.
The test correctly verifies that
openaiBridgeMaxRetries: 1normalizes to2in the payload. A companion test verifying that a value like15clamps to10would round out the boundary coverage and guard against future changes to the upper bound.🤖 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/providers-validation.test.mjs` around lines 183 - 213, Add a companion test alongside the existing minimum-clamping test for the transform provider flow, using openaiBridgeMaxRetries: 15 and asserting that context.addProvider() submits openaiBridgeMaxRetries: 10 in the add-provider payload. Reuse the existing createContext and apiCalls assertion pattern to verify the upper-bound normalization.cli.js (1)
2565-2584: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate retry-clamping logic; reuse the exported helper instead.
normalizeOpenaiBridgeMaxRetriesreimplements the exact same clamp (default 2, range 2–10) asnormalizeBridgeMaxRetriesalready exported fromcli/openai-bridge.js(seeDEFAULT_BRIDGE_MAX_RETRIES/MIN_BRIDGE_MAX_RETRIES/MAX_BRIDGE_MAX_RETRIES). Two independently maintained copies of this clamp risk silently drifting apart if the range/default is tuned in one place later.♻️ Proposed fix
-function normalizeOpenaiBridgeMaxRetries(value, fallback = 2) { - const raw = Number(value); - const fallbackRaw = Number(fallback); - const base = Number.isFinite(raw) ? raw : (Number.isFinite(fallbackRaw) ? fallbackRaw : 2); - return Math.min(10, Math.max(2, Math.floor(base))); -} +// Reuse the canonical clamp from cli/openai-bridge.js instead of duplicating it here. +const normalizeOpenaiBridgeMaxRetries = normalizeBridgeMaxRetries;Requires
normalizeBridgeMaxRetriesto already be imported/required from./cli/openai-bridgeat the top of the file (it is exported there).🤖 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.js` around lines 2565 - 2584, Replace the duplicate clamp implementation in normalizeOpenaiBridgeMaxRetries with the imported normalizeBridgeMaxRetries helper from ./cli/openai-bridge, preserving the existing fallback behavior and provider resolution in resolveProviderOpenaiBridgeMaxRetries. Ensure the helper is required or imported at the top of cli.js and remove the local normalization logic.
🤖 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.js`:
- Around line 12882-12885: Use the normalized retry count returned by
resolveOpenaiBridgeUpstream in both openai-bridge-get-provider and
buildMcpProviderListPayload. Replace
resolveProviderOpenaiBridgeMaxRetries(provider) and the related config.toml
lookup with upstream.maxRetries when constructing each response, while
preserving the existing baseUrl and hasApiKey fields.
- Around line 16350-16399: Update runOpenAiChatTaskNode so its
postOpenAiChatCompletion call is wrapped with retryTransientRequest. For
OpenAI-bridge providers, pass resolveProviderOpenaiBridgeMaxRetries(provider) as
the retry limit; for other providers, use a sane default fallback, preserving
the existing request arguments and error handling.
In `@web-ui/modules/app.methods.providers.mjs`:
- Around line 127-129: The validation in the provider validation function for
openaiBridgeMaxRetries is unreachable because normalizeBridgeMaxRetries already
clamps values to at least 2. Remove the openaiBridgeMaxRetries < 2 error
assignment and its corresponding term in the ok computation; alternatively,
validate the raw draft value before calling normalizeBridgeMaxRetries if
user-facing errors are required.
---
Nitpick comments:
In `@cli.js`:
- Around line 2565-2584: Replace the duplicate clamp implementation in
normalizeOpenaiBridgeMaxRetries with the imported normalizeBridgeMaxRetries
helper from ./cli/openai-bridge, preserving the existing fallback behavior and
provider resolution in resolveProviderOpenaiBridgeMaxRetries. Ensure the helper
is required or imported at the top of cli.js and remove the local normalization
logic.
In `@tests/unit/providers-validation.test.mjs`:
- Around line 183-213: Add a companion test alongside the existing
minimum-clamping test for the transform provider flow, using
openaiBridgeMaxRetries: 15 and asserting that context.addProvider() submits
openaiBridgeMaxRetries: 10 in the add-provider payload. Reuse the existing
createContext and apiCalls assertion pattern to verify the upper-bound
normalization.
In `@web-ui/modules/app.methods.providers.mjs`:
- Around line 44-49: normalizeBridgeMaxRetries is duplicated between the UI
module and cli/openai-bridge.js, risking divergent behavior. Prefer extracting
it into a shared utility and update both callers to import it; if the boundary
requires duplication, add a brief comment in both implementations identifying
the mirrored function.
In `@web-ui/partials/index/modals-basic.html`:
- Around line 56-67: Add max="10" to the retry number inputs bound to
newProvider.openaiBridgeMaxRetries, including both the add and edit provider
form sections, so their HTML constraints match normalizeBridgeMaxRetries
clamping.
🪄 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: c1b206dc-3349-4b8f-b960-bd020b246cc4
📒 Files selected for processing (16)
cli.jscli/openai-bridge.jstests/unit/openai-bridge-upstream-responses.test.mjstests/unit/provider-default-names.test.mjstests/unit/provider-share-command.test.mjstests/unit/provider-switch-regression.test.mjstests/unit/providers-validation.test.mjsweb-ui/app.jsweb-ui/modules/app.methods.providers.mjsweb-ui/modules/i18n/locales/en.mjsweb-ui/modules/i18n/locales/ja.mjsweb-ui/modules/i18n/locales/vi.mjsweb-ui/modules/i18n/locales/zh-tw.mjsweb-ui/modules/i18n/locales/zh.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/res/web-ui-render.precompiled.js
✅ Files skipped from review due to trivial changes (8)
- web-ui/modules/i18n/locales/ja.mjs
- web-ui/modules/i18n/locales/zh-tw.mjs
- tests/unit/provider-switch-regression.test.mjs
- web-ui/modules/i18n/locales/vi.mjs
- web-ui/app.js
- web-ui/modules/i18n/locales/en.mjs
- web-ui/modules/i18n/locales/zh.mjs
- tests/unit/provider-share-command.test.mjs
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CodeRabbit
🧰 Additional context used
🪛 ast-grep (0.44.1)
cli.js
[warning] 10040-10040: Detects non-literal values in regular expressions
Context: new RegExp(^(\\s*${escapedFieldName}\\s*=\\s*)([-+]?\\d+(?:\\.\\d+)?)(\\s+#.*)?$, 'mg')
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
[warning] 16660-16660: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(normalized.path, fileContent, { encoding: 'utf-8', mode: 0o600 })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🪛 OpenGrep (1.23.0)
cli.js
[ERROR] 16621-16621: 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 (12)
web-ui/res/web-ui-render.precompiled.js (1)
7098-7127: LGTM!Also applies to: 7240-7269
tests/unit/provider-default-names.test.mjs (1)
41-42: LGTM!tests/unit/providers-validation.test.mjs (1)
25-26: LGTM!Also applies to: 97-97
web-ui/partials/index/modals-basic.html (1)
56-67: 🎯 Functional Correctnessi18n keys already exist in all locales
field.transformMaxRetriesandhint.transformMaxRetriesare present in every locale file.> Likely an incorrect or invalid review comment.web-ui/modules/app.methods.providers.mjs (1)
450-456: 🎯 Functional CorrectnessRemove the transform-off concern — the edit modal only shows bridge settings for existing transform providers, and the update path keeps those providers in the transform branch unless their stored config is no longer bridge-shaped. There’s no edit flow here that disables
useTransform.> Likely an incorrect or invalid review comment.tests/unit/openai-bridge-upstream-responses.test.mjs (2)
138-140: Unwrappedserver.close()reintroduced in this new test.Same issue flagged (and marked addressed) on other tests in this file:
http.Server.close()without a callback doesn't return a promise, soawait bridge.close()/await upstream.close()resolve immediately instead of waiting for sockets to drain, risking leaked handles/flakiness in this newly added test.Proposed fix
- await bridge.close(); - await upstream.close(); + await Promise.all([ + new Promise((resolve, reject) => bridge.close((err) => err ? reject(err) : resolve())), + new Promise((resolve, reject) => upstream.close((err) => err ? reject(err) : resolve())) + ]);
96-141: LGTM on the retry-count assertion logic itself —maxRetries: 3correctly yields 4 total upstream hits (attempt0..3) and a final502, matchingretryTransientRequest's loop bounds.cli.js (2)
2586-2738: LGTM! Wiring ofopenaiBridgeMaxRetriesthroughaddProviderToConfig,updateProviderInConfig, andcmdUpdate(including the newreplaceTomlNumberFieldhelper) is consistent — TOML and the bridge settings file are updated together, and the "no URL and no existing upstream" edge case correctly throws before any partial write.Also applies to: 9873-10036, 10089-10170
10037-10062: LGTM!replaceTomlNumberFieldmirrors the existingreplaceTomlStringField/replaceTomlBooleanFieldpatterns (multiline-string range protection, fallback insertion). The static-analysis "non-literal RegExp" hint is a false positive here sincefieldNameis always the hardcoded literal'codexmate_bridge_max_retries'from the sole call site and is passed throughescapeRegexregardless.cli/openai-bridge.js (3)
91-117: LGTM!normalizeBridgeMaxRetries,normalizeUpstreamEntry, and the updatedupsertOpenaiBridgeProvidercorrectly derive, clamp, and persistmaxRetriesalongside the existing baseUrl/apiKey/headers handling.Also applies to: 162-195
2211-2211: LGTM! All five upstream call sites now consistently pass{ maxRetries: upstream.maxRetries }, and the export ofnormalizeBridgeMaxRetriesis straightforward.Also applies to: 2266-2266, 2313-2313, 2344-2344, 2406-2406, 2478-2478
1259-1286: 🩺 Stability & AvailabilityNo change needed for
result.retry
result.retryintentionally bubbles out so the caller can switch to the fallback URL/path; it is not meant to trigger another pass throughretryTransientRequest.> Likely an incorrect or invalid review comment.
awsl233777
force-pushed
the
fix/builtin-proxy-sub2api-hot-path
branch
from
a9c9fd6 to
003a8ea
Compare
awsl233777
force-pushed
the
fix/builtin-proxy-sub2api-hot-path
branch
from
003a8ea to
7628878
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/openai-bridge-upstream-responses.test.mjs (1)
1646-1660: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest exercises the
writableEndedearly-return, not theheadersSent-but-not-ended branch it's named after.
patchedEndcallsoriginalEnd(chunk, encoding, cb)before throwing, sores.writableEndedis alreadytrueby the time the simulated error reaches the bridge's catch block. Per the referenced catch logic (if (res.writableEnded || res.destroyed) { return; }runs before theres.headersSentcheck), this test validates thewritableEndedguard, not the "headers sent but response not yet ended" scenario the test name and layer description ("ends responses with sent headers") imply — e.g. a mid-stream failure afterwriteHead/write()but beforeend(). Consider throwing from a patchedres.write()(exercised by the SSE/streaming path) instead ofres.end()to actually cover theheadersSent && !writableEndedfallback.🤖 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/openai-bridge-upstream-responses.test.mjs` around lines 1646 - 1660, Update the test’s response-failure simulation around patchedEnd so it exercises the headersSent-but-not-writableEnded fallback. Patch res.write in the SSE/streaming path to throw after writeHead or an initial write, before res.end is called, and retain the assertion that no second writeHead occurs; avoid calling originalEnd before the simulated failure.
🤖 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/openai-bridge-upstream-responses.test.mjs`:
- Around line 1638-1681: Wrap the listener registration, bridge request, and
assertions in a try/finally block within the test, and move
process.removeListener('uncaughtException', onUncaught) into the finally clause.
Ensure the existing server and temporary-directory cleanup also remains in
guaranteed cleanup paths so assertion failures cannot leak listeners or
resources.
---
Nitpick comments:
In `@tests/unit/openai-bridge-upstream-responses.test.mjs`:
- Around line 1646-1660: Update the test’s response-failure simulation around
patchedEnd so it exercises the headersSent-but-not-writableEnded fallback. Patch
res.write in the SSE/streaming path to throw after writeHead or an initial
write, before res.end is called, and retain the assertion that no second
writeHead occurs; avoid calling originalEnd before the simulated failure.
🪄 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: b80010b4-42e8-4371-b4fe-c4a9d3f671ef
📒 Files selected for processing (2)
cli/openai-bridge.jstests/unit/openai-bridge-upstream-responses.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/openai-bridge.js
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: CodeRabbit
- GitHub Check: ci (20)
- GitHub Check: ci (18)
🔇 Additional comments (1)
tests/unit/openai-bridge-upstream-responses.test.mjs (1)
1613-1685: 🩺 Stability & AvailabilityLikely repeats the unwrapped
server.close()pattern flagged earlier in this file.Per file convention (see lines 138-140, 224-226, 291-293, 1608-1610), cleanup for this new test almost certainly ends with raw
await bridge.close(); await upstream.close();calls, which don't actually wait for socket drain sincehttp.Server.close()isn't a promise. This exact concern was raised previously on this file and marked "Addressed in commit ec07f51," yet the same unwrapped pattern still appears at multiple points in the file shown here, so it's worth confirming whether the new test's (unseen) teardown at lines ~1682-1685 was updated to use promise-wrapped shutdown.#!/bin/bash rg -n 'close\(' tests/unit/openai-bridge-upstream-responses.test.mjs | tail -20 sed -n '1680,1690p' tests/unit/openai-bridge-upstream-responses.test.mjs
| let uncaught = null; | ||
| const onUncaught = (err) => { uncaught = err; }; | ||
| process.once('uncaughtException', onUncaught); | ||
|
|
||
| const handler = createOpenaiBridgeHttpHandler({ settingsFile, expectedToken: 'codexmate' }); | ||
| const bridge = http.createServer((req, res) => { | ||
| const originalWriteHead = res.writeHead.bind(res); | ||
| let wroteSuccessHeader = false; | ||
| res.writeHead = function patchedWriteHead(statusCode, headers) { | ||
| if (statusCode === 200) wroteSuccessHeader = true; | ||
| if (wroteSuccessHeader && statusCode === 500) { | ||
| throw new Error('regression: attempted second writeHead after headers sent'); | ||
| } | ||
| return originalWriteHead(statusCode, headers); | ||
| }; | ||
| const originalEnd = res.end.bind(res); | ||
| res.end = function patchedEnd(chunk, encoding, cb) { | ||
| if (wroteSuccessHeader && !res.writableEnded) { | ||
| originalEnd(chunk, encoding, cb); | ||
| throw new Error('simulated post-header write failure'); | ||
| } | ||
| return originalEnd(chunk, encoding, cb); | ||
| }; | ||
| if (!handler(req, res)) { | ||
| res.statusCode = 404; | ||
| res.end('not handled'); | ||
| } | ||
| }); | ||
| const { port: bridgePort } = await listen(bridge); | ||
|
|
||
| const resp = await requestText(`http://127.0.0.1:${bridgePort}/bridge/openai/test/v1/responses`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Accept': 'application/json', | ||
| 'Authorization': 'Bearer codexmate' | ||
| }, | ||
| body: { model: 'gpt-test', input: 'ping' } | ||
| }); | ||
|
|
||
| assert.equal(resp.status, 200); | ||
| assert.equal(uncaught, null); | ||
|
|
||
| process.removeListener('uncaughtException', onUncaught); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wrap cleanup in try/finally to avoid leaking the uncaughtException listener on assertion failure.
If assert.equal(resp.status, 200) or assert.equal(uncaught, null) fails, process.removeListener('uncaughtException', onUncaught) (and presumably the subsequent server/tmpDir cleanup) never runs. Since registering any 'uncaughtException' listener suppresses Node's default crash behavior, a leaked .once() listener can silently consume an unrelated uncaught exception thrown by a later test in the same process, masking a real failure.
Proposed fix
- const resp = await requestText(`http://127.0.0.1:${bridgePort}/bridge/openai/test/v1/responses`, {
- method: 'POST',
- headers: {
- 'Content-Type': 'application/json',
- 'Accept': 'application/json',
- 'Authorization': 'Bearer codexmate'
- },
- body: { model: 'gpt-test', input: 'ping' }
- });
-
- assert.equal(resp.status, 200);
- assert.equal(uncaught, null);
-
- process.removeListener('uncaughtException', onUncaught);
+ try {
+ const resp = await requestText(`http://127.0.0.1:${bridgePort}/bridge/openai/test/v1/responses`, {
+ method: 'POST',
+ headers: {
+ 'Content-Type': 'application/json',
+ 'Accept': 'application/json',
+ 'Authorization': 'Bearer codexmate'
+ },
+ body: { model: 'gpt-test', input: 'ping' }
+ });
+
+ assert.equal(resp.status, 200);
+ assert.equal(uncaught, null);
+ } finally {
+ process.removeListener('uncaughtException', onUncaught);
+ }📝 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.
| let uncaught = null; | |
| const onUncaught = (err) => { uncaught = err; }; | |
| process.once('uncaughtException', onUncaught); | |
| const handler = createOpenaiBridgeHttpHandler({ settingsFile, expectedToken: 'codexmate' }); | |
| const bridge = http.createServer((req, res) => { | |
| const originalWriteHead = res.writeHead.bind(res); | |
| let wroteSuccessHeader = false; | |
| res.writeHead = function patchedWriteHead(statusCode, headers) { | |
| if (statusCode === 200) wroteSuccessHeader = true; | |
| if (wroteSuccessHeader && statusCode === 500) { | |
| throw new Error('regression: attempted second writeHead after headers sent'); | |
| } | |
| return originalWriteHead(statusCode, headers); | |
| }; | |
| const originalEnd = res.end.bind(res); | |
| res.end = function patchedEnd(chunk, encoding, cb) { | |
| if (wroteSuccessHeader && !res.writableEnded) { | |
| originalEnd(chunk, encoding, cb); | |
| throw new Error('simulated post-header write failure'); | |
| } | |
| return originalEnd(chunk, encoding, cb); | |
| }; | |
| if (!handler(req, res)) { | |
| res.statusCode = 404; | |
| res.end('not handled'); | |
| } | |
| }); | |
| const { port: bridgePort } = await listen(bridge); | |
| const resp = await requestText(`http://127.0.0.1:${bridgePort}/bridge/openai/test/v1/responses`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'Accept': 'application/json', | |
| 'Authorization': 'Bearer codexmate' | |
| }, | |
| body: { model: 'gpt-test', input: 'ping' } | |
| }); | |
| assert.equal(resp.status, 200); | |
| assert.equal(uncaught, null); | |
| process.removeListener('uncaughtException', onUncaught); | |
| let uncaught = null; | |
| const onUncaught = (err) => { uncaught = err; }; | |
| process.once('uncaughtException', onUncaught); | |
| const handler = createOpenaiBridgeHttpHandler({ settingsFile, expectedToken: 'codexmate' }); | |
| const bridge = http.createServer((req, res) => { | |
| const originalWriteHead = res.writeHead.bind(res); | |
| let wroteSuccessHeader = false; | |
| res.writeHead = function patchedWriteHead(statusCode, headers) { | |
| if (statusCode === 200) wroteSuccessHeader = true; | |
| if (wroteSuccessHeader && statusCode === 500) { | |
| throw new Error('regression: attempted second writeHead after headers sent'); | |
| } | |
| return originalWriteHead(statusCode, headers); | |
| }; | |
| const originalEnd = res.end.bind(res); | |
| res.end = function patchedEnd(chunk, encoding, cb) { | |
| if (wroteSuccessHeader && !res.writableEnded) { | |
| originalEnd(chunk, encoding, cb); | |
| throw new Error('simulated post-header write failure'); | |
| } | |
| return originalEnd(chunk, encoding, cb); | |
| }; | |
| if (!handler(req, res)) { | |
| res.statusCode = 404; | |
| res.end('not handled'); | |
| } | |
| }); | |
| const { port: bridgePort } = await listen(bridge); | |
| try { | |
| const resp = await requestText(`http://127.0.0.1:${bridgePort}/bridge/openai/test/v1/responses`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'Accept': 'application/json', | |
| 'Authorization': 'Bearer codexmate' | |
| }, | |
| body: { model: 'gpt-test', input: 'ping' } | |
| }); | |
| assert.equal(resp.status, 200); | |
| assert.equal(uncaught, null); | |
| } finally { | |
| process.removeListener('uncaughtException', onUncaught); | |
| } |
🤖 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/openai-bridge-upstream-responses.test.mjs` around lines 1638 -
1681, Wrap the listener registration, bridge request, and assertions in a
try/finally block within the test, and move
process.removeListener('uncaughtException', onUncaught) into the finally clause.
Ensure the existing server and temporary-directory cleanup also remains in
guaranteed cleanup paths so assertion failures cannot leak listeners or
resources.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/openai-bridge-retry.js (1)
30-33: 🚀 Performance & Scalability | 🔵 TrivialConsider respecting
Retry-Afterfor 429/503 if available.
getTransientRetryDelayMsalways uses fixed exponential backoff regardless of status. For429/503many upstreams return aRetry-Afterheader indicating the correct wait time; a blind exponential backoff can under- or over-wait relative to what the provider expects. This is only actionable ifresult(fromproxyRequestJson) surfaces response headers toretryTransientRequest— worth checking before implementing.🤖 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-retry.js` around lines 30 - 33, Update the transient retry flow around getTransientRetryDelayMs and retryTransientRequest to inspect response headers from proxyRequestJson when available; for 429/503 responses, parse a valid Retry-After value and use it as the retry delay, while preserving the existing capped exponential backoff when headers are absent or invalid. If proxyRequestJson does not expose headers, first extend its result handling so retryTransientRequest can access them.
🤖 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 `@cli/openai-bridge-retry.js`:
- Around line 30-33: Update the transient retry flow around
getTransientRetryDelayMs and retryTransientRequest to inspect response headers
from proxyRequestJson when available; for 429/503 responses, parse a valid
Retry-After value and use it as the retry delay, while preserving the existing
capped exponential backoff when headers are absent or invalid. If
proxyRequestJson does not expose headers, first extend its result handling so
retryTransientRequest can access them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cfe554ec-c3f6-4d8b-854e-3c9ca4d9349c
📒 Files selected for processing (3)
cli/openai-bridge-retry.jscli/openai-bridge.jstests/unit/openai-bridge-upstream-responses.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/openai-bridge.js
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: CodeRabbit
- GitHub Check: ci (18)
- GitHub Check: ci (18)
🔇 Additional comments (5)
tests/unit/openai-bridge-upstream-responses.test.mjs (3)
144-197: Recurring un-promisified shutdown + no try/finally (same class of issue flagged previously in this file).
bridge.close()/upstream.close()at Lines 193-194 are rawhttp.Server.close()calls —awaitresolves immediately without waiting for socket drain, same as the earlier flagged/addressed instance. Additionally, none of the assertions (Lines 189-191) are wrapped in try/finally, so a failed assertion (e.g.hitCount !== 3) skips server/tmpDir cleanup entirely, leaking two listening servers and a temp directory into later test runs.Proposed fix
- const resp = await requestText(`http://127.0.0.1:${bridgePort}/bridge/openai/test/v1/models`, { - headers: { Authorization: 'Bearer codexmate' } - }); - - assert.equal(resp.status, 200); - assert.equal(hitCount, 3); - assert.deepEqual(JSON.parse(resp.text), { object: 'list', data: [{ id: 'gpt-test' }] }); - - await bridge.close(); - await upstream.close(); - await rm(tmpDir, { recursive: true, force: true }); + try { + const resp = await requestText(`http://127.0.0.1:${bridgePort}/bridge/openai/test/v1/models`, { + headers: { Authorization: 'Bearer codexmate' } + }); + + assert.equal(resp.status, 200); + assert.equal(hitCount, 3); + assert.deepEqual(JSON.parse(resp.text), { object: 'list', data: [{ id: 'gpt-test' }] }); + } finally { + await Promise.all([ + new Promise((resolve, reject) => bridge.close((err) => err ? reject(err) : resolve())), + new Promise((resolve, reject) => upstream.close((err) => err ? reject(err) : resolve())) + ]); + await rm(tmpDir, { recursive: true, force: true }); + }
1668-1740: 🩺 Stability & AvailabilityVerify the previously-flagged try/finally fix for the uncaughtException listener was applied here.
This range (per the line-range summary) is the "no HTTP 500/second write after headers sent" regression test, matching the test a prior review flagged for leaking the
uncaughtExceptionlistener on assertion failure — that prior comment doesn't carry an "✅ Addressed" marker (unlike the sibling comment on the close()-promisification fix), so it may still be open. This exact code isn't in the provided snippet; please confirm current state.#!/bin/bash sed -n '1655,1745p' tests/unit/openai-bridge-upstream-responses.test.mjs
1742-1809: Same close()-promisification/try-finally gap as the earlier new test.Lines 1806-1808 repeat the raw
await bridge.close(); await upstream.close();pattern without try/finally around the assertions above (Lines 1793-1804), so a failedassert.rejects/assert.equal(destroyCalled, ...)again leaks both servers and tmpDir. Same fix as proposed for the 524-retry test applies here.Aside from that, the test logic itself is sound: since the patched
res.endthrows before invoking the original, no bytes are ever flushed to the socket (Node buffers headers until the firstwrite/end), so the eventualres.destroy(e)manifests to the client as a connection-reset on the request object, whichrequestText'sreq.on('error', reject)correctly surfaces viaassert.rejects.cli/openai-bridge-retry.js (2)
14-17: 🎯 Functional CorrectnessConfirm intent of treating 409/425 as transient/retryable.
Standard retry sets for HTTP APIs (OpenAI-compatible gateways, gRPC-derived retry policies, etc.) typically cover
408/429/500/502/503/504(and sometimes499);409 Conflictand425 Too Earlyare unusual additions — a 409 conflict generally won't resolve from a blind retry of the same request, and blindly retrying it can mask a real error or waste retry budget before surfacing a definitive failure to the user.If a specific upstream provider is known to emit
409/425transiently (e.g. as a busy/lock signal), this is fine — otherwise consider dropping them from the transient set.
51-56: LGTM!Also applies to: 68-68
awsl233777
changed the title
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 `@web-ui/styles/layout-shell.css`:
- Around line 41-43: Insert a blank line between the --side-rail-width
custom-property declaration and the grid-template-columns declaration in
.app-shell.sidebar-collapsed to satisfy declaration-empty-line-before.
🪄 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: 9debaad6-dcc7-4062-82d2-f9fd62fe5649
📒 Files selected for processing (1)
web-ui/styles/layout-shell.css
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: CodeRabbit
- GitHub Check: ci (18)
- GitHub Check: ci (18)
🧰 Additional context used
🪛 Stylelint (17.14.0)
web-ui/styles/layout-shell.css
[error] 43-43: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
| .app-shell.sidebar-collapsed { | ||
| --side-rail-width: var(--side-rail-collapsed-width, 136px); | ||
| grid-template-columns: var(--side-rail-collapsed-width, 136px) minmax(0, 1fr); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required blank line before the new declaration.
Stylelint reports declaration-empty-line-before for line 43; insert an empty line after the custom-property declaration.
Proposed fix
.app-shell.sidebar-collapsed {
--side-rail-width: var(--side-rail-collapsed-width, 136px);
+
grid-template-columns: var(--side-rail-collapsed-width, 136px) minmax(0, 1fr);
}📝 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.
| .app-shell.sidebar-collapsed { | |
| --side-rail-width: var(--side-rail-collapsed-width, 136px); | |
| grid-template-columns: var(--side-rail-collapsed-width, 136px) minmax(0, 1fr); | |
| .app-shell.sidebar-collapsed { | |
| --side-rail-width: var(--side-rail-collapsed-width, 136px); | |
| grid-template-columns: var(--side-rail-collapsed-width, 136px) minmax(0, 1fr); | |
| } |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 43-43: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 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 `@web-ui/styles/layout-shell.css` around lines 41 - 43, Insert a blank line
between the --side-rail-width custom-property declaration and the
grid-template-columns declaration in .app-shell.sidebar-collapsed to satisfy
declaration-empty-line-before.
Source: Linters/SAST tools
awsl233777
changed the title
awsl233777
changed the title
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-retry.js (1)
14-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor
Retry-Afteron transient retries.isTransientHttpStatus()already includes 429, butretryTransientRequest()always uses the fixed backoff and ignores the server’sRetry-Afterhint, so rate-limited requests can be retried too early.🤖 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-retry.js` around lines 14 - 17, Update retryTransientRequest() to read the response’s Retry-After header for transient HTTP failures, including status 429, and delay the retry until at least the server-specified duration. Preserve the existing fixed backoff as the fallback when the header is absent or invalid, while leaving isTransientHttpStatus() unchanged.
🧹 Nitpick comments (2)
cli/openai-bridge.js (1)
512-529: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider logging when
res.end()succeeds after a mid-flight error.When
res.headersSentis true andres.end()succeeds (line 519), the original erroreis silently swallowed — no log, no metric. This makes mid-flight failures invisible to operators. A singleconsole.erroror structured log here would aid debugging without changing behavior.♻️ Suggested addition
if (res.headersSent) { try { res.end(); } catch (_) { // Headers are already committed. Close the socket instead of leaving the client waiting forever. if (!res.destroyed && typeof res.destroy === 'function') { res.destroy(e); } } + // Best-effort log: the client received a truncated response. + if (typeof console !== 'undefined' && console.error) { + console.error('[openai-bridge] mid-flight error after headers sent:', message); + } 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/openai-bridge.js` around lines 512 - 529, Add logging for the original error in the res.headersSent branch of the catch handler when res.end() completes successfully, while preserving the existing response behavior and fallback socket-destruction handling.cli/openai-bridge-retry.js (1)
38-58: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a maximum total retry duration or attempt cap for production safety.
With
DEFAULT_BRIDGE_MAX_RETRIES = Infinityand backoff capped at 5s, a persistently failing upstream will cause the bridge to retry indefinitely (~5s per attempt after ~8 attempts). Multiple concurrent clients hitting a failing upstream could accumulate retry loops holding connections open, risking resource exhaustion. Consider adding an optionalmaxRetryDurationMsor a high finite ceiling (e.g., 100) as a safety net, even if the default remains effectively unlimited.🤖 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-retry.js` around lines 38 - 58, Add a production safety bound to the retry loop in the bridge executor, using either an optional maxRetryDurationMs or a high finite attempt cap while preserving the existing retry behavior by default. Update the loop around executor(attempt) to stop retrying once the configured duration or cap is reached, and return the latest result rather than continuing indefinitely.
🤖 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-retry.js`:
- Around line 14-17: Update retryTransientRequest() to read the response’s
Retry-After header for transient HTTP failures, including status 429, and delay
the retry until at least the server-specified duration. Preserve the existing
fixed backoff as the fallback when the header is absent or invalid, while
leaving isTransientHttpStatus() unchanged.
---
Nitpick comments:
In `@cli/openai-bridge-retry.js`:
- Around line 38-58: Add a production safety bound to the retry loop in the
bridge executor, using either an optional maxRetryDurationMs or a high finite
attempt cap while preserving the existing retry behavior by default. Update the
loop around executor(attempt) to stop retrying once the configured duration or
cap is reached, and return the latest result rather than continuing
indefinitely.
In `@cli/openai-bridge.js`:
- Around line 512-529: Add logging for the original error in the res.headersSent
branch of the catch handler when res.end() completes successfully, while
preserving the existing response behavior and fallback socket-destruction
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0e3cc67-4423-439f-beb5-c405c514dc89
📒 Files selected for processing (15)
cli/openai-bridge-retry.jscli/openai-bridge.jstests/unit/openai-bridge-upstream-responses.test.mjstests/unit/provider-default-names.test.mjstests/unit/provider-switch-regression.test.mjstests/unit/providers-validation.test.mjsweb-ui/app.jsweb-ui/modules/app.methods.providers.mjsweb-ui/modules/i18n/locales/en.mjsweb-ui/modules/i18n/locales/ja.mjsweb-ui/modules/i18n/locales/vi.mjsweb-ui/modules/i18n/locales/zh-tw.mjsweb-ui/modules/i18n/locales/zh.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/res/web-ui-render.precompiled.js
💤 Files with no reviewable changes (8)
- web-ui/modules/i18n/locales/zh-tw.mjs
- web-ui/modules/i18n/locales/zh.mjs
- web-ui/modules/i18n/locales/en.mjs
- web-ui/modules/i18n/locales/ja.mjs
- web-ui/partials/index/modals-basic.html
- web-ui/modules/i18n/locales/vi.mjs
- web-ui/res/web-ui-render.precompiled.js
- tests/unit/provider-default-names.test.mjs
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CodeRabbit
🔇 Additional comments (23)
cli/openai-bridge.js (5)
434-445: 🩺 Stability & AvailabilityStreaming retry safety concern — see verification request on
openai-bridge-retry.js.This call passes
resdirectly tostreamChatCompletionsAsResponsesSse. If the streaming function commits headers/SSE output and then fails with a transient status, the retry loop (which checksresult.statusbeforeresult.retry) will retry and attempt to write to an already-committed response. The downstream error handling at lines 446-458 checksres.headersSent, but that only runs afterretryTransientRequestreturns — it doesn't prevent the retry itself.
108-119: LGTM!
165-195: LGTM!
374-383: LGTM!Also applies to: 463-470
551-558: LGTM!tests/unit/openai-bridge-upstream-responses.test.mjs (3)
1810-1877: 🩺 Stability & AvailabilityVerify whether the promise-wrapped server shutdown pattern is applied consistently in this new test.
A past review comment flagged that
await bridge.close()/await upstream.close()return immediately becausehttp.Server.close()returns the server object, not a Promise. The comment was marked "✅ Addressed in commit ec07f51," but this newly added test (lines 1874-1876) still uses the rawawaitpattern. If the fix was applied to other tests, this new test should use the same pattern for consistency and to avoid leaking open handles.#!/bin/bash # Check if any tests in this file use promise-wrapped server.close() rg -n 'new Promise.*close' tests/unit/openai-bridge-upstream-responses.test.mjs # Check the pattern used for server shutdown across all tests rg -n 'await (bridge|upstream)\.close' tests/unit/openai-bridge-upstream-responses.test.mjs
96-146: LGTM!Also applies to: 148-199, 202-259
1736-1808: LGTM!cli/openai-bridge-retry.js (2)
1-11: LGTM!
38-58: 🩺 Stability & AvailabilityNo retry-after-commit issue here.
streamChatCompletionsAsResponsesSse()only returns{ status: ... }before any SSE headers are written; once it has committed output, it resolves{ ok: true }or finishes the stream with SSE failure events. The retry loop won’t retry a committed stream on the current code path.> Likely an incorrect or invalid review comment.web-ui/app.js (1)
293-295: LGTM!web-ui/modules/app.methods.providers.mjs (8)
44-58: LGTM!
72-124: LGTM!
173-223: LGTM!
318-344: LGTM!
346-414: LGTM!
416-466: LGTM!
473-473: LGTM!
556-556: LGTM!tests/unit/providers-validation.test.mjs (3)
25-26: LGTM!
97-97: LGTM!
184-208: LGTM!tests/unit/provider-switch-regression.test.mjs (1)
273-273: LGTM!
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 `@cli/openai-bridge-runtime.js`:
- Around line 1475-1488: Add the next sequence value to the top-level payload of
the response.created event, matching the Responses streaming schema. Keep
response.in_progress using its existing state.nextSeq() call and do not alter
its ordering behavior.
🪄 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: 7119aa9e-aba4-4999-bb61-13c1741128ce
📒 Files selected for processing (3)
cli/openai-bridge-retry.jscli/openai-bridge-runtime.jstests/unit/openai-bridge-upstream-responses.test.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/openai-bridge-retry.js
- tests/unit/openai-bridge-upstream-responses.test.mjs
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CodeRabbit
| created_at: state.createdAt, | ||
| status: 'in_progress', | ||
| output: [] | ||
| } | ||
| }); | ||
| writeSse(res, 'response.in_progress', { | ||
| type: 'response.in_progress', | ||
| sequence_number: state.nextSeq(), | ||
| response: { | ||
| id: state.responseId, | ||
| model: state.model, | ||
| created_at: state.createdAt, | ||
| status: 'in_progress', | ||
| output: [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1468,1492p' cli/openai-bridge-runtime.js
rg -n -A8 -B4 'response\.created|response\.in_progress|sequence_number' tests/unit/openai-bridge-upstream-responses.test.mjsRepository: SakuraByteCore/codexmate
Length of output: 997
🏁 Script executed:
#!/bin/bash
set -eu
printf '\n== response event emitters ==\n'
rg -n -A6 -B4 "writeSse\\(res, 'response\\.(created|in_progress|completed|failed|incomplete)'" cli/openai-bridge-runtime.js
printf '\n== sequence number helpers/usages ==\n'
rg -n "nextSeq\\(|sequence_number" cli tests --glob '!**/node_modules/**'
printf '\n== response streaming tests ==\n'
sed -n '1,260p' tests/unit/openai-bridge-upstream-responses.test.mjsRepository: SakuraByteCore/codexmate
Length of output: 17661
🏁 Script executed:
#!/bin/bash
set -eu
printf '\n== shared Responses SSE emitter ==\n'
sed -n '800,920p' cli/openai-bridge-runtime.js
printf '\n== tests mentioning response.created or sequence_number ==\n'
rg -n -A4 -B4 "response\\.created|response\\.in_progress|sequence_number" tests cli/openai-bridge-runtime.js cli/openai-bridge.js
printf '\n== response SSE parsing helpers in tests ==\n'
rg -n -A5 -B5 "event: 'response\\.|type: 'response\\.|sequence_number" tests/unit/openai-bridge-upstream-responses.test.mjsRepository: SakuraByteCore/codexmate
Length of output: 18992
🌐 Web query:
OpenAI Responses streaming schema response.created sequence_number required response.in_progress
💡 Result:
In the OpenAI Responses API, streaming events follow a structured schema that includes both type and sequence_number fields [1][2]. Key Event Types: - response.created: Emitted immediately after a response is created. It includes the response object and the sequence_number [2][3]. - response.in_progress: Emitted while the response is being generated. Like other lifecycle events, it includes the response object and the sequence_number [2][3]. Sequence Number: The sequence_number is a required integer field for streaming events in the Responses API [1][4][5]. It is monotonically increasing for each stream and is used by clients to enforce correct event ordering and to ignore duplicate events [6]. Documentation and Implementation: Official OpenAI documentation defines these events with a specific schema [1]. Client libraries, such as the official Node.js SDK, define these events in their type definitions (e.g., ResponseCreatedEvent and ResponseInProgressEvent) as requiring both a type and a sequence_number [7][2]. While the API specification mandates these fields, developers have noted that implementation behavior can occasionally deviate from the spec in specific scenarios (such as error events), requiring robust handling in client implementations [5]. Note: This structure specifically applies to the Responses API (often used in the context of the Assistants API/Realtime capabilities) and differs from the traditional Chat Completions streaming API, which utilizes a different chunk-based structure (chat.completion.chunk) [8][9][10].
Citations:
- 1: https://developers.openai.com/api/reference/resources/responses/streaming-events/
- 2: https://cdn.jsdelivr.net/npm/openai@6.14.0/src/resources/responses/responses.ts
- 3: https://community.openai.com/t/responses-api-streaming-the-simple-guide-to-events/1363122/1
- 4: fix missing sequence numbers openai-php/client#764
- 5: [BUG] StreamingResponseErrorUpdate Silently Fails to Deserialize openai/openai-dotnet#1176
- 6: https://community.openai.com/t/responses-api-streaming-the-simple-guide-to-events/1363122
- 7: https://github.com/openai/openai-node/blob/c9a4d688/src/resources/responses/responses.ts
- 8: https://developers.openai.com/api/docs/guides/streaming-responses
- 9: https://developers.openai.com/api/reference/resources/chat/subresources/completions/streaming-events/
- 10: https://github.com/openai/openai-python/blob/main/src/openai/types/chat/chat_completion_chunk.py
🏁 Script executed:
#!/bin/bash
set -eu
printf '\n== local docs for Responses sequencing ==\n'
rg -n "sequence_number|response\\.created|response\\.in_progress|Responses streaming|streaming schema" README* docs cli tests --glob '!**/node_modules/**'
printf '\n== all response.created emitters ==\n'
rg -n -A5 -B3 "writeSse\\(res, 'response\\.created'" cli testsRepository: SakuraByteCore/codexmate
Length of output: 4152
Add sequence_number to response.created. This lifecycle event is missing the top-level ordering field, so it doesn’t match the Responses streaming schema; response.in_progress should keep the next sequence value.
Proposed fix
created_at: state.createdAt,
status: 'in_progress',
output: []
- }
+ },
+ sequence_number: state.nextSeq()
});📝 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.
| created_at: state.createdAt, | |
| status: 'in_progress', | |
| output: [] | |
| } | |
| }); | |
| writeSse(res, 'response.in_progress', { | |
| type: 'response.in_progress', | |
| sequence_number: state.nextSeq(), | |
| response: { | |
| id: state.responseId, | |
| model: state.model, | |
| created_at: state.createdAt, | |
| status: 'in_progress', | |
| output: [] | |
| created_at: state.createdAt, | |
| status: 'in_progress', | |
| output: [] | |
| }, | |
| sequence_number: state.nextSeq() | |
| }); | |
| writeSse(res, 'response.in_progress', { | |
| type: 'response.in_progress', | |
| sequence_number: state.nextSeq(), | |
| response: { | |
| id: state.responseId, | |
| model: state.model, | |
| created_at: state.createdAt, | |
| 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/openai-bridge-runtime.js` around lines 1475 - 1488, Add the next sequence
value to the top-level payload of the response.created event, matching the
Responses streaming schema. Keep response.in_progress using its existing
state.nextSeq() call and do not alter its ordering behavior.
Source: MCP tools
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 `@web-ui/modules/app.methods.openclaw-core.mjs`:
- Around line 500-538: Update getOpenclawConfigSummary’s legacy agent.model
fallback to handle both string and plain-object values, using the object’s
primary field when present before passing the result to
formatOpenclawSummaryValue. Preserve the existing agents.defaults model handling
and ensure object-form legacy configurations no longer render as [object
Object].
🪄 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: 787ea684-4609-4c8c-89ff-7b53f7111667
📒 Files selected for processing (6)
tests/unit/openclaw-core.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/modules/app.methods.openclaw-core.mjsweb-ui/partials/index/panel-config-openclaw.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/openclaw-structured.css
📜 Review details
🔇 Additional comments (12)
web-ui/modules/app.methods.openclaw-core.mjs (3)
227-237: LGTM!
239-250: LGTM!
540-561: LGTM!tests/unit/openclaw-core.test.mjs (1)
453-501: LGTM!tests/unit/web-ui-behavior-parity.test.mjs (1)
835-839: LGTM!web-ui/partials/index/panel-config-openclaw.html (2)
14-48: LGTM!
96-100: LGTM!web-ui/res/web-ui-render.precompiled.js (2)
2246-2288: LGTM!
2359-2371: LGTM!web-ui/styles/openclaw-structured.css (3)
366-496: LGTM!
674-690: LGTM!
711-719: LGTM!
| getOpenclawConfigSummary(config) { | ||
| const content = config && typeof config.content === 'string' ? config.content : ''; | ||
| if (!content.trim()) { | ||
| return []; | ||
| } | ||
| const parsed = this.parseOpenclawContent(content, { allowEmpty: true }); | ||
| if (!parsed.ok || !isPlainRecord(parsed.data)) { | ||
| return [ | ||
| { key: 'parse-error', label: '配置状态', value: '解析失败', tone: 'warning' } | ||
| ]; | ||
| } | ||
| const data = parsed.data; | ||
| const agentDefaults = readNestedValue(data, ['agents', 'defaults']); | ||
| const modelConfig = isPlainRecord(agentDefaults) ? agentDefaults.model : undefined; | ||
| const primaryModel = isPlainRecord(modelConfig) | ||
| ? modelConfig.primary | ||
| : (typeof modelConfig === 'string' ? modelConfig : readNestedValue(data, ['agent', 'model'])); | ||
| const fallbacks = isPlainRecord(modelConfig) && Array.isArray(modelConfig.fallbacks) | ||
| ? modelConfig.fallbacks | ||
| : []; | ||
| const workspace = isPlainRecord(agentDefaults) ? agentDefaults.workspace : ''; | ||
| const browser = isPlainRecord(data.browser) ? data.browser : {}; | ||
| const browserMode = browser.enabled === false | ||
| ? '关闭' | ||
| : (browser.headless === true ? 'Headless' : (browser.headless === false ? '可见窗口' : '未声明')); | ||
| const executablePath = typeof browser.executablePath === 'string' ? browser.executablePath.trim() : ''; | ||
| const providerNames = collectDistinctProviderKeys( | ||
| readNestedValue(data, ['models', 'providers']), | ||
| data.providers | ||
| ); | ||
| return [ | ||
| { key: 'primary', label: '默认模型', value: formatOpenclawSummaryValue(primaryModel) }, | ||
| { key: 'fallbacks', label: 'Fallback', value: formatOpenclawSummaryValue(fallbacks) }, | ||
| { key: 'workspace', label: 'Workspace', value: formatOpenclawSummaryValue(workspace) }, | ||
| { key: 'browser', label: 'Browser', value: browserMode }, | ||
| { key: 'browser-path', label: 'Chrome', value: formatOpenclawSummaryValue(executablePath) }, | ||
| { key: 'providers', label: 'Providers', value: providerNames.length ? providerNames.join(' / ') : '未配置' } | ||
| ]; | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Legacy agent.model object format produces [object Object] in summary.
When agents.defaults is absent and the config uses the legacy agent: { model: { primary: 'openai/gpt-4' } } format, line 516 falls back to readNestedValue(data, ['agent', 'model']) which returns the object directly. formatOpenclawSummaryValue then converts it to [object Object] via String(value). The existing fillOpenclawQuickFromConfig (lines 376-382) handles this case by checking for string vs object with .primary. The summary method should do the same.
🐛 Proposed fix for legacy model fallback
const agentDefaults = readNestedValue(data, ['agents', 'defaults']);
const modelConfig = isPlainRecord(agentDefaults) ? agentDefaults.model : undefined;
+ const legacyModel = readNestedValue(data, ['agent', 'model']);
+ const legacyPrimary = typeof legacyModel === 'string'
+ ? legacyModel
+ : (isPlainRecord(legacyModel) && typeof legacyModel.primary === 'string' ? legacyModel.primary : undefined);
const primaryModel = isPlainRecord(modelConfig)
? modelConfig.primary
- : (typeof modelConfig === 'string' ? modelConfig : readNestedValue(data, ['agent', 'model']));
+ : (typeof modelConfig === 'string' ? modelConfig : legacyPrimary);📝 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.
| getOpenclawConfigSummary(config) { | |
| const content = config && typeof config.content === 'string' ? config.content : ''; | |
| if (!content.trim()) { | |
| return []; | |
| } | |
| const parsed = this.parseOpenclawContent(content, { allowEmpty: true }); | |
| if (!parsed.ok || !isPlainRecord(parsed.data)) { | |
| return [ | |
| { key: 'parse-error', label: '配置状态', value: '解析失败', tone: 'warning' } | |
| ]; | |
| } | |
| const data = parsed.data; | |
| const agentDefaults = readNestedValue(data, ['agents', 'defaults']); | |
| const modelConfig = isPlainRecord(agentDefaults) ? agentDefaults.model : undefined; | |
| const primaryModel = isPlainRecord(modelConfig) | |
| ? modelConfig.primary | |
| : (typeof modelConfig === 'string' ? modelConfig : readNestedValue(data, ['agent', 'model'])); | |
| const fallbacks = isPlainRecord(modelConfig) && Array.isArray(modelConfig.fallbacks) | |
| ? modelConfig.fallbacks | |
| : []; | |
| const workspace = isPlainRecord(agentDefaults) ? agentDefaults.workspace : ''; | |
| const browser = isPlainRecord(data.browser) ? data.browser : {}; | |
| const browserMode = browser.enabled === false | |
| ? '关闭' | |
| : (browser.headless === true ? 'Headless' : (browser.headless === false ? '可见窗口' : '未声明')); | |
| const executablePath = typeof browser.executablePath === 'string' ? browser.executablePath.trim() : ''; | |
| const providerNames = collectDistinctProviderKeys( | |
| readNestedValue(data, ['models', 'providers']), | |
| data.providers | |
| ); | |
| return [ | |
| { key: 'primary', label: '默认模型', value: formatOpenclawSummaryValue(primaryModel) }, | |
| { key: 'fallbacks', label: 'Fallback', value: formatOpenclawSummaryValue(fallbacks) }, | |
| { key: 'workspace', label: 'Workspace', value: formatOpenclawSummaryValue(workspace) }, | |
| { key: 'browser', label: 'Browser', value: browserMode }, | |
| { key: 'browser-path', label: 'Chrome', value: formatOpenclawSummaryValue(executablePath) }, | |
| { key: 'providers', label: 'Providers', value: providerNames.length ? providerNames.join(' / ') : '未配置' } | |
| ]; | |
| }, | |
| getOpenclawConfigSummary(config) { | |
| const content = config && typeof config.content === 'string' ? config.content : ''; | |
| if (!content.trim()) { | |
| return []; | |
| } | |
| const parsed = this.parseOpenclawContent(content, { allowEmpty: true }); | |
| if (!parsed.ok || !isPlainRecord(parsed.data)) { | |
| return [ | |
| { key: 'parse-error', label: '配置状态', value: '解析失败', tone: 'warning' } | |
| ]; | |
| } | |
| const data = parsed.data; | |
| const agentDefaults = readNestedValue(data, ['agents', 'defaults']); | |
| const modelConfig = isPlainRecord(agentDefaults) ? agentDefaults.model : undefined; | |
| const legacyModel = readNestedValue(data, ['agent', 'model']); | |
| const legacyPrimary = typeof legacyModel === 'string' | |
| ? legacyModel | |
| : (isPlainRecord(legacyModel) && typeof legacyModel.primary === 'string' ? legacyModel.primary : undefined); | |
| const primaryModel = isPlainRecord(modelConfig) | |
| ? modelConfig.primary | |
| : (typeof modelConfig === 'string' ? modelConfig : legacyPrimary); | |
| const fallbacks = isPlainRecord(modelConfig) && Array.isArray(modelConfig.fallbacks) | |
| ? modelConfig.fallbacks | |
| : []; | |
| const workspace = isPlainRecord(agentDefaults) ? agentDefaults.workspace : ''; | |
| const browser = isPlainRecord(data.browser) ? data.browser : {}; | |
| const browserMode = browser.enabled === false | |
| ? '关闭' | |
| : (browser.headless === true ? 'Headless' : (browser.headless === false ? '可见窗口' : '未声明')); | |
| const executablePath = typeof browser.executablePath === 'string' ? browser.executablePath.trim() : ''; | |
| const providerNames = collectDistinctProviderKeys( | |
| readNestedValue(data, ['models', 'providers']), | |
| data.providers | |
| ); | |
| return [ | |
| { key: 'primary', label: '默认模型', value: formatOpenclawSummaryValue(primaryModel) }, | |
| { key: 'fallbacks', label: 'Fallback', value: formatOpenclawSummaryValue(fallbacks) }, | |
| { key: 'workspace', label: 'Workspace', value: formatOpenclawSummaryValue(workspace) }, | |
| { key: 'browser', label: 'Browser', value: browserMode }, | |
| { key: 'browser-path', label: 'Chrome', value: formatOpenclawSummaryValue(executablePath) }, | |
| { key: 'providers', label: 'Providers', value: providerNames.length ? providerNames.join(' / ') : '未配置' } | |
| ]; | |
| }, |
🤖 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 `@web-ui/modules/app.methods.openclaw-core.mjs` around lines 500 - 538, Update
getOpenclawConfigSummary’s legacy agent.model fallback to handle both string and
plain-object values, using the object’s primary field when present before
passing the result to formatOpenclawSummaryValue. Preserve the existing
agents.defaults model handling and ensure object-form legacy configurations no
longer render as [object Object].
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 `@web-ui/partials/index/modal-config-template-agents.html`:
- Around line 132-137: Update the agents content label conditional in the
template to use a translated `modal.agents.contentLabel.openclawWorkspace` key
with the workspace filename passed via the existing `{fileName}` interpolation
pattern, replacing the hardcoded Chinese text. Add the corresponding key and
interpolation text to every locale file, preserving the existing Claude and
default label branches.
In `@web-ui/res/web-ui-render.precompiled.js`:
- Around line 9240-9243: Replace the hardcoded “内容” suffix in the
openclaw-workspace branch of the agents content label with the localized
`modal.agents.contentLabel.openclawWorkspace` translation, preserving the
workspace filename fallback. Update the corresponding source template
`modal-config-template-agents.html`, regenerate the precompiled output, and
ensure the required key exists consistently in the en, ja, vi, zh-tw, and
Chinese locale files.
🪄 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: 0b144264-cc9c-4774-a75e-cb4fcf386fdd
📒 Files selected for processing (6)
web-ui/modules/app.methods.agents.mjsweb-ui/modules/i18n/locales/zh.mjsweb-ui/partials/index/modal-config-template-agents.htmlweb-ui/partials/index/panel-config-openclaw.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/modals-core.css
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/partials/index/panel-config-openclaw.html
📜 Review details
🔇 Additional comments (5)
web-ui/res/web-ui-render.precompiled.js (1)
2256-2269: LGTM!Also applies to: 2359-2371, 7226-7227, 7338-7339, 9228-9231, 9341-9341
web-ui/partials/index/modal-config-template-agents.html (1)
120-121: LGTM!web-ui/modules/app.methods.agents.mjs (1)
209-215: LGTM!web-ui/modules/i18n/locales/zh.mjs (1)
518-520: LGTM!Also applies to: 945-947, 1517-1521
web-ui/styles/modals-core.css (1)
64-142: LGTM!
| <div class="form-group agents-content-group"> | ||
| <label class="form-label"> | ||
| {{ agentsContext === 'openclaw-workspace' | ||
| ? `${agentsWorkspaceFileName || '工作区文件'} 内容` | ||
| : t(agentsContext === 'claude-project' ? 'modal.agents.contentLabel.claudeProject' : 'modal.agents.contentLabel') }} | ||
| </label> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target template around the cited lines
sed -n '120,150p' web-ui/partials/index/modal-config-template-agents.html
# Find all references to the relevant context and translation keys
rg -n "openclaw-workspace|claude-project|modal\.agents\.contentLabel" web-ui -S
# Locate locale files that define modal.agents.contentLabel keys
fd -a -e mjs -e js -e json -g '*zh*' web-ui
fd -a -e mjs -e js -e json -g '*en*' web-uiRepository: SakuraByteCore/codexmate
Length of output: 13071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect i18n implementation for parameter interpolation support
fd -a -e mjs -e js web-ui/modules/i18n
rg -n "function t\\(|const t =|translate\\(|\\{\\s*fileName\\s*\\}|interpolat|replace\\(/\\$\\{|\\{fileName\\}" web-ui/modules/i18n web-ui/modules -S
# Show the relevant locale sections around modal.agents.contentLabel keys
sed -n '530,555p' web-ui/modules/i18n/locales/en.mjs
sed -n '530,555p' web-ui/modules/i18n/locales/zh.mjs
sed -n '535,560p' web-ui/modules/i18n/locales/zh-tw.mjs
sed -n '530,555p' web-ui/modules/i18n/locales/ja.mjs
sed -n '745,770p' web-ui/modules/i18n/locales/vi.mjs
# Look for existing translation keys that use fileName interpolation or similar placeholders
rg -n "fileName|\\{.*\\}" web-ui/modules/i18n/locales -SRepository: SakuraByteCore/codexmate
Length of output: 544
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the i18n module files
fd . web-ui/modules/i18n
# Inspect the translation helper implementation
ast-grep outline web-ui/modules/i18n --view expanded || true
rg -n "function t\\(|const t =|translate\\(|interpolat|replace\\(/\\$\\{|\\{fileName\\}" web-ui/modules/i18n -S
# Show locale blocks around the existing keys
sed -n '535,555p' web-ui/modules/i18n/locales/en.mjs
sed -n '535,555p' web-ui/modules/i18n/locales/zh.mjs
sed -n '935,950p' web-ui/modules/i18n/locales/zh.mjs
sed -n '935,950p' web-ui/modules/i18n/locales/en.mjs
sed -n '935,950p' web-ui/modules/i18n/locales/zh-tw.mjs
# Search for placeholder-style translations already in use
rg -n "\\{[A-Za-z0-9_]+\\}" web-ui/modules/i18n/locales -SRepository: SakuraByteCore/codexmate
Length of output: 50380
Localize the openclaw-workspace content label
In web-ui/partials/index/modal-config-template-agents.html, this branch still hardcodes ${agentsWorkspaceFileName || '工作区文件'} 内容 while the other branches use t(). Add a modal.agents.contentLabel.openclawWorkspace key and route this label through the locale files so non-Chinese locales can translate it, matching the existing {fileName} pattern used for the workspace title/hint.
🤖 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 `@web-ui/partials/index/modal-config-template-agents.html` around lines 132 -
137, Update the agents content label conditional in the template to use a
translated `modal.agents.contentLabel.openclawWorkspace` key with the workspace
filename passed via the existing `{fileName}` interpolation pattern, replacing
the hardcoded Chinese text. Add the corresponding key and interpolation text to
every locale file, preserving the existing Claude and default label branches.
| _createElementVNode("div", { class: "form-group agents-content-group" }, [ | ||
| _createElementVNode("label", { class: "form-label" }, _toDisplayString(_ctx.agentsContext === 'openclaw-workspace' | ||
| ? `${_ctx.agentsWorkspaceFileName || '工作区文件'} 内容` | ||
| : _ctx.t(_ctx.agentsContext === 'claude-project' ? 'modal.agents.contentLabel.claudeProject' : 'modal.agents.contentLabel')), 1 /* TEXT */), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hardcoded Chinese "内容" bypasses i18n for openclaw-workspace content label.
The openclaw-workspace branch uses a template literal ${agentsWorkspaceFileName || '工作区文件'} 内容 while the claude-project and default branches use t(). Non-Chinese locales cannot localize the "内容" suffix. This is inconsistent with setAgentsModalContext which uses tr() with translation keys for title and hint in the same context.
This is flagged on the precompiled output; the source template in modal-config-template-agents.html lines 132-137 has the same issue.
Verify whether other locale files (en, ja, vi, zh-tw) define a modal.agents.contentLabel.openclawWorkspace key:
#!/bin/bash
# Check if any locale file defines the openclawWorkspace content label key
rg -n 'modal\.agents\.contentLabel\.openclawWorkspace' --type=js web-ui/modules/i18n/locales/🤖 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 `@web-ui/res/web-ui-render.precompiled.js` around lines 9240 - 9243, Replace
the hardcoded “内容” suffix in the openclaw-workspace branch of the agents content
label with the localized `modal.agents.contentLabel.openclawWorkspace`
translation, preserving the workspace filename fallback. Update the
corresponding source template `modal-config-template-agents.html`, regenerate
the precompiled output, and ensure the required key exists consistently in the
en, ja, vi, zh-tw, and Chinese locale files.
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 (2)
web-ui/partials/index/panel-config-openclaw.html (1)
14-30: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winHardcoded Chinese strings bypass i18n in the OpenClaw summary card.
Lines 14, 19, 21, and 30 contain hardcoded Chinese text instead of using
t()translation calls, breaking localization for non-Chinese locales. This is inconsistent with the rest of the template which usest()throughout (e.g., lines 79, 95). Thearia-labelon line 14 is particularly impactful — screen readers in non-Chinese locales will read untranslated Chinese text.🌐 Proposed fix: use t() for all hardcoded strings
-<section class="settings-card settings-card--wide openclaw-summary-card" aria-label="OpenClaw 当前摘要"> +<section class="settings-card settings-card--wide openclaw-summary-card" :aria-label="t('openclaw.summary.ariaLabel')"> <div class="settings-card-body"> <div class="openclaw-summary-header"> <div> <div class="summary-eyebrow">OPENCLAW</div> - <div class="summary-title">配置与本地诊断</div> + <div class="summary-title">{{ t('openclaw.summary.title') }}</div> </div> - <div class="summary-note">配置摘要</div> + <div class="summary-note">{{ t('openclaw.summary.note') }}</div> </div> <div class="openclaw-summary-grid"> <div v-for="item in getOpenclawStatusSummaryItems()" :key="item.key" :class="['openclaw-summary-item', item.tone || '']"> <div class="summary-item-label">{{ item.label }}</div> <div class="summary-item-value">{{ item.value }}</div> </div> </div> <div class="openclaw-diagnostic-note"> - 本页仅管理配置与工作区文件。 + {{ t('openclaw.summary.diagnostic') }} </div>🤖 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 `@web-ui/partials/index/panel-config-openclaw.html` around lines 14 - 30, Replace the hardcoded Chinese strings in the OpenClaw summary card—its section aria-label, summary title, summary note, and diagnostic note—with the existing t() translation helper, adding or reusing appropriate translation keys while preserving the current markup and displayed meaning.web-ui/res/web-ui-render.precompiled.js (1)
9227-9230: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLocalize the
openclaw-workspacelabel
Theopenclaw-workspacebranch still appends hardcoded内容; route that branch through at()key so the label stays consistent with the other cases.🤖 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 `@web-ui/res/web-ui-render.precompiled.js` around lines 9227 - 9230, The label rendering in the agents-content-group branch should localize the openclaw-workspace case instead of appending the hardcoded 内容 text. Update the agentsContext conditional to use the existing t() translation mechanism with an appropriate workspace-content key, while preserving the workspace filename fallback and the existing claude-project and default label branches.
🤖 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 `@web-ui/partials/index/panel-config-openclaw.html`:
- Around line 42-59: Replace the hardcoded “Workspace file” labels with the
localized openclaw.workspace.file key. Update
web-ui/partials/index/panel-config-openclaw.html lines 42-59 to use t() in both
locations, add the key to all locale files, and regenerate or synchronously
update web-ui/res/web-ui-render.precompiled.js lines 2283 and 2306 to use
_ctx.t(...) with the required text patch flag.
- Line 42: Replace the hardcoded “Workspace file” labels in the template and its
precompiled render function with the appropriate t() localization keys,
restoring t('openclaw.agents.hint') for the agent metadata and using the
matching localized key for quick workspace file buttons. Update both occurrences
consistently, including the loading/non-loading expression at the agent metadata
element.
---
Outside diff comments:
In `@web-ui/partials/index/panel-config-openclaw.html`:
- Around line 14-30: Replace the hardcoded Chinese strings in the OpenClaw
summary card—its section aria-label, summary title, summary note, and diagnostic
note—with the existing t() translation helper, adding or reusing appropriate
translation keys while preserving the current markup and displayed meaning.
In `@web-ui/res/web-ui-render.precompiled.js`:
- Around line 9227-9230: The label rendering in the agents-content-group branch
should localize the openclaw-workspace case instead of appending the hardcoded
内容 text. Update the agentsContext conditional to use the existing t()
translation mechanism with an appropriate workspace-content key, while
preserving the workspace filename fallback and the existing claude-project and
default label branches.
🪄 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: 5a87021a-f9c1-46eb-9217-196f8d924fa8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonweb-ui/modules/app.methods.openclaw-core.mjsweb-ui/partials/index/panel-config-openclaw.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/openclaw-structured.css
🚧 Files skipped from review as they are similar to previous changes (2)
- web-ui/modules/app.methods.openclaw-core.mjs
- web-ui/styles/openclaw-structured.css
📜 Review details
🔇 Additional comments (1)
package.json (1)
3-3: LGTM!
| <div class="tool-content"> | ||
| <div class="tool-title">AGENTS.md</div> | ||
| <div class="tool-meta">{{ agentsLoading ? t('config.modelLoading') : t('openclaw.agents.hint') }}</div> | ||
| <div class="tool-meta">{{ agentsLoading ? t('config.modelLoading') : 'Workspace file' }}</div> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Hardcoded "Workspace file" bypasses i18n — line 42 is a regression from t('openclaw.agents.hint').
Line 42 replaces the previously localized t('openclaw.agents.hint') with a hardcoded 'Workspace file' string, and line 59 uses static Workspace file text for quick workspace file buttons. Both should use t() for consistency with the rest of the template. The same hardcoded strings appear in the precompiled render function at lines 2283 and 2306 — see consolidated comment.
🌐 Proposed fix
- <div class="tool-meta">{{ agentsLoading ? t('config.modelLoading') : 'Workspace file' }}</div>
+ <div class="tool-meta">{{ agentsLoading ? t('config.modelLoading') : t('openclaw.workspace.file') }}</div>
</div>
<svg class="tool-chevron" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2">
<path d="M9 18l6-6-6-6"/>
</svg>
</button>
<button
v-for="fileName in getOpenclawQuickWorkspaceFiles()"
:key="fileName"
type="button"
class="openclaw-tool-btn"
`@click`="openOpenclawQuickWorkspaceFile(fileName)"
:disabled="loading || !!initError || agentsLoading">
<div class="tool-icon">📄</div>
<div class="tool-content">
<div class="tool-title">{{ fileName }}</div>
- <div class="tool-meta">Workspace file</div>
+ <div class="tool-meta">{{ t('openclaw.workspace.file') }}</div>Also applies to: 59-59
🤖 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 `@web-ui/partials/index/panel-config-openclaw.html` at line 42, Replace the
hardcoded “Workspace file” labels in the template and its precompiled render
function with the appropriate t() localization keys, restoring
t('openclaw.agents.hint') for the agent metadata and using the matching
localized key for quick workspace file buttons. Update both occurrences
consistently, including the loading/non-loading expression at the agent metadata
element.
| <div class="tool-meta">{{ agentsLoading ? t('config.modelLoading') : 'Workspace file' }}</div> | ||
| </div> | ||
| <svg class="tool-chevron" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"> | ||
| <path d="M9 18l6-6-6-6"/> | ||
| </svg> | ||
| </button> | ||
|
|
||
| <div class="openclaw-workspace-card"> | ||
| <label class="workspace-label" for="openclaw-workspace-file">{{ t('openclaw.workspaceFile') }}</label> | ||
| <div class="workspace-input-group"> | ||
| <input | ||
| id="openclaw-workspace-file" | ||
| class="form-input" | ||
| v-model="openclawWorkspaceFileName" | ||
| :placeholder="t('openclaw.workspace.placeholder')"> | ||
| <button class="btn-tool" @click="openOpenclawWorkspaceEditor" :disabled="loading || !!initError || agentsLoading"> | ||
| {{ agentsLoading ? t('config.modelLoading') : t('openclaw.workspace.open') }} | ||
| </button> | ||
| <button | ||
| v-for="fileName in getOpenclawQuickWorkspaceFiles()" | ||
| :key="fileName" | ||
| type="button" | ||
| class="openclaw-tool-btn" | ||
| @click="openOpenclawQuickWorkspaceFile(fileName)" | ||
| :disabled="loading || !!initError || agentsLoading"> | ||
| <div class="tool-icon">📄</div> | ||
| <div class="tool-content"> | ||
| <div class="tool-title">{{ fileName }}</div> | ||
| <div class="tool-meta">Workspace file</div> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Hardcoded "Workspace file" bypasses i18n across template and precompiled render. The shared root cause is a hardcoded English string that should use t() for localization — line 42 in the template is a regression from the previously localized t('openclaw.agents.hint').
web-ui/partials/index/panel-config-openclaw.html#L42-L59: Replace'Workspace file'on line 42 witht('openclaw.workspace.file')and the staticWorkspace fileon line 59 with{{ t('openclaw.workspace.file') }}. Add theopenclaw.workspace.filekey to all locale files.web-ui/res/web-ui-render.precompiled.js#L2283-L2283: Replace'Workspace file'in the ternary with_ctx.t('openclaw.workspace.file')(or regenerate from the fixed template).web-ui/res/web-ui-render.precompiled.js#L2306-L2306: Replace the static"Workspace file"node with_toDisplayString(_ctx.t('openclaw.workspace.file'))and add the1 /* TEXT */patch flag (or regenerate from the fixed template).
📍 Affects 2 files
web-ui/partials/index/panel-config-openclaw.html#L42-L59(this comment)web-ui/res/web-ui-render.precompiled.js#L2283-L2283web-ui/res/web-ui-render.precompiled.js#L2306-L2306
🤖 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 `@web-ui/partials/index/panel-config-openclaw.html` around lines 42 - 59,
Replace the hardcoded “Workspace file” labels with the localized
openclaw.workspace.file key. Update
web-ui/partials/index/panel-config-openclaw.html lines 42-59 to use t() in both
locations, add the key to all locale files, and regenerate or synchronously
update web-ui/res/web-ui-render.precompiled.js lines 2283 and 2306 to use
_ctx.t(...) with the required text patch flag.
2 participants
Summary
Working./v1/responseslifecycle events by emittingresponse.in_progressbefore completion.Changes
cli/openai-bridge.jswriteHead(500)calls when response headers/body state has already advanced.cli/openai-bridge-retry.js408,409,425,429, and all upstream5xxstatuses as transient retryable statuses.cli/openai-bridge-runtime.jsresponse.in_progressto converted streaming Responses SSE output.web-ui/*tests/unit/*Validation
node --check cli/openai-bridge.jsnode --check cli/openai-bridge-runtime.jsnode --check cli/openai-bridge-retry.jsnode --check web-ui/modules/app.methods.providers.mjsnode --check web-ui/app.jsnode --test tests/unit/openai-bridge-upstream-responses.test.mjs— 33/33 passnode tests/unit/run.mjs providers-validation provider-switch-regression provider-default-names web-ui-source-bundle— reportsAll 751 tests passednpm run test:unit -- openclaw-core— reportsAll 753 tests passednpm run lint— reportsLint passed for 233 file(s)git diff --checkNotes
search_sessions.py --limit -1diagnostic after passing; exit code is 0 for the scoped runs.Summary by CodeRabbit