Fix Slack thread streaming missing recipient IDs (#20337)#20377
Fix Slack thread streaming missing recipient IDs (#20337)#20377AsserAl1012 wants to merge 1 commit intoopenclaw:mainfrom
Conversation
| if (!recipientTeamId || !recipientUserId) { | ||
| logVerbose( | ||
| "slack-stream: missing recipient ids for stream start, falling back to normal delivery", | ||
| ); | ||
| streamFailed = true; | ||
| await deliverNormally(payload, streamThreadTs); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Redundant recipient ID guard (defense-in-depth)
This guard can never trigger at runtime. deliverWithStreaming is only called when useStreaming is true (line 249), and useStreaming is already gated by shouldUseSlackNativeStreaming which checks both recipientTeamId and recipientUserId (lines 167-172). If the IDs are missing, useStreaming will be false and this code path is never reached.
This isn't a bug — keeping it as defense-in-depth is a valid choice — but it's worth noting that TypeScript narrowing won't help downstream code here since the variables are already narrowed by the outer gate. If the intent is belt-and-suspenders, a brief comment explaining that would help future readers.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/slack/monitor/message-handler/dispatch.ts
Line: 211-218
Comment:
**Redundant recipient ID guard (defense-in-depth)**
This guard can never trigger at runtime. `deliverWithStreaming` is only called when `useStreaming` is `true` (line 249), and `useStreaming` is already gated by `shouldUseSlackNativeStreaming` which checks both `recipientTeamId` and `recipientUserId` (lines 167-172). If the IDs are missing, `useStreaming` will be `false` and this code path is never reached.
This isn't a bug — keeping it as defense-in-depth is a valid choice — but it's worth noting that TypeScript narrowing won't help downstream code here since the variables are already narrowed by the outer gate. If the intent is belt-and-suspenders, a brief comment explaining that would help future readers.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Since Prompt To Fix With AIThis is a comment left during a code review.
Path: src/slack/monitor/message-handler/dispatch.ts
Line: 424-429
Comment:
**Dead try/catch after `stopSlackStream` change**
Since `stopSlackStream` now internally catches and swallows its own errors (see `streaming.ts:138-143`), this outer `try/catch` will never fire — `stopSlackStream` always resolves successfully. This isn't a bug, but it is dead code that may mislead future readers into thinking `stopSlackStream` can throw. Consider removing the try/catch wrapper here:
```suggestion
const finalStream = streamSession as SlackStreamSession | null;
if (finalStream && !finalStream.stopped) {
await stopSlackStream({ session: finalStream });
}
```
How can I resolve this? If you propose a fix, please make it concise. |
|
watching for when this PR gets merged as I'm affected by it |
Summary
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation: N/ARepro + Verification
Environment
Steps
Expected
Actual
Evidence
Attach at least one:
Failing test/log before + passing after
Added unit tests:
- src/slack/streaming.test.ts asserts chatStream called with recipient_team_id and recipient_user_id, and stopSlackStream swallows stop failures.
- src/slack/monitor/message-handler/dispatch.streaming.test.ts covers gating when teamId or userId is missing.
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)
Human Verification (required)
What you personally verified (not just CI), and how:
Verified scenarios: Unit tests for streaming start parameters (recipient_team_id, recipient_user_id) and defensive stop behavior.
Gating logic: streaming disabled when recipient IDs missing; fallback delivery remains reachable.
Edge cases checked:
- Missing ctx.teamId ⇒ streaming is skipped.
- Missing message.user ⇒ streaming is skipped.
- Stream stop failure ⇒ caught/logged, no crash/throw path.
What you did not verify: Full end-to-end Slack thread streaming on a live Slack workspace in Socket Mode (left to CI / maintainers to validate in real environment).
Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
How to disable/revert this change quickly: Disable Slack streaming: openclaw config set channels.slack.streaming false and restart gateway, or revert this PR.
Files/config to restore: src/slack/streaming.ts - src/slack/monitor/message-handler/dispatch.ts
Known bad symptoms reviewers should watch for: Slack thread replies not appearing when streaming is enabled -- Reappearance of missing_recipient_team_id errors in slack-stream logs
Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.AI Usage Disclosure
Greptile Summary
This PR fixes Slack thread streaming failures caused by missing
recipient_team_idandrecipient_user_idwhen callingchat.startStream. The fix adds these required fields tostartSlackStream, gates native streaming on their presence via the newshouldUseSlackNativeStreamingfunction, and wrapsstopSlackStreamin a try/catch to prevent unhandled rejections from disrupting delivery.startSlackStreamnow accepts and forwardsrecipientTeamIdandrecipientUserIdtoclient.chatStream()shouldUseSlackNativeStreaminggating function ensures streaming is only attempted when all required IDs are present; otherwise falls back to normal (non-streaming) deliverystopSlackStreamnow catches and logs errors instead of throwing, preventing delivery disruptionschannelrenamed tochannelIdfor clarity inStartSlackStreamParamsstopSlackStreamindispatch.tsis now dead code sincestopSlackStreaminternally swallows errorsConfidence Score: 4/5
src/slack/monitor/message-handler/dispatch.tshas minor dead code but no functional issues.Last reviewed commit: 7b28b33
(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!