From eb929478065294ed3482de7524b3520365787e8f Mon Sep 17 00:00:00 2001 From: "open-swe[bot]" <215916821+open-swe[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 23:11:42 +0000 Subject: [PATCH] fix: propagate Slack reply errors (#1358) * fix: propagate Slack reply errors Co-authored-by: Johannes du Plessis <51395795+johannes117@users.noreply.github.com> * fix: classify Slack 429 as rate_limited with retry-after Slack chat.postMessage rate limiting returns HTTP 429, which raise_for_status turned into a generic http_error and told the agent to retry immediately, ignoring Slack's retry window. Special-case 429 (threading Retry-After) and normalize the ratelimited body code before the generic HTTP path so the existing rate_limited hint actually fires. --------- Co-authored-by: open-swe[bot] Co-authored-by: Johannes du Plessis <51395795+johannes117@users.noreply.github.com> Co-authored-by: Johannes du Plessis --- agent/prompt.py | 1 + agent/tools/slack_thread_reply.py | 37 ++++++-- agent/utils/slack.py | 33 ++++--- tests/test_slack_assistants_status.py | 100 +++++++++++++++++++++ tests/test_slack_context.py | 8 +- tests/test_slack_thread_reply_tool.py | 121 ++++++++++++++++++++++++++ 6 files changed, 281 insertions(+), 19 deletions(-) create mode 100644 tests/test_slack_thread_reply_tool.py diff --git a/agent/prompt.py b/agent/prompt.py index 66c80be3..da8486b2 100644 --- a/agent/prompt.py +++ b/agent/prompt.py @@ -166,6 +166,7 @@ Posts a comment to a Linear ticket given a `ticket_id`. Call this after opening/ #### `slack_thread_reply` Posts a message to the active Slack thread. Use this for clarifying questions, mid-run progress updates, and final summaries when the task was triggered from Slack. You can call it multiple times during a run — if you're about to do something long-running (cloning a large repo, big refactors, running heavy test suites), post a short status update first so the user knows what's happening. Always end the run with a final reply that summarizes what you did or answers the question. Do not post a status reply before quick, single-tool answers — only when the user would otherwise be left waiting. +If `slack_thread_reply` returns `success: False`, treat it like any other tool failure. Read the `slack_error` and `hint` fields. Never emit a final response message as if the user received it when the Slack post failed. Format messages using Slack's mrkdwn format, NOT standard Markdown. Key differences: *bold*, _italic_, ~strikethrough~, , bullet lists with "• ", ```code blocks```, > blockquotes. diff --git a/agent/tools/slack_thread_reply.py b/agent/tools/slack_thread_reply.py index d79e21f9..54476adc 100644 --- a/agent/tools/slack_thread_reply.py +++ b/agent/tools/slack_thread_reply.py @@ -49,13 +49,40 @@ def slack_thread_reply(message: str) -> dict[str, Any]: return {"success": False, "error": "Message cannot be empty"} message = convert_mentions_to_slack_format(message) - message_ts = asyncio.run(_post_and_store_mapping(channel_id, thread_ts, message)) - return {"success": message_ts is not None} + message_ts, slack_error = asyncio.run(_post_and_store_mapping(channel_id, thread_ts, message)) + if message_ts is None: + return { + "success": False, + "error": slack_error or "post failed", + "slack_error": slack_error, + "message_chars": len(message), + "hint": _slack_reply_failure_hint(slack_error), + } + return {"success": True} -async def _post_and_store_mapping(channel_id: str, thread_ts: str, message: str) -> str | None: - message_ts = await post_slack_thread_reply_with_ts(channel_id, thread_ts, message) +def _slack_reply_failure_hint(slack_error: str | None) -> str: + if slack_error == "msg_too_long": + return "Slack rejected the message as too long; retry with a shorter message." + if slack_error in {"channel_not_found", "not_in_channel"}: + return "Slack rejected the channel; do not retry. Surface the failure to the user via the trace output instead." + if slack_error and slack_error.startswith("rate_limited"): + retry_after = slack_error.partition(":")[2].strip() + if retry_after: + return f"Slack rate limited the request; wait at least {retry_after}s before retrying, or surface the failure to the user via the trace output." + return "Slack rate limited the request; wait before retrying, or surface the failure to the user via the trace output." + if slack_error == "missing_slack_bot_token": + return "Slack bot token is missing; do not retry. Surface the failure to the user via the trace output instead." + if slack_error and slack_error.startswith("http_error:"): + return "Slack posting hit an HTTP error; retry once, then surface the failure to the user via the trace output." + return "Slack post failed; retry once with a concise message or surface the failure to the user via the trace output." + + +async def _post_and_store_mapping( + channel_id: str, thread_ts: str, message: str +) -> tuple[str | None, str | None]: + message_ts, slack_error = await post_slack_thread_reply_with_ts(channel_id, thread_ts, message) if message_ts: langgraph_client = get_client(url=LANGGRAPH_URL) await store_slack_message_run_mapping(langgraph_client, channel_id, thread_ts, message_ts) - return message_ts + return message_ts, slack_error diff --git a/agent/utils/slack.py b/agent/utils/slack.py index 113c0a48..71fb612f 100644 --- a/agent/utils/slack.py +++ b/agent/utils/slack.py @@ -330,10 +330,10 @@ async def post_slack_thread_reply_with_ts( *, unfurl_links: bool = True, unfurl_media: bool = True, -) -> str | None: - """Post a reply in a Slack thread and return its Slack timestamp.""" +) -> tuple[str | None, str | None]: + """Post a reply in a Slack thread and return its Slack timestamp and error.""" if not SLACK_BOT_TOKEN: - return None + return None, "missing_slack_bot_token" payload: dict[str, Any] = { "channel": channel_id, @@ -350,21 +350,33 @@ async def post_slack_thread_reply_with_ts( headers=_slack_headers(), json=payload, ) + if response.status_code == 429: + retry_after = response.headers.get("Retry-After") + logger.warning("Slack chat.postMessage rate limited (retry-after=%s)", retry_after) + if retry_after: + return None, f"rate_limited: {retry_after}" + return None, "rate_limited" response.raise_for_status() data = response.json() if not data.get("ok"): - logger.warning("Slack chat.postMessage failed: %s", data.get("error")) - return None + error = data.get("error") + logger.warning("Slack chat.postMessage failed: %s", error) + if error == "ratelimited": + return None, "rate_limited" + return None, error message_ts = data.get("ts") - return message_ts if isinstance(message_ts, str) and message_ts else None - except httpx.HTTPError: + if isinstance(message_ts, str) and message_ts: + return message_ts, None + return None, None + except httpx.HTTPError as exc: logger.exception("Slack chat.postMessage request failed") - return None + return None, f"http_error: {type(exc).__name__}" async def post_slack_thread_reply(channel_id: str, thread_ts: str, text: str) -> bool: """Post a reply in a Slack thread.""" - return await post_slack_thread_reply_with_ts(channel_id, thread_ts, text) is not None + message_ts, _ = await post_slack_thread_reply_with_ts(channel_id, thread_ts, text) + return message_ts is not None async def post_slack_ephemeral_message( @@ -713,13 +725,14 @@ def _format_trace_reply(trace_url: str | None) -> str: async def post_slack_trace_reply(channel_id: str, thread_ts: str, thread_id: str) -> str | None: """Post a trace URL reply in a Slack thread and return its Slack timestamp.""" trace_url = get_langsmith_trace_url(thread_id) - return await post_slack_thread_reply_with_ts( + message_ts, _ = await post_slack_thread_reply_with_ts( channel_id, thread_ts, _format_trace_reply(trace_url), unfurl_links=False, unfurl_media=False, ) + return message_ts _SLACK_RUN_MAP_NAMESPACE = "slack_run_map" diff --git a/tests/test_slack_assistants_status.py b/tests/test_slack_assistants_status.py index addca55f..ca3c7e2b 100644 --- a/tests/test_slack_assistants_status.py +++ b/tests/test_slack_assistants_status.py @@ -23,6 +23,13 @@ def _err_response(error: str = "channel_not_found") -> MagicMock: return response +def _rate_limited_response(retry_after: str | None = None) -> MagicMock: + response = MagicMock() + response.status_code = 429 + response.headers = {"Retry-After": retry_after} if retry_after else {} + return response + + def _async_client_cm(post_response: MagicMock) -> AsyncMock: client_cm = AsyncMock() client_cm.__aenter__.return_value = client_cm @@ -140,3 +147,96 @@ async def test_post_slack_thread_reply_does_not_call_set_status( assert ok is True assert client_cm.post.await_count == 1 assert client_cm.post.call_args.args[0].endswith("/chat.postMessage") + + +@pytest.mark.asyncio +async def test_post_slack_thread_reply_with_ts_returns_missing_token_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(slack_utils, "SLACK_BOT_TOKEN", "") + + client_cm = _async_client_cm(_ok_response()) + with patch.object(slack_utils.httpx, "AsyncClient", return_value=client_cm): + result = await slack_utils.post_slack_thread_reply_with_ts("C1", "1.0", "hello") + + assert result == (None, "missing_slack_bot_token") + client_cm.post.assert_not_called() + + +@pytest.mark.asyncio +async def test_post_slack_thread_reply_with_ts_returns_slack_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(slack_utils, "SLACK_BOT_TOKEN", "xoxb-test") + + client_cm = _async_client_cm(_err_response("msg_too_long")) + with patch.object(slack_utils.httpx, "AsyncClient", return_value=client_cm): + result = await slack_utils.post_slack_thread_reply_with_ts("C1", "1.0", "hello") + + assert result == (None, "msg_too_long") + + +@pytest.mark.asyncio +async def test_post_slack_thread_reply_with_ts_returns_rate_limited_with_retry_after( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(slack_utils, "SLACK_BOT_TOKEN", "xoxb-test") + + client_cm = _async_client_cm(_rate_limited_response(retry_after="30")) + with patch.object(slack_utils.httpx, "AsyncClient", return_value=client_cm): + result = await slack_utils.post_slack_thread_reply_with_ts("C1", "1.0", "hello") + + assert result == (None, "rate_limited: 30") + + +@pytest.mark.asyncio +async def test_post_slack_thread_reply_with_ts_returns_rate_limited_without_retry_after( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(slack_utils, "SLACK_BOT_TOKEN", "xoxb-test") + + client_cm = _async_client_cm(_rate_limited_response()) + with patch.object(slack_utils.httpx, "AsyncClient", return_value=client_cm): + result = await slack_utils.post_slack_thread_reply_with_ts("C1", "1.0", "hello") + + assert result == (None, "rate_limited") + + +@pytest.mark.asyncio +async def test_post_slack_thread_reply_with_ts_normalizes_ratelimited_body_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(slack_utils, "SLACK_BOT_TOKEN", "xoxb-test") + + client_cm = _async_client_cm(_err_response("ratelimited")) + with patch.object(slack_utils.httpx, "AsyncClient", return_value=client_cm): + result = await slack_utils.post_slack_thread_reply_with_ts("C1", "1.0", "hello") + + assert result == (None, "rate_limited") + + +@pytest.mark.asyncio +async def test_post_slack_thread_reply_with_ts_returns_http_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(slack_utils, "SLACK_BOT_TOKEN", "xoxb-test") + + client_cm = _async_client_cm(_ok_response()) + client_cm.post = AsyncMock(side_effect=slack_utils.httpx.ConnectError("boom")) + with patch.object(slack_utils.httpx, "AsyncClient", return_value=client_cm): + result = await slack_utils.post_slack_thread_reply_with_ts("C1", "1.0", "hello") + + assert result == (None, "http_error: ConnectError") + + +@pytest.mark.asyncio +async def test_post_slack_thread_reply_preserves_bool_return_on_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(slack_utils, "SLACK_BOT_TOKEN", "xoxb-test") + + client_cm = _async_client_cm(_err_response("channel_not_found")) + with patch.object(slack_utils.httpx, "AsyncClient", return_value=client_cm): + ok = await slack_utils.post_slack_thread_reply("C1", "1.0", "hello") + + assert ok is False diff --git a/tests/test_slack_context.py b/tests/test_slack_context.py index a077cc16..2c10d1c3 100644 --- a/tests/test_slack_context.py +++ b/tests/test_slack_context.py @@ -233,9 +233,9 @@ def test_post_slack_trace_reply_emits_tip_only_when_no_trace_url( *, unfurl_links: bool = True, unfurl_media: bool = True, - ) -> str | None: + ) -> tuple[str | None, str | None]: posted.append({"text": text, "unfurl_links": unfurl_links, "unfurl_media": unfurl_media}) - return "1.1" + return "1.1", None monkeypatch.setattr( slack_utils, "post_slack_thread_reply_with_ts", fake_post_slack_thread_reply_with_ts @@ -264,9 +264,9 @@ def test_post_slack_trace_reply_includes_trace_link_and_tip( *, unfurl_links: bool = True, unfurl_media: bool = True, - ) -> str | None: + ) -> tuple[str | None, str | None]: posted.append({"text": text, "unfurl_links": unfurl_links, "unfurl_media": unfurl_media}) - return "1.1" + return "1.1", None monkeypatch.setattr( slack_utils, "post_slack_thread_reply_with_ts", fake_post_slack_thread_reply_with_ts diff --git a/tests/test_slack_thread_reply_tool.py b/tests/test_slack_thread_reply_tool.py new file mode 100644 index 00000000..2e78462d --- /dev/null +++ b/tests/test_slack_thread_reply_tool.py @@ -0,0 +1,121 @@ +from __future__ import annotations + +import importlib +from typing import Any + +import pytest + +slack_reply_tool = importlib.import_module("agent.tools.slack_thread_reply") + + +def _config() -> dict[str, Any]: + return { + "configurable": { + "slack_thread": { + "channel_id": "C1", + "thread_ts": "1.0", + } + } + } + + +def test_slack_thread_reply_returns_structured_error_for_msg_too_long( + monkeypatch: pytest.MonkeyPatch, +) -> None: + async def fake_post_and_store_mapping( + channel_id: str, thread_ts: str, message: str + ) -> tuple[str | None, str | None]: + return None, "msg_too_long" + + monkeypatch.setattr(slack_reply_tool, "get_config", _config) + monkeypatch.setattr(slack_reply_tool, "_post_and_store_mapping", fake_post_and_store_mapping) + + result = slack_reply_tool.slack_thread_reply("hello") + + assert result == { + "success": False, + "error": "msg_too_long", + "slack_error": "msg_too_long", + "message_chars": 5, + "hint": "Slack rejected the message as too long; retry with a shorter message.", + } + + +@pytest.mark.parametrize("slack_error", ["channel_not_found", "not_in_channel"]) +def test_slack_thread_reply_hints_not_to_retry_channel_errors( + slack_error: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + async def fake_post_and_store_mapping( + channel_id: str, thread_ts: str, message: str + ) -> tuple[str | None, str | None]: + return None, slack_error + + monkeypatch.setattr(slack_reply_tool, "get_config", _config) + monkeypatch.setattr(slack_reply_tool, "_post_and_store_mapping", fake_post_and_store_mapping) + + result = slack_reply_tool.slack_thread_reply("hello") + + assert result["success"] is False + assert result["error"] == slack_error + assert result["slack_error"] == slack_error + assert result["message_chars"] == 5 + assert "do not retry" in result["hint"] + assert "trace output" in result["hint"] + + +def test_slack_thread_reply_rate_limited_hint_includes_retry_after( + monkeypatch: pytest.MonkeyPatch, +) -> None: + async def fake_post_and_store_mapping( + channel_id: str, thread_ts: str, message: str + ) -> tuple[str | None, str | None]: + return None, "rate_limited: 30" + + monkeypatch.setattr(slack_reply_tool, "get_config", _config) + monkeypatch.setattr(slack_reply_tool, "_post_and_store_mapping", fake_post_and_store_mapping) + + result = slack_reply_tool.slack_thread_reply("hello") + + assert result["success"] is False + assert result["error"] == "rate_limited: 30" + assert result["slack_error"] == "rate_limited: 30" + assert "30s" in result["hint"] + assert "wait" in result["hint"] + + +def test_slack_thread_reply_rate_limited_hint_without_retry_after( + monkeypatch: pytest.MonkeyPatch, +) -> None: + async def fake_post_and_store_mapping( + channel_id: str, thread_ts: str, message: str + ) -> tuple[str | None, str | None]: + return None, "rate_limited" + + monkeypatch.setattr(slack_reply_tool, "get_config", _config) + monkeypatch.setattr(slack_reply_tool, "_post_and_store_mapping", fake_post_and_store_mapping) + + result = slack_reply_tool.slack_thread_reply("hello") + + assert result["success"] is False + assert result["slack_error"] == "rate_limited" + assert "wait" in result["hint"] + + +def test_slack_thread_reply_uses_post_failed_without_slack_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + async def fake_post_and_store_mapping( + channel_id: str, thread_ts: str, message: str + ) -> tuple[str | None, str | None]: + return None, None + + monkeypatch.setattr(slack_reply_tool, "get_config", _config) + monkeypatch.setattr(slack_reply_tool, "_post_and_store_mapping", fake_post_and_store_mapping) + + result = slack_reply_tool.slack_thread_reply("hello") + + assert result["success"] is False + assert result["error"] == "post failed" + assert result["slack_error"] is None + assert result["message_chars"] == 5