fix(core,vscode): keep provider update versions in sync - #8889
Conversation
The version recorded by a provider install hashed the installed model list (built-ins plus every owned model), while the launch check that detects a pending update hashed the built-in template alone. A user owning any id outside the current built-in list — including a built-in that a release renamed away — therefore stored a version that could never equal the recomputed one, so "Update all" did not clear the prompt and it returned on every launch. Record the built-in template version instead, and route both sides through a single computeProviderTemplateVersion so the two inputs cannot drift apart again. Custom models are still carried through the update; no new field is persisted. Relates to #8504
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Re-run at the restructured head ( Template looks good ✓ (the earlier nit is fixed too — there is now a proper Problem: observed bug with hard evidence — #8504 reproduces the repeating update prompt step by step and even shows the two divergent hashes (the stored version hashes built-ins + custom model, the launch check hashes the template alone). The extension half is the same class of bug: a global-region sign-in wrote no Direction: aligned — "accepting an update must clear the prompt" is a basic correctness expectation of this flow, and the region-fallback false prompt hits users right after sign-in. No auth/sandbox/telemetry/public-contract surface changes. Size: 175 production lines ( Approach: this is now exactly the design the review thread converged on — Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明在重构后的 head( 模板完整 ✓(此前的小瑕疵也已修复——现在有了独立的 问题:已观测到的 bug,证据扎实——#8504 提供了逐步复现,甚至给出两个分歧的哈希值(存储版本对"内置 + 自定义模型"做哈希,启动检查只对模板做哈希)。扩展端是同类问题:Global 区域登录未写入 方向:对齐——"接受更新后提示必须消失"是该流程的基本正确性预期;区域回退导致的误报在登录后立即伤害用户信任。不涉及 auth/sandbox/telemetry/公共契约面。 规模:生产逻辑 175 行( 方案:现已与评审线程收敛出的方案完全一致—— 风险:无升级风险信号——改动文件均未命中与回滚相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-reviewed at My independent take before reading the diff: keep Walking the mechanics:
On the final "never switch model" commit ( Testing evidenceThe PR's own CI on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The suite pins the hash invariants at the writer/launch-check boundaries, but not the end-to-end behaviour users see. A sandboxed Not independently verified here: the launch-twice behaviour (CI cannot run PR code in this lane), the removed-selected-model path beyond the new unit test, and Windows/Linux, which the author did not exercise. 中文说明代码审查在 看 diff 前的独立思路: 逐项核对机制:
关于最后的"绝不切换模型"提交( 测试证据
单测锁定的是写入方/启动检查边界的哈希不变量,而非用户可见的端到端行为。一个沙箱 此处未独立验证:两次启动行为(本环节不能运行 PR 代码)、"选中模型被删除"路径除新单测之外的行为、以及作者未验证的 Windows/Linux。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean restructured fix with every raised blocker resolved and the last behavioural decision now test-pinned; one non-blocking copy nit named below. Stepping back: the hard parts of this PR are genuinely well done. The two-semantics design ("a normal install hashes what was installed; a template refresh hashes the template") is exactly the shape the review thread converged on, and it's mechanically tight — the stamp keying holds, the non-null assertions are justified, the subset-install regression is test-pinned at the right boundary, and the IDE writer can no longer record a version the CLI launch check cannot reproduce (verified for all four plan/region pairs, with the region My one reservation from the previous head — the "never switch model" commit — was settled by the follow-up test: the decision is deliberate and documented ("Template updates do not carry a model-selection intent"), and it sidesteps the The ubuntu unit suite is still in flight on this head, so approval is deferred until CI lands green on (Note: the two standing 中文说明置信度:4/5 —— 重构后的修复干净,此前提出的所有阻塞问题均已解决,最后一次行为决策也已被测试锁定;仅剩一个非阻塞的文案问题(见下)。 退一步看:这个 PR 的难点处理得确实好。双语义设计("普通安装对实际安装内容做哈希;模板刷新对模板做哈希")正是评审线程收敛出的形态,机制上严丝合缝——版本戳键位正确、非空断言成立、子集安装回归在正确的边界被测试锁定,IDE 写入方再也不可能记录下 CLI 启动检查无法复现的版本(已在全部四个 plan/region 组合上验证,且区域 我对上一个 head 的唯一保留意见——"绝不切换模型"提交——已被后续测试化解:该决策是有意的、有据可查的("模板更新不携带模型选择意图"),并且避开了 该 head 上 ubuntu 单测仍在运行,因此批准推迟到 CI 在 (注:bot 现有的两个 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
yiliang114
left a comment
There was a problem hiding this comment.
The reported update-loop bug is real, but the fix is too broad: global template stamping regresses deliberate subset installs, so I left one merge-blocking P1 inline. The ponytail note shows that an update-only fix can also drop about 96 lines and avoid a new public core API.
Verified at 1df9c1d: changed-file ESLint, core/CLI typecheck, provider-config.test.ts (80/80), and useProviderUpdates.test.ts (26/26) all pass; the full PR CI is green. Those checks do not exercise the subset path described inline.
Keep ordinary install metadata tied to the models in the install plan, while confirmed provider updates persist the detected built-in template version.
The second commit (71bd8f1) reverted the core fix from 1df9c1d: it deleted computeProviderTemplateVersion and restored the old resolveProviderState that hashes the full model list (built-ins + custom). The CLI executeUpdate path was patched post-hoc, but setup wizard, VS Code companion, and ACP flows still went through the broken path — storing a version that would never match the template-only launch-time check whenever custom models were present. This commit: 1. Restores computeProviderTemplateVersion with the invariant comment. 2. Restores resolveProviderState to hash only the built-in template. 3. Removes the post-hoc version patching in executeUpdate (no longer needed). 4. Restores the comprehensive invariant tests for every built-in provider. 5. Re-exports computeProviderTemplateVersion from the core package index.
|
The second commit (71bd8f1) accidentally reverted the core fix from 1df9c1d — it deleted 244451c restores the original approach:
中文第二个 commit (71bd8f1) 意外回滚了 1df9c1d 的核心修复——删除了 244451c 恢复了原始方案:
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): Change summary: This PR reworks provider template version...: did not execute the two touched test files — the review worktree has no node_modules and I did not install dependencies; verification is read-based (CI runs t….
中文说明
未探索到全部深度(达到工具调用预算):Change summary: This PR reworks provider template version...:did not execute the two touched test files — the review worktree has no node_modules and I did not install dependencies; verification is read-based (CI runs t…。
— qwen3.8-max via Qwen Code /review (v0.21.9)
The source-import pass left `resolveMetadataKey as resolveMetadataKeySrc` imported twice and never imported `PROVIDER_METADATA_NS`, so `tsc --build` failed on the core workspace and took Test / web-shell E2E / Post Coverage down with it.
- Switch provider-config test to source import (computeProviderTemplateVersionSrc) to match the file's convention for dist-bypass imports - Replace hand-composed version computation in CLI test fixtures with computeProviderTemplateVersion to prevent drift
The IDE kept its own copy of the subscription plan model lists and hashed that copy for `providerMetadata.<plan>.version`. Neither matched the preset the CLI reads, so signing in from the IDE wrote a version the CLI could never reproduce — an update prompt on the next launch — and persisted model entries that were missing fields the preset carries. The copy had also fallen behind: it still listed `deepseek-v4-flash`, renamed to `deepseek-v4-flash-0731` in 0.21.8, so the IDE installed a model id that no longer exists. Derive both the template and the version from the matching core provider, found by baseUrl + envKey, and delete the duplicated definitions along with the "keep in sync" comment that admitted the drift. The test no longer pins model ids — pinning them is what let the copy drift — and asserts against the shared preset instead.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): PR #8889 (fix for issue #8504's update-prompt loop) makes...: none — all checks above completed within budget. I did not run the test suites or an E2E global-region sign-in; the Critical's chain is verified statically end-…; PR #8889 (fix for issue #8504's update-prompt loop) makes...: did not run typecheck/unit tests (verification agents cover this); did not read __tests__/presets/alibaba-coding-plan.test.ts and alibaba-token-plan.test.ts …; PR #8889 (fix for issue #8504's update-prompt loop) makes...: none — no check was left unfinished..
Test Plan (not a blocker): 161 passed — this review observed 18897, 19585, 482, 1200, 1502, 3069, 473 passed; 26 passed — this review observed 18897, 19585, 482, 1200, 1502, 3069, 473 passed; 125 passed — this review observed 18897, 19585, 482, 1200, 1502, 3069, 473 passed.
[Critical] R1-5 resolveProviderState still stamps the full built-in template hash for every buildInstallPlan caller (update flow, setup wizard, VS Code companion, ACP, serve), not only confirmed updates — a modelsEditable subset install (built-ins a,b; user deselects b) installs only [a] but stores hash(template[a,b]), and findAllPendingUpdates exits at metadata.version === currentVersion, so the deselected built-in is never offered again until the template itself changes. Still-standing round-1 blocker: the author's unresolved merge-blocking P1 thread (provider-config.ts:238, comment 3751481461); the anchored file is unchanged since round 1 reviewed 244451c, and the mechanism was re-verified at this head. Fix as proposed in the thread: restore resolveProviderState to hashing the plan models, pass PendingUpdate.currentVersion into executeUpdate, overwrite only that update plan's version before applyProviderInstallPlan, and add a subset-install regression test.
— qwen3.8-max via Qwen Code /review (v0.21.9)
chiga0
left a comment
There was a problem hiding this comment.
Reviewed at 3646f92.
Verdict
No blocking issues. The PR correctly separates the two provider-version semantics: normal installs now record the hash of the actually-installed model list, while a confirmed template update stamps the current built-in template hash. The VS Code companion derives its plan template and version from the core presets and persists the selected region's baseUrl, so the CLI launch check can reproduce the global-region version. Earlier Critical findings are addressed.
Findings
Minor
-
Implicit version overwrite in
executeUpdate(packages/cli/src/ui/hooks/useProviderUpdates.ts)
buildInstallPlancomputes a version from the combined[...defaultIds, ...customIds]list, and the update flow immediately overwrites it withpending.currentVersion. This works, but the two-step arrangement is fragile: it relies oninstallPlan.providerStatebeing non-null and on the reader knowing that the overwrite is the intended semantic boundary. Consider making the intent explicit (e.g., a dedicated helper or a comment at the overwrite site) so future changes tobuildInstallPlando not accidentally reintroduce the global-stamping regression. -
Unconditional model-selection preservation (
packages/cli/src/ui/hooks/useProviderUpdates.ts)
Patch 9/9 deletesinstallPlan.modelSelectionunconditionally with the comment "Template updates never change the selected model." This intentionally prevents model switches, but the test file still contains a case named "switches model when previous model is no longer available" that expects a fallback switch. If the fallback is now driven byapplyProviderInstallPlanorsyncAfterAuthRefresh, the test name is misleading; otherwise the test and source are out of sync. -
Test import ordering (
packages/core/src/providers/__tests__/provider-config.test.ts)
buildInstallPlanSrcis used at line 72 but imported at line 686. ESM hoisting makes this legal, but it is confusing for readers and easy to break if the file is ever rearranged. Move source-only imports to the top of the module alongside the package-name imports.
Cross-validation with existing review
The earlier CHANGES_REQUESTED review (4898992577) raised two key points:
- P1 — global template stamping regressing subset installs: Resolved.
resolveProviderStatenow hashes the actual installed model list, and onlyexecuteUpdateoverwrites the stored version with the template hash for the specific provider being refreshed. - P2 — all-provider invariant matrix was testing the discarded global design: Resolved. The large matrix was removed in later commits; coverage is now focused on the
useProviderUpdatesupdate path and the subset-install assertion inprovider-config.test.ts.
Additional earlier suggestions (modelNamePrefix removal, sourcing constants from core, persisting baseUrl, testing global-region consumption) are all addressed in the current HEAD.
Summary
- Critical / Major: 0
- Minor: 3
- Nit: 0
No changes requested; the remaining items are polish or clarifications.
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; PR #8889 (fix #8504's provider update-prompt loop) separa...: I didn't run the full repo-wide preflight (build + lint + full test suite) — only the three changed test files and targeted tsc for cli / companion.; PR #8889 (fix #8504's provider update-prompt loop) separa...: did not run the full repo-wide preflight (lint + complete suite) — only the three changed test files and targeted typechecks..
Test Plan (not a blocker): 65 passed — this review observed 19572, 483, 1200, 1502, 3069, 473 passed; 27 passed — this review observed 19572, 483, 1200, 1502, 3069, 473 passed; 21 passed — this review observed 19572, 483, 1200, 1502, 3069, 473 passed.
中文说明
已审查。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.;PR #8889 (fix #8504's provider update-prompt loop) separa...:I didn't run the full repo-wide preflight (build + lint + full test suite) — only the three changed test files and targeted tsc for cli / companion.;PR #8889 (fix #8504's provider update-prompt loop) separa...:did not run the full repo-wide preflight (lint + complete suite) — only the three changed test files and targeted typechecks.。
Test Plan(非阻断):65 passed — this review observed 19572, 483, 1200, 1502, 3069, 473 passed; 27 passed — this review observed 19572, 483, 1200, 1502, 3069, 473 passed; 21 passed — this review observed 19572, 483, 1200, 1502, 3069, 473 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
Resolved round 3 on |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): PR #8889 fixes issue #8504's provider update-prompt loop:...: none — all planned checks completed (~24 tool calls).; PR #8889 fixes issue #8504's provider update-prompt loop:...: trace ModelsConfig runtime fallback for a model.name absent from modelProviders (would refine the cost of Finding 1 — greps in contentGenerator.ts came ba….
Test Plan (not a blocker): 65 passed — this review observed 483, 1200, 1502, 3069, 473 passed; 27 passed — this review observed 483, 1200, 1502, 3069, 473 passed; 21 passed — this review observed 483, 1200, 1502, 3069, 473 passed.
中文说明
已审查。 建议见行内评论。
未探索到全部深度(达到工具调用预算):PR #8889 fixes issue #8504's provider update-prompt loop:...:none — all planned checks completed (~24 tool calls).;PR #8889 fixes issue #8504's provider update-prompt loop:...:trace ModelsConfig runtime fallback for a model.name absent from modelProviders (would refine the cost of Finding 1 — greps in contentGenerator.ts came ba…。
Test Plan(非阻断):65 passed — this review observed 483, 1200, 1502, 3069, 473 passed; 27 passed — this review observed 483, 1200, 1502, 3069, 473 passed; 21 passed — this review observed 483, 1200, 1502, 3069, 473 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // Template updates never change the selected model. | ||
| delete installPlan.modelSelection; |
There was a problem hiding this comment.
[Suggestion] The syncAuthState callback passed to applyProviderInstallPlan in executeUpdate (~line 303) is dead wiring left behind by this diff: modelSelection is now unconditionally deleted before the plan is applied, and applyProviderInstallPlan only invokes syncAuthState inside if (effectiveModelSelection?.modelId) (install.ts), so the callback can never fire on the update path. — Concrete cost: the call site reads as if the update flow still syncs model/auth state, contradicting the invariant the added comment states; a future change that lets a modelSelection survive this path would silently re-enable model switching through a callback everyone assumed was inert. Fix: drop the syncAuthState property from the options object passed here (it is optional in ApplyProviderInstallPlanOptions).
中文说明
[建议] executeUpdate 传给 applyProviderInstallPlan 的 syncAuthState 回调(约第 303 行)是本 diff 留下的死代码:modelSelection 现在在应用计划前被无条件删除,而 applyProviderInstallPlan 只在 if (effectiveModelSelection?.modelId) 内调用 syncAuthState(install.ts),因此更新路径上该回调永远不可能触发。 — 具体代价:调用点看起来仍像更新流程会同步模型/认证状态,与新增注释声明的不变量矛盾;未来若有改动让 modelSelection 在此路径存活,会通过一个人人以为已失效的回调悄悄重新启用模型切换。修复:删除此处传入的 options 对象中的 syncAuthState 属性(它在 ApplyProviderInstallPlanOptions 中是可选的)。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| expect(settings.providerMetadata?.['coding-plan']).toMatchObject({ | ||
| region: 'global', | ||
| baseUrl: CODING_PLAN_GLOBAL_BASE_URL, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] No test asserts the version value writeCodingPlanConfig persists — this toMatchObject checks only region and baseUrl. — Failure scenario (probe-verified in this review): removing version: planConfig.version from writeCodingPlanConfig leaves the entire vscode-ide-companion suite green (483 passed | 5 pre-existing env failures, identical to baseline). With version missing, the CLI's findAllPendingUpdates hits if (!metadata.version) continue; and IDE-signed-in users are never offered template updates; with a locally computed hash the #8504 prompt loop returns on every launch. The suggested assertion flips the probe (fails under the mutation, passes on the clean tree); it needs getSubscriptionPlanConfig imported from ./subscriptionPlanDefinitions.js.
| expect(settings.providerMetadata?.['coding-plan']).toMatchObject({ | |
| region: 'global', | |
| baseUrl: CODING_PLAN_GLOBAL_BASE_URL, | |
| }); | |
| expect(settings.providerMetadata?.['coding-plan']).toMatchObject({ | |
| region: 'global', | |
| baseUrl: CODING_PLAN_GLOBAL_BASE_URL, | |
| version: getSubscriptionPlanConfig('coding', 'global').version, | |
| }); |
中文说明
[建议] 没有测试断言 writeCodingPlanConfig 持久化的 version 值 —— 此 toMatchObject 只检查 region 和 baseUrl。 — 失败场景(本次评审已通过探针验证):从 writeCodingPlanConfig 中删除 version: planConfig.version 后,整个 vscode-ide-companion 测试套件仍全绿(483 通过 | 5 个预存环境失败,与基线完全相同)。version 缺失时,CLI 的 findAllPendingUpdates 会在 if (!metadata.version) continue; 处跳过,IDE 登录用户将永远不会收到模板更新提示;若换成本地计算的哈希,#8504 的更新提示循环会在每次启动时复现。上方 suggestion 可翻转该探针(变异后失败、干净代码上通过);需要从 ./subscriptionPlanDefinitions.js 导入 getSubscriptionPlanConfig。
— qwen3.8-max via Qwen Code /review (v0.21.9)
chiga0
left a comment
There was a problem hiding this comment.
Re-Review at HEAD bec8736c
Previous review 4903560393 was at 3646f923. Two new commits landed since:
92ca1f4ftest(cli): update model selection test for unconditional modelSelection deletionbec8736ctest(cli): pin provider update metadata routing
All statuses below were verified against the actual file content at bec8736c, not commit messages.
Previous Findings Status
| # | Previous Finding | Status | Evidence at HEAD |
|---|---|---|---|
| Minor 1 | Implicit version overwrite in executeUpdate |
useProviderUpdates.ts:275-277 unchanged — still a bare two-step overwrite with no helper and no comment at the overwrite site. Mitigation found on re-verification: the semantics are pinned by useProviderUpdates.test.ts:252 ("persists the template version and preserves custom models"), which installs a custom model and asserts the stored version equals the pure template hash chinaVersion. That assertion only passes because of the overwrite, so an accidental buildInstallPlan regression would fail CI. Remaining concern is readability only. |
|
| Minor 2 | Unconditional model-selection preservation vs. misleading test name | ✅ Fixed | 92ca1f4f renamed the case to "leaves the model selection alone when the previous model is gone" (useProviderUpdates.test.ts:557), added an intent comment, and inverted the assertions: syncAfterAuthRefresh not called, no model.name / model.baseUrl writes, and the success message no longer contains "Model switched". Source and test now agree. |
| Minor 3 | buildInstallPlanSrc imported after use |
❌ Still present | packages/core/src/providers/__tests__/provider-config.test.ts — used at line 72, imported at line 686. bec8736c touched this file's import block region without reordering. |
New Findings
New-1 [Minor] — the update prompt still promises a model switch that the update no longer performs
Because installPlan.modelSelection is now deleted unconditionally (useProviderUpdates.ts:280), applyProviderInstallPlan skips the entire model-selection block (packages/core/src/providers/install.ts:236-271) and never calls syncAuthState (install.ts:288-295) — precisely what 92ca1f4f now asserts.
But packages/cli/src/ui/components/ProviderUpdatePrompt.tsx:99-102 still renders, before the user confirms:
Note: Your selected model is being removed. It will switch to "{{model}}" after update.
with model = affectedEntry.diff.fallbackModel, i.e. newModelIds[0] computed at useProviderUpdates.ts:151. That promise is no longer kept:
model.nameis never rewritten, so settings keep pointing at the model that was just removed from the list.- For the active provider only
refreshAuthruns, and the model it re-resolves is not guaranteed to equalfallbackModel. - For an inactive provider
refreshAuthis not called at all, so the dangling selection survives to the next launch.
Suggested fix: either drop/soften the warning (e.g. "Your selected model is being removed; use /model to pick a new one"), or restore an explicit switch for the currentModelAffected case only. ProviderUpdatePrompt.tsx is outside this PR's diff, but this PR is what makes its text wrong.
Two related observations that are not findings:
- The
Model switched to "{{model}}"branch (useProviderUpdates.ts:325-336) is not dead code.Config.getModel()readsgetContentGeneratorConfig()?.model, andrefreshAuthre-resolves the content generator config — see the comment atconfig.ts:3686-3687("the resolved model can differ from the pre-auth one"). Keep it. - The
syncAuthStatecallback wired atuseProviderUpdates.ts:303-306is now unreachable on this path, corroborating CI-bot comment 3757044800.
Fresh Pass Over the New Commits
Verified sound:
bec8736c"preserves the stored global base URL when updating" (test:353) correctly pins region preservation: the stored globalbaseUrlround-trips and the persisted version is the global-region hash rather than the China default. Closes the gap in CI-bot comment 3756510227.bec8736c"updates both provider metadata keys from a batched prompt" (test:394) pins per-metadataKeyrouting for coding-plan and token-plan within one confirm loop. Closes CI-bot comment 3756510236.- Removing the
// keep in sync ...comment inalibaba-coding-plan.tsis correct:ALIBABA_SUBSCRIPTION_MODELSno longer exists andsubscriptionPlanDefinitions.tsderives its template and version from the core preset. Closes CI-bot comment 3756510244. - The
installPlan.providerState!...!['version']non-null assertions are safe.findAllPendingUpdatesskips providers without aresolveMetadataKey(useProviderUpdates.ts:201-202), andresolveProviderStatealways emits exactly that key when one exists (provider-config.ts:229-244). No crash path. - Earlier import Criticals are genuinely gone: single
resolveMetadataKeySrcimport, and zero remainingPROVIDER_METADATA_NSreferences inprovider-config.test.ts.
Nit: settingsWriter.test.ts:66-69 asserts only region and baseUrl from writeCodingPlanConfig and never the version value — the single field this PR exists to keep in sync. Corroborates CI-bot comment 3757044808.
Inline Comment Audit
| Comment | Status |
|---|---|
| 3751481461 / 3755085214 / 3752120990 (P1 subset-install stamping) | OBSOLETE — resolveProviderState hashes the actually-installed models |
3752120958 (duplicate resolveMetadataKeySrc import) |
OBSOLETE — single import at line 688 |
3752120971 (PROVIDER_METADATA_NS never imported) |
OBSOLETE — zero references remain in that file |
| 3752120980 (source-built plan oracle) | OBSOLETE — assertion now compares against computeModelListVersion(models) |
3756510227 (no non-default baseUrl update test) |
OBSOLETE — covered by test:353 |
| 3756510236 (single-provider version-write coverage) | OBSOLETE — covered by test:394 |
| 3756510244 (dangling sync instruction) | OBSOLETE — comment removed in bec8736c |
3757044800 (dead syncAuthState callback) |
STILL VALID |
3757044808 (no version assertion in settingsWriter) |
STILL VALID |
Summary
- Critical / Major: 0
- Minor: 2 (New-1 new; Minor 3 carried over)
- Nit: 2 (Minor 1 downgraded;
settingsWriterversion coverage)
Verdict
COMMENT — not blocking. Previous Minor 2 is properly fixed and the two new tests close real coverage gaps in the version/region routing. Withholding APPROVE only because of New-1: the pre-confirm prompt still tells users their model "will switch to X" while the update path now deliberately never switches it, leaving model.name pointing at a removed model.
This review was generated by QoderWork AI
| ]!['version'] = pending.currentVersion; | ||
| delete installPlan.env; | ||
| // Template updates never change the selected model. | ||
| delete installPlan.modelSelection; |
There was a problem hiding this comment.
[Minor] The prompt still promises a model switch this line prevents
Deleting modelSelection makes applyProviderInstallPlan skip the model-selection block (core/src/providers/install.ts:236-271) and never call syncAuthState (install.ts:288-295) — which 92ca1f4f now correctly asserts.
However packages/cli/src/ui/components/ProviderUpdatePrompt.tsx:99-102 still shows, before the user confirms:
Note: Your selected model is being removed. It will switch to "{{model}}" after update.
using diff.fallbackModel (newModelIds[0], computed at line 151 of this file). After this change that never happens:
model.nameis never rewritten, so settings keep pointing at the removed model.- For the active provider only
refreshAuthruns, and the model it re-resolves is not guaranteed to befallbackModel. - For an inactive provider
refreshAuthis not called at all, so the stale selection survives to the next launch.
Either soften the prompt text (e.g. "Your selected model is being removed; use /model to pick a new one") or restore an explicit switch limited to the currentModelAffected case.
Note: the Model switched to "{{model}}" branch below (lines 325-336) is still reachable via refreshAuth re-resolution (see config.ts:3686-3687), so it should not be removed. But the syncAuthState callback at lines 303-306 is now unreachable on this path.
This review was generated by QoderWork AI
| getAllProviderBaseUrls as getAllProviderBaseUrlsSrc, | ||
| } from '../all-providers.js'; | ||
| import { | ||
| buildInstallPlan as buildInstallPlanSrc, |
There was a problem hiding this comment.
[Minor] Still unresolved from the previous review: source import placed after first use
buildInstallPlanSrc is first used at line 72 but only imported here at line 686. ESM hoisting makes this legal, so tests pass, but a reader scanning the top import block cannot tell where buildInstallPlanSrc comes from, and any future reordering or a switch to a non-hoisting transform breaks it.
Please move this source-relative import block (and the ../all-providers.js block above it) to the top of the module alongside the @qwen-code/qwen-code-core imports.
This review was generated by QoderWork AI
ytahdn
left a comment
There was a problem hiding this comment.
Approve. Eliminating the duplicated model list in subscriptionPlanDefinitions.ts and deriving from core presets via buildProviderTemplate + computeModelListVersion is the right DRY fix. Tests asserting "version reproducible by CLI" rather than pinning model IDs prevents future drift. The executeUpdate signature change to accept PendingUpdate is cleaner.
Comments:
useProviderUpdates.ts:275— direct mutation ofinstallPlan.providerState: ConfirmapplyProviderInstallPlandeep-copies or that this mutation has no side effects on the caller's object.useProviderUpdates.ts—delete installPlan.modelSelection: When the user's current model no longer exists in the refreshed template, confirm the CLI handles the invalid model ID gracefully elsewhere (fallback prompt, etc.) rather than leaving the user stuck.subscriptionPlanDefinitions.ts:192—findProviderByCredentialsthrows on mismatch: If a core preset is renamed or endpoint changes, IDE sign-in will crash. Consider a fallback or try/catch at the call site with a user-friendly message.
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Reviewed at bec8736.
Verdict
No blocking issues. The two version semantics are now correctly separated: resolveProviderState hashes the actual installed model list, and only executeUpdate overwrites it with the template hash on a confirmed refresh. The VS Code companion derives plan templates from the core presets and persists the selected region baseUrl, so the CLI launch check can reproduce the global-region version. Earlier Critical findings (duplicate import, missing PROVIDER_METADATA_NS, global template stamping) are all addressed at this HEAD.
Summary
- Critical / Major: 0
- Minor: 0
- Nit: 0
CI is green on the current head. The reviewer test plan is covered by the focused test suites (65 + 27 + 21 passed).
|
Released in v0.21.10. |
QwenLM#8889 made the template update path drop the plan's model selection unconditionally, so the mixed-batch test that expected the active provider to migrate now pins removed behavior. Invert it: a batch mixing the active provider (current model gone from its plan) with an inactive one must leave model.name and model.baseUrl untouched while both updates still run to completion — the batch-loop side of the invariant whose single-provider side QwenLM#8889 pins itself.
…ates (QwenLM#8879) * test(cli): pin the model selection across multi-provider template updates The worst case reported in QwenLM#8863 was the batch path: with several providers updating in one confirmation, each executeUpdate in the loop rewrote model.name in turn and the last provider in registry order won. The QwenLM#8868 gate protects it, but no test failed if that regressed — its regression tests cover a single updating provider. Pin the batch scenario: two stale providers, a current model owned by neither, one 'Update all' confirmation, and no model.name/model.baseUrl writes from either pass. Reverting the gate to the pre-QwenLM#8868 form now fails this test alongside the two single-provider ones. * test(cli): verify both batch entries execute and pin the mixed-batch gate Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(cli): pin no-migration in the mixed batch after QwenLM#8889 QwenLM#8889 made the template update path drop the plan's model selection unconditionally, so the mixed-batch test that expected the active provider to migrate now pins removed behavior. Invert it: a batch mixing the active provider (current model gone from its plan) with an inactive one must leave model.name and model.baseUrl untouched while both updates still run to completion — the batch-loop side of the invariant whose single-provider side QwenLM#8889 pins itself. --------- Co-authored-by: ComplexSimply <rudy.arrowsong@gmail.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
What this PR does
Keeps two provider-version meanings separate. A normal provider install records the hash of the models actually installed, including a deliberately selected subset. A template refresh records the current built-in template hash after applying that template, so accepting an update clears the prompt while preserving user-added models.
The VS Code sign-in flow now builds plan models and versions from the core presets, persists the selected region's base URL with provider metadata, reuses the core environment-key and endpoint constants, and removes unused model-name-prefix fields.
Why it's needed
Hashing the full built-in template during every install made a subset installation look as though every built-in model had already been installed. Conversely, hashing the final model list after a template refresh made custom models keep the update prompt alive forever. The version must describe the actual install for ordinary installs and the applied template for the dedicated update path.
The extension also wrote only the selected region name and version. A global-region sign-in therefore fell back to the China endpoint when the CLI checked for updates, causing a false prompt and potentially flipping the region on update.
Relates to #8504.
Reviewer Test Plan
How to verify
Focused verification completed:
Evidence (Before & After)
N/A — covered by unit tests at the writer, install-plan, and launch-check boundaries.
Tested on
Environment (optional)
Node.js 22, local macOS checkout.
Risk & Scope
baseUrlfield.Linked Issues
Relates to #8504.
中文说明
本 PR 做了什么
区分两种 provider 版本语义:普通安装记录实际安装模型列表(包括用户主动选择的子集)的哈希;模板更新在应用内置模板后记录当前模板哈希,因此接受更新后提示会消失,同时保留用户自定义模型。
VS Code 登录流程现在直接使用 core 中的套餐预设生成模型和版本,将所选区域的 base URL 一并写入 provider 元数据,复用 core 的环境变量名和 endpoint 常量,并删除未使用的模型名前缀字段。
为什么需要
如果普通安装总是记录完整内置模板的哈希,只安装部分模型也会被误认为已经安装全部内置模型。反过来,如果模板更新后记录包含自定义模型的最终列表哈希,更新提示就会一直出现。因此普通安装必须描述实际安装内容,而专用更新路径必须描述刚刚应用的模板。
扩展之前只记录区域名称和版本。Global 区域登录后,CLI 检查更新时会回退到中国区 endpoint,产生错误更新提示,并可能在更新时切换区域。
关联 #8504。
Reviewer 测试计划
已完成的验证:core provider 配置 65 项、CLI provider 更新 27 项、VS Code 套餐定义和设置写入 21 项测试全部通过;全仓构建、全仓类型检查、Prettier 和 ESLint pre-commit 检查通过。
风险与范围
baseUrl字段。