merge-guard: is_dangerous_command false-positive — `gh issue comment` body quoting a dangerous command is rejected (substring-scan brittleness)

[michael-wojcik]

⚠️ STATUS UPDATE (2026-06-28) — general fix deferred; over-block accepted as safe

Resolving this over-block via a general benign-arg-literal suppressor proved intractable for this SACROSANCT control. A HYBRID (regex-based, per-segment, verb-class-aware) suppressor was built and passed an 80-probe adversarial security re-probe + a GREEN auditor + 44 tests + a fully green suite — yet two independent clean-room blind rounds each found NEW critical UNDER-blocks it introduced (escaped-quote grammar; process-substitution <(, then $(/backtick; single &).

Root cause: a regex cannot model bash grammar. In bash an "argument" can still execute (via $(), backtick, <(…), ${ …; }, &, eval, bash -c), so "the dangerous literal is only a quoted arg" is not a safe conclusion without a real parser — every regex patch closed the found case and missed adjacent siblings (whack-a-mole). The over-block is safe (over-block, never under-block); the under-block risk of a general fix is not worth it.

Disposition:

1. Partial safe fix (separate PR): extend the proven, anchored gh carrier-strip to gh issue/pr comment (their --body is a non-executing API value, like create/edit). Fixes the gh-comment FP only.
2. The general over-block (grep / git commit -m / pact_memory search quoting a dangerous literal) is an accepted safe limitation — workaround: rephrase to avoid the literal.
3. A sound general fix needs a real bash parser (e.g. bashlex) with fail-closed-on-parse-failure — a larger, lower-priority project, deferred. This issue tracks it.

Sibling pre-existing UNDER-blocks surfaced by the blind review (real security gaps, distinct from this over-block): #1053, #1055, #1056.

---

(original report below)

Problem

merge_guard_pre.py's dangerous-command detection (is_dangerous_command / DANGEROUS_PATTERNS) scans the command string for dangerous-op literals (git push --force, git branch -D, gh pr close --delete-branch, etc.). To avoid false-positives when such a literal appears only as non-executing quoted prose, there is a _strip_non_executable_content pass that strips quoted carrier arguments before the scan.

That carrier-strip covers the --title/--body (-t/-b) values of gh issue create, gh issue edit, and gh pr create — but not gh issue comment (nor gh pr comment). So a benign comment whose body merely quotes a dangerous command (e.g. as documentation/example text) is itself flagged dangerous and rejected.

This is a distinct root cause from the #1031/#1032 mint-vs-read auth-token asymmetry — it is a command-line substring scan, not the token mint/validate path — and was kept explicitly out of scope of the merge-guard-auth-symmetry PR (the #1031/#1032 fix) to avoid widening an already security-sensitive change. Surfaced in #1031 (third comment, the sibling-asymmetry audit).

Mechanism (verified against current main)

• The carrier-strip verb alternation is issue create|edit, pr create (see _strip_non_executable_content step 7, merge_guard_pre.py ~L528-572; the verb-alternation comment is at ~L572: "Verb alternation: issue create|edit, pr create. NOT pr close").
• gh issue comment is a non-executing GitHub-API call (the --body value is sent to the API, never run by a shell) — exactly the same exemption rationale as create/edit — but it is absent from the alternation, so its --body value is not stripped and reaches DANGEROUS_PATTERNS.

Reproduction (illustrative)

gh issue comment 1031 --body "Before the fix, the guard wrongly rejected git push --force origin main"

The quoted git push --force survives the strip pass → DANGEROUS_PATTERNS matches → the harmless comment is blocked, requiring an awkward rephrase to post it.

Relation to #1031 / #1032

Same prose-vs-structure brittleness class as the merge-guard auth-token fix (which moved the mint/read boundary from loose prose onto the actual command structure), but on a different surface — dangerous-command detection rather than token minting. Filed separately so the two don't entangle.

Proposed fixes (for discussion)

• (a) Narrow — add gh issue comment (and likely gh pr comment) to the carrier-strip verb alternation, mirroring the existing create|edit handling (same quote-aware span + the command-substitution / pipe-to-shell guards). Cheap, low-risk, follows the established pattern — but it is another entry in a list that will keep growing as new API-carrier subcommands appear.
• (b) Principled — classify the operation actually being invoked (the leading gh/git verb) and gate on that, rather than substring-scanning the whole (stripped) line for dangerous-op literals. The robust direction the merge_guard_post mints token for the WRONG PR when approval text contains an unrelated #NNNN (false 'token does not match' rejections) #1031 comment advocated; a larger change to the detection model. Must preserve the INV-D2 over-block-not-under-block property (an unquoted executing op in a compound tail must still be caught).

Scope

A real, still-live false-positive in merge_guard_pre.py, fully within PACT's control. Independent of #1031/#1032; safe to address on its own timeline. PATCH-class (internal hook robustness, no user-visible capability change) when fixed.

[michael-wojcik]

Two live reproductions in a single session (real-world frequency)

While shipping an unrelated merge-guard PR, the is_dangerous_command substring false-positive fired twice on completely ordinary dev operations that merely mention a dangerous command — neither was an actual merge/push:

1. git commit whose commit-message body described merge-guard behavior and quoted the literal gh pr merge N → the commit was over-blocked and demanded merge approval. (Worked around by rephrasing the message to drop the literal.)
2. A pact-memory search whose query string contained gh pr merge → the search command was over-blocked the same way.

Both are fail-safe (over-block, never under-block) and harmless individually, but the frequency point stands: the substring scan fires on routine git commit messages and tooling queries that describe the guard, not just on real destructive commands. This is a recurring papercut for anyone working on the merge-guard itself (commit messages and notes naturally quote the patterns).

Reinforces the carrier-strip / context-aware-tokenization direction in the issue body: the scan should not treat a dangerous-command literal embedded in a benign carrier (a commit message, an issue/PR/search body, a quoted argument) as an executable invocation.