Skip to content

fix(slack): guard _resolve_thread_ts against async/cron deliveries using stale thread context - #59194

Closed
wesleysimplicio wants to merge 2 commits into
NousResearch:mainfrom
wesleysimplicio:simplicio/fix-59097-slack-cron-thread-routing
Closed

fix(slack): guard _resolve_thread_ts against async/cron deliveries using stale thread context#59194
wesleysimplicio wants to merge 2 commits into
NousResearch:mainfrom
wesleysimplicio:simplicio/fix-59097-slack-cron-thread-routing

Conversation

@wesleysimplicio

Copy link
Copy Markdown
Contributor

Closes #59097

Cron/async deliveries (reply_to=None) should go to the home/target channel,
but _resolve_thread_ts() returns metadata.thread_id/thread_ts even when
reply_to is None, routing messages to the thread where the cron job was
created instead of the configured home channel.

Adds early return None when reply_to is None, so async deliveries
never inherit stale thread context from metadata.

Change: plugins/platforms/slack/adapter.py:_resolve_thread_ts()

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/slack Slack app adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P3 Low — cosmetic, nice to have labels Jul 5, 2026

@AmirF194 AmirF194 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for chasing this down, the symptom in #59097 is real: a cron job scheduled from a thread should not keep replying into that thread once a home channel is set. I do not think this fix is safe as written though, and CI is red for the reason I would flag.

The guard treats reply_to is None as "this is a cron/async delivery," but in this codebase that is not what it means. The established contract, which the test suite pins down in a lot of places, is that send(chat_id, content, metadata={"thread_id": ...}) with no reply_to means "reply in this thread." Several legitimate callers pass reply_to=None plus a real metadata.thread_id and rely on getting the thread back: send_exec_approval and send_slash_confirm (approval/confirm prompts should stay in the command's thread), send_multiple_images, send_private_notice, the file-upload paths, and the plain threaded send(). Returning None for all of them routes those to the channel root.

I confirmed the breakage in a clean Python 3.11 container matching CI: on the PR head tests/gateway/test_slack.py is 9 failed / 207 passed (plus 2 in test_slack_approval_buttons.py), all of the "metadata thread + no reply_to should reply in-thread" variety, and reverting the adapter to main makes them pass again. That is the same set failing on CI slices 1/8 and 4/8, so it is a genuine regression, not environmental.

It also breaks the explicit-thread cron case: gateway/delivery.py copies target.thread_id into send_metadata["thread_id"] and calls send() with no reply_to, so a cron job intentionally targeted at a specific thread would now post to the channel root too.

The actual leak is upstream in cron/scheduler.py::_resolve_single_delivery_target: for deliver=<platform>/all, the same-platform-origin branch returns "thread_id": origin.get("thread_id") (the thread the job was created in) and wins over the configured home target. That origin thread flows through delivery into the adapter. I would fix it there, for non-origin deliveries with a home channel configured resolve to the home root instead of the origin thread. That fixes it for every platform at once (the issue lists Telegram/Discord/WhatsApp as affected) and keeps real thread replies working. If you still want a belt-and-suspenders check in the adapter, key it off an explicit cron/async flag in metadata rather than the absence of reply_to, and add a regression test for "thread-scheduled cron + home set delivers to channel root."

@wesleysimplicio

wesleysimplicio commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@AmirF194 Thanks — you were right, and I've moved the fix to the point you identified.

Changes applied:

  • Reverted the broad reply_to is None guard in the Slack adapter, which wrongly routed legitimate in-thread callers (send_exec_approval, send_slash_confirm, send_multiple_images, threaded send(), etc.) to the channel root.
  • Fixed the real leak in cron/scheduler.py::_resolve_single_delivery_target: for a bare per-platform delivery (deliver=<platform>/all) where a home channel is configured, it now resolves to the home root instead of reusing the origin thread. This fixes every platform at once (Telegram/Discord/WhatsApp) and keeps deliver=origin and explicit-thread targets replying in-thread.
  • Added a regression test (test_bare_platform_delivery_uses_home_root_instead_of_origin_thread) covering a job scheduled from a thread with a home channel set → delivered to the home root.

Validation:

Command Result
python -m pytest tests/cron/test_scheduler.py::TestResolveDeliveryTarget -q pass
python -m pytest tests/gateway/test_slack.py -q pass (no adapter regression)

CI is green. Happy to adjust further if you'd prefer the belt-and-suspenders metadata flag as well.

@wesleysimplicio

Copy link
Copy Markdown
Contributor Author

Revisei o feedback e movi a correção para o ponto certo do fluxo.

Mudanças aplicadas:

  • reverti o guard amplo em plugins/platforms/slack/adapter.py, que quebrava replies legítimos em thread quando reply_to=None
  • corrigi cron/scheduler.py::_resolve_single_delivery_target() para que entregas bare por plataforma (discord, telegram, etc.), quando existe home target configurado, resolvam para o home target em vez de reutilizar a thread de origem
  • adicionei regressão cobrindo o caso deliver=discord com origem numa thread, garantindo entrega no home root

Validação:

  • uv run --extra dev --extra slack pytest tests/cron/test_scheduler.py tests/gateway/test_slack.py tests/gateway/test_slack_approval_buttons.py -q
  • 455 passed

@wesleysimplicio

Copy link
Copy Markdown
Contributor Author

Closing due to no maintainer engagement after more than 4 days open. Re-opening if there is renewed interest, or will resubmit rebased/split if still relevant.

@wesleysimplicio

Copy link
Copy Markdown
Contributor Author

Reopening — this PR has an active maintainer review (@AmirF194, 2026-07-05) that was already addressed with a follow-up commit and explanation on 2026-07-06. It was closed in error by the automated staleness sweep, which should only apply to PRs with zero maintainer engagement. Apologies for the noise.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for moving the fix to the scheduler resolver after the initial adapter-level approach was shown to break legitimate threaded sends.

Current main still has the reported defect: cron/scheduler.py:1162-1167 returns the same-platform origin chat and thread before it reaches the configured-home lookup at cron/scheduler.py:1171-1178. deliver=all reaches the same helper through cron/scheduler.py:1251, so the revised change covers both affected forms. The added regression test is aligned with this path, and deliver=origin remains unchanged.

No blocking correctness or design issues found. GitHub currently reports this PR as MERGEABLE / CLEAN.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 15, 2026
…ing stale thread context

Cron/async deliveries (reply_to=None) should always go to the home/target
channel, but _resolve_thread_ts() returns metadata.thread_id/thread_ts even
when reply_to is None, routing messages to the thread where the cron job
was created instead of the configured home channel.

Add early return None when reply_to is None, so async deliveries never
inherit stale thread context from metadata.

Closes NousResearch#59097
@wesleysimplicio
wesleysimplicio force-pushed the simplicio/fix-59097-slack-cron-thread-routing branch from 5db7df8 to 2dc7c08 Compare July 22, 2026 19:42
@teknium1

Copy link
Copy Markdown
Contributor

Merged via #69480 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: both your commits were cherry-picked — including your own cron-side relayering, which was the right call and closes #59097.

Thanks for the contribution!

@teknium1 teknium1 closed this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Slack adapter routes async/cron deliveries to stale thread context instead of home channel

4 participants