fix(ms teams): preserve proactive conversation payload#59223
fix(ms teams): preserve proactive conversation payload#59223sahilsatralkar wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a proactive-send regression in the Microsoft Teams integration introduced by the Teams SDK migration: the custom adapter was rebuilding a reduced conversation object ( Key changes:
One minor finding: the Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/msteams/src/sdk.ts
Line: 224
Comment:
**Unreachable fallback branch in `??` expression**
The `??` fallback `{ id: conversationId }` can never be evaluated. When we reach this line, `apiClient` must be truthy — which is only possible when both `params.serviceUrl` and `conversationId` are truthy (see the `apiClient` initialisation logic above). `conversationId` is derived from `params.conversation?.id`, so if `params.conversation` were falsy/undefined, `conversationId` would also be undefined, `apiClient` would be `undefined`, and the `if (!apiClient || !conversationId)` guard at line 214 would have already returned `{ id: "unknown" }` before this point.
In practice, `params.conversation` is always truthy here, so the `??` fallback is dead code that can mislead future readers into thinking `params.conversation` may be absent at this point.
```suggestion
conversation: params.conversation,
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(msteams): preserve proactive convers..." | Re-trigger Greptile |
d2f06d9 to
94ee7fb
Compare
6f31d50 to
344fd6e
Compare
|
Thanks for the context here. I did a careful shell check against current Close as implemented on main. The Teams proactive-send 403 regression behind this PR is already fixed in current main by the shipped tenant/AAD forwarding path, with regression coverage for inbound capture and proactive forwarding; the fix was introduced by 2b5b581 and is contained in v2026.4.10. So I’m closing this as already implemented rather than keeping a duplicate issue open. Review detailsBest possible solution: Close this PR and keep the shipped v2026.4.10 implementation as the canonical fix; any future need to preserve additional opaque Teams conversation fields should be tracked as a narrower new bug with a concrete payload field and failing Teams behavior. Do we have a high-confidence way to reproduce the issue? Yes. The repro path is a stored Teams channel/group conversation reference followed by a proactive top-level send; current main has regression tests for tenant capture from inbound channelData and proactive forwarding through the send path. Is this the best way to solve the issue? Yes for the reported regression, but the PR is no longer the best change to land because current main already solves the user-visible 403 failure with explicit tenantId/aadObjectId forwarding and focused tests. Preserving every opaque conversation property would need a separate demonstrated Teams requirement. Security review: Security review cleared: The PR only changes the Microsoft Teams plugin send payload construction and tests, with no new dependencies, permissions, secrets handling, workflow changes, or new network destinations. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 099037cca6f5; fix evidence: release v2026.4.10, commit 2b5b58194b7e. |
Summary
Describe the problem and fix in 2–5 bullets:
If this PR fixes a plugin beta-release blocker, title it
fix(<plugin-id>): beta blocker - <summary>and link the matchingBeta blocker: <plugin-name> - <summary>issue labeledbeta-blocker. Contributors cannot label PRs, so the title is the PR-side signal for maintainers and automation.conversation reference details.
reference layers.
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write
N/A. If the cause is unclear, writeUnknown.conversation metadata needed by the downstream send transport.
proactive transport payload.
CloudAdapter.
Regression Test Plan (if applicable)
For bug fixes or regressions, name the smallest reliable test coverage that should have caught this. Otherwise write
N/A.- extensions/msteams/src/sdk.test.ts
- extensions/msteams/src/send.test.ts
- extensions/msteams/src/messenger.test.ts
path, and 403 failures are still surfaced as actionable send errors.
User-visible / Behavior Changes
List user-visible changes (including defaults/config).
If none, write None.
Teams proactive sends should resume working for previously stored conversation references that require the full conversation payload, instead of failing with HTTP 403 after the SDK migration.
Diagram (if applicable)
For UI changes or non-trivial logic flows, include a small ASCII diagram reviewers can scan quickly. Otherwise write N/A.
Before:
[stored Teams conversation ref] -> [custom proactive adapter rebuilds reduced conversation object] -> [Teams send path loses metadata] -> [HTTP 403]
After:
[stored Teams conversation ref] -> [custom proactive adapter preserves stored conversation payload] -> [SDK client send uses full metadata] -> [delivery succeeds]
Security Impact (required)
Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write None.