Repository navigation
[PFR-4635] Tell MCP clients which tools only read #20
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
43a3619
[PFR-4635] Tell MCP clients which tools only read
brucesw a729bb0
[PFR-4635] Mark the bundled spec's POST reads and warn on unknown access
brucesw 5e0f5ac
[PFR-4635] Require MCP writes in the bundled spec to declare their ac…
brucesw 4ba7275
[PFR-4635] Check x-cortex-mcp-access values in the bundled spec
brucesw ecf1050
[PFR-4635] Leave idempotentHint unset
brucesw 0d7c482
[PFR-4635] Build the tool annotations from booleans
brucesw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,13 +6,16 @@ | |
| OpenAPITool, | ||
| ) | ||
| from fastmcp.utilities.openapi.models import HTTPRoute | ||
| from mcp.types import ToolAnnotations | ||
|
|
||
| from ..utils.logging import get_logger | ||
|
|
||
| logger = get_logger(__name__) | ||
|
|
||
| ComponentType = OpenAPITool | OpenAPIResource | OpenAPIResourceTemplate | ||
|
|
||
| ACCESS_VALUES = ("read", "additive", "destructive") | ||
|
|
||
| def customize_components( | ||
| route: HTTPRoute, | ||
| component: ComponentType, | ||
|
|
@@ -51,6 +54,11 @@ def customize_components( | |
| # Hide this fully for now. | ||
| component.output_schema = None | ||
|
|
||
| # Without a title, FastMCP derives one from the operationId: "Addcustomdataforentity". | ||
| if route.summary: | ||
| component.title = route.summary | ||
| component.annotations = _tool_annotations(route) | ||
|
Comment on lines
+57
to
+60
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| # SCHEMA REFERENCE HANDLING: | ||
| # OpenAPI-generated MCP tool schemas contain $defs with nested $ref references | ||
| # (e.g., $ref -> $defs -> $ref chains). | ||
|
|
@@ -100,6 +108,32 @@ def customize_components( | |
| logger.debug(f"Final tags: {component.tags}") | ||
|
|
||
|
|
||
| def _tool_annotations(route: HTTPRoute) -> ToolAnnotations: | ||
| """Hint whether a tool only reads, only adds, or may overwrite data. | ||
|
|
||
| The x-cortex-mcp-access extension ("read", "additive" or "destructive") wins, | ||
| and any other value counts as destructive. Without it a GET reads and anything | ||
| else may be destructive, which is also what MCP clients assume. | ||
| """ | ||
| method = route.method.upper() | ||
| access = route.extensions.get("x-cortex-mcp-access") | ||
| if access is None: | ||
| access = "read" if method == "GET" else "destructive" | ||
| elif access not in ACCESS_VALUES: | ||
| logger.warning( | ||
| f"Unknown x-cortex-mcp-access {access!r} on {method} {route.path}, " | ||
| "treating it as destructive" | ||
| ) | ||
| access = "destructive" | ||
| read_only = access == "read" | ||
| # MCP defines destructiveHint only for tools that aren't read-only. | ||
| destructive = None if read_only else access == "destructive" | ||
| return ToolAnnotations( | ||
| read_only_hint=read_only, | ||
| destructive_hint=destructive, | ||
| ) | ||
|
|
||
|
|
||
| def _format_tool_description(route: HTTPRoute, original: str) -> str: | ||
| """Format description for tool components.""" | ||
| method = route.method.upper() | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| """Tests for the bundled OpenAPI spec.""" | ||
| import json | ||
| from pathlib import Path | ||
|
|
||
| from src.components.customizers import ACCESS_VALUES | ||
|
|
||
| SPEC_PATH = Path(__file__).parent.parent / "swagger.json" | ||
|
|
||
|
|
||
| class TestBundledSpec: | ||
| """Test suite for the swagger.json the server ships with.""" | ||
|
|
||
| def test_mcp_tools_declare_valid_access(self): | ||
| """Test that MCP-enabled operations set a valid x-cortex-mcp-access. | ||
|
|
||
| Every operation other than a GET must set it. Without it, or with an | ||
| unknown value, the tool is published as destructive even if it only reads. | ||
| """ | ||
| spec = json.loads(SPEC_PATH.read_text()) | ||
|
|
||
| invalid = [ | ||
| f"{method.upper()} {path}" | ||
| for path, item in spec["paths"].items() | ||
| for method, operation in item.items() | ||
| if isinstance(operation, dict) | ||
| and operation.get("x-cortex-mcp-enabled") == "true" | ||
| and (method != "get" or "x-cortex-mcp-access" in operation) | ||
| and operation.get("x-cortex-mcp-access") not in ACCESS_VALUES | ||
| ] | ||
|
|
||
| assert not invalid, ( | ||
| f"Set x-cortex-mcp-access to one of {', '.join(ACCESS_VALUES)} on: " | ||
| f"{', '.join(invalid)}" | ||
| ) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.

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.
should we just have CRUD as access types?
Uh oh!
There was an error while loading. Please reload this page.
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.
or just read/write/delete if we can't identify that?
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.
Yeah agree with Keith here
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.
Original plan was to have a followup PR annotating each tool individually in brain-backend. But, if the proposal is to do a simple mapping as follows, yeah I could absolutely do that. Seems good
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.
Oh I didn't realize there was only
readOnlyHintanddestructiveHint, I think in this case GET asreadOnlyHintand anything else that modifies data asdestructiveHintis fine.I was more so thinking we could label the operations more fine grained but seems like that's not part of the MCP spec.
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.
@akwirick spoke with Keith and we had a misunderstanding as to what's happening here. The protocol itself fixes the two annotations - readOnlyHint and destructiveHint.
The code as it is live now does this (different in that POST - Create defaults

destructiveHinttofalse):Then I would introduce overrides in brain-backend to classify as follows:

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.
Another way of saying that is we have
readOnly,nonDestructiveWritesanddestructiveWritesright?I'm not gonna nit if it's the spec, just trying to understand the usage
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.
yes, that's right