fix(msteams): preserve channel thread isolation during proactive fallback#59314
fix(msteams): preserve channel thread isolation during proactive fallback#59314tazmon95 wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes channel thread isolation during proactive fallback in MS Teams: it stores the thread root message ID on the conversation reference and uses it in Two minor P2 concerns: the Confidence Score: 5/5
|
| const proactiveRef = | ||
| isChannel && ref.threadRootMessageId | ||
| ? { | ||
| ...ref, | ||
| conversation: { | ||
| ...ref.conversation!, | ||
| id: `${ref.conversation!.id};messageid=${ref.threadRootMessageId}`, | ||
| }, | ||
| } | ||
| : ref; |
There was a problem hiding this comment.
conversation.id mutation is a no-op
The ;messageid= suffix appended here is immediately stripped by normalizeConversationId (which splits on ;) inside buildConversationReference, then re-appended by sendProactively from threadRootMessageId. The thread routing is driven entirely by threadRootMessageId on the ref — the conversation.id mutation in send.ts has no observable effect and could mislead future maintainers into thinking it's load-bearing.
Consider removing the conversation override here and passing ref directly, since sendProactively already handles appending ;messageid=THREAD_ROOT when threadRootMessageId is set:
| const proactiveRef = | |
| isChannel && ref.threadRootMessageId | |
| ? { | |
| ...ref, | |
| conversation: { | |
| ...ref.conversation!, | |
| id: `${ref.conversation!.id};messageid=${ref.threadRootMessageId}`, | |
| }, | |
| } | |
| : ref; | |
| const proactiveRef = ref; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/send.ts
Line: 337-346
Comment:
**Redundant `conversation.id` mutation is a no-op**
The `;messageid=` suffix appended here is immediately stripped by `normalizeConversationId` (which splits on `;`) inside `buildConversationReference`, then re-appended by `sendProactively` from `threadRootMessageId`. The thread routing is driven entirely by `threadRootMessageId` on the ref — the `conversation.id` mutation in `send.ts` has no observable effect and could mislead future maintainers into thinking it's load-bearing.
Consider removing the `conversation` override here and passing `ref` directly, since `sendProactively` already handles appending `;messageid=THREAD_ROOT` when `threadRootMessageId` is set:
```suggestion
const proactiveRef = ref;
```
How can I resolve this? If you propose a fix, please make it concise.| // No TurnContext available (proactive send path). Go straight to sendProactively | ||
| // with preserveThread=true so the message lands in the thread. | ||
| // The conversationRef already has ;messageid=THREAD_ROOT in conversation.id. | ||
| return await sendProactively(messages, 0); |
There was a problem hiding this comment.
conversation.id
The comment "The conversationRef already has ;messageid=THREAD_ROOT in conversation.id" is inaccurate: buildConversationReference normalises the ID by splitting on ; before sendProactively ever sees it. The suffix that ultimately lands in the proactive reference is re-appended by sendProactively itself from threadRootMessageId. The comment should describe what sendProactively does, not what send.ts put into conversation.id.
| // No TurnContext available (proactive send path). Go straight to sendProactively | |
| // with preserveThread=true so the message lands in the thread. | |
| // The conversationRef already has ;messageid=THREAD_ROOT in conversation.id. | |
| return await sendProactively(messages, 0); | |
| // No TurnContext available (proactive send path). Fall through to sendProactively, | |
| // which will set activityId and ;messageid= on the conversation reference when | |
| // threadRootMessageId is present, routing the reply into the correct thread. | |
| return await sendProactively(messages, 0); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/messenger.ts
Line: 534-537
Comment:
**Misleading comment about `conversation.id`**
The comment "The conversationRef already has `;messageid=THREAD_ROOT` in `conversation.id`" is inaccurate: `buildConversationReference` normalises the ID by splitting on `;` before `sendProactively` ever sees it. The suffix that ultimately lands in the proactive reference is re-appended by `sendProactively` itself from `threadRootMessageId`. The comment should describe what `sendProactively` does, not what `send.ts` put into `conversation.id`.
```suggestion
// No TurnContext available (proactive send path). Fall through to sendProactively,
// which will set activityId and ;messageid= on the conversation reference when
// threadRootMessageId is present, routing the reply into the correct thread.
return await sendProactively(messages, 0);
```
How can I resolve this? If you propose a fix, please make it concise.|
Addressed in 8b6d0e1 — The 462/462 tests still pass. — Fitzy 🐾 |
…back When a bot reply falls back to proactive messaging in a Teams channel thread (due to turn context revocation), the proactive reference now carries the original thread root message ID. This ensures the reply lands in the correct thread instead of posting as a new top-level message. Changes: - conversation-store.ts: add threadRootMessageId to StoredConversationReference - message-handler.ts: populate threadRootMessageId from the inbound activity's ;messageid=… suffix (already parsed by extractMSTeamsConversationMessageId) - messenger.ts: sendProactively() sets activityId to threadRootMessageId for channel conversations (DMs unchanged). When replyStyle=thread but no TurnContext is available, fall through to sendProactively instead of throwing. - send.ts: sendTextWithMedia() detects channel threads and uses replyStyle=thread with the ;messageid= suffix instead of top-level. - messenger.test.ts: four new tests covering channel thread isolation, DM behavior preservation, cross-thread contamination prevention, and proactive thread fallback without context. AI-assisted: yes (Claude). Fully tested (pnpm test:extension msteams — 462/462 pass).
…le sends Address review feedback: sendProactiveActivityRaw (used by SharePoint file cards, OneDrive markdown links, and FileConsentCard paths) was still using activityId: undefined, causing file responses in channel threads to post top-level. Apply the same thread-aware logic so all proactive send paths respect threadRootMessageId. AI-assisted: yes (Claude). Fully tested (462/462 pass).
8ad5af9 to
e2f5744
Compare
|
Rebased onto current
All original test cases preserved + adapted for the merged logic. — Fitzy 🐾 |
|
Confirming this bug still reproduces on v2026.4.21 (installed on a production OpenClaw deployment with MS Teams). Reproduction
Gateway log excerpt (timestamps sanitized)HH:MM:00 msteams: received messageHH:MM:00 msteams: dispatching to agent Total turn duration: ~69 seconds. The session key shows the thread id was correctly tracked inbound — the thread context is lost only at the proactive send step, exactly as this PR diagnoses. ImpactUsers perceive replies as appearing out-of-order in unrelated threads, breaking the threaded conversation model. With active-memory enabled (which adds ~6s baseline on top of any tool-call time), most channel turns exceed the 15s window and hit this path. ValidationThe Would be great to see this merged. Happy to help validate on our deployment if that helps land it. Thanks @Fitzy for the work. |
|
@grafoteka - thanks for the tag, however I think on this occasion the wrong Fitzy handle has definitely been tagged 😁 |
|
My apologies @Fitzy — I tagged the wrong handle. The PR author is @tazmon95 (the "Fitzy 🐾" signature in the commits is the same person under their real GitHub handle). Sorry for the noise. @tazmon95 — the previous message stands: production confirmation on v2026.4.21 holds, happy to help validate this PR on our deployment. |
|
Codex review: needs changes before merge. Summary Reproducibility: yes. The discussion gives a production repro on v2026.4.21 with a Teams channel thread, a long tool-using turn, and proactive fallback posting top-level; code inspection also shows current Next step before merge Security Review findings
Review detailsBest possible solution: Reuse current main's Do we have a high-confidence way to reproduce the issue? Yes. The discussion gives a production repro on v2026.4.21 with a Teams channel thread, a long tool-using turn, and proactive fallback posting top-level; code inspection also shows current Is this the best way to solve the issue? No as written. The intended routing fix is valid, but adding Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 473fc0aad84a. |
Summary
When a bot reply in a Teams channel thread falls back to proactive messaging (due to turn context revocation), the reply previously posted as a new top-level message instead of staying in the thread. This PR fixes that by carrying the thread root message ID through the proactive send path.
Problem
In Microsoft Teams channels, conversations happen in threads. When OpenClaw receives a message in a thread, it responds via
TurnContext.sendActivity(). However, Teams revokes theTurnContextafter ~15 seconds, forcing a fallback tocontinueConversation()(proactive messaging). The proactive path was not thread-aware — it always sent to the channel top-level, breaking thread isolation.This caused:
Fix
4 files changed, 5 files touched (including tests):
conversation-store.ts: Add
threadRootMessageIdtoStoredConversationReference— stores the thread root ID extracted from the inbound activity's;messageid=…suffix.message-handler.ts: Populate
threadRootMessageIdfrom the already-parsedconversationMessageIdwhen upserting conversation references.messenger.ts:
sendProactively()now setsactivityIdtothreadRootMessageIdfor channel conversations and appends;messageid=THREAD_ROOTtoconversation.idsocontinueConversation()routes to the correct thread.activityIdremainsundefined).replyStyle=threadbut noTurnContextis available, fall through tosendProactively()instead of throwing.send.ts:
sendTextWithMedia()(message tool proactive path) detects channel threads and usesreplyStyle: "thread"with the;messageid=suffix instead of"top-level".Testing
pnpm test:extension msteams)activityIdandconversation.idactivityId(preserves existing behavior)replyStyle=threadwith no context falls through tosendProactivelyinstead of throwingReal-world validation
This fix has been running in production on a self-hosted OpenClaw instance (2026.3.31) with Microsoft Teams for ~8 hours with correct thread routing confirmed across multiple channel threads and DMs.
AI Disclosure
— Fitzy 🐾