fix(feishu): comprehensive reply mechanism — outbound replyToId forwarding + topic-aware reply targeting#33789
Merged
Takhoffman merged 7 commits intoopenclaw:mainfrom Mar 5, 2026
Conversation
Contributor
Greptile SummaryThis PR fixes two related Feishu reply-mechanism bugs: (1) outbound
Confidence Score: 4/5
Last reviewed commit: 0d50406 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
extensions/feishu/src/outbound.ts
Outdated
| textChunkLimit: 4000, | ||
| sendText: async ({ cfg, to, text, accountId }) => { | ||
| sendText: async ({ cfg, to, text, accountId, replyToId }) => { | ||
| const replyToMessageId = replyToId ?? undefined; |
Contributor
There was a problem hiding this comment.
Redundant null-coercing assignment
replyToId is typed as string | null | undefined (from ChannelOutboundContext). The expression replyToId ?? undefined converts null → undefined but this intermediate variable adds a layer of indirection with no explanation. Consider either a comment or an inline cast at the call sites:
Suggested change
| const replyToMessageId = replyToId ?? undefined; | |
| const replyToMessageId = replyToId ?? undefined; // coerce null → undefined for downstream APIs |
The same pattern appears at line 101 in sendMedia. Both are semantically correct but the intent would be clearer with a brief inline comment, since a reader unfamiliar with the type might assume this is a no-op.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/outbound.ts
Line: 70
Comment:
**Redundant null-coercing assignment**
`replyToId` is typed as `string | null | undefined` (from `ChannelOutboundContext`). The expression `replyToId ?? undefined` converts `null` → `undefined` but this intermediate variable adds a layer of indirection with no explanation. Consider either a comment or an inline cast at the call sites:
```suggestion
const replyToMessageId = replyToId ?? undefined; // coerce null → undefined for downstream APIs
```
The same pattern appears at line 101 in `sendMedia`. Both are semantically correct but the intent would be clearer with a brief inline comment, since a reader unfamiliar with the type might assume this is a no-op.
How can I resolve this? If you propose a fix, please make it concise.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!
Comment on lines
+1553
to
1596
| rootId: "om_original_msg", | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it("replies to topic root in topic-mode group with root_id", async () => { | ||
| mockShouldComputeCommandAuthorized.mockReturnValue(false); | ||
|
|
||
| const cfg: ClawdbotConfig = { | ||
| channels: { | ||
| feishu: { | ||
| groups: { | ||
| "oc-group": { | ||
| requireMention: false, | ||
| groupSessionScope: "group_topic", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } as ClawdbotConfig; | ||
|
|
||
| const event: FeishuMessageEvent = { | ||
| sender: { sender_id: { open_id: "ou-topic-user" } }, | ||
| message: { | ||
| message_id: "om_topic_reply", | ||
| root_id: "om_topic_root", | ||
| chat_id: "oc-group", | ||
| chat_type: "group", | ||
| message_type: "text", | ||
| content: JSON.stringify({ text: "hello in topic group" }), | ||
| }, | ||
| }; | ||
|
|
||
| await dispatchMessage({ cfg, event }); | ||
|
|
||
| expect(mockCreateFeishuReplyDispatcher).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| replyToMessageId: "om_topic_root", | ||
| rootId: "om_topic_root", | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it("forces thread replies when inbound message contains thread_id", async () => { |
Contributor
There was a problem hiding this comment.
Missing test for group_topic_sender scope
The new tests cover group_topic (line 1555) and the normal group scope (line 1520), but group_topic_sender is equally handled by the isTopicSession branch:
const isTopicSession =
isGroup &&
(groupSession?.groupSessionScope === "group_topic" ||
groupSession?.groupSessionScope === "group_topic_sender");A test mirroring the group_topic case with groupSessionScope: "group_topic_sender" would ensure this path isn't inadvertently broken by future changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/bot.test.ts
Line: 1553-1596
Comment:
**Missing test for `group_topic_sender` scope**
The new tests cover `group_topic` (line 1555) and the normal `group` scope (line 1520), but `group_topic_sender` is equally handled by the `isTopicSession` branch:
```typescript
const isTopicSession =
isGroup &&
(groupSession?.groupSessionScope === "group_topic" ||
groupSession?.groupSessionScope === "group_topic_sender");
```
A test mirroring the `group_topic` case with `groupSessionScope: "group_topic_sender"` would ensure this path isn't inadvertently broken by future changes.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Additional Comments (1)
When A common alternative is to mark only the first (text caption) message as the explicit reply and let the image follow as a plain message in the same conversation. If the intent is to always quote the original for both messages, a code comment explaining this decision would help future readers understand why Prompt To Fix With AIThis is a comment left during a code review.
Path: extensions/feishu/src/outbound.ts
Line: 103-124
Comment:
**Dual reply-preview headers when text + media are both sent**
When `sendMedia` is called with both a non-empty `text` caption and a `mediaUrl`, the caption is sent as a reply to `replyToMessageId` and the image is also sent as a reply to the same `replyToMessageId`. In Feishu's UI this renders two independent messages, each showing a "reply preview" quote of the original message — a potentially cluttered appearance.
A common alternative is to mark only the first (text caption) message as the explicit reply and let the image follow as a plain message in the same conversation. If the intent is to always quote the original for both messages, a code comment explaining this decision would help future readers understand why `replyToMessageId` is forwarded to both calls.
How can I resolve this? If you propose a fix, please make it concise.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! |
…orwarding + topic-aware reply targeting - Forward replyToId from ChannelOutboundContext through sendText/sendMedia to sendMessageFeishu/sendMarkdownCardFeishu/sendMediaFeishu, enabling reply-to-message via the message tool. - Fix group reply targeting: use ctx.messageId (triggering message) in normal groups to prevent silent topic thread creation (openclaw#32980). Preserve ctx.rootId targeting for topic-mode groups (group_topic/group_topic_sender) and groups with explicit replyInThread config. - Add regression tests for both fixes. Fixes openclaw#32980 Fixes openclaw#32958 Related openclaw#19784
guoqunabc
force-pushed
the
fix/feishu-reply-mechanism
branch
from
0d50406 to
45cef46
Compare
…l targets - Add normalizeDeliveryTarget helper to strip user:/chat: prefixes for Feishu - Apply normalization in matchesMessagingToolDeliveryTarget before comparison - This ensures cron duplicate suppression works when session uses prefixed targets (user:ou_xxx) but messaging tool extract uses normalized bare IDs (ou_xxx) Fixes review comment on PR openclaw#32755 (cherry picked from commit fc20106)
The Feishu Lark SDK can throw exceptions (SDK errors with .code or AxiosErrors with .response.data.code) for withdrawn/deleted reply targets, in addition to returning error codes in the response object. Wrap reply calls in sendMessageFeishu and sendCardFeishu with try-catch to handle thrown withdrawn/not-found errors (230011, 231003) and fall back to client.im.message.create, matching the existing response-level fallback behavior. Also extract sendFallbackDirect helper to deduplicate the direct-send fallback block across both functions. Closes openclaw#33496 (cherry picked from commit ad0901a)
(cherry picked from commit c129a69)
(cherry picked from commit f85ec61)
mrosmarin
added a commit
to mrosmarin/openclaw
that referenced
this pull request
Mar 5, 2026
* main: fix(feishu): comprehensive reply mechanism — outbound replyToId forwarding + topic-aware reply targeting (openclaw#33789) fix: restore auto-reply system events timeline (openclaw#34794) (thanks @anisoptera) (openclaw#34794) docs(changelog): document dependency security fixes fix(deps): bump tar to 7.5.10 fix(security): avoid prototype-chain account path checks (openclaw#34982) fix(deps): patch hono transitive audit vulnerabilities fix: add spanish locale support (openclaw#35038) (thanks @DaoPromociones) fix: finalize spanish locale support Changelog: add gateway restart health entry (openclaw#34874) Changelog: add daemon systemd user-bus fallback entry (openclaw#34884) fix: align AGENTS.md template section names with post-compaction extraction (openclaw#25029) (openclaw#25098) agents: preserve totalTokens on request failure instead of using contextWindow (openclaw#34275) Fix Linux daemon install checks when systemd user bus env is missing (openclaw#34884)
Kansodata
pushed a commit
to Kansodata/openclaw
that referenced
this pull request
Mar 5, 2026
…rding + topic-aware reply targeting (openclaw#33789) * fix(feishu): comprehensive reply mechanism fix — outbound replyToId forwarding + topic-aware reply targeting - Forward replyToId from ChannelOutboundContext through sendText/sendMedia to sendMessageFeishu/sendMarkdownCardFeishu/sendMediaFeishu, enabling reply-to-message via the message tool. - Fix group reply targeting: use ctx.messageId (triggering message) in normal groups to prevent silent topic thread creation (openclaw#32980). Preserve ctx.rootId targeting for topic-mode groups (group_topic/group_topic_sender) and groups with explicit replyInThread config. - Add regression tests for both fixes. Fixes openclaw#32980 Fixes openclaw#32958 Related openclaw#19784 * fix: normalize Feishu delivery.to before comparing with messaging tool targets - Add normalizeDeliveryTarget helper to strip user:/chat: prefixes for Feishu - Apply normalization in matchesMessagingToolDeliveryTarget before comparison - This ensures cron duplicate suppression works when session uses prefixed targets (user:ou_xxx) but messaging tool extract uses normalized bare IDs (ou_xxx) Fixes review comment on PR openclaw#32755 (cherry picked from commit fc20106) * fix(feishu): catch thrown SDK errors for withdrawn reply targets The Feishu Lark SDK can throw exceptions (SDK errors with .code or AxiosErrors with .response.data.code) for withdrawn/deleted reply targets, in addition to returning error codes in the response object. Wrap reply calls in sendMessageFeishu and sendCardFeishu with try-catch to handle thrown withdrawn/not-found errors (230011, 231003) and fall back to client.im.message.create, matching the existing response-level fallback behavior. Also extract sendFallbackDirect helper to deduplicate the direct-send fallback block across both functions. Closes openclaw#33496 (cherry picked from commit ad0901a) * feishu: forward outbound reply target context (cherry picked from commit c129a69) * feishu extension: tighten reply target fallback semantics (cherry picked from commit f85ec61) * fix(feishu): align synthesized fallback typing and changelog attribution * test(feishu): cover group_topic_sender reply targeting --------- Co-authored-by: Xu Zimo <[email protected]> Co-authored-by: Munem Hashmi <[email protected]> Co-authored-by: bmendonca3 <[email protected]> Co-authored-by: Tak Hoffman <[email protected]>
Summary
sendText/sendMediadroppedreplyToIdfromChannelOutboundContext, making reply-to-message impossible via themessagetool.ctx.messageIdtoctx.rootId ?? ctx.messageIdglobally. This works for topic-mode groups but causes normal group replies to silently enter topic threads (fix(feishu): group reply targets root message instead of the triggering message #32980, [Bug]: Feishu topic group - Bot creates new thread instead of replying in existing topic #32958).replyToIdthroughsendOutboundTextandsendMediaFeishuin the outbound adapter.ctx.rootIdonly in topic-mode groups (group_topic/group_topic_sender) or whenreplyInThreadis explicitly enabled in config. Normal groups always reply toctx.messageId.Change Type
Scope
Linked Issue/PR
replyTargetconfig; our approach auto-detects without new config)replyToId/threadIdforwarding; our PR includes this fix)ctx.messageId; our PR preserves topic-mode behavior)User-visible / Behavior Changes
messagetool withreplyTonow correctly create Feishu reply messages.Security Impact
Repro + Verification
Steps (Problem 2 — #32980)
replyInThread)root_id→ enters invisible topic threadmessage_id→ visible in main chatSteps (Problem 1 — outbound replyToId)
messagetool to send a Feishu message withreplyToparameterreplyToIdwas silently droppedEvidence
pnpm checkpasses (format + types + lint)Human Verification
replyToIdforwarding through both text and card render pathsroot_idreplies to triggering messageroot_idreplies to topic rootreplyInThread: "enabled"preserves root-targeting behaviorCompatibility / Migration
Failure Recovery
Relation to Other PRs
replyTargetconfig optiongroupSessionScope— no new config neededreplyToId/threadIdforwardingctx.messageIdrootIdfor topic-mode groups and explicitreplyInThread