From b964eed524bcfaca778d22f299cbaa2a9a957175 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 6 Oct 2026 20:28:26 +0000 Subject: [PATCH] fix(auto-update): hold an update whose hooks do not parse, before the checkout moves bin/gstack-session-update fast-forwards the live checkout before setup runs, and Claude Code runs hooks straight from it, so setup's gate cannot protect an auto-update. After the Bun floor check the updater extracts the incoming revision with git archive and runs bin/gstack-hook-check on it; a failure holds the update like bun-too-old, records hook-does-not-parse with file:line in the update status, and the next session start says the current hooks keep running. Fixture-remote tests cover a broken shim, a broken imported module and a parseable control. Co-authored-by: BenjaminDSmithy --- bin/gstack-session-update | 40 +++++-- docs/troubleshooting.md | 13 +++ test/session-update-hook-hold.test.ts | 161 ++++++++++++++++++++++++++ 3 files changed, 207 insertions(+), 7 deletions(-) create mode 100644 test/session-update-hook-hold.test.ts diff --git a/bin/gstack-session-update b/bin/gstack-session-update index 259ec2aa9..01e7c0a1a 100755 --- a/bin/gstack-session-update +++ b/bin/gstack-session-update @@ -8,7 +8,12 @@ # Before the live checkout moves, the updater fetches, reads the incoming # release's Bun floor (bin/gstack-bun-version.sh) and checks the `bun` setup # will run; below it the update is held with reason bun-too-old and nothing -# changes (E1). Updaters older than this check cannot protect their first pull. +# changes (E1). Then it parse-checks every hook the incoming revision registers +# (bin/gstack-hook-check on a `git archive` of that revision): Claude Code runs +# hooks straight from this checkout, so an unparseable hook would block tool +# calls the moment the checkout moved. A failure holds the update with reason +# hook-does-not-parse and its file:line. Updaters older than a check cannot +# protect their first pull. # # Stages (B11): pull, setup and migrations are tracked separately. The # "just upgraded" marker is written only when every stage succeeded. A failed @@ -37,12 +42,28 @@ THROTTLE_FILE="$STATE_DIR/.last-session-update" LOCK_DIR="$STATE_DIR/.setup-lock" LOG_FILE="$STATE_DIR/analytics/session-update.log" PENDING_FILE="$STATE_DIR/session-update-pending" +HOOK_CHECK="$(cd "$(dirname "$0")" && pwd)/gstack-hook-check" THROTTLE_SECONDS=3600 # 1 hour log_entry() { mkdir -p "$(dirname "$LOG_FILE")" echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) $1" >> "$LOG_FILE" 2>/dev/null || true # best-effort: logging never blocks a session } +# hook_incoming_hold — parse-check the hooks of (extracted with +# git archive) before the live checkout moves. Prints the held reason and +# returns 0 when a hook does not parse; otherwise prints nothing and returns 1 +# (an archive or bun failure fails open, like every other error here). +hook_incoming_hold() { + _hc_dir=$(mktemp -d "${TMPDIR:-/tmp}/gstack-session-hooks-XXXXXX" 2>/dev/null) || return 1 + _hc_out="" + if git -C "$GSTACK_DIR" archive --format=tar "$1" 2>/dev/null | tar -xf - -C "$_hc_dir" 2>/dev/null; then + _hc_out=$("$HOOK_CHECK" "$_hc_dir" 2>/dev/null) + fi + rm -rf "$_hc_dir" + _hc_fail=$(printf '%s\n' "$_hc_out" | sed -n 's/^fail [^ ]* //p' | head -1) + [ -n "$_hc_fail" ] || return 1 + printf 'hook-does-not-parse: %s\n' "${_hc_fail#"$_hc_dir"/}" | head -c 300 +} pending_get() { sed -n "s/^$1=//p" "$PENDING_FILE" 2>/dev/null | head -1; } # pending_save — persist the P_* stage state; nothing pending removes the file. pending_save() { @@ -74,6 +95,8 @@ if [ -f "$PENDING_FILE" ]; then case "$_P_PULL" in bun-too-old*) echo "gstack auto-update: update held ($_P_PULL); nothing was installed or changed. Fix now: bun upgrade && cd $GSTACK_DIR && git pull --ff-only && ./setup" ;; + hook-does-not-parse*) + echo "gstack auto-update: update held ($_P_PULL); nothing was installed or changed, and your current hooks keep running. This is a gstack bug; report ${_P_PULL#hook-does-not-parse: } at https://github.com/garrytan/gstack/issues. gstack retries at the next check. https://github.com/garrytan/gstack/blob/main/docs/troubleshooting.md#auto-update-hook-does-not-parse" ;; *) echo "gstack auto-update: git pull did not finish ($_P_PULL); gstack is not updating. Fix now: cd $GSTACK_DIR && git pull --ff-only && ./setup" ;; esac @@ -188,14 +211,17 @@ fi # The fast-forward targets the exact revision whose requirement was read. # Setup runs `bun` from this PATH, so that is the binary checked; a # malformed version passes here because setup only warns on it. - INCOMING="" BUN_HELD="" + INCOMING="" HELD="" if [ "$PULL_EXIT" -eq 0 ]; then INCOMING=$(git -C "$GSTACK_DIR" rev-parse --verify '@{upstream}^{commit}' 2>"${PULL_ERR_FILE:-/dev/null}") || PULL_EXIT=1 fi if [ "$PULL_EXIT" -eq 0 ] && [ "$INCOMING" != "$OLD_HEAD" ]; then - BUN_HELD=$(gstack_bun_incoming_hold "$GSTACK_DIR" "$INCOMING" 2>/dev/null) || BUN_HELD="" + HELD=$(gstack_bun_incoming_hold "$GSTACK_DIR" "$INCOMING" 2>/dev/null) || HELD="" fi - if [ "$PULL_EXIT" -eq 0 ] && [ -z "$BUN_HELD" ]; then + if [ "$PULL_EXIT" -eq 0 ] && [ -z "$HELD" ] && [ "$INCOMING" != "$OLD_HEAD" ]; then + HELD=$(hook_incoming_hold "$INCOMING") || HELD="" + fi + if [ "$PULL_EXIT" -eq 0 ] && [ -z "$HELD" ]; then git -C "$GSTACK_DIR" merge --ff-only --autostash -q "$INCOMING" >/dev/null 2>"${PULL_ERR_FILE:-/dev/null}" PULL_EXIT=$? fi @@ -212,9 +238,9 @@ fi case "$P_FAILURES" in ''|*[!0-9]*) P_FAILURES=0 ;; esac case "$P_NEXT" in ''|*[!0-9]*) P_NEXT=0 ;; esac - if [ -n "$BUN_HELD" ]; then - log_entry "HELD $BUN_HELD incoming=$INCOMING" - P_PULL="$BUN_HELD" + if [ -n "$HELD" ]; then + log_entry "HELD $HELD incoming=$INCOMING" + P_PULL="$HELD" pending_save rm -f "$PULL_ERR_FILE" 2>/dev/null exit 0 diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 6cecc0bd3..13217c1af 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -541,6 +541,19 @@ cd ~/.claude/skills/gstack && ./setup **Expected result.** Setup finishes with exit 0 and no refusal line. + +### `gstack auto-update: update held (hook-does-not-parse: :: ); nothing was installed or changed, and your current hooks keep running. ...` + +**Meaning.** Team-mode auto-update fetched a release with a hook that does not +parse. It checked the incoming revision before moving your checkout, so your +checkout, installed skills and registered hooks stay at the current revision. +gstack checks again at the next update check and installs the first release +whose hooks parse. This is a gstack bug: report the printed `:`. + +**Fix.** Nothing to do locally. A manual `git pull` followed by `./setup` +cannot be checked before the pull; setup then refuses the broken hook (see the +entry above). + ### `CSO unavailable: its native helper was not built (windows-msvc-compile)` diff --git a/test/session-update-hook-hold.test.ts b/test/session-update-hook-hold.test.ts new file mode 100644 index 000000000..baf954a83 --- /dev/null +++ b/test/session-update-hook-hold.test.ts @@ -0,0 +1,161 @@ +import { afterEach, describe, expect, test } from 'bun:test'; +import { execFileSync, spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +// The auto-updater fast-forwards the live checkout before setup runs, and +// Claude Code runs hooks straight from that checkout, so setup's parse gate +// alone cannot protect an auto-update. The updater parse-checks every hook of +// the incoming revision (git archive into a temp dir, bin/gstack-hook-check) +// before the fast-forward; a failure holds the update like a too-old Bun: +// the checkout does not move and the installed hooks keep running. + +const ROOT = path.resolve(import.meta.dir, '..'); +const SCRIPT = path.join(ROOT, 'bin', 'gstack-session-update'); +const HOOK = 'hosts/claude/hooks/question-log-hook'; +const bases: string[] = []; +afterEach(() => { for (const b of bases.splice(0)) fs.rmSync(b, { recursive: true, force: true }); }); + +function git(cwd: string, ...args: string[]): string { + return execFileSync('git', args, { cwd, encoding: 'utf8', timeout: 30_000 }).trim(); +} + +function makeFixture() { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-supd-hooks-')); + bases.push(base); + const origin = path.join(base, 'origin.git'); + const seed = path.join(base, 'seed'); + const install = path.join(base, 'install'); + const state = path.join(base, 'state'); + const home = path.join(base, 'home'); + fs.mkdirSync(state, { recursive: true }); + fs.mkdirSync(path.join(home, '.claude'), { recursive: true }); + execFileSync('git', ['init', '-q', '--bare', '-b', 'main', origin], { timeout: 30_000 }); + fs.mkdirSync(path.join(seed, 'bin'), { recursive: true }); + fs.mkdirSync(path.join(seed, path.dirname(HOOK)), { recursive: true }); + fs.writeFileSync(path.join(seed, 'VERSION'), '1.0.0\n'); + fs.writeFileSync(path.join(seed, 'bin', 'gstack-config'), + '#!/usr/bin/env bash\nif [ "$1" = "get" ]; then case "$2" in auto_upgrade) echo true;; skill_prefix) echo false;; *) echo "";; esac; fi\nexit 0\n', { mode: 0o755 }); + fs.writeFileSync(path.join(seed, 'bin', 'gstack-patch-names'), '#!/usr/bin/env bash\nexit 0\n', { mode: 0o755 }); + // Stub setup: records each run, and registers the hook the way real setup + // does, which is where the check reads the hook list from. + fs.writeFileSync(path.join(seed, 'setup'), [ + '#!/usr/bin/env bash', + 'echo "$*" >> "$SETUP_CALLS"', + `LOG_HOOK="$(_hook_command_path ${HOOK} || true)"`, + 'exit 0', + ].join('\n') + '\n', { mode: 0o755 }); + fs.copyFileSync(path.join(ROOT, HOOK), path.join(seed, HOOK)); + fs.chmodSync(path.join(seed, HOOK), 0o755); + fs.writeFileSync(path.join(seed, `${HOOK}.ts`), "import { note } from './note';\nconsole.log(note);\n"); + fs.writeFileSync(path.join(seed, path.dirname(HOOK), 'note.ts'), "export const note = 'old hook ran';\n"); + git(seed, 'init', '-q'); + git(seed, 'add', '-A'); + git(seed, 'commit', '-q', '-m', 'seed'); + git(seed, 'branch', '-M', 'main'); + git(seed, 'remote', 'add', 'origin', origin); + git(seed, 'push', '-q', 'origin', 'main'); + execFileSync('git', ['clone', '-q', origin, install], { timeout: 30_000 }); + // An existing registration pointing into the live checkout. + const settings = path.join(home, '.claude', 'settings.json'); + const hookCmd = path.join(install, HOOK); + fs.writeFileSync(settings, JSON.stringify({ hooks: { PostToolUse: [{ matcher: 'AskUserQuestion', hooks: [{ type: 'command', command: hookCmd }] }] } }, null, 2)); + return { base, seed, install, state, home, settings, hookCmd, calls: path.join(base, 'calls') }; +} +type Fx = ReturnType; + +function release(fx: Fx, version: string, edit: () => void) { + edit(); + fs.writeFileSync(path.join(fx.seed, 'VERSION'), `${version}\n`); + git(fx.seed, 'add', '-A'); + git(fx.seed, 'commit', '-qm', `release ${version}`); + git(fx.seed, 'push', '-q', 'origin', 'main'); +} + +function runHook(fx: Fx) { + return spawnSync('bash', [SCRIPT], { + encoding: 'utf8', + env: { PATH: `${path.dirname(process.execPath)}:${process.env.PATH}`, HOME: fx.home, GSTACK_DIR: fx.install, GSTACK_STATE_ROOT: fx.state, SETUP_CALLS: fx.calls, TMPDIR: fx.base }, + timeout: 20_000, + }); +} + +async function waitFor(fx: Fx, pattern: RegExp, ms = 15_000): Promise { + const logFile = path.join(fx.state, 'analytics', 'session-update.log'); + const deadline = Date.now() + ms; + let content = ''; + while (Date.now() < deadline) { + content = fs.existsSync(logFile) ? fs.readFileSync(logFile, 'utf8') : ''; + if (pattern.test(content) && !fs.existsSync(path.join(fx.state, '.setup-lock'))) return content; + await new Promise(r => setTimeout(r, 100)); + } + throw new Error(`timed out waiting for ${pattern}; log:\n${content}`); +} + +/** Run the hook the existing registration names, as Claude Code would. */ +function runRegisteredHook(fx: Fx) { + const settings = JSON.parse(fs.readFileSync(fx.settings, 'utf8')); + const cmd = settings.hooks.PostToolUse[0].hooks[0].command as string; + return spawnSync('/bin/sh', ['-c', cmd], { encoding: 'utf8', input: '{}', env: { PATH: `${path.dirname(process.execPath)}:${process.env.PATH}` }, timeout: 20_000 }); +} + +describe.skipIf(process.platform === 'win32')('gstack-session-update: incoming hooks are parse-checked before the fast-forward', () => { + test('an unparseable shim at the incoming revision holds the update; the checkout and the old hook stay', async () => { + const fx = makeFixture(); + const oldHead = git(fx.install, 'rev-parse', 'HEAD'); + const shimLines = fs.readFileSync(path.join(fx.seed, HOOK), 'utf8').split('\n').length; + release(fx, '1.1.0', () => fs.appendFileSync(path.join(fx.seed, HOOK), 'if then\n')); + + expect(runHook(fx).stdout).toBe(''); + const log = await waitFor(fx, /Z HELD hook-does-not-parse/); + expect(log).toContain(`HELD hook-does-not-parse: ${HOOK}:${shimLines}: syntax error`); + expect(git(fx.install, 'rev-parse', 'HEAD')).toBe(oldHead); + expect(fs.readFileSync(path.join(fx.install, 'VERSION'), 'utf8')).toBe('1.0.0\n'); + expect(fs.existsSync(fx.calls)).toBe(false); + expect(fs.readFileSync(path.join(fx.state, 'session-update-pending'), 'utf8')).toContain(`pull=hook-does-not-parse: ${HOOK}:${shimLines}:`); + expect(fs.existsSync(path.join(fx.state, 'just-upgraded-from'))).toBe(false); + const ran = runRegisteredHook(fx); + expect(ran.status).toBe(0); + expect(ran.stdout).toContain('old hook ran'); + // No leftover archive extraction. + expect(fs.readdirSync(fx.base).filter(f => f.startsWith('gstack-session-hooks-'))).toEqual([]); + + // The next session start says what was held, where, and that the old hooks run. + const next = runHook(fx).stdout; + expect(next).toContain(`gstack auto-update: update held (hook-does-not-parse: ${HOOK}:${shimLines}:`); + expect(next).toContain('your current hooks keep running'); + expect(next).toContain(`This is a gstack bug; report ${HOOK}:${shimLines}: `); + expect(next).toContain('https://github.com/garrytan/gstack/blob/main/docs/troubleshooting.md#auto-update-hook-does-not-parse'); + }); + + test('a broken module the incoming TypeScript entry imports holds the update', async () => { + const fx = makeFixture(); + const oldHead = git(fx.install, 'rev-parse', 'HEAD'); + release(fx, '1.1.0', () => fs.writeFileSync(path.join(fx.seed, path.dirname(HOOK), 'note.ts'), 'export const note = ;\n')); + runHook(fx); + const log = await waitFor(fx, /Z HELD /); + expect(log).toContain(`HELD hook-does-not-parse: ${path.dirname(HOOK)}/note.ts:1:`); + expect(git(fx.install, 'rev-parse', 'HEAD')).toBe(oldHead); + expect(runRegisteredHook(fx).stdout).toContain('old hook ran'); + }); + + test('a parseable incoming revision fast-forwards and runs setup (control)', async () => { + const fx = makeFixture(); + release(fx, '1.1.0', () => fs.writeFileSync(path.join(fx.seed, path.dirname(HOOK), 'note.ts'), "export const note = 'new hook ran';\n")); + runHook(fx); + await waitFor(fx, /Z UPDATED from=1\.0\.0 to=1\.1\.0/); + expect(fs.readFileSync(path.join(fx.install, 'VERSION'), 'utf8')).toBe('1.1.0\n'); + expect(runRegisteredHook(fx).stdout).toContain('new hook ran'); + }); + + test('the hook check runs after the Bun floor check and before the fast-forward', () => { + const src = fs.readFileSync(SCRIPT, 'utf8'); + const bun = src.indexOf('HELD=$(gstack_bun_incoming_hold'); + const hooks = src.indexOf('HELD=$(hook_incoming_hold'); + const ff = src.indexOf('merge --ff-only'); + expect(bun).toBeGreaterThan(-1); + expect(hooks).toBeGreaterThan(bun); + expect(ff).toBeGreaterThan(hooks); + }); +});