test(integration): wait for ACP command update notification - #10516
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
E2E verification
The merged change preserves the notification payload and |
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. Main E2E run 33259149948 failed this check on all three attempts with Direction: aligned. This is the fixed-wait race #10491 already tracks; the wait originated in #1113 and #9045 never touched this path, so the framing — runner load exposed an existing test race, not a product regression — checks out. Fixing CI flakiness at the synchronization point is the right direction. Size: not applicable — one integration-test file (+9/-2), no core paths touched. Approach: minimal and right-sized — the diff replaces exactly one fixed sleep with bounded condition polling and keeps every assertion. Nothing to cut. Risk: no elevated risk signals — test-only file, excluded from the high-risk path check. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到,不是理论问题。main E2E run 33259149948 中该检查连续三次失败( 方向:对齐。这正是 #10491 追踪的固定等待竞态;该等待源自 #1113,#9045 未触及这条路径,因此"共享 runner 负载暴露既有测试竞态,而非产品回归"的判断成立。在同步点修复 CI 抖动,方向正确。 规模:不适用——仅一个集成测试文件(+9/-2),未触及核心路径。 方案:最小且范围合理——diff 只把一处固定等待替换为有条件的有界轮询,所有断言保持不变。没有可砍的部分。 风险:无升级风险信号——纯测试文件,不在高风险路径检查范围内。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewNo blockers — re-verified against the base tree on this re-run, not carried over:
One CI fact worth knowing (not a defect of this PR): the required no-AK integration gate runs a fixed file list that does not include Test evidence — the PR's own CI via API (no PR code executed here)CI on
Everything inside the PR's actual blast radius is green, including the no-AK integration gate.
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Sandboxed verification would settle the one claim CI cannot: Real-scenario testing: N/A on the CI path — the change is integration-test synchronization only, nothing user-visible. 中文说明代码审查无阻塞问题——本次复跑重新对照基线代码核实,不是照搬上次结论:
一个值得了解的 CI 事实(不是本 PR 的缺陷):必需的 no-AK 集成门禁运行的是固定文件列表,其中不包含 测试证据——通过 API 读取 PR 自身检查(此处未执行任何 PR 代码)
PR 实际影响范围内的检查全部为绿,包括 no-AK 集成门禁。 CI 结果表见上方英文部分(与上次相同,检查结论已落定)。 沙箱验证可以补齐 CI 无法覆盖的那一点: 真实场景测试:CI 路径不适用——本改动仅涉及集成测试同步,无用户可见行为变化。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a minimal, structurally correct fix for a well-evidenced race; the one reservation stays evidential, not technical — no PR-level check runs the changed test, so the final proof lands post-merge. Stepping back: this is the kind of PR the gate should wave through. The problem is observed and measured — three consecutive failures, two stall readings above the one-second wait — and the root-cause story is internally consistent: the wait came from #1113, #9045 never touched this path. The fix is the smallest possible change: one sleep replaced by bounded polling on the exact condition the assertion already checks, reusing the rig's existing helper, assertions untouched. My independent proposal matches the diff; there is no simpler version of this fix, and in six months nobody will curse the author for it. What this re-run adds: CI has settled, and the one thing to judge was the red column. Judged — Maintainer @wenshao already approved this exact commit; 中文说明置信度:4/5——对一个证据充分的竞态,这是最小且结构上正确的修复;唯一的保留在证据层面而非技术层面——PR 级别的检查都不会运行被修改的测试,最终验证要到合并后才落地。 退一步看:这正是门禁应当放行的 PR。问题是观测到且有度量的——连续三次失败,两次停顿读数超过一秒等待——根因叙述也自洽:等待源自 #1113,#9045 未触及这条路径。修复是可能的最小改动:把一处固定等待替换为对断言已在检查条件本身的有界轮询,复用测试框架已有的助手函数,断言原封不动。我的独立方案与 diff 一致;这个修复不存在更简单的形态,半年后也不会有人抱怨它。 本次复跑的增量:CI 已落定,需要判断的就是那一列红色。判断结论—— 维护者 @wenshao 已批准同一提交; — Qwen Code · qwen3.8-max Reviewed at |
|
No blocking findings. The change correctly replaces the fixed 1-second delay with condition-based polling, preserves the existing command-payload assertions, and remains isolated to the single ACP test session. The reported build, bundle, formatting, and focused integration checks also pass. |
|
@qwen-code-review-bot approve this pr if no blockers |
|
I’m not approving yet because the required CI checks are still running. No code blockers are currently evident; approval can proceed once the checks finish successfully. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:反向审计——评审时间预算不足,未能开始第 1 轮。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 200 passed · 0 failed · 200 total Flakiness gate: not applicable — no runnable changed test files (1 out-of-scope file(s) noted in the log) 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:200 通过 · 0 失败 · 200 总计 抖动门:不适用 — no runnable changed test files (1 out-of-scope file(s) noted in the log) Verification reportPR #10516 — test(integration): wait for ACP command update notificationVerdict: 中文摘要
Central claim + A/BCentral claim: replacing the fixed How the A/B drives the real code:
Run A: 89/89 scripted assertions ( Witnesses: Mutation control (vacuity): in a scratch copy of the bundle, the single emission site's literal was renamed ( End-to-end suite A/B (real vitest file, Reviewer Test Plan walkthrough
Findings
Not covered
MethodologyEnvironment: CI merge-ref checkout ( Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅


What this PR does
This PR replaces the fixed one-second sleep in the ACP command-update integration check with condition-based polling. The check now stops as soon as
available_commands_updatearrives and keeps the existing assertions that validate the command payload.Why it's needed
Main E2E run 33259149948 failed this check on all three attempts. The first two attempts reported ACP event-loop stalls of 1415.58 ms and 1190.13 ms, both longer than the fixed one-second wait, so the assertion inspected the notification list before the asynchronously queued update was delivered.
The failed run was triggered by the main commit that merged #9045, but #9045 did not change this ACP path. The fixed wait originated in #1113 and has also produced the earlier failure tracked by #10491. Runner load exposed the existing test race; this is not evidence that #9045 introduced an ACP product regression.
Reviewer Test Plan
How to verify
Run the focused ACP integration check with an OpenAI-shaped local test configuration. Confirm that session creation is followed by an
available_commands_update, that the check returns as soon as the notification arrives, and that theinitcommand assertions still pass. Repeating the focused check should not fail when delivery takes longer than one second but remains within the five-second bound.Evidence (Before & After)
Before: the Linux
sandbox:noneshard failed three times withAssertionError: expected undefined to be definedat the command-update assertion. Two attempts logged event-loop stalls longer than the fixed wait.After:
npm run buildandnpm run bundlepassed. The focused ACP check passed four consecutive local runs.git diff --checkand the Prettier check also passed.UI verification: N/A — this PR changes only integration-test synchronization and does not affect user-visible behavior.
Tested on
Environment (optional)
Node.js 22.22.0,
QWEN_SANDBOX=false, and a non-routable local OpenAI-shaped endpoint for the authentication-only setup. The full Linux CLI integration suite runs only formerge_group; the no-AK gate on the PR does not include this ACP file, so the focused local run is the direct verification until merge-queue execution.Risk & Scope
Linked Issues
Fixes #10491
Fixes #10515
中文说明
本 PR 的改动
本 PR 将 ACP 命令更新集成检查中固定的一秒等待改为基于条件的轮询。收到
available_commands_update后检查会立即继续,同时保留现有断言来验证命令数据。为什么需要这个改动
main E2E run 33259149948 的该检查连续三次失败。前两次分别记录了 1415.58 毫秒和 1190.13 毫秒的 ACP 事件循环停顿,都超过了固定的一秒等待,因此异步排队的更新尚未送达时,断言就已经检查了通知列表。
失败 run 由合入 #9045 后的 main 提交触发,但 #9045 没有修改这条 ACP 路径。固定等待最早来自 #1113,之前 #10491 追踪的失败也属于同一问题。共享 runner 的负载暴露了既有的测试竞态,并不说明 #9045 引入了 ACP 产品回归。
Reviewer 测试计划
验证方式
使用 OpenAI 形态的本地测试配置运行聚焦的 ACP 集成检查。确认创建 session 后能够收到
available_commands_update,通知到达后检查立即继续,并且针对init命令的现有断言仍然通过。重复运行时,即使通知耗时超过一秒,只要在五秒上限内到达,检查也不应失败。前后证据
改动前:Linux
sandbox:none分片在命令更新断言处连续三次以AssertionError: expected undefined to be defined失败,其中两次事件循环停顿超过固定等待时间。改动后:
npm run build和npm run bundle通过;聚焦的 ACP 检查在本地连续四次通过;git diff --check和 Prettier 检查也通过。UI 验证:不适用——本 PR 只修改集成测试同步方式,不影响用户可见行为。
测试平台
环境(可选)
Node.js 22.22.0、
QWEN_SANDBOX=false,认证初始化使用不可路由的本地 OpenAI 形态端点。完整的 Linux CLI 集成测试仅在merge_group触发;PR 的 no-AK gate 不包含这个 ACP 文件,因此在 Merge Queue 执行前,聚焦的本地测试是这条路径的直接验证。风险与范围
关联 Issue
Fixes #10491
Fixes #10515