fix(core): run web terminal PTYs on the bundled ConPTY backend - #11643
Conversation
The web terminal spawned its PTYs with node-pty's default inbox ConPTY backend, where a natural shell exit orphans the conhost.exe --headless the backend spawned: the native exit watcher erases the pty baton before delivering onExit, pty_baton has no destructor, and the erased HPCON is unreachable from JS (microsoft/node-pty#965). Every exited web terminal leaked ~8 MB for the life of the CLI, and a pin bump does not fix it. Mirror the #11497 shell path: spawn with useConptyDll on Windows so the bundled backend releases its host reference right after spawn and the host exits with its last client. The bundled backend adds one synchronous throw point — a missing or unloadable conpty.dll — and a web terminal has no child_process fallback, so a failed bundled spawn retries once on the inbox backend: the pre-fix leaking behavior beats a terminal that cannot start at all. A failed spawn produced no child, so the retry cannot double-spawn. The 4001 non-retryable close path is unchanged. Also switch the agent-view PTY host to the bundled backend (same leak, lower rate), refresh the conpty-host release docs, and add a VERIFIED_NODE_PTY pin test so a node-pty bump forces re-verification of the native teardown semantics the release path depends on. Refs #11352
5cb5f11 to
cce1866
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Thanks — re-running the gate at Template looks good ✓ (all nine headings present, and the two design docs carry reciprocal links with matching section order in both languages). Problem: observed and measured, not theoretical. #11352 is open as Direction: aligned. #11497 (merged) already moved the shell tool onto bundled ConPTY; this brings the last two spawn sites — the web-terminal registry and the agent-view PTY host — onto the same backend, so Windows PTY creation follows one policy instead of two. Upstream CHANGELOG has no direct conhost/ConPTY entry, but Windows terminal-process lifecycle is a recurring fix area there: neutral-to-supportive rather than a precedent. Size: core paths are touched ( Worth saying out loud because it cuts against the usual direction: core-side production logic shrank since the last pass ( Approach: the scope feels right, and this round it got smaller in the place that mattered. My independent baseline before reading the diff was: mirror #11497 at the two remaining spawn sites, reuse That is the right call and it is not just tidier. The filter approach had produced the standing R2-1 Critical and the Two things I checked rather than assumed, since both were raised earlier in this thread:
What I would push back on is round count, not scope: this is the sixth round. Per AGENTS.md, from here only Critical fixes should land — and I did not find one at this head, so the five Suggestions in my Stage 2 notes belong in a follow-up issue rather than another widening of this diff. Risk: no elevated risk signals — none of the changed files match the revert-correlated paths ( Moving on to code review. 🔍 中文说明感谢贡献 —— 在 模板完整 ✓（九个标题齐全；两份设计文档带有互链，两种语言的章节顺序一致）。 问题： 已观测且已测量，不是理论性加固。#11352 以 方向： 对齐。#11497（已合并）已把 shell 工具切到 bundled ConPTY；本 PR 把最后两个 spawn 点 —— web-terminal registry 与 agent-view PTY host —— 统一到同一后端，使 Windows 上的 PTY 创建只遵循一套策略。上游 CHANGELOG 没有 conhost/ConPTY 的直接条目，但 Windows 终端进程生命周期在该文件里是反复出现的修复领域：属于中性偏支持，而非既有先例。 规模： 触及核心路径（ 有一点值得明说，因为它与通常的方向相反：核心侧生产逻辑相比上一轮减少了（ 方案： 范围合理，而且这一轮在关键处变小了。我在读 diff 之前的独立方案是：在剩下两个 spawn 点复刻 #11497、复用 这是正确的选择，而且不只是更整洁。过滤器方案产生了仍然挂着的 R2-1 Critical，以及 有两点我是去核对的、而不是想当然的，因为它们在本帖里都被提过：
我要提出异议的是轮次，而不是范围：这已经是第六轮。按 AGENTS.md，从这里开始只应落地 Critical 修复 —— 而我在当前 head 上没有发现 Critical，所以 Stage 2 里的五条 Suggestion 应该开成后续 issue，而不是再次扩大这个 diff。 风险： 无升级风险信号 —— 改动文件都不匹配与 revert 相关的路径（ 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code review at
|
| File | What changed |
|---|---|
packages/core/src/services/web-terminal-registry.ts |
Spawns with bundled ConPTY on win32 plus one inbox retry; adds a headless responder that forwards only primary DA; keeps the buffered stream byte-exact; disposes the responder on every teardown path |
packages/core/src/services/web-terminal-registry.test.ts |
Nine new cases on the real @xterm/headless: backend choice per platform, the retry, byte preservation across split escapes, DA-only answering, the no-responder fallback, and both cancellation windows during the headless load |
packages/core/src/services/conpty-host.ts |
Comments only — the doc previously claimed web-terminal and agent-view PTYs still use the inbox backend, which this PR makes false |
packages/core/src/services/conpty-host.test.ts |
New file. Pins the six @lydell/node-pty versions core and root declare, and drives the release through each WindowsPtyAgent internal shape |
packages/cli/src/agent-view/pty-host.ts |
Five lines: useConptyDll becomes a required spawn option, set from process.platform at the single construction site |
packages/cli/src/agent-view/pty-host.test.ts |
Asserts the option reaches the underlying spawn |
packages/cli/src/serve/routes/terminal.ts |
Requires replay=1 after the release check, and emits a NUL-prefixed snapshot control frame carrying replay and handlesPrimaryDa immediately before the binary history |
packages/cli/src/serve/routes/terminal.test.ts |
Pins the frame's exact bytes and position on both the fresh and reconnect paths, and that a legacy client is closed before any PTY is created |
packages/web-shell/client/components/terminal/TerminalPanel.tsx |
Sends replay=1; restores a marked snapshot into a second terminal on a detached host with no input forwarder, keeps the old one accepting keystrokes, then swaps on the write callback; rejects an unmarked snapshot from an older daemon |
packages/web-shell/client/components/terminal/TerminalPanel.test.tsx |
Multi-instance terminal mock; pins replay silence, live answering after the swap, typing during replay, interrupted-replay disposal, and old-daemon rejection |
scripts/prepare-package.js |
Derives the six node-pty pins from core's manifest instead of a third hardcoded table |
docs/design/web-terminal-replay.md |
New design: query ownership, the transport marker, browser restoration, acceptance criteria |
docs/design/web-terminal-replay.zh-CN.md |
Chinese counterpart, same five sections and same technical identifiers |
Testing
This is an unattended CI run, so per the gate's rules I did not build, run, or execute anything from this PR's tree. All test evidence below is the PR's own CI, read through the API at the reviewed commit — real check names and real conclusions.
41 check-runs on b7934e39: 23 success, 17 skipped, 0 failure, 1 in progress (review-pr, which is bot orchestration on an issue_comment event, not PR CI). All five pull_request-event workflow runs are complete and green. Nothing failed, so there is no failing-job log to quote.
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | success |
| Lint & Static (ubuntu-latest, Node 22.x) | success |
| Integration Tests (no-AK, No Sandbox) | success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | success |
| Serve A/B (ubuntu-latest, Node 22.x) | success |
| Capture web-shell visuals (ubuntu-latest, Node 22.x) | success |
| Desktop Shell (ubuntu-22.04) | success |
| Desktop Shell (windows-2022) | success |
| TUI parity snapshots (ink vs opentui) | success |
| OpenTUI no-flicker gate | success |
| Real daemon E2E / Java 11 | success |
| SDK Java (ubuntu 11/17/21, windows 21, macos 21) | success |
| Classify PR · assign · label · Remind on force-push | success |
| Test (windows-latest, Node 22.x) | skipped |
| Test (macos-latest, Node 22.x) | skipped |
| Integration Tests (CLI, No Sandbox) | skipped |
| review-pr | in progress |
Three honest qualifications on that table, because a wall of green oversells it:
- The Windows and macOS unit lanes are skipped by workflow design, not by this PR.
ci.yml:1653and:1556gate both onmerge_group || schedule || workflow_dispatch, so they never run onpull_requestand will run in the merge queue after approval. The win32 code paths are still covered on Ubuntu — the tests force them withosPlatform.mockReturnValue('win32')— but that is mocked-win32, not a real ConPTY. Integration Tests (CLI, No Sandbox)is skipped whileIntegration Tests (no-AK, No Sandbox)passed. Integration coverage exists; it is just the AK lane that did not run.- The two supporting bot lanes at this head are weaker evidence than they look.
Serve A/Bdiffed base vs head across the endpoint set and found exactly one changed field,activeWorkStaleMs4 → 5 — timing noise, and good news for the daemon side.Web-shell Visualsreported no screenshot changed, but its own output says it has no scenario that rendersTerminalPanel, so that is a coverage gap rather than a clean bill of health for the restore path.
Not verified: the head's responder and replay protocol against a real Windows shell. @wenshao's A/B covers the backend switch at 9e54d5ab — whose spawn path is byte-identical to head, so the conhost-leak claim carries forward — but the responder's DA forwarding and the browser restore have only ever run against mocked win32 on Linux, and the author's Chromium component check, which is the author's own claim and not something I re-ran.
Sandboxed verification would settle part of this: @qwen-code /verify is already running against b7934e39 (its state comment is in this thread) and will A/B the responder and replay logic against the base build with mock-free harnesses — worth reading specifically for whether the previous round's 12 failures are now zero, since the code that caused them no longer exists. It will not settle the Windows half: that container is Linux, so native conhost process accounting at this head still needs the merge-queue Test (windows-latest) lane plus a real process-count run like @wenshao's. That is the single unsubstantiated item standing between this PR and a confident merge, and it is a platform-availability gap, not something more review can close.
Real-scenario tmux testing: not applicable — this is an unattended CI run, and the gate never executes PR-derived code on that path. The live-behaviour signal is the CI evidence above plus the /verify lane.
中文说明
在 b7934e39 的代码审查
我在读 diff 之前的独立方案是：在剩下两个 spawn 点复刻 #11497、复用已有的 loadXtermHeadless 而不是新增加载器、保留 inbox 重试（因为 bundled ConPTY 引入了同步抛出）、用推导代替硬编码第三份 conpty.dll pin 表，并且让回放的接收端区分历史与实时，而不是从字节流里用模式匹配抠掉转义序列。PR 全部做到了。唯一超出我方案的是跨包的标记握手，而它超出的理由我能验证：没有标记边界，客户端就无法区分「必须保持沉默的历史」和「必须应答的实时输出」，而全局静音标志会连按键一起丢掉。
当前 head 上没有 Critical。下面是我核对过的、这套设计可能悄悄出错的几个具体位置，然后是仍然挂着的问题。
挂着的 R2-1 Critical 是被结构性消除的，不是被补丁修掉的
R2-1 以及 /verify 那轮的 12 个失败（同一个根因）都关于查询过滤器：无界的 pending 暂存 —— 一个未结束的 OSC 就扣住其后所有字节 —— 加上正则没有覆盖的查询族。我是 grep head 而不是采信描述：TerminalQueryStripper、TERMINAL_QUERY_SEQUENCE_RE、PARTIAL_ESCAPE_SUFFIX_RE、MAX_HELD_ESCAPE_CHARS 和 strip( 在 web-terminal-registry.ts 里全部不存在。registry 现在完全不再改写终端字节。前几轮关于那些正则的任何结论都已经没有对象了。
三条此前提出的 Suggestion 也关闭了，其中两条是结构性关闭而非打补丁：
- responder 的
scrollback: 0—— 已在web-terminal-registry.ts:263。也就是 @wenshao 实测的每个活跃会话 2.76 MB → 0.05 MB。 - spawn 失败的 catch 里释放 responder ——
:440的 catch 中现在有queryReplyDisposable?.dispose(); queryTerminal?.dispose();。 - responder 从不跟随客户端 resize —— 现在是失效而非被修复，而且是正确的失效：唯一被转发的应答是常量
\x1b[?1;2c，所以 responder 的几何尺寸对它发出的内容毫无影响。复现该问题时产生的过期 80 列 CPR（ESC[6n被答成ESC[8;21R）现在交给拥有真实几何的浏览器。:532的resize()只动 PTY 是对的，不是遗漏。 - 未被测试钉住的实时监听那一半 —— 上一轮称为「承重却无测试」的性质现在被钉住了，而且钉在真正要紧的边界上。
TerminalPanel.test.tsx的 "keeps the old terminal interactive until reconnect replay finishes" 断言：恢复终端按序收到历史块和实时块、切换前onData未挂载、回放期间旧终端的按键仍能到达 socket、切换回调之后旧终端被释放、转发器被安装、经它发出的应答能到达 socket。把服务端改回listener(data)不再能保持全绿，因为服务端现在按设计转发原始字节，抑制逻辑住在客户端。
我对着上游源码核实、而不是假设的四点
这套设计有四个承重结论，单看 diff 都看不出来，所以我按 pin 的版本读了 xterm.js 源码。
- DA 抑制是前缀精确的。
term.parser.registerCsiHandler({ final: 'c' }, () => handlesPrimaryDa)看起来会连 DA2、DA3 一起吞掉，而那样它们就没人应答了 —— 服务端只转发 DA1。实际不会。在xterm.js@6.0.0的EscapeSequenceParser.ts中，_identifier({final:'c'})得到0x63，分发按this._collect << 8 | code取键，所以\x1b[>c（0x3e63）和\x1b[=c（0x3d63）完全命不中该 handler，会落到_csiHandlerFb即原生 handler。返回false同样会冒泡到那个 fallback，而这正是「浏览器拥有 DA」的情形。所以「primary DA 只有一个应答方」成立，且不会连带抑制另外两族。 - 空快照的切换仍会触发。 全新 PTY 通常还没有输出，而路由无论如何都会发
Buffer.from('')——terminal.test.ts钉住了这一点。如果 xterm 对空字符串跳过 write 回调，restoreSnapshot就永远不会切换：输出会进入挂在游离 DOM 容器上的终端，用户面对一个空白面板，而输入仍连在那个不可见的终端上。WriteBuffer.ts@5.5.0无条件压入数据和回调，_innerWrite在推进前会对该条目调用cb()，所以切换会发生。这是整条首连路径唯一可能静默死掉的地方。 - 设计文档里的顺序声明是真的。「快照 write 回调安装新转发器并切换可见容器，然后排队的实时 write 才会解析」正是让历史沉默、实时应答的机制，而它依赖 xterm 内部行为。
_innerWrite在对第 N+1 块执行_action之前先调用第 N 块的回调，所以该声明成立。 - 不存在「实时输出先于标记」的竞态。
terminal.ts在:309挂载输出监听，却在:339才发快照控制帧 —— 中间若有实时块到达，客户端会收到未标记的二进制帧并触发 4002「restart the daemon」。这不会发生：从addOutputListener到sendOutput全部同步、没有await，因此 PTY 数据事件无法插入。先挂监听再读快照也是既有的正确顺序 —— 它正是防止输出在「读取」与「挂载」之间丢失的手段。
每个新增字段的消费方核查
按本仓库「声明却无人赋值的选项就是死开关」这条规则：
WebTerminalSnapshot.handlesPrimaryDa—— 只有一个生产读取点terminal.ts:342，且确实被赋值（只要session.queryTerminal存在，readSnapshot就会设置）。注意它在releasePtyResources释放 responder 之后仍为true；无害，因为 shell 已经没了、不会再有查询，并且有测试钉住。AgentViewPtySpawnOptions.useConptyDll—— 变成了必填字段，所以任何第二个构造点都会编译失败。构造点只有一个，在pty-host.ts:353，直达pty.module.spawn(...)。不是死开关。/terminal新增的replay=1要求 —— 该路由只有一个客户端TerminalPanel.buildWsUrl，在同一提交里更新。没有集成测试、desktop-shell 或 bridge 客户端连它；我 grep 过。release-only 路径在replay门禁之前判断，所以旧页面仍能释放终端。@xterm/headless和loadXtermHeadless是复用 fix(core): host Windows shell PTYs via bundled ConPTY to stop orphaning conhost.exe #11497 的产物，不是新加；加载器缓存 module promise，因此第二到第八个终端不会重复 import。
仍然挂着 —— 全部 Suggestion 级，都不阻塞
这是第六轮，所以按 AGENTS.md 这些应该开成后续 issue，而不是再过一遍这个 diff。五条都是要么此前已报告并被作者婉拒，要么是新增且很小。
- bundled→inbox 重试静默丢弃
firstError（web-terminal-registry.ts:381）。无日志、无会话标志。后果是运维层面而非功能层面：conpty.dll加载失败的 Windows 用户会退回泄漏后端，[Windows] web-terminal PTYs leak conhost.exe on natural exit — inbox backend; shell fixed by #11497 #11352 对他们悄悄复发，而 daemon 输出里没有任何提示。一行debugLogger.warn就能关闭。此前作为 R1-2 报告过。 prepare-package.js的nodePtyPins推导仍然静默产出{}，而紧邻的sharp推导会抛。新增的conpty-host.test.tstripwire 是实质改进 —— 它断言 core 清单恰好带六个@lydell/node-pty*pin、全部为已验证版本、且 root 与 core 一致 —— 但它守的是推导的输入。把.filter(...)谓词本身改坏，发布产物就会 0/6 个 pin 而test:scripts全绿。此前作为 R3-3 报告过；我确认了今天六个 pin 都在且相同，所以这是漂移保险，不是现存缺陷。- 新增的客户端协议提示绕过了 i18n。
TerminalPanel.tsx:311硬编码英文'Terminal protocol changed; restart the daemon and reload this page.'，而同文件里每一条同类提示都走t('terminal.notice.*')，且 Web Shell 有完整的 zh-CN 语言包（i18n.tsx:5119-5122）。中文语言环境的用户会在 otherwise 中文的终端里看到一行英文。 restoring?.term在onclose提前返回时不被释放。 那次 dispose 位于ws.onclose里if (ended || NON_RETRYABLE_CLOSE_CODES.has(event.code)) return;之后，而过期 write 回调的守卫只释放liveInput、不释放终端。有界 —— effect cleanup 会在卸载时释放它，且在常见的ended情形下回调仍会运行并完成切换 —— 所以这是短命的游离实例，不是增长型泄漏。- 描述漂移，以及一条我无法核实的断言。 Tested-on 表仍标 Windows「Not tested on a native host」，尽管本帖里已有带具体数字的维护者 Windows A/B；Evidence 也只描述了 Chromium 组件检查。另外，两份设计文档都写着跨包修正「was approved on 2026-09-13」—— 一个晚于 head 提交（
2026-09-12T17:59Z）的日期 —— 而我在整个 thread 里找不到那次批准。之所以提出来，是因为已提交的设计文档正是未来读者会信任的东西；如果签署发生在线下，写明出处会让文档更准确。
一个小 nit：queryTerminal.write(data) 外那个 catch 把抛出原因只归于流中途 dispose，但 xterm 的 WriteBuffer.write 在 DISCARD_WATERMARK 溢出时也会抛。两者都被这个吞异常正确处理了；只是注释只列了两个原因中的一个，而「吞掉一切」的 catch 恰恰是未来读者最需要完整清单的地方。
测试
这是无人值守的 CI 运行，因此按门禁规则我没有构建、运行或执行本 PR 树里的任何东西。下面所有测试证据都来自 PR 自己的 CI，通过在受审提交上调用 API 读取 —— 真实的检查名和真实的结论。
b7934e39 上 41 个 check-run：23 success、17 skipped、0 failure，1 个进行中（review-pr，属于 issue_comment 事件上的机器人编排，不是 PR CI）。五个 pull_request 事件的工作流运行全部完成且为绿。没有失败，因此没有失败日志可引。
对那张表有三点诚实的限定，因为一片绿会高估它：
- Windows 与 macOS 单元测试通道是工作流设计跳过的，不是本 PR 造成的。
ci.yml:1653与:1556都把两者限定在merge_group || schedule || workflow_dispatch，所以在pull_request上永不运行，会在批准后的合并队列里运行。win32 代码路径在 Ubuntu 上仍有覆盖 —— 测试用osPlatform.mockReturnValue('win32')强制进入 —— 但那是 mocked-win32，不是真实 ConPTY。 Integration Tests (CLI, No Sandbox)被跳过，而Integration Tests (no-AK, No Sandbox)通过。 集成覆盖是存在的，只是 AK 通道没跑。- 本 head 上两条辅助机器人通道的证据强度低于表面。
Serve A/B对 base 与 head 跑了整套端点，只发现一个字段变化，activeWorkStaleMs4 → 5 —— 计时噪声，对 daemon 侧是好消息。Web-shell Visuals报告没有截图变化，但它自己的输出说明没有任何场景渲染TerminalPanel，所以对恢复路径而言这是覆盖缺口，不是合格证明。
未验证：head 上的 responder 与回放协议对着真实 Windows shell 的表现。@wenshao 的 A/B 覆盖的是 9e54d5ab 上的后端切换 —— 其 spawn 路径与 head 逐字节相同，所以 conhost 泄漏这一结论可以延续 —— 但 responder 的 DA 转发和浏览器恢复只在 Linux 上对着 mocked win32 跑过，以及作者自己的 Chromium 组件检查（那是作者的声明，不是我重跑的结果）。
沙箱验证能定论其中一部分：@qwen-code /verify 已在对 b7934e39 运行（本帖里有它的状态评论），会用无 mock 的 harness 把 responder 与回放逻辑对着 base 构建做 A/B —— 特别值得看上一轮那 12 个失败是否已归零，因为导致它们的代码已经不存在了。但它不能定论 Windows 那一半：那个容器是 Linux，所以本 head 上的原生 conhost 进程记账仍需要合并队列的 Test (windows-latest) 通道，加上一次像 @wenshao 那样的真实进程计数运行。这是本 PR 与「有把握地合并」之间唯一未被证实的项目，而它是平台可用性缺口，不是更多审查能关闭的。
真实场景 tmux 测试：不适用 —— 这是无人值守 CI 运行，该路径下门禁从不执行 PR 衍生的代码。实时行为信号来自上面的 CI 证据加上 /verify 通道。
— Qwen Code · qwen3.8-max-2026-09-02
Reviewed at b7934e39c60c8a3eb473f31a015d907e34cf3163 · re-run with @qwen-code /triage
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Agent-assisted review at cce1866c0b8eb028130b5618923de7c51ce83423 — no confirmed code Criticals in this static review.
Reviewed the full six-file diff against base 518f6795f9eda4b9911c2a1e8ea76ddc787551cb, the complete changed production files, and the added tests. Traced useConptyDll from both spawn sites through the actual PTY loaders; checked the registry's failed-spawn retry, creation bookkeeping, early output/exit, live release and exit-time cleanup. Downstream inspection included the terminal WebSocket handler, server registry ownership/workspace drain, the shell-service precedent, and Agent View's host-process launch/exit cleanup. No daemon route is added or changed here; the terminal handler continues to supply its resolved workspace cwd/environment to the registry.
Suggestion (non-blocking): packages/cli/src/agent-view/pty-host.ts:346-357 has no inbox-backend retry, unlike web-terminal-registry.ts:325-336. A bundled-DLL load failure propagates through runAgentViewPtyHostProcess before it starts the host server (pty-host-process.ts:477-481). Please confirm that failing Agent View startup, rather than using the web terminal's compatibility fallback, is intentional. This is not a claim that the shipped DLL is missing or that normal Windows launches fail.
History / scope: There were no prior formal review Criticals in the fetched history; the triage comment's fallback asymmetry is the suggestion above, not an independently reproduced native failure. Core/cross-package policy: approximately 57 changed production-file lines including comments; this is a small fix, not a 500+ line refactor. The author has repository admin permission, verified via GitHub's collaborator-permission endpoint.
Verification limits: Static source/test inspection only; no PR code, unit tests, builds, Windows PTYs, or native leak measurements were executed. The mock tests verify option propagation and cleanup calls, not native conhost.exe reclamation. The deliberately retained inbox fallback's natural-exit leak is documented, not presented as fixed.
Comment only; no approval implied.
|
Confidence: 4/5 — the leak fix is real and was measured on the target platform by a maintainer, the standing Critical is gone because the code it was about no longer exists, every load-bearing assumption in the new design checks out against upstream xterm sources, and what remains is five Suggestions plus one platform I cannot reach from here. Going back to my own Stage 2 proposal: the PR does everything I would have done, and the one place it went further — the marked replay boundary across the daemon/Web Shell seam — is further for a reason I could verify rather than take on faith. What I did not expect going in was that this round would be a reduction. Two commits deleted a hand-written escape-sequence filter that three review rounds and a sandboxed verification had all found new ways to fail, and replaced it with an ownership rule that is shorter, has no byte-rewriting to get wrong, and closes the injection on every host rather than only Windows. When a round makes the diff smaller and the standing Critical loses its subject, that is the direction a sixth round should go. The thing I am most satisfied with is that the four assumptions this design rests on are all real, and none of them is visible from the diff. I checked each against the pinned xterm sources rather than reasoning from the API names: the DA suppression is prefix-exact so DA2/DA3 stay browser-answered; the empty-snapshot write callback does fire, so a fresh terminal with no output yet still swaps into view instead of stranding the user on a blank pane wired to an invisible terminal; the snapshot callback really does run before queued live chunks parse, which is the whole "history silent, live answered" mechanism; and the route's listener-before-marker ordering cannot race because the block is synchronous. Any one of those being wrong would have been a silent, hard-to-diagnose failure in production. They are not wrong. Equally, the property the last round called load-bearing and untested is now pinned — and pinned at the boundary where it actually lives. The suppression moved from the server's byte stream to the client's forwarder attachment, and Two honest reservations, neither of which I think should hold this up: The head's Windows runtime path has never executed on Windows. The conhost-leak half is solid — @wenshao's A/B measured 18 orphaned hosts and 111.5 MB at base against 0 and 0 at This is the sixth round, and the diff is not the only thing that has drifted. The Tested-on table still marks Windows untested although this thread contains real Windows numbers, and both design docs assert a cross-package approval "on 2026-09-13" — a date after the head commit — that I could not find anywhere in the thread. Per the re-run rules, hygiene is not a reason to defer, so I am not deferring on it. But the design doc is what someone reads in six months to understand why the daemon and the client must move together, and an unverifiable sign-off date is a poor thing to leave in it. Please refresh the body and either cite the approval or drop the claim before merging. Six months from now, would I curse this or thank it? Thank it. The comments explain why where the why is genuinely non-obvious — the baton-erase ordering, the Verdict: approve. Guardrails cross-checked rather than eyeballed: not a fork PR ( One thing a maintainer needs to do that I will not do myself: Filing the five Suggestions from Stage 2 as a follow-up issue is the right home for them under AGENTS.md's five-round rule. If someone picks them up, the two I would prioritise are the silent 中文说明Confidence: 4/5 —— 泄漏修复是真实的，并由维护者在目标平台上实测；挂着的 Critical 之所以消失，是因为它所指向的代码已不存在；新设计的每一个承重假设都对着上游 xterm 源码核实通过；剩下的是五条 Suggestion，加上一个我在这里够不到的平台。 回到我在 Stage 2 写下的自己的方案：PR 做到了我会做的一切，而它唯一走得更远的地方 —— 跨 daemon/Web Shell 接缝的标记回放边界 —— 走远的原因是我能核实的，而不是只能采信。我进来时没料到的是这一轮是一次缩减。两个提交删掉了一个手写转义序列过滤器 —— 三轮审查加一次沙箱验证都在不断找到它新的失败方式 —— 换成一条归属规则：更短、没有会被写错的字节改写、并且在所有平台（而不只是 Windows）关闭了注入。当一轮让 diff 变小、并让挂着的 Critical 失去对象时，第六轮就该往这个方向走。 最让我满意的是，这套设计依赖的四个假设全都是真的，而且从 diff 里一个都看不出来。我按 pin 的 xterm 源码逐条核对，而不是从 API 名字推断：DA 抑制是前缀精确的，所以 DA2/DA3 仍由浏览器应答；空快照的 write 回调确实会触发，所以一个还没有输出的全新终端仍会切换进视野，而不是把用户留在一个空白面板上、输入却连在不可见的终端；快照回调确实先于排队的实时块解析，而这正是「历史沉默、实时应答」的全部机制；路由「先挂监听后发标记」的顺序不会竞态，因为那一段是同步的。这四条里任何一条错了，都会在生产环境里变成一个静默且难以诊断的故障。它们没有错。 同样，上一轮称为承重却无测试的那个性质现在被钉住了 —— 而且钉在它真正所在的边界上。抑制从服务端的字节流移到了客户端的转发器挂载， 两点诚实的保留，我都不认为应当挡住它： head 的 Windows 运行时路径从未在 Windows 上执行过。 conhost 泄漏那一半是扎实的 —— @wenshao 的 A/B 测得 base 上 18 个孤儿 host、111.5 MB，而 这是第六轮，而漂移的不只是 diff。 Tested-on 表仍标 Windows 未测，尽管本帖里就有真实的 Windows 数字；两份设计文档都断言跨包修正「was approved on 2026-09-13」—— 一个晚于 head 提交的日期 —— 而我在整个 thread 里找不到。按重跑规则，卫生问题不构成 defer 的理由，所以我没有据此 defer。但设计文档正是六个月后有人用来理解「为什么 daemon 和客户端必须同步升级」的东西，把一个无法核实的签署日期留在里面并不好。请在合并前刷新 PR 正文，并且要么写明那次批准的出处，要么删掉该断言。 六个月后我会骂它还是谢它？谢它。注释在 why 真正不显然的地方解释了 why —— baton 擦除的顺序、 结论：批准。 门禁是交叉核对过的，不是靠眼估：不是 fork PR（ 有一件需要维护者做、而我自己不会做的事： 按 AGENTS.md 的五轮规则，把 Stage 2 的五条 Suggestion 开成后续 issue 是它们正确的归宿。如果有人接手，我会优先处理两条：inbox 重试里静默的 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-1 agent-view PTY host spawn has no inbox fallback for the bundled backend's synchronous throw — already reported (comment 5632316061)
- R1-2 bundled-to-inbox retry discards firstError with no log or session flag — already reported (comment 5632316061)
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI, and every local build/test run happened on Linux where useConptyDll is inert, so no executed suite exercised this diff's win32 runtime behaviour.
Not explored to full depth (tool budget reached): "agent 1d": none — but the win32 @lydell/node-pty prebuild is not installed in this Linux worktree, so the bundled backend's synchronous throw sites could only be establi….
Test Plan (not a blocker): src/services/web-terminal-registry.test.ts — no such file or directory; src/services/conpty-host.test.ts — no such file or directory; src/agent-view/pty-host.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI, and every local build/test run happened on Linux where useConptyDll is inert, so no executed suite exercised this diff's win32 runtime behaviour.
未探索到全部深度（达到工具调用预算）："agent 1d"：none — but the win32 @lydell/node-pty prebuild is not installed in this Linux worktree, so the bundled backend's synchronous throw sites could only be establi…。
Test Plan（非阻断）：src/services/web-terminal-registry.test.ts — no such file or directory; src/services/conpty-host.test.ts — no such file or directory; src/agent-view/pty-host.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.23.3)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtx6sqi9yb
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- bundled-to-inbox retry discards firstError with no log or session flag — already reported (triage issue comment 5632316061, point 3)
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI, and every local build and test run happened on Linux where useConptyDll is inert, so no executed suite exercised this diff's win32 runtime behaviour.
Not reviewed: test-efficacy probe — unmeasured (harnessValidated: null): the probe runner tripped this repo's vitest globalSetup prerequisite guard and produced no parseable JSON, so no revert, mutant or hunk measurement ran; the run is neither validated nor refuted and clean coverage is not claimed.
Not explored to full depth (tool budget reached): "agent 4": reading node-pty's Windows agent source ( windowsPtyAgent / conpty.cc ) to confirm or refute a post- CreateProcess throw site — the package is not vendored in …; "agent reverse-audit (round 1)": could not verify the bundled backend's Windows internals against source — @lydell/node-pty-linux-x64 ships only unixTerminal.js , so windowsTerminal.js 's …; "agent reverse-audit (round 1)": did not trace who consumes the agent-view bridge's stdout ( pty-host-process.ts:478 → supervisor-process.ts:2173 → client), so whether any xterm.js answers….
Test Plan (not a blocker): src/services/web-terminal-registry.test.ts — no such file or directory; src/services/conpty-host.test.ts — no such file or directory; src/agent-view/pty-host.test.ts — no such file or directory.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/agent-view/pty-host.test.ts:371 — [review] agent-view suite pins only the win32 half of the new platform branch — flattening pty-host.ts:356 to an unconditional true leaves 37/37 green, while both sibling suites pin both si…packages/core/src/services/web-terminal-registry.test.ts:56 — [review] shared win32 PTY fake still reports _useConptyDll: false, so the live-release path is covered only in its inbox fallback shape and line 526 pins a dispose count producti…packages/core/src/services/conpty-host.test.ts:52 — [review] the 8-line WindowsPtyAgent internals fixture is retyped in all four behavioural cases; two assert only not.toHaveBeenCalled(), so a copy missed on a node-pty field rename degrades…
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI, and every local build and test run happened on Linux where useConptyDll is inert, so no executed suite exercised this diff's win32 runtime behaviour.
未审查（原文为英文）：test-efficacy probe — unmeasured (harnessValidated: null): the probe runner tripped this repo's vitest globalSetup prerequisite guard and produced no parseable JSON, so no revert, mutant or hunk measurement ran; the run is neither validated nor refuted and clean coverage is not claimed.
未探索到全部深度（达到工具调用预算）："agent 4"：reading node-pty's Windows agent source ( windowsPtyAgent / conpty.cc ) to confirm or refute a post- CreateProcess throw site — the package is not vendored in …；"agent reverse-audit (round 1)"：could not verify the bundled backend's Windows internals against source — @lydell/node-pty-linux-x64 ships only unixTerminal.js , so windowsTerminal.js 's …；"agent reverse-audit (round 1)"：did not trace who consumes the agent-view bridge's stdout ( pty-host-process.ts:478 → supervisor-process.ts:2173 → client), so whether any xterm.js answers…。
Test Plan（非阻断）：src/services/web-terminal-registry.test.ts — no such file or directory; src/services/conpty-host.test.ts — no such file or directory; src/agent-view/pty-host.test.ts — no such file or directory。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.3)
The six `@lydell/node-pty*` pins live in three unsynchronized places — the root manifest, packages/core's manifest, and a hardcoded map in scripts/prepare-package.js — but the conpty-host tripwire asserted only one key of one manifest, so bumping the win32 prebuild keys or the root manifest left it green and skipped the human re-check it exists to force. Derive the six pins in writeDistPackageJson from packages/core's manifest (deleting the third hardcoded table), and widen the tripwire to assert all six core keys plus an equality against the root manifest's six — the declaration packages/cli's agent-view actually resolves in a dev tree. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtxennouyn
…eb terminal Switching the web-terminal registry to the bundled ConPTY backend let a probing shell's DA / DSR query bytes into the recorded scrollback; a reconnect that replays the buffer made the client's xterm.js re-answer each query and write the fresh reply back into the still-live shell's stdin. The bundled backend answers no queries itself, and the browser is not guaranteed to be attached when the startup probe fires. Mirror the other half of the shell path (#11497): on win32, feed a headless terminal the PTY stream so it answers the probe server-side, and strip the query bytes from both the scrollback and the live stream so a replay cannot re-emit them. Also bring the releasePtyResources docstring in line with the bundled backend (R1-8). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtxennouyn
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 205 passed · 0 failed · 205 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：205 通过 · 0 失败 · 205 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11643 — fix(core): run web terminal PTYs on the bundled ConPTY backendVerdict: 中文摘要
Central claim and A/BCentral claim. On Windows the web-terminal registry and the agent-view PTY host spawn with node-pty's bundled ConPTY backend ( Arms: H1 — spawn-option / retry matrix (recording fake pty, forced platform)
20 assertions per arm × 4 arms = 80/80 pass. The flip is on the option the pty layer receives and on the retry count; both are exactly the change under test. H2 — end-to-end over a REAL PTY (
|
| arm | responder writes into the PTY | what the shell's stdin received | ESC[c / ESC[6n in scrollback & live stream |
|---|---|---|---|
| v0 (base) | 0 | (empty) | PRESENT / PRESENT |
| v1 (backend only) | 0 | (empty) | PRESENT / PRESENT |
| v2 (head) | 2: ESC[?1;2c, ESC[2;1R |
ESC[?1;2c + ESC[2;1R |
absent / absent |
| v2dist (head, tsc) | 2: same | same | absent / absent |
27 assertions per arm × 4 arms = 108/108 pass, including zero-collateral: ESC[31mRED ESC[0m, ESC[2K, ESC[1;1H, ESC[?25l, ESC[?1049h/l, 中文, 😀 survive byte-identically on every arm. Scrollback shrinks 132 → 119 bytes at head; the 13 removed are ESC[c (3) + ESC[6n (4) + ESC[?62c (6) — the last of which is Finding 4.
H3 — probe set vs strip set (real @xterm/headless, measurement table, no scripted assertions)
19 probes: the responder answers 8; the strip removes 9; one sequence is answered but not stripped (ESC[1$p, DECRQSS → reply ESC[1;0$y). Disproved scarier readings: DA3 (ESC[=c, ESC[=0c) is not answered by xterm.js, so no double answer there; DA1's answer is size-independent.
H4/H5 — the two invariants the new comments assert — 10/10 pass
- H4: a single-chunk probe is stripped; the same probe split across two chunks leaves residue in both the scrollback and the live listener while the responder still answers it (3 writes: two DA answers + one CPR) — i.e. the double-answer the strip exists to prevent returns whenever the single-chunk assumption fails.
- H5:
registry.resize(200,50)returnstruebut the responder's CPR answer is unchanged (ESC[3;1Rbefore and after); a 200×50 terminal fed the identical 150-char line answersESC[2;1R. The responder is pinned at 80×24 and never resized.
H6 — pin derivation — 7/7 pass
Derived set and versions are byte-equal to the deleted hardcoded table (six keys, all 1.2.0-beta.10); a simulated core-manifest bump propagates to the emitted pin while the platform pins stay behind, which is the behaviour the new comment promises. scripts/tests/package-assets.test.js: 37/37 pass at head.
Corrections
- The PR body states the core gate is "42/42". Running exactly the command it names gives 44/44 (
web-terminal-registry.test.ts39 +conpty-host.test.ts5); the cli gate is 37/37 as stated. This is a correction to the description only — the code is fine.
Findings
F-1 (Suggestion, measured) — the responder answers CPR from a stale 80×24 grid and the client never sees the request
web-terminal-registry.ts constructs the responder with hardcoded cols: 80, rows: 24 and never resizes it; registry.resize() resizes only the PTY. Because the strip removes the query from the live listener stream as well, the browser's xterm.js can no longer answer it correctly either. Measured (H5): after registry.resize(200,50), the responder still answers ESC[3;1R for output a 200×50 client would place at row 2 (ESC[2;1R). Any Windows program that issues CPR (PSReadLine being the named motivator for the n final byte) now receives a position computed on a grid that is neither its own nor the client's. The precedent this PR mirrors, shellExecutionService.ts, sizes its headless terminal from the real cols/rows.
Reproduce: node --import <artdir>/register.mjs <artdir>/h4h5-chunks-and-geometry.mjs (H5 block).
Minimal suggested fix (not applied; measured only in the mutation matrix as F2-survived)
Size the responder from the same cols/rows the spawn options use and resize it inside resize(). Note the suite is green with and without this change (F2 survived 44/44), so it must ship with a fixture that feeds a >80-column line and asserts the CPR row follows resize() — that fixture does not exist today.
F-2 (Suggestion, measured) — the responder's answer set is not a subset of the strip set (DECRQSS)
H3 shows ESC[1$p (DECRQSS) is answered by the responder (ESC[1;0$y) but is not removed by stripTerminalQueries, whose regex only matches finals c and n. The raw request therefore reaches the client, whose xterm.js answers it a second time; both replies are written into the still-live shell's stdin — precisely the double-answer the strip was added to prevent, one door down from the doors it closes.
Reproduce: node <artdir>/h3-probe-vs-strip.mjs (row DECRQSS ESC[1$p true false … ANSWERED-BUT-NOT-STRIPPED).
Disproved sibling: DA3 (ESC[=c) is not answered by xterm.js at all, so the same asymmetry there is harmless.
F-3 (Suggestion, measured shape / unproven trigger) — a probe split across two chunks escapes the strip and double-answers
The strip's comment states "node-pty delivers these 3-6 byte probes as a single chunk" as an invariant. H4 shows the invariant is load-bearing: ESC[ + c in two chunks leaves ESC[c in both the scrollback and the live listener while the responder still answers it, so the shell receives two DA answers. This reproduces the shape under controlled chunking; whether ConPTY ever splits a 3-byte probe in production is not established here (it is exactly the kind of coalescing assumption that holds until a load spike). Either the strip must be chunk-boundary-safe (carry a trailing partial sequence across chunks) or the comment should say what enforces single-chunk delivery.
Reproduce: H4 block of h4h5-chunks-and-geometry.mjs.
F-4 (nit, measured) — the strip's stated invariant "can never drop rendered content" is false for responses
ESC[?62c is a DA response, not a request, and the regex strips it. H2 emits one as literal content: at head it is absent from both the live stream and the scrollback (6 of the 13 removed bytes), while the surrounding text survives. Practical impact is small (a captured session log replayed through cat loses DA/CPR response bytes), but the comment's absolute claim is what a future reader will rely on.
Reproduce: H2, assertion DA response printed as content (ESC[?62c) reaches the client (expected absent at head by design — measured, and reported here as the contradicted invariant).
F-5 (Suggestion, measured) — three unpinned surfaces, including the six shipped node-pty pins
Mutation matrix (02-mutation-matrix-killed-vs-survived.png; control 44/44 core + 37/37 cli green, 8 positive kills so the harness is live):
| mutant | edit | result |
|---|---|---|
| M1 | useConptyDll: useBundled → false |
KILLED (2 red, correct tests) |
| M2 | delete the inbox retry | KILLED (2 red, correct tests) |
| M3 | buffered = stripTerminalQueries(data) → data |
KILLED (1 red, correct test) |
| M4 | stop feeding the responder | KILLED (1 red, correct test) |
| M5 | strip regex [cn] → [c] |
SURVIVED 44/44 |
| M6 | never construct the responder | KILLED (1 red, correct test) |
| M7 | delete session.queryTerminal?.dispose() |
SURVIVED 44/44 |
| M8 | drop proc.write(reply) |
KILLED (1 red, correct test) |
| M9 | cli useConptyDll → false |
KILLED (1 red, correct test) |
| M10 | bump pinned @lydell/node-pty in core manifest |
KILLED (pin tripwire) |
| M11 | derivation filter @lydell/node-pty → @lydell/node-pty- |
SURVIVED 37/37 |
| F1 | FIX CANDIDATE: also strip DECRQSS | SURVIVED 44/44 |
| F2 | FIX CANDIDATE: responder 200×50 + scrollback: 0 |
SURVIVED 44/44 |
- M5 is a coverage gap, not dead code: H2 proves the
nbranch is load-bearing (its removal would leaveESC[6nin the scrollback), yet no test asserts CPR stripping — the very case the comment names (PSReadLine's cursor-position request). - M7 is a coverage gap on the responder's teardown:
releasePtyResources()does dispose it (verified by reading), so in a PR whose entire purpose is a per-terminal leak, the new per-terminal object's own release is unpinned. - M11 is the sharpest of the three:
package-assets.test.jsassertsdistPackageJson.optionalDependenciesforsharpand the audio-capture package but contains zero@lydellassertions, so the six pins that decide whichconpty.dll/OpenConsole.exereal Windows users get can silently change shape with the suite green. - F1/F2 surviving is the unpinned-axis signal for Findings 1 and 2: the fixtures that would pin them (a DECRQSS double-answer case; a >80-column CPR case around
resize()) do not exist.
Not covered
- Windows native behaviour: real
conhost.exe --headless/OpenConsole.exeaccounting, the real bundledconpty.dllload/unload throw path,COMSPEC=powershellfirst-output latency, and interactive rendering (vim, Ctrl+C, resize, CJK input). This container is Linux; the win32 prebuild is not installed and@lydell/node-ptyselects its prebuild at import time. My forced-win32harnesses verify the registry's decisions (what it passes, what it does on a synchronous throw, what it does with the output stream), not the native backend. The PR itself lists these as manual Windows acceptance. - Agent-view path end-to-end:
packages/cli/src/agent-view/pty-host.tsis one line plus an interface field; verified via its own suite (37/37) and M9 (killed), not by an A/B of the worker path. - Per-commit attribution: the checkout is depth 2; only
9e54d5abof the four PR commits is reachable (cce1866c,c253fd9d,73dd42fcare not). The aggregateHEAD^1..HEADdiff is what was verified. Thev1arm was therefore reconstructed by surgically reverting the responder half (each revert asserted to apply exactly once), not taken from a commit. - Base drift: the metadata snapshot's
baseRefOid(78bbd9f5…) is not present in this shallow checkout; per the merge-ref contract I usedHEAD^1(aaa6a32a), which is what the merge commit was built against. - Trial merge into current
mainand repo-wide gates: not possible (shallow, no network) / out of scope by design. - ReDoS ladder on
stripTerminalQueries: not run. The pattern has a single unnested star (linear), and its input is local program output rather than remote attacker text; noted so the cap is explicit. - Mutation matrix and H3 are not counted in
assertions.json: the matrix's classifier and H3's table are scripted measurements without encoded pass/fail expectations, so only H1 (80), H2 (108), H4/H5 (10) and H6 (7) contribute to the 205/0 totals.
Methodology
Linux node:22 container, CI merge-ref checkout (HEAD = merge, HEAD^1 = base tip, HEAD^2 = PR head); npm ci/npm run build pre-existing at head. Arms are the real tsc-built packages/core/dist (or esbuild transpiles of base/head/intermediate sources, proven equivalent by the v2 ≡ v2dist columns) with an ESM resolve hook redirecting getPty.js and, per arm, web-terminal-registry.js; variants were copied beside the shipped file so their relative imports resolve identically, and removed afterwards. H2/H4/H5 force os.platform()/process.platform to win32 — the registry's only gate for the bundled backend — while using the real POSIX node-pty prebuild, real shells and the real @xterm/headless responder; H2's target script is written with literal ESC bytes so no shell escape rules are in play. Targeted gates: packages/core web-terminal-registry.test.ts + conpty-host.test.ts (44/44), packages/cli pty-host.test.ts (37/37), scripts/tests/package-assets.test.js (37/37). Raw logs, harness sources and the mutation matrix live in logs/ (h1-*.log, h2-*.log, h3-probe-vs-strip.log, h4h5.log, h6-pin-derivation.log, mutants/*.log, mutation-matrix.txt, gate-*.log); evidence images in evidence/.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/agent-view/pty-host.test.ts: (cd packages/cli) npx --no-install vitest run ./src/agent-view/pty-host.test.ts
file packages/core/src/services/conpty-host.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/conpty-host.test.ts
file packages/core/src/services/web-terminal-registry.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/web-terminal-registry.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/agent-view/pty-host.test.ts: PPPPP
packages/core/src/services/conpty-host.test.ts: PPPPP
packages/core/src/services/web-terminal-registry.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 1 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 1 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 2 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 2 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 2 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 3 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 3 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 3 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 4 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 4 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 4 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 5 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 5 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 5 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 326 passed · 0 failed · 326 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：326 通过 · 0 失败 · 326 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11643 — fix(core): run web terminal PTYs on the bundled ConPTY backendVerdict: Verified head:
中文摘要
Previous-finding status
Central claim and A/BCentral claim. On Windows the web-terminal registry and the agent-view PTY host spawn with node-pty's bundled ConPTY backend ( Arms: H1 — spawn-option / retry matrix (raw:
|
| scenario | v0 (base) | v1 / v2 / v2dist |
|---|---|---|
| win32, spawn ok | 1 call, no useConptyDll key |
1 call, true |
| win32, bundled throws | 1 call (predicate cannot fire — base sets no option) | 2 calls [true,false], created |
| win32, both throw | 1 call, Failed to spawn shell |
2 calls [true,false], Failed to spawn shell |
| linux, spawn throws | 1 call, error | 1 call, false, error — no retry |
| linux, spawn ok | 1 call, no key | 1 call, false |
108/108 pass. The flip is on the option the pty layer receives and on the retry count — exactly the change under test.
H2 — end-to-end over a REAL PTY (01-ab-real-pty-base-vs-head.png)
Real @lydell/node-pty child, real /bin/sh running a generated target (driven via COMSPEC, since the registry spawns file with args: []), real @xterm/headless responder, platform forced to win32 at the registry's own gate. Three independent oracles: a file the shell captures from its own stdin (destination side of the wire), a pass-through recorder on the real UnixTerminal prototype's write() (registry side), and the listener stream + readSnapshot() (the reconnect replay).
| arm | replies written into the PTY | shell's own stdin capture | ESC[c / ESC[6n in scrollback & live |
scrollback |
|---|---|---|---|---|
| v0 (base) | 0 | 0 B | PRESENT / PRESENT | 181 B |
| v1 (backend switch only) | 0 | 0 B | PRESENT / PRESENT | 181 B — byte-identical to base |
| v2 (head, esbuild) | 2: ESC[?1;2c, ESC[3;10R |
14 B, equal to what the registry wrote | absent / absent | 168 B |
| v2dist (head, tsc) | 2: same | 14 B, same | absent / absent | 168 B |
79/79 pass. The v1 row is the load-bearing half of the A/B: switching the backend alone changes the output stream not at all, so the entire behavioural delta belongs to the responder/strip half. Zero collateral asserted per arm on ESC[31mRED ESC[0m, ESC[2K, ESC[1;1H, ESC[?25l, ESC[?1049h/l, 中文, 😀, and the text surrounding each stripped query. Residual fully accounted: 181 − 168 = 13 = 3 + 4 + 6, and the 6 is F-4.
Harness-validity assertions ran first on every arm (FIXTURES_DONE seen, scrollback and live stream non-empty), so the cells cannot pass vacuously. Two harness defects were caught by those checks and fixed before the numbers above were taken: POSIX redirects a backgrounded command's stdin from /dev/null, and GNU timeout puts its child in a new process group where a tty read draws SIGTTIN — either alone silently yields a 0-byte capture that looks like "the responder answered nothing".
H6 — pin derivation — 14/14 pass
The derived set is byte-equal to the deleted hardcoded table (six keys, all 1.2.0-beta.10), and the root manifest's six equal core's six — which is what the widened tripwire asserts. A simulated bump of @lydell/node-pty-win32-x64 propagates to the emitted pin while the other five stay behind; a seventh key would be picked up automatically and a removed key drops out.
Findings
N-1 (Suggestion, measured) — the responder headless Terminal leaks on the failed-spawn path
create() constructs queryTerminal before the spawn try. When both the bundled and the inbox spawn throw, it returns { error: 'Failed to spawn shell' } without storing the Terminal in a session and without disposing it — so it is unreachable and never released. In a PR whose entire purpose is that "every exited web terminal leaked ~8 MB for the life of the CLI", this reopens the same class on the exact path the PR introduces the retry for.
Measured (H7, 36/36 pass, 03-h7-responder-leak-on-failed-spawn.png):
| scenario | constructed | disposed | leaked |
|---|---|---|---|
| both spawns throw — v0 / v1 | 0 | 0 | 0 (nothing to leak) |
| both spawns throw — v2 / v2dist | 1 | 0 | 1 |
5 failed create() attempts on head |
5 | 0 | 5, and readSnapshot() is undefined for every id — none reachable |
success then release() — v2 / v2dist |
1 | 1 | 0 |
| bundled throws, inbox retry succeeds | 1 | 1 | 0 |
| linux, spawn ok (all arms) | 0 | 0 | 0 |
The success and retry rows are the positive control: the probe can observe disposal, so disposed=0 on the failure path is a measurement, not a dead instrument. Note the retry path itself is fine — the leak is only on the both-fail path.
Blast radius, bounded: this needs Windows with a missing/unloadable bundled conpty.dll and a failing inbox backend, and it scales with failed attempts, not with successful terminals. What makes it more than theoretical is that a client retrying a failed open leaks one Terminal per attempt, forever.
Reproduce: bash <artdir>/run-harness.sh h7-responder-leak.mjs
Minimal suggested fix (measured: applied as mutant M12, suite stayed 44/44 green)
} catch {
queryTerminal?.dispose();
this.finishCreating(terminalId);
return { error: 'Failed to spawn shell' };
}M12 surviving 44/44 is the unpinned-axis signal: the suite cannot tell head from head-plus-fix. It must ship with a fixture — extend the existing reports a spawn failure and frees the id when both backends fail test with a dispose spy on the responder. That fixture does not exist today.
N-2 (Suggestion, measured) — create() gained a second await after its cancellation guards, so cancellation and dispose() can be silently lost
Base create() has exactly one await after this.creating.set(terminalId, …):
ptyImpl = await getPty();
if (this.cancelledCreations.has(terminalId)) { …return cancelled }
if (this.disposed) { …return disposed }The PR inserts a second await after both guards and never re-checks either:
const { Terminal: HeadlessTerminal } = await loadXtermHeadless();An interrupt landing in that window is never observed. Measured (H8, 62/62 pass, 02-h8-cancel-dispose-race-base-vs-head.png), one timed interrupt driving all arms:
| interrupt | arm | spawned at interrupt time | create() result |
session registered after | PTY killed | after the idle-reclaim window |
|---|---|---|---|---|---|---|
dispose() |
v0 base | yes (1) | success | false | 1 | gone |
dispose() |
v1 backend-only | yes (1) | success | false | 1 | gone |
dispose() |
v2 / v2dist head | no (0) | success, on a disposed registry | true | 0 | still registered, still not killed |
release() |
v0 / v1 | yes (1) | success | false | 1 | gone |
release() |
v2 / v2dist head | no (0) | success, though release() returned true |
true | 0 | released by the idle reclaim |
Two distinct consequences, ranked by observability:
dispose()→ unbounded. The session is registered afterdispose()already iteratedthis.sessions, andscheduleReclaim's callback is guarded by!this.disposed, so the 15-minute reclaim fires and deliberately does nothing. Nothing ever releases this PTY, its conout worker, or its responder (constructed 1 / disposed 0). Verified empirically, not by reading: with long timers scaled 1000×, the session is still registered and still unkilled after the reclaim window.release()/releaseWorkspace()→ bounded but contract-breaking.release()returnstrue, i.e. "cancelled", while a live PTY is spawned holding the drained workspace's cwd. The idle reclaim does eventually release it (measured gone after the scaled window), so this is 15 minutes, not forever.
Reproduced with no substitution at all. The real rows use the genuine load-xterm-headless.js and a single await new Promise(setImmediate) — no delay double, no timing margin. Base's only await resolves in microtasks once node-pty is warm, so base has already spawned; head's cold import('@xterm/headless') is real file I/O still pending on the first immediate turn. Window validity is asserted on both sides of every cell (callsAtInterrupt 0 for head, 1 for base), which is what caught an earlier harness defect where a memoized double made only the first arm pay the delay.
Window width, measured: cold @xterm/headless import 18.9–21.2 ms over 5 samples (logs/xterm-import-cost.log), against 8.6 ms for the warm getPty() await that base does guard — so the unguarded window is roughly 2× wider than the guarded one it sits behind. Honest bound: loadXtermHeadless() memoizes, so the wide window occurs once per CLI process, on the first Windows web terminal. Afterwards the await is a microtask and only another in-flight microtask could interleave.
Attribution: v1 is immune to both interrupts, so the cause is the new await, not the backend switch.
Real callers, so the trigger is not hypothetical: stopWebTerminalRegistry → dispose() (packages/cli/src/serve/server.ts:993, stopAppResource at :3537, acp-http/index.ts:2491/2497/2569), releaseWebTerminalsForWorkspace → releaseWorkspace() (server.ts:995), and registry.release(terminalId, workspace.workspaceCwd) on the WebSocket release-only/close/error paths (routes/terminal.ts:150, 203, 219, 225, 247, 273). Opening the first web terminal tab and closing it immediately — or a daemon shutdown / workspace drain landing at that moment — is the whole trigger.
Reproduce: node <artdir>/h8-driver.mjs
Minimal suggested fix (measured: applied as mutant M13, suite stayed 44/44 green)
Re-check both guards after the new await, releasing the responder it may have just built:
if (this.disposed || this.cancelledCreations.has(terminalId)) {
queryTerminal?.dispose();
this.finishCreating(terminalId);
return { error: 'Web terminal creation cancelled' };
}M13 surviving 44/44 means the suite pins nothing along this axis either. The fixture that would go red: a create() whose responder import is still pending when release()/dispose() runs, asserting no session is registered and no PTY is spawned. Note the fix should return the disposed error when this.disposed, to match base's two distinct messages.
N-3 (nit, measured) — the responder-construction failure is swallowed with no trace
} catch {
// No responder available: the query stays unanswered (a bounded ~2s
// stall), never injected — the strip below still keeps it out of the
// scrollback.
queryTerminal = undefined;
}Following the value: grep -n "logger\|console\.\|debug(" over packages/core/src/services/web-terminal-registry.ts returns a single hit, and it is the word "console" inside a prose comment at line 350 — there is no logging call in the file at all, and no field records that the responder is absent. So the documented degradation is unobservable — in production, in devtools, and to any future reader trying to work out why a Windows terminal stalled ~2 s on startup. The fail-open itself is the right call; only its silence is the finding. This harness hit it by accident: an early version of the counting double threw inside loadXtermHeadless(), and the registry absorbed it so completely that the only symptom was "responder constructed 0", with no error anywhere.
F-1 … F-5 (standing, re-measured)
See the status table above. Each was re-measured at this head rather than carried forward on the strength of the identical OID; the harnesses are h3h4h5-responder-surface.mjs (F-1 via H5, F-2 via H3, F-3 via H4, F-4 via H2/H3) and the mutation matrix (F-5). The strip function under test is lifted verbatim out of the shipped tsc build, not retyped, and the lifted regex literal is itself asserted to be /\x1b\[[?0-9;>]*[cn]/g.
Mutation matrix (04-mutation-matrix-killed-vs-survived.png)
Controls green first, or the kills mean nothing: control-core 44/44, control-scripts 37/37.
| mutant | edit | result | red test(s) |
|---|---|---|---|
| M1 | useConptyDll: useBundled → false |
KILLED (2) | "spawns Windows terminals with the bundled ConPTY backend", "retries a failed bundled spawn once on the inbox backend" |
| M2 | delete the inbox retry | KILLED (2) | "retries a failed bundled spawn…", "reports a spawn failure and frees the id when both backends fail" |
| M3 | buffered = stripTerminalQueries(data) → data |
KILLED (1) | "answers the bundled-backend DA probe and keeps it out of the scrollback" |
| M4 | stop feeding the responder | KILLED (1) | same |
| M8 | drop proc.write(reply) |
KILLED (1) | same |
| M5 | strip regex [cn] → [c] |
SURVIVED 44/44 | coverage gap — H2 proves the n branch load-bearing |
| M7 | delete session.queryTerminal?.dispose() |
SURVIVED 44/44 | coverage gap on responder teardown |
| M14 | delete both teardown lines (combination row) | SURVIVED 44/44 | ⇒ M7 is a real gap, not layered defence |
| M11 | derivation filter → '@lydell/node-pty-' |
SURVIVED 37/37 | the six shipped pins are unpinned |
| M16 | mutate @qwen-code/audio-capture in the same object literal |
KILLED (1) | positive control proving the scripts suite is live |
| M12 | FIX CANDIDATE for N-1 | SURVIVED 44/44 | unpinned axis |
| M13 | FIX CANDIDATE for N-2 | SURVIVED 44/44 | unpinned axis |
Every kill names the test that should catch it, and all five live in web-terminal-registry.test.ts — the same file as the M5/M7/M14 survivors, so those survivals are not an artifact of the runner collecting the wrong file. For the scripts suite my first control attempt (M15, bumping @teddyzhu/clipboard) survived, so I landed M16 inside the very optionalDependencies literal the @lydell spread is emitted into; M16 was killed, which is what licenses the M11 claim.
5 killed / 6 survived. Completeness reporting, not merge conditions — except M12/M13, which are the reason N-1 and N-2 must ship with fixtures.
Scaling ladder on stripTerminalQueries (closes a gap the previous round listed as not run)
The pattern has one unnested star, and the ladder confirms it is linear rather than merely arguing so:
| shape | 2 k | 3 k | 5 k | 20 k |
|---|---|---|---|---|
ESC[ + digits, no final byte |
0.007 ms | 0.006 ms | 0.007 ms | 0.024 ms |
many unterminated ESC[ runs |
0.010 ms | 0.011 ms | 0.016 ms | 0.065 ms |
| realistic mixed output | 0.004 ms | 0.005 ms | 0.007 ms | 0.028 ms |
Worst rung 0.065 ms at ~19 k characters — no superlinearity, nothing near the 30 s cap. Input here is local program output rather than remote attacker text, so this was a completeness check, not a hazard hunt.
Not covered
- Windows native behaviour: real
conhost.exe --headless/OpenConsole.exeaccounting, the real bundledconpty.dllload/unload throw,COMSPEC=powershellfirst-output latency, and interactive rendering (vim, Ctrl+C, resize, CJK). Only@lydell/node-pty-linux-x64is installed here (ls node_modules/@lydell/), and node-pty selects its prebuild at import time. My forced-win32harnesses verify the registry's decisions — what it passes, what it does on a synchronous throw, what it does with the output stream — not the native backend. This reproduces the shape of the Windows path, not the leak the PR was filed against. The PR itself lists these as manual Windows acceptance. - Agent-view path end-to-end:
packages/cli/src/agent-view/pty-host.tsis 5 added lines and 0 removed — one interface field plus oneuseConptyDll: process.platform === 'win32'option and its comment. Verified via its own suite (37/37) and by reading, not by an A/B of the worker path. - Per-commit attribution: depth 2.
git rev-list HEAD^1..HEAD^2yields 1 commit while the metadatacommitsarray holds 4 (cce1866c,c253fd9d,73dd42fcunreachable), so the aggregateHEAD^1..HEADdiff is what was verified.v1was reconstructed by surgically reverting the responder half — each of the 8 replacements asserted to apply exactly once — not taken from a commit. - Base drift: the snapshot's
baseRefOid(78bbd9f5…) is not present in this shallow checkout; per the merge-ref contract I usedHEAD^1(aaa6a32a), which is what the merge commit was built against. - Trial merge into current
mainand repo-wide gates (lint, typecheck, full suites): not possible (shallow, no network) / out of scope by design. The flakiness gate is the workflow's, not this round's; I did not re-run it. - The
dispose()-race consequence beyond one session: I proved one leaked PTY per racedcreate(). I did not measure how many can accumulate, or drive it through the real HTTP/WebSocket routes. - Mutation matrix, the H3 probe census and the H5 drift measurement are not counted in
assertions.json: they are scripted measurements without encoded pass/fail expectations. Only H1 (108), H2 (79), H3/H4/H5 (27), H6 (14), H7 (36) and H8 (62) contribute to the 326/0 totals.
Methodology
Linux node:22-bookworm container, CI merge-ref checkout (HEAD = merge, HEAD^1 = base tip, HEAD^2 = PR head), npm ci and npm run build pre-existing at head. Arms are esbuild transpiles of base / intermediate / head source written beside the shipped packages/core/dist/src/services/web-terminal-registry.js so their relative imports resolve identically, plus the unmodified tsc build as v2dist; build-arms.mjs rebuilds them and cleanup-arms.mjs archives and removes them. H1/H7/H8 drive the arms through an ESM resolve hook (register.mjs + arms/hooks.mjs) that redirects only getPty.js to a recording fake and, where stated, load-xterm-headless.js to a double that subclasses the real @xterm/headless Terminal to count construct/dispose/resize without changing its behaviour. H2 and H8's real mode use no double in the causal path: real node-pty, real /bin/sh, real xterm. H2/H7/H8 force os.platform() and process.platform to win32 — the registry's only gate for the bundled backend — after importing the POSIX prebuild, since node-pty picks its prebuild at import time. Fake PTY pids are 1 on purpose: release() calls killPtyTree(), whose POSIX arm signals real pids when pid > 1. Targeted gates: packages/core web-terminal-registry.test.ts + conpty-host.test.ts (44/44), packages/cli pty-host.test.ts (37/37), scripts/tests/package-assets.test.js (37/37). Raw logs, harness sources, arm sources and per-mutant vitest output live in logs/ (h1.log, h2.log, h3h4h5.log, h6.log, h7.log, h8.log, gates.log, xterm-import-cost.log, mutation-matrix.log, mutation-matrix-parsed.log, mutants/*.log, mutants/matrix.json); evidence images in evidence/.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/agent-view/pty-host.test.ts: (cd packages/cli) npx --no-install vitest run ./src/agent-view/pty-host.test.ts
file packages/core/src/services/conpty-host.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/conpty-host.test.ts
file packages/core/src/services/web-terminal-registry.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/web-terminal-registry.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/agent-view/pty-host.test.ts: PPPPP
packages/core/src/services/conpty-host.test.ts: PPPPP
packages/core/src/services/web-terminal-registry.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 1 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 1 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 2 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 2 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 2 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 3 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 3 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 3 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 4 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 4 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 4 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 5 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 5 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 5 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Maintainer verification — the Windows half, on a real Windows machineI verified this locally as a maintainer, deliberately aimed at the one thing the PR lists as not validated: "Windows real-machine acceptance (conhost/OpenConsole counting, pwsh latency, interactive rendering) — needs a Windows environment; PR CI skips the Windows job." The sandboxed verification above also had to leave exactly that uncovered ("This container is Linux"). So I ran the PR on a real Verdict: LGTM — the central claim holds on real Windows, and the fix is exactly as large as advertised. Three non-blocking follow-ups below; two of them independently reproduce findings the sandbox report already filed. 1 · The leak, and its absence, measured on Windows18 web terminals opened and exited through the real registry (3 rounds × 6, each shell exits naturally), counting
The same A/B at the raw backend level (6 PTYs, no registry) isolates it to the backend rather than to anything in this diff, and the interactive arms answer the rest of the manual acceptance list —
Two results in that figure are worth calling out:
Also: the PR's own unit tests actually executed on Windows — the platform PR CI skips. 2 · POSIX is unchanged, and the built product still works
Against a real PTY and a real shell, head·native ≡ base·native on every observable, and the new behaviour appears only behind the win32 gate (2 × 2 matrix in the next figure). 3 · Do the new tests hold the new behaviour down?10 mutations, each applied alone, then the PR's own suites re-run — 7 killed, 3 survived:
Two of the survivors are the same gaps the sandbox report filed (responder dispose; the 4 · Follow-ups (none blocks the merge)
5 · The description has drifted from the head commit
Environment, harnesses and how to reproduce
中文版维护者本地验证 —— 在真实 Windows 机器上补齐 Windows 那一半我以维护者身份在本地做了一轮验证，目标明确对准 PR 自己列为未验证的那一项：「Windows 真机验收（conhost/OpenConsole 计数、pwsh 延迟、交互渲染）—— 需要 Windows 环境；PR CI 跳过 Windows job」。上面那份沙箱验证同样把这块留空了（"This container is Linux"）。 所以我在真实的 结论：LGTM —— 核心声明在真机上成立，修复范围与描述一致。 下面三条后续项均不阻塞合并，其中两条与沙箱报告的发现相互独立复现。 1 · Windows 上实测到的泄漏与其消失通过真实 registry 开/关 18 个 web terminal（3 轮 × 6，每个 shell 自然退出），统计挂在 daemon 进程下的
裸后端层面的同款 A/B（6 个 PTY，不经 registry）说明泄漏来自后端本身而非本 diff 的任何代码；交互臂回答了手工验收清单的其余部分 —— 其中两点值得单独说：
另外，PR 自带单测这次真的在 Windows 上跑过了（PR CI 跳过的那个平台）： 2 · POSIX 行为不变，打出来的产物照常可用head 上 对真实 PTY + 真实 shell，head·native 与 base·native 在每一个可观测量上完全一致，新行为只出现在 win32 门后（见 2 × 2 矩阵）。 3 · 新增测试能否钉住新行为10 个变异逐个施加后重跑 PR 自带套件：7 个被杀、3 个存活。其中两个与沙箱报告所报的缺口相同（应答器 dispose； 4 · 后续项（都不阻塞合并）
5 · PR 描述已与 head commit 脱节
（复现环境与方法见英文正文折叠块；证据图托管在 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@yiliang114 — needs one change before this can ship. Details in my Stage 2 and Stage 3 notes above.
Blocking: the headless query responder added in 9e54d5ab is constructed at 80x24 (web-terminal-registry.ts:275-279) and never resized. WebTerminalRegistry.resize() (:547-556) resizes only session.pty, while the precedent this PR mirrors resizes both together (shellExecutionService.ts:2615-2616). Because stripTerminalQueries removes CSI c/n from the live listener stream too (:316, :337), the correctly-sized browser xterm.js can no longer answer a CPR — so the stale 80x24 grid is the only answer, and it is wrong for essentially the whole session: TerminalPanel.tsx:167-168 fits and sends a resize on every mount, forwarded at serve/routes/terminal.ts:279. One line in resize() (session.queryTerminal?.resize(cols, rows)) plus a win32 CPR-after-resize case.
Everything else is notes, not blockers: the stale "no headless xterm is introduced into the registry" line in Risk & Scope, the unbounded responder scrollback (xterm default 1000 vs the shell path's 200), the three silent catches where the shell path logs, and prepare-package.js deriving to {} without throwing.
R2-1, R1-6 and R1-8 are all verified fixed at this commit — that work is good.
中文说明
合并前需要改一处，细节见上方我的 Stage 2 和 Stage 3 备注。
阻断项： 9e54d5ab 新增的 headless 查询应答器以 80x24 构造（web-terminal-registry.ts:275-279），且从不 resize。WebTerminalRegistry.resize()（:547-556）只 resize 了 session.pty，而本 PR 对标的先例是两者一起 resize（shellExecutionService.ts:2615-2616）。由于 stripTerminalQueries 也会从实时监听流里剥掉 CSI c/n（:316、:337），尺寸正确的浏览器 xterm.js 再也无法应答 CPR——于是那个过期的 80x24 网格成了唯一应答者，而且几乎在整个会话期间都是错的：TerminalPanel.tsx:167-168 在每次挂载时自适应并发送 resize，由 serve/routes/terminal.ts:279 转发。修复是 resize() 里的一行（session.queryTerminal?.resize(cols, rows)），外加一个 win32 下 resize 后 CPR 的用例。
其余都是备注，不是阻断项：Risk & Scope 里那句过期的「registry 未引入 headless xterm」、应答器无上限的 scrollback（xterm 默认 1000，shell 路径是 200）、shell 路径有日志而这里三处静默 catch、以及 prepare-package.js 在不抛错的情况下派生成 {}。
R2-1、R1-6、R1-8 在当前 commit 均已确认修复——这部分做得很好。
— Qwen Code · qwen3.8-max-2026-09-02
|
@qwen-code /triage |
|
Closeout at The registry no longer rewrites terminal bytes. Its Windows responder forwards only primary DA, and snapshot metadata tells the browser when that probe is server-owned. A new PTY's initial output can receive replies. Reconnect history restores into a detached terminal with no forwarding listener; the old visible terminal keeps accepting input, and the snapshot callback swaps views and enables replies before queued live output parses. Both sides reject incompatible replay protocols explicitly. This applies to Web Shell on all hosts, not only VS Code; daemon and Web Shell must update together. Verification:
Scope integrity (changed lines = additions + deletions, including comments/scripts; not pure production-logic LOC):
The two added implementation files are the existing terminal WebSocket route and Web Shell component, both required by the explicitly approved replay contract. This additive commit touches three implementation files, +123/-131, replacing the handwritten filters with the boundary above; it does not add dependencies or extend agent view. The original baseline is preserved and the conservative round count is now five. Scope verdict: corrected under the recorded user approval, not a new baseline. Four obsolete tests were deleted and two payload/display cases merged into the exact-stream regression after independent deletion defense; the parallel color regression remains. Feedback: R2-1 is addressed by this correction. The two overlapping color threads were already resolved by the parallel patrol; their correction was retained. Public Status snapshot: the pushed head is mergeable, but CI/review gates are still pending. Lint & Static, Ubuntu tests and no-AK integration were in progress; the native Windows unit job was skipped. No automated-review wait or merge was performed. Native Windows process counts, startup/DLL-fallback behavior and real-daemon/PTY acceptance remain outstanding; the existing inbox fallback is not claimed to fix the leak. No further Suggestions or unrelated agent-view changes are included in this round. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed
packages/core/src/services/web-terminal-registry.ts(+185/-16)packages/core/src/services/web-terminal-registry.test.ts(+466/-43)packages/core/src/services/conpty-host.ts(+10/-8)packages/core/src/services/conpty-host.test.ts(+156, new file)packages/cli/src/agent-view/pty-host.ts(+5)packages/cli/src/agent-view/pty-host.test.ts(+22)scripts/prepare-package.js(+14/-6)
Findings
No blockers found. All previously raised Critical findings (query scrub, resize forwarding, loadXtermHeadless cancellation, stripper/responder gate mismatch, DCS DECRQSS coverage, chunk-split handling) are resolved at HEAD.
Clean areas
- ConPTY backend selection:
useConptyDll: os.platform() === 'win32'is correct; bundled spawn with inbox fallback on failure is sound (failed synchronous spawn produces no child, no double-spawn risk). TerminalQueryStripperregex correctness: Covers all xterm.js query families (DA1/2/3, DSR, DECRQM, DECRQSS, XTVERSION, DECREQTPARM, OSC colour queries).PARTIAL_ESCAPE_SUFFIX_REcorrectly identifies viable query prefixes without swallowing non-query OSC title sets. Non-query escapes (SGR, erase, cursor) preserved.- Lifecycle management:
releasePtyResourceswithptyResourcesReleasedflag prevents double-disposal.queryTerminalandqueryReplyDisposableproperly disposed. No resource leaks — all synchronous code between creation andsessions.set()prevents event-loop interleaving. - Concurrency: Each session has its own
TerminalQueryStripperandqueryTerminalinstance. No shared mutable state between terminal sessions. prepare-package.js: Derivation frompackages/core/package.jsonoptionalDependencies is correct. Version tripwire test asserts all 6 pins match across both manifests.- Platform gating: Off Windows,
queryTerminalstaysundefined, stripping is a no-op. Test-pinned. - Test coverage: Comprehensive — bundled backend spawn, DA probe answer/strip, split chunks, DCS DECRQSS (complete and split), OSC partial handling, non-query escape preservation, responder resize, cancellation during headless load, inbox retry on bundled failure, double-failure cleanup.
Needs human review
- Windows CI skipped:
windows-latestjob was not run. Actual native ConPTY behavior, bundled-to-inbox fallback with realconpty.dll, andconhost.exeleak reclamation are unverified on real Windows. @xterm/headlessdependency: Dynamically imported on every Windows terminal creation. Verify it's in the published package's dependencies (not just devDependencies) — runtimeimport()requires it to be installed.
Reviewed with AI assistance.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
COMMENT
核对基线：head 77257a5acc1d1e663f2b42433f0d3d21bb456bbf。
本次不 Approve：唯一未解决的线程是一条 Critical（R2-1，web-terminal-registry.ts:112），作者已明确说明只修了其中两个症状、其余三个转入 #11734 并刻意不关闭该线程。我在当前 head 的代码上独立复核，确认其中症状 (3) 仍然成立。
已确认修好的部分：症状 (1) 与 (4)
我逐条对过 web-terminal-registry.ts:83-122，缓解措施与线程里给出的方案一致且实现正确：
pending现在有上界，吞字节不会永不结束。MAX_HELD_ESCAPE_CHARS = 256（:110）配合strip()里的const hold = partial !== null && partial[0].length <= MAX_HELD_ESCAPE_CHARS;（:118）——超界时pending清空、complete取整个combined，也就是把累积内容当 payload 整体放行。原报告里「之后每个 chunk 都返回''、pending在MAX_BUFFER_BYTES之外持续增长、$锚定正则反复全量重扫」这条失效路径已经关掉。- OSC 挂起分支收窄成只hold 颜色查询前缀。
PARTIAL_ESCAPE_SUFFIX_RE的 OSC 分支是\x1b\](?:10|11|4;[0-9]+);\?(?:[^\x07\x1b\x9c])*：保留?谓词（因此未终结的标题/颜色 SET 不会被误当查询挂起，也就不会被从两个出口丢掉）、字符类排除\x1b（遇到裸 ESC 即结束，与 xterm 取消 OSC 的行为一致）、排除\x9c（接受 C1 ST 作为终结符）。三点都对上了。 - DCS 引导符已纳入挂起。 正则末尾新增的
|\x1bP\$?让ESC P与ESC P $这两种落在引导符中间的切分也被hold，因此不会再两半漏进session.buffer重新拼成一个完整 DECRQSS 被浏览器二次应答。症状 (4) 关闭。
作者记录的红绿确认（移除 OSC 上界会让 does not swallow payload after an unterminated non-query OSC 变红、移除 DCS 分支会让 DECRQSS 用例变红，48/48 registry 测试通过）与我在代码上看到的一致。
仍然存在的阻塞：症状 (3) —— OSC 12 与多下标 OSC 4 逃过过滤，回放会把应答写进仍然存活的 shell
这一条我不是转述，而是在当前 head 的正则上直接核出来的。TERMINAL_QUERY_SEQUENCE_RE（:83-85）的 OSC 分支是：
\x1b\](?:10|11|4;[0-9]+);\?(?:\x07|\x1b\\)
它只覆盖 OSC 10、OSC 11，以及单下标形式的 OSC 4，且要求 ;? 之后紧跟 BEL 或 ESC \。因此：
- OSC 12（光标色查询）
\x1b]12;?\x07不在(?:10|11|4;[0-9]+)的候选里，原样通过strip()。 - 多下标 OSC 4 如
\x1b]4;0;?;1;?\x07：4;0;?之后跟的是;而不是\x07或\x1b\\，无法匹配，同样原样通过。
而按该 Critical 的实测，声明的客户端对这两者都会应答。于是它们会被记进 scrollback，每次重连回放都把它们重新应答进仍然存活的 shell 的 stdin —— 正是这个特性要消除的危害本身。这与「过滤清单在服务端手工枚举」是同一个根因：漏掉一族就漏掉一族，且漏掉的那一族恰好是会写回 stdin 的。
其余两个症状
- 症状 (2)（OSC 10/11/4 从两个出口都被删掉，但 pin 住的
@xterm/headless5.5.0 responder 结构上无法应答，导致 Neovim/bat/delta 的ESC ] 11 ; ?背景探测从「merge base 上浏览器会应答」变成「无人应答」）：我没有独立复核 responder 的能力边界，按现有 GitHub 数据记为作者已承认、仍未关闭。若成立，这是一条明确回归。 - 症状 (5)（responder 按自身 DECRQM 状态作答，36 个模式里 1 处与出货客户端分歧）：影响面小，我未独立复核。
关于结论分歧
chiga0 已在同一 head（16:34:27Z）APPROVED。我的结论不同，原因是本渠道的判定规则：只要有一条历史阻塞问题在当前 head 上仍然存在，就不能 Approve——而症状 (3) 我已在代码上确认仍成立，且作者本人也说明 (2)/(3)/(5) 未被本次缓解关闭。把结构性修复拆到 #11734 是合理的工程安排，但它不改变「当前 head 上这条 Critical 的一部分仍在」这个事实。
CI：Lint & Static、Test (ubuntu-latest, Node 22.x)、Integration Tests (no-AK, No Sandbox) 在当前 head 上均为 pass；review-pr pending，按策略不作为门禁。Test (windows-latest, Node 22.x) 为 skipped，而本 PR 的主题正是 Windows 上的 ConPTY 后端，真机确认仍欠着。
下一步
两个方向任选其一：
- 按线程里的结构性方案收口（也是我认同的方向）：不再在服务端改写字节，而是只在回放期间抑制应答——
serve/routes/terminal.ts已有CONTROL_FRAME_PREFIX的服务端到客户端控制帧通道，把 snapshot 作为{ type: 'replay', output }下发，在TerminalPanel.tsx里用replaying = true; term.write(output, () => { replaying = false; })包住写入并在该标志期间丢弃onData。这样现有与未来的所有查询族一次覆盖，两个正则和它们的逐角落测试都可以删掉；headless responder 保留给 bundled backend 的 DA 停顿，但收窄到它本来要解决的 DA 族，避免它按自身模式状态作答。 - 若本轮必须保留服务端过滤，则至少把 OSC 分支从手工枚举改成能覆盖 OSC 12 与多下标 OSC 4 的形式（例如按
OSC <数字>;?的通用查询形态匹配，并保留?谓词以免误删 SET），同时补上线程里点名的两个用例。
无论走哪条，请把症状 (2) 的回归一并给出结论：要么让 responder 能应答颜色查询，要么不要从客户端出口删掉这些查询。
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed at 77257a5acc (7 files, +858/-73) against ea10c604bf.
Verdict: the standing [Critical] R2-1 is stale at this head, and I could not reproduce any of its blocking witnesses. What is left is a wiring gap in the responder, which is non-blocking.
I transcribed the two regexes from this head and ran R2-1's own witnesses against them.
| R2-1 witness | as reported (at 1bd418b328) |
this head (77257a5acc) |
|---|---|---|
\x1b]0;title, then visible, then more visible text\r\n |
listener payloads ["","",""], snapshot "" |
\x1b]0;titlevisiblemore visible text\r\n -- nothing is swallowed |
DCS split after \x1bP, and after \x1bP$ |
the report measured leaks in that window | both deliver AFTER; the \x1bP\$? alternative closes them |
Symptoms (1) and (4) -- the two the report itself named as the urgent mitigation -- landed in f6c306adfd, which is one commit after the round-4 review at 1bd418b328. The thread is describing code that is no longer here, so it can be resolved.
One measured correction to that finding's evidence. Symptom (2) rests on grep -c "onColor(" returning 0 for the pinned headless build. That count is an artifact of the minified bundle: @xterm/headless 5.5.0 does ship the handlers -- setOrReportSpecialColor (x4) and setOrReportIndexedColor (x2) are present, and the OSC 4 / 10 / 11 / 12 handlers are registered. The real gap is narrower, and it is a channel mismatch:
// @xterm/headless 5.5.0, driven directly
t.write('\x1b]11;?\x07'); t.write('\x1b]10;?\x07'); t.write('\x1b]4;1;?\x07');
t.write('\x1b[6n'); t.write('\x1b[c');
// onData = ["\u001b[1;1R", "\u001b[?1;2c"] <- DSR + DA1 answered here
// onColor = [] <- colour reports do not touch this channel
_setOrReportSpecialColor only calls this._onColor.fire(...); it never routes the reply through the data event. This registry subscribes to onData only (web-terminal-registry.ts:507), so the OSC 10/11/4 arm of the strip removes those queries from the browser's stream while the server-side responder cannot answer them on the channel it uses. Either also forward onColor into proc.write, or drop the colour arm from TERMINAL_QUERY_SEQUENCE_RE -- as it stands that is the one family that is stripped with no server-side answerer. (xterm 6.0.0's browser build takes the same _onColor path and the web-shell app has no onColor subscription, so I could not establish that main answered colour probes either; treat this as an unproven benefit rather than a demonstrated regression.)
Also confirmed at this head:
- The earlier blocking item -- the responder constructed at 80x24 and never resized -- is fixed:
resize()now carriessession.queryTerminal?.resize(cols, rows). - R2-1 symptom (3) still stands:
strip('\x1b]12;?\x07')andstrip('\x1b]4;0;?;1;?\x07')return their input unchanged while\x1b]11;?\x07is removed. Inconsistent within one family, andmainis no worse, so it is a coverage gap rather than a regression. - The agent-view host sets
useConptyDll: win32with no inbox retry (packages/cli/src/agent-view/pty-host.ts:356) while the registry retries once. The description's reasoning -- a web terminal has nochild_processfallback -- applies equally there, so either add the same single retry or say in a comment why the two sites differ.
None of these block merge.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES
已核对 head 77257a5acc1d1e663f2b42433f0d3d21bb456bbf（vs merge-base ea10c604）。required 四项全部 success（Test (ubuntu-latest, Node 22.x)、Lint & Static、Integration Tests (no-AK, No Sandbox)、web-shell E2E Smoke）。ConPTY 后端切换、创建期取消、退出期资源释放与打包那几块我复核没有发现问题。
拦的是线程 R2-1（未解决，且作者自己写明「(2)/(3)/(5) 未被本轮缓解关闭，留档在 #11734」）。先说清楚已确认修好的部分：PARTIAL_ESCAPE_SUFFIX_RE（web-terminal-registry.ts:99）的 OSC 分支现在保留 ? 谓词、遇裸 ESC 即终止并接受 C1 ST \x9c，DCS 引导串 ESC P / ESC P$ 也被兜住。我在该 head 复跑基线 Tests 48 passed (48)，并分别注入两个变异：把 OSC 分支退回「任何未终止的 ESC ]」、删掉 |\x1bP\$? 这一支 → 各自恰好打红 1 条具名用例，所以「吃掉渲染字节」和「chunk 边界分裂」两条性质确实已经闭合。
阻塞项：剥离集合与服务端应答器的能力不匹配（症状 (2)(3)(5)，我实测）
应答器是 core 钉死的 @xterm/headless@5.5.0（packages/core/package.json:69；实例 web-terminal-registry.ts:328，其 onData 在 :506 回灌进 PTY）。我用本机这份 5.5.0 直接量各族查询的应答：
OSC 10;? / OSC 11;? / OSC 4;1;? / OSC 4;1;2;? / OSC 12;? -> ""（完全不应答）
CSI 6n -> ESC [ 1 ; 1 R
DECRQM ?2026;$p -> ESC [ ?2026;0$y
DECRQSS ESC P $ q ... ESC \ -> ESC P 0 $ r ESC \
而 TERMINAL_QUERY_SEQUENCE_RE（:85）剥掉的恰是 10|11|4;[0-9]+ 这三族颜色查询。于是这些探针既不再到达浏览器、服务端也答不出来，结果退化成代码里为「无应答器」写明的「查询被丢弃、最多等约 2 秒」——合并基线上它们本可得到应答。这是本 PR 引入的行为回归（Neovim / bat / delta 的背景色探测是直接触发方）。
同一处的另两点：OSC 12 与多索引 OSC 4;1;2;?... 不在交替式里，仍会原样落入 buffer 并在每次重连回放时被浏览器再应答进仍然存活的 shell stdin（即本功能要消除的害处对这两族依旧存在）；而剥掉查询等于把回答者从「正在使用的客户端」换成另一份状态的 headless —— 我用随包的 @xterm/xterm@6.0.0 对同一输入实测得到 ESC [ ?2026;2$y（识别），服务端给的是 ;0（不识别），应用据此会判定同步输出不可用而放弃使用。
不必等 #11734 的结构化方案，最小修法即可：把剥离集合收窄到「这份应答器确实会答的族」，暂时让 10|11|4（以及本就未剥的 12、多索引 4）继续到达浏览器，恢复合并基线行为；或为这几族补上服务端应答并在用例里钉住。若决定维持现状，请至少把「颜色查询此后不再被任何一侧应答」与 OSC 12/多索引 4 的处理差异写进 :66-84 的注释与 docs/——现在那段注释把这三族描述成已由服务端职责覆盖，与实测不符。
一处缓解自身缺见证（同一线程，顺手补）
把 strip() 里的 partial[0].length <= MAX_HELD_ESCAPE_CHARS 整个删掉（即恢复 R2-1 症状 (1) 里「pending 无上限、每个 chunk 重扫整段累积」的形态）后，48 条用例全部仍绿：256 字符封顶这一条目前没有可失败的见证。请补一条：喂入超过该长度且始终以 ESC ] / ESC P 前缀结尾的输出，断言前缀之外的字节仍然落到 buffer 与 readSnapshot()，并确认去掉封顶会让它变红。
其余
另外 17 条线程（readSnapshot 回放不再携带查询、退出期 releasePtyResources、ConPTY 探测、prepare-package.js 打包面等）逐条对过当前 head，作者回复所述与代码一致，本轮不再重复提出。review-pr 在本 head 仍 pending，属评审机器人自身档，不计入门禁。
…nnot answer The stateful stripper added here removed the OSC 10/11/12/4 colour queries from both the scrollback and the live listener, but the pinned @xterm/headless 5.5.0 responder answers none of them: `onData` carries DA/DSR/DECRQM/DECRQSS only, while `_setOrReportSpecialColor` reports on the internal `_onColor` emitter that the headless Terminal does not expose (`term.onColor` is undefined) and that nothing in this file subscribes to. Deleting the family from the browser's stream as well left a probing program unanswered, where the browser's own xterm.js 6.0.0 `_handleColorEvent` is the answerer and answered it at the merge base. Narrow both regexes to the families the responder actually answers (plus the never-display-content DA3 / XTVERSION / DECREQTPARM trio that no build answers), drop the half-covered colour arm from the partial matcher — so the strip set and the doc block no longer disagree about what the colour family is — and correct the doc block to state the split. Covered by a new test asserting the whole family reaches both the live listener and the snapshot untouched, and that nothing is written back; it fails if the colour arm is restored. The colour queries that a reconnect replay re-answers are part of the replay-suppression redesign tracked in #11734. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtyno0zh0n
Preserve raw PTY bytes and limit the Windows server responder to primary DA. Mark snapshots explicitly and restore history into a detached browser terminal while the old terminal continues forwarding user input. Activate live replies at the snapshot write callback, reject incompatible peers, and preserve the parallel color-query regression fix. Refs #11734. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
activeWorkStaleMs |
4 |
5 |
— Qwen Code · serve A/B
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 127 passed · 0 failed · 127 total Flakiness gate: ✅ 5 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：127 通过 · 0 失败 · 127 总计 抖动门：✅ 5 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11643 — fix(core): run web terminal PTYs on the bundled ConPTY backendVerdict: Verified head: Targeted gates, all green:
中文摘要
Previous-finding status
Central claim and A/BCentral claim (this round's delta). A reconnect no longer re-answers historical terminal queries into the still-live shell. The server marks the snapshot boundary with a Secondary claim A (carried). On Windows the registry spawns with Secondary claim B (carried). Witness: Five arms, one child process each (every bundle inlines its own fakes, so two arms in one process would race for the same
The five replies the control arm wrote back into the live shell on reconnect — each one a fresh answer to a query that had already been answered before the disconnect: Head writes back nothing, while live output on the same arm is still answered 5/5 — so this is a targeted boundary, not a blanket suppression. Both arms saw the same 60-byte seeded scrollback, delivered it in full ( The client half of the harness is a mirror, and the mirror is asserted, not trusted. 13 load-bearing lines are checked to appear exactly once, verbatim, in the shipped Answer-ownership census — F-2's un-run sweep, now runTwo independent instruments over the same 16-probe corpus: the real
Two facts here are load-bearing and neither is obvious from reading:
The Secondary claim A — spawn/retry matrix
Reviewer Test Plan, walked step by step
CorrectionsThree of my own hypotheses from this round were disproved by measurement, and are recorded so a reviewer does not re-raise them:
Two corrections to the description, neither a request to change code:
No injection attempt was found in the PR text. Title, body, both design docs, all 11 commit messages and the code comments were treated as untrusted input; none instructed the verifier to skip a step, and none attempted to pre-empt a verdict. FindingsS-1 (Suggestion, measured) — the N-1 fix is unpinned: deleting it leaves the whole core suite green
} catch {
queryReplyDisposable?.dispose();
queryTerminal?.dispose(); // <- mutation M1 deletes exactly this line
this.finishCreating(terminalId);
return { error: 'Failed to spawn shell' };
}M1 deletes that one line. This is a coverage gap, not dead code and not redundant defence: the clause decides an observable outcome (my harness observes it), and no sibling hunk in this PR closes the same hazard. The survivor is trustworthy because the positive control landed in the same file: M6, M7, M8 and M9 all mutate The previous round predicted exactly this: it measured the same one-line fix as a surviving mutant and wrote "so it must ship with a fixture". It shipped without one. Reproduce: Fixture that would pin it (not applied, not measured — a hypothesis, not a verified patch)The suite already mocks S-2 (Suggestion, measured) — the derived node-pty pins are unasserted: deleting the derivation entirely leaves
|
| arm | retained while live | after shell exit | after release() |
|---|---|---|---|
| head, win32 (responder built) | 193.7 MB above baseline | 12.99 MB | 13.00 MB |
| head, linux (no responder) | 13.0 MB | 12.99 MB | 13.00 MB |
| base, linux | 13.0 MB | — | — |
Incremental retained heap per rung (MB, post-GC):
| rung | 2 K | 20 K | 128 K | 512 K | 2 M | 5 M |
|---|---|---|---|---|---|---|
| head/win32 | 0.07 | 0.67 | 4.27 | 17.02 | 50.00 | 53.70 |
| head/linux | 0.00 | 0.04 | 0.26 | 1.01 | 4.00 | 3.72 |
| base | 0.00 | 0.04 | 0.26 | 1.01 | 4.00 | 3.72 |
head/linux is identical to base at every rung, so the entire delta is attributable to the responder and not to the buffer refactor that sits next to it in the same hunk.
Bounded honestly — what does NOT hold. This is not the leak the PR was filed against, and it is not permanent:
- It is freed in full on shell exit.
handleExit→releasePtyResources()→session.queryTerminal?.dispose(). Measured: win32 falls from 193.7 MB to 12.99 MB, which is exactly the linux arm's figure, while the session's 4,128,778 bytes of scrollback survive andexited:true— so the release costs the user nothing they were promised. release()frees it too (13.00 MB) and drops the session, so the heap was genuinely released rather than moved.- POSIX is unaffected — no responder is constructed off win32, and the census/H2 arms confirm identical retention and identical byte-for-byte output.
- It is not a native leak. This is JS heap in the daemon process, not
conhost.exe; it does not survive the CLI, and it is bounded by session lifetime and byMAX_CONCURRENT_WEB_TERMINALS = 8.
What does hold, and is the reason to report it: while a Windows terminal is live, retention scales with total output volume at roughly 15–33× the bytes fed, and it sits entirely outside MAX_BUFFER_BYTES — the 4 MB cap bounds the replay buffer, not the responder. Worst case across a full daemon is therefore ~8 × 180 MB ≈ 1.4 GB of JS heap for eight chatty Windows terminals, in a PR whose motivation sentence is "every exited web terminal leaked ~8 MB for the life of the CLI". The mechanism may well be worth that price — the responder is what makes the bundled backend's startup probe answerable — but the price is nowhere stated: not in the PR body's Risk & Scope, and not in docs/design/web-terminal-replay.md, which mentions the responder's existence and its fail-open behaviour but never its cost.
Not measured, and named so it is not over-read: whether retention plateaus beyond 8 MB (the per-rung ratio falls from 33× to 10× across the ladder, which is consistent with a plateau but does not prove one); which internal xterm structure holds it; and the shape under realistic output (my ladder is x repeated with no newlines, fed in 64 KB chunks).
Reproduce: cd <artdir> && node --expose-gc arms/h3-child.mjs --scenario buffer --arm registry-head --platform win32 --art $(pwd) then the same with --platform linux.
N-3 (nit, stands) — a failed headless load is still completely silent
Carried from the previous round, unchanged in substance: await loadXtermHeadless().catch(() => undefined) with no logging call anywhere in web-terminal-registry.ts and no user-visible surface for the degradation. Partially mitigated this round — handlesPrimaryDa is derived from the responder's existence, so the failure is now observable on the wire by anyone inspecting the snapshot control frame. The fail-open itself is the right call; only its silence is the finding, and it is now a smaller one.
Not covered
- Windows native behaviour — the PR's actual headline. Real
conhost.exe --headless/OpenConsole.exeaccounting, the real bundledconpty.dllload/unload throw, PowerShell first-output latency, and interactive rendering (vim, Ctrl+C, resize, CJK). Only@lydell/node-pty-linux-x64is installed here and node-pty selects its prebuild at import time, so my forced-win32arms verify the registry's decisions, never the native backend. This reproduces the shape of the Windows path, not the conhost leak [Windows] web-terminal PTYs leak conhost.exe on natural exit — inbox backend; shell fixed by #11497 #11352 was filed against. The author lists this as required manual Windows acceptance and I agree it is still required. - The real React component in a real browser. H2's client is a mirror of
TerminalPanel.tsx's message-handling logic driving the real@xterm/xterm6.0.0 under jsdom, with 13 source lines asserted verbatim-present and 5 controls asserting the base arm lacks them. It is notTerminalPanel.tsxitself: no React, noFitAddon, noResizeObserver, no real DOM swap (replaceChildren), no focus management, no theme propagation across the swap, and no realWebSocket(nodewsinstead). The author's own Chromium evidence covers that layer; I did not reproduce it. jsdom also needed a stubbed canvas 2D context andmatchMediafor xterm 6.0.0 toopen()at all — neither participates in parsing or reply generation, but a renderer-level defect would be invisible here. - Interrupted restoration, end-to-end. Test-plan step 3's "interrupt restoration and confirm a stale callback cannot replace the current view" was not driven at wire level; the PR's
disposes an interrupted replay and ignores its late write callbackcovers it against a mocked xterm, and my mirror reproduces the guard but I never fired the interrupt. - The responder-unavailable composition. Proven in two halves (registry test for the absent
handlesPrimaryDa; census forhandled=false→ browser answers DA1) but not driven as one end-to-end path. releaseDetachedWebTerminal. Verified by reading the route's branch order (therelease=1path returns before the replay gate) andbuildWsUrl(which setsreplay=1for the release URL too), not by driving it.- Agent-view end-to-end.
pty-host.tsis 5 added lines, 0 removed: one interface field plususeConptyDll: process.platform === 'win32'. Mutation M10 is killed by its own suite (37/37). Note it gates onprocess.platformwhile the registry gates onos.platform(); the two agree in production, and that split is what let my harness force the registry's decision while a POSIX shell ran. No A/B of the worker path. - Repo-wide gates.
npm run typecheckpassed (exit 0, every workspace,logs/typecheck.log) — see Corrections. Repo-wide lint, the full suites, and a trial merge into currentmainwere not run. A trial merge was not possible: the snapshot'sbaseRefOid(ea10c604…) exists as an object in this depth-2 checkout butgit merge-base --is-ancestorreports it is not an ancestor ofHEAD, and currentmainis not fetched, so I could not establish what actually lands. Per the merge-ref contract I usedHEAD^1(b5c7635f) as base throughout. - Per-commit attribution. Depth 2:
git rev-list HEAD^1..HEAD^2yields 1 commit while the metadatacommitsarray holds 11, andgit rev-parse --is-shallow-repositoryistrue. The aggregateHEAD^1..HEADdiff is what was verified; no per-commit table is presented. Theregistry-norecheckcontrol arm was reconstructed by an anchored source edit (asserted to match exactly once, and asserted to leave the identical 4-space guard pair aftergetPty()intact), not taken from a commit, so it is equivalent to head-minus-the-rechecks on the axes tested rather than identical to any historical tree. - Flakiness gate. The workflow's, not this round's; I did not re-run it. The previous round reported 3 files × 5 rounds with no divergence.
- Not counted in
assertions.json(scripted checks reported separately): the 14 arm-integrity controls inbuild-arms.mjs, the 11 mutations plus 3 same-file controls, the 172 targeted-gate tests, and the two design-doc language/structure checks.assertions.jsoncounts the two behavioural harnesses only: H2 57 + H3 70 = 127, of which 0 failed. Both design docs are present, reciprocally linked, and structurally synchronized (6 matching sections each, same order); a translation gap check found none.
Methodology
Linux node:22-bookworm container (node v22.23.2, npm 10.9.8, esbuild 0.25.6, @xterm/headless 5.5.0, @xterm/xterm 6.0.0, ws 8.21.3, @lydell/node-pty linux-x64 only), CI merge-ref checkout (HEAD = merge 842b28dc, HEAD^1 = base tip b5c7635f, HEAD^2 = PR head b7934e39), with npm ci and npm run build pre-existing at head. readlink -f node_modules/@qwen-code/qwen-code-core resolves to /__w/qwen-code/qwen-code/packages/core — i.e. the internal workspace link points at head — so every arm is an esbuild bundle of its own source root and cannot resolve through it; the base side came from a scratch git worktree at HEAD^1, removed afterwards (git worktree list shows one entry, git status --porcelain is empty). The PR touches no manifest or lockfile (git diff --name-only HEAD^1..HEAD -- package.json package-lock.json 'packages/*/package.json' is empty), so sharing the root node_modules is a clean control. Only two seams are substituted per arm — node:os (platform forcing, with the shim's own node:os import excluded from the redirect after it recursed) and utils/getPty.js (the recording fake, pid: 1 on purpose so killPtyTree()'s POSIX arm never signals a real process group); @xterm/headless, ws and node builtins stay external and real. The responder leak/race arms run under an ESM resolve hook (arms/hooks.mjs + arms/resolver.mjs) that redirects only the bare specifier @xterm/headless to a subclass of the real Terminal, so disposal is observed on a real object; the N-2 suspension point is a gate file the harness opens after issuing the interrupt, so there is no timer and no timing margin. H2 drives real ws loopback sockets end to end and its client is the real browser xterm 6.0.0 under jsdom (canvas 2D context and matchMedia stubbed, neither on the parse or reply path); its oracle is the bytes the fake PTY's write() received, i.e. what reached the live shell's stdin, plus readSnapshot() and the rendered buffer line. Harness-validity checks run inside every cell (window validity on both sides of the N-2 race, historyDelivered === seededSnapshotBytes, identical seeded scrollback across arms), so no cell can pass vacuously, and expected base-arm breakage is encoded as a passing assertion — fail counts only unexpected outcomes. Raw logs and harness sources live in logs/ (gates.log, typecheck.log, build-arms.log, h0-responder-census.log, probe-browser-xterm.log, h2.log + h2-assertions.json + five h2-<server>-<client>-<platform>.raw, h3.log + h3-assertions.json + 19 h3-*.raw, mutations.log + mutations.json, m11-control.log, print-ab.txt, print-carried.txt); harnesses are h2.mjs / h3.mjs / mutations.mjs / m11-control.mjs / h0-responder-census.mjs / probe-browser-xterm.mjs / print-ab.mjs / print-carried.mjs plus arms/ (build-arms.mjs, h2-child.mjs, h3-child.mjs, dom.mjs, hooks.mjs, resolver.mjs, xterm-counting.mjs, and the five built arm bundles). Evidence images in evidence/.
Flakiness gate log
rounds=5 files=5 skipped=0
file packages/cli/src/agent-view/pty-host.test.ts: (cd packages/cli) npx --no-install vitest run ./src/agent-view/pty-host.test.ts
file packages/cli/src/serve/routes/terminal.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/routes/terminal.test.ts
file packages/core/src/services/conpty-host.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/conpty-host.test.ts
file packages/core/src/services/web-terminal-registry.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/web-terminal-registry.test.ts
file packages/web-shell/client/components/terminal/TerminalPanel.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/terminal/TerminalPanel.test.tsx
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/agent-view/pty-host.test.ts: PPPPP
packages/cli/src/serve/routes/terminal.test.ts: PPPPP
packages/core/src/services/conpty-host.test.ts: PPPPP
packages/core/src/services/web-terminal-registry.test.ts: PPPPP
packages/web-shell/client/components/terminal/TerminalPanel.test.tsx: PPPPP
verdict: pass
summary: 5 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/routes/terminal.test.ts: P (exit 0)
round 1 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 1 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 1 · packages/web-shell/client/components/terminal/TerminalPanel.test.tsx: P (exit 0)
round 2 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/routes/terminal.test.ts: P (exit 0)
round 2 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 2 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 2 · packages/web-shell/client/components/terminal/TerminalPanel.test.tsx: P (exit 0)
round 3 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/routes/terminal.test.ts: P (exit 0)
round 3 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 3 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 3 · packages/web-shell/client/components/terminal/TerminalPanel.test.tsx: P (exit 0)
round 4 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/routes/terminal.test.ts: P (exit 0)
round 4 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 4 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 4 · packages/web-shell/client/components/terminal/TerminalPanel.test.tsx: P (exit 0)
round 5 · packages/cli/src/agent-view/pty-host.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/routes/terminal.test.ts: P (exit 0)
round 5 · packages/core/src/services/conpty-host.test.ts: P (exit 0)
round 5 · packages/core/src/services/web-terminal-registry.test.ts: P (exit 0)
round 5 · packages/web-shell/client/components/terminal/TerminalPanel.test.tsx: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅ The query stripper that the standing change-requests are anchored to no longer exists at this commit — R2-1 and the sandbox round's 12 failures lost their subject, and the DA/replay design checks out against the pinned xterm sources. Two asks before merge, neither blocking: refresh the PR body (Tested-on still marks Windows untested, and the design doc cites an approval date I cannot find in the thread), and dismiss the two stale CHANGES_REQUESTED reviews so this is not gated on findings about deleted code. Native Windows acceptance of the responder + replay path at this head is still the one open item.
11643 — state at the current head, and a re-review requestThe standing
So the change-request is stale: it predates both the fix and the approval. Requesting a fresh review of — 巡检 jmtyrycka0u (pr-closeout-sweep) |
Maintainer verification, round 2 —
|
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
Released in v0.23.4. |
8 participants
What this PR does
Windows web-terminal and agent-view PTYs use bundled ConPTY to address the inbox backend's natural-exit host leak. Web terminals retry once on the inbox backend if bundled spawn fails, and release the remaining worker resources after exit while retaining scrollback.
The review follow-up keeps terminal output intact. The Windows server answers only primary device-attribute startup probes; the browser owns colors, modes and geometry. Reconnect history is restored without forwarding its automatic replies, while the old visible terminal keeps accepting keyboard input. First-connection output and subsequent live output can still receive replies. This removes the hand-written query filters instead of adding more escape-sequence cases.
Why it's needed
#11352 tracks Windows terminal processes left behind after exit. Selecting bundled ConPTY changes who answers terminal probes, so the backend switch also needs to preserve live answers without injecting historical responses into the shell on reconnect. The original filtering approach lost some color answers and replayed others. #11734 tracks that follow-up; the cross-package replay contract was explicitly approved for this PR.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before this correction, real package probes at
77257a5showed OSC 11 losing its only answer and OSC 12/multi-index OSC 4 answering again on replay. The interveningd8975farestored live color queries; this change preserves that correction and removes replayed replies as well.After: the actual modified React terminal component, real xterm and FitAddon were mounted in Chromium with controlled surrounding contexts and WebSocket transport. Fresh output produced six color/mode/DCS replies, plus one DA only when browser-owned. On reconnect, 14 native historical reply events produced zero outgoing replies; a real keyboard
karrived once during delayed restoration and the queued live batch produced seven replies once. Canceled restoration and legacy-protocol rejection passed, with no browser errors. This is component-boundary evidence, not a real daemon/PTY or Windows process-count test.Tested on
Environment (optional)
Node 22.22.0, React 19.2.4, browser xterm 6.0.0, FitAddon 0.11.0, headless xterm 5.5.0 and Chromium 149.0.7827.55. Focused registry, WebSocket-route and terminal-component tests passed, as did ESLint on the six edited TypeScript files. Package typechecks encountered stale project-reference build declarations, including APIs already present in source; they are not reported as passing. No dependency installation or full rebuild was run in this round.
Risk & Scope
Linked Issues
Refs #11352 · Refs #11497 · Refs #11734 · microsoft/node-pty#965. No issue is automatically closed by this description.
中文说明
这个 PR 做了什么
Windows web-terminal 和 agent-view PTY 改用 bundled ConPTY，处理 inbox 后端在自然退出时遗留 host 的问题。web terminal 在 bundled spawn 失败时回退 inbox 重试一次，并在退出后释放剩余 worker 资源，同时保留 scrollback。
本次评审修正保留完整终端输出。Windows 服务端只应答启动时的 primary DA 探测，颜色、模式和几何查询交给浏览器。重连历史恢复时不转发自动应答，旧的可见终端仍接受键盘输入；首次连接输出和随后的实时输出仍能得到应答。删除手写查询过滤器，不再继续枚举转义序列。
为什么需要
#11352 跟踪 Windows 终端退出后残留进程的问题。切换 bundled ConPTY 会改变终端探测的应答方，因此还必须保留实时应答，并防止重连把历史应答重新注入 shell。原有过滤方案会丢失一部分颜色应答，也会重放另一部分。#11734 跟踪该后续问题；这次跨包回放协议已获明确批准，放在本 PR 中修正。
评审测试计划
如何验证
证据（前后对比）
修正前，
77257a5上的真实终端包探测显示 OSC 11 丢失唯一应答，OSC 12 和多下标 OSC 4 在回放时再次应答。期间提交的d8975fa已恢复实时颜色查询；本次保留该修复，并消除历史回放应答。修正后，在 Chromium 中挂载实际修改的 React 终端组件、真实 xterm 和 FitAddon，仅控制外围 context 和 WebSocket 传输。首次输出产生六次颜色/模式/DCS 应答，只有浏览器负责 DA 时才额外产生一次 DA。重连时观察到的 14 次原生历史应答没有任何一次向外转发；延迟恢复期间真实键盘
k到达一次，排队的实时查询产生七次应答且各一次。中断恢复、旧协议拒绝均通过，浏览器没有错误。这是组件边界验证，不是真实 daemon/PTY 或 Windows 进程计数测试。测试平台
环境（可选）
Node 22.22.0、React 19.2.4、浏览器 xterm 6.0.0、FitAddon 0.11.0、headless xterm 5.5.0、Chromium 149.0.7827.55。registry、WebSocket 路由和终端组件的定向测试通过，六个改动 TypeScript 文件的 ESLint 通过。包级类型检查遇到过期的 project-reference 构建声明，其中包含源码中已有的 API；不将类型检查报告为通过。本轮没有安装依赖或运行完整构建。
风险与范围
关联 Issue
Refs #11352 · Refs #11497 · Refs #11734 · microsoft/node-pty#965。本描述不自动关闭任何 issue。