mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[F048] notify: reject human-only recipients (prompter/secretary) — no agent ack path
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).
This commit is contained in:
@@ -1141,7 +1141,11 @@ class ContentActions:
|
|||||||
# A dependency block is a "wait silently" situation — never a CEO signal.
|
# 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
|
# 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.
|
# 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
|
return reject
|
||||||
await self.notifications.send_ack_notification(
|
await self.notifications.send_ack_notification(
|
||||||
from_agent=agent_id,
|
from_agent=agent_id,
|
||||||
@@ -1157,6 +1161,44 @@ class ContentActions:
|
|||||||
context_briefing={},
|
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(
|
async def _reject_ceo_dependency_notify(
|
||||||
self, target: str, task_id: UUID | None
|
self, target: str, task_id: UUID | None
|
||||||
) -> Envelope | None:
|
) -> Envelope | None:
|
||||||
|
|||||||
@@ -232,6 +232,91 @@ async def test_notify_auditor_rejected_with_not_authorized() -> None:
|
|||||||
notif_svc.send_ack_notification.assert_not_awaited()
|
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
|
@pytest.mark.asyncio
|
||||||
async def test_notify_auto_fills_task_id_from_active_task() -> None:
|
async def test_notify_auto_fills_task_id_from_active_task() -> None:
|
||||||
"""When the PM has an active task, notify auto-attaches it."""
|
"""When the PM has an active task, notify auto-attaches it."""
|
||||||
|
|||||||
Reference in New Issue
Block a user