mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-24 16:54:43 +00:00
fix(gateway): make Slack platform note capability-aware when slack tools present
Ports #63234 forward onto current main per teknium1's review. gateway/session.py hard-coded the stale-API disclaimer for every Slack session regardless of whether Slack tools were actually loaded. This contradicted the system prompt when MCP or native slack tools were present, causing the agent to refuse Slack API actions it could actually perform (issue #6536). Per review, the original predicate only checked the native 'slack' toolset, missing Slack MCP servers (registered under mcp-<server> in tools/mcp_tool.py) entirely. _slack_tools_loaded() now checks two independent paths: 1. Native 'slack' toolset + SLACK_BOT_TOKEN (as before, but now calls _get_platform_tools() with include_default_mcp_servers=True instead of False, so a default-enabled MCP server also counts). 2. A connected MCP server that has ACTUALLY registered tools into the live registry (new tools.mcp_tool.get_registered_mcp_server_names()), whose name suggests Slack. This is session-scoped in the sense that matters here: MCP servers connect once per gateway process (not per-session), so checking the live per-server tool-registration map is the correct availability-filtered signal -- unlike the earlier get_all_tool_names() approach this replaces, which conflated ALL built-in tool names process-wide, this only inspects the small, purpose-built MCP server-name map. Added a real regression test that registers a tool via the actual tools.mcp_tool._track_mcp_tool_server() tracking function (not a mock of the capability check) to verify a genuine Slack MCP server is detected, plus a negative case for an unrelated MCP server. 5/5 Slack-specific tests pass; 126/126 in the full tests/gateway/test_session.py file.
This commit is contained in:
parent
24d7333eb1
commit
96f21e8a54
3 changed files with 181 additions and 11 deletions
|
|
@ -342,6 +342,52 @@ that requires raw IDs). Discord is excluded because mentions use ``<@user_id>``
|
|||
and the LLM needs the real ID to tag users."""
|
||||
|
||||
|
||||
def _slack_tools_loaded() -> bool:
|
||||
"""True iff the agent will actually have Slack tools this session.
|
||||
|
||||
Two independent paths grant Slack capability:
|
||||
1. Native `slack` toolset enabled via `hermes tools` (opt-in, default
|
||||
OFF) AND `SLACK_BOT_TOKEN` set — the tool's `check_fn` gates on it
|
||||
at registry time, so config alone isn't enough.
|
||||
2. An MCP server that has ACTUALLY registered tools into the live
|
||||
registry (tools/mcp_tool.get_registered_mcp_server_names()), whose
|
||||
name suggests Slack. This is the real, availability-filtered
|
||||
signal (post-connection, post include/exclude filtering) rather
|
||||
than just what's listed in config.yaml -- a configured-but-
|
||||
unconnected or zero-tool MCP server must not claim capability.
|
||||
Named MCP servers are process-wide (one gateway connects each MCP
|
||||
server once, not per-session), so this check is intentionally NOT
|
||||
scoped further per-session -- unlike the earlier get_all_tool_names()
|
||||
approach this replaces, which conflated ALL built-in tool names
|
||||
process-wide, this only inspects the small, purpose-built MCP
|
||||
server-name map.
|
||||
|
||||
Returns False (safe default — keeps the stale-API disclaimer) on any
|
||||
error so a bad config can never silently promise tools the agent lacks.
|
||||
"""
|
||||
try:
|
||||
from tools.mcp_tool import get_registered_mcp_server_names
|
||||
if any("slack" in name.lower() for name in get_registered_mcp_server_names()):
|
||||
return True
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
if not (os.environ.get("SLACK_BOT_TOKEN") or "").strip():
|
||||
return False
|
||||
try:
|
||||
from hermes_cli.config import load_config
|
||||
from hermes_cli.tools_config import _get_platform_tools
|
||||
cfg = load_config()
|
||||
# include_default_mcp_servers=True (the default) so a Slack MCP
|
||||
# server that's enabled by default for this platform (not
|
||||
# explicitly listed) is also counted, in addition to the native
|
||||
# 'slack' toolset.
|
||||
enabled = _get_platform_tools(cfg, "slack")
|
||||
return "slack" in enabled
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
|
||||
def _discord_tools_loaded() -> bool:
|
||||
"""True iff the agent will actually have Discord tools this session.
|
||||
|
||||
|
|
@ -517,15 +563,29 @@ def build_session_context_prompt(
|
|||
|
||||
# Platform-specific behavioral notes
|
||||
if context.source.platform == Platform.SLACK:
|
||||
lines.append("")
|
||||
lines.append(
|
||||
"**Platform notes:** You are running inside Slack. "
|
||||
"You do NOT have access to Slack-specific APIs — you cannot search "
|
||||
"channel history, pin/unpin messages, manage channels, or list users. "
|
||||
"Do not promise to perform these actions. The gateway may inline the "
|
||||
"current message's Slack block/attachment payload when available, but "
|
||||
"you still cannot call Slack APIs yourself."
|
||||
)
|
||||
# Inject the Slack capability note only when the agent actually has
|
||||
# Slack tools loaded this session — native `slack` toolset opt-in,
|
||||
# or a connected MCP server that has registered Slack tools.
|
||||
# Otherwise keep the stale-API disclaimer honest so we never
|
||||
# promise tools the agent lacks. Mirrors the Discord pattern below.
|
||||
if _slack_tools_loaded():
|
||||
lines.append("")
|
||||
lines.append(
|
||||
"**Platform notes:** You are running inside Slack and have access "
|
||||
"to Slack-specific tools. You CAN search channel history, post "
|
||||
"messages, react to messages, and perform other Slack operations "
|
||||
"via the available Slack toolset."
|
||||
)
|
||||
else:
|
||||
lines.append("")
|
||||
lines.append(
|
||||
"**Platform notes:** You are running inside Slack. "
|
||||
"You do NOT have access to Slack-specific APIs — you cannot search "
|
||||
"channel history, pin/unpin messages, manage channels, or list users. "
|
||||
"Do not promise to perform these actions. The gateway may inline the "
|
||||
"current message's Slack block/attachment payload when available, but "
|
||||
"you still cannot call Slack APIs yourself."
|
||||
)
|
||||
if context.shared_multi_user_session:
|
||||
lines.append(
|
||||
"In shared Slack threads, use the current turn's sender prefix "
|
||||
|
|
|
|||
|
|
@ -278,7 +278,9 @@ class TestBuildSessionContextPrompt:
|
|||
# Static pointer tells the agent where the volatile id actually lives.
|
||||
assert "provided per-turn in the incoming user message" in p1
|
||||
|
||||
def test_slack_prompt_includes_platform_notes(self):
|
||||
def test_slack_prompt_no_tools_shows_disclaimer(self):
|
||||
"""Without slack toolset loaded, prompt must show the stale-API disclaimer."""
|
||||
from unittest.mock import patch
|
||||
config = GatewayConfig(
|
||||
platforms={
|
||||
Platform.SLACK: PlatformConfig(enabled=True, token="fake"),
|
||||
|
|
@ -292,7 +294,100 @@ class TestBuildSessionContextPrompt:
|
|||
user_name="bob",
|
||||
)
|
||||
ctx = build_session_context(source, config)
|
||||
prompt = build_session_context_prompt(ctx)
|
||||
with patch("gateway.session._slack_tools_loaded", return_value=False):
|
||||
prompt = build_session_context_prompt(ctx)
|
||||
|
||||
assert "Slack" in prompt
|
||||
assert "cannot search" in prompt.lower()
|
||||
assert "pin" in prompt.lower()
|
||||
assert "current message's slack block/attachment payload" in prompt.lower()
|
||||
assert "you can" not in prompt.lower() or "you cannot" in prompt.lower()
|
||||
|
||||
def test_slack_prompt_with_tools_shows_capability(self):
|
||||
"""When slack toolset is loaded, prompt must advertise API access."""
|
||||
from unittest.mock import patch
|
||||
config = GatewayConfig(
|
||||
platforms={
|
||||
Platform.SLACK: PlatformConfig(enabled=True, token="fake"),
|
||||
},
|
||||
)
|
||||
source = SessionSource(
|
||||
platform=Platform.SLACK,
|
||||
chat_id="C123",
|
||||
chat_name="general",
|
||||
chat_type="group",
|
||||
user_name="bob",
|
||||
)
|
||||
ctx = build_session_context(source, config)
|
||||
with patch("gateway.session._slack_tools_loaded", return_value=True):
|
||||
prompt = build_session_context_prompt(ctx)
|
||||
|
||||
assert "Slack" in prompt
|
||||
assert "have access" in prompt.lower() or "you can" in prompt.lower()
|
||||
assert "you do not have access" not in prompt.lower()
|
||||
|
||||
def test_slack_tools_loaded_detects_real_mcp_registration(self):
|
||||
"""Regression (review of #63234): a connected MCP server whose tools
|
||||
are ACTUALLY registered in the live registry must be detected as
|
||||
Slack capability, without mocking _slack_tools_loaded itself -- this
|
||||
exercises the real tools.mcp_tool registration signal the earlier
|
||||
(mocked-wholesale) tests didn't reach. Native SLACK_BOT_TOKEN/toolset
|
||||
config is intentionally left unset so only the MCP path can pass."""
|
||||
import os as _os
|
||||
from unittest.mock import patch
|
||||
from gateway.session import _slack_tools_loaded
|
||||
import tools.mcp_tool as _mcp_tool_mod
|
||||
|
||||
# No native slack toolset / token configured.
|
||||
with patch.dict(_os.environ, {}, clear=False):
|
||||
_os.environ.pop("SLACK_BOT_TOKEN", None)
|
||||
|
||||
# Simulate a connected MCP server ("company-slack") that has
|
||||
# registered a real tool, via the actual tracking function used
|
||||
# by the live registration path (tools/mcp_tool.py:_track_mcp_tool_server),
|
||||
# not a mock of the capability check.
|
||||
_mcp_tool_mod._track_mcp_tool_server("mcp-company-slack_post_message", "company-slack")
|
||||
try:
|
||||
assert _slack_tools_loaded() is True, (
|
||||
"A connected MCP server with 'slack' in its name and "
|
||||
"registered tools must be detected as Slack capability"
|
||||
)
|
||||
finally:
|
||||
_mcp_tool_mod._forget_mcp_tool_server("mcp-company-slack_post_message")
|
||||
|
||||
def test_slack_tools_loaded_false_when_no_matching_mcp_server(self):
|
||||
"""An MCP server unrelated to Slack must not grant Slack capability."""
|
||||
import os as _os
|
||||
from unittest.mock import patch
|
||||
from gateway.session import _slack_tools_loaded
|
||||
import tools.mcp_tool as _mcp_tool_mod
|
||||
|
||||
with patch.dict(_os.environ, {}, clear=False):
|
||||
_os.environ.pop("SLACK_BOT_TOKEN", None)
|
||||
_mcp_tool_mod._track_mcp_tool_server("mcp-github_create_issue", "github")
|
||||
try:
|
||||
assert _slack_tools_loaded() is False
|
||||
finally:
|
||||
_mcp_tool_mod._forget_mcp_tool_server("mcp-github_create_issue")
|
||||
|
||||
def test_slack_prompt_includes_platform_notes(self):
|
||||
"""Legacy: backward-compat alias -- no tools loaded shows disclaimer."""
|
||||
from unittest.mock import patch
|
||||
config = GatewayConfig(
|
||||
platforms={
|
||||
Platform.SLACK: PlatformConfig(enabled=True, token="fake"),
|
||||
},
|
||||
)
|
||||
source = SessionSource(
|
||||
platform=Platform.SLACK,
|
||||
chat_id="C123",
|
||||
chat_name="general",
|
||||
chat_type="group",
|
||||
user_name="bob",
|
||||
)
|
||||
ctx = build_session_context(source, config)
|
||||
with patch("gateway.session._slack_tools_loaded", return_value=False):
|
||||
prompt = build_session_context_prompt(ctx)
|
||||
|
||||
assert "Slack" in prompt
|
||||
assert "cannot search" in prompt.lower()
|
||||
|
|
|
|||
|
|
@ -5969,6 +5969,21 @@ def has_registered_mcp_tools() -> bool:
|
|||
return bool(_mcp_tool_server_names)
|
||||
|
||||
|
||||
def get_registered_mcp_server_names() -> set:
|
||||
"""Return the set of MCP server names that have actually registered at
|
||||
least one tool into the registry (post-connection, post check_fn/include-
|
||||
exclude filtering) -- i.e. the real, availability-filtered signal, not
|
||||
just what's present in config.yaml under ``mcp_servers``.
|
||||
|
||||
Used by capability-aware prompt building (e.g. gateway/session.py's
|
||||
Slack platform note) to detect an MCP server that provides a given
|
||||
platform's capability regardless of what its config key is named.
|
||||
"""
|
||||
with _lock:
|
||||
return set(_mcp_tool_server_names.values())
|
||||
|
||||
|
||||
|
||||
def refresh_agent_mcp_tools(
|
||||
agent,
|
||||
*,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue