diff --git a/roboco/services/task.py b/roboco/services/task.py index 312039c3..bced1ff3 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -312,6 +312,24 @@ _PM_OWNED_CELL_TASK_TYPES: frozenset[str] = frozenset( } ) +# Review-pipeline handoff states. Per CLAIM_RULES these are owned by a +# non-PM role (QA / documenter / PR reviewer), so the cell-PM ownership +# redirect must NOT fire when a cell-PM-owned task is mid-handoff into one +# of them. The redirect exists to route *delegation/escalation* ownership +# (PENDING / CLAIMED / NEEDS_REVISION) away from main-pm and onto the cell +# PM; applying it to a QA/documenter/PR-reviewer handoff clobbers the +# review's rightful claimant with the cell PM, who has no claim right on +# these states — the task then deadlocks until a human manually reassigns. +# AWAITING_PM_REVIEW is deliberately excluded: there the cell PM *is* the +# rightful owner, so the redirect still applies. +_REVIEW_HANDOFF_STATUSES: frozenset[str] = frozenset( + { + TaskStatus.AWAITING_QA.value, + TaskStatus.AWAITING_DOCUMENTATION.value, + TaskStatus.AWAITING_PR_REVIEW.value, + } +) + def _is_cell_pm_owned_task(task: TaskTable) -> bool: """True for a descendant cell-team task that must be owned by its cell PM. @@ -3643,6 +3661,10 @@ class TaskService(BaseService): descendants = await self.get_all_descendants(task_id) descendants.reverse() # Delete deepest children first + # Capture ids before deletion — the row is gone after flush, and + # we need them to prune every surviving dependent's dependency_ids. + removed_ids = [task_id, *(getattr(d, "id", None) for d in descendants)] + for descendant in descendants: await self.session.delete(descendant) @@ -3656,6 +3678,15 @@ class TaskService(BaseService): await self.session.delete(task) await self.session.flush() + # Cascade the dependency cleanup the COMPLETE path runs. Without + # this, a task BLOCKED on the deleted one is never auto-revived and + # the stale id lingers in every dependent's dependency_ids forever + # (the claim gate treats a missing row as "met", but BLOCKED + # dependents are past the gate and only _unblock_dependents revives). + for removed_id in removed_ids: + if removed_id is not None: + await self._unblock_dependents(removed_id) + self.log.info("Task deleted", task_id=str(task_id)) return True @@ -8132,6 +8163,16 @@ class TaskService(BaseService): await self.session.flush() await self._alert_coroner_of_cancel(task) + # Cascade the dependency cleanup the COMPLETE path runs. Without + # this, a task BLOCKED on the cancelled one is never auto-revived + # and the stale id lingers in every dependent's dependency_ids + # forever. Prune for the whole subtree (root + all descendants); + # idempotent for already-terminal descendants whose edges may have + # been pruned before — a second prune just finds no matching edge. + for cancelled_id in (task_id, *(getattr(d, "id", None) for d in descendants)): + if cancelled_id is not None: + await self._unblock_dependents(cancelled_id) + # Origin fix: a cancelled child may have declared parent_ac_refs that # no surviving sibling covers, leaving the roll-up gate # (_parent_acs_covered_envelope) demanding coverage for already- @@ -10874,6 +10915,17 @@ class TaskService(BaseService): ) if not _is_cell_pm_owned_task(task): return noop + # A cell-PM-owned task that is already mid-handoff into a review + # state (QA / documenter / PR reviewer per CLAIM_RULES) must keep + # that handoff target — the ownership redirect is for routing + # delegation/escalation, not for clobbering the review claimant. + status_value = ( + task.status.value + if isinstance(task.status, TaskStatus) + else str(task.status) + ) + if status_value in _REVIEW_HANDOFF_STATUSES: + return noop assert task.team is not None # guarded by _is_cell_pm_owned_task team_enum = Team(str(getattr(task.team, "value", task.team))) cell_pm = await self.cell_pm_for_team(team_enum) diff --git a/tests/integration/test_task_service_transitions.py b/tests/integration/test_task_service_transitions.py index 02f1df2f..6f49b1c1 100644 --- a/tests/integration/test_task_service_transitions.py +++ b/tests/integration/test_task_service_transitions.py @@ -1780,6 +1780,79 @@ async def test_unblock_dependents_keeps_blocked_when_other_deps_remain( assert other_blocker.id not in refreshed.completed_dependency_ids +# --------------------------------------------------------------------------- +# delete / cancel must cascade the dependency prune + revive blocked dependents +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_delete_unblocks_blocked_dependent( + task_setup: dict, db_session: AsyncSession +) -> None: + """A task BLOCKED on a deleted task must be auto-revived — the delete + path cascades the same _unblock_dependents prune the COMPLETE path runs. + Without it the BLOCKED dependent is never revived (it is past the claim + gate, which only treats a missing dep row as "met" for PENDING claims).""" + svc = task_setup["svc"] + blocker = await svc.create(_req(task_setup)) + dependent = await svc.create(_req(task_setup)) + dependent.status = TaskStatus.BLOCKED + dependent.dependency_ids = [blocker.id] + await db_session.flush() + + deleted = await svc.delete(blocker.id) + assert deleted is True + + refreshed = await svc.get(dependent.id) + assert refreshed is not None + assert blocker.id not in refreshed.dependency_ids + # Revived out of BLOCKED — back to workable state. + assert refreshed.status != TaskStatus.BLOCKED + + +@pytest.mark.asyncio +async def test_delete_prunes_stale_dependency_from_pending_dependent( + task_setup: dict, db_session: AsyncSession +) -> None: + """A PENDING dependent's stale dep id is pruned on delete (data hygiene); + it is not revived because it was never BLOCKED.""" + svc = task_setup["svc"] + blocker = await svc.create(_req(task_setup)) + dependent = await svc.create(_req(task_setup)) + dependent.dependency_ids = [blocker.id] + await db_session.flush() + + await svc.delete(blocker.id) + + refreshed = await svc.get(dependent.id) + assert refreshed is not None + assert refreshed.status == TaskStatus.PENDING + assert blocker.id not in refreshed.dependency_ids + + +@pytest.mark.asyncio +async def test_cancel_unblocks_blocked_dependent( + task_setup: dict, db_session: AsyncSession +) -> None: + """Cancelling a blocker revives a BLOCKED dependent the same way — the + cancelled id is pruned and the dependent resumes. A cancelled task is + terminal, so it must never hold a dependent in BLOCKED forever.""" + svc = task_setup["svc"] + blocker = await svc.create(_req(task_setup)) + dependent = await svc.create(_req(task_setup)) + dependent.status = TaskStatus.BLOCKED + dependent.dependency_ids = [blocker.id] + await db_session.flush() + + cancelled = await svc.cancel(blocker.id, agent_role="cell_pm") + assert cancelled is not None + + refreshed = await svc.get(dependent.id) + assert refreshed is not None + assert blocker.id not in refreshed.dependency_ids + assert refreshed.status != TaskStatus.BLOCKED + + # --------------------------------------------------------------------------- # claim — gate validations (team mismatch, role mismatch, self-review) # --------------------------------------------------------------------------- diff --git a/tests/unit/services/test_task_assignment_invariants.py b/tests/unit/services/test_task_assignment_invariants.py index 77deaa28..608a764e 100644 --- a/tests/unit/services/test_task_assignment_invariants.py +++ b/tests/unit/services/test_task_assignment_invariants.py @@ -311,3 +311,122 @@ def test_create_does_not_redirect_misassigned_cell_planning_child() -> None: assert not hasattr(svc, "_redirect_cell_team_pm_task"), ( "create-path redirect was re-added; remove it (see audit Gap 1)." ) + + +# --------------------------------------------------------------------------- +# Regression — review-pipeline handoffs must NOT be redirected to the cell PM +# +# A cell-PM-owned task type (documentation / design / research / planning / +# administrative) that enters a review state (awaiting_qa / awaiting_documentation +# / awaiting_pr_review) is mid-handoff to a non-PM claimant (QA / documenter / +# PR reviewer per CLAIM_RULES). The cell-PM ownership redirect is for +# delegation/escalation routing, not for clobbering the review claimant — +# redirecting here lands the task on the cell PM, who has no claim right on +# these states, so it deadlocks until a human manually reassigns. +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "status", + [ + TaskStatus.AWAITING_QA, + TaskStatus.AWAITING_DOCUMENTATION, + TaskStatus.AWAITING_PR_REVIEW, + ], +) +@pytest.mark.parametrize( + "task_type", + [TaskType.DOCUMENTATION, TaskType.DESIGN, TaskType.RESEARCH], +) +@pytest.mark.asyncio +async def test_reassign_keeps_review_handoff_for_cell_pm_owned_child( + status: TaskStatus, task_type: TaskType +) -> None: + """The QA / documenter / PR-reviewer handoff into a review state keeps + the requested reviewer — the cell-PM redirect does not fire there.""" + svc = _service() + reviewer_id = uuid4() + task = _task( + team=Team.BACKEND, + task_type=task_type, + assigned_to=None, + status=status, + ) + _bind(svc, "get", AsyncMock(return_value=task)) + _bind(svc, "_is_board_advisory_agent", AsyncMock(return_value=False)) + # cell_pm_for_team must NOT be consulted when the status gate fires — + # if it is, the test fails because the AsyncMock has no return_value. + _bind( + svc, + "cell_pm_for_team", + AsyncMock( + side_effect=AssertionError( + "cell_pm_for_team must not run for a review-handoff state" + ) + ), + ) + + result = await svc.reassign(task.id, reviewer_id) + + assert result is task + assert task.assigned_to == reviewer_id + assert task.claimed_by == reviewer_id + assert "[ASSIGNMENT REDIRECTED]" not in (task.dev_notes or "") + + +@pytest.mark.asyncio +async def test_reassign_still_redirects_on_awaiting_pm_review() -> None: + """AWAITING_PM_REVIEW is NOT a review-handoff state — the cell PM is the + rightful owner there, so the redirect still fires. Guards against the + status gate over-broadening and breaking PM-review ownership.""" + svc = _service() + be_pm_id = uuid4() + main_pm_id = uuid4() + task = _task( + team=Team.BACKEND, + task_type=TaskType.PLANNING, + assigned_to=main_pm_id, + status=TaskStatus.AWAITING_PM_REVIEW, + ) + _bind(svc, "get", AsyncMock(return_value=task)) + _bind(svc, "_is_board_advisory_agent", AsyncMock(return_value=False)) + _bind( + svc, + "cell_pm_for_team", + AsyncMock(return_value=MagicMock(id=be_pm_id, slug="be-pm")), + ) + + result = await svc.reassign(task.id, main_pm_id) + + assert result is task + assert task.assigned_to == be_pm_id + assert "[ASSIGNMENT REDIRECTED]" in task.dev_notes + + +@pytest.mark.asyncio +async def test_reassign_still_redirects_on_claimed_planning_child() -> None: + """The non-review routing paths (delegation/escalation on PENDING / + CLAIMED / NEEDS_REVISION) keep the redirect — the status gate only + covers the review-handoff states, not ownership routing.""" + svc = _service() + be_pm_id = uuid4() + main_pm_id = uuid4() + task = _task( + team=Team.BACKEND, + task_type=TaskType.PLANNING, + assigned_to=main_pm_id, + status=TaskStatus.CLAIMED, + ) + _bind(svc, "get", AsyncMock(return_value=task)) + _bind(svc, "_is_board_advisory_agent", AsyncMock(return_value=False)) + _bind( + svc, + "cell_pm_for_team", + AsyncMock(return_value=MagicMock(id=be_pm_id, slug="be-pm")), + ) + + result = await svc.reassign(task.id, main_pm_id) + + assert result is task + assert task.assigned_to == be_pm_id + assert "[ASSIGNMENT REDIRECTED]" in task.dev_notes