fix(core): pass verbatim arguments to cmd.exe for command hooks on Windows - #9357
C0d3N1nja97342 wants to merge 6 commits into
Conversation
…ndows Command hooks run via `cmd.exe /d /s /c <command>` on Windows. The spawn in HookRunner omitted windowsVerbatimArguments, so Node's MSVC CRT escaping mangled quotes and backslashes in the command string - e.g. `bash "C:\Program Files\...\script.sh" arg` was passed to cmd with the quoted path corrupted, and every command hook with a quoted path failed entirely (not just extension hooks: project/user/extension level). Set `windowsVerbatimArguments: process.platform === 'win32' && shell === 'cmd'`, mirroring shellExecutionService (which already does this). cmd parses the original command line; PowerShell keeps the default escaping so args round-trip through CommandLineToArgvW. QwenLM#8649
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the fix — the root-cause analysis is solid, and mirroring the windowsVerbatimArguments handling the shell execution service already uses is a sensible direction.
Before this can move to code review, the PR body needs to follow the PR template. Several required sections are missing:
- Why it's needed — you have "Root cause" / "Change" instead; please fold that content under the template heading.
- Reviewer Test Plan — with How to verify, Evidence (Before & After), and the Tested on table. This is a Windows-only spawn behavior change, so the OS table matters: mark ✅ for the Windows A/B you described, and state the macOS/Linux status explicitly (the flag is
win32-only, so⚠️ / N/A is fine — just say so). - Risk & Scope — main risk/tradeoff, what was not validated, breaking-change notes.
- Chinese translation in the
<details>block.
@C0d3N1nja97342 once the PR body follows the template, re-run triage and this will continue — the change itself is small and worth reviewing.
中文说明
感谢这个修复——根因分析很扎实，参照 shell 执行服务已有的 windowsVerbatimArguments 处理方式也是合理的方向。
在进入代码审查之前，PR 描述需要按照 PR 模板 补全。目前缺少几个必需章节：
- Why it's needed——目前写的是 "Root cause" / "Change"，请把内容归到模板标题下。
- Reviewer Test Plan——包含 How to verify、Evidence (Before & After) 和 Tested on 表格。这是一个仅 Windows 的 spawn 行为变更，OS 表格很重要：请把你描述的 Windows A/B 标为 ✅，并明确标注 macOS/Linux 的状态（该标志仅
win32生效，标⚠️ / N/A 即可——注明就行）。 - Risk & Scope——主要风险/权衡、未验证的内容、破坏性变更说明。
<details>中的中文翻译。
@C0d3N1nja97342 请按模板更新 PR 描述后重新触发 triage，流程会继续——这个改动本身很小，值得评审。
— Qwen Code · qwen3.8-max
|
@qwen-code /review |
…dows-verbatim-args
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.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group-only job) and the win32+cmd branch of the new gate was not exercised locally (local runs were Linux-only).
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group-only job) and the win32+cmd branch of the new gate was not exercised locally (local runs were Linux-only)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const spawnOptions = mockSpawn.mock.calls[0][2]; | ||
| expect(spawnOptions.windowsVerbatimArguments).toBe( | ||
| process.platform === 'win32' && shell.shell === 'cmd', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The gating test computes its expectation from the live process.platform and the real getShellConfiguration() instead of mocking them — on the Linux/macOS runners that gate PRs it degenerates to expect(false).toBe(false), and the win32+cmd → true branch (the exact behavior #8649 fixes) is never deterministically pinned. The sibling precedent the test's own comment cites (shellExecutionService.test.ts:3242-3320) mocks the platform and shell config and asserts literals. — Failure scenario: a gate mutation (&& → ||, dropping the shell === 'cmd' conjunct — which would wrongly enable verbatim args for PowerShell hooks — or an always-false regression that silently re-breaks #8649) keeps the Linux PR gate green because the expectation mirrors the same live expression; the Windows unit-test job runs only in the merge queue, so such a regression passes PR CI. Probe-verified on this runner: the &&→|| and always-false mutants both pass the PR's test, while a mocked-literal probe flips (fails against the mutant, passes against the original).
// Follow the sibling pattern (shellExecutionService.test.ts):
vi.spyOn(process, 'platform', 'get').mockReturnValue('win32');
// mock getShellConfiguration to a cmd config, then:
expect(spawnOptions.windowsVerbatimArguments).toBe(true);
// add a powershell-config case asserting toBe(false)中文说明
[建议] 该门控测试直接用真实的 process.platform 和真实的 getShellConfiguration() 计算期望值，而不是对它们做 mock —— 在作为 PR 门禁的 Linux/macOS runner 上，断言退化为 expect(false).toBe(false)，win32+cmd → true 分支（即 #8649 所修复的行为本身）从未被确定性地固定。测试注释中引用的同类先例（shellExecutionService.test.ts:3242-3320）会 mock 平台和 shell 配置并断言字面量。— 失败场景：门控发生变异（&& → ||、丢掉 shell === 'cmd' 合取项——这会错误地为 PowerShell 钩子启用 verbatim 参数——或恒为 false 的回归悄悄重新破坏 #8649）时，由于期望值镜像了同一份运行时表达式，Linux PR 门禁依然为绿色；Windows 单元测试任务仅在 merge queue 中运行，因此这类回归可通过 PR CI。已在本 runner 上用探针验证：&&→|| 与恒 false 两个变异体都能通过该 PR 的测试，而 mock 字面量探针可以翻转（对变异体失败、对原始实现通过）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| windowsVerbatimArguments: | ||
| process.platform === 'win32' && shellConfig.shell === 'cmd', |
There was a problem hiding this comment.
[Suggestion] The win32 && shell === 'cmd' verbatim-args policy is now hand-copied per spawn site — shellExecutionService.ts:801 (childProcessFallback), shellExecutionService.ts:~1498 (node-pty arg-form choice), and this diff — instead of living at its natural depth in shell-utils.ts, and the copies have already started diverging in spelling (process.platform here vs os.platform() in the service). — Failure scenario: demonstrated cost, not hypothetical — packages/core/src/tools/monitor.ts:366-384 spawns via the identical spawn(executable, [...argsPrefix, command], ...) shape from getShellConfiguration() with no windowsVerbatimArguments, so the exact #8649 bug (CRT escaping mangling quoted paths under Windows cmd) persists there today. Any future policy adjustment must be applied to every copy in lockstep; missing one re-opens the #8649 bug class on the missed path. Suggested fix: extract the policy into shell-utils.ts (e.g. needsVerbatimArguments(shell: ShellType) or extend ShellConfiguration with the spawn-options fragment) and consume it from hookRunner and shellExecutionService, so monitor.ts can pick it up too (fixing monitor.ts itself is out of this PR's scope).
// shell-utils.ts
export function needsVerbatimArguments(shell: ShellType): boolean {
return isWindows() && shell === 'cmd';
}
// hookRunner.ts
windowsVerbatimArguments: needsVerbatimArguments(shellConfig.shell),中文说明
[建议] win32 && shell === 'cmd' 的 verbatim 参数策略现在是按 spawn 调用点手工复制的——shellExecutionService.ts:801（childProcessFallback）、shellExecutionService.ts:~1498（node-pty 参数形态选择）以及本 diff——而不是放在它自然的归属层 shell-utils.ts；而且各份复制在写法上已经开始分叉（这里用 process.platform，service 里用 os.platform()）。— 失败场景：这是已证实的代价而非假设——packages/core/src/tools/monitor.ts:366-384 以完全相同的 spawn(executable, [...argsPrefix, command], ...) 形态从 getShellConfiguration() 启动子进程，却没有设置 windowsVerbatimArguments，因此同样的 #8649 缺陷（Windows cmd 下 CRT 转义破坏带引号路径）至今仍存在于该路径。未来任何策略调整都必须同步应用到每一处复制；漏掉一处就会在被漏掉的路径上重新打开 #8649 这类缺陷。建议修复：将策略提取到 shell-utils.ts（如 needsVerbatimArguments(shell: ShellType)，或为 ShellConfiguration 增加 spawn 选项片段），并在 hookRunner 与 shellExecutionService 中消费，monitor.ts 也能随之复用（monitor.ts 本身的修复不在本 PR 范围内）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // quotes and backslashes, which breaks quoted paths (e.g. | ||
| // `bash "C:\Program Files\...\script.sh"`). PowerShell uses the | ||
| // default escaping so args round-trip through CommandLineToArgvW. | ||
| // Mirrors shellExecutionService. #8649. |
There was a problem hiding this comment.
[Suggestion] This PR claims Fixes #8649, but the #8649 thread assigned the fix to a different, still-open PR (#8646) using a competing mechanism (falling back to powershell -NoProfile -Command when the resolved global shell is cmd), and this PR never reconciles with it; #8646's own body also declares Fixes #8649. — Failure scenario: merging this PR auto-closes #8649 while the maintainer-designated tracking PR is mid-flight; #8646's hookRunner.ts patch modifies getShellConfigForHook — the exact function this PR's gate reads — adding a cmd→powershell fallback, after which a hook's shellConfig.shell can never be 'cmd', making this PR's gate dead for hooks; the two PRs also collide in getShellConfigForHook and the hook spawn path. Suggested fix: before merge, reconcile with the maintainer and #8646's author — either get explicit agreement that this PR supersedes #8646's hook-shell work (and update #8646's scope), or change the link to Related to #8649 so the issue stays open until the tracked work lands.
中文说明
[建议] 本 PR 声明 Fixes #8649，但 #8649 的讨论串已把该修复指派给另一个仍然开放的 PR（#8646），且两者机制相互竞争（当解析出的全局 shell 为 cmd 时回退到 powershell -NoProfile -Command）；本 PR 从未与之协调，#8646 的描述中同样声明了 Fixes #8649。— 失败场景：合并本 PR 会在维护者指定的跟踪 PR 仍在进行时自动关闭 #8649；#8646 的 hookRunner.ts 补丁修改的正是本 PR 门控所读取的 getShellConfigForHook，加入 cmd→powershell 回退后，钩子的 shellConfig.shell 将永远不可能是 'cmd'，本 PR 的门控对钩子而言即成死代码；两个 PR 还在 getShellConfigForHook 与钩子 spawn 路径上直接冲突。建议修复：合并前与维护者及 #8646 作者协调——要么明确达成由本 PR 取代 #8646 钩子 shell 工作的共识（并同步调整 #8646 的范围），要么把链接改为 Related to #8649，让 issue 在跟踪的工作落地前保持开放。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…gate Round-1 review suggestions: - R1-2: the 'win32 && shell === cmd' verbatim-args policy was hand-copied per spawn site (shellExecutionService twice, hookRunner once) and already drifting in spelling. Extract needsVerbatimArguments(shell) into shell-utils.ts and consume it from hookRunner. - R1-1: the hookRunner gating test computed its expectation from the live process.platform + real shell config, so on Linux CI it degenerated to expect(false).toBe(false) and never pinned win32+cmd. Pin the gate deterministically in shell-utils.test.ts via the mocked os.platform. QwenLM#8649
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed diff-only — the PR’s existing discussion could not be fetched, so this is not an approval and not a no-blockers claim. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-3 hookRunner wiring-test assertion degenerates on non-Windows runners — already reported (comment 3796338752)
中文说明
仅审查了 diff——无法获取 PR 已有的讨论，因此这不构成批准，也不构成"无阻断问题"的结论。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| * escaping so args round-trip through CommandLineToArgvW. Shared by every | ||
| * cmd.exe spawn site so the policy cannot drift. #8649. |
There was a problem hiding this comment.
[Suggestion] The new needsVerbatimArguments helper's JSDoc claims it is "Shared by every cmd.exe spawn site so the policy cannot drift", but at this commit the helper has exactly one production caller (hookRunner.ts:612). A repo-wide sweep confirms the rest of the policy is still unshared: shellExecutionService.ts:801 keeps the identical policy inline (windowsVerbatimArguments: isWindows && shell === 'cmd'), a second inline predicate of the same shape exists on the node-pty path (shellExecutionService.ts ~:1498), and monitor.ts's spawn (:369–370) sets no windowsVerbatimArguments at all. — Failure scenario: a future change to the verbatim policy is made once in needsVerbatimArguments; a maintainer trusting this comment does not sync shellExecutionService.ts's inline copies, so the shell tool and hooks silently diverge on argument quoting — exactly the drift the comment declares impossible — and the comment also masks that monitor commands with quoted paths on Windows cmd.exe still hit the same MSVC CRT mangling this PR fixes for hooks.
If migrating shellExecutionService.ts:801 is out of scope here, narrow the comment to match reality at minimum:
| * escaping so args round-trip through CommandLineToArgvW. Shared by every | |
| * cmd.exe spawn site so the policy cannot drift. #8649. | |
| * escaping so args round-trip through CommandLineToArgvW. Used by the hook | |
| * spawn site; keep in sync with shellExecutionService's inline policy. #8649. |
中文说明
问题：新增 needsVerbatimArguments 助手的 JSDoc 声称 "Shared by every cmd.exe spawn site so the policy cannot drift"，但本提交中该助手只有一个生产调用方（hookRunner.ts:612）。全仓扫描确认其余策略仍未共享：shellExecutionService.ts:801 仍内联完全相同的策略（windowsVerbatimArguments: isWindows && shell === 'cmd'）；node-pty 路径（shellExecutionService.ts 约 :1498）存在第二个同形内联谓词；monitor.ts 的 spawn（:369–370）完全没有设置 windowsVerbatimArguments。
失败场景：未来对 verbatim 策略的修改只会改 needsVerbatimArguments 一处；信任该注释的维护者不会去同步 shellExecutionService.ts 的内联副本，导致 shell 工具与 hooks 的参数转义行为悄悄分叉——正是该注释声称不可能发生的漂移；同时该注释还会掩盖 monitor 命令在 Windows cmd.exe 下带引号路径时仍会出现本 PR 为 hooks 修复的同类 MSVC CRT 转义损坏（#8649 同类问题）的事实。
修复建议：将 shellExecutionService.ts:801 迁移为 windowsVerbatimArguments: needsVerbatimArguments(shell)（一行改动即可使注释对非 PTY 路径成立）；若不在本 PR 范围内，至少把注释收窄为实际范围（见上方 suggestion），并为 monitor.ts 的缺失单独开 follow-up issue。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
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 |
|
@qwen-code /review |
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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-3 wiring-test expectation derives from live process.platform, degenerating on non-Windows runners — already reported (comment 3796338752)
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group-only job) and the win32 spawn behavior it exercises cannot run on this Linux reviewer; unit tests pin the gate only via an os.platform mock.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/shell-utils.test.ts:1281 — [review] describe('needsVerbatimArguments') nested inside describe('getShellConfiguration') against the file's one-top-level-describe-per-export convention — deferred (code unchanged since …
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group-only job) and the win32 spawn behavior it exercises cannot run on this Linux reviewer; unit tests pin the gate only via an os.platform mock。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * escaping so args round-trip through CommandLineToArgvW. Shared by every | ||
| * cmd.exe spawn site so the policy cannot drift. #8649. |
There was a problem hiding this comment.
[Suggestion] R1-1: This JSDoc claims the helper is "Shared by every cmd.exe spawn site so the policy cannot drift", but at this commit it still has exactly one production caller — the hookRunner spawn this PR adds. Two sibling spawn sites contradict the claim: shellExecutionService.childProcessFallback (shellExecutionService.ts:801) keeps its own inline duplicate (windowsVerbatimArguments: isWindows && shell === 'cmd'), and monitor.ts:370 spawns the shell with the identical spawn(executable, [...argsPrefix, command], {...}) shape while passing no windowsVerbatimArguments at all. If the verbatim policy ever changes (a new shell type, a Windows escaping edge case), the edit will land in this unit-tested helper on the reasonable belief that every cmd.exe spawn follows it, while the sibling sites silently keep the old behavior — the exact drift the comment promises is impossible. Meanwhile a Windows monitor command with a quoted path still suffers the same #8649 mangling this PR fixes for hooks. Either route the other cmd.exe child-process spawn sites through needsVerbatimArguments(shell) (shellExecutionService already imports from shell-utils; monitor.ts would also destructure shell), or, until every site is migrated, soften the JSDoc to describe reality:
| * escaping so args round-trip through CommandLineToArgvW. Shared by every | |
| * cmd.exe spawn site so the policy cannot drift. #8649. | |
| * escaping so args round-trip through CommandLineToArgvW. Used by hookRunner; | |
| * other cmd.exe spawn sites are not yet migrated. #8649. |
中文说明
[Suggestion] R1-1：这段 JSDoc 声称该 helper "被所有 cmd.exe spawn 调用点共享，策略不会漂移"，但在此提交中它仍然只有一个生产调用点——即本 PR 为 hookRunner 添加的 spawn。另外两个兄弟 spawn 调用点与该声明矛盾：shellExecutionService.childProcessFallback（shellExecutionService.ts:801）仍保留自己的内联副本（windowsVerbatimArguments: isWindows && shell === 'cmd'）；monitor.ts:370 以完全相同的 spawn(executable, [...argsPrefix, command], {...}) 形态启动 shell，却完全没有传 windowsVerbatimArguments。将来如果 verbatim 策略需要变更（新增 shell 类型、Windows 转义边界情况），修改会落在这个有单测覆盖的 helper 上——人们会理所当然地以为所有 cmd.exe spawn 都遵循它——而兄弟调用点会悄悄保留旧行为，注释所承诺的"不会漂移"恰恰不可能成立。同时，带引号路径的 Windows monitor 命令仍会遭遇本 PR 为 hooks 修复的同类 #8649 参数破坏。建议：让其余 cmd.exe 子进程 spawn 调用点也改用 needsVerbatimArguments(shell)（shellExecutionService 已从 shell-utils 导入；monitor.ts 还需额外解构出 shell）；或者，在所有调用点迁移完成之前，把 JSDoc 弱化为描述现状。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
…ws-verbatim-args Resolve packages/core/src/hooks/hookRunner.ts: main split the hook spawn into a detached supervisor path (MessageDisplay/StopFailure/ SessionDelete) and a direct path. Keep main's structure and re-apply this branch's windowsVerbatimArguments to the direct spawn. The supervisor path re-spawns the shell inside SURVIVING_HOOK_SUPERVISOR_SOURCE and is left untouched here.
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.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group-only job) and the win32+cmd spawn behavior this PR changes cannot be exercised on this Linux reviewer; the gate is pinned only via os.platform mocks plus the PR author's local Windows A/B.
Not reviewed: build-and-test — Test (ubuntu-latest, Node 22.x) is failing in CI at the reviewed commit; the failing suites are in files this diff does not touch and could not be A/B-measured against the merge base (base tree failed to build — infrastructure timeout).
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group-only job) and the win32+cmd spawn behavior this PR changes cannot be exercised on this Linux reviewer; the gate is pinned only via os.platform mocks plus the PR author's local Windows A/B。
未审查：build-and-test — Test (ubuntu-latest, Node 22.x) is failing in CI at the reviewed commit; the failing suites are in files this diff does not touch and could not be A/B-measured against the merge base (base tree failed to build — infrastructure timeout)。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| // cmd.exe needs verbatim arguments (Node's MSVC CRT escaping | ||
| // mangles quoted paths); PowerShell keeps the default escaping. | ||
| // #8649. | ||
| windowsVerbatimArguments: needsVerbatimArguments(shellConfig.shell), |
There was a problem hiding this comment.
[Suggestion] R3-1: The verbatim-arguments fix lands on only one of the two spawn paths in executeCommandHook. Hooks for MessageDisplay / StopFailure / SessionDelete events take the survivesParentExit branch, where the eval'd supervisor (SURVIVING_HOOK_SUPERVISOR_SOURCE, inner spawn(executable, args, ...) at ~hookRunner.ts:204) re-spawns the same shell with [...argsPrefix, command] and no windowsVerbatimArguments. On Windows with the default ComSpec=cmd.exe, a hook command containing a quoted path — e.g. bash "C:\Program Files\tools\cleanup.cmd" arg, quoting mandatory under a directory with a space — then has its quotes mangled by Node's MSVC CRT escaping and fails with '\"C:\Program Files\tools\cleanup.cmd\"' is not recognized, the exact failure this PR fixes for every other hook event, while the identical command configured as PreToolUse works. The supervisor code is pre-existing (it came in from main and this diff does not touch it), so this records a gap rather than blocking the change; but the PR description's "this affects every command hook" and the new JSDoc's "Shared by every cmd.exe spawn site" both read as covering this path, and the #8649 failure class stays alive for the three surviving events.
Witness:
probe at HEAD (real HookRunner.executeHook, mocked spawn):
ARM-A [SessionDelete] outer spawn cmd === process.execPath: true
supervisor source contains "windowsVerbatimArguments": false
supervisor inner args: ["-c","bash \"C:\\Program Files\\tools\\cleanup.cmd\" arg"]
ARM-B [PreToolUse] spawn opts has windowsVerbatimArguments key: true
flip arm: adding windowsVerbatimArguments: true to the supervisor inner spawn
flipped the probe (buggy-state assertion failed, 1 failed | 1 passed)
(The win32 CRT-mangling step itself cannot execute on this Linux reviewer; it is traced against Node's documented win32 spawn behavior and the PR's own Windows A/B for the identical direct-spawn shape.)
Suggested fix — pass the policy into the supervisor as data (the eval'd CommonJS string cannot import the ESM helper): append the boolean to the supervisor argv and set the option on its inner spawn:
// hookRunner.ts — producer spawn (~line 1104-1117): append one argv element
JSON.stringify(needsVerbatimArguments(shellConfig.shell)),
// SURVIVING_HOOK_SUPERVISOR_SOURCE — destructure it, set the option
hook = spawn(executable, args, {
cwd: process.cwd(),
env: hookEnv,
stdio: [inputFd, 'ignore', 'ignore'],
shell: false,
detached: process.platform !== 'win32',
windowsVerbatimArguments: verbatimArgsValue === 'true',
});The supervisor argv is a positional contract — built at hookRunner.ts:1104-1117 and destructured via process.argv.slice(1) inside SURVIVING_HOOK_SUPERVISOR_SOURCE (hookRunner.ts:72-79), and the eval source can only require Node builtins (hookRunner.ts:68) — so the new element must be appended and destructured on both sides together, and PowerShell must keep the flag false. A hookRunner.test.ts case for a surviving event (e.g. HookEventName.SessionDelete) asserting the supervisor argv carries the verbatim flag (true only on win32+cmd) must go red when the propagation is removed.
中文说明
[建议] R3-1：verbatim 参数修复只落在 executeCommandHook 两条 spawn 路径中的一条。MessageDisplay / StopFailure / SessionDelete 事件的钩子走 survivesParentExit 分支：eval 出的监督进程（SURVIVING_HOOK_SUPERVISOR_SOURCE，其内部 spawn(executable, args, ...) 位于约 hookRunner.ts:204）以同样的 [...argsPrefix, command] 重新启动 shell，却没有设置 windowsVerbatimArguments。在默认 ComSpec=cmd.exe 的 Windows 上，命令中带引号路径的钩子（如 bash "C:\Program Files\tools\cleanup.cmd" arg——路径含空格时引号必不可少）仍会被 Node 的 MSVC CRT 转义破坏，报出 '\"C:\Program Files\tools\cleanup.cmd\"' is not recognized——正是本 PR 为其他所有钩子事件修复的失败——而同一条命令配置为 PreToolUse 时却能正常运行。监督进程代码是既有代码（来自 main，本 diff 未触碰），因此本条记录缺口而非阻断合并；但 PR 描述中的 "this affects every command hook" 与新 JSDoc 的 "Shared by every cmd.exe spawn site" 读起来都覆盖了这条路径，#8649 的失败类别在这三个存活事件上依然存在。
Witness:
在 HEAD 上运行探针（真实 HookRunner.executeHook + mock spawn）：
ARM-A [SessionDelete] outer spawn cmd === process.execPath: true
supervisor source contains "windowsVerbatimArguments": false
supervisor inner args: ["-c","bash \"C:\\Program Files\\tools\\cleanup.cmd\" arg"]
ARM-B [PreToolUse] spawn opts has windowsVerbatimArguments key: true
翻转臂：在监督进程内部 spawn 加上 windowsVerbatimArguments: true 后探针翻转（1 failed | 1 passed）
（win32 CRT 转义这一步本身无法在本 Linux 审查机上执行；该层依据 Node 官方文档的 win32 spawn 行为以及 PR 自带的同形态直连 spawn Windows A/B 证据推定。）
修复建议：在父进程中计算 needsVerbatimArguments(shellConfig.shell)，以数据形式传入监督进程（eval 出的 CommonJS 字符串无法导入 ESM helper）——在监督进程 argv 末尾追加该布尔值，并在其内部 spawn 上设置 windowsVerbatimArguments（见英文部分代码示例）。监督进程的 argv 是位置契约——由 hookRunner.ts:1104-1117 构建、在 SURVIVING_HOOK_SUPERVISOR_SOURCE 内经 process.argv.slice(1) 解构（hookRunner.ts:72-79），且 eval 源码只能 require Node 内置模块（hookRunner.ts:68）——因此新元素必须在两侧同时追加并同步解构；PowerShell 必须保持该标志为 false。修复验收：为存活事件（如 HookEventName.SessionDelete）新增测试，断言监督进程 argv 携带 verbatim 标志（仅 win32+cmd 为 true），移除该传递后测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| * escaping so args round-trip through CommandLineToArgvW. Shared by every | ||
| * cmd.exe spawn site so the policy cannot drift. #8649. |
There was a problem hiding this comment.
[Suggestion] R1-1: This JSDoc still claims the helper is "Shared by every cmd.exe spawn site so the policy cannot drift", but at this commit it has exactly one production caller — the hookRunner spawn this PR adds. A repo-wide sweep at HEAD finds four cmd.exe spawn sites not routed through it: shellExecutionService.childProcessFallback (shellExecutionService.ts:801) keeps its own inline duplicate (windowsVerbatimArguments: isWindows && shell === 'cmd'); the node-pty path (shellExecutionService.ts:1497-1500) keeps a same-shaped inline predicate for its arg-form choice; monitor.ts:370 spawns [...argsPrefix, command] with no windowsVerbatimArguments at all; and the hook supervisor's inner spawn (hookRunner.ts:204) likewise has none. If the verbatim policy ever changes (a new shell type, a Windows escaping edge case), the edit lands in this unit-tested helper on the reasonable belief that every cmd.exe spawn follows it, while the sibling sites silently keep the old behaviour — the exact drift the comment promises is impossible; the comment also masks that monitor commands and surviving hooks with quoted paths still suffer the same #8649 mangling on Windows cmd.exe.
Witness:
sweep at HEAD 21f799c8:
production callers of needsVerbatimArguments: 1 (hookRunner.ts:1156)
shellExecutionService.ts:801 windowsVerbatimArguments: isWindows && shell === 'cmd'
shellExecutionService.ts:1497 os.platform() === 'win32' && shell === 'cmd' (node-pty arg form)
monitor.ts:370 spawn(executable, [...argsPrefix, command], ...) — no verbatim option
hookRunner.ts:204 supervisor inner spawn — no verbatim option (probe-measured)
Preferred fix: migrate shellExecutionService.ts:801 to windowsVerbatimArguments: needsVerbatimArguments(shell) (its import at shellExecutionService.ts:17 already pulls from shell-utils; behavior-identical) — that makes the claim true for both sites that apply the option. If that migration is out of scope here, narrow the JSDoc to match reality:
| * escaping so args round-trip through CommandLineToArgvW. Shared by every | |
| * cmd.exe spawn site so the policy cannot drift. #8649. | |
| * escaping so args round-trip through CommandLineToArgvW. Used by the hook | |
| * spawn sites; other cmd.exe spawn sites are not yet migrated. #8649. |
The migration must keep PowerShell on the default escaping (false) — pinned by shellExecutionService.test.ts:3313 ("should use PowerShell with UTF-8 prefix without windowsVerbatimArguments on Windows") — and the node-pty path's same-shaped predicate serves a different mechanism (arg-form choice) and must not be converted to this spawn option. shellExecutionService.test.ts already pins cmd → true (:3260) and powershell → false (:3313) through the migration; a comment-only fix changes no behaviour, so no new test can pin it.
中文说明
[建议] R1-1：该 JSDoc 仍声称此 helper "被所有 cmd.exe spawn 调用点共享，策略不会漂移"，但在此提交中它只有一个生产调用点——即本 PR 为 hookRunner 添加的 spawn。全仓扫描（HEAD）发现四处 cmd.exe spawn 调用点未经过它：shellExecutionService.childProcessFallback（shellExecutionService.ts:801）仍保留自己的内联副本（windowsVerbatimArguments: isWindows && shell === 'cmd'）；node-pty 路径（shellExecutionService.ts:1497-1500）为其参数形态选择保留了同形的内联谓词；monitor.ts:370 以 [...argsPrefix, command] 启动 shell 却完全没有设置 windowsVerbatimArguments；钩子监督进程的内部 spawn（hookRunner.ts:204）同样没有。将来如果 verbatim 策略需要变更（新增 shell 类型、Windows 转义边界情况），修改会落在这个有单测覆盖的 helper 上——人们会理所当然地以为所有 cmd.exe spawn 都遵循它——而兄弟调用点会悄悄保留旧行为，注释所承诺的"不会漂移"恰恰不可能成立；该注释还会掩盖 monitor 命令与存活钩子在 Windows cmd.exe 下带引号路径时仍会遭遇同类 #8649 破坏的事实。
Witness:
HEAD 21f799c8 全仓扫描：
needsVerbatimArguments 的生产调用点：1 处（hookRunner.ts:1156）
shellExecutionService.ts:801 windowsVerbatimArguments: isWindows && shell === 'cmd'
shellExecutionService.ts:1497 os.platform() === 'win32' && shell === 'cmd'（node-pty 参数形态）
monitor.ts:370 spawn(executable, [...argsPrefix, command], ...) —— 无 verbatim 选项
hookRunner.ts:204 监督进程内部 spawn —— 无 verbatim 选项（探针实测）
首选修复：把 shellExecutionService.ts:801 迁移为 windowsVerbatimArguments: needsVerbatimArguments(shell)（该文件在 shellExecutionService.ts:17 已从 shell-utils 导入；行为完全等价）——这样该声明对两处真正应用该策略的调用点都成立。若迁移不在本 PR 范围内，至少把 JSDoc 收窄为描述现状（见上方 suggestion）。迁移必须让 PowerShell 保持默认转义（false）——由 shellExecutionService.test.ts:3313（"should use PowerShell with UTF-8 prefix without windowsVerbatimArguments on Windows"）固定；node-pty 路径的同形谓词服务于不同机制（参数形态选择），不应改用此 spawn 选项。shellExecutionService.test.ts 已固定 cmd → true（:3260）与 powershell → false（:3313），迁移后两者都应保持通过；仅改注释不改变行为，无新测试可固定。
— qwen3.8-max via Qwen Code /review (v0.22.3)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Verified the helper and both call sites at the PR head. No issues found.
needsVerbatimArguments(shell)is exactlyos.platform() === 'win32' && shell === 'cmd'— the same predicateshellExecutionService.ts:801uses inline (isWindows && shell === 'cmd'), so the hook spawn now follows the policy the comment in that service states ("windowsVerbatimArguments must only be true for cmd.exe: it skips the escaping PowerShell needs"). PowerShell and bash hooks keep the default escaping; non-Windows is false for every shell.- The option is passed on the
shell: falsecmd spawn, which is where Node's MSVC CRT escaping otherwise mangles quoted paths (#8649), and the test asserts it as a function of both platform and shell type, so a regression in either conjunct flips it red. - Centralising the predicate in
shell-utils.ts("Shared by every cmd.exe spawn site so the policy cannot drift") is the right shape for a policy that has to agree across spawn sites.
|
Thanks for the patch and for staying with it through the review rounds. Closing this one in favour of a different direction for Windows command hooks. #11778 moves the hook path off If you have Windows cases that break under #11778's approach, a comment there with the exact hook command would be very useful. |
7 participants
What this PR does
Windows command hooks with a quoted path (e.g.
bash "C:\Program Files\...\script.sh" arg) failed entirely:HookRunner.executeCommandHookspawnscmd.exe /d /s /c <command>but omittedwindowsVerbatimArguments, so Node's MSVC CRT escaping mangled the quotes and backslashes in the command string before cmd.exe parsed it. This affects every command hook — project-level, user-level, and extension-level — whenever the command contains a shell prefix plus a quoted path (unavoidable when the path has a space, e.g. underC:\Program Files\). The fix mirrors thewindowsVerbatimArgumentshandlingshellExecutionServicealready applies to the Bash tool's cmd.exe path, extracted into a sharedneedsVerbatimArguments(shell)helper.Why it's needed
On Windows, quoted script paths are required the moment a hook lives under a directory with a space (Program Files, OneDrive folders, etc.). Without this fix those hooks silently fail with
'\"C:\...\"' is not recognized as an internal or external command— no hook runs, soPreToolUse/PostToolUse/Stopsecurity and workflow hooks are effectively broken for those users.shellExecutionServicealready sets the flag for exactly this reason;HookRunnerwas the one cmd.exe spawn caller missing it.Reviewer Test Plan
How to verify
Reproduce on Windows: add a command hook with a quoted path, e.g.
bash "C:\Program Files\...\script.sh" arg(orcmd /c "C:\path with space\script.cmd" arg), trigger the event, and observe it fails before the fix. With the fix the hook executes and prints its output. Verify PowerShell hooks still work (the flag is gated toshell === 'cmd', so PowerShell keeps default escaping).Evidence (Before & After)
Non-TUI change; unit-level evidence on Windows (path under a directory with a space):
Also verified
bash -c "echo hi from quoted"andpowershell -NoProfile -Command "Write-Host ps-ok"pass through cmd with the flag set.Tested on
Environment (optional)
Local Windows 11 dev checkout; unit tests via vitest.
Risk & Scope
windowsVerbatimArguments: truedisables Node's CRT escaping for cmd.exe only; a malicious command string could rely on that escaping, but hooks are user-authored config (same trust model as the shell command the CLI already executes), and this exactly matchesshellExecutionService's existing cmd.exe path.Linked Issues
Related to #8649
中文说明
Windows 上带引号路径的命令钩子（如
bash "C:\Program Files\...\script.sh" arg）会整体失败：HookRunner.executeCommandHook以cmd.exe /d /s /c <command>启动子进程，但漏设了windowsVerbatimArguments，导致 Node 默认的 MSVC CRT 转义在 cmd.exe 解析前就把引号和反斜杠破坏了。修复方式与shellExecutionService（Bash 工具）已有的 cmd.exe 处理一致：抽成共享的needsVerbatimArguments(shell)helper，仅对 Windows + cmd.exe 开启（PowerShell 保留默认转义）。Windows 上 A/B 实测：修复前'\"C:\...\"' is not recognized（失败），修复后脚本正确执行输出HOOK_OK arg。与 #8646 的协调：该 PR 用另一机制修同一 issue（Windows 默认 hook shell 改 powershell）。本 PR 修 cmd.exe 路径本身，两者互补但都动 hook spawn 路径，故用
Related to而非Fixes，等维护者决定机制取舍。