Skip to content

test(core): cover bundled ConPTY fallback and terminal replies - #11555

Merged
yiliang114 merged 1 commit into
mainfrom
codex/windows-bundled-conpty-regressions
Sep 10, 2026
Merged

test(core): cover bundled ConPTY fallback and terminal replies#11555
yiliang114 merged 1 commit into
mainfrom
codex/windows-bundled-conpty-regressions

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

What this PR does

Closes the focused test and documentation gaps left after #11497 and #11313 landed. The Windows bundled-ConPTY decision is now shared by backend selection, terminal-reply forwarding, and the pre-spawn fallback guard. The fallback coverage proves that a command is never handed to child_process after its PTY has already started, and the reply coverage now sends a real device-attributes query through @xterm/headless instead of manually invoking the forwarding listener. The ConPTY explanation also names the actual ConptyReleasePseudoConsole mechanism and the correct Windows host census.

Why it's needed

Before this change, removing the post-spawn guard still left all 152 tests green even though the command could execute once through PTY and then again through the fallback. The terminal-reply test also supplied a synthetic response directly to the listener, so it could not detect a parser/protocol integration regression and asserted a response that the installed xterm version does not generate. Finally, the source comment cited a conhost.exe-only before/after count even though bundled ConPTY normally launches OpenConsole.exe.

Reviewer Test Plan

How to verify

Confirm that a Windows PTY which spawns successfully and then hits a synchronous handle-construction error returns executionMethod: 'none' without invoking the fallback. Feed CSI c through the real headless terminal and confirm that its generated CSI ? 1 ; 2 c reply is written back to the PTY. The focused suite passes 153/153 tests. Mutation checks independently confirmed that removing the post-spawn marker makes the fallback test fail with one child_process spawn, while disabling the Windows reply forwarder makes the real-parser test fail with zero PTY writes.

Evidence (Before & After)

N/A — test hardening and comment correction only; no user-visible UI change.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

macOS arm64, Node 22, Vitest 3.2.7, and @xterm/headless 5.5.0. The focused suite, core typecheck, core build, targeted ESLint, and Prettier checks passed.

Risk & Scope

  • Main risk or tradeoff: no intended runtime behavior change; the stable platform check is evaluated once per PTY execution so the three causally linked Windows decisions cannot drift apart.
  • Not validated / out of scope: packaged Windows execution, actual conhost.exe/OpenConsole.exe process counts, and the console-attached-survivor behavior called out in fix(core): host Windows shell PTYs via bundled ConPTY to stop orphaning conhost.exe #11497's verification comment.
  • Breaking changes / migration notes: none.

Linked Issues

Follow-up to #11497 and #11313. Refs #11303.

中文说明

这个 PR 做了什么

补齐 #11497#11313 合入后留下的定向测试与文档缺口。Windows bundled ConPTY 的判断现在由后端选择、终端应答转发和 spawn 前回退守卫共同复用。回退测试证明 PTY 已经启动后,命令绝不会再次交给 child_process;应答测试则通过真实 @xterm/headless 发送 device-attributes 查询,不再手工调用转发 listener。ConPTY 注释也改为说明实际的 ConptyReleasePseudoConsole 机制和正确的 Windows host 统计口径。

为什么需要

修改前,即使删除 post-spawn 守卫,原有 152 个测试仍全部通过,但命令可能先经 PTY 执行一次,再经 fallback 重复执行。原来的终端应答测试也只是把合成响应直接交给 listener,无法发现 parser/protocol 集成回归,而且断言的响应并不是当前 xterm 版本实际生成的值。源码注释还引用了只统计 conhost.exe 的前后对比,但 bundled ConPTY 通常启动的是 OpenConsole.exe

审阅者测试计划

如何验证

确认 Windows PTY 成功 spawn 后,如果构造返回 handle 时同步失败,会返回 executionMethod: 'none',且不会调用 fallback。把 CSI c 送入真实 headless terminal,确认它生成的 CSI ? 1 ; 2 c 应答被写回 PTY。定向测试 153/153 通过。独立 mutation 验证表明:删除 post-spawn 标记后,fallback 测试会因出现一次 child_process spawn 而失败;关闭 Windows 应答转发后,真实 parser 测试会因 PTY write 次数为 0 而失败。

证据(改动前后)

N/A——仅测试加固和注释纠正,没有用户可见的 UI 变化。

测试环境

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

macOS arm64、Node 22、Vitest 3.2.7、@xterm/headless 5.5.0。定向测试、core typecheck、core build、定向 ESLint 和 Prettier 均通过。

风险与范围

  • 主要风险或取舍:无预期运行时行为变化;稳定的平台判断现在每次 PTY 执行只计算一次,避免三个因果相关的 Windows 决策发生漂移。
  • 未验证 / 超出范围:打包后的 Windows 运行、真实 conhost.exe/OpenConsole.exe 进程计数,以及 fix(core): host Windows shell PTYs via bundled ConPTY to stop orphaning conhost.exe #11497 验证评论指出的 console-attached survivor 行为。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

#11497#11313 的后续。Refs #11303

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on 6a8f250 and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— 6a8f250 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed, not theoretical — and I could confirm the gap statically. The Windows bundled ConPTY backend (#11303) suite only covered the pre-spawn side of the guard: falls back to child_process when the bundled ConPTY spawn throws on Windows stubs pty.spawn itself, so nothing ever raised after ptySpawned = true. Dropping the marker would indeed have left the suite green while a command could run once through the PTY and again through the child_process fallback. The device-attributes test was tautological the same way — it fed a literal into the captured listener and asserted that same literal reached pty.write, proving the forwarder forwards and nothing about xterm's parser. Real defects in the tests, with the mutation runs as corroboration.

Direction: aligned. Follow-up hardening on #11497 and #11313 (both merged) for #11303 (still open), so the area's direction is already settled and there is no product question here. I skipped the external CHANGELOG signal — it is not probative for internal test quality on an already-shipped Windows fix.

Size: core paths are touched (packages/core/src/services/**). Production 49 lines (shellExecutionService.ts +24/−25 — mostly the comment rewrite and a ternary re-indent; the logic delta is one hoisted const plus three substitutions), test 60 lines (shellExecutionService.test.ts +43/−17), generated/schema 0. Well under 500, so no maintainer-awareness escalation and no large-PR advisory.

Approach: the scope feels right and the two tests pair correctly with what was already there. Two things worth thinking about, neither a blocker:

Risk: Stage 1e matched — shellExecutionService.ts is on the revert-correlated path list. Not a block; it means full Stage 2 depth, CI evidence required before approval, and a named sandboxed lane. Both are in the Stage 2 comment.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题: 是已观测到的问题,不是理论性加固——而且我从代码上静态确认了这个缺口。Windows bundled ConPTY backend (#11303) 这组测试原本只覆盖了守卫的 spawn 前 一侧:falls back to child_process when the bundled ConPTY spawn throws on Windows 直接 stub 掉 pty.spawn,所以从来没有任何用例在 ptySpawned = true 之后 抛错。也就是说,删掉这个标记,测试确实依然全绿,而命令可能先经 PTY 执行一次、再经 child_process fallback 重复执行。device-attributes 测试同样是同义反复——它把一个字面量喂给捕获到的 listener,再断言同一个字面量到达了 pty.write,只证明了转发器会转发,完全没有触及 xterm 的 parser。这些都是测试里的真实缺陷,你提供的 mutation 验证也印证了这一点。

方向: 对齐。这是 #11497#11313(均已合入)针对 #11303(仍然 open)的后续加固,方向已经确定,不存在产品取向问题。我跳过了外部 CHANGELOG 信号——对一个已上线 Windows 修复的内部测试质量改动,它没有参考价值。

规模: 触及核心路径(packages/core/src/services/**)。生产代码 49 行(shellExecutionService.ts +24/−25——主要是注释重写和三元表达式重新缩进;真正的逻辑改动只有一个上提的 const 加三处替换),测试 60 行(shellExecutionService.test.ts +43/−17),生成/schema 0 行。远低于 500 行,因此不需要维护者关注升级,也不触发大 PR 提示。

方案: 范围合理,两个新测试与已有测试形成了正确的配对。有两点值得考虑,都不是阻塞项:

风险: Stage 1e 命中——shellExecutionService.ts 在与 revert 相关的高风险路径列表里。这不是阻塞,但意味着 Stage 2 要走完整深度、批准前必须有 CI 证据,并且要指明沙箱验证通道。两者都写在 Stage 2 评论里。

进入代码审查 🔍

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

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

Before reading the diff I wrote down what I'd do from the description alone: add a win32 case where the PTY spawns fine and a later step throws, asserting child_process.spawn is never called; replace the synthetic-listener DA test with one that drives a real @xterm/headless Terminal and spies on pty.write, waiting asynchronously because that path is queued; and leave production code alone unless the platform check really is evaluated three times in one function. The PR lands on essentially that, including the async widening the second change forces. No Critical blockers and no AGENTS.md violations that block merge. What follows is what I actually verified, plus two Suggestions.

The new fallback test throws where it claims to, and its cleanup is load-bearing. Tracing the pid getter: the first read is this.activePtys.set(ptyProcess.pid, …) (line 1576), which succeeds and stores 12345; the next is return { pid: ptyProcess.pid, result } (line 2529), the last statement in the try. That read throws into the catch, ptySpawned is already true so the win32 fallback branch is skipped, and the message has no posix_spawnp failed, so the catch returns executionMethod: 'none' with pid: undefined — precisely the three assertions. Because the throw happens at the return, none of the teardown closures ever run, so the map entry set at 1576 really does leak and the finally { activePtys.delete(12345) } is required rather than defensive. That pairing with the existing pre-spawn test is what makes the guard pinned from both sides.

No cross-test pollution from the getter. This was my main isolation worry: the top-level beforeEach reassigns mockPtyProcess = new EventEmitter() and sets pid = 12345 as a plain data property, so Object.defineProperty(…, { configurable: true }) redefines a per-test-fresh object. Clean.

The DA test now traverses xterm's real parser and the real render queue. Dropping mockResolvedValueOnce({ Terminal: ReplyCapturingTerminal }) falls back to the beforeEach default mockLoadXtermHeadless.mockResolvedValue({ Terminal }) — the real headless Terminal. pty.onData.mock.calls[0][0] is the handler registered at line 1816, which calls handleOutput, which enqueues onto processingChain and eventually reaches headlessTerminal.write(decodedChunk, cb). So the reply is generated by xterm's parser and can only reach pty.write through the production forwarder. That path is queued behind a promise chain, which is why await vi.waitFor(…) is needed, which is why simulateExecution had to accept Promise<void> — the three edits are causally linked, not incidental. ReplyCapturingTerminal and capturedReplyListener are still used by the error-containment and platform-gate tests, so nothing is orphaned here.

The production change is behavior-preserving and I can name every consumer. useBundledConpty is a function-local const in executeWithPty, read at exactly three sites: useConptyDll, the queryResponseDisposable gate, and the catch's !ptySpawned && … branch. os.platform() cannot change within a process, so hoisting is identical. It is declared outside the try, before let ptySpawned, so the catch cannot hit a TDZ. Nothing outside the method observes it, and the public surface is untouched.

The corrected comment agrees with what already shipped. ConptyReleasePseudoConsole is not a new assertion — merged conpty-host.ts already documents that bundled ConPTY "releases its host reference immediately after spawn", so this aligns two comments instead of introducing a claim. The Bundled ConPTY (useBundledConpty above) cross-reference was updated correctly too.

Suggestions (non-blocking)

  1. The test's injection point is positional, and nothing says so. It pins "some error after spawn" by exploiting the fact that pid happens to be read twice, at 1576 and again at 2529. Add a pid read anywhere between those two and the throw moves — possibly into a closure where it surfaces differently, leaving the test green or red for a reason nobody intended. One line in the test naming why the pid getter is the injection point (first read at activePtys.set, second at the return, so it lands after ptySpawned = true) would keep that from drifting silently.
  2. The rewritten comment drops two things that were not wrong. It no longer says what the option does (load the conpty.dll node-pty ships instead of the inbox one) or that node-pty's typings mark it EXPERIMENTAL, and the [Windows] qwen-cli (VS Code Companion) leaks headless conhost.exe ConPTY processes - 347 processes / ~2.8 GB after ~12h uptime #11303 measurement provenance is gone entirely. AGENTS.md asks us not to delete existing comments as cleanup; only the conhost.exe/OpenConsole.exe claim needed correcting. With [Windows] qwen-cli (VS Code Companion) leaks headless conhost.exe ConPTY processes - 347 processes / ~2.8 GB after ~12h uptime #11303 still open and this PR explicitly scoping out real process counts, useConptyDll now sits on no current measurement while the comment states a verification requirement nobody has met — worth a follow-up on [Windows] qwen-cli (VS Code Companion) leaks headless conhost.exe ConPTY processes - 347 processes / ~2.8 GB after ~12h uptime #11303.
sequenceDiagram
    participant P1 as Test simulation
    participant P2 as PTY onData handler
    participant P3 as handleOutput
    participant P4 as processingChain
    participant P5 as headless xterm Terminal
    participant P6 as query reply forwarder
    P1->>P2: feed a DA1 query as PTY output
    P2->>P3: handleOutput(buffer)
    P3->>P4: enqueue the render step
    P4->>P5: headlessTerminal.write(chunk)
    P5->>P6: onData emits the generated reply
    P6->>P2: ptyProcess.write(reply)
    P1->>P1: vi.waitFor asserts the reply reached the PTY
Loading

Testing

This is an unattended CI run, so no PR code was built, run, or checked out — the gate is static. The evidence below is the PR's own CI, read through the API for the reviewed commit. I did not re-fetch after the initial read and did not poll; the finalize job rewrites the table once CI settles.

Green so far: Lint & Static (ubuntu-latest, Node 22.x) and Integration Tests (no-AK, No Sandbox), plus both Desktop Shell legs. Lint going green is worth noting because the diff removes usages of ReplyCapturingTerminal and capturedReplyListener from one test, which is exactly where an unused-symbol error would have surfaced. The decisive check — Test (ubuntu-latest, Node 22.x), the unit suite that runs the changed test file — is still in progress, so nothing here confirms 153/153.

Test (macos-latest) and Test (windows-latest) are skipped on this commit. The win32 branches are still exercised, because every test in this area reaches them through mockPlatform rather than the host OS — but that means mocked-platform coverage only. Real packaged Windows execution stays untested, which the PR's own Risk section already says.

Not verified: the literal \x1b[?1;2c. This runner has no node_modules, so I could not read the installed @xterm/headless bundle to confirm that is its DA1 reply. packages/core/package.json pins it to exactly 5.5.0 rather than a range, so the expectation can only move on a deliberate bump — and the pending ubuntu test leg is what settles it.

Not verified: the mutation results in the description. Those are the author's claim, not something I re-ran, and the gate cannot execute PR code to check them.

Sandboxed verification would settle this: @qwen-code /verify — the entire point of the PR is that these two tests are mutation-sensitive, and a green suite structurally cannot show that, because it passes identically with the ptySpawned marker present, which is the only state CI ever tests. An A/B run that drops ptySpawned = true and confirms the fallback test goes red with one child_process spawn, then separately disables the Windows forwarder and confirms the DA test goes red with zero PTY writes, is exactly the load-bearing proof that is missing here. /tmux is not the right lane — there is no TUI surface in this diff.

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

Check Conclusion
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
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,失败项排在最前。

中文说明

代码审查

在读 diff 之前,我先只根据描述写下了自己会怎么做:加一个 win32 用例,让 PTY 正常 spawn、但之后的某一步抛错,断言 child_process.spawn 从未被调用;把合成 listener 的 DA 测试换成驱动真实 @xterm/headless Terminal 并 spy pty.write 的写法,而且要异步等待,因为这条路径是排队执行的;除非平台判断真的在一个函数里被算了三次,否则不动生产代码。这个 PR 基本就是这么做的,也包括了第二项改动必然带来的异步化。没有 Critical 阻塞项,也没有阻塞合并的 AGENTS.md 违规。 下面是我实际验证过的内容,以及两条 Suggestion。

新的 fallback 测试确实在它声称的位置抛错,而且它的清理是必需的。 追踪 pid getter:第一次读发生在 this.activePtys.set(ptyProcess.pid, …)(1576 行),成功并存入 12345;下一次读在 return { pid: ptyProcess.pid, result }(2529 行),也就是 try 里的最后一条语句。这次读抛错进入 catch,此时 ptySpawned 已经为 true,所以跳过 win32 fallback 分支,而错误信息里没有 posix_spawnp failed,于是 catch 返回 executionMethod: 'none'pid: undefined——正好是那三条断言。因为抛错发生在 return,所有 teardown 闭包都不会执行,1576 行存入的 map 条目确实会泄漏,所以 finally { activePtys.delete(12345) } 是必需的,而不是防御性冗余。它与已有的 spawn 前测试配对,才让守卫两侧都被钉住。

getter 不会污染其他测试。 这是我最担心的隔离问题:顶层 beforeEach 会重新赋值 mockPtyProcess = new EventEmitter(),并把 pid = 12345 设成普通数据属性,所以 Object.defineProperty(…, { configurable: true }) 重定义的是一个每个测试都全新的对象。干净。

DA 测试现在真正穿过了 xterm 的 parser 和真实的渲染队列。 去掉 mockResolvedValueOnce({ Terminal: ReplyCapturingTerminal }) 后,会回落到 beforeEach 的默认值 mockLoadXtermHeadless.mockResolvedValue({ Terminal })——真实的 headless Terminal。pty.onData.mock.calls[0][0] 是 1816 行注册的那个 handler,它调用 handleOutput,后者把任务排进 processingChain,最终到达 headlessTerminal.write(decodedChunk, cb)。所以应答是由 xterm 的 parser 生成的,也只能经由生产代码里的转发器到达 pty.write。这条路径排在 promise 链后面,因此必须用 await vi.waitFor(…),也因此 simulateExecution 必须接受 Promise<void>——这三处改动是因果相连的,不是顺手改的。ReplyCapturingTerminalcapturedReplyListener 仍被 error-containment 与 platform-gate 测试使用,所以这里没有产生孤立符号。

生产代码改动保持行为不变,而且我能点名每一个消费方。 useBundledConptyexecuteWithPty 里的函数局部 const,恰好被三处读取:useConptyDllqueryResponseDisposable 的判断、以及 catch 里的 !ptySpawned && … 分支。os.platform() 在进程内不会变化,所以只算一次完全等价。它声明在 try 之外let ptySpawned 之前,因此 catch 不会遇到 TDZ。方法之外观察不到它,公开接口也没动。

修正后的注释与已经合入的代码一致。 ConptyReleasePseudoConsole 不是新断言——已合入的 conpty-host.ts 早就写明 bundled ConPTY "releases its host reference immediately after spawn",所以这是让两处注释对齐,而不是引入一个新说法。Bundled ConPTY (useBundledConpty above) 这个交叉引用也改对了。

Suggestion(非阻塞)

  1. 测试的注入点是位置性的,但没有任何说明。 它钉住的是"spawn 之后的某个错误",靠的是 pid 恰好在 1576 和 2529 被读了两次这一事实。只要有人在这两行之间新增一次 pid 读取,抛错点就会移动——有可能移进某个闭包,导致异常以不同方式浮现,让测试因为没人预期的原因变绿或变红。在测试里加一行说明为什么用 pid getter 作为注入点(第一次读在 activePtys.set,第二次在 return,所以它落在 ptySpawned = true 之后),可以避免这种悄悄漂移。
  2. 重写后的注释删掉了两条本来没有错的信息。 它不再说明这个选项做什么(加载 node-pty 自带的 conpty.dll,而不是系统内置的那个),也不再提 node-pty 的类型定义把它标记为 EXPERIMENTAL,[Windows] qwen-cli (VS Code Companion) leaks headless conhost.exe ConPTY processes - 347 processes / ~2.8 GB after ~12h uptime #11303 的测量出处也完全消失了。AGENTS.md 要求不要把删除已有注释当作顺手清理,而这次只有 conhost.exe/OpenConsole.exe 那句需要修正。在 [Windows] qwen-cli (VS Code Companion) leaks headless conhost.exe ConPTY processes - 347 processes / ~2.8 GB after ~12h uptime #11303 仍然 open、本 PR 又明确把真实进程计数划出范围的情况下,useConptyDll 现在没有任何当前测量支撑,注释却提出了一个还没人满足的验证要求——这更适合在 [Windows] qwen-cli (VS Code Companion) leaks headless conhost.exe ConPTY processes - 347 processes / ~2.8 GB after ~12h uptime #11303 上跟进。

测试

这是一次无人值守的 CI 运行,因此没有构建、运行或 checkout 任何 PR 代码——审查是静态的。下面的证据来自 PR 自身的 CI,通过 API 读取所审查的那个 commit。我只读取了一次、没有轮询;CI 稳定后 finalize 任务会就地重写这张表。

目前已绿:Lint & Static (ubuntu-latest, Node 22.x)Integration Tests (no-AK, No Sandbox),以及两条 Desktop Shell。Lint 变绿值得一提,因为这个 diff 从某个测试里移除ReplyCapturingTerminalcapturedReplyListener 的使用,这正是未使用符号错误最可能出现的地方。关键的那条——Test (ubuntu-latest, Node 22.x),也就是运行被改测试文件的单元测试套件——仍在进行中,所以这里没有任何东西能确认 153/153。

Test (macos-latest)Test (windows-latest) 在这个 commit 上是 skipped。win32 分支仍然被覆盖,因为该区域的每个测试都是通过 mockPlatform 而非宿主系统进入这些分支的——但那意味着只有 mock 平台的覆盖。真实的打包后 Windows 执行仍未被测试,PR 自己的 Risk 部分也已经说明了这点。

未验证:字面量 \x1b[?1;2c。这台 runner 没有 node_modules,所以我无法读取已安装的 @xterm/headless 产物来确认这就是它的 DA1 应答。packages/core/package.json 把它精确锁定在 5.5.0(不是范围),所以这个期望值只有在主动升级时才可能变化——而正在进行的那条 ubuntu 测试就是它的判定依据。

未验证:描述里的 mutation 结果。那是作者的声明,不是我重新跑出来的,而且审查门禁不能执行 PR 代码来核对。

沙箱验证可以解决这一点:@qwen-code /verify——这个 PR 的全部意义在于这两个测试对 mutation 敏感,而绿色的套件在结构上无法证明这一点,因为它在 ptySpawned 标记存在时同样通过,而那正是 CI 唯一会测到的状态。一次 A/B 运行:先去掉 ptySpawned = true,确认 fallback 测试因出现一次 child_process spawn 而变红;再单独关掉 Windows 转发器,确认 DA 测试因 PTY write 次数为 0 而变红——这正是这里缺失的 load-bearing 证明。/tmux 不是合适的通道,这个 diff 里没有 TUI 界面。

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — the coverage gap was real and I could confirm it statically; the two Suggestions are about comment content and test legibility, not correctness.

Going back to what I proposed before reading the diff: this matches it, and beats it in one place. I would not have thought to clean up activePtys in a finally, because I hadn't yet worked out where the injected throw actually lands. Once traced, it's obvious and necessary — the throw happens at the return statement, past every teardown closure, so without that finally the test would leak a live PTY entry into the rest of the file. That's the detail that tells me the author traced the path rather than writing a plausible-looking test.

On the question the gate is supposed to ask first — does the problem exist — I'm satisfied, and not on the PR's word. Before this change the #11303 suite stubbed pty.spawn itself to throw, so it only ever exercised ptySpawned === false. Nothing raised after the PTY came up, which means the marker could be deleted with a green suite while a command ran twice on Windows. The DA test was worse than weak, it was circular: hand a literal to the captured listener, assert the same literal arrived at pty.write. Both are defects in tests protecting a fix that is already merged, for an issue (#11303, 347 leaked hosts / ~2.8 GB) that is still open. Hardening those tests is worth merging on its own merits.

What I can't confirm is that the hardening holds. A green suite is structurally incapable of showing mutation-sensitivity — it passes the same way with the guard in place, which is the only state CI ever runs. The mutation numbers in the description are the author's claim and I did not re-run them, and I could not check the \x1b[?1;2c literal either, because this runner has no node_modules. The exact pin at @xterm/headless 5.5.0 makes that expectation stable, and the pending ubuntu leg settles it. None of that is a reason to hold the PR; it's a reason to say plainly what is still open.

In six months I'd thank whoever wrote this rather than curse them. The three edits in the test helper, the DA test, and the simulation signature are causally linked and each one is forced by the one before, which is what a small honest diff looks like. My two reservations stay where I put them in Stage 2: the injection point is positional with nothing documenting it, and the rewritten comment gave up two true statements (what useConptyDll does, and that node-pty marks it EXPERIMENTAL) plus the #11303 measurement provenance while only the process-name claim needed fixing. Both are follow-ups, not blockers.

shellExecutionService.ts is on the revert-correlated path list, so I'm not approving on review alone — approval is deferred until CI lands green on 6a8f25027a9ccd1c57b597b5cb8e16a7163c7ef7, with the unit-test leg as the check that matters. If it lands red, or the head moves, no approval posts. Given the diff is Windows-gated through mockPlatform and both non-Linux test legs are skipped on this commit, a maintainer may also want the A/B run named in Stage 2 before merging — that's a judgement call, not a gate.

中文说明

Confidence: 4/5 —— 覆盖缺口是真实存在的,而且我能静态确认;两条 Suggestion 针对的是注释内容和测试可读性,不是正确性。

回到我在读 diff 之前写下的方案:这个 PR 与它一致,并且在一处做得更好。我本来想不到要在 finally 里清理 activePtys,因为那时我还没弄清注入的异常究竟落在哪里。追踪之后就清楚了,而且这一步是必需的——异常发生在 return 语句处,已经越过了所有 teardown 闭包,所以没有这个 finally,测试就会把一个活的 PTY 条目泄漏给文件里剩下的用例。正是这个细节说明作者是追踪过这条路径的,而不是写了一个看起来合理的测试。

关于门禁最该先问的问题——问题是否真实存在——我是信服的,而且不是凭 PR 的说法。在这次改动之前,#11303 那组测试是通过 stub pty.spawn 本身来抛错的,所以它只覆盖到 ptySpawned === false。从来没有任何用例在 PTY 起来之后抛错,这意味着在 Windows 上命令可能执行两次的情况下,删掉这个标记测试依然全绿。DA 测试比"弱"更糟,它是循环论证:把一个字面量交给捕获到的 listener,再断言同一个字面量到达了 pty.write。这两处都是测试本身的缺陷,而它们保护的修复已经合入,对应的 issue(#11303,347 个泄漏进程 / 约 2.8 GB)却仍然 open。加固这些测试本身就有合并价值。

我无法确认的是这份加固是否真的有效。绿色的套件在结构上就无法证明 mutation 敏感性——在守卫存在时它同样通过,而那正是 CI 唯一会运行的状态。描述里的 mutation 数据是作者的声明,我没有重跑;\x1b[?1;2c 这个字面量我也无法核对,因为这台 runner 没有 node_modules@xterm/headless 精确锁定在 5.5.0 让这个期望值是稳定的,而正在进行的 ubuntu 测试会给出判定。这些都不是扣住 PR 的理由,只是需要如实说明哪些部分仍未落定。

六个月后我会感谢写这段代码的人,而不是埋怨他。测试 helper、DA 测试和 simulation 签名这三处改动是因果相连的,每一处都是被前一处逼出来的——这就是一个小而诚实的 diff 该有的样子。我的两点保留意见仍放在 Stage 2:注入点是位置性的却没有任何说明;重写后的注释放弃了两句本来正确的话(useConptyDll 究竟做什么,以及 node-pty 把它标记为 EXPERIMENTAL)外加 #11303 的测量出处,而其实只有进程名那一句需要修正。两者都是后续跟进项,不是阻塞项。

shellExecutionService.ts 在与 revert 相关的高风险路径列表里,所以我不会只凭审查就批准——批准将推迟到 CI 在 6a8f25027a9ccd1c57b597b5cb8e16a7163c7ef7 上全绿之后,其中真正关键的是单元测试那一条。如果它变红,或者 head 发生移动,就不会发出批准。鉴于这个 diff 是通过 mockPlatform 做 Windows 门禁的,而本 commit 上两条非 Linux 测试都是 skipped,维护者在合并前可能还想要 Stage 2 里点名的那次 A/B 运行——这是一个判断,不是门禁要求。

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

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

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship — CI landed green after the review. ✅

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking findings.
Approval blockers: none.

Triage: Standard — test hardening + comment correction with a small source refactoring.

What I checked:

  1. Single source of truth for Windows bundled ConPTY decision: const useBundledConpty = os.platform() === 'win32' extracted once, replacing three separate os.platform() === 'win32' checks. The three consumers (useConptyDll option, terminal query reply forwarding, post-spawn fallback guard) are causally linked — they must agree on whether bundled ConPTY is in use. A local variable at method top prevents drift. The variable is named after its semantic meaning, not the platform it tests.

  2. Comment accuracy for useConptyDll: Updated to name the actual mechanism (ConptyReleasePseudoConsole), correct the process census (OpenConsole.exe is what bundled ConPTY launches, not conhost.exe), and provide guidance ("count both conhost.exe and OpenConsole.exe or all attributable children"). Removed specific #11303 measurements (version numbers, before/after counts) that belong in that issue's thread.

  3. New fallback test (does not run the fallback after the PTY has already spawned):

    • Uses a getter on pid that succeeds on first read (for activePtys.set at line ~1570) but throws on second read (the return { pid: ptyProcess.pid, result } after the try/catch).
    • This triggers the catch block with ptySpawned === true, exercising the else-branch that returns executionMethod: 'none'.
    • Verifies: mockPtySpawn called once, mockCpSpawn NOT called (no double-execution), error message propagated.
    • finally block cleans up activePtys.delete(12345) — the PTY was registered before the throw.
    • The PR description confirms mutation: removing the post-spawn guard makes this test fail with one child_process spawn.
  4. Rewritten terminal reply test (writes xterm's device-attributes reply back to the PTY on Windows):

    • No longer uses ReplyCapturingTerminal (removed mockLoadXtermHeadless.mockResolvedValueOnce). Uses the real @xterm/headless Terminal.
    • Sends \x1b[c (CSI c — Device Attributes query) via pty.onData, waits for \x1b[?1;2c (the DA reply xterm actually generates) to be written back via ptyProcess.write.
    • vi.waitFor handles the async parser→forwarder→write chain.
    • The old test supplied \x1b[?64;1;22c directly to the listener — a response xterm does not actually generate. The new test catches parser/protocol integration regressions.
    • PR description confirms mutation: disabling the forwarder makes this test fail with zero PTY writes.
  5. simulateExecution async callback support: simulation: (pty, ac) => void | Promise<void> + await simulation(...). Backwards-compatible — existing sync callbacks still work. Required by the new terminal reply test's vi.waitFor.

  6. Cross-check: qwen-code-ci-bot approved. No prior review from me. Follows up on #11497 and #11313 (the ConPTY host lifecycle work reviewed in prior rounds).

Unreviewed dimensions: actual Windows runtime behavior (no Windows host available; PR description marks Windows as ⚠️).

Reviewed with AI assistance.

@wenshao

wenshao commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Runtime verification — PR 11555

I rebuilt this locally as a real runtime environment (not a re-read of the diff) and ran the PR head (6a8f2502) against its own parent (0ef35351) as two independent worktrees. Every claim in the PR description reproduces. Verdict: LGTM — safe to merge; two optional follow-ups below, neither a blocker.

Environment: Linux x86_64, Node 22.22.2, Vitest 3.2.7, @xterm/headless 5.5.0, @lydell/node-pty 1.2.0-beta.10 (linux-x64 prebuild), real compiled packages/core/dist.

Summary

# Claim under test Method Result
1 Focused suite is 153/153 (main: 152) run both worktrees, 5× repeats ✅ 153/153, 5/5 runs, no flake; main 152/152
2 "removing the post-spawn guard still left all 152 tests green" delete ptySpawned = true;, re-run the whole suite on both arms ✅ main GREEN, PR RED — the gap was real and is now closed
3 The guard prevents a real double execution real PTY, real command, synchronous post-spawn failure injected into dist ✅ 1 execution with the marker, 2 executions under 2 different pids without it
4 The old reply test could not see a parser/protocol regression strip the DA query before it reaches xterm's parser ✅ main GREEN, PR RED — exactly the blind spot described
5 The old test asserted a reply the installed xterm never generates drive real @xterm/headless 5.5.0 ESC[cESC[?1;2c; ESC[?64;1;22c produces no reply
6 The forwarder is what saves the ~2 s stall real node-pty + real xterm on Linux, gate forced on/off in dist ✅ 2063 ms + no reply → 94 ms + ESC[?1;2c
7 ConptyReleasePseudoConsole / OpenConsole.exe comment correction inspect the shipped @lydell/node-pty-win32-x64 1.2.0-beta.10 artifacts ✅ both substantiated (details below)
8 "Off Windows the option is inert" grep the POSIX prebuild + a real Linux spawn with useConptyDll: true ✅ 0 occurrences in lib/*.js and pty.node; spawn succeeds, exitCode 0

1. Mutation matrix — teeth, and the gap on main

Each cell re-runs the whole shellExecutionService suite against a surgically mutated production file.

mutation matrix

  • M1 (ptySpawned = true; removed): main stays green at 152/152; the PR fails with expected "spy" to not be called at all, but actually been called 1 times — one child_process.spawn of the same command. This is the headline claim, reproduced exactly.
  • M3 (headlessTerminal.write(decodedChunk) → the DA query stripped before the parser): main stays green; the PR fails with Number of calls: 0. This is the regression class the rewritten reply test now covers and the old one structurally could not, because it invoked the forwarder's listener by hand.
  • M2 (forwarder deleted outright) is a control: both arms already caught that one.
  • M5 is a surviving mutant on both arms — see follow-up (a).

2. What the post-spawn guard is actually worth

Real PTY execution of echo RUN-$$ >> marker, with a synchronous post-spawn failure injected into the compiled dist (the guard's real trigger shape):

double execution proof

With the marker present: executionMethod: "none", one line in the marker file. With it removed: executionMethod: "child_process" and two lines, written by two different pids. For a tool-executed shell command that is a duplicated side effect, so the new test is guarding something with real consequences — not just a code path.

3. The reply forwarder, end to end on a real PTY

The Windows-only gate is the only platform-specific part of the mechanism, so I forced useBundledConpty on in the compiled dist and ran the real service against a real @lydell/node-pty child that sends a primary Device-Attributes query and waits for the answer:

DA forwarder e2e

Forwarder off: the child times out after 2005 ms (2063 ms wall). Forwarder on: it gets ESC[?1;2c in ~10 ms (94 ms wall). That is the exact byte string the rewritten test asserts, produced by the real parser through the real production code path — so the new assertion is not merely self-consistent, it matches what the shipped stack does. It also reproduces the ~2 s stall story from #11497 on a platform where it can be measured.

4. The two corrected comment claims

claims forensics

  • @lydell/node-pty-win32-x64@1.2.0-beta.10 ships prebuilds/win32-x64/conpty/conpty.dll and prebuilds/win32-x64/conpty/OpenConsole.exe; the bundled conpty.dll carries UTF-16 references to both OpenConsole.exe and \conhost.exe. A conhost.exe-only census under the bundled backend is therefore an under-count — the correction is right.
  • That same bundled conpty.dll exports ConptyReleasePseudoConsole (alongside ConptyClose/Create/Resize/...), and the native addon conpty.node resolves that symbol by name. The mechanism the new comment names is real and specific to the bundled DLL.
  • The POSIX prebuild contains zero occurrences of useConptyDll in lib/*.js and in prebuilds/linux-x64/pty.node, and a real Linux spawn with useConptyDll: true succeeds with exitCode 0 — "inert off Windows" holds.

5. Regression surface

  • shellExecutionService.test.ts: 153/153, 5 consecutive runs, no flake (main: 152/152).
  • Dependent suites src/tools/shell.test.ts + src/tools/shell.backgroundStatus.test.ts: 327/327.
  • Full packages/core suite on the PR head: 24543 passed / 3 failed — the 3 failures (session-writer-lease, git-branches, skill-curator) plus the client.test.ts mock error reproduce identically on the parent commit; they are environment-related (running as root, local git state), not attributable to this PR.
  • Typecheck: the PR-head and parent builds emit the same 42 pre-existing TS errors, no new ones.
  • ESLint / Prettier on the two changed files report the same pre-existing findings on both arms (2 vitest/no-conditional-expect errors at the untouched parametrized abort test; formatting drift from a newer local Prettier 3.9.6), so nothing here is introduced by the PR.
  • The useBundledConpty hoist is semantically identical: os.platform() is stable within a call, and no test flips the mocked platform between execute() and the return. Three of the six os.platform() calls in executeWithPty are now shared; the other three belong to unrelated decisions and are correctly left alone.

Optional follow-ups (non-blocking)

(a) The platform half of the same guard is still uncovered. Mutating if (!ptySpawned && useBundledConpty)if (!ptySpawned) leaves both arms green (M5 above). Today a POSIX spawn-phase throw whose message lacks posix_spawnp failed resolves as executionMethod: 'none', and dropping the gate would silently turn it into a child_process fallback with no test noticing. One POSIX-side case would close the other half of the condition this PR hardens.

(b) A post-spawn failure orphans the child it just started. Probing the same injected failure with sleep 30, after execute() returns executionMethod: 'none' the PTY child is still alive (/proc/<pid>/cmdline = sleep 30) and its activePtys entry is still in the registry — which is precisely why the new test needs finally { ShellExecutionService['activePtys'].delete(12345) }. That is pre-existing behavior, correctly out of scope here, but the cleanup line in the test is a symptom worth a follow-up (kill the PTY / dispose the terminal before returning 'none').

(c) Cosmetic: the new test's pidReads === 1 couples to how many times ptyProcess.pid is read between spawn and return (today: activePtys.set, then the returned handle). It fails safe — an earlier throw still yields 'none' with no fallback — but a future refactor that adds a pid read would silently move which post-spawn point is being simulated.

Not verified

Real Windows runtime: no Windows host here, so packaged Windows execution and an actual conhost.exe/OpenConsole.exe process census remain unverified — matching this PR's own "Tested on" table. The Linux runs above exercise the mechanism by forcing the platform decision, not the Windows host lifecycle itself.

中文版本(点击展开)

本地运行时验证 — PR 11555

我在本地搭建了真实运行环境(不是重读 diff),把 PR head(6a8f2502)与其父提交(0ef35351)作为两个独立 worktree 做 A/B。PR 描述中的每一条声明都复现成功。 结论:LGTM,可以合并;下面 2 条是可选后续,均不阻塞。

环境: Linux x86_64、Node 22.22.2、Vitest 3.2.7、@xterm/headless 5.5.0、@lydell/node-pty 1.2.0-beta.10(linux-x64 预编译包)、真实编译产物 packages/core/dist

结论汇总

# 待验证声明 方法 结果
1 定向测试 153/153(main 为 152) 两个 worktree 各跑 5 次 ✅ 153/153,5/5 稳定无 flake;main 152/152
2 「删掉 post-spawn 守卫后 152 个测试仍全绿」 删除 ptySpawned = true; 后整套重跑 ✅ main 绿,PR — 缺口真实存在,现已补上
3 守卫确实阻止了真实的重复执行 真实 PTY + 真实命令,在 dist 注入 post-spawn 同步失败 ✅ 有标记位时执行 1 次,去掉后两个不同 pid 执行了 2 次
4 旧的应答测试发现不了 parser/protocol 回归 在数据进入 xterm parser 前把 DA 查询剥掉 ✅ main 绿,PR — 正是所描述的盲区
5 旧测试断言的应答当前 xterm 根本不会生成 直接驱动真实 @xterm/headless 5.5.0 ESC[cESC[?1;2cESC[?64;1;22c 不产生任何应答
6 转发器才是消除 ~2s 卡顿的关键 真实 node-pty + 真实 xterm,在 dist 里开关平台判断 ✅ 2063 ms 无应答 → 94 ms 且收到 ESC[?1;2c
7 ConptyReleasePseudoConsole / OpenConsole.exe 注释纠正 直接检查已发布的 @lydell/node-pty-win32-x64 1.2.0-beta.10 制品 ✅ 两点均被证实
8 「Windows 之外该选项是惰性的」 检索 POSIX 预编译包 + 在 Linux 上用 useConptyDll: true 真实 spawn lib/*.jspty.node 中 0 处出现;spawn 成功,exitCode 0

1. Mutation 矩阵 —— 测试有没有牙齿,main 上的缺口是否真实

每一格都是把生产代码做定点变异后,重跑整套 shellExecutionService 测试。

  • M1(删除 ptySpawned = true;):main 依旧 152/152 全绿;PR 报错 expected "spy" to not be called at all, but actually been called 1 times,即同一条命令被 child_process.spawn 又执行了一次。核心声明精确复现。
  • M3headlessTerminal.write(decodedChunk) 前把 DA 查询剥掉):main 全绿;PR 报 Number of calls: 0。这正是重写后的应答测试能覆盖、而旧测试(手工调用转发 listener)结构上无法覆盖的回归类型。
  • M2(整块删掉转发器)是对照组:两侧原本就能发现。
  • M5 在两侧都存活,见后续建议 (a)。

2. post-spawn 守卫到底价值几何

真实 PTY 执行 echo RUN-$$ >> marker,并在编译产物中注入 post-spawn 同步失败(守卫真正的触发形态):保留标记位时 executionMethod: "none"、marker 文件 1 行;去掉后 executionMethod: "child_process"、marker 2 行且来自两个不同 pid。对于工具执行的 shell 命令,这就是一次重复的副作用——所以新测试守的是真实后果,而不只是一条代码路径。

3. 真实 PTY 上的端到端应答链路

该机制中唯一与平台相关的只有那个 Windows 判断,因此我在编译产物里强制打开 useBundledConpty,用真实 @lydell/node-pty 子进程发送 primary Device-Attributes 查询并等待应答:关闭转发器时子进程 2005 ms 超时(整体 2063 ms);开启后约 10 ms 收到 ESC[?1;2c(整体 94 ms)。这正是重写后测试断言的字节串,由真实 parser 经真实生产路径产生——说明新断言不只是自洽,而是与实际发布的行为一致,同时也在可测量的平台上复现了 #11497 提到的 ~2s 卡顿。

4. 两条被纠正的注释

  • @lydell/node-pty-win32-x64@1.2.0-beta.10 同时打包了 conpty/conpty.dll conpty/OpenConsole.exe;该 conpty.dll 内含 OpenConsole.exe\conhost.exe 两个 UTF-16 字符串。因此在 bundled 后端下只统计 conhost.exe 确实会少算——纠正是正确的。
  • 同一个 bundled conpty.dll 导出了 ConptyReleasePseudoConsole(与 ConptyClose/Create/Resize/... 并列),原生插件 conpty.node 也按名解析该符号。新注释所述机制真实存在,且确属 bundled DLL 特有。
  • POSIX 预编译包的 lib/*.jsprebuilds/linux-x64/pty.nodeuseConptyDll 出现 0 次,且 Linux 上带 useConptyDll: true 的真实 spawn 正常返回 exitCode 0——「Windows 之外惰性」成立。

5. 回归面

  • shellExecutionService.test.ts153/153,连跑 5 次无 flake(main:152/152)。
  • 关联套件 src/tools/shell.test.ts + src/tools/shell.backgroundStatus.test.ts327/327
  • PR head 上的 packages/core 全量:24543 通过 / 3 失败——这 3 个失败(session-writer-leasegit-branchesskill-curator)以及 client.test.ts 的 mock 报错在父提交上完全一致地复现,属于环境因素(root 身份、本地 git 状态),与本 PR 无关。
  • 类型检查:PR head 与父提交产生同样的 42 个既有 TS 报错,没有新增。
  • 两个改动文件上的 ESLint / Prettier 在两侧给出相同的既有结论(未改动的参数化 abort 测试里 2 处 vitest/no-conditional-expect;本地较新的 Prettier 3.9.6 造成的格式漂移),均非本 PR 引入。
  • useBundledConpty 提取在语义上完全等价:单次调用内 os.platform() 稳定,且没有任何测试会在 execute() 执行途中翻转被 mock 的平台。executeWithPty 中 6 处 os.platform() 里的 3 处被合并,另外 3 处属于无关判断,保持原样是正确的。

可选后续(不阻塞)

(a) 同一守卫的平台判断部分仍未被覆盖。if (!ptySpawned && useBundledConpty) 改成 if (!ptySpawned) 后,两侧都保持全绿(上面的 M5)。当前在 POSIX 上,spawn 阶段抛出的、消息中不含 posix_spawnp failed 的错误会返回 executionMethod: 'none';去掉平台判断会悄悄变成走 child_process 回退,而没有任何测试会发现。补一个 POSIX 侧用例即可闭合这个条件的另一半。

(b) post-spawn 失败会遗留刚启动的子进程。sleep 30 复测同样的注入失败:execute() 返回 executionMethod: 'none' 之后,PTY 子进程仍然存活(/proc/<pid>/cmdline = sleep 30),其 activePtys 注册项也还在——这正是新测试需要 finally { ShellExecutionService['activePtys'].delete(12345) } 的原因。这是既有行为,本 PR 不处理是合理的,但测试里的这行清理是一个值得后续跟进的症状(在返回 'none' 前 kill PTY / dispose terminal)。

(c) 细节: 新测试的 pidReads === 1 与「spawn 到 return 之间读取 ptyProcess.pid 的次数」耦合(目前是 activePtys.set 与返回句柄两次)。它是安全失败的——更早抛出同样得到 'none' 且不走回退——但将来若有重构增加一次 pid 读取,被模拟的 post-spawn 位置会悄悄改变。

未验证部分

真实 Windows 运行时:本机没有 Windows 环境,因此打包后的 Windows 执行以及真实的 conhost.exe/OpenConsole.exe 进程计数仍未验证,这与本 PR 自己的「测试环境」表格一致。上面的 Linux 实验是通过强制平台判断来验证机制本身,并不覆盖 Windows host 的生命周期。

@yiliang114
yiliang114 added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit 424e40c Sep 10, 2026
128 of 129 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.3.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants