Merge branch 'pull_request_conversation_1' into pull_request_conversation_2

Author: anthony-nhs

packages/slackBotFunction/app/handler.py

@@ -27,8 +27,11 @@ def handler(event: dict, context: LambdaContext) -> dict:
     3. Lazy Async invocation processes Bedrock query and responds to Slack
 
     If message starts with pr: then pull request id is extracted from message and it invokes lambda in pull request
-    This message has a pull_request_processing property so when it is received by the lambda in the pull request
+    This message has a pull_request_event property so when it is received by the lambda in the pull request
     It triggers function process_pull_request_slack_event
+    When a session is started with a pr: prefix, the pull request is stored in dynamo
+    When subsequent actions or events are processed, this is looked up, and if it exists, then the pull request lambda
+    is triggered with either pull_request_event or pull_request_action
     """
     app = get_app(logger=logger)
     # handle pull request processing requests

packages/slackBotFunction/app/services/app.py

@@ -10,6 +10,7 @@
 @lru_cache
 def get_app(logger: Logger) -> App:
     bot_token, signing_secret = get_ssm_params()
+    # pass the correct logger to slack bolt so it can be pickled correctly
     powertools_logger = logging.getLogger(name=logger.name)
 
     # initialise the Slack app

packages/slackBotFunction/app/slack/slack_events.py

@@ -262,6 +262,7 @@ def process_async_slack_action(body: Dict[str, Any], client: WebClient) -> None:
 
 
 def process_async_slack_event(event: Dict[str, Any], event_id: str, client: WebClient) -> None:
+    logger.debug("Processing async Slack event", extra={"event_id": event_id, "event": event})
     original_message_text = (event.get("text") or "").strip()
     message_text = strip_mentions(message_text=original_message_text)
     conversation_key, thread_ts = conversation_key_and_root(event)

packages/slackBotFunction/app/slack/slack_handlers.py

@@ -23,6 +23,7 @@
     forward_event_to_pull_request_lambda,
     gate_common,
     respond_with_eyes,
+    should_reply_to_message,
 )
 from app.slack.slack_events import process_async_slack_action, process_async_slack_event
 
@@ -49,7 +50,8 @@ def setup_handlers(app: App) -> None:
 
 # ack function for events where we respond with eyes
 def respond_to_events(event: Dict[str, Any], ack: Ack, client: WebClient):
-    respond_with_eyes(event=event, client=client)
+    if should_reply_to_message(event):
+        respond_with_eyes(event=event, client=client)
     logger.debug("Sending ack response")
     ack()
 
@@ -94,8 +96,17 @@ def unified_message_handler(client: WebClient, event: Dict[str, Any], body: Dict
 
     """
     event_id = gate_common(event=event, body=body)
+    logger.debug("logging result of gate_common", extra={"event_id": event_id, "body": body})
     if not event_id:
         return
+    # if its in a group chat
+    # and its a message
+    # and its not in a thread
+    # then ignore it as it will be handled as an app_mention event
+    if not should_reply_to_message(event):
+        logger.debug("Ignoring message in group chat not in a thread", extra={"event": event})
+        # ignore messages in group chats
+        return
     user_id = event.get("user", "unknown")
     conversation_key, _ = conversation_key_and_root(event=event)
     session_pull_request_id = extract_session_pull_request_id(conversation_key)

packages/slackBotFunction/app/utils/handler_utils.py

@@ -38,6 +38,7 @@ def is_duplicate_event(event_id: str) -> bool:
         return False  # Not a duplicate
     except ClientError as e:
         if e.response["Error"]["Code"] == "ConditionalCheckFailedException":
+            logger.info(f"Duplicate event detected: {event_id}")
             return True  # Duplicate
         logger.error("Error checking event duplication", extra={"error": traceback.format_exc()})
         return False
@@ -203,3 +204,17 @@ def extract_session_pull_request_id(conversation_key: str) -> str | None:
     except Exception as e:
         logger.error(f"Error checking pull request session: {e}", extra={"error": traceback.format_exc()})
         return None
+
+
+def should_reply_to_message(event: Dict[str, Any]) -> bool:
+    """
+    Determine if the bot should reply to the message.
+
+    Conditions:
+    should not reply if:
+    - Message is in a group chat (channel_type == 'group') but not in a thread
+    """
+
+    if event.get("channel_type") == "group" and event.get("type") == "message" and event.get("thread_ts") is None:
+        return False
+    return True

packages/slackBotFunction/tests/test_slack_handlers.py

@@ -39,6 +39,42 @@ def test_unified_message_handler_successful_call(
     mock_forward_event_to_pull_request_lambda.assert_not_called()
 
 
+@patch("app.slack.slack_events.process_async_slack_event")
+@patch("app.utils.handler_utils.extract_session_pull_request_id")
+@patch("app.utils.handler_utils.forward_event_to_pull_request_lambda")
+def test_unified_message_handler_messages_with_no_thread_are_dropped(
+    mock_forward_event_to_pull_request_lambda: Mock,
+    mock_extract_session_pull_request_id: Mock,
+    mock_process_async_slack_event: Mock,
+    mock_get_parameter: Mock,
+    mock_env: Mock,
+):
+    """Test app mention handler execution"""
+    # set up mocks
+    mock_event = {
+        "user": "U123",
+        "text": "<@U456> test",
+        "channel": "C123",
+        "channel_type": "group",
+        "type": "message",
+    }
+    mock_body = {"event_id": "evt123"}
+    mock_client = Mock()
+    mock_extract_session_pull_request_id.return_value = None
+
+    # delete and import module to test
+    if "app.slack.slack_handlers" in sys.modules:
+        del sys.modules["app.slack.slack_handlers"]
+    from app.slack.slack_handlers import unified_message_handler
+
+    # perform operation
+    unified_message_handler(event=mock_event, body=mock_body, client=mock_client)
+
+    # assertions
+    mock_process_async_slack_event.assert_not_called()
+    mock_forward_event_to_pull_request_lambda.assert_not_called()
+
+
 @patch("app.slack.slack_events.process_async_slack_event")
 @patch("app.utils.handler_utils.extract_session_pull_request_id")
 @patch("app.utils.handler_utils.forward_event_to_pull_request_lambda")