aminghadersohi commented on PR #43828: URL: https://github.com/apache/superset/pull/43828#issuecomment-5574851854
Addressed all three blocking findings and reviewed the newer automated suggestions: - **Factory default-disabled validation:** factory-created servers now install the same complete core middleware stack as built-in startup, before custom middleware. The compatibility middleware also keeps a protocol result envelope when stripping structured content, so it remains safe even when used independently. A real streamable-HTTP regression exercises an isolated, factory-like FastMCP server in default-disabled mode and verifies a successful text-only response rather than an output-validation error. - **Task protocol:** restored the exact `isinstance(result, ToolResult)` guard, widened the middleware result typing, and added a regression using a real `CreateTaskResult` to prove pass-through without attribute access. - **`get_chart_info`:** the selected-column success path and not-found error path both execute through the protocol client and validate `structuredContent` against the advertised `outputSchema`; the native-tool inventory also locks representative chart-info fields. - **Additional consistency findings:** removed the obsolete dashboard `result.data` assertion, centralized structured-output configuration resolution, and made OAuth chart-preview errors include the schema/API version fields required by their declared contract, with coverage for both OAuth exception types. I kept three low-priority suggestions unchanged after evaluation: - `DeprecationWarning` remains appropriate for a library import alias; the migration is explicitly surfaced in `UPDATING.md` and the MCP administration documentation, while a runtime logger warning would add unsolicited server noise. - Dataset validation intentionally uses a filtered/column-trimmed response because that is the serializer projection and alias-drift boundary the regression is designed to protect; the schema deliberately permits the full model. - The native-tool inventory intentionally uses exact equality rather than a subset. This PR is an exhaustive metadata audit, so allowing newly registered tools to bypass review would weaken the regression. Local validation is green: 260 focused tests, 3,735 tests in the full MCP unit suite, and all staged pre-commit hooks. The PR remains ready for review. -- 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]
