tools/hooks/mergeguard: two merge shapes reach the tool unblocked — `merge` before `gh`, and `gh api` wrapped in `eval`
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 9
- Forks
- 0
- Avg merge
- 3h 3m
- Merged PRs (30d)
- 509
Description
mergeUsesShellExpansion's text heuristics have two order- and vocabulary-dependent gaps that let a merge-capable command through. Both predate #1972's false-positive fix and neither is caused or widened by it; they were found while establishing that fix's baseline.
The gaps
1. The word merge before the gh token. ghExpansionMergeText is (?s)\bgh\b.*\bmerge\b, which requires gh to appear first. A command that assigns the word earlier is not matched:
X=merge; gh api -X PUT repos/o/r/pulls/1/$X
ghDynamicPRMergeText and dynamicGHExecutable do not cover it either: both want pr-ish or p… m… shapes that gh api -X PUT does not have. Verified allowed on 612983c.
2. gh api inside eval or bash -c. nestedEvaluation gates on ghMergeText, which is (?s)\bgh\b.*\bpr\b.*\bmerge\b — it requires the literal word pr. gh api never carries it, so the nested-evaluation path never fires for the REST spelling of a merge:
eval 'gh api -X PUT repos/o/r/pulls/1/merge'
bash -c 'gh api -X PUT repos/o/r/pulls/1/merge'
Both verified allowed on 612983c. The quoting also collapses the inner command into a single word, so the tokenizer sees no gh invocation at all.
The common cause is that every predicate here was written against gh pr merge, the CLI spelling. gh api -X PUT .../merge is the same operation over REST and satisfies none of the pr-shaped patterns.
Evidence
Computed by evaluating the checked-in predicates directly, and confirmed against the built hook:
| command | expMrg |
dynPR |
dynGH |
nested |
result on 612983c |
|---|---|---|---|---|---|
X=merge; gh api -X PUT .../$X |
false | false | false | false | ALLOWED |
eval 'gh api -X PUT .../merge' |
true | false | false | false | ALLOWED |
bash -c 'gh api -X PUT .../merge' |
true | false | false | false | ALLOWED |
gh api -X PUT .../merge --jq .$f |
true | false | false | false | denied |
gh pr merge 1 -R o/r --auto $X |
true | true | false | false | denied |
The third column pair is what shows the cause: the two allowed rows fail for different reasons — the first because of ordering, the next two because nested needs pr.
Why this is bounded, and still worth closing
This hook is one control among several, and it is not the merge gate. A merge still needs shipcheck to pass on an unchanged head with an exact-head independent review, and the pinned-head precondition (--match-head-commit, or expectedHeadSha through the GitHub tool) is enforced separately by mergeHeadPinned. Neither gap grants an approval or hides a review. What they do is let a merge-capable command run without the hook's last-local-action check, which is precisely the control .agents/ship.md describes as "independent of the model's summary".
Acceptance
Close both without reintroducing #1972's false positive:
- Make the merge-text predicates order-independent, or match on the merge capability rather than on word order: a
gh apiinvocation carrying a method-changing flag (-X,--method, or a body flag) and a URL that ends in/mergeis a merge regardless of where the word sits.provableGHAPIReadinmain.goalready classifies the read side of exactly this and can be inverted for the write side. - Drop the
prrequirement from the nested-evaluation gate, or tokenize one level intoevalandbash -cstrings so the inner command is analyzed as a command rather than as an opaque word. - Keep #1972's regression test passing, and flip the three cases currently pinned as known gaps in
TestReadOnlyGHAPIIsNotAMergeinto the denied set as each is closed — they are pinned there with a pointer to this issue so the expectation has an obvious home.
A regression test per shape, in both directions, and no change to the existing cases in TestMergeShellExpansionIsRejected.
Correction to #1972
#1972's "Why the obvious narrowing is wrong" section argued against segment-scoping ghExpansionMergeText on the grounds that X=merge; gh api -X PUT repos/o/r/pulls/1/$X would stop matching. That reasoning was wrong: as gap 1 above shows, the command does not match today either. The conclusion still held for a different reason — the exemption that shipped is about provable read capability rather than about segment distance — but the stated justification was not accurate, and #1972 has been corrected.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in main.go with the merge-text predicates, nestedEvaluation, and provableGHAPIRead. Run TestReadOnlyGHAPIIsNotAMerge and TestMergeShellExpansionIsRejected, then add regression coverage for both command shapes in both directions. Done means the three known gaps are denied without changing the existing regression cases or reintroducing #1972's false positive.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, shell
- Domain
- security, tooling
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100