Skip to content

revert(core): revert Protocol enum & model-identity decoupling (#5089) - #5745

Merged
wenshao merged 5 commits into
QwenLM:mainfrom
wenshao:revert-5089-structural
Jun 23, 2026
Merged

revert(core): revert Protocol enum & model-identity decoupling (#5089)#5745
wenshao merged 5 commits into
QwenLM:mainfrom
wenshao:revert-5089-structural

Conversation

@wenshao

@wenshao wenshao commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This reverts the structural changes introduced by #5089 back to the pre-#5089 shape, while preserving the features that were merged on top of it, and adds a downgrade migration so configs already migrated by the released version keep loading.

Reverted back to the pre-#5089 structure: AuthType is a fixed enum again instead of string; the Protocol enum is removed; modelProviders is Record<authType, ModelConfig[]> (arrays) again instead of { protocol, models }; createContentGenerator dispatches on authType; the v4→v5 settings migration is removed and SETTINGS_VERSION returns to 4.

Preserved and re-adapted to the enum + array structure (NOT reverted): #5632 fastOnly/voiceOnly model flags, #5638 workspace provider defaults, #5729 active-runtime-model listing, and #5728 ACP set_config_option deterministic provider fixture. Their production code was already format-agnostic or tolerant (e.g. readProviderModels handles both shapes); only test fixtures that hard-coded the v5 { protocol, models } shape were reshaped back to arrays.

Added a v5→v4 settings downgrade migration: settings already migrated to $version: 5 (shipped in v0.19.0) carry a modelProviders shape the reverted v4 readers consume as arrays, which would throw models is not iterable on load. The new migration unwraps each { protocol, models } back to its models array, drops the now key-derived protocol, and resets $version to 4 automatically on load.

Why it's needed

#5089 was merged in error and needs to be backed out. A plain git revert of the squash commit does not build, because several PRs merged after it (#5632, #5638, #5729, #5728) were written against its Protocol / ProviderConfig structures. This PR backs out only the structural change and re-adapts those dependents so the tree still builds and their features keep working, and it ships the downgrade migration so users who already ran v0.19.0 are not left with an unreadable config.

Reviewer Test Plan

How to verify

  1. npm run build — full monorepo build succeeds.
  2. npm run typecheck — all packages typecheck (also npm --prefix packages/vscode-ide-companion run check-types).
  3. Targeted suites: npx vitest run --root packages/core src/models/ src/providers/ src/core/contentGenerator.test.ts and npx vitest run --root packages/cli src/config/.
  4. v5→v4 auto-migration: create ~/.qwen/settings.json at "$version": 5 with modelProviders: { "openai": { "protocol": "openai", "models": [ { "id": "gpt-4o" } ] } }, start the CLI, and confirm the file is rewritten to "$version": 4 with modelProviders: { "openai": [ { "id": "gpt-4o" } ] } and the app loads without models is not iterable.

Evidence (Before & After)

N/A — non-user-visible refactor/revert. Verified by full-workspace typecheck, full npm run build, and the affected unit suites (core models/providers/config + cli config, plus a new end-to-end loadSettings downgrade test). The integration test integration-tests/cli/acp-integration.test.ts had its fixture reshaped to the v4 array form but was not executed locally (requires the full build+run harness).

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Risk & Scope

Linked Issues

Reverts #5089.

中文说明

这个 PR 做了什么

回退 #5089 引入的结构性改动到 #5089 之前的形态,同时保留在它之后合入、叠加于其上的功能,并新增一个降级迁移,使已被发布版本迁移过的配置仍能正常加载。

回退到 #5089 之前的结构:AuthType 重新变回固定枚举而不是 string;移除 Protocol 枚举;modelProviders 重新变回 Record<authType, ModelConfig[]>(数组)而不是 { protocol, models }createContentGeneratorauthType 分发;移除 v4→v5 设置迁移,SETTINGS_VERSION 回到 4。

保留并适配回枚举 + 数组结构(未回退):#5632 fastOnly/voiceOnly 模型标记、#5638 workspace provider defaults、#5729 运行时模型列表、#5728 ACP set_config_option 确定性 provider fixture。它们的生产代码本就与格式无关或双格式兼容(例如 readProviderModels 同时处理两种形状);只有硬编码了 v5 { protocol, models } 形状的测试 fixture 被改回数组。

新增 v5→v4 设置降级迁移:已迁移到 $version: 5(随 v0.19.0 发布)的设置,其 modelProviders 形状会被回退后的 v4 读取逻辑当作数组消费,加载时抛 models is not iterable。新迁移在加载时自动把每个 { protocol, models } 解包回 models 数组、丢弃可由 key 推导的 protocol,并把 $version 重置为 4。

为什么需要

#5089 是错误合入的,需要回退。直接对 squash commit 做 git revert 编译不过,因为在它之后合入的多个 PR(#5632#5638#5729#5728)是基于它的 Protocol / ProviderConfig 结构编写的。本 PR 只回退结构性改动并把这些依赖项适配回去,使代码仍能构建、功能仍然可用;同时附带降级迁移,让已经运行过 v0.19.0 的用户不会被卡在无法读取的配置上。

Reviewer 测试计划

如何验证

  1. npm run build —— 全仓库构建通过。
  2. npm run typecheck —— 所有包类型检查通过(外加 npm --prefix packages/vscode-ide-companion run check-types)。
  3. 定向用例:npx vitest run --root packages/core src/models/ src/providers/ src/core/contentGenerator.test.tsnpx vitest run --root packages/cli src/config/
  4. v5→v4 自动迁移:准备 "$version": 5modelProviders: { "openai": { "protocol": "openai", "models": [ { "id": "gpt-4o" } ] } }~/.qwen/settings.json,启动 CLI,确认文件被改写为 "$version": 4modelProviders: { "openai": [ { "id": "gpt-4o" } ] },且加载时不再抛 models is not iterable

证据(前后对比)

N/A —— 非用户可见的重构/回退。通过全仓库类型检查、完整 npm run build,以及受影响的单元测试套件(core models/providers/config + cli config,外加一个新的端到端 loadSettings 降级测试)验证。集成测试 integration-tests/cli/acp-integration.test.ts 的 fixture 已改回 v4 数组形式,但未在本地执行(需要完整的构建 + 运行 harness)。

测试环境

操作系统 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

风险与范围

关联 Issue

Reverts #5089.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR, @wenshao.

Template looks good ✓ — all required sections present, bilingual, test plan is concrete.

On direction: this is reverting #5089, which was merged in error per the PR body. That's a valid motivation, but it's also backing out a shipped, foundational refactor — which means reopening at least six previously-closed issues (#5090, #4877, #5080, #4814, #4813, #4722) that #5089 claimed to resolve. Touching the model-identity / provider-selection core like this feels maintainer-territory enough that I'm flagging it for human judgment on the tradeoff rather than rubber-stamping either way.

On approach: the two-commit structure is clean — (1) revert + re-adapt dependents, (2) add v5→v4 downgrade migration. The migration is the right call: anyone who ran v0.19.0 already has $version: 5 with { protocol, models } shapes, and without the downgrade they'd hit models is not iterable on next load. The PR explicitly acknowledges the "pathological untagged file mixing v1 keys with v5 shape" corner case and correctly notes no shipped version can produce it.

Scope-wise the diff is net -882 lines across 78 files, which is consistent with a revert. But I spotted two small pieces of drive-by formatting churn that don't belong in a revert:

  • .qwen/skills/desktop-pet/SKILL.md — markdown tables were reformatted from padded alignment to single-pipe (| -------------- ||---|). Pure cosmetic, unrelated to the revert.
  • docs/users/qwen-serve.md — trailing whitespace added on one table row.

Both are tiny, but a revert should be mechanically minimal. Worth splitting those out (or just dropping them) so the diff stays a pure structural revert.

Moving on to code review. 🔍

中文说明

感谢 @wenshao 的 PR。

模板完整 ✓ — 所有章节齐全,双语,测试计划具体。

方向:本 PR 回退被描述为"错误合入"的 #5089。动机合理,但同时也回退了一个已经发布的基础性重构 —— 意味着重新打开至少 6 个此前已被关闭的 issue(#5090#4877#5080#4814#4813#4722)。涉及模型标识 / provider 选择的核心结构,个人判断这已经进入需要维护者拍板的范围,因此标记为需要人工判断,而不是直接通过或拒绝。

方案:两次提交的结构干净 ——(1)回退 + 适配依赖项,(2)新增 v5→v4 降级迁移。迁移是正确的选择:任何跑过 v0.19.0 的用户已经持有 $version: 5{ protocol, models } 形状,缺少降级迁移会在下次加载时抛 models is not iterable。PR 也明确承认了"v1 时代 key 混 v5 形状"的病态边界情况,并正确指出没有任何已发布版本会产生该状态。

范围上 diff 净 -882 行、涉及 78 个文件,符合回退的预期。但发现两处不属于回退的顺手格式改动:

  • .qwen/skills/desktop-pet/SKILL.md —— markdown 表格从对齐填充被重排为单竖线(| -------------- ||---|)。纯装饰,与回退无关。
  • docs/users/qwen-serve.md —— 某表格行尾增加了尾随空白。

都很小,但回退应当机械地最小化。建议把这些拆出去(或直接丢弃),让 diff 保持纯粹的结构性回退。

进入代码审查 🔍

Qwen Code · qwen3.7-max

qqqys
qqqys previously approved these changes Jun 23, 2026

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the revert/downgrade migration path. Focused migration/model-provider tests passed; only noted a minor formatting issue in BaseTextInput.tsx.

@qqqys

qqqys commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

@zzhenyao 感谢你之前提供 #5089 的 PR。这个方向本身很有价值,不过考虑到这类改动涉及 provider/model identity、settings migration 和多端配置兼容,影响面比较大,我倾向于先由 maintainer @qqqys 来主导后续方案设计和落地。

这次先通过 #5745 回滚相关结构性改动,避免已经发布后的配置兼容风险继续扩大。方便的话,能否新开一个 issue,把原始动机、目标场景和你设想的具体方案展开一下?我们可以在 issue 里继续讨论一个更稳妥的实现路径。

Comment thread packages/cli/src/config/settings.ts
yiliang114
yiliang114 previously approved these changes Jun 23, 2026

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@wenshao
wenshao dismissed stale reviews from yiliang114 and qqqys via 5714682 June 23, 2026 06:37

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ Downgraded from Request Changes to Comment: self-PR; CI failing: review-pr.

Summary: 1 Critical finding + 4 low-confidence Suggestions (terminal only).

Critical: VS Code findOpenaiModels drops V5 { protocol, models } wrapper handling. The extension writes settings.json directly without running the CLI migration — if a user's file is still at $version: 5 when the extension writes config, existing OpenAI models are silently lost.

Build ✅ | 1091 tests ✅ | TypeCheck: 0 new findings.

— qwen3.7-max via Qwen Code /review

Comment thread packages/vscode-ide-companion/src/services/settingsWriter.ts Outdated
Comment thread packages/core/src/models/modelRegistry.ts
Comment thread packages/cli/src/config/migration/index.ts
Comment thread packages/cli/src/config/settings.test.ts Outdated
@yiliang114

Copy link
Copy Markdown
Collaborator

+1 on reverting #5089 — I just hit the exact crash this revert guards against, so it's real, not theoretical.

What happened: v0.19.0 (which carries #5089) auto-migrated my ~/.qwen/settings.json to "$version": 5, rewriting each provider from a bare array to { protocol, models: [...] }. After that, an older pre-#5089 binary on the same machine couldn't start at all — ModelRegistry.registerAuthTypeModels runs for (const config of models) where models is now the { protocol, models } object instead of an array, so it throws TypeError: models is not iterable during Config construction, before the UI loads.

The painful part is the migration is one-way: once v0.19.0 touches the file, every older binary reading it crashes, with no way back short of this revert. Plain version skew between the binary that wrote the config and the one reading it is enough to trigger it (in my case an older standalone install shadowing a newer npm one on PATH).

So this revert plus the v5→v4 downgrade migration is the right call. One thing worth a line in the release note: the downgrade only kicks in on builds that include this PR, so users pinned to a pre-#5089 binary still have to upgrade past it first.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal (before reading the diff): To revert #5089's structural changes while preserving post-#5089 features, I would: (1) restore AuthType from a string type back to a fixed enum; (2) remove the Protocol enum entirely; (3) flatten modelProviders from Record<authType, { protocol, models }> back to Record<authType, ModelConfig[]>; (4) switch createContentGenerator dispatch from protocol back to authType; (5) revert SETTINGS_VERSION to 4; (6) add a v5→v4 downgrade migration for users already on v0.19.0; (7) adapt all post-#5089 PRs (#5632, #5638, #5729, #5728) in place.

Comparison with the PR: The approach matches my proposal exactly. Every structural element is reverted cleanly, the migration is well-designed, and the dependent PRs are adapted without losing their features.

Findings:

  • Migration quality is solid. The V5ToV4Migration correctly identifies v5 configs by typeof value === 'object' && !Array.isArray(value), uses structuredClone to avoid mutating the input, warns on protocol mismatches, and handles edge cases (missing models → empty array, already-array values skipped). The test suite (17 tests) is comprehensive — covers shouldMigrate for versioned/versionless/various-shape inputs, idempotency, mutation safety, and the protocol-mismatch warning.

  • Convergence architecture is clean. The separation of ALL_MIGRATIONS (forward) and DOWNGRADE_MIGRATIONS into a combined CONVERGENCE_MIGRATIONS set is well-designed. The needsMigration logic correctly handles $version === SETTINGS_VERSION (no-op), $version > SETTINGS_VERSION (falls through to check downgrade migrations), and $version < SETTINGS_VERSION (forward migration).

  • modelConfigResolver.ts safety regression (minor). The old code used Object.hasOwn(AUTH_ENV_MAPPINGS, authType) as a runtime guard before lookup; the reverted code accesses AUTH_ENV_MAPPINGS[authType] directly. Since AUTH_ENV_MAPPINGS is now typed as Record<AuthType, AuthEnvMapping> (strict enum key), the guard was removed because the type system guarantees the key. This is consistent with the revert — pre-refactor(core): extract Protocol enum and decouple model identity from auth type #5089 also lacked this guard — but worth noting that any code path passing an unexpected authType string would now get undefined instead of a safe fallback. Not a blocker, since validateAuthTypeKey in modelRegistry.ts already constrains user input to enum values.

  • Two cosmetic changes don't belong in a revert. .qwen/skills/desktop-pet/SKILL.md reformats markdown tables (padded alignment → single-pipe), and docs/users/qwen-serve.md adds trailing whitespace. These are drive-by formatting churn unrelated to the structural revert.

Test Results

npm run build        ✅  0 errors (15 pre-existing lint warnings in vscode-ide-companion)
npm run typecheck    ✅  all packages (core, cli, vscode-ide-companion, webui, sdk, audio)
core unit tests      ✅  328 passed / 18 files (models, providers, contentGenerator)
cli config tests     ✅  764 passed / 23 files (including v5-to-v4 migration: 17 tests)

Real-Scenario Test: v5→v4 Auto-Migration

Created $QWEN_HOME/settings.json at $version: 5 with modelProviders: { openai: { protocol: "openai", models: [...] } }, then launched the CLI with npm run dev.

$ QWEN_HOME=/tmp/tmp.JPwNuAHB3m npm run dev -- -p 'say hello'

> @qwen-code/qwen-code@0.19.0 dev
> node scripts/dev.js -p say hello

DEV is set to true, but the React DevTools server is not running. Start it with:

$ npx react-devtools

Hello! How can I help you today?

After exit, settings.json was rewritten to $version: 4 with modelProviders: { openai: [ { id: "gpt-4o", ... } ] } — no models is not iterable error, clean transparent migration.

{
  "$version": 4,
  "modelProviders": {
    "openai": [
      {
        "id": "gpt-4o",
        "name": "GPT-4o",
        "baseUrl": "https://api.openai.com/v1",
        "envKey": "OPENAI_API_KEY"
      }
    ]
  }
}
中文说明

代码审查

独立方案(读 diff 之前): 要回退 #5089 的结构性改动同时保留之后的功能,我会:(1)把 AuthTypestring 类型恢复为固定枚举;(2)完全移除 Protocol 枚举;(3)把 modelProvidersRecord<authType, { protocol, models }> 拍平回 Record<authType, ModelConfig[]>;(4)createContentGenerator 分发从 protocol 切回 authType;(5)SETTINGS_VERSION 回到 4;(6)新增 v5→v4 降级迁移,服务已经升级到 v0.19.0 的用户;(7)就地适配后续合入的 PR(#5632#5638#5729#5728)。

与 PR 的对比: 方案完全吻合。每个结构要素都干净地回退了,迁移设计良好,依赖的 PR 在不丢失功能的前提下适配完成。

发现:

  • 迁移质量扎实。 V5ToV4Migration 通过 typeof value === 'object' && !Array.isArray(value) 正确识别 v5 配置,使用 structuredClone 避免修改输入,在协议不匹配时发出警告,并处理了边界情况(缺少 models → 空数组、已经是数组的值直接跳过)。测试套件(17 个用例)覆盖全面 —— 包括有版本号 / 无版本号 / 各种形状的 shouldMigrate、幂等性、不可变性,以及协议不匹配警告。

  • 汇聚架构干净。 ALL_MIGRATIONS(正向)和 DOWNGRADE_MIGRATIONS 合并为 CONVERGENCE_MIGRATIONS 的设计清晰。needsMigration 逻辑正确处理 $version === SETTINGS_VERSION(无操作)、$version > SETTINGS_VERSION(穿透检查降级迁移)和 $version < SETTINGS_VERSION(正向迁移)。

  • modelConfigResolver.ts 安全性小回退(轻微)。 旧代码用 Object.hasOwn(AUTH_ENV_MAPPINGS, authType) 做运行时守卫;回退后直接访问 AUTH_ENV_MAPPINGS[authType]。因为 AUTH_ENV_MAPPINGS 现在类型是 Record<AuthType, AuthEnvMapping>(严格枚举键),守卫被移除是类型安全的。这与回退一致 —— refactor(core): extract Protocol enum and decouple model identity from auth type #5089 之前也没有这个守卫 —— 但需注意如果某个代码路径传入了意外的 authType 字符串,现在会得到 undefined 而不是安全兜底。不构成阻断,因为 modelRegistry.tsvalidateAuthTypeKey 已经把用户输入约束为枚举值。

  • 两处装饰性改动不属于回退。 .qwen/skills/desktop-pet/SKILL.md 重新格式化了 markdown 表格(对齐填充 → 单竖线),docs/users/qwen-serve.md 增加了尾随空白。这些都是与结构性回退无关的顺手格式改动。

测试结果

npm run build        ✅  0 错误(vscode-ide-companion 中 15 个预存 lint 警告)
npm run typecheck    ✅  所有包(core, cli, vscode-ide-companion, webui, sdk, audio)
core 单元测试         ✅  328 通过 / 18 文件(models, providers, contentGenerator)
cli config 测试       ✅  764 通过 / 23 文件(含 v5-to-v4 迁移:17 个用例)

真实场景测试:v5→v4 自动迁移

$QWEN_HOME/settings.json 中创建 $version: 5modelProviders: { openai: { protocol: "openai", models: [...] } },然后用 npm run dev 启动 CLI。

启动正常,无 models is not iterable 错误。退出后 settings.json 被改写为 $version: 4modelProviders 为纯数组形式 —— 迁移透明完成。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

This PR does exactly what it says and does it well. The structural revert is mechanically clean — AuthType enum restored, Protocol enum removed, modelProviders flattened back to arrays, dispatch switched from protocol to authType. The migration is the highlight: well-tested (17 unit tests + 4 integration tests), handles edge cases properly, and the real-scenario test confirmed it works transparently on startup.

The convergence architecture (ALL_MIGRATIONS + DOWNGRADE_MIGRATIONS) is a nice pattern that future reverts can follow. The needsMigration fix for version === SETTINGS_VERSION short-circuit (instead of >=) is the right call — it lets downgrade migrations run when needed.

Two cosmetic changes (desktop-pet table formatting, qwen-serve trailing whitespace) are noise in a revert PR, but not worth blocking a time-sensitive revert over. The maintainer (@qqqys) has already signaled intent to merge this quickly.

1092 unit tests pass. Build and typecheck clean. Migration verified end-to-end in tmux. LGTM. ✅

中文说明

这个 PR 做到了它所承诺的一切。结构性回退机械地干净 —— AuthType 枚举恢复、Protocol 枚举移除、modelProviders 拍平回数组、分发从 protocol 切回 authType。迁移是亮点:测试充分(17 个单元测试 + 4 个集成测试),边界情况处理到位,真实场景测试确认启动时透明完成。

汇聚架构(ALL_MIGRATIONS + DOWNGRADE_MIGRATIONS)是一个值得未来回退参考的好模式。needsMigrationversion === SETTINGS_VERSION 短路(而不是 >=)的修正是正确的 —— 让降级迁移在需要时能够执行。

两处装饰性改动(desktop-pet 表格格式化、qwen-serve 尾随空白)是回退 PR 中的噪音,但不值得用来阻断一个时间敏感的回退。维护者(@qqqys)已经表态要快速合入。

1092 个单元测试通过。构建和类型检查全部通过。迁移在 tmux 中端到端验证完成。LGTM。✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, looks ready to ship. ✅

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Re-review of the incremental changes since the last review (554dec7..da43b37). All findings from the previous review are resolved and verified against the new code:

  • modelRegistry.registerAuthTypeModels now guards Array.isArray (skip-and-warn instead of an opaque models is not iterable) — placed inside the method, so it also covers the reload path. ✅
  • needsMigration JSDoc corrected to match the === SETTINGS_VERSION logic (and the higher-version fall-through). ✅
  • The v5→v4 downgrade test now also asserts the persisted .tmp payload ($version: 4 + unwrapped arrays), not just the in-memory result. ✅
  • findOpenaiModels (VS Code extension) restored read-side tolerance for the wrapped { protocol, models } shape, so a not-yet-downgraded file no longer drops existing OpenAI models. ✅
  • Integration suites' CURRENT_SETTINGS_VERSION realigned to 4. ✅

No new issues. Verified locally: full typecheck ✅, full build ✅, and the affected suites all pass (modelRegistry 56, settings + migration 272, settingsWriter 17). The new tests genuinely exercise each fix.

Downgraded from Approve to Comment: self-PR; CI still running. — claude-opus-4-8 via Qwen Code /qreview

qqqys
qqqys previously approved these changes Jun 23, 2026
Comment thread packages/core/src/core/contentGenerator.ts Outdated
Comment thread packages/core/src/core/contentGenerator.test.ts
Comment thread packages/cli/src/acp-integration/acpAgent.ts
wenshao added 4 commits June 23, 2026 15:38
…M#5089)

Reverts the structural changes from QwenLM#5089 back to the pre-QwenLM#5089 shape:
AuthType stays a fixed enum (not `string`), the Protocol enum is removed,
modelProviders is `Record<authType, ModelConfig[]>` again (not
`{ protocol, models }`), and createContentGenerator dispatches on authType.
The v4->v5 settings migration is removed and SETTINGS_VERSION reverts to 4.

Features merged on top of QwenLM#5089 are kept and re-adapted to the old
enum+array structure (not reverted):
- QwenLM#5632 fastOnly/voiceOnly model flags (test fixtures reshaped to arrays)
- QwenLM#5638 workspace provider defaults (readProviderModels already tolerates
  both shapes; test fixtures reshaped to arrays)
- QwenLM#5729 active-runtime-model listing (pre-QwenLM#5089 getAllConfiguredModels
  already enumerates Object.values(AuthType), so the runtime model is
  included natively)
- QwenLM#5728 ACP set_config_option deterministic provider fixture (reshaped to
  array; the flake fix is preserved)

KNOWN DOWNGRADE CAVEAT: settings already migrated to $version:5 (shipped in
v0.19.0) retain the v5 `{ protocol, models }` modelProviders shape, which
the reverted ModelRegistry consumes as an array. Such settings will throw
on load until re-configured. A v5->v4 downgrade guard/migration is a
separate follow-up if backward compatibility for migrated users is needed.
…vert

After reverting QwenLM#5089, settings already migrated to $version:5 (shipped in
v0.19.0) carry a modelProviders `{ protocol, models }` shape that the
reverted v4 readers consume as arrays, throwing "models is not iterable"
on load. This adds the inverse migration so those configs auto-converge to
v4 on load (the user-facing "automatically migrate $version:5 to 4").

- V5ToV4Migration: unwraps each modelProviders `{ protocol, models }` back
  to its `models` array, drops the now-implicit protocol (warning only when
  the explicit protocol differs from the key-derived one), and resets
  $version to 4.
- DOWNGRADE_MIGRATIONS keeps the downgrade out of the ascending forward
  ALL_MIGRATIONS chain (preserving its invariants); runMigrations and
  needsMigration consider both via a combined convergence set.
- needsMigration now gates on `=== SETTINGS_VERSION` instead of `>=`, so a
  newer-but-handled version (v5) is reported as needing migration while a
  genuinely unknown newer version (v6+) is still left untouched.

Covered by unit tests for the migration, the framework wiring, and an
end-to-end loadSettings downgrade-on-load test.
The integration suites hard-coded CURRENT_SETTINGS_VERSION = 5 (introduced
by QwenLM#5676), which mismatched the reverted SETTINGS_VERSION = 4 and failed the
migration assertions ($version now writes 4, not 5). Revert the constant to
4 in both settings-migration and qwen-config-dir integration tests.

Verified: QWEN_SANDBOX=false vitest run --root ./integration-tests
cli/settings-migration.test.ts cli/qwen-config-dir.test.ts → 21 passed.
Addresses /qreview feedback on the revert:

- vscode findOpenaiModels: restore read-side tolerance for the V5
  { protocol, models } shape. The extension reads/writes settings.json
  without running the CLI v5->v4 migration, so a not-yet-downgraded
  $version:5 file would otherwise return [] and silently drop existing
  OpenAI models on the next write. (Critical)
- modelRegistry.registerAuthTypeModels: guard against a non-array provider
  value (skip + warn) instead of throwing an opaque "models is not
  iterable" — covers hand-edited or unmigrated files the downgrade misses.
- needsMigration JSDoc: update the stale ">= SETTINGS_VERSION" wording to
  match the "=== SETTINGS_VERSION, else fall through" logic the downgrade
  path depends on.
- settings.test.ts: also assert the v5->v4 downgrade is persisted to disk
  (.tmp write-back), not just the in-memory merged result.

Adds tests for the registry guard and the vscode V5 read tolerance.
@wenshao
wenshao dismissed stale reviews from qqqys and qwen-code-ci-bot via 29cf90e June 23, 2026 07:40
@wenshao
wenshao force-pushed the revert-5089-structural branch from da43b37 to 29cf90e Compare June 23, 2026 07:40
@wenshao

wenshao commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

✅ Maintainer local verification — real qwen binary (tmux TUI + headless)

Built the actual qwen binary from the PR head and drove it, not just unit tests.

Setup — worktree at PR head da43b37a0 (base origin/main d350dd8), fresh npm ci; macOS (darwin), Node v22.22.2.

1. Build · typecheck · targeted suites — all green

Check Command Result
Build npm run build ✅ exit 0, no errors
Typecheck npm run typecheck (acp-bridge, audio-capture, cli, core, sdk, webui) ✅ all pass
VSCode types npm --prefix packages/vscode-ide-companion run check-types ✅ pass
Migration suites vitest run --root packages/cli src/config/migration/ 137 / 6 files (incl. new v5-to-v4.test.ts = 17)
CLI config vitest run --root packages/cli src/config/ 764 / 23 files
Core models/providers vitest run --root packages/core src/models/ src/providers/ src/core/contentGenerator.test.ts 328 / 18 files

2. The new downgrade tests are non-vacuous (mutation test)

Neutered the core transform in v5-to-v4.tsproviders[key] = value instead of unwrapping value.models — and re-ran the suites: 5 tests FAIL in v5-to-v4.test.ts + index.test.ts (e.g. "downgrades v5 modelProviders back to v4 arrays", "treats a wrapped config with missing models as an empty array"). Restoring the line → 17/17 green again. The tests genuinely guard the unwrap, not just that a migration ran.

3. Real-binary A/B — the v5→v4 downgrade migration (the marquee fix)

The same $version: 5 settings file (modelProviders.openai = { protocol, models: [...] } — the shape v0.19.0 shipped) was fed to two binaries:

PRE-FIX (pure revert) FIXED (this PR)
Boot TypeError: models is not iterable ✅ boots normally
Crash site ModelRegistry.registerAuthTypeModelsnew ConfigloadCliConfig — (config initializes fully)
Real TUI (tmux) never renders, exit 1 renders; /model lists [openai] GPT-4o (gpt-4o)
settings.json on disk unchanged — stuck at $version: 5 auto-rewritten to $version: 4, providers → arrays

PRE-FIX — crash on the main CLI boot path (real binary, captured in tmux):

An unexpected critical error occurred:
TypeError: models is not iterable
    at ModelRegistry.registerAuthTypeModels (.../models/modelRegistry.js)
    at new ModelRegistry (.../models/modelRegistry.js)
    at new ModelsConfig (.../models/modelsConfig.js)
    at new Config (.../config/config.js)
    at loadCliConfig (.../cli/dist/src/config/config.js)
PRE_FIX_EXIT=1

Because the throw is in loadCliConfig → new Config, a plain revert of #5089 without this PR would brick every launch for any user whose config v0.19.0 had already migrated to v5.

FIXED — the file is auto-migrated on load:

BEFORE   $version: 5   modelProviders.openai = { protocol, models: [ { id: gpt-4o } ] }
AFTER    $version: 4   modelProviders.openai = [ { id: gpt-4o } ]

…and the migrated model is live and selectable in the real TUI (/model):

│ Select Model                  │
│ › 1. [openai] GPT-4o (gpt-4o) │

(Headless, the FIXED binary gets all the way to a real model API call — failing only with 401 on the dummy key, i.e. past the exact new Config step PRE-FIX dies in.)

Multi-provider (mirrors a real openai + anthropic config) — all providers, models and order preserved:

BEFORE   $ver=5   openai=obj                       anthropic=obj
AFTER    $ver=4   openai=[gpt-4o, qwen3.7-max]     anthropic=[claude-opus-4-6]

Not run (disclosed)

  • integration-tests/ — needs the full build+run rig; not executed (consistent with the author's note). The reshaped acp-integration.test.ts fixture (object → array) matches the reverted readProviderModels, which now reads arrays directly — confirmed by reading the diff, not by execution.

Verdict

The revert builds and typechecks clean, the preserved features' suites pass, the new downgrade tests are non-vacuous, and the real binary confirms the exact failure this PR exists to prevent (models is not iterable on the main boot path) is fixed: a v5 config auto-migrates to v4 and its models stay usable. LGTM from a runtime-verification standpoint.

🇨🇳 中文版

✅ 维护者本地验证 —— 真实 qwen 二进制(tmux TUI + headless)

从 PR head 构建真实 qwen 二进制并实际驱动运行,不止跑单测。

环境 —— 在 PR head da43b37a0(base origin/main d350dd8)建独立 worktree,全新 npm ci;macOS(darwin),Node v22.22.2

1. 构建 · 类型检查 · 定向套件 —— 全绿

检查项 命令 结果
构建 npm run build ✅ exit 0,无报错
类型检查 npm run typecheck(acp-bridge、audio-capture、cli、core、sdk、webui) ✅ 全部通过
VSCode 类型 npm --prefix packages/vscode-ide-companion run check-types ✅ 通过
迁移套件 vitest run --root packages/cli src/config/migration/ 137 / 6 文件(含新增 v5-to-v4.test.ts = 17)
CLI config vitest run --root packages/cli src/config/ 764 / 23 文件
Core models/providers vitest run --root packages/core src/models/ src/providers/ src/core/contentGenerator.test.ts 328 / 18 文件

2. 新增的降级测试非空过(变异测试)

v5-to-v4.ts 的核心转换中和 —— 用 providers[key] = value 取代解包 value.models —— 重跑套件:v5-to-v4.test.ts + index.test.ts5 个测试 FAIL(如 "downgrades v5 modelProviders back to v4 arrays""treats a wrapped config with missing models as an empty array")。恢复该行后 → 重新 17/17 全绿。说明测试真正守卫了 unwrap 转换,而非只校验"有迁移跑过"。

3. 真实二进制 A/B —— v5→v4 降级迁移(核心修复)

同一份 $version: 5 设置文件(modelProviders.openai = { protocol, models: [...] },即 v0.19.0 发布时写入的形状)分别喂给两个二进制:

PRE-FIX(纯回退) FIXED(本 PR)
启动 TypeError: models is not iterable ✅ 正常启动
崩溃点 ModelRegistry.registerAuthTypeModelsnew ConfigloadCliConfig —(config 完整初始化)
真实 TUI(tmux) 界面从未渲染,exit 1 正常渲染;/model 列出 [openai] GPT-4o (gpt-4o)
磁盘上的 settings.json 不变 —— 卡在 $version: 5 自动改写为 $version: 4,provider → 数组

PRE-FIX —— 崩在主 CLI 启动路径(真实二进制,tmux 捕获):

An unexpected critical error occurred:
TypeError: models is not iterable
    at ModelRegistry.registerAuthTypeModels (.../models/modelRegistry.js)
    at new ModelRegistry (.../models/modelRegistry.js)
    at new ModelsConfig (.../models/modelsConfig.js)
    at new Config (.../config/config.js)
    at loadCliConfig (.../cli/dist/src/config/config.js)
PRE_FIX_EXIT=1

抛错位置在 loadCliConfig → new Config,因此对 #5089 做纯回退而不带本 PR,会让任何在 v0.19.0 已迁移到 v5 的用户每次启动都崩

FIXED —— 文件在加载时被自动迁移:

BEFORE   $version: 5   modelProviders.openai = { protocol, models: [ { id: gpt-4o } ] }
AFTER    $version: 4   modelProviders.openai = [ { id: gpt-4o } ]

迁移后的模型在真实 TUI 里可见、可选/model):

│ Select Model                  │
│ › 1. [openai] GPT-4o (gpt-4o) │

(headless 下 FIXED 一路走到真实模型 API 调用 —— 只因 dummy key 返回 401,即已越过 PRE-FIX 崩溃的那个 new Config 步骤。)

多 provider(对应真实的 openai + anthropic 配置)—— 所有 provider、模型与顺序都保留:

BEFORE   $ver=5   openai=obj                       anthropic=obj
AFTER    $ver=4   openai=[gpt-4o, qwen3.7-max]     anthropic=[claude-opus-4-6]

未执行(如实说明)

  • integration-tests/ —— 需要完整 build+run harness,未执行(与作者说明一致)。被改回数组的 acp-integration.test.ts fixture(object → array)与回退后的 readProviderModels 一致 —— 后者现在直接读数组,已通过阅读 diff 确认,但未实际运行。

结论

回退构建与类型检查干净,被保留功能的套件通过,新增降级测试非空过,真实二进制确认本 PR 要防的那个故障(主启动路径上的 models is not iterable)已修复:v5 配置自动迁移到 v4,模型仍可用。从运行时验证角度 LGTM。

… paths

Addresses /review suggestions on the revert:

- contentGenerator: import PROVIDER_SOURCED_FIELDS from constants.js (where
  it is actually defined) instead of modelsConfig.js, breaking the runtime
  import cycle contentGenerator -> modelsConfig -> contentGenerator.
  constants.js only references contentGenerator at the type level, which is
  erased at runtime, so no cycle remains.
- contentGenerator.test: add coverage for the two authType error paths the
  revert restored (missing authType -> "must have an authType"; unknown
  authType -> "Unsupported authType"), which QwenLM#5089's protocol-based tests
  had replaced. Neither was covered before.

The acpAgent z.nativeEnum(AuthType).parse(methodId) suggestion is left as-is:
that line is byte-identical to pre-QwenLM#5089, so it is pre-existing behavior the
revert faithfully restores rather than a regression of this PR.

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@wenshao
wenshao merged commit 663967e into QwenLM:main Jun 23, 2026
53 checks passed
@wenshao
wenshao deleted the revert-5089-structural branch June 23, 2026 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor: Decouple Provider Identity from SDK Protocol

4 participants