feat(channels): stream tool progress into preview edits#69611
Conversation
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🟡 Discord streamed preview tool-progress can trigger unwanted mentions (@everyone/@here) due to missing allowed_mentions suppression
DescriptionIn Discord preview streaming, tool/progress payload fields are concatenated into a preview message and sent via
This allows attacker-controlled or tool-controlled strings to inject mentions into streamed preview updates, potentially causing notification spam or social-engineering content in channels where previews are enabled. RecommendationSuppress mentions for preview/draft messages (and ideally for all bot messages unless explicitly needed). For Discord REST payloads, set await rest.post(Routes.channelMessages(channelId), {
body: {
content: trimmed,
allowed_mentions: { parse: [] },
...(messageReference ? { message_reference: messageReference } : {}),
},
});
await rest.patch(Routes.channelMessage(channelId, streamMessageId), {
body: {
content: trimmed,
allowed_mentions: { parse: [] },
},
});Additionally consider neutralizing 2. 🟡 Slack streamed preview tool-progress can inject special mentions (//@channel) due to lack of mrkdwn escaping
DescriptionIn Slack preview streaming, tool/progress payload fields are concatenated into a preview message and sent/edited via
This can be abused to trigger channel-wide notifications or craft misleading formatted preview updates if an attacker can influence these strings (directly or indirectly via tool output). RecommendationEscape/neutralize Slack mrkdwn and special mention tokens before calling For example, reuse an escaping helper (there is already import { escapeSlackMrkdwn } from "../mrkdwn.js";
const normalized = escapeSlackMrkdwn(line?.replace(/\s+/g, " ").trim() ?? "");Additionally, explicitly neutralize const safe = normalized.replace(/<!/g, "<\\!");Apply the same escaping when editing existing preview messages (draft stream). Analyzed PR: #69611 at commit Last updated on: 2026-04-21T16:37:55Z |
openclaw-barnacle
Bot
added
docs
Greptile SummaryThis PR streams live tool/activity progress into channel preview draft messages (Discord, Slack, Telegram) that support edit-in-place streaming. A new Confidence Score: 5/5Safe to merge; no P0/P1 issues found and the feature logic is sound across all three channel implementations. State management (previewToolProgressSuppressed, previewToolProgressLines, hasStreamedMessage) is correctly reset in onAssistantMessageStart and onReasoningEnd. The config opt-out path gracefully falls back to default progress messages. The only finding is a P2 style concern about code duplication in the three pushPreviewToolProgress implementations. No files require special attention; the duplicated helper logic across the three channel dispatch files is a minor maintenance concern but not a bug. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/discord/src/monitor/message-handler.process.ts
Line: 608-634
Comment:
**Duplicated `pushPreviewToolProgress` across all three channel implementations**
The logic in `pushPreviewToolProgress` is nearly identical across Discord (`message-handler.process.ts`), Slack (`dispatch.ts`), and Telegram (`bot-message-dispatch.ts`): normalize the label, deduplicate against the previous entry, cap the list at 8, format as `"Working…\n• item1\n• item2"`, and call `draftStream.update(...)`. The only channel-specific differences are how `hasStreamedMessage` and `draftText`/`lastPartialText` are reset.
Extracting a shared `buildPreviewToolProgressText(lines: string[]): string` helper (or the full queue-management logic as a factory) into `channel-streaming.ts` would reduce the surface area for divergence over time.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(channels): stream tool progress int..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const previewToolProgressEnabled = | ||
| Boolean(draftStream) && resolveChannelStreamingPreviewToolProgress(discordConfig); | ||
| let previewToolProgressSuppressed = false; | ||
| let previewToolProgressLines: string[] = []; | ||
|
|
||
| const pushPreviewToolProgress = (line?: string) => { | ||
| if (!draftStream || !previewToolProgressEnabled || previewToolProgressSuppressed) { | ||
| return; | ||
| } | ||
| const normalized = line?.replace(/\s+/g, " ").trim(); | ||
| if (!normalized) { | ||
| return; | ||
| } | ||
| const previous = previewToolProgressLines.at(-1); | ||
| if (previous === normalized) { | ||
| return; | ||
| } | ||
| previewToolProgressLines = [...previewToolProgressLines, normalized].slice(-8); | ||
| const previewText = ["Working…", ...previewToolProgressLines.map((entry) => `• ${entry}`)].join( | ||
| "\n", | ||
| ); | ||
| lastPartialText = previewText; | ||
| draftText = previewText; | ||
| hasStreamedMessage = true; | ||
| draftChunker?.reset(); | ||
| draftStream.update(previewText); | ||
| }; |
There was a problem hiding this comment.
pushPreviewToolProgress across all three channel implementations
The logic in pushPreviewToolProgress is nearly identical across Discord (message-handler.process.ts), Slack (dispatch.ts), and Telegram (bot-message-dispatch.ts): normalize the label, deduplicate against the previous entry, cap the list at 8, format as "Working…\n• item1\n• item2", and call draftStream.update(...). The only channel-specific differences are how hasStreamedMessage and draftText/lastPartialText are reset.
Extracting a shared buildPreviewToolProgressText(lines: string[]): string helper (or the full queue-management logic as a factory) into channel-streaming.ts would reduce the surface area for divergence over time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/discord/src/monitor/message-handler.process.ts
Line: 608-634
Comment:
**Duplicated `pushPreviewToolProgress` across all three channel implementations**
The logic in `pushPreviewToolProgress` is nearly identical across Discord (`message-handler.process.ts`), Slack (`dispatch.ts`), and Telegram (`bot-message-dispatch.ts`): normalize the label, deduplicate against the previous entry, cap the list at 8, format as `"Working…\n• item1\n• item2"`, and call `draftStream.update(...)`. The only channel-specific differences are how `hasStreamedMessage` and `draftText`/`lastPartialText` are reset.
Extracting a shared `buildPreviewToolProgressText(lines: string[]): string` helper (or the full queue-management logic as a factory) into `channel-streaming.ts` would reduce the surface area for divergence over time.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe88adcd5e
ℹ️ 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 hasExecApproval = | ||
| execApproval && typeof execApproval === "object" && !Array.isArray(execApproval); | ||
| if (!hasMedia && !hasExecApproval && deliveryPayload.isError !== true) { | ||
| return; |
There was a problem hiding this comment.
Do not drop every text-only tool payload when suppressDefaultToolProgressMessages is enabled: this branch only preserves media, errors, or payloads with channelData.execApproval, but buildExecApprovalUnavailableReplyPayload(...) produces plain text guidance without execApproval. In preview-enabled channels (where suppression is now enabled by default), users can lose the only actionable “how to enable approvals” message and only see transient preview status instead.
Useful? React with 👍 / 👎.
thewilloftheshadow
force-pushed
the
shadow/stream-verbose
branch
from
fe88adc to
170293f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 170293f6b3
ℹ️ 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".
| draftStream.update( | ||
| ["Working…", ...previewToolProgressLines.map((entry) => `• ${entry}`)].join("\n"), | ||
| ); | ||
| hasStreamedMessage = true; |
There was a problem hiding this comment.
Marking tool-progress preview writes as hasStreamedMessage makes the existing onDraftBoundary handler call forceNewMessage() on the next onAssistantMessageStart/onReasoningEnd, which leaves the prior "Working…" preview behind and posts the answer in a second message. In a normal tool-call turn (tool events before answer text) with streaming.preview.toolProgress enabled (default), this regresses from a single evolving preview to noisy duplicate messages in the thread.
Useful? React with 👍 / 👎.
|
Landed via temp rebase onto main.
Thanks @thewilloftheshadow! |
1 participant
No description provided.