fix(pilot): stop silently swallowing fs errors in agent token helpers (#985)

* fix(pilot): stop silently swallowing fs errors in agent token helpers

The pilot-agent audit found that both filesystem-touching helpers in
the agent process discarded every fs error class:

  - readPersistedToken caught all errors and returned null. ENOENT
    (normal first boot) and EACCES / EIO / EROFS (a real disk or
    volume-permission failure) were indistinguishable; the latter
    silently fell back to "no persisted token", and on a node where
    the enrollment token had already been consumed the agent would
    enter a re-enrollment loop with no log signal pointing at the disk.
  - persistToken caught all errors and logged at console.warn. The
    operator saw the next-boot loop with the same diagnostic gap.

Replace both with errno-aware handling:

  - readPersistedToken calls fs.readFileSync directly (no TOCTOU race
    against existsSync), treats ENOENT as silent, and logs every other
    errno at ERROR with the path.
  - persistToken logs at ERROR (not WARN) with the failing errno and an
    explicit "next agent restart will require re-enrollment until the
    volume is writable" message. The agent still continues with the
    in-memory token so the current session is unaffected.

Both helpers are now exported (marked @internal) so the new test file
can mock fs and assert the error-class-vs-log-level matrix.

12 unit cases in pilot-agent-fs-errors.test.ts cover ENOENT,
EACCES, EIO, EROFS, ENOSPC, missing errno, empty file, and the
happy path. Mock pattern follows backend/src/__tests__/filesystem.test.ts.

* fix(pilot): address code-review on the fs-error branch

Three findings from the review pass:

  - Em dash in a test description (Directive 18). Rewritten as
    "does not throw, so the in-memory token stays usable for the
    current session".
  - persistToken still had an existsSync + mkdirSync pair around
    the token-write. mkdirSync({ recursive: true }) is idempotent on
    existing directories, so the existsSync was redundant and added
    a TOCTOU window where the directory could be removed between the
    probe and the write. Dropped the existsSync; the test that
    previously primed mockExistsSync now asserts mockExistsSync is
    NOT called, locking the TOCTOU removal.
  - The two new exports used the @internal JSDoc tag, but this
    repo's existing pattern for "public-by-convention-for-tests"
    helpers (e.g. RegistryService.ts:481) is plain prose
    "Exposed for unit tests." Switched to that style.

No behavior change beyond the TOCTOU removal.

backend/src/__tests__/pilot-agent-fs-errors.test.ts

@@ -0,0 +1,169 @@
+/**
+ * Unit tests for the pilot agent's filesystem-touching helpers.
+ *
+ * Both readPersistedToken and persistToken used to swallow every fs error
+ * silently; the audit found that an unwritable /app/data volume looked
+ * exactly like a fresh first boot to the rest of the agent, leaving the
+ * operator with no diagnostic signal when a node entered a re-enrollment
+ * loop. These tests lock the new behavior:
+ *
+ *   - readPersistedToken treats ENOENT as silent (normal first boot) and
+ *     surfaces every other errno at ERROR with the path and code.
+ *   - persistToken logs at ERROR (not WARN) with an actionable
+ *     "next restart will require re-enrollment" message and the failing
+ *     errno, so the operator has a single log line that points at the disk.
+ *
+ * fs is mocked at the module-load layer per the existing pattern in
+ * filesystem.test.ts. The agent's top-level imports have no side effects,
+ * so importing the agent module after vi.mock is safe.
+ */
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+
+const { mockReadFileSync, mockWriteFileSync, mockExistsSync, mockMkdirSync } = vi.hoisted(() => ({
+    mockReadFileSync: vi.fn(),
+    mockWriteFileSync: vi.fn(),
+    mockExistsSync: vi.fn(),
+    mockMkdirSync: vi.fn(),
+}));
+
+vi.mock('fs', () => {
+    const mock = {
+        readFileSync: mockReadFileSync,
+        writeFileSync: mockWriteFileSync,
+        existsSync: mockExistsSync,
+        mkdirSync: mockMkdirSync,
+    };
+    return { ...mock, default: mock };
+});
+
+// agent.ts is imported AFTER vi.mock so the mock is in place when the
+// module's top-level fs import resolves.
+import { readPersistedToken, persistToken } from '../pilot/agent';
+
+let errorSpy: ReturnType<typeof vi.spyOn>;
+let warnSpy: ReturnType<typeof vi.spyOn>;
+
+beforeEach(() => {
+    vi.clearAllMocks();
+    errorSpy = vi.spyOn(console, 'error').mockImplementation(() => { /* swallow */ });
+    warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => { /* swallow */ });
+});
+
+afterEach(() => {
+    errorSpy.mockRestore();
+    warnSpy.mockRestore();
+});
+
+function fsError(code: string, message?: string): NodeJS.ErrnoException {
+    const err = new Error(message ?? code) as NodeJS.ErrnoException;
+    err.code = code;
+    return err;
+}
+
+describe('readPersistedToken', () => {
+    it('returns the trimmed token when the file is present', () => {
+        mockReadFileSync.mockReturnValueOnce('  eyJhbGciOiJIUzI1NiJ9.payload.sig  \n');
+        const token = readPersistedToken();
+        expect(token).toBe('eyJhbGciOiJIUzI1NiJ9.payload.sig');
+        expect(errorSpy).not.toHaveBeenCalled();
+        expect(warnSpy).not.toHaveBeenCalled();
+    });
+
+    it('returns null and logs nothing on ENOENT (normal first boot)', () => {
+        mockReadFileSync.mockImplementationOnce(() => { throw fsError('ENOENT', 'no such file'); });
+        const token = readPersistedToken();
+        expect(token).toBeNull();
+        expect(errorSpy).not.toHaveBeenCalled();
+        expect(warnSpy).not.toHaveBeenCalled();
+    });
+
+    it('returns null and logs at ERROR on EACCES (volume permission flip)', () => {
+        mockReadFileSync.mockImplementationOnce(() => { throw fsError('EACCES', 'permission denied'); });
+        const token = readPersistedToken();
+        expect(token).toBeNull();
+        expect(errorSpy).toHaveBeenCalledOnce();
+        const msg = String(errorSpy.mock.calls[0][0]);
+        expect(msg).toContain('Failed to read persisted tunnel token');
+        expect(msg).toContain('EACCES');
+        expect(warnSpy).not.toHaveBeenCalled();
+    });
+
+    it('returns null and logs at ERROR on EIO (disk failure)', () => {
+        mockReadFileSync.mockImplementationOnce(() => { throw fsError('EIO', 'i/o error'); });
+        const token = readPersistedToken();
+        expect(token).toBeNull();
+        expect(errorSpy).toHaveBeenCalledOnce();
+        expect(String(errorSpy.mock.calls[0][0])).toContain('EIO');
+    });
+
+    it('returns null and logs at ERROR even when the errno is missing', () => {
+        // Synthetic error with no .code attached: still classified as failure,
+        // not as ENOENT, so the operator gets a signal.
+        mockReadFileSync.mockImplementationOnce(() => { throw new Error('something weird'); });
+        const token = readPersistedToken();
+        expect(token).toBeNull();
+        expect(errorSpy).toHaveBeenCalledOnce();
+        expect(String(errorSpy.mock.calls[0][0])).toContain('unknown');
+    });
+
+    it('returns null when the file is empty (avoid handing a blank string to the WS auth header)', () => {
+        mockReadFileSync.mockReturnValueOnce('   \n  ');
+        const token = readPersistedToken();
+        expect(token).toBeNull();
+        expect(errorSpy).not.toHaveBeenCalled();
+    });
+});
+
+describe('persistToken', () => {
+    it('writes the token with mode 0o600 on the happy path', () => {
+        persistToken('test-token');
+        expect(mockWriteFileSync).toHaveBeenCalledWith(
+            expect.stringContaining('pilot.jwt'),
+            'test-token',
+            { mode: 0o600 },
+        );
+        expect(errorSpy).not.toHaveBeenCalled();
+        expect(warnSpy).not.toHaveBeenCalled();
+    });
+
+    it('creates the data directory recursively without an existsSync probe', () => {
+        persistToken('test-token');
+        expect(mockMkdirSync).toHaveBeenCalledWith(expect.any(String), { recursive: true });
+        expect(mockWriteFileSync).toHaveBeenCalled();
+        // Lock the TOCTOU removal: an existsSync call here would re-introduce
+        // the race window between the probe and the mkdir/write.
+        expect(mockExistsSync).not.toHaveBeenCalled();
+    });
+
+    it('logs at ERROR (not WARN) on ENOSPC and includes the actionable message', () => {
+        mockWriteFileSync.mockImplementationOnce(() => { throw fsError('ENOSPC', 'no space left'); });
+        persistToken('test-token');
+        expect(errorSpy).toHaveBeenCalledOnce();
+        const msg = String(errorSpy.mock.calls[0][0]);
+        expect(msg).toContain('Failed to persist tunnel token');
+        expect(msg).toContain('ENOSPC');
+        expect(msg).toContain('next agent restart will require re-enrollment');
+        // Critical: no console.warn fallback. The previous behavior was a
+        // silent warn that the operator missed.
+        expect(warnSpy).not.toHaveBeenCalled();
+    });
+
+    it('logs at ERROR on EACCES (read-only volume mount)', () => {
+        mockWriteFileSync.mockImplementationOnce(() => { throw fsError('EACCES', 'permission denied'); });
+        persistToken('test-token');
+        expect(errorSpy).toHaveBeenCalledOnce();
+        expect(String(errorSpy.mock.calls[0][0])).toContain('EACCES');
+    });
+
+    it('logs at ERROR on EROFS (read-only filesystem)', () => {
+        mockWriteFileSync.mockImplementationOnce(() => { throw fsError('EROFS', 'read-only file system'); });
+        persistToken('test-token');
+        expect(errorSpy).toHaveBeenCalledOnce();
+        expect(String(errorSpy.mock.calls[0][0])).toContain('EROFS');
+    });
+
+    it('does not throw, so the in-memory token stays usable for the current session', () => {
+        mockWriteFileSync.mockImplementationOnce(() => { throw fsError('EIO', 'i/o error'); });
+        expect(() => persistToken('test-token')).not.toThrow();
+    });
+});

backend/src/pilot/agent.ts

@@ -603,13 +603,31 @@ type MeshResolveResult =
     | { ok: true; host: string; port: number }
     | { ok: false; err: MeshErrCode };
 
-function readPersistedToken(): string | null {
+/**
+ * Read the persisted long-lived tunnel token from disk if present. ENOENT is
+ * the normal first-boot case and stays silent. Any other error class
+ * (EACCES, EIO, EISDIR, etc.) almost certainly means the volume is
+ * misconfigured or corrupt; log at ERROR with the path and the errno so the
+ * operator has an actionable signal, then return null. Returning null here
+ * lets the caller fall back to SENCHO_ENROLL_TOKEN if one is set, or exit
+ * with a clear "no credentials" message if not.
+ *
+ * Calls readFileSync directly rather than racing existsSync + readFileSync
+ * to avoid TOCTOU and to surface the actual errno on real failures.
+ *
+ * Exposed for unit tests.
+ */
+export function readPersistedToken(): string | null {
     try {
-        if (fs.existsSync(TOKEN_PATH)) {
-            return fs.readFileSync(TOKEN_PATH, 'utf8').trim() || null;
-        }
-    } catch { /* ignore */ }
-    return null;
+        return fs.readFileSync(TOKEN_PATH, 'utf8').trim() || null;
+    } catch (err) {
+        const code = (err as NodeJS.ErrnoException).code;
+        if (code === 'ENOENT') return null;
+        console.error(
+            `[Pilot] Failed to read persisted tunnel token at ${sanitizeForLog(TOKEN_PATH)}: ${sanitizeForLog(code ?? 'unknown')} - ${sanitizeForLog((err as Error).message)}`,
+        );
+        return null;
+    }
 }
 
 /**
@@ -630,13 +648,28 @@ function readPilotCaBundle(): Buffer | null {
     }
 }
 
-function persistToken(token: string): void {
+/**
+ * Persist the long-lived tunnel token so the agent can reconnect after a
+ * container restart without re-enrolling. On failure we log at ERROR (not
+ * WARN) with an explicit "next agent restart will require re-enrollment"
+ * message: a silent warning here meant the operator saw the next-boot
+ * re-enrollment loop with no signal pointing at the disk. The current
+ * tunnel session continues with the in-memory token regardless.
+ *
+ * mkdirSync with recursive:true is idempotent on existing directories, so
+ * the prior existsSync guard was redundant and added a TOCTOU window.
+ *
+ * Exposed for unit tests.
+ */
+export function persistToken(token: string): void {
     try {
-        const dir = path.dirname(TOKEN_PATH);
-        if (!fs.existsSync(dir)) fs.mkdirSync(dir, { recursive: true });
+        fs.mkdirSync(path.dirname(TOKEN_PATH), { recursive: true });
         fs.writeFileSync(TOKEN_PATH, token, { mode: 0o600 });
     } catch (err) {
-        console.warn('[Pilot] Failed to persist tunnel token:', (err as Error).message);
+        const code = (err as NodeJS.ErrnoException).code;
+        console.error(
+            `[Pilot] Failed to persist tunnel token at ${sanitizeForLog(TOKEN_PATH)} (${sanitizeForLog(code ?? 'unknown')}: ${sanitizeForLog((err as Error).message)}). Continuing with the in-memory token; the next agent restart will require re-enrollment until the volume is writable.`,
+        );
     }
 }