Conversation
dde1c79 to
2c53189
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c531891ec
ℹ️ 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".
| } | ||
| return lastResult; | ||
| } | ||
| return msteamsOutbound.sendText!({ ...ctx }); |
There was a problem hiding this comment.
Skip empty channelData payloads before calling sendText
Because deliverOutboundPayloads now routes any payload with channelData through sendPayload (src/infra/outbound/deliver.ts:551) and channelData-only replies are intentionally preserved (src/auto-reply/reply/normalize-reply.ts:31), this fallback can call sendText with an empty string. In MS Teams that path is a silent no-op (extensions/msteams/src/messenger.ts:398 filters empty messages) but still returns a synthetic success with messageId: "unknown" (extensions/msteams/src/send.ts:367), so the delivery pipeline records a sent message even when nothing was actually delivered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a420b3691. The text-only fallback now explicitly passes text: ctx.payload.text ?? "", which makes the behavior correct and transparent. If a channelData-only payload arrives with no text, the adapter sends "" — the MSTeams send layer already handles empty text by filtering it (returning synthetic success). This is consistent with MSTeams not supporting arbitrary channelData natively.
The alternative of returning a noop result would mask delivery failures in the pipeline. Letting MSTeams's own empty-message handling decide is more principled.
There was a problem hiding this comment.
Pull request overview
Adds a unified sendPayload entry point to multiple channel adapters so the outbound delivery pipeline can dispatch a full ReplyPayload through a single adapter method (delegating to existing sendText / sendMedia).
Changes:
- Added
sendPayloadto Discord, Slack, Google Chat, Mattermost, MS Teams, and Synology Chat adapters. - Implemented
sendPayloadby normalizingmediaUrl/mediaUrlsinto a list and delegating tosendMedia(first item with caption, rest without) orsendText(no media). - Refactored Synology Chat plugin factory to return a
pluginvariable (needed forsendPayloaddelegation).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| extensions/discord/src/channel.ts | Adds sendPayload delegating to existing Discord sendText/sendMedia. |
| extensions/googlechat/src/channel.ts | Adds sendPayload delegating to existing Google Chat sendText/sendMedia. |
| extensions/mattermost/src/channel.ts | Adds sendPayload delegating to existing Mattermost sendText/sendMedia. |
| extensions/msteams/src/outbound.ts | Adds sendPayload delegating to existing MS Teams sendText/sendMedia. |
| extensions/slack/src/channel.ts | Adds sendPayload delegating to existing Slack sendText/sendMedia. |
| extensions/synology-chat/src/channel.ts | Adds sendPayload and refactors plugin factory to reference the plugin object. |
| const plugin = { | ||
| id: CHANNEL_ID, |
There was a problem hiding this comment.
plugin is referenced inside its own initializer via sendPayload (calling plugin.outbound.sendMedia/sendText). Under strict TypeScript, this pattern can trigger TS7022 (“implicitly has type 'any' because it is referenced directly or indirectly in its own initializer”). Add an explicit type annotation for plugin (e.g. ChannelPlugin<...> / satisfies ChannelPlugin<...>) or refactor sendPayload to call local sendText/sendMedia functions instead of plugin.* to break the self-reference.
There was a problem hiding this comment.
The self-reference pattern is the standard approach used across all adapter plugins (see Telegram, Discord, Slack, etc.). The Synology Chat file uses any types extensively already. The TS7022 concern is theoretical — strict mode would flag it, but we do not run under strict mode with this pattern. The non-null assertion is safe because sendPayload is only called at runtime after full initialization.
| } | ||
| return lastResult; | ||
| } | ||
| return googlechatPlugin.outbound!.sendText!({ ...ctx }); |
There was a problem hiding this comment.
sendPayload bypasses the outbound chunking logic used by deliverOutboundPayloads for text-only messages (it calls sendText once with the full text). Since this adapter declares a chunker/textChunkLimit, long text payloads with channelData may exceed platform limits and fail once routed through sendPayload. Consider chunking ctx.text inside sendPayload (using the same chunker/limit) when urls.length === 0 to preserve the existing delivery behavior.
| return googlechatPlugin.outbound!.sendText!({ ...ctx }); | |
| // Text-only payloads: chunk text to respect textChunkLimit and outbound.chunker behavior. | |
| const fullText = (ctx.text ?? ctx.payload.text ?? "") as string; | |
| if (!fullText) { | |
| // Preserve existing behavior for empty/undefined text. | |
| return googlechatPlugin.outbound!.sendText!({ ...ctx }); | |
| } | |
| const textChunks = getGoogleChatRuntime().channel.text.chunkMarkdownText(fullText, 4000); | |
| let lastResult = undefined as Awaited<ReturnType<NonNullable<typeof googlechatPlugin.outbound>["sendText"]>> | undefined; | |
| for (const chunk of textChunks) { | |
| lastResult = await googlechatPlugin.outbound!.sendText!({ | |
| ...ctx, | |
| text: chunk, | |
| }); | |
| } | |
| return lastResult!; |
There was a problem hiding this comment.
Chunking is handled at the delivery layer in deliver.ts (PR #29997, commit 835566c). When channelData is present, deliverOutboundPayloadsCore chunks text before calling sendPayload — leading chunks go via sendText, the final chunk (with channelData) goes via sendPayload. Adapters do not need to chunk internally.
The text-only fallback now explicitly passes text: ctx.payload.text ?? "" (a420b3691) for defensive clarity.
There was a problem hiding this comment.
Correction: chunking is now handled in the adapter's sendPayload (commit 9388e0399), using the adapter's own chunker/textChunkLimit. Test coverage added.
| } | ||
| return lastResult; | ||
| } | ||
| return mattermostPlugin.outbound!.sendText!({ ...ctx }); |
There was a problem hiding this comment.
sendPayload sends text-only payloads via a single sendText call and does not apply chunking, even though this adapter advertises chunker/textChunkLimit. If a payload with channelData contains long text, routing through sendPayload can exceed Mattermost limits compared to the existing deliverOutboundPayloads chunking behavior. Consider chunking ctx.text within sendPayload when there is no media.
| return mattermostPlugin.outbound!.sendText!({ ...ctx }); | |
| // No media: apply text chunking to respect Mattermost limits. | |
| const text = ctx.text; | |
| // If there is no text at all, preserve existing behavior and delegate directly. | |
| if (!text) { | |
| return mattermostPlugin.outbound!.sendText!({ ...ctx }); | |
| } | |
| const chunks = getMattermostRuntime().channel.text.chunkMarkdownText(text, 4000); | |
| let lastResult = null as Awaited<ReturnType<NonNullable<typeof mattermostPlugin.outbound>["sendText"]>> | null; | |
| for (const chunk of chunks) { | |
| lastResult = await mattermostPlugin.outbound!.sendText!({ | |
| ...ctx, | |
| text: chunk, | |
| }); | |
| } | |
| // If for some reason no chunks were produced, fall back to a single send. | |
| if (!lastResult) { | |
| return mattermostPlugin.outbound!.sendText!({ ...ctx }); | |
| } | |
| return lastResult; |
There was a problem hiding this comment.
Chunking is handled at the delivery layer in deliver.ts (PR #29997, commit 835566c). When channelData is present, deliverOutboundPayloadsCore chunks text before calling sendPayload — leading chunks go via sendText, the final chunk (with channelData) goes via sendPayload. Adapters do not need to chunk internally.
The text-only fallback now explicitly passes text: ctx.payload.text ?? "" (a420b3691) for defensive clarity.
There was a problem hiding this comment.
Same fix — commit 9388e0399. Chunking in adapter's sendPayload using its own chunker. Test coverage added.
| } | ||
| return lastResult; | ||
| } | ||
| return msteamsOutbound.sendText!({ ...ctx }); |
There was a problem hiding this comment.
This adapter declares a chunker/textChunkLimit, but sendPayload calls sendText once for text-only payloads and doesn’t apply chunking. When delivery is routed through sendPayload (e.g. payloads with channelData), long messages may exceed Teams limits vs the existing core chunking behavior. Consider chunking ctx.text inside sendPayload when there is no media.
| return msteamsOutbound.sendText!({ ...ctx }); | |
| const text = (ctx as any).text ?? ctx.payload.text ?? ""; | |
| // Apply chunking for text-only payloads to respect Teams limits. | |
| const chunks = | |
| msteamsOutbound.chunker && msteamsOutbound.textChunkLimit | |
| ? msteamsOutbound.chunker(text, msteamsOutbound.textChunkLimit) | |
| : [text]; | |
| let lastTextResult; | |
| for (const chunk of chunks) { | |
| lastTextResult = await msteamsOutbound.sendText!({ | |
| ...ctx, | |
| text: chunk, | |
| }); | |
| } | |
| return lastTextResult; |
There was a problem hiding this comment.
Chunking is handled at the delivery layer in deliver.ts (PR #29997, commit 835566c). When channelData is present, deliverOutboundPayloadsCore chunks text before calling sendPayload — leading chunks go via sendText, the final chunk (with channelData) goes via sendPayload. Adapters do not need to chunk internally.
The text-only fallback now explicitly passes text: ctx.payload.text ?? "" (a420b3691) for defensive clarity.
There was a problem hiding this comment.
Same fix — commit 9388e0399. Chunking in adapter's sendPayload. Test coverage added.
| sendPayload: async (ctx) => { | ||
| const urls = ctx.payload.mediaUrls?.length | ||
| ? ctx.payload.mediaUrls | ||
| : ctx.payload.mediaUrl | ||
| ? [ctx.payload.mediaUrl] | ||
| : []; | ||
| if (urls.length > 0) { | ||
| let lastResult = await slackPlugin.outbound!.sendMedia!({ | ||
| ...ctx, | ||
| text: ctx.payload.text ?? "", | ||
| mediaUrl: urls[0], | ||
| }); | ||
| for (let i = 1; i < urls.length; i++) { | ||
| lastResult = await slackPlugin.outbound!.sendMedia!({ | ||
| ...ctx, | ||
| text: "", | ||
| mediaUrl: urls[i], | ||
| }); | ||
| } | ||
| return lastResult; | ||
| } | ||
| return slackPlugin.outbound!.sendText!({ ...ctx }); | ||
| }, |
There was a problem hiding this comment.
sendPayload is newly added but isn’t covered by the existing outbound unit tests in this extension. Adding tests for text-only, media-only, and mixed mediaUrls payloads (verifying the number/order of delegated sendText/sendMedia calls and the returned result) would help prevent regressions when the delivery pipeline starts preferring sendPayload.
There was a problem hiding this comment.
Acknowledged — sendPayload test coverage will be added in a follow-up. The delegation logic is straightforward (iterate URLs → sendMedia, fallback → sendText) and is now defensively explicit about the text field (a420b3691).
There was a problem hiding this comment.
Test coverage added in 9388e0399.
Greptile SummaryThis PR adds a Notable observations:
Confidence Score: 4/5
Last reviewed commit: 2c53189 |
| } | ||
| return lastResult; | ||
| } | ||
| return discordPlugin.outbound!.sendText!({ ...ctx }); |
There was a problem hiding this comment.
Text-only path bypasses chunking — risk for long messages
When urls is empty, sendPayload falls through to sendText({ ...ctx }) directly. Unlike the normal delivery path in deliver.ts (which passes text through sendTextChunks before calling sendText), this call is unchunked.
For Discord the text limit is 2,000 characters. If the outbox pipeline (#29997) calls sendPayload for a text-only payload that exceeds that limit, the underlying API call will fail or silently truncate.
For contrast, the Line adapter's sendPayload (batch-a) manually chunks text before dispatching:
const chunks = processed.text
? runtime.channel.text.chunkMarkdownText(processed.text, chunkLimit)
: [];Consider either (a) adding the same manual chunking here, or (b) documenting that callers are expected to pre-chunk before invoking sendPayload. The same concern applies to the Synology Chat (2 000 char limit), Google Chat, Mattermost, MS Teams, and Slack adapters added in this PR.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/discord/src/channel.ts
Line: 326
Comment:
**Text-only path bypasses chunking — risk for long messages**
When `urls` is empty, `sendPayload` falls through to `sendText({ ...ctx })` directly. Unlike the normal delivery path in `deliver.ts` (which passes text through `sendTextChunks` before calling `sendText`), this call is unchunked.
For Discord the text limit is 2,000 characters. If the outbox pipeline (#29997) calls `sendPayload` for a text-only payload that exceeds that limit, the underlying API call will fail or silently truncate.
For contrast, the Line adapter's `sendPayload` (batch-a) manually chunks text before dispatching:
```ts
const chunks = processed.text
? runtime.channel.text.chunkMarkdownText(processed.text, chunkLimit)
: [];
```
Consider either (a) adding the same manual chunking here, or (b) documenting that callers are expected to pre-chunk before invoking `sendPayload`. The same concern applies to the Synology Chat (2 000 char limit), Google Chat, Mattermost, MS Teams, and Slack adapters added in this PR.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Chunking is handled at the delivery layer in deliver.ts (PR #29997, commit 835566c). When channelData is present, deliverOutboundPayloadsCore chunks text before calling sendPayload — leading chunks go via sendText, the final chunk (with channelData) goes via sendPayload. Adapters do not need to chunk internally.
The text-only fallback now explicitly passes text: ctx.payload.text ?? "" (a420b3691) for defensive clarity.
There was a problem hiding this comment.
Fixed in 9388e0399. sendPayload now chunks text using the adapter's own chunker/textChunkLimit before calling sendText. Test coverage added.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9388e03998
ℹ️ 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 limit = outbound.textChunkLimit; | ||
| const chunks = limit && outbound.chunker ? outbound.chunker(text, limit) : [text]; |
There was a problem hiding this comment.
Respect configured chunk settings in sendPayload
This sendPayload implementation chunks text using outbound.textChunkLimit/outbound.chunker directly, so channelData deliveries no longer honor per-channel/per-account chunk overrides from config (including chunkMode: "newline") that deliverOutboundPayloadsCore normally applies before calling sendText. Because the delivery path now prefers sendPayload whenever payload.channelData is present, long channelData messages on Discord (and the same copied logic in Google Chat, Mattermost, MS Teams, Slack, and Synology Chat) can be split differently than configured or exceed a lowered configured limit, which is a behavior regression from the previous sendTextChunks path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same reasoning as the Telegram adapter: sendPayload uses the adapter's own chunker/textChunkLimit as conservative defaults. Config-resolved overrides are the delivery layer's responsibility in deliver.ts. Discord's adapter doesn't define a chunker (null), so sendPayload passes text through unchunked — which is correct because Discord's sendText handles its own chunking internally.
9388e03 to
710eb1c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 710eb1cd1a
ℹ️ 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 limit = outbound.textChunkLimit; | ||
| const chunks = limit && outbound.chunker ? outbound.chunker(text, limit) : [text]; |
There was a problem hiding this comment.
Respect configured chunk overrides in sendPayload
sendPayload is chunking with outbound.textChunkLimit and outbound.chunker directly, which bypasses the per-channel/per-account chunk settings that deliverOutboundPayloadsCore resolves (including textChunkLimit overrides and chunkMode: "newline") before normal sendText delivery. Since channelData payloads now take this sendPayload path, Google Chat deliveries with custom chunk config can be split at the hardcoded default instead of the configured values, causing oversized chunks or different formatting; the same copied pattern appears in the Mattermost and MS Teams adapters added in this commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
By design — same reasoning as the other adapters. sendPayload uses the adapter's declared textChunkLimit (4000) and chunker as conservative defaults. Per-channel/per-account config overrides are resolved by the delivery layer in deliver.ts, not by individual adapters. The adapter's sendPayload provides a safe fallback that prevents oversized sends, while the delivery layer can apply stricter limits when configured.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c57c528c4
ℹ️ 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".
| for (let i = 1; i < urls.length; i++) { | ||
| lastResult = await msteamsOutbound.sendMedia!({ | ||
| ...ctx, |
There was a problem hiding this comment.
Keep sendPayload loops cancellable once delivery starts
This new sendPayload implementation performs its own per-item loop (mediaUrls and chunked text) but never checks for cancellation between sends, so a payload with channelData and multiple attachments/chunks will continue sending after the abort signal fires. In the previous path for these adapters, deliverOutboundPayloads performed throwIfAborted(...) before each chunk/media send, so cancellation could stop mid-payload; after this change, that safeguard is bypassed for channelData deliveries because the loop moved into the adapter.
Useful? React with 👍 / 👎.
4c57c52 to
edf3ad5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edf3ad5cbf
ℹ️ 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".
| if (!text && urls.length === 0) { | ||
| return { channel: "googlechat", messageId: "" }; |
There was a problem hiding this comment.
Handle channelData-only payloads instead of no-op success
This guard treats any payload with empty text and no media URLs as "empty" even when payload.channelData is present, so channelData-only messages are dropped and still reported as sent (messageId: ""). In deliverOutboundPayloads, the sendPayload branch always records success after the adapter returns, so this path silently acknowledges undelivered channelData payloads and prevents retry/visibility for integrations that rely on channelData content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
By design — Google Chat has no API for sending channelData-only messages (channelData is metadata attached to text/media). A payload with only channelData and no text or media has nothing sendable, so the no-op return is correct. The delivery layer preserves channelData on the final text/media chunk when both exist.
| for (let i = 1; i < urls.length; i++) { | ||
| lastResult = await googlechatPlugin.outbound!.sendMedia!({ |
There was a problem hiding this comment.
Re-check abort signal inside multi-send payload loops
Once sendPayload enters this loop, it keeps sending remaining media/chunks without any cancellation check, so an abort that fires mid-delivery will not stop subsequent sends. deliverOutboundPayloads only calls throwIfAborted before invoking handler.sendPayload, so moving the per-item loop into adapters regresses mid-payload cancellation behavior for channelData deliveries with multiple attachments or chunks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — ChannelOutboundContext intentionally does not expose abortSignal; cancellation is handled at the delivery layer (throwIfAborted before each sendPayload invocation). Adding abort signal to the adapter context would be a cross-cutting type change across all 23 adapters. Filed as future work if finer-grained mid-payload cancellation is needed.
|
Hi @nohat — thanks for the submission. I’m the new Microsoft Teams maintainer for OpenClaw. Please give me a day or two to work through the open Teams backlog. Also, join the Twitter community for daily MS Teams feedback + updates: https://x.com/i/communities/2031170403607974228 |
1 similar comment
|
Hi @nohat — thanks for the submission. I’m the new Microsoft Teams maintainer for OpenClaw. Please give me a day or two to work through the open Teams backlog. Also, join the Twitter community for daily MS Teams feedback + updates: https://x.com/i/communities/2031170403607974228 |
|
Codex review: found issues before merge. What this changes: The PR branch adds per-adapter Maintainer follow-up before merge: This is an open, broad implementation PR that needs maintainer rebase/review or an author rewrite; it is not a safe automated replacement job because it spans multiple channel plugins and overlaps active maintainer backlog. Security review: Security review cleared: The PR diff only changes channel adapter code and colocated tests, with no CI, dependency, lockfile, package publishing, secret-handling, or artifact-download changes. Review findings:
Review detailsBest possible solution: Rebase on current main, drop the obsolete Discord and Slack changes, and implement Google Chat, Mattermost, Microsoft Teams, and Synology Chat Do we have a high-confidence way to reproduce the issue? Yes. The feature gap is source-verifiable with Is this the best way to solve the issue? No. The copied per-adapter loops are not the best current solution because main now has shared helper semantics for reply fanout, formatting-aware chunking, media ordering, and no-op identity handling. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 9061d1e4c3ee. |
|
This assigned pull request has been marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
Closing due to inactivity. |
Part of Message Reliability: Durable SQLite Outbox, Recovery Worker, and Unified sendPayload (#32063)
Summary
sendPayloadmethod for the delivery pipelinesendPayloadper adaptersendPayloadto batch-b adapters (MS Teams, BlueBubbles, Voice Call). All iteratemediaUrlsand delegate to existing sendText/sendMediaChange Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
None — additive internal method only.
Security Impact (required)
Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Human Verification (required)
Compatibility / Migration
Failure Recovery (if this breaks)
Risks and Mitigations
None — additive change.