fix(cli): keep settings v5 migration idempotent - #5676
Merged
Conversation
Collaborator
|
Thanks for the PR, @yiliang114! Template looks good ✓ — all sections present, bilingual, test plan with evidence. On direction: this is a straightforward release-blocking bug fix. The v5 migration not being idempotent for settings without legacy providers is a real problem that broke CI — aligns perfectly with the project's need for reliable settings migration. No concerns here. On approach: the fix itself is admirably minimal — 3 lines in CI is showing Lint and Test failures on this run (still in progress, logs not yet available) — worth keeping an eye on. Moving on to code review and testing. 🔍 中文说明感谢 PR，@yiliang114！ 模板完整 ✓ — 所有章节齐全，双语，测试计划带证据。 方向：这是一个直接明确的 release 阻塞级 bug 修复。v5 migration 对没有旧 provider 数组的 settings 不具备幂等性，导致 CI 失败——完全符合项目对 settings migration 可靠性的需求。没有疑虑。 方案：修复本身非常精简——在 CI 当前显示 Lint 和 Test 失败（仍在运行中，日志尚不可用）——值得关注。 进入代码审查和测试阶段 🔍 — Qwen Code · qwen3.7-max |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a settings migration edge case where v4 settings could remain at version 4 after the first startup (non-idempotent migration), and updates integration tests to align with the current settings version and model-selection behavior.
Changes:
- Make the v4→v5 migration always execute for
$version: 4settings so version metadata is bumped on first run even when no provider arrays need reshaping. - Update unit/integration tests to expect settings version 5 and add coverage for “version bump only” v4→v5 migration.
- Adjust ACP and file-system integration tests to remove stale assumptions and stabilize expectations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/config/migration/versions/v4-to-v5.ts | Ensures v4 settings always run the v4→v5 migration, making the upgrade idempotent on first startup. |
| packages/cli/src/config/migration/versions/v4-to-v5.test.ts | Updates shouldMigrate expectations and adds coverage for bumping v4 settings without modelProviders. |
| integration-tests/cli/settings-migration.test.ts | Updates assertions to settings v5 via a local “current version” constant. |
| integration-tests/cli/qwen-config-dir.test.ts | Updates migration-version assertions and seeded settings version for QWEN_HOME scenarios. |
| integration-tests/cli/file-system.test.ts | Tightens the prompt for writing to spaced paths and loosens content assertion via trimming. |
| integration-tests/cli/acp-integration.test.ts | Removes provider-name substring assumption and reuses the current model id in set-config test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
112
to
+113
| const newFileContent = rig.readFile(fileName); | ||
| expect(newFileContent).toBe('hello'); | ||
| expect(newFileContent.trimEnd()).toBe('hello'); |
Comment on lines
+511
to
+516
| const modelId = modelOption!.currentValue; | ||
| expect( | ||
| newSession.models.availableModels.some( | ||
| (model) => model.modelId === modelId, | ||
| ), | ||
| ).toBe(true); |
| v3GitCoAuthorBooleanSettings, | ||
| } = workspacesSettings; | ||
|
|
||
| const CURRENT_SETTINGS_VERSION = 5; |
Comment on lines
31
to
+33
| import { join, resolve } from 'node:path'; | ||
|
|
||
| const CURRENT_SETTINGS_VERSION = 5; |
yiliang114
enabled auto-merge (squash)
June 22, 2026 17:21
qwen-code-ci-bot
approved these changes
Jun 22, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Collaborator
Code ReviewThe fix in The test updates are mechanical and correct: hardcoded The Real-Scenario TestingCreated v4 settings files (no legacy providers) and ran both the installed CLI and the PR code to verify migration behavior: Before (installed qwen v0.18.5)Bug confirmed: settings stay at After (this PR via
|
Collaborator
|
This is a clean, well-scoped bug fix that does exactly what it says. The root cause analysis in the PR description is accurate — the v5 migration's The before/after tmux test is the clearest evidence: installed qwen leaves Minor note: the The CI failures on this PR's run are still in progress — logs aren't available yet. Worth monitoring, but the local test results are comprehensive. Approving. ✅ 中文说明这是一个干净、范围明确的 bug 修复，完全做到了它声称的事情。PR 描述中的根因分析准确——v5 migration 的 tmux 前后对比测试是最清晰的证据：已安装的 qwen 让 小提示： 此 PR 的 CI 失败仍在运行中——日志尚不可用。值得监控，但本地测试结果全面。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 22, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Contributor
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. |
wenshao
added a commit
that referenced
this pull request
Jun 22, 2026
…st (#5721) PR #5676 (an unrelated v5 settings-migration fix) inadvertently reverted the model-selection assertion in the ACP integration test back to a flaky form that asserts the session's current model is present in availableModels. In CI (no real auth) the default current model is not in availableModels, so the assertion fails with "expected false to be true", breaking the Release workflow's "Integration Tests (No Sandbox)" job and gating the Publish Release step. Restore the robust form that explicitly picks an openai model from availableModels (matching the version that shipped in every successful release since March), which avoids the auth-dependent current-model mismatch.
wenshao
added a commit
to wenshao/qwen-code
that referenced
this pull request
Jun 23, 2026
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.
wenshao
added a commit
to wenshao/qwen-code
that referenced
this pull request
Jun 23, 2026
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.
wenshao
added a commit
that referenced
this pull request
Jun 23, 2026
#5745) * revert(core): revert Protocol enum & model-identity decoupling (#5089) Reverts the structural changes from #5089 back to the pre-#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 #5089 are kept and re-adapted to the old enum+array structure (not reverted): - #5632 fastOnly/voiceOnly model flags (test fixtures reshaped to arrays) - #5638 workspace provider defaults (readProviderModels already tolerates both shapes; test fixtures reshaped to arrays) - #5729 active-runtime-model listing (pre-#5089 getAllConfiguredModels already enumerates Object.values(AuthType), so the runtime model is included natively) - #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. * feat(cli): add v5->v4 settings downgrade migration for #5089 revert After reverting #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. * fix(test): align integration settings-version constant with reverted v4 The integration suites hard-coded CURRENT_SETTINGS_VERSION = 5 (introduced by #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. * fix: harden v5-era settings handling on the #5089 revert path 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. * fix(core): break contentGenerator import cycle + cover reverted error 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 #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-#5089, so it is pre-existing behavior the revert faithfully restores rather than a regression of this PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release failure cause
This PR targets the release failures from run 27965159059, including jobs 82757723498 and 82757723621.
Root cause: after the settings version moved from 4 to 5 in #5089, the v5 migration only ran when legacy provider arrays existed. Some older settings could therefore stop at version 4 on the first startup and only normalize to version 5 on a later startup, which broke migration idempotency. The release integration tests also still asserted version 4, so the current behavior failed with
expected 5 to be 4. Separately, ACP coverage still assumed an available model ID would includeopenai, which became stale after protocol and model identity were decoupled.What this PR does
This PR makes the settings v5 migration complete in one startup for all v4 settings, even when there are no legacy provider arrays to reshape. It also updates the affected integration coverage to the current settings version and removes the stale ACP model-ID assumption.
Why it's needed
This fixes the release-only failure path instead of reverting the settings version bump: old settings now migrate directly to the current version, the integration tests assert the current version, and ACP coverage no longer depends on provider names being embedded in model IDs.
Reviewer Test Plan
How to verify
Confirm that old settings migrate directly to the current settings version on first startup, repeated startup migration produces the same result, ACP model switching works without assuming an
openaimodel ID, and file writing to a path with spaces still writes the expected content.Evidence (Before & After)
Before: release logs showed
settings-migrationandqwen-config-dirintegration tests failing withexpected 5 to be 4, and ACP coverage contained a stale assumption that an available model ID would includeopenai.After:
npm run build: passed with exit code 0. Existing warnings only: VS Code companion curly-rule warnings and stale Browserslist data.npm run bundle: passed with exit code 0.cd integration-tests && QWEN_SANDBOX=false npx vitest run cli/settings-migration.test.ts cli/qwen-config-dir.test.ts cli/acp-integration.test.ts: passed, 3 test files, 32 tests.cd packages/cli && npx vitest run src/config/migration/versions/v4-to-v5.test.ts src/config/migration/index.test.ts src/config/settings.test.ts: passed, 3 test files, 179 tests.cd integration-tests && QWEN_SANDBOX=false npx vitest run cli/file-system.test.ts cli/simple-mcp-server.test.ts: passed, 2 test files, 6 tests passed and 1 skipped.git diff --check: passed with no output.Docker sandbox verification was attempted locally, but Docker was not available:
Cannot connect to the Docker daemon at unix:///var/run/docker.sock.Tested on
Environment (optional)
Node.js v22.20.0, no-sandbox integration tests. Docker sandbox was not locally available.
Risk & Scope
Linked Issues
Refs #5665
中文说明
Release 失败原因
这个 PR 针对 release run 27965159059 的失败，包括 jobs 82757723498 和 82757723621。
根因：#5089 把 settings version 从 4 升到 5 后，v5 migration 只有在存在旧 provider 数组时才会执行。一些旧 settings 会在第一次启动后停在 version 4，直到后续启动才 normalize 到 version 5，破坏 migration idempotency。release integration tests 也还在断言 version 4，所以当前行为会报
expected 5 to be 4。另外，ACP 覆盖里仍假设可用 model ID 会包含openai，这在 protocol 和 model identity 解耦后已经过期。What this PR does
这个 PR 让 settings v5 migration 对所有 v4 settings 都能在第一次启动时完成，即使 settings 中没有需要转换的旧 provider 数组。它也同步更新了受影响的 integration tests 到当前 settings version，并移除了 ACP 测试里的旧 model-ID 假设。
Why it's needed
这个 PR 修复 release-only 的失败路径，而不是回退 settings version bump：旧 settings 现在会直接迁移到当前 version，integration tests 断言当前 version，ACP 覆盖也不再依赖 provider 名称被嵌入 model ID。
Reviewer Test Plan
How to verify
确认旧 settings 在第一次启动时直接迁移到当前 settings version，重复运行 migration 结果保持一致，ACP 切换 model 不再依赖
openai出现在 model ID 中，并且带空格路径写文件仍能写入预期内容。Evidence (Before & After)
Before：release logs 显示
settings-migration和qwen-config-dirintegration tests 报expected 5 to be 4，ACP 覆盖里也有“可用 model ID 会包含openai”的旧假设。After：
npm run build: 通过，exit code 0。只有既有 warning：VS Code companion curly-rule warnings 和 Browserslist 数据过期提示。npm run bundle: 通过，exit code 0。cd integration-tests && QWEN_SANDBOX=false npx vitest run cli/settings-migration.test.ts cli/qwen-config-dir.test.ts cli/acp-integration.test.ts: 通过，3 个 test files，32 个 tests。cd packages/cli && npx vitest run src/config/migration/versions/v4-to-v5.test.ts src/config/migration/index.test.ts src/config/settings.test.ts: 通过，3 个 test files，179 个 tests。cd integration-tests && QWEN_SANDBOX=false npx vitest run cli/file-system.test.ts cli/simple-mcp-server.test.ts: 通过，2 个 test files，6 个 tests passed，1 个 skipped。git diff --check: 通过，无输出。本地尝试过 Docker sandbox 验证，但 Docker 不可用：
Cannot connect to the Docker daemon at unix:///var/run/docker.sock。Tested on
Environment (optional)
Node.js v22.20.0，no-sandbox integration tests。本地 Docker sandbox 不可用。
Risk & Scope
Linked Issues
Refs #5665