fix(agents): honor ruleset-required reviews in the merge author bound - #547
Conversation
GitHub leaves reviewDecision null when a ruleset, not classic branch
protection, requires reviews, so the merge tool refused teammates' PRs
on ruleset-protected repos and said the repo required no review. Read
the base branch's active rules when reviewDecision is null, word the
refusal as what was observed, accept GITHUB_APP_NAME with a [bot]
suffix, and return refusals in the bare { error } shape the other
guards use.
![mogplex[bot]](https://avatars.githubusercontent.com/in/3107927?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
Approve-ready: the ruleset review detection is correct and fail-closed. GitHub documents that GET /repos/{owner}/{repo}/rules/branches/{branch} returns only active (enforced) rules, so evaluate-mode rulesets cannot widen the merge bound, and the endpoint is callable with the installation token the tool already holds, matching the PR's premise. One non-blocking warning: the branch-rules fetch omits pagination, so branches with more than 30 applicable rules can still hit the false refusal this PR fixes.
4 findings were added inline.
| }) { | ||
| const doFetch = input.fetchImpl ?? fetch; | ||
| const res = await doFetch( | ||
| `https://api.github.com/repos/${input.owner}/${input.repo}/rules/branches/${encodeURIComponent(input.branch)}`, |
There was a problem hiding this comment.
Warning: Branch-rules fetch omits pagination; branches with >30 rules can be misread as unconfirmed
The call to GET /repos/{owner}/{repo}/rules/branches/{branch} uses GitHub's default per_page=30 and reads only the first page (documented at https://docs.github.com/en/rest/repos/rules#get-rules-for-a-branch). Organizations that stack many rulesets can exceed 30 applicable rules on one branch; if the pull_request rule lands on a later page, rulesetRequiresReview returns false and someone else's PR gets the exact false refusal this PR fixes ("Mogplex could not confirm the repository requires a human review"). The failure direction is safe — it refuses rather than merging unsafely — so this is a warning, not a blocker. Suggest ?per_page=100 and following pages until one returns fewer than per_page rules, with a small page cap.
| rules.some( | ||
| (rule) => | ||
| rule.type === "pull_request" && | ||
| (rule.parameters?.required_approving_review_count ?? 0) > 0 |
There was a problem hiding this comment.
Suggestion: Review requirements without an approval count are still treated as unconfirmed
A pull_request rule can require a human review while required_approving_review_count is 0: parameters.require_code_owner_review: true requires a code-owner approval, and parameters.required_reviewers (with minimum_approvals) requires named reviewers. For those rulesets the predicate returns false, so the tool refuses a genuinely review-required pull request and the message claims the requirement could not be confirmed. The PR description scopes this deliberately to "at least one required approval", so if intentional, a one-line comment naming the excluded rule parameters would prevent future confusion; otherwise extend the predicate to also honor require_code_owner_review or positive required_reviewers[].minimum_approvals.
| rulesetRequiresReview: (branch: string) => Promise<boolean>; | ||
| }; | ||
|
|
||
| async function requiresHumanReview( |
There was a problem hiding this comment.
Suggestion: Ruleset bypass actors can make a confirmed review requirement unenforced
The bound's premise — GitHub keeps a person in the loop because the branch requires review — does not hold when a ruleset grants an actor a bypass, and in principle the GitHub App's own installation can be such an actor (an admin adding the app as an Integration bypass actor). GitHub then allows the merge without the review while the tool reports basis human_review. This is not detectable from this code: bypass_actors is only returned to callers with write access to the ruleset, and it mirrors the exemption already accepted for classic branch protection in #545, so it is a residual risk rather than a regression introduced here. A sentence in the module doc comment noting that ruleset bypasses are outside this check would document the limit.
| audit: { | ||
| decision: "needs_user_merge", | ||
| error: `author ${author} without a required review`, | ||
| error: `author ${author} without a confirmed review requirement`, |
There was a problem hiding this comment.
Suggestion: Rules fetch network errors are reported as ownership-check failures
A thrown fetch (network error rather than a non-OK response) propagates to the surrounding catch in authorizeMergeAuthor, which returns the "could not check who opened" refusal instead of the new "could not confirm a review requirement" wording. It still fails closed and is audited, but the message points debugging at the wrong API call. Consider catching fetch rejections in rulesetRequiresReview and returning false, consistent with how non-OK responses and malformed bodies are handled.
Page through the branch rules instead of reading the first 30, count code-owner and named-reviewer requirements alongside approval counts, treat a network failure on the rules call as unconfirmed, and note that ruleset bypass actors are outside what a reader can check.
Mogplex PR ReviewStatus: No material issues found No issues found; approve-ready. The ruleset review check matches GitHub's documented rule parameters and endpoint semantics, every unreadable-rules path fails closed to a refusal, and the tests meaningfully cover the new behavior including pagination, failure modes, and the
|
Treat a ruleset that requires approval of the most recent push as a review requirement, and log each unreadable rules read so a transient GitHub failure is distinguishable from a repo with no requirement.
Encode owner, repo, and branch in the branch-rules path so the exported helper can't read another repository's rules, and log when the page bound ends a scan so every unconfirmed answer is traceable.
1 participant
Follow-up to #545 from its last review round.
The merge tool's author bound lets the agent merge someone else's pull request only where the repository requires a human review, and it read that from GitHub's
reviewDecision. GitHub populates that field only for classic branch protection and CODEOWNERS; when a ruleset requires reviews it stays null. So a teammate's PR on a ruleset-protected repo was refused, and the refusal claimed the repository required no review.When
reviewDecisionis null, the tool now also reads the base branch's active rules (GET /repos/{owner}/{repo}/rules/branches/{branch}, readable with read access) and treats apull_requestrule that makes a person approve (an approval count, code-owner review, named reviewers, or approval of the most recent push) as a review requirement. It pages through all rules at 100 per page. If the rules can't be read, including on a network error, the requirement counts as not confirmed, so the tool still refuses, and the failed read is logged so a transient GitHub error can be told apart from a repo with no requirement. Ruleset bypass actors aren't visible to readers, so a bypass granted to the app is outside this check, the same as for classic branch protection. The refusal now says what was observed: Mogplex could not confirm the repository requires a human review.Two smaller fixes from the same review:
GITHUB_APP_NAMEset with a[bot]suffix no longer stops Mogplex-opened PRs from matching, and the author refusal returns the bare{ error }shape the other guards use.Tests:
tests/unit/github-pr-merge-author-bound.test.tsadds the ruleset-protected case and pins the new refusal wording (both fail against main).lib/agents/tools/github-pr-merge-ownership.test.tscovers the rules parsing, code-owner and named-reviewer requirements, a requirement past the first page, the zero-approval, unreadable, and network-failure cases, and the[bot]-suffixed app name.pnpm typecheck,pnpm test:unit(3753 pass), and thelib/agentsvitest tier are green.