rebenitez1802 commented on PR #41698:
URL: https://github.com/apache/superset/pull/41698#issuecomment-4960352508
from my agent reviewer:
🟡 Medium — New max_list_items config key missing from admin docs
docs/admin_docs/configuration/mcp-server.mdx:527-546 enumerates every
MCP_RESPONSE_SIZE_CONFIG key (code block + table), but the PR’s new
operator-facing max_list_items knob isn’t added there. Operators reading the
canonical config reference won’t discover the setting the PR’s own docstrings
point them to. Fix: add "max_list_items": 100 to the code block and a matching
table row (leave the frozen version-6.1.0/… snapshot untouched).
🟡 Medium — Docstring/field-description “regression” tests assert on
documentation text
test_dashboard_schemas.py:284-298 and test_dashboard_tools.py:312-325 assert
substrings ("list_charts", "dashboards", "page", "max_list_items") exist in
docstrings/descriptions — they pin wording, not behavior, and break on harmless
rewording. Defensible since these strings are the load-bearing agent escape
hatch per the acceptance criteria, but consider narrowing to one critical token
per surface, or leaning on the behavioral truncation tests that already prove
the cap is exposed.
🟢 Low — truncate_oversized_response / _truncate_lists don’t clamp
max_list_items
The middleware __init__ clamps max(1, max_list_items) (middleware.py:1192),
but truncate_oversized_response (token_utils.py:601) and _truncate_lists
(token_utils.py:~543) don’t. The sole production caller is the clamped
middleware, so it’s not currently reachable, but a direct caller passing 0
empties every list and a negative value silently drops the tail via
value[:max_items]. Fix: add max_items = max(1, max_items) at the top of
_truncate_lists so the guarantee lives at the mutation site.
🟢 Low — No test for a cap below Phase 4’s hardcoded 10
Phase 4 uses _truncate_lists(..., max_items=10) (token_utils.py:651). A
max_list_items of e.g. 5 caps in Phase 2 first, and Phase 4 (which only
shrinks) won’t re-expand it — correct, but untested. Fix: add one
truncate_oversized_response(payload, tiny_limit, max_list_items=5) case
reaching Phase 4, asserting len(...) <= 5.
🟢 Low — list_charts docstring omits dashboards from its filter-column list
chart/tool/list_charts.py:106-108 lists valid filter columns (slice_name,
viz_type, …) but not dashboards. The escape hatch itself works (ChartFilter.col
accepts "dashboards" with eq), but an agent trusting this list over the new
get_dashboard_info guidance may think it’s invalid. This PR makes dashboards a
documented happy path, so adding it here is worthwhile.
🟢 Low — Backward-compat: 30→100 default change is a silent behavior shift
Raising the Phase-2 cap changes response sizes for all four info tools
(get_chart_info, get_dataset_info, get_dashboard_info, get_instance_info) for
any tenant that hit the old cap. It’s monotonically safe (a higher cap only
keeps more before falling through, still bounded by token_limit), so not an
UPDATING.md breaking change — but a one-line note in the docs Response Size
Guard section on the new default and how to lower it would help operators who
tuned around 30.
🟢 Low — DEFAULT_MAX_LIST_ITEMS formatting/typing inconsistent with siblings
constants.py:9-11 adds : int and wraps the value in parens just to fit a
trailing comment, producing an awkward 3-line form; the two constants above use
a bare literal + inline #. The : int annotation is redundant (int literal is
inferred). Fix: DEFAULT_MAX_LIST_ITEMS = 100 with a leading comment. Related
nit: _MAX_LIST_ITEMS in token_utils.py:473 is now just an alias of
DEFAULT_MAX_LIST_ITEMS — inlining the constant as the param default and
dropping the alias would remove a layer of indirection.
Nice-to-note for the PR description: _safe_int_config (middleware.py:1426)
also fixes a real latent bug — pre-change, a malformed
token_limit/warn_threshold_pct (None or non-numeric) would raise past the
narrow except (ImportError, AttributeError, KeyError) and abort middleware
init. Routing all three keys through the helper hardens them too; worth calling
out.
--
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]
