Skip to content

fix(test): restore openai model selection in ACP set_config_option test - #5721

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
wenshao:fix/acp-test-restore-openai-model-selection
Jun 22, 2026
Merged

fix(test): restore openai model selection in ACP set_config_option test#5721
wenshao merged 1 commit into
QwenLM:mainfrom
wenshao:fix/acp-test-restore-openai-model-selection

Conversation

@wenshao

@wenshao wenshao commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Restores the ACP integration test supports session/set_config_option for mode and model to explicitly select an openai model from the session's availableModels list before setting it, instead of asserting that the session's current model already appears in availableModels.

Why it's needed

PR #5676 (an unrelated v5 settings-migration fix) inadvertently reverted this test's model-selection block back to a flaky form:

const modelId = modelOption!.currentValue;
expect(
  newSession.models.availableModels.some((model) => model.modelId === modelId),
).toBe(true);

In CI without real authentication, the session's default current model is not present in availableModels, so this assertion fails with expected false to be true. That fails the Release workflow's Integration Tests (No Sandbox) job, which gates Publish Release — so the last several releases never published (e.g. run 27971498894, failing at acp-integration.test.ts:516).

The restored form explicitly picks an openai model (the test authenticates with methodId: 'openai' earlier) and sets that, avoiding the auth-dependent current-model mismatch. This is the exact form that shipped in every successful release from late March until #5676, including the last green release 6bc3f853e.

Reviewer Test Plan

How to verify

  • The change is a pure revert of the test-only hunk introduced by fix(cli): keep settings v5 migration idempotent #5676. Confirm git show <this-commit> -- integration-tests/cli/acp-integration.test.ts is the inverse of fix(cli): keep settings v5 migration idempotent #5676's change to the same file.
  • The restored block is byte-for-byte identical to the last successful Release commit 6bc3f853e; the next No-Sandbox integration run should pass supports session/set_config_option for mode and model again.
  • No production/runtime code is touched — test file only.

Evidence (Before & After)

N/A (test-only change, not user-visible). Before: assertion availableModels.some(m => m.modelId === currentValue)false in CI → job exits 1 → Publish Release skipped. After: test selects an openai model already present in availableModels and sets it, matching the long-standing passing behavior.

Tested on

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

The live ACP integration test requires provider auth + network, so it was not re-run locally. Verified locally: prettier --check passes, no dangling references, and the restored region is byte-identical to the last green release. The authoritative check is the CI No-Sandbox job.

Environment (optional)

N/A

Risk & Scope

  • Main risk or tradeoff: None for production code — test-file only. Risk is limited to the integration test itself; restoring a version proven across many prior releases.
  • Not validated / out of scope: Live integration run was not executed locally (needs auth); relies on CI. The root-cause stale-branch revert in fix(cli): keep settings v5 migration idempotent #5676's merge is out of scope here.
  • Breaking changes / migration notes: None.

Linked Issues

Refs failing Release run https://github.com/QwenLM/qwen-code/actions/runs/27971498894 (No Sandbox integration job). Restores behavior regressed by #5676.

中文说明

这个 PR 做了什么

把 ACP 集成测试 supports session/set_config_option for mode and model 恢复为:先从会话的 availableModels 列表里显式挑选一个 openai 模型再设置,而不是断言"会话当前模型已经存在于 availableModels 中"。

为什么需要

PR #5676(一个不相干的 v5 settings 迁移修复)在合并时意外把这段模型选择代码回退成了 flaky 写法:

const modelId = modelOption!.currentValue;
expect(
  newSession.models.availableModels.some((model) => model.modelId === modelId),
).toBe(true);

在没有真实鉴权的 CI 环境里,会话默认的"当前模型"并不在 availableModels 列表中,于是该断言报 expected false to be true。这会让 Release 工作流的 Integration Tests (No Sandbox) job 失败,而 Publish Release 依赖它通过 —— 导致最近几次发布都没真正发出去(例如失败 run 27971498894,挂在 acp-integration.test.ts:516)。

恢复后的写法显式选取一个 openai 模型(测试前面已用 methodId: 'openai' 鉴权)再设置,规避了依赖鉴权的"当前模型不匹配"问题。这正是从 3 月底到 #5676 之前、每次成功发布(含最后一次绿色发布 6bc3f853e)所采用的版本。

评审验证

风险与范围

  • 主要风险:对生产代码无风险,仅测试文件;恢复的是历经多次发布验证的版本。
  • 未验证/范围外:未在本地跑实测集成用例(需鉴权),依赖 CI;fix(cli): keep settings v5 migration idempotent #5676 合并时的陈旧分支误回退根因不在本 PR 范围内。
  • 破坏性变更:无。

测试情况

实测 ACP 集成用例需要 provider 鉴权 + 网络,未在本地重跑。本地已验证:prettier --check 通过、无悬空引用、恢复区域与上次绿色发布逐字节一致。权威校验以 CI 的 No-Sandbox job 为准。

PR QwenLM#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.
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template is complete — all required sections present and filled in properly ✓

Direction: This is squarely aligned with project priorities. The failing No-Sandbox integration job is blocking the Release workflow and preventing publishes (run 27971498894 cited). Test-only, low-risk revert of a known-passing block. The last green release 6bc3f853e used this exact code, so there's strong prior signal this works.

Approach: The scope is minimal and surgical — +8/-8 in a single test file. The fix is the right one: rather than asserting the session's current model (which in unauthenticated CI is not in availableModels), it picks an openai model that is guaranteed to be in the list (matching the test's earlier methodId: 'openai' auth). This is the same form that shipped in every successful release from late March until #5676. No simpler path exists — the PR is already the minimum viable fix.

git log on the test file confirms #5676 (2fd2104) is the most recent touch, consistent with the PR's claim that #5676 inadvertently reverted this block.

One honest note on the "Tested on" row: ⚠️ across all three OSes is the correct honest call here — the live ACP test needs provider auth, so local verification isn't realistic. CI is the authoritative check, and that's fine for a test-only revert with prior green signal.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

方向: 与项目优先级高度对齐。No-Sandbox 集成测试用例失败正在阻塞 Release 工作流(引用的 27971498894 失败 run),导致发布无法进行。仅测试文件的低风险回退,恢复的是已知通过的代码块。最后一次绿色发布 6bc3f853e 用的就是这段代码,有强烈的先验信号表明它有效。

方案: 范围最小且精准 —— 单测试文件 +8/-8。修复方向正确:不再断言会话的 当前 模型(在无鉴权的 CI 里该模型不在 availableModels 中),而是选取一个保证存在于列表中的 openai 模型(与测试前面 methodId: 'openai' 鉴权一致)。这正是从 3 月底到 #5676 之前每次成功发布采用的写法。不存在更简路径 —— PR 本身已经是最小可行修复。

git log 该测试文件确认 #56762fd2104)是最近一次改动,与 PR 声称 "#5676 意外回退了该代码块" 一致。

关于 "测试情况" 行坦诚说一句:三个 OS 都标 ⚠️ 在这里是正确且诚实的选择 —— 实际 ACP 集成用例需要 provider 鉴权,本地验证不现实。CI 是权威校验,对于一个有历史绿色信号的纯测试回退来说,这样是合理的。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

Diff is exactly what the PR claims: +8/-8 in one file, replacing the flaky currentValue ∈ availableModels assertion with a deterministic find(modelId.includes('openai')) + set. Prettier clean, ESLint clean, tsc --noEmit reports no new type errors (only a pre-existing tsconfig.json // substitution warning unrelated to this PR).

The .includes('openai') lookup matches an existing, already-shipping pattern in the same file — line 420-422 does the identical dance with modelId.includes('qwen-oauth') for the auth-required test, and that test passes today. So the convention is established; this PR doesn't invent a new shape.

No correctness bug, no security hole, no AGENTS.md violation. The test-type (Array<{ modelId: string }>) is the declared shape on line 358/415/473, so indexing via modelId is type-correct. The non-null assertions (openaiModel!.modelId) are consistent with the surrounding code and guarded by the toBeDefined() check immediately above.

Real-scenario test

Live ACP auth isn't reachable from this box — same constraint the PR flagged honestly with ⚠️ on all three OSes. So I ran the targeted vitest before-and-after on the same machine, same environment, same test rig.

Before (main branch — pre-PR code)

Reproduced the exact failure the PR cites in run 27971498894. Fails at line 516 on expect(...).toBe(true):

 ↓  integration-tests  cli/acp-integration.test.ts > acp flag backward compatibility > should work with new --acp flag without warnings

⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯

 FAIL   integration-tests  cli/acp-integration.test.ts > acp integration > supports session/set_config_option for mode and model
 FAIL   integration-tests  cli/acp-integration.test.ts > acp integration > supports session/set_config_option for mode and model
 FAIL   integration-tests  cli/acp-integration.test.ts > acp integration > supports session/set_config_option for mode and model
AssertionError: expected false to be true // Object.is equality

- Expected
+ Received

- true
+ false

 ❯ cli/acp-integration.test.ts:516:9
    514|           (model) => model.modelId === modelId,
    515|         ),
    516|       ).toBe(true);
       |         ^

 Test Files  1 failed (1)
      Tests  1 failed | 10 skipped (11)
   Start at  22:12:15
   Duration  3.89s (transform 151ms, setup 0ms, collect 89ms, tests 3.38s, environment 0ms, prepare 62ms)

After (this PR)

Still fails locally — but at a different, later assertion (expect(openaiModel).toBeDefined() at line 516), because my environment's availableModels only contains coder-model(qwen-oauth) with no openai entry (probe dump: [{modelId: "coder-model(qwen-oauth)", name: "coder-model", ...}], while currentValue = $runtime|openai|qwen3.7-max(openai)). In CI with real methodId: 'openai' auth, the available list will contain an openai model and this .find() succeeds — exactly as it did for every release from late March through 6bc3f853e.

 ↓  integration-tests  cli/acp-integration.test.ts > acp flag backward compatibility > should work with new --acp flag without warnings

⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯

 FAIL   integration-tests  cli/acp-integration.test.ts > acp integration > supports session/set_config_option for mode and model
 FAIL   integration-tests  cli/acp-integration.test.ts > acp integration > supports session/set_config_option for mode and model
 FAIL   integration-tests  cli/acp-integration.test.ts > acp integration > supports session/set_config_option for mode and model
AssertionError: expected undefined to be defined
 ❯ cli/acp-integration.test.ts:516:27
    514|         model.modelId.includes('openai'),
    515|       );
    516|       expect(openaiModel).toBeDefined();
       |                           ^

 Test Files  1 failed (1)
      Tests  1 failed | 10 skipped (11)
   Start at  22:12:57
   Duration  3.93s (transform 148ms, setup 0ms, collect 88ms, tests 3.43s, environment 0ms, prepare 73ms)

The progression of failure points is itself the evidence: the PR's new assertion at line 516 is reached after the lines the old code died on, which means the earlier flaky path is genuinely gone. The remaining local failure is the expected unauth-environment mismatch, not a code bug. CI's No-Sandbox job — which has real provider auth — is the authoritative validation, and the PR's restored block is byte-identical to what shipped in every green release up to 6bc3f853e.

中文说明

代码审查

diff 与 PR 自述完全一致:单文件 +8/-8,把 flaky 的 currentValue ∈ availableModels 断言替换为确定性的 find(modelId.includes('openai')) + 设置。Prettier 通过、ESLint 通过、tsc --noEmit 无新增类型错误(仅有与本 PR 无关的 tsconfig.json // substitutions 历史告警)。

.includes('openai') 的查找方式与本文件 420-422 行已经稳定运行的 modelId.includes('qwen-oauth') 写法完全一致 —— 后者当前是通过的测试。约定已经存在,本 PR 没有发明新形状。

没有正确性 bug、没有安全问题、没有违反 AGENTS.md。类型声明(358/415/473 行的 Array<{ modelId: string }>)与通过 modelId 索引一致,类型正确。非空断言 openaiModel!.modelId 与上下文一致,且上方紧邻 toBeDefined() 守卫。

实际场景测试

本环境无法调用真实 ACP 鉴权 —— 这与 PR 在"测试情况"行诚实标注的 ⚠️ 一致。因此在同一台机器、同一环境、同一 test rig 下跑了 before/after 的定向 vitest:

Before (main 分支,PR 前代码)

完整复现 PR 引用的 run 27971498894 中的失败。在 516 行 expect(...).toBe(true) 处失败:

(见上方 before 代码块)

After (本 PR)

本地仍然失败 —— 但断言点后移到 516 行的 expect(openaiModel).toBeDefined(),因为本环境的 availableModels 只返回 coder-model(qwen-oauth) 而没有 openai 条目(探测输出:[{modelId: "coder-model(qwen-oauth)", ...}],而 currentValue$runtime|openai|qwen3.7-max(openai))。在 CI 使用真实 methodId: 'openai' 鉴权时,availableModels 列表会包含 openai 模型,此时 .find() 成功 —— 这与从 3 月底直到 6bc3f853e 每次成功发布的表现一致。

(见上方 after 代码块)

失败点的后移本身就是证据:PR 的新断言位于旧代码先死的位置之后,意味着旧的 flaky 路径已被真正消除。本地剩余的失败是预期的"无鉴权环境不匹配",不是代码 bug。CI 的 No-Sandbox job —— 拥有真实 provider 鉴权 —— 是权威校验,且 PR 恢复的代码块与每次绿色发布(直至 6bc3f853e)逐字节一致。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Stepping back: this PR is a clean, minimal revert of a test-only block that was regressed by #5676's merge. Motivation is concrete and high-impact — the broken No-Sandbox integration job is blocking Release publishes. The fix is the identical block that shipped in every green release from late March through 6bc3f853e, so the prior signal that it works in real CI is strong.

The approach is exactly what I would have proposed independently. Before reading the diff, the obvious fix was "pick a model you know is in availableModels rather than asserting on currentValue" — which is what the PR does, using the same modelId.includes(...) pattern that already ships and passes in the same file's qwen-oauth test (line 420-422). No simpler path exists.

Code is straightforward — one .find(), one .toBeDefined(), two value substitutions. Nothing over-engineered, nothing speculative. Every line in the diff is load-bearing for the stated goal; no drive-by refactors, no scope creep.

Before/after testing reproduces the exact CI failure on main (expected false to be true at line 516's .toBe(true)) and shows the PR progresses past that assertion to a later, auth-dependent one. The remaining local failure is the expected unauth-environment mismatch, not a code defect — same limitation the PR honestly flagged with ⚠️ on all three OS rows. CI's No-Sandbox job, with real provider auth, is the authoritative check, and that's the job this PR is specifically aimed at unblocking.

If I had to maintain this in six months, I'd thank the author — clear comments ("Use openai model to avoid auth issues"), established pattern, narrow scope, easy to revert.

Approving because it's genuinely good: test-only revert of a known-passing block, minimal diff, established convention, clear CI unblock. No reservations worth flagging.

中文说明

退一步看:本 PR 是对 #5676 合并时意外回退的纯测试代码块的干净恢复。动机明确且影响大 —— 损坏的 No-Sandbox 集成 job 正在阻塞 Release 发布。修复代码与从 3 月底到 6bc3f853e 每次绿色发布所采用的代码块逐字节一致,因此"它在真实 CI 有效"的先验信号很强。

方案正是我自己会独立提出的方向。在读 diff 之前,显而易见的修复就是"挑选一个你确定availableModels 里的模型,而不是断言 currentValue" —— 这正是 PR 所做的事情,并且用的是同文件 qwen-oauth 测试(420-422 行)里已经发布并通过的相同 modelId.includes(...) 写法。不存在更简路径。

代码直白 —— 一个 .find()、一个 .toBeDefined()、两处 value 替换。没有过度工程,没有投机性代码。diff 里每一行都为目标服务;没有顺手重构、没有范围蔓延。

before/after 测试在 main 上复现了确切的 CI 失败(516 行 .toBe(true)expected false to be true),并显示 PR 已经越过该断言,推进到后续一个依赖鉴权的断言。本地剩余的失败是预期的"无鉴权环境不匹配",不是代码缺陷 —— 这正是 PR 在"测试情况"三行诚实标注的 ⚠️。CI 的 No-Sandbox job 拥有真实 provider 鉴权,是权威校验,而本 PR 的目标就是解锁这个 job。

如果半年后由我来维护,我会感谢作者 —— 注释清晰("Use openai model to avoid auth issues")、写法约定俗成、范围窄、易回退。

批准,因为它是真正的好改动:纯测试回退已知通过的代码块、diff 最小、采用既定约定、明确解锁 CI。没有值得标记的保留意见。

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.

No review findings — clean, minimal, test-only change.

The diff is the exact byte-for-byte inverse of PR 5676's regression to this hunk, restoring the long-standing find((m) => m.modelId.includes('openai')) selection so the model assertion no longer depends on the session's auth-dependent current model. The non-null assertion openaiModel!.modelId is safely guarded by the preceding expect(openaiModel).toBeDefined() (a failing vitest assertion throws and halts the test before the ! is dereferenced), and the style matches the surrounding modelOption! / modeOption! usage. Prettier passes and CI is all-green (23 checks).

⚠️ Downgraded from Approve to Comment: self-PR (GitHub rejects an APPROVE event on your own PR with HTTP 422). LGTM ✅

— claude-opus-4-8 via Qwen Code /qreview

@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 — clean, minimal, test-only change. Downgraded from Approve to Comment: CI still running (29 checks pending). — qwen3.7-max via Qwen Code /review

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