fix(doctor): set non-zero exit code when error-severity issues are found

src/cli/commands/doctor.ts

@@ -10,7 +10,8 @@
 
 import type { Command } from 'commander';
 import { AdapterResolver } from '../../adapters/resolver.ts';
-import type { SiteConfig } from '../../types.ts';
+import { WpApiError } from '../../adapters/types.ts';
+import { ExitCode, type SiteConfig } from '../../types.ts';
 import { loadConfig, resolveActiveSite } from '../utils/config.ts';
 import { error, info, printJson, warn } from '../utils/output.ts';
 
@@ -77,6 +78,12 @@ interface DoctorIssue {
   severity: 'error' | 'warning' | 'info';
   message: string;
   fix?: string;
+  /**
+   * Structured classification for error-severity issues, used to pick an
+   * exit code. Set this explicitly at each push site rather than pattern-
+   * matching `message` later — message text is for humans and can change.
+   */
+  code?: 'auth' | 'network' | 'generic';
 }
 
 export function registerDoctorCommand(program: Command): void {
@@ -99,9 +106,11 @@ export function registerDoctorCommand(program: Command): void {
 
       if (siteNames.length === 0) {
         error('No sites configured. Run `localpress init` to add one.');
-        process.exit(3);
+        process.exit(ExitCode.ConfigError);
       }
 
+      let worstExit: number = ExitCode.Success;
+
       for (const name of siteNames) {
         const site = config.sites[name];
         if (!site) continue;
@@ -122,23 +131,33 @@ export function registerDoctorCommand(program: Command): void {
             connectionOk = true;
           }
         } catch (err) {
-          const msg = err instanceof Error ? err.message : String(err);
-          if (msg.includes('401') || msg.includes('Unauthorized')) {
+          // WpApiError means the server responded (with a structured HTTP
+          // status) — so this is an application-level error, not a
+          // connectivity failure. Anything else means fetch() itself never
+          // got a response (DNS failure, connection refused, timeout, etc.).
+          // We key off that distinction — and the exact HTTP status for auth
+          // — rather than pattern-matching error message text, which varies
+          // by runtime (e.g. Bun's fetch() doesn't include Node-style codes
+          // like ECONNREFUSED/ENOTFOUND in its error messages).
+          if (err instanceof WpApiError && err.status === 401) {
             issues.push({
               severity: 'error',
               message: 'Authentication failed — Application Password rejected',
               fix: 'Run `localpress sites remove <name>` then `localpress init` to re-enter credentials',
+              code: 'auth',
             });
-          } else if (msg.includes('ENOTFOUND') || msg.includes('ECONNREFUSED')) {
+          } else if (err instanceof WpApiError) {
             issues.push({
               severity: 'error',
-              message: `Cannot reach ${site.url} — check the URL and your network connection`,
-              fix: 'Verify the site URL with `localpress sites` and update if needed',
+              message: `REST API error: ${err.message}`,
+              code: 'generic',
             });
           } else {
             issues.push({
               severity: 'error',
-              message: `REST API error: ${msg}`,
+              message: `Cannot reach ${site.url} — check the URL and your network connection`,
+              fix: 'Verify the site URL with `localpress sites` and update if needed',
+              code: 'network',
             });
           }
         }
@@ -176,13 +195,15 @@ export function registerDoctorCommand(program: Command): void {
                   severity: 'error',
                   message:
                     'sharp installation completed but module still not found. Try restarting your shell.',
+                  code: 'generic',
                 });
               }
             } else {
               issues.push({
                 severity: 'error',
                 message: 'Auto-install failed. Neither bun nor npm is available.',
                 fix: 'Install bun or npm, then run `localpress doctor --fix` again',
+                code: 'generic',
               });
             }
           } else {
@@ -191,6 +212,7 @@ export function registerDoctorCommand(program: Command): void {
               message:
                 'sharp is not installed — optimize, convert, resize, and remove-bg will not work',
               fix: 'Run `localpress doctor --fix` to auto-install, or manually: `bun install -g sharp`',
+              code: 'generic',
             });
           }
         }
@@ -262,16 +284,23 @@ export function registerDoctorCommand(program: Command): void {
             }
           }
           // For auth errors, offer to re-test with updated credentials.
-          const authIssue = issues.find(
-            (i) => i.severity === 'error' && i.message.includes('Authentication'),
-          );
+          const authIssue = issues.find((i) => i.severity === 'error' && i.code === 'auth');
           if (authIssue && connectionOk === false) {
             info('\n  To update credentials, run:');
             info(`    localpress sites remove ${name}`);
             info('    localpress init');
           }
         }
 
+        // -- Exit code accumulation --------------------------------------------
+        // Map this site's error-severity issues to an exit code and track the
+        // worst code seen across all sites (for --all-sites). The exit codes
+        // this loop can produce (Success=0, GenericError=1, NetworkError=4,
+        // AuthError=5) are numbered by increasing severity, so a plain max
+        // is correct — no need for an explicit precedence chain.
+        const siteExitCode = issueListToExitCode(issues);
+        worstExit = Math.max(worstExit, siteExitCode);
+
         // -- Output ------------------------------------------------------------
         if (parentOpts.json) {
           printJson({
@@ -332,9 +361,40 @@ export function registerDoctorCommand(program: Command): void {
           }
         }
       }
+
+      // Set process.exitCode after the loop so --all-sites finishes all sites.
+      if (worstExit !== ExitCode.Success) {
+        process.exitCode = worstExit;
+      }
     });
 }
 
+// -- Exit code helpers --------------------------------------------------------
+
+/**
+ * Map a site's issue list to the appropriate ExitCode, keyed off each
+ * issue's structured `code` (not message text — see DoctorIssue.code).
+ * Priority: Auth(5) > Network(4) > Generic(1) > Success(0).
+ */
+function issueListToExitCode(issues: DoctorIssue[]): number {
+  const errorIssues = issues.filter((i) => i.severity === 'error');
+
+  if (errorIssues.some((i) => i.code === 'auth')) {
+    return ExitCode.AuthError;
+  }
+
+  if (errorIssues.some((i) => i.code === 'network')) {
+    return ExitCode.NetworkError;
+  }
+
+  if (errorIssues.length > 0) {
+    // Any other error (e.g. sharp missing, or an uncategorized REST error).
+    return ExitCode.GenericError;
+  }
+
+  return ExitCode.Success;
+}
+
 // -- Plugin detection ---------------------------------------------------------
 
 interface WpPluginResponse {

test/fixtures/doctor-unreachable.ts

@@ -0,0 +1,39 @@
+/**
+ * Subprocess fixture for doctor-exit-code tests.
+ *
+ * Runs `localpress doctor` with a mock fetch that always throws TypeError
+ * (simulating an unreachable host), so tests can exercise the NetworkError
+ * exit-code path without real network I/O and without setting process.exitCode
+ * in the test runner's own process (which would poison bun's test exit code).
+ *
+ * Usage (via Bun.spawnSync):
+ *   bun run test/fixtures/doctor-unreachable.ts [doctor args...]
+ *
+ * The site config is read from XDG_CONFIG_HOME, which the caller seeds before
+ * spawning this script.
+ */
+
+import { Command, Option } from 'commander';
+import { registerDoctorCommand } from '../../src/cli/commands/doctor.ts';
+import { setOutputOptions } from '../../src/cli/utils/output.ts';
+
+// Mock fetch before any module that calls it is initialized.
+globalThis.fetch = (async () => {
+  throw new TypeError('fetch failed');
+}) as unknown as typeof fetch;
+
+const program = new Command();
+program
+  .name('localpress')
+  .exitOverride()
+  .addOption(new Option('--site <name>', 'override the active site for this command'))
+  .addOption(new Option('--json', 'machine-readable JSON output').default(false))
+  .addOption(new Option('--quiet', 'errors only; suppress info messages').default(false))
+  .hook('preAction', (thisCommand) => {
+    const opts = thisCommand.opts();
+    setOutputOptions({ json: Boolean(opts.json), quiet: Boolean(opts.quiet) });
+  });
+
+registerDoctorCommand(program);
+
+await program.parseAsync(process.argv.slice(2), { from: 'user' });

test/unit/doctor-exit-code.test.ts

@@ -0,0 +1,169 @@
+/**
+ * doctor exit-code contract tests.
+ *
+ * Exercises the exit-code behaviour added in #267 (OSS-1354):
+ *   - no sites configured  → exit 3 (ConfigError)
+ *   - unreachable site     → exit 4 (NetworkError), connectionOk:false in --json
+ *   - healthy site         → exit 0 (manual / integration tests only — needs live WP)
+ *
+ * All cases use Bun.spawnSync to test exit codes in a subprocess, keeping
+ * process.exitCode in the test runner's own process always zero. In-process
+ * invocations that set process.exitCode would cause bun test to exit with a
+ * non-zero code even when all assertions pass.
+ *
+ * For unreachable-site cases we use test/fixtures/doctor-unreachable.ts, a
+ * small harness that mocks globalThis.fetch before running doctor. This gives
+ * us deterministic "connection failed" behaviour regardless of CI network
+ * configuration — no real sockets needed.
+ */
+
+import { describe, expect, test } from 'bun:test';
+import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+
+const CLI_ENTRY = join(process.cwd(), 'src', 'cli', 'index.ts');
+const UNREACHABLE_FIXTURE = join(process.cwd(), 'test', 'fixtures', 'doctor-unreachable.ts');
+const SITE_NAME = 'testsite';
+
+/**
+ * Seed a minimal localpress config file in the given config dir so
+ * `loadConfig()` finds a site without any real network calls.
+ */
+function seedConfig(
+  configDir: string,
+  sites: Record<string, { url: string; username?: string; appPassword?: string }>,
+  activeSite = SITE_NAME,
+): void {
+  mkdirSync(join(configDir, 'localpress'), { recursive: true });
+  const config = {
+    version: 1,
+    activeSite,
+    sites: Object.fromEntries(
+      Object.entries(sites).map(([name, s]) => [
+        name,
+        {
+          name,
+          url: s.url,
+          username: s.username ?? 'admin',
+          appPassword: s.appPassword ?? 'fake fake fake fake fake fake',
+          createdAt: new Date(0).toISOString(),
+        },
+      ]),
+    ),
+  };
+  writeFileSync(
+    join(configDir, 'localpress', 'config.json'),
+    JSON.stringify(config, null, 2),
+    'utf8',
+  );
+}
+
+/** Spawn the CLI (no site seeded) with an ephemeral config dir. */
+function runCli(
+  args: string[],
+  extraEnv: Record<string, string> = {},
+): { stdout: string; stderr: string; exitCode: number } {
+  const configDir = mkdtempSync(join(tmpdir(), 'localpress-doctor-test-'));
+  try {
+    const result = Bun.spawnSync(['bun', 'run', CLI_ENTRY, ...args], {
+      env: { ...process.env, XDG_CONFIG_HOME: configDir, ...extraEnv },
+      timeout: 30_000,
+    });
+    return {
+      stdout: result.stdout.toString(),
+      stderr: result.stderr.toString(),
+      exitCode: result.exitCode ?? 1,
+    };
+  } finally {
+    rmSync(configDir, { recursive: true, force: true });
+  }
+}
+
+/**
+ * Spawn the unreachable-fixture script with a pre-seeded config dir so
+ * globalThis.fetch is mocked before doctor runs. The fixture exits with
+ * process.exitCode set by the doctor action — no pollution of the test
+ * runner's own process.exitCode.
+ */
+function runUnreachable(
+  doctorArgs: string[],
+  sites: Record<string, { url: string }> = { [SITE_NAME]: { url: 'https://example.test' } },
+  activeSite = SITE_NAME,
+): { stdout: string; stderr: string; exitCode: number } {
+  const configDir = mkdtempSync(join(tmpdir(), 'localpress-doctor-unreachable-'));
+  try {
+    seedConfig(configDir, sites, activeSite);
+    const result = Bun.spawnSync(['bun', 'run', UNREACHABLE_FIXTURE, ...doctorArgs], {
+      env: { ...process.env, XDG_CONFIG_HOME: configDir },
+      timeout: 30_000,
+    });
+    return {
+      stdout: result.stdout.toString(),
+      stderr: result.stderr.toString(),
+      exitCode: result.exitCode ?? 1,
+    };
+  } finally {
+    rmSync(configDir, { recursive: true, force: true });
+  }
+}
+
+// ---------------------------------------------------------------------------
+// No sites configured — uses the real CLI entry point, no site seeded.
+// ---------------------------------------------------------------------------
+
+describe('doctor exit codes — no sites configured', () => {
+  test('doctor with no sites configured exits 3 (ConfigError)', () => {
+    const { exitCode } = runCli(['doctor']);
+    expect(exitCode).toBe(3);
+  });
+
+  test('doctor --all-sites with no sites configured exits 3 (ConfigError)', () => {
+    const { exitCode } = runCli(['doctor', '--all-sites']);
+    expect(exitCode).toBe(3);
+  });
+});
+
+// ---------------------------------------------------------------------------
+// Unreachable site — fetch is mocked inside the subprocess fixture so the
+// connection check always fails deterministically, regardless of CI network.
+// ---------------------------------------------------------------------------
+
+describe('doctor exit codes — unreachable site', () => {
+  test('doctor with unreachable site exits 4 (NetworkError)', () => {
+    const { exitCode } = runUnreachable(['doctor']);
+    expect(exitCode).toBe(4);
+  });
+
+  test('doctor --json with unreachable site exits 4 and emits connectionOk:false', () => {
+    const { stdout, exitCode } = runUnreachable(['doctor', '--json']);
+    expect(exitCode).toBe(4);
+
+    const lines = stdout.trim().split('\n').filter(Boolean);
+    expect(lines.length).toBeGreaterThan(0);
+    const parsed = JSON.parse(lines[0]);
+    expect(parsed.connectionOk).toBe(false);
+
+    const errorIssues = (parsed.issues as Array<{ severity: string }>).filter(
+      (i) => i.severity === 'error',
+    );
+    expect(errorIssues.length).toBeGreaterThan(0);
+  });
+
+  test('doctor --all-sites with unreachable site exits 4', () => {
+    const { exitCode } = runUnreachable(['doctor', '--all-sites'], {
+      [SITE_NAME]: { url: 'https://example.test' },
+      'second-site': { url: 'https://example2.test' },
+    });
+    expect(exitCode).toBe(4);
+  });
+
+  test('doctor with a second, independent unreachable site exits 4 (NetworkError)', () => {
+    const { exitCode } = runUnreachable(
+      ['doctor'],
+      { 'another-site': { url: 'https://another-example.test' } },
+      'another-site',
+    );
+    expect(exitCode).toBe(4);
+  });
+});