From e4f8962356a3dc4cc29d838fa57efb1ed7aaf095 Mon Sep 17 00:00:00 2001 From: exiao Date: Mon, 29 Jun 2026 04:17:01 -0400 Subject: [PATCH 01/12] fix(approval): narrow force-push gate to match shell guard (auto-allow lease push to feature branch) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runtime approval gate flagged EVERY force push for an operator yes/no, regardless of target branch, so a routine force-with-lease PR rebase sat in pending_approval and never ran headless — while the shell guard (~/.local/bin/git) already allows exactly that case. Workers can't self-grant, so they block and a human approves a push the shell layer considers safe (observed: t_c5e6855d, t_d35127e9 on PR #806's wt/t_89fe8606; PR #141 babysit). Add a branch-aware carve-out to detect_dangerous_command: new helper _is_safe_lease_push_to_feature_branch auto-allows a git push that carries --force-with-lease, has no bare --force/-f, and does NOT target the default branch. The main/master refspec forms it rejects (+main, HEAD:main, *:main, bare main/master) mirror the shell guard's enumeration so the two layers can't disagree. When it matches, the loop skips ONLY the three force-push pattern descriptions; the main/master push backstop and every other pattern still fire. The lease is the safety belt (refuses if the remote moved), so a shared-worktree second pusher bounces instead of clobbering; a bare force has no lease and keeps prompting. Verified (real invocations): (a) lease->feature branch auto-approves headless (no prompt) via check_all_command_guards in a gateway context; (b) bare force still prompts; (c) lease->main still prompts/blocks; (d) a stale lease real git push is rejected (stale info), remote not clobbered. Differential probe shows the carve-out un-gates ONLY the lease-to-feature shapes, all other verdicts unchanged. Updated test_git_push_force_with_lease_* + added two regression tests; 262 approval tests pass, ruff clean. Patch note: ~/.hermes/plans/hermes-patches/force-push-lease-feature-branch-carveout.md --- tests/tools/test_approval.py | 37 ++++++++++++++-- tools/approval.py | 83 ++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 3 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index 4ef92e7d10128..50a4543d8c38b 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1361,11 +1361,42 @@ def test_git_push_to_main_flagged(self): assert dangerous is True, f"expected block, got allow for: {cmd}" assert "main/master" in (desc or ""), f"wrong reason for: {cmd} -> {desc}" - def test_git_push_force_with_lease_flagged(self): - """--force-with-lease still rewrites history and MUST be flagged.""" + def test_git_push_force_with_lease_to_feature_branch_not_flagged(self): + """A force-WITH-LEASE push to a NON-default branch is the routine + PR-rebase case the shell git-guard (~/.local/bin/git) already allows. + The lease is the safety belt (it refuses if the remote moved), so this + auto-approves headless instead of stalling for an operator prompt. + """ cmd = "git push --force-with-lease origin feature" dangerous, _, _ = detect_dangerous_command(cmd) - assert dangerous is True + assert dangerous is False + + def test_git_push_force_with_lease_to_main_still_flagged(self): + """A force-with-lease whose refspec targets the DEFAULT branch could + rewrite main/master and MUST still be flagged (mirrors the shell guard's + main/master refspec enumeration).""" + for cmd in ( + "git push --force-with-lease origin main", + "git push --force-with-lease origin master", + "git push --force-with-lease origin HEAD:main", + "git push --force-with-lease origin feature:main", + "git push --force-with-lease origin +main", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + + def test_git_push_bare_force_still_flagged_even_on_feature_branch(self): + """A BARE force (no lease) has no safety belt, so it keeps prompting + regardless of target branch — only the lease form is carved out.""" + for cmd in ( + "git push --force origin feature", + "git push -f origin feature", + "git push --force", + # bare force AND lease together: the bare force could still clobber + "git push --force --force-with-lease origin feature", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" def test_gh_pr_merge_flagged(self): """`gh pr merge` bypasses PR review and MUST be flagged.""" diff --git a/tools/approval.py b/tools/approval.py index 5f4d9b7fa3d64..4c2a304d7579b 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -820,6 +820,82 @@ def _scrub_quoted(m: re.Match) -> str: return view +# ========================================================================= +# Force-push carve-out (mirror the shell git-guard at ~/.local/bin/git) +# ========================================================================= +# The shell PATH wrapper (~/.local/bin/git) already encodes the safe rule: +# force-pushing a FEATURE branch (rebasing your own PR) is allowed; it only +# blocks a force-push that could rewrite the default branch (main/master) — +# either via a main/master-targeting refspec or a bare force while standing on +# a main/master checkout. A force-WITH-LEASE to a feature branch sails through. +# +# The three force-push DANGEROUS_PATTERNS entries below do not make that +# distinction — they flag EVERY force push for an operator prompt, which stalls +# routine PR-rebase work headless. This carve-out narrows them to match the +# shell guard: a force-WITH-LEASE push whose target ref is NOT the default +# branch is auto-allowed; everything else (a bare force, any force whose refspec +# targets main/master) still prompts. The lease is the safety belt — it refuses +# if the remote moved since the last fetch, so even on a shared worktree branch +# the second pusher's lease bounces instead of clobbering. A bare force is NOT +# covered (no lease = no safety belt), so it keeps prompting. +# +# The main/master refspec forms here are the SAME ones the shell guard +# enumerates (lines ~49-62 of ~/.local/bin/git) so the two layers cannot +# disagree. The main/master push patterns in DANGEROUS_PATTERNS stay active as +# an independent backstop. +_FORCE_PUSH_DESCRIPTIONS = frozenset({ + "git force push (rewrites remote history)", + "git force push short flag (rewrites remote history)", + "git force-with-lease push (rewrites remote history)", +}) + +# A bare --force flag (NOT the --force-with-lease form). `--force-with-lease` +# contains the substring `--force`, so require a word boundary that the lease +# suffix does not satisfy: --force/--force=… followed by end or a non-hyphen. +_BARE_FORCE_FLAG_RE = re.compile(r'--force(?![\w-])') +# Short force flag: -f, or packed combos like -uf / -fv (a flag token that is +# all letters and contains an `f`). Excludes long flags (already handled above). +_SHORT_FORCE_FLAG_RE = re.compile(r'(? bool: + """True iff *command_lower* is a force-WITH-LEASE git push to a NON-default + branch — the exact case the shell git-guard auto-allows. + + *command_lower* is the already-normalized + lowercased command (same view + the dangerous-pattern loop scans). Returns False (→ keep prompting) for: + - anything that is not a `git push`, + - a push with no `--force-with-lease` (a bare `--force`/`-f` has no lease + safety belt, so it stays gated), + - a push that ALSO carries a bare `--force`/`-f` (the bare force could + still clobber regardless of the lease intent), + - a push whose refspec/target is the default branch (main/master). + """ + if not re.search(r'\bgit\s+push\b', command_lower): + return False + # Require the lease form — the safety belt that makes auto-approve safe. + if not _FORCE_WITH_LEASE_RE.search(command_lower): + return False + # Reject if a BARE force is also present (e.g. `--force --force-with-lease` + # or `-f`). Strip the lease token first so its `--force` substring and any + # `=` value don't read as a bare force. + without_lease = _FORCE_WITH_LEASE_RE.sub(' ', command_lower) + if _BARE_FORCE_FLAG_RE.search(without_lease) or _SHORT_FORCE_FLAG_RE.search(without_lease): + return False + # Reject if the refspec/target is the default branch — let the main/master + # patterns (and this guard's refusal) handle it via the normal prompt. + if _DEFAULT_BRANCH_PUSH_RE.search(command_lower): + return False + return True + + def detect_dangerous_command(command: str) -> tuple: """Check if a command matches any dangerous patterns. @@ -832,7 +908,14 @@ def detect_dangerous_command(command: str) -> tuple: # obfuscated/redirected self-kills are still caught while redirect targets # with a self keyword are not false positives. self_term_view = _build_self_term_scan_view(command_lower) + # Branch-aware force-push carve-out: a force-with-lease push to a non-default + # branch is safe (mirrors ~/.local/bin/git), so suppress the three blanket + # force-push patterns for it. All other patterns (incl. the main/master push + # backstop) still apply. + skip_force_push = _is_safe_lease_push_to_feature_branch(command_lower) for pattern_re, description in DANGEROUS_PATTERNS_COMPILED: + if skip_force_push and description in _FORCE_PUSH_DESCRIPTIONS: + continue target = self_term_view if description in _SELF_TERM_DESCRIPTIONS else command_lower if pattern_re.search(target): pattern_key = description From 747add7a9058548ddf0504340c8dcc9c6815559d Mon Sep 17 00:00:00 2001 From: exiao Date: Mon, 29 Jun 2026 04:39:19 -0400 Subject: [PATCH 02/12] fix(approval): gate refs/heads/main force-push forms + harden lease carve-out Reviewer (kanban t_8a8f70fe) FAILED #66: two main-rewriting shapes auto-approved headless because _DEFAULT_BRANCH_PUSH_RE and the DANGEROUS_PATTERNS main/master entries missed the fully-qualified ref forms. - _DEFAULT_BRANCH_PUSH_RE: anchor main/master on a refspec-boundary char (^|[/:+\s]) ... (\s|$|['"`]) so refs/heads/main, HEAD:refs/heads/main, refs/heads/topic:refs/heads/main, *:main and the quoted '*:main' are all gated, while names that merely contain the word (mainline/my-main/main-event) are not. - DANGEROUS_PATTERNS push-to-main backstop: same boundary anchoring so the qualified forms are caught even when the carve-out is not in play. - Harden _is_safe_lease_push_to_feature_branch (codex P1s, same bug class): reject --all/--mirror broadcasts, a push with NO explicit destination refspec (push.default may push current branch=main), and a leading-`+` refspec (forced update with no lease guarantee). Carve-out now auto-approves ONLY an explicit, single, non-`+`, non-default feature ref. - Tests: extend the main/master loops with the refs/heads forms (main+master), *:main quoted+unquoted, refs/heads RHS; add qualified-feature negatives and a new test for the +refspec / broadcast / no-refspec classes. Suite: 262 passed / 2 failed (pre-existing prompt_toolkit ModuleNotFoundError, unrelated). --- tests/tools/test_approval.py | 53 ++++++++++++++++++++++++++++++++++- tools/approval.py | 54 ++++++++++++++++++++++++++++++++---- 2 files changed, 100 insertions(+), 7 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index 50a4543d8c38b..b42f1c372ed0e 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1356,6 +1356,9 @@ def test_git_push_to_main_flagged(self): "git push origin HEAD:main", "git push origin main:main", "git push upstream master", + "git push origin refs/heads/main", + "git push origin refs/heads/master", + "git push origin HEAD:refs/heads/main", ): dangerous, _, desc = detect_dangerous_command(cmd) assert dangerous is True, f"expected block, got allow for: {cmd}" @@ -1374,13 +1377,25 @@ def test_git_push_force_with_lease_to_feature_branch_not_flagged(self): def test_git_push_force_with_lease_to_main_still_flagged(self): """A force-with-lease whose refspec targets the DEFAULT branch could rewrite main/master and MUST still be flagged (mirrors the shell guard's - main/master refspec enumeration).""" + main/master refspec enumeration). Covers the bare, `+`, `HEAD:`, and + colon-RHS forms AND the fully-qualified `refs/heads/` / + `HEAD:refs/heads/` forms for both main and master.""" for cmd in ( "git push --force-with-lease origin main", "git push --force-with-lease origin master", "git push --force-with-lease origin HEAD:main", + "git push --force-with-lease origin HEAD:master", "git push --force-with-lease origin feature:main", "git push --force-with-lease origin +main", + # Fully-qualified refspec forms (the carve-out hole this loop guards): + "git push --force-with-lease origin refs/heads/main", + "git push --force-with-lease origin refs/heads/master", + "git push --force-with-lease origin HEAD:refs/heads/main", + "git push --force-with-lease origin HEAD:refs/heads/master", + # Wildcard LHS clobbering main (minor finding — plausible clobber), + # both unquoted and shell-quoted (the quote must not defeat the gate): + "git push --force-with-lease origin *:main", + "git push --force-with-lease origin '*:main'", ): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is True, f"expected block, got allow for: {cmd}" @@ -1398,6 +1413,27 @@ def test_git_push_bare_force_still_flagged_even_on_feature_branch(self): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is True, f"expected block, got allow for: {cmd}" + def test_git_push_lease_carveout_only_for_explicit_single_feature_ref(self): + """The lease carve-out auto-approves ONLY an explicit, non-`+`, + single-destination feature-ref push. A leading-`+` refspec (a forced + update with no lease guarantee), a `--all`/`--mirror` broadcast (pushes + every local ref incl. main), and an omitted refspec (resolves via + push.default — may push the current branch, which could be main) must + all keep prompting.""" + for cmd in ( + # leading-`+` refspec = forced update, bypasses the lease belt + "git push --force-with-lease origin +feature", + "git push --force-with-lease origin +refs/heads/feature", + # broadcast pushes touch every ref incl. main/master + "git push --force-with-lease --all origin", + "git push --force-with-lease --mirror origin", + # no explicit destination refspec + "git push --force-with-lease origin", + "git push --force-with-lease", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + def test_gh_pr_merge_flagged(self): """`gh pr merge` bypasses PR review and MUST be flagged.""" for cmd in ("gh pr merge 42 --squash", "gh pr merge 42", "gh pr merge"): @@ -1436,6 +1472,19 @@ def test_gh_read_only_subcommands_not_flagged(self): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is False, f"expected allow, got block for: {cmd}" + def test_git_push_force_with_lease_to_qualified_feature_branch_not_flagged(self): + """A fully-qualified `refs/heads/` lease push must still + auto-allow — the boundary anchor only fires when the final ref + component is exactly main/master, not for a feature ref under + refs/heads/.""" + for cmd in ( + "git push --force-with-lease origin refs/heads/feature", + "git push --force-with-lease origin HEAD:refs/heads/my-feature", + "git push --force-with-lease origin refs/heads/mainline", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is False, f"expected allow, got block for: {cmd}" + def test_git_push_to_branches_containing_main_not_flagged(self): """Branch names that *contain* 'main' must not trigger the main/master rule.""" for cmd in ( @@ -1443,6 +1492,8 @@ def test_git_push_to_branches_containing_main_not_flagged(self): "git push origin my-main", "git push origin mainline", "git push origin master-key", + "git push origin refs/heads/mainline", + "git push origin refs/heads/my-main", ): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is False, f"expected allow, got block for: {cmd}" diff --git a/tools/approval.py b/tools/approval.py index 4c2a304d7579b..ba578b55b9cc0 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -543,8 +543,18 @@ def _sudo_stdin_block_result(description: str) -> dict: # sibling PATH wrappers at ~/.local/bin/{gh,git} and ~/.claude/guard.sh # and the ~/.hermes/plugins/block-dangerous-merges/ plugin for parallel # enforcement at the shell and Claude-Code tool layers. - (r'\bgit\s+push\b\s+\S+\s+(?:\S+:)?(\+?main|\+?master)(?:\s|$)', "git push to main/master (bypasses PR review)"), - (r'\bgit\s+push\b\s+\S+\s+HEAD:(main|master)(?:\s|$)', "git push HEAD to main/master (bypasses PR review)"), + # Match a push whose refspec targets main/master in any of the standard + # forms: bare `main`, `+main`, `HEAD:main`, `main:main`, `feature:main`, + # `*:main`, AND the fully-qualified `refs/heads/main` / `HEAD:refs/heads/main` + # (the `+`/colon force forms too). The `main`/`master` token is anchored on + # a REQUIRED preceding refspec-boundary char (the space after the remote, or + # `/`/`:`/`+` inside the refspec) so `refs/heads/main` and + # `HEAD:refs/heads/main` are caught, while a branch that merely contains the + # word (`mainline`, `my-main`, `main-event`) is NOT: a `-`-joined name has no + # boundary char immediately before `main`, and the trailing `(?:\s|$)` rejects + # a trailing suffix. Mirrors the shell guard at ~/.local/bin/git. + (r'\bgit\s+push\b\s+\S+\s+\S*[/:+\s](main|master)(?:\s|$|[\'"`])', "git push to main/master (bypasses PR review)"), + (r'\bgit\s+push\b\s+\S+\s+(main|master)(?:\s|$|[\'"`])', "git push to main/master (bypasses PR review)"), (r'\bgit\s+push\b.*--force-with-lease\b', "git force-with-lease push (rewrites remote history)"), (r'\bgh\s+pr\s+merge\b', "gh pr merge (bypasses PR review — Eric merges)"), (r'\bgh\s+(release|repo|workflow)\s+delete\b', "gh delete destructive resource"), @@ -859,9 +869,14 @@ def _scrub_quoted(m: re.Match) -> str: _FORCE_WITH_LEASE_RE = re.compile(r'--force-with-lease(?:=\S*)?(?![\w])') # Refspec/target forms that would rewrite the default branch — mirrors the # shell guard's main/master enumeration (`+main`, `HEAD:main`, `*:main`, -# `main:main`, a bare `main`/`master` push target, the `+`/colon force forms). +# `main:main`, a bare `main`/`master` push target, the `+`/colon force forms), +# AND the fully-qualified `refs/heads/main` / `HEAD:refs/heads/main` forms. +# Anchor `main`/`master` on a preceding refspec-boundary char (`/`, `:`, `+`, +# or whitespace / start) so a `heads/`-prefixed or colon-RHS-qualified ref is +# caught, while a branch that merely CONTAINS the word (`mainline`, `my-main`, +# `main-event`) is not — the trailing `(?:\s|$)` keeps those un-gated. _DEFAULT_BRANCH_PUSH_RE = re.compile( - r'(?:^|\s)(?:\+?main|\+?master|head:main|head:master|\S*:main|\S*:master)(?:\s|$)' + r'(?:^|[/:+\s])(?:main|master)(?:\s|$|[\'"`])' ) @@ -876,7 +891,14 @@ def _is_safe_lease_push_to_feature_branch(command_lower: str) -> bool: safety belt, so it stays gated), - a push that ALSO carries a bare `--force`/`-f` (the bare force could still clobber regardless of the lease intent), - - a push whose refspec/target is the default branch (main/master). + - a `--all`/`--mirror` push (pushes EVERY local ref, incl. main/master — + a single explicit destination ref cannot be reasoned about), + - a push with NO explicit destination refspec (an omitted refspec follows + push.default and may push the current branch — which could be main), + - a refspec carrying a leading `+` (git documents `+:` as the + same forced update as `--force`, with NO lease safety belt), + - a push whose refspec/target is the default branch (main/master), in any + form incl. `refs/heads/main` / `HEAD:refs/heads/main`. """ if not re.search(r'\bgit\s+push\b', command_lower): return False @@ -889,7 +911,27 @@ def _is_safe_lease_push_to_feature_branch(command_lower: str) -> bool: without_lease = _FORCE_WITH_LEASE_RE.sub(' ', command_lower) if _BARE_FORCE_FLAG_RE.search(without_lease) or _SHORT_FORCE_FLAG_RE.search(without_lease): return False - # Reject if the refspec/target is the default branch — let the main/master + # Reject a broadcast push: `--all`/`--mirror` push every local ref (incl. + # main/master), so a "no main token in the args" check cannot vouch for them. + if re.search(r'(? Date: Mon, 29 Jun 2026 13:02:51 -0400 Subject: [PATCH 03/12] fix(approval): exclude trailing-hyphen flags from lease-push detection `_FORCE_WITH_LEASE_RE` used `(?![\\w])` which let a different flag that merely starts with the lease string (e.g. `--force-with-lease-foo`) read as the lease form. Switch to `(?![\\w-])` so a trailing hyphen also fails the boundary, matching the convention the bare-force regexes already use; the `=` value form is unaffected. Adds a regex-boundary test. Addresses gemini-code-assist review (line 869). --- tests/tools/test_approval.py | 11 +++++++++++ tools/approval.py | 5 ++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index b42f1c372ed0e..eea39ea0c1ed4 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1434,6 +1434,17 @@ def test_git_push_lease_carveout_only_for_explicit_single_feature_ref(self): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is True, f"expected block, got allow for: {cmd}" + def test_force_with_lease_re_excludes_trailing_hyphen_flag(self): + """`_FORCE_WITH_LEASE_RE` must match the real lease flag (bare and the + `=` value form) but NOT a different flag that merely starts + with the same string, e.g. `--force-with-lease-foo`. The trailing-hyphen + exclusion (`(?![\\w-])`) is what draws that boundary.""" + rx = approval_module._FORCE_WITH_LEASE_RE + assert rx.search("--force-with-lease") + assert rx.search("--force-with-lease=origin/main") + assert rx.search("--force-with-lease=expected-sha") + assert not rx.search("--force-with-lease-foo") + def test_gh_pr_merge_flagged(self): """`gh pr merge` bypasses PR review and MUST be flagged.""" for cmd in ("gh pr merge 42 --squash", "gh pr merge 42", "gh pr merge"): diff --git a/tools/approval.py b/tools/approval.py index ba578b55b9cc0..e647d6c65803e 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -866,7 +866,10 @@ def _scrub_quoted(m: re.Match) -> str: # Short force flag: -f, or packed combos like -uf / -fv (a flag token that is # all letters and contains an `f`). Excludes long flags (already handled above). _SHORT_FORCE_FLAG_RE = re.compile(r'(? Date: Mon, 29 Jun 2026 13:17:38 -0400 Subject: [PATCH 04/12] fix(approval): harden lease-push carve-out (codex P1/P2 follow-up to #66) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #66. Codex left genuine hardening gaps in _is_safe_lease_push_to_feature_branch — every case below was reproduced auto-approving headless on the pr-66 head before this change: - live-config exclusion (P1): add _PROTECTED_BRANCHES = {main, master, live-config}. live-config is the long-lived integration branch the running gateway checks out (AGENTS.md), so a headless leased force-with-lease push to it would bypass the PR-to-self flow. Matched against the parsed destination branch in bare / refs/heads / HEAD: / src:dst forms. - branch-ref-only carve-out (P2 "Restrict to branch refs"): new _refspec_destination() parses each refspec's destination and returns None for anything that is not a single refs/heads/. refs/tags, refs/notes, refs/remotes and a tag RHS (feature:refs/tags/v1) now keep prompting — a leased tag rewrite no longer slips through. - reject ambiguous shorthands (P1): HEAD (resolves to the current checkout to main when on main), the bare colon matching-refspec, and wildcards (refs/heads/*) are rejected by the destination parser. - don't miscount flag values / shell prefix (P1): strip everything up to and including the push verb (so "cd repo && ..." no longer leaves phantom refspec tokens) and drop space-separated value-flag VALUES (-o ci.skip, --push-option ci.skip, --repo, --receive-pack, --exec) before the refspec count. Boolean flags (--force-if-includes, -q) are NOT in the value-flag set, so they don't eat the following positional. - reject delete pushes (P1, same carve-out surface): --delete/-d removes the remote ref and the flag-strip would leave it reading like a routine rebase; now rejected up front. - normalize quoted destinations: strip surrounding shell quotes so HEAD:'main' / 'master' / 'live-config' are still caught while 'feature' still carves out. Stale Codex threads (refs/heads/main gating, leading-plus, force-with-lease-foo boundary, *:main quoted) were already fixed on the pr-66 head and are resolved separately. Tests: 28 new assertions across 7 methods in tests/tools/test_approval.py covering protected branches, non-branch refs, shorthands, flag-value/prefix miscount, delete pushes, quoted destinations, and boolean-flag non-consumption, plus ordinary-feature negatives (mainline/my-main/feature/head still carve out). tests/tools/test_approval.py: 272 passed / 0 failed. --- tests/tools/test_approval.py | 122 +++++++++++++++++++++++++++++++++++ tools/approval.py | 120 ++++++++++++++++++++++++++++++---- 2 files changed, 230 insertions(+), 12 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index eea39ea0c1ed4..70c878a96b8dd 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1445,6 +1445,128 @@ def test_force_with_lease_re_excludes_trailing_hyphen_flag(self): assert rx.search("--force-with-lease=expected-sha") assert not rx.search("--force-with-lease-foo") + def test_git_push_lease_carveout_excludes_protected_integration_branches(self): + """A leased force-push to a long-lived integration branch the gateway + tracks (live-config) must NOT be carved out — auto-approving it + headlessly would bypass the PR-to-self review flow. Same posture as + main/master, in bare, refs/heads, and HEAD: forms.""" + for cmd in ( + "git push --force-with-lease origin live-config", + "git push --force-with-lease origin refs/heads/live-config", + "git push --force-with-lease origin HEAD:live-config", + "git push --force-with-lease origin feature:live-config", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + + def test_git_push_lease_carveout_rejects_non_branch_destinations(self): + """The carve-out is for refs/heads branch pushes ONLY. A leased rewrite + of a tag (or any non-refs/heads namespace) must keep prompting — a + forced tag move is just as destructive as a branch rewrite.""" + for cmd in ( + "git push --force-with-lease origin refs/tags/v1.0", + "git push --force-with-lease origin feature:refs/tags/v1.0", + "git push --force-with-lease origin refs/notes/commits", + "git push --force-with-lease origin refs/remotes/origin/x", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + + def test_git_push_lease_carveout_rejects_ambiguous_refspec_shorthands(self): + """Refspec shorthands that don't name a single explicit non-default + branch must keep prompting: `HEAD` (resolves to the current checkout, + which may be main), the bare `:` matching-refspec, and the + `refs/heads/*:refs/heads/*` wildcard (touches every branch incl. main).""" + for cmd in ( + "git push --force-with-lease origin HEAD", + "git push --force-with-lease origin :", + "git push --force-with-lease origin refs/heads/*:refs/heads/*", + "git push --force-with-lease origin 'refs/heads/*'", + "git push --force-with-lease origin feature:", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + + def test_git_push_lease_carveout_does_not_miscount_flag_values_or_prefix(self): + """The explicit-destination check must not be fooled by a `-o/ + --push-option ` value or a pre-`git` shell prefix being + miscounted as a refspec. `-o ci.skip origin` (no real refspec) and + `cd repo && git push --force-with-lease` (omitted refspec) must keep + prompting; a value-flag alongside a REAL refspec still carves out.""" + for cmd in ( + "git push --force-with-lease -o ci.skip origin", + "git push --force-with-lease --push-option ci.skip origin", + "cd repo && git push --force-with-lease", + "cd repo && git push --force-with-lease origin", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + # A value-flag PLUS a genuine feature refspec is still safe to carve out. + for cmd in ( + "git push --force-with-lease -o ci.skip origin feature", + "git push --force-with-lease --push-option=ci.skip origin feature", + "cd repo && git push --force-with-lease origin feature", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is False, f"expected allow, got block for: {cmd}" + + def test_git_push_lease_carveout_allows_ordinary_feature_branches(self): + """Branches that merely contain 'main'/'master' or a 'head' path + segment are ordinary feature branches and must still carve out — the + protected-branch and HEAD checks must not over-match.""" + for cmd in ( + "git push --force-with-lease origin mainline", + "git push --force-with-lease origin my-main", + "git push --force-with-lease origin feature/head", + "git push --force-with-lease origin refs/heads/feature", + "git push --force-with-lease origin HEAD:feature", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is False, f"expected allow, got block for: {cmd}" + + def test_git_push_lease_carveout_rejects_delete_pushes(self): + """A leased DELETE push (`--delete` / `-d`) removes the remote ref — + just as destructive as a force rewrite, and the flag-strip would + otherwise leave it reading like a routine feature rebase. Must keep + prompting regardless of branch.""" + for cmd in ( + "git push --force-with-lease --delete origin feature", + "git push --force-with-lease -d origin feature", + "git push --force-with-lease --delete origin main", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + + def test_git_push_lease_carveout_normalizes_quoted_protected_destination(self): + """A shell-quoted protected destination (`HEAD:'main'`, `'master'`) must + still be caught — the carve-out validates the bare branch name after + stripping surrounding quotes, while a quoted ordinary feature branch + still carves out.""" + for cmd in ( + "git push --force-with-lease origin HEAD:'main'", + "git push --force-with-lease origin 'master'", + "git push --force-with-lease origin 'live-config'", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + for cmd in ( + "git push --force-with-lease origin 'feature'", + "git push --force-with-lease origin HEAD:'feature'", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is False, f"expected allow, got block for: {cmd}" + + def test_git_push_lease_carveout_boolean_flags_do_not_eat_refspec(self): + """A boolean push flag like `--force-if-includes` takes no value, so it + must NOT consume the following positional. A feature push carrying one + still carves out (the refspec is intact).""" + for cmd in ( + "git push --force-with-lease --force-if-includes origin feature", + "git push --force-with-lease -q origin feature", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is False, f"expected allow, got block for: {cmd}" + def test_gh_pr_merge_flagged(self): """`gh pr merge` bypasses PR review and MUST be flagged.""" for cmd in ("gh pr merge 42 --squash", "gh pr merge 42", "gh pr merge"): diff --git a/tools/approval.py b/tools/approval.py index e647d6c65803e..ac6c2e57dcf95 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -882,10 +882,80 @@ def _scrub_quoted(m: re.Match) -> str: r'(?:^|[/:+\s])(?:main|master)(?:\s|$|[\'"`])' ) +# Long-lived integration branches the running gateway / deploy flow tracks +# directly. Auto-approving a headless leased force-push to one of these would +# bypass the PR-to-self review flow (AGENTS.md: `live-config` is the branch the +# live gateway checks out), so they are NEVER carved out — same posture as +# main/master. Lowercased; matched against the parsed destination branch name. +_PROTECTED_BRANCHES = frozenset({"main", "master", "live-config"}) + +# Push flags that consume the NEXT token as a value (space-separated form). If +# we don't drop the value too it survives token-splitting and gets miscounted +# as the remote or a refspec (`-o ci.skip origin` → `ci.skip` read as remote, +# `origin` read as a refspec). The `--flag=value` forms are already handled by +# the generic flag-strip, so only the space-separated short/long names matter. +# Only genuinely value-taking flags belong here — boolean flags like +# `--force-if-includes` must NOT be listed or they would wrongly eat the next +# positional (the remote/refspec). +_VALUE_FLAGS = frozenset({ + "-o", "--push-option", "--repo", "--receive-pack", "--exec", +}) + +# A destination that names exactly one ordinary branch under refs/heads — the +# only shape we auto-approve. Accepts a bare `feature`, `refs/heads/feature`, or +# the `src:dst` form (we validate the RHS destination). Rejects wildcards (`*`), +# tag/other-namespace refs (`refs/tags/…`, `refs/notes/…`), the `HEAD` +# shorthand, the bare `:` matching-refspec, and empty destinations. +_VALID_BRANCH_NAME_RE = re.compile(r'^[\w./-]+$') + + +def _refspec_destination(refspec: str) -> Optional[str]: + """Return the lowercased destination BRANCH NAME a push refspec writes to, + or ``None`` if the refspec is not a single, ordinary ``refs/heads`` branch + target this carve-out is willing to reason about. + + Handles `src:dst` (uses the RHS), bare `branch` / `refs/heads/branch`, and + rejects: wildcards (`*`), `HEAD` (resolves to the current checkout — may be + main), the bare `:` matching-refspec, an empty RHS (`src:` = delete), and + any ref outside `refs/heads/` (`refs/tags/…`, `refs/notes/…`, etc. — a + leased tag rewrite must NOT slip through). + """ + rs = refspec.strip() + # Strip surrounding shell quotes the detection view may still carry (the + # normalizer only removes EMPTY '' / "" literals). A quoted destination + # like HEAD:'main' or 'feature' must validate on its bare branch name so a + # quoted protected branch is still caught and a quoted feature branch still + # carves out. + rs = rs.strip('\'"`') + if not rs or '*' in rs: + return None + # `src:dst` — the written ref is the RHS. A bare `:` (matching refspec) or a + # `src:` (delete) leaves an empty dst → reject. + if ':' in rs: + dst = rs.split(':', 1)[1] + else: + dst = rs + dst = dst.strip('\'"`') + if not dst: + return None + # `HEAD` resolves to whatever branch is checked out — could be main. + if dst == 'head': + return None + # Qualified refs: only refs/heads/ is a branch push; refs/tags/…, + # refs/notes/…, refs/remotes/… and any other namespace are not carved out. + if dst.startswith('refs/'): + if not dst.startswith('refs/heads/'): + return None + dst = dst[len('refs/heads/'):] + if not dst or not _VALID_BRANCH_NAME_RE.match(dst): + return None + return dst + def _is_safe_lease_push_to_feature_branch(command_lower: str) -> bool: - """True iff *command_lower* is a force-WITH-LEASE git push to a NON-default - branch — the exact case the shell git-guard auto-allows. + """True iff *command_lower* is a force-WITH-LEASE git push to a single, + ordinary, NON-protected branch — the exact case the shell git-guard + auto-allows. *command_lower* is the already-normalized + lowercased command (same view the dangerous-pattern loop scans). Returns False (→ keep prompting) for: @@ -900,8 +970,11 @@ def _is_safe_lease_push_to_feature_branch(command_lower: str) -> bool: push.default and may push the current branch — which could be main), - a refspec carrying a leading `+` (git documents `+:` as the same forced update as `--force`, with NO lease safety belt), - - a push whose refspec/target is the default branch (main/master), in any - form incl. `refs/heads/main` / `HEAD:refs/heads/main`. + - a destination that is not a single `refs/heads/` target: the + `HEAD` shorthand, the bare `:` matching-refspec, a wildcard + (`refs/heads/*:refs/heads/*`), or a non-branch ref (`refs/tags/…`), + - a push to a PROTECTED branch (main/master/live-config), in any form + incl. `refs/heads/main` / `HEAD:refs/heads/main`. """ if not re.search(r'\bgit\s+push\b', command_lower): return False @@ -918,14 +991,28 @@ def _is_safe_lease_push_to_feature_branch(command_lower: str) -> bool: # main/master), so a "no main token in the args" check cannot vouch for them. if re.search(r'(? ` and `=` forms. + value_flag_alt = '|'.join(re.escape(f) for f in _VALUE_FLAGS) + args = re.sub(rf'(? bool: # Reject a leading-`+` refspec (forced update, no lease guarantee). if any(rs.startswith('+') or ':+' in rs for rs in refspecs): return False - # Reject if any refspec/target is the default branch — let the main/master - # patterns (and this guard's refusal) handle it via the normal prompt. + # Every refspec must name a single ordinary branch under refs/heads, and + # NONE of them may target a protected branch. A refspec we can't parse to a + # clean branch destination (tag ref, HEAD, `:`, wildcard) fails the push. + for rs in refspecs: + dst = _refspec_destination(rs) + if dst is None: + return False + if dst in _PROTECTED_BRANCHES: + return False + # Final backstop: the main/master refspec patterns (catches forms the parse + # above might normalize differently). Keeps the two layers from disagreeing. if _DEFAULT_BRANCH_PUSH_RE.search(command_lower): return False return True From c109343f94e05f1c2d4217cd54036cb910f79444 Mon Sep 17 00:00:00 2001 From: exiao Date: Mon, 29 Jun 2026 13:33:17 -0400 Subject: [PATCH 05/12] fix(approval): close 3 codex P1 gaps in lease-push carve-out (#66) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second Codex pass on the pr-66 head flagged 3 more headless auto-approve gaps in _is_safe_lease_push_to_feature_branch — each reproduced before this change: - reject --tags pushes (P1): `--tags` pushes tags alongside the named refspec, so a leased `--force-with-lease=refs/tags/v1: --tags origin feature` can force-update a tag while the visible refspec looks like a routine feature push. Added `tags` to the broadcast rejection (next to --all/--mirror). (--tags has no short form on git push.) - consume --recurse-submodules value (P1): the space form `--recurse-submodules on-demand` consumes the next word; without dropping the value it survived as `origin` and got miscounted as a refspec, carving out an omitted-refspec push (push.default → could be main). Added to _VALUE_FLAGS. - strip numeric short flags (P1): the generic flag-strip only matched letter-first flags, so `-4`/`-6` (IPv4/IPv6) survived and `origin` was misread as a refspec. Broadened the leading char class to `[a-z0-9]` (a remote/refspec never starts with a dash, so flags only). Tests: 3 new methods in tests/tools/test_approval.py (block on the bare forms, still carve out with a real feature refspec). 276 passed / 0 failed. --- tests/tools/test_approval.py | 56 ++++++++++++++++++++++++++++++++++++ tools/approval.py | 18 ++++++++++-- 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index 70c878a96b8dd..e8966fc4bddd1 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1567,6 +1567,62 @@ def test_git_push_lease_carveout_boolean_flags_do_not_eat_refspec(self): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is False, f"expected allow, got block for: {cmd}" + def test_git_push_lease_carveout_rejects_tag_pushes(self): + """`--tags` pushes tags alongside the named refspec, so a leased + `--force-with-lease=refs/tags/v1: --tags origin feature` can + force-update a tag while the visible refspec looks like a routine + feature push. The branch-only carve-out must keep prompting; an + ordinary feature push without `--tags` still carves out.""" + for cmd in ( + "git push --force-with-lease --tags origin feature", + "git push --force-with-lease=refs/tags/v1:old --tags origin feature", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + dangerous, _, _ = detect_dangerous_command( + "git push --force-with-lease origin feature" + ) + assert dangerous is False + + def test_git_push_lease_carveout_consumes_recurse_submodules_value(self): + """`--recurse-submodules ` consumes the next word in + its space form. Without dropping the value, `--recurse-submodules + on-demand origin` leaves `origin` miscounted as a refspec and would + carve out an omitted-refspec push (which follows push.default → could be + main). Must keep prompting; the `=value` form plus a real refspec still + carves out.""" + for cmd in ( + "git push --force-with-lease --recurse-submodules on-demand origin", + "git push --force-with-lease --recurse-submodules check origin", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + for cmd in ( + "git push --force-with-lease --recurse-submodules=on-demand origin feature", + "git push --force-with-lease --recurse-submodules on-demand origin feature", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is False, f"expected allow, got block for: {cmd}" + + def test_git_push_lease_carveout_strips_numeric_short_flags(self): + """Numeric short flags `-4`/`-6` (IPv4/IPv6) take no value but the + generic flag-strip used to only match letter-first flags, leaving `-4` + to be miscounted as a refspec so `git push --force-with-lease -4 origin` + (omitted refspec) wrongly carved out. Must keep prompting; a numeric + flag alongside a REAL feature refspec still carves out.""" + for cmd in ( + "git push --force-with-lease -4 origin", + "git push --force-with-lease -6 origin", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + for cmd in ( + "git push --force-with-lease -4 origin feature", + "git push --force-with-lease -6 origin feature", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is False, f"expected allow, got block for: {cmd}" + def test_gh_pr_merge_flagged(self): """`gh pr merge` bypasses PR review and MUST be flagged.""" for cmd in ("gh pr merge 42 --squash", "gh pr merge 42", "gh pr merge"): diff --git a/tools/approval.py b/tools/approval.py index ac6c2e57dcf95..54c52677e9fc1 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -899,6 +899,11 @@ def _scrub_quoted(m: re.Match) -> str: # positional (the remote/refspec). _VALUE_FLAGS = frozenset({ "-o", "--push-option", "--repo", "--receive-pack", "--exec", + # `--recurse-submodules ` consumes the next word in its + # space form; if we don't drop the value it survives token-splitting and + # gets miscounted as the remote/refspec (`--recurse-submodules on-demand + # origin` → `origin` read as a refspec). + "--recurse-submodules", }) # A destination that names exactly one ordinary branch under refs/heads — the @@ -989,7 +994,11 @@ def _is_safe_lease_push_to_feature_branch(command_lower: str) -> bool: return False # Reject a broadcast push: `--all`/`--mirror` push every local ref (incl. # main/master), so a "no main token in the args" check cannot vouch for them. - if re.search(r'(?` can rewrite a tag while the + # visible refspec looks like a routine feature push, so the branch-only + # carve-out must keep prompting for it. + if re.search(r'(? bool: value_flag_alt = '|'.join(re.escape(f) for f in _VALUE_FLAGS) args = re.sub(rf'(? Date: Mon, 29 Jun 2026 13:36:37 -0400 Subject: [PATCH 06/12] fix(approval): reject colon-prefix branch delete in lease carve-out (PR #66 P1) A leased colon-prefix delete push (the `:branch` shorthand that removes a remote branch) returned carve=True and auto-approved the deletion headlessly. The --delete/-d flag forms were already rejected, but _refspec_destination only rejected an empty DST (src: = delete), never an empty SOURCE (:dst). The split-on-colon returned the RHS as a clean destination, so no DANGEROUS_PATTERN caught it; protected :main/:live-config were saved only incidentally. Fix: in _refspec_destination, reject (return None) when the SOURCE side of src:dst is empty -- a :dst push is a branch delete and must NOT auto-approve. Added the colon-prefix delete cases to test_git_push_lease_carveout_rejects_delete_pushes. Patch note: ~/.hermes/plans/hermes-patches/force-push-lease-feature-branch-carveout.md --- tests/tools/test_approval.py | 15 +++++++++++---- tools/approval.py | 15 ++++++++++----- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index e8966fc4bddd1..dea64df9c7ec7 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1525,14 +1525,21 @@ def test_git_push_lease_carveout_allows_ordinary_feature_branches(self): assert dangerous is False, f"expected allow, got block for: {cmd}" def test_git_push_lease_carveout_rejects_delete_pushes(self): - """A leased DELETE push (`--delete` / `-d`) removes the remote ref — - just as destructive as a force rewrite, and the flag-strip would - otherwise leave it reading like a routine feature rebase. Must keep - prompting regardless of branch.""" + """A leased DELETE push removes the remote ref — just as destructive as + a force rewrite. Both the `--delete` / `-d` flag forms AND the + colon-prefix `:branch` shorthand (`git push origin :feature`) must keep + prompting regardless of branch — the flag-strip / RHS-parse would + otherwise leave them reading like a routine feature rebase.""" for cmd in ( "git push --force-with-lease --delete origin feature", "git push --force-with-lease -d origin feature", "git push --force-with-lease --delete origin main", + # Colon-prefix delete shorthand: empty SOURCE in `src:dst`. + "git push --force-with-lease origin :feature", + "git push --force-with-lease origin :refs/heads/feature", + "git push --force-with-lease origin :main", + "git push --force-with-lease origin :live-config", + "git push --force-with-lease origin :'feature'", ): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is True, f"expected block, got allow for: {cmd}" diff --git a/tools/approval.py b/tools/approval.py index 54c52677e9fc1..6130656f4dc99 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -921,9 +921,10 @@ def _refspec_destination(refspec: str) -> Optional[str]: Handles `src:dst` (uses the RHS), bare `branch` / `refs/heads/branch`, and rejects: wildcards (`*`), `HEAD` (resolves to the current checkout — may be - main), the bare `:` matching-refspec, an empty RHS (`src:` = delete), and - any ref outside `refs/heads/` (`refs/tags/…`, `refs/notes/…`, etc. — a - leased tag rewrite must NOT slip through). + main), the bare `:` matching-refspec, an empty RHS (`src:` = delete), an + empty SOURCE (`:dst` = the colon-prefix delete shorthand), and any ref + outside `refs/heads/` (`refs/tags/…`, `refs/notes/…`, etc. — a leased tag + rewrite must NOT slip through). """ rs = refspec.strip() # Strip surrounding shell quotes the detection view may still carry (the @@ -935,9 +936,13 @@ def _refspec_destination(refspec: str) -> Optional[str]: if not rs or '*' in rs: return None # `src:dst` — the written ref is the RHS. A bare `:` (matching refspec) or a - # `src:` (delete) leaves an empty dst → reject. + # `src:` (delete) leaves an empty dst → reject. An empty SOURCE (`:dst`) is + # the colon-prefix DELETE shorthand (`git push origin :feature` removes the + # remote ref) — just as destructive as `--delete`, so reject it too. if ':' in rs: - dst = rs.split(':', 1)[1] + src, dst = rs.split(':', 1) + if not src.strip('\'"`'): + return None else: dst = rs dst = dst.strip('\'"`') From 93cac1943bcfdd61b31fd8db1003c2ebb57f6adb Mon Sep 17 00:00:00 2001 From: exiao Date: Mon, 29 Jun 2026 13:45:29 -0400 Subject: [PATCH 07/12] fix(approval): reject packed numeric+force short bundle in lease carve-out (#66 P1) Codex flagged a packed short-flag bypass beyond the prior `-4` flag-strip fix: `git push -h` lists numeric `-4`/`-6` (IPv4/IPv6) AND `-f` force, and Git accepts packed combos like `-4f` (IPv4 + bare force). The bare-force detector `_SHORT_FORCE_FLAG_RE` only allowed LETTERS around `f` (`-[a-z]*f[a-z]*`), so `-4f` was not recognized as a bare force; the generic flag-strip then erased the whole token and `_is_safe_lease_push_to_feature_branch()` carved out a push that overrides the lease with no safety belt. Fix: broaden the short-force char class to `[a-z0-9]` (`-[a-z0-9]*f[a-z0-9]*`) so `-4f`/`-6f`/`-f4` are caught as a bare force and keep prompting. A packed bundle without `f` (e.g. `-uq`) is unaffected. Tests: test_git_push_lease_carveout_rejects_packed_numeric_force_bundle (blocks -4f/-6f/-f4, still carves out -uq). 277 passed / 0 failed. --- tests/tools/test_approval.py | 20 ++++++++++++++++++++ tools/approval.py | 10 +++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index dea64df9c7ec7..7050b12b11be2 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1630,6 +1630,26 @@ def test_git_push_lease_carveout_strips_numeric_short_flags(self): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is False, f"expected allow, got block for: {cmd}" + def test_git_push_lease_carveout_rejects_packed_numeric_force_bundle(self): + """A packed short bundle like `-4f` is IPv4 + a BARE `--force` (no lease + safety belt), but the short-force regex used to only allow letters + around `f`, so `-4f`/`-6f` slipped past the bare-force check and the + generic flag-strip then erased the whole token. A leased push carrying a + packed numeric force bundle must keep prompting; a packed all-letter + bundle without `f` (e.g. `-uq`) still carves out.""" + for cmd in ( + "git push --force-with-lease -4f origin feature", + "git push --force-with-lease -6f origin feature", + "git push --force-with-lease -f4 origin feature", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + # A packed bundle WITHOUT a force `f` is a normal feature push. + dangerous, _, _ = detect_dangerous_command( + "git push --force-with-lease -uq origin feature" + ) + assert dangerous is False + def test_gh_pr_merge_flagged(self): """`gh pr merge` bypasses PR review and MUST be flagged.""" for cmd in ("gh pr merge 42 --squash", "gh pr merge 42", "gh pr merge"): diff --git a/tools/approval.py b/tools/approval.py index 6130656f4dc99..349a24e764ee8 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -863,9 +863,13 @@ def _scrub_quoted(m: re.Match) -> str: # contains the substring `--force`, so require a word boundary that the lease # suffix does not satisfy: --force/--force=… followed by end or a non-hyphen. _BARE_FORCE_FLAG_RE = re.compile(r'--force(?![\w-])') -# Short force flag: -f, or packed combos like -uf / -fv (a flag token that is -# all letters and contains an `f`). Excludes long flags (already handled above). -_SHORT_FORCE_FLAG_RE = re.compile(r'(? Date: Mon, 29 Jun 2026 13:52:54 -0400 Subject: [PATCH 08/12] fix(approval): reject `git push tag ` shorthand in lease carve-out (#66 P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex flagged the tag-shorthand bypass: git documents `git push tag ` as sugar for `refs/tags/:refs/tags/`, so a leased `--force-with-lease=refs/tags/v1: origin tag v1` force-updates a TAG while the parsed tokens (`tag`, `v1`) both look like ordinary branch refspecs and the branch-only carve-out wrongly skipped the force-push approval. Fix: reject when the first refspec token is the literal `tag` AND a name trails it (the shorthand requires both). A lone branch literally named `tag` (`git push origin tag`) is unaffected — it's validated as a normal branch destination. Tests: test_git_push_lease_carveout_rejects_tag_shorthand (blocks the `tag v1` shorthand + the explicit-lease tag form, still carves out a lone `tag` branch). 278 passed / 0 failed. --- tests/tools/test_approval.py | 19 +++++++++++++++++++ tools/approval.py | 9 +++++++++ 2 files changed, 28 insertions(+) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index 7050b12b11be2..5d45e8b015fff 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1650,6 +1650,25 @@ def test_git_push_lease_carveout_rejects_packed_numeric_force_bundle(self): ) assert dangerous is False + def test_git_push_lease_carveout_rejects_tag_shorthand(self): + """The `git push tag ` shorthand expands to a tag + refspec (`refs/tags/`), so a leased `--force-with-lease=refs/ + tags/v1: origin tag v1` force-updates a TAG while the tokens + (`tag`, `v1`) look like two ordinary branch refspecs. Must keep + prompting. A branch literally named `tag` (lone token) is still an + ordinary feature push and carves out.""" + for cmd in ( + "git push --force-with-lease origin tag v1", + "git push --force-with-lease=refs/tags/v1:old origin tag v1", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + # A lone branch named "tag" is a normal feature push. + dangerous, _, _ = detect_dangerous_command( + "git push --force-with-lease origin tag" + ) + assert dangerous is False + def test_gh_pr_merge_flagged(self): """`gh pr merge` bypasses PR review and MUST be flagged.""" for cmd in ("gh pr merge 42 --squash", "gh pr merge 42", "gh pr merge"): diff --git a/tools/approval.py b/tools/approval.py index 349a24e764ee8..9ab61d04cfceb 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -1041,6 +1041,15 @@ def _is_safe_lease_push_to_feature_branch(command_lower: str) -> bool: if len(tokens) < 2: return False refspecs = tokens[1:] + # Reject the `git push tag ` shorthand: git documents `tag + # ` as sugar for `refs/tags/:refs/tags/`, so a leased + # `--force-with-lease=refs/tags/v1: origin tag v1` force-updates a TAG + # while the tokens (`tag`, `v1`) look like two ordinary branch refspecs. The + # shorthand needs the literal `tag` keyword FOLLOWED by a name, so only + # reject when a name trails it (a lone `tag` is an ordinary branch named + # "tag", validated as a normal destination below). Keep prompting otherwise. + if len(refspecs) >= 2 and refspecs[0] == 'tag': + return False # Reject a leading-`+` refspec (forced update, no lease guarantee). if any(rs.startswith('+') or ':+' in rs for rs in refspecs): return False From 2ceadbe4e7079a6dfd8faf4149a85ec8de413704 Mon Sep 17 00:00:00 2001 From: exiao Date: Mon, 29 Jun 2026 14:04:51 -0400 Subject: [PATCH 09/12] fix(approval): close 3 more lease carve-out gaps; shlex-tokenize args (#66 P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex 6th-pass follow-up. Three cleanly-fixable bypasses on the pr-66 head: - packed numeric DELETE bundle (`-4d` = IPv4 + delete): the delete guard only allowed letters around `d`, so `-4d` slipped past and the flag-strip erased it, deleting a remote branch headlessly. Broadened to `[a-z0-9]` (mirrors the `-4f` packed-force fix). - `tag ` shorthand at ANY refspec position: git accepts `origin feature tag v1`, not just `origin tag v1`. The check only rejected `refspecs[0] == 'tag'`; now rejects any `tag` token followed by a name (a lone trailing `tag` is still an ordinary branch named "tag"). - quoted option value with whitespace (`--push-option 'ci skip'`): the plain whitespace split leaked `skip` as a phantom refspec. Replaced the regex flag-strip + `str.split()` with `shlex.split()` (try/except → keep prompting on unbalanced quotes) and token-level value-flag consumption, so a quoted value is one argument and is consumed whole. Tests: 2 new methods (packed numeric delete, quoted option value) + extended the tag-shorthand test to cover the trailing-position form. 280 passed / 0 failed. --- tests/tools/test_approval.py | 40 ++++++++++++++++++++++ tools/approval.py | 66 +++++++++++++++++++++++------------- 2 files changed, 82 insertions(+), 24 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index 5d45e8b015fff..e1229fe12afe5 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1660,6 +1660,9 @@ def test_git_push_lease_carveout_rejects_tag_shorthand(self): for cmd in ( "git push --force-with-lease origin tag v1", "git push --force-with-lease=refs/tags/v1:old origin tag v1", + # The `tag ` shorthand may also trail other refspecs. + "git push --force-with-lease origin feature tag v1", + "git push --force-with-lease=refs/tags/v1:old origin feature tag v1", ): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is True, f"expected block, got allow for: {cmd}" @@ -1669,6 +1672,43 @@ def test_git_push_lease_carveout_rejects_tag_shorthand(self): ) assert dangerous is False + def test_git_push_lease_carveout_rejects_packed_numeric_delete_bundle(self): + """A packed short bundle like `-4d` is IPv4 + a delete (`-d`), but the + delete guard used to only allow letters around `d`, so `-4d` slipped + past and the generic flag-strip erased it — a leased push then read like + a routine feature rebase and the remote branch was deleted without + approval. Must keep prompting; a packed bundle without `d`/`f` carves + out.""" + for cmd in ( + "git push --force-with-lease -4d origin feature", + "git push --force-with-lease -6d origin feature", + "git push --force-with-lease -d4 origin feature", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + dangerous, _, _ = detect_dangerous_command( + "git push --force-with-lease -uq origin feature" + ) + assert dangerous is False + + def test_git_push_lease_carveout_handles_quoted_option_value(self): + """A QUOTED option value carrying whitespace (`--push-option 'ci skip'`) + is a single shell argument, so the value must be consumed whole — a + plain whitespace split would leak `skip` as a phantom refspec and carve + out an omitted-refspec push (push.default → could be main). Must keep + prompting; the same quoted value alongside a REAL feature refspec still + carves out.""" + for cmd in ( + "git push --force-with-lease --push-option 'ci skip' origin", + "git push --force-with-lease -o 'ci skip' origin", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + dangerous, _, _ = detect_dangerous_command( + "git push --force-with-lease --push-option 'ci skip' origin feature" + ) + assert dangerous is False + def test_gh_pr_merge_flagged(self): """`gh pr merge` bypasses PR review and MUST be flagged.""" for cmd in ("gh pr merge 42 --squash", "gh pr merge 42", "gh pr merge"): diff --git a/tools/approval.py b/tools/approval.py index 9ab61d04cfceb..b39dee38dc121 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -14,6 +14,7 @@ import logging import os import re +import shlex import sys import threading import time @@ -1013,8 +1014,10 @@ def _is_safe_lease_push_to_feature_branch(command_lower: str) -> bool: # entirely (just as destructive as a force rewrite), and the flag-strip # below would erase the `--delete` token and leave the branch reading like a # routine rebase target. A deletion is never the carve-out's "rebase a - # feature branch" intent, so keep prompting. - if re.search(r'(? bool: # Strip EVERYTHING up to and including the `git push` verb so a shell prefix # (`cd repo && git push …`) does not survive as a phantom remote/refspec. args = re.sub(r'^.*?\bgit\s+push\b', ' ', without_lease) - # Drop the VALUE of space-separated value-flags FIRST (`-o ci.skip`, - # `--push-option ci.skip`, `--repo url`). If we stripped the flag alone, its - # value would survive token-splitting and be miscounted as the remote or a - # refspec. Handle both the ` ` and `=` forms. - value_flag_alt = '|'.join(re.escape(f) for f in _VALUE_FLAGS) - args = re.sub(rf'(? tag ` shorthand: git documents `tag - # ` as sugar for `refs/tags/:refs/tags/`, so a leased - # `--force-with-lease=refs/tags/v1: origin tag v1` force-updates a TAG - # while the tokens (`tag`, `v1`) look like two ordinary branch refspecs. The - # shorthand needs the literal `tag` keyword FOLLOWED by a name, so only - # reject when a name trails it (a lone `tag` is an ordinary branch named - # "tag", validated as a normal destination below). Keep prompting otherwise. - if len(refspecs) >= 2 and refspecs[0] == 'tag': + # Reject the `git push ... tag ` shorthand: git documents + # `tag ` as sugar for `refs/tags/:refs/tags/`, and it may + # appear at ANY refspec position (`origin tag v1`, `origin feature tag v1`), + # so a leased `--force-with-lease=refs/tags/v1: origin feature tag v1` + # force-updates a TAG while the `tag`/`v1` tokens look like ordinary branch + # refspecs. The shorthand is the literal `tag` keyword FOLLOWED by a name, + # so reject whenever a `tag` token has another token after it (a lone + # trailing `tag` is an ordinary branch named "tag", validated below). + if any(rs == 'tag' and i + 1 < len(refspecs) for i, rs in enumerate(refspecs)): return False # Reject a leading-`+` refspec (forced update, no lease guarantee). if any(rs.startswith('+') or ':+' in rs for rs in refspecs): From 3d844c820002cdce71def7c0c4291fec4ae87aae Mon Sep 17 00:00:00 2001 From: exiao Date: Mon, 29 Jun 2026 14:10:47 -0400 Subject: [PATCH 10/12] fix(approval): reject unqualified colon push destinations --- tests/tools/test_approval.py | 38 +++++++++++++++++++++++++++--------- tools/approval.py | 15 +++++++++++++- 2 files changed, 43 insertions(+), 10 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index e1229fe12afe5..1c30fa68cb373 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1519,11 +1519,31 @@ def test_git_push_lease_carveout_allows_ordinary_feature_branches(self): "git push --force-with-lease origin my-main", "git push --force-with-lease origin feature/head", "git push --force-with-lease origin refs/heads/feature", - "git push --force-with-lease origin HEAD:feature", + "git push --force-with-lease origin HEAD:refs/heads/feature", ): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is False, f"expected allow, got block for: {cmd}" + def test_git_push_lease_carveout_rejects_unqualified_colon_destinations(self): + """A colon-form refspec writes the RHS destination. If that RHS is + unqualified, Git may expand it against an existing remote tag/other ref + (`HEAD:v1` can update `refs/tags/v1` when such a tag exists). The + branch-only carve-out cannot resolve remote namespaces, so colon-form + destinations must be explicit `refs/heads/` refs. Bare + source-only branch pushes (`origin feature`) remain allowed above.""" + for cmd in ( + "git push --force-with-lease=refs/tags/v1:old origin HEAD:v1", + "git push --force-with-lease origin HEAD:feature", + "git push --force-with-lease origin feature:v1", + "git push --force-with-lease origin refs/heads/feature:v1", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + dangerous, _, _ = detect_dangerous_command( + "git push --force-with-lease origin HEAD:refs/heads/feature" + ) + assert dangerous is False + def test_git_push_lease_carveout_rejects_delete_pushes(self): """A leased DELETE push removes the remote ref — just as destructive as a force rewrite. Both the `--delete` / `-d` flag forms AND the @@ -1547,21 +1567,21 @@ def test_git_push_lease_carveout_rejects_delete_pushes(self): def test_git_push_lease_carveout_normalizes_quoted_protected_destination(self): """A shell-quoted protected destination (`HEAD:'main'`, `'master'`) must still be caught — the carve-out validates the bare branch name after - stripping surrounding quotes, while a quoted ordinary feature branch - still carves out.""" + stripping surrounding quotes. A quoted bare source-only feature branch + still carves out; quoted colon-form destinations stay gated unless the + RHS is fully-qualified as refs/heads (covered above).""" for cmd in ( "git push --force-with-lease origin HEAD:'main'", + "git push --force-with-lease origin HEAD:'feature'", "git push --force-with-lease origin 'master'", "git push --force-with-lease origin 'live-config'", ): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is True, f"expected block, got allow for: {cmd}" - for cmd in ( - "git push --force-with-lease origin 'feature'", - "git push --force-with-lease origin HEAD:'feature'", - ): - dangerous, _, _ = detect_dangerous_command(cmd) - assert dangerous is False, f"expected allow, got block for: {cmd}" + dangerous, _, _ = detect_dangerous_command( + "git push --force-with-lease origin 'feature'" + ) + assert dangerous is False def test_git_push_lease_carveout_boolean_flags_do_not_eat_refspec(self): """A boolean push flag like `--force-if-includes` takes no value, so it diff --git a/tools/approval.py b/tools/approval.py index b39dee38dc121..3a80f2cb2e205 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -944,7 +944,16 @@ def _refspec_destination(refspec: str) -> Optional[str]: # `src:` (delete) leaves an empty dst → reject. An empty SOURCE (`:dst`) is # the colon-prefix DELETE shorthand (`git push origin :feature` removes the # remote ref) — just as destructive as `--delete`, so reject it too. - if ':' in rs: + # + # In colon form, require the RHS destination to be fully qualified as + # `refs/heads/`. Git resolves an unqualified RHS against the remote + # namespace, so `HEAD:v1` can update an existing remote tag `refs/tags/v1`; + # without remote ref resolution, the branch-only carve-out cannot safely + # accept unqualified colon destinations. Bare source-only pushes like + # `git push origin feature` are handled by the `else` branch and remain the + # ordinary feature-branch convenience this carve-out exists to allow. + colon_form = ':' in rs + if colon_form: src, dst = rs.split(':', 1) if not src.strip('\'"`'): return None @@ -958,6 +967,10 @@ def _refspec_destination(refspec: str) -> Optional[str]: return None # Qualified refs: only refs/heads/ is a branch push; refs/tags/…, # refs/notes/…, refs/remotes/… and any other namespace are not carved out. + # In colon form, an unqualified destination is also not carved out because + # Git may expand it to an existing non-branch remote ref. + if colon_form and not dst.startswith('refs/heads/'): + return None if dst.startswith('refs/'): if not dst.startswith('refs/heads/'): return None From e631b8b9044b86d88c2896c62220ddc6af1ecf20 Mon Sep 17 00:00:00 2001 From: exiao Date: Mon, 29 Jun 2026 14:12:56 -0400 Subject: [PATCH 11/12] fix(approval): drop bare `--` option terminator before refspec count (#66 P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex 7th-pass: `git push --force-with-lease origin --` survived the flag-strip (the bare `--` matches no `[a-z0-9]` flag pattern), so tokens became `[origin, --]`, len >= 2 passed the explicit-refspec check, and the carve-out skipped the force-push approval — but git treats `--` as the option terminator, NOT a refspec, so it's an omitted-refspec push that follows push.default (could rewrite the current protected branch). Fix: drop the bare `--` token alongside the flag-strip so it doesn't count toward the explicit-destination check. A real refspec after `--` (`origin -- feature`) still carves out. Tests: test_git_push_lease_carveout_rejects_bare_option_terminator. 282 passed / 0 failed. --- tests/tools/test_approval.py | 17 +++++++++++++++++ tools/approval.py | 9 +++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index 1c30fa68cb373..ff11a4d4e0cae 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1729,6 +1729,23 @@ def test_git_push_lease_carveout_handles_quoted_option_value(self): ) assert dangerous is False + def test_git_push_lease_carveout_rejects_bare_option_terminator(self): + """The bare `--` option terminator is not a refspec — git treats + `git push --force-with-lease origin --` as an OMITTED-refspec push + (push.default → could be main). It must not be counted as proof of an + explicit destination, so the carve-out keeps prompting. A real feature + refspec after `--` still carves out.""" + for cmd in ( + "git push --force-with-lease origin --", + "git push --force-with-lease -- origin", + ): + dangerous, _, _ = detect_dangerous_command(cmd) + assert dangerous is True, f"expected block, got allow for: {cmd}" + dangerous, _, _ = detect_dangerous_command( + "git push --force-with-lease origin -- feature" + ) + assert dangerous is False + def test_gh_pr_merge_flagged(self): """`gh pr merge` bypasses PR review and MUST be flagged.""" for cmd in ("gh pr merge 42 --squash", "gh pr merge 42", "gh pr merge"): diff --git a/tools/approval.py b/tools/approval.py index 3a80f2cb2e205..48ba386058aca 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -1065,8 +1065,13 @@ def _is_safe_lease_push_to_feature_branch(command_lower: str) -> bool: # A short flag's leading char may be a letter (`--repo`, `-q`) OR a digit — # git push documents numeric short flags `-4`/`-6` (IPv4/IPv6); without the # digit they'd survive and be miscounted as a refspec. A remote/refspec - # never starts with a dash, so this only drops flags. - tokens = [t for t in pruned if not re.match(r'--?[a-z0-9]', t)] + # never starts with a dash, so this only drops flags. Also drop the bare `--` + # option terminator: git does not treat it as a refspec (`git push origin --` + # is an omitted-refspec push), so it must not count toward the explicit- + # destination check below. + tokens = [ + t for t in pruned if t != '--' and not re.match(r'--?[a-z0-9]', t) + ] # First surviving token is the remote; a refspec must follow it. if len(tokens) < 2: return False From 5814861e5076f458beeef563367ede727ee805e1 Mon Sep 17 00:00:00 2001 From: exiao Date: Mon, 29 Jun 2026 14:24:38 -0400 Subject: [PATCH 12/12] fix(approval): scope lease-push backstop to refspecs --- tests/tools/test_approval.py | 2 ++ tools/approval.py | 6 ++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index ff11a4d4e0cae..73ed06e11e3a9 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1506,6 +1506,8 @@ def test_git_push_lease_carveout_does_not_miscount_flag_values_or_prefix(self): "git push --force-with-lease -o ci.skip origin feature", "git push --force-with-lease --push-option=ci.skip origin feature", "cd repo && git push --force-with-lease origin feature", + "cd main && git push --force-with-lease origin feature", + "cd /tmp/master && git push --force-with-lease origin feature", ): dangerous, _, _ = detect_dangerous_command(cmd) assert dangerous is False, f"expected allow, got block for: {cmd}" diff --git a/tools/approval.py b/tools/approval.py index 48ba386058aca..d5aeb809a4b2c 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -1099,8 +1099,10 @@ def _is_safe_lease_push_to_feature_branch(command_lower: str) -> bool: if dst in _PROTECTED_BRANCHES: return False # Final backstop: the main/master refspec patterns (catches forms the parse - # above might normalize differently). Keeps the two layers from disagreeing. - if _DEFAULT_BRANCH_PUSH_RE.search(command_lower): + # above might normalize differently). Limit this to the parsed push refspecs + # so an unrelated shell prefix/path like `cd main && git push ... feature` + # does not defeat the safe feature-branch carve-out. + if _DEFAULT_BRANCH_PUSH_RE.search(' '.join(refspecs)): return False return True