feat: add automatic pull request merging support#97
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces automatic pull request merging functionality to the Repository class, including helper methods to validate PR titles, check approval states (via reviews or marker comments), and enforce a minimum age guard. Feedback on these changes highlights a logic flaw where a PR could be merged via a marker comment despite an active CHANGES_REQUESTED block, a potential crash in _head_commit_date if the commits list is empty, and the risk of aborting the entire batch of auto-merges on unexpected exceptions other than requests.HTTPError.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Introduce automatic pull request merging capability (`auto_merge_pull_requests`) to class `Repository` in `github_rest_api/github.py` along with support functions, validating author/reviewer lists, title Conventional-Commit types, head commit minimum age, and clean mergeable state. Add comprehensive test coverage in `tests/test_github.py` and bump package version to 0.44.0.
dclong
force-pushed
the
push-zqwzknyuwluk
branch
from
3565a5d to
c4f19f2
Compare
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements an automatic pull request merging feature (auto_merge_pull_requests) with checks for author/approver allowlists, Conventional-Commits title types, and commit age. Feedback on the changes includes simplifying the ISO-8601 parsing by leveraging Python 3.12's native support for the 'Z' suffix, normalizing GitHub usernames to lowercase to prevent case-sensitivity issues, and adding unit tests to cover the asynchronous retry logic in _settle_mergeable_state.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
dclong
force-pushed
the
push-zqwzknyuwluk
branch
from
4f037ed to
c30c646
Compare
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
dclong
force-pushed
the
push-zqwzknyuwluk
branch
from
c30c646 to
17fbd4a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces automatic pull request merging functionality to the Repository class, including helper functions to validate PR authors, title types, and approval signals (formal reviews or marker comments). It also adds retry logic to wait for GitHub to compute the mergeable state and includes comprehensive unit tests. The reviewer feedback suggests improving robustness by handling timezone-naive datetime strings in _old_enough to prevent a potential TypeError, and wrapping nested dictionary access in _head_commit_date with a try-except block to handle unexpected API payloads gracefully.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
dclong
force-pushed
the
push-zqwzknyuwluk
branch
from
54fa379 to
1250afc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces automatic pull request merging functionality to the Repository class, including eligibility checks based on author, title type, age, mergeable state, and approval. The reviewer provided valuable feedback, identifying a security risk where marker comments could approve outdated commits, a bug where failed merges would be treated as successful due to a missing raise_for_status() call, and a robustness improvement to handle case-insensitive reviewer logins.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
dclong
force-pushed
the
push-zqwzknyuwluk
branch
from
a5a74d5 to
2d3c760
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces automatic pull request merging functionality to the Repository class, enabling open PRs that meet specific criteria (such as trusted author, eligible title type, clean mergeable state, and approval via reviews or marker comments) to be merged automatically. It also adds comprehensive unit tests for these new features. The reviewer's feedback highlights several performance and robustness improvements, specifically recommending that the authors and approvers lists be pre-normalized into sets outside of loops to avoid redundant processing, and that raise_for_status() be called in _head_commit_date to handle HTTP errors gracefully before parsing the JSON response.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def _field_gate_failure( | ||
| pr: dict[str, Any], authors: Sequence[str], allowed_types: Sequence[str] | ||
| ) -> str | None: | ||
| """Return why a PR fails the field-only auto-merge gates, or None if it passes. | ||
|
|
||
| These gates (not a draft, trusted author, eligible title type) read only | ||
| fields carried by both the list and single-PR payloads, so they can filter | ||
| list items before the more expensive single-PR detail is fetched. | ||
|
|
||
| :param pr: A PR payload (a list item or full detail). | ||
| :param authors: Logins whose PRs are eligible for auto-merge. | ||
| :param allowed_types: Conventional-Commits title types eligible for auto-merge. | ||
| """ | ||
| if pr.get("draft"): | ||
| return "it is a draft" | ||
| if not _in_allowlist(_login(pr), _normalized_logins(authors)): | ||
| return "its author is not in the allowlist" | ||
| if not _title_type_allowed(pr.get("title") or "", allowed_types): | ||
| return "its title type is not auto-merge eligible" | ||
| return None |
There was a problem hiding this comment.
The helper function _field_gate_failure currently calls _normalized_logins(authors) internally. Since _field_gate_failure is called inside a loop over all open pull requests in auto_merge_pull_requests, this results in redundant normalization of the authors list on every iteration.
To align with the design of _normalized_logins (which is intended to normalize the allowlist once), update _field_gate_failure to accept a pre-normalized set[str] of authors, while maintaining backwards compatibility for sequences.
def _field_gate_failure(
pr: dict[str, Any],
authors: Sequence[str] | set[str],
allowed_types: Sequence[str],
) -> str | None:
"""Return why a PR fails the field-only auto-merge gates, or None if it passes.
These gates (not a draft, trusted author, eligible title type) read only
fields carried by both the list and single-PR payloads, so they can filter
list items before the more expensive single-PR detail is fetched.
:param pr: A PR payload (a list item or full detail).
:param authors: Logins whose PRs are eligible for auto-merge (can be pre-normalized set).
:param allowed_types: Conventional-Commits title types eligible for auto-merge.
"""
if pr.get("draft"):
return "it is a draft"
authors_set = authors if isinstance(authors, set) else _normalized_logins(authors)
if not _in_allowlist(_login(pr), authors_set):
return "its author is not in the allowlist"
if not _title_type_allowed(pr.get("title") or "", allowed_types):
return "its title type is not auto-merge eligible"
return None| def _approver_review_states( | ||
| reviews: Sequence[dict[str, Any]], approvers: Sequence[str] | ||
| ) -> set[str]: | ||
| """Return the set of latest decisive review states among allowlisted reviewers. | ||
|
|
||
| Only the latest decisive review per reviewer is kept. ``APPROVED``, | ||
| ``CHANGES_REQUESTED`` and ``DISMISSED`` are decisive (so a later | ||
| ``DISMISSED`` clears an earlier approval), while ``COMMENTED`` and | ||
| ``PENDING`` are ignored (a comment does not revoke an approval). | ||
|
|
||
| Returning the set (rather than a single approved/not-approved boolean) lets | ||
| the caller treat ``CHANGES_REQUESTED`` as a veto independently of how | ||
| approval is granted, so a marker comment cannot override a standing change | ||
| request. | ||
|
|
||
| :param reviews: Reviews as returned by ``get_pull_request_reviews``. | ||
| :param approvers: Logins whose reviews are trusted for auto-merge. | ||
| """ | ||
| approver_set = _normalized_logins(approvers) | ||
| # Reviews are returned in chronological order, so a later decisive entry | ||
| # overrides an earlier one for the same reviewer. | ||
| latest: dict[str, str] = {} | ||
| for review in reviews: | ||
| login = _login(review) | ||
| state = review.get("state") | ||
| if _in_allowlist(login, approver_set) and state in ( | ||
| "APPROVED", | ||
| "CHANGES_REQUESTED", | ||
| "DISMISSED", | ||
| ): | ||
| latest[login.lower()] = state | ||
| return set(latest.values()) | ||
|
|
There was a problem hiding this comment.
Similarly, _approver_review_states can be updated to accept a pre-normalized set[str] of approvers to avoid redundant normalization inside the function.
def _approver_review_states(
reviews: Sequence[dict[str, Any]], approvers: Sequence[str] | set[str]
) -> set[str]:
"""Return the set of latest decisive review states among allowlisted reviewers.
Only the latest decisive review per reviewer is kept. ``APPROVED``,
``CHANGES_REQUESTED`` and ``DISMISSED`` are decisive (so a later
``DISMISSED`` clears an earlier approval), while ``COMMENTED`` and
``PENDING`` are ignored (a comment does not revoke an approval).
Returning the set (rather than a single approved/not-approved boolean) lets
the caller treat ``CHANGES_REQUESTED`` as a veto independently of how
approval is granted, so a marker comment cannot override a standing change
request.
:param reviews: Reviews as returned by ``get_pull_request_reviews``.
:param approvers: Logins whose reviews are trusted for auto-merge (can be pre-normalized set).
"""
approver_set = (
approvers if isinstance(approvers, set) else _normalized_logins(approvers)
)
# Reviews are returned in chronological order, so a later decisive entry
# overrides an earlier one for the same reviewer.
latest: dict[str, str] = {}
for review in reviews:
login = _login(review)
state = review.get("state")
if _in_allowlist(login, approver_set) and state in (
"APPROVED",
"CHANGES_REQUESTED",
"DISMISSED",
):
latest[login.lower()] = state
return set(latest.values())| def _marker_approved( | ||
| comments: Sequence[dict[str, Any]], | ||
| approvers: Sequence[str], | ||
| marker: str, | ||
| since: datetime, | ||
| ) -> bool: | ||
| """Check whether an allowlisted approver marked the current head for merge. | ||
|
|
||
| Only comments created after ``since`` (the head commit's timestamp) count: a | ||
| marker left before the current head commit approved an older revision, so | ||
| honouring it would auto-merge newer, unreviewed changes. | ||
|
|
||
| :param comments: Issue comments as returned by ``get_issue_comments``. | ||
| :param approvers: Logins whose marker comments are trusted for auto-merge. | ||
| :param marker: The marker substring signalling approval. | ||
| :param since: The head commit's timestamp; earlier comments are ignored. | ||
| """ | ||
| approver_set = _normalized_logins(approvers) | ||
| return any( | ||
| _in_allowlist(_login(comment), approver_set) | ||
| and marker in (comment.get("body") or "") | ||
| and _parse_iso(comment["created_at"]) > since | ||
| for comment in comments | ||
| ) |
There was a problem hiding this comment.
Update _marker_approved to accept a pre-normalized set[str] of approvers to avoid redundant normalization.
def _marker_approved(
comments: Sequence[dict[str, Any]],
approvers: Sequence[str] | set[str],
marker: str,
since: datetime,
) -> bool:
"""Check whether an allowlisted approver marked the current head for merge.
Only comments created after ``since`` (the head commit's timestamp) count: a
marker left before the current head commit approved an older revision, so
honouring it would auto-merge newer, unreviewed changes.
:param comments: Issue comments as returned by ``get_issue_comments``.
:param approvers: Logins whose marker comments are trusted for auto-merge (can be pre-normalized set).
:param marker: The marker substring signalling approval.
:param since: The head commit's timestamp; earlier comments are ignored.
"""
approver_set = (
approvers if isinstance(approvers, set) else _normalized_logins(approvers)
)
return any(
_in_allowlist(_login(comment), approver_set)
and marker in (comment.get("body") or "")
and _parse_iso(comment["created_at"]) > since
for comment in comments
)| pr = self.get_pull_request(pr_number) | ||
|
|
||
| def skip(reason: str) -> None: | ||
| logger.info("Skipping auto-merge of PR #%s: %s.", pr_number, reason) | ||
| return None | ||
|
|
||
| # Field-only gates first; these need no extra requests. | ||
| field_failure = _field_gate_failure(pr, authors, allowed_types) | ||
| if field_failure: | ||
| return skip(field_failure) | ||
| # Check age before mergeable_state: one commit-date fetch is cheaper than | ||
| # settling mergeable_state, and mergeable_state is only trustworthy once | ||
| # the PR is old enough for its checks to have registered. | ||
| committed = self._head_commit_date(pr["head"]["sha"]) | ||
| if not _old_enough(committed, min_age_minutes): | ||
| return skip(f"its head commit is newer than {min_age_minutes} minutes") | ||
| pr = self._settle_mergeable_state(pr) | ||
| if pr.get("mergeable_state") != "clean": | ||
| return skip(f"its mergeable_state is {pr.get('mergeable_state')!r}") | ||
| # The head SHA is read after settling so it matches the mergeable_state | ||
| # just validated, and is returned so the caller can pin the merge to it. | ||
| head_sha = pr["head"]["sha"] | ||
| reviews = self.get_pull_request_reviews(pr_number) | ||
| review_states = _approver_review_states(reviews, approvers) | ||
| # A standing change request vetoes the merge regardless of how approval | ||
| # would otherwise be granted, so it is checked before the marker path. | ||
| if "CHANGES_REQUESTED" in review_states: | ||
| return skip("an approver has requested changes") | ||
| if "APPROVED" in review_states: | ||
| return head_sha | ||
| comments = self.get_issue_comments(pr_number) | ||
| if _marker_approved(comments, approvers, marker, _parse_iso(committed)): | ||
| return head_sha | ||
| return skip("it lacks an approving review or marker comment") |
There was a problem hiding this comment.
Update should_auto_merge to pre-normalize authors and approvers into sets, and pass these pre-normalized sets to the helper functions.
pr = self.get_pull_request(pr_number)
def skip(reason: str) -> None:
logger.info("Skipping auto-merge of PR #%s: %s.", pr_number, reason)
return None
authors_set = _normalized_logins(authors)
approver_set = _normalized_logins(approvers)
# Field-only gates first; these need no extra requests.
field_failure = _field_gate_failure(pr, authors_set, allowed_types)
if field_failure:
return skip(field_failure)
# Check age before mergeable_state: one commit-date fetch is cheaper than
# settling mergeable_state, and mergeable_state is only trustworthy once
# the PR is old enough for its checks to have registered.
committed = self._head_commit_date(pr["head"]["sha"])
if not _old_enough(committed, min_age_minutes):
return skip(f"its head commit is newer than {min_age_minutes} minutes")
pr = self._settle_mergeable_state(pr)
if pr.get("mergeable_state") != "clean":
return skip(f"its mergeable_state is {pr.get('mergeable_state')!r}")
# The head SHA is read after settling so it matches the mergeable_state
# just validated, and is returned so the caller can pin the merge to it.
head_sha = pr["head"]["sha"]
reviews = self.get_pull_request_reviews(pr_number)
review_states = _approver_review_states(reviews, approver_set)
# A standing change request vetoes the merge regardless of how approval
# would otherwise be granted, so it is checked before the marker path.
if "CHANGES_REQUESTED" in review_states:
return skip("an approver has requested changes")
if "APPROVED" in review_states:
return head_sha
comments = self.get_issue_comments(pr_number)
if _marker_approved(comments, approver_set, marker, _parse_iso(committed)):
return head_sha
return skip("it lacks an approving review or marker comment")| eligible = [] | ||
| for pr in self.get_pull_requests(): | ||
| number = pr["number"] | ||
| # Pre-filter on the list payload so PRs failing a field-only gate are | ||
| # dropped without fetching their (more expensive) single-PR detail. | ||
| field_failure = _field_gate_failure(pr, authors, allowed_types) | ||
| if field_failure: | ||
| logger.info("Skipping auto-merge of PR #%s: %s.", number, field_failure) | ||
| continue |
There was a problem hiding this comment.
Pre-normalize the authors list into a set outside of the loop in auto_merge_pull_requests to avoid redundant normalization on every single open pull request.
| eligible = [] | |
| for pr in self.get_pull_requests(): | |
| number = pr["number"] | |
| # Pre-filter on the list payload so PRs failing a field-only gate are | |
| # dropped without fetching their (more expensive) single-PR detail. | |
| field_failure = _field_gate_failure(pr, authors, allowed_types) | |
| if field_failure: | |
| logger.info("Skipping auto-merge of PR #%s: %s.", number, field_failure) | |
| continue | |
| authors_set = _normalized_logins(authors) | |
| eligible = [] | |
| for pr in self.get_pull_requests(): | |
| number = pr["number"] | |
| # Pre-filter on the list payload so PRs failing a field-only gate are | |
| # dropped without fetching their (more expensive) single-PR detail. | |
| field_failure = _field_gate_failure(pr, authors_set, allowed_types) | |
| if field_failure: | |
| logger.info("Skipping auto-merge of PR #%s: %s.", number, field_failure) | |
| continue |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
Summary
Changed files
Modified
github_rest_api/__init__.py(+9/-1)github_rest_api/github.py(+343/-1)tests/test_github.py(+359/-0)uv.lock(+1/-1)Commits