aminghadersohi commented on PR #41921: URL: https://github.com/apache/superset/pull/41921#issuecomment-5063205976
Thanks for the round-2 pass and the approval — folded the follow-ups into 11fc221ce4cfa180f22d6c7c1b95e460ba933826 so they land here rather than as a separate PR. **Medium — OAuth swallowed on the `form_data_key` path**: applied exactly your sketch — `_query_from_form_data` now re-raises `(OAuth2RedirectError, OAuth2Error)` ahead of its generic handler, so the caller's outer OAuth handlers produce the redirect. Added `test_oauth2_redirect_on_form_data_key_path` driving the full tool through the client with no identifier and a raising command: asserts `error_type == "OAUTH2_REDIRECT"`, not `DataError`. **Low — last-resort handler can raise via `str(exc)`**: the stripper's final except now guards its own formatting/sanitizing and falls back to `type(e).__name__` for both the response text and the hook's `sanitized_message`. Regression test with a hostile `__str__` asserts the handler returns `Error: HostileStrError` instead of propagating. **Low — test gaps**: the two metrics-guard tests now assert the swallowed error is logged (`"Failed to emit MCP tool metrics"`), added a chain-level test pinning the structured-error `.error` bucket through `build_middleware_list()`, and the hostile-name rejection is now also exercised through a reachable registry returning `None` (the production path), not just the regex branch. **Low — doc nits**: the Sentry example now practices its own caveat (`EventScrubber(recursive=True)` + `send_default_pii=False`, with a pointer to `before_send` for deployment-specific keys), and both the `mcp_config.py` contract comment and the example docstring now spell out per-path availability: `user_id`/`duration_ms` None on the last-resort path, `mcp_call_id` None outside a tool call, `tool_name` → `"unknown"` for non-tool messages. **Low — structured errors bucketing into `.error`** (design note): leaving as-is in this PR, deliberately. Agreed the tradeoff is real — mapping a curated set of user-class `error_type` strings to `.warning` (or a separate `.structured_error` key) would make `.error` mean "system failure", but with ~100 free-form `error_type` literals across the tools today any curated list would be immediately stale. The right sequencing is the planned `error_type` StrEnum consolidation from the audit (P1); once the vocabulary is closed, classifying structured responses becomes a one-line mapping. Happy to pick that up as the follow-up. -- 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]
