[codex] Honor all ack scope for room events#87433
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 1:48 PM ET / 17:48 UTC. Summary 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 Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest 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 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 Is this the best way to solve the issue? No. Honoring Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5f60724b6d7b. Label changesLabel justifications:
Evidence reviewedPR surface: Source +11, Tests +133, Docs +9. Total +153 across 10 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (1 earlier review cycle)
|
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
This comment was marked as spam.
This comment was marked as spam.
9b80983 to
1b66ffb
Compare
|
@openclaw-mantis telegram desktop proof: verify messages.ackReactionScope=all sends an ack reaction for an unmentioned Telegram room_event while status reactions stay suppressed. |
There was a problem hiding this comment.
💡 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" && |
There was a problem hiding this comment.
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 👍 / 👎.
|
Land-ready maintainer repair at exact head
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. |
1b66ffb to
7552e14
Compare
|
Merged via squash.
|
* 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]>
* 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]>
What Problem This Solves
messages.ackReactionScope: "all"promised an acknowledgment for every message, but ambientroom_eventinput 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, andoff: ambient room events remain reaction-silent.Docs now describe the same behavior for Discord, Slack, Telegram, and the ambient room-event contract.
Evidence
tbx_01kwta2h5ek3jktr1tnt6r2vk7: 326 tests passed across the shared channel gate and Discord, Slack, and Telegram handlers.git diff --check: passed.tbx_01kwta6nhdqc138c9g5af2zevv: conflict/dependency guards, all four typecheck lanes, and full core lint passed. It found one Promise-executor lint error in the new Discord test.tbx_01kwtaw92bdxkvybxfm901827v: all 111 Discord handler tests passed.tbx_01kwtaw6npt6mdyecvkayrjmxj: targeted extension lint passed with zero findings.tbx_01kwtaw8tfg3jtcjffcavrkz2p: 692 files passed.tbx_01kwtawbr2kdj80f34yctcpb0c: all 14 changed files passed.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.