aminghadersohi commented on code in PR #44656:
URL: https://github.com/apache/superset/pull/44656#discussion_r4161436722
##########
superset/mcp_service/server.py:
##########
@@ -301,29 +302,29 @@ def _strip_titles(obj: Any, in_properties_map: bool =
False) -> Any:
def _truncate_description(text: str, max_length: int) -> str:
- """Truncate a tool description for search results.
-
- Cuts at the last sentence boundary before *max_length*, or at
- *max_length* with an ellipsis if no sentence boundary is found.
-
- Dedents first: Python 3.13 has the compiler strip a docstring's common
- leading whitespace at compile time (``__doc__`` comes out already
- cleaned), while 3.11/3.12 store it raw and leave that to the caller. A
- multi-line tool docstring's raw, un-dedented form is longer per line, so
- the same character budget lands at a different point in the text
- depending on which Python compiled it. Cleaning here first makes the cut
- point (and this function's callers' byte budgets) consistent regardless
- of interpreter version.
+ """Keep whole paragraphs, or sentences in single-paragraph prose, within
budget.
+
+ Clean docstring indentation before applying the budget so the cut point
+ is consistent across Python versions that store docstrings differently.
"""
+ if max_length <= 0:
+ return ""
text = inspect.cleandoc(text) if text else text
if not text or len(text) <= max_length:
return text
- # Try to cut at the last sentence boundary
- truncated = text[:max_length]
- last_period = truncated.rfind(". ")
- if last_period > max_length // 2:
- return truncated[: last_period + 1]
- return truncated.rstrip() + "..."
+ # Do not leave a heading or a numbered/bulleted workflow partly advertised.
+ if paragraphs := list(re.finditer(r"\n\s*\n", text)):
+ ends = [match.start() for match in paragraphs if match.start() <=
max_length]
+ return text[: ends[-1]].strip() if ends else ""
+ # Calling constraints belong in request schema metadata, not truncated
prose.
+ boundaries = list(re.finditer(r"[.!?](?=\s|$)", text[: max_length + 1]))
+ ends = [match.end() for match in boundaries if match.end() <= max_length]
+ return text[: ends[-1]].strip() if ends else ""
+
+
+def _request_instructions(input_schema: dict[str, Any]) -> str:
+ """Read unabridged calling instructions from the request wrapper's
schema."""
+ return input_schema.get("properties", {}).get("request",
{}).get("description", "")
Review Comment:
Fixed in 81bcc73ef3 (branch head 78e8a55a06). `_request_instructions` now
takes the tool and returns only the `Field(description=...)` text on its
`request` parameter. The request model's docstring, which the list_tools
middleware inlines onto `properties.request`, is no longer deducted from the
budget or appended to `parameters_hint`. Untouched tools' hints are back to
bare parameter names.
Served `search_tools` entries (`list_tools(run_middleware=True)`, default
config, 300 limit) compared with current master `33292f249d`:
| | master | 87a5768 | 78e8a55 |
| --- | --- | --- | --- |
| empty `description` | 0 | 9 | 0 |
| `manage_dashboard_owners` | 303 | 0 | 128 |
| `find_users` | 186 | 0 | 186 |
| `generate_bug_report` | 158 | 0 | 158 |
| `get_chart_sql` | 161 | 0 | 256 |
| summary-mode bytes | 42,041 | 38,679 | 39,761 |
| schema-mode bytes | 162,527 | 152,790 | 160,080 |
`manage_dashboard_owners` stops at 128 because master's extra text came from
the old hard cut ("...and rejects any cha..."); the next complete sentence does
not fit in 300. The ten touched tools are unchanged and still pass every
constraint test.
New regression tests:
`test_served_discovery_keeps_untouched_tool_descriptions` checks every served
tool in both modes. Each must have a non-empty description; tools outside the
ten must have no deduction and a parameter-name-only hint.
`test_served_discovery_regressions_keep_prose` covers your nine tools plus
`delete_chart`. Both fail at 87a5768 and pass at 78e8a55.
##########
superset/mcp_service/server.py:
##########
@@ -301,29 +302,29 @@ def _strip_titles(obj: Any, in_properties_map: bool =
False) -> Any:
def _truncate_description(text: str, max_length: int) -> str:
- """Truncate a tool description for search results.
-
- Cuts at the last sentence boundary before *max_length*, or at
- *max_length* with an ellipsis if no sentence boundary is found.
-
- Dedents first: Python 3.13 has the compiler strip a docstring's common
- leading whitespace at compile time (``__doc__`` comes out already
- cleaned), while 3.11/3.12 store it raw and leave that to the caller. A
- multi-line tool docstring's raw, un-dedented form is longer per line, so
- the same character budget lands at a different point in the text
- depending on which Python compiled it. Cleaning here first makes the cut
- point (and this function's callers' byte budgets) consistent regardless
- of interpreter version.
+ """Keep whole paragraphs, or sentences in single-paragraph prose, within
budget.
+
+ Clean docstring indentation before applying the budget so the cut point
+ is consistent across Python versions that store docstrings differently.
"""
+ if max_length <= 0:
+ return ""
text = inspect.cleandoc(text) if text else text
if not text or len(text) <= max_length:
return text
- # Try to cut at the last sentence boundary
- truncated = text[:max_length]
- last_period = truncated.rfind(". ")
- if last_period > max_length // 2:
- return truncated[: last_period + 1]
- return truncated.rstrip() + "..."
+ # Do not leave a heading or a numbered/bulleted workflow partly advertised.
+ if paragraphs := list(re.finditer(r"\n\s*\n", text)):
+ ends = [match.start() for match in paragraphs if match.start() <=
max_length]
+ return text[: ends[-1]].strip() if ends else ""
Review Comment:
Folded in with 81bcc73ef3 (branch head 78e8a55a06). After the last whole
paragraph that fits, `_truncate_description` adds the complete sentences of the
next paragraph that fit. It skips a next paragraph that has list items (your
guard), opens with `IMPORTANT`, or has a first line ending in `:` (`Workflow:`,
`IMPORTANT BEHAVIOR:`), so partial workflows and blocks stay excluded. An
overlong first paragraph now falls back to its leading sentences instead of
`""`.
Served entries at the default 300, compared with current master
`33292f249d`: untouched tools shorter than master dropped from 49 (8,832 chars)
to 25 (2,555 chars). `delete_chart` is back to the full 263 characters,
including "NOT chart name". Of the remaining 25, 24 match or exceed master's
text after its trailing hard-cut fragment ("...") is trimmed back to whole
sentences. The last is `list_metrics` (279 vs 293), where master's extra text
is the "Workflow:\n1." fragment. `get_schema` correctly stays on its summary
line.
Unit tests in `test_tool_search_transform.py` cover the sentence fallback,
the list/heading guards, and the long first paragraph.
##########
tests/unit_tests/mcp_service/test_tool_description_constraints.py:
##########
@@ -0,0 +1,591 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Calling guidance must survive discovery without relying on truncated
prose."""
+
+import inspect
+import re
+
+import pytest
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.server import _create_search_result_serializer
+
+CONSTRAINTS = {
+ "list_dashboards": ("Do NOT pass", "top-level", '{"request": {'),
+ "list_charts": ("Do NOT pass", "top-level", '{"request": {'),
+ "list_datasets": (
+ "Do NOT pass",
+ "search/page/filters",
+ "top-level",
+ "out-of-scope",
+ '{"request": {',
+ ),
+ "get_chart_info": (
+ "NOT chart name",
+ "form_data_key",
+ "permalink_key",
+ '{"request": {',
+ ),
+ "get_dashboard_info": (
+ "filter_state",
+ "permalink_key",
+ "snapshots, not query predicates",
+ '{"request": {',
+ ),
+ "generate_dashboard": ("Never use as a fallback", "accessible",
'{"request": {'),
+}
+
+
[email protected]
[email protected]("name", CONSTRAINTS)
[email protected]("include_schemas", [True, False])
[email protected]("max_desc", [1, 20, 300, 0])
+async def test_registered_tool_calling_constraints(
+ name: str, include_schemas: bool, max_desc: int
+) -> None:
+ """Exercise real registrations, including small limits and summary
discovery."""
+ tool = await mcp.get_tool(name)
+ assert tool is not None
+ entry = _create_search_result_serializer(
+ {"include_schemas": include_schemas, "max_description_length":
max_desc}
+ )([tool])[0]
+ if include_schemas:
+ guidance =
entry["inputSchema"]["properties"]["request"].get("description", "")
+ schema_text = _schema_text(entry["inputSchema"])
+ for _, phrases in SCHEMA_DOCSTRING_CONSTRAINTS[name]:
+ assert all(phrase in schema_text for phrase in phrases), (name,
phrases)
+ else:
+ guidance = str(entry.get("parameters_hint", ""))
+ for constraint in CONSTRAINTS[name]:
+ assert constraint in guidance, (name, constraint, entry["description"])
+ if max_desc:
+ assert len(entry["description"]) <= max_desc
+
+
[email protected]
[email protected]("name", CONSTRAINTS)
+async def test_registered_constraints_have_bounded_size(name: str) -> None:
+ """Critical metadata has its own fixed ceiling, not an unbounded IMPORTANT
block."""
+ tool = await mcp.get_tool(name)
+ assert tool is not None
+ instructions = tool.parameters["properties"]["request"]["description"]
+ assert len(instructions) <= 300
+ entry = _create_search_result_serializer({"include_schemas":
True})([tool])[0]
+ assert len(instructions) + len(entry["description"]) <= 300
+
+
[email protected]
[email protected]("name", CONSTRAINTS)
[email protected]("include_schemas", [True, False])
+async def test_oversized_registered_description_keeps_calling_constraints(
+ name: str, include_schemas: bool
+) -> None:
+ """Long introductory prose cannot displace metadata or exhaust its
budget."""
+ tool = await mcp.get_tool(name)
+ assert tool is not None
+ oversized = tool.model_copy(
+ update={"description": "Long introduction. " * 10_000 +
tool.description}
+ )
+ entry = _create_search_result_serializer({"include_schemas":
include_schemas})(
+ [oversized]
+ )[0]
+ instructions = tool.parameters["properties"]["request"]["description"]
+ if include_schemas:
+ assert entry["inputSchema"]["properties"]["request"]["description"] ==
(
+ instructions
+ )
+ schema_text = _schema_text(entry["inputSchema"])
+ for _, phrases in SCHEMA_DOCSTRING_CONSTRAINTS[name]:
+ assert all(phrase in schema_text for phrase in phrases), (name,
phrases)
+ else:
+ assert instructions in entry["parameters_hint"]
+ assert len(entry["description"]) + len(instructions) <= 300
+
+
[email protected]
+async def test_direct_inventory_keeps_bounded_calling_metadata() -> None:
+ """Direct tools/list also carries instructions without relying on
search."""
+ for tool in await mcp.list_tools(run_middleware=False):
Review Comment:
Done in 81bcc73ef3 (branch head 78e8a55a06).
`test_direct_inventory_keeps_bounded_calling_metadata` now reads
`mcp.list_tools(run_middleware=True)`. For every served tool, it asserts the
instructions the serializer deducts and advertises are at most 300 characters.
When instructions are present, they must equal the served
`properties.request.description`. Model docstrings inlined by the middleware
(up to 633 characters, e.g. `manage_dashboard_owners`) no longer count as
instructions. The new `test_served_discovery_keeps_untouched_tool_descriptions`
also asserts the 300 cap on every served `description` in both modes.
--
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]
