mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(lifecycle): stop cell-PM redirect clobbering review handoffs; cascade dependency prune on delete/cancel
Two live bugs the CEO hit daily, both in TaskService: 1. awaiting_qa / awaiting_documentation tasks were assigned to the cell PM (be-pm) and deadlocked until manually reassigned. _resolve_cell_pm_redirect forces every cell-PM-owned task type (documentation/design/research/planning/ administrative) onto the cell PM on every reassign(). The gateway hands off to QA (i_am_done) and to the documenter (pass_review) via reassign(), so the redirect clobbered the rightful reviewer — who has no claim right on those states per CLAIM_RULES. The redirect is for delegation/escalation ownership routing, not for clobbering a review handoff. Gate it on _REVIEW_HANDOFF_STATUSES (awaiting_qa / awaiting_documentation / awaiting_pr_review); awaiting_pm_review is deliberately excluded (the cell PM IS the owner there, so the redirect still fires). 2. Deleting or cancelling a task left a dependent BLOCKED on it stuck forever — the CEO caught a good-to-go task held by a stale dependency on a deleted task. delete() and cancel() never called _unblock_dependents (the edge-prune only the complete()/ceo_approve paths ran), so a BLOCKED dependent was never auto-revived. (A PENDING dependent's claim was not blocked — unmet_dependency_ids treats a missing row as "met" — but a BLOCKED dependent is past the gate and only _unblock_dependents revives it.) Both paths now loop _unblock_dependents over the root and every descendant; idempotent, and a deleted id in completed_dependency_ids is a dead-end node in the sequence graph so it cannot false-block. Tests: 3 unit (review-handoff preserved for QA/doc/PR-reviewer; PM-review and CLAIMED redirects still fire) + 3 integration (delete/cancel revive a BLOCKED dependent; delete prunes a PENDING dependent).
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user