From dc00deaff085262a9b6d838a041e1cbaa1b46451 Mon Sep 17 00:00:00 2001 From: "Jeffrey (Dongkyu) Kim" Date: Sat, 28 Mar 2026 14:47:30 +0900 Subject: [PATCH] Keep approval automation alive when GitHub merge outcomes vary The merge wrapper now decides success from the merge result itself, then performs branch deletion as a separate best-effort step. This preserves the resolver OMX path when GitHub or PyGithub report merge failures via exceptions or merged=false, while avoiding false conflict handling when only branch cleanup fails. Constraint: Final-verdict automation must keep queueing merge_conflict_resolution jobs without confusing post-merge branch deletion failures for merge conflicts Rejected: Keep delete_branch=True inside the merge call | mixes merge failure with branch cleanup failure and hides the real outcome Rejected: Restrict resolver retries to 405/409 only | misses real merge-endpoint failures surfaced under other GitHub status codes Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep merge success evaluation separate from branch cleanup, and treat any failed merge API outcome as resolver-worthy unless a stronger non-conflict taxonomy is added later Tested: uv run pytest -q tests/test_github.py; uv run pytest -q; uv run ruff check; uv run ty check Not-tested: Live GitHub merge API failure against a real conflicted PR in production --- dani/github.py | 18 ++++++++++++---- tests/test_github.py | 51 ++++++++++++++++++++++++++++++++++++++------ 2 files changed, 59 insertions(+), 10 deletions(-) diff --git a/dani/github.py b/dani/github.py index 5e519a9..ec195c1 100644 --- a/dani/github.py +++ b/dani/github.py @@ -1,5 +1,6 @@ from __future__ import annotations +import logging import os from collections.abc import Callable from typing import Any @@ -10,6 +11,7 @@ from dani.signatures import parse_signature TOKEN_ENV_VARS = ("DANI_GITHUB_TOKEN", "GITHUB_TOKEN", "GH_TOKEN", "GITHUB_PAT") +LOGGER = logging.getLogger(__name__) class MergeConflictError(RuntimeError): @@ -128,8 +130,16 @@ def ensure_pull_request( def merge_pull_request(self, repo_full_name: str, pr_number: int) -> None: pull_request = self._repo(repo_full_name).get_pull(pr_number) try: - pull_request.merge(merge_method="merge", delete_branch=True) + merge_status = pull_request.merge(merge_method="merge", delete_branch=False) except GithubException as exc: - if exc.status in {405, 409}: - raise MergeConflictError(repo_full_name, pr_number, status=exc.status, message=str(exc)) from exc - raise + raise MergeConflictError(repo_full_name, pr_number, status=exc.status, message=str(exc)) from exc + if not merge_status.merged: + raise MergeConflictError(repo_full_name, pr_number, message=merge_status.message) + try: + pull_request.delete_branch() + except Exception: + LOGGER.warning( + "Pull request merged but failed to delete head branch", + extra={"repo_full_name": repo_full_name, "pr_number": pr_number}, + exc_info=True, + ) diff --git a/tests/test_github.py b/tests/test_github.py index 445b707..81292f5 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -31,6 +31,12 @@ def __init__(self, ref: str) -> None: self.ref = ref +class FakeMergeStatus: + def __init__(self, *, merged: bool = True, message: str = "merged") -> None: + self.merged = merged + self.message = message + + class FakePullRequest: def __init__( self, @@ -51,6 +57,9 @@ def __init__( self.edits: list[dict[str, str]] = [] self.merged = False self.merge_exception: Exception | None = None + self.merge_status = FakeMergeStatus() + self.delete_branch_exception: Exception | None = None + self.delete_branch_calls = 0 self._refresh_raw_data() def _refresh_raw_data(self) -> None: @@ -76,12 +85,18 @@ def edit(self, *, title: str, body: str, base: str) -> None: self.edits.append({"title": title, "body": body, "base": base}) self._refresh_raw_data() - def merge(self, *, merge_method: str, delete_branch: bool) -> None: + def merge(self, *, merge_method: str, delete_branch: bool) -> FakeMergeStatus: assert merge_method == "merge" - assert delete_branch is True + assert delete_branch is False if self.merge_exception is not None: raise self.merge_exception - self.merged = True + self.merged = self.merge_status.merged + return self.merge_status + + def delete_branch(self) -> None: + self.delete_branch_calls += 1 + if self.delete_branch_exception is not None: + raise self.delete_branch_exception class FakeRepo: @@ -186,8 +201,10 @@ def test_get_pull_request_returns_raw_pull_request_payload(fake_repo: FakeRepo) assert pull_request["base"]["ref"] == "dev" -@pytest.mark.parametrize("status", [405, 409]) -def test_merge_pull_request_raises_merge_conflict_error_for_conflicts(fake_repo: FakeRepo, status: int) -> None: +@pytest.mark.parametrize("status", [405, 409, 422]) +def test_merge_pull_request_raises_merge_conflict_error_for_merge_api_failures( + fake_repo: FakeRepo, status: int +) -> None: fake_repo.pulls[7].merge_exception = GithubException( status, {"message": "Base branch was modified. Review and try the merge again."}, @@ -195,5 +212,27 @@ def test_merge_pull_request_raises_merge_conflict_error_for_conflicts(fake_repo: ) github = GitHubCLI(token="unit-test-token", client_factory=lambda _token: FakeClient(fake_repo)) - with pytest.raises(MergeConflictError): + with pytest.raises(MergeConflictError) as exc_info: github.merge_pull_request("acme/demo", 7) + + assert exc_info.value.status == status + + +def test_merge_pull_request_raises_merge_conflict_error_when_merge_status_is_false(fake_repo: FakeRepo) -> None: + fake_repo.pulls[7].merge_status = FakeMergeStatus(merged=False, message="Pull Request is not mergeable") + github = GitHubCLI(token="unit-test-token", client_factory=lambda _token: FakeClient(fake_repo)) + + with pytest.raises(MergeConflictError, match="Pull Request is not mergeable"): + github.merge_pull_request("acme/demo", 7) + + assert fake_repo.pulls[7].delete_branch_calls == 0 + + +def test_merge_pull_request_allows_branch_delete_failure_after_success(fake_repo: FakeRepo) -> None: + fake_repo.pulls[7].delete_branch_exception = RuntimeError("branch delete failed") + github = GitHubCLI(token="unit-test-token", client_factory=lambda _token: FakeClient(fake_repo)) + + github.merge_pull_request("acme/demo", 7) + + assert fake_repo.pulls[7].merged is True + assert fake_repo.pulls[7].delete_branch_calls == 1