Skip to content

MSTeams: add upload session fallback for large files#32558

Open
zwright8 wants to merge 1 commit intoopenclaw:mainfrom
zwright8:codex/pr26712-msteams-upload-fallback
Open

MSTeams: add upload session fallback for large files#32558
zwright8 wants to merge 1 commit intoopenclaw:mainfrom
zwright8:codex/pr26712-msteams-upload-fallback

Conversation

@zwright8
Copy link
Copy Markdown

@zwright8 zwright8 commented Mar 3, 2026

Summary

Validation

  • pnpm exec vitest run extensions/msteams/src/graph-upload.test.ts

Context

This PR is one focused slice extracted from the previously oversized PR:

@openclaw-barnacle openclaw-barnacle bot added channel: msteams Channel integration: msteams size: M labels Mar 3, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 3, 2026

Greptile Summary

This PR adds a resumable Graph upload-session fallback to graph-upload.ts so that both OneDrive and SharePoint uploads automatically switch to chunked transfers for files larger than 4 MB. The refactor cleanly extracts uploadWithGraphSession, uploadSimpleOrChunked, and toUploadResult helpers, and tests cover the major paths (simple upload, multi-chunk upload, per-chunk retry, stall detection, session creation failure, and SharePoint).

Key observations:

  • The 5 MB chunk size (UPLOAD_SESSION_CHUNK_BYTES) is exactly 16 × 320 KiB, satisfying the Microsoft Graph requirement that all non-final ranges be multiples of 320 KiB.
  • Chunk uploads correctly omit the Authorization header, relying on the pre-authenticated upload session URL returned by Graph — this is intentional and correct per the API spec.
  • There is a subtle off-by-one: UPLOAD_SESSION_MAX_STALLS = 5 combined with the > guard actually allows 6 stalled retries before throwing. The test is internally consistent with this behaviour (6 stall mocks are queued), but the constant's name/value implies a limit of 5.

Confidence Score: 4/5

  • Safe to merge with one minor off-by-one fix recommended before landing
  • The core upload logic is correct, chunk alignment satisfies Graph API constraints, error handling and retry logic work as intended, and tests cover the main paths. The only issue is a semantic inconsistency in the stall-detection guard (> vs >=) that causes one extra retry beyond what the constant's value implies.
  • extensions/msteams/src/graph-upload.ts — stall-detection guard at line 125

Last reviewed commit: 5b6eee3

greptile-apps[bot]
greptile-apps bot reviewed Mar 3, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

extensions/msteams/src/graph-upload.ts

if (nextChunkStart === chunkStart) {
stalledResponses += 1;
if (stalledResponses > UPLOAD_SESSION_MAX_STALLS) {
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-by-one: allows 6 stalls, not 5

UPLOAD_SESSION_MAX_STALLS is set to 5, but the guard uses strict greater-than (>), so stalledResponses must reach 6 before the error is thrown. This means the session is actually retried 6 times, not 5. The test confirms this (it provides 6 stalled responses before expecting the throw).

The constant's name and value imply the limit is 5, but the implementation allows one extra retry. Use >= to match the constant's intent:

Suggested change
if (stalledResponses > UPLOAD_SESSION_MAX_STALLS) {
if (stalledResponses >= UPLOAD_SESSION_MAX_STALLS) {
Prompt To Fix With AI 
This is a comment left during a code review.
Path: extensions/msteams/src/graph-upload.ts
Line: 125

Comment:
**Off-by-one: allows 6 stalls, not 5**

`UPLOAD_SESSION_MAX_STALLS` is set to `5`, but the guard uses strict greater-than (`>`), so `stalledResponses` must reach **6** before the error is thrown. This means the session is actually retried 6 times, not 5. The test confirms this (it provides 6 stalled responses before expecting the throw).

The constant's name and value imply the limit is 5, but the implementation allows one extra retry. Use `>=` to match the constant's intent:

```suggestion
        if (stalledResponses >= UPLOAD_SESSION_MAX_STALLS) {
```

How can I resolve this? If you propose a fix, please make it concise.

@zwright8 zwright8 mentioned this pull request Mar 3, 2026
Closed
18 tasks
@zwright8 zwright8 force-pushed the codex/pr26712-msteams-upload-fallback branch from 5b6eee3 to 45bd05e Compare March 3, 2026 05:06
@BradGroux BradGroux self-assigned this Mar 10, 2026
@BradGroux
Copy link
Copy Markdown
Contributor

BradGroux commented Mar 10, 2026

Hi @zwright8 — thanks for the submission. I’m the new Microsoft Teams maintainer for OpenClaw. Please give me a day or two to work through the open Teams backlog. Also, join the Twitter community for daily MS Teams feedback + updates: https://x.com/i/communities/2031170403607974228

1 similar comment
@BradGroux
Copy link
Copy Markdown
Contributor

BradGroux commented Mar 10, 2026

Hi @zwright8 — thanks for the submission. I’m the new Microsoft Teams maintainer for OpenClaw. Please give me a day or two to work through the open Teams backlog. Also, join the Twitter community for daily MS Teams feedback + updates: https://x.com/i/communities/2031170403607974228

@BradGroux BradGroux mentioned this pull request Mar 17, 2026
Draft
@BradGroux BradGroux mentioned this pull request Mar 26, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@BradGroux BradGroux

Labels

channel: msteams Channel integration: msteams size: M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zwright8 @BradGroux