-
Notifications
You must be signed in to change notification settings - Fork 492
Fix MCP tool UI display #1462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix MCP tool UI display #1462
Conversation
WalkthroughSwitches short-name extraction to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
7b2ff88 to
7d88ba1
Compare
willkill07
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with minor feedback
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/nat/front_ends/fastapi/test_mcp_client_endpoint.py (1)
146-150: Inconsistent separator usage with the first test.This test still uses the legacy separator (
.) in the function keys at lines 148-149, whiletest_mcp_client_tool_list_success_with_aliaswas updated to useFunctionGroup.SEPARATOR(__). For consistency with the PR's goal of switching to the new separator, consider updating these keys as well.Suggested fix
group_name = "mcp_math" group_instance = _GroupInstanceStub(client, { - f"{group_name}.calculator__add": _FnStub("Add"), - f"{group_name}.calculator__subtract": _FnStub("Subtract"), + f"{group_name}{FunctionGroup.SEPARATOR}calculator__add": _FnStub("Add"), + f"{group_name}{FunctionGroup.SEPARATOR}calculator__subtract": _FnStub("Subtract"), })Alternatively, if testing legacy separator support is intentional, consider renaming the test or adding a comment to clarify this intent.
|
/merge |
Description
Tools were not being displayed in the UI for
mcp_clientfunction groups. This PR fixes the issue by updating the separator from period to dunder.By Submitting this PR I confirm:
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.