feat(cli): add local playground + suspicious-exit warning for shelve run#749
Merged
Conversation
Previously the only way to iterate on `shelve run` locally was to bump the CLI version, publish to npm, install globally, then test in some other repo. When something silently went wrong (e.g. the child exited after a host-project pnpm reinstall prompt) there was no fast way to reproduce and inspect. Adds: - `playground/run/`: standalone fixture (not part of the workspace, no deps) with `dev` / `start` / `fail` scripts that just call `scripts/print-env.mjs`. Keep-alive mode for testing signals and `--watch`. Edit `shelve.json` to point at a real project you own, log in once, then iterate. - Root scripts `pnpm play`, `pnpm play:start`, `pnpm play:fail`, `pnpm play:watch`. Each stubs the CLI first (`unbuild --stub`) so source edits in `packages/cli/src/**` are picked up without a rebuild step. - `shelve run` now warns when the child exits cleanly in <250ms, suggesting `--debug` and `shelve run -- <pm> <script>` to bypass script resolution. Script-resolution errors are also debug-logged instead of being silently swallowed.
|
Deployment failed with the following error: |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Thank you for following the naming conventions! 🙏 |
Contributor
|
Warning Review limit reached
More reviews will be available in 13 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds a local CLI playground at ChangesLocal CLI Playground and Observability
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@playground/run/README.md`:
- Around line 25-28: Add a blank line immediately before the fenced code block
that contains "```sh" so the snippet starting with "pnpm cli login" is preceded
by an empty line; update the README.md section where the fenced block appears to
insert that blank line before the triple-backtick fence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8a53b1ab-56a2-46bf-84c2-f84e401c1a83
📒 Files selected for processing (8)
.changeset/cli-playground.md.gitignorepackage.jsonpackages/cli/src/commands/run.tsplayground/run/README.mdplayground/run/package.jsonplayground/run/scripts/print-env.mjsplayground/run/shelve.json
Previous playground required a real Shelve project + login. Replace
that with a zero-dep `node:http` fake server seeded from `seed.json`
(team / project / 3 environments / variables / fake token).
- `server.mjs` — implements the 4 endpoints `shelve run` hits, plus a
`/health` probe and two `/__playground/{variables,reset}` admin
helpers for exercising `--watch` from another terminal.
- `start.mjs` — orchestrator: boots the server, waits for `/health`,
spawns the locally-stubbed CLI with `SHELVE_URL` / `SHELVE_TOKEN` /
`SHELVE_TEAM_SLUG` / `SHELVE_PROJECT` / `SHELVE_DEFAULT_ENV` set
from the seed, kills the server on exit. Strips a forwarded `--`
so `pnpm play -- --debug` works.
- `seed.json` — single source of truth for the fixture data.
- `shelve.json` — points at `127.0.0.1:7777` so `shelve run` also
works if you `cd playground/run && node ../../packages/cli/dist/index.mjs run dev`
with the server already up via `pnpm play:server`.
- `pnpm play:server` — server-only, for iterating on the fake API.
Smoke-tested all variants. CLI fetches project → env → variables from
the fake server, child gets the 4 seeded vars + the 5 `SHELVE_*`
overrides, exit codes propagate (`play:fail` → 7), Ctrl-C tears down
the server.
commit: |
Two long-standing `shelve run` bugs the playground surfaced:
1. `runMain(main).then(() => process.exit(0))` in index.ts resolves
as soon as the `run` command function returns, which happens right
after `await spawnChild(...)` — BEFORE the child has done anything.
The CLI then force-exits, orphaning the child. With stdio:'inherit'
the orphan keeps writing to the terminal so it *looks* fine, but
signal forwarding, watch reload and exit-code propagation are all
broken. Fix: the run command awaits a never-resolving promise, so
the only path to process.exit is via attachLifecycle's child.on('exit').
2. With --watch --restart-on-change, killing the old child fires
its exit handler which calls process.exit(143) before the new
child can spawn. Fix: mark the outgoing child with __restarting=true
so attachLifecycle's exit handler becomes a no-op while watch is
swapping in a replacement.
Playground harden:
- `pnpm play:watch` now adds `--restart-on-change` so var changes
are actually visible (plain --watch only sends SIGHUP and the
child keeps its frozen process.env).
- print-env banner gets a timestamp + ── borders so reloads are
obvious; SIGHUP re-prints the env in-place; var name filter is
overridable via PLAYGROUND_SHOW_ENV_PATTERN.
- server.mjs uses closeAllConnections + a guard so it always
releases the port (was leaving 7777 in TIME_WAIT under stress).
- start.mjs spawns the server and the CLI detached so killAll can
signal whole process groups; teardown gives them 200-300ms to
shut down cleanly before exiting.
- README documents the watch reload flow with a copy-pasteable
curl, and the harmless `node_modules missing` pnpm warning.
Smoke-tested every variant: play, play:start, play:fail, play:watch
with live curl-driven reload, Ctrl-C cleanup, port release.
Two cosmetic-but-loud issues the playground surfaced once `shelve run`
actually stayed alive long enough to print:
1. Every spawn went through `pnpm run <script>` (via nypm), so the
user got `$ node scripts/whatever.mjs` as the script prefix, plus
`[WARN] Local package.json exists, but node_modules missing` once
per spawn, plus `[ELIFECYCLE] Command failed with exit code 143`
every time we SIGTERM'd the child for a watch reload. The 143 in
particular made it look like the reload failed.
Fix: when `resolveCommandWithScripts` resolves a script through
`pnpm`/`npm`, prepend `--silent` so the package-manager wrapper
stops printing its own banner and exit-code complaints. The script
itself still gets full stdio.
2. `EnvService.getEnvVariables` was wrapped in `withLoading('Fetch
variables', …)`, so every watch poll (every 5s by default) flashed
a spinner row. Now accepts `{ quiet: true }`; the watch loop uses
it, so only the initial fetch shows the spinner.
Combined effect: a watch session is now essentially silent between
events, and a reload shows just `● Variables changed — reloading
child process.` followed immediately by the new banner.
Two robustness fixes for the case where a terminal is closed (or killed) while shelve run is still alive: - packages/cli: listen for `EIO` on `process.stdin` and exit 129 (SIGHUP-equivalent). Without this, an orphaned CLI whose parent terminal died keeps polling the API on its watch interval, and every interactive render attempt throws `setRawMode EIO` to the user's next terminal. Now it just exits. - playground/run/start.mjs: also handle SIGHUP so closing the host terminal cleanly tears down the fake server + the CLI subtree instead of leaving them as orphans.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/run.ts (1)
320-325:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the process signal handlers when a child is replaced.
attachLifecycle()adds threeprocess.on(...)listeners on every spawn, but watch restarts keep reattaching them. After a few reloads you'll accumulate stale handlers, hitMaxListenersExceededWarning, and fan signals out to dead child PIDs/process groups.🧹 Proposed fix
const signals: NodeJS.Signals[] = ['SIGINT', 'SIGTERM', 'SIGHUP'] - for (const sig of signals) process.on(sig, () => onSignal(sig)) + const signalHandlers = signals.map((sig) => { + const handler = () => onSignal(sig) + process.on(sig, handler) + return [sig, handler] as const + }) + const cleanup = () => { + for (const [sig, handler] of signalHandlers) process.off(sig, handler) + } child.on('exit', (code, signal) => { if (killTimer) clearTimeout(killTimer) + cleanup() if ((child as ChildWithFlag).__restarting) return @@ child.on('error', (err) => { if (killTimer) clearTimeout(killTimer) + cleanup() consola.error(`Failed to spawn process: ${err.message}`) process.exit(1) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/run.ts` around lines 320 - 325, attachLifecycle currently adds process.on handlers for signals (the signals array and onSignal) on every spawn and never removes them, causing listener leaks on restarts; update attachLifecycle so it returns a cleanup function (or store the bound handler references) and ensure you call process.off/removeListener for each signal when replacing or when the child is marked restarting (see ChildWithFlag.__restarting and the child.on('exit') handler), i.e., register handlers with named/bound functions, keep those references, and call signals.forEach(sig => process.off(sig, boundHandler)) when the child is cleaned up or about to be replaced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/run.ts`:
- Around line 161-164: The outgoing child process isn't marked as restarting
early enough: set the restart flag on the old child (cast to ChildWithFlag and
set old.__restarting = true) before you send SIGTERM or otherwise kill it inside
spawnNew (and the analogous restart path around spawnChild elsewhere); this
ensures attachLifecycle sees a restarting exit instead of treating it as a
normal exit and calling process.exit(143). Locate the spawnNew function and the
other restart block that creates a replacement child (references: spawnNew,
spawnChild, child, ChildWithFlag, __restarting, attachLifecycle, argv, env) and
move/ensure the assignment old.__restarting = true happens before any
kill/termination is issued.
In `@playground/run/README.md`:
- Around line 9-18: The README's directory-tree code fence is unlabeled and
flagged by markdownlint; edit the README.md directory-tree block that begins
with triple backticks and change the opening fence from ``` to ```text (or
```plain) so the block is explicitly marked as plain text; no other content
changes needed.
In `@playground/run/scripts/print-env.mjs`:
- Around line 4-6: The code currently constructs SHOW with new
RegExp(process.env.PLAYGROUND_SHOW_ENV_PATTERN) which will throw on invalid
patterns; wrap that RegExp construction in a try/catch: when
PLAYGROUND_SHOW_ENV_PATTERN is present attempt to create the RegExp inside try,
on error print a concise error to stderr (e.g. "Invalid
PLAYGROUND_SHOW_ENV_PATTERN: <error message>") and call process.exit(1); leave
the fallback default regex intact for the no-env case and reference the SHOW
constant name in your change so it’s clear where the guard is applied.
In `@playground/run/server.mjs`:
- Around line 114-117: The handler currently assigns mutableVariables[env] =
body.variables before validating body.variables, which can corrupt state if
body.variables is invalid and later accessing body.variables.length throws; fix
by validating payload first (check that body and body.variables exist and that
body.variables is the expected array/type and safe to read length) before
mutating mutableVariables[env], and only after validation perform the assignment
and call send(res, 200, { ok: true, env, count: body.variables.length }); use
existing notFound(res, `Unknown env: ${env}`) early for env checks and ensure
all validations occur prior to writing mutableVariables[env].
- Around line 34-45: The requireBearer function currently accepts any non-empty
token; update it to validate the token value against the configured expected
token (e.g., process.env.PLAYGROUND_BEARER_TOKEN or an app-config value) and
call unauthorized(res, 'Invalid Bearer token') when it does not match; keep the
existing format checks (startsWith 'Bearer ' and non-empty) and only return the
token when it equals the expected value, referencing the requireBearer function
and the unauthorized helper to locate the change.
---
Outside diff comments:
In `@packages/cli/src/commands/run.ts`:
- Around line 320-325: attachLifecycle currently adds process.on handlers for
signals (the signals array and onSignal) on every spawn and never removes them,
causing listener leaks on restarts; update attachLifecycle so it returns a
cleanup function (or store the bound handler references) and ensure you call
process.off/removeListener for each signal when replacing or when the child is
marked restarting (see ChildWithFlag.__restarting and the child.on('exit')
handler), i.e., register handlers with named/bound functions, keep those
references, and call signals.forEach(sig => process.off(sig, boundHandler)) when
the child is cleaned up or about to be replaced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4df3b41d-40d8-4758-a923-023e343a5887
📒 Files selected for processing (11)
.changeset/cli-playground.mdpackage.jsonpackages/cli/src/commands/run.tspackages/cli/src/index.tspackages/cli/src/services/env.tsplayground/run/README.mdplayground/run/scripts/print-env.mjsplayground/run/seed.jsonplayground/run/server.mjsplayground/run/shelve.jsonplayground/run/start.mjs
Audit + rewrite of every process-handling path in `shelve run` and
the playground orchestrator. Goal: zero orphans, on any OS, with any
package manager, under any termination signal — including SIGKILL on
the parent.
shelve run (packages/cli/src/commands/run.ts)
- Use `tree-kill` (already a dep) for all process termination.
Replaces `process.kill(-pid, sig)` which only works on Unix; on
Windows it now uses `taskkill /F /T` under the hood.
- Drop `detached: !isWindows` on the spawned child. With tree-kill
we don't need our own process group, and not being detached means
the child correctly inherits SIGHUP when the controlling terminal
closes.
- Add `shell: true` on Windows so `pnpm.cmd` / `npm.cmd` / `yarn.cmd`
/ `bun.cmd` shims actually resolve. tree-kill walks through the
cmd.exe + shim chain.
- Add `windowsHide: true` to avoid opening flash console windows.
- Add a parent-death watchdog: every 2s, ping `process.ppid` with
signal 0. If the parent is gone (terminal force-quit, Cursor
crashed, …), kill the child tree and exit 129. This is the main
defense against the `setRawMode EIO` orphan-spam case from earlier.
- Register the SIGINT/SIGTERM/SIGHUP handlers ONCE (module-level
ref to current child) instead of once per spawn. Fixes the
listener-leak warning after 10 watch reloads.
Playground orchestrator (playground/run/start.mjs)
- Inline a tiny cross-platform tree-kill (taskkill on Windows,
process-group + direct kill on Unix). The playground has no deps,
so we can't import tree-kill.
- Drop `detached: !isWindows` on both server and CLI spawn (same
reasoning).
- Add `windowsHide: true`.
- Add SIGHUP handler (terminal close) and parent-death watchdog.
- Add `serverProc.on('exit')` so an unexpected server crash doesn't
leave the CLI thinking everything's fine.
- Drop `setTimeout().unref()` on final-exit timer — was letting the
orchestrator exit code-0 before the timer fired, swallowing the
CLI's real exit code. Now `pnpm play:fail` correctly propagates 7.
Playground server (playground/run/server.mjs)
- Add parent-death watchdog (same poll-ppid pattern).
Smoke-tested on macOS:
- pnpm play + Ctrl-C → server + child clean shutdown, no orphans, port released.
- pnpm play + kill -9 on the orchestrator → 5s later: parent-death
watchdogs fire, server logs SIGTERM, CLI logs SIGTERM, 0 orphans.
- pnpm play:watch + 2 live variable changes via curl → both reloads
visible, no listener-leak warning, clean teardown.
- pnpm play:fail → exit 7 propagated cleanly, no [ELIFECYCLE] noise.
- run: set __restarting on the outgoing child before SIGTERM in watch restart (not inside spawnNew after killTree), closing the race where attachLifecycle could process.exit before the replacement spawns. - server: reject Bearer tokens that do not match seed.json. - server: validate body.variables is an array before mutating state. - print-env: catch invalid PLAYGROUND_SHOW_ENV_PATTERN with a clear exit. - README: label directory tree fence as text; document real token auth.
When stopping `shelve run dev` with Ctrl-C, pnpm/npm printed `[ELIFECYCLE] Command failed with exit code 130` even though the interruption was intentional. - Resolve package.json script names by running the script body directly (`sh -c` / `cmd /c`) instead of `pnpm run <script>`, so the package-manager wrapper never sees a non-zero exit on SIGINT. - Treat signal exit codes (130/143/129) and user-initiated shutdown as success (exit 0) in the CLI lifecycle handler. - Map the same codes to exit 0 in the playground orchestrator so `pnpm play` also finishes cleanly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
I had no fast loop on
shelve runand one silent failure took an hour to reproduce. This PR sets up a self-contained local fixture so the full request flow (auth → fetch project → fetch env → fetch variables → spawn child with injected secrets) is exercised in <2s with no network, no Shelve app running, no real account.What's in
playground/run/Fake API
A ~110-line
node:httpserver with no deps. Implements every endpointshelve runcalls:/health{ ok: true, ts }/api/user/me/api/teams/:slug/projects/name/:name/api/teams/:slug/environments/api/teams/:slug/environments/:envName/api/teams/:slug/projects/:projectId/variables/env/:envId/api/teams/:slug/projectsAll authed:
Authorization: Bearer …required (any non-empty token, the orchestrator sends the seed token).Plus two admin helpers (no auth) for exercising
--watch:/__playground/variables{ env, variables }— swap variables in-memory./__playground/resetOrchestrator (
start.mjs)server.mjsand probes/healthfor up to 5s.SHELVE_URL/SHELVE_TOKEN/SHELVE_TEAM_SLUG/SHELVE_PROJECT/SHELVE_DEFAULT_ENVinjected from the seed.Also strips a forwarded
--sopnpm play -- --debugreaches the CLI as--debug(otherwise pnpm splices the--in literally, which the CLI'sparseRawArgsthen treats as the command separator).Root scripts
unbuild --stubwrites a tinydist/index.mjsthat re-exports fromsrc/, so edits inpackages/cli/src/**are picked up without a rebuild.Sanity guard in
shelve runWhile I was here I added a small UX guard for the kind of silent failure that started all this:
--debugandshelve run -- <pm> <script>to bypass script resolution.nypm.runScriptare now debug-logged instead of being silently swallowed.How to use
Smoke tests run locally
pnpm play:start— fetches project / env / variables from fake API, child gets 4 seed vars + 5SHELVE_*overrides, exits 0.pnpm play:fail— child exits 7, propagated as exit 7.pnpm play -- --debug—--no longer eats the flag.pnpm typecheck+pnpm lintclean.Out of scope
The "reinstall
node_modules?" prompt I hit earlier today ispnpmin my ownhr-foliohost project (lockfile drift).nypm.runScript({ dry: true })is pure — it doesn't execute or prompt — so it's not a CLI bug. The new playground sidesteps it by being depless, and the suspicious-exit warning catches similar cases in real projects.Changeset
Included (
@shelve/cli: patch).Summary by CodeRabbit
Release Notes
New Features
pnpm playscript family.Bug Fixes
shelve runexiting prematurely after spawning child process.--watch --restart-on-changeterminating during first reload.