From ac46a5a0a4b8adea1b5fda2c75eb890e980e1fb0 Mon Sep 17 00:00:00 2001
From: Toby Hede <toby@cipherstash.com>
Date: Thu, 13 Aug 2026 11:13:18 +1000
Subject: [PATCH] fix(doctor): an absent optional package is a skipped check,
 not a pass
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two pre-existing issues, both older than the probe rework in #883 and
untouched by it.

`stash doctor` ended with "All checks passed." when `@cipherstash/stack`
was absent. It is an optional peer, so this is the plain `npx stash
doctor` path in a project that has not run `stash init` — the row said
"not installed (run `stash init`)" and the outro then claimed a pass for
a check that never executed. It now ends with `checksIncomplete`, the
line the too-old-to-probe arm already used for exactly this distinction,
and still exits 0: absence there is recoverable, not a failure. The row
stays green for the same reason.

The probe labels move to `messages.ts`. Both E2E suites match a row as
`<label> — <detail>`; the detail halves were already constants while the
labels were copied into each suite, one of them under a comment
conceding the drift. `packages/cli/AGENTS.md` asks for exactly this:
strings tests assert on live in `messages.ts`, and its "only when a test
asserts on it" caveat is now satisfied.

Verified non-vacuous: reverting the one-line outro change fails the
absent-package E2E and nothing else.

- CLI unit 1243 passed, e2e 108 passed
- typecheck 3/3, biome error-free
---
 .changeset/doctor-optional-absent-is-not-a-pass.md    | 10 ++++++++++
 packages/cli/src/commands/doctor/index.ts             | 11 ++++++++---
 packages/cli/src/messages.ts                          |  9 +++++++++
 .../cli/tests/e2e/doctor-missing-binary.e2e.test.ts   |  4 ++--
 .../tests/e2e/doctor-probe-classification.e2e.test.ts | 11 +++++++----
 5 files changed, 36 insertions(+), 9 deletions(-)
 create mode 100644 .changeset/doctor-optional-absent-is-not-a-pass.md

diff --git a/.changeset/doctor-optional-absent-is-not-a-pass.md b/.changeset/doctor-optional-absent-is-not-a-pass.md
new file mode 100644
index 000000000..43af18459
--- /dev/null
+++ b/.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.
diff --git a/packages/cli/src/commands/doctor/index.ts b/packages/cli/src/commands/doctor/index.ts
index 56d31a14c..0e2b466c5 100644
--- a/packages/cli/src/commands/doctor/index.ts
+++ b/packages/cli/src/commands/doctor/index.ts
@@ -40,7 +40,7 @@ interface Probe {
 
 const PROBES: Probe[] = [
   {
-    label: 'Encryption engine (@cipherstash/stack → protect-ffi)',
+    label: messages.doctor.encryptionProbeLabel,
     pkg: '@cipherstash/stack',
     subpath: './diagnostics',
     optional: true,
@@ -55,7 +55,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 =
@@ -177,7 +177,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
diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts
index fc9e77251..e04f826b8 100644
--- a/packages/cli/src/messages.ts
+++ b/packages/cli/src/messages.ts
@@ -26,6 +26,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',
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..adf37d852 100644
--- a/packages/cli/tests/e2e/doctor-missing-binary.e2e.test.ts
+++ b/packages/cli/tests/e2e/doctor-missing-binary.e2e.test.ts
@@ -35,7 +35,7 @@ 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.
@@ -43,7 +43,7 @@ const TARGETS: Target[] = [
   },
   {
     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$`,
   },
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 1650f7dd8..a0b6fdc06 100644
--- a/packages/cli/tests/e2e/doctor-probe-classification.e2e.test.ts
+++ b/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