Python: Improvements for MCP#14003
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR enhances the Python MCP connector to (1) optionally gate MCP “sampling” requests behind a user-provided async consent callback and (2) avoid tool/prompt name collisions with existing plugin attributes, with accompanying unit tests.
Changes:
- Add
sampling_consent_callback(async) to MCP plugin constructors and enforce it insampling_callback(). - Add name-conflict detection to prevent MCP tool/prompt names from overwriting existing plugin attributes.
- Add unit tests covering sampling consent denial, default auto-approval warning logging, and reserved-name collision behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| python/semantic_kernel/connectors/mcp.py | Introduces optional sampling consent callback + warning behavior; adds conflict checks when materializing MCP tools/prompts. |
| python/tests/unit/connectors/mcp/test_mcp.py | Adds tests for consent denial, warning logging when no callback is configured, and prevention of attribute shadowing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _has_mcp_function_name_conflict(self, item_type: str, remote_name: str, local_name: str) -> bool: | ||
| if not hasattr(self, local_name): | ||
| return False | ||
| logger.warning( | ||
| "Skipping MCP %s '%s' because normalized name '%s' conflicts with an existing plugin attribute.", | ||
| item_type, | ||
| remote_name, | ||
| local_name, | ||
| ) | ||
| return True |
There was a problem hiding this comment.
Addressed in 448b887: reserved attribute names are now snapshotted before MCP-generated attributes are added, so reloads can replace existing MCP functions without warning spam.
| elif not await self.sampling_consent_callback(self.name, params): | ||
| return types.ErrorData( | ||
| code=types.INTERNAL_ERROR, | ||
| message="Sampling denied by policy.", | ||
| ) |
There was a problem hiding this comment.
Addressed in 448b887: consent callback exceptions are logged and treated as policy denial, returning ErrorData instead of propagating through the MCP session.
| @@ -487,6 +534,8 @@ async def load_tools(self): | |||
| # Create methods with the kernel_function decorator for each tool | |||
There was a problem hiding this comment.
Addressed in 448b887: moved the comment to directly precede the load_tools loop.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Motivation and Context
Improvements for MCP tool calls by adding an optional sampling callback.
Description
Improvements for MCP tool calls by adding an optional sampling callback.
Contribution Checklist