mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[chore] admin_set_status: attribute the blocked-restore to the admin actor + emit override row (#2176)
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.
This commit is contained in:
+61
-7
@@ -2197,7 +2197,16 @@ class TaskService(BaseService):
|
|||||||
and new_status in (TaskStatus.PENDING, TaskStatus.IN_PROGRESS)
|
and new_status in (TaskStatus.PENDING, TaskStatus.IN_PROGRESS)
|
||||||
and task.pre_block_assignee is not None
|
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
|
task.status = new_status
|
||||||
await self.session.flush()
|
await self.session.flush()
|
||||||
self._emit_status_transition_audit(
|
self._emit_status_transition_audit(
|
||||||
@@ -8460,7 +8469,13 @@ class TaskService(BaseService):
|
|||||||
return await self._apply_pre_block_restore(task, restored_status)
|
return await self._apply_pre_block_restore(task, restored_status)
|
||||||
|
|
||||||
async def _apply_pre_block_restore(
|
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:
|
) -> TaskTable:
|
||||||
"""Restore a blocked task to its snapshotted status + owner.
|
"""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
|
emits the audit explicitly, applies the branchless guard legacy
|
||||||
unblock() relies on, restores ownership from the snapshot, and clears
|
unblock() relies on, restores ownership from the snapshot, and clears
|
||||||
the pre-block snapshot fields.
|
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
|
# A task with no branch cannot resume in_progress — the dispatcher
|
||||||
# refuses a branchless in_progress task and loops — so divert it to
|
# refuses a branchless in_progress task and loops — so divert it to
|
||||||
@@ -8493,15 +8516,46 @@ class TaskService(BaseService):
|
|||||||
await self.session.flush()
|
await self.session.flush()
|
||||||
# This restore path sets the status directly (bypassing the strict
|
# This restore path sets the status directly (bypassing the strict
|
||||||
# transition validator), so emit the audit explicitly — no status
|
# transition validator), so emit the audit explicitly — no status
|
||||||
# change may skip the audit log. A restore to a snapshotted
|
# change may skip the audit log. Attribute to the admin actor when
|
||||||
# needs_revision is the same rework cycle resuming, not a fresh
|
# present (admin_set_status); else the restored owner (in-band unblock).
|
||||||
# rejection, so undo the bump the chokepoint applied above.
|
# 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(
|
self._emit_status_transition_audit(
|
||||||
task,
|
task,
|
||||||
from_status=pre_status,
|
from_status=pre_status,
|
||||||
to_status=restored_status.value,
|
to_status=restored_status.value,
|
||||||
agent_role=None,
|
agent_role=actor_role,
|
||||||
audit_agent_id=restored_owner,
|
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 (
|
if (
|
||||||
restored_status == TaskStatus.NEEDS_REVISION
|
restored_status == TaskStatus.NEEDS_REVISION
|
||||||
|
|||||||
@@ -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)
|
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
|
@pytest.mark.asyncio
|
||||||
async def test_pre_block_restore_skips_revision_count_bump() -> None:
|
async def test_pre_block_restore_skips_revision_count_bump() -> None:
|
||||||
"""#101 Gap B: restoring a blocked task to its snapshotted needs_revision
|
"""#101 Gap B: restoring a blocked task to its snapshotted needs_revision
|
||||||
|
|||||||
Reference in New Issue
Block a user