diff --git a/docs/rag/lifecycle/status-transitions.md b/docs/rag/lifecycle/status-transitions.md index 083614ab..d7f39fdf 100644 --- a/docs/rag/lifecycle/status-transitions.md +++ b/docs/rag/lifecycle/status-transitions.md @@ -11,7 +11,6 @@ | awaiting_documentation | claimed | claim | documenter | | awaiting_pm_review | awaiting_ceo_approval | escalate_to_ceo | head_marketing, main_pm, product_owner | | awaiting_pm_review | cancelled | cancel | cell_pm, ceo, main_pm | -| awaiting_pm_review | claimed | claim | cell_pm, main_pm | | awaiting_pm_review | completed | complete | cell_pm, main_pm | | awaiting_pm_review | needs_revision | request_changes | cell_pm, main_pm | | awaiting_pr_review | awaiting_pm_review | pr_pass | pr_reviewer | diff --git a/panel/lib/lifecycle.json b/panel/lib/lifecycle.json index 8d0aa4c3..c673a92e 100644 --- a/panel/lib/lifecycle.json +++ b/panel/lib/lifecycle.json @@ -2,7 +2,6 @@ "claim_rules": { "auditor": [], "cell_pm": [ - "awaiting_pm_review", "needs_revision", "pending" ], @@ -17,7 +16,6 @@ ], "head_marketing": [], "main_pm": [ - "awaiting_pm_review", "needs_revision", "pending" ], @@ -531,15 +529,6 @@ "source": "awaiting_pm_review", "target": "cancelled" }, - { - "action": "claim", - "roles": [ - "cell_pm", - "main_pm" - ], - "source": "awaiting_pm_review", - "target": "claimed" - }, { "action": "complete", "roles": [ diff --git a/roboco/enforcement/task_lifecycle.py b/roboco/enforcement/task_lifecycle.py index 257902a3..90c46ab5 100644 --- a/roboco/enforcement/task_lifecycle.py +++ b/roboco/enforcement/task_lifecycle.py @@ -92,8 +92,14 @@ _LEGACY_OPERATIONAL_EDGES: dict[Status, frozenset[Status]] = { Status.VERIFYING: frozenset({Status.NEEDS_REVISION, Status.PENDING}), # QA can park a task as blocked while waiting on dev clarification. Status.AWAITING_QA: frozenset({Status.BLOCKED}), - # PM claim + PM reject path on review queue. - Status.AWAITING_PM_REVIEW: frozenset({Status.CLAIMED, Status.NEEDS_REVISION}), + # PM reject path on review queue. No CLAIMED edge here (removed): a PM + # claiming its own awaiting_pm_review task used to legally reset it to + # in_progress via i_will_plan's composed claim, looping submit_up -> + # pr_pass -> awaiting_pm_review forever. lifecycle.CLAIM_RULES / + # _ROLE_CLAIM_STATUSES close the edge upstream of this legacy view; the + # choreographer's _handle_pm_reentry now steers a re-entering PM to + # complete/request_changes instead. + Status.AWAITING_PM_REVIEW: frozenset({Status.NEEDS_REVISION}), # Re-entry from revision back into active dev work (without re-claim), or # voluntary unclaim back to the pool (TaskService.unclaim_for_agent) — a # dev sent back for revision otherwise had no legal exit but in_progress. @@ -113,13 +119,6 @@ _LEGACY_ROLE_GATES: dict[tuple[Status, Status], tuple[str, ...]] = { "main_pm", "product_owner", ), - # PM claim of review queue. - (Status.AWAITING_PM_REVIEW, Status.CLAIMED): ( - "cell_pm", - "head_marketing", - "main_pm", - "product_owner", - ), # PM reject back to dev (needs_revision). (Status.AWAITING_PM_REVIEW, Status.NEEDS_REVISION): ( "cell_pm", diff --git a/roboco/foundation/policy/lifecycle.py b/roboco/foundation/policy/lifecycle.py index 380c3bf0..6194aa50 100644 --- a/roboco/foundation/policy/lifecycle.py +++ b/roboco/foundation/policy/lifecycle.py @@ -245,14 +245,12 @@ _STATUS_TRANSITIONS: tuple[StatusTransition, ...] = ( frozenset({Role.DOCUMENTER}), ), StatusTransition(Status.NEEDS_REVISION, Status.CLAIMED, "claim", None), - # A PM re-claims an awaiting_pm_review task it already owns (CLAIM_RULES - # grants this to CELL_PM/MAIN_PM) — see the "claim" ActionSpec comment. - StatusTransition( - Status.AWAITING_PM_REVIEW, - Status.CLAIMED, - "claim", - frozenset({Role.CELL_PM, Role.MAIN_PM}), - ), + # No AWAITING_PM_REVIEW -> CLAIMED edge: a PM re-entering its own + # review-queue task is steered by the choreographer's i_will_plan + # re-entry contract straight to complete/request_changes, never via a + # claim (a claim edge here let i_will_plan legally reset the task and + # loop the submit_up -> pr_pass -> awaiting_pm_review cycle forever — + # see the CLAIM_RULES comment below). # Start StatusTransition(Status.CLAIMED, Status.IN_PROGRESS, "start", None), # Block / pause / resume @@ -454,14 +452,6 @@ _ATOMIC_ACTIONS: dict[str, ActionSpec] = { Status.AWAITING_QA, Status.AWAITING_DOCUMENTATION, Status.AWAITING_PR_REVIEW, - # A PM re-claims a review/queue task it already owns (e.g. after - # a respawn) via i_will_plan — CLAIM_RULES[CELL_PM/MAIN_PM] - # grants AWAITING_PM_REVIEW. Without it here, can_invoke_intent - # rejected i_will_plan on an awaiting_pm_review task with - # invalid_state even though CLAIM_RULES said it was allowed. - # test_claim_rules_match_pre_gateway_table keeps this source set - # and CLAIM_RULES in sync. - Status.AWAITING_PM_REVIEW, } ), target_status=Status.CLAIMED, @@ -749,21 +739,22 @@ CLAIM_RULES: dict[Role, frozenset[Status]] = { # agent its own assigned tasks. (A per-instance ownership gate at the gateway # would diverge from this spec — the parity invariant forbids that.) # - # AWAITING_PM_REVIEW: a PM re-claims its own review-queue task (e.g. after - # a respawn) via i_will_plan. roboco/services/task.py's - # _ROLE_CLAIM_STATUSES already granted this to "cell_pm"/"main_pm" on the - # stated belief that "the spec (lifecycle.CLAIM_RULES) grants it" — but - # CLAIM_RULES never actually did, so can_invoke_intent silently rejected - # every i_will_plan attempt on an awaiting_pm_review task with - # invalid_state regardless of what the service layer allowed. Added here - # to match; test_claim_rules_match_pre_gateway_table asserts CLAIM_RULES - # and the service table stay in sync so they can't diverge again. - Role.CELL_PM: frozenset( - {Status.PENDING, Status.NEEDS_REVISION, Status.AWAITING_PM_REVIEW} - ), - Role.MAIN_PM: frozenset( - {Status.PENDING, Status.NEEDS_REVISION, Status.AWAITING_PM_REVIEW} - ), + # AWAITING_PM_REVIEW is deliberately ABSENT here — do not re-add it. A task + # in this status already passed the in-path PR gate and is waiting on the + # owning PM's merge decision (complete / request_changes), not on + # re-planning. A prior revision granted CELL_PM/MAIN_PM a claim from + # AWAITING_PM_REVIEW so a respawned PM could "re-claim its own review-queue + # task", but claim composes into i_will_plan's (claim, set_plan, start) + # sequence: every respawn legally re-claimed the task, reset it to + # in_progress, and re-ran the full submit_up -> pr_pass -> + # awaiting_pm_review cycle — looping forever with no progress (one + # production task cycled 11 times across 37 spawns in 4h before this was + # caught). A PM re-entering its own AWAITING_PM_REVIEW task is now steered + # by the choreographer's i_will_plan re-entry contract (_handle_pm_reentry) + # straight to complete/request_changes, with no claim and no status change + # — closing the edge that made the reset possible in the first place. + Role.CELL_PM: frozenset({Status.PENDING, Status.NEEDS_REVISION}), + Role.MAIN_PM: frozenset({Status.PENDING, Status.NEEDS_REVISION}), Role.PRODUCT_OWNER: frozenset(), Role.HEAD_MARKETING: frozenset(), Role.AUDITOR: frozenset(), diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 9dc14257..424e40fc 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -519,7 +519,7 @@ class Choreographer: role_str: str, briefing: dict[str, Any], ) -> Envelope | None: - """Handle two distinct re-entry contracts for i_will_plan. + """Handle three distinct re-entry contracts for i_will_plan. Idempotent heartbeat: the PM already owns the task in in_progress — touch the heartbeat and return OK without re-running the spec gate. @@ -531,10 +531,20 @@ class Choreographer: re-claim (claimed is not a valid source for the claim transition) and run set_plan+start to complete the interrupted sequence. - Returns None when neither condition applies, signalling the caller to - continue to the normal claim-plan-start path. PLR0911 budget is the - secondary reason this lives in a helper; the domain contract above is - the primary one. + Review-queue steering: the task already passed the in-path PR gate and + is sitting in awaiting_pm_review awaiting this PM's merge decision. A + respawned PM re-offered this task by give_me_work used to call + i_will_plan on it, and CLAIM_RULES used to let that legally re-claim + it — running the full composed (claim, set_plan, start) sequence and + resetting the task to in_progress, which re-ran submit_up -> pr_pass + -> awaiting_pm_review forever (one production task looped 11 cycles / + 37 spawns in 4h before this was caught). Steer to complete / + request_changes instead, with NO claim and NO status change. + + Returns None when none of the three conditions applies, signalling the + caller to continue to the normal claim-plan-start path. PLR0911 budget + is the secondary reason this lives in a helper; the domain contract + above is the primary one. """ status = str(t.status) if status == "in_progress" and t.assigned_to == pm_agent_id: @@ -550,6 +560,20 @@ class Choreographer: return await self._post_claim_journal_gate( "i_will_plan", pm_agent_id, task_id, envelope ) + if status == "awaiting_pm_review" and t.assigned_to == pm_agent_id: + return Envelope.ok( + status=status, + task_id=str(task_id), + next=( + "this task already passed the PR-review gate and is" + " awaiting your merge decision — do NOT re-plan or" + " re-submit it. Call complete(task_id) to merge the" + " assembled PR, or request_changes(task_id," + " findings=[...]) to bounce it back with concrete" + " findings." + ), + context_briefing=briefing, + ).with_introspection(task=t, role=role_str) return None async def _pm_sub_tasks_gate( @@ -830,6 +854,12 @@ class Choreographer: handed an awaiting_documentation task (or QA an awaiting_qa task) was told to call a dev verb it doesn't have — it looped. Map to the verb that actually claims the task for this role. + + awaiting_pm_review is a review-queue state, not a re-plan state: a + PM offered its own already-gated task here must be steered to + complete/request_changes, never i_will_plan (i_will_plan legally + re-claiming from this status used to reset the task and loop the + submit_up -> pr_pass -> awaiting_pm_review cycle forever). """ tid = str(getattr(task, "id", "")) status = str(getattr(task, "status", "")) @@ -837,6 +867,13 @@ class Choreographer: return f"call claim_doc_task(task_id='{tid}') to start" if status == "awaiting_qa": return f"call claim_review(task_id='{tid}') to start" + if status == "awaiting_pm_review" and role in ("cell_pm", "main_pm"): + return ( + f"this task already passed the PR-review gate — call" + f" complete(task_id='{tid}') to merge, or" + f" request_changes(task_id='{tid}', findings=[...]) to bounce" + " it back; do NOT call i_will_plan" + ) if role in ("cell_pm", "main_pm", "product_owner", "head_marketing"): return f"call i_will_plan(task_id='{tid}', plan='') to start" return f"call i_will_work_on(task_id='{tid}', plan='') to start" diff --git a/roboco/services/task.py b/roboco/services/task.py index fb3dc550..8a70e67f 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -107,15 +107,26 @@ _ROLE_CLAIM_STATUSES: dict[str, set[TaskStatus]] = { # claim() returns None -> INVALID_STATE -> the PM respawn-loops on its own # rejected root, unable to plan or idle it. Parity is locked by # tests/unit/services/test_pm_claim_needs_revision.py. + # + # AWAITING_PM_REVIEW is deliberately absent: granting it here once let a + # respawned PM's i_will_plan legally re-claim its own review-queue task, + # resetting it to in_progress and re-running submit_up -> pr_pass -> + # awaiting_pm_review forever. lifecycle.CLAIM_RULES agrees, and unlike the + # NEEDS_REVISION parity above (spec ⊆ runtime only, via + # test_runtime_pm_claim_mapping_covers_spec_claim_rules), this exact + # per-PM-role SET is pinned bidirectionally against spec.CLAIM_RULES by + # test_claim_rules_and_role_statuses_are_identical — re-adding + # AWAITING_PM_REVIEW here alone (without touching the spec) fails that + # test instead of silently reopening the loop. The choreographer's + # _handle_pm_reentry now steers a re-entering PM straight to + # complete/request_changes instead, with no claim involved. "cell_pm": { TaskStatus.PENDING, TaskStatus.NEEDS_REVISION, - TaskStatus.AWAITING_PM_REVIEW, }, "main_pm": { TaskStatus.PENDING, TaskStatus.NEEDS_REVISION, - TaskStatus.AWAITING_PM_REVIEW, }, } @@ -3741,11 +3752,18 @@ class TaskService(BaseService): if task.assigned_to and str(task.assigned_to) != str(agent.id): markers.set_original_developer(task, task.assigned_to) + # Consulted only inside _finalize_claim (both its target-status write and + # its branch-failure rollback's reversal-audit check), reached only via + # claim() -> _validate_claim_preconditions -> _get_valid_claim_statuses, + # which already screens the pre-claim status against _ROLE_CLAIM_STATUSES + # per role. AWAITING_PM_REVIEW is deliberately absent: no role's + # _ROLE_CLAIM_STATUSES grants it anymore (the i_will_plan re-claim loop + # fix), so _finalize_claim can never run with that pre-claim status — + # listing it here would be dead weight, not an independent gate. _CLAIMABLE_STATUSES: ClassVar[set[TaskStatus]] = { TaskStatus.PENDING, TaskStatus.AWAITING_QA, TaskStatus.AWAITING_DOCUMENTATION, - TaskStatus.AWAITING_PM_REVIEW, } async def _claim_blocked_by_dependencies(self, task: TaskTable) -> bool: diff --git a/tests/foundation/test_lifecycle_consumer_parity.py b/tests/foundation/test_lifecycle_consumer_parity.py index 23930ad2..86011d05 100644 --- a/tests/foundation/test_lifecycle_consumer_parity.py +++ b/tests/foundation/test_lifecycle_consumer_parity.py @@ -869,6 +869,26 @@ async def test_complete_matches_spec(role: str, status: str) -> None: id=task_id, status="awaiting_ceo_approval", assigned_to=None, team="backend" ) task_svc.all_subtasks_terminal.return_value = True + # complete's merge path runs _stamp_pm_findings_verified_or_rejection, + # which opens a session.begin_nested() savepoint and reads the findings + # ledger via ReviewFindingsRepository.list_for_task (session.execute -> + # .scalars().all()). Unconfigured, both calls resolve to auto-generated + # AsyncMock children: `async with ():` fails the async + # context-manager protocol and `.scalars()` returns an + # unawaited coroutine — caught by the verb's own except Exception, but + # the never-awaited coroutine leaks a RuntimeWarning at GC time. + task_svc.session = MagicMock() + task_svc.session.execute = AsyncMock( + return_value=MagicMock( + scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[]))) + ) + ) + task_svc.session.begin_nested = MagicMock( + return_value=MagicMock( + __aenter__=AsyncMock(return_value=None), + __aexit__=AsyncMock(return_value=False), + ) + ) git_svc = AsyncMock() git_svc.pr_merge.return_value = {"merged": True, "merge_commit_sha": "x"} git_svc.create_pr.return_value = {"pr_number": 99, "pr_url": "x"} @@ -1268,6 +1288,18 @@ async def test_claim_review_matches_spec(role: str, status: str) -> None: task_svc.qa_claim.return_value = after task_svc.list_in_progress_for_agent.return_value = [] task_svc.list_paused_for_agent.return_value = [] + # claim_review's full=True briefing reads the findings ledger + # (findings.open_findings_for_task -> session.execute -> .scalars().all()). + # Unconfigured, session.execute auto-generates as an AsyncMock child whose + # call returns another AsyncMock; .scalars() on that is itself an + # unawaited coroutine — caught by open_findings_for_task's own fail-open + # except, but the dangling coroutine leaks a RuntimeWarning at GC time. + task_svc.session = MagicMock() + task_svc.session.execute = AsyncMock( + return_value=MagicMock( + scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[]))) + ) + ) deps = _make_deps(task_svc=task_svc) c = Choreographer(deps) @@ -1546,6 +1578,14 @@ async def test_claim_doc_task_matches_spec(role: str, status: str) -> None: task_svc.doc_claim.return_value = after task_svc.list_in_progress_for_agent.return_value = [] task_svc.list_paused_for_agent.return_value = [] + # claim_doc_task's full=True briefing reads the findings ledger the same + # way claim_review's does (see that test's comment) — same fix. + task_svc.session = MagicMock() + task_svc.session.execute = AsyncMock( + return_value=MagicMock( + scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[]))) + ) + ) deps = _make_deps(task_svc=task_svc) c = Choreographer(deps) diff --git a/tests/foundation/test_lifecycle_spec.py b/tests/foundation/test_lifecycle_spec.py index 238a7ce9..cfca53e1 100644 --- a/tests/foundation/test_lifecycle_spec.py +++ b/tests/foundation/test_lifecycle_spec.py @@ -491,6 +491,11 @@ def test_claim_rules_match_pre_gateway_table() -> None: re-delegating fixes (scoped by give_me_work routing, which offers only the caller's own assigned tasks). BACKLOG → PENDING is a separate `activate` action (strict transitions; no implicit activate-on-claim). + + AWAITING_PM_REVIEW is deliberately absent from both PM roles — a claim + edge there let a respawned PM's i_will_plan legally re-claim its own + review-queue task and loop the submit_up -> pr_pass -> awaiting_pm_review + cycle forever. See test_awaiting_pm_review_not_claimable_by_any_role. """ assert spec.CLAIM_RULES[spec.Role.DEVELOPER] == frozenset( {spec.Status.PENDING, spec.Status.NEEDS_REVISION} @@ -500,21 +505,26 @@ def test_claim_rules_match_pre_gateway_table() -> None: {spec.Status.PENDING, spec.Status.AWAITING_DOCUMENTATION} ) assert spec.CLAIM_RULES[spec.Role.CELL_PM] == frozenset( - { - spec.Status.PENDING, - spec.Status.NEEDS_REVISION, - spec.Status.AWAITING_PM_REVIEW, - } + {spec.Status.PENDING, spec.Status.NEEDS_REVISION} ) assert spec.CLAIM_RULES[spec.Role.MAIN_PM] == frozenset( - { - spec.Status.PENDING, - spec.Status.NEEDS_REVISION, - spec.Status.AWAITING_PM_REVIEW, - } + {spec.Status.PENDING, spec.Status.NEEDS_REVISION} ) +def test_awaiting_pm_review_not_claimable_by_any_role() -> None: + """The claim edge that let a respawned PM's i_will_plan reset an + awaiting_pm_review task (looping submit_up -> pr_pass -> awaiting_pm_review + forever) is permanently closed: no role may claim from this status. A PM + re-entering its own review task is steered by the choreographer directly + to complete/request_changes, never through claim. + """ + for role, statuses in spec.CLAIM_RULES.items(): + assert spec.Status.AWAITING_PM_REVIEW not in statuses, role + # PR_REVIEWER's own review-gate claim (a different status) is untouched. + assert spec.Status.AWAITING_PR_REVIEW in spec.CLAIM_RULES[spec.Role.PR_REVIEWER] + + def test_team_rules_pin_team_for_seeded_agents() -> None: assert spec.ROLE_TEAM_RULES["be-dev-1"] == "backend" assert spec.ROLE_TEAM_RULES["be-pm"] == "backend" diff --git a/tests/unit/enforcement/test_task_lifecycle.py b/tests/unit/enforcement/test_task_lifecycle.py index 799794d8..c998c997 100644 --- a/tests/unit/enforcement/test_task_lifecycle.py +++ b/tests/unit/enforcement/test_task_lifecycle.py @@ -277,6 +277,40 @@ def test_status_classification_is_mutually_disjoint() -> None: assert waiting & terminal == set() +# --------------------------------------------------------------------------- +# awaiting_pm_review -> claimed is closed (the i_will_plan re-claim loop): +# a PM re-entering its own review-queue task is steered by the choreographer +# straight to complete/request_changes, never via a claim that resets the +# task and re-runs submit_up -> pr_pass -> awaiting_pm_review forever. This +# legacy shim (_LEGACY_OPERATIONAL_EDGES / _LEGACY_ROLE_GATES) used to grant +# the same edge lifecycle.CLAIM_RULES had already closed — the identical +# two-tables-drift shape that caused the incident. +# --------------------------------------------------------------------------- + + +def test_awaiting_pm_review_claim_no_longer_allowed_for_pm_roles() -> None: + assert can_agent_transition("awaiting_pm_review", "claimed", "cell_pm") is False + assert can_agent_transition("awaiting_pm_review", "claimed", "main_pm") is False + with pytest.raises(TaskLifecycleError): + validate_task_transition("awaiting_pm_review", "claimed", "cell_pm") + with pytest.raises(TaskLifecycleError): + validate_task_transition("awaiting_pm_review", "claimed", "main_pm") + + +def test_awaiting_pm_review_needs_revision_still_allowed() -> None: + """The PM reject-back-to-dev path (request_changes) is untouched.""" + assert ( + can_agent_transition("awaiting_pm_review", "needs_revision", "cell_pm") is True + ) + assert ( + can_agent_transition("awaiting_pm_review", "needs_revision", "main_pm") is True + ) + assert ( + validate_task_transition("awaiting_pm_review", "needs_revision", "cell_pm") + is True + ) + + def test_status_classification_covers_every_enum_member() -> None: """Every Status enum member must be classified by EXACTLY one of is_terminal_state / is_active_state / is_waiting_state — the coverage diff --git a/tests/unit/gateway/test_claim_doc_task_checkout.py b/tests/unit/gateway/test_claim_doc_task_checkout.py index ff2ce514..b09344f2 100644 --- a/tests/unit/gateway/test_claim_doc_task_checkout.py +++ b/tests/unit/gateway/test_claim_doc_task_checkout.py @@ -86,6 +86,16 @@ def test_claim_verb_hint_pm_for_planning() -> None: assert "i_will_plan" in hint +def test_claim_verb_hint_pm_for_awaiting_pm_review_steers_to_complete() -> None: + """A PM's own review-queue task must never hint i_will_plan — that verb + used to legally re-claim and reset it, looping submit_up -> pr_pass -> + awaiting_pm_review forever.""" + for role in ("cell_pm", "main_pm"): + hint = Choreographer._claim_verb_hint(role, _task("awaiting_pm_review")) + assert "complete" in hint + assert "call i_will_plan(" not in hint + + def test_claim_verb_hint_dev_default() -> None: hint = Choreographer._claim_verb_hint("developer", _task("pending")) assert "i_will_work_on" in hint diff --git a/tests/unit/gateway/test_pm_review_reentry_loop_fix.py b/tests/unit/gateway/test_pm_review_reentry_loop_fix.py new file mode 100644 index 00000000..c56ce90e --- /dev/null +++ b/tests/unit/gateway/test_pm_review_reentry_loop_fix.py @@ -0,0 +1,148 @@ +"""PM re-entry on an awaiting_pm_review task must steer, never re-claim. + +Live incident: an awaiting_pm_review task (already past the in-path PR gate) +kept getting re-offered to its owning PM by give_me_work. The respawned PM +called i_will_plan, and CLAIM_RULES used to grant CELL_PM/MAIN_PM a claim from +AWAITING_PM_REVIEW — so the composed (claim, set_plan, start) sequence legally +reset the task to in_progress and re-ran submit_up -> pr_pass -> +awaiting_pm_review forever (one production task looped 11 cycles across 37 +spawns in 4h). ``_handle_pm_reentry`` now recognizes this status for the +owning PM and returns a steering-only OK envelope (complete / request_changes) +with no claim and no state change; CLAIM_RULES no longer permits the claim at +all, so a non-owner (or any other caller) falls through to a normal spec +rejection. +""" + +from __future__ import annotations + +from datetime import UTC, datetime +from typing import Any +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps + +# --------------------------------------------------------------------------- +# Shared fixture helpers — same pattern as test_i_will_plan_sub_tasks_gate.py +# --------------------------------------------------------------------------- + + +def _make_deps(**overrides: Any) -> ChoreographerDeps: + base: dict[str, Any] = { + "task": AsyncMock(), + "work_session": AsyncMock(), + "git": AsyncMock(), + "a2a": AsyncMock(), + "journal": AsyncMock(), + "audit": AsyncMock(), + "evidence_repo": AsyncMock(), + } + base.update(overrides) + task = base["task"] + task.session = MagicMock() + task.session.begin_nested = MagicMock( + return_value=MagicMock( + __aenter__=AsyncMock(return_value=None), + __aexit__=AsyncMock(return_value=False), + ) + ) + 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 = [] + _ldef = base["journal"].latest_decision_at.return_value + if type(_ldef).__name__ in ("MagicMock", "AsyncMock"): + base["journal"].latest_decision_at.return_value = datetime.now(UTC) + return ChoreographerDeps(**base) + + +def _review_task_svc(task_id: object, pm_id: object, *, role: str) -> AsyncMock: + """TaskService mock for a PM re-entering its own awaiting_pm_review task.""" + task_svc = AsyncMock() + task_svc.get.return_value = MagicMock( + id=task_id, + status="awaiting_pm_review", + plan={"text": "already planned"}, + assigned_to=pm_id, + task_type="planning", + parent_task_id=None, + sequence=0, + team="backend", + commits=["abc123"], + pr_number=42, + branch_name="feature/backend/abc", + quick_context=None, + ) + task_svc.agent_for.return_value = MagicMock( + id=pm_id, role=role, team="backend", slug=None + ) + 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.session = MagicMock() + task_svc.session.begin_nested = MagicMock( + return_value=MagicMock( + __aenter__=AsyncMock(return_value=None), + __aexit__=AsyncMock(return_value=False), + ) + ) + return task_svc + + +async def _assert_steers_without_reclaiming(role: str) -> None: + pm_id = uuid4() + task_id = uuid4() + task_svc = _review_task_svc(task_id, pm_id, role=role) + deps = _make_deps(task=task_svc) + c = Choreographer(deps) + + env = await c.i_will_plan(pm_id, task_id, plan="resume") + body = env.as_dict() + + assert body.get("error") is None, body + assert body["status"] == "awaiting_pm_review", body + assert "complete" in body["next"], body + + task_svc.claim.assert_not_awaited() + task_svc.set_plan.assert_not_awaited() + task_svc.start.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_cell_pm_reentry_awaiting_pm_review_steers_to_complete() -> None: + await _assert_steers_without_reclaiming("cell_pm") + + +@pytest.mark.asyncio +async def test_main_pm_reentry_awaiting_pm_review_steers_to_complete() -> None: + await _assert_steers_without_reclaiming("main_pm") + + +@pytest.mark.asyncio +async def test_non_owner_awaiting_pm_review_is_rejected_not_reclaimed() -> None: + """A PM that does NOT own the review task gets the normal spec rejection — + CLAIM_RULES no longer grants a claim from awaiting_pm_review to anyone, so + this falls straight through to invalid_state instead of resetting the task. + """ + pm_id = uuid4() + other_pm_id = uuid4() + task_id = uuid4() + task_svc = _review_task_svc(task_id, other_pm_id, role="cell_pm") + deps = _make_deps(task=task_svc) + c = Choreographer(deps) + + env = await c.i_will_plan(pm_id, task_id, plan="resume") + body = env.as_dict() + + assert body.get("error") == "invalid_state", body + task_svc.claim.assert_not_awaited() + task_svc.set_plan.assert_not_awaited() + task_svc.start.assert_not_awaited() diff --git a/tests/unit/services/test_pm_claim_needs_revision.py b/tests/unit/services/test_pm_claim_needs_revision.py index 963062e1..d95ec57a 100644 --- a/tests/unit/services/test_pm_claim_needs_revision.py +++ b/tests/unit/services/test_pm_claim_needs_revision.py @@ -22,7 +22,11 @@ from typing import TYPE_CHECKING, cast import pytest from roboco.foundation.policy import lifecycle as spec from roboco.models.base import TaskStatus -from roboco.services.task import _default_claim_statuses, _get_valid_claim_statuses +from roboco.services.task import ( + _ROLE_CLAIM_STATUSES, + _default_claim_statuses, + _get_valid_claim_statuses, +) if TYPE_CHECKING: from roboco.db.tables import AgentTable @@ -52,3 +56,26 @@ def test_runtime_pm_claim_mapping_covers_spec_claim_rules(role: spec.Role) -> No f"runtime claim mapping for {role.value} is missing spec-allowed " f"status '{status.value}'" ) + + +@pytest.mark.parametrize("role", [spec.Role.CELL_PM, spec.Role.MAIN_PM]) +def test_claim_rules_and_role_statuses_are_identical(role: spec.Role) -> None: + """Genuine bidirectional cross-check between the two claim tables. + + ``test_runtime_pm_claim_mapping_covers_spec_claim_rules`` above only + checks spec ⊆ runtime — it would still pass if ``_ROLE_CLAIM_STATUSES`` + carried an EXTRA status the spec doesn't grant (e.g. AWAITING_PM_REVIEW + re-added to the runtime table alone, with lifecycle.CLAIM_RULES left + untouched). That silent one-sided drift — "the service table already + granted this on the belief that the spec granted it too" — is exactly the + shape that caused the awaiting_pm_review re-claim loop + (``lifecycle.py``'s ``CLAIM_RULES`` comment covers the incident). This + test imports both tables directly and asserts the per-role sets are + IDENTICAL, not just one-way-covering. + """ + spec_values = {s.value for s in spec.CLAIM_RULES[role]} + runtime_values = {s.value for s in _ROLE_CLAIM_STATUSES[role.value]} + assert spec_values == runtime_values, ( + f"spec.CLAIM_RULES[{role.value}]={spec_values} != " + f"task._ROLE_CLAIM_STATUSES[{role.value!r}]={runtime_values}" + )