fix(hooks): re-gate the destructive git push forms - #2734
Merged
Conversation
5aadf36 lifted the git push confirmation gate by deleting push_is_safe outright, so push now auto-permits in every form, including the three that can destroy published history: a force push over the default ref, a delete refspec, and --mirror, which deletes remote refs that are absent locally. The stall that motivated the lift came from one form only -- `git push --force-with-lease <remote> <task-branch>`, the rebase-then-push flow every agent worker runs -- and that form fails safe by design, refusing when the remote moved. The removal was therefore broader than the defect it fixed. push_is_destructive restores the destructive subset rather than the original helper. It escalates a force-flavored push (-f, --force, --force-with-lease) whose refspec names main, master, or the resolved default ref; a delete refspec in either the `:branch` or the -d/--delete form; and --mirror/--all. Everything else keeps auto-permitting with the NOTICE 5aadf36 introduced. Two deliberate departures from the original push_is_safe, both narrowing: it escalated on any token naming the default ref and on --force-with-lease anywhere, which is what gated the worker flow; here the default-ref test applies only in combination with a force flag. One departure widens: --delete/-d was not in the original flag list and auto-permitted there, though it performs the same remote-ref deletion as the `:branch` refspec the original did escalate, so it is gated here as one class. --tags is not restored; publishing tags destroys nothing. A force-flavored push carrying no refspec is left auto-permitting. Which ref it rewrites depends on push.default and the checked-out branch, neither of which a PreToolUse hook can observe, and escalating it would re-stall a worker that force-with-leases its own tracked branch. The gate matches the raw command bytes of the tool call, so a push performed inside a script is invisible to it -- the repository's own merge helper fast-forwards main by pushing to it and this arm never fired, because the tool call was the script name. The hook comment and the SKILL.md paragraph now say so rather than claiming protection the gate does not provide. Out of scope here, as is the related raw-byte behavior 5aadf36's PR already reports, where protected words inside quoted strings and heredocs trip other arms. No OpenSpec requirement binds this hook: openspec/specs/ contains no reference to it, and the only mention anywhere under openspec/ is an archived task narrating a `jj git push` invocation. Testing: `nix build .#checks.aarch64-darwin.hook-gate-dangerous-commands`, the one oracle driving this script. Its case table pinned "git push auto-permits in every form"; the case asserting `git push --force origin main` allows is inverted, and eighteen cases are added -- twelve pinning each restored arm and six pinning what must keep auto-permitting, including `--force-with-lease` to a task branch in bare, `=<expect>`, and `git -C` forms, which is the regression guard for the defect this change must not reintroduce. Severity was checked by replaying the push cases against both prior scripts: all twelve escalation cases fail against 5aadf36's script, so they pin the restoration rather than restating it, and eight of the auto-permit cases -- the force-with-lease trio, bare push, push to main, and a branch merely containing "main" -- fail against the pre-5aadf369 push_is_safe, so the regression guard pins a real difference; the --delete pair fails against both. Also built home-manager-crs58, the activationPackage that consumes both edited nix files and runs shellcheck over the script through writeShellApplication, plus package-apm-skills-compose and eval-md-format for the SKILL.md edit. `just check-fast` was deliberately not run: nothing outside this script, its check, and the two documentation surfaces changed behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
5aadf369lifted thegit pushconfirmation gate by deletingpush_is_safeoutright, so push auto-permits in every form — including the three that can destroy published history.The stall that motivated the lift came from one form only,
git push --force-with-lease <remote> <task-branch>, which fails safe by design: it refuses when the remote moved.The removal was broader than the defect it fixed.
What changed
push_is_destructiverestores the destructive subset rather than the original helper:-f/--force/--force-with-leasenamingmain,master, or the resolved default ref:branchand-d/--delete--mirror,--all--force-with-leaseto a task branchmainTwo departures from the original
push_is_safenarrow it: it escalated on any token naming the default ref and on--force-with-leaseanywhere, which is exactly what stalled workers; here the default-ref test applies only in combination with a force flag.One widens:
-d/--deleteauto-permitted in the original despite performing the same remote-ref deletion as the:branchrefspec it did escalate, so both are gated as one class.--tagsis not restored — publishing tags destroys nothing.A force-flavored push carrying no refspec keeps auto-permitting.
Which ref it rewrites depends on
push.defaultand the checked-out branch, neither of which a PreToolUse hook can observe, and escalating it would re-stall a worker that force-with-leases its own tracked branch.nix run/nix shellare untouched; that lift stands.Limitation stated rather than fixed
The gate matches the raw command bytes of the tool call, so a push performed inside a script is invisible to it — the repository's own merge helper fast-forwards
mainby pushing to it and this arm never fired, because the tool call was the script name.The hook comment and the SKILL.md paragraph now say so instead of claiming protection the gate does not provide.
The related raw-byte behavior
5aadf369's PR already reports, where protected words inside quoted strings and heredocs trip other arms, is likewise out of scope here.No OpenSpec requirement binds this hook:
openspec/specs/contains no reference to it, and the only mention underopenspec/is an archived task narrating ajj git pushinvocation.Generated artifact
Driving the
writeShellApplicationoutput built byhome-manager-crs58:Testing
nix build .#checks.aarch64-darwin.hook-gate-dangerous-commandsis the one oracle driving this script.Its case table pinned "git push auto-permits in every form"; the case asserting
git push --force origin mainallows is inverted, and eighteen cases are added — twelve pinning each restored arm, six pinning what must keep auto-permitting, including--force-with-leaseto a task branch in bare,=<expect>, andgit -Cforms, which is the regression guard for the defect this change must not reintroduce.Severity was checked by replaying the push cases against both prior scripts.
All twelve escalation cases fail against
5aadf369's script, so they pin the restoration rather than restating it.Eight of the auto-permit cases — the force-with-lease trio, bare push, push to
main,HEAD:main,git -Cpush, and a branch merely containing "main" — fail against the pre-5aadf369push_is_safe, so the regression guard pins a real difference; the--deletepair fails against both.Also built
home-manager-crs58(the activationPackage consuming both edited nix files, which runs shellcheck over the script throughwriteShellApplication), pluspackage-apm-skills-composeandeval-md-formatfor the SKILL.md edit.just check-fastwas deliberately not run: nothing outside this script, its check, and the two documentation surfaces changed behavior.