Skip to content

fix: use 'exec' in binstub#53

Merged
zkochan merged 2 commits into
pnpm:mainfrom
Mortal:binstub-exec
May 11, 2026
Merged

fix: use 'exec' in binstub#53
zkochan merged 2 commits into
pnpm:mainfrom
Mortal:binstub-exec

Conversation

@Mortal
Copy link
Copy Markdown

@Mortal Mortal commented Apr 1, 2026

When cmd-shim is used to create a wrapper for node (or any other binary with no shebang and no recognized extension), you cannot kill the wrapped process by killing the process spawned by invoking the shim, because the sh binstub doesn't use exec.

Commit 1033be3 ("fix: improve sh binstubs", 2020-01-16) added exec to the shLongProg code path, but for some reason didn't add it to the non-shLongProg (else) path — the one taken when opts.prog is null (e.g. a .exe or other extension-less binary). This PR adds exec to that branch so signal delivery and PID inheritance behave consistently across both paths.

Test plan

  • Existing unit-test snapshots updated to reflect the new exec-prefixed output (3 snapshots in test.js.snapshot, 1 in e2e.test.js.snapshot).
  • New POSIX e2e regression test that wraps process.execPath (a no-shebang native binary, exercising the non-shLongProg branch), spawns the shim, and asserts the wrapped binary's process.pid equals the shim's spawn PID — which is only possible when the shell execs into the target. Manually verified: 7/59 tests fail when the exec change is reverted, 59/59 pass with it.
  • CI green on ubuntu / windows × Node 22 / 24.

Rebased onto current main (which includes the recently merged MSYS escape fix from #55).

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes POSIX sh binstub generation so the shim uses exec even when the “long program path” (shLongProg) branch is not taken, allowing signals (e.g., kill) to correctly reach the underlying node process.

Changes:

  • Add exec to the non-shLongProg branch in generateShShim() so the shim process is replaced by the target process.
  • Update JS snapshot expectations for generated shims to include exec.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/index.ts Ensures generated sh shim uses exec in the non-shLongProg path so signals propagate correctly.
test/test.js.snapshot Updates snapshots to match the new exec-prefixed sh shim output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/index.ts
Mathias Rav and others added 2 commits May 12, 2026 01:07
When cmd-shim is used in pnpm to create a wrapper for "node",
you cannot kill the node process by killing the process spawned by
invoking node_modules/.bin/node, because the binstub doesn't use 'exec'.

Commit 1033be3 ("fix: improve sh binstubs", 2020-01-16) added 'exec' to
the "shLongProg" code path, but for some reason didn't add it to the
non-"shLongProg" path.

Add it in the "shLongProg" case to fix the issue of killing node.
Adds a POSIX regression test that wraps `process.execPath` (a no-shebang
binary, the case that hits the non-shLongProg branch of the sh shim),
invokes the shim, and asserts the wrapped binary's `process.pid` equals
the shim's spawn pid. That can only hold when the shell `exec`s into the
target — without exec the shell wraps the binary in its own process,
breaking signal delivery and producing a different PID. Manually
verified: 7 tests fail when the `exec` is reverted, 59/59 pass with it.

Also updates the `.exe` shim e2e snapshot so the existing Windows test
matches the new `exec`-prefixed output.

---
Written by an agent (Claude Code, claude-opus-4-7).
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 11, 2026

📝 Walkthrough

Walkthrough

POSIX shell shims are modified to use exec for target binary invocation, replacing the shim process instead of forking it. The shell template no longer includes a trailing exit $?. E2E and unit tests are updated with a new PID-matching regression test and corresponding snapshot updates.

Changes

POSIX Shell Shim Exec Replacement

Layer / File(s) Summary
Shell Shim Template
src/index.ts
The generateShShim template for the non-shLongProg branch is updated to use exec ... "$@" without a trailing exit $?, allowing the shell to be replaced by the target process.
E2E Test and Helper
test/e2e.test.js
spawn is imported alongside spawnSync, a describeOnPosix helper is added to gate tests to non-Windows platforms, and a new regression test verifies that the shim's child process PID matches the shim's own PID (confirming exec replaces the process).
Test Snapshots
test/e2e.test.js.snapshot, test/test.js.snapshot
POSIX shim snapshots for env.shim, exe.shim (no cmd file), and exe.shim (no shebang) are updated to show exec "$basedir/..." invocations instead of direct calls.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • pnpm/cmd-shim#55: Both PRs modify generateShShim in src/index.ts — this PR updates POSIX shells to use exec, while that PR alters argument escaping for cmd targets.

Poem

🐰 A shim that execs instead of fork,
Lets child processes take the walk,
No double exit, just one clean go—
The PID stays true, as rabbits know! 🐇✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: use 'exec' in binstub' directly and precisely describes the main change: adding 'exec' to shell binstubs to ensure proper signal handling.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@zkochan zkochan merged commit e4f6173 into pnpm:main May 11, 2026
5 of 6 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.

3 participants