feat: add Claude proxy target APIs with Ollama support#171
Open
awsl233777 wants to merge 16 commits into
Open
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 10 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (32)
📝 WalkthroughWalkthroughAdds a canonical Claude config field ChangesClaude Proxy Dual-Mode Routing with targetApi
Estimated code review effort:
🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/unit/web-ui-logic.test.mjs (1)
84-87: ⚡ Quick winCover canonical
chat_completionsin normalization test.This test validates two aliases but not the canonical value itself; adding it will better lock the API contract.
Proposed test addition
test('normalizeClaudeConfig accepts chat completions target api aliases', () => { + assert.strictEqual(normalizeClaudeConfig({ targetApi: 'chat_completions' }).targetApi, 'chat_completions'); assert.strictEqual(normalizeClaudeConfig({ targetApi: 'chat/completions' }).targetApi, 'chat_completions'); assert.strictEqual(normalizeClaudeConfig({ targetApi: 'chat-completions' }).targetApi, 'chat_completions'); });🤖 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/web-ui-logic.test.mjs` around lines 84 - 87, Add an assertion to the existing test that covers the canonical value: call normalizeClaudeConfig with targetApi: 'chat_completions' and assert its targetApi remains 'chat_completions' so the test verifies aliases plus the canonical form; update the test block in tests/unit/web-ui-logic.test.mjs around the normalizeClaudeConfig assertions to include this additional assertion referencing normalizeClaudeConfig and targetApi.tests/unit/claude-settings-sync.test.mjs (1)
480-513: ⚡ Quick winAdd a preservation case for non-default
targetApiduring merge.Current test only validates default
responses; it doesn’t catch regressions where an existingchat_completionsvalue is unintentionally reset on edit.Proposed additional assertion
test('mergeClaudeConfig preserves externalCredentialType across edits without api key', () => { @@ assert.deepStrictEqual(merged, { apiKey: '', baseUrl: 'https://api.anthropic.com/', model: 'claude-3-7-sonnet', hasKey: true, externalCredentialType: 'auth-token', targetApi: 'responses' }); + + const mergedChat = mergeClaudeConfig.call(context, { + apiKey: '', + baseUrl: 'https://api.anthropic.com', + model: 'claude-3-7-sonnet', + hasKey: true, + externalCredentialType: 'auth-token', + targetApi: 'chat_completions' + }, { + model: 'claude-sonnet-4-6' + }); + assert.strictEqual(mergedChat.targetApi, 'chat_completions'); });🤖 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/claude-settings-sync.test.mjs` around lines 480 - 513, The test currently only verifies that mergeClaudeConfig preserves defaults (targetApi='responses') and misses regressions where an existing non-default targetApi (e.g., 'chat_completions') is lost on merge; update the test for mergeClaudeConfig in tests/unit/claude-settings-sync.test.mjs to include an input/initial config with targetApi set to a non-default value (such as 'chat_completions') and assert that the merged result retains that explicit targetApi rather than being reset to 'responses'; use the same context.normalizeClaudeConfig and the mergeClaudeConfig function reference to craft the call and expected assertion so the test will fail if mergeClaudeConfig incorrectly overwrites non-default targetApi.tests/e2e/test-claude-proxy.js (1)
335-342: ⚡ Quick winUse predicate-based request matching instead of positional indexes.
Index-based checks can become flaky if an extra upstream chat call is introduced (retry, preflight, future hook). Matching by payload shape keeps this test stable.
Proposed assertion hardening
const upstreamChatMessages = upstream.requests.filter((item) => item.path === '/v1/chat/completions'); assert(upstreamChatMessages.length >= 2, 'claude proxy should hit upstream /v1/chat/completions'); -assert(upstreamChatMessages[0].headers.authorization === 'Bearer sk-claude-upstream', 'claude proxy chat should use provider auth for upstream'); -assert(upstreamChatMessages[0].body.messages[0].role === 'system', 'claude proxy chat should map system prompt to system message'); -assert(upstreamChatMessages[0].body.max_tokens === 128, 'claude proxy chat should map max_tokens to max_tokens'); -assert(upstreamChatMessages[0].body.stream === false, 'claude proxy chat should synthesize Anthropic streaming locally'); -assert(upstreamChatMessages[1].body.tool_choice.function.name === 'lookup', 'claude proxy chat should map tool_choice'); +const plainChatReq = upstreamChatMessages.find((item) => + item.body && item.body.model === 'DeepSeek-V4-pro' && !item.body.tools +); +const toolChatReq = upstreamChatMessages.find((item) => + item.body && Array.isArray(item.body.tools) && item.body.tools.length > 0 +); +assert(plainChatReq && plainChatReq.headers.authorization === 'Bearer sk-claude-upstream', 'claude proxy chat should use provider auth for upstream'); +assert(plainChatReq.body.messages[0].role === 'system', 'claude proxy chat should map system prompt to system message'); +assert(plainChatReq.body.max_tokens === 128, 'claude proxy chat should map max_tokens to max_tokens'); +assert(plainChatReq.body.stream === false, 'claude proxy chat should synthesize Anthropic streaming locally'); +assert(toolChatReq && toolChatReq.body.tool_choice.function.name === 'lookup', 'claude proxy chat should map tool_choice');🤖 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-claude-proxy.js` around lines 335 - 342, The tests are using positional indexes on upstreamChatMessages which is flaky; update the assertions to locate requests by predicate on their payload shape instead of array position: use upstream.requests.filter(...) or find(...) to match by headers.authorization === 'Bearer sk-claude-upstream' for the provider-auth check, match a request whose body.messages[0].role === 'system' and body.max_tokens === 128 and body.stream === false for the system/max_tokens/stream assertions, and match a request whose body.tool_choice?.function?.name === 'lookup' for the tool_choice assertion; replace the index-based assertions that reference upstreamChatMessages[0] and [1] with these predicate-found entries and assert they exist and have the expected properties.
🤖 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/modals-basic.html`:
- Around line 136-143: Replace the hardcoded Chinese label, option texts and
hint for the target API selector with i18n keys rendered through t(...);
specifically update the strings inside the select block bound to
newClaudeConfig.targetApi (the label "目标 API", the two option texts "Anthropic /
OpenAI Responses" and "OpenAI Chat Completions (/v1/chat/completions)", and the
form-hint copy) to use translation keys (e.g., t('claude.targetApi.label'),
t('claude.targetApi.option.responses'),
t('claude.targetApi.option.chat_completions'), t('claude.targetApi.hint')) and
ensure the same change is applied to the duplicated block around the other
selector (the block you modified at the later occurrence). Ensure keys exist in
the locale files.
In `@web-ui/partials/index/panel-config-claude.html`:
- Line 132: The subtitle text inside the div with class "card-subtitle" (the
element gated by v-if="config.targetApi === 'chat_completions'") is hardcoded;
replace it with a localized string by calling the translation helper (e.g.,
t('panel.mode.openai_chat_completions')) so it follows the app locale, and add
the corresponding translation key to the i18n resources; ensure the
component/context provides the t(...) function before using it.
In `@web-ui/res/web-ui-render.precompiled.js`:
- Around line 1890-1894: The hardcoded UI strings shown when checking
config.targetApi (e.g., the "OpenAI Chat Completions" subtitle rendered in the
_createElementBlock with class "card-subtitle", plus the field label, option
labels and hint around the same conditional branches at the other locations)
must be replaced with i18n keys and rendered via _ctx.t(...). Update the three
affected render sites to call _ctx.t('key_name') instead of raw text, add
appropriate i18n keys for the card subtitle, field label, option labels and hint
(use descriptive keys like "card.subtitle.openai_chat_completions",
"field.label.target_api", "option.label.x", "field.hint.target_api"), and ensure
the conditional branches still pick the right key based on config.targetApi so
translations are used for language switching.
---
Nitpick comments:
In `@tests/e2e/test-claude-proxy.js`:
- Around line 335-342: The tests are using positional indexes on
upstreamChatMessages which is flaky; update the assertions to locate requests by
predicate on their payload shape instead of array position: use
upstream.requests.filter(...) or find(...) to match by headers.authorization ===
'Bearer sk-claude-upstream' for the provider-auth check, match a request whose
body.messages[0].role === 'system' and body.max_tokens === 128 and body.stream
=== false for the system/max_tokens/stream assertions, and match a request whose
body.tool_choice?.function?.name === 'lookup' for the tool_choice assertion;
replace the index-based assertions that reference upstreamChatMessages[0] and
[1] with these predicate-found entries and assert they exist and have the
expected properties.
In `@tests/unit/claude-settings-sync.test.mjs`:
- Around line 480-513: The test currently only verifies that mergeClaudeConfig
preserves defaults (targetApi='responses') and misses regressions where an
existing non-default targetApi (e.g., 'chat_completions') is lost on merge;
update the test for mergeClaudeConfig in
tests/unit/claude-settings-sync.test.mjs to include an input/initial config with
targetApi set to a non-default value (such as 'chat_completions') and assert
that the merged result retains that explicit targetApi rather than being reset
to 'responses'; use the same context.normalizeClaudeConfig and the
mergeClaudeConfig function reference to craft the call and expected assertion so
the test will fail if mergeClaudeConfig incorrectly overwrites non-default
targetApi.
In `@tests/unit/web-ui-logic.test.mjs`:
- Around line 84-87: Add an assertion to the existing test that covers the
canonical value: call normalizeClaudeConfig with targetApi: 'chat_completions'
and assert its targetApi remains 'chat_completions' so the test verifies aliases
plus the canonical form; update the test block in
tests/unit/web-ui-logic.test.mjs around the normalizeClaudeConfig assertions to
include this additional assertion referencing normalizeClaudeConfig and
targetApi.
🪄 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: 6ae0fbd9-703c-4ef4-b64e-cefdd78b6682
📒 Files selected for processing (13)
cli.jscli/claude-proxy.jstests/e2e/test-claude-proxy.jstests/unit/claude-proxy-adapter.test.mjstests/unit/claude-settings-sync.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/app.jsweb-ui/logic.claude.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/res/web-ui-render.precompiled.js
📜 Review details
🔇 Additional comments (10)
cli.js (1)
293-294: LGTM!Also applies to: 5492-5494
cli/claude-proxy.js (2)
136-151: LGTM!Also applies to: 238-352, 432-505, 1268-1270
644-647: LGTM!Also applies to: 676-683, 757-784, 810-813, 1015-1015, 1041-1041, 1074-1077, 1080-1084, 1097-1099, 1198-1199, 1247-1247
web-ui/logic.claude.mjs (1)
72-75: LGTM!Also applies to: 82-83
web-ui/app.js (1)
278-279: LGTM!Also applies to: 284-285, 292-294, 539-542
web-ui/modules/app.methods.claude-config.mjs (1)
55-57: LGTM!Also applies to: 67-69, 88-88, 110-110, 186-186, 208-210
web-ui/modules/app.methods.startup-claude.mjs (1)
250-252: LGTM!tests/unit/claude-proxy-adapter.test.mjs (1)
7-10: LGTM!Also applies to: 67-105, 145-172
tests/unit/web-ui-logic.test.mjs (1)
54-55: LGTM!Also applies to: 66-80
tests/e2e/test-claude-proxy.js (1)
101-142: LGTM!Also applies to: 260-334, 343-394
Collaborator
Author
|
Updated the PR to close the Claude Chat Completions apply-path gap and the UI i18n review comment. Changes in this push:
Validation run locally:
@coderabbitai re-review |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli.js (1)
9176-9214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback proxy runtime on post-start failure paths.
If the proxy starts successfully but a later step fails (e.g., reading/writing Claude settings), the function returns an error while leaving the proxy running. That creates inconsistent state (
applyfailed, runtime still active).Proposed fix
async function applyToClaudeSettings(config = {}) { + let proxyStarted = false; try { @@ if (targetApi === 'chat_completions') { await stopBuiltinClaudeProxyRuntime(); proxyResult = await startBuiltinClaudeProxyRuntime({ @@ if (!proxyResult || proxyResult.error || proxyResult.success === false || !proxyResult.listenUrl) { + await stopBuiltinClaudeProxyRuntime(); return { success: false, mode: 'claude-proxy', error: (proxyResult && proxyResult.error) || '启动 Claude 兼容代理失败' }; } + proxyStarted = true; settingsBaseUrl = proxyResult.listenUrl; settingsApiKey = 'codexmate'; } else { @@ const readResult = readJsonObjectFromFile(CLAUDE_SETTINGS_FILE, {}); if (!readResult.ok) { + if (proxyStarted) { + await stopBuiltinClaudeProxyRuntime(); + } return { success: false, mode: 'settings-file', error: readResult.error }; } @@ } catch (e) { + if (proxyStarted) { + try { await stopBuiltinClaudeProxyRuntime(); } catch (_) { } + } return { success: false, mode: 'settings-file', error: e.message || '应用 Claude 配置失败' }; } }Also applies to: 9262-9267
🤖 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 9176 - 9214, The code may leave the built-in Claude proxy running if later steps fail; ensure we stop the proxy on all post-start failure paths by calling stopBuiltinClaudeProxyRuntime() before any early return after a successful start (i.e., when proxyResult indicates success and you later hit an error while handling CLAUDE_SETTINGS_FILE, readJsonObjectFromFile, or writeJsonAtomic). Specifically, after using startBuiltinClaudeProxyRuntime() and assigning settingsBaseUrl/settingsApiKey (and likewise in the branch that writes BUILTIN_CLAUDE_PROXY_SETTINGS_FILE), add logic to call stopBuiltinClaudeProxyRuntime() (or otherwise tear down the runtime) whenever you are about to return an error, so proxyResult/started runtimes are cleaned up 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.
Inline comments:
In `@cli.js`:
- Around line 9178-9197: The code currently sets a predictable proxy credential
(settingsApiKey = 'codexmate') after calling startBuiltinClaudeProxyRuntime,
which makes the proxy guessable if bound to a non-loopback address; replace this
by generating a cryptographically-secure random token for settingsApiKey (e.g.,
using crypto.randomBytes) and assign that token instead of the constant, and
additionally validate proxyResult.listenUrl (from
startBuiltinClaudeProxyRuntime) to ensure it is bound to a loopback address
(127.0.0.1/::1); if the listenUrl is non-loopback either refuse to use a
fixed/predictable key (fail or require explicit config) or log and rotate a
random token, ensuring settingsBaseUrl uses proxyResult.listenUrl and only the
random token is exposed as the API key.
---
Outside diff comments:
In `@cli.js`:
- Around line 9176-9214: The code may leave the built-in Claude proxy running if
later steps fail; ensure we stop the proxy on all post-start failure paths by
calling stopBuiltinClaudeProxyRuntime() before any early return after a
successful start (i.e., when proxyResult indicates success and you later hit an
error while handling CLAUDE_SETTINGS_FILE, readJsonObjectFromFile, or
writeJsonAtomic). Specifically, after using startBuiltinClaudeProxyRuntime() and
assigning settingsBaseUrl/settingsApiKey (and likewise in the branch that writes
BUILTIN_CLAUDE_PROXY_SETTINGS_FILE), add logic to call
stopBuiltinClaudeProxyRuntime() (or otherwise tear down the runtime) whenever
you are about to return an error, so proxyResult/started runtimes are cleaned up
consistently.
🪄 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: f1c4589c-b7ff-4435-a393-681c33a08def
📒 Files selected for processing (9)
cli.jscli/claude-proxy.jstests/e2e/test-claude.jstests/unit/web-ui-logic.test.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/res/web-ui-render.precompiled.js
✅ Files skipped from review due to trivial changes (1)
- web-ui/modules/i18n.dict.mjs
📜 Review details
🔇 Additional comments (22)
tests/e2e/test-claude.js (1)
67-91: LGTM!web-ui/partials/index/panel-config-claude.html (1)
132-132: Ensureclaude.targetApi.chatCompletionsBadgeis present in i18n resources.
claude.targetApi.chatCompletionsBadgeis defined inweb-ui/modules/i18n.dict.mjsfor all supported locale blocks (zh,ja,en), so thet('claude.targetApi.chatCompletionsBadge')call won’t fail due to a missing key.web-ui/res/web-ui-render.precompiled.js (1)
1894-1894: LGTM!Also applies to: 5561-5571, 5675-5685
tests/unit/web-ui-logic.test.mjs (1)
84-88: LGTM!web-ui/partials/index/modals-basic.html (1)
136-143: LGTM!Also applies to: 175-182
cli.js (1)
9154-9159: LGTM!Also applies to: 9399-9403, 11008-11008, 15556-15558
cli/claude-proxy.js (10)
136-151: LGTM!
238-293: LGTM!
295-352: LGTM!
432-505: LGTM!
816-840: LGTM!
1212-1212: LGTM!
1099-1111: LGTM!
526-617: LGTM!
681-684: LGTM!
757-785: LGTM!web-ui/modules/app.methods.claude-config.mjs (6)
50-59: LGTM!
61-72: LGTM!
85-89: LGTM!
110-127: LGTM!
186-199: LGTM!
202-211: LGTM!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli.js (1)
9176-9267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop the proxy again if the later settings write path fails.
After Line 9178 starts the runtime, any later failure in
readJsonObjectFromFile,backupFileIfNeededOnce, orwriteJsonAtomicreturns an error but never tears the proxy back down. That leaves a leaked background process and persisted proxy state that no longer matches~/.claude/settings.json.🧯 Suggested rollback
async function applyToClaudeSettings(config = {}) { try { const apiKey = (config.apiKey || '').trim(); if (!apiKey) { return { success: false, mode: 'settings-file', error: '请先输入 API Key' }; @@ const targetApi = normalizeClaudeTargetApi(config.targetApi); let settingsBaseUrl = baseUrl; let settingsApiKey = apiKey; let proxyResult = null; + let proxyStarted = false; @@ proxyResult = await startBuiltinClaudeProxyRuntime({ enabled: true, provider: typeof config.name === 'string' ? config.name.trim() : '', authSource: 'provider', targetApi, @@ if (!proxyResult || proxyResult.error || proxyResult.success === false || !proxyResult.listenUrl) { return { success: false, mode: 'claude-proxy', error: (proxyResult && proxyResult.error) || '启动 Claude 兼容代理失败' }; } + proxyStarted = true; settingsBaseUrl = proxyResult.listenUrl; settingsApiKey = 'codexmate'; @@ } catch (e) { + if (proxyStarted) { + try { + await stopBuiltinClaudeProxyRuntime(); + } catch (_) { } + } return { success: false, mode: 'settings-file', error: e.message || '应用 Claude 配置失败' };🤖 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 9176 - 9267, The code can leak a running proxy if startBuiltinClaudeProxyRuntime succeeds but later steps (readJsonObjectFromFile, backupFileIfNeededOnce, writeJsonAtomic) fail; modify the flow around startBuiltinClaudeProxyRuntime/stopBuiltinClaudeProxyRuntime so that when proxyResult indicates a proxy was started you always stop it on any early return or exception—either wrap the post-proxy logic in a try/finally that calls stopBuiltinClaudeProxyRuntime() in the finally, or track a boolean like proxyStarted and call stopBuiltinClaudeProxyRuntime() before each error return and in the catch block; ensure this covers the branches that return after readJsonObjectFromFile errors and after writeJsonAtomic/backup failures, and still preserve returning proxy info only when the operation completes successfully.
♻️ Duplicate comments (1)
cli.js (1)
9178-9196:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForce loopback before writing the fixed proxy token.
startBuiltinClaudeProxyRuntime(...)is called without a host override, so a previously persisted non-loopback bind can still be reused here. Line 9196 then writes a constantANTHROPIC_API_KEY, which makes the proxy credential guessable off-box if this runtime comes up anywhere other than loopback.🔒 Minimal hardening
proxyResult = await startBuiltinClaudeProxyRuntime({ enabled: true, + host: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.host, provider: typeof config.name === 'string' ? config.name.trim() : '', authSource: 'provider', targetApi, timeoutMs: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.timeoutMs, upstreamProviderName: typeof config.name === 'string' ? config.name.trim() : '', upstreamBaseUrl: baseUrl, upstreamApiKey: apiKey }); if (!proxyResult || proxyResult.error || proxyResult.success === false || !proxyResult.listenUrl) { return { success: false, mode: 'claude-proxy', error: (proxyResult && proxyResult.error) || '启动 Claude 兼容代理失败' }; } + { + const listenHost = new URL(proxyResult.listenUrl).hostname; + if (listenHost !== '127.0.0.1' && listenHost !== 'localhost' && listenHost !== '::1') { + await stopBuiltinClaudeProxyRuntime(); + return { + success: false, + mode: 'claude-proxy', + error: 'Claude 兼容代理在 chat_completions 模式下必须仅监听 loopback' + }; + } + } settingsBaseUrl = proxyResult.listenUrl; settingsApiKey = 'codexmate';🤖 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 9178 - 9196, The code currently starts the Claude proxy without forcing a loopback bind and then writes a fixed proxy API key ('codexmate'/ANTHROPIC_API_KEY), which can leak if the runtime binds non-loopback; update the call to startBuiltinClaudeProxyRuntime to force a loopback host (e.g., pass bindHost: '127.0.0.1' or equivalent) and only persist or expose a generated/ephemeral settingsApiKey when proxyResult.listenUrl is loopback; replace the constant 'codexmate' assignment to settingsApiKey with a securely generated random token (or skip persisting the token) when proxyResult.listenUrl is not loopback so the credential cannot be guessed off-box (references: startBuiltinClaudeProxyRuntime, settingsApiKey, ANTHROPIC_API_KEY).
🤖 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/claude-proxy.js`:
- Around line 769-776: resolveOpenaiBridgeUpstream may return null/undefined
causing a TypeError when accessing bridgeUpstream.baseUrl; update the logic
around resolveOpenaiBridgeUpstream (the bridgeUpstream variable) to explicitly
check for null/undefined and return an error if so, then only access
bridgeUpstream.baseUrl (used to set bridgeBaseUrl) after confirming
bridgeUpstream is an object and has a baseUrl property; keep existing
isValidHttpUrl(providerName) check and error messages using providerName
unchanged.
---
Outside diff comments:
In `@cli.js`:
- Around line 9176-9267: The code can leak a running proxy if
startBuiltinClaudeProxyRuntime succeeds but later steps (readJsonObjectFromFile,
backupFileIfNeededOnce, writeJsonAtomic) fail; modify the flow around
startBuiltinClaudeProxyRuntime/stopBuiltinClaudeProxyRuntime so that when
proxyResult indicates a proxy was started you always stop it on any early return
or exception—either wrap the post-proxy logic in a try/finally that calls
stopBuiltinClaudeProxyRuntime() in the finally, or track a boolean like
proxyStarted and call stopBuiltinClaudeProxyRuntime() before each error return
and in the catch block; ensure this covers the branches that return after
readJsonObjectFromFile errors and after writeJsonAtomic/backup failures, and
still preserve returning proxy info only when the operation completes
successfully.
---
Duplicate comments:
In `@cli.js`:
- Around line 9178-9196: The code currently starts the Claude proxy without
forcing a loopback bind and then writes a fixed proxy API key
('codexmate'/ANTHROPIC_API_KEY), which can leak if the runtime binds
non-loopback; update the call to startBuiltinClaudeProxyRuntime to force a
loopback host (e.g., pass bindHost: '127.0.0.1' or equivalent) and only persist
or expose a generated/ephemeral settingsApiKey when proxyResult.listenUrl is
loopback; replace the constant 'codexmate' assignment to settingsApiKey with a
securely generated random token (or skip persisting the token) when
proxyResult.listenUrl is not loopback so the credential cannot be guessed
off-box (references: startBuiltinClaudeProxyRuntime, settingsApiKey,
ANTHROPIC_API_KEY).
🪄 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: 7f3075cd-6ca8-427d-85f1-4f2a07df91b4
📒 Files selected for processing (15)
cli.jscli/claude-proxy.jstests/e2e/test-claude-proxy.jstests/e2e/test-claude.jstests/unit/claude-proxy-adapter.test.mjstests/unit/claude-settings-sync.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/app.jsweb-ui/logic.claude.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/res/web-ui-render.precompiled.js
📜 Review details
🔇 Additional comments (22)
web-ui/partials/index/panel-config-claude.html (1)
132-132: LGTM!web-ui/logic.claude.mjs (1)
72-75: LGTM!Also applies to: 82-83
web-ui/app.js (1)
278-278: LGTM!Also applies to: 284-284, 292-294, 539-542
web-ui/partials/index/modals-basic.html (1)
136-143: LGTM!Also applies to: 175-182
web-ui/modules/i18n.dict.mjs (1)
1051-1055: LGTM!Also applies to: 2112-2116, 3183-3187
tests/unit/claude-settings-sync.test.mjs (1)
490-492: LGTM!Also applies to: 511-513
tests/unit/claude-proxy-adapter.test.mjs (1)
7-7: LGTM!Also applies to: 9-9, 67-105, 145-172
tests/unit/web-ui-logic.test.mjs (1)
54-55: LGTM!Also applies to: 67-68, 79-80, 84-88
tests/e2e/test-claude.js (1)
67-84: LGTM!Also applies to: 90-92
tests/e2e/test-claude-proxy.js (1)
101-142: LGTM!Also applies to: 260-384, 392-394
web-ui/modules/app.methods.startup-claude.mjs (1)
250-252: LGTM!web-ui/res/web-ui-render.precompiled.js (1)
1890-1900: LGTM!Also applies to: 5560-5572, 5674-5686
web-ui/modules/app.methods.claude-config.mjs (1)
55-56: LGTM!Also applies to: 67-68, 88-88, 110-110, 112-112, 186-186, 188-188, 208-209
cli.js (1)
293-294: LGTM!Also applies to: 5492-5494, 9154-9159, 9399-9403, 11007-11008, 15556-15564
cli/claude-proxy.js (8)
136-151: LGTM!
238-293: LGTM!
295-352: LGTM!
432-505: LGTM!
644-695: LGTM!
816-839: LGTM!
1100-1125: LGTM!
1224-1224: LGTM!Also applies to: 1273-1273, 1294-1296
Collaborator
Author
|
Pushed a follow-up hardening commit for the latest CodeRabbit actionable comments: Changes:
Validation run locally after this commit:
@coderabbitai re-review |
Contributor
|
Triggering a full re-review now. 🐇✨ ✅ Actions performedFull review triggered. |
Collaborator
Author
|
Systematic test pass added for the Claude What changed in this push:
Validation run locally:
Additional real-upstream smoke:
@coderabbitai re-review |
Contributor
|
Triggering a full re-review now. 🐇✨ ✅ Actions performedFull review triggered. |
Contributor
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 (1)
cli.js (1)
9216-9251:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelay proxy shutdown until the settings write succeeds.
In the
responsesbranch, Lines 9217-9218 tear down the existing proxy beforeCLAUDE_SETTINGS_FILEis even read. If the later read/write fails, apply returns an error but the previously working proxy is already gone.Suggested fix
if (targetApi === 'chat_completions') { await stopBuiltinClaudeProxyRuntime(); const proxyToken = crypto.randomBytes(24).toString('hex'); proxyResult = await startBuiltinClaudeProxyRuntime({ enabled: true, host: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.host, provider: typeof config.name === 'string' ? config.name.trim() : '', authSource: 'provider', targetApi, timeoutMs: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.timeoutMs, upstreamProviderName: typeof config.name === 'string' ? config.name.trim() : '', upstreamBaseUrl: baseUrl, upstreamApiKey: apiKey }); if (!proxyResult || proxyResult.error || proxyResult.success === false || !proxyResult.listenUrl) { await stopBuiltinClaudeProxyRuntime(); resetBuiltinClaudeProxySavedSettingsToResponses(); return { success: false, mode: 'claude-proxy', error: (proxyResult && proxyResult.error) || '启动 Claude 兼容代理失败' }; } proxyStarted = true; settingsBaseUrl = proxyResult.listenUrl; settingsApiKey = proxyToken; - } else { - await stopBuiltinClaudeProxyRuntime(); - resetBuiltinClaudeProxySavedSettingsToResponses(); } @@ ensureDir(CLAUDE_DIR); const backupPath = backupFileIfNeededOnce(CLAUDE_SETTINGS_FILE); writeJsonAtomic(CLAUDE_SETTINGS_FILE, nextSettings); + if (targetApi !== 'chat_completions') { + await stopBuiltinClaudeProxyRuntime(); + resetBuiltinClaudeProxySavedSettingsToResponses(); + }
🤖 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-claude-proxy.js`:
- Around line 356-370: Wrap the block that mutates and tests the bridge settings
in a try...finally so the original file is always restored: move the
fs.writeFileSync(bridgeSettingsPath, JSON.stringify({ providers: {} }, ...)) and
the subsequent api('claude-proxy-start') / asserts into a try block and put
fs.writeFileSync(bridgeSettingsPath, savedBridgeSettings, 'utf-8') inside a
finally block; reference the bridgeSettingsPath and savedBridgeSettings
variables and ensure the api calls missingBridgeStartResult and
missingBridgeStatus remain inside the try so any thrown error still triggers the
finally restoration.
In `@tests/e2e/test-claude.js`:
- Around line 99-109: The test corrupts the Claude settings.json
(claudeSettingsPath) but only restores it at the end, so failures leave the file
corrupted; wrap the mutation and subsequent assertions (the call to
api('apply-claude-config') and the checks against failedChatApply and
claudeProxyStatusAfterFailedApply from api('claude-proxy-status')) in a
try/finally and always write back validClaudeSettings to claudeSettingsPath in
the finally block to guarantee restoration even if an assertion throws.
🪄 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: 5ad0bbee-40c7-483e-83ae-36c3bd62ac49
📒 Files selected for processing (4)
cli.jscli/claude-proxy.jstests/e2e/test-claude-proxy.jstests/e2e/test-claude.js
📜 Review details
🔇 Additional comments (3)
cli.js (2)
9161-9172: LGTM!
9192-9215: ⚡ Quick winRemove the “keep proxyToken and ANTHROPIC_API_KEY in sync” requirement
proxyTokenis only generated incli.jsand written toenv.ANTHROPIC_API_KEY, butstartBuiltinClaudeProxyRuntime(...)is called without it. The proxy’s incoming authentication is driven byprocess.env.CODEXMATE_HTTP_TOKENand checksAuthorization/x-codexmate-tokenheaders (notx-api-key/ANTHROPIC_API_KEY), so Claude’sANTHROPIC_API_KEYvalue doesn’t need to matchproxyTokenfor the proxy to accept requests.> Likely an incorrect or invalid review comment.cli/claude-proxy.js (1)
769-771: LGTM!
Contributor
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 (1)
web-ui/app.js (1)
534-543:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard malformed persisted Claude config entries before assigning
targetApi.On Line 540,
config.targetApi = ...assumesconfigis an object. Corrupted localStorage entries can throw here and break startup normalization.Suggested fix
for (const [name, config] of Object.entries(this.claudeConfigs)) { + if (!config || typeof config !== 'object' || Array.isArray(config)) { + delete this.claudeConfigs[name]; + continue; + } if (config.apiKey && config.apiKey.includes('****')) { config.apiKey = ''; config.hasKey = false; } const targetApiRaw = typeof config.targetApi === 'string' ? config.targetApi.trim().toLowerCase() : ''; config.targetApi = targetApiRaw === 'chat_completions' || targetApiRaw === 'chat-completions' || targetApiRaw === 'chat/completions' ? 'chat_completions' : 'responses'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/app.js` around lines 534 - 543, The loop over this.claudeConfigs assumes each entry is a valid object; guard against malformed persisted entries by checking that config is a non-null object before reading or writing properties like config.apiKey, config.hasKey and config.targetApi, and if an entry is invalid either skip normalization or replace it with a safe default object (e.g., { apiKey: '', hasKey: false, targetApi: 'responses' }) so the subsequent assignment to config.targetApi cannot throw.
🤖 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 9183-9203: The code allows targetApi === 'chat_completions' to
startBuiltinClaudeProxyRuntime even when config.baseUrl is empty (falling back
to the Anthropic Messages URL), which silently wires the proxy to the wrong
upstream; update the validation around normalizeClaudeTargetApi/targetApi and
baseUrl before calling startBuiltinClaudeProxyRuntime so that if targetApi ===
'chat_completions' and config.baseUrl is missing or equal to the default
Anthropic messages URL you either throw/return a validation error or call
startBuiltinClaudeProxyRuntime without upstreamBaseUrl (omit the upstreamBaseUrl
property) so the runtime resolves the upstream from config.name; adjust the
block that prepares proxyResult (the call to startBuiltinClaudeProxyRuntime and
the upstreamBaseUrl/upstreamProviderName fields) accordingly to enforce this
check.
- Around line 9192-9203: The generated proxyToken from
applyToClaudeSettings(...) is not passed into startBuiltinClaudeProxyRuntime,
causing an auth mismatch; update the call to startBuiltinClaudeProxyRuntime (the
invocation that currently sets host, provider, authSource, targetApi, timeoutMs,
upstreamProviderName, upstreamBaseUrl, upstreamApiKey) to include the generated
proxyToken (e.g., as an authToken or proxyToken parameter) so the runtime
enforces the same token that was written into Claude’s ANTHROPIC_API_KEY, and
also verify cli/claude-proxy.js checks that token (or falls back to
CODEXMATE_HTTP_TOKEN only when appropriate) for non-loopback requests when
DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.host is non-loopback. Ensure parameter
naming matches startBuiltinClaudeProxyRuntime’s signature and update any
doc/comment accordingly.
---
Outside diff comments:
In `@web-ui/app.js`:
- Around line 534-543: The loop over this.claudeConfigs assumes each entry is a
valid object; guard against malformed persisted entries by checking that config
is a non-null object before reading or writing properties like config.apiKey,
config.hasKey and config.targetApi, and if an entry is invalid either skip
normalization or replace it with a safe default object (e.g., { apiKey: '',
hasKey: false, targetApi: 'responses' }) so the subsequent assignment to
config.targetApi cannot throw.
🪄 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: 858ec6da-e7ea-42fc-b114-09db6512dd0d
📒 Files selected for processing (15)
cli.jscli/claude-proxy.jstests/e2e/test-claude-proxy.jstests/e2e/test-claude.jstests/unit/claude-proxy-adapter.test.mjstests/unit/claude-settings-sync.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/app.jsweb-ui/logic.claude.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/res/web-ui-render.precompiled.js
📜 Review details
🔇 Additional comments (9)
cli.js (1)
293-294: LGTM!Also applies to: 5492-5494, 9154-9172, 9417-9421, 11025-11026
tests/e2e/test-claude-proxy.js (1)
356-370: Always restore bridge settings withtry/finally.Line 358 mutates
codexmate-openai-bridge.json, but restore on Line 370 is not guaranteed if an assertion or API call fails first.tests/e2e/test-claude.js (1)
99-109: Wrap Claude settings corruption/restore intry/finally.Line 101 corrupts
settings.json, but restoration on Line 109 is skipped if intermediate assertions fail.web-ui/modules/app.methods.claude-config.mjs (1)
55-56: LGTM!Also applies to: 67-68, 88-88, 110-113, 186-189, 208-209
web-ui/modules/app.methods.startup-claude.mjs (1)
250-251: LGTM!web-ui/modules/i18n.dict.mjs (1)
1051-1055: LGTM!Also applies to: 2112-2116, 3183-3187
web-ui/partials/index/modals-basic.html (1)
136-143: LGTM!Also applies to: 175-182
web-ui/partials/index/panel-config-claude.html (1)
132-132: LGTM!web-ui/res/web-ui-render.precompiled.js (1)
1890-1900: LGTM!Also applies to: 5560-5572, 5674-5686
…letions-proxy # Conflicts: # cli.js # web-ui/modules/app.methods.claude-config.mjs # web-ui/modules/i18n.dict.mjs # web-ui/res/web-ui-render.precompiled.js
Collaborator
Author
|
Resolved the What changed in this push:
Local validation run on Node 22.22.1:
@coderabbitai re-review |
Contributor
|
Triggering a full re-review now. 🐇✨ ✅ Actions performedFull review triggered. |
Collaborator
Author
|
Minimized the PR diff with No runtime code changed in this follow-up. The effective PR diff is now 18 files, limited to the Claude Chat Completions proxy path, required Web UI/i18n integration, generated render output, and regression tests. Previous local validation still applies for the code-bearing merge resolution:
@coderabbitai re-review |
Contributor
|
✅ Actions performedFull review triggered. |
Collaborator
Author
|
Updated this PR with the full follow-up pass:
Validation actually run:
@coderabbitai review |
Contributor
|
The test count growth (533 → 583) and the real Ollama smoke test (including the ✅ Actions performedReview triggered.
|
Contributor
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.js (1)
15816-15829:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the MCP schema match the runtime's
ollamarules.
applyToClaudeSettings()allowstargetApi: 'ollama'without an API key, but this MCP tool still requiresapiKeyunconditionally. That blocks the new Ollama mode for MCP clients even though the backend supports it.Suggested fix
inputSchema: { - type: 'object', - properties: { - apiKey: { type: 'string' }, - baseUrl: { type: 'string' }, - model: { type: 'string' }, - name: { type: 'string' }, - targetApi: { type: 'string' } - }, - required: ['apiKey'], - additionalProperties: false + oneOf: [ + { + type: 'object', + properties: { + baseUrl: { type: 'string' }, + model: { type: 'string' }, + name: { type: 'string' }, + targetApi: { enum: ['ollama'] } + }, + additionalProperties: false + }, + { + type: 'object', + properties: { + apiKey: { type: 'string' }, + baseUrl: { type: 'string' }, + model: { type: 'string' }, + name: { type: 'string' }, + targetApi: { type: 'string' } + }, + required: ['apiKey'], + additionalProperties: 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.js` around lines 15816 - 15829, The MCP tool's inputSchema for action 'codexmate.claude.config.apply' currently requires apiKey unconditionally, which conflicts with applyToClaudeSettings() that allows targetApi: 'ollama' without an API key; update the schema so apiKey is optional by removing it from the top-level required array and add a JSON Schema conditional (if/then) that only requires apiKey when targetApi is not 'ollama' (or equivalently, require apiKey when targetApi equals the non-ollama value(s)); reference the inputSchema object for 'codexmate.claude.config.apply' and the applyToClaudeSettings() behavior when making this change.
♻️ Duplicate comments (1)
cli.js (1)
9360-9378:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
chat_completionshas no OpenAI-compatible upstream.
targetApi === 'chat_completions'still falls back to the Anthropic Messages base URL and passes it asupstreamBaseUrl. That silently starts the proxy against the wrong wire API instead of rejecting the config or letting runtime-side provider resolution take over.Suggested fix
- const baseUrl = (config.baseUrl || (targetApi === 'ollama' ? 'http://127.0.0.1:11434' : 'https://open.bigmodel.cn/api/anthropic')).trim(); + const configuredBaseUrl = typeof config.baseUrl === 'string' ? config.baseUrl.trim() : ''; + const baseUrl = (configuredBaseUrl || (targetApi === 'ollama' ? 'http://127.0.0.1:11434' : 'https://open.bigmodel.cn/api/anthropic')).trim(); const model = (config.model || DEFAULT_CLAUDE_MODEL).trim(); let settingsBaseUrl = baseUrl; let settingsApiKey = apiKey; let proxyResult = null; if (targetApi === 'chat_completions' || targetApi === 'ollama') { + if (targetApi === 'chat_completions' && !configuredBaseUrl && !(typeof config.name === 'string' && config.name.trim())) { + return { success: false, mode: 'claude-proxy', error: 'chat_completions 模式需要显式的上游 Base URL 或可解析的 provider 名称' }; + } await stopBuiltinClaudeProxyRuntime(); const proxyToken = crypto.randomBytes(24).toString('hex'); proxyResult = await startBuiltinClaudeProxyRuntime({ enabled: true, host: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.host, @@ - upstreamBaseUrl: baseUrl, + ...(configuredBaseUrl ? { upstreamBaseUrl: configuredBaseUrl } : {}), upstreamApiKey: apiKey });🤖 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 9360 - 9378, When targetApi === 'chat_completions' the code currently uses the Anthropic default baseUrl and blindly starts the built-in proxy with that upstreamBaseUrl; instead, in the block that calls stopBuiltinClaudeProxyRuntime() and startBuiltinClaudeProxyRuntime() validate that baseUrl (or config.baseUrl) is OpenAI-compatible before starting the proxy. If the upstream is not OpenAI-compatible, reject/fail fast (throw or return an error) or skip starting the proxy and allow runtime-side provider resolution to proceed; update the logic around targetApi, upstreamBaseUrl, and startBuiltinClaudeProxyRuntime to either set upstreamBaseUrl only when it's OpenAI-compatible or to error out when targetApi === 'chat_completions' and no OpenAI-compatible upstream is provided.
🤖 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.js`:
- Around line 15816-15829: The MCP tool's inputSchema for action
'codexmate.claude.config.apply' currently requires apiKey unconditionally, which
conflicts with applyToClaudeSettings() that allows targetApi: 'ollama' without
an API key; update the schema so apiKey is optional by removing it from the
top-level required array and add a JSON Schema conditional (if/then) that only
requires apiKey when targetApi is not 'ollama' (or equivalently, require apiKey
when targetApi equals the non-ollama value(s)); reference the inputSchema object
for 'codexmate.claude.config.apply' and the applyToClaudeSettings() behavior
when making this change.
---
Duplicate comments:
In `@cli.js`:
- Around line 9360-9378: When targetApi === 'chat_completions' the code
currently uses the Anthropic default baseUrl and blindly starts the built-in
proxy with that upstreamBaseUrl; instead, in the block that calls
stopBuiltinClaudeProxyRuntime() and startBuiltinClaudeProxyRuntime() validate
that baseUrl (or config.baseUrl) is OpenAI-compatible before starting the proxy.
If the upstream is not OpenAI-compatible, reject/fail fast (throw or return an
error) or skip starting the proxy and allow runtime-side provider resolution to
proceed; update the logic around targetApi, upstreamBaseUrl, and
startBuiltinClaudeProxyRuntime to either set upstreamBaseUrl only when it's
OpenAI-compatible or to error out when targetApi === 'chat_completions' and no
OpenAI-compatible upstream is provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8710ccd6-8260-45da-8a0b-5de00b139384
📒 Files selected for processing (21)
cli.jscli/claude-proxy.jstests/e2e/test-claude-proxy.jstests/e2e/test-claude.jstests/unit/claude-proxy-adapter.test.mjstests/unit/claude-settings-sync.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/app.jsweb-ui/logic.claude.mjsweb-ui/logic.runtime.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.session-timeline.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/i18n/locales/en.mjsweb-ui/modules/i18n/locales/ja.mjsweb-ui/modules/i18n/locales/vi.mjsweb-ui/modules/i18n/locales/zh.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/controls-forms.css
💤 Files with no reviewable changes (3)
- web-ui/modules/app.methods.session-timeline.mjs
- web-ui/res/web-ui-render.precompiled.js
- web-ui/logic.runtime.mjs
✅ Files skipped from review due to trivial changes (3)
- web-ui/modules/i18n/locales/zh.mjs
- tests/unit/claude-settings-sync.test.mjs
- web-ui/modules/i18n/locales/ja.mjs
🚧 Files skipped from review as they are similar to previous changes (9)
- web-ui/partials/index/panel-config-claude.html
- web-ui/app.js
- web-ui/modules/i18n/locales/vi.mjs
- web-ui/modules/app.methods.startup-claude.mjs
- web-ui/logic.claude.mjs
- web-ui/partials/index/modals-basic.html
- web-ui/modules/app.methods.claude-config.mjs
- tests/e2e/test-claude.js
- tests/e2e/test-claude-proxy.js
📜 Review details
🔇 Additional comments (20)
web-ui/modules/i18n/locales/en.mjs (1)
1094-1100: LGTM!tests/unit/web-ui-logic.test.mjs (1)
514-525: LGTM!Also applies to: 982-983, 1003-1004
web-ui/styles/controls-forms.css (1)
409-413: LGTM!Also applies to: 441-532
cli/claude-proxy.js (12)
87-114: LGTM!
135-141: LGTM!Also applies to: 163-165
277-364: LGTM!
366-423: LGTM!
427-538: LGTM!
693-740: LGTM!
916-921: LGTM!
995-1024: LGTM!
1056-1082: LGTM!
1283-1284: LGTM!Also applies to: 1307-1307
1342-1373: LGTM!
1472-1472: LGTM!Also applies to: 1521-1521, 1543-1546
tests/unit/claude-proxy-adapter.test.mjs (5)
1-14: LGTM!
69-90: LGTM!
132-169: LGTM!
171-227: LGTM!
296-317: LGTM!
awsl233777
changed the title
Collaborator
Author
|
Follow-up E2E pass completed after the previous update. What the end-to-end run covered:
The E2E run caught a real bug: Ollama upstream paths were being built as Validation after the fix:
@coderabbitai review |
Contributor
|
✅ Actions performedReview triggered.
|
Collaborator
Author
|
Follow-up for the latest CodeRabbit review is pushed in Changes:
Validation:
@coderabbitai review |
Contributor
|
Triggering a full review now. 🐇 ✅ Actions performedReview triggered.
|
Collaborator
Author
|
Fixed the Ollama apply path that could leave Claude Code on a stale remote Changes in
Validation run locally:
@coderabbitai review |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/claude-settings-sync.test.mjs (1)
1239-1261: ⚡ Quick winConsider behavioral tests for CLI validation.
The static analysis tests (regex matching on source code) verify that important guards and schema constraints exist, but they're brittle—they'll break if the implementation is refactored even when behavior stays correct.
For better maintainability, consider testing the actual runtime behavior instead. For example:
- For the chat_completions guard (lines 1239-1249): call
applyToClaudeSettingswithtargetApi: 'chat_completions'and nobaseUrl/provider, then verify it rejects with the expected error.- For the schema validation (lines 1251-1261): test that the schema correctly validates/rejects configs at runtime rather than checking its structure.
That said, the current approach does provide value by ensuring critical code paths exist.
🤖 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/claude-settings-sync.test.mjs` around lines 1239 - 1261, The tests are fragile because they assert source text instead of behavior; replace or augment the regex-based checks with behavioral tests: call the async function applyToClaudeSettings with targetApi: 'chat_completions' and no baseUrl/upstreamProvider and assert it rejects/throws with the expected error message, and instead of matching the CLI schema text for the tool name 'codexmate.claude.config.apply' extract or import the schema used by that tool and run it against example payloads (one with targetApi:'ollama' and no apiKey should pass, one with other targets should require apiKey) to assert runtime validation succeeds/fails as intended.
🤖 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/unit/claude-settings-sync.test.mjs`:
- Around line 1239-1261: The tests are fragile because they assert source text
instead of behavior; replace or augment the regex-based checks with behavioral
tests: call the async function applyToClaudeSettings with targetApi:
'chat_completions' and no baseUrl/upstreamProvider and assert it rejects/throws
with the expected error message, and instead of matching the CLI schema text for
the tool name 'codexmate.claude.config.apply' extract or import the schema used
by that tool and run it against example payloads (one with targetApi:'ollama'
and no apiKey should pass, one with other targets should require apiKey) to
assert runtime validation succeeds/fails as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d01381f-e5f1-4549-83d8-93915635f291
📒 Files selected for processing (3)
cli.jstests/unit/claude-settings-sync.test.mjsweb-ui/modules/app.methods.claude-config.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- web-ui/modules/app.methods.claude-config.mjs
- cli.js
📜 Review details
🔇 Additional comments (1)
tests/unit/claude-settings-sync.test.mjs (1)
6-6: LGTM!Also applies to: 589-592, 651-714, 753-802
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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.claude-config.mjs (1)
206-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
externalCredentialTypecheck causes inconsistent behavior.Line 206 checks
!config.apiKey && config.targetApi !== 'ollama'but does not account forexternalCredentialType. This is inconsistent with:
- Line 43 (validation):
!apiKey && !externalCredentialType && targetApi !== 'ollama'- Line 96 (onClaudeModelChange):
!this.claudeConfigs[name].apiKey && !this.claudeConfigs[name].externalCredentialType && this.claudeConfigs[name].targetApi !== 'ollama'A user with external credentials configured but no API key will see "已保存（未填写 API Key）" and the config will not be applied, even though external auth should be sufficient.
Proposed fix
-if (!config.apiKey && config.targetApi !== 'ollama') { +if (!config.apiKey && !config.externalCredentialType && config.targetApi !== 'ollama') { this.showMessage('已保存（未填写 API Key）', 'info'); this.closeEditConfigModal();🤖 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.claude-config.mjs` around lines 206 - 213, The early-exit condition in the save flow checks only config.apiKey and config.targetApi, causing configs that rely on externalCredentialType to be treated as "missing API Key"; update the if condition in the block that calls this.showMessage('已保存（未填写 API Key）', ...), this.closeEditConfigModal(), and this.refreshClaudeModelContext() to also check config.externalCredentialType (i.e., require both apiKey and externalCredentialType be missing before showing the "未填写 API Key" message), mirroring the validation logic used at line 43 and the onClaudeModelChange logic, so external credential-based configs are accepted and applied when saved.
♻️ Duplicate comments (1)
tests/e2e/test-claude-proxy.js (1)
358-372:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBridge settings restoration still needs
try/finallywrapper.The previous review comment flagged this issue and it was marked as addressed, but the fix doesn't appear to be present. If assertions at lines 369-371 fail, line 372 (
fs.writeFileSync(bridgeSettingsPath, savedBridgeSettings, 'utf-8')) is skipped, leaving the bridge settings file in a broken state. The outerfinallyblock (lines 405-416) does not restore this file.Suggested fix
const bridgeSettingsPath = path.join(tmpHome, '.codex', 'codexmate-openai-bridge.json'); const savedBridgeSettings = fs.readFileSync(bridgeSettingsPath, 'utf-8'); -fs.writeFileSync(bridgeSettingsPath, JSON.stringify({ providers: {} }, null, 2), 'utf-8'); -const missingBridgeStartResult = await api('claude-proxy-start', { - host: '127.0.0.1', - port: proxyPort, - provider: 'claude-proxy-openai-bridge-e2e', - authSource: 'provider', - targetApi: 'chat_completions', - timeoutMs: 5000 -}); -assert(missingBridgeStartResult.error && missingBridgeStartResult.error.includes('OpenAI 转换未配置'), 'claude proxy should return an explicit error when OpenAI bridge upstream is missing'); -const missingBridgeStatus = await api('claude-proxy-status'); -assert(missingBridgeStatus.running === false, 'failed OpenAI bridge resolution must not start Claude proxy runtime'); -fs.writeFileSync(bridgeSettingsPath, savedBridgeSettings, 'utf-8'); +try { + fs.writeFileSync(bridgeSettingsPath, JSON.stringify({ providers: {} }, null, 2), 'utf-8'); + const missingBridgeStartResult = await api('claude-proxy-start', { + host: '127.0.0.1', + port: proxyPort, + provider: 'claude-proxy-openai-bridge-e2e', + authSource: 'provider', + targetApi: 'chat_completions', + timeoutMs: 5000 + }); + assert(missingBridgeStartResult.error && missingBridgeStartResult.error.includes('OpenAI 转换未配置'), 'claude proxy should return an explicit error when OpenAI bridge upstream is missing'); + const missingBridgeStatus = await api('claude-proxy-status'); + assert(missingBridgeStatus.running === false, 'failed OpenAI bridge resolution must not start Claude proxy runtime'); +} finally { + fs.writeFileSync(bridgeSettingsPath, savedBridgeSettings, 'utf-8'); +}🤖 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-claude-proxy.js` around lines 358 - 372, The test modifies the bridge settings file but doesn’t guarantee restoration if assertions fail; wrap the block that writes to bridgeSettingsPath, calls api('claude-proxy-start') and api('claude-proxy-status'), and runs the assertions in a try/finally so savedBridgeSettings is always written back in the finally; specifically, move the fs.writeFileSync(bridgeSettingsPath, savedBridgeSettings, 'utf-8') into a finally that pairs with the try starting before the write of JSON.stringify({ providers: {} }, ...) to ensure restoration even on test assertion failures.
🤖 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 9367-9412: You tear down the existing builtin Claude proxy
(stopBuiltinClaudeProxyRuntime /
resetBuiltinClaudeProxySavedSettingsToResponses) before performing settings I/O
(readJsonObjectFromFile, backupFileIfNeededOnce, writeJsonAtomic), so failures
leave the old runtime un-restored; instead capture the current runtime/settings
state before touching anything (e.g. save whether a runtime was running and its
baseUrl/apiKey), delay calling stopBuiltinClaudeProxyRuntime until after
settings I/O succeeds, or if you must stop it first then on any subsequent
failure restart the previous runtime by calling startBuiltinClaudeProxyRuntime
with the saved parameters and restore saved settings via
resetBuiltinClaudeProxySavedSettingsToResponses; update the branches around
startBuiltinClaudeProxyRuntime, proxyStarted, settingsBaseUrl and settingsApiKey
and the error-return paths (including the later block noted at 9462-9465) so
failures perform restoration and do not leave Claude pointing at a stopped
proxy.
- Around line 15831-15846: The schema exemption currently only matches exact raw
targetApi 'ollama' so inputs like "Ollama" or " ollama " are rejected; update
the allOf/if clause in codexmate.claude.config.apply inputSchema to match
normalized values by replacing the const check for targetApi with a regex-based
pattern that allows optional surrounding whitespace and is case-insensitive
(e.g. use a pattern like '^[\\s]*[oO][lL][lL][aA][mM][aA][\\s]*$' for broad JSON
Schema compatibility), so the schema exemption mirrors normalizeClaudeTargetApi
and the behavior in applyToClaudeSettings.
In `@tests/unit/claude-proxy-adapter.test.mjs`:
- Around line 399-408: The test's findFreePortForTest() creates a TOCTOU by
probing a free port then closing it before
controller.startBuiltinClaudeProxyRuntime() binds, causing flaky EADDRINUSE; fix
by either (preferred) adding support for port: 0 in the proxy runtime so
startBuiltinClaudeProxyRuntime(controller/startBuiltinClaudeProxyRuntime or the
runtime that validates settings.port) accepts 0, performs server.listen(0,
host), and returns the actual bound port in its listenUrl/settings report, or
(alternatively) implement a small retry loop around
controller.startBuiltinClaudeProxyRuntime() in the test to catch EADDRINUSE and
retry a few times with backoff; update the runtime validation that currently
rejects port <= 0 to allow 0 and ensure the bound port is surfaced back to the
caller.
In `@tests/unit/claude-settings-sync.test.mjs`:
- Around line 1239-1261: The current tests only regex-match fragments and can be
fooled by unrelated strings; update them to assert the actual code
structure/behavior: for applyToClaudeSettings/cmdClaude, slice the source
between the identified indices (startIndex/endIndex), parse that slice into an
AST or evaluate the exported function to ensure the branch condition (targetApi
=== 'chat_completions' && !configuredBaseUrl && !upstreamProviderName) is
present as a guard (not just as text) and that the spread expression uses
configuredBaseUrl in the resulting returned object; for the MCP Claude schema
(name: 'codexmate.claude.config.apply' / handler -> applyToClaudeSettings),
extract the schema object (parse the JS AST or require the module) and assert
that the conditional schema is structured so that the property targetApi
const:'ollama' is paired with a then:{ required:['apiKey'] } (i.e., the then is
nested under the if/allOf clause), rather than just matching two separate
regexes.
In `@web-ui/logic.claude.mjs`:
- Around line 72-86: The duplicate/match helpers that compare Claude configs
(used by normalizeClaudeConfig and any config deduping/matching logic) currently
only compare baseUrl, model, and credential fields, so different targetApi
values (responses, chat_completions, ollama) collapse into one config; update
those helper functions to include the normalized targetApi value (the targetApi
produced in normalizeClaudeConfig) in their equality/hash checks and any
canonical key generation so configs with different targetApi are treated as
distinct. Ensure you reference the targetApi returned by normalizeClaudeConfig
and update all callers that build or compare identity keys so they incorporate
targetApi consistently.
---
Outside diff comments:
In `@web-ui/modules/app.methods.claude-config.mjs`:
- Around line 206-213: The early-exit condition in the save flow checks only
config.apiKey and config.targetApi, causing configs that rely on
externalCredentialType to be treated as "missing API Key"; update the if
condition in the block that calls this.showMessage('已保存（未填写 API Key）', ...),
this.closeEditConfigModal(), and this.refreshClaudeModelContext() to also check
config.externalCredentialType (i.e., require both apiKey and
externalCredentialType be missing before showing the "未填写 API Key" message),
mirroring the validation logic used at line 43 and the onClaudeModelChange
logic, so external credential-based configs are accepted and applied when saved.
---
Duplicate comments:
In `@tests/e2e/test-claude-proxy.js`:
- Around line 358-372: The test modifies the bridge settings file but doesn’t
guarantee restoration if assertions fail; wrap the block that writes to
bridgeSettingsPath, calls api('claude-proxy-start') and
api('claude-proxy-status'), and runs the assertions in a try/finally so
savedBridgeSettings is always written back in the finally; specifically, move
the fs.writeFileSync(bridgeSettingsPath, savedBridgeSettings, 'utf-8') into a
finally that pairs with the try starting before the write of JSON.stringify({
providers: {} }, ...) to ensure restoration even on test assertion failures.
🪄 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: 6f4a6567-0a47-4768-a4fc-a6bf2d22f2fc
📒 Files selected for processing (21)
cli.jscli/claude-proxy.jstests/e2e/test-claude-proxy.jstests/e2e/test-claude.jstests/unit/claude-proxy-adapter.test.mjstests/unit/claude-settings-sync.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/app.jsweb-ui/logic.claude.mjsweb-ui/logic.runtime.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.session-timeline.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/i18n/locales/en.mjsweb-ui/modules/i18n/locales/ja.mjsweb-ui/modules/i18n/locales/vi.mjsweb-ui/modules/i18n/locales/zh.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/controls-forms.css
💤 Files with no reviewable changes (2)
- web-ui/modules/app.methods.session-timeline.mjs
- web-ui/logic.runtime.mjs
📜 Review details
🔇 Additional comments (37)
web-ui/modules/i18n/locales/en.mjs (1)
1094-1100: LGTM!web-ui/partials/index/panel-config-claude.html (1)
152-153: LGTM!web-ui/modules/i18n/locales/vi.mjs (1)
239-245: LGTM!web-ui/app.js (4)
280-280: LGTM!
286-286: LGTM!
294-295: LGTM!
549-556: LGTM!web-ui/modules/i18n/locales/zh.mjs (1)
1097-1103: LGTM!cli.js (1)
9603-9607: LGTM!Also applies to: 11267-11268
cli/claude-proxy.js (13)
87-114: LGTM!
135-165: LGTM!
175-190: LGTM!
277-423: LGTM!
427-538: LGTM!
618-740: LGTM!
869-876: LGTM!
920-942: LGTM!
1004-1063: LGTM!
1065-1091: LGTM!
1351-1382: LGTM!
1469-1469: LGTM!
1548-1558: LGTM!tests/e2e/test-claude.js (2)
107-118: Always restoresettings.jsonin afinallyblock.If any assertion from Line 110 through Line 117 throws, the temp Claude settings stay corrupted and can poison later E2E cases.
1-2: LGTM!Also applies to: 6-6, 78-105
web-ui/modules/app.methods.startup-claude.mjs (1)
245-258: LGTM!web-ui/res/web-ui-render.precompiled.js (1)
1912-1925: LGTM!Also applies to: 5800-5813, 5953-5966
tests/e2e/test-claude-proxy.js (4)
1-2: LGTM!
103-144: LGTM!
263-347: LGTM!
412-414: LGTM!web-ui/modules/app.methods.claude-config.mjs (7)
26-29: LGTM!
43-45: LGTM!
96-100: LGTM!
119-127: LGTM!Also applies to: 147-159, 181-189
161-179: LGTM!
215-217: LGTM!
234-258: LGTM!Also applies to: 282-321
Comment on lines
+9367
to
9412
| if (targetApi === 'chat_completions' || targetApi === 'ollama') { | ||
| const upstreamProviderName = typeof config.name === 'string' ? config.name.trim() : ''; | ||
| if (targetApi === 'chat_completions' && !configuredBaseUrl && !upstreamProviderName) { | ||
| return { | ||
| success: false, | ||
| mode: 'claude-proxy', | ||
| error: 'chat_completions 模式需要显式的上游 Base URL 或可解析的 provider 名称' | ||
| }; | ||
| } | ||
| await stopBuiltinClaudeProxyRuntime(); | ||
| const proxyToken = crypto.randomBytes(24).toString('hex'); | ||
| proxyResult = await startBuiltinClaudeProxyRuntime({ | ||
| enabled: true, | ||
| host: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.host, | ||
| provider: upstreamProviderName, | ||
| authSource: 'provider', | ||
| targetApi, | ||
| timeoutMs: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.timeoutMs, | ||
| upstreamProviderName, | ||
| ...(configuredBaseUrl ? { upstreamBaseUrl: configuredBaseUrl } : {}), | ||
| upstreamApiKey: apiKey | ||
| }); | ||
| if (!proxyResult || proxyResult.error || proxyResult.success === false || !proxyResult.listenUrl) { | ||
| await stopBuiltinClaudeProxyRuntime(); | ||
| resetBuiltinClaudeProxySavedSettingsToResponses(); | ||
| return { | ||
| success: false, | ||
| mode: 'claude-proxy', | ||
| error: (proxyResult && proxyResult.error) || '启动 Claude 兼容代理失败' | ||
| }; | ||
| } | ||
| proxyStarted = true; | ||
| settingsBaseUrl = proxyResult.listenUrl; | ||
| settingsApiKey = proxyToken; | ||
| } else { | ||
| await stopBuiltinClaudeProxyRuntime(); | ||
| resetBuiltinClaudeProxySavedSettingsToResponses(); | ||
| } | ||
|
|
||
| const readResult = readJsonObjectFromFile(CLAUDE_SETTINGS_FILE, {}); | ||
| if (!readResult.ok) { | ||
| if (proxyStarted) { | ||
| await stopBuiltinClaudeProxyRuntime(); | ||
| resetBuiltinClaudeProxySavedSettingsToResponses(); | ||
| } | ||
| return { success: false, mode: 'settings-file', error: readResult.error }; |
Contributor
There was a problem hiding this comment.
Preserve the current Claude runtime until settings I/O succeeds.
This path tears down the active builtin Claude proxy before settings.json is even read. If readJsonObjectFromFile(...), backupFileIfNeededOnce(...), or writeJsonAtomic(...) fails, the rollback only stops the new proxy and resets saved mode; it never restores the previously running proxy or the old Claude settings. That can leave Claude still pointing at a proxy URL that has already been stopped, so a failed apply breaks an otherwise working setup.
Also applies to: 9462-9465
🤖 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 9367 - 9412, You tear down the existing builtin Claude
proxy (stopBuiltinClaudeProxyRuntime /
resetBuiltinClaudeProxySavedSettingsToResponses) before performing settings I/O
(readJsonObjectFromFile, backupFileIfNeededOnce, writeJsonAtomic), so failures
leave the old runtime un-restored; instead capture the current runtime/settings
state before touching anything (e.g. save whether a runtime was running and its
baseUrl/apiKey), delay calling stopBuiltinClaudeProxyRuntime until after
settings I/O succeeds, or if you must stop it first then on any subsequent
failure restart the previous runtime by calling startBuiltinClaudeProxyRuntime
with the saved parameters and restore saved settings via
resetBuiltinClaudeProxySavedSettingsToResponses; update the branches around
startBuiltinClaudeProxyRuntime, proxyStarted, settingsBaseUrl and settingsApiKey
and the error-return paths (including the later block noted at 9462-9465) so
failures perform restoration and do not leave Claude pointing at a stopped
proxy.
Comment on lines
+399
to
+408
| function findFreePortForTest() { | ||
| return new Promise((resolve, reject) => { | ||
| const server = net.createServer(); | ||
| server.once('error', reject); | ||
| server.listen(0, '127.0.0.1', () => { | ||
| const port = server.address().port; | ||
| server.close(() => resolve(port)); | ||
| }); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the relevant portion of the test file around the reported lines
sed -n '340,520p' tests/unit/claude-proxy-adapter.test.mjs | nl -ba | sed -n '1,220p'
# 2) Locate where the helper is called and how the port is passed into the runtime/controller
rg -n "findFreePortForTest|createBuiltinClaudeProxyRuntimeController|startBuiltinClaudeProxyRuntime" tests/unit/claude-proxy-adapter.test.mjs
# 3) Find the implementation that actually binds/listens on the chosen port
# (search in repo for the controller/runtime creation function name)
rg -n "createBuiltinClaudeProxyRuntimeController" -S .
rg -n "startBuiltinClaudeProxyRuntime" -S .
# 4) Inspect the implementation(s) that accept a port argument and listen/bind
# (grab a limited window around likely definitions)
rg -n "listen\\(|createServer\\(|net\\.createServer\\(" -S . | head -n 50Repository: SakuraByteCore/codexmate
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show relevant portion with line numbers
sed -n '340,520p' tests/unit/claude-proxy-adapter.test.mjs | cat -n
# 2) Find where helper/controller functions are called in the test file
rg -n "findFreePortForTest|createBuiltinClaudeProxyRuntimeController|startBuiltinClaudeProxyRuntime" tests/unit/claude-proxy-adapter.test.mjs
# 3) Locate implementations in repo
rg -n "createBuiltinClaudeProxyRuntimeController" -S .
rg -n "startBuiltinClaudeProxyRuntime" -S .
# 4) Look for port-binding/listen logic (limited)
rg -n "listen\\(|net\\.createServer\\(" -S tests/unit/claude-proxy-adapter.test.mjsRepository: SakuraByteCore/codexmate
Length of output: 9470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show relevant portion with line numbers
sed -n '340,520p' tests/unit/claude-proxy-adapter.test.mjs | cat -n
# 2) Find where helper/controller functions are called in the test file
rg -n "findFreePortForTest|createBuiltinClaudeProxyRuntimeController|startBuiltinClaudeProxyRuntime" tests/unit/claude-proxy-adapter.test.mjs
# 3) Locate implementations in repo
rg -n "createBuiltinClaudeProxyRuntimeController" -S .
rg -n "startBuiltinClaudeProxyRuntime" -S .
# 4) Look for port-binding/listen logic (limited)
rg -n "listen\\(|net\\.createServer\\(" -S tests/unit/claude-proxy-adapter.test.mjsRepository: SakuraByteCore/codexmate
Length of output: 9470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation details where the runtime binds the provided port
# (startBuiltinClaudeProxyRuntime + any listen/createServer calls in its body)
sed -n '1420,1565p' cli/claude-proxy.js | cat -n
# Also capture the body more narrowly around server.listen / http(s).listen usage
rg -n "startBuiltinClaudeProxyRuntime\\(|listen\\(|createServer\\(|http\\.createServer\\(|https\\.createServer\\(" cli/claude-proxy.js
# Show a tighter window around the most relevant listen call(s) if found
# (we'll use rg output line numbers later, but for now print a broader window around 1453)
sed -n '1440,1515p' cli/claude-proxy.js | cat -n
sed -n '1515,1605p' cli/claude-proxy.js | cat -nRepository: SakuraByteCore/codexmate
Length of output: 10884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and view buildBuiltinClaudeProxyListenUrl
rg -n "function buildBuiltinClaudeProxyListenUrl|buildBuiltinClaudeProxyListenUrl\\(" cli/claude-proxy.js
# Print around its definition (bounded window)
sed -n '1300,1425p' cli/claude-proxy.js | cat -n
# Inspect createBuiltinClaudeProxyServer around where it listens and sets listenUrl
sed -n '1360,1465p' cli/claude-proxy.js | cat -n
# Confirm second usage range in the test file
sed -n '420,490p' tests/unit/claude-proxy-adapter.test.mjs | cat -nRepository: SakuraByteCore/codexmate
Length of output: 15956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print buildBuiltinClaudeProxyListenUrl implementation
sed -n '960,1025p' cli/claude-proxy.js | cat -n
# Print any callers if needed (bounded)
rg -n "buildBuiltinClaudeProxyListenUrl\\(" cli/claude-proxy.jsRepository: SakuraByteCore/codexmate
Length of output: 3884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '380,490p' tests/unit/claude-proxy-adapter.test.mjs | cat -nRepository: SakuraByteCore/codexmate
Length of output: 5323
Fix TOCTOU risk from findFreePortForTest() by retrying or supporting port: 0.
In tests/unit/claude-proxy-adapter.test.mjs (399-408), findFreePortForTest() probes a port with server.listen(0, ...) and then immediately server.close()s it; later (439-470) the test passes that proxyPort into controller.startBuiltinClaudeProxyRuntime(), which binds with server.listen(settings.port, settings.host, ...). This creates a short TOCTOU window where another process can grab the port and make the test flaky (EADDRINUSE).
Either add port: 0 support in the proxy runtime (it currently rejects port <= 0) so the controller reports the actual bound port for listenUrl, or wrap startBuiltinClaudeProxyRuntime() in a small retry when it fails due to EADDRINUSE.
🤖 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/claude-proxy-adapter.test.mjs` around lines 399 - 408, The test's
findFreePortForTest() creates a TOCTOU by probing a free port then closing it
before controller.startBuiltinClaudeProxyRuntime() binds, causing flaky
EADDRINUSE; fix by either (preferred) adding support for port: 0 in the proxy
runtime so
startBuiltinClaudeProxyRuntime(controller/startBuiltinClaudeProxyRuntime or the
runtime that validates settings.port) accepts 0, performs server.listen(0,
host), and returns the actual bound port in its listenUrl/settings report, or
(alternatively) implement a small retry loop around
controller.startBuiltinClaudeProxyRuntime() in the test to catch EADDRINUSE and
retry a few times with backoff; update the runtime validation that currently
rejects port <= 0 to allow 0 and ensure the bound port is surfaced back to the
caller.
Collaborator
Author
|
Update pushed in
Validation run:
@coderabbitai re-review |
Contributor
|
Triggering a full re-review now. 🐇 ✅ Actions performedFull review triggered. |
Collaborator
Author
|
Follow-up pushed in
Validation run:
@coderabbitai re-review |
Contributor
|
Triggering a full re-review now. 🐇 ✅ Actions performedFull review triggered. |
Collaborator
Author
|
Follow-up pushed in
Validation run:
@coderabbitai re-review |
Contributor
|
Triggering a full re-review now. 🐇 ✅ Actions performedFull review triggered. |
awsl233777
changed the title
Collaborator
Author
|
Updated the Ollama Claude share/import path. Changes:
Validation actually run:
@coderabbitai re-review |
Contributor
|
Triggering a full re-review now. 🐇 ✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 9585-9591: The CLI currently accepts --target-api/--targetApi and
uses normalizeClaudeTargetApi(nextValue) but silently maps unknown values to
"responses"; change this to validate the normalized value and throw an
informative Error when normalizeClaudeTargetApi returns an unrecognized value so
typos don't fall back to responses and bypass proxy/token flow; update the same
validation where targetApi is later handled (the branch that applies direct
Claude settings) to reject unknown targetApi values as well, referencing the
normalizeClaudeTargetApi function and the targetApi variable to locate the
changes.
- Around line 8000-8012: The current validation checks raw config strings
(apiKey, baseUrl, model) before trimming, so whitespace-only values pass and
produce empty payload fields; fix by trimming each value up-front (e.g., assign
trimmedBaseUrl = (config.baseUrl || '').trim(), trimmedApiKey = (config.apiKey
|| '').trim(), trimmedModel = (config.model || '').trim()), then use those
trimmed variables for the existence checks (and when calling
normalizeClaudeTargetApi if needed) and finally serialize
trimmedBaseUrl/trimmedApiKey/(trimmedModel || DEFAULT_CLAUDE_MODEL) into the
payload so whitespace-only inputs are rejected correctly; update references to
apiKey/baseUrl/model in this block accordingly.
- Line 16361: Update the help text for the "codexmate claude" usage string so it
shows the Ollama no-key shorthand rather than implying an API key is required:
modify the console.log that currently prints "codexmate claude <BaseURL> <API密钥>
[模型] [--target-api responses|chat_completions|ollama] 写入 Claude Code 配置" to
document the optional/alternate form (e.g., "codexmate claude <BaseURL>
[<API密钥>] <模型> --target-api ollama" or explicitly note that the API密钥 is
optional for --target-api ollama); locate and update the string used by the
help/output generator where "codexmate claude" is emitted so both the standard
and Ollama no-key forms are shown or the API key is marked optional.
🪄 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: aa9bbb3e-ebe6-4472-91f0-9d9bad9c5d08
📒 Files selected for processing (18)
cli.jscli/update.jstests/e2e/test-claude.jstests/unit/config-tabs-ui.test.mjstests/unit/install-target-cards.test.mjstests/unit/provider-share-command.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/modules/app.methods.index.mjsweb-ui/modules/app.methods.install.mjsweb-ui/modules/app.methods.session-actions.mjsweb-ui/modules/i18n/locales/en.mjsweb-ui/modules/i18n/locales/ja.mjsweb-ui/modules/i18n/locales/vi.mjsweb-ui/modules/i18n/locales/zh.mjsweb-ui/partials/index/layout-header.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/layout-shell.css
✅ Files skipped from review due to trivial changes (4)
- tests/unit/web-ui-behavior-parity.test.mjs
- web-ui/modules/i18n/locales/en.mjs
- web-ui/modules/i18n/locales/ja.mjs
- web-ui/modules/i18n/locales/zh.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- web-ui/modules/i18n/locales/vi.mjs
- web-ui/app.js
- web-ui/res/web-ui-render.precompiled.js
- tests/e2e/test-claude.js
📜 Review details
🔇 Additional comments (17)
cli.js (1)
151-151: LGTM!Also applies to: 11114-11134
tests/unit/provider-share-command.test.mjs (4)
377-398: LGTM!
400-413: LGTM!
429-442: LGTM!
444-461: LGTM!web-ui/modules/app.methods.session-actions.mjs (2)
389-393: LGTM!
394-398: LGTM!tests/unit/config-tabs-ui.test.mjs (3)
138-140: LGTM!Also applies to: 148-149
292-297: LGTM!
298-305: LGTM!cli/update.js (1)
67-104: LGTM!web-ui/modules/app.methods.install.mjs (2)
20-63: LGTM!Also applies to: 94-99
65-92: Alignversion-statusresponse fields withloadAppVersionStatus
cli.js’scase 'version-status'returns{ currentVersion, latestVersion }on success and{ currentVersion, latestVersion: '', error }on failure, matchingloadAppVersionStatus’s usage ofres.currentVersion,res.latestVersion, andres.error.web-ui/modules/app.methods.index.mjs (1)
92-92: LGTM!web-ui/partials/index/layout-header.html (1)
113-124: LGTM!web-ui/styles/layout-shell.css (1)
510-563: LGTM!tests/unit/install-target-cards.test.mjs (1)
45-78: LGTM!
Comment on lines
+9585
to
+9591
| if (token === '--target-api' || token === '--targetApi') { | ||
| const nextValue = String(argv[i + 1] ?? ''); | ||
| if (!nextValue || nextValue.startsWith('--')) { | ||
| throw new Error('错误: --target-api 需要一个值（responses、chat_completions 或 ollama）'); | ||
| } | ||
| targetApi = normalizeClaudeTargetApi(nextValue); | ||
| i += 1; |
Contributor
There was a problem hiding this comment.
Reject unknown --target-api values instead of silently falling back to responses.
Line 9590 maps any unrecognized value to responses. Here that is not a safe default: a typo skips the proxy-backed path and Line 9640-Line 9645 will apply direct Claude settings instead of the local proxy/token flow.
Proposed fix
if (token === '--target-api' || token === '--targetApi') {
const nextValue = String(argv[i + 1] ?? '');
if (!nextValue || nextValue.startsWith('--')) {
throw new Error('错误: --target-api 需要一个值（responses、chat_completions 或 ollama）');
}
- targetApi = normalizeClaudeTargetApi(nextValue);
+ const rawTargetApi = nextValue.trim().toLowerCase();
+ const allowedTargetApis = new Set([
+ 'responses',
+ 'chat_completions',
+ 'chat-completions',
+ 'chat/completions',
+ 'ollama'
+ ]);
+ if (!allowedTargetApis.has(rawTargetApi)) {
+ throw new Error(`错误: 不支持的 --target-api 值: ${nextValue}`);
+ }
+ targetApi = normalizeClaudeTargetApi(rawTargetApi);
i += 1;
continue;
}Also applies to: 9640-9645
🤖 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 9585 - 9591, The CLI currently accepts
--target-api/--targetApi and uses normalizeClaudeTargetApi(nextValue) but
silently maps unknown values to "responses"; change this to validate the
normalized value and throw an informative Error when normalizeClaudeTargetApi
returns an unrecognized value so typos don't fall back to responses and bypass
proxy/token flow; update the same validation where targetApi is later handled
(the branch that applies direct Claude settings) to reject unknown targetApi
values as well, referencing the normalizeClaudeTargetApi function and the
targetApi variable to locate the changes.
Collaborator
Author
|
Updated the Ollama Claude share/import path and added a real end-to-end acceptance check. Changes in this round:
Validation actually run:
Real project-demand acceptance run against the provided Ollama server:
Note: subscription-gated models on that Ollama server ( @coderabbitai re-review |
Contributor
|
Triggering a full re-review now. 🐇 ✅ Actions performedFull review triggered. |
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
/v1/modelsand/v1/messagesthrough the selected upstream API, including Anthropic → Ollama/api/chatconversion.Real endpoint validation
Validated the Ollama target mode against a provided real Ollama endpoint without writing to the user's real
~/.codexconfig:targetApi: 'ollama'and a direct upstream base URL.GET /v1/modelsrouted through the proxy to upstream/api/tagsand returned the expected Ollama cloud model list.POST /v1/messagesrouted through the proxy to upstream/api/chatand converted the response back into an Anthropic-style message:minimax-m3:cloudsucceeded with a non-empty response whenmax_tokenswas raised to 128.gemma4:31b-cloudsucceeded with a non-empty response.Observed edge case:
minimax-m3:cloudcan return onlymessage.thinkingwith an emptymessage.contentwhenmax_tokens/ Ollamanum_predictis too small (for example 32 with a system prompt). This is upstream model behavior; practical configs should avoid very small max token limits for that model.Validation
zip-lib; that failure was an environment dependency issue, not an Ollama proxy-path failure.Summary by CodeRabbit
Release Notes
New Features
Improvements