Skip to content

Commit 4845c5c

Browse files
Fix: [AEA-5868] - stops bots replying to threads where they were not mentioned (#199)
## Summary - Routine Change ### Details The bot was replying to threads it wasn’t invited to - we now check thread history via Slack API to confirm if this bot was mentioned in a thread before responding
1 parent bb67dc3 commit 4845c5c

File tree

4 files changed

+329
-7
lines changed

4 files changed

+329
-7
lines changed

.github/workflows/run_regression_tests.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,8 @@ jobs:
7373
GITHUB-TOKEN: ${{ steps.generate-token.outputs.token }}
7474
run: |
7575
if [[ "$TARGET_ENVIRONMENT" != "prod" && "$TARGET_ENVIRONMENT" != "ref" ]]; then
76-
REGRESSION_TEST_REPO_TAG="v3.5.0" # This is the tag or branch of the regression test code to run, usually a version tag like v3.1.0 or a branch name
77-
REGRESSION_TEST_WORKFLOW_TAG="v3.5.0" # This is the tag of the github workflow to run, usually the same as REGRESSION_TEST_REPO_TAG
76+
REGRESSION_TEST_REPO_TAG="v3.5.16" # This is the tag or branch of the regression test code to run, usually a version tag like v3.1.0 or a branch name
77+
REGRESSION_TEST_WORKFLOW_TAG="v3.5.16" # This is the tag of the github workflow to run, usually the same as REGRESSION_TEST_REPO_TAG
7878
7979
if [[ -z "$REGRESSION_TEST_REPO_TAG" || -z "$REGRESSION_TEST_WORKFLOW_TAG" ]]; then
8080
echo "Error: One or both tag variables are not set" >&2

packages/slackBotFunction/app/slack/slack_handlers.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ def setup_handlers(app: App) -> None:
5050

5151
# ack function for events where we respond with eyes
5252
def respond_to_events(event: Dict[str, Any], ack: Ack, client: WebClient):
53-
if should_reply_to_message(event):
53+
if should_reply_to_message(event, client):
5454
respond_with_eyes(event=event, client=client)
5555
logger.debug("Sending ack response")
5656
ack()
@@ -103,9 +103,9 @@ def unified_message_handler(client: WebClient, event: Dict[str, Any], body: Dict
103103
# and its a message
104104
# and its not in a thread
105105
# then ignore it as it will be handled as an app_mention event
106-
if not should_reply_to_message(event):
107-
logger.debug("Ignoring message in group chat not in a thread", extra={"event": event})
108-
# ignore messages in group chats
106+
if not should_reply_to_message(event, client):
107+
logger.debug("Ignoring message in group chat not in a thread or bot not in thread", extra={"event": event})
108+
# ignore messages in group chats or threads where bot wasn't mentioned
109109
return
110110
user_id = event.get("user", "unknown")
111111
conversation_key, _ = conversation_key_and_root(event=event)

packages/slackBotFunction/app/utils/handler_utils.py

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,15 +211,107 @@ def extract_session_pull_request_id(conversation_key: str) -> str | None:
211211
return None
212212

213213

214-
def should_reply_to_message(event: Dict[str, Any]) -> bool:
214+
def get_bot_user_id(client: WebClient) -> str | None:
215+
"""
216+
Get the bot's user ID using auth.test API.
217+
This is cached to avoid repeated API calls.
218+
"""
219+
if not hasattr(get_bot_user_id, "_cache"):
220+
get_bot_user_id._cache = {}
221+
222+
cache_key = id(client)
223+
224+
if cache_key in get_bot_user_id._cache:
225+
return get_bot_user_id._cache[cache_key]
226+
227+
try:
228+
auth_response = client.auth_test()
229+
if auth_response.get("ok"):
230+
bot_user_id = auth_response.get("user_id")
231+
get_bot_user_id._cache[cache_key] = bot_user_id
232+
logger.info("Cached bot user ID", extra={"bot_user_id": bot_user_id})
233+
return bot_user_id
234+
except Exception:
235+
logger.error("Error fetching bot user ID", extra={"error": traceback.format_exc()})
236+
237+
return None
238+
239+
240+
def was_bot_mentioned_in_thread_root(channel: str, thread_ts: str, client: WebClient) -> bool:
241+
"""
242+
Check if THIS specific bot was mentioned anywhere in the thread history.
243+
This handles cases where:
244+
- Multiple bots are in the same channel (checks for this bot's specific user ID)
245+
- Bot is mentioned later in a thread (not just the root message)
246+
"""
247+
try:
248+
# get this bot's user ID
249+
bot_user_id = get_bot_user_id(client)
250+
if not bot_user_id:
251+
logger.warning("Could not determine bot user ID, failing open")
252+
return True
253+
254+
response = client.conversations_replies(channel=channel, ts=thread_ts, inclusive=True)
255+
256+
if not response.get("ok") or not response.get("messages"):
257+
logger.warning("Failed to fetch thread messages", extra={"channel": channel, "thread_ts": thread_ts})
258+
return True
259+
260+
# check if THIS bot is mentioned in any message in the thread
261+
bot_mention_pattern = rf"<@{re.escape(bot_user_id)}(?:\|[^>]+)?>"
262+
263+
for message in response["messages"]:
264+
message_text = message.get("text", "")
265+
if re.search(bot_mention_pattern, message_text):
266+
logger.debug(
267+
"Found bot mention in thread",
268+
extra={
269+
"channel": channel,
270+
"thread_ts": thread_ts,
271+
"bot_user_id": bot_user_id,
272+
"message_ts": message.get("ts"),
273+
},
274+
)
275+
return True
276+
277+
logger.debug(
278+
"Bot not mentioned in thread",
279+
extra={"channel": channel, "thread_ts": thread_ts, "bot_user_id": bot_user_id},
280+
)
281+
return False
282+
283+
except Exception:
284+
logger.error("Error checking bot mention in thread", extra={"error": traceback.format_exc()})
285+
return True
286+
287+
288+
def should_reply_to_message(event: Dict[str, Any], client: WebClient = None) -> bool:
215289
"""
216290
Determine if the bot should reply to the message.
217291
218292
Conditions:
219293
should not reply if:
220294
- Message is in a group chat (channel_type == 'group') but not in a thread
295+
- Message is in a channel thread where the bot was not initially mentioned
221296
"""
222297

298+
# we don't reply to non-threaded messages in group chats
223299
if event.get("channel_type") == "group" and event.get("type") == "message" and event.get("thread_ts") is None:
224300
return False
301+
302+
# for channel threads, check if bot was mentioned anywhere in the thread history
303+
if event.get("channel_type") == "channel" and event.get("thread_ts"):
304+
if not client:
305+
logger.warning("No Slack client provided to check thread participation")
306+
return True
307+
308+
channel = event.get("channel")
309+
thread_ts = event.get("thread_ts")
310+
311+
if not was_bot_mentioned_in_thread_root(channel, thread_ts, client):
312+
logger.debug(
313+
"Bot not mentioned in thread, ignoring message", extra={"channel": channel, "thread_ts": thread_ts}
314+
)
315+
return False
316+
225317
return True

packages/slackBotFunction/tests/test_handler_utils.py

Lines changed: 230 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -359,3 +359,233 @@ def test_extract_pull_request_id_extracts_when_there_is_a_mention():
359359
# assertions
360360
assert pr_id == "12345"
361361
assert text == "<@U123> some question"
362+
363+
364+
def test_was_bot_mentioned_in_thread_root_with_mention(mock_env: Mock):
365+
"""test that was_bot_mentioned_in_thread_root returns true when this bot is mentioned"""
366+
mock_client = Mock()
367+
mock_client.auth_test.return_value = {"ok": True, "user_id": "U123ABC"}
368+
mock_client.conversations_replies.return_value = {
369+
"ok": True,
370+
"messages": [{"text": "<@U123ABC> hello bot", "ts": "1234567890.123456"}],
371+
}
372+
373+
if "app.utils.handler_utils" in sys.modules:
374+
del sys.modules["app.utils.handler_utils"]
375+
from app.utils.handler_utils import was_bot_mentioned_in_thread_root
376+
377+
result = was_bot_mentioned_in_thread_root("C123", "1234567890.123456", mock_client)
378+
379+
assert result is True
380+
mock_client.conversations_replies.assert_called_once_with(channel="C123", ts="1234567890.123456", inclusive=True)
381+
382+
383+
def test_was_bot_mentioned_in_thread_root_without_mention(mock_env: Mock):
384+
"""test that was_bot_mentioned_in_thread_root returns false when bot is not mentioned"""
385+
mock_client = Mock()
386+
mock_client.auth_test.return_value = {"ok": True, "user_id": "U123ABC"}
387+
mock_client.conversations_replies.return_value = {
388+
"ok": True,
389+
"messages": [{"text": "hello everyone", "ts": "1234567890.123456"}],
390+
}
391+
392+
if "app.utils.handler_utils" in sys.modules:
393+
del sys.modules["app.utils.handler_utils"]
394+
from app.utils.handler_utils import was_bot_mentioned_in_thread_root
395+
396+
result = was_bot_mentioned_in_thread_root("C123", "1234567890.123456", mock_client)
397+
398+
assert result is False
399+
400+
401+
def test_was_bot_mentioned_in_thread_root_exception(mock_env: Mock):
402+
"""tes that was_bot_mentioned_in_thread_root fails open on exception"""
403+
mock_client = Mock()
404+
mock_client.conversations_replies.side_effect = Exception("API Error")
405+
406+
if "app.utils.handler_utils" in sys.modules:
407+
del sys.modules["app.utils.handler_utils"]
408+
from app.utils.handler_utils import was_bot_mentioned_in_thread_root
409+
410+
result = was_bot_mentioned_in_thread_root("C123", "1234567890.123456", mock_client)
411+
412+
assert result is True
413+
414+
415+
def test_should_reply_to_message_dm(mock_env: Mock):
416+
"""test should_reply_to_message returns True for DMs"""
417+
if "app.utils.handler_utils" in sys.modules:
418+
del sys.modules["app.utils.handler_utils"]
419+
from app.utils.handler_utils import should_reply_to_message
420+
421+
event = {"channel_type": "im", "type": "message", "channel": "D123", "ts": "123"}
422+
result = should_reply_to_message(event)
423+
424+
assert result is True
425+
426+
427+
def test_should_reply_to_message_group_without_thread(mock_env: Mock):
428+
"""test should_reply_to_message returns False for group messages without thread"""
429+
if "app.utils.handler_utils" in sys.modules:
430+
del sys.modules["app.utils.handler_utils"]
431+
from app.utils.handler_utils import should_reply_to_message
432+
433+
event = {"channel_type": "group", "type": "message", "channel": "G123", "ts": "123"}
434+
result = should_reply_to_message(event)
435+
436+
assert result is False
437+
438+
439+
def test_should_reply_to_message_channel_thread_with_bot_mention(mock_env: Mock):
440+
"""test should_reply_to_message returns True for channel thread where bot was mentioned"""
441+
mock_client = Mock()
442+
mock_client.auth_test.return_value = {"ok": True, "user_id": "U123"}
443+
mock_client.conversations_replies.return_value = {
444+
"ok": True,
445+
"messages": [{"text": "<@U123> help me", "ts": "1234567890.123456"}],
446+
}
447+
448+
if "app.utils.handler_utils" in sys.modules:
449+
del sys.modules["app.utils.handler_utils"]
450+
from app.utils.handler_utils import should_reply_to_message
451+
452+
event = {"channel_type": "channel", "type": "message", "channel": "C123", "thread_ts": "1234567890.123456"}
453+
result = should_reply_to_message(event, mock_client)
454+
455+
assert result is True
456+
457+
458+
def test_should_reply_to_message_channel_thread_without_bot_mention(mock_env: Mock):
459+
"""test should_reply_to_message returns False for channel thread where bot was not mentioned"""
460+
mock_client = Mock()
461+
mock_client.auth_test.return_value = {"ok": True, "user_id": "U123"}
462+
mock_client.conversations_replies.return_value = {
463+
"ok": True,
464+
"messages": [{"text": "some discussion", "ts": "1234567890.123456"}],
465+
}
466+
467+
if "app.utils.handler_utils" in sys.modules:
468+
del sys.modules["app.utils.handler_utils"]
469+
from app.utils.handler_utils import should_reply_to_message
470+
471+
event = {"channel_type": "channel", "type": "message", "channel": "C123", "thread_ts": "1234567890.123456"}
472+
result = should_reply_to_message(event, mock_client)
473+
474+
assert result is False
475+
476+
477+
def test_should_reply_to_message_channel_thread_no_client(mock_env: Mock):
478+
"""test should_reply_to_message fails open when no client provided"""
479+
# setup mocks
480+
if "app.utils.handler_utils" in sys.modules:
481+
del sys.modules["app.utils.handler_utils"]
482+
from app.utils.handler_utils import should_reply_to_message
483+
484+
event = {"channel_type": "channel", "type": "message", "channel": "C123", "thread_ts": "1234567890.123456"}
485+
result = should_reply_to_message(event, None)
486+
487+
assert result is True
488+
489+
490+
def test_was_bot_mentioned_in_thread_different_bot(mock_env: Mock):
491+
"""test that was_bot_mentioned_in_thread_root returns false when a different bot is mentioned"""
492+
mock_client = Mock()
493+
mock_client.auth_test.return_value = {"ok": True, "user_id": "U123ABC"}
494+
mock_client.conversations_replies.return_value = {
495+
"ok": True,
496+
"messages": [{"text": "<@U999XYZ> hello other bot", "ts": "1234567890.123456"}], # Different bot
497+
}
498+
499+
if "app.utils.handler_utils" in sys.modules:
500+
del sys.modules["app.utils.handler_utils"]
501+
from app.utils.handler_utils import was_bot_mentioned_in_thread_root
502+
503+
result = was_bot_mentioned_in_thread_root("C123", "1234567890.123456", mock_client)
504+
505+
assert result is False
506+
507+
508+
def test_was_bot_mentioned_later_in_thread(mock_env: Mock):
509+
"""test that bot mention later in thread is detected"""
510+
mock_client = Mock()
511+
mock_client.auth_test.return_value = {"ok": True, "user_id": "U123ABC"}
512+
mock_client.conversations_replies.return_value = {
513+
"ok": True,
514+
"messages": [
515+
{"text": "Starting a discussion", "ts": "1234567890.123456"},
516+
{"text": "Some reply", "ts": "1234567890.123457"},
517+
{"text": "<@U123ABC> can you help with this?", "ts": "1234567890.123458"}, # Bot mentioned later
518+
],
519+
}
520+
521+
if "app.utils.handler_utils" in sys.modules:
522+
del sys.modules["app.utils.handler_utils"]
523+
from app.utils.handler_utils import was_bot_mentioned_in_thread_root
524+
525+
result = was_bot_mentioned_in_thread_root("C123", "1234567890.123456", mock_client)
526+
527+
assert result is True
528+
529+
530+
def test_get_bot_user_id_auth_test_fails(mock_env: Mock):
531+
"""test get_bot_user_id returns None when auth_test fails"""
532+
mock_client = Mock()
533+
mock_client.auth_test.return_value = {"ok": False}
534+
535+
if "app.utils.handler_utils" in sys.modules:
536+
del sys.modules["app.utils.handler_utils"]
537+
from app.utils.handler_utils import get_bot_user_id
538+
539+
if hasattr(get_bot_user_id, "_cache"):
540+
get_bot_user_id._cache.clear()
541+
542+
result = get_bot_user_id(mock_client)
543+
544+
assert result is None
545+
546+
547+
def test_get_bot_user_id_exception(mock_env: Mock):
548+
"""test get_bot_user_id returns None when exception occurs"""
549+
mock_client = Mock()
550+
mock_client.auth_test.side_effect = Exception("Auth failed")
551+
552+
if "app.utils.handler_utils" in sys.modules:
553+
del sys.modules["app.utils.handler_utils"]
554+
from app.utils.handler_utils import get_bot_user_id
555+
556+
if hasattr(get_bot_user_id, "_cache"):
557+
get_bot_user_id._cache.clear()
558+
559+
result = get_bot_user_id(mock_client)
560+
561+
assert result is None
562+
563+
564+
def test_was_bot_mentioned_no_messages_in_response(mock_env: Mock):
565+
"""test fail when API returns no messages"""
566+
mock_client = Mock()
567+
mock_client.auth_test.return_value = {"ok": True, "user_id": "U123ABC"}
568+
mock_client.conversations_replies.return_value = {"ok": True, "messages": []}
569+
570+
if "app.utils.handler_utils" in sys.modules:
571+
del sys.modules["app.utils.handler_utils"]
572+
from app.utils.handler_utils import was_bot_mentioned_in_thread_root
573+
574+
result = was_bot_mentioned_in_thread_root("C123", "1234567890.123456", mock_client)
575+
576+
assert result is True
577+
578+
579+
def test_was_bot_mentioned_api_not_ok(mock_env: Mock):
580+
"""test fail when API returns ok: False"""
581+
mock_client = Mock()
582+
mock_client.auth_test.return_value = {"ok": True, "user_id": "U123ABC"}
583+
mock_client.conversations_replies.return_value = {"ok": False}
584+
585+
if "app.utils.handler_utils" in sys.modules:
586+
del sys.modules["app.utils.handler_utils"]
587+
from app.utils.handler_utils import was_bot_mentioned_in_thread_root
588+
589+
result = was_bot_mentioned_in_thread_root("C123", "1234567890.123456", mock_client)
590+
591+
assert result is True

0 commit comments

Comments
 (0)