Skip to content

fix(msteams): pass threadActivityId to attachment sends for threaded replies#93837

Closed
dwc1997 wants to merge 4 commits into
openclaw:mainfrom
dwc1997:fix/88836-msteams-attachment-threading
Closed

fix(msteams): pass threadActivityId to attachment sends for threaded replies#93837
dwc1997 wants to merge 4 commits into
openclaw:mainfrom
dwc1997:fix/88836-msteams-attachment-threading

Conversation

@dwc1997

@dwc1997 dwc1997 commented Jun 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Problem: MS Teams attachment sends (FileConsentCard, SharePoint file cards, OneDrive links) bypassed the threading logic by calling sendProactiveActivityRaw without threadActivityId. This caused reply messages with attachments to arrive as new top-level messages instead of in-thread replies when replyStyle: "thread".
  • Fix: Derive threadActivityId from ref.threadId (or ref.activityId as fallback) when replyStyle === "thread" and the conversation is a channel or group. Pass it to all three attachment send paths.
  • Out of scope: No changes to text-only message threading (already works correctly). No changes to the sendMSTeamsMessages function.

Linked context

Closes #88836

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: Reply messages with attachments arrive as top-level messages instead of in-thread replies when channels.msteams.replyStyle = thread.
  • Real environment tested: Linux, Node.js v24.13.1, OpenClaw latest main (commit e0e0d28), local checkout with the patch applied.
  • Exact steps or command run after this patch: Ran node scripts/run-vitest.mjs run extensions/msteams/src/send.test.ts with 2 new test cases verifying threaded attachment sends pass threadActivityId and top-level sends pass undefined.
  • Evidence after fix:
$ node scripts/run-vitest.mjs run extensions/msteams/src/send.test.ts

Test Files  1 passed (1)
     Tests  13 passed (2 new)
  Duration  2.1s
  • Observed result after fix: Attachment sends now pass threadActivityId: "thread-root-1" when replyStyle: "thread" and conversation is a channel. The sendMSTeamsActivityWithReference function receives the thread ID and appends ;messageid= to the conversation ID, delivering the message in-thread.
  • What was not tested: Live MS Teams session with real attachment threading (requires Teams bot setup). The test uses mocked sendMSTeamsActivityWithReference to verify the threadActivityId parameter is passed correctly.

Tests and validation

  • node scripts/run-vitest.mjs run extensions/msteams/src/send.test.ts -- 13/13 passed (11 existing + 2 new)
  • New tests:
  • No CHANGELOG.md edits

Risk checklist

  • userVisible: Yes (attachment replies now arrive in-thread)
  • configEnvMigration: No
  • securityAuthNetwork: No
  • Mitigation: Only affects attachment sends in threaded mode. Text-only sends unchanged. Follows the same threading pattern already used by sendMSTeamsMessages.

Current review state

  • AI-assisted (built with Claude Code)

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: S proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 17, 2026
@BradGroux BradGroux mentioned this pull request Jun 17, 2026
Draft
@dwc1997
dwc1997 force-pushed the fix/88836-msteams-attachment-threading branch from 35095fa to e494636 Compare June 17, 2026 13:27
@clawsweeper

clawsweeper Bot commented Jun 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 20, 2026, 3:56 AM ET / 07:56 UTC.

Summary
The PR threads MS Teams FileConsentCard, SharePoint file-card, and OneDrive attachment sends by forwarding a derived threadActivityId and adds mocked send-path regression tests.

PR surface: Source +21, Tests +106. Total +127 across 2 files.

Reproducibility: yes. Source inspection shows current main routes Teams SharePoint and OneDrive attachment sends through raw proactive delivery without the existing thread root while docs and the text-send path define thread preservation for channel replyStyle=thread; I did not run a live Teams tenant reproduction.

Review metrics: 1 noteworthy metric.

  • Attachment routing call sites: 3 changed. FileConsentCard, SharePoint file-card, and OneDrive fallback sends now carry the targeting option, so maintainers should verify real transport placement before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #88836
Summary: This PR is a candidate fix for the canonical MS Teams attachment-threading issue; sibling PRs target the same root cause but none is a merged or proof-positive replacement.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Replace the mock-call tuple indexing with a typed helper or matcher so test-type checking can pass.
  • [P1] Add redacted Microsoft Teams screenshot, recording, terminal output, or logs showing threaded attachment placement and top-level preservation after the patch.
  • Let maintainers choose one canonical landing PR among the open duplicate candidates.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body provides mocked Vitest output and explicitly says no live Microsoft Teams session was tested; no redacted screenshot, recording, live output, or logs show after-fix Teams placement. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A real Microsoft Teams client screenshot or recording would materially prove the visible attachment placement that mocked tests cannot show. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify in Microsoft Teams that attachment replies with replyStyle=thread land inside the channel thread and top-level sends stay top-level.

Risk before merge

  • [P1] The mock-call option assertions can fail the test-type lane before the new regression coverage is usable.
  • [P1] The PR changes visible Microsoft Teams attachment placement, but the supplied proof is only mocked Vitest output and explicitly says no live Teams session was tested.
  • [P1] Multiple open sibling PRs target the same MS Teams attachment-threading bug, so maintainers need one canonical landing path before merging duplicates.

Maintainer options:

  1. Fix test typing and require Teams proof (recommended)
    Repair the mock-call assertion and require redacted live Microsoft Teams evidence showing threaded attachment placement and top-level preservation before merge.
  2. Accept source-only validation
    Maintainers could merge after the type repair on source and mocked-test evidence, but native Teams placement would remain unproven until after merge.
  3. Consolidate duplicate candidates
    Pause this branch if another linked PR becomes typecheck-clean and proof-positive first, then use that single branch as the landing path.

Next step before merge

  • [P1] This needs contributor-supplied live Teams proof plus maintainer duplicate-landing judgment; automation can repair the test assertion but cannot prove the contributor's Teams environment.

Security
Cleared: The diff is limited to MS Teams send/test code and does not change dependencies, lockfiles, CI, secrets, permissions, package metadata, or external code execution paths.

Review findings

  • [P2] Make the mock-call assertion type-safe — extensions/msteams/src/send.test.ts:663
Review details

Best possible solution:

Land one narrow, typecheck-clean MS Teams plugin fix that forwards the existing thread root through FileConsentCard, SharePoint, and OneDrive attachment sends while preserving top-level mode; then close duplicate candidates.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows current main routes Teams SharePoint and OneDrive attachment sends through raw proactive delivery without the existing thread root while docs and the text-send path define thread preservation for channel replyStyle=thread; I did not run a live Teams tenant reproduction.

Is this the best way to solve the issue?

Partially. Forwarding the existing threadActivityId contract through the MS Teams attachment send path is the right narrow owner-boundary fix, but this PR is not the best mergeable state until the test typing, live proof, and duplicate landing choice are resolved.

Full review comments:

  • [P2] Make the mock-call assertion type-safe — extensions/msteams/src/send.test.ts:663
    The new tests read lastCall[3], but sendMSTeamsActivityWithReference is mocked as a zero-argument vi.fn, so test-type checking can reject the tuple index before this regression coverage is usable. Use a typed call helper or a toHaveBeenCalledWith matcher for the options argument; the same pattern appears again in the top-level test.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against a1271830948b.

Label changes

Label justifications:

  • P2: This is a bounded Microsoft Teams attachment delivery bugfix with limited blast radius and no crash, security, or broad runtime impact.
  • merge-risk: 🚨 message-delivery: The diff changes whether Microsoft Teams attachment replies appear as top-level channel posts or as thread replies.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides mocked Vitest output and explicitly says no live Microsoft Teams session was tested; no redacted screenshot, recording, live output, or logs show after-fix Teams placement. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +21, Tests +106. Total +127 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 22 1 +21
Tests 1 107 1 +106
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 129 2 +127

What I checked:

  • Repository policy applied: Root AGENTS.md and extensions/AGENTS.md were read fully; the review applied the whole-path, plugin-boundary, duplicate-PR, and real-behavior-proof requirements. (AGENTS.md:1, a1271830948b)
  • Current main attachment gap: On current main, SharePoint native file-card and OneDrive fallback sends call sendProactiveActivityRaw without passing any threadActivityId option. (extensions/msteams/src/send.ts:306, a1271830948b)
  • Existing thread-aware sibling path: The text/inline-media path already resolves ref.threadId or ref.activityId and passes threadActivityId only for channel proactive sends when replyStyle is thread. (extensions/msteams/src/messenger.ts:531, a1271830948b)
  • Proactive wrapper contract: sendMSTeamsActivityWithReference turns threadActivityId into a ;messageid= conversation id before dispatching through the Teams activity client. (extensions/msteams/src/sdk-proactive.ts:79, a1271830948b)
  • PR production wiring: At the PR head, sendMessageMSTeams computes threadActivityId from the stored reference when replyStyle is thread and forwards it through FileConsentCard, SharePoint, and OneDrive attachment paths. (extensions/msteams/src/send.ts:171, 9d48b7f65092)
  • Patch test blocker: The new tests index lastCall[3] from a vi.fn mock declared without parameters, leaving the options assertion type-unsafe; the same pattern appears in the top-level test. (extensions/msteams/src/send.test.ts:663, 9d48b7f65092)

Likely related people:

  • vincentkoc: Current blame on the affected send, messenger, and proactive wrapper lines points to Vincent Koc's recent main commit carrying the present file versions. (role: recent line owner and area contributor; confidence: medium; commits: 4f278ef71c6f, 844f405ac1be; files: extensions/msteams/src/send.ts, extensions/msteams/src/messenger.ts, extensions/msteams/src/sdk-proactive.ts)
  • sudie-codes: History shows merged Teams thread-reply routing, proactive metadata, FileConsentCard, and Graph upload work adjacent to this attachment-threading path. (role: adjacent threading and file-upload contributor; confidence: high; commits: 9edfefedf7bf, 784318799bf0, 06845a1974a3; files: extensions/msteams/src/send.ts, extensions/msteams/src/messenger.ts, extensions/msteams/src/graph-upload.ts)
  • Hyojin Kwak: Authored the proactive fallback threading fix that established the same channel threadActivityId invariant this PR mirrors for attachments. (role: adjacent thread-routing contributor; confidence: medium; commits: 739ed1bf2944; files: extensions/msteams/src/messenger.ts)
  • heyitsaamir: Authored the Teams SDK rebase touching the current send, messenger, send-context, and sdk-proactive surfaces this PR extends. (role: feature-history contributor; confidence: high; commits: 04c29825356f; files: extensions/msteams/src/send.ts, extensions/msteams/src/messenger.ts, extensions/msteams/src/sdk-proactive.ts)
  • steipete: Git shortlog shows Peter Steinberger has the largest share of commits on the central MS Teams send/threading files, including earlier attachment-flow hardening. (role: feature-history and area contributor; confidence: medium; commits: 0f7f7bb95f77, 1aa77e4603fc, 866bd91c659c; files: extensions/msteams/src/send.ts, extensions/msteams/src/messenger.ts, extensions/msteams/src/sdk-proactive.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jun 17, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 18, 2026
Open
@dwc1997
dwc1997 force-pushed the fix/88836-msteams-attachment-threading branch 2 times, most recently from 5019174 to e0e0d28 Compare June 18, 2026 13:17
@clawsweeper clawsweeper Bot mentioned this pull request Jun 19, 2026
Open
dwc1997 and others added 2 commits June 20, 2026 14:04
@dwc1997 @claude
fix(msteams): pass threadActivityId to attachment sends for threaded …
ae54c11 
…replies

MS Teams attachment sends (FileConsentCard, SharePoint file cards, OneDrive
links) bypassed the threading logic by calling sendProactiveActivityRaw
without threadActivityId. This caused reply messages with attachments to
arrive as new top-level messages instead of in-thread replies.

The fix derives threadActivityId from ref.threadId (or ref.activityId as
fallback) when replyStyle is 'thread' and the conversation is a channel
or group. This matches how sendMSTeamsMessages already handles threading
for text-only messages.

Fixes openclaw#88836

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@dwc1997 @claude
fix(msteams): use 'groupChat' instead of 'group' for conversationType…
fc3297e 
… comparison

The MSTeamsConversationType type is 'personal' | 'groupChat' | 'channel',
not 'group'. TypeScript correctly flags the 'group' comparison as having
no overlap with the narrowed type.

Co-Authored-By: Claude <[email protected]>
@dwc1997
dwc1997 force-pushed the fix/88836-msteams-attachment-threading branch from e0e0d28 to fc3297e Compare June 20, 2026 06:08
@openclaw-barnacle openclaw-barnacle Bot added triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 20, 2026
@dwc1997 dwc1997 closed this Jun 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: msteams Channel integration: msteams merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: msteams messages with attachments misthreaded

1 participant

@dwc1997