fix(core): report signal-terminated shell commands as errors - #8501
Conversation
|
Re-run on Template: headings still deviate from the current template ( Problem: observed bug, not theory. #8491 carries a deterministic reproduction (a foreground command killed by an external signal classifies as success) and is labeled Direction: aligned. Correct failure classification is core agent reliability — the scheduler drives the model's next step from Size: 88 production lines ( Approach: minimal and focused. One shared Risk: Moving on to code review. 🔍 中文说明在 5 轮 review 和维护者运行时验证之后，对 模板：标题仍与当前模板不一致（ 问题：已观测到的 bug，不是理论问题。#8491 带有确定性复现（被外部信号杀死的前台命令被判定为成功），标签为 方向：对齐。准确的失败分类是 agent 可靠性的基础——调度器依据 规模：生产逻辑 88 行（ 方案：最小且聚焦。一个共享的 风险： 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal before reading the diff was the same one floated in the issue thread: add a signal branch to the foreground error chain guarded by Verified against the prior review rounds' ledger:
Extra scrutiny because this is a high-risk path: the registry's No critical findings. Every edit in the diff serves the stated goal. Test evidence — the PR's own CI on
|
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | ❌ failure — setup step, no tests ran |
| Test (macos-latest, Node 22.x) | ⏭️ skipped |
| Test (windows-latest, Node 22.x) | ⏭️ skipped |
| Integration Tests (CLI, No Sandbox) | ⏭️ skipped |
| web-shell E2E Smoke (ubuntu-latest) | ⏭️ skipped |
| Desktop Shell (ubuntu-22.04) | ✅ success |
| Classify PR / label / authorize / precheck-pr | ✅ success |
The one red check is not a test failure — the job dies in checkout setup, before any test executes:
##[error]Can't find 'action.yml', 'action.yaml' or 'Dockerfile' under
'/home/runner/work/qwen-code/qwen-code/.github/actions/verify-checkout-head'.
That local action exists on main but not in this fork tree, because the branch is 47 commits behind (compare main...96afcd9 → behind_by: 47, status diverged). The failure predates every test step, so it cannot be caused by this diff — and it cannot clear without a rebase onto current main.
- Not verified: the unit/integration suite has never executed on this commit (per above). The author's local results (294/294 core shell tests, 35/35 shellProcessor on a Windows checkout) remain the author's claim — fork code is never executed in triage.
- Independent behavioural evidence does exist in-thread: maintainer @wenshao built base-vs-head CLI bundles from source and drove both through a real terminal against a mock provider on a real node-pty, and reports the fix is correct (see the runtime verification report above). That is currently the strongest evidence for the behavioural claim.
- Bot-driven real-scenario testing (Stage 2c) applies to local invocations only, not CI runs. The sandboxed lane for this PR is already active: a
/verifyrun was triggered on this head at 03:43 UTC and has not posted its report yet — the claim it would settle is that the new regression tests actually fail on the base build, which no CI run on this head has demonstrated so far.
中文说明
代码审查
读 diff 之前我的独立方案与 issue 讨论中的思路一致：给前台错误链加一个由 !result.aborted 守卫的信号分支，并让所有消费方共用同一个"非零信号"谓词。PR 与该方案一致，并且在最关键的地方做得更好——它修的是生产者契约而不是逐个消费方打补丁：node-pty 干净退出的 signal: 0 在服务边界（前台结果构造与 promote 后 settle 触发两处）被归一化为 null，isSignalTermination 作为针对遗留/ mock 形状的防御保留。
对照此前各轮 review 的台账逐条验证：
- R4-3（signal 0 生产者契约问题）：已由
shellExecutionService.ts的边界归一化解决，并写入ShellExecutionResult字段文档。 - R4-2（终止判定存在三处内联副本）：已解决——现在由一个导出谓词覆盖所有位置：长时运行提示、展示消息、错误链、后台 settle 门与原因三元式，以及两个 CLI 消费方。
- R3-1（promoted shell 被
task_stop后又被重新分类为信号失败）：已由transitionRegistry中新增的entryAc.signal.aborted守卫解决，在分类器运行前把条目 settle 为cancelled。 - R4-1（测试在未 mock
process.kill的情况下 abort 了真实控制器）：新的task_stop测试对process.kill打了桩，并在 settle 前断言 SIGTERM→SIGKILL 升级。 - R5-1（失败原因三元式与其门条件不一致）：现在两者使用同一谓词。
由于命中高风险路径，额外审查了：registry 的 cancel / complete / fail 对非 running 条目都是 no-op，因此新增的无条件 registry.cancel 调用不会对已终态条目重复 settle；ShellPostPromoteSettleInfo.signal 的类型本就包含 NodeJS.Signals，字符串信号（'SIGTERM'）的 settle 测试反映的是真实契约；classifySettle 先查信号再查退出码，能正确把 node-pty 的 { exitCode: 0, signal: N } 被杀形状判为失败；!result.aborted 守卫把用户取消挡在错误路径之外（有测试钉住），isShellExitError 中 grep/rg 退出码 1 的白名单未受影响。
无阻塞级发现。diff 中每一处改动都服务于既定目标。
测试证据——该 PR 在 96afcd9 上的 CI
（见上方 CI 表格）
唯一的红色检查不是测试失败——任务在 checkout 安装步骤就终止了，任何测试都未执行。该本地 action 存在于 main，但此 fork 树中没有，因为分支落后 47 个提交（compare main...96afcd9 → behind_by: 47，状态 diverged）。失败发生在所有测试步骤之前，因此不可能由本 diff 引起——不 rebase 到当前 main 也无法转绿。
- 未验证：单元/集成套件从未在该提交上运行过。作者本地结果（Windows 检出上 core shell 测试 294/294、shellProcessor 35/35）仍属作者声明——triage 从不执行 fork 代码。
- 线程内存在独立的行为证据：维护者 @wenshao 从源码构建了 base 与 head 两个 CLI bundle，在真实 node-pty 上用真实终端配合 mock provider 驱动两者，报告修复正确（见上方运行时验证报告）。这是目前行为声明最有力的证据。
- 机器人驱动的真实场景测试（Stage 2c）仅适用于本地调用，不适用于 CI 运行。本 PR 的沙箱通道已激活：03:43 UTC 已对该 head 触发
/verify，报告尚未发布——它将验证的是新回归测试在 base 构建上确实失败，这是该 head 上任何 CI 运行都尚未证明的。
— Qwen Code · qwen3.8-max
Reviewed at 96afcd9ba53b2e72c27b1ca346e27e1221b1229a · re-run with @qwen-code /triage
|
Confidence: 4/5 — a clean, minimal fix with every review-round finding verifiably resolved and maintainer runtime A/B verification behind it; the last point withheld because this head's own CI never ran the suite (stale-branch setup failure) and the template headings still deviate. Stepping back: the approach matches my independent proposal and then improves on it — normalizing node-pty's The one thing standing between this PR and merge is not the code: the branch is 47 commits behind Approving, pinned to 中文说明置信度：4/5 —— 一个干净、最小的修复，每一轮 review 的发现都已可验证地解决，且有维护者的运行时 A/B 验证支撑；扣掉的一分是因为该 head 自己的 CI 从未跑过测试套件（分支过旧导致的安装步骤失败），且 PR 模板标题仍不一致。 退一步看：方案与我的独立提议一致，并且做得更好——把 node-pty 的 这个 PR 与合并之间唯一的障碍不是代码：分支落后 批准，钉在 — Qwen Code · qwen3.8-max Reviewed at |
e00d972 to
ea8e75a
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）为单个提交。 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— claude-opus-4-6 via Qwen Code /review (v0.21.5)
| : (result.exitCode === null && result.signal !== null) || | ||
| isShellExitError(this.params.command, result.exitCode) |
There was a problem hiding this comment.
[Suggestion] Missing !result.aborted guard — the new signal-only condition fires for user-cancelled commands too ({ aborted: true, exitCode: null, signal: 15 }). While the scheduler's abort-early-return path currently overrides at the model-facing level, endToolExecutionSpan is called before that early return, so the span telemetry already carries errorType: SHELL_EXECUTE_ERROR alongside cancelled: true. The sibling shouldAppendLongRunHint (line 2838) already uses !result.aborted for the same signal-vs-abort distinction.
| : (result.exitCode === null && result.signal !== null) || | |
| isShellExitError(this.params.command, result.exitCode) | |
| : (!result.aborted && | |
| result.exitCode === null && | |
| result.signal !== null) || | |
| isShellExitError(this.params.command, result.exitCode) |
— claude-opus-4-6 via Qwen Code /review (v0.21.5)
| : (result.exitCode === null && result.signal !== null) || | ||
| isShellExitError(this.params.command, result.exitCode) |
There was a problem hiding this comment.
[Critical] R1-1: The new guard never fires on the default PTY execution path — the linked issue's bug remains unfixed in the default configuration. PTY is the default foreground path on all POSIX platforms (shouldDefaultToNodePty() returns true and getShouldUseNodePtyShell() defaults to it), and the PTY resolve (shellExecutionService.ts:1891-1901) passes node-pty's numeric exitCode through — it is never null. A probe of the repo's pinned @lydell/node-pty prebuild shows the real shapes: normal exits → {exitCode: N, signal: 0} (3/3); SIGTERM/SIGKILL kills (external kill and self-kill, bash and direct binary) → {exitCode: 0, signal: 15|9} (6/6). An end-to-end probe through the real ShellExecutionService.execute PTY path confirmed an externally-killed command resolves {exitCode: 0, signal: N, aborted: false}: exitCode === null is false and isShellExitError(cmd, 0) is false → no error attached → reported as success. The fix works only on the non-default child_process fallback. — Failure scenario: a foreground command is OOM-killed, reaped by container eviction, or killed by a sibling process with PTY enabled (the default) → resolves {exitCode: 0, signal: 15} → no error → the model is told the command succeeded with Exit Code: 0 — the exact behaviour the linked issue reports.
One fix that covers both execution paths:
: (result.signal !== null && result.signal !== 0) ||
isShellExitError(this.params.command, result.exitCode)(child_process normal exits carry signal: null; PTY normal exits carry signal: 0; both kill shapes carry a non-zero signal.) Alternatively, normalize the PTY resolve in shellExecutionService so a signal-killed PTY child reports exitCode: null like the child_process path, and keep the current guard. Verify ConPTY's exit shape on Windows before settling on either.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| resolveShellExecution({ | ||
| output: 'completed', | ||
| exitCode: 0, | ||
| signal: 15, | ||
| aborted: false, | ||
| }); |
There was a problem hiding this comment.
[Critical] R1-2: This fixture pins a signal-killed shape as success. The probe evidence shows {exitCode: 0, signal: 15, aborted: false} is exactly what a SIGTERM-killed PTY child produces, while the genuine benign PTY exit reports {exitCode: N, signal: 0} (node-pty reports signal: 0 on normal completion — 3/3 probes; a clean WIFEXITED exit cannot carry signal 15). No code path produces {exitCode: 0, signal: 15, aborted: false} on a clean exit: the service never kills its own PTY on POSIX clean-exit, and the cancel path sets aborted: true. So this test asserts success for a real killed command — the opposite of this PR's intent. Flip proof: with the guard fixed to cover the PTY path this test fails while the signal-termination test still passes, pressuring a future fixer to revert the fix to keep the suite green. — Failure scenario: anyone fixing the PTY-path gap sees this test fail and reverts the fix to keep the suite green.
Use the genuine benign PTY shape (signal: 0 on a successful exit) as the fixture:
| resolveShellExecution({ | |
| output: 'completed', | |
| exitCode: 0, | |
| signal: 15, | |
| aborted: false, | |
| }); | |
| resolveShellExecution({ | |
| output: 'completed', | |
| exitCode: 0, | |
| signal: 0, | |
| aborted: false, | |
| }); |
Once the guard covers the PTY path, also add a test pinning {exitCode: 0, signal: 15} (a PTY signal kill) as SHELL_EXECUTE_ERROR.
— qwen3.8-max via Qwen Code /review (v0.21.5)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — the 'Qwen Code CI' test suite never ran on this commit (first-time-contributor workflow approval is pending) and its integration/E2E portion did not run locally; the unit scope was covered locally (build green, 291/291 in the PR's test file, full-suite failures measured pre-existing via base-tree delta).
— qwen3.8-max via Qwen Code /review (v0.21.5)
| : (!result.aborted && result.signal !== null && result.signal !== 0) || | ||
| isShellExitError(this.params.command, result.exitCode) |
There was a problem hiding this comment.
[Suggestion] The promote-path settle classifier is missing the signal guard this diff adds to the foreground path.
classifySettle (shell.ts ~3397-3408) checks info.exitCode === 0 → 'completed' before info.signal !== null. node-pty reports signal kills as {exitCode: 0, signal: N} (re-probed this round against the pinned @lydell/node-pty), and the PTY post-promote settle handler passes those raw values through (shellExecutionService.ts ~2128-2138). A signal-killed promoted PTY shell therefore settles as completed in the registry, in /tasks, and in the model-facing Status: completed copy. Both sibling classifiers now carry a signal guard — the new foreground ternary here and executeBackground's settle handler (~3717-3729) — except this one.
— Failure scenario: a user promotes a long-running foreground command; the child is then OOM-killed or reaped by container eviction → settle info {exitCode: 0, signal: 15} → classifySettle returns 'completed' → the model is told a killed command succeeded. The same misreport class this PR fixes, on the promote path.
This is pre-existing unchanged code, so it is follow-up material rather than a blocker for this scoped fix — but this diff leaves this classifier as the only member of the exit-classifier family without a signal guard.
Suggested follow-up — classify a real signal before the zero-exit branch:
if (info.signal !== null && info.signal !== 0)
return { status: 'failed', failMsg: `Terminated by signal ${info.signal}` };
if (info.exitCode === 0) return { status: 'completed', failMsg: null };— qwen3.8-max via Qwen Code /review (v0.21.5)
| : (!result.aborted && result.signal !== null && result.signal !== 0) || | ||
| isShellExitError(this.params.command, result.exitCode) |
There was a problem hiding this comment.
[Suggestion] The PR description still documents the previous round's condition and contradicts the shipped code.
The description says the fix will "Classify only signal-only results (exitCode === null && signal !== null)" and that "a result with a successful exit code remains successful even when PTY metadata also contains a signal" — but the shipped guard, and this PR's own test reports a PTY signal termination as a tool error, classify {exitCode: 0, signal: 15} as SHELL_EXECUTE_ERROR. The code is correct (a node-pty signal kill carries exitCode: 0, so the description's exitCode === null test could not catch it); the description is stale. On squash-merge the stale description enters the commit history.
— Failure scenario: a maintainer later triaging "my exit-code-0 PTY command is now reported as failed" reads the Compatibility/Risk section, concludes this is a regression the PR promised could not happen, and reverts or narrows the guard — re-opening the gap where signal-killed PTY commands are reported to the model as success (the exact behaviour of the linked issue).
Suggested fix: update the description's What/Solution/Compatibility sections before merge — any non-aborted foreground result with a non-null, non-zero signal is classified as SHELL_EXECUTE_ERROR on both execution paths; only signal: null (child_process normal exit) and signal: 0 (PTY normal exit) remain non-errors via the signal clause.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| : (!result.aborted && result.signal !== null && result.signal !== 0) || | ||
| isShellExitError(this.params.command, result.exitCode) |
There was a problem hiding this comment.
[Suggestion] The result.signal !== 0 exclusion encodes node-pty's "clean exit reports signal 0" convention, which contradicts the ShellExecutionResult.signal doc ("The signal that terminated the process, if any") and is explained nowhere in production code — only the new test name hints at it.
— Failure scenario: a maintainer cleaning up this ternary notices the child_process path can never produce signal: 0 (its mapping yields real signal numbers or null), concludes !== 0 is dead code, and removes it. On Linux/macOS, where foreground commands run under node-pty and every clean exit carries signal: 0, every successful foreground shell command becomes a SHELL_EXECUTE_ERROR — a blanket false-error regression.
Minimal fix (see the suggestion block): a one-line comment documenting the convention. Longer-term follow-up: normalise at the producer — map 0 → null in the PTY resolve sites in shellExecutionService.ts (~1891, ~2135) — which simplifies this guard to !result.aborted && result.signal !== null and also fixes the existing reader divergences (the Signal: 0 render for successful PTY commands at ~2761, and the long-run-hint suppression at ~2839 treating a successful PTY command as an external kill).
| : (!result.aborted && result.signal !== null && result.signal !== 0) || | |
| isShellExitError(this.params.command, result.exitCode) | |
| // node-pty reports signal 0 (not null) for clean PTY exits — treat 0 as "no signal". | |
| : (!result.aborted && result.signal !== null && result.signal !== 0) || | |
| isShellExitError(this.params.command, result.exitCode) |
— qwen3.8-max via Qwen Code /review (v0.21.5)
| if (info.signal !== null && info.signal !== 0) { | ||
| return { | ||
| status: 'failed', | ||
| failMsg: `Terminated by signal ${info.signal}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
[Critical] task_stop on a promoted foreground shell settles the registry entry as failed ("Terminated by signal 15") instead of cancelled — the new signal branch has no user-cancel guard, and this diff extends that misreport to the default PTY path (pre-PR such a stop settled as completed). task_stop → requestCancel → entryAc.abort() → cancelChild(), which SIGTERMs the process group and only calls registry.cancel() after the 200 ms SIGKILL-grace delay; the child dies within milliseconds and node-pty reports {exitCode: 0, signal: 15}, so the settle path reaches registry.fail while the entry is still 'running', and the delayed registry.cancel is a no-op (guarded on status === 'running'). Both sibling paths guard this case: the foreground classifier this same PR changes excludes result.aborted, and executeBackground's settle callback checks entryAc.signal.aborted first and records cancelled — the promote settle path is the only member of the family without the guard (ShellPostPromoteSettleInfo has no aborted field, so it must live in transitionRegistry). Probe-verified at this commit: abort-then-settle lands registry.fail('…', 'Terminated by signal 15', …) with zero registry.cancel calls until the grace delay elapses; adding the guard flips the probe to cancel. This also contradicts the PR description's "user cancellation ... retain their existing semantics". — Failure scenario: the model promotes a long-running command, then follows the tool's own instructions and calls task_stop on it → /tasks, the status sidecar, the user-facing "Background shell … failed." and the model-facing <task-notification> (<status>failed</status>, <result>Terminated by signal 15</result>) all report a deliberate user stop as a failure, which the model may then retry or try to "fix".
Mirror the executeBackground guard in transitionRegistry:
const transitionRegistry = (info: ShellPostPromoteSettleInfo) => {
if (entryAc.signal.aborted) {
registry.cancel(shellId, info.endTime);
return;
}
const cls = classifySettle(info);
// ...unchanged...
};plus a test that requestCancel + signal settle lands cancelled, not failed.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (info.signal !== null && info.signal !== 0) { | ||
| return { | ||
| status: 'failed', | ||
| failMsg: `Terminated by signal ${info.signal}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] This new check fully subsumes the pre-existing trailing if (info.signal !== null) branch ~12 lines below (identical failMsg body): the new branch catches every non-null signal, including string signals ('SIGTERM' !== 0), so the trailing branch is now reachable only for {error: undefined, exitCode: null, signal: 0} — a shape neither settle producer can emit (the PTY post-promote handler passes node-pty's numeric exitCode, never null; the child_process post-promote handler passes NodeJS.Signals | null, never numeric 0). If that shape ever did arrive, the entry would be reported as "Terminated by signal 0" — contradicting the signal-0-is-clean-exit invariant this PR's own comment states — instead of reaching the all-null fallback whose debugLogger.warn exists to make defective settle objects diagnosable. The rewritten decision-table comment also no longer mentions the trailing branch, so it now describes a table the code does not implement. — Concrete cost: every future reader must re-prove the second identical Terminated by signal branch is dead, and a future producer regression emitting that shape gets a contradictory failure report instead of the warn-logged unknown-status diagnostic.
Delete the trailing branch (behavior-preserving for every shape the service can emit today):
- if (info.signal !== null)
- return {
- status: 'failed',
- failMsg: `Terminated by signal ${info.signal}`,
- };— qwen3.8-max via Qwen Code /review (v0.21.6)
| // Decision table: `error` → fail (spawn-side failure); a non-zero | ||
| // signal means the process was killed (including node-pty's | ||
| // `exitCode: 0, signal: N` shape), then `exitCode === 0` → complete; |
There was a problem hiding this comment.
[Suggestion] The ShellPostPromoteHandlers.onSettle interface doc (packages/core/src/services/shellExecutionService.ts:297-300) still promises "natural exit (exitCode set, signal: null), signal kill (exitCode: null, signal set)" — contradicted for the PTY path by this PR's own comment and tests (PTY kills carry {exitCode: 0, signal: N}; clean PTY exits carry {exitCode: 0, signal: 0}). The doc predates this PR, but this diff makes the divergence actionable: it introduces the very branches (signal !== 0 guard, exitCode: 0 + signal: N handling) that the interface doc implies are dead code. — Concrete cost: a future maintainer wiring a new onSettle consumer or refactoring classifySettle reads the authoritative-looking contract, concludes the new handling is dead for "natural exit", and simplifies it away — silently re-introducing issue 8491 on the promote path, with no test for the doc-described (but wrong) shape to catch it.
Update the onSettle doc to state the PTY reality: natural PTY exits can carry signal: 0; PTY signal kills can carry exitCode: 0 with a non-zero signal; child_process settles keep the exitCode: null + string-signal shape.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // `task_stop` aborts the fresh registry controller before the | ||
| // child reports its SIGTERM/SIGKILL settle event. | ||
| entry.abortController.abort(); |
There was a problem hiding this comment.
[Critical] This new test aborts the real entryAc without mocking process.kill, so the production cancelChild listener delivers real signals to process group 12345 on the test machine: process.kill(-12345, 'SIGTERM') immediately on abort, then process.kill(-12345, 'SIGKILL') after the live 200 ms PROMOTE_CANCEL_SIGKILL_TIMEOUT_MS timer (probe-verified with a recording spy). resolveShellExecution defaults pid: 12345 and beforeEach forces platform 'linux', so the real kill path runs on every execution. Every sibling test exercising this kill path mocks process.kill (the only three spyOn(process, 'kill') sites in this file); this new test is the only abort-triggering one without it. — Failure scenario: a CI runner or dev machine with a live process group led by pgid 12345 (plausible in the default 32768 pid space of a busy runner) runs this test → the test SIGTERM/SIGKILLs an unrelated process group, potentially another CI job's processes.
Mirror the sibling tests:
vi.useFakeTimers();
const processKillSpy = vi
.spyOn(process, 'kill')
.mockImplementation(() => true);
try {
// ...existing test body...
await vi.advanceTimersByTimeAsync(250); // only if the SIGKILL leg matters
} finally {
processKillSpy.mockRestore();
vi.useRealTimers();
}— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (info.signal !== null && info.signal !== 0) { | ||
| return { | ||
| status: 'failed', | ||
| failMsg: `Exited with code ${info.exitCode}`, | ||
| failMsg: `Terminated by signal ${info.signal}`, |
There was a problem hiding this comment.
[Suggestion] The "what counts as a signal termination" decision now exists in three inline copies in this file; this PR patches two of them (the foreground check at :3021 and classifySettle here) and leaves the third divergent: the originally-background settle block in executeBackground (~3728-3741) still uses result.signal !== null with no !== 0 exclusion. Today it is shielded only by the hardcoded shouldUseNodePty: false at the background call site (:3673), since the child_process path never emits numeric 0. — Concrete cost: every change to signal semantics must be hand-applied to each copy (this PR is the second patch to this same decision table), and the moment a background shell runs under node-pty (a one-liner at the call site, since getShouldUseNodePtyShell() already enables PTY for the foreground path), every cleanly-exiting background shell would be reported failed with "terminated by signal 0" in /tasks, the status sidecar, and notifications.
Extract one module-private predicate and use it at all three sites:
function isSignalTermination(
signal: number | NodeJS.Signals | null,
): boolean {
return signal !== null && signal !== 0;
}— qwen3.8-max via Qwen Code /review (v0.21.6)
| : // node-pty reports signal 0 (not null) for clean PTY exits; only a | ||
| // non-zero signal represents an actual signal termination. | ||
| (!result.aborted && result.signal !== null && result.signal !== 0) || |
There was a problem hiding this comment.
[Suggestion] The node-pty signal: 0 clean-exit quirk is a producer-contract violation patched at consumer call sites; the normalization belongs in shellExecutionService.ts, which already normalizes the child_process shape (clean exit → signal: null, signal names → numbers via os.constants.signals at :960). Both PTY sites leak node-pty's 0 verbatim (signal: signal ?? null at :1893 and in the post-promote firePostSettle at :2137), even though ShellExecutionResult.signal is documented as "The signal that terminated the process, if any". This diff adds the !== 0 dance at two of the readers, but other live readers of the same field keep misreading a clean PTY exit as a signal kill (see the sibling comments on the long-run hint and shellProcessor.ts), and the new tests pin the quirk shape at the tool layer, so a later producer fix and the tool-layer expectations drift apart. — Concrete cost: every future reader of signal must repeat the !== 0 guard or misreport clean PTY exits.
Normalize at the producer: emit signal: signal || null (0 → null) at both PTY sites in shellExecutionService.ts, matching the child_process contract; the !== 0 clauses here and in classifySettle can then be dropped (and the onSettle JSDoc this PR just updated re-worded to match).
— qwen3.8-max via Qwen Code /review (v0.21.6)
| : // node-pty reports signal 0 (not null) for clean PTY exits; only a | ||
| // non-zero signal represents an actual signal termination. |
There was a problem hiding this comment.
[Suggestion] The sibling shell-result classifier in packages/cli/src/services/prompt-processors/shellProcessor.ts:221-223 (the !{cmd} prompt-injection path) lacks the signal-0 guard this diff adds, and it runs through the same PTY default (config.getShouldUseNodePtyShell() at shellProcessor.ts:197), so it misclassifies every clean PTY exit as a signal kill. The sibling defect is pre-existing in unchanged code — the diff itself is correct — but this PR establishes the signal-0 semantics for the classifier family and this is the one member still missing the guard (the other sibling, shellCommandProcessor.ts:291, is safe via a truthiness check). — Failure scenario: a user prompt containing !{npm test} runs on Linux/macOS (PTY default), the command succeeds and resolves {exitCode: 0, signal: 0} → the chain aborted (false) → exitCode !== 0 && exitCode !== null (false) → signal !== null (true for 0) appends [Shell command 'npm test' terminated by signal 0] to the model prompt for every successful injection.
Mirror the new guard in shellProcessor.ts:
} else if (executionResult.signal !== null && executionResult.signal !== 0) {(or fold into a follow-up issue if out of this PR's intended scope).
— qwen3.8-max via Qwen Code /review (v0.21.6)
| (!result.aborted && result.signal !== null && result.signal !== 0) || | ||
| isShellExitError(this.params.command, result.exitCode) |
There was a problem hiding this comment.
[Suggestion] Inside this same method, ~180 lines above, shouldAppendLongRunHint (:2836-2840) still answers "did the result carry a real signal?" with result.signal === null — missing the signal !== 0 exclusion this diff introduces. Pre-existing and untouched by this diff, but the same method now holds two contradictory answers to "was this result signal-terminated?" — the divergence this PR exists to remove. — Failure scenario: on Linux/macOS (PTY default), node-pty attaches a numeric signal to every exit (0 on clean exit), so result.signal === null is never true on the PTY path: a foreground npm run build that takes 90 s and exits cleanly ({exitCode: 0, signal: 0}, past the 60 s threshold) never gets the "next time use is_background: true" advisory — contradicting the comment block above the hint, which claims it fires on successful completions too.
Align the condition:
result.signal === null || result.signal === 0(or a follow-up issue).
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (info.signal !== null && info.signal !== 0) { | ||
| return { | ||
| status: 'failed', |
There was a problem hiding this comment.
[Suggestion] This new signal: 0 carve-out is pinned by no test — a mutation probe deleting && info.signal !== 0 keeps all 292 tests in the file green, while the identical mutation on the foreground path is caught by the new foreground test. The settle decision-table test also omits the child_process string-signal shape the diff's own doc comment documents (exitCode: null with a string signal); the tests' inline onSettle typing (signal: number | null) hides the NodeJS.Signals string variant the real service emits (shellExecutionService.ts:1269). The shipped code is correct — this is a regression-protection gap, not a current defect. — Failure scenario: a promoted PTY shell exiting cleanly fires onSettle({exitCode: 0, signal: 0}); a follow-up refactor deleting && info.signal !== 0 regresses the promoted-shell path silently (such a settle would transition the entry to failed — "Terminated by signal 0" — instead of completed in /tasks), with no test to catch it.
Extend the settle tests:
// PTY clean exit — signal 0 must stay 'completed'
onSettle({ exitCode: 0, signal: 0, endTime: 2 });
expect(registry.complete).toHaveBeenCalledWith(entry.shellId, 0, 2);
// child_process string-signal shape
onSettle({ exitCode: null, signal: 'SIGTERM' as NodeJS.Signals, endTime: 3 });
expect(registry.fail).toHaveBeenCalledWith(
entry.shellId,
'Terminated by signal SIGTERM',
3,
);(widening the captured onSettle param type to ShellPostPromoteSettleInfo['signal']).
— qwen3.8-max via Qwen Code /review (v0.21.6)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: reverse audit — stopped before round 2 by the review time budget.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| function isSignalTermination(signal: number | NodeJS.Signals | null): boolean { | ||
| // node-pty reports signal 0 for a clean exit; only a non-zero signal | ||
| // represents an actual signal termination. | ||
| return signal !== null && signal !== 0; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R4-3: Still-standing from round 4 — the node-pty signal: 0 clean-exit encoding is compensated for in every consumer (this module-private helper, the hand-inlined copy in shellProcessor.ts:221-224, and the truthiness check in shellCommandProcessor.ts:291) instead of being normalised at the two PTY producer sites in shellExecutionService.ts (foreground resolve ~:1894, post-promote settle ~:2138). The child_process path already normalises clean exits to null (:961), and this diff's onSettle doc change documents the leak into the contract while ShellExecutionResult.signal's field doc still asserts the null invariant. — Concrete cost: this bug class is precisely "consumer trusts the documented contract" — it already materialised twice in two packages (CLI prompt injection appending terminated by signal 0 to clean PTY exits; core suppressing the long-run hint and mis-settling {exitCode: 0, signal: N} kills). The next consumer of ShellExecutionResult.signal / ShellPostPromoteSettleInfo.signal must independently rediscover the PTY quirk or reintroduce either the false terminated by signal 0 report or the swallowed signal kill.
// shellExecutionService.ts — normalise at the two PTY emission sites:
signal: signal === 0 ? null : signal // foreground resolve (~1894) and post-promote settle (~2138)
// then this predicate degrades to `signal !== null` (or can be dropped),
// the shellProcessor.ts inline patch becomes unnecessary, and the
// ShellExecutionResult.signal field doc stays true.If the raw 0 is deliberately kept for observability, at minimum export one shared predicate next to ShellExecutionResult so both packages use one definition, and update the field doc.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| (result.exitCode !== null && result.exitCode !== 0) || | ||
| result.signal !== null | ||
| isSignalTermination(result.signal) |
There was a problem hiding this comment.
[Suggestion] The failure-reason ternary ~8 lines below this gate (:~3742) still discriminates with the pre-PR raw check result.signal !== null, while this hunk converts the enclosing fail gate to isSignalTermination(result.signal) — same block, two spellings of "had a signal". — Failure scenario: no divergence today (executeBackground hardcodes shouldUseNodePty: false, and the child_process path never yields signal 0), but probe-verified latent bug: if this path ever becomes PTY-capable, a background shell exiting {exitCode: 1, signal: 0} passes the outer gate via the exit-code arm while the reason ternary takes the signal branch and reports terminated by signal 0 instead of exited with code 1 in /tasks, the status sidecar, and the completion notification.
// :~3742 — use the shared predicate in the reason ternary too:
: isSignalTermination(result.signal)
? `terminated by signal ${result.signal}`
: `exited with code ${result.exitCode}`;— qwen3.8-max via Qwen Code /review (v0.21.6)
| !isSignalTermination(result.signal) && | ||
| elapsedMs >= longRunThreshold; |
There was a problem hiding this comment.
[Suggestion] The explanatory comment block ~25 lines above (:2818-2819) still states the hint is "Suppressed on external signal kills (result.signal != null …)" — now wrong for PTY clean exits (signal: 0, non-null), which this PR's own new test (appends the hint when PTY reports a clean exit with signal 0) pins as NOT suppressing. — Failure scenario: a maintainer debugging "why did/didn't this command get the long-run hint" reads the bullet and concludes a PTY clean exit suppresses the hint — the opposite of the new behaviour; a follow-up "fix" aligning code to comment would regress the PTY case this PR just fixed.
// update the bullet to:
// Suppressed on external signal kills (`isSignalTermination(result.signal)` —
// non-zero signal with `aborted: false`; note node-pty's `signal: 0` clean
// exit does NOT suppress), ...— qwen3.8-max via Qwen Code /review (v0.21.6)
| * child-process exit (`exitCode` set, `signal: null`), natural PTY | ||
| * exit (`exitCode` set, `signal: 0`), signal kill (which may carry |
There was a problem hiding this comment.
[Suggestion] This onSettle doc now describes the PTY signal shapes, but the sibling ShellExecutionResult field docs (:164-167) still assert the pre-PTY contract: signal is "The signal that terminated the process, if any." and exitCode is "null if terminated by a signal" — both now false for PTY (signal: 0 = clean exit; PTY signal kills can carry exitCode: 0, which the foreground resolve at ~:1894 provably passes through). — Failure scenario: the next consumer of execute() reads the field doc and writes signal !== null as the termination test — exactly the check this PR removes from shellProcessor.ts — and reports clean PTY exits as signal-killed (wrong prompt injection / wrong tool error).
// :164-167 — align the field docs, e.g.:
/** The signal that terminated the process, if any. Node-PTY reports `0`
* for a clean exit; only a non-zero value is a real signal termination. */
// and note on `exitCode` that PTY signal kills may still carry a numeric code.— qwen3.8-max via Qwen Code /review (v0.21.6)
| } else if (isSignalTermination(result.signal)) { | ||
| returnDisplayMessage = `Command terminated by signal: ${result.signal}`; |
There was a problem hiding this comment.
[Suggestion] This signal-termination branch of returnDisplayMessage is asserted by no test. The new foreground tests drive exactly this input shape (empty output, signal: 15, aborted: false) but only assert result.error and result.llmContent; error.message is built from llmContent, not returnDisplayMessage. Probe-verified: mutating the branch to else if (false && isSignalTermination(result.signal)) leaves all 294 tests in shell.test.ts green, and the assertion below fails with the mutation active / passes with it reverted. — Failure scenario: if this branch regresses or is dropped during future work on the shared predicate, a signal-killed foreground command with empty output renders an empty TUI status line while the model-facing copy still reports correctly — nothing in CI fires.
// in `reports a foreground signal termination as a tool error`:
expect(result.returnDisplay).toContain('Command terminated by signal: 15');— qwen3.8-max via Qwen Code /review (v0.21.6)
Runtime verification report — PR #8501I built two real CLI bundles from source (PR base vs. PR head) and drove both through a real terminal against a local mock OpenAI provider, on a real Verdict: the fix is correct, and it fixes more than the linked issue claims. Recommend merge once the branch is refreshed from Harness (click to expand)
Ground truth: what real node-pty actually reports on LinuxDriving
Two things follow, and they are the whole PR:
Finding 1 — Issue #8491 reproduced, and fixed
Verified on both result shapes:
The pre-PR TUI was self-contradicting: a green check and "terminated by signal: 15" on the same call.
Finding 2 —
|
| arm | model-facing Signal: |
long-run hint |
|---|---|---|
| BEFORE (PTY) | 0 |
absent |
| BEFORE (child_process) | (none) |
present |
| AFTER (PTY) | (none) |
present |
Finding 3 — !{...} in custom commands appended a bogus termination notice to every prompt
Custom command body PROBE_MARKER_START !{echo hello-from-injection} PROBE_MARKER_END, read off the wire:
BEFORE: PROBE_MARKER_START hello-from-injection
[Shell command 'echo hello-from-injection' terminated by signal 0] PROBE_MARKER_END
AFTER: PROBE_MARKER_START hello-from-injection PROBE_MARKER_END
Pre-PR this fired for every successful !{...} injection on PTY platforms — the model was told the command was killed when it had exited cleanly.
Finding 4 — promoted (Ctrl+B) background shells were misreported in /tasks
Foreground sleep 300 → Ctrl+B → task_stop bg_xxx:
| arm | notification | /tasks |
|---|---|---|
| BEFORE | … completed. |
[bg_9b75cbad] completed (exit 0) 48ms |
| AFTER | … was cancelled. |
[bg_32ee3f08] cancelled 40ms |
Same flow, but killed by an external signal instead of task_stop:
| arm | /tasks |
|---|---|
| BEFORE | completed (exit 0) |
| AFTER | failed: Terminated by signal 15 |
Note "48 ms" for a sleep 300 that the user explicitly stopped — pre-PR /tasks reported it as a clean success.
Regression controls — nothing else moved
Every row below was run on both arms end-to-end.
| scenario | BEFORE | AFTER |
|---|---|---|
echo hi |
success | success |
exit 3 |
error / shell_execute_error |
identical |
sleep 9 with timeout: 2000 |
error / execution_timeout |
identical |
yes | head -1 (SIGPIPE-heavy pipeline) |
success | success |
cat /dev/urandom | head -c 4 |
success | success |
echo abc | grep zzz (exit 1, not an error) |
success | success |
Esc cancel mid-command |
Request cancelled. |
identical |
! shell mode: echo / kill -TERM $$ |
✓ / x |
identical |
| promoted shell, natural exit | completed (exit 0) |
identical |
promoted shell, exit 7 |
failed: Exited with code 7 |
identical |
No false positives: SIGPIPE pipelines and grep-style exit 1 are untouched, timeouts still classify as execution_timeout (the new clause sits after timeoutSummary), and user cancellation still wins over the signal (!result.aborted guard).
Test quality — mutation testing
Baseline on the PR head, running the PR's four test files: core shell.test.ts + shellExecutionService.test.ts 429/429, cli shellProcessor.test.ts + shellCommandProcessor.test.ts 58/58. Each mutant was applied to the PR's production code with the PR's tests unchanged.
| # | mutation | result |
|---|---|---|
| M1 | isSignalTermination → signal !== null (drop the !== 0) |
killed by the 2 new cli tests; survives the core suite (see note 3) |
| M3 | drop !result.aborted && isSignalTermination(...) from the error chain |
killed (2) |
| M4 | restore the old classifySettle ordering (exitCode before signal) |
killed (1) |
| M5 | drop the entryAc.signal.aborted → registry.cancel guard |
killed (1) |
| M6 | drop signal === 0 ? null normalization (both sites) |
killed (2) |
| M7 | shellProcessor → executionResult.signal !== null |
killed (1) |
| M8 | shellCommandProcessor → result.signal !== null |
killed (1) |
| M9 | long-run hint → result.signal === null |
killed (1) |
| M10 | drop the !result.aborted guard |
killed (1) — the cancellation test |
| M8b | shellCommandProcessor → result.signal (exact pre-PR code) |
survives (expected, see note 2) |
Every production hunk except the shellCommandProcessor one is pinned by a test that fails when it is reverted.
Notes for the author / maintainer — none of these block merge
-
The red CI check is not this PR's fault.
Test (ubuntu-latest, Node 22.x)fails at the "Verify checkout includes expected head commit" step withCan't find 'action.yml' … under .github/actions/verify-checkout-head. That composite action does not exist at the PR's base3ad4fbdb7d; it was added tomainafterwards (mainis 45 commits ahead). Mergingmaininto the branch should turn it green. Per the force-push reminder bot on this thread, merge rather than rebase. -
The
shellCommandProcessor.tschange is behaviourally a no-op, not a fix: fornumber | null,result.signal(truthy) andisSignalTermination(result.signal)agree on every value. Mutant M8b restoring the exact pre-PR expression leaves the suite green, and the TUI output for!echo …and!kill -TERM $$is identical on both arms. It's worthwhile consistency hardening — just not a behaviour change, so the PR description slightly oversells that file. -
packages/core/src/tools/shell.test.tsmocksisSignalTerminationwith a hand-copied implementation (vi.mock(... isSignalTermination: (signal) => signal !== null && signal !== 0 ...)). That is why M1 survives the whole 294-test core suite — the core tests are structurally blind to the real predicate. The two new cli tests do cover it, so coverage is not actually missing, but considerimportOriginal/importActualfor the predicate so the guarantee lives next to the code it guards. -
Minor asymmetry between the two settle paths. The background path guards with
if (registry.get(shellId)?.status === 'running') registry.cancel(...); the new promoted guard callsregistry.cancel(...)unconditionally. Safe today —BackgroundShellRegistry.cancelre-checksstatus !== 'running'internally — but the two now read differently for the same concern. -
The new
entryAc.signal.abortedguard runs beforeinfo.error. A spawn-side error arriving after atask_stopwill be reportedcancelledrather thanfailed. Arguably correct (explicit user intent should win) — flagging it only so the choice is deliberate. -
A real cosmetic downside worth a follow-up (pre-existing, but this PR makes it far more visible). Once a shell call carries
error, the TUI renders the raw arguments JSON as the row title and the full 7-lineCommand/Directory/Output/Error/Exit Code/Signal/PGIDdump, instead of the conciseCommand terminated by signal: 15:BEFORE: ✓ Shell kill -TERM $$ Command terminated by signal: 15 AFTER: x Shell {"command":"kill -TERM $$","is_background":false} Command: kill -TERM $$ Directory: (root) … 5 more linesI confirmed this is not introduced here:
exit 3renders exactly the same way on the PR base. But signal kills now take that path, so more users will see it. A separate fix toreturnDisplayhandling for errored shell calls would be welcome.
中文版报告
运行时验证报告 —— PR #8501
我从源码构建了两个真实的 CLI bundle（PR base 与 PR head），并在真实 pty + 真实 node-pty 下、配合本地 mock OpenAI provider 完整跑通了两侧。以下是行为证据，不是对 diff 的复述。
结论：修复是正确的，而且实际修掉的问题比 issue 里描述的更多。建议在把 main 合入分支、让 CI 真正跑起来之后合并（红色 check 与本 PR 无关，见"备注 1"）。
环境
| BEFORE | 3ad4fbdb7d —— origin/main 与 PR 分支的 merge-base（即 PR 的真实 base） |
| AFTER | 96afcd9ba5 —— PR head |
| 构建 | 每侧独立 git worktree → packages/core/dist（tsc）→ npm run bundle（esbuild）→ 真实 dist/cli.js |
| 构建哨兵 | isSignalTermination 在 BEFORE bundle 中出现 0 次，在 AFTER bundle 中 14 次 |
| 平台 | Linux x64、Node v22.22.2、@lydell/node-pty 1.2.0-beta.10、走 PTY 路径（非 Windows 默认） |
| 模型 | 127.0.0.1:8501 的 mock OpenAI server，落盘每一次 /v1/chat/completions 请求体 |
| 状态判据 | --telemetry --telemetry-target=local → 可机读的 qwen-code.tool_call 事件（status / success / error_type） |
| TUI | tmux 真实 pty + capture-pane |
事实基准：真实 node-pty 在 Linux 上到底返回什么
直接驱动 @lydell/node-pty（不含任何 mock，也不经过 qwen-code）：
| 命令 | exitCode |
signal |
|---|---|---|
echo hi |
0 |
0 |
exit 3 |
3 |
0 |
kill -TERM $$ |
0 |
15 |
kill -KILL $$ |
0 |
9 |
由此得出两点，也正是这个 PR 的全部内容：
- 在 PTY 平台上，被信号杀死的前台命令
exitCode是0，isShellExitError()永远无法把它判为失败 → 即 issue bug(core): signal-terminated shell commands can report success #8491。 - 每一次 PTY 正常退出都带
signal: 0，而 PR 之前的代码把它当作"被信号终止"。这悄悄破坏了另外三处行为。
发现 1 —— issue #8491 复现并被修复
run_shell_command 执行 kill -TERM $$，tool_call 遥测：
| 分支 | status |
success |
error_type |
|---|---|---|---|
| BEFORE | success |
true |
— |
| AFTER | error |
false |
shell_execute_error |
两种结果形态都验证过：PTY（{exitCode: 0, signal: 15}，Linux/macOS 上实际发生的形态）与 child_process（{exitCode: null, signal: 15}，即 #8491 中引用的形态，通过 tools.shell.enableInteractiveShell: false 触发）。
PR 之前的 TUI 是自相矛盾的：同一次调用既显示绿色对勾，又显示 "terminated by signal: 15"。
发现 2 —— signal: 0 使 is_background 提示在所有 PTY 平台上彻底失效
shouldAppendLongRunHint 要求 result.signal === null，而在 PTY 上这永远不成立，因此 buildLongRunningForegroundHint() 在 Linux/macOS（以及 build ≥ 19042 的 Windows）上是不可达代码。我精确验证了这个机制：在 BEFORE 分支上，同一条命令强制走 child_process 路径（signal: null）时提示会出现，走 PTY 路径（signal: 0）时不会出现。
run_shell_command {command: "sleep 3 …", timeout: 4000}（阈值 = timeout/2 = 2000 ms）：
| 分支 | 模型看到的 Signal: |
长时运行提示 |
|---|---|---|
| BEFORE（PTY） | 0 |
缺失 |
| BEFORE（child_process） | (none) |
出现 |
| AFTER（PTY） | (none) |
出现 |
发现 3 —— 自定义命令中的 !{...} 会给每个 prompt 追加错误的终止提示
自定义命令内容 PROBE_MARKER_START !{echo hello-from-injection} PROBE_MARKER_END，从网络请求中读到：
BEFORE: PROBE_MARKER_START hello-from-injection
[Shell command 'echo hello-from-injection' terminated by signal 0] PROBE_MARKER_END
AFTER: PROBE_MARKER_START hello-from-injection PROBE_MARKER_END
PR 之前，在 PTY 平台上每一次成功的 !{...} 注入都会触发这行 —— 命令明明干净退出，却告诉模型它被杀死了。
发现 4 —— 被提升（Ctrl+B）到后台的 shell 在 /tasks 中状态错误
前台 sleep 300 → Ctrl+B → task_stop bg_xxx：
| 分支 | 通知 | /tasks |
|---|---|---|
| BEFORE | … completed. |
[bg_9b75cbad] completed (exit 0) 48ms |
| AFTER | … was cancelled. |
[bg_32ee3f08] cancelled 40ms |
同样的流程，但改为被外部信号杀死（而非 task_stop）：
| 分支 | /tasks |
|---|---|
| BEFORE | completed (exit 0) |
| AFTER | failed: Terminated by signal 15 |
注意那个 sleep 300 只跑了 48 ms 就被用户显式停止 —— PR 之前 /tasks 却把它报告为干净成功。
回归对照 —— 其余行为均未变化
下表每一行都在两侧做了端到端运行。
| 场景 | BEFORE | AFTER |
|---|---|---|
echo hi |
success | success |
exit 3 |
error / shell_execute_error |
一致 |
sleep 9 + timeout: 2000 |
error / execution_timeout |
一致 |
yes | head -1（大量 SIGPIPE 的管道） |
success | success |
cat /dev/urandom | head -c 4 |
success | success |
echo abc | grep zzz（exit 1，不算错误） |
success | success |
命令执行中按 Esc 取消 |
Request cancelled. |
一致 |
! shell 模式：echo / kill -TERM $$ |
✓ / x |
完全一致 |
| 提升后的 shell 自然退出 | completed (exit 0) |
一致 |
提升后的 shell exit 7 |
failed: Exited with code 7 |
一致 |
没有误报：SIGPIPE 管道与 grep 式的 exit 1 不受影响；超时仍归类为 execution_timeout（新分支位于 timeoutSummary 之后）；用户取消仍然优先于信号（!result.aborted 守卫）。
测试质量 —— 变异测试
PR head 基线（跑 PR 涉及的四个测试文件）：core shell.test.ts + shellExecutionService.test.ts 429/429，cli shellProcessor.test.ts + shellCommandProcessor.test.ts 58/58。每个变异体都施加在 PR 的生产代码上，测试保持不变。
| # | 变异 | 结果 |
|---|---|---|
| M1 | isSignalTermination → signal !== null（去掉 !== 0） |
被 2 个新增 cli 测试杀死；在 core 套件中存活（见备注 3） |
| M3 | 从错误判定链中去掉 !result.aborted && isSignalTermination(...) |
杀死（2） |
| M4 | 恢复 classifySettle 的旧顺序（exitCode 在 signal 之前） |
杀死（1） |
| M5 | 去掉 entryAc.signal.aborted → registry.cancel 守卫 |
杀死（1） |
| M6 | 去掉 signal === 0 ? null 归一化（两处） |
杀死（2） |
| M7 | shellProcessor → executionResult.signal !== null |
杀死（1） |
| M8 | shellCommandProcessor → result.signal !== null |
杀死（1） |
| M9 | 长时运行提示 → result.signal === null |
杀死（1） |
| M10 | 去掉 !result.aborted 守卫 |
杀死（1）—— 即取消场景的测试 |
| M8b | shellCommandProcessor → result.signal（完全等同 PR 之前的写法） |
存活（符合预期，见备注 2） |
除 shellCommandProcessor 那处外，每个生产改动都有一个"回退即失败"的测试钉住。
给作者 / 维护者的备注 —— 均不阻塞合并
-
红色 CI 与本 PR 无关。
Test (ubuntu-latest, Node 22.x)挂在 "Verify checkout includes expected head commit" 步骤：Can't find 'action.yml' … under .github/actions/verify-checkout-head。该 composite action 在 PR base3ad4fbdb7d上并不存在，是之后才加进main的（main已领先 45 个提交）。把main合入分支即可转绿。按本线程中 force-push 提醒机器人的建议，用 merge 而非 rebase。 -
shellCommandProcessor.ts的改动在行为上是 no-op，不是修复：对number | null而言，result.signal（真值判断）与isSignalTermination(result.signal)在所有取值上一致。变异体 M8b 恢复 PR 之前的原始表达式后套件依然全绿，且!echo …与!kill -TERM $$的 TUI 输出在两侧完全相同。作为一致性加固是有价值的，但它不是行为变更，PR 描述在这一处略有夸大。 -
packages/core/src/tools/shell.test.ts用手抄的实现 mock 了isSignalTermination（vi.mock(... isSignalTermination: (signal) => signal !== null && signal !== 0 ...)）。这正是 M1 能在 294 个 core 测试中存活的原因 —— core 测试在结构上对真实谓词是"盲"的。两个新增的 cli 测试确实覆盖了它，所以覆盖并未真正缺失，但建议对该谓词改用importOriginal/importActual，让保证与被保护的代码待在一起。 -
两条 settle 路径存在轻微不对称。 background 路径的守卫是
if (registry.get(shellId)?.status === 'running') registry.cancel(...)，而新增的 promoted 守卫无条件调用registry.cancel(...)。目前是安全的 ——BackgroundShellRegistry.cancel内部会再次检查status !== 'running'—— 但同一件事现在有两种写法。 -
新增的
entryAc.signal.aborted守卫排在info.error之前。 若 spawn 侧错误在task_stop之后到达，会被报告为cancelled而不是failed。这大概率是对的（用户的显式意图应当优先），提出来只是希望这是个有意识的选择。 -
一个值得后续跟进的观感问题（本身是既有问题，但本 PR 让它明显得多）。 一旦 shell 调用带上
error，TUI 会把原始参数 JSON 作为行标题、并渲染完整的 7 行Command/Directory/Output/Error/Exit Code/Signal/PGIDdump，而不是简洁的Command terminated by signal: 15：BEFORE: ✓ Shell kill -TERM $$ Command terminated by signal: 15 AFTER: x Shell {"command":"kill -TERM $$","is_background":false} Command: kill -TERM $$ Directory: (root) … 还有 5 行我确认这不是本 PR 引入的：在 PR base 上
exit 3的渲染方式完全相同。但信号终止现在也走这条路径，会有更多用户看到。建议单独修一下 errored shell 调用的returnDisplay处理。
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (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: 995 passed · 0 failed · 995 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：995 通过 · 0 失败 · 995 总计 Verification reportPR #8501 Deep Verification —
|
| command | exitCode | signal | aborted | (both arms) |
|---|---|---|---|---|
kill -TERM $$ |
null |
15 |
false |
— |
kill -KILL $$ |
null |
9 |
false |
— |
echo hello && exit 0 |
0 |
null |
false |
— |
exit 3 |
3 |
null |
false |
— |
Harness B — tool-level A/B (ab-tool.test.ts, real ShellToolInvocation.execute() + real service; only Config faked, mirroring shell.test.ts scaffolding; expectations arm-encoded so predicted base behavior is a passing assertion). Witness: 02-ab-tool-cells-head-vs-base.png.
| cell | base (7edc16b) | head (96afcd9) |
|---|---|---|
1. kill -TERM $$ |
error UNDEFINED → scheduler success (the bug), returnDisplay: 'Command terminated by signal: 15' |
error DEFINED, message contains Signal: 15 |
2. kill -KILL $$ |
error UNDEFINED | error DEFINED, Signal: 9 |
3. echo verify-ok (positive control, both arms) |
no error | no error |
4. exit 3 (pre-existing behavior control) |
error DEFINED | error DEFINED |
5. user-cancel mid-sleep |
no error, cancelled |
no error, cancelled |
2/5 cells flip broken→fixed; 3/3 controls unchanged. Result: head 5/5, base 5/5 (predictions held).
Secondary claims — PR's own tests run on BOTH arms (the PTY branches are exercised through the real service code with mocked pty events, since node-pty is absent here):
| suite | head | base (head's test files copied in) |
|---|---|---|
core shell.test.ts + shellExecutionService.test.ts |
429/429 | 422 pass, 7 fail — exactly the new/changed tests (witness 03-base-arm-sevens-reds.png) |
cli shellProcessor.test.ts + shellCommandProcessor.test.ts |
58/58 | 57 pass, 1 fail — exactly the signal-0 prompt test |
The 7 base-arm core reds: foreground-signal error, PTY-signal error, signal-0 normalization, post-promote settle signal-0, long-run hint under signal 0, classifySettle {exitCode: 0, signal: 15} ordering, task_stop guard. Base failure output quotes the injected [Shell command 'cmd' terminated by signal 0] prompt text — the prompt-injection bug observed, not inferred. treats a child-process signal string as a failed settle passes on base too (base's signal !== null already caught strings) — correctly reported as pre-existing behavior.
Mutation matrix (vacuity)
Every mutant ran against head with positive controls live (unmutated suites green: 429 core / 58 cli). Witness for M1: 04-m1-classification-revert-kills-tests.png.
| # | mutation | suite | result | failure mode observed |
|---|---|---|---|---|
| M1 | revert error-classification hunk in shell.ts |
core shell.test.ts |
KILLED (2 tests) | expected undefined to deeply equal { message: StringContaining… } — the intended behavioral assertion |
| M2 | revert signal-0 normalization in shellExecutionService.ts |
shellExecutionService.test.ts |
KILLED (1) | expected +0 to be null |
| M3 | delete entryAc.signal.aborted guard in transitionRegistry |
core shell.test.ts |
KILLED (1) | registry.cancel never called |
| M4 | revert shellProcessor.ts predicate to !== null |
cli shellProcessor.test.ts |
KILLED (1) | [Shell command 'cmd' terminated by signal 0] injected |
| M5 | predicate → signal !== null (drop !== 0) |
core suites | SURVIVED 429/429 | see classification below |
| M5 | same mutant | cli shellProcessor.test.ts |
KILLED (1) | signal-0 prompt test red |
M5 survivor classification: coverage gap, not dead code — the clause is decidable (the CLI kill proves it), but core's shell.test.ts mocks shellExecutionService.js wholesale and re-implements isSignalTermination inside the mock factory, so the core suite can never see a regression in the real export. The real predicate's !== 0 clause is pinned cross-package by the CLI suite alone. Completeness reporting, not a merge condition.
Predicate type-boundary sweep (predicate-boundary.mjs, witness 05-predicate-boundary-sweep.png): null→false, 0→false, 15/9/'SIGTERM'/'SIGKILL'/-1→true, undefined→true (see Findings).
Corrections
- To the PR description's Testing section ("
npm run typecheck --workspace=@qwen-code/qwen-code— not verified: the checkout has pre-existing missing@qwen-code/acp-bridgeexports/modules and Ink selection type errors"): in the verification environment (Linux,npm ci+ full build at HEAD) the CLI typecheck is clean, 0 errors, and core typecheck is clean too. The author's errors were artifacts of their checkout state (unbuilt workspacedist/), not pre-existing source errors. The Compatibility/Risk section's "Full CLI typecheck … unverified" is therefore over-cautious; both affected workspaces typecheck at head. No code change requested — correcting the record only.
Findings (non-blocking)
- Nit —
isSignalTermination(undefined)returnstrue(undefined !== null && undefined !== 0). Unreachable through every typed boundary:ShellExecutionResult.signalandShellPostPromoteSettleInfo.signalexcludeundefined, and both settle paths normalize withsignal === 0 ? null : (signal ?? null). Documented as defensive for "legacy or mocked result objects"; a mocked object omittingsignalwould classify as terminated. No action required. - Coverage topology — the real predicate is pinned only cross-package. M5 shows core's suites stay green against a broken predicate because
shell.test.ts's module mock re-implements it. If the CLIshellProcessortests are ever mocked away, the!== 0clause becomes unpinned. Consider one core-side test that imports the real export (cheap follow-up, not a merge condition). - Note — the
shellCommandProcessor.tshunk is behaviorally equivalent hardening. Base's truthy checkresult.signalalready treats0as falsy and strings as truthy, so the new predicate changes nothing observable there; its new test passes on base as well as head (measured). Harmless consistency with the shared predicate. - Note — base-arm CLI typecheck in the scratch worktree was blocked by unbuilt sibling
dist/(42 × TS6305 cascade). Environmental artifact of the cheap base control, not a PR property; head's clean typecheck is the operative gate.
Not covered
- Per-commit attribution: the PR has 7 commits per
$QWEN_VERIFY_CONTEXT; the depth-2 checkout makes only the merge/base/head reachable (git rev-list HEAD^1..HEAD^2returns 1 at the shallow boundary). Verified the aggregateHEAD^1..HEADdiff; per-commit behavior was out of reach. - Real node-pty runtime: not installed in this container (
require('node-pty')fails;getPty()returns null → child_process fallback). PTY branches were driven through the real service code with mocked pty events (the PR's harness style). This reproduces the handling of the documented{exitCode: 0, signal: 0}node-pty shape, not a live node-pty producer. - End-to-end promote /
task_stopagainst a live daemon (Ctrl+B mid-flight): pinned by unit tests + mutation kills (M3), not exercised at TUI/daemon level. - Repo-wide test suite, repo-wide typecheck beyond the two affected workspaces, integration tests, Windows paths (the changed semantics are POSIX signal classification; Windows kill paths untouched).
scripts/verify-capture.mjsworked for all 5 captures; no gaps.
Methodology
Environment: GitHub-hosted node:22-bookworm container, merge-ref checkout (HEAD = merge commit, HEAD^1 = base tip 7edc16b, HEAD^2 = PR head 96afcd9), npm ci + npm run build pre-run at HEAD; node-pty absent. Base arm: git worktree add tmp/base-tree HEAD^1 with root node_modules symlinked; the root @qwen-code/qwen-code-core workspace link points at the head tree, so base-arm runs avoided it by construction — core tests import sources relatively, and CLI vitest aliases @qwen-code/qwen-code-core to the base tree's own ../core/index.ts (alias resolution verified from the worktree's own config). Realpath assertions and the unchanged lockfile (PR touches no package.json/package-lock.json) keep the A/B a pure code comparison. Harnesses drove code mock-free at the unit under test: Harness A via tsx against each tree's real shellExecutionService.ts with real spawned processes; Harness B via vitest with the real tool + real service (only Config faked). Mutation reverts were applied with exact-match text replacement, confirmed single-hunk by git diff --stat, and restored with git checkout -- (0 dirty files after each). Raw logs in logs/; harnesses in this directory (ab-service.mjs, ab-tool.test.ts, predicate-boundary.mjs).
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. ✅
|
Released in v0.21.8. |
4 participants
What this PR does
Reports signal-terminated foreground shell commands as tool errors while preserving normal PTY exits and user cancellation semantics, including promoted foreground shells.
Problem
A signal-terminated foreground command can resolve as
{ exitCode: null, signal: 15, aborted: false }. The result formatter displayed the signal, but the scheduler could treat the command as successful. A promoted foreground shell stopped throughtask_stopcould also report its later SIGTERM/SIGKILL settle event as a failure after cancellation. PTY clean exits additionally reportsignal: 0, which must not be treated as a termination.Root Cause
Signal classification was repeated across foreground execution, promoted-shell settlement, background settlement, long-run hints, and CLI prompt injection. The paths did not share one non-zero-signal predicate, and the prompt processor treated every non-null signal as termination. The promoted-foreground settle transition also needed to give an already-aborted cancellation state priority over a later signal callback.
Solution
Use one module-private predicate that treats only non-zero signals as termination. Apply it consistently to foreground and promoted/background classification, long-run hints, and prompt status injection. Preserve
signal: 0as the node-pty clean-exit convention, and keep an aborted promoted entry incancelledwhen its signal settle arrives later.Changes
task_stopcancellation over delayed promoted-shell signal settlement.signal: 0as a failure, signal termination, or prompt status error.task_stoppromoted results.Testing
npm test --workspace=@qwen-code/qwen-code-core -- src/tools/shell.test.ts --run— passed 294/294.npm test --workspace=@qwen-code/qwen-code -- src/services/prompt-processors/shellProcessor.test.ts --run— passed 35/35.npx prettier --check ...on all 5 changed files — passed.npx eslint ...on all 5 changed files — passed.npm run typecheck --workspace=@qwen-code/qwen-code-core— passed.npm run typecheck --workspace=@qwen-code/qwen-code— not verified: the checkout has pre-existing missing@qwen-code/acp-bridgeexports/modules and Ink selection type errors outside this diff.git diff --check— passed.npm run test:integration:no-ak:sandbox:none— not verified: this Windows checkout lacks the builtdist/cli.jsrequired by the integration runner, so E2E cases stop at the executable precondition.Compatibility / Risk
The change is limited to shell result classification, prompt status formatting, documentation, and promoted-shell lifecycle settlement. Non-zero signal termination is surfaced as failure on the relevant paths; clean PTY exits with
signal: 0, normal child-process exits, timeouts, and user cancellation retain their existing semantics. No dependencies or public APIs changed. Full CLI typecheck and integration behavior remain unverified because of checkout-wide environment/dependency preconditions.Notes for Reviewer
Please review the shared non-zero-signal predicate, the signal-before-exit-code ordering, the
task_stopcancellation guard, and the fixtures modeling node-pty{ exitCode: 0, signal: 0 }clean exit versus{ exitCode: 0, signal: N }termination. The follow-up remains within the same shell signal-classification and cancellation bug family.Linked Issue
Fixes #8491