Skip to content

fix: keep threaded Slack message sends route-bound for reply dedupe#90045

Closed
ffluk3 wants to merge 1 commit into
openclaw:mainfrom
ffluk3:agents/openclaw-message-thread-bug-fix
Closed

fix: keep threaded Slack message sends route-bound for reply dedupe#90045
ffluk3 wants to merge 1 commit into
openclaw:mainfrom
ffluk3:agents/openclaw-message-thread-bug-fix

Conversation

@ffluk3

@ffluk3 ffluk3 commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • preserve the active thread id when the generic message tool implicitly sends into the current Slack thread
  • make reply-payload dedupe compare threaded routes exactly so a thread-scoped send no longer suppresses later channel or other-thread replies
  • add regression coverage for extraction, main reply delivery, and followup delivery while keeping existing Telegram topic suppression behavior intact

Verification

  • pnpm tsgo:core
  • node scripts/run-vitest.mjs src/agents/embedded-agent-subscribe.tools.extract.test.ts src/auto-reply/reply/reply-payloads.test.ts src/auto-reply/reply/followup-delivery.test.ts src/auto-reply/reply/agent-runner.misc.runreplyagent.test.ts

Closes #90044

@ffluk3
fix(auto-reply): keep threaded Slack replies route-bound
3e44832 
Preserve implicit Slack thread ids in messaging-tool send telemetry and require thread-aware dedupe for later automatic replies and followups.

Adds regression coverage for Slack thread routing while keeping Telegram topic suppression behavior intact.

Refs: openclaw#90044

Co-authored-by: Copilot <[email protected]>
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 3, 2026
@clawsweeper

clawsweeper Bot commented Jun 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 14, 2026, 12:49 AM ET / 04:49 UTC.

Summary
The PR passes active Slack session thread IDs into generic message-tool send telemetry and carries originatingThreadId through final and follow-up reply dedupe with regression tests.

PR surface: Source +24, Tests +163. Total +187 across 11 files.

Reproducibility: yes. source-reproducible: current main omits the routed origin thread from reply dedupe and records only explicit generic message-tool thread ids. I did not run a live Slack workspace reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Overlapping route-dedupe PRs: 1 open overlapping PR. Another open branch touches the same reply-payload dedupe path, so maintainers need one canonical landing path before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof is added.

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

Rank-up moves:

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists tests and typecheck, but no after-fix real Slack proof; the contributor should add a redacted screenshot, recording, terminal output, copied live output, linked artifact, or logs, then update the PR body to trigger re-review or ask a maintainer to comment @clawsweeper re-review.

Mantis proof suggestion
A real Slack transport smoke would materially improve confidence for this message-delivery and dedupe fix. 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 generic message-tool sends in Slack only suppress final replies when the actual send used the same thread, including an off-thread or top-level control.

Risk before merge

  • [P1] The patch can record a session thread for a generic Slack send even when Slack delivery would send top-level or to a different target, which can still suppress a valid later threaded reply.
  • [P1] No after-fix real Slack proof is present; tests and typecheck do not prove the route-bound delivery behavior in a real workspace.
  • [P1] The open overlapping PR at fix(reply): deliver final reply when queued follow-up claims session; scope dedupe to routed thread #90943 touches the same dedupe seam, so maintainers should avoid landing conflicting variants without choosing the canonical shape.

Maintainer options:

  1. Fix telemetry to mirror delivery threading (recommended)
    Before merge, make extraction record threadId only when the same channel auto-thread resolver would apply to that target and reply mode, with coverage for replyToMode off, consumed first replies, and different targets.
  2. Add real Slack delivery proof
    After the code fix, attach redacted Slack runtime proof showing same-thread suppression and off-thread or top-level delivery behavior.
  3. Pick one dedupe branch
    If maintainers choose fix(reply): deliver final reply when queued follow-up claims session; scope dedupe to routed thread #90943 instead, port this PR's unique implicit-send telemetry fix there or close this branch after replacement.

Next step before merge

  • [P1] Needs contributor and maintainer follow-up for the thread-telemetry correctness bug, real Slack proof, and canonical-branch choice; automation cannot supply the contributor's real-workspace proof.

Security
Cleared: The diff touches TypeScript reply-runtime code and tests only; I found no concrete security or supply-chain concern.

Review findings

  • [P1] Record only the thread the send resolver actually used — src/agents/embedded-agent-subscribe.tools.ts:649
Review details

Best possible solution:

Land one canonical Slack thread-aware reply-dedupe fix that records message-tool thread evidence from the actual outbound threading decision and includes redacted real Slack proof.

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

Yes, source-reproducible: current main omits the routed origin thread from reply dedupe and records only explicit generic message-tool thread ids. I did not run a live Slack workspace reproduction in this read-only review.

Is this the best way to solve the issue?

No, not yet: carrying thread evidence through the existing dedupe seam is the right direction, but this patch records currentThreadId without Slack resolver target and reply-mode checks. The safer fix is to mirror the actual outbound threading decision before recording dedupe evidence.

Full review comments:

  • [P1] Record only the thread the send resolver actually used — src/agents/embedded-agent-subscribe.tools.ts:649
    Here threadImplicit only means the provider has an auto-thread resolver; it does not mean the send inherited the current thread. Slack returns no thread for replyToMode=off, a consumed first reply, or a different target, so recording currentThreadId here can make a top-level send look thread-scoped and suppress a later valid threaded reply.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 231b5a14d5d7.

Label changes

Label justifications:

  • P2: The PR targets a real Slack reply-suppression bug with limited channel blast radius, but it is not an emergency or proven urgent regression.
  • merge-risk: 🚨 message-delivery: The patch changes reply-dedupe route evidence that decides whether final or follow-up Slack messages are delivered or suppressed.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • 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 lists tests and typecheck, but no after-fix real Slack proof; the contributor should add a redacted screenshot, recording, terminal output, copied live output, linked artifact, or logs, then update the PR body to trigger re-review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +24, Tests +163. Total +187 across 11 files.

View PR surface stats
Area Files Added Removed Net
Source 7 27 3 +24
Tests 4 163 0 +163
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 11 190 3 +187

What I checked:

Likely related people:

  • vincentkoc: Local blame on the current dedupe/extraction blocks points to a recent Vincent Koc commit, and GitHub path history shows several recent reply-dedupe and direct-delivery commits in the same files. (role: recent area contributor; confidence: high; commits: 2d4a9eb40538, 642e1dfcdfb1, 16b0a6202c69; files: src/auto-reply/reply/reply-payloads-dedupe.ts, src/agents/embedded-agent-subscribe.tools.ts, src/auto-reply/reply/agent-runner.ts)
  • steipete: GitHub history shows earlier route-dedupe, final-reply scoping, route helper, and follow-up delivery extraction work in the central reply-routing files. (role: feature owner; confidence: high; commits: 72f6016ce596, eb66def65604, e27c32b9b0d9; files: src/auto-reply/reply/reply-payloads-dedupe.ts, src/auto-reply/reply/followup-delivery.ts, src/plugin-sdk/channel-route.ts)
  • TurboTheTurtle: Recent Slack reply transcript persistence touched the same dedupe and follow-up delivery files adjacent to this visible Slack reply path. (role: recent adjacent Slack reply contributor; confidence: medium; commits: 7c088045413f; files: src/auto-reply/reply/reply-payloads-dedupe.ts, src/auto-reply/reply/followup-delivery.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.

@clawsweeper clawsweeper Bot added the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label Jun 3, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 4, 2026
Closed
@steipete steipete self-assigned this Jun 13, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 14, 2026
@steipete steipete mentioned this pull request Jun 14, 2026
Merged
@steipete

steipete commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Superseded by #90943, now landed as c67dc59.

The landed fix includes this PR's route-aware Slack thread dedupe and extends it across queued follow-ups, delivery receipts, queue restart/overflow handling, and the other affected delivery paths. Thank you, @ffluk3, for isolating the threaded-route failure and providing the focused regression coverage.

@steipete steipete closed this Jun 14, 2026
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

agents Agent runtime and tooling merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Threaded Slack messaging-tool sends can suppress later automatic replies

2 participants

@ffluk3 @steipete