From 929f389d50f4d5588e0f678ddf7fb10da2745214 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Tue, 18 Aug 2026 17:55:31 -0600 Subject: [PATCH] fix(hooks): guard-git.sh commit edit-log check falls back to HOOK_CWD too Same root cause #2386 fixed for validate_branch_name: a bare git commit -m "msg" relying on the Bash tool's persistent cwd from an earlier, separate tool call leaves WORK_DIR empty, and the commit edit-log check fell back to the hook process's own ambient cwd instead of the payload's reported cwd -- resolving against a completely unrelated repo. Milder than #2386's bug: the wrong repo's git diff --cached comes back empty, hitting the existing "no staged files" early-out and silently allowing, rather than wrongly denying. But it means the edit-log protection (catching another session's files sneaking into your commit) doesn't actually run when it should. Applies the same HOOK_CWD fallback to this section, keeping .claude/hooks/guard-git.sh and docs/examples/claude-code-hooks/guard-git.sh byte-identical per the existing hook-guard-git-clean.test.ts check. Closes #2526 --- .claude/hooks/guard-git.sh | 13 ++ docs/examples/claude-code-hooks/guard-git.sh | 13 ++ ...hook-guard-git-commit-cwd-fallback.test.ts | 134 ++++++++++++++++++ 3 files changed, 160 insertions(+) create mode 100644 tests/unit/hook-guard-git-commit-cwd-fallback.test.ts diff --git a/.claude/hooks/guard-git.sh b/.claude/hooks/guard-git.sh index 88cfc9d95..a7fc7e1fa 100644 --- a/.claude/hooks/guard-git.sh +++ b/.claude/hooks/guard-git.sh @@ -332,6 +332,19 @@ if echo "$NCOMMAND" | grep -qE '(^|[[:space:]]|&&[[:space:]]*)git[[:space:]]+com # Resolve the target worktree so the edit log and staged-file listing come # from the same repo the commit targets (e.g. `git -C commit`). WORK_DIR=$(detect_work_dir commit) + + # No explicit -C/cd in the command text — fall back to the Bash tool's + # actual cwd for this call (reported by the harness on every PreToolUse + # payload), NOT the hook process's own ambient cwd. A bare + # `git commit -m "msg"` relying on a `cd` from an earlier, separate + # tool call has no directory hint in $COMMAND at all; substituting the + # hook's own cwd there resolves the edit-log check against a completely + # unrelated repo (#2526, the same root cause #2386 fixed for + # validate_branch_name). + if [ -z "$WORK_DIR" ] && [ -n "$HOOK_CWD" ] && [ -d "$HOOK_CWD" ]; then + WORK_DIR="$HOOK_CWD" + fi + MERGE_IN_PROGRESS=false if [ -n "$WORK_DIR" ] && [ -d "$WORK_DIR" ]; then PROJECT_DIR=$(git -C "$WORK_DIR" rev-parse --show-toplevel 2>/dev/null) || PROJECT_DIR="${CLAUDE_PROJECT_DIR:-.}" diff --git a/docs/examples/claude-code-hooks/guard-git.sh b/docs/examples/claude-code-hooks/guard-git.sh index 88cfc9d95..a7fc7e1fa 100644 --- a/docs/examples/claude-code-hooks/guard-git.sh +++ b/docs/examples/claude-code-hooks/guard-git.sh @@ -332,6 +332,19 @@ if echo "$NCOMMAND" | grep -qE '(^|[[:space:]]|&&[[:space:]]*)git[[:space:]]+com # Resolve the target worktree so the edit log and staged-file listing come # from the same repo the commit targets (e.g. `git -C commit`). WORK_DIR=$(detect_work_dir commit) + + # No explicit -C/cd in the command text — fall back to the Bash tool's + # actual cwd for this call (reported by the harness on every PreToolUse + # payload), NOT the hook process's own ambient cwd. A bare + # `git commit -m "msg"` relying on a `cd` from an earlier, separate + # tool call has no directory hint in $COMMAND at all; substituting the + # hook's own cwd there resolves the edit-log check against a completely + # unrelated repo (#2526, the same root cause #2386 fixed for + # validate_branch_name). + if [ -z "$WORK_DIR" ] && [ -n "$HOOK_CWD" ] && [ -d "$HOOK_CWD" ]; then + WORK_DIR="$HOOK_CWD" + fi + MERGE_IN_PROGRESS=false if [ -n "$WORK_DIR" ] && [ -d "$WORK_DIR" ]; then PROJECT_DIR=$(git -C "$WORK_DIR" rev-parse --show-toplevel 2>/dev/null) || PROJECT_DIR="${CLAUDE_PROJECT_DIR:-.}" diff --git a/tests/unit/hook-guard-git-commit-cwd-fallback.test.ts b/tests/unit/hook-guard-git-commit-cwd-fallback.test.ts new file mode 100644 index 000000000..a5e4aefe2 --- /dev/null +++ b/tests/unit/hook-guard-git-commit-cwd-fallback.test.ts @@ -0,0 +1,134 @@ +/** + * Regression guard for issue #2526: `.claude/hooks/guard-git.sh`'s + * commit edit-log check has the same unresolved-cwd gap #2386 fixed for + * branch validation. A bare `git commit -m "msg"` relying on the + * Bash tool's persistent cwd from an EARLIER, separate tool call leaves + * `WORK_DIR` empty, and the check fell back to the hook process's own + * ambient cwd instead of the payload's reported `cwd` — resolving the + * edit-log check against a completely unrelated repo. + * + * Unlike #2386's branch-validation bug, this doesn't produce a false + * *deny*: the wrong repo's `git diff --cached --name-only` comes back + * empty (nothing staged there), hitting the existing + * `if [ -z "$STAGED_FILES" ]; then exit 0` early-out and silently + * *allowing* — so the symptom is "the edit-log check gets silently + * skipped" rather than "a valid commit gets wrongly blocked." + * + * The fix: apply the same `$HOOK_CWD` fallback #2386 introduced for + * `validate_branch_name` to this section too. + */ +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = path.resolve(__dirname, '..', '..'); +const HOOK_PATH = path.join(REPO_ROOT, '.claude', 'hooks', 'guard-git.sh'); + +interface HookDecision { + hookSpecificOutput?: { + permissionDecision?: string; + permissionDecisionReason?: string; + }; +} + +function runHook(command: string, opts: { cwd?: string; hookCwd?: string } = {}): HookDecision { + const payload: Record = { tool_input: { command } }; + if (opts.hookCwd) payload.cwd = opts.hookCwd; + const stdout = execFileSync('bash', [HOOK_PATH], { + input: JSON.stringify(payload), + encoding: 'utf8', + cwd: opts.cwd, + }); + return stdout.trim() ? JSON.parse(stdout) : {}; +} + +function initRepo(dir: string): void { + fs.mkdirSync(dir, { recursive: true }); + execFileSync('git', ['init', '-q', '-b', 'main'], { cwd: dir }); + execFileSync('git', ['config', 'user.email', 'test@example.com'], { cwd: dir }); + execFileSync('git', ['config', 'user.name', 'Test'], { cwd: dir }); + fs.writeFileSync(path.join(dir, 'README.md'), 'test\n'); + execFileSync('git', ['add', 'README.md'], { cwd: dir }); + execFileSync('git', ['commit', '-q', '-m', 'init'], { cwd: dir }); +} + +describe('guard-git.sh commit edit-log check resolves the actual commit target (#2526)', () => { + let base: string; + let sessionRepo: string; + let otherRepo: string; + + beforeEach(() => { + base = fs.mkdtempSync(path.join(os.tmpdir(), 'guard-git-commit-')); + sessionRepo = path.join(base, 'session-repo'); + otherRepo = path.join(base, 'other-repo'); + // The hook process's own ambient cwd — has nothing staged, so if the + // check wrongly resolves here it silently allows via the "no staged + // files" early-out instead of ever inspecting the real target repo. + initRepo(sessionRepo); + // The subagent's actual commit target, reported via the payload's cwd. + initRepo(otherRepo); + }); + + afterEach(() => { + fs.rmSync(base, { recursive: true, force: true }); + }); + + it('uses the hook payload cwd for a bare git commit with no -C/cd, catching an unedited staged file', () => { + fs.mkdirSync(path.join(otherRepo, '.claude'), { recursive: true }); + fs.writeFileSync(path.join(otherRepo, '.claude', 'session-edits.log'), 'edit tracked.txt\n'); + fs.writeFileSync(path.join(otherRepo, 'sneaky.txt'), 'not edited by this session\n'); + execFileSync('git', ['add', 'sneaky.txt'], { cwd: otherRepo }); + + const result = runHook('git commit -m "msg"', { + cwd: sessionRepo, // ambient cwd — the WRONG repo, nothing staged there + hookCwd: otherRepo, // payload cwd — the RIGHT repo, has the unauthorized file + }); + + expect(result.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(result.hookSpecificOutput?.permissionDecisionReason).toContain('sneaky.txt'); + }); + + it('still allows via the payload cwd when every staged file was actually edited', () => { + fs.mkdirSync(path.join(otherRepo, '.claude'), { recursive: true }); + fs.writeFileSync(path.join(otherRepo, '.claude', 'session-edits.log'), 'edit tracked.txt\n'); + fs.writeFileSync(path.join(otherRepo, 'tracked.txt'), 'edited by this session\n'); + execFileSync('git', ['add', 'tracked.txt'], { cwd: otherRepo }); + + const result = runHook('git commit -m "msg"', { + cwd: sessionRepo, + hookCwd: otherRepo, + }); + + expect(result.hookSpecificOutput?.permissionDecision).not.toBe('deny'); + }); + + it('still prefers an explicit git -C over the payload cwd', () => { + fs.mkdirSync(path.join(otherRepo, '.claude'), { recursive: true }); + fs.writeFileSync(path.join(otherRepo, '.claude', 'session-edits.log'), 'edit tracked.txt\n'); + fs.writeFileSync(path.join(otherRepo, 'sneaky.txt'), 'not edited\n'); + execFileSync('git', ['add', 'sneaky.txt'], { cwd: otherRepo }); + + const result = runHook(`git -C "${otherRepo}" commit -m "msg"`, { + cwd: sessionRepo, + hookCwd: sessionRepo, // payload cwd also wrong — explicit -C must win regardless + }); + + expect(result.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(result.hookSpecificOutput?.permissionDecisionReason).toContain('sneaky.txt'); + }); + + it('falls back to silently allowing (not denying) when no payload cwd is available either', () => { + // Documents the pre-existing, milder fallback behavior this fix doesn't + // change: with no -C/cd AND no payload cwd, the check has no repo to + // resolve against at all and must not guess — it allows, same as + // validate_branch_name's equivalent last-resort case. + const result = runHook('git commit -m "msg"', { + cwd: sessionRepo, + }); + expect(result.hookSpecificOutput?.permissionDecision).not.toBe('deny'); + }); +});