Skip to content

fix(cli): switch shelve run to spawn + process-group signal handling#730

Merged
HugoRCD merged 2 commits into
mainfrom
feat/v5-cli-subprocess
Apr 20, 2026
Merged

fix(cli): switch shelve run to spawn + process-group signal handling#730
HugoRCD merged 2 commits into
mainfrom
feat/v5-cli-subprocess

Conversation

@HugoRCD
Copy link
Copy Markdown
Owner

@HugoRCD HugoRCD commented Apr 20, 2026

Summary

Two bugs landed in the previous `shelve run` rewrite:

  • `tinyexec` and `consola` were used in the CLI but never declared in `packages/cli/package.json`, so the binary crashed on a clean install.
  • The signal-forwarding code captured a `childPid` variable that was never reassigned, so SIGINT/SIGTERM were dropped and child processes turned into zombies.

This PR replaces `tinyexec` with `node:child_process.spawn` (no extra dependency) and uses process-group killing so that `pnpm dev` / `npm start`-style commands — which themselves spawn children — get cleanly torn down.

  • Adds `tinyexec` and `consola` to direct dependencies (they were transitive before; depending on transitives is unsafe).
  • Forwards SIGINT, SIGTERM, SIGHUP to the child's process group on Unix; uses `taskkill /T` on Windows.
  • Exits the parent with the child's exit code (or 128 + signal) so CI gets accurate status.

This is the smallest, lowest-risk piece of the v5 CLI work; ship it first to unblock anyone using `shelve run` today.

Test plan

  • `shelve run -- pnpm dev` → Ctrl-C kills the dev server cleanly, no orphaned Vite/Nuxt process.
  • `shelve run -- node -e "setInterval(()=>{},1000)"` → SIGTERM from another shell terminates the child.
  • CI signals on a long-running test command propagate.

- Replace tinyexec + tree-kill with node:child_process.spawn and proper
  process-group kill via process.kill(-pid). Fixes the long-standing
  childPid bug where signals were never propagated and child trees were
  left as zombies.
- Drop the implicit npx fallback (~200ms cold start, broken signals) and
  invoke node_modules/.bin/nr directly when shorthand commands are passed.
- Declare tinyexec and consola as direct dependencies of @shelve/cli.
  They were transitive-only before, so installs under npm/yarn/bun broke
  whenever pnpm hoisting wasn't there to save the day.
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 20, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
shelve-app Ready Ready Preview, Comment, Open in v0 Apr 20, 2026 11:44am
shelve-lp Ready Ready Preview, Comment Apr 20, 2026 11:44am
shelve-vault Ready Ready Preview, Comment Apr 20, 2026 11:44am

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 20, 2026

Thank you for following the naming conventions! 🙏

ESLint require-await flagged spawnChild as async without an await
inside; the function never needed to be async — it returns the
ChildProcess immediately and lifecycle handlers wire process.exit.
Dropped the await at the call site too (it was awaiting a non-promise).
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Apr 20, 2026

Open in StackBlitz

npm i https://pkg.pr.new/@shelve/cli@730

commit: 8bdeeea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant