Repository navigation
Added an opt-in approval step to bot-pr-sweep - #41
CodyCBakerPhD wants to merge 8 commits into
Conversation
--allow-approve lets the sweep approve, in the name of the account running it, a bot PR whose CI finished green on the head commit, so branch protection that requires a review lets it merge. GitHub, not the gate, then decides whether the PR is mergeable: the sweep gates the PR again after approving and merges only on a `merge` verdict, and a PR that protection still blocks is reported. The gate reads the PR's reviews (--reviews, bound to the PR). It sets "approve": true only when nothing but branch protection stands in the way, no approval already sits on the head commit, and no reviewer's latest review requests changes; with the option, requested changes also block the merge. The approval is pinned to the gated commit and carries a fixed body saying it is automated. New outcomes `approved` and `approve-failed`; the report notes each approval. Version 0.2.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQrzw4VG1wfzDPFtKCgnay
|
@yarikoptic-gitmate I beseech thee for thy wisdom |
yarikoptic-gitmate
left a comment
There was a problem hiding this comment.
Thanks @CodyCBakerPhD. CI is green. Approving only to satisfy review protection, with no admin bypass, is the right direction, and the change reuses the existing helpers (_dict, _int, sanitize, pages, GateInputError, the blocked()/pending() closures). Two independent reviews of the diff (one on branch-protection and review semantics, one on code reuse and tests) agreed on the points below. I read the diff and the code only. I did not run tox or the tests.
Should address
-
Approve only when protection is what blocks.
approveis also true for verdictmerge(mergeable_stateclean), so on repos with no required review the sweep still posts an automated approval in the maintainer's name on every green bot PR, and it stays on the PR if the merge then fails. The Inputs text itself says the flag exists "so branch protection that requires a review lets it merge". Restricting toblockedby protection alone also drops the "approve first, then merge" ordering for the clean case in step 5. Update SKILL.md step 5, the Inputs text and the README row to match. -
Don't compare reason prose to drive control flow.
all(reason == PROTECTION_REASON ...)means rewording that message silently turns approval off.update_branchright above it already uses a boolean pluslen(reasons[BLOCKED]) == 1; do the same and delete thePROTECTION_REASONconstant:
+ protected = False
...
elif mergeable_state == "blocked" and approved_at_head:
+ protected = True
blocked("branch protection still blocks although the head commit is approved; ...")
elif mergeable_state == "blocked":
+ protected = True
blocked("branch protection is not satisfied (reviews, required checks or merge queue)")
...
- "approve": bool(allow_approve and not approved_at_head and verdict in (MERGE, BLOCKED)
- and not reasons[PENDING]
- and all(reason == PROTECTION_REASON for reason in reasons[BLOCKED])),
+ "approve": bool(allow_approve and protected and not approved_at_head and verdict == BLOCKED
+ and not reasons[PENDING] and len(reasons[BLOCKED]) == 1),-
DISMISSEDas a decisive review (please verify).DECISIVE_REVIEWSincludesDISMISSED, andread_reviewskeeps only the last one per reviewer. Take reviewer A withCHANGES_REQUESTED, thenAPPROVED, and then that approval is dismissed after a push. The listing's latest decisive entry isDISMISSED, so A's earlier request for changes drops out ofchanges, and the sweep could approve over a person's review, which the skill promises it never does. I believe GitHub falls back to the latest non-dismissed review here, but I did not check that against the API. If you agree, dropDISMISSEDfrom the set so dismissed entries are skipped, and adjusttest_no_second_approval_on_the_same_headandtest_requested_changes_block_approval_and_merge, which currently pin the opposite. If you keep the current semantics, add a regression test for the[CHANGES_REQUESTED, APPROVED, DISMISSED]order and say why in a comment. -
--reviewswithout--allow-approveis silently ignored, while the reverse gives exit 2. Reject it in the samecmd_gateguard (thenreviews=pages(load_json(args.reviews)) if args.reviews else Noneis enough), and add one CLI test. -
Coordination with #39. Both PRs claim 0.2.0 and touch the same places (Inputs, step 5,
RESULTS,evaluate()signature,render_report,cmd_gate). #39 also reads the PR's reviews, treatsCHANGES_REQUESTEDas blocking and keys deleted reviewers per review, which is the same logic asread_reviews. Whichever lands second should rebase onto the first, reuse one reviews reader and take 0.3.0. Which is meant to land first?
Smaller points
- SKILL.md step 5 is long and repeats itself. "In the name of the account running the sweep" appears three times, and the approval conditions appear in both the
blockedtable row and the step 5 opening. The re-gate procedure could read: after approving ablockeddecision, re-run step 2 for that PR (PR and reviews fetched fresh, other files reused) and act on the new verdict; if it ispendingonly for mergeability, repeat once. Alternatively, "re-gate once; if it is notmerge, report it and let the next run merge" drops the third-gate paragraph. - Reword "pin the review to the gated commit" so it doesn't read as a TOCTOU guard (the merge's
sha=is the guard): "so the approval can never cover a commit the gate did not see". - The
--dry-runline in Inputs still says "neither assign, update, remove a label nor merge"; add "approve". The report header "dry run (nothing merged or assigned)" has the same gap. approved_at_headcounts anAPPROVEDreview on the head from any account, including one that GitHub doesn't count toward protection (no write access, another bot). That fails closed (the PR sits blocked with the "head is approved" reason), so one sentence in SKILL.md is enough unless you want to filter onauthor_association. Likewise, a human dismissing the sweep's approval makes it re-approve on each run, and with--allow-branch-updatethe sweep's own approval never counts under "latest push approved by someone other than the pusher". A note under Inputs that the two options don't combine in that setup would help.- The existing caveat that the
Bash(gh api repos/*/pulls*)glob can't pin the HTTP method now also covers posting approvals; worth adding "and approvals" there. - Tests:
test_requested_changes_block_approval_and_mergepacks about eight scenarios into one test (subTestor a split would localise failures). Several assertions pin reason text andreasons[0]position. Optional additions: a CLI test with a{"message": "Not Found"}reviews file expectingpending, and a PR-number-suffix case such as 7 against 17. - The new
RESULTSline is over 100 characters; check it against the repo's line-length config.
I'm happy to re-review after the changes.
Generated by Claude Code
- The gate decides on a `protected` flag and the count of blocked reasons, not on reason text; PROTECTION_REASON is gone. - A DISMISSED review is skipped, so a request for changes followed by a dismissed approval still blocks. Tests cover that order and a dismissed request. - --reviews without --allow-approve is refused like the reverse. - SKILL.md: step 5's re-gate is shorter and the account wording appears once; Inputs says where GitHub ignores the approval, that branch updates and approvals do not combine under the latest-push rule, that any approval on the head counts and that a dismissed one comes back; the dry-run line and the report header mention approvals; the allowed-tools caveat covers posting approvals. - Tests use subTest for the review scenarios and add a PR-number suffix case and a CLI case for an error object as the reviews payload. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQrzw4VG1wfzDPFtKCgnay
|
|
Thanks for the review. bfe95a3 addresses points 2 to 4 and the smaller points. Points 1 and 5 wait on the author. Should address
Smaller points
The 93 unit tests and Generated by Claude Code |
The gate sets "approve": true only on a `blocked` decision whose one problem is branch protection. A green PR that merges as it is no longer gets an automated approval, so repositories without a review requirement see none, and a failed merge leaves no stray approval behind. Step 5, Inputs, the verdict table, the description and the README row say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQrzw4VG1wfzDPFtKCgnay
|
The two open points are settled, and fca67db makes the change for point 1.
The 93 unit tests and the four tox gates pass. A read-only re-gate of the sweep's open bot PRs gives the same 7 approvals as before, since all 7 are held back by branch protection. Generated by Claude Code |
yarikoptic-gitmate
left a comment
There was a problem hiding this comment.
Thanks @CodyCBakerPhD. Re-checked fca67db (delta since 60a1040) with two independent reviews (branch-protection/review semantics; skill authoring and tests). CI is 4/4 green, and the 93 unit tests pass locally. They agree there is nothing blocking. Points 1 to 5 from my first review are addressed: approve fires only on a blocked decision whose one reason is protection, protected plus len(reasons[BLOCKED]) == 1 mirrors update_branch, PROTECTION_REASON is gone, and --reviews / --allow-approve are guarded both ways. A merge verdict never approves. I read the diff and the code, and re-ran the unit tests only, not tox.
Worth fixing (wording)
- "can only block more, never less" (comment above
DECISIVE_REVIEWS, repeated in SKILL.md) is not strictly true: a dismissedCHANGES_REQUESTEDno longer blocks, and a PR with no other live request can then merge. That is intended and tested ("the request dismissed" givesmerge). Suggest: "so the gate never approves over a request that is still live". - SKILL.md step 5
- "Name the gated commit, so the approval can never cover..." is vaguer than the earlier "Pin the review to the gated commit". Consider going back to "Pin".
- "A PR that merges as it is gets no approval." repeats the sentence before it. Fold it in or drop it.
- There is an orphaned short line, "says why). After approving, rerun step 2 for that PR,". Please reflow the paragraph.
- "a PR that protection still blocks (see Inputs) says the head is approved" has the PR doing the saying. Suggest "is reported with the reason that the head is approved but protection still blocks".
- Inputs: "counts as already given, which fails closed" has a dangling clause; "(fail-closed)" reads better. The approval-limits paragraph is long; trimming it is optional.
Tests (optional)
4. The [CHANGES_REQUESTED, DISMISSED] coverage is gone for the case where a second reviewer's dismissal follows an approval at the head. One case showing approve is False there would test the comment in point 1.
5. The "request dismissed" case is a lone review("DISMISSED"), so it shows a dismissed review is ignored rather than that a request was dismissed. Rename it or add a comment.
6. The ApproveTests loop that rewrites mergeable_state from clean to blocked is convoluted, and its comment is wrong for the "mergeability unknown" case, which stays unknown. A mergeable_state="blocked" default in the PR builder would be simpler.
Minor
- The
RESULTSreflow is churn with no functional change; harmless. - The gate cannot tell a missing review from other protection causes (e.g. a required check that has not reported), so it can approve pointlessly. The Inputs paragraph already covers this.
Ready to merge from my side once the wording points are settled; the test points are optional. I'll look at follow-ups.
Generated by Claude Code
- The DECISIVE_REVIEWS comment says what skipping dismissed reviews guarantees: the gate never approves over a request that is still live. - SKILL.md step 5 pins the review to the gated commit again, drops a repeated sentence, reflows the re-gate paragraph and says how a PR that protection still blocks is reported. Inputs reads "(fail-closed)". - Tests: a held() builder replaces the loop that rewrote mergeable_state, the lone dismissed-review case is named for what it shows, and a new case has a second reviewer's request outlive that reviewer's dismissed approval while another approval sits on the head. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQrzw4VG1wfzDPFtKCgnay
|
Thanks for the second pass. 721b2a3 takes all the wording points and the optional test points, and CI is 4/4 green on it. Wording
Tests
The 93 unit tests and the four tox gates pass. On a clean tree the scanner gives the same 3 findings as master. Generated by Claude Code |
yarikoptic-gitmate
left a comment
There was a problem hiding this comment.
Thanks @CodyCBakerPhD. Re-checked 721b2a3 (delta since fca67db) with two independent reviews (skill authoring and docs consistency; test design and review/protection semantics). CI is 4/4 green and the 93 unit tests pass locally. Both reviewers found nothing blocking and nothing that must change. All six points from my last review are addressed: the DECISIVE_REVIEWS comment, the step 5 wording ("Pin", the dropped duplicate sentence, the reflowed paragraph, the "reported with the reason" phrasing), "(fail-closed)" in Inputs, the second-reviewer test case, the renamed "dismissed review on its own" case, and the held() builder.
Optional nits only:
- SKILL.md step 5: "is reported with the reason that the head is approved but protection still blocks" could be tighter: "is reported as approved at the head but still blocked by protection".
held()could also replace the remainingpull(mergeable_state="blocked")calls in the tests (e.g. aroundtest_off_by_default), for consistency.- The second-reviewer case could carry a short comment such as "A = default maintainer, B = bob".
Ready to merge from my side. I read the diff and ran the unit tests only, not tox.
Generated by Claude Code
|
Thanks for the third pass. All three nits are optional, so 721b2a3 stays as it is:
A push only for these would re-run CI and move the commit CodyCBakerPhD's daily sweep is pinned to. If another change to bot-pr-sweep comes along, these go in with it. Generated by Claude Code |
- SKILL.md step 5: a PR that protection still blocks "is reported as approved at the head but still blocked by protection". - Tests: held() replaces every remaining pull(mergeable_state="blocked"), and the second-reviewer case names its two reviewers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQrzw4VG1wfzDPFtKCgnay
|
CodyCBakerPhD asked for the nits after all, so 2827274 takes all three:
The 93 unit tests and the four tox gates pass. On a clean tree the scanner gives the same 3 findings as master. The daily sweep is re-pinned to this commit. Generated by Claude Code |
|
@yarikoptic I assume this would be preferred over the concerns of #39 ? |
|
I wonder if there could be some form of interactive approval, and hence have smth like |
|
The entire point of my bot sweep here is full automation without interaction |
The review asked for a way to see and confirm approvals before the sweep gives them; the sweep's own scheduled use needs none. --approve-mode covers both: `auto` approves every eligible PR as before, and `ask`, the default, asks the user once per repository after the step 3 question. Declined PRs and PRs with no one to ask are recorded as the new outcome `approve-skipped` and reported under Blocked as not approved. Step 7 confirms the approval mode with the other standing answers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQrzw4VG1wfzDPFtKCgnay
yarikoptic-gitmate
left a comment
There was a problem hiding this comment.
Thanks @CodyCBakerPhD. Re-checked e07e8ef (delta since 2827274: --approve-mode ask|auto). CI is 4/4 green. Two independent reviews of the delta (skill authoring; gate code, tests and approval safety) found nothing blocking. The approve-skipped result is wired correctly: it is in RESULTS, not in the failures set, lands under Blocked exactly once, and its note goes through sanitize. I read the diff and code; I did not run tox.
Worth fixing (SKILL.md)
- The scheduled-invocation example in step 7 is unchanged. With
askas the default, a copy-pasted schedule that adds--allow-approvesilently approves nothing. Add a sentence after the example, e.g.: "To approve unattended, add--allow-approve --approve-mode autoto the invocation." Also "needs both answers" is vague, since themajoranswer is always needed and the mode only with--allow-approve. Suggest: "An unattended run cannot ask, so amajorPR stays blocked without the standing answer and, with--allow-approve, nothing is approved under--approve-mode ask." --approve-modewithout--allow-approveis undefined. Say in Inputs that it is ignored, or reject it in thecmd_gateguard the same way--reviewsis (the second keeps the two options symmetric).- Step 5 does not say where the merge happens for PRs approved in the deferred pass. One clause would do: "approved PRs are re-gated and merged in this pass".
Trim (optional)
4. The ask semantics appear in the constraints bullet, Inputs, step 5, the run-order paragraph and step 7. Inputs could shrink to: "ask asks the user before approving (step 5); with no user to ask, nothing is approved. auto approves every "approve": true PR and is what a scheduled run needs." The constraints bullet reads better split in two.
Tests (optional)
5. In test_gate_approve_option the loop gives approve-skipped the detail HEAD, which is not what a skip records. Use (("approved", HEAD), ("approve-skipped", "declined by the user")).
Observation, no change requested: ask is enforced only by SKILL.md prose, so the report cannot show whether an approval was confirmed or automatic. Having the approved outcome detail or the report header name the mode would make that visible; no enforcement logic needed.
Version stays 0.2.0, which is right while this PR is open. Ready to merge from my side once 1 to 3 are settled.
Generated by Claude Code
- Step 7 says why an unattended run needs each standing answer and, after the example, how to approve unattended. - Inputs says --approve-mode is ignored without --allow-approve and never reaches the gate, in a shorter entry. - Step 5 says PRs approved in the deferred `ask` pass are re-gated and merged in that pass. - The never-does bullet about approvals stands on its own. - The CLI test records a skip with a skip's detail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQrzw4VG1wfzDPFtKCgnay
|
Thanks. 432e9ae takes points 1 to 5. The PR description now covers
On your observation: the run's invocation already names the mode, and the report keeps The 93 unit tests and the four tox gates pass. On a clean tree the scanner gives the same 3 findings as master. Generated by Claude Code |
yarikoptic-gitmate
left a comment
There was a problem hiding this comment.
Thanks @CodyCBakerPhD. Re-checked 432e9ae (delta since e07e8ef) with two independent reviews (skill authoring and docs consistency; gate/approval semantics and test design). CI is 4/4 green. Both found nothing blocking and nothing that must change; I read the diff only and did not run tox or the tests.
Points 1 to 5 from my last review are addressed:
- Step 7 now says why an unattended run needs each standing answer and gives the
--allow-approve --approve-mode autoline after the example. - Inputs says
--approve-modeis ignored without--allow-approveand never reaches the gate. Documenting that rather than rejecting it incmd_gateis right, since the gate never sees the flag and a code guard would only add a flag to reject it. - Step 5 says PRs approved in the deferred
askpass are re-gated and merged in that pass. - The
asksemantics are now in Inputs and step 5; the constraints bullet and step 7 only point to them. - The CLI test records
approve-skippedwith a skip's detail.
An unattended run cannot approve without the explicit auto opt-in: the default fails closed.
Optional nits only (no push needed for these alone):
- Add a second
approve-skippedrow withno one to ask (--approve-mode ask)intest_gate_approve_option, since step 5 records that detail for unattended runs. - Step 5: say the re-gate fetches reviews at the current head rather than reusing the saved file ("fetching the PR and its reviews afresh" already implies it).
Ready to merge from my side.
Generated by Claude Code
- Step 5 says the re-gate fetches the reviews afresh, not the saved file that predates the approval. - The CLI test also records the unattended skip's detail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQrzw4VG1wfzDPFtKCgnay
|
Thanks. e686c0e takes both of the fifth review's nits:
The unit tests and the four tox gates pass. On a clean tree the scanner gives the same 3 findings as master. The daily sweep is re-pinned to this commit, with Generated by Claude Code |
|
@yarikoptic I built in a configuration option to either prompt for confirmation or (in my case) go full throttle |
Summary
Adds an opt-in, per-repository
--allow-approveto bot-pr-sweep. When branch protection is the only thing holding back a bot PR whose CI finished green on the head commit, the sweep approves it and then lets GitHub decide whether it can merge. A run-level--approve-mode ask|autosets whether the sweep asks first. This replaces #39's admin bypass; #39 will be closed once this lands. bot-pr-sweep goes from 0.1.0 to 0.2.0, a MINOR bump for an added option and step.Why
In the repositories the first live sweep covered, branch protection requires an approving review. So every green bot PR there gated
blocked, and the sweep could only report it. #39 solved this by merging past branch protection as an administrator. To make that safe, the gate re-checked every protection rule it could read. In review, that was judged to override the protections the repositories impose.Approving keeps every protection in force. The only thing the sweep supplies is a review, and that review is visible on the PR. GitHub then enforces required checks, code owners, approval counts and merge queues itself. Nothing is merged past branch protection, and the gate does not have to re-implement it.
What the option does
Reviews input. The gate reads the PR's reviews (
--reviews, every page; the two flags go together). Each review must point at this PR in this repository. Otherwise the payload counts as wrong and the PR stayspending; an error object is treated the same way.When a PR is eligible. The decision carries
"approve": trueonly when all of these hold:blocked, and branch protection is its one problem (aprotectedflag and a single blocked reason, asupdate_branchworks).A PR that merges as it is gets no approval. With the option on, requested changes also block the merge.
--approve-mode ask|autois a run-level input and never reaches the gate. Withauto, every eligible PR is approved, which is what a scheduled run needs. Withask, the default, the sweep collects the eligible PRs and, after the step 3majorquestion, asks the user once per repository. It approves the PRs confirmed and re-gates and merges them in that pass. Declined PRs, and all of them when there is no one to ask, are recorded asapprove-skippedand reported under Blocked as not approved. Step 7 confirms the mode with the other standing answers, and the scheduled-invocation example says how to approve unattended.Step 5. The sweep approves, pinning the review to the gated commit (
commit_id) so the approval can never cover a commit the gate did not see. The fixed body says the approval is automated and not a human code review. The sweep then re-runs step 2 with the PR and reviews fetched fresh, and merges only on amergeverdict, guarded by the head SHA as before.Still blocked after approval. A PR protection still blocks (more approvals required, a code owner, or approval of the latest push by someone other than its pusher) is reported as approved at the head but still blocked by protection. It is not approved again. Inputs notes that
--allow-branch-updateand--allow-approvedo not combine under the latest-push rule.Ledger and report. New outcomes
approved,approve-failedandapprove-skipped. The report notes each approval and each skip, and failed approvals go under Needs attention.The approval is in the name of the account running the sweep, and SKILL.md says so. The existing
Bash(gh api repos/*/pulls*)entry already covers the review call, and the hardening note now says so.Testing
tests/test_pr_gate.pygrows from 84 to 93 tests. Review scenarios run assubTests. They cover:major, unknown mergeability)tox -e validate,apm,security-static,security-depspasses. On a clean tree the scanner reports the same 3 findings for bot-pr-sweep as on master.--approve-mode autonow makes them:mergeand squash-merged.Notes
🤖 Generated with Claude Code
https://claude.ai/code/session_01DQrzw4VG1wfzDPFtKCgnay