fix(cli): keep the user's model selection intact on provider template updates - #8873
fix(cli): keep the user's model selection intact on provider template updates#8873ComplexSimply wants to merge 1 commit into
Conversation
… updates 'Update all' in the built-in provider update dialog rewrote model.name to the provider's first built-in model and cleared model.baseUrl whenever the current model was not one of that provider's models — exactly the case where the update has no business touching it (QwenLM#8863, a QwenLM#5819 regression left open by the QwenLM#5835 guard, which only matches models the plan itself offers). With several providers updating in one confirmation, each rewrote model.name in turn and the last one in registry order won. A template update is not first-time setup and carries no model-selection intent: executeUpdate now drops the plan's modelSelection unconditionally. Models the provider owns are already carried into the plan (including removed built-ins the user still sits on), so the previous conditional delete was a no-op in every legitimate case and live only in the buggy one. The toast now reports a switch only when the active model actually changed, instead of predicting one from the plan.
|
@qwen-code /takeover |
|
Thanks for the PR! Template looks good ✓ Problem: this is an observed P1 bug, not theory. The linked #8863 carries a full reproduction (isolated Direction: aligned. A provider template update is not first-time setup and should carry no model-selection intent, yet the old path silently rewrote Size: 16 production lines ( Approach: minimal and focused. It unconditionally drops the plan's Risk: no high-revert-risk path matched. The Ubuntu CI unit suite was still running at review time (macOS/Windows/integration are merge-queue-only by design here, so their Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:这是已观测到的 P1 bug,不是理论问题。关联的 #8863 提供了完整复现(隔离 方向:对齐。provider 模板更新不是首次安装,本身不应携带选模型的意图,但旧路径会静默改写 规模:生产代码 16 行( 方案:最小且聚焦。在更新路径上无条件丢弃 plan 的 风险:未命中高回滚风险路径。审查时 Ubuntu CI 单元测试仍在运行(此仓库 macOS/Windows/集成测试仅在 merge queue 触发,其 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent take before reading the diff: a template update carries no model-selection intent, so the update path should strip
The tests pin the change the right way: they filter Test evidence (this PR's own CI — I do not run PR code)The Ubuntu unit suite (the only test job that runs on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Sandboxed lane for the remaining gapThe unit tests pin the hook logic, but the end-to-end claim — 中文说明代码审查我在看 diff 之前的独立判断:模板更新不携带选模型的意图,因此应在更新调用点丢弃 plan 的
测试以正确方式钉住改动:过滤 测试证据(来自 PR 自身 CI——不运行 PR 代码)Ubuntu 单元测试( 沙箱验证通道单元测试钉住了 hook 逻辑,但端到端结论( — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review and the fix is right, but this is policy-driven caution rather than doubt: it changes model-selection behavior from a fork, so it needs a maintainer's sign-off. Stepping back: this is exactly the kind of PR the gate should let through. The problem is real and costly — #8863 reproduces cleanly, and the precedent #5819 burned real money the last time this surface misbehaved. My independent proposal (drop Why I'm not approving: the change writes to the model-selection surface ( ⏸️ Deferring to @yiliang114 (filed #8863 and traced the root cause; core codeowner) — the fix looks correct and complete, but model-selection changes from a fork need a maintainer's call. Needs a human sign-off on this one. 中文说明置信度:3/5 —— 审查干净、修复正确,这里的保留是政策性的而非存疑:它从 fork 修改了模型选择行为,需要 maintainer 签核。 退一步看:这正是门禁应该放行的 PR。问题真实且代价高——#8863 可完整复现,前车之鉴 #5819 曾因同一敏感面出错烧掉真实资金。我的独立方案(在更新调用点丢弃 不批准的原因:该改动触及模型选择面(provider 更新时 ⏸️ 转交 @yiliang114(#8863 的报告者与根因分析者、core codeowner)——修复看起来正确且完整,但来自 fork 的模型选择变更需要 maintainer 拍板。 — Qwen Code · qwen3.8-max Reviewed at |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 |
|
🤖 Could not produce a passing fix for this feedback (round 1/100). This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31389561905 🧠 Handled by Qwen Code · model/模型 |
|
Superseded by #8868, which landed 22 minutes after this PR was opened — closing. Two independent fixes converged on the same diagnosis within the hour, which is itself nice confirmation of the root cause. The differences in #8868 are improvements over this PR: the migration is retained for the active provider when its model genuinely vanished (keyed on One case from the issue report that #8868's tests don't pin: the multi-provider batch update, where each For what it's worth, this PR's live before/after TUI run (issue repro script, 中文被 #8868 取代(比本 PR 晚 22 分钟提交但先合入)——关闭。 两个独立修复在一小时内收敛到同一诊断,本身就是根因的再确认。#8868 相比本 PR 的差异是改进:迁移在活动 provider 且模型真正消失时被保留(用 issue 报告里有一个 #8868 测试未钉住的场景:多 provider 批量更新——循环里每个 另外:本 PR 的真实 TUI 前后对比(issue 复现脚本 + 真实弹窗 Update all)确认了旧构建上的破坏,也刚在 |
What this PR does
Makes the built-in provider template update path leave the user's model selection alone.
executeUpdatenow drops the install plan'smodelSelectionunconditionally, so choosingUpdate allrefreshes the provider's model list and version metadata without ever rewritingmodel.nameor clearingmodel.baseUrl. The completion toast now reports a model switch only when the active model actually changed, instead of predicting one from the plan — which is how the old code could announceModel switched to "<old model>"while writing a different model to disk.The previous conditional (
delete modelSelectiononly when the plan still offers the current model) was a no-op in every legitimate case and live only in the buggy one: models the provider owns are already carried into the refreshed plan as custom entries — including removed built-ins the user still sits on — so a current model that is absent from the plan is precisely a model that was never this provider's to migrate.Why it's needed
Fixes #8863 (P1): when the current
model.namebelongs to another provider (a self-hosted gateway, another vendor on the same protocol),Update allsilently rewrote it to the updating provider's first built-in model and clearedmodel.baseUrl, while the toast claimed the opposite. All 11 built-in providers hit this with different overwrite targets, and with several providers updating in one confirmation each rewrotemodel.namein turn — the last one in registry order won. This is the unfinished half of #5819 (which caused real monetary damage); the #5835 guard inapplyProviderInstallPlanonly matches models the plan itself offers, so it never protected models owned elsewhere. The first-install path (/auth, where adopting the provider default is genuine intent) is untouched — this PR changes only the update path inuseProviderUpdates.ts.Reviewer Test Plan
How to verify
vitest run src/ui/hooks/useProviderUpdates.test.tsinpackages/cli— 20/20. Three new tests cover the single-provider overwrite, the misleading toast, and the multi-provider last-writer-wins case; the pre-existingswitches model when previous model is no longer availabletest pinned the buggy migration and is inverted intodoes not adopt the provider default when the previous model is gone from the plan(rationale above).delete installPlan.modelSelectionfails 4 tests (the 3 new ones + the inverted one); flipping the toast condition (===→!==) fails exactly the toast test.src/ui/hooks/slice: 69 files, 1448/1448.tsc --noEmitonpackages/cli: clean.npm run bundlebuild, isolated$HOME, real TUI dialog via tmux): see Evidence.Evidence (Before & After)
Before (main @ 4bc75c2) — dialog confirmed with
Update all, current modelmy-own-modelowned by no built-in provider:After (this branch) — same script, same dialog, same confirmation:
The update itself still lands in both runs; verified after the fix:
providerMetadata.token-plan.versionadvanced from the stale marker to the current hash, the provider's 15 refreshed built-ins are installed, and both the user's own entry and the provider-prefixed custom (tp-custom) are preserved (17 models total).Tested on
Environment (optional)
node dist/cli.jsfromnpm run bundle, driven in tmux with an isolated$HOMEand the issue's dummy-key settings fixture; no real API traffic.Risk & Scope
currentModelAffected/fallbackModelfrom built-in ids only (display-only, pre-existing); theModel switched totoast branch is kept for the case whererefreshAuthgenuinely changes the active model.Linked Issues
Fixes #8863. Refs #5819, #5835.
中文说明
本 PR 做了什么
让内置 provider 模板更新路径不再触碰用户的模型选择。
executeUpdate现在无条件丢弃 install plan 的modelSelection:选择Update all只刷新该 provider 的模型列表与版本元数据,任何情况下都不再改写model.name、不再清空model.baseUrl。完成提示改为只在活动模型真的变化时才报告切换,而不是按 plan 预测——旧代码正因此一边宣称Model switched to "<旧模型>",一边把另一个模型写进磁盘。原来的条件删除(仅当 plan 仍提供当前模型时删
modelSelection)在所有正当情形下都是空操作、只在缺陷情形下生效:该 provider 名下的模型(包括用户仍在使用的已移除内置模型)都会作为自定义条目被携带进刷新后的 plan,因此「当前模型不在 plan 里」恰恰意味着它从来不归本 provider 管、更轮不到本次更新迁移。为什么需要
修复 #8863(P1):当前
model.name属于其他 provider(自建网关、同协议下别家厂商)时,Update all会把它静默改写为该 provider 内置列表第一个模型并清空model.baseUrl,提示文案却反着说。全部 11 个内置 provider 都有此问题、覆写目标各不相同;多个 provider 在同一次确认里更新时逐个改写model.name,注册表顺序最后的胜出。这是 #5819(造成过实际资金损失)的未完成修复:#5835 在applyProviderInstallPlan加的保护只匹配 plan 自己提供的模型,从未保护过别家模型。首次安装路径(/auth,采纳 provider 默认模型是真实意图)不受影响——本 PR 只改useProviderUpdates.ts的更新路径。验证方式
packages/cli下vitest run src/ui/hooks/useProviderUpdates.test.ts—— 20/20。三个新测试覆盖单 provider 覆写、误导性提示、多 provider 最后写入者胜出;既有的switches model when previous model is no longer available测试钉住的是缺陷迁移行为,已反转为does not adopt the provider default when the previous model is gone from the plan(理由见上)。delete installPlan.modelSelection导致 4 个测试失败(3 个新测试 + 反转测试);翻转提示条件(===→!==)恰好导致提示测试失败。src/ui/hooks/全量切片:69 文件 1448/1448;packages/cli的tsc --noEmit干净。npm run bundle构建、隔离$HOME、tmux 驱动真实 TUI 弹窗):见证据一节。风险与范围
currentModelAffected/fallbackModel(纯展示、先前已有);保留Model switched to提示分支以覆盖refreshAuth真的改变活动模型的情形。关联 Issue
Fixes #8863;参考 #5819、#5835。