aminghadersohi commented on code in PR #44386:
URL: https://github.com/apache/superset/pull/44386#discussion_r4037870891
##########
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:
Your premise is right, and it surfaced a real bug — but the conclusion about
`COMMITTED_WRITE_TOOLS` itself is not one I'm taking. Splitting the two:
**The premise is correct.** In `update_chart.py`, `UpdateChartCommand` runs
only under `if not request.generate_preview:`; the default
`generate_preview=True` branch just calls `_create_preview_url`, which caches
`form_data`. Nothing is persisted, and the response carries
`chart.is_unsaved_state = not saved`.
**The real bug this exposed.** `_shrink_minimal_response` reduced `chart` to
`("id", "uuid", "slice_name", "url")` — dropping `is_unsaved_state`, the
caller's only in-band way to tell a cached preview from a persisted write.
Combined with a note saying the call "completed and was not rolled back," a
shrunken preview response was genuinely hard to distinguish from a committed
update. Fixed in 28096e0041 by retaining the flag, with a regression test
(`test_minimal_response_keeps_unsaved_state_flag`).
**Why the membership stays.** `COMMITTED_WRITE_TOOLS` is not a claim that a
write happened; it selects the *failure mode* for an oversized response —
truncate-and-return instead of raising `ToolError`. Over-inclusion is therefore
safe in one direction only: a preview call gets a truncated-but-useful response
rather than a hard error. It never fabricates a write, and no user-visible
string asserts persistence (the note deliberately avoids "committed"; asserted
by `test_committed_write_falls_back_to_minimal_response_not_error`).
Gating on `generate_preview` would be actively worse. The middleware sees
`context.message.arguments`, and `update_chart` takes a single `request` model
— the arguments are `{"request": {...}}`, so a top-level `generate_preview`
lookup is always absent and defaults to `True`. A real `generate_preview=False`
write would be classified as a preview and hard-blocked, which is exactly the
bug this PR fixes. Reaching into `params["request"]["generate_preview"]` would
couple the middleware to each tool's schema shape to buy a strictly worse
outcome on the path it gets wrong.
So: flag retained, predicate unchanged.
--
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]
