fix(slack): surface attachment access diagnostics
Translate Slack attachment failures into actionable user-facing notices
instead of generic download errors. When a scope/auth/permission issue
breaks attachment processing, the user sees:
[Slack attachment notice]
- Slack attachment access failed for photo.jpg. Missing scope:
files:read. Update the Slack app scopes/settings and reinstall
the app to the workspace.
Two helpers do the translation:
_describe_slack_api_error — handles SlackApiError responses
(missing_scope, invalid_auth, file_not_found, access_denied, etc.)
_describe_slack_download_failure — handles httpx.HTTPStatusError
(401/403/404) and Slack-returns-HTML-sign-in fallbacks
Wired into three existing call sites:
- the Slack Connect files.info path (PR #11111) so scope errors
surface instead of being logged as generic "files.info failed"
- the image, audio, and document download paths so 401/403 and
HTML-body responses translate into actionable notices
Adjustment from original PR: dropped _probe_slack_file_access_issue,
the proactive pre-download files.info probe. It added one extra
Slack API call per attachment even on healthy ones, and overlapped
with the existing files.info call from PR #11111. The post-failure
translation path covers the same user-facing diagnostic value
without the per-message tax.
Also documents files:read scope more prominently in the Slack setup
guide and troubleshooting table.
Contributed back from https://github.com/xinbenlv/zn-hermes-agent.
Closes #7015.
Co-authored-by: xinbenlv <zzn+pa@zzn.im>
This commit is contained in:
committed by
Teknium
parent
45bfcb9e71
commit
778fd1898e
@@ -540,7 +540,7 @@ from gateway.config import Platform, PlatformConfig # noqa: E402
|
||||
|
||||
|
||||
def _make_slack_adapter():
|
||||
config = PlatformConfig(enabled=True, token="xoxb-fake-token")
|
||||
config = PlatformConfig(enabled=True, token="***")
|
||||
adapter = SlackAdapter(config)
|
||||
adapter._app = MagicMock()
|
||||
adapter._app.client = AsyncMock()
|
||||
@@ -549,6 +549,39 @@ def _make_slack_adapter():
|
||||
return adapter
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# SlackAdapter diagnostics helpers
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestSlackAttachmentDiagnostics:
|
||||
def test_missing_scope_error_returns_actionable_notice(self):
|
||||
"""_describe_slack_api_error translates a missing_scope response into
|
||||
a user-facing notice mentioning the needed scope and the reinstall
|
||||
step. This is the helper used by every files.info call site (Slack
|
||||
Connect stubs + post-download failures) to surface scope problems
|
||||
without making an extra probe call per attachment.
|
||||
"""
|
||||
adapter = _make_slack_adapter()
|
||||
|
||||
response = {
|
||||
"error": "missing_scope",
|
||||
"needed": "files:read",
|
||||
"provided": "chat:write,files:write",
|
||||
}
|
||||
detail = adapter._describe_slack_api_error(response, file_obj={"id": "F123", "name": "photo.jpg"})
|
||||
assert detail is not None
|
||||
assert "files:read" in detail
|
||||
assert "reinstall" in detail.lower()
|
||||
assert "chat:write,files:write" in detail
|
||||
|
||||
def test_download_failure_403_returns_permission_notice(self):
|
||||
adapter = _make_slack_adapter()
|
||||
exc = _make_http_status_error(403)
|
||||
detail = adapter._describe_slack_download_failure(exc, file_obj={"name": "report.pdf"})
|
||||
assert "403" in detail
|
||||
assert "permission or scope" in detail
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# SlackAdapter._download_slack_file
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -511,6 +511,35 @@ class TestIncomingDocumentHandling:
|
||||
msg_event = adapter.handle_message.call_args[0][0]
|
||||
assert msg_event.message_type == MessageType.PHOTO
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_download_failure_is_surfaced_in_message_text(self, adapter):
|
||||
"""Attachment download failures (401/403/HTML-body/etc.) should be
|
||||
translated into a user-facing `[Slack attachment notice]` block so
|
||||
the agent can tell the user what to fix (e.g. missing files:read
|
||||
scope). No proactive files.info probe is made — the diagnostic
|
||||
runs only when the download actually fails.
|
||||
"""
|
||||
import httpx
|
||||
req = httpx.Request("GET", "https://files.slack.com/photo.jpg")
|
||||
resp = httpx.Response(403, request=req)
|
||||
|
||||
with patch.object(adapter, "_download_slack_file", new_callable=AsyncMock) as dl:
|
||||
dl.side_effect = httpx.HTTPStatusError("403", request=req, response=resp)
|
||||
event = self._make_event(text="what's in this?", files=[{
|
||||
"id": "F123",
|
||||
"mimetype": "image/jpeg",
|
||||
"name": "photo.jpg",
|
||||
"url_private_download": "https://files.slack.com/photo.jpg",
|
||||
"size": 1024,
|
||||
}])
|
||||
await adapter._handle_slack_message(event)
|
||||
|
||||
msg_event = adapter.handle_message.call_args[0][0]
|
||||
assert msg_event.message_type == MessageType.TEXT
|
||||
assert "[Slack attachment notice]" in msg_event.text
|
||||
assert "403" in msg_event.text
|
||||
assert "what's in this?" in msg_event.text
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# TestMessageRouting
|
||||
|
||||
Reference in New Issue
Block a user