aminghadersohi commented on code in PR #44386:
URL: https://github.com/apache/superset/pull/44386#discussion_r4045826604
##########
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:
Fixed. `COMMITTED_WRITE_TOOLS` is now derived from a `COMMITTED_WRITE_SPECS`
table covering every mutating tool that commits to the metadata DB before the
guard runs — `update_dashboard` included.
The hardcoded `{'chart'}` is gone with it: each spec names its own
identifying container fields (`chart`, `dashboard`, `metric`), and tools whose
identity is top-level scalars (`delete_chart`'s `deleted_id`,
`create_dataset`'s `id`) map to an empty set, since no truncation phase drops a
top-level scalar. The minimal confirmation also selects from the payload's own
keys instead of a fixed chart-shaped list, so a dashboard response no longer
gains a `chart`/`explore_url`/`success` its model never declares.
Three mutating tools stay out, for reasons about what they commit rather
than response size: `execute_sql` commits against the analytics database and
already degrades via row truncation, and
`generate_explore_link`/`update_chart_preview` only cache a form_data key.
Tests assert the table equals `MUTATING_TOOLS` minus those three and check
every spec against the registered output schemas, so it can't drift from the
models.
##########
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:
Added — notes that read-only tools commit nothing, so hard-blocking an
oversized response there has no completed write for a retry to replay.
##########
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:
Added — notes that the map names the field to bisect, `get_chart_sql`'s
payload being dominated by `sql`.
--
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]
