Skip to content

fix(feishu): expand merge_forward content when quoting a reply - #1671

Open
re-f wants to merge 1 commit into
chenhg5:mainfrom
re-f:fix/feishu-quote-merge-forward
Open

fix(feishu): expand merge_forward content when quoting a reply#1671
re-f wants to merge 1 commit into
chenhg5:mainfrom
re-f:fix/feishu-quote-merge-forward

Conversation

@re-f

@re-f re-f commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • When a user replies to a Feishu merge_forward message, fetchSingleMessage only produced a bare [merge_forward] placeholder instead of expanding sub-message text via parseMergeForward.
  • Combined with the create_time watermark: Feishu delivers the forward and the reply nearly together; the newer reply can mark the slower async forward dispatch as stale and drop it — so the agent never sees the forwarded content even though the UI shows a non-empty forward.
  • Also select the Get-message item by message_id when the response includes merge_forward sub-messages (Items[0] may be a child).

Root cause

  1. Reply text (parent_id → merge_forward) goes through fetchQuotedMessagefetchSingleMessage, which fell into default and set text to [merge_forward].
  2. The standalone merge_forward event was parsed successfully (content_len > 0) but often dropped as stale because the reply had a newer create_time and set the watermark first.

Reproduction

  1. In Feishu P2P with the bot, merge-forward a multi-message conversation.
  2. Immediately reply to that forward (e.g. “总结这些需求”).
  3. Before this fix: agent says it only received empty [merge_forward]; logs show merge_forward sub-messages fetched then stale user message dropped for the forward, while the reply is handled with no expanded quote.

Test plan

  • go test ./platform/feishu/ -run TestDispatchMessageExpandsQuotedMergeForward -count=1
  • go test ./platform/feishu/ -count=1
  • Manual: merge-forward a chat to the bot, reply asking to summarize — agent should see <forwarded_messages> content in context

Made with Cursor

When a user replies to a merge_forward, the quote path only emitted a
"[merge_forward]" placeholder while the slower standalone forward event
was often dropped by the create_time watermark — so the agent never saw
the forwarded text. Expand via parseMergeForward and select the root
item by message_id when Get-message returns sub-messages too.

Co-authored-by: Cursor <cursoragent@cursor.com>
@re-f
re-f requested a review from chenhg5 as a code owner August 12, 2026 08:27

@chenhg5 chenhg5 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

结论: Comment(CI red 但与 PR scope 无关)

总体判断: PR 本身的改动范围干净,只动 platform/feishu/,fix 一个真实的 merge_forward quoted 消息 bug。但 CI unit-test 步骤 FAILURE,需要 author 先确认失败原因(看起来是 core/TestHandleCronExec_TriggersJob 的 pre-existing flaky test cleanup)。

Review 范围:

  • 看了 platform/feishu/feishu.gofetchSingleMessage 对 merge_forward 的处理路径(+27/-4)+ 新增测试 TestDispatchMessageExpandsQuotedMergeForward(+138/-0)。
  • 跑了本地 cron test 3 次:3/3 PASS(HEAD f2e4ed83 on main,cron package unchanged)。
  • 比对 PR diff 文件列表:仅 platform/feishu/feishu.goplatform/feishu/feishu_test.go未触碰 core/ 任何文件。所以 PR 的实际 code change 不可能引起 TestHandleCronExec_TriggersJob 失败。

🚨/🔴 必须处理(owner/author 决策):

  • CI unit-test FAILURE (run 31578417923, job 94056186707, 2026-08-12T08:32:51Z):
    • 失败测试: core/TestHandleCronExec_TriggersJob
    • 错误信息: TempDir RemoveAll cleanup: unlinkat /tmp/TestHandleCronExec_TriggersJob3543579406/001/crons: directory not empty
    • 失败耗时: 0.00s(test 本身过了,是 cleanup race 失败)
    • 下游 smoke/regression/performance 因前置失败被 SKIPPED。
  • 我的判断: 这是 pre-existing flaky test(cleanup race with async scheduler file writes),与 PR scope 无关(PR 不动 core/)。
  • 本地复现: 我在 main HEAD f2e4ed83 跑了 go test ./core/ -run TestHandleCronExec_TriggersJob -count=3 -v,3/3 PASS。
  • 建议:
    1. author push 一个空 commit(--allow-empty)触发 CI 重跑,确认是 flake。
    2. 如果重跑仍红,则需要排查 cron test 是否真有回归(与本 PR 无直接关系,可能是 main 上其他近期 commit 引入)。
    3. 如果重跑转绿,则按 flake 处理,本 PR 可合。
  • 不需要 author 修改代码,只需触发 CI 重跑。

✅ 做得好的地方(PR 本身):

  • Root cause 分析准确:作者明确指出两个独立 bug(quoted [merge_forward] placeholder + create_time watermark 竞态),并指明两个根因点(fetchSingleMessage fallback + dispatch ordering)。PR 描述结构清晰,可以直接对照 diff 验证。
  • 修复面克制——只动 feishu.go 中 quote-fetch path 的 merge_forward 分支 + select-by-message-id 选择子消息,不改 core watermark 逻辑(避免回归面爆炸)。watermark 竞态应该靠 create_time 的 ordering 保证,本 PR 不动这块。
  • 测试覆盖到位TestDispatchMessageExpandsQuotedMergeForward mock 了 SDK 返回的 Items[0] 是 child 的真实 Feishu wire 形态,断言「reply text + forwarded_messages 上下文」都注入到 handler。

🟠 建议改进(PR 本身,不阻塞):

  • fetchSingleMessage 的 merge_forward 分支逻辑没看到新增 type assertion 的兜底(例如 wire 上 upper_message_id 缺失 / items 为空时的降级)。建议作者跑一次 merge-forward + 立即删除源消息 的边界 case,确认不会因为缺数据 panic;测试可加一个 fixture。
  • watermark 竞态部分 PR 描述里提到但没改代码——可考虑另开 issue / follow-up PR 跟踪,避免被遗漏。

🔵 可选优化:

  • 无。

❓ 需要确认:

  • owner:是否接受 CI flake → re-run 后合并?还是要求先修 cron test cleanup race?
  • author:是否方便 push 空 commit 重跑 CI 验证 flake 假设?

Testing / Risk:

  • PR 本身: feishu quote path 改动小,blast radius 仅限于 feishu platform;新增测试覆盖真实 wire 形态。
  • CI 失败: 与本 PR 无关(pre-existing cron test cleanup race)。
  • Watermark 竞态: 本 PR 没改这块,独立 bug;建议单独 follow-up。

Next step:

  • author: push 空 commit 触发 CI 重跑;或 owner 决定先解决 cron test cleanup race 再合。
  • reviewer 立场: PR code 本身没问题,CI red 是独立 system-level issue,待 CI 转绿后再 review gate。

@chenhg5

chenhg5 commented Aug 13, 2026

Copy link
Copy Markdown
Owner

@re-f CI unit-test 步骤 FAILURE 但看起来是 pre-existing flaky(TestHandleCronExec_TriggersJob cleanup race,PR scope 不动 core/)。方便 push 一个空 commit 触发 CI 重跑验证一下吗?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants