merge-guard: cross-leg leg-isolation + API-merge mint (gh-api-only) + target-aware retirement (#1087, #1086, #1096, #1097)#1095
Merged
Conversation
…e-arm cross-leg laundering Pull the two close danger arms out of DANGEROUS_PATTERNS into _CLOSE_LITERAL_ARMS, matched per-leg at the read floor (mirroring _FORCE_PUSH_LITERAL_ARMS). The ambiguous multi-close is now is_dangerous=False per-leg, so the mint write-gate refuses -> no token -> the approval-source laundering channel is structurally dead; a real same-leg close+--delete-branch still gates. Rewrite the collateral TestLegBoundedMintWindow close-member pins, invert the ambiguous approval canary, add the approval-source + end-to-end seam laundering-closed proofs, and update the retention/tripwire docstrings.
…oss-leg over-blocks Pull the 17 API danger arms (merge / git-refs / branch-protection / contents / implicit-POST across gh api, curl, wget) out of DANGEROUS_PATTERNS into _API_LITERAL_ARMS, matched per-leg at the read floor (mirroring the close/force-push conversions). A method or body-flag token in a benign continuation leg no longer over-blocks the compound; a same-leg dangerous API call still gates. No-laundering safety is structural (an isolated API leg is method-less hence detect-negative), pinned by generalizing the detect-negative tripwire across every API family. detect and the danger floor are untouched (read-floor-only).
…e-probe for #1087/#1086 Add TestCloseLiteralArmCrossLegSweep (16 cases) + TestApiLiteralArmCrossLegSweep (24 cases) permanent sweep classes modeled on the force-push cross-leg pair — over-block-removed rows (D=False) + same-leg-still-gates rows (D=True). Each cured row carries a two-stage non-vacuity: identity-slice _slice_stripped_legs flips D False->True, then per-family arm-neuter returns it to False, proving single-family coupling (stronger than the single-representative flip). Append the fix-level pre-merge live-probe runbook row: 14/14 both-sides (pre=60f14796 vs post=3e2da931) through the real hook mains — the #1087 close-arm laundering channel is dead end-to-end (mint 1->0, escalated single ALLOW->DENY); #1086's six cross-leg over-blocks are removed (DENY->ALLOW); every same-leg dangerous form and the faithful round-trips are preserved. Full suite 10476 passed / 0 errors.
There was a problem hiding this comment.
Pull request overview
This PR completes the merge-guard “leg isolation” migration for close (gh pr close --delete-branch) and API-based dangerous operations by moving the relevant regex “danger arms” out of whole-command matching and into per-leg matching over the shared _slice_stripped_legs substrate. This both (1) removes cross-leg over-blocks for API and close compounds and (2) closes the remaining #1087 close-arm approval-laundering channel by making the ambiguous multi-close form not-dangerous (so it cannot mint a token).
Changes:
- Introduces
_CLOSE_LITERAL_ARMSand_API_LITERAL_ARMSand matches them per-leg insideis_dangerous_commandinstead of via whole-commandDANGEROUS_PATTERNS. - Adds/updates permanent sweep tests to prove (a) cross-leg over-blocks are removed and (b) same-leg dangerous forms still gate (no new under-block).
- Adds real seam integration coverage and updates runbook evidence for the fix-level live-probe.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pact-plugin/hooks/shared/merge_guard_common.py | Moves close/API danger matching to per-leg literal arms and integrates them into is_dangerous_command. |
| pact-plugin/tests/test_merge_guard_op_recognition_completeness.py | Adds permanent bidirectional cross-leg sweep classes for close/API per-leg conversions, including non-vacuity/faithfulness mutations. |
| pact-plugin/tests/test_merge_guard_auth_symmetry.py | Updates laundering/over-block symmetry canaries for close now that ambiguous forms mint nothing; adds non-vacuity mutation for whole-command restoration. |
| pact-plugin/tests/test_merge_guard_over_block_batch.py | Updates re-cert narrative and adds API “bare form detect-negative” tripwire coverage. |
| pact-plugin/tests/test_merge_guard_seam_integration.py | Adds end-to-end post→pre seam test proving ambiguous multi-close no longer mints and escalated single is denied. |
| pact-plugin/tests/runbooks/RUNBOOK_RUN_DATES.md | Records fix-level live-probe results for #1087/#1086. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1022
to
+1024
| q = "Close it?" | ||
| opts = [_opt("Yes, close", f"On approval run: `{member}`"), _opt("Cancel", "Abort")] | ||
| assert _invoke_post([_q(q, opts)], {q: "Yes, close"}, tmp_path) == 0 | ||
| assert _token_bound_flags(tmp_path) == [] | ||
| _invoke_post([_q(q, opts)], {q: "Yes, close"}, tmp_path) |
Comment on lines
+1107
to
+1110
| q = "Close it?" | ||
| opts = [_opt("Yes, close", f"On approval run: `{ambiguous}`"), _opt("Cancel", "Abort")] | ||
| _invoke_post([_q(q, opts)], {q: "Yes, close"}, tmp_path) | ||
| assert len(_minted_tokens(tmp_path)) == 0 |
Comment on lines
+2385
to
+2404
| for _leg in _slice_stripped_legs(stripped): | ||
| if any(arm.search(_leg) for arm in _FORCE_PUSH_LITERAL_ARMS): | ||
| return True | ||
| # Literal close arms, matched PER-LEG (#1087): same leg substrate and strip | ||
| # provenance as the force-push loop above. An arm fires iff `gh pr close` and | ||
| # `--delete-branch` co-occur within ONE leg — so the ambiguous cross-leg | ||
| # multi-close is is_dangerous=False here, the mint write-gate refuses (no token), | ||
| # and the escalated-single laundering channel is structurally closed. | ||
| for _leg in _slice_stripped_legs(stripped): | ||
| if any(arm.search(_leg) for arm in _CLOSE_LITERAL_ARMS): | ||
| return True | ||
| # Literal API danger arms, matched PER-LEG (#1086): same leg substrate and strip | ||
| # provenance as the loops above. An arm fires iff the API client, its mutating | ||
| # method (or implicit-POST body flag), and the target endpoint co-occur within ONE | ||
| # leg — so a method/body-flag token in a benign continuation leg no longer | ||
| # over-blocks the compound, while a same-leg dangerous API call still gates. The | ||
| # negative-lookahead arms operate within-leg (correct exclusion/inclusion direction). | ||
| for _leg in _slice_stripped_legs(stripped): | ||
| if any(arm.search(_leg) for arm in _API_LITERAL_ARMS): | ||
| return True |
This was referenced Jul 3, 2026
…ative tripwire docstring, legs hoist C1: the two mint-nothing close canaries now assert the post-hook exit==0 alongside mint==0, so a post-hook failure can't false-pass as "minted nothing" (strengthens the #1087 laundering-closure proof). M2: soften the OPEN-Q D detect-negative tripwire docstring from universal to enumerative, and fix a stale test-name reference. legs-hoist: compute _slice_stripped_legs(stripped) once and share it across the force-push/close/API per-leg loops (behavior-inert hot-path micro-op).
… runbook (keep both probe sections chronologically: #1090 post-install, then cross-leg fix-level)
| ) | ||
|
|
||
|
|
||
| class TestAmbiguousApprovalByteIdenticalIsSafeDirection: |
Comment on lines
+510
to
+514
| # curl / wget bare-GET equivalents for git/refs and protection | ||
| "curl https://api.github.com/repos/o/r/git/refs/heads/x", | ||
| "curl https://api.github.com/repos/o/r/branches/main/protection", | ||
| "wget https://api.github.com/repos/o/r/git/refs/heads/x", | ||
| ], |
Add an additive per-leg detect arm that recognizes an API-based pull-request merge (gh-api / curl / wget with a mutating method on the merge endpoint) plus a path-based PR-number extractor wired into the mint context, reusing the existing pr_number key. The tolerant client matcher matches the sibling API detect arms, so the global-flag spelling also mints — curing the gated-but-unmintable over-block (a faithful API-merge click can now mint and authorize). Read floor untouched; recognition is mint-side only and additive per-leg (benign compounds stay unclassified).
Narrow Layer-1 token retirement to op-type + target + session: a target-carrying token now retires only on a target match, so an unrelated same-operation command no longer retires an operator's approved token — curing the over-block on a faithful re-execution. A target-less token keeps the prior op+session fallback (defensive, not strict) so genuine target-less self-consumes still retire, adding no under-block.
…ement (#1096, #1097) Add TestApiMergeMintParity (34 cases) and TestTargetAwareRetirement (8 cases), 42 cases total, covering the full bidirectional certification of the two fold-in over-block cures: over-block-cured across all API-merge spellings including the global-flag round-trip, additive-purity corpus plus per-leg canaries, the no-new-under-block sweep (cross-spelling and privileged-flag refuse), the target-aware retirement cure with positive-control-gated survival rows, and target-completeness (a single mint chokepoint fed only by target-complete pairs).
Comment on lines
+684
to
+691
| def _extract_api_merge_pr(command: str) -> str | None: | ||
| """Parse the PR number from an API PR-merge command's `pulls/<N>/merge` path | ||
| (#1096). detect_command_operation_type classifies gh api|curl|wget PUT/PATCH/POST | ||
| on a pulls/<N>/merge endpoint as a `merge`; the PR number is the path component. | ||
| Returns the digits, or None when the command is not a recognized API-merge form. | ||
| Same recognition<->extractability contract as _extract_api_ref.""" | ||
| m = re.search(r"pulls/(\d+)/merge\b", command) | ||
| return m.group(1) if m else None |
Comment on lines
+518
to
+522
| # API-based PR merge (#1096): gh api|curl|wget with a mutating method (PUT/PATCH/ | ||
| # POST) on a pulls/<N>/merge endpoint is a merge via API. Recognized PER-LEG (the | ||
| # #1086 leg-isolation philosophy) so a method token in a benign continuation leg | ||
| # (`gh api .../pulls/5/merge && echo -X PUT`) does NOT misclassify the benign | ||
| # compound — additive purity is exact (only a genuine same-leg API-merge classifies; |
…et-confusion laundering (#1096) Bind the API-merge token to the endpoint-position PR (the URL argument the command actually merges), not the first pulls/<N>/merge substring, closing a target-confusion laundering channel where a decoy PR in a flag body or an earlier leg could bind a token that authorizes an un-approved merge. - Factor a shared per-leg helper called by both the detect arm and the extractor, so recognition and extraction read the same surface (recognize a leg iff its endpoint PR is extractable — no gated-but-unmintable, over-block free by construction; a tokenizer-failure fallback never returns None). - Endpoint-positional walk over the shlex view, skipping per-client value-taking-flag values (per-client sets: gh api vs curl/wget differ in flag arity), binding the URL positional's PR. - Extend the HTTP-body strip to unquoted body/field values (also closes an unquoted-body read-floor over-block, #1037). - Align the contents-API strip exception to case-insensitive, matching its case-insensitive read arm, so a capital-case Contents/ write-to-main keeps its gating body (fixes a latent under-block the unquoted-body strip exposed). Recognition regex byte-identical; detect close-unconditional and the first-leg flag-condition invariants untouched.
…esidual boundary (#1096) Add 36 cases across 4 classes: laundering-closed for all confirmed decoy shapes (compound/echo, unquoted and quoted -f body, header, global-flag, two-path, curl -d) — each binds the endpoint PR and denies the standalone decoy merge; no-over-block for every faithful spelling and the body-coincidence merge; additive-purity byte-identical; the D4 residual-boundary tripwire (a no-widening pin over enumerated per-client value-flags and a no-over-block never-None pin, with no positive decoy-authorizes assertion); the carrier-4/carrier-8 interaction and an independent capital-case contents write-to-main gating row; and the unquoted-body over-block cure.
| "--data-ascii", "--data-urlencode", "-F", "--form", "-u", "--user", | ||
| "-b", "--cookie", "-e", "--referer", "-A", "--user-agent", "-o", | ||
| "-T", "--upload-file", "-x", "--proxy", "-E", "--cert", | ||
| "--method", "--body-data", "--post-data", |
Comment on lines
+518
to
+522
| # API-based PR merge (#1096): gh api|curl|wget with a mutating method (PUT/PATCH/ | ||
| # POST) on a pulls/<N>/merge endpoint is a merge via API. Recognized AND extracted by | ||
| # the ONE shared per-leg helper _api_merge_leg_endpoint, so recognition<->extractability | ||
| # read the SAME surface — a leg classifies merge IFF its ENDPOINT-position PR is | ||
| # extractable, which gives no gated-but-unmintable AND binds the URL-positional |
…/wget arm (#1096) Restrict the API-merge recognition and mint to the gh api client, whose flag set is finite and documented and whose endpoint is the first positional — a provably sound surface. The curl/wget arm relied on a value-taking-flag denylist over an unbounded, per-release flag vocabulary, which cannot be a security boundary: a decoy pull-request URL hidden in any un-enumerated value flag bound the wrong PR into the approval token (target-confusion laundering). Dropping the arm makes curl/wget API-merge classify as no operation, so it mints nothing and reverts to its pre-existing gated-but-unmintable state (the read floor still gates it) — eliminating the laundering class by construction rather than chasing the flag vocabulary. The gh api recognition, endpoint-positional walk, and never-None fallback are unchanged; target-aware retirement, the unquoted-body and contents-exception fixes are untouched. Extending API-merge minting to curl/wget with a real argv parser is deferred to a follow-up.
…anaries (#1087) The mint-count non-vacuity canaries invoked the post-hook without asserting its exit code, so a post-hook failure could false-pass a mint==0 assertion (no token minted is also true when the hook never ran). Assert exit 0 above each of the four mint-count sites, matching the existing idiom, so the laundering-closure proof cannot pass vacuously. Test-only; strengthens the non-vacuity direction.
michael-wojcik
changed the title
This was referenced Jul 3, 2026
This was referenced Jul 3, 2026
Merged
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.
Summary
Completes the cross-leg leg-isolation started in PR #1085, then folds in two adjacent over-block cures and — after multi-lens review surfaced a security defect — lands the API-merge cure in its provably-sound form. The branch grew through review; this description reflects the full, current scope.
Governing principle (the merge-guard is an honest-mistake control): a faithful single-command click always mints and merges. Over-block = cardinal sin (PRIMARY, inviolable); under-block / laundering = must-fix, tolerated-with-defense-in-depth.
What landed
Original scope (#1087 + #1086):
_CLOSE_LITERAL_ARMS._API_LITERAL_ARMS.Folded in during review:
gh apiPUT/PATCH/POST to a pull-request merge endpoint now mints an approval token (previously gated-but-unmintable — a permanent over-block). Recognition and endpoint extraction share one per-leg helper, binding the URL-positional PR (not a decoy in a flag body or another leg).gh issue commentbody quoting a dangerous command is rejected (substring-scan brittleness) #1037 (bonus) — the unquoted-body read-floor over-block is cured as a side effect of the body-strip extension.Why #1096 is gh-api-only (the review story)
The first #1096 implementation recognized
gh api,curl, andwgetmerge spellings, extracting the endpoint PR by skipping a denylist of value-taking flags. Multi-lens review (GitHub Copilot + adversarial security review) found this denylist is structurally unsound for curl/wget: their flag space is unbounded (~200+ / ~130+ flags, growing per release), so any un-enumerated value-flag can carry a decoy pull-request URL that binds the wrong PR into the approval token — a target-confusion laundering class (approve merge-of-X, authorize merge-of-Y). A completeness audit confirmed 52+ leaking flags including the most common (-X,--request,-O).gh apihas a finite, documented flag set + a first-positional-endpoint convention, making its extraction provably sound. So the API-merge mint is restricted to gh-api; curl/wget merge reverts to its pre-existing gated-but-unmintable state (the read floor still gates it — not a new hole, verified base-identical). The whole laundering class dies by construction (curl/wget cannot classify a merge → cannot mint → cannot launder) rather than by chasing the flag vocabulary. Extending the API-merge cure to curl/wget with a real argv parser is a tracked follow-up.Invariants
The two hard invariants — the
detect_command_operation_typeclose-unconditional arm and the_flag_condition_danger_opfirst-leg anchoring (the #1078 cure) — are untouched throughout. Note: #1096 additively extendeddetectwith a gh-api merge-recognition arm (a new classification for a previously-Noneinput); every pre-existing classification is byte-identical (additive-removal purity certified via corpus-diff + source-revert).Verification
Scope tracked separately
echo <danger>-mints cross-op class; the op-target-vs-flags retirement granularity nit.Version + issue closure
PATCH bump to 4.4.53 (canonical 4 files). Completes the acceptance criteria of #1087, #1086, #1096, #1097; closes #1037 (unquoted-body over-block). Closure pending the post-install live-probe re-run against the installed version (per the #1063/#1085 gated-closure discipline). Not using auto-close keywords for that reason.