feat(teams): migrate outbound send/edit/delete/typing to the SDK (#93 PR 2/4)#144
Conversation
…PR 2/4) Route the outbound public Adapter methods through the Microsoft Teams SDK ``App``, mirroring upstream adapter-teams@chat@4.30.0, and untangle the Bot Framework / Graph token caches. Outbound delegation: - post_message / start_typing -> self._app.send(conversation_id, activity) - edit_message -> self._app.api.conversations.activities(id).update(msg_id, activity) - delete_message -> self._app.api.conversations.activities(id).delete(msg_id) The SDK splits input from output activity models, so we build MessageActivityInput / TypingActivityInput (the ActivityParams union that app.send / activities.update accept) from the existing camelCase wire dict. The dict is still returned as RawMessage.raw, preserving the public contract (attachment shape, file delivery, returned SentMessage id). Per-thread routing: thread IDs encode a service URL, but the SDK binds the App's ApiClient to one URL at construction. _point_app_api_at retargets the real ApiClient's service-url chain (validated against the SSRF allow-list first) before each call, so different regions / sovereign clouds reach the right endpoint. Errors now pass the raw SDK exception to handleTeamsError, which already understands the SDK exception shape (401 -> AuthenticationError, 429 -> AdapterRateLimitError, etc.). Token-cache untangle: _get_access_token (Bot Framework) and _get_graph_token (Graph) previously shared _access_token / _token_expiry, so the last writer clobbered the other scope's token (a Graph read could ship a Bot Framework token and vice versa). Graph now caches on dedicated _graph_token / _graph_token_expiry fields. _get_access_token + its fields are retained for the still-hand-rolled Bot Framework callers (native streaming via _teams_send in PR 3, and open_dm), now isolated by scope. Streaming is untouched: _teams_send (and its _get_access_token token) remain for the streaming envelope + server-assigned streamId, which app.send does not surface. Removed the now-unused _teams_update / _teams_delete aiohttp helpers. Tests: outbound tests assert the SDK app.send / app.api.conversations.activities are called (not aiohttp); added a token-cache isolation regression proving the two scopes no longer collide, and a per-thread service-url routing regression exercising the real ApiClient chain.
|
Warning Review limit reached
More reviews will be available in 22 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| def routed_post(url, **kwargs): | ||
| scope = (kwargs.get("data") or {}).get("scope", "") | ||
| if "graph.microsoft.com" in scope: |
| scope = (kwargs.get("data") or {}).get("scope", "") | ||
| if "graph.microsoft.com" in scope: | ||
| return session._make_cm(graph_resp) | ||
| if "botframework.com" in scope: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates outbound operations (posting, editing, deleting, and typing) in the Teams adapter to use the official Microsoft Teams SDK instead of hand-rolled REST API helpers. It also resolves a token cache collision issue (#93) by introducing dedicated fields for the Microsoft Graph token. However, a critical race condition was identified in the new _point_app_api_at method: mutating the shared API client's service URL concurrently across asynchronous calls can cause requests to be misrouted. It is recommended to serialize these outbound operations using an asyncio.Lock.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def _point_app_api_at(self, service_url: str) -> None: | ||
| """Aim the SDK ``App``'s Bot Framework client at ``service_url``. | ||
|
|
||
| The migrated outbound paths call ``self._app.send(...)`` and | ||
| ``self._app.api.conversations.activities(...)`` directly (parity with | ||
| upstream ``this.app.send`` / ``this.app.api.conversations``). The SDK | ||
| binds the App's :class:`ApiClient` to a single service URL at | ||
| construction, and ``app.send`` reads ``self.api.service_url`` into the | ||
| outgoing :class:`ConversationReference`. Our thread IDs encode a | ||
| per-thread service URL, so before each call we retarget the App's API | ||
| client — validating against the SSRF allow-list first, exactly as the | ||
| retired hand-rolled senders did. | ||
|
|
||
| The setter walks the real :class:`ApiClient`'s service-url chain | ||
| (the client itself, its ``conversations`` sub-client, and that | ||
| sub-client's ``activities_client``). It is defensive about test doubles | ||
| that replace ``self._app.api`` with a mock lacking those attributes — | ||
| an ``AttributeError`` there is harmless because the mock ignores the | ||
| service URL anyway. | ||
| """ | ||
| _validate_service_url(service_url) | ||
| normalized = service_url.rstrip("/") | ||
| api = self._app.api | ||
| try: | ||
| api.service_url = normalized | ||
| conversations = api.conversations | ||
| conversations.service_url = normalized | ||
| conversations.activities_client.service_url = normalized | ||
| except AttributeError: | ||
| # ``self._app.api`` is a test double without the real client's | ||
| # service-url chain; nothing to retarget. | ||
| pass |
There was a problem hiding this comment.
Critical Race Condition in Concurrent Requests
Since TeamsAdapter is a shared instance (often a singleton) handling concurrent requests, mutating the shared self._app.api service URL in _point_app_api_at introduces a critical race condition.
When multiple concurrent outbound operations (e.g., post_message, edit_message, delete_message, start_typing) are in-flight for different threads with different regional or sovereign service URLs (such as https://smba.trafficmanager.net/amer/ vs https://smba.trafficmanager.net/emea/), they will concurrently overwrite self._app.api.service_url. Because these outbound operations are asynchronous (await self._app.send(...)), one request can yield control and have its service URL overwritten by another concurrent request before or during its API call. This will cause requests to be routed to the wrong regional endpoints, resulting in 404 Not Found or 401 Unauthorized errors.
Suggested Fix
To prevent this, serialize outbound operations that modify the shared API client's service URL using an asyncio.Lock. For example, you can define an async context manager to safely acquire a lock, point the API, and release the lock after the request completes:
# In __init__:
# self._api_lock = asyncio.Lock()
@asynccontextmanager
async def _api_at(self, service_url: str):
async with self._api_lock:
self._point_app_api_at(service_url)
yieldAnd then wrap the outbound calls:
async with self._api_at(decoded.service_url):
sent = await self._app.send(...)![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| async def test_delete_message_failure(self): | ||
| adapter = _make_adapter(logger=_make_logger()) | ||
| adapter._teams_delete = AsyncMock(side_effect=Exception("delete failed")) | ||
| _update, _delete = _mock_app_activities(adapter, delete_side_effect=Exception("delete failed")) |
| async def test_delete_message_failure(self): | ||
| adapter = _make_adapter(logger=_make_logger()) | ||
| adapter._teams_delete = AsyncMock(side_effect=Exception("delete failed")) | ||
| _update, _delete = _mock_app_activities(adapter, delete_side_effect=Exception("delete failed")) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54dc77eded
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if hasattr(error, "status"): | ||
| error_dict["statusCode"] = error.status | ||
| _handle_teams_error(error_dict, "postMessage") | ||
| _handle_teams_error(error, "postMessage") |
There was a problem hiding this comment.
When the SDK call fails, this now passes the raw exception into _handle_teams_error, but the Microsoft Teams SDK HTTP client raises httpx.HTTPStatusError via response.raise_for_status(), where the status is on error.response.status_code rather than direct status_code/status/code attributes. In real 401/403/429 responses from app.send (and the same pattern in edit/delete), the mapper therefore falls through to a generic NetworkError instead of the existing AuthenticationError, permission, or rate-limit errors, so callers lose the actionable failure type.
Useful? React with 👍 / 👎.
2 participants
Summary
PR 2 of the Teams adapter migration to the official
microsoft-teams-appsSDK (issue #93). PR 1 (inbound + auth via the SDKApp) is merged; this PR migrates the outbound operations, mirroring upstreamadapter-teams@chat@4.30.0(index.ts). Streaming and Graph reads stay hand-rolled (PR 3 / separate).What changed
Outbound ops now delegate to the SDK
post_message,start_typingself._app.send(conversation_id, activity)(returns aSentActivitywith.id)edit_messageself._app.api.conversations.activities(conversation_id).update(message_id, activity)delete_messageself._app.api.conversations.activities(conversation_id).delete(message_id)The SDK splits input from output activity models, so we build
MessageActivityInput/TypingActivityInput(theActivityParamsunion thatapp.send/activities.updateaccept) from the existing camelCase wire dict. That dict is still returned asRawMessage.raw, so the public Adapter contract is preserved exactly (text/markdown format, file/attachment shape, returnedSentMessageid).Per-thread service-URL routing
Thread IDs encode a per-thread service URL, but the SDK binds the App's
ApiClientto a single URL at construction (andapp.sendreadsself.api.service_urlinto the outgoingConversationReference)._point_app_api_atretargets the realApiClient's service-url chain — validated against the SSRF allow-list first, exactly as the retired hand-rolled senders did — before each call, so different regions / sovereign clouds reach the right endpoint. Errors pass the raw SDK exception tohandleTeamsError, which already maps the SDK exception shape (401 →AuthenticationError, 429 →AdapterRateLimitError, …).Token-cache untangle (the shared-token bug)
_get_access_token(Bot Framework, scopeapi.botframework.com) and_get_graph_token(Graph, scopegraph.microsoft.com) previously shared_access_token/_token_expiry, so the last writer clobbered the other scope — a Graph read could ship a Bot Framework token and vice versa. Graph now caches on dedicated_graph_token/_graph_token_expiryfields._get_access_token+ its fields are retained for the still-hand-rolled Bot Framework callers (native streaming via_teams_sendin PR 3, andopen_dm), now isolated by scope.Streaming kept intact
_teams_send(and the Bot Framework token it uses) remain for the streaming envelope (channelData/entities/ server-assignedstreamId), whichapp.senddoes not surface. Removed the now-unused_teams_update/_teams_deleteaiohttp helpers.Tests
app.send/app.api.conversations.activitiesare called (not aiohttp), via small AsyncMock helpers.TestTokenCacheIsolation): a scope-routed token endpoint proves the two scopes no longer collide, cache on separate fields, and a warm Bot Framework cache does not satisfy a Graph fetch.TestOutboundServiceUrlRouting): exercises the realApiClientchain (only the leafupdate/sendpatched) so the retarget can't silently no-op.SDK-forced divergence
MessageActivity/TypingActivity; the Python SDK only accepts the input variants (MessageActivityInput/TypingActivityInput) onsend/update. Behavior is identical on the wire.app.sendrequiresapp.initialize()to have run (the SDK raises otherwise). This matches the adapter lifecycle (initialize()always precedes outbound ops) and upstream.Gauntlet
ruff checkOK ·ruff format --checkOK ·audit_test_quality0 hard failures ·pyrefly check0 errors ·pytest4797 passed, 3 skipped.Not touched (out of scope)
Streaming (
stream(),_TeamsStreamSession,_emit_streaming_activity), Graph reads (fetch_*/get_user— only their token-cache field changed),cards.py,format_converter.py, thread-id, the inbound path (PR 1), and the publicTeamsAdapterConfigfields.