Skip to content

fix(slack): preserve buffered thread stream replies#78536

Draft
vincentkoc wants to merge 1 commit into
mainfrom
fix/slack-thread-delivery-78061
Draft

fix(slack): preserve buffered thread stream replies#78536
vincentkoc wants to merge 1 commit into
mainfrom
fix/slack-thread-delivery-78061

Conversation

@vincentkoc

@vincentkoc vincentkoc commented May 6, 2026

Copy link
Copy Markdown
Member

Summary

  • Problem: Slack native streaming can keep short thread replies in the SDK buffer until finalization, and unexpected chat.stopStream failures before that flush can drop the generated reply.
  • Why it matters: Slack thread session generates responses but fails to deliver to Slack #78061 reports Slack thread sessions generating assistant transcript replies while nothing appears in the Slack thread.
  • What changed: stopSlackStream() now converts any finalize failure with pending buffered text into the existing fallback-delivery error, so the message is posted through normal threaded chat.postMessage delivery.
  • What did NOT change (scope boundary): no Slack config/default changes, no thread-routing changes, no native streaming disablement.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: Slack thread replies generated by the agent should still post to the Slack thread when native stream finalization rejects a locally buffered reply.
  • Real environment tested: local OpenClaw checkout on macOS, Node/pnpm repo test harness; Blacksmith Testbox was attempted but unavailable before command execution.
  • Exact steps or command run after this patch: pnpm test:serial extensions/slack/src/streaming.test.ts extensions/slack/src/monitor/message-handler/dispatch.preview-fallback.test.ts extensions/slack/src/monitor/message-handler/dispatch.streaming.test.ts extensions/slack/src/monitor/message-handler/prepare.thread-session-key.test.ts extensions/slack/src/monitor/replies.test.ts
  • Evidence after fix: 5 test files passed, 90 tests passed.
  • Observed result after fix: pending native stream text is routed through deliverReplies with the original Slack thread timestamp when finalization fails before the SDK buffer flushes.
  • What was not tested: live Slack Socket Mode thread delivery; Testbox pnpm check:changed could not run because tbx_01kqydekhtz0ktjtfh8mwfgfb6 timed out/shut down before sync and replacement tbx_01kqyz9d18px8wjb8zz7mytd77 shut down while queued.
  • Before evidence: Slack thread session generates responses but fails to deliver to Slack #78061 includes v2026.5.4 transcript replies with no Slack thread delivery, plus successful direct chat.postMessage to the same thread.

Root Cause (if applicable)

  • Root cause: Slack SDK ChatStreamer buffers short markdown_text locally until finalization; OpenClaw only converted a small allowlist of finalize errors into fallback-deliverable pending text.
  • Missing detection / guardrail: there was no regression test for unexpected native stream finalization failures while text was still local-only.
  • Contributing context (if known): append-time failures already had broad buffered fallback handling, but stop/finalize failures did not.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file: extensions/slack/src/streaming.test.ts; extensions/slack/src/monitor/message-handler/dispatch.preview-fallback.test.ts
  • Scenario the test should lock in: unexpected native stream finalization errors with pending text must route through normal threaded delivery instead of being dropped.
  • Why this is the smallest reliable guardrail: it locks the Slack SDK buffer contract and the message-handler fallback seam without requiring live Slack credentials.
  • Existing test that already covers this (if any): existing append-failure fallback tests covered mid-stream errors, not stop/finalize errors.
  • If no new test is added, why not: N/A

User-visible / Behavior Changes

Slack thread replies that previously disappeared when native streaming finalization failed should now post through normal threaded delivery.

Diagram (if applicable)

Before:
short native stream reply -> SDK buffer -> stopStream rejects -> reply can disappear

After:
short native stream reply -> SDK buffer -> stopStream rejects -> threaded deliverReplies fallback -> Slack thread post

Security Impact (required)

  • New permissions/capabilities? (Yes/No) No
  • Secrets/tokens handling changed? (Yes/No) No
  • New/changed network calls? (Yes/No) No, fallback uses the existing Slack send path already used for normal replies.
  • Command/tool execution surface changed? (Yes/No) No
  • Data access scope changed? (Yes/No) No
  • If any Yes, explain risk + mitigation: N/A

Repro + Verification

Environment

  • OS: macOS local checkout
  • Runtime/container: repo pnpm/Vitest harness
  • Model/provider: N/A
  • Integration/channel (if any): Slack
  • Relevant config (redacted): Slack native streaming enabled path

Steps

  1. Simulate a native Slack stream with pending buffered text.
  2. Make stream finalization reject with an unexpected Slack error code or non-Slack error.
  3. Dispatch the prepared Slack message.

Expected

  • Buffered text is sent through normal Slack threaded reply delivery.

Actual

  • Fixed: deliverReplies receives the pending text and original threadTs.

Evidence

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios: local targeted Slack regression suite passed; diff/status sanity after clean rebase passed; git diff --check origin/main...HEAD passed.
  • Edge cases checked: unexpected Slack finalize code with pending text; non-Slack stop error with pending text; existing benign-code behavior; existing append fallback behavior; replies/thread timestamp tests.
  • What you did not verify: live Slack Socket Mode delivery and pnpm check:changed, blocked by Testbox leases shutting down before execution.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? (Yes/No) Yes
  • Config/env changes? (Yes/No) No
  • Migration needed? (Yes/No) No
  • If yes, exact upgrade steps: N/A

Risks and Mitigations

  • Risk: native stream finalization may have partially delivered earlier chunks before a pending tail fallback.
    • Mitigation: the fallback only posts session.pendingText, which is cleared after acknowledged native flushes, so it avoids replaying already-acknowledged chunks.

@openclaw-barnacle openclaw-barnacle Bot added channel: slack Channel integration: slack size: S maintainer Maintainer-authored PR labels May 6, 2026
@clawsweeper

clawsweeper Bot commented May 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

Summary
The PR broadens Slack native stream finalization fallback so locally buffered pending text is sent through normal threaded delivery, adds regression tests, and records a changelog fix.

Reproducibility: Do we have a high-confidence way to reproduce the issue? Yes at the source level: on current main, make appendSlackStream buffer text locally, then make stopSlackStream throw an unexpected finalize error; the current code propagates instead of routing that buffered text through the existing fallback.

Real behavior proof
Not applicable: This is a maintainer PR, so the external contributor real-behavior-proof gate does not apply; the body reports targeted local Slack tests but no live Slack run.

Next step before merge
This is a maintainer-labeled draft PR with no narrow automated repair finding; maintainers should review, validate, and undraft when ready.

Security
Cleared: The diff does not touch workflows, dependencies, package resolution, secrets, permissions, or new command execution paths; it reuses the existing Slack send path.

Review details

Best possible solution:

Merge the focused Slack extension fallback after maintainer readiness, targeted Slack validation, changed-gate proof, and preferably a live Socket Mode thread check; keep the linked user issue open until this PR lands.

Do we have a high-confidence way to reproduce the issue?

Do we have a high-confidence way to reproduce the issue? Yes at the source level: on current main, make appendSlackStream buffer text locally, then make stopSlackStream throw an unexpected finalize error; the current code propagates instead of routing that buffered text through the existing fallback.

Is this the best way to solve the issue?

Is this the best way to solve the issue? Yes, the PR uses the existing Slack fallback delivery path and keeps the change limited to locally buffered stream finalization failures, without changing Slack config, routing, or native streaming defaults.

Acceptance criteria:

  • pnpm test:serial extensions/slack/src/streaming.test.ts extensions/slack/src/monitor/message-handler/dispatch.preview-fallback.test.ts extensions/slack/src/monitor/message-handler/dispatch.streaming.test.ts extensions/slack/src/monitor/message-handler/prepare.thread-session-key.test.ts extensions/slack/src/monitor/replies.test.ts
  • pnpm check:changed
  • Live Slack Socket Mode proof that a generated reply appears in the same Slack thread after a native stream finalize failure, if feasible

What I checked:

  • Protected maintainer PR: The live PR metadata shows this PR is draft, authored by a repository member, and labeled maintainer, so cleanup policy keeps it open for explicit maintainer handling. (552ebcc7f042)
  • Current main fallback boundary: Current main only converts stopSlackStream failures into SlackStreamNotDeliveredError for the known benign finalize codes; other stop errors propagate, while dispatch only invokes the pending-stream fallback for SlackStreamNotDeliveredError. (extensions/slack/src/streaming.ts:235, d648673b3105)
  • Existing fallback uses threaded delivery: The existing deliverPendingStreamFallback path sends buffered text through deliverReplies with replyThreadTs: session.threadTs, preserving the Slack thread target when fallback delivery succeeds. (extensions/slack/src/monitor/message-handler/dispatch.ts:501, d648673b3105)
  • Slack SDK buffering contract checked: @slack/web-api 7.15.2 ChatStreamer.append buffers markdown below the default 256-character buffer_size and returns null; stop() later sends buffered markdown as chat.stopStream chunks, matching the reported root cause.
  • PR diff matches the suspected bug path: The diff moves the pendingText check ahead of benign-code classification so any stop failure with local-only buffered text becomes the existing fallback-deliverable error, and it reports an error if fallback also fails before any delivery. (extensions/slack/src/streaming.ts:235, 552ebcc7f042)
  • Regression coverage added: The PR adds unit coverage for unexpected Slack finalize codes and non-Slack stop errors with pending text, plus a dispatch seam test that verifies the pending reply reaches deliverReplies with the original thread timestamp. (extensions/slack/src/streaming.test.ts:83, 552ebcc7f042)

Likely related people:

  • vincentkoc: Current-main blame for the central Slack streaming and dispatch fallback regions points to Vincent Koc, and this PR continues that same extension-owned fallback path. (role: recent maintainer; confidence: medium; commits: a2f1d1dfd8ab, 552ebcc7f042; files: extensions/slack/src/streaming.ts, extensions/slack/src/monitor/message-handler/dispatch.ts)
  • thewilloftheshadow: Historical commits by Shadow introduced Slack/Discord thread sessions, the routing model that makes Slack thread reply delivery part of the supported behavior under review here. (role: introduced thread behavior; confidence: medium; commits: 7e5cef29a034, d4bba937a08a; files: src/slack/monitor.ts, src/auto-reply/reply.ts, extensions/slack/src/monitor/message-handler/prepare-routing.ts)
  • SocialNerd42069: A prior merged Slack fix by SocialNerd42069 preserved Slack thread context for tool notification delivery, which is adjacent to this PR's threaded reply fallback surface. (role: adjacent owner; confidence: medium; commits: 0d6e78b718f4; files: src/slack/monitor/message-handler/prepare.ts, src/channels/plugins/outbound/slack.ts, src/auto-reply/reply/agent-runner-helpers.ts)

Remaining risk / open question:

  • No live Slack Socket Mode thread run was inspected for this PR; the proof is targeted local unit/seam behavior plus issue reports.
  • The PR remains draft and protected by the maintainer label, so maintainer readiness and final validation are still unresolved.
  • The broad changed gate was not shown as completed in the inspected PR evidence; the PR body reports targeted Slack tests passing.

Codex review notes: model gpt-5.5, reasoning high; reviewed against d648673b3105.

byungskers
byungskers reviewed May 6, 2026
View reviewed changes

@byungskers byungskers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Excellent bug fix with thorough documentation and test coverage. The Slack native streaming buffer contract is tricky, and losing short replies because stopStream failed before flush is a frustrating user experience.

A few constructive notes:

  1. Fallback delivery duplication risk: The PR mentions that pendingText is cleared after acknowledged native flushes, which avoids replay. Could you add a test that explicitly verifies no duplicate delivery occurs if a partial flush succeeded before the final stop failure?

  2. Error classification: The change broadens the error allowlist for fallback delivery. Is there any error type where fallback delivery would be incorrect (e.g., rate limits or auth failures where retrying via chat.postMessage would also fail)? The current approach seems safe since it only posts already-generated text, but worth confirming.

  3. Testbox gap: You noted Testbox was unavailable for pnpm check:changed. If this merges before you can run it, consider scheduling a follow-up Testbox run or asking a maintainer to trigger one — the Slack extension has enough surface area that changed-surface validation is valuable.

The real behavior proof section is particularly well done. The before/after flow diagram makes the change easy to understand. LGTM with minor test suggestions.

@sean-codevasp

sean-codevasp commented May 11, 2026

Copy link
Copy Markdown

when will this be merged to main?

This was referenced May 14, 2026
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@byungskers byungskers byungskers left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

channel: slack Channel integration: slack maintainer Maintainer-authored PR size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Slack thread session generates responses but fails to deliver to Slack

3 participants

@vincentkoc @sean-codevasp @byungskers