Give agents a tool for posting messages to Slack independently - #421
Open
alex-clickhouse wants to merge 6 commits into
Open
Give agents a tool for posting messages to Slack independently#421alex-clickhouse wants to merge 6 commits into
alex-clickhouse wants to merge 6 commits into
Conversation
alex-clickhouse
changed the title
alex-clickhouse
changed the title
alex-clickhouse
marked this pull request as ready for review
September 3, 2026 12:32
alex-clickhouse
force-pushed
the
alex/slack-outbound-delivery
branch
from
September 3, 2026 14:37
1aac33a to
360a3b2
Compare
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
alex-clickhouse
force-pushed
the
alex/slack-outbound-delivery
branch
from
September 4, 2026 07:13
360a3b2 to
b2fa8a2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #406 (
alex/slack-channel-live-tests). The outbound half of"Slack both directions"; #422 is the inbound half, stacked on this one.
What
An agent in any session — including a cron session with no inbound Slack
context — can post to a Slack conversation it names, via a new
send_channel_message(channel, target, text)tool.BaseChannel.authorize_outbound(target) -> Decision, refusing by default.SlackChannel.authorize_outbound, backed bySlackAccessPolicy.check_outbound.router.deliver()gets the check. It keeps its signature and delegates toa new
deliver_addressed()that returns theDecision.send_channel_messagetool + spec, next tosend_file.slack.allow_outbound(default false) enables the capability;slack.allow_channelsbounds where it may go.While no channel has outbound on, the tool is not offered to the agent at all
— dropped from the session's MCP server and from the system prompt, since
every call could only be refused. Both defaults are off, so that is the
ordinary install. The check reads live config per session, so a reload adds or
removes it without a restart.
Why this shape
Write policy belongs to the channel. Not the tool, not the router — only
the channel knows what a target means. The router asks; the channel decides.
Telegram inherits the default refusal and can opt in later without the tool
changing.
router.deliverwas dead code — zero call sites innerve/ortests/,despite a docstring claiming cron uses it. Guarding it now costs nothing and
closes the bypass that would open the moment someone wires cron output
through it, as that docstring invites. This PR is also its first test
coverage.
The security-relevant decisions
Three deliberate asymmetries with the inbound policy, all in
docs/config.md:slack.allow_usersgrants nothing here. It says who may drive theagent, not where the agent may broadcast, and outbound has no sender to
have vetted.
allow_direct_messages— andthat includes group DMs (see below).
_notification_target()still accepts aD. That is an operatorwriting one config value, not an agent naming a destination at runtime.
Targets must be literal conversation ids, never names: a name would make the
destination depend on a lookup the caller does not control.
🤖 Generated with Claude Code