fix(msteams): drop unsupported $search on msteams:search (AI-assisted)#70287
fix(msteams): drop unsupported $search on msteams:search (AI-assisted)#70287gaurav0107 wants to merge 4 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryReplaces the unsupported Confidence Score: 5/5Safe to merge — the fix is targeted, all 17 new unit tests pass, and no P0/P1 issues were found. All remaining findings are P2: the raw-HTML-in-text field is pre-existing behavior, and the contentType-default divergence is intentional and documented. Neither blocks the primary fix. No files require special attention.
|
| : (res.value ?? []).filter((msg) => { | ||
| const raw = msg.body?.content ?? ""; | ||
| // Teams bodies default to HTML — strip tags so queries match the | ||
| // rendered text rather than raw markup (e.g. "bold" would otherwise | ||
| // match "<b>old"). Only skip stripping for explicit plain-text bodies; | ||
| // missing contentType falls through to the HTML-safe path. | ||
| const text = msg.body?.contentType === "text" ? raw : stripHtmlFromTeamsMessage(raw); | ||
| return text.toLowerCase().includes(needle); | ||
| }); |
There was a problem hiding this comment.
contentType handling differs from graph-thread.ts
graph-thread.ts (line 133) defaults a missing contentType to "text" (no stripping), but here a missing contentType falls through to the HTML-safe strip path because the check is === "text". The two helpers therefore treat the same message differently. The PR notes this divergence is intentional, but it may surprise readers who see "text" as the documented default in Graph API documentation. A brief inline comment (or extracting the logic to a shared helper) would make the intent explicit and prevent future callers from accidentally re-aligning with the graph-thread default.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/graph-messages.ts
Line: 542-550
Comment:
**Default `contentType` handling differs from `graph-thread.ts`**
`graph-thread.ts` (line 133) defaults a missing `contentType` to `"text"` (no stripping), but here a missing `contentType` falls through to the HTML-safe strip path because the check is `=== "text"`. The two helpers therefore treat the same message differently. The PR notes this divergence is intentional, but it may surprise readers who see `"text"` as the documented default in Graph API documentation. A brief inline comment (or extracting the logic to a shared helper) would make the intent explicit and prevent future callers from accidentally re-aligning with the `graph-thread` default.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in 3c2d5a5589 by expanding the inline comment above the filter predicate to explicitly call out the divergence from graph-thread.ts and the rationale. The two helpers serve different audiences: graph-thread.ts is display-facing and prefers the Graph-documented "text" default, while search needs to be safe against raw markup leaking through missing contentType, so it defaults to HTML-strip. Extracting to a shared helper would widen the seam for a two-line branch — the comment makes the intent explicit without the extra surface.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1057674184
ℹ️ 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".
|
|
||
| // Build query string manually (not URLSearchParams) to preserve literal $ | ||
| // in OData parameter names, consistent with other Graph calls in this module. | ||
| const parts = [`$search=${encodeURIComponent(`"${sanitizedQuery}"`)}`]; | ||
| parts.push(`$top=${top}`); | ||
| const parts = [`$top=${listWindow}`]; |
There was a problem hiding this comment.
Cap list window so Graph
$top never exceeds 50
searchMessagesMSTeams can issue GET .../messages?$top=200 (for example, the default limit of 25 computes listWindow=200), but Microsoft Graph documents a maximum $top of 50 for both chat and channel message-list endpoints. In environments where Graph enforces that limit, this turns normal searches into 400 responses; even if tolerated, the intended 200-message local-filter window is never actually retrieved, so recall is lower than this implementation assumes. Please clamp the requested $top to 50 and use paging if a wider search window is required.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 3c2d5a5589. The outgoing $top is now hard-capped at 50 (Graph's documented max for chat/channel message collections), and a pagination loop follows @odata.nextLink until the configured list window is filled or Graph runs out of messages. New tests assert $top=50 is always on the outgoing URL, $top=200 never appears, and the nextLink is followed when one page doesn't fill the window (and NOT followed once it does).
) Graph rejects `$search` on `/chats/{id}/messages` and `/teams/{id}/channels/{id}/messages` under Application permissions with "Query option 'Search' is not allowed", so every `msteams:search` call from an app-only bot returns 400. The alternative `/search/query` endpoint also blocks Application permissions for `chatMessage`, leaving no app-only-compatible server-side search path. Switch `searchMessagesMSTeams` to a list-and-local-filter implementation: fetch up to a capped recent window (clamp(limit * 10, 50, 200)) via `/messages?$top=...`, keep the `$filter` pushdown for sender (still supported app-only), and apply a case-insensitive `includes` against `body.content` locally before slicing to the requested limit. Drop the `ConsistencyLevel` header since it was only required for `$search`. Document the trade-off in the function JSDoc: full-archive search would require Delegated auth and is out of scope here. Update `graph-messages.search.test.ts` to lock in the new behavior — that `$search` is never emitted, that the server-side `$top` grows past the caller's limit to allow local filtering, and that returned messages are filtered and capped locally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3c2d5a5 to
98d4731
Compare
Failing CI checks are pre-existing on
|
|
Codex review: needs changes before merge. What this changes: The branch updates MS Teams message search to fetch a bounded recent chat/channel window with Required change before merge: The blockers are narrow and mechanical on a maintainer-editable contributor PR: remove the unsupported sender Security review: Security review cleared: The diff changes existing MS Teams Graph query construction, regression tests, and changelog only; it adds no dependencies, CI execution, secrets handling, permissions, or package/runtime execution surface. Review findings:
Review detailsBest possible solution: Land a corrected version that uses only app-only-supported chat/channel message-list parameters, applies both text and sender matching locally over the bounded recent window, keeps the no- Do we have a high-confidence way to reproduce the issue? Yes. The original bug is reproducible at the URL-construction seam on current main because the helper and tests still build Is this the best way to solve the issue? No, not yet. Removing Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against a256745323ab. |
Summary
msteams:searchaction hitsHTTP 400: Search is not supportedbecause Graph API blocks$searchon/chats/{id}/messagesand/teams/*/channels/*/messageswhen called with Application permissions (the default for bot auth).$searchwith a list-and-local-filter path that works under Application permissions. Sender filtering still pushes down via$filter(which Graph supports app-only). Local filter strips HTML bodies so queries match rendered text./search/querycallers elsewhere in the repo. Search range is now bounded to the recent list window rather than the full archive — full-archive search requires Delegated auth and is out of scope for this fix.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause
searchMessagesMSTeamsused$searchin the Graph query string. Graph API documentation is explicit that$searchon message collections is not available for Application permissions — only/search/querywith Delegated auth supports it for chat messages, and even that blocks thechatMessageentity for app-only.$search. Unit tests mocked successful responses, so the permission mismatch never surfaced in CI.$searchworked because it does on some other Graph collections under app-only.Regression Test Plan
extensions/msteams/src/graph-messages.search.test.ts$search, regardless of query value. New testdoes not use Graph $search (unsupported under Application permissions)asserts this directly.$searchwithout needing a live Graph call.User-visible / Behavior Changes
msteams:searchnow works on app-only (bot) credentials instead of returning Graph 400.contentType: "text".Diagram
Security Impact (required)
/chats/{id}/messagesand/teams/*/channels/*/messagesendpoints, different query string.Repro + Verification
Environment
Steps
msteams:searchaction against that chat with any query.Expected
Actual (before this PR)
HTTP 400: Search is not supported on this organizationregardless of query.Actual (after this PR)
$filterpushdown for sender, pagination window sizing, anduser:target resolution.Evidence
extensions/msteams/src/graph-messages.search.test.tsnow has 17 assertions locking the new contract; one specifically asserts the path never contains$search.pnpm test extensions/msteams/src/graph-messages.search.test.ts→ 17/17 passing.pnpm test extensions/msteams→ 874/874 passing (68 files).pnpm tsgo:prodandpnpm check:test-typesboth green.pnpm lint:extensions→ 0 warnings, 0 errors.pnpm format:check extensions/msteams/src/graph-messages*.ts→ clean.Human Verification (required)
$searchis absent from the outgoing query string (asserted by unit test)<b>does not match<b>world</b>)@mentiondisplay names are preserved for matchingcontentType: "text"bodies pass through unstrippeduser:<aadId>targets resolve through the conversation store$filterescaping handles single quotes (e.g.,O'Brien→O''Brien)contentTypedefaults to HTML-safe (strip) rather than rawteamId/channelId) routes through channel path$searchblocked under Application permissions) matches Microsoft's documented constraint referenced by the issue reporter and the Microsoft Graph known issue.Review Conversations
Compatibility / Migration
Risks and Mitigations
searchMessagesMSTeams. Full-archive search requires Delegated auth and is a separate follow-up. The prior behavior was fully broken under app-only, so this is net-positive.textcontentType now goes throughstripHtmlFromTeamsMessage.contentType: "text"bodies are NOT stripped.AI/Vibe-Coded PR Disclosure
CC msteams maintainers per CONTRIBUTING: @onutc @osolmaz