From b9d3f7b5b7607b63c42d60ec789c794a4b56b76f Mon Sep 17 00:00:00 2001 From: Sameer Pashikanti Date: Mon, 25 May 2026 19:26:56 +0200 Subject: [PATCH 1/2] fix(daemon): _is_side_pane_event must short-circuit on ANY member channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug confirmed in production logs 2026-05-25 on @-mention in #team-engineering: 19:20:07 ignoring event from non-whitelisted channel C0884QPQF6W 19:20:29 redirected side-pane message in C0884QPQF6W Slack staples an `assistant_thread` field onto events for assistant-type apps even when the event arrives on a real workspace channel. The previous short-circuit only excluded SAM_CHANNEL (or `broadcast_channel_id` after the aed41ba refactor) — leaving every OTHER member channel vulnerable: on_app_mention correctly ignored (SAM_CHANNEL narrowed scope), but on_message saw the `assistant_thread` staple, classified the event as side-pane, and posted "i live in #sam — talk to me there, not here" in response to a legitimate @sam in #team-engineering. Fix: short-circuit on `channel in self.member_channels` BEFORE checking the `assistant_thread` field. Real workspace channels are never side- panes regardless of metadata. PR #79's multi-channel routing depends on this — without the fix, every channel besides #sam would still get the redirect. 5 new tests pin the behavior across genuine side-panes, member channels with stapled metadata, known assistant-thread channels, unknown channels, and empty events. --- src/runtime/daemon.py | 29 +++++++---- tests/runtime/test_channel_routing.py | 72 ++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 13 deletions(-) diff --git a/src/runtime/daemon.py b/src/runtime/daemon.py index 7351b76..2327e99 100644 --- a/src/runtime/daemon.py +++ b/src/runtime/daemon.py @@ -381,19 +381,26 @@ def _catchup_target_channels(self) -> list[str]: def _is_side_pane_event(self, event: dict) -> bool: """Detect Agents & AI Apps assistant-thread (side-pane) messages. - Slack delivers side-pane messages to a separate channel id from - the workspace channel. So anything arriving on the whitelisted - SAM_CHANNEL is, by definition, NOT a side-pane event — even when - Slack staples an `assistant_thread` field onto it as workspace- - level metadata (which it does for assistant-type apps). - - After that short-circuit, two signals: an `assistant_thread` - field on the event, or a channel id we've previously seen open - via `assistant_thread_started`. + Slack delivers side-pane messages on a SEPARATE synthetic channel + id (the assistant-thread channel) — never on a real workspace + channel id. So any event arriving on a channel Sam is a member + of is, by definition, NOT a side-pane event — even when Slack + staples an `assistant_thread` field onto it as workspace-level + metadata (which it does on regular-channel events for + assistant-type apps; verified in production logs 2026-05-25 on + an @-mention in #team-engineering). + + After the member-channel short-circuit, two signals identify a + genuine side-pane event: an `assistant_thread` field on the + event, or a channel id we've previously seen open via + `assistant_thread_started`. """ channel = event.get("channel") - target_channel = self.broadcast_channel_id - if target_channel and channel == target_channel: + # Real workspace channels Sam is a member of are not side-panes. + # This MUST come before the `assistant_thread`-field check because + # Slack staples that field onto every event for assistant-type + # apps regardless of channel kind. + if channel and channel in self.member_channels: return False if event.get("assistant_thread"): return True diff --git a/tests/runtime/test_channel_routing.py b/tests/runtime/test_channel_routing.py index dc78f61..186348d 100644 --- a/tests/runtime/test_channel_routing.py +++ b/tests/runtime/test_channel_routing.py @@ -34,8 +34,8 @@ 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.""" + read: bot_user_id, app.client, the two name<->id dicts, the + member_channels set, and the assistant-thread channel set.""" from src.runtime.daemon import Daemon d = Daemon.__new__(Daemon) d.bot_user_id = "U0BOT" @@ -44,6 +44,7 @@ def _mock_daemon(): d._channel_id_by_name = {} d._channel_name_by_id = {} d.member_channels = set() + d._assistant_thread_channels = set() return d @@ -240,3 +241,70 @@ def test_channel_allowed_rejects_None(): d = _mock_daemon() assert d._channel_allowed(None) is False assert d._channel_allowed(None, force_allow_member=True) is False + + +# ─── _is_side_pane_event — member-channel short-circuit ─────────────────────── + + +def test_is_side_pane_event_false_for_member_channel_even_with_assistant_thread_metadata(): + """The headline bug observed 2026-05-25: Slack staples an + `assistant_thread` field onto events for assistant-type apps even + when the event arrived on a real workspace channel. The previous + `_is_side_pane_event` short-circuited only on SAM_CHANNEL (or + `broadcast_channel_id`), leaving every OTHER member channel + vulnerable — an @-mention in #team-engineering got misclassified + as side-pane and Sam posted "i live in #sam — talk to me there, + not here" instead of responding. + + Fix: short-circuit on `channel in self.member_channels`. Real + workspace channels are never side-panes regardless of metadata. + """ + d = _mock_daemon() + d.member_channels = {"C_SAM", "C_TEAM_ENG"} + event = { + "channel": "C_TEAM_ENG", # real workspace channel + "assistant_thread": {"channel_id": "D_SIDEPANE"}, # Slack-stapled metadata + "text": "<@U0BOT> hello", + } + assert d._is_side_pane_event(event) is False + + +def test_is_side_pane_event_true_for_synthetic_channel_with_metadata(): + """Genuine side-pane: channel is the synthetic assistant-thread id + (NOT a member of the workspace channel set), `assistant_thread` + metadata is present. This is what the redirect is for — keep it + firing here.""" + d = _mock_daemon() + d.member_channels = {"C_SAM", "C_TEAM_ENG"} + event = { + "channel": "D_SIDEPANE", + "assistant_thread": {"channel_id": "D_SIDEPANE"}, + } + assert d._is_side_pane_event(event) is True + + +def test_is_side_pane_event_true_for_known_assistant_thread_channel(): + """A channel we've previously seen via `assistant_thread_started` is + a side-pane even without the metadata on this specific event.""" + d = _mock_daemon() + d.member_channels = {"C_SAM"} + d._assistant_thread_channels = {"D_KNOWN_SIDEPANE"} + event = {"channel": "D_KNOWN_SIDEPANE"} + assert d._is_side_pane_event(event) is True + + +def test_is_side_pane_event_false_for_unknown_channel_with_no_metadata(): + """Unknown channel, no metadata, not in member_channels, not in + assistant_thread_channels — not a side-pane (the caller's other + gates will reject it for being non-member anyway).""" + d = _mock_daemon() + d.member_channels = {"C_SAM"} + event = {"channel": "C_UNKNOWN"} + assert d._is_side_pane_event(event) is False + + +def test_is_side_pane_event_false_for_event_with_no_channel(): + """Defensive: events without a channel field can't be side-pane.""" + d = _mock_daemon() + assert d._is_side_pane_event({}) is False + assert d._is_side_pane_event({"assistant_thread": {}}) is False From 79331558ad4b96dfbad396c813cd0b6874c65906 Mon Sep 17 00:00:00 2001 From: Sameer Pashikanti Date: Mon, 25 May 2026 19:34:10 +0200 Subject: [PATCH 2/2] docs(self-maintenance): solve the class, not the instance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New section under "Pre-push self-checks": when something breaks, the work is identifying the class of problem and closing every door the class can come back through — not just patching the visible case. Four-step protocol: name the class (not the instance), find where else the class shows up, pick a durable guard (test shape / review item / process step), ship the patch AND the guard. Anchored in the 2026-05-25 side-pane misclassification: the instance was an @sam in #team-engineering getting redirected; the class was "review checked diff-correctness against one channel kind, never traced behavior under all channel kinds." Naming both is the discipline. Counter-examples explicitly called out: single-bug test, "be careful" prose, deferred ticket. Each looks like a class fix; none is. --- src/capabilities/self-maintenance.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/capabilities/self-maintenance.md b/src/capabilities/self-maintenance.md index 1c54a60..2b6d7e9 100644 --- a/src/capabilities/self-maintenance.md +++ b/src/capabilities/self-maintenance.md @@ -180,6 +180,29 @@ Whenever a change touches *how Sam interacts with an external system* — a new Local "it ran in docker compose" tells you the code is well-formed. It does *not* tell you the deploy will succeed or the integration will work. Different layer of question. +## When something breaks — solve the class, not the instance + +When a bug surfaces, a review misses something, or a deploy regresses, the work is not the specific fix — it is **identifying the class of problem the instance belongs to** and closing every door the class can come back through. The single-instance fix is cheap and feels productive; the class fix is the one that prevents the next variant from shipping in a week with the same shape. + +This is a learned discipline, not a default. The shortest path is always "patch the visible case and move on." Don't take it. + +**The protocol.** When something breaks — before reaching for the patch — answer in order: + +1. **Name the class, not the instance.** The instance is "side-pane misclassification when @sam fires in #team-engineering." The class is "review checked diff-correctness against one channel kind, never traced behavior under all channel kinds." The class fix is harder to spot and worth more. +2. **Where else does the class show up?** Same shape applied to other handler chains, classifier helpers, anywhere a heuristic gets fooled by stapled metadata, anywhere a rule narrows on one named target when it should narrow on the full set. Spell those locations out by name, even if Sam doesn't touch them in this PR. +3. **What guard closes the class permanently?** A new test shape (end-to-end routing tests, not just helper unit tests). A new review checklist item (trace a concrete event through every handler, with each channel kind as input). A process step in an existing skill or capability. Pick one durable mechanism — not "be more careful." +4. **Ship the patch AND the guard.** The patch alone is theater. The next instance of the class surfaces in a week or a month with the same shape and the same root review failure. Land the guard in the same PR or as an immediate follow-up — and *name it* in the PR description so the reviewer can grade Sam's class-naming. + +**Counter-examples — these look like class fixes but aren't:** + +- Adding a single new test for the exact bug. That's a patch with a paper trail. The next variant slips through because the *kind of check* hasn't changed. +- Adding a prose rule that says "be careful about X." Prose Sam already ignored under load is not enforcement. +- Filing a ticket "look into this later." Almost never revisited; the next variant hits before the ticket does. + +**Why it matters.** Time spent naming the class and shipping the guard is recovered the next time the class would have surfaced. Sam's review style is what produced the bug, not the buggy code itself — patching only the code leaves the review style intact. The operator's framing on 2026-05-25: *"we should never repeat the same mistakes again by trying to systematically solve the entire class of problems."* That sentence is the rule. + +Cross-references: `src/skills/github-pr-workflow/skill.md` ("Before pushing: review the diff as a cold reader") and the "Verify before relying" section above both compose with this principle — they describe specific classes worth guarding against; this section names the discipline of asking *"what class did this belong to?"* every time something breaks. + ## What Sam does not do - **Bypass the review gate.** Sam never uses `gh pr merge` without `--auto` (which respects branch protection). No `--admin` flag, no ruleset edits, no bypass actors. The required-review gate exists for a reason — Sam leans on it.