feat(daemon): route scheduled posts to #sam and document multi-channel triggers#79
Merged
Merged
Conversation
spashii
reviewed
May 25, 2026
| 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" |
Contributor
Author
There was a problem hiding this comment.
good catch. fixed in the subsequent commit aed41ba by resolving this dynamically via self.broadcast_channel_id (which reads SAM_CHANNEL or looks for a channel named sam from member channels, or fallback via environment variable SAM_BROADCAST_CHANNEL).
|
|
||
| ### 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: |
Member
There was a problem hiding this comment.
Even if sam channel is set we should be able to talk to sam by doing @sam (after an invite ofc)
Contributor
Author
There was a problem hiding this comment.
agreed and addressed in commit aed41ba—added force_allow_member=True to the app mention handler so explicit @sam direct mentions are always active across any member channel, even if SAM_CHANNEL is set during testing.
| 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). |
Member
There was a problem hiding this comment.
General rule dont hard code these
Contributor
Author
There was a problem hiding this comment.
fixed in commit aed41ba—removed the hardcoded ID and fallback names from the documentation, referring generally to the designated broadcast channel.
…lti-channel mentions
spashii
reviewed
May 25, 2026
| 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] = {} |
Member
There was a problem hiding this comment.
any tests possible for this mapping?
Member
There was a problem hiding this comment.
added in c0f9c8a — 13 tests in tests/runtime/test_channel_routing.py covering _resolve_member_channels (populates both dicts, skips nameless entries, paginates, degrades on partial API failure), broadcast_channel_id (SAM_CHANNEL precedence, sam-by-name fallback, SAM_BROADCAST_CHANNEL override, None when target missing), and _channel_allowed (narrows by default, bypasses with force_allow_member, rejects non-members). 183 passing locally.
spashii
requested changes
May 25, 2026
…pass 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.
…rce) 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.
spashii
added a commit
that referenced
this pull request
May 25, 2026
…ide-pane (#85) ## What this changes - @-mentioning Sam in any member channel besides #sam now triggers a real session reply instead of the side-pane redirect — `src/runtime/daemon.py` - `_is_side_pane_event` short-circuits on `channel in self.member_channels` BEFORE checking Slack's stapled `assistant_thread` metadata - 5 new tests in `test_channel_routing.py` pin the classifier across genuine side-panes, member channels with stapled metadata (the bug), known assistant-thread channels, unknown channels, and empty events - `_mock_daemon` helper now initializes `_assistant_thread_channels` so the new tests don't trip on it - **New self-maintenance section: "When something breaks — solve the class, not the instance"** — a four-step protocol Sam now follows when responding to bugs: name the class (not the instance), find where else it shows up, pick a durable guard, ship the patch AND the guard ## The bug (production logs, 2026-05-25) Operator invited Sam to #team-engineering and @-mentioned. Sam replied with the side-pane redirect ("i live in #sam — talk to me there, not here") instead of responding to the mention. ``` 19:20:07 ignoring event from non-whitelisted channel C0884QPQF6W ← on_app_mention silently dropped 19:20:29 redirected side-pane message in C0884QPQF6W ← on_message fired the redirect ``` Slack staples an `assistant_thread` field onto every event for assistant-type apps, including events that arrive on real workspace channels. The previous `_is_side_pane_event` short-circuit only excluded `SAM_CHANNEL` (and after #79's aed41ba, `broadcast_channel_id`) — leaving every other member channel vulnerable to misclassification. PR #79's multi-channel routing landed on main as `a09ee9d` but the side-pane handler still treats every-channel-except-broadcast as side-pane. So the operator-visible behavior of #79 didn't actually work in production. ## The class fix The instance is `_is_side_pane_event` misfiring on one stapled-metadata signal. The class is **"review checked diff-correctness against one channel kind, never traced behavior under all channel kinds."** PR #79 changed the short-circuit from `SAM_CHANNEL` to `broadcast_channel_id` — correct edit, wrong frame: still one named target, not the full member-channels set. So this PR ships two things in one: 1. The targeted fix to `_is_side_pane_event` (member-channel short-circuit) + tests. 2. The durable guard in `src/capabilities/self-maintenance.md` — a four-step protocol Sam runs when something breaks, with the side-pane bug as the worked example. The guard is the part that prevents the next variant of this class from shipping in a week. ## Confidence High. Bug reproduced in production logs, fixed deterministically with a member-channel short-circuit, 5 tests pin the corrected behavior. The self-maintenance addition is docs-only and doesn't affect runtime. All 199 tests pass.
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
2 participants
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.
What is this change?
Updates the daemon to route scheduled/cron posts (like the daily-maintenance status report) specifically to
#sam(C0B2VBYU79V) even in multi-channel environments (whenSAM_CHANNELis unset), and documents these multi-channel and top-level trigger rules insrc/capabilities/slack.md.What did Sam notice that led to this?
Operator requested (on 2026-05-25): "Next, I like top level messages still being able to be sent only in <#C0B2VBYU79V>. And only respond in other channels after a mention or a follow up message after an initial mention?"
Previously, unsetting
SAM_CHANNELdisabled cron/scheduled tasks completely. Now, they run and post to the default channel#sam(C0B2VBYU79V).Tier?
Tier 3 (daemon.py changes) and Tier 1 (slack.md documentation).
Confidence?
High confidence. The changes are fully localized to daemon.py and slack.md, with no risk of breaking existing thread-based message handling.