feat(feishu): add group thread isolation lists#1446
Open
luoye-fe wants to merge 1 commit into
Open
Conversation
Author
|
@chenhg5 hi,帮忙 review 下这个 pr 吧,主要是有些群聊希望按话题形式回复,有些群聊希望用同一个上下文持续对话 |
chenhg5
approved these changes
Jun 29, 2026
chenhg5
left a comment
Owner
There was a problem hiding this comment.
结论: Approve
总体判断: 这是一个 well-scoped 的 Feishu platform 扩展, 给现有 thread_isolation 全局开关加上 per-group 黑白名单覆盖, 同时保证 native Feishu topic 消息始终走 topic-thread reply (跟现有 thread_isolation 设置解耦)。 实现方式正确 (helper 函数抽取 + 双重 session tracking + 完整 test 覆盖), 不破坏 backward compat, 4/4 CI green + 完整 PR 模板。 当前可合并。
Review 范围:
- 看了 4 files / +284/-12:
platform/feishu/feishu.go(+245/-11) +config.example.toml(+4/-0) + 2 个 test 文件 (+35/-1)。 - 重点关注 correctness (per-group 覆盖逻辑) / native topic thread 解耦 / backward compat / test 覆盖 / 配置文件说明。
✅ 做得好的地方:
- 行为变更清晰: PR 描述明确给出 3 个新行为 —
thread_isolation_black_group(global-on 时 excluded 群保持普通 reply) /thread_isolation_white_group(global-off 时 included 群走 thread reply) / native topic 始终走 thread (跟 global 设置解耦)。 每个行为都有对应 test。 - Helper 抽取合理 (
platform/feishu/feishu.go:3346-3408): 5 个新 helper (shouldUseThreadIsolationForMessage/isNativeThreadMessage/shouldUseThreadIsolationForChat/configuredListContains/shouldTreatSessionAsThread) 每个职责单一, 跟核心 platform 文件的现有 helper 风格一致 (e.g.isAttachmentMsgType/stringValue)。shouldUseThreadIsolationForMessage把「native topic 检测」和「chat-level 配置」封装到一个函数, 主流程makeSessionKey调用变干净。 - Native topic 解耦做得对 (
platform/feishu/feishu.go:174 + 3342-3352): 引入nativeThreadSessions sync.Map单独追踪 native topic 消息, 跟activeThreadSessions分开, 避免跟 global thread_isolation 状态混淆。markNativeThreadSession/isNativeThreadSession配对使用, 跟现有markThreadSessionActive/isActiveThreadSession模式一致。 shouldUseThreadContext集成自然 (platform/feishu/feishu.go:1438): 把p.threadIsolation && isThreadSessionKey(sessionKey)替换为p.shouldUseThreadContext(sessionKey), 考虑了 native topic + chat 黑白名单两种情况, 主流程 dispatchMessage 改动最小 (只动一行)。- Backward compat 完整保留:
markThreadSessionActive保留为 wrapper 调用新markThreadSessionActiveForContext(sessionKey, false), 现有调用方不需要改;isActiveThreadSession/makeSessionKey内部逻辑替换, 但外部 contract 不变 (group + global-on 仍按 thread 隔离, group + global-off 仍按 chat-id 共享 session)。 - Test 覆盖全面 (
platform/feishu/platform_test.go+feishu_test.go):TestNewFeishu_ThreadIsolationGroupLists— 验证新 option parsingTestFeishu_MakeSessionKeyThreadIsolationGroupLists— 5 个 case (global-on blacklist / global-off whitelist / native topic / private chat / shared-channel)TestBuildReplyMessageReqBody_SetsReplyInThreadFlag— extended 验证 blacklist/whitelist/native-topic 的 reply-thread 决策TestMarkAndIsActiveThreadSession— extended 验证 active thread tracking 跟新规则一致
每个 test 都有清晰的 case 命名, 跟前一轮 review 反馈一致 (test 应覆盖「global-on blacklist」+「global-off whitelist」两个对称 case)。
- PR 模板完整: 包括 Summary / Type of change / Testing (4 个 test + 命令) / CUJ impact (No CUJ touched) / Manual behavior change / Checklist / Related。 报告诚实标明「
go test ./...在 pre-existing 环境敏感 test 失败 (TempDir RemoveAll cleanup / launchd status), 跟本 PR 改动无关」。 - config.example.toml 注释完整 (
config.example.toml:1008-1011): 每个新 option 都有中英双语说明 + 行为解释, 跟现有 Feishu config block 风格一致。
🔴 必须处理: 无。
🟠 建议改进:
- 🟠 P2:
configuredListContains(platform/feishu/feishu.go:3389) 跟现有core.AllowList都有 trim/empty 处理逻辑, 看起来configuredListContains等价于strings.TrimSpace(list) != "" && core.AllowList(list, value)。 建议直接用core.AllowList一个函数 (假定core.AllowList已经处理了 trim 和 empty), 减少一层间接。 但这不阻塞本 PR, 未来 cleanup 即可。
验证: 检查core.AllowList源码确认是否已处理 trim/empty。
影响: 微, 主要是函数调用层次清晰度。
🔵 可选优化:
- 未来考虑: 把
threadIsolationBlackGroup/threadIsolationWhiteGroup拆成独立的core.AllowListConfigstruct 复用 (类似allow_from), 跟core.AllowList配套。 但本 PR 用字符串 + 逗号分隔跟现有allow_from一致, 改动范围保持小是正确的取舍。
❓ 需要确认: 无。
Testing / Risk:
- 已验证: 5/5 CI checks PASS (lint 1m58s / unit-test 16m49s / smoke 33s / regression 31s / performance 48s) + 作者本地
go test ./platform/feishu+go test ./...(Feishu 部分全 PASS, 标出 pre-existing 环境失败)。 - 覆盖: option parsing + 5 case session-key (global-on blacklist / global-off whitelist / native topic / private / shared) + reply-thread decision + active thread tracking。
- 未覆盖 (不在本 PR 范围, 未来 follow-up): 黑白名单同时配置时的优先级 (没在 PR 描述里说, 看起来是「black > white, native 优先」但 code 没显式处理互斥配置)。
- 剩余风险: 低。 修改面限制在 Feishu platform 包内, 不动 core/engine/session。 backward compat 完整保留。
Next step:
- 作者: P2 是 nit, 跟 cleanup 一起做即可。 考虑未来 cleanup
configuredListContains直接用core.AllowList。 - owner: 这是一个 additive feature, 不破坏现有部署, 可以安全 merge 进 v1.4.x。 建议 release notes 加一条 「Feishu: per-group thread_isolation 黑白名单 + native topic 始终 thread」, 方便运营根据具体群配置 reply 行为。
- QA: 本次 review 后会
pr set --workflow-state ready-to-merge+render。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add per-group controls for Feishu
thread_isolationso deployments can opt specific regular groups in or out without changing private-chat behavior. Native Feishu topic messages are always kept in topic-thread reply mode, even when globalthread_isolationis disabled.Type of change
Testing
Automated tests added in this PR
TestNewFeishu_ThreadIsolationGroupListsinplatform/feishu/platform_test.go— verifies the new Feishu platform options are parsed into the platform.TestFeishu_MakeSessionKeyThreadIsolationGroupListsinplatform/feishu/platform_test.go— verifies global-on blacklist, global-off whitelist, native topic, private chat, and shared-channel session-key behavior.TestBuildReplyMessageReqBody_SetsReplyInThreadFlaginplatform/feishu/platform_test.go— extended to verify blacklist/whitelist/native-topic reply-thread decisions.TestMarkAndIsActiveThreadSessioninplatform/feishu/feishu_test.go— extended to verify active thread tracking honors blacklist/whitelist behavior.Commands run locally:
Both commands passed.
GOTOOLCHAIN=auto /opt/homebrew/bin/go test ./...was also run. It failed on unrelated local/environment-sensitive tests outside this change:core:TestCUJ_A3_ImageReachesAgentandTestCUJ_A5_FileReachesAgentfailed duringTempDir RemoveAll cleanup: directory not empty.daemon:TestLaunchdStatusUsesUserDomainWhenGUIDomainUnavailablefailed withStatus().Running = false, want true.All Feishu platform tests passed in that full-suite run.
For bug fixes only — regression test
Critical User Journeys (CUJ) impact
/new/switch/list/historyetc.)/mode/cancel/stoppermissions)allow_fromadmin_frombanned_wordsrate limits)/cron/timer)/lang/provider/modelreload)If any CUJ group is touched, confirm:
go test ./core/ -run TestCUJpasses locally.CUJ test was updated (or a new CUJ added) to cover the new behavior.
Manual / user-visible behavior change
Feishu platform users can now configure:
thread_isolation_black_group: whenthread_isolation = true, listed group chat IDs keep normal non-topic replies.thread_isolation_white_group: whenthread_isolation = false, listed group chat IDs use topic-thread replies.Native Feishu topic messages continue to reply in the topic thread regardless of the global
thread_isolationsetting. Private chat behavior is unchanged.Checklist (reviewer will verify)
go build ./...passesgo test ./...passes (with-raceif touching concurrency)core/Related