Skip to content

[codex] Honor all ack scope for room events#87433

Merged
steipete merged 5 commits into
openclaw:mainfrom
scoootscooob:codex/fix-discord-room-event-ack
Jul 6, 2026
Merged

[codex] Honor all ack scope for room events#87433
steipete merged 5 commits into
openclaw:mainfrom
scoootscooob:codex/fix-discord-room-event-ack

Conversation

@scoootscooob

@scoootscooob scoootscooob commented May 27, 2026

Copy link
Copy Markdown
Member

What Problem This Solves

messages.ackReactionScope: "all" promised an acknowledgment for every message, but ambient room_event input stayed reaction-silent on Discord, Slack, and Telegram.

Fixes #87368.

Why This Change Was Made

The shared ack-reaction gate now owns the cross-channel rule: ambient room events may receive one plain acknowledgment only when the configured scope is explicitly "all". Discord, Slack, and Telegram pass their prepared event kind through that gate.

The maintainer repair also keeps typing and lifecycle status reactions suppressed for room events. Slack dispatch has a guard at the controller boundary so the newly permitted plain ack cannot accidentally start progress reactions.

User Impact

  • ackReactionScope: "all": ambient room events receive the configured initial ack reaction.
  • group-all, group-mentions, direct, and off: ambient room events remain reaction-silent.
  • Room events never produce typing, processing, tool, done, or error status reactions.

Docs now describe the same behavior for Discord, Slack, Telegram, and the ambient room-event contract.

Evidence

Live Discord, Slack, and Telegram delivery was not captured. Exact-head Mantis run https://github.com/openclaw/openclaw/actions/runs/28759215102 built baseline and candidate successfully, but the Crabbox coordinator rejected both baseline capture attempts at its active-lease limit before Telegram Desktop started; this is a proof-capacity gap, not a behavior verdict.

@openclaw-barnacle openclaw-barnacle Bot added channel: discord Channel integration: discord size: XS maintainer Maintainer-authored PR labels May 27, 2026
@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 1:48 PM ET / 17:48 UTC.

Summary
The PR changes Discord, Slack, and Telegram ack-reaction logic, tests, channel docs, and changelog text so messages.ackReactionScope: "all" can emit initial ack reactions for ambient room_event messages while lifecycle status reactions are intended to remain suppressed.

PR surface: Source +11, Tests +133, Docs +9. Total +153 across 10 files.

Reproducibility: yes. Source inspection shows current main and v2026.6.10 still gate room-event acks before ackReactionScope: "all" can apply; I did not run a live transport repro in this read-only review.

Review metrics: 1 noteworthy metric.

  • Ack scope semantics: 1 existing setting behavior changed. messages.ackReactionScope: "all" would gain visible ambient-room ack behavior across three chat plugins, so maintainers should accept the upgrade behavior before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #87368
Summary: This PR is the active candidate fix for the canonical ackReactionScope room-event issue; codegraph-overlap PRs touch adjacent handlers but solve different user problems.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
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:

  • [P1] Fix Slack so room-event all sends only the intended plain ack and never lifecycle status reactions.
  • [P1] Add redacted real Discord, Slack, and Telegram proof for the changed ack behavior; redact private information such as API keys, phone numbers, IPs, and non-public endpoints.
  • [P1] Get maintainer acceptance for the visible ambient-room ack behavior before merge.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists focused unit/format/docs checks and explicitly says live Discord/Slack/Telegram delivery was not tested, so contributor-supplied redacted screenshots, recordings, terminal/live output, artifacts, or logs are still needed before merge. 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 native Telegram UI recording would directly show the visible room-event ack reaction and whether lifecycle status reactions stay suppressed. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis telegram desktop proof: verify messages.ackReactionScope=all sends an ack reaction for an unmentioned Telegram room_event while status reactions stay suppressed.

Risk before merge

  • [P1] Existing users with messages.ackReactionScope: "all" and ambient room-event listening will start seeing visible ack reactions in Discord, Slack, and Telegram rooms that were previously reaction-silent.
  • [P1] Slack currently still turns the PR's intended plain room-event ack into lifecycle status reactions during dispatch because dispatch keys lifecycle enablement off ackReactionPromise.
  • [P1] The proof is only focused tests and local checks; no redacted live Discord, Slack, or Telegram transport output shows the visible after-fix behavior.

Maintainer options:

  1. Fix Slack and require live proof (recommended)
    Before merge, keep Slack room-event acks out of lifecycle status reactions and add redacted live or artifact-backed Discord, Slack, and Telegram proof for the changed behavior.
  2. Accept the visible behavior change
    Maintainers can explicitly accept that existing all configurations will begin acknowledging quiet room events after the Slack bug and proof gap are resolved.
  3. Pause if room silence should remain
    If ambient room events must stay reaction-silent even under all, pause or close this PR and clarify the docs/schema instead of changing runtime delivery.

Next step before merge

  • [P1] Manual maintainer/contributor handling is needed: the PR has a protected maintainer label, a Slack lifecycle correctness blocker, missing live transport proof, and a visible behavior decision.

Security
Cleared: The diff changes channel reaction logic, tests, docs, and changelog text; no dependency, workflow, secret, or supply-chain surface changed.

Review findings

  • [P1] Keep Slack room events out of lifecycle status reactions — extensions/slack/src/monitor/message-handler/prepare.ts:1079
Review details

Best possible solution:

Carry a plain-ack versus lifecycle-status decision through Slack dispatch, then land this PR or a replacement only after maintainers accept the ambient-room all semantics and live Discord/Slack/Telegram proof shows only the intended ack appears for room events.

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

Yes. Source inspection shows current main and v2026.6.10 still gate room-event acks before ackReactionScope: "all" can apply; I did not run a live transport repro in this read-only review.

Is this the best way to solve the issue?

No. Honoring all across sibling chat plugins is plausible, but this patch is not the best fix until Slack dispatch preserves the plain-ack-only room-event path and live proof covers the visible transport behavior.

Full review comments:

  • [P1] Keep Slack room events out of lifecycle status reactions — extensions/slack/src/monitor/message-handler/prepare.ts:1079
    This makes room-event all produce a prepared ack promise, but dispatchPreparedSlackMessage still enables the status-reaction controller for any ack promise plus timestamp. As a result Slack can emit queued/thinking/tool/done lifecycle reactions for ambient room events even though the PR says only the plain ack should be sent; carry an explicit lifecycle-status flag or have dispatch also reject room_event.
    Confidence: 0.91

Overall correctness: patch is incorrect
Overall confidence: 0.92

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5f60724b6d7b.

Label changes

Label justifications:

  • P2: This is a normal-priority cross-channel reaction behavior fix with limited blast radius, not a core outage.
  • merge-risk: 🚨 compatibility: Existing ambient-room users with ackReactionScope: "all" may see new visible reactions after upgrade.
  • merge-risk: 🚨 message-delivery: The diff changes whether Discord, Slack, and Telegram emit visible ack and lifecycle reaction messages for quiet room events.
  • 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 focused unit/format/docs checks and explicitly says live Discord/Slack/Telegram delivery was not tested, so contributor-supplied redacted screenshots, recordings, terminal/live output, artifacts, or logs are still needed before merge. 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: telegram-visible-proof: Mantis should capture Telegram visible proof. The PR changes visible Telegram room-event ack reaction behavior, which can be shown in a short Telegram Desktop recording.
Evidence reviewed

PR surface:

Source +11, Tests +133, Docs +9. Total +153 across 10 files.

View PR surface stats
Area Files Added Removed Net
Source 3 16 5 +11
Tests 3 139 6 +133
Docs 4 13 4 +9
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 10 168 15 +153

What I checked:

Likely related people:

  • steipete: Peter Steinberger authored the shared ack-reaction gate that defines scope: "all" semantics. (role: historical ack-reaction owner; confidence: high; commits: 02bd6e4a249d; files: src/channels/ack-reactions.ts)
  • Hsiao A: The Slack lifecycle status-reaction behavior that this PR must keep separate from plain acks was introduced in the Slack status-reaction lifecycle commit. (role: Slack status-reaction feature contributor; confidence: medium; commits: cea7162490f4; files: extensions/slack/src/monitor/message-handler/dispatch.ts, extensions/slack/src/monitor/message-handler/prepare.ts, src/channels/status-reactions.ts)
  • lizeyu-xydt: Recent merged work touched Discord and Slack status-reaction behavior adjacent to the plain-ack versus lifecycle boundary. (role: recent status-reaction contributor; confidence: medium; commits: 5afddf547ed7; files: extensions/discord/src/monitor/message-handler.process.ts, extensions/slack/src/monitor/message-handler/dispatch.ts)
  • scoootscooob: This author previously moved the affected Discord, Slack, and Telegram channel implementations into extensions/, so they have relevant prior merged history beyond this PR. (role: prior channel mover and candidate fix author; confidence: medium; commits: 5682ec37fada, 8746362f5ebf, e5bca0832fbd; files: extensions/discord/src/monitor/message-handler.process.ts, extensions/slack/src/monitor/message-handler/prepare.ts, extensions/telegram/src/bot-message-context.ts)
  • Alix-007: Current line blame points to a recent broad commit that reintroduced the current files wholesale, though the behavioral ownership is better explained by older feature history. (role: recent current-main importer; confidence: low; commits: f0e2f7b4f5ae; files: extensions/discord/src/monitor/message-handler.process.ts, extensions/slack/src/monitor/message-handler/prepare.ts, 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.
Review history (1 earlier review cycle)
  • reviewed 2026-06-21T06:29:02.023Z sha 9b80983 :: needs real behavior proof before merge. :: [P1] Keep Slack room events out of lifecycle status reactions

@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: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. 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.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation channel: slack Channel integration: slack channel: telegram Channel integration: telegram size: S and removed size: XS labels May 28, 2026
@scoootscooob scoootscooob changed the title [codex] Honor Discord all ack scope for room events [codex] Honor all ack scope for room events May 28, 2026
@clawsweeper clawsweeper Bot added the mantis: telegram-visible-proof Mantis should capture Telegram visible proof. label May 28, 2026
@BingqingLyu

This comment was marked as spam.

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. label May 29, 2026
@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed 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. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. labels May 29, 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. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 15, 2026
@steipete
steipete force-pushed the codex/fix-discord-room-event-ack branch from 9b80983 to 1b66ffb Compare July 5, 2026 23:50
@steipete
steipete marked this pull request as ready for review July 5, 2026 23:51
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

@openclaw-mantis telegram desktop proof: verify messages.ackReactionScope=all sends an ack reaction for an unmentioned Telegram room_event while status reactions stay suppressed.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1b66ffbe9f

ℹ️ 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".

const incomingThreadTs = message.thread_ts;
let didSetStatus = false;
const statusReactionsEnabled =
prepared.ctxPayload.InboundEventKind !== "room_event" &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Badge Suppress Slack typing for room events too

For Slack ambient room events, this only gates lifecycle reactions; dispatch still sends the turn without replyOptions.suppressTyping. Core maps sourceReplyDeliveryMode: "message_tool_only" to instant typing unless suppressTyping is true (src/auto-reply/reply/typing-mode.ts:33-46), and Slack's typing callback posts setSlackThreadStatus("is typing..."), so an unmentioned room event can still show visible typing even though this change documents room events as quiet except for the explicit ack. Discord and Telegram both pass explicit room-event typing suppression; Slack needs the same guard in its dispatch options.

Useful? React with 👍 / 👎.

@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Land-ready maintainer repair at exact head 7552e1418f6a3db2f66de77fd2081f8da0e14ce6:

  • Centralized ambient room_event ack policy in src/channels/ack-reactions.ts: only explicit ackReactionScope: "all" permits one plain ack.
  • Kept Discord, Slack, and Telegram typing/lifecycle status reactions suppressed for room events; Slack now guards the actual controller start boundary.
  • Added focused shared and per-channel regression coverage plus aligned channel/ambient-event docs.
  • Focused Testbox tbx_01kwta2h5ek3jktr1tnt6r2vk7: 326 tests passed across shared channels, Discord, Slack, and Telegram.
  • Broad Testbox tbx_01kwta6nhdqc138c9g5af2zevv: guards, all four typecheck lanes, and full core lint passed; its one new-test Promise-executor lint finding was fixed.
  • Post-fix Testboxes: tbx_01kwtaw92bdxkvybxfm901827v (111 Discord tests), tbx_01kwtaw6npt6mdyecvkayrjmxj (targeted extension lint), tbx_01kwtaw8tfg3jtcjffcavrkz2p (692-doc MDX check), and tbx_01kwtawbr2kdj80f34yctcpb0c (14-file format check) all passed.
  • Fresh post-fix Codex branch autoreview: no accepted/actionable findings, 0.86 confidence.
  • Exact-head hosted CI: https://github.com/openclaw/openclaw/actions/runs/28759907942
  • Native Telegram Mantis attempt on the equivalent pre-sync patch: https://github.com/openclaw/openclaw/actions/runs/28759215102. Baseline and candidate builds passed, but native capture could not start because the Crabbox coordinator rejected both baseline attempts at the active-lease limit; this produced no behavior verdict.

Known proof gap: live Discord, Slack, and Telegram workspace delivery were not captured. The shared gate, per-channel unit coverage, focused Testbox proof, and exact-head hosted CI cover the changed invariant; Mantis's failure was capture capacity, not a candidate behavior failure.

@steipete
steipete force-pushed the codex/fix-discord-room-event-ack branch from 1b66ffb to 7552e14 Compare July 6, 2026 00:15
@steipete
steipete merged commit 7e7fc00 into openclaw:main Jul 6, 2026
102 checks passed
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 6, 2026
* fix(discord): honor all ack scope for room events

* fix(channels): align all ack scope for room events

* fix(channels): centralize ambient ack scope

* test(telegram): restore room-event ack fixture

* test(discord): satisfy promise executor lint

---------

Co-authored-by: Peter Steinberger <[email protected]>
giodl73-repo pushed a commit to giodl73-repo/openclaw that referenced this pull request Jul 8, 2026
* fix(discord): honor all ack scope for room events

* fix(channels): align all ack scope for room events

* fix(channels): centralize ambient ack scope

* test(telegram): restore room-event ack fixture

* test(discord): satisfy promise executor lint

---------

Co-authored-by: Peter Steinberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: discord Channel integration: discord channel: slack Channel integration: slack channel: telegram Channel integration: telegram docs Improvements or additions to documentation maintainer Maintainer-authored PR mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. 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: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ackReactionScope: "all" is silently overridden by isRoomEvent gate

4 participants