diff --git a/config.example.yaml b/config.example.yaml index e1861dc9..c10f1797 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -129,6 +129,15 @@ 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. + # 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. # # How a reply appears while the agent works. "partial" posts a placeholder diff --git a/docs/config.md b/docs/config.md index 4ec6b385..b8899d2f 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1254,6 +1254,32 @@ 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. +### Outbound Slack message tool + +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. 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 | + +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 + agent, not where the agent may broadcast. +- **Unsolicited DMs are refused**, even with `slack.allow_direct_messages`. + **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. + ## Quiet Hours | Key | Type | Default | Description | diff --git a/nerve/agent/backends/base.py b/nerve/agent/backends/base.py index 5d0d2112..ccb71c83 100644 --- a/nerve/agent/backends/base.py +++ b/nerve/agent/backends/base.py @@ -183,9 +183,30 @@ def validate_resume_target(self, native_id: str, cwd: str) -> bool: ... def excluded_tools(self) -> set[str]: - """Nerve-registry tool names NOT to expose for this backend.""" + """Nerve-registry tool names NOT to expose for this backend. + + Union the runtime's own exclusions with + :func:`config_excluded_tools`, which every backend shares. + """ ... async def validate_model(self, model: str) -> None: """Raise :class:`BackendError` when *model* cannot be served.""" ... + + +def config_excluded_tools(config: Any) -> set[str]: + """Registry tools the configuration leaves nothing to serve. + + Separate from a backend's own exclusions, which turn on the runtime + rather than the config. Read per session, so a reload takes effect + without a restart. + + A tool that is offered but can only refuse costs a turn to find that + out. ``send_channel_message`` is the case: outbound is off by default, + and with it off no destination is reachable. + """ + excluded: set[str] = set() + if not config.outbound_channels: + excluded.add("send_channel_message") + return excluded diff --git a/nerve/agent/backends/claude.py b/nerve/agent/backends/claude.py index 3880b441..686688b7 100644 --- a/nerve/agent/backends/claude.py +++ b/nerve/agent/backends/claude.py @@ -57,6 +57,7 @@ SessionSpec, TransportDiedError, TurnInput, + config_excluded_tools, ) from nerve.agent.backends.images import validate_image_data, validate_image_file from nerve.agent.cache_policy import cache_ttl_env @@ -410,7 +411,7 @@ def excluded_tools(self) -> set[str]: # ScheduleWakeup is a Claude CLI built-in (captured via the # PostToolUse hook) — the registry equivalent exists for backends # without built-ins and would be a confusing duplicate here. - return {"schedule_wakeup"} + return {"schedule_wakeup"} | config_excluded_tools(self.config) def validate_resume_target(self, native_id: str, cwd: str) -> bool: """Check whether Claude Code still has the conversation .jsonl diff --git a/nerve/agent/backends/codex/backend.py b/nerve/agent/backends/codex/backend.py index a0e50d08..683e90ae 100644 --- a/nerve/agent/backends/codex/backend.py +++ b/nerve/agent/backends/codex/backend.py @@ -39,6 +39,7 @@ SessionSpec, TransportDiedError, TurnInput, + config_excluded_tools, ) from nerve.agent.backends.codex.appserver import ( CodexAppServerClient, @@ -159,7 +160,7 @@ def default_model(self, source: str) -> str: return self.codex.model def excluded_tools(self) -> set[str]: - return set() + return config_excluded_tools(self.config) async def validate_model(self, model: str) -> None: """Reject obvious cross-backend model leakage before spawning Codex. diff --git a/nerve/agent/tools/claude_sdk_adapter.py b/nerve/agent/tools/claude_sdk_adapter.py index 9623a86b..466ed4c4 100644 --- a/nerve/agent/tools/claude_sdk_adapter.py +++ b/nerve/agent/tools/claude_sdk_adapter.py @@ -115,9 +115,11 @@ def build_session_mcp_server( ask_user/react/etc. always reference the correct session — no shared global, no race under concurrent sessions. - ``exclude`` drops tools by name — used by agent backends to hide - tools that duplicate a runtime built-in (e.g. the Claude backend - excludes ``schedule_wakeup``; the CLI's ScheduleWakeup covers it). + ``exclude`` drops tools by name — used by agent backends to hide tools + that duplicate a runtime built-in (e.g. the Claude backend excludes + ``schedule_wakeup``; the CLI's ScheduleWakeup covers it) and ones the + config leaves nothing to serve (see + :func:`~nerve.agent.backends.base.config_excluded_tools`). The returned dict matches the SDK's ``McpSdkServerConfig`` shape; ``alwaysLoad`` is set to ``True`` so the Claude Code CLI skips tool- diff --git a/nerve/agent/tools/handlers/notifications.py b/nerve/agent/tools/handlers/notifications.py index 0c9ce35a..ba8c524d 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,58 @@ 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 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.", 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.", is_error=True) + if not target: + return ToolResult.text("Error: target is required.", is_error=True) + if not text.strip(): + return ToolResult.text("Error: text is required.", is_error=True) + + 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) + # 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}.") + return ToolResult.text( + f"Refused: cannot send to {channel} target {target} — {decision.reason}" + ) + + NOTIFY_SPEC = ToolSpec( name="notify", description=( @@ -527,6 +586,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 +611,5 @@ async def send_file_handler(ctx: ToolContext, args: dict) -> ToolResult: REACT_SPEC, SEND_STICKER_SPEC, SEND_FILE_SPEC, + SEND_CHANNEL_MESSAGE_SPEC, ] diff --git a/nerve/agent/tools/schemas.py b/nerve/agent/tools/schemas.py index 8c921e0d..0385b5f4 100644 --- a/nerve/agent/tools/schemas.py +++ b/nerve/agent/tools/schemas.py @@ -832,6 +832,32 @@ "required": ["file_path"], } +SEND_CHANNEL_MESSAGE_SCHEMA = { + "type": "object", + "properties": { + "channel": { + "type": "string", + "description": ( + "Transport to send through, e.g. 'slack'. This is not the " + "conversation — that is 'target'." + ), + }, + "target": { + "type": "string", + "description": ( + "Conversation to post to, in that transport's own addressing. " + "Slack: a conversation id such as 'C0123ABCD', or " + "'C0123ABCD:1700000000.000100' to reply inside a thread." + ), + }, + "text": { + "type": "string", + "description": "Message body, in Markdown.", + }, + }, + "required": ["channel", "target", "text"], +} + # ----- MCP admin tools ----- NERVE_API_SCHEMA = { diff --git a/nerve/channels/base.py b/nerve/channels/base.py index 65f84f34..44b03a4f 100644 --- a/nerve/channels/base.py +++ b/nerve/channels/base.py @@ -13,6 +13,8 @@ from enum import Flag, auto from typing import Any +from nerve.channels.access import Decision + class ChannelCapability(Flag): """Capabilities a channel can declare. @@ -191,6 +193,23 @@ async def send_interaction( For Web, this is a JSON event over WebSocket. """ + # ------------------------------------------------------------------ # + # Optional: addressed delivery # + # ------------------------------------------------------------------ # + + async def authorize_outbound(self, target: str) -> Decision: + """Whether an agent may send an unsolicited message to *target*. + + Addressed delivery is the one path where the destination comes from + the agent rather than from a person who wrote in first, so the write + policy belongs to the channel that knows what a target means. The + router asks; the channel decides. + + Refusing by default matches :meth:`send_file`, which declines rather + than infer a destination. A channel opts in by overriding this. + """ + return Decision(False, f"{self.name} does not accept addressed delivery") + # ------------------------------------------------------------------ # # Optional: file delivery # # Only called if channel declares ChannelCapability.SEND_FILES. # diff --git a/nerve/channels/router.py b/nerve/channels/router.py index f904c754..9587e4cf 100644 --- a/nerve/channels/router.py +++ b/nerve/channels/router.py @@ -17,6 +17,7 @@ from nerve.agent.interactive import get_handler from nerve.agent.streaming import broadcaster +from nerve.channels.access import Decision from nerve.channels.base import ( BaseChannel, ChannelCapability, @@ -498,12 +499,44 @@ async def deliver( ) -> None: """Deliver a complete message to a channel target. - Used by cron jobs and other non-interactive output delivery. + Used by cron jobs and other non-interactive output delivery, which + have no return value to read — see :meth:`deliver_addressed` for the + same send with the refusal reason attached. + """ + await self.deliver_addressed(channel_name, target, message, session_id) + + async def deliver_addressed( + self, + channel_name: str, + target: str, + message: str, + session_id: str | None = None, + ) -> Decision: + """Deliver to a target the caller names, reporting why if refused. + + The target is always supplied by the caller and never inferred from + ``_message_context``, so a cron run cannot spill into whatever chat + last touched the session — the same reasoning as :meth:`send_file`. + + The channel authorizes the destination first, because only it knows + what a target means; see :meth:`BaseChannel.authorize_outbound`. + Returning the :class:`~nerve.channels.access.Decision` lets a caller + say why nothing was sent instead of only that nothing was. + + A transport failure propagates rather than becoming a refusal: "the + policy said no" and "Slack was down" are different answers. """ channel = self._channels.get(channel_name) if not channel: logger.warning("Cannot deliver to unknown channel: %s", channel_name) - return + return Decision(False, f"unknown channel {channel_name!r}") + + verdict = await channel.authorize_outbound(target) + if not verdict.allowed: + logger.info( + "Refused addressed delivery to %s: %s", channel_name, verdict.reason, + ) + return verdict formatted = channel.format_response(message) await channel.send(OutboundMessage( @@ -511,6 +544,7 @@ async def deliver( text=formatted, session_id=session_id or "", )) + return verdict # ------------------------------------------------------------------ # # Streaming adapter lifecycle # diff --git a/nerve/channels/slack.py b/nerve/channels/slack.py index 8684a017..d0dd7ccb 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, @@ -1213,6 +1213,134 @@ 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. + + 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. + """ + 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") + + if not is_slack_id(channel_id): + return Decision( + False, + "target must be a Slack conversation id, not a name", + ) + if channel_id[0] not in "CG": + return Decision( + False, + "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. 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( + channel_id, + "channel", + needs_name_resolution(policy.channels, is_id=is_slack_id), + ) + 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.""" configured = self.config.notifications.slack_channel_id.strip() diff --git a/nerve/channels/slack_access.py b/nerve/channels/slack_access.py index cd002e45..9f638cf8 100644 --- a/nerve/channels/slack_access.py +++ b/nerve/channels/slack_access.py @@ -80,6 +80,24 @@ def check( ) return self.channels.check(channel) + def check_outbound(self, channel: Identity) -> Decision: + """Decide whether the agent may post to a shared conversation unasked. + + Read in the write direction the policy is short one term: there is no + sender to run through :attr:`users`. An allow list of users therefore + grants nothing here — it says who may drive the agent, not where the + agent may broadcast — so an explicit :attr:`channels` grant is + required, the same instinct as refusing shared channels to a lone + ``allow_direct_messages``. + """ + if not self.channels.allow: + return Decision( + False, + "no slack.allow_channels configured, so no conversation is " + "approved for addressed delivery", + ) + return self.channels.check(channel) + def describe(self) -> str: """Summarize the policy without exposing configured patterns.""" return ( diff --git a/nerve/config.py b/nerve/config.py index 1c288d53..0aea2480 100644 --- a/nerve/config.py +++ b/nerve/config.py @@ -1066,6 +1066,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. @@ -1101,6 +1108,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")), ) @@ -2771,6 +2782,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/slack_live.py b/tests/slack_live.py index 8d838273..151cb6dc 100644 --- a/tests/slack_live.py +++ b/tests/slack_live.py @@ -776,3 +776,32 @@ def build_instrumented_socket( channel._build_socket_client = build_instrumented_socket channel._live_diagnostics = diagnostics return channel, cfg + + +def build_outbound_channel(**slack_kwargs): + """A SlackChannel that can post live, with no Socket Mode connection. + + Addressed delivery never reads an inbound event, so opening a socket + would only take a share of this app's envelopes away from whichever + test is waiting on one. The web client is real, which is the point: + ``authorize_outbound`` resolves conversation names through + ``conversations.info`` and the answer is Slack's, not a fixture's. + + ``allow_outbound`` defaults on so each test states only the policy it is + about; the switch itself is unit-tested. + """ + from nerve.channels.slack import SlackChannel + from nerve.config import NerveConfig, SlackConfig + + slack_kwargs.setdefault("allow_outbound", True) + cfg = NerveConfig() + cfg.slack = SlackConfig( + enabled=True, + bot_token=BOT_TOKEN, + app_token=APP_TOKEN, + **slack_kwargs, + ) + channel = SlackChannel(cfg, RecordingRouter()) + channel._web = make_client(BOT_TOKEN) + channel._state = "running" + return channel diff --git a/tests/test_channel_outbound.py b/tests/test_channel_outbound.py new file mode 100644 index 00000000..d59faddf --- /dev/null +++ b/tests/test_channel_outbound.py @@ -0,0 +1,438 @@ +"""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. + +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 + +from unittest.mock import AsyncMock, MagicMock + +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 +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. + + ``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, + 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_a_denied_conversation_is_refused(self, caplog): + channel = _slack(allow_channels=["*"], deny_channels=["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 "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 + # 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_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 conversation id" 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 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 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") + + assert not verdict.allowed + assert "addressed delivery" in verdict.reason + + +# ---------------------------------------------------------------------- # +# Router guard # +# ---------------------------------------------------------------------- # + + +class TestRouterDeliver: + 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_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_engine_backend_selection.py b/tests/test_engine_backend_selection.py index cda37e27..31ef883b 100644 --- a/tests/test_engine_backend_selection.py +++ b/tests/test_engine_backend_selection.py @@ -98,7 +98,25 @@ class TestExcludedTools: def test_claude_excludes_schedule_wakeup(self, tmp_path, db): engine = _engine(tmp_path, db) assert "schedule_wakeup" in engine._backends["claude"].excluded_tools() - assert engine._backends["codex"].excluded_tools() == set() + assert "schedule_wakeup" not in engine._backends["codex"].excluded_tools() + + def test_both_backends_drop_a_tool_the_config_cannot_serve(self, tmp_path, db): + # Outbound is off by default, so send_channel_message could only + # refuse. Backend-specific exclusions still apply alongside it. + engine = _engine(tmp_path, db) + for name in ("claude", "codex"): + excluded = engine._backends[name].excluded_tools() + assert "send_channel_message" in excluded, name + + def test_the_tool_returns_once_a_channel_accepts_outbound(self, tmp_path, db): + engine = _engine(tmp_path, db) + engine.config.slack.enabled = True + engine.config.slack.allow_outbound = True + for name in ("claude", "codex"): + excluded = engine._backends[name].excluded_tools() + assert "send_channel_message" not in excluded, name + # Read per session off the live config, so a reload is enough. + assert "schedule_wakeup" in engine._backends["claude"].excluded_tools() def test_prompt_tool_list_respects_exclusions(self): from nerve.agent.prompts import _format_tool_list diff --git a/tests/test_slack_live.py b/tests/test_slack_live.py index 082dda02..1458412d 100644 --- a/tests/test_slack_live.py +++ b/tests/test_slack_live.py @@ -18,11 +18,16 @@ import asyncio import time +import uuid from types import SimpleNamespace import pytest import pytest_asyncio +from nerve.agent.tools.handlers.notifications import send_channel_message_handler +from nerve.agent.tools.registry import ToolContext +from nerve.channels.base import OutboundMessage +from nerve.channels.router import ChannelRouter from nerve.channels.slack import ( format_target, is_slack_id, @@ -44,6 +49,7 @@ Posted, RecordingRouter, build_channel, + build_outbound_channel, direct_message_guardrails, make_client, requires_no_email_token, @@ -481,3 +487,153 @@ async def test_users_info_omits_email_without_the_scope_instead_of_failing( assert not response["user"]["profile"].get("email"), ( "the no-email token returned an email; it still has the scope" ) + + +# ---------------------------------------------------------------------- # +# Addressed delivery — the agent names the destination # +# ---------------------------------------------------------------------- # + + +@requires_outbound +class TestAddressedDelivery: + """``send_channel_message``, end to end against the real workspace. + + The unit tests settle the policy branches, which are pure. What they + cannot settle is whether the conversation Slack describes is the one + ``allow_channels`` was written against: a name grant matches + ``conversations.info``'s ``name`` field, and a fixture returning + ``{"name": "general"}`` proves only that the test knows what the code + reads. These run the same policy over Slack's own answer, then post. + """ + + async def test_the_scratch_channel_resolves_to_a_name_we_can_grant_on( + self, bot, + ): + # The premise the two name tests below rest on. Slack returns the + # name without a leading '#', which is what allow_channels matches. + info = await bot.conversations_info(channel=TEST_CHANNEL) + name = info["channel"]["name"] + assert name and not name.startswith("#"), name + + async def test_an_id_grant_posts_to_the_conversation(self, bot, posted): + channel = build_outbound_channel(allow_channels=[TEST_CHANNEL]) + marker = f"nvz-outbound-{uuid.uuid4().hex[:8]}" + + verdict = await channel.authorize_outbound(TEST_CHANNEL) + assert verdict.allowed, verdict.reason + await channel.send(OutboundMessage(target=TEST_CHANNEL, text=marker)) + + ts = await _find_posted(bot, marker) + posted.note_bot(TEST_CHANNEL, ts) + assert ts, "the message never reached the conversation" + + async def test_a_name_grant_matches_what_slack_calls_the_conversation( + self, bot, posted, + ): + info = await bot.conversations_info(channel=TEST_CHANNEL) + channel = build_outbound_channel( + allow_channels=[info["channel"]["name"]], + ) + marker = f"nvz-outbound-name-{uuid.uuid4().hex[:8]}" + + verdict = await channel.authorize_outbound(TEST_CHANNEL) + assert verdict.allowed, verdict.reason + await channel.send(OutboundMessage(target=TEST_CHANNEL, text=marker)) + + ts = await _find_posted(bot, marker) + posted.note_bot(TEST_CHANNEL, ts) + assert ts + + async def test_a_deny_pattern_on_the_real_name_refuses_it(self, bot): + info = await bot.conversations_info(channel=TEST_CHANNEL) + channel = build_outbound_channel( + allow_channels=[TEST_CHANNEL], + deny_channels=[info["channel"]["name"]], + ) + + verdict = await channel.authorize_outbound(TEST_CHANNEL) + + assert not verdict.allowed + + async def test_a_name_slack_cannot_resolve_cannot_clear_a_deny_list(self): + # Slack answers channel_not_found, so the identity is short the name + # the deny list is written against. An unread name must not walk past + # the list that might have named it. + channel = build_outbound_channel( + allow_channels=["*"], deny_channels=["secrets"], + ) + + verdict = await channel.authorize_outbound("C00000000000") + + assert not verdict.allowed + + async def test_a_thread_target_posts_inside_the_thread(self, bot, posted): + root = await bot.chat_postMessage( + channel=TEST_CHANNEL, text="nvz-outbound-thread-root", + ) + posted.note_bot(TEST_CHANNEL, root["ts"]) + channel = build_outbound_channel(allow_channels=[TEST_CHANNEL]) + marker = f"nvz-outbound-reply-{uuid.uuid4().hex[:8]}" + target = format_target(TEST_CHANNEL, root["ts"]) + + verdict = await channel.authorize_outbound(target) + assert verdict.allowed, verdict.reason + await channel.send(OutboundMessage(target=target, text=marker)) + + replies = await bot.conversations_replies( + channel=TEST_CHANNEL, ts=root["ts"], + ) + texts = [m["text"] for m in replies["messages"]] + assert marker in texts, texts + + async def test_the_tool_posts_through_the_router(self, bot, posted): + # The whole path the agent actually takes: handler → router → + # authorize_outbound → Slack. + channel = build_outbound_channel(allow_channels=[TEST_CHANNEL]) + router = ChannelRouter(engine=SimpleNamespace(db=None)) + router.register(channel) + ctx = ToolContext( + session_id="live-outbound", + engine=SimpleNamespace(router=router), + ) + marker = f"nvz-outbound-tool-{uuid.uuid4().hex[:8]}" + + result = await send_channel_message_handler(ctx, { + "channel": "slack", "target": TEST_CHANNEL, "text": marker, + }) + + assert not result.is_error, result.content[0]["text"] + ts = await _find_posted(bot, marker) + posted.note_bot(TEST_CHANNEL, ts) + assert ts, "the tool reported success but nothing was posted" + + async def test_the_tool_refuses_a_conversation_off_the_allow_list(self, bot): + channel = build_outbound_channel(allow_channels=["C0NOTTHISONE"]) + router = ChannelRouter(engine=SimpleNamespace(db=None)) + router.register(channel) + ctx = ToolContext( + session_id="live-outbound", + engine=SimpleNamespace(router=router), + ) + marker = f"nvz-outbound-refused-{uuid.uuid4().hex[:8]}" + + result = await send_channel_message_handler(ctx, { + "channel": "slack", "target": TEST_CHANNEL, "text": marker, + }) + + assert "Refused" in result.content[0]["text"] + assert await _find_posted(bot, marker) is None, "a refusal still posted" + + +async def _find_posted(bot, marker: str) -> "str | None": + """The ts of the bot message carrying *marker*, or None. + + Reads recent history rather than a returned ts: ``send`` splits and + posts without handing one back, and the question here is whether Slack + holds the message, not whether the call returned. + """ + history = await bot.conversations_history(channel=TEST_CHANNEL, limit=30) + for message in history["messages"]: + if marker in (message.get("text") or ""): + return message["ts"] + return None diff --git a/tests/test_tool_registry.py b/tests/test_tool_registry.py index 1ae291c0..86a0ca53 100644 --- a/tests/test_tool_registry.py +++ b/tests/test_tool_registry.py @@ -161,6 +161,7 @@ def test_default_registry_contains_expected_tools(self): "list_sources", "poll_source", # notifications "notify", "ask_user", "react", "send_sticker", "send_file", + "send_channel_message", # mcp admin "nerve_api", "mcp_reload", # workflow runs