fix(msteams): handle fileConsent/invoke callback for bot-to-user file upload (#55386)#63954
fix(msteams): handle fileConsent/invoke callback for bot-to-user file upload (#55386)#63954sudie-codes wants to merge 1 commit intoopenclaw:mainfrom
Conversation
|
Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit. |
Greptile SummaryThis PR fixes a cross-process race in the MSTeams Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/quality improvements that do not affect correctness or security. The core fix is correct: the dual-lookup strategy, file locking, TTL pruning, UUID validation, and conversation-mismatch protection all work as intended. Errors from the FS store are caught at every call site so a misconfigured state dir cannot crash the invoke handler. The three P2 findings (orphan blob accumulation, missing ensureStoreDir in read paths, overly permissive UUID regex) are quality improvements without impact on the primary user path. extensions/msteams/src/pending-uploads-fs.ts — minor gaps in orphan-blob cleanup and storeDir pre-existence assumptions. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/msteams/src/pending-uploads-fs.ts
Line: 183-184
Comment:
**Orphan blobs from crash are never reclaimed**
The comment says orphan blobs (written before the index update) are "disk litter that the next prune cycle can skip," but `readAndPruneLocked` only iterates over *expired index entries* — it never scans the filesystem for blobs with no matching index entry. A blob written here that then crashes before `withFileLock` updates the index will persist on disk indefinitely, accumulating across deployments.
Consider adding a periodic or startup sweep that lists `*.blob` files in the store dir and unlinks any whose ID is absent from the index, or acknowledge the gap in the comment.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/msteams/src/pending-uploads-fs.ts
Line: 206-213
Comment:
**`get()`, `remove()`, `count()` throw instead of returning defaults when `storeDir` doesn't exist**
`withFileLock` calls `ensureJsonFile`, which on ENOENT tries to create the index file via `writeJsonFileAtomically`. If the store directory hasn't been created yet (i.e., `store()` was never called), the parent-dir creation in `writeJsonFileAtomically` will fail with ENOENT, and `get()`/`remove()`/`count()` will throw rather than returning `undefined`/`0`.
`resolvePendingUpload` and `removeResolvedPendingUpload` both catch and swallow these errors, so the runtime is unaffected. But `count()` has no such guard and will throw unexpectedly for any caller that invokes it on a freshly created store before the first `store()` call (e.g., future monitoring/diagnostic code). Adding `await ensureStoreDir(storeDir)` at the top of `get()`, `remove()`, and `count()` would make the API uniformly safe.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/msteams/src/pending-uploads-fs.ts
Line: 79-82
Comment:
**`isSafeUploadId` regex accepts non-UUID strings**
The pattern `^[0-9a-f-]{36}$` allows strings like 36 hyphens or `aaaa-aaaa-aaaa-aaaa-aaaa-aaaa-aaaa-aaaa` that aren't canonical UUIDs but do pass the check. Path traversal is still prevented (no `/`, `\`, or `.`), so there's no security gap. Tightening to a proper UUID shape makes the intent clearer and closes the acceptance window:
```suggestion
return /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(id);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(msteams): handle fileConsent/invoke ..." | Re-trigger Greptile |
| // just disk litter that the next prune cycle can skip). | ||
| await fs.promises.writeFile(blobPath(storeDir, id), new Uint8Array(params.buffer)); |
There was a problem hiding this comment.
The comment says orphan blobs (written before the index update) are "disk litter that the next prune cycle can skip," but readAndPruneLocked only iterates over expired index entries — it never scans the filesystem for blobs with no matching index entry. A blob written here that then crashes before withFileLock updates the index will persist on disk indefinitely, accumulating across deployments.
Consider adding a periodic or startup sweep that lists *.blob files in the store dir and unlinks any whose ID is absent from the index, or acknowledge the gap in the comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/pending-uploads-fs.ts
Line: 183-184
Comment:
**Orphan blobs from crash are never reclaimed**
The comment says orphan blobs (written before the index update) are "disk litter that the next prune cycle can skip," but `readAndPruneLocked` only iterates over *expired index entries* — it never scans the filesystem for blobs with no matching index entry. A blob written here that then crashes before `withFileLock` updates the index will persist on disk indefinitely, accumulating across deployments.
Consider adding a periodic or startup sweep that lists `*.blob` files in the store dir and unlinks any whose ID is absent from the index, or acknowledge the gap in the comment.
How can I resolve this? If you propose a fix, please make it concise.| async get(id) { | ||
| if (!id || !isSafeUploadId(id)) { | ||
| return undefined; | ||
| } | ||
| const storeDir = getStoreDir(); | ||
| const indexPath = getIndexPath(); | ||
| return await withFileLock(indexPath, EMPTY_INDEX, async () => { | ||
| const current = await readAndPruneLocked(storeDir, indexPath); |
There was a problem hiding this comment.
get(), remove(), count() throw instead of returning defaults when storeDir doesn't exist
withFileLock calls ensureJsonFile, which on ENOENT tries to create the index file via writeJsonFileAtomically. If the store directory hasn't been created yet (i.e., store() was never called), the parent-dir creation in writeJsonFileAtomically will fail with ENOENT, and get()/remove()/count() will throw rather than returning undefined/0.
resolvePendingUpload and removeResolvedPendingUpload both catch and swallow these errors, so the runtime is unaffected. But count() has no such guard and will throw unexpectedly for any caller that invokes it on a freshly created store before the first store() call (e.g., future monitoring/diagnostic code). Adding await ensureStoreDir(storeDir) at the top of get(), remove(), and count() would make the API uniformly safe.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/pending-uploads-fs.ts
Line: 206-213
Comment:
**`get()`, `remove()`, `count()` throw instead of returning defaults when `storeDir` doesn't exist**
`withFileLock` calls `ensureJsonFile`, which on ENOENT tries to create the index file via `writeJsonFileAtomically`. If the store directory hasn't been created yet (i.e., `store()` was never called), the parent-dir creation in `writeJsonFileAtomically` will fail with ENOENT, and `get()`/`remove()`/`count()` will throw rather than returning `undefined`/`0`.
`resolvePendingUpload` and `removeResolvedPendingUpload` both catch and swallow these errors, so the runtime is unaffected. But `count()` has no such guard and will throw unexpectedly for any caller that invokes it on a freshly created store before the first `store()` call (e.g., future monitoring/diagnostic code). Adding `await ensureStoreDir(storeDir)` at the top of `get()`, `remove()`, and `count()` would make the API uniformly safe.
How can I resolve this? If you propose a fix, please make it concise.| function isSafeUploadId(id: string): boolean { | ||
| // UUIDs are safe; reject anything that could traverse paths. | ||
| return /^[0-9a-f-]{36}$/i.test(id); | ||
| } |
There was a problem hiding this comment.
isSafeUploadId regex accepts non-UUID strings
The pattern ^[0-9a-f-]{36}$ allows strings like 36 hyphens or aaaa-aaaa-aaaa-aaaa-aaaa-aaaa-aaaa-aaaa that aren't canonical UUIDs but do pass the check. Path traversal is still prevented (no /, \, or .), so there's no security gap. Tightening to a proper UUID shape makes the intent clearer and closes the acceptance window:
| function isSafeUploadId(id: string): boolean { | |
| // UUIDs are safe; reject anything that could traverse paths. | |
| return /^[0-9a-f-]{36}$/i.test(id); | |
| } | |
| return /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(id); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/pending-uploads-fs.ts
Line: 79-82
Comment:
**`isSafeUploadId` regex accepts non-UUID strings**
The pattern `^[0-9a-f-]{36}$` allows strings like 36 hyphens or `aaaa-aaaa-aaaa-aaaa-aaaa-aaaa-aaaa-aaaa` that aren't canonical UUIDs but do pass the check. Path traversal is still prevented (no `/`, `\`, or `.`), so there's no security gap. Tightening to a proper UUID shape makes the intent clearer and closes the acceptance window:
```suggestion
return /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(id);
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f34fb7255
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // just disk litter that the next prune cycle can skip). | ||
| await fs.promises.writeFile(blobPath(storeDir, id), new Uint8Array(params.buffer)); | ||
| try { | ||
| await withFileLock(indexPath, EMPTY_INDEX, async () => { |
There was a problem hiding this comment.
Serialize index bootstrap before storing pending uploads
The new store() path calls withFileLock(indexPath, EMPTY_INDEX, ...), but withFileLock initializes index.json via an access/write check before taking the lock. On a fresh directory, two concurrent send processes can both observe "missing index"; one process can commit an entry, then the other process’s pre-lock fallback write can replace the index with EMPTY_INDEX, dropping the first upload row. That dropped row makes a valid consent callback look expired because the upload can no longer be resolved. Bootstrap/creation of the index file needs to be done under the same lock (or with atomic create semantics) to avoid this first-write clobber race.
Useful? React with 👍 / 👎.
Summary
Bot-to-user file uploads via `openclaw message send --media` were silently failing because the `fileConsent/invoke` callback could not find the pending upload. The root cause was cross-process: the CLI sender registered the pending upload in its own in-memory Map and then exited, but the `fileConsent/invoke` callback lands on the long-running monitor webhook in a different process, so the invoke handler always saw an empty store and could not honor "Accept".
Fixes #55386.
Fix
Relationship to PR #49580
PR #49580 (`fix/msteams-file-consent-card-update`) is in flight and updates the consent card after a successful upload (calls `context.updateActivity`). That PR extends the in-memory `PendingUpload` with `consentCardActivityId` and teaches the handler to replace the consent card in place. This PR is a separate concern — it fixes the cross-process pending upload lookup so the accept handler actually has a file to upload in the first place. The two changes touch overlapping files (`monitor-handler.ts`, `send.ts`, `file-consent-helpers.ts`) so some manual rebasing may be needed, but the behaviors are compatible: the FS store can persist `consentCardActivityId` in a follow-up if needed.
Test plan