Slack: support Block Kit payloads in agent replies#44592
Conversation
🔒 Aisle Security Analysis✅ We scanned this PR and did not find any security vulnerabilities. Aisle supplements but does not replace security review. Analyzed PR: #44592 at commit Last updated on: 2026-03-13T04:36:05Z |
Greptile SummaryThis PR extends the Slack outbound adapter to forward Block Kit payloads when Key changes:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/channels/plugins/outbound/slack.ts
Line: 115-127
Comment:
**Unhandled `blocks` + `mediaUrl` combination**
When a payload contains both `channelData.slack.blocks` and `mediaUrl`, this branch passes both to `sendSlackOutboundMessage`, which in turn passes both to `sendMessageSlack`. Inside `sendMessageSlack`, the check at the `blocks` handling path throws `"Slack send does not support blocks with mediaUrl"` — but only after the hook runner has already been invoked and any side-effects from hooks have taken place.
Consider detecting and rejecting the conflicting combination early at the adapter level, before any I/O:
```suggestion
if (ctx.payload.mediaUrl) {
throw new Error(
"Slack send does not support blocks with mediaUrl; provide either blocks or mediaUrl",
);
}
return await sendSlackOutboundMessage({
cfg: ctx.cfg,
to: ctx.to,
text: ctx.payload.text ?? "",
mediaLocalRoots: ctx.mediaLocalRoots,
blocks,
accountId: ctx.accountId,
deps: ctx.deps,
replyToId: ctx.replyToId,
threadId: ctx.threadId,
identity: ctx.identity,
});
```
This edge case is also not covered by the new tests, so adding a test for `blocks + mediaUrl` would help document the constraint explicitly.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 6579b9d |
![greptile-apps[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return await sendSlackOutboundMessage({ | ||
| cfg: ctx.cfg, | ||
| to: ctx.to, | ||
| text: ctx.payload.text ?? "", | ||
| mediaUrl: ctx.payload.mediaUrl, | ||
| mediaLocalRoots: ctx.mediaLocalRoots, | ||
| blocks, | ||
| accountId: ctx.accountId, | ||
| deps: ctx.deps, | ||
| replyToId: ctx.replyToId, | ||
| threadId: ctx.threadId, | ||
| identity: ctx.identity, | ||
| }); |
There was a problem hiding this comment.
Unhandled blocks + mediaUrl combination
When a payload contains both channelData.slack.blocks and mediaUrl, this branch passes both to sendSlackOutboundMessage, which in turn passes both to sendMessageSlack. Inside sendMessageSlack, the check at the blocks handling path throws "Slack send does not support blocks with mediaUrl" — but only after the hook runner has already been invoked and any side-effects from hooks have taken place.
Consider detecting and rejecting the conflicting combination early at the adapter level, before any I/O:
| return await sendSlackOutboundMessage({ | |
| cfg: ctx.cfg, | |
| to: ctx.to, | |
| text: ctx.payload.text ?? "", | |
| mediaUrl: ctx.payload.mediaUrl, | |
| mediaLocalRoots: ctx.mediaLocalRoots, | |
| blocks, | |
| accountId: ctx.accountId, | |
| deps: ctx.deps, | |
| replyToId: ctx.replyToId, | |
| threadId: ctx.threadId, | |
| identity: ctx.identity, | |
| }); | |
| if (ctx.payload.mediaUrl) { | |
| throw new Error( | |
| "Slack send does not support blocks with mediaUrl; provide either blocks or mediaUrl", | |
| ); | |
| } | |
| return await sendSlackOutboundMessage({ | |
| cfg: ctx.cfg, | |
| to: ctx.to, | |
| text: ctx.payload.text ?? "", | |
| mediaLocalRoots: ctx.mediaLocalRoots, | |
| blocks, | |
| accountId: ctx.accountId, | |
| deps: ctx.deps, | |
| replyToId: ctx.replyToId, | |
| threadId: ctx.threadId, | |
| identity: ctx.identity, | |
| }); |
This edge case is also not covered by the new tests, so adding a test for blocks + mediaUrl would help document the constraint explicitly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/channels/plugins/outbound/slack.ts
Line: 115-127
Comment:
**Unhandled `blocks` + `mediaUrl` combination**
When a payload contains both `channelData.slack.blocks` and `mediaUrl`, this branch passes both to `sendSlackOutboundMessage`, which in turn passes both to `sendMessageSlack`. Inside `sendMessageSlack`, the check at the `blocks` handling path throws `"Slack send does not support blocks with mediaUrl"` — but only after the hook runner has already been invoked and any side-effects from hooks have taken place.
Consider detecting and rejecting the conflicting combination early at the adapter level, before any I/O:
```suggestion
if (ctx.payload.mediaUrl) {
throw new Error(
"Slack send does not support blocks with mediaUrl; provide either blocks or mediaUrl",
);
}
return await sendSlackOutboundMessage({
cfg: ctx.cfg,
to: ctx.to,
text: ctx.payload.text ?? "",
mediaLocalRoots: ctx.mediaLocalRoots,
blocks,
accountId: ctx.accountId,
deps: ctx.deps,
replyToId: ctx.replyToId,
threadId: ctx.threadId,
identity: ctx.identity,
});
```
This edge case is also not covered by the new tests, so adding a test for `blocks + mediaUrl` would help document the constraint explicitly.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](proxy.php?url=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: 59b7fec2a6
ℹ️ 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".
| text: ctx.payload.text ?? "", | ||
| mediaUrl: ctx.payload.mediaUrl, | ||
| mediaLocalRoots: ctx.mediaLocalRoots, |
There was a problem hiding this comment.
When channelData.slack.blocks is present, sendPayload bypasses sendTextMediaPayload and only forwards ctx.payload.mediaUrl, so payloads that use mediaUrls (the normal multi-attachment shape) silently drop all attachments and send only the blocks/text message. This is a regression from the prior path where mediaUrls were iterated and delivered, and it will break replies that include both Block Kit metadata and attachment lists.
Useful? React with 👍 / 👎.
* Slack: route reply blocks through outbound adapter * Slack: cover Block Kit outbound payloads * Changelog: add Slack Block Kit agent reply entry
|
THANK YOU! |
* Slack: route reply blocks through outbound adapter * Slack: cover Block Kit outbound payloads * Changelog: add Slack Block Kit agent reply entry
* Slack: route reply blocks through outbound adapter * Slack: cover Block Kit outbound payloads * Changelog: add Slack Block Kit agent reply entry
* Slack: route reply blocks through outbound adapter * Slack: cover Block Kit outbound payloads * Changelog: add Slack Block Kit agent reply entry (cherry picked from commit 42efd98)
2 participants
Summary
ReplyPayload.channelData.slack.blocksis now parsed and forwarded through the Slack outbound adapter tosendMessageSlack.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Agent replies can now send Slack Block Kit payloads by populating
channelData.slack.blockson the reply payload. Plain text Slack replies are unchanged.Security Impact (required)
NoNoNoNoNoYes, explain risk + mitigation:Repro + Verification
Environment
Steps
channelData.slack.blocks.blockstosendMessageSlackand rejects malformed block payloads.Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
YesNoNoFailure Recovery (if this breaks)
channelData.slack.blocks.src/channels/plugins/outbound/slack.tschannelData.slack.blocksis produced upstream.Risks and Mitigations
AI Assistance