From b8d347df3594c6f3901b80b07c707955d89af253 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 4 Sep 2026 17:28:00 +0000 Subject: [PATCH] fix(hooks): partial upgrades fail closed for freeze and fall back for careful A hook script and its sourced helper can be copied at different times. With an older careful/bin/hook-extract.sh that lacks gstack_hook_state_root: - check-freeze.sh now emits a deny ("fail closed, re-run ./setup or /unfreeze") instead of dying under set -e with no decision JSON. - check-careful.sh falls back to ${GSTACK_HOME:-$HOME/.gstack} so project rules under the plain chain still load and a decision is always emitted (a warn hook must never break on a stale helper). gstack_hook_state_root prints its root without a trailing newline and both callers capture it with a printf-x sentinel, so a GSTACK_HOME ending in a newline round-trips byte-for-byte with the writer's %q form. gstack_hook_log_fire stays on ${GSTACK_HOME:-$HOME/.gstack}/analytics, the same two-step chain every other analytics writer and reader uses, so the usage log remains one file under a plugin install. Co-Authored-By: Claude Fable 5.1 --- careful/bin/check-careful.sh | 10 ++- careful/bin/hook-extract.sh | 22 +++--- freeze/bin/check-freeze.sh | 9 ++- test/hook-scripts.test.ts | 130 +++++++++++++++++++++++++++++++++++ 4 files changed, 161 insertions(+), 10 deletions(-) diff --git a/careful/bin/check-careful.sh b/careful/bin/check-careful.sh index b6bebb9d3..11993f110 100755 --- a/careful/bin/check-careful.sh +++ b/careful/bin/check-careful.sh @@ -265,7 +265,15 @@ fi # ERE per line; blank lines and #-comments skipped; an invalid regex is # skipped (never fatal — the hook must not break on a typo in config). if [ -z "$WARN" ]; then - _GSTACK_HOME_DIR="${GSTACK_HOME:-$HOME/.gstack}" + # Same state root the writer (/careful via gstack-paths) uses — see + # gstack_hook_state_root in hook-extract.sh (#1459 class). + if command -v gstack_hook_state_root >/dev/null 2>&1; then + _GSTACK_HOME_DIR="$(gstack_hook_state_root; printf x)"; _GSTACK_HOME_DIR="${_GSTACK_HOME_DIR%x}" + else + # Older hook-extract.sh (partial upgrade): the plain chain beats dying + # under set -e with no decision JSON — rules under $HOME/.gstack still load. + _GSTACK_HOME_DIR="${GSTACK_HOME:-$HOME/.gstack}" + fi _PATTERN_FILES="$_GSTACK_HOME_DIR/careful-patterns.txt" # Short-circuit: resolving the project slug costs a subprocess + git call on # EVERY Bash command while /careful is active — only pay it when some diff --git a/careful/bin/hook-extract.sh b/careful/bin/hook-extract.sh index 592e8fbd7..594b4d6c0 100644 --- a/careful/bin/hook-extract.sh +++ b/careful/bin/hook-extract.sh @@ -73,25 +73,31 @@ gstack_hook_decision() { # /guard, /unfreeze, /investigate) resolve through gstack-paths; a reader that # used a different chain failed OPEN whenever GSTACK_HOME was set (#1459). # test/hook-scripts.test.ts pins parity against gstack-paths. +# Printed WITHOUT a trailing newline: callers capture with a sentinel +# (`r="$(gstack_hook_state_root; printf x)"; r="${r%x}"`) so a root that +# itself ends in a newline round-trips exactly as gstack-paths' %q does — +# otherwise writer and reader would again disagree on the directory. gstack_hook_state_root() { if [ -n "${GSTACK_HOME:-}" ]; then - printf '%s\n' "$GSTACK_HOME" + printf '%s' "$GSTACK_HOME" elif [ -n "${CLAUDE_PLUGIN_DATA:-}" ] && printf '%s' "${CLAUDE_PLUGIN_ROOT:-}" | grep -qi "gstack"; then - printf '%s\n' "$CLAUDE_PLUGIN_DATA" + printf '%s' "$CLAUDE_PLUGIN_DATA" elif [ -n "${HOME:-}" ]; then - printf '%s\n' "$HOME/.gstack" + printf '%s' "$HOME/.gstack" else - printf '%s\n' ".gstack" + printf '%s' ".gstack" fi } # gstack_hook_log_fire SKILL PATTERN # Append a hook_fire analytics record (pattern name only, never command -# content). Resolves the state root through gstack_hook_state_root so tests -# never pollute the operator's real analytics file. Best-effort: failures -# never affect the hook decision. +# content). Respects GSTACK_HOME so tests never pollute the operator's real +# analytics file. Deliberately NOT gstack_hook_state_root: every other +# analytics writer and reader (gstack-skill-start, gstack-retro-metrics, +# gstack-analytics) uses this two-step chain, and the usage log must stay one +# file. Best-effort: failures never affect the hook decision. gstack_hook_log_fire() { - _ghlf_dir="$(gstack_hook_state_root)/analytics" + _ghlf_dir="${GSTACK_HOME:-$HOME/.gstack}/analytics" mkdir -p "$_ghlf_dir" 2>/dev/null || true # Fields are JSON-encoded (a repo basename can carry quotes/backslashes) — # same rule this file states for decisions: never raw-interpolate into JSON. diff --git a/freeze/bin/check-freeze.sh b/freeze/bin/check-freeze.sh index 6e76da78e..803dc9494 100755 --- a/freeze/bin/check-freeze.sh +++ b/freeze/bin/check-freeze.sh @@ -36,7 +36,14 @@ fi # fails open: with GSTACK_HOME set, /freeze wrote freeze-dir.txt under # GSTACK_HOME while this hook read $HOME/.gstack, found nothing, and allowed # everything (#1459, #1509). gstack_hook_state_root mirrors gstack-paths. -STATE_DIR="$(gstack_hook_state_root)" +# A helper from an older install that lacks the function must fail CLOSED +# (the existence check above only proves the file sourced), never exit 127 +# with no JSON — Claude Code treats that as non-blocking. +if ! command -v gstack_hook_state_root >/dev/null 2>&1; then + printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"deny","permissionDecisionReason":"[freeze] Hook helpers out of date (partial upgrade?) - blocked, fail closed. Re-run ./setup or /unfreeze."}}\n' + exit 0 +fi +STATE_DIR="$(gstack_hook_state_root; printf x)"; STATE_DIR="${STATE_DIR%x}" FREEZE_FILE="$STATE_DIR/freeze-dir.txt" # If no freeze file exists, allow everything (not yet configured) diff --git a/test/hook-scripts.test.ts b/test/hook-scripts.test.ts index a274b9c00..2a3514e7c 100644 --- a/test/hook-scripts.test.ts +++ b/test/hook-scripts.test.ts @@ -694,6 +694,28 @@ describe('check-careful.sh', () => { }); }); + test('an older hook-extract.sh without gstack_hook_state_root still loads rules from $HOME/.gstack and emits a decision (no set -e death)', () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-careful-oldhelper-')); + const carefulBin = path.join(base, 'careful', 'bin'); + fs.mkdirSync(carefulBin, { recursive: true }); + fs.copyFileSync(CAREFUL_SCRIPT, path.join(carefulBin, 'check-careful.sh')); + const helper = fs.readFileSync(HOOK_EXTRACT, 'utf-8'); + const start = helper.indexOf('gstack_hook_state_root() {'); + const end = helper.indexOf('\n}\n', start) + 3; + fs.writeFileSync(path.join(carefulBin, 'hook-extract.sh'), helper.slice(0, start) + helper.slice(end)); + const fakeHome = path.join(base, 'home'); + fs.mkdirSync(path.join(fakeHome, '.gstack'), { recursive: true }); + fs.writeFileSync(path.join(fakeHome, '.gstack', 'careful-patterns.txt'), 'terraform\\s+destroy\n'); + try { + const { exitCode, output } = runHook(path.join(carefulBin, 'check-careful.sh'), carefulInput('terraform destroy'), { HOME: fakeHome, GSTACK_HOME: '' }); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('ask'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('Project rule'); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); + test('safe commands still allow with a pattern file present', () => { withPatternFile('terraform\\s+destroy\n', (gstackHome) => { const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('ls -la'), { GSTACK_HOME: gstackHome }); @@ -1004,3 +1026,111 @@ describe('check-freeze.sh state-root resolution (#1459 / #1509)', () => { } }); }); + +// ============================================================ +// gstack_hook_log_fire analytics sink follows the same state root (#1459) +// ============================================================ +// The hook_fire record lands under ${GSTACK_HOME:-$HOME/.gstack}/analytics — +// the SAME two-step chain every other analytics writer and reader uses +// (gstack-skill-start, gstack-retro-metrics, gstack-analytics) — deliberately +// NOT the plugin-aware state root the freeze FILE uses, so the usage log stays +// one file. Logging is best-effort: an unwritable sink never changes the decision. +describe('gstack_hook_log_fire writes under the resolved state root', () => { + const BOUNDARY = '/Users/dev/project/src/'; + const OUTSIDE = '/Users/dev/other-project/index.ts'; + + function withEmptyDir(fn: (dir: string) => void) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-logfire-')); + try { fn(dir); } finally { fs.rmSync(dir, { recursive: true, force: true }); } + } + function lastRecord(file: string): any { + const lines = fs.readFileSync(file, 'utf-8').trim().split('\n'); + return JSON.parse(lines[lines.length - 1]); + } + + test('REGRESSION: a freeze deny under GSTACK_HOME appends hook_fire to $GSTACK_HOME/analytics, not $HOME/.gstack', () => { + withFreezeDir(BOUNDARY, (gstackHome) => { + withEmptyDir((fakeHome) => { + const { exitCode, output } = runHook(FREEZE_SCRIPT, freezeInput(OUTSIDE), { + GSTACK_HOME: gstackHome, HOME: fakeHome, CLAUDE_PLUGIN_DATA: '', CLAUDE_PLUGIN_ROOT: '', + }); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + const rec = lastRecord(path.join(gstackHome, 'analytics', 'skill-usage.jsonl')); + expect(rec.event).toBe('hook_fire'); + expect(rec.skill).toBe('freeze'); + expect(rec.pattern).toBe('boundary_deny'); + expect(typeof rec.ts).toBe('string'); + expect(fs.existsSync(path.join(fakeHome, '.gstack'))).toBe(false); + }); + }); + }); + + test('plugin install: the freeze FILE is read from CLAUDE_PLUGIN_DATA but hook_fire still lands under $HOME/.gstack/analytics (one usage log)', () => { + withFreezeDir(BOUNDARY, (pluginData) => { + withEmptyDir((fakeHome) => { + const { output } = runHook(FREEZE_SCRIPT, freezeInput(OUTSIDE), + freezeEnv(pluginData, { HOME: fakeHome, CLAUDE_PLUGIN_ROOT: '/Plugins/GSTACK' })); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + const rec = lastRecord(path.join(fakeHome, '.gstack', 'analytics', 'skill-usage.jsonl')); + expect(rec.event).toBe('hook_fire'); + expect(rec.skill).toBe('freeze'); + expect(fs.existsSync(path.join(pluginData, 'analytics'))).toBe(false); + }); + }); + }); + + test('a GSTACK_HOME ending in a newline round-trips exactly (writer %q and reader sentinel agree)', () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-freeze-nl-')); + const nlDir = path.join(base, 'root\n'); + fs.mkdirSync(nlDir); + fs.writeFileSync(path.join(nlDir, 'freeze-dir.txt'), BOUNDARY); + try { + const { output } = runHook(FREEZE_SCRIPT, freezeInput(OUTSIDE), { + GSTACK_HOME: nlDir, HOME: base, CLAUDE_PLUGIN_DATA: '', CLAUDE_PLUGIN_ROOT: '', + }); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); + + test('a hook helper from an older install that lacks gstack_hook_state_root DENIES (fail closed), never exit 127', () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-freeze-oldhelper-')); + const freezeBin = path.join(base, 'freeze', 'bin'); + const carefulBin = path.join(base, 'careful', 'bin'); + fs.mkdirSync(freezeBin, { recursive: true }); + fs.mkdirSync(carefulBin, { recursive: true }); + fs.copyFileSync(FREEZE_SCRIPT, path.join(freezeBin, 'check-freeze.sh')); + const helper = fs.readFileSync(HOOK_EXTRACT, 'utf-8'); + const start = helper.indexOf('gstack_hook_state_root() {'); + const end = helper.indexOf('\n}\n', start) + 3; + fs.writeFileSync(path.join(carefulBin, 'hook-extract.sh'), helper.slice(0, start) + helper.slice(end)); + try { + withFreezeDir(BOUNDARY, (stateDir) => { + const { exitCode, output } = runHook(path.join(freezeBin, 'check-freeze.sh'), freezeInput('/Users/dev/project/src/x.ts'), freezeEnv(stateDir)); + expect(exitCode).toBe(0); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('fail closed'); + }); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); + + test('an unwritable analytics sink never changes the decision: deny is still emitted as valid JSON', () => { + withFreezeDir(BOUNDARY, (gstackHome) => { + // `analytics` is a regular FILE, so mkdir -p and the >> append both fail. + fs.writeFileSync(path.join(gstackHome, 'analytics'), 'not a directory'); + withEmptyDir((fakeHome) => { + const { exitCode, output, raw } = runHook(FREEZE_SCRIPT, freezeInput(OUTSIDE), { + GSTACK_HOME: gstackHome, HOME: fakeHome, CLAUDE_PLUGIN_DATA: '', CLAUDE_PLUGIN_ROOT: '', + }); + expect(exitCode).toBe(0); + expect(() => JSON.parse(raw)).not.toThrow(); + expect(output.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(fs.readFileSync(path.join(gstackHome, 'analytics'), 'utf-8')).toBe('not a directory'); + }); + }); + }); +});