mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(gateway): PR base/target is the parent task's branch, not derived
submit_up opened the cell->root PR with a base computed by merge_chain.parent_branch_for, which drops the last --segment but REUSES the child's team prefix. Across a team boundary (cell feature/backend/ROOT--CELL -> root feature/main_pm/ROOT) that yields a ref that does not exist on the remote, so GitHub rejects the PR with 422 base: invalid and the cell parent wedges. Resolve the base/target from the parent task's authoritative branch_name (what branch creation already cuts each child from), via a shared VerbRunner._parent_branch_for helper used by _do_create_pr and _do_pr_merge. Falls back to parent_branch_for only when there is no parent (root->master -> master) or it has no branch yet. The leaf->cell path is unchanged (same team). Latent since the merge chain landed; first run to reach cell-PM bubble-up exposed it after #180.
This commit is contained in:
@@ -20,6 +20,7 @@ from __future__ import annotations
|
||||
from collections.abc import Awaitable, Callable
|
||||
from dataclasses import dataclass
|
||||
from typing import Any
|
||||
from uuid import UUID
|
||||
|
||||
from roboco.foundation.policy import lifecycle as spec
|
||||
|
||||
@@ -177,18 +178,38 @@ class VerbRunner:
|
||||
async def _do_push_branch(self, task: Any, _agent: Any) -> Any:
|
||||
return await self.git_service.push_branch(task.branch_name)
|
||||
|
||||
async def _do_create_pr(self, task: Any, _agent: Any) -> Any:
|
||||
async def _parent_branch_for(self, task: Any) -> str:
|
||||
"""Base/target branch for a child→parent PR: the parent task's own
|
||||
branch_name.
|
||||
|
||||
#181: ``merge_chain.parent_branch_for`` derives the parent branch by
|
||||
dropping the last ``--`` segment but REUSING the child's team segment.
|
||||
That only holds within one team. Across a team boundary — every
|
||||
cell→root PR, where the cell is ``feature/backend/…`` but the root is
|
||||
``feature/main_pm/…`` — it yields a branch name that doesn't exist on
|
||||
the remote, and GitHub rejects the PR with ``base: invalid``. The
|
||||
parent task's stored ``branch_name`` is authoritative: branch creation
|
||||
already cuts and pushes each child from it
|
||||
(``TaskService._resolve_parent_branch``). Fall back to string
|
||||
derivation only when there is no parent or it has no branch yet.
|
||||
"""
|
||||
from roboco.services.gateway.merge_chain import parent_branch_for
|
||||
|
||||
parent = parent_branch_for(task.branch_name)
|
||||
parent_id = getattr(task, "parent_task_id", None)
|
||||
if parent_id is not None:
|
||||
parent = await self.task_service.get(UUID(str(parent_id)))
|
||||
if parent is not None and parent.branch_name:
|
||||
return str(parent.branch_name)
|
||||
return parent_branch_for(task.branch_name)
|
||||
|
||||
async def _do_create_pr(self, task: Any, _agent: Any) -> Any:
|
||||
parent = await self._parent_branch_for(task)
|
||||
return await self.git_service.create_pr(
|
||||
task.branch_name, parent=parent, is_root_pr=False
|
||||
)
|
||||
|
||||
async def _do_pr_merge(self, task: Any, agent: Any) -> Any:
|
||||
from roboco.services.gateway.merge_chain import parent_branch_for
|
||||
|
||||
target = parent_branch_for(task.branch_name)
|
||||
target = await self._parent_branch_for(task)
|
||||
return await self.git_service.pr_merge(
|
||||
task.pr_number, target=target, actor_agent_id=agent.id
|
||||
)
|
||||
|
||||
@@ -890,6 +890,7 @@ async def test_submit_up_opens_pr_and_reassigns_to_main_pm() -> None:
|
||||
id=task_id,
|
||||
status="in_progress",
|
||||
assigned_to=pm_id,
|
||||
parent_task_id=None,
|
||||
branch_name="feature/backend/abc123",
|
||||
team="backend",
|
||||
)
|
||||
@@ -897,6 +898,7 @@ async def test_submit_up_opens_pr_and_reassigns_to_main_pm() -> None:
|
||||
id=task_id,
|
||||
status="awaiting_pm_review",
|
||||
assigned_to=pm_id,
|
||||
parent_task_id=None,
|
||||
branch_name="feature/backend/abc123",
|
||||
team="backend",
|
||||
)
|
||||
|
||||
@@ -70,6 +70,10 @@ async def test_open_pr_pushes_and_opens_pr() -> None:
|
||||
plan="x",
|
||||
commits=[{"sha": "abc"}],
|
||||
pr_number=None,
|
||||
# No parent → _do_create_pr falls back to parent_branch_for; this
|
||||
# test asserts push+create mechanics, not the cell→root base (#181,
|
||||
# covered in test_verb_runner).
|
||||
parent_task_id=None,
|
||||
branch_name="feature/backend/abc12345",
|
||||
)
|
||||
# Re-fetched task post-runner has the new pr_number written by
|
||||
@@ -82,6 +86,7 @@ async def test_open_pr_pushes_and_opens_pr() -> None:
|
||||
commits=[{"sha": "abc"}],
|
||||
pr_number=42,
|
||||
pr_url="https://gh/x/42",
|
||||
parent_task_id=None,
|
||||
branch_name="feature/backend/abc12345",
|
||||
)
|
||||
task_svc = AsyncMock()
|
||||
|
||||
@@ -70,6 +70,7 @@ async def test_runner_runs_side_effects_after_db_commit() -> None:
|
||||
status="in_progress",
|
||||
commits=["abc"],
|
||||
pr_number=None,
|
||||
parent_task_id=None,
|
||||
branch_name="feature/backend/ABC12345",
|
||||
)
|
||||
agent = MagicMock(id=uuid4(), role="developer")
|
||||
@@ -115,6 +116,7 @@ async def test_submit_up_creates_pr_before_transition() -> None:
|
||||
task = MagicMock(
|
||||
id=uuid4(),
|
||||
status="in_progress",
|
||||
parent_task_id=None,
|
||||
branch_name="feature/backend/ABC12345--DEF67890",
|
||||
)
|
||||
agent = MagicMock(id=uuid4(), role="cell_pm")
|
||||
@@ -126,6 +128,53 @@ async def test_submit_up_creates_pr_before_transition() -> None:
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_pr_base_is_parent_task_branch_across_team() -> None:
|
||||
"""#181: the cell→root PR base is the PARENT task's actual branch_name,
|
||||
not the team-preserving string derivation.
|
||||
|
||||
The cell branch is feature/backend/ROOT--CELL but the root branch is
|
||||
feature/main_pm/ROOT (different team). parent_branch_for() would derive
|
||||
feature/backend/ROOT — a ref that doesn't exist on the remote, which
|
||||
GitHub rejects with base: invalid. The base must come from the parent
|
||||
task's branch_name.
|
||||
"""
|
||||
task_svc = AsyncMock()
|
||||
task_svc.session.begin_nested = MagicMock(
|
||||
return_value=MagicMock(__aenter__=AsyncMock(), __aexit__=AsyncMock())
|
||||
)
|
||||
task_svc.submit_pm_review = AsyncMock(
|
||||
return_value=MagicMock(status="awaiting_pm_review")
|
||||
)
|
||||
root_id = uuid4()
|
||||
# Parent (root) task lives under a DIFFERENT team prefix.
|
||||
task_svc.get = AsyncMock(
|
||||
return_value=MagicMock(branch_name="feature/main_pm/ROOT0001")
|
||||
)
|
||||
|
||||
git_svc = AsyncMock()
|
||||
git_svc.create_pr = AsyncMock(return_value={"pr_number": 32})
|
||||
runner = VerbRunner(task_service=task_svc, git_service=git_svc)
|
||||
|
||||
task = MagicMock(
|
||||
id=uuid4(),
|
||||
status="in_progress",
|
||||
parent_task_id=root_id,
|
||||
branch_name="feature/backend/ROOT0001--CELL0001",
|
||||
)
|
||||
agent = MagicMock(id=uuid4(), role="cell_pm")
|
||||
ctx = spec.Context(notes="cell scope complete; bubbling up to main pm")
|
||||
|
||||
await runner.run_intent("submit_up", task, agent, ctx)
|
||||
|
||||
task_svc.get.assert_awaited_once_with(root_id)
|
||||
_, kwargs = git_svc.create_pr.call_args
|
||||
assert kwargs["parent"] == "feature/main_pm/ROOT0001", (
|
||||
"cell→root PR base must be the parent task's real branch, not the "
|
||||
f"team-derived name; got parent={kwargs['parent']!r}"
|
||||
)
|
||||
|
||||
|
||||
def test_submit_up_spec_pins_pr_before_transition() -> None:
|
||||
"""The submit_up spec declares create_pr as a pre_side_effect, not a
|
||||
trailing side_effect — the ordering fix lives in the spec."""
|
||||
|
||||
Reference in New Issue
Block a user