fix(core): preserve shared baseUrl on auth refresh#4828
Conversation
|
Thanks for the PR! Template deviations: the PR body uses custom headings ("What changed" / "Why" / "To verify") instead of the template's ("What this PR does" / "Why it's needed" / "Reviewer Test Plan"), and is missing the "Risk & Scope", "Tested on", and "Evidence (Before & After)" sections. Not blocking for a focused bugfix — the substance is all there — but worth aligning with the template on future PRs. On direction: this is a clear, narrow bugfix for a real user problem — shared On approach: the scope is right — 15 lines that mirror the existing Moving on to code review. 🔍 中文说明感谢贡献! 模板偏差:PR 使用了自定义标题("What changed" / "Why" / "To verify"),而非模板要求的("What this PR does" / "Why it's needed" / "Reviewer Test Plan"),并缺少 "Risk & Scope"、"Tested on" 和 "Evidence (Before & After)" 部分。对于一个聚焦的 bugfix 不作为阻塞项——实质内容都有——但建议后续 PR 对齐模板。 方向:这是一个清晰、范围恰当的 bugfix,解决真实用户问题——auth refresh 时共享 方案:范围恰当——15 行代码,完全复刻已有的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing (triage). — DeepSeek/deepseek-v4-pro via Qwen Code /review
Local Verification ReportBranch: TypeScript Compilation (
|
| Package | PR Branch | main | Status |
|---|---|---|---|
packages/core |
0 errors | 0 errors | ✅ Clean |
Unit Tests (vitest)
| Test File | PR Branch | main | Status |
|---|---|---|---|
modelsConfig.test.ts |
69 passed | 66 passed | ✅ All pass (+3 new tests) |
New tests use it.each with 3 source kinds (cli, env, settings) to verify that syncAfterAuthRefresh() preserves the user-supplied baseUrl when the model hasn't changed. Each test verifies:
baseUrlretains its original value after refreshapiKeyis preserved- Provider defaults (e.g.,
timeout) are still applied generationConfigSources['baseUrl']retains its original source metadata
Code Review
Bug: When syncAfterAuthRefresh() was called with the same model, it called applyResolvedModelDefaults() which overwrote baseUrl with the provider's default, discarding user-specified values from CLI --base-url, environment variable, or settings.
Fix (modelsConfig.ts):
- Before
applyResolvedModelDefaults(): savesbaseUrland its source if (1) model is unchanged, (2)baseUrlexists, and (3) source iscli/env/settings - After
applyResolvedModelDefaults(): restores the savedbaseUrland source
This mirrors the existing apiKey preservation pattern (lines 1005–1011) that was already in place. The fix is minimal, well-scoped, and correctly excludes provider-default baseUrls (only preserves user-specified ones).
Verdict
✅ Ready to merge — No regressions. All 69 tests pass. Core TSC is clean. Fix is minimal and follows existing patterns.
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing (triage). — qwen3.7-max via Qwen Code /review
…4828) Co-authored-by: harold <haroldmciver@google.com> Co-authored-by: N. Taylor Mullen <ntaylormullen@google.com>
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: No code issues found. Downgraded from Approve to Comment due to CI failing (triage).
This is a clean, focused bugfix that preserves a user-specified shared baseUrl during same-model auth refresh. The logic correctly mirrors the existing apiKey preservation pattern in syncAfterAuthRefresh.
What was reviewed:
- URL precedence logic in
syncAfterAuthRefresh()— correct. TheshouldPreserveResolvedBaseUrlguard properly requiresisUnchanged(same auth type, same model, no provider change) and only preserves when the source iscli,env, orsettings. - Edge cases — cross-provider switches, auth type changes, and provider config hot-reloads all correctly bypass preservation via the
isUnchangedcheck. ThehasBeenApplied/isProviderChangeddetection from #3417 is properly respected. - Test coverage — thorough. The
it.eachover all three source kinds (cli,env,settings) verifies both value and source preservation, while confirming other provider defaults (e.g.,timeout) still apply. - Deterministic checks (tsc, eslint) — 0 findings.
— qwen-code via Qwen Code /review
tanzhenxin
left a comment
There was a problem hiding this comment.
LGTM ✅ — correct, well-tested fix for the intended #4813 scenario (multiple modelProviders entries with distinct model ids sharing one user-provided baseUrl). Provenance discrimination via ConfigSourceKind is authoritative and the isUnchanged gate is the right key, so the common path is solid and this is a strict improvement over main (which dropped the override entirely for that config).
Known edge case to keep in mind (not blocking): if the same model id is configured in two modelProviders entries at different baseUrls and the user overrides baseUrl to the non-first one, the provider resolver disambiguates by model id alone (user-sourced baseUrl ⇒ providerBaseUrl=undefined) and falls back to the first entry — pairing that entry's API key with the other endpoint. The trigger config is uncommon and both endpoints are the user's own, so impact is mostly an auth failure rather than a leak. If it surfaces, the fix is to disambiguate resolved by the preserved baseUrl (or preserve the user-sourced apiKey alongside it) before applying defaults. Merging as-is. Thanks @he-yufeng!
What this PR does
This preserves a same-model
baseUrlthat was resolved from CLI flags, environment variables, or settings duringsyncAfterAuthRefresh. It still keeps provider defaults for other generation settings, but it no longer replaces a user-provided shared endpoint with the model entry's baked-in default during a same-model auth refresh.Why it's needed
syncAfterAuthRefresh()resolves a configured model frommodelProviders, thenapplyResolvedModelDefaults()applies that provider entry. For same-model refreshes, that could overwrite a sharedbaseUrlwith a model default, making one shared endpoint unusable across multiplemodelProvidersentries unless every model repeated the same URL.Reviewer Test Plan
How to verify
npm run test --workspace=packages/core -- modelsConfig.test.ts -- --testTimeout=30000 npx prettier --check packages/core/src/models/modelsConfig.ts packages/core/src/models/modelsConfig.test.ts npx eslint packages/core/src/models/modelsConfig.ts packages/core/src/models/modelsConfig.test.ts --max-warnings 0 git diff --checkThe regression test covers CLI/env/settings-sourced shared base URLs and confirms a same-model refresh keeps the shared endpoint.
Evidence (Before & After)
Before: same-model auth refresh could replace the resolved shared
baseUrlwith the model entry's default endpoint.After: same-model auth refresh keeps the already resolved shared
baseUrlwhile still applying the model entry's other defaults.Tested on
Environment (optional)
Node/npm workspace test environment for
packages/core.Risk & Scope
Linked Issues
Fixes #4813.
中文说明
这个 PR 做了什么
这个 PR 在
syncAfterAuthRefresh期间保留已经从 CLI、环境变量或 settings 解析出的同模型baseUrl。它仍然会保留 provider 默认的其他 generation 设置,但不会在同模型 auth refresh 时把用户提供的共享 endpoint 替换成模型条目里的默认 endpoint。为什么需要
syncAfterAuthRefresh()会从modelProviders里解析当前模型,然后applyResolvedModelDefaults()应用对应 provider entry。同模型 refresh 时,这可能把共享baseUrl覆盖成模型默认值,导致一个共享 endpoint 不能被多个modelProviders条目复用,除非每个模型都重复配置相同 URL。Reviewer Test Plan
如何验证
npm run test --workspace=packages/core -- modelsConfig.test.ts -- --testTimeout=30000 npx prettier --check packages/core/src/models/modelsConfig.ts packages/core/src/models/modelsConfig.test.ts npx eslint packages/core/src/models/modelsConfig.ts packages/core/src/models/modelsConfig.test.ts --max-warnings 0 git diff --check回归测试覆盖从 CLI/env/settings 解析出的共享 base URL,并确认同模型 refresh 会保留该 endpoint。
Before / After 证据
Before:同模型 auth refresh 可能把已解析的共享
baseUrl替换成模型 entry 的默认 endpoint。After:同模型 auth refresh 保留已解析的共享
baseUrl,同时继续应用模型 entry 的其他默认值。Tested on
环境
packages/core的 Node/npm workspace test 环境。风险与范围
关联 issue
Fixes #4813.