From 19eed1b39287d499a8ef8bb29a67562e52c1d577 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 18 Aug 2026 09:03:12 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20pre-landing=20review=20fixes=20=E2=80=94?= =?UTF-8?q?=20review-army=20findings=20hardened?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- bin/gstack-settings-hook | 161 +++++++++++++----- bin/gstack-uninstall | 10 +- setup | 32 +++- .../gstack-settings-hook-schema-aware.test.ts | 114 +++++++++++++ test/setup-hook-canonical-paths.test.ts | 28 +++ test/uninstall.test.ts | 51 ++++++ 6 files changed, 340 insertions(+), 56 deletions(-) diff --git a/bin/gstack-settings-hook b/bin/gstack-settings-hook index 28e7ae026..1cd62d41f 100755 --- a/bin/gstack-settings-hook +++ b/bin/gstack-settings-hook @@ -5,10 +5,13 @@ # # 1. Legacy (SessionStart only — kept so old installs still clean up): # gstack-settings-hook add # adds SessionStart hook -# gstack-settings-hook remove # removes matching SessionStart hook items +# gstack-settings-hook remove # removes gstack-session-update items +# # (the arg is accepted for +# # interface compat; matching is by +# # the gstack-session-update basename) # -# 2. Schema-aware (plan-tune cathedral T3 — supports PreToolUse + PostToolUse): -# gstack-settings-hook add-event --event \ +# 2. Schema-aware (plan-tune cathedral T3): +# gstack-settings-hook add-event --event \ # --command --source [--matcher ] [--timeout ] # gstack-settings-hook remove-source --source # gstack-settings-hook diff-event --event ... --command ... --source ... [--matcher ...] @@ -84,7 +87,11 @@ function gsHadBashPrefix(c) { return String(c == null ? "" : c).trim().indexOf(" function gsStripWrap(c) { var s = String(c == null ? "" : c).trim(); if (s.indexOf("bash ") === 0) s = s.slice(5).trim(); - if (s.length >= 2 && s.charAt(0) === "\"" && s.charAt(s.length - 1) === "\"") s = s.slice(1, -1); + if (s.length >= 2 && s.charAt(0) === "\"" && s.charAt(s.length - 1) === "\"") { + // Unescape the gsQuoteCmd form so a re-pointed escaped command is still + // recognized as ours on later passes (identity round-trips). + s = s.slice(1, -1).replace(/\\([\\"$\x60])/g, "$1"); + } return s.replace(/\\/g, "/"); } function gsBaseOf(c) { var p = gsStripWrap(c); return p.split("/").pop(); } @@ -108,7 +115,38 @@ function gsIsAlive(cmd) { if (!st.isFile()) return false; fs.accessSync(p, fs.constants.X_OK); return true; - } catch (e) { return false; } + } catch (e) { + // Only provable absence counts as dead. EACCES/EIO/unmounted-volume + // errors are transient unreachability -- pruning on those would be a + // one-way ratchet, so conservatively treat the item as alive. + var code = e && e.code; + return !(code === "ENOENT" || code === "ENOTDIR"); + } +} +function gsQuoteCmd(target, hadBash) { + // Shell-metacharacter hardening: the command string is executed by a shell + // when Claude Code fires the hook, so a path containing $, backtick (x60 -- + // written as an escape so the prelude itself stays backtick-free), or a + // quote must be neutralized, not just space-wrapped. + var needsQuote = /[\s$\x60"\\]/.test(target); + var quoted = needsQuote + ? "\"" + target.replace(/[\\"$\x60]/g, function (ch) { return "\\" + ch; }) + "\"" + : target; + return (hadBash ? "bash " : "") + quoted; +} +function gsRotateBackups(settingsPath, keep) { + // Backup files are change-gated but unbounded across months of setups -- + // keep the most recent N so ~/.claude does not accumulate forever. + var fs = require("fs"); + var path = require("path"); + try { + var dir = path.dirname(settingsPath); + var base = path.basename(settingsPath) + ".bak."; + var baks = fs.readdirSync(dir).filter(function (f) { return f.indexOf(base) === 0; }).sort(); + for (var i = 0; i < baks.length - keep; i++) { + try { fs.unlinkSync(path.join(dir, baks[i])); } catch (e2) {} + } + } catch (e) {} } function gsLoadSettings(path) { var fs = require("fs"); @@ -129,12 +167,19 @@ function gsWriteIfChanged(path, beforeText, settings, existed) { var fs = require("fs"); var afterText = JSON.stringify(settings, null, 2); if (afterText === beforeText) return false; + // Preserve the live file mode across the tmp+rename (settings.json can + // carry API keys in its env block -- a user-tightened 0600 must never be + // silently broadened to the default 0644). Fresh files start 0600. + var mode = 0o600; if (existed) { + try { mode = fs.statSync(path).mode & 0o777; } catch (e) {} fs.copyFileSync(path, process.env.GSTACK_BACKUP_PATH); fs.writeFileSync(process.env.GSTACK_BAK_LATEST, process.env.GSTACK_BACKUP_PATH + "\n"); + gsRotateBackups(path, 10); } var tmp = process.env.GSTACK_TMP_PATH; fs.writeFileSync(tmp, afterText + "\n"); + try { fs.chmodSync(tmp, mode); } catch (e) {} fs.renameSync(tmp, path); return true; } @@ -156,9 +201,12 @@ _release_lock() { } _acquire_lock() { - # GSTACK_SETTINGS_LOCK_TIMEOUT_MS: test-only override for the 10s give-up + # GSTACK_SETTINGS_LOCK_TIMEOUT_MS: test-only override for the give-up # (a contention test should not stall the suite for 10 real seconds). - local waited_ms=0 token stale mtime now give_up_ms="${GSTACK_SETTINGS_LOCK_TIMEOUT_MS:-10000}" + local waited_ms=0 token stale mtime now + local give_up_ms="${GSTACK_SETTINGS_LOCK_TIMEOUT_MS:-10000}" + local stale_after_s=30 # lock older than this belongs to a crashed holder + local poll_ms=50 # retry cadence; sleep below derives from this token="$$-$RANDOM$RANDOM" while :; do if mkdir "$_LOCK_DIR" 2>/dev/null; then @@ -169,12 +217,11 @@ _acquire_lock() { trap 'exit 143' TERM return 0 fi - # Stale takeover: a lock older than 30s belongs to a crashed holder. - # Atomic rename means exactly one contender wins the takeover; the loser - # loops and re-contends against the winner's fresh mkdir. + # Stale takeover: atomic rename means exactly one contender wins; the + # loser loops and re-contends against the winner's fresh mkdir. mtime=$(stat -f %m "$_LOCK_DIR" 2>/dev/null || stat -c %Y "$_LOCK_DIR" 2>/dev/null || echo "") now=$(date +%s) - if [ -n "$mtime" ] && [ $(( now - mtime )) -gt 30 ]; then + if [ -n "$mtime" ] && [ $(( now - mtime )) -gt "$stale_after_s" ]; then stale="$_LOCK_DIR.stale.$$-$RANDOM" if mv "$_LOCK_DIR" "$stale" 2>/dev/null; then rm -rf "$stale" 2>/dev/null || true; fi continue @@ -183,8 +230,8 @@ _acquire_lock() { echo "gstack-settings-hook: could not acquire lock $_LOCK_DIR -- skipping this mutation (the next setup retries it)" >&2 return 1 fi - sleep 0.05 - waited_ms=$(( waited_ms + 50 )) + sleep "$(printf '0.%03d' "$poll_ms")" + waited_ms=$(( waited_ms + poll_ms )) done } @@ -242,16 +289,19 @@ case "$ACTION" in const before = JSON.stringify(settings, null, 2); if (settings.hooks && settings.hooks.SessionStart) { // Item-aware: remove only matching hook items; foreign items in the - // same entry survive; the entry is dropped only when it empties. + // same entry survive; an entry is dropped ONLY when this pass emptied + // it. Malformed/foreign entries (hooks absent, non-array, or already + // empty) are preserved verbatim -- they are not ours to judge. settings.hooks.SessionStart = settings.hooks.SessionStart - .map(entry => { - if (!Array.isArray(entry.hooks)) return entry; + .filter(entry => { + if (!Array.isArray(entry.hooks)) return true; + const beforeLen = entry.hooks.length; entry.hooks = entry.hooks.filter(h => !(h && h.command && h.command.includes("gstack-session-update")) ); - return entry; - }) - .filter(entry => Array.isArray(entry.hooks) && entry.hooks.length > 0); + if (entry.hooks.length === 0 && beforeLen > 0) return false; + return true; + }); if (settings.hooks.SessionStart.length === 0) delete settings.hooks.SessionStart; if (Object.keys(settings.hooks).length === 0) delete settings.hooks; } @@ -337,19 +387,18 @@ case "$ACTION" in return !!row && gsBaseOf(h.command) === gsBaseOf(cmd); }); } - if (idx < 0 && entry._gstack_source === source) { - if (entry.hooks.length === 1) { - idx = 0; - } else { - entry.hooks.push(hookEntry); - entry._gstack_source = source; - placed = true; - break; - } + if (idx < 0 && entry._gstack_source === source && entry.hooks.length === 1) { + idx = 0; } if (idx >= 0) { entry.hooks[idx] = hookEntry; - entry._gstack_source = source; + // Mixed-version ratchet guard: tag ONLY single-item entries (the + // item we just placed). Old gstack versions in sibling worktrees do + // entry-level ownership (remove-source deletes the whole tagged + // entry; add-event clobbers entry.hooks wholesale) -- a tag on a + // mixed entry hands them permission to destroy the user items in it. + if (entry.hooks.length === 1) entry._gstack_source = source; + else delete entry._gstack_source; placed = true; break; } @@ -409,10 +458,14 @@ case "$ACTION" in // entry are preserved and the tag is dropped with the last owned item. const single = entry.hooks.length === 1; const remain = entry.hooks.filter(h => { + // Command-less items cannot be ours (gstack only writes + // type:command items) -- preserve them. const owned = (h && h.command) ? gsOwnedRow(h.command, event, entry.matcher || "") !== null - : true; - if (owned || single) { removed++; return false; } + : false; + // The single-item stray claim requires a command item (gstack + // never writes command-less items). + if (owned || (single && h && h.command)) { removed++; return false; } return true; }); if (remain.length === 0) continue; @@ -488,20 +541,26 @@ case "$ACTION" in const wasSingle = entry.hooks.length === 1; let removedHere = 0; const remain = []; + const seenInEntry = new Set(); for (const h of entry.hooks) { const cmdRaw = h && h.command; const row = cmdRaw ? gsOwnedRow(cmdRaw, event, matcher) : null; // A tagged SINGLE-item entry with no table match is an owned - // legacy stray; tags never claim items in multi-item entries. - const stray = !row && !!entry._gstack_source && wasSingle; + // legacy stray (command items only -- gstack never writes + // command-less items); tags never claim items in multi-item entries. + const stray = !row && !!cmdRaw && !!entry._gstack_source && wasSingle; if (!row && !stray) { remain.push(h); continue; } // foreign: never touched if (all) { removed++; removedHere++; continue; } if (row && root && !(ptOptout && PT_SOURCES[row.source])) { const target = root + "/" + row.relpath; if (gsIsAlive(target)) { - const newCmd = (gsHadBashPrefix(cmdRaw) ? "bash " : "") - + (/\s/.test(target) ? "\"" + target + "\"" : target); + const newCmd = gsQuoteCmd(target, gsHadBashPrefix(cmdRaw)); if (h.command !== newCmd) { h.command = newCmd; repointed++; } + // Within-entry twin collapse: two dead copies of the same + // hook re-point to the same canonical command -- keeping + // both would fire the hook twice per event, forever. + if (seenInEntry.has(newCmd)) { removed++; removedHere++; continue; } + seenInEntry.add(newCmd); if (wasSingle) entry._gstack_source = row.source; // tag restore remain.push(h); continue; @@ -512,10 +571,13 @@ case "$ACTION" in } if (remain.length === 0) continue; // entry emptied → dropped entry.hooks = remain; - // Tag no longer owns anything → drop it (mixed entry whose gstack - // item was removed). Single-item stray entries keep their tag. - if (entry._gstack_source && removedHere > 0 - && !remain.some(h => h && h.command && gsOwnedRow(h.command, event, matcher))) { + // Tag hygiene: a tag must never sit on an entry containing foreign + // items (old gstack versions in sibling worktrees treat tags as + // entry-level ownership and would destroy the user items). Drop + // the tag from any mixed entry; single-item strays keep theirs. + if (entry._gstack_source && entry.hooks.length > 0 + && !entry.hooks.every(h => h && h.command && gsOwnedRow(h.command, event, matcher)) + && !(entry.hooks.length === 1 && wasSingle)) { delete entry._gstack_source; } rebuilt.push(entry); @@ -529,8 +591,7 @@ case "$ACTION" in const fullyOwned = items.length > 0 && items.every(h => h && h.command && gsOwnedRow(h.command, event, entry.matcher || "")); if (!fullyOwned) { out.push(entry); continue; } - const key = (entry.matcher || "") + "" - + items.map(h => h.command).sort().join(""); + const key = JSON.stringify([entry.matcher || ""].concat(items.map(function (h) { return h.command; }).sort())); const at = seen.get(key); if (at === undefined) { seen.set(key, out.length); out.push(entry); } else { @@ -556,6 +617,16 @@ case "$ACTION" in fi LATEST=$(cat "$SETTINGS_FILE.bak-latest") LATEST=$(printf '%s' "$LATEST" | tr -d '\n') + # Defense in depth: only ever restore a sibling settings.json.bak.* file + # (a corrupted/hostile pointer must not install an arbitrary file as the + # live settings.json). + case "$LATEST" in + "$SETTINGS_FILE".bak.*) ;; + *) + echo "rollback: pointer target $LATEST is not a $SETTINGS_FILE.bak.* file -- refusing" >&2 + exit 1 + ;; + esac if [ ! -f "$LATEST" ]; then echo "rollback: pointer references missing backup $LATEST" >&2 exit 1 @@ -572,7 +643,13 @@ case "$ACTION" in GSTACK_SETTINGS_PATH="$SETTINGS_FILE" bun -e "$_HOOK_JS_PRELUDE"' const fs = require("fs"); let settings = {}; - try { settings = JSON.parse(fs.readFileSync(process.env.GSTACK_SETTINGS_PATH, "utf8")); } catch { process.exit(0); } + try { settings = JSON.parse(fs.readFileSync(process.env.GSTACK_SETTINGS_PATH, "utf8")); } + catch (e) { + // Read-only surface: report loudly (setup guards read this output and + // must not mistake corrupt-file for no-hooks) but exit 0. + process.stderr.write("gstack-settings-hook: settings.json unparseable (" + e.message + ") -- fix or rollback\n"); + process.exit(0); + } const hooks = settings.hooks || {}; let any = false; for (const event of Object.keys(hooks)) { diff --git a/bin/gstack-uninstall b/bin/gstack-uninstall index e86447330..d131fc71d 100755 --- a/bin/gstack-uninstall +++ b/bin/gstack-uninstall @@ -139,24 +139,24 @@ fi SETTINGS_HOOK="$(dirname "$0")/gstack-settings-hook" SESSION_UPDATE="$(dirname "$0")/gstack-session-update" if [ -x "$SETTINGS_HOOK" ]; then - "$SETTINGS_HOOK" remove "$SESSION_UPDATE" 2>/dev/null && REMOVED+=("SessionStart hook") || true + "$SETTINGS_HOOK" remove "$SESSION_UPDATE" && REMOVED+=("SessionStart hook") || true # Cathedral T8 cleanup: also remove plan-tune PreToolUse + PostToolUse hooks. - if "$SETTINGS_HOOK" remove-source --source plan-tune-cathedral 2>/dev/null | grep -q "removed [1-9]"; then + if "$SETTINGS_HOOK" remove-source --source plan-tune-cathedral | grep -q "removed [1-9]"; then REMOVED+=("plan-tune cathedral hooks") fi # AskUserQuestion error-fallback hook (registered by setup; previously never # torn down). - if "$SETTINGS_HOOK" remove-source --source auq-error-fallback 2>/dev/null | grep -q "removed [1-9]"; then + if "$SETTINGS_HOOK" remove-source --source auq-error-fallback | grep -q "removed [1-9]"; then REMOVED+=("AskUserQuestion error-fallback hook") fi # Timeline Stop hook (#2553). - if "$SETTINGS_HOOK" remove-source --source gstack-timeline-stop 2>/dev/null | grep -q "removed [1-9]"; then + if "$SETTINGS_HOOK" remove-source --source gstack-timeline-stop | grep -q "removed [1-9]"; then REMOVED+=("timeline Stop hook") fi # Identity sweep for untagged strays (Claude Code strips _gstack_source # tags; pre-v1.67 setups baked worktree paths). Removes every gstack-owned # hook item, live or dead — the binaries they point at are being deleted. - if "$SETTINGS_HOOK" prune-stale --all 2>/dev/null | grep -q "removed [1-9]"; then + if "$SETTINGS_HOOK" prune-stale --all | grep -q "removed [1-9]"; then REMOVED+=("stray gstack hook entries") fi fi diff --git a/setup b/setup index b01acaa6f..0a4435d81 100755 --- a/setup +++ b/setup @@ -1896,7 +1896,7 @@ if [ -x "$SETTINGS_HOOK" ]; then _HEAL_REMOVED=$(printf '%s' "$_HEAL_OUT" | sed -n 's/^OK: removed \([0-9]*\).*/\1/p') _HEAL_REPOINTED=$(printf '%s' "$_HEAL_OUT" | sed -n 's/.*repointed \([0-9]*\).*/\1/p') if [ "${_HEAL_REMOVED:-0}" -gt 0 ] 2>/dev/null || [ "${_HEAL_REPOINTED:-0}" -gt 0 ] 2>/dev/null; then - log " healed hook registrations: removed ${_HEAL_REMOVED:-0}, repointed ${_HEAL_REPOINTED:-0} (backup: settings.json.bak.; undo: $SETTINGS_HOOK rollback)" + log " healed hook registrations: removed ${_HEAL_REMOVED:-0}, repointed ${_HEAL_REPOINTED:-0} (backup: settings.json.bak.; note: later registrations in this run move the rollback pointer — restore the heal's own .bak file directly if needed)" fi # Explicit opt-out + live plan-tune hooks is a contradiction worth surfacing: # the heal honors the opt-out (dead plan-tune entries pruned, never @@ -1920,8 +1920,13 @@ fi SESSION_UPDATE_CMD="$(_hook_command_path bin/gstack-session-update || true)" HOOK_CMD="" if [ -n "$SESSION_UPDATE_CMD" ]; then + # The registered command is executed by a shell when Claude Code fires the + # hook — neutralize shell metacharacters, not just whitespace (mirrors + # gsQuoteCmd in gstack-settings-hook). case "$SESSION_UPDATE_CMD" in - *" "*) SESSION_UPDATE_CMD="\"$SESSION_UPDATE_CMD\"" ;; + *[\ \"\$\`\\]*) + SESSION_UPDATE_CMD="\"$(printf '%s' "$SESSION_UPDATE_CMD" | sed 's/[\\"$`]/\\&/g')\"" + ;; esac if [ "$IS_WINDOWS" -eq 1 ]; then HOOK_CMD="bash $SESSION_UPDATE_CMD" @@ -1937,15 +1942,21 @@ if [ "$TEAM_MODE" -eq 1 ]; then # Register SessionStart hook in Claude Code settings (schema-aware: the # legacy `add` action's substring dedupe bypasses the KNOWN_HOOKS identity # system; add-event re-points stale paths in place instead of appending). + # stderr stays attached (zero silent settings mutations — a fail-closed + # parse error or lock give-up must reach the user). if [ -x "$SETTINGS_HOOK" ] && [ -n "$HOOK_CMD" ]; then - "$SETTINGS_HOOK" add-event --event SessionStart --command "$HOOK_CMD" --source gstack-session-update >/dev/null 2>&1 || true + "$SETTINGS_HOOK" add-event --event SessionStart --command "$HOOK_CMD" --source gstack-session-update >/dev/null || true elif [ -z "$HOOK_CMD" ]; then log " SessionStart hook not registered: bin/gstack-session-update missing at $CANONICAL_GSTACK_ROOT (no stable install)" fi log "" - log "Team mode enabled: gstack will auto-update at the start of each Claude Code session." - log " Hook: $HOOK_CMD" + if [ -n "$HOOK_CMD" ]; then + log "Team mode enabled: gstack will auto-update at the start of each Claude Code session." + log " Hook: $HOOK_CMD" + else + log "Team mode enabled (auto-update hook pending a stable install — re-run ./setup after installing globally)." + fi log " To disable: ./setup --no-team" log "" log "Bootstrap your repo:" @@ -2279,11 +2290,14 @@ fi # Also tear down plan-tune + timeline hooks on --no-team (matches the existing pattern). # Tag-only remove-source misses untagged entries (Claude Code strips # _gstack_source), so the identity sweep (prune-stale --all) finishes the job. +# stderr stays attached on every call: a lock give-up or fail-closed parse +# error during TEARDOWN must be visible — "the next setup retries" does not +# apply when the user is turning the hooks off. if [ "$NO_TEAM_MODE" -eq 1 ] && [ -x "$SETTINGS_HOOK" ]; then - "$SETTINGS_HOOK" remove-source --source plan-tune-cathedral 2>/dev/null || true - "$SETTINGS_HOOK" remove-source --source auq-error-fallback 2>/dev/null || true - "$SETTINGS_HOOK" remove-source --source gstack-timeline-stop 2>/dev/null || true - "$SETTINGS_HOOK" prune-stale --all >/dev/null 2>&1 || true + "$SETTINGS_HOOK" remove-source --source plan-tune-cathedral >/dev/null || true + "$SETTINGS_HOOK" remove-source --source auq-error-fallback >/dev/null || true + "$SETTINGS_HOOK" remove-source --source gstack-timeline-stop >/dev/null || true + "$SETTINGS_HOOK" prune-stale --all >/dev/null || true fi # ─── Redact pre-push guard consent (#1946) ─────────────────────────────────── diff --git a/test/gstack-settings-hook-schema-aware.test.ts b/test/gstack-settings-hook-schema-aware.test.ts index 9a3750e22..f8e0cfc62 100644 --- a/test/gstack-settings-hook-schema-aware.test.ts +++ b/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); diff --git a/test/setup-hook-canonical-paths.test.ts b/test/setup-hook-canonical-paths.test.ts index b67eddfd1..b5f4b8a9b 100644 --- a/test/setup-hook-canonical-paths.test.ts +++ b/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)'); + } + }); +}); diff --git a/test/uninstall.test.ts b/test/uninstall.test.ts index 9cc5d6c8b..8ad6870b9 100644 --- a/test/uninstall.test.ts +++ b/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 }); + } + }); +});