From 7e427e4d12286b9b7de6d0c58ce667f568793a4a 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/7] 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 8bf6893ce..3c1377204 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1327,6 +1327,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 0c9ce35a3..77ebd8f6f 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 8c921e0dd..0385b5f4b 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 65f84f34c..44b03a4fc 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 5fbd1f338..18a5ac628 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, @@ -503,12 +504,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( @@ -516,6 +549,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 d66029f37..5ff6dce70 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, @@ -1236,6 +1236,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 cd002e453..9f638cf89 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 000000000..569782071 --- /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 1ae291c02..86a0ca533 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 5fd04476d25e923722f9280f8a5701a019f1822e 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/7] 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 3c1377204..9d477b90f 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1349,10 +1349,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 77ebd8f6f..ba8c524dd 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 5ff6dce70..c74133d39 100644 --- a/nerve/channels/slack.py +++ b/nerve/channels/slack.py @@ -1254,26 +1254,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( @@ -1281,7 +1309,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 569782071..83fc2d425 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 1861d1d3223f43b6a0b7c6435030bda31f0f46a1 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/7] 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 c0d1e3866..30bd0c143 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -146,6 +146,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 9d477b90f..9484ac629 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1334,17 +1334,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 c74133d39..98f6bed87 100644 --- a/nerve/channels/slack.py +++ b/nerve/channels/slack.py @@ -1261,6 +1261,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 ca65119c1..32847ddd4 100644 --- a/nerve/config.py +++ b/nerve/config.py @@ -1196,6 +1196,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. @@ -1231,6 +1238,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 83fc2d425..587bbc959 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 cd9d2a3b2d349fd173953c1afdac741d0f4177bc Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis Date: Thu, 3 Sep 2026 13:49:22 +0200 Subject: [PATCH 4/7] 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 9484ac629..9af2da31c 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1327,55 +1327,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 a56ab2da5e45518783b3e359c37b660a53ffacbf Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Thu, 3 Sep 2026 14:05:29 +0200 Subject: [PATCH 5/7] 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 30bd0c143..e5f7fbde8 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -152,6 +152,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 9af2da31c..dbf6dd6a0 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1339,6 +1339,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 5d0d21127..ccb71c835 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 e17c90142..9048beba1 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 @@ -434,7 +435,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 495955c9e..77af20ee5 100644 --- a/nerve/agent/backends/codex/backend.py +++ b/nerve/agent/backends/codex/backend.py @@ -40,6 +40,7 @@ SessionSpec, TransportDiedError, TurnInput, + config_excluded_tools, ) from nerve.agent.backends.codex.appserver import ( CodexAppServerClient, @@ -237,7 +238,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 9623a86b8..466ed4c4f 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 32847ddd4..82e2b0e82 100644 --- a/nerve/config.py +++ b/nerve/config.py @@ -2954,6 +2954,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 587bbc959..85f7dd56a 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 06b1c3e35..c5009aaec 100644 --- a/tests/test_engine_backend_selection.py +++ b/tests/test_engine_backend_selection.py @@ -110,7 +110,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 2673b7523e5b60fa8f24462e33d65d38f282481d Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Thu, 3 Sep 2026 14:45:49 +0200 Subject: [PATCH 6/7] 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 7bd1e8810..221f4e9e9 100644 --- a/tests/slack_live.py +++ b/tests/slack_live.py @@ -807,3 +807,32 @@ def build_channel( if diagnostics_label: instrument_channel(channel, diagnostics_label) 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 85f7dd56a..d59faddf8 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 91a941d95..ebbaead07 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_inbound, @@ -488,3 +494,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 From 415fd6b799fc35772ebd5f6df4225b2fefa52c98 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Wed, 16 Sep 2026 15:47:18 +0200 Subject: [PATCH 7/7] Drop an unservable tool from the endpoint Codex reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **A Codex session was still offered the tool.** `config_excluded_tools` is read by two things: the system-prompt builder, and the Claude backend's in-process MCP server. A Codex session takes its nerve tools from the gateway's Streamable HTTP endpoint instead, and `_list_tools` there enumerated the registry whole. So with outbound delivery off, `tools/list` still carried `send_channel_message` while the prompt built from the same config did not name it, and a call spent a turn learning what the config already knew. The channel policy refused every target throughout, so this was a claim and a consistency defect rather than a security one. `build_mcp_server` takes an `excluded` callable now, and `build_manager` passes `config_excluded_tools` over the engine's live config. A callable rather than a set, because the endpoint is built once in the gateway lifespan and a reload has to reach it — the same per-session read the other two consumers get. `_call_tool` refuses an excluded name as well, for the reason the HoA gate is enforced there too: a client holding an older list can still name the tool. The test that covered this asserted only that `excluded_tools()` contains the name, which for Codex nothing but the prompt consumed. The new ones read the surfaces an agent actually sees: `tools/list` over real HTTP against the mounted endpoint, before and after the config flips, plus a refused call that proves the handler never ran. `docs/config.md` said the tool "is not offered to the agent at all". That now says which two places it is absent from, and that the answer does not depend on the backend. Verification: 3781 passed, 29 skipped, no failures. Against the code before this commit the HTTP test fails with `send_channel_message` present in the list the endpoint returned, which is the reported behavior. Co-Authored-By: Claude Opus 5 (1M context) --- docs/config.md | 5 ++-- nerve/mcp_server/http.py | 6 ++++ nerve/mcp_server/server.py | 19 +++++++++++++ tests/test_mcp_http_integration.py | 41 ++++++++++++++++++++++++++++ tests/test_mcp_server.py | 44 ++++++++++++++++++++++++++++++ 5 files changed, 113 insertions(+), 2 deletions(-) diff --git a/docs/config.md b/docs/config.md index dbf6dd6a0..2bf64c99a 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1340,8 +1340,9 @@ 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. +to the agent at all: it is absent from the system prompt and from the tool +list, whichever backend runs the session. The check reads live config per +session, so a reload adds or removes it. Three differences from the inbound policy: diff --git a/nerve/mcp_server/http.py b/nerve/mcp_server/http.py index 16b3204d3..0343dddfd 100644 --- a/nerve/mcp_server/http.py +++ b/nerve/mcp_server/http.py @@ -33,6 +33,7 @@ from mcp.server.streamable_http_manager import StreamableHTTPSessionManager from starlette.types import Receive, Scope, Send +from nerve.agent.backends.base import config_excluded_tools from nerve.agent.tools import ToolContext, ToolRegistry from nerve.gateway.auth import ( MCP_WORKER_CLAIM, @@ -241,6 +242,11 @@ def build_manager( ctx_resolver=ctx_resolver, audit_writer=audit_writer, include_hoa=config.mcp_endpoint.include_hoa, + # Read off the engine's live config, not the snapshot above: a + # Codex session takes its nerve tools from this endpoint, and its + # tool list has to agree with the system prompt the same config + # builds. Satellite clients read one list per connection. + excluded=lambda: config_excluded_tools(engine.config), ) return StreamableHTTPSessionManager( app=server, diff --git a/nerve/mcp_server/server.py b/nerve/mcp_server/server.py index cfcae5ff1..7ef880465 100644 --- a/nerve/mcp_server/server.py +++ b/nerve/mcp_server/server.py @@ -57,6 +57,7 @@ def build_mcp_server( ctx_resolver: CtxResolver, audit_writer: AuditWriter | None = None, include_hoa: bool = False, + excluded: "Callable[[], set[str]] | None" = None, name: str = "nerve", version: str = "1.0.0", ) -> Server: @@ -75,6 +76,13 @@ def build_mcp_server( include_hoa: If ``True``, expose HoA tools (``hoa_*``) to the external endpoint. Off by default since these tools spawn subprocess agents and warrant a separate trust decision. + excluded: Called per request for the names the configuration + leaves nothing to serve (see + :func:`~nerve.agent.backends.base.config_excluded_tools`). A + callable rather than a set, because this endpoint is built + once at startup and a config reload has to reach it. The + Claude backend passes the same names to its in-process MCP + server, so both tool surfaces answer one config. name: Server name advertised to clients. version: Server version advertised to clients. @@ -92,10 +100,14 @@ def _error(message: str) -> CallToolResult: isError=True, ) + def _excluded_now() -> set[str]: + return excluded() if excluded is not None else set() + async def _list_tools( rctx: ServerRequestContext, params: PaginatedRequestParams | None = None, ) -> ListToolsResult: + hidden = _excluded_now() return ListToolsResult( tools=[ Tool( @@ -104,6 +116,7 @@ async def _list_tools( inputSchema=spec.input_schema, ) for spec in registry.list(include_hoa=include_hoa) + if spec.name not in hidden ] ) @@ -124,6 +137,12 @@ async def _call_tool( if not include_hoa and name.startswith("hoa_"): return _error(f"Tool not available: {name!r}") + # Same reason, for a name the config leaves nothing to serve: a + # caller that kept an older tool list would otherwise spend the + # call on a refusal from deeper in. + if name in _excluded_now(): + return _error(f"Tool not available: {name!r}") + # Validate arguments against the tool's declared inputSchema before # any handler sees them. mcp 1.x's call_tool decorator did this by # default; the 2.x callback does not, and this endpoint is reachable diff --git a/tests/test_mcp_http_integration.py b/tests/test_mcp_http_integration.py index feb145ee0..b2915c42c 100644 --- a/tests/test_mcp_http_integration.py +++ b/tests/test_mcp_http_integration.py @@ -225,6 +225,47 @@ def test_mcp_list_tools(app_with_mcp): assert not any(n.startswith("hoa_") for n in names) +def test_mcp_list_tools_drops_what_the_config_cannot_serve(app_with_mcp): + """A Codex session takes its nerve tools from this endpoint. + + ``send_channel_message`` can only refuse while no channel accepts + outbound delivery, and the system prompt built from the same config + does not name it, so the list must not either. Flipping the config on + the running app also shows the check is read per request rather than + frozen when the endpoint was built. + """ + from nerve.config import get_config + + config = get_config() + with TestClient(app_with_mcp) as client: + init_resp = _post_jsonrpc(client, { + "jsonrpc": "2.0", "id": 1, "method": "initialize", + "params": { + "protocolVersion": "2025-06-18", + "capabilities": {}, + "clientInfo": {"name": "test-client", "version": "0.1"}, + }, + }) + sid = init_resp.headers["mcp-session-id"] + _post_jsonrpc(client, { + "jsonrpc": "2.0", "method": "notifications/initialized", + }, session_id=sid) + + def _tool_names() -> set[str]: + resp = _post_jsonrpc(client, { + "jsonrpc": "2.0", "id": 2, "method": "tools/list", + }, session_id=sid) + assert resp.status_code == 200, resp.text + body = _parse_response(resp) + return {t["name"] for t in body["result"]["tools"]} + + assert "send_channel_message" not in _tool_names() + + config.slack.enabled = True + config.slack.allow_outbound = True + assert "send_channel_message" in _tool_names() + + def test_mcp_call_tool_attributes_to_satellite_session(app_with_mcp): """``tools/call`` over real HTTP, asserting satellite attribution. diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index e21fc3034..c3a669d94 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -112,6 +112,35 @@ async def test_list_tools_includes_hoa_when_opted_in(self): names = {t.name for t in result.tools} assert names == {"regular", "hoa_execute"} + async def test_list_tools_hides_what_the_config_cannot_serve(self): + # A satellite session reads its tools from here, so a name the + # config leaves nothing to serve has to be absent from this list + # as well as from the prompt. + registry = ToolRegistry() + registry.register(_make_spec("alpha")) + registry.register(_make_spec("beta")) + server = build_mcp_server( + registry, ctx_resolver=_ctx_resolver(), excluded=lambda: {"beta"}, + ) + + result = await _invoke_list_tools(server) + assert {t.name for t in result.tools} == {"alpha"} + + async def test_the_exclusions_are_read_per_request(self): + # This endpoint is built once at startup, so a config reload can + # only reach it through the call. + registry = ToolRegistry() + registry.register(_make_spec("alpha")) + hidden: set[str] = {"alpha"} + server = build_mcp_server( + registry, ctx_resolver=_ctx_resolver(), excluded=lambda: hidden, + ) + + assert (await _invoke_list_tools(server)).tools == [] + hidden.clear() + result = await _invoke_list_tools(server) + assert {t.name for t in result.tools} == {"alpha"} + # mcp 2.x dispatches through `get_request_handler(method)`, keyed by method # string, and hands the handler the request context plus a params model. The @@ -192,6 +221,21 @@ async def test_hoa_tool_rejected_when_include_hoa_false(self): assert call_result.is_error is True assert "not available" in call_result.content[0].text + async def test_an_excluded_tool_is_refused_by_name(self): + # A client holding an older tool list can still name the tool, and + # the handler behind it has nothing to serve. + seen: list[dict] = [] + registry = ToolRegistry() + registry.register(_make_spec("alpha", seen=seen)) + server = build_mcp_server( + registry, ctx_resolver=_ctx_resolver(), excluded=lambda: {"alpha"}, + ) + + result = await _invoke_call_tool(server, "alpha") + assert result.is_error is True + assert "not available" in result.content[0].text + assert seen == [] + async def test_audit_writer_called_on_success(self): registry = ToolRegistry() registry.register(_make_spec("alpha", response_text="ack"))