fix(msteams): paginate thread replies and keep recent context#69428
fix(msteams): paginate thread replies and keep recent context#69428hkalkoti1 wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a silent data-quality bug in the MS Teams Graph thread helper: Confidence Score: 5/5Safe 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 AIThis 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 |
There was a problem hiding this comment.
💡 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".
|
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? |
|
Review requested from @onutc (Onur) for the MS Teams surface. Waiting for CI to resolve before final merge readiness. |
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: yes. for the helper-level bug: current main is source-reproducible because Real behavior proof Next step before merge Security Review detailsBest 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 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 Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against bbead1bb1ee0. |
2296bc1 to
c7b2314
Compare
|
Maintainer update: rebased this onto current main and prepared the branch at What changed in the prepared head:
Verification run locally:
Waiting on fresh CI/ClawSweeper for this prepared head before merge. |
Title: fix(msteams): paginate thread replies and keep recent context
Summary
fetchThreadRepliesnow follows@odata.nextLinkunder a hard page cap and returns the most recent bounded reply window instead of only the first page.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
@odata.nextLink.$orderby, so a single-page implementation silently favors stale context on long threads.Regression Test Plan (if applicable)
extensions/msteams/src/graph-thread.test.tsUser-visible / Behavior Changes
Diagram (if applicable)
Security Impact (required)
Yes, explain risk + mitigation:Repro + Verification
Environment
pnpm openclawSteps
reply-1,reply-2, and an@odata.nextLink.reply-3,reply-4.fetchThreadReplies(..., limit=2).Expected
reply-3,reply-4.Actual
reply-1,reply-2, dropping the newer page entirely.Evidence
Human Verification (required)
extensions/msteams/src/graph-thread.test.ts; paginated reply path; hard page-cap path; existing thread-helper tests remained green.value, min/max limit clamping, bounded pagination cap.Review Conversations
Compatibility / Migration
Risks and Mitigations