fix(mcp): make search_tools query optional to fix null content on tool discovery by aminghadersohi · Pull Request #40906 · apache/superset

aminghadersohi · 2026-06-09T16:19:58Z

Summary

Root cause: BM25SearchTransform._make_search_tool() declares query: str (required). When an LLM calls search_tools({}) for tool discovery, TypeAdapter.validate_python({}) raises ValidationError because the required field is missing. StructuredContentStripperMiddleware catches this and returns error text instead of the tool catalog — so the LLM receives null/empty content.
Fix: Override _make_search_tool() in both _FixedBM25SearchTransform and _FixedRegexSearchTransform to accept query: str | None = None. When omitted, returns all visible tools directly; when provided, runs the configured search strategy as before.
Tests: Three new unit tests verify the schema marks query as optional, calling with no args returns all tools (BM25 + regex strategies).

Test plan

pytest tests/unit_tests/mcp_service/test_tool_search_transform.py::test_search_tool_query_is_optional_in_schema
pytest tests/unit_tests/mcp_service/test_tool_search_transform.py::test_search_tool_with_no_query_returns_all_visible_tools
pytest tests/unit_tests/mcp_service/test_tool_search_transform.py::test_search_tool_regex_with_no_query_returns_all_visible_tools

…covery BM25SearchTransform._make_search_tool() declared query: str (required). When an LLM calls search_tools({}) for tool discovery, TypeAdapter raises ValidationError, which StructuredContentStripperMiddleware catches and returns as error text instead of the tool catalog. Override _make_search_tool() in both _FixedBM25SearchTransform and _FixedRegexSearchTransform to make query optional (str | None = None). When omitted, returns all visible tools directly; when provided, runs the configured search strategy.

codecov · 2026-06-09T16:23:59Z

Codecov Report

❌ Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.12%. Comparing base (443fd7b) to head (acc3534).
⚠️ Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/mcp_service/server.py	0.00%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #40906      +/-   ##
==========================================
- Coverage   64.13%   64.12%   -0.01%     
==========================================
  Files        2653     2653              
  Lines      143601   143615      +14     
  Branches    33132    33133       +1     
==========================================
- Hits        92104    92099       -5     
- Misses      49884    49901      +17     
- Partials     1613     1615       +2

Flag	Coverage Δ
hive	`39.50% <0.00%> (-0.01%)`	⬇️
mysql	`58.23% <0.00%> (-0.02%)`	⬇️
postgres	`58.30% <0.00%> (-0.02%)`	⬇️
presto	`41.09% <0.00%> (-0.01%)`	⬇️
python	`59.77% <0.00%> (-0.02%)`	⬇️
sqlite	`57.92% <0.00%> (-0.02%)`	⬇️
unit	`100.00% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bito-code-review · 2026-06-09T19:48:15Z

Code Review Agent Run #21bbd5

Actionable Suggestions - 0

Additional Suggestions - 2

superset/mcp_service/server.py - 1
- Dead noqa suppression comment · Line 622-622
  
  The `# noqa: C901` comment was added but the function complexity remains unchanged. Consider either refactoring to reduce complexity or removing the suppression comment.
tests/unit_tests/mcp_service/test_tool_search_transform.py - 1
- Duplicate inline imports · Line 1252-1255
  
  Inline imports `asyncio` and `AsyncMock` are duplicated in two functions (lines 1254-1255, 1285-1286) instead of living at module level with other mock imports at line 21. Move to module scope for consistency and clarity.

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

superset/mcp_service/server.py - 1
- Type annotation inconsistency · Line 640-640

Review Details

Files reviewed - 2 · Commit Range: acc3534..acc3534
- superset/mcp_service/server.py
- tests/unit_tests/mcp_service/test_tool_search_transform.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

gabotorresruiz

Nice fix, search_tools({}) failing closed is a real footgun for any MCP client that enumerate tools with no args.

One related thought for a follow-up (not this PR):
StructuredContentStripperMiddleware.on_list_tools catches all exceptions and returns [], so a transient error in the tools/list pipeline is indistinguishable from "this server has no tools." MCP clients that cache tools/list then treat it as an empty catalog rather than a retryable failure (similar to the empty-result problem this PR fixes for search_tools, but at the tools/list layer). Might be worth surfacing those as real protocol errors so clients can retry.

gabotorresruiz · 2026-06-09T23:26:23Z

+            query: Annotated[
+                str | None,
+                "Natural language query. Omit to list all available tools.",
+            ] = None,


One thing on the schema: query: str | None renders as {"anyOf": [{"type":"string"},{"type":"null"}]} with no top-level type, same shape as call_tool's arguments, which is exactly what _fix_call_tool_arguments flattens (its docstring notes mcp-remote/Claude Desktop strip anyOf and leave the field typeless). So we'd be reintroducing an unflattened anyOf for those bridges.

Probably milder than the call_tool case (query is optional and usually omitted, so it may not actually break) but it's the same shape that bit us before, and flattening to a plain {"type": "string"} is cheap. Might also be worth having test_search_tool_query_is_optional_in_schema assert query has a concrete type, since that's what the bridges trip on.

bito-code-review · 2026-06-09T23:57:04Z

The concern regarding the query: str | None schema is valid. Because fastmcp (and the underlying Pydantic-based schema generation) renders str | None as an anyOf structure, it can cause compatibility issues with certain MCP clients that expect a concrete, top-level type. Flattening this to a plain {"type": "string"} is a recommended approach to ensure compatibility with the bridges, similar to how call_tool arguments are handled. Additionally, updating test_search_tool_query_is_optional_in_schema to assert that the query field has a concrete type would provide a necessary safeguard against this regression.

superset/mcp_service/server.py

query: Annotated[
                str,
                "Natural language query. Omit to list all available tools.",
            ] = None,

pull-request-size Bot added the size/L label Jun 9, 2026

aminghadersohi marked this pull request as ready for review June 9, 2026 16:45

aminghadersohi requested review from gabotorresruiz and rusackas and removed request for gabotorresruiz June 9, 2026 18:54

aminghadersohi requested a review from gabotorresruiz June 9, 2026 20:32

gabotorresruiz reviewed Jun 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mcp): make search_tools query optional to fix null content on tool discovery#40906

fix(mcp): make search_tools query optional to fix null content on tool discovery#40906
aminghadersohi wants to merge 1 commit into
apache:masterfrom
aminghadersohi:sc-108423-mcp-search-null-content

aminghadersohi commented Jun 9, 2026

Uh oh!

codecov Bot commented Jun 9, 2026 •

edited

Loading

Uh oh!

bito-code-review Bot commented Jun 9, 2026 •

edited

Loading

Code Review Agent Run #21bbd5

Uh oh!

gabotorresruiz left a comment

Uh oh!

gabotorresruiz Jun 9, 2026

Uh oh!

bito-code-review Bot commented Jun 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

aminghadersohi commented Jun 9, 2026

Summary

Test plan

Uh oh!

codecov Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bito-code-review Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #21bbd5

Uh oh!

gabotorresruiz left a comment

Choose a reason for hiding this comment

Uh oh!

gabotorresruiz Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

bito-code-review Bot commented Jun 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jun 9, 2026 •

edited

Loading

bito-code-review Bot commented Jun 9, 2026 •

edited

Loading