bito-code-review[bot] commented on code in PR #44386:
URL: https://github.com/apache/superset/pull/44386#discussion_r4045827728
##########
tests/unit_tests/mcp_service/utils/test_token_utils.py:
##########
@@ -441,6 +448,27 @@ def test_info_tools_does_not_contain_list_tools(self) ->
None:
assert "generate_chart" not in INFO_TOOLS
+class TestCommittedWriteToolsSet:
+ """Test the COMMITTED_WRITE_TOOLS constant."""
+
+ def test_contains_update_chart(self) -> None:
+ """update_chart commits before the size guard runs and must be
+ truncation-eligible instead of hard-blocked."""
+ assert "update_chart" in COMMITTED_WRITE_TOOLS
+
+ def test_does_not_contain_read_only_tools(self) -> None:
Review Comment:
<!-- Bito Reply -->
The suggestion is appropriate as it ensures consistency with the existing
test suite and adheres to the project's documentation standards. Adding a
docstring to the new test function improves maintainability and clarity for
future developers.
**tests/unit_tests/mcp_service/utils/test_token_utils.py**
```
def test_does_not_contain_read_only_tools(self) -> None:
"""Ensure read-only tools are excluded from the committed write
set."""
assert "list_tools" not in COMMITTED_WRITE_TOOLS
```
##########
tests/unit_tests/mcp_service/utils/test_token_utils.py:
##########
@@ -441,6 +448,27 @@ def test_info_tools_does_not_contain_list_tools(self) ->
None:
assert "generate_chart" not in INFO_TOOLS
+class TestCommittedWriteToolsSet:
+ """Test the COMMITTED_WRITE_TOOLS constant."""
+
+ def test_contains_update_chart(self) -> None:
+ """update_chart commits before the size guard runs and must be
+ truncation-eligible instead of hard-blocked."""
+ assert "update_chart" in COMMITTED_WRITE_TOOLS
+
+ def test_does_not_contain_read_only_tools(self) -> None:
+ assert "get_chart_info" not in COMMITTED_WRITE_TOOLS
+ assert "list_charts" not in COMMITTED_WRITE_TOOLS
+ assert "execute_sql" not in COMMITTED_WRITE_TOOLS
+
+
+class TestStringFieldTruncationToolsMap:
+ """Test the STRING_FIELD_TRUNCATION_TOOLS constant."""
+
+ def test_get_chart_sql_maps_to_sql_field(self) -> None:
Review Comment:
<!-- Bito Reply -->
The suggestion is appropriate as it improves code documentation by following
the project's established rules for new test functions. Adding a docstring that
explains the mapping of the tool to its bisected field provides necessary
context for future maintainers. You should apply this suggestion to ensure
compliance with the project's documentation standards.
**tests/unit_tests/mcp_service/utils/test_token_utils.py**
```
def test_get_chart_sql_maps_to_sql_field(self) -> None:
"""Test that get_chart_sql maps to the sql field for bisection."""
```
##########
superset/mcp_service/utils/token_utils.py:
##########
@@ -481,6 +489,59 @@ def _get_tool_specific_suggestions(
}
)
+# Mutating tools whose transaction commits (via @transaction) before this
+# middleware ever inspects the response -- by the time an oversized response
+# is detected, the write already happened. Raising ToolError here would
+# report a completed write as a failure, and a retrying MCP client would
+# replay the mutation. These are truncated with the same field-level phases
+# as INFO_TOOLS (see ``_handle_oversized_response``), with the tool's
+# identifying field protected from the final "clear everything" phase so the
+# caller can always confirm what was written.
+COMMITTED_WRITE_TOOLS = frozenset(
+ {
+ "update_chart",
+ }
+)
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding `update_dashboard` is valid. Since
`update_dashboard` commits to the database before the response size guard runs,
it should be included in `COMMITTED_WRITE_TOOLS` to prevent an oversized
response from triggering a `ToolError` that would cause a retry-replay of the
mutation.
To resolve this, you should add `"update_dashboard"` to the
`COMMITTED_WRITE_TOOLS` set in `superset/mcp_service/utils/token_utils.py`.
**superset/mcp_service/utils/token_utils.py**
```
COMMITTED_WRITE_TOOLS = frozenset(
{
"update_chart",
"update_dashboard",
}
)
```
--
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]
