OBSINTA-1383: filter MCP tools by available UI extensions on client#2952
OBSINTA-1383: filter MCP tools by available UI extensions on client#2952tremes wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@tremes: This pull request references OBSINTA-1383 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughAdds ChangesAvailable Tool UI ID Filtering
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint as ols.py generate_response
participant Summarizer as DocsSummarizer
participant MCP as get_mcp_tools
participant Utils as filter_tools_by_available_ui
Client->>Endpoint: request with available_tool_ui_ids
Endpoint->>Summarizer: construct(available_tool_ui_ids)
Summarizer->>MCP: get_mcp_tools()
MCP-->>Summarizer: tools
Summarizer->>Utils: filter_tools_by_available_ui(tools, ids)
Utils-->>Summarizer: filtered tools
Summarizer-->>Endpoint: response generation continues
Endpoint-->>Client: final response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
tremes
force-pushed
the
filter-available-ui-tools
branch
from
09a39b3 to
c979fc5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/utils/test_mcp_utils.py (1)
653-717: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest names omit the function under test.
Per repository guidelines, test naming should follow
test_<function>_<scenario>, but these methods (e.g.test_none_available_ids_returns_all) dropfilter_tools_by_available_uifrom the name. The class name provides some context, but explicit naming would improve traceability in test-failure output.As per coding guidelines, "Follow the test naming convention
test_<function>_<scenario>."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/utils/test_mcp_utils.py` around lines 653 - 717, The test method names in TestFilterToolsByAvailableUI do not follow the required test_<function>_<scenario> pattern. Rename each test to include filter_tools_by_available_ui in the method name, keeping the scenario-specific suffixes, so failures are easier to trace back to the function under test.Source: Coding guidelines
ols/src/query_helpers/docs_summarizer.py (1)
431-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a DocsSummarizer-level test for the new filtering wiring.
The provided cohort only includes unit tests for
filter_tools_by_available_uiitself; there's no test verifying thatDocsSummarizer.generate_responseactually calls it withself.available_tool_ui_idsand applies the filtered tool list downstream. Given this sits on a critical tool-calling path, a focused test would guard against future regressions (e.g., accidental removal or reordering relative to the skill-support-tool append at Line 438).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ols/src/query_helpers/docs_summarizer.py` around lines 431 - 436, Add a focused DocsSummarizer-level test around generate_response to verify the new filtering wiring is exercised end-to-end. The test should confirm that after get_mcp_tools returns tools, filter_tools_by_available_ui is called with self.available_tool_ui_ids and that the filtered list is the one passed downstream, including before any later skill-support-tool append logic in generate_response. Use the DocsSummarizer.generate_response flow and the filter_tools_by_available_ui call site as the key symbols to anchor the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ols/utils/mcp_utils.py`:
- Around line 204-218: The tool filtering logic in `mcp_utils` can crash when
`_meta["olsUi"]` is `None` or not a dict because `meta.get("olsUi",
{}).get("id")` assumes a mapping. Update the filtering code around
`meta`/`ols_ui_id` to defensively validate `olsUi` the same way `meta` is
checked, only reading `"id"` when `olsUi` is a dict and otherwise treating it as
missing before the `available_set` check.
---
Nitpick comments:
In `@ols/src/query_helpers/docs_summarizer.py`:
- Around line 431-436: Add a focused DocsSummarizer-level test around
generate_response to verify the new filtering wiring is exercised end-to-end.
The test should confirm that after get_mcp_tools returns tools,
filter_tools_by_available_ui is called with self.available_tool_ui_ids and that
the filtered list is the one passed downstream, including before any later
skill-support-tool append logic in generate_response. Use the
DocsSummarizer.generate_response flow and the filter_tools_by_available_ui call
site as the key symbols to anchor the test.
In `@tests/unit/utils/test_mcp_utils.py`:
- Around line 653-717: The test method names in TestFilterToolsByAvailableUI do
not follow the required test_<function>_<scenario> pattern. Rename each test to
include filter_tools_by_available_ui in the method name, keeping the
scenario-specific suffixes, so failures are easier to trace back to the function
under test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4c4a5d65-c656-49fc-bf32-a6eae7451b3b
📒 Files selected for processing (6)
ols/app/endpoints/ols.pyols/app/models/models.pyols/src/query_helpers/docs_summarizer.pyols/utils/mcp_utils.pytests/unit/app/models/test_models.pytests/unit/utils/test_mcp_utils.py
Add available_tool_ui_ids field to LLMRequest so clients can report which console UI extensions are installed. Tools declaring an olsUi.id in their metadata are excluded when the corresponding extension is not available, preventing the LLM from calling tools the client cannot render. Signed-off-by: Tomáš Remeš <tremes@redhat.com> Assisted-by: Claude Code:claude-opus-4-6
tremes
force-pushed
the
filter-available-ui-tools
branch
from
c979fc5 to
93221f4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/openapi.json (1)
1569-1584: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSchema field added correctly, but docstring/examples not updated.
The new
available_tool_ui_idsproperty is correctly added to the schema, but theLLMRequestdescription (Line 1592) still only documents the previous attributes and omitsavailable_tool_ui_ids; theexamplesblock also has no sample value for it. Since this file is generated from theLLMRequestdocstring inols/app/models/models.py, update that docstring's Attributes list to keep the generated spec accurate.Also applies to: 1592-1592
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/openapi.json` around lines 1569 - 1584, The new available_tool_ui_ids schema field is present in openapi.json, but the source LLMRequest docstring in models.py still omits it from the Attributes list and examples, so update that docstring to include available_tool_ui_ids alongside the other request fields and add a representative example value for it; this will regenerate the openapi spec with matching documentation and samples.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/openapi.json`:
- Around line 1569-1584: The new available_tool_ui_ids schema field is present
in openapi.json, but the source LLMRequest docstring in models.py still omits it
from the Attributes list and examples, so update that docstring to include
available_tool_ui_ids alongside the other request fields and add a
representative example value for it; this will regenerate the openapi spec with
matching documentation and samples.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 643350ca-382c-4188-8064-0833c386f148
📒 Files selected for processing (7)
docs/openapi.jsonols/app/endpoints/ols.pyols/app/models/models.pyols/src/query_helpers/docs_summarizer.pyols/utils/mcp_utils.pytests/unit/app/models/test_models.pytests/unit/utils/test_mcp_utils.py
🚧 Files skipped from review as they are similar to previous changes (6)
- ols/app/models/models.py
- ols/utils/mcp_utils.py
- tests/unit/utils/test_mcp_utils.py
- tests/unit/app/models/test_models.py
- ols/src/query_helpers/docs_summarizer.py
- ols/app/endpoints/ols.py
|
@tremes: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Add available_tool_ui_ids field to LLMRequest so clients can report which console UI extensions are installed. Tools declaring an olsUi.id in their metadata are excluded when the corresponding extension is not available, preventing the LLM from calling tools the client cannot render.
This depends on openshift/lightspeed-console#2058
Assisted-by: Claude Code:claude-opus-4-6
Description
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
available_tool_ui_idsfield to requests (including OpenAPI), allowing clients to specify which tool UIs are available.Bug Fixes
available_tool_ui_idsis omitted (null) or provided as an empty list—behavior is preserved correctly.Tests