fix(msteams): delete FileConsentCard after user accepts, declines, or upload expires#57364
Conversation
Greptile SummaryThis PR fixes a UX issue in the MS Teams integration where
One gap to address: The Confidence Score: 4/5Safe to merge; the upload-failure gap is a UX inconsistency and the one code path not addressed by this PR. The core implementation is correct, well-tested, and follows MS documentation. The only issue is the upload-failure catch branch not deleting the consent card — all other paths clean up correctly. This is a P2 gap that leaves stale interactive cards in one edge case but does not break the main upload flow or introduce data loss. extensions/msteams/src/monitor-handler.ts — the catch block at lines 196-199 inside handleFileConsentInvoke.
|
| Filename | Overview |
|---|---|
| extensions/msteams/src/monitor-handler.ts | Adds a nested tryDeleteConsentCard() best-effort helper and calls it on four of the five consent-processing code paths (success, decline, expiry, conversation-mismatch). The upload-failure catch branch is the only path that still leaves the consent card visible. |
| extensions/msteams/src/monitor-handler.file-consent.test.ts | Adds deleteActivity mock to the shared test context builder and four new tests covering each cleanup path. Coverage is solid; the 'does not throw' resilience test correctly verifies best-effort behaviour. |
Comments Outside Diff (1)
-
extensions/msteams/src/monitor-handler.ts, line 196-199 (link)Orphaned consent card on upload failure
When
uploadToConsentUrlthrows, thecatchblock sends an error message to the user but never callstryDeleteConsentCard. At the same time,removePendingUpload(uploadId)runs infinally, so the pending upload is gone. This leaves the original consent card visible in the chat with active Allow/Decline buttons — clicking "Accept" again will just produce the "expired" message, which is confusing UX.All other paths in this PR now clean up the card. Consider adding deletion in the
catchbranch as well:Prompt To Fix With AI
This is a comment left during a code review. Path: extensions/msteams/src/monitor-handler.ts Line: 196-199 Comment: **Orphaned consent card on upload failure** When `uploadToConsentUrl` throws, the `catch` block sends an error message to the user but never calls `tryDeleteConsentCard`. At the same time, `removePendingUpload(uploadId)` runs in `finally`, so the pending upload is gone. This leaves the original consent card visible in the chat with active Allow/Decline buttons — clicking "Accept" again will just produce the "expired" message, which is confusing UX. All other paths in this PR now clean up the card. Consider adding deletion in the `catch` branch as well: How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/msteams/src/monitor-handler.ts
Line: 196-199
Comment:
**Orphaned consent card on upload failure**
When `uploadToConsentUrl` throws, the `catch` block sends an error message to the user but never calls `tryDeleteConsentCard`. At the same time, `removePendingUpload(uploadId)` runs in `finally`, so the pending upload is gone. This leaves the original consent card visible in the chat with active Allow/Decline buttons — clicking "Accept" again will just produce the "expired" message, which is confusing UX.
All other paths in this PR now clean up the card. Consider adding deletion in the `catch` branch as well:
```suggestion
} catch (err) {
log.debug?.("file upload failed", { uploadId, error: String(err) });
await tryDeleteConsentCard("upload-failed");
await context.sendActivity(`File upload failed: ${String(err)}`);
} finally {
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(msteams): delete FileConsentCard aft..." | Re-trigger Greptile
8188114 to
2e90523
Compare
|
Addressed — added |
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: yes. source inspection gives a high-confidence reproduction path: current main routes Teams file-consent invokes into Real behavior proof Next step before merge Security Review detailsBest possible solution: Land the active-handler cleanup after the contributor or maintainer adds inspectable after-fix Teams proof, while preserving the successful-accept FileInfoCard replacement behavior. Do we have a high-confidence way to reproduce the issue? Yes, source inspection gives a high-confidence reproduction path: current main routes Teams file-consent invokes into Is this the best way to solve the issue? Yes for the code direction: the rebased patch moves cleanup into the active handler, uses the stored consent-card id or What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against b0966f535644. |
… upload expires The FileConsentCard (Allow/Decline prompt) previously remained in the chat after the user acted on it, cluttering the conversation with stale cards. Now the consent card message is deleted via context.deleteActivity() on all four paths: - User accepts and upload succeeds - User declines - Pending upload expired (not found in memory) - Conversation ID mismatch (cross-conversation replay protection) Deletion is best-effort with a try/catch — if it fails, the upload flow continues normally. Per Microsoft docs: 'Optionally, remove the original consent card if you do not want the user to accept further uploads of the same file.' https://learn.microsoft.com/en-us/microsoftteams/platform/bots/how-to/bots-filesv4 Co-authored-by: Clawdbot <clawdbot@openclaw.ai>
2e90523 to
5e0b7a9
Compare
|
Maintainer prep update for #57364. What changed:
Prepared head:
Verification run locally after the final rebase:
Fresh CI is now running on the prepared head. |
Problem
The
FileConsentCard(Allow/Decline prompt for file uploads) remains in the Teams chat after the user acts on it, cluttering the conversation with stale interactive cards. Users see orphaned Allow/Decline buttons even after files have been successfully uploaded or the upload has expired.Solution
Call
context.deleteActivity(activity.replyToId)to remove the original consent card after processing. The invoke activity from Bot Framework includes the consent card's activity ID inreplyToId.Deletion is applied on all four code paths in
handleFileConsentInvoke:Implemented as a best-effort
tryDeleteConsentCard()helper — if deletion fails (e.g. permissions, timing), the upload flow continues normally with a debug log.Per Microsoft docs:
Testing
deleteActivitymock andreplyToIdfieldAI Disclosure
This PR was developed with AI assistance (Clawdbot on OpenClaw). The bug was identified during real-world testing of file uploads in a Teams bot deployment.