fix(msteams): tighten html attachment fallback by neeravmakwana · Pull Request #58632 · openclaw/openclaw

neeravmakwana · 2026-04-01T00:57:25Z

Summary

Problem: Teams mention-only text/html attachments still triggered Graph media fallback, and the Graph media path still attempted ${messageUrl}/attachments instead of using the message body's attachments array.
Why it matters: mention-only messages generated noisy fallback attempts and misleading empty-fetch diagnostics, while legitimate Graph attachment handling depended on a redundant and undocumented fetch path.
What changed: gate the Graph fallback on real <attachment> tags in HTML attachments, reuse chatMessage.attachments from the fetched message body, and add regression tests for both behaviors.
What did NOT change (scope boundary): this PR does not change message ID candidate selection or broader Teams file-download auth behavior.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes [Bug]: msteams channel file attachments not downloaded — Graph fallback triggers on non-file HTML attachments #58617
Related [Bug]: msteams channel file attachments not downloaded — Graph fallback triggers on non-file HTML attachments #58617
This PR fixes a bug or regression

Root Cause / Regression History (if applicable)

Root cause: resolveMSTeamsInboundMedia() treated any all-text/html attachment set as a Graph-media fallback candidate, even though Teams mention markup also arrives as text/html and does not imply a downloadable file.
Missing detection / guardrail: there was no regression test ensuring mention-only HTML skips Graph fallback, and no test asserting Graph media reuses chatMessage.attachments from the message resource.
Prior context (git blame, prior PR, issue, or refactor if known): issue [Bug]: msteams channel file attachments not downloaded — Graph fallback triggers on non-file HTML attachments #58617 documents the observed behavior; I did not trace a single introducing PR with confidence.
Why this regressed now: Teams channel mentions and file-card markup share the same text/html transport shape, so the broader fallback condition matched both.
If unknown, what was ruled out: this PR does not assume a universal message-id mismatch because the current code already tries several message-id candidates.

Regression Test Plan (if applicable)

Coverage level that should have caught this:
- Unit test
- Seam / integration test
- End-to-end test
- Existing coverage already sufficient
Target test or file: extensions/msteams/src/monitor-handler/inbound-media.test.ts, extensions/msteams/src/attachments.test.ts
Scenario the test should lock in: mention-only HTML attachments must not trigger Graph fallback, and Graph media downloads must source attachments from the fetched message body rather than ${messageUrl}/attachments.
Why this is the smallest reliable guardrail: both decisions are local branching rules with stable inputs and do not need a full Teams roundtrip to validate.
Existing test that already covers this (if any): existing attachment tests covered SharePoint reference downloads but not the fallback trigger condition or the redundant /attachments fetch.
If no new test is added, why not: N/A

User-visible / Behavior Changes

Teams mention-only channel messages stop triggering empty Graph media fallback attempts.
Teams Graph media downloads now rely on the message body's attachment data instead of a redundant follow-up fetch.

Diagram (if applicable)

Before:
mention-only html -> onlyHtmlAttachments=true -> Graph fallback -> empty fetch noise
file html -> onlyHtmlAttachments=true -> Graph fallback -> attachment handling

After:
mention-only html -> attachmentTags=0 -> no Graph fallback
file html with <attachment> -> attachmentTags>0 -> Graph fallback -> attachment handling

Security Impact (required)

New permissions/capabilities? (Yes/No): No
Secrets/tokens handling changed? (Yes/No): No
New/changed network calls? (Yes/No): No
Command/tool execution surface changed? (Yes/No): No
Data access scope changed? (Yes/No): No
If any Yes, explain risk + mitigation: N/A

Repro + Verification

Environment

OS: macOS 15.3 / darwin 25.3.0
Runtime/container: Node 22+ via repo scripts
Model/provider: N/A
Integration/channel (if any): msteams
Relevant config (redacted): default test config

Steps

Send a Teams channel message that contains only mention HTML attachments.
Observe the inbound media resolution path.
Send a Teams channel file message that resolves through Graph message attachments.

Expected

Mention-only HTML does not trigger Graph media fallback.
Graph file handling uses the fetched message body's attachments array.

Actual

Before this change, mention-only HTML could enter Graph fallback.
Before this change, Graph media also tried ${messageUrl}/attachments.

Evidence

Attach at least one:

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

Verified scenarios: ran pnpm test:extension msteams successfully; verified the new unit coverage for mention-only HTML and the updated Graph attachment path; ran pnpm build successfully.
Edge cases checked: mention-only HTML skips fallback, HTML with <attachment> tags still falls back, SharePoint reference attachments still merge with hosted content without calling ${messageUrl}/attachments.
What you did not verify: live Teams channel traffic against a real tenant.

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.

Compatibility / Migration

Backward compatible? (Yes/No): Yes
Config/env changes? (Yes/No): No
Migration needed? (Yes/No): No
If yes, exact upgrade steps: N/A

Risks and Mitigations

Risk: Teams file-card HTML without <attachment> tags would no longer trigger Graph fallback.
- Mitigation: the fallback now keys off the explicit attachment markup Teams uses for file cards, and the direct attachment downloader remains unchanged for normal attachment payloads.
Risk: repo-wide pnpm check is currently blocked by unrelated existing tsgo failures in extensions/diffs/src/language-hints.test.ts.
- Mitigation: this PR still includes a green pnpm test:extension msteams lane and a green pnpm build run for the touched surface.

AI Assistance

AI-assisted
Testing: lightly tested on the touched surface (pnpm test:extension msteams, pnpm build; pnpm check blocked by unrelated existing tsgo failures in extensions/diffs/src/language-hints.test.ts)
Prompts/session logs: available on request
Confirmed understanding: yes

Made with Cursor

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8daa6af80e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

extensions/msteams/src/monitor-handler/inbound-media.ts

greptile-apps · 2026-04-01T01:11:30Z

Greptile Summary

This PR tightens the Graph media fallback in the MS Teams integration by requiring explicit <attachment> tags in HTML attachment content (attachmentTags > 0) before triggering the Graph message-URL resolution path. It also eliminates a redundant GET ${messageUrl}/attachments network call in graph.ts by reusing the attachments array already present in the fetched message body, and wires new unit tests for both behaviors.

Key changes:

inbound-media.ts: onlyHtmlAttachments guard is now additionally gated on effectiveHtmlSummary.attachmentTags > 0; summarizeMSTeamsHtmlAttachments is called as a fallback when the caller does not supply a pre-computed summary.
attachments/graph.ts: Dedicated GET /attachments fetch removed; messageAttachments captured from the message body response; attachmentStatus now reflects the message-body HTTP status instead of a separate endpoint status (compatible with the optional-number typing).
Tests: inbound-media.test.ts (new) and updated attachments.test.ts lock in both the skip-on-mention and the no-/attachments-fetch behaviors.

Minor gap: The inbound-media.test.ts cases always supply an explicit htmlSummary, so the internal fallback path that calls summarizeMSTeamsHtmlAttachments when no summary is provided goes untested.

Confidence Score: 5/5

Safe to merge; the logic change is correct, type-safe, and covered by regression tests for the two main scenarios.
All findings are P2 (a minor test coverage gap for the internal-fallback path). The core logic—gating Graph fallback on attachment tags and reusing the message body's attachments array—is sound and consistent with how MSTeamsGraphMediaResult.attachmentStatus is typed. No P0/P1 issues found.
No files require special attention.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: extensions/msteams/src/monitor-handler/inbound-media.test.ts
Line: 94-105

Comment:
**Fallback path for `htmlSummary` not covered**

Both new tests always supply an explicit `htmlSummary`, so the branch inside `resolveMSTeamsInboundMedia` where `htmlSummary` is `undefined` and the function falls back to calling `summarizeMSTeamsHtmlAttachments(attachments)` is never exercised here. A third test that omits `htmlSummary` (relying on the internal fallback) would confirm the fallback correctly computes `attachmentTags` from attachment content and routes accordingly — e.g., passing a mention-only HTML attachment without providing `htmlSummary` should still skip the Graph fallback.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(msteams): tighten html attachment fa..." | Re-trigger Greptile}

extensions/msteams/src/monitor-handler/inbound-media.test.ts

neeravmakwana · 2026-04-01T01:16:12Z

Addressed the current bot feedback in a follow-up update.

Changes made:

Restored Graph fallback for HTML-only messages that carry hosted content through cid: image sources by treating cidImages > 0 as a valid Graph-media hint alongside <attachment> tags.
Added coverage for the internal htmlSummary fallback path by testing a mention-only HTML message with htmlSummary omitted.
Added a regression test that confirms CID-hosted HTML images still take the Graph fallback path even when there are no <attachment> tags.

Verification:

pnpm test:extension msteams ✅ (45 files, 459 tests passing)

The addressed review threads are resolved now.

aisle-research-bot · 2026-04-01T07:02:07Z

🤖 We're reviewing this PR with Aisle

We're running a security check on the changes in this PR now. This usually takes a few minutes. ⌛
We'll post the results here as soon as they're ready.

Progress:

Analysis
Finalization

_{Last updated on: 2026-04-01T07:02:07Z}

fix(msteams): tighten html attachment fallback

8daa6af

openclaw-barnacle bot added channel: msteams Channel integration: msteams size: S labels Apr 1, 2026

chatgpt-codex-connector bot reviewed Apr 1, 2026

View reviewed changes

extensions/msteams/src/monitor-handler/inbound-media.ts Outdated Show resolved Hide resolved

greptile-apps bot reviewed Apr 1, 2026

View reviewed changes

extensions/msteams/src/monitor-handler/inbound-media.test.ts Show resolved Hide resolved

fix(msteams): keep cid html graph fallback

212a0f0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(msteams): tighten html attachment fallback#58632

fix(msteams): tighten html attachment fallback#58632
neeravmakwana wants to merge 2 commits intoopenclaw:mainfrom
neeravmakwana:fix-msteams-html-attachment-fallback

neeravmakwana commented Apr 1, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

Uh oh!

greptile-apps bot commented Apr 1, 2026

Uh oh!

Uh oh!

neeravmakwana commented Apr 1, 2026

Uh oh!

aisle-research-bot bot commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

neeravmakwana commented Apr 1, 2026

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Root Cause / Regression History (if applicable)

Regression Test Plan (if applicable)

User-visible / Behavior Changes

Diagram (if applicable)

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Review Conversations

Compatibility / Migration

Risks and Mitigations

AI Assistance

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

greptile-apps bot commented Apr 1, 2026

Greptile Summary

Confidence Score: 5/5

Uh oh!

Uh oh!

neeravmakwana commented Apr 1, 2026

Uh oh!

aisle-research-bot bot commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant