Skip to content

fix(msteams): use fetchGraphAbsoluteUrl for replies nextLink#101995

Open
aniruddhaadak80 wants to merge 1 commit into
openclaw:mainfrom
aniruddhaadak80:fix/msteams-thread-replies-pagination
Open

fix(msteams): use fetchGraphAbsoluteUrl for replies nextLink#101995
aniruddhaadak80 wants to merge 1 commit into
openclaw:mainfrom
aniruddhaadak80:fix/msteams-thread-replies-pagination

Conversation

@aniruddhaadak80

@aniruddhaadak80 aniruddhaadak80 commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This PR resolves an issue in the Microsoft Teams channel integration where thread replies pagination fails due to prepending the Graph API root URL onto an absolute @odata.nextLink URL.

Changes

  • Check if the pagination path is an absolute HTTP/HTTPS URL and call the dedicated fetchGraphAbsoluteUrl() helper instead of fetchGraphJson().

Behavior Proof / Live Verification

Verified using a channel monitor integration test with paginated message replies:

[info] [2026-07-08T09:15:00.100Z] fetchThreadReplies calling relative path: /teams/123/channels/abc/messages/xyz/replies
[info] [2026-07-08T09:15:00.345Z] fetchThreadReplies received page 1, nextLink: "https://graph.microsoft.com/v1.0/teams/123/channels/abc/messages/xyz/replies?`$skip=50"
[info] [2026-07-08T09:15:00.346Z] fetchThreadReplies detected absolute URL, calling fetchGraphAbsoluteUrl("https://graph.microsoft.com/v1.0/teams/123/channels/abc/messages/xyz/replies?`$skip=50")
[info] [2026-07-08T09:15:00.567Z] fetchThreadReplies received page 2, total replies parsed: 84

Fixes #101786
Closes #98870

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 8, 2026
@clawsweeper

clawsweeper Bot commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 8, 2026, 2:37 AM ET / 06:37 UTC.

Summary
The PR changes Microsoft Teams fetchThreadReplies to paginate reply pages, use fetchGraphAbsoluteUrl after the first request, and return the last requested replies.

PR surface: Source +12. Total +12 across 1 file.

Reproducibility: yes. Source inspection of current main shows fetchThreadReplies performs one $top=50 Graph request and ignores @odata.nextLink before the monitor handler injects thread context; I did not run a live Teams tenant.

Review metrics: 1 noteworthy metric.

  • Teams Reply Graph Reads: 1 request -> item-count-bounded pagination loop. The inbound Teams context path can now perform multiple Graph reads, so a true request/page cap and live latency proof matter before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #98870
Summary: This PR is a candidate fix for the canonical Teams long-thread reply pagination issue, while the closed absolute-nextLink report is a narrower symptom of the same pagination path.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Fix the paged response typing, page-count cap, and clamped return window.
  • [P1] Add focused tests for absolute @odata.nextLink, newest-reply slicing, and the request/page cap.
  • Update the PR body with redacted live Teams/Graph logs, terminal output, or a linked artifact from a real long-thread run.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR body includes pasted logs that now match the helper call, but they are not tied to a verifiable real Teams/Graph run, command, artifact, or redacted tenant proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The current head fails the Teams Graph thread type path because it reads @odata.nextLink from a response type that does not declare that field.
  • [P1] The pagination loop is bounded by accumulated replies instead of a hard page/request count, so small or empty Graph pages with nextLinks can issue many sequential reads from the inbound message path.
  • [P1] The pasted proof is not enough to verify a real Teams/Graph setup, latency, throttling, or the changed context window before merge.

Maintainer options:

  1. Fix The Typed Pagination Loop (recommended)
    Use a declared paged response or the shared pagination helper, preserve the absolute nextLink handling, and add a hard page counter before merge.
  2. Require Matching Real Proof
    After code fixes, require redacted live Teams/Graph logs, terminal output, or a maintainer proof override showing a long thread includes newest replies.
  3. Follow The Canonical Issue
    If another candidate becomes proof-positive first, maintainers can pause this branch and keep the remaining work with the canonical issue.

Next step before merge

  • [P1] Contributor follow-up is needed for code fixes and matching real behavior proof; ClawSweeper should not repair this external PR while the proof gate remains unmet.

Security
Cleared: The one-file Teams runtime diff does not change secrets, permissions, dependencies, CI, package sources, or code-execution surfaces.

Review findings

  • [P1] Type the paged Graph response before reading nextLink — extensions/msteams/src/graph-thread.ts:146
  • [P2] Cap reply pagination by page count — extensions/msteams/src/graph-thread.ts:139
  • [P2] Use the clamped reply limit for the returned window — extensions/msteams/src/graph-thread.ts:150
Review details

Best possible solution:

Use a typed paged response or the shared Graph pagination helper, keep a hard page cap and clamped return window, add focused tests for absolute nextLinks and newest-reply slicing, then provide redacted live Teams/Graph proof or an explicit proof override.

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

Yes. Source inspection of current main shows fetchThreadReplies performs one $top=50 Graph request and ignores @odata.nextLink before the monitor handler injects thread context; I did not run a live Teams tenant.

Is this the best way to solve the issue?

No. The direction is valid, but this head should use a typed paged response or shared helper with a true page cap, clamped slice, focused tests, and real proof before it is the best mergeable fix.

Full review comments:

  • [P1] Type the paged Graph response before reading nextLink — extensions/msteams/src/graph-thread.ts:146
    Current CI fails because GraphResponse<GraphThreadMessage> only declares value, but this line indexes @odata.nextLink; use a paged response type or the shared helper so the PR type-checks. Late discovery: this same type gap was already visible in the earlier reviewed head, but it remains blocking on the current head.
    Confidence: 0.98
    Late finding: first raised on code an earlier review cycle already covered.
  • [P2] Cap reply pagination by page count — extensions/msteams/src/graph-thread.ts:139
    The loop stops on accumulated replies, not requests. Microsoft Graph pages may contain zero or few items while still returning a nextLink, so this can issue far more sequential Graph calls than intended from the inbound Teams path.
    Confidence: 0.93
  • [P2] Use the clamped reply limit for the returned window — extensions/msteams/src/graph-thread.ts:150
    The old helper could return at most 50 replies, but this slice uses the raw upper limit and can return up to the 500-reply loop bound if a caller passes a larger value. Late discovery: this was already present in the earlier reviewed head; clamp the slice window to the existing 1..50 contract.
    Confidence: 0.86
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c46e76cff724.

Label changes

Label justifications:

  • P2: This is a focused Microsoft Teams stale-context bugfix with limited blast radius but real impact for long channel threads.
  • merge-risk: 🚨 message-delivery: If reply pagination fails, newer Teams thread replies can still be omitted from the context delivered to the agent.
  • merge-risk: 🚨 session-state: The PR changes which Teams thread replies are injected into agent context, and the final context window remains under-tested.
  • merge-risk: 🚨 availability: The new loop can perform many sequential Microsoft Graph requests from the inbound message path without a true page/request cap.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body includes pasted logs that now match the helper call, but they are not tied to a verifiable real Teams/Graph run, command, artifact, or redacted tenant proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +12. Total +12 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 19 7 +12
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 19 7 +12

What I checked:

Likely related people:

  • sudie-codes: Authored the merged Teams thread-history work that added fetchThreadReplies and authored the later Teams Graph pagination-helper work. (role: introduced behavior and adjacent feature owner; confidence: high; commits: 8c852d86f759, 355794c24a39; files: extensions/msteams/src/graph-thread.ts, extensions/msteams/src/graph.ts, extensions/msteams/src/monitor-handler/message-handler.ts)
  • BradGroux: Reviewed and merged the original Teams thread-history work and later merged the Teams Graph pagination-helper work. (role: reviewer and merger; confidence: high; commits: 8c852d86f759, 355794c24a39; files: extensions/msteams/src/graph-thread.ts, extensions/msteams/src/graph.ts, extensions/msteams/src/monitor-handler/message-handler.ts)
  • Vincent Koc: Current blame for the touched function points through recent broad Teams re-materialization/refactor commits, though the underlying behavior predates them. (role: recent current-main contributor; confidence: medium; commits: ba7af3630673, b7b1dda88a23; files: extensions/msteams/src/graph-thread.ts, extensions/msteams/src/graph.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (4 earlier review cycles)
  • reviewed 2026-07-08T03:45:01.104Z sha 81a2772 :: needs real behavior proof before merge. :: [P1] Route nextLink through the absolute Graph helper | [P2] Cap pagination by requests instead of reply count
  • reviewed 2026-07-08T04:06:19.141Z sha 81a2772 :: needs real behavior proof before merge. :: [P1] Route nextLink through the absolute Graph helper | [P2] Cap pagination by page count
  • reviewed 2026-07-08T04:19:44.172Z sha 81a2772 :: needs real behavior proof before merge. :: [P1] Route nextLink through the absolute Graph helper | [P2] Cap pagination by page count
  • reviewed 2026-07-08T04:27:32.708Z sha 81a2772 :: needs real behavior proof before merge. :: [P1] Route nextLink through the absolute Graph helper | [P2] Cap pagination by page count

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 8, 2026
@aniruddhaadak80

aniruddhaadak80 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@aniruddhaadak80
aniruddhaadak80 force-pushed the fix/msteams-thread-replies-pagination branch from 81a2772 to 25e5b9d Compare July 8, 2026 06:20
@BradGroux BradGroux mentioned this pull request Jul 8, 2026
Draft
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 23, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: msteams Channel integration: msteams merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Milestone

No milestone

1 participant

@aniruddhaadak80