Skip to content

fix(channels): a failed turn tells the chat so in plain words, never the operator's error (#3603) - #3839

Merged
vybe merged 5 commits into
devfrom
fix/3603-channel-error-reply
Oct 23, 2026
Merged

vybe merged 5 commits into
devfrom
fix/3603-channel-error-reply

Conversation

@dolho

@dolho dolho commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The problem: a failed channel turn posted the execution's own error into the chat, for example "Authentication failure: Not logged in · Please run /login. Check subscription token or API key configuration." In a Telegram group every member saw it, and Slack, WhatsApp and DMs went through the same code.
  • Failed turns: the chat now gets the router's existing generic line, "Sorry, I encountered an error processing your message. Please try again." The exception branch already used it, so no new wording is added. The real error stays on the execution row and in the router's log line for the operator.
  • Cancelled turns: the cancel reason is still sent, as feat(executions): plumb cancel signal into agent task-runner reply (#671 defense-in-depth) #679 requires, for example "Execution cancelled by user". A cancel with no reason sends "That request was cancelled before it was answered."
  • Precedent: public chat got the same fix in fix(public-chat): a failed turn shows visitors a plain message, never the raw execution error (#3461) #3478.

Changes

  • src/backend/adapters/message_router.py: two constants, CHANNEL_FAILURE_MESSAGE and CHANNEL_CANCELLED_MESSAGE, used by the failed/cancelled branch and the exception branch.
  • tests/unit/test_3603_channel_failure_reply.py (new, 19 tests).
  • tests/unit/test_slack_timeout_fix.py: removed TestErrorMessageRouting. It copied fix: Slack message router ignores per-agent execution timeout (TIMEOUT-001) #221's old forwarding logic into the test instead of calling the router, so it could never fail; a comment points to the new tests.

Test Plan

  • cd tests && pytest unit/test_3603_channel_failure_reply.py: 19 pass, through the real ChannelMessageRouter._run_agent_task with only the dispatch stubbed (the test_3274_channel_quote setup).
    • In a group and one-to-one, a failed turn sends the plain line and never the error text. Covered errors: auth failure, exit code, timeout, capacity, rate limit, and an empty or unknown error.
    • The error does appear in the router's log line.
    • A cancelled turn sends its cancel reason, or the cancelled line when there is none.
    • A dispatch that raises an exception sends the same plain line.
    • A successful turn is unchanged.
  • Mutation: putting the old error forwarding back turns 14 of the 19 new tests red.
  • Related router tests pass: 3274, 679, 3746, 903, 350, slack timeout, ent751.
  • 3 TestPaidA2A tests in test_ent751_gate_callers.py fail locally the same way on a clean dev checkout, so they're unrelated.

Fixes #3603

🤖 Generated with Claude Code

dolho and others added 2 commits October 20, 2026 15:56
…the operator's error (#3603)

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 <noreply@anthropic.com>
Resolves the tests/registry.json conflict by keeping both sides' entries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dolho
dolho requested a review from obasilakis October 22, 2026 09:04

@obasilakis obasilakis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slack, Telegram and WhatsApp all reply through this one branch, and the remaining str(e) uses are admin routes. Tests run the real _run_agent_task: 19 pass on the branch, 15 fail with dev's router.

Major

  • src/backend/adapters/message_router.py:867 — a cancelled turn still sends result.error verbatim. On pull-pilot agents a terminated turn keeps the failure body's content (docker/base-image/agent_server/services/pull_worker.py:452-461, content set at :270/:284), and the backend stores it as the cancelled row's error (pull_coordination_service.py:715). The chat then gets the operator's error, which the first AC forbids. Always send CHANNEL_CANCELLED_MESSAGE for a cancelled turn; it still says the turn was cancelled, as #679 requires.

Traced in code; not reproduced live. If the pull worker already sends a fixed text for terminated turns, this drops to minor.

dolho and others added 2 commits October 22, 2026 12:28
…#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 <noreply@anthropic.com>
@dolho

dolho commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

@obasilakis thanks, confirmed in code: pull_worker.py relabels a terminated turn cancelled but keeps the failure body's content, and the pull sink stores it as the cancelled row's error.

Fixed in d99fa6c: a cancelled turn now always sends CHANNEL_CANCELLED_MESSAGE ("That request was cancelled before it was answered."), never result.error. It still says the turn was cancelled, so #679 holds.

Tests (tests/unit/test_3603_channel_failure_reply.py, now 26)

  • New: a cancelled turn whose error is each operator error (auth failure, exit code, timeout, capacity, rate limit) sends the fixed cancelled line and never the error.
  • Changed: the cancel-reason test now expects the fixed line for every error (None, empty, "Execution cancelled by user", "Cancelled by user while queued", a [code] prefix).
  • Mutation: putting back result.error or CHANNEL_CANCELLED_MESSAGE turns 8 red. With the fix, 26 pass.
  • Router tests 3274, 679 callers, slack timeout, 903, 350 and 3746 pass. The 3 TestPaidA2A failures in ent751 are the same ones that fail on dev.

Other paths checked. For inline turns, _run_agent_task is the only place a turn's error reaches a channel. Slack, Telegram and WhatsApp all reply through it. The push vs pull and sync vs queued waits all end in dispatch_and_await_terminal. A failed row's response is rebuilt as "", so the success branch can't carry an error either. The other send_response calls send fixed text or skill-gate copy.

One path is outside this PR: services/channel_completion_report.py. When a delegated or background task fails, the Slack/Telegram completion report (and the portal one) puts the credential-scrubbed error under "Task ended with status failed". It's the same kind of leak, but that code shows failure detail on purpose (ent#224/#265/ent#457), so I left it for a follow-up.

Also merged origin/dev (one conflict in tests/registry.json, kept both entries).

@obasilakis obasilakis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The cancelled-turn branch in adapters/message_router.py now always sends CHANNEL_CANCELLED_MESSAGE, and the cancel tests feed operator error text into a cancelled turn and assert the fixed line goes out. The only other reply text in the router is the skill-gate message, which is user-facing. No new findings.

@obasilakis obasilakis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at 7ed36cc. The only commit since my approval merges origin/dev; the three PR code/test files are byte-identical to 3ac16e4, and the only patch change is the tests/registry.json hunk moving. The ent#934 Telegram album code that came in from dev adds no reply path that sends result.error to a chat (it only logs), so the router's failed/cancelled branch is still the single place a turn error reaches a chat. CI green. Approval stands.

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge-train: batch validated on train/20261023-2211 (#3930, all checks green)

@vybe
vybe merged commit 6c55f40 into dev Oct 23, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants