aminghadersohi commented on code in PR #40906:
URL: https://github.com/apache/superset/pull/40906#discussion_r3404398337
##########
superset/mcp_service/server.py:
##########
@@ -509,6 +509,27 @@ def _fix_call_tool_arguments(tool: Any) -> Any:
return tool
+def _fix_search_tool_query(tool: Any) -> Any:
+ """Fix anyOf schema in search_tools ``query`` for MCP bridge compatibility.
+
+ The optional ``query: str | None`` parameter emits an ``anyOf`` JSON
+ Schema with no top-level ``type``. Some MCP bridges (mcp-remote,
+ Claude Desktop) don't handle ``anyOf`` and strip it, leaving the field
+ typeless — the same failure mode ``_fix_call_tool_arguments`` guards
+ against. Replaces the ``anyOf`` with a flat ``type: string``.
+
+ Only the advertised schema changes; FastMCP validates calls against
+ the function signature, so omitting ``query`` remains valid.
+ """
+ if "query" in (props := (tool.parameters or {}).get("properties", {})):
+ props["query"] = {
+ "default": None,
+ "description": "Natural language query. Omit to list all available
tools.",
+ "type": "string",
+ }
Review Comment:
Declining this one: `default` is an annotation keyword in JSON Schema, not a
validation keyword — `{"type": "string", "default": null}` is legal and does
not make instances invalid. More importantly, this is deliberate parity with
`_fix_call_tool_arguments` directly above, which advertises the identical shape
(`"type": "object", "default": None`) and has been running in production
against the exact bridges (mcp-remote, Claude Desktop) this comment worries
about, without issue. Diverging from that battle-tested shape would be the
riskier change.
##########
superset/mcp_service/server.py:
##########
@@ -619,14 +640,40 @@ async def call_tool(
)
-def _create_search_transform(
+def _create_search_transform( # noqa: C901
*,
strategy: str,
kwargs: dict[str, Any],
make_normalizing_call_tool: Callable[[Any], Any],
) -> Any:
"""Create the configured search transform with tool-permission
filtering."""
from fastmcp.server.context import Context
+ from fastmcp.tools.base import Tool
Review Comment:
The "Critical" failure claim is incorrect: in fastmcp (verified on 3.4.2),
`fastmcp.tools.base.Tool` is a re-export of the same class as
`fastmcp.tools.tool.Tool` (`fastmcp.tools.base.Tool is fastmcp.tools.tool.Tool
== True`), so `from_function` exists and the unit tests in this PR exercise
this exact construction path successfully. That said, the import inconsistency
point is fair — aligned to `from fastmcp.tools.tool import Tool` in
efb0dac53dc888cf39b618a0216ccccc8b8ee290 to match the rest of the file.
##########
superset/mcp_service/server.py:
##########
@@ -619,14 +640,40 @@ async def call_tool(
)
-def _create_search_transform(
+def _create_search_transform( # noqa: C901
*,
strategy: str,
kwargs: dict[str, Any],
make_normalizing_call_tool: Callable[[Any], Any],
) -> Any:
"""Create the configured search transform with tool-permission
filtering."""
from fastmcp.server.context import Context
+ from fastmcp.tools.base import Tool
+
+ def _make_optional_query_search_tool(transform: Any) -> Any:
+ """Create search tool with optional query — returns all tools when
omitted."""
+
+ async def search_tools(
+ query: Annotated[
+ str | None,
+ "Natural language query. Omit to list all available tools.",
+ ] = None,
+ ctx: Context = None,
+ ) -> str | list[dict[str, Any]]:
+ """Search for tools using natural language.
+
+ Returns matching tool definitions ranked by relevance.
+ If no query is provided, returns all available tools.
+ """
+ hidden = await transform._get_visible_tools(ctx)
+ if not query:
+ results = hidden
+ else:
+ results = await transform._search(hidden, query)
Review Comment:
This is intentional fail-open behavior, not a bug. An empty string carries
no search terms: BM25/regex ranking against `""` yields zero results, which
recreates the exact empty-discovery footgun this PR fixes (client asks for the
catalog, gets nothing). LLM clients do send `{"query": ""}` to mean "list
everything", so treating provided-but-empty the same as omitted returns the
full visible catalog — which is also still permission-filtered via
`_get_visible_tools`, so there's no exposure concern. Locked the behavior in
with a test in efb0dac53dc888cf39b618a0216ccccc8b8ee290:
`test_search_tool_empty_string_query_returns_all_visible_tools` asserts
`{"query": ""}` returns the catalog and never hits the search path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
