Skip to content

fix(slack): handle leading-space text commands #590

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.

Sign up for GitHub

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

Open
hashbender wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(slack): handle leading-space text commands #590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 39 additions & 109 deletions gateway/platforms/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,12 +546,13 @@ async def _ssrf_redirect_guard(response):

Must be async because httpx.AsyncClient awaits response event hooks.
"""
from tools.url_safety import is_safe_url, redirect_target_from_response
redirect_url = redirect_target_from_response(response)
if redirect_url and not is_safe_url(redirect_url):
raise ValueError(
f"Blocked redirect to private/internal address: {safe_url_for_log(redirect_url)}"
)
if response.is_redirect and response.next_request:
redirect_url = str(response.next_request.url)
from tools.url_safety import is_safe_url
if not is_safe_url(redirect_url):
raise ValueError(
f"Blocked redirect to private/internal address: {safe_url_for_log(redirect_url)}"
)


# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -1159,18 +1160,12 @@ def _media_delivery_denied_paths() -> List[Path]:
# Bitwarden Secrets Manager plaintext disk cache.
os.path.join("cache", "bws_cache.json"),
)
# Directory trees whose every child is credential material.
#
# mcp-tokens/ holds live MCP OAuth access tokens (<server>.json) and
# dynamically-registered client credentials (<server>.client.json); see
# tools/mcp_oauth.py. Same credential class as auth.json/credentials/.
# The write side already denies it (file_tools _check_sensitive_path);
# this pairs the media-delivery (exfil) side so a prompt-injection MEDIA
# tag can't deliver a live bearer token as a native attachment.
# (session/kanban SQLite stores are handled by #41071 — kept out here.)
# Directory trees whose every child is credential material. (MCP OAuth
# tokens under mcp-tokens/ are handled by the sibling targeted PR #37222;
# session/kanban SQLite stores by #41071 — kept out of this diff to avoid
# overlap.)
_ROOT_CREDENTIAL_DIRS = (
"pairing",
"mcp-tokens",
)
for hermes_root in (_HERMES_HOME, _HERMES_ROOT):
for rel in _ROOT_CREDENTIAL_FILES:
Expand Down Expand Up @@ -1781,14 +1776,15 @@ class MessageEvent:

def is_command(self) -> bool:
"""Check if this is a command message (e.g., /new, /reset)."""
return self.text.startswith("/")
return (self.text or "").lstrip().startswith("/")

def get_command(self) -> Optional[str]:
"""Extract command name if this is a command message."""
if not self.is_command():
return None
# Split on space and get first word, strip the /
parts = self.text.split(maxsplit=1)
command_text = (self.text or "").lstrip()
parts = command_text.split(maxsplit=1)
raw = parts[0][1:].lower() if parts else None
if raw and "@" in raw:
raw = raw.split("@", 1)[0]
Expand All @@ -1801,7 +1797,8 @@ def get_command_args(self) -> str:
"""Get the arguments after a command."""
if not self.is_command():
return self.text
parts = self.text.split(maxsplit=1)
command_text = (self.text or "").lstrip()
parts = command_text.split(maxsplit=1)
args = parts[1] if len(parts) > 1 else ""
# iOS auto-corrects -- to — (em dash) and - to – (en dash)
args = args.replace("\u2014\u2014", "--").replace("\u2014", "--").replace("\u2013", "-")
Expand Down Expand Up @@ -1914,42 +1911,6 @@ class SendResult:
}
)

# ``not_found`` substrings split by blast radius. A *chat-level* not_found means
# the chat/user/group itself is gone, so the whole target is dead. A
# *thread/topic/message-level* not_found (a deleted forum topic, an edited-away
# message) leaves the parent chat reachable — it must NOT mark the whole chat
# dead. ``classify_send_error`` collapses both into ``"not_found"``;
# ``is_chat_level_not_found`` recovers the distinction for the dead-target path.
# See gateway.dead_targets.
_CHAT_LEVEL_NOT_FOUND_SUBSTRINGS = ("chat not found",)
_SUBCHAT_NOT_FOUND_SUBSTRINGS = (
"message to edit not found",
"message to reply not found",
"thread not found",
"topic_deleted",
"message_id_invalid",
)


def _error_blob(exc: Optional[BaseException] = None, error_text: str = "") -> str:
"""Build the lowercased text blob both send-error classifiers match against.

Single source of truth so ``classify_send_error`` and
``is_chat_level_not_found`` can never drift (e.g. one including the
exception class name and the other not) and silently disagree on the same
failure. Includes ``str(exc)`` (when non-empty) and the exception's class
name, plus any explicit ``error_text``.
"""
parts = []
if error_text:
parts.append(error_text)
if exc is not None:
exc_str = str(exc)
if exc_str:
parts.append(exc_str)
parts.append(exc.__class__.__name__)
return " ".join(parts).lower()


def classify_send_error(exc: Optional[BaseException], error_text: str = "") -> str:
"""Map a send exception / error string to a :data:`SEND_ERROR_KINDS` value.
Expand All @@ -1959,7 +1920,13 @@ def classify_send_error(exc: Optional[BaseException], error_text: str = "") -> s
use. Conservative — anything unrecognized returns ``"unknown"`` so callers
never mistake an unclassified failure for a benign one.
"""
blob = _error_blob(exc, error_text)
parts = []
if error_text:
parts.append(error_text)
if exc is not None:
parts.append(str(exc))
parts.append(exc.__class__.__name__)
blob = " ".join(parts).lower()
if not blob.strip():
return "unknown"
if "message_too_long" in blob or "too long" in blob or "message is too long" in blob:
Expand All @@ -1983,8 +1950,13 @@ def classify_send_error(exc: Optional[BaseException], error_text: str = "") -> s
or "not a member" in blob
):
return "forbidden"
if any(s in blob for s in _CHAT_LEVEL_NOT_FOUND_SUBSTRINGS) or any(
s in blob for s in _SUBCHAT_NOT_FOUND_SUBSTRINGS
if (
"chat not found" in blob
or "message to edit not found" in blob
or "message to reply not found" in blob
or "thread not found" in blob
or "topic_deleted" in blob
or "message_id_invalid" in blob
):
return "not_found"
if (
Expand All @@ -2002,26 +1974,6 @@ def classify_send_error(exc: Optional[BaseException], error_text: str = "") -> s
return "unknown"


def is_chat_level_not_found(exc: Optional[BaseException] = None, error_text: str = "") -> bool:
"""Whether a ``not_found`` failure means the *whole chat* is gone.

:func:`classify_send_error` collapses chat-level and thread/topic/message-level
not_found into the single ``"not_found"`` kind. Only the chat-level case (the
chat/user/group no longer exists) should mark a delivery target dead; a deleted
forum topic or an edited-away message leaves the parent chat reachable. When
both a chat-level and a sub-chat marker are present, the sub-chat reading wins
(conservative: never kill a chat that may still be reachable).

Argument order mirrors :func:`classify_send_error` (``exc`` first) and both
share :func:`_error_blob`, so the two classifiers cannot disagree on the same
failure.
"""
blob = _error_blob(exc, error_text)
if any(s in blob for s in _SUBCHAT_NOT_FOUND_SUBSTRINGS):
return False
return any(s in blob for s in _CHAT_LEVEL_NOT_FOUND_SUBSTRINGS)


class EphemeralReply(str):
"""System-notice reply that auto-deletes after a TTL.

Expand Down Expand Up @@ -2307,21 +2259,6 @@ class BasePlatformAdapter(ABC):
# "typed_command_prefix", "/"); no per-platform branching at call sites.
typed_command_prefix: str = "/"

# Whether this adapter supports the ``in_channel`` continuable-cron surface
# (``platforms.<p>.extra.cron_continuable_surface: in_channel``): a
# continuable cron job delivered FLAT into a channel (no dedicated thread),
# with the user's plain channel reply continuing the job in-context via the
# shared-channel session. Only coherent on a platform that has BOTH a
# flat-reply outbound gate AND a whole-channel inbound session bucket keyed
# ``(platform, chat_id, None)`` — today that is Slack (``reply_in_thread:
# false``). Default False: an unsupported platform fails SAFE, treating
# ``in_channel`` as ``thread`` (a threaded continuation ≈ today's
# behaviour), never a dropped continuation. Read generically by the cron
# scheduler via ``getattr(adapter, "supports_inchannel_continuable",
# False)`` — no per-platform branching at the call site (the key stays a
# generic seam; Slack is merely the first consumer).
supports_inchannel_continuable: bool = False

def __init__(self, config: PlatformConfig, platform: Platform):
self.config = config
self.platform = platform
Expand Down Expand Up @@ -3961,22 +3898,15 @@ def register_post_delivery_callback(
_prev = existing_cb
_new = callback

async def _chained() -> None:
# Both _prev and _new may be sync or async. The chained
# wrapper itself must be async because the outer invoker
# (``_handle_message`` etc.) awaits awaitable callbacks; a
# sync wrapper here would call ``_prev()`` / ``_new()`` and
# silently drop any returned coroutine, breaking chained
# async post-delivery hooks (e.g. ``/goal`` continuations).
for _cb in (_prev, _new):
try:
_result = _cb()
if inspect.isawaitable(_result):
await _result
except Exception:
logger.debug(
"Post-delivery callback failed", exc_info=True
)
def _chained() -> None:
try:
_prev()
except Exception:
logger.debug("Post-delivery callback failed", exc_info=True)
try:
_new()
except Exception:
logger.debug("Post-delivery callback failed", exc_info=True)

callback = _chained

Expand Down
34 changes: 19 additions & 15 deletions plugins/platforms/slack/adapter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2097,10 +2097,10 @@ async def send_image(

async def _ssrf_redirect_guard(response):
"""Re-check redirect targets so public URLs cannot bounce into private IPs."""
from tools.url_safety import redirect_target_from_response
redirect_url = redirect_target_from_response(response)
if redirect_url and not is_safe_url(redirect_url):
raise ValueError("Blocked redirect to private/internal address")
if response.is_redirect and response.next_request:
redirect_url = str(response.next_request.url)
if not is_safe_url(redirect_url):
raise ValueError("Blocked redirect to private/internal address")

# Download the image first
async with httpx.AsyncClient(
Expand Down Expand Up @@ -2563,11 +2563,12 @@ async def _handle_slack_message(self, event: dict) -> None:
# gateway dispatcher) handles it like a normal slash command. Only
# rewrite when the first token resolves to a known gateway command
# so casual messages like "!nice work" pass through unchanged.
if original_text.startswith("!"):
command_probe_text = original_text.lstrip()
if command_probe_text.startswith("!"):
try:
from hermes_cli.commands import is_gateway_known_command

first_token = original_text[1:].split(maxsplit=1)[0]
first_token = command_probe_text[1:].split(maxsplit=1)[0]
# Strip "@suffix" the same way get_command() does, so
# forms like ``!stop@hermes`` still resolve.
cmd_name = first_token.split("@", 1)[0].lower()
Expand All @@ -2576,10 +2577,12 @@ async def _handle_slack_message(self, event: dict) -> None:
and "/" not in cmd_name
and is_gateway_known_command(cmd_name)
):
original_text = "/" + original_text[1:]
original_text = "/" + command_probe_text[1:]
command_probe_text = original_text
except Exception: # pragma: no cover - defensive
pass

is_command_text = command_probe_text.startswith("/")
text = original_text

# Extract quoted/forwarded content from Slack blocks.
Expand Down Expand Up @@ -2811,7 +2814,7 @@ async def _handle_slack_message(self, event: dict) -> None:

# When entering a thread for the first time (no existing session),
# fetch thread context so the agent understands the conversation.
if is_thread_reply and not self._has_active_session_for_thread(
if is_thread_reply and not is_command_text and not self._has_active_session_for_thread(
channel_id=channel_id,
thread_ts=event_thread_ts,
user_id=user_id,
Expand All @@ -2827,9 +2830,15 @@ async def _handle_slack_message(self, event: dict) -> None:

# Determine message type
msg_type = MessageType.TEXT
if (original_text or "").startswith("/"):
if is_command_text:
msg_type = MessageType.COMMAND

# Commands typed as Slack text messages often intentionally carry a
# leading space (`` /stop``) so Slack itself does not intercept the
# slash. Once classified as a command, pass only the command text into
# the gateway dispatcher; do not prepend fetched thread context or
# block/attachment rendering before the leading slash.

# Handle file attachments
media_urls = []
media_types = []
Expand Down Expand Up @@ -3094,11 +3103,6 @@ async def _handle_slack_message(self, event: dict) -> None:
user_id=user_id,
user_name=user_name,
thread_id=thread_ts,
# Slack Workflow Builder / app posts arrive as
# subtype=bot_message with user=None; flag them so the
# gateway SLACK_ALLOW_BOTS bypass can authorize them
# (they carry no user_id to match against the allowlist).
is_bot=bool(event.get("bot_id")) or event.get("subtype") == "bot_message",
)

# Per-channel ephemeral prompt
Expand Down Expand Up @@ -3138,7 +3142,7 @@ async def _handle_slack_message(self, event: dict) -> None:
reply_to_text = None

msg_event = MessageEvent(
text=text,
text=(command_probe_text if is_command_text else text),
message_type=msg_type,
source=source,
raw_message=event,
Expand Down
10 changes: 10 additions & 0 deletions tests/e2e/test_platform_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ async def test_stop_when_no_agent_running(self, adapter, platform):
response_lower = response_text.lower()
assert "no" in response_lower or "stop" in response_lower or "not running" in response_lower

@pytest.mark.asyncio
async def test_leading_space_stop_is_still_a_command(self, adapter, platform):
"""Slack users type `` /stop`` to avoid native Slack slash interception."""
send = await send_and_capture(adapter, " /stop", platform)

send.assert_called_once()
response_text = send.call_args[1].get("content") or send.call_args[0][1]
response_lower = response_text.lower()
assert "no" in response_lower or "stop" in response_lower or "not running" in response_lower

@pytest.mark.asyncio
async def test_commands_shows_listing(self, adapter, platform):
send = await send_and_capture(adapter, "/commands", platform)
Expand Down
Loading