Guard against agent git operations that require a branch-rule bypass - #449
Conversation
The write-guard permitted a direct commit to a protected branch when the authenticated user can bypass the rule, so a plain-looking update silently lands on develop or main under admin bypass. Add Rule 4: inspect the target branch's live rules and deny an operation that would only succeed by bypassing one - a direct update where a pull request is required, a force where history is protected, a delete where deletion is blocked - plus the explicit-bypass flags (gh pr merge --admin, --no-verify). Code-style and config-style develop are told apart by the live rules, not a hardcoded list; the protected-default branches fail closed when the rules cannot be read. Each denial names the bypassed rule and hands the command to the maintainer. 44/44 self-test cases pass; verified end-to-end against live branch rules. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the host-setup/agent-safety/gh-write-guard.py PreToolUse hook with a new Rule 4 that blocks git operations which would only succeed by bypassing active branch rules (plus explicit bypass flags like gh pr merge --admin and --no-verify).
Changes:
- Adds live branch-rule lookups (
gh api repos/{owner}/{repo}/rules/branches/{branch}) to deny direct pushes, force pushes, and branch deletions when the branch rules prohibit them. - Adds explicit bypass-flag detection for
gh pr merge --adminandgit commit/push --no-verify(plusgit commit -n). - Expands
--selftestto cover the new Rule 4 cases deterministically via injectablerules_lookupandcurrent_branch.
Copilot review of #449 surfaced two parser gaps: - _push_targets tokenized the segment with split(), so a bare push with a redirect (... >push.log 2>&1) read the redirect as the remote and refspec and skipped the current-branch resolution, missing a protected target. Stop parsing at the first redirection token (> or <). - _GH_ADMIN_MERGE excluded newlines, so a backslash-newline continued admin merge slipped past. Fold backslash-newline continuations to spaces in classify before the bypass checks run. Two self-test cases added for each gap; 46/46 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot review of #449: the push-bypass gate scanned the raw command, so a `git push ...` appearing only inside a quoted --body/--message (for example a gh issue comment that documents a command) tripped the check and could falsely deny. Run the gate and the target parse on the command with quoted spans removed, matching how the suppression and bypass-flag scans already treat quoted text as non-executable. Self-test case added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Third Copilot round on #449: - _push_targets hand-parsed quotes: a quoted refspec ('HEAD:develop') kept a trailing quote and a fully-quoted refspec was lost when spans were removed upstream. Tokenize with shlex and key off a real git-push argv adjacency, so a quoted refspec is unquoted cleanly and a push named only inside a quoted body forms no adjacency (no target). Removes the span-removal workaround. - _live_branch_rules built the API path from the raw branch name, so a name with a slash (feature/x) split the path and the lookup failed to None. URL encode the branch. - _handoff hardcoded a personal name; use the generic maintainer wording. Self-test now 49 cases (quoted refspec, quoted-mention-before-real-push); verified live: slashed-branch lookup returns a set, quoted refspec denies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
host-setup/agent-safety/gh-write-guard.py:186
_push_targets()only stops parsing at redirection tokens (>/<). Shell control operators like&&,||,;,|, and&can also appear aftergit pushand are not part of git argv. Todaygit push && echo okis mis-parsed as having a remote of&&, which can prevent the bare-push branch resolution and potentially let a protected-branch push slip past Rule 4.
while i < len(toks):
t = toks[i]
if ">" in t or "<" in t:
break # a redirection operator (>, 2>, >>, <, 2>&1): end of git argv, start of shell syntax
if t in ("--force", "-f") or t.startswith("--force-with-lease"):
host-setup/agent-safety/gh-write-guard.py:303
- Rule 4's gate
if _GIT_PUSH.search(cmd):only matchesgit pushwith no global git options. A real push likegit -C /path push origin develop(orgit -c key=val push ...) won't run_check_push_bypass()at all, which defeats the branch-rule bypass protection for a common invocation style.
# `_push_targets` tokenizes with shlex and keys off a real `git push` argv adjacency, so a push named
# only inside a quoted argument yields no target - the raw substring is just a cheap pre-filter.
if _GIT_PUSH.search(cmd):
dec, reason = _check_push_bypass(cmd, cwd, origin, current_branch, rules_lookup)
if dec == "deny":
Copilot review of #449: the push detection required a bare git-push token adjacency, so a global option between them (git -C <dir> push, git -c k=v push, git --git-dir=... push) dodged Rule 4 entirely and a direct push to a PR-gated branch could slip through. Add _git_push_args(), which skips git's value-taking global options before the subcommand, and route all three sites through it (the push parser, the write classifier, and the pre-filter). The loose pre-filter regex now allows the intervening options. Self-test now 54 cases (-C, -c k=v, --git-dir= forms deny; feature allowed); verified live: git -C ... push origin develop denies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot review of #449: a push with only a remote positional was always treated as a bare push resolving the current branch. That misclassified whole-repo pushes: --all and --mirror update every branch (protected ones included, so a current-branch-only check misses the bypass), while --tags pushes no branch (so resolving one is a false deny). Detect the flags: --all and --mirror target the protected-default branches (--mirror as a force, matching its prune/rewrite), --tags yields no branch target. Non-existent defaults return no rules and are skipped. Self-test now 58 cases. (--all/--mirror scan the default branch names, not arbitrary custom-protected refs - a documented precision-over-recall bound.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
host-setup/agent-safety/gh-write-guard.py:344
- Rule-4's
git pushpre-filter runs on the raw command string, so agit pushmentioned inside a quoted body/title can still trigger_check_push_bypass(), which in turn can run unnecessarygit remote get-url origin/ branch-resolution subprocesses. This is avoidable and was explicitly called out as a goal in earlier iterations: use the quoted-span-stripped string only to decide whether an executable push exists, but keep passing the originalcmdinto_check_push_bypass()for accurate parsing.
# `_push_targets` tokenizes with shlex and keys off a real `git push` argv adjacency, so a push named
# only inside a quoted argument yields no target - the raw substring is just a cheap pre-filter.
if _GIT_PUSH.search(cmd):
dec, reason = _check_push_bypass(cmd, cwd, origin, current_branch, rules_lookup)
if dec == "deny":
Copilot review of #449: the push parser stopped at any token merely containing > or <, so a > inside a quoted option value (--push-option='a>b') ended parsing before the refspec and fell back to current-branch resolution, a potential bypass. Deeper, shlex.split does not isolate operators glued to a token (develop;cmd) and only the first push in a compound was parsed. Tokenize with shlex punctuation_chars so real shell operators are their own tokens while a quoted > stays part of its word, and parse every git push in the command (each argv runs up to the next operator), so develop;cmd, a pipe, and push A && push B are all handled. _push_targets now returns every (op, branch) pair. Self-test 62 cases; verified live: quoted-value and compound pushes deny. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot review of #449: the push scan matched only a token exactly equal to "git", and the commit -n short-form detection matched only a bare "git commit", so an absolute-path or .exe invocation (/usr/bin/git push origin develop, git.exe push, /usr/bin/git commit -n) bypassed Rule 4 entirely. Add _is_git_exe(), which matches any token whose basename is git or git.exe, and broaden _GIT_COMMIT to allow a path prefix and .exe suffix. Self-test 65 cases (absolute-path push and commit -n, git.exe push). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot review of #449: --follow-tags was grouped with --tags as tags-only, so git push --follow-tags <remote> looked like it updated no branch and skipped the checks - but --follow-tags pushes the current branch plus reachable tags. Only --tags is tags-only; --follow-tags falls through to normal bare-push branch resolution. Self-test 66 cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot review of #449: - --no-verify was denied on any command, so a non-git tool using the same flag name (npm publish --no-verify) false-denied. Gate it to a git commit or git push context. - A comment still named _git_push_args, renamed to _push_arg_lists. Self-test 67 cases (non-git --no-verify allowed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
host-setup/agent-safety/gh-write-guard.py:312
_check_bypass_flags()only recognizesgit commitvia the_GIT_COMMITregex, which does not allow git global options betweengitandcommit(for examplegit -C /repo commit -n ...orgit -c k=v commit --no-verify ...). Those forms are valid and would bypass the intended--no-verify/-ndenial.
# --no-verify / commit -n skip the git hooks, so they only matter for a git commit or push - other
# tools use --no-verify for unrelated things, and denying those would be a false positive.
is_commit = _GIT_COMMIT.search(bare) is not None
is_push = bool(_push_arg_lists(bare))
long_no_verify = (is_commit or is_push) and _NO_VERIFY_LONG.search(bare)
short_n_commit = is_commit and _COMMIT_SHORT_N.search(bare) # `-n` is --no-verify for commit (push -n is dry-run)
Copilot review of #449: the --no-verify/-n check used a regex that matched only a bare "git commit", so git -C <dir> commit --no-verify (a global option before the subcommand) bypassed it - the same gap already fixed for push. Generalize the token scanner to _git_subcommand_arglists(cmd, sub) and detect the bypass flag as an actual arg of the commit/push argv, which also drops the three now-unused regexes (_GIT_COMMIT/_NO_VERIFY_LONG/_COMMIT_SHORT_N). Self-test 69 cases (git -C/-c commit -n/--no-verify deny). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
host-setup/agent-safety/gh-write-guard.py:311
_check_bypass_flags()treats--no-verify/-nas a bypass whenever the token appears anywhere in thegit commit/git pushargv. That can false-deny valid commands when the token is actually a value to a value-taking flag (for examplegit commit -m --no-verifywhere the commit message is "--no-verify", orgit push --push-option --no-verify ...where the push-option value happens to be "--no-verify"). Since the hook is explicitly "precision over recall", the bypass detection should skip values consumed by known value flags before deciding a bypass flag is present.
commit_lists = _git_subcommand_arglists(cmd, "commit")
push_lists = _push_arg_lists(cmd)
commit_bypass = any(("--no-verify" in a) or ("-n" in a) for a in commit_lists)
push_bypass = any("--no-verify" in a for a in push_lists)
if commit_bypass or push_bypass:
Copilot review of #449: _check_push_bypass resolved origin (a git subprocess) before checking whether any executable push was found, so a command that only mentions git push in a quoted argument still did git work. Compute the targets first and return early when there are none. Also, the fail-closed message assumed API unreachability, but the cause can be missing repo context (not a git checkout) - distinguish the two reasons. Self-test 69 cases, unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot review of #449: the argv collector stopped at any shell-operator token, so a redirection placed before the refspec (git push 2>push.log origin develop, which POSIX allows) had its fd digit read as a positional and parsing stopped at >, dropping the real origin develop and falling back to current- branch resolution - a bypass. Distinguish redirections (>, >>, <, >&, and a leading fd digit) which are skipped so args continue, from command separators (|, &&, ;) which end the invocation. Also catch TypeError in _shell_tokens so the tokenizer degrades on a Python without punctuation_chars instead of crashing (which would skip Rule 4 entirely). Self-test 71 cases; verified live: leading-redirection push to develop denies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
host-setup/agent-safety/gh-write-guard.py:334
_check_bypass_flags()treats any--no-verifytoken in agit commitargv as a bypass flag, butgit commit -m --no-verifyis valid and sets the commit message to--no-verify(i.e.,--no-verifyis an option value, not a flag). This can false-deny legitimate commits and contradicts the stated "precision over recall" goal.
# --no-verify / commit -n skip the git hooks, so they only matter as an actual arg to a git commit or
# push (other tools use --no-verify for unrelated things; shlex keeps a quoted mention out of the argv).
# `-n` is --no-verify only for commit; `git push -n` is --dry-run.
commit_lists = _git_subcommand_arglists(cmd, "commit")
push_lists = _push_arg_lists(cmd)
commit_bypass = any(("--no-verify" in a) or ("-n" in a) for a in commit_lists)
push_bypass = any("--no-verify" in a for a in push_lists)
if commit_bypass or push_bypass:
return "deny", (
"This uses --no-verify, which skips the git hooks (signing, lint, and pre-push gates). "
"Skipping verification is a bypass; run the command without it." + _handoff(cmd)
)
…450) Adds one paragraph to the GitHub Copilot Review Runbook's Bounded Retry Workflow. ## Why While driving #449 through 13 review rounds in an hour, Copilot throttled and posted its final re-review ~36 minutes after the request - beyond a 15-minute poll window. The poll timed out, and the Bounded Retry Workflow treated that as a *genuinely missing* review and escalated. It was not missing, only pending. ## Change Clarify that a slow review is **pending, not missing**: a poll timeout is evidence only that the review has not landed yet, so report `review still pending`, poll on a widening interval, and enter escalation only when the `requestReviews` mutation no-ops/errors or after a genuinely long confirmed-accepted wait - never on one fixed poll window elapsing. This complements the #444 head-coverage gate (which prevents concluding *clean* too early) by preventing the opposite error - concluding *unresponsive/blocked* too early. Docs only - no release. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…449, #450) (#451) Promotes two develop PRs to main: - **#449** - Rule 4 in `gh-write-guard.py`: deny any agent git operation that would only land by bypassing an active branch rule (direct push to a PR-gated branch, force where history is protected, delete where deletion is blocked, and the explicit-bypass flags `gh pr merge --admin` / `git commit|push --no-verify`). Judged against the branch's live rules, so a code-style develop denies and a config-style develop allows with no hardcoded list. 71 self-test cases; 18 Copilot findings resolved across the review. - **#450** - Copilot Review Runbook: a slow/throttled Copilot review is *pending, not missing* - poll with backoff and report "still pending" rather than escalating on a timeout. Carried-file/spec changes, so downstream repos re-vendor `.github/copilot-instructions.md`; the guard lives under `host-setup/` (hub tooling). Docs/tooling only - no release. Deploy of the guard to `~/.claude/hooks/` is held until this promotion passes review; the idempotent host installer + per-machine refresh tracking (#365) follow separately. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Extends the
gh-write-guardPreToolUse hook with Rule 4: deny any git operation that would only succeed by bypassing an active branch rule - the gap behind the direct-push-to-develop incident this session, where an admin-bypass identity makes a plaingit push origin developsilently land on a PR-gated branch.What it denies
main; code-styledevelop). A config-styledevelophas no such rule and stays allowed - decided by the live rules, no hardcoded repo list.non_fast_forward/required_linear_historyis active.deletionis active.gh pr merge --admin,git commit/push --no-verify.Behavior
repos/{owner}/{repo}/rules/branches/{branch}(visible even to a bypass actor).main/master/developwhen the rules cannot be read; fail-open elsewhere (feature branches).rules_lookup/current_branchkeep--selftestdeterministic and offline.Verification
--selftest: 44/44 (existing 21 write-footgun cases + 22 new Rule-4 cases: code/config develop, main, feature, HEAD/sha/bare/+refspec/delete refspecs, fail-closed vs fail-open, the explicit flags).develop/maindeny, this feature branch allow,--admin/--no-verifydeny.Docs/tooling only - no release. Deploy to
~/.claude/hooks/is held until this and the develop->main promotion both pass review.🤖 Generated with Claude Code