fix(test): make ACP set_config_option test use a deterministic openai provider model#5728
Conversation
… provider model Follow-up to QwenLM#5724. The `supports session/set_config_option for mode and model` test asserted that an openai model appears in `availableModels`, which depended on the env-driven OPENAI_MODEL being captured as a runtime-model snapshot and enumerated at session/new. That capture is environment-sensitive: in CI the openai model was absent from `availableModels`, failing `expect(openaiModel).toBeDefined()` (acp-integration.test.ts:535). The earlier QWEN_HOME isolation (QwenLM#5724) did not address this. Inject an openai provider model via `modelProviders` in the test's settings so the model is a registry entry that is always enumerated and switchable without inference. The test now targets that specific model, making it deterministic regardless of how the ambient openai credentials resolve. Verified locally to pass both with and without OPENAI_MODEL set.
|
Thanks for the PR! Template looks good ✓ On direction: this is a straightforward CI-flakiness fix — the ACP On approach: the scope is minimal and focused — one file, +28/-3, only touches the failing test. The Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 方向:这是一个直接的 CI 稳定性修复——ACP 方案:范围最小且聚焦——一个文件,+28/-3,只改了失败的测试。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe diff is clean and minimal. One file, +28/-3 — exactly the scope the PR describes. The approach injects a deterministic No correctness bugs, no security concerns, no AGENTS.md violations. The comment block is slightly verbose for a test change but justified given this is the third attempt at fixing this flake — the context helps future readers. TestingTarget test (without Sibling tests (confirming no regression): All 5 non-inference tests pass ( 中文说明代码审查Diff 干净且最小化。一个文件,+28/-3——与 PR 描述完全一致。 方案通过现有的 无正确性 bug、无安全隐患、无 AGENTS.md 违规。注释块对测试改动来说稍长,但考虑到这是第三次修复这个 flake,上下文有助于后续读者理解。 测试目标测试(不设 — Qwen Code · qwen3.7-max |
|
This is a clean, minimal test-only fix that does exactly what it says. The CI flake at The code review found no issues. The test results confirm the fix works: the target test passes both with and without The PR description and inline comments are thorough — helpful given this is the third attempt at this flake. Future readers will understand why the deterministic model was injected and what problem it solves. Approving. ✅ 中文说明这是一个干净、最小化的仅测试修复,完全符合描述。 代码审查无问题。测试结果确认修复有效:目标测试在有/无 PR 描述和内联注释详尽——考虑到这是第三次修复此 flake,这对后续读者很有帮助。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
There was a problem hiding this comment.
Approved. I reviewed the failing E2E context, this patch, and the ACP session/set_config_option path.
The change removes the environment-sensitive dependency on ambient OPENAI_MODEL runtime-model snapshot enumeration by injecting a deterministic openai provider model in the test settings. It still exercises the real session/set_config_option -> session.setModel(..., persistDefault:false) -> buildConfigOptions round trip, so it fixes the flaky assertion without masking the behavior under test.
I did not find additional code issues that need to be fixed in this PR. The current triage check failure appears unrelated to the patch: the job timed out inside qwen-code-action after 483s, rather than failing on the PR contents.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
Clean, well-reasoned test-only change. The modelProviders injection correctly targets a deterministic registry model, and the settings shape matches ModelProvidersConfig (protocol, models, baseUrl, envKey all valid). Build passes, type-check clean.
— qwen3.7-max via Qwen Code /review
…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.
#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.
What this PR does
Makes the ACP integration test
supports session/set_config_option for mode and modelinject a deterministic openai provider model viamodelProvidersin the test's settings, and target that specific model when exercisingsession/set_config_option. Test-only change.Why it's needed
The test asserted that an openai model appears in the session's
availableModels(expect(openaiModel).toBeDefined()). That entry only exists if the ambientOPENAI_MODELis captured as a runtime-model snapshot and enumerated atsession/new— a path that is environment-sensitive. In CI the openai model was absent fromavailableModels, so the assertion failed atacp-integration.test.ts:535, which fails the Integration Tests (No Sandbox) job and gates Publish Release. This persisted across the previous two attempts: #5721 (restored the assertion shape) and #5724 (isolatedQWEN_HOMEper agent) — neither addressed the dependency on env-driven runtime-model capture.A model declared via
modelProvidersis a registry entry: it is always enumerated inavailableModelsand is switchable viaset_config_optionwithout any inference. Targeting an injected provider model removes the dependency on the fragile runtime-capture path, so the test is deterministic regardless of how the ambient openai credentials resolve in a given environment.Reviewer Test Plan
How to verify
npm run bundleExpected: pass. It also passes without
OPENAI_MODELset — proving the assertion no longer depends on env-driven runtime-model enumeration:The sibling tests (
returns error for invalid configId in set_config_option,returns internal error details when model auth is required) are untouched and still pass.Evidence (Before & After)
N/A (test-only). Before:
availableModelscontained no openai entry in CI →expect(openaiModel).toBeDefined()failed → No-Sandbox job exited 1 → Publish Release skipped. After: an injected openai registry model is always present and switchable, so the find + set + round-trip assertions are deterministic.Tested on
macOS: built the bundle and ran the target test both with and without
OPENAI_MODEL(pass), plus the two sibling tests (pass). Windows/Linux not run locally; the authoritative check is the CI No-Sandbox job.Environment (optional)
npm run bundle+vitest run --root ./integration-testswith fakeOPENAI_*env,QWEN_SANDBOX=false.Risk & Scope
modelProvidersentry to one test's settings and narrows the model it targets. The test still exercises the realsession/set_config_optionmodel round-trip.Linked Issues
Refs the failing No-Sandbox runs at
acp-integration.test.ts:535. Follow-up to #5721 and #5724.中文说明
这个 PR 做了什么
让 ACP 集成测试
supports session/set_config_option for mode and model通过测试 settings 里的modelProviders注入一个确定性的 openai provider 模型,并在跑session/set_config_option时精确针对这个模型。仅改测试。为什么需要
该测试断言会话的
availableModels里存在一个 openai 模型(expect(openaiModel).toBeDefined())。这个条目只有在环境变量OPENAI_MODEL被捕获为 runtime-model 快照并在session/new时枚举出来才存在——而这条路径对环境很敏感。在 CI 里 openai 模型不在availableModels中,断言在acp-integration.test.ts:535失败,从而让 Integration Tests (No Sandbox) job 失败,而它 gating Publish Release。前两次尝试都没解决:#5721(恢复断言形态)和 #5724(给每个 agent 隔离QWEN_HOME)都没触及"依赖 env 驱动的 runtime 模型捕获"这个根本点。通过
modelProviders声明的模型是 registry 条目:它一定会出现在availableModels里,并且能在不做任何推理的情况下通过set_config_option切换。针对一个注入的 provider 模型,就把测试从脆弱的 runtime 捕获路径上解耦出来,使它不受环境里 openai 凭证如何解析的影响,变得确定。评审验证
npm run bundle预期通过。在不设
OPENAI_MODEL时也通过——证明断言不再依赖 env 驱动的 runtime 模型枚举:同文件的另外两个用例(
returns error for invalid configId in set_config_option、returns internal error details when model auth is required)未改动,仍通过。证据(Before & After)
N/A(仅改测试)。Before:CI 里
availableModels无 openai 条目 →expect(openaiModel).toBeDefined()失败 → No-Sandbox job 退出码 1 → Publish Release 被跳过。After:注入的 openai registry 模型一定存在且可切换,find + set + 往返断言都确定。测试平台
macOS:构建 bundle 后,对目标用例分别在设/不设
OPENAI_MODEL下各跑一次(通过),外加两个同文件用例(通过)。Windows/Linux 未在本地运行;以 CI 的 No-Sandbox job 为权威校验。风险与范围
modelProviders条目并收窄了它针对的模型。测试仍然走真实的session/set_config_option模型往返。关联
关联
acp-integration.test.ts:535的 No-Sandbox 失败。是 #5721 与 #5724 的后续。