-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(test): stop forwarding a cleaned-up PTY session into stdout (#11002) #11007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,11 @@ function isProcessAlive(pid: number): boolean { | |||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); | ||||||||||||
|
|
||||||||||||
| /** Emitted by the stand-in CLI below; nothing else in the run writes it. */ | ||||||||||||
| const FORWARD_CANARY = 'PTY_FORWARD_CANARY_11002'; | ||||||||||||
|
|
||||||||||||
| describe('TestRig', () => { | ||||||||||||
| const originalKeepOutput = process.env['KEEP_OUTPUT']; | ||||||||||||
|
|
||||||||||||
|
|
@@ -89,6 +94,64 @@ describe('TestRig', () => { | |||||||||||
| .toBe(false); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it("detaches a session's output forwarding during cleanup", async () => { | ||||||||||||
| // KEEP_OUTPUT is what the OpenTUI leg sets, and it is what makes the rig | ||||||||||||
| // forward every PTY byte into this worker's stdout. | ||||||||||||
| process.env['KEEP_OUTPUT'] = 'true'; | ||||||||||||
| const rig = new TestRig(); | ||||||||||||
| await rig.setup('cleanup detaches interactive output'); | ||||||||||||
| // Stands in for the CLI bundle, which traps the SIGHUP that node-pty's | ||||||||||||
| // signal-less kill() sends and keeps rendering while its exit cleanup | ||||||||||||
| // drains. The child therefore outlives cleanup() on purpose and keeps | ||||||||||||
| // producing bytes: what is under test is whether the harness still | ||||||||||||
| // forwards them into a stdout pipe vitest is about to destroy (#11002). | ||||||||||||
| rig.bundlePath = rig.createFile( | ||||||||||||
| 'slow-exit-cli.js', | ||||||||||||
| [ | ||||||||||||
| "process.on('SIGHUP', () => {});", | ||||||||||||
| `setInterval(() => process.stdout.write('${FORWARD_CANARY}\\n'), 20);`, | ||||||||||||
| 'setTimeout(() => process.exit(0), 30000);', | ||||||||||||
| '', | ||||||||||||
| ].join('\n'), | ||||||||||||
| ); | ||||||||||||
|
|
||||||||||||
| const { ptyProcess } = rig.runInteractive(); | ||||||||||||
| try { | ||||||||||||
| await expect | ||||||||||||
| .poll(() => rig._interactiveOutput.includes(FORWARD_CANARY), { | ||||||||||||
| message: 'the stand-in CLI never produced output', | ||||||||||||
| timeout: 20_000, | ||||||||||||
| }) | ||||||||||||
| .toBe(true); | ||||||||||||
|
|
||||||||||||
| const forwarded: string[] = []; | ||||||||||||
| vi.spyOn(process.stdout, 'write').mockImplementation((chunk) => { | ||||||||||||
| forwarded.push(String(chunk)); | ||||||||||||
| return true; | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| await rig.cleanup(); | ||||||||||||
| // Still alive: it swallowed SIGHUP. That is the window the real CLI's | ||||||||||||
| // graceful shutdown opens between cleanup() and the child's own exit. | ||||||||||||
| expect(isProcessAlive(ptyProcess.pid)).toBe(true); | ||||||||||||
|
|
||||||||||||
| forwarded.length = 0; | ||||||||||||
| await sleep(500); | ||||||||||||
|
Comment on lines
+138
to
+139
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-4: The witness certifies the producer child is alive only at the instant Witness:
Suggested change
The re-check must use the same 中文说明该见证测试只在 建议修复:在窗口之后重新断言子进程存活(上方 suggestion 块;若偶发不稳,可加宽窗口并在窗口末尾复查,而不是去掉该检查)。 修复约束:复查必须使用与现有存活检查(test-helper.test.ts:136)相同的 — qwen3.8-max via Qwen Code /review (v0.23.0) |
||||||||||||
|
|
||||||||||||
| expect( | ||||||||||||
| forwarded.filter((chunk) => chunk.includes(FORWARD_CANARY)), | ||||||||||||
| 'cleanup() left the session forwarding PTY bytes into stdout', | ||||||||||||
| ).toEqual([]); | ||||||||||||
| } finally { | ||||||||||||
| vi.restoreAllMocks(); | ||||||||||||
| try { | ||||||||||||
| process.kill(ptyProcess.pid, 'SIGKILL'); | ||||||||||||
| } catch { | ||||||||||||
| // Already gone | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it.each([ | ||||||||||||
| [ | ||||||||||||
| 'telemetry events', | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] R1-1: This witness test has no positive control — it never asserts that stdout forwarding was actually live before cleanup, so it can pass vacuously if the forwarding branch is ever silently disabled. The first poll pins
rig._interactiveOutput, which is appended unconditionally BEFORE theKEEP_OUTPUTgate (test-helper.ts:960-963), so it proves only that theonDatahandler fired — never that the gatedprocess.stdout.write(data)ran. The spy is installed only after that poll, andforwarded.length = 0discards whatever it captured before cleanup, so a future one-line regression — deletingprocess.stdout.write(data)or inverting the gate — produces exactly the empty output the post-cleanup assertion expects, and the suite's only guard over the #11002 fix passes while the path it guards is dead.Witness:
Suggested fix — insert a positive control after the spy block, before cleanup:
The fix rests on the gate
if (env.KEEP_OUTPUT === 'true' || env.VERBOSE === 'true')(integration-tests/test-helper.ts:961-963) whereenvis the liveprocess.env(test-helper.ts:12) — the positive control must exercise that same gate, not a locally re-implemented condition. The positive-control assertion is its own pin: deletingprocess.stdout.write(data)(test-helper.ts:962) or inverting the KEEP_OUTPUT gate turns it red instead of the test passing silently.中文说明
该见证测试缺少阳性对照——它从未断言清理之前 stdout 转发确实处于启用状态,因此一旦转发分支被静默禁用,测试就会空洞地通过。第一个
expect.poll钉住的是rig._interactiveOutput,而它是在KEEP_OUTPUT门(test-helper.ts:960-963)之前无条件追加的,所以它只能证明onData回调触发过,无法证明受门控的process.stdout.write(data)真的执行过。spy 在该 poll 之后才安装,且forwarded.length = 0会丢弃清理前捕获到的所有内容——于是未来任何一行回归(删除process.stdout.write(data)或反转门条件)都会恰好产生清理后断言所期望的空数组,这个套件中针对 #11002 修复的唯一守卫会在其所守护的路径已经失效时依然通过。建议修复:在清理之前加入阳性对照——安装 spy 后先轮询直到观察到转发(上方代码块),然后再清零
forwarded、执行rig.cleanup(),并保留现有的清理后空数组断言。修复约束:所要断言的门是
if (env.KEEP_OUTPUT === 'true' || env.VERBOSE === 'true')(integration-tests/test-helper.ts:961-963),其中env是实时的process.env(test-helper.ts:12)——阳性对照必须真实经过同一个门,而不是本地重新实现的条件。修复验收标准:阳性对照断言本身即是钉桩——删除process.stdout.write(data)(test-helper.ts:962)或反转 KEEP_OUTPUT 门会使其变红,而不再是测试静默通过。— qwen3.8-max via Qwen Code /review (v0.23.0)