mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[fix] fail_qa routes needs_revision back to the dev, never the pool
A dev task in needs_revision must go back to the developer, never the pool. The pool path let a cell PM re-claim the revision (PMs can claim needs_revision) — the live 2026-06-27 'needs revision on a dev task sent to the cell PM' bug. fail_qa's original_developer marker is the fast path, but it is unreliable in practice (live observation: never persisted), so the unassign else-branch was the load-bearing path and it dropped the task into the pool. Add a work-session fallback (_resolve_revision_dev) that resolves the developer who actually worked the task — the most recent work session whose agent is a developer, the QA's own session excluded — and reassigns to that dev instead of unassigning. Only unassign when no developer ever touched the task. Self-heals the marker so a subsequent re-fail takes the fast path and the QA-review index attributes the work correctly.
This commit is contained in:
+69
-6
@@ -4090,13 +4090,41 @@ class TaskService(BaseService):
|
|||||||
original_developer=original_dev,
|
original_developer=original_dev,
|
||||||
)
|
)
|
||||||
else:
|
else:
|
||||||
# If no original developer found, unassign so it can be claimed
|
# A dev task in needs_revision must go back to THE DEV, never to
|
||||||
task.assigned_to = None
|
# the pool. ``submit_for_qa`` sets the ``original_developer`` marker
|
||||||
task.claimed_by = None
|
# on the normal path, so a missing marker means the task did not
|
||||||
self.log.warning(
|
# pass through it. Unassigning here used to drop the task into the
|
||||||
"No original developer found, task unassigned",
|
# pool, where a cell PM (PMs can re-claim needs_revision) would
|
||||||
task_id=str(task_id),
|
# 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()
|
await self.session.flush()
|
||||||
|
|
||||||
@@ -4125,6 +4153,41 @@ class TaskService(BaseService):
|
|||||||
self.log.info("Task failed QA", task_id=str(task_id))
|
self.log.info("Task failed QA", task_id=str(task_id))
|
||||||
return task
|
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(
|
async def docs_complete(
|
||||||
self,
|
self,
|
||||||
task_id: UUID,
|
task_id: UUID,
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ from roboco.db.tables import (
|
|||||||
JournalEntryTable,
|
JournalEntryTable,
|
||||||
ProductTable,
|
ProductTable,
|
||||||
ProjectTable,
|
ProjectTable,
|
||||||
|
WorkSessionTable,
|
||||||
)
|
)
|
||||||
from roboco.events import EventType
|
from roboco.events import EventType
|
||||||
from roboco.foundation.policy.content import markers
|
from roboco.foundation.policy.content import markers
|
||||||
@@ -31,6 +32,7 @@ from roboco.models.base import (
|
|||||||
TaskType,
|
TaskType,
|
||||||
)
|
)
|
||||||
from roboco.models.task import TaskCreateRequest
|
from roboco.models.task import TaskCreateRequest
|
||||||
|
from roboco.models.work_session import WorkSessionStatus
|
||||||
from roboco.seeds.initial_data import AGENT_UUIDS
|
from roboco.seeds.initial_data import AGENT_UUIDS
|
||||||
from roboco.services.base import NotFoundError
|
from roboco.services.base import NotFoundError
|
||||||
from roboco.services.task import SoftBlockInfo, TaskService
|
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
|
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
|
# ceo_approve / ceo_reject — happy paths
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
Reference in New Issue
Block a user