feat(cli): guard against missing native binaries#516
Conversation
stash loads native Rust addons (protect-ffi via @cipherstash/stack, and @cipherstash/auth) distributed as per-platform optional npm packages. npm intermittently skips installing these optional deps (npm/cli#4828), leaving the base package present but the platform binary missing. The failure was a raw MODULE_NOT_FOUND stack trace with no guidance. - Split the bin into a thin launcher (stash.ts) that dynamically imports the command graph (main.ts), so a native-load failure during module evaluation is caught and rendered as actionable fix guidance instead of a stack trace. - Add src/native.ts: isNativeBinaryMissing() detection + reportNativeBinaryMissing() guidance, with unit tests. - Add 'stash doctor' to diagnose runtime + native modules. Dispatched before the command graph loads so it runs even when a binary is missing. Interim hardening ahead of the planned Rust/Homebrew CLI.
🦋 Changeset detectedLatest commit: 646354b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesNative Binary Guards and stash doctor Command
Sequence Diagram(s)sequenceDiagram
actor User
participant stash.ts as stash.ts (launcher)
participant native as native.ts
participant main as bin/main.ts
participant doctor as commands/doctor
User->>stash.ts: run stash <command>
alt command === "doctor"
stash.ts->>doctor: import() + doctorCommand()
doctor->>native: isNativeBinaryMissing() / reportNativeBinaryMissing()
doctor-->>User: health report + exit code
else other commands
stash.ts->>main: import() + run()
alt import throws (native binary missing)
main-->>stash.ts: throws ModuleError
stash.ts->>native: isNativeBinaryMissing(err)
stash.ts->>native: reportNativeBinaryMissing(err)
stash.ts-->>User: actionable error + exit 1
else import succeeds
main->>main: parseArgs + dispatch subcommand
main-->>User: command output
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/bin/stash.ts (1)
15-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd required E2E coverage for the launcher error-handling path
This PR changes
stash.tstop-level error handling, but no E2E test is included in the provided changes to validate argv/exit-code behavior for the new launcher path.As per coding guidelines,
packages/cli/src/bin/stash.ts: “Add an E2E test when touchingsrc/bin/stash.tsargv parsing, exit codes, or top-level error handling”.🤖 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/bin/stash.ts` around lines 15 - 46, The bootstrap function in stash.ts has been modified to add top-level error handling with the isNativeBinaryMissing check and reportNativeBinaryMissing call, but no E2E test has been added to validate this behavior. Add an E2E test that covers the error-handling paths in the bootstrap function, specifically validating that the correct exit codes and error messages are returned when isNativeBinaryMissing conditions are met, and also when general errors occur in the catch handler that wraps the bootstrap() call.Source: Coding guidelines
🤖 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/bin/stash.ts`:
- Around line 36-45: The bootstrap error handler in the catch block is logging
the raw error message directly via p.log.error(`Fatal error: ${message}`), which
risks exposing sensitive data like credentials or tokens. Replace the error
logging to use only a generic error message without including the actual error
details from err.message or String(err). Remove the interpolation of the message
variable and log a generic fatal error message instead.
---
Outside diff comments:
In `@packages/cli/src/bin/stash.ts`:
- Around line 15-46: The bootstrap function in stash.ts has been modified to add
top-level error handling with the isNativeBinaryMissing check and
reportNativeBinaryMissing call, but no E2E test has been added to validate this
behavior. Add an E2E test that covers the error-handling paths in the bootstrap
function, specifically validating that the correct exit codes and error messages
are returned when isNativeBinaryMissing conditions are met, and also when
general errors occur in the catch handler that wraps the bootstrap() call.
🪄 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: 59e1fc26-bed9-4515-ac95-81722404e607
📒 Files selected for processing (6)
.changeset/native-binary-guards.mdpackages/cli/src/__tests__/native.test.tspackages/cli/src/bin/main.tspackages/cli/src/bin/stash.tspackages/cli/src/commands/doctor/index.tspackages/cli/src/native.ts
There was a problem hiding this comment.
Pull request overview
This PR improves the stash CLI’s resilience when npm fails to install platform-specific optional native binaries (e.g. @cipherstash/protect-ffi-<platform>-<arch>), replacing raw module-load stack traces with actionable guidance and adding a stash doctor diagnostic command that can run even when native binaries are missing.
Changes:
- Split the CLI entry into a thin launcher (
bin/stash.ts) and dynamically-imported body (bin/main.ts) to catch native module load failures and render friendly remediation guidance. - Add
stash doctorto probe runtime/platform and force-load native-bearing packages, reporting per-check status. - Introduce
src/native.tshelpers to detect missing native optional packages and format user guidance, with unit tests and a Changeset for the CLI release.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/native.ts | Adds native-missing detection and user-facing guidance helpers. |
| packages/cli/src/commands/doctor/index.ts | Implements stash doctor probes and reporting logic. |
| packages/cli/src/bin/stash.ts | New thin launcher that intercepts native-missing errors and dispatches doctor early. |
| packages/cli/src/bin/main.ts | Moves the CLI command graph and help text into a dynamically imported module; adds doctor wiring. |
| packages/cli/src/tests/native.test.ts | Adds Vitest coverage for native-missing detection. |
| .changeset/native-binary-guards.md | Declares a minor version bump for the new guards + doctor command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…r E2E - native.ts: build the re-run command via runnerCommand(detectPackageManager()) instead of a hardcoded `npx`, fixing the lint:runners CI failure and making the guidance correct for bun/pnpm/yarn. The npx cache-clear hint is now shown only for npm. - missingModuleName(): extract the real platform package from the message/ requireStack (so Linux's libc suffix like -linux-x64-gnu is preserved) and parse the ESM 'Cannot find package' form, not just CJS 'Cannot find module'. Fallback wording no longer implies a specific package suffix. (Copilot review) - Add tests/e2e/doctor.e2e.test.ts covering the launcher's doctor dispatch and exit code, per the src/bin/stash.ts coding guideline.
|
Pushed 571e55f addressing the review + the failing CI failure ( @copilot — native.ts:65 (good catch) — fixed. E2E coverage (bin guideline) — added. @coderabbitai — stash.ts:45 (logging |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
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 `@packages/cli/src/native.ts`:
- Around line 63-74: The reinstallCommand function uses POSIX-only shell
commands (rm -rf) that will fail on Windows platforms, even though win32 is an
explicitly supported target. Modify the reinstallCommand function to generate
platform-appropriate recovery commands by detecting the operating system and
returning either POSIX syntax (for Unix-like systems) or Windows-compatible
syntax (using del, rmdir for Windows). Ensure all package manager recovery
commands in the switch statement (for bun, pnpm, yarn, and npm) are adapted to
work correctly on both Windows and Unix-like platforms.
In `@packages/cli/tests/e2e/doctor.e2e.test.ts`:
- Around line 12-15: The test file has hard-coded user-facing strings in expect
assertions on multiple lines (the strings 'stash doctor', the platform string
template, and 'All checks passed'). Replace these hard-coded strings with
imported message constants from src/messages.ts. Import the appropriate message
constants from that file and update the expect statements to assert against
those constants instead of the literal strings, ensuring test expectations
remain synchronized with any future changes to the CLI copy.
🪄 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: 92a40c9f-9b70-4752-aea9-48e57d42ba49
📒 Files selected for processing (2)
packages/cli/src/native.tspackages/cli/tests/e2e/doctor.e2e.test.ts
…ages - native.ts: reinstall/cache-clear guidance now emits PowerShell on win32 (Remove-Item) instead of non-runnable POSIX (rm -rf/$(...)), since win32 is a supported native target. (CodeRabbit) - Extract doctor's asserted user-facing strings into messages.doctor (title, platformLabel, allChecksPassed); doctor and its E2E test reference the constants so copy changes stay in one place. (CodeRabbit)
|
Both addressed in 646354b. native.ts:74 — Windows guidance (good catch). doctor.e2e.test.ts — extract asserted strings. Added a Verified: |
Background
npx stash@latest auth logincan crash with a raw, unactionable stack trace:stashloads native Rust addons —protect-ffi(via@cipherstash/stack) and@cipherstash/auth— distributed as per-platform optional npm packages (@cipherstash/<pkg>-<platform>-<arch>) and selected at runtime by@neon-rs/load. npm intermittently skips installing these optional dependencies (npm/cli#4828), typically after a partial/stale cache (~/.npm/_npxis a common culprit). The base package is present but the platform binary is missing.The packaging is correct (all platforms published, versions pinned) — the failure is npm-side install flakiness made worse by an unfriendly error. These guards turn that into a self-explaining, recoverable experience. Interim hardening ahead of the planned Rust/Homebrew CLI.
What this does
1. Friendly error guard. The bin is split into a thin launcher (
bin/stash.ts, ~1 KB) that dynamically imports the command graph (bin/main.ts). The native addons are pulled in eagerly during that module's evaluation, so the dynamic import lets us wrap evaluation in atry/catch. A missing native binary is detected and rendered as actionable guidance instead of a stack trace:2.
stash doctor. New command that checks the Node runtime, platform, and force-loads each native-bearing package, reporting a per-check status. Dispatched by the launcher before the command graph loads, so it runs even when a binary is missing — i.e. exactly when you need to diagnose.3.
src/native.ts.isNativeBinaryMissing()(distinguishes a missing platform binary from a missing top-level package or any other module error) +reportNativeBinaryMissing(). Unit-tested.Files
src/bin/stash.ts— now a thin launcher (dynamic import + native-error catch)src/bin/main.ts— the formerstash.tsbody, exported asrun()src/native.ts— detection + guidance helperssrc/commands/doctor/index.ts—stash doctorsrc/__tests__/native.test.ts— detection testsVerification
pnpm test— 319 passed (7 new)pnpm run build— clean; entrystash.jsis 983 B and loadsmain(93 KB) as a separate chunk; entry contains zeroprotect-ffi/auth references (native graph no longer eagerly bundled into the entry)stash --version,stash doctor(healthy) verified against the built binaryNotes
biomelint currently fails for an unrelated reason: root pins@biomejs/biome@^2.4.15butbiome.jsonstill uses the 1.8.3 schema (files.ignorewas renamed tofiles.includesin biome 2.x). Not touched here; worth a separate fix. New files follow the existing style (single quotes, no semicolons, 2-space).Summary by CodeRabbit
stash doctorto run a health check, report the current platform target, and verify required runtime components.stash doctor, plus unit coverage for native-binary missing detection.stash doctorsuccess UI text.