fix(core): preserve the selected model when re-applying a provider install plan - #5835
Conversation
8b31a41 to
eaefd61
Compare
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28136461891)._ |
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28137433957)._ |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Two callers of applyProviderInstallPlan — acpAgent.ts:4856 and run-qwen-serve.ts:1708 — read plan.modelSelection?.modelId after the call to construct their response. The plan object is never mutated; only the local effectiveModelSelection variable is set to undefined when the model is preserved. These callers will report the plan's default model rather than the actually active (preserved) model, causing ACP clients and serve-mode clients to display the wrong model name after reconnection. Consider having applyProviderInstallPlan return the effective model selection in its result, or reading the effective model from settings after the call.
— qwen3.7-max via Qwen Code /review
| (plan.modelProviders ?? []).some((patch) => | ||
| patch.models.some((model) => model.id === currentModelId), | ||
| ); | ||
| if (planOffersCurrentModel) { |
There was a problem hiding this comment.
[Suggestion] When planOffersCurrentModel is true, effectiveModelSelection is set to undefined, which skips the entire model write block below — including the model.baseUrl clearing (the empty-string tombstone on the else branch). If a user has a stale baseUrl from a previous provider setup and re-authenticates with a provider that offers the same model ID, the old baseUrl survives and could route API traffic to a stale or unintended endpoint.
Consider still applying the plan's baseUrl logic (write or clear) when preserving the model — only the model.name write should be skipped:
if (planOffersCurrentModel) {
// Preserve the model name, but still apply the plan's baseUrl decision
if (plan.modelSelection?.baseUrl) {
settings.setValue('model.baseUrl', plan.modelSelection.baseUrl);
} else {
settings.setValue('model.baseUrl', '');
}
effectiveModelSelection = undefined;
}— qwen3.7-max via Qwen Code /review
| let effectiveModelSelection = plan.modelSelection; | ||
| if (effectiveModelSelection?.modelId) { | ||
| const currentModelId = settings.getValue('model.name'); | ||
| const planOffersCurrentModel = |
There was a problem hiding this comment.
[Suggestion] This new planOffersCurrentModel branch is a significant behavioral change, but no tests in packages/core/src/providers/__tests__/install.test.ts exercise it. The existing test adapter's getValue is vi.fn() which returns undefined by default, so typeof currentModelId === 'string' is always false and the preservation path is never reached.
The PR description includes a comprehensive drop-in regression test (the install-preserve-user-model.test.ts block) that covers all five scenarios — consider including it in the diff, or adding equivalent cases to the existing install.test.ts.
— qwen3.7-max via Qwen Code /review
| backupSettingsFile: vi.fn(), | ||
| restoreSettingsFromBackup: vi.fn(), | ||
| cleanupSettingsBackup: vi.fn(), | ||
| getNestedProperty: vi.fn((obj, key) => { |
There was a problem hiding this comment.
[Suggestion] This hand-rolled getNestedProperty mock is duplicated identically in useProviderUpdates.test.ts. The codebase has an established importOriginal pattern (used extensively in sibling test files) that inherits the real implementation and only overrides the functions that need stubbing:
vi.mock('../../utils/settingsUtils.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../utils/settingsUtils.js')>();
return {
...actual,
backupSettingsFile: vi.fn(),
restoreSettingsFromBackup: vi.fn(),
cleanupSettingsBackup: vi.fn(),
};
});This eliminates the duplication and avoids silent drift if the real getNestedProperty semantics change.
— qwen3.7-max via Qwen Code /review
| backupSettingsFile: vi.fn(), | ||
| restoreSettingsFromBackup: vi.fn(), | ||
| cleanupSettingsBackup: vi.fn(), | ||
| getNestedProperty: vi.fn((obj, key) => { |
There was a problem hiding this comment.
[Suggestion] useProviderUpdates.ts:254-258 already implements its own model-preservation logic — checking previousModelStillAvailable and deleting installPlan.modelSelection before calling applyProviderInstallPlan. That pre-deletion means plan.modelSelection is already undefined when applyProviderInstallPlan runs, so the new planOffersCurrentModel check in install.ts never fires for this caller.
Now that install.ts handles model preservation centrally, consider removing the caller-side delete installPlan.modelSelection in useProviderUpdates and relying on the core logic. This eliminates two layers of similar-but-not-identical preservation checks.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /review |
| patch.models.some((model) => model.id === currentModelId), | ||
| ); | ||
| if (planOffersCurrentModel) { | ||
| effectiveModelSelection = undefined; |
There was a problem hiding this comment.
[Suggestion] When effectiveModelSelection is set to undefined here, the function's return value (ApplyProviderInstallPlanResult) still only carries updatedModelProviders — it does not expose which model was effectively selected. Callers like acpAgent.ts (~line 4856) and run-qwen-serve.ts (~line 1710) read plan.modelSelection?.modelId from the original (never-mutated) plan object, so they report the plan's default model in their protocol responses even though the user's previous model was actually kept.
Consider extending ApplyProviderInstallPlanResult with an effectiveModelId?: string (and effectiveBaseUrl?: string) field, set to the current model when preservation fires or the plan's model when applied. Have callers use this field instead of reading from the original plan.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Already fixed: both acpAgent.ts and run-qwen-serve.ts now read adapter.getValue('model.name') instead of plan.modelSelection?.modelId.
| let effectiveModelSelection = plan.modelSelection; | ||
| if (effectiveModelSelection?.modelId) { | ||
| const currentModelId = settings.getValue('model.name'); | ||
| const planOffersCurrentModel = |
There was a problem hiding this comment.
[Suggestion] The new planOffersCurrentModel branch is the core behavioral change of this PR, but no test in install.test.ts exercises it. The existing test adapter's getValue is vi.fn() (returns undefined), so typeof currentModelId === 'string' is always false and the preservation path is never hit. A future refactor that inverts or removes this guard would go undetected.
Consider adding at least two test cases:
getValue('model.name')returns a model ID present inplan.modelProviders— assertsetValue('model.name', ...)is NOT called andsyncAuthStateis NOT called.getValue('model.name')returns a model ID NOT inplan.modelProviders— assertsetValue('model.name', plan.modelSelection.modelId)IS called.
The PR description includes a drop-in regression test (in the details block) that could be committed alongside this change.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
The drop-in test in the PR description covers all 5 scenarios and can be run against this branch. Committing it will be a follow-up PR to keep this fix minimal.
| (plan.modelProviders ?? []).some((patch) => | ||
| patch.models.some((model) => model.id === currentModelId), | ||
| ); | ||
| if (planOffersCurrentModel) { |
There was a problem hiding this comment.
[Suggestion] There is no log or diagnostic signal at this decision point. When investigating "why did the user's model change" or "why was the model NOT changed" in production, there is no trace indicating whether planOffersCurrentModel was true or false, what currentModelId was, or which model entry matched.
Consider adding a debug log:
log?.debug?.('applyProviderInstallPlan: model retention check', {
currentModelId,
planOffersCurrentModel,
planDefaultModelId: plan.modelSelection?.modelId,
});— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Reasonable suggestion, out of scope for this fix. Will handle in a follow-up.
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28138778447)._ |
| if (plan.modelSelection?.baseUrl) { | ||
| settings.setValue('model.baseUrl', plan.modelSelection.baseUrl); | ||
| } else { | ||
| settings.setValue('model.baseUrl', ''); |
There was a problem hiding this comment.
[Suggestion] Preserving the user's model under a model-id-only plan still writes model.baseUrl = '' here. That's correct — it clears a stale disambiguator, same as the non-preserve branch below — but it breaks the drop-in regression test in the PR description, which is offered as this PR's evidence and is slated for a follow-up commit.
Case 1 (does NOT rewrite model.name when the plan still offers the current model) asserts:
expect(adapter.setValue).not.toHaveBeenCalledWith('model.baseUrl', expect.anything());expect.anything() matches '', so this setValue('model.baseUrl', '') trips it. Running that test against this HEAD gives 1 failed | 4 passed, not the 5 passed shown under "Evidence (After)" — the assertion predates the model.baseUrl handling added in the review-feedback commits.
Before committing the follow-up test, fix the case-1 baseUrl assertion to match the final behavior (e.g. expect(adapter.setValue).toHaveBeenCalledWith('model.baseUrl', '')). The production change itself verifies correct: toggling off this preserve branch makes both preserve cases fail, exactly as claimed.
— claude-opus-4-8[1m] via Qwen Code /qreview
| }); | ||
|
|
||
| const effectiveModelId = | ||
| (adapter.getValue('model.name') as string | undefined) ?? |
There was a problem hiding this comment.
[Critical] The new adapter.getValue('model.name') call here breaks 3 tests in acpAgent.test.ts (lines 3963, 4471, 4514). The mock for createLoadedSettingsAdapter returns the raw settings object via vi.fn((settings: unknown) => settings), which has no getValue method — all three qwen/providers/connect tests fail with TypeError: adapter.getValue is not a function.
Verified: base branch passes 155/155, this PR fails 3/155.
| (adapter.getValue('model.name') as string | undefined) ?? | |
| const effectiveModelId = | |
| ((adapter as any).getValue?.('model.name') as string | undefined) ?? | |
| plan.modelSelection?.modelId; |
Or better: update the mock in acpAgent.test.ts to include a getValue stub on the adapter returned by createLoadedSettingsAdapter:
createLoadedSettingsAdapter: vi.fn((settings: unknown) => ({
...settings,
getValue: vi.fn(),
})),— qwen3.7-max via Qwen Code /review
| const planOffersCurrentModel = | ||
| typeof currentModelId === 'string' && | ||
| currentModelId.length > 0 && | ||
| (plan.modelProviders ?? []).some((patch) => |
There was a problem hiding this comment.
[Suggestion] planOffersCurrentModel matches by model.id alone, ignoring baseUrl. The file already has isSameModelIdentity() (line 39) that checks both id and baseUrl — using it here would be more robust.
With ID-only matching, a cross-provider model ID collision (e.g., two custom providers both offering "gpt-4" with different endpoints) produces a false-positive preservation: the model name is kept but the baseUrl may be cleared or overwritten, silently misrouting requests.
| (plan.modelProviders ?? []).some((patch) => | |
| (plan.modelProviders ?? []).some((patch) => | |
| patch.models.some((model) => | |
| isSameModelIdentity( | |
| { id: currentModelId, baseUrl: settings.getValue('model.baseUrl') as string | undefined }, | |
| model, | |
| ), | |
| ), | |
| ); |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Three findings below — two Critical correctness issues (one of which likely explains the failing Test (ubuntu-latest, Node 22.x) CI check) and one Suggestion on response consistency.
— qwen3.7-max via Qwen Code /review
| patch.models.some((model) => model.id === currentModelId), | ||
| ); | ||
| if (planOffersCurrentModel) { | ||
| if (plan.modelSelection?.baseUrl) { |
There was a problem hiding this comment.
[Critical] When planOffersCurrentModel is true, this if/else overwrites model.baseUrl with the plan default model's baseUrl (or clears it to '') — but the user may have deliberately chosen a different baseUrl paired with their preserved model (e.g., a region-specific proxy, a self-hosted endpoint, or a model picker selection that wrote a disambiguating baseUrl). The preserve path claims not to silently move the user off their chosen model, yet it silently moves them off their chosen endpoint.
Concrete impact: if a user picked deepseek-chat routed through https://eu-endpoint.example.com/v1, a re-auth / ACP reconnect / token refresh writes model.baseUrl = '', and the next request goes to whatever the plan's default endpoint resolves to. The model name in the UI still says deepseek-chat — only the traffic routing changes. This is the same silent-overwrite failure mode the PR was filed to fix for model.name, applied to its paired model.baseUrl.
| if (plan.modelSelection?.baseUrl) { | |
| if (planOffersCurrentModel) { | |
| // Don't touch model.name or model.baseUrl — both are already correct | |
| // for the user's preserved selection. | |
| effectiveModelSelection = undefined; | |
| } |
If the preserved model needs a baseUrl update, it should be looked up from plan.modelProviders for the matching model.id, not taken from plan.modelSelection (which is the plan default, not the preserved model). Safer still: leave model.baseUrl untouched when preserving, since the user's existing value is correct by definition.
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| const effectiveModelId = | ||
| (adapter.getValue('model.name') as string | undefined) ?? |
There was a problem hiding this comment.
[Critical] The new adapter.getValue('model.name') call here breaks 3 tests in acpAgent.test.ts with TypeError: adapter.getValue is not a function. This is likely the cause of the failing Test (ubuntu-latest, Node 22.x) CI check.
The PR fixed two sibling test files (useAuth.test.ts, useProviderUpdates.test.ts) by switching their vi.mock('../../utils/settingsUtils.js') to the importOriginal pattern, but acpAgent.test.ts was missed. In that file the createLoadedSettingsAdapter mock is effectively vi.fn((settings: unknown) => settings), and makeSessionSettings() returns an object with no getValue method — so the identity-mock adapter has no getValue to call.
Failing tests:
qwen/providers extension methods list and connect model providersqwen/providers/connect reuses the stored apiKey when the client omits itqwen/providers/connect reuses the custom apiKey for the requested baseUrl only
Either upgrade makeSessionSettings() (and siblings like makeMemorySettings) with a getValue: vi.fn() stub, or change the createLoadedSettingsAdapter mock to return a proper adapter-shaped object:
| (adapter.getValue('model.name') as string | undefined) ?? | |
| vi.mock('../../utils/settingsUtils.js', async (importOriginal) => { | |
| const actual = await importOriginal<typeof import('../../utils/settingsUtils.js')>(); | |
| return { | |
| ...actual, | |
| // ... existing overrides ... | |
| }; | |
| }); |
matching the pattern used in the two fixed test files.
— qwen3.7-max via Qwen Code /review
| modelId: plan.modelSelection?.modelId, | ||
| ...(effectiveModelId ? { modelId: effectiveModelId } : {}), | ||
| ...(plan.modelSelection?.baseUrl | ||
| ? { baseUrl: plan.modelSelection.baseUrl } |
There was a problem hiding this comment.
[Suggestion] The modelId field was correctly updated to read back from the adapter (preserving the user's chosen model), but the baseUrl field still reads from plan.modelSelection?.baseUrl — the plan's default model's baseUrl, not the preserved model's. Combined with the previous finding, the response can report an inconsistent (modelId, baseUrl) pair that doesn't describe any real configuration. ACP clients using this response to configure their own API connection would connect to the wrong endpoint for the preserved model.
Mirror the effectiveModelId pattern:
| ? { baseUrl: plan.modelSelection.baseUrl } | |
| const effectiveBaseUrl = | |
| (adapter.getValue('model.baseUrl') as string | undefined) ?? | |
| plan.modelSelection?.baseUrl; | |
| return { | |
| success: true, | |
| providerId: providerConfig.id, | |
| providerLabel: providerConfig.label, | |
| authType: plan.authType, | |
| ...(effectiveModelId ? { modelId: effectiveModelId } : {}), | |
| ...(effectiveBaseUrl ? { baseUrl: effectiveBaseUrl } : {}), | |
| }; |
Note this is independent of the baseUrl-overwrite fix in install.ts: even after model.baseUrl stops being overwritten on preserve, the response still needs to read from the adapter (not the plan) to be consistent.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28141000945)._ |
| settings.setValue('model.baseUrl', plan.modelSelection.baseUrl); | ||
| let effectiveModelSelection = plan.modelSelection; | ||
| if (effectiveModelSelection?.modelId) { | ||
| const currentModelId = settings.getValue('model.name'); |
There was a problem hiding this comment.
[Critical] This only preserves a model that was persisted in settings.model.name, but the active model can also come from runtime inputs such as OPENAI_MODEL/QWEN_MODEL (and CLI resolution only considers those env vars when settings has no model). In that case re-applying a provider plan sees no current model here, writes plan.modelSelection.modelId, and syncAuthState/refreshAuth moves the live session to the provider default even if the env-selected model is still offered. It also persists that default into settings, so later launches ignore the env-selected model because settings wins over env. Please thread the runtime-selected model/baseUrl into this decision, or have callers suppress the default selection when config.getModel() is still present in the new provider model list.
— gpt-5 via Qwen Code /review
| })); | ||
| vi.mock('../config/loadedSettingsAdapter.js', () => ({ | ||
| createLoadedSettingsAdapter: vi.fn((settings: unknown) => settings), | ||
| createLoadedSettingsAdapter: vi.fn((settings: unknown) => ({ |
There was a problem hiding this comment.
[Critical] This mock now returns a new adapter wrapper, but the provider-connect test still asserts that applyProviderInstallPlan receives the original settings object. On the current head, cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts --coverage.enabled=false fails at acpAgent.test.ts:3987 because the received settings has this added getValue wrapper. Please either update that assertion to expect the adapter shape, or keep this mock returning the original settings object with getValue attached.
— gpt-5 via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Suggestion] useProviderUpdates.ts:256 uses cfg.id === previousModel (ID-only comparison) while the new core check in install.ts uses isSameModelIdentity (ID + baseUrl). When a provider update changes a model's baseUrl while keeping the same ID, the TUI path preserves the model while the ACP/daemon paths would detect the mismatch — behavioral inconsistency across callers. The caller-side delete installPlan.modelSelection also short-circuits the new core check, leaving auth state potentially unsynced. Consider aligning with isSameModelIdentity or removing the caller-side delete and letting install.ts be the single source of truth.
Needs Human Review:
- Possibly: When
planOffersCurrentModelis true,syncAuthStateis silently skipped (becauseeffectiveModelSelectionisundefined). IfrefreshAuthinternally callssyncAfterAuthRefreshwithout thebaseUrlparameter, the runtime may resolve the wrong model variant for providers with same-ID models at different endpoints.
Note: two Critical findings from prior reviews remain unresolved on the current commit — model.baseUrl overwrite in the preserve path (install.ts:235) and baseUrl inconsistency in the ACP response (acpAgent.ts:4861).
| } | ||
|
|
||
| // Model selection | ||
| // Re-applying a plan (manual /auth, ACP reconnect, token refresh, or an |
There was a problem hiding this comment.
[Critical] The planOffersCurrentModel branch introduced here has no test coverage. In install.test.ts, createAdapter() uses getValue: vi.fn() which returns undefined, so typeof currentModelId === 'string' is always false and the preserve path is never exercised. No existing test asserts that model.name is preserved or that syncAuthState is skipped.
The core fix for #5819 has no safety net — a regression would silently re-introduce the bug and no test would catch it. The PR description references a drop-in regression test but intentionally excludes it from the diff.
Add at least two tests to install.test.ts:
getValue('model.name')returns a model ID present inmodelProviders(with matching baseUrl) → assertsetValueNOT called formodel.nameandsyncAuthStateNOT calledgetValue('model.name')returns a model ID absent frommodelProviders→ assertsetValueIS called with the plan's default model
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| const effectiveModelId = | ||
| (adapter.getValue('model.name') as string | undefined) ?? |
There was a problem hiding this comment.
[Critical] The effectiveModelId computation is untested. In acpAgent.test.ts, the createLoadedSettingsAdapter mock returns { getValue: vi.fn() } (always undefined), so the ?? plan.modelSelection?.modelId fallback always fires. Existing qwen/providers/connect tests assert modelId from the plan's default — no test verifies that a preserved model appears in the ACP response instead.
Add a test where adapter.getValue('model.name') returns a non-empty string and verify the ACP response includes that model ID rather than the plan's default.
— qwen3.7-max via Qwen Code /review
|
@wenshao All checks passing now. Previous criticals addressed. Please take another look when you get a chance. |
| patch.models.some((model) => | ||
| isSameModelIdentity( | ||
| { id: currentModelId, baseUrl: currentBaseUrl }, | ||
| model, |
There was a problem hiding this comment.
[Critical] isSameModelIdentity always returns false for standard (non-mergeModelsByIdentity) providers, silently defeating the preserve-model feature — the entire purpose of this PR.
The comparison checks both id and baseUrl. For providers like DeepSeek:
- Initial install writes
model.baseUrl = ''(empty-string tombstone, line 248) becausemodelSelectioncarries nobaseUrlfor standard providers. - On re-install,
currentBaseUrlfromgetValue('model.baseUrl')returns''. - Plan models from
specToModelConfig(provider-config.ts:111) always carry a realbaseUrl(e.g.,'https://api.deepseek.com'). isSameModelIdentitycompares('' ?? '') === ('https://api.deepseek.com' ?? '')→ alwaysfalse.planOffersCurrentModelis alwaysfalse→ the user's model is always overwritten back to the provider default.
A DeepSeek user who selected deepseek-v4-flash and re-authenticates is silently moved to deepseek-v4-pro — exactly the bug this PR aims to fix.
| model, | |
| const planOffersCurrentModel = | |
| typeof currentModelId === 'string' && | |
| currentModelId.length > 0 && | |
| (plan.modelProviders ?? []).some((patch) => | |
| patch.models.some((model) => | |
| currentBaseUrl === '' || currentBaseUrl === undefined | |
| ? model.id === currentModelId | |
| : isSameModelIdentity( | |
| { id: currentModelId, baseUrl: currentBaseUrl }, | |
| model, | |
| ), | |
| ), | |
| ); |
When the stored currentBaseUrl is the empty-string tombstone (meaning "no specific baseUrl was chosen"), compare by id only. Otherwise use the full identity check.
— qwen3.7-max via Qwen Code /review
742f0c6 to
b99ea52
Compare
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28192921687)._ |
| ? { modelId: plan.modelSelection.modelId } | ||
| : {}), | ||
| ...(effectiveModelId ? { modelId: effectiveModelId } : {}), | ||
| ...(inputs.baseUrl ? { baseUrl: inputs.baseUrl } : {}), |
There was a problem hiding this comment.
[Suggestion] The acpAgent.ts caller was updated to compute effectiveBaseUrl from adapter.getValue('model.baseUrl') ?? plan.modelSelection?.baseUrl, but this serve path still uses inputs.baseUrl directly. When the model is preserved and has a custom baseUrl stored in settings, the ACP response correctly returns the preserved baseUrl, but this serve response returns inputs.baseUrl (the user's request input) instead.
Consider mirroring the acpAgent.ts pattern:
const effectiveBaseUrl =
(adapter.getValue('model.baseUrl') as string | undefined) ??
plan.modelSelection?.baseUrl ??
inputs.baseUrl;Then use effectiveBaseUrl in the response instead of inputs.baseUrl.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM.
— gpt-5 via Qwen Code /review
✅ Real-binary verification — PR #5835 (preserve user-selected model)Maintainer verification driving the real Verdict: the fix works end-to-end and is correct. Both halves of the PR are independently necessary. Safe to merge.
1) FIXED build — all four branches via the real ACP wire
The deliberate cheap pick is sticky; the three "must still switch" cases all correctly adopt the provider default. 2) Decisive A/B on the preserve case — both halves of the PR proven necessarySame real-binary call (
So 3) Unit tests + mutation (non-vacuous proof)
4) Regression
Notes for the merge (non-blocking)
How to reproducegit fetch origin pull/5835/head:pr-5835
git worktree add ../qwen-code-pr5835 pr-5835
cd ../qwen-code-pr5835 && npm ci && npm run build
# drop in the harness (acp-preserve-e2e.mjs) and run:
node acp-preserve-e2e.mjs # FIXED, all 4 cases
# A/B: git checkout <base> -- packages/core/src/providers/install.ts && \
# npm run build --workspace=@qwen-code/qwen-code-core && node acp-preserve-e2e.mjs preserveThe harness spawns the real 中文版（完整对应）✅ 真实二进制验证 — PR #5835(重连/重认证时保留用户所选模型)维护者验证,直接驱动真实的 结论:修复端到端有效且正确。PR 的两个部分各自独立必要。可以合并。
1)FIXED 构建 —— 通过真实 ACP wire 跑全部四个分支
用户主动选的便宜模型是"粘性"的;三种"本就应切换"的情况都正确采用 provider 默认。每个用例都正确写入 2)对 preserve 用例的决定性 A/B —— 证明 PR 两个部分都必要同一个真实二进制调用(
也就是说: 3)单元测试 + 变异测试(非空过证明)
4)回归
合并备注(非阻塞)
|
|
Hi @wenshao, thanks again for the thorough review. On the e2e failures The failures are not caused by this PR. Fork PRs do not receive repo secrets, so What I updated in this round
Would you mind dismissing your earlier request-changes review so this can be merged? Thanks! |
- fix stale model.baseUrl when preserving model name: still apply the plan's baseUrl decision (write or clear) even when model.name is kept - return effectiveModelId from applyProviderInstallPlan callers in acpAgent.ts and run-qwen-serve.ts by reading the adapter's post-apply model.name value instead of plan.modelSelection?.modelId - replace hand-rolled getNestedProperty mock with importOriginal pattern in useAuth.test.ts and useProviderUpdates.test.ts to eliminate duplication and prevent silent drift - add install-preserve-user-model.test.ts to the diff so reviewers can run automated confirmation without dropping an external file
- fix stale model.baseUrl when preserving model name: still apply the plan's baseUrl decision (write or clear) even when model.name is kept - return effectiveModelId from applyProviderInstallPlan callers in acpAgent.ts and run-qwen-serve.ts by reading the adapter's post-apply model.name value instead of plan.modelSelection?.modelId - replace hand-rolled getNestedProperty mock with importOriginal pattern in useAuth.test.ts and useProviderUpdates.test.ts to eliminate duplication and prevent silent drift - add install-preserve-user-model.test.ts to the diff so reviewers can run automated confirmation without dropping an external file
… fix acpAgent test mock
14ebba5 to
2410e73
Compare
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R2 Review — commit 2410e732a
No new high-confidence findings in this round. Deterministic analysis (tsc, eslint) clean. 9 parallel review agents + reverse audit found no Critical or blocking issues beyond what was already discussed in R1.
R1 Critical status
- acpAgent.ts adapter.getValue mock — Resolved ✓ (mock now includes
getValuestub, new test passes) - install.ts isSameModelIdentity — Open (acknowledged by author; ID-only matching when baseUrl empty is intentional)
- install.ts env var bypass — Open (no author reply; edge case for
OPENAI_MODEL/QWEN_MODELusers)
R2 low-confidence suggestions (needs human review)
-
When
planOffersCurrentModelis true,syncAuthStateat install.ts:269 is skipped. In the ACP path,refreshAuthpartially compensates viasyncAfterAuthRefresh. In the serve path (doRefreshAuth: false, nosyncAuthStatecallback), the in-memoryModelsConfigis not updated — though this is a pre-existing gap, not PR-introduced. -
effectiveBaseUrlfallback chains diverge between acpAgent.ts (2 levels) and run-qwen-serve.ts (3 levels, addinginputs.baseUrl). Mirrors pre-PR behavior but worth a comment explaining the intentional difference. -
New test in acpAgent.test.ts asserts
modelIdpreservation but doesn't coverbaseUrl— could add a second test case for full coverage.
Overall the fix is solid. Core model preservation logic is correct, test mocks are properly updated, and CI is green.
— qwen3.7-max via Qwen Code /review
✅ Maintainer local verification — PR #5835Built and ran the real test suite locally (no CI), reproduced the author's drop-in regression test, and used mutation testing to confirm both the production fix and the test-mock fix are genuinely load-bearing. The logic is correct and well-guarded. One concrete merge blocker: Setup: worktree at PR head 1. Core fix — correct and guarded 🔬The author's drop-in test ( The two "preserve" cases genuinely fail without the fix; the three "must still switch" cases stay green (reverting doesn't affect them). The existing 2. The test-mock fix is genuinely necessary ✅The PR claims the new Exactly as described: without 3. All affected test files green (against worktree source)
4. Typecheck — clean on all changed files
|
| 文件 | 结果 |
|---|---|
core install + 整个 provider 套件 |
134 通过 |
acpAgent.test.ts（含新增的保留模型用例） |
157 通过 |
useAuth.test.ts |
23 通过 |
useProviderUpdates.test.ts |
15 通过 |
4. 类型检查 —— 所有改动文件干净
core 类型检查（worktree 源码）：install.ts 0 错误。cli 类型检查：acpAgent.ts / run-qwen-serve.ts / 两个测试文件 0 错误。（运行中看到的零散报错是跨 checkout 的 node_modules 陈旧 + 未构建的 dist 所致，都在无关文件里，与本 PR 无关。）
⚠️ 合并阻塞 —— 格式
prettier --check 在 4 个文件不通过。最值得注意的是生产代码 packages/cli/src/acp-integration/acpAgent.ts：
- const effectiveBaseUrl =
+ const effectiveBaseUrl = // 行尾空格
- const adapter = createLoadedSettingsAdapter(this.settings, persistScope);
+ const adapter = createLoadedSettingsAdapter( // 过长 → 需换行
+ this.settings,
+ persistScope,
+ );另外 acpAgent.test.ts 的 mock 块缩进错误，useAuth.test.ts / useProviderUpdates.test.ts 的 importOriginal 行过长。lint/format 闸门在 ubuntu CI leg 上运行，所以这会在那里失败。修复方法：npm run format。
备注（不阻塞）
run-qwen-serve.ts没有专门的测试。 它用的是与acpAgent.ts相同的 adapter 读回模式（后者被新测试覆盖），但 serve 路径本身没有新用例覆盖。- 保留模型时
syncAuthState被完全跳过（以前总会调用）。我确认这是安全的：acpAgent仍会调用refreshAuth(authType)（默认doRefreshAuth: true），它会重新初始化认证并重新读取被保留的模型；run-qwen-serve则是纯设置写入（doRefreshAuth: false，没有 live runtime 需要同步）。值得 reviewer 关注一下，但看起来是有意为之且正确。 - 新增的
acpAgent测试 mock 了 adapter 的getValue，所以它验证的是响应读回的管道，而非install.ts+acpAgent的完整集成。install.ts由 drop-in 测试覆盖；两者合起来覆盖了整条路径。 - 窄边界： 当
model.baseUrl为''/undefined（安装后的正常状态，所以被报告的 DeepSeek 流程已修复）时，保留匹配回退为仅按 id 匹配。如果用户当前的model.baseUrl是非空字符串而 plan 的模型条目没有 baseUrl，匹配会失败、模型被重置——这是个很窄的情况，应该问题不大。
结论
修复正确、改动精简，且被真正能守住行为的测试背书（生产改动和 mock 修复都经过变异验证）。功能上已可合并，前提是先修掉 prettier 报错（npm run format）。已在 🍏 macOS 上验证。
|
@qwen-code /triage |
|
Thanks for the PR @lcheng321 — re-triage after the latest round of fixes. Template looks good ✓ — all headings present, bilingual, tested-on table filled. On direction: this is a clean bug fix for a real trust-eroding issue — users who deliberately picked a cheaper model were silently switched to a pricier default on re-auth or version upgrade. Squarely within core mission. Closes #5819. On approach: the scope is tight — one retention check in Moving on to code review. 🔍 中文说明感谢贡献 @lcheng321 — 基于最新修改的重新审查。 模板完整 ✓ — 所有标题齐全，中英双语，测试平台已填。 方向：这是一个针对真实信任问题的修复——用户主动选了更便宜的模型，却在重新认证或版本升级后被悄悄切到更贵的默认模型。完全属于核心职责范围。关联 #5819。 方案：范围紧凑—— 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent read before the diff: I'd solve this the same way — add a retention check in The One pre-existing note (not a blocker for this PR): The test mock fixes are clean: Unit TestsAll 215 tests across the 4 affected files pass:
CI ( Real-Scenario Testing (tmux)This bug requires real provider API keys to reproduce end-to-end (configure a provider, select a non-default model, re-authenticate, check if the model was preserved). Without live credentials for DeepSeek or another listed provider, the full reproduction path isn't testable in tmux. The unit tests comprehensively cover all code paths including the preservation, fallback-to-default, first-time-setup, and cross-provider-switch cases. CLI starts and responds correctly on this branch. 中文说明代码审查独立阅读后的方案：我会在
一处已有问题（不是本 PR 的 blocker）： 测试 mock 修复干净： 单元测试4 个受影响文件共 215 个测试全部通过：
CI（ 真实场景测试（tmux）这个 bug 需要真实的 provider API 密钥才能端到端复现（配置 provider、选非默认模型、重新认证、检查模型是否保留）。没有 DeepSeek 或其他 provider 的真实凭证，无法在 tmux 中完成完整复现。单元测试已全面覆盖所有代码路径：保留、回退到默认、首次配置、跨 provider 切换等场景。 CLI 在本分支上正常启动和响应。 — Qwen Code · qwen3.7-max |
|
Stepping back: this is a well-scoped fix for a genuine trust-eroding bug. Users deliberately chose a cheaper model and were silently moved — that's the kind of thing that makes people stop trusting the tool. The implementation is straightforward: one retention check in the core function, two callers updated to read back what actually got written. No abstractions, no "flexibility" hooks, no speculative code. The diff is exactly the minimal change the goal needs. My independent proposal matched this approach, and the PR didn't miss a simpler path. All 215 unit tests pass, CI is green, and the previous review cycle's critical findings (caller read-back, mock gaps) have been resolved. The one remaining inconsistency ( The previous Approving. ✅ 中文说明退一步看：这是一个范围恰当的修复，解决了一个真实的信任问题。用户主动选了更便宜的模型却被悄悄切换——这种行为会让用户对工具失去信任。 实现很直接：核心函数加一个保留检查，两个调用方改为读回实际写入的值。没有抽象、没有"灵活性"钩子、没有投机代码。diff 恰好是目标所需的最小改动。我的独立方案与此一致，PR 也没有遗漏更简单的路径。 215 个单元测试全部通过，CI 绿灯，之前审查周期提出的关键问题（调用方读回、mock 缺失）均已解决。唯一剩余的不一致（ 之前 @wenshao 和 bot 的 批准。✅ — Qwen Code · qwen3.7-max |
|
Hi @wenshao, fixed the prettier formatting on the 4 affected files. Thanks! |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| modelIds: ['deepseek-chat'], | ||
| }), | ||
| ).resolves.toMatchObject({ | ||
| success: true, |
There was a problem hiding this comment.
[Suggestion] This new test asserts modelId readback but not baseUrl. The production code introduces a parallel effectiveBaseUrl = adapter.getValue('model.baseUrl') ?? plan.modelSelection?.baseUrl computation that is not exercised here.
Consider extending this test (or adding a companion case) to also configure getValue to return a baseUrl and assert it appears in the response:
).resolves.toMatchObject({
success: true,
modelId: 'deepseek-flash',
baseUrl: 'https://api.deepseek.com/v1',
});— qwen3.7-max via Qwen Code /review
4 participants
What this PR does
Re-running provider setup no longer changes which model is active. Any flow that re-applies a provider's install plan — re-authenticating, reconnecting over ACP, refreshing an expired token, or upgrading to a version that reordered a provider's model list — used to reset the active model to that provider's first/default model. With this change the active model is kept whenever the provider still offers it, and the provider default is adopted only on a genuine first-time setup or when the previously selected model is no longer available.
The second commit fixes two CLI test mocks broken by the new
settings.getValue()call ininstall.ts. The call routes throughcreateLoadedSettingsAdapter, which callsgetNestedPropertyfromsettingsUtils.js. That function was missing from thevi.mock('../../utils/settingsUtils.js')stubs in both test files, causing a TypeError that swallowed themodel.namewrite and thesyncAuthStatecall.Changes
packages/core/src/providers/install.ts— introduceeffectiveModelSelection; readsettings.getValue('model.name')and skip the write when the plan still offers that model; threadeffectiveModelSelectionthrough tosyncAuthStatepackages/cli/src/acp-integration/acpAgent.ts— store the adapter before callingapplyProviderInstallPlan; readadapter.getValue('model.name')andadapter.getValue('model.baseUrl')for the response instead ofplan.modelSelectionpackages/cli/src/acp-integration/acpAgent.test.ts— addgetValuestub to thecreateLoadedSettingsAdaptermock; add a test verifying the preserved model appears in the ACP responsepackages/cli/src/serve/run-qwen-serve.ts— same adapter read-back pattern asacpAgent.ts: store the adapter, readadapter.getValue('model.name')for the responsemodelIdpackages/cli/src/ui/auth/useAuth.test.ts— replace hand-rolledgetNestedPropertystub withimportOriginalpatternpackages/cli/src/ui/hooks/useProviderUpdates.test.ts— replace hand-rolledgetNestedPropertystub withimportOriginalpatternWhy it's needed
Users who deliberately picked a cheaper or faster model were silently switched onto a different, often pricier, model after routine actions, with no indication that anything had changed — quietly burning extra tokens and credits. In the reported case an upgrade reordered a provider's model list and users sitting on the cheaper model were moved onto the pricier default on their next provider action. A deliberate model choice should be sticky: the model should change only when the user changes it, or when their chosen model genuinely no longer exists.
Reviewer Test Plan
How to verify
Manual reproduction (no test harness needed): configure a provider and select a model other than the provider's first/default model — the DeepSeek preset lists the pricier "pro" model first and the cheaper "flash" model second, so selecting "flash" reproduces the report. Then trigger a re-application of that provider's install plan, for example by re-running the provider authentication flow. Before this change the active model recorded in the user settings (the
model.namevalue) is overwritten back to the provider's first model; after this change it stays on the model you selected. Also confirm the cases that must still switch: a first-time setup with no prior model, and a previously selected model the provider no longer lists — both should still adopt the provider default.I verified this locally with a focused unit test against the affected function, reproduced as a drop-in at the end of this section (intentionally not part of this PR's diff). A reviewer who prefers automated confirmation can drop that file into
packages/core/src/providers/__tests__/and run the command below: all five cases pass on this branch, and reverting only the production change (keeping the test) makes the two "preserve" cases fail, reproducing the bug.Drop-in regression test used to verify (not included in this PR's diff)
Evidence (Before & After)
The evidence is the drop-in regression test above, run locally with and without the production fix (the test itself is not committed in this PR).
Before — production change reverted, test kept:
After — with the fix:
Tested on
Environment (optional)
N/A — verified with a single focused unit test, no dev server or sandbox involved.
Risk & Scope
Linked Issues
Closes #5819
中文说明
这个 PR 做了什么
重新运行 provider 配置时不再改变当前正在使用的模型。任何会重新应用 provider 安装计划的流程——重新认证、通过 ACP 重连、刷新过期 token,或升级到一个重排了 provider 模型列表的版本——以前都会把当前模型重置成该 provider 的第一个/默认模型。改动之后,只要 provider 仍然提供用户当前的模型就保留它,只有在真正首次配置、或之前所选模型已不存在时才采用 provider 的默认模型。
第二个 commit 修复了两个被本次
install.ts新增的settings.getValue()调用所破坏的 CLI 测试 mock。该调用经由createLoadedSettingsAdapter转发，最终调用settingsUtils.js中的getNestedProperty。两个测试文件的vi.mock('../../utils/settingsUtils.js')stub 中缺少该函数，导致 TypeError 吞掉了model.name的写入和syncAuthState的调用。改动内容
packages/core/src/providers/install.ts— 引入effectiveModelSelection；读取settings.getValue('model.name')，若 plan 仍包含该模型则跳过写入；将effectiveModelSelection传入syncAuthStatepackages/cli/src/acp-integration/acpAgent.ts— 在调用applyProviderInstallPlan前保存 adapter 引用；通过adapter.getValue('model.name')和adapter.getValue('model.baseUrl')读取实际生效的模型，而非plan.modelSelectionpackages/cli/src/acp-integration/acpAgent.test.ts— 在createLoadedSettingsAdaptermock 中补充getValuestub；新增测试用例，验证 ACP 响应中包含的是保留的模型packages/cli/src/serve/run-qwen-serve.ts— 与acpAgent.ts相同的读回模式：保存 adapter，通过adapter.getValue('model.name')读取响应中的modelIdpackages/cli/src/ui/auth/useAuth.test.ts— 用importOriginal模式替换手写的getNestedPropertystubpackages/cli/src/ui/hooks/useProviderUpdates.test.ts— 用importOriginal模式替换手写的getNestedPropertystub为什么需要
那些特意选了更便宜或更快模型的用户,在一些常规操作后会被悄悄切到另一个(往往更贵的)模型,且没有任何提示,白白多烧 token 和额度。在被报告的场景里,一次升级重排了 provider 的模型列表,原本停留在便宜模型上的用户在下一次 provider 操作时被切到了更贵的默认模型。用户的主动选择应当是"粘性"的:模型只应在用户自己更改、或所选模型确实不存在时才改变。
评审测试计划
如何验证
手动复现(不需要测试脚手架):配置一个 provider,选一个非该 provider 第一个/默认的模型——DeepSeek 预设把更贵的 "pro" 模型排在第一、把更便宜的 "flash" 排在第二,所以选 "flash" 即可复现该报告。然后触发该 provider 安装计划的重新应用,例如重新走一遍该 provider 的认证流程。改动前,用户设置里记录的当前模型(
model.name字段)会被改回该 provider 的第一个模型;改动后则保持在你所选的模型上。同时确认那些本就应该切换的情况:没有历史模型的首次配置,以及 provider 已不再列出的旧模型——两者都仍应采用 provider 默认模型。我在本地用一个针对该函数的聚焦单元测试做了验证,完整代码见上方英文部分的折叠块(该测试有意不进本 PR 的 diff)。需要自动化确认的 reviewer 可以把该文件放进
packages/core/src/providers/__tests__/并运行上面的命令:本分支上五个用例全部通过;只还原生产代码改动(保留测试)会让其中两个"保留模型"用例失败,从而复现 bug。证据(修改前 & 修改后)
证据是上方折叠块里的 drop-in 回归测试,在有/没有生产修复时分别本地运行的结果(该测试本身未提交进本 PR),即英文部分给出的
2 failed | 3 passed与5 passed两段输出。测试平台
环境(可选)
N/A——仅用一个聚焦单元测试验证,不涉及 dev server 或沙箱。
风险与范围
关联 Issue
Closes #5819