mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[F009] notification: derive requires_ack from ACK_REQUIRED_BY_TYPE, not the True default
NotificationService._create_notification built NotificationTable without requires_ack, so the column default (True) applied to EVERY notification - including informational REVIEW_REQUEST / DOCUMENTATION_REQUEST / A2A_REQUEST / KNOWLEDGE_SHARE (ACK_REQUIRED_BY_TYPE -> False) and every @mention from MessagingService._notify_mentions. Each false ack-required inflated the recipient's unacked set and soft-blocked i_am_idle into respawn churn. - _create_notification: requires_ack=ACK_REQUIRED_BY_TYPE.get(type, True) (unmapped types default True - preserve the action-required bias). - _notify_mentions: requires_ack=False explicit (MENTION is informational). TDD red->green (identity is False/is True assertions - the mocked flush doesn't apply SQLA's insert-time default, so pre-fix the attribute was None); ruff + mypy clean; notification suite green (18 passed).
This commit is contained in:
@@ -1386,6 +1386,11 @@ class MessagingService(BaseService):
|
|||||||
subject=f"You were mentioned in #{channel_slug}",
|
subject=f"You were mentioned in #{channel_slug}",
|
||||||
body=message.content[:500], # Truncate for notification
|
body=message.content[:500], # Truncate for notification
|
||||||
related_task_id=message.task_id,
|
related_task_id=message.task_id,
|
||||||
|
# F009: MENTION is informational (ACK_REQUIRED_BY_TYPE -> False).
|
||||||
|
# The column default True made every @mention require an ack,
|
||||||
|
# inflating the recipient's unacked set and soft-blocking
|
||||||
|
# i_am_idle into respawn churn.
|
||||||
|
requires_ack=False,
|
||||||
)
|
)
|
||||||
self.session.add(notification)
|
self.session.add(notification)
|
||||||
await self.session.flush()
|
await self.session.flush()
|
||||||
|
|||||||
@@ -14,6 +14,7 @@ from sqlalchemy import select
|
|||||||
|
|
||||||
from roboco.db.base import get_db_context
|
from roboco.db.base import get_db_context
|
||||||
from roboco.db.tables import AgentTable, NotificationTable
|
from roboco.db.tables import AgentTable, NotificationTable
|
||||||
|
from roboco.foundation.policy.communications import ACK_REQUIRED_BY_TYPE
|
||||||
from roboco.models import NotificationPriority, NotificationType
|
from roboco.models import NotificationPriority, NotificationType
|
||||||
from roboco.models.notification import CreateNotificationParams
|
from roboco.models.notification import CreateNotificationParams
|
||||||
from roboco.utils.converters import require_uuid
|
from roboco.utils.converters import require_uuid
|
||||||
@@ -522,6 +523,15 @@ class NotificationService:
|
|||||||
subject=params.subject,
|
subject=params.subject,
|
||||||
body=params.body,
|
body=params.body,
|
||||||
related_task_id=params.related_task_id,
|
related_task_id=params.related_task_id,
|
||||||
|
# F009: requires_ack follows ACK_REQUIRED_BY_TYPE (the spec's
|
||||||
|
# action-required vs informational split), not the column's True
|
||||||
|
# default. Without this every notification — including
|
||||||
|
# informational REVIEW_REQUEST / DOCUMENTATION_REQUEST /
|
||||||
|
# A2A_REQUEST / MENTION / KNOWLEDGE_SHARE — became requires_ack,
|
||||||
|
# inflating recipients' unacked sets and soft-blocking
|
||||||
|
# i_am_idle into respawn churn. Default to True for an unmapped
|
||||||
|
# type (preserve the safe action-required bias).
|
||||||
|
requires_ack=ACK_REQUIRED_BY_TYPE.get(params.notification_type, True),
|
||||||
)
|
)
|
||||||
db.add(notification)
|
db.add(notification)
|
||||||
await db.flush()
|
await db.flush()
|
||||||
|
|||||||
@@ -17,6 +17,7 @@ from unittest.mock import AsyncMock, MagicMock, patch
|
|||||||
from uuid import UUID, uuid4
|
from uuid import UUID, uuid4
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
|
from roboco.foundation.policy.communications import ACK_REQUIRED_BY_TYPE
|
||||||
from roboco.models import NotificationPriority, NotificationType
|
from roboco.models import NotificationPriority, NotificationType
|
||||||
from roboco.models.notification import CreateNotificationParams
|
from roboco.models.notification import CreateNotificationParams
|
||||||
from roboco.services.notification import (
|
from roboco.services.notification import (
|
||||||
@@ -321,3 +322,91 @@ async def test_create_notification_skips_when_no_resolvable_recipients(
|
|||||||
)
|
)
|
||||||
)
|
)
|
||||||
assert db.added == []
|
assert db.added == []
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# F009 — requires_ack must follow ACK_REQUIRED_BY_TYPE, not the True default
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_informational_notification_does_not_require_ack(
|
||||||
|
svc: NotificationService,
|
||||||
|
) -> None:
|
||||||
|
"""F009: REVIEW_REQUEST / DOCUMENTATION_REQUEST / A2A_REQUEST are
|
||||||
|
informational (pickup proves receipt) — requires_ack must be False, not the
|
||||||
|
NotificationTable True default. A False type forced to True inflates the
|
||||||
|
recipient's unacked set and soft-blocks i_am_idle → respawn churn."""
|
||||||
|
aid = uuid4()
|
||||||
|
db = _FakeDb(agent_uuid=aid)
|
||||||
|
with _patch_db_context(db):
|
||||||
|
await svc.send_qa_ready_notification(
|
||||||
|
task_id="t1", from_agent="be-dev-1", to_qa="be-qa"
|
||||||
|
)
|
||||||
|
await svc.send_a2a_notification(
|
||||||
|
task_id="t2",
|
||||||
|
a2a_context={
|
||||||
|
"from_agent": "be-dev-1",
|
||||||
|
"to_agent": "fe-dev-1",
|
||||||
|
"skill": "react",
|
||||||
|
"message": "hi",
|
||||||
|
"priority": NotificationPriority.NORMAL,
|
||||||
|
},
|
||||||
|
)
|
||||||
|
qa_rows = [r for r in db.added if r.type == NotificationType.REVIEW_REQUEST]
|
||||||
|
a2a_rows = [r for r in db.added if r.type == NotificationType.A2A_REQUEST]
|
||||||
|
assert qa_rows, "REVIEW_REQUEST row should have been inserted"
|
||||||
|
assert a2a_rows, "A2A_REQUEST row should have been inserted"
|
||||||
|
# Identity checks (``is False``) — the mocked flush doesn't apply SQLA's
|
||||||
|
# insert-time default, so pre-fix the attribute is None, not False. The fix
|
||||||
|
# must set it explicitly on the NotificationTable constructor.
|
||||||
|
assert all(r.requires_ack is False for r in qa_rows)
|
||||||
|
assert all(r.requires_ack is False for r in a2a_rows)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_action_required_notification_still_requires_ack(
|
||||||
|
svc: NotificationService,
|
||||||
|
) -> None:
|
||||||
|
"""F009: BLOCKER_ESCALATION / APPROVAL / ALERT are action-required —
|
||||||
|
requires_ack stays True (ACK_REQUIRED_BY_TYPE maps them True)."""
|
||||||
|
aid = uuid4()
|
||||||
|
db = _FakeDb(agent_uuid=aid)
|
||||||
|
with _patch_db_context(db):
|
||||||
|
await svc.send_blocker_notification(
|
||||||
|
task_id="t1", blocker_reason="r", from_agent="system", to_pm="cell-pm"
|
||||||
|
)
|
||||||
|
await svc.send_board_review_complete_notification(task_id="t2")
|
||||||
|
blocker_rows = [
|
||||||
|
r for r in db.added if r.type == NotificationType.BLOCKER_ESCALATION
|
||||||
|
]
|
||||||
|
approval_rows = [r for r in db.added if r.type == NotificationType.APPROVAL]
|
||||||
|
assert blocker_rows and all(r.requires_ack is True for r in blocker_rows)
|
||||||
|
assert approval_rows and all(r.requires_ack is True for r in approval_rows)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_create_notification_requires_ack_derives_from_type(
|
||||||
|
svc: NotificationService,
|
||||||
|
) -> None:
|
||||||
|
"""F009: a raw _create_notification call derives requires_ack from the type
|
||||||
|
via ACK_REQUIRED_BY_TYPE (KNOWLEDGE_SHARE → False)."""
|
||||||
|
aid = uuid4()
|
||||||
|
db = _FakeDb(agent_uuid=aid)
|
||||||
|
with _patch_db_context(db):
|
||||||
|
await svc._create_notification(
|
||||||
|
CreateNotificationParams(
|
||||||
|
notification_type=NotificationType.KNOWLEDGE_SHARE,
|
||||||
|
priority=NotificationPriority.NORMAL,
|
||||||
|
from_agent="be-dev-1",
|
||||||
|
to_agents=["fe-dev-1"],
|
||||||
|
subject="tip",
|
||||||
|
body="reuse the helper",
|
||||||
|
)
|
||||||
|
)
|
||||||
|
rows = [r for r in db.added if r.type == NotificationType.KNOWLEDGE_SHARE]
|
||||||
|
assert rows
|
||||||
|
assert (
|
||||||
|
rows[0].requires_ack is ACK_REQUIRED_BY_TYPE[NotificationType.KNOWLEDGE_SHARE]
|
||||||
|
)
|
||||||
|
assert rows[0].requires_ack is False
|
||||||
|
|||||||
Reference in New Issue
Block a user