feat(teams): installation lifecycle and bot join events (#217) - #293
Conversation
Port vercel/chat aaeede70 (#899, chat@4.40.0) and the Teams handler half
of 2e2426d1 (#914, chat@4.41.0):
- installationUpdate (add/add-upgrade/remove/remove-upgrade) dispatches to
on_installed / on_uninstalled with a persistable channel_id (activity
serviceUrl, else the validated token's service URL)
- the bot's own channel/group-chat join dispatches on_member_joined_channel
- bot_user_id is 28:{app_id}; the self check is case-insensitive
- team-scoped conversationUpdate caches Graph channel context from the
base 19: conversation id
Closes #217
# Conflicts: # CHANGELOG.md
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Teams adapter now dispatches installation and bot-join events, updates bot identity and channel-context handling, and adds webhook-based tests and documentation. The changelog also removes a temporary Slack divergence note. ChangesTeams installation and bot joins
Slack changelog note removal
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature · Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant WebhookRequest
participant TeamsAdapter
participant Chat
WebhookRequest->>TeamsAdapter: handle_webhook(activity, options)
TeamsAdapter->>TeamsAdapter: _dispatch_activity(installationUpdate or conversationUpdate)
alt Installation update
TeamsAdapter->>Chat: process_installed(event, options) or process_uninstalled(event, options)
else Qualifying bot join
TeamsAdapter->>Chat: process_member_joined_channel(event, options)
end
Merge Risk: ⚪ Minimal · up to The Teams lifecycle and bot-join changes have no established merge-blocking issue. Built-in sending rejects disallowed destinations. Live Teams verification remains pending, but does not by itself prevent merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new lifecycle callbacks preserve the existing authentication path, and built-in messaging operations validate destinations before sending. No introduced security vulnerability was established. Applications remain responsible for callback-owned state, duplicate deliveries, and installation/removal ordering; destination trust and downstream behavior were not fully verified. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 2 | ❌ 2 | ❓ 1❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation The implementation and tests cover the active coding objectives in Full details: Out of Scope Changes checkExplanation The PR adds a Slack pasted-table and alert-attachment changelog entry and removes a temporary Slack divergence note. These changes do not implement or support the Teams installation and bot-join objectives in Full details: Docstring CoverageExplanation Docstring coverage is 23.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 88 functions across 6 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit hops where Teams events flow Comment |
…ards with tests (#217 review)
# Conflicts: # CHANGELOG.md
|
Merge gate: CI green (Analyze (actions), Analyze (python), CodeQL, Lint & Type Check, test (3.12), test (3.13); CodeRabbit pass/approved) on 98da0c4, which already contains origin/main f381f69 (no new merge needed). Local Codex review (gpt-6-astra, xhigh, --base origin/main) on 98da0c4: "No actionable regressions found. All 792 Teams and installation-event tests passed, along with targeted lint/format checks and type checking; live Teams behavior was not verified." 3 astra rounds on this PR (round 1 P2 root-path connector URL in token fallback, fixed; rounds 2 and 3 clean). Local re-validation on the same SHA: ruff/format/audit/fidelity --check-docs/strict 733/733 green, pytest 7481 passed / 24 skipped, pyrefly 0 errors. Bots: CodeRabbit approved. Merging with --admin (Protect Main requires a code-owner approval). |
Summary
Ports the Teams half of upstream installation lifecycle support and bot join events.
installationUpdate(add,add-upgrade,remove,remove-upgrade) now dispatches to the coreon_installed/on_uninstalledhandlers from [4.41/C4] Core lifecycle events: message updated/deleted, installed/uninstalled, app context changed #196. The event carries a persistablechannel_idbuilt from the activity'sserviceUrl, or the validated token'sservice_urlwhen the activity has none. It isNonewhen neither exists.on_member_joined_channelonce, withuser_id == bot_user_idandinviter_id = from.id.bot_user_iddecision: we adopt upstream's28:{app_id}, which is the id Teams gives the bot infrom/recipient. This is recorded indocs/UPSTREAM_SYNC.md._is_bot_account_id) is now case-insensitive.author.is_meand both new handlers use it._cache_user_contextnow caches Graph channel context from a team-scopedconversationUpdatethat has nochannelData.channel, using the base19:conversation id.teams/installation.py:INSTALLATION_ACTIONS,parse_installation_action,is_install_action.Upstream commits mapped
aaeede70feat(teams): dispatch bot join events (feat(teams): dispatch bot join events vercel/chat#899, chat@4.40.0). CovershandleConversationUpdate,resolveBotJoin,botUserId = 28:{appId},isBotAccountIdand the team-context cache fallback.2e2426d1feat(teams): add installation lifecycle events (feat(teams): add installation lifecycle events vercel/chat#914, chat@4.41.0), Teams handler half only. CovershandleInstallationUpdateandinstallation.ts. The core half was [4.41/C4] Core lifecycle events: message updated/deleted, installed/uninstalled, app context changed #196 and thesendTohalf was [4.41/T1] Teams routing & outbound text: stop <at> rewriting, conversationType in thread ids, per-service-URL clients #216.Verify-first result
event.token.service_urlexists onTokenProtocoland onJsonWebTokeninmicrosoft-teams-api2.0.13.4, 2.0.16 and 2.1.0. OnJsonWebTokenit defaults tohttps://smba.trafficmanager.net/teamsand drops any trailing slash. In skip-auth mode the SDK's placeholder token sets it to the body'sserviceUrl, or""when the body has none.Tests ported
All tests drive the real
handle_webhook→BridgeHttpAdapter→ SDKHttpServer→_dispatch_activitypath. They use the #180 skip-auth fixture, with shared helpers intests/_teams_harness.py.tests/test_teams_installation.pycovers all 9 tests ininstallation.test.ts› "Teams installation lifecycle", including theit.eachcases.a:group-chat type preservation, a disallowed token URL, and action parsing.tests/test_teams_joins.pycovers all 14 tests injoins.test.ts› "Teams bot joins".test_teams_connect.py::TestLazyTeamsIdentity, from [4.41/T6] Teams auth: custom token factory precedence, webhook_verifier, sovereign endpoint allowlists #221.19:prefix,author.is_meis case-insensitive, and text mention detection uses@28:{app_id}.toUpperCase()casing tests pass whether or not the comparison ignores case.getattrprocessor probe, token fallback, URL validation, team-cache fallback, personal-chat skip, the28:id, tenant fallback, recipient check, conversation type, self check). At least one new test failed for every break.test_teams_connect.py: thebot_user_idexpectations now use28:(upstreamconnect.test.tsasserts28:lazy-appetc.).microsoft-teams-*2.0.16, run locally through auv run --withoverlay.Fidelity: adapter tests are not fidelity-mapped.
Delta vs committed report (HEAD): missing 104 -> 104 (+0)(after merging current main), soscripts/fidelity_target.jsonis unchanged. Strict at the pin: 733/733.Teams native streaming and fixture-replay tests were run explicitly (
test_teams_native_streaming.py,test_fixture_replay.py,integration/test_replay_fetch_messages_teams.py): 125 passed. A live Teams check of installs and joins is pending.Divergences
One divergence, with a row in the non-parity table, a code breadcrumb and a regression test. When the activity has no
serviceUrl, the token'sservice_urlmust pass_validate_service_urlbefore it is encoded into the persistedchannel_id. The SDK strips the token URL's trailing slash, so a root-path endpoint first gets that/back; otherwise the check would rejecthttps://host. If it fails, the adapter logs a warning andchannel_idisNone. The activity's ownserviceUrlis encoded without this check, exactly like message thread IDs (parity).Consumer impact
on_installed/on_uninstalledfor installation updates.on_member_joined_channelwhen the bot joins a channel or group chat. A team install fires both, with the samechannel_id.TeamsAdapter.bot_user_idchanges from{app_id}to28:{app_id}. Plain-text@{app_id}no longer counts as a text mention in core;@28:{app_id}does. Teams mention entities are unchanged.author.is_menow ignores GUID casing.conversationUpdatelogs a warning (upstream parity).Merge gate
Independent review (2 reviewers, 5 findings): 4 fixed, 1 declined.
nullforchannelData.channel,channelData.team,channelData,conversationorfromcrashed_cache_user_contextwith a 500 before the join or install was dispatched. Lookups are now null-safe, as upstream's?.is, and the tenant comes from_tenant_id_from_activity(upstreamtenantIdFromActivity). New testtest_explicit_null_channel_data_fields_still_dispatch_the_joinsends real JSONnull(the newreceive(..., keep_nulls=True)) and fails without the fix.test_prefers_the_conversation_tenant_over_channel_datapins theconversation.tenantId ?? channelData.tenant.idprecedence.evil{APP_ID}and28x{APP_ID}in the self-check negatives pin the:suffix boundary.:dropped from the suffix) now fails a test._cache_user_contextis awaited, and a failing state write gives a 500 before any activity is dispatched. Upstream fire-and-forgets these writes with.catch(() => {}). This affects every activity type onmainand no sibling issue under [4.41] Tracking: sync upstream chat@4.31.0 → chat@4.41.1 (0.4.41 wave) #184 owns it, so I filed Teams: make _cache_user_context state writes best-effort so a state outage does not drop dispatch #298 for a single fix covering all of them.gpt-6-astra: 3 rounds; the final verdict on HEAD
98da0c4is clean ("No actionable regressions found").JsonWebToken.service_urlstrips the trailing slash, so a root-path token URL (https://smba.infra.gcc.teams.microsoft.com/) failed the SSRF allow-list andchannel_idbecameNone. The root/is now restored before validation. New testtest_restores_the_root_slash_the_token_strips_from_a_root_path_endpointfails without the fix. The non-parity row indocs/UPSTREAM_SYNC.mdis updated.Bots: CodeRabbit approved
69657ebwith no actionable comments. On the final merge commit it was rate-limited; that commit changed onlyCHANGELOG.mdagainstmain. Its walkthrough says the changelog "removes a temporary Slack divergence note"; that was a misread of a blank-line change, and the blank line is now restored, so the changelog diff only adds lines. Gemini left no comments.CI: all green on
98da0c4(Lint & Type Check, test 3.12/3.13, CodeQL). Local full validation is green: ruff, format, audit (0 hard failures),--check-docs, strict fidelity 733/733, pytest 7481 passed, pyrefly 0 errors.Closes #217
Part of #184
Summary by CodeRabbit