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/server/utils.py (2)

35-36: Complete the parameter documentation.

The docstring is missing descriptions for formatted_begin_timestamp and formatted_end_timestamp. These should document that both parameters accept ISO 8601 UTC date strings (with second or millisecond precision) ending with 'Z'.

---

14-15: Docstring overly restrictive on accepted timestamp formats.

The docstring states "millisecond precision (YYYY-MM-DDTHH:mm:ss.fffZ)" but datetime.fromisoformat on line 21 also accepts second precision (e.g., YYYY-MM-DDTHH:mm:ssZ). Update the docstring to reflect both accepted formats for accuracy.

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

134-136: Broaden docstring to accept both second and millisecond precision.

The docstring specifies YYYY-MM-DDTHH:mm:ss.fffZ (millisecond precision) but the implementation via parse_timestamp_range → convert_date_string_to_epoch also accepts second precision (e.g., YYYY-MM-DDTHH:mm:ssZ). Update to document both accepted formats.

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

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

154-175: Remove print statements and use pytest.raises match parameter.

Lines 169 and 173 contain print() statements that serve no purpose in tests. Additionally, all pytest.raises blocks should use the match parameter to validate exception messages more precisely.

Apply this diff to improve the test:

     def test_convert_date_string_to_epoch_invalid_date_string(self):
         """Validates the handling of invalid date string."""
-        with pytest.raises(ValueError) as exc_info:
+        with pytest.raises(ValueError, match=self.INVALID_DATE_STRING_ERROR):
             convert_date_string_to_epoch("not-a-date")
-        assert self.INVALID_DATE_STRING_ERROR in str(exc_info.value)
 
-        with pytest.raises(ValueError) as exc_info:
+        with pytest.raises(ValueError, match=self.INVALID_DATE_STRING_ERROR):
             convert_date_string_to_epoch("2024-13-45T25:99:99Z")
-        assert self.INVALID_DATE_STRING_ERROR in str(exc_info.value)
 
-        with pytest.raises(ValueError) as exc_info:
+        with pytest.raises(ValueError, match=self.INVALID_DATE_STRING_ERROR):
             convert_date_string_to_epoch("")
-        assert self.INVALID_DATE_STRING_ERROR in str(exc_info.value)
 
-        with pytest.raises(ValueError) as exc_info:
-            print(convert_date_string_to_epoch("2024.10.18T16:00:00.123"))
-        assert self.INVALID_DATE_STRING_ERROR in str(exc_info.value)
+        with pytest.raises(ValueError, match=self.INVALID_DATE_STRING_ERROR):
+            convert_date_string_to_epoch("2024.10.18T16:00:00.123")
 
-        with pytest.raises(ValueError) as exc_info:
-            print(convert_date_string_to_epoch("2024-10-18T16-00-00-123"))
-        assert self.INVALID_DATE_STRING_ERROR in str(exc_info.value)
+        with pytest.raises(ValueError, match=self.INVALID_DATE_STRING_ERROR):
+            convert_date_string_to_epoch("2024-10-18T16-00-00-123")

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

30-64: Clarify timestamp units in the docstring.

The parameters begin_ts and end_ts represent Unix epoch timestamps in milliseconds (as expected by connector.submit_query on line 54), but the docstring on lines 41-42 doesn't specify the units.

Apply this diff to clarify:

-        :param begin_ts: The beginning of the time range (inclusive).
-        :param end_ts: The end of the time range (inclusive).
+        :param begin_ts: The beginning of the time range in Unix epoch milliseconds (inclusive).
+        :param end_ts: The end of the time range in Unix epoch milliseconds (inclusive).

---

136-169: Update docstring to document both accepted timestamp formats.

The docstring on line 142 specifies only millisecond precision (YYYY-MM-DDTHH:mm:ss.fffZ), but the implementation via parse_timestamp_range and convert_date_string_to_epoch also accepts second precision without milliseconds (e.g., YYYY-MM-DDTHH:mm:ssZ). This discrepancy can mislead callers.

Apply this diff to document both accepted formats:

         Searches log events that match the given Kibana Query Language (KQL) query within the given
-        time range. Timestamps must follow the ISO 8601 UTC format (`YYYY-MM-DDTHH:mm:ss.fffZ`),
-        where the trailing `Z` indicates UTC. Timestamps that do not follow this format will be
-        rejected.
+        time range. Timestamps must follow the ISO 8601 UTC format (e.g., `YYYY-MM-DDTHH:mm:ssZ`
+        or `YYYY-MM-DDTHH:mm:ss.fffZ`), where the trailing `Z` indicates UTC. Timestamps that do
+        not follow this format will be rejected.

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

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

16-20: Fix the spacing before the assignment operator on Line 19.

Line 19 is missing a space before the = operator, which violates PEP 8 style guidelines. This issue was previously flagged but remains unresolved.

Apply this diff to fix the spacing:

-    INVALID_DATE_STRING_VALUE_ERROR= "is earlier than `formatted_begin_timestamp`"
+    INVALID_DATE_STRING_VALUE_ERROR = "is earlier than `formatted_begin_timestamp`"

---

154-175: Use pytest.raises match parameter and remove print statements.

The test logic is correct, but it violates pytest best practices (PT011) and contains debug print statements (T201) that should be removed. Use the match parameter to validate exception messages inline for consistency with the rest of the test suite.

Apply this diff to address the issues:

-        with pytest.raises(ValueError) as exc_info:
-            convert_date_string_to_epoch("not-a-date")
-        assert self.INVALID_DATE_STRING_ERROR in str(exc_info.value)
+        with pytest.raises(ValueError, match=self.INVALID_DATE_STRING_ERROR):
+            convert_date_string_to_epoch("not-a-date")
 
-        with pytest.raises(ValueError) as exc_info:
-            convert_date_string_to_epoch("2024-13-45T25:99:99Z")
-        assert self.INVALID_DATE_STRING_ERROR in str(exc_info.value)
+        with pytest.raises(ValueError, match=self.INVALID_DATE_STRING_ERROR):
+            convert_date_string_to_epoch("2024-13-45T25:99:99Z")
 
-        with pytest.raises(ValueError) as exc_info:
-            convert_date_string_to_epoch("")
-        assert self.INVALID_DATE_STRING_ERROR in str(exc_info.value)
+        with pytest.raises(ValueError, match=self.INVALID_DATE_STRING_ERROR):
+            convert_date_string_to_epoch("")
 
-        with pytest.raises(ValueError) as exc_info:
-            print(convert_date_string_to_epoch("2024.10.18T16:00:00.123"))
-        assert self.INVALID_DATE_STRING_ERROR in str(exc_info.value)
+        with pytest.raises(ValueError, match=self.INVALID_DATE_STRING_ERROR):
+            convert_date_string_to_epoch("2024.10.18T16:00:00.123")
 
-        with pytest.raises(ValueError) as exc_info:
-            print(convert_date_string_to_epoch("2024-10-18T16-00-00-123"))
-        assert self.INVALID_DATE_STRING_ERROR in str(exc_info.value)
+        with pytest.raises(ValueError, match=self.INVALID_DATE_STRING_ERROR):
+            convert_date_string_to_epoch("2024-10-18T16-00-00-123")

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro