fix(test): route ACP cron fixtures by prompt - #10374
Merged
Merged
Conversation
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: this is an observed bug with concrete evidence, not theoretical hardening. Main E2E run 33150408634 (attempt 1) failed the Direction: aligned. This is a test-stability fix that removes a cross-test ordering coupling from the ACP cron fixture; it changes no production behavior. Well within scope. Size: test-only. 0 production-logic lines (the single changed file is Approach: the scope feels right and minimal. Keying the scripted Risk: no elevated risk signals — the changed file is a test, which the high-risk-path screen excludes. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个有具体证据的已观测 bug，而非理论性加固。main E2E run 33150408634（attempt 1）中 方向：对齐。这是一个测试稳定性修复，移除了 ACP cron 夹具对跨测试请求顺序的依赖，不改动任何生产行为，完全在范围内。 规模：仅测试。生产逻辑行数为 0（唯一改动文件是 方案：范围恰当且最小化。把脚本化的 风险：无升级风险信号——改动文件是测试，高风险路径筛查本就将其排除。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Code review My independent take before reading the diff was the same as what landed here: the only durable fix for an ordering assumption is to route the scripted response by request content instead of by a global counter, so the PR matches my baseline. Concretely, CI evidence (this commit's own checks) One structural point a reviewer should know: the suite this PR fixes does not run on PR CI. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the one claim CI cannot: This was an unattended CI run, so no tmux real-scenario capture was driven; live-behavior signal for this test comes from the lane above, not from a simulated session. 中文说明代码审查 我在看 diff 之前的独立想法与最终方案一致：要消除顺序假设，唯一稳妥的做法是按请求内容分发脚本化响应，而不是按全局序号。具体来看， CI 证据（本提交自身的检查） 有一个结构性事实需要审稿人知晓：本 PR 修复的这个测试并不会在 PR CI 中运行。运行 沙箱化验证可以补上 CI 无法覆盖的那一点： 本次为无人值守 CI 运行，因此没有驱动 tmux 真实场景抓取；该测试的实时行为信号来自上面给出的验证通道，而不是模拟会话。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, minimal test fix for a flake I confirmed in the CI log; the only reservation is that PR CI structurally can't execute this integration test, so behavior proof leans on the review plus the author's local run. Stepping back: the problem is real — I read the failing job log from run 33150408634 and request index 0 was unmistakably the The one honest reservation, and the reason this is 4 rather than 5: the suite being fixed does not run on PR CI ( Verdict: approve. CI for this commit is still running, so approval is deferred until it lands green on 中文说明置信度：4/5 —— 对一个我在 CI 日志里确认过的 flake，这是一个干净且最小的测试修复；唯一的保留意见是，PR CI 在结构上无法运行这个集成测试，因此行为层面的证明依赖于审查加上作者的本地运行。 退一步看：问题是真实的——我读取了 run 33150408634 的失败任务日志，第 0 号请求明确就是 唯一诚实的保留意见，也是给 4 分而非 5 分的原因：被修复的这个测试并不会在 PR CI 中运行（ 结论：批准。由于该提交的 CI 仍在运行，批准将推迟到其在 — Qwen Code · qwen3.8-max Reviewed at |
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. |
qwen-code-ci-bot
approved these changes
Aug 28, 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 — CI landed green after the review. ✅
yiliang114
commented
Aug 28, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed at 6e68840 — no findings. Verified against the head source: the Part 1 cron prompt carries the matcher string verbatim (Call cron_create with cron expression "*/1 * * * *" ..., acp-cron.test.ts:445), so the body-routed handler serves the tool call regardless of request arrival order; requiring the last message to be a user message plus the specificity of CRON_CREATE_INSTRUCTION means foreign requests from parallel suites cannot match. The rewritten fail-fast assertion (some request matches) is consistent with the new routing and keeps the original purpose — surfacing a missing cron prompt immediately instead of an opaque 75s timeout in Part 3a. Non-cron requests keep the ordinary fallback, matching the prior behavior for every other turn.
(Self-PR: GitHub blocks self-approval, so this is a comment review; merge approvals need to come from other maintainers.)
chiga0
approved these changes
Aug 28, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
No blocking findings. Approving.
Scope: integration-tests/cli/acp-cron.test.ts (1 file, test-only change; no production code touched).
Checked:
- Routing logic (
isCronCreateRequest):CRON_CREATE_INSTRUCTION = 'Call cron_create with cron expression'is a verbatim substring of the actual test prompt sent at line 437 of the test — routing fires correctly for the intended request. - Callback signature: Confirmed
startFakeOpenAIServerpasses{ body: JsonObject; requestIndex: number }to every handler. Destructuring{ body }in the new callback is valid;requestIndexis simply not needed anymore. - Assertion change: Old assertion checked
requests[0]?.body['messages']for'CRONFIRE7742'— order-dependent, exactly the defect being fixed. New assertion checksrequests.some(({ body }) => isCronCreateRequest(body))— removes the ordering assumption while sharing the same discriminator as the router. The fail-fast intent is preserved. CRON_CREATE_INSTRUCTIONconstant: Used in both the router and the assertion, so a prompt-text change breaks both consistently. No duplication concern.- Cross-check: No prior inline comments or
CHANGES_REQUESTEDreviews found on this PR.
CI:
Test (ubuntu-latest, Node 22.x): ✓ PASS (24m46s)Integration Tests (CLI, No Sandbox): SKIPPED — the changed file lives inintegration-tests/; no CI run covered it on this PR. Author tested locally (macOS, Node 22, focused Vitest run, 1/1 pass).Test (macos-latest, Node 22.x)andTest (windows-latest, Node 22.x): SKIPPEDweb-shell E2E Smoke (ubuntu-latest, Node 22.x): PENDING at review time
Not covered: platform-specific filesystem behaviour (no cross-platform test ran on this PR); the underlying SDK child-process teardown race noted in the PR description (out of scope per author and this PR).
Reviewed with AI assistance.
Collaborator
|
Released in v0.22.3. |
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
3 participants
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.
What this PR does
Routes the ACP cron fixture's scripted tool response by the latest prompt content instead of the fake server's global request number. Unrelated requests now receive the ordinary fallback response, while the cron creation prompt receives the required tool call regardless of arrival order.
Why it's needed
Main E2E run 33150408634 showed a request from a parallel SDK lifecycle test arriving at this fake server before the ACP cron prompt. That foreign request consumed index zero, received the cron tool response, and shifted every later response; the cron test then failed before creating its scheduled job. Prompt-based routing removes the cross-test ordering assumption without changing production behavior.
Regression origin
This failure was not introduced by #10077; that merge only triggered the main run where the flake surfaced. The brittle
requestIndex === 0dispatch and matching assertion were introduced by #8082 when the ACP cron test moved to the fake OpenAI server. The foreign SDK request seen in the log exposed that pre-existing ordering assumption.Reviewer Test Plan
How to verify
Confirm the ACP cron integration test creates the recurring job, remains responsive for the interactive prompt, and receives both the cron-sourced user notification and the later agent notification even when model request order is not assumed.
Evidence (Before & After)
Before: Linux sandbox:none shard 1/3 failed because request index zero contained the SDK lifecycle prompt
Hello worldinstead of the cron creation prompt. After: the focused ACP cron E2E passed 1/1 locally.Tested on
Environment (optional)
Node.js 22, sandbox disabled, focused Vitest integration run.
Risk & Scope
Linked Issues
Closes #10370
中文说明
本 PR 的修改
ACP cron 测试夹具不再按 fake server 的全局请求序号返回脚本化工具响应，而是根据最新一条 prompt 的内容进行分发。无关请求会收到普通兜底响应，cron 创建 prompt 无论以什么顺序到达，都会收到所需的工具调用。
修改原因
main E2E run 33150408634 显示，并行 SDK 生命周期测试的请求先于 ACP cron prompt 到达了这个 fake server。该外来请求占用了第零号索引并错误收到 cron 工具响应，导致后续响应全部错位；cron 测试因此在创建定时任务前失败。按 prompt 分发可以消除跨测试的顺序假设，同时不修改生产行为。
回归来源
该失败不是由 #10077 引入；该合并只触发了暴露此 flake 的 main run。ACP cron 测试迁移到 fake OpenAI server 时，#8082 引入了脆弱的
requestIndex === 0分发逻辑及对应断言。日志中的外来 SDK 请求暴露了这个既有的顺序假设。Reviewer 测试计划
验证方式
确认 ACP cron 集成测试能够创建循环任务、在交互 prompt 中保持响应，并依次收到 cron 来源的用户通知和后续 agent 通知，且不依赖模型请求的固定顺序。
修改前后证据
修改前：Linux sandbox:none shard 1/3 失败，因为第零号请求包含 SDK 生命周期测试的
Hello world，而不是 cron 创建 prompt。修改后：本地聚焦运行 ACP cron E2E，1/1 通过。测试环境
环境（可选）
Node.js 22，关闭 sandbox，聚焦 Vitest 集成测试。
风险与范围
关联 Issue
Closes #10370