codeant-ai-for-open-source[bot] commented on code in PR #39914: URL: https://github.com/apache/superset/pull/39914#discussion_r3507506196
########## superset/reports/notifications/slack.py: ########## @@ -53,7 +53,11 @@ logger = logging.getLogger(__name__) -# TODO: Deprecated: Remove this class in Superset 6.0.0 +# Deprecated: Slack v1 will be removed in the next major release. The Slack +# `files.upload` endpoint was retired in 2025, so file-bearing sends already +# fail at the API level; only text-only `chat_postMessage` sends still work +# here. Recipients with the `channels:read` scope are auto-upgraded to +# SlackV2 on first send via `update_report_schedule_slack_v2`. Review Comment: **Suggestion:** This deprecation comment is now factually inconsistent with the runtime behavior: the probe and warning path require both `channels:read` and `groups:read`, not only `channels:read`. Update the comment to match actual scope requirements so operators do not apply an incomplete fix. [comment mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Operators misconfigure Slack scopes from incomplete deprecation comment. - ⚠️ Slack v2 auto-upgrade delayed; deprecated v1 used longer. - ⚠️ Alerts with attachments fail until groups:read also granted. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `superset/reports/notifications/slack.py` and inspect the deprecation comment above `SlackNotification` at lines 56-60, which states that recipients with the `channels:read` scope are auto-upgraded to SlackV2 on first send. 2. Compare this comment with the runtime deprecation message `_SLACK_V1_DEPRECATION_MESSAGE` in `superset/utils/slack.py` at lines 39-46 and the scope-missing warning in `should_use_v2_api()` at lines 39-47, both of which explicitly require the `channels:read` and `groups:read` scopes. 3. Confirm via tests `TestShouldUseV2Api.test_returns_false_when_scope_missing_and_emits_deprecation_once` in `superset/tests/unit_tests/utils/slack_test.py` (around lines 51-80) and the follow-up assertions at lines 1-4 in the same file that the log message always mentions both `channels:read` and `groups:read` when scopes are missing. 4. In a real deployment, an operator reading only the inline comment in `superset/reports/notifications/slack.py` may grant only `channels:read` to the Slack bot; when a report with a Slack recipient runs (via `SlackNotification.send()` in `superset/reports/notifications/slack.py` lines 51-96 and `should_use_v2_api()` in `superset/utils/slack.py` lines 264-309), Slack still reports a missing-scope error and falls back to v1, because `groups:read` is also required, demonstrating that the comment’s scope requirement is incomplete and misleading. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=90a8e71ab9474eba8f5a78f5151e8c21&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=90a8e71ab9474eba8f5a78f5151e8c21&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/reports/notifications/slack.py **Line:** 59:60 **Comment:** *Comment Mismatch: This deprecation comment is now factually inconsistent with the runtime behavior: the probe and warning path require both `channels:read` and `groups:read`, not only `channels:read`. Update the comment to match actual scope requirements so operators do not apply an incomplete fix. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39914&comment_hash=e17878b3666b4c264078b3470a292ff47df88fb770b3736908b7483f5e051505&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39914&comment_hash=e17878b3666b4c264078b3470a292ff47df88fb770b3736908b7483f5e051505&reaction=dislike'>👎</a> ########## superset/utils/slack.py: ########## @@ -224,21 +256,75 @@ def get_channels_with_search( return channels +_SCOPE_MISSING_ERROR_CODES = frozenset( + {"missing_scope", "not_allowed_token_type", "no_permission"} +) + + def should_use_v2_api() -> bool: if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_SLACK_V2"): + _emit_v1_flag_off_deprecation() return False try: client = get_slack_client() + extra_params = {"types": _SLACK_CONVERSATION_TYPES} team_id = get_team_id() - client.conversations_list(**({"team_id": team_id} if team_id else {})) + if team_id: + extra_params["team_id"] = team_id + client.conversations_list( + limit=1, + exclude_archived=True, + **extra_params, + ) logger.info("Slack API v2 is available") return True - except SlackApiError: - # use the v1 api but warn with a deprecation message + except SlackApiError as ex: + # Only the scope-missing branch is a v1-deprecation signal; other + # SlackApiError codes (invalid_auth, ratelimited, server errors, etc.) + # are unrelated probe failures and should not be reported as a missing + # scope. We still fall back to v1 in both cases so a transient probe + # failure doesn't break sends — operators get an actionable log either + # way. + # `response` is normally a SlackResponse whose payload lives in `.data`, + # but the SDK (and our tests) can also hand back a plain dict. Read the + # error code in either shape so the scope-missing branch isn't missed. + response = getattr(ex, "response", None) + data = getattr(response, "data", None) + if not isinstance(data, dict): + data = response if isinstance(response, dict) else {} + error_code = data.get("error", "") + if error_code in _SCOPE_MISSING_ERROR_CODES: + # The DeprecationWarning fires once per process, but the actionable + # log line fires every send so operators see it in their report logs. + _emit_v1_scope_missing_deprecation() + logger.warning( + "Slack bot is missing the `channels:read` and `groups:read` " + "scopes; falling back to the deprecated " + "v1 API. %s", + _SLACK_V1_DEPRECATION_MESSAGE, + ) + else: + logger.warning( + "Slack v2 probe failed with error %r; falling back to the " + "deprecated v1 API for this send. Investigate the underlying " Review Comment: **Suggestion:** Falling back to v1 for every non-scope `SlackApiError` in the v2 probe creates a false-negative path where transient probe errors (for example rate limiting or temporary Slack outages) force the send down deprecated v1, which is known to fail for file attachments. Keep this branch from immediately forcing v1 for transient/probe-only failures (or attempt v2 send directly with retry) so recoverable probe errors do not convert into guaranteed delivery failures. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ File attachments fail on transient Slack v2 probe errors. - ⚠️ Notifications routed through deprecated v1 despite v2 capability. - ⚠️ Operators see generic probe warnings, masking delivery failures. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable the Slack v2 feature flag (`ALERT_REPORT_SLACK_V2=True`) and configure a report with a Slack recipient whose content includes inline files, e.g. screenshots, so that `_get_inline_files()` in `superset/reports/notifications/slack.py` at lines 40-49 returns a non-empty `files` list and `file_type` `"png"`. 2. During a report send, `SlackNotification.send()` in `superset/reports/notifications/slack.py` at lines 51-63 calls `should_use_v2_api()` (line 59); this probes Slack via `client.conversations_list(...)` inside `should_use_v2_api()` in `superset/utils/slack.py` at lines 264-280. 3. If Slack responds to the probe with a non-scope `SlackApiError` such as `"ratelimited"` or `"invalid_auth"` (the behavior exercised by `TestShouldUseV2Api.test_returns_false_without_scope_warning_on_other_slack_errors` in `superset/tests/unit_tests/utils/slack_test.py` at lines 38-75), the code enters the `else` branch at lines 303-308, logs a generic `"probe failed; falling back to the deprecated v1 API"` warning, and returns `False`, marking v2 as “unavailable” based solely on the probe error. 4. Because `should_use_v2_api()` returns `False`, `SlackNotification.send()` does not raise `SlackV1NotificationError` and instead proceeds down the v1 path, calling `client.files_upload(...)` for each file at lines 69-76; as documented in the deprecation comment in `superset/reports/notifications/slack.py` lines 17-21 and `_SLACK_V1_DEPRECATION_MESSAGE` in `superset/utils/slack.py` lines 39-46, Slack retired the `files.upload` endpoint and file-bearing sends via v1 “already fail at the API level”, so this transient v2 probe error is converted into a guaranteed failure for all file attachments rather than attempting a v2 send with built-in retries. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5042e55dc904499b908b14f32f0c8b13&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5042e55dc904499b908b14f32f0c8b13&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/slack.py **Line:** 303:309 **Comment:** *Logic Error: Falling back to v1 for every non-scope `SlackApiError` in the v2 probe creates a false-negative path where transient probe errors (for example rate limiting or temporary Slack outages) force the send down deprecated v1, which is known to fail for file attachments. Keep this branch from immediately forcing v1 for transient/probe-only failures (or attempt v2 send directly with retry) so recoverable probe errors do not convert into guaranteed delivery failures. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39914&comment_hash=b1583065ef9b6694b8c28147a1ac35a0ff7940ff65888e52d8d1146662643552&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39914&comment_hash=b1583065ef9b6694b8c28147a1ac35a0ff7940ff65888e52d8d1146662643552&reaction=dislike'>👎</a> -- 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]
