mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(security): active guard enforcement, CEO A2A target check, notification expiry (#595)
* fix(security): guard goes active; CEO A2A respects no-comms roles; ack notifications expire ROBOCO_GUARD_PASSIVE_MODE defaults to false in both compose files — the deferred post-calibration flip; fail_secure stays off and the env override remains the rollback. can_a2a_direct no longer short-circuits the CEO past the no-comms set (auditor/pr_reviewer/prompter/secretary), now canonical in foundation.policy.communications.NO_COMMS_ROLES and shared with the content-actions gate; the A2A service refuses at conversation creation instead of silently suppressing the wake. Ack-required notifications get expires_at stamped from ROBOCO_NOTIFICATION_ACK_TTL_HOURS (default 48, 0 disables), so the re-escalation sweeper's expires_at query matches rows for the first time. * refactor(notification): extract _ack_and_expiry — xenon rank back under B The expires_at stamping pushed _create_notification_with_session to rank C; the requires_ack + expiry derivation moves into a helper with the same semantics and comments. * test(conftest): dispose the global DB engine after every test Production code reaching get_db_context()/get_engine() lazily creates the process-global engine bound to the current event loop; with per-test function-scoped loops, any later test touching the global path inherits a dead-loop engine and dies with 'Future attached to a different loop' — the order-dependent class that has been wandering the suite (cloud_auth login, metrics, tasks-routes, full-lifecycle) whenever collection order shifts. An autouse fixture now close_db()s after every test, keeping the global path loop-local; no-op when untouched. --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -2494,21 +2494,36 @@ async def test_agent_reply_to_ceo_creates_no_wake(a2a_setup: dict) -> None:
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ceo_dm_to_non_a2a_role_creates_no_wake(a2a_setup: dict) -> None:
|
||||
"""A CEO DM to a role with no read_a2a on its manifest (pr_reviewer,
|
||||
auditor) must NOT create a wake row — the recipient could never ack it,
|
||||
so it would be immortal, permanently suppress future wakes via the dedup
|
||||
pre-check, and drive futile respawns."""
|
||||
async def test_ceo_dm_to_non_a2a_role_denied_at_conversation_creation(
|
||||
a2a_setup: dict,
|
||||
) -> None:
|
||||
"""A CEO DM to a role with no dm/read_a2a on its manifest (pr_reviewer,
|
||||
auditor) must be refused outright at conversation creation — the root-
|
||||
cause fix (can_a2a_direct's CEO branch now excludes NO_COMMS_ROLES)
|
||||
supersedes the old symptom-level fix of letting the conversation exist
|
||||
and only suppressing the wake notification (the recipient could never
|
||||
ack it, so it would be immortal, permanently suppress future wakes via
|
||||
the dedup pre-check, and drive futile respawns)."""
|
||||
svc: A2AService = a2a_setup["svc"]
|
||||
conv = await svc.get_or_create_conversation(agent_a="ceo", agent_b="pr-reviewer-1")
|
||||
conv_id = UUID(conv.id)
|
||||
with pytest.raises(A2AAccessDeniedError, match="no agent-comms surface"):
|
||||
await svc.get_or_create_conversation(agent_a="ceo", agent_b="pr-reviewer-1")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_maybe_wake_ceo_recipient_still_noops_for_no_comms_role(
|
||||
a2a_setup: dict,
|
||||
) -> None:
|
||||
"""Defense-in-depth: _maybe_wake_ceo_recipient's own read_a2a manifest
|
||||
check independently no-ops for a no-comms role — unreachable through the
|
||||
normal send path now that conversation creation refuses it first, but
|
||||
it must stay safe if ever called directly (e.g. on a pre-fix row)."""
|
||||
svc: A2AService = a2a_setup["svc"]
|
||||
mock_ns = AsyncMock()
|
||||
mock_ns.send_a2a_notification = AsyncMock(return_value=None)
|
||||
with patch(
|
||||
"roboco.services.notification.NotificationService", return_value=mock_ns
|
||||
):
|
||||
await svc.send_chat_message(conv_id, "ceo", "review status?")
|
||||
await svc._maybe_wake_ceo_recipient("ceo", "pr-reviewer-1", None)
|
||||
|
||||
mock_ns.send_a2a_notification.assert_not_awaited()
|
||||
|
||||
|
||||
@@ -0,0 +1,169 @@
|
||||
"""expires_at is now stamped at creation (NotificationService) and actually
|
||||
matched by NotificationDeliveryService.sweep_expired_notifications' SQL
|
||||
WHERE clause — before the fix the column was never written, so this query
|
||||
always matched zero rows regardless of how stale a notification was.
|
||||
|
||||
Integration tests against the migrated Postgres DB: `sweep_expired_notifications`
|
||||
issues a real `expires_at < now()` query, so a mocked session (as
|
||||
`tests/unit/services/test_notification_delivery.py` uses) can't exercise it.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from datetime import UTC, datetime, timedelta
|
||||
from typing import TYPE_CHECKING, cast
|
||||
from uuid import UUID, uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.config import settings
|
||||
from roboco.db.tables import AgentTable, NotificationTable
|
||||
from roboco.models import AgentRole, AgentStatus, NotificationPriority, NotificationType
|
||||
from roboco.models.base import Team
|
||||
from roboco.models.notification import CreateNotificationParams
|
||||
from roboco.services.notification import NotificationService
|
||||
from roboco.services.notification_delivery import get_notification_delivery_service
|
||||
from sqlalchemy import select
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
|
||||
async def _seed_agent(db: AsyncSession, *, role: AgentRole, slug: str) -> UUID:
|
||||
agent = AgentTable(
|
||||
id=uuid4(),
|
||||
name=slug,
|
||||
slug=slug,
|
||||
role=role,
|
||||
team=Team.BACKEND,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt=slug,
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
db.add(agent)
|
||||
await db.flush()
|
||||
return cast("UUID", agent.id)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_created_notification_expires_at_is_stamped_and_matched_by_sweep(
|
||||
db_session: AsyncSession,
|
||||
) -> None:
|
||||
"""End-to-end: NotificationService._create_notification stamps expires_at
|
||||
for an ack-required row, and once that deadline is in the past,
|
||||
sweep_expired_notifications' real Postgres query finds it (count 1) —
|
||||
the exact round trip that was a dead no-op before this fix, since
|
||||
expires_at was always NULL and `expires_at < now()` never matched."""
|
||||
unique = uuid4().hex[:8]
|
||||
sender = await _seed_agent(
|
||||
db_session, role=AgentRole.DEVELOPER, slug=f"sndr-{unique}"
|
||||
)
|
||||
recipient = await _seed_agent(
|
||||
db_session, role=AgentRole.CELL_PM, slug=f"pm-{unique}"
|
||||
)
|
||||
|
||||
svc = NotificationService()
|
||||
await svc._create_notification(
|
||||
CreateNotificationParams(
|
||||
notification_type=NotificationType.BLOCKER_ESCALATION,
|
||||
priority=NotificationPriority.HIGH,
|
||||
from_agent=str(sender),
|
||||
to_agents=[str(recipient)],
|
||||
subject="blocked",
|
||||
body="external dependency",
|
||||
),
|
||||
db_session=db_session,
|
||||
)
|
||||
await db_session.flush()
|
||||
|
||||
row = (
|
||||
await db_session.execute(
|
||||
select(NotificationTable).where(
|
||||
NotificationTable.type == NotificationType.BLOCKER_ESCALATION,
|
||||
NotificationTable.from_agent == sender,
|
||||
)
|
||||
)
|
||||
).scalar_one()
|
||||
assert row.expires_at is not None
|
||||
assert row.requires_ack is True
|
||||
|
||||
# Backdate it past the deadline (no real clock wait) and confirm the
|
||||
# sweep's `expires_at < now()` predicate now actually matches.
|
||||
row.expires_at = datetime.now(UTC) - timedelta(minutes=1)
|
||||
await db_session.flush()
|
||||
|
||||
deliv = get_notification_delivery_service(db_session)
|
||||
count = await deliv.sweep_expired_notifications()
|
||||
assert count >= 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_directly_stamped_expired_row_is_matched_by_sweep_query(
|
||||
db_session: AsyncSession,
|
||||
) -> None:
|
||||
"""Isolates the sweep query mechanics from creation: a hand-built
|
||||
ack-required, unacked row with expires_at in the past must be counted."""
|
||||
unique = uuid4().hex[:8]
|
||||
sender = await _seed_agent(
|
||||
db_session, role=AgentRole.DEVELOPER, slug=f"s2-{unique}"
|
||||
)
|
||||
recipient = await _seed_agent(db_session, role=AgentRole.QA, slug=f"r2-{unique}")
|
||||
|
||||
notification = NotificationTable(
|
||||
type=NotificationType.ALERT,
|
||||
priority=NotificationPriority.HIGH,
|
||||
from_agent=sender,
|
||||
to_agents=[recipient],
|
||||
subject="stale alert",
|
||||
body="body",
|
||||
requires_ack=True,
|
||||
expires_at=datetime.now(UTC) - timedelta(hours=1),
|
||||
)
|
||||
db_session.add(notification)
|
||||
await db_session.flush()
|
||||
|
||||
deliv = get_notification_delivery_service(db_session)
|
||||
count = await deliv.sweep_expired_notifications()
|
||||
assert count >= 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_zero_ttl_disables_expires_at_stamping_end_to_end(
|
||||
db_session: AsyncSession, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""notification_ack_ttl_hours=0 leaves expires_at NULL even for an
|
||||
ack-required notification created through the real service."""
|
||||
monkeypatch.setattr(settings, "notification_ack_ttl_hours", 0)
|
||||
unique = uuid4().hex[:8]
|
||||
sender = await _seed_agent(
|
||||
db_session, role=AgentRole.DEVELOPER, slug=f"s3-{unique}"
|
||||
)
|
||||
recipient = await _seed_agent(
|
||||
db_session, role=AgentRole.CELL_PM, slug=f"pm3-{unique}"
|
||||
)
|
||||
|
||||
svc = NotificationService()
|
||||
await svc._create_notification(
|
||||
CreateNotificationParams(
|
||||
notification_type=NotificationType.BLOCKER_ESCALATION,
|
||||
priority=NotificationPriority.HIGH,
|
||||
from_agent=str(sender),
|
||||
to_agents=[str(recipient)],
|
||||
subject="blocked",
|
||||
body="external dependency",
|
||||
),
|
||||
db_session=db_session,
|
||||
)
|
||||
await db_session.flush()
|
||||
|
||||
row = (
|
||||
await db_session.execute(
|
||||
select(NotificationTable).where(
|
||||
NotificationTable.type == NotificationType.BLOCKER_ESCALATION,
|
||||
NotificationTable.from_agent == sender,
|
||||
)
|
||||
)
|
||||
).scalar_one()
|
||||
assert row.expires_at is None
|
||||
Reference in New Issue
Block a user