Skip to content

fix(discord): preserve tool progress messages#78559

Closed
mingming099 wants to merge 1 commit into
openclaw:mainfrom
mingming099:fix-discord-tool-progress-sanitizer
Closed

fix(discord): preserve tool progress messages#78559
mingming099 wants to merge 1 commit into
openclaw:mainfrom
mingming099:fix-discord-tool-progress-sanitizer

Conversation

@mingming099

@mingming099 mingming099 commented May 6, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

Preserve Discord tool-progress messages (📖 Read:, 🛠️ Exec:, etc.) when the Discord front-channel safety scrubber runs.

The Discord sanitizer intentionally strips internal trace/scaffolding lines from final channel output. However, normal tool-progress deliveries use the same visible labels, so the sanitizer also removed legitimate progress messages before they reached Discord. In practice, 📖 Read: and 🛠️ Exec: disappeared while ✍️ Write: happened to survive because that emoji was not part of the internal-trace regex.

Fix

  • Mark default Discord tool-progress deliveries with channelData.openclawDiscordToolProgress.
  • Let the Discord front-channel sanitizer preserve marked tool-progress text while still applying the generic assistant-visible text sanitizer.
  • Do not treat the marker-only channelData as content by itself if text scrubs empty.
  • Add regression coverage for both the process delivery path and final Discord send boundary.

Validation

  • Live Discord OpenClaw smoke after restart:
    • ✍️ Write visible
    • 📖 Read visible
    • 🛠️ Exec visible
  • git diff --check
  • Focused Discord tests:
pnpm test:extensions -- extensions/discord/src/monitor/reply-delivery.test.ts extensions/discord/src/monitor/message-handler.process.test.ts

Result: the targeted Discord test pass completed successfully (2 passed, 66 passed). The repository test runner then continued into broader extension suites; I stopped that broader run after Discord coverage had passed.

@mingming099 mingming099 mentioned this pull request May 6, 2026
Closed
@openclaw-barnacle openclaw-barnacle Bot added channel: discord Channel integration: discord size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 6, 2026
@clawsweeper

clawsweeper Bot commented May 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge.

Summary
The PR marks Discord tool-kind replies with channelData.openclawDiscordToolProgress, preserves marked text at the Discord final-send sanitizer boundary, treats marker-only channel data as empty content, and adds focused Discord monitor tests.

Reproducibility: yes. as a source-level reproduction: current main sends final Discord replies through the front-channel sanitizer that strips 📖 Read: and 🛠️ Exec: progress-shaped lines. I did not establish a live Discord reproduction in this read-only review.

Real behavior proof
Needs real behavior proof before merge: The PR claims a live Discord smoke, but the contributor still needs to attach redacted after-fix proof such as a screenshot, recording, terminal output, live output, artifact, or log before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Contributor or maintainer follow-up is needed because the external PR lacks inspectable real Discord proof and the current diff has a security-sensitive sanitizer bypass.

Security
Needs attention: The diff introduces a concrete Discord front-channel leakage risk by bypassing Discord-specific trace/channel-line scrubbing for broadly marked tool payloads.

Review findings

  • [P1] Keep Discord trace scrubbing for marked tool payloads — extensions/discord/src/monitor/reply-safety.ts:97-99
Review details

Best possible solution:

Preserve only validated Discord tool-progress labels or apply the Discord trace/channel-line scrub before preserving marked progress text, with unsafe-marked-payload regression coverage and redacted real Discord proof.

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

Yes, as a source-level reproduction: current main sends final Discord replies through the front-channel sanitizer that strips 📖 Read: and 🛠️ Exec: progress-shaped lines. I did not establish a live Discord reproduction in this read-only review.

Is this the best way to solve the issue?

No. The final-send boundary is the right place to fix the bug, but this patch is too broad because all marked tool deliveries skip Discord-specific trace and channel-line scrubbing.

Full review comments:

  • [P1] Keep Discord trace scrubbing for marked tool payloads — extensions/discord/src/monitor/reply-safety.ts:97-99
    When openclawDiscordToolProgress is present, this branch skips sanitizeDiscordFrontChannelText and only runs sanitizeAssistantVisibleText. Because message-handler.process.ts now marks every tool-kind reply, a payload containing analysis:, commentary:, tool_call:, or other internal trace prefixes would be posted to Discord instead of stripped; restrict the marker to known-safe progress formats or keep the Discord scrub active.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

Security concerns:

  • [medium] Do not bypass Discord trace scrub for all tool payloads — extensions/discord/src/monitor/reply-safety.ts:97
    The marker is applied to every tool-kind delivery, and marked payloads skip the Discord-specific sanitizer that strips internal trace and channel-prefix lines before posting to the channel.
    Confidence: 0.88

Acceptance criteria:

  • git diff --check
  • node scripts/run-vitest.mjs extensions/discord/src/monitor/reply-delivery.test.ts extensions/discord/src/monitor/message-handler.process.test.ts
  • Real Discord proof showing Write, Read, and Exec progress messages after the fix with private details redacted

What I checked:

  • Current main strips progress-shaped internal trace lines: sanitizeDiscordFrontChannelText runs sanitizeAssistantVisibleText and then stripDiscordInternalTraceLines; the trace regex includes 📖 Read: and 🛠️ Exec:, so legitimate progress-shaped text can be removed at the Discord front-channel boundary. (extensions/discord/src/monitor/reply-safety.ts:5, 64b94daf92c4)
  • Final Discord delivery uses the scrubber: deliverDiscordReply sanitizes payloads through sanitizeDiscordFrontChannelReplyPayloads before outbound delivery, making this the affected final-send path. (extensions/discord/src/monitor/reply-delivery.ts:179, 64b94daf92c4)
  • Existing test documents trace stripping: Current Discord delivery coverage expects internal trace lines including 📖 Read: to be dropped while preserving user-visible text, so a preservation fix must keep trace/channel-line scrubbing for unsafe content. (extensions/discord/src/monitor/reply-delivery.test.ts:158, 64b94daf92c4)
  • PR marks every tool delivery: The PR branch applies openclawDiscordToolProgress: true whenever info.kind === "tool", without checking that the payload text is one of the known safe progress labels. (extensions/discord/src/monitor/message-handler.process.ts:542, 5c9b69327d01)
  • PR bypasses Discord-specific trace and channel scrubbing: For marked payloads, the PR branch routes text through only sanitizeAssistantVisibleText(...).trim(), skipping sanitizeDiscordFrontChannelText and therefore skipping Discord-specific internal trace and channel prefix removal. (extensions/discord/src/monitor/reply-safety.ts:97, 5c9b69327d01)
  • Real behavior proof remains missing: Live PR metadata still shows triage: needs-real-behavior-proof; the body claims a live Discord smoke but provides no inspectable screenshot, recording, terminal/live output, transcript, linked artifact, or redacted log. (5c9b69327d01)

Likely related people:

  • steipete: Recent GitHub history for reply-safety.ts, reply-delivery.ts, and message-handler.process.ts shows repeated Discord reply, progress draft, and sanitizer work in this area. (role: recent Discord channel and sanitizer contributor; confidence: high; commits: 343f859b900c, 827b0de0ce74, 11dc5cb94e9e; files: extensions/discord/src/monitor/reply-safety.ts, extensions/discord/src/monitor/reply-delivery.ts, extensions/discord/src/monitor/message-handler.process.ts)
  • Patrick-Erichsen: Authored recent current-main work on the same final Discord reply delivery boundary used by this PR. (role: adjacent Discord reply-delivery contributor; confidence: medium; commits: 9e97cdb21389; files: extensions/discord/src/monitor/message-handler.process.ts, extensions/discord/src/monitor/reply-delivery.ts, extensions/discord/src/monitor/reply-delivery.test.ts)
  • Satoshi: Recent history for reply-safety.ts includes Discord reply scrub hardening and non-text payload preservation work under this name. (role: adjacent reply scrubber contributor; confidence: medium; commits: 5572c8137cb3, e259938e9643; files: extensions/discord/src/monitor/reply-safety.ts, extensions/discord/src/monitor/reply-delivery.test.ts)

Remaining risk / open question:

  • Marked tool payloads can expose Discord internal trace or channel-prefixed text such as analysis:, commentary:, or tool_call: until the marker is narrowed or the Discord scrub remains active.
  • The PR body claims live Discord validation, but no inspectable after-fix proof is attached for review.
  • Live PR metadata reports the branch is conflicting and several checks are failing, so it needs a normal branch refresh after code fixes.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 64b94daf92c4.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

💡 Codex Review

https://github.com/openclaw/openclaw/blob/a92a4aac0acd854dd52d52d2325d46ff9180df21/extensions/discord/src/monitor/message-handler.process.ts#L555
P1 Badge Restrict tool-progress marker to safe labels

Marking every info.kind === "tool" payload with openclawDiscordToolProgress causes those messages to bypass sanitizeDiscordFrontChannelText and skip stripDiscordInternalTraceLines. Any tool payload that still carries internal trace text (for example 📊 Session Status: lines that were previously scrubbed) will now be sent to Discord unchanged, so this widens exposure beyond the intended Read/Exec/Write progress lines. Gate this marker to explicitly allowed progress formats (or keep the trace-line scrub for marked payloads).

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

@mingming099
mingming099 force-pushed the fix-discord-tool-progress-sanitizer branch from a92a4aa to 5c9b693 Compare May 6, 2026 17:17
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

💡 Codex Review

openclaw/extensions/discord/src/monitor/reply-safety.ts

Lines 97 to 99 in 5c9b693

? shouldPreserveDiscordToolProgressText(payload)
? sanitizeAssistantVisibleText(payload.text).trim()
: sanitizeDiscordFrontChannelText(payload.text)

P1 Badge Keep Discord channel-line scrub for marked tool payloads

When openclawDiscordToolProgress is present, this branch skips sanitizeDiscordFrontChannelText and only runs sanitizeAssistantVisibleText, which means the Discord-specific channel-line filter (analysis:, commentary:, tool_call: etc.) no longer runs for any marked payload. Because message-handler.process.ts now auto-marks all info.kind === "tool" replies, a tool result that includes one of those internal prefixes (for example from raw debug/progress text) will now be posted to Discord instead of being stripped, which is a user-visible leakage regression.

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

@mingming099 mingming099 closed this by deleting the head repository May 25, 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 size: S 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.

1 participant

@mingming099