fix(msteams): forward messageBack card actions (Action.Submit) to agent (#60952)#64503
fix(msteams): forward messageBack card actions (Action.Submit) to agent (#60952)#64503ndholakia wants to merge 3 commits intoopenclaw:mainfrom
Conversation
This comment was marked as spam.
This comment was marked as spam.
Greptile SummaryThis PR fixes a long-standing issue where Confidence Score: 4/5Safe to merge; the logic is correct and isolated, but the new code path has no automated test. The fix is functionally correct and well-scoped. The only open item is the absence of an automated test for the new messageBack path — the analogous invoke path already has coverage in monitor-handler.adaptive-card.test.ts and a parallel test here would prevent silent regressions. All other findings are P2. extensions/msteams/src/monitor-handler/message-handler.ts — new card-action branch lacks automated test coverage. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/msteams/src/monitor-handler/message-handler.ts
Line: 197-215
Comment:
**Missing automated test for the new messageBack path**
The analogous `adaptiveCard/action` invoke path (PR #60431) has coverage in `monitor-handler.adaptive-card.test.ts`, but no test was added for this new `messageBack`/`Action.Submit` path. Without a test, a future refactor of the empty-body drop guard could silently re-introduce the regression.
A minimal case to add to `monitor-handler.adaptive-card.test.ts` would send a `type: "message"` activity with empty text and a non-empty `value` object (e.g. `{ intent: "approve", env: "prod" }`) and assert that `dispatchReplyFromConfigWithSettledDispatcher` is called once with `RawBody` matching `[CARD_ACTION] {"intent":"approve","env":"prod"}`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(msteams): forward messageBack card a..." | Re-trigger Greptile |
| // If text and attachments are both empty but activity.value is present, | ||
| // this is a messageBack card action (Action.Submit). Serialize the value | ||
| // payload so it reaches the agent instead of being dropped. (#60952) | ||
| let rawBody = text || attachmentPlaceholder; | ||
| if ( | ||
| !rawBody && | ||
| activity.value != null && | ||
| typeof activity.value === "object" && | ||
| Object.keys(activity.value as Record<string, unknown>).length > 0 | ||
| ) { | ||
| try { | ||
| rawBody = `[CARD_ACTION] ${JSON.stringify(activity.value)}`; | ||
| log.info("messageBack card action detected; forwarding value payload", { | ||
| valueKeys: Object.keys(activity.value as Record<string, unknown>), | ||
| }); | ||
| } catch { | ||
| log.warn("failed to serialize messageBack value payload"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The analogous adaptiveCard/action invoke path (PR #60431) has coverage in monitor-handler.adaptive-card.test.ts, but no test was added for this new messageBack/Action.Submit path. Without a test, a future refactor of the empty-body drop guard could silently re-introduce the regression.
A minimal case to add to monitor-handler.adaptive-card.test.ts would send a type: "message" activity with empty text and a non-empty value object (e.g. { intent: "approve", env: "prod" }) and assert that dispatchReplyFromConfigWithSettledDispatcher is called once with RawBody matching [CARD_ACTION] {"intent":"approve","env":"prod"}.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/monitor-handler/message-handler.ts
Line: 197-215
Comment:
**Missing automated test for the new messageBack path**
The analogous `adaptiveCard/action` invoke path (PR #60431) has coverage in `monitor-handler.adaptive-card.test.ts`, but no test was added for this new `messageBack`/`Action.Submit` path. Without a test, a future refactor of the empty-body drop guard could silently re-introduce the regression.
A minimal case to add to `monitor-handler.adaptive-card.test.ts` would send a `type: "message"` activity with empty text and a non-empty `value` object (e.g. `{ intent: "approve", env: "prod" }`) and assert that `dispatchReplyFromConfigWithSettledDispatcher` is called once with `RawBody` matching `[CARD_ACTION] {"intent":"approve","env":"prod"}`.
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: 7edd1d1047
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| activity.value != null && | ||
| typeof activity.value === "object" && | ||
| Object.keys(activity.value as Record<string, unknown>).length > 0 |
There was a problem hiding this comment.
Accept non-object messageBack payloads
This new branch only forwards card actions when activity.value is an object with at least one key, so messageBack actions that carry a string/primitive payload (or {}) still hit the later empty-body guard and get dropped. In Teams, Action.Submit/messageBack can be configured without object-shaped data, so those button clicks remain invisible to the agent despite this fix. Consider serializing activity.value with the same rules used in the invoke path (string + JSON fallback) instead of requiring a non-empty object.
Useful? React with 👍 / 👎.
7edd1d1 to
7964480
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79644805c4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const deps = createDeps(); | ||
| const { handler, run } = createActivityHandler(); | ||
| const registered = registerMSTeamsHandlers(handler, deps) as MSTeamsActivityHandler & { | ||
| const _registered = registerMSTeamsHandlers(handler, deps) as MSTeamsActivityHandler & { |
There was a problem hiding this comment.
Restore
registered handle in invoke test setup
Renaming the handler registration variable to _registered breaks this test because it still calls registered.run(...) later, which is now an undefined identifier at runtime. As written, the adaptiveCard/action invoke test throws before assertions, so this regression path is no longer validated and the test file can fail immediately when that case runs.
Useful? React with 👍 / 👎.
|
Hi team — friendly bump on this PR. All 3 test cases requested by Greptile review have been added (commit 7964480). CI is green on all msteams-specific jobs. This fixes a real Teams bot interop issue where Action.Submit button clicks are silently dropped. Happy to address any additional feedback. Thanks! |
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: yes. A high-confidence source reproduction is a Teams Real behavior proof Next step before merge Security Review detailsBest possible solution: Keep this PR open and land the prepared head only after redacted real Teams Action.Submit/messageBack proof is posted and required checks finish green. Do we have a high-confidence way to reproduce the issue? Yes. A high-confidence source reproduction is a Teams Is this the best way to solve the issue? Yes for the code direction. The handler-level serialization before the empty-message guard is narrow and regression-tested, but merge should wait for real Teams proof of the prepared head. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against df22284f8550. |
…nt (openclaw#60952) When a user clicks an Action.Submit button on an Adaptive Card in Teams, the click arrives as a messageBack activity (type="message" with empty text and a value object). The message handler strips mentions, finds empty text, and drops the message as "skipping empty message." This change checks for activity.value before the empty-text drop. If the value payload is a non-empty object, it serializes it as `[CARD_ACTION] {json}` so the agent receives the button payload. The detection pattern matches the existing invoke-proxy sidecar workaround and is consistent with the adaptiveCard/action invoke handling added in openclaw#60431. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…essageBack tests The original adaptiveCard/action invoke test uses registered.run() which requires the variable name. The three new messageBack tests call the mock run() directly and only need registerMSTeamsHandlers for its side effect. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
f3bb08c to
55a5ad8
Compare
|
Maintainer update: rebased this onto current main and prepared the branch at What changed in the prepared head:
Verification run locally:
Waiting on fresh CI/ClawSweeper for this prepared head before merge. |
Summary
Forward
Action.SubmitmessageBack activities to the agent instead of dropping them as empty messages. Closes #60952.When a user clicks an
Action.Submitbutton on an Adaptive Card in Teams, the click is sent as a messageBack activity (type="message"with empty text and avalueobject). The message handler strips mentions, finds empty text, and drops the message with "skipping empty message after stripping mentions." The value payload is never forwarded to the agent.This change adds a check for
activity.valuebefore the empty-text drop at line 452 ofmessage-handler.ts. If the value payload is a non-empty object, it serializes it as[CARD_ACTION] {json}so the agent receives the button payload. This is consistent with:adaptiveCard/actioninvoke handling added in fix(msteams): handle Adaptive Card Action.Submit invoke activities #60431 (which handlesAction.Execute)fileConsent/invokehandling added in fix(msteams): handle fileConsent/invoke callback for bot-to-user file upload (#55386) #64087The detection pattern:
activity.type === "message" && !text && activity.value && typeof activity.value === "object"Test plan
Action.Submitbuttons to a Teams DM[CARD_ACTION] {"key":"value"}as the message bodyAction.Executeinvoke path (fix(msteams): handle Adaptive Card Action.Submit invoke activities #60431) is unaffectedContext
This is the companion fix to #55384/#60431 (Action.Execute). Together they cover both invoke formats Teams uses for Adaptive Card interactions. We've been running a sidecar HTTP proxy workaround for this since March 2026 — this fix eliminates the need for that sidecar for card actions.
🤖 Generated with Claude Code