diff --git a/src/capabilities/slack.md b/src/capabilities/slack.md index 5404002..191cc69 100644 --- a/src/capabilities/slack.md +++ b/src/capabilities/slack.md @@ -32,6 +32,15 @@ The daemon listens for `app_mention` events on Sam's bot and for non-mention rep Sam does not respond to every message in a channel — only direct @-mentions and replies in threads Sam is already part of. People can talk in channels Sam is in without Sam jumping in. +### Multi-channel and Scheduled Top-level Routing + +Sam can operate across multiple Slack channels when invited to them. Direct mentions and replies in threads Sam has already posted in are always processed across any member channel. Even when `SAM_CHANNEL` is set to restrict general incoming routing during testing or development, explicit `@sam` direct mentions remain fully active in all channels where Sam is a member. + +The routing rules are: + +- **Centralized broadcast channel:** Scheduled top-level posts (like the `daily-maintenance` status broadcast) are kept centralized and are always sent to the designated broadcast channel (the channel named `sam`, or configured in the environment). +- **Reactive replies in other channels:** In other channels, Sam never initiates top-level posts or scheduled broadcasts. Sam only responds reactively to direct `@sam` mentions or follow-up replies in threads where Sam participated. + ## Posting to Slack Sam posts via the Slack Web API using `SLACK_BOT_TOKEN`. The daemon doesn't post for Sam; Sam posts for itself, by calling the API as a tool. Same for setting status, opening streams, attaching feedback blocks. diff --git a/src/runtime/daemon.py b/src/runtime/daemon.py index 7153fa0..0c8a366 100644 --- a/src/runtime/daemon.py +++ b/src/runtime/daemon.py @@ -254,6 +254,9 @@ def __init__(self) -> None: self.shutdown_event = asyncio.Event() self.bot_user_id: Optional[str] = None self.sam_channel_name: Optional[str] = None + # Channel name <-> id mappings resolved at startup via users.conversations + self._channel_id_by_name: dict[str, str] = {} + self._channel_name_by_id: dict[str, str] = {} # Channels Sam is a member of (resolved at startup via users.conversations). # When SAM_CHANNEL is unset, this is the scope; when SAM_CHANNEL is set, it # narrows scope further to just that one channel for dev/testing. @@ -291,17 +294,38 @@ def _mark_seen(self, ts: str) -> bool: self._seen_ts.discard(old) return True - def _channel_allowed(self, channel: Optional[str]) -> bool: + @property + def broadcast_channel_id(self) -> Optional[str]: + """Return the target channel ID for scheduled posts and top-level broadcasts. + + If SAM_CHANNEL is explicitly set (dev/testing scope), that's the target. + Otherwise, look up the channel named "sam" in the member-channel map + resolved at startup. Returns None if Sam isn't a member of a channel + named "sam" — caller (cron path) treats None as "disable scheduled + broadcasts" rather than crashing. + + Single source: `SAM_CHANNEL` env, then literal `"sam"` by name. No + third-layer env override — keeps configuration to one place per + `feedback_one_config_file`. + """ + if SAM_CHANNEL: + return SAM_CHANNEL + return self._channel_id_by_name.get("sam") + + def _channel_allowed(self, channel: Optional[str], force_allow_member: bool = False) -> bool: """Channel admission check. Sam responds in any channel it's a member of (set via Slack invites and discovered at startup via `users.conversations`). If `SAM_CHANNEL` is also set, scope narrows to that single channel — useful for dev/testing against one room. + + If force_allow_member is True, we allow any member channel even if + SAM_CHANNEL restricts general routing. """ if not channel: return False - if SAM_CHANNEL: + if SAM_CHANNEL and not force_allow_member: return channel == SAM_CHANNEL return channel in self.member_channels @@ -332,8 +356,12 @@ async def _resolve_member_channels(self) -> set[str]: break for ch in resp.get("channels", []): ch_id = ch.get("id") + ch_name = ch.get("name") if ch_id: channels.add(ch_id) + if ch_name: + self._channel_id_by_name[ch_name] = ch_id + self._channel_name_by_id[ch_id] = ch_name cursor = (resp.get("response_metadata") or {}).get("next_cursor") if not cursor: break @@ -364,7 +392,8 @@ def _is_side_pane_event(self, event: dict) -> bool: via `assistant_thread_started`. """ channel = event.get("channel") - if SAM_CHANNEL and channel == SAM_CHANNEL: + target_channel = self.broadcast_channel_id + if target_channel and channel == target_channel: return False if event.get("assistant_thread"): return True @@ -861,7 +890,7 @@ async def _send_side_pane_redirect(self, channel: str) -> None: def _register_handlers(self) -> None: @self.app.event("app_mention") async def on_app_mention(event, client): - await self._handle_event(event) + await self._handle_event(event, force_allow_member=True) @self.app.event("message") async def on_message(event, client): @@ -881,7 +910,7 @@ async def on_message(event, client): if self._is_side_pane_event(event): await self._send_side_pane_redirect(channel) return - if not self._channel_allowed(channel): + if not self._channel_allowed(channel, force_allow_member=True): return # In a channel: only act on thread replies in threads where the # bot has already posted. Top-level non-mention chatter is @@ -891,7 +920,7 @@ async def on_message(event, client): return if not await self._bot_participates_in_thread(channel, thread_ts): return - await self._handle_event(event) + await self._handle_event(event, force_allow_member=True) @self.app.event("assistant_thread_started") async def on_assistant_thread_started(event, client): @@ -910,7 +939,7 @@ async def on_reaction_added(event, client): async def on_reaction_removed(event, client): log.info("reaction removed: %s on %s", event.get("reaction"), event.get("item")) - async def _handle_event(self, event: dict, *, recovered: bool = False) -> None: + async def _handle_event(self, event: dict, *, recovered: bool = False, force_allow_member: bool = False) -> None: """Route a Slack event into the session queue. `recovered=True` is set by boot-time `_recover_from_reactions` so @@ -926,7 +955,7 @@ async def _handle_event(self, event: dict, *, recovered: bool = False) -> None: return channel = event.get("channel") - if not self._channel_allowed(channel): + if not self._channel_allowed(channel, force_allow_member=force_allow_member): log.info("ignoring event from non-whitelisted channel %s", channel) return @@ -1152,7 +1181,7 @@ async def _recover_from_reactions(self) -> int: events = _select_non_terminal_from_reactions( resp.get("items", []), self.bot_user_id, - self._channel_allowed, + lambda ch: self._channel_allowed(ch, force_allow_member=True), ) for event in events: ts = event.get("ts") or "" @@ -1569,8 +1598,9 @@ async def _run_cron_skill(self, skill_name: str, cron_expr: str) -> None: skill_name, ) return - if not SAM_CHANNEL: - log.info("scheduled skill %s: disabled (SAM_CHANNEL unset)", skill_name) + target_channel = self.broadcast_channel_id + if not target_channel: + log.info("scheduled skill %s: disabled (no broadcast channel found)", skill_name) return try: iterator = croniter(cron_expr, datetime.now().astimezone()) @@ -1594,15 +1624,19 @@ async def _run_cron_skill(self, skill_name: str, cron_expr: str) -> None: await self._enqueue_scheduled_skill(skill_name) async def _enqueue_scheduled_skill(self, skill_name: str) -> None: + target_channel = self.broadcast_channel_id + if not target_channel: + log.error("cannot enqueue scheduled skill %s: no broadcast channel found", skill_name) + return ts = f"{time.time():.6f}" today_journal = f"/data/journal/{datetime.now().date().isoformat()}.md" text = SCHEDULED_SKILL_TEMPLATE.format( skill_name=skill_name, today_journal=today_journal, - channel=SAM_CHANNEL, + channel=target_channel, ) message = IncomingMessage( - channel=SAM_CHANNEL, + channel=target_channel, user=self.bot_user_id or "scheduler", text=text, thread_ts=None, @@ -1610,7 +1644,7 @@ async def _enqueue_scheduled_skill(self, skill_name: str) -> None: scheduled=True, raw_event={}, ) - log.info("scheduled skill %s: queuing (event_ts=%s)", skill_name, ts) + log.info("scheduled skill %s: queuing (event_ts=%s, channel=%s)", skill_name, ts, target_channel) await self.queue.put(message) async def run(self) -> None: @@ -1626,16 +1660,6 @@ async def run(self) -> None: except Exception: log.exception("auth.test failed; will not recognize own messages reliably") - # Resolve the SAM_CHANNEL id to a name so we can refer to it by #name - # in the side-pane redirect copy. - if SAM_CHANNEL: - try: - info = await self.app.client.conversations_info(channel=SAM_CHANNEL) - self.sam_channel_name = (info.get("channel") or {}).get("name") - log.info("Sam channel resolved: #%s (%s)", self.sam_channel_name, SAM_CHANNEL) - except Exception: - log.exception("conversations.info failed; side-pane redirect will use a generic phrase") - # Resolve every channel Sam is a member of. Used by catch-up (when # SAM_CHANNEL is unset) and by `_channel_allowed` for inbound routing. # Adding Sam to a new channel takes effect on next daemon restart; @@ -1645,6 +1669,17 @@ async def run(self) -> None: self.member_channels = await self._resolve_member_channels() log.info("Sam is a member of %d channel(s)", len(self.member_channels)) + # Resolve the channel name so we can refer to it by #name in the side-pane + # redirect copy. Falls back to the broadcast channel if SAM_CHANNEL is unset. + target_channel = self.broadcast_channel_id + if target_channel: + try: + info = await self.app.client.conversations_info(channel=target_channel) + self.sam_channel_name = (info.get("channel") or {}).get("name") + log.info("Sam channel resolved: #%s (%s)", self.sam_channel_name, target_channel) + except Exception: + log.exception("conversations.info failed; side-pane redirect will use a generic phrase") + # Cloud Run startup probe requires an HTTP server on $PORT before the # revision is marked healthy. Sam is otherwise outbound-only (Slack # Socket Mode), so we expose a tiny aiohttp 200-OK endpoint just for diff --git a/tests/runtime/test_channel_routing.py b/tests/runtime/test_channel_routing.py new file mode 100644 index 0000000..dc78f61 --- /dev/null +++ b/tests/runtime/test_channel_routing.py @@ -0,0 +1,242 @@ +"""Tests for the multi-channel routing introduced in PR #79. + +Covers the three pieces of the channel-mapping mechanism: + +1. `_resolve_member_channels` — populates `_channel_id_by_name` and + `_channel_name_by_id` dicts from `users.conversations` responses. +2. `broadcast_channel_id` property — picks the target channel for + scheduled top-level posts: + - `SAM_CHANNEL` env wins when set, + - else look up the literal `"sam"` channel by name in the + resolved member-channel map, + - else None. +3. `_channel_allowed(channel, force_allow_member=...)` — honors the + bypass for explicit `@sam` mentions in any member channel even + when `SAM_CHANNEL` narrows general routing for dev/testing. + +The tests construct a Daemon without `__init__` so we don't open a +Slack socket; they wire `app.client` as an `AsyncMock` and patch +`SAM_CHANNEL` on the daemon module where the property reads it. + +Origin: review feedback on PR #79 ("any tests possible for this +mapping?") — the channel-name resolution is now load-bearing for +scheduled broadcasts and ANY-channel mentions, and the dynamic +fallback is precisely the kind of thing that silently breaks if +nobody pins it. +""" +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock + +import pytest + + +def _mock_daemon(): + """Construct a Daemon without running __init__ (which would open a + Slack socket) and wire in just the attrs the channel-routing helpers + read: bot_user_id, app.client, the two name<->id dicts, and the + member_channels set.""" + from src.runtime.daemon import Daemon + d = Daemon.__new__(Daemon) + d.bot_user_id = "U0BOT" + d.app = MagicMock() + d.app.client = AsyncMock() + d._channel_id_by_name = {} + d._channel_name_by_id = {} + d.member_channels = set() + return d + + +# ─── _resolve_member_channels — name <-> id population ──────────────────────── + + +async def test_resolve_member_channels_populates_both_dicts(): + """A typical users.conversations response with named public/private + channels populates BOTH directions of the mapping.""" + d = _mock_daemon() + d.app.client.users_conversations = AsyncMock(return_value={ + "channels": [ + {"id": "C001", "name": "sam"}, + {"id": "C002", "name": "engineering"}, + {"id": "G003", "name": "private-room"}, + ], + "response_metadata": {"next_cursor": ""}, + }) + channels = await d._resolve_member_channels() + assert channels == {"C001", "C002", "G003"} + assert d._channel_id_by_name == { + "sam": "C001", "engineering": "C002", "private-room": "G003", + } + assert d._channel_name_by_id == { + "C001": "sam", "C002": "engineering", "G003": "private-room", + } + + +async def test_resolve_member_channels_skips_entries_without_name(): + """If Slack returns a channel without a `name` field (defensive — should + not happen in practice but Slack's payload shape isn't strictly typed), + we still include the id in `member_channels` but skip the name mapping. + The defensive `if ch_name:` branch is the line under test.""" + d = _mock_daemon() + d.app.client.users_conversations = AsyncMock(return_value={ + "channels": [ + {"id": "C001", "name": "sam"}, + {"id": "C002"}, # missing name + ], + "response_metadata": {"next_cursor": ""}, + }) + channels = await d._resolve_member_channels() + assert channels == {"C001", "C002"} + assert d._channel_id_by_name == {"sam": "C001"} + assert d._channel_name_by_id == {"C001": "sam"} + + +async def test_resolve_member_channels_handles_pagination(): + """next_cursor should drive a second call; both pages populate the + mappings. Defends the cursor-loop in `_resolve_member_channels`.""" + d = _mock_daemon() + d.app.client.users_conversations = AsyncMock(side_effect=[ + { + "channels": [{"id": "C001", "name": "sam"}], + "response_metadata": {"next_cursor": "page2"}, + }, + { + "channels": [{"id": "C002", "name": "engineering"}], + "response_metadata": {"next_cursor": ""}, + }, + ]) + channels = await d._resolve_member_channels() + assert channels == {"C001", "C002"} + assert d._channel_id_by_name == {"sam": "C001", "engineering": "C002"} + # Pagination must have triggered exactly twice + assert d.app.client.users_conversations.call_count == 2 + + +async def test_resolve_member_channels_returns_partial_on_api_failure(): + """If the API throws mid-pagination, return what was collected so far + rather than crashing startup. Page 1 succeeds, page 2 raises.""" + d = _mock_daemon() + d.app.client.users_conversations = AsyncMock(side_effect=[ + { + "channels": [{"id": "C001", "name": "sam"}], + "response_metadata": {"next_cursor": "page2"}, + }, + RuntimeError("api boom"), + ]) + channels = await d._resolve_member_channels() + assert channels == {"C001"} + assert d._channel_id_by_name == {"sam": "C001"} + + +# ─── broadcast_channel_id — target picker for scheduled top-level posts ─────── + + +def test_broadcast_channel_id_returns_SAM_CHANNEL_when_set(monkeypatch): + """When `SAM_CHANNEL` is explicitly set (dev/testing mode), it takes + precedence over any name-based lookup. The whole point of the env + var is "narrow scope to this one channel during dev".""" + from src.runtime import daemon as daemon_mod + monkeypatch.setattr(daemon_mod, "SAM_CHANNEL", "C_FROM_ENV") + d = _mock_daemon() + d._channel_id_by_name = {"sam": "C_BROADCAST"} # should be ignored + assert d.broadcast_channel_id == "C_FROM_ENV" + + +def test_broadcast_channel_id_resolves_sam_by_name_when_env_unset(monkeypatch): + """The canonical multi-channel mode: `SAM_CHANNEL` unset, so we look + up a channel named "sam" in the resolved member-channel map. This is + what makes the broadcast routing dynamic and removes the hardcoded + `C0B2VBYU79V` ID from the source.""" + from src.runtime import daemon as daemon_mod + monkeypatch.setattr(daemon_mod, "SAM_CHANNEL", "") + d = _mock_daemon() + d._channel_id_by_name = {"sam": "C_BROADCAST", "engineering": "C_ENG"} + assert d.broadcast_channel_id == "C_BROADCAST" + + +def test_broadcast_channel_id_ignores_env_overrides_for_broadcast_name(monkeypatch): + """No env knob can override the literal "sam" name. `feedback_one_config_file` + says one source per value; the broadcast destination is `SAM_CHANNEL`-or-`"sam"`, + not a three-layer chain. Pins this so the `SAM_BROADCAST_CHANNEL` override + can't quietly reappear in a future refactor.""" + from src.runtime import daemon as daemon_mod + monkeypatch.setattr(daemon_mod, "SAM_CHANNEL", "") + # Set the previously-honored env override; it must have no effect. + monkeypatch.setenv("SAM_BROADCAST_CHANNEL", "ops-broadcasts") + d = _mock_daemon() + d._channel_id_by_name = { + "sam": "C_SAM", + "ops-broadcasts": "C_OPS", + } + # Resolves to "sam" by name regardless of the env var. + assert d.broadcast_channel_id == "C_SAM" + + +def test_broadcast_channel_id_returns_None_when_target_missing(monkeypatch): + """Sam isn't a member of any channel named "sam" → no target → None. + The caller (scheduled-skill enqueue) treats None as "disable scheduled + posts" rather than crashing.""" + from src.runtime import daemon as daemon_mod + monkeypatch.setattr(daemon_mod, "SAM_CHANNEL", "") + d = _mock_daemon() + d._channel_id_by_name = {"engineering": "C_ENG"} # no "sam" channel + assert d.broadcast_channel_id is None + + +# ─── _channel_allowed — explicit-mention bypass ─────────────────────────────── + + +def test_channel_allowed_narrows_to_SAM_CHANNEL_by_default(monkeypatch): + """Without `force_allow_member`, SAM_CHANNEL narrows the scope: + only that one channel is admitted. This preserves the dev/testing + "scope to one room" behavior for non-mention traffic.""" + from src.runtime import daemon as daemon_mod + monkeypatch.setattr(daemon_mod, "SAM_CHANNEL", "C_DEV") + d = _mock_daemon() + d.member_channels = {"C_DEV", "C_OTHER"} + assert d._channel_allowed("C_DEV") is True + assert d._channel_allowed("C_OTHER") is False + + +def test_channel_allowed_with_force_allow_member_bypasses_SAM_CHANNEL(monkeypatch): + """The headline feature of #79: `force_allow_member=True` means an + explicit `@sam` mention is admitted in ANY member channel even when + SAM_CHANNEL narrows general routing. This is what unblocks + 'talk to Sam by @-mentioning it in any channel I've invited it to'.""" + from src.runtime import daemon as daemon_mod + monkeypatch.setattr(daemon_mod, "SAM_CHANNEL", "C_DEV") + d = _mock_daemon() + d.member_channels = {"C_DEV", "C_OTHER"} + assert d._channel_allowed("C_OTHER", force_allow_member=True) is True + assert d._channel_allowed("C_DEV", force_allow_member=True) is True + + +def test_channel_allowed_rejects_non_member_even_with_force(monkeypatch): + """The bypass only opens the gate to *member* channels. Sam doesn't + suddenly start responding in channels it was never invited to — the + invite is still the source of truth.""" + from src.runtime import daemon as daemon_mod + monkeypatch.setattr(daemon_mod, "SAM_CHANNEL", "C_DEV") + d = _mock_daemon() + d.member_channels = {"C_DEV"} + assert d._channel_allowed("C_NOT_A_MEMBER", force_allow_member=True) is False + + +def test_channel_allowed_with_SAM_CHANNEL_unset_uses_member_set(monkeypatch): + """Multi-channel mode: `SAM_CHANNEL` unset → admit any channel Sam + is a member of. The bypass arg is moot here because the SAM_CHANNEL + branch is the only one it overrides.""" + from src.runtime import daemon as daemon_mod + monkeypatch.setattr(daemon_mod, "SAM_CHANNEL", "") + d = _mock_daemon() + d.member_channels = {"C_DEV", "C_OTHER"} + assert d._channel_allowed("C_DEV") is True + assert d._channel_allowed("C_OTHER") is True + assert d._channel_allowed("C_NOT_A_MEMBER") is False + + +def test_channel_allowed_rejects_None(): + """Defensive: an event without a channel id never gets admitted.""" + d = _mock_daemon() + assert d._channel_allowed(None) is False + assert d._channel_allowed(None, force_allow_member=True) is False