Repository navigation
feat(mcp): opt-in compact native tools/list and trimmed tool guidance - #45084
Conversation
…ibility mode Add tests that build the native and compatibility (search_tools/call_tool) surfaces from the registered tools and the default middleware stack, and check that both expose the same definitions, results, validation errors, and authorization decisions: RBAC, token scopes, restricted-principal policy, and permission revocation. Record a measured native catalog inventory (per-tool annotations, description and schema sizes, list entry sizes with structured output disabled and enabled, catalog totals) and fail on unreviewed growth. Fix LoggingMiddleware dropping isError when it rewraps a returned error result. Calls forwarded by the call_tool proxy return their converted error as a result, so permission and validation errors reached clients as successful results and were logged as successes. Document the native and compatibility discovery modes, migration, and mixed-version rollout.
…ative-list-compaction
Add MCP_NATIVE_TOOL_LIST_CONFIG (compact=False by default). When tool search is disabled and compact is enabled, tools/list bounds each description with the same rule tool search applies to its results, shared through a single _bounded_description helper. Schemas, annotations and server-side validation are unchanged; tools/call resolves the registered tool. Trim redundant guidance from the largest tool definitions: generate_chart and update_chart defer per-type fields and examples to get_chart_type_schema, the list tools drop filter docstrings that repeat field descriptions, and dashboard markdown/native filter specs shorten placement and scope text. Regenerate the native tool inventory with compact catalog totals.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
CodeAnt PR Risk: Low Risk
Assessed commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45084 +/- ##
==========================================
- Coverage 82.79% 82.74% -0.06%
==========================================
Files 3016 3012 -4
Lines 192466 191771 -695
Branches 44835 44406 -429
==========================================
- Hits 159354 158674 -680
+ Misses 30079 30067 -12
+ Partials 3033 3030 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
AI review at head b2558a6Verdict: CLEAN Reviewed the full 14-file diff against master, including the merged prerequisite changes, for correctness, regressions, security against SECURITY.md, and test coverage. No actionable findings identified. Checked that compaction changes only listed descriptions, leaves schemas/annotations and call validation intact, and preserves registered definitions for calls. Reviewed both startup paths, shared search serialization, returned-error logging, editorial trims, documentation, and inventory/authorization contract tests. Validation with FastMCP 3.4.7 and MCP 1.29.1:
The local HTTP health endpoint was unavailable; these unit tests ran independently. No fixes, commits, or pushes were needed. Live head was reverified before posting. |
Acceptance test at head b2558a6Setup: fresh Python 3.11 venv from
Unit tests: Acceptance criteria this PR claims1. Each tool is listed under its real name with a bounded, useful description, and its input schema, output schema and annotations stay accurate. PASS
2. One canonical definition and execution path, with both structured-output settings tested. PASS
3. Schemas stay valid and keep nullability, server validation is unaffected, and catalog size is measured. PASS
These relative reductions match the PR body's table. 4. No default changes and the opt-in is gated. PASS
Out of scope for this PR, so not tested: RBAC/permission parity between native and compatibility paths, mixed-version behaviour, and target-client size qualification. Verdict: PASS. No fixes needed, nothing pushed. |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hey @aminghadersohi, thanks for splitting this out from #45081, and the size table made the diff easy to follow. I checked out the head, ran tests/unit_tests/mcp_service in full plus the three suites in your testing instructions, and queried three real superset mcp run servers (default, compact=True at 300, and at 250) over HTTP. The opt-in is inert by default, and with compact=True every wire field except description is byte identical for all 79 tools in both output modes, with tools/call results and validation errors identical too. Two things I would like to settle before this merges, both on the lines below: the per tool budget at and below 300, and the recorded inventory. Happy to re-review quickly once those are sorted. CI is green at b2558a6.
|
Addressed the review’s budget and inventory points in ab667c9. Native descriptions use the full prose budget, with non-empty descriptions and unchanged schemas at 150/250/300 in both output modes; compatibility search is unchanged. Regenerated the inventory and updated the body table, pinned the box-plot required fields/examples, and corrected the structured-paragraph wording. All four inline threads have replies naming the commit and are resolved. Validation: tests/unit_tests/mcp_service — 7,494 passed, 4 skipped; focused native-surface/search-transform run — 133 passed; staged and branch-changed pre-commit checks, including mypy, passed. |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hey @aminghadersohi, thanks for the quick turnaround. Three of the four are closed on this head: nothing in the native listing comes out empty at 150, 250 or 300 in either output mode, list_datasets keeps its list_metrics/get_table steer at every budget, the structured paragraph wording reads right, and the new box_plot assertion does fail when get_chart_type_schema stops requiring distribute_across. The inventory is the one still open, on its thread below, and it is a single regeneration away. I ran tests/unit_tests/mcp_service in full and queried real superset mcp run servers at 300, 250, 150, 100 and 60. CI is green at 325c6ca. Happy to approve as soon as that file is squared.
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
@aminghadersohi Both of my open items check out on this head, so this is an approve from me. The inventory regenerates to a zero diff against the committed file under the locked fastmcp==3.4.7 / mcp==1.29.1, and I measured the search budget fix live at 150, 250 and 300 in both include_schemas modes. Full tests/unit_tests/mcp_service is green apart from three dateutil transition-offset tests that fail identically on master. CI is green on cf056c0f. Details are on the three threads; one wording nit inline.
SUMMARY
Builds on #45081 and should merge after it: this branch merges that PR's branch to reuse its native tool inventory test and
native_tool_inventory.json. Once #45081 lands, the diff here reduces to the changes below.1. Opt-in compact native
tools/list(default off). NewMCP_NATIVE_TOOL_LIST_CONFIG = {"compact": False, "max_description_length": 300}. It only applies when tool search is disabled (native mode). Withcompact=True, a FastMCP transform bounds each listed description using the shared_truncate_descriptionhelper: whole paragraphs first, then whole sentences. Once a whole paragraph fits, a following heading,IMPORTANTblock or list is not partly advertised; an overlong first paragraph can be cut at a sentence boundary even if structured. Native mode budgets prose alone, without deducting request-parameter instructions already served untruncated in the unchanged input schema. Compatibility-mode search also budgets prose alone; request instructions remain intact ininputSchemaorparameters_hint. Only the listing changes. Names, input/output schemas (nullable unions, constraints,$defs) and annotations are served unchanged.get_tool/tools/callresolve the registered tool, so Pydantic validation and execution do not depend on the setting. FastMCP's$refinlining is kept, because keeping$refs would be larger. No default changes.2. Editorial trims of redundant guidance, applied in both modes:
generate_chart: the per-chart-type required-field list, the table/pie/SQL-metric examples and the validation-pipeline notes are removed.get_chart_type_schemaalready serves each type's fields, required fields and examples, and the description now points to it first. The discriminator rules, the natural-language →chart_typelookup and one XY example stay.update_chart: duplicate examples and the "Use when" list are folded; it points toget_chart_type_schemafor config.list_charts/list_dashboards/list_datasets: filter-model docstrings that restated thecol/opr/valuefield descriptions are removed, the sharedfiltersdescription is shortened, deployment-internal wording is dropped fromdeleted_state, and a developer note moves from theListDatasetsRequestdocstring into a code comment.manage_dashboard_markdown/manage_native_filters: placement and scope text that the tool docstring already covers is shortened on the spec models.No execution behaviour changes. The description-constraint audits (
test_tool_description_constraints.py) pass with independent prose and instruction budgets, including a regression for thelist_datasetssemantic-view steer in both search modes.Sizes (regenerated on this branch with locked
fastmcp==3.4.7/mcp==1.29.1; compact UTF-8 JSON of thetools/listentries, 79 tools; chart_type enum excluded as in the inventory; tokens ≈ bytes / 4.6):compact=True, 300)Historical pre-merge text-only per-tool reductions in the default listing:
generate_chart−3,731 B,update_chart−824,manage_dashboard_markdown−777,list_datasets−507,list_charts/list_dashboards−433 each,manage_native_filters−224, and the other list tools −71 each. The inventory now recordscompact_description_charsper tool andcompact_catalog_bytestotals, and enforces both with the existing growth budget.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
pytest tests/unit_tests/mcp_service/test_native_tool_surface.py \ tests/unit_tests/mcp_service/test_mcp_server.py \ tests/unit_tests/mcp_service/test_tool_description_constraints.py # regenerate the inventory after an intentional change SUPERSET_MCP_UPDATE_TOOL_INVENTORY=1 pytest tests/unit_tests/mcp_service/test_native_tool_surface.pyNew tests in
test_native_tool_surface.py:test_compact_listing_is_off_by_default: the default config, an empty config, and a zero budget all leave the listing alone.test_compact_listing_only_bounds_descriptions(both output modes at 150, 250, and 300 characters): every wire field exceptdescriptionis identical to the default listing. Each description is non-empty, within the prose budget, equals_truncate_description(...)of the full text, and is a prefix of it. The semantic-view discovery/query steer survives at every tested budget.test_compact_schemas_keep_nullability_constraints_and_definitions: input and output schemas are identical, and the counts of nullableanyOf,enum,minimum/maximum,maxLength,$defs/$ref, etc. match.test_compact_listing_keeps_full_definitions_for_calls:get_toolreturns the full description and parameters for every tool.test_compact_listing_validates_calls_identically: constraint violations, wrong types and missing required fields give identical validation errors, and nullable fields acceptnull.test_compact_listing_keeps_chart_guidance_reachable: the compact chart tools referenceget_chart_type_schema, which returns required fields and examples for every core chart type, explicitly including the box-plotmetrics/distribute_acrossrequirements and sample-axis examples.In
test_mcp_server.py,run_serverpassesMCP_NATIVE_TOOL_LIST_CONFIGthrough only in native mode.ADDITIONAL INFORMATION