Skip to content

fix(msteams): update FileConsentCard after user accepts upload#49580

Open
sudie-codes wants to merge 2 commits intoopenclaw:mainfrom
sudie-codes:fix/msteams-file-consent-card-update
Open

fix(msteams): update FileConsentCard after user accepts upload#49580
sudie-codes wants to merge 2 commits intoopenclaw:mainfrom
sudie-codes:fix/msteams-file-consent-card-update

Conversation

@sudie-codes
Copy link
Copy Markdown
Contributor

@sudie-codes sudie-codes commented Mar 18, 2026

Summary

Fixes #47268 — After a user accepts a FileConsentCard in MS Teams, the original consent card remained in its "pending" state instead of being replaced with the completed file info card.

What & Why

Problem: When the bot sends a file to a 1:1 DM user via FileConsentCard, the user clicks "Accept", the file uploads successfully, and a new file info card is sent — but the original consent card stays visible in the chat showing "pending". Users don't know if the upload succeeded, leading to confusion and repeated attempts.

Root cause: monitor-handler.ts sent the file info card via context.sendActivity() but never called context.updateActivity() to replace/remove the original consent card. Additionally, the consent card's activity ID was not being tracked alongside the pending upload metadata.

Fix:

  • pending-uploads.ts: Added consentCardActivityId field to PendingUpload type; fixed timer cleanup gap — removePendingUpload now calls clearTimeout() on the TTL timer so ghost Map deletions can't fire after early removal
  • monitor-handler.ts: After successful upload + file info card send, calls context.updateActivity() to replace the original consent card (non-fatal — upload success is not blocked if the card update fails)
  • sdk-types.ts: Added updateActivity to MSTeamsTurnContext type

Files changed: extensions/msteams/src/pending-uploads.ts, extensions/msteams/src/monitor-handler.ts, extensions/msteams/src/sdk-types.ts

Screenshots

N/A — This is a server-side bot framework fix. The change affects how Bot Framework updateActivity() is called after file upload completion. Verification requires a running Azure Bot Service instance connected to a Teams client. The fix is validated via unit tests that mock the turn context.

Test Results

  • 3 new updateActivity test cases in monitor-handler.file-consent.test.ts
  • 10 new timer/cleanup test cases in pending-uploads.test.ts
  • All 237 msteams tests pass (27 test files)

AI Disclosure

  • AI-assisted (Claude Code with team orchestration)
  • Fully tested — all existing + new tests pass
  • I understand what the code does: turn-context updateActivity() replaces the stale consent card; timer refs are tracked and cleared on early removal to prevent memory leaks

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

greptile-apps bot commented Mar 18, 2026

Greptile Summary

This PR fixes the stale FileConsentCard UX in MS Teams by (1) storing the consent card's activity ID in PendingUpload, (2) calling context.updateActivity() after a successful upload to replace the consent card in-place, and (3) improving timer hygiene in pending-uploads.ts so TTL timers are cancelled on explicit removal rather than left to fire as no-ops.

Key concerns:

  • Duplicate file info card (P1): sendActivity is still unconditionally called with fileInfoCard before the updateActivity call. When consentCardActivityId is present, the same card ends up in two places — a new message and the updated consent card slot. The new test for this path does not assert the total sendActivity message count, so the duplication is not currently caught.
  • Orphaned timer in getPendingUpload (P2): The early-expiry branch deletes from pendingUploads but leaves the entry in pendingUploadTimers without calling clearTimeout, creating a briefly orphaned timer that self-cleans when it eventually fires.
  • The timer-tracking improvements to removePendingUpload and clearPendingUploads, the new pending-uploads.test.ts, and the sdk-types.ts addition are all solid.

Confidence Score: 3/5

  • Not safe to merge without addressing the duplicate file info card — when consentCardActivityId is set, the same card appears as both a new message and the updated consent card, which is a visible UX regression.
  • The timer cleanup improvements and new test coverage are well done, but the core fix introduces a duplication: sendActivity and updateActivity both post the same fileInfoCard when consentCardActivityId is present. This is a P1 logic issue that needs resolution before merge.
  • extensions/msteams/src/monitor-handler.ts (duplicate card logic) and the corresponding test in monitor-handler.file-consent.test.ts (missing assertion on sendActivity call count).

Comments Outside Diff (2)

  1. extensions/msteams/src/monitor-handler.ts, line 105-126 (link)

    P1 Duplicate file info card when consentCardActivityId is present

    When consentCardActivityId is stored, both sendActivity (line 105-108) and updateActivity (line 114-118) are called with the identical fileInfoCard attachment. This means the user sees the card twice in the chat:

    1. The original consent card's position → replaced by fileInfoCard via updateActivity
    2. A brand-new message → also showing fileInfoCard via sendActivity

    If the goal of updateActivity is to eliminate the stale "pending" consent prompt, the sendActivity call for the file info card should be skipped when replacing the card in-place — or vice versa (only sendActivity, using a lighter "accepted" update for the consent card). The current combination unconditionally does both, duplicating the attachment in the conversation.

    Consider making the sendActivity call conditional:

    // Send confirmation card (only if not replacing the consent card in-place)
    if (!pendingFile.consentCardActivityId) {
      await context.sendActivity({
        type: "message",
        attachments: [fileInfoCard],
      });
    }
    
    if (pendingFile.consentCardActivityId) {
      try {
        await context.updateActivity({
          id: pendingFile.consentCardActivityId,
          type: "message",
          attachments: [fileInfoCard],
        });
      } catch (updateErr) {
        // Non-fatal: the upload succeeded; just log if Teams rejects the update
        log.debug?.("failed to update consent card activity", {
          uploadId,
          error: String(updateErr),
        });
      }
    }

    The existing test for this path only asserts that sendActivity was called with an invokeResponse and doesn't assert the total call count for message attachments, so this duplication is not currently caught by the test suite.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: extensions/msteams/src/monitor-handler.ts
    Line: 105-126
    
    Comment:
    **Duplicate file info card when `consentCardActivityId` is present**
    
    When `consentCardActivityId` is stored, both `sendActivity` (line 105-108) and `updateActivity` (line 114-118) are called with the identical `fileInfoCard` attachment. This means the user sees the card twice in the chat:
    
    1. The original consent card's position → replaced by `fileInfoCard` via `updateActivity`
    2. A brand-new message → also showing `fileInfoCard` via `sendActivity`
    
    If the goal of `updateActivity` is to eliminate the stale "pending" consent prompt, the `sendActivity` call for the file info card should be skipped when replacing the card in-place — or vice versa (only `sendActivity`, using a lighter "accepted" update for the consent card). The current combination unconditionally does both, duplicating the attachment in the conversation.
    
    Consider making the `sendActivity` call conditional:
    
    ```typescript
    // Send confirmation card (only if not replacing the consent card in-place)
    if (!pendingFile.consentCardActivityId) {
      await context.sendActivity({
        type: "message",
        attachments: [fileInfoCard],
      });
    }
    
    if (pendingFile.consentCardActivityId) {
      try {
        await context.updateActivity({
          id: pendingFile.consentCardActivityId,
          type: "message",
          attachments: [fileInfoCard],
        });
      } catch (updateErr) {
        // Non-fatal: the upload succeeded; just log if Teams rejects the update
        log.debug?.("failed to update consent card activity", {
          uploadId,
          error: String(updateErr),
        });
      }
    }
    ```
    
    The existing test for this path only asserts that `sendActivity` was called with an `invokeResponse` and doesn't assert the total call count for message attachments, so this duplication is not currently caught by the test suite.
    
    How can I resolve this? If you propose a fix, please make it concise.
  2. extensions/msteams/src/pending-uploads.ts, line 60-68 (link)

    P2 Orphaned timer when getPendingUpload manually expires an entry

    When getPendingUpload detects a TTL-expired entry and deletes it from pendingUploads (line 67), it does not call clearTimeout or remove the entry from pendingUploadTimers. The background timer is now effectively orphaned — it will still fire, do a no-op pendingUploads.delete(id), and then self-clean via pendingUploadTimers.delete(id), so there is no functional breakage. However, it is a minor inconsistency: pendingUploads and pendingUploadTimers are transiently out of sync, and the timer wastes a small amount of memory until it fires.

    For consistency with removePendingUpload, consider cancelling the timer here too:

    if (Date.now() - entry.createdAt > PENDING_UPLOAD_TTL_MS) {
      pendingUploads.delete(id);
      const timer = pendingUploadTimers.get(id);
      if (timer !== undefined) {
        clearTimeout(timer);
        pendingUploadTimers.delete(id);
      }
      return undefined;
    }
    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: extensions/msteams/src/pending-uploads.ts
    Line: 60-68
    
    Comment:
    **Orphaned timer when `getPendingUpload` manually expires an entry**
    
    When `getPendingUpload` detects a TTL-expired entry and deletes it from `pendingUploads` (line 67), it does not call `clearTimeout` or remove the entry from `pendingUploadTimers`. The background timer is now effectively orphaned — it will still fire, do a no-op `pendingUploads.delete(id)`, and then self-clean via `pendingUploadTimers.delete(id)`, so there is no functional breakage. However, it is a minor inconsistency: `pendingUploads` and `pendingUploadTimers` are transiently out of sync, and the timer wastes a small amount of memory until it fires.
    
    For consistency with `removePendingUpload`, consider cancelling the timer here too:
    
    ```typescript
    if (Date.now() - entry.createdAt > PENDING_UPLOAD_TTL_MS) {
      pendingUploads.delete(id);
      const timer = pendingUploadTimers.get(id);
      if (timer !== undefined) {
        clearTimeout(timer);
        pendingUploadTimers.delete(id);
      }
      return undefined;
    }
    ```
    
    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: 105-126

Comment:
**Duplicate file info card when `consentCardActivityId` is present**

When `consentCardActivityId` is stored, both `sendActivity` (line 105-108) and `updateActivity` (line 114-118) are called with the identical `fileInfoCard` attachment. This means the user sees the card twice in the chat:

1. The original consent card's position → replaced by `fileInfoCard` via `updateActivity`
2. A brand-new message → also showing `fileInfoCard` via `sendActivity`

If the goal of `updateActivity` is to eliminate the stale "pending" consent prompt, the `sendActivity` call for the file info card should be skipped when replacing the card in-place — or vice versa (only `sendActivity`, using a lighter "accepted" update for the consent card). The current combination unconditionally does both, duplicating the attachment in the conversation.

Consider making the `sendActivity` call conditional:

```typescript
// Send confirmation card (only if not replacing the consent card in-place)
if (!pendingFile.consentCardActivityId) {
  await context.sendActivity({
    type: "message",
    attachments: [fileInfoCard],
  });
}

if (pendingFile.consentCardActivityId) {
  try {
    await context.updateActivity({
      id: pendingFile.consentCardActivityId,
      type: "message",
      attachments: [fileInfoCard],
    });
  } catch (updateErr) {
    // Non-fatal: the upload succeeded; just log if Teams rejects the update
    log.debug?.("failed to update consent card activity", {
      uploadId,
      error: String(updateErr),
    });
  }
}
```

The existing test for this path only asserts that `sendActivity` was called with an `invokeResponse` and doesn't assert the total call count for message attachments, so this duplication is not currently caught by the test suite.

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.ts
Line: 60-68

Comment:
**Orphaned timer when `getPendingUpload` manually expires an entry**

When `getPendingUpload` detects a TTL-expired entry and deletes it from `pendingUploads` (line 67), it does not call `clearTimeout` or remove the entry from `pendingUploadTimers`. The background timer is now effectively orphaned — it will still fire, do a no-op `pendingUploads.delete(id)`, and then self-clean via `pendingUploadTimers.delete(id)`, so there is no functional breakage. However, it is a minor inconsistency: `pendingUploads` and `pendingUploadTimers` are transiently out of sync, and the timer wastes a small amount of memory until it fires.

For consistency with `removePendingUpload`, consider cancelling the timer here too:

```typescript
if (Date.now() - entry.createdAt > PENDING_UPLOAD_TTL_MS) {
  pendingUploads.delete(id);
  const timer = pendingUploadTimers.get(id);
  if (timer !== undefined) {
    clearTimeout(timer);
    pendingUploadTimers.delete(id);
  }
  return undefined;
}
```

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

Last reviewed commit: "fix(msteams): update..."

Copy link
Copy Markdown
Contributor

@BradGroux BradGroux left a comment

Choose a reason for hiding this comment

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

Core FileConsentCard fix is correct, tracking consentCardActivityId in PendingUpload and calling updateActivity() after upload success is the right approach. Non-fatal handling (catch around updateActivity) is appropriate. Timer cleanup with clearTimeout() on early removal is a good catch.

Key concern: This PR overlaps heavily with #49582 (security fix), #49585 (Graph chat ID), and #49587 (singleton guard), ~400 lines of shared code. Specifically:

  • resolveGraphChatId here is less safe than #49585's version (returns first result without ambiguity check vs. returning null on ambiguous matches)
  • Recommend using #49585's implementation as the canonical one

Merge order: This should land last in the series (#49582#49929#49585#49925#49587#49580) and will need a rebase to drop the duplicated code.

Minor nits: double-if guard in monitor could be if/else, and one error log is missing the error object.

✅ Approve, with rebase after the other PRs land.

@sudie-codes sudie-codes force-pushed the fix/msteams-file-consent-card-update branch from 2d78fac to 62d1967 Compare March 20, 2026 19:14
@sudie-codes sudie-codes force-pushed the fix/msteams-file-consent-card-update branch from 62d1967 to 6578951 Compare March 20, 2026 19:19
@BradGroux
Copy link
Copy Markdown
Contributor

BradGroux commented Mar 23, 2026

@sudie-codes Heads up — both #49925 and this PR need rebases against main. #49587 has been merged.

For #49925: there are merge conflicts and 2 failing CI checks (check, extensions) that need resolving.

For this PR (#49580): once #49925 is rebased and merged, you'll need to rebase this one too. The sdk-types.ts updateActivity signature from #49925 will conflict with yours — your stricter version ({ id: string } & Record<string, unknown>) is the correct final shape, just preserve deleteActivity from #49925 during the rebase.

Copy link
Copy Markdown
Contributor

@BradGroux BradGroux left a comment

Choose a reason for hiding this comment

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

Review: fix(msteams) — update FileConsentCard after user accepts upload

Verdict: Request changes 🔴 (minor — one missing test)

The intent is right — replacing the stale "pending" consent card with the completed file info card is a real UX improvement. Implementation logic is correct.

What works well

  • consentCardActivityId tracking in pending-uploads.ts is clean. Storing the activity ID at consent-card-send time and retrieving it at upload-complete time is the right approach.
  • Non-fatal updateActivity fallback — if the card replacement fails, fall back to sending a new message. Good defensive pattern for cases where the bot loses edit permission or the activity ID goes stale.
  • Dual guard structure in monitor-handler.ts is correct: if (!consentCardActivityId) sends a new message; if (consentCardActivityId) replaces the consent card in-place. These are complementary guards, not a dual-send bug.
  • Pending upload test coverage is thorough — TTL expiry, timer cleanup on explicit removal, clearPendingUploads, and edge cases (undefined/unknown IDs).

Missing test (blocking)

Add a test asserting: when consentCardActivityId is present and updateActivity succeeds, sendActivity is not called for the file info card.

The current test suite covers the updateActivity failure fallback path (which correctly sends a new message), but does not assert the happy path avoids a duplicate send. This is the regression guard that ensures future refactors do not accidentally introduce double cards.

CI failures

Not caused by this PR. The extensions test failure is the pre-existing setup-surface.test.ts async assertion issue.

Bottom line

Logic is sound, coverage is close. Add the happy-path no-duplicate-send assertion and this is good to go.

BradGroux

This comment was marked as duplicate.

Copy link
Copy Markdown
Contributor

@BradGroux BradGroux left a comment

Choose a reason for hiding this comment

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

Review: FileConsentCard Update After Upload

Summary

The PR adds a consentCardActivityId field to PendingUpload and uses it in the accept handler to update the original consent card after a successful upload. The timer cleanup improvements (clearing the timeout on accept/decline) are solid and prevent stale state.

Blocker: consentCardActivityId is never populated in production

The core issue is a data flow gap. The storePendingUpload() call in file-consent-helpers.ts (the real send path) does not capture or store the activity ID returned from sending the consent card. The field exists in the type, and the accept handler reads it, but nothing in the actual production flow ever writes it.

The tests pass because they inject consentCardActivityId directly into the pending upload fixture. This masks the fact that in a real bot conversation, the field will always be undefined, and the updateActivity() call will silently skip (or fail).

To fix: Capture the activity ID from the sendActivity() response when sending the consent card, and pass it through to storePendingUpload().

What works well

  • Timer cleanup on accept/decline prevents orphaned timeouts
  • Test coverage is thorough for the happy path
  • Type additions are clean

Verdict

Request Changes. The timer fix is worth merging on its own, but the headline feature (updating the consent card) cannot work without wiring the activity ID from the send path into storage.

@sudie-codes
Copy link
Copy Markdown
Contributor Author

@BradGroux Thanks for the thorough review — both issues addressed in bf4548726a:

Blocker (consentCardActivityId never populated): Added setPendingUploadActivityId() to pending-uploads.ts. In the proactive send path (send.ts), the activity ID from sendProactiveActivity() is now stored back. In the reply/thread path (messenger.ts), the consent activity is tagged with _pendingUploadId so sendMessageInContext() can call setPendingUploadActivityId() after getting the response.

Missing test: Added a happy-path test asserting that when consentCardActivityId is present and updateActivity succeeds, sendActivity is not called with a file info card (only the invokeResponse call). Also added unit tests for the new setPendingUploadActivityId function.

All 258 msteams tests pass.

Copy link
Copy Markdown
Contributor

@BradGroux BradGroux left a comment

Choose a reason for hiding this comment

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

Review: fix(msteams) FileConsentCard update after user accepts upload

Commit reviewed: bf45487


Blocker from prior review: RESOLVED ✅

The original review flagged that consentCardActivityId was never populated in the real send path, making the in-place card update dead code. This is now fixed end-to-end:

  1. buildActivity() tags the consent activity with _pendingUploadId in the activity value
  2. sendMSTeamsMessages() extracts the tag, strips it before sending over the wire, and calls setPendingUploadActivityId(uploadId, messageId) to persist the mapping
  3. send.ts (proactive path) also calls setPendingUploadActivityId after sendActivity
  4. monitor-handler.ts retrieves the stored consentCardActivityId and uses it with updateActivity to replace the consent card in-place, with a fallback to sendActivity if the update fails

The new setPendingUploadActivityId() function in pending-uploads.ts cleanly encapsulates the persistence logic.


Test coverage

  • ✅ Happy path test: asserts updateActivity called once, sendActivity NOT called with file info card
  • ✅ Fallback test: when updateActivity throws, verifies sendActivity is used instead

Suggestions (non-blocking)

1. Guard setPendingUploadActivityId calls (Low)
Consider wrapping setPendingUploadActivityId(...) in a try/catch with logging. If the persistence layer ever changes or throws unexpectedly, the upload flow should not break just because activity ID tracking failed. The upload itself is the critical path; the in-place card update is a UX enhancement.

2. Telemetry on updateActivity fallback (Low)
When updateActivity fails and the fallback sendActivity path is used, consider emitting a log or telemetry event. This helps diagnose Teams-side issues (expired activity IDs, permission changes) without needing to reproduce the failure.

3. Duplicate consent card scenario (Low)
If retries or race conditions cause multiple consent cards to be sent for the same upload ID, only the last setPendingUploadActivityId call wins. The earlier consent card(s) become orphaned in the conversation. This is an edge case but worth considering whether a cleanup mechanism or dedupe check would be valuable.

4. Race condition: accept before persistence (Low)
If the user clicks "Accept" on the consent card before setPendingUploadActivityId has completed, the consentCardActivityId lookup returns nothing and the fallback (send new activity) is used. This degrades gracefully, but is unverified by tests. A test asserting the fallback behavior when consentCardActivityId is missing would strengthen confidence.


Summary

This PR correctly fixes the FileConsentCard update flow. The consentCardActivityId is now wired through both the messenger and proactive send paths. The fallback behavior on update failure is clean. All suggestions above are non-blocking improvements.

@BradGroux
Copy link
Copy Markdown
Contributor

This has merge conflicts against main. Can you rebase and resolve? The review looks good — ready to merge once conflicts are cleared.

sudie-codes and others added 2 commits March 28, 2026 11:24
- Adds consentCardActivityId to PendingUpload so the consent card
  activity can be replaced in-place after upload succeeds
- Uses context.updateActivity() to replace the FileConsentCard with
  the file info card; falls back to sendActivity if update fails
- Adds updateActivity to MSTeamsTurnContext type
- Fixes timer leak in pending-uploads: clears TTL setTimeout on
  explicit removal and on clearPendingUploads()
- Adds pending-uploads.test.ts covering all new timer/cleanup paths
…th updateActivity test

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@sudie-codes sudie-codes force-pushed the fix/msteams-file-consent-card-update branch from bf45487 to c50db13 Compare March 28, 2026 18:25
@sudie-codes
Copy link
Copy Markdown
Contributor Author

@BradGroux Rebased onto origin/main — 3 merge conflicts resolved:

  1. sdk-types.ts — kept main's broader updateActivity + deleteActivity signatures
  2. monitor-handler.file-consent.test.ts — kept main's type assertion while preserving updateActivity destructuring
  3. messenger.ts — kept the PR's separated buildActivity/sendActivity flow (needed for _pendingUploadId extraction) while incorporating main's feedbackLoopEnabled option

All 459 msteams tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: msteams Channel integration: msteams size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[msteams] FileConsentCard not updated after user accepts — consent card remains frozen

2 participants