perf(npm): copy-over-shim for near-native CLI startup#69
Merged
Conversation
Replace the spawn-shim launcher with the esbuild/ast-grep copy-over model so `.bin/csp` resolves directly to the native Rust binary — no Node.js process on the hot path. Measured `csp --version` via the installed bin drops from ~59 ms (Node boot + spawnSync) to ~5 ms (~11x faster); the win applies to npm/pnpm installs and bun installs that trust the package. - add `install.js` postinstall: copies (hard-links, falls back to byte-copy) the resolved platform binary over `bin/csp.js` via temp + atomic rename. Best-effort (always exits 0) and idempotent via an inode short-circuit that avoids the POSIX same-inode rename() no-op leaving a stray temp file. - extract shared platform resolution into `lib/resolve.js` (never overwritten), required by both the launcher and the postinstall so `npm rebuild` never require()s a native binary as CommonJS. - harden `bin/csp.js` as the fallback launcher (used when postinstall is skipped, e.g. bun-untrusted / `bunx` / --ignore-scripts): forward argv, stdio, exit code, and SIGINT/SIGTERM/SIGHUP so killing the launcher cleanly stops a long-running `csp mcp` child instead of orphaning it; print a TTY-only fast-path hint; resolve a dev `target/` binary from a source checkout. The dev resolver is NOT used by the postinstall, so it can never clobber the source shim. - generator ships install.js + lib/resolve.js in the wrapper; package.json files/postinstall updated. - document the model, startup numbers, and the bun `trustedDependencies` caveat in npm/README.md and CLAUDE.md.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a shared platform resolver, a postinstall copy-over that can replace the JS launcher with the native binary, a fallback launcher path with signal forwarding, and updates package wiring plus documentation. ChangesPostinstall copy-over launcher
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant npm
participant installjs as install.js
participant resolvejs as resolve.js
participant shim as bin/csp.js
npm->>installjs: run postinstall
installjs->>resolvejs: resolveBinaryPath()
resolvejs-->>installjs: native binary path or null
installjs->>shim: compare inode with binary
alt shim not yet replaced
installjs->>shim: hard-link or copy binary to temp file
installjs->>shim: rename temp file over bin/csp.js
else already copied
installjs-->>npm: no-op
end
installjs-->>npm: exit 0
sequenceDiagram
participant User
participant Launcher as bin/csp.js
participant Resolver as resolve.js
participant NativeBinary
User->>Launcher: invoke csp
Launcher->>Resolver: resolveBinaryPath() / resolveDevBinaryPath()
Resolver-->>Launcher: binary path or null
alt binary found
Launcher->>User: emit TTY fallback warning (unless suppressed)
Launcher->>NativeBinary: spawn(argv, inherited stdio)
User--)Launcher: SIGINT/SIGTERM/SIGHUP
Launcher--)NativeBinary: forward signal
NativeBinary-->>Launcher: exit code / signal
Launcher-->>User: propagate exit code or re-raise signal
else binary not found
Launcher-->>User: error - platform package not installed
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
Up to standards ✅🟢 Issues
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an esbuild-style copy-over-shim optimization to the @pleaseai/csp npm wrapper. During postinstall, the native platform binary is copied or hard-linked directly over the JS launcher shim (bin/csp.js), eliminating Node.js startup overhead on subsequent executions. A fallback runtime launcher is maintained for environments where postinstall scripts are blocked (e.g., under Bun by default). Feedback on the changes highlights a performance risk in the libc detection logic (isMusl), where calling process.report.getReport() may cause synchronous network lookups that block the main thread, and suggests disabling network lookups during this call.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
Greptile SummaryThis PR replaces the Node.js spawn shim in
Confidence Score: 5/5Safe to merge — the copy-over is fully best-effort and idempotent, the fallback launcher preserves all existing behavior, and the source-checkout guard prevents accidental mutation of tracked files. All failure modes in install.js leave the original JS launcher in place and exit 0. The atomic rename, inode idempotency check, and node_modules path guard are correctly implemented. The only notable trade-off is that the fallback launcher's signal forwarding delivers SIGINT twice to the child in an interactive terminal session, but this has no impact on the primary MCP use case and no impact on the copy-over fast path at all. No files require special attention; all changes are additive and the existing fallback path is preserved unchanged. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[npm/pnpm/bun install] --> B{postinstall blocked?}
B -->|No| C[install.js runs]
B -->|Yes - bun untrusted or ignore-scripts| F
C --> D{Source checkout? no node_modules in path}
D -->|Yes| F[bin/csp.js stays as JS launcher]
D -->|No| E{Windows?}
E -->|Yes| F
E -->|No| G[resolveBinaryPath]
G --> H{Platform pkg installed?}
H -->|No| F
H -->|Yes| I{Same inode? idempotency}
I -->|Yes| J[Already replaced - skip]
I -->|No| K[linkSync or copyFileSync to temp]
K --> L[renameSync temp over bin/csp.js]
L --> M[.bin/csp resolves to native binary]
F --> N[User runs .bin/csp]
M --> O[User runs .bin/csp]
O --> P[Native Rust - 5-12ms startup]
N --> Q[bin/csp.js JS launcher - 60ms startup]
Q --> R[resolveBinaryPath or resolveDevBinaryPath]
R --> S[spawn native binary with signal forwarding]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[npm/pnpm/bun install] --> B{postinstall blocked?}
B -->|No| C[install.js runs]
B -->|Yes - bun untrusted or ignore-scripts| F
C --> D{Source checkout? no node_modules in path}
D -->|Yes| F[bin/csp.js stays as JS launcher]
D -->|No| E{Windows?}
E -->|Yes| F
E -->|No| G[resolveBinaryPath]
G --> H{Platform pkg installed?}
H -->|No| F
H -->|Yes| I{Same inode? idempotency}
I -->|Yes| J[Already replaced - skip]
I -->|No| K[linkSync or copyFileSync to temp]
K --> L[renameSync temp over bin/csp.js]
L --> M[.bin/csp resolves to native binary]
F --> N[User runs .bin/csp]
M --> O[User runs .bin/csp]
O --> P[Native Rust - 5-12ms startup]
N --> Q[bin/csp.js JS launcher - 60ms startup]
Q --> R[resolveBinaryPath or resolveDevBinaryPath]
R --> S[spawn native binary with signal forwarding]
Reviews (3): Last reviewed commit: "fix(npm): musl-arm64 resolution + cross-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@npm/csp/install.js`:
- Around line 35-77: The install flow in `install.js` currently rewrites the
tracked launcher at `shimPath` in place, which can mutate the repo checkout and
poison later packaging. Update the `postinstall` path to only replace
`bin/csp.js` when running from an installed package, or switch the
`linkSync`/`copyFileSync` + `renameSync` logic to write from an immutable
template instead of the tracked file. Keep the existing idempotency and cleanup
behavior around `tempPath`, but ensure
`npm/scripts/generate-platform-packages.mjs` never reads a locally rewritten
launcher.
In `@npm/README.md`:
- Around line 81-84: The README entry for csp/install.js currently implies
bin/csp.js is always overwritten, but install.js only performs the copy in
supported, successful postinstall cases. Update the wording in the csp package
section to make the overwrite note conditional, and reference the install.js
postinstall copy-over behavior plus the bin/csp.js launcher so it clearly states
the file is left untouched on Windows, unsupported platforms, skipped lifecycle
scripts, or filesystem failures.
🪄 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 Plus
Run ID: 9fd071f5-bd23-48bb-a5b5-01d7ebdc8b47
📒 Files selected for processing (7)
CLAUDE.mdnpm/README.mdnpm/csp/bin/csp.jsnpm/csp/install.jsnpm/csp/lib/resolve.jsnpm/csp/package.jsonnpm/scripts/generate-platform-packages.mjs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 7 files
Architecture diagram
sequenceDiagram
participant PM as Package Manager (npm/pnpm/bun)
participant Install as install.js (postinstall)
participant Resolver as resolve.js (shared resolver)
participant FS as Filesystem
participant Bin as .bin/csp (symlink)
participant Native as Native binary (csp)
participant User
Note over PM,User: Installation flow (postinstall)
PM->>Install: npm/pnpm install / bun install (if trusted)
Install->>Resolver: resolveBinaryPath()
Resolver->>Resolver: Map platform/arch/libc
alt Unsupported platform (e.g. win32) or optional dep missing
Resolver-->>Install: return null
Install-->>PM: exit 0 (no op)
else Binary found
Resolver-->>Install: path to platform binary
Install->>FS: stat(bin/csp.js) && stat(binary)
alt Same inode (already hardlinked)
Install->>Install: skip (idempotent)
else Different inode
Install->>FS: linkSync(binary, temp) or copyFileSync + chmod
Install->>FS: renameSync(temp, bin/csp.js)
Note over Install: atomic overwrite, temp cleaned up
FS-->>Install: success
Install-->>PM: exit 0
end
end
Note over PM,Bin: .bin/csp now points to native binary<br/>No Node.js involved on hot path
User->>Bin: csp --version
alt Postinstall succeeded (symlink → native binary)
Bin->>Native: Execute directly (no Node)
Native-->>User: version output (fast ~5ms)
else Postinstall did not run (e.g. bun default, win32)
Note over Bin: .bin/csp → node bin/csp.js (fallback launcher)
Bin->>Bin: require('./lib/resolve.js')
Bin->>Resolver: resolveBinaryPath()
alt Binary found
Resolver-->>Bin: path to platform binary
else Binary not found (optional dep missing)
Bin->>Resolver: resolveDevBinaryPath()
alt Dev checkout found
Resolver-->>Bin: target/release/csp
else Not found
Bin->>Bin: exit with error message
end
end
Bin->>Native: spawnSync(csp, argv)
Native-->>Bin: stdout/stderr/exit code
Bin->>Bin: Forward signals (SIGINT/SIGTERM/SIGHUP)
opt TTY and not CSP_NO_FALLBACK_WARNING
Bin->>User: print hint (fast path available)
end
Bin-->>User: exit code matching child
Note over Bin,Native: ~60ms including Node boot
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review feedback from CodeRabbit / Greptile / Gemini on the copy-over-shim: - install.js: only run the copy-over when installed under node_modules. From a source checkout `bin/csp.js` is tracked and the generator reads it into every wrapper, so an in-place rewrite could ship a native binary as the launcher. Verified the guard blocks the clobber even when the platform binary resolves. - lib/resolve.js: set `process.report.excludeNetwork = true` around getReport() (restored after) so libc detection can't trigger a blocking reverse-DNS lookup; stop exporting the internal `isMusl` helper. - bin/csp.js: broaden the fallback hint to mention `--ignore-scripts`, not just bun trustedDependencies. - README: make the bin/csp.js overwrite note conditional (Windows / unsupported / skipped scripts / copy failure leave it as the fallback launcher).
Contributor
Author
|
Addressed the review feedback in c43be18:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
npm/csp/lib/resolve.js (1)
53-79: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGate this workaround on Node 22.12.0+
process.report.excludeNetworkdidn’t fully suppress the blocking libuv network queries until Node 22.12.0, so on Node 22.0.0–22.11.x this can still hang duringgetReport().🤖 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 `@npm/csp/lib/resolve.js` around lines 53 - 79, Gate the workaround in isMusl() on Node 22.12.0+ because process.report.excludeNetwork is not reliable on 22.0.0–22.11.x and can still hang during process.report.getReport(). Update the logic in resolve.js so the excludeNetwork toggle and report-based musl detection only run on supported Node versions, and fall back to returning false for older 22.x releases.
🤖 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.
Outside diff comments:
In `@npm/csp/lib/resolve.js`:
- Around line 53-79: Gate the workaround in isMusl() on Node 22.12.0+ because
process.report.excludeNetwork is not reliable on 22.0.0–22.11.x and can still
hang during process.report.getReport(). Update the logic in resolve.js so the
excludeNetwork toggle and report-based musl detection only run on supported Node
versions, and fall back to returning false for older 22.x releases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 797b3ae6-8029-4d54-aacc-57c4be92b8f7
📒 Files selected for processing (4)
npm/README.mdnpm/csp/bin/csp.jsnpm/csp/install.jsnpm/csp/lib/resolve.js
✅ Files skipped from review due to trivial changes (1)
- npm/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- npm/csp/bin/csp.js
- npm/csp/install.js
Second round of PR review (cubic): - resolve.js: on musl arm64 (Alpine/aarch64) return null instead of the glibc arm64 package, so the launcher reports an unsupported platform rather than exec'ing an incompatible glibc binary. Add a matching `libc: 'glibc'` constraint to the linux-arm64 target so npm/bun skip it on musl arm64. - install.js: compare device + inode (not inode alone) in the idempotency short-circuit — inode numbers are only unique within a filesystem.
Contributor
Author
|
Second review round addressed in 71ed3af:
Note: several bot comments re-anchored to
|
|
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.
What & why
Replaces the npm wrapper's spawn shim with the esbuild/ast-grep-style copy-over-shim, so the installed
.bin/cspresolves directly to the native Rust binary — no Node.js process on the hot path. This preserves the standalone-binary / no-runtime distribution model (does not convert to napi); the Node launcher stays only as a fallback.Startup measurement
csp --versionthrough a real installed.bin/csp(macOS, quiet machine, hyperfine):spawnSync≈11× faster, eliminating ~54 ms of Node boot + spawn per invocation.
Where the win applies (and where it doesn't)
The copy-over runs as a
postinstall. Verified end-to-end withnpm pack+ install:bun installwith@pleaseai/cspintrustedDependencies→ copy-over active, fast path.bun installblocks lifecycle scripts for untrusted deps (Blocked 1 postinstall) → stays on the JS launcher. Still fully functional, no regression, just no speedup.bunx @pleaseai/csp(README's headline entrypoint, incl. MCP config) is ephemeral + untrusted → always the launcher.So for the default
bunxpath this is correct and non-regressing but only speeds up if users opt in viatrustedDependencies(documented) or use an installed project bin.Changes
npm/csp/install.js(new postinstall): copies (hard-links, falls back to byte-copy) the platform binary overbin/csp.jsvia temp + atomic rename. Best-effort (always exits 0) and idempotent via an inode short-circuit (avoids the POSIX same-inoderename()no-op that would leave a stray temp file onnpm rebuild).npm/csp/lib/resolve.js(new): shared platform resolution required by both the launcher and the postinstall — never overwritten, so re-running postinstall neverrequire()s a native binary as CommonJS.npm/csp/bin/csp.js(hardened fallback launcher): forwards argv, stdio, exit code, and SIGINT/SIGTERM/SIGHUP so killing the launcher cleanly stops a long-runningcsp mcpchild (no orphan); TTY-only fast-path hint (CSP_NO_FALLBACK_WARNING=1to mute); resolves a devtarget/binary from a source checkout. The dev resolver is intentionally not used by the postinstall, so it can never clobber the source shim.generate-platform-packages.mjs: shipsinstall.js+lib/resolve.jsin the published wrapper.package.json:files+postinstall.npm/README.md+CLAUDE.md(model, numbers, bun caveat).Verification
npm install; idempotent over repeated runs (no stray temp).spawnSyncorphaned it), dev-target fallback.install.jsdoes not clobber the sourcebin/csp.jsin a checkout withtarget/built.eslint+tsc --noEmitclean.Summary by cubic
Switch the npm wrapper to a copy-over shim so project
.bin/cspruns the native Rustcspdirectly. This removes Node from startup and cuts launch time from ~59 ms to ~5–12 ms (~10–11×), with the Node launcher kept as a fallback when postinstall is skipped (e.g.--ignore-scriptsor bun without trust).New Features
npm/csp/install.jscopies or hard-links the platform binary overbin/csp.jsvia atomic rename; best-effort, idempotent, and guarded to run only undernode_modules/.npm/csp/lib/resolve.jsused by installer and launcher; avoids requiring a native binary and hardens libc detection by temporarily settingprocess.report.excludeNetwork = true.npm/csp/bin/csp.js: forwards argv/stdio/exit and SIGINT/SIGTERM/SIGHUP, prints a TTY-only hint (mentions--ignore-scriptsand bun trust), and supports a devtarget/binary; Windows intentionally stays on the JS launcher.npm/csp/package.jsonaddspostinstalland shipsinstall.js/lib/resolve.js; generator updated to include both.Bug Fixes
libc: "glibc"to@pleaseai/csp-linux-arm64so musl arm64 is skipped cleanly.Written for commit 71ed3af. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation