bito-code-review[bot] commented on code in PR #44573:
URL: https://github.com/apache/superset/pull/44573#discussion_r4081519202
##########
tests/unit_tests/mcp_service/test_mcp_tool_registration.py:
##########
@@ -249,6 +249,46 @@ def _run(coro):
return asyncio.run(coro)
+# Tools whose request model tells "field omitted" from "field set to null":
+# omitting leaves the stored value alone, passing null clears it. Their
+# optional fields must not advertise a default, or a client that materialises
+# defaults turns every call into a clear of everything it did not mention.
+OMITTED_MEANS_UNCHANGED_TOOLS = (
+ "update_chart",
+ "update_dashboard",
+ "update_dataset_metric",
+)
+
+
+def _request_model_schema(tool: Any) -> dict[str, Any]:
+ """Return the JSON Schema of a tool's ``request`` argument."""
+ schema = tool.parameters or {}
+ request = schema.get("properties", {}).get("request", {})
+ reference = request.get("$ref", "")
+ if reference.startswith("#/$defs/"):
+ return schema.get("$defs", {}).get(reference.split("/")[-1], {})
+ return request
+
+
+def test_partial_update_tools_advertise_no_null_default():
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing return type annotation</b></div>
<div id="fix">
BITO.md adaptive rule 7819 requires explicit return type hints on all
functions including test methods; sibling tests here (e.g.
`test_schema_title_annotations_do_not_remove_title_fields`) annotate `-> None`.
Add `-> None` to this new test for consistency and static-check coverage.
</div>
</div>
<small><i>Code Review Run #e7e06e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/test_mcp_tool_registration.py:
##########
@@ -249,6 +249,46 @@ def _run(coro):
return asyncio.run(coro)
+# Tools whose request model tells "field omitted" from "field set to null":
+# omitting leaves the stored value alone, passing null clears it. Their
+# optional fields must not advertise a default, or a client that materialises
+# defaults turns every call into a clear of everything it did not mention.
+OMITTED_MEANS_UNCHANGED_TOOLS = (
+ "update_chart",
+ "update_dashboard",
+ "update_dataset_metric",
+)
+
+
+def _request_model_schema(tool: Any) -> dict[str, Any]:
+ """Return the JSON Schema of a tool's ``request`` argument."""
+ schema = tool.parameters or {}
+ request = schema.get("properties", {}).get("request", {})
+ reference = request.get("$ref", "")
+ if reference.startswith("#/$defs/"):
+ return schema.get("$defs", {}).get(reference.split("/")[-1], {})
+ return request
+
+
+def test_partial_update_tools_advertise_no_null_default():
+ """No optional field of a partial-update tool offers null as its
default."""
+ registered = {tool.name: tool for tool in _run(mcp.list_tools())}
+ advertised = {}
+ for name in OMITTED_MEANS_UNCHANGED_TOOLS:
+ fields = _request_model_schema(registered[name]).get("properties", {})
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unguarded tool lookup KeyError</b></div>
<div id="fix">
`registered[name]` raises `KeyError` when one of the three tools is absent:
`test_disabled_tools_are_removed_from_mcp_server` shows `mcp.list_tools()`
omits configured tools, and `test_disabled_tools_read_from_flask_app_config`
shows that set is config-driven. Prefer `registered.get(name)` plus
`pytest.fail` so a disabled tool reports absence clearly.
</div>
</div>
<small><i>Code Review Run #e7e06e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
