9bd7a37a77
* fix: _extract_retry_after returns None for non-retryable errors
* fix: update _extract_retry_after to handle generic transient errors with default retry delay
* fix: extend base _non_retryable_patterns in channel subclasses
* fix(channels): merge structured SDK error check into non-retryable step
* fix(channels): decouple status code and SDK error code extraction in retry logic
- Independently evaluate HTTP status codes and structured SDK error codes
- Fix misleading doc comments for Feishu and DingTalk patterns
- Remove redundant try-except AttributeError on getattr with default
- Expand test coverage for dual-signal matrix and header parsing
* refactor(channels): simplify status code and SDK error extraction via channel overrides
- Handle httpx and aiohttp exceptions in base Channel class
- Override _extract_status_code and _extract_sdk_error_code in SlackChannel and DiscordChannel
- Replace mock exception types in comprehensive test suite with real httpx and aiohttp errors
- Add dedicated Slack and Discord retry error extraction test suites
* fix(channels): clean up Slack and Discord error code extraction
- Remove defensive string checks and attribute guards in SlackChannel
- Directly access exc.response.status_code and exc.response.get('error') in SlackChannel
- Remove unnecessary _extract_sdk_error_code override in DiscordChannel
- Use real SlackApiError, SlackResponse, and discord.HTTPException in unit tests
* refactor: reorder retry logic to prioritize non-retryable checks, remove aiohttp dependency, and clean up exception handling in base and channel modules.
* test(channels): skip Slack/Discord retry tests when the SDK extra is absent
The retry-extraction tests build real SlackApiError / discord.HTTPException
objects, but slack-sdk and discord.py are optional extras that the dev
dependency group does not install. Under CI's `uv sync --dev` all nine
tests failed with ModuleNotFoundError raised from the channel override.
Gate both test classes with skipif(find_spec(...) is None) so the suite is
green without the extras and the tests still run wherever they are installed.
* refactor(channels): replace retry-delay lookup with _extract_retry_delay
_extract_retry_after still read the server-supplied delay by probing
exc.retry_after and exc.response.headers via getattr/hasattr, the last
remnant of the pattern the extractors moved away from. Replace both steps
with one overridable hook, _extract_retry_delay, implemented against the
real exception types:
- base: httpx.HTTPStatusError -> Retry-After header (httpx.Headers is
case-insensitive; HTTP-date form remains unsupported)
- SlackChannel: SlackApiError -> Retry-After, matched case-insensitively
because SlackResponse.headers is a plain dict whose casing depends on the
HTTP client (same approach as slack_sdk's RateLimitErrorRetryHandler)
- TelegramChannel: telegram.error.RetryAfter.retry_after (int, or timedelta
under PTB_TIMEDELTA)
- DiscordChannel: discord.RateLimited.retry_after, which the old duck-typed
getattr matched and would otherwise have been lost
Drop the isinstance(retry, bool) and val >= 0 guards; no SDK produces those.
Delete the test that asserted the duck-typed attribute; add real-object tests
for each override, guarded like the existing SDK-dependent classes.
* ci: install the all-channels extra so SDK-dependent channel tests run
The Slack, Discord, and Telegram retry tests build real SDK exception
objects and are skipped when the SDK is absent. CI only ran `uv sync --dev`,
so those tests never executed there. Install the existing all-channels
extra alongside the dev group; the skipif guards remain for lean local runs.
* fix(channels): honor HTTP-date Retry-After and tolerate malformed values
RFC 9110 allows Retry-After as either delay-seconds or an HTTP-date. The
httpx path treated a date as unparseable and fell back to the 1.0 s default,
so a 503 asking for a specific wait was retried too early. Add
Channel._parse_retry_after, which returns delay-seconds as-is and converts
an HTTP-date to the non-negative seconds until it (tz-less dates read as
UTC).
SlackChannel used a bare float() on the header. A non-numeric value raised
inside the retry predicate, which escapes retry_async and drops the chunk
instead of retrying. Route Slack through the same helper so a bad header
falls back to _rate_limit_delay.
Addresses CodeRabbit review comments on base.py:866 and slack/channel.py:229.
* fix(channels): treat HTTP 400 and 404 as non-retryable
Both are permanent for a given request, so retrying burns the attempt
budget for nothing. Add them to _non_retryable_status_codes alongside
401/403.
Deliberately not a 4xx range check: 408 and 425 are retryable by
definition and 429 is handled by the rate-limit path. A test pins 408 as
still retryable so the range shortcut is not reintroduced later.
Partially addresses CodeRabbit's outside-diff comment on base.py:749-750.
* fix(channels): guard Slack retry extractors against raw aiohttp responses
slack_sdk attaches the bare aiohttp.ClientResponse to SlackApiError when a
JSON-declared body fails to parse. That object has neither status_code nor
get(), so _extract_status_code raised AttributeError inside should_retry,
replacing the original error and skipping the remaining attempts. Narrow
both extractors to SlackResponse/AsyncSlackResponse so such errors fall
through to the message patterns and retry as before. Add a wire-level
regression test against a local aiohttp server.
---------
Co-authored-by: Dinos Papakostas <dinospk1999@gmail.com>
Co-authored-by: X-iZhang <zacharyzhang2022@gmail.com>
265 lines
9.7 KiB
Python
265 lines
9.7 KiB
Python
"""Tests for Slack channel implementation."""
|
|
|
|
import importlib.util
|
|
|
|
import pytest
|
|
|
|
from EvoScientist.channels.base import ChannelError
|
|
from EvoScientist.channels.slack.channel import SlackChannel, SlackConfig
|
|
|
|
|
|
class TestSlackConfig:
|
|
def test_default_values(self):
|
|
config = SlackConfig()
|
|
assert config.bot_token == ""
|
|
assert config.app_token == ""
|
|
assert config.allowed_senders is None
|
|
assert config.allowed_channels is None
|
|
assert config.text_chunk_limit == 4096
|
|
|
|
def test_custom_values(self):
|
|
config = SlackConfig(
|
|
bot_token="xoxb-test",
|
|
app_token="xapp-test",
|
|
allowed_senders={"U123"},
|
|
allowed_channels={"C456"},
|
|
text_chunk_limit=2000,
|
|
)
|
|
assert config.bot_token == "xoxb-test"
|
|
assert config.app_token == "xapp-test"
|
|
assert config.allowed_senders == {"U123"}
|
|
assert config.allowed_channels == {"C456"}
|
|
assert config.text_chunk_limit == 2000
|
|
|
|
|
|
class TestSlackChannel:
|
|
def test_init(self):
|
|
config = SlackConfig(bot_token="xoxb-test", app_token="xapp-test")
|
|
channel = SlackChannel(config)
|
|
assert channel.config is config
|
|
assert channel._running is False
|
|
|
|
async def test_start_raises_without_bot_token(self):
|
|
config = SlackConfig(bot_token="", app_token="xapp-test")
|
|
channel = SlackChannel(config)
|
|
with pytest.raises(ChannelError, match="bot token"):
|
|
await channel.start()
|
|
|
|
async def test_start_raises_without_app_token(self):
|
|
config = SlackConfig(bot_token="xoxb-test", app_token="")
|
|
channel = SlackChannel(config)
|
|
with pytest.raises(ChannelError, match="app token"):
|
|
await channel.start()
|
|
|
|
async def test_stop_when_not_running(self):
|
|
config = SlackConfig(bot_token="xoxb-test", app_token="xapp-test")
|
|
channel = SlackChannel(config)
|
|
await channel.stop()
|
|
|
|
async def test_send_returns_false_without_client(self):
|
|
from EvoScientist.channels.base import OutboundMessage
|
|
|
|
config = SlackConfig(bot_token="xoxb-test", app_token="xapp-test")
|
|
channel = SlackChannel(config)
|
|
msg = OutboundMessage(
|
|
channel="slack",
|
|
chat_id="C123",
|
|
content="hello",
|
|
metadata={"chat_id": "C123"},
|
|
)
|
|
result = await channel.send(msg)
|
|
assert result is False
|
|
|
|
|
|
class TestSlackChannelRegistration:
|
|
def test_slack_registered(self):
|
|
from EvoScientist.channels.channel_manager import available_channels
|
|
|
|
channels = available_channels()
|
|
assert "slack" in channels
|
|
|
|
|
|
@pytest.mark.skipif(
|
|
importlib.util.find_spec("slack_sdk") is None,
|
|
reason="slack-sdk not installed",
|
|
)
|
|
class TestSlackRetryErrorExtraction:
|
|
"""Test Slack-specific status code and SDK error code extraction."""
|
|
|
|
def test_extract_slack_auth_error_not_retryable(self):
|
|
from slack_sdk.errors import SlackApiError
|
|
from slack_sdk.web.slack_response import SlackResponse
|
|
|
|
ch = SlackChannel(SlackConfig(bot_token="xoxb-test", app_token="xapp-test"))
|
|
resp = SlackResponse(
|
|
client=None,
|
|
http_verb="POST",
|
|
api_url="https://slack.com/api/chat.postMessage",
|
|
req_args={},
|
|
data={"ok": False, "error": "invalid_auth"},
|
|
headers={},
|
|
status_code=200,
|
|
)
|
|
exc = SlackApiError("The request to the Slack API failed.", response=resp)
|
|
assert ch._extract_sdk_error_code(exc) == "invalid_auth"
|
|
assert ch._extract_retry_after(exc) is None
|
|
|
|
def test_extract_slack_token_expired_not_retryable(self):
|
|
from slack_sdk.errors import SlackApiError
|
|
from slack_sdk.web.slack_response import SlackResponse
|
|
|
|
ch = SlackChannel(SlackConfig(bot_token="xoxb-test", app_token="xapp-test"))
|
|
resp = SlackResponse(
|
|
client=None,
|
|
http_verb="POST",
|
|
api_url="https://slack.com/api/chat.postMessage",
|
|
req_args={},
|
|
data={"ok": False, "error": "token_expired"},
|
|
headers={},
|
|
status_code=200,
|
|
)
|
|
exc = SlackApiError("The token has expired.", response=resp)
|
|
assert ch._extract_sdk_error_code(exc) == "token_expired"
|
|
assert ch._extract_retry_after(exc) is None
|
|
|
|
def test_extract_slack_status_code_401_not_retryable(self):
|
|
from slack_sdk.errors import SlackApiError
|
|
from slack_sdk.web.slack_response import SlackResponse
|
|
|
|
ch = SlackChannel(SlackConfig(bot_token="xoxb-test", app_token="xapp-test"))
|
|
resp = SlackResponse(
|
|
client=None,
|
|
http_verb="POST",
|
|
api_url="https://slack.com/api/chat.postMessage",
|
|
req_args={},
|
|
data={"ok": False, "error": "unknown_custom"},
|
|
headers={},
|
|
status_code=401,
|
|
)
|
|
exc = SlackApiError("Unauthorized", response=resp)
|
|
assert ch._extract_status_code(exc) == 401
|
|
assert ch._extract_retry_after(exc) is None
|
|
|
|
def test_extract_slack_status_code_500_is_retryable(self):
|
|
from slack_sdk.errors import SlackApiError
|
|
from slack_sdk.web.slack_response import SlackResponse
|
|
|
|
ch = SlackChannel(SlackConfig(bot_token="xoxb-test", app_token="xapp-test"))
|
|
resp = SlackResponse(
|
|
client=None,
|
|
http_verb="POST",
|
|
api_url="https://slack.com/api/chat.postMessage",
|
|
req_args={},
|
|
data={"ok": False, "error": "internal_error"},
|
|
headers={},
|
|
status_code=500,
|
|
)
|
|
exc = SlackApiError("Internal Server Error", response=resp)
|
|
assert ch._extract_status_code(exc) == 500
|
|
assert ch._extract_retry_after(exc) == 1.0
|
|
|
|
def test_slack_channel_fallback_to_httpx(self):
|
|
import httpx
|
|
|
|
ch = SlackChannel(SlackConfig(bot_token="xoxb-test", app_token="xapp-test"))
|
|
exc = httpx.HTTPStatusError(
|
|
"unauthorized",
|
|
request=httpx.Request("POST", "https://example.invalid"),
|
|
response=httpx.Response(401),
|
|
)
|
|
assert ch._extract_status_code(exc) == 401
|
|
assert ch._extract_retry_after(exc) is None
|
|
|
|
def test_slack_ratelimited_uses_retry_after_header(self):
|
|
from slack_sdk.errors import SlackApiError
|
|
from slack_sdk.web.slack_response import SlackResponse
|
|
|
|
ch = SlackChannel(SlackConfig(bot_token="xoxb-test", app_token="xapp-test"))
|
|
resp = SlackResponse(
|
|
client=None,
|
|
http_verb="POST",
|
|
api_url="https://slack.com/api/chat.postMessage",
|
|
req_args={},
|
|
data={"ok": False, "error": "ratelimited"},
|
|
headers={"Retry-After": "30"},
|
|
status_code=429,
|
|
)
|
|
exc = SlackApiError("ratelimited", response=resp)
|
|
assert ch._extract_retry_delay(exc) == 30.0
|
|
assert ch._extract_retry_after(exc) == 30.0
|
|
|
|
def test_slack_malformed_retry_after_falls_through(self):
|
|
from slack_sdk.errors import SlackApiError
|
|
from slack_sdk.web.slack_response import SlackResponse
|
|
|
|
ch = SlackChannel(SlackConfig(bot_token="xoxb-test", app_token="xapp-test"))
|
|
resp = SlackResponse(
|
|
client=None,
|
|
http_verb="POST",
|
|
api_url="https://slack.com/api/chat.postMessage",
|
|
req_args={},
|
|
data={"ok": False, "error": "ratelimited"},
|
|
headers={"Retry-After": "soon"},
|
|
status_code=429,
|
|
)
|
|
exc = SlackApiError("ratelimited", response=resp)
|
|
assert ch._extract_retry_delay(exc) is None
|
|
assert ch._extract_retry_after(exc) == ch._rate_limit_delay
|
|
|
|
|
|
@pytest.mark.skipif(
|
|
importlib.util.find_spec("slack_sdk") is None
|
|
or importlib.util.find_spec("aiohttp") is None,
|
|
reason="slack_sdk or aiohttp not installed",
|
|
)
|
|
class TestSlackRetryWithRawClientResponse:
|
|
"""slack_sdk wraps the raw aiohttp response in SlackApiError when a
|
|
JSON-declared body fails to parse; the retry path must survive that."""
|
|
|
|
async def test_malformed_json_body_is_retried_and_surfaces_sdk_error(
|
|
self, monkeypatch
|
|
):
|
|
import aiohttp
|
|
from aiohttp import web
|
|
from slack_sdk.errors import SlackApiError
|
|
from slack_sdk.web.async_client import AsyncWebClient
|
|
|
|
from EvoScientist.channels.retry import RetryConfig
|
|
|
|
for var in ("HTTP_PROXY", "HTTPS_PROXY", "http_proxy", "https_proxy"):
|
|
monkeypatch.delenv(var, raising=False)
|
|
|
|
calls = 0
|
|
|
|
async def handler(request):
|
|
nonlocal calls
|
|
calls += 1
|
|
return web.Response(
|
|
status=200, text="<<not json>>", content_type="application/json"
|
|
)
|
|
|
|
app = web.Application()
|
|
app.router.add_post("/api/chat.postMessage", handler)
|
|
runner = web.AppRunner(app)
|
|
await runner.setup()
|
|
try:
|
|
await web.TCPSite(runner, "127.0.0.1", 0).start()
|
|
port = runner.addresses[0][1]
|
|
client = AsyncWebClient(
|
|
token="xoxb-test",
|
|
base_url=f"http://127.0.0.1:{port}/api/",
|
|
retry_handlers=[],
|
|
)
|
|
ch = SlackChannel(SlackConfig(bot_token="xoxb-test", app_token="xapp-test"))
|
|
ch._retry_config = RetryConfig(
|
|
attempts=3, min_delay_s=0.01, max_delay_s=0.02, jitter=0.0
|
|
)
|
|
with pytest.raises(SlackApiError) as excinfo:
|
|
await ch._send_with_retry(
|
|
lambda: client.chat_postMessage(channel="C1", text="hi")
|
|
)
|
|
assert isinstance(excinfo.value.response, aiohttp.ClientResponse)
|
|
assert calls == 3
|
|
finally:
|
|
await runner.cleanup()
|