From 3bb54e73bcbf06e18134c6a8bb6e483df692dcab Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:55:20 +0000 Subject: [PATCH 1/6] Let an agent post to a Slack conversation it names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every outbound path today answers someone who wrote in first, so the destination comes from their message. A cron run has no such message, and `router.deliver` — the one target-addressed path — had no callers and no authorization, so wiring anything through it would have posted wherever it was told. Write policy belongs to the channel, not the tool and not the router: only the channel knows what a target means. `BaseChannel.authorize_outbound` refuses by default, matching `send_file`, which declines rather than infer a destination. Slack reads `slack.allow_channels` in the write direction — no new config keys, so writes cannot widen while reads narrow. Two asymmetries with the inbound policy are deliberate. `allow_users` grants nothing, because it says who may drive the agent, not where it may broadcast, and there is no sender here to have vetted; with no `allow_channels` set every target is refused. Unsolicited DMs are refused outright even under `allow_direct_messages`: an inbound DM comes from someone who chose to write, an outbound one does not, and gating the recipient properly needs a member lookup through the user rules. `_notification_target` still accepts a `D` — that is an operator writing one config value, not an agent picking a destination at runtime. `deliver` keeps its signature for fire-and-forget callers and delegates to `deliver_addressed`, which returns the Decision so the tool can report why a refusal happened. A transport failure propagates instead of becoming a refusal — "the policy said no" and "Slack was down" are different answers. Co-Authored-By: Claude Opus 5 --- docs/config.md | 29 ++ nerve/agent/tools/handlers/notifications.py | 71 +++- nerve/agent/tools/schemas.py | 26 ++ nerve/channels/base.py | 19 ++ nerve/channels/router.py | 38 ++- nerve/channels/slack.py | 49 ++- nerve/channels/slack_access.py | 18 + tests/test_channel_outbound.py | 343 ++++++++++++++++++++ tests/test_tool_registry.py | 1 + 9 files changed, 588 insertions(+), 6 deletions(-) create mode 100644 tests/test_channel_outbound.py diff --git a/docs/config.md b/docs/config.md index 7172221b..5db2b67a 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1275,6 +1275,35 @@ warning. Slack in the list costs nothing while Slack is off. Names and globs are not resolved for the `slack_channel_id` fallback. Without a literal channel ID, delivery is skipped with a warning. +### Addressed delivery + +The `send_channel_message` tool posts to a conversation the agent names, +rather than to whichever chat it is answering. That makes it usable from a +cron run with no conversation attached — and means the destination, not the +sender, is what has to be authorized. + +The grant is `slack.allow_channels` read in the write direction: the agent may +post to a conversation an operator already named, and `slack.deny_channels` +still refuses. There are no separate write keys, so writes cannot widen while +reads narrow. + +Three differences from the inbound policy are deliberate: + +- **`slack.allow_users` grants nothing here.** It says who may drive the + agent, not where the agent may broadcast. With no `slack.allow_channels` + set, every target is refused — unlike an inbound check, there is no sender + to have vetted first. +- **Unsolicited DMs are refused**, even with `slack.allow_direct_messages`. + An inbound DM comes from someone who chose to write; an outbound one does + not. Gating one properly means resolving the conversation's member and + running them through the user rules, which is a separate change. +- **`notifications.slack_channel_id` may still be a `D`.** That is an + operator writing one config value, not an agent choosing a destination at + runtime, so the two do not share a policy. + +Other channels refuse addressed delivery outright until they implement the +same seam. + ## Quiet Hours | Key | Type | Default | Description | diff --git a/nerve/agent/tools/handlers/notifications.py b/nerve/agent/tools/handlers/notifications.py index 0c9ce35a..77ebd8f6 100644 --- a/nerve/agent/tools/handlers/notifications.py +++ b/nerve/agent/tools/handlers/notifications.py @@ -1,9 +1,15 @@ -"""Notification tool handlers — notify, ask_user, propose_action, react, send_sticker, send_file. +"""Notification tool handlers — notify, ask_user, propose_action, react, +send_sticker, send_file, send_channel_message. -All six tools need ``ctx.session_id`` so the channel router can deliver -to the correct chat (web, Telegram). The session_id arrives via +Most of these need ``ctx.session_id`` so the channel router can deliver to +the correct chat (web, Telegram). The session_id arrives via :class:`ToolContext`; there's no per-tool special-casing left. +``send_channel_message`` is the exception: it addresses a conversation the +caller names, so it never reads the session's message context. That is what +makes it usable from a cron run, and it is why the destination has to clear +the channel's own write policy first. + ``propose_action`` files an ``approval``-kind notification whose answer routes through a server-side dispatcher (``ctx.notification_service``) instead of being injected back into the originating session. @@ -29,6 +35,7 @@ NOTIFY_SCHEMA, PROPOSE_ACTION_SCHEMA, REACT_SCHEMA, + SEND_CHANNEL_MESSAGE_SCHEMA, SEND_FILE_SCHEMA, SEND_STICKER_SCHEMA, ) @@ -438,6 +445,47 @@ async def send_file_handler(ctx: ToolContext, args: dict) -> ToolResult: ) +async def send_channel_message_handler(ctx: ToolContext, args: dict) -> ToolResult: + """Post to a conversation the caller names, on the transport it names. + + The target is passed straight through and never inferred from the + session's last inbound message, so this works from a cron run with no + chat context — and cannot silently retarget a different conversation + when it does have one. + + A policy refusal comes back as text rather than ``is_error``, matching + ``react``: the agent asked a reasonable question and got a "no" with a + reason, which is an answer, not a malfunction. + """ + if not ctx.engine: + return ToolResult.text("Engine not available.") + + channel = args.get("channel", "").strip() + target = args.get("target", "").strip() + text = args.get("text", "") + + if not channel: + return ToolResult.text("Error: channel is required.") + if not target: + return ToolResult.text("Error: target is required.") + if not text.strip(): + return ToolResult.text("Error: text is required.") + + try: + decision = await ctx.engine.router.deliver_addressed( + channel, target, text, session_id=ctx.session_id, + ) + except Exception as e: + logger.error("send_channel_message dispatch failed: %s", e) + return ToolResult.text(f"Failed to send message on {channel}: {e}") + + if decision.allowed: + return ToolResult.text(f"Message sent to {channel} target {target}.") + return ToolResult.text( + f"Refused: cannot send to {channel} target {target} — {decision.reason}" + ) + + NOTIFY_SPEC = ToolSpec( name="notify", description=( @@ -527,6 +575,22 @@ async def send_file_handler(ctx: ToolContext, args: dict) -> ToolResult: handler=send_file_handler, ) +SEND_CHANNEL_MESSAGE_SPEC = ToolSpec( + name="send_channel_message", + description=( + "Post a message to a chat conversation you name, on the transport " + "you name — e.g. a Slack channel. Unlike 'notify', this does not go " + "to the user's notification inbox, and unlike a normal reply it is " + "not tied to the current chat, so it works from a cron run with no " + "conversation attached. The destination must be approved by that " + "channel's write policy (Slack: it must match slack.allow_channels); " + "a refusal comes back with the reason. Unsolicited direct messages " + "are not supported." + ), + input_schema=SEND_CHANNEL_MESSAGE_SCHEMA, + handler=send_channel_message_handler, +) + NOTIFICATION_SPECS = [ NOTIFY_SPEC, @@ -536,4 +600,5 @@ async def send_file_handler(ctx: ToolContext, args: dict) -> ToolResult: REACT_SPEC, SEND_STICKER_SPEC, SEND_FILE_SPEC, + SEND_CHANNEL_MESSAGE_SPEC, ] diff --git a/nerve/agent/tools/schemas.py b/nerve/agent/tools/schemas.py index 8c921e0d..0385b5f4 100644 --- a/nerve/agent/tools/schemas.py +++ b/nerve/agent/tools/schemas.py @@ -832,6 +832,32 @@ "required": ["file_path"], } +SEND_CHANNEL_MESSAGE_SCHEMA = { + "type": "object", + "properties": { + "channel": { + "type": "string", + "description": ( + "Transport to send through, e.g. 'slack'. This is not the " + "conversation — that is 'target'." + ), + }, + "target": { + "type": "string", + "description": ( + "Conversation to post to, in that transport's own addressing. " + "Slack: a conversation id such as 'C0123ABCD', or " + "'C0123ABCD:1700000000.000100' to reply inside a thread." + ), + }, + "text": { + "type": "string", + "description": "Message body, in Markdown.", + }, + }, + "required": ["channel", "target", "text"], +} + # ----- MCP admin tools ----- NERVE_API_SCHEMA = { diff --git a/nerve/channels/base.py b/nerve/channels/base.py index 65f84f34..44b03a4f 100644 --- a/nerve/channels/base.py +++ b/nerve/channels/base.py @@ -13,6 +13,8 @@ from enum import Flag, auto from typing import Any +from nerve.channels.access import Decision + class ChannelCapability(Flag): """Capabilities a channel can declare. @@ -191,6 +193,23 @@ async def send_interaction( For Web, this is a JSON event over WebSocket. """ + # ------------------------------------------------------------------ # + # Optional: addressed delivery # + # ------------------------------------------------------------------ # + + async def authorize_outbound(self, target: str) -> Decision: + """Whether an agent may send an unsolicited message to *target*. + + Addressed delivery is the one path where the destination comes from + the agent rather than from a person who wrote in first, so the write + policy belongs to the channel that knows what a target means. The + router asks; the channel decides. + + Refusing by default matches :meth:`send_file`, which declines rather + than infer a destination. A channel opts in by overriding this. + """ + return Decision(False, f"{self.name} does not accept addressed delivery") + # ------------------------------------------------------------------ # # Optional: file delivery # # Only called if channel declares ChannelCapability.SEND_FILES. # diff --git a/nerve/channels/router.py b/nerve/channels/router.py index f904c754..9587e4cf 100644 --- a/nerve/channels/router.py +++ b/nerve/channels/router.py @@ -17,6 +17,7 @@ from nerve.agent.interactive import get_handler from nerve.agent.streaming import broadcaster +from nerve.channels.access import Decision from nerve.channels.base import ( BaseChannel, ChannelCapability, @@ -498,12 +499,44 @@ async def deliver( ) -> None: """Deliver a complete message to a channel target. - Used by cron jobs and other non-interactive output delivery. + Used by cron jobs and other non-interactive output delivery, which + have no return value to read — see :meth:`deliver_addressed` for the + same send with the refusal reason attached. + """ + await self.deliver_addressed(channel_name, target, message, session_id) + + async def deliver_addressed( + self, + channel_name: str, + target: str, + message: str, + session_id: str | None = None, + ) -> Decision: + """Deliver to a target the caller names, reporting why if refused. + + The target is always supplied by the caller and never inferred from + ``_message_context``, so a cron run cannot spill into whatever chat + last touched the session — the same reasoning as :meth:`send_file`. + + The channel authorizes the destination first, because only it knows + what a target means; see :meth:`BaseChannel.authorize_outbound`. + Returning the :class:`~nerve.channels.access.Decision` lets a caller + say why nothing was sent instead of only that nothing was. + + A transport failure propagates rather than becoming a refusal: "the + policy said no" and "Slack was down" are different answers. """ channel = self._channels.get(channel_name) if not channel: logger.warning("Cannot deliver to unknown channel: %s", channel_name) - return + return Decision(False, f"unknown channel {channel_name!r}") + + verdict = await channel.authorize_outbound(target) + if not verdict.allowed: + logger.info( + "Refused addressed delivery to %s: %s", channel_name, verdict.reason, + ) + return verdict formatted = channel.format_response(message) await channel.send(OutboundMessage( @@ -511,6 +544,7 @@ async def deliver( text=formatted, session_id=session_id or "", )) + return verdict # ------------------------------------------------------------------ # # Streaming adapter lifecycle # diff --git a/nerve/channels/slack.py b/nerve/channels/slack.py index 80560fcc..b6f76be1 100644 --- a/nerve/channels/slack.py +++ b/nerve/channels/slack.py @@ -16,7 +16,7 @@ from pathlib import Path from typing import Any, TYPE_CHECKING -from nerve.channels.access import Identity, needs_name_resolution +from nerve.channels.access import Decision, Identity, needs_name_resolution from nerve.channels.archives import ( IMAGE_EXT_TO_MIME, MAX_TEXT_SIZE, @@ -1226,6 +1226,53 @@ async def _post( ) return resp.get("ts") + async def authorize_outbound(self, target: str) -> Decision: + """Whether an agent may post to *target* unprompted. + + The grant is ``slack.allow_channels`` read in the write direction: the + agent may post to a conversation an operator already named. No new + config keys, so writes cannot be widened by accident while reads are + narrowed. + + Unsolicited direct messages are refused outright. An inbound DM comes + from someone who chose to write; an outbound one does not, and + ``allow_direct_messages`` was never asked to authorize a recipient the + agent names for itself. Gating that properly means resolving the + conversation's member and running it through ``policy.users``, which + is a separate decision with its own test surface. + + This is deliberately stricter than :meth:`_notification_target`, which + does accept a ``D``: that is an operator writing one config value, not + an agent choosing a destination at runtime. + """ + channel_id, _ = parse_target(target) + if not channel_id: + return Decision(False, "no Slack conversation id in the target") + + if channel_id.startswith("D"): + return Decision( + False, + "unsolicited direct messages are not supported; address a " + "channel the policy allows instead", + ) + if not channel_id.startswith(("C", "G")): + return Decision( + False, + f"{channel_id!r} is not a Slack channel or group conversation id", + ) + + policy = self.policy + if not policy.channels.allow: + # Skip the lookup a refusal cannot use. + return policy.check_outbound(Identity(id=channel_id)) + + conversation = await self._identify_conversation( + channel_id, + "channel", + needs_name_resolution(policy.channels, is_id=is_slack_id), + ) + return policy.check_outbound(conversation) + def _notification_target(self) -> str | None: """Resolve a concrete conversation from the active config generation.""" configured = self.config.notifications.slack_channel_id.strip() diff --git a/nerve/channels/slack_access.py b/nerve/channels/slack_access.py index cd002e45..9f638cf8 100644 --- a/nerve/channels/slack_access.py +++ b/nerve/channels/slack_access.py @@ -80,6 +80,24 @@ def check( ) return self.channels.check(channel) + def check_outbound(self, channel: Identity) -> Decision: + """Decide whether the agent may post to a shared conversation unasked. + + Read in the write direction the policy is short one term: there is no + sender to run through :attr:`users`. An allow list of users therefore + grants nothing here — it says who may drive the agent, not where the + agent may broadcast — so an explicit :attr:`channels` grant is + required, the same instinct as refusing shared channels to a lone + ``allow_direct_messages``. + """ + if not self.channels.allow: + return Decision( + False, + "no slack.allow_channels configured, so no conversation is " + "approved for addressed delivery", + ) + return self.channels.check(channel) + def describe(self) -> str: """Summarize the policy without exposing configured patterns.""" return ( diff --git a/tests/test_channel_outbound.py b/tests/test_channel_outbound.py new file mode 100644 index 00000000..56978207 --- /dev/null +++ b/tests/test_channel_outbound.py @@ -0,0 +1,343 @@ +"""Target-addressed delivery — who may post where, unprompted. + +Every other outbound path answers a person who wrote in first, so the +destination comes from their message. Here the agent names it, which makes +the destination the thing that has to be authorized. The policy seam is +``BaseChannel.authorize_outbound``: the router asks, the channel decides. + +Covers the Slack write policy, the default refusal every other channel +inherits, the router guard on ``deliver``/``deliver_addressed``, and the +``send_channel_message`` handler that reports a refusal's reason. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from nerve.agent.tools.handlers.notifications import send_channel_message_handler +from nerve.agent.tools.registry import ToolContext +from nerve.channels.access import Decision +from nerve.channels.base import BaseChannel, ChannelCapability +from nerve.channels.router import ChannelRouter +from nerve.channels.slack import SlackChannel +from nerve.config import NerveConfig, SlackConfig + +pytestmark = pytest.mark.asyncio + + +# ---------------------------------------------------------------------- # +# Doubles # +# ---------------------------------------------------------------------- # + + +class _PlainChannel(BaseChannel): + """A channel that never overrode ``authorize_outbound``.""" + + def __init__(self, name: str = "plain"): + self._name = name + self.sent: list[tuple[str, str]] = [] + + @property + def name(self) -> str: + return self._name + + @property + def capabilities(self) -> ChannelCapability: + return ChannelCapability.SEND_TEXT + + async def start(self) -> None: + pass + + async def stop(self) -> None: + pass + + async def send(self, message) -> None: + self.sent.append((message.target, message.text)) + + +def _slack(**slack_kwargs) -> SlackChannel: + """A Slack channel with a stub transport, ready to authorize.""" + cfg = NerveConfig() + cfg.slack = SlackConfig( + enabled=True, + bot_token="xoxb-test", + app_token="xapp-test", + **slack_kwargs, + ) + channel = SlackChannel(cfg, router=MagicMock()) + channel._web = MagicMock() + channel._web.chat_postMessage = AsyncMock(return_value={"ts": "1.1"}) + channel._web.conversations_info = AsyncMock( + return_value={"channel": {"name": "general"}}, + ) + channel._state = "running" + return channel + + +# ---------------------------------------------------------------------- # +# Slack write policy # +# ---------------------------------------------------------------------- # + + +class TestSlackAuthorizeOutbound: + async def test_an_allowed_conversation_is_approved(self): + channel = _slack(allow_channels=["C0123ABCD"]) + + verdict = await channel.authorize_outbound("C0123ABCD") + + assert verdict.allowed + + async def test_a_thread_target_is_approved_on_its_conversation(self): + # The thread ts addresses a reply inside a conversation already + # granted; splitting it off would refuse the allowed channel. + channel = _slack(allow_channels=["C0123ABCD"]) + + verdict = await channel.authorize_outbound("C0123ABCD:1700000000.000100") + + assert verdict.allowed + + async def test_a_conversation_off_the_allow_list_is_refused(self): + channel = _slack(allow_channels=["C0123ABCD"]) + + verdict = await channel.authorize_outbound("C0999ZZZZ") + + assert not verdict.allowed + assert "allow list" in verdict.reason + + async def test_a_denied_conversation_names_the_pattern(self): + channel = _slack(allow_channels=["*"], deny_channels=["C0999ZZZZ"]) + + verdict = await channel.authorize_outbound("C0999ZZZZ") + + assert not verdict.allowed + assert "C0999ZZZZ" in verdict.reason + assert "deny" in verdict.reason + + async def test_a_direct_message_is_refused(self): + # An inbound DM comes from someone who chose to write. An outbound + # one does not, and allow_direct_messages never authorized a + # recipient the agent picks for itself. + channel = _slack(allow_channels=["*"], allow_direct_messages=True) + + verdict = await channel.authorize_outbound("D0123ABCD") + + assert not verdict.allowed + assert "direct message" in verdict.reason + + async def test_an_unresolvable_name_is_refused_when_a_deny_list_needs_one(self): + # conversations.info failing leaves the identity incomplete, and a + # deny list cannot be checked against a name nobody could read. + channel = _slack(allow_channels=["*"], deny_channels=["secrets"]) + channel._web.conversations_info = AsyncMock(side_effect=RuntimeError("boom")) + + verdict = await channel.authorize_outbound("C0123ABCD") + + assert not verdict.allowed + assert "could not be fully identified" in verdict.reason + + async def test_a_name_grant_matches_the_resolved_conversation(self): + channel = _slack(allow_channels=["general"]) + + verdict = await channel.authorize_outbound("C0123ABCD") + + assert verdict.allowed + channel._web.conversations_info.assert_awaited_once() + + async def test_no_allow_channels_refuses_everything(self): + # The empty PatternGate allows all comers, which is right for an + # inbound check that already ran the user gate and wrong here: + # there is no sender to have vetted. Without an explicit grant + # there is no approved destination at all. + channel = _slack(allow_users=["U0123ABCD"]) + + verdict = await channel.authorize_outbound("C0123ABCD") + + assert not verdict.allowed + assert "slack.allow_channels" in verdict.reason + + async def test_an_allowed_user_does_not_grant_a_channel(self): + # allow_users says who may drive the agent, not where it may + # broadcast. Reading it as a write grant would hand every + # conversation the bot sits in to a cron run. + channel = _slack(allow_users=["*"], deny_channels=["C0999ZZZZ"]) + + verdict = await channel.authorize_outbound("C0123ABCD") + + assert not verdict.allowed + + async def test_a_refusal_costs_no_slack_api_call(self): + channel = _slack() + + await channel.authorize_outbound("C0123ABCD") + + channel._web.conversations_info.assert_not_awaited() + + async def test_a_user_id_is_not_a_conversation(self): + channel = _slack(allow_channels=["*"]) + + verdict = await channel.authorize_outbound("U0123ABCD") + + assert not verdict.allowed + assert "not a Slack channel" in verdict.reason + + async def test_an_empty_target_is_refused(self): + channel = _slack(allow_channels=["*"]) + + verdict = await channel.authorize_outbound("") + + assert not verdict.allowed + assert "no Slack conversation id" in verdict.reason + + +class TestDefaultRefusal: + async def test_a_channel_without_an_override_refuses(self): + verdict = await _PlainChannel().authorize_outbound("anything") + + assert not verdict.allowed + assert "addressed delivery" in verdict.reason + + +# ---------------------------------------------------------------------- # +# Router guard # +# ---------------------------------------------------------------------- # + + +class TestRouterDeliver: + async def test_an_approved_target_receives_the_message(self): + router = ChannelRouter(MagicMock()) + channel = _slack(allow_channels=["C0123ABCD"]) + router.register(channel) + + verdict = await router.deliver_addressed("slack", "C0123ABCD", "hello") + + assert verdict.allowed + channel._web.chat_postMessage.assert_awaited_once() + assert channel._web.chat_postMessage.await_args.kwargs["channel"] == "C0123ABCD" + + async def test_the_caller_target_is_used_verbatim(self): + # The whole point of this path: a session's last inbound message + # must never redirect a delivery the caller addressed itself. + router = ChannelRouter(MagicMock()) + channel = _slack(allow_channels=["C0123ABCD"]) + router.register(channel) + router._message_context["s1"] = { + "channel_name": "slack", + "target": "C0999ZZZZ", + "message_id": "1.0", + } + + await router.deliver_addressed("slack", "C0123ABCD", "hello", "s1") + + assert channel._web.chat_postMessage.await_args.kwargs["channel"] == "C0123ABCD" + + async def test_a_refused_target_is_not_sent_to(self): + router = ChannelRouter(MagicMock()) + channel = _slack(allow_channels=["C0123ABCD"]) + router.register(channel) + + verdict = await router.deliver_addressed("slack", "C0999ZZZZ", "hello") + + assert not verdict.allowed + channel._web.chat_postMessage.assert_not_awaited() + + async def test_deliver_refuses_a_channel_that_never_opted_in(self): + router = ChannelRouter(MagicMock()) + channel = _PlainChannel(name="plain") + router.register(channel) + + await router.deliver("plain", "somewhere", "hello") + + assert channel.sent == [] + + async def test_an_unknown_channel_is_refused(self): + router = ChannelRouter(MagicMock()) + + verdict = await router.deliver_addressed("nope", "C0123ABCD", "hello") + + assert not verdict.allowed + assert "unknown channel" in verdict.reason + + async def test_a_transport_failure_is_not_reported_as_a_refusal(self): + # "the policy said no" and "Slack was down" are different answers, + # and only one of them is worth changing the config over. + router = ChannelRouter(MagicMock()) + channel = _slack(allow_channels=["C0123ABCD"]) + channel._web.chat_postMessage = AsyncMock(side_effect=RuntimeError("boom")) + router.register(channel) + + with pytest.raises(RuntimeError): + await router.deliver_addressed("slack", "C0123ABCD", "hello") + + +# ---------------------------------------------------------------------- # +# Tool handler # +# ---------------------------------------------------------------------- # + + +def _ctx(decision) -> ToolContext: + engine = MagicMock() + engine.router.deliver_addressed = AsyncMock(return_value=decision) + return ToolContext(session_id="s1", engine=engine) + + +class TestSendChannelMessageHandler: + async def test_a_delivered_message_is_confirmed(self): + ctx = _ctx(Decision(True, "ok")) + + result = await send_channel_message_handler( + ctx, {"channel": "slack", "target": "C0123ABCD", "text": "hi"}, + ) + + assert "sent" in result.content[0]["text"].lower() + ctx.engine.router.deliver_addressed.assert_awaited_once_with( + "slack", "C0123ABCD", "hi", session_id="s1", + ) + + async def test_a_refusal_reports_the_reason(self): + ctx = _ctx(Decision(False, "channel general (C1) is not on the allow list")) + + result = await send_channel_message_handler( + ctx, {"channel": "slack", "target": "C0123ABCD", "text": "hi"}, + ) + + text = result.content[0]["text"] + assert "Refused" in text + assert "not on the allow list" in text + assert not result.is_error + + async def test_a_transport_failure_is_reported(self): + engine = MagicMock() + engine.router.deliver_addressed = AsyncMock(side_effect=RuntimeError("boom")) + ctx = ToolContext(session_id="s1", engine=engine) + + result = await send_channel_message_handler( + ctx, {"channel": "slack", "target": "C0123ABCD", "text": "hi"}, + ) + + assert "Failed" in result.content[0]["text"] + + @pytest.mark.parametrize( + "args,missing", + [ + ({"channel": "", "target": "C1", "text": "hi"}, "channel"), + ({"channel": "slack", "target": " ", "text": "hi"}, "target"), + ({"channel": "slack", "target": "C1", "text": " "}, "text"), + ], + ) + async def test_a_missing_field_is_named(self, args, missing): + ctx = _ctx(Decision(True, "ok")) + + result = await send_channel_message_handler(ctx, args) + + assert missing in result.content[0]["text"] + ctx.engine.router.deliver_addressed.assert_not_awaited() + + async def test_no_engine_is_reported(self): + result = await send_channel_message_handler( + ToolContext(session_id="s1"), + {"channel": "slack", "target": "C1", "text": "hi"}, + ) + + assert "Engine not available" in result.content[0]["text"] diff --git a/tests/test_tool_registry.py b/tests/test_tool_registry.py index 1ae291c0..86a0ca53 100644 --- a/tests/test_tool_registry.py +++ b/tests/test_tool_registry.py @@ -161,6 +161,7 @@ def test_default_registry_contains_expected_tools(self): "list_sources", "poll_source", # notifications "notify", "ask_user", "react", "send_sticker", "send_file", + "send_channel_message", # mcp admin "nerve_api", "mcp_reload", # workflow runs From 795669e386f400e3e2db2d4b7531aa8d74306495 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Wed, 2 Sep 2026 16:33:49 +0000 Subject: [PATCH 2/6] Close the group-DM hole, and stop echoing policy detail to the agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the previous commit. A `G` id is not just a legacy private channel — Slack also uses it for multi-person DMs, which expose `is_mpim`. Refusing only `D` therefore let a group DM straight through the door marked "no unsolicited DMs": allow_channels ["*"] plus a `G` MPIM target posted the message. A `G` target now costs one cached conversations.info call to tell the two apart, and a lookup that cannot answer is refused rather than guessed. `C` targets are unambiguous and still free. The target must also now clear `is_slack_id` in full rather than matching on its first letter, so a malformed id is refused with an accurate reason instead of failing later at the Slack API. Names remain rejected: a name would make the destination depend on a lookup the caller does not control. The refusal reason went back to the agent verbatim, and a PatternGate verdict names both the resolved conversation and the glob that matched it — with deny_channels ["secret-*"], a probe returned "channel secret-payroll (...) matches deny pattern 'secret-*'". The agent may repeat that into a chat. Detail now goes to the log and the agent gets a coarse refusal, the same reasoning behind SlackAccessPolicy.describe. The unconfigured case is still explicit: naming an unset key tells an operator what to do without disclosing what is in it. Transport failures now set is_error, so turn telemetry stops recording a message that never arrived as a successful call; policy refusals stay non-error, because a reasoned "no" is an answer. The failure text also warns that a long message may have been partly posted before the failure, since a bare "failed" reads as "nothing happened" and invites a duplicating retry. Not fixed here, both pre-existing and shared with the inbound path: a failed name lookup is cached for the full 10 minutes, and a cold burst can issue one conversations.info per concurrent message. Neither is introduced by this change and both want their own PR. Co-Authored-By: Claude Opus 5 --- docs/config.md | 14 ++++ nerve/agent/tools/handlers/notifications.py | 27 +++++-- nerve/channels/slack.py | 88 ++++++++++++++++++-- tests/test_channel_outbound.py | 89 ++++++++++++++++++--- 4 files changed, 194 insertions(+), 24 deletions(-) diff --git a/docs/config.md b/docs/config.md index 5db2b67a..c24705cc 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1297,10 +1297,24 @@ Three differences from the inbound policy are deliberate: An inbound DM comes from someone who chose to write; an outbound one does not. Gating one properly means resolving the conversation's member and running them through the user rules, which is a separate change. + **Group DMs count.** A `G` id is either a legacy private channel or a + multi-person DM, and only `conversations.info` can tell them apart, so a + `G` target costs one cached lookup; a lookup that cannot answer is refused + rather than guessed. - **`notifications.slack_channel_id` may still be a `D`.** That is an operator writing one config value, not an agent choosing a destination at runtime, so the two do not share a policy. +The target must be a literal conversation id, never a name: a name would make +the destination depend on a lookup the caller does not control. + +**Refusal reasons are coarse on purpose.** The reason travels back to the +agent, which may repeat it into a chat, so the detailed verdict — which names +the resolved conversation and the pattern that matched it — goes to the log +and the agent is told only that the destination is not approved. The same +reasoning is why `SlackAccessPolicy.describe()` reports counts rather than +patterns. + Other channels refuse addressed delivery outright until they implement the same seam. diff --git a/nerve/agent/tools/handlers/notifications.py b/nerve/agent/tools/handlers/notifications.py index 77ebd8f6..ba8c524d 100644 --- a/nerve/agent/tools/handlers/notifications.py +++ b/nerve/agent/tools/handlers/notifications.py @@ -453,23 +453,25 @@ async def send_channel_message_handler(ctx: ToolContext, args: dict) -> ToolResu chat context — and cannot silently retarget a different conversation when it does have one. - A policy refusal comes back as text rather than ``is_error``, matching - ``react``: the agent asked a reasonable question and got a "no" with a - reason, which is an answer, not a malfunction. + A policy refusal comes back as plain text, not ``is_error``: the agent + asked a reasonable question and got a reasoned "no", which is an answer + rather than a malfunction. A transport failure *is* ``is_error``, so + turn telemetry does not record a message that never arrived as a + successful call. """ if not ctx.engine: - return ToolResult.text("Engine not available.") + return ToolResult.text("Engine not available.", is_error=True) channel = args.get("channel", "").strip() target = args.get("target", "").strip() text = args.get("text", "") if not channel: - return ToolResult.text("Error: channel is required.") + return ToolResult.text("Error: channel is required.", is_error=True) if not target: - return ToolResult.text("Error: target is required.") + return ToolResult.text("Error: target is required.", is_error=True) if not text.strip(): - return ToolResult.text("Error: text is required.") + return ToolResult.text("Error: text is required.", is_error=True) try: decision = await ctx.engine.router.deliver_addressed( @@ -477,7 +479,16 @@ async def send_channel_message_handler(ctx: ToolContext, args: dict) -> ToolResu ) except Exception as e: logger.error("send_channel_message dispatch failed: %s", e) - return ToolResult.text(f"Failed to send message on {channel}: {e}") + # A long message is split into several posts, so a failure partway + # through leaves the earlier parts delivered. Say so: a plain + # "failed" reads as "nothing happened" and invites a retry that + # posts those parts a second time. + return ToolResult.text( + f"Failed to send message on {channel}: {e}. If the message was " + f"long, earlier parts of it may already have been posted — check " + f"the conversation before retrying.", + is_error=True, + ) if decision.allowed: return ToolResult.text(f"Message sent to {channel} target {target}.") diff --git a/nerve/channels/slack.py b/nerve/channels/slack.py index b6f76be1..95f027e0 100644 --- a/nerve/channels/slack.py +++ b/nerve/channels/slack.py @@ -1244,26 +1244,54 @@ async def authorize_outbound(self, target: str) -> Decision: This is deliberately stricter than :meth:`_notification_target`, which does accept a ``D``: that is an operator writing one config value, not an agent choosing a destination at runtime. + + The refusal reason returned here is deliberately coarse. It goes back + to the agent, which may repeat it into a chat, and the detailed + verdict names the resolved conversation and the pattern that matched + — the same reasoning behind :meth:`SlackAccessPolicy.describe`. The + detail goes to the log instead. """ channel_id, _ = parse_target(target) if not channel_id: return Decision(False, "no Slack conversation id in the target") - if channel_id.startswith("D"): + if not is_slack_id(channel_id): return Decision( False, - "unsolicited direct messages are not supported; address a " - "channel the policy allows instead", + "target must be a Slack conversation id, not a name", ) - if not channel_id.startswith(("C", "G")): + if channel_id[0] not in "CG": return Decision( False, - f"{channel_id!r} is not a Slack channel or group conversation id", + "unsolicited direct messages are not supported; address a " + "channel the policy allows instead" + if channel_id[0] == "D" + else f"{channel_id!r} is not a Slack conversation id", ) + # A `G` is ambiguous: legacy private channel or multi-person DM. Only + # conversations.info can say which, so refusing `D` alone would let a + # group DM through the door marked "no unsolicited DMs". + if channel_id[0] == "G": + private = await self._is_private_conversation(channel_id) + if private is None: + return Decision( + False, + f"could not establish what kind of conversation " + f"{channel_id} is", + ) + if private: + return Decision( + False, + "unsolicited direct messages are not supported; address a " + "channel the policy allows instead", + ) + policy = self.policy if not policy.channels.allow: - # Skip the lookup a refusal cannot use. + # Skip the lookup a refusal cannot use. Not redacted: naming an + # unset config key tells the operator what to do and discloses + # nothing about what is in it. return policy.check_outbound(Identity(id=channel_id)) conversation = await self._identify_conversation( @@ -1271,7 +1299,53 @@ async def authorize_outbound(self, target: str) -> Decision: "channel", needs_name_resolution(policy.channels, is_id=is_slack_id), ) - return policy.check_outbound(conversation) + return self._public_verdict(policy.check_outbound(conversation)) + + @staticmethod + def _public_verdict(verdict: Decision) -> Decision: + """Log the policy's detailed reason; hand back a coarse one. + + A refusal reason from :class:`PatternGate` names the conversation it + resolved and the glob that matched it. That is what a log wants and + the opposite of what should travel back to an agent that may be + talking to whoever prompted the send. + """ + if verdict.allowed: + return verdict + logger.info("Slack refused addressed delivery: %s", verdict.reason) + return Decision( + False, "the destination is not approved by the Slack channel policy", + ) + + async def _is_private_conversation(self, channel_id: str) -> bool | None: + """Whether *channel_id* is a DM or multi-person DM. + + Returns None when Slack could not say, so the caller can fail closed + rather than guess. Cached beside the resolved names, since the answer + is a property of the conversation and does not change. + """ + cache_key = f"kind:{channel_id}" + cached = self._name_cache.get(cache_key) + if cached and cached[1] > time.monotonic(): + return cached[0] + try: + info = await self._web.conversations_info(channel=channel_id) + conversation = info.get("channel") or {} + private = bool( + conversation.get("is_mpim") or conversation.get("is_im"), + ) + except Exception as e: + logger.warning( + "Slack conversations.info failed for %s, so its kind is " + "unknown and delivery is refused: %s", + channel_id, e, + ) + return None + self._remember( + self._name_cache, cache_key, + (private, time.monotonic() + _NAME_CACHE_TTL), _NAME_CACHE_MAX, + ) + return private def _notification_target(self) -> str | None: """Resolve a concrete conversation from the active config generation.""" diff --git a/tests/test_channel_outbound.py b/tests/test_channel_outbound.py index 56978207..83fc2d42 100644 --- a/tests/test_channel_outbound.py +++ b/tests/test_channel_outbound.py @@ -104,16 +104,84 @@ async def test_a_conversation_off_the_allow_list_is_refused(self): verdict = await channel.authorize_outbound("C0999ZZZZ") assert not verdict.allowed - assert "allow list" in verdict.reason + assert "not approved" in verdict.reason - async def test_a_denied_conversation_names_the_pattern(self): + async def test_a_denied_conversation_is_refused(self, caplog): channel = _slack(allow_channels=["*"], deny_channels=["C0999ZZZZ"]) - verdict = await channel.authorize_outbound("C0999ZZZZ") + with caplog.at_level("INFO"): + verdict = await channel.authorize_outbound("C0999ZZZZ") + + assert not verdict.allowed + # Coarse to the agent, specific to the log. + assert "C0999ZZZZ" not in verdict.reason + assert "deny pattern" in caplog.text + + async def test_a_group_dm_is_refused(self): + # A `G` is ambiguous: legacy private channel or multi-person DM. + # Refusing only `D` would let a group DM in through the door + # marked "no unsolicited DMs". + channel = _slack(allow_channels=["*"]) + channel._web.conversations_info = AsyncMock( + return_value={"channel": {"is_mpim": True}}, + ) + + verdict = await channel.authorize_outbound("G0123ABCD") + + assert not verdict.allowed + assert "direct message" in verdict.reason + + async def test_a_private_channel_is_allowed(self): + # The other half of the same ambiguity: a real `G` private channel + # must still work. + channel = _slack(allow_channels=["G0123ABCD"]) + channel._web.conversations_info = AsyncMock( + return_value={"channel": {"name": "private-eng", "is_mpim": False}}, + ) + + verdict = await channel.authorize_outbound("G0123ABCD") + + assert verdict.allowed + + async def test_an_unknowable_conversation_kind_fails_closed(self): + channel = _slack(allow_channels=["*"]) + channel._web.conversations_info = AsyncMock(side_effect=RuntimeError("boom")) + + verdict = await channel.authorize_outbound("G0123ABCD") assert not verdict.allowed - assert "C0999ZZZZ" in verdict.reason - assert "deny" in verdict.reason + assert "could not establish" in verdict.reason + + async def test_a_conversation_name_is_not_a_target(self): + # Targets are ids. Accepting a name would make the destination + # depend on a lookup the caller does not control. + channel = _slack(allow_channels=["general"]) + + verdict = await channel.authorize_outbound("general") + + assert not verdict.allowed + assert "not a name" in verdict.reason + + async def test_a_malformed_id_is_refused(self): + channel = _slack(allow_channels=["*"]) + + verdict = await channel.authorize_outbound("Cx") + + assert not verdict.allowed + + async def test_a_refusal_does_not_name_the_pattern_or_channel(self): + # The reason goes back to the agent, which may repeat it into a + # chat. The detail belongs in the log, not the reply. + channel = _slack(allow_channels=["*"], deny_channels=["secret-*"]) + channel._web.conversations_info = AsyncMock( + return_value={"channel": {"name": "secret-payroll"}}, + ) + + verdict = await channel.authorize_outbound("C0123ABCD") + + assert not verdict.allowed + assert "secret-payroll" not in verdict.reason + assert "secret-*" not in verdict.reason async def test_a_direct_message_is_refused(self): # An inbound DM comes from someone who chose to write. An outbound @@ -126,16 +194,19 @@ async def test_a_direct_message_is_refused(self): assert not verdict.allowed assert "direct message" in verdict.reason - async def test_an_unresolvable_name_is_refused_when_a_deny_list_needs_one(self): + async def test_an_unresolvable_name_is_refused_when_a_deny_list_needs_one( + self, caplog, + ): # conversations.info failing leaves the identity incomplete, and a # deny list cannot be checked against a name nobody could read. channel = _slack(allow_channels=["*"], deny_channels=["secrets"]) channel._web.conversations_info = AsyncMock(side_effect=RuntimeError("boom")) - verdict = await channel.authorize_outbound("C0123ABCD") + with caplog.at_level("INFO"): + verdict = await channel.authorize_outbound("C0123ABCD") assert not verdict.allowed - assert "could not be fully identified" in verdict.reason + assert "could not be fully identified" in caplog.text async def test_a_name_grant_matches_the_resolved_conversation(self): channel = _slack(allow_channels=["general"]) @@ -180,7 +251,7 @@ async def test_a_user_id_is_not_a_conversation(self): verdict = await channel.authorize_outbound("U0123ABCD") assert not verdict.allowed - assert "not a Slack channel" in verdict.reason + assert "not a Slack conversation id" in verdict.reason async def test_an_empty_target_is_refused(self): channel = _slack(allow_channels=["*"]) From 4a1d42da13ecf3a691cb6ca10e301ddd02cdacc7 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Thu, 3 Sep 2026 07:07:50 +0000 Subject: [PATCH 3/6] Put addressed delivery behind its own switch, off by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deriving the write grant from `slack.allow_channels` alone looked tidy — no new keys, writes can never be wider than reads — but it made this feature on-by-default for almost everyone. `allow_channels` is the ordinary inbound access grant; it is what `notifications.slack_channel_id` falls back to, and any channel-based deployment already sets it. Merging as it stood would have handed every agent, including cron sessions, unprompted posting into those channels with no opt-in and nothing in the release notes a reader would connect to it. `slack.allow_outbound` (default false) is now the capability switch, and `allow_channels` remains the bound on where it may go. The original property survives: turning the switch on widens nothing, because a target still has to clear the read grant. This also makes the three Slack capabilities symmetric — `slack.enabled` for the channel, `slack.source.enabled` for the inbox feed, `slack.allow_outbound` for addressed delivery — where before the most consequential of the three was the only one with no explicit opt-in. Co-Authored-By: Claude Opus 5 --- config.example.yaml | 8 ++++++++ docs/config.md | 19 +++++++++++------ nerve/channels/slack.py | 7 +++++++ nerve/config.py | 11 ++++++++++ tests/test_channel_outbound.py | 37 +++++++++++++++++++++++++++++++++- 5 files changed, 75 insertions(+), 7 deletions(-) diff --git a/config.example.yaml b/config.example.yaml index e1861dc9..7a659993 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -129,6 +129,14 @@ slack: # allow_channels: ["eng-*", "C0456DEF"] # deny_channels: ["*-random", "*-social"] # + # Let an agent post to a conversation it names (send_channel_message), + # including from a cron run with no chat attached. Off by default and + # separate from the read grant above: allow_channels is set by nearly every + # deployment for inbound access, so deriving writes from it alone would open + # unprompted posting on upgrade. Turning this on never widens where writes + # may go — allow_channels still bounds that, and DMs are always refused. + # allow_outbound: false + # # Every shared-channel thread is its own session; DMs use one conversation. # # How a reply appears while the agent works. "partial" posts a placeholder diff --git a/docs/config.md b/docs/config.md index c24705cc..fa2bacb9 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1282,17 +1282,24 @@ rather than to whichever chat it is answering. That makes it usable from a cron run with no conversation attached — and means the destination, not the sender, is what has to be authorized. -The grant is `slack.allow_channels` read in the write direction: the agent may -post to a conversation an operator already named, and `slack.deny_channels` -still refuses. There are no separate write keys, so writes cannot widen while -reads narrow. +It is **off by default**: `slack.allow_outbound: true` enables the capability, +and `slack.allow_channels` then bounds where it may go. Two keys rather than +one because `allow_channels` is set by nearly every Slack deployment for +inbound access — deriving writes from it alone would have handed every cron +run a megaphone into those channels on upgrade, with no opt-in. The switch +never widens the destination set: with it on, `allow_channels` still decides, +and `slack.deny_channels` still refuses. + +| Key | Type | Default | Description | +|-----|------|---------|-------------| +| `slack.allow_outbound` | bool | `false` | Let an agent post to a conversation it names | Three differences from the inbound policy are deliberate: - **`slack.allow_users` grants nothing here.** It says who may drive the agent, not where the agent may broadcast. With no `slack.allow_channels` - set, every target is refused — unlike an inbound check, there is no sender - to have vetted first. + set, every target is refused even when `allow_outbound` is on — unlike an + inbound check, there is no sender to have vetted first. - **Unsolicited DMs are refused**, even with `slack.allow_direct_messages`. An inbound DM comes from someone who chose to write; an outbound one does not. Gating one properly means resolving the conversation's member and diff --git a/nerve/channels/slack.py b/nerve/channels/slack.py index 95f027e0..d6c8b601 100644 --- a/nerve/channels/slack.py +++ b/nerve/channels/slack.py @@ -1251,6 +1251,13 @@ async def authorize_outbound(self, target: str) -> Decision: — the same reasoning behind :meth:`SlackAccessPolicy.describe`. The detail goes to the log instead. """ + if not self.config.slack.allow_outbound: + return Decision( + False, + "slack.allow_outbound is not enabled, so the agent may not " + "post to a conversation it names", + ) + channel_id, _ = parse_target(target) if not channel_id: return Decision(False, "no Slack conversation id in the target") diff --git a/nerve/config.py b/nerve/config.py index 216108fd..8c787a45 100644 --- a/nerve/config.py +++ b/nerve/config.py @@ -1079,6 +1079,13 @@ class SlackConfig: allow_direct_messages: bool = False allow_channels: list[str] = field(default_factory=list) deny_channels: list[str] = field(default_factory=list) + # Whether an agent may post to a conversation it names, unprompted + # (send_channel_message). Off by default and separate from the read + # grant: allow_channels is set by nearly every Slack deployment for + # inbound access, so deriving writes from it alone would hand every + # cron run a megaphone into those channels on upgrade. Turning this on + # never widens where writes may go — allow_channels still bounds that. + allow_outbound: bool = False stream_mode: str = "partial" # None keeps safe defaults; [] disables commands. Host-wide and # cross-channel commands are opt-in. See SLACK_*_COMMANDS. @@ -1114,6 +1121,10 @@ def from_dict(cls, d: dict, locked: bool = False) -> SlackConfig: allow_direct_messages=d.get("allow_direct_messages", False), allow_channels=d.get("allow_channels") or [], deny_channels=d.get("deny_channels") or [], + allow_outbound=_as_bool( + d.get("allow_outbound", False), False, + label="SlackConfig.allow_outbound", + ), stream_mode=stream_mode, commands=_slack_commands(d.get("commands")), ) diff --git a/tests/test_channel_outbound.py b/tests/test_channel_outbound.py index 83fc2d42..587bbc95 100644 --- a/tests/test_channel_outbound.py +++ b/tests/test_channel_outbound.py @@ -58,7 +58,12 @@ async def send(self, message) -> None: def _slack(**slack_kwargs) -> SlackChannel: - """A Slack channel with a stub transport, ready to authorize.""" + """A Slack channel with a stub transport, ready to authorize. + + ``allow_outbound`` defaults on here so each test states only the policy + it is about; the switch itself is covered by TestOutboundSwitch. + """ + slack_kwargs.setdefault("allow_outbound", True) cfg = NerveConfig() cfg.slack = SlackConfig( enabled=True, @@ -262,6 +267,36 @@ async def test_an_empty_target_is_refused(self): assert "no Slack conversation id" in verdict.reason +class TestOutboundSwitch: + async def test_addressed_delivery_is_off_by_default(self): + # allow_channels is set by nearly every Slack deployment for inbound + # access. Deriving writes from it alone would hand every cron run a + # megaphone into those channels the moment this shipped. + channel = _slack(allow_channels=["C0123ABCD"], allow_outbound=False) + + verdict = await channel.authorize_outbound("C0123ABCD") + + assert not verdict.allowed + assert "slack.allow_outbound" in verdict.reason + + async def test_the_switch_does_not_widen_where_writes_may_go(self): + # On, but the conversation is still not granted: the switch enables + # the capability, allow_channels still bounds it. + channel = _slack(allow_channels=["C0123ABCD"], allow_outbound=True) + + verdict = await channel.authorize_outbound("C0999ZZZZ") + + assert not verdict.allowed + + async def test_the_switch_alone_grants_nothing(self): + channel = _slack(allow_outbound=True) + + verdict = await channel.authorize_outbound("C0123ABCD") + + assert not verdict.allowed + assert "slack.allow_channels" in verdict.reason + + class TestDefaultRefusal: async def test_a_channel_without_an_override_refuses(self): verdict = await _PlainChannel().authorize_outbound("anything") From 0d0f9c40cfe3dba917099ed2affce3cffb6ba8a7 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis Date: Thu, 3 Sep 2026 13:49:22 +0200 Subject: [PATCH 4/6] Update config.md --- docs/config.md | 42 +++++++----------------------------------- 1 file changed, 7 insertions(+), 35 deletions(-) diff --git a/docs/config.md b/docs/config.md index fa2bacb9..d2cc6d17 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1275,55 +1275,27 @@ warning. Slack in the list costs nothing while Slack is off. Names and globs are not resolved for the `slack_channel_id` fallback. Without a literal channel ID, delivery is skipped with a warning. -### Addressed delivery +### Outbound Slack message tool -The `send_channel_message` tool posts to a conversation the agent names, -rather than to whichever chat it is answering. That makes it usable from a -cron run with no conversation attached — and means the destination, not the -sender, is what has to be authorized. +The `send_channel_message` tool posts to a given conversation. That makes it usable +from a cron run with no conversation attached. It is **off by default**: `slack.allow_outbound: true` enables the capability, -and `slack.allow_channels` then bounds where it may go. Two keys rather than -one because `allow_channels` is set by nearly every Slack deployment for -inbound access — deriving writes from it alone would have handed every cron -run a megaphone into those channels on upgrade, with no opt-in. The switch -never widens the destination set: with it on, `allow_channels` still decides, -and `slack.deny_channels` still refuses. +and `slack.allow_channels` then bounds where it may go. The target must be a literal conversation id. | Key | Type | Default | Description | |-----|------|---------|-------------| | `slack.allow_outbound` | bool | `false` | Let an agent post to a conversation it names | -Three differences from the inbound policy are deliberate: +Three differences from the inbound policy: -- **`slack.allow_users` grants nothing here.** It says who may drive the - agent, not where the agent may broadcast. With no `slack.allow_channels` - set, every target is refused even when `allow_outbound` is on — unlike an - inbound check, there is no sender to have vetted first. +- **`slack.allow_users` has no effect.** It says who may drive the + agent, not where the agent may broadcast. - **Unsolicited DMs are refused**, even with `slack.allow_direct_messages`. - An inbound DM comes from someone who chose to write; an outbound one does - not. Gating one properly means resolving the conversation's member and - running them through the user rules, which is a separate change. **Group DMs count.** A `G` id is either a legacy private channel or a multi-person DM, and only `conversations.info` can tell them apart, so a `G` target costs one cached lookup; a lookup that cannot answer is refused rather than guessed. -- **`notifications.slack_channel_id` may still be a `D`.** That is an - operator writing one config value, not an agent choosing a destination at - runtime, so the two do not share a policy. - -The target must be a literal conversation id, never a name: a name would make -the destination depend on a lookup the caller does not control. - -**Refusal reasons are coarse on purpose.** The reason travels back to the -agent, which may repeat it into a chat, so the detailed verdict — which names -the resolved conversation and the pattern that matched it — goes to the log -and the agent is told only that the destination is not approved. The same -reasoning is why `SlackAccessPolicy.describe()` reports counts rather than -patterns. - -Other channels refuse addressed delivery outright until they implement the -same seam. ## Quiet Hours From bce9a11f2bd0406eaa6218c6eb30b78ffecb186b Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Thu, 3 Sep 2026 14:05:29 +0200 Subject: [PATCH 5/6] Offer addressed delivery only where it could succeed allow_outbound is off by default and Slack is off by default, so on an ordinary install send_channel_message was advertised, tried, and refused. That costs a turn to learn. Its description also names allow_channels as the remaining condition, which only holds once the switch is on, so the model was told the wrong rule. The tool is now dropped from the session's MCP server and from the system prompt while no channel is both running and outbound-enabled. config_excluded_tools sits beside the backend protocol because the exclusion follows the config, not the runtime, and both backends union it with their own. It reads live config per session, like include_hoa, so a reload adds or removes the tool without a restart. The registry still holds the spec; only the per-session view changes. Co-Authored-By: Claude Opus 5 (1M context) --- config.example.yaml | 1 + docs/config.md | 4 ++ nerve/agent/backends/base.py | 23 +++++++++++- nerve/agent/backends/claude.py | 3 +- nerve/agent/backends/codex/backend.py | 3 +- nerve/agent/tools/claude_sdk_adapter.py | 8 ++-- nerve/config.py | 11 ++++++ tests/test_channel_outbound.py | 50 +++++++++++++++++++++++++ tests/test_engine_backend_selection.py | 20 +++++++++- 9 files changed, 116 insertions(+), 7 deletions(-) diff --git a/config.example.yaml b/config.example.yaml index 7a659993..c10f1797 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -135,6 +135,7 @@ slack: # deployment for inbound access, so deriving writes from it alone would open # unprompted posting on upgrade. Turning this on never widens where writes # may go — allow_channels still bounds that, and DMs are always refused. + # While this is off, the send_channel_message tool is not offered at all. # allow_outbound: false # # Every shared-channel thread is its own session; DMs use one conversation. diff --git a/docs/config.md b/docs/config.md index d2cc6d17..53edc399 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1287,6 +1287,10 @@ and `slack.allow_channels` then bounds where it may go. The target must be a lit |-----|------|---------|-------------| | `slack.allow_outbound` | bool | `false` | Let an agent post to a conversation it names | +While no channel is both enabled and outbound-enabled, the tool is not offered +to the agent at all. The check reads live config per session, so a reload adds +or removes it. + Three differences from the inbound policy: - **`slack.allow_users` has no effect.** It says who may drive the diff --git a/nerve/agent/backends/base.py b/nerve/agent/backends/base.py index 5d0d2112..ccb71c83 100644 --- a/nerve/agent/backends/base.py +++ b/nerve/agent/backends/base.py @@ -183,9 +183,30 @@ def validate_resume_target(self, native_id: str, cwd: str) -> bool: ... def excluded_tools(self) -> set[str]: - """Nerve-registry tool names NOT to expose for this backend.""" + """Nerve-registry tool names NOT to expose for this backend. + + Union the runtime's own exclusions with + :func:`config_excluded_tools`, which every backend shares. + """ ... async def validate_model(self, model: str) -> None: """Raise :class:`BackendError` when *model* cannot be served.""" ... + + +def config_excluded_tools(config: Any) -> set[str]: + """Registry tools the configuration leaves nothing to serve. + + Separate from a backend's own exclusions, which turn on the runtime + rather than the config. Read per session, so a reload takes effect + without a restart. + + A tool that is offered but can only refuse costs a turn to find that + out. ``send_channel_message`` is the case: outbound is off by default, + and with it off no destination is reachable. + """ + excluded: set[str] = set() + if not config.outbound_channels: + excluded.add("send_channel_message") + return excluded diff --git a/nerve/agent/backends/claude.py b/nerve/agent/backends/claude.py index 3880b441..686688b7 100644 --- a/nerve/agent/backends/claude.py +++ b/nerve/agent/backends/claude.py @@ -57,6 +57,7 @@ SessionSpec, TransportDiedError, TurnInput, + config_excluded_tools, ) from nerve.agent.backends.images import validate_image_data, validate_image_file from nerve.agent.cache_policy import cache_ttl_env @@ -410,7 +411,7 @@ def excluded_tools(self) -> set[str]: # ScheduleWakeup is a Claude CLI built-in (captured via the # PostToolUse hook) — the registry equivalent exists for backends # without built-ins and would be a confusing duplicate here. - return {"schedule_wakeup"} + return {"schedule_wakeup"} | config_excluded_tools(self.config) def validate_resume_target(self, native_id: str, cwd: str) -> bool: """Check whether Claude Code still has the conversation .jsonl diff --git a/nerve/agent/backends/codex/backend.py b/nerve/agent/backends/codex/backend.py index a0e50d08..683e90ae 100644 --- a/nerve/agent/backends/codex/backend.py +++ b/nerve/agent/backends/codex/backend.py @@ -39,6 +39,7 @@ SessionSpec, TransportDiedError, TurnInput, + config_excluded_tools, ) from nerve.agent.backends.codex.appserver import ( CodexAppServerClient, @@ -159,7 +160,7 @@ def default_model(self, source: str) -> str: return self.codex.model def excluded_tools(self) -> set[str]: - return set() + return config_excluded_tools(self.config) async def validate_model(self, model: str) -> None: """Reject obvious cross-backend model leakage before spawning Codex. diff --git a/nerve/agent/tools/claude_sdk_adapter.py b/nerve/agent/tools/claude_sdk_adapter.py index 9623a86b..466ed4c4 100644 --- a/nerve/agent/tools/claude_sdk_adapter.py +++ b/nerve/agent/tools/claude_sdk_adapter.py @@ -115,9 +115,11 @@ def build_session_mcp_server( ask_user/react/etc. always reference the correct session — no shared global, no race under concurrent sessions. - ``exclude`` drops tools by name — used by agent backends to hide - tools that duplicate a runtime built-in (e.g. the Claude backend - excludes ``schedule_wakeup``; the CLI's ScheduleWakeup covers it). + ``exclude`` drops tools by name — used by agent backends to hide tools + that duplicate a runtime built-in (e.g. the Claude backend excludes + ``schedule_wakeup``; the CLI's ScheduleWakeup covers it) and ones the + config leaves nothing to serve (see + :func:`~nerve.agent.backends.base.config_excluded_tools`). The returned dict matches the SDK's ``McpSdkServerConfig`` shape; ``alwaysLoad`` is set to ``True`` so the Claude Code CLI skips tool- diff --git a/nerve/config.py b/nerve/config.py index 8c787a45..17d353bb 100644 --- a/nerve/config.py +++ b/nerve/config.py @@ -2795,6 +2795,17 @@ def ollama_routable(self) -> bool: """ return self.ollama.enabled and self.proxy.enabled + @property + def outbound_channels(self) -> list[str]: + """Transports an agent may post to unprompted. + + A channel that is not running has nothing to post through, and one + without its own outbound switch refuses every target. Empty means + ``send_channel_message`` can only refuse, which is what decides + whether it is offered at all. + """ + return ["slack"] if self.slack.enabled and self.slack.allow_outbound else [] + def selectable_claude_models( self, discovered: list[str] | None = None, ) -> list[str]: diff --git a/tests/test_channel_outbound.py b/tests/test_channel_outbound.py index 587bbc95..85f7dd56 100644 --- a/tests/test_channel_outbound.py +++ b/tests/test_channel_outbound.py @@ -16,6 +16,7 @@ import pytest +from nerve.agent.backends.base import config_excluded_tools from nerve.agent.tools.handlers.notifications import send_channel_message_handler from nerve.agent.tools.registry import ToolContext from nerve.channels.access import Decision @@ -297,6 +298,55 @@ async def test_the_switch_alone_grants_nothing(self): assert "slack.allow_channels" in verdict.reason +class TestToolVisibility: + """The tool is offered only where it could succeed. + + Outbound is off by default and Slack is off by default, so on an + ordinary install the tool would otherwise be advertised, tried, and + refused, costing a turn to learn that. Its description also names + allow_channels as the remaining condition, which is only true once the + switch is on. + """ + + @pytest.mark.parametrize( + "enabled,outbound,offered", + [ + (False, False, False), + (True, False, False), + (False, True, False), # nothing running to post through + (True, True, True), + ], + ) + def test_the_gate_needs_a_running_channel_and_the_switch( + self, enabled, outbound, offered, + ): + cfg = NerveConfig() + cfg.slack = SlackConfig(enabled=enabled, allow_outbound=outbound) + + assert bool(cfg.outbound_channels) is offered + excluded = config_excluded_tools(cfg) + assert ("send_channel_message" not in excluded) is offered + + def test_the_prompt_stops_advertising_it_too(self): + # Two places name the tool: the session's MCP server and the + # system-prompt tool list. Hiding one and not the other tells the + # model about a tool it cannot call. + from nerve.agent.prompts import _format_tool_list + + full = _format_tool_list() + filtered = _format_tool_list({"send_channel_message"}) + + assert "mcp__nerve__send_channel_message" in full + assert "mcp__nerve__send_channel_message" not in filtered + + def test_the_registry_still_holds_it(self): + # The gate is per session, not per registry: an install that turns + # outbound on mid-run gets the tool at the next session. + from nerve.agent.tools import build_default_registry + + assert "send_channel_message" in build_default_registry() + + class TestDefaultRefusal: async def test_a_channel_without_an_override_refuses(self): verdict = await _PlainChannel().authorize_outbound("anything") diff --git a/tests/test_engine_backend_selection.py b/tests/test_engine_backend_selection.py index cda37e27..31ef883b 100644 --- a/tests/test_engine_backend_selection.py +++ b/tests/test_engine_backend_selection.py @@ -98,7 +98,25 @@ class TestExcludedTools: def test_claude_excludes_schedule_wakeup(self, tmp_path, db): engine = _engine(tmp_path, db) assert "schedule_wakeup" in engine._backends["claude"].excluded_tools() - assert engine._backends["codex"].excluded_tools() == set() + assert "schedule_wakeup" not in engine._backends["codex"].excluded_tools() + + def test_both_backends_drop_a_tool_the_config_cannot_serve(self, tmp_path, db): + # Outbound is off by default, so send_channel_message could only + # refuse. Backend-specific exclusions still apply alongside it. + engine = _engine(tmp_path, db) + for name in ("claude", "codex"): + excluded = engine._backends[name].excluded_tools() + assert "send_channel_message" in excluded, name + + def test_the_tool_returns_once_a_channel_accepts_outbound(self, tmp_path, db): + engine = _engine(tmp_path, db) + engine.config.slack.enabled = True + engine.config.slack.allow_outbound = True + for name in ("claude", "codex"): + excluded = engine._backends[name].excluded_tools() + assert "send_channel_message" not in excluded, name + # Read per session off the live config, so a reload is enough. + assert "schedule_wakeup" in engine._backends["claude"].excluded_tools() def test_prompt_tool_list_respects_exclusions(self): from nerve.agent.prompts import _format_tool_list From 9c7460bd1ba08f8cc9fb769df448f8e6dcce7000 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Thu, 3 Sep 2026 14:45:49 +0200 Subject: [PATCH 6/6] Settle addressed delivery against Slack, not a fixture The mocked tests answer whatever the fixture was told to say, so a grant written against a channel name was only ever matched against a name the test supplied. TestAddressedDelivery runs the same policy over conversations.info's real answer and then posts, so an approved send is one Slack holds. Eight live tests: an id grant, a name grant, a deny pattern on the real name, a name Slack cannot resolve, a thread reply, and the tool handler through the router in both directions. build_outbound_channel opens no socket, since addressed delivery reads no inbound event and a second connection would take envelopes from whichever test is waiting on one. Seven mocked tests are gone, each subsumed by one of those. What stays is pure branching, injected failures, and paths a live test cannot provoke: a group DM, a transport that dies mid-send, a channel with no override. Co-Authored-By: Claude Opus 5 (1M context) --- tests/slack_live.py | 29 ++++++ tests/test_channel_outbound.py | 77 ++-------------- tests/test_slack_live.py | 156 +++++++++++++++++++++++++++++++++ 3 files changed, 193 insertions(+), 69 deletions(-) diff --git a/tests/slack_live.py b/tests/slack_live.py index 8d838273..151cb6dc 100644 --- a/tests/slack_live.py +++ b/tests/slack_live.py @@ -776,3 +776,32 @@ def build_instrumented_socket( channel._build_socket_client = build_instrumented_socket channel._live_diagnostics = diagnostics return channel, cfg + + +def build_outbound_channel(**slack_kwargs): + """A SlackChannel that can post live, with no Socket Mode connection. + + Addressed delivery never reads an inbound event, so opening a socket + would only take a share of this app's envelopes away from whichever + test is waiting on one. The web client is real, which is the point: + ``authorize_outbound`` resolves conversation names through + ``conversations.info`` and the answer is Slack's, not a fixture's. + + ``allow_outbound`` defaults on so each test states only the policy it is + about; the switch itself is unit-tested. + """ + from nerve.channels.slack import SlackChannel + from nerve.config import NerveConfig, SlackConfig + + slack_kwargs.setdefault("allow_outbound", True) + cfg = NerveConfig() + cfg.slack = SlackConfig( + enabled=True, + bot_token=BOT_TOKEN, + app_token=APP_TOKEN, + **slack_kwargs, + ) + channel = SlackChannel(cfg, RecordingRouter()) + channel._web = make_client(BOT_TOKEN) + channel._state = "running" + return channel diff --git a/tests/test_channel_outbound.py b/tests/test_channel_outbound.py index 85f7dd56..d59faddf 100644 --- a/tests/test_channel_outbound.py +++ b/tests/test_channel_outbound.py @@ -8,6 +8,14 @@ Covers the Slack write policy, the default refusal every other channel inherits, the router guard on ``deliver``/``deliver_addressed``, and the ``send_channel_message`` handler that reports a refusal's reason. + +What is *not* here is anything that turns on Slack's own answer: whether a +grant written against a channel name matches what ``conversations.info`` +calls it, and whether an authorized send arrives. A fixture returning +``{"name": "general"}`` proves only that the test knows what the code reads. +Those live in ``TestAddressedDelivery`` in :mod:`tests.test_slack_live`, +against a real workspace. Kept here: the branches that are pure, the failures +that have to be injected, and the paths a live test cannot provoke. """ from __future__ import annotations @@ -88,30 +96,6 @@ def _slack(**slack_kwargs) -> SlackChannel: class TestSlackAuthorizeOutbound: - async def test_an_allowed_conversation_is_approved(self): - channel = _slack(allow_channels=["C0123ABCD"]) - - verdict = await channel.authorize_outbound("C0123ABCD") - - assert verdict.allowed - - async def test_a_thread_target_is_approved_on_its_conversation(self): - # The thread ts addresses a reply inside a conversation already - # granted; splitting it off would refuse the allowed channel. - channel = _slack(allow_channels=["C0123ABCD"]) - - verdict = await channel.authorize_outbound("C0123ABCD:1700000000.000100") - - assert verdict.allowed - - async def test_a_conversation_off_the_allow_list_is_refused(self): - channel = _slack(allow_channels=["C0123ABCD"]) - - verdict = await channel.authorize_outbound("C0999ZZZZ") - - assert not verdict.allowed - assert "not approved" in verdict.reason - async def test_a_denied_conversation_is_refused(self, caplog): channel = _slack(allow_channels=["*"], deny_channels=["C0999ZZZZ"]) @@ -200,28 +184,6 @@ async def test_a_direct_message_is_refused(self): assert not verdict.allowed assert "direct message" in verdict.reason - async def test_an_unresolvable_name_is_refused_when_a_deny_list_needs_one( - self, caplog, - ): - # conversations.info failing leaves the identity incomplete, and a - # deny list cannot be checked against a name nobody could read. - channel = _slack(allow_channels=["*"], deny_channels=["secrets"]) - channel._web.conversations_info = AsyncMock(side_effect=RuntimeError("boom")) - - with caplog.at_level("INFO"): - verdict = await channel.authorize_outbound("C0123ABCD") - - assert not verdict.allowed - assert "could not be fully identified" in caplog.text - - async def test_a_name_grant_matches_the_resolved_conversation(self): - channel = _slack(allow_channels=["general"]) - - verdict = await channel.authorize_outbound("C0123ABCD") - - assert verdict.allowed - channel._web.conversations_info.assert_awaited_once() - async def test_no_allow_channels_refuses_everything(self): # The empty PatternGate allows all comers, which is right for an # inbound check that already ran the user gate and wrong here: @@ -361,17 +323,6 @@ async def test_a_channel_without_an_override_refuses(self): class TestRouterDeliver: - async def test_an_approved_target_receives_the_message(self): - router = ChannelRouter(MagicMock()) - channel = _slack(allow_channels=["C0123ABCD"]) - router.register(channel) - - verdict = await router.deliver_addressed("slack", "C0123ABCD", "hello") - - assert verdict.allowed - channel._web.chat_postMessage.assert_awaited_once() - assert channel._web.chat_postMessage.await_args.kwargs["channel"] == "C0123ABCD" - async def test_the_caller_target_is_used_verbatim(self): # The whole point of this path: a session's last inbound message # must never redirect a delivery the caller addressed itself. @@ -439,18 +390,6 @@ def _ctx(decision) -> ToolContext: class TestSendChannelMessageHandler: - async def test_a_delivered_message_is_confirmed(self): - ctx = _ctx(Decision(True, "ok")) - - result = await send_channel_message_handler( - ctx, {"channel": "slack", "target": "C0123ABCD", "text": "hi"}, - ) - - assert "sent" in result.content[0]["text"].lower() - ctx.engine.router.deliver_addressed.assert_awaited_once_with( - "slack", "C0123ABCD", "hi", session_id="s1", - ) - async def test_a_refusal_reports_the_reason(self): ctx = _ctx(Decision(False, "channel general (C1) is not on the allow list")) diff --git a/tests/test_slack_live.py b/tests/test_slack_live.py index 082dda02..1458412d 100644 --- a/tests/test_slack_live.py +++ b/tests/test_slack_live.py @@ -18,11 +18,16 @@ import asyncio import time +import uuid from types import SimpleNamespace import pytest import pytest_asyncio +from nerve.agent.tools.handlers.notifications import send_channel_message_handler +from nerve.agent.tools.registry import ToolContext +from nerve.channels.base import OutboundMessage +from nerve.channels.router import ChannelRouter from nerve.channels.slack import ( format_target, is_slack_id, @@ -44,6 +49,7 @@ Posted, RecordingRouter, build_channel, + build_outbound_channel, direct_message_guardrails, make_client, requires_no_email_token, @@ -481,3 +487,153 @@ async def test_users_info_omits_email_without_the_scope_instead_of_failing( assert not response["user"]["profile"].get("email"), ( "the no-email token returned an email; it still has the scope" ) + + +# ---------------------------------------------------------------------- # +# Addressed delivery — the agent names the destination # +# ---------------------------------------------------------------------- # + + +@requires_outbound +class TestAddressedDelivery: + """``send_channel_message``, end to end against the real workspace. + + The unit tests settle the policy branches, which are pure. What they + cannot settle is whether the conversation Slack describes is the one + ``allow_channels`` was written against: a name grant matches + ``conversations.info``'s ``name`` field, and a fixture returning + ``{"name": "general"}`` proves only that the test knows what the code + reads. These run the same policy over Slack's own answer, then post. + """ + + async def test_the_scratch_channel_resolves_to_a_name_we_can_grant_on( + self, bot, + ): + # The premise the two name tests below rest on. Slack returns the + # name without a leading '#', which is what allow_channels matches. + info = await bot.conversations_info(channel=TEST_CHANNEL) + name = info["channel"]["name"] + assert name and not name.startswith("#"), name + + async def test_an_id_grant_posts_to_the_conversation(self, bot, posted): + channel = build_outbound_channel(allow_channels=[TEST_CHANNEL]) + marker = f"nvz-outbound-{uuid.uuid4().hex[:8]}" + + verdict = await channel.authorize_outbound(TEST_CHANNEL) + assert verdict.allowed, verdict.reason + await channel.send(OutboundMessage(target=TEST_CHANNEL, text=marker)) + + ts = await _find_posted(bot, marker) + posted.note_bot(TEST_CHANNEL, ts) + assert ts, "the message never reached the conversation" + + async def test_a_name_grant_matches_what_slack_calls_the_conversation( + self, bot, posted, + ): + info = await bot.conversations_info(channel=TEST_CHANNEL) + channel = build_outbound_channel( + allow_channels=[info["channel"]["name"]], + ) + marker = f"nvz-outbound-name-{uuid.uuid4().hex[:8]}" + + verdict = await channel.authorize_outbound(TEST_CHANNEL) + assert verdict.allowed, verdict.reason + await channel.send(OutboundMessage(target=TEST_CHANNEL, text=marker)) + + ts = await _find_posted(bot, marker) + posted.note_bot(TEST_CHANNEL, ts) + assert ts + + async def test_a_deny_pattern_on_the_real_name_refuses_it(self, bot): + info = await bot.conversations_info(channel=TEST_CHANNEL) + channel = build_outbound_channel( + allow_channels=[TEST_CHANNEL], + deny_channels=[info["channel"]["name"]], + ) + + verdict = await channel.authorize_outbound(TEST_CHANNEL) + + assert not verdict.allowed + + async def test_a_name_slack_cannot_resolve_cannot_clear_a_deny_list(self): + # Slack answers channel_not_found, so the identity is short the name + # the deny list is written against. An unread name must not walk past + # the list that might have named it. + channel = build_outbound_channel( + allow_channels=["*"], deny_channels=["secrets"], + ) + + verdict = await channel.authorize_outbound("C00000000000") + + assert not verdict.allowed + + async def test_a_thread_target_posts_inside_the_thread(self, bot, posted): + root = await bot.chat_postMessage( + channel=TEST_CHANNEL, text="nvz-outbound-thread-root", + ) + posted.note_bot(TEST_CHANNEL, root["ts"]) + channel = build_outbound_channel(allow_channels=[TEST_CHANNEL]) + marker = f"nvz-outbound-reply-{uuid.uuid4().hex[:8]}" + target = format_target(TEST_CHANNEL, root["ts"]) + + verdict = await channel.authorize_outbound(target) + assert verdict.allowed, verdict.reason + await channel.send(OutboundMessage(target=target, text=marker)) + + replies = await bot.conversations_replies( + channel=TEST_CHANNEL, ts=root["ts"], + ) + texts = [m["text"] for m in replies["messages"]] + assert marker in texts, texts + + async def test_the_tool_posts_through_the_router(self, bot, posted): + # The whole path the agent actually takes: handler → router → + # authorize_outbound → Slack. + channel = build_outbound_channel(allow_channels=[TEST_CHANNEL]) + router = ChannelRouter(engine=SimpleNamespace(db=None)) + router.register(channel) + ctx = ToolContext( + session_id="live-outbound", + engine=SimpleNamespace(router=router), + ) + marker = f"nvz-outbound-tool-{uuid.uuid4().hex[:8]}" + + result = await send_channel_message_handler(ctx, { + "channel": "slack", "target": TEST_CHANNEL, "text": marker, + }) + + assert not result.is_error, result.content[0]["text"] + ts = await _find_posted(bot, marker) + posted.note_bot(TEST_CHANNEL, ts) + assert ts, "the tool reported success but nothing was posted" + + async def test_the_tool_refuses_a_conversation_off_the_allow_list(self, bot): + channel = build_outbound_channel(allow_channels=["C0NOTTHISONE"]) + router = ChannelRouter(engine=SimpleNamespace(db=None)) + router.register(channel) + ctx = ToolContext( + session_id="live-outbound", + engine=SimpleNamespace(router=router), + ) + marker = f"nvz-outbound-refused-{uuid.uuid4().hex[:8]}" + + result = await send_channel_message_handler(ctx, { + "channel": "slack", "target": TEST_CHANNEL, "text": marker, + }) + + assert "Refused" in result.content[0]["text"] + assert await _find_posted(bot, marker) is None, "a refusal still posted" + + +async def _find_posted(bot, marker: str) -> "str | None": + """The ts of the bot message carrying *marker*, or None. + + Reads recent history rather than a returned ts: ``send`` splits and + posts without handing one back, and the question here is whether Slack + holds the message, not whether the call returned. + """ + history = await bot.conversations_history(channel=TEST_CHANNEL, limit=30) + for message in history["messages"]: + if marker in (message.get("text") or ""): + return message["ts"] + return None