Merge pull request #884 from cipherstash/toby/doctor-preexisting-label-and-outro

stash doctor: an absent optional package is a skipped check, not a pass

Author: tobyhede

.changeset/doctor-optional-absent-is-not-a-pass.md

@@ -0,0 +1,10 @@
+---
+'stash': patch
+---
+
+`stash doctor` no longer reports "All checks passed." when `@cipherstash/stack`
+is absent. The package is an optional peer, so running `doctor` before `stash
+init` skips the encryption check entirely — the row already said so, but the
+outro claimed a pass for a check that never ran. It now ends with "stash doctor
+could not run every check.", the same line an unprobeable install gets, and
+still exits 0: an absent optional package is recoverable, not a failure.

packages/cli/src/commands/doctor/index.ts

@@ -37,7 +37,7 @@ interface Probe {
 
 const PROBES: Probe[] = [
   {
-    label: 'Encryption engine (@cipherstash/stack → protect-ffi)',
+    label: messages.doctor.encryptionProbeLabel,
     pkg: '@cipherstash/stack',
     subpath: './diagnostics',
     optional: true,
@@ -52,7 +52,7 @@ const PROBES: Probe[] = [
     },
   },
   {
-    label: 'Auth (@cipherstash/auth)',
+    label: messages.doctor.authProbeLabel,
     pkg: '@cipherstash/auth',
     async force() {
       // No counterpart call needed. This package's entry is `module.exports =
@@ -116,7 +116,12 @@ export async function doctorCommand(): Promise<void> {
             ? messages.doctor.notInstalledOptional
             : messages.doctor.notInstalled,
         )
-        if (!probe.optional) failed = true
+        // The row stays green — absence before `stash init` is expected, and
+        // the detail already says so — but the check did not RUN, which is the
+        // same thing the too-old arm below records. Without this the outro said
+        // every check passed while one of the two never executed.
+        if (probe.optional) incomplete = true
+        else failed = true
       } else if (isSubpathUnavailable(err, probe)) {
         report('warn', probe.label, messages.doctor.cannotProbe)
         incomplete = true

packages/cli/src/messages.ts

@@ -35,6 +35,15 @@ export const messages = {
     title: 'stash doctor',
     /** Leader of the platform check line; the `<platform>-<arch>` is appended. */
     platformLabel: 'Platform',
+    /**
+     * Probe row labels. Here rather than inline in `commands/doctor/index.ts`
+     * because both E2E suites match a row as `<label> — <detail>`: the detail
+     * halves were already constants while the labels were copied into each
+     * suite, so a copy tweak broke the tests from a file they don't name.
+     */
+    encryptionProbeLabel:
+      'Encryption engine (@cipherstash/stack → protect-ffi)',
+    authProbeLabel: 'Auth (@cipherstash/auth)',
     allChecksPassed: 'All checks passed.',
     /** Row detail when a probe reached the loader and no platform binary was there. */
     nativeBinaryMissing: 'native binary missing',

packages/cli/tests/e2e/doctor-missing-binary.e2e.test.ts

@@ -35,15 +35,15 @@ interface Target {
 const TARGETS: Target[] = [
   {
     pkg: '@cipherstash/protect-ffi',
-    label: 'Encryption engine (@cipherstash/stack → protect-ffi)',
+    label: messages.doctor.encryptionProbeLabel,
     // `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)',
+    label: messages.doctor.authProbeLabel,
     // napi's local-build arm, tried before the platform package.
     extra: String.raw`|(?:^|[\\/])stack-auth-node\.node$`,
   },

packages/cli/tests/e2e/doctor-probe-classification.e2e.test.ts

@@ -26,9 +26,8 @@ import { render } from '../helpers/pty.js'
 // 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)'
-const AUTH_LABEL = 'Auth (@cipherstash/auth)'
+const ENCRYPTION_LABEL = messages.doctor.encryptionProbeLabel
+const AUTH_LABEL = messages.doctor.authProbeLabel
 
 interface Unresolve {
   /** The bare specifier to divert. */
@@ -125,7 +124,11 @@ describe.skipIf(!hooksAvailable)('stash doctor — probe classification', () =>
     expect(r.output).toContain(
       `${ENCRYPTION_LABEL} — ${messages.doctor.notInstalledOptional}`,
     )
-    expect(r.output).toContain(messages.doctor.allChecksPassed)
+    // Recoverable, so exit 0 and a green row — but the encryption check did not
+    // run, and the outro must not say it passed. Same distinction the too-old
+    // arm makes below; this arm predates it and claimed a pass.
+    expect(r.output).toContain(messages.doctor.checksIncomplete)
+    expect(r.output).not.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