From c1529e58da2202328b13d3aef80c92a41cbeb4a2 Mon Sep 17 00:00:00 2001 From: nanckh <35049985+nanckh@users.noreply.github.com> Date: Wed, 3 Jun 2026 19:44:36 +0900 Subject: [PATCH] fix(gateway): quiet Slack missing_scope channel directory fallback Treat Slack users.conversations missing_scope as an expected limited-scope app condition and fall back to session history without recurring warnings. Add tests for not-ok and SlackApiError-like missing_scope responses. --- gateway/channel_directory.py | 37 ++++++++++++++++++++++--- tests/gateway/test_channel_directory.py | 29 +++++++++++++++---- 2 files changed, 57 insertions(+), 9 deletions(-) diff --git a/gateway/channel_directory.py b/gateway/channel_directory.py index 939ab20ffc09..2eb83585fe98 100644 --- a/gateway/channel_directory.py +++ b/gateway/channel_directory.py @@ -244,13 +244,29 @@ def _build_discord(adapter) -> List[Dict[str, str]]: return channels +def _slack_api_error_code(error: Exception) -> Optional[str]: + """Return Slack Web API error code from SlackApiError-like exceptions.""" + response = getattr(error, "response", None) + if isinstance(response, dict): + value = response.get("error") + return str(value) if value else None + if response is not None: + try: + value = response.get("error") + return str(value) if value else None + except Exception: + pass + return None + + async def _build_slack(adapter) -> List[Dict[str, Any]]: """List Slack channels the bot has joined across all workspaces. Uses ``users.conversations`` against each workspace's web client. Pulls public + private channels the bot is a member of, then merges in DMs discovered from session history (IMs aren't useful to enumerate - proactively). + proactively). If the Slack app lacks channels:read, fall back to session + history quietly instead of logging a recurring warning every refresh. """ team_clients = getattr(adapter, "_team_clients", None) or {} if not team_clients: @@ -270,8 +286,15 @@ async def _build_slack(adapter) -> List[Dict[str, Any]]: cursor=cursor, ) if not response.get("ok"): - detail = f"users.conversations not ok: {response.get('error', 'unknown')}" - _warn_slack_directory(team_id, detail) + error_code = response.get("error", "unknown") + if error_code == "missing_scope": + logger.debug( + "Channel directory: Slack team %s lacks channels:read; using session history only", + team_id, + ) + else: + detail = f"users.conversations not ok: {error_code}" + _warn_slack_directory(team_id, detail) break for ch in response.get("channels", []): cid = ch.get("id") @@ -288,7 +311,13 @@ async def _build_slack(adapter) -> List[Dict[str, Any]]: if not cursor: break except Exception as e: - _warn_slack_directory(team_id, str(e)) + if _slack_api_error_code(e) == "missing_scope": + logger.debug( + "Channel directory: Slack team %s lacks channels:read; using session history only", + team_id, + ) + else: + _warn_slack_directory(team_id, str(e)) continue # Merge in DM/group entries discovered from session history. diff --git a/tests/gateway/test_channel_directory.py b/tests/gateway/test_channel_directory.py index 488655f5ac85..e11d6d7a6d5a 100644 --- a/tests/gateway/test_channel_directory.py +++ b/tests/gateway/test_channel_directory.py @@ -568,21 +568,40 @@ class TestBuildSlack: assert {e["id"] for e in entries} == {"C001"} - def test_response_not_ok_breaks_pagination_for_that_workspace(self, tmp_path): + def test_response_not_ok_missing_scope_falls_back_quietly(self, tmp_path, caplog): client = _make_slack_client([ {"ok": False, "error": "missing_scope"}, ]) - with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}), caplog.at_level("WARNING"): entries = asyncio.run(_build_slack(_make_slack_adapter({"T1": client}))) assert entries == [] + assert "missing_scope" not in caplog.text + + def test_missing_scope_exception_falls_back_quietly(self, tmp_path, caplog): + class SlackLikeError(Exception): + def __init__(self): + super().__init__("The request to the Slack API failed") + self.response = {"ok": False, "error": "missing_scope"} + + client = MagicMock() + client.users_conversations = AsyncMock(side_effect=SlackLikeError()) + + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}), caplog.at_level("WARNING"): + entries = asyncio.run(_build_slack(_make_slack_adapter({"T1": client}))) + + assert entries == [] + assert "missing_scope" not in caplog.text def test_repeated_workspace_errors_are_warning_throttled( self, tmp_path, caplog, monkeypatch ): + # NOTE: uses a non-missing_scope error code — missing_scope is + # demoted to DEBUG entirely (quiet channels:read fallback), so the + # throttle path only sees other recurring workspace errors. client = _make_slack_client([ - {"ok": False, "error": "missing_scope"}, - {"ok": False, "error": "missing_scope"}, + {"ok": False, "error": "ratelimited"}, + {"ok": False, "error": "ratelimited"}, ]) _slack_directory_warning_last.clear() monkeypatch.setattr("gateway.channel_directory.time.monotonic", lambda: 1000.0) @@ -597,7 +616,7 @@ class TestBuildSlack: if record.levelname == "WARNING" ] assert len(warning_messages) == 1 - assert "missing_scope" in warning_messages[0] + assert "ratelimited" in warning_messages[0] assert any( "suppressed repeated Slack channel list failure" in record.getMessage() for record in caplog.records