diff --git a/roboco/services/task.py b/roboco/services/task.py index dc19d02e..ecf24c64 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -4090,13 +4090,41 @@ class TaskService(BaseService): original_developer=original_dev, ) else: - # If no original developer found, unassign so it can be claimed - task.assigned_to = None - task.claimed_by = None - self.log.warning( - "No original developer found, task unassigned", - task_id=str(task_id), + # A dev task in needs_revision must go back to THE DEV, never to + # the pool. ``submit_for_qa`` sets the ``original_developer`` marker + # on the normal path, so a missing marker means the task did not + # pass through it. Unassigning here used to drop the task into the + # pool, where a cell PM (PMs can re-claim needs_revision) would + # grab it — exactly "needs revision on a dev task sent to the cell + # PM" (live 2026-06-27). Fall back to the developer who actually + # worked it: the most recent work session whose agent is a + # developer (the QA's own session excluded). Only unassign when no + # developer ever touched the task. + fallback_dev = await self._resolve_revision_dev( + task, exclude=to_python_uuid(qa_agent_id) ) + if fallback_dev is not None: + task.assigned_to = cast("Any", fallback_dev) + task.claimed_by = cast("Any", fallback_dev) + # Self-heal the marker so a subsequent re-fail takes the fast + # path and the QA-review index (below) attributes the work to + # the right developer. The marker is unreliable in practice + # (live 2026-06-27: never persisted), so the work session is + # the load-bearing resolver; stamping it here makes the two + # paths converge instead of the marker staying absent forever. + markers.set_original_developer(task, str(fallback_dev)) + self.log.info( + "Task reassigned to revision developer (marker missing)", + task_id=str(task_id), + revision_developer=str(fallback_dev), + ) + else: + task.assigned_to = None + task.claimed_by = None + self.log.warning( + "No developer found for revision; task unassigned", + task_id=str(task_id), + ) await self.session.flush() @@ -4125,6 +4153,41 @@ class TaskService(BaseService): self.log.info("Task failed QA", task_id=str(task_id)) return task + async def _resolve_revision_dev( + self, task: TaskTable, *, exclude: UUID | None + ) -> UUID | None: + """The developer who should receive a needs_revision dev task when the + ``original_developer`` marker is missing. + + A dev task in needs_revision must return to the dev, never the pool: an + unassigned needs_revision task is claimable by a cell PM, which is how a + dev task's revision landed on the cell PM live (2026-06-27). Resolves the + developer who actually worked it — the most recent work session on the + task whose agent is a developer (the QA's own session, ``exclude``, is + skipped so the fallback can't hand the task back to QA). Returns None + only when no developer ever touched the task (then unassign is the last + resort). + """ + conditions = [WorkSessionTable.task_id == task.id] + if exclude is not None: + conditions.append(WorkSessionTable.agent_id != exclude) + result = await self.session.execute( + select(WorkSessionTable) + .where(and_(*conditions)) + .order_by(WorkSessionTable.started_at.desc()) + ) + for ws in result.scalars().all(): + agent_id = to_python_uuid(ws.agent_id) + if agent_id is None: + continue + agent = await self.agent_for(agent_id) + if agent is None: + continue + role = agent.role.value if hasattr(agent.role, "value") else str(agent.role) + if role == "developer": + return agent_id + return None + async def docs_complete( self, task_id: UUID, diff --git a/tests/integration/test_task_service_transitions.py b/tests/integration/test_task_service_transitions.py index 34c3abc3..be58b4b7 100644 --- a/tests/integration/test_task_service_transitions.py +++ b/tests/integration/test_task_service_transitions.py @@ -18,6 +18,7 @@ from roboco.db.tables import ( JournalEntryTable, ProductTable, ProjectTable, + WorkSessionTable, ) from roboco.events import EventType from roboco.foundation.policy.content import markers @@ -31,6 +32,7 @@ from roboco.models.base import ( TaskType, ) from roboco.models.task import TaskCreateRequest +from roboco.models.work_session import WorkSessionStatus from roboco.seeds.initial_data import AGENT_UUIDS from roboco.services.base import NotFoundError from roboco.services.task import SoftBlockInfo, TaskService @@ -602,6 +604,148 @@ async def test_fail_qa_with_no_original_dev_unassigns( assert failed.assigned_to is None +@pytest.mark.asyncio +async def test_fail_qa_routes_to_dev_via_work_session_when_marker_missing( + task_setup: dict, db_session: AsyncSession +) -> None: + """A dev task in needs_revision with a MISSING original_developer marker + routes back to the developer who worked it (resolved from the work + session), NOT to the pool. + + Unassigning sends the task to the pool where a cell PM (PMs can re-claim + needs_revision) grabs it — the live 2026-06-27 "needs revision on a dev + task sent to the cell PM" bug. The marker is the fast path; the work + session is the load-bearing fallback (the marker is unreliable in + practice). The fallback also self-heals the marker so a subsequent + re-fail takes the fast path. + """ + svc = task_setup["svc"] + dev_id = task_setup["agent_id"] + task = await svc.create(_req(task_setup)) + task.branch_name = "feature/backend/abc" + await db_session.flush() + + # The dev worked the task (a work session exists) but the marker was never + # set — e.g. the task was rerouted before submit_for_qa ran, or the marker + # failed to persist (live observation). This is the "OG DEV NEVER + # PERSISTED" scenario. + ws = WorkSessionTable( + id=uuid4(), + project_id=task_setup["project_id"], + task_id=task.id, + agent_id=dev_id, + branch_name="feature/backend/abc", + base_branch="main", + target_branch="main", + status=WorkSessionStatus.ACTIVE, + ) + db_session.add(ws) + + # A separate QA agent claims and fails the task. + qa = AgentTable( + id=uuid4(), + name="QA", + slug=f"be-qa-{uuid4().hex[:8]}", + role=AgentRole.QA, + team=Team.BACKEND, + status=AgentStatus.ACTIVE, + model_config={}, + system_prompt="qa", + capabilities=[], + permissions={}, + metrics={}, + ) + db_session.add(qa) + await db_session.flush() + + task.status = TaskStatus.AWAITING_QA + task.assigned_to = qa.id + task.claimed_by = qa.id + # No orchestration_markers — extract_original_developer returns None. + await db_session.flush() + + failed = await svc.fail_qa(task.id, notes="needs more") + assert failed is not None + assert failed.status == TaskStatus.NEEDS_REVISION + # Routed back to the dev — NOT unassigned (pool, where a cell PM would + # claim it) and NOT left on the QA. + assert failed.assigned_to == dev_id + assert failed.claimed_by == dev_id + # Self-healed: the marker is now stamped so the next fail_qa uses the + # fast path and the QA-review index attributes the work correctly. + assert markers.get_original_developer(failed) == str(dev_id) + + +@pytest.mark.asyncio +async def test_fail_qa_work_session_fallback_excludes_qa_session( + task_setup: dict, db_session: AsyncSession +) -> None: + """The work-session fallback must not hand the task back to the QA. + + If the only developer work session were the QA's (it isn't — QA sessions + are skipped at creation — but defensively the exclude filter must keep a + QA-attributed session from being misread as the revision dev), the + fallback would loop the task back to the reviewer. The exclude filter + (qa_agent_id) guarantees only a real developer is resolved. + """ + svc = task_setup["svc"] + dev_id = task_setup["agent_id"] + task = await svc.create(_req(task_setup)) + task.branch_name = "feature/backend/abc" + await db_session.flush() + + qa = AgentTable( + id=uuid4(), + name="QA", + slug=f"be-qa-{uuid4().hex[:8]}", + role=AgentRole.QA, + team=Team.BACKEND, + status=AgentStatus.ACTIVE, + model_config={}, + system_prompt="qa", + capabilities=[], + permissions={}, + metrics={}, + ) + db_session.add(qa) + await db_session.flush() + + # A QA-attributed work session (older) and a dev-attributed one (newer). + qa_ws = WorkSessionTable( + id=uuid4(), + project_id=task_setup["project_id"], + task_id=task.id, + agent_id=qa.id, + branch_name="feature/backend/abc", + base_branch="main", + target_branch="main", + status=WorkSessionStatus.ACTIVE, + ) + dev_ws = WorkSessionTable( + id=uuid4(), + project_id=task_setup["project_id"], + task_id=task.id, + agent_id=dev_id, + branch_name="feature/backend/abc", + base_branch="main", + target_branch="main", + status=WorkSessionStatus.ACTIVE, + ) + db_session.add_all([qa_ws, dev_ws]) + await db_session.flush() + + task.status = TaskStatus.AWAITING_QA + task.assigned_to = qa.id + task.claimed_by = qa.id + await db_session.flush() + + failed = await svc.fail_qa(task.id, notes="needs more") + assert failed is not None + # Resolved to the dev, NOT the QA — the exclude filter did its job. + assert failed.assigned_to == dev_id + assert failed.assigned_to != qa.id + + # --------------------------------------------------------------------------- # ceo_approve / ceo_reject — happy paths # ---------------------------------------------------------------------------