mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(git): create_pr auto-creates a missing PR base branch on origin
open_pr -> GitService.create_pr posted "base": parent straight to GitHub, so
when the parent (an ancestor task's integration branch) was never pushed — a PM
paused before its first push, or the workspace was wiped — GitHub 422'd "base
field invalid" and stranded every child PR. The base-existence fallback added
in 3d9dd298 lived only in create_pull_request, which open_pr never calls.
Add _ensure_base_on_remote and call it in create_pr: if the base branch is
absent on origin, create it off the default branch's tip (preserving the
integration hierarchy) instead of failing; fall back to the default branch only
if that create push itself fails. Covered by 3 new tests.
This commit is contained in:
@@ -2244,6 +2244,62 @@ class GitService(BaseService):
|
|||||||
)
|
)
|
||||||
return await self.push(workspace)
|
return await self.push(workspace)
|
||||||
|
|
||||||
|
async def _ensure_base_on_remote(
|
||||||
|
self,
|
||||||
|
workspace: Path,
|
||||||
|
base_branch: str,
|
||||||
|
project_slug: str,
|
||||||
|
git_token: str,
|
||||||
|
) -> str:
|
||||||
|
"""Ensure the PR base branch exists on origin; create it if missing.
|
||||||
|
|
||||||
|
``open_pr`` (via :meth:`create_pr`) targets an ancestor task's branch
|
||||||
|
— e.g. the cell-PM integration branch — which may never have been
|
||||||
|
pushed (a PM paused before its first push, or the workspace was
|
||||||
|
wiped). GitHub then rejects the PR with 422 "base field invalid".
|
||||||
|
Rather than fail, create the missing base on the remote off the
|
||||||
|
default branch's tip so the PR has a valid base and the integration
|
||||||
|
layering is preserved. Fall back to the default branch only if the
|
||||||
|
create push itself fails.
|
||||||
|
"""
|
||||||
|
default_branch = await self._project_default_branch(project_slug)
|
||||||
|
if base_branch == default_branch:
|
||||||
|
return base_branch
|
||||||
|
ls = await self._run_git(
|
||||||
|
workspace,
|
||||||
|
["ls-remote", "--heads", "origin", base_branch],
|
||||||
|
check=False,
|
||||||
|
token=git_token,
|
||||||
|
)
|
||||||
|
if ls.stdout.strip():
|
||||||
|
return base_branch
|
||||||
|
await self._run_git(
|
||||||
|
workspace,
|
||||||
|
["fetch", "origin", default_branch],
|
||||||
|
check=False,
|
||||||
|
token=git_token,
|
||||||
|
)
|
||||||
|
push = await self._run_git(
|
||||||
|
workspace,
|
||||||
|
["push", "origin", f"origin/{default_branch}:refs/heads/{base_branch}"],
|
||||||
|
check=False,
|
||||||
|
token=git_token,
|
||||||
|
)
|
||||||
|
if push.returncode != 0:
|
||||||
|
self.log.warning(
|
||||||
|
"could not create missing PR base on remote; retargeting to default",
|
||||||
|
base_branch=base_branch,
|
||||||
|
default_branch=default_branch,
|
||||||
|
stderr=(push.stderr or "")[:200],
|
||||||
|
)
|
||||||
|
return default_branch
|
||||||
|
self.log.info(
|
||||||
|
"created missing PR base branch on remote off default",
|
||||||
|
base_branch=base_branch,
|
||||||
|
default_branch=default_branch,
|
||||||
|
)
|
||||||
|
return base_branch
|
||||||
|
|
||||||
async def create_pr(
|
async def create_pr(
|
||||||
self,
|
self,
|
||||||
branch_name: str,
|
branch_name: str,
|
||||||
@@ -2278,6 +2334,15 @@ class GitService(BaseService):
|
|||||||
git_token = await self._get_project_token_or_raise(project.slug)
|
git_token = await self._get_project_token_or_raise(project.slug)
|
||||||
owner, repo = self._parse_github_remote(workspace)
|
owner, repo = self._parse_github_remote(workspace)
|
||||||
|
|
||||||
|
# `open_pr` targets an ancestor task's branch (e.g. the cell-PM
|
||||||
|
# integration branch) that may not exist on origin — a PM paused
|
||||||
|
# before pushing it, or the workspace was wiped. Create it on the
|
||||||
|
# remote off the default branch so GitHub doesn't 422 'base invalid'
|
||||||
|
# and the integration hierarchy is preserved.
|
||||||
|
parent = await self._ensure_base_on_remote(
|
||||||
|
workspace, parent, project.slug, git_token
|
||||||
|
)
|
||||||
|
|
||||||
pr_title = f"[{str(task.id)[:8]}] {task.title}"
|
pr_title = f"[{str(task.id)[:8]}] {task.title}"
|
||||||
pr_body = task.description or ""
|
pr_body = task.description or ""
|
||||||
|
|
||||||
|
|||||||
@@ -294,6 +294,8 @@ async def test_create_pr_returns_pr_dict() -> None:
|
|||||||
_bind(svc, "_get_project_token_or_raise", AsyncMock(return_value="tok"))
|
_bind(svc, "_get_project_token_or_raise", AsyncMock(return_value="tok"))
|
||||||
_bind(svc, "_parse_github_remote", MagicMock(return_value=("acme", "repo")))
|
_bind(svc, "_parse_github_remote", MagicMock(return_value=("acme", "repo")))
|
||||||
_bind(svc, "_record_pr_atomically", AsyncMock())
|
_bind(svc, "_record_pr_atomically", AsyncMock())
|
||||||
|
# parent == default → _ensure_base_on_remote short-circuits (no git call)
|
||||||
|
_bind(svc, "_project_default_branch", AsyncMock(return_value="master"))
|
||||||
|
|
||||||
fake_resp = MagicMock()
|
fake_resp = MagicMock()
|
||||||
fake_resp.is_success = True
|
fake_resp.is_success = True
|
||||||
@@ -321,6 +323,80 @@ async def test_create_pr_raises_when_branch_not_found() -> None:
|
|||||||
await svc.create_pr("missing/branch", parent="master", is_root_pr=False)
|
await svc.create_pr("missing/branch", parent="master", is_root_pr=False)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# _ensure_base_on_remote: create the PR base branch if it's missing on origin
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_ensure_base_creates_missing_base_off_default() -> None:
|
||||||
|
"""Missing base branch is created on origin off the default branch tip."""
|
||||||
|
svc = _service()
|
||||||
|
_bind(svc, "_project_default_branch", AsyncMock(return_value="master"))
|
||||||
|
calls: list[list[str]] = []
|
||||||
|
|
||||||
|
async def fake_run_git(_ws: Path, args: list[str], **_: object) -> MagicMock:
|
||||||
|
calls.append(args)
|
||||||
|
if args[0] == "ls-remote":
|
||||||
|
return MagicMock(stdout="", returncode=0, stderr="") # base absent
|
||||||
|
return MagicMock(stdout="", returncode=0, stderr="")
|
||||||
|
|
||||||
|
_bind(svc, "_run_git", fake_run_git)
|
||||||
|
out = await svc._ensure_base_on_remote(
|
||||||
|
Path("/tmp/ws"), "feature/frontend/abc--def", "roboco", "tok"
|
||||||
|
)
|
||||||
|
assert out == "feature/frontend/abc--def"
|
||||||
|
assert any(
|
||||||
|
a[0] == "push" and a[-1] == "origin/master:refs/heads/feature/frontend/abc--def"
|
||||||
|
for a in calls
|
||||||
|
), calls
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_ensure_base_passthrough_when_present() -> None:
|
||||||
|
"""An existing base branch is returned unchanged with no push."""
|
||||||
|
svc = _service()
|
||||||
|
_bind(svc, "_project_default_branch", AsyncMock(return_value="master"))
|
||||||
|
pushed = False
|
||||||
|
|
||||||
|
async def fake_run_git(_ws: Path, args: list[str], **_: object) -> MagicMock:
|
||||||
|
nonlocal pushed
|
||||||
|
if args[0] == "push":
|
||||||
|
pushed = True
|
||||||
|
if args[0] == "ls-remote":
|
||||||
|
return MagicMock(
|
||||||
|
stdout="sha\trefs/heads/feature/x", returncode=0, stderr=""
|
||||||
|
)
|
||||||
|
return MagicMock(stdout="", returncode=0, stderr="")
|
||||||
|
|
||||||
|
_bind(svc, "_run_git", fake_run_git)
|
||||||
|
out = await svc._ensure_base_on_remote(
|
||||||
|
Path("/tmp/ws"), "feature/x", "roboco", "tok"
|
||||||
|
)
|
||||||
|
assert out == "feature/x"
|
||||||
|
assert pushed is False
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_ensure_base_falls_back_to_default_when_create_fails() -> None:
|
||||||
|
"""If the create push fails, retarget to the default branch (never 422)."""
|
||||||
|
svc = _service()
|
||||||
|
_bind(svc, "_project_default_branch", AsyncMock(return_value="master"))
|
||||||
|
|
||||||
|
async def fake_run_git(_ws: Path, args: list[str], **_: object) -> MagicMock:
|
||||||
|
if args[0] == "ls-remote":
|
||||||
|
return MagicMock(stdout="", returncode=0, stderr="")
|
||||||
|
if args[0] == "push":
|
||||||
|
return MagicMock(stdout="", returncode=1, stderr="denied")
|
||||||
|
return MagicMock(stdout="", returncode=0, stderr="")
|
||||||
|
|
||||||
|
_bind(svc, "_run_git", fake_run_git)
|
||||||
|
out = await svc._ensure_base_on_remote(
|
||||||
|
Path("/tmp/ws"), "feature/x", "roboco", "tok"
|
||||||
|
)
|
||||||
|
assert out == "master"
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# pr_merge: returns merge commit dict
|
# pr_merge: returns merge commit dict
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
Reference in New Issue
Block a user