Skip to content

fix(slack): split reply threading defaults by surface - #32580

Closed
jackberger03 wants to merge 1 commit into
NousResearch:mainfrom
jackberger03:fix/slack-dm-direct-replies
Closed

fix(slack): split reply threading defaults by surface#32580
jackberger03 wants to merge 1 commit into
NousResearch:mainfrom
jackberger03:fix/slack-dm-direct-replies

Conversation

@jackberger03

Copy link
Copy Markdown

Summary

  • split Slack reply threading defaults by surface: DMs default to direct replies, channels keep threaded replies
  • preserve legacy reply_in_thread as an override for both surfaces
  • include Slack chat type/message id in thread metadata so synthetic top-level threads are not mistaken for real threads by progress/final sends

Linear

  • OTO-497

Tests

  • python -m pytest tests/gateway/test_slack.py::TestProgressMessageThread::test_resolve_thread_ts_splits_dm_and_channel_defaults tests/gateway/test_slack.py::TestProgressMessageThread::test_resolve_thread_ts_honors_existing_threads tests/gateway/test_slack.py::TestProgressMessageThread::test_resolve_thread_ts_legacy_reply_in_thread_overrides_both_surfaces tests/gateway/test_slack_mention.py::test_config_bridges_slack_reply_in_thread_surface_knobs -q -o 'addopts='
  • python -m pytest tests/gateway/test_slack.py tests/gateway/test_slack_mention.py tests/gateway/test_config.py tests/gateway/test_config_env_bridge_authority.py -q -o 'addopts='

Note: this is a salvage of the closed PR #30088 approach; commit authorship is credited to the original author.

@alt-glitch alt-glitch added type/feature New feature or request P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/slack Slack app adapter area/config Config system, migrations, profiles labels May 26, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for preserving the original Slack threading work. The underlying behavior is still present on current main: top-level DMs receive a synthetic thread_ts at plugins/platforms/slack/adapter.py:2781-2784, and _resolve_thread_ts returns the metadata thread id by default at plugins/platforms/slack/adapter.py:1661-1666.

Problems

  • The diff targets gateway/platforms/slack.py, but Slack was migrated to plugins/platforms/slack/adapter.py in 560010547. The old adapter path is no longer the runtime implementation.
  • The proposed Slack-specific additions in gateway/config.py and gateway/run.py conflict with the current plugin ownership: Slack YAML translation now lives in plugins/platforms/slack/adapter.py:_apply_yaml_config (4485-4519), and thread metadata is generic in gateway/run.py:14357-14401.

Suggested changes

  • Port the resolver and its send call sites to plugins/platforms/slack/adapter.py.
  • Make the two new settings plugin-owned extras while retaining legacy reply_in_thread precedence.
  • Update coverage for direct DM final/progress sends through the current plugin path.

Automated hermes-sweeper review.

@@ -962,39 +962,67 @@ def _resolve_thread_ts(
self,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Current main migrated this adapter to plugins/platforms/slack/adapter.py in 560010547; this path is no longer the runtime Slack implementation. Please port this resolver change and its send call sites to the bundled Slack plugin.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via #70196 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: your surface-split reply-threading defaults were cherry-picked.

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

area/config Config system, migrations, profiles comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants