fix(auth): preserve custom provider models on install - #5404
Conversation
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] acpAgent.ts:1514 — resolveExistingProviderApiKey credential mix-up
With identity-based merge, models from multiple custom endpoints now coexist under the same env-key prefix (QWEN_CUSTOM_API_KEY_). When the ACP client reconnects to endpoint B without supplying an apiKey, findExistingProviderModels (which matches ALL custom models by prefix) returns models[0] — which may carry endpoint A's envKey. This sends endpoint A's API key to endpoint B, a credential mix-up.
Fix: Filter by baseUrl in resolveExistingProviderApiKey, or compute the envKey directly via generateCustomEnvKey(protocol, baseUrl) instead of walking the stored model array.
— qwen3.7-max via Qwen Code /review
| reloadModelProviders: (mp) => config.reloadModelProvidersConfig(mp), | ||
| syncAuthState: (authType, modelId) => | ||
| config.getModelsConfig().syncAfterAuthRefresh(authType, modelId), | ||
| syncAuthState: (authType, modelId, baseUrl) => |
There was a problem hiding this comment.
[Critical] Test failure: useProviderUpdates.test.ts:300 asserts syncAfterAuthRefresh is called with 2 args, but this change forwards a 3rd baseUrl parameter. For non-custom providers, the mock receives (AuthType.USE_OPENAI, 'qwen3.5-plus', undefined) — 3 args, not 2. CI confirms this test fails on all 3 platforms.
Update the test assertion:
expect(mockModelsConfig.syncAfterAuthRefresh).toHaveBeenCalledWith(
AuthType.USE_OPENAI,
'qwen3.5-plus',
undefined,
);— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
yep, fixed the assertion to include the optional third arg (undefined) and re-ran the hook test locally.
| if (plan.modelSelection?.modelId) { | ||
| currentStep = 'syncAuthState'; | ||
| syncAuthState?.(plan.authType, plan.modelSelection.modelId); | ||
| if (plan.modelSelection.baseUrl) { |
There was a problem hiding this comment.
[Suggestion] The if/else branching is unnecessary. The syncAuthState callback type already declares baseUrl?: string (optional), so a single call suffices:
| if (plan.modelSelection.baseUrl) { | |
| syncAuthState?.( | |
| plan.authType, | |
| plan.modelSelection.modelId, | |
| plan.modelSelection.baseUrl, | |
| ); |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
done, collapsed this to the single optional-baseUrl call and updated the affected test expectation.
|
fixed the credential mix-up too. ACP now resolves the stored key after parsing the requested protocol/baseUrl, so reconnecting endpoint B looks up B’s env key instead of whatever custom model happens to be first. added a two-custom-endpoint regression test for that path. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
The R1 Critical findings (credential mix-up in resolveExistingProviderApiKey, test assertion mismatch) are addressed in this revision. The lazy callback pattern for apiKey resolution is correct — env key is derived from the requested (protocol, baseUrl) pair, not the first stored model. mergeModelsByIdentity correctly scopes install-time deletion to identity-matched entries while preserving prefix-based ownsModel for listing flows. Tests pass locally.
— qwen3.7-max via Qwen Code /review
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical issue from the earlier review appears resolved in the current head. I rechecked the endpoint-scoped custom provider key reuse path and did not find new critical issues in this pass.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM ✅
Re-reviewed at HEAD 4678b2c3 — CI is now green (51 checks; my earlier review only held off because CI was still running). The R1 credential-mix-up Critical is resolved: resolveExistingProviderApiKey now resolves the stored key for the requested protocol/baseUrl (via a callback invoked after parsing), so reconnecting endpoint B looks up B's key rather than reusing the first model's. Both earlier inline comments (the useProviderUpdates test assertion and the install.ts optional-baseUrl collapse) are addressed. Verified locally: cli+core typecheck clean, changed-area tests pass (core 101, cli 138).
— claude-opus-4-8 via Claude Code /qreview
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No review findings. LGTM! ✅
The identity-based merge strategy is well-scoped, and the baseUrl threading through resolveExistingProviderApiKey/syncAfterAuthRefresh correctly scopes credential resolution to the requested endpoint. CI all pass (30/30).
— qwen3.7-max via Qwen Code /review
Verification report — preserve custom provider models on installI built this PR locally and verified it with real runtime tests: real Verdict: ✅ Fix is correct and well-covered. Recommend merge. Both commits do what they claim, with genuine regression coverage. Only minor, non-blocking notes below. Environment & method
1. Unit tests (real
|
| Suite | Result |
|---|---|
core install.test.ts + custom-provider.test.ts + zai.test.ts + modelsConfig.test.ts |
104/104 pass |
core full providers/ + models/ dirs (regression sweep) |
300/300 pass (16 files) |
cli acpAgent.test.ts + useProviderUpdates.test.ts |
138/138 pass |
core + cli tsc (during build) |
0 errors |
2. Revert-proof (PR's new tests fail on base source)
install.test.ts > preserves existing custom provider models and selects the installed endpoint→ fails on base (expected [Function ownsModel] to be undefined).install.test.ts > persists env, auth selection, …→ fails on base (syncAuthState called with 2 args, test expects the 3rdbaseUrl).modelsConfig.test.ts > should use explicit provider baseUrl when syncing after provider install→ fails on base.acpAgent.test.ts > qwen/providers/connect reuses the custom apiKey for the requested baseUrl only→ fails on base (commit 2 — base resolves the wrong endpoint's key).
3. Real-source + real-settings.json A/B (commit 1 — the core fix)
Scenario: two custom models already installed at endpoint A (keep-model@A, shared-model@A), then install shared-model at a different endpoint B.
| Observable | BASE (the bug) | PR (the fix) |
|---|---|---|
modelProviders.openai after install |
only shared-model@B — keep-model@A and shared-model@A WIPED |
all three preserved: shared-model@B, keep-model@A, shared-model@A |
model.baseUrl |
"" (cleared) |
…endpoint-b… (persisted) |
syncAuthState(...) |
(openai, shared-model) |
(openai, shared-model, …endpoint-b…) |
plan.modelProviders[0].ownsModel |
function (removes all prefix-owned) |
undefined (merges by id+baseUrl) |
This is exactly the bug in #4814: under the old ownsModel prefix match, installing any custom model deleted every other custom model. The PR keeps ownsModel for discovery but installs merge by identity.
4. Real binary E2E over ACP (dist/cli.js, both commits, also re-run in tmux)
Four sequential qwen/providers/connect calls against the shipping binary: shared-model@A (key sk-A), keep-model@A (key sk-A), shared-model@B (key sk-B), then keep-model@A with NO apiKey.
- All four return
success, and the response now carriesbaseUrl(a PR addition). - Persisted
modelProviders.openaiholds all 3 distinct identities — nothing wiped across installs at different endpoints. - Both endpoint keys persist:
envhas the endpoint-A key =sk-Aand the endpoint-B key =sk-B(keys are namespaced per baseUrl). - The 4th call (no apiKey) succeeds by reusing endpoint-A's stored key — this is commit 2: on base,
resolveExistingProviderApiKeyused the first stored model's key (which, after the B install, would besk-B), reusing the wrong endpoint's secret.
Findings (all non-blocking)
- Test-import inconsistency in
custom-provider.test.ts. Unlike its sibling provider tests (which import relatively), it importscustomProvider/buildInstallPlanfrom the package entry@qwen-code/qwen-code-core, i.e. the builtdist/. Those assertions therefore validate freshly-built output, not raw source — fine under build-then-test CI (the PR's test plan builds first), but it can silently pass against staledist/in a localvitest-only loop. The file even re-importsgenerateCustomEnvKeyrelatively to dodge this. Consider importing all of them from the relative source for consistency. - Endpoint key reuse is keyed by exact normalized baseUrl (commit 2). Reconnecting with a cosmetically different baseUrl (e.g. a trailing slash) yields a different env key, so the stored secret won't be found and the user must re-enter it. Correct-by-design, but a possible sharp edge worth a docs note.
- Installs no longer remove other custom models — distinct custom models/endpoints now accumulate by design. Re-installing the same
id+baseUrlreplaces in place (no duplicate), but there is no per-entry cleanup short ofuninstall/identity-replace. This is the intended behavior; noting for awareness.
What's good
- Focused, correct fix:
ownsModelretained for UI/ACP discovery, whilemergeModelsByIdentitymakes installs merge byid+baseUrlinstead of nuking every prefix-owned entry. model.baseUrlis persisted and threaded intosyncAfterAuthRefresh(…, providerBaseUrlOverride), so the just-installed endpoint is selected even when the same model id exists on multiple endpoints.- Backward compatible: the new
baseUrl/providerBaseUrlOverrideparams are optional; the generalrefreshAuthsync path is unchanged. - Genuine regression coverage across both commits (every new assertion fails on base).
🇨🇳 中文版（点击展开）
验证报告 —— 安装时保留自定义 provider 的模型
我在本地构建了该 PR 并用真实运行时测试验证：真实 vitest、一个直接驱动真实 buildInstallPlan / applyProviderInstallPlan 且写真实 settings.json 的“真实源码 + 真实文件”A/B 脚本，以及用构建出的 dist/cli.js 二进制通过 ACP（qwen/providers/connect）做端到端验证。
结论：✅ 修复正确、覆盖充分，建议合并。 两个 commit 都做到了所声明的行为，且有真正的回归测试覆盖。只有下面几条不阻塞合并的小提示。
环境与方法
- 验证两个 commit，合并进当前
main（a55eae9→ 合并提交b39ded4）：ad3d0c01（安装时保留自定义 provider 模型）和4678b2c3（按 endpoint 限定 ACP 自定义 key 复用）。 ⚠️ 给 reviewer 的提醒：在我刷新origin/main之前，本地git diff origin/main...HEAD显示 81 个文件。PR 分支是基于几个尚未合并的 commit 构建的，因此本地main过期会污染三点 diff。git fetch origin main之后，真实 diff 就是 GitHub 显示的 13 个文件 / +348 / −74。- 构建
dist/cli.js（npm run build --cli-only && npm run bundle，typecheck 0 错误）。 - A/B 通过
tsx在 base 源码 vs PR 源码 上各自运行（相对./src导入，所以两边各跑各自的代码），写入真实磁盘上的settings.json。Node v22.22.2。
1. 单元测试（真实 vitest，合并后的代码树）
coreinstall.test.ts+custom-provider.test.ts+zai.test.ts+modelsConfig.test.ts：104/104 通过core整个providers/+models/目录（回归扫描）：300/300 通过（16 个文件）cliacpAgent.test.ts+useProviderUpdates.test.ts：138/138 通过；构建期tsc：0 错误
2. 回归证明（PR 新增测试在 base 源码上失败）
install.test.ts > preserves existing custom provider models …→ base 失败（expected [Function ownsModel] to be undefined）。install.test.ts > persists env, auth selection …→ base 失败（syncAuthState 只传了 2 个参数，测试要求第 3 个baseUrl）。modelsConfig.test.ts > should use explicit provider baseUrl when syncing after provider install→ base 失败。acpAgent.test.ts > qwen/providers/connect reuses the custom apiKey for the requested baseUrl only→ base 失败（commit 2 —— base 取错了 endpoint 的 key）。
3. 真实源码 + 真实 settings.json 的 A/B（commit 1 —— 核心修复）
场景：endpoint A 上已装两个自定义模型（keep-model@A、shared-model@A），随后在不同的 endpoint B 上安装 shared-model。
| 观测项 | BASE（bug） | PR（修复） |
|---|---|---|
安装后的 modelProviders.openai |
只剩 shared-model@B，keep-model@A 与 shared-model@A 被清除 |
三个全部保留：shared-model@B、keep-model@A、shared-model@A |
model.baseUrl |
""（被清空） |
…endpoint-b…（被持久化） |
syncAuthState(...) |
(openai, shared-model) |
(openai, shared-model, …endpoint-b…) |
plan.modelProviders[0].ownsModel |
function（删除所有 prefix-owned） |
undefined（按 id+baseUrl 合并） |
这正是 #4814 的 bug：旧的 ownsModel 前缀匹配下，安装任意自定义模型都会删掉其它所有自定义模型。PR 保留 ownsModel 用于发现，但安装改为按身份合并。
4. 真实二进制 ACP 端到端（dist/cli.js，覆盖两个 commit，并在 tmux 中复跑）
对 shipping 二进制连续发 4 次 qwen/providers/connect：shared-model@A（key sk-A）、keep-model@A（key sk-A）、shared-model@B（key sk-B），最后 keep-model@A 不带 apiKey。
- 四次都返回
success，且响应里现在带上了baseUrl（PR 新增）。 - 持久化后的
modelProviders.openai保留了全部 3 个不同身份——跨不同 endpoint 多次安装没有丢失任何条目。 - 两个 endpoint 的 key 都在：
env里 endpoint-A 的 key =sk-A，endpoint-B 的 key =sk-B（key 按 baseUrl 命名空间隔离）。 - 第 4 次（不带 apiKey）靠复用 endpoint-A 已存的 key 成功——这就是 commit 2：在 base 上，
resolveExistingProviderApiKey用的是第一个已存模型的 key（在装了 B 之后会是sk-B），从而复用了错误 endpoint 的密钥。
发现的问题（均不阻塞合并）
custom-provider.test.ts的导入不一致：和同目录其它 provider 测试（相对导入）不同，它从包入口@qwen-code/qwen-code-core（即构建出的dist/）导入customProvider/buildInstallPlan。这些断言因此校验的是构建产物而非源码——在“先构建再测试”的 CI 下没问题（PR 的测试计划会先构建），但在本地只跑vitest时可能对着过期的dist/通过。该文件甚至特意把generateCustomEnvKey改成相对导入来规避这一点。建议统一改为相对源码导入。- endpoint key 复用按归一化后的精确 baseUrl 命中（commit 2）：用一个外观略不同的 baseUrl（如多了结尾斜杠）重连会算出不同的 env key，从而找不到已存密钥、需要用户重新输入。属于设计正确，但是个可能的“坑”，值得在文档里提一句。
- 安装不再删除其它自定义模型——不同的自定义模型/endpoint 现在会按设计累积：重装相同
id+baseUrl会原地替换（不重复），但除了uninstall/同身份替换之外没有逐条清理。这是预期行为，仅作提醒。
优点
- 修复聚焦且正确：保留
ownsModel用于 UI/ACP 发现，同时用mergeModelsByIdentity让安装按id+baseUrl合并，而不是清空所有 prefix-owned 条目。 model.baseUrl被持久化并透传进syncAfterAuthRefresh(…, providerBaseUrlOverride)，因此即便同一个 model id 存在于多个 endpoint，也能选中刚安装的那个 endpoint。- 向后兼容：新增的
baseUrl/providerBaseUrlOverride参数都是可选的；通用refreshAuth同步路径未变。 - 两个 commit 都有真正的回归覆盖（每条新断言在 base 上都会失败）。
Verified by building the PR merged with main, running the suites, an A/B harness on base-vs-PR source against a real settings.json, and driving the built dist/cli.js over ACP.
| models, | ||
| mergeStrategy: 'prepend-and-remove-owned' as const, | ||
| ownsModel: resolveOwnsModel(config), | ||
| ...(ownsModel ? { ownsModel } : {}), |
There was a problem hiding this comment.
[Critical] Trailing-slash normalization mismatch between identity merge and env key derivation
This line suppresses ownsModel when mergeModelsByIdentity is true, causing applyModelProvidersPatch to fall through to isSameModelIdentity which compares baseUrl with raw string equality: (a.baseUrl ?? '') === (b.baseUrl ?? '') (install.ts:40-44).
However, generateCustomEnvKey strips trailing slashes before hashing (stripTrailingSlashes(baseUrl.trim())). Two URLs differing only in trailing slash — e.g. https://proxy.example/v1 vs https://proxy.example/v1/ — produce the same envKey but are treated as different model identities. The second install does not replace the first; it prepends a duplicate entry sharing the same API key. Each reinstall with a slash variant accumulates another orphan entry.
Normalize baseUrl in isSameModelIdentity to match generateCustomEnvKey's canonicalization:
| ...(ownsModel ? { ownsModel } : {}), | |
| function isSameModelIdentity( | |
| a: { id: string; baseUrl?: string }, | |
| b: { id: string; baseUrl?: string }, | |
| ): boolean { | |
| const normalizeUrl = (url?: string) => (url ?? '').trim().replace(/\/+$/, ''); | |
| return a.id === b.id && normalizeUrl(a.baseUrl) === normalizeUrl(b.baseUrl); | |
| } |
Alternatively, normalize inputs.baseUrl once in readProviderSetupInputs before it flows into model configs and env key derivation.
— qwen3.7-max via Qwen Code /review
3 participants
Summary
Refs #4814
Tests
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.