fix(msteams): route attachment proactive sends through channel threads (fixes #88836)#94522
Closed
zenglingbiao wants to merge 1 commit into
Closed
Conversation
Contributor
Author
|
@clawsweeper re-review |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
1 task
Contributor
Author
|
关闭原因:CI 多项检查失败 — Auto response FAILURE, Dependency Guard FAILURE, Labeler FAILURE, Security Sensitive Guard FAILURE, CodeQL NEUTRAL。4个竞争者抢同一个issue,不再维护此PR。 |
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
replyStyle: thread, messages containing file attachments arrive as new top-level messages instead of inside the channel thread they belong to.sendProactiveActivityRaw(used for all three attachment send paths — FileConsentCard, SharePoint native file card, OneDrive fallback link) callssendMSTeamsActivityWithReferencewithout passingthreadActivityId. The text-only path (sendTextWithMedia→sendMSTeamsMessages) correctly passes it, but the attachment paths bypass that function and lose the thread routing.ref.threadId ?? ref.activityId) insendMessageMSTeamsfor channel conversations and pass it throughsendProactiveActivityRaw→sendMSTeamsActivityWithReference, following the same pattern already used insendMSTeamsMessages.extensions/msteams/src/send.ts— addthreadActivityIdtoProactiveActivityParams/ProactiveActivityRawParamstypes; wire it throughsendProactiveActivityRawandsendProactiveActivity; resolve and passresolvedThreadIdfromsendMessageMSTeamsfor the three attachment pathsReproduction
channels.msteams.replyStyle: threadReal behavior proof
Behavior or issue addressed (#88836): In
sendMessageMSTeams, attachment proactive sends (FileConsentCard, SharePoint file card, OneDrive fallback link) now carrythreadActivityIdso they route into the correct channel thread. Before this fix,sendProactiveActivityRawalways omittedthreadActivityId— the attachment message would post at the top level even whenreplyStyleisthread.Real environment tested: Linux, Node 22 — msteams extension test runner via
test/vitest/vitest.extension-msteams.config.tsExact steps or command run after this patch:
node scripts/run-vitest.mjs extensions/msteams/src/send.test.tsEvidence before fix:
The existing 11 tests all pass because none of them verify thread routing for proactive attachment sends. The missing coverage is the bug —
sendProactiveActivityRawwas never tested forthreadActivityIdpassthrough.Evidence after fix:
The new test ("passes threadActivityId to proactive attachment sends for channel thread routing") sets up a channel conversation with
threadId: "thread-root-msg-1", sends a message with a SharePoint file attachment, and asserts thatsendMSTeamsActivityWithReferencereceivesoptions.threadActivityId === "thread-root-msg-1".Observed result after fix:
sendMSTeamsActivityWithReferenceis called withthreadActivityIdset to the resolved thread root ID when the conversation is a channel. The new test assertion passes, confirming the fix routes attachment messages into the correct channel thread.What was not tested: Live Microsoft Teams round-trip with an actual Bot Framework conversation reference — the test uses mocked SDK calls. The
sendProactiveActivityRawlogic mirroringsendMSTeamsMessages's existing channel guard (isChannel ? threadActivityId : undefined) was verified through code-path analysis rather than a separate test case.Repro confirmation: The new test case verifies the fix by asserting
options.threadActivityIdequals the expected thread root on thesendMSTeamsActivityWithReferencemock. Before the fix, this assertion would fail becausethreadActivityIdwas never passed throughsendProactiveActivityRaw.Risk / Mitigation
threadActivityIdcould be stale or incorrect for some edge-case conversations.Mitigation: Uses the same resolution pattern (
ref.threadId ?? ref.activityId) already battle-tested insendMSTeamsMessageslines 534-536. TheisChannelguard mirrors the same condition used in the messenger.sendProactiveActivitypath.Mitigation: Those are separate features not covered by this bug report. The fix pattern is the same (add
threadActivityIdto the call), so they can be addressed in a follow-up if bugs are reported.Change Type (select all)
Scope (select all touched areas)
Regression Test Plan
node scripts/run-vitest.mjs extensions/msteams/src/send.test.ts— 12 tests including the new thread routing assertionReview Findings Addressed
N/A (initial submission)
Linked Issue/PR
Fixes #88836