mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(gateway): cell PM completes its own cell task; drop main-PM handoff
submit_up bubbled the cell task to Main PM (_handoff_to_main_pm), but
main_pm_complete rejects any task with a parent_task_id ("only operates
on root tasks"), so the cell->root PR had no one to merge it and the
cell task wedged at awaiting_pm_review. _maybe_advance_parent_to_pm_review
already intends the CELL PM to complete it.
Cell PM now owns cell completion:
- submit_up no longer hands off to Main PM; the cell task stays assigned
to the cell PM, which is respawned to complete() it. Removed the
now-unused _handoff_to_main_pm.
- cell_pm_complete resolves the merge target from the parent task's real
branch_name (shared merge_chain.resolve_parent_branch, also used by the
PR side-effects) so the cell->root PR merges into feature/main_pm/...,
not the team-mis-derived feature/<cellteam>/... (same root cause as the
prior PR-base fix).
- submit_up description + next_hint updated; lifecycle artifacts regen.
Main PM still only completes the ROOT (root->master + escalate-to-CEO).
First run to reach cell-PM bubble-up exposed this.
This commit is contained in:
Renn F
@@ -10,7 +10,7 @@ other verb will be rejected with a Decision telling you the right one.
|
||||
- **i_am_idle**: Signal you have no active work. PMs auto-pause owned in_progress tasks.
|
||||
- **i_will_plan**: PM mirror of i_will_work_on for parent tasks. Claim, plan, transition to in_progress; from there delegate subtasks.
|
||||
- **resume**: Resume a paused task you own. paused -> in_progress.
|
||||
- **submit_up**: Cell PM bubbles a finished cell-scope task up to Main PM.
|
||||
- **submit_up**: Cell PM opens the cell→root PR and moves the cell task to awaiting_pm_review. The same Cell PM then completes it.
|
||||
- **triage**: List actionable tasks in your scope.
|
||||
- **unblock**: PM unblocks a blocked task; restores pre-block state.
|
||||
- **unclaim**: Voluntarily release a claim back to pending. The work-in-progress branch is preserved.
|
||||
|
||||
@@ -167,7 +167,7 @@ Resume a paused task you own. paused -> in_progress.
|
||||
|
||||
## submit_up
|
||||
|
||||
Cell PM bubbles a finished cell-scope task up to Main PM.
|
||||
Cell PM opens the cell→root PR and moves the cell task to awaiting_pm_review. The same Cell PM then completes it.
|
||||
|
||||
**Allowed roles:** cell_pm
|
||||
|
||||
|
||||
@@ -255,7 +255,7 @@
|
||||
"composes": [
|
||||
"submit_pm_review"
|
||||
],
|
||||
"description": "Cell PM bubbles a finished cell-scope task up to Main PM.",
|
||||
"description": "Cell PM opens the cell\u2192root PR and moves the cell task to awaiting_pm_review. The same Cell PM then completes it.",
|
||||
"name": "submit_up",
|
||||
"pre_side_effects": [
|
||||
"create_pr"
|
||||
|
||||
@@ -973,7 +973,10 @@ _INTENT_VERBS: dict[str, IntentSpec] = {
|
||||
"submit_up": IntentSpec(
|
||||
name="submit_up",
|
||||
allowed_roles=frozenset({Role.CELL_PM}),
|
||||
description="Cell PM bubbles a finished cell-scope task up to Main PM.",
|
||||
description=(
|
||||
"Cell PM opens the cell→root PR and moves the cell task to"
|
||||
" awaiting_pm_review. The same Cell PM then completes it."
|
||||
),
|
||||
composes=("submit_pm_review",),
|
||||
extra_preconditions=(),
|
||||
# The cell→root PR must exist BEFORE submit_pm_review runs — its
|
||||
@@ -983,7 +986,9 @@ _INTENT_VERBS: dict[str, IntentSpec] = {
|
||||
# sees pr_created=True. Mirrors the dev's open_pr→i_am_done split.
|
||||
pre_side_effects=("create_pr",),
|
||||
side_effects=(),
|
||||
next_hint=lambda _t: "idle until Main PM reviews",
|
||||
# #182: the Cell PM owns cell completion — it merges the cell→root PR
|
||||
# via complete(). Main PM only completes the ROOT task.
|
||||
next_hint=lambda _t: "complete(task_id) to merge the cell→root PR",
|
||||
),
|
||||
"unblock": IntentSpec(
|
||||
name="unblock",
|
||||
|
||||
@@ -32,7 +32,7 @@ from roboco.services.gateway.evidence_builder import (
|
||||
build_context_briefing,
|
||||
build_evidence_for_task,
|
||||
)
|
||||
from roboco.services.gateway.merge_chain import parent_branch_for
|
||||
from roboco.services.gateway.merge_chain import resolve_parent_branch
|
||||
from roboco.services.gateway.remediation import (
|
||||
hint_for_evidence_not_inspected,
|
||||
hint_for_missing_doc_files,
|
||||
@@ -3386,7 +3386,11 @@ class Choreographer:
|
||||
verb="submit_up",
|
||||
)
|
||||
t = outcome
|
||||
await self._handoff_to_main_pm(pm_agent_id, task_id)
|
||||
# #182: do NOT hand the cell task to Main PM. The cell PM owns cell
|
||||
# completion — it stays assigned to the cell PM, which is respawned to
|
||||
# `complete` the task (merging the cell→root PR). Main PM only
|
||||
# completes the ROOT (root→master + escalate-to-CEO).
|
||||
# `_maybe_advance_parent_to_pm_review` already keeps it on the cell PM.
|
||||
return Envelope.ok(
|
||||
status=str(t.status),
|
||||
task_id=str(task_id),
|
||||
@@ -3492,21 +3496,6 @@ class Choreographer:
|
||||
)
|
||||
return None
|
||||
|
||||
async def _handoff_to_main_pm(self, pm_agent_id: UUID, task_id: UUID) -> None:
|
||||
"""Reassign the task to the Main PM and A2A-notify the handoff."""
|
||||
main_pm = await self.task.main_pm_agent()
|
||||
if main_pm is None:
|
||||
return
|
||||
main_pm_uuid = UUID(str(main_pm.id))
|
||||
await self.task.reassign(task_id, main_pm_uuid)
|
||||
await self.a2a.send(
|
||||
from_agent=pm_agent_id,
|
||||
to_agent=main_pm_uuid,
|
||||
skill="task_management",
|
||||
task_id=task_id,
|
||||
body=f"Cell scope complete for {task_id}. Ready for Main PM review.",
|
||||
)
|
||||
|
||||
async def pm_give_me_work(self, pm_agent_id: UUID) -> Envelope:
|
||||
"""Return the PM's first assigned task in any active status, or idle.
|
||||
|
||||
@@ -3771,7 +3760,11 @@ class Choreographer:
|
||||
task_id=task_id,
|
||||
verb="cell_pm_complete",
|
||||
)
|
||||
target = parent_branch_for(t.branch_name)
|
||||
# #181/#182: resolve the merge target from the PARENT task's real
|
||||
# branch_name. For a leaf this is the cell branch (same team — no
|
||||
# change); for a cell task it is the root branch (feature/main_pm/…),
|
||||
# which parent_branch_for would have mis-derived as feature/<cellteam>/…
|
||||
target = await resolve_parent_branch(t, self.task)
|
||||
merge_result = await self.git.pr_merge(
|
||||
t.pr_number, target=target, actor_agent_id=pm_agent_id
|
||||
)
|
||||
|
||||
@@ -20,7 +20,6 @@ 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
|
||||
|
||||
@@ -178,38 +177,18 @@ 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 _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_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)
|
||||
from roboco.services.gateway.merge_chain import resolve_parent_branch
|
||||
|
||||
parent = await resolve_parent_branch(task, self.task_service)
|
||||
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:
|
||||
target = await self._parent_branch_for(task)
|
||||
from roboco.services.gateway.merge_chain import resolve_parent_branch
|
||||
|
||||
target = await resolve_parent_branch(task, self.task_service)
|
||||
return await self.git_service.pr_merge(
|
||||
task.pr_number, target=target, actor_agent_id=agent.id
|
||||
)
|
||||
|
||||
@@ -12,6 +12,8 @@ Merge chain:
|
||||
from __future__ import annotations
|
||||
|
||||
import re
|
||||
from typing import Any
|
||||
from uuid import UUID
|
||||
|
||||
_TYPES = ("feature", "bug", "chore", "docs", "hotfix")
|
||||
_TYPE_PATTERN = "|".join(_TYPES)
|
||||
@@ -33,7 +35,16 @@ def branch_depth(branch: str) -> int:
|
||||
|
||||
|
||||
def parent_branch_for(branch: str) -> str:
|
||||
"""Return the merge target for `branch`."""
|
||||
"""Return the merge target for `branch` by string surgery.
|
||||
|
||||
NOTE: this REUSES ``branch``'s own team segment for the parent, so it is
|
||||
only correct within a single team. Across a team boundary — every
|
||||
cell→root hop, where the cell is ``feature/backend/…`` but the root is
|
||||
``feature/main_pm/…`` — it yields a non-existent ref. For PR base/target
|
||||
resolution prefer :func:`resolve_parent_branch`, which reads the parent
|
||||
task's real branch_name. This stays as the fallback for the
|
||||
rootless / same-team / diff-base cases.
|
||||
"""
|
||||
if branch == "master":
|
||||
return "master"
|
||||
m = _BRANCH_RE.match(branch)
|
||||
@@ -46,3 +57,21 @@ def parent_branch_for(branch: str) -> str:
|
||||
return "master"
|
||||
parent_segments = "--".join(segments[:-1])
|
||||
return f"{type_}/{team}/{parent_segments}"
|
||||
|
||||
|
||||
async def resolve_parent_branch(task: Any, task_service: Any) -> str:
|
||||
"""Base/target branch for a child→parent PR: the parent task's own
|
||||
branch_name.
|
||||
|
||||
The parent task's stored ``branch_name`` is authoritative — branch
|
||||
creation already cuts and pushes each child from it
|
||||
(``TaskService._resolve_parent_branch``). Unlike :func:`parent_branch_for`
|
||||
it is correct across a team boundary (#181). Falls back to string
|
||||
derivation only when there is no parent or the parent has no branch yet.
|
||||
"""
|
||||
parent_id = getattr(task, "parent_task_id", None)
|
||||
if parent_id is not None:
|
||||
parent = await 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)
|
||||
|
||||
@@ -704,25 +704,6 @@ async def test_submit_up_no_branch_rejected() -> None:
|
||||
assert "no branch" in body["message"]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _handoff_to_main_pm: main_pm_agent returns None (line 1567)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_handoff_to_main_pm_no_main_pm_returns_silently() -> None:
|
||||
pm_id = uuid4()
|
||||
task_id = uuid4()
|
||||
task_svc = AsyncMock()
|
||||
task_svc.main_pm_agent.return_value = None # No main PM in DB
|
||||
deps = _make_deps(task=task_svc)
|
||||
c = Choreographer(deps)
|
||||
# Should silently return without error.
|
||||
await c._handoff_to_main_pm(pm_id, task_id)
|
||||
# reassign + a2a never called.
|
||||
task_svc.reassign.assert_not_called()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _pm_next_hint: each branch (lines 1612-1616)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -251,18 +251,25 @@ async def test_unblock_restore_false_returns_legacy_message() -> None:
|
||||
async def test_cell_pm_complete_merges_then_completes() -> None:
|
||||
pm_id = uuid4()
|
||||
task_id = uuid4()
|
||||
parent_id = uuid4()
|
||||
t = MagicMock(
|
||||
id=task_id,
|
||||
status="awaiting_pm_review",
|
||||
assigned_to=pm_id,
|
||||
pr_number=8,
|
||||
branch_name="feature/backend/abc--def",
|
||||
parent_task_id=uuid4(),
|
||||
parent_task_id=parent_id,
|
||||
team="backend",
|
||||
)
|
||||
after = MagicMock(**{**t.__dict__, "status": "completed"})
|
||||
# #181/#182: the merge target is the PARENT task's real branch_name —
|
||||
# here under a DIFFERENT team prefix, which the old parent_branch_for
|
||||
# would have mis-derived as feature/backend/abc.
|
||||
parent = MagicMock(
|
||||
id=parent_id, branch_name="feature/main_pm/abc", parent_task_id=None
|
||||
)
|
||||
task_svc = AsyncMock()
|
||||
task_svc.get.return_value = t
|
||||
task_svc.get.side_effect = lambda tid: parent if tid == parent_id else t
|
||||
task_svc.all_subtasks_terminal.return_value = True
|
||||
task_svc.cell_pm_complete.return_value = after
|
||||
git_svc = AsyncMock()
|
||||
@@ -278,7 +285,7 @@ async def test_cell_pm_complete_merges_then_completes() -> None:
|
||||
assert env.error is None
|
||||
assert env.status == "completed"
|
||||
git_svc.pr_merge.assert_awaited_once_with(
|
||||
8, target="feature/backend/abc", actor_agent_id=pm_id
|
||||
8, target="feature/main_pm/abc", actor_agent_id=pm_id
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -882,10 +882,13 @@ async def test_delegate_invalid_team_enum_rejected() -> None:
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_submit_up_opens_pr_and_reassigns_to_main_pm() -> None:
|
||||
async def test_submit_up_opens_pr_and_keeps_cell_pm_assignment() -> None:
|
||||
"""#182: submit_up opens the cell→root PR and moves the cell task to
|
||||
awaiting_pm_review, but does NOT hand it off to Main PM. The cell PM
|
||||
owns cell completion (it is respawned to `complete` the task), so the
|
||||
assignment stays put — no reassign."""
|
||||
pm_id = uuid4()
|
||||
task_id = uuid4()
|
||||
main_pm_id = uuid4()
|
||||
t = MagicMock(
|
||||
id=task_id,
|
||||
status="in_progress",
|
||||
@@ -907,7 +910,6 @@ async def test_submit_up_opens_pr_and_reassigns_to_main_pm() -> None:
|
||||
task_svc.agent_for.return_value = MagicMock(role="cell_pm", team="backend")
|
||||
task_svc.all_subtasks_terminal.return_value = True
|
||||
task_svc.submit_pm_review.return_value = after
|
||||
task_svc.main_pm_agent.return_value = MagicMock(id=main_pm_id)
|
||||
git_svc = AsyncMock()
|
||||
git_svc.create_pr.return_value = {"pr_number": 12, "pr_url": "x"}
|
||||
journal_svc = AsyncMock()
|
||||
@@ -922,7 +924,7 @@ async def test_submit_up_opens_pr_and_reassigns_to_main_pm() -> None:
|
||||
assert env.error is None
|
||||
assert env.status == "awaiting_pm_review"
|
||||
git_svc.create_pr.assert_awaited_once()
|
||||
task_svc.reassign.assert_awaited_once()
|
||||
task_svc.reassign.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
@@ -438,12 +438,15 @@ async def test_cell_pm_complete_reassigns_parent_when_all_subtasks_done() -> Non
|
||||
id=parent_id,
|
||||
team="backend",
|
||||
parent_task_id=None,
|
||||
branch_name="feature/backend/abc",
|
||||
)
|
||||
|
||||
task_svc = AsyncMock()
|
||||
# First .get is for the leaf (status check), then for the parent walk-up.
|
||||
task_svc.get.side_effect = [leaf, parent]
|
||||
task_svc.all_subtasks_terminal.side_effect = [True, True]
|
||||
# get(leaf) for the status check, get(parent) for the merge-target
|
||||
# resolution (#181/#182) and the parent walk-up. Keyed by id so it is
|
||||
# robust to call count/order.
|
||||
task_svc.get.side_effect = lambda tid: parent if tid == parent_id else leaf
|
||||
task_svc.all_subtasks_terminal.return_value = True
|
||||
task_svc.cell_pm_complete.return_value = after
|
||||
task_svc.cell_pm_for_team.return_value = MagicMock(id=new_pm_id)
|
||||
|
||||
|
||||
@@ -2,8 +2,15 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.services.gateway.merge_chain import branch_depth, parent_branch_for
|
||||
from roboco.services.gateway.merge_chain import (
|
||||
branch_depth,
|
||||
parent_branch_for,
|
||||
resolve_parent_branch,
|
||||
)
|
||||
|
||||
_DEPTH_ROOT = 1
|
||||
_DEPTH_ONE_SUBTASK = 2
|
||||
@@ -44,6 +51,47 @@ class TestParentBranchFor:
|
||||
parent_branch_for("not-a-branch")
|
||||
|
||||
|
||||
class TestResolveParentBranch:
|
||||
"""#181/#182: base/target comes from the parent TASK's branch_name, which
|
||||
is correct across a team boundary; parent_branch_for is the fallback."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_uses_parent_task_branch_across_team(self) -> None:
|
||||
root_id = uuid4()
|
||||
task = MagicMock(
|
||||
parent_task_id=root_id,
|
||||
branch_name="feature/backend/ROOT0001--CELL0001",
|
||||
)
|
||||
task_service = AsyncMock()
|
||||
# Root lives under a DIFFERENT team prefix than the cell.
|
||||
task_service.get = AsyncMock(
|
||||
return_value=MagicMock(branch_name="feature/main_pm/ROOT0001")
|
||||
)
|
||||
result = await resolve_parent_branch(task, task_service)
|
||||
assert result == "feature/main_pm/ROOT0001"
|
||||
task_service.get.assert_awaited_once_with(root_id)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_falls_back_when_no_parent(self) -> None:
|
||||
task = MagicMock(parent_task_id=None, branch_name="feature/backend/ROOT0001")
|
||||
task_service = AsyncMock()
|
||||
# No parent → root→master.
|
||||
assert await resolve_parent_branch(task, task_service) == "master"
|
||||
task_service.get.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_falls_back_when_parent_has_no_branch(self) -> None:
|
||||
task = MagicMock(
|
||||
parent_task_id=uuid4(),
|
||||
branch_name="feature/backend/ROOT0001--CELL0001",
|
||||
)
|
||||
task_service = AsyncMock()
|
||||
task_service.get = AsyncMock(return_value=MagicMock(branch_name=None))
|
||||
# Parent exists but has no branch yet → string derivation.
|
||||
result = await resolve_parent_branch(task, task_service)
|
||||
assert result == "feature/backend/ROOT0001"
|
||||
|
||||
|
||||
def test_branch_depth_master_is_zero() -> None:
|
||||
"""Line 28: master returns 0."""
|
||||
assert branch_depth("master") == 0
|
||||
|
||||
Reference in New Issue
Block a user