Skip to content

fix(test): make ACP set_config_option test use a deterministic openai provider model#5728

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
wenshao:fix/acp-config-option-deterministic-model
Jun 23, 2026
Merged

fix(test): make ACP set_config_option test use a deterministic openai provider model#5728
wenshao merged 1 commit into
QwenLM:mainfrom
wenshao:fix/acp-config-option-deterministic-model

Conversation

@wenshao

@wenshao wenshao commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Makes the ACP integration test supports session/set_config_option for mode and model inject a deterministic openai provider model via modelProviders in the test's settings, and target that specific model when exercising session/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 ambient OPENAI_MODEL is captured as a runtime-model snapshot and enumerated at session/new — a path that is environment-sensitive. In CI the openai model was absent from availableModels, so the assertion failed at acp-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 (isolated QWEN_HOME per agent) — neither addressed the dependency on env-driven runtime-model capture.

A model declared via modelProviders is a registry entry: it is always enumerated in availableModels and is switchable via set_config_option without 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

  1. npm run bundle
  2. Run the test against the bundle:
cd integration-tests && OPENAI_API_KEY=sk-fake OPENAI_BASE_URL=https://api.openai.com/v1 OPENAI_MODEL=qwen3.7-max QWEN_SANDBOX=false \
  npx vitest run cli/acp-integration.test.ts -t "for mode and model"

Expected: pass. It also passes without OPENAI_MODEL set — proving the assertion no longer depends on env-driven runtime-model enumeration:

cd integration-tests && OPENAI_API_KEY=sk-fake OPENAI_BASE_URL=https://api.openai.com/v1 QWEN_SANDBOX=false \
  npx vitest run cli/acp-integration.test.ts -t "for mode and model"

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: availableModels contained 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

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

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-tests with fake OPENAI_* env, QWEN_SANDBOX=false.

Risk & Scope

  • Main risk or tradeoff: None for production — the change only adds a modelProviders entry to one test's settings and narrows the model it targets. The test still exercises the real session/set_config_option model round-trip.
  • Not validated / out of scope: The exact CI-environment condition under which the env-driven runtime openai model fails to enumerate was not reproduced locally (it reproduces only in CI); this change makes the test independent of that condition rather than diagnosing it. Inference-driven ACP tests were not re-run (need real provider auth).
  • Breaking changes / migration notes: None.

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 凭证如何解析的影响,变得确定。

评审验证

  1. npm run bundle
  2. 对着 bundle 跑测试:
cd integration-tests && OPENAI_API_KEY=sk-fake OPENAI_BASE_URL=https://api.openai.com/v1 OPENAI_MODEL=qwen3.7-max QWEN_SANDBOX=false \
  npx vitest run cli/acp-integration.test.ts -t "for mode and model"

预期通过。在不设 OPENAI_MODEL 时也通过——证明断言不再依赖 env 驱动的 runtime 模型枚举:

cd integration-tests && OPENAI_API_KEY=sk-fake OPENAI_BASE_URL=https://api.openai.com/v1 QWEN_SANDBOX=false \
  npx vitest run cli/acp-integration.test.ts -t "for mode and model"

同文件的另外两个用例(returns error for invalid configId in set_config_optionreturns 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 + 往返断言都确定。

测试平台

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

macOS:构建 bundle 后,对目标用例分别在设/不设 OPENAI_MODEL 下各跑一次(通过),外加两个同文件用例(通过)。Windows/Linux 未在本地运行;以 CI 的 No-Sandbox job 为权威校验。

风险与范围

  • 主要风险:对生产代码零风险——只在一个测试的 settings 里加了一个 modelProviders 条目并收窄了它针对的模型。测试仍然走真实的 session/set_config_option 模型往返。
  • 未验证/范围外:env 驱动的 runtime openai 模型在何种 CI 环境条件下枚举失败,未在本地复现(只在 CI 复现);本改动是让测试不依赖该条件,而非诊断它。依赖推理的 ACP 用例未重跑(需真实 provider 鉴权)。
  • 破坏性变更:无。

关联

关联 acp-integration.test.ts:535 的 No-Sandbox 失败。是 #5721#5724 的后续。

… 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.
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

On direction: this is a straightforward CI-flakiness fix — the ACP set_config_option test depended on env-driven runtime-model enumeration that's unreliable in CI, gating Publish Release. Making the test deterministic by injecting a registry model is exactly the right approach. No direction concerns.

On approach: the scope is minimal and focused — one file, +28/-3, only touches the failing test. The TestRig.setup() API already supports options.settings merging, so this leverages existing infrastructure cleanly. The injected modelProviders entry is deterministic and the test still exercises the real session/set_config_option round-trip. No simpler path needed.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

方向:这是一个直接的 CI 稳定性修复——ACP set_config_option 测试依赖 env 驱动的 runtime-model 枚举,在 CI 中不可靠,阻塞了 Publish Release。通过注入 registry 模型使测试确定化,是正确的做法。没有方向上的顾虑。

方案:范围最小且聚焦——一个文件,+28/-3,只改了失败的测试。TestRig.setup() API 已经支持 options.settings 合并,所以是复用了现有基础设施。注入的 modelProviders 条目是确定性的,测试仍走真实的 session/set_config_option 往返。无需更简路径。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

The diff is clean and minimal. One file, +28/-3 — exactly the scope the PR describes.

The approach injects a deterministic modelProviders entry into the test settings via the existing TestRig.setup({ settings: { ... } }) API, then targets the injected model by ID (e2e-set-config-option-model) instead of substring-matching openai. This removes the dependency on env-driven runtime-model enumeration that flaked in CI. The test still exercises the real session/set_config_option model round-trip, so coverage intent is preserved.

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.

Testing

Target test (without OPENAI_MODEL — proves the fix doesn't depend on env):

runner@runnervm7b5n9:~/work/qwen-code/qwen-code/integration-tests$ OPENAI_API_KEY=sk-fake OPENAI_BASE_URL=https://api.openai.com/v1 QWEN_SANDBOX=false npx vitest run cli/acp-integration.test.ts -t 'for mode and model' 2>&1 | tee /home/runner/work/qwen-code/qwen-code/tmp/triage-5728-014552/test.log

 RUN  v3.2.4 /home/runner/work/qwen-code/qwen-code/integration-tests

 ✓ cli/acp-integration.test.ts (11 tests | 10 skipped) 1108ms
   ✓ acp integration > supports session/set_config_option for mode and model  1107ms

 Test Files  1 passed (1)
      Tests  1 passed | 10 skipped (11)
   Start at  01:45:53
   Duration  1.53s (transform 136ms, setup 0ms, collect 84ms, tests 1.11s, environment 0ms, prepare 56ms)

Sibling tests (confirming no regression):

runner@runnervm7b5n9:~/work/qwen-code/qwen-code/integration-tests$ OPENAI_API_KEY=sk-fake OPENAI_BASE_URL=https://api.openai.com/v1 QWEN_SANDBOX=false npx vitest run cli/acp-integration.test.ts -t 'set_config_option' 2>&1 | tee /home/runner/work/qwen-code/qwen-code/tmp/triage-5728-014552/siblings.log

 RUN  v3.2.4 /home/runner/work/qwen-code/qwen-code/integration-tests

 ✓ cli/acp-integration.test.ts (11 tests | 9 skipped) 2219ms
   ✓ acp integration > supports session/set_config_option for mode and model  1101ms
   ✓ acp integration > returns error for invalid configId in set_config_option  1117ms

 Test Files  1 passed (1)
      Tests  2 passed | 9 skipped (11)
   Start at  01:50:09
   Duration  2.63s (transform 141ms, setup 0ms, collect 84ms, tests 2.22s, environment 0ms, prepare 79ms)

All 5 non-inference tests pass (initializes and allows setting mode, returns internal error details when model auth is required, target test, sibling test, receives available_commands_update with slash commands). The 6 failing tests require real API keys for inference — unrelated to this PR.

中文说明

代码审查

Diff 干净且最小化。一个文件,+28/-3——与 PR 描述完全一致。

方案通过现有的 TestRig.setup({ settings: { ... } }) API 在测试 settings 中注入确定性的 modelProviders 条目,然后按 ID (e2e-set-config-option-model) 定位注入的模型,而非子串匹配 openai。这消除了对 env 驱动的 runtime 模型枚举的依赖(这正是 CI 中 flake 的原因)。测试仍走真实的 session/set_config_option 模型往返,覆盖了原有意图。

无正确性 bug、无安全隐患、无 AGENTS.md 违规。注释块对测试改动来说稍长,但考虑到这是第三次修复这个 flake,上下文有助于后续读者理解。

测试

目标测试(不设 OPENAI_MODEL——证明修复不依赖环境变量):通过 ✓
兄弟测试(确认无回归):通过 ✓
全部 5 个非推理测试通过。6 个失败的测试需要真实 API key 做推理——与本 PR 无关。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

This is a clean, minimal test-only fix that does exactly what it says. The CI flake at acp-integration.test.ts:535 was caused by a fragile dependency on env-driven runtime-model enumeration — this PR eliminates that dependency by injecting a deterministic registry model via modelProviders, which is the right lever to pull.

The code review found no issues. The test results confirm the fix works: the target test passes both with and without OPENAI_MODEL, and all sibling tests remain green. The 6 inference-test failures are expected (fake API key) and unrelated.

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. ✅

中文说明

这是一个干净、最小化的仅测试修复,完全符合描述。acp-integration.test.ts:535 的 CI flake 由对 env 驱动的 runtime-model 枚举的脆弱依赖引起——本 PR 通过注入确定性的 registry 模型消除了该依赖,是正确的修复方向。

代码审查无问题。测试结果确认修复有效:目标测试在有/无 OPENAI_MODEL 时均通过,兄弟测试保持绿色。6 个推理测试失败属于预期(假 API key),与本 PR 无关。

PR 描述和内联注释详尽——考虑到这是第三次修复此 flake,这对后续读者很有帮助。

批准 ✅

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. ✅

@doudouOUC doudouOUC 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.

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.

@wenshao
wenshao merged commit fc9d01e into QwenLM:main Jun 23, 2026
32 of 33 checks passed

@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.

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

wenshao added a commit to wenshao/qwen-code that referenced this pull request Jun 23, 2026
…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.
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.
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.

3 participants