test(core): enable agent headless termination coverage - #5282
Conversation
|
@qwen-code /triage |
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ On direction: this is a clean test-coverage restoration — re-enables two skipped AgentHeadless termination tests (TIMEOUT and ERROR). Directly addresses issue #5290. Low-risk, well-scoped. Aligned with keeping CI coverage honest. On approach: minimal change — un-skips two tests, wraps the timeout test in Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 方向:纯粹的测试覆盖恢复——重新启用两个被 skip 的 AgentHeadless 终止模式测试(TIMEOUT 和 ERROR)。直接对应 issue #5290,低风险、范围清晰,有助于保持 CI 覆盖的可靠性。 方案:最小改动——取消两个测试的 skip,给超时测试包 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: un-skip both tests, wrap the timeout test's fake-timer usage in Code is clean. The No blockers. Test ResultsRan All 34 tests pass — including the two re-enabled TIMEOUT and ERROR termination tests. No regressions in the rest of the file. This is a test-only change with no user-visible behavior, so no before/after CLI scenario — the test suite itself is the evidence. 中文说明代码审查独立方案:取消两个测试的 skip,给超时测试的 fake-timer 使用包上 代码干净。 无阻塞问题。 测试结果在 PR 分支上从 34 个测试全部通过——包括重新启用的 TIMEOUT 和 ERROR 终止模式测试。文件内其余测试无回归。 这是纯测试改动,无用户可见行为变化,因此没有 CLI 前后对比——测试套件本身就是证据。 — Qwen Code · qwen3.7-max |
|
This is exactly the kind of small, focused PR that's easy to say yes to. The goal is clear (restore two skipped regression tests), the fix is minimal (un-skip + add My independent proposal matched the PR's approach exactly. There's no simpler path because this already is the simplest path: two The only minor observation: the ERROR test doesn't mock LGTM, approving. ✅ 中文说明这是一个非常干净、聚焦的 PR,容易通过。目标明确(恢复两个被 skip 的回归测试),改动最小(取消 skip + 添加 我的独立方案与 PR 做法完全一致。没有更简路径,因为这已经是最简路径:两个 唯一的次要观察:ERROR 测试没有做 LGTM,批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Local verification — PR #5282 (
|
| Test | Result |
|---|---|
should terminate with TIMEOUT … during an LLM call |
✓ 16 ms |
should terminate with ERROR if the model call throws |
✓ 13 ms |
3. Not flaky (the upstream skip-reason)
Ran the full file 10× consecutively → 34 passed every single time, 0 failures. The fake-timer advanceTimersByTimeAsync TIMEOUT test is deterministic.
4. Mutation testing — do the tests actually catch regressions?
I broke the runtime and confirmed the tests turn red (i.e. they are not vacuous):
| Mutation | Expected | Observed |
|---|---|---|
agent-core.ts:807 disable timeout check (>= maxTimeMinutes → >= 999999) |
TIMEOUT test fails | ✅ FAIL: expected 'GOAL' to be 'TIMEOUT' |
agent-headless.ts:287 catch sets MAX_TURNS instead of ERROR |
ERROR mode assertion fails | ✅ FAIL: expected 'MAX_TURNS' to be 'ERROR' |
So TIMEOUT and the getTerminateMode() === ERROR assertion are both load-bearing. ✔️
5. The try/finally cleanup is load-bearing (the actual fix)
There is no global timer reset anywhere: test-setup.ts has none, vitest.config.ts sets no restoreMocks/timer hooks, and the file's afterEach only calls vi.restoreAllMocks() — which does not restore fake timers. So if the un-skipped TIMEOUT test ever throws before vi.useRealTimers(), fake timers leak into every later test in the file.
I reproduced the exact layout difference with a 2-test demo (test A forced to fail, test B depends on a real setTimeout):
| Layout | Test A | Test B (unrelated) |
|---|---|---|
OLD (useRealTimers() at end) |
❌ fails | ❌ times out at 2000 ms — fake timers leaked, real setTimeout never fires |
NEW (this PR's try/finally) |
❌ fails | ✅ passes in 21 ms — timers restored, failure stays isolated |
This is the cascading cross-test contamination the PR prevents. The try/finally is the only thing guaranteeing isolation here. ✔️
6. Non-blocking nit (pre-existing; PR only un-skips, body unchanged)
The ERROR test's await expect(scope.execute(...)).rejects.toThrow('API Failure') passes for an incidental reason, not the intended one. Live trace + mutation:
- I made the re-throw at
agent-headless.ts:294swallow the error and separately throw a different marker — the test still passed both times. - Reason:
AgentEventType.ERROR === 'error', andAgentCorealways creates a default listener-lessAgentEventEmitter. Inside the catch,eventEmitter.emit(AgentEventType.ERROR, …)at line 289 hits Node's unhandled-'error'-event rule and throws before line 294:The wrapper's stringified payload contains the substring[[TRACE]] CATCH-285 reached, err=API Failure [[TRACE]] EMIT-289 THREW: name=Error msg=Unhandled error. ({ … error: 'API Failure' … })API Failure, sotoThrow('API Failure')matches by coincidence.
Net: the test verifies the ERROR terminate-mode and that execute() rejects, but it does not actually guard the intended re-throw at :294. A future swallow there would go undetected. Optional follow-up: attach a no-op 'error' listener in the test (so emit doesn't throw) and assert the rejection is the original API Failure error instance. Not required for this PR.
Side note (out of scope): the same mechanism means a headless run with the default emitter masks a real model error behind a confusing
Unhandled error.wrapper. Pre-existing product behavior, unrelated to this test-only PR.
7. Static checks & CI
npx eslint …agent-headless.test.ts→ exit 0 (the movedeslint-disablecomments are correctly placed).tsc --noEmit(packages/core) → exit 0.- CI: Lint ✓, CodeQL ✓, Test on macOS ✓ / Ubuntu ✓ / Windows ✓ (Node 22.x).
Verdict
LGTM / safe to merge. Restores 2 legitimate regression tests with a correct, load-bearing fake-timer cleanup; deterministic on Linux; both tests proven meaningful. The §6 item is a pre-existing, non-blocking test-quality nit and can be a separate follow-up.
🇨🇳 中文版本(点击展开)
✅ 本地验证 — PR #5282(test(core): enable agent headless termination coverage)
作为维护者,我在隔离的 tmux 会话里构建真实测试,在 Linux 上端到端验证了本 PR —— 也就是 PR 描述里标注「未本地测试」的平台。以下是供合并决策参考的证据。
环境: Linux x86-64 · Node v22.22.2 · Vitest 3.2.4 · 独立 tmux -L pr5282 socket · 分支 pull/5282/head @ a0b17f4f9
结论速览
PR 的行为与描述完全一致。两个被重新启用的测试稳定通过,并且经突变测试证明确实覆盖了真实的运行时终止逻辑。try/finally 的 fake-timer 清理是关键改动,并非可有可无。ESLint 与 tsc 均通过,三平台 CI 全绿。LGTM —— 可以合并。 关于 ERROR 测试断言强度有一个不阻塞、且本来就存在的小问题,记录在下方供后续跟进。
1. 改动范围与前提核对
- 单文件、纯测试:
agent-headless.test.ts(+41/−39)。 main上有 2 个it.skip;本 PR 为 0。即恢复了 2 个回归测试。- 这个
.skip是**上游 Gemini-CLI v0.8.2 同步(Sync upstream Gemini-CLI v0.8.2 #838)**引入的 —— 属于上游遗留的过期覆盖,并非 qwen 运行时缺失。运行时本就记录两种模式:TIMEOUT在agent-core.ts:807、ERROR在agent-headless.ts:287(默认值在:142)。这印证了 test(core): re-enable headless termination mode coverage #5290 的前提。✔️
2. 干净 PR 分支上测试通过
npx vitest run src/agents/runtime/agent-headless.test.ts
→ Test Files 1 passed (1) | Tests 34 passed (34) (0 skipped, 0 failed)
按名称单独运行的两个目标测试:
| 测试 | 结果 |
|---|---|
should terminate with TIMEOUT … during an LLM call |
✓ 16 ms |
should terminate with ERROR if the model call throws |
✓ 13 ms |
3. 不存在 flaky(上游当初 skip 的理由)
整文件连续跑 10 次 → 每次都是 34 passed,零失败。基于 fake timer 的 advanceTimersByTimeAsync 超时测试是确定性的。
4. 突变测试 —— 测试是否真能抓到回归?
我故意破坏运行时,确认测试会变红(即不是「空测试」):
| 突变 | 预期 | 实际 |
|---|---|---|
agent-core.ts:807 禁用超时判断(>= maxTimeMinutes → >= 999999) |
TIMEOUT 测试失败 | ✅ 失败:expected 'GOAL' to be 'TIMEOUT' |
agent-headless.ts:287 catch 里改成 MAX_TURNS 而非 ERROR |
ERROR 模式断言失败 | ✅ 失败:expected 'MAX_TURNS' to be 'ERROR' |
因此 TIMEOUT 与 getTerminateMode() === ERROR 这两处断言都是**有效(load-bearing)**的。✔️
5. try/finally 清理是关键改动(真正的修复点)
代码中没有任何全局 timer 重置:test-setup.ts 没有,vitest.config.ts 没设 restoreMocks/timer 钩子,文件里的 afterEach 只调用 vi.restoreAllMocks() —— 而它不会恢复 fake timer。所以一旦被启用的 TIMEOUT 测试在 vi.useRealTimers() 之前抛错,fake timer 就会泄漏到该文件后续所有测试。
我用一个 2 测试的 demo 复现了这正是 PR 改动的差异(A 强制失败,B 依赖真实 setTimeout):
| 写法 | 测试 A | 测试 B(无关) |
|---|---|---|
旧(useRealTimers() 放在末尾) |
❌ 失败 | ❌ 2000 ms 超时 —— fake timer 泄漏,真实 setTimeout 永不触发 |
新(本 PR 的 try/finally) |
❌ 失败 | ✅ 21 ms 通过 —— timer 已恢复,失败被隔离 |
这正是 PR 所防止的「跨测试级联污染」。此处 try/finally 是唯一能保证隔离的机制。✔️
6. 不阻塞的小问题(本就存在;PR 只是取消 skip,测试体未改)
ERROR 测试的 await expect(scope.execute(...)).rejects.toThrow('API Failure') 之所以通过,是巧合,并非其本意。实时 trace + 突变:
- 我把
agent-headless.ts:294的 re-throw 改成吞掉错误、又另外改成抛一个不同的 marker —— 两种情况下测试仍然通过。 - 原因:
AgentEventType.ERROR === 'error',而AgentCore总会创建一个没有监听器的默认AgentEventEmitter。catch 内 第 289 行 的eventEmitter.emit(AgentEventType.ERROR, …)命中 Node「未处理'error'事件」规则,在到达第 294 行之前就抛出:这个包装错误把 payload 字符串化后含有子串[[TRACE]] CATCH-285 reached, err=API Failure [[TRACE]] EMIT-289 THREW: name=Error msg=Unhandled error. ({ … error: 'API Failure' … })API Failure,所以toThrow('API Failure')只是恰好匹配上了。
结论:该测试验证了 ERROR 终止模式、以及 execute() 会 reject,但并未真正守护第 :294 行的 re-throw。将来若有人在那里吞掉错误,本测试不会发现。可选后续:在测试里挂一个空的 'error' 监听器(让 emit 不抛),并断言 reject 出来的就是原始的 API Failure 错误实例。本 PR 不必处理。
附注(超出本 PR 范围):同一机制意味着,若 headless 运行使用默认 emitter,真实的模型错误会被一个令人困惑的
Unhandled error.包装层掩盖。这是既有的产品行为,与这个纯测试 PR 无关。
7. 静态检查与 CI
npx eslint …agent-headless.test.ts→ exit 0(被移动的eslint-disable注释位置正确)。tsc --noEmit(packages/core)→ exit 0。- CI:Lint ✓、CodeQL ✓、macOS ✓ / Ubuntu ✓ / Windows ✓(Node 22.x)测试全过。
结论
LGTM / 可以合并。 以正确且关键的 fake-timer 清理恢复了 2 个合理的回归测试;在 Linux 上确定性通过;两个测试均被证明有效。第 6 节是既有的、不阻塞的测试质量小问题,可作为独立后续处理。
Verification done locally with real test runs (tmux), mutation testing, and live instrumentation traces. Runtime files were mutated only transiently and restored to pristine; the working tree is clean at a0b17f4f9.
What this PR does
Re-enables AgentHeadless termination coverage for timeout and model-call failure paths. The timeout test now restores real timers with try/finally so fake timers cannot leak into later tests.
Why it's needed
The runtime already records TIMEOUT and ERROR terminate modes for these paths, but both regression tests were still skipped. Restoring them makes the headless agent termination behavior visible to CI again.
Reviewer Test Plan
How to verify
Run the AgentHeadless test file and confirm the TIMEOUT and ERROR termination tests pass together with the rest of the file.
Evidence (Before & After)
N/A. This is test coverage only.
Tested on
Environment (optional)
Local npm workspace tests on macOS.
Risk & Scope
Linked Issues
Fixes #5290
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
这个 PR 做了什么
重新启用 AgentHeadless 在超时和模型调用失败路径上的终止模式测试。超时测试现在用 try/finally 恢复真实 timer,避免 fake timers 泄漏到后续测试。
为什么需要
运行时代码已经会在这些路径记录 TIMEOUT 和 ERROR terminate mode,但对应回归测试仍然是 skipped。恢复测试后,headless agent 的终止行为会重新进入 CI 覆盖。
Reviewer Test Plan
如何验证
运行 AgentHeadless 测试文件,确认 TIMEOUT 和 ERROR 两个终止模式测试以及整份测试文件都通过。
Evidence (Before & After)
N/A。这是测试覆盖改动。
Tested on
Environment (optional)
macOS 本地 npm workspace 测试。
Risk & Scope
Linked Issues
Fixes #5290