mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(task): drop cell_pm→main_pm auto-escalation in complete (#178)
`service.complete()` ran a two-tier approval chain
(`_apply_complete_approval_chain`): when a Cell PM completed an
`awaiting_pm_review` task, `_handle_cell_pm_escalation` silently
reassigned `task.assigned_to = main_pm.id` (+ `claimed_by`) and kept
the task in `awaiting_pm_review` for a second-tier review by Main PM.
That model is incompatible with the gateway's `main_pm_complete`,
which explicitly rejects any non-root task
(`if t.parent_task_id is not None: return invalid_state("main_pm
complete only operates on root tasks")` —
choreographer/_impl.py:3860). Result: the leaf got handed to main-pm
with no verb that could advance it → permanent wedge.
Observed end-to-end this session (smoke run 02:25–02:35):
- 02:25:56 leaf → awaiting_pm_review (correctly assigned to be-pm via
notify_pm_of_docs_complete).
- 02:26:42 be-pm cell_pm_complete REJECTED tracing_gap journal:reflect
(proves leaf IS assigned to be-pm).
- ~02:27 (silent — success-path is INFO, filtered): be-pm wrote the
reflect note + retried → choreographer cell_pm_complete → git.pr_merge
→ service.cell_pm_complete → service.complete(agent=be-pm) →
_apply_complete_approval_chain → _handle_cell_pm_escalation →
task.assigned_to = main_pm.id (no `task.reassigned` audit because the
event goes to `_emit_task_event(EventType.TASK_ESCALATED_TO_MAIN_PM)`,
not the gateway audit log).
- 02:27:30 _dispatch_pm_review_work (orchestrator) saw leaf with
assigned_to=main-pm → spawned main-pm against the leaf (target_id
in audit_log confirms it: `target_id=f3bdd585 agent_slug=main-pm`).
- 02:28:21+ be-pm cell_pm_complete → not_authorized "not assigned to
you". main-pm main_pm_complete → invalid_state "only operates on
root tasks". Closure dispatcher cycled both PMs to budget-reap.
Fix: remove the cell_pm branch from `_apply_complete_approval_chain`.
Cell PM completing a non-root awaiting_pm_review task now transitions
it to COMPLETED (the gateway model). Cell→main escalation, when
intended, uses the dedicated `submit_up` verb on the cell-level parent,
not `complete`. The main_pm → CEO branch for root parents stays.
Removed the now-dead `_handle_cell_pm_escalation` helper and the now-
unused `agent_id` parameter on `_apply_complete_approval_chain`.
Tests inverted: `test_complete_cell_pm_escalates_to_main_pm` →
`test_complete_cell_pm_does_not_escalate_to_main_pm` (asserts
status=COMPLETED, assigned_to != main_pm.id). The no-Main-PM-fallback
test trivially still passes (the path is now the only path).
Lifecycle test comment updated. make quality green.
This commit is contained in:
@@ -619,18 +619,13 @@ async def test_pm_complete_simple_task(
|
||||
) -> None:
|
||||
"""awaiting_pm_review → cell_pm complete → completed.
|
||||
|
||||
With no Main PM seeded, ``_handle_cell_pm_escalation`` short-circuits
|
||||
and the task transitions straight to COMPLETED instead of being
|
||||
handed up to the Main PM. This is the "simple task" path the plan
|
||||
calls out — no parent task, no Main PM, no CEO escalation.
|
||||
|
||||
Test isolation note: ``test_groups_routes.py`` exercises the
|
||||
groups POST endpoint, which commits via ``db.commit()`` and
|
||||
persists a MAIN_PM agent across sessions in the test DB. We
|
||||
delete any pre-existing MAIN_PM rows at the top of this test so
|
||||
the cell PM completion is the only one in play. The delete runs
|
||||
inside this test's session and is unwound by the conftest's
|
||||
rollback, so committed state in the shared DB is untouched.
|
||||
Post-#178: cell PM completing a non-root awaiting_pm_review task
|
||||
always transitions straight to COMPLETED — there is no longer a
|
||||
cell→main escalation in ``complete`` (the old branch is gone; the
|
||||
cell→main hand-off, when intended, uses ``submit_up``). The
|
||||
MAIN_PM deletion below is now a historical artifact (kept because
|
||||
other tests in this file rely on the same isolation pattern); it
|
||||
no longer affects this test's outcome.
|
||||
"""
|
||||
await db_session.execute(
|
||||
delete(AgentTable).where(AgentTable.role == AgentRole.MAIN_PM)
|
||||
|
||||
@@ -1090,19 +1090,23 @@ async def test_complete_in_progress_for_own_task(
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_complete_cell_pm_escalates_to_main_pm(
|
||||
async def test_complete_cell_pm_does_not_escalate_to_main_pm(
|
||||
task_setup: dict, db_session: AsyncSession
|
||||
) -> None:
|
||||
"""When a Cell PM completes a task, it gets escalated to Main PM."""
|
||||
"""#178: cell_pm completing an awaiting_pm_review non-root task
|
||||
transitions it to COMPLETED and does NOT reassign to main_pm.
|
||||
|
||||
Pre-#178 the cell_pm branch of ``_apply_complete_approval_chain``
|
||||
reassigned every awaiting_pm_review task to main_pm and kept it in
|
||||
awaiting_pm_review for a second-tier review — but the gateway's
|
||||
``main_pm_complete`` rejects every non-root task
|
||||
(``parent_task_id IS NOT NULL`` → invalid_state), so main_pm had
|
||||
no verb to advance it. Result: the leaf was permanently wedged
|
||||
(observed end-to-end this session). The fix removes the cell_pm
|
||||
escalation branch; cell PM now completes non-root tasks directly,
|
||||
and cell→main escalation, when intended, uses ``submit_up``.
|
||||
"""
|
||||
svc = task_setup["svc"]
|
||||
# Strip leaked Main PMs so the picked Main PM is the one we're seeding.
|
||||
# `_handle_cell_pm_escalation` orders by created_at and would otherwise
|
||||
# pick a leaked-from-prior-test Main PM with an older timestamp.
|
||||
await db_session.execute(
|
||||
AgentTable.__table__.update()
|
||||
.where(AgentTable.role == AgentRole.MAIN_PM)
|
||||
.values(role=AgentRole.SYSTEM)
|
||||
)
|
||||
cell_pm = AgentTable(
|
||||
id=uuid4(),
|
||||
name="CellPM",
|
||||
@@ -1133,31 +1137,25 @@ async def test_complete_cell_pm_escalates_to_main_pm(
|
||||
await db_session.flush()
|
||||
task = await svc.create(_req(task_setup))
|
||||
task.status = TaskStatus.AWAITING_PM_REVIEW
|
||||
task.assigned_to = cell_pm.id
|
||||
await db_session.flush()
|
||||
out = await svc.complete(task.id, agent_id=cell_pm.id)
|
||||
assert out is not None
|
||||
# Escalated — task reassigned to main_pm
|
||||
assert out.assigned_to == main_pm.id
|
||||
assert out.status == TaskStatus.AWAITING_PM_REVIEW
|
||||
assert out.status == TaskStatus.COMPLETED
|
||||
assert out.assigned_to != main_pm.id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_complete_cell_pm_no_main_pm_falls_through(
|
||||
async def test_complete_cell_pm_no_main_pm_completes(
|
||||
task_setup: dict, db_session: AsyncSession
|
||||
) -> None:
|
||||
"""If no Main PM exists, escalation returns None and chain falls through.
|
||||
|
||||
Other tests in earlier modules may have committed Main PMs that the
|
||||
rollback fixture can't undo (commits stick). Delete any inside this
|
||||
test's transaction so the rollback restores them at teardown — within
|
||||
this test they appear absent.
|
||||
"""#178: cell_pm completing awaiting_pm_review transitions to
|
||||
COMPLETED regardless of whether a Main PM exists. (Pre-#178 this
|
||||
test guarded the "no Main PM → escalation returns None → falls
|
||||
through to completion" fallback; post-#178 the cell_pm escalation
|
||||
branch is gone entirely, so this path is the only path.)
|
||||
"""
|
||||
svc = task_setup["svc"]
|
||||
await db_session.execute(
|
||||
AgentTable.__table__.update()
|
||||
.where(AgentTable.role == AgentRole.MAIN_PM)
|
||||
.values(role=AgentRole.SYSTEM)
|
||||
)
|
||||
cell_pm = AgentTable(
|
||||
id=uuid4(),
|
||||
name="CellPM",
|
||||
|
||||
Reference in New Issue
Block a user