fix(fleet): harden #791 upgrade rollback against find/reset failures

Second- and third-round independent-review reliability fixes on the keep-mode upgrade rollback path, plus accurate abort messaging. All fixed red-first with self-verifying controls in the rollback gate. Round 2 (blockers A/B, should-fix C): - install.sh: `trap 'restore_snapshot; exit 1' ERR INT TERM` so an INT/TERM mid-sync terminates instead of resuming past the interrupt and reporting success (a bash signal handler that only returns does not terminate). - manifest.{ts,sh}: reject a degenerate [framework] section whose entries are all empty or bare-dot (`/`, `./`, `.`, `..`) — it passed the non-empty guard yet yielded zero usable globs, silently resolving everything to operator. Parity via a shared `[^/.]` usable-glob test; TS throws ManifestError. - finalize.ts: classify the sync-abort message — a ManifestError is a pre-sync validation abort ("no files were changed"); any other error may be partial. Round 3 (blockers D1, D2): - install.sh: enumerate framework files with a checked temp file (_scan_or_die) instead of `< <(find …)` — process substitution discards find's exit status, so an EACCES/I/O failure mid-scan would truncate the file list yet leave the loop exiting 0, committing a partial upgrade as success (ERR trap never fires). - install.sh: guard the `rm -rf; mkdir -p` target reset inside restore_snapshot — a bare reset failing under set -e exits silently after partial deletion, never printing the snapshot-recovery pointer. Now checked like the cp -a restore: on failure it preserves the snapshot and tells the operator where. Tests: rollback gate 14→28 (Parts C/D/E with disabled-guard controls); new finalize-sync-abort.spec.ts (3). No secret value is ever emitted; snapshots stay 0700. Gates green: typecheck, lint, format:check, full mosaic vitest 1094, HARD GATE 193, rollback 28, migration 21. Refs #791 Co-Authored-By: Claude Opus 4.8 <[email protected]>
2026-07-16 17:30:19 -05:00
parent 0a5e703a70
commit af627e7583
11 changed files with 1003 additions and 26 deletions
@@ -1,6 +1,6 @@
 import { afterAll, describe, it, expect } from 'vitest';
-import { execFileSync } from 'node:child_process';
-import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
+import { execFileSync, spawnSync } from 'node:child_process';
+import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { fileURLToPath } from 'node:url';
@@ -55,6 +55,19 @@ function bashSubtreeRoots(): string[] {
    .filter((s) => s.length > 0);
 }

+/**
+ * Drive the bash resolver CLI against a manifest file and report how it exited.
+ * A fail-closed manifest must make the CLI exit non-zero with a message on
+ * stderr — never exit 0 having silently resolved everything to operator.
+ */
+function bashCli(manifestFile: string): { status: number; stderr: string } {
+  const res = spawnSync('bash', [MANIFEST_SH, 'resolve', 'CONSTITUTION.md'], {
+    encoding: 'utf-8',
+    env: { ...process.env, MANIFEST_FILE: manifestFile },
+  });
+  return { status: res.status ?? -1, stderr: res.stderr ?? '' };
+}
+
 // Paths spanning every ownership class: framework single-files, framework
 // subtrees, operator declared trees, operator carve-out inside a framework
 // subtree, local overlays, and deliberately UNANTICIPATED paths (fail-safe).
@@ -250,3 +263,81 @@ describe.skipIf(!hasBash)('bash ↔ TS manifest format-edge parity (§6.1, Decis
    ]);
  });
 });
+
+/**
+ * Failure-mode parity (#791 B2/B3). A bad manifest is the dangerous case: if the
+ * two resolvers DISAGREED on rejection — one throwing while the other quietly
+ * resolved everything to operator — an upgrade could fail loud on one code path
+ * and no-op on the other. So for every malformed/empty/missing manifest, BOTH
+ * must reject: TS throws, and the bash CLI exits non-zero with a stderr message.
+ */
+describe.skipIf(!hasBash)('bash ↔ TS manifest failure-mode parity (§6.1, B2/B3)', () => {
+  const tmp = mkdtempSync(join(tmpdir(), 'mf-failmode-'));
+  afterAll(() => rmSync(tmp, { recursive: true, force: true }));
+
+  let seq = 0;
+  function writeFixture(text: string): string {
+    const file = join(tmp, `bad-manifest-${seq++}.txt`);
+    writeFileSync(file, text);
+    return file;
+  }
+
+  // TS throws AND bash CLI exits non-zero with a non-empty stderr — identical rejection.
+  function expectBothReject(label: string, manifestFile: string): void {
+    expect(() => parseManifestFile(manifestFile), `TS accepted ${label}`).toThrow();
+    const cli = bashCli(manifestFile);
+    expect(cli.status, `bash did not exit non-zero for ${label}`).not.toBe(0);
+    expect(cli.stderr.trim().length, `bash was silent for ${label}`).toBeGreaterThan(0);
+  }
+
+  // Read the file for the TS side the same way loadManifest does, so both halves
+  // see identical bytes (loadManifest keys off a directory, not an arbitrary file).
+  function parseManifestFile(file: string): void {
+    parseManifest(readFileSync(file, 'utf-8'));
+  }
+
+  it('both reject a completely empty manifest', () => {
+    expectBothReject('empty', writeFixture(''));
+  });
+
+  it('both reject a comment/blank-only manifest', () => {
+    expectBothReject('comment-only', writeFixture('# header only\n\n   \n'));
+  });
+
+  it('both reject an operator-only manifest (zero framework paths)', () => {
+    expectBothReject('operator-only', writeFixture('[operator]\nSOUL.md\n*.local.md\n'));
+  });
+
+  it('both reject a [framework] section with no entries', () => {
+    expectBothReject('empty-framework-section', writeFixture('[framework]\n[operator]\nSOUL.md\n'));
+  });
+
+  it('both reject a [framework] entry that normalizes to an empty glob (/)', () => {
+    expectBothReject('root-slash-framework', writeFixture('[framework]\n/\n'));
+  });
+
+  it('both reject a [framework] entry that normalizes to nothing (./)', () => {
+    expectBothReject('dot-slash-framework', writeFixture('[framework]\n./\n[operator]\nSOUL.md\n'));
+  });
+
+  it('both reject [framework] entries that are only bare dot segments', () => {
+    expectBothReject('bare-dot-framework', writeFixture('[framework]\n.\n..\n'));
+  });
+
+  it('both reject an entry that appears before any section header', () => {
+    expectBothReject('entry-before-header', writeFixture('stray.md\n[framework]\nguides/**\n'));
+  });
+
+  it('both reject an unknown section header', () => {
+    expectBothReject('unknown-header', writeFixture('[bogus]\nx\n'));
+  });
+
+  it('both reject a missing manifest file (fail-closed, not empty result)', () => {
+    const missing = join(tmp, 'does-not-exist.txt');
+    // TS: loadManifest would throw a read error; here read-then-parse throws on read.
+    expect(() => parseManifestFile(missing)).toThrow();
+    const cli = bashCli(missing);
+    expect(cli.status).not.toBe(0);
+    expect(cli.stderr.trim().length).toBeGreaterThan(0);
+  });
+});
@@ -9,6 +9,7 @@ import {
  resolveOwnership,
  frameworkSubtreeRoots,
  planPrune,
+  ManifestError,
  type FrameworkManifest,
 } from './manifest.js';

@@ -49,6 +50,69 @@ describe('parseManifest', () => {
  });
 });

+// Fail-closed parsing/loading (#791 B2/B3). An empty, comment-only, operator-only,
+// or unreadable manifest must NOT resolve to "framework owns nothing" (which would
+// make an upgrade a silent no-op). Both must throw so finalizeStage surfaces the
+// abort instead of reporting "Installation complete". The bash reader rejects the
+// same inputs — asserted for parity in manifest-parity.spec.ts.
+describe('parseManifest / loadManifest fail closed on empty or unreadable input', () => {
+  it('throws on a completely empty manifest', () => {
+    expect(() => parseManifest('')).toThrow(/no \[framework\] paths/);
+  });
+
+  it('throws on a comment- and blank-only manifest (no entries at all)', () => {
+    expect(() => parseManifest('# just a header comment\n\n   \n')).toThrow(
+      /no \[framework\] paths/,
+    );
+  });
+
+  it('throws when only an [operator] section is present (zero framework paths)', () => {
+    expect(() => parseManifest('[operator]\nSOUL.md\n*.local.md\n')).toThrow(
+      /no \[framework\] paths/,
+    );
+  });
+
+  it('throws on a [framework] header with no entries beneath it', () => {
+    expect(() => parseManifest('[framework]\n\n[operator]\nSOUL.md\n')).toThrow(
+      /no \[framework\] paths/,
+    );
+  });
+
+  // Degenerate framework entries that pass the length check but normalize to a
+  // glob matching nothing — the manifest would silently protect the whole tree
+  // as operator (#791 blocker-B). Both `/` and `./` normalize to '' ; `.`/`..`
+  // are bare-dot segments.
+  it.each([['/'], ['./'], ['.'], ['..'], ['/\n./']])(
+    'throws when the only [framework] entry (%j) normalizes to nothing usable',
+    (entry) => {
+      expect(() => parseManifest(`[framework]\n${entry}\n`)).toThrow(
+        /no usable \[framework\] paths/,
+      );
+    },
+  );
+
+  it('accepts a wildcard-only framework glob (** is usable)', () => {
+    expect(() => parseManifest('[framework]\n**\n')).not.toThrow();
+  });
+
+  it('loadManifest throws a clear fail-closed error when the manifest file is missing', () => {
+    const missingRoot = fileURLToPath(new URL('./__no_such_framework_root__', import.meta.url));
+    expect(() => loadManifest(missingRoot)).toThrow(/Cannot read framework manifest/);
+  });
+
+  // The distinct error type is what lets finalizeStage tell a pre-sync validation
+  // abort (nothing written) from a mid-sync filesystem failure (#791 blocker-C).
+  it('every fail-closed rejection is a ManifestError', () => {
+    expect(() => parseManifest('')).toThrow(ManifestError);
+    expect(() => parseManifest('[operator]\nSOUL.md\n')).toThrow(ManifestError);
+    expect(() => parseManifest('[framework]\n/\n')).toThrow(ManifestError);
+    expect(() => parseManifest('[bogus]\nx\n')).toThrow(ManifestError);
+    expect(() => parseManifest('stray.md\n[framework]\n')).toThrow(ManifestError);
+    const missingRoot = fileURLToPath(new URL('./__no_such_framework_root__', import.meta.url));
+    expect(() => loadManifest(missingRoot)).toThrow(ManifestError);
+  });
+});
+
 describe('matchGlob', () => {
  it('matches an exact file', () => {
    expect(matchGlob('CONSTITUTION.md', 'CONSTITUTION.md')).toBe(true);
@@ -15,6 +15,19 @@ import { readFileSync } from 'node:fs';

 export type Ownership = 'framework' | 'operator';

+/**
+ * Thrown when the manifest is missing, empty, or malformed. A distinct type lets
+ * callers (e.g. finalizeStage) tell a pre-sync validation abort — where NO files
+ * were touched — apart from a generic mid-sync filesystem failure, and message
+ * the user accurately (#791 blocker-C).
+ */
+export class ManifestError extends Error {
+  constructor(message: string) {
+    super(message);
+    this.name = 'ManifestError';
+  }
+}
+
 export interface FrameworkManifest {
  /** Globs the updater MAY create/overwrite, and prune only when retired. */
  readonly framework: readonly string[];
@@ -49,21 +62,68 @@ export function parseManifest(text: string): FrameworkManifest {
      continue;
    }
    if (line.startsWith('[')) {
-      throw new Error(`Unknown manifest section header on line ${i + 1}: ${line}`);
+      throw new ManifestError(`Unknown manifest section header on line ${i + 1}: ${line}`);
    }

    if (section === null) {
-      throw new Error(`Manifest entry before any [section] header on line ${i + 1}: ${line}`);
+      throw new ManifestError(
+        `Manifest entry before any [section] header on line ${i + 1}: ${line}`,
+      );
    }
    (section === 'framework' ? framework : operator).push(line);
  }

+  // Fail CLOSED on an empty or comment-only manifest. A manifest with zero
+  // framework-owned globs would make resolveOwnership() return `operator` for
+  // every path: an upgrade would prune nothing and refresh nothing — a silent
+  // no-op indistinguishable from success. Refuse loudly instead, mirroring the
+  // bash reader's `manifest_load` guard so both halves reject it identically (#791 B2).
+  if (framework.length === 0) {
+    throw new ManifestError(
+      'Framework manifest defines no [framework] paths — refusing to proceed (empty or malformed manifest).',
+    );
+  }
+
+  // Fail CLOSED on framework entries that normalize to nothing usable. A manifest
+  // like `[framework]\n/` or `[framework]\n./` passes the length check above but
+  // every entry normalizes to an empty (or bare-dot) glob that matches no real
+  // path — so the compiled framework matcher is empty and every path resolves
+  // `operator`: the same silent no-op as an empty manifest. Require at least one
+  // entry with a real, non-dot character (the bash reader applies the identical
+  // `[^/.]` test, so both halves reject these inputs together — #791 blocker-B).
+  if (!framework.some(isUsableFrameworkGlob)) {
+    throw new ManifestError(
+      'Framework manifest defines no usable [framework] paths (every entry is empty or a bare dot segment) — refusing to proceed (malformed manifest).',
+    );
+  }
+
  return { framework, operator };
 }

+/**
+ * A framework glob is usable only if, once normalized, it still contains a
+ * character other than `/` or `.` — i.e. it names a real path segment or a
+ * wildcard. `''`, `/`, `./`, `.`, `..` are all unusable (they compile to a glob
+ * that matches nothing). Kept byte-compatible with the bash `[[ =~ [^/.] ]]`
+ * test so TS and bash accept/reject exactly the same manifests.
+ */
+function isUsableFrameworkGlob(glob: string): boolean {
+  return /[^/.]/.test(normalizeRel(glob));
+}
+
 /** Read and parse the manifest from a framework root directory. */
 export function loadManifest(frameworkRoot: string): FrameworkManifest {
-  const text = readFileSync(`${frameworkRoot}/framework-manifest.txt`, 'utf-8');
+  const file = `${frameworkRoot}/framework-manifest.txt`;
+  let text: string;
+  try {
+    text = readFileSync(file, 'utf-8');
+  } catch (err) {
+    // A missing/unreadable manifest must fail closed with a clear message, not a
+    // raw ENOENT that a caller might mistake for an empty result set (#791 B2/B3).
+    throw new ManifestError(
+      `Cannot read framework manifest at ${file}: ${(err as Error).message} — refusing to sync (fail-closed).`,
+    );
+  }
  return parseManifest(text);
 }

@@ -0,0 +1,136 @@
+/**
+ * Tests for the framework-sync abort messaging (#791 B2 + blocker-C).
+ *
+ * finalizeStage runs `config.syncFramework()` first, inside a try/catch. If the
+ * sync throws, the wizard must:
+ *   1. NEVER fall through to "Installation complete" — the error is re-raised so
+ *      the process exits non-zero (#791 B2).
+ *   2. Classify the failure so recovery advice is accurate (#791 blocker-C):
+ *        - ManifestError  → a PRE-sync validation abort; nothing was written, so
+ *          the message states "no files were changed".
+ *        - any other error → may surface mid-copy, so the message must NOT claim
+ *          nothing changed; it warns the state "may be partially applied".
+ *
+ * We assert on the spinner's stop() message (the user-visible line) and that the
+ * original error is re-thrown unchanged in both cases.
+ */
+
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { mkdtempSync, rmSync } from 'node:fs';
+import { join } from 'node:path';
+import { tmpdir } from 'node:os';
+import type { WizardState } from '../types.js';
+import type { ConfigService } from '../config/config-service.js';
+import { ManifestError } from '../framework/manifest.js';
+
+vi.mock('node:child_process', () => ({
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  spawnSync: vi.fn<any>().mockReturnValue({ status: 0, stdout: '', stderr: '' }),
+}));
+
+vi.mock('../platform/detect.js', () => ({
+  getShellProfilePath: () => null,
+}));
+
+import { finalizeStage } from './finalize.js';
+
+function makeState(mosaicHome: string): WizardState {
+  return {
+    mosaicHome,
+    sourceDir: mosaicHome,
+    mode: 'quick',
+    installAction: 'keep',
+    soul: { agentName: 'TestBot', communicationStyle: 'direct' },
+    user: {},
+    tools: {},
+    runtimes: { detected: [], mcpConfigured: false },
+    selectedSkills: [],
+  };
+}
+
+function buildPrompter() {
+  const stop = vi.fn();
+  const update = vi.fn();
+  const prompter = {
+    intro: vi.fn(),
+    outro: vi.fn(),
+    note: vi.fn(),
+    log: vi.fn(),
+    warn: vi.fn(),
+    text: vi.fn(),
+    confirm: vi.fn(),
+    select: vi.fn(),
+    multiselect: vi.fn(),
+    groupMultiselect: vi.fn(),
+    spinner: vi.fn().mockReturnValue({ update, stop }),
+    separator: vi.fn(),
+  };
+  return { prompter, stop };
+}
+
+function makeConfigService(syncFramework: ConfigService['syncFramework']): ConfigService {
+  return {
+    readSoul: vi.fn().mockResolvedValue({}),
+    readUser: vi.fn().mockResolvedValue({}),
+    readTools: vi.fn().mockResolvedValue({}),
+    writeSoul: vi.fn().mockResolvedValue(undefined),
+    writeUser: vi.fn().mockResolvedValue(undefined),
+    writeTools: vi.fn().mockResolvedValue(undefined),
+    syncFramework,
+    get: vi.fn(),
+    set: vi.fn(),
+    getSection: vi.fn(),
+  } as unknown as ConfigService;
+}
+
+describe('finalizeStage — framework sync abort (#791 B2 + blocker-C)', () => {
+  let tmp: string;
+
+  beforeEach(() => {
+    tmp = mkdtempSync(join(tmpdir(), 'mosaic-sync-abort-'));
+  });
+
+  afterEach(() => {
+    rmSync(tmp, { recursive: true, force: true });
+    vi.clearAllMocks();
+  });
+
+  it('re-throws a ManifestError and reports that no files were changed', async () => {
+    const err = new ManifestError('Framework manifest defines no usable [framework] paths');
+    const { prompter, stop } = buildPrompter();
+    const config = makeConfigService(vi.fn().mockRejectedValue(err));
+
+    await expect(finalizeStage(prompter, makeState(tmp), config)).rejects.toBe(err);
+
+    // The abort message must state nothing was written (pre-sync validation).
+    expect(stop).toHaveBeenCalledWith(expect.stringContaining('no files were changed'));
+    // It must NOT fall through to a success line.
+    expect(stop).not.toHaveBeenCalledWith(expect.stringContaining('Installation complete'));
+  });
+
+  it('re-throws a non-ManifestError and warns the state may be partially applied', async () => {
+    const err = new Error('cp: write error mid-sync (disk full)');
+    const { prompter, stop } = buildPrompter();
+    const config = makeConfigService(vi.fn().mockRejectedValue(err));
+
+    await expect(finalizeStage(prompter, makeState(tmp), config)).rejects.toBe(err);
+
+    // A generic mid-sync failure must NOT claim nothing changed…
+    expect(stop).toHaveBeenCalledWith(expect.stringContaining('may be partially applied'));
+    expect(stop).not.toHaveBeenCalledWith(expect.stringContaining('no files were changed'));
+    expect(stop).not.toHaveBeenCalledWith(expect.stringContaining('Installation complete'));
+  });
+
+  it('does not proceed to config writes when the sync aborts', async () => {
+    const err = new ManifestError('malformed manifest');
+    const { prompter } = buildPrompter();
+    const config = makeConfigService(vi.fn().mockRejectedValue(err));
+
+    await expect(finalizeStage(prompter, makeState(tmp), config)).rejects.toBe(err);
+
+    // writeSoul/writeUser/writeTools are only reached after a successful sync.
+    expect(config.writeSoul).not.toHaveBeenCalled();
+    expect(config.writeUser).not.toHaveBeenCalled();
+    expect(config.writeTools).not.toHaveBeenCalled();
+  });
+});
@@ -6,6 +6,7 @@ import type { WizardPrompter } from '../prompter/interface.js';
 import type { ConfigService } from '../config/config-service.js';
 import type { WizardState } from '../types.js';
 import { getShellProfilePath } from '../platform/detect.js';
+import { ManifestError } from '../framework/manifest.js';

 function linkRuntimeAssets(mosaicHome: string, skipClaudeHooks: boolean): void {
  const script = join(mosaicHome, 'bin', 'mosaic-link-runtime-assets');
@@ -160,7 +161,26 @@ export async function finalizeStage(

  // 1. Sync framework files (before config writes so identity files aren't overwritten)
  spin.update('Syncing framework files...');
-  await config.syncFramework(state.installAction);
+  try {
+    await config.syncFramework(state.installAction);
+  } catch (err) {
+    // Stop the spinner loudly and re-raise so the process exits non-zero — never
+    // fall through to "Installation complete" on an aborted sync (#791 B2).
+    // A ManifestError is a PRE-sync validation abort: the manifest is loaded and
+    // validated before any file is written, so nothing was touched. Any other
+    // error can surface AFTER files were partially copied, so we must NOT claim
+    // "no files were changed" for it — that would misdirect recovery (#791 blocker-C).
+    if (err instanceof ManifestError) {
+      spin.stop(
+        'Framework sync aborted — the framework manifest is missing, empty, or malformed; no files were changed.',
+      );
+    } else {
+      spin.stop(
+        'Framework sync aborted — the update did not complete and may be partially applied; see the error below.',
+      );
+    }
+    throw err;
+  }

  // 2. Write config files (after sync so they aren't overwritten by source templates)
  if (state.installAction !== 'keep') {