feat(channel): add replyFallbackOnWithdrawn config for recalled reply targets#512
feat(channel): add replyFallbackOnWithdrawn config for recalled reply targets#512easonlh wants to merge 5 commits into
Conversation
|
关于这个 PR 的产品策略想请教下: 核心问题: 引用的消息被撤回后,机器人是否还应该回复? 已实现的配置方案:
两个典型场景:
待确认:
请给意见,谢谢! |
|
Updated: default changed to 理由:用户撤回消息 = 明确不想让内容可见,静默丢弃回复是最安全的默认行为。需要群聊回退的用户可以手动配置 另外,关于"消息撤回时应停止 AI 推理"的想法,我会向 OpenClaw 核心提一个 issue —— 这需要 channel 事件能中断正在进行的 inference,插件层面做不到。 |
evandance
left a comment
There was a problem hiding this comment.
Thanks for the fix, and for already flipping the default to silent — agreed that withdrawal is a strong signal that the user may not want the original content surfaced again, so the conservative default is the right call.
A few things to address before merge:
-
The new
getReplyFallbackMode(cfg)only readscfg.channels?.feishu.replyFallbackOnWithdrawn. It takes noaccountIdand does not resolve the merged account config viagetLarkAccount()/createAccountScopedConfig(). The same files already use this account-scoping pattern elsewhere, so an account-levelreplyFallbackOnWithdrawnoverride is silently ignored today. This should be account-aware to match the schema and the PR description. -
The inline comments in
send.ts(bothsendMessageFeishuandsendCardFeishu) and indeliver.tsstill describetop-levelas the default. Please update them to match the currentsilentdefault. -
Please add focused coverage for fallback-mode resolution, particularly the per-account override case: default
silent, top-level override, and account-scoped override.
The auto mode you raised is an interesting follow-up, but I don't think it needs to be part of this PR. The streaming card-update path and the "stop AI inference on withdrawal" piece are upstream / separate-PR concerns, so it makes sense to keep those out of this scope.
Minor: ReplyFallbackMode and getReplyFallbackMode are declared inside the import block in send.ts; please move them below the imports.
When replying to a message that has been withdrawn (230011) or deleted (231003), the Feishu API returns a terminal error. Previously the reply was silently lost. Now all three send paths (sendMessageFeishu, sendCardFeishu, sendImMessage) catch these errors and fall back to sending the message at the top level of the chat. Reuses existing isMessageUnavailableError / isTerminalMessageApiCode from core/message-unavailable.ts.
Add a configurable strategy for handling replies to withdrawn/deleted messages: - 'top-level' (default): fall back to sending as a new message - 'silent': silently discard the reply This addresses the product concern that in private chats, replying to a withdrawn message may expose content the user intended to hide, while in group chats the fallback is valuable for other participants.
When a message is withdrawn, the user's intent is clear — they don't want the content to be visible. Silently discarding the reply is the safer default. Users who need top-level fallback in group chats can opt in via config.
- Move ReplyFallbackMode/getReplyFallbackMode below imports in send.ts - Add accountId parameter to getReplyFallbackMode in both send.ts and deliver.ts - Use createAccountScopedConfig for per-account override resolution - Update comments to reflect 'silent' as the default value - Add tests for account-scoped fallback mode resolution
3b7e0f5 to
5bd7226
Compare
evandance
left a comment
There was a problem hiding this comment.
Round-1 asks all landed cleanly in 5bd7226 — thanks. Going through it a second time, four items need to clear before merge.
1. Sentinel store leak under default 'silent' (src/messaging/outbound/deliver.ts, sendTextLark). When sendImMessage silently discards a reply and returns { messageId: '', chatId: '' }, the call site still unconditionally runs recordSentinelsForChat(...), registering pending @-mention sentinels for a message that was never sent. Please skip recordSentinelsForChat on discarded sends — either check for an empty messageId in the result, or move the call into the success branch inside sendImMessage.
2. Asymmetric message-unavailable cache between deliver.ts and send.ts. send.ts wraps the API call in runWithMessageUnavailableGuard, so markMessageUnavailableFromError updates the cache before the error is thrown. The new catch block in deliver.ts reads the raw error via extractLarkApiCode(err) and never calls markMessageUnavailable, so subsequent operations on the same recalled messageId keep round-tripping the API. Please call markMessageUnavailable({ messageId: replyToMessageId, apiCode, operation }) in the terminal-code branch in deliver.ts so both paths stay symmetric.
3. CardKit streaming initial-send is uncovered. The known-limitation note in the PR body mentions mid-stream card updates. The more common pre-card flow lands in streaming-card-controller.ts:~885, which calls sendCardByCardId in cardkit.ts — that function isn't touched by this PR. So replyFallbackOnWithdrawn: 'top-level' doesn't apply to the most common streaming-card initial-create path. Please either add coverage for sendCardByCardId, or update the PR body to scope the fallback explicitly to the static reply path (sendMessageFeishu / sendCardFeishu / sendImMessage).
4. Title and label. Default 'silent' preserves current main behavior on recall (clean abort via UnavailableGuard.terminate() plus a warn log), so the user-visible new value is the opt-in 'top-level' mode. That makes it functionally a feat: rather than a fix:. Please retitle (e.g. feat(channel): add replyFallbackOnWithdrawn config for recalled reply targets); I'll update the label from bug to feature request after the retitle.
Tests for round 3: a regression test asserting 'silent' does NOT write to the sentinel store for a discarded reply, and a test asserting the deliver.ts catch path marks the unavailable cache symmetrically with send.ts.
Two non-blocking nits, can be follow-ups: getReplyFallbackMode is duplicated between send.ts and deliver.ts with identical implementation; and the as Record<string, unknown> cast in the helper bypasses what FeishuConfigSchema.extend(...) should produce on the type side.
If the next push addresses all four blockers with the test coverage above, this is ready to merge. If any remain open after one more round, I'll close this and ask for a fresh PR scoped to the correctness fixes only.
…t discard - Skip sentinel recording when sendImMessage returns empty messageId (silent discard path) to prevent stale sentinel entries - Call markMessageUnavailable in deliver.ts catch block for terminal codes (230011/231003), matching send.ts runWithMessageUnavailableGuard - Add regression tests: silent-discard sentinel guard and cache symmetry
Summary
When replying to a message that has been withdrawn (error code 230011) or deleted (231003), the Feishu API returns a terminal error. Previously the reply was silently lost. This PR adds a configurable fallback strategy so the behavior can be tuned per deployment.
Scenarios
Scenario A — Group chat (reply may be valuable):
User A asks a question in a group chat. The bot starts processing (AI inference takes a few seconds). During that time, User A withdraws the message. Other group members are still waiting for the answer. With
replyFallbackOnWithdrawn: 'top-level', the bot's response is delivered as a new message at the top level of the chat, so everyone can still see it.Scenario B — Private chat / user intent (reply should be suppressed):
User sends a message, then realizes it was a mistake and withdraws it. The bot replies anyway, exposing content the user wanted to hide. With
replyFallbackOnWithdrawn: 'silent'(default), the reply is silently discarded, respecting the user's intent.Future improvement — Stop AI inference on withdrawal:
The ideal behavior is to stop AI inference immediately when the message is withdrawn, rather than waiting for inference to complete and then discarding the reply. This would save compute and respond faster. This requires core OpenClaw support for interrupting in-progress inference based on channel events (see openclaw/openclaw#XXXX).
Configuration
New optional config field under
channels.feishu(or per-account):{ "channels": { "feishu": { "replyFallbackOnWithdrawn": "top-level" } } }'silent'(default)'top-level'Scope
This PR covers the static reply path —
sendMessageFeishu(),sendCardFeishu(), andsendImMessage()indeliver.ts. These are the code paths where a reply is sent as a single API call with no subsequent updates.Changes
src/core/config-schema.ts— AddreplyFallbackOnWithdrawnenum field toFeishuAccountConfigSchemasrc/messaging/outbound/send.ts—sendMessageFeishu()andsendCardFeishu(): read config, conditionally fallback or discardsrc/messaging/outbound/deliver.ts—sendImMessage(): accept fallback mode param, pass fromsendTextLark/sendCardLark; guard sentinel recording on silent discard; callmarkMessageUnavailablefor cache symmetry withsend.tstests/reply-fallback-mode.test.ts— Account-scoped config resolution + regression tests for sentinel leak and cache symmetryHow it works
Both
send.tsfunctions already had separate reply and create code paths. The reply path is wrapped in a try/catch that only intercepts message-unavailable errors; all other errors propagate normally. When caught, the config is checked:'silent'→ returns empty result immediately (default)'top-level'→ execution falls through to the create path belowdeliver.ts'ssendImMessagecatches the raw API error codes directly, with the fallback mode passed as a parameter from callers. On silent discard, sentinel recording is skipped (empty messageId = nothing was sent). The unavailable cache is updated symmetrically withsend.ts'srunWithMessageUnavailableGuard.Known limitation
CardKit streaming path (
sendCardByCardIdincardkit.ts): In streaming mode, the 230011 error may occur during card update (not initial send). The streaming controller catchesMessageUnavailableErrorand falls back tosendCardFeishu, which also checks the unavailable cache — so the cache-based guard already short-circuits correctly. The explicitreplyFallbackOnWithdrawnfallback logic does not apply here since the initial send already succeeded. This is an inherent limitation of the streaming architecture and does not affect the static reply paths covered by this PR.Related
@evandance Key decisions to review:
'silent'— withdrawn message = no reply (safer for most users)'top-level'is opt-in for group chat scenariosTest plan
pnpm lint— no new errorspnpm test— 295 tests pass (including 4 new regression tests)replyFallbackOnWithdrawn: 'top-level', repeat → verify reply at top level