External/roboco
1
0
Fork 0
mirror of https://github.com/rennf93/roboco.git synced 2026-08-03 07:23:24 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(gateway): correct claim/start argument order in choreographer

Browse Source 
Service signatures are (task_id, agent_id, ...) but choreographer was
calling (agent_id, task_id). Production claim path silently returned
None; unit tests pinned the buggy order so the bug was invisible. Swap
all 7 call sites and update test assertions. Add a regression pin
that locks in the correct order.
This commit is contained in:
Renn F
2026-05-03 04:27:14 +02:00
parent 4c9b7c4210
commit c33340bda1
5 changed files with 231 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/unit/gateway/test_choreographer_claim_guards.py
+2 -2
View File
@@ -147,7 +147,7 @@ async def test_i_will_work_on_allows_when_earlier_sibling_terminal() -> None:
env = await c.i_will_work_on(agent_id, target_id)
assert env.error is None
task_svc.claim.assert_awaited_once_with(agent_id, target_id)
task_svc.claim.assert_awaited_once_with(target_id, agent_id)
@pytest.mark.asyncio
@@ -242,7 +242,7 @@ async def test_i_will_work_on_resumption_does_not_self_block() -> None:
env = await c.i_will_work_on(agent_id, task_id)
assert env.error is None
task_svc.start.assert_awaited_once_with(agent_id, task_id)
task_svc.start.assert_awaited_once_with(task_id, agent_id)
# ---------------------------------------------------------------------------
tests/unit/gateway/test_choreographer_dev.py
+3 -3
View File
@@ -120,9 +120,9 @@ async def test_i_will_work_on_pending_with_plan() -> None:
env = await c.i_will_work_on(agent_id, task_id, plan="do x then y")
assert env.error is None
assert env.status == "in_progress"
task_svc.claim.assert_awaited_once_with(agent_id, task_id)
task_svc.claim.assert_awaited_once_with(task_id, agent_id)
task_svc.set_plan.assert_awaited_once()
task_svc.start.assert_awaited_once_with(agent_id, task_id)
task_svc.start.assert_awaited_once_with(task_id, agent_id)
@pytest.mark.asyncio
@@ -177,7 +177,7 @@ async def test_i_will_work_on_needs_revision_re_starts() -> None:
env = await c.i_will_work_on(agent_id, task_id)
assert env.status == "in_progress"
task_svc.claim.assert_not_awaited() # already assigned
task_svc.start.assert_awaited_once_with(agent_id, task_id)
task_svc.start.assert_awaited_once_with(task_id, agent_id)
@pytest.mark.asyncio
tests/unit/gateway/test_choreographer_pm_extras.py
+2 -2
View File
@@ -90,9 +90,9 @@ async def test_i_will_plan_claims_starts_and_sets_plan() -> None:
env = await c.i_will_plan(pm_id, task_id, plan="break the work into 3 subtasks")
assert env.error is None
assert env.status == "in_progress"
task_svc.claim.assert_awaited_once_with(pm_id, task_id)
task_svc.claim.assert_awaited_once_with(task_id, pm_id)
task_svc.set_plan.assert_awaited_once()
task_svc.start.assert_awaited_once_with(pm_id, task_id)
task_svc.start.assert_awaited_once_with(task_id, pm_id)
@pytest.mark.asyncio
tests/unit/gateway/test_claim_arg_order.py
+217
View File
@@ -0,0 +1,217 @@
"""Pin: Choreographer must call task.claim/start with (task_id, agent_id).
Service signatures in ``roboco/services/task.py`` are:
async def claim(self, task_id: UUID, agent_id: UUID, ...) -> TaskTable | None
async def start(self, task_id: UUID, agent_id: UUID | None = None, ...) -> ...
Earlier choreographer code passed (agent_id, task_id) — when the SQL lookup
ran ``WHERE id = <agent_uuid>``, no row matched and the call returned None,
which the choreographer interpreted as "task unchanged". The whole gateway
claim path was non-functional against a real DB. Existing unit tests pinned
the buggy order so the bug stayed invisible.
This test pins the correct order at every Choreographer call site that
forwards into the service.
"""
from __future__ import annotations
from typing import Any
from unittest.mock import AsyncMock, MagicMock
from uuid import uuid4
import pytest
from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps
def _make_deps(**overrides: Any) -> ChoreographerDeps:
base = {
"task": AsyncMock(),
"work_session": AsyncMock(),
"git": AsyncMock(),
"a2a": AsyncMock(),
"journal": AsyncMock(),
"audit": AsyncMock(),
"evidence_repo": AsyncMock(),
}
base.update(overrides)
repo = base["evidence_repo"]
for method in (
"list_unread_a2a",
"list_unread_mentions",
"list_pending_notifications",
"task_metadata_gaps",
"recent_team_activity",
"blockers_in_lane",
"journal_highlights_for_task",
):
getattr(repo, method).return_value = []
return ChoreographerDeps(**base)
@pytest.mark.asyncio
async def test_i_will_work_on_pending_calls_claim_with_task_id_first() -> None:
"""Dev pending claim: positional args must be (task_id, agent_id)."""
agent_id = uuid4()
task_id = uuid4()
pending = MagicMock(
id=task_id,
status="pending",
plan=None,
assigned_to=None,
task_type="code",
parent_task_id=None,
sequence=0,
team="backend",
)
claimed = MagicMock(
id=task_id,
status="claimed",
plan=None,
assigned_to=agent_id,
task_type="code",
)
with_plan = MagicMock(
id=task_id,
status="claimed",
plan={"text": "x"},
assigned_to=agent_id,
task_type="code",
)
started = MagicMock(
id=task_id,
status="in_progress",
plan={"text": "x"},
assigned_to=agent_id,
task_type="code",
)
task_svc = AsyncMock()
task_svc.get.return_value = pending
task_svc.agent_for.return_value = MagicMock(role="developer", team="backend")
task_svc.list_in_progress_for_agent.return_value = []
task_svc.list_paused_for_agent.return_value = []
task_svc.get_subtasks.return_value = []
task_svc.claim.return_value = claimed
task_svc.set_plan.return_value = with_plan
task_svc.start.return_value = started
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
env = await c.i_will_work_on(agent_id, task_id, plan="x")
# Service signature is (task_id, agent_id, ...) — pin that order.
task_svc.claim.assert_awaited_once_with(task_id, agent_id)
task_svc.start.assert_awaited_once_with(task_id, agent_id)
assert env.error is None
@pytest.mark.asyncio
async def test_i_will_work_on_needs_revision_calls_start_with_task_id_first() -> None:
"""Dev needs_revision resume: start args must be (task_id, agent_id)."""
agent_id = uuid4()
task_id = uuid4()
nr = MagicMock(
id=task_id,
status="needs_revision",
assigned_to=agent_id,
plan={"x": 1},
task_type="code",
)
started = MagicMock(
id=task_id, status="in_progress", assigned_to=agent_id, plan={"x": 1}
)
task_svc = AsyncMock()
task_svc.get.return_value = nr
task_svc.start.return_value = started
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
env = await c.i_will_work_on(agent_id, task_id)
task_svc.start.assert_awaited_once_with(task_id, agent_id)
task_svc.claim.assert_not_awaited()
assert env.error is None
@pytest.mark.asyncio
async def test_i_will_work_on_claimed_resumption_calls_start_with_task_id_first() -> (
None
):
"""Dev claimed resumption: start args must be (task_id, agent_id)."""
agent_id = uuid4()
task_id = uuid4()
claimed = MagicMock(
id=task_id,
status="claimed",
plan={"x": 1},
assigned_to=agent_id,
parent_task_id=None,
sequence=0,
task_type="code",
team="backend",
branch_name="feature/backend/abc",
)
started = MagicMock(
id=task_id, status="in_progress", plan={"x": 1}, assigned_to=agent_id
)
task_svc = AsyncMock()
task_svc.get.return_value = claimed
task_svc.agent_for.return_value = MagicMock(role="developer", team="backend")
task_svc.list_in_progress_for_agent.return_value = []
task_svc.list_paused_for_agent.return_value = []
task_svc.get_subtasks.return_value = []
task_svc.start.return_value = started
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
env = await c.i_will_work_on(agent_id, task_id)
task_svc.start.assert_awaited_once_with(task_id, agent_id)
assert env.error is None
@pytest.mark.asyncio
async def test_i_will_plan_calls_claim_and_start_with_task_id_first() -> None:
"""PM plan path: both claim and start must use (task_id, agent_id)."""
pm_id = uuid4()
task_id = uuid4()
pending = MagicMock(
id=task_id,
status="pending",
plan=None,
assigned_to=None,
task_type="planning",
parent_task_id=None,
sequence=0,
)
claimed = MagicMock(
id=task_id,
status="claimed",
plan=None,
assigned_to=pm_id,
task_type="planning",
)
started = MagicMock(
id=task_id,
status="in_progress",
plan={"text": "x"},
assigned_to=pm_id,
task_type="planning",
)
task_svc = AsyncMock()
task_svc.get.return_value = pending
task_svc.agent_for.return_value = MagicMock(role="cell_pm", team="backend")
task_svc.list_in_progress_for_agent.return_value = []
task_svc.list_paused_for_agent.return_value = []
task_svc.claim.return_value = claimed
task_svc.set_plan.return_value = claimed
task_svc.start.return_value = started
deps = _make_deps(task=task_svc)
c = Choreographer(deps)
env = await c.i_will_plan(pm_id, task_id, plan="break the work into 3 subtasks")
task_svc.claim.assert_awaited_once_with(task_id, pm_id)
task_svc.start.assert_awaited_once_with(task_id, pm_id)
assert env.error is None