diff --git a/bin/gstack-settings-hook b/bin/gstack-settings-hook index 463b3e4c5..46d22533f 100755 --- a/bin/gstack-settings-hook +++ b/bin/gstack-settings-hook @@ -10,13 +10,24 @@ # 2. Schema-aware (plan-tune cathedral T3 — supports PreToolUse + PostToolUse): # gstack-settings-hook add-event --event \ # --command --source [--matcher ] [--timeout ] +# gstack-settings-hook ensure-event --event ... --command ... --source ... [--matcher ...] [--timeout ] # gstack-settings-hook remove-source --source # gstack-settings-hook diff-event --event ... --command ... --source ... [--matcher ...] # gstack-settings-hook rollback # restore latest backup # gstack-settings-hook list-sources # show all gstack-tagged hook entries # +# ensure-event is the update-in-place verb: same flags as add-event, but it +# first compares the REGISTERED payload for (event, matcher, source) against +# the requested one. Identical → no write, no backup ("unchanged"). Different +# → the single matching entry is replaced via one atomic tmp+rename, so a +# failed update can never leave zero or two registrations. This is what heals +# a stale absolute hook path (e.g. a deleted dev worktree) baked into +# settings.json by an earlier setup — presence-only dedup never re-pointed it. +# # Every add-event/remove-source writes a backup to ~/.claude/settings.json.bak. -# before mutating (Codex correction — silent settings.json mutation is wrong). +# before mutating (Codex correction — silent settings.json mutation is wrong); +# ensure-event backs up only when it actually mutates, so a no-op re-run of +# ./setup doesn't churn backup files. # # Dedup: legacy `add`/`remove` dedupe by the historical `gstack-session-update` # substring. Schema-aware `add-event` dedupes by (event, matcher, _gstack_source) so @@ -34,6 +45,7 @@ Usage: gstack-settings-hook add # legacy SessionStart add gstack-settings-hook remove # legacy SessionStart remove gstack-settings-hook add-event --event --command --source [--matcher ] [--timeout ] + gstack-settings-hook ensure-event --event --command --source [--matcher ] [--timeout ] gstack-settings-hook remove-source --source gstack-settings-hook diff-event --event --command --source [--matcher ] [--timeout ] gstack-settings-hook rollback @@ -114,7 +126,7 @@ case "$ACTION" in ' 2>/dev/null ;; - add-event|diff-event) + add-event|diff-event|ensure-event) EVENT="" COMMAND="" SOURCE="" @@ -132,7 +144,7 @@ case "$ACTION" in esac done if [ -z "$EVENT" ] || [ -z "$COMMAND" ] || [ -z "$SOURCE" ]; then - echo "add-event/diff-event require --event, --command, --source" >&2 + echo "add-event/ensure-event/diff-event require --event, --command, --source" >&2 exit 1 fi case "$EVENT" in @@ -144,6 +156,8 @@ case "$ACTION" in fi DIFF_ONLY="" if [ "$ACTION" = "diff-event" ]; then DIFF_ONLY=1; fi + ENSURE="" + if [ "$ACTION" = "ensure-event" ]; then ENSURE=1; fi GSTACK_SETTINGS_PATH="$SETTINGS_FILE" \ GSTACK_EVENT="$EVENT" \ GSTACK_COMMAND="$COMMAND" \ @@ -151,6 +165,7 @@ case "$ACTION" in GSTACK_MATCHER="$MATCHER" \ GSTACK_TIMEOUT="$TIMEOUT" \ GSTACK_DIFF_ONLY="$DIFF_ONLY" \ + GSTACK_ENSURE="$ENSURE" \ bun -e ' const fs = require("fs"); const settingsPath = process.env.GSTACK_SETTINGS_PATH; @@ -160,6 +175,7 @@ case "$ACTION" in const matcher = process.env.GSTACK_MATCHER || ""; const timeoutRaw = process.env.GSTACK_TIMEOUT || ""; const diffOnly = process.env.GSTACK_DIFF_ONLY === "1"; + const ensure = process.env.GSTACK_ENSURE === "1"; let settings = {}; try { settings = JSON.parse(fs.readFileSync(settingsPath, "utf8")); } catch {} @@ -202,10 +218,43 @@ case "$ACTION" in process.exit(0); } - const tmp = settingsPath + ".tmp"; - fs.writeFileSync(tmp, after + "\n"); - fs.renameSync(tmp, settingsPath); - console.log("OK: " + event + " hook registered (source: " + source + ")"); + if (ensure && before === after) { + // Registered payload already matches the canonical one — no write, no + // backup, no churn. Re-running ./setup stays a true no-op. + console.log("OK: " + event + " hook unchanged (source: " + source + ")"); + process.exit(0); + } + + try { + if (ensure && fs.existsSync(settingsPath)) { + // Mirrors backup_settings (bash) — but only when a write actually + // happens, so a no-op ensure-event never creates backup files. + const d = new Date(); + const pad = (n) => String(n).padStart(2, "0"); + const ts = "" + d.getFullYear() + pad(d.getMonth() + 1) + pad(d.getDate()) + + "-" + pad(d.getHours()) + pad(d.getMinutes()) + pad(d.getSeconds()); + fs.copyFileSync(settingsPath, settingsPath + ".bak." + ts); + fs.writeFileSync(settingsPath + ".bak-latest", settingsPath + ".bak." + ts + "\n"); + } + + // Atomic tmp+rename: the settings file is either the old JSON (with + // the old single registration) or the new JSON (with the replaced + // one) — a failed update can never leave zero or two registrations. + const tmp = settingsPath + ".tmp"; + fs.writeFileSync(tmp, after + "\n"); + fs.renameSync(tmp, settingsPath); + } catch (e) { + // Explicit catch + exit 1: bun -e has been observed (1.3.13) to turn + // an uncaught sync fs error into a SILENT exit 0, which would let a + // failed update masquerade as success to the caller. + console.error("error: could not update " + settingsPath + ": " + (e && e.message ? e.message : e)); + process.exit(1); + } + if (ensure && existing) { + console.log("OK: " + event + " hook re-pointed (source: " + source + ")"); + } else { + console.log("OK: " + event + " hook registered (source: " + source + ")"); + } ' ;; diff --git a/setup b/setup index a17e5c22c..db492f975 100755 --- a/setup +++ b/setup @@ -1978,6 +1978,23 @@ if [ -x "$DETECT_BIN" ]; then fi fi +# Hook commands registered into ~/.claude/settings.json must survive deletion +# of the directory setup ran from. A dev-worktree setup used to bake +# $SOURCE_GSTACK_DIR's absolute path into the registration; deleting that +# worktree left a dead hook erroring on every trigger. Prefer the global +# install (~/.claude/skills/gstack — a persistent checkout, or a stable +# symlink) and fall back to the setup-time source tree only when no global +# install exists yet (first install from a fresh clone). +_hook_install_path() { + local rel="$1" + local global_hook="$HOME/.claude/skills/gstack/$rel" + if [ -x "$global_hook" ]; then + printf '%s' "$global_hook" + else + printf '%s' "$SOURCE_GSTACK_DIR/$rel" + fi +} + # 11. Plan-tune cathedral hook install (T8). # # Registers PostToolUse (deterministic AUQ capture) + PreToolUse (preference @@ -1986,10 +2003,12 @@ fi # per D4 + Codex: never mutate settings.json silently. # # Idempotent via _gstack_source tag = 'plan-tune-cathedral'. If both hooks -# already registered under that tag, the install is a no-op (no prompt). -PLAN_TUNE_LOG_HOOK="$SOURCE_GSTACK_DIR/hosts/claude/hooks/question-log-hook" -PLAN_TUNE_PREF_HOOK="$SOURCE_GSTACK_DIR/hosts/claude/hooks/question-preference-hook" -AUQ_ERROR_FALLBACK_HOOK="$SOURCE_GSTACK_DIR/hosts/claude/hooks/auq-error-fallback-hook" +# already registered under that tag, the install skips the consent prompt and +# only refreshes the registered command paths in place (ensure-event is a +# no-op when they already match — see the stale-path note above). +PLAN_TUNE_LOG_HOOK="$(_hook_install_path hosts/claude/hooks/question-log-hook)" +PLAN_TUNE_PREF_HOOK="$(_hook_install_path hosts/claude/hooks/question-preference-hook)" +AUQ_ERROR_FALLBACK_HOOK="$(_hook_install_path hosts/claude/hooks/auq-error-fallback-hook)" PLAN_TUNE_INSTALL_MARKER="$HOME/.gstack/.plan-tune-hooks-prompted" if [ "$NO_TEAM_MODE" -ne 1 ] \ @@ -2040,13 +2059,16 @@ if [ "$NO_TEAM_MODE" -ne 1 ] \ fi _install_plan_tune_hooks() { - "$SETTINGS_HOOK" add-event \ + # ensure-event (not add-event): registers when missing, RE-POINTS a stale + # command path in place when the registration differs, and is a true no-op + # (no write, no backup churn) when it already matches. + "$SETTINGS_HOOK" ensure-event \ --event PostToolUse \ --matcher '(AskUserQuestion|mcp__.*__AskUserQuestion)' \ --command "$PLAN_TUNE_LOG_HOOK" \ --source plan-tune-cathedral \ --timeout 5 - "$SETTINGS_HOOK" add-event \ + "$SETTINGS_HOOK" ensure-event \ --event PreToolUse \ --matcher '(AskUserQuestion|mcp__.*__AskUserQuestion)' \ --command "$PLAN_TUNE_PREF_HOOK" \ @@ -2060,7 +2082,7 @@ if [ "$NO_TEAM_MODE" -ne 1 ] \ # question-log capture hook (same event+matcher). A distinct source = a second # PostToolUse entry; both run in parallel. if [ -x "$AUQ_ERROR_FALLBACK_HOOK" ]; then - "$SETTINGS_HOOK" add-event \ + "$SETTINGS_HOOK" ensure-event \ --event PostToolUse \ --matcher '(AskUserQuestion|mcp__.*__AskUserQuestion)' \ --command "$AUQ_ERROR_FALLBACK_HOOK" \ @@ -2070,6 +2092,10 @@ if [ "$NO_TEAM_MODE" -ne 1 ] \ } if [ "$ALREADY_INSTALLED" -eq 1 ]; then + # Consent already recorded — no prompt. But a registration from an earlier + # setup may carry a stale absolute path (a since-deleted dev worktree); + # ensure-event re-points it in place and no-ops when everything matches. + _install_plan_tune_hooks >/dev/null 2>&1 || true log "" log "Plan-tune hooks already installed. Run \`$SETTINGS_HOOK list-sources\` to inspect." elif [ "$PT_DECISION" = "yes" ]; then @@ -2162,18 +2188,33 @@ fi # unenforceable — interrupted sessions leaked started > completed forever. # Register a Stop-event hook that closes dangling entries. FAIL-OPEN contract # (F5): the hook always exits 0 and repairs best-effort — it can never block -# a session. Idempotent via the (event, source) dedup in gstack-settings-hook; -# removed by --no-team and gstack-uninstall. -TIMELINE_STOP_HOOK="$SOURCE_GSTACK_DIR/hosts/claude/hooks/timeline-stop-hook" +# a session. Removed by --no-team and gstack-uninstall. +# +# The command path prefers the global install (see _hook_install_path): a +# dev-worktree setup used to bake its own absolute dir into settings.json, so +# deleting the worktree left a dead hook erroring on every session stop — and +# the old presence-only dedup (list-sources | grep) never re-pointed it on a +# re-run. ensure-event registers when missing, replaces a stale path in place +# (one atomic write — never zero or two registrations), and no-ops when the +# registration already matches. +TIMELINE_STOP_HOOK="$(_hook_install_path hosts/claude/hooks/timeline-stop-hook)" if [ "$NO_TEAM_MODE" -ne 1 ] && [ -x "$SETTINGS_HOOK" ] && [ -x "$TIMELINE_STOP_HOOK" ]; then - if ! "$SETTINGS_HOOK" list-sources 2>/dev/null | grep -q "gstack-timeline-stop"; then - if "$SETTINGS_HOOK" add-event \ - --event Stop \ - --command "$TIMELINE_STOP_HOOK" \ - --source gstack-timeline-stop \ - --timeout 5 >/dev/null 2>&1; then - log " registered Stop hook: session timeline entries now close even when a skill is interrupted (backup: settings.json.bak.; remove: $SETTINGS_HOOK remove-source --source gstack-timeline-stop)" - fi + if _TL_ENSURE_OUT=$("$SETTINGS_HOOK" ensure-event \ + --event Stop \ + --command "$TIMELINE_STOP_HOOK" \ + --source gstack-timeline-stop \ + --timeout 5 2>/dev/null); then + case "$_TL_ENSURE_OUT" in + *unchanged*) + : # already registered with the canonical command — quiet no-op + ;; + *re-pointed*) + log " re-pointed Stop hook to $TIMELINE_STOP_HOOK (previous registration held a stale path)" + ;; + *) + log " registered Stop hook: session timeline entries now close even when a skill is interrupted (backup: settings.json.bak.; remove: $SETTINGS_HOOK remove-source --source gstack-timeline-stop)" + ;; + esac fi fi diff --git a/test/timeline-stop-hook.test.ts b/test/timeline-stop-hook.test.ts index b64eb33bb..c9c171ccf 100644 --- a/test/timeline-stop-hook.test.ts +++ b/test/timeline-stop-hook.test.ts @@ -225,6 +225,8 @@ describe('timeline-stop-hook (#2553, F5 fail-open)', () => { }); describe('timeline-stop-hook wiring', () => { + const SETTINGS_HOOK = path.join(ROOT, 'bin', 'gstack-settings-hook'); + test('setup registers the Stop hook with its own source tag and tears it down on --no-team', () => { const setup = fs.readFileSync(path.join(ROOT, 'setup'), 'utf-8'); expect(setup).toContain('--event Stop'); @@ -235,6 +237,150 @@ describe('timeline-stop-hook wiring', () => { expect(teardown).toContain('remove-source --source gstack-timeline-stop'); }); + test('setup routes the Stop hook through ensure-event, not presence-only dedup', () => { + const setup = fs.readFileSync(path.join(ROOT, 'setup'), 'utf-8'); + // ensure-event registers when missing AND re-points a stale path in place. + expect(setup).toMatch(/ensure-event[\s\S]{0,220}--source gstack-timeline-stop/); + // The old guard skipped registration whenever the source tag was merely + // PRESENT, so a stale absolute path (deleted dev worktree) was never + // re-pointed on a setup re-run. + expect(setup).not.toMatch(/list-sources 2>\/dev\/null \| grep -q "gstack-timeline-stop"/); + }); + + test('fresh register prefers the global-install hook path when present', () => { + // Drive setup's _hook_install_path directly: global install present → the + // registration survives deleting the worktree setup ran from. + const setup = fs.readFileSync(path.join(ROOT, 'setup'), 'utf-8'); + const fn = setup.match(/_hook_install_path\(\) \{[\s\S]*?\n\}/); + expect(fn).not.toBeNull(); + + const fakeHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-hookpath-')); + try { + const globalHook = path.join( + fakeHome, '.claude', 'skills', 'gstack', 'hosts', 'claude', 'hooks', 'timeline-stop-hook', + ); + fs.mkdirSync(path.dirname(globalHook), { recursive: true }); + fs.writeFileSync(globalHook, '#!/bin/sh\nexit 0\n', { mode: 0o755 }); + const env = { ...process.env, HOME: fakeHome, SOURCE_GSTACK_DIR: '/some/dev/worktree' }; + + const withGlobal = spawnSync( + 'bash', + ['-c', `${fn![0]}\n_hook_install_path hosts/claude/hooks/timeline-stop-hook`], + { env, encoding: 'utf-8', timeout: 10_000 }, + ); + expect(withGlobal.stdout.trim()).toBe(globalHook); + + // No global install (fresh first install from a clone) → setup-time path. + fs.rmSync(globalHook); + const withoutGlobal = spawnSync( + 'bash', + ['-c', `${fn![0]}\n_hook_install_path hosts/claude/hooks/timeline-stop-hook`], + { env, encoding: 'utf-8', timeout: 10_000 }, + ); + expect(withoutGlobal.stdout.trim()).toBe('/some/dev/worktree/hosts/claude/hooks/timeline-stop-hook'); + } finally { + fs.rmSync(fakeHome, { recursive: true, force: true }); + } + }); + + test('ensure-event re-points a stale absolute path and leaves exactly one registration', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-ensure-')); + try { + const settingsFile = path.join(dir, 'settings.json'); + fs.writeFileSync(settingsFile, JSON.stringify({ + hooks: { + Stop: [{ + _gstack_source: 'gstack-timeline-stop', + hooks: [{ type: 'command', command: '/deleted/worktree/hosts/claude/hooks/timeline-stop-hook', timeout: 5 }], + }], + }, + }, null, 2) + '\n'); + + const r = spawnSync('bash', [ + SETTINGS_HOOK, 'ensure-event', + '--event', 'Stop', + '--command', HOOK, + '--source', 'gstack-timeline-stop', + '--timeout', '5', + ], { env: { ...process.env, GSTACK_SETTINGS_FILE: settingsFile }, encoding: 'utf-8', timeout: 15_000 }); + + expect(r.status).toBe(0); + expect(r.stdout).toContain('re-pointed'); + const s = JSON.parse(fs.readFileSync(settingsFile, 'utf-8')); + expect(s.hooks.Stop).toHaveLength(1); // replaced in place — never two + expect(s.hooks.Stop[0].hooks[0].command).toBe(HOOK); + expect(s.hooks.Stop[0]._gstack_source).toBe('gstack-timeline-stop'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + test('ensure-event is a true no-op when the registration already matches (no write, no backup churn)', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-ensure-noop-')); + try { + const settingsFile = path.join(dir, 'settings.json'); + const args = [ + SETTINGS_HOOK, 'ensure-event', + '--event', 'Stop', + '--command', HOOK, + '--source', 'gstack-timeline-stop', + '--timeout', '5', + ]; + const env = { ...process.env, GSTACK_SETTINGS_FILE: settingsFile }; + const first = spawnSync('bash', args, { env, encoding: 'utf-8', timeout: 15_000 }); + expect(first.status).toBe(0); + const bytesAfterFirst = fs.readFileSync(settingsFile, 'utf-8'); + + const second = spawnSync('bash', args, { env, encoding: 'utf-8', timeout: 15_000 }); + expect(second.status).toBe(0); + expect(second.stdout).toContain('unchanged'); + expect(fs.readFileSync(settingsFile, 'utf-8')).toBe(bytesAfterFirst); + // Re-running ./setup must not accumulate settings.json.bak. files. + const baks = fs.readdirSync(dir).filter((f) => f.includes('.bak')); + expect(baks).toEqual([]); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + test('a failed update leaves exactly one registration — never zero, never two', () => { + // Root can write through 0o555 directories, so the failure injection + // (read-only dir) does not bind there; the invariant is still covered by + // the atomic tmp+rename pinned in the re-point test above. + if (typeof process.getuid === 'function' && process.getuid() === 0) return; + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-ensure-fail-')); + try { + const settingsFile = path.join(dir, 'settings.json'); + fs.writeFileSync(settingsFile, JSON.stringify({ + hooks: { + Stop: [{ + _gstack_source: 'gstack-timeline-stop', + hooks: [{ type: 'command', command: '/stale/path/timeline-stop-hook', timeout: 5 }], + }], + }, + }, null, 2) + '\n'); + + fs.chmodSync(dir, 0o555); // every write path (backup, tmp, rename) fails + const r = spawnSync('bash', [ + SETTINGS_HOOK, 'ensure-event', + '--event', 'Stop', + '--command', HOOK, + '--source', 'gstack-timeline-stop', + '--timeout', '5', + ], { env: { ...process.env, GSTACK_SETTINGS_FILE: settingsFile }, encoding: 'utf-8', timeout: 15_000 }); + fs.chmodSync(dir, 0o755); + + expect(r.status).not.toBe(0); // the failure is loud, not swallowed + const s = JSON.parse(fs.readFileSync(settingsFile, 'utf-8')); + expect(s.hooks.Stop).toHaveLength(1); // old registration intact + expect(s.hooks.Stop[0].hooks[0].command).toBe('/stale/path/timeline-stop-hook'); + } finally { + try { fs.chmodSync(dir, 0o755); } catch {} + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + test('gstack-uninstall removes the Stop hook registration', () => { const uninstall = fs.readFileSync(path.join(ROOT, 'bin', 'gstack-uninstall'), 'utf-8'); expect(uninstall).toContain('remove-source --source gstack-timeline-stop');