Skip to content

fix(cli): probe Windows .cmd/.bat shims through cmd.exe so the tool detector no longer triggers DEP0190 - #17021

Merged
diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.52from
pacocartones:fix/tool-detector-dep0190
Oct 18, 2026
Merged

diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.52from
pacocartones:fix/tool-detector-dep0190

Conversation

@pacocartones

Copy link
Copy Markdown
Contributor

Summary

  • On Windows, omniroute doctor, status and config print Node 24's [DEP0190] DeprecationWarning: Passing args to a child process with shell option true…. This change makes the CLI tool detector probe .cmd/.bat shims without that pattern, so the warning goes away. Detection results are unchanged, except that a shim whose path contains spaces now reports its version: before, the shell: true command line was unquoted and the probe failed, so only installed: true was reported (from cmd's documented quoting rules; not run on Windows).
  • Root cause: detectBinaryWindows in src/lib/cli-helper/tool-detector.ts ran the --version probe as execFile(shimPath, ["--version"], { shell: true, windowsVerbatimArguments: true }), which is shell: true with a non-empty argument array, the exact shape DEP0190 deprecates. fix(cli): spawn npm without DEP0190 on win32 — pass a joined command line instead of an argv array #15328 fixed the npm path in bin/cli/npm-exec.mjs but not this one, as discussed in Node warning after upgrading to v3.8.51 #15315.

Related Issues

Target Branch

  • Base is the active release/v3.8.x branch (the highest open release/v*, here release/v3.8.52), not main

Proof Of Fix (required)

  • TDD — a test that fails before the fix and passes after it.
    • Test file: tests/unit/cli-helper/tool-detector-win32-7279.test.ts, new case "probes a .cmd shim through cmd.exe without shell: true plus an argument array (DEP0190, Node warning after upgrading to v3.8.51 #15315)". It records every call made through the module's __setExecFileImpl seam and fails if any of them combines shell: true with a non-empty argument array.
    • Failing output before the fix (base release/v3.8.52 with only tool-detector.ts reverted):
      ✖ probes a .cmd shim through cmd.exe without shell: true plus an argument array (DEP0190, #15315)
        AssertionError [ERR_ASSERTION]: shell: true with a non-empty argument array is what Node 24 deprecates (DEP0190): {"cmd":"C:\\Users\\dev\\AppData\\Roaming\\npm\\hermes.cmd","args":["--version"],"shell":true}
      ℹ tests 3
      ℹ pass 2
      ℹ fail 1
      
    • With the fix: 3 of 3 pass.
  • Real-environment test: not done. I don't have a Windows machine with Node 24, so this was not run on real Windows. A check by someone on Windows (omniroute doctor with Node 24, no DEP0190 line, same detection result) would settle it.

Scope

  • Only the files the failing test proves need changing are touched: src/lib/cli-helper/tool-detector.ts, the win32 test file, and a one-line changelog fragment.
  • No test was weakened or deleted. The existing mock in that file accepted only shell: true. It now accepts the shim being run through cmd.exe as well, and still throws for a direct spawn of the .cmd, which reproduces the CVE-2024-27980 refusal.

Validation

  • Change type: CLI
  • Focused tests: the five tool-detector test files (tool-detector.test.ts, cli-helper-tool-detector-paths-6162.test.ts, tool-detector-opencode-jsonc-10227.test.ts, cli-tool-detector-imports.test.ts, tool-detector-win32-7279.test.ts): 24 of 24.
  • eslint (with the repository's suppressions) and prettier --check on the two touched files and the changelog fragment, check:ai-attribution --range and check:changelog-integrity: all clean.

Reviewer Notes

  • How it is fixed, and why not the npm-exec.mjs way. When the shim needs cmd.exe, the detector now runs cmd.exe itself with an argument array, ["/d", "/c", <shim path>, "--version"], and no shell option. Node quotes each element, so a path with spaces stays one argument, nothing is interpolated into a shell string (Hard Rule deps: bump qs from 6.14.1 to 6.14.2 #13) and DEP0190 does not apply. npmExecInvocation joins the command line itself, but that is safe only because its tokens are literals; here the path is a runtime value, and cliRuntime.ts already says not to interpolate it.
  • cmd.exe comes from ComSpec (the lookup env, then process.env) with cmd.exe as the fallback.
  • Limit, unchanged by this PR: a path that contains both spaces and one of cmd's special characters (for example Program Files (x86)) still loses its quotes under cmd /c's rules, as it did before this change; this PR does not change that case.
  • Possible follow-up, not in this PR: src/lib/cliTools/codexModelCatalog.ts around line 200 spawns .cmd shims with shell: true plus an argument array too, so it likely emits the same warning.
  • Changelog fragment: changelog.d/fixes/PENDING-tool-detector-dep0190.md, one line, left for you to rename.

@diegosouzapw
diegosouzapw merged commit 1d76655 into cheaper-inference:release/v3.8.52 Oct 18, 2026
44 of 60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants