fix(msteams): recover inbound channel and group-chat files safely#90738
Conversation
openclaw-barnacle
Bot
added
channel: msteams
|
Codex review: needs maintainer review before merge. Reviewed July 9, 2026, 7:18 PM ET / 23:18 UTC. Summary PR surface: Source -52, Tests +114. Total +62 across 6 files. Reproducibility: yes. Current main and v2026.6.11 still show the old attachment-id gate and channel URL builder, and the linked issue/PR comments provide live Teams and Graph evidence for channel and group-chat attachment loss. Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Land this Teams-plugin fix, or an equivalent reconciled fix, after maintainers accept the automatic non-personal Graph fallback policy and confirm the normal exact-head merge checks. Do we have a high-confidence way to reproduce the issue? Yes. Current main and v2026.6.11 still show the old attachment-id gate and channel URL builder, and the linked issue/PR comments provide live Teams and Graph evidence for channel and group-chat attachment loss. Is this the best way to solve the issue? Yes, with maintainer acceptance of the fallback policy. The fix stays in the Microsoft Teams plugin, follows Microsoft Graph's channel/reply/chat URL shapes, removes ambiguous URL fallback attempts, and adds focused regression coverage. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against fe2486247b6f. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source -52, Tests +114. Total +62 across 6 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (3 earlier review cycles) |
openclaw-barnacle
Bot
added
proof: supplied
clawsweeper
Bot
added
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
|
Pushed 366a383 addressing the P2 finding (maintainer option 1 — narrow the fallback trigger). Gate change ( Tests ( Acceptance criteria (copied output, run on this branch): Live re-verification after the gate change: deployed this exact patch onto a v2026.6.5 install on a live Microsoft Teams tenant today. Channel @mention with a PDF attached to a top-level message → the correct file is read (agent produced an accurate summary of that document); thread-reply follow-ups → handled in-thread; mention-only channel messages with no file → answered normally with no Graph fetch diagnostics; personal-chat files → unaffected. Before/after screenshots available on request. No CHANGELOG or PR-body changes. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
CI note: the failing checks on the latest run are unrelated to this PR. The merge-ref typecheck fails at |
clawsweeper
Bot
added
proof: sufficient
366a383 to
4b500d9
Compare
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
clawsweeper
Bot
added
rating: 🐚 platinum hermit
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
Three bugs blocked reading files attached to channel messages:
- Graph /teams/{id} used channelData.team.id (the 19:[email protected] thread id)
instead of team.aadGroupId (the AAD group GUID) -> 400.
- The Graph fetch was gated on an <attachment id> HTML marker that channel
@mention activities don't carry -> fetch skipped.
- A thread reply was fetched at /messages/{replyId} (404) then fell back to the
bare thread root, returning the root's file for every reply. Replies live at
/messages/{root}/replies/{replyId}.
Fixes openclaw#89594
…r tests in channel context
Build one canonical Graph message URL per Teams activity, recover marker-free channel and group-chat file shares, and retain bounded current-main attachment handling. Co-authored-by: Colton Williams <[email protected]>
Recover channel and group-chat files through one fail-closed Graph identity path, bound inbound enrichment, and remove invalid app-only Graph fallbacks. Co-authored-by: Colton Williams <[email protected]> Co-authored-by: Joshua Packwood <[email protected]>
cb7b9b9 to
cae9fb6
Compare
openclaw-barnacle
Bot
added
docs
steipete
changed the title
|
Maintainer rewrite and exact-head verification complete. Final head: What changed:
Proof:
Live-proof gap: this host has no authenticated Microsoft 365 tenant with the required Graph/file consent and channel, group-chat, and personal-chat fixtures. Earlier contributor evidence supports the reported failure, but I am not transferring it to the final rewrite. A full live replay would use distinct PDF/audio canaries for root-A/reply-B, markerless group chat, personal chat, mention-only/no-file, and bogus/missing identity negatives. Thanks @colton-octaria and @packwood for the implementation and investigation, and @cmc099 for the report. |
|
Merged via squash.
|
…enclaw#90738) * fix(msteams): read file attachments on Teams channel messages Three bugs blocked reading files attached to channel messages: - Graph /teams/{id} used channelData.team.id (the 19:[email protected] thread id) instead of team.aadGroupId (the AAD group GUID) -> 400. - The Graph fetch was gated on an <attachment id> HTML marker that channel @mention activities don't carry -> fetch skipped. - A thread reply was fetched at /messages/{replyId} (404) then fell back to the bare thread root, returning the root's file for every reply. Replies live at /messages/{root}/replies/{replyId}. Fixes openclaw#89594 * fix(msteams): gate Graph media fallback on text/html stub; run trigger tests in channel context * fix(msteams): canonicalize Graph attachment recovery Build one canonical Graph message URL per Teams activity, recover marker-free channel and group-chat file shares, and retain bounded current-main attachment handling. Co-authored-by: Colton Williams <[email protected]> * fix(msteams): canonicalize Graph media recovery Recover channel and group-chat files through one fail-closed Graph identity path, bound inbound enrichment, and remove invalid app-only Graph fallbacks. Co-authored-by: Colton Williams <[email protected]> Co-authored-by: Joshua Packwood <[email protected]> --------- Co-authored-by: Colton Williams <[email protected]> Co-authored-by: Peter Steinberger <[email protected]> Co-authored-by: Joshua Packwood <[email protected]>
3 participants
What Problem This Solves
Microsoft Teams channel and group-chat files were not reliably recoverable from inbound activities. Channel requests could use the Bot Framework team id where Microsoft Graph requires the team's AAD group id; thread replies could fall back to the root message and surface the wrong file; and Teams can omit HTML file markers even though the Graph message still carries attachments.
The same audit found two unsafe adjacent paths: app-only bot tokens were sent to delegated-only
/me/chatsand/me/driveendpoints. Those calls could not succeed and obscured the actual identifier and SharePoint requirements.Fixes #89594.
Why This Change Was Made
The maintainer rewrite establishes one canonical, fail-closed path:
/teams/{aadGroupId}/channels/{channelId}/messages/{messageId}./messages/{rootMessageId}/replies/{replyMessageId}; a failed reply lookup never falls back to the root./chats/{chatId}/messages/{messageId}.getTeamDetailsAPI, then flows through media, thread context, and native channel actions./me/chatsdiscovery, persistedgraphChatIdstate, guessed identifiers, speculative URL candidates, and the dead/me/driveupload fallback are deleted.channels.msteams.sharePointSiteIderror.Config review metric: this adds one optional surface,
channels.msteams.graphMediaFallback. It defaults tofalse. When enabled, markerless channel/group HTML can trigger one Graph message lookup, including ordinary or mention-only activities, so operators opt into the extra traffic and Graph permissions explicitly. Explicit attachment markers continue to use Graph recovery without enabling the setting.The resulting production diff is net-negative and keeps all Teams-specific policy inside the Microsoft Teams plugin.
User Impact
With the documented Graph consent and
graphMediaFallback: true, agents can recover files from markerless top-level channel messages, the exact channel reply being handled, and group chats. Missing or invalid identity fails closed rather than returning a neighboring or root attachment. Personal-chat file handling is unchanged.For outbound files, DMs still use FileConsentCard and images can remain inline. Group-chat/channel non-image files require
sharePointSiteId; the previous app-only OneDrive path never represented a working fallback.Evidence
pnpm check:changed: passed in 8m56.493s, including guards, four tsgo lanes, core and extension lint across 6,326 files, generated metadata, database, media, sidecar, import-cycle, webhook, and pairing checks.019f497b-49d6-7e63-ae2b-1d60e7000a47reported zero findings and “patch is correct” on the locked final tree.cae9fb6979f8cc890184a4f7c5ad8a53963b47fcpassed hosted CI run29061818438, andscripts/pr prepare-run 90738completed inhosted_exactmode./merequires a signed-in delegated user: https://learn.microsoft.com/en-us/graph/api/user-get?view=graph-rest-1.0Live-proof gap: the final rewrite could not be deployed into an authenticated Microsoft 365 tenant with the necessary Graph/file consent and channel, group-chat, and personal-chat fixtures. Earlier contributor proof supports the reported group-chat failure, but is not claimed as exact-rewrite proof. Clearing this gap requires unique PDF/audio canaries, root-A/reply-B selection, markerless group activity, a personal-file regression, a mention-only/no-file case, and missing/bogus identity negatives.
Reported by @cmc099. Implementation and investigation credit to @colton-octaria and @packwood; this rewrite incorporates the safe, default-off behavior from #101096.