From 6442555f08830f8a85f9c20f4de374fefdee0116 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 25 May 2026 13:11:05 +0200 Subject: [PATCH 1/4] feat(daemon): route scheduled posts to #sam and document multi-channel triggers --- src/capabilities/slack.md | 7 +++++++ src/runtime/daemon.py | 31 ++++++++++++++++--------------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/capabilities/slack.md b/src/capabilities/slack.md index 5404002..02fc03e 100644 --- a/src/capabilities/slack.md +++ b/src/capabilities/slack.md @@ -32,6 +32,13 @@ 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 if `SAM_CHANNEL` is cleared in the daemon's environment and Sam is invited to those channels. The routing rules are: + +- **In `#sam` (C0B2VBYU79V):** Both direct mentions and scheduled top-level posts (like the daily-maintenance broadcast) are sent here. +- **In other channels:** Sam only responds to direct mentions or follow-up replies in threads where Sam was initially mentioned. Scheduled top-level posts are never sent to other channels; they remain centralized in `#sam` (C0B2VBYU79V). + ## 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..433d2f8 100644 --- a/src/runtime/daemon.py +++ b/src/runtime/daemon.py @@ -1569,9 +1569,8 @@ 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) - return + # Scheduled skills fallback to "#sam" (C0B2VBYU79V) if SAM_CHANNEL is unset, + # keeping top-level scheduled posts centralized there. try: iterator = croniter(cron_expr, datetime.now().astimezone()) except (ValueError, KeyError): @@ -1596,13 +1595,14 @@ async def _run_cron_skill(self, skill_name: str, cron_expr: str) -> None: async def _enqueue_scheduled_skill(self, skill_name: str) -> None: ts = f"{time.time():.6f}" today_journal = f"/data/journal/{datetime.now().date().isoformat()}.md" + target_channel = SAM_CHANNEL or "C0B2VBYU79V" 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 +1610,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,15 +1626,16 @@ 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 the channel name so we can refer to it by #name in the side-pane + # redirect copy. Falls back to the default "#sam" channel (C0B2VBYU79V) + # when SAM_CHANNEL is unset. + target_channel = SAM_CHANNEL or "C0B2VBYU79V" + 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") # Resolve every channel Sam is a member of. Used by catch-up (when # SAM_CHANNEL is unset) and by `_channel_allowed` for inbound routing. From aed41ba02629dd8ad404e1322aef6f9672e399ab Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 25 May 2026 13:45:46 +0200 Subject: [PATCH 2/4] feat(daemon): dynamically resolve broadcast channel name and allow multi-channel mentions --- src/capabilities/slack.md | 8 +++-- src/runtime/daemon.py | 75 +++++++++++++++++++++++++++------------ 2 files changed, 57 insertions(+), 26 deletions(-) diff --git a/src/capabilities/slack.md b/src/capabilities/slack.md index 02fc03e..191cc69 100644 --- a/src/capabilities/slack.md +++ b/src/capabilities/slack.md @@ -34,10 +34,12 @@ Sam does not respond to every message in a channel — only direct @-mentions an ### Multi-channel and Scheduled Top-level Routing -Sam can operate across multiple Slack channels if `SAM_CHANNEL` is cleared in the daemon's environment and Sam is invited to those channels. The routing rules are: +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. -- **In `#sam` (C0B2VBYU79V):** Both direct mentions and scheduled top-level posts (like the daily-maintenance broadcast) are sent here. -- **In other channels:** Sam only responds to direct mentions or follow-up replies in threads where Sam was initially mentioned. Scheduled top-level posts are never sent to other channels; they remain centralized in `#sam` (C0B2VBYU79V). +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 diff --git a/src/runtime/daemon.py b/src/runtime/daemon.py index 433d2f8..30d6dd4 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,33 @@ 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, that's the target. + Otherwise, search member channels for a channel named "sam" (or custom broadcast name) + and return its ID. Falls back to None if no suitable channel is found. + """ + if SAM_CHANNEL: + return SAM_CHANNEL + broadcast_name = os.environ.get("SAM_BROADCAST_CHANNEL", "sam") + return self._channel_id_by_name.get(broadcast_name) + + 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 +351,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 +387,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 +885,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 +905,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 +915,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 +934,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 +950,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 +1176,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 +1593,10 @@ async def _run_cron_skill(self, skill_name: str, cron_expr: str) -> None: skill_name, ) return - # Scheduled skills fallback to "#sam" (C0B2VBYU79V) if SAM_CHANNEL is unset, - # keeping top-level scheduled posts centralized there. + 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()) except (ValueError, KeyError): @@ -1593,9 +1619,12 @@ 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" - target_channel = SAM_CHANNEL or "C0B2VBYU79V" text = SCHEDULED_SKILL_TEMPLATE.format( skill_name=skill_name, today_journal=today_journal, @@ -1626,17 +1655,6 @@ async def run(self) -> None: except Exception: log.exception("auth.test failed; will not recognize own messages reliably") - # Resolve the channel name so we can refer to it by #name in the side-pane - # redirect copy. Falls back to the default "#sam" channel (C0B2VBYU79V) - # when SAM_CHANNEL is unset. - target_channel = SAM_CHANNEL or "C0B2VBYU79V" - 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") - # 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; @@ -1646,6 +1664,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 From c0f9c8a55338f72293e4209f2ec6fb7153a8108c Mon Sep 17 00:00:00 2001 From: Sameer Pashikanti Date: Mon, 25 May 2026 17:08:35 +0200 Subject: [PATCH 3/4] test(daemon): cover channel name<->id mapping + force_allow_member bypass Addresses the open review comment on #79 ("any tests possible for this mapping?"). 13 tests across three pieces: - `_resolve_member_channels`: populates both dicts; skips entries without a `name`; paginates correctly; degrades to partial result on mid-pagination API failure. - `broadcast_channel_id` property: SAM_CHANNEL wins when set; resolves "sam" by name otherwise; honors SAM_BROADCAST_CHANNEL override; returns None when no target found. - `_channel_allowed(channel, force_allow_member=...)`: narrows to SAM_CHANNEL without the bypass; admits any member channel WITH the bypass; rejects non-member channels even with the bypass; rejects None. Mocks the Daemon without __init__ (no Slack socket) and patches SAM_CHANNEL on the daemon module since the property reads it at call time. --- tests/runtime/test_channel_routing.py | 242 ++++++++++++++++++++++++++ 1 file changed, 242 insertions(+) create mode 100644 tests/runtime/test_channel_routing.py diff --git a/tests/runtime/test_channel_routing.py b/tests/runtime/test_channel_routing.py new file mode 100644 index 0000000..d1b11fb --- /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 "sam" name (or `SAM_BROADCAST_CHANNEL` override) + 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", "") + monkeypatch.delenv("SAM_BROADCAST_CHANNEL", raising=False) + 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_honors_SAM_BROADCAST_CHANNEL_override(monkeypatch): + """If `SAM_BROADCAST_CHANNEL` is set, use THAT name instead of "sam". + Lets a deployment route broadcasts to a custom channel without code + changes.""" + from src.runtime import daemon as daemon_mod + monkeypatch.setattr(daemon_mod, "SAM_CHANNEL", "") + monkeypatch.setenv("SAM_BROADCAST_CHANNEL", "ops-broadcasts") + d = _mock_daemon() + d._channel_id_by_name = { + "sam": "C_SAM_DEFAULT", + "ops-broadcasts": "C_CUSTOM_BROADCAST", + } + assert d.broadcast_channel_id == "C_CUSTOM_BROADCAST" + + +def test_broadcast_channel_id_returns_None_when_target_missing(monkeypatch): + """Sam isn't a member of any channel named "sam" (or the custom + broadcast name) → 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", "") + monkeypatch.delenv("SAM_BROADCAST_CHANNEL", raising=False) + 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 From 17559a7b7e92087fd143e646606c972505082c13 Mon Sep 17 00:00:00 2001 From: Sameer Pashikanti Date: Mon, 25 May 2026 19:06:28 +0200 Subject: [PATCH 4/4] refactor(daemon): drop SAM_BROADCAST_CHANNEL env override (single source) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The override was added unilaterally in aed41ba (no PR ask for it) and introduces a third config layer for the broadcast destination — at odds with `feedback_one_config_file` ("aim for ≤2 places to edit"). Broadcast target now resolves cleanly: SAM_CHANNEL (dev override) → literal "sam" by name from the member-channel map → None (cron path disables itself). Test renamed to pin the absence of the override (env var has no effect) so a future refactor can't quietly bring it back. --- src/runtime/daemon.py | 15 +++++++++----- tests/runtime/test_channel_routing.py | 30 +++++++++++++-------------- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/runtime/daemon.py b/src/runtime/daemon.py index 30d6dd4..0c8a366 100644 --- a/src/runtime/daemon.py +++ b/src/runtime/daemon.py @@ -298,14 +298,19 @@ def _mark_seen(self, ts: str) -> bool: 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, that's the target. - Otherwise, search member channels for a channel named "sam" (or custom broadcast name) - and return its ID. Falls back to None if no suitable channel is found. + 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 - broadcast_name = os.environ.get("SAM_BROADCAST_CHANNEL", "sam") - return self._channel_id_by_name.get(broadcast_name) + return self._channel_id_by_name.get("sam") def _channel_allowed(self, channel: Optional[str], force_allow_member: bool = False) -> bool: """Channel admission check. diff --git a/tests/runtime/test_channel_routing.py b/tests/runtime/test_channel_routing.py index d1b11fb..dc78f61 100644 --- a/tests/runtime/test_channel_routing.py +++ b/tests/runtime/test_channel_routing.py @@ -7,8 +7,8 @@ 2. `broadcast_channel_id` property — picks the target channel for scheduled top-level posts: - `SAM_CHANNEL` env wins when set, - - else look up the "sam" name (or `SAM_BROADCAST_CHANNEL` override) - in the resolved member-channel map, + - 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 @@ -149,35 +149,35 @@ def test_broadcast_channel_id_resolves_sam_by_name_when_env_unset(monkeypatch): `C0B2VBYU79V` ID from the source.""" from src.runtime import daemon as daemon_mod monkeypatch.setattr(daemon_mod, "SAM_CHANNEL", "") - monkeypatch.delenv("SAM_BROADCAST_CHANNEL", raising=False) 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_honors_SAM_BROADCAST_CHANNEL_override(monkeypatch): - """If `SAM_BROADCAST_CHANNEL` is set, use THAT name instead of "sam". - Lets a deployment route broadcasts to a custom channel without code - changes.""" +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_DEFAULT", - "ops-broadcasts": "C_CUSTOM_BROADCAST", + "sam": "C_SAM", + "ops-broadcasts": "C_OPS", } - assert d.broadcast_channel_id == "C_CUSTOM_BROADCAST" + # 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" (or the custom - broadcast name) → no target → None. The caller (scheduled-skill - enqueue) treats None as "disable scheduled posts" rather than - crashing.""" + """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", "") - monkeypatch.delenv("SAM_BROADCAST_CHANNEL", raising=False) d = _mock_daemon() d._channel_id_by_name = {"engineering": "C_ENG"} # no "sam" channel assert d.broadcast_channel_id is None