Conversation
Related: #17261 (root issue), #51198 (this PR's issue), #21800 and #23913 (earlier competing fixes). Note for reviewers: this PR targets the current Slack adapter location ( |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Overview
Fixes Slack platform adapter and send_message tool to properly upload media attachments (images, files) via the Slack files.upload API instead of failing or sending text-only messages.
Analysis
- 4 files changed: adapter, send_message_tool, and 2 test files
- Fixes a real gap where Slack file attachments were not being sent
- No credentials, secrets, or security concerns in the diff
- Tests cover the adapter changes
Looks Good
- Clean fix targeting a specific platform integration gap
- Good test coverage
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Enables media attachment uploads in the Slack send_message tool by using the Slack Web API files.getUploadURL and files.completeUpload endpoints instead of chat.postMessage with file tokens.
Changes
tools/send_message_tool.py: +26/-15 to handle Slack media uploadsplugins/platforms/slack/adapter.py: +60/-0 for new helpertests/gateway/test_slack.py,tests/tools/test_send_message_tool.py: +104 lines test coverage
Quality
- 4 files, focused change
- Good test coverage
- No security concerns
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting the current Slack plugin adapter; the underlying defect remains on current main. tools/send_message_tool.py:1034-1062 still treats Slack as non-media: media-only sends are rejected, and text-plus-media sends call Slack's standalone sender without media_files.
Problems
- The change needs to update the newer expectations in
tests/tools/test_signal_media.py:124-147and:153-191. Those tests currently require media-only Slack sends to fail and text-plus-media sends to carry an omission warning; both contradict the proposed native-upload contract.
Suggested changes
- During salvage, replace those expectations with assertions that Slack receives
media_filesand returns native-delivery success. - Keep the final-text-chunk attachment rule in the current dispatcher; it matches the existing Discord, Matrix, Signal, Feishu, and WhatsApp patterns in
tools/send_message_tool.py:861-1032.
The adapter approach fits the existing Slack files_upload_v2 batching implementation at plugins/platforms/slack/adapter.py:1717-1819. This is an automated hermes-sweeper review.
| # --- Slack: native media attachment support via the registry's | ||
| # standalone_sender_fn (plugins/platforms/slack/adapter.py::_standalone_send). | ||
| if platform == Platform.SLACK: | ||
| from gateway.platform_registry import platform_registry |
There was a problem hiding this comment.
This changes media-only Slack delivery from an error to an upload, but the PR does not update tests/tools/test_signal_media.py:124-147, which currently asserts the old error contract. Update that test and its Slack omission-warning companion at lines 153-191 when salvaging this branch.
Summary
send_message/cronMEDIA:<path>attachments through the Slack plugin standalone sender instead of dropping them as unsupported mediafiles_upload_v2handling for standalone Slack sends, including thread preservation, captions/initial comments, and 10-file upload chunkingCloses #51198
Test Plan
uv run pytest tests/tools/test_send_message_tool.py::TestSendToPlatformChunking::test_slack_media_attaches_to_last_chunk tests/gateway/test_slack.py::TestSendDocument::test_standalone_send_uploads_media_files -quv run pytest tests/tools/test_send_message_tool.py tests/gateway/test_slack.py -quv run ruff check tools/send_message_tool.py plugins/platforms/slack/adapter.py tests/tools/test_send_message_tool.py tests/gateway/test_slack.pyuv run ruff check . --extend-exclude build