🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🟡 Slack thread replies missing thread_ts can fall back to channel-scoped session when replyToMode=off (context bleed)

Description

In resolveSlackRoutingContext, the new roomThreadId/canonicalThreadId selection can produce no thread id for roomish chats when replyToMode === "off".

This becomes security-relevant when Slack delivers a semantic thread reply that is missing thread_ts (observed/handled elsewhere via best-effort recovery):

• Some Slack events can include parent_user_id but omit thread_ts (see createSlackThreadTsResolver, which explicitly tries to recover missing thread_ts via conversations.history and may fail).
• When recovery fails, resolveSlackThreadContext yields incomingThreadTs = undefined and isThreadReply = false.
• With the new logic, for roomish messages and replyToMode="off", roomThreadId becomes undefined, so canonicalThreadId becomes undefined.
• resolveThreadSessionKeys treats an empty/undefined thread id as no thread scoping, returning the base (channel-level) route.sessionKey.

Impact:

• A thread reply that is missing thread_ts can be processed in the shared top-level channel session, enabling cross-thread context bleed (the agent may use context from other threads/channel turns when responding).
• Combined with the existing reply-targeting behavior (thread targeting also depends on incomingThreadTs), this situation can also increase the likelihood of replying in the wrong place (main channel vs. thread), potentially exposing thread content more broadly.

Vulnerable code (new logic):

const roomThreadId =
  isThreadReply && threadTs
    ? threadTs
    : replyToMode === "off"
      ? undefined
      : threadContext.messageTs;
const canonicalThreadId = isRoomish
  ? roomThreadId
  : isThreadReply
    ? threadTs
    : autoThreadId;

Recommendation

Harden thread/session scoping for the known Slack edge case parent_user_id present but thread_ts missing.

Options (pick one consistent with desired UX):

1. Treat parent_user_id as an indicator of a thread reply even when thread_ts is missing, and keep such messages isolated from the channel session (e.g., scope to message.ts as a temporary thread id):

const maybeThreadReply = Boolean(message.parent_user_id) && !threadTs;

const roomThreadId =
  isThreadReply && threadTs
    ? threadTs
    : maybeThreadReply
      ? threadContext.messageTs // isolate instead of channel-scoping
      : replyToMode === "off"
        ? undefined
        : threadContext.messageTs;

2. Alternatively, update resolveSlackThreadContext to classify parent_user_id-only events as thread replies (and ensure the thread-ts resolver always runs before routing).

In all cases, add a regression test that simulates a roomish message with parent_user_id set, thread_ts missing, and replyToMode="off", asserting the resulting SessionKey is not channel-scoped (or that the message is dropped/retried until thread_ts is resolved).

Analyzed PR: #32193 at commit 4ebed6d

_{Last updated on: 2026-03-02T22:40:30Z}

Greptile Summary

This PR fixes a bug where Slack top-level channel messages were always assigned per-message :thread:<ts> session keys even with replyToMode=off (the default), causing the agent to lose conversational context across normal channel messages. The fix introduces a roomThreadId variable that returns undefined for non-thread room turns when replyToMode=off, allowing them to share the channel-level base session key.

Key changes:

• prepare.ts: Replaces the single incomingThreadTs ?? message.ts fallback for room messages with a two-step logic: thread replies → threadTs, top-level with replyToMode=off → undefined (channel-scoped), top-level with any other mode → messageTs (per-message-scoped, unchanged).
• prepare.thread-session-key.test.ts: The previous test was asserting the buggy behavior (:thread: in session key for all channel messages). It is replaced with four explicit, correctly-labeled scenarios covering off/all modes and the thread-reply/DM edge cases. replyToMode is now passed consistently to both buildCtx and createSlackTestAccount, resolving a subtle prior mismatch where the context had "all" but the account defaulted to "off".

The replyToMode="first" path in rooms is functionally unchanged (still produces a per-message session via threadContext.messageTs), though no dedicated test was added for that mode — it would be a minor improvement to cover it explicitly.

Confidence Score: 5/5

• This PR is safe to merge — the change is minimal, well-scoped, and fully covered by targeted tests.
• The logic change is isolated to three lines of session-key derivation inside prepareSlackMessage, with no impact on outbound routing, non-Slack channels, or any auth/storage paths. The before/after behavior for thread replies, replyToMode=all, replyToMode=first, and DMs is provably unchanged by the code analysis. New tests directly verify the corrected and preserved behaviors. The only minor gap is no explicit test for replyToMode="first" in rooms, but the logic is trivially equivalent to the pre-PR behavior.
• No files require special attention.

_{Last reviewed commit: b0e4203}

Landed via temp rebase onto main.

• Gate: pnpm test src/slack/monitor/message-handler/prepare.thread-session-key.test.ts src/slack/monitor/message-handler/prepare.test.ts
• Land commit: LAND_SHA_PLACEHOLDER
• Merge commit: MERGE_SHA_PLACEHOLDER

Thanks @bmendonca3!

Landed via temp rebase onto main.

• Gate: pnpm test src/slack/monitor/message-handler/prepare.thread-session-key.test.ts src/slack/monitor/message-handler/prepare.test.ts
• Land commit: 4ebed6d
• Merge commit: 2c39731

Thanks @bmendonca3!