fix(cli): refuse destructive fs.rm when registry storagePath isn't <repo>/.gitnexus (#1003 review)

Address @magyargergo's inline review finding on remove.ts:89 and the
sibling vulnerability in clean.ts --all (caught during a pre-commit
safety audit). ~/.gitnexus/registry.json is a user-writable plain-text
file, so a corrupted or hand-edited entry could point storagePath at
the repo root (catastrophic: rm the working tree), an empty string
(→ cwd), a parent dir, or anywhere else. fs.rm(recursive: true,
force: true) on any of those is a runtime disaster.

- New UnsafeStoragePathError + exported assertSafeStoragePath() in
  repo-manager.ts. Pure lexical string check (Windows-case-
  insensitive) asserting entry.storagePath === path.join(entry.path,
  '.gitnexus').
- Guard wired into BOTH destructive registry-trusting sites:
  - remove.ts: exit 1 with actionable hint
  - clean.ts --all: skip the poisoned entry with a warning and
    continue (preserves existing per-repo error tolerance — one bad
    entry doesn't halt the batch)
- clean.ts default path and server/api.ts are safe-by-construction
  (they recompute storagePath from findRepo / getStoragePath rather
  than trusting the registry field).
- 8 unit tests cover the guard (valid, repo-root, parent, empty,
  unrelated, sibling, error payload, Windows case).
- 2 integration tests prove the full CLI path: remove-poisoned exits
  1 without touching the working tree; clean --all with a poisoned
  sibling entry cleans the good entry, skips the bad one, and leaves
  the poisoned repo intact.

gitnexus/src/cli/clean.ts

@@ -6,7 +6,13 @@
  */
 
 import fs from 'fs/promises';
-import { findRepo, unregisterRepo, listRegisteredRepos } from '../storage/repo-manager.js';
+import {
+  findRepo,
+  unregisterRepo,
+  listRegisteredRepos,
+  assertSafeStoragePath,
+  UnsafeStoragePathError,
+} from '../storage/repo-manager.js';
 
 export const cleanCommand = async (options?: { force?: boolean; all?: boolean }) => {
   // --all flag: clean all indexed repos
@@ -27,6 +33,24 @@ export const cleanCommand = async (options?: { force?: boolean; all?: boolean })
 
     const entries = await listRegisteredRepos();
     for (const entry of entries) {
+      // Safety guard (#1003 review — @magyargergo): same rationale as
+      // remove.ts. `~/.gitnexus/registry.json` is user-writable, so a
+      // corrupted or hand-edited entry could point storagePath at the
+      // repo root, an empty string, or anywhere else — and
+      // fs.rm(recursive: true) on any of those would be catastrophic.
+      // Skip poisoned entries without touching disk, but keep going
+      // through the rest of the registry (preserves the existing
+      // per-repo error-tolerance semantics of `clean --all`).
+      try {
+        assertSafeStoragePath(entry);
+      } catch (err) {
+        if (err instanceof UnsafeStoragePathError) {
+          console.error(`Refusing to clean ${entry.name}: ${err.message}`);
+          continue;
+        }
+        throw err;
+      }
+
       try {
         await fs.rm(entry.storagePath, { recursive: true, force: true });
         await unregisterRepo(entry.path);

gitnexus/src/cli/remove.ts

@@ -30,9 +30,11 @@ import fs from 'fs/promises';
 import {
   readRegistry,
   resolveRegistryEntry,
+  assertSafeStoragePath,
   unregisterRepo,
   RegistryNotFoundError,
   RegistryAmbiguousTargetError,
+  UnsafeStoragePathError,
 } from '../storage/repo-manager.js';
 
 export const removeCommand = async (target: string, options?: { force?: boolean }) => {
@@ -72,6 +74,24 @@ export const removeCommand = async (target: string, options?: { force?: boolean
     return;
   }
 
+  // Safety guard (#1003 review — @magyargergo): refuse to proceed if
+  // the registry entry's `storagePath` isn't the canonical
+  // `<entry.path>/.gitnexus` subfolder. `~/.gitnexus/registry.json` is
+  // user-writable, so a corrupted or hand-edited entry could point
+  // storagePath at the repo root, an empty string (→ cwd), a parent
+  // dir, or anywhere else; `fs.rm(recursive: true, force: true)` on
+  // any of those would be a runtime disaster. Bail before touching
+  // disk, with an actionable hint for recovering a broken registry.
+  try {
+    assertSafeStoragePath(entry);
+  } catch (err) {
+    if (err instanceof UnsafeStoragePathError) {
+      console.error(`Error: ${err.message}`);
+      process.exit(1);
+    }
+    throw err;
+  }
+
   // Deletion order: fs.rm first, then unregister. If fs.rm fails mid-way,
   // the registry entry stays so the user can retry. If fs.rm succeeds but
   // unregister throws (e.g. ENOSPC on registry write), the entry becomes

gitnexus/src/storage/repo-manager.ts

@@ -550,6 +550,75 @@ export class RegistryAmbiguousTargetError extends Error {
   }
 }
 
+/**
+ * Thrown by {@link assertSafeStoragePath} when a registry entry's
+ * `storagePath` does NOT point at the expected `<entry.path>/.gitnexus`
+ * subfolder. CLI destructive commands (`remove`, `clean --all`) should
+ * catch this and exit non-zero without deleting anything — the usual
+ * cause is a corrupted or hand-edited `~/.gitnexus/registry.json`, and
+ * proceeding would mean `fs.rm(recursive: true)` on whatever odd path
+ * the entry is pointing at.
+ */
+export class UnsafeStoragePathError extends Error {
+  readonly kind = 'UnsafeStoragePathError' as const;
+  constructor(
+    public readonly entry: RegistryEntry,
+    public readonly expectedStoragePath: string,
+    public readonly actualStoragePath: string,
+  ) {
+    super(
+      `Refusing to remove storage path for safety: expected ` +
+        `"${expectedStoragePath}" under the repo's .gitnexus subfolder, ` +
+        `but the registry entry has "${actualStoragePath}". ` +
+        `This usually means the registry entry is corrupted or was ` +
+        `hand-edited. Delete the entry manually from ~/.gitnexus/registry.json ` +
+        `and re-run analyze.`,
+    );
+    this.name = 'UnsafeStoragePathError';
+  }
+}
+
+/**
+ * Guard rail for destructive CLI paths (`remove` #664,
+ * `clean --all` #258, future MCP `remove` tool): verify that a
+ * registry entry's `storagePath` is the canonical `<repo>/.gitnexus`
+ * subfolder of its `path`. If not, throw {@link UnsafeStoragePathError}
+ * so the caller exits without touching disk.
+ *
+ * Why this exists (#1003 review — @magyargergo):
+ *   - `~/.gitnexus/registry.json` is a plain-text user-writable file.
+ *     A corrupted, hand-edited, or downgrade/upgrade-racing entry
+ *     could plausibly end up with `storagePath === ""` (resolves to
+ *     cwd), `storagePath === path` (the repo root!), `storagePath`
+ *     equal to a parent/sibling of the repo, or simply any arbitrary
+ *     filesystem path.
+ *   - `fs.rm(recursive: true, force: true)` on ANY of those would be
+ *     a runtime disaster — at best delete the user's working tree, at
+ *     worst nuke an unrelated directory tree they happen to own.
+ *   - `clean` (default, cwd-scoped) is safe by construction — it
+ *     re-derives storagePath from `findRepo(cwd)` and never trusts
+ *     the registry field. But `clean --all` DOES iterate the registry
+ *     and trust each entry's stored storagePath (same shape as
+ *     `remove`), so this helper must be wired into that loop too.
+ *   - `server/api.ts` recomputes storagePath from `getStoragePath(entry.path)`
+ *     and so is likewise safe-by-construction.
+ *
+ * Pure string check — does NOT require the paths to exist on disk.
+ * Windows: case-insensitive; POSIX: case-sensitive. Matches the
+ * comparison shape used elsewhere in this module.
+ */
+export const assertSafeStoragePath = (entry: RegistryEntry): void => {
+  const expected = path.join(path.resolve(entry.path), '.gitnexus');
+  const actual = path.resolve(entry.storagePath);
+  const matches =
+    process.platform === 'win32'
+      ? expected.toLowerCase() === actual.toLowerCase()
+      : expected === actual;
+  if (!matches) {
+    throw new UnsafeStoragePathError(entry, expected, actual);
+  }
+};
+
 /**
  * Resolve a user-supplied target string (from `gitnexus remove <target>`
  * or equivalent MCP tool argument) to a single registry entry.

gitnexus/test/integration/cli-e2e.test.ts

@@ -539,6 +539,195 @@ describe('CLI end-to-end', () => {
         fs.rmSync(parentB, { recursive: true, force: true });
       }
     }, 240000); // 4-min outer budget (2 × ~60s analyze + 2 × fast remove)
+
+    it('refuses to proceed when a registry entry points storagePath outside <repo>/.gitnexus (#1003)', () => {
+      // Regression guard for the safety gap flagged by @magyargergo on
+      // PR #1003: `~/.gitnexus/registry.json` is a user-writable JSON
+      // file, so a corrupted or hand-edited entry could point
+      // storagePath at the repo root (catastrophic: rm the working
+      // tree) or at any other arbitrary path. `remove --force` must
+      // refuse to call fs.rm when storagePath isn't the canonical
+      // `<entry.path>/.gitnexus`. We verify:
+      //   1. Exit code 1 with the actionable "registry entry corrupted"
+      //      hint.
+      //   2. The .gitnexus/ storage dir is UNTOUCHED.
+      //   3. The repo itself (entry.path) is UNTOUCHED.
+      //   4. The registry entry is NOT removed (no partial mutation).
+      const gnHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-home-poison-'));
+      const repo = makeMiniRepoCopy('poisoned', 'gn-poison-');
+      const parent = path.dirname(repo);
+
+      try {
+        // Index the repo normally first so the registry has a valid
+        // entry we can then poison.
+        const r1 = runCliWithEnv(
+          ['analyze', '--name', 'poisoned-alias'],
+          repo,
+          { GITNEXUS_HOME: gnHome },
+          60000,
+        );
+        if (r1.status === null) return;
+        expect(r1.status).toBe(0);
+
+        const registryPath = path.join(gnHome, 'registry.json');
+        const original = JSON.parse(fs.readFileSync(registryPath, 'utf-8'));
+        expect(original).toHaveLength(1);
+
+        // Poison the entry: set storagePath to the REPO ROOT itself.
+        // If the guard isn't in place, `remove --force` would call
+        // `fs.rm(repo, {recursive: true, force: true})` and wipe the
+        // entire working tree.
+        const poisoned = [{ ...original[0], storagePath: repo }];
+        fs.writeFileSync(registryPath, JSON.stringify(poisoned, null, 2));
+
+        // Sanity: storage dir and working tree both still exist.
+        expect(fs.existsSync(path.join(repo, '.gitnexus'))).toBe(true);
+        expect(fs.existsSync(repo)).toBe(true);
+        expect(fs.existsSync(path.join(repo, '.git'))).toBe(true);
+
+        // Attempt the remove — must FAIL without deleting anything.
+        const r2 = runCliWithEnv(
+          ['remove', 'poisoned-alias', '--force'],
+          parent,
+          { GITNEXUS_HOME: gnHome },
+          15000,
+        );
+        if (r2.status === null) return;
+
+        expect(
+          r2.status,
+          [`remove should have exited 1`, `stdout: ${r2.stdout}`, `stderr: ${r2.stderr}`].join(
+            '\n',
+          ),
+        ).toBe(1);
+        const r2Output = `${r2.stdout}${r2.stderr}`;
+        // Must surface the actionable "registry corrupted" hint, not
+        // just a raw fs.rm error.
+        expect(r2Output).toMatch(/Refusing to remove/i);
+        expect(r2Output).toMatch(/registry\.json/i);
+
+        // Repo + .gitnexus dir + .git dir must all still exist — the
+        // guard aborts BEFORE fs.rm. This is the whole point of the
+        // test: the working tree is not allowed to disappear.
+        expect(fs.existsSync(repo), 'repo working tree must survive').toBe(true);
+        expect(fs.existsSync(path.join(repo, '.gitnexus')), 'storage dir must survive').toBe(true);
+        expect(fs.existsSync(path.join(repo, '.git')), '.git must survive').toBe(true);
+
+        // Registry unchanged — no partial mutation.
+        const afterRegistry = JSON.parse(fs.readFileSync(registryPath, 'utf-8'));
+        expect(afterRegistry).toHaveLength(1);
+        expect(afterRegistry[0].storagePath).toBe(repo); // still poisoned (we did that)
+      } finally {
+        fs.rmSync(gnHome, { recursive: true, force: true });
+        fs.rmSync(parent, { recursive: true, force: true });
+      }
+    }, 120000); // 2-min budget (1 × ~60s analyze + 1 × fast remove-refused)
+  });
+
+  // ─── clean --all: same safety guard applies (#1003 review) ───────
+  //
+  // The `clean --all` path iterates over the registry and calls
+  // `fs.rm(entry.storagePath)` — identical trust-the-registry pattern
+  // as `remove` had before the guard. A poisoned entry must be SKIPPED
+  // (not aborted), so clean --all preserves its existing per-repo
+  // error-tolerance semantics: one bad entry does not halt cleanup of
+  // the rest. We verify:
+  //   1. The poisoned entry is NOT deleted (working tree + .gitnexus
+  //      survive), and the CLI prints a "Refusing to clean" message.
+  //   2. The poisoned entry is left in the registry (nothing was
+  //      mutated for it).
+  //   3. A co-existing well-formed entry IS still cleaned (both its
+  //      .gitnexus dir AND its registry entry are gone).
+  describe('clean --all with a poisoned registry entry (#1003)', () => {
+    it('skips poisoned entries, cleans valid ones, never deletes the working tree', () => {
+      const gnHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-home-clean-poison-'));
+      const repoBad = makeMiniRepoCopy('bad-repo', 'gn-clean-bad-');
+      const repoGood = makeMiniRepoCopy('good-repo', 'gn-clean-good-');
+      const parentBad = path.dirname(repoBad);
+      const parentGood = path.dirname(repoGood);
+
+      try {
+        // Analyze both so the registry has two well-formed entries.
+        for (const [repo, alias] of [
+          [repoBad, 'bad-alias'],
+          [repoGood, 'good-alias'],
+        ] as const) {
+          const r = runCliWithEnv(
+            ['analyze', '--name', alias],
+            repo,
+            { GITNEXUS_HOME: gnHome },
+            60000,
+          );
+          if (r.status === null) return;
+          expect(r.status, `analyze ${alias} exited ${r.status}: ${r.stdout}${r.stderr}`).toBe(0);
+        }
+
+        const registryPath = path.join(gnHome, 'registry.json');
+        const original = JSON.parse(fs.readFileSync(registryPath, 'utf-8'));
+        expect(original).toHaveLength(2);
+
+        // Poison the 'bad-alias' entry by pointing its storagePath at
+        // the repo root itself. If the guard isn't wired into the
+        // clean --all loop, `clean --all --force` would fs.rm the
+        // working tree.
+        const poisoned = original.map((e: { name: string; storagePath: string; path: string }) =>
+          e.name === 'bad-alias' ? { ...e, storagePath: repoBad } : e,
+        );
+        fs.writeFileSync(registryPath, JSON.stringify(poisoned, null, 2));
+
+        // Sanity: both working trees and .gitnexus dirs still exist.
+        expect(fs.existsSync(repoBad)).toBe(true);
+        expect(fs.existsSync(path.join(repoBad, '.gitnexus'))).toBe(true);
+        expect(fs.existsSync(path.join(repoBad, '.git'))).toBe(true);
+        expect(fs.existsSync(path.join(repoGood, '.gitnexus'))).toBe(true);
+
+        // clean --all --force from a neutral cwd (parentBad), so the
+        // command isn't "inside" either repo.
+        const r = runCliWithEnv(
+          ['clean', '--all', '--force'],
+          parentBad,
+          { GITNEXUS_HOME: gnHome },
+          30000,
+        );
+        if (r.status === null) return;
+
+        // clean --all's per-entry error handling always exits 0 at
+        // the end (it only logs per-repo failures). The important
+        // assertions are on side effects, not the exit code.
+        const output = `${r.stdout}${r.stderr}`;
+        expect(output).toMatch(/Refusing to clean/i);
+        expect(output).toMatch(/bad-alias/);
+
+        // Poisoned repo: working tree + .gitnexus + .git all SURVIVE.
+        expect(fs.existsSync(repoBad), 'poisoned repo working tree must survive').toBe(true);
+        expect(
+          fs.existsSync(path.join(repoBad, '.gitnexus')),
+          'poisoned repo .gitnexus must survive (guard refused to rm repo root)',
+        ).toBe(true);
+        expect(fs.existsSync(path.join(repoBad, '.git')), '.git must survive').toBe(true);
+
+        // Good repo: its .gitnexus IS gone (cleanup succeeded despite
+        // the poisoned sibling entry — per-entry error tolerance is
+        // preserved).
+        expect(
+          fs.existsSync(path.join(repoGood, '.gitnexus')),
+          'good repo .gitnexus should be cleaned',
+        ).toBe(false);
+        // But the good repo's working tree stays (clean never touches
+        // anything outside .gitnexus).
+        expect(fs.existsSync(repoGood), 'good repo working tree must survive').toBe(true);
+
+        // Registry post-state: poisoned entry still present (skipped,
+        // not mutated); good entry unregistered.
+        const afterRegistry = JSON.parse(fs.readFileSync(registryPath, 'utf-8'));
+        expect(afterRegistry).toHaveLength(1);
+        expect(afterRegistry[0].name).toBe('bad-alias');
+      } finally {
+        fs.rmSync(gnHome, { recursive: true, force: true });
+        fs.rmSync(parentBad, { recursive: true, force: true });
+        fs.rmSync(parentGood, { recursive: true, force: true });
+      }
+    }, 240000); // 4-min budget (2 × ~60s analyze + 1 × fast clean --all)
   });
 
   describe('unhappy path', () => {

gitnexus/test/unit/repo-manager.test.ts

@@ -17,9 +17,11 @@ import {
   listRegisteredRepos,
   resolveRegistryEntry,
   canonicalizePath,
+  assertSafeStoragePath,
   RegistryNameCollisionError,
   RegistryNotFoundError,
   RegistryAmbiguousTargetError,
+  UnsafeStoragePathError,
   type RegistryEntry,
   type RepoMeta,
 } from '../../src/storage/repo-manager.js';
@@ -690,3 +692,102 @@ describe('resolveRegistryEntry backward-compat with non-canonical stored paths (
     expect(hit).toBe(entries[0]);
   });
 });
+
+// ─── assertSafeStoragePath (#1003 review — @magyargergo) ─────────────
+//
+// Guard rail against destroying more than the `.gitnexus/` subfolder.
+// `~/.gitnexus/registry.json` is user-writable plain text, so a
+// corrupted or hand-edited entry could put storagePath anywhere.
+// These tests use synthetic `RegistryEntry` fixtures (no disk I/O)
+// because the guard is a pure string check — it must not depend on
+// the paths existing.
+
+describe('assertSafeStoragePath (#1003)', () => {
+  const prefix = process.platform === 'win32' ? 'D:\\' : '/tmp/';
+  const repoPath = `${prefix}projects${path.sep}my-repo`;
+  const base: Omit<RegistryEntry, 'storagePath'> = {
+    name: 'my-repo',
+    path: repoPath,
+    indexedAt: '2026-04-21T00:00:00.000Z',
+    lastCommit: 'deadbee',
+  };
+
+  it('accepts the canonical <repo>/.gitnexus storage path', () => {
+    const entry: RegistryEntry = {
+      ...base,
+      storagePath: path.join(repoPath, '.gitnexus'),
+    };
+    expect(() => assertSafeStoragePath(entry)).not.toThrow();
+  });
+
+  it('rejects when storagePath equals the repo path itself (would delete the code)', () => {
+    const entry: RegistryEntry = {
+      ...base,
+      storagePath: repoPath, // catastrophic: rm the working tree
+    };
+    expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError);
+  });
+
+  it('rejects when storagePath is a parent of the repo path', () => {
+    const entry: RegistryEntry = {
+      ...base,
+      storagePath: path.dirname(repoPath), // also catastrophic
+    };
+    expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError);
+  });
+
+  it('rejects when storagePath is empty (path.resolve falls back to cwd)', () => {
+    const entry: RegistryEntry = {
+      ...base,
+      storagePath: '', // path.resolve('') === process.cwd() — would rm cwd
+    };
+    expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError);
+  });
+
+  it('rejects when storagePath points somewhere totally unrelated', () => {
+    const entry: RegistryEntry = {
+      ...base,
+      storagePath: `${prefix}some${path.sep}other${path.sep}place`,
+    };
+    expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError);
+  });
+
+  it('rejects when storagePath is a sibling .gitnexus (right basename, wrong parent)', () => {
+    const entry: RegistryEntry = {
+      ...base,
+      storagePath: path.join(`${prefix}different${path.sep}repo`, '.gitnexus'),
+    };
+    expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError);
+  });
+
+  it('UnsafeStoragePathError carries the original entry + expected + actual paths', () => {
+    const entry: RegistryEntry = {
+      ...base,
+      storagePath: `${prefix}evil${path.sep}path`,
+    };
+    try {
+      assertSafeStoragePath(entry);
+    } catch (e) {
+      expect(e).toBeInstanceOf(UnsafeStoragePathError);
+      const err = e as UnsafeStoragePathError;
+      expect(err.kind).toBe('UnsafeStoragePathError');
+      expect(err.entry).toBe(entry);
+      // Expected path is the canonical `<repo>/.gitnexus`.
+      expect(err.expectedStoragePath).toBe(path.join(path.resolve(repoPath), '.gitnexus'));
+      // Actual path is the corrupted value (resolved).
+      expect(err.actualStoragePath).toBe(path.resolve(entry.storagePath));
+      // Message must suggest the recovery action.
+      expect(err.message).toContain('registry.json');
+    }
+  });
+
+  it('Windows: storagePath match is case-insensitive to match register/unregister semantics', () => {
+    if (process.platform !== 'win32') return;
+    const entry: RegistryEntry = {
+      ...base,
+      storagePath: path.join(repoPath.toUpperCase(), '.GITNEXUS'),
+    };
+    // Should accept because Windows paths are case-insensitive.
+    expect(() => assertSafeStoragePath(entry)).not.toThrow();
+  });
+});