Skip to content

fix(feishu): enrich reply context for card and file replies#2144

Merged
alexhoshina merged 7 commits intosipeed:mainfrom
yangwenjie1231:fix/feishu-reply-context-v6
Apr 8, 2026
Merged

fix(feishu): enrich reply context for card and file replies#2144
alexhoshina merged 7 commits intosipeed:mainfrom
yangwenjie1231:fix/feishu-reply-context-v6

Conversation

@yangwenjie1231
Copy link
Copy Markdown
Contributor

@yangwenjie1231 yangwenjie1231 commented Mar 29, 2026

Summary

  • Enrich Feishu inbound metadata with reply linkage fields (parent_id, root_id, thread_id, reply_to_message_id) and centralize metadata building.
  • Add best-effort reply-context reconstruction for Feishu replies by resolving reply target IDs from event payload and message-detail fallback, then prepending a structured [replied_message] block.
  • Improve reply robustness for interactive/file replies: filter Feishu placeholder content, include reply media refs via implicit channel download, and provide typed fallback markers when content is unavailable.
  • Add sanitization to prevent nested tag pollution in injected reply context and expand unit tests.
  • Extract reply functions to feishu_reply.go (feishu_64.go reduced from 960 to 954 lines).
  • Add message cache (30s TTL) to avoid repeated API calls for fetchMessageByID.
  • Add early-return optimization to skip prependReplyContext for non-reply messages.
  • Add comment explaining Items[0] assumption in fetchMessageByID.

Validation

  • go test ./pkg/channels/feishu

Closes #1915

@sipeed-bot sipeed-bot bot added type: bug Something isn't working domain: channel go Pull requests that update go code labels Mar 29, 2026
Copy link
Copy Markdown
Collaborator

@yinwm yinwm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the iteration! The reply context reconstruction is well-designed — best-effort with graceful degradation, proper timeout handling, and solid test coverage.

One blocking issue:

Debug marker in production log
feishu_64.go:106replyctx-v4 in the startup log is a debug artifact that should not ship to production:

// current
logger.InfoC("feishu", "Feishu channel started (websocket mode, replyctx-v4)")
// should be
logger.InfoC("feishu", "Feishu channel started (websocket mode)")

Non-blocking suggestions for future PRs:

  1. The reply context functions (~200 lines) could be extracted to a separate feishu_reply.gofeishu_64.go is already ~960 lines.
  2. Consider a simple LRU cache for fetchMessageByID to avoid hitting Feishu API rate limits in high-traffic groups.
  3. A doc comment on the [replied_message]/[current_message] tag format would help future consumers parse the structured context.

Once the log line is cleaned up, this is ready to merge.

Comment thread pkg/channels/feishu/feishu_64.go Outdated
Comment thread pkg/channels/feishu/feishu_64.go Outdated
Comment thread pkg/channels/feishu/feishu_64.go Outdated
- Move reply-related functions to new feishu_reply.go
- Move corresponding tests to feishu_reply_test.go
- Extract magic number 600 to maxReplyContextLen constant
- Unify replyTargetID/replyTargetFromMessage (prefer parent_id, fallback root_id)
- Add source comment for containsFeishuUpgradePlaceholder
…ed media refs

- resolveReplyTargetMessageID: only call fetchMessageByID fallback when
  ThreadId is set, avoiding unnecessary API calls for non-reply messages
- prependReplyContext: prepend replied media refs before current media refs
  to maintain correct ordering
@sipeed sipeed deleted a comment Mar 31, 2026
… downloads

- Add messageCache (sync.Map) to FeishuChannel struct
- Cache fetched messages with 30s TTL to avoid re-downloading attachments
  when multiple users reply to the same parent message in a thread
- Cleanup expired entries on read access (no background goroutine needed)
@yangwenjie1231
Copy link
Copy Markdown
Contributor Author

906aded | fix(feishu): enrich reply context for card and file replies
4b47541 | refactor(feishu): extract reply functions to feishu_reply.go
a149761 | fix(feishu): skip API fallback for non-thread messages, prepend replied media refs
9627846 | fix(feishu): add message cache for fetchMessageByID (30s TTL)

Copy link
Copy Markdown
Collaborator

@yinwm yinwm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the iteration! The overall design is solid — the best-effort fallback strategy and tag sanitization are well thought out.

One blocking issue before we can merge:

Log version tag: feishu_64.go:106"replyctx-v4" in the startup log is a debug marker that will show in production. Please remove it and restore the original message:

logger.InfoC("feishu", "Feishu channel started (websocket mode)")

Non-blocking suggestions (feel free to address in a follow-up):

  • Consider an early-return in handleMessageReceive when both parent_id and root_id are empty, to skip the prependReplyContext overhead for non-reply messages.
  • The Unicode escapes in containsFeishuUpgradePlaceholder would be more readable as raw Chinese characters or a named constant.
  • fetchMessageByID assumes Items[0] is the target — a brief comment explaining this assumption would help future readers.

@yangwenjie1231
Copy link
Copy Markdown
Contributor Author

The replyctx-v4 debug marker was already removed in commit 4b47541 (current code at line 104 shows just "Feishu channel started (websocket mode)" without the version tag). Please rebase to see the latest changes.

This PR now includes all fixes:

  • replyctx-v4 removed (commit 4b47541)
  • Non-thread message optimization (commit a149761)
  • Media refs order fix (commit a149761)
  • Message cache + Items[0] comment (commits 9627846, 245946f)

@yangwenjie1231
Copy link
Copy Markdown
Contributor Author

The replyctx-v4 debug marker was already removed in commit 4b47541. Current line 104 shows: Feishu channel started (websocket mode) without any version tag.

All blocking and non-blocking items from previous review are now addressed:

  • replyctx-v4 removed (commit 4b47541)
  • Early-return for non-reply messages (commit 245946f)
  • Media refs order fix (commit a149761)
  • Message cache + Items[0] comment (commits 9627846, 245946f)

@yangwenjie1231 yangwenjie1231 requested a review from yinwm March 31, 2026 13:50
Copy link
Copy Markdown
Collaborator

@yinwm yinwm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvements — file extraction, caching, early-return, and media ref prepend all look solid. CI green too.

One blocking bug:

Duplicate prependReplyContext call in handleMessageReceive

The function is invoked twice back-to-back — first unconditionally, then again inside the conditional guard. For reply messages this causes the reply context block to be nested (the second call sees content that already contains [replied_message] tags from the first call).

// First call (unconditional) — should be removed
content, mediaRefs = c.prependReplyContext(ctx, message, chatID, content, mediaRefs)
if content == "" {
    content = "[empty message]"
}

// Second call (conditional) — keep this one
if replyTargetID(message) != "" || stringValue(message.ThreadId) != "" {
    content, mediaRefs = c.prependReplyContext(ctx, message, chatID, content, mediaRefs)
}

Fix: remove the first (unconditional) call block (~4 lines) and keep only the conditional one.

After this fix, ready to approve.

Copy link
Copy Markdown
Collaborator

@yinwm yinwm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvements across the board — file extraction, caching, early-return guard, and the prepend fix for media refs all look solid. CI green too.

.

One blocking bug:

Duplicate prependReplyContext call in handleMessageReceive

The function is called twice in sequence — first unconditionally, then again inside a conditional guard. For reply messages, this causes nested context blocks duplication.

The fix: remove the first unconditional call block (lines ~466-470) and keep only the conditional one after it fix: if content == "" { content = "[empty message]" }

Non-blocking (minor): containsFeishuUpgradePlaceholder still uses Unicode escapes — consider using raw Chinese characters for readability.
.
EOF

@yangwenjie1231 yangwenjie1231 requested a review from yinwm April 3, 2026 03:58
@yangwenjie1231
Copy link
Copy Markdown
Contributor Author

删除 handleMessageReceive第一次无条件的 prependReplyContext 调用
保留了带条件的调用(replyTargetID(message) != "" || stringValue(message.ThreadId) != "")
保留一次 if content == "" { content = "[empty message]" }

Copy link
Copy Markdown
Collaborator

@yinwm yinwm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — all blocking issues from previous rounds have been addressed. The reply context reconstruction is solid with proper best-effort degradation, caching, and sanitization.

One minor nit for a follow-up (non-blocking): the Unicode escapes in containsFeishuUpgradePlaceholder could be raw Chinese characters for readability, as suggested in the previous review.

@alexhoshina alexhoshina merged commit 8b3e502 into sipeed:main Apr 8, 2026
4 checks passed
ra1phdd pushed a commit to ra1phdd/picoclaw-pkg that referenced this pull request Apr 12, 2026
)

* fix(feishu): enrich reply context for card and file replies

* refactor(feishu): extract reply functions to feishu_reply.go

- Move reply-related functions to new feishu_reply.go
- Move corresponding tests to feishu_reply_test.go
- Extract magic number 600 to maxReplyContextLen constant
- Unify replyTargetID/replyTargetFromMessage (prefer parent_id, fallback root_id)
- Add source comment for containsFeishuUpgradePlaceholder

* fix(feishu): skip API fallback for non-thread messages, prepend replied media refs

- resolveReplyTargetMessageID: only call fetchMessageByID fallback when
  ThreadId is set, avoiding unnecessary API calls for non-reply messages
- prependReplyContext: prepend replied media refs before current media refs
  to maintain correct ordering

* fix(feishu): add message cache for fetchMessageByID to avoid repeated downloads

- Add messageCache (sync.Map) to FeishuChannel struct
- Cache fetched messages with 30s TTL to avoid re-downloading attachments
  when multiple users reply to the same parent message in a thread
- Cleanup expired entries on read access (no background goroutine needed)

* fix(feishu): early-return for non-reply messages, add cache and fetchMessageByID comment

* fix: remove duplicate test and fix gci import order

* fix(feishu): remove duplicate prependReplyContext call
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: channel go Pull requests that update go code type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants