feat: add remove_prompt() and remove_resource() for parity with remove_tool()#2333
Conversation
…e_tool() Add methods to dynamically remove prompts and resources, matching the existing remove_tool() functionality: - Add PromptError exception class for prompt operation errors - Add PromptManager.remove_prompt(name) - raises PromptError if not found - Add ResourceManager.remove_resource(uri) - raises ResourceError if not found - Add ResourceManager.remove_template(uri_template) - raises ResourceError - Add MCPServer.remove_prompt(name) - thin wrapper to prompt manager - Add MCPServer.remove_resource(uri) - thin wrapper to resource manager - Add MCPServer.remove_resource_template(uri_template) - thin wrapper This enables multi-tenant/multi-instance deployments where servers need to dynamically manage their registered primitives. Fixes modelcontextprotocol#2331
rgoldstein1989
left a comment
There was a problem hiding this comment.
Thanks for picking this up! A couple of things I noticed:
Missing tests
The project requires 100% code coverage (fail_under = 100 in CI), and CONTRIBUTING.md states "Add tests for new functionality." This PR will fail CI without tests. For reference, the existing remove_tool() has tests in both test_tool_manager.py and test_server.py covering:
- Removing an existing item
- Removing a non-existent item (error path)
- Removing one item from multiple and verifying
list_*results - Calling/reading a removed item through the client (verifying the protocol-level error)
The same coverage would be needed for remove_prompt, remove_resource, and remove_resource_template.
remove_resource uri type
The uri parameter on ResourceManager.remove_resource() is typed as str, but resources are stored via str(resource.uri) where resource.uri can be AnyUrl. If a caller passes an AnyUrl object, the lookup will fail silently (no match in the dict) and raise ResourceError even though the resource exists.
The fix is to accept AnyUrl | str and normalize before lookup, matching how the rest of the manager handles URIs:
def remove_resource(self, uri: AnyUrl | str) -> None:
uri_str = str(uri)
if uri_str not in self._resources:
raise ResourceError(f"Unknown resource: {uri}")
del self._resources[uri_str]get_resource() already does this same str(uri) normalization at the top of the method.
- Fix remove_resource() to accept AnyUrl | str and normalize with str() - Add comprehensive tests for remove_resource() and remove_template() - Add tests for remove_prompt() - All tests cover: removing existing, removing non-existent (error path), removing from multiple, and verifying protocol-level errors
|
@rgoldstein1989 Thanks for the detailed review! I've addressed both issues:
All tests should now pass. Please take another look when you have time! |
Add comprehensive tests in test_server.py following the same pattern as remove_tool tests: - test_remove_prompt: basic removal from server - test_remove_nonexistent_prompt: error path - test_remove_prompt_and_list: verify client list_prompts reflects removal - test_remove_resource: basic removal from server - test_remove_nonexistent_resource: error path - test_remove_resource_and_list: verify client list_resources reflects removal - test_remove_resource_template: template removal - test_remove_nonexistent_template: template error path These tests verify the protocol-level behavior when resources/prompts are removed, ensuring clients see the changes correctly.
Summary
MCPServer exposes
remove_tool(name)but has no equivalent for prompts or resources.This was part of the original ask in #711, which was closed when
remove_tool()landed — but the resource and prompt sides were never addressed.
Changes
add_*remove_*API Additions
PromptManager.remove_prompt(name)— raisesPromptErrorif not foundResourceManager.remove_resource(uri)— raisesResourceErrorif not foundResourceManager.remove_template(uri_template)— raisesResourceErrorif not foundMCPServer.remove_prompt(name)— thin wrapper delegating to the managerMCPServer.remove_resource(uri)— thin wrapper delegating to the managerMCPServer.remove_resource_template(uri_template)— thin wrapperPromptErrorexception class inexceptions.pyPurely additive, no breaking changes.
Use Case
Multi-tenant / multi-instance deployments where the same server image serves
different clients. Users can now filter all primitives per-instance without
reaching into private internals.
Fixes #2331