Skip to content

feat(clp-mcp-server): Add log viewer links to query results for displaying in LLM output.#1454

Merged
All-less merged 3 commits into
y-scope:mainfrom
All-less:log-viewer-integration
Oct 21, 2025
Merged

feat(clp-mcp-server): Add log viewer links to query results for displaying in LLM output.#1454
All-less merged 3 commits into
y-scope:mainfrom
All-less:log-viewer-integration

Conversation

@All-less

@All-less All-less commented Oct 20, 2025

Copy link
Copy Markdown
Contributor

Description

This PR adds integration of Log Viewer into the MCP server. Specifically, it inserts links to the Log Viewer in the result returned by search API and instructs LLM to display the log messages as clickable links. As a result, the users can click the message in the diagnosis result and be redirected to the Log Viewer.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

cd components/clp-mcp-server
uv run pytest

Summary by CodeRabbit

  • New Features

    • Search queries now accept optional begin/end timestamps.
    • Search results include direct links to corresponding log entries in the web interface.
  • Bug Fixes

    • Timestamp validation only triggers when both begin and end are provided.
    • Default dataset selection uses the configured default.
    • Returned search results no longer expose internal ID fields; status messages show readable names.
  • Documentation

    • System guidance updated to render log messages as clickable hyperlinks.
  • Tests

    • Added test verifying link field generation in search results.

@All-less All-less requested a review from a team as a code owner October 20, 2025 19:50
@coderabbitai

coderabbitai Bot commented Oct 20, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Optional begin/end timestamps added to submit_query; _webui_addr is initialized from config; job dataset defaults to CLP_DEFAULT_DATASET_NAME; read_results now clears _id and adds a computed link; SYSTEM_PROMPT updated to instruct wrapping log messages as hyperlinks; tests added for link generation.

Changes

Cohort / File(s) Summary
Query submission & result enrichment
components/clp-mcp-server/clp_mcp_server/clp_connector.py
Import CLP_DEFAULT_DATASET_NAME; set self._webui_addr in __init__; broaden submit_query() signature to `begin_ts: int
Prompt update
components/clp-mcp-server/clp_mcp_server/server/constants.py
SYSTEM_PROMPT expanded to instruct that displayed log messages should be wrapped as hyperlinks using the result link field.
Tests
components/clp-mcp-server/tests/test_clp_connector.py
CLP config mock updated (DB host and added webui host/port). New test test_read_results_adds_link_field() verifies ClpConnector.read_results() adds correct link values to documents.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant ClpConnector
  participant CLPJobAPI
  participant ResultsStore

  rect rgb(235,245,255)
    Note right of Client: Submit query (optional begin/end timestamps)
    Client->>ClpConnector: submit_query(query, begin_ts?, end_ts?)
    ClpConnector->>CLPJobAPI: create job (dataset=CLP_DEFAULT_DATASET_NAME, timestamps?)
    CLPJobAPI-->>ClpConnector: job_id
    ClpConnector->>CLPJobAPI: wait_query_completion(job_id)
    CLPJobAPI-->>ClpConnector: status
  end

  rect rgb(245,255,235)
    Note right of ClpConnector: On completion, read and enrich results\n(add `link`, set `_id` to null)
    ClpConnector->>ResultsStore: read_results(job_id)
    ResultsStore-->>ClpConnector: documents
    ClpConnector->>ClpConnector: compute link using _webui_addr + archive_id + log_event_ix
    ClpConnector-->>Client: enriched documents (with `link`)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title Check ✅ Passed The pull request title "feat(clp-mcp-server): Add log viewer links to query results for displaying in LLM output" directly and clearly captures the primary objective of this changeset. The modifications to read_results to augment documents with a link field, updates to the SYSTEM_PROMPT to guide LLM output formatting, and supporting infrastructure changes all work together to achieve exactly what the title describes. The title is concise and uses specific terminology ("log viewer links," "query results," "LLM output") that would allow a teammate scanning the history to immediately understand the changeset's purpose. While the title doesn't enumerate every modification (such as timestamp parameter adjustments), this aligns with the guideline that titles need not cover every detail.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
components/clp-mcp-server/tests/test_clp_connector.py (1)

112-122: Test will fail with KeyError due to missing required fields in mock documents.

The test test_read_results_returns_docs will fail when the read_results implementation attempts to access doc["archive_id"] (line 170 of clp_connector.py) and doc["log_event_ix"] (line 172), as these fields are not present in the mock documents [{"_id": "1"}, {"_id": "2"}, {"_id": "3"}].

Additionally, even if those fields existed, the assertion would fail because the implementation modifies each document by adding a "link" field and setting "_id" to None, resulting in a different structure than the input mock documents.

The separate test test_read_results_adds_link_field (defined in the same file) demonstrates the correct approach by including archive_id and log_event_ix fields in mock documents and properly validating the link field generation.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 11d73fb and 66230572065f1b452f82922a7dcb17f964f2d4f8.

📒 Files selected for processing (3)
  • components/clp-mcp-server/clp_mcp_server/clp_connector.py (4 hunks)
  • components/clp-mcp-server/clp_mcp_server/server/constants.py (1 hunks)
  • components/clp-mcp-server/tests/test_clp_connector.py (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/clp-mcp-server/clp_mcp_server/clp_connector.py (1)
components/clp-mcp-server/clp_mcp_server/constants.py (1)
  • QueryJobStatus (18-27)
components/clp-mcp-server/tests/test_clp_connector.py (1)
components/clp-mcp-server/clp_mcp_server/clp_connector.py (2)
  • ClpConnector (22-177)
  • read_results (157-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: package-image
  • GitHub Check: rust-checks (ubuntu-22.04)
  • GitHub Check: rust-checks (ubuntu-24.04)
  • GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (6)
components/clp-mcp-server/tests/test_clp_connector.py (2)

22-23: LGTM!

The webui configuration addition is necessary to support the new Log Viewer link generation feature and the values are appropriate for testing.


125-147: Excellent test coverage for the link field addition.

The test correctly validates that each document receives a properly formatted link field based on the webui configuration and document fields. Good use of strict=True in the zip to ensure both iterables have the same length.

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

15-16: LGTM!

The updated system prompt clearly instructs the AI assistant to wrap log messages in hyperlinks using the newly added link field. This properly integrates the Log Viewer functionality into the user experience.

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

60-62: LGTM!

The timestamp validation correctly checks that end_ts is not before begin_ts when both are provided, with a clear error message.


67-67: LGTM!

Setting the dataset to "default" ensures consistency with the link URL generated in read_results (Line 171).


168-174: Add defensive checks for missing fields or document external schema guarantees.

The code in read_results assumes doc["archive_id"] and doc["log_event_ix"] exist in every MongoDB result document. While test mocks show these fields present, the actual guarantee depends on the external query execution system that populates this collection.

Either:

  1. Add .get() with defaults or explicit existence checks before field access, or
  2. Document (in comments or docstring) that these fields are guaranteed by the query execution service, referencing the relevant schema/contract

Comment thread components/clp-mcp-server/clp_mcp_server/clp_connector.py Outdated
Comment thread components/clp-mcp-server/clp_mcp_server/clp_connector.py
@All-less All-less force-pushed the log-viewer-integration branch from 6623057 to 4e50148 Compare October 20, 2025 20:09

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 66230572065f1b452f82922a7dcb17f964f2d4f8 and 4e50148ccee1b9ae4771aa0474a23549f2d77bd0.

📒 Files selected for processing (3)
  • components/clp-mcp-server/clp_mcp_server/clp_connector.py (4 hunks)
  • components/clp-mcp-server/clp_mcp_server/server/constants.py (1 hunks)
  • components/clp-mcp-server/tests/test_clp_connector.py (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/clp-mcp-server/tests/test_clp_connector.py (1)
components/clp-mcp-server/clp_mcp_server/clp_connector.py (2)
  • ClpConnector (22-174)
  • read_results (154-174)
components/clp-mcp-server/clp_mcp_server/clp_connector.py (1)
components/clp-mcp-server/clp_mcp_server/constants.py (1)
  • QueryJobStatus (18-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: package-image
  • GitHub Check: rust-checks (ubuntu-22.04)
  • GitHub Check: rust-checks (ubuntu-24.04)
  • GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (6)
components/clp-mcp-server/clp_mcp_server/server/constants.py (1)

15-16: LGTM! Clear instruction for displaying log messages as hyperlinks.

The updated system prompt provides clear guidance to the LLM on how to display log messages with the generated links.

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

22-23: LGTM! Proper mock configuration for webui.

The webui configuration is correctly added to support testing of the link generation feature.

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

57-59: LGTM! Correct validation logic for optional timestamps.

The validation properly checks that both timestamps are provided before comparing them, allowing either parameter to be None independently.


64-64: LGTM! Consistent dataset value.

Setting the dataset to "default" ensures consistency with the link generation in read_results (line 168).


165-171: Verify error handling for missing document fields.

The link generation logic is correct and consistent with the test expectations. However, the code assumes archive_id and log_event_ix fields exist in each document. If these fields are missing, a KeyError will be raised.

Please verify whether:

  1. These fields are guaranteed to exist in all documents returned by the query
  2. Error handling should be added to gracefully handle missing fields

If error handling is needed, consider adding a check:

 async for doc in collection.find({}, limit=SEARCH_MAX_NUM_RESULTS):
+    if "archive_id" not in doc or "log_event_ix" not in doc:
+        # Log warning or skip document
+        continue
     doc["link"] = (
         f"{self._webui_addr}/streamFile?type=json"
         f'&streamId={doc["archive_id"]}'
         "&dataset=default"
         f'&logEventIdx={doc["log_event_ix"]}'
     )
     doc["_id"] = None
     results.append(doc)

139-142: LGTM! Improved error message readability.

Using QueryJobStatus(status).name provides more meaningful error messages by displaying the status name rather than just the numeric value.

Comment on lines +125 to +147
@pytest.mark.asyncio
async def test_read_results_adds_link_field(mock_clp_config: Any) -> None:
"""Ensures read_results adds a 'link' field."""
connector = ClpConnector(mock_clp_config)
mock_docs = [
{"_id": "1", "archive_id": "archA", "log_event_ix": 10},
{"_id": "2", "archive_id": "archB", "log_event_ix": 20},
]
mock_collection = AsyncMock()
mock_collection.find = MagicMock(return_value=_aiter(mock_docs))

with patch.object(connector, "_results_cache", {"12": mock_collection}):
results = await connector.read_results("12")

assert len(results) == len(mock_docs)
for original, result in zip(mock_docs, results, strict=True):
expected_link = (
f'http://{mock_clp_config.webui.host}:{mock_clp_config.webui.port}'
f'/streamFile?type=json&streamId={original["archive_id"]}'
f'&dataset=default&logEventIdx={original["log_event_ix"]}'
)
assert result["link"] == expected_link

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Good test coverage, but consider adding error-handling test.

The test correctly verifies that read_results adds the link field with the proper format. However, consider adding a test case to verify behavior when archive_id or log_event_ix fields are missing from documents.

You could add a test like:

@pytest.mark.asyncio
async def test_read_results_missing_required_fields(mock_clp_config: Any) -> None:
    """Tests read_results behavior when required fields are missing."""
    connector = ClpConnector(mock_clp_config)
    mock_docs = [{"_id": "1", "archive_id": "archA"}]  # missing log_event_ix
    mock_collection = AsyncMock()
    mock_collection.find = MagicMock(return_value=_aiter(mock_docs))
    
    with patch.object(connector, "_results_cache", {"12": mock_collection}):
        with pytest.raises(KeyError):
            await connector.read_results("12")
🤖 Prompt for AI Agents
In components/clp-mcp-server/tests/test_clp_connector.py around lines 125 to
147, add a new async test that verifies read_results behavior when required
fields are missing: create a ClpConnector with mock_clp_config, prepare
mock_docs missing either "archive_id" or "log_event_ix", set up
mock_collection.find to return those docs, patch connector._results_cache to
point to the mock_collection, and assert the expected failure (e.g., use
pytest.raises(KeyError) if the implementation currently expects those keys) so
the test fails if read_results silently ignores or mishandles missing fields.

@junhaoliao junhaoliao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes look reasonable to me and the added test looks good. please see the other comments

for the tile, how about:

feat(clp-mcp-server): Add log viewer links to query results to restore log context.

err_msg = f"end_ts {end_ts} is smaller than begin_ts {begin_ts}."
raise ValueError(err_msg)

job_config = msgpack.packb(
{
"begin_timestamp": begin_ts,
"dataset": None,
"dataset": "default",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. i believe a future plan is to also support specifying non-default dataset names when creating the archive?
  2. can we avoid hardcoding this? With build(clp-package): Adopt uv + hatchling as the build and packaging backend for Python components (resolves #1396); Upgrade dependencies for Python components. #1405 i believe we can directly import CLP_DEFAULT_DATASET_NAME from clp_py_utils.clp_config

doc["link"] = (
f"{self._webui_addr}/streamFile?type=json"
f'&streamId={doc["archive_id"]}'
"&dataset=default"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto - we shall avoid hardcoding "default"

Comment thread components/clp-mcp-server/clp_mcp_server/clp_connector.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e50148ccee1b9ae4771aa0474a23549f2d77bd0 and ea2b1b9a806b42b6845be9d44666ae65202e4458.

📒 Files selected for processing (3)
  • components/clp-mcp-server/clp_mcp_server/clp_connector.py (5 hunks)
  • components/clp-mcp-server/clp_mcp_server/server/constants.py (1 hunks)
  • components/clp-mcp-server/tests/test_clp_connector.py (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/clp-mcp-server/clp_mcp_server/clp_connector.py (1)
components/clp-mcp-server/clp_mcp_server/constants.py (1)
  • QueryJobStatus (18-27)
components/clp-mcp-server/tests/test_clp_connector.py (1)
components/clp-mcp-server/clp_mcp_server/clp_connector.py (2)
  • ClpConnector (23-175)
  • read_results (155-175)
🔇 Additional comments (10)
components/clp-mcp-server/clp_mcp_server/server/constants.py (1)

15-16: LGTM! Clear instruction for hyperlink wrapping.

The addition to the system prompt clearly instructs the LLM to wrap log messages in hyperlinks using the link field from search results, which aligns perfectly with the link field being added in read_results.

components/clp-mcp-server/tests/test_clp_connector.py (3)

22-22: Clarify the database host change.

The database host was changed from "db" to "database". Is this change related to the Log Viewer integration, or is it an unrelated correction?


23-23: LGTM! WebUI configuration added for link generation.

The webui configuration is correctly added to support testing the link generation feature.


125-147: Good test coverage. Consider adding error-handling test for missing fields.

The test correctly verifies that read_results adds the link field with the proper format. However, consider adding a test case to verify behaviour when archive_id or log_event_ix fields are missing from documents, as these fields are accessed without validation in the implementation.

You could add a test like:

@pytest.mark.asyncio
async def test_read_results_missing_required_fields(mock_clp_config: Any) -> None:
    """Tests read_results behaviour when required fields are missing."""
    connector = ClpConnector(mock_clp_config)
    mock_docs = [{"_id": "1", "archive_id": "archA"}]  # missing log_event_ix
    mock_collection = AsyncMock()
    mock_collection.find = MagicMock(return_value=_aiter(mock_docs))
    
    with patch.object(connector, "_results_cache", {"12": mock_collection}):
        with pytest.raises(KeyError):
            await connector.read_results("12")
components/clp-mcp-server/clp_mcp_server/clp_connector.py (6)

41-41: LGTM! WebUI address initialization is correct.

The initialization of self._webui_addr from the configuration is correct and necessary for generating Log Viewer links. Note that in production environments with reverse proxies, this address may not be externally reachable, but this is a known limitation tracked in issue #1458.


43-45: LGTM! Backward-compatible signature update.

The updated method signature to support optional timestamp parameters is backward compatible and well-documented, enabling time-range filtering for queries.


58-60: LGTM! Correct conditional validation.

The validation correctly checks the timestamp relationship only when both begin_ts and end_ts are provided, which is appropriate given both parameters are optional.


65-65: LGTM! Proper use of dataset constant.

The change correctly uses CLP_DEFAULT_DATASET_NAME instead of hardcoding the dataset name, addressing previous feedback.


140-143: LGTM! Improved error message formatting.

The error message now includes the human-readable status name via QueryJobStatus(status).name, which improves debugging and logging clarity.


172-172: Consider whether _id should be deleted instead of set to None.

Setting doc["_id"] = None preserves the key with a null value. If the intent is to remove MongoDB's internal ID from the response, consider using del doc["_id"] instead to remove the field entirely.

Which approach is preferred for the API consumers?

Comment thread components/clp-mcp-server/clp_mcp_server/clp_connector.py Outdated
Comment on lines +166 to +172
doc["link"] = (
f"{self._webui_addr}/streamFile?type=json"
f'&streamId={doc["archive_id"]}'
f"&dataset={CLP_DEFAULT_DATASET_NAME}"
f'&logEventIdx={doc["log_event_ix"]}'
)
doc["_id"] = None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Verify handling of missing required fields.

The link generation looks correct, but the code assumes archive_id and log_event_ix exist in every document. If these fields are missing, a KeyError will be raised. Consider adding validation or documenting this requirement.

Run the following script to check if documents in the results cache always contain these fields:


🏁 Script executed:

#!/bin/bash
# Description: Search for document schema documentation or validation

# Search for archive_id and log_event_ix field usage patterns
rg -n -C3 'archive_id|log_event_ix' --type=py -g '!tests/'

# Look for document schema definitions or validation
ast-grep --pattern 'doc = {
  $$$
}'

Length of output: 72554



🏁 Script executed:

#!/bin/bash
# Search for where documents are inserted into the results cache
# to understand the source data structure

rg -n -C5 'collection\.insert|collection\.update' --type=py -g '!tests/'

Length of output: 0



🏁 Script executed:

#!/bin/bash
# Search for where log_event_ix is set or used
# This field appears in the streamFile link, so trace back its origin

rg -n 'log_event_ix' --type=py -g '!tests/'

# Also search for how search results are prepared/transformed
rg -n 'results\.append\|results_cache\|add.*result' --type=py -C2 -g '!tests/' | head -50

Length of output: 115



🏁 Script executed:

#!/bin/bash
# Search for search task implementation that writes to results cache
# Also look for fs_search_task or similar

rg -n 'def search\|class.*Search.*Task' --type=py -g '!tests/' -A5

# Look for where results are written to MongoDB results cache
rg -n 'results_collection\|results.*insert' --type=py -C3 -g '!tests/' | head -80

Length of output: 0



🏁 Script executed:

#!/bin/bash
# Search in test files for expected document structure
fd -e py -path '*/test*' | xargs rg -l 'archive_id.*log_event_ix|log_event_ix.*archive_id' 2>/dev/null

# Look for mock documents or fixture data in tests
fd -e py -path '*/test*' -type f | xargs rg -A10 'archive_id' 2>/dev/null | grep -A10 'log_event_ix' | head -40

# Check clp_connector for any test files
fd -e py 'clp_connector' | grep -i test

Length of output: 4041



🏁 Script executed:

#!/bin/bash
# Read the full test file for clp_connector
cat components/clp-mcp-server/tests/test_clp_connector.py

# Also read the full clp_connector.py to see read_results implementation
cat components/clp-mcp-server/clp_mcp_server/clp_connector.py | head -200

Length of output: 12412


Add error handling or document field requirements for archive_id and log_event_ix.

The concern is valid. The code at lines 168 and 170 unconditionally accesses doc["archive_id"] and doc["log_event_ix"] without checking field existence. While the test suite assumes these fields exist, MongoDB's flexible schema means a KeyError could be raised if either field is missing from any document.

Recommend one of:

  • Add a try-catch block or field validation before accessing these fields
  • Document the data contract explicitly (that these fields are guaranteed by the search engine)
  • Add a helper method to safely extract these fields with graceful error handling
🤖 Prompt for AI Agents
In components/clp-mcp-server/clp_mcp_server/clp_connector.py around lines 166 to
172, the code unconditionally reads doc["archive_id"] and doc["log_event_ix"];
add defensive handling to avoid KeyError by validating those keys before use:
either check for both keys (if missing, log a warning and skip building the link
or set doc["_id"]=None and continue), or wrap the access in a try/except that
logs the document id/context and handles the missing-field case gracefully
(e.g., omit the link or populate with a safe default); update or add a brief
comment documenting that the search engine must supply archive_id and
log_event_ix if you choose to rely on the contract.

@All-less All-less force-pushed the log-viewer-integration branch from ea2b1b9 to a0c0147 Compare October 21, 2025 18:09

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ea2b1b9a806b42b6845be9d44666ae65202e4458 and a0c014745cd007d0c26973f984437ebd3408bd50.

📒 Files selected for processing (3)
  • components/clp-mcp-server/clp_mcp_server/clp_connector.py (5 hunks)
  • components/clp-mcp-server/clp_mcp_server/server/constants.py (1 hunks)
  • components/clp-mcp-server/tests/test_clp_connector.py (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/clp-mcp-server/tests/test_clp_connector.py (1)
components/clp-mcp-server/clp_mcp_server/clp_connector.py (2)
  • ClpConnector (23-175)
  • read_results (155-175)
components/clp-mcp-server/clp_mcp_server/clp_connector.py (1)
components/clp-mcp-server/clp_mcp_server/constants.py (1)
  • QueryJobStatus (18-27)
🔇 Additional comments (9)
components/clp-mcp-server/clp_mcp_server/server/constants.py (1)

15-16: LGTM! Clear instruction for LLM to generate hyperlinks.

The updated prompt clearly instructs the LLM to wrap log messages in hyperlinks using the link field from search results, which aligns with the link field being added to results in clp_connector.py.

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

22-23: LGTM! Mock configuration updated appropriately.

The webui configuration addition is necessary for testing link generation, and the database host update maintains consistency with test naming conventions.


125-147: LGTM! Test correctly verifies link field generation.

The test properly validates that read_results adds the link field with the correct URL format. Good use of strict=True in the zip to ensure both iterables have the same length.

Note: A past review comment suggested adding a test for error handling when archive_id or log_event_ix fields are missing. Consider addressing that suggestion in a follow-up to improve test coverage for edge cases.

Based on learnings

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

41-41: LGTM! WebUI address initialization is correct.

The initialization properly constructs the WebUI address from configuration. Note that in production environments with reverse proxies, this address may not be directly reachable by external clients. This limitation is tracked in issue #1458.

Based on learnings


43-45: LGTM! Backward-compatible signature enhancement.

The optional timestamp parameters enable time-range filtering while maintaining backward compatibility. The type hints are correct and the parameters are well-documented.


58-58: LGTM! Improved validation logic.

The updated validation correctly checks that both timestamps are not None before comparing them, which prevents unnecessary validation when only one timestamp is provided.


65-65: LGTM! Using constant avoids hardcoding.

Good use of CLP_DEFAULT_DATASET_NAME instead of hardcoding the dataset name, which improves maintainability.


140-143: LGTM! Enhanced error message readability.

Using QueryJobStatus(status).name provides more readable status names in error messages, improving debugging experience.


166-172: Verify whether archive_id and log_event_ix fields are schema-guaranteed, or apply defensive handling.

The read_results() method directly accesses doc["archive_id"] and doc["log_event_ix"] without validation. These documents originate from the external CLP query engine stored in MongoDB. If the CLP schema does not guarantee these fields are always present, a KeyError will occur.

Confirm with your CLP schema documentation whether these fields are guaranteed. If not certain, apply defensive handling using .get() with None checks as suggested in the earlier diff:

     async for doc in collection.find({}, limit=SEARCH_MAX_NUM_RESULTS):
-        doc["link"] = (
-            f"{self._webui_addr}/streamFile?type=json"
-            f'&streamId={doc["archive_id"]}'
-            f"&dataset={CLP_DEFAULT_DATASET_NAME}"
-            f'&logEventIdx={doc["log_event_ix"]}'
-        )
+        archive_id = doc.get("archive_id")
+        log_event_ix = doc.get("log_event_ix")
+        if archive_id is not None and log_event_ix is not None:
+            doc["link"] = (
+                f"{self._webui_addr}/streamFile?type=json"
+                f'&streamId={archive_id}'
+                f"&dataset={CLP_DEFAULT_DATASET_NAME}"
+                f'&logEventIdx={log_event_ix}'
+            )
         doc["_id"] = None
         results.append(doc)

Comment thread components/clp-mcp-server/clp_mcp_server/clp_connector.py Outdated
@All-less All-less force-pushed the log-viewer-integration branch from a0c0147 to afd0fea Compare October 21, 2025 18:20
@All-less All-less requested a review from junhaoliao October 21, 2025 19:04

@junhaoliao junhaoliao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would the proposed title at #1454 (review) be more clear on the purpose of the changes? (attaching here for convenience)

feat(clp-mcp-server): Add log viewer links to query results to restore log context.

@All-less

Copy link
Copy Markdown
Contributor Author

the changes look reasonable to me and the added test looks good. please see the other comments

for the tile, how about:

feat(clp-mcp-server): Add log viewer links to query results to restore log context.

In the context of MCP server, "restore log context" is too vague a description. We could use the following title to emphasize the purpose.

feat(clp-mcp-server): Add log viewer links to query results for displaying in LLM output.

@All-less All-less changed the title feat(clp-mcp-server): Add links to Log Viewer in LLM diagnosis result feat(clp-mcp-server): Add log viewer links to query results for displaying in LLM output. Oct 21, 2025
@All-less All-less merged commit fa375d7 into y-scope:main Oct 21, 2025
22 checks passed
LinZhihao-723 added a commit to LinZhihao-723/clp that referenced this pull request Oct 22, 2025
* feat(webui): Add drawer to display guided query and errors. (y-scope#1421)

* docs: Add Slack community invite badge to home page README. (y-scope#1418)

* refactor(clp-package): Simplify StrEnum and Path serialization via Annotated serializers. (y-scope#1417)

Co-authored-by: Junhao Liao <junhao@junhao.ca>
Co-authored-by: Junhao Liao <junhao.liao@yscope.com>

* build(clp-package): Adopt uv + hatchling as the build and packaging backend for Python components (resolves y-scope#1396); Upgrade dependencies for Python components. (y-scope#1405)

* chore(docker): Add `--link` flags to COPY/ADD operations for improved build performance (fixes y-scope#1408). (y-scope#1411)

* fix(ci): Correctly update and restore cache of `lint:check-cpp-lint-static-full`'s generated files (fixes y-scope#1419): (y-scope#1430)

- Save cache entries using unique key per entry.
- Restore latest entries using key prefix.
- Avoid using outputs from optionally-run `restore` task.

* fix(clp-rust-utils): Use AWS SDK default configuration with latest behavior version for S3 client. (y-scope#1445)

Co-authored-by: Junhao Liao <junhao.liao@yscope.com>

* refactor(clp-package): Remove unused `python-dotenv` dependency and related imports (fixes y-scope#1443). (y-scope#1444)

* fix(webui): Submit queries that failed ANTLR validation to Presto.  (y-scope#1450)

* feat(clp-s): Explicitly reject unstructured log inputs during compression. (y-scope#1434)

* feat(webui): Show query speed in native search status. (y-scope#1429)

* fix(job-orchestration): Make `tag_ids` a required `list[int]` for compatibility with Spider compressor. (y-scope#1453)

* feat(clp-mcp-server): Add log viewer links to query results for displaying in LLM output. (y-scope#1454)

Co-authored-by: Junhao Liao <junhao.liao@yscope.com>

* feat(ci): Add tasks for checking and updating Rust lock file (`Cargo.lock`); Add check to GH workflow. (y-scope#1448)

* feat(webui): Trigger submit action when pressing Enter on Monaco single line editor. (y-scope#1459)

* Add filters.

* Update cargo lock.

---------

Co-authored-by: davemarco <83603688+davemarco@users.noreply.github.com>
Co-authored-by: Abigail Matthews <abigail.v.matthews@gmail.com>
Co-authored-by: sitaowang1998 <sitaowang1998@outlook.com>
Co-authored-by: Junhao Liao <junhao@junhao.ca>
Co-authored-by: Junhao Liao <junhao.liao@yscope.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: Devin Gibson <gibber9809@users.noreply.github.com>
Co-authored-by: hoophalab <200652805+hoophalab@users.noreply.github.com>
Co-authored-by: Huangshi Tian <All-less@users.noreply.github.com>
LinZhihao-723 added a commit to LinZhihao-723/clp that referenced this pull request Oct 22, 2025
* feat(webui): Add drawer to display guided query and errors. (y-scope#1421)

* docs: Add Slack community invite badge to home page README. (y-scope#1418)

* refactor(clp-package): Simplify StrEnum and Path serialization via Annotated serializers. (y-scope#1417)

Co-authored-by: Junhao Liao <junhao@junhao.ca>
Co-authored-by: Junhao Liao <junhao.liao@yscope.com>

* build(clp-package): Adopt uv + hatchling as the build and packaging backend for Python components (resolves y-scope#1396); Upgrade dependencies for Python components. (y-scope#1405)

* chore(docker): Add `--link` flags to COPY/ADD operations for improved build performance (fixes y-scope#1408). (y-scope#1411)

* fix(ci): Correctly update and restore cache of `lint:check-cpp-lint-static-full`'s generated files (fixes y-scope#1419): (y-scope#1430)

- Save cache entries using unique key per entry.
- Restore latest entries using key prefix.
- Avoid using outputs from optionally-run `restore` task.

* fix(clp-rust-utils): Use AWS SDK default configuration with latest behavior version for S3 client. (y-scope#1445)

Co-authored-by: Junhao Liao <junhao.liao@yscope.com>

* refactor(clp-package): Remove unused `python-dotenv` dependency and related imports (fixes y-scope#1443). (y-scope#1444)

* fix(webui): Submit queries that failed ANTLR validation to Presto.  (y-scope#1450)

* feat(clp-s): Explicitly reject unstructured log inputs during compression. (y-scope#1434)

* feat(webui): Show query speed in native search status. (y-scope#1429)

* fix(job-orchestration): Make `tag_ids` a required `list[int]` for compatibility with Spider compressor. (y-scope#1453)

* feat(clp-mcp-server): Add log viewer links to query results for displaying in LLM output. (y-scope#1454)

Co-authored-by: Junhao Liao <junhao.liao@yscope.com>

* feat(ci): Add tasks for checking and updating Rust lock file (`Cargo.lock`); Add check to GH workflow. (y-scope#1448)

* feat(webui): Trigger submit action when pressing Enter on Monaco single line editor. (y-scope#1459)

* Add filters.

* Update cargo lock.

---------

Co-authored-by: davemarco <83603688+davemarco@users.noreply.github.com>
Co-authored-by: Abigail Matthews <abigail.v.matthews@gmail.com>
Co-authored-by: sitaowang1998 <sitaowang1998@outlook.com>
Co-authored-by: Junhao Liao <junhao@junhao.ca>
Co-authored-by: Junhao Liao <junhao.liao@yscope.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: Devin Gibson <gibber9809@users.noreply.github.com>
Co-authored-by: hoophalab <200652805+hoophalab@users.noreply.github.com>
Co-authored-by: Huangshi Tian <All-less@users.noreply.github.com>
LinZhihao-723 added a commit to LinZhihao-723/clp that referenced this pull request Oct 22, 2025
* feat(webui): Add drawer to display guided query and errors. (y-scope#1421)

* docs: Add Slack community invite badge to home page README. (y-scope#1418)

* refactor(clp-package): Simplify StrEnum and Path serialization via Annotated serializers. (y-scope#1417)

Co-authored-by: Junhao Liao <junhao@junhao.ca>
Co-authored-by: Junhao Liao <junhao.liao@yscope.com>

* build(clp-package): Adopt uv + hatchling as the build and packaging backend for Python components (resolves y-scope#1396); Upgrade dependencies for Python components. (y-scope#1405)

* chore(docker): Add `--link` flags to COPY/ADD operations for improved build performance (fixes y-scope#1408). (y-scope#1411)

* fix(ci): Correctly update and restore cache of `lint:check-cpp-lint-static-full`'s generated files (fixes y-scope#1419): (y-scope#1430)

- Save cache entries using unique key per entry.
- Restore latest entries using key prefix.
- Avoid using outputs from optionally-run `restore` task.

* fix(clp-rust-utils): Use AWS SDK default configuration with latest behavior version for S3 client. (y-scope#1445)

Co-authored-by: Junhao Liao <junhao.liao@yscope.com>

* refactor(clp-package): Remove unused `python-dotenv` dependency and related imports (fixes y-scope#1443). (y-scope#1444)

* fix(webui): Submit queries that failed ANTLR validation to Presto.  (y-scope#1450)

* feat(clp-s): Explicitly reject unstructured log inputs during compression. (y-scope#1434)

* feat(webui): Show query speed in native search status. (y-scope#1429)

* fix(job-orchestration): Make `tag_ids` a required `list[int]` for compatibility with Spider compressor. (y-scope#1453)

* feat(clp-mcp-server): Add log viewer links to query results for displaying in LLM output. (y-scope#1454)

Co-authored-by: Junhao Liao <junhao.liao@yscope.com>

* feat(ci): Add tasks for checking and updating Rust lock file (`Cargo.lock`); Add check to GH workflow. (y-scope#1448)

* feat(webui): Trigger submit action when pressing Enter on Monaco single line editor. (y-scope#1459)

* Add filters.

* Update cargo lock.

* Stupid fix

---------

Co-authored-by: davemarco <83603688+davemarco@users.noreply.github.com>
Co-authored-by: Abigail Matthews <abigail.v.matthews@gmail.com>
Co-authored-by: sitaowang1998 <sitaowang1998@outlook.com>
Co-authored-by: Junhao Liao <junhao@junhao.ca>
Co-authored-by: Junhao Liao <junhao.liao@yscope.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: Devin Gibson <gibber9809@users.noreply.github.com>
Co-authored-by: hoophalab <200652805+hoophalab@users.noreply.github.com>
Co-authored-by: Huangshi Tian <All-less@users.noreply.github.com>
junhaoliao added a commit to junhaoliao/clp that referenced this pull request May 17, 2026
…aying in LLM output. (y-scope#1454)

Co-authored-by: Junhao Liao <junhao.liao@yscope.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants