Skip to content

[Fix] Stop closeout retry loops when chat delivery permanently fails - #1095

Merged
daniel-lxs merged 2 commits into
developfrom
fix/slack-delivery-failure-closeout-escape
Aug 4, 2026
Merged

[Fix] Stop closeout retry loops when chat delivery permanently fails#1095
daniel-lxs merged 2 commits into
developfrom
fix/slack-delivery-failure-closeout-escape

Conversation

@daniel-lxs

@daniel-lxs daniel-lxs commented Aug 4, 2026

Copy link
Copy Markdown
Member

Problem

An automation-started task whose Slack channel rejects every chat.postMessage gets wedged in a closeout retry loop:

  1. The task finishes its work and goes idle without a delivered closeout.
  2. The stop hook blocks the idle (automation_missing_terminal_closeout) and reminds the agent to post with send_chat_reply purpose closeout.
  3. The post fails, but the real Slack error is swallowed twice: SlackNotifier.sendMessage logs it server-side and returns nothing, and the thread_reply MCP handler collapses it into a generic 502 Slack chat.postMessage returned no message timestamp.
  4. A 502 reads as transient, so the agent retries; the hook re-blocks on the next idle; repeat.

In a real occurrence the agent made 15 failed post attempts over ~6 minutes before the harness reminder cap fired, and the run then completed silently with the deliverable never posted anywhere. The closeout requirement had no escape path: satisfaction is only recordable from a successful post, which was impossible.

Fix

Propagate the real Slack error (API side)

  • SlackNotifier.postMessageDetailed returns a structured outcome: ts on success, otherwise the Slack error code, a transport-error flag, or the skipped-missing-thread-root marker. postMessage keeps its existing signature and delegates.
  • The thread_reply MCP handler throws a typed SlackPostDeliveryError and maps it to a structured response: { error, slackErrorCode, retryable }, status 422 for permanent errors (not_in_channel, channel_not_found, is_archived, invalid_auth, token_revoked, ...) and 502 for transient ones. Content-dependent errors (msg_too_long, invalid_blocks) stay retryable since a rewritten message can succeed.
  • The previously conflated "no ts" case in the existing-thread path now distinguishes a deleted thread root (still 409) from a real delivery failure.

Escape hatch (worker side)

  • The worker's chat API client turns a retryable: false response into a typed ChatDeliveryError carrying the provider error code (structured fields, no message-wording matching).
  • send_chat_reply failures from the delivery call are recorded in the reply-satisfaction state file: a non-retryable error, or 5 failed attempts (bounded fallback for unclassified errors), stamps terminalDeliveryFailureAtMs.
  • Once terminal, the tool result tells the agent explicitly that delivery is failing permanently and not to retry.
  • The stop hook and silence hook treat the terminal delivery failure as the turn's terminal outcome (terminal_delivery_failure allow) instead of demanding an undeliverable closeout, so the task completes cleanly with the outcome in the transcript.
  • A later successful post or a new inbound turn clears the failure state, so recovery restores normal closeout enforcement.

Not in this PR

  • The escape hatch covers send_chat_reply on every provider (Slack, Teams, Telegram, Discord) via the bounded failure budget, since it instruments the shared tool path. The fast path (immediate terminal on a structured non-retryable code) is Slack-only for now; surfacing structured error codes from the other providers is a follow-up.
  • post_to_channel keeps its generic errors and deliberately does not feed the delivery-failure budget: it can target arbitrary channels, so its failures must not poison the bound thread's closeout state. Structured error codes for it are part of the same follow-up.
  • Surfacing the delivery failure on the automation run record (beyond the transcript) is a follow-up.

Validation

  • pnpm lint, pnpm check-types, pnpm knip all pass.
  • Full @roomote/slack suite (350 tests), full @roomote/worker suite (1619 tests; the 4 failures in tail-file-path/command-executor are pre-existing on develop in this environment), and apps/api MCP handler suite (165 tests) pass.
  • New coverage: postMessageDetailed outcomes, error-code classification, the 422/502/409 handler mapping, delivery-failure recording (count, terminal stamp, clear-on-success, reset-on-new-turn, non-parent-session guard), the terminal rewrite of the tool result, and terminal_delivery_failure allows in both hook scripts.

An automation task whose Slack channel rejects every chat.postMessage got
stuck in a loop: the stop hook demands a terminal closeout, the closeout
post fails with an opaque generic 502, the agent retries, and the hook
re-blocks on the next idle. The task burned reminder budget on 15 failed
posts and then completed silently with no closeout delivered anywhere.

Two fixes:

- Propagate the real Slack error. SlackNotifier.postMessageDetailed
  preserves the structured Slack error code (not_in_channel,
  invalid_auth, ...) instead of collapsing every failure into a missing
  timestamp, and the thread_reply MCP handler returns it with a
  structured retryable verdict (422 for permanent errors, 502 for
  transient ones).
- Give the closeout requirement an escape hatch. The worker records
  failed deliveries in the reply-satisfaction state; a non-retryable
  error (or five failed attempts) stamps a terminal delivery failure,
  the send_chat_reply result tells the agent to stop retrying, and the
  stop/silence hooks stand down so the task can complete instead of
  being reminded into a post that cannot succeed. A later successful
  post or a new inbound turn clears the stamp.
@roomote-community

roomote-community Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

No new code issues found. See task

  • apps/worker/src/mcp/roomote-mcp-server/chat-reply-satisfaction.ts:284 Successful reactions clear terminalDeliveryFailureAtMs, even though they do not prove that chat.postMessage has recovered. A later reaction can re-enable closeout enforcement and send the task back into the same failed-delivery loop.
  • apps/worker/src/mcp/roomote-mcp-server/index.ts:1551 post_to_channel returns its handler result without annotating or recording delivery failures. It therefore never consumes the documented bounded retry budget and can still be retried indefinitely after a permanent or unclassified delivery failure. — dismissed: the bound-thread closeout loop is driven only by send_chat_reply; post_to_channel may target an unrelated channel and must not alter its delivery state.

Reviewed 796b095

A successful reaction goes through reactions.add, not chat.postMessage,
so it proves nothing about the posting path. Clearing the delivery
failure state on reactions would re-arm closeout enforcement against a
channel that still rejects posts.
@daniel-lxs

Copy link
Copy Markdown
Member Author

@roomote-roomote Addressed in 796b095 / responded below:

  1. Reactions clearing the terminal delivery failure — fixed. The delivery-failure state (count, code, terminal stamp) is now only cleared when the successful satisfaction came from send_chat_reply, i.e. an actual successful chat.postMessage. Reactions go through reactions.add and prove nothing about the posting path. Test added: a successful reaction preserves the full failure state.

  2. post_to_channel not consuming the retry budget — deliberate, and the PR description has been updated to state the scope accurately (it previously overclaimed). Two reasons it stays out: the closeout loop this PR fixes is driven exclusively by send_chat_reply (the stop hook's terminal-closeout requirement is only satisfiable through it, so post_to_channel failures cannot cause that loop), and post_to_channel accepts an arbitrary target channel — recording its failures into the bound thread's satisfaction state would let a failure against some other channel incorrectly kill closeout enforcement for a healthy bound channel. Giving post_to_channel its own structured error surface (so the agent sees a non-retryable verdict instead of a generic 502) is queued as a follow-up alongside structured error codes for the Teams/Telegram/Discord providers.

@daniel-lxs
daniel-lxs merged commit f0d18e4 into develop Aug 4, 2026
19 checks passed
@daniel-lxs
daniel-lxs deleted the fix/slack-delivery-failure-closeout-escape branch August 4, 2026 16:26
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.

1 participant