Skip to content

fix(slack): scope top-level channel messages by channel-only when reply_in_thread=false (#15421) - #15464

Closed
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/slack-top-level-session-scope
Closed

fix(slack): scope top-level channel messages by channel-only when reply_in_thread=false (#15421)#15464
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/slack-top-level-session-scope

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes `#15421` bug 1: top-level Slack channel messages previously fell back to the message's own `ts` as a synthetic `thread_ts`:

```python
thread_ts = event.get("thread_ts") or ts # ts fallback for channels
```

That value flowed into `build_source(thread_id=thread_ts)`. The gateway session store keys sessions by `(platform, channel_id, thread_id)`, so every top-level channel message ended up on a unique session. Operators who set `reply_in_thread: false` in `config.yaml` expected all top-level channel messages to share one session — instead each one spawned a fresh conversation with no context carry-over.

Fix

Three explicit cases in the channel branch:

`event.thread_ts` `reply_in_thread` `thread_ts` for session keying
non-null (real thread reply) either `event.thread_ts`
null (top-level) true (default) `ts` (legacy: own-thread sessions)
null (top-level) false `None` (shared channel session)

The outbound-reply gate at line 1264 (`reply_to_message_id = thread_ts if thread_ts != ts else None`) already works correctly in all three cases without further changes: `None != ts` is True, so shared-channel top-level messages don't get their reply threaded either — matching the operator's `reply_in_thread=false` intent end-to-end.

Genuine thread replies still scope per-thread under both modes so multi-person threaded conversations can't collide with unrelated channel chatter.

Related Issue

Fixes #15421 bug 1 only. Bug 2 ("sessions.json not persisting across compression") lives elsewhere in the session manager and is left for a separate diff.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)

Test plan

  • 7 new tests in `tests/gateway/test_slack_channel_session_scope.py` — all green on py3.11 venv
  • Full slack suite — 182 tests pass (175 existing + 7 new, zero regressions)
  • Regression guard verified: reverted the else-branch to the legacy `thread_ts = event.get("thread_ts") or ts` one-liner; `test_top_level_maps_to_none_when_reply_in_thread_false` correctly failed with an assertion message pointing at the regressed invariant. Restored → all 7 pass.
  • Pre-push discipline applied: no bare `except Exception`, no `int()` on user data, no unused imports, no dead helpers (autouse pytest fixture in the file is a false positive on a naive regex scan).

Test coverage detail

All tests drive the real `SlackAdapter._handle_slack_message` code path (not a re-implementation) via the standard pytest fixture pattern used by `tests/gateway/test_slack.py`. Messages @mention the bot so the mention gate doesn't drop them — the tests are specifically about what happens once the handler decides to emit a `MessageEvent`.

`TestChannelSessionScopeDefault` (2 cases — regression guard for the legacy default):

  • `test_top_level_maps_to_ts_when_reply_in_thread_true` — explicit `reply_in_thread: true` keeps `thread_id = ts`
  • `test_top_level_default_behaves_like_true` — unset config behaves like `true` (pins the default)

`TestChannelSessionScopeShared` (3 cases — the #15421 fix):

  • `test_top_level_maps_to_none_when_reply_in_thread_false` — the core fix
  • `test_top_level_reply_to_id_stays_none_when_shared` — outbound doesn't get threaded either
  • `test_thread_reply_scopes_by_thread_even_when_shared` — thread replies still per-thread

`TestThreadReplyAlwaysScopesByThread` (2 parametrised cases):

  • Thread replies get `thread_id = event.thread_ts` regardless of `reply_in_thread` — critical invariant for multi-thread channels

Not in scope

…ly_in_thread=false (NousResearch#15421)

Top-level Slack channel messages previously fell back to the message's
own ``ts`` as a synthetic ``thread_ts``:

    thread_ts = event.get("thread_ts") or ts  # ts fallback for channels

That value flows into ``build_source(thread_id=thread_ts)`` at
line 1247.  The gateway session store keys sessions by
``(platform, channel_id, thread_id)``, so every top-level channel
message ended up on a unique session.  Operators who set
``reply_in_thread: false`` in ``config.yaml`` expected all top-level
channel messages to share one session (the whole point of that flag)
— instead each one spawned a fresh conversation with no context
carry-over.

### Fix

Three explicit cases in the channel branch:

| event.thread_ts | reply_in_thread | thread_ts for session keying |
|---|---|---|
| non-null (real thread reply) | either | event.thread_ts |
| null (top-level) | true (default) | ts (legacy: own-thread sessions) |
| null (top-level) | false | **None** (shared channel session) |

The outbound-reply gate at line 1264 (``reply_to_message_id =
thread_ts if thread_ts != ts else None``) still works correctly in
all three cases without further changes: ``None != ts`` is True, so
shared-channel top-level messages don't get their reply threaded
either — matching the operator's ``reply_in_thread=false`` intent
end-to-end.

Genuine thread replies still scope per-thread under both modes so
multi-person threaded conversations can't collide with unrelated
channel chatter.

### Tests (7 new in ``tests/gateway/test_slack_channel_session_scope.py``)

All drive the real ``SlackAdapter._handle_slack_message`` code path
(not a re-implementation) via the standard pytest fixture pattern
used by ``tests/gateway/test_slack.py``.  Messages @mention the bot
so the mention gate doesn't drop them — the tests are specifically
about what happens once the handler decides to emit a ``MessageEvent``.

* ``TestChannelSessionScopeDefault`` (2 cases):
  - Explicit ``reply_in_thread: true`` keeps ``thread_id = ts``
    (legacy behaviour — regression guard)
  - Unset config behaves like ``reply_in_thread: true`` (pins the
    default)
* ``TestChannelSessionScopeShared`` (3 cases):
  - ``reply_in_thread: false`` + top-level → ``thread_id is None``
    (the NousResearch#15421 bug 1 fix)
  - ``reply_to_message_id is None`` in the same case (no threaded
    outbound reply)
  - Genuine thread reply still scopes per-thread when shared mode is
    on — only TOP-LEVEL messages collapse to the channel session
* ``TestThreadReplyAlwaysScopesByThread`` (2 parametrised cases):
  - Thread replies get ``thread_id = event.thread_ts`` regardless of
    ``reply_in_thread`` — critical invariant for multi-thread
    channels; a regression here would leak per-thread context across
    threads

**Regression guard verified**: reverted the else-branch to the legacy
``thread_ts = event.get("thread_ts") or ts`` one-liner;
``test_top_level_maps_to_none_when_reply_in_thread_false`` correctly
failed (asserts ``thread_id is None`` but got ``"1700000000.000003"``).
Restored → 182 slack tests pass (175 existing + 7 new).

Scope: this fixes NousResearch#15421 bug 1 only.  Bug 2 (sessions.json not
persisting across compression) lives elsewhere in the session
manager and is left for a separate diff.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 25, 2026 01:51

Copilot AI 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.

Pull request overview

Fixes Slack channel session scoping so that when reply_in_thread: false, top-level channel messages share a single channel-scoped session (instead of each message creating an isolated session keyed by its own ts).

Changes:

  • Update SlackAdapter._handle_slack_message to set thread_id=None for top-level channel messages when reply_in_thread=false, while preserving per-thread scoping for real thread replies.
  • Add targeted regression tests covering default vs shared channel session behavior and thread-reply invariants.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
gateway/platforms/slack.py Adjusts channel thread_ts derivation to enable shared channel sessions when reply_in_thread=false.
tests/gateway/test_slack_channel_session_scope.py Adds regression tests validating session keying (thread_id) and reply threading behavior across modes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread gateway/platforms/slack.py Outdated
# across the whole channel so context accumulates across
# messages (#15421 bug 1)
event_thread_ts_raw = event.get("thread_ts")
if event_thread_ts_raw:

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

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

Channel scoping currently treats any truthy event.get("thread_ts") as a “genuine thread reply”, but later in this method is_thread_reply is defined as event_thread_ts and event_thread_ts != ts. If Slack ever includes thread_ts == ts on a thread root (or in some payload variants), this branch would incorrectly keep thread_ts=ts even when reply_in_thread=false, defeating the shared-channel session scoping. Consider aligning the condition here with the same thread_ts != ts invariant used below (i.e., only treat it as a real thread when event_thread_ts_raw is present and differs from ts).

Suggested change
if event_thread_ts_raw:
if event_thread_ts_raw and event_thread_ts_raw != ts:

Copilot uses AI. Check for mistakes.
Comment on lines +160 to +169
"""The outbound-side ``reply_to_message_id`` check already
uses ``thread_ts != ts`` to decide whether to thread the
response. When ``thread_ts`` is None, the check evaluates
``None != ts`` → True → reply_to_message_id IS set. That would
thread the reply, which is the opposite of what
reply_in_thread=false means for top-level messages.

The fix ensures reply_to_message_id is None for top-level
messages in shared-session mode so the bot posts a fresh
channel message (not a threaded reply).

Copilot AI Apr 25, 2026

Copy link

Choose a reason for hiding this comment

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

This docstring claims that when thread_ts is None, the thread_ts != ts check results in reply_to_message_id being set and the reply being threaded. In the production code (reply_to_message_id = thread_ts if thread_ts != ts else None), None != ts is True so the expression evaluates to thread_ts (i.e., None), meaning the reply remains unthreaded. Please update the docstring to reflect the actual behavior/invariant being tested.

Suggested change
"""The outbound-side ``reply_to_message_id`` check already
uses ``thread_ts != ts`` to decide whether to thread the
response. When ``thread_ts`` is None, the check evaluates
``None != ts``Truereply_to_message_id IS set. That would
thread the reply, which is the opposite of what
reply_in_thread=false means for top-level messages.
The fix ensures reply_to_message_id is None for top-level
messages in shared-session mode so the bot posts a fresh
channel message (not a threaded reply).
"""In shared-session mode, top-level channel messages are
normalized to ``thread_ts = None``. With the production logic
``reply_to_message_id = thread_ts if thread_ts != ts else None``,
that still yields ``reply_to_message_id = None`` for top-level
messages, so the bot posts an unthreaded channel message.
This regression test locks in that invariant for
``reply_in_thread=false``.

Copilot uses AI. Check for mistakes.
…ilot NousResearch#15464)

Two findings from Copilot's review on NousResearch#15464, both addressed:

1. ``event.get("thread_ts")`` truthy vs
   ``event_thread_ts != ts``: the new channel branch treated ANY
   truthy ``thread_ts`` as a real thread reply, but three lines below
   ``is_thread_reply`` is defined with the stricter
   ``event_thread_ts and event_thread_ts != ts`` invariant.  If Slack
   ever ships a payload where ``thread_ts == ts`` on a thread root,
   the stricter check would treat it as a top-level message for the
   ``is_thread_reply`` path but as a thread reply for session keying
   — divergent behaviour.  Aligned this branch to the same
   ``and event_thread_ts_raw != ts`` invariant.

2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring
   had the ternary logic backwards ("None != ts → reply_to_message_id
   IS set").  The code reads
   ``reply_to_message_id = thread_ts if thread_ts != ts else None`` —
   with ``thread_ts = None``, the condition is True so the expression
   evaluates to ``thread_ts`` itself (None), meaning the reply stays
   un-threaded.  The test asserted the correct end-state; only the
   explanatory docstring was wrong.  Rewrote the docstring to match
   the actual code flow, with the note that Copilot caught the
   reversal.

7/7 tests still pass.  No behaviour change for the existing
test_thread_reply_scopes_by_thread_even_when_shared case because
``event_thread_ts_raw = "1700000000.000000"`` and ``ts =
"1700000000.000005"`` are distinct — the new
``!= ts`` guard is a no-op there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@briandevans

Copy link
Copy Markdown
Contributor Author

Thanks @copilot — both findings are real. Pushed d53003d6:

1. thread_ts invariant alignment

You're right that my branch treated any truthy event.get("thread_ts") as a real thread reply, while three lines below is_thread_reply uses the stricter event_thread_ts and event_thread_ts != ts. If Slack ever ships a payload where thread_ts == ts on a thread root, the two paths would diverge — session keying would treat it as a real thread while mention-gating would treat it as top-level. Aligned the new branch to the same and event_thread_ts_raw != ts invariant.

2. Docstring logic reversed

Caught me. The docstring claimed "None != ts → reply_to_message_id IS set" but the production code reads reply_to_message_id = thread_ts if thread_ts != ts else None — with thread_ts = None, the condition IS True, so the expression evaluates to thread_ts itself (None), and the reply stays un-threaded. The test itself asserted the correct end-state; only my explanation was wrong. Rewrote the docstring with the full ternary evaluation spelt out so future readers don't trip on the same confusion.

7/7 tests still pass. No behaviour change for test_thread_reply_scopes_by_thread_even_when_shared since event_thread_ts = "1700000000.000000" and ts = "1700000000.000005" are distinct — the new != ts guard is a no-op there.

Meta: these two findings are exactly the class of review I was hoping to reduce with the pre-push checklist I've been building. (1) needed a "check for semantic invariants defined elsewhere in the same function and align with them" rule I hadn't articulated — adding it now. (2) was a docstring-vs-code mismatch, which IS on my checklist but I missed because I was focused on the assertion being correct rather than tracing the explanatory text. Good signal for where the discipline is still weak.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/slack Slack app adapter labels Apr 25, 2026
@briandevans

Copy link
Copy Markdown
Contributor Author

Closing — superseded by 4b5a88d71 on main (fix(slack): honor reply_in_thread=false for top-level channel messages, closing #9268). The maintainer's patch addresses the same reply_in_thread=false reporting via a different surface — _resolve_thread_ts adds a reply_to == thread_id tiebreaker so synthetic-thread top-level messages route their reply to the channel instead of opening a new thread.

That's the OUT-side fix; mine was an IN-side fix in _handle_slack_message that cleared thread_id on the inbound event so the session itself was channel-scoped. The OUT-side approach is the safer migration path because it preserves Slack's natural per-message session keying for top-level messages — my approach would have changed session keys for any existing top-level-message session on upgrade.

@alt-glitch had also flagged the parent issue #15421 as a duplicate of #9268, and the reporter accepted that verdict. Closing here so the queue isn't carrying redundant work.

@briandevans briandevans closed this May 3, 2026
teknium1 pushed a commit that referenced this pull request Jun 8, 2026
…ilot #15464)

Two findings from Copilot's review on #15464, both addressed:

1. ``event.get("thread_ts")`` truthy vs
   ``event_thread_ts != ts``: the new channel branch treated ANY
   truthy ``thread_ts`` as a real thread reply, but three lines below
   ``is_thread_reply`` is defined with the stricter
   ``event_thread_ts and event_thread_ts != ts`` invariant.  If Slack
   ever ships a payload where ``thread_ts == ts`` on a thread root,
   the stricter check would treat it as a top-level message for the
   ``is_thread_reply`` path but as a thread reply for session keying
   — divergent behaviour.  Aligned this branch to the same
   ``and event_thread_ts_raw != ts`` invariant.

2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring
   had the ternary logic backwards ("None != ts → reply_to_message_id
   IS set").  The code reads
   ``reply_to_message_id = thread_ts if thread_ts != ts else None`` —
   with ``thread_ts = None``, the condition is True so the expression
   evaluates to ``thread_ts`` itself (None), meaning the reply stays
   un-threaded.  The test asserted the correct end-state; only the
   explanatory docstring was wrong.  Rewrote the docstring to match
   the actual code flow, with the note that Copilot caught the
   reversal.

7/7 tests still pass.  No behaviour change for the existing
test_thread_reply_scopes_by_thread_even_when_shared case because
``event_thread_ts_raw = "1700000000.000000"`` and ``ts =
"1700000000.000005"`` are distinct — the new
``!= ts`` guard is a no-op there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@teknium1

teknium1 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Merged via #41703 — your two commits were cherry-picked onto current main with your authorship preserved (133e0271e + ab0a6270c).

Salvage notes:

  • Both commits applied cleanly despite ~5000 commits of drift; the channel branch landed exactly where intended.
  • Added an E2E check through the real build_session_key() confirming two top-level messages collapse to one channel session while thread replies stay isolated.
  • Confirmed the issue's "Bug 2" (empty sessions.json) was a symptom of this same root cause, not a separate persistence defect — a stable channel key makes _save() persist and re-resolve correctly across restarts.

Thanks for the fix and the thorough test coverage.

changman pushed a commit to changman/hermes-agent that referenced this pull request Jun 10, 2026
…ilot NousResearch#15464)

Two findings from Copilot's review on NousResearch#15464, both addressed:

1. ``event.get("thread_ts")`` truthy vs
   ``event_thread_ts != ts``: the new channel branch treated ANY
   truthy ``thread_ts`` as a real thread reply, but three lines below
   ``is_thread_reply`` is defined with the stricter
   ``event_thread_ts and event_thread_ts != ts`` invariant.  If Slack
   ever ships a payload where ``thread_ts == ts`` on a thread root,
   the stricter check would treat it as a top-level message for the
   ``is_thread_reply`` path but as a thread reply for session keying
   — divergent behaviour.  Aligned this branch to the same
   ``and event_thread_ts_raw != ts`` invariant.

2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring
   had the ternary logic backwards ("None != ts → reply_to_message_id
   IS set").  The code reads
   ``reply_to_message_id = thread_ts if thread_ts != ts else None`` —
   with ``thread_ts = None``, the condition is True so the expression
   evaluates to ``thread_ts`` itself (None), meaning the reply stays
   un-threaded.  The test asserted the correct end-state; only the
   explanatory docstring was wrong.  Rewrote the docstring to match
   the actual code flow, with the note that Copilot caught the
   reversal.

7/7 tests still pass.  No behaviour change for the existing
test_thread_reply_scopes_by_thread_even_when_shared case because
``event_thread_ts_raw = "1700000000.000000"`` and ``ts =
"1700000000.000005"`` are distinct — the new
``!= ts`` guard is a no-op there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alt-glitch pushed a commit that referenced this pull request Jun 14, 2026
…ilot #15464)

Two findings from Copilot's review on #15464, both addressed:

1. ``event.get("thread_ts")`` truthy vs
   ``event_thread_ts != ts``: the new channel branch treated ANY
   truthy ``thread_ts`` as a real thread reply, but three lines below
   ``is_thread_reply`` is defined with the stricter
   ``event_thread_ts and event_thread_ts != ts`` invariant.  If Slack
   ever ships a payload where ``thread_ts == ts`` on a thread root,
   the stricter check would treat it as a top-level message for the
   ``is_thread_reply`` path but as a thread reply for session keying
   — divergent behaviour.  Aligned this branch to the same
   ``and event_thread_ts_raw != ts`` invariant.

2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring
   had the ternary logic backwards ("None != ts → reply_to_message_id
   IS set").  The code reads
   ``reply_to_message_id = thread_ts if thread_ts != ts else None`` —
   with ``thread_ts = None``, the condition is True so the expression
   evaluates to ``thread_ts`` itself (None), meaning the reply stays
   un-threaded.  The test asserted the correct end-state; only the
   explanatory docstring was wrong.  Rewrote the docstring to match
   the actual code flow, with the note that Copilot caught the
   reversal.

7/7 tests still pass.  No behaviour change for the existing
test_thread_reply_scopes_by_thread_even_when_shared case because
``event_thread_ts_raw = "1700000000.000000"`` and ``ts =
"1700000000.000005"`` are distinct — the new
``!= ts`` guard is a no-op there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
davidgut1982 pushed a commit to davidgut1982/hermes-agent that referenced this pull request Jun 17, 2026
…ilot NousResearch#15464)

Two findings from Copilot's review on NousResearch#15464, both addressed:

1. ``event.get("thread_ts")`` truthy vs
   ``event_thread_ts != ts``: the new channel branch treated ANY
   truthy ``thread_ts`` as a real thread reply, but three lines below
   ``is_thread_reply`` is defined with the stricter
   ``event_thread_ts and event_thread_ts != ts`` invariant.  If Slack
   ever ships a payload where ``thread_ts == ts`` on a thread root,
   the stricter check would treat it as a top-level message for the
   ``is_thread_reply`` path but as a thread reply for session keying
   — divergent behaviour.  Aligned this branch to the same
   ``and event_thread_ts_raw != ts`` invariant.

2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring
   had the ternary logic backwards ("None != ts → reply_to_message_id
   IS set").  The code reads
   ``reply_to_message_id = thread_ts if thread_ts != ts else None`` —
   with ``thread_ts = None``, the condition is True so the expression
   evaluates to ``thread_ts`` itself (None), meaning the reply stays
   un-threaded.  The test asserted the correct end-state; only the
   explanatory docstring was wrong.  Rewrote the docstring to match
   the actual code flow, with the note that Copilot caught the
   reversal.

7/7 tests still pass.  No behaviour change for the existing
test_thread_reply_scopes_by_thread_even_when_shared case because
``event_thread_ts_raw = "1700000000.000000"`` and ``ts =
"1700000000.000005"`` are distinct — the new
``!= ts`` guard is a no-op there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
T02200059 pushed a commit to T02200059/hermes-agent that referenced this pull request Jun 18, 2026
…ilot NousResearch#15464)

Two findings from Copilot's review on NousResearch#15464, both addressed:

1. ``event.get("thread_ts")`` truthy vs
   ``event_thread_ts != ts``: the new channel branch treated ANY
   truthy ``thread_ts`` as a real thread reply, but three lines below
   ``is_thread_reply`` is defined with the stricter
   ``event_thread_ts and event_thread_ts != ts`` invariant.  If Slack
   ever ships a payload where ``thread_ts == ts`` on a thread root,
   the stricter check would treat it as a top-level message for the
   ``is_thread_reply`` path but as a thread reply for session keying
   — divergent behaviour.  Aligned this branch to the same
   ``and event_thread_ts_raw != ts`` invariant.

2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring
   had the ternary logic backwards ("None != ts → reply_to_message_id
   IS set").  The code reads
   ``reply_to_message_id = thread_ts if thread_ts != ts else None`` —
   with ``thread_ts = None``, the condition is True so the expression
   evaluates to ``thread_ts`` itself (None), meaning the reply stays
   un-threaded.  The test asserted the correct end-state; only the
   explanatory docstring was wrong.  Rewrote the docstring to match
   the actual code flow, with the note that Copilot caught the
   reversal.

7/7 tests still pass.  No behaviour change for the existing
test_thread_reply_scopes_by_thread_even_when_shared case because
``event_thread_ts_raw = "1700000000.000000"`` and ``ts =
"1700000000.000005"`` are distinct — the new
``!= ts`` guard is a no-op there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
waefrebeorn pushed a commit to waefrebeorn/slermes that referenced this pull request Jul 2, 2026
…ilot NousResearch#15464)

Two findings from Copilot's review on NousResearch#15464, both addressed:

1. ``event.get("thread_ts")`` truthy vs
   ``event_thread_ts != ts``: the new channel branch treated ANY
   truthy ``thread_ts`` as a real thread reply, but three lines below
   ``is_thread_reply`` is defined with the stricter
   ``event_thread_ts and event_thread_ts != ts`` invariant.  If Slack
   ever ships a payload where ``thread_ts == ts`` on a thread root,
   the stricter check would treat it as a top-level message for the
   ``is_thread_reply`` path but as a thread reply for session keying
   — divergent behaviour.  Aligned this branch to the same
   ``and event_thread_ts_raw != ts`` invariant.

2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring
   had the ternary logic backwards ("None != ts → reply_to_message_id
   IS set").  The code reads
   ``reply_to_message_id = thread_ts if thread_ts != ts else None`` —
   with ``thread_ts = None``, the condition is True so the expression
   evaluates to ``thread_ts`` itself (None), meaning the reply stays
   un-threaded.  The test asserted the correct end-state; only the
   explanatory docstring was wrong.  Rewrote the docstring to match
   the actual code flow, with the note that Copilot caught the
   reversal.

7/7 tests still pass.  No behaviour change for the existing
test_thread_reply_scopes_by_thread_even_when_shared case because
``event_thread_ts_raw = "1700000000.000000"`` and ``ts =
"1700000000.000005"`` are distinct — the new
``!= ts`` guard is a no-op there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
santhreal pushed a commit to santhreal/hermes-agent that referenced this pull request Jul 13, 2026
…ilot NousResearch#15464)

Two findings from Copilot's review on NousResearch#15464, both addressed:

1. ``event.get("thread_ts")`` truthy vs
   ``event_thread_ts != ts``: the new channel branch treated ANY
   truthy ``thread_ts`` as a real thread reply, but three lines below
   ``is_thread_reply`` is defined with the stricter
   ``event_thread_ts and event_thread_ts != ts`` invariant.  If Slack
   ever ships a payload where ``thread_ts == ts`` on a thread root,
   the stricter check would treat it as a top-level message for the
   ``is_thread_reply`` path but as a thread reply for session keying
   — divergent behaviour.  Aligned this branch to the same
   ``and event_thread_ts_raw != ts`` invariant.

2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring
   had the ternary logic backwards ("None != ts → reply_to_message_id
   IS set").  The code reads
   ``reply_to_message_id = thread_ts if thread_ts != ts else None`` —
   with ``thread_ts = None``, the condition is True so the expression
   evaluates to ``thread_ts`` itself (None), meaning the reply stays
   un-threaded.  The test asserted the correct end-state; only the
   explanatory docstring was wrong.  Rewrote the docstring to match
   the actual code flow, with the note that Copilot caught the
   reversal.

7/7 tests still pass.  No behaviour change for the existing
test_thread_reply_scopes_by_thread_even_when_shared case because
``event_thread_ts_raw = "1700000000.000000"`` and ``ts =
"1700000000.000005"`` are distinct — the new
``!= ts`` guard is a no-op there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
donbowman pushed a commit to donbowman/hermes-agent that referenced this pull request Jul 13, 2026
…ilot NousResearch#15464)

Two findings from Copilot's review on NousResearch#15464, both addressed:

1. ``event.get("thread_ts")`` truthy vs
   ``event_thread_ts != ts``: the new channel branch treated ANY
   truthy ``thread_ts`` as a real thread reply, but three lines below
   ``is_thread_reply`` is defined with the stricter
   ``event_thread_ts and event_thread_ts != ts`` invariant.  If Slack
   ever ships a payload where ``thread_ts == ts`` on a thread root,
   the stricter check would treat it as a top-level message for the
   ``is_thread_reply`` path but as a thread reply for session keying
   — divergent behaviour.  Aligned this branch to the same
   ``and event_thread_ts_raw != ts`` invariant.

2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring
   had the ternary logic backwards ("None != ts → reply_to_message_id
   IS set").  The code reads
   ``reply_to_message_id = thread_ts if thread_ts != ts else None`` —
   with ``thread_ts = None``, the condition is True so the expression
   evaluates to ``thread_ts`` itself (None), meaning the reply stays
   un-threaded.  The test asserted the correct end-state; only the
   explanatory docstring was wrong.  Rewrote the docstring to match
   the actual code flow, with the note that Copilot caught the
   reversal.

7/7 tests still pass.  No behaviour change for the existing
test_thread_reply_scopes_by_thread_even_when_shared case because
``event_thread_ts_raw = "1700000000.000000"`` and ``ts =
"1700000000.000005"`` are distinct — the new
``!= ts`` guard is a no-op there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
…ilot NousResearch#15464)

Two findings from Copilot's review on NousResearch#15464, both addressed:

1. ``event.get("thread_ts")`` truthy vs
   ``event_thread_ts != ts``: the new channel branch treated ANY
   truthy ``thread_ts`` as a real thread reply, but three lines below
   ``is_thread_reply`` is defined with the stricter
   ``event_thread_ts and event_thread_ts != ts`` invariant.  If Slack
   ever ships a payload where ``thread_ts == ts`` on a thread root,
   the stricter check would treat it as a top-level message for the
   ``is_thread_reply`` path but as a thread reply for session keying
   — divergent behaviour.  Aligned this branch to the same
   ``and event_thread_ts_raw != ts`` invariant.

2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring
   had the ternary logic backwards ("None != ts → reply_to_message_id
   IS set").  The code reads
   ``reply_to_message_id = thread_ts if thread_ts != ts else None`` —
   with ``thread_ts = None``, the condition is True so the expression
   evaluates to ``thread_ts`` itself (None), meaning the reply stays
   un-threaded.  The test asserted the correct end-state; only the
   explanatory docstring was wrong.  Rewrote the docstring to match
   the actual code flow, with the note that Copilot caught the
   reversal.

7/7 tests still pass.  No behaviour change for the existing
test_thread_reply_scopes_by_thread_even_when_shared case because
``event_thread_ts_raw = "1700000000.000000"`` and ``ts =
"1700000000.000005"`` are distinct — the new
``!= ts`` guard is a no-op there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
benbarclay added a commit that referenced this pull request Jul 27, 2026
…es + corpora

Completes the four-platform ingress mirror (telegram/whatsapp landed in the
prior commit). Layer model (documented in discord_parse.py): layer 1
(payload→SDK object — discord.py/Bolt) is the SDK-equivalence axiom, not
oracled; layer 2 (SDK-view → MessageEvent fields — the Hermes-unique
rules) is THE extracted, vectored spec; layer 3 (effects) stays adapter-side.

- plugins/platforms/discord/discord_parse.py: SDK-view IR
  (DiscordMessageView, dual access: discord.py objects OR raw-vocabulary
  dicts) + pure rules — chat-type classification, <@id>/<@!id> mention
  stripping (strip THEN command-detect), forwarded-snapshot folding,
  referenced-attachment inheritance, attachment→type classification
  (voice-note vs audio via is_voice_message/duration+waveform),
  guild/forum thread naming. Adapter delegates: _is_discord_voice_message_
  attachment, _format_thread_chat_name, and the _handle_message
  classification block.
- plugins/platforms/slack/slack_parse.py: DM/MPIM classification (1:1 vs
  shared-surface MPIM), thread_ts session scoping (#15421/#15464
  invariants incl. the thread_ts==ts root shape), mention detection,
  bot-message classification. Adapter delegates the DM-classification and
  channel-scoping blocks.
- scripts/generate_ingress_vectors.py: +15 discord + 12 slack vectors
  (54 total across four platforms).
- tests/conformance/test_ingress_vectors.py: +3 oracle-fidelity tests
  (adapter shim ≡ core on SDK-like objects) + scar-rule coverage; 18 total.

Suites: conformance 18; discord/slack gateway suites green (the only
failures in the -k 'discord or slack' sweep are pre-existing
order-dependent pollution — reproduce identically with this diff stashed).
leewenjie pushed a commit to leewenjie/hermes-agent that referenced this pull request Aug 7, 2026
…ilot NousResearch#15464)

Two findings from Copilot's review on NousResearch#15464, both addressed:

1. ``event.get("thread_ts")`` truthy vs
   ``event_thread_ts != ts``: the new channel branch treated ANY
   truthy ``thread_ts`` as a real thread reply, but three lines below
   ``is_thread_reply`` is defined with the stricter
   ``event_thread_ts and event_thread_ts != ts`` invariant.  If Slack
   ever ships a payload where ``thread_ts == ts`` on a thread root,
   the stricter check would treat it as a top-level message for the
   ``is_thread_reply`` path but as a thread reply for session keying
   — divergent behaviour.  Aligned this branch to the same
   ``and event_thread_ts_raw != ts`` invariant.

2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring
   had the ternary logic backwards ("None != ts → reply_to_message_id
   IS set").  The code reads
   ``reply_to_message_id = thread_ts if thread_ts != ts else None`` —
   with ``thread_ts = None``, the condition is True so the expression
   evaluates to ``thread_ts`` itself (None), meaning the reply stays
   un-threaded.  The test asserted the correct end-state; only the
   explanatory docstring was wrong.  Rewrote the docstring to match
   the actual code flow, with the note that Copilot caught the
   reversal.

7/7 tests still pass.  No behaviour change for the existing
test_thread_reply_scopes_by_thread_even_when_shared case because
``event_thread_ts_raw = "1700000000.000000"`` and ``ts =
"1700000000.000005"`` are distinct — the new
``!= ts`` guard is a no-op there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 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.

Slack: top-level messages create isolated sessions; sessions.json not persisting

4 participants