fix(slack): route stream-fallback delivery through chunked sender (follow-up to #70370)#71124
Merged
steipete merged 3 commits intoApr 24, 2026
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44bd320a97
ℹ️ 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".
Comment on lines
651
to
654
| if (streamSession) { | ||
| markSlackStreamFallbackDelivered(streamSession); | ||
| } | ||
| await deliverNormally({ |
Contributor
There was a problem hiding this comment.
Do not clear the stream session before fallback delivery here: markSlackStreamFallbackDelivered(streamSession) empties pendingText before deliverNormally runs, so any earlier buffered text is lost when a non-benign append/start error happens. For example, if chunk A is buffered in the SDK and appending chunk B throws a generic network error, this branch drops A+B from pendingText and only sends payload B via deliverNormally, which can silently truncate the final reply content.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Good catch — you're right, clearing pendingText dropped anything the SDK had buffered from earlier turns. Fixed in ba81aba8: on non-benign streaming errors with non-empty pendingText, route the full buffer through deliverPendingStreamFallback with a synthetic SlackStreamNotDeliveredError and skip deliverNormally (the current payload's text is already included in pendingText per appendSlackStream's accumulation). Falls back to deliverNormally only if the chunked send itself fails. Test updated to assert the full buffered content ("first buffered\nsecond payload") gets routed through deliverReplies.
Contributor
Greptile SummaryThis PR hardens Slack stream-fallback delivery with two focused fixes: Confidence Score: 5/5Safe to merge — all changed paths have tight test coverage and no P0/P1 issues were found. Both fixes are narrowly scoped, logically consistent with stopSlackStream's stopped/pendingText contract, and verified by four targeted regression tests (two updated, two new). No silent data loss, no duplicate delivery path, no security concern introduced. No files require special attention. Reviews (1): Last reviewed commit: "fix(slack): route stream-fallback delive..." | Re-trigger Greptile |
steipete
force-pushed
the
fix/slack-stream-fallback-chunked
branch
from
April 24, 2026 21:45
ba81aba to
1fb3d9f
Compare
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.
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.
steipete
force-pushed
the
fix/slack-stream-fallback-chunked
branch
from
April 24, 2026 21:47
1fb3d9f to
5bd1083
Compare
Contributor
|
Landed via squash after rebasing onto latest Fixup added:
Proof:
|
This was referenced Apr 24, 2026
Angfr95
pushed a commit
to Angfr95/openclaw
that referenced
this pull request
Apr 25, 2026
…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]>
ogt-redknie
pushed a commit
to ogt-redknie/OPENX
that referenced
this pull request
May 2, 2026
…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]>
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
May 9, 2026
…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]>
globalcaos
pushed a commit
to globalcaos/tinkerclaw
that referenced
this pull request
May 13, 2026
…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]>
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
May 24, 2026
…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]>
jameslcowan
pushed a commit
to jameslcowan/openclaw
that referenced
this pull request
Jun 2, 2026
…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]>
sablehead
pushed a commit
to sablehead/openclaw
that referenced
this pull request
Jun 10, 2026
…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]>
This was referenced Jun 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
deliverPendingStreamFallbackwas callingchat.postMessagedirectly forerr.pendingText, which bypasses the chunked reply path used everywhere else. For Slack Connect cases where the buffered tail exceeds Slack's single-message text limit, the raw post failed withmsg_too_longand the unsent tail was dropped. Relatedly, non-benign streaming errors leftpendingTextpopulated on the session so the post-loop finalize re-posted the same chunk thatdeliverNormallyhad already sent.Two changes, one concern (stream-fallback delivery hardening):
deliverPendingStreamFallbackroutes throughdeliverReplies, which chunks long text and honors the configuredreplyToMode/ identity.deliverWithStreamingclears the session viamarkSlackStreamFallbackDeliveredbefore falling back todeliverNormally. Without this,pendingTextstays populated and the post-loop finalize (stopSlackStream→SlackStreamNotDeliveredError→ fallback) re-posts the same chunk thatdeliverNormallyalready sent.What this addresses
extensions/slack/src/monitor/message-handler/dispatch.ts:451/452: three phrasings of "Route pending-stream fallback through chunked sender" / "Route stream fallback through chunked reply sender" / "Route stream fallback through chunked Slack sender".dispatch.ts: "Avoid reposting buffered text after append fallback".The remaining Codex P1 on
extensions/slack/src/streaming.ts:247about silently swallowing finalize errors when the SDK buffer may still be non-empty touches a different file and a different class of concern — I would rather ship that as a separate follow-up than fold it in here.Suggested release note
Fix: Slack fallback for streamed replies no longer drops long buffered text on Slack Connect recipient errors, and no longer duplicates the buffered chunk when a streaming API call fails mid-turn.
Testing
pnpm check:changedgreen: extension-test typecheck,pnpm lint:extensions(5057 files, 211 rules, 0 warnings / 0 errors),pnpm check:import-cycles(0 runtime cycles), 75 tests passed across 6 files.dispatch.preview-fallback.test.ts: 4 pre-existing tests preserved, 2 updated to assert the new chunked routing, 2 new regressions added — oversizedpendingTextforwarded intact withtextLimit: 4000, and non-benign-error path clears stream state so finalize skips without reposting.dispatch.streaming.test.ts+streaming.test.ts: 35/35 green.Follow-up to #70370.