Skip to content

fix(msteams): keep attachment replies in channel threads#94348

Open
arkyu2077 wants to merge 1 commit into
openclaw:mainfrom
arkyu2077:fix/issue-88836-teams-threaded-attachments
Open

fix(msteams): keep attachment replies in channel threads#94348
arkyu2077 wants to merge 1 commit into
openclaw:mainfrom
arkyu2077:fix/issue-88836-teams-threaded-attachments

Conversation

@arkyu2077

@arkyu2077 arkyu2077 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Fixes #88836.

Summary

  • keep SharePoint native file-card sends inside the configured channel thread
  • keep OneDrive fallback file-link sends inside the configured channel thread
  • thread file-only proactive sends using the stored Teams thread root the same way text replies already do

Test

  • node scripts/run-vitest.mjs extensions/msteams/src/send.test.ts
  • node scripts/run-vitest.mjs extensions/msteams/src/messenger.test.ts

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 18, 2026
@clawsweeper

clawsweeper Bot commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 1, 2026, 2:40 PM ET / 18:40 UTC.

Summary
The PR forwards a resolved Microsoft Teams channel thread root into SharePoint file-card and OneDrive fallback attachment sends and adds regression tests for those paths.

PR surface: Source +24, Tests +87. Total +111 across 2 files.

Reproducibility: yes. at source level: current main resolves threaded channel reply style, but the SharePoint and OneDrive attachment branches bypass the existing threadActivityId delivery path. I did not run a live Microsoft Teams tenant reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Attachment delivery branches: 2 changed production branches. Both SharePoint file-card and OneDrive fallback sends change Teams message targeting, so native transport placement matters before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #88836
Summary: This PR is an open candidate fix for the canonical Microsoft Teams attachment misthreading bug; sibling PRs target the same root cause, but none is a merged or clearly proof-positive replacement for this branch.

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:

  • [P1] Add redacted real Microsoft Teams proof showing threaded attachment placement and top-level preservation after the patch.
  • Rewrite the new mock-call option assertions so check-test-types passes.
  • Coordinate with maintainers on one canonical landing PR for the linked attachment-threading bug.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body supplies mocked Vitest commands only; it needs redacted after-fix Teams proof such as screenshots, recording, live output, or logs before merge. 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.

Proof path suggestion
A visible real Microsoft Teams run would prove the changed attachment placement better than mocked unit tests. Mantis is currently scoped to Telegram, Discord, and web UI chat proof, so it is not the right proof path for this surface.
Use maintainer screenshot/manual proof, browser or Playwright proof, Crabbox where appropriate, or normal local artifact proof instead.

Risk before merge

  • [P1] The PR changes whether Microsoft Teams attachment replies land as top-level channel posts or inside channel threads, so mocked unit tests do not prove the visible transport behavior.
  • [P1] The current PR head fails check-test-types, so the added regression tests are not yet usable as merge validation.
  • [P1] Several unmerged or closed sibling PRs target the same linked Microsoft Teams attachment-threading bug, so maintainers should pick one canonical landing path before merging duplicates.

Maintainer options:

  1. Fix typechecking and require Teams proof (recommended)
    Repair the mock-call assertions, then require redacted Microsoft Teams evidence showing SharePoint and OneDrive attachment placement for threaded and top-level sends before merge.
  2. Accept source-only validation
    Maintainers could intentionally merge after the type repair on source and mocked-test evidence, but real 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 for the linked bug.

Next step before merge

  • [P1] Contributor or maintainer follow-up is needed for real Teams proof, the typecheck repair, and duplicate landing selection; automation cannot supply the contributor's real Teams proof.

Security
Cleared: The diff is limited to Microsoft 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 option assertions type-safe — extensions/msteams/src/send.test.ts:421
Review details

Best possible solution:

Land one narrow, typecheck-clean Microsoft Teams plugin fix after redacted Teams proof shows threaded and top-level attachment placement, then close duplicate candidates.

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

Yes, at source level: current main resolves threaded channel reply style, but the SharePoint and OneDrive attachment branches bypass the existing threadActivityId delivery path. I did not run a live Microsoft Teams tenant reproduction in this read-only review.

Is this the best way to solve the issue?

Partially: forwarding the existing threadActivityId contract inside the Microsoft Teams plugin is the right owner-boundary fix, but this PR is not the best mergeable state until the test typing, real Teams proof, and duplicate landing choice are resolved.

Full review comments:

  • [P2] Make the mock-call option assertions type-safe — extensions/msteams/src/send.test.ts:421
    The new tests index mock.calls[0]?.[3], but sendMSTeamsActivityWithReference is inferred as a zero-argument vi.fn, so the CI check-test-types job fails with TS2493 here and again at line 499 before the regression coverage is usable. Use a typed call helper or a matcher such as toHaveBeenCalledWith for the options argument.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: The PR targets a bounded Microsoft Teams attachment delivery bug with limited blast radius and no crash, security bypass, or broad runtime outage.
  • merge-risk: 🚨 message-delivery: The diff changes whether Microsoft Teams file attachment replies are delivered 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 supplies mocked Vitest commands only; it needs redacted after-fix Teams proof such as screenshots, recording, live output, or logs before merge. 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 +24, Tests +87. Total +111 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 25 1 +24
Tests 1 88 1 +87
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 113 2 +111

What I checked:

  • Repository policy read: Root AGENTS.md and extensions/AGENTS.md were read fully; their whole-path PR review, plugin-boundary, real behavior proof, and message-delivery risk guidance apply to this Teams plugin PR. (AGENTS.md:1, 477b27b6f87e)
  • Current main attachment-threading gap: Current main sends the SharePoint native file-card activity through sendProactiveActivityRaw without a threadActivityId option. (extensions/msteams/src/send.ts:306, 477b27b6f87e)
  • Current main OneDrive gap: The OneDrive fallback file-link branch also calls sendProactiveActivityRaw without carrying the Teams thread root. (extensions/msteams/src/send.ts:350, 477b27b6f87e)
  • Existing thread-aware sibling path: sendMSTeamsMessages already passes threadActivityId for channel proactive sends when replyStyle is thread, and sdk-proactive converts that option into a ;messageid= conversation id. (extensions/msteams/src/messenger.ts:531, 477b27b6f87e)
  • PR production wiring: At the PR head, sendMessageMSTeams resolves a channel-only threadActivityId and passes it through both changed SharePoint and OneDrive raw attachment sends. (extensions/msteams/src/send.ts:307, 091f72f85394)
  • Typecheck blocker: The CI check-test-types job fails with TS2493 at send.test.ts lines 421 and 499 because the new tests index argument 3 from a zero-argument vi.fn mock tuple. (extensions/msteams/src/send.test.ts:421, 091f72f85394)

Likely related people:

  • sudie-codes: Merged history covers Teams thread reply routing, FileConsentCard handling, and Graph upload behavior near this attachment-threading path. (role: adjacent threading and file-upload contributor; confidence: high; commits: 9edfefedf7bf, 784318799bf0, ba1b8424f48e; 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 attachment sends. (role: adjacent thread-routing contributor; confidence: medium; commits: 739ed1bf2944; files: extensions/msteams/src/messenger.ts)
  • heyitsaamir: Authored the Teams SDK rebase touching the 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: Prior history shows substantial Microsoft Teams send/threading and attachment-flow work in the central files. (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)
  • vincentkoc: Recent release and forward-port history touched the affected Teams send, messenger, and proactive wrapper files on the current code line. (role: recent area contributor; confidence: medium; commits: e085fa1a3ffd, 6f9b4b52f8e4; 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.
Review history (1 earlier review cycle)
  • reviewed 2026-06-22T03:18:12.010Z sha 091f72f :: needs real behavior proof before merge. :: [P2] Make the mock-call option assertions type-safe

@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 18, 2026
@BradGroux BradGroux mentioned this pull request Jun 18, 2026
Draft
This was referenced Jun 18, 2026
Closed
Open
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 21, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added stale Marked as stale due to inactivity triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 21, 2026
@cmc099

cmc099 commented Jul 21, 2026

Copy link
Copy Markdown

Proof video showing PR fix in live Teams environment

https://youtu.be/jRtrJrWGAaQ

@barnacle-openclaw barnacle-openclaw Bot removed the stale Marked as stale due to inactivity label Jul 22, 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. 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. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: msteams messages with attachments misthreaded

2 participants

@arkyu2077 @cmc099