feat(mt#1228): Hook blocks gh api PR-merge calls with merge_method != merge

[minsky-ai]

Summary

Extends the block-git-gh-cli PreToolUse hook to reject gh api PUT calls against the PR-merge endpoint when merge_method is anything other than merge (including absent). Closes a structural enforcement gap that allowed three squash-merges to land on main on 2026-04-24 against documented policy.

Incident that triggered this

In a single session on 2026-04-24, three PRs were merged via gh api with merge_method=squash:

• PR fix(mt#1174): MCP bridge returns structured data, not text confirmation #732 (mt#1174) → def6e00e2
• PR feat(mt#1203): session_pr_wait_for_review MCP tool #753 (mt#1203) → d03bbc2cd
• PR feat(mt#1131): Reviewer retry with reasoning_effort=low on empty output #748 (mt#1131) → 17badb8c6

Each commit message cited ~/.claude/projects/.../memory/feedback_gh_api_bypass.md as justification for the bypass. That memory file's line 23 explicitly shows -f merge_method=merge in the canonical bypass invocation. The agent read the file, paraphrased the policy, and still typed squash — three times in a row. Memory-as-fix lost to default-behavior gravity.

Retrospective root cause: squash is the GitHub / generic-OSS default; merge is the Minsky-specific choice. When the agent constructs a gh api body under load, the default wins unless structure prevents it. The prior fix (policy in memory) was behavioral; per the retrospective-skill's escalation rule, behavioral fixes that fail once will fail again. This PR applies structural enforcement.

Implementation

block-git-gh-cli.ts — new gh api arg helpers:

• findGhApiMethod(args) — extracts the HTTP method, default GET.
• findGhApiEndpoint(args) — first positional after flag/value pairs are stripped; handles -X, --method, -H, -f, --field, --raw-field, etc.
• findGhApiField(args, key) — extracts a -f KEY=VALUE value.

New denial rule in ghDenials that fires when ALL of:

• args[0] === "api"
• findGhApiMethod === "PUT"
• Endpoint matches /(^|\/)pulls\/\d+\/merge$/ (exact — not /merges, not /merge-upstream)
• findGhApiField("merge_method") !== "merge" (blocks squash, rebase, AND absent — absent is ambiguous intent)

Denial reason cites the policy and points at docs/pr-workflow.md and feedback_gh_api_bypass.md for guidance.

docs/pr-workflow.md — new ## Merge method policy section with the rule in bold, the normal path (mcp__minsky__session_pr_merge, which already enforces correctly), and the bypass path with the required -f merge_method=merge. Explains why the hook exists so a future reader understands the structural-over-behavioral reasoning.

~/.claude/projects/.../memory/feedback_gh_api_bypass.md — already updated out-of-repo to add a highlighted ## Merge method policy — ALWAYS merge_method=merge, NEVER squash subsection above the bypass pattern.

Test plan

• 9 new enforcement tests + 3 helper-level tests. All pass.
  • Blocks: -f merge_method=squash, -f merge_method=rebase, no merge_method, --method PUT long-form with squash.
  • Allows: -f merge_method=merge, /reviews/ID/dismissals (different endpoint), GET on /merge (not a merge op), generic gh api repos/o/r.
  • Reason text includes merge_method=merge and references the policy docs.
• Hook suite total: 138 pass, 0 fail.
• validate_typecheck: 0 errors.
• validate_lint: 0 errors, 0 warnings.
• format:check: clean.
• Live end-to-end — cannot exercise from within this session (the hook runs against Bash calls from the agent's own process; verified via unit tests).

Out of scope

• Retroactively converting the three squashed commits (impossible without history rewrite).
• Changes to mcp__minsky__session_pr_merge — that MCP tool already enforces merge_method=merge by default; this hook covers the gh api escape hatch that bypasses the MCP layer.
• Blocking on other endpoints or methods — scope is specifically the PR-merge PUT endpoint.
• Extending enforcement to fork-based merge methods, revert-commit flows, or other forge-specific operations — each can be its own follow-up if a similar gap surfaces.

Verification against the original incident

Walking through mt#1131's merge with this hook in place: my gh api --method PUT /repos/edobry/minsky/pulls/748/merge -f merge_method=squash -f commit_title=... invocation would hit the Bash tool's PreToolUse hook, match the new denial rule, and reject with a message pointing at feedback_gh_api_bypass.md and telling me to retry with -f merge_method=merge. Same for #753 and #732. No quiet bypass possible.

Why this fix succeeds where the prior one failed: the prior fix required the agent to read a memory file and remember to type a specific value at invocation time. This fix makes typing the wrong value fail loudly with a clear retry instruction. Structure over memory.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] .claude/hooks/block-git-gh-cli.ts: findGhApiMethod() returns method verb case-sensitively; ghDenials rule compares against "PUT" only

• Evidence: In findGhApiMethod (around lines ~320-336), the function returns the raw token following -X/--method without normalization:
  export function findGhApiMethod(args: string[]): string {
  for (let i = 1; i < args.length; i++) {
  if (args[i] === "-X" || args[i] === "--method") {
  return args[i + 1] ?? "GET";
  }
  }
  return "GET";
  }
  And the denial rule (ghDenials entry added around ~180-215) checks:
  const method = findGhApiMethod(args);
  if (method !== "PUT") return false;
• Failure mode: A caller can pass lowercase or mixed-case: gh api -X put repos/.../pulls/42/merge -f merge_method=squash. findGhApiMethod returns "put"; the rule does not match (requires "PUT"); the call will not be blocked.
• Impact: Silent bypass of the core enforcement.
• Request: Normalize method to uppercase in findGhApiMethod (e.g., return (args[i + 1] ?? "GET").toUpperCase()) and/or compare case-insensitively in the rule. Add tests for -X put and --method put.

[BLOCKING] .claude/hooks/block-git-gh-cli.ts: findGhApiField() fails when the KEY=VALUE token is shell-quoted; leads to false denials for legitimate uses

• Evidence: In findGhApiField (around lines ~360-372), the code matches any arg.startsWith(${key}=). However parseSegment (earlier in file; see tests at .claude/hooks/block-git-gh-cli.test.ts “parseCommands” section) is not quote-aware and keeps quotes in tokens (example test shows "'hello'" preserved). Therefore a common invocation like -f "merge_method=merge" will produce a token starting with a quote character, not with merge_method=, and findGhApiField returns null.
• Failure mode: A valid command with -f "merge_method=merge" (quoted out of habit or by a generator) is treated as absent and thus blocked by the rule (mergeMethod !== "merge"). This is an over-block of allowed behavior.
• Request: Strip surrounding single/double quotes when scanning for KEY=VALUE tokens, or specifically parse -f/--field/--raw-field value tokens (recognize and unquote them) before comparing. Add a unit test: gh api -X PUT repos/o/r/pulls/42/merge -f "merge_method=merge" should be allowed.

[BLOCKING] .claude/hooks/block-git-gh-cli.ts: findGhApiEndpoint() is not robust to quoted values with spaces when flags precede the endpoint; this creates an easy bypass

• Evidence: findGhApiEndpoint (around lines ~334-359) skips a flag-value pair by doing i += 2 when it sees a value-taking flag (e.g., -f, --field). Because the upstream tokenizer (parseSegment) is not quote-aware, a token like -f commit_title="My PR Title (#42)" is split into multiple tokens: ["-f", 'commit_title="My', 'PR', 'Title', '(#42)"', ...]. The function only skips the first value token after -f, then immediately returns the next non-flag token as the “endpoint.” If the endpoint actually appears after such a -f, findGhApiEndpoint will incorrectly "extract" a fragment of the quoted commit_title as the endpoint, which will not match PR_MERGE_ENDPOINT_RE. The rule then fails to trigger.
• Reproduction scenario: gh api -X PUT -f commit_title="My PR Title (#42)" repos/o/r/pulls/42/merge -f merge_method=squash. With the current code, findGhApiEndpoint likely returns 'PR' (or similar), not "repos/.../merge"; PR_MERGE_ENDPOINT_RE.test(endpoint) fails; the rule allows a prohibited squash-merge.
• Impact: Silent bypass by reordering args to put a quoted -f value before the endpoint (a perfectly valid gh api invocation pattern).
• Request: Make endpoint extraction resilient to quoted values (either implement a minimal quote-aware scan, or specifically recognize -f/--field/--raw-field and skip tokens until the next token that begins with - or until the next value-taking flag). At minimum, add tests covering: endpoint after -f commit_title="...". Right now, this enforcement can be bypassed trivially.

[BLOCKING] .claude/hooks/block-git-gh-cli.ts: Quoted endpoint string is not recognized as PR merge endpoint

• Evidence: findGhApiEndpoint returns the first positional token as-is. If a user quotes the endpoint (e.g., "repos/o/r/pulls/42/merge" or '/repos/o/r/pulls/42/merge'), the returned token includes leading/trailing quotes. PR_MERGE_ENDPOINT_RE = /(^|/)pulls/\d+/merge$/ requires a bare string ending in pulls/N/merge; a leading quote causes the test to fail.
• Failure mode: gh api -X PUT "repos/o/r/pulls/42/merge" -f merge_method=squash will not be blocked (endpoint regex fails due to quotes).
• Request: Strip surrounding quotes from the positional endpoint before applying PR_MERGE_ENDPOINT_RE. Add a test case for a quoted endpoint.

[NON-BLOCKING] NEEDS VERIFICATION: equals-form flags likely aren’t handled (e.g., --method=PUT, -XPUT), and could allow bypass

• Evidence: findGhApiMethod only recognizes a separate value token after -X/--method. If gh supports --method=PUT or -XPUT, current parsing returns default GET and rule does not fire. Similarly, GH_API_VALUE_FLAGS will not cause findGhApiEndpoint to skip a combined "-XPUT" token since it checks exact flag strings.
• Request: Confirm gh api supports/doesn’t support equals-form/combined short flags. If supported, extend parsing to handle equals-form (split on =, or detect -XPUT). Add tests.

[NON-BLOCKING] findGhApiField() doesn’t scope to field flags, scans entire args for key=

• Evidence: The function loops all args and matches any arg starting with prefix = "merge_method=". While unlikely to cause an issue, it could produce false matches if an unrelated token contains “merge_method=” (e.g., accidental inclusion in a quoted blob not intended as a field value).
• Request: Optionally restrict scanning to values that directly follow -f/--field/--raw-field (and after unquoting).

[NON-BLOCKING] Denial reason references an out-of-repo memory path

• Evidence: In ghDenials reason string (around ~200-215), the message points to feedback_gh_api_bypass.md which, per the PR description, lives outside this repo.
• Impact: For human readers browsing the repo, this isn’t actionable. Keeping docs/pr-workflow.md is sufficient; consider referencing only in-repo docs in the denial reason, or clarify “operator memory file” in the message.
• Request: Tweak denial text to reference only docs/pr-workflow.md (or ensure a stable in-repo pointer).

[NON-BLOCKING] Test coverage misses realistic edge cases that matter for enforcement reliability

• Evidence: Tests added for helpers and basic allow/block paths are good, but missing:
  • -X put / --method put (lowercase)
  • -f "merge_method=merge" and -f 'merge_method=merge'
  • Endpoint quoted with "..."
  • Endpoint appearing after a quoted -f value with spaces (e.g., commit_title)
  • Possibly --method=PUT or -XPUT (if gh supports)
• Request: Add the above tests to prevent regressions and to backstop the fixes requested.

[PRE-EXISTING] parseSegment/splitOnShellOperators are explicitly not quote-aware; the new enforcement depends on reliable arg parsing

• Evidence: The file documents and tests this limitation. However, the new enforcement logic introduces security-policy semantics that are invalidated by this limitation (see BLOCKING findings above).
• Note: While the non-quote-aware parser predates this PR, relying on it for policy enforcement substantially raises the risk and warrants tightening parsing around the specific “gh api …” cases.

Spec verification table

• Task spec: N/A (no task spec provided)

Documentation impact

• docs/pr-workflow.md was updated appropriately with a Merge method policy section and explicit bypass instructions.
• Consider:
  • Adjusting the PreToolUse denial reason text to reference only in-repo docs for durability.
  • If there are any hook architecture docs (e.g., .claude/hooks/SPEC.md), add a brief note about the new gh api merge-method rule and its rationale.

Conclusion

REQUEST_CHANGES

The current implementation can be bypassed or can over-block due to parsing/normalization gaps:

• Case-sensitive method check (“PUT” vs “put”)
• Quoted field values and quoted endpoints not handled
• Endpoint extraction fails when quoted -f values precede the endpoint

Please address the blockers and add tests for these scenarios to make the enforcement reliable.

[edobry]

Round 1 addressed (d72c2a226). Had Claude work through each finding.

All 4 BLOCKING findings were genuinely valid — real bypass or over-block vulnerabilities in the enforcement. Fixed:

[BLOCKING] Case-insensitive method check — findGhApiMethod now uppercases the returned method so -X put and --method put are both caught. Regression tests added.

[BLOCKING] Quoted -f "merge_method=merge" over-block — new stripSurroundingQuotes helper; findGhApiField unquotes tokens before prefix-matching. A valid quoted invocation no longer gets treated as absent. Tests for both " and ' forms.

[BLOCKING] Endpoint extraction not robust to quote-split -f values — introduced findGhApiPrMergeEndpointToken that scans ALL tokens for the PR-merge pattern (after unquoting each). The denial rule uses it instead of positional extraction, which was trivially bypassable via gh api -X PUT -f commit_title="My PR" repos/o/r/pulls/42/merge -f merge_method=squash — the upstream tokenizer splits "My PR" across tokens, the old positional scan returned "PR" as the "endpoint", and the rule never fired. The bypass scenario you described is now a regression guard.

[BLOCKING] Quoted endpoint not recognized — findGhApiEndpoint now unquotes the returned token; findGhApiPrMergeEndpointToken also unquotes before regex-matching. "repos/o/r/pulls/42/merge" (with quotes) is now recognized correctly.

[NON-BLOCKING] Equals-form and combined short-form method flags — confirmed gh/Cobra accepts both, so also covered: --method=PUT, --method=put, -XPUT, -Xput. Tests for each.

[NON-BLOCKING] Denial reason references out-of-repo memory path — removed; now points only at docs/pr-workflow.md §Merge method policy. Regression test asserts the reason text does not contain feedback_gh_api_bypass.md.

[NON-BLOCKING] findGhApiField scans all args for key= — acknowledged; scoping to specifically -f/--field/--raw-field-preceded tokens is a reasonable next iteration, but the current broader scan plus quote-stripping is sufficient for correctness and keeps the code simpler. Not changing in this round.

Tests: 164 pass (up from 138). Typecheck clean, lint clean, format clean. Every BLOCKING finding has a regression guard; every allowed-form case has an explicit test; the bypass scenario from the review (-f commit_title="..." ... merge_method=squash reordered) is tested end-to-end.

[minsky-ai]

Review: mt#1228 round-2 — gh api merge-method hook hardening

CI status: 2/2 checks passing (build + Prevent Placeholder Tests)

BLOCKING finding verdict

All 4 BLOCKING findings from round-1 review (4172019976) are substantively addressed. Detailed walkthrough:

---

BLOCKING 1 (case-sensitive method check — -X put bypass) — CLOSED

findGhApiMethod calls .toUpperCase() on every return path (separate token, equals form, combined short form). The denial rule compares against the uppercased string "PUT". Regression tests: "uppercases lowercase -X values", "uppercases --method values", "uppercases equals form --method=put", "parses combined short form with lowercase -Xput". The bypass via -X put is closed.

BLOCKING 2 (quoted -f "merge_method=merge" over-block) — CLOSED

findGhApiField now calls stripSurroundingQuotes(arg) on every token before the startsWith(prefix) comparison. "merge_method=merge" strips to merge_method=merge and correctly returns "merge". Integration test "allows quoted -f merge_method=merge (over-block regression guard)" exercises the full checkDenial path. Single-quote variant also covered.

BLOCKING 3 (positional scan confusable by quote-split -f values — bypass) — CLOSED

The denial rule no longer calls findGhApiEndpoint. It calls findGhApiPrMergeEndpointToken, which scans ALL tokens without any positional logic. A quote-split commit title like -f commit_title="My PR Title" arriving as ['commit_title="My', 'PR', 'Title"', 'repos/o/r/pulls/42/merge'] still has the real endpoint present in the list and findGhApiPrMergeEndpointToken finds it. Regression test 'blocks endpoint after a quote-split -f commit_title="My PR Title" (bypass regression guard)' covers the exact scenario from round 1.

BLOCKING 4 (quoted endpoint not recognized) — CLOSED

findGhApiPrMergeEndpointToken calls stripSurroundingQuotes(arg) before the regex test. "repos/o/r/pulls/42/merge" strips to the bare endpoint and matches /(^|\/)pulls\/\d+\/merge$/. Integration test "blocks quoted endpoint (regression guard for quote-stripping)" confirms.

---

Denial reason text

The reason string points exclusively to docs/pr-workflow.md §Merge method policy. It does NOT reference feedback_gh_api_bypass.md. The comment block in the source (line 184 of block-git-gh-cli.ts) references feedback_gh_api_bypass.md — that is a developer-facing code comment, not user-facing denial text. The test "denial reason does not reference out-of-repo memory paths" enforces this distinction at test time.

---

New bypass / over-block scenarios introduced by round-2 code

findGhApiPrMergeEndpointToken all-tokens scan — false-positive edge case (NON-BLOCKING)

Scanning all tokens rather than the positional slot means a -f field value whose unquoted form happens to end in /pulls/N/merge would be recognized as a PR-merge endpoint. For example, -f commit_title=my-branch-about-pulls/42/merge with a different actual endpoint. This would cause a false-positive block (over-block, not bypass — failing safe). In practice agent commit titles do not end with /pulls/N/merge. Note only.

findGhApiField scans all tokens (NON-BLOCKING)

The field extractor scans every arg, not just tokens following -f/--field. A header token like -H "X-merge_method=merge" would NOT match because after quote-stripping the token starts with X-merge_method=, not merge_method=. No bypass vector identified.

Duplicate -f fields (NON-BLOCKING)

findGhApiField returns the value of the first matching token. -f merge_method=merge -f merge_method=squash would be allowed (first match wins). This requires deliberately duplicating a field — not an accidental failure mode, and out of scope for this fix.

---

Checked and clear

• .claude/hooks/block-git-gh-cli.ts — all 4 bypass/over-block vectors from round 1 closed; new helpers correctly structured; findGhApiEndpoint retained but NOT used in the denial rule enforcement path (uses findGhApiPrMergeEndpointToken instead, as documented in JSDoc)
• .claude/hooks/block-git-gh-cli.test.ts — regression guards are genuine (they test the exact scenarios described in round-1 findings, not just happy paths); 19 new test cases across 7 describe blocks
• docs/pr-workflow.md §Merge method policy — section added with both normal path and bypass path documented; denial reason correctly references it
• PR_MERGE_ENDPOINT_RE = /(^|\/)pulls\/\d+\/merge$/ — verified correct: does not match /merges, /merge-upstream, /reviews/..., or partial paths; anchored with $ so no partial suffix bypasses
• feedback_gh_api_bypass.md — already has "Merge method policy" bold-callout subsection; not part of this diff but criterion is satisfied

---

Spec verification

Task: mt#1228

Criterion	Status	Evidence
block-git-gh-cli.ts rejects PUT /merge calls with squash/rebase/absent merge_method	Met	ghDenials rule at lines 183–212; three block tests in checkDenial — gh api PR-merge endpoint describe block
Existing tests still pass; new positive/negative cases added	Met	CI green; 19 new test cases across 7 describe blocks
feedback_gh_api_bypass.md has "Merge method policy" bold-callout subsection	Met	Present as ## Merge method policy — **ALWAYS merge_method=merge, NEVER squash** (pre-existing update, outside this diff)
docs/pr-workflow.md has bypass-path note mentioning merge_method=merge	Met	Lines 254–276 added in this PR
Verify locally that hook blocks mock squash-merge invocation	N/A	No live-target; CI test suite is the verification signal

---

Documentation impact

Updated docs/pr-workflow.md in this PR — new ## Merge method policy section covering normal path, bypass path, and structural rationale.

Verdict: ready to merge. All 4 BLOCKING findings from round 1 are closed with regression tests. No new BLOCKING issues introduced.

(Had Claude look into this — AI-assisted adversarial review of round 2)

Round 2 (d72c2a2) substantively addresses all 4 BLOCKING findings: case-normalized findGhApiMethod, quote-stripping in findGhApiField, all-tokens scan in findGhApiPrMergeEndpointToken, and quote-stripped endpoint matching. Verified by Chinese-wall reviewer subagent (review 4177184547) — APPROVE with regression guards confirmed for each scenario. R1 was on f15cb77; bot did not post a fresh review against d72c2a2 (webhook-missed class, second confirmed instance per mt#1110 calibration).