bito-code-review[bot] commented on code in PR #44386:
URL: https://github.com/apache/superset/pull/44386#discussion_r4041748462
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstring</b></div>
<div id="fix">
The sibling tests `test_contains_update_chart` and
`test_get_chart_sql_maps_to_sql_field` document their intent, but
`test_does_not_contain_read_only_tools` has no docstring. Per the repo's
BITO.md adaptive rule 12148, every new test function needs one; adding it keeps
the new `TestCommittedWriteToolsSet` class consistent.
</div>
</div>
<small><i>Code Review Run #26bf41</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
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Committed-write set incomplete</b></div>
<div id="fix">
The new `COMMITTED_WRITE_TOOLS` comment states a category invariant — tools
whose transaction commits before the size guard inspects the response — but the
set only contains `update_chart`. `update_dashboard` also commits
(`db.session.commit()` at update_dashboard.py:360) before returning, yet is
absent, so an oversized `UpdateDashboardResponse` still falls through
`_handle_oversized_response` to a hard `ToolError` after the write committed —
exactly the retry-replays-mutation failure mode this constant exists to
prevent. Note the protected-field logic in `_handle_oversized_response`
hardcodes `{'chart'}`, so membership alone does not protect
`update_dashboard`'s identifying field.
</div>
</div>
<small><i>Code Review Run #26bf41</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/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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstring</b></div>
<div id="fix">
`test_get_chart_sql_maps_to_sql_field` is the only new test in this diff
without a docstring besides `test_does_not_contain_read_only_tools`. BITO.md
adaptive rule 12148 requires one on every new test function; a line noting that
`STRING_FIELD_TRUNCATION_TOOLS` maps the tool to its bisected field would match
the class docstring's intent.
</div>
</div>
<small><i>Code Review Run #26bf41</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]
