mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(gateway): journal task_id auto-injection works from blocked/paused
Smoke-5 root cause. Agents wrote 5 decisions / 8 reflections / 1 struggle
during the run — every single entry persisted with task_id=NULL. The C8
tracing gate then never saw them and PMs spiraled forever on
'missing: journal:decision' while their decisions sat orphaned.
Cause: ContentActions.note/say/dm/notify called
TaskService.get_active_task_for_agent for task_id auto-injection. That
helper filters to _DEV_ACTIVE_STATUSES = {claimed, in_progress,
verifying, awaiting_qa, awaiting_documentation}. BLOCKED, PAUSED, and
NEEDS_REVISION fall outside that set — so the moment an agent gets
stuck (which is exactly when they journal), auto-injection returns None
and the entry persists without task_id.
Fix:
- New TaskService.get_journal_context_task_for_agent — same shape as
get_active_task_for_agent but the status set
_JOURNAL_CONTEXT_STATUSES adds BLOCKED, PAUSED, NEEDS_REVISION.
- ContentActions.note/say/dm/notify use the new lookup.
- ContentActions.commit keeps the narrow get_active_task_for_agent —
can't commit from blocked, so the dev-active set is correct there.
Tests:
- tests/unit/services/test_journal_context_lookup.py — 5 tests pinning
the two queries: journal-context INCLUDES blocked/paused/needs_revision,
dev-active EXCLUDES them.
- Existing content-actions tests updated to stub the new method
alongside the old one.
This alone may be 70% of what was killing smoke runs end-to-end.
This commit is contained in:
@@ -320,8 +320,8 @@ class ContentActions:
|
|||||||
"""Gate Set D: refuse content posts on tasks the caller does not own.
|
"""Gate Set D: refuse content posts on tasks the caller does not own.
|
||||||
|
|
||||||
Only call this for *explicit* task_id (caller passed it themselves).
|
Only call this for *explicit* task_id (caller passed it themselves).
|
||||||
Auto-fill from get_active_task_for_agent is implicitly self-owned
|
Auto-fill from get_journal_context_task_for_agent is implicitly
|
||||||
and does not need a re-check.
|
self-owned and does not need a re-check.
|
||||||
|
|
||||||
Allows ``assigned_to=None`` (post-handoff transient state) so QA /
|
Allows ``assigned_to=None`` (post-handoff transient state) so QA /
|
||||||
documenter can still inspect tasks between reassignments.
|
documenter can still inspect tasks between reassignments.
|
||||||
@@ -365,7 +365,7 @@ class ContentActions:
|
|||||||
if reject := await self._verify_explicit_task_ownership(agent_id, task_id):
|
if reject := await self._verify_explicit_task_ownership(agent_id, task_id):
|
||||||
return reject
|
return reject
|
||||||
else:
|
else:
|
||||||
t = await self.task.get_active_task_for_agent(agent_id)
|
t = await self.task.get_journal_context_task_for_agent(agent_id)
|
||||||
if t is not None:
|
if t is not None:
|
||||||
task_id = t.id
|
task_id = t.id
|
||||||
s = structured or {}
|
s = structured or {}
|
||||||
@@ -464,7 +464,7 @@ class ContentActions:
|
|||||||
if reject := await self._verify_explicit_task_ownership(agent_id, task_id):
|
if reject := await self._verify_explicit_task_ownership(agent_id, task_id):
|
||||||
return reject
|
return reject
|
||||||
else:
|
else:
|
||||||
t = await self.task.get_active_task_for_agent(agent_id)
|
t = await self.task.get_journal_context_task_for_agent(agent_id)
|
||||||
if t is not None:
|
if t is not None:
|
||||||
task_id = t.id
|
task_id = t.id
|
||||||
try:
|
try:
|
||||||
@@ -516,7 +516,7 @@ class ContentActions:
|
|||||||
if reject := await self._verify_explicit_task_ownership(agent_id, task_id):
|
if reject := await self._verify_explicit_task_ownership(agent_id, task_id):
|
||||||
return reject
|
return reject
|
||||||
else:
|
else:
|
||||||
t = await self.task.get_active_task_for_agent(agent_id)
|
t = await self.task.get_journal_context_task_for_agent(agent_id)
|
||||||
if t is not None:
|
if t is not None:
|
||||||
task_id = t.id
|
task_id = t.id
|
||||||
if task_id is None:
|
if task_id is None:
|
||||||
@@ -592,7 +592,7 @@ class ContentActions:
|
|||||||
if reject := await self._verify_explicit_task_ownership(agent_id, task_id):
|
if reject := await self._verify_explicit_task_ownership(agent_id, task_id):
|
||||||
return reject
|
return reject
|
||||||
else:
|
else:
|
||||||
t = await self.task.get_active_task_for_agent(agent_id)
|
t = await self.task.get_journal_context_task_for_agent(agent_id)
|
||||||
if t is not None:
|
if t is not None:
|
||||||
task_id = t.id
|
task_id = t.id
|
||||||
await self.notifications.send_ack_notification(
|
await self.notifications.send_ack_notification(
|
||||||
|
|||||||
@@ -4489,6 +4489,22 @@ class TaskService(BaseService):
|
|||||||
TaskStatus.AWAITING_DOCUMENTATION,
|
TaskStatus.AWAITING_DOCUMENTATION,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
# States in which the agent still owns the task for content / journal
|
||||||
|
# context, even if it isn't progressing. Used by note / say / dm /
|
||||||
|
# evidence so journal entries written from blocked or paused get the
|
||||||
|
# task_id auto-attached (otherwise the C8 + tracing gates never see
|
||||||
|
# the agent's decisions and the agent spirals).
|
||||||
|
_JOURNAL_CONTEXT_STATUSES: ClassVar[set[TaskStatus]] = {
|
||||||
|
TaskStatus.CLAIMED,
|
||||||
|
TaskStatus.IN_PROGRESS,
|
||||||
|
TaskStatus.VERIFYING,
|
||||||
|
TaskStatus.AWAITING_QA,
|
||||||
|
TaskStatus.AWAITING_DOCUMENTATION,
|
||||||
|
TaskStatus.BLOCKED,
|
||||||
|
TaskStatus.PAUSED,
|
||||||
|
TaskStatus.NEEDS_REVISION,
|
||||||
|
}
|
||||||
|
|
||||||
# Statuses that count as "still assignable to the agent" for triage.
|
# Statuses that count as "still assignable to the agent" for triage.
|
||||||
_AGENT_NON_TERMINAL_STATUSES: ClassVar[set[TaskStatus]] = {
|
_AGENT_NON_TERMINAL_STATUSES: ClassVar[set[TaskStatus]] = {
|
||||||
TaskStatus.PENDING,
|
TaskStatus.PENDING,
|
||||||
@@ -4629,6 +4645,30 @@ class TaskService(BaseService):
|
|||||||
result = await self.session.execute(query)
|
result = await self.session.execute(query)
|
||||||
return result.scalar_one_or_none()
|
return result.scalar_one_or_none()
|
||||||
|
|
||||||
|
async def get_journal_context_task_for_agent(
|
||||||
|
self, agent_id: UUID
|
||||||
|
) -> TaskTable | None:
|
||||||
|
"""Most-recently-updated task the agent owns for journal/content context.
|
||||||
|
|
||||||
|
Wider than ``get_active_task_for_agent`` — includes BLOCKED, PAUSED,
|
||||||
|
and NEEDS_REVISION so journal entries written while stuck still
|
||||||
|
get the task_id auto-attached. Smoke-5 surfaced the bug: PMs
|
||||||
|
wrote decisions during blocked state, auto-injection returned
|
||||||
|
None, entries persisted with task_id=NULL, the C8 tracing gate
|
||||||
|
never saw them, agents spiraled forever.
|
||||||
|
"""
|
||||||
|
query = (
|
||||||
|
select(TaskTable)
|
||||||
|
.where(
|
||||||
|
TaskTable.assigned_to == agent_id,
|
||||||
|
TaskTable.status.in_(self._JOURNAL_CONTEXT_STATUSES),
|
||||||
|
)
|
||||||
|
.order_by(TaskTable.updated_at.desc().nullslast())
|
||||||
|
.limit(1)
|
||||||
|
)
|
||||||
|
result = await self.session.execute(query)
|
||||||
|
return result.scalar_one_or_none()
|
||||||
|
|
||||||
async def list_pending_for_agent(self, agent_id: UUID) -> list[TaskTable]:
|
async def list_pending_for_agent(self, agent_id: UUID) -> list[TaskTable]:
|
||||||
"""Tasks assigned to this agent that are still in PENDING status.
|
"""Tasks assigned to this agent that are still in PENDING status.
|
||||||
|
|
||||||
|
|||||||
@@ -17,6 +17,7 @@ def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps:
|
|||||||
else:
|
else:
|
||||||
task = AsyncMock()
|
task = AsyncMock()
|
||||||
task.get_active_task_for_agent.return_value = None
|
task.get_active_task_for_agent.return_value = None
|
||||||
|
task.get_journal_context_task_for_agent.return_value = None
|
||||||
# commit() checks caller role server-side; default-created mocks
|
# commit() checks caller role server-side; default-created mocks
|
||||||
# need a default developer role so existing tests pass through.
|
# need a default developer role so existing tests pass through.
|
||||||
# Caller-supplied mocks must set agent_for themselves.
|
# Caller-supplied mocks must set agent_for themselves.
|
||||||
@@ -400,6 +401,7 @@ async def test_note_auto_fills_task_id_from_active_task() -> None:
|
|||||||
task_obj = MagicMock(id=task_id, status="in_progress")
|
task_obj = MagicMock(id=task_id, status="in_progress")
|
||||||
task_svc = AsyncMock()
|
task_svc = AsyncMock()
|
||||||
task_svc.get_active_task_for_agent.return_value = task_obj
|
task_svc.get_active_task_for_agent.return_value = task_obj
|
||||||
|
task_svc.get_journal_context_task_for_agent.return_value = task_obj
|
||||||
journal_svc = AsyncMock()
|
journal_svc = AsyncMock()
|
||||||
|
|
||||||
deps = _make_deps(task=task_svc, journal=journal_svc)
|
deps = _make_deps(task=task_svc, journal=journal_svc)
|
||||||
@@ -440,6 +442,7 @@ async def test_say_auto_injects_task_id_when_active_task_exists() -> None:
|
|||||||
task_obj = MagicMock(id=task_id, status="in_progress")
|
task_obj = MagicMock(id=task_id, status="in_progress")
|
||||||
task_svc = AsyncMock()
|
task_svc = AsyncMock()
|
||||||
task_svc.get_active_task_for_agent.return_value = task_obj
|
task_svc.get_active_task_for_agent.return_value = task_obj
|
||||||
|
task_svc.get_journal_context_task_for_agent.return_value = task_obj
|
||||||
messaging_svc = AsyncMock()
|
messaging_svc = AsyncMock()
|
||||||
|
|
||||||
deps = _make_deps(task=task_svc, messaging=messaging_svc)
|
deps = _make_deps(task=task_svc, messaging=messaging_svc)
|
||||||
@@ -507,6 +510,7 @@ async def test_dm_with_active_task_succeeds() -> None:
|
|||||||
task_obj = MagicMock(id=task_id, status="in_progress")
|
task_obj = MagicMock(id=task_id, status="in_progress")
|
||||||
task_svc = AsyncMock()
|
task_svc = AsyncMock()
|
||||||
task_svc.get_active_task_for_agent.return_value = task_obj
|
task_svc.get_active_task_for_agent.return_value = task_obj
|
||||||
|
task_svc.get_journal_context_task_for_agent.return_value = task_obj
|
||||||
a2a_svc = AsyncMock()
|
a2a_svc = AsyncMock()
|
||||||
|
|
||||||
deps = _make_deps(task=task_svc, a2a=a2a_svc)
|
deps = _make_deps(task=task_svc, a2a=a2a_svc)
|
||||||
|
|||||||
@@ -22,6 +22,7 @@ def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps:
|
|||||||
else:
|
else:
|
||||||
task = AsyncMock()
|
task = AsyncMock()
|
||||||
task.get_active_task_for_agent.return_value = None
|
task.get_active_task_for_agent.return_value = None
|
||||||
|
task.get_journal_context_task_for_agent.return_value = None
|
||||||
task.agent_for.return_value = MagicMock(role="cell_pm")
|
task.agent_for.return_value = MagicMock(role="cell_pm")
|
||||||
|
|
||||||
git = overrides.get("git", AsyncMock())
|
git = overrides.get("git", AsyncMock())
|
||||||
@@ -239,6 +240,7 @@ async def test_notify_auto_fills_task_id_from_active_task() -> None:
|
|||||||
task_obj = MagicMock(id=task_id, status="awaiting_pm_review")
|
task_obj = MagicMock(id=task_id, status="awaiting_pm_review")
|
||||||
task_svc = AsyncMock()
|
task_svc = AsyncMock()
|
||||||
task_svc.get_active_task_for_agent.return_value = task_obj
|
task_svc.get_active_task_for_agent.return_value = task_obj
|
||||||
|
task_svc.get_journal_context_task_for_agent.return_value = task_obj
|
||||||
task_svc.agent_for.return_value = MagicMock(role="cell_pm")
|
task_svc.agent_for.return_value = MagicMock(role="cell_pm")
|
||||||
notif_svc = AsyncMock()
|
notif_svc = AsyncMock()
|
||||||
|
|
||||||
|
|||||||
@@ -26,6 +26,7 @@ def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps:
|
|||||||
task = AsyncMock()
|
task = AsyncMock()
|
||||||
task.agent_for.return_value = MagicMock(role="developer", slug="be-dev-1")
|
task.agent_for.return_value = MagicMock(role="developer", slug="be-dev-1")
|
||||||
task.get_active_task_for_agent.return_value = None
|
task.get_active_task_for_agent.return_value = None
|
||||||
|
task.get_journal_context_task_for_agent.return_value = None
|
||||||
|
|
||||||
git = overrides.get("git", AsyncMock())
|
git = overrides.get("git", AsyncMock())
|
||||||
messaging = overrides.get("messaging", AsyncMock())
|
messaging = overrides.get("messaging", AsyncMock())
|
||||||
@@ -68,6 +69,7 @@ async def test_say_posted_status_with_active_task() -> None:
|
|||||||
task_svc = AsyncMock()
|
task_svc = AsyncMock()
|
||||||
task_svc.agent_for.return_value = MagicMock(role="developer", slug="be-dev-1")
|
task_svc.agent_for.return_value = MagicMock(role="developer", slug="be-dev-1")
|
||||||
task_svc.get_active_task_for_agent.return_value = task_obj
|
task_svc.get_active_task_for_agent.return_value = task_obj
|
||||||
|
task_svc.get_journal_context_task_for_agent.return_value = task_obj
|
||||||
|
|
||||||
ca = ContentActions(_make_deps(task=task_svc))
|
ca = ContentActions(_make_deps(task=task_svc))
|
||||||
env = await ca.say(
|
env = await ca.say(
|
||||||
|
|||||||
@@ -0,0 +1,100 @@
|
|||||||
|
"""Smoke-5: get_journal_context_task_for_agent includes BLOCKED, PAUSED,
|
||||||
|
NEEDS_REVISION so note/say/dm/notify auto-inject task_id while the agent
|
||||||
|
is stuck.
|
||||||
|
|
||||||
|
Original bug: get_active_task_for_agent filtered to DEV_ACTIVE statuses only.
|
||||||
|
PMs writing decisions during BLOCKED state got task_id=NULL on their journal
|
||||||
|
entries. The C8 tracing gate then never found the decisions and the agent
|
||||||
|
spiraled. Smoke-5 wrote 5 decisions, 8 reflections, 1 struggle — all with
|
||||||
|
task_id=NULL because the agent was stuck when journaling.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from unittest.mock import AsyncMock, MagicMock
|
||||||
|
from uuid import uuid4
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from roboco.models.base import TaskStatus
|
||||||
|
from roboco.services.task import TaskService
|
||||||
|
|
||||||
|
|
||||||
|
def _service_with(execute_returns: object) -> TaskService:
|
||||||
|
session = MagicMock()
|
||||||
|
session.execute = AsyncMock(return_value=execute_returns)
|
||||||
|
return TaskService(session)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_journal_context_returns_blocked_task() -> None:
|
||||||
|
"""A BLOCKED task is returned by the journal-context lookup."""
|
||||||
|
task = MagicMock(id=uuid4(), status=TaskStatus.BLOCKED)
|
||||||
|
result = MagicMock()
|
||||||
|
result.scalar_one_or_none.return_value = task
|
||||||
|
svc = _service_with(result)
|
||||||
|
found = await svc.get_journal_context_task_for_agent(uuid4())
|
||||||
|
assert found is task
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_journal_context_returns_paused_task() -> None:
|
||||||
|
"""A PAUSED task is returned by the journal-context lookup."""
|
||||||
|
task = MagicMock(id=uuid4(), status=TaskStatus.PAUSED)
|
||||||
|
result = MagicMock()
|
||||||
|
result.scalar_one_or_none.return_value = task
|
||||||
|
svc = _service_with(result)
|
||||||
|
found = await svc.get_journal_context_task_for_agent(uuid4())
|
||||||
|
assert found is task
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_journal_context_returns_needs_revision_task() -> None:
|
||||||
|
"""A NEEDS_REVISION task is returned so QA-rejected devs still get task_id."""
|
||||||
|
task = MagicMock(id=uuid4(), status=TaskStatus.NEEDS_REVISION)
|
||||||
|
result = MagicMock()
|
||||||
|
result.scalar_one_or_none.return_value = task
|
||||||
|
svc = _service_with(result)
|
||||||
|
found = await svc.get_journal_context_task_for_agent(uuid4())
|
||||||
|
assert found is task
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_journal_context_query_filters_by_journal_statuses() -> None:
|
||||||
|
"""The query's where clause uses _JOURNAL_CONTEXT_STATUSES, not _DEV_ACTIVE."""
|
||||||
|
result = MagicMock()
|
||||||
|
result.scalar_one_or_none.return_value = None
|
||||||
|
session = MagicMock()
|
||||||
|
session.execute = AsyncMock(return_value=result)
|
||||||
|
svc = TaskService(session)
|
||||||
|
agent_id = uuid4()
|
||||||
|
|
||||||
|
await svc.get_journal_context_task_for_agent(agent_id)
|
||||||
|
|
||||||
|
assert session.execute.await_count == 1
|
||||||
|
sent_query = session.execute.await_args.args[0]
|
||||||
|
rendered = str(sent_query.compile(compile_kwargs={"literal_binds": True}))
|
||||||
|
for s in ("blocked", "paused", "needs_revision", "in_progress", "claimed"):
|
||||||
|
assert s in rendered, (
|
||||||
|
f"Journal-context query missing status {s}. Rendered SQL: {rendered}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_dev_active_query_still_excludes_blocked() -> None:
|
||||||
|
"""The narrow get_active_task_for_agent stays narrow — commit() relies on it."""
|
||||||
|
result = MagicMock()
|
||||||
|
result.scalar_one_or_none.return_value = None
|
||||||
|
session = MagicMock()
|
||||||
|
session.execute = AsyncMock(return_value=result)
|
||||||
|
svc = TaskService(session)
|
||||||
|
|
||||||
|
await svc.get_active_task_for_agent(uuid4())
|
||||||
|
|
||||||
|
sent_query = session.execute.await_args.args[0]
|
||||||
|
rendered = str(sent_query.compile(compile_kwargs={"literal_binds": True}))
|
||||||
|
assert "'blocked'" not in rendered, (
|
||||||
|
"Dev-active query must NOT include BLOCKED — commit() would otherwise "
|
||||||
|
"allow commits from a blocked task."
|
||||||
|
)
|
||||||
|
assert "'paused'" not in rendered
|
||||||
|
assert "'needs_revision'" not in rendered
|
||||||
Reference in New Issue
Block a user