Skip to content

fix: avoid shell:true deprecation on Windows - #725

Open
fscfede-beep wants to merge 2 commits into
openai:mainfrom
fscfede-beep:fix/windows-process-shell-deprecation-717
Open

fix: avoid shell:true deprecation on Windows#725
fscfede-beep wants to merge 2 commits into
openai:mainfrom
fscfede-beep:fix/windows-process-shell-deprecation-717

Conversation

@fscfede-beep

@fscfede-beep fscfede-beep commented Sep 4, 2026

Copy link
Copy Markdown

Summary

Fixes #717.

Related: #708, #525.

  • stop defaulting shared process launches to shell: true on Windows
  • route node / npm / codex shim-style probes through an explicit ComSpec /d /s /c call ... invocation with shell: false
  • use the same Windows invocation for direct codex app-server startup
  • invoke taskkill directly with shell: false, avoiding Git Bash/MSYS conversion of /PID, /T, and /F
  • treat taskkill exit 128 as an already-stopped process, so cleanup does not depend on English error text
  • add regression coverage for the Windows shim invocation, taskkill options, and localized missing-process output

This avoids Node's DEP0190 warning without breaking .cmd shims such as npm on Windows. It also addresses the shell/argument-conversion root cause reported in #708 and #525 and makes the already-dead taskkill path locale-independent.

Validation

On Windows 11 / Node 26.3.1:

  • reproduced the pre-fix warning with node --trace-deprecation plugins/codex/scripts/codex-companion.mjs setup --json
  • after the change, the same command exits 0 with stderr_bytes=0
  • reproduced Git Bash/MSYS mangling on a nonexistent PID: taskkill /PID 999999 /T /F reaches taskkill as C:/Program Files/Git/PID and exits 1
  • verified direct, no-shell taskkill /PID 999999 /T /F returns localized Spanish ERROR: no se encontró el proceso "999999". with exit 128
  • node --test tests/process.test.mjs tests/commands.test.mjs tests/bump-version.test.mjs -> 14 passed, 0 failed
  • node --check plugins/codex/scripts/lib/process.mjs
  • node --check plugins/codex/scripts/lib/app-server.mjs
  • git diff --check

I also verified on the same host that direct spawnSync(..., { shell: false }) works for .exe binaries but returns EINVAL for an absolute npm.cmd, which is why the patch keeps explicit cmd.exe handling for Windows command shims instead of globally assuming every command is directly executable.

@fscfede-beep
fscfede-beep requested a review from a team September 4, 2026 09:22

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Explicit ComSpec handling is the right Windows boundary: .cmd shims still need command-processor semantics, but the child-process API no longer relies on shell:true. Keeping taskkill direct also avoids MSYS argument rewriting, and exit 128 removes the locale dependency.

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.

DEP0190 on Node 24: every companion command warns on stderr before its JSON

2 participants