Refactor visibility to mark-based enabled system#2912
Conversation
Rename Visibility to Enabled, collapse VisibilityRule into the transform, and move enabled filtering from Provider to Server level so server-level transforms can override provider-level disables.
marvin-context-protocol
Bot
added
enhancement
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ba6ea5a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not enabled: | ||
| self.disable(name=tool.name) |
There was a problem hiding this comment.
Here enabled=False calls disable(name=tool.name) without components or version, but Enabled._matches treats a name-only rule as matching all component types and all versions. If you have multiple versions of the same tool (or a prompt/resource sharing the same name), disabling one tool via the decorator will also disable those other components, which is a behavior regression from the prior key-based disable. Consider passing components=["tool"] and version=tool.version (or equivalent) to keep the disable scoped to the intended tool instance.
Useful? React with 👍 / 👎.
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR replaces the Visibility system with a mark-based Enabled transform that annotates components via internal metadata and defers final filtering to providers/servers. The Visibility transform module and ComponentService subsystem were removed; a new Enabled transform and is_enabled helper were added and re-exported. Provider.enable/disable signatures changed to accept names, version, tags, components and return Self (with an Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
docs/docs.json (1)
586-610: Consider adding redirects for old visibility paths.The
redirectssection handles several legacy paths but doesn't include redirects from the old visibility URLs to the new enabled URLs. Users or external links referencing/servers/visibilityor/python-sdk/fastmcp-server-transforms-visibilitywould receive 404 errors.♻️ Suggested redirects to add
{ "destination": "/servers/providers/transforms", "source": "/patterns/tool-transformation" + }, + { + "destination": "/servers/enabled", + "source": "/servers/visibility" + }, + { + "destination": "/python-sdk/fastmcp-server-transforms-enabled", + "source": "/python-sdk/fastmcp-server-transforms-visibility" } ],src/fastmcp/server/transforms/__init__.py (1)
234-237: Consider removing unusednoqadirectives.Static analysis indicates the
# noqa: E402comments are unnecessary on these import lines. If E402 (module level import not at top of file) is not enabled in your linting configuration, these directives serve no purpose.🧹 Proposed cleanup
-from fastmcp.server.transforms.enabled import Enabled, is_enabled # noqa: E402 -from fastmcp.server.transforms.namespace import Namespace # noqa: E402 -from fastmcp.server.transforms.tool_transform import ToolTransform # noqa: E402 -from fastmcp.server.transforms.version_filter import VersionFilter # noqa: E402 +from fastmcp.server.transforms.enabled import Enabled, is_enabled +from fastmcp.server.transforms.namespace import Namespace +from fastmcp.server.transforms.tool_transform import ToolTransform +from fastmcp.server.transforms.version_filter import VersionFilterdocs/servers/enabled.mdx (1)
1-6: Consider aligning title with content terminology.The title "Component Visibility" (lines 2-3) uses "visibility" terminology while the content consistently uses "enabled/disabled" language. Consider updating to "Component Enabled State" or "Enable/Disable Components" for consistency with the rest of the document and PR objectives.
♻️ Suggested title update
--- -title: Component Visibility -sidebarTitle: Component Visibility +title: Enable/Disable Components +sidebarTitle: Enable/Disable description: Control which components are available to clients icon: toggle-on ---src/fastmcp/contrib/component_manager/component_manager.py (1)
91-121: Missing error handling and response improvements.
No existence check: The endpoint doesn't verify the component exists before enabling/disabling. If a user requests
/tools/nonexistent/enable, theEnabledtransform is added but it matches nothing—this succeeds silently.Grammar in response: "Enabled tool: foo" reads oddly. Consider "Tool 'foo' enabled" or similar.
Missing type annotation: The
endpointfunction return type should be annotated.♻️ Suggested improvements
def _make_endpoint(server: FastMCP, component_type: str, action: str): """Create an endpoint function for enabling/disabling a component type.""" - async def endpoint(request: Request) -> JSONResponse: + async def endpoint(request: Request) -> JSONResponse: # Get name from path params (tools/prompts use 'name', resources use 'uri') name = request.path_params.get("name") or request.path_params.get("uri") version = request.query_params.get("version") + if name is None: + return JSONResponse( + {"error": "Component name is required"}, status_code=400 + ) + # Map component type to components list # ... existing logic ... # Call server.enable() or server.disable() method = getattr(server, action) method(name=name, version=version, components=components) - return JSONResponse( - {"message": f"{action.capitalize()}d {component_type}: {name}"} - ) + return JSONResponse( + {"message": f"{component_type.capitalize()} '{name}' {action}d"} + ) return endpoint
jlowin
removed
the
breaking change
Change enable() and disable() to accept sets: names, keys, tags. Use key-based disable for decorator enabled=False to scope exactly.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/fastmcp/server/providers/local_provider.py (1)
987-992: Missingcomponentsandversionparameters in resource decorator disable calls.The disable calls at lines 988 and 992 only pass
keys={obj.key}without specifyingcomponentsorversion. This is inconsistent with the pattern used inadd_resource(lines 376-388) and could cause unintended side effects if other component types share the same key prefix.🔧 Suggested fix
if isinstance(obj, ResourceTemplate): self.add_template(obj) if not enabled: - self.disable(keys={obj.key}) + self.disable(keys={obj.key}, components=["template"], version=obj.version) else: self.add_resource(obj) if not enabled: - self.disable(keys={obj.key}) + self.disable(keys={obj.key}, components=["resource"], version=obj.version)
🧹 Nitpick comments (5)
src/fastmcp/contrib/component_manager/component_manager.py (3)
8-8: Consider using the enhancedRequireAuthMiddlewarefrom fastmcp.The
fastmcp.server.auth.middlewaremodule provides an enhancedRequireAuthMiddlewarethat offers more detailed and actionable error messages for authentication failures (as shown in the relevant code snippet atsrc/fastmcp/server/auth/middleware.py:21-95). Using it would provide better developer experience when authentication errors occur.♻️ Suggested change
-from mcp.server.auth.middleware.bearer_auth import RequireAuthMiddleware +from fastmcp.server.auth.middleware import RequireAuthMiddleware
91-92: Add return type annotation for_make_endpoint.Per coding guidelines, Python code should have full type annotations. The function is missing a return type annotation.
♻️ Suggested change
+from collections.abc import Awaitable, Callable + ... -def _make_endpoint(server: FastMCP, component_type: str, action: str): +def _make_endpoint( + server: FastMCP, component_type: str, action: str +) -> Callable[[Request], Awaitable[JSONResponse]]: """Create an endpoint function for enabling/disabling a component type."""
99-111: Consider using aLiteraltype forcomponent_typeparameter.Since
component_typeonly accepts specific values ("tool","resource","prompt"), using aLiteraltype would provide better type safety and documentation. Currently, an invalid value would raise aKeyErrorat runtime.♻️ Suggested improvement
+from typing import Literal + ... -def _make_endpoint(server: FastMCP, component_type: str, action: str): +def _make_endpoint( + server: FastMCP, + component_type: Literal["tool", "resource", "prompt"], + action: Literal["enable", "disable"], +) -> Callable[[Request], Awaitable[JSONResponse]]:docs/python-sdk/fastmcp-server-transforms-enabled.mdx (1)
1-4: Missing required frontmatter fields.Per coding guidelines, every MDX documentation page must begin with YAML frontmatter containing both
titleanddescription. Thedescriptionfield is missing.📝 Suggested fix
--- title: enabled sidebarTitle: enabled +description: API reference for the Enabled transform that marks component enabled state. ---docs/servers/enabled.mdx (1)
264-289: Consider adding a note about thread safety in the dynamic changes example.The example shows modifying server-level enabled state based on user context during request handling. Since enabled state changes "affect all connected clients" (as the warning notes), concurrent requests from different users could create race conditions where one user's permission check affects another user's component visibility.
Consider either:
- Adding a note about this concurrency concern
- Showing a safer pattern (e.g., session-level transforms if supported)
Test Failure AnalysisSummary: Tests are failing because commit 84d7d3b changed the implementation to use Root Cause: The commit "Support plural names/keys in enable/disable API" (84d7d3b) refactored the Suggested Solution: This was already fixed in commit c4f506b. The failing workflow ran on commit 84d7d3b, but the latest commit on the branch (c4f506b) has the corrected tests. Re-running the workflow on the latest commit should pass. Detailed AnalysisThe four failing tests all follow the same pattern: Each test creates an assert enabled_transforms[0].names == {"disabled_tool"}But the Commit c4f506b ("Update test_local_provider.py") fixed all four test assertions to check Related Files
|
There was a problem hiding this comment.
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)
docs/servers/enabled.mdx (1)
208-235: Provider-level section implies pre-server filtering; update to mark/override semantics.
The PR objective says final filtering happens at the server so server-level rules can override provider-level disables. This section reads as if providers filter components before the server sees them.✏️ Suggested edit
-Each provider maintains its own enabled state. Provider-level filtering happens before components reach the server. +Each provider maintains its own enabled state. Provider-level rules mark components before they are combined at the server, and server-level rules can still override them. @@ -# Server receives only regular_action +# Clients see only regular_action (unless the server re-enables admin_action) @@ -Provider-level filtering is useful when different servers should see different subsets of the same provider's components. +Provider-level rules are useful when different servers should see different subsets of the same provider's components.
jlowin
added
feature
1 participant
The visibility system now uses a simpler, mark-based approach where each
enable()ordisable()call adds an immutableEnabledtransform. Transforms mark components via internal metadata rather than filtering inline, allowing later transforms to override earlier ones.The key architectural change: enabled filtering now happens at the Server level, not Provider level. This lets server-level (or session-level) transforms override provider-level disables - so you can disable everything at the provider but selectively re-enable at the server.
Also renames
Visibility→Enabledand collapsesVisibilityRuleinto the transform class.Closes #2910