mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(gateway): let a dev idle past lane-held code-queue siblings
Per-dev sequenced queues (the prior commit) have a PM delegate a dev's whole code queue up front, so a dev owns several pending, assigned-but-unclaimed code leaves at once (seq0 + seq2). The orchestrator's lane barrier holds the seq2 SPAWN while seq0 is non-terminal — but _pending_assignment_guard rejected i_am_idle for ANY pending assigned task, with no lane awareness. So a dev whose current leaf just moved to QA (awaiting_qa) could neither idle (guard rejects) nor proceed cleanly: it was steered to claim seq2 early (the claim path has no lane/sequence check, since delegate sets `sequence` not `dependency_ids`), jumping its own queue order, or it looped on the rejection. An adversarial review of the queue work surfaced this; it is latent until PMs actually delegate multi-item per-dev queues, so the green suite hid it. Fix: TaskService.has_earlier_incomplete_code_sibling mirrors the orchestrator's lane barrier in the service layer; _pending_assignment_guard now drops a dev's lane-held pending code leaves (via _pending_blocking_idle / _pending_not_lane_held) so the dev idles cleanly and the orchestrator spawns the next queue item when the lane clears — preserving one-leaf-at-a-time, in order. `is not True` keeps it inert under partial test mocks. Tests cover the service primitive (live / terminal / higher-seq / non-code / missing-field) and the guard (dev idles when lane-held; still blocks a non-lane-held pending leaf). Full mypy + xenon green.
This commit is contained in:
@@ -2928,13 +2928,8 @@ class Choreographer:
|
||||
if not pending:
|
||||
return None
|
||||
agent = await self.task.agent_for(agent_id)
|
||||
# Board/advisory roles (product_owner, head_marketing, auditor) review
|
||||
# and advise without ever claiming — they have no i_will_work_on /
|
||||
# i_will_plan verb. Their one-shot board dispatch is meant to leave the
|
||||
# coordination task pending for the CEO to reassign to Main PM, so they
|
||||
# must be allowed to idle after recording their review. Without this
|
||||
# they wedge: the gate would demand a claim verb the role does not have.
|
||||
if agent and agent.role in ("product_owner", "head_marketing", "auditor"):
|
||||
pending = await self._pending_blocking_idle(agent, pending)
|
||||
if not pending:
|
||||
return None
|
||||
first = pending[0]
|
||||
verb = (
|
||||
@@ -2954,6 +2949,41 @@ class Choreographer:
|
||||
context_briefing=briefing,
|
||||
)
|
||||
|
||||
async def _pending_blocking_idle(self, agent: Any, pending: list[Any]) -> list[Any]:
|
||||
"""Pending tasks that should block i_am_idle, after role exemptions.
|
||||
|
||||
Board/advisory roles (product_owner, head_marketing, auditor) review and
|
||||
advise without ever claiming — they have no i_will_work_on / i_will_plan
|
||||
verb, and their one-shot dispatch is meant to leave the coordination task
|
||||
pending for the CEO to reassign. They must idle freely, so nothing blocks
|
||||
them. Developers own a whole per-dev code queue up front, so a leaf still
|
||||
waiting behind an earlier non-terminal sibling in its own lane must not
|
||||
pin them — the orchestrator spawns it when the lane clears (see
|
||||
``_pending_not_lane_held``). Other roles: every pending task blocks.
|
||||
"""
|
||||
if not agent:
|
||||
return pending
|
||||
if agent.role in ("product_owner", "head_marketing", "auditor"):
|
||||
return []
|
||||
if agent.role == "developer":
|
||||
return await self._pending_not_lane_held(pending)
|
||||
return pending
|
||||
|
||||
async def _pending_not_lane_held(self, pending: list[Any]) -> list[Any]:
|
||||
"""Drop a dev's pending code leaves that are waiting behind an earlier
|
||||
non-terminal sibling in the same dev's lane (per-dev sequenced queues).
|
||||
|
||||
Those leaves are spawned by the orchestrator when the lane clears, so
|
||||
they must not pin the dev to idle. ``is not True`` keeps this inert under
|
||||
partial test mocks (an AsyncMock returns a truthy stub, not a real bool)
|
||||
— only a leaf the service positively confirms is lane-held is dropped.
|
||||
"""
|
||||
live: list[Any] = []
|
||||
for t in pending:
|
||||
if await self.task.has_earlier_incomplete_code_sibling(t) is not True:
|
||||
live.append(t)
|
||||
return live
|
||||
|
||||
async def _pm_unfinished_review_guard(
|
||||
self, agent_id: UUID, briefing: dict[str, Any]
|
||||
) -> Envelope | None:
|
||||
|
||||
@@ -4803,6 +4803,39 @@ class TaskService(BaseService):
|
||||
)
|
||||
return list(result.scalars().all())
|
||||
|
||||
async def has_earlier_incomplete_code_sibling(self, task: TaskTable) -> bool:
|
||||
"""True if a lower-sequence, non-terminal, same-assignee code sibling exists.
|
||||
|
||||
Service-layer mirror of the orchestrator's per-dev lane dispatch barrier
|
||||
(``_blocked_by_earlier_lane_sibling``). Used by the i_am_idle pending-work
|
||||
guard so a developer can exit cleanly while the rest of its code queue is
|
||||
still waiting its turn: a pre-delegated queue leaf (``pending``, assigned
|
||||
to this dev) that sits behind an earlier non-terminal sibling in the same
|
||||
lane should NOT pin the dev — the orchestrator spawns it once the lane
|
||||
clears. Without this the dev can neither idle nor proceed without jumping
|
||||
its own queue order. Only ``code`` queues sequence this way.
|
||||
"""
|
||||
if str(getattr(task, "task_type", "")) != TaskType.CODE.value:
|
||||
return False
|
||||
parent_id = task.parent_task_id
|
||||
owner = task.assigned_to
|
||||
seq = task.sequence
|
||||
if parent_id is None or owner is None or seq is None:
|
||||
return False
|
||||
result = await self.session.execute(
|
||||
select(TaskTable.status, TaskTable.sequence).where(
|
||||
TaskTable.parent_task_id == parent_id,
|
||||
TaskTable.assigned_to == owner,
|
||||
TaskTable.task_type == TaskType.CODE,
|
||||
TaskTable.id != task.id,
|
||||
)
|
||||
)
|
||||
terminal = {TaskStatus.COMPLETED, TaskStatus.CANCELLED}
|
||||
return any(
|
||||
(sib_seq or 0) < seq and status not in terminal
|
||||
for status, sib_seq in result.all()
|
||||
)
|
||||
|
||||
async def get_all_descendants(self, task_id: UUID) -> list[TaskTable]:
|
||||
"""Recursively get ALL descendant tasks (children, grandchildren, etc.).
|
||||
|
||||
|
||||
@@ -263,3 +263,43 @@ async def test_i_am_idle_decomposition_guard_is_pm_only() -> None:
|
||||
env = await c.i_am_idle(agent_id)
|
||||
assert env.status == "idle"
|
||||
task_svc.unclaimed_parent_acceptance_criteria.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_i_am_idle_lets_dev_idle_when_pending_is_lane_held() -> None:
|
||||
"""A dev's pending code leaf waiting behind its own earlier queue item must
|
||||
NOT pin it to idle — the orchestrator spawns it when the lane clears."""
|
||||
agent_id = uuid4()
|
||||
pending = MagicMock(id=uuid4(), status="pending")
|
||||
task_svc = AsyncMock()
|
||||
task_svc.list_assigned_for_agent.return_value = [pending]
|
||||
task_svc.list_in_progress_for_agent.return_value = []
|
||||
task_svc.agent_for.return_value = MagicMock(role="developer")
|
||||
task_svc.has_earlier_incomplete_code_sibling.return_value = True
|
||||
deps = _make_deps(task=task_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.i_am_idle(agent_id)
|
||||
assert env.status == "idle"
|
||||
task_svc.mark_agent_idle.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_i_am_idle_still_blocks_dev_with_non_lane_held_pending() -> None:
|
||||
"""A pending leaf that is NOT lane-held (no earlier live sibling) still
|
||||
blocks idle — the dev must claim or unclaim it."""
|
||||
agent_id = uuid4()
|
||||
pending = MagicMock(id=uuid4(), status="pending")
|
||||
task_svc = AsyncMock()
|
||||
task_svc.list_assigned_for_agent.return_value = [pending]
|
||||
task_svc.list_in_progress_for_agent.return_value = []
|
||||
task_svc.agent_for.return_value = MagicMock(role="developer")
|
||||
task_svc.has_earlier_incomplete_code_sibling.return_value = False
|
||||
deps = _make_deps(task=task_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
env = await c.i_am_idle(agent_id)
|
||||
body = env.as_dict()
|
||||
assert body["error"] == "invalid_state"
|
||||
assert "i_will_work_on" in body["remediate"]
|
||||
task_svc.mark_agent_idle.assert_not_awaited()
|
||||
|
||||
@@ -746,6 +746,81 @@ async def test_unclaimed_parent_acs_counts_live_children_not_just_completed() ->
|
||||
]
|
||||
|
||||
|
||||
def _svc_with_sibling_status_seq(rows: list[tuple]) -> TaskService:
|
||||
"""TaskService whose execute() yields (status, sequence) sibling rows."""
|
||||
res = MagicMock()
|
||||
res.all.return_value = rows
|
||||
return TaskService(MagicMock(execute=AsyncMock(return_value=res)))
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_earlier_incomplete_code_sibling_true_for_live_lower_seq() -> None:
|
||||
# A dev's queued code leaf (seq 2) is lane-held while its own seq-0 sibling
|
||||
# is still in flight — so it must not pin the dev to idle.
|
||||
task = _build_task(
|
||||
task_type=TaskType.CODE.value,
|
||||
parent_task_id=uuid4(),
|
||||
assigned_to=uuid4(),
|
||||
sequence=2,
|
||||
)
|
||||
svc = _svc_with_sibling_status_seq([(TaskStatus.IN_PROGRESS, 0)])
|
||||
assert await svc.has_earlier_incomplete_code_sibling(task) is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_earlier_incomplete_code_sibling_false_when_earlier_terminal() -> None:
|
||||
task = _build_task(
|
||||
task_type=TaskType.CODE.value,
|
||||
parent_task_id=uuid4(),
|
||||
assigned_to=uuid4(),
|
||||
sequence=2,
|
||||
)
|
||||
svc = _svc_with_sibling_status_seq(
|
||||
[(TaskStatus.COMPLETED, 0), (TaskStatus.CANCELLED, 1)]
|
||||
)
|
||||
assert await svc.has_earlier_incomplete_code_sibling(task) is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_earlier_incomplete_code_sibling_false_for_higher_seq_only() -> None:
|
||||
# A LATER sibling (seq 3) does not hold an earlier leaf (seq 2).
|
||||
task = _build_task(
|
||||
task_type=TaskType.CODE.value,
|
||||
parent_task_id=uuid4(),
|
||||
assigned_to=uuid4(),
|
||||
sequence=2,
|
||||
)
|
||||
svc = _svc_with_sibling_status_seq([(TaskStatus.IN_PROGRESS, 3)])
|
||||
assert await svc.has_earlier_incomplete_code_sibling(task) is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_earlier_incomplete_code_sibling_non_code_short_circuits() -> None:
|
||||
# Only code queues sequence this way; a planning/doc leaf never queries.
|
||||
session = MagicMock(execute=AsyncMock())
|
||||
svc = TaskService(session)
|
||||
task = _build_task(
|
||||
task_type="planning",
|
||||
parent_task_id=uuid4(),
|
||||
assigned_to=uuid4(),
|
||||
sequence=2,
|
||||
)
|
||||
assert await svc.has_earlier_incomplete_code_sibling(task) is False
|
||||
session.execute.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_earlier_incomplete_code_sibling_false_when_fields_missing() -> None:
|
||||
task = _build_task(
|
||||
task_type=TaskType.CODE.value,
|
||||
parent_task_id=None,
|
||||
assigned_to=uuid4(),
|
||||
sequence=2,
|
||||
)
|
||||
svc = _svc_with_sibling_status_seq([(TaskStatus.IN_PROGRESS, 0)])
|
||||
assert await svc.has_earlier_incomplete_code_sibling(task) is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_unblock_with_branch_resumes_in_progress() -> None:
|
||||
# A task claimed (has a branch) before it blocked resumes in_progress.
|
||||
|
||||
Reference in New Issue
Block a user