fix: pre-landing review fixes — review-army findings hardened

Specialist review (testing, maintainability, security, performance,
data-migration) findings, each verified against code before fixing:

- legacy remove: preserve malformed/foreign entries (hooks absent, non-array,
  or pre-existing empty) — only entries THIS pass emptied are dropped
- add-event: never tag a mixed entry (old gstack versions in sibling
  worktrees treat tags as entry-level ownership and would destroy the user's
  co-located items); tag only single-item entries; prune-stale drops tags
  from mixed entries for the same reason
- prune-stale: within-entry twin collapse (two dead copies of one hook
  re-pointed to the same canonical command no longer double-fire); command
  quoting hardened via gsQuoteCmd (escapes \\ " $ backtick; gsStripWrap
  unescapes so identity round-trips); NUL bytes in the dedupe key replaced
  with a JSON.stringify key (bash silently dropped the NULs, degrading the
  separator; the file also read as binary to tooling)
- gsIsAlive: only provable absence (ENOENT/ENOTDIR) counts as dead —
  EACCES/EIO/unmounted volumes no longer prune (one-way-ratchet guard)
- gsWriteIfChanged: preserves the live settings.json mode across rewrites
  (a user-tightened 0600 carrying API keys was silently broadened to 0644);
  fresh files start 0600; backups rotate (keep 10)
- remove-source: command-less items default to foreign (gstack only writes
  type:command items); single-item stray claim requires a command
- rollback: pointer target must be a sibling settings.json.bak.* file
- uninstall + setup --no-team + SessionStart registration: stderr stays
  attached — a lock give-up or fail-closed parse during TEARDOWN must be
  visible ("the next setup retries" does not apply after uninstall)
- setup: team-mode banner no longer claims an auto-update hook when
  registration was skipped; heal log documents the rollback-pointer caveat;
  SESSION_UPDATE_CMD quoting mirrors gsQuoteCmd; lock constants named
- list-sources: corrupt settings.json reports to stderr instead of silently
  printing nothing (setup guards must not misread corrupt as no-hooks)
- tests: 10 new pins (malformed-entry preservation, mixed no-tag, twin
  collapse, 0600 mode, metachar escaping round-trip, backup rotation,
  rollback pointer refusal, held-lock uninstall warning, matcher-drift
  tripwire, ownership negatives)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

test/gstack-settings-hook-schema-aware.test.ts

@@ -495,6 +495,120 @@ describe('legacy remove: per-item (regression)', () => {
   });
 });
 
+describe('review-army hardening (specialist findings)', () => {
+  test('legacy remove preserves malformed/foreign entries it never touched', () => {
+    fs.writeFileSync(settingsFile, JSON.stringify({
+      hooks: {
+        SessionStart: [
+          { comment: 'no hooks array at all' },
+          { hooks: 'not-an-array' },
+          { hooks: [] },
+          { hooks: [{ type: 'command', command: '/x/bin/gstack-session-update' }] },
+        ],
+      },
+    }, null, 2));
+    runIso(['remove', '/x/bin/gstack-session-update']);
+    const s = settings();
+    // Only the entry we emptied is gone; the three malformed/foreign ones stay.
+    expect(s.hooks.SessionStart).toHaveLength(3);
+  });
+
+  test('add-event never tags a mixed entry (old-version ratchet guard)', () => {
+    const canon = mkCanon(tmpDir);
+    fs.writeFileSync(settingsFile, JSON.stringify({
+      hooks: {
+        PostToolUse: [{
+          matcher: AUQ_MATCHER,
+          _gstack_source: 'plan-tune-cathedral',
+          hooks: [
+            { type: 'command', command: '/Users/me/my-own-hook' },
+            { type: 'command', command: '/dead/wt/hosts/claude/hooks/question-log-hook' },
+          ],
+        }],
+      },
+    }, null, 2));
+    runIso([
+      'add-event', '--event', 'PostToolUse', '--matcher', AUQ_MATCHER,
+      '--command', `${canon}/hosts/claude/hooks/question-log-hook`,
+      '--source', 'plan-tune-cathedral',
+    ]);
+    const s = settings();
+    expect(s.hooks.PostToolUse).toHaveLength(1);
+    const e = s.hooks.PostToolUse[0];
+    expect(e.hooks).toHaveLength(2);
+    expect(e.hooks[0].command).toBe('/Users/me/my-own-hook');
+    // A tag on a mixed entry hands old-version remove-source permission to
+    // destroy the user's item — it must be gone.
+    expect(e._gstack_source).toBeUndefined();
+  });
+
+  test('two dead twins of one hook in ONE entry collapse to a single item after --repoint', () => {
+    const canon = mkCanon(tmpDir);
+    fs.writeFileSync(settingsFile, JSON.stringify({
+      hooks: {
+        PostToolUse: [{
+          matcher: AUQ_MATCHER,
+          hooks: [
+            { type: 'command', command: '/dead/a/hosts/claude/hooks/question-log-hook' },
+            { type: 'command', command: '/dead/b/hosts/claude/hooks/question-log-hook' },
+          ],
+        }],
+      },
+    }, null, 2));
+    runIso(['prune-stale', '--repoint', canon]);
+    const items = settings().hooks.PostToolUse[0].hooks;
+    expect(items).toHaveLength(1);   // pre-fix: two identical items → hook fires twice per event
+    expect(items[0].command).toBe(`${canon}/hosts/claude/hooks/question-log-hook`);
+  });
+
+  test('a 0600 settings.json keeps its mode across mutations (API keys stay private)', () => {
+    fs.writeFileSync(settingsFile, JSON.stringify({ env: { SECRET: 'x' } }, null, 2));
+    fs.chmodSync(settingsFile, 0o600);
+    runIso(['add-event', '--event', 'Stop', '--command', '/x/hosts/claude/hooks/timeline-stop-hook', '--source', 'gstack-timeline-stop']);
+    const mode = fs.statSync(settingsFile).mode & 0o777;
+    expect(mode).toBe(0o600);
+  });
+
+  test('a canonical root containing $ is escaped in the registered command', () => {
+    const trickyBase = path.join(tmpDir, 'weird$dir');
+    fs.mkdirSync(trickyBase, { recursive: true });
+    const canon = mkCanon(trickyBase);
+    fs.writeFileSync(settingsFile, JSON.stringify({
+      hooks: { Stop: [hookEntry('/dead/wt/hosts/claude/hooks/timeline-stop-hook')] },
+    }, null, 2));
+    runIso(['prune-stale', '--repoint', canon]);
+    const cmd = settings().hooks.Stop[0].hooks[0].command;
+    expect(cmd.startsWith('"')).toBe(true);
+    expect(cmd).toContain('\\$');   // $ neutralized — shell must not expand it at hook-fire time
+    // Idempotent: the escaped command is still recognized as ours.
+    const before = fs.readFileSync(settingsFile, 'utf-8');
+    const r2 = runIso(['prune-stale', '--repoint', canon]);
+    expect(r2.stdout).toMatch(/removed 0 gstack hook entries \(repointed 0\)/);
+    expect(fs.readFileSync(settingsFile, 'utf-8')).toBe(before);
+  });
+
+  test('backups rotate: at most 10 .bak files survive repeated mutations', () => {
+    for (let i = 0; i < 13; i++) {
+      runIso(['add-event', '--event', 'Stop', '--command', `/x/hosts/claude/hooks/timeline-stop-hook-${i}`, '--source', 'gstack-timeline-stop']);
+    }
+    expect(backups().length).toBeLessThanOrEqual(10);
+    // The rollback pointer still resolves to an existing backup.
+    const latest = fs.readFileSync(path.join(tmpDir, 'settings.json.bak-latest'), 'utf-8').trim();
+    expect(fs.existsSync(latest)).toBe(true);
+  });
+
+  test('rollback refuses a pointer that names a non-backup file', () => {
+    fs.writeFileSync(settingsFile, JSON.stringify({ a: 1 }, null, 2));
+    const evil = path.join(tmpDir, 'evil.json');
+    fs.writeFileSync(evil, JSON.stringify({ hooks: { Stop: [{ hooks: [{ type: 'command', command: '/evil' }] }] } }));
+    fs.writeFileSync(path.join(tmpDir, 'settings.json.bak-latest'), evil + '\n');
+    const r = runIso(['rollback']);
+    expect(r.exitCode).not.toBe(0);
+    expect(r.stderr).toMatch(/refusing/);
+    expect(settings().a).toBe(1);
+  });
+});
+
 describe('ownership negatives', () => {
   test('owned basename+relpath under the WRONG matcher stays foreign (not re-pointed)', () => {
     const canon = mkCanon(tmpDir);

test/setup-hook-canonical-paths.test.ts

@@ -186,3 +186,31 @@ describe('the defect-class warning is written down where the next author will se
     expect(setupSrc).toMatch(/NEVER register .*SOURCE_GSTACK_DIR.*hook paths/);
   });
 });
+
+describe('matcher-literal drift tripwire (review-army)', () => {
+  test("every --matcher literal in setup equals its KNOWN_HOOKS row's matcher", () => {
+    // gsOwnedRow requires an EXACT matcher match — if setup's registration
+    // matcher drifts from the table row, identity re-pointing/pruning silently
+    // stops recognizing the hook and the phantom-duplicate class returns.
+    const rowMatcher = (name: string) => {
+      const rowStart = hookBinSrc.indexOf(`"${name}":`);
+      expect(rowStart).toBeGreaterThan(-1);
+      const row = hookBinSrc.slice(rowStart, hookBinSrc.indexOf('}', rowStart));
+      return row.match(/matcher: "([^"]*)"/)![1];
+    };
+    const pairs: Array<[string, string]> = [
+      ['question-log-hook', '(AskUserQuestion|mcp__.*__AskUserQuestion)'],
+      ['question-preference-hook', '(AskUserQuestion|mcp__.*__AskUserQuestion)'],
+      ['auq-error-fallback-hook', '(AskUserQuestion|mcp__.*__AskUserQuestion)'],
+    ];
+    for (const [name, expected] of pairs) {
+      expect(rowMatcher(name)).toBe(expected);
+    }
+    // And setup registers those hooks with exactly that matcher literal.
+    const matcherLiterals = [...setupSrc.matchAll(/--matcher '([^']+)'/g)].map((m) => m[1]);
+    expect(matcherLiterals.length).toBeGreaterThanOrEqual(3);
+    for (const lit of matcherLiterals) {
+      expect(lit).toBe('(AskUserQuestion|mcp__.*__AskUserQuestion)');
+    }
+  });
+});

test/uninstall.test.ts

@@ -298,3 +298,54 @@ describe('hook cleanup runs before the install root is deleted', () => {
     }
   });
 });
+
+describe('hook cleanup under lock contention is loud, never silent (review-army)', () => {
+  test('a held foreign lock during uninstall surfaces the give-up warning on stderr', () => {
+    const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-uninstall-lock-'));
+    try {
+      const mockHome = path.join(tmp, 'home');
+      const installRoot = path.join(mockHome, '.claude', 'skills', 'gstack');
+      const installBin = path.join(installRoot, 'bin');
+      fs.mkdirSync(installBin, { recursive: true });
+      for (const b of ['gstack-uninstall', 'gstack-settings-hook', 'gstack-session-update', 'gstack-config']) {
+        const dst = path.join(installBin, b);
+        fs.copyFileSync(path.join(ROOT, 'bin', b), dst);
+        fs.chmodSync(dst, 0o755);
+      }
+      const settingsFile = path.join(mockHome, '.claude', 'settings.json');
+      fs.writeFileSync(settingsFile, JSON.stringify({
+        hooks: {
+          Stop: [{
+            _gstack_source: 'gstack-timeline-stop',
+            hooks: [{ type: 'command', command: `${installRoot}/hosts/claude/hooks/timeline-stop-hook` }],
+          }],
+        },
+      }, null, 2));
+      fs.mkdirSync(path.join(mockHome, '.gstack'), { recursive: true });
+      // A fresh foreign lock: pre-fix, every cleanup call silently skipped and
+      // uninstall reported clean while orphaning the hooks forever.
+      fs.mkdirSync(`${settingsFile}.lock`);
+      fs.writeFileSync(path.join(`${settingsFile}.lock`, 'owner'), 'another-live-process');
+
+      const result = spawnSync('bash', [path.join(installBin, 'gstack-uninstall'), '--force', '--keep-state'], {
+        stdio: 'pipe',
+        env: {
+          ...process.env,
+          HOME: mockHome,
+          GSTACK_SETTINGS_FILE: settingsFile,
+          GSTACK_STATE_ROOT: path.join(mockHome, '.gstack'),
+          GSTACK_SETTINGS_LOCK_TIMEOUT_MS: '300',
+        },
+        cwd: tmp,
+      });
+      const stderr = result.stderr.toString();
+      const s = JSON.parse(fs.readFileSync(settingsFile, 'utf-8'));
+      const cleaned = s.hooks?.Stop === undefined;
+      // Either the sweep still happened, or the user SEES why it didn't.
+      expect(cleaned || /could not acquire lock/.test(stderr)).toBe(true);
+      expect(/could not acquire lock/.test(stderr)).toBe(true);
+    } finally {
+      fs.rmSync(tmp, { recursive: true, force: true });
+    }
+  });
+});