Skip to content

fix(msteams): page channel thread replies#98884

Closed
qingminglong wants to merge 3 commits into
openclaw:mainfrom
qingminglong:codex/issue-98870-teams-thread-replies
Closed

fix(msteams): page channel thread replies#98884
qingminglong wants to merge 3 commits into
openclaw:mainfrom
qingminglong:codex/issue-98870-teams-thread-replies

Conversation

@qingminglong

@qingminglong qingminglong commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #98870

What Problem This Solves

Fixes an issue where Microsoft Teams channel users in long thread conversations would lose the newest replies from agent context once the thread had more than 50 replies.

Why This Change Was Made

Teams Graph exposes additional channel-message replies through @odata.nextLink, while the list-replies endpoint supports $top but not $orderby. This change uses the existing bounded Graph pagination helper for thread replies, sorts replies by createdDateTime before limiting when timestamps are parseable, and falls back to arrival-order slicing when Graph omits or returns an unparseable timestamp.

User Impact

Agents responding in long Teams channel threads can now see the most recent relevant replies instead of being limited to the oldest first page of replies. The newest-window selection no longer depends on whether Graph returns reply pages oldest-first or newest-first.

Evidence

  • Claim proved: fetchThreadReplies now requests replies through bounded Graph pagination and returns the newest limited reply window without relying on Graph response order.
  • Commands / artifacts:
    • pnpm install --frozen-lockfile completed successfully after the worktree initially lacked node_modules.
    • node scripts/run-vitest.mjs extensions/msteams/src/graph-thread.test.ts passed before follow-up: 1 test file, 28 tests.
    • After the ordering follow-up, node scripts/run-vitest.mjs extensions/msteams/src/graph-thread.test.ts passed: 1 test file, 30 tests.
    • After the lint follow-up, node scripts/run-vitest.mjs extensions/msteams/src/graph-thread.test.ts passed: 1 test file, 30 tests.
    • After the proof refresh on current head 6efb75654c039efa10e2814434620aa6ba9e1006, node scripts/run-vitest.mjs extensions/msteams/src/graph-thread.test.ts extensions/msteams/src/graph.test.ts passed: 2 test files, 49 tests.
    • git diff --check passed after the lint follow-up and again on current head 6efb75654c039efa10e2814434620aa6ba9e1006.
    • pnpm exec oxlint extensions/msteams/src/graph-thread.test.ts extensions/msteams/src/graph-thread.ts passed after the lint follow-up.
    • PYTHONUTF8=1 python .agents\skills\autoreview\scripts\autoreview --mode local passed after the ordering follow-up with autoreview clean: no accepted/actionable findings reported.
  • Observed result: focused graph-thread tests cover the paginated helper call, newest-reply limit behavior, minimum/maximum limit handling, empty results, the explicit 50-page cap, newest-window selection when Graph pages arrive oldest-first, newest-window selection when Graph pages arrive newest-first, and arrival-order fallback when createdDateTime is missing or unparseable.
  • Real helper boundary proof: the refreshed graph.test.ts run exercises the shared fetchAllGraphPages helper that fetchThreadReplies now calls, including multi-page @odata.nextLink chains, Graph root stripping for v1.0/beta next links, maxPages truncation, and early findOne behavior. This proves the PR is wired through the existing Graph pagination helper rather than only a standalone reply-order mock.
  • Dependency contract checked: Microsoft Graph list channel-message replies documents $top with a maximum of 50 and says other OData query parameters are not supported; Graph paging docs direct callers to follow @odata.nextLink until it is absent.
  • Not tested / proof gaps: no live Microsoft Teams tenant / Graph API integration proof was run. This environment has no usable MSTEAMS_*, MICROSOFT_*, AZURE_*, or GRAPH_* credentials and no channels.msteams credentials in the local user config, so I could not safely query a real tenant. Remaining maintainer/live proof needed: run against a real channel thread with more than 50 replies, capture redacted Graph output showing @odata.nextLink pages and newest replies included in the final context window, and record latency/throttling behavior for the bounded 50-page cap.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: S labels Jul 2, 2026
@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 12:42 AM ET / 04:42 UTC.

Summary
This PR changes Microsoft Teams fetchThreadReplies to use the shared Graph pagination helper, select the newest bounded replies by createdDateTime when possible, and add focused unit coverage.

PR surface: Source +25, Tests +85. Total +110 across 2 files.

Reproducibility: yes. from source inspection: current main fetches one $top=50 replies page and the monitor handler injects those replies 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 page requests. The helper runs on the inbound Teams channel reply-context path, so live Graph latency and throttling matter before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #98870
Summary: This PR is an active candidate fix for the canonical Teams long-thread reply pagination bug; several same-root PRs exist, but none has merged or provided sufficient live proof.

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 from a channel thread with more than 50 replies showing newest replies in the final context window.
  • Include basic latency or throttling observations for the bounded 50-page path, with private tenant, user, phone, endpoint, and credential details redacted.
  • If contributor-side live proof is not feasible, ask a maintainer with a Teams test tenant to provide proof or explicitly override the mocked-only risk.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body and comments provide focused unit/helper command output, but explicitly state that no live Microsoft Teams tenant or Graph integration proof was run. 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 inbound Teams reply-context Graph request into as many as 50 sequential page requests, and there is no live tenant latency or throttling evidence yet.
  • [P1] The changed context window depends on live Graph pagination and timestamp behavior that is currently proven by source reasoning and mocks, not redacted real Teams or Graph output.
  • [P1] A competing open candidate targets the same canonical issue, so maintainers should choose one branch before merging overlapping fixes.

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 context plus basic latency or throttling observations.
  2. Accept Mocked Graph Evidence
    Maintainers may intentionally accept the unit/source proof for this small fix, but they would own any live Graph pagination, timestamp, or fanout mismatch.
  3. Pick One Canonical Branch
    If the competing open candidate is preferred, preserve any stronger tests or implementation details and close the other candidate after the chosen branch is landed or clearly sponsored.

Next step before merge

  • [P1] Needs maintainer/live Teams proof or explicit acceptance of mocked-only Graph risk; there is no remaining narrow automated code repair on this PR branch.

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

Review details

Best possible solution:

Land one canonical Teams helper fix that reuses fetchAllGraphPages, preserves the newest bounded reply window, and has live Graph proof or an explicit maintainer override for mocked-only risk.

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

Yes, from source inspection: current main fetches one $top=50 replies page and the monitor handler injects those replies 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, the helper-level pagination plus newest-window selection is the narrow maintainable fix shape. The remaining blocker is live external API proof or explicit maintainer acceptance of that proof gap, not a different code layer.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a focused Microsoft Teams stale-context bugfix with limited blast radius but real agent-context impact for long channel threads.
  • merge-risk: 🚨 session-state: The PR changes which Teams thread replies enter the agent conversation context, and live Graph ordering/pagination is not proven beyond mocks.
  • merge-risk: 🚨 availability: The inbound reply-context path can now perform up to 50 sequential Microsoft Graph reads for one channel reply.
  • 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 and comments provide focused unit/helper command output, but explicitly state that no live Microsoft Teams tenant or Graph integration proof was run. 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 +25, Tests +85. Total +110 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 39 14 +25
Tests 1 100 15 +85
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 139 29 +110

What I checked:

Likely related people:

  • sudie-codes: Authored the merged Teams Graph thread-history work that added fetchThreadReplies, its tests, and later adjacent thread-context changes. (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 and review discussion there explicitly called out the oldest-50 reply limitation; also appears in nearby Teams/Microsoft tracker work. (role: reviewer, merger, and adjacent area contributor; confidence: high; commits: 8c852d86f759, 01ea7e49212a, 9edfefedf7bf; 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-02T04:12:43.561Z sha f25371e :: needs real behavior proof before merge. :: [P2] Remove the unused Graph path helper
  • reviewed 2026-07-05T04:35:40.195Z sha 6efb756 :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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: 🚨 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 2, 2026
@qingminglong

qingminglong commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Updated the PR body Evidence after the ordering follow-up. The branch now sorts parseable createdDateTime values before selecting the newest reply window, preserves arrival-order slicing when timestamps are missing/unparseable, and has fresh focused proof: node scripts/run-vitest.mjs extensions/msteams/src/graph-thread.test.ts passed 30 tests, git diff --check passed, and autoreview is clean. Live Teams tenant proof is still explicitly listed as unavailable. @clawsweeper re-review

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 2, 2026
@qingminglong

qingminglong commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Pushed a lint follow-up and refreshed the PR body Evidence. Latest focused proof: node scripts/run-vitest.mjs extensions/msteams/src/graph-thread.test.ts passed 30 tests, git diff --check passed, and pnpm exec oxlint extensions/msteams/src/graph-thread.test.ts extensions/msteams/src/graph-thread.ts passed. Live Teams tenant proof remains explicitly unavailable. @clawsweeper re-review

@qingminglong

qingminglong commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed the PR body Evidence on current head 6efb75654c039efa10e2814434620aa6ba9e1006 with an additional helper-boundary proof run: node scripts/run-vitest.mjs extensions/msteams/src/graph-thread.test.ts extensions/msteams/src/graph.test.ts passed 2 files / 49 tests, and git diff --check passed again. This now covers both the fetchThreadReplies caller behavior and the shared fetchAllGraphPages @odata.nextLink path it uses.

Live Teams tenant proof is still explicitly unavailable in this environment; the PR body now states the remaining proof gap and the exact maintainer/live proof needed for a real 50+ reply channel thread and 50-page cap latency/throttling check. @clawsweeper re-review

@BradGroux BradGroux mentioned this pull request Jul 2, 2026
Draft
@clawsweeper clawsweeper Bot added status: 🔁 re-review loop A fresh ClawSweeper review was explicitly requested after the latest review. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 2, 2026
@qingminglong

qingminglong commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Current-head follow-up for #98884:

I rechecked this loop state from the current PR body/checks. The branch already has current-head focused proof for 6efb75654c039efa10e2814434620aa6ba9e1006, including graph-thread.test.ts + graph.test.ts, git diff --check, oxlint, and current GitHub checks are green.

The remaining gap is not another mock/unit test: ClawSweeper asked for real Teams/Graph proof from a live channel thread with more than 50 replies, including redacted @odata.nextLink paging output, newest replies present in the final context window, and latency/throttling notes for the bounded 50-page cap.

This environment has no MSTEAMS_*, MICROSOFT_*, AZURE_*, or GRAPH_* credentials, so I cannot safely produce that live tenant proof from here. Maintainer/live-proof help is needed for that final evidence, or maintainers need to explicitly accept the remaining mock-only Graph risk.

@qingminglong

qingminglong commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Maintainer/live-proof handoff for #98884:

I looked into whether I could provide the remaining Microsoft Teams / Graph evidence myself. The practical blocker is that I do not currently have a Microsoft work/school tenant account with Teams channel access, and creating a temporary Microsoft 365 tenant plus seeding a real channel thread with more than 50 replies would be a lot of setup for one PR proof run.

The current branch already has focused mock/helper proof and green checks on head 6efb75654c039efa10e2814434620aa6ba9e1006. The remaining evidence ClawSweeper asked for is specifically live tenant proof, not another unit test:

  • a real Teams channel root message with more than 50 replies
  • Graph replies request showing @odata.nextLink paging from /teams/{team-id}/channels/{channel-id}/messages/{message-id}/replies?$top=50
  • redacted output/screenshots showing later pages include the newest replies used by the context window
  • rough latency/throttling notes for the bounded pagination path

I tried to find a contributor-side way to supply this, but it requires a work/school Microsoft 365 tenant, Teams data, and Graph consent for permissions such as ChannelMessage.Read.All; without an existing tenant this is not realistically quick or low-risk to manufacture. I am going to stop adding mock-only proof here so the PR does not keep spinning.

Could a maintainer or someone with an existing Teams test tenant either provide the live Graph paging artifact, or explicitly decide that the current mocked Graph pagination evidence is enough for this small fix?

This was referenced Jul 4, 2026
Closed
Closed
Open
@qingminglong qingminglong reopened this Jul 5, 2026
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: 🔁 re-review loop A fresh ClawSweeper review was explicitly requested after the latest review. labels Jul 5, 2026
@github-actions github-actions Bot mentioned this pull request Jul 5, 2026
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

@qingminglong