fix(config): clear stale baseUrl when switching away from custom provider#222
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (4)src/**/*.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/providers/**/*.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/**/*.mjs📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/{history,llm}/*.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (6)
📝 WalkthroughWalkthroughCentralizes custom-provider constants, updates initialization and LLM client handling, and makes ChangesCustom provider configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
|
@c436zhan please resolve the merge conflicts |
…ider When a user switches from the custom provider (__custom__) to a built-in provider via , the previous custom baseUrl was left in the config file and continued to override the built-in provider's endpoint. Change updateConfigField so that setting the provider to any built-in also clears baseUrl, while preserving baseUrl when switching to the custom provider. Fixes 404-PF#202
c436zhan
force-pushed
the
fix/config-set-clear-stale-baseurl
branch
from
f3ae4b8 to
fae80dd
Compare
404-Page-Found
left a comment
404-Page-Found
left a comment
There was a problem hiding this comment.
PR Review: #222 — fix(config): clear stale baseUrl when switching away from custom provider
Summary
This PR fixes issue #202: when switching from the custom provider (__custom__) to a built-in provider via config set provider <builtin>, the previously stored baseUrl was left in the config file and continued to override the built-in provider default endpoint. The fix adds a targeted conditional in updateConfigField that clears baseUrl when switching to a non-custom provider. Two regression tests are included.
Strengths
- Minimal, focused fix — The change is surgically targeted at the exact root cause in
updateConfigField(config.ts:86-91). No unnecessary refactoring. - Correct test coverage — Both tests cover the exact regression and its inverse, using the existing
withTempHome/writeConfigtest infrastructure. - Well-documented — The PR description clearly explains the root cause, the fix, and the testing performed.
- No breaking changes — Existing users with stale
baseUrlwill now correctly use built-in endpoints; users who intentionally setbaseUrlon a built-in before switching to another built-in will also get it cleared, which is correct behavior.
Issues Found
🔴 Critical — None.
🟡 Suggestions
-
CUSTOM_PROVIDER_KEYis duplicated across 4 files —src/commands/config.ts:14,src/providers/index.ts:10,src/commands/init.ts:9, and hard-coded\__custom__\'insrc/llm/client.ts:9. If this sentinel value ever changes, all four must be updated. Consider exporting it from a single location (e.g.,src/types.tsorsrc/providers/registry.ts). -
apiKeyis not cleared alongsidebaseUrl— When switching from__custom__to a built-in provider,baseUrlis cleared butapiKeypersists. Built-in providers resolve their API keys from environment variables (OPENAI_API_KEY, etc.), so the storedapiKeybecomes orphaned. This is harmless but could confuse users who runconfigand see an API key that is not being used. Consider clearingapiKeytoo when switching to a non-custom provider — or at minimum, document why it is preserved. -
Domain-specific logic in a generic updater —
updateConfigFieldnow has provider-aware branching. If more provider-specific fields are added in the future (e.g.,customHeaders,apiVersion), this function will grow increasingly coupled. A future refactor could move this to aproviderSwitchCleanuphelper, but for now it is acceptable.
🟢 Nitpicks
-
The diff uses tab indentation while the rest of the file uses 2-space — The new lines in
updateConfigFieldappear to use tabs (\t). Prettier is configured with"tabWidth": 2and nouseTabs. If this passesformat:check, it may just be a rendering artifact in the diff, but worth double-checking. -
Test could explicitly verify
config.baseUrl === undefined— The current assertionassert.equal(config.baseUrl, undefined)is correct, but adding a comment like// not present in saved JSONwould clarify intent for future readers.
Testing Assessment
The two new tests cover the core regression scenarios:
- ✅ Custom → built-in clears
baseUrl - ✅ Built-in → custom preserves
baseUrl
Missing but not blocking:
- Built-in → built-in with existing
baseUrl(should clear) — implied by the fix but not explicitly tested - Verification that
commit-echo suggestactually uses the correct endpoint after the switch (integration test territory)
Verdict
Approve with suggestions — The fix is correct, minimal, and well-tested. The CUSTOM_PROVIDER_KEY duplication and apiKey persistence are worth addressing but not blockers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/config.ts`:
- Around line 194-195: Remove the redundant parse in the config-setting flow by
extracting a pure validation helper from parseConfigSetValue, such as
validateConfigSetValue, and use it in the try-catch around the command handler
while keeping updateConfigFromRawValue responsible for the single actual parse
and update.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51f9db8c-1939-4d25-a9c5-d530ee5f24f1
📒 Files selected for processing (6)
src/commands/config.tssrc/commands/init.tssrc/llm/client.tssrc/providers/index.tssrc/providers/registry.tstests/config-command.test.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
The project is ESM-only: use
.jsextensions in local imports and keepverbatimModuleSyntax-compatible TypeScript imports (for example,import typefor type-only imports).
Files:
src/providers/registry.tssrc/llm/client.tssrc/providers/index.tssrc/commands/init.tssrc/commands/config.ts
src/providers/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
All LLM provider implementations must conform to the shared
Providerinterface and integrate with the provider abstraction used by the factory/registry system.
Files:
src/providers/registry.tssrc/providers/index.ts
tests/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.mjs: Use Node.js built-innode:testfor test files intests/andtests/e2e/; do not use Jest or Mocha.
Write test assertions withnode:assert/strictin test files undertests/andtests/e2e/.
Files:
tests/config-command.test.mjs
src/{history,llm}/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Recent commit history should be stored as JSONL, analyzed by
buildProfile()for commit-style patterns, and injected into prompts as style guidance.
Files:
src/llm/client.ts
🔇 Additional comments (7)
src/providers/registry.ts (1)
3-5: LGTM!src/providers/index.ts (1)
6-14: LGTM!src/commands/config.ts (1)
91-110: LGTM!tests/config-command.test.mjs (2)
188-188: LGTM!Also applies to: 209-209
303-349: LGTM!src/commands/init.ts (1)
3-20: LGTM!Also applies to: 54-54, 75-75, 94-94, 129-133, 236-236, 295-295
src/llm/client.ts (1)
2-10: LGTM!Also applies to: 26-26
|
@coderabbitai full review |
✅ Action performedFull review finished. |
2 participants
Summary
Switching from the custom provider (
__custom__) to a built-in provider viaconfig set provider <builtin>left the previousbaseUrlin the config file. That stale URL continued to override the built-in provider's endpoint, so requests were sent to the wrong place.Root cause
updateConfigFieldinsrc/commands/config.tsused{ ...config, [key]: value }, which updated only the requested key and preserved everything else, includingbaseUrl.Changes
src/commands/config.ts: whenkey === 'provider'and the new value is not__custom__, also setbaseUrltoundefinedso the built-in provider's default endpoint is used.tests/config-command.test.mjs: add two regression tests:__custom__toopenaiclearsbaseUrlopenaito__custom__preserves the existingbaseUrlTesting
npm run build— compiles cleanlynode --test tests/config-command.test.mjs— 26 tests: 24 pass (2 pre-existing unrelated JSON-output failures)npm run format:check— no formatting issues in changed filesCompatibility
No breaking changes. Users who previously had a stale
baseUrlafter switching providers will now get the correct built-in endpoint. Users who intentionally setbaseUrlon a built-in provider and then switch to another built-in will also have it cleared, which is the desired behavior per the issue.Fixes #202
Summary by CodeRabbit
baseUrlas appropriate (e.g., switching away from or to the custom provider).config --jsonexpectations for normalized provider values.baseUrlclearing/preservation across provider changes.