stash doctor: probe the native binding for real - #883
Conversation
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.
…bpath arm 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 detectedLatest commit: bdf65fa The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…ment it `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.
|
Third round of review fixes pushed (
One thing the guard deliberately does not cover: Skill table — confirmed. Exported-for-tests internals — taken, not defended. Unescaped Duplicated resolver-hiding preload — left as is, with a reason. The two copies are in different packages ( Verified: CLI unit 1243, CLI e2e 108, stack suites 34, |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed the full diff plus the branch's surrounding code — protect-ffi's loader and isEncrypted, the classifier regexes, tests.yml, and the CLI's dependency graph. Approving.
Load-bearing claims verified:
isEncrypted(null)is safe on a healthy install: protect-ffi's ownassertNativeBindingAvailablemakes the identical call (packages/protect-ffi/src/index.cts:220), andnativeLoading.test.tspins that it returnsfalserather than throwing against a real binding. So the probe can only throw what the loader raised — which is the whole contract.- Probing through
isEncryptedinstead of re-exporting protect-ffi'sassertNativeBindingAvailableis the right call, and thepublishedFfiSurfacePreloadtest is the right way to hold it:workspace:*genuinely hides the released-surface mismatch, and that stand-in is the only thing in the repo that can see it. - The napi code-less error shape is verified end-to-end, not just against the hand-mirrored
napiLoadErrorfixture —doctor-missing-binary.e2e.test.tshides the real auth binary in a spawned CLI and asserts the classified output, so if@cipherstash/auth's generated loader ever changes its message, the e2e fails even though the unit fixture wouldn't. That's exactly the failure mode the PR body describes, closed properly. isPackageMissingkeying on the quoted specifier (with thestartsWith(${pkg}/)CJS case) fixes both false-positive directions — the installed-but-broken path and thestack-drizzleprefix collision — and both have Node-raised fixtures.isSubpathUnavailablerequiring the error to name the probe's own subpath and package is the correct narrowing; the pnpm.pnpmpath layout still contains@cipherstash/stack/package.json, so the match survives real installs. The auth-probe test correctly asserts that an unclassifiable exports failure is rethrown (exit 1) rather than answered with unrelated upgrade advice.- The
splitting: falseisolated tsup config plus the assert-on-the-artifact specifier test makes "reaches protect-ffi and nothing else" structural, and assertingtoEqual(['@cipherstash/protect-ffi'])rather thannot.toContain('auth')is the right shape — a shared chunk arrives under a relative name. - Repo hygiene: both changesets present, neither names an FFI package;
AGENTS.mdsubpath list updated per its rule 7;skills/stash-encryptionrow added; the newsubpath-types-paritytest closes a real node10-resolution gap that nothing else type-checks.
One note, non-blocking:
The healthy-path doctor.e2e.test.ts now genuinely requires a built protect-ffi binding. In CI that's fine — the Node 22/24 jobs run build-ffi-binding before the CLI e2e step, and a JS-only PR pays a cache restore. But locally, a contributor without a Rust toolchain running pnpm --filter stash test:e2e will now see the healthy doctor test fail with a red encryption row: @cipherstash/stack is a devDependency of the CLI, so the probe always finds it, and the workspace-linked platform packages only carry index.node after build:native. Before this PR the probe reached auth (whose binaries come prebuilt from npm) and passed Rust-free. Consider a line in packages/cli/AGENTS.md noting that test:e2e now needs the binding built (mise run build in packages/protect-ffi or a debug build), so the first person to hit it doesn't read it as a broken checkout.
Observed but fine:
- An
ERR_PACKAGE_PATH_NOT_EXPORTEDfrom deeper in a probe's graph that doesn't match the declared subpath still surfaces as the launcher's bareFatal error— but that's a genuinely unknown failure, and rethrowing beats misdiagnosing; the tests document this as intended. messages.doctor.cannotProbehard-codes@cipherstash/stackwhile the mechanism is generic — fine while only one probe declares a subpath.
`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.
2 participants
Closes CIP-3720.
Making the protect-ffi native load lazy was a real perf win, but it cost
stash doctorits only real check — and hid that a second one had never worked. Both rows were green with nothing installed.What was broken
The encryption check never loaded anything.
@neon-rs/load's proxy resolves the platform binary on first property access inside a wrapper body, so importing the package proves nothing: the probe passed with no binary present and the failure landed later, at the first encrypt.It was also measuring the wrong package. The root entry re-exports the auth strategies, and
@cipherstash/auth's binding is eager (module.exports = { ...native }, a spread that forces any loader), so the encryption row was a second auth check. Two rows, one signal — hiding auth's binary reddened both, hiding protect-ffi's reddened neither.A missing auth binary printed a bare
Fatal errorfrom every command. napi's loader requires each candidate insidetry { … } catch (_) {}and throws its own summary, so nocodeand norequireStacksurvive. The CLI's classifier keyed onMODULE_NOT_FOUNDand could never match it — the recovery guidance was skipped everywhere, not just indoctor.What this does
Adds
@cipherstash/stack/diagnostics, one export, called rather than merely imported. There is no consumer-side alternative: protect-ffi's loader is not in itsexportsmap (ERR_PACKAGE_PATH_NOT_EXPORTED), and reading an export never reaches the proxy. Its own tsup config withsplitting: false, because what it must not reach is the point of it — a shared chunk importing@cipherstash/authputs the probe back on auth's binary, andsplitting: falsemakes that structural rather than a property of today's module graph.doctornow names what to CALL per probe, classifies four failure modes distinctly, and exits non-zero when either platform package is missing.Two more the tests found
"not installed" was a substring match. The probe's own import path contains the package name, so an installed-but-broken
@cipherstash/stackwas reported as one you had not installed yet — green, with nothing to suggest looking further.@cipherstash/stack-drizzlematched it too. Now keyed on the specifier Node failed to resolve.The outro claimed every check passed after skipping one. A run that could not complete a check says so, and still exits 0 — an unrunnable check is not a diagnosis.
Two from review
The subpath re-exported an export that is not published.
assertNativeBindingAvailablearrived with the lazy load, whose changeset is parked.deferreduntil the publishing cutover, so no released protect-ffi has it — verified against the tarball on npm.@cipherstash/stackdepends onworkspace:*, so it built here and would have shipped against0.31.0: a link-timeSyntaxErrorunder ESM,undefinedunder CJS, neither classifiable, so a bareFatal errorfor every user of a released install. It now probes throughisEncrypted, published since 0.28.0 and the same call protect-ffi's own assert makes — pure, synchronous, no validation before the addon.The stale-subpath arm was probe-agnostic while its advice was not. It keyed on
ERR_PACKAGE_PATH_NOT_EXPORTEDalone and saw every probe's failure, so an exports problem anywhere in auth'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.Tests
Coverage for all four classification arms, at two seams — the pure classifiers, and
stash doctoras a spawned process.Every error asserted on is one Node raised, against a package layout the fixture builds (an absent package, an older one, a hidden binary). Nothing is hand-built with the code and message pasted on: a fixture like that asserts on itself and keeps passing when Node changes the shape, which is exactly how the auth arm of
isNativeBinaryMissingcame to exist without ever being able to fire.The one that matters most is non-vacuity: hiding protect-ffi's binary must redden exactly one row.
turbo typecheck3/3, biome error-freeNotes for the reviewer
AGENTS.md's subpath-export list is updated, per its own rule 7.skills/stash-clidescribesdoctorgenerically.lint:ffi-changesetis satisfied.