fix: advertise Slack context tools when available - #36676
Conversation
|
Vucar staging is already patched and restarted (Hermes gateway PID 1234536). Targeted staging test passed: |
alt-glitch
added
type/bug
mxnstrexgl
left a comment
mxnstrexgl
left a comment
There was a problem hiding this comment.
🤖 Automated PR Review
Security Scan
- ✓ No hardcoded secrets, injection sinks, unsafe deserialization, or dependency red flags found by this automated scan.
Code Quality
- ✓ No blocking code-quality issues found by this automated scan.
Summary
Status: APPROVE — security findings: 0, quality suggestions: 0.
Automated review; raw diff content intentionally omitted.
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the stale Slack platform note. The current patch needs rework before it can safely advertise capabilities.
Problems
gateway/session.py:246tests for a"slack"toolset, but current main has onlyhermes-slack(toolsets.py:468-471) and no registeredslack_get_*tools. The prompt can therefore describe APIs that are not in the model schema.gateway/session.py:348directly interpolateschat_name, bypassing current_format_untrusted_prompt_value()hardening (gateway/session.py:372-378; introduced by09666ceb).gateway/session.py:352adds volatilemessage_idto the cached system prompt. Current main deliberately moved the Discord equivalent to per-turn user content to preserve cache stability (d6c53dcd;tests/gateway/test_session.py:256-278).- The new tests stub the capability helper, so they do not validate resolver/registry behavior or prompt stability.
Suggested changes
- Gate the note on concrete, selected registered tools; align names with the final Slack-history schema.
- Serialize metadata, move message IDs to per-turn content, and add resolver, injection, and cache-invariance regressions.
Automated hermes-sweeper review.
| from hermes_cli.tools_config import _get_platform_tools | ||
| cfg = load_config() | ||
| enabled = _get_platform_tools(cfg, "slack", include_default_mcp_servers=False) | ||
| return "slack" in enabled |
There was a problem hiding this comment.
_get_platform_tools() returns toolset names, but neither this PR's base nor current main defines a slack toolset or the advertised slack_get_* tools. This can never enable under normal configuration, or can advertise APIs that are absent if an unknown slack passthrough is manually configured. Gate on actual selected registered tool definitions instead.
| ) | ||
| id_lines.append(f" - Channel ID for tool calls: `{src.chat_id}`") | ||
| if src.chat_name and src.chat_name != src.chat_id: | ||
| id_lines.append(f" - Channel name: `{src.chat_name}`") |
There was a problem hiding this comment.
chat_name is untrusted gateway metadata. Current main requires _format_untrusted_prompt_value() for this class of value (gateway/session.py:372-378); direct Markdown interpolation here reopens the prompt-injection surface fixed by 09666ceb.
| if src.thread_id: | ||
| id_lines.append(f" - Thread timestamp for `slack_get_thread`: `{src.thread_id}`") | ||
| if src.message_id: | ||
| id_lines.append(f" - Triggering message timestamp: `{src.message_id}`") |
There was a problem hiding this comment.
message_id changes every inbound turn, so placing it in build_session_context_prompt() churns the cached system-prompt signature. Current main's d6c53dcd moved the Discord equivalent into per-turn user content and tests this invariant; use the same pattern for Slack.
teknium1
added
sweeper:risk-security-boundary
4 participants
Summary
slack_get_thread/slack_get_messagesWhy
Recent Hermes Slack context tools let the agent verify Slack thread/channel context directly. The built-in Slack platform note still said the opposite, which can make the model avoid the tools and fall back to pasted snippets or assumptions.
Validation
python -m py_compile gateway/session.py tests/gateway/test_slack_session_context_prompt.pypython -m pytest tests/gateway/test_slack_session_context_prompt.py -q(2 passed)git diff --check