Skip to content

fix(msteams): paginate thread replies and keep recent context#69428

Open
hkalkoti1 wants to merge 3 commits intoopenclaw:mainfrom
hkalkoti1:fix/msteams-thread-pagination
Open

fix(msteams): paginate thread replies and keep recent context#69428
hkalkoti1 wants to merge 3 commits intoopenclaw:mainfrom
hkalkoti1:fix/msteams-thread-pagination

Conversation

@hkalkoti1
Copy link
Copy Markdown

Title: fix(msteams): paginate thread replies and keep recent context

Summary

  • Problem: Teams thread context enrichment only used the first Graph replies page, so long threads could skew toward the oldest replies.
  • Why it matters: the bot could miss the newest thread context, which is usually the most relevant context for a channel-thread reply.
  • What changed: fetchThreadReplies now follows @odata.nextLink under a hard page cap and returns the most recent bounded reply window instead of only the first page.
  • What did NOT change (scope boundary): no live-tenant Graph validation, no changes to higher-level message-handler behavior, and no prompt-formatting changes beyond the selected reply window.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

  • Closes #
  • Related #
  • This PR fixes a bug or regression

Root Cause (if applicable)

  • Root cause: the Teams thread helper fetched only the initial Graph replies page even when Graph exposed additional pages via @odata.nextLink.
  • Missing detection / guardrail: there was no unit coverage for paginated reply retrieval or for preferring the newest bounded reply window.
  • Contributing context (if known): the Graph replies endpoint is oldest-first and does not support $orderby, so a single-page implementation silently favors stale context on long threads.

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/graph-thread.test.ts
  • Scenario the test should lock in: when Graph returns multiple reply pages for a channel thread, the helper follows pagination and keeps the most recent bounded reply window rather than only the first page.
  • Why this is the smallest reliable guardrail: the bug is isolated to the Graph reply helper and can be reproduced deterministically with mocked paginated Graph responses.
  • Existing test that already covers this (if any): none before this change.
  • If no new test is added, why not: N/A

User-visible / Behavior Changes

  • Long Teams channel threads can now contribute newer reply context to the bot instead of only the oldest reply page.
  • Behavior stays best-effort: if Graph pagination or fetches fail, the surrounding handler still degrades gracefully as before.

Diagram (if applicable)

Before:
[long Teams thread] -> [fetch first replies page only] -> [oldest replies injected] -> [bot may miss latest context]

After:
[long Teams thread] -> [follow paginated replies with hard cap] -> [keep most recent bounded window] -> [bot sees newer thread context]

Security Impact (required)

  • New permissions/capabilities? (No)
  • Secrets/tokens handling changed? (No)
  • New/changed network calls? (Yes)
  • Command/tool execution surface changed? (No)
  • Data access scope changed? (No)
  • If any Yes, explain risk + mitigation:
    • Risk: this can issue additional Graph requests for long threads.
    • Mitigation: pagination is capped at 10 pages and the helper still returns only the requested bounded reply window.

Repro + Verification

Environment

  • OS: macOS
  • Runtime/container: repo-local Node via pnpm openclaw
  • Model/provider: N/A
  • Integration/channel (if any): MSTeams Graph helper unit tests
  • Relevant config (redacted): N/A

Steps

  1. Mock the first Graph replies page with reply-1, reply-2, and an @odata.nextLink.
  2. Mock the second page with reply-3, reply-4.
  3. Call fetchThreadReplies(..., limit=2).

Expected

  • The helper should follow the next link and return the newest bounded window: reply-3, reply-4.

Actual

  • Before the fix, the helper would only use the first page and return reply-1, reply-2, dropping the newer page entirely.

Evidence

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

Human Verification (required)

  • Verified scenarios: targeted unit run for extensions/msteams/src/graph-thread.test.ts; paginated reply path; hard page-cap path; existing thread-helper tests remained green.
  • Edge cases checked: missing value, min/max limit clamping, bounded pagination cap.
  • What you did not verify: live Microsoft Graph behavior against a real Teams tenant or real long thread pagination ordering/latency.

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.

Compatibility / Migration

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

Risks and Mitigations

  • Risk: Microsoft Graph pagination behavior for long Teams threads has not been validated against a real tenant in this change.
    • Mitigation: the PR makes the assumption explicit, keeps the change local to the helper, and adds deterministic regression tests around the intended pagination behavior.
  • Risk: extra Graph fetches could increase latency on large threads.
    • Mitigation: page following is hard-capped and still returns only a bounded recent window.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: S labels Apr 20, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 20, 2026

Greptile Summary

This PR fixes a silent data-quality bug in the MS Teams Graph thread helper: fetchThreadReplies previously returned only the first page of replies (oldest-first, no $orderby supported), so long threads injected stale context into the bot. The fix follows @odata.nextLink under a hard cap of 10 pages and slices the collected window to the most recent limit replies. The pagination uses the existing fetchGraphAbsoluteUrl (SSRF-guarded) function, and two new deterministic unit tests cover both the multi-page path and the cap.

Confidence Score: 5/5

Safe to merge — logic is correct, page cap is enforced, SSRF guard is in place, and new tests provide deterministic coverage of the fixed path.

No P0 or P1 findings. The single P2 comment (partial results lost on mid-pagination failure) is consistent with the pre-existing throw-on-error contract and does not represent a regression. Implementation is well-scoped and well-tested.

No files require special attention.

Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/msteams/src/graph-thread.ts
Line: 118-131

Comment:
**Partial results lost on mid-pagination failure**

If `fetchGraphAbsoluteUrl` throws on page 2 (or any subsequent page), the replies already accumulated in the loop are discarded and the caller receives an exception rather than the partial set. Before this change, a single-page failure also threw, so the top-level handler's degradation path still applies. This is consistent with the existing contract but worth noting: a transient Graph error on page 3 now means the caller sees no replies at all instead of at least the first page's results.

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

Reviews (1): Last reviewed commit: "fix(msteams): paginate thread replies" | Re-trigger Greptile

Comment thread extensions/msteams/src/graph-thread.ts
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ba10ae827a

ℹ️ 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".

Comment thread extensions/msteams/src/graph-thread.ts Outdated
@rosalind-hacks
Copy link
Copy Markdown

Pagination for Teams thread replies is easy to miss — the Graph API only returns 20 replies per page by default, so threads with any real activity would get silently truncated context. Skewing toward old messages instead of recent ones is exactly the wrong behavior for context enrichment. The change to keep only the most recent N replies after full pagination makes sense. Does the implementation handle the edge case where total reply count exceeds the pagination budget?

@hkalkoti1
Copy link
Copy Markdown
Author

Review requested from @onutc (Onur) for the MS Teams surface. Waiting for CI to resolve before final merge readiness.

@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 29, 2026

Codex review: needs real behavior proof before merge.

Summary
The PR changes the MS Teams Graph thread helper to follow bounded reply pagination, keep the newest timestamped reply window, preserve partial later-page results, add regression tests, and add a changelog entry.

Reproducibility: yes. for the helper-level bug: current main is source-reproducible because fetchThreadReplies reads only the initial Graph page, and the PR adds deterministic mocked pagination coverage. A live tenant reproduction has not been documented yet.

Real behavior proof
Needs real behavior proof before merge: Only mocked helper tests, CI, and local command claims are documented; add a screenshot/video, terminal output, copied live output, linked artifact, or redacted logs from a real Teams/Graph tenant, then update the PR body to trigger a fresh review or ask a maintainer for @clawsweeper re-review.

Next step before merge
The remaining blocker is external real behavior proof plus normal maintainer merge handling, not a narrow code repair for ClawSweeper to attempt.

Security
Cleared: No concrete security or supply-chain issue found; the diff adds bounded Microsoft Graph GET pagination through existing Graph fetch helpers and does not touch dependencies, CI, publishing, or secret storage.

Review details

Best possible solution:

Merge the prepared MS Teams plugin fix only after fresh required checks remain green and a contributor or maintainer adds redacted live Teams/Graph evidence showing a long paginated thread contributes the expected recent chronological context.

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

Yes for the helper-level bug: current main is source-reproducible because fetchThreadReplies reads only the initial Graph page, and the PR adds deterministic mocked pagination coverage. A live tenant reproduction has not been documented yet.

Is this the best way to solve the issue?

Yes for the code direction: keeping the fix in the MS Teams Graph helper, using the existing guarded absolute-URL fetch, capping pagination, preserving partial pages, and selecting by createdDateTime is the narrow maintainable solution. It is not merge-ready until real behavior proof is supplied.

Acceptance criteria:

  • Provide redacted real Microsoft Teams/Graph proof for a long thread whose replies require pagination.
  • Confirm the exact head keeps required CI green after proof is added.
  • pnpm test -- extensions/msteams/src/graph-thread.test.ts
  • pnpm build
  • pnpm check

What I checked:

Likely related people:

  • sudie-codes: Merged PR history shows the Graph thread-history helper and the original one-page reply limitation were introduced with the MS Teams thread history feature. (role: introduced behavior; confidence: high; commits: 8c852d86f759, 0f192710924f; files: extensions/msteams/src/graph-thread.ts, extensions/msteams/src/graph-thread.test.ts, extensions/msteams/src/monitor-handler/message-handler.ts)
  • steipete: Recent history on the central MS Teams Graph and handler files includes helper export trimming, Graph helper refactors, and broad Teams handler refactors that affect integration with this PR. (role: recent maintainer/refactor owner; confidence: high; commits: 3f002b10d281, af62a2c2e46b, ffe67e9cdc9e; files: extensions/msteams/src/graph.ts, extensions/msteams/src/graph-thread.ts, extensions/msteams/src/monitor-handler/message-handler.ts)
  • onutc: CONTRIBUTING.md lists Onur for MS Teams ownership, and the PR discussion explicitly requested Onur’s review for this surface. (role: adjacent owner/reviewer; confidence: medium; files: CONTRIBUTING.md, extensions/msteams/src/graph-thread.ts)
  • BradGroux: A maintainer update says Brad rebased and prepared the current PR head, including timestamp-order-safe windowing, local response typing, the changelog entry, and validation claims. (role: prepared-head maintainer; confidence: medium; commits: c7b23140cf7e; files: extensions/msteams/src/graph-thread.ts, extensions/msteams/src/graph-thread.test.ts, CHANGELOG.md)

Remaining risk / open question:

  • Live Microsoft Graph/Teams tenant behavior, pagination latency, and long-thread ordering are still not proven by contributor-facing after-fix evidence.

Codex review notes: model gpt-5.5, reasoning high; reviewed against bbead1bb1ee0.

@BradGroux BradGroux force-pushed the fix/msteams-thread-pagination branch from 2296bc1 to c7b2314 Compare May 8, 2026 17:26
@BradGroux
Copy link
Copy Markdown
Member

Maintainer update: rebased this onto current main and prepared the branch at c7b23140cf7e1dfce4c1d9115d676a0ff0336b8e.

What changed in the prepared head:

  • Dropped the unrelated version-fast-path commit/revert pair while preserving the Teams graph-thread changes.
  • Kept bounded @odata.nextLink pagination and partial-results behavior on later-page failure.
  • Made the paged response typing current-main compatible with a local helper type.
  • Selected the recent reply window by createdDateTime instead of relying on Graph page order, then returned the selected context chronologically.
  • Added the required changelog entry with contributor attribution.

Verification run locally:

  • pnpm test -- extensions/msteams/src/graph-thread.test.ts (26 tests passed)
  • pnpm build
  • pnpm check

Waiting on fresh CI/ClawSweeper for this prepared head before merge.

@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: msteams Channel integration: msteams size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants