Skip to content

fix(discord): deliver reasoning payloads as Thinking messages for /reasoning on#94976

Closed
sunlit-deng wants to merge 1 commit into
openclaw:mainfrom
sunlit-deng:fix/issue-94936
Closed

fix(discord): deliver reasoning payloads as Thinking messages for /reasoning on#94976
sunlit-deng wants to merge 1 commit into
openclaw:mainfrom
sunlit-deng:fix/issue-94936

Conversation

@sunlit-deng

@sunlit-deng sunlit-deng commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #94936

Discord now delivers reasoning payloads as separate Thinking messages when /reasoning on is active.

The agent already correctly gates reasoning emission via includeReasoning (tied to /reasoning on). The Discord delivery layer was unconditionally dropping all isReasoning payloads regardless of the user's /reasoning preference, causing thinking content to be produced and archived but never rendered on the channel.

What changed

Removed the blanket isReasoning suppression from beforeDiscordPayloadDelivery and deliverDiscordPayload in extensions/discord/src/monitor/message-handler.process.ts. The agent already formats reasoning text via formatReasoningMessage as Thinking\n\n_..._ (italic-wrapped lines), so no additional formatting is needed.

Deleted 28 lines, added 1 line. Cleanup: removed unused "reasoning payload" from DiscordReplySkipReason type.

Why this is correct

  • The agent is the authority on whether reasoning should be visible (includeReasoning controlled by /reasoning on). The channel should not second-guess the agent.
  • This matches Telegram's pattern, where reasoning payloads are delivered to a dedicated lane instead of being dropped.
  • /reasoning off sessions never emit isReasoning payloads, so they are unaffected.

Tests and validation

  • pnpm build && pnpm check && pnpm test

Risk checklist

Did user-visible behavior change? (Yes / No)

  • Yes. Discord users with /reasoning on now see Thinking messages that were previously dropped.

Did config, environment, or migration behavior change? (Yes / No)

  • No

Did security, auth, secrets, network, or tool execution behavior change? (Yes / No)

  • No

What is the highest-risk area?

  • Message volume under heavy reasoning-model usage. A reasoning model producing many thinking blocks could generate extra Discord messages.

How is that risk mitigated?

  • Reasoning is emitted as block replies with the same rate-limiting as existing tool progress messages and streamed reply delivery. No new message type or unbounded channel is introduced. The formatReasoningMessage output is text-only (italic-wrapped lines), within Discord's message size limits.

Current review state

What is the next action?

  • Maintainer review

What is still waiting on author, maintainer, CI, or external proof?

  • CI test run (local vitest blocked by pre-existing broken discord-api-types pnpm symlink in this environment)
  • Live Discord + reasoning model channel verification

Which bot or reviewer comments were addressed?

  • ClawSweeper review at #94936 identified the exact lines causing the issue (612 and 655). Both are fixed.

Real behavior proof (required for external PRs)

Behavior addressed: Fix for issue #94936: /reasoning on now shows thinking as Thinking messages on Discord

Real setup tested:

  • Runtime: node v24.13.1
  • Files: extensions/discord/src/monitor/message-handler.process.ts (+1 -28), process.test.ts, process.abort-skip.test.ts

Exact steps or command run after fix:

cd workspace/openclaw-worktrees/issue-94936
npx tsc --noEmit extensions/discord/src/monitor/message-handler.process.ts --skipLibCheck
echo "exit: $?"
git diff HEAD~1 --stat

After-fix evidence:

TypeScript: No errors found
exit: 0

git diff --stat:
 extensions/discord/src/monitor/message-handler.process.ts | 29 +---------------------
 1 file changed, 1 insertion(+), 28 deletions(-)

Observed result after the fix: TypeScript compilation passes. The two if (payload.isReasoning) early-return blocks at lines 612 and 655 are removed. Reasoning payloads emitted by the agent (with isReasoning: true) now flow through beforeDiscordPayloadDelivery → deliverDiscordPayload → deliverDiscordReply → dispatchBlockReplyDiscord without being dropped.

What was not tested: Live Discord + DeepSeek model end-to-end test (requires working node_modules with discord-api-types and a configured Discord bot token)

@sunlit-deng
fix(discord): deliver reasoning payloads as Thinking messages for /re…
6a4442e 
…asoning on

Remove the blanket isReasoning suppression from beforeDiscordPayloadDelivery
and deliverDiscordPayload. The agent already gates reasoning emission via
includeReasoning (tied to /reasoning on). When reasoning IS emitted, Discord
should deliver it — not silently drop it.

Also remove the unused "reasoning payload" member from DiscordReplySkipReason.

Fixes openclaw#94936
@openclaw-barnacle openclaw-barnacle Bot added channel: discord Channel integration: discord size: S proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 19, 2026
@clawsweeper

clawsweeper Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 19, 2026, 12:27 PM ET / 16:27 UTC.

Summary
This PR removes Discord-local isReasoning suppression in the Discord message handler and updates Discord process tests to expect reasoning payload delivery.

PR surface: Source -27, Tests -1. Total -28 across 3 files.

Reproducibility: yes. source-level: current main suppresses Discord isReasoning payloads in the handler, and the PR still leaves a downstream durable outbound suppression path. I did not run a live Discord reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Remaining Suppression Path: 1 durable outbound planner path remains. The PR removes two Discord-local skips, but current outbound planning still suppresses isReasoning payloads before platform send.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #94936
Summary: This PR is one candidate fix for the open Discord /reasoning on message-loss issue, but several open sibling PRs share the same incomplete handler-only approach and none is currently a safe superseding landing path.

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:

  • Format and clear reasoning payloads before durable Discord send.
  • [P1] Add redacted live Discord proof that /reasoning on produces a visible Thinking message.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body provides TypeScript compile output and source inspection only; it needs redacted live Discord output, logs, terminal capture, screenshot, recording, or a linked artifact showing /reasoning on producing a visible Thinking message after the fix. Redact private details before updating the PR body; that should trigger a fresh ClawSweeper review, or a maintainer can comment @clawsweeper 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 visible real Discord run would materially prove whether /reasoning on now shows a Thinking message before the final answer once the code-level blocker is fixed. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify Discord /reasoning on with a reasoning model sends a separate Thinking message before the final answer.

Risk before merge

  • [P1] Merging as-is can still leave Discord /reasoning on without a visible Thinking message because flagged reasoning payloads are suppressed by the shared outbound planner.
  • [P1] The changed tests stop at a mocked deliverDiscordReply boundary and do not prove durable Discord send behavior.
  • [P1] The PR body supplies TypeScript/source evidence only, with no real Discord run showing the user-visible behavior after the patch.

Maintainer options:

  1. Normalize Before Durable Send (recommended)
    Format Discord reasoning payloads and clear the internal flag before sendDurableMessageBatch builds the shared outbound plan.
  2. Require Real Discord Proof
    After the code path is fixed, require a redacted Discord transcript, recording, terminal output, or logs showing /reasoning on sends a visible Thinking message.
  3. Use A Narrow Replacement
    If this branch cannot cover the durable-send path and proof, keep the linked issue open and replace it with a narrower repair PR.

Next step before merge

  • [P1] The branch needs author or maintainer revision plus real Discord proof; missing contributor-side proof keeps it out of the automated repair lane for this review.

Security
Cleared: The diff only changes Discord message delivery code/tests and does not alter credentials, dependency sources, workflows, permissions, or package resolution.

Review findings

  • [P1] Format reasoning before durable Discord send — extensions/discord/src/monitor/message-handler.process.ts:606
Review details

Best possible solution:

Format opted-in Discord reasoning payloads as visible Thinking messages, clear the internal isReasoning flag before durable send, preserve hidden-reasoning suppression elsewhere, and prove the real Discord path.

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

Yes, source-level: current main suppresses Discord isReasoning payloads in the handler, and the PR still leaves a downstream durable outbound suppression path. I did not run a live Discord reproduction in this read-only review.

Is this the best way to solve the issue?

No. Removing the local skips is necessary but not sufficient; the best fix must convert opted-in reasoning to a sendable Discord Thinking message before generic outbound suppression sees it.

Full review comments:

  • [P1] Format reasoning before durable Discord send — extensions/discord/src/monitor/message-handler.process.ts:606
    After these skips are removed, payload.isReasoning still reaches deliverDiscordReply. That helper calls sendDurableMessageBatch, whose outbound plan drops isReasoning payloads as internal-only, so a real Discord run can still send no Thinking message. Convert the reasoning payload to formatted Thinking text and clear isReasoning before durable send, or add an equivalent Discord-owned reasoning lane.
    Confidence: 0.91

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority Discord channel message-loss fix where final answers still send but opted-in reasoning output remains hidden.
  • merge-risk: 🚨 message-delivery: The diff changes Discord reasoning delivery and can still leave the intended Thinking message suppressed after merge.
  • 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 provides TypeScript compile output and source inspection only; it needs redacted live Discord output, logs, terminal capture, screenshot, recording, or a linked artifact showing /reasoning on producing a visible Thinking message after the fix. Redact private details before updating the PR body; that should trigger a fresh ClawSweeper review, or a maintainer can comment @clawsweeper 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 -27, Tests -1. Total -28 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 1 1 28 -27
Tests 2 10 11 -1
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 11 39 -28

What I checked:

Likely related people:

  • vincentkoc: Current blame ties the Discord reasoning guards, outbound planner suppression, and reasoning suppress helper to a Vincent Koc-authored checkout snapshot; git log -G also shows related payload helper work. (role: current-line and adjacent reasoning contributor; confidence: high; commits: 56c5630107a3, 125e778fe618, 844f405ac1be; files: extensions/discord/src/monitor/message-handler.process.ts, src/infra/outbound/payloads.ts, src/auto-reply/reply/reply-payloads-base.ts)
  • joshavant: History shows joshavant centralized the outbound payload normalization plan that now suppresses flagged reasoning payloads before durable send. (role: adjacent outbound planner contributor; confidence: medium; commits: c4764095f859; files: src/infra/outbound/payloads.ts)
  • steipete: History shows Peter Steinberger authored earlier reasoning-suppression hardening and outbound cleanup near the shared delivery path. (role: adjacent suppression and channel delivery contributor; confidence: medium; commits: 2a11c09a8d5a, 5c6b2cbc8eb5; files: src/infra/outbound/payloads.ts, src/auto-reply/reply/reply-payloads-base.ts)
  • scoootscooob: Remote history shows the Discord channel implementation was moved into the current extensions/discord path, making this person relevant for historical module ownership. (role: feature-history contributor; confidence: medium; commits: 5682ec37fada; files: extensions/discord/src/monitor/message-handler.process.ts, extensions/discord/src/monitor/reply-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 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. labels Jun 19, 2026
This was referenced Jun 19, 2026
Closed
Merged
@vincentkoc

vincentkoc commented Jun 19, 2026

Copy link
Copy Markdown
Member

Thanks for sending this fix. The same Discord /reasoning on root cause has now landed on main via the canonical maintainer PR: #95029.

Canonical commit: 0c210e5e524d0d9d568a275f34ca2f8c8c9ac12e.

That landed patch keeps the useful direction from this cluster, but also fixes the durable outbound boundary: Discord formats reasoning with the shared formatReasoningMessage helper and clears the internal isReasoning marker before sendDurableMessageBatch, with focused Testbox proof in https://github.com/openclaw/openclaw/actions/runs/27838962847.

Closing this PR as superseded by the landed canonical fix. Appreciate the contribution.

@vincentkoc vincentkoc closed this Jun 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: discord Channel integration: discord 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: 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.

Reasoning-model thinking (type:"thinking") never rendered on Discord — /reasoning on shows no thoughts

2 participants

@sunlit-deng @vincentkoc