gabotorresruiz commented on code in PR #44656:
URL: https://github.com/apache/superset/pull/44656#discussion_r4160575829
##########
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:
This block worries me. `_request_instructions` reads
`inputSchema.properties.request.description`, but that key only holds the
compact instruction you authored for the ten tools in this PR. For the other 66
it holds whatever Pydantic put there, which is the request model's own class
docstring.
In the running server the `list_tools` middleware inlines the `request`
`$ref`, so that docstring is present and its full length is deducted from the
300 character prose budget. `max(0, max_desc - len(instructions))` then reaches
`0` and `_truncate_description` returns `""`.
I booted the service on this branch and on the merge base and compared
`search_tools` over the wire. Nine tools come back with `"description": ""`
here, where master returned real prose:
| tool | master | this branch | deducted request docstring |
| --- | --- | --- | --- |
| `manage_dashboard_owners` | 303 | 0 | 633 |
| `manage_dashboard_roles` | 191 | 0 | 510 |
| `get_chart_preview` | 303 | 0 | 442 |
| `get_chart_data` | 303 | 0 | 433 |
| `generate_bug_report` | 158 | 0 | 336 |
| `manage_dashboard_certification` | 229 | 0 | 335 |
| `update_dashboard` | 298 | 0 | 323 |
| `get_chart_sql` | 161 | 0 | 316 |
| `find_users` | 186 | 0 | 279 |
Two of those I confirmed live end to end: a `search_tools` query that
surfaces `find_users` returns a 186 character `description` on master and an
empty one here, same for `generate_bug_report` at 158. None of the nine are
tools this PR touches, and their request-model docstrings are byte identical on
both sides, so the only thing that changed is what the budget subtracts.
`manage_dashboard_owners` is the one I would most want back. Its description
is where the "do not call this tool merely to look up current owners" guidance
lives, and an empty `description` is what the model now sees for it.
Bito raised the empty-description shape earlier and your answer is right for
the case it described, an operator setting a tiny `max_description_length`.
This one happens at the shipped default.
Fix path: have `_request_instructions` return only instructions this code
authored, rather than any `description` that happens to sit on the `request`
property. The simplest variant I measured is to drop the deduction entirely:
default-mode discovery then totals 145,721 bytes against 150,982 on master, so
still a 3.5% reduction, with zero empty descriptions.
For the test, `test_direct_inventory_keeps_bounded_calling_metadata` cannot
catch this because it reads `mcp.list_tools(run_middleware=False)`, where
`properties.request` is still a bare `$ref`, so the instruction length it
measures is `0` for all 66 untouched tools. A sweep through
`run_middleware=True` asserting every tool's serialized `description` is
non-empty under the default config fails on this branch and passes once the
deduction is fixed.
##########
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:
Small note: with `run_middleware=False`, `properties.request` is still a
bare `$ref`, so `instructions` is `""` for all 66 tools outside `CONSTRAINTS`
and the `<= 300` assert below passes without measuring anything. The value
actually served reaches 633 characters once the middleware inlines the ref.
Worth flipping this to `run_middleware=True` so the cap applies to what clients
really receive.
##########
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:
Not a blocker, but worth a look since it cuts against the title. The
paragraph rule is all or nothing: `ends` only collects breaks at or before
`max_length`, so when the next paragraph would overflow it is dropped whole and
the leftover budget goes unspent.
Measured across the inventory at the default limit, 48 tools this PR does
not touch end up with a shorter advertised `description` than on master, 11,351
characters in total, with a mean of 167 of the 300 characters left unused.
`delete_chart` is the clearest: master advertises 263 characters including
"Identify the chart by numeric ID or UUID string (NOT chart name)", this branch
advertises `Delete a saved chart.` and leaves 279 characters unspent. That
identifier rule is a calling constraint, and `delete_chart` gets no
`Field(description=...)` here to carry it.
If you want to keep the whole-paragraph guarantee, falling back to whole
sentences of the following paragraph when it does not fit recovers most of it.
I tried it: `delete_chart` goes back to the full 263 characters, 22 tools
improve, and total default-mode bytes land within 1% of master instead of 6%
under. Roughly:
```python
if paragraphs := list(re.finditer(r"\n\s*\n", text)):
ends = [match.start() for match in paragraphs if match.start() <=
max_length]
if not ends:
return ""
kept = text[: ends[-1]]
rest = text[next(m.end() for m in paragraphs if m.start() == ends[-1]) :]
following = re.split(r"\n\s*\n", rest, 1)[0]
# _sentences is the existing sentence-boundary branch below, extracted
if not re.search(r"^\s*(?:[-*+]|\d+[.)])", following, re.M):
if extra := _sentences(following, max_length - len(kept) - 2):
kept = f"{kept}\n\n{extra}"
return kept.strip()
```
The list guard keeps the "never a partial numbered workflow" property you
wanted, which is why `get_schema` correctly stays on its summary line under it.
--
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]
