From c61cba3011b574f360b81b6762e019e7f96ba763 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Tue, 20 Oct 2026 15:56:51 +0300 Subject: [PATCH 1/2] fix(channels): a failed turn tells the chat so in plain words, never the operator's error (#3603) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The router posted the execution's own error into the channel: "Authentication failure: Not logged in · Please run /login. Check subscription token or API key configuration." In a Telegram group every member saw it; Slack, WhatsApp and DMs took the same path. A failed turn now sends the fixed line the router's exception branch already used; the error stays on the execution row and in the router's log line. A cancelled turn still sends its cancel reason (#679), or a cancelled line when it has none. Public chat got the same fix in #3478. test_slack_timeout_fix.py's TestErrorMessageRouting re-implemented the old #221 forwarding logic instead of calling the router, so it could not fail; it is replaced by tests that drive the real _run_agent_task. Mutation: restoring the forwarding turns 14 of 19 new tests red. Co-Authored-By: Claude Opus 5.5 --- src/backend/adapters/message_router.py | 21 ++- tests/registry.json | 11 ++ tests/unit/test_3603_channel_failure_reply.py | 140 ++++++++++++++++++ tests/unit/test_slack_timeout_fix.py | 62 +------- 4 files changed, 173 insertions(+), 61 deletions(-) create mode 100644 tests/unit/test_3603_channel_failure_reply.py diff --git a/src/backend/adapters/message_router.py b/src/backend/adapters/message_router.py index eddc44f352..a593b56c30 100644 --- a/src/backend/adapters/message_router.py +++ b/src/backend/adapters/message_router.py @@ -340,6 +340,12 @@ def _check_rate_limit(key: str, max_msgs: Optional[int] = None, window: Optional return True +# #3603: what a channel member is told when a turn does not answer. A failed +# turn's error is for the operator and never reaches the chat. +CHANNEL_FAILURE_MESSAGE = "Sorry, I encountered an error processing your message. Please try again." +CHANNEL_CANCELLED_MESSAGE = "That request was cancelled before it was answered." + + class ChannelMessageRouter: """Channel-agnostic message dispatcher.""" @@ -851,11 +857,16 @@ async def _run_agent_task( logger.info(f"[ROUTER:{channel}] Step 9 - task {result.status}: {error_msg}") await self._resolve_indicator(adapter, message, success=False) - # Reply with the actual error if available, otherwise generic message - if error_msg and error_msg != "Unknown error": - response_text = error_msg + # #3603: the error is the operator's diagnosis (a missing model + # key, an exit code, where to fix a credential) and a channel is + # read by its members — every member, in a group. They get a + # plain line; the detail stays on the execution row and in the + # log line above (the public-chat twin is #3478). A cancel + # reason is written for the person who asked (#679) and passes. + if result.status == "cancelled": + response_text = result.error or CHANNEL_CANCELLED_MESSAGE else: - response_text = "Sorry, I encountered an error processing your message." + response_text = CHANNEL_FAILURE_MESSAGE await adapter.send_response( message.channel_id, @@ -890,7 +901,7 @@ async def _run_agent_task( await adapter.send_response( message.channel_id, ChannelResponse( - text="Sorry, I encountered an error processing your message. Please try again.", + text=CHANNEL_FAILURE_MESSAGE, metadata={"bot_token": bot_token, "agent_name": agent_name} ), thread_id=message.thread_id, diff --git a/tests/registry.json b/tests/registry.json index 503826a862..54867950db 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -6169,6 +6169,17 @@ "agent-lifecycle" ], "description": "recreate_container_with_updated_config re-bakes AGENT_NAME and the trinity.agent-name label from the live name beside TRINITY_AGENT_AUTH_TOKEN (driven through the real recreate, red before the fix on the replayed old name); check_agent_name_env_matches flags a present-and-different env or label, matches absent values and unreadable attrs; start_agent_internal recreates a stuck renamed agent on COLD start only and leaves never-renamed and running agents alone." + }, + { + "file": "unit/test_3603_channel_failure_reply.py", + "feature": "#3603", + "added": "2026-10-20", + "categories": [ + "backend", + "channels", + "unit" + ], + "description": "A failed channel turn sends the fixed member-facing line, never the execution's error (auth failure, exit code, timeout, capacity, rate limit), in a group and one-to-one, through the real ChannelMessageRouter._run_agent_task; the error stays in the router's log line; a cancelled turn still sends its cancel reason, or a cancelled line when it has none (#679); a raised dispatch sends the same line; a successful turn is untouched. Mutation: restoring the error forwarding turns 14 red." } ] } diff --git a/tests/unit/test_3603_channel_failure_reply.py b/tests/unit/test_3603_channel_failure_reply.py new file mode 100644 index 0000000000..5e9a475eb2 --- /dev/null +++ b/tests/unit/test_3603_channel_failure_reply.py @@ -0,0 +1,140 @@ +"""#3603 — a failed channel turn tells the chat so in plain words, never the +operator's error. + +A channel (Telegram, Slack, WhatsApp) is read by its members — every member in +a group. The router used to post the execution's own `error` into the chat: +"Authentication failure: Not logged in · Please run /login. Check subscription +token or API key configuration." Now a failed turn sends a fixed member-facing +line, and the error stays on the execution row and in the router's log line for +the operator. A cancelled turn still says it was cancelled (#679). The +public-chat twin is #3478. + +Driven through `ChannelMessageRouter._run_agent_task` with the dispatch stubbed +where its lazy import finds it (the `test_3274_channel_quote` harness). +""" +import logging +import sys +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock + +import pytest + +_BACKEND = Path(__file__).resolve().parents[2] / "src" / "backend" +if str(_BACKEND) not in sys.path: + sys.path.insert(0, str(_BACKEND)) + +pytestmark = pytest.mark.unit + +OPERATOR_ERRORS = [ + "Authentication failure: Not logged in · Please run /login. Check subscription " + "token or API key configuration.", + "Task execution failed (exit code 1): No authentication configured. Set " + "ANTHROPIC_API_KEY or assign a subscription token.", + "Task execution timed out after 900 seconds", + "Agent at capacity (3/3 parallel tasks running)", + "HTTP error: rate_limit_exceeded", +] + + +def _harness(monkeypatch, result): + import adapters.message_router as mr + import services.task_execution_service as tes + + monkeypatch.setattr(tes, "dispatch_and_await_terminal", AsyncMock(return_value=result)) + monkeypatch.setattr(mr, "get_task_execution_service", lambda: MagicMock()) + monkeypatch.setattr(mr, "build_public_channel_caller_prompt", lambda *a, **kw: None) + monkeypatch.setattr(mr, "build_voice_capability_prompt", lambda *a, **kw: None) + monkeypatch.setattr(mr, "_get_channel_allowed_tools", lambda: ["WebSearch"]) + monkeypatch.setattr(mr.db, "get_public_channel_model", lambda agent: None) + channel = MagicMock() + channel.get_source_identifier.return_value = "telegram:9001:424242" + channel.send_response = AsyncMock() + channel.indicate_done = AsyncMock() + router = mr.ChannelMessageRouter.__new__(mr.ChannelMessageRouter) + return mr, channel, router + + +def _message(): + return SimpleNamespace(text="@bot what did we just discuss?", channel_id="-100", thread_id=None, + files=[], sender_id="5", + metadata={"raw_message": {"from": {"id": 5, "first_name": "Ada"}}, + "bot_id": "77", "chat_title": "Ops", "username": "ada"}) + + +async def _run(router, channel, *, is_group=True): + return await router._run_agent_task( + channel, _message(), "finance", "bot-token", "telegram", is_group, + container=None, upload_dir=None, context_prompt="[prompt]", + verified_email=None, image_data=[]) + + +def _sent(channel): + channel.send_response.assert_awaited_once() + return channel.send_response.await_args.args[1].text + + +@pytest.mark.asyncio +@pytest.mark.parametrize("is_group", [True, False], ids=["group", "one-to-one"]) +@pytest.mark.parametrize("error", OPERATOR_ERRORS) +async def test_a_failed_turn_sends_the_plain_line_not_the_error(monkeypatch, error, is_group): + mr, channel, router = _harness(monkeypatch, SimpleNamespace(status="failed", error=error, response="")) + assert await _run(router, channel, is_group=is_group) is None + text = _sent(channel) + assert text == mr.CHANNEL_FAILURE_MESSAGE + assert error not in text + + +@pytest.mark.asyncio +@pytest.mark.parametrize("error", [None, "", "Unknown error"]) +async def test_a_failed_turn_with_no_error_sends_the_same_line(monkeypatch, error): + mr, channel, router = _harness(monkeypatch, SimpleNamespace(status="failed", error=error, response="")) + await _run(router, channel) + assert _sent(channel) == mr.CHANNEL_FAILURE_MESSAGE + + +@pytest.mark.asyncio +async def test_the_error_stays_with_the_operator(monkeypatch, caplog): + error = OPERATOR_ERRORS[0] + _, channel, router = _harness(monkeypatch, SimpleNamespace(status="failed", error=error, response="")) + with caplog.at_level(logging.INFO, logger="adapters.message_router"): + await _run(router, channel) + assert any(error in r.getMessage() for r in caplog.records) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("reason", ["Execution cancelled by user", "Cancelled by user while queued"]) +async def test_a_cancelled_turn_still_says_it_was_cancelled(monkeypatch, reason): + """#679: the cancel reason is written for the person who asked.""" + _, channel, router = _harness(monkeypatch, SimpleNamespace(status="cancelled", error=reason, response="")) + await _run(router, channel) + assert _sent(channel) == reason + + +@pytest.mark.asyncio +async def test_a_cancelled_turn_with_no_reason_still_says_cancelled(monkeypatch): + mr, channel, router = _harness(monkeypatch, SimpleNamespace(status="cancelled", error=None, response="")) + await _run(router, channel) + assert _sent(channel) == mr.CHANNEL_CANCELLED_MESSAGE + assert "cancel" in mr.CHANNEL_CANCELLED_MESSAGE.lower() + + +@pytest.mark.asyncio +async def test_a_dispatch_that_raises_sends_the_same_line(monkeypatch): + """The exception branch and the failed-row branch say the same thing.""" + import adapters.message_router as mr + import services.task_execution_service as tes + + _, channel, router = _harness(monkeypatch, None) + monkeypatch.setattr(tes, "dispatch_and_await_terminal", + AsyncMock(side_effect=RuntimeError("upstream exploded: token=abc"))) + await _run(router, channel) + assert _sent(channel) == mr.CHANNEL_FAILURE_MESSAGE + + +@pytest.mark.asyncio +async def test_a_successful_turn_is_untouched(monkeypatch): + result = SimpleNamespace(status="success", error=None, response="Here is the summary.", cost=0.0) + _, channel, router = _harness(monkeypatch, result) + assert await _run(router, channel) is result + channel.send_response.assert_not_awaited() diff --git a/tests/unit/test_slack_timeout_fix.py b/tests/unit/test_slack_timeout_fix.py index ecb57cd7d4..000ce95763 100644 --- a/tests/unit/test_slack_timeout_fix.py +++ b/tests/unit/test_slack_timeout_fix.py @@ -3,8 +3,7 @@ Verifies: - timeout_seconds=None is passed to TaskExecutionService (uses agent timeout) -- Error messages are forwarded to Slack users as-is -- Generic fallback only for empty/unknown errors +- (error replies: see test_3603_channel_failure_reply.py) Module: src/backend/adapters/message_router.py Issue: https://github.com/abilityai/trinity/issues/221 @@ -13,60 +12,11 @@ import pytest -class TestErrorMessageRouting: - """Test the error message logic extracted from message_router.py. - - The router does: - if error_msg and error_msg != "Unknown error": - response_text = error_msg - else: - response_text = "Sorry, I encountered an error processing your message." - """ - - @staticmethod - def get_response_text(error_msg): - """Reproduce the router's error message logic.""" - if error_msg and error_msg != "Unknown error": - return error_msg - else: - return "Sorry, I encountered an error processing your message." - - def test_timeout_error_forwarded(self): - """Timeout error from TaskExecutionService is sent to Slack user.""" - msg = "Task execution timed out after 60 seconds" - assert self.get_response_text(msg) == msg - - def test_timeout_900s_forwarded(self): - """Agent-configured timeout (900s) error is forwarded.""" - msg = "Task execution timed out after 900 seconds" - assert self.get_response_text(msg) == msg - - def test_capacity_error_forwarded(self): - """Capacity error is sent to Slack user.""" - msg = "Agent at capacity (3/3 parallel tasks running)" - assert self.get_response_text(msg) == msg - - def test_auth_error_forwarded(self): - """Auth error from agent is sent to Slack user.""" - msg = "Task execution failed (exit code 1): No authentication configured. Set ANTHROPIC_API_KEY or assign a subscription token." - assert self.get_response_text(msg) == msg - - def test_billing_error_forwarded(self): - """Billing/rate-limit error is sent to Slack user.""" - msg = "HTTP error: rate_limit_exceeded" - assert self.get_response_text(msg) == msg - - def test_unknown_error_gets_generic(self): - """'Unknown error' gets the generic fallback message.""" - assert self.get_response_text("Unknown error") == "Sorry, I encountered an error processing your message." - - def test_none_error_gets_generic(self): - """None error gets the generic fallback message.""" - assert self.get_response_text(None) == "Sorry, I encountered an error processing your message." - - def test_empty_error_gets_generic(self): - """Empty string error gets the generic fallback message.""" - assert self.get_response_text("") == "Sorry, I encountered an error processing your message." +# #3603 retired #221's error forwarding: a channel member is no longer sent the +# execution's own error (it is the operator's diagnosis, and in a group every +# member reads it). The behaviour is executed against the real router in +# test_3603_channel_failure_reply.py; the copy of the old logic that stood here +# could not fail. class TestTimeoutParameter: From d99fa6cb555abd0259759e9d16c5f6157af4f65b Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Thu, 22 Oct 2026 12:28:11 +0300 Subject: [PATCH 2/2] fix(channels): a cancelled turn always sends the fixed cancelled line (#3603) On a pull-pilot agent a terminated turn keeps the failure body's content and the pull sink stores it as the cancelled row's error, so forwarding a cancelled row's error could still post the operator's error to the chat. The cancelled branch now always sends CHANNEL_CANCELLED_MESSAGE. Co-Authored-By: Claude Opus 5.5 --- src/backend/adapters/message_router.py | 11 ++++--- tests/registry.json | 2 +- tests/unit/test_3603_channel_failure_reply.py | 29 +++++++++++++------ 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/backend/adapters/message_router.py b/src/backend/adapters/message_router.py index a593b56c30..2ea62e5359 100644 --- a/src/backend/adapters/message_router.py +++ b/src/backend/adapters/message_router.py @@ -343,7 +343,7 @@ def _check_rate_limit(key: str, max_msgs: Optional[int] = None, window: Optional # #3603: what a channel member is told when a turn does not answer. A failed # turn's error is for the operator and never reaches the chat. CHANNEL_FAILURE_MESSAGE = "Sorry, I encountered an error processing your message. Please try again." -CHANNEL_CANCELLED_MESSAGE = "That request was cancelled before it was answered." +CHANNEL_CANCELLED_MESSAGE = "That request was cancelled before it was answered." # always, never the row's error class ChannelMessageRouter: @@ -861,10 +861,13 @@ async def _run_agent_task( # key, an exit code, where to fix a credential) and a channel is # read by its members — every member, in a group. They get a # plain line; the detail stays on the execution row and in the - # log line above (the public-chat twin is #3478). A cancel - # reason is written for the person who asked (#679) and passes. + # log line above (the public-chat twin is #3478). A cancelled + # turn still says so (#679), in the fixed line: on a pull-pilot + # agent a terminated turn keeps the failure body's content and + # the pull sink stores it as the cancelled row's error, so that + # error can be the operator's too. if result.status == "cancelled": - response_text = result.error or CHANNEL_CANCELLED_MESSAGE + response_text = CHANNEL_CANCELLED_MESSAGE else: response_text = CHANNEL_FAILURE_MESSAGE diff --git a/tests/registry.json b/tests/registry.json index 760dd37590..80342d6880 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -6191,7 +6191,7 @@ "channels", "unit" ], - "description": "A failed channel turn sends the fixed member-facing line, never the execution's error (auth failure, exit code, timeout, capacity, rate limit), in a group and one-to-one, through the real ChannelMessageRouter._run_agent_task; the error stays in the router's log line; a cancelled turn still sends its cancel reason, or a cancelled line when it has none (#679); a raised dispatch sends the same line; a successful turn is untouched. Mutation: restoring the error forwarding turns 14 red." + "description": "A failed channel turn sends the fixed member-facing line, never the execution's error (auth failure, exit code, timeout, capacity, rate limit), in a group and one-to-one, through the real ChannelMessageRouter._run_agent_task; the error stays in the router's log line; a cancelled turn always sends the fixed cancelled line (#679), never the row's error, including a pull-pilot terminated turn whose cancelled row carries the operator's error; a raised dispatch sends the same line; a successful turn is untouched. Mutation: restoring the error forwarding turns 14 red; restoring the cancel-reason forwarding turns 8 red." }, { "file": "unit/test_ent858_event_stream_owner_state.py", diff --git a/tests/unit/test_3603_channel_failure_reply.py b/tests/unit/test_3603_channel_failure_reply.py index 5e9a475eb2..d9db44f99f 100644 --- a/tests/unit/test_3603_channel_failure_reply.py +++ b/tests/unit/test_3603_channel_failure_reply.py @@ -6,7 +6,8 @@ "Authentication failure: Not logged in · Please run /login. Check subscription token or API key configuration." Now a failed turn sends a fixed member-facing line, and the error stays on the execution row and in the router's log line for -the operator. A cancelled turn still says it was cancelled (#679). The +the operator. A cancelled turn still says it was cancelled (#679), in a fixed +line: on pull-pilot agents a cancelled row's error can be the operator's error. The public-chat twin is #3478. Driven through `ChannelMessageRouter._run_agent_task` with the dispatch stubbed @@ -103,17 +104,27 @@ async def test_the_error_stays_with_the_operator(monkeypatch, caplog): @pytest.mark.asyncio -@pytest.mark.parametrize("reason", ["Execution cancelled by user", "Cancelled by user while queued"]) -async def test_a_cancelled_turn_still_says_it_was_cancelled(monkeypatch, reason): - """#679: the cancel reason is written for the person who asked.""" - _, channel, router = _harness(monkeypatch, SimpleNamespace(status="cancelled", error=reason, response="")) - await _run(router, channel) - assert _sent(channel) == reason +@pytest.mark.parametrize("error", OPERATOR_ERRORS) +async def test_a_cancelled_pull_pilot_turn_never_sends_the_operator_error(monkeypatch, error): + """A pull-pilot turn terminated mid-flight keeps the failure body's content + (`pull_worker.py` relabels it `cancelled` but leaves `content`), and the + pull sink stores that content as the cancelled row's `error`. So a + cancelled row's error can be the operator's error; the chat must get the + fixed cancelled line instead.""" + mr, channel, router = _harness(monkeypatch, SimpleNamespace(status="cancelled", error=error, response="")) + assert await _run(router, channel) is None + text = _sent(channel) + assert text == mr.CHANNEL_CANCELLED_MESSAGE + assert error not in text @pytest.mark.asyncio -async def test_a_cancelled_turn_with_no_reason_still_says_cancelled(monkeypatch): - mr, channel, router = _harness(monkeypatch, SimpleNamespace(status="cancelled", error=None, response="")) +@pytest.mark.parametrize("error", [None, "", "Execution cancelled by user", "Cancelled by user while queued", + "[agent_error] caller went away"]) +async def test_a_cancelled_turn_always_says_cancelled_in_the_fixed_line(monkeypatch, error): + """#679: a cancelled turn still says it was cancelled, in one fixed line, + whatever the row's error holds.""" + mr, channel, router = _harness(monkeypatch, SimpleNamespace(status="cancelled", error=error, response="")) await _run(router, channel) assert _sent(channel) == mr.CHANNEL_CANCELLED_MESSAGE assert "cancel" in mr.CHANNEL_CANCELLED_MESSAGE.lower()