Skip to content

fix(msteams): paginate channel thread replies so newest replies aren't dropped#100100

Closed
gunjanjaswal wants to merge 5 commits into
openclaw:mainfrom
gunjanjaswal:fix/98870-msteams-thread-replies-pagination
Closed

fix(msteams): paginate channel thread replies so newest replies aren't dropped#100100
gunjanjaswal wants to merge 5 commits into
openclaw:mainfrom
gunjanjaswal:fix/98870-msteams-thread-replies-pagination

Conversation

@gunjanjaswal

@gunjanjaswal gunjanjaswal commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown

Closes #98870

What Problem This Solves

Fixes an issue where the Teams channel thread context an agent sees is missing the newest replies on any thread with more than 50 replies. fetchThreadReplies requested a single page of /messages/{id}/replies, and since Graph caps $top at 50 and returns replies oldest-first, long threads exposed only the oldest 50 replies to the agent and silently dropped the most recent, usually most relevant, messages.

Why This Change Was Made

fetchThreadReplies now pages through the thread with fetchAllGraphPages (the helper this module already uses for listChannelsForTeam and listTeamsByName, which follows @odata.nextLink) and returns the newest limit replies. Newest selection is by createdDateTime with a fallback to arrival order, so it no longer depends on undocumented page ordering. Per-page $top clamping is unchanged, so short threads behave exactly as before.

Because the replies endpoint is oldest-first with no $orderby, reaching the newest replies means walking the whole thread. The walk is bounded by a named MAX_REPLY_PAGES = 50 (up to 2500 replies scanned). Open question for maintainers: happy to lower that cap if the extra sequential Graph reads on the inbound context path are a concern, though a lower cap means very long threads fall back to fewer of the newest replies.

User Impact

On Teams channel threads with more than 50 replies, the agent now gets the most recent replies as context instead of only the oldest 50, so its responses reflect the current state of the conversation.

Evidence

  • Reuses fetchAllGraphPages, which already handles @odata.nextLink pagination and is covered in graph.test.ts, so there is no new Graph interaction here, just the existing helper applied to replies.
  • Unit tests in graph-thread.test.ts (run in CI): newest 50 selected from 60 paginated replies, newest window by createdDateTime regardless of page order, plus the path, $top clamp, and empty-thread cases on the paginated helper.
  • I do not have a live Teams tenant, so I could not add live long-thread proof. Validation here is the unit tests and CI. If a maintainer with a tenant can capture a redacted over-50-reply run, or is comfortable accepting on the patch plus the reused helper, that would clear the proof gate.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: S labels Jul 4, 2026
@gunjanjaswal gunjanjaswal mentioned this pull request Jul 4, 2026
Open
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 4, 2026
@clawsweeper

clawsweeper Bot commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 4, 2026, 6:16 PM ET / 22:16 UTC.

Summary
The PR changes the Microsoft Teams thread reply helper to page through Graph replies with fetchAllGraphPages, select the newest reply window, and update focused tests.

PR surface: Source +9, Tests +30. Total +39 across 2 files.

Reproducibility: yes. source-level reproduction is high confidence: current main fetches one $top=50 replies page and the monitor handler injects that result into thread context. I did not run a live Microsoft Teams tenant in this read-only review.

Review metrics: 1 noteworthy metric.

  • Graph Page Fanout: 1 request -> up to 50 pages. The changed helper runs on the Teams inbound reply-context path, so real Graph latency and throttling matter before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #98870
Summary: This PR is the active candidate fix for the canonical Teams thread reply pagination bug.

Members:

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • [P1] Add redacted live Teams or Graph output for a channel thread with more than 50 replies showing @odata.nextLink paging and newest replies in the final context window.
  • Include rough latency or throttling observations for the bounded 50-page path, or get an explicit maintainer decision accepting mocked-only proof.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body explicitly says no live Teams tenant proof was run; the provided evidence is unit tests, CI, and mocked Graph helper coverage only. 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 PR can turn one Teams reply-context request into as many as 50 sequential Graph page reads on an inbound channel path, and no live tenant latency or throttling proof is provided.
  • [P1] The implementation depends on live Graph reply pagination and timestamps matching the mocked path; recent Microsoft Learn Answers reports make redacted over-50-reply Graph proof especially useful before merge.

Maintainer options:

  1. Require Live Long-Thread Proof (recommended)
    Pause merge until the PR body includes redacted Teams or Graph output from a channel thread with more than 50 replies showing newest replies in the final context window and acceptable latency.
  2. Tune The Page Cap First
    If maintainers are uncomfortable with up to 50 sequential Graph reads, lower the cap or add an approved performance guard before merge.
  3. Accept Mocked-Only Graph Risk
    Maintainers can intentionally accept the unit-tested helper path without live tenant proof, but they would own any live pagination, timestamp, or throttling mismatch.

Next step before merge

  • [P1] The remaining blockers are contributor or maintainer live-proof and risk-acceptance decisions, not a narrow code defect ClawSweeper can repair automatically.

Security
Cleared: The diff does not change secrets, permissions, package sources, CI, or code-execution surfaces; it only adds bounded authorized Graph reads through existing helpers.

Review details

Best possible solution:

Land the helper-level pagination fix after redacted long-thread Teams or Graph proof, or after maintainers explicitly accept the bounded fanout and mocked-only Graph risk.

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

Yes, source-level reproduction is high confidence: current main fetches one $top=50 replies page and the monitor handler injects that result into thread context. I did not run a live Microsoft Teams tenant in this read-only review.

Is this the best way to solve the issue?

Yes for code shape: the narrow fix belongs in the Teams fetchThreadReplies helper and reuses the existing Graph pagination helper. It is not merge-ready until live Graph proof or explicit maintainer acceptance covers the fanout and pagination risk.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add merge-risk: 🚨 session-state: The PR changes which Teams thread replies are injected into agent context, and live Graph pagination behavior is not proven beyond mocks.
  • add merge-risk: 🚨 availability: The inbound Teams reply-context path can now make up to 50 sequential Microsoft Graph requests for one message.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body explicitly says no live Teams tenant proof was run; the provided evidence is unit tests, CI, and mocked Graph helper coverage only. 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.

Label justifications:

  • P2: This is a focused Teams channel stale-context bugfix with limited blast radius and real context quality impact for long threads.
  • merge-risk: 🚨 session-state: The PR changes which Teams thread replies are injected into agent context, and live Graph pagination behavior is not proven beyond mocks.
  • merge-risk: 🚨 availability: The inbound Teams reply-context path can now make up to 50 sequential Microsoft Graph requests for one message.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body explicitly says no live Teams tenant proof was run; the provided evidence is unit tests, CI, and mocked Graph helper coverage only. 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 +9, Tests +30. Total +39 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 23 14 +9
Tests 1 51 21 +30
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 74 35 +39

What I checked:

Likely related people:

  • sudie-codes: Authored the merged Teams Graph thread-history PR that added fetchThreadReplies, its tests, and the monitor-handler integration. (role: introduced behavior and adjacent feature owner; confidence: high; commits: 8c852d86f759, 01ea7e49212a, 9edfefedf7bf; files: extensions/msteams/src/graph-thread.ts, extensions/msteams/src/graph-thread.test.ts, extensions/msteams/src/monitor-handler/message-handler.ts)
  • BradGroux: Merged the original thread-history PR, raised the oldest-first reply-window concern in review, and has adjacent Teams handler history. (role: reviewer, merger, and adjacent contributor; confidence: high; commits: 8c852d86f759, 03c64df39fe7, 568b0a22bb02; files: extensions/msteams/src/graph-thread.ts, extensions/msteams/src/monitor-handler/message-handler.ts)
  • steipete: Recent blame and shortlog history show substantial current-main touches in the same Teams helper and handler files, though mostly as broad/current-main maintenance rather than the original feature owner. (role: recent adjacent contributor; confidence: medium; commits: d9dbee7676fb, bc27278d6de9, 2ebc7e3ded2a; files: extensions/msteams/src/graph-thread.ts, extensions/msteams/src/monitor-handler/message-handler.ts, extensions/msteams/src/monitor-handler/message-handler.thread-parent.test.ts)
  • vincentkoc: Recent repository history includes Teams thread-handler test setup work and the latest release tag commit carries the current one-page implementation in release provenance. (role: recent area contributor; confidence: medium; commits: d00ab0604838, e085fa1a3ffd; files: extensions/msteams/src/graph-thread.ts, extensions/msteams/src/monitor-handler/message-handler.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 (2 earlier review cycles)
  • reviewed 2026-07-04T21:57:52.151Z sha 8755b30 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-04T22:03:48.985Z sha 8755b30 :: needs real behavior proof before merge. :: none

@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 4, 2026
@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: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 4, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 4, 2026
@gunjanjaswal

gunjanjaswal commented Jul 4, 2026

Copy link
Copy Markdown
Author

Closing this. #98884 already covered the same fix (pagination plus createdDateTime selection) with more proof, and it hit the same wall: ClawSweeper needs live Teams tenant proof that neither attempt can produce right now. No point in a second PR spinning on the same blocker. Leaving #98870 open for a maintainer with a tenant to pick up.

@clawsweeper clawsweeper Bot added the status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. label Jul 4, 2026
@gunjanjaswal
gunjanjaswal deleted the fix/98870-msteams-thread-replies-pagination branch July 4, 2026 22:17
@clawsweeper clawsweeper Bot added 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 4, 2026
This was referenced Jul 5, 2026
Closed
Open
This was referenced Jul 5, 2026
Closed
Closed
Open
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: 🚨 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: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Teams thread context omits newer replies when a channel thread has more than 50 replies

1 participant

@gunjanjaswal