mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix: read the real commit key (hash) and audit the restore-unblock path
- The auto-pause checkpoint and _extract_first_commit_sha read commit dicts by key 'sha', but persisted commits are keyed 'hash' (CommitRef.hash) — the prior change stopped the crash but silently dropped every ref. Read 'hash' (sha fallback) at both sites; the test now uses the production dict shape so the regression can't hide. - unblock_with_restore set status directly and skipped the audit log; emit the status-transition audit there too, like the other direct-set paths.
This commit is contained in:
@@ -1503,14 +1503,14 @@ class Choreographer:
|
|||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def _extract_first_commit_sha(t: Any) -> str | None:
|
def _extract_first_commit_sha(t: Any) -> str | None:
|
||||||
"""Read the first commit sha off the task, dict or model alike."""
|
"""Read the first commit hash off the task, dict or model alike."""
|
||||||
commits: list[Any] = list(getattr(t, "commits", []) or [])
|
commits: list[Any] = list(getattr(t, "commits", []) or [])
|
||||||
if not commits:
|
if not commits:
|
||||||
return None
|
return None
|
||||||
first = commits[0]
|
first = commits[0]
|
||||||
if isinstance(first, dict):
|
if isinstance(first, dict):
|
||||||
return first.get("sha")
|
return first.get("hash") or first.get("sha")
|
||||||
return getattr(first, "sha", None)
|
return getattr(first, "hash", None) or getattr(first, "sha", None)
|
||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def _already_addressed_criteria(existing_status: list[dict[str, Any]]) -> set[str]:
|
def _already_addressed_criteria(existing_status: list[dict[str, Any]]) -> set[str]:
|
||||||
@@ -2710,9 +2710,12 @@ class Choreographer:
|
|||||||
try:
|
try:
|
||||||
commits = task.commits or []
|
commits = task.commits or []
|
||||||
# commits may be hydrated as CommitRef objects or as plain dicts
|
# commits may be hydrated as CommitRef objects or as plain dicts
|
||||||
# (JSON column round-trip); tolerate both rather than assuming `.sha`.
|
# (JSON column round-trip); the identifier field is `hash` (a stray
|
||||||
|
# `sha` only ever appears on a gateway return value, never persisted).
|
||||||
commit_refs = [
|
commit_refs = [
|
||||||
c.get("sha") if isinstance(c, dict) else getattr(c, "sha", None)
|
(c.get("hash") or c.get("sha"))
|
||||||
|
if isinstance(c, dict)
|
||||||
|
else (getattr(c, "hash", None) or getattr(c, "sha", None))
|
||||||
for c in commits[-3:]
|
for c in commits[-3:]
|
||||||
]
|
]
|
||||||
commit_refs = [ref for ref in commit_refs if ref]
|
commit_refs = [ref for ref in commit_refs if ref]
|
||||||
|
|||||||
@@ -5643,6 +5643,12 @@ class TaskService(BaseService):
|
|||||||
except ValueError:
|
except ValueError:
|
||||||
return await self.unblock(task_id, agent_role="cell_pm")
|
return await self.unblock(task_id, agent_role="cell_pm")
|
||||||
|
|
||||||
|
pre_status = (
|
||||||
|
task.status.value
|
||||||
|
if isinstance(task.status, TaskStatus)
|
||||||
|
else str(task.status)
|
||||||
|
)
|
||||||
|
restored_owner = cast("Any", task.pre_block_assignee or task.claimed_by)
|
||||||
task.status = restored_status
|
task.status = restored_status
|
||||||
if task.pre_block_assignee:
|
if task.pre_block_assignee:
|
||||||
task.assigned_to = cast("Any", task.pre_block_assignee)
|
task.assigned_to = cast("Any", task.pre_block_assignee)
|
||||||
@@ -5653,6 +5659,16 @@ class TaskService(BaseService):
|
|||||||
task.blocker_resolver_type = None
|
task.blocker_resolver_type = None
|
||||||
task.blocker_raised_by = None
|
task.blocker_raised_by = None
|
||||||
await self.session.flush()
|
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.
|
||||||
|
self._emit_status_transition_audit(
|
||||||
|
task,
|
||||||
|
from_status=pre_status,
|
||||||
|
to_status=restored_status.value,
|
||||||
|
agent_role=None,
|
||||||
|
audit_agent_id=restored_owner,
|
||||||
|
)
|
||||||
return task
|
return task
|
||||||
|
|
||||||
async def cell_pm_complete(
|
async def cell_pm_complete(
|
||||||
|
|||||||
@@ -144,7 +144,10 @@ async def test_i_am_idle_with_commits_includes_last_three_in_remaining_work() ->
|
|||||||
agent_id = uuid4()
|
agent_id = uuid4()
|
||||||
task_id = uuid4()
|
task_id = uuid4()
|
||||||
|
|
||||||
commits = [MagicMock(sha=f"sha{i}") for i in range(5)]
|
# Production shape: task.commits is a JSON list[dict] keyed by `hash`
|
||||||
|
# (CommitRef.hash) — NOT `sha`. A prior fix read `sha` and silently lost
|
||||||
|
# every ref; this test uses the real shape so the regression can't hide.
|
||||||
|
commits = [{"hash": f"hash{i}", "message": f"c{i}"} for i in range(5)]
|
||||||
task_obj = MagicMock()
|
task_obj = MagicMock()
|
||||||
task_obj.id = task_id
|
task_obj.id = task_id
|
||||||
task_obj.status = "in_progress"
|
task_obj.status = "in_progress"
|
||||||
@@ -163,12 +166,10 @@ async def test_i_am_idle_with_commits_includes_last_three_in_remaining_work() ->
|
|||||||
|
|
||||||
call_kwargs = task_svc.add_checkpoint.await_args
|
call_kwargs = task_svc.add_checkpoint.await_args
|
||||||
remaining = call_kwargs.kwargs.get("remaining_work", [])
|
remaining = call_kwargs.kwargs.get("remaining_work", [])
|
||||||
# Last 3 commit SHAs should appear somewhere in remaining_work entries
|
# Last 3 commit hashes (the real persisted key) must appear in remaining_work.
|
||||||
last_3_shas = {c.sha for c in commits[-3:]}
|
last_3 = {c["hash"] for c in commits[-3:]}
|
||||||
mentioned_shas = {entry for entry in remaining if isinstance(entry, str)}
|
mentioned = {entry for entry in remaining if isinstance(entry, str)}
|
||||||
assert last_3_shas & mentioned_shas or any(
|
assert last_3 & mentioned or any(h in str(remaining) for h in last_3)
|
||||||
sha in str(remaining) for sha in last_3_shas
|
|
||||||
)
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
|
|||||||
@@ -393,3 +393,32 @@ async def test_apply_escalation_emits_blocked_audit_event() -> None:
|
|||||||
assert kwargs["event_type"] == "task.blocked"
|
assert kwargs["event_type"] == "task.blocked"
|
||||||
assert kwargs["details"]["from_status"] == "in_progress"
|
assert kwargs["details"]["from_status"] == "in_progress"
|
||||||
assert kwargs["details"]["to_status"] == "blocked"
|
assert kwargs["details"]["to_status"] == "blocked"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_unblock_with_restore_emits_audit_event() -> None:
|
||||||
|
"""The PM restore path sets status directly (bypassing the validated
|
||||||
|
transition) and used to skip the audit log; it must record the transition."""
|
||||||
|
svc = _service()
|
||||||
|
task = MagicMock(
|
||||||
|
id=uuid4(),
|
||||||
|
status=TaskStatus.BLOCKED,
|
||||||
|
pre_block_state="in_progress",
|
||||||
|
pre_block_assignee=None,
|
||||||
|
claimed_by=uuid4(),
|
||||||
|
)
|
||||||
|
_bind(svc, "get", AsyncMock(return_value=task))
|
||||||
|
audit_mock = MagicMock(log_task_event=AsyncMock())
|
||||||
|
|
||||||
|
with patch("roboco.services.audit.get_audit_service", return_value=audit_mock):
|
||||||
|
await svc.unblock_with_restore(uuid4(), uuid4(), restore=True)
|
||||||
|
pending = list(svc._background_tasks)
|
||||||
|
if pending:
|
||||||
|
await asyncio.gather(*pending, return_exceptions=True)
|
||||||
|
|
||||||
|
assert task.status == TaskStatus.IN_PROGRESS
|
||||||
|
audit_mock.log_task_event.assert_awaited_once()
|
||||||
|
kwargs = audit_mock.log_task_event.await_args.kwargs
|
||||||
|
assert kwargs["event_type"] == "task.in_progress"
|
||||||
|
assert kwargs["details"]["from_status"] == "blocked"
|
||||||
|
assert kwargs["details"]["to_status"] == "in_progress"
|
||||||
|
|||||||
Reference in New Issue
Block a user