Repository navigation
fix(slack): slash-command turns keep the channel prompt, skill binding and source names (#128720, salvage #128722) #139858
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
064aa53
5be6c85
03aaf3d
c754b70
40fb1fe
65f4ac5
9faa6d9
575e77d
c271b06
811e4b9
dc5dee3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,10 +52,12 @@ | |
| ) | ||
| from gateway.platforms.event import MessageEvent, MessageType, ProcessingOutcome | ||
|
|
||
| try: # sibling module; support both package and flat plugin-dir import | ||
| try: # sibling modules; support both package and flat plugin-dir import | ||
| from .block_kit import render_blocks, sanitize_blocks | ||
| from .adapter_slash_payload import slash_command_text, slash_thread_id | ||
| except ImportError: # pragma: no cover - plugin loaded outside package context | ||
| from block_kit import render_blocks, sanitize_blocks # type: ignore | ||
| from adapter_slash_payload import slash_command_text, slash_thread_id # type: ignore | ||
|
|
||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
@@ -6062,8 +6064,8 @@ async def _handle_slash_command(self, command: dict) -> None: | |
| team_id = command.get("team_id", "") | ||
| if team_id and channel_id: | ||
| self._remember_channel_team(channel_id, team_id) | ||
| text = self._slash_command_text(command) | ||
| thread_id = self._slash_thread_id(command) | ||
| text = slash_command_text(command) | ||
| thread_id = slash_thread_id(command) | ||
| if self._slash_channel_gated(channel_id): | ||
| logger.debug("[Slack] Ignoring slash command in ignored/non-allowed channel: %s", channel_id) | ||
| return | ||
|
|
@@ -6073,13 +6075,34 @@ async def _handle_slash_command(self, command: dict) -> None: | |
| "[Slack] Ignoring slash command from DM because Slack DMs are disabled: channel=%s user=%s", | ||
| channel_id, user_id) | ||
| return | ||
| # A sender the gateway rejects must not cost Slack lookups first (the message path's rule). | ||
| # In a channel the runner only drops them; in a DM it answers per unauthorized_dm_behavior | ||
| # (pairing code or decline), so it still gets the event, without names. | ||
| rejected = self._early_reject_unauthorized(user_id, channel_id, is_dm) | ||
| if rejected and not is_dm: | ||
| return | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new early access check omits the decoded Pass Prompt To Fix With AIThis is a comment left during a code review.
Path: plugins/platforms/slack/adapter.py
Line: 6081-6083
Comment:
**Routed commands get dropped**
The new early access check omits the decoded `thread_id`. If a shared Slack bot has a thread-specific `profile_routes` entry and the caller is approved only in the destination profile's pairing store, this checks the default profile instead. In a channel, the command is then dropped before its correctly routed source is built. Even `/stop` never reaches the runner.
Pass `thread_id` through `_early_reject_unauthorized` to both authorization paths. Add a regression test with a thread route and separate pairing stores.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in dc5dee3: |
||
| source = self.build_source( | ||
| chat_id=channel_id, chat_type="dm" if is_dm else "group", user_id=user_id, | ||
| thread_id=thread_id, scope_id=team_id or None) | ||
| from gateway.platforms.base import resolve_channel_skills | ||
| event = MessageEvent( | ||
| text=text, | ||
| message_type=(MessageType.COMMAND if text.startswith("/") else MessageType.TEXT), | ||
| source=source, raw_message=command) | ||
| source=source, raw_message=command, | ||
| channel_prompt=self._channel_prompt_with_identity(channel_id, team_id), | ||
| # Bound skills load only when a session starts; "/hermes <question>" can start one. | ||
| auto_skill=resolve_channel_skills(self.config.extra, channel_id, None)) | ||
| # A slash turn is a human turn: it re-pins the channel prompt and session context, so it | ||
| # must carry the same names as a message here or the next message flips them. A command | ||
| # that interrupts, unblocks or pauses work starts no turn of its own, and a slow | ||
| # users.info must not hold /stop or /pause back while the worker keeps running. | ||
| from hermes_cli.commands import resolve_command | ||
| cmd = resolve_command(event.get_command() or "") | ||
| controls_running_turn = cmd is not None and ( | ||
| cmd.busy_policy == "interrupt_then_dispatch" or cmd.name in ("approve", "deny", "pause")) | ||
| if not rejected and not controls_running_turn: | ||
| source.chat_name = await self._resolve_channel_name(channel_id, team_id=team_id) | ||
| source.user_name = await self._resolve_user_name(user_id, chat_id=channel_id, team_id=team_id) | ||
| # Stash response_url so the first reply for this channel+user goes ephemeral. COMMAND | ||
| # events only: free-form "/hermes <question>" replies must stay public. | ||
| response_url = command.get("response_url", "") | ||
|
|
@@ -6093,40 +6116,6 @@ async def _handle_slash_command(self, command: dict) -> None: | |
| finally: | ||
| _slash_user_id.reset(_slash_user_id_token) | ||
|
|
||
| @staticmethod | ||
| def _slash_command_text(command: dict) -> str: | ||
| """Gateway message text for a slash payload. Native slashes keep Slack's raw argument | ||
| payload verbatim (internal/trailing spacing). ``/hermes`` (or a missing ``command``) maps | ||
| ``<subcommand> [args]`` via the registry, else free-form text is a regular question.""" | ||
| slash_name = (command.get("command") or "").lstrip("/").strip() | ||
| raw_text = str(command.get("text") or "") | ||
| if slash_name not in {"hermes", ""}: | ||
| return f"/{slash_name}" if not raw_text else f"/{slash_name} {raw_text}" | ||
| legacy_text = raw_text.strip() | ||
| from hermes_cli.commands_platforms import slack_subcommand_map | ||
| subcommand_map = slack_subcommand_map() | ||
| subcommand_map["compact"] = "/compress" | ||
| first_word = legacy_text.split()[0] if legacy_text.split() else "" | ||
| if first_word in subcommand_map: | ||
| rest = legacy_text[len(first_word) :].strip() | ||
| mapped = subcommand_map[first_word] | ||
| return f"{mapped} {rest}".strip() if rest else mapped | ||
| return legacy_text or "/help" | ||
|
|
||
| @staticmethod | ||
| def _slash_thread_id(command: dict) -> Optional[str]: | ||
| """Thread anchor for a slash payload so session-scoped commands (``/model``) | ||
| hit the same thread session. Shape varies by surface: top-level or nested | ||
| ``message``/``container``; ``thread_ts`` preferred over ``message_ts``.""" | ||
| nested = (command.get(k) for k in ("message", "container")) | ||
| candidates = [command] + [n for n in nested if isinstance(n, dict)] | ||
| for ts_key in ("thread_ts", "message_ts"): | ||
| for payload in candidates: | ||
| value = payload.get(ts_key) | ||
| if value: | ||
| return str(value) | ||
| return None | ||
|
|
||
| def _stash_slash_context( | ||
| self, team_id: str, channel_id: str, user_id: str, response_url: str) -> None: | ||
| """Remember a slash ``response_url`` (+ user for the postEphemeral fallback), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| """Slack slash-command payload decoding: the gateway text and the thread anchor of a slash.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import Optional | ||
|
|
||
|
|
||
| def slash_command_text(command: dict) -> str: | ||
| """Gateway message text for a slash payload. Native slashes keep Slack's raw argument | ||
| payload verbatim (internal/trailing spacing). ``/hermes`` (or a missing ``command``) maps | ||
| ``<subcommand> [args]`` via the registry, else free-form text is a regular question.""" | ||
| slash_name = (command.get("command") or "").lstrip("/").strip() | ||
| raw_text = str(command.get("text") or "") | ||
| if slash_name not in {"hermes", ""}: | ||
| return f"/{slash_name}" if not raw_text else f"/{slash_name} {raw_text}" | ||
| legacy_text = raw_text.strip() | ||
| from hermes_cli.commands_platforms import slack_subcommand_map | ||
| subcommand_map = slack_subcommand_map() | ||
| subcommand_map["compact"] = "/compress" | ||
| first_word = legacy_text.split()[0] if legacy_text.split() else "" | ||
| if first_word in subcommand_map: | ||
| rest = legacy_text[len(first_word) :].strip() | ||
| mapped = subcommand_map[first_word] | ||
| return f"{mapped} {rest}".strip() if rest else mapped | ||
| return legacy_text or "/help" | ||
|
|
||
|
|
||
| def slash_thread_id(command: dict) -> Optional[str]: | ||
| """Thread anchor for a slash payload so session-scoped commands (``/model``) | ||
| hit the same thread session. Shape varies by surface: top-level or nested | ||
| ``message``/``container``; ``thread_ts`` preferred over ``message_ts``.""" | ||
| nested = (command.get(k) for k in ("message", "container")) | ||
| candidates = [command] + [n for n in nested if isinstance(n, dict)] | ||
| for ts_key in ("thread_ts", "message_ts"): | ||
| for payload in candidates: | ||
| value = payload.get(ts_key) | ||
| if value: | ||
| return str(value) | ||
| return None |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| """Slack slash-command turns carry the same channel prompt, skill binding and source names as messages. | ||
|
|
||
| A ``/hermes <question>`` turn is a human turn, so the gateway re-pins ``channel_pin`` and the | ||
| session-context key from it. Built without ``channel_prompt``, ``chat_name`` and ``user_name``, | ||
| it ran without the configured channel prompt and flipped both pins, and the next ordinary | ||
| message flipped them back: two agent rebuilds and two prompt-cache misses per slash turn. Without | ||
| ``auto_skill``, a session opened by ``/hermes <question>`` never loaded the channel's bound skill, | ||
| and neither did the turn a ``/goal <text>`` kickoff queues. | ||
| """ | ||
|
|
||
| from unittest.mock import AsyncMock, MagicMock | ||
|
|
||
| import pytest | ||
|
|
||
| import gateway.run as gateway_run | ||
| from gateway.config import GatewayConfig, Platform, PlatformConfig | ||
| from gateway.session import build_session_context | ||
| from gateway.slash_commands_goals import GatewayGoalCommandsMixin | ||
| from plugins.platforms.slack.adapter import SlackAdapter | ||
|
|
||
|
|
||
| def _adapter(channel_id: str) -> SlackAdapter: | ||
| a = SlackAdapter(PlatformConfig( | ||
| enabled=True, token="xoxb-fake", extra={ | ||
| "channel_prompts": {channel_id: "Answer in haiku."}, | ||
| "channel_skill_bindings": [{"id": channel_id, "skill": "triage"}]})) | ||
| a._app = MagicMock() | ||
| a._app.client = AsyncMock() | ||
| a._app.client.users_info = AsyncMock( | ||
| return_value={"user": {"profile": {"display_name": "Alice"}, "real_name": "Alice"}}) | ||
| a._app.client.conversations_info = AsyncMock( | ||
| return_value={"ok": True, "channel": ( | ||
| {"id": channel_id, "is_im": True, "user": "U_ALICE"} if channel_id.startswith("D") | ||
| else {"id": channel_id, "name": "ops"})}) | ||
| a._bot_user_id = "U_BOT" | ||
| a._bot_display_name = "HermesBot" | ||
| a._running = True | ||
| a.handle_message = AsyncMock() | ||
| return a | ||
|
|
||
|
|
||
| def _agent_turn(event): | ||
| """The turn the agent runs for *event*: for ``/goal <text>`` that is the kickoff the gateway | ||
| queues (real ``_enqueue_goal_turn``), not the command itself.""" | ||
| if event.get_command() != "goal": | ||
| return event | ||
| runner, queued = object.__new__(GatewayGoalCommandsMixin), [] | ||
| runner._adapter_and_key_for = lambda _event: (object(), "sk") | ||
| runner._enqueue_fifo = lambda _key, turn, _adapter: queued.append(turn) | ||
| runner._enqueue_goal_turn(event, event.get_command_args(), label="kickoff", kickoff=True) | ||
| return queued[0] | ||
|
|
||
|
|
||
| def _distinct_pins(turns) -> int: | ||
| """Distinct (session-context prompt, channel prompt) pairs the real gateway pins render.""" | ||
| runner = object.__new__(gateway_run.GatewayRunner) | ||
| config = GatewayConfig(platforms={Platform.SLACK: PlatformConfig(enabled=True, token="x")}) | ||
| pins = set() | ||
| for turn in turns: | ||
| channel_prompt, _ = runner._pinned_channel_inputs( | ||
| "k", turn.channel_prompt, turn.source, internal=False) | ||
| context = runner._pinned_session_context_prompt( | ||
| build_session_context(turn.source, config), False, "k") | ||
| pins.add((context, channel_prompt)) | ||
| return len(pins) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize("channel_id, channel_type, chat_name, text", [ | ||
| ("C_OPS", "channel", "ops", "what broke?"), ("D_ALICE", "im", "Alice", "what broke?"), | ||
| ("C_OPS", "channel", "ops", "queue what broke?"), ("C_OPS", "channel", "ops", "goal fix what broke"), | ||
| ], ids=["channel", "dm", "queue-command", "goal-kickoff"]) | ||
| async def test_slash_turn_matches_message_turn_prompt_and_names(channel_id, channel_type, chat_name, text): | ||
| """message -> slash -> message in one thread session renders one pinned prompt pair. | ||
| ``queue-command``: a registered command that starts a turn (``/queue <prompt>``) is a human | ||
| turn like the free-form question. ``goal-kickoff``: the turn ``/goal <text>`` queues carries | ||
| the same inputs too.""" | ||
| adapter = _adapter(channel_id) | ||
| message = {"text": "<@U_BOT> what broke?", "user": "U_ALICE", "channel": channel_id, | ||
| "channel_type": channel_type, "ts": "1700.000100", "thread_ts": "1700.000050"} | ||
| await adapter._handle_slack_message(message, {"team_id": "T1"}) | ||
| await adapter._handle_slash_command( | ||
| {"command": "/hermes", "text": text, "user_id": "U_ALICE", | ||
| "channel_id": channel_id, "team_id": "T1", "thread_ts": "1700.000050"}) | ||
| await adapter._handle_slack_message(dict(message, ts="1700.000200"), {"team_id": "T1"}) | ||
| assert adapter.handle_message.await_count == 3 | ||
| first, slash, last = (c.args[0] for c in adapter.handle_message.await_args_list) | ||
| slash = _agent_turn(slash) | ||
| assert first.channel_prompt and "Answer in haiku." in first.channel_prompt | ||
| assert slash.channel_prompt == first.channel_prompt | ||
| assert slash.auto_skill == first.auto_skill == ["triage"] | ||
| assert (first.source.chat_name, first.source.user_name) == (chat_name, "Alice") | ||
| assert (slash.source.chat_name, slash.source.user_name) == (chat_name, "Alice") | ||
| assert _distinct_pins((first, slash, last)) == 1 | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize("channel_id, authorized, command, reaches_runner", [ | ||
| ("C_OPS", False, "/hermes", 0), ("D_ALICE", False, "/hermes", 1), | ||
| ("C_OPS", True, "/stop", 1), ("C_OPS", True, "/approve", 1), ("C_OPS", True, "/pause", 1), | ||
| ], ids=["rejected-channel", "rejected-dm", "stop", "approve", "pause"]) | ||
| async def test_slash_that_starts_no_turn_costs_no_slack_lookup(channel_id, authorized, command, reaches_runner): | ||
| """The message path rejects an unauthorized sender before any Slack lookup, and the names the | ||
| slash path now resolves must not cost one either. In a DM the runner still gets the event, | ||
| without names: it answers an unauthorized DM per ``unauthorized_dm_behavior`` (pairing code | ||
| or decline), and a slash command there is how an unpaired user gets that answer. | ||
| ``stop`` / ``approve`` / ``pause``: a command that interrupts, unblocks or pauses work is handed over | ||
| before any lookup. A cold ``users.info`` for a second operator must not hold ``/stop`` back | ||
| while the worker keeps running.""" | ||
| adapter = _adapter(channel_id) | ||
| adapter.set_authorization_check(lambda *_args, **_kwargs: authorized) | ||
| await adapter._handle_slash_command( | ||
| {"command": command, "text": "what broke?" if command == "/hermes" else "", | ||
| "user_id": "U_MALLORY", "channel_id": channel_id, "team_id": "T1"}) | ||
| adapter._app.client.users_info.assert_not_awaited() | ||
| adapter._app.client.conversations_info.assert_not_awaited() | ||
| assert adapter.handle_message.await_count == reaches_runner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass
thread_idhere (and thread it into_is_sender_authorized(..., thread_id=...)/ the fallbackbuild_source). Without it a thread-routed profile's paired user is rejected and even/stopis dropped in a channel. The message path's call has the same gap; fix both.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in dc5dee3 (both call sites, regression through
_handle_slash_commandand_handle_slack_message).