Skip to content

fix(wecom): preserve quoted messages as agent context - #1669

Open
TongLing916 wants to merge 2 commits into
chenhg5:mainfrom
TongLing916:main
Open

fix(wecom): preserve quoted messages as agent context#1669
TongLing916 wants to merge 2 commits into
chenhg5:mainfrom
TongLing916:main

Conversation

@TongLing916

Copy link
Copy Markdown

WeCom's WebSocket callback exposed quote payloads, but the inbound parser merged them with the current message. That made quoted text and attachments indistinguishable from a new instruction for the agent.

Separate current and quoted message parts at the adapter boundary, format quotes through the existing ExtraContent path, and retain attachment markers plus quote-first attachment ordering.

Constraint: The WeCom long-connection payload does not include the quoted sender identity
Rejected: Add quote-specific fields to core.Message | Existing ExtraContent flow already preserves contextual prefixes across engine processing
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Keep quote data separate from current message text before mention stripping
Tested: go test ./platform/wecom; go test ./core
Not-tested: Full repository build requires missing web/dist embed assets

Summary

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing behavior to change)
  • Documentation only
  • Internal refactor / chore (no user-visible change)

Testing

Automated tests added in this PR

  • TestX_Y_Z in path/to/file_test.go — what it asserts

For bug fixes only — regression test

  • Regression test name: Test...
  • Manual verification this test catches the regression:
    • Reverted the fix locally; the regression test failed as expected.

Critical User Journeys (CUJ) impact

  • No CUJ touched (small refactor, doc change, etc.)
  • A — basic conversation
  • B — session lifecycle (/new /switch /list /history etc.)
  • C — agent execution control (/mode /cancel /stop permissions)
  • D — security & permissions (allow_from admin_from banned_words rate limits)
  • E — scheduled tasks (/cron /timer)
  • F — config switching (/lang /provider /model reload)
  • G — error handling & robustness (LLM failure, ws reconnect, agent crash)
  • H — multi-platform / multi-project isolation
  • I — UI rendering correctness (cards, streaming, display modes)

If any CUJ group is touched, confirm:

  • go test ./core/ -run TestCUJ passes locally.
  • If the change alters an existing user-visible flow, the corresponding
    CUJ test was updated (or a new CUJ added) to cover the new behavior.

Manual / user-visible behavior change

Checklist (reviewer will verify)

  • go build ./... passes
  • go test ./... passes (with -race if touching concurrency)
  • AGENTS.md Pre-Commit Checklist items are satisfied
  • No new hardcoded platform/agent names in core/
  • i18n strings have all-language translations (if any new user-facing text)
  • No secrets / credentials in source

Related

  • Issue:
  • Related PR:

WeCom's WebSocket callback exposed quote payloads, but the inbound parser merged them with the current message. That made quoted text and attachments indistinguishable from a new instruction for the agent.

Separate current and quoted message parts at the adapter boundary, format quotes through the existing ExtraContent path, and retain attachment markers plus quote-first attachment ordering.

Constraint: The WeCom long-connection payload does not include the quoted sender identity
Rejected: Add quote-specific fields to core.Message | Existing ExtraContent flow already preserves contextual prefixes across engine processing
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Keep quote data separate from current message text before mention stripping
Tested: go test ./platform/wecom; go test ./core
Not-tested: Full repository build requires missing web/dist embed assets
@TongLing916
TongLing916 requested a review from chenhg5 as a code owner August 12, 2026 03:12
Staticcheck rejected the split declaration and assignment of the local mixed-message walker, blocking CI despite unchanged behavior.

Constraint: Preserve the existing WeCom message parsing behavior
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Keep this helper as a direct local closure unless it needs recursive behavior
Tested: go test ./platform/wecom
Not-tested: Full golangci-lint run is locally blocked because web/dist is absent

@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.

结论: Approve

总体判断: 一个干净的 refactor,把 WeCom WebSocket inbound parser 从「合并 quote + current」改成「分离 current + quoted + 用 ExtraContent 注入」。架构变化正确,blast radius 仅限于 wecom package,所有测试点已更新为新签名,CI 全绿。建议合入。

Review 范围:

  • 看了 platform/wecom/websocket_media.go 引入的 wsInboundParts struct、formatWSQuotedContent helper、以及 wsCollectInboundParts 的拆分逻辑。
  • 看了 platform/wecom/websocket.gohandleMsgCallback 三条分支(voice / 纯文本 / 含媒体)的改写,确认都正确线程 ExtraContent + current/quoted。
  • 比对了所有 wsCollectInboundParts 调用点(4 个测试 + 1 个生产调用),全部更新为新 (current, quoted) 签名。
  • CI 验证: run 31560110392 全绿(lint / unit-test / smoke / regression / performance)。

✅ 做得好的地方:

  • 架构清晰wsInboundParts{content, images, files} 把「读得懂的字符串 / 媒体引用 / quoted vs current」分到正交维度,比之前 (texts []string, imgs, files []wsMediaRef) 平铺结构更可扩展。hasMedia() 方法用得克制,避免在每个 if 里重复 len(...)>0
  • 复用现有 ExtraContent 通道:作者明确拒绝「在 core.Message 加 quote-specific 字段」方案(PR body 里写了 constraint + rejected),选择走 ExtraContent —— 这是已经被 feishu / telegram / discord 等平台使用过的稳定 contract,engine 处理路径不动。
  • Attachment 顺序保留downloadImages(quoted.images)downloadImages(current.images) 之前——这样最终 Images 数组的顺序与「quoted 上下文 → 当前内容」语义对齐。TestDeliverWSMediaInbound_QuotedAttachmentsPrecedeCurrentAttachments 用 httptest server + 真实下载验证顺序,这是少数能稳定验证「下载顺序」contract 的好测试。
  • Download 失败时的 context marker 保留:quoted 图片 URL 失效时仍保留 [image] 占位符 + 完整内容上下文(TestDeliverWSMediaInbound_QuotedDownloadFailureKeepsContextMarker)——避免「图片下载失败 → 上下文完全消失」的回归。
  • Mixed 边界 case 处理TestWsCollectInboundParts_fileWithNonEmptyMixedUsesTopLevelFile 覆盖 WeCom 把 top-level file/image 对象 + non-empty mixed block 同时发的真实场景,下载时取 top-level URL(这是 SDK 已知行为)。

🚨/🔴 必须处理:

  • 未发现。

🟠 建议改进(不阻塞):

  • Voice 分支的行为微改:旧代码 out := []string{vt}; out = append(out, texts...) 把 voice transcription 拼在前面。新代码 current.content = append([]string{vt}, current.content...) 也是 prepend,但因为 current.content 已经过 appendText 里的 strings.TrimSpace,顺序与旧实现等价——OK。但建议在 voice 分支加一个 fixture 测试 TestWsCollectInboundParts_VoiceTranscriptionPrependedToCurrent,把这条契约钉死,避免未来重构时改回 append(拼到末尾)破坏 IM 显示。当前 voice 路径没有针对 quoted+voice 的测试覆盖。
  • formatWSQuotedContent 标签本地化:返回 "[Quoted message]:" 是硬编码英文。其他 platform 类似 contract 是否走 i18n?建议确认这条是否需要走 i18n key(如 [%s] 占位),避免后续多语言用户看到不一致前缀。
  • fromVoice 参数传播到 deliverWSMediaInbound:新增参数让函数多了条隐藏路径(voice + media 走 deliverWSMediaInbound 而不是直接 handler)。建议在 deliverWSMediaInbound 顶部加一行注释明确 fromVoice=true 时 FromVoice 字段的意义,与 handleMsgCallbackContent: content, FromVoice: true 的赋值保持一致——目前是正确的,但意图对 future reader 不直观。

🔵 可选优化:

  • quoted.hasMedia() 这个方法只在 voice 分支 + 纯文本分支用了一次。可以考虑 inline,但留着方法化可读性更好。低优先
  • PR body 描述精简,但 CI 测试矩阵完整——可以补充「Test plan」段加上 4 个新测试名字,方便 reviewer 快速对照。

❓ 需要确认:

  • 无。

Testing / Risk:

  • 已看到的验证: CI 全绿;4 个新 + 4 个改的测试都通过;httptest mock server 验证 quoted 媒体下载顺序与失败保留。
  • 未覆盖风险: WeCom 真实 webhook payload 在「quote 内嵌 mixed with 多种 msg_type」的复杂组合——TestWsCollectInboundParts_SeparatesQuotedMixedContent 已经覆盖基础场景,但生产中可能出现的「quote 嵌套 quote」或「quote 与 current 都有 voice」未测。建议作者在本机用真实 WeCom aibot 跑一次 30 分钟的 IM 流量冒烟,验证 quote → context 注入到 LLM 的内容是否符合预期。
  • Blast radius: 仅 platform/wecom/,不影响其他 platform。core.Message.ExtraContent 字段已存在,无 schema 变更。

Next step:

  • 建议 owner 直接 merge。Scope 控制得当(仅 wecom package),fix 一个真实用户痛点(WeCom 用户引用消息时,引用内容被当成新指令执行),架构选择合理(复用 ExtraContent contract),测试覆盖关键边界(顺序、下载失败、mixed)。

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