fix(bridge): raise idle timeout and cache probe#160
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds OpenAI bridge fixes: StringDecoder-based SSE UTF-8 handling, configurable stream/non-stream timeouts, and per-baseUrl caching to skip repeated unsupported /v1/responses probes; expands exported helpers. Separately adds a new local bridge HTTP handler with CLI/UI integration, config/bootstrap defaults, settings, and tests. ChangesOpenAI Bridge Streaming and Caching
Local Bridge: handler, CLI, config, and UI
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bridge
participant Cache
participant UpstreamSSE as Upstream SSE
participant StringDecoder
participant BridgedSSE as Bridged SSE
Client->>Bridge: POST /v1/responses (request 1)
Bridge->>Cache: check unsupported baseUrl
Cache-->>Bridge: not cached
Bridge->>UpstreamSSE: probe /v1/responses
UpstreamSSE-->>Bridge: 404 unknown_endpoint
Bridge->>Cache: mark baseUrl unsupported
Bridge->>UpstreamSSE: POST /chat/completions
UpstreamSSE->>StringDecoder: write() chunk with partial UTF-8
UpstreamSSE->>StringDecoder: write() chunk with rest of UTF-8
StringDecoder->>BridgedSSE: end() with complete character
BridgedSSE-->>Client: SSE with correct multibyte text
Client->>Bridge: POST /v1/responses (request 2)
Bridge->>Cache: check unsupported baseUrl
Cache-->>Bridge: cached as unsupported
Bridge->>UpstreamSSE: POST /chat/completions (skip probe)
UpstreamSSE-->>BridgedSSE: SSE stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/openai-bridge.js (1)
756-778: ⚡ Quick winConsider deduplicating with
shouldFallbackFromUpstreamResponses.
isResponsesEndpointUnsupportedis essentiallyshouldFallbackFromUpstreamResponsesminus the twoname is a required propertytool-format checks (lines 738 and 750). The intent (endpoint-level vs per-request tool error) is correct, but the two predicates will inevitably drift as new upstream gateways are encountered, and any new endpoint-unsupported signal will need to be added in two places. Consider expressing the relationship explicitly, e.g.:♻️ Suggested refactor
+function isResponsesToolFormatError(bodyText) { + const text = String(bodyText || ''); + if (!text) return false; + const isToolNameMissing = (s) => /name['"`]?\s+is a required property/i.test(s) && /tools/i.test(s) && /function/i.test(s); + if (isToolNameMissing(text)) return true; + try { + const parsed = JSON.parse(text); + const msg = parsed && parsed.error && typeof parsed.error.message === 'string' ? parsed.error.message : ''; + if (isToolNameMissing(msg)) return true; + } catch (_) {} + return false; +} + +function shouldFallbackFromUpstreamResponses(status, bodyText) { + return isResponsesEndpointUnsupported(status, bodyText) || isResponsesToolFormatError(bodyText); +}This also makes the cache-marking site at lines 1548–1550 read more naturally: "fallback was triggered, and it was an endpoint-level signal".
🤖 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 756 - 778, The two predicates diverge riskily; refactor by extracting the shared detection logic into a single helper (e.g., matchUpstreamResponsesError(textOrParsed)) and then implement shouldFallbackFromUpstreamResponses and isResponsesEndpointUnsupported by calling that helper: have shouldFallbackFromUpstreamResponses include the additional "tool format" checks (the `name is a required property` checks) while isResponsesEndpointUnsupported calls the same helper but omits those extra tool-format predicates (or call shouldFallbackFromUpstreamResponses with a flag like includeToolChecks=false). Update references to isResponsesEndpointUnsupported and shouldFallbackFromUpstreamResponses to use the common matcher so future signals are added in one place.
🤖 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.js`:
- Around line 756-778: The two predicates diverge riskily; refactor by
extracting the shared detection logic into a single helper (e.g.,
matchUpstreamResponsesError(textOrParsed)) and then implement
shouldFallbackFromUpstreamResponses and isResponsesEndpointUnsupported by
calling that helper: have shouldFallbackFromUpstreamResponses include the
additional "tool format" checks (the `name is a required property` checks) while
isResponsesEndpointUnsupported calls the same helper but omits those extra
tool-format predicates (or call shouldFallbackFromUpstreamResponses with a flag
like includeToolChecks=false). Update references to
isResponsesEndpointUnsupported and shouldFallbackFromUpstreamResponses to use
the common matcher so future signals are added in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 063a7772-7b03-452e-afca-74e82aa7ae87
📒 Files selected for processing (2)
cli/openai-bridge.jstests/unit/openai-bridge-upstream-responses.test.mjs
📜 Review details
🔇 Additional comments (7)
cli/openai-bridge.js (5)
4-4: LGTM!Also applies to: 10-13
1298-1317: LGTM!
1140-1140: LGTM!Also applies to: 1168-1168, 1179-1180
1498-1516: LGTM!Also applies to: 1542-1552
1635-1652: LGTM!tests/unit/openai-bridge-upstream-responses.test.mjs (2)
1020-1084: LGTM!
1086-1142: LGTM!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/modules/app.methods.providers.mjs (1)
73-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReserved-name validation message is stale for
local.Validation now rejects both
codexmate-proxyandlocal, but the message only namescodexmate-proxy, which is misleading.💡 Suggested fix
- } else if (isReservedProviderCreationNameInput(name)) { - errors.name = 'codexmate-proxy 为保留名称，不可手动添加'; + } else if (isReservedProviderCreationNameInput(name)) { + errors.name = 'local / codexmate-proxy 为保留名称，不可手动添加';🤖 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 73 - 75, The reserved-name validation message in the branch guarded by isReservedProviderCreationNameInput(name) is stale (it only mentions "codexmate-proxy"); update the errors.name assignment in that branch (where name and vm.providersList are used and before the findProviderByName check) to mention both reserved names (e.g. "codexmate-proxy 和 local 为保留名称，不可手动添加") or otherwise list the actual reserved names returned by isReservedProviderCreationNameInput to keep the message accurate.
🧹 Nitpick comments (5)
cli/local-bridge.js (2)
85-97: ⚡ Quick winCircuit-breaker reset always falls back to
pool[0].When every upstream's circuit is open,
circuitState.clear()is followed byreturn { entry: pool[0], idx: 0 }. This means the same upstream is always retried first after a global cooldown — entries further in the pool only get traffic ifpool[0]succeeds first, which is the opposite of fair recovery and biases all retries onto one provider. Consider usingrrIndexafter the reset (or picking the entry with the oldestopenUntil).🔧 Suggested change
- // all circuits open, reset and retry first - circuitState.clear(); - return { entry: pool[0], idx: 0 }; + // all circuits open, reset and continue round-robin from current index + circuitState.clear(); + const idx = rrIndex++ % pool.length; + return { entry: pool[idx], idx };🤖 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/local-bridge.js` around lines 85 - 97, The pickUpstream function currently resets circuits with circuitState.clear() then always returns pool[0], biasing retries; change the reset branch to pick a fair starting index (e.g., use rrIndex to select idx = rrIndex++ % pool.length and return { entry: pool[idx], idx }) or choose the entry with the oldest openUntil from the previously-stored states so the retry is rotated instead of always using pool[0]; update references to rrIndex and circuitState in pickUpstream accordingly.
27-52: 💤 Low value
openaiBridgeFileparameter is unused here.
buildUpstreamPoolacceptsopenaiBridgeFilebut never references it — the only consumer of that path isresolveUpstreamAuth. Drop the parameter to avoid a stale signature, or document why it's threaded through.🤖 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/local-bridge.js` around lines 27 - 52, The function buildUpstreamPool currently declares an unused parameter openaiBridgeFile; remove that parameter from buildUpstreamPool's signature and update all call sites that pass openaiBridgeFile to call buildUpstreamPool(readConfigFn, excludedProviders) (or the two-arg form) so the signature is not stale, or alternatively if the value is actually needed, thread it into the function and use it where resolveUpstreamAuth expects it—prefer removing the unused openaiBridgeFile parameter from buildUpstreamPool and adjusting callers to match; keep references to buildUpstreamPool, resolveUpstreamAuth and openaiBridgeFile to locate the changes.cli/config-bootstrap.js (3)
33-33: 💤 Low value
BUILTIN_LOCAL_PROVIDER_NAMEis validated but never referenced.Lines 33/62 destructure and assert this dependency, yet the template at lines 123 and 128-129 hardcodes the string
"local". Either consume the constant in the template (so the dependency is meaningful) or drop the dependency wiring to avoid a misleading contract.🔧 Example: use the constant
-model_provider = "local" +model_provider = "${BUILTIN_LOCAL_PROVIDER_NAME}" model = "${defaultModel}" ... -[model_providers.local] -name = "local" +[model_providers.${BUILTIN_LOCAL_PROVIDER_NAME}] +name = "${BUILTIN_LOCAL_PROVIDER_NAME}"(and the matching regex in
ensureLocalProviderSection)Also applies to: 62-62, 123-123, 128-129
🤖 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/config-bootstrap.js` at line 33, The code currently validates and destructures BUILTIN_LOCAL_PROVIDER_NAME but the template uses the hardcoded "local"; either use the constant or remove the unused dependency. Update the template generation to reference BUILTIN_LOCAL_PROVIDER_NAME instead of the string "local" (and update the regex/validation inside ensureLocalProviderSection to match BUILTIN_LOCAL_PROVIDER_NAME), or if you prefer to keep the literal, remove BUILTIN_LOCAL_PROVIDER_NAME from the destructuring/validation so it isn’t asserted but unused.
173-186: ⚖️ Poor tradeoffText-based TOML append is fragile if the trailing section uses multi-line constructs.
fs.appendFileSyncblindly appends[model_providers.local]to the end ofconfig.toml. If the existing file ends inside a multi-line array, inline table, or a[[…]]table, the new section will be parsed as part of the preceding scope and silently break the config (or worse, attachname = "local"to the wrong table). Consider parsing/rewriting viatoml.stringifyinstead of string append, or at minimum verify the last non-blank line is not inside an open structure before appending.🤖 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/config-bootstrap.js` around lines 173 - 186, The current ensureLocalProviderSection function appends localSection to CONFIG_FILE using fs.appendFileSync which can corrupt TOML if the file ends inside a multi-line construct; instead load and parse the file (use a TOML parser), check for an existing model_providers.local table, add or merge the local table into the parsed object, then serialize back with toml.stringify and overwrite CONFIG_FILE; if you prefer a minimal change, in ensureLocalProviderSection read content, verify the file does not end inside an open structure by scanning for unclosed quotes/brackets/braces or an unfinished multiline array/table before calling fs.appendFileSync, and still check /\[model_providers\.local\]/ and use the localSection constant when safe.
118-138: ⚡ Quick winLocal provider TOML section is duplicated in two places.
The
[model_providers.local]block at lines 128-138 (insidebuildDefaultConfigContent) and lines 184 (insideensureLocalProviderSection) define the same provider with identical fields and the same hardcodedhttp://127.0.0.1:3737/bridge/local/v1. They will inevitably drift (e.g., timeouts, retries, base URL port). Extract a single source of truth.🔧 Suggested change
+ const LOCAL_PROVIDER_TOML = `[model_providers.local] +name = "local" +base_url = "http://127.0.0.1:3737/bridge/local/v1" +wire_api = "responses" +requires_openai_auth = true +preferred_auth_method = "codexmate" +codexmate_bridge = "local" +request_max_retries = 4 +stream_max_retries = 10 +stream_idle_timeout_ms = 300000 +`; + function buildDefaultConfigContent(initializedAt) { const defaultModel = DEFAULT_MODELS[0] || 'gpt-4'; return `${CODEXMATE_MANAGED_MARKER} # codexmate-initialized-at: ${initializedAt} model_provider = "local" model = "${defaultModel}" model_context_window = ${DEFAULT_MODEL_CONTEXT_WINDOW} model_auto_compact_token_limit = ${DEFAULT_MODEL_AUTO_COMPACT_TOKEN_LIMIT} -[model_providers.local] -name = "local" -base_url = "http://127.0.0.1:3737/bridge/local/v1" -wire_api = "responses" -requires_openai_auth = true -preferred_auth_method = "codexmate" -codexmate_bridge = "local" -request_max_retries = 4 -stream_max_retries = 10 -stream_idle_timeout_ms = 300000 -`; +${LOCAL_PROVIDER_TOML}`; } ... function ensureLocalProviderSection() { if (!fs.existsSync(CONFIG_FILE)) return; let content; try { content = fs.readFileSync(CONFIG_FILE, 'utf-8'); } catch (e) { return; } if (/\[model_providers\.local\]/.test(content)) return; - const localSection = `\n[model_providers.local]\nname = "local"\nbase_url = "http://127.0.0.1:3737/bridge/local/v1"\nwire_api = "responses"\nrequires_openai_auth = true\npreferred_auth_method = "codexmate"\ncodexmate_bridge = "local"\nrequest_max_retries = 4\nstream_max_retries = 10\nstream_idle_timeout_ms = 300000\n`; - fs.appendFileSync(CONFIG_FILE, localSection, 'utf-8'); + fs.appendFileSync(CONFIG_FILE, `\n${LOCAL_PROVIDER_TOML}`, 'utf-8'); }Also applies to: 173-186
🤖 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/config-bootstrap.js` around lines 118 - 138, The [model_providers.local] TOML block is duplicated between buildDefaultConfigContent and ensureLocalProviderSection; extract that block into a single source of truth (e.g., a constant or helper that returns the local provider TOML) and have both buildDefaultConfigContent and ensureLocalProviderSection reference it so fields like base_url, request_max_retries, stream_max_retries, stream_idle_timeout_ms, wire_api, requires_openai_auth and preferred_auth_method are defined only once; update usages to use the new LOCAL_PROVIDER_TOML (or getLocalProviderTOML) symbol and remove the hardcoded duplicate blocks, preserving the same formatting and CODEXMATE_MANAGED_MARKER behavior.
🤖 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 5491-5505: The code unconditionally writes
settings.lastActiveProvider (restoreProvider) into CONFIG_FILE, which can point
to a deleted provider; before replacing model_provider validate that
restoreProvider exists in the current provider list (e.g., parse CONFIG_FILE or
load the providers registry used by your app) and only perform the
content.replace/writeConfig if the provider is found; if not found, set
settings.enabled = false, call writeLocalBridgeSettings(settings), and return a
failure/notice (or choose a safe default provider) instead of writing an invalid
model_provider; reference restoreProvider, settings.lastActiveProvider,
CONFIG_FILE, model_provider, writeConfig, and writeLocalBridgeSettings when
making the check and early-return.
- Around line 5475-5505: The toggle code updates CONFIG_FILE and local bridge
settings but never updates AUTH_FILE, so authentication can remain tied to the
previous provider; modify the enable/disable branches in this block (the code
that reads/writes CONFIG_FILE, uses settings, writeLocalBridgeSettings,
writeConfig) to also refresh/sync auth credentials whenever the active provider
changes—mirror the behavior used by cmdSwitch() and applyConfigTemplate():
determine the new active provider (local or restoreProvider), then update
AUTH_FILE accordingly (either copy/replace the provider credentials or call the
existing auth-sync helper if one exists) before returning so requests use the
correct credential set.
In `@cli/local-bridge.js`:
- Around line 218-224: The error handler currently calls res.writeHead(500, ...)
unconditionally which will throw ERR_HTTP_HEADERS_SENT if an SSE response has
already started (e.g. after sendResponsesSse). Update the error paths (the outer
catch that writes 500 and any other places that call res.writeHead after
attempting SSE) to first check res.headersSent; if headers are already sent,
avoid calling writeHead or res.end with a status change and instead log the
error and, if possible, write an SSE-formatted error event to the stream;
otherwise call res.writeHead/res.end as before. Ensure changes reference the
sendResponsesSse call and the catch block that currently performs res.writeHead
so reviewers can find and update those spots.
- Around line 55-58: The inner ternary in the auth return is redundant: inspect
what you want for non-API-key tokens and either simplify to always wrap the
token (replace the whole ternary with a single token ? `Bearer ${token}` : ''
using reqAuthToken/token), or implement the intended distinction (e.g., return
`Bearer ${token}` when token.startsWith('sk-') and return the other prefix or
raw token for session tokens) — update the branch that currently mirrors the
API-key branch and adjust the return in the block guarded by entry.authMethod
and entry.requiresOpenaiAuth accordingly, referencing reqAuthToken, token,
token.startsWith and entry.authMethod/entry.requiresOpenaiAuth.
- Around line 135-137: The routing prefix check accepts versions like "v10"
because suffix.startsWith('v1') is too permissive; update the check in the
function that examines pathname/suffix so it only allows exact "v1" or "v1/..."
(e.g. replace the suffix.startsWith('v1') test with a stricter test such as
suffix === 'v1' || suffix.startsWith('v1/'), or use a regex /^\s*v1(?:\/|$)/ to
validate), and align the later strip/replace logic (currently using /^v1\/?/) to
the same exact-match pattern so requests like /bridge/local/v100/foo are
rejected rather than rewritten into /v1/00/foo.
- Around line 294-312: The passthrough handler currently discards the incoming
request body and hardcodes the response Content-Type; fix it by reading the
incoming body with readRequestBody(req, maxBodySize) and passing that
buffer/string as the body to proxyRequestJson (instead of body: null), preserve
the Authorization header as already done, and when sending the response use the
upstreamResult.headers['content-type'] (if present) as the Content-Type header
(fallback to application/json; charset=utf-8 only if upstreamResult.headers has
no content-type); update the logic around
joinApiUrl/upstreamResult/retryTransientRequest/proxyRequestJson to forward the
real request payload and propagate upstreamResult.headers into the res.writeHead
call, keeping recordFailure and recordSuccess behavior unchanged.
- Around line 198-228: The code is sending the raw string bodyResult.body into
proxyRequestJson causing double-stringification and forwarding stream=true;
change the POST body to pass the parsed object responsesRequest (or better,
normalize stream like { ...responsesRequest, stream: false }) instead of
bodyResult.body when calling proxyRequestJson so the upstream receives a proper
JSON object; update the call that currently uses body: bodyResult.body to use
body: responsesRequest or body: { ...responsesRequest, stream: false } and
mirror the toUpstreamNonStreamingResponsesPayload behavior from
openai-bridge.js.
In `@web-ui/modules/app.methods.providers.mjs`:
- Around line 503-518: The toggleLocalBridgeExcluded method currently swallows
errors from the api('local-bridge-set-excluded') call which can leave
this.localBridgeExcluded out of sync; update toggleLocalBridgeExcluded to (1)
optimistically compute next but only assign this.localBridgeExcluded = next
after a successful response from api('local-bridge-set-excluded'), (2) in the
catch block restore the previous localBridgeExcluded value (e.g., keep a copy of
the original array before mutating) and (3) surface the failure to the user
(e.g., call a UI notification/alert function or set an error state) instead of
silently ignoring errors so the checkbox and persisted server state remain
consistent.
In `@web-ui/partials/index/panel-config-codex.html`:
- Around line 324-325: The two hardcoded Chinese UI strings in the template (the
header "轮询池 — 勾选参与负载均衡的提供商" and the empty-state "暂无可用上游 provider，请先添加直连
provider") bypass i18n; replace them with calls to the translation function
(e.g., t('...')) in the template so they use the app's localization system,
ensuring the header element and the v-if empty-state div call t(...) with
appropriate new translation keys (for example panel.pollingPool.title and
panel.pollingPool.emptyState) and update your locale files with those keys and
translations; ensure you keep the existing localBridgeCandidateProviders() usage
unchanged.
---
Outside diff comments:
In `@web-ui/modules/app.methods.providers.mjs`:
- Around line 73-75: The reserved-name validation message in the branch guarded
by isReservedProviderCreationNameInput(name) is stale (it only mentions
"codexmate-proxy"); update the errors.name assignment in that branch (where name
and vm.providersList are used and before the findProviderByName check) to
mention both reserved names (e.g. "codexmate-proxy 和 local 为保留名称，不可手动添加") or
otherwise list the actual reserved names returned by
isReservedProviderCreationNameInput to keep the message accurate.
---
Nitpick comments:
In `@cli/config-bootstrap.js`:
- Line 33: The code currently validates and destructures
BUILTIN_LOCAL_PROVIDER_NAME but the template uses the hardcoded "local"; either
use the constant or remove the unused dependency. Update the template generation
to reference BUILTIN_LOCAL_PROVIDER_NAME instead of the string "local" (and
update the regex/validation inside ensureLocalProviderSection to match
BUILTIN_LOCAL_PROVIDER_NAME), or if you prefer to keep the literal, remove
BUILTIN_LOCAL_PROVIDER_NAME from the destructuring/validation so it isn’t
asserted but unused.
- Around line 173-186: The current ensureLocalProviderSection function appends
localSection to CONFIG_FILE using fs.appendFileSync which can corrupt TOML if
the file ends inside a multi-line construct; instead load and parse the file
(use a TOML parser), check for an existing model_providers.local table, add or
merge the local table into the parsed object, then serialize back with
toml.stringify and overwrite CONFIG_FILE; if you prefer a minimal change, in
ensureLocalProviderSection read content, verify the file does not end inside an
open structure by scanning for unclosed quotes/brackets/braces or an unfinished
multiline array/table before calling fs.appendFileSync, and still check
/\[model_providers\.local\]/ and use the localSection constant when safe.
- Around line 118-138: The [model_providers.local] TOML block is duplicated
between buildDefaultConfigContent and ensureLocalProviderSection; extract that
block into a single source of truth (e.g., a constant or helper that returns the
local provider TOML) and have both buildDefaultConfigContent and
ensureLocalProviderSection reference it so fields like base_url,
request_max_retries, stream_max_retries, stream_idle_timeout_ms, wire_api,
requires_openai_auth and preferred_auth_method are defined only once; update
usages to use the new LOCAL_PROVIDER_TOML (or getLocalProviderTOML) symbol and
remove the hardcoded duplicate blocks, preserving the same formatting and
CODEXMATE_MANAGED_MARKER behavior.
In `@cli/local-bridge.js`:
- Around line 85-97: The pickUpstream function currently resets circuits with
circuitState.clear() then always returns pool[0], biasing retries; change the
reset branch to pick a fair starting index (e.g., use rrIndex to select idx =
rrIndex++ % pool.length and return { entry: pool[idx], idx }) or choose the
entry with the oldest openUntil from the previously-stored states so the retry
is rotated instead of always using pool[0]; update references to rrIndex and
circuitState in pickUpstream accordingly.
- Around line 27-52: The function buildUpstreamPool currently declares an unused
parameter openaiBridgeFile; remove that parameter from buildUpstreamPool's
signature and update all call sites that pass openaiBridgeFile to call
buildUpstreamPool(readConfigFn, excludedProviders) (or the two-arg form) so the
signature is not stale, or alternatively if the value is actually needed, thread
it into the function and use it where resolveUpstreamAuth expects it—prefer
removing the unused openaiBridgeFile parameter from buildUpstreamPool and
adjusting callers to match; keep references to buildUpstreamPool,
resolveUpstreamAuth and openaiBridgeFile to locate the changes.
🪄 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: 9d580c6e-f769-4afc-a99a-309759535d79
📒 Files selected for processing (11)
cli.jscli/config-bootstrap.jscli/local-bridge.jstests/e2e/test-config.jstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/modules/app.computed.dashboard.mjsweb-ui/modules/app.methods.codex-config.mjsweb-ui/modules/app.methods.providers.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/partials/index/panel-config-codex.html
✅ Files skipped from review due to trivial changes (1)
- web-ui/app.js
Comment on lines
+5475
to
+5505
| if (enable) { | ||
| if (currentProvider === 'local') return { success: true, enabled: true, notice: '已启用 local 转换' }; | ||
| settings.lastActiveProvider = currentProvider; | ||
| settings.lastModel = currentModel; | ||
| settings.enabled = true; | ||
| writeLocalBridgeSettings(settings); | ||
| let content = fs.readFileSync(CONFIG_FILE, 'utf-8'); | ||
| content = content.replace(/^(model_provider\s*=\s*)(["']).*?(["'])/m, `$1$2local$3`); | ||
| writeConfig(content); | ||
| return { success: true, enabled: true, previousProvider: currentProvider }; | ||
| } else { | ||
| if (currentProvider !== 'local') { | ||
| settings.enabled = false; | ||
| writeLocalBridgeSettings(settings); | ||
| return { success: true, enabled: false, notice: 'local 转换未启用' }; | ||
| } | ||
| const restoreProvider = settings.lastActiveProvider || ''; | ||
| if (!restoreProvider) { | ||
| settings.enabled = false; | ||
| writeLocalBridgeSettings(settings); | ||
| return { success: true, enabled: false, notice: '已关闭 local 转换（无历史 provider 可恢复）' }; | ||
| } | ||
| let content = fs.readFileSync(CONFIG_FILE, 'utf-8'); | ||
| content = content.replace(/^(model_provider\s*=\s*)(["']).*?(["'])/m, `$1$2${restoreProvider}$3`); | ||
| if (settings.lastModel) { | ||
| content = content.replace(/^(model\s*=\s*)(["']).*?(["'])/m, `$1$2${settings.lastModel}$3`); | ||
| } | ||
| writeConfig(content); | ||
| settings.enabled = false; | ||
| writeLocalBridgeSettings(settings); | ||
| return { success: true, enabled: false, restoredProvider: restoreProvider, restoredModel: settings.lastModel }; |
Contributor
There was a problem hiding this comment.
Sync auth.json when the active provider changes.
This function flips model_provider, but unlike cmdSwitch() and applyConfigTemplate(), it never refreshes AUTH_FILE. After enabling local, requests can keep using the previous provider credential; after disabling it, they can keep using codexmate. The toggle looks successful in config.toml, but the next request can authenticate against the wrong target.
Suggested fix
if (enable) {
if (currentProvider === 'local') return { success: true, enabled: true, notice: '已启用 local 转换' };
settings.lastActiveProvider = currentProvider;
settings.lastModel = currentModel;
settings.enabled = true;
writeLocalBridgeSettings(settings);
let content = fs.readFileSync(CONFIG_FILE, 'utf-8');
content = content.replace(/^(model_provider\s*=\s*)(["']).*?(["'])/m, `$1$2local$3`);
writeConfig(content);
+ const localProvider = config.model_providers && config.model_providers[BUILTIN_LOCAL_PROVIDER_NAME];
+ updateAuthJson(localProvider && localProvider.preferred_auth_method ? localProvider.preferred_auth_method : '');
return { success: true, enabled: true, previousProvider: currentProvider };
} else {
if (currentProvider !== 'local') {
settings.enabled = false;
writeLocalBridgeSettings(settings);
return { success: true, enabled: false, notice: 'local 转换未启用' };
}
const restoreProvider = settings.lastActiveProvider || '';
+ const restoreConfig = config.model_providers && config.model_providers[restoreProvider];
if (!restoreProvider) {
settings.enabled = false;
writeLocalBridgeSettings(settings);
return { success: true, enabled: false, notice: '已关闭 local 转换（无历史 provider 可恢复）' };
}
let content = fs.readFileSync(CONFIG_FILE, 'utf-8');
content = content.replace(/^(model_provider\s*=\s*)(["']).*?(["'])/m, `$1$2${restoreProvider}$3`);
if (settings.lastModel) {
content = content.replace(/^(model\s*=\s*)(["']).*?(["'])/m, `$1$2${settings.lastModel}$3`);
}
writeConfig(content);
+ updateAuthJson(restoreConfig && restoreConfig.preferred_auth_method ? restoreConfig.preferred_auth_method : '');
settings.enabled = false;
writeLocalBridgeSettings(settings);
return { success: true, enabled: false, restoredProvider: restoreProvider, restoredModel: settings.lastModel };
}🤖 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 5475 - 5505, The toggle code updates CONFIG_FILE and
local bridge settings but never updates AUTH_FILE, so authentication can remain
tied to the previous provider; modify the enable/disable branches in this block
(the code that reads/writes CONFIG_FILE, uses settings,
writeLocalBridgeSettings, writeConfig) to also refresh/sync auth credentials
whenever the active provider changes—mirror the behavior used by cmdSwitch() and
applyConfigTemplate(): determine the new active provider (local or
restoreProvider), then update AUTH_FILE accordingly (either copy/replace the
provider credentials or call the existing auth-sync helper if one exists) before
returning so requests use the correct credential set.
Comment on lines
+5491
to
+5505
| const restoreProvider = settings.lastActiveProvider || ''; | ||
| if (!restoreProvider) { | ||
| settings.enabled = false; | ||
| writeLocalBridgeSettings(settings); | ||
| return { success: true, enabled: false, notice: '已关闭 local 转换（无历史 provider 可恢复）' }; | ||
| } | ||
| let content = fs.readFileSync(CONFIG_FILE, 'utf-8'); | ||
| content = content.replace(/^(model_provider\s*=\s*)(["']).*?(["'])/m, `$1$2${restoreProvider}$3`); | ||
| if (settings.lastModel) { | ||
| content = content.replace(/^(model\s*=\s*)(["']).*?(["'])/m, `$1$2${settings.lastModel}$3`); | ||
| } | ||
| writeConfig(content); | ||
| settings.enabled = false; | ||
| writeLocalBridgeSettings(settings); | ||
| return { success: true, enabled: false, restoredProvider: restoreProvider, restoredModel: settings.lastModel }; |
Contributor
There was a problem hiding this comment.
Guard against restoring a provider that no longer exists.
lastActiveProvider is persisted state, not a live config reference. If that provider was deleted or replaced while local stayed active, this branch writes an invalid model_provider and returns success, leaving the config pointed at a nonexistent provider.
Suggested fix
const restoreProvider = settings.lastActiveProvider || '';
- if (!restoreProvider) {
+ const restoreConfig = config.model_providers && config.model_providers[restoreProvider];
+ if (!restoreProvider || !restoreConfig) {
settings.enabled = false;
writeLocalBridgeSettings(settings);
- return { success: true, enabled: false, notice: '已关闭 local 转换（无历史 provider 可恢复）' };
+ return { success: true, enabled: false, notice: '已关闭 local 转换（历史 provider 不存在，未恢复）' };
}
let content = fs.readFileSync(CONFIG_FILE, 'utf-8');
content = content.replace(/^(model_provider\s*=\s*)(["']).*?(["'])/m, `$1$2${restoreProvider}$3`);📝 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.
Suggested change
| const restoreProvider = settings.lastActiveProvider || ''; | |
| if (!restoreProvider) { | |
| settings.enabled = false; | |
| writeLocalBridgeSettings(settings); | |
| return { success: true, enabled: false, notice: '已关闭 local 转换（无历史 provider 可恢复）' }; | |
| } | |
| let content = fs.readFileSync(CONFIG_FILE, 'utf-8'); | |
| content = content.replace(/^(model_provider\s*=\s*)(["']).*?(["'])/m, `$1$2${restoreProvider}$3`); | |
| if (settings.lastModel) { | |
| content = content.replace(/^(model\s*=\s*)(["']).*?(["'])/m, `$1$2${settings.lastModel}$3`); | |
| } | |
| writeConfig(content); | |
| settings.enabled = false; | |
| writeLocalBridgeSettings(settings); | |
| return { success: true, enabled: false, restoredProvider: restoreProvider, restoredModel: settings.lastModel }; | |
| const restoreProvider = settings.lastActiveProvider || ''; | |
| const restoreConfig = config.model_providers && config.model_providers[restoreProvider]; | |
| if (!restoreProvider || !restoreConfig) { | |
| settings.enabled = false; | |
| writeLocalBridgeSettings(settings); | |
| return { success: true, enabled: false, notice: '已关闭 local 转换（历史 provider 不存在，未恢复）' }; | |
| } | |
| let content = fs.readFileSync(CONFIG_FILE, 'utf-8'); | |
| content = content.replace(/^(model_provider\s*=\s*)(["']).*?(["'])/m, `$1$2${restoreProvider}$3`); | |
| if (settings.lastModel) { | |
| content = content.replace(/^(model\s*=\s*)(["']).*?(["'])/m, `$1$2${settings.lastModel}$3`); | |
| } | |
| writeConfig(content); | |
| settings.enabled = false; | |
| writeLocalBridgeSettings(settings); | |
| return { success: true, enabled: false, restoredProvider: restoreProvider, restoredModel: settings.lastModel }; |
🤖 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 5491 - 5505, The code unconditionally writes
settings.lastActiveProvider (restoreProvider) into CONFIG_FILE, which can point
to a deleted provider; before replacing model_provider validate that
restoreProvider exists in the current provider list (e.g., parse CONFIG_FILE or
load the providers registry used by your app) and only perform the
content.replace/writeConfig if the provider is found; if not found, set
settings.enabled = false, call writeLocalBridgeSettings(settings), and return a
failure/notice (or choose a safe default provider) instead of writing an invalid
model_provider; reference restoreProvider, settings.lastActiveProvider,
CONFIG_FILE, model_provider, writeConfig, and writeLocalBridgeSettings when
making the check and early-return.
Comment on lines
+55
to
+58
| if (entry.authMethod === 'codexmate' || entry.requiresOpenaiAuth) { | ||
| const token = reqAuthToken || ''; | ||
| return token ? (token.startsWith('sk-') ? `Bearer ${token}` : `Bearer ${token}`) : ''; | ||
| } |
Contributor
There was a problem hiding this comment.
Both ternary branches return the same value.
Line 57's inner ternary token.startsWith('sk-') ? \Bearer ${token}` : `Bearer ${token}`produces identical output regardless of the condition. This is either dead code from an incomplete refactor or a forgotten differentiation (e.g., session tokens vs. API keys should likely not both be wrapped withBearer `). Resolve the intent before merge.
🔧 If the intent was to always wrap with `Bearer`
function resolveUpstreamAuth(entry, openaiBridgeFile, reqAuthToken) {
if (entry.authMethod === 'codexmate' || entry.requiresOpenaiAuth) {
const token = reqAuthToken || '';
- return token ? (token.startsWith('sk-') ? `Bearer ${token}` : `Bearer ${token}`) : '';
+ return token ? `Bearer ${token}` : '';
}📝 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.
Suggested change
| if (entry.authMethod === 'codexmate' || entry.requiresOpenaiAuth) { | |
| const token = reqAuthToken || ''; | |
| return token ? (token.startsWith('sk-') ? `Bearer ${token}` : `Bearer ${token}`) : ''; | |
| } | |
| if (entry.authMethod === 'codexmate' || entry.requiresOpenaiAuth) { | |
| const token = reqAuthToken || ''; | |
| return token ? `Bearer ${token}` : ''; | |
| } |
🤖 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/local-bridge.js` around lines 55 - 58, The inner ternary in the auth
return is redundant: inspect what you want for non-API-key tokens and either
simplify to always wrap the token (replace the whole ternary with a single token
? `Bearer ${token}` : '' using reqAuthToken/token), or implement the intended
distinction (e.g., return `Bearer ${token}` when token.startsWith('sk-') and
return the other prefix or raw token for session tokens) — update the branch
that currently mirrors the API-key branch and adjust the return in the block
guarded by entry.authMethod and entry.requiresOpenaiAuth accordingly,
referencing reqAuthToken, token, token.startsWith and
entry.authMethod/entry.requiresOpenaiAuth.
Comment on lines
+135
to
+137
| if (!pathname.startsWith('/bridge/local/')) return false; | ||
| const suffix = pathname.replace(/^\/bridge\/local\/?/, ''); | ||
| if (!suffix.startsWith('v1')) return false; |
Contributor
There was a problem hiding this comment.
Routing prefix check is too permissive.
suffix.startsWith('v1') accepts v1, v10, v100, v1beta, etc. Combined with the strip regex /^v1\/?/ on line 171, a request to /bridge/local/v100/foo is accepted and proxied as <upstream>/v1/00/foo. Restrict to exact v1 or v1/....
🔧 Proposed fix
- if (!suffix.startsWith('v1')) return false;
+ if (suffix !== 'v1' && !suffix.startsWith('v1/')) return false;📝 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.
Suggested change
| if (!pathname.startsWith('/bridge/local/')) return false; | |
| const suffix = pathname.replace(/^\/bridge\/local\/?/, ''); | |
| if (!suffix.startsWith('v1')) return false; | |
| if (!pathname.startsWith('/bridge/local/')) return false; | |
| const suffix = pathname.replace(/^\/bridge\/local\/?/, ''); | |
| if (suffix !== 'v1' && !suffix.startsWith('v1/')) return 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/local-bridge.js` around lines 135 - 137, The routing prefix check accepts
versions like "v10" because suffix.startsWith('v1') is too permissive; update
the check in the function that examines pathname/suffix so it only allows exact
"v1" or "v1/..." (e.g. replace the suffix.startsWith('v1') test with a stricter
test such as suffix === 'v1' || suffix.startsWith('v1/'), or use a regex
/^\s*v1(?:\/|$)/ to validate), and align the later strip/replace logic
(currently using /^v1\/?/) to the same exact-match pattern so requests like
/bridge/local/v100/foo are rejected rather than rewritten into /v1/00/foo.
Comment on lines
+198
to
+228
| const responsesRequest = parsed.value; | ||
| const wantsSse = !!(responsesRequest && responsesRequest.stream); | ||
| const upstreamResponsesUrl = joinApiUrl(upstreamBase, 'responses'); | ||
| const upstreamResponsesResult = await retryTransientRequest(() => proxyRequestJson(upstreamResponsesUrl, { | ||
| method: 'POST', | ||
| body: bodyResult.body, | ||
| headers: { ...(authHeader ? { Authorization: authHeader } : {}) }, | ||
| maxBytes: maxUpstreamBytes, | ||
| httpAgent, | ||
| httpsAgent | ||
| })); | ||
|
|
||
| if (upstreamResponsesResult.ok && upstreamResponsesResult.status < 400) { | ||
| recordSuccess(entry.name); | ||
| const upstreamPayload = parseJsonOrError(upstreamResponsesResult.bodyText); | ||
| if (upstreamPayload.error) { | ||
| res.writeHead(502, { 'Content-Type': 'application/json; charset=utf-8' }); | ||
| res.end(JSON.stringify({ error: `Upstream parse failed: ${upstreamPayload.error}` })); | ||
| return; | ||
| } | ||
| if (wantsSse) { | ||
| res.writeHead(200, { 'Content-Type': 'text/event-stream; charset=utf-8', 'Cache-Control': 'no-cache', 'Connection': 'keep-alive', 'X-Accel-Buffering': 'no' }); | ||
| if (typeof res.flushHeaders === 'function') res.flushHeaders(); | ||
| sendResponsesSse(res, upstreamPayload.value); | ||
| res.end(); | ||
| return; | ||
| } | ||
| res.writeHead(200, { 'Content-Type': 'application/json; charset=utf-8' }); | ||
| res.end(JSON.stringify(ensureResponseMetadata(upstreamPayload.value))); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect proxyRequestJson to confirm it does not transparently collapse SSE into JSON,
# and check how openai-bridge.js handles the stream flag when calling /responses.
ast-grep --pattern $'function proxyRequestJson($_, $_) { $$$ }'
rg -nP -C3 '\bproxyRequestJson\s*\(' --type=js
rg -nP -C3 '\bstream\s*:\s*(true|false)\b' cli/openai-bridge.jsRepository: SakuraByteCore/codexmate
Length of output: 14557
🏁 Script executed:
# Check the context around bodyResult construction in local-bridge.js
sed -n '160,228p' cli/local-bridge.jsRepository: SakuraByteCore/codexmate
Length of output: 3962
🏁 Script executed:
# Find readRequestBody definition
rg -n "function readRequestBody|const readRequestBody" cli/local-bridge.js cli/openai-bridge.js cli/builtin-proxy.js
# Also check what bodyResult.body is used for elsewhere
rg -n "readRequestBody" cli/local-bridge.js -A5Repository: SakuraByteCore/codexmate
Length of output: 766
🏁 Script executed:
# Read the readRequestBody function from openai-bridge.js
sed -n '162,210p' cli/openai-bridge.jsRepository: SakuraByteCore/codexmate
Length of output: 1849
🏁 Script executed:
# Look at the chat/completions path in local-bridge.js
sed -n '230,280p' cli/local-bridge.jsRepository: SakuraByteCore/codexmate
Length of output: 3246
🏁 Script executed:
# Check how openai-bridge.js handles the responses endpoint
sed -n '1495,1550p' cli/openai-bridge.jsRepository: SakuraByteCore/codexmate
Length of output: 3242
Pass the parsed request object to proxyRequestJson, not the raw body string.
bodyResult.body is a string, and proxyRequestJson calls JSON.stringify() on it (line 1211 of openai-bridge.js), causing double-stringification. This sends malformed JSON upstream. Additionally, if the client sets stream: true, it will be forwarded unchanged. In openai-bridge.js, the equivalent call uses toUpstreamNonStreamingResponsesPayload(responsesRequest) to transform the request and explicitly set stream: false before sending upstream.
Pass responsesRequest instead of bodyResult.body at line 201:
Suggested fix
body: responsesRequest,
Or follow openai-bridge.js's pattern more closely and also normalize the stream flag:
body: { ...responsesRequest, stream: 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/local-bridge.js` around lines 198 - 228, The code is sending the raw
string bodyResult.body into proxyRequestJson causing double-stringification and
forwarding stream=true; change the POST body to pass the parsed object
responsesRequest (or better, normalize stream like { ...responsesRequest,
stream: false }) instead of bodyResult.body when calling proxyRequestJson so the
upstream receives a proper JSON object; update the call that currently uses
body: bodyResult.body to use body: responsesRequest or body: {
...responsesRequest, stream: false } and mirror the
toUpstreamNonStreamingResponsesPayload behavior from openai-bridge.js.
Comment on lines
+218
to
+224
| if (wantsSse) { | ||
| res.writeHead(200, { 'Content-Type': 'text/event-stream; charset=utf-8', 'Cache-Control': 'no-cache', 'Connection': 'keep-alive', 'X-Accel-Buffering': 'no' }); | ||
| if (typeof res.flushHeaders === 'function') res.flushHeaders(); | ||
| sendResponsesSse(res, upstreamPayload.value); | ||
| res.end(); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
writeHead after SSE has started will throw and mask the real error.
The outer catch on line 313 calls res.writeHead(500, ...) unconditionally. If sendResponsesSse (lines 221/285) has already flushed headers and written any bytes, writeHead throws ERR_HTTP_HEADERS_SENT, the client sees a half-finished SSE stream, and the original failure is lost. Guard the error response with res.headersSent.
🔧 Suggested change
} catch (e) {
- res.writeHead(500, { 'Content-Type': 'application/json; charset=utf-8' });
- res.end(JSON.stringify({ error: e && e.message ? e.message : 'Internal Error' }));
+ if (!res.headersSent) {
+ res.writeHead(500, { 'Content-Type': 'application/json; charset=utf-8' });
+ res.end(JSON.stringify({ error: e && e.message ? e.message : 'Internal Error' }));
+ } else {
+ try { res.end(); } catch (_) {}
+ }
}Also applies to: 282-288, 313-316
🤖 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/local-bridge.js` around lines 218 - 224, The error handler currently
calls res.writeHead(500, ...) unconditionally which will throw
ERR_HTTP_HEADERS_SENT if an SSE response has already started (e.g. after
sendResponsesSse). Update the error paths (the outer catch that writes 500 and
any other places that call res.writeHead after attempting SSE) to first check
res.headersSent; if headers are already sent, avoid calling writeHead or res.end
with a status change and instead log the error and, if possible, write an
SSE-formatted error event to the stream; otherwise call res.writeHead/res.end as
before. Ensure changes reference the sendResponsesSse call and the catch block
that currently performs res.writeHead so reviewers can find and update those
spots.
Comment on lines
+294
to
+312
| // passthrough for other v1/* paths | ||
| const upstreamUrl = joinApiUrl(upstreamBase, normalizedSuffix); | ||
| const upstreamResult = await retryTransientRequest(() => proxyRequestJson(upstreamUrl, { | ||
| method: req.method || 'GET', | ||
| body: null, | ||
| headers: { ...(authHeader ? { Authorization: authHeader } : {}) }, | ||
| maxBytes: maxUpstreamBytes, | ||
| httpAgent, | ||
| httpsAgent | ||
| })); | ||
| if (!upstreamResult.ok) { | ||
| recordFailure(entry.name); | ||
| res.writeHead(502, { 'Content-Type': 'application/json; charset=utf-8' }); | ||
| res.end(JSON.stringify({ error: `Upstream request failed: ${upstreamResult.error}` })); | ||
| return; | ||
| } | ||
| recordSuccess(entry.name); | ||
| res.writeHead(upstreamResult.status, { 'Content-Type': 'application/json; charset=utf-8' }); | ||
| res.end(upstreamResult.bodyText); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm proxyRequestJson exposes upstream headers (needed to propagate Content-Type)
ast-grep --pattern $'function proxyRequestJson($_, $_) { $$$ }'
rg -nP -C3 'bodyText|headers' cli/openai-bridge.js | head -120Repository: SakuraByteCore/codexmate
Length of output: 13776
🏁 Script executed:
# Read the context around lines 294-312 in cli/local-bridge.js
head -320 cli/local-bridge.js | tail -80 | cat -nRepository: SakuraByteCore/codexmate
Length of output: 5011
🏁 Script executed:
# Also get broader context to understand what endpoints are being handled
sed -n '250,330p' cli/local-bridge.js | cat -nRepository: SakuraByteCore/codexmate
Length of output: 4597
🏁 Script executed:
# Check if there are tests or references to this passthrough behavior
rg -n "passthrough|v1/\*" cli/local-bridge.js | head -20Repository: SakuraByteCore/codexmate
Length of output: 120
🏁 Script executed:
# Verify the structure: confirm this is indeed handling non-/responses v1/* paths
sed -n '200,320p' cli/local-bridge.js | cat -n | head -150Repository: SakuraByteCore/codexmate
Length of output: 7883
🏁 Script executed:
# Check if readRequestBody function exists (used in the sketch)
rg -n "readRequestBody|function.*Body" cli/local-bridge.js cli/openai-bridge.js | head -20Repository: SakuraByteCore/codexmate
Length of output: 438
🏁 Script executed:
# Check how the /responses endpoint handles body buffering (line 4 uses bodyResult.body)
sed -n '150,210p' cli/local-bridge.js | cat -nRepository: SakuraByteCore/codexmate
Length of output: 3695
Forward request body and propagate upstream Content-Type in passthrough handler.
The passthrough for non-/responses v1/* paths (lines 294–312) has two critical issues:
-
Request body is discarded:
body: null(line 298) loses the incoming request payload, causing POST/PUT/PATCH endpoints (e.g.,/embeddings,/audio/transcriptions,/files) to fail with 400 errors. UsereadRequestBody(req, maxBodySize)(already available and used for/responsesat line 186) to buffer and forward the actual request body. -
Response Content-Type is hardcoded: Line 311 unconditionally sets
Content-Type: application/json; charset=utf-8even when the upstream replied withtext/plain,text/event-stream, or binary content.proxyRequestJsonexposesupstreamResult.headers—propagate the upstreamContent-Typeheader instead.
🔧 Sketch of the changes
+ const method = (req.method || 'GET').toUpperCase();
+ const passBody = (method === 'GET' || method === 'HEAD')
+ ? null
+ : (await readRequestBody(req, maxBodySize)).body;
const upstreamUrl = joinApiUrl(upstreamBase, normalizedSuffix);
const upstreamResult = await retryTransientRequest(() => proxyRequestJson(upstreamUrl, {
- method: req.method || 'GET',
- body: null,
- headers: { ...(authHeader ? { Authorization: authHeader } : {}) },
+ method,
+ body: passBody,
+ headers: {
+ ...(authHeader ? { Authorization: authHeader } : {}),
+ ...(req.headers && req.headers['content-type'] ? { 'Content-Type': req.headers['content-type'] } : {})
+ },
maxBytes: maxUpstreamBytes,
httpAgent,
httpsAgent
}));
if (!upstreamResult.ok) {
recordFailure(entry.name);
res.writeHead(502, { 'Content-Type': 'application/json; charset=utf-8' });
res.end(JSON.stringify({ error: `Upstream request failed: ${upstreamResult.error}` }));
return;
}
recordSuccess(entry.name);
+ const upstreamCt = (upstreamResult.headers && upstreamResult.headers['content-type'])
+ || 'application/json; charset=utf-8';
- res.writeHead(upstreamResult.status, { 'Content-Type': 'application/json; charset=utf-8' });
+ res.writeHead(upstreamResult.status, { 'Content-Type': upstreamCt });
res.end(upstreamResult.bodyText);🤖 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/local-bridge.js` around lines 294 - 312, The passthrough handler
currently discards the incoming request body and hardcodes the response
Content-Type; fix it by reading the incoming body with readRequestBody(req,
maxBodySize) and passing that buffer/string as the body to proxyRequestJson
(instead of body: null), preserve the Authorization header as already done, and
when sending the response use the upstreamResult.headers['content-type'] (if
present) as the Content-Type header (fallback to application/json; charset=utf-8
only if upstreamResult.headers has no content-type); update the logic around
joinApiUrl/upstreamResult/retryTransientRequest/proxyRequestJson to forward the
real request payload and propagate upstreamResult.headers into the res.writeHead
call, keeping recordFailure and recordSuccess behavior unchanged.
Comment on lines
+503
to
+518
| async toggleLocalBridgeExcluded(providerName) { | ||
| const name = String(providerName || '').trim(); | ||
| if (!name) return; | ||
| const idx = this.localBridgeExcluded.indexOf(name); | ||
| const next = [...this.localBridgeExcluded]; | ||
| if (idx >= 0) { | ||
| next.splice(idx, 1); | ||
| } else { | ||
| next.push(name); | ||
| } | ||
| try { | ||
| const res = await api('local-bridge-set-excluded', { names: next }); | ||
| if (res && !res.error) { | ||
| this.localBridgeExcluded = next; | ||
| } | ||
| } catch (e) { /* ignore */ } |
Contributor
There was a problem hiding this comment.
Do not silently swallow local-bridge exclusion save failures.
If local-bridge-set-excluded fails, the current flow ignores it. That can leave perceived checkbox state out of sync with persisted server state and gives no user feedback.
💡 Suggested fix
async toggleLocalBridgeExcluded(providerName) {
const name = String(providerName || '').trim();
if (!name) return;
- const idx = this.localBridgeExcluded.indexOf(name);
- const next = [...this.localBridgeExcluded];
+ const current = Array.isArray(this.localBridgeExcluded) ? this.localBridgeExcluded : [];
+ const idx = current.indexOf(name);
+ const next = [...current];
if (idx >= 0) {
next.splice(idx, 1);
} else {
next.push(name);
}
try {
const res = await api('local-bridge-set-excluded', { names: next });
- if (res && !res.error) {
- this.localBridgeExcluded = next;
- }
- } catch (e) { /* ignore */ }
+ if (!res || res.error) {
+ throw new Error((res && res.error) || '保存失败');
+ }
+ this.localBridgeExcluded = next;
+ } catch (e) {
+ this.showMessage((e && e.message) || '轮询池保存失败', 'error');
+ await this.loadLocalBridgeExcluded();
+ }
},📝 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.
Suggested change
| async toggleLocalBridgeExcluded(providerName) { | |
| const name = String(providerName || '').trim(); | |
| if (!name) return; | |
| const idx = this.localBridgeExcluded.indexOf(name); | |
| const next = [...this.localBridgeExcluded]; | |
| if (idx >= 0) { | |
| next.splice(idx, 1); | |
| } else { | |
| next.push(name); | |
| } | |
| try { | |
| const res = await api('local-bridge-set-excluded', { names: next }); | |
| if (res && !res.error) { | |
| this.localBridgeExcluded = next; | |
| } | |
| } catch (e) { /* ignore */ } | |
| async toggleLocalBridgeExcluded(providerName) { | |
| const name = String(providerName || '').trim(); | |
| if (!name) return; | |
| const current = Array.isArray(this.localBridgeExcluded) ? this.localBridgeExcluded : []; | |
| const idx = current.indexOf(name); | |
| const next = [...current]; | |
| if (idx >= 0) { | |
| next.splice(idx, 1); | |
| } else { | |
| next.push(name); | |
| } | |
| try { | |
| const res = await api('local-bridge-set-excluded', { names: next }); | |
| if (!res || res.error) { | |
| throw new Error((res && res.error) || '保存失败'); | |
| } | |
| this.localBridgeExcluded = next; | |
| } catch (e) { | |
| this.showMessage((e && e.message) || '轮询池保存失败', 'error'); | |
| await this.loadLocalBridgeExcluded(); | |
| } | |
| }, |
🤖 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 503 - 518, The
toggleLocalBridgeExcluded method currently swallows errors from the
api('local-bridge-set-excluded') call which can leave this.localBridgeExcluded
out of sync; update toggleLocalBridgeExcluded to (1) optimistically compute next
but only assign this.localBridgeExcluded = next after a successful response from
api('local-bridge-set-excluded'), (2) in the catch block restore the previous
localBridgeExcluded value (e.g., keep a copy of the original array before
mutating) and (3) surface the failure to the user (e.g., call a UI
notification/alert function or set an error state) instead of silently ignoring
errors so the checkbox and persisted server state remain consistent.
Comment on lines
+324
to
+325
| <div style="font-size:13px;font-weight:600;margin-bottom:8px;color:var(--text-secondary)">轮询池 — 勾选参与负载均衡的提供商</div> | ||
| <div v-if="localBridgeCandidateProviders().length === 0" style="font-size:12px;color:var(--text-muted)">暂无可用上游 provider，请先添加直连 provider</div> |
Contributor
There was a problem hiding this comment.
New local-bridge panel text bypasses i18n.
These new labels are hardcoded strings, while surrounding UI uses t(...). This will not localize for non-Chinese language modes.
💡 Suggested fix
- <div style="font-size:13px;font-weight:600;margin-bottom:8px;color:var(--text-secondary)">轮询池 — 勾选参与负载均衡的提供商</div>
- <div v-if="localBridgeCandidateProviders().length === 0" style="font-size:12px;color:var(--text-muted)">暂无可用上游 provider，请先添加直连 provider</div>
+ <div style="font-size:13px;font-weight:600;margin-bottom:8px;color:var(--text-secondary)">
+ {{ t('config.localBridge.poolTitle') }}
+ </div>
+ <div v-if="localBridgeCandidateProviders().length === 0" style="font-size:12px;color:var(--text-muted)">
+ {{ t('config.localBridge.empty') }}
+ </div>📝 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.
Suggested change
| <div style="font-size:13px;font-weight:600;margin-bottom:8px;color:var(--text-secondary)">轮询池 — 勾选参与负载均衡的提供商</div> | |
| <div v-if="localBridgeCandidateProviders().length === 0" style="font-size:12px;color:var(--text-muted)">暂无可用上游 provider，请先添加直连 provider</div> | |
| <div style="font-size:13px;font-weight:600;margin-bottom:8px;color:var(--text-secondary)"> | |
| {{ t('config.localBridge.poolTitle') }} | |
| </div> | |
| <div v-if="localBridgeCandidateProviders().length === 0" style="font-size:12px;color:var(--text-muted)"> | |
| {{ t('config.localBridge.empty') }} | |
| </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-codex.html` around lines 324 - 325, The
two hardcoded Chinese UI strings in the template (the header "轮询池 —
勾选参与负载均衡的提供商" and the empty-state "暂无可用上游 provider，请先添加直连 provider") bypass
i18n; replace them with calls to the translation function (e.g., t('...')) in
the template so they use the app's localization system, ensuring the header
element and the v-if empty-state div call t(...) with appropriate new
translation keys (for example panel.pollingPool.title and
panel.pollingPool.emptyState) and update your locale files with those keys and
translations; ensure you keep the existing localBridgeCandidateProviders() usage
unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
response.failedwhile upstream is still thinking; matches codexstream_idle_timeout_ms = 300000.StringDecoderin the SSE upstream parser to keep multibyte UTF-8 deltas intact across chunk boundaries.localbridge that delegates to a round-robin pool of the user's own direct providers with a 3-failure circuit breaker; reservelocalas a non-deletable provider name; default the bootstrapped Codex config to it.Tests
node --test tests/unit/openai-bridge-upstream-responses.test.mjs(16 pass, includes 2 new)npm run test:unit(531 pass)npm run test:e2e(exit 0)