From 483a925f5e6e03a2e10e5cf72594361b20d6f5c5 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 13 Aug 2026 09:35:50 +1000 Subject: [PATCH 1/4] feat(doctor): probe the native binding for real (CIP-3720) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making the protect-ffi load lazy cost `stash doctor` its only real check: `@neon-rs/load`'s proxy resolves the platform binary on first property access inside a wrapper body, so importing the package proves nothing and the failure lands later, at the first encrypt. Add `@cipherstash/stack/diagnostics` — one export, `assertNativeBindingAvailable()`, re-exported from protect-ffi. There is no consumer-side alternative: the loader is not in protect-ffi's `exports` map, and reading an export never reaches the proxy. Its own tsup config with `splitting: false`, because what it must NOT reach is the point of it — a shared chunk importing `@cipherstash/auth` would put the probe back on auth's binary. That was the second bug: the root entry re-exports the auth strategies, and auth's binding is eager, so the encryption row was a second auth check — two rows, one signal. And a missing auth binary printed a bare `Fatal error` from every command, not just doctor: napi's loader swallows the resolver's MODULE_NOT_FOUND and throws a summary with no error `code`, which the classifier could not recognise. Two more the tests found: * "not installed" was matched by looking for the package name anywhere in the message. The probe's own import path contains it, so an installed but broken `@cipherstash/stack` was reported as one you had not installed yet — green, with nothing to suggest looking further. Match on the specifier Node failed to resolve instead. * A run that could not complete a check said every check passed. It now says so, and still exits 0: an unrunnable check is not a diagnosis. Coverage for all four classification arms. The e2e fixtures make a binary or a package genuinely unresolvable in the spawned CLI — `Module._load` for the CJS binding, a `module.registerHooks` resolve hook for the ESM probe — so every error asserted on is one Node raised. The one that matters most is non-vacuity: hiding protect-ffi's binary must redden exactly one row. Before this change, hiding auth's reddened both and hiding protect-ffi's reddened neither. --- .changeset/stack-diagnostics-subpath.md | 11 + .../stash-doctor-probes-native-bindings.md | 15 ++ AGENTS.md | 2 +- .../doctor-probe-classification.test.ts | 130 ++++++++++ packages/cli/src/__tests__/native.test.ts | 55 +++- packages/cli/src/commands/doctor/index.ts | 138 ++++++++-- packages/cli/src/messages.ts | 20 ++ packages/cli/src/native.ts | 29 ++- .../e2e/doctor-missing-binary.e2e.test.ts | 159 ++++++++++++ .../doctor-probe-classification.e2e.test.ts | 165 ++++++++++++ .../stack/__tests__/diagnostics-entry.test.ts | 238 ++++++++++++++++++ packages/stack/package.json | 10 + packages/stack/src/diagnostics.ts | 27 ++ packages/stack/tsup.config.ts | 29 ++- 14 files changed, 982 insertions(+), 46 deletions(-) create mode 100644 .changeset/stack-diagnostics-subpath.md create mode 100644 .changeset/stash-doctor-probes-native-bindings.md create mode 100644 packages/cli/src/__tests__/doctor-probe-classification.test.ts create mode 100644 packages/cli/tests/e2e/doctor-missing-binary.e2e.test.ts create mode 100644 packages/cli/tests/e2e/doctor-probe-classification.e2e.test.ts create mode 100644 packages/stack/__tests__/diagnostics-entry.test.ts create mode 100644 packages/stack/src/diagnostics.ts diff --git a/.changeset/stack-diagnostics-subpath.md b/.changeset/stack-diagnostics-subpath.md new file mode 100644 index 000000000..e23a0f0c3 --- /dev/null +++ b/.changeset/stack-diagnostics-subpath.md @@ -0,0 +1,11 @@ +--- +'@cipherstash/stack': minor +--- + +Add a `@cipherstash/stack/diagnostics` subpath, for tooling that needs to prove the protect-ffi native binding is installed. + +It exports one function, `assertNativeBindingAvailable()`, re-exported from `@cipherstash/protect-ffi`. Calling it forces the platform binary to load and throws the loader's own `MODULE_NOT_FOUND` — unwrapped, naming the missing `@cipherstash/protect-ffi--` package — if it is absent. Importing the subpath does not force anything, so the laziness that makes the native load cost nothing for callers that never encrypt is preserved. + +The subpath exists because there is no way to do this from outside: the package's loader is not in its `exports` map, and reading an export never reaches the `@neon-rs/load` proxy. Importing `@cipherstash/stack` itself is not a substitute either — the root entry re-exports the auth strategies, so evaluating it resolves `@cipherstash/auth`'s binding instead. This entry reaches protect-ffi and nothing else. + +Available as both `import` and `require`. diff --git a/.changeset/stash-doctor-probes-native-bindings.md b/.changeset/stash-doctor-probes-native-bindings.md new file mode 100644 index 000000000..ff94dcd6e --- /dev/null +++ b/.changeset/stash-doctor-probes-native-bindings.md @@ -0,0 +1,15 @@ +--- +'stash': patch +--- + +`stash doctor` now detects a missing native binary. Both of its checks had stopped doing so, in different ways, and each reported a green row instead. + +**The encryption engine check never loaded anything.** Since the protect-ffi native load became lazy, importing the package resolves no platform binary — `@neon-rs/load`'s proxy resolves on first use — so the probe passed with nothing installed and the failure surfaced later, at the first encrypt. It now calls `assertNativeBindingAvailable()` through the new `@cipherstash/stack/diagnostics` subpath, which forces the load. + +**It was also reporting the wrong package.** Importing `@cipherstash/stack` reaches `@cipherstash/auth`, whose binding is eager, so the encryption row was really a second auth check: one signal rendered as two rows. The diagnostics subpath does not reach auth, so each row now means what it says. + +**A missing `@cipherstash/auth` binary produced a bare `Fatal error`.** That package's napi loader swallows the resolver's `MODULE_NOT_FOUND` and throws a summary carrying no error `code`, which the CLI's native-binary classifier did not recognise — so every command, not only `doctor`, skipped the recovery guidance and printed a raw message. The classifier now recognises that shape, and prints the missing package with the reinstall steps. + +`stash doctor` exits non-zero when either platform package is missing, and reports an install of `@cipherstash/stack` that predates the diagnostics subpath as unprobeable rather than failing on it. A run that could not complete a check now ends with "stash doctor could not run every check." instead of claiming they all passed — still exit 0, since an unrunnable check is not a diagnosis. + +**A package that is installed but broken is no longer reported as "not installed".** The check for an absent package matched the package name anywhere in the failure message, and the probe's own import path contains it — so a partially installed or partially built `@cipherstash/stack` was reported as one you simply had not installed yet, in green, with nothing to suggest looking further. It now matches on the specifier Node failed to resolve. diff --git a/AGENTS.md b/AGENTS.md index da9f95f20..c040b78bb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -72,7 +72,7 @@ If these variables are missing, tests that require live encryption will fail or ## Repository Layout - `packages/stack`: Main package (`@cipherstash/stack`) containing the encryption client and all integrations - - Subpath exports: `@cipherstash/stack`, `@cipherstash/stack/identity`, `@cipherstash/stack/schema`, `@cipherstash/stack/eql/v3`, `@cipherstash/stack/v3`, `@cipherstash/stack/types`, `@cipherstash/stack/dynamodb`, `@cipherstash/stack/encryption`, `@cipherstash/stack/errors`, `@cipherstash/stack/adapter-kit`, `@cipherstash/stack/wasm-inline` (the Drizzle and Supabase integrations moved to their own packages — see below) + - Subpath exports: `@cipherstash/stack`, `@cipherstash/stack/identity`, `@cipherstash/stack/schema`, `@cipherstash/stack/eql/v3`, `@cipherstash/stack/v3`, `@cipherstash/stack/types`, `@cipherstash/stack/dynamodb`, `@cipherstash/stack/encryption`, `@cipherstash/stack/errors`, `@cipherstash/stack/adapter-kit`, `@cipherstash/stack/wasm-inline`, `@cipherstash/stack/diagnostics` (the Drizzle and Supabase integrations moved to their own packages — see below) - `packages/cli`: The `stash` CLI — auth, init, encryption schema, and database setup (`stash eql install`). Has its own `AGENTS.md`. - `packages/wizard`: AI-powered encryption setup (`@cipherstash/wizard`) - `packages/migrate`: Plaintext-to-encrypted column migration (`@cipherstash/migrate`) — resumable backfill, per-column state diff --git a/packages/cli/src/__tests__/doctor-probe-classification.test.ts b/packages/cli/src/__tests__/doctor-probe-classification.test.ts new file mode 100644 index 000000000..f3e8974d4 --- /dev/null +++ b/packages/cli/src/__tests__/doctor-probe-classification.test.ts @@ -0,0 +1,130 @@ +import { createRequire } from 'node:module' +import path from 'node:path' +import { fileURLToPath } from 'node:url' +import { describe, expect, it } from 'vitest' +import { + isPackageMissing, + isSubpathUnavailable, +} from '../commands/doctor/index.js' +import { isNativeBinaryMissing } from '../native.js' + +// The classifiers `stash doctor` sorts a failed probe with. Each arm renders a +// different row and a different exit code, so a probe error landing in the +// wrong one is a wrong diagnosis, not a cosmetic slip. +// +// Every fixture below is an error NODE raised, never one built by hand with the +// code and message pasted on. A hand-built fixture asserts on itself: it keeps +// passing when Node changes the shape the classifier has to recognise, which is +// exactly how `isNativeBinaryMissing` came to have a `@cipherstash/auth` arm +// that could never fire. +const require = createRequire(import.meta.url) + +function resolutionError(specifier: string): unknown { + try { + require.resolve(specifier) + } catch (err) { + return err + } + throw new Error(`${specifier} resolved; it was expected to fail`) +} + +// `stash` declares `@cipherstash/stack` as an OPTIONAL PEER at `>=1.0.0-rc.0`, +// and the encryption probe imports its `./diagnostics` subpath. So any install +// predating that subpath — a range the CLI itself permits — fails resolution +// with neither a missing package nor a missing binary. Without its own arm that +// error reaches doctor's `else` and is rethrown, surfacing as the launcher's +// bare `Fatal error` for an install that may be perfectly healthy. +describe('isSubpathUnavailable', () => { + it('matches an installed package that does not publish the subpath', () => { + // A real package (so resolution gets as far as reading its exports map) + // with a subpath it has never had. + const err = resolutionError('@cipherstash/stack/no-such-subpath') + + expect((err as { code?: string }).code).toBe( + 'ERR_PACKAGE_PATH_NOT_EXPORTED', + ) + expect(isSubpathUnavailable(err)).toBe(true) + }) + + it('does not match the errors the other arms own', () => { + // An absent package: Node names the BASE package here, which is why the + // probe classifies against `@cipherstash/stack` while importing the + // subpath, and why this must not be mistaken for a stale install. + const missing = resolutionError('@cipherstash/no-such-package/diagnostics') + expect((missing as { code?: string }).code).toBe('MODULE_NOT_FOUND') + expect(isSubpathUnavailable(missing)).toBe(false) + + const binary = new Error( + "Cannot find module '@cipherstash/protect-ffi-darwin-arm64'", + ) as Error & { code?: string } + binary.code = 'MODULE_NOT_FOUND' + expect(isSubpathUnavailable(binary)).toBe(false) + expect(isNativeBinaryMissing(binary)).toBe(true) + }) + + it('ignores non-Error values', () => { + expect(isSubpathUnavailable(undefined)).toBe(false) + expect( + isSubpathUnavailable({ code: 'ERR_PACKAGE_PATH_NOT_EXPORTED' }), + ).toBe(false) + }) +}) + +// The arm that decides whether a probe failure means "you have not installed +// this yet" — a green row for the optional peer — or something the user has to +// act on. Getting a false positive here is the worst outcome doctor has: it +// tells a user with a broken install that there is nothing to fix. +describe('isPackageMissing', () => { + it('matches the package the probe named', () => { + const err = resolutionError('@cipherstash/no-such-package/diagnostics') + + expect(isPackageMissing(err, '@cipherstash/no-such-package')).toBe(true) + }) + + it('does not match a load failure for a file inside an installed package', () => { + // The probe imports `@cipherstash/stack/diagnostics`, so its failures name + // paths INSIDE the package — `…/node_modules/@cipherstash/stack/dist/ + // diagnostics.js`. A substring test for the package name matches that + // happily and reports a package that is installed but broken (an + // interrupted install, a partially built workspace) as one the user has + // simply not installed yet: a green row, and no reason to look further. + // + // The path the CLI's own resolution produces — `packages/cli/node_modules/ + // @cipherstash/stack/…`, the workspace's stand-in for a user's install. + // Not `require.resolve('@cipherstash/stack/package.json')`: that returns + // the symlink's REAL path (`packages/stack/…`), which drops the scoped + // name the bug turns on. Node raises the error either way; only the path + // handed to it is composed here. + // + // Resolved through CJS to keep Node's own resolver in play rather than + // Vitest's module pipeline. The ESM form of this message differs only by a + // trailing `imported from …`, and both quote the same specifier. + const cliRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '../..', + ) + const err = resolutionError( + path.join( + cliRoot, + 'node_modules/@cipherstash/stack/dist/no-such-file.js', + ), + ) + + expect((err as { code?: string }).code).toBe('MODULE_NOT_FOUND') + expect((err as Error).message).toContain('@cipherstash/stack') + expect(isPackageMissing(err, '@cipherstash/stack')).toBe(false) + }) + + it('does not match a sibling package that merely shares a prefix', () => { + const err = resolutionError('@cipherstash/no-such-package-extra') + + expect(isPackageMissing(err, '@cipherstash/no-such-package')).toBe(false) + }) + + it('ignores errors that are not module resolution failures', () => { + expect( + isPackageMissing(new Error('@cipherstash/stack'), '@cipherstash/stack'), + ).toBe(false) + expect(isPackageMissing(undefined, '@cipherstash/stack')).toBe(false) + }) +}) diff --git a/packages/cli/src/__tests__/native.test.ts b/packages/cli/src/__tests__/native.test.ts index 544b13f3d..8a98a94bd 100644 --- a/packages/cli/src/__tests__/native.test.ts +++ b/packages/cli/src/__tests__/native.test.ts @@ -16,6 +16,20 @@ function moduleError( return err } +/** + * What napi-rs's generated loader throws when the platform package is absent — + * `@cipherstash/auth`'s shape. Verbatim from `stack-auth-node.js`, whose + * `loadBinding()` swallows each candidate's resolver error (`try { … } catch + * (_) {}`) and ends with this, so no `code` and no `requireStack` survive. + */ +function napiLoadError(pkg: string): Error { + return new Error( + `Failed to load native binding for ${process.platform}-${process.arch}. ` + + `Ensure the optional dependency "${pkg}" is installed, ` + + 'or run "napi build" for local development.', + ) +} + describe('isNativeBinaryMissing', () => { it('matches a missing platform-specific protect-ffi binary', () => { // The real-world failure: npm skipped the optional native dependency. @@ -29,17 +43,38 @@ describe('isNativeBinaryMissing', () => { expect(isNativeBinaryMissing(err)).toBe(true) }) - it('matches the auth native binary on linux/windows targets', () => { - expect( - isNativeBinaryMissing( - moduleError("Cannot find module '@cipherstash/auth-linux-x64-gnu'"), - ), - ).toBe(true) + it('matches the auth native binary across targets', () => { + // Auth is napi-rs, so these are the shape below, not `moduleError` — kept + // as a set because the platform token varies (`-gnu`, `-msvc`, plain). + for (const pkg of [ + '@cipherstash/auth-linux-x64-gnu', + '@cipherstash/auth-win32-x64-msvc', + '@cipherstash/auth-darwin-arm64', + ]) { + expect(isNativeBinaryMissing(napiLoadError(pkg)), pkg).toBe(true) + } + }) + + it('matches the napi loader failure, which carries no code at all', () => { + // The shape that made this whole helper a no-op for `@cipherstash/auth`. + // Its loader requires each candidate inside `try { … } catch (_) {}` and + // throws its own summary, so the resolver's MODULE_NOT_FOUND never + // escapes: no `code`, no `requireStack`, only the message. This assertion + // existed before as a hand-built error carrying `code = + // 'MODULE_NOT_FOUND'` — a shape `@cipherstash/auth` has never thrown, so + // it passed over a path that could not work, and `stash doctor` printed a + // bare `Fatal error` for a missing auth binary instead of the guidance. + const err = napiLoadError('@cipherstash/auth-darwin-arm64') + expect((err as ModuleError).code).toBeUndefined() + expect(isNativeBinaryMissing(err)).toBe(true) + }) + + it('does not match a napi loader failure from someone else', () => { + // Both halves of the message test have to hold: the platform package is + // what makes it ours. expect( - isNativeBinaryMissing( - moduleError("Cannot find module '@cipherstash/auth-win32-x64-msvc'"), - ), - ).toBe(true) + isNativeBinaryMissing(napiLoadError('@other/thing-darwin-arm64')), + ).toBe(false) }) it('matches when only the neon loader appears in the require stack', () => { diff --git a/packages/cli/src/commands/doctor/index.ts b/packages/cli/src/commands/doctor/index.ts index bc5ec8542..f5a5c83c6 100644 --- a/packages/cli/src/commands/doctor/index.ts +++ b/packages/cli/src/commands/doctor/index.ts @@ -1,74 +1,158 @@ import * as p from '@clack/prompts' import { messages } from '../../messages.js' +import { + isModuleNotFound, + moduleNotFoundSpecifier, +} from '../../module-error.js' import { currentTarget, isNativeBinaryMissing, reportNativeBinaryMissing, } from '../../native.js' -// Native-bearing packages the CLI loads at runtime. Importing each forces its -// @neon-rs/load proxy to resolve the platform binary — the same load that fails -// when npm skips the optional dependency. @cipherstash/stack is the peer that -// pulls protect-ffi; it may legitimately be absent until `stash init`. -const PROBES: { label: string; pkg: string; optional?: boolean }[] = [ +// Native-bearing packages the CLI loads at runtime, and — per package — the +// operation that forces its platform binary to resolve. +// +// Importing a package is not a probe on its own. It was, for protect-ffi, until +// the load became lazy: `@neon-rs/load`'s proxy now resolves the binary on +// first property access inside a wrapper body, so the package imports cleanly +// with nothing installed and fails at the first encrypt instead. Each probe +// therefore names what to CALL, not just what to import. +interface Probe { + label: string + /** + * The package a "not installed" message names, which is not always what the + * probe imports: Node reports the base package for a missing subpath of an + * absent package, so the encryption probe imports `…/diagnostics` and + * classifies against `@cipherstash/stack`. + */ + pkg: string + /** May legitimately be absent until `stash init`, so absence is not failure. */ + optional?: boolean + force(): Promise +} + +const PROBES: Probe[] = [ { label: 'Encryption engine (@cipherstash/stack → protect-ffi)', pkg: '@cipherstash/stack', optional: true, + async force() { + // `@cipherstash/stack/diagnostics` exists for this call: it reaches + // protect-ffi WITHOUT reaching `@cipherstash/auth`, which the root entry + // does (it re-exports the auth strategies, and that package's binding is + // eager). Probing the root entry measured auth's binary and reported it + // under this label — two rows, one signal. + const diagnostics = await import('@cipherstash/stack/diagnostics') + diagnostics.assertNativeBindingAvailable() + }, + }, + { + label: 'Auth (@cipherstash/auth)', + pkg: '@cipherstash/auth', + async force() { + // No counterpart call needed. This package's entry is `module.exports = + // { ...native }`, and the spread resolves the binding at module + // evaluation — so here the import IS the probe. + await import('@cipherstash/auth') + }, }, - { label: 'Auth (@cipherstash/auth)', pkg: '@cipherstash/auth' }, ] -function report(ok: boolean, label: string, detail?: string) { +type Outcome = 'ok' | 'warn' | 'fail' + +function report(outcome: Outcome, label: string, detail?: string) { const text = detail ? `${label} — ${detail}` : label - if (ok) p.log.success(text) + if (outcome === 'ok') p.log.success(text) + else if (outcome === 'warn') p.log.warn(text) else p.log.error(text) } -function isPackageMissing(err: unknown, pkg: string): boolean { +/** + * True when `pkg` itself is what failed to resolve — the package is not + * installed, as opposed to installed and unable to load something. + * + * Matched on the SPECIFIER Node quotes, not on the message text. A substring + * test reads the probe's own import path as a hit: `@cipherstash/stack/ + * diagnostics` failing on a missing `dist/` file names + * `…/node_modules/@cipherstash/stack/dist/diagnostics.js`, which contains the + * package name, and doctor would call an installed-but-broken package "not + * installed" — a green row, and no reason for the user to look further. It also + * swallowed the neighbours: `@cipherstash/stack-drizzle` contains + * `@cipherstash/stack`. + */ +export function isPackageMissing(err: unknown, pkg: string): boolean { + if (!isModuleNotFound(err)) return false + const specifier = moduleNotFoundSpecifier(err) + if (specifier === undefined) return false + // The probe imports a subpath, and Node quotes the base package for an absent + // one under ESM but the full specifier under CJS. Both mean this package. + return specifier === pkg || specifier.startsWith(`${pkg}/`) +} + +/** + * True when the package is installed but does not publish the subpath the probe + * asked for — an `@cipherstash/stack` older than `./diagnostics`. + * + * Its own arm because the alternative is the `else` below: an unclassified + * error, rethrown, surfacing as a bare `Fatal error` from the launcher. `stash` + * declares `@cipherstash/stack` as an OPTIONAL PEER with a wide range, so every + * install predating that subpath lands here — a case a user hits by doing + * nothing wrong. + */ +export function isSubpathUnavailable(err: unknown): boolean { if (!(err instanceof Error)) return false - const code = (err as { code?: string }).code - if (code !== 'ERR_MODULE_NOT_FOUND' && code !== 'MODULE_NOT_FOUND') { - return false - } - return err.message.includes(pkg) + return (err as { code?: string }).code === 'ERR_PACKAGE_PATH_NOT_EXPORTED' } export async function doctorCommand(): Promise { p.intro(messages.doctor.title) let failed = false + // A check that could not be RUN, as distinct from one that ran and failed. + // Tracked separately so the outro can say so — `failed` would exit 1 on an + // install with nothing known to be wrong with it, and neither flag would + // claim every check passed. + let incomplete = false let nativeError: unknown const nodeMajor = Number(process.versions.node.split('.')[0]) const nodeOk = Number.isFinite(nodeMajor) && nodeMajor >= 22 report( - nodeOk, + nodeOk ? 'ok' : 'fail', `Node.js ${process.versions.node}`, nodeOk ? '' : 'requires >= 22', ) if (!nodeOk) failed = true - report(true, `${messages.doctor.platformLabel} ${currentTarget()}`) + report('ok', `${messages.doctor.platformLabel} ${currentTarget()}`) for (const probe of PROBES) { try { - await import(probe.pkg) - report(true, probe.label) + await probe.force() + report('ok', probe.label) } catch (err) { if (isNativeBinaryMissing(err)) { - report(false, probe.label, 'native binary missing') + report('fail', probe.label, messages.doctor.nativeBinaryMissing) failed = true - nativeError = err + // First one wins. The guidance below is the same whichever package + // reported it; keeping the first keeps the note aligned with the first + // failing row rather than the last. + nativeError ??= err } else if (isPackageMissing(err, probe.pkg)) { // A missing top-level package is a different problem from a missing // native binary; only the latter is what these guards exist for. report( - Boolean(probe.optional), + probe.optional ? 'ok' : 'fail', probe.label, - probe.optional ? 'not installed (run `stash init`)' : 'not installed', + probe.optional + ? messages.doctor.notInstalledOptional + : messages.doctor.notInstalled, ) if (!probe.optional) failed = true + } else if (isSubpathUnavailable(err)) { + report('warn', probe.label, messages.doctor.cannotProbe) + incomplete = true } else { throw err } @@ -80,8 +164,14 @@ export async function doctorCommand(): Promise { } if (failed) { - p.outro('stash doctor found problems.') + p.outro(messages.doctor.problemsFound) process.exit(1) } - p.outro(messages.doctor.allChecksPassed) + // Exit 0 either way — an unrunnable check is not a diagnosis — but only one + // of these two is true. + p.outro( + incomplete + ? messages.doctor.checksIncomplete + : messages.doctor.allChecksPassed, + ) } diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts index aff38c792..fc9e77251 100644 --- a/packages/cli/src/messages.ts +++ b/packages/cli/src/messages.ts @@ -27,6 +27,26 @@ export const messages = { /** Leader of the platform check line; the `-` is appended. */ platformLabel: 'Platform', allChecksPassed: 'All checks passed.', + /** Row detail when a probe reached the loader and no platform binary was there. */ + nativeBinaryMissing: 'native binary missing', + /** Row detail for an absent optional package — recoverable, not a failure. */ + notInstalledOptional: 'not installed (run `stash init`)', + /** Row detail for an absent required package. */ + notInstalled: 'not installed', + /** + * Row detail when `@cipherstash/stack` is installed but predates the + * `./diagnostics` subpath, so there is no way to force its binding. Not a + * failure: the install may be perfectly healthy, we just can't prove it. + */ + cannotProbe: 'installed, but too old to probe — upgrade @cipherstash/stack', + problemsFound: 'stash doctor found problems.', + /** + * Outro when a check could not be run but nothing failed. Distinct from + * `allChecksPassed` on purpose: a run that skipped a check has not passed + * it, and saying otherwise is the one line of doctor's output a user has no + * way to second-guess. + */ + checksIncomplete: 'stash doctor could not run every check.', }, auth: { /** Same shape as `cli.usagePrefix` — leader only. */ diff --git a/packages/cli/src/native.ts b/packages/cli/src/native.ts index e79e33e38..011469b80 100644 --- a/packages/cli/src/native.ts +++ b/packages/cli/src/native.ts @@ -32,19 +32,34 @@ export function currentTarget(): string { return `${process.platform}-${process.arch}` } +// napi-rs's generated loader, which is how `@cipherstash/auth` loads (neon is +// only protect-ffi). It requires each candidate inside `try { … } catch (_) {}` +// and then throws this — so the resolver's MODULE_NOT_FOUND never escapes, and +// what does escape is a plain Error with NO `code` and no `requireStack`. It +// names the platform package, which is the only thing left to key on. +const NAPI_LOAD_FAILURE = /Failed to load native binding for / + /** * True when `err` is a failure to load one of our prebuilt native addons (a * missing `@cipherstash/--` optional package), as opposed * to a missing top-level package or any other module error. */ export function isNativeBinaryMissing(err: unknown): err is ModuleError { - if (!isModuleNotFound(err)) return false - const haystack = `${err.message}\n${(err.requireStack ?? []).join('\n')}` - // A platform-suffixed @cipherstash package, or a failure surfaced from the - // neon loader, both mean the optional native binary wasn't installed. - return ( - PLATFORM_PKG.test(haystack) || /[\\/]@neon-rs[\\/]load[\\/]/.test(haystack) - ) + if (isModuleNotFound(err)) { + const haystack = `${err.message}\n${(err.requireStack ?? []).join('\n')}` + // A platform-suffixed @cipherstash package, or a failure surfaced from the + // neon loader, both mean the optional native binary wasn't installed. + return ( + PLATFORM_PKG.test(haystack) || + /[\\/]@neon-rs[\\/]load[\\/]/.test(haystack) + ) + } + + if (!(err instanceof Error)) return false + // The code-less napi shape. Both halves are required: neither is narrow + // enough alone — `Failed to load native binding` pins the thrower, the + // platform package pins it to one of ours. No `requireStack` to consult. + return NAPI_LOAD_FAILURE.test(err.message) && PLATFORM_PKG.test(err.message) } function missingModuleName(err: ModuleError): string | undefined { diff --git a/packages/cli/tests/e2e/doctor-missing-binary.e2e.test.ts b/packages/cli/tests/e2e/doctor-missing-binary.e2e.test.ts new file mode 100644 index 000000000..8d216aad6 --- /dev/null +++ b/packages/cli/tests/e2e/doctor-missing-binary.e2e.test.ts @@ -0,0 +1,159 @@ +import { mkdtempSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' +import { messages } from '../../src/messages.js' +import { render } from '../helpers/pty.js' + +// The case `stash doctor` exists for, and the one nothing covered: a platform +// binary that npm skipped (https://github.com/npm/cli/issues/4828). +// +// `doctor.e2e.test.ts` runs the healthy install. That cannot catch a probe that +// has stopped probing — and both of them had. Since the protect-ffi load became +// lazy, importing the package resolves no binary at all, so the encryption row +// went green with nothing installed; and `@cipherstash/auth`'s napi loader +// throws a code-less Error the CLI's classifier did not recognise, so a missing +// auth binary surfaced as a bare `Fatal error` with none of the recovery +// guidance. Both are only visible from outside the process, with a binary +// actually absent. +// +// Absent by patching the resolver in the spawned CLI, not by moving files: the +// suite must not mutate the checkout it runs in, and this works whether or not +// the developer has run `build:native`. Same technique as +// `packages/protect-ffi/src/lintWiring.test.ts`, which re-runs its own suite +// this way. + +interface Target { + /** Package whose platform binary disappears. */ + pkg: '@cipherstash/protect-ffi' | '@cipherstash/auth' + /** The doctor row that must go red. */ + label: string + /** Extra specifiers this package's loader reaches for. */ + extra: string +} + +const TARGETS: Target[] = [ + { + pkg: '@cipherstash/protect-ffi', + label: 'Encryption engine (@cipherstash/stack → protect-ffi)', + // `src/load.cts`'s debug arm — a local cargo build sitting beside `lib/`. + // A contributor who has run `pnpm run debug` has one, and without this the + // fixture would load the binding it means to be missing. + extra: String.raw`|(?:^|[\\/])index\.node$`, + }, + { + pkg: '@cipherstash/auth', + label: 'Auth (@cipherstash/auth)', + // napi's local-build arm, tried before the platform package. + extra: String.raw`|(?:^|[\\/])stack-auth-node\.node$`, + }, +] + +/** + * Writes a CJS preload that makes `pkg`'s platform binary unresolvable, and + * returns the `NODE_OPTIONS` value that loads it. + * + * `Module._load`, not `Module._resolveFilename`: `_load` keys a relative-resolve + * fast path on (parent directory, request) and returns cached modules without + * consulting the resolver, so a redirect installed there can simply never be + * asked. `fs` is patched alongside it because a loader that stats an artifact + * before requiring it would otherwise see a file that `require` then refuses. + */ +function hideBinaryOf(target: Target): string { + const dir = mkdtempSync(join(tmpdir(), 'stash-doctor-missing-')) + const preload = join(dir, 'hide-binding.cjs') + const scope = target.pkg.replace('@cipherstash/', '') + writeFileSync( + preload, + ` +const fs = require('node:fs') +const Module = require('node:module') + +const BINDING = /@cipherstash[\\\\/]${scope}-(?:darwin|linux|win32)-${target.extra}/ + +const load = Module._load +Module._load = function (request, parent, isMain) { + if (BINDING.test(request)) { + const error = new Error("Cannot find module '" + request + "'") + error.code = 'MODULE_NOT_FOUND' + throw error + } + return load.call(this, request, parent, isMain) +} + +const existsSync = fs.existsSync +fs.existsSync = (p) => (BINDING.test(String(p)) ? false : existsSync(p)) + +const statSync = fs.statSync +fs.statSync = (p, ...rest) => { + if (!BINDING.test(String(p))) return statSync(p, ...rest) + const error = new Error('ENOENT: no such file or directory, stat ' + p) + error.code = 'ENOENT' + throw error +} + +Module.syncBuiltinESMExports() +`, + ) + // Quoted: Node splits NODE_OPTIONS on whitespace unless a value is wrapped in + // double quotes, and `preload` sits under a tmpdir this file did not choose. + return [process.env.NODE_OPTIONS, `--require "${preload}"`] + .filter(Boolean) + .join(' ') +} + +const currentTarget = `${process.platform}-${process.arch}` + +describe('stash doctor — a platform binary is missing', () => { + it.each( + TARGETS.map((t) => [t.pkg, t] as const), + )('fails the %s row, names the package and exits non-zero', async (_pkg, target) => { + // Wider than the 100-col default so the note's `Missing package:` line is + // asserted as written rather than as clack happened to wrap it. + const r = render(['doctor'], { + env: { NODE_OPTIONS: hideBinaryOf(target) }, + cols: 140, + }) + const { exitCode } = await r.exit + + expect( + exitCode, + `stash doctor passed with no ${target.pkg} binary installed:\n${r.output}`, + ).toBe(1) + expect(r.output).toContain( + `${target.label} — ${messages.doctor.nativeBinaryMissing}`, + ) + expect(r.output).toContain(messages.doctor.problemsFound) + + // The guidance, not just a red row: the platform package by name is the + // one piece of it a user cannot work out for themselves. + expect(r.output).toContain(`${target.pkg}-${currentTarget}`) + expect(r.output).not.toContain('Fatal error') + }) + + it('fails only the row whose binary is missing', async () => { + // Non-vacuity for the pair above. Both probes went through + // `@cipherstash/stack` before this change — the root entry reaches + // `@cipherstash/auth`, whose binding is eager — so hiding auth's binary + // reddened both rows and hiding protect-ffi's reddened neither. Either way + // the two rows reported one signal, and the assertions above cannot see it. + const protectFfi = TARGETS[0] + if (!protectFfi) throw new Error('TARGETS is empty') + + const r = render(['doctor'], { + env: { NODE_OPTIONS: hideBinaryOf(protectFfi) }, + cols: 140, + }) + await r.exit + + const failures = r.output.match( + new RegExp(messages.doctor.nativeBinaryMissing, 'g'), + ) + expect( + failures, + `expected exactly one failing row:\n${r.output}`, + ).toHaveLength(1) + expect(r.output).toContain(protectFfi.label) + expect(r.output).not.toContain('@cipherstash/auth-') + }) +}) diff --git a/packages/cli/tests/e2e/doctor-probe-classification.e2e.test.ts b/packages/cli/tests/e2e/doctor-probe-classification.e2e.test.ts new file mode 100644 index 000000000..9385a3ea9 --- /dev/null +++ b/packages/cli/tests/e2e/doctor-probe-classification.e2e.test.ts @@ -0,0 +1,165 @@ +import { mkdtempSync, writeFileSync } from 'node:fs' +import { registerHooks } from 'node:module' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { pathToFileURL } from 'node:url' +import { describe, expect, it } from 'vitest' +import { messages } from '../../src/messages.js' +import { render } from '../helpers/pty.js' + +// `doctor` sorts a probe failure into four arms, and only one of them — the +// missing platform binary — had coverage (`doctor-missing-binary.e2e.test.ts`). +// These are the two a user reaches by doing nothing wrong: +// +// * `@cipherstash/stack` absent. It is an OPTIONAL PEER, so `npx stash +// doctor` in a project that has not run `stash init` lands here. Must read +// as recoverable, not as a failure. +// * `@cipherstash/stack` installed but older than the `./diagnostics` +// subpath — every version inside the `>=1.0.0-rc.0` peer range that the CLI +// itself permits. Unclassified, this reaches doctor's `else`, is rethrown, +// and surfaces as the launcher's bare `Fatal error`. +// +// Both errors are raised by NODE'S OWN resolver, never hand-built: a +// `module.registerHooks` resolve hook re-points the specifier and lets +// resolution fail on its own terms. A fixture that pasted the message and code +// on by hand would keep passing if Node ever changed either, which is the whole +// risk being covered here. + +/** Mirrors the probe labels in `src/commands/doctor/index.ts`. */ +const ENCRYPTION_LABEL = 'Encryption engine (@cipherstash/stack → protect-ffi)' +const AUTH_LABEL = 'Auth (@cipherstash/auth)' + +interface Unresolve { + /** The bare specifier to divert. */ + specifier: string + /** + * Resolve this instead — a subpath the package really does not publish, so + * Node raises its own `ERR_PACKAGE_PATH_NOT_EXPORTED`. Omit to keep the + * specifier and move the IMPORTER to an empty directory instead, which is how + * an absent package is staged: same specifier, no `node_modules` chain to + * find it in, so Node raises `ERR_MODULE_NOT_FOUND` naming the base package. + */ + redirectTo?: string +} + +/** + * Writes a hook module that makes `specifier` unresolvable in the spawned CLI, + * and returns the `NODE_OPTIONS` value that loads it. + * + * A resolve hook rather than the `Module._load` patch its sibling suite uses: + * the probe is an `await import()` of a real ESM package, so it never reaches + * the CJS loader. `registerHooks` is synchronous and in-process, so the error + * propagates out of the dynamic import exactly as an unhooked failure would. + */ +function unresolve({ specifier, redirectTo }: Unresolve): string { + const dir = mkdtempSync(join(tmpdir(), 'stash-doctor-probe-')) + const hook = join(dir, 'unresolve.mjs') + // Importer inside the fresh temp dir. It is never written — resolution fails + // before anything is read — it only has to sit somewhere with no + // `node_modules` above it holding the package. + const emptyParent = pathToFileURL(join(dir, 'importer.mjs')).href + writeFileSync( + hook, + `import { registerHooks } from 'node:module' + +const SPECIFIER = ${JSON.stringify(specifier)} +const REDIRECT = ${JSON.stringify(redirectTo ?? null)} +const EMPTY_PARENT = ${JSON.stringify(emptyParent)} + +registerHooks({ + resolve(specifier, context, nextResolve) { + if (specifier !== SPECIFIER) return nextResolve(specifier, context) + if (REDIRECT) return nextResolve(REDIRECT, context) + return nextResolve(specifier, { ...context, parentURL: EMPTY_PARENT }) + }, +}) +`, + ) + // Quoted: Node splits NODE_OPTIONS on whitespace unless a value is wrapped in + // double quotes, and the hook sits under a tmpdir this file did not choose. + return [process.env.NODE_OPTIONS, `--import "${pathToFileURL(hook).href}"`] + .filter(Boolean) + .join(' ') +} + +// `module.registerHooks` landed in Node 22.15, and `engines` only asks for +// `>=22`. Skipped rather than failed on an older runtime: the child would die +// in the preload with a TypeError that reads as a broken CLI. CI runs 22 and 24 +// at their latest patch, so this never skips there. +const hooksAvailable = typeof registerHooks === 'function' + +describe.skipIf(!hooksAvailable)('stash doctor — probe classification', () => { + it('reports an absent @cipherstash/stack as recoverable, not a failure', async () => { + // The `npx stash doctor` path in a project that has not run `stash init`. + // Node names the BASE package for a missing subpath of an absent package, + // which is why the probe imports `./diagnostics` but classifies against + // `@cipherstash/stack`. + const r = render(['doctor'], { + env: { + NODE_OPTIONS: unresolve({ + specifier: '@cipherstash/stack/diagnostics', + }), + }, + cols: 140, + }) + const { exitCode } = await r.exit + + expect(exitCode, r.output).toBe(0) + expect(r.output).toContain( + `${ENCRYPTION_LABEL} — ${messages.doctor.notInstalledOptional}`, + ) + expect(r.output).toContain(messages.doctor.allChecksPassed) + expect(r.output).not.toContain('Fatal error') + // The optional package's absence must not be dressed up as a missing + // binary — that would send the user to a reinstall for a package they + // simply have not installed yet. + expect(r.output).not.toContain(messages.doctor.nativeBinaryMissing) + }) + + it('warns, and does not claim every check passed, when a probe cannot run', async () => { + // An `@cipherstash/stack` older than the `./diagnostics` subpath. Nothing + // is known to be wrong with it — the install may be perfectly healthy — so + // this is not a failure and must not exit non-zero. It is also not a pass: + // the check did not run, and an outro saying every check passed would be + // the one line of output that is untrue. + const r = render(['doctor'], { + env: { + NODE_OPTIONS: unresolve({ + specifier: '@cipherstash/stack/diagnostics', + redirectTo: '@cipherstash/stack/no-such-subpath', + }), + }, + cols: 140, + }) + const { exitCode } = await r.exit + + expect(exitCode, r.output).toBe(0) + expect(r.output).toContain( + `${ENCRYPTION_LABEL} — ${messages.doctor.cannotProbe}`, + ) + expect(r.output).toContain(messages.doctor.checksIncomplete) + expect(r.output).not.toContain(messages.doctor.allChecksPassed) + // The arm exists to keep this error out of doctor's `else`, where it is + // rethrown and the launcher prints it raw. + expect(r.output).not.toContain('Fatal error') + }) + + it('fails on an absent required package', async () => { + // `@cipherstash/auth` is a hard dependency, so absence is a broken install + // — the row must go red and the run must exit non-zero. Non-optional is a + // separate branch from the case above and would otherwise be asserted + // nowhere. + const r = render(['doctor'], { + env: { NODE_OPTIONS: unresolve({ specifier: '@cipherstash/auth' }) }, + cols: 140, + }) + const { exitCode } = await r.exit + + expect(exitCode, r.output).toBe(1) + expect(r.output).toContain( + `${AUTH_LABEL} — ${messages.doctor.notInstalled}`, + ) + expect(r.output).toContain(messages.doctor.problemsFound) + expect(r.output).not.toContain('Fatal error') + }) +}) diff --git a/packages/stack/__tests__/diagnostics-entry.test.ts b/packages/stack/__tests__/diagnostics-entry.test.ts new file mode 100644 index 000000000..40379b04c --- /dev/null +++ b/packages/stack/__tests__/diagnostics-entry.test.ts @@ -0,0 +1,238 @@ +import { execFileSync } from 'node:child_process' +import { + existsSync, + mkdtempSync, + readdirSync, + readFileSync, + statSync, + writeFileSync, +} from 'node:fs' +import { tmpdir } from 'node:os' +import path from 'node:path' +import { describe, expect, it } from 'vitest' + +// `@cipherstash/stack/diagnostics` exists so `stash doctor` can prove the +// protect-ffi native binding is installed. Three properties make it work, and +// each one is invisible to a normal unit test: +// +// 1. It must not reach `@cipherstash/auth`. The root entry does — it +// re-exports the auth strategies — and that package's binding is EAGER +// (`module.exports = { ...native }`). A probe that lands on auth reports +// auth's binary under the encryption engine's label, which is the bug +// this entry was added to fix. Bundling is where that regresses: the main +// tsup config emits ESM with `splitting: true`, so a shared chunk is one +// import away. Hence an assertion on the ARTIFACT, not on the source. +// +// 2. Importing it must NOT force the load, and calling it must. That is the +// whole contract `doctor` classifies on, and it is a property of the +// emitted code — esbuild's CJS interop enumerates the required module to +// build its re-exports, and enumerating the wrong object is exactly what +// made the load eager before `b99cbd92`. +// +// 3. The loader's error must arrive unwrapped, so `MODULE_NOT_FOUND` and the +// platform package name survive for the CLI to key on. +// +// Mirrors `wasm-inline-bundle-isolation.test.ts`, which guards the same class +// of accident on the other native-free entry. + +const packageRoot = path.resolve(__dirname, '..') +const distDir = path.join(packageRoot, 'dist') +const srcDir = path.join(packageRoot, 'src') + +function newestMtime(dir: string): number { + let newest = 0 + const stack: string[] = [dir] + while (stack.length > 0) { + const current = stack.pop() as string + const stat = statSync(current) + if (stat.isDirectory()) { + for (const entry of readdirSync(current)) { + stack.push(path.join(current, entry)) + } + } else if (stat.mtimeMs > newest) { + newest = stat.mtimeMs + } + } + return newest +} + +// Same freshness gate as `cjs-require.test.ts`: assert against the CURRENT +// source, never a stale artifact that would pass for the wrong reason. +function distIsFresh(): boolean { + if (!existsSync(distDir)) return false + const tsupConfigMtime = statSync( + path.join(packageRoot, 'tsup.config.ts'), + ).mtimeMs + const distMtime = newestMtime(distDir) + return distMtime >= newestMtime(srcDir) && distMtime >= tsupConfigMtime +} + +if (!distIsFresh()) { + execFileSync('pnpm', ['run', 'build'], { + cwd: packageRoot, + stdio: 'inherit', + }) +} + +const ESM_BUNDLE = path.join(distDir, 'diagnostics.js') +const CJS_BUNDLE = path.join(distDir, 'diagnostics.cjs') + +/** Every bare specifier the bundle imports or requires. */ +function specifiersOf(bundlePath: string): string[] { + const source = readFileSync(bundlePath, 'utf8') + const found = new Set() + for (const [, spec] of source.matchAll( + /(?:\brequire\(\s*|\bfrom\s*|\bimport\s*\(\s*)['"]([^'"]+)['"]/g, + )) { + if (spec) found.add(spec) + } + return [...found] +} + +/** + * A CJS preload that makes every shape of the protect-ffi binding + * unresolvable — the platform packages and the local debug `index.node`, which + * is what `src/load.cts` reaches for. Patching the resolver rather than moving + * files keeps the checkout untouched and works whether or not the developer + * has run `build:native`. + * + * Adapted from `packages/protect-ffi/src/lintWiring.test.ts`, which runs its + * own suite under the same hook. `fs` is patched too: a loader that stats the + * artifact before requiring it would otherwise see a file that `require` then + * refuses to load. + */ +function hideBindingPreload(): string { + const dir = mkdtempSync(path.join(tmpdir(), 'stack-diagnostics-')) + const file = path.join(dir, 'hide-binding.cjs') + writeFileSync( + file, + ` +const fs = require('node:fs') +const Module = require('node:module') + +const BINDING = + /(?:^|[\\\\/])index\\.node$|@cipherstash[\\\\/]protect-ffi-(?:darwin|linux|win32)-/ + +const load = Module._load +Module._load = function (request, parent, isMain) { + if (BINDING.test(request)) { + const error = new Error("Cannot find module '" + request + "'") + error.code = 'MODULE_NOT_FOUND' + throw error + } + return load.call(this, request, parent, isMain) +} + +const existsSync = fs.existsSync +fs.existsSync = (p) => (BINDING.test(String(p)) ? false : existsSync(p)) + +const statSync = fs.statSync +fs.statSync = (p, ...rest) => { + if (!BINDING.test(String(p))) return statSync(p, ...rest) + const error = new Error('ENOENT: no such file or directory, stat ' + p) + error.code = 'ENOENT' + throw error +} + +Module.syncBuiltinESMExports() +`, + ) + return file +} + +/** + * Runs `script` in a child process resolved from this package, optionally with + * the binding hidden. A child rather than an in-process require: the point is + * to go through Node's own resolution of `@cipherstash/stack/diagnostics` + * against the published `exports` map, which Vitest's aliases would bypass. + */ +function runNode( + script: string, + { esm = false, hideBinding = false } = {}, +): string { + const args = esm ? ['--input-type=module', '-e', script] : ['-e', script] + return execFileSync(process.execPath, args, { + cwd: packageRoot, + encoding: 'utf8', + stdio: 'pipe', + env: hideBinding + ? { + ...process.env, + NODE_OPTIONS: [ + process.env.NODE_OPTIONS, + `--require "${hideBindingPreload()}"`, + ] + .filter(Boolean) + .join(' '), + } + : process.env, + }) +} + +describe('@cipherstash/stack/diagnostics', () => { + it('reaches protect-ffi and nothing else', () => { + for (const bundle of [ESM_BUNDLE, CJS_BUNDLE]) { + const specifiers = specifiersOf(bundle) + expect(specifiers, path.basename(bundle)).toContain( + '@cipherstash/protect-ffi', + ) + // Not `.not.toContain('@cipherstash/auth')` — the failure mode is a + // SHARED CHUNK that imports auth, whose own specifier is a relative + // `./chunk-XXXX.js`. Anything beyond the one bare specifier is the + // symptom, whatever its name. + expect(specifiers, path.basename(bundle)).toEqual([ + '@cipherstash/protect-ffi', + ]) + } + }) + + it('is importable from both module systems', () => { + expect( + runNode( + `const d = require('@cipherstash/stack/diagnostics') + if (typeof d.assertNativeBindingAvailable !== 'function') throw new Error('missing export') + process.stdout.write('ok')`, + ), + ).toBe('ok') + + expect( + runNode( + `const d = await import('@cipherstash/stack/diagnostics') + if (typeof d.assertNativeBindingAvailable !== 'function') throw new Error('missing export') + process.stdout.write('ok')`, + { esm: true }, + ), + ).toBe('ok') + }) + + // The reason the entry exists. If importing it were enough, `doctor` would + // not need this module at all — and if importing it FORCED the load, this + // package would have re-broken the laziness that `b99cbd92` bought. + it.each([ + ['require', false], + ['import', true], + ])('with no binding installed, %s succeeds and only the call throws', (_label, esm) => { + const script = esm + ? `const d = await import('@cipherstash/stack/diagnostics') + let raised = 'none' + try { d.assertNativeBindingAvailable() } catch (e) { raised = e.code + ' ' + e.message.split('\\n')[0] } + process.stdout.write('imported ' + raised)` + : `const d = require('@cipherstash/stack/diagnostics') + let raised = 'none' + try { d.assertNativeBindingAvailable() } catch (e) { raised = e.code + ' ' + e.message.split('\\n')[0] } + process.stdout.write('imported ' + raised)` + + const output = runNode(script, { esm, hideBinding: true }) + + // "imported" at all means the import did not throw — the property under + // test. A child that died during import produces no stdout and fails the + // execFileSync above. + expect(output).toMatch(/^imported /) + // Unwrapped: same `code` and the platform package by name, which is what + // `packages/cli/src/native.ts` classifies on. + expect(output).toContain('MODULE_NOT_FOUND') + expect(output).toMatch( + /Cannot find module '@cipherstash\/protect-ffi-(darwin|linux|win32)-/, + ) + }) +}) diff --git a/packages/stack/package.json b/packages/stack/package.json index 17de66bc0..4a5e0fd69 100644 --- a/packages/stack/package.json +++ b/packages/stack/package.json @@ -175,6 +175,16 @@ "default": "./dist/wasm-inline.js" } }, + "./diagnostics": { + "import": { + "types": "./dist/diagnostics.d.ts", + "default": "./dist/diagnostics.js" + }, + "require": { + "types": "./dist/diagnostics.d.cts", + "default": "./dist/diagnostics.cjs" + } + }, "./package.json": "./package.json" }, "scripts": { diff --git a/packages/stack/src/diagnostics.ts b/packages/stack/src/diagnostics.ts new file mode 100644 index 000000000..64f0a2eeb --- /dev/null +++ b/packages/stack/src/diagnostics.ts @@ -0,0 +1,27 @@ +/** + * Install diagnostics. One export, and it exists so `stash doctor` can prove + * the native binding is present. + * + * **Why a subpath and not the root entry.** Since the protect-ffi native load + * became lazy, importing that package proves nothing: `@neon-rs/load`'s proxy + * resolves the platform binary on first property access, inside a wrapper body, + * so a missing binary imports cleanly and fails later at the first encrypt. + * Nor can a caller force it from outside — `@cipherstash/protect-ffi/lib/load.cjs` + * is not in that package's `exports` (`ERR_PACKAGE_PATH_NOT_EXPORTED`), and + * touching an export never reaches the proxy because the exports are + * protect-ffi's own wrapper functions. `assertNativeBindingAvailable()` is the + * named operation that does reach it. + * + * **Why not `@cipherstash/stack` itself.** The root entry re-exports the auth + * strategies, which means evaluating it reads a property off + * `@cipherstash/auth` — a NAPI module whose entry is `module.exports = { + * ...native }`, eager on both counts. So a probe that imports the root entry + * measures AUTH's binary and reports it as the encryption engine's. This module + * imports protect-ffi and nothing else, so the signal belongs to the package + * named in the row. + * + * Kept pure deliberately: no side effects at module scope, and the loader's + * error is not caught, wrapped or re-thrown anywhere on the path — a caller + * classifying `MODULE_NOT_FOUND` sees exactly what the loader raised. + */ +export { assertNativeBindingAvailable } from '@cipherstash/protect-ffi' diff --git a/packages/stack/tsup.config.ts b/packages/stack/tsup.config.ts index a96c010c4..c00987893 100644 --- a/packages/stack/tsup.config.ts +++ b/packages/stack/tsup.config.ts @@ -1,9 +1,9 @@ import { defineConfig } from 'tsup' -// Two configs run in parallel inside tsup. They share the same `dist/` -// output dir, so neither uses `clean: true` — a parallel-run race could -// otherwise wipe the other config's output. The pre-tsup `rimraf dist` -// in `package.json`'s build script clears the dir once before either +// Three configs run in parallel inside tsup. They share the same `dist/` +// output dir, so none uses `clean: true` — a parallel-run race could +// otherwise wipe another config's output. The pre-tsup `rimraf dist` +// in `package.json`'s build script clears the dir once before any // starts. export default defineConfig([ // Main entries — dual ESM + CJS bundles. @@ -49,4 +49,25 @@ export default defineConfig([ tsconfig: './tsconfig.json', noExternal: ['evlog', 'uuid', 'zod', '@byteslice/result'], }, + // Diagnostics entry — its own config rather than another `entry` in the + // main one, because what it must NOT reach is the whole point of it. + // `splitting` is on for ESM in the main config, and a shared chunk is + // exactly how `@cipherstash/auth` would arrive here: the probe would then + // fail on auth's binary while reporting the encryption engine, which is the + // bug this entry exists to fix. `splitting: false` makes that structural + // rather than a property of today's module graph. + // + // Nothing is bundled in (no `noExternal`): `@cipherstash/protect-ffi` has to + // stay a bare specifier so the load goes through the installed package's own + // loader, which is the thing under test. + { + entry: { diagnostics: 'src/diagnostics.ts' }, + format: ['cjs', 'esm'], + splitting: false, + sourcemap: true, + dts: { entry: { diagnostics: 'src/diagnostics.ts' } }, + clean: false, + target: 'es2022', + tsconfig: './tsconfig.json', + }, ]) From b719dc0e04ead77442ef13a845d74492cef331bf Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 13 Aug 2026 10:41:47 +1000 Subject: [PATCH 2/4] fix(doctor): probe through a published export, and scope the stale-subpath arm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both real. `@cipherstash/stack/diagnostics` re-exported protect-ffi's `assertNativeBindingAvailable`, which is in no released version of that package: it arrived with the lazy native load, whose changeset is parked `.deferred` until the publishing cutover. `@cipherstash/stack` depends on `workspace:*`, so it built against the sibling directory here and would have shipped against 0.31.0 on npm — where the export does not exist. Verified against the published tarball: under ESM the entry dies at import with a link-time SyntaxError, under CJS it hands back `undefined`. `doctor` classifies neither, so every user on a released install would have got a bare `Fatal error` from the command whose job is to explain those. It now calls `isEncrypted(null)`, published since 0.28.0 and the same call protect-ffi's own assert makes, for the same reason: pure, synchronous, and no validation before the addon. A stand-in carrying the published surface covers it; the existing tests still cover the real package and a real binding. The too-old-to-probe arm keyed on `ERR_PACKAGE_PATH_NOT_EXPORTED` alone, and every probe's failure was offered to it — including auth's, which asks for no subpath. Any exports failure in a probe's import graph was answered with "upgrade @cipherstash/stack" and an exit code of 0. A probe now declares the subpath it imports, and the arm requires the error to name that subpath of that package. The e2e fixture moves the IMPORTER to a directory this test builds — absent package, or an older one whose `exports` predates the subpath — rather than rewriting the specifier. Redirecting produced the right error code against the wrong subpath, a shape the real failure never has, which is precisely what the tightened classifier rejects. Also fixes a fixture that would have gone on lying: a CJS stand-in reached through macOS's symlinked tmpdir loads twice, and the named exports bind against the instance that never ran — indistinguishable from the bug under test. --- .changeset/stack-diagnostics-subpath.md | 4 +- .../doctor-probe-classification.test.ts | 41 +++++- packages/cli/src/commands/doctor/index.ts | 38 +++++- .../doctor-probe-classification.e2e.test.ts | 102 +++++++++++---- .../stack/__tests__/diagnostics-entry.test.ts | 119 +++++++++++++++--- packages/stack/src/diagnostics.ts | 22 +++- 6 files changed, 271 insertions(+), 55 deletions(-) diff --git a/.changeset/stack-diagnostics-subpath.md b/.changeset/stack-diagnostics-subpath.md index e23a0f0c3..cb949f29d 100644 --- a/.changeset/stack-diagnostics-subpath.md +++ b/.changeset/stack-diagnostics-subpath.md @@ -4,8 +4,10 @@ Add a `@cipherstash/stack/diagnostics` subpath, for tooling that needs to prove the protect-ffi native binding is installed. -It exports one function, `assertNativeBindingAvailable()`, re-exported from `@cipherstash/protect-ffi`. Calling it forces the platform binary to load and throws the loader's own `MODULE_NOT_FOUND` — unwrapped, naming the missing `@cipherstash/protect-ffi--` package — if it is absent. Importing the subpath does not force anything, so the laziness that makes the native load cost nothing for callers that never encrypt is preserved. +It exports one function, `assertNativeBindingAvailable()`. Calling it forces the platform binary to load and throws the loader's own `MODULE_NOT_FOUND` — unwrapped, naming the missing `@cipherstash/protect-ffi--` package — if it is absent. Importing the subpath does not force anything, so the laziness that makes the native load cost nothing for callers that never encrypt is preserved. The subpath exists because there is no way to do this from outside: the package's loader is not in its `exports` map, and reading an export never reaches the `@neon-rs/load` proxy. Importing `@cipherstash/stack` itself is not a substitute either — the root entry re-exports the auth strategies, so evaluating it resolves `@cipherstash/auth`'s binding instead. This entry reaches protect-ffi and nothing else. +It probes by calling `isEncrypted`, which has been published since 0.28.0, rather than protect-ffi's own `assertNativeBindingAvailable` — that export arrived with the lazy native load and is not in any released version, so re-exporting it would build here and fail wherever it shipped (a link-time error under ESM, an `undefined` under CJS). + Available as both `import` and `require`. diff --git a/packages/cli/src/__tests__/doctor-probe-classification.test.ts b/packages/cli/src/__tests__/doctor-probe-classification.test.ts index f3e8974d4..3a292fe80 100644 --- a/packages/cli/src/__tests__/doctor-probe-classification.test.ts +++ b/packages/cli/src/__tests__/doctor-probe-classification.test.ts @@ -35,6 +35,11 @@ function resolutionError(specifier: string): unknown { // error reaches doctor's `else` and is rethrown, surfacing as the launcher's // bare `Fatal error` for an install that may be perfectly healthy. describe('isSubpathUnavailable', () => { + // The two probes as `doctor` declares them: one imports a subpath, one does + // not. + const encryption = { pkg: '@cipherstash/stack', subpath: './no-such-subpath' } + const auth = { pkg: '@cipherstash/auth' } + it('matches an installed package that does not publish the subpath', () => { // A real package (so resolution gets as far as reading its exports map) // with a subpath it has never had. @@ -43,7 +48,30 @@ describe('isSubpathUnavailable', () => { expect((err as { code?: string }).code).toBe( 'ERR_PACKAGE_PATH_NOT_EXPORTED', ) - expect(isSubpathUnavailable(err)).toBe(true) + expect(isSubpathUnavailable(err, encryption)).toBe(true) + }) + + it('does not match a probe that imports no subpath', () => { + // The advice this arm renders names `@cipherstash/stack` and tells the user + // to upgrade it. Applied to the auth probe — which imports the package root + // — any exports failure raised somewhere inside auth's own dependency graph + // would answer a broken install with an unrelated upgrade and exit 0. + const err = resolutionError('@cipherstash/stack/no-such-subpath') + + expect(isSubpathUnavailable(err, auth)).toBe(false) + }) + + it('does not match a failure on a different subpath of the same package', () => { + // A dependency deeper in the probe's own import graph with an exports + // problem of its own is not "your @cipherstash/stack is too old". + const err = resolutionError('@cipherstash/stack/no-such-subpath') + + expect( + isSubpathUnavailable(err, { + pkg: '@cipherstash/stack', + subpath: './diagnostics', + }), + ).toBe(false) }) it('does not match the errors the other arms own', () => { @@ -52,20 +80,23 @@ describe('isSubpathUnavailable', () => { // subpath, and why this must not be mistaken for a stale install. const missing = resolutionError('@cipherstash/no-such-package/diagnostics') expect((missing as { code?: string }).code).toBe('MODULE_NOT_FOUND') - expect(isSubpathUnavailable(missing)).toBe(false) + expect(isSubpathUnavailable(missing, encryption)).toBe(false) const binary = new Error( "Cannot find module '@cipherstash/protect-ffi-darwin-arm64'", ) as Error & { code?: string } binary.code = 'MODULE_NOT_FOUND' - expect(isSubpathUnavailable(binary)).toBe(false) + expect(isSubpathUnavailable(binary, encryption)).toBe(false) expect(isNativeBinaryMissing(binary)).toBe(true) }) it('ignores non-Error values', () => { - expect(isSubpathUnavailable(undefined)).toBe(false) + expect(isSubpathUnavailable(undefined, encryption)).toBe(false) expect( - isSubpathUnavailable({ code: 'ERR_PACKAGE_PATH_NOT_EXPORTED' }), + isSubpathUnavailable( + { code: 'ERR_PACKAGE_PATH_NOT_EXPORTED' }, + encryption, + ), ).toBe(false) }) }) diff --git a/packages/cli/src/commands/doctor/index.ts b/packages/cli/src/commands/doctor/index.ts index f5a5c83c6..56d31a14c 100644 --- a/packages/cli/src/commands/doctor/index.ts +++ b/packages/cli/src/commands/doctor/index.ts @@ -27,6 +27,12 @@ interface Probe { * classifies against `@cipherstash/stack`. */ pkg: string + /** + * The subpath of `pkg` that `force()` imports, when it imports one. Only a + * probe that asks for a subpath can fail for want of it, so this is what + * scopes the too-old-to-probe arm to the probe it has advice for. + */ + subpath?: string /** May legitimately be absent until `stash init`, so absence is not failure. */ optional?: boolean force(): Promise @@ -36,6 +42,7 @@ const PROBES: Probe[] = [ { label: 'Encryption engine (@cipherstash/stack → protect-ffi)', pkg: '@cipherstash/stack', + subpath: './diagnostics', optional: true, async force() { // `@cipherstash/stack/diagnostics` exists for this call: it reaches @@ -91,18 +98,39 @@ export function isPackageMissing(err: unknown, pkg: string): boolean { } /** - * True when the package is installed but does not publish the subpath the probe - * asked for — an `@cipherstash/stack` older than `./diagnostics`. + * True when the package is installed but does not publish the subpath THIS + * probe asked for — an `@cipherstash/stack` older than `./diagnostics`. * * Its own arm because the alternative is the `else` below: an unclassified * error, rethrown, surfacing as a bare `Fatal error` from the launcher. `stash` * declares `@cipherstash/stack` as an OPTIONAL PEER with a wide range, so every * install predating that subpath lands here — a case a user hits by doing * nothing wrong. + * + * Narrow on purpose. The row it renders tells the user to upgrade + * `@cipherstash/stack`, and the error code alone does not mean that: an exports + * failure can come from anywhere in a probe's import graph, including the auth + * probe, which asks for no subpath at all. So the probe must name a subpath and + * the error must be about that one — otherwise a broken install gets an + * unrelated upgrade and an exit code of 0. */ -export function isSubpathUnavailable(err: unknown): boolean { +export function isSubpathUnavailable( + err: unknown, + probe: { pkg: string; subpath?: string }, +): boolean { + if (probe.subpath === undefined) return false if (!(err instanceof Error)) return false - return (err as { code?: string }).code === 'ERR_PACKAGE_PATH_NOT_EXPORTED' + if ((err as { code?: string }).code !== 'ERR_PACKAGE_PATH_NOT_EXPORTED') { + return false + } + // Node names both halves: `Package subpath './diagnostics' is not defined by + // "exports" in /…/@cipherstash/stack/package.json`. Separators normalised + // because win32 is a supported target and that path is built by the OS. + const message = err.message.replaceAll('\\', '/') + return ( + message.includes(`'${probe.subpath}'`) && + message.includes(`${probe.pkg}/package.json`) + ) } export async function doctorCommand(): Promise { @@ -150,7 +178,7 @@ export async function doctorCommand(): Promise { : messages.doctor.notInstalled, ) if (!probe.optional) failed = true - } else if (isSubpathUnavailable(err)) { + } else if (isSubpathUnavailable(err, probe)) { report('warn', probe.label, messages.doctor.cannotProbe) incomplete = true } else { diff --git a/packages/cli/tests/e2e/doctor-probe-classification.e2e.test.ts b/packages/cli/tests/e2e/doctor-probe-classification.e2e.test.ts index 9385a3ea9..1650f7dd8 100644 --- a/packages/cli/tests/e2e/doctor-probe-classification.e2e.test.ts +++ b/packages/cli/tests/e2e/doctor-probe-classification.e2e.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, writeFileSync } from 'node:fs' +import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs' import { registerHooks } from 'node:module' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -9,7 +9,7 @@ import { render } from '../helpers/pty.js' // `doctor` sorts a probe failure into four arms, and only one of them — the // missing platform binary — had coverage (`doctor-missing-binary.e2e.test.ts`). -// These are the two a user reaches by doing nothing wrong: +// These are the ones a user reaches by doing nothing wrong: // // * `@cipherstash/stack` absent. It is an OPTIONAL PEER, so `npx stash // doctor` in a project that has not run `stash init` lands here. Must read @@ -18,12 +18,13 @@ import { render } from '../helpers/pty.js' // subpath — every version inside the `>=1.0.0-rc.0` peer range that the CLI // itself permits. Unclassified, this reaches doctor's `else`, is rethrown, // and surfaces as the launcher's bare `Fatal error`. +// * The same error code from a probe that asks for no subpath, which cannot +// mean "upgrade `@cipherstash/stack`" and must not be answered with it. // -// Both errors are raised by NODE'S OWN resolver, never hand-built: a -// `module.registerHooks` resolve hook re-points the specifier and lets -// resolution fail on its own terms. A fixture that pasted the message and code -// on by hand would keep passing if Node ever changed either, which is the whole -// risk being covered here. +// Every error here is raised by NODE'S OWN resolver against a package layout +// this file builds — an absent package, an older one — never hand-built with +// the code and message pasted on. A fixture like that keeps passing when Node +// changes either, which is the whole risk being covered. /** Mirrors the probe labels in `src/commands/doctor/index.ts`. */ const ENCRYPTION_LABEL = 'Encryption engine (@cipherstash/stack → protect-ffi)' @@ -33,44 +34,60 @@ interface Unresolve { /** The bare specifier to divert. */ specifier: string /** - * Resolve this instead — a subpath the package really does not publish, so - * Node raises its own `ERR_PACKAGE_PATH_NOT_EXPORTED`. Omit to keep the - * specifier and move the IMPORTER to an empty directory instead, which is how - * an absent package is staged: same specifier, no `node_modules` chain to - * find it in, so Node raises `ERR_MODULE_NOT_FOUND` naming the base package. + * A package to install into the directory the specifier is re-resolved from, + * carrying an `exports` map that does not answer what the probe asks for. + * Omit and the directory stays empty, which is how an ABSENT package is + * staged — no `node_modules` chain to find it in, so Node raises + * `ERR_MODULE_NOT_FOUND` naming the base package. */ - redirectTo?: string + installed?: { pkg: string; exports: Record } } /** - * Writes a hook module that makes `specifier` unresolvable in the spawned CLI, - * and returns the `NODE_OPTIONS` value that loads it. + * Writes a hook module that re-resolves `specifier` against a directory this + * test controls, and returns the `NODE_OPTIONS` value that loads it. + * + * Moving the IMPORTER rather than rewriting the specifier: the specifier the + * CLI asks for has to reach Node's resolver unchanged, because what the + * classifier keys on is which subpath of which package the error names. A + * redirect to some other subpath produces the right error CODE against the + * wrong subpath, which is a shape the real failure never has. * * A resolve hook rather than the `Module._load` patch its sibling suite uses: * the probe is an `await import()` of a real ESM package, so it never reaches * the CJS loader. `registerHooks` is synchronous and in-process, so the error * propagates out of the dynamic import exactly as an unhooked failure would. */ -function unresolve({ specifier, redirectTo }: Unresolve): string { +function unresolve({ specifier, installed }: Unresolve): string { const dir = mkdtempSync(join(tmpdir(), 'stash-doctor-probe-')) + if (installed) { + const pkgDir = join(dir, 'node_modules', ...installed.pkg.split('/')) + mkdirSync(pkgDir, { recursive: true }) + writeFileSync( + join(pkgDir, 'package.json'), + JSON.stringify({ + name: installed.pkg, + version: '0.0.0', + exports: installed.exports, + }), + ) + } + // The importer is never written — resolution fails before anything is read. + // It only has to sit in this directory, so that what is (or is not) in the + // `node_modules` beside it is what Node resolves against. + const parent = pathToFileURL(join(dir, 'importer.mjs')).href const hook = join(dir, 'unresolve.mjs') - // Importer inside the fresh temp dir. It is never written — resolution fails - // before anything is read — it only has to sit somewhere with no - // `node_modules` above it holding the package. - const emptyParent = pathToFileURL(join(dir, 'importer.mjs')).href writeFileSync( hook, `import { registerHooks } from 'node:module' const SPECIFIER = ${JSON.stringify(specifier)} -const REDIRECT = ${JSON.stringify(redirectTo ?? null)} -const EMPTY_PARENT = ${JSON.stringify(emptyParent)} +const PARENT = ${JSON.stringify(parent)} registerHooks({ resolve(specifier, context, nextResolve) { if (specifier !== SPECIFIER) return nextResolve(specifier, context) - if (REDIRECT) return nextResolve(REDIRECT, context) - return nextResolve(specifier, { ...context, parentURL: EMPTY_PARENT }) + return nextResolve(specifier, { ...context, parentURL: PARENT }) }, }) `, @@ -126,7 +143,12 @@ describe.skipIf(!hooksAvailable)('stash doctor — probe classification', () => env: { NODE_OPTIONS: unresolve({ specifier: '@cipherstash/stack/diagnostics', - redirectTo: '@cipherstash/stack/no-such-subpath', + // An `@cipherstash/stack` from before the subpath existed: present, + // resolvable, and its `exports` answers `.` and nothing else. + installed: { + pkg: '@cipherstash/stack', + exports: { '.': './dist/index.js' }, + }, }), }, cols: 140, @@ -144,6 +166,36 @@ describe.skipIf(!hooksAvailable)('stash doctor — probe classification', () => expect(r.output).not.toContain('Fatal error') }) + it('does not answer an exports failure elsewhere with stack-upgrade advice', async () => { + // The too-old-to-probe arm keys on an error code, and every probe's failure + // is offered to it. `ERR_PACKAGE_PATH_NOT_EXPORTED` can come from anywhere + // in a probe's import graph — a dependency with an exports problem of its + // own — and on the auth probe, which imports no subpath at all, it cannot + // mean "your @cipherstash/stack is too old". Answering a broken install + // with an unrelated upgrade and exiting 0 is a worse outcome than not + // classifying it. + const r = render(['doctor'], { + env: { + NODE_OPTIONS: unresolve({ + specifier: '@cipherstash/auth', + // An auth package whose `exports` does not answer its own root — + // same error code as the case above, from a package the arm has no + // advice for. + installed: { + pkg: '@cipherstash/auth', + exports: { './sub': './sub.js' }, + }, + }), + }, + cols: 140, + }) + const { exitCode } = await r.exit + + expect(r.output).not.toContain(messages.doctor.cannotProbe) + expect(r.output).not.toContain(messages.doctor.allChecksPassed) + expect(exitCode, r.output).toBe(1) + }) + it('fails on an absent required package', async () => { // `@cipherstash/auth` is a hard dependency, so absence is a broken install // — the row must go red and the run must exit non-zero. Non-optional is a diff --git a/packages/stack/__tests__/diagnostics-entry.test.ts b/packages/stack/__tests__/diagnostics-entry.test.ts index 40379b04c..b9d5886c7 100644 --- a/packages/stack/__tests__/diagnostics-entry.test.ts +++ b/packages/stack/__tests__/diagnostics-entry.test.ts @@ -4,11 +4,13 @@ import { mkdtempSync, readdirSync, readFileSync, + realpathSync, statSync, writeFileSync, } from 'node:fs' import { tmpdir } from 'node:os' import path from 'node:path' +import { pathToFileURL } from 'node:url' import { describe, expect, it } from 'vitest' // `@cipherstash/stack/diagnostics` exists so `stash doctor` can prove the @@ -140,32 +142,87 @@ Module.syncBuiltinESMExports() return file } +/** Thrown by the stand-in's `isEncrypted`, so the call is observable. */ +const PUBLISHED_SURFACE_MARKER = 'reached the published protect-ffi surface' + +/** + * A preload that swaps `@cipherstash/protect-ffi` for a stand-in carrying the + * surface of the version on npm — `isEncrypted`, and no + * `assertNativeBindingAvailable`. + * + * This entry is the one part of the package whose correctness depends on WHICH + * protect-ffi is installed, and `workspace:*` hides that: it resolves to the + * sibling directory here and to whatever version publishing pins there. The + * stand-in's `isEncrypted` throws a marker instead of loading a binding, so the + * assertion is that the call reaches it at all — that the entry probes through + * an export the published package actually has. The tests above cover the other + * half, against the real protect-ffi and a real binding. + */ +function publishedFfiSurfacePreload(): string { + // Realpath'd, and it is load-bearing. `os.tmpdir()` is behind a symlink on + // macOS (`/var` → `/private/var`), and a CJS module reached through one is + // loaded TWICE: the ESM loader keys the module on the symlinked URL it was + // given, while executing a `.cjs` goes through the CJS loader, which + // realpaths. The stand-in's named exports then bind against the instance + // that never ran, so `isEncrypted` arrives `undefined` — a failure that + // looks exactly like the bug under test and is not one. + const dir = realpathSync( + mkdtempSync(path.join(tmpdir(), 'stack-diagnostics-ffi-')), + ) + const standIn = path.join(dir, 'protect-ffi-as-published.cjs') + writeFileSync( + standIn, + `exports.isEncrypted = function isEncrypted() { + throw new Error('${PUBLISHED_SURFACE_MARKER}') +} +exports.encrypt = function encrypt() {} +`, + ) + const hook = path.join(dir, 'substitute-ffi.mjs') + writeFileSync( + hook, + `import { registerHooks } from 'node:module' +import { pathToFileURL } from 'node:url' + +const STAND_IN = pathToFileURL(${JSON.stringify(standIn)}).href + +registerHooks({ + resolve(specifier, context, nextResolve) { + if (specifier === '@cipherstash/protect-ffi') { + return { url: STAND_IN, shortCircuit: true } + } + return nextResolve(specifier, context) + }, +}) +`, + ) + return `--import "${pathToFileURL(hook).href}"` +} + /** - * Runs `script` in a child process resolved from this package, optionally with - * the binding hidden. A child rather than an in-process require: the point is - * to go through Node's own resolution of `@cipherstash/stack/diagnostics` - * against the published `exports` map, which Vitest's aliases would bypass. + * Runs `script` in a child process resolved from this package. A child rather + * than an in-process require: the point is to go through Node's own resolution + * of `@cipherstash/stack/diagnostics` against the published `exports` map, + * which Vitest's aliases would bypass. */ function runNode( script: string, - { esm = false, hideBinding = false } = {}, + { esm = false, hideBinding = false, publishedFfi = false } = {}, ): string { const args = esm ? ['--input-type=module', '-e', script] : ['-e', script] + const preloads = [ + process.env.NODE_OPTIONS, + hideBinding ? `--require "${hideBindingPreload()}"` : undefined, + publishedFfi ? publishedFfiSurfacePreload() : undefined, + ].filter(Boolean) return execFileSync(process.execPath, args, { cwd: packageRoot, encoding: 'utf8', stdio: 'pipe', - env: hideBinding - ? { - ...process.env, - NODE_OPTIONS: [ - process.env.NODE_OPTIONS, - `--require "${hideBindingPreload()}"`, - ] - .filter(Boolean) - .join(' '), - } - : process.env, + env: + preloads.length > 0 + ? { ...process.env, NODE_OPTIONS: preloads.join(' ') } + : process.env, }) } @@ -186,6 +243,36 @@ describe('@cipherstash/stack/diagnostics', () => { } }) + // `@cipherstash/protect-ffi` is a `workspace:*` dependency, so this entry is + // built against the sibling directory and shipped against whatever version + // publishing pins — today 0.31.0, which is on npm WITHOUT + // `assertNativeBindingAvailable`: that export arrived with the lazy load, + // whose changeset is parked as `.deferred` until the publishing cutover, so + // no version carrying it exists. Re-exporting it would ship an entry that + // dies on import under ESM ("Named export … not found", a link-time + // SyntaxError) and hands back `undefined` under CJS — and `stash doctor` + // classifies neither, so it would print a bare `Fatal error` for every user + // on a released install. + it.each([ + ['require', false], + ['import', true], + ])('probes through an export the published protect-ffi has, so %s survives it', (_label, esm) => { + const call = `let raised = 'none' + try { d.assertNativeBindingAvailable() } catch (e) { raised = e.message } + process.stdout.write('imported ' + raised)` + const script = esm + ? `const d = await import('@cipherstash/stack/diagnostics')\n${call}` + : `const d = require('@cipherstash/stack/diagnostics')\n${call}` + + const output = runNode(script, { esm, publishedFfi: true }) + + // Reaching stdout at all means the entry loaded against that surface. + expect(output).toMatch(/^imported /) + // And that the probe ran, rather than resolving to `undefined` and + // throwing a TypeError the CLI would report as an unknown failure. + expect(output).toContain(PUBLISHED_SURFACE_MARKER) + }) + it('is importable from both module systems', () => { expect( runNode( diff --git a/packages/stack/src/diagnostics.ts b/packages/stack/src/diagnostics.ts index 64f0a2eeb..fd1bec92f 100644 --- a/packages/stack/src/diagnostics.ts +++ b/packages/stack/src/diagnostics.ts @@ -1,3 +1,5 @@ +import { isEncrypted } from '@cipherstash/protect-ffi' + /** * Install diagnostics. One export, and it exists so `stash doctor` can prove * the native binding is present. @@ -9,8 +11,7 @@ * Nor can a caller force it from outside — `@cipherstash/protect-ffi/lib/load.cjs` * is not in that package's `exports` (`ERR_PACKAGE_PATH_NOT_EXPORTED`), and * touching an export never reaches the proxy because the exports are - * protect-ffi's own wrapper functions. `assertNativeBindingAvailable()` is the - * named operation that does reach it. + * protect-ffi's own wrapper functions. CALLING one is what reaches it. * * **Why not `@cipherstash/stack` itself.** The root entry re-exports the auth * strategies, which means evaluating it reads a property off @@ -20,8 +21,23 @@ * imports protect-ffi and nothing else, so the signal belongs to the package * named in the row. * + * **Why `isEncrypted` and not protect-ffi's own `assertNativeBindingAvailable`, + * which is this function's twin.** That export does not exist in any version of + * protect-ffi on npm. It arrived with the lazy load, whose changeset is parked + * as `.deferred` until the publishing cutover, so no released version carries + * it — and `@cipherstash/stack` depends on `workspace:*`, which resolves to the + * sibling directory here and to a published version for everyone else. A + * re-export would therefore work in this repo and fail everywhere it shipped: + * under ESM at import, with a link-time SyntaxError, and under CJS as an + * `undefined` that is not a function. `isEncrypted` has been published since + * 0.28.0 and is the call protect-ffi's own assert makes, for the same reason — + * it is pure, synchronous, and validates nothing before reaching the addon, so + * whatever it raises came from the loader. + * * Kept pure deliberately: no side effects at module scope, and the loader's * error is not caught, wrapped or re-thrown anywhere on the path — a caller * classifying `MODULE_NOT_FOUND` sees exactly what the loader raised. */ -export { assertNativeBindingAvailable } from '@cipherstash/protect-ffi' +export function assertNativeBindingAvailable(): void { + isEncrypted(null) +} From 12d1c0d207e85039f579b9aebd15b7e2329db22a Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 13 Aug 2026 11:12:40 +1000 Subject: [PATCH 3/4] fix(stack): declare diagnostics types for node10 resolution, and document it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `typesVersions` and `exports` describe the same subpaths to two different resolvers, and only one of them is exercised here. `./diagnostics` was in `exports` and not in `typesVersions`, so a consumer on classic node10 resolution — still the default when `module` is `commonjs` — resolved it to `any` with no error at the import site. Nothing type-checks that: the package builds, publishes and installs clean. Guarded now rather than just fixed. The parity test asserts every exported subpath has a `typesVersions` entry pointing at the same declaration file, so the next subpath fails here instead of shipping half-declared. It is keyed on what `exports` publishes, which leaves the reverse drift alone — `secrets` has an entry for a subpath that no longer exists, dead weight rather than a consumer-visible gap, and not this branch's to remove. `skills/stash-encryption` carries the canonical subpath table and ships inside the `stash` tarball. `adapter-kit` is listed there with a "do not import in application code" caveat, so a tooling-only entry belongs in it; `./diagnostics` is documented the same way. The root AGENTS.md list was updated when the subpath landed, the skill was not — the map in AGENTS.md points `packages/stack` subpath exports at this skill precisely so that does not happen. Also from review: * `isPackageMissing` and `isSubpathUnavailable` were exported from `commands/doctor` solely so a test could reach them, against this repo's rule about testing through the public API. They are module-resolution error classifiers, so they now live in `module-error.ts` next to `isModuleNotFound` and `moduleNotFoundSpecifier` — imported by doctor, tested where they are defined, and no wider a surface than before. * Counting failing rows by splitting on the message rather than `new RegExp(message)`. The needle is copy from `messages.ts`, and copy is free to grow a `.` or `(` that a regex would reinterpret instead of failing on. --- ...ts => module-error-classification.test.ts} | 13 ++-- packages/cli/src/commands/doctor/index.ts | 63 +------------------ packages/cli/src/module-error.ts | 59 ++++++++++++++++- .../e2e/doctor-missing-binary.e2e.test.ts | 13 ++-- .../__tests__/subpath-types-parity.test.ts | 42 +++++++++++++ packages/stack/package.json | 3 + skills/stash-encryption/SKILL.md | 1 + 7 files changed, 117 insertions(+), 77 deletions(-) rename packages/cli/src/__tests__/{doctor-probe-classification.test.ts => module-error-classification.test.ts} (93%) create mode 100644 packages/stack/__tests__/subpath-types-parity.test.ts diff --git a/packages/cli/src/__tests__/doctor-probe-classification.test.ts b/packages/cli/src/__tests__/module-error-classification.test.ts similarity index 93% rename from packages/cli/src/__tests__/doctor-probe-classification.test.ts rename to packages/cli/src/__tests__/module-error-classification.test.ts index 3a292fe80..78ece773d 100644 --- a/packages/cli/src/__tests__/doctor-probe-classification.test.ts +++ b/packages/cli/src/__tests__/module-error-classification.test.ts @@ -2,15 +2,14 @@ import { createRequire } from 'node:module' import path from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' -import { - isPackageMissing, - isSubpathUnavailable, -} from '../commands/doctor/index.js' +import { isPackageMissing, isSubpathUnavailable } from '../module-error.js' import { isNativeBinaryMissing } from '../native.js' -// The classifiers `stash doctor` sorts a failed probe with. Each arm renders a -// different row and a different exit code, so a probe error landing in the -// wrong one is a wrong diagnosis, not a cosmetic slip. +// The `module-error.ts` classifiers `stash doctor` sorts a failed probe with. +// Each arm renders a different row and a different exit code, so a probe error +// landing in the wrong one is a wrong diagnosis, not a cosmetic slip — and the +// two here are the ones whose answers are indistinguishable to the user: a +// green "not installed", or advice to upgrade. // // Every fixture below is an error NODE raised, never one built by hand with the // code and message pasted on. A hand-built fixture asserts on itself: it keeps diff --git a/packages/cli/src/commands/doctor/index.ts b/packages/cli/src/commands/doctor/index.ts index 56d31a14c..008b09b50 100644 --- a/packages/cli/src/commands/doctor/index.ts +++ b/packages/cli/src/commands/doctor/index.ts @@ -1,9 +1,6 @@ import * as p from '@clack/prompts' import { messages } from '../../messages.js' -import { - isModuleNotFound, - moduleNotFoundSpecifier, -} from '../../module-error.js' +import { isPackageMissing, isSubpathUnavailable } from '../../module-error.js' import { currentTarget, isNativeBinaryMissing, @@ -75,64 +72,6 @@ function report(outcome: Outcome, label: string, detail?: string) { else p.log.error(text) } -/** - * True when `pkg` itself is what failed to resolve — the package is not - * installed, as opposed to installed and unable to load something. - * - * Matched on the SPECIFIER Node quotes, not on the message text. A substring - * test reads the probe's own import path as a hit: `@cipherstash/stack/ - * diagnostics` failing on a missing `dist/` file names - * `…/node_modules/@cipherstash/stack/dist/diagnostics.js`, which contains the - * package name, and doctor would call an installed-but-broken package "not - * installed" — a green row, and no reason for the user to look further. It also - * swallowed the neighbours: `@cipherstash/stack-drizzle` contains - * `@cipherstash/stack`. - */ -export function isPackageMissing(err: unknown, pkg: string): boolean { - if (!isModuleNotFound(err)) return false - const specifier = moduleNotFoundSpecifier(err) - if (specifier === undefined) return false - // The probe imports a subpath, and Node quotes the base package for an absent - // one under ESM but the full specifier under CJS. Both mean this package. - return specifier === pkg || specifier.startsWith(`${pkg}/`) -} - -/** - * True when the package is installed but does not publish the subpath THIS - * probe asked for — an `@cipherstash/stack` older than `./diagnostics`. - * - * Its own arm because the alternative is the `else` below: an unclassified - * error, rethrown, surfacing as a bare `Fatal error` from the launcher. `stash` - * declares `@cipherstash/stack` as an OPTIONAL PEER with a wide range, so every - * install predating that subpath lands here — a case a user hits by doing - * nothing wrong. - * - * Narrow on purpose. The row it renders tells the user to upgrade - * `@cipherstash/stack`, and the error code alone does not mean that: an exports - * failure can come from anywhere in a probe's import graph, including the auth - * probe, which asks for no subpath at all. So the probe must name a subpath and - * the error must be about that one — otherwise a broken install gets an - * unrelated upgrade and an exit code of 0. - */ -export function isSubpathUnavailable( - err: unknown, - probe: { pkg: string; subpath?: string }, -): boolean { - if (probe.subpath === undefined) return false - if (!(err instanceof Error)) return false - if ((err as { code?: string }).code !== 'ERR_PACKAGE_PATH_NOT_EXPORTED') { - return false - } - // Node names both halves: `Package subpath './diagnostics' is not defined by - // "exports" in /…/@cipherstash/stack/package.json`. Separators normalised - // because win32 is a supported target and that path is built by the OS. - const message = err.message.replaceAll('\\', '/') - return ( - message.includes(`'${probe.subpath}'`) && - message.includes(`${probe.pkg}/package.json`) - ) -} - export async function doctorCommand(): Promise { p.intro(messages.doctor.title) diff --git a/packages/cli/src/module-error.ts b/packages/cli/src/module-error.ts index 4f5f06c98..524ec669c 100644 --- a/packages/cli/src/module-error.ts +++ b/packages/cli/src/module-error.ts @@ -3,7 +3,8 @@ // import) whose message names the unresolved specifier. These turn that raw // error into structured data callers translate into guidance — missing native // binaries in `native.ts`, missing CipherStash packages in -// `config/missing-package.ts`. +// `config/missing-package.ts`, and which of those a failed `stash doctor` probe +// hit in `commands/doctor`. /** A Node module-resolution error. */ export interface ModuleError extends Error { @@ -26,3 +27,59 @@ export function isModuleNotFound(err: unknown): err is ModuleError { export function moduleNotFoundSpecifier(err: ModuleError): string | undefined { return /Cannot find (?:module|package) '([^']+)'/.exec(err.message)?.[1] } + +/** + * True when `pkg` itself is what failed to resolve — it is not installed — as + * opposed to installed and unable to load something. + * + * Matched on the SPECIFIER Node quotes, never on the message text. A substring + * test for the package name is wrong in both directions: an import of + * `@cipherstash/stack/diagnostics` that fails on a missing `dist/` file names + * `…/node_modules/@cipherstash/stack/dist/diagnostics.js` — installed and + * broken, reported as never installed — and `@cipherstash/stack-drizzle` + * contains `@cipherstash/stack`. + */ +export function isPackageMissing(err: unknown, pkg: string): boolean { + if (!isModuleNotFound(err)) return false + const specifier = moduleNotFoundSpecifier(err) + if (specifier === undefined) return false + // For a subpath of an absent package Node quotes the base package under ESM + // but the whole specifier under CJS. Both mean this package. + return specifier === pkg || specifier.startsWith(`${pkg}/`) +} + +/** + * True when `target.pkg` is installed but does not publish `target.subpath` — + * an `@cipherstash/stack` older than `./diagnostics`, say. False when the + * caller asked for no subpath, since then there is none to be missing. + * + * Worth telling apart because it is not a broken install: `stash` declares + * `@cipherstash/stack` as an optional peer with a wide range, so every version + * inside that range predating a subpath lands here, and the user did nothing + * wrong. + * + * Narrow on purpose, because the guidance that follows from it names a package + * and a version. `ERR_PACKAGE_PATH_NOT_EXPORTED` alone does not carry that: an + * exports failure can come from anywhere in an import graph, including a + * dependency of a dependency. So the error must be about this subpath of this + * package, or it is somebody else's problem and must not be answered with an + * upgrade. + */ +export function isSubpathUnavailable( + err: unknown, + target: { pkg: string; subpath?: string }, +): boolean { + if (target.subpath === undefined) return false + if (!(err instanceof Error)) return false + if ((err as { code?: string }).code !== 'ERR_PACKAGE_PATH_NOT_EXPORTED') { + return false + } + // Node names both halves: `Package subpath './diagnostics' is not defined by + // "exports" in /…/@cipherstash/stack/package.json`. Separators normalised + // because win32 is a supported target and that path is built by the OS. + const message = err.message.replaceAll('\\', '/') + return ( + message.includes(`'${target.subpath}'`) && + message.includes(`${target.pkg}/package.json`) + ) +} diff --git a/packages/cli/tests/e2e/doctor-missing-binary.e2e.test.ts b/packages/cli/tests/e2e/doctor-missing-binary.e2e.test.ts index 8d216aad6..0f5ae2594 100644 --- a/packages/cli/tests/e2e/doctor-missing-binary.e2e.test.ts +++ b/packages/cli/tests/e2e/doctor-missing-binary.e2e.test.ts @@ -146,13 +146,12 @@ describe('stash doctor — a platform binary is missing', () => { }) await r.exit - const failures = r.output.match( - new RegExp(messages.doctor.nativeBinaryMissing, 'g'), - ) - expect( - failures, - `expected exactly one failing row:\n${r.output}`, - ).toHaveLength(1) + // Counted by splitting, not by `new RegExp(message)`: the needle is copy + // from `messages.ts`, and copy is free to grow a `.`, `(` or `?` — which a + // regex would silently reinterpret rather than fail on. + const failures = + r.output.split(messages.doctor.nativeBinaryMissing).length - 1 + expect(failures, `expected exactly one failing row:\n${r.output}`).toBe(1) expect(r.output).toContain(protectFfi.label) expect(r.output).not.toContain('@cipherstash/auth-') }) diff --git a/packages/stack/__tests__/subpath-types-parity.test.ts b/packages/stack/__tests__/subpath-types-parity.test.ts new file mode 100644 index 000000000..5c548961d --- /dev/null +++ b/packages/stack/__tests__/subpath-types-parity.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from 'vitest' +import packageJson from '../package.json' + +// `exports` and `typesVersions` describe the same subpaths to two different +// resolvers, and only one of them is exercised by anything in this repo. +// +// `exports` carries the types for `moduleResolution: "node16"`/`"bundler"`, +// which is what every consumer here uses. A consumer on classic node10 +// resolution — still the default for `moduleResolution` when `module` is +// `commonjs`, so not a rarity — reads `typesVersions` instead, and a subpath +// missing from it resolves to `any` with no error at the import site. Nothing +// type-checks that: the package builds, publishes, and installs clean. +// +// So the guard is parity, asserted on the manifest. Adding a subpath means +// adding both, and the failure here names the one that was forgotten. +describe('subpath exports', () => { + it('declare types for node10 resolution as well as node16', () => { + const typesVersions = packageJson.typesVersions['*'] as Record< + string, + string[] + > + + // Keyed on what `exports` publishes, so a `typesVersions` entry left behind + // by a REMOVED subpath is not the failure — it is dead weight, not a + // consumer-visible gap, and pinning it here would make this test fail for + // something it is not about. + const declared: Record = {} + const resolvable: Record = {} + for (const [subpath, target] of Object.entries(packageJson.exports)) { + if (subpath === '.' || subpath === './package.json') continue + const key = subpath.slice(2) + const types = (target as { import?: { types?: string } }).import?.types + if (types === undefined) continue + declared[key] = types + resolvable[key] = typesVersions[key]?.[0] ?? '(no typesVersions entry)' + } + + // Compared as whole objects: the diff names every subpath at once, rather + // than failing on the first and hiding the rest. + expect(resolvable).toEqual(declared) + }) +}) diff --git a/packages/stack/package.json b/packages/stack/package.json index 4a5e0fd69..233c4dd2b 100644 --- a/packages/stack/package.json +++ b/packages/stack/package.json @@ -65,6 +65,9 @@ ], "wasm-inline": [ "./dist/wasm-inline.d.ts" + ], + "diagnostics": [ + "./dist/diagnostics.d.ts" ] } }, diff --git a/skills/stash-encryption/SKILL.md b/skills/stash-encryption/SKILL.md index fa006decd..e3a742752 100644 --- a/skills/stash-encryption/SKILL.md +++ b/skills/stash-encryption/SKILL.md @@ -195,6 +195,7 @@ The SDK never logs plaintext data. | `@cipherstash/stack/schema` | Low-level encrypt-config types and validation helpers; it is not a schema-authoring DSL | | `@cipherstash/stack/encryption` | The `Encryption` factory and the chainable operation classes its methods return (`EncryptOperation`, `DecryptOperation`, `EncryptQueryOperation`, `BulkEncryptModelsOperation`, …). Import these only to *name* an operation's type; author schemas and build the client from `@cipherstash/stack/v3` | | `@cipherstash/stack/adapter-kit` | The internal seam for the **first-party** adapter packages (`@cipherstash/stack-drizzle`, `@cipherstash/stack-supabase`). Not a general-purpose public API — anything an end user needs has a dedicated subpath above. Do not import it in application code | +| `@cipherstash/stack/diagnostics` | One export, `assertNativeBindingAvailable()`, for **tooling** that needs to prove the protect-ffi native binding is installed — this is what `stash doctor` calls. Importing it forces nothing; calling it forces the platform binary to load and throws the loader's own error, naming the missing `@cipherstash/protect-ffi--` package, if it is absent. Not part of the encryption API and not needed in application code | ## Schema Definition From bdf65fa00043a55047dd05e3140651dea50a8ff8 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 13 Aug 2026 18:50:47 +1000 Subject: [PATCH 4/4] docs(cli): note that the doctor E2E now needs a built ffi binding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `stash doctor`'s encryption probe now calls through `@cipherstash/stack/diagnostics`, so the healthy-install E2E needs a real `index.node`. `@cipherstash/stack` is a devDependency here, so the probe always resolves the package, and the workspace-linked platform package carries no binary until cargo has run — which no `build` does, protect-ffi's being `tsc` by design. Without one the run fails on a red encryption row and exit 1, and offers the reinstall advice it has for the npm optional-dependency bug, which does not fix this. That reads as a broken checkout. CI is unaffected: `run-tests` builds the binding long before the CLI E2E step. --- packages/cli/AGENTS.md | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index fed5d1771..1208997ad 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -7,7 +7,7 @@ This package has **two** Vitest configs (plus a self-skipping live-Postgres mode | Command | Config | Scope | Needs build? | | --- | --- | --- | --- | | `pnpm --filter stash test` | `vitest.config.ts` | Unit tests under `src/__tests__/**` and `src/**/__tests__/**` | **Partly** — needs `@cipherstash/stack` built (see below). Turbo's `^build` supplies it in CI. | -| `pnpm --filter stash test:e2e` | `vitest.integration.config.ts` | E2E tests under `tests/e2e/**.e2e.test.ts` driving the built `dist/bin/stash.js` through a real pty (`node-pty`) | **Yes** — run `pnpm --filter stash build` first, or use the turbo `test:e2e` task which depends on `build`. | +| `pnpm --filter stash test:e2e` | `vitest.integration.config.ts` | E2E tests under `tests/e2e/**.e2e.test.ts` driving the built `dist/bin/stash.js` through a real pty (`node-pty`) | **Yes** — run `pnpm --filter stash build` first, or use the turbo `test:e2e` task which depends on `build`. One test also needs protect-ffi's native binding, which no `build` produces (see below). | The unit config explicitly excludes `tests/e2e/**` so the default `pnpm test` stays fast. @@ -86,6 +86,29 @@ exercise the same code paths. - **Build before E2E.** `dist/bin/stash.js` is the artifact under test. The turbo `test:e2e` task already depends on `build`, but if you invoke the script directly you must build first. +- **`doctor.e2e.test.ts` also needs protect-ffi's native binding, and no + `build` produces one.** `stash doctor` probes the encryption engine by + *calling* through `@cipherstash/stack/diagnostics` — importing it proves + nothing, since the neon load is lazy. `@cipherstash/stack` is a devDependency + of this package, so in the workspace the probe always resolves it and never + takes the "not installed, that's fine" arm; and the workspace-linked + `@cipherstash/protect-ffi-` carries no `index.node` until cargo has + run (protect-ffi's `build` is `tsc`, deliberately cargo-free — see the root + `AGENTS.md`). Without a binding the healthy-install test fails on a red + encryption row and exit 1, and `doctor` offers the recovery it has for an npm + user — reinstall `node_modules` — which does not fix this. **That is a + missing binding, not a broken checkout.** Build one (needs a Rust toolchain), + from `packages/protect-ffi`: + + ```bash + mise run build:debug # or: pnpm --filter @cipherstash/protect-ffi build:native + ``` + + CI never hits this: the `run-tests` job in `tests.yml` runs + `.github/actions/build-ffi-binding` long before the CLI E2E step, and that + action caches on a hash of the Rust inputs, so a JS-only PR pays a restore. + Only the healthy-path test needs a real binding: `doctor-missing-binary` + stages the absence itself, in the spawned CLI, and passes either way. - **macOS spawn-helper exec bit.** pnpm strips the executable bit when unpacking node-pty's prebuilds. The helper auto-fixes this at module load via `ensureSpawnHelperExecutable`. If you see `posix_spawnp failed` after