fix(integration): use QWEN_CODE_TEST_CRON_FAST in acp-cron test (#8237) - #8243
Merged
Conversation
The acp-cron E2E test relied on real minute-boundary cron timing, waiting up to 75s for the scheduler to fire. This made it flaky in CI where timing is unpredictable. The interactive cron test already uses the QWEN_CODE_TEST_CRON_FAST test seam to auto-fire after 5s; apply the same approach here and reduce the wait timeout to 30s.
Collaborator
Author
E2E Report — Issue #8237Root CauseThe The interactive cron test ( FixAdded Test ResultThe test now completes in ~6s instead of potentially 60–75s, and is deterministic with respect to cron timing. Verification
中文说明E2E 报告 — Issue #8237根本原因
交互式 cron 测试（ 修复在 测试结果测试现在在约 6 秒内完成，而不是可能的 60–75 秒，并且对 cron 定时具有确定性。 验证
🧠 Handled by Qwen Code · model/模型 |
Collaborator
|
|
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed CI flake — linked issue #8237 documents a real failure of Direction: aligned — test infrastructure fix that removes a known flakiness source, consistent with the approach already established in Size: not applicable — no core paths touched. Single file in Approach: the scope is exactly right. Three changes, all necessary: (1) add the Risk: no elevated risk signals — no high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 CI 不稳定——关联 issue #8237 记录了 方向：对齐——测试基础设施修复，消除已知的不稳定因素，与 规模：不适用——未触及核心路径。仅修改 方案：范围完全正确。三处改动，全部必要：(1) 为生成的 agent 添加 风险：无升级风险信号——未匹配高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code ReviewClean, minimal change. The diff does exactly what the interactive cron test already does — adds Verified the test seam in No correctness issues, no convention violations, nothing to cut. TestingFinal CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Unit suite on ubuntu is still running. Integration tests and macOS/Windows are skipped for this commit. No failures so far. This is a test-infrastructure change with no user-visible behavioral claim — the diff itself substantiates the fix (the test seam is well-established and the 中文说明代码审查干净、最小化的改动。diff 做了与交互式 cron 测试完全相同的事情——为生成的 agent 环境添加 已验证 无正确性问题，无规范违反，无需删减。 测试ubuntu 上的单元测试套件仍在运行。集成测试和 macOS/Windows 在此提交上被跳过。目前无失败。 这是一个测试基础设施变更，无用户可见的行为声明——diff 本身已证实了修复（测试接口已充分建立， — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 5/5 — textbook flake fix: observed failure, established pattern, minimal diff, nothing to second-guess. This is exactly the kind of PR that should be easy to review. The problem is real (CI failure documented in #8237), the fix mirrors what If I had to maintain this in six months, I'd thank the author. Nothing to curse. Approval deferred until CI lands green on 中文说明置信度：5/5 —— 教科书式的不稳定修复：已观测到的失败、已建立的模式、最小化的 diff，无需犹豫。 这个 PR 正是那种应该很容易审查的。问题是真实的（CI 失败记录在 #8237），修复方案与 如果六个月后我来维护这个，我会感谢作者。没有任何让人头疼的地方。 审批推迟到 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
github-actions
Bot
added
the
review/self-reported
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
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
Collaborator
Author
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
Collaborator
Author
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
Collaborator
Author
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix Review Round — No Action RequiredPR #8243 (issue #8237) — Feedback triage
No actionable feedback was raised this round. Failed check:
|
| 来源 | 数量 | 操作 |
|---|---|---|
| 自动审查建议 | 1 条（已丢弃——无法锚定到改动行） | 无 |
| 行内评论 | 0 | — |
| Issue 级评论 | 0 | — |
本轮未收到可操作的反馈。
失败检查：Test (ubuntu-latest Node 22.x)
本 PR 仅修改了一个文件——integration-tests/cli/acp-cron.test.ts——该文件不会被失败的 "Test" CI 作业执行。该作业运行单元测试（npm run test:ci，跨工作区）、代码检查、格式化和 schema 新鲜度检查；被修改的集成测试文件仅由单独的 "Integration Tests (CLI, No Sandbox)" 作业收集，而该作业在本次 CI 运行中被跳过。
在 PR 分支上的本地验证确认，"Test" 作业执行的所有检查均通过（见下文）。该失败与共享 main 基线上的不稳定单元测试或瞬态 CI 运行器环境问题一致，并非本 PR 引入的缺陷。
本地验证
npm run build— 通过npm run typecheck— 通过npm run lint— 通过npx prettier --check integration-tests/cli/acp-cron.test.ts— 通过npm run generate:settings-schema+ schema 新鲜度检查 — 通过（无差异）npx eslint integration-tests/cli/acp-cron.test.ts— 通过
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。
🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview
Collaborator
Author
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
Collaborator
Author
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action neededThis round had no actionable feedback on PR #8243 (issue #8237).
The PR diff is unchanged (a single focused edit to 中文说明Autofix 审查轮次 —— 无需处理本轮针对 PR #8243（issue #8237）没有可处理的反馈。
PR 的 diff 未发生变化（仅对 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
Collaborator
Author
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action taken (PR #8243, issue #8237)This round contained no actionable feedback, so no code change was made. What the feedback contained
Why no change is warrantedThe single review is an informational coverage disclosure, not a finding:
There is therefore nothing to implement, decline, defer, or escalate, and no failed or still-red check to address. Code-level sanity check of the existing change (read-only)For confidence, I re-verified the committed change against the codebase:
The change remains minimal, coherent, and consistent with project conventions. No modification is needed this round. 中文说明Autofix 审查轮次 — 未采取行动（PR #8243，issue #8237）本轮没有可操作的反馈，因此未做任何代码改动。 反馈包含的内容
为什么不需要改动这条唯一的审查是一条信息性的覆盖范围说明，并非一个发现（finding）：
因此没有任何需要实现、拒绝、延后或升级处理的内容，也没有任何失败或持续失败的检查需要处理。 对现有改动的代码级核查（只读）为增强信心，我对照代码库重新核查了已提交的改动：
该改动保持最小化、连贯，并符合项目约定。本轮无需任何修改。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Collaborator
Review —
|
| Variant | Run 1 | Run 2 | Run 3 | Run 4 | Run 5 |
|---|---|---|---|---|---|
| Pre-PR (minute boundary) | 13.2s | 61.3s | 58.3s | 39.6s | 59.5s |
This PR (QWEN_CODE_TEST_CRON_FAST=1) |
7.20s | 7.46s | 7.45s | — | — |
The premise holds: pre-PR wall time is uniformly distributed over the minute period (13s–61s observed), and a 61.3s Part 3a is uncomfortably close to the 75s budget inside a 120s test timeout. Post-PR it is flat at ~7.2–7.5s. That is a genuine win on the sharded E2E suite.
1. The linked failure was not a Part 3a timeout — it was Part 3b
The PR description and the E2E report both state the test "could time out waiting for the scheduler to fire" in the 75s Part 3a window. The actual failure recorded in #8237 (run 30635549911, job 91172225760, E2E Test - macOS - shard 1/2) says otherwise:
× acp cron integration > cron job fires and streams results via sessionUpdate after prompt returns 86245ms
→ Timed out waiting for sessionUpdate: agent_message_chunk after cron fire
(after 15000ms, saw 70 updates: [..., agent_message_chunk, user_message_chunk])
The update list ends with user_message_chunk — Part 3a succeeded, the cron fired and its user message was delivered. What blew up is Part 3b's 15s budget (acp-cron.test.ts:423), waiting for the model's response to the cron prompt. This PR does not touch that line.
Two consequences:
- The failing commit
63780e0d2predatese8d72673a("migrate acp-cron to fake-openai-server", test(integration): migrate acp-cron to fake-openai-server (#8076) #8082) —git merge-base --is-ancestor e8d72673a 63780e0d2is false, and the log is full ofagent_thought_chunk, i.e. a real thinking model. The 15s Part-3b budget was being spent on real-model latency on a loaded macOS runner. test(integration): migrate acp-cron to fake-openai-server (#8076) #8082 is the change that plausibly already fixed Main CI failed: E2E Tests — cli/acp-cron.test.ts > … > cron job fires and streams results via sessionUpdate after prompt returns #8237; the issue has recorded no recurrence since. - If Part 3b were still the weak spot, this PR would not help. (With the fake server it isn't — 15s is now enormous headroom; I saw Part 3b resolve immediately in every run.)
Suggested action: keep the change, but reword the "Why it's needed" section to what it actually is — the test spends up to 60s of wall clock waiting on a minute boundary for no coverage benefit; make it deterministic and ~10× faster — and downgrade Fixes #8237 to Refs #8237, since the recorded failure mode is elsewhere and already addressed.
2. Stale comment — the 75s reference survives the change (acp-cron.test.ts:378)
// Fail fast if the cron_create tool call was not served to the first
// user prompt. ... would shift dispatch and otherwise
// surface only as an opaque 75s timeout in Part 3.Part 3a is now 30s. Change 75s → 30s.
3. Nit — pin the delay alongside the flag
setupAcpCronTest spawns with ...process.env, so an ambient QWEN_CODE_TEST_CRON_DELAY_MS silently retunes the seam and makes the new "auto-fires ~5s after creation" comments wrong. I confirmed this end to end: running the PR test with QWEN_CODE_TEST_CRON_DELAY_MS=100 in the ambient environment changed the fire delay and the test finished in 2.6s. Consider adding it explicitly next to the flag:
QWEN_CODE_TEST_CRON_FAST: '1',
QWEN_CODE_TEST_CRON_DELAY_MS: '5000',Things I checked and cleared (no action needed)
- Does the seam actually apply here? Yes. The scripted
cron_createpasses nodurable,CronCreateTooldefaults it tofalse→CronScheduler.create()→ the!job.durablebranch atcronScheduler.ts:405. If the job were durable the seam would be inert and the 30s budget would fail ~50% of the time; it isn't. - Is
onFireinstalled before the 5s timer fires? Yes —forceFireJobsilently returnsfalsewhenonFireis null and the timer is not retried, which would be a silent regression to minute-boundary timing under a 30s budget. It can't happen on the ACP path:Session.startCronScheduler()runs atsession/new,enableDurable()setsdurableEnabled, sohasPendingWorkis already true andstart(onFire)runs before the first prompt. - Reverse race — can the fire land before
promptDoneAtand break thereceivedAt > promptDoneAtpredicate? Empirically no. Forcing the fire deep inside the create turn (QWEN_CODE_TEST_CRON_DELAY_MS=100, 3/3 runs) still passed in ~2.6s, because the cron queue drains after the turn completes. The 5s default has ~3–5× margin over the observed Part-1 tail even so. _meta.source === 'cron'is unaffected: thestart()callback derivessourcefromcronExpr, identically for tick-fired and force-fired jobs.- Coverage tradeoff is accurately described in the PR body.
forceFireJobbypassestick(), so after this PR no integration test exercises real minute-boundary scheduling (cron-interactive.test.tsalready uses the seam) — butpackages/core/src/services/cronScheduler.test.tshas a dedicateddescribe('tick')block covering boundary, jitter and expiry semantics, and this test's stated purpose (delivery of cron-fired turns over ACP after the originating prompt returned) is fully preserved. prettier --checkandeslintare clean on the changed file.
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 1/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/10 轮）。改动内容与我反驳保留之处如下： Review feedback — round summary1. PR description: "Fixes #8237" → "Refs #8237" and reword root causeDecision: Acknowledged — not a code change. The reviewer's analysis is correct: the recorded failure in #8237 (run 30635549911) was a Part 3b timeout (model response latency on a real thinking model), not a Part 3a timeout (cron fire timing). #8082 (fake-openai-server migration) is the change that plausibly already fixed #8237. This PR is a determinism/speed improvement — it removes up to 60s of wall-clock waiting on a minute boundary for no coverage benefit. The PR body framing ("Fixes" vs "Refs", root cause description) is managed by the workflow and cannot be updated from a code commit. Flagging here so the maintainer can adjust the PR description before merge. 2. Stale comment:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
Collaborator
Author
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
Collaborator
Re-review —
|
| Item | Status |
|---|---|
Stale 75s comment (acp-cron.test.ts:379) |
✅ now reads opaque 30s timeout in Part 3a — names the specific sub-part, which is better than what I asked for |
Pin QWEN_CODE_TEST_CRON_DELAY_MS next to the flag (:118) |
✅ QWEN_CODE_TEST_CRON_DELAY_MS: '5000' added; the ambient-override hole I reproduced with QWEN_CODE_TEST_CRON_DELAY_MS=100 is closed |
Fixes #8237 → Refs #8237 + reword root cause |
The delta from 782ed3d1b is exactly those two edits plus main merges; nothing else moved.
Suggestion — the 30s Part 3a budget removes the minute-boundary fallback
This is the one part of the diff that adds risk rather than removing it, and I don't think it buys anything.
A timeout only elapses on failure. The ~7s runtime comes entirely from the seam — I measured that in round 1 (7.20 / 7.46 / 7.45s), and the bot's own run at head is 6.21s. Leaving Part 3a at 75s would not have made a single passing run slower. So "reduced from 75s to 30s to match the new deterministic timing" describes a change with no upside on the green path.
What it does remove is the safety net. If the seam fire is ever lost, the job is not gone — it still fires off the normal tick:
create()stampslastFiredAt = now - (now % 60_000), so the creation minute is skipped → next*/1 * * * *boundary is ≤ 60s out.processJobfires atcandidateMs + job.jitterMs(cronScheduler.ts:1520), andcomputeJittergives a 1-minute recurring job up to 10% of the period → ≤ 6s.
So the fallback lands at ≤ ~66s after creation. 75s covered that with margin; 30s does not, and the test carries retry: 0. A lost seam fire used to be an invisible slow pass; it is now a red CI.
How could the seam fire be lost? forceFireJob returns false and drops the fire when onFire is null (cronScheduler.ts:1244) — unlike fireOrBuffer, it does not buffer into pendingFires. I cleared this path in round 1 and it still holds at head: acpAgent.ts:11217 calls session.startCronScheduler() at session/new, enableDurable() flips durableEnabled so hasPendingWork is already true, and start(onFire) runs before the first prompt. But that call is fire-and-forget (.catch at Session.ts:1804) and session/new does not await it, so the ordering is a property of timing on the runner, not an invariant the test enforces — which is precisely the class of assumption this PR exists to stop relying on.
Concretely: either keep Part 3a at ~70–75s, or keep 30s and say in the comment that the minute-boundary fallback is deliberately no longer covered so a dropped seam fire surfaces as a failure instead of a slow pass. I'd take the first — a defensive fallback that costs zero wall clock on green is worth keeping. Your call; I won't hold the PR on it.
Re-cleared at head (no action needed)
- Seam applies: scripted
cron_createpasses nodurable→CronCreateTooldefaultsfalse(cron-create.ts:79) →scheduler.create()→ the!job.durablebranch atcronScheduler.ts:405. Still true. - Pinning
5000is a no-op against the current default (Number(env) || 5000) — that is the point; it defends the~5scomments against both an ambient value and a future default change. - Reverse race (fire landing before
promptDoneAt, breaking thereceivedAt > promptDoneAtpredicate): still cleared — forcing the fire deep inside the create turn withQWEN_CODE_TEST_CRON_DELAY_MS=100passed 3/3 in round 1, because the cron queue drains after the turn completes. _meta.source === 'cron'is derived fromcronExprin thestart()callback, identically for tick-fired and force-fired jobs.- Coverage tradeoff is accurately described.
tick()boundary/jitter/expiry semantics stay covered bypackages/core/src/services/cronScheduler.test.ts. - Style: matches
cron-interactive.test.ts; comments explain the why and point at the precedent.
I did not re-run the suite for this round — the delta from the commit I ran in round 1 is one env var and one comment, and the bot reports vitest run cli/acp-cron green at 6.21s.
中文说明
复审 —— 94074d6dd（第 1 轮跟进）
结论：第 1 轮的两个代码项均已正确处理。仅剩一条建议（75s → 30s 的超时缩减），以及仍未处理的非代码项（PR 描述）。 无阻塞问题。
第 1 轮项目 —— 已在 head 验证
| 项目 | 状态 |
|---|---|
过期的 75s 注释（acp-cron.test.ts:379） |
✅ 现为 opaque 30s timeout in Part 3a，还额外指明了子部分，好于我原本的要求 |
在标志旁固定 QWEN_CODE_TEST_CRON_DELAY_MS（:118） |
✅ 已添加；我用 QWEN_CODE_TEST_CRON_DELAY_MS=100 复现的环境覆盖漏洞已堵上 |
Fixes #8237 → Refs #8237 并重写根因 |
相对 782ed3d1b 的增量正是这两处编辑加上 main 合并，无其他改动。
建议 —— 30s 的 Part 3a 预算移除了分钟边界兜底
这是 diff 中唯一增加风险而非降低风险的部分，而且我认为它没有带来收益。
超时只在失败时才会走满。约 7s 的运行时间完全来自测试接口——第 1 轮我实测为 7.20 / 7.46 / 7.45s，bot 在 head 上的运行是 6.21s。把 Part 3a 保持在 75s 不会让任何一次通过的运行变慢。所以"缩减到 30s 以匹配新的确定性定时"这个改动在绿色路径上没有任何好处。
它移除的是安全网。如果测试接口的触发丢失，任务并不会消失，仍会由正常 tick 触发：
create()将lastFiredAt标记为now - (now % 60_000)，跳过创建所在分钟 → 下一个*/1 * * * *边界 ≤ 60s。processJob在candidateMs + job.jitterMs触发（cronScheduler.ts:1520），而computeJitter对 1 分钟周期的循环任务给出至多 10% 周期的抖动 → ≤ 6s。
因此兜底触发落在创建后 ≤ 约 66s。75s 有余量覆盖，30s 覆盖不到，而该测试带 retry: 0。丢失一次接口触发，过去只是"慢一点但通过"，现在会直接红。
接口触发怎么会丢？当 onFire 为 null 时，forceFireJob 返回 false 并丢弃该次触发（cronScheduler.ts:1244）——与 fireOrBuffer 不同，它不会缓冲进 pendingFires。第 1 轮我已排除该路径，在 head 上依然成立：acpAgent.ts:11217 在 session/new 调用 session.startCronScheduler()，enableDurable() 置位 durableEnabled 使 hasPendingWork 已为真，start(onFire) 在首个 prompt 之前完成。但该调用是 fire-and-forget（Session.ts:1804 的 .catch），session/new 并不等待它，所以这个顺序是运行机上的时序属性，而非测试强制的不变量——而这恰恰是本 PR 想要摆脱的那一类假设。
具体建议：要么把 Part 3a 保持在约 70–75s；要么保留 30s，并在注释里写明分钟边界兜底已被有意放弃，以便丢失的接口触发暴露为失败而非慢通过。我倾向前者——一个在绿色路径上零成本的防御性兜底值得保留。由你定夺，我不会因此卡住 PR。
在 head 上重新确认（无需处理）
- 接口确实生效：脚本化的
cron_create未传durable→CronCreateTool默认false（cron-create.ts:79）→scheduler.create()→cronScheduler.ts:405的!job.durable分支。仍然成立。 - 固定
5000相对当前默认值是空操作（Number(env) || 5000）——这正是目的：同时防住环境中的既有值和未来默认值变更。 - 反向竞争（触发早于
promptDoneAt，破坏receivedAt > promptDoneAt判定）：仍已排除——第 1 轮用QWEN_CODE_TEST_CRON_DELAY_MS=100把触发压进创建回合内部，3/3 通过，因为 cron 队列在回合结束后才排空。 _meta.source === 'cron'由start()回调从cronExpr推导，tick 触发与强制触发路径一致。- 覆盖率取舍描述准确。
tick()的边界/抖动/过期语义仍由packages/core/src/services/cronScheduler.test.ts覆盖。 - 风格：与
cron-interactive.test.ts一致；注释解释了 why 并指向了先例。
本轮我未重跑测试套件——相对第 1 轮我实跑过的提交，增量只有一个环境变量和一条注释，且 bot 报告 vitest run cli/acp-cron 在 6.21s 通过。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
wenshao
previously approved these changes
Aug 1, 2026
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 2/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/10 轮）。改动内容与我反驳保留之处如下： Round summary — PR #8243 (issue #8237)Addressed the maintainer's round-1 follow-up suggestion. No conflict resolution was needed ( Feedback points1. Suggestion — restore the Part 3a fallback timeout (30s → 75s) — @wenshaoDecision: implemented (option 1, the maintainer's stated preference). The 30s ceiling bought nothing on the green path — a timeout only elapses on failure, and the run completes in ~6–7s from the Changes in
2. PR description:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
Collaborator
Author
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action requiredThe only feedback on this PR is an informational The change itself is verified to be consistent with the existing codebase:
No code changes are needed this round. 中文说明无需操作本 PR 唯一的反馈是自动审查机器人的一条 变更本身已验证与现有代码库一致：
本轮无需代码变更。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
approved these changes
Aug 1, 2026
wenshao
approved these changes
Aug 1, 2026
Collaborator
|
Released in v0.21.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
4 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
Enables the
QWEN_CODE_TEST_CRON_FASTtest seam in the ACP cron integration test so that cron jobs auto-fire after a 5-second delay instead of waiting for the real wall-clock minute boundary. This is the same approach already used by the interactive cron test (cron-interactive.test.ts). The Part 3a wait timeout is reduced from 75s to 30s to match the new deterministic timing.Why it's needed
The ACP cron E2E test was flaky on main CI because it depended on real minute-boundary cron scheduling. In CI, where process timing is unpredictable, the test could time out waiting for the scheduler to fire. The interactive cron test already eliminated this flakiness source by using the
QWEN_CODE_TEST_CRON_FASTenvironment variable, but the ACP test was never migrated.Reviewer Test Plan
How to verify
Run the ACP cron integration test and confirm it passes quickly and deterministically:
Expected: the test completes in ~6s (previously up to 75s) and passes consistently across multiple runs.
Evidence (Before & After)
N/A — test infrastructure change, no user-visible behavior.
Tested on
Environment (optional)
Integration test run via
vitest run cli/acp-cronwithQWEN_SANDBOX=falseon Linux CI runner.Risk & Scope
CronSchedulertick logic is covered by unit tests.Linked Issues
Fixes #8237
中文说明
本 PR 做了什么
在 ACP cron 集成测试中启用
QWEN_CODE_TEST_CRON_FAST测试接口，使 cron 任务在 5 秒延迟后自动触发，而不是等待真实的分钟边界。这与交互式 cron 测试（cron-interactive.test.ts）已使用的方案一致。Part 3a 的等待超时从 75 秒减少到 30 秒，以匹配新的确定性定时。为什么需要
ACP cron E2E 测试在 main CI 上不稳定，因为它依赖真实的分钟边界 cron 调度。在 CI 环境中，进程时间不可预测，测试可能因等待调度器触发而超时。交互式 cron 测试已经通过使用
QWEN_CODE_TEST_CRON_FAST环境变量消除了这一不稳定因素，但 ACP 测试从未迁移。审阅者测试计划
如何验证
运行 ACP cron 集成测试，确认其快速且确定性地通过：
预期：测试在约 6 秒内完成（之前最多 75 秒），并在多次运行中一致通过。
证据（修改前后）
N/A — 测试基础设施变更，无用户可见行为。
测试平台
环境（可选）
在 Linux CI 运行器上通过
vitest run cli/acp-cron和QWEN_SANDBOX=false运行集成测试。风险与范围
CronScheduler的 tick 逻辑由单元测试覆盖。关联 Issue
Fixes #8237