Skip to content

fix: support Slack thread delivery targets - #18911

Closed
EndeavorYen wants to merge 1 commit into
NousResearch:mainfrom
EndeavorYen:codex/slack-thread-delivery
Closed

fix: support Slack thread delivery targets#18911
EndeavorYen wants to merge 1 commit into
NousResearch:mainfrom
EndeavorYen:codex/slack-thread-delivery

Conversation

@EndeavorYen

Copy link
Copy Markdown
Contributor

Summary

  • allow explicit Slack send_message targets to include a thread_ts suffix, e.g. slack:D123ABCDEF:1777740958.516299
  • pass the parsed Slack thread_ts through to chat.postMessage
  • add cron/send_message regression coverage for Slack thread delivery targets
  • isolate silent-delivery cron tests from the shared scheduler lock under xdist

Why

Telegram and Discord delivery targets already support platform:chat_id:thread_id, but Slack explicit conversation IDs were parsed as channel-only targets. That means a cron or tool delivery target like slack:D...:thread_ts could be treated as an invalid Slack channel value instead of posting into the intended thread.

Validation

  • /Users/simon/.hermes/hermes-agent/venv/bin/python -m pytest tests/tools/test_send_message_tool.py::TestParseTargetRefSlack tests/tools/test_send_message_tool.py::TestSendToPlatformChunking::test_slack_thread_id_is_passed_to_send tests/cron/test_scheduler.py::TestResolveDeliveryTarget
  • /Users/simon/.hermes/hermes-agent/venv/bin/python -m pytest tests/tools/test_send_message_tool.py tests/cron/test_scheduler.py

Local runtime smoke on a forked Hermes deployment also confirmed cron delivery posted into the expected Slack thread.

Allow explicit Slack delivery targets to include a thread_ts suffix and pass it through to chat.postMessage. Add regression coverage for send_message and cron target resolution, plus per-test cron lock isolation for silent-delivery tests under xdist.
@EndeavorYen
EndeavorYen marked this pull request as ready for review May 2, 2026 17:38
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets comp/cron Cron scheduler and job management platform/slack Slack app adapter labels May 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #16992 — same fix for Slack thread_ts parsing in send_message targets. Also related to earlier closed #14146 and #16962.

@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #16992

@EndeavorYen

Copy link
Copy Markdown
Contributor Author

Thanks, agreed. #16992 covers the main runtime fix here: parsing Slack thread delivery targets and passing thread_ts through to chat.postMessage.

I’ll close this PR to avoid splitting the same fix across two branches. The only extra pieces here are:

  • a cron delivery-target regression test for slack:conversation_id:thread_ts
  • xdist isolation for TestSilentDelivery, which was sharing the scheduler lock
  • slightly stricter Slack conversation ID validation (C/G/D...) for explicit targets

Happy to move any of those into #16992 if useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists platform/slack Slack app adapter type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants