Skip to content

fix(core): preserve shared baseUrl on auth refresh#4828

Merged
tanzhenxin merged 1 commit into
QwenLM:mainfrom
he-yufeng:fix/model-provider-base-url-precedence
Jun 9, 2026
Merged

fix(core): preserve shared baseUrl on auth refresh#4828
tanzhenxin merged 1 commit into
QwenLM:mainfrom
he-yufeng:fix/model-provider-base-url-precedence

Conversation

@he-yufeng

@he-yufeng he-yufeng commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

What this PR does

This preserves a same-model baseUrl that was resolved from CLI flags, environment variables, or settings during syncAfterAuthRefresh. 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 from modelProviders, then applyResolvedModelDefaults() applies that provider entry. For same-model refreshes, that could overwrite a shared baseUrl with a model default, making one shared endpoint unusable across multiple modelProviders entries 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 --check

The 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 baseUrl with the model entry's default endpoint.

After: same-model auth refresh keeps the already resolved shared baseUrl while still applying the model entry's other defaults.

Tested on

OS Status
🍏 macOS ⚠️ not tested
🪟 Windows ✅ tested
🐧 Linux ⚠️ not tested

Environment (optional)

Node/npm workspace test environment for packages/core.

Risk & Scope

  • Main risk or tradeoff: low; the change only preserves an explicit already-resolved base URL for the same model.
  • Not validated / out of scope: provider-specific network integration tests; this is covered at config-resolution level.
  • Breaking changes / migration notes: none.

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

OS Status
🍏 macOS ⚠️ not tested
🪟 Windows ✅ tested
🐧 Linux ⚠️ not tested

环境

packages/core 的 Node/npm workspace test 环境。

风险与范围

  • 主要风险或取舍:低;改动只在同模型场景保留明确解析出的 base URL。
  • 未验证 / 范围外:provider-specific 网络集成测试;这里用配置解析层单测覆盖。
  • 破坏性变更 / 迁移说明:无。

关联 issue

Fixes #4813.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

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 baseUrl across modelProviders entries gets clobbered on auth refresh. The linked issue (#4813) describes a genuine pain point for anyone running multiple models behind a single proxy/endpoint. Claude Code's CHANGELOG has multiple entries fixing ANTHROPIC_BASE_URL-related issues (e.g. background side-queries, auto-naming), confirming this is a relevant concern area. Aligned and worth fixing.

On approach: the scope is right — 15 lines that mirror the existing savedApiKey/savedApiKeySource pattern exactly. No new abstractions, no scope creep. The condition kind === 'cli' || kind === 'env' || kind === 'settings' correctly captures "user explicitly set this URL" vs "it came from the provider entry." I don't see a simpler path.

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 时共享 baseUrl 被覆盖。关联的 issue (#4813) 描述了在单个代理/端点运行多个模型时的真实痛点。Claude Code CHANGELOG 有多条修复 ANTHROPIC_BASE_URL 相关问题的条目(如后台侧查询、自动命名),确认这是相关关注领域。方向对齐,值得修复。

方案:范围恰当——15 行代码,完全复刻已有的 savedApiKey/savedApiKeySource 模式。无新抽象,无范围蔓延。条件 kind === 'cli' || kind === 'env' || kind === 'settings' 正确区分了"用户显式设置了 URL"与"来自 provider 配置"。没有看到更简路径。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No review findings. Downgraded from Approve to Comment: CI failing (triage). — DeepSeek/deepseek-v4-pro via Qwen Code /review

@wenshao

wenshao commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Local Verification Report

Branch: fix/model-provider-base-url-precedencemain
Environment: macOS Darwin 25.4.0, Node.js local

TypeScript Compilation (tsc --noEmit)

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:

  • baseUrl retains its original value after refresh
  • apiKey is 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(): saves baseUrl and its source if (1) model is unchanged, (2) baseUrl exists, and (3) source is cli/env/settings
  • After applyResolvedModelDefaults(): restores the saved baseUrl and 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 wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No review findings. Downgraded from Approve to Comment: CI failing (triage). — qwen3.7-max via Qwen Code /review

xaelistic pushed a commit to xaelistic/qwen-code that referenced this pull request Jun 7, 2026
…4828)

Co-authored-by: harold <haroldmciver@google.com>
Co-authored-by: N. Taylor Mullen <ntaylormullen@google.com>
@tanzhenxin tanzhenxin added the type/bug Something isn't working as expected label Jun 8, 2026

@DragonnZhang DragonnZhang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. The shouldPreserveResolvedBaseUrl guard properly requires isUnchanged (same auth type, same model, no provider change) and only preserves when the source is cli, env, or settings.
  • Edge cases — cross-provider switches, auth type changes, and provider config hot-reloads all correctly bypass preservation via the isUnchanged check. The hasBeenApplied / isProviderChanged detection from #3417 is properly respected.
  • Test coverage — thorough. The it.each over 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 tanzhenxin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 baseUrlproviderBaseUrl=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!

@tanzhenxin tanzhenxin merged commit ea4cbaf into QwenLM:main Jun 9, 2026
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/bug Something isn't working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

modelProviders: shared baseUrl cannot be set once for multiple models

5 participants