fix(approval): narrow force-push gate to match shell guard (auto-allow lease push to feature branch) - #66
Conversation
…w lease push to feature branch) 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 NousResearch#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
🔎 Lint report:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a branch-aware force-push carve-out that allows --force-with-lease pushes to feature branches while keeping bare force pushes and pushes targeting default branches (main/master) flagged. The review feedback identifies a potential security bypass where fully qualified refspecs (e.g., refs/heads/main) are not intercepted, suggesting regex and test updates to cover these cases. It also recommends refining the --force-with-lease regex to avoid matching flags with trailing hyphens.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4f8962356
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…arve-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).
|
Pushed 747add7 addressing the review findings: Critical (refs/heads/main bypass) — fixed. P1 (leading- P1 (require explicit feature ref) — fixed. The carve-out now rejects P1 (live-config not carved out): out of scope for this fix loop — the carve-out deliberately mirrors the shell guard, which only protects main/master. Protecting Tests: extended |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 747add7a90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`_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 `=<expected>` value form is unaffected. Adds a regex-boundary test. Addresses gemini-code-assist review (line 869).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46b2bcc1a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Blocking review addressed: the 3 gemini-code-assist threads (lines 880/869/test 1399) are resolved — the lease-regex hyphen-boundary fix landed in 46b2bcc (with a new boundary test), and the two The 8 Codex P1/P2 findings on the force-push carve-out are deferred to a focused follow-up (not blocking this PR) — tracked internally. Several are genuine hardening gaps worth a dedicated pass: excluding |
) 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/<branch>. 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec681ac45d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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:<old> --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.
#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
|
P1 fix pushed (93573e9): colon-prefix
Fix: reject (return None) when the source side of Verification: original approval.py + new test → 1 FAIL (the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93573e9c1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e-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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93cac1943b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…se carve-out (#66 P1) Codex flagged the tag-shorthand bypass: git documents `git push <remote> tag <tag>` as sugar for `refs/tags/<tag>:refs/tags/<tag>`, so a leased `--force-with-lease=refs/tags/v1:<old> 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 467ecf3ee3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…#66 P1) 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 <name>` 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ceadbe4e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…66 P1) 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e631b8b904
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
What
Narrow the runtime force-push approval gate to mirror the shell git-guard
(
~/.local/bin/git): auto-allow a force-with-lease push to a non-defaultbranch; keep prompting for everything else.
Why
Two independent layers gate a force-push:
feature branch (rebasing your own PR) is allowed; it only blocks a force-push
that could rewrite main/master. A force-with-lease to a feature branch
sails through.
detect_dangerous_commandintools/approval.py) flagged every force push for an operator yes/no,regardless of target branch. Headless workers can't self-grant, so a routine
force-with-lease PR rebase "sits in pending_approval and never executes
headless" and a human has to approve a push the shell layer already considers
safe. Observed repeatedly: t_c5e6855d, t_d35127e9 (PR [Bug]: get_due_jobs may skip legacy naive timestamps under non-local Hermes timezone NousResearch/hermes-agent#806 branch
wt/t_89fe8606), and the PR fix(kanban): scrub inherited TERMINAL_* backend/image vars in worker spawn #141 babysit flow.How
New helper
_is_safe_lease_push_to_feature_branch(command_lower)returns Trueonly for a
git pushthat:--force-with-lease,--force/-f(no lease-less force), andThe main/master refspec forms it rejects (
+main,HEAD:main,*:main,main:main, baremain/master) mirror the shell guard's enumeration so thetwo layers cannot disagree. When it matches,
detect_dangerous_commandskipsonly the three force-push pattern descriptions; the independent main/master
push backstop and every other pattern still fire.
The lease is the safety belt —
--force-with-leaserefuses if the remote movedsince the last fetch, so even on a shared worktree branch the second pusher's
lease bounces instead of clobbering. A bare force has no lease, so it is not
carved out and keeps prompting.
Verification (real invocations, not just code)
git push --force-with-lease origin wt/t_89fe8606prompted=False, approved=True— runs headless, no promptgit push --force origin wt/t_89fe8606prompted=True, approved=False— still promptsgit push --force-with-lease origin mainprompted=True, approved=False— still prompts/blocks! [rejected] feature -> feature (stale info), remote not clobbered(a)–(c) driven through the real
check_all_command_guardsin a simulatedgateway approval context. A differential probe (base vs patched
detect_dangerous_command) confirms the carve-out un-gates only the fiveforce-with-lease-to-feature-branch shapes; all other verdicts are unchanged.
Tests: updated
test_git_push_force_with_lease_*(the old test asserted thefeature-branch lease push IS flagged — now asserts it is NOT) and added
test_git_push_force_with_lease_to_main_still_flagged+test_git_push_bare_force_still_flagged_even_on_feature_branch.pytest tests/tools/test_approval.py→ 262 passed, ruff clean.Residual risk
gated by the lease requirement (bare force is never carved out) + the
main/master patterns, not by a working-tree HEAD check — matches the
"auto-approve REQUIRES the lease form" rule.
(e.g.
live-config) is auto-approved, same as the shell guard (which onlyspecial-cases main/master). To gate more branches, extend
_DEFAULT_BRANCH_PUSH_REand the shell guard together.Patch note:
~/.hermes/plans/hermes-patches/force-push-lease-feature-branch-carveout.md