From 67b88930718c66a6362492bf8c7b2fb0bf626d40 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 28 Jun 2026 15:38:56 +0200 Subject: [PATCH] =?UTF-8?q?[F048]=20notify:=20reject=20human-only=20recipi?= =?UTF-8?q?ents=20(prompter/secretary)=20=E2=80=94=20no=20agent=20ack=20pa?= =?UTF-8?q?th?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit notify() only checked the SENDER role. The recipient was resolved by NotificationService._resolve_recipients, which drops only unresolvable slugs — it does not exclude human-only roles. The prompter (intake-1) and secretary (secretary-1) are seeded agent rows, so they resolved, and an ack-required ALERT addressed to them sat permanently unacked (no agent auto-acks it), polluted the panel's pending-ack view, and — via the dedup query's ~acked_by.contains — permanently suppressed any later same-purpose notification from the same sender to that human role. The knowledge-share path already excludes all three human-only roles; the general notify path did not. Fix: a recipient-role guard in notify() via _reject_disallowed_recipient (folds the new check into the existing CEO-dependency-block return slot so notify stays under the PLR0911 return limit). Rejects prompter/secretary with not_authorized; the CEO is human too but acks via the panel, so it stays an allowed recipient (its only disallowed case, a dependency-block page, is preserved). TDD test_notify.py (+3: reject prompter, reject secretary, allow CEO). --- roboco/services/gateway/content_actions.py | 44 ++++++++++- tests/unit/gateway/test_notify.py | 85 ++++++++++++++++++++++ 2 files changed, 128 insertions(+), 1 deletion(-) diff --git a/roboco/services/gateway/content_actions.py b/roboco/services/gateway/content_actions.py index 0b0e6cfc..1e248870 100644 --- a/roboco/services/gateway/content_actions.py +++ b/roboco/services/gateway/content_actions.py @@ -1141,7 +1141,11 @@ class ContentActions: # A dependency block is a "wait silently" situation — never a CEO signal. # An agent must not page the CEO to relax or escalate a task that is # simply waiting on an unfinished upstream; that wait clears on its own. - if reject := await self._reject_ceo_dependency_notify(target, task_id): + # F048: also reject human-only recipients (prompter/secretary) — they + # have no agent ack path, so an ack-required signal would sit permanently + # unacked and suppress later same-purpose notifications via the dedup + # query. The CEO acks via the panel and stays an allowed recipient. + if reject := await self._reject_disallowed_recipient(target, task_id): return reject await self.notifications.send_ack_notification( from_agent=agent_id, @@ -1157,6 +1161,44 @@ class ContentActions: context_briefing={}, ) + async def _reject_disallowed_recipient( + self, target: str, task_id: UUID | None + ) -> Envelope | None: + """Rejection envelope for a notify() recipient the design disallows. + + Two cases, checked in order: + 1. F048 — a human-only recipient (prompter/secretary) with no agent ack + path. The knowledge-share path already excludes all three human-only + roles (learning.py); the general notify path did not, so an + ack-required ALERT could reach a human-driven role and sit permanently + unacked (polluting the panel's pending-ack view and, via the dedup + query's ``~acked_by.contains``, permanently suppressing any later + same-purpose notification from the same sender to that human role). + The CEO is human too but acks via the panel, so it is NOT rejected + here (its only disallowed case — a dependency-block page — is case 2). + 2. A CEO notification about an open dependency block — pure noise; the + wait clears when the upstream completes. + """ + from roboco.agents_config import get_agent_role + + recipient_role = get_agent_role(target) + if recipient_role in ("prompter", "secretary"): + return Envelope.not_authorized( + message=( + f"cannot notify {target!r} — the {recipient_role} is a" + " human-only role with no agent ack path; an ack-required" + " signal would sit permanently unacked and suppress later" + " same-purpose notifications via the dedup query" + ), + remediate=( + "use say() to a channel the human reads, or escalate via the" + " CEO route. ack-required notify() targets must be agents" + " (or the CEO, who acks via the panel)" + ), + context_briefing={}, + ) + return await self._reject_ceo_dependency_notify(target, task_id) + async def _reject_ceo_dependency_notify( self, target: str, task_id: UUID | None ) -> Envelope | None: diff --git a/tests/unit/gateway/test_notify.py b/tests/unit/gateway/test_notify.py index 12fd8ac0..1497eee4 100644 --- a/tests/unit/gateway/test_notify.py +++ b/tests/unit/gateway/test_notify.py @@ -232,6 +232,91 @@ async def test_notify_auditor_rejected_with_not_authorized() -> None: notif_svc.send_ack_notification.assert_not_awaited() +@pytest.mark.asyncio +async def test_notify_rejects_prompter_recipient() -> None: + """F048: the prompter (intake-1) is a human-only role with no agent ack + path. An ack-required ALERT sent to it sits permanently unacked and — via + the dedup query's ``~acked_by.contains`` — permanently suppresses any + later same-purpose notification to that role. The notify verb must reject + a prompter recipient at the handler, not deliver an un-ackable signal.""" + agent_id = uuid4() + task_svc = AsyncMock() + task_svc.get_active_task_for_agent.return_value = None + task_svc.agent_for.return_value = MagicMock(role="cell_pm") + notif_svc = AsyncMock() + + deps = _make_deps(task=task_svc, notifications=notif_svc) + ca = ContentActions(deps) + + env = await ca.notify( + agent_id=agent_id, + target="intake-1", + text="Please ack this formal signal before proceeding.", + ) + body = env.as_dict() + + assert body["error"] == "not_authorized" + assert ( + "prompter" in body["message"].lower() or "human-only" in body["message"].lower() + ) + notif_svc.send_ack_notification.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_notify_rejects_secretary_recipient() -> None: + """F048: the secretary (secretary-1) is human-only with no agent ack path — + same un-ackable-signal + dedup-suppression hazard as the prompter.""" + agent_id = uuid4() + task_svc = AsyncMock() + task_svc.get_active_task_for_agent.return_value = None + task_svc.agent_for.return_value = MagicMock(role="main_pm") + notif_svc = AsyncMock() + + deps = _make_deps(task=task_svc, notifications=notif_svc) + ca = ContentActions(deps) + + env = await ca.notify( + agent_id=agent_id, + target="secretary-1", + text="Please ack this formal signal before proceeding.", + ) + body = env.as_dict() + + assert body["error"] == "not_authorized" + assert ( + "secretary" in body["message"].lower() + or "human-only" in body["message"].lower() + ) + notif_svc.send_ack_notification.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_notify_allows_ceo_recipient() -> None: + """F048: the CEO is human-only too, but the human acks via the panel, so a + non-dependency-block CEO notification is a valid ack-required target. The + recipient guard must NOT over-exclude the CEO (only prompter/secretary).""" + agent_id = uuid4() + task_svc = AsyncMock() + task_svc.get_active_task_for_agent.return_value = None + task_svc.get_journal_context_task_for_agent.return_value = None + task_svc.agent_for.return_value = MagicMock(role="cell_pm") + notif_svc = AsyncMock() + + deps = _make_deps(task=task_svc, notifications=notif_svc) + ca = ContentActions(deps) + + env = await ca.notify( + agent_id=agent_id, + target="ceo", + text="Heads up: this major task is ready for your final approval.", + ) + body = env.as_dict() + + assert body["error"] is None + assert body["status"] == "sent" + notif_svc.send_ack_notification.assert_awaited_once() + + @pytest.mark.asyncio async def test_notify_auto_fills_task_id_from_active_task() -> None: """When the PM has an active task, notify auto-attaches it."""