fix(feishu): bootstrap context for first thread mention(修复机器人第一次进入话题,读不到话题内容的问题) - #1627
Merged
chenhg5 merged 2 commits intoAug 13, 2026
Merged
Conversation
Contributor
Author
|
@chenhg5
#1648 does not inject prior topic context, so it does not address #1453. |
chenhg5
approved these changes
Aug 13, 2026
chenhg5
left a comment
Owner
There was a problem hiding this comment.
结论: Approve
总体判断: 一个窄而准的 bug fix——修复 Feishu topic 里机器人第一次被 @ 时读不到 topic root 上下文的问题。设计克制,没有全量同步 thread history,只在「第一次接受到的 thread mention」一次性 bootstrap 父消息,避免引入新的 map / 新状态。建议合入。
Review 范围:
- 看了
platform/feishu/feishu.go中markThreadSessionActive的语义变化(无返回值 → 返回bool)、replyContext.bootstrapThread新字段、以及onMessage中parentID兜底逻辑。 - 看了新增测试
TestOnMessageThreadIsolationBootstrapsExistingThreadContext和更新的TestMarkAndIsActiveThreadSession。 - 旁路核对
isThreadSessionKey与activeThreadSessions的现有用法,确认LoadOrStore替换原Store的语义差异(避免 concurrent first-mark 误判)。
✅ 做得好的地方:
- 设计克制:刻意比「全量 thread history 同步」窄,只在
bootstrapThread && parentID == ""时把RootId当作parentID传给fetchQuotedMessage,复用现有 quote-injection 通道,不另起 thread-history fetcher。这与 #525(未合并)的失败原因形成对比——本 PR 不引入新的 message→session / root-context map,复用activeThreadSessions单状态机。 - API 演进合理:
markThreadSessionActive从无返回改为bool,并更新所有调用点测试;bool 语义清晰(首次激活 / 重复 / 关闭隔离 / 非 thread key),没有破坏现有调用者。 - 测试覆盖两个独立维度:(1) 新 bootstrap 路径的端到端(SessionKey、Content、ExtraContent 三件事),(2)
markThreadSessionActivebool 契约(disabled / non-thread / first / subsequent 四种 case)。回归测试覆盖明确,作者列出的「if I reverted the fix locally this test fails」路径符合预期。
🚨/🔴 必须处理:
- 未发现。
🟠 建议改进:
markThreadSessionActive内部这段:loaded := p.activeThreadSessions.LoadOrStore(sessionKey, time.Now()); if loaded { p.activeThreadSessions.Store(sessionKey, time.Now()) }实际上是「总是覆盖时间戳」+「区分是否首次」。如果意图只是区分首次激活,不应每次都覆盖时间戳(LoadOrStore已写入了首次时间,后续Store会刷新它)。建议二选一:- 若希望
isActiveThreadSession反映「最近一次互动时间」:保留双写,但加注释说明「store 刷新是有意为之,避免 stale」。 - 若只关心「是否曾经被 @ 过」:删掉
if loaded那段,后续LoadOrStore直接丢弃loaded,函数仍返回!loaded。 - 当前写法功能正确,但语义有歧义,对 future reader 不友好。
- 若希望
- 不阻塞,但建议在本 PR 或 follow-up 里加一行注释明确意图。
🔵 可选优化:
- 新测试里 mock server 的
default分支用t.Fatalf("unexpected path %s", r.URL.Path),对 SDK 自带的轮询 / 健康检查路径(如果有)可能误伤;建议加一个t.Logf+ return 200 的更宽容分支,便于后续 SDK 升级不破测试。低优先。
❓ 需要确认:
- 无。
Testing / Risk:
- 已看到的验证: CI 全绿(lint / unit / smoke / regression / performance);新增
TestOnMessageThreadIsolationBootstrapsExistingThreadContext覆盖 happy path;TestMarkAndIsActiveThreadSession覆盖 bool 契约。 - 未覆盖风险: 真实飞书环境根消息被删除 / 不可见的情况下
fetchQuotedMessage会拿到什么(与现有 quote 路径行为一致,应该 OK,但作者没显式断言)。低优先,因为本 PR 走的就是现有 quote 通道,行为与原有 quote 处理同源。 - 并发安全: 改为
LoadOrStore之后,markThreadSessionActive在并发第一次 mark 时只有一个 goroutine 会拿到false(首次),其余拿到true(重复),bool 返回仍然 deterministic。已确认。
Next step:
- 建议 owner 直接 merge。scope 极小,fix 真实用户痛点(飞书 topic 第一次进入机器人看不到上下文),设计合理,测试充分。可以现在合,也可以攒到下次 batch。
Contributor
Author
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
Fix Feishu thread bootstrap when
thread_isolation = true.If a thread root is posted without mentioning the bot, cc-connect does not dispatch that message. When the bot is mentioned later in the same thread, the isolated agent session therefore starts empty and sees only the trigger text (for example, “看看这个”). This change detects the first accepted message in an existing isolated thread and injects its parent/root reply context once.
Later messages preserve the quote-skip behavior introduced for #764, avoiding repeated context and oversized quoted prefixes. This change is intentionally narrower than full thread-history synchronization: it bootstraps the parent/root context once and does not fetch every sibling message or other bots' replies.
Type of change
Testing
go test ./...— passed for the functional change before the lint-only follow-up.lint— passes after simplifying the condition flagged by staticcheck QF1001.Automated tests added in this PR
TestOnMessageThreadIsolationBootstrapsExistingThreadContextinplatform/feishu/feishu_test.go@bottrigger withthread_isolation = true;ExtraContent.TestMarkAndIsActiveThreadSessionnow verifies that only the first activation reports a bootstrap and subsequent activations do not.For bug fixes only — regression test
TestOnMessageThreadIsolationBootstrapsExistingThreadContextCritical 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.Manual / user-visible behavior change
Before:
@bot 看看这个in that topic.After:
thread_isolation = trueremains enabled, so unrelated topics stay isolated.Checklist (reviewer will verify)
go build ./...passesgo test ./...passes (with-raceif touching concurrency)core/Related
Unlike #525, this implementation reuses the existing
activeThreadSessionsstate and is narrowly scoped to bootstrapping the first accepted mention in an unengaged isolated thread; it does not introduce separate message-to-session or root-context maps.