Skip to content

Fix guard_master.py: judge branch from a cd-chain, not session cwd - #320

Merged
WilfordGrimley merged 3 commits into
masterfrom
sark/guard-master-effective-dir
Jul 22, 2026
Merged

Fix guard_master.py: judge branch from a cd-chain, not session cwd#320
WilfordGrimley merged 3 commits into
masterfrom
sark/guard-master-effective-dir

Conversation

@WilfordGrimley

@WilfordGrimley WilfordGrimley commented Jul 22, 2026

Copy link
Copy Markdown

Summary

Tonight's incident: guard_master.py's git merge/git push worktree
rules judge branch state via current_branch(cwd), where cwd is the
Claude Code session's registered working directory -- not wherever the
command itself redirects git to via a leading cd <path> && chain. A
command like:

cd /path/to/feature-worktree && git merge origin/master

run from a session whose registered cwd happens to be a master checkout
got judged as if it were running on master, and was wrongly denied even
though it was legitimate feature-branch conflict resolution, not an
unauthorized merge into master.

Update (same-day follow-up, still 2026-07-22): the first cut of this
PR also added git -C <path> support, resolved via an unanchored scan
over the whole command. The orchestrator flagged that as a false-ALLOW
risk before merge, I traced and confirmed it, and this PR now ships the
tightened version: git -C support is dropped entirely. See "What
changed since the first commit" below for the exact delta.

Fix

Adds effective_dir(command, session_cwd, target_re):

  • splits the command into simple-command segments on ;, &, |,
    &&, and || (longer operators matched first so &&/|| aren't
    mistaken for two single-character separators).
  • locates the segment matching target_re (the same "git merge"/"git
    push" the calling rule already detected).
  • walks backward from that segment collecting only the segments joined
    to it by an unbroken chain of && -- a cd behind a ;, single
    &, |, or || boundary is a different shell statement and is
    deliberately not followed. This mirrors real shell semantics: &&
    only runs the next step if the previous one succeeded, so a cd
    genuinely chained by && is guaranteed to have actually taken effect
    before the merge/push ran; a cd behind any other separator carries
    no such guarantee.
  • processes that chain left to right, applying each whole-segment cd <path> in turn (quoted or bare).
  • falls back to session_cwd (today's pre-fix behavior) whenever
    there's no leading cd chain, or any cd in it targets a path that
    isn't a real directory -- fail-closed: a resolution failure is never
    more permissive than session-cwd-only judging.
  • no git -C <path> support (see below).

Used in both current_branch(...) == "master" checks (the git merge
rule and the git push worktree rule), each passing its own leading
pattern (MERGE_LEAD_RE / PUSH_LEAD_RE) so effective_dir() locates
the right segment for whichever rule is asking. Explicitly unchanged:
the unconditional gh pr merge denial, the --ff-only exemption, all
deny messages, log_stub's behavior, and the push rule's
in_worker_worktree gate (still computed from raw session cwd -- it
identifies which session is running a worker worktree, not which
directory a given command targets; routing it through effective_dir
too would have broken the existing "bare git push while on master,
worker -> DENY" case).

What changed since the first commit (closing the git -C false-ALLOW gap)

The first cut resolved a bare git -C <path> anywhere in the command
via an unanchored re.search. Traced and confirmed: an earlier,
unrelated git -C /some/repo status && git merge origin/master got its
unrelated -C path substituted in for the real merge's own context --
if that unrelated repo wasn't on master, a genuine merge-into-master
from the session's actual branch would read as non-master and get
wrongly ALLOWED. That's the more dangerous failure direction (a
false ALLOW, not a false DENY), so per the orchestrator's explicit
tightening request, git -C support is now dropped entirely rather than
anchored to the specific triggering invocation (anchoring was
considered; dropping it is the acceptable, smaller-blast-radius
simplification since leading-cd-chains are the pattern that actually
occurred in production). This reopens a narrower, already-existing,
safe gap: a bare git -C <path> merge/push ... command still isn't
recognized as a merge/push attempt at all (the calling rules' own
detection regexes require "git" and "merge"/"push" adjacent with no
intervening flag -- unchanged, pre-existing). Under-triggering only ever
produces an unnecessary DENY, never a wrong ALLOW, so it's accepted as
open item #1 below rather than fixed.

Minimal change, no broader process modification, per the approved shape.

Tests

.claude/hooks/test_guard_master.py already existed (added in #136,
subprocess-driven black-box harness piping synthetic PreToolUse JSON at
the hook and asserting exit codes) -- extended it in place rather than
rewriting it as a fresh pytest suite (see Deviations below). Coverage:

  • end-to-end cases through the real git merge/git push rules for the
    cd <path> && chain form: allowed-when-resolves-off-master,
    denied-unchanged-baseline, fail-closed-on-nonexistent-path, for both
    merge and push, plus confirming gh pr merge and the --ff-only
    exemption are untouched.
  • the two orchestrator-requested regression cases:
    (a) git -C /unrelated status && git merge x, session cwd on master
    -> DENIED (the false-ALLOW gap is closed).
    (b) cd /worktree && git fetch && git merge origin/master -> ALLOWED
    (a cd earlier in the same && chain still carries through an
    intervening non-cd step).
  • direct unit cases against effective_dir() itself (via a small
    load_hook_module() helper): multi-step && chains, quoted paths,
    chain-boundary behavior (;/single-&/|/|| correctly NOT
    carrying a cd across), and confirming git -C is no longer resolved
    at all.

All pass locally (python3 .claude/hooks/test_guard_master.py -- 35
cases, 0 failures), plus ruff check, black --check, and isort --check --profile black clean on both changed files. All PR CI checks
green (gh pr checks), mergeable: true.

Deviations from spec

  1. Test file already existed (.claude/hooks/test_guard_master.py,
    Phase 1/2 orchestration groundwork: guard hook, worker roster, corrections ledger, docs manifest #136) using a subprocess-driven black-box harness, not pytest, and not
    calling main() in-process. The task asked for "plain pytest... driving
    main()". Given this repo's explicit "minimal change, no broader process
    modification" constraint, and that the existing harness already
    satisfies the substance of the ask (pipes synthetic stdin payloads,
    asserts exit codes) with a proven, currently-green suite, I extended it
    in place rather than rewriting it into pytest. Happy to convert on
    request.

  2. No CI wiring. Confirmed no workflow under .github/workflows/
    currently runs .claude/hooks/test_guard_master.py -- it has never
    been part of CI. Per the "minimal change" instruction I did not add
    wiring; flagging it here since it means this suite is currently run
    manually only.

Open items / needs an owner decision

  1. git -C reachability gap (under-triggering, safe direction,
    pre-existing and unchanged by this PR).
    The git merge/git push
    rules' own outer detection regexes (git\s+merge, git\s+push)
    require "git" and "merge"/"push" adjacent with no intervening flag. A
    bare git -C <path> merge ... command therefore isn't recognized as
    a merge/push attempt at all today. Only reachable via a compound
    command where the outer regex matches a separate git merge/git push elsewhere in the string (in which case effective_dir() now
    correctly ignores the unrelated -C, per the fix above). Not fixed
    here -- widening those regexes to also catch git -C ... merge/push is a separate, broader change than this fix's remit.

  2. Whether to wire .claude/hooks/test_guard_master.py into any CI
    workflow (see Deviations Promote PringlePrints to a full ordering tab #2) -- currently manual-only.

Docs

docs/troubleshooting.md's entry for this incident
(guard_master.py denies a git merge/git push you ran from inside a feature worktree) is updated in place (not appended) to describe the
final, tightened fix and the same-day git -C follow-up, per the repo's
docs convention.

…ands

Add effective_dir() so git-merge/git-push worktree rules resolve branch
state from where a `cd <path> &&` or `git -C <path>` command actually
runs, not the session's registered cwd -- fixes tonight's false deny of
legitimate feature-branch conflict resolution.
…ffective-dir

# Conflicts:
#	docs/troubleshooting.md
Drop freestanding `git -C <path>` support from effective_dir() -- it was
an unanchored scan an unrelated earlier `-C` in a compound command could
hijack, wrongly ALLOWing a real merge-into-master. Now only follows a
`cd <path>` chain connected to the merge/push by an unbroken `&&` (never
across ';', single '&', '|', or '||'), matching real shell semantics.
@WilfordGrimley WilfordGrimley changed the title Fix guard_master.py: judge branch from where cd/-C actually points, not session cwd Fix guard_master.py: judge branch from a cd-chain, not session cwd Jul 22, 2026
@WilfordGrimley
WilfordGrimley merged commit 90f6366 into master Jul 22, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant