aminghadersohi commented on PR #45081: URL: https://github.com/apache/superset/pull/45081#issuecomment-6058678763
## Acceptance test at head e388bda075d893b09494ada7c2308bc26f08fd1c **Setup.** Local Python 3.11 venv from `requirements/development.txt` (fastmcp 3.4.7), with a throwaway SQLite metadata DB, an `admin` user and a `Gamma` user. I ran the real HTTP server (`superset mcp run`) in both discovery modes (`MCP_TOOL_SEARCH_CONFIG["enabled"]` True/False), as each user, with `MCP_STRUCTURED_OUTPUT_ENABLED` both off and on. All calls went through a FastMCP `Client` over streamable HTTP. For the "before" runs, the same server was started with `master`'s `middleware.py`. All servers were stopped afterwards, and the only writes were to the throwaway DB. The PR also runs green locally: the two test files pass, 93 passed. Swapping `master`'s `middleware.py` back in gives 8 failures: 7 in `test_native_tool_surface.py` and 1 in `test_middleware_logging.py`. That matches the PR body. CI is green at this head. | # | Acceptance criterion (as claimed in the PR body) | Result | Evidence (live server) | |---|---|---|---| | 1 | Every allowed operation is exposed under its real name with description, input schema, output schema when enabled, and annotations | **PASS** | Native `tools/list` as admin returns 77 tools in one page (`nextCursor: null`) with no `search_tools`/`call_tool`. All 77 carry `readOnlyHint` and `destructiveHint`. `idempotentHint` is set on 30; leaving it unset on read-only tools is a gap the PR documents. With structured output on, 77/77 have `outputSchema`; with it off, 0/77. The inventory's 79 tools minus `list_tasks`/`get_task_info` = 77; those two are removed by the `GLOBAL_TASK_FRAMEWORK` flag. | | 1 | `isError`, `structuredContent` and validation details are preserved | **PASS** (fixed by this PR) | Before/after through `call_tool`. **Gamma `create_theme`:** before = `isError=False` with "Permission denied: can_write on Theme…", after = `isError=True` with the same text. **Missing `chart_type` on `get_chart_type_schema`:** before = `isError=False` with "Validation error…: chart_type: Field required", after = `isError=True` with the same text. With the fix, all three paths (native, direct named call in compatibility mode, `call_tool`) return identical text and `isError=True`. The server log records the proxied denial as `success=False`; before the fix it was `success=True`. A successful `get_chart_type_schema` returns identical content on all three paths, with `structuredContent` present only when structured output is enabled. | | 2 | Native mode needs no schema fetch or opaque dispatcher; compatibility mode keeps working and is tested | **PASS** | Native mode calls tools directly by name. Compatibility mode lists `health_check`, `get_instance_info`, `search_tools` and `call_tool` in 2,675 bytes; real names stay directly callable, and `search_tools`/`call_tool` work. Defaults are unchanged. | | 2 | Migration and deprecation policy documented without breaking existing clients | **PASS** | `mcp-server.mdx` adds "Tool discovery modes" and "Migrating to native mode", with mixed-version and multi-pod notes. It states that compatibility mode stays supported and that any default change will be announced in `UPDATING.md`. | | 3 | Canonical definitions reused, not a second catalog; both structured-output settings tested | **PASS** | The tests and the inventory are generated from the registered tools over the MCP protocol. Live: text-only native catalog is 215,049 bytes and structured is 516,627 bytes. The largest entries are `manage_native_filters` at 10,916 bytes text-only and `generate_chart` at 28,245 bytes structured, within the inventory's 2%/200-byte headroom. | | 4 | List and call coverage for every tool; deterministic pagination; schemas valid with no dangling refs; catalog and largest tool/page measured | **PASS** | Live structured listing: all 154 input and output schemas pass `Draft202012Validator.check_schema`, and none contains a `$ref` (they are dereferenced). `list_charts` `structuredContent` validates against its advertised `outputSchema`. Two consecutive listings come back in the same order. No single entry is over 100 KB. The full native catalog is larger than one 100 KB page and is not paginated; the docs and tests say so. | | 5 | Same RBAC, scopes, restricted-role policy and service flags on both paths; named calls cannot bypass filtering; permission revocation applies | **PASS** | **Gamma:** native `tools/list` shows 48 of 77 tools; `create_theme` is absent from both the list and `search_tools`, and calling it by name or through `call_tool` is denied with the same error. **Live revocation, no restart:** after granting `can_write on Theme` to Gamma, `create_theme` is listed natively, found by search, and succeeds on both paths. After revoking it, the next request hides it on both paths and denies the call on both. **Flag-removed `list_tasks`:** rejected natively and through `call_tool`, and absent from search. Token scopes and the restricted-principal policy are covered by the PR's contract tests, which pass, rather than this live run. | | 6 | Superset-side changes only; existing native code retained, missing tests and docs added | **PASS** | Only `middleware.py` changes (+7/−1), plus tests, the inventory and docs. Native mode itself already existed. | **Observation, not blocking:** a tool removed by a service flag, called by name, returns `Internal error in list_tasks: An unexpected error occurred` rather than an "unknown tool" message. It is still rejected (`isError=True`) on every path, and `master` behaves the same way. **Verdict: PASS.** No changes pushed. -- 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]
