From 4238cd8313b13571082731c34fcfac585cacc133 Mon Sep 17 00:00:00 2001 From: Mo Date: Wed, 3 Jun 2026 10:01:56 +0200 Subject: [PATCH] fix(hook): tolerate branches without tracked specs/INDEX.md + e2e suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second real-user pre-commit-hook bug surfaced on `sensor-sender`: checking out a branch that predates the first `regen` (or a fresh init pre-regen) carried `.zettelgeist.yaml` but had no `specs/INDEX.md` in its history, so the hook's `regen --check` errored with `specs/INDEX.md is missing` and blocked every commit on the branch. Fix: HOOK_BLOCK gains a second pre-flight — git ls-files --error-unmatch specs/INDEX.md >/dev/null 2>&1 || exit 0 If the index file isn't tracked on the current branch there's nothing to be stale about, and the hook exits 0. Re-run `install-hook` to update existing hooks (they pick up the new block via marker-region replacement, no config change needed). Add an end-to-end pre-commit suite under packages/git-hook/tests that spins up real git repos, installs the hook through the same code path users hit, and drives `git commit` through five states: 1. non-zg repo → commit passes; CLI not invoked 2. zg repo, INDEX.md not in HEAD → commit passes; CLI not invoked 3. zg repo, INDEX.md in HEAD + check passes → commit passes; CLI invoked 4. zg repo, INDEX.md in HEAD + check fails → commit blocked; CLI invoked 5. checkout to a pre-INDEX branch + commit → commit passes; CLI not invoked The CLI is stubbed with a small `zettelgeist` shell shim on PATH so we can deterministically drive both the success-path and the failure-path and assert pre-flight reachability via a touched marker file. Tests (1) and (5) regression-guard the two real bugs users have hit; the others pin the "actually-stale state must still block" invariant the hook exists for. --- .../hook-skips-when-index-not-tracked.md | 5 + packages/git-hook/src/install-hook.ts | 20 +- packages/git-hook/tests/install-hook.test.ts | 6 + .../git-hook/tests/pre-commit-e2e.test.ts | 244 ++++++++++++++++++ 4 files changed, 271 insertions(+), 4 deletions(-) create mode 100644 .changeset/hook-skips-when-index-not-tracked.md create mode 100644 packages/git-hook/tests/pre-commit-e2e.test.ts diff --git a/.changeset/hook-skips-when-index-not-tracked.md b/.changeset/hook-skips-when-index-not-tracked.md new file mode 100644 index 0000000..91895a2 --- /dev/null +++ b/.changeset/hook-skips-when-index-not-tracked.md @@ -0,0 +1,5 @@ +--- +'@zettelgeist/cli': patch +--- + +Pre-commit hook now also self-disables on branches where `specs/INDEX.md` is not part of the branch's git history. Before this, a back-in-time checkout (or a fresh `zettelgeist init` before the first `regen`) blocked every commit with `error: specs/INDEX.md is missing` because the hook's `regen --check` had nothing to compare against. The hook now performs a `git ls-files --error-unmatch specs/INDEX.md` pre-flight: if the index file isn't tracked on this branch, there's nothing to be stale about and the hook exits 0. Re-run `zettelgeist install-hook` to update existing hooks. The full pre-commit flow now also has end-to-end test coverage in `@zettelgeist/git-hook`'s test suite, regression-guarding both this bug and the earlier non-zg-repo bug. diff --git a/packages/git-hook/src/install-hook.ts b/packages/git-hook/src/install-hook.ts index 32791cc..b99c9b1 100644 --- a/packages/git-hook/src/install-hook.ts +++ b/packages/git-hook/src/install-hook.ts @@ -9,13 +9,25 @@ export const HOOK_MARKER_END = '# <<< zettelgeist <<<'; // run with the user's login PATH, which won't include ./node_modules/.bin — // so we fall back to the workspace-local binary if PATH lookup misses. // -// The leading `.zettelgeist.yaml` guard makes the hook self-disabling in -// repos that aren't zettelgeist repos: a stale install left over from a -// removed config, or a partial init, would otherwise block every commit -// with `error: not a zettelgeist repo`. +// Two pre-flight guards keep the hook from blocking commits in states +// where its check is meaningless: +// +// 1. No `.zettelgeist.yaml` → not a zettelgeist repo. A stale install +// from a removed config or a partial init would otherwise fail every +// commit with `error: not a zettelgeist repo`. +// +// 2. `specs/INDEX.md` not tracked in HEAD on this branch → the index +// file isn't part of this branch's history (back-in-time checkout, +// fresh repo before the first regen, branch that predates the +// `init` commit). There's nothing to be stale about; without this +// guard, `regen --check` fails with `specs/INDEX.md is missing` and +// the user can't commit on the branch. We hardcode the `specs/` +// path because `specs_dir` overrides are out of scope for the +// shell-level guard; users who override can write their own hook. export const HOOK_BLOCK = HOOK_MARKER_BEGIN + '\n' + '[ -f .zettelgeist.yaml ] || exit 0\n' + + 'git ls-files --error-unmatch specs/INDEX.md >/dev/null 2>&1 || exit 0\n' + 'if command -v zettelgeist >/dev/null 2>&1; then\n' + ' zettelgeist regen --check\n' + 'elif [ -x ./node_modules/.bin/zettelgeist ]; then\n' + diff --git a/packages/git-hook/tests/install-hook.test.ts b/packages/git-hook/tests/install-hook.test.ts index 98beab0..e380238 100644 --- a/packages/git-hook/tests/install-hook.test.ts +++ b/packages/git-hook/tests/install-hook.test.ts @@ -54,6 +54,12 @@ describe('mergeHookContent', () => { it('HOOK_BLOCK self-disables when .zettelgeist.yaml is missing', () => { expect(HOOK_BLOCK).toContain('[ -f .zettelgeist.yaml ] || exit 0'); }); + + it('HOOK_BLOCK self-disables when specs/INDEX.md is not tracked in HEAD', () => { + expect(HOOK_BLOCK).toContain( + 'git ls-files --error-unmatch specs/INDEX.md >/dev/null 2>&1 || exit 0', + ); + }); }); describe('HOOK_BLOCK execution', () => { diff --git a/packages/git-hook/tests/pre-commit-e2e.test.ts b/packages/git-hook/tests/pre-commit-e2e.test.ts new file mode 100644 index 0000000..fdea545 --- /dev/null +++ b/packages/git-hook/tests/pre-commit-e2e.test.ts @@ -0,0 +1,244 @@ +/** + * End-to-end pre-commit hook tests. + * + * Spins up real git repos, installs the hook via the same code path + * users run (`installPreCommitHook`), and drives `git commit` through + * every state the hook is supposed to handle. These tests exist to + * regression-guard the two real bugs that hit users on `sensor-sender`: + * + * 1. A stale install in a non-zg repo blocked every commit with + * `error: not a zettelgeist repo`. + * 2. A back-in-time checkout where `specs/INDEX.md` was not yet part + * of branch history blocked commits with `specs/INDEX.md is + * missing`, because the hook ran `regen --check` unconditionally. + * + * The hook's "should I run the check at all?" pre-flight is pure shell; + * the `regen --check` invocation behind it is the CLI's job and is + * unit-tested elsewhere. We stub the CLI here so we can observe whether + * the pre-flight reached it (skip cases must NOT reach the stub; the + * pass-through case MUST). + */ + +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { execFile } from 'node:child_process'; +import { promises as fs } from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { promisify } from 'node:util'; +import { installPreCommitHook } from '../src/install-hook.js'; + +const execFileP = promisify(execFile); + +async function setupRepo(): Promise { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'zg-hook-e2e-')); + await execFileP('git', ['init', '-q'], { cwd: tmp }); + // Deterministic identity so commits don't bail on missing config. + await execFileP('git', ['config', 'user.email', 'test@e2e.local'], { cwd: tmp }); + await execFileP('git', ['config', 'user.name', 'E2E'], { cwd: tmp }); + // Some CIs default to no initial branch; pin one for repeatability. + await execFileP('git', ['symbolic-ref', 'HEAD', 'refs/heads/main'], { cwd: tmp }); + return tmp; +} + +/** + * Write a `zettelgeist` stub into a sandbox `bin/` dir so we can: + * (a) detect whether the hook's pre-flight skipped it (file present + * but stub-was-called marker absent), and + * (b) force a deterministic pass/fail when the pre-flight does invoke + * it. + * + * Returns the bin dir; prepend it to PATH when running commits to + * make the stub the resolution target of `command -v zettelgeist`. + */ +async function installZettelgeistStub( + repo: string, + opts: { exitCode: number; marker?: string }, +): Promise { + const binDir = path.join(repo, '.test-bin'); + await fs.mkdir(binDir, { recursive: true }); + const stubPath = path.join(binDir, 'zettelgeist'); + const markerPath = path.join(repo, '.test-stub-was-called'); + // Touch the marker file so the test can assert the stub ran. Exit + // with the requested code so we can drive both pass and fail paths. + const body = + '#!/bin/sh\n' + + `touch "${markerPath}"\n` + + `exit ${opts.exitCode}\n`; + await fs.writeFile(stubPath, body); + await fs.chmod(stubPath, 0o755); + return binDir; +} + +async function stubWasCalled(repo: string): Promise { + return fs + .access(path.join(repo, '.test-stub-was-called')) + .then(() => true) + .catch(() => false); +} + +interface CommitOpts { + pathOverride?: string; + expectFail?: boolean; +} + +async function commitFile( + repo: string, + file: string, + content: string, + message: string, + opts: CommitOpts = {}, +): Promise<{ ok: boolean; stderr: string }> { + const abs = path.join(repo, file); + await fs.mkdir(path.dirname(abs), { recursive: true }); + await fs.writeFile(abs, content); + await execFileP('git', ['add', file], { cwd: repo }); + const env = opts.pathOverride + ? { ...process.env, PATH: opts.pathOverride } + : process.env; + try { + await execFileP('git', ['commit', '-q', '-m', message], { cwd: repo, env }); + return { ok: true, stderr: '' }; + } catch (err) { + const e = err as { stderr?: string }; + return { ok: false, stderr: e.stderr ?? '' }; + } +} + +describe('pre-commit hook end-to-end', () => { + let tmp: string; + + beforeEach(async () => { + tmp = await setupRepo(); + }); + + afterEach(async () => { + await fs.rm(tmp, { recursive: true, force: true }); + }); + + // ------------------------------------------------------------------ + // Pre-flight guard 1: non-zg repo (no .zettelgeist.yaml) + // ------------------------------------------------------------------ + + it('non-zg repo: hook does not block commits and never invokes zettelgeist', async () => { + // Reproduces the original Sander bug: a stale hook install in a + // repo that has no `.zettelgeist.yaml` was blocking every commit. + await installPreCommitHook(tmp); + const binDir = await installZettelgeistStub(tmp, { exitCode: 1 }); + + const r = await commitFile(tmp, 'hello.txt', 'hello\n', 'first', { + pathOverride: `${binDir}:/usr/bin:/bin`, + }); + + expect(r.ok).toBe(true); + expect(await stubWasCalled(tmp)).toBe(false); + }); + + // ------------------------------------------------------------------ + // Pre-flight guard 2: zg repo, but specs/INDEX.md not tracked on this + // branch (back-in-time checkout or fresh init pre-regen). + // ------------------------------------------------------------------ + + it('zg repo without tracked INDEX.md: hook does not block commits and never invokes zettelgeist', async () => { + // Reproduces the apple-updates branch bug: a branch that predates + // the first `regen` has `.zettelgeist.yaml` but no `specs/INDEX.md` + // in its history. The hook used to fail with `specs/INDEX.md is + // missing` and block every commit on that branch. + await fs.writeFile(path.join(tmp, '.zettelgeist.yaml'), 'format_version: "0.1"\n'); + await execFileP('git', ['add', '.zettelgeist.yaml'], { cwd: tmp }); + await execFileP('git', ['commit', '-q', '-m', 'init zg config'], { cwd: tmp }); + + await installPreCommitHook(tmp); + const binDir = await installZettelgeistStub(tmp, { exitCode: 1 }); + + const r = await commitFile(tmp, 'feature.txt', 'work\n', 'feature', { + pathOverride: `${binDir}:/usr/bin:/bin`, + }); + + expect(r.ok).toBe(true); + expect(await stubWasCalled(tmp)).toBe(false); + }); + + // ------------------------------------------------------------------ + // Hook reaches the binary when both guards pass. + // ------------------------------------------------------------------ + + it('zg repo with tracked INDEX.md: hook invokes zettelgeist regen --check', async () => { + await fs.writeFile(path.join(tmp, '.zettelgeist.yaml'), 'format_version: "0.1"\n'); + await fs.mkdir(path.join(tmp, 'specs'), { recursive: true }); + await fs.writeFile(path.join(tmp, 'specs', 'INDEX.md'), '# generated\n'); + await execFileP('git', ['add', '.'], { cwd: tmp }); + await execFileP('git', ['commit', '-q', '-m', 'init with INDEX'], { cwd: tmp }); + + await installPreCommitHook(tmp); + // Stub returns success → commit should pass; stub_was_called → true. + const binDir = await installZettelgeistStub(tmp, { exitCode: 0 }); + + const r = await commitFile(tmp, 'a.txt', 'a\n', 'add a', { + pathOverride: `${binDir}:/usr/bin:/bin`, + }); + + expect(r.ok).toBe(true); + expect(await stubWasCalled(tmp)).toBe(true); + }); + + it('zg repo with tracked INDEX.md but zettelgeist exits non-zero: commit is blocked', async () => { + await fs.writeFile(path.join(tmp, '.zettelgeist.yaml'), 'format_version: "0.1"\n'); + await fs.mkdir(path.join(tmp, 'specs'), { recursive: true }); + await fs.writeFile(path.join(tmp, 'specs', 'INDEX.md'), '# generated\n'); + await execFileP('git', ['add', '.'], { cwd: tmp }); + await execFileP('git', ['commit', '-q', '-m', 'init with INDEX'], { cwd: tmp }); + + await installPreCommitHook(tmp); + // Stub fails → commit must fail. This is the "INDEX is stale" + // protection the hook exists for. + const binDir = await installZettelgeistStub(tmp, { exitCode: 1 }); + + const r = await commitFile(tmp, 'a.txt', 'a\n', 'add a', { + pathOverride: `${binDir}:/usr/bin:/bin`, + }); + + expect(r.ok).toBe(false); + expect(await stubWasCalled(tmp)).toBe(true); + }); + + // ------------------------------------------------------------------ + // Cross-branch scenario: the hook does the right thing when checking + // out between a branch that has INDEX.md tracked and one that doesn't. + // ------------------------------------------------------------------ + + it('checking out a branch without tracked INDEX.md does not block commits there', async () => { + // Set up: main has INDEX.md tracked. Then create a branch from a + // commit that pre-dates INDEX.md and commit on it. This is the + // exact `apple-updates` shape Sander hit. + await fs.writeFile(path.join(tmp, '.zettelgeist.yaml'), 'format_version: "0.1"\n'); + await fs.writeFile(path.join(tmp, 'README.md'), '# repo\n'); + await execFileP('git', ['add', '.'], { cwd: tmp }); + await execFileP('git', ['commit', '-q', '-m', 'pre-zg root'], { cwd: tmp }); + + // Capture the pre-INDEX commit so we can branch from it later. + const { stdout: rootSha } = await execFileP('git', ['rev-parse', 'HEAD'], { cwd: tmp }); + + // Land INDEX.md on main. + await fs.mkdir(path.join(tmp, 'specs'), { recursive: true }); + await fs.writeFile(path.join(tmp, 'specs', 'INDEX.md'), '# generated\n'); + await execFileP('git', ['add', '.'], { cwd: tmp }); + await execFileP('git', ['commit', '-q', '-m', 'add INDEX'], { cwd: tmp }); + + // Install the hook AFTER both states exist so it lives across the + // checkout (the realistic install order). + await installPreCommitHook(tmp); + const binDir = await installZettelgeistStub(tmp, { exitCode: 1 }); + + // Branch off the pre-INDEX commit — `INDEX.md` is NOT in HEAD here. + await execFileP('git', ['checkout', '-q', '-b', 'apple-updates', rootSha.trim()], { + cwd: tmp, + }); + + const r = await commitFile(tmp, 'apple.txt', 'feature\n', 'apple feature', { + pathOverride: `${binDir}:/usr/bin:/bin`, + }); + + expect(r.ok).toBe(true); + expect(await stubWasCalled(tmp)).toBe(false); + }); +});