Skip to content

fix(discord): allow marked room_event final fallback delivery#86672

Closed
anyech wants to merge 2 commits into
openclaw:mainfrom
anyech:fix/discord-room-event-final-visible-fallback-84022-fresh
Closed

fix(discord): allow marked room_event final fallback delivery#86672
anyech wants to merge 2 commits into
openclaw:mainfrom
anyech:fix/discord-room-event-final-visible-fallback-84022-fresh

Conversation

@anyech

@anyech anyech commented May 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #84022 by allowing Discord room_event final-visible fallback/failure payloads to be delivered when runtime code explicitly marks them with deliverDespiteSourceReplySuppression.

The change keeps the ambient room-event privacy guard intact:

  • unmarked room_event final payloads still remain private under source-reply suppression;
  • sendPolicy: deny still suppresses marked fallback delivery;
  • empty/whitespace payloads stay private;
  • exact NO_REPLY / silent-envelope payloads stay private;
  • successful message-tool / side-effect sends are still filtered upstream by the existing payload construction/dedupe path.

Why

room_event dispatch currently has an absolute suppression check, so even runtime-marked final fallback/failure notices cannot reach sendFinalReply. That means a user-visible final can be lost on Discord room-event/message-tool-only surfaces when the runtime deliberately prepared a visible fallback.

#84328 is only partial/unmerged for this case because it does not cover the room-event dispatch suppressor.

Real behavior proof

  • Behavior or issue addressed: Discord room_event final-visible fallback/failure payloads that runtime code explicitly marks with deliverDespiteSourceReplySuppression should reach final reply dispatch, while ordinary ambient room_event content, sendPolicy: deny, empty payloads, exact NO_REPLY, and silent envelopes stay suppressed.

  • Real environment tested: Disposable OpenClaw source checkout using this PR branch on Linux with Node 22. The proof exercised the actual dispatchReplyFromConfig delivery gate with a fake dispatcher and synthetic Discord room_event inputs. No production Gateway, live runtime state, live config, or real Discord channel was used.

  • Exact steps or command run after this patch:

    corepack pnpm install --offline --ignore-scripts --frozen-lockfile
    corepack pnpm exec vitest run src/auto-reply/reply/dispatch-from-config.test.ts \
      --testNamePattern "marked runtime fallback|marked room-event runtime fallback|marked empty room-event|marked NO_REPLY room-event|ambient room-event" \
      --reporter=dot --pool=forks --maxWorkers=1
    ./node_modules/.bin/vitest run src/auto-reply/reply/dispatch-from-config.test.ts \
      -t "marked room-event|sendPolicy denies delivery|marked empty room-event|marked NO_REPLY room-event|ambient room-event" \
      --reporter=verbose
  • Evidence after fix: Copied terminal output from the disposable checkout showed:

    Test Files  1 passed (1)
    Tests       5 passed | 109 skipped (114)
    
    Targeted non-live dispatch-path validation at 9dec5405a5c6ea5fc2f1811692f26e9e2a3f6eeb:
    Test Files 1 passed (1)
    Tests 6 passed | 150 skipped (156)
    
  • Observed result after fix: The patched dispatch path delivered the explicitly marked Discord room_event fallback exactly once when sendPolicy allowed it, while unmarked ambient room_event finals, empty marked fallbacks, exact NO_REPLY marked fallbacks, and sendPolicy: deny remained private.

  • What was not tested: No production Gateway, live runtime state, live config, or real Discord transport was exercised. This is targeted non-live source-level proof for the final-delivery gate changed by this PR; maintainer/Mantis live proof would still be stronger if required for real transport evidence.

Reviewer / AI review map

This PR now intentionally supersedes #86634 so reviewers only need to evaluate one dispatch-from-config branch for this overlapping seam.

  • Combined scope: two narrow final/block delivery guards in the same dispatcher file:
    1. sanitize streamed/block reply previews before queued/progress, TTS, routed, or dispatcher block delivery;
    2. allow explicitly marked Discord room_event fallback/failure finals to bypass source-reply suppression when sendPolicy allows it.
  • Shared files: src/auto-reply/reply/dispatch-from-config.ts and src/auto-reply/reply/dispatch-from-config.test.ts only.
  • Non-overlap within the combined PR: the sanitizer path normalizes block reply text before callback/delivery use; the Discord room_event change is a later final-delivery suppression exception guarded by provider, marker, non-silent content, and sendPolicy.
  • Privacy/silence boundaries preserved: empty sanitized blocks drop; exact NO_REPLY and silent envelopes remain suppressed; sendPolicy: deny still wins; unmarked ambient room_event finals remain private.
  • Why combine: fix: sanitize block reply previews #86634 and this PR were the only authored open PRs with direct file overlap. Combining them reduces queue pressure and removes one cross-PR conflict without broadening beyond the existing dispatcher seam.
  • What not to review here: unrelated Discord queue behavior, native subagent completion ownership, session replay repair, or broader auto-reply refactors.

Combined validation update

After combining #86634 into this branch in a disposable checkout, targeted non-live validation passed:

openclaw-heavy-run --backend auto -- \
  bash -lc "node <pnpm.mjs> install --offline --ignore-scripts --frozen-lockfile && \
  node <pnpm.mjs> exec vitest run src/auto-reply/reply/dispatch-from-config.test.ts \
    -t 'sanitize|marked room-event|marked runtime fallback|sendPolicy denies delivery|ambient room-event|NO_REPLY room-event' \
    --reporter=dot --pool=forks --maxWorkers=1"

Test Files 1 passed (1)
Tests 7 passed | 154 skipped (161)

git diff --check origin/main...HEAD
# rc=0

This validation is intentionally source-level and non-production; no live Gateway, runtime state, live config, or real Discord transport was exercised.

Risk / notes

  • Scope is intentionally narrow: dispatch-from-config final delivery gating plus focused unit coverage.
  • No old thread missing finals are supplemented by this patch.
  • No live runtime or production Discord validation was performed.
  • CI should still run the full project matrix.

Related

@clawsweeper

clawsweeper Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed May 30, 2026, 4:15 AM ET / 08:15 UTC.

Summary
The PR widens dispatchReplyFromConfig final-delivery suppression for marked Discord room_event fallback payloads and adds block-reply sanitization before queued or dispatched previews.

PR surface: Source +17, Tests +219. Total +236 across 2 files.

Reproducibility: yes. source inspection shows current main suppresses marked non-command room_event final replies under message-tool-only delivery, and the linked issue reports transcript-only Discord finals. I did not run a live Discord reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Delivery Gate Changed: 1 source-reply suppression exception widened. The diff changes when suppressed final replies can become visible in Discord room_event handling, which maintainers should review before merge.

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 the sanitizer import to the existing embedded-agent-helpers module path.
  • Rebase and resolve the dirty PR against current main while preserving the explicit-command room_event guard.
  • [P2] Add redacted real Discord/Gateway proof showing marked fallback delivery exactly once and unmarked, silent, empty, and sendPolicy: deny cases staying quiet.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR supplies copied terminal output from fake-dispatcher source tests, but no real Discord/Gateway after-fix proof; contributor action is needed with redacted logs, terminal/live output, screenshots, recording, or linked artifact, then an updated PR body for re-review. 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 short real Discord/Gateway proof would materially improve confidence because the PR changes visible transport delivery, and no more specific Discord Mantis lane fits this exact fallback case. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

visual task: prove a real Discord/Gateway room_event marked fallback delivers exactly once while unmarked, silent, empty, and sendPolicy-deny cases stay quiet.

Risk before merge

  • [P2] The PR is dirty against current main, and conflict resolution must preserve the newer explicit-command room_event bypass while adding any Discord-specific marked-fallback exception.
  • [P2] This changes a source-reply suppression gate; a bad merge can either keep dropping the marked fallback or make ambient Discord room_event finals visible when they should remain private.
  • [P2] The supplied proof is source-level validation with a fake dispatcher, not a real Discord/Gateway proof that the marked fallback visibly delivers exactly once and suppressor cases stay quiet.

Maintainer options:

  1. Repair, Rebase, And Prove (recommended)
    Resolve the dirty base, fix the sanitizer import, preserve the current explicit-command guard, and add redacted real Discord/Gateway proof before merge.
  2. Accept Source-Level Proof Only
    Maintainers may choose to accept the fake-dispatcher proof, but that leaves the real transport path and exact-once visible delivery unproven.
  3. Pause For Policy Direction
    If the desired Discord room_event fallback contract is still unsettled, pause this PR and keep the linked issue as the product-decision thread.

Next step before merge

  • [P1] Human review is needed because the PR is dirty against main, needs a branch rebase, has a blocking import defect, and still needs real Discord/Gateway proof that automation cannot supply for the contributor.

Security
Cleared: No supply-chain, dependency, workflow, credential, or secrets-handling change was found; the main safety concern is message-delivery/privacy behavior, covered as merge risk.

Review findings

  • [P1] Import the existing sanitizer module — src/auto-reply/reply/dispatch-from-config.ts:13
Review details

Best possible solution:

Rebase onto current main, fix the sanitizer import, merge the Discord marked-fallback logic with the current explicit-command guard, then provide focused regression coverage plus redacted real Discord/Gateway proof before merge.

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

Yes, source inspection shows current main suppresses marked non-command room_event final replies under message-tool-only delivery, and the linked issue reports transcript-only Discord finals. I did not run a live Discord reproduction in this read-only review.

Is this the best way to solve the issue?

No, the submitted branch is not yet the best merge shape because it is dirty against current main and imports a non-existent sanitizer path. The right solution is the narrow Discord marked-fallback exception after a rebase that preserves current explicit-command behavior.

Full review comments:

  • [P1] Import the existing sanitizer module — src/auto-reply/reply/dispatch-from-config.ts:13
    This new import points at ../../agents/pi-embedded-helpers/sanitize-user-facing-text.js, but current main has no pi-embedded-helpers module; existing callers import sanitizeUserFacingText from ../../agents/embedded-agent-helpers/sanitize-user-facing-text.js. As written, the patched file will fail module resolution during TypeScript/build validation.
    Confidence: 0.92

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🧂 unranked krab, so this older rating label is no longer current.

Label justifications:

  • P2: This is a normal-priority Discord message-delivery fix with limited scope, but it is not merge-ready due to a compile-time import issue, dirty base, and missing live proof.
  • merge-risk: 🚨 message-delivery: The PR modifies final-reply suppression logic, so a mistake can drop marked fallback messages or expose room_event replies that should stay private.
  • 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 supplies copied terminal output from fake-dispatcher source tests, but no real Discord/Gateway after-fix proof; contributor action is needed with redacted logs, terminal/live output, screenshots, recording, or linked artifact, then an updated PR body for re-review. 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 +17, Tests +219. Total +236 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 26 9 +17
Tests 1 222 3 +219
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 248 12 +236

What I checked:

Likely related people:

  • openperf: GitHub commit history shows fb6f2c61bf91efded1a88e184ce5a294726de6bf recently restored explicit command replies in room-event/message-tool-only delivery, the same guard this PR now needs to rebase over. (role: recent area contributor; confidence: high; commits: fb6f2c61bf91; files: src/auto-reply/reply/dispatch-from-config.ts, src/auto-reply/reply/dispatch-from-config.test.ts)
  • steipete: Local blame on the current shallow checkout attributes the active dispatch guard region to c806a736affbc1f885067c8103d322807391b694, and GitHub history shows multiple recent dispatch/Discord fallback commits by this person. (role: recent adjacent contributor; confidence: medium; commits: c806a736affb, 0e262d20e749, da279041aba2; files: src/auto-reply/reply/dispatch-from-config.ts, extensions/discord/src/monitor/message-handler.process.ts)
  • anyech: Beyond authoring this PR, GitHub history shows prior merged Discord reply-delivery work in 57da466ecbb5f63b5a1881ae8caa7359c02ec40c, so this account has relevant area history on current main. (role: prior merged adjacent contributor; confidence: medium; commits: 57da466ecbb5; files: src/auto-reply/reply/dispatch-from-config.ts, extensions/discord/src/monitor/message-handler.process.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 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. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels May 25, 2026
@clawsweeper

clawsweeper Bot commented May 25, 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.

@anyech

anyech commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

I added the strongest proof I can provide from this contributor side without touching live Gateway/Discord state.

Non-live dispatch-path validation

Checkout: 9dec5405a5c6ea5fc2f1811692f26e9e2a3f6eeb

Command run locally in the patched checkout:

./node_modules/.bin/vitest run src/auto-reply/reply/dispatch-from-config.test.ts \
  -t "marked room-event|sendPolicy denies delivery|marked empty room-event|marked NO_REPLY room-event|ambient room-event" \
  --reporter=verbose

Result:

Test Files  1 passed (1)
Tests       6 passed | 150 skipped (156)

Covered cases through the actual dispatchReplyFromConfig path with a fake dispatcher:

  • marked Discord room_event runtime fallback payload with deliverDespiteSourceReplySuppression: true and sendPolicy: allow delivers exactly once (sendFinalReply called once; no block/tool result delivery)
  • marked runtime failure notice with sendPolicy: deny stays suppressed
  • marked Discord room_event runtime fallback with sendPolicy: deny stays suppressed
  • marked Discord room_event fallback with empty text stays suppressed
  • marked Discord room_event fallback with NO_REPLY text stays suppressed
  • ordinary/unmarked ambient Discord room_event final without a message-tool send stays suppressed

Caveat: this is isolated source-checkout validation through the real dispatch path, not live Discord/Gateway proof. It therefore should be treated as a non-live harness result, not as evidence that a live Discord transport received exactly one visible message.

Live proof request

@openclaw-mantis Could you run or prepare a live Discord/Gateway room_event final-visible proof for this PR? Desired scenario:

  1. marked Discord room_event runtime fallback payload with deliverDespiteSourceReplySuppression: true visibly delivers exactly once;
  2. unmarked ambient room_event finals remain suppressed;
  3. silent/empty/NO_REPLY fallback payloads remain suppressed;
  4. sendPolicy: deny remains a hard delivery suppression gate.

I did not run this against my live Gateway/Discord state because that would use private production runtime/config rather than a maintainer-controlled public QA surface.

Secondary note: I also see checks-node-agentic-agents failing, but I cannot inspect those job logs with the current token. It looks unrelated to the dispatch/final-visible change unless the logs show otherwise.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels May 26, 2026
@anyech
anyech force-pushed the fix/discord-room-event-final-visible-fallback-84022-fresh branch from 9dec540 to aa2066a Compare May 26, 2026 02:01
@anyech

anyech commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

I pushed a follow-up update to narrow the exception to Discord room_event only, addressing the cross-channel policy concern from the latest review.

Updated commit: aa2066a450b32b4f9106408449f343573839762d

What changed:

  • marked fallback delivery still requires deliverDespiteSourceReplySuppression: true, visible non-silent content, and sendPolicy not denied;
  • the new room-event exception is now limited to Provider === "discord";
  • added a guard test that a non-Discord marked room_event fallback remains suppressed.

Validation note: the original focused dispatch validation passed before this scope-narrowing update. After rebasing onto latest main, a local rerun was blocked because the fresh checkout now depends on [email protected], which is not present in the local offline pnpm store. I did not switch to online dependency download for this follow-up. CI should run the updated focused and full matrix against the refreshed branch.

This still does not claim live Discord/Gateway proof; the Mantis/maintainer live-proof request remains the next proof step.

@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 26, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels May 26, 2026
@BingqingLyu

This comment was marked as spam.

@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed 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. labels May 29, 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. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels May 29, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels May 30, 2026
@steipete

Copy link
Copy Markdown
Contributor

Closing this PR, but keeping #84022 open.

This patch is no longer the right shape for the remaining Discord room-event/thread-bound delivery problem. The branch is stale, imports a helper path that does not exist on current main, mixes an unrelated sanitizer change into the delivery fix, and changes a sensitive room_event suppression gate without covering the newer ACP/thread-bound repro shape discussed on #84022.

The next fix should be a fresh, narrower patch for plugin-owned/bound thread replies and ACP completion delivery, with #84022 as the canonical issue. Thanks for the work here; the issue itself is still real.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. proof: supplied External PR includes structured after-fix real behavior proof. 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.

Discord room events can suppress final assistant replies into transcript-only mode

4 participants