mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(notifications): one duplicate-tolerant role lookup for every singleton-role site
The duplicate-CEO-row fix landed on one call site but three identical bare 'select(...).where(role == ...)' + scalar_one_or_none() lookups remained in the orchestrator (auditor + two CEO), each still raising MultipleResultsFound under the same condition. All five sites now route through a shared get_agent_by_role helper (earliest-created row wins). The five event-bus notification handlers also thread task_title so a revived producer renders titles instead of #id8 (protocol signatures updated to match the service), the pull_request CI trigger mirrors the push trigger's paths so a fork PR touching only those paths still runs CI, and a stale a2a comment about auditor/pr_reviewer lacking read_a2a is corrected.
This commit is contained in:
@@ -62,6 +62,14 @@ on:
|
||||
- 'uv.lock'
|
||||
- 'alembic.ini'
|
||||
- '.github/workflows/ci.yml'
|
||||
# Kept identical to the push trigger's paths above so an external-fork
|
||||
# PR touching only these (panel/docs-only, motion-only, ...) still
|
||||
# fires CI instead of merging on a false "no checks required" green.
|
||||
- 'panel/**'
|
||||
- 'CLAUDE.md'
|
||||
- 'CHANGELOG.md'
|
||||
- 'docs/**'
|
||||
- 'motion/**'
|
||||
workflow_dispatch:
|
||||
|
||||
# A fleet branch that's also an open PR head can get both a `push` and a
|
||||
|
||||
@@ -77,6 +77,7 @@ async def _handle_task_blocked(
|
||||
blocker_reason=blocker_reason,
|
||||
from_agent=event.source_agent,
|
||||
to_pm=_get_pm_id(team),
|
||||
task_title=event.data.get("task_title"),
|
||||
)
|
||||
|
||||
|
||||
@@ -96,6 +97,7 @@ async def _handle_task_awaiting_qa(
|
||||
task_id=task_id,
|
||||
from_agent=event.source_agent,
|
||||
to_qa=_get_qa_id(team),
|
||||
task_title=event.data.get("task_title"),
|
||||
)
|
||||
|
||||
|
||||
@@ -116,6 +118,7 @@ async def _handle_task_qa_failed(
|
||||
task_id=task_id,
|
||||
qa_notes=qa_notes,
|
||||
to_developer=developer_id,
|
||||
task_title=event.data.get("task_title"),
|
||||
)
|
||||
|
||||
|
||||
@@ -135,6 +138,7 @@ async def _handle_task_awaiting_docs(
|
||||
task_id=task_id,
|
||||
from_agent=event.source_agent,
|
||||
to_documenter=_get_doc_id(team),
|
||||
task_title=event.data.get("task_title"),
|
||||
)
|
||||
|
||||
|
||||
@@ -203,6 +207,7 @@ async def handle_handoff_created(event: Event) -> None:
|
||||
handoff_id=handoff_id,
|
||||
from_agent=from_agent,
|
||||
to_documenter=_get_doc_id(team),
|
||||
task_title=event.data.get("task_title"),
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -135,6 +135,7 @@ class NotificationServiceProtocol(Protocol):
|
||||
blocker_reason: str,
|
||||
from_agent: str | None,
|
||||
to_pm: str,
|
||||
task_title: str | None = None,
|
||||
) -> None: ...
|
||||
|
||||
async def send_qa_ready_notification(
|
||||
@@ -142,6 +143,7 @@ class NotificationServiceProtocol(Protocol):
|
||||
task_id: str,
|
||||
from_agent: str | None,
|
||||
to_qa: str,
|
||||
task_title: str | None = None,
|
||||
) -> None: ...
|
||||
|
||||
async def send_qa_failed_notification(
|
||||
@@ -149,6 +151,7 @@ class NotificationServiceProtocol(Protocol):
|
||||
task_id: str,
|
||||
qa_notes: str,
|
||||
to_developer: str,
|
||||
task_title: str | None = None,
|
||||
) -> None: ...
|
||||
|
||||
async def send_docs_ready_notification(
|
||||
@@ -156,6 +159,7 @@ class NotificationServiceProtocol(Protocol):
|
||||
task_id: str,
|
||||
from_agent: str | None,
|
||||
to_documenter: str,
|
||||
task_title: str | None = None,
|
||||
) -> None: ...
|
||||
|
||||
async def send_handoff_notification(
|
||||
@@ -164,6 +168,7 @@ class NotificationServiceProtocol(Protocol):
|
||||
handoff_id: str,
|
||||
from_agent: str | None,
|
||||
to_documenter: str,
|
||||
task_title: str | None = None,
|
||||
) -> None: ...
|
||||
|
||||
async def send_a2a_notification(
|
||||
|
||||
@@ -7866,10 +7866,8 @@ Start by:
|
||||
stop the health loop.
|
||||
"""
|
||||
try:
|
||||
from sqlalchemy import select
|
||||
|
||||
from roboco.db.base import get_session_factory
|
||||
from roboco.db.tables import AgentTable, NotificationTable
|
||||
from roboco.db.tables import NotificationTable
|
||||
from roboco.models.base import (
|
||||
AgentRole,
|
||||
NotificationPriority,
|
||||
@@ -7878,18 +7876,13 @@ Start by:
|
||||
from roboco.services.notification_delivery import (
|
||||
get_notification_delivery_service,
|
||||
)
|
||||
from roboco.services.repositories.query_helpers import get_agent_by_role
|
||||
from roboco.utils.converters import require_uuid
|
||||
|
||||
session_factory = get_session_factory()
|
||||
async with session_factory() as db:
|
||||
orch_agent = await db.execute(
|
||||
select(AgentTable).where(AgentTable.role == AgentRole.AUDITOR)
|
||||
)
|
||||
auditor = orch_agent.scalar_one_or_none()
|
||||
ceo_result = await db.execute(
|
||||
select(AgentTable).where(AgentTable.role == AgentRole.CEO)
|
||||
)
|
||||
ceo = ceo_result.scalar_one_or_none()
|
||||
auditor = await get_agent_by_role(db, AgentRole.AUDITOR)
|
||||
ceo = await get_agent_by_role(db, AgentRole.CEO)
|
||||
recipients = [a.id for a in (auditor, ceo) if a is not None]
|
||||
if not recipients:
|
||||
logger.warning(
|
||||
@@ -9550,10 +9543,8 @@ Start by:
|
||||
``_notify_stranded_agent`` — direct DB insert + delivery.deliver().
|
||||
"""
|
||||
try:
|
||||
from sqlalchemy import select as _select
|
||||
|
||||
from roboco.db.base import get_session_factory
|
||||
from roboco.db.tables import AgentTable, NotificationTable
|
||||
from roboco.db.tables import NotificationTable
|
||||
from roboco.models.base import (
|
||||
AgentRole,
|
||||
NotificationPriority,
|
||||
@@ -9562,6 +9553,7 @@ Start by:
|
||||
from roboco.services.notification_delivery import (
|
||||
get_notification_delivery_service,
|
||||
)
|
||||
from roboco.services.repositories.query_helpers import get_agent_by_role
|
||||
from roboco.utils.converters import require_uuid
|
||||
|
||||
# Compute human-friendly duration
|
||||
@@ -9579,10 +9571,7 @@ Start by:
|
||||
|
||||
session_factory = get_session_factory()
|
||||
async with session_factory() as db:
|
||||
ceo_result = await db.execute(
|
||||
_select(AgentTable).where(AgentTable.role == AgentRole.CEO)
|
||||
)
|
||||
ceo = ceo_result.scalar_one_or_none()
|
||||
ceo = await get_agent_by_role(db, AgentRole.CEO)
|
||||
if ceo is None:
|
||||
logger.warning(
|
||||
"CEO agent not found; skipping rate-limit CEO notification",
|
||||
|
||||
@@ -1998,11 +1998,13 @@ class A2AService:
|
||||
if from_slug != "ceo" or not is_spawnable_agent_slug(to_slug):
|
||||
return
|
||||
# A wake only helps a role that can actually drain the DM and close
|
||||
# the notification (read_a2a → _ack_pending_wake_notifications). For
|
||||
# a role without it (auditor, pr_reviewer) the row would be unackable
|
||||
# and immortal: it permanently blocks future wakes via the dedup
|
||||
# pre-check and drives futile respawns. Local imports: the gateway
|
||||
# package cycles back into this module at module scope.
|
||||
# the notification (read_a2a → _ack_pending_wake_notifications). Both
|
||||
# auditor and pr_reviewer carry read_a2a now, but role tools are
|
||||
# re-derived here rather than assumed, so a future role without it
|
||||
# still gets the same protection: an unackable row would otherwise be
|
||||
# immortal, permanently blocking future wakes via the dedup pre-check
|
||||
# and driving futile respawns. Local imports: the gateway package
|
||||
# cycles back into this module at module scope.
|
||||
from roboco.agents_config import get_agent_role
|
||||
from roboco.services.gateway.role_config import get_role_config
|
||||
|
||||
|
||||
@@ -37,6 +37,7 @@ from roboco.services.notification_dedup import (
|
||||
clear_dedup_key,
|
||||
)
|
||||
from roboco.services.notification_text import task_display
|
||||
from roboco.services.repositories.query_helpers import get_agent_by_role
|
||||
from roboco.utils.converters import require_uuid
|
||||
|
||||
if TYPE_CHECKING:
|
||||
@@ -1207,27 +1208,16 @@ class NotificationDeliveryService(BaseService):
|
||||
async def _get_ceo_agent(self) -> AgentTable | None:
|
||||
"""Find the CEO agent (org-wide singleton; earliest-created if many).
|
||||
|
||||
Mirrors `_get_auditor_agent`: a plain one-or-none raises
|
||||
MultipleResultsFound if a second CEO-role row ever exists, so pin to
|
||||
the earliest-created — the canonical seeded CEO — instead.
|
||||
Delegates to the shared `get_agent_by_role` helper — a plain
|
||||
one-or-none raises MultipleResultsFound if a second CEO-role row ever
|
||||
exists, so it pins to the earliest-created (the canonical seeded CEO)
|
||||
instead.
|
||||
"""
|
||||
result = await self.session.execute(
|
||||
select(AgentTable)
|
||||
.where(AgentTable.role == AgentRole.CEO)
|
||||
.order_by(AgentTable.created_at)
|
||||
.limit(1)
|
||||
)
|
||||
return result.scalar_one_or_none()
|
||||
return await get_agent_by_role(self.session, AgentRole.CEO)
|
||||
|
||||
async def _get_auditor_agent(self) -> AgentTable | None:
|
||||
"""Find the auditor agent (org-wide; earliest-created if many)."""
|
||||
result = await self.session.execute(
|
||||
select(AgentTable)
|
||||
.where(AgentTable.role == AgentRole.AUDITOR)
|
||||
.order_by(AgentTable.created_at)
|
||||
.limit(1)
|
||||
)
|
||||
return result.scalar_one_or_none()
|
||||
return await get_agent_by_role(self.session, AgentRole.AUDITOR)
|
||||
|
||||
async def _persist_and_deliver(self, notification: NotificationTable) -> None:
|
||||
"""Add to session, flush (to get an id), deliver. Caller commits."""
|
||||
|
||||
@@ -15,6 +15,7 @@ from sqlalchemy.ext.asyncio import AsyncSession
|
||||
from sqlalchemy.sql import Select
|
||||
|
||||
from roboco.db.tables import AgentTable
|
||||
from roboco.models.base import AgentRole
|
||||
|
||||
|
||||
def pagination(
|
||||
@@ -262,3 +263,32 @@ async def get_agent_by_slug(
|
||||
"""
|
||||
result = await db.execute(select(AgentTable).where(AgentTable.slug == slug))
|
||||
return result.scalar_one_or_none()
|
||||
|
||||
|
||||
async def get_agent_by_role(
|
||||
db: AsyncSession,
|
||||
role: AgentRole,
|
||||
) -> AgentTable | None:
|
||||
"""
|
||||
Get an org-wide singleton-role agent (e.g. CEO, AUDITOR) by role.
|
||||
|
||||
A plain `role == ...` + `scalar_one_or_none()` raises MultipleResultsFound
|
||||
the moment a second row of that role ever exists (a real hazard: shared
|
||||
test DBs and misconfigured seeds both do this). Pin to the
|
||||
earliest-created row instead so the lookup is deterministic and never
|
||||
crashes.
|
||||
|
||||
Args:
|
||||
db: Database session
|
||||
role: The singleton role to resolve (e.g. AgentRole.CEO)
|
||||
|
||||
Returns:
|
||||
The earliest-created agent with this role, or None if none exist
|
||||
"""
|
||||
result = await db.execute(
|
||||
select(AgentTable)
|
||||
.where(AgentTable.role == role)
|
||||
.order_by(AgentTable.created_at)
|
||||
.limit(1)
|
||||
)
|
||||
return result.scalar_one_or_none()
|
||||
|
||||
@@ -240,3 +240,49 @@ async def test_get_ceo_agent_tolerates_duplicate_ceo_rows(env: dict) -> None:
|
||||
# Does not raise, and pins to the earliest-created (never the later row).
|
||||
assert resolved is not None
|
||||
assert resolved.id != later_ceo.id
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_auditor_agent_tolerates_duplicate_auditor_rows(env: dict) -> None:
|
||||
"""Mirrors `test_get_ceo_agent_tolerates_duplicate_ceo_rows`: a second
|
||||
role=AUDITOR row must not make `_get_auditor_agent()` raise
|
||||
MultipleResultsFound — both now delegate to the shared
|
||||
`get_agent_by_role` helper."""
|
||||
db = env["db"]
|
||||
first_auditor = AgentTable(
|
||||
id=uuid4(),
|
||||
name="Auditor 1",
|
||||
slug=f"auditor-{uuid4().hex[:6]}",
|
||||
role=AgentRole.AUDITOR,
|
||||
team=None,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="x",
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db.add(first_auditor)
|
||||
await db.flush()
|
||||
later_auditor = AgentTable(
|
||||
id=uuid4(),
|
||||
name="Auditor 2",
|
||||
slug=f"auditor-{uuid4().hex[:6]}",
|
||||
role=AgentRole.AUDITOR,
|
||||
team=None,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="x",
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
created_at=datetime.now(UTC) + timedelta(hours=1),
|
||||
)
|
||||
db.add(later_auditor)
|
||||
await db.flush()
|
||||
|
||||
delivery = get_notification_delivery_service(db)
|
||||
resolved = await delivery._get_auditor_agent()
|
||||
|
||||
assert resolved is not None
|
||||
assert resolved.id != later_auditor.id
|
||||
|
||||
@@ -13,6 +13,7 @@ from roboco.models import AgentRole, AgentStatus, Team
|
||||
from roboco.services.repositories.query_helpers import (
|
||||
agent_id_filter,
|
||||
days_ago,
|
||||
get_agent_by_role,
|
||||
get_agent_by_slug,
|
||||
get_agent_slug,
|
||||
pagination,
|
||||
@@ -256,3 +257,48 @@ async def test_get_agent_by_slug(db_session: AsyncSession) -> None:
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_agent_by_slug_missing(db_session: AsyncSession) -> None:
|
||||
assert await get_agent_by_slug(db_session, "ghost-slug") is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_agent_by_role_tolerates_duplicate_rows(
|
||||
db_session: AsyncSession,
|
||||
) -> None:
|
||||
"""A second row of the same singleton role (a real hazard for CEO/AUDITOR
|
||||
in prod, and for the shared test DB here) must not raise
|
||||
MultipleResultsFound — the later-created duplicate is never selected."""
|
||||
earlier = AgentTable(
|
||||
id=uuid4(),
|
||||
name="Auditor Earlier",
|
||||
slug=f"qh-role-a-{uuid4().hex[:8]}",
|
||||
role=AgentRole.AUDITOR,
|
||||
team=None,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="x",
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db_session.add(earlier)
|
||||
await db_session.flush()
|
||||
later = AgentTable(
|
||||
id=uuid4(),
|
||||
name="Auditor Later",
|
||||
slug=f"qh-role-b-{uuid4().hex[:8]}",
|
||||
role=AgentRole.AUDITOR,
|
||||
team=None,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="x",
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
created_at=datetime.now(UTC) + timedelta(hours=1),
|
||||
)
|
||||
db_session.add(later)
|
||||
await db_session.flush()
|
||||
|
||||
resolved = await get_agent_by_role(db_session, AgentRole.AUDITOR)
|
||||
|
||||
assert resolved is not None
|
||||
assert resolved.id != later.id
|
||||
|
||||
@@ -86,10 +86,18 @@ async def test_handle_task_blocked_calls_send_blocker() -> None:
|
||||
notif.send_blocker_notification = AsyncMock()
|
||||
set_event_context(notification_service=notif)
|
||||
event = _make_event(
|
||||
EventType.TASK_BLOCKED, task_id=str(uuid4()), team="backend", reason="x"
|
||||
EventType.TASK_BLOCKED,
|
||||
task_id=str(uuid4()),
|
||||
team="backend",
|
||||
reason="x",
|
||||
task_title="Ship the widget",
|
||||
)
|
||||
await handle_task_status_change(event)
|
||||
notif.send_blocker_notification.assert_called_once()
|
||||
assert (
|
||||
notif.send_blocker_notification.call_args.kwargs["task_title"]
|
||||
== "Ship the widget"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -108,10 +116,17 @@ async def test_handle_task_awaiting_qa() -> None:
|
||||
notif.send_qa_ready_notification = AsyncMock()
|
||||
set_event_context(notification_service=notif)
|
||||
event = _make_event(
|
||||
EventType.TASK_AWAITING_QA, task_id=str(uuid4()), team="backend"
|
||||
EventType.TASK_AWAITING_QA,
|
||||
task_id=str(uuid4()),
|
||||
team="backend",
|
||||
task_title="Ship the widget",
|
||||
)
|
||||
await handle_task_status_change(event)
|
||||
notif.send_qa_ready_notification.assert_called_once()
|
||||
assert (
|
||||
notif.send_qa_ready_notification.call_args.kwargs["task_title"]
|
||||
== "Ship the widget"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -124,9 +139,14 @@ async def test_handle_task_qa_failed() -> None:
|
||||
task_id=str(uuid4()),
|
||||
assigned_to="be-dev-1",
|
||||
qa_notes="please fix",
|
||||
task_title="Ship the widget",
|
||||
)
|
||||
await handle_task_status_change(event)
|
||||
notif.send_qa_failed_notification.assert_called_once()
|
||||
assert (
|
||||
notif.send_qa_failed_notification.call_args.kwargs["task_title"]
|
||||
== "Ship the widget"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -145,10 +165,17 @@ async def test_handle_task_awaiting_docs() -> None:
|
||||
notif.send_docs_ready_notification = AsyncMock()
|
||||
set_event_context(notification_service=notif)
|
||||
event = _make_event(
|
||||
EventType.TASK_AWAITING_DOCS, task_id=str(uuid4()), team="backend"
|
||||
EventType.TASK_AWAITING_DOCS,
|
||||
task_id=str(uuid4()),
|
||||
team="backend",
|
||||
task_title="Ship the widget",
|
||||
)
|
||||
await handle_task_status_change(event)
|
||||
notif.send_docs_ready_notification.assert_called_once()
|
||||
assert (
|
||||
notif.send_docs_ready_notification.call_args.kwargs["task_title"]
|
||||
== "Ship the widget"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -173,9 +200,14 @@ async def test_handle_handoff_created_calls_notification() -> None:
|
||||
task_id=str(uuid4()),
|
||||
handoff_id=str(uuid4()),
|
||||
team="backend",
|
||||
task_title="Ship the widget",
|
||||
)
|
||||
await handle_handoff_created(event)
|
||||
notif.send_handoff_notification.assert_called_once()
|
||||
assert (
|
||||
notif.send_handoff_notification.call_args.kwargs["task_title"]
|
||||
== "Ship the widget"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
Reference in New Issue
Block a user