fix(cli): chmod +x bundled spawn-helper + add live PTY spawn smoke test#4058
Merged
Conversation
User reported `[terminal] open ...: posix_spawnp failed.` opening
terminals in cli-v0.2.5. The dist tarball ships
`lib/node_modules/node-pty/prebuilds/darwin-{arch}/spawn-helper` from
node-pty's npm package, which lands on disk with mode 0644 (npm strips
exec bits). node-pty `posix_spawnp`'s spawn-helper as the fork helper
on darwin; without +x the kernel returns EACCES and node-pty surfaces
it as the cryptic "posix_spawnp failed" with no errno. Normal install
paths run `npm rebuild node-pty` which restores the bit; we ship the
raw prebuild so we have to fix it ourselves.
Reproduced locally against the published 0.2.5 tarball:
- chmod 0644 spawn-helper → `pty.spawn(...)` throws `posix_spawnp failed.`
- chmod 0755 spawn-helper → spawn works
Fixes:
- build-dist.ts: chmodSync(spawn-helper, 0o755) on darwin after
`fixNativeBinariesForNode` removes node-pty's `build/` (which forces
the bindings loader to use the prebuild path).
- Smoke test (build-cli.yml + build-dist-linux-docker.sh): static
`test -x` assertion on darwin, plus an end-to-end `pty.spawn` against
the bundled Node + bundled prebuild that asserts the bundled node-pty
resolves (no host-tree leak via cwd-walking) and that the spawn
actually emits expected output. Run from /tmp because Node's resolver
walks up from cwd before consulting NODE_PATH — staying in the
workspace silently masked this exact bug in earlier versions of the
smoke test (the host repo's electron-rebuilt node-pty has +x and made
the test pass against a broken bundle).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPost-build and CI smoke-test validation are strengthened for the bundled CLI: DIST paths are made absolute, Node module resolution is isolated to the bundled lib, macOS node-pty spawn-helper is ensured executable, and an integration PTY spawn test asserts resolution, output, exit code, and enforces a 5s timeout. ChangesCLI Bundle Validation Hardening
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI / Workflow
participant FS as Filesystem (DIST)
participant Node as Bundled Node
participant node-pty as node-pty module
participant Shell as /bin/sh
CI->>FS: compute absolute DIST, ensure files present
CI->>Node: run script from /tmp with NODE_PATH="$DIST/lib/node_modules"
Node->>FS: require.resolve("node-pty/lib/unixTerminal")
FS-->>Node: path (must be inside DIST)
Node->>node-pty: spawn("/bin/sh -c 'echo SPAWN_OK'")
node-pty->>Shell: fork & exec
Shell-->>node-pty: "SPAWN_OK" + exit 0
node-pty-->>Node: onData + onExit(0)
Node-->>CI: success or fail (on timeout/error/output mismatch)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR fixes a production
Confidence Score: 3/5Safe to merge after fixing the broken relative path in the docker script; core fix in build-dist.ts is correct. One P1 defect in build-dist-linux-docker.sh where cd /tmp leaves the final ls echo using a relative path that breaks under set -euxc. The chmodSync fix in build-dist.ts is clean and the CI workflow changes are correct. packages/cli/scripts/build-dist-linux-docker.sh — line 102 relative path broken by cd /tmp
|
| Filename | Overview |
|---|---|
| packages/cli/scripts/build-dist.ts | Adds chmodSync(spawn-helper, 0o755) for darwin targets after the existing node-pty build/ removal step; chmodSync was already imported, logic is guarded by existsSync, and arch extraction reuses the existing targetParts helper. |
| .github/workflows/build-cli.yml | Adds absolute-path DIST resolution, test -x spawn-helper for darwin, and a live PTY spawn smoke test with a potential onData/onExit race that could cause intermittent CI failures on Linux. |
| packages/cli/scripts/build-dist-linux-docker.sh | Mirrors CI smoke-test additions but the final echo line still uses a relative dist/... path that resolves to /tmp/dist/... after the new cd /tmp step, breaking the script under -euxc. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[build-dist.ts: fixNativeBinariesForNode] --> B{platform === darwin?}
B -- yes --> C[Remove node-pty build/ dir\nforces prebuild path]
C --> D[Resolve spawn-helper path\nprebuilds/darwin-arch/spawn-helper]
D --> E{existsSync?}
E -- yes --> F[chmodSync 0o755\nnew fix]
E -- no --> G[skip]
B -- no --> H[Linux: no spawn-helper needed]
subgraph Smoke Tests
I[cd /tmp\nprevent host node_modules leak]
I --> J[require smoke: better-sqlite3\nnode-pty, watcher, libsql]
J --> K{darwin target?}
K -- yes --> L[test -x spawn-helper]
K -- no --> M[skip]
L --> N[Live PTY spawn\necho SPAWN_OK]
M --> N
N --> O{onExit: got SPAWN_OK and exitCode === 0?}
O -- yes --> P[pty spawn OK]
O -- no --> Q[FAIL]
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/cli/scripts/build-dist-linux-docker.sh:102
**Relative path breaks after `cd /tmp`**
`cd /tmp` is now run before this `echo`, so `dist/superset-${TARGET}.tar.gz` resolves to `/tmp/dist/...` instead of `/work/packages/cli/dist/...`. With `set -euxc` active in the Docker heredoc the failing `$(ls -la ...)` substitution will cause the entire script to exit non-zero, turning every linux-target local build into a failure.
```suggestion
echo "[docker-build] tarball: $(ls -la "$DIST.tar.gz")"
```
### Issue 2 of 2
.github/workflows/build-cli.yml:113-119
**`onExit` / `onData` ordering race on Linux PTY**
node-pty's `onExit` can fire before the final `onData` callback is delivered, because the kernel's EOF on the pty master and the SIGCHLD for the child process are independent events. For a short-lived command like `echo SPAWN_OK` this is unlikely but not impossible under CI load. If the last chunk containing `"SPAWN_OK"` arrives after `onExit`, the check fails and the build fails with `pty spawn FAIL exit=0 got=""`. Deferring the assertion by one event-loop tick (`setTimeout(check, 0)`) would eliminate the flake risk. The same applies to the equivalent block in `build-dist-linux-docker.sh`.
Reviews (1): Last reviewed commit: "fix(cli): chmod +x bundled spawn-helper ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }); | ||
| setTimeout(() => { console.error(\"pty spawn timeout\"); process.exit(1); }, 5000); | ||
| " | ||
| echo "[docker-build] tarball: $(ls -la dist/superset-${TARGET}.tar.gz)" |
Contributor
There was a problem hiding this comment.
cd /tmp
cd /tmp is now run before this echo, so dist/superset-${TARGET}.tar.gz resolves to /tmp/dist/... instead of /work/packages/cli/dist/.... With set -euxc active in the Docker heredoc the failing $(ls -la ...) substitution will cause the entire script to exit non-zero, turning every linux-target local build into a failure.
Suggested change
| echo "[docker-build] tarball: $(ls -la dist/superset-${TARGET}.tar.gz)" | |
| echo "[docker-build] tarball: $(ls -la "$DIST.tar.gz")" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli/scripts/build-dist-linux-docker.sh
Line: 102
Comment:
**Relative path breaks after `cd /tmp`**
`cd /tmp` is now run before this `echo`, so `dist/superset-${TARGET}.tar.gz` resolves to `/tmp/dist/...` instead of `/work/packages/cli/dist/...`. With `set -euxc` active in the Docker heredoc the failing `$(ls -la ...)` substitution will cause the entire script to exit non-zero, turning every linux-target local build into a failure.
```suggestion
echo "[docker-build] tarball: $(ls -la "$DIST.tar.gz")"
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+113
to
+119
| term.onExit(({ exitCode }) => { | ||
| if (got.includes("SPAWN_OK") && exitCode === 0) { | ||
| console.log("pty spawn OK"); | ||
| process.exit(0); | ||
| } | ||
| console.error("pty spawn FAIL exit=" + exitCode + " got=" + JSON.stringify(got)); | ||
| process.exit(1); |
Contributor
There was a problem hiding this comment.
onExit / onData ordering race on Linux PTY
node-pty's onExit can fire before the final onData callback is delivered, because the kernel's EOF on the pty master and the SIGCHLD for the child process are independent events. For a short-lived command like echo SPAWN_OK this is unlikely but not impossible under CI load. If the last chunk containing "SPAWN_OK" arrives after onExit, the check fails and the build fails with pty spawn FAIL exit=0 got="". Deferring the assertion by one event-loop tick (setTimeout(check, 0)) would eliminate the flake risk. The same applies to the equivalent block in build-dist-linux-docker.sh.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/build-cli.yml
Line: 113-119
Comment:
**`onExit` / `onData` ordering race on Linux PTY**
node-pty's `onExit` can fire before the final `onData` callback is delivered, because the kernel's EOF on the pty master and the SIGCHLD for the child process are independent events. For a short-lived command like `echo SPAWN_OK` this is unlikely but not impossible under CI load. If the last chunk containing `"SPAWN_OK"` arrives after `onExit`, the check fails and the build fails with `pty spawn FAIL exit=0 got=""`. Deferring the assertion by one event-loop tick (`setTimeout(check, 0)`) would eliminate the flake risk. The same applies to the equivalent block in `build-dist-linux-docker.sh`.
How can I resolve this? If you propose a fix, please make it concise.![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 `@packages/cli/scripts/build-dist-linux-docker.sh`:
- Around line 71-102: After the script does cd /tmp the relative path
dist/superset-${TARGET}.tar.gz no longer resolves; capture an absolute tarball
path before changing directory (e.g. set
TARBALL="$(pwd)/dist/superset-${TARGET}.tar.gz" or use an existing DIST variable
to build an absolute path) and then replace the final echo that uses ls -la
dist/superset-${TARGET}.tar.gz with ls -la "$TARBALL" (or ls -la "$DIST/...") so
the file lookup works after cd /tmp; update references to the tarball
accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52dfaaac-84b0-4dd7-9f35-d687311057b8
📒 Files selected for processing (3)
.github/workflows/build-cli.ymlpackages/cli/scripts/build-dist-linux-docker.shpackages/cli/scripts/build-dist.ts
Two issues caught by greptile + coderabbit on #4058: 1. build-dist-linux-docker.sh: the final `echo "tarball: ..."` line still used the relative path `dist/superset-${TARGET}.tar.gz`, which after the new `cd /tmp` step resolves to `/tmp/dist/...` and fails under `bash -euxc` — bricking every local linux build. Fix: build the tarball path off `$DIST` (already absolute). 2. node-pty's `onExit` and `onData` are independent events (SIGCHLD on the child + EOF on the pty master). For `echo SPAWN_OK` it almost always lands data-then-exit, but under CI load `onExit` can fire before the final `onData` — flake risk. Defer the assertion one tick after `onExit` so any in-flight `onData` callback runs first.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Collaborator
Author
|
Both issues fixed in ff9fe0a:
Re-verified locally on darwin-arm64: smoke test still passes ( |
saddlepaddle
added a commit
that referenced
this pull request
May 4, 2026
Single bug fixed since v0.2.5: - spawn-helper bundled without exec bit on darwin → terminals crashed with `posix_spawnp failed.` on every open. chmod +x in build-dist + live PTY spawn smoke test that catches this regression class (#4058). Push cli-v0.2.6 after this lands to fire the release pipeline.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 25, 2026
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
User reported on cli-v0.2.5:
The dist tarball ships
lib/node_modules/node-pty/prebuilds/darwin-<arch>/spawn-helperdirectly from node-pty's npm package, which lands with mode0644(npm strips exec bits on tarball pack). On darwin, node-ptyposix_spawnpsspawn-helperas the actual fork helper for every terminal. Without+xthe kernel returns EACCES and the failure surfaces as the crypticposix_spawnp failedwith no errno. The normal install path runsnpm rebuild node-ptywhich restores the bit; we ship the raw prebuild so we have to fix it ourselves.Why earlier smoke tests didn't catch it
require()worked, not that PTY spawn worked.pty.spawnsmoke test in an earlier iteration, I ran it from the workspace cwd. Node's module resolution walks up from cwd before consultingNODE_PATH, sonode-ptyresolved to the host repo'snode_modules/node-pty(electron-rebuilt by the desktop app, with proper+x). The smoke test passed against a broken bundle.What changes
packages/cli/scripts/build-dist.tsAfter the existing darwin
node-pty/build/removal step (which forces the loader onto the prebuild path),chmodSync(spawn-helper, 0o755).Smoke test (
build-cli.yml+build-dist-linux-docker.sh)cd /tmpbefore running the smoke test scripts so Node module resolution doesn't walk up into the host repo.require.resolve("node-pty/lib/unixTerminal")starts with the expected dist path — host-tree leaks now hard-fail the build instead of silently passing.test -xonspawn-helperfor darwin targets.pty.spawn("/bin/sh", ["-c", "echo SPAWN_OK"])against the bundled Node + bundled prebuild. Asserts the spawn actually emits the expected bytes and exits 0.Verification
Reproduced and fixed against the published
cli-v0.2.5tarball locally:The "broken mode" output matches the user's production error verbatim — meaning the new smoke test would have caught this exact bug, and would catch any future regression in the same class (spawn-helper, pty.node ABI mismatch, arch mismatch).
Test plan
cli-v0.2.6, push tag, confirm release artifacts pass smoke test in CIsuperset start→ open a terminal → succeeds (noposix_spawnp failed)Summary by cubic
Fixes macOS terminal spawn failures by making the bundled
node-ptyspawn-helperexecutable. Adds and hardens a live PTY spawn smoke test to catch exec-bit, ABI/arch, and host-tree leak issues.lib/node_modules/node-pty/prebuilds/darwin-*/spawn-helperduring dist packaging.DISTas an absolute path beforecd /tmp; assertnode-ptyresolves from the dist; verifyspawn-helperis+xon darwin; runpty.spawn("/bin/sh","-c","echo SPAWN_OK")and expect output and exit 0; defer the check a tick to avoidonExit/onDatarace; fix Docker script to print"$DIST.tar.gz"instead of a relative path.Written for commit ff9fe0a. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Chores