From 20f1f9ba25b7b52b009fad061801e13f75e4f357 Mon Sep 17 00:00:00 2001 From: Renn F Date: Tue, 30 Jun 2026 19:23:05 +0200 Subject: [PATCH] [chore] admin_set_status: attribute the blocked-restore to the admin actor + emit override row (#2176) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit admin_set_status taking a BLOCKED task to pending/in_progress with a pre-block snapshot returned early via _apply_pre_block_restore, which emitted its audit row with agent_role=None and audit_agent_id=restored_owner (the pre-block dev) — the admin actor_id/actor_role were dropped entirely. Because this branch runs with force=false (pending/in_progress aren't hatch destinations), the distinguishing task.admin_override row (written only on the non-restore path, gated by force) was never written, so an operator could silently re-own a blocked task with no trace of who triggered it. Thread actor_id/actor_role into _apply_pre_block_restore (admin_set_status passes them with admin_override=True) so the transition audit row attributes the re-owning to the admin, and emit a task.admin_override row (forced=False, restore=True) on this branch independent of the force flag. The in-band unblock(restore=True) path passes no actor and keeps the legacy attribution (restored owner) with no override row. Test: admin PATCH status=pending on a BLOCKED task with a snapshot attributes every audit row to the admin (not the restored dev) and emits the override row. --- roboco/services/task.py | 68 ++++++++++++++++++++++++++++---- tests/unit/services/test_task.py | 48 ++++++++++++++++++++++ 2 files changed, 109 insertions(+), 7 deletions(-) diff --git a/roboco/services/task.py b/roboco/services/task.py index 174c0659..65b31711 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -2197,7 +2197,16 @@ class TaskService(BaseService): and new_status in (TaskStatus.PENDING, TaskStatus.IN_PROGRESS) and task.pre_block_assignee is not None ): - return await self._apply_pre_block_restore(task, new_status) + # Thread the admin actor into the restore so the audit attributes + # the re-owning to the admin (not the restored owner) and an + # admin_override row is emitted (#2176). + return await self._apply_pre_block_restore( + task, + new_status, + actor_id=actor_id, + actor_role=actor_role, + admin_override=True, + ) task.status = new_status await self.session.flush() self._emit_status_transition_audit( @@ -8460,7 +8469,13 @@ class TaskService(BaseService): return await self._apply_pre_block_restore(task, restored_status) async def _apply_pre_block_restore( - self, task: TaskTable, restored_status: TaskStatus + self, + task: TaskTable, + restored_status: TaskStatus, + *, + actor_id: str | UUID | None = None, + actor_role: str | None = None, + admin_override: bool = False, ) -> TaskTable: """Restore a blocked task to its snapshotted status + owner. @@ -8468,6 +8483,14 @@ class TaskService(BaseService): emits the audit explicitly, applies the branchless guard legacy unblock() relies on, restores ownership from the snapshot, and clears the pre-block snapshot fields. + + When reached from ``admin_set_status`` (``admin_override=True``) the + admin caller's ``actor_id``/``actor_role`` stamp the audit row — the + re-owning of the task must record WHO triggered it, not the restored + owner — and a ``task.admin_override`` row is emitted so the override is + distinguishable from an in-band ``unblock(restore=True)`` (#2176). The + in-band unblock path passes no actor and keeps the legacy attribution + (the restored owner) with no override row. """ # A task with no branch cannot resume in_progress — the dispatcher # refuses a branchless in_progress task and loops — so divert it to @@ -8493,16 +8516,47 @@ class TaskService(BaseService): await self.session.flush() # This restore path sets the status directly (bypassing the strict # transition validator), so emit the audit explicitly — no status - # change may skip the audit log. A restore to a snapshotted - # needs_revision is the same rework cycle resuming, not a fresh - # rejection, so undo the bump the chokepoint applied above. + # change may skip the audit log. Attribute to the admin actor when + # present (admin_set_status); else the restored owner (in-band unblock). + # A restore to a snapshotted needs_revision is the same rework cycle + # resuming, not a fresh rejection, so undo the bump the chokepoint + # applied above. self._emit_status_transition_audit( task, from_status=pre_status, to_status=restored_status.value, - agent_role=None, - audit_agent_id=restored_owner, + agent_role=actor_role, + audit_agent_id=actor_id if actor_id is not None else restored_owner, ) + if admin_override: + # #2176: an admin-triggered restore is an explicit override past the + # lifecycle gate — stamp it as such so the re-owning is traceable to + # the admin, independent of the force flag (this branch runs with + # force=false because pending/in_progress aren't hatch destinations). + from roboco.db.tables import AuditLogTable + + agent_uuid: UUID | None = None + if actor_id is not None: + try: + agent_uuid = UUID(str(actor_id)) + except (ValueError, AttributeError): + agent_uuid = None + self.session.add( + AuditLogTable( + event_type="task.admin_override", + agent_id=agent_uuid, + target_type="task", + target_id=task.id, + severity="warning", + details={ + "from_status": pre_status, + "to_status": restored_status.value, + "agent_role": actor_role, + "forced": False, + "restore": True, + }, + ) + ) if ( restored_status == TaskStatus.NEEDS_REVISION and pre_status != TaskStatus.NEEDS_REVISION.value diff --git a/tests/unit/services/test_task.py b/tests/unit/services/test_task.py index 838f7e51..dcb58769 100644 --- a/tests/unit/services/test_task.py +++ b/tests/unit/services/test_task.py @@ -652,6 +652,54 @@ async def test_admin_set_status_no_force_emits_no_override_audit_row() -> None: assert not any(r.event_type == "task.admin_override" for r in rows) +@pytest.mark.asyncio +async def test_admin_set_status_blocked_restore_attributes_admin_actor() -> None: + """#2176: an admin restore out of BLOCKED (with a pre-block snapshot) must + stamp the ADMIN actor on the audit rows — not the restored owner — and emit + a task.admin_override row so the re-owning is traceable. Previously this + branch returned early via _apply_pre_block_restore with agent_role=None and + audit_agent_id=restored_owner, and (force=false here) wrote no override row, + so the privilege use was untraceable.""" + dev = uuid4() + pm = uuid4() + admin = uuid4() + task = _build_task( + status=TaskStatus.BLOCKED, + assigned_to=pm, + claimed_by=pm, + branch_name="feature/backend/abc--def", + pre_block_state="in_progress", + pre_block_assignee=dev, + ) + added: list[object] = [] + session = MagicMock() + session.flush = AsyncMock() + session.add.side_effect = added.append + svc = TaskService(session) + _bind(svc, "get", AsyncMock(return_value=task)) + + # force defaults to False — pending is not a hatch destination, so the only + # way this override is recorded is the restore branch's own override row. + out = await svc.admin_set_status( + task.id, TaskStatus.PENDING, actor_id=admin, actor_role="ceo" + ) + assert out is task + assert task.status == TaskStatus.PENDING + assert task.assigned_to == dev # ownership restored to the pre-block dev + + rows = [r for r in added if isinstance(r, AuditLogTable)] + override_rows = [r for r in rows if r.event_type == "task.admin_override"] + assert len(override_rows) == 1 + override = override_rows[0] + assert override.agent_id == admin # the admin, not the restored dev + assert override.details["restore"] is True + assert override.details["forced"] is False + # Every audit row (the transition row AND the override row) is attributed to + # the admin actor — the restored owner is NOT recorded as the actor. + assert all(r.agent_id == admin for r in rows) + assert not any(r.agent_id == dev for r in rows) + + @pytest.mark.asyncio async def test_pre_block_restore_skips_revision_count_bump() -> None: """#101 Gap B: restoring a blocked task to its snapshotted needs_revision