mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(lifecycle): stop PMs re-claiming tasks out of the closure queue — kills the i_will_plan review loop
A cell/main PM's i_will_plan could legally re-claim its own task from awaiting_pm_review (a CLAIM_RULES edge added for post-respawn recovery), resetting the task to in_progress and re-running submit_up -> pr_pass -> awaiting_pm_review forever: one Sentinel conventions child looped eleven full laps in four hours (14 reviews on one PR, 37 agent spawns) while its root's closure check fired eighteen times and the Main PM could never close anything. The claim edge is gone from every table that carried it — CLAIM_RULES, the claim ActionSpec's source statuses, the StatusTransition row, the service-layer _ROLE_CLAIM_STATUSES twin, and the legacy enforcement shim's operational-edge/role-gate entries (left divergent, it would be the same silent two-table drift that produced this bug). The respawn case the edge existed for is now served properly: _handle_pm_reentry gained a third contract — a PM calling i_will_plan on its own awaiting_pm_review task gets a steering envelope (no claim, no state change) pointing at complete/request_changes, and give_me_work's next hint for that status says the same instead of steering back into i_will_plan. The no-transition review-claim path and the pm-review dispatch prompt were already correct and are untouched, so closure still converges through them. A new bidirectional test asserts CLAIM_RULES and _ROLE_CLAIM_STATUSES stay identical per PM role (the old comment claimed a sync test existed; it checked one direction only). Lifecycle artifacts regenerated; the parity suite's three unshaped session mocks fixed, zero AsyncMock warnings remain.
This commit is contained in:
@@ -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 |
|
||||
|
||||
@@ -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": [
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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='<plan>') to start"
|
||||
return f"call i_will_work_on(task_id='{tid}', plan='<plan>') to start"
|
||||
|
||||
+21
-3
@@ -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:
|
||||
|
||||
@@ -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 <AsyncMock>():` fails the async
|
||||
# context-manager protocol and `<AsyncMock-result>.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)
|
||||
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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()
|
||||
@@ -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}"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user