fix(adapters): Slack streaming team_id + Teams DM Graph conversation IDs (vercel/chat#330, #403)#85
Conversation
vercel/chat#330) Slack carries the workspace ID in different shapes depending on the webhook envelope. Block_actions / view_submission payloads use a nested ``team.id`` (object) with ``user.team_id`` as a fallback, while message events use the top-level ``team_id`` / ``team`` (string). The previous ``raw.get("team_id") or raw.get("team")`` extraction returned the entire ``team`` dict for block_actions, causing Slack streaming API calls to fail or hit the wrong workspace. Move the extraction into a dedicated helper that walks each shape in order and returns ``None`` when no string ID is found. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
…est (vercel/chat#330) Upstream's chat@4.27.0 added a thread.test.ts case verifying that a block_actions-created thread can stream structured chunks (text + TaskUpdateChunk) through ``adapter.stream`` with the resolved ``recipient_team_id``. Port it to keep test_fidelity at 0 missing for the thread.test.ts mapping. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
…l/chat#403) Bot Framework hands out opaque DM conversation IDs (e.g. ``a:1xWhatever``) which Microsoft Graph's ``/chats/{chat-id}/messages`` endpoint rejects with 404. The canonical Graph chat ID for a 1:1 DM is ``19:{userAadId}_{botId}@unq.gbl.spaces``. Cache the user's AAD object ID from incoming activities and resolve the Graph chat ID before issuing Graph calls. Add a discriminated union ``TeamsGraphContext`` (channel | DM) and dispatch on context type from ``fetch_messages``, ``fetch_channel_messages``, and ``fetch_channel_info``. Group chats (no cached context) keep falling back to the raw conversation ID, which works as-is with Graph. Backwards-compatible with the pre-#403 cache shape: entries without a ``type`` discriminator are treated as ``channel``. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
|
Warning Review limit reached
More reviews will be available in 44 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 (6)
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 |
![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 implements canonical Microsoft Graph chat ID resolution for Teams DMs to prevent 404 errors and improves Slack workspace ID extraction from interactive payloads. The Teams adapter now caches and utilizes a broader Graph context for both channels and direct messages, ensuring the correct IDs are used for Graph API calls. Additionally, a new helper function in thread.py ensures correct Slack team ID parsing across different webhook shapes. Feedback was provided regarding duplicated pagination logic in the Teams adapter, suggesting a refactor to improve maintainability.
| chat_id = self._chat_id_from_context(graph_context, base_conversation_id) | ||
| if direction == "forward": | ||
| params = {"$top": limit, "$orderby": "createdDateTime asc"} | ||
| if options.cursor: | ||
| params["$filter"] = f"createdDateTime gt {options.cursor}" | ||
| graph_messages = await self._graph_list_chat_messages(chat_id, params) | ||
| has_more = len(graph_messages) >= limit | ||
| else: | ||
| params = {"$top": limit, "$orderby": "createdDateTime desc"} | ||
| if options.cursor: | ||
| params["$filter"] = f"createdDateTime lt {options.cursor}" | ||
| graph_messages = await self._graph_list_chat_messages(chat_id, params) | ||
| graph_messages.reverse() | ||
| has_more = len(graph_messages) >= limit |
There was a problem hiding this comment.
The logic for handling forward and backward pagination is duplicated here and in fetch_messages (lines 1092-1110). To improve maintainability and reduce code duplication, you could refactor this block and apply a similar change in fetch_messages.
Here's a suggested refactoring for this block:
| chat_id = self._chat_id_from_context(graph_context, base_conversation_id) | |
| if direction == "forward": | |
| params = {"$top": limit, "$orderby": "createdDateTime asc"} | |
| if options.cursor: | |
| params["$filter"] = f"createdDateTime gt {options.cursor}" | |
| graph_messages = await self._graph_list_chat_messages(chat_id, params) | |
| has_more = len(graph_messages) >= limit | |
| else: | |
| params = {"$top": limit, "$orderby": "createdDateTime desc"} | |
| if options.cursor: | |
| params["$filter"] = f"createdDateTime lt {options.cursor}" | |
| graph_messages = await self._graph_list_chat_messages(chat_id, params) | |
| graph_messages.reverse() | |
| has_more = len(graph_messages) >= limit | |
| chat_id = self._chat_id_from_context(graph_context, base_conversation_id) | |
| order_by = "createdDateTime asc" | |
| filter_op = "gt" | |
| if direction != "forward": | |
| order_by = "createdDateTime desc" | |
| filter_op = "lt" | |
| params = {"$top": limit, "$orderby": order_by} | |
| if options.cursor: | |
| params["$filter"] = f"createdDateTime {filter_op} {options.cursor}" | |
| graph_messages = await self._graph_list_chat_messages(chat_id, params) | |
| has_more = len(graph_messages) >= limit | |
| if direction != "forward": | |
| graph_messages.reverse() |
There was a problem hiding this comment.
Addressed in commit cce5c75 — extracted _paginate_graph_chat_messages(chat_id, limit, direction, cursor) -> (messages, has_more) and both fetch_messages (was L1092-1110) and fetch_channel_messages (was L1213-1225) now share a single code path. Behavior is preserved: gt + asc + no-reverse on forward, lt + desc + reverse on backward, has_more = len >= limit for both empty and exact-page edges. Existing TestGraphDmConversationIdResolution + fetch_messages / fetch_channel_messages tests cover both branches.
Generated by Claude Code
Address gemini-code-assist review on PR #85 (line 1225). The forward/ backward pagination block was duplicated between fetch_messages and fetch_channel_messages; extract a single helper that takes (chat_id, limit, direction, cursor) and returns (messages, has_more). Both call sites now share one code path. Same behavior — reverses on backward, ``gt``/``lt`` filter ops, ``$top`` page size — covered by the existing TestGraphDmConversationIdResolution + fetch_messages / fetch_channel_messages tests (13 pass). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
patrick-chinchill
left a comment
patrick-chinchill
left a comment
There was a problem hiding this comment.
Code review — vercel/chat#330 + #403 ports
Both ports are tight 1:1 transliterations of upstream 8a0c7b3 / 4c24c94. Slack helper walk order matches upstream exactly (team_id → team:str → team.id:object → user.team_id); Teams discriminated-union dispatch matches the TS version including the legacy-cache fallthrough (type absent → treated as channel). The follow-up _paginate_graph_chat_messages extraction is behavior-preserving (forward = gt + asc + no reverse, backward = lt + desc + reverse, has_more = len >= limit for both empty and exact-page edges). The concurrency test genuinely interleaves with asyncio.gather + asyncio.sleep(0) — Hazard #6 is exercised, not just claimed.
🟡 Medium
-
Adversarial gap in
_extract_slack_recipient_team_idtest sweep (src/chat_sdk/thread.pyL144‑183,tests/test_thread_faithful.pyL797‑818). Perdocs/SELF_REVIEW.mdprinciple #1 (adversarial input sweep), the parametrized test only covers the four happy paths. The helper has explicit guards for shapes that aren't exercised:raw.teamis a dict butidis missing or non-string (e.g.{"team": {"domain": "x"}}→ must fall through touser.team_id)team_id/teamis an empty string""(the truthiness guardand team_idis the only thing keeping that from being returned — Hazard #1)rawisNone/ not a dict at alluser.team_idis non-string
Each is a one-line
pytest.mark.parametrizeextension; without them the next refactor of this helper has no signal it broke a fallback.
🔵 Nit
-
aadObjectIdis interpolated into a Graph URL path with no shape validation (src/chat_sdk/adapters/teams/adapter.pyL330). The cachedgraph_chat_idbecomeschat_id→https://graph.microsoft.com/v1.0/chats/{chat_id}/messages. Bot Framework JWT verification authenticates the activity envelope but does not constrain the contents offrom.aadObjectId. Real Azure AD object IDs are GUIDs, but a malicious / non-conforming activity could inject/,?,#,:. The host is hardcoded so this can't be SSRF, and the bearer token is Graph-scoped so worst case is a 404 against a different Graph endpoint — but a one-linere.fullmatch(r"[0-9a-fA-F-]{36}", aad_object_id)guard before formatting would make this robust and matches the same defense-in-depth pattern as_validate_service_urltwo functions up. -
Backwards-compat claim has no end-to-end regression test for legacy cache shape (PR body &
_chat_id_from_contextdocstring claim "cached entries written before #403 are treated as channel").TestGetGraphContext.test_valid_context_from_cacheconfirms the legacy{"team_id": "t1", "channel_id": "c1"}shape loads, but no test asserts it routes throughfetch_messagesto the channel branch (analogous to the newtest_fetch_messages_uses_graph_chat_id_for_dm). One added test covering legacy cache →fetch_channel_thread_messageswould lock the claim. -
Channel-context cache writes a
"type": "channel"field that upstream omits (src/chat_sdk/adapters/teams/adapter.pyL316). Upstream's TS only setstypeon the DM context; channel entries are written without the discriminator. Functionally equivalent (both code paths treattype != "dm"as channel), but it's a wire-shape divergence that future cross-language deployments could trip on. Either match upstream and drop the key, or add a row to the non-parity table.
✅ Looks good
_extract_slack_recipient_team_idwalk order is identical to upstream and the parametrized + structured-stream tests faithfully port the upstreamit.eachblock.- Concurrent
block_actionstest is a real concurrency test (asyncio.gather+await asyncio.sleep(0)interleave point insidemock_stream), not the typical sequential-disguised-as-concurrent. _paginate_graph_chat_messagesrefactor is semantically identical to the duplicated blocks it replaces; both edge cases (len == 0→False,len == limit→True) match upstream and are covered transitively by the existing fetch tests.- The
is_dm-prefix guard for DM caching (not base_channel_id.startswith("19:")) is tested both positively (DM cached) and adversarially (channel-with-aadObjectId not mis-cached as DM). - All 14 new tests pass locally.
Posted by an automated reviewer agent. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code
Three review-driven fixes: 1. Adversarial input sweep on _extract_slack_recipient_team_id (Medium): extend the parametrized test from 4 happy paths to 11 cases, covering the helper's fallthrough guards (team dict missing id, empty team_id/team strings, non-dict raw, non-string user.team_id, team.id non-string). Per docs/SELF_REVIEW.md principle #1. 2. Defense-in-depth GUID guard on aadObjectId before formatting it into the Graph chat ID (Nit). Bot Framework JWT verification authenticates the activity envelope but does not constrain from.aadObjectId; a malformed value containing /, ?, # could otherwise inject into the Graph URL path. New _AAD_OBJECT_ID_PATTERN reject malformed shapes; adversarial test covers 6 attack patterns. 3. End-to-end legacy-cache-shape test for #403 backwards-compat (Nit). Cached entries written before the discriminator landed lack a "type" key. The test asserts _get_graph_context loads them and _chat_id_from_context returns the raw conversation ID (channel semantics), not a misclassified Graph DM URL. 4. Wire-shape parity (Nit): the channel-context cache now omits the "type": "channel" key to match upstream TS, which only sets the discriminator on the DM branch. Functionally equivalent (absent type is treated as channel by the dispatcher), but the cross-language wire shape is now identical. All TestGraphDmConversationIdResolution tests pass (10 cases). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
patrick-chinchill
left a comment
patrick-chinchill
left a comment
There was a problem hiding this comment.
Re-review of fixes (round 2)
Walked the four claimed fixes against f55378a (chat@4.27.0) upstream commits 8a0c7b3 (#330) and 4c24c94 (#403), then hunted for residual gaps. 299 tests pass in test_teams_coverage.py + test_thread_faithful.py + test_coverage_gaps.py.
Verified fixes
- Adversarial parametrize (
test_should_pass_stream_options_from_current_message_context) — 11 cases now: 4 happy paths + 4 fall-through cascades + 3 None-returning rejections (non-dict, empty dict, non-stringteam.id). Order matches the helper'steam_id→team(str) →team.id(obj) →user.team_idcascade. Solid. - AAD GUID guard (
_AAD_OBJECT_ID_PATTERN.fullmatchat adapter.py:348) —fullmatchconfirmed to reject path-injection (.../messages), missing hyphens, non-hex chars, short strings. Test sweeps 6 attack patterns including../etc/passwd. Defense-in-depth is real. - Legacy cache E2E (
test_fetch_messages_uses_legacy_cache_shape_for_channel) — asserts that pre-#403 entries (notypekey) load via_get_graph_contextand_chat_id_from_contextreturns the raw conversation ID rather than a misclassified DM URL. Good backwards-compat coverage. - Wire-shape parity — channel cache write at adapter.py:322 omits
"type": "channel", exactly matching upstream TS index.ts:220. Cross-language wire shape is now identical.
Hunt for residual upstream parity gaps
- Race conditions / ContextVar:
recipient_team_idflows through per-threadself._current_message, no module-global caches. Concurrency test (test_concurrent_block_actions_team_ids_do_not_cross_contaminate) verifies two interleavedblock_actionsposts don't cross-pollute. Clean. - StreamOptions wire-shape: walked upstream
chat/src/types.ts— onlyrecipientUserId+recipientTeamIdare derived fromcurrentMessage. No new fields in #330. No gap. - Other Graph endpoints consuming
chat_id: TS upstream applieschatIdFromContextin 3 sites —fetchMessages,fetchChannelMessages, ANDlistThreads. Python ports the first two correctly.list_threadsis not implemented in the Python Teams adapter at all (pre-existing gap onmain, not introduced here, but worth tracking — when ported, must thread_chat_id_from_contextthrough).fetch_channel_infois correctly DM-aware (channel branch only fires whentype != "dm"). graph-api.test.tschatIdFromContextdescribe block: all 3 upstreamit()cases (DM context →graphChatId, no context → raw ID, channel context → raw ID) are mirrored astest_chat_id_from_context_*inTestGraphDmConversationIdResolution. Full fidelity.- TS DM-context simplification commit 2 ("DMs never have threadMessageId so the channel guard doesn't need an explicit DM exclusion"): Python's
if graph_context and context_type != "dm" and thread_message_id:keeps the explicit DM exclusion. Functionally equivalent (more defensive than upstream); no parity bug.
Nits (non-blocking)
TeamsChannelContext(TypedDict, total=False)makesteam_idandchannel_idoptional at the type level. Upstream TS only markstypeoptional. Considerclass TeamsChannelContext(TypedDict): team_id: str; channel_id: str; type: NotRequired[str]for stricter typing. Runtime is unaffected.self._app_idis interpolated into the Graph chat ID without GUID validation. It's config-sourced (TEAMS_APP_IDenv), so not user input — but a misconfigured/-containing value would silently produce a malformed chat ID. Optional symmetric guard.
Re-review verdict: PASS
All 4 review-driven fixes land cleanly. Wire-shape, behavior, and test fidelity match upstream. The two nits above are advisory-only.
Posted by an automated re-reviewer agent. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code
Final upstream-coverage audit before merging the 7 sync PRs (#84-#90) identified one undocumented N/A item: vercel/chat#415 (Teams SDK 2.0.8 + User-Agent) is a JS-only botbuilder dependency bump. The Python Teams adapter uses raw aiohttp (no botbuilder), so there is no equivalent dependency to bump. The optional User-Agent: Vercel.ChatSDK header on the ~9 outbound aiohttp call sites is a defense-in-depth nice-to-have; deferred as a follow-up rather than landed in this sync. Updates: - CHANGELOG.md: tick all completed items and link them to their PRs (#84, #85, #86, #87, #88, #89, #90, plus already-merged PR #74). Document #415 inline as N/A. - docs/UPSTREAM_SYNC.md non-parity table: add row for Teams User-Agent header divergence so future syncers don't try to "port" the JS bump. Item #6 (concurrency.maxConcurrent) is already implementation-covered in the Python port (existing divergence row at L492). The 4 new TS concurrency tests in chat.test.ts have Python-specific equivalents at test_chat_faithful.py L2969-3055 that don't name-match — leaving as deferred fidelity-baseline polish since the behavior is verified. Verdict from the coverage audit: all 18 substantive ports across PRs #84-#90 are upstream-verified. No commits in chat@4.26.0..f55378a were missed. Ready to start merging. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Pyrefly does not narrow the TeamsGraphContext union via
context.get("type") == "dm", so the subsequent access of
context["graph_chat_id"] fails the pyrefly check. Add an explicit
cast inside the narrowed branch to resolve the CI type-check error.
https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
![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: 3947238759
ℹ️ 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".
| and self._app_id | ||
| and not base_channel_id.startswith("19:") | ||
| and state | ||
| and _AAD_OBJECT_ID_PATTERN.fullmatch(aad_object_id) |
There was a problem hiding this comment.
The new DM-context branch calls _AAD_OBJECT_ID_PATTERN.fullmatch(aad_object_id) directly, but aadObjectId comes from inbound activity JSON and can be non-string (int, object, etc.). In that case fullmatch raises TypeError, which propagates out of handle_webhook (there is no surrounding catch) and turns an otherwise ignorable malformed field into a webhook failure. This breaks message handling for requests with unexpected from.aadObjectId shapes; add a string type check before evaluating the regex.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f99de08.
re.fullmatch raises TypeError on non-string input, which would have escaped _cache_user_context → handle_webhook and turned a malformed from.aadObjectId (int/dict/list/etc.) into a 500. Added an isinstance(aad_object_id, str) gate before the regex so non-conforming activities are silently skipped (no DM caching), matching the existing defense-in-depth contract.
Regression test added in tests/test_teams_coverage.py::test_cache_user_context_handles_non_string_aad_object_id covering int, dict, list, None, and bool shapes. Verified the test raises TypeError: expected string or bytes-like object, got 'int' against the pre-fix code and passes after.
Validation: pyrefly 0 errors, ruff clean, full suite 3691 passed (only the pre-existing unrelated test_github_webhook::test_throws_when_no_auth failure remains).
Generated by Claude Code
A malformed activity could supply a non-string ``from.aadObjectId`` (int, dict, list, etc.); ``re.fullmatch`` would then raise ``TypeError`` and escape ``handle_webhook``, turning an ignorable malformed field into a 500. Add an ``isinstance(..., str)`` gate before the regex so malformed activities are silently skipped (no DM caching), as intended. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
|
@codex review Re-requesting on the latest state (commit Fresh focus: the Generated by Claude Code |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Summary
Two small upstream bug-fix ports bundled into one PR. Both touch adapter dispatch code and have new regression tests that fail before the fix.
fix(slack): interactive-payload team_id through streaming context — vercel/chat#330
Slack carries the workspace ID in different shapes depending on the webhook envelope:
message,app_mention): top-levelteam_id/team(string).block_actions,view_submission, …): nestedteam.id(object), withuser.team_idas a final fallback.The old extraction (
raw.get("team_id") or raw.get("team")) returned the entireteamdict forblock_actions, which then traveled to the Slack adapter asrecipient_team_idand either crashed Slack streaming API calls or routed them to the wrong workspace.Moved the extraction into a dedicated
_extract_slack_recipient_team_idhelper insrc/chat_sdk/thread.pythat walks each shape in order and returnsNonewhen no string ID is found.fix(teams): canonical DM conversation ID for Microsoft Graph API — vercel/chat#403
Bot Framework hands out opaque DM conversation IDs (e.g.
a:1xWhatever) which Graph's/chats/{chat-id}/messagesendpoint rejects with 404. The canonical Graph chat ID for a 1:1 DM is19:{userAadId}_{botId}@unq.gbl.spaces.from.aadObjectId) into a newTeamsDmContextkeyed by base conversation ID.TeamsGraphContext = TeamsChannelContext | TeamsDmContext._get_channel_context→_get_graph_contextand add_chat_id_from_context()dispatch helper.fetch_messages,fetch_channel_messages, andfetch_channel_infoto dispatch on context type.typediscriminator and are treated aschannel.Tests
tests/test_thread_faithful.py:test_should_pass_stream_options_from_current_message_contextover all four Slack payload shapes (team_id,teamstring,team.idobject,user.team_idfallback).test_concurrent_block_actions_team_ids_do_not_cross_contaminatecovering hazard chore: bump to 0.0.1a3 #6 (no team_id leak across concurrent requests).test_should_forward_structured_stream_chunks_to_adapter_stream_from_an_action_created_thread(port of upstream's #330 test; restores 0-missing onthread.test.tsfidelity).tests/test_teams_coverage.py: newTestGraphDmConversationIdResolutionclass with 8 cases covering_chat_id_from_context(DM / channel / no-context branches),_cache_user_context(DM cached / channel skipped / no-aad skipped / DM-like channel adversarial), and end-to-endfetch_messages(DM resolves to19:{aadId}_{botId}@unq.gbl.spaces, group chat falls back to raw ID).Adversarial checks ran per
docs/SELF_REVIEW.md: pass-interaction (concurrent team_ids) and the "DM-like channel" misclassification both have explicit tests.Test plan
uv run ruff check src/ tests/ scripts/— cleanuv run ruff format --check src/ tests/ scripts/— cleanuv run python scripts/audit_test_quality.py— 0 hard failures (39 pre-existing warnings unchanged)TS_ROOT=/tmp/vercel-chat uv run python scripts/verify_test_fidelity.py—thread.test.tsnow reports 0 missinguv run pytest tests/ --tb=short -q—3681 passed, 2 skipped, 1 failedtests/test_github_webhook.py::TestGitHubAdapterConstructor::test_throws_when_no_auth(called out as ignorable in the task brief)Upstream refs
[chat] fix Slack streaming team ID for interactive payloads(commit8a0c7b3)fix(adapter-teams): resolve DM conversation IDs for Graph API(commit4c24c94)https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code