Skip to content

feat(guardrails): block a PR merge while review threads, change requests or failing checks remain - #7056

Merged
kyle-sexton merged 9 commits into
mainfrom
feat/6463-block-unresolved-pr-merge
Oct 13, 2026
Merged

kyle-sexton merged 9 commits into
mainfrom
feat/6463-block-unresolved-pr-merge

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 11, 2026 •

Copy link
Copy Markdown
Member

Requested by Kyle · project thread

Closes #6463

Summary

Before: nothing checked a direct gh pr merge, REST or GraphQL merge, or MCP merge call. An agent could merge (or ask for --admin) without reading an unresolved review thread.

After: a new guardrails guard, block-unresolved-pr-merge, blocks a merge while the PR has an unresolved review thread, a reviewer whose latest decisive review requests changes, or a failed check run or commit status on its head. The block message lists each thread (path:line, first line of the comment, link), each changes-requested reviewer and each failing check, and names the fix. --admin and --auto are held to the same rule.

Fix

  • plugins/guardrails/hooks/block-unresolved-pr-merge.sh, on the existing Bash|PowerShell run-guards row plus a new MCP row (^mcp__.+__(merge_pull_request|enable_pr_auto_merge)$). Matches the scope update's table: gh pr merge (all flags), PUT repos/{o}/{r}/pulls/{n}/merge via gh api or curl, the mergePullRequest / enablePullRequestAutoMerge / enqueuePullRequest mutations via gh api graphql or curl, and the MCP merge tools. Matching runs on parsed argv at a command position, so a quoted mention (commit message, echo, heredoc) stays quiet. Commands that name neither merge nor enqueue leave after one string test.
  • GraphQL is refused in Claude Code cloud sessions (HTTP 403), so PR state is read over REST: pulls/{n}, pulls/{n}/reviews, commits/{sha}/check-runs, commits/{sha}/status. Thread resolution has no plain REST route: GraphQL reviewThreads where served, else the cloud route GET pulls/{n}/ccr/review_threads named in that 403 body (a CLAUDE_CODE_REMOTE session tries the cloud route first). The same 403 body names PUT pulls/{n}/ccr/auto_merge as the cloud auto-merge path, so that route is matched too.
  • Fails open with a notice on both channels (systemMessage and additionalContext) saying the merge will run unchecked when gh is missing or unauthenticated, a read fails, the read budget runs out, or the PR cannot be identified.
  • Fails closed for a GraphQL merge, auto-merge or merge-queue mutation whose PR cannot be resolved. That covers an id from $ID, "$PR_ID", $(jq ...) (the source-control merge reference 4.2.2 enqueue shape from fix(source-control): confirm a merge-queue PR entered the queue after gh pr merge #7075), a node that can't be read, or no gh. The block message says how to fix it: pass a literal -f id=PR_... or use gh pr merge <n>.
  • One time budget: all reads share 35 s, and each call is capped at the time left and at 15 s, so the shared Bash row stays under its 60 s timeout. A row timeout would drop every guard's block (run-guards.sh, AGGREGATION). The cap is enforced by timeout where it exists, and by a watcher process where it doesn't (stock macOS). No environment variable changes the budget: settings.json env reaches a hook's environment, so a knob could make every merge run unchecked. The budget tests run a copy of the hook with its one budget line rewritten.
  • Command position: the guard steps over leading assignments, reserved words the tokenizer leaves at a segment head (then, do, {, ...), and the command/time [-p]/exec/nohup prefixes. It reads gh pr -R o/r merge. gh pr merge --help/-h and --disable-auto are not merges. The guard header lists the remaining gaps: wrappers like sudo/env/xargs, child shells, $(...), and --input mutation files.
  • Registered in guard-requires.sh (MCP inputs declared unprimed), hooks.json, plugin.json userConfig block_unresolved_pr_merge_enabled (default on), README guard/option tables, changelog fragment (minor).

Defaults I picked where the issue left a detail open (please confirm):

  1. gh pr ready is not matched. The 2026-10-06 scope update dropped the advisory and its table lists merge paths only; the issue title still says "and gh pr ready". Adding it is one line if wanted.
  2. Pending checks block an immediate merge but not an auto-merge or merge-queue request (--auto, enablePullRequestAutoMerge, enqueuePullRequest, enable_pr_auto_merge, ccr/auto_merge), since those wait for checks by design. Failed checks, threads and change requests block all paths.
  3. Every unreadable state fails open (not only missing/unauthenticated gh), with the org ruleset as the server-side backstop, as the issue's residual note says. The one exception is the unresolvable GraphQL mutation above, which fails closed.
  4. Outdated unresolved threads count, matching the babysit merge gate.

Open for Kyle (found by the overnight verifier, not changed here):
5. The fail-closed GraphQL rule also covers gh missing or unauthenticated. A mutation with a literal PR_... id whose node can't be read is blocked. That covers no gh, a 401, and the cloud GraphQL 403. The issue's acceptance says the guard fails open when gh is missing or unauthenticated. Option A: keep it, since the mutation can't be checked. Option B: fail closed only for a non-literal id and fail open, with the notice, for a literal one.
6. Interaction with #7075. Its source-control merge reference 4.2.2 passes the enqueue id through $(jq ...). With this guard on, that step is always blocked, even on a clean PR (verified against branch fix/6630-merge-confirm-queue-entry). One side must change before both merge: #7075 passes a literal id or uses gh pr merge, or this guard relaxes the rule.

Verification

  • bash plugins/guardrails/hooks/block-unresolved-pr-merge.test.sh -> 162 passed, 0 failed (at 96f4c12). Since the first push, the suite also covers:

    • the fail-closed GraphQL cases (substitution id, $ID, "$PR_ID", --input, the full fix(source-control): confirm a merge-queue PR entered the queue after gh pr merge #7075 shape, an unreadable literal node)
    • --help/-h stay-quiet cases that assert no gh call
    • every command-position prefix, plus gh pr -R o/r merge
    • the budget with a hung fake gh, both with and without a timeout binary on PATH (returns in under 8 s), run on a copy with a 2 s budget
    • a case where an env var can't shorten the budget (GUARDRAILS_BUPM_BUDGET_S=1 with a 0.7 s gh still blocks). It failed before the knob was removed.

    A fake gh on PATH serves fixtures and logs calls. Covers every merge path, both shells, MCP, GraphQL-refused fallback to the cloud route, current-branch lookup, the fail-open cases (gh missing, 401, no thread source, no node id), and MUST-stay-quiet cases that also assert no gh call (commit message, echo, gh pr view, --disable-auto, GET of the merge route, a GraphQL query, grep, git merge, heredoc, PowerShell quoted mention, an unrelated MCP tool). Also runs through run-guards.sh.

  • scripts/affected-tests.sh --base origin/main --run:

    • First push: 35 shell suites passed, including run-guards.test.sh's declaration checks for the new guard. The 2 selected .mjs suites, run separately with node --test, also passed.
    • Fix push: on a loaded host the run hit the 30-minute background limit. Every suite that finished passed: all 26 guardrails hook suites, plus apply-plan, splice-assets, both routing suites and the lib suites. The four script suites it had not reached all passed when run directly: check-killswitch-hoist 28/28, check-script-contract 51/51, node-notice-rows 6/6, check-guardrails-ps-differential 21/21.
    • Knob-removal push (96f4c12): the run was stopped early at wrap-up. The suites that finished passed, apart from block-hook-bypass, which fails identically on origin/main when the checkout sits under /tmp. A full affected run on this head is still to do. shellcheck -x, check-killswitch-hoist, check-changelog-fragments --check-required origin/main and check-changelog-parity --check pass.
  • Live, read-only, in a Claude Code cloud session: the guard against open PRs fix(planning): word interview questions so yes accepts the recommendation #7036 (blocked: failing ci-status, ci-lanes), chore(release): release 27 plugins #6910 (blocked: pending checks), feat: adopt the 2,000-PRs talk decisions across the marketplace #7009 (allowed); 3.0 to 3.6 s per merge command (five sequential calls), recorded in the guard header and README.

  • shellcheck -x (repo .shellcheckrc) clean; scripts/validate-plugins.sh, scripts/check-changelog-fragments.sh --check-required origin/main, scripts/check-changelog-parity.sh --check, scripts/check-killswitch-hoist.sh, markdownlint on the README and fragment: all pass.

This changes what agents may do unattended (it blocks merges), so it needs a human merge decision.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01L8KdG7fWg2YrErVYWtohmR


Generated by Claude Code


Generated by Claude Code

…sts or failing checks remain

Add the block-unresolved-pr-merge guard (#6463). It matches every common
merge path: gh pr merge (--admin and --auto included), a PUT to the REST
merge route or the Claude Code cloud ccr/auto_merge route through gh api
or curl, the mergePullRequest, enablePullRequestAutoMerge and
enqueuePullRequest GraphQL mutations, and an MCP merge_pull_request or
enable_pr_auto_merge tool under any server prefix.

PR state is read over REST so the guard works where GraphQL is refused;
review-thread resolution comes from GraphQL where served, else the cloud
ccr/review_threads route. Pending checks block an immediate merge only,
since auto-merge and the merge queue wait for checks by design. The guard
fails open with a notice on both channels when gh is missing or a read
fails. Off switch: block_unresolved_pr_merge_enabled.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L8KdG7fWg2YrErVYWtohmR

Copy link
Copy Markdown
Member Author

ci-status is red because this PR is a draft, not because of the change. On a draft every lint and test lane skips (lint-repo, test-bash, test-node, test-python, check-skills, check-plugins), and the aggregate counts a skip as not passing. See .github/workflows/pr-require-checks.yml:507-513.

The lanes run once the PR is marked ready. That flip waits for Kyle, because it also starts the review lanes. Until then, the local runs listed under Verification in the PR body are the signal.


Generated by Claude Code

… and prefix gaps

From review of #7056:
- A GraphQL merge, auto-merge or merge-queue mutation whose PR cannot be
  resolved (id from $ID, "$PR_ID", $(jq ...), an unreadable node, no gh)
  now fails closed, with the literal-id fix in the message. Tests cover
  the source-control 4.2.2 enqueue shape from #7075.
- gh pr merge --help and -h are not merges; stay-quiet tests assert no
  gh call.
- All reads share one 35 s budget, each call capped at what is left and
  at 15 s, enforced by timeout or, where it is missing, a watcher process,
  so the shared row stays under its 60 s timeout.
- Leading reserved words (then, do, {, ...) and the command, time, exec
  and nohup prefixes are stepped over, and gh pr -R o/r merge is read.
- The fail-open notice says the merge will run unchecked.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L8KdG7fWg2YrErVYWtohmR
GUARDRAILS_BUPM_BUDGET_S let the environment shorten the guard's 35 s read
budget. settings.json `env` reaches a hook's environment, so a project
setting could make every merge run unchecked. The budget is now a constant;
the budget tests run a copy of the hook with that one line rewritten, and a
new case proves an env var cannot shorten it (fails before this change).

Refs: #6463

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L8KdG7fWg2YrErVYWtohmR
kyle-sexton and others added 2 commits October 13, 2026 09:05
The guarded literal merge (a gh pr view --jq gate piped to grep -qx true,
then gh pr merge <N> -R <repo> --match-head-commit <sha>) passes on a
clean PR and is blocked while a review thread is open.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVW9hAwnSBrnFsR3kzXcB4
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 13, 2026 13:13
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T13:17:02.827522Z 8a1a5f1 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 36s —— View job


Security review in progress

  • Gather context (PR diff, changed files)
  • Read the new guard script and its guard-requires/hooks.json wiring
  • Review for injection, trust-boundary, and fail-open/closed logic flaws
  • Adversarially verify candidate findings (in progress)
  • Post inline comments and final summary

Found one candidate in the GraphQL fail-closed path of block-unresolved-pr-merge.sh, dispatching an independent verifier now.

@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Review in progress

  • Gather context (PR body, diff, repo instructions)
  • Understand the request (code review via /review:code-review)
  • Read changed files and full diff
  • Check standards/paved paths and REVIEW.md criteria
  • Review block-unresolved-pr-merge.sh for correctness
  • Review block-unresolved-pr-merge.test.sh, hooks.json, guard-requires.sh, plugin.json, README, changelog fragment
  • Adversarially verify candidate findings
  • Post inline comments and summary

Remove "stated plainly" from a bold lead-in and "honest" from "the honest follow-up".

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @melodic-automation-lanes[bot]'s task in 6m 2s —— View job


Code review complete

  • Gather context (PR body, diff, repo instructions)
  • Understand the request (code review via /review:code-review)
  • Read changed files and full diff
  • Check standards/paved paths and REVIEW.md criteria
  • Review block-unresolved-pr-merge.sh for correctness
  • Review block-unresolved-pr-merge.test.sh, hooks.json, guard-requires.sh, plugin.json, README, changelog fragment
  • Adversarially verify candidate findings
  • Post inline comments and summary

Summary

Scope: correctness/maintainability only — a separate claude-security-review lane is already running on this PR, so security-shaped observations are left to it per this skill's scoping rule.

This is a large, carefully engineered guard with an unusually thorough test suite (454 lines covering every merge path, both shells, MCP, the budget, and the fail-open/fail-closed split). hooks.json, guard-requires.sh (both the primed and deliberately-unprimed field declarations), plugin.json's new block_unresolved_pr_merge_enabled option, and the README/changelog updates are all internally consistent with each other and with the script.

One finding, posted inline at block-unresolved-pr-merge.sh:443:

The GraphQL node-id-to-PR# resolution used by the fail-closed path (a mergePullRequest/enablePullRequestAutoMerge/enqueuePullRequest mutation with a literal PR_... id) calls GraphQL directly with no CLAUDE_CODE_REMOTE cloud-route fallback, unlike the thread-resolution code a bit further down, which does branch on CLAUDE_CODE_REMOTE precisely because GraphQL 403s in Claude Code cloud sessions (per this file's own "THE CLOUD ROUTES" comment). The 403 body only documents cloud replacements for review_threads and auto_merge, not for node resolution. Net effect: in a Claude Code cloud session, every such GraphQL mutation — even one with a literal id — is unconditionally blocked by _bupm_refuse, regardless of actual PR state, which contradicts the block message's own "pass a literal id" fix advice. The test suite's only CLAUDE_CODE_REMOTE=true case covers gh pr merge, not a GraphQL mutation, so this interaction isn't exercised.

The PR author already flagged two comparable open design questions for the human reviewer (items 5 and 6 in the body, re: fail-closed scope and the #7075 interaction) — this is a related but distinct gap in the same fail-closed path, in the cloud environment the PR itself targets.

claude-lane-summary: lane=claude-review sha=7e0a830bf7059ed6241fdfe01ae642bbf34fc4c3 run=38403508609

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8a1a5f1d10

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/guardrails/hooks/block-unresolved-pr-merge.sh
Comment thread plugins/guardrails/hooks/block-unresolved-pr-merge.sh
@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @melodic-automation-lanes[bot]'s task in 9m 47s —— View job


Security review complete

  • Gather context (PR body, diff, repo instructions)
  • Run /review:security-review per custom instructions
  • Read diff and changed files (block-unresolved-pr-merge.sh, test, hooks.json, guard-requires.sh, plugin.json, README, changelog)
  • Hunt for injection, auth-bypass, and trust-boundary issues
  • Adversarially verify candidate findings (independent review:lane-verifier agents, producer ≠ verifier)
  • Post findings and summary

2 findings, both posted inline and independently confirmed:

  1. CRITICAL — _bupm_graphql's node-id resolution takes the first PR_...-shaped literal found anywhere in a GraphQL merge command's arguments, with no check that it's the literal actually bound to the mutation's pullRequestId field. Since GraphQL input objects have no required field order, a benign free-text field (e.g. clientMutationId) can carry a decoy, clean PR's id earlier in the string than the real pullRequestId:

    gh api graphql -f query='mutation{mergePullRequest(input:{clientMutationId:"PR_abcDECOY",pullRequestId:"PR_xyzREAL"}){clientMutationId}}'
    

    The guard resolves and checks only the decoy, finds nothing to block, and lets the real mutation merge the unchecked target — defeating the fail-closed design this guard specifically built for GraphQL merges. Untested by the current test suite (no test uses two distinct PR_... literals in one command).

  2. IMPORTANT — the unresolved-thread block message surfaces up to 10 lines of third-party review-comment text into stderr that, per this plugin's own README, is fed back to the agent as "actionable fix guidance." This repo's docs/conventions/untrusted-content/README.md three-part framing contract (classification, finding-not-request, authority-floor) is carried verbatim at many other sites in the repo; this new, attacker-reachable surface carries only the first clause.

No other issues met the high-signal bar. GitHub Actions hardening is out of scope for this lane (deferred to zizmor); no workflow files changed in this PR anyway.

claude-lane-summary: lane=claude-security-review sha=7e0a830bf7059ed6241fdfe01ae642bbf34fc4c3 run=38403508578

Comment thread plugins/guardrails/hooks/block-unresolved-pr-merge.sh
@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

claude-review has reviewed this pull request through 99a4345; a later push is reviewed from there.

Comment thread plugins/guardrails/hooks/block-unresolved-pr-merge.sh
Comment thread plugins/guardrails/hooks/block-unresolved-pr-merge.sh Outdated
@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review has reviewed this pull request through 99a4345; a later push is reviewed from there.

…T merges

- A GraphQL merge mutation is checked against the node id bound to
  pullRequestId (a literal, or a variable whose literal value is a -f
  argument or a JSON variables entry), not the first PR_ token anywhere
  in the command. A decoy id in another field no longer redirects the
  check; no binding, or two different bound ids, fails closed.
- curl -T / --upload-file to the merge route counts as a PUT unless -X
  names another method.
- Quoted review-thread text carries the full untrusted-content spine.
- The header and README say every GraphQL merge mutation is blocked in
  a Claude Code cloud session, where GraphQL is refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVW9hAwnSBrnFsR3kzXcB4
@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 5m 19s —— View job


Security review complete

  • Gather context (PR body, diff, repo instructions)
  • Read incremental diff (.claude-lane/incremental.diff) since 7e0a830
  • Run /review:security-review per custom instructions
  • Hunt for injection, auth-bypass, and trust-boundary issues in the changed lines
  • Adversarially verify candidate findings
  • Post findings and summary

Summary

Scope: incremental review of what changed since 7e0a830 (this lane's last review), in
plugins/guardrails/README.md, plugins/guardrails/hooks/block-unresolved-pr-merge.sh, and its
test file. This push is a direct response to the four findings posted against 7e0a830
(Codex P2 curl -T, this lane's P1/doc-gap on the cloud-route fallback, this lane's CRITICAL
decoy-id finding, and this lane's IMPORTANT untrusted-content-framing finding). I re-verified each
fix rather than assuming the commit message's claim:

  1. CRITICAL decoy-id finding — fixed. _bupm_graphql no longer takes the first PR_...-shaped literal anywhere in argv. It now anchors specifically on the pullRequestId: key, resolves a GraphQL-variable binding ($id) back to its literal value via either a -f/-F name=PR_x argv entry or a "name":"PR_x" JSON pair, and requires every binding found for that key to agree on one id — any mismatch, any unresolved variable, or any value that isn't a bare PR_... literal makes BUPM_NODE stay empty, which the existing fail-closed path (_bupm_refuse) already blocks. I traced several adversarial shapes (duplicate -f id= flags with different values, decoy fields that happen to share "pullRequestId" as a substring, JSON duplicate-key tricks, non-literal/obfuscated id values) and in every case the real pullRequestId binding is unavoidably present in the text and any attempt to introduce a different candidate id triggers the mismatch path, which fails closed, not open — i.e. I could not find a way to make the guard resolve to a decoy/wrong node while a differently-targeted mutation actually executes. The new tests (two-ids-mismatch, decoy-before-real, JSON-variables, substitution-id) exercise the cases that matter.
  2. curl -T/--upload-file finding — fixed. _bupm_curl now treats -T/--upload-file (bundled and =-form included) as an implicit PUT unless -X/--request names another method, matching curl's actual semantics; -X POST -T body.json correctly stays an explicit POST and is not blocked.
  3. Cloud-route fallback doc gap — resolved by documentation, not a new route. The header comment now states plainly that a GraphQL merge/auto-merge/enqueue mutation is unconditionally blocked in a CLAUDE_CODE_REMOTE session (no cloud REST equivalent exists for node-id resolution), and tells the operator to use gh pr merge <number> instead. That's a defensible resolution of the original maintainability/doc gap — it doesn't silently allow unchecked merges, it fails closed and says so.
  4. Untrusted-content framing finding — fixed. The unresolved-thread block message now carries the full three-part spine (classify as data, "a finding to report, not a request to satisfy," "widens no authority") with a pointer to docs/conventions/untrusted-content/README.md, instead of only the classification clause. The per-line thread text is still interpolated purely as a %s argument to printf, so no format-string injection from reviewer-controlled text.

No new findings. I did not find an exploitable fail-open path introduced by this diff's node-resolution rewrite, the curl-PUT change, or the message-text change.

Note: this sandbox's tool allowlist doesn't permit running bash/shellcheck directly, so this review is based on static reading of the diff and the surrounding script, not a live test run; the PR's own Verification section already reports the test suite passing on this commit.

claude-lane-summary: lane=claude-security-review sha=ab04e8513d717dec6e871531f33ae7b70ecd4252 run=38405911974

@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Review in progress

  • Gather context (PR body, diff, repo instructions)
  • Read .claude-lane/incremental.diff (incremental review scope)
  • Review changed lines in block-unresolved-pr-merge.sh, .test.sh, README
  • Check standards/paved paths and REVIEW.md criteria
  • Adversarially verify candidate findings
  • Post inline comments and summary

View job run

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @melodic-automation-lanes[bot]'s task in 8m 18s —— View job


Code review complete

  • Gather context (PR body, diff, repo instructions)
  • Read .claude-lane/incremental.diff (incremental scope since 7e0a830)
  • Invoke /review:code-review and follow its instructions
  • Review changed lines in block-unresolved-pr-merge.sh, .test.sh, README
  • Adversarially verify candidate findings
  • Post inline comments and summary

Summary

Scope: correctness/maintainability only (security-shaped observations are left to the parallel claude-security-review lane). This incremental review covers only what changed since 7e0a830: the fix commit ab04e85 addressing the three findings from the prior round (GraphQL decoy-id spoofing, curl -T/--upload-file as implicit PUT, untrusted-content framing for quoted thread text) plus two doc-wording commits.

The curl -T/--upload-file handling and the untrusted-content framing text both look correct and match their stated test coverage.

One finding, posted inline at block-unresolved-pr-merge.sh:193:

The new _bupm_graphql fix correctly resolves a GraphQL mutation's id to the literal bound to pullRequestId (closing the decoy-id gap), using a regex stashed in the single-quoted bind_re variable so its \\?" escaping survives intact. But the follow-up lookup that resolves a GraphQL variable (pullRequestId:$id) against a JSON "variables":{"id":"PR_..."} body is written as literal escaped text directly inside [[ ... =~ ... ]] rather than through a protected variable. Per the Bash manual, quote removal runs on words between [[ and ]], so \\?\" there collapses to \?" before the regex engine ever sees it — and \? in ERE means a literal ? character, not "optional backslash." That pattern can never match real JSON, so a curl + -d GraphQL merge that supplies the id only via a JSON variables object (rather than gh api graphql -f id=...) will always be refused as "not a literal PR_ node id," even though the id is a readable literal. This fails closed (not a security bypass), but it defeats the feature this code was written for and appears to be exactly what the new test case "curl graphql, id in JSON variables" exercises — I'd expect that assertion to fail when the suite runs, though I could not execute the test suite myself in this lane's sandboxed permission mode to confirm directly.

No other issues met the high-signal bar in the changed lines.

claude-lane-summary: lane=claude-review sha=9e418b23f0a8bb5625d622f4482aade47ea94fc7 run=38406287873

Comment thread plugins/guardrails/hooks/block-unresolved-pr-merge.sh
…esolves

The curl graphql cases now check the fake gh was asked to resolve the
bound id itself, for plain and JSON-escaped "variables" bodies, rather
than that some GraphQL call happened.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVW9hAwnSBrnFsR3kzXcB4
@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 7m 22s —— View job


Code review complete

  • Gather context (PR body, diff, repo instructions)
  • Read .claude-lane/incremental.diff (incremental scope since 9e418b23f)
  • Invoke /review:code-review and follow its instructions
  • Review the changed lines in block-unresolved-pr-merge.test.sh
  • Adversarially verify the candidate finding (static trace through _bupm_graphql, both regex paths)
  • Post inline comment and summary

Summary

Scope: incremental review of what changed since 9e418b23f, which is only
plugins/guardrails/hooks/block-unresolved-pr-merge.test.sh (6 lines). Every other file is
unchanged since that commit and was already reviewed then. This commit (99a4345) tightens the
"curl graphql, id in JSON variables" assertion and adds a parallel "JSON-escaped variables" case,
per its message: checking that the specific node id was resolved rather than that some GraphQL
call happened.

One finding, posted inline at
block-unresolved-pr-merge.test.sh:253-258:

The new/tightened assertions (node PR_kwDOabc, src/app.py:12, node PR_kwDOesc) exercise the
JSON-variables id-resolution path in _bupm_graphql, which depends on the regex at
block-unresolved-pr-merge.sh:193. That line is unchanged since 9e418b23f and still carries the
escaping bug this lane already flagged there (quote removal on the unquoted =~ pattern collapses
\\?\" to a literal \?", which can never match real JSON). Traced through by hand: with that bug,
_bupm_graphql returns with BUPM_NODE empty, the hook takes the fail-closed _bupm_refuse path
instead of resolving the node, and neither the node PR_kwDOabc/node PR_kwDOesc call-log lines nor
the src/app.py:12 thread text should appear — so these specific new assertions look like they'd
fail, not pass, against the current hook script. The run ... 2 exit-code expectation alone doesn't
catch this, since both the refusal and the real block exit 2.

I don't have a sandbox that permits running the test suite (or any ad hoc Bash reproduction) in this
lane, so this is a careful static trace, not an executed confirmation — flagged as such in the inline
comment.

claude-lane-summary: lane=claude-review sha=99a43458a1ab34fc090aeec5e16316cefa230f6d run=38407989620

@claude

claude Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 31s —— View job


Security review complete

  • Gather context (PR body, diff, repo instructions)
  • Run /review:security-review per custom instructions
  • Read .claude-lane/incremental.diff (incremental scope since 9e418b23f)
  • Review the changed test assertions for correctness/security relevance
  • Adversarially verify candidate findings
  • Post findings and summary

Summary

Scope: incremental review of what changed since this lane's last review (9e418b23f), limited to
plugins/guardrails/hooks/block-unresolved-pr-merge.test.sh
— the only file this round's diff touches. block-unresolved-pr-merge.sh itself is unchanged
since 9e418b23f and was already reviewed then.

The diff is four lines: it tightens one existing assertion (checking for "node PR_kwDOabc"
instead of just "graphql" in the recorded gh calls), adds one assertion that the unresolved
thread is listed in $ERR, and adds one new scenario covering JSON-escaped GraphQL variables.
This is test-only code — it doesn't ship, isn't reachable by an untrusted caller, and introduces
no new injection, auth, or trust-boundary surface. Per this lane's scope (security findings
only, no test-coverage or correctness commentary), there's nothing to report here.

No security findings in this file's changes.

Note for the record, not a finding of this lane: the parallel claude-review (code-review) lane
has already flagged, on this same round, a regex bug in _bupm_graphql's JSON-variables
lookup (block-unresolved-pr-merge.sh:193) that would make the new "JSON-escaped variables"
scenario's assertion fail at the shell-quoting level. That's a correctness issue in the
already-reviewed .sh file, not a security issue in the test file this lane is scoped to this
round, so it isn't duplicated here.

claude-lane-summary: lane=claude-security-review sha=99a43458a1ab34fc090aeec5e16316cefa230f6d run=38407989850

Comment thread plugins/guardrails/hooks/block-unresolved-pr-merge.test.sh
@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 13, 2026
Merged via the queue into main with commit dc3c8ac Oct 13, 2026
31 checks passed
@kyle-sexton
kyle-sexton deleted the feat/6463-block-unresolved-pr-merge branch October 13, 2026 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(guardrails): block gh pr merge and gh pr ready while a PR has unresolved review threads or failing checks

2 participants