v1.68.1.0 fix: phantom AskUserQuestion hooks — canonical-only registration + self-healing settings.json (#2631)

* fix(settings-hook): KNOWN_HOOKS identity healer — per-item ownership, mutation lock, fail-closed parse Claude Code strips the unknown _gstack_source key when it rewrites settings.json, so tag-based dedupe degraded to exact-command equality and every Conductor worktree's setup appended a fresh hook entry; deleted worktrees left dead hooks erroring on every AskUserQuestion fire. - KNOWN_HOOKS identity table (shared JS prelude, single source of truth): ownership is intrinsic and PER HOOK ITEM — basename + relpath suffix + event (+ matcher where defined). Tags never claim foreign items. - New `prune-stale [--repoint <root>] [--all]`: prune dead gstack items, re-point survivors at the stable install (tag restore from the table), exact-duplicate collapse, uninstall/no-team identity sweep. Explicit plan_tune_hooks:no is honored (dead pruned, live never re-pointed). - add-event / remove-source become item-aware: replace/remove only the owned item; a user's co-located hook in the same entry is never collateral. - Mutation safety: mkdir lock with owner token, ownership-checked release, atomic stale takeover; per-process-unique tmp + backup names; backup-on-change everywhere; fail-closed on parse failure (a corrupt settings.json is never overwritten — previously catch{} clobbered it); locked atomic rollback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(gstack-config): `has <key>` — key-presence provenance through STATE_DIR resolution `get` returns the DEFAULTS value for absent keys, so callers that need to know whether the USER decided something (vs inherited a default) had no correct primitive — setup's consent logic was about to grep a hardcoded ~/.gstack/config.yaml, which misclassifies under GSTACK_STATE_ROOT / GSTACK_HOME / GSTACK_STATE_DIR overrides. `has` exits 0 iff the key is literally present in the resolved config file, with the same C-locale key validation as get/set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(setup): canonical-only hook registration, heal-first, PT_EXPLICIT consent provenance Three root causes of the phantom-AskUserQuestion-hooks class, all in the registration path: - Bug A: the Conductor auto-opt-in upgraded PT_DECISION "prompt" -> "yes" even when "prompt" was dev-setup's EXPLICIT --plan-tune-hooks=prompt pin, so every new Conductor workspace installed hooks. PT_EXPLICIT (flag/env/ config-key-presence via `gstack-config has`) now gates the auto-opt-in to the true silent fall-through. - Bug B: hook commands were baked from $SOURCE_GSTACK_DIR (`pwd -P` of the running tree — ephemeral for worktrees). Registration is now CANONICAL-ONLY via _hook_command_path (${CLAUDE_CONFIG_DIR:-$HOME/.claude}/skills/gstack); missing canonical hook = skip + log, never a baked tree path. SessionStart moves to schema-aware add-event under its identity source; whitespace paths are quoted. - Bug C: nothing ever pruned, and dead tagged entries blocked the "already installed" guards forever. Setup now heals FIRST on every run (prune-stale --repoint at the stable install), surfaces a one-line summary only when something changed, surfaces the plan_tune_hooks:no-vs-live-hooks contradiction, and --no-team tears down all three sources plus an identity sweep for untagged strays. dev-setup's no-mutation guarantee gains its stated repair exception (prune dead / re-point existing, never ADD). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(uninstall): run hook cleanup BEFORE install-root deletion + full identity sweep SETTINGS_HOOK resolves via $(dirname "$0") INSIDE the install root, but the cleanup ran after `rm -rf ~/.claude/skills/gstack` — a real global uninstall (running the installed copy) silently no-op'd and orphaned every hook entry. Tests masked it by running the uninstaller from the repo checkout. The relocated block also removes the auq-error-fallback source (registered by setup, previously never torn down) and finishes with a prune-stale --all identity sweep so untagged strays (Claude Code strips _gstack_source) go too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: phantom-hooks heal coverage — incident facsimile, per-item safety, lock, canonical tripwires - gstack-settings-hook-schema-aware: 16 new cases — identity re-point (tag restore), foreign-basename rejection, mixed-entry per-item safety for add-event/remove-source/--all, prune-stale modes incl. bash-prefix + Windows-backslash + spaced-path idempotence, duplicate collapse preferring the tagged twin, plan_tune_hooks:no split, backup-on-change no-churn, fail-closed corrupt-JSON for every mutator, stale-lock takeover, fresh-foreign-lock skip, two-writer concurrency smoke, and an INCIDENT FACSIMILE replaying the exact 2026-08-17 production damage (6/3/2 entries, mixed tags, live-ephemeral Stop) healing to 2/1/1 canonical. - NEW setup-hook-canonical-paths: static tripwires — canonical-only resolver (no $SOURCE_GSTACK_DIR anywhere in it), heal-before-guards ordering, unsuppressed heal output, ${VAR:-0} counter idiom, shared-prelude concatenation at every bun call site, KNOWN_HOOKS completeness vs setup's registrations, uninstall cleanup-before-deletion ordering, defect-class warning present. - setup-plan-tune-hooks-noninteractive: PT_EXPLICIT pins + `gstack-config has` provenance + has-subcommand behavior (env-resolution, malformed keys). - auq-error-fallback-hook: registration + both-teardown wiring (previously untested). - uninstall: behavioral ordering test running the INSTALLED copy from inside the root it deletes. - setup-windows-fallback / gstack-config-key-locale: pins updated for the new HOOK_CMD shape and the third C-locale validator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(test): banner-tripwire exec used JSON.stringify as shell quoting — vacuous pass + stray artifact JSON escaping is not shell escaping. Interpolating JSON.stringify(script) into `bash -c ${...}` left every JSON "\n" as a literal backslash-n inside shell double quotes, collapsing the extracted release-body tripwire block onto one line: `then\n` parsed as the command word `thenn`, and `>&2\nelse\n` parsed as the redirect `>&2nelsen` — so every full-suite run littered a `2nelsen` file (containing "bash: thenn: command not found") in the repo root, and the test's single not-contains assertion passed VACUOUSLY because all output had been redirected into that file. The "and it actually fires" functional check never verified anything. Fix: pass the script as an argv element (spawnSync array form) and assert both branches for real — ABORT case must print the leak message to stderr, clean case must print "banner tripwire clean" to stdout. Verified: `bun test test/binding-template-drift.test.ts` previously created the artifact deterministically; the full free suite now runs artifact-free. The other shell-interpolation sites (evidence, schema-aware concurrency, empty-find-fallthrough, branch-slug-hygiene) already use correct quoting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: regression pin for legacy remove mixed-entry filtering + ownership negatives Coverage-audit iron rule: the rewritten legacy `remove` action filters per-item (pre-v1.67.2 it dropped the whole entry, destroying a user's co-located SessionStart hook) — modified existing behavior, previously untested. Also pins two ownership negatives: an owned basename+relpath under the WRONG matcher stays foreign, and prune-stale on an absent settings file exits 0 with removed 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * 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> * fix: red-team findings — verify-gate identity, single quoting authority, Windows paths Red-team pass over the hardened diff (several findings empirically verified by the reviewer before reporting): - KNOWN_HOOKS gains the sixth identity: gstack-verify-gate (README-documented opt-in Stop hook). A tag-stripped verify-gate entry previously survived prune-stale --all and errored at the end of EVERY turn after uninstall deleted the install root — the exact phantom-hook class this branch fixes. Uninstall also sweeps its tagged form. - add-event is now the single quoting authority: every registered command is normalized through the same gsQuoteCmd/gsStripWrap round-trip the healer uses. Pre-fix, only SessionStart got caller-side quoting — a spaced/metachar canonical root registered broken plan-tune/AUQ/timeline hooks that the very next heal rewrote (the codebase disagreed with its own registrations). - Windows: MSYS-form paths (/c/Users/...) are drive-translated for fs checks only (gsWinPath) — native bun resolved them drive-relative, so the heal judged every LIVE Windows hook dead and pruned it. The three AskUserQuestion hooks and the Stop hook now also get the mandatory 'bash ' prefix on Windows (previously only SessionStart did; extensionless bash shims otherwise hit the file-association dialog). - CANONICAL_GSTACK_ROOT falls back to $HOME/.claude/skills/gstack when a CLAUDE_CONFIG_DIR-derived root was never installed (the installer hardcodes the home path — split-brain left such users permanently hookless). - prune-stale preserves foreign entries that STARTED empty (they were silently deleted, uncounted, on every heal). - The timeline Stop registration and its list-sources guard join the zero-silent-mutations contract (stderr attached). Tests: verify-gate tag-stripped heal+sweep, started-empty preservation, add-event quoting-authority round-trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: bump version and changelog (v1.68.1.0) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: update project documentation for v1.68.1.0 README: document canonical-only hook registration + the prune-stale self-heal in the setup hooks section; expand the manual-uninstall note to cover every gstack hook identity, not just timeline-stop-hook. CONTRIBUTING: record PT_EXPLICIT provenance (Conductor auto-opt-in fires only on the true silent fall-through) and the heal-first repair exception in the dev-setup paragraph. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(settings-hook): fail-loud hardening — gsMain umbrella, lock exit 5, prototype-safe ownership bun in -e mode swallows uncaught exceptions thrown after a require() and exits 0 (verified on 1.3.13; uncaughtException handlers never fire either), so any runtime throw in a mutator was a SILENT SUCCESS. Every script body now runs inside a gsMain try/catch that prints "internal error ... refusing to mutate" and exits 4. Also: lock give-up now exits 5 instead of 0 (callers must not report a skipped mutation as registered); basename lookup uses hasOwnProperty so a foreign hook named "toString"/"constructor" can't resolve to an inherited Object.prototype member and abort the sweep; ownership-checked release also clears an empty/missing owner file; backup rotation sorts by mtime, not name; Windows-only backslash normalization (a legal Unix path containing a backslash is no longer rewritten); GSTACK_SWEEP_EXCLUDE_SOURCES lets a sweep spare named sources; lock tradeoffs documented at the lock helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(setup): honest hook-registration reporting + verify-gate sweep exclusion _install_plan_tune_hooks now propagates per-add-event failures (lock contention exits 5, fail-closed settings errors exit 3) and both caller sites branch on it: success logs the installed message, failure logs a visible "NOT registered — re-run ./setup" warning instead of claiming success for a mutation that never happened. --no-team's identity sweep runs with GSTACK_SWEEP_EXCLUDE_SOURCES= verify-gate: turning team mode off must not delete the user-registered verify-gate opt-in whose binary still exists (uninstall still sweeps it, correctly, because there the binary itself is being removed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: adversarial regression pins — wrong-shape fail-loud, prototype basename, sweep exclusion, lock exit 5 New pins for the fail-loud hardening: a wrong-shape hooks value (object where an array belongs) exits 4 with "refusing to mutate" and leaves the file byte-identical (pre-gsMain this was a silent exit-0 no-op); a foreign hook whose basename collides with Object.prototype ("toString") survives an --all sweep that still removes gstack rows; GSTACK_SWEEP_EXCLUDE_SOURCES preserves the verify-gate row during --all; the fresh-foreign-lock test now asserts the loud exit 5 instead of a quiet skip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(verify-gate): allow the --no-team sweep exclusion, keep registration banned setup now legitimately mentions verify-gate once: the --no-team identity sweep excludes it via GSTACK_SWEEP_EXCLUDE_SOURCES so team-mode teardown can't delete a user-registered gate. The opt-in pin tightens from a blanket not-contains to: every mention must be a comment or that exclusion, and no mention may sit on an add-event line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(settings-hook): GNU-first stat in the lock stale check — Linux abort on held locks On Linux, BSD-style `stat -f %m` prints a multi-line FILESYSTEM block to stdout before exiting 1, so the BSD-first || chain captured that garbage concatenated with the real `stat -c %Y` epoch. The non-numeric mtime made `$(( now - mtime ))` a syntax error and set -e killed the binary with exit 1 whenever a lock dir already existed — every contention path (stale takeover, give-up, concurrent writers) broke on CI while staying green on macOS, where BSD stat -f succeeds cleanly. GNU `stat -c %Y` now goes first (BSD stat rejects -c with no stdout, so macOS falls through cleanly), and a numeric guard blanks any residual garbage so a future platform quirk degrades to the normal give-up path instead of an arithmetic abort. Same defect class as gstack-repo-mode's GNU-first ordering (#2195). Verified in an oven/bun Linux container: the four CI-failing lock tests now pass (62/62 across both files). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(uninstall): 30s budgets for the two subprocess-heavy behavioral tests Both tests spawn the copied uninstaller, which itself runs several settings-hook bun -e children (the lock-contention one also waits out a 300ms give-up per call). On a loaded box those cold starts blow bun's default 5s per-test timeout, and a timeout kill reports as a bare fail with no assertion diff — observed at 5.6-8.5s under load avg 25+. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-08-23 14:32:33 +02:00 · 2026-08-19 16:53:19 -07:00
parent 28d59ad56c
commit 9da6692930
21 changed files with 2201 additions and 271 deletions
@@ -231,3 +231,126 @@ describe('gstack-uninstall', () => {
    });
  });
 });
+
+// ----------------------------------------------------------------------
+// Hook-cleanup ordering (phantom-hooks fix). Pre-v1.67.2, the settings
+// cleanup ran AFTER `rm -rf $CLAUDE_SKILLS/gstack` — and SETTINGS_HOOK
+// resolves via $(dirname "$0") INSIDE that root, so a real global uninstall
+// (running the installed copy) silently orphaned every hook. Prior tests
+// masked this by running the uninstaller from the repo checkout. This test
+// runs the INSTALLED copy.
+// ----------------------------------------------------------------------
+
+describe('hook cleanup runs before the install root is deleted', () => {
+  test('uninstall executed FROM the install root still removes hook entries', () => {
+    const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-uninstall-order-'));
+    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 });
+      // The installed copies — the uninstaller under test IS the one inside
+      // the root it deletes.
+      for (const b of ['gstack-uninstall', 'gstack-settings-hook', 'gstack-session-update', 'gstack-config']) {
+        const src = path.join(ROOT, 'bin', b);
+        const dst = path.join(installBin, b);
+        fs.copyFileSync(src, dst);
+        fs.chmodSync(dst, 0o755);
+      }
+      const settingsFile = path.join(mockHome, '.claude', 'settings.json');
+      fs.writeFileSync(settingsFile, JSON.stringify({
+        hooks: {
+          PostToolUse: [{
+            matcher: '(AskUserQuestion|mcp__.*__AskUserQuestion)',
+            _gstack_source: 'auq-error-fallback',
+            hooks: [{ type: 'command', command: '/dead/wt/hosts/claude/hooks/auq-error-fallback-hook', timeout: 5 }],
+          }],
+          Stop: [{
+            hooks: [{ type: 'command', command: `${installRoot}/hosts/claude/hooks/timeline-stop-hook`, timeout: 5 }],
+          }],
+          PreCompact: [{ hooks: [{ type: 'command', command: '/Users/me/my-own-hook' }] }],
+        },
+      }, null, 2));
+      fs.mkdirSync(path.join(mockHome, '.gstack'), { recursive: true });
+
+      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'),
+        },
+        cwd: tmp,
+      });
+      expect(result.status).toBe(0);
+
+      // Install root gone…
+      expect(fs.existsSync(installRoot)).toBe(false);
+      // …and the hook entries were still cleaned (cleanup ran BEFORE deletion).
+      const s = JSON.parse(fs.readFileSync(settingsFile, 'utf-8'));
+      expect(s.hooks?.PostToolUse).toBeUndefined();
+      expect(s.hooks?.Stop).toBeUndefined();
+      // The user's own hook survives the sweep.
+      expect(s.hooks?.PreCompact?.[0]?.hooks?.[0]?.command).toBe('/Users/me/my-own-hook');
+    } finally {
+      fs.rmSync(tmp, { recursive: true, force: true });
+    }
+    // 30s: the copied uninstaller spawns several bun -e children; on a loaded
+    // box their cold starts blow bun's default 5s per-test timeout.
+  }, 30000);
+});
+
+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 });
+    }
+    // 30s: several settings-hook calls each wait out the 300ms lock give-up,
+    // and their bun -e cold starts stack up under load — the default 5s
+    // per-test budget is too tight on a busy box.
+  }, 30000);
+});