Skip to content

fix(#47): catch gh api .../merge bypass in merge-gate hooks#54

Merged
atlas-apex merged 2 commits into
mainfrom
fix/#47-merge-gate-catch-gh-api
Apr 14, 2026
Merged

fix(#47): catch gh api .../merge bypass in merge-gate hooks#54
atlas-apex merged 2 commits into
mainfrom
fix/#47-merge-gate-catch-gh-api

Conversation

@atlas-apex

@atlas-apex atlas-apex commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes the silent merge-gate bypass surfaced in #47. The three merge-gate hooks were only wired on Bash(gh pr merge *), so merges performed via the raw REST-API shape gh api repos/<owner>/<repo>/pulls/<N>/merge -X PUT skipped all three gates. Incident: me2resh/curios-dog#190 was merged via gh api while CI was still running.

  • Shared helper — .claude/hooks/_lib-extract-pr.sh exposes is_merge_command + extract_pr_number, recognising both gh pr merge <N> and gh api .../pulls/<N>/merge. Sourced by all three merge-gate hooks so the parser lives in exactly one place.
  • Hooks refactored — block-unreviewed-merge.sh, require-design-review-for-ui.sh, and block-merge-on-red-ci.sh now call the helper instead of inline-grepping. Each also recovers owner/repo from the gh api URL path when --repo is absent, so downstream gh pr view / gh pr diff / gh pr checks remain scoped correctly.
  • Settings wiring — .claude/settings.json gets sibling Bash(gh api *) matcher entries for each of the three hooks. Option A (sibling entries), because the if field is a glob pattern, not a regex, so alternation isn't supported. The hooks themselves filter non-merge gh api calls via is_merge_command, which specifically looks for /pulls/<N>/merge in the URL path — any other gh api call exits 0 silently.
  • Docs — .claude/rules/pr-workflow.md § "Both merge shapes are gated (#47)" states the rule explicitly and calls out that using gh api .../merge to route around a gate is itself a rule violation on par with forging an approval marker. .claude/hooks/README.md notes the dual registration.

Test plan

  • bash .claude/hooks/block-unreviewed-merge.sh stdin = classic shape → BLOCKED, PR=99999
  • bash .claude/hooks/block-unreviewed-merge.sh stdin = API shape → BLOCKED, PR=99999
  • bash .claude/hooks/block-merge-on-red-ci.sh stdin = API shape → fires gh pr checks against the correct repo
  • bash .claude/hooks/require-design-review-for-ui.sh stdin = API shape → extracts PR, proceeds through diff check
  • Non-merge gh pr view 123 passes through silently (exit 0)
  • Non-merge gh api repos/foo/bar/issues/42 passes through silently (helper is_merge_command requires /pulls/<N>/merge)
  • JSON still valid after settings.json edit (jq . .claude/settings.json)
  • End-to-end verification that a real gh api merge attempt is blocked in a live Claude Code session (requires a PR to merge against — ideally this PR itself, after approval)

Glossary

Term Definition
Merge-gate hook A PreToolUse hook that runs before a merge command and exits 2 to block the merge when preconditions aren't met (Rex approval, CEO approval, green CI, design review for UI).
gh pr merge shape The high-level gh subcommand form of merging a PR, e.g. gh pr merge 123 --squash. What Claude is supposed to use.
gh api shape The raw REST-API form, e.g. gh api repos/owner/repo/pulls/123/merge -X PUT. Equivalent effect, different matcher — which is exactly what let it bypass the gates.
Matcher (if field) The glob pattern in .claude/settings.json that decides whether a hook fires on a given tool call. Globs don't support alternation, so two shapes need two matcher entries.
Option A vs Option B A — sibling matcher entries per hook (chosen, because globs only). B — one catch-all regex matcher (rejected, unsupported).
_lib- prefix Convention for a sourced helper script in .claude/hooks/ that is NOT a hook itself. The harness ignores it; other hook scripts source it.
Silent bypass A merge that appears to succeed and produces no hook output because no matcher pattern matched the command. The worst kind of failure — there's no audit trail.

Ticket: #47

@me2resh
fix: catch gh api .../merge bypass in merge-gate hooks
a44b3c5 
The three merge-gate hooks (block-unreviewed-merge.sh,
block-merge-on-red-ci.sh, require-design-review-for-ui.sh) only matched
`Bash(gh pr merge *)` in .claude/settings.json, and each extracted the PR
number by grepping the first integer after `gh pr merge`. Merges via the
raw REST-API shape `gh api repos/<owner>/<repo>/pulls/<N>/merge -X PUT`
silently skipped all three gates.

Incident: me2resh/curios-dog#190 was merged via `gh api` while CI was
still running.

Changes:
- New shared helper .claude/hooks/_lib-extract-pr.sh with
  is_merge_command and extract_pr_number that recognise both shapes.
- Refactor all three merge-gate hooks to source the helper. Also recover
  owner/repo from the gh api URL path so downstream `gh pr view`,
  `gh pr diff`, and `gh pr checks` stay repo-scoped.
- Settings.json: add sibling matchers on `Bash(gh api *)` for each of
  the three merge-gate hooks (Option A — matchers are globs, not regex,
  so alternation isn't supported).
- Docs: .claude/rules/pr-workflow.md gets a new "Both merge shapes are
  gated (#47)" subsection; .claude/hooks/README.md notes the dual
  registration and points future edits at the shared helper.

Verified both shapes via manual hook invocation:
  gh pr merge 99999 --squash                           -> BLOCKED, PR=99999
  gh api repos/me2resh/curios-dog/pulls/190/merge ...  -> BLOCKED, PR=190

Ticket: #47
atlas-apex
atlas-apex commented Apr 14, 2026
View reviewed changes

@atlas-apex atlas-apex left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #54

Commit: a44b3c5e27f3c2c3c7d30728e4badeffa49e5058

Summary

Closes the gh api .../pulls/<N>/merge -X PUT silent-bypass gap on all three merge-gate hooks (#47). Introduces _lib-extract-pr.sh as the single source of truth for PR-number parsing, adds sibling Bash(gh api *) matcher entries in settings.json, and updates docs.

Checklist Results

  • Architecture & Design: Pass — single parser in _lib-extract-pr.sh, three hooks source it, no duplication.
  • Code Quality: Pass — well-commented, defensive extraction (handles |, &&, ; boundaries), URL-path fallback for --repo recovery.
  • Testing: Pass (manual) — author's test plan covers both shapes + non-merge passthrough. Reproduced locally: classic shape blocks PR #100, gh api shape blocks the same PR #100, gh api .../issues/42 exits 0, gh pr view 123 exits 0.
  • Security: Pass — no cross-repo marker lookup; markers correctly scoped to local repo root via onboarding.yaml walk.
  • Performance: Pass — cheap greps, no new network calls in helper.
  • PR Description & Glossary: Pass — Glossary includes 7 terms covering gh api shape, matcher semantics, _lib- convention, silent bypass, Option A/B.
  • Technical Decisions (AgDR): N/A — the Option A vs Option B tradeoff is narrow (globs don't support alternation, so sibling entries are forced) and documented in PR body + pr-workflow.md. Not material enough for an AgDR.

Findings

  1. Minor — false-positive on gh api .../pulls/<N>/merge GET. is_merge_command matches the URL path without checking for -X PUT. A GET to inspect merge status (rare but legal) would also trip the gate. Not blocking — worst case is "use a different command to inspect." Safer than the opposite failure.
  2. Verified no self-approval: .claude/session/reviews/54-*.approved does not exist.
  3. Cross-repo resolution confirmed correct: markers read from local apexstack root regardless of the target repo in the API URL — exactly right for session-state semantics.

Follow-up (not blocking)

  • File: tighten is_merge_command to require -X PUT or --method PUT alongside the /merge path, to eliminate the GET false-positive.

Verdict

APPROVED

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: `a44b3c5e27f3c2c3c7d30728e4badeffa49e5058`

atlas-apex
atlas-apex commented Apr 14, 2026
View reviewed changes

@atlas-apex atlas-apex left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: PR #54

Commit: 1238688be77c7654ddb38aef530e90a7b0a6a44a

Verification

  • Incremental diff is exactly 4 deletions, 0 additions in block-unreviewed-merge.sh (the dead REPO_FLAG="" / if [ -n "$CMD_REPO" ] / REPO_FLAG="--repo $CMD_REPO" / fi block). Nothing else snuck in.
  • Grepped the file for REPO_FLAG — zero remaining references. CMD_REPO is still computed for parity with the sibling hooks but the unused REPO_FLAG is gone. No downstream logic broken: extract_pr_number from _lib-extract-pr.sh handles PR-number extraction without needing the flag.
  • CI on new HEAD: all green — shellcheck .claude/hooks passes, along with Verify Ticket ID, lychee, markdownlint-cli2.

Verdict

APPROVED (posted as comment — GitHub blocks self-approval)

🤖 Re-reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 1238688be77c7654ddb38aef530e90a7b0a6a44a

@atlas-apex atlas-apex merged commit 1299b59 into main Apr 14, 2026
4 checks passed
@atlas-apex atlas-apex deleted the fix/#47-merge-gate-catch-gh-api branch April 14, 2026 10:06
This was referenced Apr 14, 2026
Closed
Closed
Closed
osama-abu-baker pushed a commit to osama-abu-baker/apexyard that referenced this pull request Jun 3, 2026
@atlas-apex @me2resh
fix(me2resh#47): catch gh api .../merge bypass in merge-gate hooks (m…
3916ff1 
…e2resh#54)

* fix: catch gh api .../merge bypass in merge-gate hooks

The three merge-gate hooks (block-unreviewed-merge.sh,
block-merge-on-red-ci.sh, require-design-review-for-ui.sh) only matched
`Bash(gh pr merge *)` in .claude/settings.json, and each extracted the PR
number by grepping the first integer after `gh pr merge`. Merges via the
raw REST-API shape `gh api repos/<owner>/<repo>/pulls/<N>/merge -X PUT`
silently skipped all three gates.

Incident: me2resh/curios-dog#190 was merged via `gh api` while CI was
still running.

Changes:
- New shared helper .claude/hooks/_lib-extract-pr.sh with
  is_merge_command and extract_pr_number that recognise both shapes.
- Refactor all three merge-gate hooks to source the helper. Also recover
  owner/repo from the gh api URL path so downstream `gh pr view`,
  `gh pr diff`, and `gh pr checks` stay repo-scoped.
- Settings.json: add sibling matchers on `Bash(gh api *)` for each of
  the three merge-gate hooks (Option A — matchers are globs, not regex,
  so alternation isn't supported).
- Docs: .claude/rules/pr-workflow.md gets a new "Both merge shapes are
  gated (me2resh#47)" subsection; .claude/hooks/README.md notes the dual
  registration and points future edits at the shared helper.

Verified both shapes via manual hook invocation:
  gh pr merge 99999 --squash                           -> BLOCKED, PR=99999
  gh api repos/me2resh/curios-dog/pulls/190/merge ...  -> BLOCKED, PR=190

Ticket: me2resh#47

* fix: remove unused REPO_FLAG from block-unreviewed-merge.sh (shellcheck SC2034)

---------

Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com>
mosta7il pushed a commit to mosta7il/apexyard that referenced this pull request Jun 8, 2026
@atlas-apex @me2resh
fix(me2resh#47): catch gh api .../merge bypass in merge-gate hooks (m…
e5db018 
…e2resh#54)

* fix: catch gh api .../merge bypass in merge-gate hooks

The three merge-gate hooks (block-unreviewed-merge.sh,
block-merge-on-red-ci.sh, require-design-review-for-ui.sh) only matched
`Bash(gh pr merge *)` in .claude/settings.json, and each extracted the PR
number by grepping the first integer after `gh pr merge`. Merges via the
raw REST-API shape `gh api repos/<owner>/<repo>/pulls/<N>/merge -X PUT`
silently skipped all three gates.

Incident: me2resh/curios-dog#190 was merged via `gh api` while CI was
still running.

Changes:
- New shared helper .claude/hooks/_lib-extract-pr.sh with
  is_merge_command and extract_pr_number that recognise both shapes.
- Refactor all three merge-gate hooks to source the helper. Also recover
  owner/repo from the gh api URL path so downstream `gh pr view`,
  `gh pr diff`, and `gh pr checks` stay repo-scoped.
- Settings.json: add sibling matchers on `Bash(gh api *)` for each of
  the three merge-gate hooks (Option A — matchers are globs, not regex,
  so alternation isn't supported).
- Docs: .claude/rules/pr-workflow.md gets a new "Both merge shapes are
  gated (me2resh#47)" subsection; .claude/hooks/README.md notes the dual
  registration and points future edits at the shared helper.

Verified both shapes via manual hook invocation:
  gh pr merge 99999 --squash                           -> BLOCKED, PR=99999
  gh api repos/me2resh/curios-dog/pulls/190/merge ...  -> BLOCKED, PR=190

Ticket: me2resh#47

* fix: remove unused REPO_FLAG from block-unreviewed-merge.sh (shellcheck SC2034)

---------

Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@me2resh me2resh me2resh approved these changes

Assignees

No one assigned

Labels

github-issue has-ticket

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@atlas-apex @me2resh