Skip to content

feat(slack): multi-workspace Socket Mode (semantic port of #20587) - #66398

Closed
jordanhubbard wants to merge 3 commits into
NousResearch:mainfrom
jordanhubbard:feature/slack-multi-workspace-socket-mode-v3
Closed

feat(slack): multi-workspace Socket Mode (semantic port of #20587)#66398
jordanhubbard wants to merge 3 commits into
NousResearch:mainfrom
jordanhubbard:feature/slack-multi-workspace-socket-mode-v3

Conversation

@jordanhubbard

Copy link
Copy Markdown
Contributor

Supersedes #20587. That PR modified the pre-relocation gateway/platforms/slack.py; commit 5600105 moved the Slack adapter to the bundled plugin, so this is a semantic re-implementation against plugins/platforms/slack/adapter.py that also resolves the hermes-sweeper review findings.

Stack

Stacked on #20583 (scopes Slack sessions/routing by workspace). Review/merge #20583 first; the meaningful diff here is the single top commit on top of that branch. main has advanced since the stack was cut, so a rebase onto latest main is needed before merge (same situation as #20587). Advances #10099.

What this adds

  • ~/.hermes/slack_accounts.json: one AsyncApp + Socket Mode websocket per account. Slack distributes a single app token's events across competing sockets, so each workspace needs its own app-token connection to receive reliably.
  • The primary account keeps the legacy self._app/_handler/_socket_mode_task fields and lock, so existing single-workspace behavior (and its tests) are unchanged; extra accounts live in self._extra_connections with their own scoped slack-app-token locks.
  • _register_app_handlers(app) shares handler registration across every account's app.
  • Channel→team routing is persisted to ~/.hermes/slack_channel_teams.json and reloaded on connect, so outbound sends after a restart reach the right workspace client.

Addresses the hermes-sweeper review on #20587

  • Ported into the bundled plugin, not the relocated gateway/platforms/slack.py.
  • Retains per-socket recovery: the existing Socket Mode watchdog now monitors every extra connection and restarts dead/disconnected ones per-account (_restart_extra_connection) — the original PR's raw create_task() path had no monitor.
  • Session/routing key changes stay Slack-scoped (scope_id); Discord and other platforms keep their existing key format (via the fix(slack): scope sessions and routing by workspace #20583 base this is stacked on) — no cross-platform session-key regression.
  • Preserves file_shared_handle_slack_file_shared (the original PR regressed it to a no-op pass) and all newer handlers.

Tests

tests/gateway/test_slack.py::TestSlackMultiAccountSocketMode (9 tests):

  • one connection per account; each holds its own app-token lock
  • watchdog restarts a dead extra connection; restart rebuilds the socket
  • file_shared dispatches to the real handler; action handlers registered per app
  • colliding workspace IDs still open both sockets; duplicate app token in config is skipped; channel→team routing survives a restart

Full tests/gateway/test_slack.py + test_session.py + test_slack_approval_buttons.py: 403 passed (394 pre-existing + 9 new).

🤖 Generated with Claude Code

jordanhubbard and others added 2 commits July 16, 2026 00:00
…ch#20587)

Salvages PR NousResearch#20587 onto current main. The original PR modified the
pre-relocation gateway/platforms/slack.py; commit 5600105 moved the Slack
adapter to the bundled plugin, so this is a semantic re-implementation
against plugins/platforms/slack/adapter.py that also fixes the
hermes-sweeper review findings.

What this adds
- ~/.hermes/slack_accounts.json: one AsyncApp + Socket Mode websocket per
  account. Slack distributes a token's events across competing sockets, so
  each workspace needs its own app-token connection to receive reliably.
- The primary account keeps the legacy self._app/_handler/_socket_mode_task
  fields and lock, so existing single-workspace behavior and tests are
  unchanged; extra accounts live in self._extra_connections with their own
  scoped slack-app-token locks.
- _register_app_handlers(app) shares handler registration across every
  account's app.
- Channel→team routing is persisted to ~/.hermes/slack_channel_teams.json
  and reloaded on connect so outbound sends after a restart reach the right
  workspace client.

Addresses the review feedback on NousResearch#20587
- Ported into the bundled plugin, not the relocated module.
- Retains per-socket recovery: the existing Socket Mode watchdog now
  monitors every extra connection and restarts dead/disconnected ones
  per-account (_restart_extra_connection).
- Session/routing key changes stay Slack-scoped (scope_id); Discord and
  other platforms keep their existing key format (from the session-scoping
  work this is stacked on).
- Preserves file_shared -> _handle_slack_file_shared (the original PR
  regressed it to a no-op) and all newer handlers.

Tests (tests/gateway/test_slack.py::TestSlackMultiAccountSocketMode)
- one connection per account; each holds its own app-token lock
- watchdog restarts a dead extra connection; restart rebuilds the socket
- file_shared dispatches to the real handler; action handlers registered
  per app; colliding workspace IDs still open both sockets; duplicate app
  token in config is skipped; channel→team routing survives a restart

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery 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 sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state needs-decision Awaiting maintainer decision before any implementation labels Jul 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #20587 and stacked on #20583, rather than a duplicate: this is a plugin-path semantic port with additional workspace-scoped routing/session migration and recovery behavior. Please review/rebase the stack together.

…-workspace-socket-mode-v3

# Conflicts:
#	gateway/session.py
#	plugins/platforms/slack/adapter.py
@jordanhubbard

Copy link
Copy Markdown
Contributor Author

Merged latest main and resolved conflicts (no longer CONFLICTING). Resolutions:

ruff check clean; tests/gateway/test_slack.py + test_session.py + test_slack_approval_buttons.py → 410 passed.

@teknium1 teknium1 added 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 18, 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 the semantic port and for retaining the per-socket recovery design. The multi-account receive premise remains real on current main: website/docs/user-guide/messaging/slack.md:564-605 configures multiple bot tokens but one Socket Mode app token.

Problems

  • gateway/session.py:1527-1533 catches every find_latest_gateway_session_for_peer failure in _find_gateway_session_row() and returns None. This bypasses _recover_session_from_db(..., raise_on_lookup_error=True) at gateway/session.py:1541-1569. Current main's startup prune deliberately catches that propagated failure and retains the stale route (gateway/session.py:1203-1220); with this change, an unavailable/failed DB lookup is indistinguishable from no recoverable session and can remove the route.

Suggested changes

  • Preserve the existing fail-closed propagation path, then add a regression test for a raised recovery lookup during stale-route pruning.

Automated hermes-sweeper review.

Comment thread gateway/session.py
chat_type=source.chat_type if allow_peer_fallback else None,
thread_id=source.thread_id,
)
except Exception as exc:

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.

This catch makes raise_on_lookup_error=True ineffective: _recover_session_from_db() receives None rather than the DB exception. Current startup pruning relies on that exception to retain a stale route rather than remove it. Please propagate errors for the fail-closed caller (or parameterize this helper) and add coverage.

teknium1 added a commit that referenced this pull request Jul 23, 2026
Conflict-resolution follow-up composing #68925 (Bob) with the already-
applied #20583/#66398 (jordanhubbard) recovery design:

- #68925's caller-level second _query_recoverable_session pass (via
  lookup_session_key=) referenced a variable that no longer exists —
  the legacy exact-key fallback now lives INSIDE
  _query_recoverable_session, which also claims the legacy key once per
  process and rewrites the peer row to the scoped key. Drop the dead
  caller-level pass.
- Keep #68925's _recovered_row_matches_source_scope origin guard wired
  into both recovery paths: a scoped channel lookup refuses rows whose
  recorded origin names another workspace (or no workspace at all).
- Routing-index migration adoption policy documented at the site:
  origin names a workspace -> exact match only; scope-less DM -> first
  workspace claims once; scope-less channel -> refuse.
teknium1 added a commit that referenced this pull request Jul 23, 2026
Conflict-resolution follow-up composing #68925 (Bob) with the already-
applied #20583/#66398 (jordanhubbard) recovery design:

- #68925's caller-level second _query_recoverable_session pass (via
  lookup_session_key=) referenced a variable that no longer exists —
  the legacy exact-key fallback now lives INSIDE
  _query_recoverable_session, which also claims the legacy key once per
  process and rewrites the peer row to the scoped key. Drop the dead
  caller-level pass.
- Keep #68925's _recovered_row_matches_source_scope origin guard wired
  into both recovery paths: a scoped channel lookup refuses rows whose
  recorded origin names another workspace (or no workspace at all).
- Routing-index migration adoption policy documented at the site:
  origin names a workspace -> exact match only; scope-less DM -> first
  workspace claims once; scope-less channel -> refuse.
teknium1 added a commit that referenced this pull request Jul 23, 2026
Conflict-resolution follow-up composing #68925 (Bob) with the already-
applied #20583/#66398 (jordanhubbard) recovery design:

- #68925's caller-level second _query_recoverable_session pass (via
  lookup_session_key=) referenced a variable that no longer exists —
  the legacy exact-key fallback now lives INSIDE
  _query_recoverable_session, which also claims the legacy key once per
  process and rewrites the peer row to the scoped key. Drop the dead
  caller-level pass.
- Keep #68925's _recovered_row_matches_source_scope origin guard wired
  into both recovery paths: a scoped channel lookup refuses rows whose
  recorded origin names another workspace (or no workspace at all).
- Routing-index migration adoption policy documented at the site:
  origin names a workspace -> exact match only; scope-less DM -> first
  workspace claims once; scope-less channel -> refuse.
@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded by #70190 (merged): the session-scoping correctness half landed via #70190 with your authorship; the multi-app Socket Mode feature half remains open for the multi-tenant feature review.

Thanks for the work — it's credited in #70190's summary.

@teknium1 teknium1 closed this Jul 23, 2026
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
Conflict-resolution follow-up composing NousResearch#68925 (Bob) with the already-
applied NousResearch#20583/NousResearch#66398 (jordanhubbard) recovery design:

- NousResearch#68925's caller-level second _query_recoverable_session pass (via
  lookup_session_key=) referenced a variable that no longer exists —
  the legacy exact-key fallback now lives INSIDE
  _query_recoverable_session, which also claims the legacy key once per
  process and rewrites the peer row to the scoped key. Drop the dead
  caller-level pass.
- Keep NousResearch#68925's _recovered_row_matches_source_scope origin guard wired
  into both recovery paths: a scoped channel lookup refuses rows whose
  recorded origin names another workspace (or no workspace at all).
- Routing-index migration adoption policy documented at the site:
  origin names a workspace -> exact match only; scope-less DM -> first
  workspace claims once; scope-less channel -> refuse.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins needs-decision Awaiting maintainer decision before any implementation 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-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.

3 participants