diff --git a/roboco/services/git.py b/roboco/services/git.py index 4f63b305..063bb242 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -1395,6 +1395,42 @@ class GitService(BaseService): {"owner": owner, "repo": repo, "head": payload.get("head")}, ) from e + async def _pr_base_on_remote( + self, + workspace: Path, + target_branch: str, + default_branch: str, + git_token: str, + task_id: UUID, + ) -> str: + """Return a PR base branch that actually exists on origin. + + GitHub rejects PR creation with 422 "base field invalid" when the base + branch is absent on the remote — which happens when an ancestor task's + branch was claimed but never pushed (e.g. a PM paused before any commit), + stranding every child at PR time. Mirror the branch-cutting fallback in + ``create_branch``: if the resolved base is missing on origin, retarget to + the default branch instead of hard-failing. The default branch is assumed + present, so it is returned unchecked. + """ + if target_branch == default_branch: + return target_branch + ls = await self._run_git( + workspace, + ["ls-remote", "--heads", "origin", target_branch], + check=False, + token=git_token, + ) + if ls.stdout.strip(): + return target_branch + self.log.warning( + "PR base branch not on remote; retargeting PR to the default branch", + base_branch=target_branch, + default_branch=default_branch, + task_id=str(task_id), + ) + return default_branch + async def create_pull_request( self, workspace: Path, request: GitCreatePRRequest ) -> tuple[int, str, str, str, str]: @@ -1414,6 +1450,9 @@ class GitService(BaseService): target_branch = await self._resolve_pr_target_branch( request, task, default_branch ) + target_branch = await self._pr_base_on_remote( + workspace, target_branch, default_branch, git_token, request.task_id + ) pr_title, pr_body = await self._generate_pr_title_body( request, task, source_branch, target_branch, request.task_id ) @@ -1814,6 +1853,45 @@ class GitService(BaseService): owner, repo, branch_name, git_token ) + async def _first_allowed_merge_method( + self, + owner: str, + repo: str, + git_token: str, + *, + exclude: str | None = None, + ) -> str | None: + """Return a merge method the repo permits (squash > merge > rebase), + skipping ``exclude``; ``None`` if the lookup fails. + + Recovers when a merge is refused with 405 because the repo disables that + merge method in its settings. + """ + try: + async with httpx.AsyncClient(timeout=_default_git_timeout()) as client: + resp = await client.get( + f"https://api.github.com/repos/{owner}/{repo}", + headers={ + "Authorization": f"Bearer {git_token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }, + ) + if not resp.is_success: + return None + data = resp.json() + except httpx.HTTPError: + return None + allowed = { + "squash": data.get("allow_squash_merge", True), + "merge": data.get("allow_merge_commit", True), + "rebase": data.get("allow_rebase_merge", True), + } + for method in ("squash", "merge", "rebase"): + if method != exclude and allowed.get(method): + return method + return None + async def merge_pull_request( self, workspace: Path, pr_number: int, merge_method: str, project_slug: str ) -> tuple[str, str]: @@ -1829,6 +1907,26 @@ class GitService(BaseService): resp = await self._call_merge_api( owner, repo, pr_number, git_token, merge_method ) + # A 405 means the repo disallows this merge method (e.g. "Squash merges + # are not allowed on this repository" when that button is off). Fall back + # to a method the repo permits and retry once, so a repo's merge-button + # settings can't permanently wedge the PM on an open, mergeable PR. + if resp.status_code == httpx.codes.METHOD_NOT_ALLOWED: + fallback = await self._first_allowed_merge_method( + owner, repo, git_token, exclude=merge_method + ) + if fallback and fallback != merge_method: + self.log.info( + "Merge method refused by repo; retrying with a permitted one", + requested=merge_method, + fallback=fallback, + owner=owner, + repo=repo, + pr=pr_number, + ) + resp = await self._call_merge_api( + owner, repo, pr_number, git_token, fallback + ) if not resp.is_success: raise GitError( f"GitHub API refused PR merge ({resp.status_code}): {resp.text[:200]}", diff --git a/tests/unit/services/test_git_merge_method_fallback.py b/tests/unit/services/test_git_merge_method_fallback.py new file mode 100644 index 00000000..c65c5595 --- /dev/null +++ b/tests/unit/services/test_git_merge_method_fallback.py @@ -0,0 +1,162 @@ +"""GitService falls back to a permitted merge method when the repo refuses one. + +A repo with squash merges disabled returns 405 ("Squash merges are not allowed") +on the default squash merge. Without a fallback the PM strands on an open, +mergeable PR — code built, QA passed, docs written, one API call from done. +merge_pull_request must look up a method the repo permits and retry once. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any +from unittest.mock import AsyncMock, MagicMock + +import pytest +import roboco.services.git as git_module +from roboco.services.git import GitService + + +def _git_service() -> GitService: + svc = GitService.__new__(GitService) + svc.log = MagicMock() + return svc + + +def _resp( + status_code: int, *, is_success: bool, json_data: dict[str, Any] | None = None +) -> Any: + payload = json_data or {} + return type( + "R", + (), + { + "status_code": status_code, + "is_success": is_success, + "text": "", + "json": lambda _self=None, _p=payload: _p, + }, + )() + + +class _FakeClient: + def __init__(self, resp: Any) -> None: + self._resp = resp + + async def __aenter__(self) -> _FakeClient: + return self + + async def __aexit__(self, *_a: Any) -> bool: + return False + + async def get(self, _url: str, **_kwargs: Any) -> Any: + return self._resp + + +@pytest.mark.asyncio +async def test_first_allowed_skips_disabled_method( + monkeypatch: pytest.MonkeyPatch, +) -> None: + svc = _git_service() + resp = _resp( + 200, + is_success=True, + json_data={ + "allow_squash_merge": False, + "allow_merge_commit": True, + "allow_rebase_merge": True, + }, + ) + monkeypatch.setattr( + git_module.httpx, "AsyncClient", lambda *_a, **_k: _FakeClient(resp) + ) + method = await svc._first_allowed_merge_method("o", "r", "tok", exclude="squash") + assert method == "merge" # squash disabled + excluded -> next permitted + + +@pytest.mark.asyncio +async def test_first_allowed_returns_none_when_lookup_fails( + monkeypatch: pytest.MonkeyPatch, +) -> None: + svc = _git_service() + resp = _resp(403, is_success=False) + monkeypatch.setattr( + git_module.httpx, "AsyncClient", lambda *_a, **_k: _FakeClient(resp) + ) + assert await svc._first_allowed_merge_method("o", "r", "tok") is None + + +@pytest.mark.asyncio +async def test_merge_retries_with_allowed_method_on_405( + monkeypatch: pytest.MonkeyPatch, +) -> None: + svc = _git_service() + monkeypatch.setattr( + svc, "_get_project_token_or_raise", AsyncMock(return_value="tok") + ) + monkeypatch.setattr(svc, "_parse_github_remote", lambda _ws: ("owner", "repo")) + monkeypatch.setattr( + svc, "_delete_pr_branch_best_effort", AsyncMock(return_value=None) + ) + monkeypatch.setattr( + svc, "_project_default_branch", AsyncMock(return_value="master") + ) + monkeypatch.setattr(svc, "_sync_target_branch", AsyncMock(return_value="abc123")) + monkeypatch.setattr( + svc, "_first_allowed_merge_method", AsyncMock(return_value="merge") + ) + + calls: list[str] = [] + + async def fake_call( + _owner: str, _repo: str, _pr: int, _token: str, method: str + ) -> Any: + calls.append(method) + return ( + _resp(200, is_success=True) + if method == "merge" + else _resp(405, is_success=False) + ) + + monkeypatch.setattr(svc, "_call_merge_api", fake_call) + + target, commit = await svc.merge_pull_request(Path("/tmp/ws"), 7, "squash", "proj") + + assert calls == ["squash", "merge"] # refused squash, retried with merge + assert target == "master" + assert commit == "abc123" + + +@pytest.mark.asyncio +async def test_merge_does_not_retry_when_method_allowed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + svc = _git_service() + monkeypatch.setattr( + svc, "_get_project_token_or_raise", AsyncMock(return_value="tok") + ) + monkeypatch.setattr(svc, "_parse_github_remote", lambda _ws: ("owner", "repo")) + monkeypatch.setattr( + svc, "_delete_pr_branch_best_effort", AsyncMock(return_value=None) + ) + monkeypatch.setattr( + svc, "_project_default_branch", AsyncMock(return_value="master") + ) + monkeypatch.setattr(svc, "_sync_target_branch", AsyncMock(return_value="abc123")) + lookup = AsyncMock(return_value="merge") + monkeypatch.setattr(svc, "_first_allowed_merge_method", lookup) + + calls: list[str] = [] + + async def fake_call( + _owner: str, _repo: str, _pr: int, _token: str, method: str + ) -> Any: + calls.append(method) + return _resp(200, is_success=True) + + monkeypatch.setattr(svc, "_call_merge_api", fake_call) + + await svc.merge_pull_request(Path("/tmp/ws"), 7, "squash", "proj") + + assert calls == ["squash"] # allowed first time, no second call + lookup.assert_not_awaited() # fallback lookup never triggered diff --git a/tests/unit/services/test_git_pr_base_fallback.py b/tests/unit/services/test_git_pr_base_fallback.py new file mode 100644 index 00000000..8311807b --- /dev/null +++ b/tests/unit/services/test_git_pr_base_fallback.py @@ -0,0 +1,77 @@ +"""GitService retargets a PR base to the default branch when it's missing on origin. + +create_pull_request resolves the base from the parent task's branch. If that +branch was never pushed to origin (an ancestor claimed but paused before its +first commit), GitHub rejects creation with 422 "base field invalid" and every +child task strands at open_pr. _pr_base_on_remote mirrors the branch-cutting +fallback in create_branch: ls-remote the resolved base and retarget to the +default branch when it's absent. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from roboco.services.git import GitService + + +def _git_service() -> GitService: + svc = GitService.__new__(GitService) + svc.log = MagicMock() + return svc + + +def _result(stdout: str = "") -> Any: + return type("R", (), {"stdout": stdout, "returncode": 0})() + + +@pytest.mark.asyncio +async def test_retargets_to_default_when_base_absent_on_origin( + monkeypatch: pytest.MonkeyPatch, +) -> None: + svc = _git_service() + monkeypatch.setattr(svc, "_run_git", AsyncMock(return_value=_result(stdout=""))) + + base = await svc._pr_base_on_remote( + Path("/tmp/ws"), "feature/backend/root--parent", "master", "tok", uuid4() + ) + + assert base == "master" + + +@pytest.mark.asyncio +async def test_keeps_base_when_present_on_origin( + monkeypatch: pytest.MonkeyPatch, +) -> None: + svc = _git_service() + monkeypatch.setattr( + svc, + "_run_git", + AsyncMock(return_value=_result(stdout="abc123\trefs/heads/feature/x\n")), + ) + + base = await svc._pr_base_on_remote( + Path("/tmp/ws"), "feature/x", "master", "tok", uuid4() + ) + + assert base == "feature/x" + + +@pytest.mark.asyncio +async def test_noop_when_base_is_default_branch( + monkeypatch: pytest.MonkeyPatch, +) -> None: + svc = _git_service() + run = AsyncMock(return_value=_result(stdout="")) + monkeypatch.setattr(svc, "_run_git", run) + + base = await svc._pr_base_on_remote( + Path("/tmp/ws"), "master", "master", "tok", uuid4() + ) + + assert base == "master" + run.assert_not_awaited() # no remote lookup needed when base is the default