codeant-ai-for-open-source[bot] commented on code in PR #44386:
URL: https://github.com/apache/superset/pull/44386#discussion_r4036445807
##########
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:
**Suggestion:** Every `update_chart` response is treated as a committed
write, although the default `generate_preview=True` only caches an unsaved
preview and does not persist changes.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Often` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fac55c1892b34ea294a8a75eee8070cc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fac55c1892b34ea294a8a75eee8070cc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/utils/token_utils.py
**Line:** 500:504
**Comment:**
*Api Mismatch: Every `update_chart` response is treated as a committed
write, although the default `generate_preview=True` only caches an unsaved
preview and does not persist changes.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44386&comment_hash=7a22b31e5a79a2b300c5d923a9415ea2ee10ca6cf5516052bb2bca7894ccd852&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44386&comment_hash=7a22b31e5a79a2b300c5d923a9415ea2ee10ca6cf5516052bb2bca7894ccd852&reaction=dislike'>๐</a>
##########
superset/mcp_service/middleware.py:
##########
@@ -1357,6 +1388,193 @@ def _try_truncate_data_query_response(
return truncated
+ def _try_truncate_string_field_response(
+ self,
+ tool_name: str,
+ response: Any,
+ estimated_tokens: int,
+ field: str,
+ ) -> Any | None:
+ """Attempt to truncate a response by bisecting one oversized string
field.
+
+ Returns the truncated response if successful, None otherwise.
+ """
+ extracted = self._extract_payload_from_tool_result(response)
+ if extracted is None and isinstance(response, ToolResult):
+ # A ToolResult whose payload can't be parsed is opaque: truncating
+ # it would model_dump() the wrapper itself and hand FastMCP a
+ # plain dict, which then fails in to_mcp_result(). Decline instead
+ # and let the caller fall through to its own fallback.
+ logger.warning(
+ "Cannot truncate %s: ToolResult payload is not a JSON object",
+ tool_name,
+ )
+ return None
+
+ truncation_target = extracted if extracted is not None else response
+
+ try:
+ truncated, was_truncated, notes = truncate_string_field_response(
+ truncation_target, self.token_limit, field
+ )
+ except Exception as trunc_error: # noqa: BLE001
+ logger.warning(
+ "String field truncation failed for %s due to %s: %s",
+ tool_name,
+ type(trunc_error).__name__,
+ trunc_error,
+ )
+ return None
+
+ if not was_truncated:
+ return None
+
+ truncated_tokens = estimate_response_tokens(truncated)
+ if truncated_tokens > self.token_limit:
+ return None
+
+ logger.warning(
+ "Response for %s truncated from ~%d to ~%d tokens (limit: %d). %s",
+ tool_name,
+ estimated_tokens,
+ truncated_tokens,
+ self.token_limit,
+ "; ".join(notes),
+ )
+
+ try:
+ user_id = get_user_id()
+ event_logger.log(
+ user_id=user_id,
+ action="mcp_response_truncated",
+ dashboard_id=None,
+ duration_ms=None,
+ slice_id=None,
+ referrer=None,
+ curated_payload={
+ "tool": tool_name,
+ "original_tokens": estimated_tokens,
+ "truncated_tokens": truncated_tokens,
+ "token_limit": self.token_limit,
+ "truncation_notes": notes,
+ },
+ )
+ except Exception as log_error: # noqa: BLE001
+ logger.warning("Failed to log truncation event: %s", log_error)
+
+ if extracted is not None and isinstance(truncated, dict):
+ return self._rewrap_as_tool_result(truncated, response)
+
+ return truncated
+
+ def _minimal_committed_write_response(
+ self,
+ tool_name: str,
+ response: Any,
+ estimated_tokens: int,
+ ) -> Any:
+ """Build a guaranteed-small success response for a committed write.
+
+ Last-resort fallback for COMMITTED_WRITE_TOOLS: reached only when
+ even the nuclear phase of ``truncate_oversized_response`` can't bring
+ the response under budget (in practice this should not happen, since
+ the protected identifying field alone is tiny). The underlying
+ mutation already committed by the time this middleware runs, so this
+ path must never raise -- it keeps only what confirms the write
+ succeeded and drops everything else.
+ """
+ if (extracted := self._extract_payload_from_tool_result(response)) is
not None:
+ payload = extracted
+ elif isinstance(response, dict):
+ payload = response
+ else:
+ payload = {}
+ truncation_notes = [
+ f"Response for {tool_name} exceeded the size limit even after "
+ "truncation; non-essential fields were dropped. The tool call "
+ "itself completed and was not rolled back by this size limit -- "
+ "re-read the chart to see its full state."
+ ]
+ minimal = {
+ "chart": payload.get("chart"),
+ "error": payload.get("error"),
+ "success": payload.get("success", True),
+ "explore_url": payload.get("explore_url"),
+ "schema_version": payload.get("schema_version"),
+ "api_version": payload.get("api_version"),
+ "_response_truncated": True,
+ "_truncation_notes": truncation_notes,
+ }
+ self._shrink_minimal_response(minimal)
+ logger.warning(
+ "Response for %s could not fit under the size limit after full "
+ "truncation (~%d tokens, limit %d); returning a minimal write "
+ "confirmation instead of blocking a completed write.",
+ tool_name,
+ estimated_tokens,
+ self.token_limit,
+ )
+ try:
+ user_id = get_user_id()
+ event_logger.log(
+ user_id=user_id,
+ action="mcp_response_truncated",
+ dashboard_id=None,
+ duration_ms=None,
+ slice_id=None,
+ referrer=None,
+ curated_payload={
+ "tool": tool_name,
+ "original_tokens": estimated_tokens,
+ "token_limit": self.token_limit,
+ "truncation_notes": truncation_notes,
+ },
+ )
+ except Exception as log_error: # noqa: BLE001
+ logger.warning("Failed to log truncation event: %s", log_error)
+
+ # Rewrap whenever the tool returned a ToolResult, including the case
+ # where its payload could not be parsed: returning a bare dict there
+ # would blow up in FastMCP's ``result.to_mcp_result()`` and surface
+ # the completed write as an internal error after all.
+ if isinstance(response, ToolResult):
+ return self._rewrap_as_tool_result(minimal, response)
+ return minimal
+
+ def _shrink_minimal_response(self, minimal: dict[str, Any]) -> None:
+ """Force ``minimal`` under the token limit, degrading ``chart`` in
place.
+
+ ``chart`` is copied from the *untruncated* payload, so when it is
+ itself the oversized field the "minimal" response is not actually
+ small. Reduce it to identifying scalars, which is bounded by
+ construction, rather than handing back something the transport will
+ reject.
+
+ Only one measurement is taken, and a failed measurement counts as
+ "too big": the reduced form is small enough that there is nothing to
+ re-check, and the chart identity is never dropped just because the
+ estimator errored -- surfacing which chart was written is the whole
+ point of this fallback.
+ """
+ if _fits(minimal, self.token_limit):
+ return
+
+ chart = minimal.get("chart")
+ if isinstance(chart, dict):
+ minimal["chart"] = {
+ key: chart[key]
+ for key in ("id", "uuid", "slice_name", "url")
+ if key in chart
+ }
+ minimal["_truncation_notes"].append(
+ "Chart details reduced to identifying fields only."
+ )
+ else:
+ minimal["chart"] = None
+ minimal["_truncation_notes"].append(
+ "Chart details omitted entirely to fit the size limit."
Review Comment:
**Suggestion:** `_shrink_minimal_response` checks size only before reduction
and never rechecks afterward, so tiny limits or long notes can still produce an
over-budget response.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Possible bug`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ede9e83900e84f24a99d1d57ba5bd7bd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ede9e83900e84f24a99d1d57ba5bd7bd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/middleware.py
**Line:** 1559:1575
**Comment:**
*Possible Bug: `_shrink_minimal_response` checks size only before
reduction and never rechecks afterward, so tiny limits or long notes can still
produce an over-budget response.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44386&comment_hash=28e3585ec097b18e3753c3fcbe52cea5eeef4e830ce1bbc62fe0ea8036f3c051&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44386&comment_hash=28e3585ec097b18e3753c3fcbe52cea5eeef4e830ce1bbc62fe0ea8036f3c051&reaction=dislike'>๐</a>
--
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]
