Skip to content

fix(test): stop forwarding a cleaned-up PTY session into stdout (#11002) - #11007

Open
qwen-code-dev-bot wants to merge 1 commit into
mainfrom
autofix/issue-11002
Open

fix(test): stop forwarding a cleaned-up PTY session into stdout (#11002)#11007
qwen-code-dev-bot wants to merge 1 commit into
mainfrom
autofix/issue-11002

Conversation

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

What this PR does

The integration test harness now detaches an interactive terminal session's output forwarding as part of cleaning up, rather than only signalling the spawned CLI to exit. Each session the harness spawns is recorded together with the handle that lets its byte-forwarding listener be removed, and cleanup removes that listener before signalling the child. After cleanup returns, no byte from that session can reach the test worker's own standard output.

A witness test pins the new behaviour with a stand-in child that traps the hangup signal exactly the way the CLI does and keeps printing a unique marker, asserting both that the child is genuinely still alive once cleanup has returned and that none of its output reaches standard output afterwards.

Why it's needed

The E2E Interactive - OpenTUI renderer (bun) leg has been reddening main intermittently without naming a failing test, which leaves the main-CI detector nothing to dedupe on and files one issue per commit. Run 33834473606 is the latest such red, and the same commit passed the same leg in run 33836390526, whose step started 7.8 minutes after the failing one and took 301 seconds against the failing step's 306 — so the suite ran to completion and the failure is not a deterministic defect in the code under test.

The earlier fix for this class made cleanup kill the sessions a test never closed, but killing does not close the window. A signal-less kill through the pseudo-terminal library sends a hangup, and the CLI deliberately traps that into an asynchronous graceful shutdown, so the child is still running and still rendering after cleanup has returned. The harness was still forwarding every byte that child produced into the worker's standard output, because this leg enables verbose output keeping. Once the test runner tears the worker down, the reader end of that pipe is gone, so the next forwarded write raises a broken-pipe error that nothing listens for, and Node escalates it to an uncaught exception — the run exits non-zero having printed no test failure at all.

This is fatal on that leg alone. The integration configuration deliberately keeps unhandled errors fatal on GitHub-hosted Linux so local and nightly runs retain the signal, and that leg is the only Linux lane which is GitHub-hosted, since the shards moved to the self-hosted pool and macOS is exempt by platform. So the same race is swallowed everywhere else and only ever reddens this one job.

Whether the race is lost depends on whether the child happens to emit a byte in the narrow window between worker teardown and its own exit, which is exactly why the same commit can pass minutes later. Detaching the forwarding removes the dependency on winning that race: a byte that never reaches standard output cannot break the pipe, however long the child's graceful shutdown takes.

Reviewer Test Plan

How to verify

The behaviour under test is the harness itself, so it is verifiable without bun, without the OpenTUI renderer, and without any model access:

  1. Run the harness suite: cd integration-tests && npx vitest run test-helper.test.ts. Expect 8 passing, including the new case that a cleaned-up session stops forwarding output. It completes in well under a second of test time.
  2. Confirm the witness has teeth by deleting the single line in cleanup that disposes the forwarding handle, then re-running with npx vitest run test-helper.test.ts -t "detaches" --retry=0. Expect it to fail, reporting roughly 25 marker chunks still written to standard output in the 500ms after cleanup. Restore the line and the file goes green again.
  3. Confirm the precondition the witness asserts is real rather than decorative: the same probe with a child that does not trap the hangup signal dies within 500ms, which is why the test first asserts the child is still alive — without that, the forwarding assertion could pass vacuously on a child that had already exited.
  4. For the surrounding blast radius, the neighbouring harness specs are worth a run: cd integration-tests && npx vitest run test-helper.test.ts renderer-matrix.test.ts globalSetup.test.ts fake-openai-server.test.ts — 4 files, 33 passing.
  5. The end-to-end confirmation is that this leg stops producing red runs with no named test. That is intermittent by nature, so it can only be observed over subsequent main runs rather than on demand.

Nothing in the interactive suite reads accumulated session output after cleanup — in every interactive spec the cleanup call is the last statement of the teardown hook — so detaching the listener at that point cannot hide output an assertion still needs.

Evidence (Before & After)

N/A — no user-visible or TUI change. The observable difference is in CI: before, this leg could exit non-zero with every test passing and no FAIL line, which is what produced the per-commit issue; after, a session that outlives cleanup can no longer write into the worker's standard output.

The mutation probe is the concrete before/after, and step 2 of the test plan above is how to reproduce it: with the disposal removed the witness fails showing roughly 25 marker chunks forwarded after cleanup, and with it in place the witness passes showing zero.

Tested on

OS Status
🍏 macOS ⚠️ not tested
🪟 Windows ⚠️ not tested
🐧 Linux ✅ tested

Environment (optional)

GitHub-hosted Linux runner image, Node v22.23.2, npm 10.9.8. The exact failing leg could not be reproduced here: the OpenTUI renderer path requires bun, which is not installed on this runner, and the original job log is only readable with admin rights. The change is in renderer-independent harness code and the witness runs on the default ink/node path, which exercises the same spawn-then-cleanup code that leg uses. Run, job and annotation metadata from the public API was used to establish the failure signature instead of the log text.

Risk & Scope

  • Main risk or tradeoff: cleanup now stops accumulating a session's output as well as stopping to forward it. Any future test that expects a session to keep producing readable output after cleanup would see a frozen buffer. No current test does — every interactive spec calls cleanup last in its teardown hook — and the accumulated string already holds everything produced up to that point.
  • Not validated / out of scope: this leg has a second, independent source of red runs with no named test — the test runner's fixed 60-second worker-to-main RPC budget, which a stall under load can exceed and surface as an unhandled error. It is already exempted off-Linux and on the self-hosted pool, but this leg is GitHub-hosted Linux and so is still exposed while running up to four parallel forks of pseudo-terminal-heavy tests. Widening that exemption was deliberately not done: it would silence the unhandled-error signal for the whole integration suite, including the broken-pipe class fixed here, and the existing configuration comment records the narrowing as intentional. That tradeoff is a maintainer decision, and the leg may therefore stay intermittently red for that separate reason.
  • Breaking changes / migration notes: none. The change is confined to the integration test harness; no product code, CI workflow, or test configuration changed, and no test was deleted or weakened.

Linked Issues

Fixes #11002

Related, for context on the failure class rather than as a closing reference: #10969 introduced the cleanup-time kill this PR completes, #10112 and #10325 established the unhandled-error exemption policy that makes this leg the only exposed Linux lane, and #10085 moved the other Linux shards to the self-hosted pool.

中文说明

这个 PR 做了什么

集成测试脚手架现在会在清理阶段摘除某个交互式终端会话的输出转发,而不仅仅是向派生的 CLI 发送退出信号。脚手架派生的每个会话都会连同「可移除其字节转发监听器的句柄」一起被记录下来,清理时会先移除该监听器、再向子进程发信号。清理返回之后,该会话的任何字节都无法再到达测试 worker 自身的标准输出。

一个见证测试(witness test)用一个替身子进程钉住了这一新行为:该子进程像 CLI 一样捕获挂断信号(hangup),并持续打印一个唯一标记;测试同时断言「清理返回后子进程确实仍然存活」以及「此后它的输出不会到达标准输出」。

为什么需要它

E2E Interactive - OpenTUI renderer (bun) 这个检查项一直在间歇性地把 main 弄红,却不指出任何失败的测试,这使得 main-CI 检测器无从去重,只能按提交逐个建 issue。运行 33834473606 就是最近这样一次红灯,而同一个提交在运行 33836390526 中通过了同一个检查项;后者的步骤比失败的那次晚开始 7.8 分钟,耗时 301 秒对失败步骤的 306 秒 —— 说明测试套件是跑完的,失败并不是被测代码里的确定性缺陷。

针对这一类问题的先前修复让清理阶段去杀掉测试从未关闭的会话,但杀掉进程并没有关闭这个时间窗口。通过伪终端库发送的不带信号的 kill 会发出一个挂断信号,而 CLI 有意把它捕获为一次异步的优雅退出,因此子进程在清理已经返回之后仍在运行、仍在渲染。而脚手架仍在把该子进程产生的每个字节转发进 worker 的标准输出,因为这个检查项开启了输出保留(verbose output keeping)。一旦测试运行器拆除 worker,该管道的读端就消失了,于是下一次转发写入会抛出一个无人监听的 broken-pipe 错误,Node 随即将其升级为 uncaughtException —— 整个运行以非零退出,却没有打印任何测试失败。

这只在那一个检查项上是致命的。集成测试配置有意在 GitHub 托管的 Linux 上保持「未处理错误即致命」,以便本地运行和 nightly 运行保留该信号;而该检查项是唯一由 GitHub 托管的 Linux 通道,因为分片已迁往自托管机器池,macOS 则因平台而被豁免。所以同一个竞态在其他地方都被吞掉,只会把这一个 job 弄红。

是否输掉这个竞态,取决于子进程是否恰好在「worker 被拆除」与「它自己退出」之间的狭窄窗口里发出了一个字节 —— 这正是为什么同一个提交几分钟后就能通过。摘除转发消除了对「赢得该竞态」的依赖:一个从未到达标准输出的字节不可能打破管道,无论子进程的优雅退出耗时多久。

审阅者测试计划

如何验证

被测对象是脚手架本身,因此无需 bun、无需 OpenTUI 渲染器、也无需任何模型访问权限即可验证:

  1. 运行脚手架测试套件:cd integration-tests && npx vitest run test-helper.test.ts。预期 8 个通过,其中包含新增的「已清理的会话停止转发输出」用例。测试耗时远低于一秒。
  2. 通过删除清理逻辑中 dispose 转发句柄的那一行来确认该见证测试是有效的,然后用 npx vitest run test-helper.test.ts -t "detaches" --retry=0 重跑。预期它会失败,并报告在清理之后的 500 毫秒内仍有约 25 个标记数据块被写入标准输出。恢复该行后文件重新变绿。
  3. 确认见证测试所断言的前提是真实的而非装饰性的:同样的探针若换成一个捕获挂断信号的子进程,会在 500 毫秒内死亡 —— 这正是测试要先断言子进程仍存活的原因;否则转发断言可能在一个早已退出的子进程上空洞地通过。
  4. 就周边影响范围而言,邻近的脚手架规格测试也值得一跑:cd integration-tests && npx vitest run test-helper.test.ts renderer-matrix.test.ts globalSetup.test.ts fake-openai-server.test.ts —— 4 个文件,33 个通过。
  5. 端到端的确认是:该检查项不再产生「没有指出测试名的红灯运行」。这本质上是间歇性的,因此只能在后续的 main 运行中观察,无法按需触发。

交互式套件中没有任何测试会在清理之后读取累积的会话输出 —— 在每个交互式规格文件中,清理调用都是 teardown 钩子的最后一条语句 —— 所以在该时点摘除监听器不会隐藏任何断言仍需要的输出。

证据(改动前与改动后)

N/A —— 没有用户可见或 TUI 的变化。可观察的差异体现在 CI 上:改动前,该检查项可能在所有测试都通过、且没有任何 FAIL 行的情况下以非零退出,这正是产生按提交归档 issue 的原因;改动后,一个比清理活得更久的会话已无法再写入 worker 的标准输出。

变异探针(mutation probe)就是具体的前后对比,上方测试计划的第 2 步即其复现方式:移除 disposal 后见证测试失败并显示清理之后仍转发了约 25 个标记数据块;保留它则见证测试通过并显示为零。

测试环境

OS Status
🍏 macOS ⚠️ 未测试
🪟 Windows ⚠️ 未测试
🐧 Linux ✅ 已测试

环境(可选)

GitHub 托管的 Linux runner 镜像,Node v22.23.2,npm 10.9.8。此处无法复现完全一致的失败检查项:OpenTUI 渲染器路径需要 bun,而本 runner 上没有安装;原始 job 日志也只有管理员权限才能读取。改动位于与渲染器无关的脚手架代码中,且见证测试运行在默认的 ink/node 路径上,执行的正是该检查项所使用的同一套「派生—清理」代码。已改用公开 API 的运行、job 与注解元数据来确立失败特征,以替代日志正文。

风险与范围

  • 主要风险或取舍:清理现在不仅停止转发,也会停止累积会话输出。任何未来期望会话在清理之后仍继续产生可读输出的测试,都会看到缓冲区被冻结。目前没有测试这样做 —— 每个交互式规格文件都在其 teardown 钩子的最后调用清理 —— 而且累积字符串中已经保留了截至该时点产生的全部内容。
  • 未验证 / 范围之外:该检查项还有第二个、彼此独立的「无测试名红灯」来源 —— 测试运行器固定的 60 秒 worker→main RPC 预算,负载下的停顿可能超出它并以未处理错误的形式浮现。它在非 Linux 平台和自托管机器池上已被豁免,但本检查项是 GitHub 托管的 Linux,因此仍然暴露在外,同时还会以最多四个并行 fork 运行伪终端密集型测试。我们刻意没有扩大该豁免:那会让整个集成测试套件的未处理错误信号被静默掉,其中就包括此处修复的 broken-pipe 一类;而且现有配置注释已把这种收窄记录为有意为之。这个取舍属于维护者决定,因此该检查项仍可能因这个独立原因保持间歇性红灯。
  • 破坏性变更 / 迁移说明:无。改动仅限于集成测试脚手架;没有产品代码、CI workflow 或测试配置发生变化,也没有删除或弱化任何测试。

关联 Issue

Fixes #11002

相关(用于说明该类失败问题的背景,而非作为关闭引用):#10969 引入了本 PR 所完善的「清理时杀进程」;#10112#10325 确立了未处理错误的豁免策略,正是该策略使本检查项成为唯一暴露的 Linux 通道;#10085 把其他 Linux 分片迁往了自托管机器池。

The `E2E Interactive - OpenTUI renderer (bun)` leg reddened run 33834473606
without printing a single `FAIL` line, so the main-CI detector had no test to
dedupe on and filed it per commit. The same SHA passed the same leg nine
minutes later (run 33836390526, its step 301s against the failing step's
306s), which rules out a deterministic defect at HEAD and matches the class
#10969 described: every test passes, then an unhandled error exits the run
red. It is fatal on this leg alone because `dangerouslyIgnoreUnhandledErrors`
is off for github-hosted Linux, and the OpenTUI job is the only Linux lane
that is github-hosted — the shards moved to the self-hosted pool (#10085) and
macOS is exempt by platform.

#10969 closed one source of that class by killing the interactive children
`cleanup()` inherits, but killing does not close the window. node-pty's
signal-less `kill()` sends SIGHUP, and the CLI traps SIGHUP into an
asynchronous graceful shutdown (`beginExit` -> `runExitCleanup()` -> `exit`),
so the child keeps rendering after `cleanup()` returns while the rig's
`onData` handler keeps forwarding every byte into `process.stdout` under this
leg's `KEEP_OUTPUT`/`VERBOSE`. One such write after vitest destroys the
worker's stdout pipe raises EPIPE, and with no listener Node escalates it to
an uncaught exception.

Detach the forwarding listener in `cleanup()` rather than relying on the child
to die first: a byte that never reaches `process.stdout` cannot EPIPE it,
however long the shutdown takes. Measured — a stand-in child that swallows
SIGHUP survives `kill()` for over a second, while a child that does not dies
within 500ms; and `dispose()` on node-pty's `onData` removes only that
listener, leaving the child and the master descriptor alone.

The witness pins both halves. It asserts the child is still alive after
`cleanup()`, so the forwarding check cannot pass vacuously on a child that
died instantly, and then that no canary byte reaches stdout. Removing the
`dispose()` call reddens it with roughly 25 canary writes in the 500ms window.

Out of scope: this leg's other no-`FAIL`-line source, vitest's 60s
worker-to-main `onTaskUpdate` RPC timeout under load, is exempted for
self-hosted and off-Linux lanes but not for this github-hosted one. Widening
that exemption would silence the very signal this fix preserves, so it is left
to a maintainer.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator Author

Autofix E2E Report — Issue #11002

Issue: Main CI failed: E2E Tests on 56f75adf2992
Failing leg: E2E Interactive - OpenTUI renderer (bun) — step Run interactive E2E tests (OpenTUI)
Run: 33834473606 on commit 56f75adf29926783855a890e46b6cf3a32e6c986
Fix commit: d1e250dd42 on branch autofix/issue-11002

What failed

The leg exited non-zero without printing a single FAIL line. That is evidence rather than assumption: the issue was filed through the detector's per-commit path, which renderPerCommitBody only reaches when analyzeLogs extracted zero failing tests from the downloaded job log. A real assertion failure or a test timeout would have produced a FAIL <file> > <suite> > <case> line, which extractFailingTests matches, and that would have deduped this onto an existing test-keyed issue instead of opening a per-commit one.

Evidence gathered

The job log text itself is not retrievable from here — GET /actions/jobs/<id>/logs returns 403 Must have admin rights to Repository, and this agent holds no GitHub credentials by design. The public read-only run metadata was available, and the diagnosis rests on it:

Observation Source Why it matters
Step conclusion failure; sole annotation Process completed with exit code 1. check-run annotations API vitest exited 1 and no test was named
Failing step ran 306s (04:20:40 → 04:25:46 UTC) jobs API Comparable to a healthy run, so the suite ran to completion instead of hanging; also rules out the job's 60-minute timeout
The same SHA passed the same leg in run 33836390526, step 301s (04:28:28 → 04:33:29 UTC) jobs API Rules out a deterministic defect at HEAD — the failure is intermittent
Every other job in run 33834473606 succeeded jobs API Isolated to this leg

The same-SHA green run is the decisive fact, and the two legs' steps started 7.8 minutes apart. It excludes both candidate signatures that a code change at HEAD would have had to explain: the /quit mid-turn composer race that commit 56f75adf29 had itself just fixed, and the "30s boot/first-delta timeout on a loaded runner" that its commit message explicitly left out of scope. Either would have failed deterministically, and either would have printed a FAIL line.

What remains is exactly the class 60161cb64a (#10969) documented for this leg: every test passes, then the run dies on what vitest reports as an unhandled error rather than a failure. That is fatal on this leg alone, because integration-tests/vitest.config.ts sets dangerouslyIgnoreUnhandledErrors: process.platform !== 'linux' || isSelfHostedRunner, and the OpenTUI job is the only Linux lane that is GitHub-hosted — the shards moved to the self-hosted pool (#10085) and macOS is exempt by platform.

Root cause

60161cb64a closed one source of that class by making TestRig.cleanup() kill the interactive PTY children a test never closed. Killing does not actually close the window:

  1. node-pty's signal-less kill() is process.kill(this.pid, signal || 'SIGHUP') — verified at node_modules/@lydell/node-pty-linux-x64/lib/unixTerminal.js:228.
  2. The CLI traps SIGHUP: packages/cli/src/llm.tsx registers handleSighupbeginExit('SIGHUP')await runExitCleanup()process.exit(129). That is an asynchronous graceful shutdown, so the child keeps running and keeps rendering the TUI after cleanup() has already returned.
  3. cleanup() neither awaited that exit nor stopped the rig's onData handler, which forwards every PTY byte into process.stdout under this leg's KEEP_OUTPUT=true / VERBOSE=true.
  4. Once vitest tears the worker down the reader end of that pipe is gone, so the next forwarded write raises EPIPE as an 'error' event on process.stdout. With no listener, Node escalates it to an uncaught exception and the run exits 1 having printed no FAIL line.

Whether the race is lost depends on whether the child happens to emit a byte in the narrow window between worker teardown and its own exit — which is why the same SHA passes when the leg runs again.

Two probes confirmed the mechanism instead of assuming it:

  • A standalone node-pty probe: a child that swallows SIGHUP survives kill() for over a second and keeps producing bytes, while a child that does not trap it dies within 500ms. Calling dispose() on the onData subscription removed only that listener — the child stayed alive and the parent stopped receiving its bytes.
  • The harness's other PTY driver was ruled out: InteractiveSession is used only by cron-interactive.test.ts, which this leg excludes, and its onData never writes to process.stdout. ctrl-c-exit.test.ts, the one interactive spec with no cleanup call, is entirely it.skip.

The fix

TestRig now pairs each spawned PTY with the IDisposable returned by ptyProcess.onData, and cleanup() disposes that listener before killing. After cleanup() no PTY byte can reach process.stdout, so the EPIPE mechanism is impossible however long the child's graceful shutdown takes — the fix no longer depends on winning a race against the child's exit.

Two files changed, 80 insertions and 6 deletions, both inside the integration harness. No product code, no CI workflow, no vitest configuration, and no test was deleted or weakened.

Witness test

detaches a session's output forwarding during cleanup in integration-tests/test-helper.test.ts spawns a stand-in child that traps SIGHUP exactly like the real CLI and prints a unique canary every 20ms. It asserts both halves of the claim:

  • the child is still alive after cleanup() — without this, the forwarding check could pass vacuously on a child that died instantly;
  • no canary byte reaches process.stdout in the 500ms after cleanup().

Out of scope

This leg has a second, distinct source of no-FAIL-line reds: vitest's worker→main onTaskUpdate RPC has a fixed 60s budget, and under resource pressure a longer stall surfaces as an unhandled [vitest-worker]: Timeout calling "onTaskUpdate" error. That is precisely what 2bd0ff923e (#10112) exempted off-Linux and what the later isSelfHostedRunner clause exempted for the self-hosted pool — but this leg is GitHub-hosted Linux, so it is still exposed, and it runs up to 4 forks of PTY-heavy tests.

That was deliberately not changed. Widening dangerouslyIgnoreUnhandledErrors to GitHub-hosted Linux would silence the unhandled-error signal for the whole integration suite, including the very EPIPE class this PR fixes, and the existing config comment records the narrowing as a deliberate decision. That tradeoff belongs to a maintainer, not to this fix. It is noted here so the leg's residual intermittency stays visible rather than being assumed away.

Verification

Commands actually run in this checkout (GitHub-hosted runner image, Node v22.23.2, npm 10.9.8):

  • npm run buildpassed (exit 0, ~509s)
  • npm run typecheckpassed (exit 0); includes typecheck:integration (tsc -p integration-tests/tsconfig.json), which covers both changed files
  • npm run lintpassed (exit 0); includes eslint integration-tests
  • npx prettier --check integration-tests/test-helper.ts integration-tests/test-helper.test.tspassed ("All matched files use Prettier code style!")
  • cd integration-tests && npx vitest run test-helper.test.ts8 passed, including the new witness (~0.7s of test time)
  • cd integration-tests && npx vitest run test-helper.test.ts renderer-matrix.test.ts globalSetup.test.ts fake-openai-server.test.ts4 files, 33 passed
  • Mutation probe: removed session.forwardOutput.dispose(); from cleanup(), then npx vitest run test-helper.test.ts -t "detaches" --retry=0failed as required, reporting ~25 canary chunks still written to process.stdout in the 500ms after cleanup(). Restored the line and re-ran the full file — 8 passed. The witness therefore has teeth.
  • Pre-commit hook (node scripts/pre-commit.js / lint-staged) — passed; the committed content was re-verified after the hook ran.
  • npm run generate:settings-schemanot run, correctly: no settings source changed.

Checks that could not be run here

  • The exact failing leg. QWEN_E2E_RENDERER=opentui requires bun — resolveE2eCliCommand probes bun --version and throws otherwise — and bun 1.3.14 is not installed on this runner; installing it is a networked package operation outside this skill's allowed commands. The fix is in renderer-independent harness code, and the witness runs on the default ink/node path, so it exercises the same spawn-then-cleanup code the OpenTUI leg uses.
  • The original job log. Requires admin rights (403); the public run, job and annotation metadata tabulated above was used instead.
  • The interactive suite's model-backed cases were not run end-to-end here. The workflow's independent CI remains the final verification gate.

One correction to the commit message

The commit message says the same SHA passed the same leg "nine minutes later". The measured gap between the two legs' step starts is 7.8 minutes (04:20:40 vs 04:28:28 UTC); the two runs were created 32.3 minutes apart. The substantive claim is unaffected — the same SHA passed the same leg shortly afterwards — but the figure is rounded up. This report states the precise timestamps. The branch history is additive-only under this skill's rules, so the message was not amended to correct it.

中文说明

Autofix E2E 报告 — Issue #11002

Issue: Main CI failed: E2E Tests on 56f75adf2992
失败的检查项: E2E Interactive - OpenTUI renderer (bun) —— 步骤 Run interactive E2E tests (OpenTUI)
运行记录: 33834473606,提交 56f75adf29926783855a890e46b6cf3a32e6c986
修复提交: 分支 autofix/issue-11002 上的 d1e250dd42

失败的是什么

该检查项以非零状态退出,但没有打印任何一行 FAIL。这是证据而非假设:该 issue 是通过检测器的按提交(per-commit)路径创建的,而 renderPerCommitBody 只有在 analyzeLogs 从已下载的 job 日志中提取到零个失败测试时才会走到这条路径。真正的断言失败或测试超时都会产生一行 FAIL <file> > <suite> > <case>extractFailingTests 能匹配到它,那样就会把本次失败去重合并到已有的、按测试名归档的 issue 上,而不是新建一个按提交归档的 issue。

已收集的证据

job 日志正文在此处无法获取 —— GET /actions/jobs/<id>/logs 返回 403 Must have admin rights to Repository,而本 agent 按设计不持有任何 GitHub 凭据。但公开的只读运行元数据可以获取,本次诊断正是建立在这些数据之上:

观察结果 来源 为什么重要
步骤结论为 failure;唯一的注解是 Process completed with exit code 1. check-run annotations API vitest 以 1 退出,且没有指出任何测试名
失败步骤运行了 306 秒(04:20:40 → 04:25:46 UTC) jobs API 与一次健康运行的时长相当,说明测试套件是跑完的而不是卡住了;同时也排除了该 job 的 60 分钟超时
同一个 SHA 在运行 33836390526 中通过了同一个检查项,步骤耗时 301 秒(04:28:28 → 04:33:29 UTC) jobs API 排除了 HEAD 上存在确定性缺陷的可能 —— 该失败是间歇性的
运行 33834473606 中其他所有 job 都成功 jobs API 问题局限于这一个检查项

「同一 SHA 变绿」是决定性的事实,而且两次检查项的步骤开始时间相差 7.8 分钟。它排除了两个本来必须由 HEAD 上的代码改动来解释的候选特征:一是提交 56f75adf29 自己刚刚修掉的 /quit 回合中(mid-turn)输入框竞态,二是该提交信息中明确列为范围之外的「在负载较高的 runner 上 30 秒启动/首个 delta 超时」。这两者要么会确定性失败,要么会打印出 FAIL 行。

剩下的恰好就是 60161cb64a#10969)为该检查项记录过的那一类:所有测试都通过,然后整个运行死于 vitest 报告为 unhandled error(未处理错误)而非失败的东西。这一类只在这个检查项上是致命的,因为 integration-tests/vitest.config.ts 设置的是 dangerouslyIgnoreUnhandledErrors: process.platform !== 'linux' || isSelfHostedRunner,而 OpenTUI 这个 job 是唯一一个由 GitHub 托管的 Linux 通道 —— 分片已经迁到自托管机器池(#10085),macOS 则因平台而被豁免。

根因

60161cb64a 通过让 TestRig.cleanup() 杀掉测试自己从未关闭的交互式 PTY 子进程,堵住了该类问题的一个来源。但仅仅杀掉进程并没有真正关闭这个时间窗口:

  1. node-pty 不带信号的 kill() 实际是 process.kill(this.pid, signal || 'SIGHUP') —— 已在 node_modules/@lydell/node-pty-linux-x64/lib/unixTerminal.js:228 核实。
  2. CLI 捕获了 SIGHUP:packages/cli/src/llm.tsx 注册了 handleSighupbeginExit('SIGHUP')await runExitCleanup()process.exit(129)。这是一个异步的优雅退出,因此子进程在 cleanup() 已经返回之后仍在运行,并仍在渲染 TUI。
  3. cleanup() 既没有等待子进程退出,也没有停止 rig 的 onData 处理器;而在本检查项的 KEEP_OUTPUT=true / VERBOSE=true 下,该处理器会把每一个 PTY 字节转发进 process.stdout
  4. 一旦 vitest 拆除 worker,该管道的读端就消失了,于是下一次转发写入会以 'error' 事件的形式在 process.stdout 上抛出 EPIPE。由于没有监听者,Node 会把它升级为 uncaughtException,运行随即以 1 退出,且没有打印任何 FAIL 行。

是否会输掉这个竞态,取决于子进程是否恰好在「worker 被拆除」与「它自己退出」之间的那个狭窄窗口里发出了一个字节 —— 这也正是为什么该检查项再次运行时,同一个 SHA 就能通过。

有两个探针证实了该机制,而不是靠假设:

  • 一个独立的 node-pty 探针:吞掉 SIGHUP 的子进程kill() 之后仍存活超过一秒并持续产生字节,而不捕获该信号的子进程会在 500 毫秒内死亡。对 onData 订阅调用 dispose() 只移除了那个监听器 —— 子进程依然存活,而父进程不再收到它的字节。
  • 脚手架中另一个 PTY 驱动已被排除:InteractiveSession 只被 cron-interactive.test.ts 使用,而本检查项排除了该文件;并且它的 onData 从不写入 process.stdoutctrl-c-exit.test.ts 是唯一没有清理调用的交互式规格文件,而它整体是 it.skip

修复方案

TestRig 现在把每个派生的 PTY 与 ptyProcess.onData 返回的 IDisposable 配对保存,并在 cleanup() 中先 dispose 该监听器再杀进程。cleanup() 之后任何 PTY 字节都无法再到达 process.stdout,因此无论子进程的优雅退出耗时多久,EPIPE 机制都不可能发生 —— 该修复不再依赖于「抢在子进程退出之前」赢得竞态。

改动了两个文件,80 行新增、6 行删除,全部位于集成测试脚手架内。没有改动产品代码、CI workflow、vitest 配置,也没有删除或弱化任何测试。

见证测试(Witness test)

integration-tests/test-helper.test.ts 中的 detaches a session's output forwarding during cleanup 会派生一个像真实 CLI 一样捕获 SIGHUP 的替身子进程,并每 20 毫秒打印一个唯一的 canary 字符串。它同时断言该主张的两个部分:

  • 子进程在 cleanup() 之后仍然存活 —— 没有这一条,转发检查可能会在一个瞬间就死掉的子进程上空洞地通过;
  • cleanup() 之后的 500 毫秒内没有任何 canary 字节到达 process.stdout

范围之外

该检查项还有第二个、彼此独立的「无 FAIL 行」红灯来源:vitest 的 worker→main onTaskUpdate RPC 有固定的 60 秒预算,在资源压力下更长的停顿会以未处理的 [vitest-worker]: Timeout calling "onTaskUpdate" 错误浮现。这正是 2bd0ff923e#10112)为非 Linux 平台豁免、后来的 isSelfHostedRunner 分支为自托管机器池豁免的东西 —— 但本检查项是 GitHub 托管的 Linux,因此仍然暴露在外,而且它最多会以 4 个 fork 并行运行 PTY 密集型测试。

这一点被刻意没有改动。把 dangerouslyIgnoreUnhandledErrors 扩大到 GitHub 托管的 Linux,会让整个集成测试套件的未处理错误信号被静默掉,其中就包括本 PR 所修复的这一类 EPIPE;而现有配置注释已把这种收窄记录为一个有意为之的决定。这个取舍属于维护者,而不属于本次修复。在此说明是为了让该检查项残留的间歇性保持可见,而不是被当作不存在。

验证

在本次检出中实际执行过的命令(GitHub 托管 runner 镜像,Node v22.23.2,npm 10.9.8):

  • npm run build —— 通过(退出码 0,约 509 秒)
  • npm run typecheck —— 通过(退出码 0);其中包含 typecheck:integrationtsc -p integration-tests/tsconfig.json),覆盖了两个被改动的文件
  • npm run lint —— 通过(退出码 0);其中包含 eslint integration-tests
  • npx prettier --check integration-tests/test-helper.ts integration-tests/test-helper.test.ts —— 通过("All matched files use Prettier code style!")
  • cd integration-tests && npx vitest run test-helper.test.ts —— 8 个通过,含新增见证测试(测试耗时约 0.7 秒)
  • cd integration-tests && npx vitest run test-helper.test.ts renderer-matrix.test.ts globalSetup.test.ts fake-openai-server.test.ts —— 4 个文件,33 个通过
  • 变异探针(Mutation probe):cleanup() 中移除 session.forwardOutput.dispose();,然后执行 npx vitest run test-helper.test.ts -t "detaches" --retry=0 —— 按要求失败,报告在 cleanup() 之后的 500 毫秒内仍有约 25 个 canary 数据块被写入 process.stdout。恢复该行并重跑整个文件 —— 8 个通过。因此该见证测试是有效的。
  • pre-commit 钩子(node scripts/pre-commit.js / lint-staged)—— 通过;钩子执行后已重新核实提交内容。
  • npm run generate:settings-schema —— 未运行,这是正确的:没有 settings 源文件发生变化。

在此处无法运行的检查

  • 完全一致的失败检查项。 QWEN_E2E_RENDERER=opentui 需要 bun —— resolveE2eCliCommand 会探测 bun --version,否则抛错 —— 而本 runner 上没有安装 bun 1.3.14;安装它属于本 skill 允许命令之外的联网包操作。修复位于与渲染器无关的脚手架代码中,且见证测试运行在默认的 ink/node 路径上,因此它执行的正是 OpenTUI 检查项所使用的同一套「派生—清理」代码。
  • 原始 job 日志。 需要管理员权限(403);已改用上方表格所列的公开 run、job 与 annotation 元数据。
  • 交互式测试套件中依赖真实模型的用例未在此处端到端运行。workflow 的独立 CI 仍是最终的验证关口。

对提交信息的一处更正

提交信息中写的是同一个 SHA 在「九分钟后」通过了同一个检查项。实测两次检查项步骤开始时间的间隔是 7.8 分钟(04:20:40 对 04:28:28 UTC);两个运行的创建时间则相差 32.3 分钟。其实质性主张不受影响 —— 同一个 SHA 在不久之后通过了同一个检查项 —— 但该数字被向上取整了。本报告给出的是精确时间戳。按照本 skill 的规则,分支历史只能是增量式的,因此没有通过 amend 去更正该提交信息。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-2026-09-02

@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Sep 4, 2026
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed, not theoretical — and the evidence holds up under independent checking. I pulled both runs cited in the description rather than taking them on faith:

Run Event Conclusion Run interactive E2E tests (OpenTUI) step
33834473606 push failure 04:20:40 → 04:25:46 = 306 s
33836390526 schedule success 04:28:28 → 04:33:29 = 301 s

Same head SHA 56f75adf2992, same job name, 7 m 48 s apart, near-identical durations. Every number in the description matches the API exactly, and a suite that runs to completion in 301 s and then fails in 306 s without naming a test is the signature of a teardown-time unhandled error rather than a defect in the code under test. Issue #11002 is open and was filed by the main-CI detector for exactly this run.

I also checked the two load-bearing premises in the codebase rather than trusting the prose:

  • The CLI really does trap the hangup into an async shutdown. packages/cli/src/llm.tsx installs process.on('SIGHUP', handleSighup)beginExit('SIGHUP'), which is a .catch().finally(() => process.exit(...)) chain — so the child is genuinely still alive and still rendering after the harness's kill() returns.
  • This leg really is the only exposed Linux lane. integration-tests/vitest.config.ts sets dangerouslyIgnoreUnhandledErrors: process.platform !== 'linux' || isSelfHostedRunner. The shards run on the self-hosted ECS pool (exempt), macOS is exempt by platform, the nightly isolated legs are continue-on-error — and e2e-interactive-opentui is runs-on: ubuntu-latest with KEEP_OUTPUT: 'true' and VERBOSE: 'true'. Fatal, forwarding, GitHub-hosted. The claim checks out.

Direction: aligned. This is CI reliability for the repo's own harness, and it fixes a failure mode that costs maintainer attention every time it fires (one auto-filed issue per commit, with nothing to dedupe on). Worth noting the repo already learned this lesson once in a different corner — packages/cli/src/agent-view/pty-host.ts carries the comment "node-pty's signal-less kill falls back to SIGHUP on POSIX" and deliberately sends SIGTERM instead. This PR completes the same reasoning on the harness side.

Size: not applicable — no core paths touched. For the record: 86 changed lines total, 23 in integration-tests/test-helper.ts and 63 in its colocated spec. Nothing near the 500/1000-line thresholds, and Stage 0's two-tier gate does not engage.

Approach: the scope feels right, and I want to be explicit about the alternative I considered first, because it is the obvious one and it does not work. My initial instinct was "kill harder" — send SIGTERM the way pty-host.ts does, or SIGKILL, so the child stops rendering before the pipe goes away. Reading llm.tsx killed that idea: the CLI traps SIGTERM as well as SIGHUP into the same async beginExit, so switching signals just changes which signal is trapped and the window stays open. Only SIGKILL is untrappable, and paying for teardown determinism with a hard kill would forfeit the graceful shutdown that reaps the CLI's own children (MCP servers, daemons) — a worse trade than the one being fixed. Removing the write path instead of racing the child's exit is the correct choice, and it is the smaller blast radius.

The one thing I'd ask you to sit with, not as a blocker: a narrower variant exists that keeps the listener attached and just guards the write with a cleanedUp flag — a few lines fewer, no restructure of the session list, and it preserves _interactiveOutput accumulation, which is precisely the tradeoff your own Risk section names as the main one. I landed on the dispose version being better anyway — it releases the closure, cannot be defeated by a second forwarding path, and mirrors the disposables array pty-host.ts already maintains — but since the frozen buffer is the cost you're paying, it's worth a sentence in the description saying the flag variant was considered.

No unrelated changes, no drive-by refactors: the interactiveProcessesinteractiveSessions rename is forced by the payload change, and I confirmed by grep that those were the only three references in the repo.

Risk: no elevated risk signals — neither changed file matches the revert-correlated path list. The residual risk is the one you already documented (a future test expecting readable output after cleanup), and it is bounded: I checked every consumer, see the code review for that.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

**问题:**是已观测到的问题,不是理论性加固——而且证据经得起独立核查。我没有直接采信描述里的数字,而是自己拉取了所引用的两次运行:

运行 触发事件 结论 Run interactive E2E tests (OpenTUI) 步骤
33834473606 push failure 04:20:40 → 04:25:46 = 306 秒
33836390526 schedule success 04:28:28 → 04:33:29 = 301 秒

同一个 head SHA 56f75adf2992、同一个 job 名、相隔 7 分 48 秒、耗时几乎一致。描述里的每一个数字都与 API 完全吻合;一个跑满 301 秒成功、却在 306 秒失败且不指出任何测试名的套件,正是「teardown 阶段未处理错误」的特征,而不是被测代码里的缺陷。Issue #11002 仍处于 open 状态,且正是 main-CI 检测器针对这次运行建的。

描述里两个关键前提我也在代码库中核实过,而不是只看文字:

  • CLI 确实把挂断信号捕获成一次异步退出。packages/cli/src/llm.tsx 里装了 process.on('SIGHUP', handleSighup)beginExit('SIGHUP'),那是一条 .catch().finally(() => process.exit(...)) 链——所以脚手架的 kill() 返回之后,子进程确实还活着、还在渲染。
  • 这个检查项确实是唯一暴露的 Linux 通道。integration-tests/vitest.config.ts 里是 dangerouslyIgnoreUnhandledErrors: process.platform !== 'linux' || isSelfHostedRunner。分片跑在自托管 ECS 机器池(豁免),macOS 因平台豁免,nightly 隔离通道是 continue-on-error——而 e2e-interactive-opentuiruns-on: ubuntu-latest,并且带 KEEP_OUTPUT: 'true'VERBOSE: 'true'。既致命、又转发、又是 GitHub 托管。这个判断成立。

**方向:**对齐。这是仓库自身脚手架的 CI 可靠性问题,而它修掉的失败模式每次发生都要消耗维护者的注意力(检测器无从去重,只能按提交逐个建 issue)。值得一提的是,仓库在另一个角落已经吃过一次同样的亏——packages/cli/src/agent-view/pty-host.ts 里就写着注释「node-pty 不带信号的 kill 在 POSIX 上会退化为 SIGHUP」,并因此刻意改发 SIGTERM。本 PR 是在脚手架这一侧把同一套推理补完。

**规模:**不适用——没有触及核心路径。作为记录:共改动 86 行,其中 integration-tests/test-helper.ts 23 行、其同目录测试文件 63 行。远未达到 500/1000 行的阈值,Stage 0 的两级门禁不触发。

**方案:**范围合理。我想明确说明我最初考虑的替代方案,因为那是最直觉的一个,而它行不通。我一开始的想法是「杀得更狠」——像 pty-host.ts 那样发 SIGTERM,或者直接 SIGKILL,让子进程在管道消失之前就停止渲染。读完 llm.tsx 之后这个想法被否掉了:CLI SIGTERMSIGHUP 一视同仁,都捕获进同一个异步 beginExit,所以换信号只是换了个被捕获的信号,时间窗口依然开着。只有 SIGKILL 无法被捕获,但用硬杀来换 teardown 的确定性,会牺牲掉那次优雅退出——而正是它负责回收 CLI 自己的子进程(MCP server、daemon)——这个代价比要修的问题更糟。不去和子进程的退出抢时间、而是直接移除写入路径,是正确的选择,影响面也更小。

有一点想请你考虑,但不是阻塞项:存在一个更窄的变体——保留监听器,只用一个 cleanedUp 标志守住写入。它少几行、不需要重构会话列表,而且能保住 _interactiveOutput 的累积——而后者恰恰是你自己在「风险与范围」里点出的主要取舍。我最终仍认为 dispose 版本更好:它释放了闭包、不会被第二条转发路径绕过、也与 pty-host.ts 已经在维护的 disposables 数组相呼应。但既然冻结缓冲区就是你要付的代价,描述里加一句「已考虑过标志位变体」会更完整。

没有夹带无关改动,也没有顺手重构:interactiveProcessesinteractiveSessions 的重命名是载荷结构变化所必需的,而且我用 grep 确认了全仓库只有那三处引用。

**风险:**无升级风险信号——两个改动文件都不匹配与 revert 相关的路径清单。剩余风险就是你已经写下的那一条(未来某个测试期望清理之后仍能读到输出),而且是有界的:我核查了所有消费方,详见代码审查。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at d1e250dd4265fe6cee6cf6c54f5d68ccbbdf7fe5 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Code review

I wrote down my own fix before opening the diff, from the title and the "Why it's needed" section alone: stop the write path rather than try to win the race against the child's exit — either keep the IDisposable that onData hands back and dispose it during cleanup, or leave the listener attached and gate the process.stdout.write on a cleanedUp flag. The PR implements the first of those. It matches my proposal, and having now read both the diff and the surrounding code I think it's the better of the two — a flag would leave the closure subscribed and the accumulation growing for a child nobody is reading any more.

No critical blockers, and no AGENTS.md violations. What follows is what I checked and the two nits I'd leave to your judgement.

The pattern is already the repo's own. packages/cli/src/agent-view/pty-host.ts captures const dataDisposable = ptyProcess.onData(...) into a disposables array and drains it in dispose() immediately after ptyProcess.kill(...); terminal-bridge.ts does the same. So this isn't a new invention in the harness — it brings the harness in line with how the product side already tears a PTY session down. That's the reuse check satisfied rather than a parallel helper added.

The rename is complete. interactiveProcesses had exactly three references repo-wide (declaration, cleanup loop, push site) and all three are in the diff. Nothing dangles.

Disposing the data listener doesn't break the exit path. This was my main correctness worry going in — onData and onExit are independent subscriptions, only the former is disposed, so the onExit resolve in runInteractive still fires. The related worry, that removing the subscriber stalls the master fd and hangs the child, doesn't hold either: node-pty's read loop is wired at spawn and fires into its own emitter regardless of subscriber count.

The frozen-buffer risk is real but bounded — I checked every consumer. runInteractive is called from exactly eight interactive/*.test.ts specs plus test-helper.test.ts; no cli/, hook-integration/, sdk-typescript/ or terminal-bench/ caller. In all eight, rig.cleanup() sits in the afterEach and every await promise / rig._interactiveOutput read is in a test body far below it, so nothing reads accumulated output after cleanup. The two internal readers are safe for the same reason: waitForText() polls _interactiveOutput but is never called after cleanup, and the onExit resolve would return a truncated output for a post-cleanup exit but no spec awaits that promise after cleanup. ctrl-c-exit.test.ts reads result.output but is it.skip'd and never calls cleanup, and external-context-mem0-write.test.ts spawns through its own local pty.spawn helper that bypasses interactiveSessions entirely — unaffected by this change, and it already closes its window by awaiting the exit in a finally.

One correction to the description, not to the code: it says cleanup is "the last statement of the teardown hook" in every interactive spec. In external-context-auto-recall.test.ts the afterEach restores saved environment variables after await rig.cleanup(). Harmless — that block only touches process.env — so the conclusion stands, but the claim as written is a shade too strong.

The witness is well built. It asserts the child is still alive before asserting nothing was forwarded, which is what stops it passing vacuously on an already-dead child — the failure mode that would make the whole test decorative. The KEEP_OUTPUT precondition genuinely reaches the forwarding guard: test-helper.ts does import { env } from 'node:process', and that is the same object as process.env, so the test's mutation is visible to it (the two pre-existing KEEP_OUTPUT specs already rely on this). It reuses the neighbouring spec's stand-in-CLI pattern (rig.bundlePath = rig.createFile(...)) rather than building a new harness, and the assertion filters the captured chunks for the canary so unrelated worker stdout can't produce a false failure. The finally SIGKILLs the 30-second stand-in so a failing run can't leak it.

Two nits, neither blocking:

  • Aliveness is asserted immediately after cleanup() but not again after the 500 ms window. A child that died at +100 ms would still pass, with five suppressed chunks instead of twenty-five. Your mutation probe covers this, but a second isProcessAlive check after the sleep would make the witness self-contained rather than relying on step 2 of the test plan to prove it has teeth.
  • The process.stdout.write mock is worker-global and stays live across the 500 ms sleep, so anything else the worker writes in that window is captured and dropped rather than printed. Harmless in practice — vitest reports over IPC, forks are separate processes, and the canary filter isolates the assertion — but it is a global side effect worth knowing about.

The window being closed

sequenceDiagram
    participant P1 as TestRig cleanup
    participant P2 as PTY child (CLI)
    participant P3 as Worker stdout pipe
    participant P4 as Vitest worker
    P1->>P3: dispose the onData listener (this PR)
    P1->>P2: kill with no signal, sends SIGHUP
    P2->>P2: trap SIGHUP, begin async graceful exit
    P4->>P3: tear down worker, read end gone
    P2->>P1: still rendering, emits bytes
    Note over P1,P3: no subscriber left, bytes dropped, no write, no EPIPE
    P2->>P2: exits on its own schedule
Loading

Before this PR, the step after "still rendering, emits bytes" was a process.stdout.write into a pipe whose read end was already gone — an unlistened EPIPE, escalated by Node to an uncaught exception, non-zero exit with no FAIL line anywhere in the log. Disposing the listener first removes the write, so there is nothing left to lose the race.

Test evidence

This is an unattended CI run, so per the gate rules I did not build or execute anything from this PR. Everything below is the PR's own CI, read through the API for the reviewed commit.

The important one is green, and it's green on the specific new test rather than just the file:

✓ test-helper.test.ts (8 tests) 617ms
  ✓ TestRig > detaches a session's output forwarding during cleanup  606ms

 Test Files  21 passed (21)
   Duration  114.66s

— from Integration Tests (no-AK, No Sandbox) (job 100935044407). Eight tests in that file matches the count your test plan predicts, and there is no FAIL, Unhandled, or EPIPE line anywhere in the 167 KB log. The neighbouring pre-existing witness, kills an interactive session a test never closed during cleanup, also passed through the same cleanup path that now disposes first — so the reordering doesn't regress the #10969 behaviour.

The one red check is not yours. Dependency CVE audit failed with:

npm warn audit 503 Service Unavailable - POST https://registry.npmjs.org/-/npm/v1/security/audits/quick - Service Unavailable
{ error: 'Service Unavailable' }
npm error audit endpoint returned an error

twice, at 06:43:58 and 06:51:03. That's the npm registry's audit endpoint being unavailable, and I classified it as infra from the diff and the check identity rather than from anything the log claims about itself: this PR touches two .ts files under integration-tests/ and no manifest, so there is no dependency change for a CVE audit to react to.

Final CI results for d1e250d (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Dependency CVE audit ❌ failure
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Secret scan (TruffleHog) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Two coverage gaps I want to state plainly rather than let a green table imply otherwise:

The specs that actually consume the changed cleanup path don't run on this PR at all. e2e.yml triggers on push to main, schedule, and workflow_dispatch — not pull_request. So neither the OpenTUI leg being fixed nor the Linux shards that would run test-helper.test.ts under KEEP_OUTPUT=true execute here. The eight interactive/*.test.ts specs are the real consumers of rig.cleanup(), and the first time this change meets them is post-merge on main. That's structural, not something you did wrong, but it does mean the blast-radius analysis above is static reasoning plus one green harness file — not an observed green interactive suite.

Not verified: that the witness actually fails with the dispose line removed. Your step 2 mutation probe is the only evidence for that, and it's your claim, not something CI reproduces — a suite that passes identically with and without the fix is green and worthless, and nothing in the check table distinguishes those two cases. Also not verified: that a child which writes still exits normally after its data listener is disposed. The pre-existing witness proves kill-after-dispose terminates a child, but its stand-in is idle, and your new stand-in is SIGKILLed in the finally rather than polled to death — so the writing-child case sits between the two tests rather than inside either.

Sandboxed verification would settle both: @qwen-code /verify — specifically the A/B mutation arm, that removing the single session.forwardOutput.dispose() line turns the new witness red with roughly 25 canary chunks forwarded after cleanup, and that a writing stand-in child still reaches exit once its data listener is disposed. Neither is observable from the diff, and this PR's suite passes without proof of either. You have write access, so this is a direct run rather than a sponsored one. /tmux is not the right lane here — there is no TUI surface, and the failure lives in worker teardown.

中文说明

代码审查

我在打开 diff 之前,只凭标题和「为什么需要它」写下了自己的修法:不要去和子进程的退出抢时间,而是直接切断写入路径——要么保留 onData 返回的 IDisposable、在清理时 dispose 掉,要么保留监听器、只用一个 cleanedUp 标志守住 process.stdout.write。本 PR 实现的是前者。它与我的提案一致;读完 diff 和周边代码之后,我认为两者中它更好——标志位方案会让闭包一直挂着,让一个再没人读的字符串继续增长。

没有阻塞性问题,也没有违反 AGENTS.md。下面是我核查过的内容,以及两个留给你判断的小点。

这个模式本来就是仓库自己的。packages/cli/src/agent-view/pty-host.ts 就是把 const dataDisposable = ptyProcess.onData(...) 收进 disposables 数组,并在 dispose() 里紧跟 ptyProcess.kill(...) 之后逐个排空;terminal-bridge.ts 同理。所以这不是在脚手架里新发明的东西,而是把脚手架对齐到产品侧本来就在用的 PTY 会话拆除方式。复用检查因此是满足的,而不是又加了一个平行 helper。

重命名是完整的。interactiveProcesses 全仓库只有三处引用(声明、清理循环、push 点),三处都在 diff 里,没有遗漏。

**dispose 数据监听器不会破坏退出路径。**这是我进来时主要的正确性顾虑——onDataonExit 是两个独立订阅,被 dispose 的只有前者,所以 runInteractive 里的 onExit resolve 仍会触发。相关的另一个顾虑(移除订阅者会卡住 master fd、把子进程挂死)也不成立:node-pty 的读取循环是在 spawn 时接好的,无论订阅者数量如何都会向它自己的 emitter 派发。

缓冲区冻结的风险是真实的,但有界——我核查了每一个消费方。runInteractive 的调用方恰好是八个 interactive/*.test.ts 加上 test-helper.test.tscli/hook-integration/sdk-typescript/terminal-bench/ 里都没有。这八个里,rig.cleanup() 都在 afterEach 中,而所有 await promise / rig._interactiveOutput 的读取都在远低于它的测试体内,所以没有任何地方在清理之后读取累积输出。两个内部读取方同理安全:waitForText() 会轮询 _interactiveOutput,但从不在清理之后被调用;onExit 的 resolve 对「清理之后才退出」的情况会返回被截断的 output,但没有哪个 spec 在清理之后 await 那个 promise。ctrl-c-exit.test.ts 读了 result.output,但它是 it.skip,且从不调用 cleanup;external-context-mem0-write.test.ts 用自己本地的 pty.spawn helper 派生,完全绕过 interactiveSessions——不受本改动影响,而且它已经在 finally 里 await 退出、自己关掉了那个窗口。

对描述(不是对代码)的一处更正:它说在每个交互式 spec 里 cleanup 都是「teardown 钩子的最后一条语句」。在 external-context-auto-recall.test.ts 里,afterEachawait rig.cleanup() 之后还会还原保存的环境变量。无害——那段只动 process.env——所以结论不变,但这个说法写得略强了些。

**见证测试写得扎实。**它先断言子进程仍然存活、再断言没有任何东西被转发,这正是防止它在一个早已死掉的子进程上空洞通过的关键——那种失效会让整个测试变成装饰品。KEEP_OUTPUT 这个前提确实能到达转发判断:test-helper.ts 里是 import { env } from 'node:process',而它与 process.env 是同一个对象,所以测试对它的修改是可见的(两个已有的 KEEP_OUTPUT spec 早就依赖这一点)。它复用了相邻 spec 的替身 CLI 写法(rig.bundlePath = rig.createFile(...)),没有另造一套脚手架;断言还会对捕获到的数据块按 canary 过滤,所以 worker 里无关的 stdout 不会造成误报。finally 会 SIGKILL 那个 30 秒的替身,失败时不会泄漏进程。

两个小点,都不阻塞:

  • 存活性是在 cleanup() 之后立刻断言的,但没有在那 500 毫秒窗口之后再断言一次。一个在 +100 毫秒就死掉的子进程仍然会让测试通过,只是被抑制的数据块是 5 个而不是 25 个。你的变异探针覆盖了这一点,但在 sleep 之后再加一次 isProcessAlive 会让见证测试自成闭环,而不必依赖测试计划的第 2 步来证明它有牙齿。
  • process.stdout.write 的 mock 是 worker 全局的,并且在那 500 毫秒 sleep 期间一直生效,所以该窗口内 worker 写的任何其他内容都会被捕获并丢弃、而不是打印出来。实践中无害——vitest 通过 IPC 上报、fork 是独立进程、canary 过滤又把断言隔离开了——但这确实是一个值得知道的全局副作用。

被关上的那个时间窗口

上方的时序图展示了本 PR 之后的路径:清理先 dispose 掉 onData 监听器,再发出不带信号的 kill(即 SIGHUP);子进程把 SIGHUP 捕获成一次异步优雅退出,vitest 随后拆除 worker、管道读端消失,而子进程仍在渲染、仍在发字节——但已经没有订阅者了,字节被丢弃,不产生写入,也就不会有 EPIPE。

改动之前,「仍在渲染、发出字节」之后的那一步是往一个读端已消失的管道里做 process.stdout.write:一个无人监听的 EPIPE,被 Node 升级为 uncaught exception,运行以非零退出,而日志里没有任何 FAIL 行。先 dispose 监听器就移除了这次写入,于是再没有东西需要去赢那场竞态。

测试证据

这是一次无人值守的 CI 运行,因此按门禁规则我没有构建或执行本 PR 的任何代码。下面全部内容都是本 PR 自己的 CI,通过 API 针对被审查的那个提交读取。

关键的那一项是绿的,而且是绿在具体那个新测试上、不只是文件级别:Integration Tests (no-AK, No Sandbox)(job 100935044407)报告 test-helper.test.ts (8 tests) 617ms,其中 TestRig > detaches a session's output forwarding during cleanup 606ms 通过,整套 Test Files 21 passed (21),耗时 114.66 秒。八个测试与你测试计划里预测的数字一致,167 KB 的日志里没有任何 FAILUnhandledEPIPE 行。相邻那个已有的见证测试 kills an interactive session a test never closed during cleanup 也通过了同一条现在会先 dispose 的清理路径——所以这个顺序调整没有让 #10969 的行为回退。

唯一的红灯不是你的。Dependency CVE audit 失败于 npm registry 的审计端点返回 503(06:43:58 与 06:51:03 各一次)。我把它归为基础设施问题,依据是 diff 和检查项本身的性质,而不是日志正文里的任何自我声明:本 PR 只改了 integration-tests/ 下两个 .ts 文件,没有动任何 manifest,因此没有任何依赖变化可供 CVE 审计反应。

上方的 CI 表格已用机器可读的区域标记包裹,CI 跑完之后 finalize 任务会就地更新它。

有两个覆盖缺口我想直接说明,而不是让一张绿色的表暗示出不存在的结论:

真正消费这条被改动清理路径的那些 spec,在本 PR 上根本没有运行。e2e.yml 的触发条件是 push 到 mainscheduleworkflow_dispatch——不包含 pull_request。所以被修的那个 OpenTUI 检查项、以及在 KEEP_OUTPUT=true 下会跑 test-helper.test.ts 的 Linux 分片,在这里都不会执行。八个 interactive/*.test.ts 才是 rig.cleanup() 的真实消费方,而本改动第一次遇到它们,是合并之后在 main 上。这是结构性的,不是你做错了什么,但确实意味着上面对影响范围的分析是静态推理加一个变绿的脚手架文件——而不是一个被观测到变绿的交互式套件。

未验证:把 dispose 那一行删掉之后见证测试是否真的会失败。你的第 2 步变异探针是这一点唯一的证据,而它是你的说法,CI 并不会复现它——一个「有改动」和「没改动」都同样通过的套件既是绿的、也是没有价值的,而检查表里没有任何东西能区分这两种情况。同样未验证:一个会写输出的子进程在其数据监听器被 dispose 之后是否仍能正常退出。已有的见证测试证明了「先 dispose 再 kill」能终止子进程,但它的替身是空闲的;而你的新替身是在 finally 里被 SIGKILL 的,并没有被轮询到自然死亡——所以「会写的子进程」这一情形落在两个测试之间,而不在任何一个之内。

沙箱验证可以把这两点都定下来:@qwen-code /verify——具体是 A/B 变异那一臂,即删掉 session.forwardOutput.dispose() 这一行是否会让新见证测试变红、并显示清理之后仍转发了约 25 个 canary 数据块;以及一个会写输出的替身子进程在其数据监听器被 dispose 之后是否仍能走到退出。这两点都无法从 diff 观察,而本 PR 的套件在两者都没有被证明的情况下就通过了。你有 write 权限,所以这是一次直接运行,而不是需要维护者背书的 sponsored run。/tmux 在这里不是合适的通道——没有 TUI 界面,失效发生在 worker teardown 里。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at d1e250dd4265fe6cee6cf6c54f5d68ccbbdf7fe5 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — the change itself is clean and I would merge it on its own merits; what I can't decide is which of two competing open PRs should own these lines.

⏸️ Deferring to a maintainer rather than approving. Not because of anything wrong in this diff — because approving here would silently settle a choice that isn't mine to make.

The thing that stopped me

I ran the "is this part of a pattern?" check the gate asks for, and it found something. This PR's author has eleven PRs open, three of them fix(test) in the last two hours. Two of those three are competing fixes for the same failure window in the same function:

#11007 (this PR) #11001
Title stop forwarding a cleaned-up PTY session into stdout wait for interactive PTY sessions to end during cleanup
Opened 06:36Z 04:58Z — 1 h 38 m earlier
Linked issue #11002 (04:59Z) #10990 (03:22Z)
Files integration-tests/test-helper.ts + its spec the identical two files
Strategy dispose the onData listener, then kill kill, then await the child's exit (10 s grace)
Session payload { ptyProcess, forwardOutput: pty.IDisposable } { ptyProcess, exited: Promise<unknown> }

They rewrite the same three sites — the interactiveProcesses field at line 203, the cleanup loop at 500–506, and the push site at 947 — with incompatible payload shapes. Both are MERGEABLE against main right now, because GitHub computes that per-PR; the collision lands the moment the second one merges. Three more concrete overlaps:

And neither PR mentions the other. This one's Related list names #10969, #10112, #10325 and #10085 — not #11001, not #10990.

The two linked issues are the same failure: both name workflow E2E Tests, job E2E Interactive - OpenTUI renderer (bun), step Run interactive E2E tests (OpenTUI), differing only in commit (b7815a7e1a82 vs 56f75adf2992). That is precisely the per-commit issue churn with nothing to dedupe on that this PR's own description opens by complaining about — and the autofix pipeline answered it by filing twice and producing two mutually exclusive fixes.

This bot already triaged #11001: Confidence 4/5, with a deferred approval that was then withheld when its CI didn't settle. So both PRs currently sit at "the bot liked this one", and they cannot both land. main requires two approving reviews; a bot approval on each of two mutually exclusive PRs is worse than no approval, because it reads as two independent endorsements of one decision that was never made.

The substantive part of the choice

This isn't just merge mechanics — the two strategies differ in a way that matters for this specific leg, and it's the reason I'd want a human to pick rather than let merge order decide.

#11001 makes cleanup() block until each session's child actually exits, bounded by INTERACTIVE_EXIT_GRACE_MS = 10_000, awaited per session inside every interactive spec's afterEach. That genuinely closes the window — a child that is gone cannot write.

But this PR's own Risk section names the leg's second independent source of red runs with no named test: "the test runner's fixed 60-second worker-to-main RPC budget, which a stall under load can exceed", while the leg runs "up to four parallel forks of pseudo-terminal-heavy tests". Adding up to ten seconds of awaited teardown per session, in the most PTY-heavy suite, on the most loaded lane, pushes directly against that budget. So on the evidence in this PR's description, #11001's strategy risks feeding the failure mode this PR deliberately declined to widen the exemption for.

Detaching costs zero teardown latency and closes the window unconditionally — a byte that never reaches stdout cannot break a pipe however long the graceful shutdown takes — at the price of freezing _interactiveOutput, which I verified nothing reads.

My read: this PR's strategy is the better of the two for this leg. If a maintainer wants both, they compose well, and the ordering matters: dispose first, then await. Once nothing can EPIPE, the await is no longer safety-critical, so its grace can drop well below ten seconds — you'd get #11001's guarantee that no child leaks past teardown without paying its latency in the RPC budget. But that's a design call with a tradeoff attached, and it isn't one an approval on either PR should settle by accident.

Why I'm not approving

Not the code. The code is minimal, correct, mirrors pty-host.ts's existing teardown pattern, and its witness is green on CI at 606 ms with 21 files passing. My two nits are in Stage 2 and neither blocks.

I'm deferring because the merge decision is contested and I'd be breaking the tie silently. What I'd ask a maintainer to do:

  1. Pick one strategy for TestRig.cleanup() — or take the compose-both option above — and reconcile the other PR against it rather than letting merge order decide.
  2. Close the loser against the survivor so the work isn't stranded, and dedupe Main CI failed: E2E Tests on b7815a7e1a82 #10990 / Main CI failed: E2E Tests on 56f75adf2992 #11002, which are one failure filed twice. The detector's per-commit keying is what produced this, and it will keep producing it until the leg stops reddening.
  3. If this one is the survivor, @qwen-code /verify is still worth running for the reason in Stage 2: nothing in CI proves the witness has teeth, and the writing-child-still-exits-after-dispose case sits between the two tests rather than inside either.

One small corroboration for the red check: Dependency CVE audit also went red on #11001, whose author bot then recorded that it passes on current main. That's a second, independent source for my Stage 2 classification — registry 503, pre-existing infra, clears on re-run.

No @mention, deliberately. I ran the deterministic owner resolver rather than eyeballing one: $QWEN_MAINTAINER_HANDLE is unset, this PR carries only review/self-reported so no area in .github/issue-owners.json matches, and there is no human reviewer to fall back to. The resolver returned nothing, and guessing a login would notify the wrong person — so this comment goes out unaddressed. Adding an area label would route it, and whoever picks it up: the question is the strategy choice above, not the diff.

中文说明

Confidence: 3/5 —— 改动本身是干净的,单看它我会合并;我无法判断的是「这两个互相竞争的开放 PR,哪一个才该拥有这几行代码」。

⏸️ **转交维护者,而不是批准。**不是因为这份 diff 有任何问题——而是因为在这里批准,等于悄悄替别人做了一个不该由我做的选择。

让我停下来的那件事

我执行了门禁要求的「这是否属于某种模式」检查,结果发现了问题。本 PR 的作者有 11 个开放 PR,其中三个是最近两小时内的 fix(test)。而这三个里有两个,是对同一个函数里同一个失效窗口的竞争性修复

#11007(本 PR) #11001
标题 stop forwarding a cleaned-up PTY session into stdout wait for interactive PTY sessions to end during cleanup
开启时间 06:36Z 04:58Z——早 1 小时 38 分
关联 issue #11002(04:59Z) #10990(03:22Z)
文件 integration-tests/test-helper.ts 及其 spec 完全相同的两个文件
策略 dispose 掉 onData 监听器,然后 kill kill,然后 await 子进程退出(10 秒宽限)
会话载荷 { ptyProcess, forwardOutput: pty.IDisposable } { ptyProcess, exited: Promise<unknown> }

它们改写了同样的三处——第 203 行的 interactiveProcesses 字段、500–506 行的清理循环、947 行的 push 点——而且载荷结构互不兼容。两者目前对 main 都显示 MERGEABLE,因为 GitHub 是按 PR 单独计算的;冲突会在第二个合并的那一刻才落地。另外三处具体重叠:

而且两个 PR 都没有提到对方。本 PR 的「相关」清单点了 #10969#10112#10325#10085——没有 #11001,也没有 #10990

这两个关联 issue 是同一次失效:都指向 workflow E2E Tests、job E2E Interactive - OpenTUI renderer (bun)、step Run interactive E2E tests (OpenTUI),只有提交不同(b7815a7e1a8256f75adf2992)。这恰恰就是本 PR 描述开篇所抱怨的「按提交逐个建 issue、检测器无从去重」——而 autofix 流水线的回应是建了两次、并产出了两个互斥的修复。

本 bot 已经对 #11001 做过 triage:Confidence 4/5,随后因其 CI 未收敛而撤回了延迟批准。所以两个 PR 目前都停在「bot 认为这个不错」的状态,而它们不可能同时落地。main 需要两个批准;对两个互斥的 PR 各给一个 bot 批准,比不批准更糟——因为它看起来像是对一个从未被做出过的决定的两次独立背书。

这个选择里真正有实质内容的部分

这不只是合并机制问题——两种策略的差异对这个具体检查项是有影响的,这也是我希望由人来挑、而不是让合并顺序来定的原因。

#11001cleanup() 阻塞到每个会话的子进程真正退出为止,上界是 INTERACTIVE_EXIT_GRACE_MS = 10_000,并且是在每个交互式 spec 的 afterEach 里按会话逐个 await。这确实关上了窗口——已经消失的子进程不可能再写。

但本 PR 自己的「风险与范围」点出了该检查项第二个彼此独立的「无测试名红灯」来源:「测试运行器固定的 60 秒 worker→main RPC 预算,负载下的停顿可能超出它」,而该检查项会「以最多四个并行 fork 运行伪终端密集型测试」。在最重伪终端的套件里、在最拥挤的通道上,按会话增加最多十秒的 await teardown,正是直接顶着那个预算。所以按本 PR 描述里的证据,#11001 的策略有喂养「本 PR 刻意拒绝为其扩大豁免」那个失效模式的风险。

摘除转发的 teardown 延迟是零,并且无条件关上窗口——一个从未到达 stdout 的字节不可能打破管道,无论优雅退出耗时多久——代价是冻结 _interactiveOutput,而我已核实没有任何地方读它。

**我的判断:就这个检查项而言,本 PR 的策略是两者中更好的一个。**如果维护者想要两者,它们可以很好地组合,而且顺序很重要:先 dispose,再 await。一旦不可能 EPIPE,那个 await 就不再是安全关键的,它的宽限时间就可以远低于十秒——你既能得到 #11001 的「没有子进程泄漏到 teardown 之后」的保证,又不必在 RPC 预算里付出它的延迟。但这是一个带着取舍的设计决定,不该由对任一 PR 的一次批准来意外地拍板。

为什么我不批准

不是因为代码。代码是最小化的、正确的、与 pty-host.ts 已有的 teardown 模式相呼应,而且它的见证测试在 CI 上是绿的:606 毫秒,21 个文件通过。我的两个小点在 Stage 2 里,都不构成阻塞。

我之所以转交,是因为合并决定存在竞争,而我会在无人察觉的情况下打破这个平局。我希望维护者做的:

  1. TestRig.cleanup() 选定一种策略——或者采用上面的「两者组合」方案——并让另一个 PR 与之对齐,而不是让合并顺序来决定。
  2. 把落选的那个对着胜出者关闭,免得工作被搁浅;并对 Main CI failed: E2E Tests on b7815a7e1a82 #10990 / Main CI failed: E2E Tests on 56f75adf2992 #11002 去重,它们是同一次失效被建了两遍。检测器按提交建 issue 的键控方式正是成因,而在那个检查项停止变红之前,它会继续这样产出。
  3. 如果本 PR 是胜出者,@qwen-code /verify 仍值得跑一次,理由见 Stage 2:CI 里没有任何东西能证明这个见证测试有牙齿,而「会写输出的子进程在监听器被 dispose 之后仍能退出」这一情形落在两个测试之间,而不在任何一个之内。

关于那个红灯检查项的一点旁证:Dependency CVE audit#11001 上也变红了,而其作者 bot 随后记录它在当前 main 上是通过的。这为我 Stage 2 的归类提供了第二个独立来源——registry 503,既有基础设施问题,重跑即可恢复。

**刻意不做 @mention。**我运行的是确定性的 owner 解析器,而不是凭眼力挑一个:$QWEN_MAINTAINER_HANDLE 未设置,本 PR 只带 review/self-reported,因此 .github/issue-owners.json 里没有任何 area 匹配,也没有可回退的人类审阅者。解析器什么都没返回,而猜一个 login 只会通知到错误的人——所以这条评论不点名发出。加上一个 area 标签就能把它路由出去;无论谁接手:需要判断的是上面那个策略选择,而不是这份 diff。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at d1e250dd4265fe6cee6cf6c54f5d68ccbbdf7fe5 · re-run with @qwen-code /triage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Main CI failed: E2E Tests on 56f75adf2992

2 participants