Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

components/clp-mcp-server/tests/server/test-utils.py (4)

1-1: Confirm tests package is explicit to avoid INP001.

If missing, add components/clp-mcp-server/tests/init.py (empty) or tests/server/init.py.

#!/bin/bash
# Verify explicit package to silence INP001
fd -a 'tests/__init__.py' components/clp-mcp-server || true
fd -a 'tests/server/__init__.py' components/clp-mcp-server || true

---

3-3: Annotate class-level fixtures with ClassVar and import it to silence RUF012.

Apply:

-from clp_mcp_server.server.utils import filter_query_results, sort_query_results
+from typing import ClassVar
+from clp_mcp_server.server.utils import filter_query_results, sort_query_results
@@
-    RAW_LOG_ENTRIES = [
+    RAW_LOG_ENTRIES: ClassVar[list[dict]] = [
@@
-    EXPECTED_RESULTS = [
+    EXPECTED_RESULTS: ClassVar[list[str]] = [
@@
-    MISSING_TIMESTAMP_AND_MESSAGE_ENTRY = [
+    MISSING_TIMESTAMP_AND_MESSAGE_ENTRY: ClassVar[list[dict]] = [
@@
-    MISSING_TIMESTAMP_AND_MESSAGE_EXPECTED = [
+    MISSING_TIMESTAMP_AND_MESSAGE_EXPECTED: ClassVar[list[str]] = [
@@
-    INVALID_TYPE_ENTRIES = [
+    INVALID_TYPE_ENTRIES: ClassVar[list[dict]] = [
@@
-    INVALID_TYPE_EXPECTED = [
+    INVALID_TYPE_EXPECTED: ClassVar[list[str]] = [
@@
-    INVALID_VALUE_ENTRIES = [
+    INVALID_VALUE_ENTRIES: ClassVar[list[dict]] = [
@@
-    INVALID_VALUE_EXPECTED = [
+    INVALID_VALUE_EXPECTED: ClassVar[list[str]] = [

Also applies to: 9-105

---

107-113: Add a robustness test for mixed timestamp types to protect against TypeError in sorting.

Append:

+    def test_sort_with_mixed_timestamp_types(self):
+        mixed = [
+            {"timestamp": "1729267200000", "message": "str ts"},
+            {"timestamp": 1729267200000.0, "message": "float ts"},
+            {"timestamp": None, "message": "none ts"},
+            {"timestamp": 1729267200123, "message": "int ts"},
+        ]
+        # sort should not raise; entries with bad/non-int timestamps end up after valid ints
+        sorted_result = sort_query_results(mixed)
+        assert sorted_result[0]["timestamp"] == 1729267200123
+        # filtering should convert invalid timestamps to "N/A"
+        filtered = filter_query_results(sorted_result)
+        assert filtered[0].startswith("timestamp: 2024-10-18T16:00:00.123Z")

---

130-130: Ensure file ends with a single trailing newline to silence W292.

components/clp-mcp-server/clp_mcp_server/server/server.py (1)

75-102: Guard empty KQL input to prevent submitting blank/whitespace-only jobs.

Apply:

 @mcp.tool
 async def search_kql_query(kql_query: str, ctx: Context) -> dict[str, object]:
@@
-        try:
+        try:
+            if not kql_query or not kql_query.strip():
+                return {"Error": "Empty KQL query."}
             query_id = await connector.submit_query(kql_query)
             await connector.wait_query_completion(query_id)
             results = await connector.read_results(query_id)
         except (ValueError, RuntimeError, TimeoutError) as e:
             return {"Error": str(e)}

components/clp-mcp-server/clp_mcp_server/server/utils.py (1)

33-41: Harden sort key to avoid TypeError with mixed timestamp types and ensure bad/missing values sort last.

Apply:

-def sort_query_results(query_results: list[dict]) -> list[dict]:
+def sort_query_results(query_results: list[dict]) -> list[dict]:
@@
-    return sorted(query_results, key=lambda log_entry: log_entry.get("timestamp", 0), reverse=True)
+    def _ts_key(entry: dict) -> int:
+        ts = entry.get("timestamp")
+        try:
+            return int(ts)
+        except (TypeError, ValueError):
+            return -1  # sentinel: oldest
+    return sorted(query_results, key=_ts_key, reverse=True)

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

components/clp-mcp-server/tests/server/__init__.py (1)

1-1: Add a trailing newline (PEP 8 / Ruff W292).

File appears to end without a newline; add a single newline at EOF.

-"""Tests for server module."""
+"""Tests for server module."""

components/clp-mcp-server/tests/server/test_utils.py (1)

107-113: Add a robustness test for mixed-type timestamps in sort.

Current tests only cover int timestamps; add a case mixing int, str, float, and missing to ensure no TypeError and deterministic ordering.

Apply:

@@
     def test_sort_and_filter_query_results(self):
         """Validates the functionality of post-processing for the query response."""
         sorted_result = sort_query_results(self.RAW_LOG_ENTRIES)
         filtered_result = filter_query_results(sorted_result)
 
         assert filtered_result == self.EXPECTED_RESULTS
+
+    def test_sort_handles_mixed_timestamp_types(self):
+        """sort_query_results should not raise on mixed timestamp types."""
+        mixed = [
+            {"timestamp": 2, "message": "int-2\n"},
+            {"timestamp": "3", "message": "str-3\n"},
+            {"timestamp": 1.0, "message": "float-1\n"},
+            {"message": "missing\n"},
+        ]
+        # Should not raise:
+        sorted_mixed = sort_query_results(mixed)
+        # After sorting, valid ints should come first (latest-to-oldest), others last.
+        assert sorted_mixed[0]["message"] == "int-2\n"
+        # Sanity: length unchanged
+        assert len(sorted_mixed) == 4
+        # Filtering should still handle invalid types as N/A without raising:
+        _ = filter_query_results(sorted_mixed)

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

components/clp-mcp-server/tests/server/test_utils.py (2)

107-113: LGTM! Test correctly validates the full pipeline.

The test properly exercises both sorting and filtering with homogeneous integer timestamps. However, as noted in previous reviews, consider adding a test case for mixed-type timestamps to ensure sort_query_results handles them without raising TypeError.

---

9-105: Add ClassVar annotations to class-level test data.

These class attributes should be annotated with typing.ClassVar to satisfy RUF012 and clarify they are class-level constants, not mutable instance attributes.

Based on static analysis hints.

Apply this diff:

-from clp_mcp_server.server.utils import filter_query_results, sort_query_results
+from typing import ClassVar
+from clp_mcp_server.server.utils import filter_query_results, sort_query_results

 class TestUtils:
     """Test suite for utility functions."""
     
     # Testing basic functionality.
-    RAW_LOG_ENTRIES = [
+    RAW_LOG_ENTRIES: ClassVar[list[dict]] = [
         {
             "_id": "test001",
             "timestamp": 0,
@@ -39,7 +40,7 @@
         },
     ]

-    EXPECTED_RESULTS = [
+    EXPECTED_RESULTS: ClassVar[list[str]] = [
         (
             'timestamp: 2024-10-18T16:00:00.123Z, message: '
             '{"ts":1729267200123,"pid":1234,"tid":5678,'
@@ -59,11 +60,11 @@
     ]

     # Test case: missing timestamp and message fields.
-    MISSING_TIMESTAMP_AND_MESSAGE_ENTRY = [
+    MISSING_TIMESTAMP_AND_MESSAGE_ENTRY: ClassVar[list[dict]] = [
         {
             "_id": "test001",
         }
     ]
-    MISSING_TIMESTAMP_AND_MESSAGE_EXPECTED = [
+    MISSING_TIMESTAMP_AND_MESSAGE_EXPECTED: ClassVar[list[str]] = [
         "timestamp: N/A, message: "
     ]

     # Test case: invalid timestamp types.
-    INVALID_TYPE_ENTRIES = [
+    INVALID_TYPE_ENTRIES: ClassVar[list[dict]] = [
         {
             "timestamp": None,
             "message": '{"message":"Log with None timestamp"}\n',
@@ -83,7 +84,7 @@
             "message": '{"message":"Log with float timestamp"}\n',
         },
     ]
-    INVALID_TYPE_EXPECTED = [
+    INVALID_TYPE_EXPECTED: ClassVar[list[str]] = [
         'timestamp: N/A, message: {"message":"Log with None timestamp"}\n',
         'timestamp: N/A, message: {"message":"Log with str timestamp"}\n',
         'timestamp: N/A, message: {"message":"Log with float timestamp"}\n',
@@ -91,7 +92,7 @@

     # Test case: invalid timestamp values.
-    INVALID_VALUE_ENTRIES = [
+    INVALID_VALUE_ENTRIES: ClassVar[list[dict]] = [
         {
             "timestamp": 9999999999999999,
             "message": '{"message":"Log with overflow timestamp"}\n',
@@ -101,7 +102,7 @@
             "message": '{"message":"Log with negative overflow timestamp"}\n',
         },
     ]
-    INVALID_VALUE_EXPECTED = [
+    INVALID_VALUE_EXPECTED: ClassVar[list[str]] = [
         'timestamp: N/A, message: {"message":"Log with overflow timestamp"}\n',
         'timestamp: N/A, message: {"message":"Log with negative overflow timestamp"}\n',
     ]

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

components/clp-mcp-server/clp_mcp_server/server/utils.py (2)

48-49: Consider coercing message to string for robustness.

If the message field is accidentally a non-string type (e.g., int or dict), it will be interpolated directly into the formatted string, potentially producing unexpected output. Coerce it to str for defensive programming.

Apply this diff:

-        message = obj.get("message", "")
-        filtered.append(f"timestamp: {timestamp_str}, message: {message}")
+        message = str(obj.get("message", ""))
+        filtered.append(f"timestamp: {timestamp_str}, message: {message}")

Optionally, add a test case with a non-string message to verify behavior.

---

54-61: Guard against TypeError with mixed-type timestamps.

If any entry has a non-integer timestamp (str/float/None), Python's sort will raise TypeError when comparing mixed key types. Add a safe key function that returns valid int timestamps as-is and a sentinel value for invalid types, ensuring invalid timestamps sort last.

Apply this diff:

 def sort_query_results(query_results: list[dict]) -> list[dict]:
     """
     :param query_results: A list of dictionary containing log entries with its metadata read from
     MongoDB.
     :return: A sorted list of dictionary containing log entries with its metadata, ordered by epoch
     from latest to oldest.
     """
-    return sorted(query_results, key=lambda log_entry: log_entry.get("timestamp", 0), reverse=True)
+    def _sort_key(log_entry: dict) -> int:
+        ts = log_entry.get("timestamp")
+        return ts if isinstance(ts, int) else -1  # Non-ints sort last
+
+    return sorted(query_results, key=_sort_key, reverse=True)

This aligns with filter_query_results, which treats non-int timestamps as invalid ("N/A").

components/clp-mcp-server/tests/server/test_utils.py (1)

3-106: Annotate class fixtures with ClassVar and add blank line after docstring.

All class-level fixture attributes trigger RUF012 and should be annotated with ClassVar to indicate they are class constants, not mutable instance attributes. Additionally, insert one blank line after the class docstring per D204.

Based on static analysis hints.

Apply this diff:

-from clp_mcp_server.server.utils import filter_query_results, sort_query_results
+from typing import ClassVar
+
+from clp_mcp_server.server.utils import filter_query_results, sort_query_results

 class TestUtils:
     """Test suite for utility functions."""
-    
+
 
     # Test case: invalid timestamp types.
-    INVALID_TYPE_ENTRIES = [
+    INVALID_TYPE_ENTRIES: ClassVar[list[dict]] = [
         {
             "timestamp": None,
             "message": '{"message":"Log with None timestamp"}\n',
         },
         {
             "timestamp": "1729267200000",  # str instead of int
             "message": '{"message":"Log with str timestamp"}\n',
         },
         {
             "timestamp": 1729267200000.0,  # float instead of int
             "message": '{"message":"Log with float timestamp"}\n',
         },
     ]
-    INVALID_TYPE_EXPECTED = [
+    INVALID_TYPE_EXPECTED: ClassVar[list[str]] = [
         'timestamp: N/A, message: {"message":"Log with None timestamp"}\n',
         'timestamp: N/A, message: {"message":"Log with str timestamp"}\n',
         'timestamp: N/A, message: {"message":"Log with float timestamp"}\n',
     ]
 
     # Test case: invalid timestamp values.
-    INVALID_VALUE_ENTRIES = [
+    INVALID_VALUE_ENTRIES: ClassVar[list[dict]] = [
         {
             "timestamp": 9999999999999999,
             "message": '{"message":"Log with overflow timestamp"}\n',
         },
         {
             "timestamp": -9999999999999999,
             "message": '{"message":"Log with negative overflow timestamp"}\n',
         },
     ]
-    INVALID_VALUE_EXPECTED = [
+    INVALID_VALUE_EXPECTED: ClassVar[list[str]] = [
         'timestamp: N/A, message: {"message":"Log with overflow timestamp"}\n',
         'timestamp: N/A, message: {"message":"Log with negative overflow timestamp"}\n',
     ]
 
     # Test case: missing timestamp and message fields.
-    MISSING_TIMESTAMP_AND_MESSAGE_ENTRY = [
+    MISSING_TIMESTAMP_AND_MESSAGE_ENTRY: ClassVar[list[dict]] = [
         {
             "_id": "test001",
         }
     ]
-    MISSING_TIMESTAMP_AND_MESSAGE_EXPECTED = [
+    MISSING_TIMESTAMP_AND_MESSAGE_EXPECTED: ClassVar[list[str]] = [
         "timestamp: N/A, message: "
     ]
 
     # Testing basic functionality.
-    RAW_LOG_ENTRIES = [
+    RAW_LOG_ENTRIES: ClassVar[list[dict]] = [
         {
             "_id": "test001",
             "timestamp": 0,
             "message": '{"ts":0,"pid":null,"tid":null,"message":"Log at epoch zero"}\n',
             "orig_file_path": "/var/log/app.log",
             "archive_id": "abc123",
             "log_event_ix": 100,
         },
         {
             "_id": "test002",
             "timestamp": 1729267200000,  # Oct 18, 2024 16:00:00.000 (milliseconds)
             "message": (
                 '{"ts":1729267200000,"pid":1234,"tid":5678,'
                 '"message":"Log with zero milliseconds"}\n'
             ),
             "orig_file_path": "/var/log/app.log",
             "archive_id": "abc124",
             "log_event_ix": 101,
         },
         {
             "_id": "test003",
             "timestamp": 1729267200123,  # Oct 18, 2024 16:00:00.123 (milliseconds)
             "message": (
                 '{"ts":1729267200123,"pid":1234,"tid":5678,'
                 '"message":"Log with millisecond precision"}\n'
             ),
             "orig_file_path": "/var/log/app.log",
             "archive_id": "abc125",
             "log_event_ix": 102,
         },
     ]
 
-    EXPECTED_RESULTS = [
+    EXPECTED_RESULTS: ClassVar[list[str]] = [
         (
             'timestamp: 2024-10-18T16:00:00.123Z, message: '
             '{"ts":1729267200123,"pid":1234,"tid":5678,'
             '"message":"Log with millisecond precision"}\n'
         ),
         (
             'timestamp: 2024-10-18T16:00:00.000Z, message: '
             '{"ts":1729267200000,"pid":1234,"tid":5678,'
             '"message":"Log with zero milliseconds"}\n'
         ),
         (
             'timestamp: 1970-01-01T00:00:00.000Z, message: '
             '{"ts":0,"pid":null,"tid":null,'
             '"message":"Log at epoch zero"}\n'
         ),
     ]

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

components/clp-mcp-server/clp_mcp_server/server/utils.py (1)

48-49: Consider explicit string coercion for defensive coding.

While Python's f-string formatting implicitly converts non-string types, explicitly coercing the message field ensures consistent behaviour if unexpected types (e.g., dict, list) appear in the data.

Apply this diff if you prefer explicit type safety:

         message = obj.get("message", "")
-        filtered.append(f"timestamp: {timestamp_str}, message: {message}")
+        filtered.append(f"timestamp: {timestamp_str}, message: {str(message)}")

components/clp-mcp-server/tests/server/test_utils.py (2)

3-3: Add ClassVar import to satisfy static analysis.

The class-level test data fixtures should be annotated with ClassVar to indicate they are class variables, not mutable instance attributes.

Based on static analysis hints.

Apply this diff:

+from typing import ClassVar
+
 from clp_mcp_server.server.utils import filter_query_results, sort_query_results

---

9-119: Annotate class-level fixtures with ClassVar.

All class-level test data fixtures should be annotated with ClassVar to indicate they are class variables, not mutable instance attributes. This resolves the RUF012 warnings from static analysis.

Based on static analysis hints.

Apply this diff:

     """Test suite for utility functions."""
 
     # Test case: invalid timestamp types.
-    INVALID_TYPE_ENTRIES = [
+    INVALID_TYPE_ENTRIES: ClassVar[list[dict]] = [
         {
             "timestamp": None,
             "message": '{"message":"Log with None timestamp"}\n',
         },
         {
             "timestamp": "1729267200000",  # str instead of int
             "message": '{"message":"Log with str timestamp"}\n',
         },
         {
             "timestamp": 1729267200000.0,  # float instead of int
             "message": '{"message":"Log with float timestamp"}\n',
         },
     ]
-    INVALID_TYPE_EXPECTED = [
+    INVALID_TYPE_EXPECTED: ClassVar[list[str]] = [
         'timestamp: N/A, message: {"message":"Log with None timestamp"}\n',
         'timestamp: N/A, message: {"message":"Log with str timestamp"}\n',
         'timestamp: N/A, message: {"message":"Log with float timestamp"}\n',
     ]
 
     # Test case: invalid timestamp values.
-    INVALID_VALUE_ENTRIES = [
+    INVALID_VALUE_ENTRIES: ClassVar[list[dict]] = [
         {
             "timestamp": 9999999999999999,
             "message": '{"message":"Log with overflow timestamp"}\n',
         },
         {
             "timestamp": -9999999999999999,
             "message": '{"message":"Log with negative overflow timestamp"}\n',
         },
     ]
-    INVALID_VALUE_EXPECTED = [
+    INVALID_VALUE_EXPECTED: ClassVar[list[str]] = [
         'timestamp: N/A, message: {"message":"Log with overflow timestamp"}\n',
         'timestamp: N/A, message: {"message":"Log with negative overflow timestamp"}\n',
     ]
 
     # Test case: missing timestamp and message fields.
-    MISSING_TIMESTAMP_AND_MESSAGE_ENTRY = [
+    MISSING_TIMESTAMP_AND_MESSAGE_ENTRY: ClassVar[list[dict]] = [
         {
             "_id": "test001",
         }
     ]
-    MISSING_TIMESTAMP_AND_MESSAGE_EXPECTED = [
+    MISSING_TIMESTAMP_AND_MESSAGE_EXPECTED: ClassVar[list[str]] = [
         "timestamp: N/A, message: "
     ]
 
     # Testing basic functionality.
-    RAW_LOG_ENTRIES = [
+    RAW_LOG_ENTRIES: ClassVar[list[dict]] = [
         {
             "_id": "test000",
             "timestamp": None,
             ...
         },
     ]
 
-    EXPECTED_RESULTS = [
+    EXPECTED_RESULTS: ClassVar[list[str]] = [
         (
             'timestamp: 2024-10-18T16:00:00.123Z, message: '
             ...
         ),
     ]

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

components/clp-mcp-server/clp_mcp_server/server/server.py (3)

15-23: Rename parameter to match its type and update docstring.

The parameter is named config_path but its type is CLPConfig, not a path. This is misleading. Rename to clp_config and document it properly.

Apply this diff:

-def create_mcp_server(config_path: CLPConfig) -> FastMCP:
+def create_mcp_server(clp_config: CLPConfig) -> FastMCP:
     """
     Creates and defines API tool calls for the CLP MCP server.
 
-    :param config_path:
+    :param clp_config: The CLP configuration object.
     :return: A configured `FastMCP` instance.
     :raise: Propagates `FastMCP.__init__`'s exceptions.
     :raise: Propagates `FastMCP.tool`'s exceptions.
     """
     mcp = FastMCP(name=constants.SERVER_NAME)
 
     session_manager = SessionManager(session_ttl_seconds=constants.SESSION_TTL_SECONDS)
 
-    connector = ClpConnector(config_path)
+    connector = ClpConnector(clp_config)

Based on learnings.

---

79-96: Remove magic number and fix duplicate :return: sections in docstring.

The docstring has two issues:

1. Lines 82-83 hardcode "10 log messages", but this is a configurable constant (NUM_ITEMS_PER_PAGE) that the return value already includes as num_items_per_page.
2. Lines 87 and 94 both use :return:, creating duplicate return documentation.

Apply this diff:

     """
     Searches the logs for the specified Kibana Query Language (KQL) query and returns the first
-    page of the matching result. The paginated results are ordered from latest to oldest, with
-    each page containing the paging metadata and 10 log messages along with their date string
-    timestamps.
+    page of the matching result. The paginated results are ordered from latest to oldest.
 
     :param kql_query:
     :param ctx: The `FastMCP` context containing the metadata of the underlying MCP session.
-    :return: A dictionary containing the following key-value pairs on success:
+    :return: On success, a dictionary containing the following key-value pairs:
         - "items": A list of log entries in the requested page.
         - "num_total_pages": Total number of pages available from the query as an integer.
         - "num_total_items": Total number of log entries available from the query as an integer.
         - "num_items_per_page": Number of log entries per page.
         - "has_next": Whether a page exists after the returned one.
         - "has_previous": Whether a page exists before the returned one.
-    :return: A dictionary with the following key-value pair on failures:
+        On failure, a dictionary with the following key-value pair:
         - "Error": An error message describing the failure.
     """

Based on learnings.

---

99-104: Add empty query validation and broaden exception handling.

Two issues:

1. No validation for empty or whitespace-only kql_query. Submitting an empty query may cause unexpected behavior.
2. Exception handling only catches ValueError, RuntimeError, and TimeoutError. Database/driver exceptions (e.g., aiomysql.Error, pymongo.errors.PyMongoError) may escape and break the stable {"Error": "..."} response contract.

Apply this diff:

         await session_manager.start()
 
         try:
+            if not kql_query or not kql_query.strip():
+                return {"Error": "Empty KQL query."}
             query_id = await connector.submit_query(kql_query)
             await connector.wait_query_completion(query_id)
             results = await connector.read_results(query_id)
         except (ValueError, RuntimeError, TimeoutError) as e:
             return {"Error": str(e)}
+        except Exception as e:
+            return {"Error": str(e)}
 
         sorted_results = sort_by_timestamp(results)
         filtered_results = filter_query_results(sorted_results)

components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py (1)

68-75: Include config path in error messages for better debugging.

The error messages on lines 71 and 74 don't include the configuration file path, making it harder to debug issues. Include the config_path in the log messages to provide better context.

Apply this diff:

     try:
         clp_config = CLPConfig.model_validate(read_yaml_config_file(config_path))
     except ValidationError:
-        logger.exception("Configuration validation failed.")
+        logger.exception("Configuration validation failed for: %s", config_path)
         exit_code = 1
     except Exception:
-        logger.exception("Failed to load configuration.")
+        logger.exception("Failed to load configuration from: %s", config_path)
         exit_code = 1

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Learnt from: 20001020ycx
PR: y-scope/clp#1436
File: components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py:22-27
Timestamp: 2025-10-20T20:19:40.504Z
Learning: In the clp-mcp-server project (components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py), the --config Click option should use exists=True to validate the configuration file path at option processing time, even though this requires the default path to exist on all machines.

components/clp-mcp-server/clp_mcp_server/clp_connector.py (1)

3-5: Type the constructor with CLPConfig and document the parameter.

Improves type safety and clarity.

Apply:

-from typing import Any
+from typing import Any
+from clp_py_utils.clp_config import CLPConfig
@@
-    def __init__(self, clp_config: Any) -> None:
-        """Initializes the ClpConnector with MongoDB and MariaDB configurations."""
+    def __init__(self, clp_config: CLPConfig) -> None:
+        """
+        Initializes the ClpConnector with MongoDB and MariaDB configurations.
+
+        :param clp_config: CLP configuration containing database, results cache and webui settings.
+        """

Also applies to: 26-31

components/clp-mcp-server/tests/server/test_utils.py (1)

121-131: Add dedicated test for mixed-type sorting.

Cover sort_by_timestamp with int/str/float/None timestamps to ensure no TypeError and stable ordering.

Example:

@@
     def test_invalid_timestamp_value(self):
         """Validates the handling of invalid timestamp values."""
         result = format_query_results(self.INVALID_VALUE_ENTRIES)
 
         assert result == self.INVALID_VALUE_EXPECTED
+
+    def test_sort_by_timestamp_mixed_types(self):
+        """sort_by_timestamp handles heterogeneous timestamp types."""
+        mixed = [
+            {"timestamp": 1000, "message": "int 1000"},
+            {"timestamp": "2000", "message": "str 2000"},
+            {"timestamp": 500.5, "message": "float 500.5"},
+            {"timestamp": None, "message": "none"},
+            {"timestamp": 2000, "message": "int 2000"},
+        ]
+        result = sort_by_timestamp(mixed)
+        assert result[0]["timestamp"] == 2000
+        assert result[1]["timestamp"] == 1000
+        assert not isinstance(result[-1]["timestamp"], int)

components/clp-mcp-server/clp_mcp_server/server/server.py (1)

77-94: Harden tool boundary: empty KQL guard and catch‑all to preserve {"Error": "..."} contract.

Prevents submitting blank jobs and leaks of driver exceptions.

Apply:

 @mcp.tool
 async def search_by_kql(kql_query: str, ctx: Context) -> dict[str, Any]:
@@
-        await session_manager.start()
+        await session_manager.start()
@@
-        try:
+        try:
+            if not kql_query or not kql_query.strip():
+                return {"Error": "Empty KQL query."}
             query_id = await connector.submit_query(kql_query)
             await connector.wait_query_completion(query_id)
             results = await connector.read_results(query_id)
         except (ValueError, RuntimeError, TimeoutError) as e:
             return {"Error": str(e)}
+        except Exception as e:
+            return {"Error": str(e)}

Also applies to: 95-109

components/clp-mcp-server/clp_mcp_server/server/utils.py (2)

12-17: Docstring style: use “:raises” with the exception type.

Keep Sphinx conventions consistent.

Apply:

-    :raise: ValueError if `epoch_ts` cannot be converted to a valid date string.
+    :raises ValueError: If `epoch_ts` cannot be converted to a valid date string.

---

44-46: Coerce message to string to avoid non‑str leakage.

Prevents accidental types (dict/int) from leaking into output.

Apply:

-        message = obj.get("message", "")
-        filtered.append(f"timestamp: {timestamp_str}, message: {message}")
+        message = obj.get("message", "")
+        filtered.append(f"timestamp: {timestamp_str}, message: {str(message)}")

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

components/clp-mcp-server/clp_mcp_server/clp_connector.py (2)

26-27: Document the clp_config parameter in the constructor docstring.

The clp_config parameter is missing from the docstring, as previously noted in review comments.

Apply this diff to add the parameter documentation:

     def __init__(self, clp_config: Any) -> None:
-        """Initializes the ClpConnector with MongoDB and MariaDB configurations."""
+        """
+        Initializes the ClpConnector with MongoDB and MariaDB configurations.
+        
+        :param clp_config: The CLP configuration object containing database and cache settings.
+        """

---

53-57: Use standard Sphinx :raises format for exception documentation.

The current :raise: format is non-standard. Sphinx convention uses :raises ExceptionType: description.

Apply this diff:

-        :raise: ValueError if `end_ts` is smaller than `begin_ts`.
-        :raise: RuntimeError if it fails to retrieve the ID of the submitted query.
-        :raise: aiomysql.Error if there is an error connecting to or querying MariaDB.
-        :raise: pymongo.errors.PyMongoError if there is an error interacting with MongoDB.
-        :raise: Exception for any other unexpected errors.
+        :raises ValueError: If `end_ts` is smaller than `begin_ts`.
+        :raises RuntimeError: If it fails to retrieve the ID of the submitted query.
+        :raises aiomysql.Error: If there is an error connecting to or querying MariaDB.
+        :raises pymongo.errors.PyMongoError: If there is an error interacting with MongoDB.
+        :raises Exception: For any other unexpected errors.

components/clp-mcp-server/clp_mcp_server/server/server.py (3)

77-94: Consolidate duplicate :return: sections in docstring.

The docstring has two :return: sections (lines 85 and 92), which is non-standard. Consolidate into a single section describing both success and failure cases.

Apply this diff:

         :param kql_query:
         :param ctx: The `FastMCP` context containing the metadata of the underlying MCP session.
-        :return: A dictionary containing the following key-value pairs on success:
+        :return: On success, a dictionary containing the following key-value pairs:
             - "items": A list of log entries in the requested page.
             - "num_total_pages": Total number of pages available from the query as an integer.
             - "num_total_items": Total number of log entries available from the query as an integer.
             - "num_items_per_page": Number of log entries per page.
             - "has_next": Whether a page exists after the returned one.
             - "has_previous": Whether a page exists before the returned one.
-        :return: A dictionary with the following key-value pair on failures:
+            On failure, a dictionary with the following key-value pair:
             - "Error": An error message describing the failure.

---

97-102: Add validation for empty KQL query.

The function doesn't validate whether kql_query is empty before submitting it, which could result in unnecessary database operations.

Apply this diff to add validation:

         await session_manager.start()
 
         try:
+            if not kql_query or not kql_query.strip():
+                return {"Error": "Empty KQL query."}
             query_id = await connector.submit_query(kql_query)
             await connector.wait_query_completion(query_id)
             results = await connector.read_results(query_id)

---

101-102: Broaden exception handling to catch database exceptions.

The current exception tuple only catches (ValueError, RuntimeError, TimeoutError), but connector.submit_query(), wait_query_completion(), and read_results() can raise aiomysql.Error and pymongo.errors.PyMongoError per their docstrings. These exceptions will escape the handler and break the stable {"Error": "..."} response shape.

Apply this diff to add a catch-all handler:

         except (ValueError, RuntimeError, TimeoutError) as e:
             return {"Error": str(e)}
+        except Exception as e:
+            return {"Error": str(e)}

components/clp-mcp-server/clp_mcp_server/server/utils.py (3)

39-53: Rename variable to reflect its purpose.

The variable filtered (line 39) is misleading since the function formats results rather than filtering them. A previous review comment suggested formatted_log_events.

Apply this diff:

-    filtered = []
+    formatted_log_events = []
     for obj in query_results:
         epoch = obj.get("timestamp")
         timestamp_str = TIMESTAMP_NOT_AVAILABLE
 
         if isinstance(epoch, int):
             try:
                 timestamp_str = convert_epoch_to_date_string(epoch)
             except (TypeError, ValueError) as e:
                 logger.warning("Failed to convert epoch timestamp=%s to date string: %s.", epoch, e)
 
         message = obj.get("message", "")
-        filtered.append(f"timestamp: {timestamp_str}, message: {message}")
+        formatted_log_events.append(f"timestamp: {timestamp_str}, message: {message}")
 
-    return filtered
+    return formatted_log_events

---

56-68: Expand docstring to clarify sorting behaviour and timestamp handling.

The docstring should explicitly document the in-place sorting, expected timestamp format, and handling of missing/invalid timestamps, as suggested in previous review comments.

Apply this diff:

     """
+    Sorts the query results in-place by timestamp in descending order (latest to oldest).
+
+    NOTE:
+    - Timestamp is expected to be an integer representing the epoch timestamp in milliseconds,
+      stored under the `timestamp` key.
+    - If `timestamp` is missing or not an integer, it is treated as the oldest possible timestamp.
+
     :param query_results: A list of dictionaries representing kv-pair log events.
-    :return: The same list, sorted in-place by epoch from latest to oldest.
+    :return: The input list sorted in-place.
     """

---

12-24: Use standard Sphinx :raises format in docstring.

Line 16 uses :raise: but the standard Sphinx format is :raises ValueError:.

Apply this diff:

-    :raise: ValueError if `epoch_ts` cannot be converted to a valid date string.
+    :raises ValueError: If `epoch_ts` cannot be converted to a valid date string.

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Learnt from: 20001020ycx
PR: y-scope/clp#1436
File: components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py:22-27
Timestamp: 2025-10-20T20:19:40.504Z
Learning: In the clp-mcp-server project (components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py), the --config Click option should use exists=True to validate the configuration file path at option processing time, even though this requires the default path to exist on all machines.