gabotorresruiz commented on PR #44386:
URL: https://github.com/apache/superset/pull/44386#issuecomment-5764920766
@aminghadersohi Good fix, and the regression test earns its place. I checked
out `83f1ebc` and ran the new tests against the merge base `a7dab4a`: 24 of
them fail there, including
`test_update_chart_write_committed_before_size_check_still_succeeds` with
`Response blocked for update_chart: ~1049 tokens exceeds limit of 500`. The
full `tests/unit_tests/mcp_service/` tree passes on the head (4511 passed, 1
skipped). Two follow ups, neither a reason to revert anything.
#### Not a blocker, but the invariant this PR establishes does not hold when
`MCP_STRUCTURED_OUTPUT_ENABLED=True`
`_rewrap_as_tool_result` (`middleware.py:1230`) builds the replacement
`ToolResult` from `content` alone, so `structured_content` comes back `None`.
With `MCP_STRUCTURED_OUTPUT_ENABLED=True` the tool keeps its `outputSchema` in
`tools/list`, and the SDK rejects the call on the client side at
`mcp/client/session.py:433`.
I ran the shipped middleware stack
(`build_middleware_list(structured_output_enabled=...)` plus the guard) against
a tool returning a real `GenerateChartResponse`:
```text
truncation, structured off -> is_error=False (shipped default,
fine)
minimal fallback, structured off -> is_error=False (shipped default,
fine)
truncation, structured on -> RuntimeError: Tool update_chart has an
output schema but did not return structured content
minimal fallback, structured on -> RuntimeError: same
```
So with that flag on, a committed `update_chart` write still reaches the
caller as a hard error, just from a different layer. This predates the PR
(`get_dashboard_info` does the same thing at `a7dab4a`, I verified), but the
committed write tools only started taking this path here. Setting
`structured_content=payload` in that rewrap turns both structured on cases into
`is_error=False`, and the payload still validates against the real output
schema. Something like
`test_truncated_response_keeps_structured_content_when_structured_output_enabled`
would pin it.
#### Small one: `error` is bounded in the fallback but wiped in the path
that runs first
`_clip_error` (`middleware.py:1863`) deliberately keeps `error_type`,
`message` and `details`, and
`test_minimal_response_bounds_structured_error_object` asserts exactly that.
That test reaches the fallback by patching `estimate_response_tokens` to a
constant `600`. With the real estimator the response goes through
`_try_truncate_info_response` first, `error` is not in
`spec.identifying_fields` for `update_chart`, so Phase 5
`_replace_collections_with_summaries` (`token_utils.py:741`) sets it to `{}`,
the result then fits, and `_minimal_committed_write_response` never runs.
At the shipped `token_limit=25000`, an `update_chart` failure carrying 3000
`validation_errors` entries (53065 estimated tokens) comes back as:
```text
success = False
error = {}
notes = ["Field 'error' dict (4 keys) cleared to fit limit"]
```
`success: False` survives, so nobody reads it as a success, but `error_type`
and `message` are gone. Phase 3 only clips long strings, so many short nested
entries are exactly the shape that reaches Phase 5. Adding `error` to the
protected keys at `middleware.py:1726`, or running `_clip_error` before Phase
5, would give the truncation path the guarantee the fallback already makes, and
the same test without the estimator patch would lock it in.
Separately, and outside this PR: the size guard is not the only place a
committed `update_chart` can be reported as failed. I forced a raise in each
post commit step, and the handler at `update_chart.py:1134` returns `success:
False, chart: None` for a `ValueError`, `KeyError` or `AttributeError` raised
after `UpdateChartCommand.run()` has committed, while a `RuntimeError` out of
`_get_chart_preview_internal` escapes the preview `except` tuple entirely and
surfaces as `ToolError`. Happy to open a follow up for that if you think it is
worth one.
--
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]
