Skip to content

fix(slack): retain delivered final replies during late cleanup#87364

Merged
steipete merged 1 commit into
openclaw:mainfrom
tianxiaochannel-oss88:fix/slack-final-reply-retain-after-late-payload
May 27, 2026
Merged

fix(slack): retain delivered final replies during late cleanup#87364
steipete merged 1 commit into
openclaw:mainfrom
tianxiaochannel-oss88:fix/slack-final-reply-retain-after-late-payload

Conversation

@tianxiaochannel-oss88

@tianxiaochannel-oss88 tianxiaochannel-oss88 commented May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Track when a Slack turn has already produced a visible final reply, including normal delivery, native stream delivery, and preview finalization.
  • Stop passing the draft stream into later finalizable-preview handling after that point, so late same-turn error/recovery payloads cannot call draftStream.clear() and delete a visible answer.
  • Keep the existing first-fallback behavior unchanged for non-finalized previews, so transient previews are still cleaned up when initially replaced by normal delivery.

Fixes #87363.

Root cause

Slack's draftStream.clear() deletes the draft message. The dispatch path already protected in-place finalized previews with draftPreviewCommitted, but a final reply can also become visible through normal or streaming delivery. A later same-turn payload could still reuse the draft finalizer when draftPreviewCommitted was false, reaching fallback cleanup and deleting the message.

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: Slack draft cleanup can delete or replace a user-visible final reply when a late same-turn error/recovery payload reuses the draft finalizer after final delivery.
  • Real environment tested: local macOS OpenClaw source checkout on this PR branch, running the real OpenClaw Vitest project runner against the Slack extension dispatch shard and the shared channel lifecycle shard.
  • Exact steps or command run after this patch: node scripts/test-projects.mjs extensions/slack/src/monitor/message-handler/dispatch.preview-fallback.test.ts src/channels/message/lifecycle.test.ts -- --reporter=verbose
  • Evidence after fix (screenshot, recording, terminal capture, console output, redacted runtime log, linked artifact, or copied live output): copied terminal output from the local run:
[test] starting test/vitest/vitest.extension-slack.config.ts
✓ |extension-slack| extensions/slack/src/monitor/message-handler/dispatch.preview-fallback.test.ts > dispatchPreparedSlackMessage preview fallback > does not reuse draft cleanup after a normally delivered final reply
Test Files  1 passed (1)
Tests  40 passed (40)

[test] starting test/vitest/vitest.channels.config.ts
✓ |channels| src/channels/message/lifecycle.test.ts > message lifecycle primitives > treats live preview fallback delivery as terminal state
Test Files  1 passed (1)
Tests  16 passed (16)

[test] passed 2 Vitest shards in 8.75s
  • Observed result after fix: the new Slack regression delivers the original final payload and the later error payload, but draftStream.clear() is called only for the initial normal-delivery fallback and is not reused for the late same-turn cleanup payload. The shared lifecycle tests still pass, confirming the generic first-fallback cleanup behavior remains intact.
  • What was not tested: I did not send a new live message to a private Slack workspace from this PR branch.
  • Proof limitations or environment constraints: the original reproduction involved a private Slack channel and private runtime logs, so public proof is limited to redacted log timing plus the local OpenClaw Slack dispatch shard. The patch is intentionally Slack dispatch-scoped and does not change the generic live-preview lifecycle.
  • Before evidence (optional but encouraged): redacted local runtime/session evidence showed delivered reply to channel:[redacted] around 2026-05-28 00:45:22 +08:00, followed by a Slack message_deleted session event roughly 15 seconds later during the same broader recovery/error sequence. No token, account, full channel id, user id, session id, or full Responses item id is included.

Tests and validation

  • node scripts/test-projects.mjs extensions/slack/src/monitor/message-handler/dispatch.preview-fallback.test.ts src/channels/message/lifecycle.test.ts -- --reporter=verbose

Regression coverage added:

  • A Slack dispatch regression where a normally delivered final reply is followed by a late same-turn error final; the late payload is delivered without reusing draft cleanup.

What failed before this fix:

  • The late same-turn payload could still receive the draft stream and reach draftStream.clear() because only in-place preview finalization set draftPreviewCommitted.

@openclaw-barnacle openclaw-barnacle Bot added channel: slack Channel integration: slack size: S labels May 27, 2026
@clawsweeper

clawsweeper Bot commented May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed May 27, 2026, 1:47 PM ET / 17:47 UTC.

Summary
The PR tracks visible final Slack reply delivery and prevents later same-turn preview fallback cleanup from reusing the draft stream after final content is already visible.

PR surface: Source +18, Tests +36. Total +54 across 2 files.

Reproducibility: Do we have a high-confidence way to reproduce the issue? Yes at source and unit level: current main still allows the draft adapter after normal final delivery, and the PR encodes the media-final followed by late-error sequence; I did not run tests or a live Slack repro because this review is read-only.

Review metrics: none identified.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Add redacted after-fix Slack proof showing the final reply remains visible after a late same-turn error/recovery payload.
  • Update the PR body with that proof so ClawSweeper can re-review automatically, or ask a maintainer to comment @clawsweeper re-review if it does not.

Proof guidance:
Needs real behavior proof before merge: The PR body provides copied Vitest output and redacted before logs, but no after-fix live Slack message, recording, terminal log, or runtime output from the real Slack path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A real Slack desktop smoke would materially prove the visible reply-retention behavior that mocked tests cannot show. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

slack desktop smoke: verify a Slack final reply with draft preview remains visible when a late same-turn error final is delivered.

Risk before merge

  • The patch changes Slack final/draft cleanup in a user-visible delivery path; without after-fix live Slack proof, duplicate-message, retained-draft, or deletion regressions are still covered only by mocked tests.

Maintainer options:

  1. Add live Slack proof before merge (recommended)
    Have the contributor or a maintainer run a redacted Slack workspace scenario showing the visible final reply remains after a late same-turn error/recovery payload and no stale draft remains.
  2. Accept test-only proof
    Maintainers could intentionally merge based on the targeted Slack regression and shared lifecycle tests, accepting that the exact Slack deletion path was not proven live after the patch.

Next step before merge
The PR needs human/contributor real Slack proof before merge; there is no narrow code repair for ClawSweeper to apply on the branch.

Security
Cleared: The diff only changes Slack dispatch logic and a colocated test; it does not alter dependencies, CI, secrets handling, package metadata, or code-execution surfaces.

Review details

Best possible solution:

Land the Slack-scoped final-delivery guard with the regression test, after adding redacted live Slack proof or maintainer-run Slack smoke proof for the late-payload scenario.

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

Do we have a high-confidence way to reproduce the issue? Yes at source and unit level: current main still allows the draft adapter after normal final delivery, and the PR encodes the media-final followed by late-error sequence; I did not run tests or a live Slack repro because this review is read-only.

Is this the best way to solve the issue?

Is this the best way to solve the issue? Yes for the code direction: the guard is Slack-scoped, preserves the shared live-preview fallback contract, and keeps the first fallback cleanup behavior intact; the remaining blocker is proof, not a different implementation shape.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P1: The linked bug can make a delivered Slack final answer disappear, which is an urgent channel workflow and message-loss regression.
  • add merge-risk: 🚨 message-delivery: The diff changes Slack draft cleanup and final delivery gating, so an incorrect merge could affect visible Slack reply retention or duplicate delivery.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides copied Vitest output and redacted before logs, but no after-fix live Slack message, recording, terminal log, or runtime output from the real Slack path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P1: The linked bug can make a delivered Slack final answer disappear, which is an urgent channel workflow and message-loss regression.
  • merge-risk: 🚨 message-delivery: The diff changes Slack draft cleanup and final delivery gating, so an incorrect merge could affect visible Slack reply retention or duplicate delivery.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides copied Vitest output and redacted before logs, but no after-fix live Slack message, recording, terminal log, or runtime output from the real Slack path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +18, Tests +36. Total +54 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 19 1 +18
Tests 1 36 0 +36
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 55 1 +54

What I checked:

  • Repository policy read: Read the full root AGENTS.md and the scoped extensions/AGENTS.md; the review applied the OpenClaw proof bar, bundled-plugin boundary guidance, and message-delivery risk handling. (AGENTS.md:17, 1806b152a919)
  • Current-main source still passes the draft adapter after non-preview final delivery: Current main passes a draft adapter whenever draftStream exists and draftPreviewCommitted is false; there is no separate guard for finals delivered by normal or stream paths. (extensions/slack/src/monitor/message-handler/dispatch.ts:990, 1806b152a919)
  • Shared fallback clears drafts after normal delivery: The shared finalizable live-preview fallback intentionally discards or clears the draft, delivers normally, and clears again after successful delivery, which explains why Slack must stop reusing this fallback after a visible final reply. (src/channels/message/live.ts:194, 1806b152a919)
  • PR patch blocks late draft reuse after final delivery: The branch adds observedFinalReplyDelivery, marks it from normal, stream, fallback, and preview-finalized final paths, and excludes the draft adapter once that flag is set. (extensions/slack/src/monitor/message-handler/dispatch.ts:594, fc4833d6ad8e)
  • Regression coverage targets the reported sequence: The new test sends a media final through normal delivery followed by a late error final, then asserts both replies deliver and draftStream.clear is not reused for the late payload. (extensions/slack/src/monitor/message-handler/dispatch.preview-fallback.test.ts:1241, fc4833d6ad8e)
  • Related issue context: The linked issue reports a visible Slack final reply disappearing after a late same-turn recovery/error payload and explicitly ties the source-level cause to Slack draftStream.clear reuse.

Likely related people:

  • @vincentkoc: Current checkout blame for the central Slack finalizer region and shared live-preview fallback points to commit 101c834 authored by Vincent Koc. (role: recent area contributor; confidence: medium; commits: 101c83448b89; files: extensions/slack/src/monitor/message-handler/dispatch.ts, src/channels/message/live.ts)
  • @steipete: Recent history on Slack dispatch includes Slack thread starter and draft-reply work by Peter Steinberger, including commit eee37bf for duplicate draft replies. (role: adjacent Slack delivery contributor; confidence: medium; commits: 6b525023d4d2, eee37bf83634; files: extensions/slack/src/monitor/message-handler/dispatch.ts)
  • @gumadeiras: Recent Slack dispatch history includes turn-local and partial-streaming dedupe work by Gustavo Madeira Santana in the same delivery path. (role: adjacent Slack dedupe contributor; confidence: medium; commits: bd7801eefaa1, 10c87527d5d8; files: extensions/slack/src/monitor/message-handler/dispatch.ts)
  • @zimeg: Recent history on the same dispatch file includes shared-channel stream recipient handling, which is adjacent to the paths this patch marks as final-delivered. (role: adjacent Slack streaming contributor; confidence: low; commits: 25ce5a5822b2; files: extensions/slack/src/monitor/message-handler/dispatch.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@openclaw-barnacle openclaw-barnacle Bot added the proof: supplied External PR includes structured after-fix real behavior proof. label May 27, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels May 27, 2026
@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@steipete steipete self-assigned this May 27, 2026
@steipete

steipete commented May 27, 2026

Copy link
Copy Markdown
Contributor

Maintainer landing proof:

  • Reviewed Slack dispatch path, shared live-preview lifecycle, reply dispatcher ordering, and @slack/web-api@7.16.0 stream/update/delete contracts.
  • CI is green at fc4833d6ad8eb81d5bbd4401387c086012b99954; latest Real behavior proof: https://github.com/openclaw/openclaw/actions/runs/26528543580/job/78138669979
  • Existing PR proof ran node scripts/test-projects.mjs extensions/slack/src/monitor/message-handler/dispatch.preview-fallback.test.ts src/channels/message/lifecycle.test.ts -- --reporter=verbose.
  • Known gap: no new live Slack workspace replay from this branch; accepting targeted source/test proof for this Slack-scoped guard.

Landing now. Thanks @tianxiaochannel-oss88.

@steipete steipete merged commit fb1dfd4 into openclaw:main May 27, 2026
150 of 161 checks passed
@steipete steipete mentioned this pull request May 27, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

channel: slack Channel integration: slack merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P1 High-priority user-facing bug, regression, or broken workflow. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Slack draft cleanup can delete visible final replies after late same-turn payloads

2 participants

@tianxiaochannel-oss88 @steipete