fix(slack): treat Slack Connect finalize errors as benign in stopSlackStream#70370
Conversation
Greptile SummaryThis PR adds targeted error handling in Confidence Score: 5/5Safe to merge — the change is narrowly scoped to the finalize path, all error paths are covered by tests, and no behavior changes for non-Slack-Connect users. All findings are P2 or lower. The implementation correctly guards against retry loops, the allowlist is documented and easy to extend, and the test suite covers every claimed scenario including unexpected-error re-throw and duplicate-stop no-op. No files require special attention. Reviews (1): Last reviewed commit: "fix(slack): treat Slack Connect finalize..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d5aa5eed1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (session.delivered) { | ||
| logVerbose( | ||
| `slack-stream: finalize rejected by Slack (${code}); prior appends delivered, treating stream as stopped`, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
This branch treats benign chat.stopStream errors as success whenever session.delivered is true, but delivered only means at least one earlier flush succeeded, not that the current ChatStreamer buffer is empty. A common case is: one chunk flushes (so delivered=true), then a final sub-buffer_size tail stays buffered, and stop() fails with user_not_found/team_not_found; returning here drops that tail and silently truncates the reply. Before this change the failure was surfaced via runtime.error, but now the truncation can be hidden behind verbose logging.
Useful? React with 👍 / 👎.
|
Pushed 9d5aa5e addressing a silent-data-loss cascade the first revision would have introduced for short Slack Connect replies. The problem: @slack/web-api's
11 new streaming tests (up from 6), full Slack extension lane passes (739 tests across 89 files). |
9d5aa5e to
995019c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 995019c1f9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await postChatMessage({ | ||
| channel: finalStream.channel, | ||
| thread_ts: finalStream.threadTs, | ||
| text: err.pendingText, |
There was a problem hiding this comment.
When appendSlackStream fails, deliverWithStreaming already falls back to deliverNormally for that payload, but finalization later catches SlackStreamNotDeliveredError and always posts err.pendingText. Because pendingText is accumulated before streamer.append succeeds, the failed chunk is included and gets sent again in this fallback path. In Slack Connect recipient-error scenarios where append and stop both fail before any flush, users can receive duplicated content (one normal fallback message plus another message containing the same chunk).
Useful? React with 👍 / 👎.
995019c to
cd950fb
Compare
cd950fb to
6d5fe17
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d5fe17498
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await postChatMessage({ | ||
| channel: session.channel, | ||
| thread_ts: session.threadTs, | ||
| text: fallbackText, | ||
| }); |
There was a problem hiding this comment.
deliverPendingStreamFallback sends err.pendingText via one raw chat.postMessage call, which bypasses the normal deliverNormally/deliverReplies path that chunks long replies. In Slack Connect cases where appendSlackStream raises SlackStreamNotDeliveredError with a large pending chunk, this single post can fail (for example due to message-length limits), and deliverWithStreaming then rethrows instead of using the chunked fallback, so the unsent text is dropped. Before this change, append failures always fell back through the normal sender and did not have this regression.
Useful? React with 👍 / 👎.
6d5fe17 to
a74fb6a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a74fb6a24b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await postChatMessage({ | ||
| channel: session.channel, | ||
| thread_ts: session.threadTs, | ||
| text: fallbackText, | ||
| }); |
There was a problem hiding this comment.
deliverPendingStreamFallback sends err.pendingText with one raw chat.postMessage call instead of the normal deliverReplies/sendMessageSlack path that enforces chunking and Slack text limits. Because pendingText is built by repeated session.pendingText += text appends in streaming.ts, this fallback can contain multi-chunk buffered content; if it exceeds Slack limits, this single post fails and the buffered reply is not reliably recovered (append fallback rethrows, finalize fallback only logs). Using the standard reply sender here would avoid dropping long Slack Connect replies.
Useful? React with 👍 / 👎.
…kStream When Slack's chat.stopStream fails with user_not_found (Slack Connect DM recipients), team_not_found (cross-workspace shared channels), or missing_recipient_user_id (DM closed mid-stream), the text already delivered via append() is still visible to the user. Swallow those specific codes and mark the session stopped rather than surfacing a spurious 'slack-stream: failed to stop stream' error in dispatch. Other Slack API errors still propagate. Fixes openclaw#70295
…pre-flush Address adversarial review finding on openclaw#70295: the prior swallow-on-benign fix silently dropped short replies to Slack Connect users. The SDK's ChatStreamer buffers text locally until buffer_size (256 default), so short replies never trigger chat.startStream via append(). streamer.stop() then issues startStream internally; on Slack Connect recipients this throws user_not_found. With the prior fix that error was swallowed and the dispatcher marked the turn delivered - user saw 'done' reaction but no message. SlackStreamSession now tracks delivered (true once any Slack API call returned a response) and pendingText (accumulation of every append + final-stop text). stopSlackStream: - swallows the benign code when delivered=true (prior append flushed; text is visible; same behavior as before) - throws a new SlackStreamNotDeliveredError carrying pendingText when delivered=false (nothing reached Slack) dispatch.ts catches SlackStreamNotDeliveredError and posts pendingText via a rename-bound chat.postMessage (to dodge the unicorn lint rule), and flips streamFallbackDelivered so anyReplyDelivered stays correct. Fixes openclaw#70295
720a73e to
9dbefb7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 720a73e377
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await postChatMessage({ | ||
| channel: session.channel, | ||
| thread_ts: session.threadTs, | ||
| text: fallbackText, |
There was a problem hiding this comment.
deliverPendingStreamFallback sends err.pendingText in a single chat.postMessage call, but this bypasses the normal deliverReplies/sendMessageSlack chunking path used elsewhere in this file. Because pendingText can accumulate multiple buffered chunks before a flush error, this fallback can exceed Slack text limits; when that happens, deliverPendingStreamFallback returns false and the buffered reply is either rethrown from deliverWithStreaming or only logged during finalize, so users can lose the unsent tail instead of getting a chunked fallback delivery.
Useful? React with 👍 / 👎.
deliverPendingStreamFallback was calling chat.postMessage directly for err.pendingText, which bypasses the chunked reply path used everywhere else. For Slack Connect cases where appendSlackStream throws SlackStreamNotDeliveredError with a large pending buffer, the single raw post could fail (msg_too_long) and drop the unsent tail. Two changes: 1. deliverPendingStreamFallback now routes through deliverReplies so long pendingText is chunked by the normal sender and the fallback honors the configured replyToMode / identity. 2. The non-benign streaming-error branch in deliverWithStreaming now clears the session via markSlackStreamFallbackDelivered before falling back to deliverNormally. Without this, pendingText stays populated and the post-loop finalize (stopSlackStream → SlackStreamNotDeliveredError → fallback) re-posts the same chunk that deliverNormally already sent. Addresses the three Codex P1 findings on openclaw#70370 about bypassing the chunked sender, and the related "avoid reposting buffered text after append fallback" P1 about duplicate delivery. Tests updated to assert deliverReplies routing (instead of raw postMessage) and a new case covers the non-benign-error dedup. Follow-up to openclaw#70370.
…llow-up to #70370) (#71124) * fix(slack): route stream-fallback delivery through chunked sender deliverPendingStreamFallback was calling chat.postMessage directly for err.pendingText, which bypasses the chunked reply path used everywhere else. For Slack Connect cases where appendSlackStream throws SlackStreamNotDeliveredError with a large pending buffer, the single raw post could fail (msg_too_long) and drop the unsent tail. Two changes: 1. deliverPendingStreamFallback now routes through deliverReplies so long pendingText is chunked by the normal sender and the fallback honors the configured replyToMode / identity. 2. The non-benign streaming-error branch in deliverWithStreaming now clears the session via markSlackStreamFallbackDelivered before falling back to deliverNormally. Without this, pendingText stays populated and the post-loop finalize (stopSlackStream → SlackStreamNotDeliveredError → fallback) re-posts the same chunk that deliverNormally already sent. Addresses the three Codex P1 findings on #70370 about bypassing the chunked sender, and the related "avoid reposting buffered text after append fallback" P1 about duplicate delivery. Tests updated to assert deliverReplies routing (instead of raw postMessage) and a new case covers the non-benign-error dedup. Follow-up to #70370. * fix(slack): preserve pending buffered text on non-benign stream errors Address Codex P1 on #71124: `markSlackStreamFallbackDelivered` was clearing `pendingText` before `deliverNormally` ran, so any earlier buffered chunk was lost. E.g. chunk A buffered in the SDK, then appending chunk B throws a generic network error → previous fix dropped A+B and only sent B via `deliverNormally`, silently truncating the final reply. Route the full buffered `pendingText` through `deliverPendingStreamFallback` with a synthetic `SlackStreamNotDeliveredError`, then skip `deliverNormally` entirely (pendingText already contains this payload's text, per `appendSlackStream` accumulating before throw). If the chunked fallback fails, fall back to `deliverNormally` so at least the current payload lands. Test updated to assert the full pendingText ("first buffered\nsecond payload") gets routed through the chunked sender, not the chunk-B-only partial send. * fix(slack): harden stream fallback docs and chunking test (#71124) --------- Co-authored-by: Peter Steinberger <[email protected]>
…llow-up to openclaw#70370) (openclaw#71124) * fix(slack): route stream-fallback delivery through chunked sender deliverPendingStreamFallback was calling chat.postMessage directly for err.pendingText, which bypasses the chunked reply path used everywhere else. For Slack Connect cases where appendSlackStream throws SlackStreamNotDeliveredError with a large pending buffer, the single raw post could fail (msg_too_long) and drop the unsent tail. Two changes: 1. deliverPendingStreamFallback now routes through deliverReplies so long pendingText is chunked by the normal sender and the fallback honors the configured replyToMode / identity. 2. The non-benign streaming-error branch in deliverWithStreaming now clears the session via markSlackStreamFallbackDelivered before falling back to deliverNormally. Without this, pendingText stays populated and the post-loop finalize (stopSlackStream → SlackStreamNotDeliveredError → fallback) re-posts the same chunk that deliverNormally already sent. Addresses the three Codex P1 findings on openclaw#70370 about bypassing the chunked sender, and the related "avoid reposting buffered text after append fallback" P1 about duplicate delivery. Tests updated to assert deliverReplies routing (instead of raw postMessage) and a new case covers the non-benign-error dedup. Follow-up to openclaw#70370. * fix(slack): preserve pending buffered text on non-benign stream errors Address Codex P1 on openclaw#71124: `markSlackStreamFallbackDelivered` was clearing `pendingText` before `deliverNormally` ran, so any earlier buffered chunk was lost. E.g. chunk A buffered in the SDK, then appending chunk B throws a generic network error → previous fix dropped A+B and only sent B via `deliverNormally`, silently truncating the final reply. Route the full buffered `pendingText` through `deliverPendingStreamFallback` with a synthetic `SlackStreamNotDeliveredError`, then skip `deliverNormally` entirely (pendingText already contains this payload's text, per `appendSlackStream` accumulating before throw). If the chunked fallback fails, fall back to `deliverNormally` so at least the current payload lands. Test updated to assert the full pendingText ("first buffered\nsecond payload") gets routed through the chunked sender, not the chunk-B-only partial send. * fix(slack): harden stream fallback docs and chunking test (openclaw#71124) --------- Co-authored-by: Peter Steinberger <[email protected]>
|
Thanks for landing the benign-error treatment, @steipete. |
…llow-up to openclaw#70370) (openclaw#71124) * fix(slack): route stream-fallback delivery through chunked sender deliverPendingStreamFallback was calling chat.postMessage directly for err.pendingText, which bypasses the chunked reply path used everywhere else. For Slack Connect cases where appendSlackStream throws SlackStreamNotDeliveredError with a large pending buffer, the single raw post could fail (msg_too_long) and drop the unsent tail. Two changes: 1. deliverPendingStreamFallback now routes through deliverReplies so long pendingText is chunked by the normal sender and the fallback honors the configured replyToMode / identity. 2. The non-benign streaming-error branch in deliverWithStreaming now clears the session via markSlackStreamFallbackDelivered before falling back to deliverNormally. Without this, pendingText stays populated and the post-loop finalize (stopSlackStream → SlackStreamNotDeliveredError → fallback) re-posts the same chunk that deliverNormally already sent. Addresses the three Codex P1 findings on openclaw#70370 about bypassing the chunked sender, and the related "avoid reposting buffered text after append fallback" P1 about duplicate delivery. Tests updated to assert deliverReplies routing (instead of raw postMessage) and a new case covers the non-benign-error dedup. Follow-up to openclaw#70370. * fix(slack): preserve pending buffered text on non-benign stream errors Address Codex P1 on openclaw#71124: `markSlackStreamFallbackDelivered` was clearing `pendingText` before `deliverNormally` ran, so any earlier buffered chunk was lost. E.g. chunk A buffered in the SDK, then appending chunk B throws a generic network error → previous fix dropped A+B and only sent B via `deliverNormally`, silently truncating the final reply. Route the full buffered `pendingText` through `deliverPendingStreamFallback` with a synthetic `SlackStreamNotDeliveredError`, then skip `deliverNormally` entirely (pendingText already contains this payload's text, per `appendSlackStream` accumulating before throw). If the chunked fallback fails, fall back to `deliverNormally` so at least the current payload lands. Test updated to assert the full pendingText ("first buffered\nsecond payload") gets routed through the chunked sender, not the chunk-B-only partial send. * fix(slack): harden stream fallback docs and chunking test (openclaw#71124) --------- Co-authored-by: Peter Steinberger <[email protected]>
…llow-up to openclaw#70370) (openclaw#71124) * fix(slack): route stream-fallback delivery through chunked sender deliverPendingStreamFallback was calling chat.postMessage directly for err.pendingText, which bypasses the chunked reply path used everywhere else. For Slack Connect cases where appendSlackStream throws SlackStreamNotDeliveredError with a large pending buffer, the single raw post could fail (msg_too_long) and drop the unsent tail. Two changes: 1. deliverPendingStreamFallback now routes through deliverReplies so long pendingText is chunked by the normal sender and the fallback honors the configured replyToMode / identity. 2. The non-benign streaming-error branch in deliverWithStreaming now clears the session via markSlackStreamFallbackDelivered before falling back to deliverNormally. Without this, pendingText stays populated and the post-loop finalize (stopSlackStream → SlackStreamNotDeliveredError → fallback) re-posts the same chunk that deliverNormally already sent. Addresses the three Codex P1 findings on openclaw#70370 about bypassing the chunked sender, and the related "avoid reposting buffered text after append fallback" P1 about duplicate delivery. Tests updated to assert deliverReplies routing (instead of raw postMessage) and a new case covers the non-benign-error dedup. Follow-up to openclaw#70370. * fix(slack): preserve pending buffered text on non-benign stream errors Address Codex P1 on openclaw#71124: `markSlackStreamFallbackDelivered` was clearing `pendingText` before `deliverNormally` ran, so any earlier buffered chunk was lost. E.g. chunk A buffered in the SDK, then appending chunk B throws a generic network error → previous fix dropped A+B and only sent B via `deliverNormally`, silently truncating the final reply. Route the full buffered `pendingText` through `deliverPendingStreamFallback` with a synthetic `SlackStreamNotDeliveredError`, then skip `deliverNormally` entirely (pendingText already contains this payload's text, per `appendSlackStream` accumulating before throw). If the chunked fallback fails, fall back to `deliverNormally` so at least the current payload lands. Test updated to assert the full pendingText ("first buffered\nsecond payload") gets routed through the chunked sender, not the chunk-B-only partial send. * fix(slack): harden stream fallback docs and chunking test (openclaw#71124) --------- Co-authored-by: Peter Steinberger <[email protected]>
…llow-up to openclaw#70370) (openclaw#71124) * fix(slack): route stream-fallback delivery through chunked sender deliverPendingStreamFallback was calling chat.postMessage directly for err.pendingText, which bypasses the chunked reply path used everywhere else. For Slack Connect cases where appendSlackStream throws SlackStreamNotDeliveredError with a large pending buffer, the single raw post could fail (msg_too_long) and drop the unsent tail. Two changes: 1. deliverPendingStreamFallback now routes through deliverReplies so long pendingText is chunked by the normal sender and the fallback honors the configured replyToMode / identity. 2. The non-benign streaming-error branch in deliverWithStreaming now clears the session via markSlackStreamFallbackDelivered before falling back to deliverNormally. Without this, pendingText stays populated and the post-loop finalize (stopSlackStream → SlackStreamNotDeliveredError → fallback) re-posts the same chunk that deliverNormally already sent. Addresses the three Codex P1 findings on openclaw#70370 about bypassing the chunked sender, and the related "avoid reposting buffered text after append fallback" P1 about duplicate delivery. Tests updated to assert deliverReplies routing (instead of raw postMessage) and a new case covers the non-benign-error dedup. Follow-up to openclaw#70370. * fix(slack): preserve pending buffered text on non-benign stream errors Address Codex P1 on openclaw#71124: `markSlackStreamFallbackDelivered` was clearing `pendingText` before `deliverNormally` ran, so any earlier buffered chunk was lost. E.g. chunk A buffered in the SDK, then appending chunk B throws a generic network error → previous fix dropped A+B and only sent B via `deliverNormally`, silently truncating the final reply. Route the full buffered `pendingText` through `deliverPendingStreamFallback` with a synthetic `SlackStreamNotDeliveredError`, then skip `deliverNormally` entirely (pendingText already contains this payload's text, per `appendSlackStream` accumulating before throw). If the chunked fallback fails, fall back to `deliverNormally` so at least the current payload lands. Test updated to assert the full pendingText ("first buffered\nsecond payload") gets routed through the chunked sender, not the chunk-B-only partial send. * fix(slack): harden stream fallback docs and chunking test (openclaw#71124) --------- Co-authored-by: Peter Steinberger <[email protected]>
…llow-up to openclaw#70370) (openclaw#71124) * fix(slack): route stream-fallback delivery through chunked sender deliverPendingStreamFallback was calling chat.postMessage directly for err.pendingText, which bypasses the chunked reply path used everywhere else. For Slack Connect cases where appendSlackStream throws SlackStreamNotDeliveredError with a large pending buffer, the single raw post could fail (msg_too_long) and drop the unsent tail. Two changes: 1. deliverPendingStreamFallback now routes through deliverReplies so long pendingText is chunked by the normal sender and the fallback honors the configured replyToMode / identity. 2. The non-benign streaming-error branch in deliverWithStreaming now clears the session via markSlackStreamFallbackDelivered before falling back to deliverNormally. Without this, pendingText stays populated and the post-loop finalize (stopSlackStream → SlackStreamNotDeliveredError → fallback) re-posts the same chunk that deliverNormally already sent. Addresses the three Codex P1 findings on openclaw#70370 about bypassing the chunked sender, and the related "avoid reposting buffered text after append fallback" P1 about duplicate delivery. Tests updated to assert deliverReplies routing (instead of raw postMessage) and a new case covers the non-benign-error dedup. Follow-up to openclaw#70370. * fix(slack): preserve pending buffered text on non-benign stream errors Address Codex P1 on openclaw#71124: `markSlackStreamFallbackDelivered` was clearing `pendingText` before `deliverNormally` ran, so any earlier buffered chunk was lost. E.g. chunk A buffered in the SDK, then appending chunk B throws a generic network error → previous fix dropped A+B and only sent B via `deliverNormally`, silently truncating the final reply. Route the full buffered `pendingText` through `deliverPendingStreamFallback` with a synthetic `SlackStreamNotDeliveredError`, then skip `deliverNormally` entirely (pendingText already contains this payload's text, per `appendSlackStream` accumulating before throw). If the chunked fallback fails, fall back to `deliverNormally` so at least the current payload lands. Test updated to assert the full pendingText ("first buffered\nsecond payload") gets routed through the chunked sender, not the chunk-B-only partial send. * fix(slack): harden stream fallback docs and chunking test (openclaw#71124) --------- Co-authored-by: Peter Steinberger <[email protected]>
…llow-up to openclaw#70370) (openclaw#71124) * fix(slack): route stream-fallback delivery through chunked sender deliverPendingStreamFallback was calling chat.postMessage directly for err.pendingText, which bypasses the chunked reply path used everywhere else. For Slack Connect cases where appendSlackStream throws SlackStreamNotDeliveredError with a large pending buffer, the single raw post could fail (msg_too_long) and drop the unsent tail. Two changes: 1. deliverPendingStreamFallback now routes through deliverReplies so long pendingText is chunked by the normal sender and the fallback honors the configured replyToMode / identity. 2. The non-benign streaming-error branch in deliverWithStreaming now clears the session via markSlackStreamFallbackDelivered before falling back to deliverNormally. Without this, pendingText stays populated and the post-loop finalize (stopSlackStream → SlackStreamNotDeliveredError → fallback) re-posts the same chunk that deliverNormally already sent. Addresses the three Codex P1 findings on openclaw#70370 about bypassing the chunked sender, and the related "avoid reposting buffered text after append fallback" P1 about duplicate delivery. Tests updated to assert deliverReplies routing (instead of raw postMessage) and a new case covers the non-benign-error dedup. Follow-up to openclaw#70370. * fix(slack): preserve pending buffered text on non-benign stream errors Address Codex P1 on openclaw#71124: `markSlackStreamFallbackDelivered` was clearing `pendingText` before `deliverNormally` ran, so any earlier buffered chunk was lost. E.g. chunk A buffered in the SDK, then appending chunk B throws a generic network error → previous fix dropped A+B and only sent B via `deliverNormally`, silently truncating the final reply. Route the full buffered `pendingText` through `deliverPendingStreamFallback` with a synthetic `SlackStreamNotDeliveredError`, then skip `deliverNormally` entirely (pendingText already contains this payload's text, per `appendSlackStream` accumulating before throw). If the chunked fallback fails, fall back to `deliverNormally` so at least the current payload lands. Test updated to assert the full pendingText ("first buffered\nsecond payload") gets routed through the chunked sender, not the chunk-B-only partial send. * fix(slack): harden stream fallback docs and chunking test (openclaw#71124) --------- Co-authored-by: Peter Steinberger <[email protected]>
…llow-up to openclaw#70370) (openclaw#71124) * fix(slack): route stream-fallback delivery through chunked sender deliverPendingStreamFallback was calling chat.postMessage directly for err.pendingText, which bypasses the chunked reply path used everywhere else. For Slack Connect cases where appendSlackStream throws SlackStreamNotDeliveredError with a large pending buffer, the single raw post could fail (msg_too_long) and drop the unsent tail. Two changes: 1. deliverPendingStreamFallback now routes through deliverReplies so long pendingText is chunked by the normal sender and the fallback honors the configured replyToMode / identity. 2. The non-benign streaming-error branch in deliverWithStreaming now clears the session via markSlackStreamFallbackDelivered before falling back to deliverNormally. Without this, pendingText stays populated and the post-loop finalize (stopSlackStream → SlackStreamNotDeliveredError → fallback) re-posts the same chunk that deliverNormally already sent. Addresses the three Codex P1 findings on openclaw#70370 about bypassing the chunked sender, and the related "avoid reposting buffered text after append fallback" P1 about duplicate delivery. Tests updated to assert deliverReplies routing (instead of raw postMessage) and a new case covers the non-benign-error dedup. Follow-up to openclaw#70370. * fix(slack): preserve pending buffered text on non-benign stream errors Address Codex P1 on openclaw#71124: `markSlackStreamFallbackDelivered` was clearing `pendingText` before `deliverNormally` ran, so any earlier buffered chunk was lost. E.g. chunk A buffered in the SDK, then appending chunk B throws a generic network error → previous fix dropped A+B and only sent B via `deliverNormally`, silently truncating the final reply. Route the full buffered `pendingText` through `deliverPendingStreamFallback` with a synthetic `SlackStreamNotDeliveredError`, then skip `deliverNormally` entirely (pendingText already contains this payload's text, per `appendSlackStream` accumulating before throw). If the chunked fallback fails, fall back to `deliverNormally` so at least the current payload lands. Test updated to assert the full pendingText ("first buffered\nsecond payload") gets routed through the chunked sender, not the chunk-B-only partial send. * fix(slack): harden stream fallback docs and chunking test (openclaw#71124) --------- Co-authored-by: Peter Steinberger <[email protected]>
2 participants
Summary
chat.stopStreamfails withAn API error occurred: user_not_found. OpenClaw logsslack-stream: failed to stop stream: ...viaruntime.errorin dispatch, which makes every Slack Connect reply look like an error even though the text delivered viachat.appendStreamis already visible to the user.stopSlackStreaminextensions/slack/src/streaming.tsnow catchessession.streamer.stop(...)errors. When the Slack error code is one ofuser_not_found,team_not_found, ormissing_recipient_user_id(all Slack Connect / recipient-resolution cases), swallow the error, keep the session marked asstopped, and log through the existinglogVerbosepath explaining that appended text is still visible. Any other Slack error still propagates so the caller's existingruntime.error(...)log is preserved.extensions/slack/src/monitor/message-handler/dispatch.tsis unchanged; this PR stays inside thestreaming.tsboundary. No other plugin, core, or SDK changes.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
stopSlackStreaminextensions/slack/src/streaming.tsawaitedsession.streamer.stop(...)without any error handling. The Slack Connect recipient_user_id / team_id path is not resolvable at finalize time, so Slack returnsuser_not_found(or sibling codes) onchat.stopStream. The error bubbled up through the outer try/catch indispatch.tsand was logged viaruntime.erroras if the reply itself had failed.extensions/slack/src/streaming.tsstopSlackStream()as the missing-error-handling site and the finalize failure mode as Slack-Connect-specific.Regression Test Plan (if applicable)
extensions/slack/src/streaming.test.ts(new)session.streamer.stop()throwing each of the three benign Slack Connect error shapes ->stopSlackStreamresolves without throwing and marks the session stopped. An unexpected Slack error code (not_authed) AND a non-Slack-shaped error (new Error("socket reset")) still re-throw so the caller's log path stays intact. Duplicate-stop on an already-stopped session remains a no-op.stopSlackStreamis the seam; the dispatch call site is unchanged.streaming.tshad no direct unit coverage.User-visible / Behavior Changes
runtime.error("slack-stream: failed to stop stream: user_not_found")line when the stream text was actually delivered.logVerbosewith a clear note that appended text remains visible and the stream is treated as stopped.Diagram (if applicable)
N/A
Security Impact (required)
append()call's text is already at Slack).Yes, explain risk + mitigation: N/ARepro + Verification
Environment
Steps
Expected
Reply appears in thread (as it already does via
append()) and noslack-stream: failed to stop streamerror line appears at the dispatch layer for theuser_not_found/team_not_found/missing_recipient_user_idcases.Actual (before this PR)
[slack] slack-stream: failed to stop stream: Error: An API error occurred: user_not_foundatruntime.errorlevel, even when the reply text is visible.Evidence
Local checks before push:
pnpm test extensions/slack/src/streaming.test.ts: 1 file, 6 tests pass.pnpm test extensions/slack: 90 files, 736 tests pass.pnpm check:changed --staged(pre-commit): extension lane ran 89 files, 734 tests, all pass.pnpm format:check,pnpm linton touched files: clean.Human Verification (required)
ChatStreamer.stop):user_not_foundthrow ->stopSlackStreamresolves, session marked stopped.team_not_foundthrow -> same.missing_recipient_user_idthrow -> same.not_authed) -> re-thrown with the original message; session still marked stopped to prevent retry loops.socket reset) -> re-thrown unchanged.streamer.stopnot called.session.stopped = trueon both throw paths so a retry would not re-enterstreamer.stop. The error classification reads fromerr.data.error(shape used by@slack/web-apierrors) with a message-string fallback for custom Error subclasses.Review Conversations
Compatibility / Migration
Risks and Mitigations
chat.appendStreamuses the same recipient context, so auser_not_foundat stop generally means start/append also hit the same validation path and either already failed (caller sees that as a throw) or succeeded (text is visible). If Slack changes the semantics, the allowlist is a single file and easy to adjust.stopSlackStreaminsideextensions/slack/src/streaming.ts. Non-streaming delivery paths do not call it.