Skip to content

fix(msteams): bound attachment JSON response reads to prevent OOM#97709

Closed
cxbAsDev wants to merge 1 commit into
openclaw:mainfrom
cxbAsDev:fix/bound-msteams-json-response
Closed

fix(msteams): bound attachment JSON response reads to prevent OOM#97709
cxbAsDev wants to merge 1 commit into
openclaw:mainfrom
cxbAsDev:fix/bound-msteams-json-response

Conversation

@cxbAsDev

@cxbAsDev cxbAsDev commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Replace unbounded response.json() with readProviderJsonResponse (16 MiB cap) in MSTeams Bot Framework attachment info fetch and Graph collection fetch. Both already have try-catch error handling — this adds size bounding to the JSON parse path.

Real behavior proof (required for external PRs)

Behavior addressed: Unbounded response.json() replaced with readProviderJsonResponse (16 MiB cap) in two MSTeams attachment paths.

Real environment tested: Linux, Node 22, OpenClaw main @ 87db23e

Exact steps or command run after fix:

node --import tsx proof-msteams.mts

Evidence after fix:

=== Bot Framework attachmentInfo ===
  chunks: 17, cancelled: true
  PASS
=== Graph collection ===
  chunks: 17, cancelled: true
  PASS

PASS: both MSTeams read paths bounded at 16 MiB

All existing tests pass: 31/31 tests (bot-framework + graph).

What was not tested: Live Bot Framework v3 attachments API or Microsoft Graph API calls.

Risk

Low. Both functions already have try-catch that returns undefined or { status, items: [] } on errors. The overflow error from readProviderJsonResponse is caught by the existing catch blocks, preserving the same graceful degradation.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: XS labels Jun 29, 2026
@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 6:05 AM ET / 10:05 UTC.

Summary
The PR replaces three MSTeams attachment JSON response.json() reads with readProviderJsonResponse and adds oversized-response regression tests for Graph and Bot Framework attachment paths.

PR surface: Source +11, Tests +158. Total +169 across 4 files.

Reproducibility: yes. for the source-level bug class: current main and v2026.6.10 have raw response.json() reads in the affected MSTeams attachment paths, and the PR head routes those reads through the shared capped helper. I did not run local tests because this review was read-only.

Review metrics: 2 noteworthy metrics.

  • Attachment JSON reads: 3 changed. The current head covers the Graph collection, Graph message, and Bot Framework attachmentInfo reads, so runtime coverage is broad enough for this attachment path.
  • Current-head proof coverage: 2 of 3 paths shown. The PR body terminal output predates the Graph main-message change, so proof needs one more current-head path before merge.

Stored data model
Persistent data-model change detected: vector/embedding metadata: extensions/msteams/src/attachments.graph.test.ts, vector/embedding metadata: extensions/msteams/src/attachments/graph.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
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:

  • Return a fresh streaming Response in the Bot Framework oversized-response test and rerun focused MSTeams attachment validation.
  • Update the PR body with current-head terminal or log proof for Graph message, Graph collection, and Bot Framework attachmentInfo overflow behavior.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR body includes terminal proof for Bot Framework attachmentInfo and Graph collection, but the current head also changes Graph main-message parsing without after-fix proof for that path; update the PR body with redacted terminal/log output and re-review should run automatically, or a maintainer can comment @clawsweeper re-review.

Risk before merge

  • [P1] The Bot Framework oversized-response test can hang or time out because it passes a streaming Response through createMockFetch, which returns response.clone(), while readResponseWithLimit awaits reader.cancel() on overflow.
  • [P1] The PR body proof covers Bot Framework attachmentInfo and Graph collection, but the current head also changes Graph main-message parsing and needs refreshed after-fix proof for that path.
  • [P1] Oversized successful Microsoft JSON metadata now fails at the shared 16 MiB cap and falls through existing empty/undefined paths, so maintainers should explicitly accept that fail-closed behavior for attachment metadata.

Maintainer options:

  1. Fix the test and refresh proof (recommended)
    Change the Bot Framework overflow test to avoid cloned streaming responses, rerun focused MSTeams attachment validation, and update the PR body with current-head proof for Graph message, Graph collection, and Bot Framework attachmentInfo.
  2. Accept with maintainer-owned proof
    A maintainer could independently prove the exact head and accept the shared 16 MiB cap behavior, but that would replace the missing contributor proof rather than satisfy it.
  3. Pause until proof is refreshed
    Keep the PR paused if the author cannot provide current-head proof and maintainers do not want to own the remaining test and availability-hardening validation gap.

Next step before merge

  • [P1] Human/contributor follow-up is needed because current-head real behavior proof is incomplete and the remaining test defect must be fixed before maintainer cap acceptance.

Security
Cleared: No new dependency, permission, secret, workflow, install, package, endpoint, or auth-header surface is introduced; the diff reuses an existing SDK HTTP helper.

Review findings

  • [P2] Return a fresh streaming Response in this test — extensions/msteams/src/attachments/bot-framework.test.ts:541-548
Review details

Best possible solution:

Keep the bounded-reader runtime changes, return a fresh streaming Response in the Bot Framework overflow test, refresh current-head proof for all three reads, then let maintainers accept the 16 MiB cap tradeoff before merge.

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

Yes for the source-level bug class: current main and v2026.6.10 have raw response.json() reads in the affected MSTeams attachment paths, and the PR head routes those reads through the shared capped helper. I did not run local tests because this review was read-only.

Is this the best way to solve the issue?

No as-is. Reusing readProviderJsonResponse is the best runtime fix, but the submitted branch should fix the cloned-stream test and refresh proof for the added Graph main-message read before merge.

Full review comments:

  • [P2] Return a fresh streaming Response in this test — extensions/msteams/src/attachments/bot-framework.test.ts:541-548
    This test sends a streaming Response through createMockFetch, but that helper returns entry.response.clone(). Since readResponseWithLimit awaits reader.cancel() on overflow, the cloned tee branch can leave cancellation unsettled and hang the test before the assertions. Use a local fetch mock or response factory that returns a fresh Response for this oversized case.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority MSTeams availability hardening PR with channel-limited blast radius and concrete pre-merge blockers.
  • merge-risk: 🚨 automation: The PR adds a regression test that can hang on cloned streaming response cancellation, which can break focused test automation after merge.
  • merge-risk: 🚨 compatibility: Existing oversized successful attachment metadata responses would change from native unbounded parsing to labelled 16 MiB failures handled by fallback paths.
  • merge-risk: 🚨 availability: If Microsoft returns unexpectedly large success metadata, MSTeams attachment download paths can now return empty or undefined instead of completing.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • 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 terminal proof for Bot Framework attachmentInfo and Graph collection, but the current head also changes Graph main-message parsing without after-fix proof for that path; update the PR body with redacted terminal/log output and re-review should run automatically, or a maintainer can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +11, Tests +158. Total +169 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 2 14 3 +11
Tests 2 158 0 +158
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 172 3 +169

What I checked:

Likely related people:

  • steipete: Live path history shows repeated recent MSTeams attachment and shared media hardening/refactor work near this surface, including security-sensitive flows and bounded remote download centralization. (role: recent area contributor; confidence: high; commits: c56b56e514f8, 53d007bc878c, 8f22632a2900; files: extensions/msteams/src/attachments/graph.ts, extensions/msteams/src/attachments/bot-framework.ts, packages/media-core/src/read-response-with-limit.ts)
  • sudie-codes: History shows this contributor introduced and repaired the MSTeams Bot Framework DM media and Graph attachment paths that the PR now hardens. (role: feature introducer; confidence: high; commits: ab9be8dba547, 2c211d171e88, 2084441b51aa; files: extensions/msteams/src/attachments/graph.ts, extensions/msteams/src/attachments/bot-framework.ts)
  • pgondhi987: Recent GitHub path history shows the attachment fetch DNS pinning work that routes these MSTeams attachment downloads through the guarded fetch path now feeding the JSON reads. (role: recent adjacent owner; confidence: medium; commits: 9497629c1e89; files: extensions/msteams/src/attachments/graph.ts, extensions/msteams/src/attachments/bot-framework.ts)
  • Dallin Romney: Local git blame attributes the current raw JSON read lines to the grafted main commit 2ec6708, though broader path history indicates that commit carried existing MSTeams code forward. (role: current line provenance; confidence: low; commits: 2ec670898018; files: extensions/msteams/src/attachments/graph.ts, extensions/msteams/src/attachments/bot-framework.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.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. labels Jun 29, 2026
@BradGroux BradGroux mentioned this pull request Jun 29, 2026
Draft
This was referenced Jun 29, 2026
Merged
Closed
@cxbAsDev
cxbAsDev force-pushed the fix/bound-msteams-json-response branch from 1b47339 to f017a92 Compare June 30, 2026 09:05
@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. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 30, 2026
@cxbAsDev @claude
fix(msteams): bound Graph main message JSON read plus oversized-respo…
6c8f0e4 
…nse tests

- Replace unbounded msgRes.json() in downloadMSTeamsGraphMedia with
  readProviderJsonResponse (16 MiB cap), matching the existing bounded
  reads in fetchGraphCollection and fetchBotFrameworkAttachmentInfo.
- Add three oversized-response tests covering all bounded-read paths:
  Graph main message, hostedContents collection, and Bot Framework
  attachmentInfo. Each test verifies the stream is cancelled on overflow
  and the caller handles the error gracefully.

Co-Authored-By: Claude <[email protected]>
@cxbAsDev
cxbAsDev force-pushed the fix/bound-msteams-json-response branch from f017a92 to 6c8f0e4 Compare June 30, 2026 09:51
@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 30, 2026
@cxbAsDev cxbAsDev closed this Jun 30, 2026
This was referenced Jul 2, 2026
Closed
Merged
@cxbAsDev
cxbAsDev deleted the fix/bound-msteams-json-response branch July 15, 2026 03:32
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: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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.

1 participant

@cxbAsDev