Fix provider base URL defaults and local auth header support - #1945
GautamBytes wants to merge 9 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🪛 ast-grep (0.44.1)src/utils/providerProfiles.test.ts[error] 748-754: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. (prototype-pollution-recursive-merge-typescript) [error] 811-817: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. (prototype-pollution-recursive-merge-typescript) [error] 873-879: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. (prototype-pollution-recursive-merge-typescript) 🪛 Biome (2.5.3)src/utils/providerFlag.ts[error] 428-428: Other switch clauses can erroneously access this declaration. (lint/correctness/noSwitchDeclarations) [error] 429-429: Other switch clauses can erroneously access this declaration. (lint/correctness/noSwitchDeclarations) 🛑 Comments failed to post (2)
🔇 Additional comments (13)
📝 WalkthroughWalkthroughThe change tightens local and remote Ollama route inference, separates local auth-header support from API-format selection, updates runtime and profile routing, and centralizes OpenAI-compatible provider base URL and model defaults with expanded tests. ChangesProvider capability and environment handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/providerProfiles.test.ts`:
- Around line 694-731: Update the test named “local profiles apply auth-header
env without API-format selection” to isolate process environment state by saving
the relevant OPENAI_* variables before applying profiles and restoring them in a
finally block after assertions. Include OPENAI_BASE_URL, OPENAI_AUTH_HEADER,
OPENAI_AUTH_SCHEME, OPENAI_AUTH_HEADER_VALUE, and OPENAI_API_FORMAT so the
undefined assertion and subsequent tests are unaffected.
- Around line 2620-2689: Clean up the CLAUDE_CONFIG_DIR environment variable in
the test’s finally block after removing the temporary directories. Preserve the
existing process.chdir restoration and directory cleanup while ensuring
subsequent tests do not inherit the deleted config path.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e4039ce1-95ee-4fd1-987d-decc155a8a5d
📒 Files selected for processing (5)
src/integrations/index.test.tssrc/integrations/routeMetadata.tssrc/utils/providerFlag.test.tssrc/utils/providerFlag.tssrc/utils/providerProfiles.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/integrations/index.test.tssrc/utils/providerFlag.test.tssrc/integrations/routeMetadata.tssrc/utils/providerProfiles.test.tssrc/utils/providerFlag.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/integrations/index.test.tssrc/utils/providerFlag.test.tssrc/integrations/routeMetadata.tssrc/utils/providerProfiles.test.tssrc/utils/providerFlag.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/integrations/index.test.tssrc/utils/providerFlag.test.tssrc/integrations/routeMetadata.tssrc/utils/providerProfiles.test.tssrc/utils/providerFlag.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/integrations/index.test.tssrc/utils/providerFlag.test.tssrc/integrations/routeMetadata.tssrc/utils/providerProfiles.test.tssrc/utils/providerFlag.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/integrations/index.test.tssrc/utils/providerFlag.test.tssrc/utils/providerProfiles.test.ts
🔇 Additional comments (4)
src/integrations/routeMetadata.ts (1)
871-898: LGTM!src/integrations/index.test.ts (1)
42-47: LGTM!src/utils/providerFlag.ts (1)
376-379: LGTM!Also applies to: 388-391, 402-405, 465-468, 478-481, 504-507
src/utils/providerFlag.test.ts (1)
353-388: LGTM!
jatmn
left a comment
There was a problem hiding this comment.
Findings
[MEDIUM] Custom base URL on Ollama/LM Studio's signature is silently clobbered when switching providers
Files: src/integrations/routeMetadata.ts:99-126 (resolveKnownLocalRouteIdFromBaseUrl), reached via resolveRouteIdFromBaseUrl:953-956; src/utils/providerFlag.ts:176-191 (shouldReplaceStaleKnownBaseUrl), 202-222 (applyOpenAIBaseUrlDefault).
Root cause: The local-route heuristic classifies any URL whose host ends with :11434/:1234 or whose host/path substring-contains ollama/lmstudio/lm-studio as a known local route (port/substring match, not host-boundary). Such a URL resolves to a known route, so shouldReplaceStaleKnownBaseUrl returns true whenever the chosen provider differs, overwriting the user's URL.
Empirical repro (--provider xai with various pre-set OPENAI_BASE_URL):
http://localhost:11434/v1→ CLOBBERED tohttps://api.x.ai/v1https://myollama.example.com/v1→ CLOBBERED tohttps://api.x.ai/v1https://proxy.example.com/v1→ preserved ✅https://api.openai.com/v1→ replaced (intended) ✅
Impact: A user running a custom gateway on Ollama/LM Studio's port (or a similarly-named host) loses that configuration silently when they later select a different --provider. This is config "data loss" and directly contradicts the PR's stated guarantee "preserve custom base URLs that do not resolve to a known provider route" — these DO resolve (falsely) to a known route. This path is newly reachable for the six converted providers (pre-PR ??= never consulted route resolution). The new test (providerFlag.test.ts "preserves a custom unknown base URL") only uses an unrelated host (proxy.example.com), so it misses this.
Recommendation: Tighten resolveKnownLocalRouteIdFromBaseUrl to host-boundary matching (match only localhost/127.0.0.1 exactly, or require the exact defaultBaseUrl), and/or exempt the user's currently configured route from stale-replacement. Add tests for port-11434/1234 and substring hostnames.
[MEDIUM] OPENAI_API_BASE is now consulted; a custom OPENAI_API_BASE is preserved instead of overridden
Files: src/utils/providerFlag.ts:167-174 (getConfiguredOpenAIBaseUrl), 216-221, 176-191.
Behavioral change: Pre-PR OPENAI_BASE_URL ??= default ignored OPENAI_API_BASE. Now getConfiguredOpenAIBaseUrl returns OPENAI_API_BASE when OPENAI_BASE_URL is blank, so a custom OPENAI_API_BASE is preserved and the chosen provider's default is not applied.
- Repro: blank
OPENAI_BASE_URL+OPENAI_API_BASE=https://my-custom-gateway.example.com/v1+--provider ollama→OPENAI_BASE_URLstays unset (falls back to the custom gateway), whereas pre-PR??=forcedhttp://localhost:11434/v1. - A known-provider
OPENAI_API_BASEis still replaced (intended stale-replacement). - The test matrix does not cover the
OPENAI_API_BASE-alias path for the six providers (onlygitlawb-opengateway).
Assessment: Arguably an improvement (respects an explicit OPENAI_API_BASE), but a silent behavioral change that can surprise users who relied on the old override. Recommendation: document the precedence change and add a test.
[LOW] Enabling local supportsAuthHeaders also enables local custom headers (side effect)
Files: src/integrations/routeMetadata.ts:824-833 (routeSupportsCustomHeaders keys on openaiShim.supportsAuthHeaders with no transportKind gate) vs :871-898 (routeSupportsAuthHeaders has an explicit && transportKind === 'local').
Because routeSupportsCustomHeaders reuses the same flag without the local gate, turning on local auth headers also turns on local custom headers. Verified safe in applySupportedProfileCustomHeaders / sanitizeProfileCustomHeaders, and the UI now shows custom-header editing for ollama/lmstudio. Likely intended, but recommend the maintainer confirm and consider applying the same transportKind gate for consistency.
[LOW] Plaintext persistence of auth-header value; no CRLF validation
Files: src/utils/providerProfiles.ts (setActiveProviderProfile persistence writes OPENAI_AUTH_HEADER_VALUE in plaintext into .openclaude-profile.json); authHeaderValue is trimmed but not CRLF-validated.
Assessment: Consistent with existing apiKey storage (file mode 0600); pre-existing gap now reachable for local routes. Worth a note in secret-handling review; not a regression.
[LOW] Test coverage gaps
src/utils/providerFlag.test.ts: no tests fornearai/fireworksbase-URL-default behavior (they also useapplyOpenAIBaseUrlDefault); no test for theOPENAI_API_BASE-blocks-default edge for the six providers.- The new "preserves custom unknown base URL" test uses only an unrelated host, missing the port-11434/substring cases that actually fail (see MEDIUM).
Recommendation: extend coverage.
[INFO] Minor PR description inaccuracy (no functional impact)
xai's pre-PR line was OPENAI_BASE_URL ??= 'https://api.x.ai/v1' (no defaultBaseUrl reference); the other five did reference defaultBaseUrl. The PR body implies all six used defaultBaseUrl ?? '...'. No functional impact — the xai descriptor default equals the hardcoded value (vendors/xai.ts:7).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/integrations/routeMetadata.ts`:
- Around line 101-108: Remove the redundant hostname === '[::1]' branch from
isDefaultLocalRouteHostname, retaining the existing ::1 check and all other
default-hostname checks unchanged.
In `@src/utils/providerFlag.test.ts`:
- Around line 397-414: Split the custom URL coverage in the provider flag tests
into independently reported assertions, preferably using test.each over
customBaseUrls. Preserve the existing applyProviderFlag call and expectations
for every URL pattern while ensuring one failure does not prevent the remaining
cases from running.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 3d646660-7dd6-4420-b4b5-745c8f80bdb4
📒 Files selected for processing (3)
src/integrations/routeMetadata.test.tssrc/integrations/routeMetadata.tssrc/utils/providerFlag.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/integrations/routeMetadata.test.tssrc/utils/providerFlag.test.tssrc/integrations/routeMetadata.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/integrations/routeMetadata.test.tssrc/utils/providerFlag.test.tssrc/integrations/routeMetadata.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/integrations/routeMetadata.test.tssrc/utils/providerFlag.test.tssrc/integrations/routeMetadata.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/integrations/routeMetadata.test.tssrc/utils/providerFlag.test.tssrc/integrations/routeMetadata.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/integrations/routeMetadata.test.tssrc/utils/providerFlag.test.ts
🔇 Additional comments (7)
src/integrations/routeMetadata.ts (2)
110-133: Local route detection logic is sound.The narrowing to
http:protocol + loopback hostnames + specific default ports correctly prevents stale-URL replacement for custom or non-loopback endpoints. Thetry/catcharoundnew URLhandles malformed input gracefully.
884-893: Two-flag capability gate is correct.
supportsApiFormatSelectionis properly restricted toopenai-compatibletransport, whilesupportsAuthHeadersis additionally enabled forlocaltransport. The descriptor-missing guard and the fallthroughreturn falseare correct. This aligns with the downstream consumer inproviderProfiles.tswhich conditionally setsOPENAI_AUTH_HEADER/OPENAI_API_FORMATbased on these flags.src/integrations/routeMetadata.test.ts (3)
91-104: Good loopback coverage for both local ports.All three loopback addresses are tested for both Ollama (11434) and LM Studio (1234) ports. The
http://[::1]:11434/v1case correctly validates IPv6 loopback resolution.
106-124: Good negative coverage for lookalike URLs.HTTPS on the same host/port, lookalike domains, path-based resemblance, and
https://localhost:11434all correctly expectnull. This validates the protocol restriction and hostname narrowing.
91-124: 📐 Maintainability & Code QualityNo additional tests needed here —
src/integrations/index.test.tsalready coversrouteSupportsApiFormatSelectionandrouteSupportsAuthHeaders, including the local gateway auth-header path.> Likely an incorrect or invalid review comment.src/utils/providerFlag.test.ts (2)
37-38: LGTM!Also applies to: 76-77
357-430: 🎯 Functional CorrectnessDrop this concern for nearai and fireworks. They already route through
applyOpenAIBaseUrlDefault, so the added stale-default replacement and custom-URL preservation tests apply to them too.> Likely an incorrect or invalid review comment.
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep remote Ollama endpoints on the local route
src/integrations/routeMetadata.ts:119
The narrowed local-route inference only returnsollamaforhttp://localhost/127.0.0.1/::1on port 11434. OpenClaude already supports remote Ollama setups without an API key, though:providerValidation.test.tscovershttp://203.0.113.5:11434/v1,http://my-ollama-server.example.com:11434/v1, andhttps://ollama.corp.example.com/v1as valid Ollama endpoints. With this PR those URLs resolve tonull, so the provider profile path treats them as generic OpenAI-compatible routes and allows/persistsOPENAI_API_FORMAT=responses. I reproduced this with the real profile APIs: loopback Ollama stripsOPENAI_API_FORMAT, buthttp://203.0.113.5:11434/v1andhttps://ollama.corp.example.com/v1both persistOPENAI_API_FORMAT: "responses", andresolveProviderRequest()then choosestransport: "responses"against the Ollama base URL. That breaks remote Ollama profiles even though the PR says local gateways still do not expose API-format selection. Please preserve the new custom-URL protection without losing the existing remote-Ollama route identity for capability/runtime decisions. -
[P2] Reset stale models when replacing a stale provider base URL
src/utils/providerFlag.ts:388
applyOpenAIBaseUrlDefault()now replaces a known staleOPENAI_BASE_URLwhen the user explicitly switches providers, but the provider block still keeps any existingOPENAI_MODELvia??=. That produces a mixed configuration: for example, starting withOPENAI_BASE_URL=https://api.openai.com/v1 OPENAI_MODEL=gpt-5.5 NVIDIA_API_KEY=...and running--provider nvidia-nimnow setsOPENAI_BASE_URL=https://integrate.api.nvidia.com/v1while leavingOPENAI_MODEL=gpt-5.5, so the first request is sent to NVIDIA NIM with an OpenAI model instead of the descriptor defaultnvidia/llama-3.1-nemotron-70b-instruct. The same pattern applies to the other converted routes that replace a stale base URL but leave the old model unless--modelis passed. Before this PR the stale base URL prevented the new defaulting path entirely; after this PR the base switches while the model does not. Please clear or replace the stale model whenever the stale base URL is replaced, while still preserving an explicit--model.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/integrations/routeMetadata.ts`:
- Around line 169-177: Remove the redundant resolveKnownLocalRouteIdFromBaseUrl
call from resolveLocalCompatibleRouteIdFromBaseUrl, leaving
resolveRouteIdFromBaseUrl followed directly by
resolveRemoteOllamaRouteIdFromBaseUrl as the fallback chain.
In `@src/utils/providerFlag.test.ts`:
- Around line 359-397: Extend the parameterized stale URL replacement test data
in providerFlag.test.ts to include the xiaomi-mimo-token provider with its
expected base URL and default model, matching the applyOpenAIModelDefault and
applyOpenAIBaseUrlDefault behavior of xiaomi-mimo. Consider separate coverage
for atlas-cloud only if its special API key handling requires it.
In `@src/utils/providerProfiles.test.ts`:
- Around line 752-792: Update the remote Ollama test.each block around
applyProviderProfileToProcessEnv to save the relevant OPENAI_* environment
variables before mutation and restore them after each test iteration, including
deleting variables that were originally unset. Keep the existing assertions and
request-resolution behavior unchanged, and follow the isolation pattern used by
the neighboring local profiles auth-header test.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 7bcf125a-19eb-45e2-8679-6539f1ff5406
📒 Files selected for processing (10)
src/integrations/index.tssrc/integrations/routeMetadata.test.tssrc/integrations/routeMetadata.tssrc/integrations/runtimeMetadata.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/utils/providerFlag.test.tssrc/utils/providerFlag.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/integrations/index.tssrc/services/api/providerConfig.local.test.tssrc/integrations/runtimeMetadata.tssrc/integrations/routeMetadata.test.tssrc/utils/providerFlag.test.tssrc/services/api/providerConfig.tssrc/utils/providerProfiles.tssrc/utils/providerProfiles.test.tssrc/integrations/routeMetadata.tssrc/utils/providerFlag.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/integrations/index.tssrc/services/api/providerConfig.local.test.tssrc/integrations/runtimeMetadata.tssrc/integrations/routeMetadata.test.tssrc/utils/providerFlag.test.tssrc/services/api/providerConfig.tssrc/utils/providerProfiles.tssrc/utils/providerProfiles.test.tssrc/integrations/routeMetadata.tssrc/utils/providerFlag.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/integrations/index.tssrc/services/api/providerConfig.local.test.tssrc/integrations/runtimeMetadata.tssrc/integrations/routeMetadata.test.tssrc/utils/providerFlag.test.tssrc/services/api/providerConfig.tssrc/utils/providerProfiles.tssrc/utils/providerProfiles.test.tssrc/integrations/routeMetadata.tssrc/utils/providerFlag.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/integrations/index.tssrc/services/api/providerConfig.local.test.tssrc/integrations/runtimeMetadata.tssrc/integrations/routeMetadata.test.tssrc/utils/providerFlag.test.tssrc/services/api/providerConfig.tssrc/utils/providerProfiles.tssrc/utils/providerProfiles.test.tssrc/integrations/routeMetadata.tssrc/utils/providerFlag.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/api/providerConfig.local.test.tssrc/integrations/routeMetadata.test.tssrc/utils/providerFlag.test.tssrc/utils/providerProfiles.test.ts
🔇 Additional comments (13)
src/utils/providerFlag.ts (4)
239-259: LGTM!
426-442: LGTM! Provider cases consistently route throughapplyOpenAIBaseUrlDefault+applyOpenAIModelDefault. Thecloudflaretoken-mirroring guard (L651-667) correctly prevents leakingCLOUDFLARE_API_TOKENto non-Cloudflare or placeholder endpoints.Also applies to: 444-475, 502-529, 539-606, 620-624, 637-641, 672-676
261-274: 🎯 Functional CorrectnessNo issue:
OPENAI_MODELstill defaults on fresh Ollama runs.applyOpenAIModelDefaultuses??=when no stale base URL was replaced, so the fresh-start/custom-URL path still gets the route default model;resetOpenAIModelDefaultWhenStaleBaseUrlReplacedonly covers the stale-base-URL case.> Likely an incorrect or invalid review comment.
227-234: 🗄️ Data Integrity & IntegrationNo change needed.
OPENAI_BASE_URLalready takes precedence on the relevant code paths, so leavingOPENAI_API_BASEset here does not change endpoint selection.> Likely an incorrect or invalid review comment.src/utils/providerFlag.test.ts (1)
37-38: LGTM!ENV_KEYS/RESET_KEYSadditions ensure clean state forNEARAI_API_KEYandFIREWORKS_API_KEY. The parameterized stale-URL-replacement tests cover both model reset and explicit model preservation.Also applies to: 76-77, 418-443
src/integrations/routeMetadata.ts (2)
142-167: 🎯 Functional CorrectnessBroad
http:+ port11434match applies to any hostname, not just Ollama-like ones.
resolveRemoteOllamaRouteIdFromBaseUrltreats any plain-HTTP URL on port11434as'ollama'regardless of hostname (only the port is checked in that branch). This is clearly intentional for bare-IP tunneled/proxied Ollama instances (matches the203.0.113.5:11434test case), and 11434 is indeed Ollama's well-known default port, commonly exposed via reverse proxies/tunnels to public hosts. Still, this means any unrelated HTTP service that happens to reuse port 11434 gets silently routed/gated as Ollama (forcingchat_completions-only transport, stripping API-format selection, enabling auth-header capability) downstream inproviderConfig.tsandproviderProfiles.ts. Worth a quick confirmation that this tradeoff is accepted, since path instructions call for high scrutiny on hardcoded provider assumptions in routing code.
1061-1061: LGTM!Also applies to: 1082-1084, 1117-1119
src/integrations/index.ts (1)
144-144: LGTM!src/integrations/routeMetadata.test.ts (1)
12-12: LGTM!Also applies to: 92-156
src/integrations/runtimeMetadata.ts (1)
24-24: LGTM!Also applies to: 273-275
src/services/api/providerConfig.ts (1)
961-964: LGTM!src/services/api/providerConfig.local.test.ts (1)
173-193: LGTM!src/utils/providerProfiles.ts (1)
45-45: LGTM!Also applies to: 216-216
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Rebase this branch before merging; it restores several already-merged mainline fixes.
src/cli/update.ts:160
The PR head is based on4f971a13, whilemaincontains fixes this branch does not: #1944, #1934, #1933, #1932, #1927, and #1917. This makes the provider change overwrite unrelated current behavior and its tests. In particular, the package-manager branch now unconditionally printsbrew upgrade claude-code,winget upgrade Anthropic.ClaudeCode, orapk upgrade claude-codefor an OpenClaude installation. Those commands target the upstream package rather than@gitlawb/openclaude, so following/update,openclaude update, or the passive update notice can update an unrelated product instead of OpenClaude. Rebase onto currentmainand retain the product-aware update guidance rather than carrying these reverts. -
[P1] Preserve the complete default value when it contains
:-.
src/services/mcp/envExpansion.ts:18
String.split(':-', 2)discards everything after the second delimiter. Consequently, an unset${VAR:-a:-b}now expands toa, rather than the requireda:-b. This silently corrupts valid MCP server environment values and command arguments; use the prior first-delimiterindexOf/slicebehavior and restore its regression coverage. -
[P1] Restore envelope timestamp ordering in relevance pruning.
src/utils/relevancePruning.ts:107
Messagechronology lives in the envelope'stimestamp, but this reverts the code tomessage.message.created_at, which normal user/system/attachment messages do not populate. The fallback therefore makes all retained messages epoch-old: recency scoring and tie-breaking disappear, andpruneByRelevancecan return preserved recent messages before retained older tool rounds. That can send a reordered conversation (including a tool result before its tool use) to the provider. Use the envelope timestamp for scoring and both sorts. -
[P2] Keep the tool-failure diagnostic sanitization.
src/query/toolFailureLoopGuard.ts:440
The stopping message again interpolates model/MCP-controlled tool names, paths, and fallback error text directly into terminal and transcript output. A repeated failure with a path or tool name containing control characters, ANSI escapes, newlines, or backticks can spoof the rendered diagnostic or inject terminal control text. Restore the prior allow-list/control-character sanitization before formatting these values. -
[P2] Number edited-file snippets from the new-file hunk position.
src/tools/FileEditTool/utils.ts:385
The attachment filters deletions and displays new-file lines, but this usesoldStartagain. After an earlier hunk changes the line count, every later snippet is offset by the cumulative delta (for example, a new line 58 is reported as old line 55), so follow-up model or user edits are directed at the wrong location. UsenewStartand restore the multi-hunk regression test. -
[P2] Enforce the diff limit in UTF-8 bytes.
src/utils/gitDiff.ts:214
The advertised 1 MiB cap now uses JavaScript UTF-16 code-unit length. A CJK or emoji-heavy repository diff can be three to four times the byte limit while still passing this check, after which it is fully parsed and retained for context consumers. RestoreBuffer.byteLength(fileDiff, 'utf8')so untrusted diffs cannot bypass the resource bound. -
[P2] Clean up the temporary CLI-test roots and retain a child-process watchdog.
src/entrypoints/cli.skills.test.ts:18
runSkillsList()creates a fresh/tmp/openclaude-skills-cli-*root for every invocation but no longer removes it, and the--add-dirtest likewise leaves its own root behind. The removed timeout/kill handling also lets a hung spawned CLI wait for the outer test timeout and leave its process running. Reintroduce thefinallycleanup and bounded child-process handling so repeated CI runs do not accumulate files or orphan children.
873564d to
085fea1
Compare
jatmn
left a comment
There was a problem hiding this comment.
please rebase on main and fix merge issues.
…olvers Addresses maintainer review of provider routing PR: MEDIUM-1: hostnameContainsRouteToken now requires exact dot-label match (no dash-split sub-token), so ollama-corp / my-ollama no longer match. Dropped host-agnostic :11434 port rule from the remote matcher — only loopback :11434 (strict resolver) classifies as Ollama, keeping vLLM / LM Studio tunnels from inheriting Ollama's local-shim behavior. MEDIUM-2: shouldReplaceStaleKnownBaseUrl now routes through resolveLocalCompatibleRouteIdFromBaseUrl (aligned with the runtime capability path), so remote Ollama URLs identified by hostname token are also replaced on provider switch instead of leaving the new provider's traffic routed at the old host. LOW-1: applyOpenAIBaseUrlDefault now clears OPENAI_API_BASE when it was the stale source being replaced, so the alias no longer lingers alongside the new OPENAI_BASE_URL. LOW-2: ollama case now uses applyOpenAIModelDefault (with ?= fresh selection) like the other descriptor-backed providers; removed the unused resetOpenAIModelDefaultWhenStaleBaseUrlReplaced helper. LOW-3: buildOpenAICompatibleStartupEnv now gates OPENAI_API_FORMAT serialization on routeSupportsApiFormatSelection at the persistence layer, matching the runtime gate already applied by applyProviderProfileToProcessEnv. LOW-4: the new profile tests' finally blocks now also restore OPENAI_MODEL, CLAUDE_CODE_USE_OPENAI, and the *_APPLIED flags so they are self-contained instead of relying solely on shared afterEach. LOW-5: added loopback-port cross-provider replacement tests covering ollama :11434 and lmstudio :1234 (loopback) and a remote hostname-token form, replacing the over-broad PR-description contract with verified behavior.
e380cca to
727f211
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/providerFlag.ts (1)
693-702: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not retain a stale provider route when Cloudflare’s default is unresolved.
If
OPENAI_BASE_URLpoints to a known provider such as OpenAI, the placeholder Cloudflare default is skipped, leaving requests routed to the stale host while potentially applying a Cloudflare model. Return a setup error or clear the stale route until a concrete account-scoped URL is configured, and add this regression case.As per path instructions, block “silent default changes” and test provider base URL routing explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/providerFlag.ts` around lines 693 - 702, Update the cloudflare branch in the provider flag handling to detect when applyOpenAIBaseUrlDefault leaves an unresolved or stale OPENAI_BASE_URL instead of routing requests to it with a Cloudflare model. Return a setup error or clear the stale route until a concrete account-scoped URL is configured, while preserving valid explicit Cloudflare URLs. Add a regression test that explicitly verifies provider base URL routing and prevents silent default changes.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api/client.test.ts`:
- Around line 1-2: Move the provider mock.module registrations from top-level
initialization into this test file’s beforeEach setup, alongside the shared
mutation lock acquisition. Ensure both provider registrations are re-established
under the lock before client.js is imported, while preserving the existing
afterEach cleanup.
In `@src/utils/providerFlag.ts`:
- Around line 417-429: Block-scope the `custom-anthropic` case in the
provider-flag switch so its lexical declarations, including `hasAuthToken` and
`hasApiKey`, do not share the switch scope. Wrap the case body in braces while
preserving its existing validation and return behavior.
---
Outside diff comments:
In `@src/utils/providerFlag.ts`:
- Around line 693-702: Update the cloudflare branch in the provider flag
handling to detect when applyOpenAIBaseUrlDefault leaves an unresolved or stale
OPENAI_BASE_URL instead of routing requests to it with a Cloudflare model.
Return a setup error or clear the stale route until a concrete account-scoped
URL is configured, while preserving valid explicit Cloudflare URLs. Add a
regression test that explicitly verifies provider base URL routing and prevents
silent default changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb96523f-f66b-4e3f-899f-7e7437603a17
📒 Files selected for processing (13)
src/integrations/index.test.tssrc/integrations/index.tssrc/integrations/routeMetadata.test.tssrc/integrations/routeMetadata.tssrc/integrations/runtimeMetadata.tssrc/services/api/client.test.tssrc/services/api/openaiShim.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/utils/providerFlag.test.tssrc/utils/providerFlag.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Runbun run typecheckandbun run typecheck:type-testsfor TypeScript changes.
Run provider tests and provider recommendation tests when changing provider behavior:bun run test:providerandbun run test:provider-recommendation.
Files:
src/integrations/index.test.tssrc/integrations/index.tssrc/services/api/providerConfig.tssrc/integrations/runtimeMetadata.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerFlag.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.tssrc/utils/providerProfiles.test.tssrc/integrations/routeMetadata.test.tssrc/services/api/client.test.tssrc/integrations/routeMetadata.tssrc/utils/providerFlag.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Keep changes focused on one problem or feature and avoid mixing unrelated cleanup into the same change.
Preserve existing repository patterns unless intentionally refactoring them.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting them.
Follow the existing code style in touched files.
Prefer small, readable changes over broad rewrites.
Do not reformat unrelated files.
Keep comments useful and concise.
Provider changes must explicitly identify affected providers, limitations, and follow-up work in the pull request description.
Do not assign or use provider tags; provider tags are controlled by maintainers.
Run the relevant validation checks locally before submitting changes; pull requests must pass CI checks.
Runbun run security:pr-scanbefore submitting a pull request.
Dependency changes require a concrete project benefit, such as fixing a bug, addressing a security issue, or supporting an approved feature.
Do not change the project's language, core runtime, or dependency stack without prior maintainer agreement.
Files:
src/integrations/index.test.tssrc/integrations/index.tssrc/services/api/providerConfig.tssrc/integrations/runtimeMetadata.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerFlag.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.tssrc/utils/providerProfiles.test.tssrc/integrations/routeMetadata.test.tssrc/services/api/client.test.tssrc/integrations/routeMetadata.tssrc/utils/providerFlag.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/integrations/index.test.tssrc/integrations/index.tssrc/services/api/providerConfig.tssrc/integrations/runtimeMetadata.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerFlag.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.tssrc/utils/providerProfiles.test.tssrc/integrations/routeMetadata.test.tssrc/services/api/client.test.tssrc/integrations/routeMetadata.tssrc/utils/providerFlag.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when a code change affects behavior.
Files:
src/integrations/index.test.tssrc/integrations/index.tssrc/services/api/providerConfig.tssrc/integrations/runtimeMetadata.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerFlag.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.tssrc/utils/providerProfiles.test.tssrc/integrations/routeMetadata.test.tssrc/services/api/client.test.tssrc/integrations/routeMetadata.tssrc/utils/providerFlag.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/integrations/index.test.tssrc/integrations/index.tssrc/services/api/providerConfig.tssrc/integrations/runtimeMetadata.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerFlag.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.tssrc/utils/providerProfiles.test.tssrc/integrations/routeMetadata.test.tssrc/services/api/client.test.tssrc/integrations/routeMetadata.tssrc/utils/providerFlag.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/integrations/index.test.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerFlag.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/integrations/routeMetadata.test.tssrc/services/api/client.test.ts
🪛 ast-grep (0.44.1)
src/utils/providerProfiles.test.ts
[error] 748-754: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const key of envKeys) {
if (savedEnv[key] === undefined) {
delete process.env[key]
} else {
process.env[key] = savedEnv[key]
}
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
[error] 811-817: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const key of envKeys) {
if (savedEnv[key] === undefined) {
delete process.env[key]
} else {
process.env[key] = savedEnv[key]
}
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
[error] 873-879: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const key of envKeys) {
if (savedEnv[key] === undefined) {
delete process.env[key]
} else {
process.env[key] = savedEnv[key]
}
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
🪛 Biome (2.5.3)
src/utils/providerFlag.ts
[error] 428-428: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 429-429: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (13)
src/utils/providerFlag.ts (1)
18-26: LGTM!Also applies to: 52-69, 144-279, 390-415, 440-685, 733-737
src/utils/providerFlag.test.ts (1)
24-43: LGTM!Also applies to: 66-103, 164-272, 474-657, 735-747
src/integrations/routeMetadata.ts (1)
102-136: LGTM!Also applies to: 143-187, 892-896, 939-950, 1067-1093, 1114-1116, 1149-1151
src/integrations/index.ts (1)
144-144: LGTM!src/integrations/routeMetadata.test.ts (1)
12-12: LGTM!Also applies to: 92-105, 107-126, 127-177, 178-192, 230-287
src/integrations/index.test.ts (1)
40-48: LGTM!src/services/api/openaiShim.test.ts (1)
4114-4144: LGTM!Also applies to: 6987-6991, 7016-7016, 8781-8984
src/utils/providerProfiles.ts (1)
45-45: LGTM!Also applies to: 216-221, 412-421, 461-476, 522-529, 622-625, 871-872, 1218-1219, 1337-1357, 1447-1451, 1676-1677, 1699-1700, 1734-1744
src/integrations/runtimeMetadata.ts (1)
24-24: LGTM!Also applies to: 273-275
src/services/api/providerConfig.ts (1)
961-964: LGTM!src/services/api/providerConfig.local.test.ts (1)
173-216: LGTM!src/services/api/client.test.ts (1)
75-75: LGTM!Also applies to: 122-129, 167-167, 216-217, 255-255, 307-450
src/utils/providerProfiles.test.ts (1)
22-22: LGTM!Also applies to: 49-49, 114-114, 697-884, 2449-2461, 2787-2857, 3515-3552, 3760-3854
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/providerFlag.ts (1)
693-702: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not retain a stale provider route when Cloudflare’s default is unresolved.
If
OPENAI_BASE_URLpoints to a known provider such as OpenAI, the placeholder Cloudflare default is skipped, leaving requests routed to the stale host while potentially applying a Cloudflare model. Return a setup error or clear the stale route until a concrete account-scoped URL is configured, and add this regression case.As per path instructions, block “silent default changes” and test provider base URL routing explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/providerFlag.ts` around lines 693 - 702, Update the cloudflare branch in the provider flag handling to detect when applyOpenAIBaseUrlDefault leaves an unresolved or stale OPENAI_BASE_URL instead of routing requests to it with a Cloudflare model. Return a setup error or clear the stale route until a concrete account-scoped URL is configured, while preserving valid explicit Cloudflare URLs. Add a regression test that explicitly verifies provider base URL routing and prevents silent default changes.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api/client.test.ts`:
- Around line 1-2: Move the provider mock.module registrations from top-level
initialization into this test file’s beforeEach setup, alongside the shared
mutation lock acquisition. Ensure both provider registrations are re-established
under the lock before client.js is imported, while preserving the existing
afterEach cleanup.
In `@src/utils/providerFlag.ts`:
- Around line 417-429: Block-scope the `custom-anthropic` case in the
provider-flag switch so its lexical declarations, including `hasAuthToken` and
`hasApiKey`, do not share the switch scope. Wrap the case body in braces while
preserving its existing validation and return behavior.
---
Outside diff comments:
In `@src/utils/providerFlag.ts`:
- Around line 693-702: Update the cloudflare branch in the provider flag
handling to detect when applyOpenAIBaseUrlDefault leaves an unresolved or stale
OPENAI_BASE_URL instead of routing requests to it with a Cloudflare model.
Return a setup error or clear the stale route until a concrete account-scoped
URL is configured, while preserving valid explicit Cloudflare URLs. Add a
regression test that explicitly verifies provider base URL routing and prevents
silent default changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb96523f-f66b-4e3f-899f-7e7437603a17
📒 Files selected for processing (13)
src/integrations/index.test.tssrc/integrations/index.tssrc/integrations/routeMetadata.test.tssrc/integrations/routeMetadata.tssrc/integrations/runtimeMetadata.tssrc/services/api/client.test.tssrc/services/api/openaiShim.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/utils/providerFlag.test.tssrc/utils/providerFlag.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.ts
📜 Review details
🔇 Additional comments (13)
src/utils/providerFlag.ts (1)
18-26: LGTM!Also applies to: 52-69, 144-279, 390-415, 440-685, 733-737
src/utils/providerFlag.test.ts (1)
24-43: LGTM!Also applies to: 66-103, 164-272, 474-657, 735-747
src/integrations/routeMetadata.ts (1)
102-136: LGTM!Also applies to: 143-187, 892-896, 939-950, 1067-1093, 1114-1116, 1149-1151
src/integrations/index.ts (1)
144-144: LGTM!src/integrations/routeMetadata.test.ts (1)
12-12: LGTM!Also applies to: 92-105, 107-126, 127-177, 178-192, 230-287
src/integrations/index.test.ts (1)
40-48: LGTM!src/services/api/openaiShim.test.ts (1)
4114-4144: LGTM!Also applies to: 6987-6991, 7016-7016, 8781-8984
src/utils/providerProfiles.ts (1)
45-45: LGTM!Also applies to: 216-221, 412-421, 461-476, 522-529, 622-625, 871-872, 1218-1219, 1337-1357, 1447-1451, 1676-1677, 1699-1700, 1734-1744
src/integrations/runtimeMetadata.ts (1)
24-24: LGTM!Also applies to: 273-275
src/services/api/providerConfig.ts (1)
961-964: LGTM!src/services/api/providerConfig.local.test.ts (1)
173-216: LGTM!src/services/api/client.test.ts (1)
75-75: LGTM!Also applies to: 122-129, 167-167, 216-217, 255-255, 307-450
src/utils/providerProfiles.test.ts (1)
22-22: LGTM!Also applies to: 49-49, 114-114, 697-884, 2449-2461, 2787-2857, 3515-3552, 3760-3854
🛑 Comments failed to post (2)
src/services/api/client.test.ts (1)
1-2: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C3 --type=ts "mock\.module\(.*utils/model/providers" . rg -n -C3 --type=ts "acquireSharedMutationLock" srcRepository: Gitlawb/openclaude
Length of output: 50375
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' src/services/api/client.test.ts printf '\n---\n' sed -n '1,180p' src/utils/model/model.openai-shim-providers.test.ts printf '\n---\n' sed -n '1,120p' src/services/compact/compact.test.ts printf '\n---\n' sed -n '1,120p' src/test/sharedMutationLock.tsRepository: Gitlawb/openclaude
Length of output: 23270
Re-register the provider mock in
beforeEach.
mock.module()is process-global, so the top-level registration can be overwritten by another suite before this file importsclient.js. Move the two provider registrations under the locked setup so this test always binds the real provider module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/client.test.ts` around lines 1 - 2, Move the provider mock.module registrations from top-level initialization into this test file’s beforeEach setup, alongside the shared mutation lock acquisition. Ensure both provider registrations are re-established under the lock before client.js is imported, while preserving the existing afterEach cleanup.Source: Path instructions
src/utils/providerFlag.ts (1)
417-429: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Block-scope the
custom-anthropicswitch clause.The lexical declarations on Lines 428–429 remain scoped across the switch, triggering Biome’s
noSwitchDeclarationserrors and blocking CI.Proposed fix
- case 'custom-anthropic': + case 'custom-anthropic': { if (!process.env.ANTHROPIC_BASE_URL?.trim()) { return { error: 'Custom Anthropic-compatible provider requires ANTHROPIC_BASE_URL.', } } // ... break + }As per coding guidelines, “pull requests must pass CI checks.”
📝 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.case 'custom-anthropic': { if (!process.env.ANTHROPIC_BASE_URL?.trim()) { return { error: 'Custom Anthropic-compatible provider requires ANTHROPIC_BASE_URL.', } } if (isFirstPartyAnthropicBaseUrlForEnv(process.env)) { return { error: 'Custom Anthropic-compatible provider requires a non-Anthropic ANTHROPIC_BASE_URL.', } } const hasAuthToken = Boolean(process.env.ANTHROPIC_AUTH_TOKEN?.trim()) const hasApiKey = Boolean(process.env.ANTHROPIC_API_KEY?.trim())🧰 Tools
🪛 Biome (2.5.3)
[error] 428-428: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.(lint/correctness/noSwitchDeclarations)
[error] 429-429: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/providerFlag.ts` around lines 417 - 429, Block-scope the `custom-anthropic` case in the provider-flag switch so its lexical declarations, including `hasAuthToken` and `hasApiKey`, do not share the switch scope. Wrap the case body in braces while preserving its existing validation and return behavior.Sources: Coding guidelines, Linters/SAST tools
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore the real modules after installing process-global test mocks
src/services/compact/compact.test.ts:635
This removes the registrations that restoredmock.module()stubs, even thoughmock.restore()does not unregister them. The same cleanup regression appears inautoCompact, the tips suites, attribution, andProviderManager. As a result, later serialized tests import partial mocks instead of the production modules; for example,bun test --max-concurrency=1 src/services/compact/compact.test.ts src/services/compact/autoCompact.test.ts src/services/tips/sponsoredTips.test.ts src/services/tips/tipScheduler.test.tsnow has 28 auto-compact failures, beginning with a missingcheckHasTrustDialogAcceptedexport. Restore the full real-module registrations (or otherwise scope the mocks) before releasing each suite's shared lock. -
[P1] Keep HTTPS loopback Ollama endpoints on the local chat-completions path
src/integrations/routeMetadata.ts:113
The tightened resolver rejectshttps://localhost:11434/v1, despite this being an existing supported Ollama proxy form. It therefore resolves as generic OpenAI; withOPENAI_API_FORMAT=responses,resolveProviderRequest()now selectsresponsesrather than Ollama's chat-completions handling. The endpoint consequently receives an unsupported API shape. Preserve the local route identity for HTTPS loopback endpoints (and add the API-format regression case). -
[P1] Do not bind the startup-override test to a leaked module instance
src/utils/providerStartupOverrides.test.ts:3
Replacing the per-test cache-busted import with a top-level import makes this test retain whichever settings/mock dependency was registered while the test files initialized. In the complete changed-suite serialization, the injectedupdateUserSettingsis never called and this test fails. Keep a fresh import inside the test, or restore the dependency deterministically before importing the helper. -
[P2] Use the compatible route resolver for the provider editor
src/components/ProviderManager.tsx:349
A generic profile athttps://ollama.example.com/v1is still classified as OpenAI-compatible in the editor, so the UI offers a Responses API selection. The new profile/runtime resolver classifies the same URL as Ollama and silently strips that selection on save, then forces chat completions at runtime. Resolve editor capabilities withresolveLocalCompatibleRouteIdFromBaseUrlas well, and cover the remote-Ollama editing flow. -
[P2] Re-register the provider module mock under the shared lock
src/services/api/client.test.ts:131
The client import now occurs inbeforeEach, but the realprovidersmock is registered only during test-file evaluation. Because module mocks are process-global, another suite can replace it between evaluation and the locked import; these tests can then exercise a foreign stub rather than the real provider logic. Re-register both module specifiers immediately before the cache-bustedclient.tsimport.
Summary
--providerexplicitly selects Ollama, NVIDIA NIM, Bankr, xAI, Xiaomi MiMo, Venice, Near AI, or Fireworks:11434,:1234,ollama,lmstudio, andlm-studioOPENAI_API_BASEas an explicit configured OpenAI-compatible base URL whenOPENAI_BASE_URLis unset, so custom aliases are preserved rather than overwrittenImpact
OPENAI_API_BASEaliases are intentionally respected whenOPENAI_BASE_URLis blank.Testing
bun test --feature=UNATTENDED_RETRY --max-concurrency=1 src/utils/providerFlag.test.ts src/integrations/routeMetadata.test.ts src/utils/providerValidation.test.ts src/integrations/discoveryService.test.tsbun run typecheckgit diff --checkNotes
Summary by CodeRabbit
OPENAI_API_FORMATstays unset whileOPENAI_AUTH_HEADER/scheme/value are still applied correctly.