Skip to content

fix(slack): retain public channel discovery without groups scope #71114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Vasallius wants to merge 1 commit into
base: main
Choose a base branch
from
+104 −44
Open

fix(slack): retain public channel discovery without groups scope #71114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 67 additions & 40 deletions gateway/channel_directory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,9 @@ async def _build_slack(adapter) -> List[Dict[str, Any]]:
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). If the Slack app lacks channels:read, fall back to session
history quietly instead of logging a recurring warning every refresh.
proactively). If the Slack app lacks private-channel scopes, retries with
public channels only. If public-channel scopes are also absent, falls back
to session history quietly instead of logging on every refresh.
"""
team_clients = getattr(adapter, "_team_clients", None) or {}
if not team_clients:
Expand All @@ -276,49 +277,75 @@ async def _build_slack(adapter) -> List[Dict[str, Any]]:
seen_ids: set = set()

for team_id, client in team_clients.items():
try:
cursor: Optional[str] = None
for _page in range(20): # safety cap on pagination
cursor: Optional[str] = None
conversation_types = "public_channel,private_channel"
tried_public_only = False
page = 0

while page < 20: # safety cap on successful pagination
try:
response = await client.users_conversations(
types="public_channel,private_channel",
types=conversation_types,
exclude_archived=True,
limit=200,
cursor=cursor,
)
if not response.get("ok"):
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")
name = ch.get("name")
if not cid or not name or cid in seen_ids:
continue
seen_ids.add(cid)
channels.append({
"id": cid,
"name": name,
"type": "private" if ch.get("is_private") else "channel",
})
cursor = (response.get("response_metadata") or {}).get("next_cursor")
if not cursor:
break
except Exception as 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
except Exception as e:
error_code = _slack_api_error_code(e)
if error_code == "missing_scope" and not tried_public_only:
conversation_types = "public_channel"
tried_public_only = True
cursor = None
logger.debug(
"Channel directory: Slack team %s lacks private-channel scope; retrying public channels",
team_id,
)
continue
if error_code == "missing_scope":
logger.debug(
"Channel directory: Slack team %s lacks channel read scopes; using session history only",
team_id,
)
else:
_warn_slack_directory(team_id, str(e))
break

if not response.get("ok"):
error_code = response.get("error", "unknown")
if error_code == "missing_scope" and not tried_public_only:
conversation_types = "public_channel"
tried_public_only = True
cursor = None
logger.debug(
"Channel directory: Slack team %s lacks private-channel scope; retrying public channels",
team_id,
)
continue
if error_code == "missing_scope":
logger.debug(
"Channel directory: Slack team %s lacks channel read scopes; using session history only",
team_id,
)
else:
detail = f"users.conversations not ok: {error_code}"
_warn_slack_directory(team_id, detail)
break

page += 1
for ch in response.get("channels", []):
cid = ch.get("id")
name = ch.get("name")
if not cid or not name or cid in seen_ids:
continue
seen_ids.add(cid)
channels.append({
"id": cid,
"name": name,
"type": "private" if ch.get("is_private") else "channel",
})
cursor = (response.get("response_metadata") or {}).get("next_cursor")
if not cursor:
break

# Merge in DM/group entries discovered from session history.
# Build a lookup from API-discovered channels so we can enrich session entries.
Expand Down
41 changes: 37 additions & 4 deletions tests/gateway/test_channel_directory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,29 +568,62 @@ def test_skips_channels_with_no_id_or_name(self, tmp_path):

assert {e["id"] for e in entries} == {"C001"}

def test_response_not_ok_missing_scope_falls_back_quietly(self, tmp_path, caplog):
def test_response_missing_private_scope_retries_public_channels(self, tmp_path, caplog):
client = _make_slack_client([
{"ok": False, "error": "missing_scope"},
{
"ok": True,
"channels": [
{"id": "C001", "name": "public-only", "is_private": False},
],
"response_metadata": {},
},
])
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 {entry["id"] for entry in entries} == {"C001"}
assert client.users_conversations.await_args_list[0].kwargs["types"] == (
"public_channel,private_channel"
)
assert client.users_conversations.await_args_list[1].kwargs["types"] == "public_channel"
assert "missing_scope" not in caplog.text

def test_missing_scope_exception_falls_back_quietly(self, tmp_path, caplog):
def test_missing_scope_exception_retries_public_channels(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())
client.users_conversations = AsyncMock(side_effect=[
SlackLikeError(),
{
"ok": True,
"channels": [
{"id": "C002", "name": "public-after-error", "is_private": False},
],
"response_metadata": {},
},
])

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 {entry["id"] for entry in entries} == {"C002"}
assert client.users_conversations.await_args_list[1].kwargs["types"] == "public_channel"
assert "missing_scope" not in caplog.text

def test_missing_all_channel_scopes_falls_back_quietly(self, tmp_path, caplog):
client = _make_slack_client([
{"ok": False, "error": "missing_scope"},
{"ok": False, "error": "missing_scope"},
])
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 client.users_conversations.await_count == 2
assert "missing_scope" not in caplog.text

def test_repeated_workspace_errors_are_warning_throttled(
Expand Down