Conversation
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
📝 WalkthroughWalkthroughThis PR removes the built-in proxy ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant WebUI as Web UI
participant LocalAPI as Local HTTP API
participant MCP as MCP Server
participant Core as Core Services
Note over CLI,WebUI: Prior flow included builtin proxy & auth management
CLI->>LocalAPI: config / proxy control requests
WebUI->>LocalAPI: provider/auth/proxy requests
LocalAPI->>MCP: relay/proxy requests
MCP->>Core: service ops
MCP-->>LocalAPI: proxy/auth callbacks
LocalAPI-->>CLI: proxy status & responses
Note over CLI,WebUI: New flow removes builtin proxy/auth control paths
CLI->>LocalAPI: config/start/status requests (proxy ops -> removed/error)
WebUI->>LocalAPI: provider/auth requests (no proxy-status polling)
LocalAPI->>MCP: relay requests
MCP->>Core: service ops
MCP-->>LocalAPI: responses (no builtin proxy callbacks)
LocalAPI-->>CLI: responses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.0)web-ui/app.jsComment |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 5948-5954: The CLI stubs (applyBuiltinProxyProvider and
ensureBuiltinProxyForCodexDefault) block only CLI wrappers but other entry
points still start the proxy; update the API handlers that dispatch
"proxy-start", "proxy-stop", and "proxy-save-config" and the exported
createMcpTools surfaces (codexmate.proxy.start, codexmate.proxy.stop,
codexmate.proxy.provider.apply) to return the same removal error object
{"error":"该功能已移除"} immediately instead of performing any action; locate the
proxy-start/stop/save-config dispatch functions and the createMcpTools
implementation and add an early return/error throw that mirrors
applyBuiltinProxyProvider/ensureBuiltinProxyForCodexDefault so the feature is
uniformly unavailable.
- Around line 5576-5671: The removePersistedBuiltinProxyProviderFromConfig
function currently deletes the provider block but doesn't update top-level
fields (model_provider/model), leaving stale references; fix it by updating the
same config/content pass: after determining providerSegmentVariants (and before
writing), remove rebuilt.model_providers[BUILTIN_PROXY_PROVIDER_NAME] and if
rebuilt.model_provider === BUILTIN_PROXY_PROVIDER_NAME or rebuilt.model ===
BUILTIN_PROXY_PROVIDER_NAME (or points to the removed provider via legacy
segments), set those top-level fields to the same fallback selection logic you
use for sanitized reads (choose the next available provider or delete the
field), and ensure writeConfig writes the updated TOML; reference symbols:
removePersistedBuiltinProxyProviderFromConfig, BUILTIN_PROXY_PROVIDER_NAME,
providerSegmentVariants, providerSegments, rebuilt, readConfig, writeConfig.
In `@tests/e2e/test-setup.js`:
- Around line 55-59: The test currently calls runSync(node, [cliPath], { env })
which exercises the no-arg behavior; change it to call the actual help
entrypoint instead (e.g., runSync(node, [cliPath, '--help'], { env }) or
runSync(node, [cliPath, 'help'], { env }) depending on the CLI) so helpResult
reflects the real help renderer; update the runSync invocation that produces
helpResult and keep subsequent assertions against helpResult.stdout as-is to
ensure the removed entries are not exposed.
In `@web-ui/app.js`:
- Around line 574-576: The backend is doing a case-sensitive check for the
builtin proxy name, causing mismatches with the UI's case-insensitive filter;
update isBuiltinProxyProvider (and any other occurrences around the noted
blocks) to compare in a case-insensitive way by normalizing the input and
BUILTIN_PROXY_PROVIDER_NAME with .toLowerCase() (e.g., replace
providerName.trim() === BUILTIN_PROXY_PROVIDER_NAME with
providerName.trim().toLowerCase() ===
BUILTIN_PROXY_PROVIDER_NAME.toLowerCase()), so variants like "CODEXMATE-PROXY"
are correctly recognized and blocked (also ensure related checks used by
runHealthCheck() use the same normalized comparison).
🪄 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: ac62347d-7aa7-402e-b4ec-3e06fe91d230
📒 Files selected for processing (6)
README.mdcli.jstests/e2e/test-auth-proxy.jstests/e2e/test-setup.jsweb-ui/app.jsweb-ui/index.html
💤 Files with no reviewable changes (1)
- README.md
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (16)
web-ui/index.html (3)
435-435: The bridge hint now points users at the remaining editable surface.This keeps the bridge guidance intact while making it clear that edits only happen from Codex mode.
520-523: Read-only providers are now clearly distinguished as system entries.The
系统badge plus the simplified subtitle fits the legacy-surface removal and keeps non-editable providers easy to recognize.
1833-1833: The OpenClaw empty-state copy now matches the removed proxy/auth flows.This avoids pointing users at configuration paths that no longer exist in the product.
tests/e2e/test-auth-proxy.js (6)
50-50: The initial proxy assertion now validates the contract without assuming startup state.This makes the test more deterministic while still checking that
proxy-statusreturns the expected shape.
105-113: Auth-profile storage is now pinned to the.codexlocation.These assertions guard both the intended write path and the regression back to the legacy
~/.codexmate/...location.
154-168: The legacy provider block is injected throughconfig.tomldirectly.That exercises the cleanup path without reviving the removed built-in provider workflow.
169-178: Proxy startup is now validated through the surviving runtime surface.Checking
listenUrlhere keeps the e2e flow aligned with the post-removal API.
180-188: This covers both hidden-provider behavior and on-disk cleanup.Asserting the provider stays out of
listand the TOML section is physically removed gives solid end-to-end coverage of the sanitizer.
191-199: The removed enable flow is still checked for a clean rejection.Keeping this assertion protects the “unavailable by design” behavior instead of letting the old surface reappear silently.
web-ui/app.js (1)
728-731: Nice bootstrap split.Keeping the auth-profile refresh outside the main init block avoids holding the shell in loading state for secondary startup work.
cli.js (6)
929-941: Good first-run auth profile guard.Pre-creating
AUTH_PROFILES_DIRbefore registry/profile access removes theENOENTedge case on fresh installs without changing the profile format.Also applies to: 999-1000, 1089-1090, 1135-1135, 1184-1184
3188-3214: Read-path sanitization is in the right place.Filtering
codexmate-proxyfrom both persisted and virtual config loads keeps status/list/setup flows from resurfacing the removed provider.Also applies to: 3219-3250
3346-3347: Nice defensive normalization around provider/model selection.Restricting trims to strings and basing wizard/switch defaults on sanitized config avoids legacy or non-string values leaking into interactive flows.
Also applies to: 7595-7596, 7882-7882
3424-3424: Reserved-name errors are now consistent.The
codexmate-proxywording is aligned across add/update/delete paths, which makes the removed-provider rule clearer.Also applies to: 3502-3502, 3520-3520, 3550-3550, 8049-8049
5555-5574: The residual proxy helpers are easier to reason about now.Keeping the listen URL and provider-block shape in one place reduces drift while the legacy proxy cleanup paths still need to understand that config.
12767-12767: Thecodexwrapper help is explicit.Surfacing the queued follow-up flags directly in the usage line makes the passthrough behavior easier to discover.
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)
3415-3425:⚠️ Potential issue | 🟠 MajorReserve
codexmate-proxyon the CLI add/setup path too.This guard only protects
addProviderToConfig().cmdAdd()still acceptscodexmate-proxy, socodexmate add codexmate-proxy ...(and the setup flow that delegates to it) can recreate the removed provider and then have it disappear again on the next sanitized read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3415 - 3425, The cmdAdd flow still allows creating the reserved provider; update cmdAdd (and any setup flow that calls it) to perform the same validation as addProviderToConfig by rejecting names where isBuiltinProxyProvider(name) is true unless allowManaged is explicitly true, returning the same error message ('codexmate-proxy 为保留名称，不可手动添加'); ensure the same validations (isValidProviderName, isReservedProviderNameForCreation) are applied in cmdAdd so the CLI and addProviderToConfig behave consistently and the setup path cannot recreate the reserved provider.
♻️ Duplicate comments (3)
tests/e2e/test-auth-proxy.js (1)
154-189:⚠️ Potential issue | 🟠 MajorExercise a mixed-case legacy proxy name in this cleanup test.
The new coverage only seeds/asserts lowercase
codexmate-proxy, but the cleanup helper removes the legacy provider by exact key. A persisted[model_providers."CodexMate-Proxy"]block would evade both the exact-name.some(...)check and the case-sensitive table regex here, whileweb-ui/app.jsstill hides that name case-insensitively at Line 576. That means this e2e can pass while a hidden legacy proxy remains inconfig.toml.🧪 Suggested test adjustment
- fs.appendFileSync(configTomlPath, [ + const legacyProxyName = 'CodexMate-Proxy'; + fs.appendFileSync(configTomlPath, [ '', - '[model_providers.codexmate-proxy]', - 'name = "codexmate-proxy"', + `[model_providers."${legacyProxyName}"]`, + `name = "${legacyProxyName}"`, `base_url = "http://127.0.0.1:${proxyPort}/v1"`, 'wire_api = "responses"', 'requires_openai_auth = false', @@ const providerList = await api('list'); assert( - !providerList.providers.some((item) => item && item.name === 'codexmate-proxy'), + !providerList.providers.some((item) => + String(item && item.name ? item.name : '').trim().toLowerCase() === 'codexmate-proxy' + ), 'provider list should not expose removed builtin proxy provider' ); const configTomlAfterList = fs.readFileSync(configTomlPath, 'utf-8'); assert( - !/^\s*\[\s*model_providers\s*\.\s*(?:"codexmate-proxy"|'codexmate-proxy'|codexmate-proxy)\s*\]\s*$/m.test(configTomlAfterList), + !/^\s*\[\s*model_providers\s*\.\s*(?:"codexmate-proxy"|'codexmate-proxy'|codexmate-proxy)\s*\]\s*$/mi.test(configTomlAfterList), 'provider list read should physically remove legacy codexmate-proxy block from config.toml' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/test-auth-proxy.js` around lines 154 - 189, The test seeds only lowercase "codexmate-proxy" but the cleanup is case-sensitive, so update the seeded config block and assertions to exercise mixed-case legacy names and perform case-insensitive checks: when appending to configTomlPath include a variant like "CodexMate-Proxy"; change the provider-list assertion that checks providerList.providers.some(...) to compare item.name.toLowerCase() === 'codexmate-proxy'; and change the config.toml regex test to use a case-insensitive match (or normalize the file string to lower-case before testing) so legacy table names with mixed case are detected and removed.cli.js (2)
5948-5954:⚠️ Potential issue | 🟠 MajorThe proxy is still startable outside these CLI stubs.
applyBuiltinProxyProvider()andcmdProxy()now fail fast, but/apistill dispatchesproxy-start/proxy-save-config, andcreateMcpTools()still exposescodexmate.proxy.start/stop/provider.apply. The removed proxy remains programmatically available unless those shared entry points return the same该功能已移除error too.Also applies to: 10789-10792
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5948 - 5954, API and programmatic entry points still allow starting the removed proxy; update the shared interfaces so all callers return the same "该功能已移除" error: modify the /api handlers that dispatch "proxy-start" and "proxy-save-config" to short-circuit and return that error, update cmdProxy to return/throw the same error, and change createMcpTools so codexmate.proxy.start, codexmate.proxy.stop and codexmate.proxy.provider.apply all return { error: '该功能已移除' } (or throw an equivalent error) so the proxy cannot be started programmatically through any exposed surface.
5576-5671:⚠️ Potential issue | 🟠 MajorPersisted cleanup still leaves stale
model_provider/modelpointers.This helper removes the
[model_providers.codexmate-proxy]block, but it never rewrites the top-level selection, and the early return once the section is already gone means the file can keepmodel_provider = "codexmate-proxy"indefinitely. RawreadConfig()callers likecmdUseModel()will still consume that stale provider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5576 - 5671, The function removePersistedBuiltinProxyProviderFromConfig() currently bails out if the [model_providers.codexmate-proxy] block is missing, leaving top-level model_provider/model pointers stale; modify it to always inspect and, when removing the provider (or when provider block already absent), also remove or rewrite any top-level references (e.g., config.model_provider, config.model) that point to BUILTIN_PROXY_PROVIDER_NAME so callers like cmdUseModel()/readConfig() don't keep consuming a removed provider. Concretely: after reading config (and even if model_providers[BUILTIN_PROXY_PROVIDER_NAME] is falsy), check and delete any top-level keys (config.model_provider, config.model, etc.) whose value === BUILTIN_PROXY_PROVIDER_NAME, then proceed to rebuild/write updatedContent (using writeConfig) so the TOML output no longer contains stale references; ensure this logic is applied both in the branch that edits raw file ranges and in the fallback rebuilt branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cli.js`:
- Around line 3415-3425: The cmdAdd flow still allows creating the reserved
provider; update cmdAdd (and any setup flow that calls it) to perform the same
validation as addProviderToConfig by rejecting names where
isBuiltinProxyProvider(name) is true unless allowManaged is explicitly true,
returning the same error message ('codexmate-proxy 为保留名称，不可手动添加'); ensure the
same validations (isValidProviderName, isReservedProviderNameForCreation) are
applied in cmdAdd so the CLI and addProviderToConfig behave consistently and the
setup path cannot recreate the reserved provider.
---
Duplicate comments:
In `@cli.js`:
- Around line 5948-5954: API and programmatic entry points still allow starting
the removed proxy; update the shared interfaces so all callers return the same
"该功能已移除" error: modify the /api handlers that dispatch "proxy-start" and
"proxy-save-config" to short-circuit and return that error, update cmdProxy to
return/throw the same error, and change createMcpTools so codexmate.proxy.start,
codexmate.proxy.stop and codexmate.proxy.provider.apply all return { error:
'该功能已移除' } (or throw an equivalent error) so the proxy cannot be started
programmatically through any exposed surface.
- Around line 5576-5671: The function
removePersistedBuiltinProxyProviderFromConfig() currently bails out if the
[model_providers.codexmate-proxy] block is missing, leaving top-level
model_provider/model pointers stale; modify it to always inspect and, when
removing the provider (or when provider block already absent), also remove or
rewrite any top-level references (e.g., config.model_provider, config.model)
that point to BUILTIN_PROXY_PROVIDER_NAME so callers like
cmdUseModel()/readConfig() don't keep consuming a removed provider. Concretely:
after reading config (and even if model_providers[BUILTIN_PROXY_PROVIDER_NAME]
is falsy), check and delete any top-level keys (config.model_provider,
config.model, etc.) whose value === BUILTIN_PROXY_PROVIDER_NAME, then proceed to
rebuild/write updatedContent (using writeConfig) so the TOML output no longer
contains stale references; ensure this logic is applied both in the branch that
edits raw file ranges and in the fallback rebuilt branch.
In `@tests/e2e/test-auth-proxy.js`:
- Around line 154-189: The test seeds only lowercase "codexmate-proxy" but the
cleanup is case-sensitive, so update the seeded config block and assertions to
exercise mixed-case legacy names and perform case-insensitive checks: when
appending to configTomlPath include a variant like "CodexMate-Proxy"; change the
provider-list assertion that checks providerList.providers.some(...) to compare
item.name.toLowerCase() === 'codexmate-proxy'; and change the config.toml regex
test to use a case-insensitive match (or normalize the file string to lower-case
before testing) so legacy table names with mixed case are detected and removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc2c68df-d52c-43e1-b9f9-9b8210e20645
📒 Files selected for processing (6)
README.mdcli.jstests/e2e/test-auth-proxy.jstests/e2e/test-setup.jsweb-ui/app.jsweb-ui/index.html
💤 Files with no reviewable changes (1)
- README.md
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (4)
tests/e2e/test-setup.js (1)
55-59: LGTM! Correctly verifies removed proxy/auth CLI entries.The assertions properly validate that the help output no longer exposes
codexmate proxy,codexmate auth, or内建代理, aligning with the PR's goal of removing legacy proxy surfaces.web-ui/index.html (3)
435-436: LGTM! Hint text correctly updated for proxy/auth removal.The hint now only mentions "模板" (templates) as editable in Codex mode, consistent with the removal of authentication and proxy management features.
520-524: LGTM! Provider card UI correctly simplified.
- Badge text changed from "内建" to "系统" for read-only providers.
- Subtitle now consistently shows the URL (or placeholder), removing the special handling for legacy proxy descriptions.
Both changes align with the removal of built-in proxy surfaces.
1833-1834: LGTM! Empty-state hint updated for new architecture.The hint now correctly indicates that missing providers may come from "环境变量或外部配置" instead of the removed "内置 provider 或 auth profiles".
ymkiux
left a comment
ymkiux
left a comment
There was a problem hiding this comment.
This issue was automatically fixed by @coderabbitai.
ymkiux
left a comment
ymkiux
left a comment
There was a problem hiding this comment.
This issue was automatically fixed by coderabbitai
|
@coderabbitai Autofix |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
|
Triggering a full re-review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
ymkiux
left a comment
ymkiux
left a comment
There was a problem hiding this comment.
This issue was automatically fixed by coderabbitai
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cli.js (2)
5576-5671:⚠️ Potential issue | 🟠 MajorCleanup still leaves
config.tomlinternally inconsistent.This helper removes the
codexmate-proxyprovider block, but it still never rewrites top-levelmodel_provider/model, and Line 5588 stops cleanup completely once the section is already gone. After any sanitized read, later plainreadConfig()callers likecmdUseModel()can still readmodel_provider = "codexmate-proxy"and keep persisting state against a removed provider. Please apply the same fallback selection here before writing the file back.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5576 - 5671, removePersistedBuiltinProxyProviderFromConfig currently deletes the provider TOML section but does not update top-level model_provider/model fields, leaving config.toml internally inconsistent and causing callers like cmdUseModel/readConfig to keep referencing BUILTIN_PROXY_PROVIDER_NAME; after you build the rebuilt object (rebuilt.model_providers), run the same fallback selection logic used elsewhere (the sanitized read) to choose a new top-level model_provider and model (or remove those keys) when the builtin provider was removed, update rebuilt.model_provider and rebuilt.model accordingly (or delete them if no fallback), then continue with toml.stringify/marker preservation and writeConfig as before to ensure the file is consistent.
5948-5954:⚠️ Potential issue | 🟠 MajorRemoved proxy is still reachable through API/MCP entry points.
Line 5949 only blocks the CLI wrapper.
/apistill routesproxy-save-config,proxy-start, andproxy-stopto the live implementations, and the MCP catalog still exposescodexmate.proxy.start/stop/provider.apply, so the removed proxy can still be configured and started programmatically. Those entry points should return the same该功能已移除error too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5948 - 5954, The CLI-only removal left runtime endpoints still enabling the proxy; update the API/MCP handlers for proxy-save-config, proxy-start, and proxy-stop as well as the MCP catalog entries codexmate.proxy.start, codexmate.proxy.stop, and provider.apply to return the same { error: '该功能已移除' } response; locate where those routes/handlers call applyBuiltinProxyProvider or ensureBuiltinProxyForCodexDefault (or where the MCP action handlers are registered) and change them to short-circuit and return the identical error object so the proxy cannot be configured or started programmatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cli.js`:
- Around line 5576-5671: removePersistedBuiltinProxyProviderFromConfig currently
deletes the provider TOML section but does not update top-level
model_provider/model fields, leaving config.toml internally inconsistent and
causing callers like cmdUseModel/readConfig to keep referencing
BUILTIN_PROXY_PROVIDER_NAME; after you build the rebuilt object
(rebuilt.model_providers), run the same fallback selection logic used elsewhere
(the sanitized read) to choose a new top-level model_provider and model (or
remove those keys) when the builtin provider was removed, update
rebuilt.model_provider and rebuilt.model accordingly (or delete them if no
fallback), then continue with toml.stringify/marker preservation and writeConfig
as before to ensure the file is consistent.
- Around line 5948-5954: The CLI-only removal left runtime endpoints still
enabling the proxy; update the API/MCP handlers for proxy-save-config,
proxy-start, and proxy-stop as well as the MCP catalog entries
codexmate.proxy.start, codexmate.proxy.stop, and provider.apply to return the
same { error: '该功能已移除' } response; locate where those routes/handlers call
applyBuiltinProxyProvider or ensureBuiltinProxyForCodexDefault (or where the MCP
action handlers are registered) and change them to short-circuit and return the
identical error object so the proxy cannot be configured or started
programmatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3356273a-fd61-4ff7-8ec0-dfe710470d27
📒 Files selected for processing (2)
cli.jstests/e2e/test-setup.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/test-setup.js
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cli.js (2)
5576-5590:⚠️ Potential issue | 🟠 MajorPersisted proxy cleanup can still leave
config.tomlinconsistent.Line 5588 exits as soon as the exact
model_providers.codexmate-proxykey is gone, and neither write path rewrites top-levelmodel_provider/model. After one cleanup pass the file can still keepmodel_provider = "codexmate-proxy"indefinitely; mixed-case legacy keys also bypass cleanup now that matching is case-insensitive elsewhere. Raw readers likecmdUseModel()andgetConfigTemplate()will then continue from an invalid provider state.Also applies to: 5649-5665
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5576 - 5590, removePersistedBuiltinProxyProviderFromConfig currently only deletes model_providers[BUILTIN_PROXY_PROVIDER_NAME] and returns early, leaving top-level fields like model_provider/model untouched (and legacy mixed-case keys unhandled), which causes cmdUseModel() and getConfigTemplate() to read an invalid provider; update removePersistedBuiltinProxyProviderFromConfig (and the analogous cleanup code around the 5649-5665 region) to: locate and remove any case-variant keys for the builtin provider under model_providers, rewrite the config object to clear or normalize top-level model_provider and model fields when the provider is removed, and persist the modified config back to CONFIG_FILE (ensuring both success and error paths write the corrected TOML) so the file is consistent for raw readers like cmdUseModel() and getConfigTemplate().
5948-5953:⚠️ Potential issue | 🟠 MajorThe removed proxy is still reachable through
/apiand MCP.These CLI stubs only close the CLI entry points.
/apistill runsproxy-startat Line 10270, andcreateMcpTools()still exposescodexmate.proxy.startat Line 12518, so the builtin proxy can still be launched programmatically after this PR.Also applies to: 10789-10792
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5948 - 5953, The CLI stubs (applyBuiltinProxyProvider and ensureBuiltinProxyForCodexDefault) were changed but other entry points still start the builtin proxy; locate and modify the code paths that call the proxy, specifically where "proxy-start" is invoked by the API startup and where createMcpTools() exposes codexmate.proxy.start, and short-circuit them to return the same removed-feature error (or remove the export) so the builtin proxy cannot be launched programmatically; update the proxy-start invocation to check these stubs (applyBuiltinProxyProvider / ensureBuiltinProxyForCodexDefault) and propagate their { error: '该功能已移除' } response, and remove/replace the codexmate.proxy.start export in createMcpTools() to prevent external launches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 3423-3425: The cmdAdd() path does not enforce the same
reserved-name guard as addProviderToConfig(), allowing users to re-add the
built-in proxy "codexmate-proxy"; update cmdAdd() to call
isBuiltinProxyProvider(name) (same check used in addProviderToConfig()) and
reject the add unless allowManaged is true, returning the same error
object/message used elsewhere; specifically modify the cmdAdd handler to perform
the isBuiltinProxyProvider(name) && !allowManaged check before creating or
persisting a provider so the CLI cannot recreate reserved entries.
In `@tests/e2e/test-auth-proxy.js`:
- Around line 180-189: The test currently dereferences providerList.providers
directly after calling api('list'), which will throw if api returns an error
object; before calling .some(...) assert the response shape by checking that
providerList is an object and providerList.providers is an array (e.g.,
assert(providerList && Array.isArray(providerList.providers), 'expected
api("list") to return { providers: [...] } but got: ' +
JSON.stringify(providerList))), then proceed to assert no provider named
'codexmate-proxy' and the config.toml content checks; use the existing
providerList variable and api('list') call so you only add the defensive
assertion prior to the .some(...) call.
---
Duplicate comments:
In `@cli.js`:
- Around line 5576-5590: removePersistedBuiltinProxyProviderFromConfig currently
only deletes model_providers[BUILTIN_PROXY_PROVIDER_NAME] and returns early,
leaving top-level fields like model_provider/model untouched (and legacy
mixed-case keys unhandled), which causes cmdUseModel() and getConfigTemplate()
to read an invalid provider; update
removePersistedBuiltinProxyProviderFromConfig (and the analogous cleanup code
around the 5649-5665 region) to: locate and remove any case-variant keys for the
builtin provider under model_providers, rewrite the config object to clear or
normalize top-level model_provider and model fields when the provider is
removed, and persist the modified config back to CONFIG_FILE (ensuring both
success and error paths write the corrected TOML) so the file is consistent for
raw readers like cmdUseModel() and getConfigTemplate().
- Around line 5948-5953: The CLI stubs (applyBuiltinProxyProvider and
ensureBuiltinProxyForCodexDefault) were changed but other entry points still
start the builtin proxy; locate and modify the code paths that call the proxy,
specifically where "proxy-start" is invoked by the API startup and where
createMcpTools() exposes codexmate.proxy.start, and short-circuit them to return
the same removed-feature error (or remove the export) so the builtin proxy
cannot be launched programmatically; update the proxy-start invocation to check
these stubs (applyBuiltinProxyProvider / ensureBuiltinProxyForCodexDefault) and
propagate their { error: '该功能已移除' } response, and remove/replace the
codexmate.proxy.start export in createMcpTools() to prevent external launches.
🪄 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: 8090f03e-c4a4-4113-b468-7282533171cd
📒 Files selected for processing (6)
README.mdcli.jstests/e2e/test-auth-proxy.jstests/e2e/test-setup.jsweb-ui/app.jsweb-ui/index.html
💤 Files with no reviewable changes (1)
- README.md
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (6)
tests/e2e/test-setup.js (2)
164-164: No actionable issue in this segment.
55-60: Help regression coverage is correctly targeted.Switching to explicit
--helpand asserting removed proxy/auth entries are absent is the right guard for this PR scope.web-ui/index.html (3)
434-436: Bridge hint text update is consistent with feature removal.This copy now matches the reduced Codex-only editable scope without changing runtime behavior.
520-523: Provider card display change is safe and coherent.The “系统” badge wording and URL fallback keep the card readable while preserving existing null-safe rendering.
1832-1834: OpenClaw empty-state hint now reflects current source of truth.The revised message correctly points users to environment/external configuration after built-in surface removal.
web-ui/app.js (1)
567-576: No action required. The backendstatusendpoint already sanitizes the removed builtin proxy provider throughreadConfigOrVirtualDefault(), which appliessanitizeRemovedBuiltinProxyProvider()before returning the config. The UI will never receivecodexmate-proxyfrom the status path, so the hidden-selection boot scenario cannot occur.
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
|
Triggering a full re-review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cli.js (2)
10271-10279:⚠️ Potential issue | 🟠 MajorThe removed proxy runtime is still reachable through
/apiand MCP.
cmdProxy()andapplyBuiltinProxyProvider()now return该功能已移除, but these API actions and MCP tools still callsaveBuiltinProxySettings(),startBuiltinProxyRuntime(), andstopBuiltinProxyRuntime()directly. So the builtin proxy can still be configured and launched programmatically even after this PR “removes” it.Also applies to: 12522-12547
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 10271 - 10279, The CLI still calls saveBuiltinProxySettings, startBuiltinProxyRuntime, and stopBuiltinProxyRuntime for the 'proxy-save-config'/'proxy-start'/'proxy-stop' commands allowing the removed builtin proxy to be configured; update the command dispatch so these cases short-circuit and return the same "该功能已移除" response (or throw a consistent error) instead of invoking saveBuiltinProxySettings/startBuiltinProxyRuntime/stopBuiltinProxyRuntime, and make the same change where MCP tooling or handlers call them (e.g. in applyBuiltinProxyProvider and cmdProxy) to ensure all programmatic entry points uniformly return the removed-feature message and never call the runtime/config functions.
3188-3213:⚠️ Potential issue | 🟠 MajorBuiltin-proxy cleanup still does not fully converge.
After the Line 255 change made reserved-name detection case-insensitive, these cleanup paths still only match the exact lowercase key/value, and
removePersistedBuiltinProxyProviderFromConfig()stops as soon as the provider block is already gone. That means configs like[model_providers.CODEXMATE-PROXY]/model_provider = "CODEXMATE-PROXY"bypass cleanup entirely, and files that already lost the section but still keepmodel_provider = "codexmate-proxy"never get rewritten.readConfigOrVirtualDefault()hides part of this in memory, but directreadConfig()callers such ascmdUseModel()on Line 7937 still see the stale removed provider.Also applies to: 5576-5671
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3188 - 3213, sanitizeRemovedBuiltinProxyProvider currently only matches BUILTIN_PROXY_PROVIDER_NAME in exact case which misses differently-cased persisted keys and values; update this function (and the related removePersistedBuiltinProxyProviderFromConfig logic and callers like cmdUseModel/readConfigOrVirtualDefault if they repeat the check) to perform case-insensitive detection and removal: detect the actual provider key in safeConfig.model_providers by comparing key.toLowerCase() to BUILTIN_PROXY_PROVIDER_NAME.toLowerCase(), delete that actual key from nextProviders, and treat safeConfig.model_provider equality case-insensitively (using .toLowerCase()) when deciding to replace model_provider and model (use readCurrentModels to compute fallbackModel as you do now). Ensure the code rewrites the config even when the section key differed by case so stale model_provider values like "CODEXMATE-PROXY" are normalized/removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cli.js`:
- Around line 10271-10279: The CLI still calls saveBuiltinProxySettings,
startBuiltinProxyRuntime, and stopBuiltinProxyRuntime for the
'proxy-save-config'/'proxy-start'/'proxy-stop' commands allowing the removed
builtin proxy to be configured; update the command dispatch so these cases
short-circuit and return the same "该功能已移除" response (or throw a consistent
error) instead of invoking
saveBuiltinProxySettings/startBuiltinProxyRuntime/stopBuiltinProxyRuntime, and
make the same change where MCP tooling or handlers call them (e.g. in
applyBuiltinProxyProvider and cmdProxy) to ensure all programmatic entry points
uniformly return the removed-feature message and never call the runtime/config
functions.
- Around line 3188-3213: sanitizeRemovedBuiltinProxyProvider currently only
matches BUILTIN_PROXY_PROVIDER_NAME in exact case which misses differently-cased
persisted keys and values; update this function (and the related
removePersistedBuiltinProxyProviderFromConfig logic and callers like
cmdUseModel/readConfigOrVirtualDefault if they repeat the check) to perform
case-insensitive detection and removal: detect the actual provider key in
safeConfig.model_providers by comparing key.toLowerCase() to
BUILTIN_PROXY_PROVIDER_NAME.toLowerCase(), delete that actual key from
nextProviders, and treat safeConfig.model_provider equality case-insensitively
(using .toLowerCase()) when deciding to replace model_provider and model (use
readCurrentModels to compute fallbackModel as you do now). Ensure the code
rewrites the config even when the section key differed by case so stale
model_provider values like "CODEXMATE-PROXY" are normalized/removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d58c4520-f011-45e7-8a00-1e8fd7810612
📒 Files selected for processing (3)
cli.jstests/e2e/test-auth-proxy.jstests/e2e/test-setup.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/test-setup.js
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (6)
tests/e2e/test-auth-proxy.js (6)
180-193: Past review concern addressed.The response shape assertion now properly validates
providerList && Array.isArray(providerList.providers)before calling.some(), with an informative error message that surfacesproviderList.errorif present. The regex check for TOML block removal is correctly structured with multiline mode and handles quoted/unquoted variants.
49-50: LGTM!The relaxed assertion correctly validates the response structure (boolean
runningfield) with proper null-safety via short-circuit evaluation. This aligns with the intent to check response shape rather than asserting the proxy is already running.
105-113: LGTM!Good defensive filesystem assertions verifying auth profiles are stored in the correct
~/.codex/auth-profiles/directory and not written to the legacy~/.codexmate/codex/auth-profiles/path.
154-167: LGTM!Good test setup that injects a legacy
codexmate-proxyprovider block intoconfig.tomlto verify the sanitization logic removes it. The TOML block is well-formed and correctly uses the dynamic proxy port.
169-178: LGTM!Proper proxy-start assertions with informative error messages. The
listenUrlcheck correctly usesString(proxyPort)for safe comparison.
195-204: LGTM!Correctly validates that the removed
proxy-enable-codex-defaultcommand returns an error containing the expected removal message. The type checktypeof enableRes.error === 'string'ensures safe string method invocation.
|
@coderabbitai 如果我直接合并会带来回归性异常吗 |
|
Tip For best results, initiate chat on the files or code changes.
✅ 低风险区域
|
| 场景 | 风险等级 | 说明 |
|---|---|---|
| 全新安装用户 | 🟢 无风险 | 无历史配置需迁移 |
使用 codexmate-proxy 的存量用户 |
🟡 中等 | 配置将被自动迁移，不可逆 |
依赖 proxy/auth CLI 命令的脚本 |
🔴 回归 | 命令已移除，调用直接报错 |
| Web UI 用户 | 🟢 无风险 | 仅为 UI 删减 |
e2e 测试已覆盖核心路径，主流程合并是安全的。若你的用户群中存在通过脚本调用 codexmate proxy 或依赖自动代理启动的场景，建议在发版说明中明确提示该行为变更。
🧠 Learnings used
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
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)
3336-3348:⚠️ Potential issue | 🟠 MajorTemplate reads still bypass the proxy sanitization path.
getConfigTemplate()still reads rawconfig.tomlinstead of going through the sanitized config flow. If an older file still contains[model_providers.codexmate-proxy], this path can re-surface the removed provider in the template editor/MCP response and write it back unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 3336 - 3348, getConfigTemplate currently reads CONFIG_FILE directly via fs.readFileSync, bypassing the app's sanitization and allowing removed providers (e.g., [model_providers.codexmate-proxy]) to reappear; fix by replacing the direct file read with the canonical sanitized config loader/sanitizer used elsewhere (i.e., call the existing config-loading/sanitization function instead of fs.readFileSync) and then pass that sanitized content into normalizeTopLevelConfigWithTemplate so removed proxy providers are never returned or written back.
♻️ Duplicate comments (3)
cli.js (2)
5948-5954:⚠️ Potential issue | 🟠 MajorThe removed proxy is still reachable outside the CLI wrappers.
These stubs only block the direct CLI commands. In the same file,
createWebServer()still handlesproxy-save-config/proxy-start/proxy-stop, andcreateMcpTools()still exposescodexmate.proxy.start/codexmate.proxy.stop, so callers can still manage or launch the removed proxy programmatically.Also applies to: 10793-10796
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5948 - 5954, The CLI stubs block only direct commands but the removed proxy remains reachable via programmatic handlers; update createWebServer() to remove or disable handling for "proxy-save-config", "proxy-start", and "proxy-stop" routes/actions and update createMcpTools() to remove or return an explicit error for the exposed methods codexmate.proxy.start and codexmate.proxy.stop (and any other proxy-related entries) so callers cannot programmatically manage the removed proxy; ensure the handlers and exported method entries reference the same error message as applyBuiltinProxyProvider/ensureBuiltinProxyForCodexDefault to keep behavior consistent.
5576-5671:⚠️ Potential issue | 🟠 MajorPersisted cleanup still leaves stale top-level provider/model fields.
removePersistedBuiltinProxyProviderFromConfig()deletes the provider block, but it never rewrites top-levelmodel_provider/model, and it returns early once the block is already gone. That letsmodel_provider = "codexmate-proxy"survive indefinitely, which still leaks into directreadConfig()callers such ascmdUseModel().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5576 - 5671, The function removePersistedBuiltinProxyProviderFromConfig currently returns early if the provider block is missing, leaving top-level model_provider/model fields pointing to BUILTIN_PROXY_PROVIDER_NAME intact; change the early-return logic so that if config.model_providers[BUILTIN_PROXY_PROVIDER_NAME] is missing you still check and remove top-level fields (config.model_provider and config.model) when they equal BUILTIN_PROXY_PROVIDER_NAME, then rebuild and write the TOML (using readConfig(), writeConfig(), toml.stringify()) to persist the cleaned config; ensure you reference BUILTIN_PROXY_PROVIDER_NAME when comparing and delete the keys from the rebuilt object before calling writeConfig so callers like cmdUseModel() no longer see the stale values.tests/e2e/test-setup.js (1)
55-59:⚠️ Potential issue | 🟡 MinorExercise the explicit help path here.
runSync(node, [cliPath], ...)only checks the no-arg behavior. That can drift from the real help renderer, so these assertions can still pass while the actual help output re-exposes removed entries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/test-setup.js` around lines 55 - 59, The test currently invokes runSync(node, [cliPath], { env }) which only exercises no-arg behavior; update the test to call the explicit help path (e.g., runSync(node, [cliPath, '--help'], { env }) or runSync(node, [cliPath, 'help'], { env })) so helpResult reflects the real help renderer, then keep the same assertions on helpResult.status and helpResult.stdout to ensure removed entries like 'codexmate proxy', 'codexmate auth', and '内建代理' are not present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/app.js`:
- Around line 728-731: When a refresh fails, clear the cached auth list so
providerPillState() can't read stale status: in the error paths of
loadCodexAuthProfiles() (and in any catch around await
this.loadCodexAuthProfiles() shown in the diff), set this.codexAuthProfiles = []
(or null) before returning/propagating the error so the UI will stop showing
`local` as 已登录; update any catch blocks inside loadCodexAuthProfiles() and the
outer catch around await this.loadCodexAuthProfiles() to ensure
codexAuthProfiles is reset on failure.
---
Outside diff comments:
In `@cli.js`:
- Around line 3336-3348: getConfigTemplate currently reads CONFIG_FILE directly
via fs.readFileSync, bypassing the app's sanitization and allowing removed
providers (e.g., [model_providers.codexmate-proxy]) to reappear; fix by
replacing the direct file read with the canonical sanitized config
loader/sanitizer used elsewhere (i.e., call the existing
config-loading/sanitization function instead of fs.readFileSync) and then pass
that sanitized content into normalizeTopLevelConfigWithTemplate so removed proxy
providers are never returned or written back.
---
Duplicate comments:
In `@cli.js`:
- Around line 5948-5954: The CLI stubs block only direct commands but the
removed proxy remains reachable via programmatic handlers; update
createWebServer() to remove or disable handling for "proxy-save-config",
"proxy-start", and "proxy-stop" routes/actions and update createMcpTools() to
remove or return an explicit error for the exposed methods codexmate.proxy.start
and codexmate.proxy.stop (and any other proxy-related entries) so callers cannot
programmatically manage the removed proxy; ensure the handlers and exported
method entries reference the same error message as
applyBuiltinProxyProvider/ensureBuiltinProxyForCodexDefault to keep behavior
consistent.
- Around line 5576-5671: The function
removePersistedBuiltinProxyProviderFromConfig currently returns early if the
provider block is missing, leaving top-level model_provider/model fields
pointing to BUILTIN_PROXY_PROVIDER_NAME intact; change the early-return logic so
that if config.model_providers[BUILTIN_PROXY_PROVIDER_NAME] is missing you still
check and remove top-level fields (config.model_provider and config.model) when
they equal BUILTIN_PROXY_PROVIDER_NAME, then rebuild and write the TOML (using
readConfig(), writeConfig(), toml.stringify()) to persist the cleaned config;
ensure you reference BUILTIN_PROXY_PROVIDER_NAME when comparing and delete the
keys from the rebuilt object before calling writeConfig so callers like
cmdUseModel() no longer see the stale values.
In `@tests/e2e/test-setup.js`:
- Around line 55-59: The test currently invokes runSync(node, [cliPath], { env
}) which only exercises no-arg behavior; update the test to call the explicit
help path (e.g., runSync(node, [cliPath, '--help'], { env }) or runSync(node,
[cliPath, 'help'], { env })) so helpResult reflects the real help renderer, then
keep the same assertions on helpResult.status and helpResult.stdout to ensure
removed entries like 'codexmate proxy', 'codexmate auth', and '内建代理' are not
present.
🪄 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: 5d721380-265d-447a-a237-5431122f8744
📒 Files selected for processing (6)
README.mdcli.jstests/e2e/test-auth-proxy.jstests/e2e/test-setup.jsweb-ui/app.jsweb-ui/index.html
💤 Files with no reviewable changes (1)
- README.md
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
🔇 Additional comments (9)
web-ui/app.js (1)
567-576: Proxy-specific UI helpers are now aligned with the removal.Only
localremains special-cased, and legacycodexmate-proxyentries stay hidden from the visible provider list. That keeps the removed surface from leaking back into the UI.Also applies to: 3811-3869
web-ui/index.html (4)
434-436: LGTM!The bridge hint text correctly removes the legacy auth/proxy reference while preserving the template editing note. The computed property
activeProviderBridgeHintfromconfig-mode.computed.mjsproperly provides the mode-specific prefix.
519-521: LGTM!The badge terminology change from "内建" to "系统" appropriately reflects the removal of the built-in proxy concept while still clearly indicating read-only provider status.
522-524: LGTM!The card subtitle simplification correctly removes the conditional proxy description logic, now consistently displaying the provider URL with an appropriate fallback message.
1832-1834: LGTM!The empty state hint correctly removes references to built-in providers and auth profiles, replacing them with the more accurate "环境变量或外部配置" which reflects the actual configuration sources.
tests/e2e/test-auth-proxy.js (4)
49-50: Reasonable contract check before the explicit proxy start.This keeps the precondition here scoped to the
proxy-statusresponse shape and leaves the running-state verification to the laterproxy-startflow.
105-113: Good regression guard on the new auth-profile storage path.This pins the uploaded profile and registry under
~/.codex/auth-profilesand makes sure the removed legacy tree stays untouched.
154-193: Nice cleanup-safeguard coverage.Seeding a persisted
codexmate-proxyblock and then assertinglistremoves it from both the returned provider set andconfig.tomlexercises the exact cleanup path this PR is protecting.
195-204: Good lock-down of the removed proxy command.Checking for the explicit removal error keeps this surface unavailable while the rest of the proxy e2e flow still runs through the supported path.
| try { | ||
| await Promise.all([ | ||
| this.loadCodexAuthProfiles(), | ||
| this.loadProxyStatus() | ||
| ]); | ||
| await this.loadCodexAuthProfiles(); | ||
| } catch (e) { | ||
| // 认证/代理状态加载失败不阻塞主界面 | ||
| // 认证状态加载失败不阻塞主界面 |
There was a problem hiding this comment.
Clear stale auth state when this refresh fails.
providerPillState() uses codexAuthProfiles to decide whether local is shown as 已登录. After this new loadAll() refresh path runs once successfully, any later list-auth-profiles failure will leave the old array in place, so the UI can keep showing a stale logged-in state. Please reset codexAuthProfiles in the error paths of loadCodexAuthProfiles() so a failed refresh does not preserve cached auth status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/app.js` around lines 728 - 731, When a refresh fails, clear the cached
auth list so providerPillState() can't read stale status: in the error paths of
loadCodexAuthProfiles() (and in any catch around await
this.loadCodexAuthProfiles() shown in the diff), set this.codexAuthProfiles = []
(or null) before returning/propagating the error so the UI will stop showing
`local` as 已登录; update any catch blocks inside loadCodexAuthProfiles() and the
outer catch around await this.loadCodexAuthProfiles() to ensure
codexAuthProfiles is reset on failure.
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
|
Triggering a full re-review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/coderabbit-review.yml:
- Around line 39-46: The loop parsing markerRegex currently trusts any comment;
restrict it to only workflow-authored comments by checking the comment author
before accepting the marker. In the for loop over comments (where markerRegex
and previousCommitCount are used), add a guard that ensures comment.user?.login
=== 'github-actions[bot]' (or another specific workflow bot login your workflows
use) and/or comment.user?.type === 'Bot' before running match; only then parse
match and set previousCommitCount. This ensures only comments created by the
workflow actor are considered.
🪄 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: e435e015-104e-491e-bc35-3ec3da77ad8a
📒 Files selected for processing (4)
.github/workflows/coderabbit-review.ymltests/e2e/run.jstests/e2e/test-auth-proxy.jstests/unit/coderabbit-workflows.test.mjs
💤 Files with no reviewable changes (2)
- tests/e2e/run.js
- tests/e2e/test-auth-proxy.js
📜 Review details
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
📚 Learning: 2026-03-31T12:37:50.988Z
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Applied to files:
tests/unit/coderabbit-workflows.test.mjs.github/workflows/coderabbit-review.yml
📚 Learning: 2026-03-30T04:47:15.339Z
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Applied to files:
tests/unit/coderabbit-workflows.test.mjs.github/workflows/coderabbit-review.yml
📚 Learning: 2026-03-29T02:42:32.761Z
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Applied to files:
.github/workflows/coderabbit-review.yml
🔇 Additional comments (1)
tests/unit/coderabbit-workflows.test.mjs (1)
21-31: Good coverage for the new re-review gating behavior.These assertions correctly lock in commit-count gating and the ASCII-safe command/message expectations.
| for (const comment of comments.slice().reverse()) { | ||
| const match = comment.body?.match(markerRegex); | ||
| if (!match) { | ||
| continue; | ||
| } | ||
|
|
||
| previousCommitCount = Number(match[1]); | ||
| break; |
There was a problem hiding this comment.
Filter marker parsing to workflow-authored comments only.
The skip gate currently trusts any PR comment containing the marker, so a non-workflow comment can spoof previousCommitCount and suppress the re-review trigger.
Proposed hardening patch
let previousCommitCount = null;
for (const comment of comments.slice().reverse()) {
+ if (comment.user?.login !== "github-actions[bot]") {
+ continue;
+ }
const match = comment.body?.match(markerRegex);
if (!match) {
continue;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/coderabbit-review.yml around lines 39 - 46, The loop
parsing markerRegex currently trusts any comment; restrict it to only
workflow-authored comments by checking the comment author before accepting the
marker. In the for loop over comments (where markerRegex and previousCommitCount
are used), add a guard that ensures comment.user?.login ===
'github-actions[bot]' (or another specific workflow bot login your workflows
use) and/or comment.user?.type === 'Bot' before running match; only then parse
match and set previousCommitCount. This ensures only comments created by the
workflow actor are considered.
|
@coderabbitai 如果我直接合并会带来回归性异常吗 |
|
Tip For best results, initiate chat on the files or code changes.
✅ 低风险区域
|
| 场景 | 风险等级 | 说明 |
|---|---|---|
| 全新安装用户 | 🟢 无风险 | 无历史配置需迁移 |
使用 codexmate-proxy 的存量用户 |
🟡 中等 | 配置将被自动迁移，不可逆 |
依赖 proxy/auth CLI 命令的脚本 |
🔴 回归 | 命令已移除，调用直接报错 |
| Web UI 用户 | 🟢 无风险 | 仅为 UI 删减 |
结论：e2e 测试已覆盖核心路径，对于全新安装或未使用内建代理的用户，直接合并是安全的。若你的用户中存在通过脚本调用 codexmate proxy 或依赖自动代理启动的场景，建议在发版说明（Release Notes）中明确说明该行为变更，以便用户提前知悉。
🧠 Learnings used
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
.github/workflows/coderabbit-review.yml (1)
39-46:⚠️ Potential issue | 🟠 MajorRestrict marker parsing to workflow-authored bot comments.
At Line 39, the parser trusts any PR comment body containing the marker. A user comment can spoof
previousCommitCountand suppress the re-review trigger.Proposed hardening patch
let previousCommitCount = null; for (const comment of comments.slice().reverse()) { + if (comment.user?.type !== "Bot" || comment.user?.login !== "github-actions[bot]") { + continue; + } const match = comment.body?.match(markerRegex); if (!match) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/coderabbit-review.yml around lines 39 - 46, The loop that parses PR comments (for ... comments.slice().reverse()) currently accepts any comment containing markerRegex which lets users spoof previousCommitCount; restrict parsing to workflow-authored bot comments by adding a guard before matching: check comment.user?.type === 'Bot' and comment.user?.login equals the bot username (make the bot login configurable via env var if needed) and only then run comment.body?.match(markerRegex) and set previousCommitCount from the match; target the loop where comments, markerRegex and previousCommitCount are used.cli.js (2)
5948-5953:⚠️ Potential issue | 🟠 MajorThe proxy is still startable through the API and MCP tools.
Lines 5948-5953 only block the CLI wrapper.
/apistill dispatchesproxy-start/proxy-stop/proxy-save-config, andcreateMcpTools()still exposescodexmate.proxy.start/codexmate.proxy.stop/codexmate.proxy.provider.apply, so the removed runtime can still be configured and launched programmatically.Those entry points should return the same
{ error: '该功能已移除' }result as these stubs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5948 - 5953, Handlers and exposed programmatic APIs still allow starting/configuring the removed proxy; update the API dispatchers for "proxy-start", "proxy-stop", and "proxy-save-config" and the programmatic surfaces created by createMcpTools() (specifically codexmate.proxy.start, codexmate.proxy.stop, and codexmate.proxy.provider.apply) to return the same sentinel object { error: '该功能已移除' } instead of performing any runtime actions—locate the route/command dispatch code that maps those action names and the createMcpTools() implementation and replace their operational behaviour with a simple immediate return of the same error object.
5576-5671:⚠️ Potential issue | 🟠 MajorPersisted cleanup still leaves stale top-level provider/model references.
When this helper removes the
[model_providers.codexmate-proxy]block, it never rewritesmodel_provider/model. Line 5588 then short-circuits future cleanup once the section is gone, so a file can keepmodel_provider = "codexmate-proxy"indefinitely. PlainreadConfig()callers such ascmdUseModel()will still read the removed provider from disk.Please apply the same fallback-selection logic from
sanitizeRemovedBuiltinProxyProvider()before writing the file back.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 5576 - 5671, removePersistedBuiltinProxyProviderFromConfig removes the provider section but never updates top-level model selection, leaving model_provider/model pointing at the removed BUILTIN_PROXY_PROVIDER_NAME; update the rebuilt config before writing by reusing the same fallback-selection logic from sanitizeRemovedBuiltinProxyProvider(): after deleting rebuilt.model_providers[BUILTIN_PROXY_PROVIDER_NAME] compute and set rebuilt.model_provider (and rebuilt.model if present) to a valid fallback (using the same helper/path calculus used in sanitizeRemovedBuiltinProxyProvider), then proceed to stringify and write via writeConfig; ensure you reference removePersistedBuiltinProxyProviderFromConfig, sanitizeRemovedBuiltinProxyProvider, BUILTIN_PROXY_PROVIDER_NAME, model_providers, model_provider, model, readConfig and writeConfig when making the change.web-ui/app.js (1)
728-731:⚠️ Potential issue | 🟡 MinorClear
codexAuthProfileswhen this startup refresh fails.This path still preserves the previous auth cache on
list-auth-profilesfailures, soproviderPillState()can keep renderinglocalas已登录after the refresh has already failed. Reset the array in the API-error and exception paths before returning.💡 Minimal fix
async loadCodexAuthProfiles(options = {}) { const silent = !!options.silent; try { const res = await api('list-auth-profiles'); if (res && res.error) { + this.codexAuthProfiles = []; if (!silent) { this.showMessage(res.error, 'error'); } return; } const list = Array.isArray(res && res.profiles) ? res.profiles : []; this.codexAuthProfiles = list.sort((a, b) => { if (!!a.current !== !!b.current) { return a.current ? -1 : 1; } return String(a.name || '').localeCompare(String(b.name || '')); }); } catch (e) { + this.codexAuthProfiles = []; if (!silent) { this.showMessage('读取认证列表失败', 'error'); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/app.js` around lines 728 - 731, The startup refresh failure leaves stale auth state; update loadCodexAuthProfiles to clear the in-memory cache by setting codexAuthProfiles = [] whenever the API call fails or an exception is caught (both in the API-error path that returns early and in the catch block around await this.loadCodexAuthProfiles()), so providerPillState() no longer shows a lingering "已登录" local state after a refresh error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 3188-3214: The sanitization currently checks for the exact
BUILTIN_PROXY_PROVIDER_NAME string and deletes that literal key, which misses
case-variants (e.g., "CodexMate-Proxy"); update
sanitizeRemovedBuiltinProxyProvider to use the case-insensitive matcher
isBuiltinProxyProvider() for both detecting the current provider and when
removing provider keys: set currentIsRemovedBuiltin =
isBuiltinProxyProvider(currentProvider), and build nextProviders by copying only
those entries whose key does NOT satisfy isBuiltinProxyProvider(key) (i.e.,
filter Object.keys(providers) with isBuiltinProxyProvider to exclude all
case-variants), then proceed to compute fallbackProvider/fallbackModel and the
returned fields as before using these filtered providers.
---
Duplicate comments:
In @.github/workflows/coderabbit-review.yml:
- Around line 39-46: The loop that parses PR comments (for ...
comments.slice().reverse()) currently accepts any comment containing markerRegex
which lets users spoof previousCommitCount; restrict parsing to
workflow-authored bot comments by adding a guard before matching: check
comment.user?.type === 'Bot' and comment.user?.login equals the bot username
(make the bot login configurable via env var if needed) and only then run
comment.body?.match(markerRegex) and set previousCommitCount from the match;
target the loop where comments, markerRegex and previousCommitCount are used.
In `@cli.js`:
- Around line 5948-5953: Handlers and exposed programmatic APIs still allow
starting/configuring the removed proxy; update the API dispatchers for
"proxy-start", "proxy-stop", and "proxy-save-config" and the programmatic
surfaces created by createMcpTools() (specifically codexmate.proxy.start,
codexmate.proxy.stop, and codexmate.proxy.provider.apply) to return the same
sentinel object { error: '该功能已移除' } instead of performing any runtime
actions—locate the route/command dispatch code that maps those action names and
the createMcpTools() implementation and replace their operational behaviour with
a simple immediate return of the same error object.
- Around line 5576-5671: removePersistedBuiltinProxyProviderFromConfig removes
the provider section but never updates top-level model selection, leaving
model_provider/model pointing at the removed BUILTIN_PROXY_PROVIDER_NAME; update
the rebuilt config before writing by reusing the same fallback-selection logic
from sanitizeRemovedBuiltinProxyProvider(): after deleting
rebuilt.model_providers[BUILTIN_PROXY_PROVIDER_NAME] compute and set
rebuilt.model_provider (and rebuilt.model if present) to a valid fallback (using
the same helper/path calculus used in sanitizeRemovedBuiltinProxyProvider), then
proceed to stringify and write via writeConfig; ensure you reference
removePersistedBuiltinProxyProviderFromConfig,
sanitizeRemovedBuiltinProxyProvider, BUILTIN_PROXY_PROVIDER_NAME,
model_providers, model_provider, model, readConfig and writeConfig when making
the change.
In `@web-ui/app.js`:
- Around line 728-731: The startup refresh failure leaves stale auth state;
update loadCodexAuthProfiles to clear the in-memory cache by setting
codexAuthProfiles = [] whenever the API call fails or an exception is caught
(both in the API-error path that returns early and in the catch block around
await this.loadCodexAuthProfiles()), so providerPillState() no longer shows a
lingering "已登录" local state after a refresh error.
🪄 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: cea40ca3-064b-467d-84a9-0a6dab2a68ba
📒 Files selected for processing (9)
.github/workflows/coderabbit-review.ymlREADME.mdcli.jstests/e2e/run.jstests/e2e/test-auth-proxy.jstests/e2e/test-setup.jstests/unit/coderabbit-workflows.test.mjsweb-ui/app.jsweb-ui/index.html
💤 Files with no reviewable changes (3)
- README.md
- tests/e2e/run.js
- tests/e2e/test-auth-proxy.js
📜 Review details
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
📚 Learning: 2026-03-31T12:37:50.988Z
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:50.988Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Applied to files:
tests/unit/coderabbit-workflows.test.mjs.github/workflows/coderabbit-review.ymlcli.js
📚 Learning: 2026-03-30T04:47:15.339Z
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.339Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Applied to files:
tests/unit/coderabbit-workflows.test.mjs.github/workflows/coderabbit-review.ymlcli.js
📚 Learning: 2026-03-29T02:42:32.761Z
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.761Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Applied to files:
.github/workflows/coderabbit-review.ymlcli.js
🔇 Additional comments (6)
.github/workflows/coderabbit-review.yml (1)
31-36: Good reliability improvements in pagination and idempotent gating.Using
github.paginate(...)and thepreviousCommitCount === pr.commitsearly return is a solid way to avoid duplicate bot comments.Also applies to: 49-54, 62-67
tests/unit/coderabbit-workflows.test.mjs (1)
21-30: Test updates look consistent with workflow changes.Coverage now checks the pagination path, commit-count gate, and ASCII-safe command content as expected.
tests/e2e/test-setup.js (1)
55-59: Solid regression coverage for removed CLI surfaces.The assertions correctly verify that
codexmate proxy,codexmate auth, and内建代理are no longer exposed in the CLI help output, aligning with the PR's removal of legacy proxy/auth features.web-ui/index.html (3)
434-436: Text update correctly reflects the removal of auth/proxy editability.The simplified wording accurately describes what remains editable after the legacy surfaces are removed.
519-524: Provider card updates align with the removal of built-in proxy semantics.Renaming the badge from "内建" to "系统" and simplifying the subtitle logic are appropriate changes given that the built-in proxy distinction no longer applies.
1831-1834: Empty-state wording now accurately reflects the available configuration sources.The updated text ("环境变量或外部配置") correctly describes how providers can be configured after the removal of built-in provider/auth profile features.
| function sanitizeRemovedBuiltinProxyProvider(config) { | ||
| const safeConfig = isPlainObject(config) ? config : {}; | ||
| const providers = isPlainObject(safeConfig.model_providers) ? safeConfig.model_providers : null; | ||
| const currentProvider = typeof safeConfig.model_provider === 'string' ? safeConfig.model_provider.trim() : ''; | ||
| const hasRemovedBuiltin = !!(providers && providers[BUILTIN_PROXY_PROVIDER_NAME]); | ||
| const currentIsRemovedBuiltin = currentProvider === BUILTIN_PROXY_PROVIDER_NAME; | ||
|
|
||
| if (!hasRemovedBuiltin && !currentIsRemovedBuiltin) { | ||
| return safeConfig; | ||
| } | ||
|
|
||
| const nextProviders = providers ? { ...providers } : {}; | ||
| delete nextProviders[BUILTIN_PROXY_PROVIDER_NAME]; | ||
| const providerNames = Object.keys(nextProviders); | ||
| const fallbackProvider = providerNames[0] || ''; | ||
| const currentModels = readCurrentModels(); | ||
| const fallbackModel = fallbackProvider | ||
| ? (currentModels[fallbackProvider] || (typeof safeConfig.model === 'string' ? safeConfig.model : '')) | ||
| : ''; | ||
|
|
||
| return { | ||
| ...safeConfig, | ||
| model_providers: nextProviders, | ||
| model_provider: currentIsRemovedBuiltin ? fallbackProvider : safeConfig.model_provider, | ||
| model: currentIsRemovedBuiltin ? fallbackModel : safeConfig.model | ||
| }; | ||
| } |
There was a problem hiding this comment.
Make cleanup honor the new case-insensitive builtin-proxy match.
Lines 3192-3193 still look for the literal codexmate-proxy. After Line 256 made isBuiltinProxyProvider() case-insensitive, a manually edited config like CodexMate-Proxy will now be rejected by add/update/delete paths but will survive this sanitization pass, and the persisted cleanup below has the same gap.
Minimal fix
function sanitizeRemovedBuiltinProxyProvider(config) {
const safeConfig = isPlainObject(config) ? config : {};
const providers = isPlainObject(safeConfig.model_providers) ? safeConfig.model_providers : null;
const currentProvider = typeof safeConfig.model_provider === 'string' ? safeConfig.model_provider.trim() : '';
- const hasRemovedBuiltin = !!(providers && providers[BUILTIN_PROXY_PROVIDER_NAME]);
- const currentIsRemovedBuiltin = currentProvider === BUILTIN_PROXY_PROVIDER_NAME;
+ const removedProviderName = providers
+ ? Object.keys(providers).find((name) => isBuiltinProxyProvider(name))
+ : '';
+ const hasRemovedBuiltin = !!removedProviderName;
+ const currentIsRemovedBuiltin = isBuiltinProxyProvider(currentProvider);
if (!hasRemovedBuiltin && !currentIsRemovedBuiltin) {
return safeConfig;
}
const nextProviders = providers ? { ...providers } : {};
- delete nextProviders[BUILTIN_PROXY_PROVIDER_NAME];
+ if (removedProviderName) {
+ delete nextProviders[removedProviderName];
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function sanitizeRemovedBuiltinProxyProvider(config) { | |
| const safeConfig = isPlainObject(config) ? config : {}; | |
| const providers = isPlainObject(safeConfig.model_providers) ? safeConfig.model_providers : null; | |
| const currentProvider = typeof safeConfig.model_provider === 'string' ? safeConfig.model_provider.trim() : ''; | |
| const hasRemovedBuiltin = !!(providers && providers[BUILTIN_PROXY_PROVIDER_NAME]); | |
| const currentIsRemovedBuiltin = currentProvider === BUILTIN_PROXY_PROVIDER_NAME; | |
| if (!hasRemovedBuiltin && !currentIsRemovedBuiltin) { | |
| return safeConfig; | |
| } | |
| const nextProviders = providers ? { ...providers } : {}; | |
| delete nextProviders[BUILTIN_PROXY_PROVIDER_NAME]; | |
| const providerNames = Object.keys(nextProviders); | |
| const fallbackProvider = providerNames[0] || ''; | |
| const currentModels = readCurrentModels(); | |
| const fallbackModel = fallbackProvider | |
| ? (currentModels[fallbackProvider] || (typeof safeConfig.model === 'string' ? safeConfig.model : '')) | |
| : ''; | |
| return { | |
| ...safeConfig, | |
| model_providers: nextProviders, | |
| model_provider: currentIsRemovedBuiltin ? fallbackProvider : safeConfig.model_provider, | |
| model: currentIsRemovedBuiltin ? fallbackModel : safeConfig.model | |
| }; | |
| } | |
| function sanitizeRemovedBuiltinProxyProvider(config) { | |
| const safeConfig = isPlainObject(config) ? config : {}; | |
| const providers = isPlainObject(safeConfig.model_providers) ? safeConfig.model_providers : null; | |
| const currentProvider = typeof safeConfig.model_provider === 'string' ? safeConfig.model_provider.trim() : ''; | |
| const removedProviderName = providers | |
| ? Object.keys(providers).find((name) => isBuiltinProxyProvider(name)) | |
| : ''; | |
| const hasRemovedBuiltin = !!removedProviderName; | |
| const currentIsRemovedBuiltin = isBuiltinProxyProvider(currentProvider); | |
| if (!hasRemovedBuiltin && !currentIsRemovedBuiltin) { | |
| return safeConfig; | |
| } | |
| const nextProviders = providers ? { ...providers } : {}; | |
| if (removedProviderName) { | |
| delete nextProviders[removedProviderName]; | |
| } | |
| const providerNames = Object.keys(nextProviders); | |
| const fallbackProvider = providerNames[0] || ''; | |
| const currentModels = readCurrentModels(); | |
| const fallbackModel = fallbackProvider | |
| ? (currentModels[fallbackProvider] || (typeof safeConfig.model === 'string' ? safeConfig.model : '')) | |
| : ''; | |
| return { | |
| ...safeConfig, | |
| model_providers: nextProviders, | |
| model_provider: currentIsRemovedBuiltin ? fallbackProvider : safeConfig.model_provider, | |
| model: currentIsRemovedBuiltin ? fallbackModel : safeConfig.model | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.js` around lines 3188 - 3214, The sanitization currently checks for the
exact BUILTIN_PROXY_PROVIDER_NAME string and deletes that literal key, which
misses case-variants (e.g., "CodexMate-Proxy"); update
sanitizeRemovedBuiltinProxyProvider to use the case-insensitive matcher
isBuiltinProxyProvider() for both detecting the current provider and when
removing provider keys: set currentIsRemovedBuiltin =
isBuiltinProxyProvider(currentProvider), and build nextProviders by copying only
those entries whose key does NOT satisfy isBuiltinProxyProvider(key) (i.e.,
filter Object.keys(providers) with isBuiltinProxyProvider to exclude all
case-variants), then proceed to compute fallbackProvider/fallbackModel and the
returned fields as before using these filtered providers.
SurviveM
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
Force-dismissed by maintainer to unblock PR.
|
@coderabbitai 当前我就是不需要代理跟鉴权命令 如果我直接合并会带来回归性异常吗 |
|
Tip For best results, initiate chat on the files or code changes.
✅ 对你的场景：可以安全合并
唯一需留意的前提如果你的存量用户中有人曾经配置过 结论：在你不需要代理与鉴权命令的前提下，直接合并是安全的，不会带来回归性异常。 🧠 Learnings used |
awsl233777
left a comment
awsl233777
left a comment
There was a problem hiding this comment.
Approving for force-merge per maintainer instruction.
3 participants
Summary
Tests
Summary by CodeRabbit
Removed Features
Documentation
Bug Fixes / Reliability
Tests / CI