release: 0.4.29 completion train — stranded-PR recovery + remaining 4.29 ports + cut#134
Merged
Merged
Conversation
#124) * feat(messenger): adapter — webhook, Graph API, send/stream (vercel/chat#461) Port of `packages/adapter-messenger/src/index.ts` (PR 2 of 2). Builds on the scaffolding (types/format converter/cards) added in PR #118. Includes: - `MessengerAdapter`: webhook routing (GET verification + POST events), X-Hub-Signature-256 HMAC-SHA256 verification, Graph API client backed by aiohttp, send paths (text / Generic template / Button template / text fallback), buffered streaming, postback / reaction / echo / delivery / read handling, attachment extraction with lazy download, message cache (Messenger has no history API), user-profile cache, thread / channel helpers, typed Graph-API error mapping. - `create_messenger_adapter` factory with FACEBOOK_* env fallbacks. - Exports wired through `chat_sdk.adapters.messenger`. Q1 (init-failure behavior, see #110): match `WhatsAppAdapter` — raise `ValidationError` from both the factory and the constructor when any required credential is missing, so config errors surface loudly at startup. Q3 (signature verification, see #110): pin upstream's X-Hub-Signature-256 + App Secret HMAC contract. A swappable verifier (Slack-style) would diverge from Meta's protocol with no offsetting benefit for a single-secret integration; flagged as a possible future divergence but not introduced here. Tests: `tests/test_messenger_webhook.py` (57) and `tests/test_messenger_api.py` (64) — mirrors the Telegram/WhatsApp file split. Covers signature valid/invalid/missing/wrong-algo/replay, webhook routing for all event types, postback decoding (raw + chat: prefix), card → template paths, stream buffering, truncation, error mapping, and the Q1 constructor failure path. 121 new tests, full suite: 4220 passed, 3 skipped. * fix(messenger): verify HMAC on raw bytes + URL/profile defenses (gemini review) Four follow-ups to PR #124 from Gemini's review of the Messenger adapter: - `_handle_verification` casts `request.url` to `str` before `urlparse`. Starlette/yarl `URL` objects are non-`str` and would raise. - `_get_request_body` now returns `bytes` instead of `str`. Decoding to UTF-8 and re-encoding for the HMAC step risks replacement characters (U+FFFD) for any non-UTF-8 byte sequence — that breaks signature parity with Meta's reference implementation and silently rejects legitimate webhooks. Body sources are also reordered: `body` attribute first (canonical raw bytes on Starlette/Django), `text` attribute fallback (aiohttp's str path). `is not None` everywhere so a legitimately empty `b""` body isn't skipped. - `_verify_signature(body: bytes, ...)` matches the new contract; the `body.encode("utf-8")` step is dropped — HMAC operates on the exact wire bytes Meta signed. JSON parsing at the call site still works (`json.loads` accepts both str and bytes). - `_fetch_user_profile` now `isinstance(profile, dict)` checks the Graph API response before caching. A non-dict response (None, list, unexpected shape) used to poison `_user_profile_cache` and raise `AttributeError` on the next `.get` call in `_profile_display_name`; we now fall back to the minimal `{"id": user_id}` profile and leave the cache untouched. Regression coverage (+7 tests): - `TestVerifySignature::test_verifies_raw_bytes_without_encoding_roundtrip` feeds a body with lone continuation bytes (0x80 0xff) that don't survive UTF-8 round-trip. Fails on the old decode+re-encode path. - `TestGetRequestBody` (new class, 5 cases) pins the bytes return type across all four framework-shaped inputs: bytes body attribute, str body attribute, async-callable body (Starlette/FastAPI), async-callable text (aiohttp), and missing-body → `b""`. - `test_non_dict_profile_response_falls_back` exercises both `None` and list responses, asserting the fallback display name and that the cache stays empty. Existing `TestVerifySignature` cases updated to pass `bytes` bodies, matching the new contract. `_sign` test helper accepts `bytes | str` for convenience. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj * fix(messenger): accept uppercase-hex signatures + clarify init-failure docstring (review) https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj * fix(messenger): implement rehydrate_attachment for queue/debounce safety (codex) Codex P2: Messenger attachments processed under queue/debounce/burst concurrency lose their fetch_data closure during JSON serialization in the state backend. Without a rehydrate_attachment hook, dequeued handlers see attachment.fetch_data is None and cannot download the file. Mirrors WhatsAppAdapter.rehydrate_attachment (same Meta family, same queue-mode failure shape): - _extract_attachments now persists the download URL on attachment.fetch_metadata={"url": url} - New rehydrate_attachment reads that URL and rebuilds the lazy downloader via _make_attachment_downloader, reusing the shared aiohttp session. Returns the attachment unchanged when metadata is missing/incomplete (degraded mode, matches the documented "leave unchanged when no hook" behavior). No auth headers are attached by the rebuilt closure — Messenger payload URLs are signature-gated by Meta and the original _download_attachment already operated without Bearer tokens. Upstream parity: vercel/chat's TS adapter-messenger does not implement this hook because queue mode is Python-only. Tests (4 new, load-bearing): - fetch_metadata carries the URL after extraction - queue/serialize roundtrip + rehydrate restores a working downloader that hits the original URL (would fail without the hook — verified by temporary revert) - two degraded-mode pins (no metadata, metadata without url key) * fix(messenger): use is-not-None for limit/user_name/cache checks + regression tests (audit) Targeted audit follow-up — three real truthiness bugs in src/chat_sdk/adapters/messenger/adapter.py, same Port Rule #1 root pattern: 1. _paginate_messages: ``options.limit or 50`` silently swallowed an explicit ``limit=0`` and substituted 50. Switched to ``is not None``. Regression: tests/test_messenger_api.py::test_explicit_zero_limit_is_not_swallowed_to_default. 2. __init__: ``config.user_name or "bot"`` paired with ``bool(config.user_name)`` silently replaced an explicit ``user_name=""`` with the ``"bot"`` fallback AND left ``_has_explicit_user_name`` False, so ``initialize()`` would then overwrite it from ``chat.get_user_name()`` / ``/me``. Switched both sites to ``is not None`` to match upstream's ``hasExplicitUserName`` semantics. Regression: tests/test_messenger_webhook.py::test_explicit_empty_user_name_is_respected. 3. _fetch_user_profile: ``if cached:`` treated a cached ``{}`` (empty dict, falsy) as a miss and re-fetched every call. Switched to ``if cached is not None``. Theoretical (Graph API rarely returns ``{}``) but cheap to make robust. Regression: tests/test_messenger_api.py::test_empty_dict_cache_entry_is_a_hit_not_a_miss. All three regression tests are load-bearing: each asserts an outcome the old code would have produced differently. Other Meta-family adapters (Teams, Discord, Google Chat) already use the ``is not None`` pattern for the same code site. Validation: ruff check / format clean, audit_test_quality.py 0 hard failures, 4235 passed / 3 skipped (was 4232 + 3 new tests = 4235). * fix(messenger): thread echo events by recipient PSID in parse_message (codex review) For a Messenger echo event (a bot-sent message echoed back), sender.id is the Page ID and recipient.id is the user's PSID — the reverse of a normal inbound message. parse_message previously keyed the thread ID off sender.id unconditionally, so a replayed echo was threaded under messenger:<page id> instead of messenger:<user PSID>, causing fetch_messages('messenger:<PSID>') to miss the bot's own echoed message. Branch on message.is_echo so the thread ID keys off recipient.id for echoes (matching _handle_echo) and sender.id otherwise. Non-echo behavior unchanged. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj * build(messenger): add messenger extra declaring aiohttp (codex review) The Messenger adapter lazy-imports aiohttp on its runtime paths (initialize, send, profile lookup, attachment downloads), but the package ships `dependencies = []` and had no `messenger` optional-dependencies extra. A base install enabling only Messenger would raise `ModuleNotFoundError: aiohttp` the moment the adapter talks to Meta. Add `messenger = ["aiohttp>=3.9"]`, matching the sibling aiohttp adapters (telegram/whatsapp/teams/linear). The `all` extra already includes aiohttp>=3.9, so no change there. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --------- Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit 3ad373a) https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64
* feat(ai): add create_chat_tools tool factory (PR 2 of 3) (vercel/chat#492) Port of `packages/chat/src/ai/tools.ts` (plus the supporting `tools/{channels,messages,reactions,threads,users}.ts` and `types.ts`) introduced by vercel/chat#492. Builds on top of #116, which moved `chat_sdk.ai` from a single module to a package so `tools.py` has a home. `create_chat_tools(chat, preset=, require_approval=, overrides=)` returns a mapping of tool-name -> `ChatTool` dataclass holding a description, JSON-Schema-shaped `input_schema`, an async `execute` callable, and the `needs_approval` flag. Three presets (`reader` / `messenger` / `moderator`) and per-tool overrides match the upstream API surface verbatim. Individual factories (`post_message`, `add_reaction`, ...) are also exported so callers can cherry-pick. PR 3 will wire these tools into the existing handler paths; this PR adds the surface only. Validation: uv run ruff check src/ tests/ -> clean uv run ruff format --check src/ tests/ -> clean uv run python scripts/audit_test_quality.py -> 0 hard failures uv run pytest tests/ -q -> 4081 passed, 3 skipped Refs #98, #109. * fix(ai/tools): wrap ChatNotImplementedError + tighten preset check (gemini review) Four small follow-ups to PR 2 of the chat-ai port from Gemini's review: - Import `ChatNotImplementedError` alongside `ChatError`. - Wrap `fetch_channel_messages` invocation in `try/except ChatNotImplementedError` and re-raise as `ChatError` (preserves cause). The early `getattr(adapter, "fetch_channel_messages", None) is None` branch only catches missing attributes; `BaseAdapter` exposes the attribute as a stub that itself raises `ChatNotImplementedError`, so without this wrap the tool surfaces a different exception type than callers expect. Matches the `Chat.get_user` pattern. - Same wrap for `list_threads`. - `_resolve_preset_tools` now distinguishes a single `str` preset from any iterable of presets via `isinstance(preset, str)` rather than `isinstance(preset, list)`, so tuples/sets/Sequences work correctly. Two new regression tests (`test_fetch_channel_messages_wraps_not_implemented`, `test_list_threads_wraps_not_implemented`) drive an `AsyncMock` whose `side_effect` is `ChatNotImplementedError`, assert the surfaced exception is `ChatError` with the expected message, and pin the cause chain (`__cause__ is ChatNotImplementedError`). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj * fix(ai/tools): isolate per-tool input schemas to prevent mutation bleed (review) * fix(ai/tools): pass list_threads options as keyword (codex P2) `MockAdapter.list_threads` is declared as `list_threads(self, channel_id, **kwargs)`, so the tool's positional `list_method(channel_id, ListThreadsOptions(...))` raised `TypeError` for any consumer wiring `create_chat_tools` into MockAdapter (the SDK's own mock). Production adapters accept the kwarg form just as readily as the positional form, so the change is universally safe. The existing `test_list_threads_projects_summaries` used `AsyncMock(return_value=...)` which masked the TypeError by replacing the real `MockAdapter.list_threads` entirely. New `test_list_threads_uses_keyword_options` exercises the real MockAdapter and asserts the default empty result — fails on the old positional call with `TypeError`, passes after. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj * style(ai/tools): apply is-not-None idiom + hoist core import (audit nits) --------- Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit bf66edb) https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64
📝 WalkthroughWalkthroughAdds a Messenger (Meta) adapter with webhook HMAC verification and Graph API send support; implements an AI tools layer exposing chat operations as ChatTool factories with approval/preset controls; introduces persistent ThreadHistoryCache and TranscriptsApiImpl; adds callback_url encoding/resolve/posting; updates Slack/Discord/other adapters and format converters; includes broad test coverage, docs, and a release/version bump to 0.4.29. ChangesCore features and integrations
✨ Finishing Touches📝 Generate docstrings
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the Meta Messenger adapter for the Chat SDK, implementing webhook routing, signature verification, and message/card sending via the Meta Graph API. It also adds a framework-agnostic tool factory to expose Chat SDK operations to AI agents, along with comprehensive test suites. The feedback highlights a potential RuntimeError in _get_request_body when handling aiohttp requests, a possible AttributeError in _project_message when message.metadata is None, and an opportunity to cache fallback user profiles to prevent redundant Graph API calls on failure.
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.
Comment on lines
+1095
to
+1120
| async def _get_request_body(request: Any) -> bytes: | ||
| """Extract the raw request body as bytes (sync or async). | ||
|
|
||
| Returned as raw bytes so signature verification operates on the exact | ||
| wire bytes Meta signed. Any encode/decode round-trip risks replacement | ||
| characters or altered code points and breaks HMAC parity. | ||
| """ | ||
| body = getattr(request, "body", None) | ||
| if body is not None: | ||
| if callable(body): | ||
| result = body() | ||
| body = await result if inspect.isawaitable(result) else result | ||
| if isinstance(body, (bytes, bytearray)): | ||
| return bytes(body) | ||
| if isinstance(body, str): | ||
| return body.encode("utf-8") | ||
| text_attr = getattr(request, "text", None) | ||
| if text_attr is not None: | ||
| if callable(text_attr): | ||
| result = text_attr() | ||
| text_attr = await result if inspect.isawaitable(result) else result | ||
| if isinstance(text_attr, (bytes, bytearray)): | ||
| return bytes(text_attr) | ||
| if isinstance(text_attr, str): | ||
| return text_attr.encode("utf-8") | ||
| return b"" |
Contributor
There was a problem hiding this comment.
In frameworks like aiohttp, accessing request.body directly as a property before calling await request.read() raises a RuntimeError: Read first. Since getattr(request, "body", None) does not catch RuntimeError, this can crash the webhook handler. Additionally, falling back to request.text performs an encode/decode round-trip which can alter the raw wire bytes and break HMAC signature verification.
To prevent this, we should try calling request.read() first (which is standard in aiohttp to get raw bytes), and wrap the attribute accesses in try...except blocks to ensure robustness. Since this involves handling exceptions for asynchronous operations, ensure that the await statement is placed inside the try block so that exceptions are caught during execution, and always accompany the fix with a regression test that verifies the exception is correctly logged and propagated.
@staticmethod
async def _get_request_body(request: Any) -> bytes:
"""Extract the raw request body as bytes (sync or async).
Returned as raw bytes so signature verification operates on the exact
wire bytes Meta signed. Any encode/decode round-trip risks replacement
characters or altered code points and breaks HMAC parity.
"""
# Try reading via a callable 'read' method first (e.g. aiohttp) to get raw bytes directly
read_attr = getattr(request, "read", None)
if read_attr is not None and callable(read_attr):
try:
result = read_attr()
body_bytes = await result if inspect.isawaitable(result) else result
if isinstance(body_bytes, (bytes, bytearray)):
return bytes(body_bytes)
except Exception:
pass
# Fallback to 'body' attribute, guarding against properties that raise exceptions (e.g. aiohttp's BaseRequest.body)
try:
body = getattr(request, "body", None)
if body is not None:
if callable(body):
result = body()
body = await result if inspect.isawaitable(result) else result
if isinstance(body, (bytes, bytearray)):
return bytes(body)
if isinstance(body, str):
return body.encode("utf-8")
except Exception:
pass
# Fallback to 'text' attribute/method
try:
text_attr = getattr(request, "text", None)
if text_attr is not None:
if callable(text_attr):
result = text_attr()
text_attr = await result if inspect.isawaitable(result) else result
if isinstance(text_attr, (bytes, bytearray)):
return bytes(text_attr)
if isinstance(text_attr, str):
return text_attr.encode("utf-8")
except Exception:
pass
return b""References
- When handling exceptions for asynchronous operations, ensure that the
awaitstatement is placed inside thetryblock so that exceptions are caught during execution, and always accompany the fix with a regression test that verifies the exception is correctly logged and propagated.
Comment on lines
+258
to
+259
| "dateSent": (message.metadata.date_sent.isoformat() if message.metadata.date_sent else None), | ||
| "edited": message.metadata.edited, |
Contributor
There was a problem hiding this comment.
If message.metadata is None, accessing message.metadata.date_sent and message.metadata.edited directly will raise an AttributeError. Since message.metadata is optional, we should add appropriate is not None checks before accessing its properties to ensure defensive programming and prevent runtime crashes. Avoid using truthiness checks (like if message.metadata) to check for optional values that can be falsy but valid, to ensure we do not silently ignore them.
Suggested change
| "dateSent": (message.metadata.date_sent.isoformat() if message.metadata.date_sent else None), | |
| "edited": message.metadata.edited, | |
| "dateSent": (message.metadata.date_sent.isoformat() if message.metadata is not None and message.metadata.date_sent is not None else None), | |
| "edited": message.metadata.edited if message.metadata is not None else False, |
References
- When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use
is not Noneinstead of a truthiness check to avoid silently ignoring them.
Comment on lines
+918
to
+921
| except Exception: | ||
| # On any error, fall back to a minimal profile carrying just the | ||
| # user ID. Matches upstream's silent fallback. | ||
| return {"id": user_id} |
Contributor
There was a problem hiding this comment.
When fetching the user profile fails, the adapter falls back to a minimal profile carrying just the user ID. However, this fallback profile is not cached. As a result, every subsequent message from this user will trigger another failing Graph API call, leading to unnecessary network overhead and potential performance bottlenecks.
We should cache the fallback profile in self._user_profile_cache to prevent repeated failing API calls.
Suggested change
| except Exception: | |
| # On any error, fall back to a minimal profile carrying just the | |
| # user ID. Matches upstream's silent fallback. | |
| return {"id": user_id} | |
| except Exception: | |
| # On any error, fall back to a minimal profile carrying just the | |
| # user ID. Matches upstream's silent fallback. Cache the fallback | |
| # to avoid spamming the Graph API on subsequent messages. | |
| fallback: MessengerUserProfile = {"id": user_id} | |
| self._user_profile_cache[user_id] = fallback | |
| return fallback |
![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: 5b3b48734e
ℹ️ 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".
Comment on lines
+703
to
+705
| async def open_dm(self, user_id: str) -> str: | ||
| """Open a DM with a user. Returns the encoded thread ID.""" | ||
| return self.encode_thread_id(MessengerThreadId(recipient_id=user_id)) |
There was a problem hiding this comment.
When callers use the generic Chat.open_dm path (including the new AI sendDirectMessage tool), this Messenger open_dm implementation is not reachable: Chat._infer_adapter_from_user_id has cases for Slack, Teams, Google Chat, Linear, and numeric Discord/Telegram/GitHub IDs, but no Messenger case. As a result, a Messenger-only bot calling sendDirectMessage with a PSID such as the adapter’s own USER_123 examples raises Cannot infer adapter... instead of returning messenger:USER_123; add Messenger inference/qualified user-id handling or avoid advertising DM support through the generic path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chat_sdk/adapters/messenger/adapter.py`:
- Around line 799-807: Inbound attachment URLs are used directly to create
Attachment objects and downloader closures (via _make_attachment_downloader) and
then passed to session.get, which risks SSRF; add strict URL validation before
adding to result and inside the downloader: parse the URL in the
attachment-processing loop (where Attachment(...) is constructed) and reject or
skip if scheme is not http/https, host is empty, or host resolves to a
private/local IP, and likewise perform the same validation inside
_make_attachment_downloader (and any code paths that call session.get) so
rehydrated closures re-check the URL before issuing the request; reference the
Attachment construction, _make_attachment_downloader, _map_attachment_type, and
any session.get call sites and implement host whitelisting/blacklisting + IP
address checks (block RFC1918/localhost/169.254/IPv6 link-local) to satisfy the
SSRF protection requirement.
- Line 128: The current assignment self._api_version = config.api_version or
DEFAULT_API_VERSION (and similar "x or default" patterns for logger, app_secret,
page_access_token, verify_token) can override explicit falsy values like empty
strings; update these to use explicit None checks: if config.api_version is not
None then use config.api_version else DEFAULT_API_VERSION; similarly for logger,
app_secret, page_access_token, and verify_token, prefer the provided config
value when it is not None and only fall back to the environment (e.g.,
os.environ.get("...")) or a default when the config field is None to avoid
silently masking config bugs.
- Around line 914-917: After the isinstance(profile, dict) guard in the method
that retrieves and caches user profiles, cast profile to the declared type
MessengerUserProfile before assigning it into self._user_profile_cache and
before returning; specifically replace storing/returning the raw dict with a
casted value (e.g., profile_cast = cast(MessengerUserProfile, profile)) and then
set self._user_profile_cache[user_id] = profile_cast and return profile_cast so
the cache matches its declared dict[str, MessengerUserProfile] type.
- Around line 867-875: The function _map_attachment_type currently returns a
plain str but should be narrowed to the Attachment.type literal union
(Literal["image","file","video","audio"]); change the return type annotation of
_map_attachment_type to that literal union and ensure its returned values remain
one of "image", "video", "audio", or "file" so callers (e.g., where
type=self._map_attachment_type(...)) no longer need casts and type-checkers
accept the result.
- Around line 89-176: Add an async get_user method to MessengerAdapter with
signature async def get_user(self, user_id: str) -> UserInfo | None that calls
the existing self._fetch_user_profile(user_id), returns None if that call
returns None, and otherwise converts the returned MessengerUserProfile into a
UserInfo: set user_id, user_name and full_name from the MessengerUserProfile
helper _profile_display_name (or equivalent), avatar_url from profile_pic,
is_bot=False and email=None; ensure the method name and signature exactly match
the Adapter protocol so callers expecting chat_sdk.types.Adapter accept
MessengerAdapter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9f6c501-1da0-4cf4-9f61-72afa7b4c0af
📒 Files selected for processing (8)
pyproject.tomlsrc/chat_sdk/adapters/messenger/__init__.pysrc/chat_sdk/adapters/messenger/adapter.pysrc/chat_sdk/ai/__init__.pysrc/chat_sdk/ai/tools.pytests/test_ai_tools.pytests/test_messenger_api.pytests/test_messenger_webhook.py
Comment on lines
+89
to
+176
| class MessengerAdapter: | ||
| """Messenger (Meta) adapter for chat SDK. | ||
|
|
||
| Implements the chat-sdk ``Adapter`` interface for the Messenger Platform. | ||
| """ | ||
|
|
||
| def __init__(self, config: MessengerAdapterConfig) -> None: | ||
| # Upstream's constructor takes the resolved credentials directly. Our | ||
| # ``MessengerAdapterConfig`` exposes them via ``resolved_*`` helpers so | ||
| # the constructor and the factory both go through the same fallback | ||
| # chain. Constructing the adapter without resolved credentials is a | ||
| # programmer error — the factory enforces presence, and direct callers | ||
| # opt in to env-var fallbacks by leaving fields as ``None``. | ||
| app_secret = config.resolved_app_secret() | ||
| page_access_token = config.resolved_page_access_token() | ||
| verify_token = config.resolved_verify_token() | ||
| if not app_secret: | ||
| raise ValidationError( | ||
| "messenger", | ||
| f"appSecret is required. Set {ENV_APP_SECRET} or provide it in config.", | ||
| ) | ||
| if not page_access_token: | ||
| raise ValidationError( | ||
| "messenger", | ||
| f"pageAccessToken is required. Set {ENV_PAGE_ACCESS_TOKEN} or provide it in config.", | ||
| ) | ||
| if not verify_token: | ||
| raise ValidationError( | ||
| "messenger", | ||
| f"verifyToken is required. Set {ENV_VERIFY_TOKEN} or provide it in config.", | ||
| ) | ||
|
|
||
| self._name = "messenger" | ||
| self._lock_scope: LockScope = "channel" | ||
| self._persist_message_history = True | ||
|
|
||
| self._app_secret = app_secret | ||
| self._page_access_token = page_access_token | ||
| self._verify_token = verify_token | ||
| self._api_version = config.api_version or DEFAULT_API_VERSION | ||
| self._graph_api_url = f"{GRAPH_API_BASE}/{self._api_version}" | ||
| self._logger: Logger = config.logger | ||
| self._format_converter = MessengerFormatConverter() | ||
|
|
||
| # If a user_name is provided we treat it as explicit and never | ||
| # overwrite it from ``/me`` / ``chat.get_user_name()`` — matches the | ||
| # upstream ``hasExplicitUserName`` gate. ``is not None`` (not truthy) | ||
| # so an explicit ``user_name=""`` is respected rather than silently | ||
| # replaced by the ``"bot"`` fallback. | ||
| self._has_explicit_user_name = config.user_name is not None | ||
| self._user_name = config.user_name if config.user_name is not None else "bot" | ||
|
|
||
| self._chat: ChatInstance | None = None | ||
| self._bot_user_id: str | None = None | ||
|
|
||
| # Local caches. Messenger has no message-history API, so the adapter | ||
| # holds every parsed message in memory to back ``fetch_messages``. | ||
| self._message_cache: dict[str, list[Message]] = {} | ||
| self._user_profile_cache: dict[str, MessengerUserProfile] = {} | ||
|
|
||
| # Shared aiohttp session for connection pooling; created lazily. | ||
| self._http_session: Any | None = None | ||
|
|
||
| # ========================================================================= | ||
| # Adapter interface properties | ||
| # ========================================================================= | ||
|
|
||
| @property | ||
| def name(self) -> str: | ||
| return self._name | ||
|
|
||
| @property | ||
| def lock_scope(self) -> LockScope: | ||
| return self._lock_scope | ||
|
|
||
| @property | ||
| def persist_message_history(self) -> bool: | ||
| return self._persist_message_history | ||
|
|
||
| @property | ||
| def user_name(self) -> str: | ||
| return self._user_name | ||
|
|
||
| @property | ||
| def bot_user_id(self) -> str | None: | ||
| return self._bot_user_id | ||
|
|
||
| # ========================================================================= |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the adapter file and print relevant ranges around the reported line numbers
FILE="src/chat_sdk/adapters/messenger/adapter.py"
echo "=== File: $FILE ==="
test -f "$FILE" && true
python3 - <<'PY'
import itertools,sys
path="src/chat_sdk/adapters/messenger/adapter.py"
start_lines=[330,350,370,380,420,440]
for s in start_lines:
print(f"\n--- {path}:{s}-{s+120} ---")
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if s <= i <= s+120:
sys.stdout.write(f"{i:5d}:{line}")
PY
# Search for get_user in the messenger adapter
echo "=== rg: get_user in messenger adapter ==="
rg -n "def get_user|get_user\(" src/chat_sdk/adapters/messenger/adapter.py || true
# Locate Adapter protocol definition
echo "=== Find Adapter protocol/interface ==="
rg -n "class Adapter\b|Protocol\W*Adapter|get_user\(" src/chat_sdk -S || true
# Print the Adapter definition block(s)
python3 - <<'PY'
import re,sys,os,glob
paths=[]
for root,dirs,files in os.walk("src/chat_sdk"):
for fn in files:
if fn.endswith(".py"):
paths.append(os.path.join(root,fn))
needle=re.compile(r"(class\s+Adapter\b|Protocol\s*\(?\s*Adapter\b|class\s+Adapter\s*\(|get_user\s*\()")
for p in paths:
try:
with open(p,'r',encoding='utf-8') as f:
txt=f.read()
except Exception:
continue
if "Adapter" in p or "adapter" in p:
pass
if "get_user" in txt and ("Adapter" in txt or "Protocol" in txt):
if needle.search(txt):
print("\n### Candidate file:",p)
# Print a small window around get_user occurrences
lines=txt.splitlines()
for i,l in enumerate(lines, start=1):
if "get_user" in l and ("def " in l or "get_user(" in l):
a=max(1,i-20); b=min(len(lines),i+40)
print(f"--- {p}:{a}-{b} ---")
for j in range(a,b+1):
print(f"{j:5d}:{lines[j-1]}")
PY
# Find attachment download URL usage (for the URL-validation part of the original comment context)
echo "=== Search attachment download URL fetch path ==="
rg -n "_extract_attachments|_download_attachment|attachment.*url|download.*attachment" src/chat_sdk/adapters/messenger/adapter.py || true
# Check credential/env fallback usage for guideline issues ('x or default')
echo "=== Search for 'or DEFAULT' / 'x or ' patterns around credential fallback ==="
rg -n "or\s+DEFAULT|resolved_.*\(|config\..*or\s+DEFAULT|ENV_.*or" src/chat_sdk/adapters/messenger/adapter.py || trueRepository: Chinchill-AI/chat-sdk-python
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/chat_sdk/adapters/messenger/adapter.py"
echo "=== class MessengerAdapter definition (bases) ==="
rg -n "class MessengerAdapter\\b" "$FILE" || true
python3 - <<'PY'
import re
path="src/chat_sdk/adapters/messenger/adapter.py"
with open(path,'r',encoding='utf-8') as f: lines=f.readlines()
for i,l in enumerate(lines,1):
if re.search(r'class\s+MessengerAdapter\b', l):
# print a small window of the class header
start=max(1,i-5); end=min(len(lines), i+5)
for j in range(start,end+1):
print(f"{j:5d}:{lines[j-1].rstrip()}")
break
PY
echo "=== rg: def get_user in messenger adapter ==="
rg -n "def get_user\\b|async def get_user\\b" "$FILE" || true
echo "=== types.py: where Adapter protocol / BaseAdapter defines get_user ==="
rg -n "class Adapter\\(Protocol\\)|class BaseAdapter|async def get_user\\(" src/chat_sdk/types.py | head -n 50Repository: Chinchill-AI/chat-sdk-python
Length of output: 864
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/chat_sdk/adapters/messenger/adapter.py"
echo "=== rg: _fetch_user_profile (or similar) ==="
rg -n "_fetch_user_profile|fetch_user_profile|get_user_profile|user_profile" "$FILE" || true
echo "=== Show nearby definitions for any match ==="
python3 - <<'PY'
import re
path="src/chat_sdk/adapters/messenger/adapter.py"
with open(path,'r',encoding='utf-8') as f: lines=f.readlines()
for i,l in enumerate(lines,1):
if re.search(r"def\s+_fetch_user_profile|def\s+fetch_user_profile|def\s+_get_user_profile|get_user_profile", l):
a=max(1,i-10); b=min(len(lines),i+60)
print(f"\n--- {path}:{a}-{b} ---")
for j in range(a,b+1):
print(f"{j:5d}:{lines[j-1].rstrip()}")
PYRepository: Chinchill-AI/chat-sdk-python
Length of output: 4008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== types.py: locate UserInfo definition ==="
rg -n "class UserInfo\\b|UserInfo\\s*=\\s*TypedDict|UserInfo\\b.*TypedDict" src/chat_sdk/types.py
# print nearby block(s)
python3 - <<'PY'
import re
path="src/chat_sdk/types.py"
with open(path,'r',encoding='utf-8') as f: lines=f.readlines()
targets=[]
for i,l in enumerate(lines,1):
if re.search(r"class\s+UserInfo\b", l) or re.search(r"\bUserInfo\s*[:=]\s*TypedDict\b", l) or "UserInfo" in l and "TypedDict" in l:
targets.append(i)
for i in targets[:5]:
a=max(1,i-5); b=min(len(lines),i+60)
print(f"\n--- {path}:{a}-{b} ---")
for j in range(a,b+1):
print(f"{j:5d}:{lines[j-1].rstrip()}")
PY
echo "=== Inspect ActionEvent/ReactionEvent typing for adapter field ==="
rg -n "class ActionEvent\\b|class ReactionEvent\\b|ActionEvent\\(|ReactionEvent\\(" src/chat_sdk | head -n 50
# Show their definitions (small windows)
python3 - <<'PY'
import os,re
def show(p, start_re):
with open(p,'r',encoding='utf-8') as f: lines=f.readlines()
for i,l in enumerate(lines,1):
if re.search(start_re, l):
a=max(1,i-5); b=min(len(lines),i+80)
print(f"\n--- {p}:{a}-{b} ---")
for j in range(a,b+1):
print(f"{j:5d}:{lines[j-1].rstrip()}")
return
print("not found",p)
# find likely file(s) containing ActionEvent/ReactionEvent
files=[]
for root,dirs,fs in os.walk("src/chat_sdk"):
for fn in fs:
if fn.endswith(".py"):
p=os.path.join(root,fn)
files.append(p)
for p in files:
try:
txt=open(p,'r',encoding='utf-8').read()
except:
continue
if "class ActionEvent" in txt and "adapter:" in txt:
show(p, r"class\s+ActionEvent\b")
if "class ReactionEvent" in txt and "adapter:" in txt:
show(p, r"class\s+ReactionEvent\b")
PY
echo "=== Other adapters get_user implementations (signature) ==="
rg -n "async def get_user\\(self, user_id: str\\) -> UserInfo \\| None" src/chat_sdk/adapters -S | head -n 50
for f in $(rg -l "async def get_user\\(self, user_id: str\\)" src/chat_sdk/adapters | head -n 5); do
echo "---- $f (get_user block) ----"
python3 - <<PY
import re
p="$f"
with open(p,'r',encoding='utf-8') as f2: lines=f2.readlines()
for i,l in enumerate(lines,1):
if re.search(r"async def get_user\\(", l):
a=max(1,i-5); b=min(len(lines),i+80)
print(f"--- {p}:{a}-{b} ---")
for j in range(a,b+1):
print(f"{j:5d}:{lines[j-1].rstrip()}")
break
PY
doneRepository: Chinchill-AI/chat-sdk-python
Length of output: 32350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/chat_sdk/adapters/messenger/adapter.py"
echo "=== rg: MessengerUserProfile definition ==="
rg -n "MessengerUserProfile" "$FILE" | head -n 50
python3 - <<'PY'
import re
path="src/chat_sdk/adapters/messenger/adapter.py"
with open(path,'r',encoding='utf-8') as f: lines=f.readlines()
for i,l in enumerate(lines,1):
if re.search(r"\bMessengerUserProfile\b", l) and ("TypedDict" in l or "class MessengerUserProfile" in l or "MessengerUserProfile =" in l):
a=max(1,i-5); b=min(len(lines),i+80)
print(f"\n--- {path}:{a}-{b} ---")
for j in range(a,b+1):
print(f"{j:5d}:{lines[j-1].rstrip()}")
break
PY
echo "=== rg: where _profile_display_name is used ==="
rg -n "_profile_display_name" "$FILE" || trueRepository: Chinchill-AI/chat-sdk-python
Length of output: 682
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/chat_sdk/adapters/messenger/adapter.py"
python3 - <<'PY'
path="src/chat_sdk/adapters/messenger/adapter.py"
with open(path,'r',encoding='utf-8') as f: lines=f.readlines()
for target in [630,640,650,660,670,675,680,690,700]:
end=target+80
print(f"\n--- {path}:{target}-{end} ---")
for i in range(target, min(end,len(lines))+1):
print(f"{i:5d}:{lines[i-1].rstrip()}")
PYRepository: Chinchill-AI/chat-sdk-python
Length of output: 36902
Add missing get_user to MessengerAdapter to satisfy the Adapter protocol (fix type-check CI).
Type errors at lines 359/393/432 come from passing self where chat_sdk.types.Adapter is expected; MessengerAdapter has no get_user. Implement:
async def get_user(self, user_id: str) -> UserInfo | None(matchingchat_sdk/types.py)- Delegate to the existing
self._fetch_user_profile(user_id)and mapMessengerUserProfile→UserInfo(e.g.,user_id,user_name/full_namefrom_profile_display_name,avatar_urlfromprofile_pic,is_bot=False,email=None), returningNoneonly if you intentionally want “not found” semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/adapters/messenger/adapter.py` around lines 89 - 176, Add an
async get_user method to MessengerAdapter with signature async def
get_user(self, user_id: str) -> UserInfo | None that calls the existing
self._fetch_user_profile(user_id), returns None if that call returns None, and
otherwise converts the returned MessengerUserProfile into a UserInfo: set
user_id, user_name and full_name from the MessengerUserProfile helper
_profile_display_name (or equivalent), avatar_url from profile_pic, is_bot=False
and email=None; ensure the method name and signature exactly match the Adapter
protocol so callers expecting chat_sdk.types.Adapter accept MessengerAdapter.
Source: Pipeline failures
| self._app_secret = app_secret | ||
| self._page_access_token = page_access_token | ||
| self._verify_token = verify_token | ||
| self._api_version = config.api_version or DEFAULT_API_VERSION |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/chat_sdk/adapters/messenger/adapter.py"
echo "== Target file =="
ls -l "$FILE" || true
echo
echo "== Snippet around line 128 =="
nl -ba "$FILE" | sed -n '110,150p'
echo
echo "== Search for common truthy fallbacks in this file =="
rg -n "config\.api_version\s+or\s+DEFAULT_API_VERSION|logger\s+or\s+ConsoleLogger|app_secret\s+or\s+os\.environ|page_access_token\s+or\s+os\.environ|verify_token\s+or\s+os\.environ" "$FILE" || true
echo
echo "== Snippet around lines 1165-1182 (as referenced) =="
nl -ba "$FILE" | sed -n '1145,1210p'Repository: Chinchill-AI/chat-sdk-python
Length of output: 276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/chat_sdk/adapters/messenger/adapter.py"
echo "== Line count =="
wc -l "$FILE" || true
echo
echo "== Snippet around ~line 128 =="
cat -n "$FILE" | sed -n '110,150p'
echo
echo "== Search for truthy fallbacks in this file =="
rg -n "config\.api_version\s+or\s+DEFAULT_API_VERSION|logger\s+or\s+ConsoleLogger|_logger\s*=.*\s+logger\s+or\s+ConsoleLogger|app_secret\s+or\s+os\.environ|page_access_token\s+or\s+os\.environ|verify_token\s+or\s+os\.environ" "$FILE" || true
echo
echo "== Snippet around ~lines 1165-1182 (if present) =="
if [ "$(wc -l < "$FILE")" -ge 1182 ]; then
cat -n "$FILE" | sed -n '1145,1210p'
else
echo "File has fewer than 1182 lines; skipping range."
fi
echo
echo "== Search for 'or os.environ' occurrences =="
rg -n "or\s+os\.environ\.get\(" "$FILE" || true
echo
echo "== Search for 'logger or' occurrences =="
rg -n "logger\s+or\s+" "$FILE" || true
echo
echo "== Search for 'api_version or' occurrences =="
rg -n "api_version\s+or\s+" "$FILE" || trueRepository: Chinchill-AI/chat-sdk-python
Length of output: 5747
Avoid truthy or fallbacks in Messenger config resolution
x or default can silently replace explicitly provided falsy values (e.g., "") with env values for credentials/tokens, masking configuration bugs. Use explicit is not None checks in src/chat_sdk/adapters/messenger/adapter.py for config.api_version and the env fallback wiring for logger, app_secret, page_access_token, and verify_token.
Suggested patch
- self._api_version = config.api_version or DEFAULT_API_VERSION
+ self._api_version = config.api_version if config.api_version is not None else DEFAULT_API_VERSION
...
- _logger = logger or ConsoleLogger("info").child("messenger")
+ _logger = logger if logger is not None else ConsoleLogger("info").child("messenger")
...
- _app_secret = app_secret or os.environ.get(ENV_APP_SECRET)
+ _app_secret = app_secret if app_secret is not None else os.environ.get(ENV_APP_SECRET)
...
- _page_access_token = page_access_token or os.environ.get(ENV_PAGE_ACCESS_TOKEN)
+ _page_access_token = (
+ page_access_token if page_access_token is not None else os.environ.get(ENV_PAGE_ACCESS_TOKEN)
+ )
...
- _verify_token = verify_token or os.environ.get(ENV_VERIFY_TOKEN)
+ _verify_token = verify_token if verify_token is not None else os.environ.get(ENV_VERIFY_TOKEN)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/adapters/messenger/adapter.py` at line 128, The current
assignment self._api_version = config.api_version or DEFAULT_API_VERSION (and
similar "x or default" patterns for logger, app_secret, page_access_token,
verify_token) can override explicit falsy values like empty strings; update
these to use explicit None checks: if config.api_version is not None then use
config.api_version else DEFAULT_API_VERSION; similarly for logger, app_secret,
page_access_token, and verify_token, prefer the provided config value when it is
not None and only fall back to the environment (e.g., os.environ.get("...")) or
a default when the config field is None to avoid silently masking config bugs.
Source: Coding guidelines
Comment on lines
+799
to
+807
| url = payload.get("url") | ||
| if not url: | ||
| continue | ||
| result.append( | ||
| Attachment( | ||
| type=self._map_attachment_type(attachment.get("type", "")), | ||
| url=url, | ||
| fetch_data=self._make_attachment_downloader(url), | ||
| # Persist the download URL so the closure can be rebuilt |
There was a problem hiding this comment.
Validate attachment URLs before outbound fetches to prevent SSRF paths.
Attachment URLs from inbound payload/metadata are passed straight to session.get(...) without validation. Add URL validation (scheme/host constraints and private-network blocking) before building/rehydrating download closures and before issuing requests.
As per coding guidelines, validate external URLs before making requests to prevent SSRF attacks.
Also applies to: 831-864, 877-882
🧰 Tools
🪛 GitHub Actions: Lint & Type Check / 0_Lint & Type Check.txt
[error] 804-804: pyrefly/mypy: Argument str is not assignable to parameter type with type Literal['audio', 'file', 'image', 'video'] in chat_sdk.types.Attachment.__init__ [bad-argument-type].
🪛 GitHub Actions: Lint & Type Check / Lint & Type Check
[error] 804-804: pyrefly type check failed: Argument str is not assignable to parameter type with type Literal['audio', 'file', 'image', 'video'] in chat_sdk.types.Attachment.__init__ [bad-argument-type].
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/adapters/messenger/adapter.py` around lines 799 - 807, Inbound
attachment URLs are used directly to create Attachment objects and downloader
closures (via _make_attachment_downloader) and then passed to session.get, which
risks SSRF; add strict URL validation before adding to result and inside the
downloader: parse the URL in the attachment-processing loop (where
Attachment(...) is constructed) and reject or skip if scheme is not http/https,
host is empty, or host resolves to a private/local IP, and likewise perform the
same validation inside _make_attachment_downloader (and any code paths that call
session.get) so rehydrated closures re-check the URL before issuing the request;
reference the Attachment construction, _make_attachment_downloader,
_map_attachment_type, and any session.get call sites and implement host
whitelisting/blacklisting + IP address checks (block
RFC1918/localhost/169.254/IPv6 link-local) to satisfy the SSRF protection
requirement.
Source: Coding guidelines
19 tasks
…alignment Ports the core slice of vercel/chat#444 (processMessage returns the inner task as Promise<void>): process_message now returns the eagerly-started asyncio.Task so streaming callers can await full handler completion and observe handler exceptions; wait_until keeps swallowed-error semantics. Fidelity alignment for the chat@4.29.0 pin: - MAPPING follows upstream's ai.test.ts split into ai/messages.test.ts + ai/index.test.ts (vercel/chat#492) - tests/test_ai_tools.py: 14 tests renamed to converter-exact names so the strict checker exact-matches the upstream it() titles - new faithful ports in tests/test_chat_faithful.py: the #444 awaitable test, plus the #459/#495 queue/burst subject-rehydration tests, skipif- gated on BaseAdapter.fetch_subject so they activate when PR #131 lands https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64
…in, ai example - pyproject 0.4.29a2 -> 0.4.29; README status + 9-platform claims - CHANGELOG: consolidated 0.4.29 release entry (0.4.29a3 notes folded in) + backfilled 0.4.27.1 section - lint.yml upstream clone re-pinned chat@4.26.0 -> chat@4.29.0 - docs/UPSTREAM_SYNC.md: version-table rows for 0.4.27.1/0.4.29; Known Non-Parity rows for GitHub octokit / Linear linear_client getters (no Python SDK object to expose), @chat-adapter/tests kit, and the Teams microsoft-teams-apps migration deferral to 0.4.30 (issue #93) - CLAUDE.md: pin references moved to chat@4.29.0 - examples/ai_tools_example.py + README AI section (chat/ai PR 3 of 3, design #109) - 0.4.30 wave tracking issue: #135 https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64
) Port of upstream 3546b3f. Slack natively renders markdown via the markdown_text parameter on chat.postMessage / postEphemeral / update / scheduleMessage, so the adapter passes markdown through directly instead of converting to mrkdwn. - str / PostableRaw messages still go to text (preserves literal *). - PostableMarkdown / PostableAst go to markdown_text (12k char limit). - to_blocks_with_table, _mdast_table_to_slack_block, and the _render_with_table_blocks call sites are removed (tables now ride along in markdown_text). - SlackMarkdownConverter alias removed; use SlackFormatConverter. - render_formatted(ast) / from_ast now return standard markdown (was mrkdwn) via stringify_markdown. - response_url payloads reject markdown_text (no_text), so those render through the retained mrkdwn node renderer (to_response_url_text). - Incoming message events still arrive as mrkdwn and parse unchanged. https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64 (cherry picked from commit f07815e327ef1f2ba8ff688b2eb593af818c4ab6)
…ercel/chat#467) Port of upstream c46fdb6. Adds SlackAdapterConfig.installation_provider (SlackInstallationProvider protocol) for multi-workspace apps using external token management (e.g. Vercel Connect). When set, the adapter bypasses internal StateAdapter storage for token lookups on incoming webhooks — the provider is authoritative (no state fallback) and read-only (set_installation / delete_installation / OAuth callback still write to internal state). Enterprise Grid support rides along: org-wide installs (is_enterprise_install) resolve by enterprise_id instead of team_id across event_callback, slash command, and interactive payload entry paths; RequestContext carries enterprise_id/is_enterprise_install; attachment fetch_metadata captures enterpriseId/isEnterpriseInstall (omitted when absent) and rehydrate_attachment routes through _resolve_token_for_team so the provider is honored after a JSON roundtrip. Composes with the existing bot_token resolver design (see docs/UPSTREAM_SYNC.md non-parity rows): a configured default bot_token (static or resolver) still selects single-workspace mode and bypasses per-installation resolution entirely; _get_token / client caches are untouched. https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64 (cherry picked from commit ee1e30703514d8a871d7f64c0ccc17c0774a7d6d)
Port of upstream b9b17cd. Discord sends interactions through either
the Gateway or an Interactions Endpoint URL, not both — deployments
without an endpoint URL receive interactions over the Gateway, and the
adapter previously dropped them ("Forwarded Gateway event (no
handler)").
The Python adapter is HTTP-interactions-only with a gateway-forwarder
receiver, so the port lands on that surface: a forwarded
GATEWAY_INTERACTION_CREATE event (raw wire-format INTERACTION_CREATE
dispatch payload) is now acknowledged via the interaction callback
REST endpoint — POST /interactions/{id}/{token}/callback with type 5
for slash commands / type 6 for components, the same wire calls
upstream's resident discord.js handler makes via deferReply() /
deferUpdate() — then routed through the existing slash-command and
action handler paths. Deferred slash responses resolve exactly like
HTTP ones (post_message PATCHes the @original webhook message).
Callback path segments are URL-quoted (hazard #12) and defer failures
are logged without dispatching the handler, matching upstream's
listener-level catch.
Also aligns slash-command boolean option flattening with TS
String(true) — "status true", not Python str(True) "status True" —
pinned by the ported upstream test.
https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64
(cherry picked from commit 458428afc383b57704e6fe7c6d8abde23235afda)
…history (vercel/chat#448) Port of upstream 46d183b (chat@4.29.0). Rename (with back-compat, mirroring upstream): - message_history.py -> thread_history.py; MessageHistoryCache -> ThreadHistoryCache. Old module path kept as a deprecated re-export shim. - ChatConfig.thread_history added; deprecated ChatConfig.message_history still read, thread_history wins when both are set. - Adapter.persist_thread_history added; deprecated persist_message_history still honored (either flag enables persistence). Telegram and WhatsApp adapters switch to the new flag, matching upstream. - State storage key prefix "msg-history:" is deliberately unchanged so existing persisted data is not orphaned. New Transcripts API: - transcripts.py: TranscriptsApiImpl (append/list/count/delete) keyed by a cross-platform user key, backed by StateAdapter.append_to_list. delete() writes a tombstone via append_to_list(max_length=1) because state.delete only addresses the k/v namespace on non-memory adapters. - ChatConfig.transcripts + ChatConfig.identity (IdentityResolver); the constructor raises when transcripts is set without identity. Inbound dispatch resolves message.user_key once per message via the resolver. - chat.transcripts accessor raises when not configured (fail loudly). - New types: TranscriptEntry, TranscriptsConfig, TranscriptRole, AppendInput, AppendOptions, ListQuery, CountQuery, DeleteTarget, DeleteResult, IdentityContext, IdentityResolver, DurationString. Tests: test_message_history.py renamed to test_thread_history.py (names aligned to thread-history.test.ts titles, deprecated-alias test added); chat.test.ts persistThreadHistory block ported into test_chat_faithful.py; new test_transcripts.py and test_transcripts_wiring.py port transcripts.test.ts and transcripts-wiring.test.ts 1:1. https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64 (cherry picked from commit 765420dc4d21804b19f1d8eccddebc3a392b0e61)
https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64 (cherry picked from commit 20a5fd10a2dd46094fdf812ff47628a12bb9d362)
- MAPPING: four new chat@4.29.0 core test files mapped (callback-url, thread-history, transcripts, transcripts-wiring) — 12 of 19 files now in scope; stale 4.26-era references in the script/template updated - fidelity_baseline.json regenerated at chat@4.29.0 (731 TS tests in scope, 0 missing; strict mode green) - docs/UPSTREAM_SYNC.md: Known Non-Parity rows from the port wave — GitHub octokit / Linear linear_client getters, @chat-adapter/tests kit, Teams modal-submit webhook options slice, jsx-runtime callbackUrl props, Transcripts API Python adaptations, Slack legacy mrkdwn renderer scope note, Discord gateway-only interactions surface, and the Teams microsoft-teams-apps deferral to 0.4.30 - tests/test_chat_faithful.py: the [Slash Commands] duplicate-title openModal-unsupported port (matcher counts names as a multiset) and the #459 subject-wiring port (skipif-gated on BaseAdapter.fetch_subject, activates when PR #131 lands) https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64
patrick-chinchill
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| "slack:C123:1234.5678", | ||
| create_test_message("msg-burst-json-2", "Hey @slack-bot two"), | ||
| ) | ||
| await task |
|
|
||
| def get_installation( | ||
| self, installation_id: str, is_enterprise_install: bool | ||
| ) -> Awaitable[SlackInstallation | None]: ... |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/chat_sdk/adapters/discord/adapter.py (1)
170-172: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
persist_thread_historyto satisfy theAdapterprotocol.
DiscordAdapterexposespersist_message_historybut is missingpersist_thread_history, which is now required by the protocol. This is the root cause of the type-check breakages reported downstream (Line 404, Line 491, Line 763, Line 857) whenselfis passed whereAdapteris expected.Proposed fix
`@property` def persist_message_history(self) -> bool | None: return None + + `@property` + def persist_thread_history(self) -> bool | None: + # Backward-compatible alias for newer Adapter protocol surface. + return self.persist_message_history🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat_sdk/adapters/discord/adapter.py` around lines 170 - 172, The DiscordAdapter class currently implements persist_message_history but is missing the persist_thread_history property required by the Adapter protocol; add a persist_thread_history property on DiscordAdapter (matching the type signature bool | None) that returns the same default/behavior as persist_message_history so instances satisfy the Adapter protocol (refer to the persist_message_history property on DiscordAdapter and the Adapter protocol's persist_thread_history requirement when implementing).Source: Pipeline failures
src/chat_sdk/adapters/slack/adapter.py (1)
542-548: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExpose
persist_thread_historyon the adapter.The class still only advertises
persist_message_history, so it no longer satisfies theAdapterprotocol and this file keeps failing pyright/pyrefly at every event-construction site.💡 Proposed fix
`@property` def persist_message_history(self) -> bool: return self._persist_message_history + + `@property` + def persist_thread_history(self) -> bool: + return self._persist_message_history🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat_sdk/adapters/slack/adapter.py` around lines 542 - 548, The adapter class currently exposes lock_scope and persist_message_history but is missing the persist_thread_history property required by the Adapter protocol; add a `@property` named persist_thread_history that returns self._persist_thread_history (mirroring persist_message_history) so code sites expecting Adapter.persist_thread_history type-check correctly and reference the same internal flag used by this adapter.Source: Pipeline failures
src/chat_sdk/chat.py (2)
952-988: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate the protocol contract to match the new return value.
Chat.process_message()now returns the created task, butChatInstance.process_message()insrc/chat_sdk/types.pystill returnsNone. That leaves the concrete class out of sync with the interface and blocks adapters typed againstChatInstancefrom legally using the new task handle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat_sdk/chat.py` around lines 952 - 988, The interface for ChatInstance.process_message is out of sync with Chat.process_message: Chat.process_message now returns asyncio.Task[None] | None but ChatInstance.process_message in types still returns None; update the ChatInstance.process_message signature to return the same type (asyncio.Task[None] | None) so implementations and adapters can use the returned task. Locate the ChatInstance.process_message declaration in src/chat_sdk/types.py and change its return type to match the concrete method (use the same union type or an appropriate alias), and update any related type imports (e.g., import asyncio.Task or typing annotations) to keep type checking passing.
1336-1355: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist modal context before calling
open_modal().
_store_modal_context()schedules the state write and returns immediately. If that write lags or gets dropped at request teardown, the modal submit path losesrelated_*andcallback_urlstate for the samecontext_id. Making this helper async and awaiting it at both modal-opening call sites removes that race.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat_sdk/chat.py` around lines 1336 - 1355, Make _store_modal_context an async function that awaits the state write instead of fire-and-forget: change def _store_modal_context(...) to async def _store_modal_context(...), and inside either await self._state_adapter.set(key, context, MODAL_CONTEXT_TTL_MS) directly or await the task returned by _create_task so the write completes before returning. Then update both modal-opening call sites that invoke _store_modal_context (the places that call open_modal / the modal-opening logic) to await _store_modal_context(...) so the modal submit path reliably sees related_* and callback_url state for the same context_id.src/chat_sdk/channel.py (1)
363-377: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon't mint 30-day callback tokens at schedule time.
schedule()now tokenizes card callbacks immediately, but the stored state entry expires 30 days after scheduling, not 30 days after delivery. Any message scheduled beyond that window will arrive with callback buttons that are already unresolvable. This needs either delivery-time tokenization or a TTL derived frompost_at.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat_sdk/channel.py` around lines 363 - 377, The schedule() method currently calls _process_callback_urls(message) at schedule time which mints callback tokens that expire 30 days from now; change schedule() to avoid minting tokens immediately—remove the _process_callback_urls call and pass the raw message to adapter.schedule_message(self._id, message, {"post_at": post_at}) so tokenization is deferred to delivery-time (e.g., in send()/deliver() path where _process_callback_urls can be called), or if you must mint now compute a TTL based on post_at (expires_at = post_at) and pass that TTL into _process_callback_urls or adapter.schedule_message so minted tokens expire at delivery time; update schedule(), _process_callback_urls, and the adapter.schedule_message usage accordingly to support deferred or TTL-based tokenization.
🧹 Nitpick comments (1)
tests/integration/test_replay_callback_url.py (1)
57-60: 🎯 Functional Correctness | ⚡ Quick winRoot cause: truthiness-based defaulting (
x or default) in Python test helpers.
tests/integration/test_replay_callback_url.pyandtests/test_callback_url.pyboth useorfallbacks where explicitNonechecks are required. This is the same portability/correctness trap and should be fixed consistently in both files.As per coding guidelines:
Use x if x is not None else default instead of x or default to avoid truthiness trap when porting from TypeScript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_replay_callback_url.py` around lines 57 - 60, The constructor __init__ currently uses truthiness fallback "headers or {}" which incorrectly replaces valid falsy values; change it to an explicit None check by setting self.headers = headers if headers is not None else {} in tests/integration/test_replay_callback_url.py and apply the same fix in the corresponding constructor or helper in tests/test_callback_url.py so both use "x if x is not None else default" instead of "x or default".Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chat_sdk/adapters/discord/cards.py`:
- Around line 54-69: The current encode_discord_custom_id only validates overall
length, allowing an empty or delimiter-containing action_id which leads to
broken decoding/dispatch; update encode_discord_custom_id to explicitly reject
an empty action_id and any action_id containing DISCORD_CUSTOM_ID_DELIMITER or
newline characters by raising ValidationError, and still validate the final
encoded string length via _validate_discord_custom_id; reference
_validate_discord_custom_id, encode_discord_custom_id, decode_discord_custom_id
and DISCORD_CUSTOM_ID_DELIMITER so the change is clearly tied to the
encoding/decoding flow.
In `@src/chat_sdk/adapters/slack/adapter.py`:
- Around line 994-1039: The current try/except in _resolve_token_for_team
swallows all exceptions (including from
self._installation_provider.get_installation) and returns None, which callers
interpret as "not found"; change the error handling so provider failures are not
collapsed into cache misses: call
self._installation_provider.get_installation(...) inside a small try/except that
logs the exception (use self._logger.error with the
installationId/isEnterpriseInstall and the exception) and then re-raise the
exception (or return a distinct failure signal upstream) instead of returning
None; keep the original fallback to await self.get_installation(installation_id)
only when the provider call succeeds or explicitly returns no installation, and
ensure RequestContext construction remains unchanged.
- Around line 505-507: The factory and adapter must reject using an external
installation provider in Socket Mode: update create_slack_adapter() to validate
that if config.mode == "socket" (or mode parameter equals "socket") then
config.installation_provider (and any passed SlackInstallationProvider) is not
accepted—raise a clear error (or ValueError) when installation_provider is set
for socket mode; also ensure the adapter's attribute self._installation_provider
(and any logic that assigns config.installation_provider) is only set when mode
is not "socket" so Socket Mode remains single-workspace-only.
In `@src/chat_sdk/adapters/slack/format_converter.py`:
- Around line 29-32: BARE_MENTION_REGEX is too permissive and rewrites
unresolved literals like "`@alice`" or "`@channel`" into `<`@alice`>`; change it to
only match Slack user-ID-shaped tokens and/or exclude special commands: update
BARE_MENTION_REGEX to something like r"(?<![<\w])@([UW][A-Z0-9]+)" (match Slack
IDs that start with U or W and contain caps/numbers) and ensure the
transformation code in SlackFormatConverter._finalize() and
SlackFormatConverter._node_to_mrkdwn() only applies this regex; separately
handle or explicitly exclude the special words "channel", "here", "everyone",
and "group" (or convert them to `<!channel>`/`<!here>`/`<!everyone>`/`<!group>`
where intended) and do not apply the rewrite when
adapter.py::_resolve_outgoing_mentions() left a literal unresolved.
In `@src/chat_sdk/adapters/telegram/adapter.py`:
- Line 597: The Adapter protocol still expects persist_message_history but this
adapter only defines _persist_thread_history/persist_thread_history; add a
temporary alias property named persist_message_history on the Telegram adapter
class that delegates to the existing self._persist_thread_history (getter and
setter) so the class satisfies the old protocol; do the same change in the
WhatsApp adapter (add a persist_message_history property that reads/writes the
existing _persist_thread_history/persist_thread_history) or alternatively
migrate the protocol and all call sites together—ensure the alias uses the same
backing field (self._persist_thread_history) to maintain identical behavior.
In `@src/chat_sdk/callback_url.py`:
- Around line 184-197: The _fetch function currently forwards any string URL to
aiohttp, creating an SSRF risk; before opening
aiohttp.ClientSession/session.request validate the URL: parse the URL in _fetch,
ensure the scheme is http or https, reject URLs containing credentials
(username/password in netloc), resolve the host (via socket.getaddrinfo) and
verify none of the resolved IPs are loopback, private, link-local, multicast, or
otherwise non-public using the ipaddress module; if validation fails raise an
exception (e.g., ValueError) and do not call
aiohttp.ClientSession/session.request. Ensure these checks live at the top of
_fetch so unsafe URLs are rejected before any network call.
In `@src/chat_sdk/channel.py`:
- Around line 470-475: When rebinding chat context in ChannelImpl.from_json the
code clears channel._thread_history but only restores _state_adapter_instance
when chat is provided, which loses the messages() cache/fallback; change the
logic so that when chat is passed in you also rebind the history cache instead
of unconditionally clearing it — e.g. if chat is not None set
channel._thread_history from chat.thread_history or recreate it according to
ChatConfig.thread_history (and mirror the same change in the other identical
block affecting lines around the second rebind) so messages() behavior and
append fallback are preserved.
In `@src/chat_sdk/chat.py`:
- Around line 1108-1115: The log statements calling self._logger.error that
include the full callback_url should be changed to log a redacted version of
callback_url instead of the raw value; implement or call a small sanitizer
(e.g., redact_callback_url or similar) that strips or masks query params and
sensitive path segments and use that sanitized value in both the Modal callback
POST error branches shown (the block with str(post_result.error) and the except
Exception as error branch), and make the same replacement for the analogous
logger calls around lines 1462-1468 so no raw callback_url is emitted in logs.
In `@src/chat_sdk/transcripts.py`:
- Around line 70-83: The helper _parse_duration should be narrowed to return
only int | None to match StateAdapter.append_to_list's contract: change the type
hint to int | None and ensure any numeric or computed duration is normalized to
an integer (e.g., after computing n * MS_PER_UNIT[...] or if a numeric input is
float cast/convert to int) before returning; keep the current bool exclusion and
string parsing logic but explicitly cast the final result to int to avoid
returning floats.
In `@tests/test_chat_faithful.py`:
- Around line 4362-4367: Replace the hardcoded asyncio.sleep calls used to wait
for background persistence of modal context with a deterministic polling wait
with a timeout: repeatedly check state.cache for keys starting with
"modal-context:" (as done in the current assertion) until the expected count
appears or a short timeout elapses, and use asyncio.wait_for or an equivalent
awaitable to fail deterministically on timeout; apply the same polling pattern
for both occurrences (the block around _store_modal_context and the later block
around the modal callback at the other location) so tests wait reliably for the
background task to complete.
---
Outside diff comments:
In `@src/chat_sdk/adapters/discord/adapter.py`:
- Around line 170-172: The DiscordAdapter class currently implements
persist_message_history but is missing the persist_thread_history property
required by the Adapter protocol; add a persist_thread_history property on
DiscordAdapter (matching the type signature bool | None) that returns the same
default/behavior as persist_message_history so instances satisfy the Adapter
protocol (refer to the persist_message_history property on DiscordAdapter and
the Adapter protocol's persist_thread_history requirement when implementing).
In `@src/chat_sdk/adapters/slack/adapter.py`:
- Around line 542-548: The adapter class currently exposes lock_scope and
persist_message_history but is missing the persist_thread_history property
required by the Adapter protocol; add a `@property` named persist_thread_history
that returns self._persist_thread_history (mirroring persist_message_history) so
code sites expecting Adapter.persist_thread_history type-check correctly and
reference the same internal flag used by this adapter.
In `@src/chat_sdk/channel.py`:
- Around line 363-377: The schedule() method currently calls
_process_callback_urls(message) at schedule time which mints callback tokens
that expire 30 days from now; change schedule() to avoid minting tokens
immediately—remove the _process_callback_urls call and pass the raw message to
adapter.schedule_message(self._id, message, {"post_at": post_at}) so
tokenization is deferred to delivery-time (e.g., in send()/deliver() path where
_process_callback_urls can be called), or if you must mint now compute a TTL
based on post_at (expires_at = post_at) and pass that TTL into
_process_callback_urls or adapter.schedule_message so minted tokens expire at
delivery time; update schedule(), _process_callback_urls, and the
adapter.schedule_message usage accordingly to support deferred or TTL-based
tokenization.
In `@src/chat_sdk/chat.py`:
- Around line 952-988: The interface for ChatInstance.process_message is out of
sync with Chat.process_message: Chat.process_message now returns
asyncio.Task[None] | None but ChatInstance.process_message in types still
returns None; update the ChatInstance.process_message signature to return the
same type (asyncio.Task[None] | None) so implementations and adapters can use
the returned task. Locate the ChatInstance.process_message declaration in
src/chat_sdk/types.py and change its return type to match the concrete method
(use the same union type or an appropriate alias), and update any related type
imports (e.g., import asyncio.Task or typing annotations) to keep type checking
passing.
- Around line 1336-1355: Make _store_modal_context an async function that awaits
the state write instead of fire-and-forget: change def _store_modal_context(...)
to async def _store_modal_context(...), and inside either await
self._state_adapter.set(key, context, MODAL_CONTEXT_TTL_MS) directly or await
the task returned by _create_task so the write completes before returning. Then
update both modal-opening call sites that invoke _store_modal_context (the
places that call open_modal / the modal-opening logic) to await
_store_modal_context(...) so the modal submit path reliably sees related_* and
callback_url state for the same context_id.
---
Nitpick comments:
In `@tests/integration/test_replay_callback_url.py`:
- Around line 57-60: The constructor __init__ currently uses truthiness fallback
"headers or {}" which incorrectly replaces valid falsy values; change it to an
explicit None check by setting self.headers = headers if headers is not None
else {} in tests/integration/test_replay_callback_url.py and apply the same fix
in the corresponding constructor or helper in tests/test_callback_url.py so both
use "x if x is not None else default" instead of "x or default".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84ed5d76-b34e-493f-8997-6eb409415c7b
📒 Files selected for processing (51)
.github/workflows/lint.ymlCHANGELOG.mdCLAUDE.mdREADME.mddocs/UPSTREAM_SYNC.mdexamples/ai_tools_example.pypyproject.tomlscripts/fidelity_baseline.jsonscripts/verify_test_fidelity.pysrc/chat_sdk/__init__.pysrc/chat_sdk/adapters/discord/__init__.pysrc/chat_sdk/adapters/discord/adapter.pysrc/chat_sdk/adapters/discord/cards.pysrc/chat_sdk/adapters/discord/types.pysrc/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/adapters/slack/format_converter.pysrc/chat_sdk/adapters/slack/types.pysrc/chat_sdk/adapters/teams/adapter.pysrc/chat_sdk/adapters/telegram/adapter.pysrc/chat_sdk/adapters/whatsapp/adapter.pysrc/chat_sdk/callback_url.pysrc/chat_sdk/cards.pysrc/chat_sdk/channel.pysrc/chat_sdk/chat.pysrc/chat_sdk/message_history.pysrc/chat_sdk/modals.pysrc/chat_sdk/shared/mock_adapter.pysrc/chat_sdk/thread.pysrc/chat_sdk/thread_history.pysrc/chat_sdk/transcripts.pysrc/chat_sdk/types.pytests/integration/test_replay_callback_url.pytests/test_ai_tools.pytests/test_callback_url.pytests/test_channel_faithful.pytests/test_chat_faithful.pytests/test_chat_resolver.pytests/test_discord_adapter.pytests/test_discord_cards.pytests/test_discord_extended.pytests/test_serialization.pytests/test_slack_api.pytests/test_slack_format.pytests/test_slack_installation_provider.pytests/test_teams_native_streaming.pytests/test_telegram_adapter.pytests/test_thread_faithful.pytests/test_thread_history.pytests/test_transcripts.pytests/test_transcripts_wiring.pytests/test_whatsapp_adapter.py
✅ Files skipped from review due to trivial changes (6)
- .github/workflows/lint.yml
- scripts/fidelity_baseline.json
- src/chat_sdk/adapters/discord/types.py
- tests/test_teams_native_streaming.py
- CHANGELOG.md
- src/chat_sdk/adapters/teams/adapter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_ai_tools.py
Comment on lines
+54
to
+69
| def _validate_discord_custom_id(custom_id: str) -> None: | ||
| if len(custom_id) == 0 or len(custom_id) > DISCORD_CUSTOM_ID_MAX_LENGTH: | ||
| raise ValidationError( | ||
| "discord", | ||
| f"Discord custom_id must be 1-{DISCORD_CUSTOM_ID_MAX_LENGTH} characters. Shorten the button id or value.", | ||
| ) | ||
|
|
||
|
|
||
| def encode_discord_custom_id(action_id: str, value: str | None = None) -> str: | ||
| """Encode a button's action ID and optional value into a custom_id.""" | ||
| if value is None or value == "": | ||
| _validate_discord_custom_id(action_id) | ||
| return action_id | ||
| encoded = f"{action_id}{DISCORD_CUSTOM_ID_DELIMITER}{value}" | ||
| _validate_discord_custom_id(encoded) | ||
| return encoded |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject empty or delimiter-containing action_id in custom_id encoding.
Current validation only checks overall length. If action_id is empty (or contains \n), decode_discord_custom_id can produce an empty or truncated action_id, which then breaks action dispatch when consumed in the adapter (Line 392 in src/chat_sdk/adapters/discord/adapter.py).
Proposed fix
def encode_discord_custom_id(action_id: str, value: str | None = None) -> str:
"""Encode a button's action ID and optional value into a custom_id."""
+ if not action_id or DISCORD_CUSTOM_ID_DELIMITER in action_id:
+ raise ValidationError(
+ "discord",
+ "Button id must be non-empty and must not contain a newline.",
+ )
if value is None or value == "":
_validate_discord_custom_id(action_id)
return action_id
encoded = f"{action_id}{DISCORD_CUSTOM_ID_DELIMITER}{value}"
_validate_discord_custom_id(encoded)
return encoded🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/adapters/discord/cards.py` around lines 54 - 69, The current
encode_discord_custom_id only validates overall length, allowing an empty or
delimiter-containing action_id which leads to broken decoding/dispatch; update
encode_discord_custom_id to explicitly reject an empty action_id and any
action_id containing DISCORD_CUSTOM_ID_DELIMITER or newline characters by
raising ValidationError, and still validate the final encoded string length via
_validate_discord_custom_id; reference _validate_discord_custom_id,
encode_discord_custom_id, decode_discord_custom_id and
DISCORD_CUSTOM_ID_DELIMITER so the change is clearly tied to the
encoding/decoding flow.
Comment on lines
+505
to
+507
| # External installation provider (e.g. Vercel Connect). When set, | ||
| # per-installation token lookups bypass internal StateAdapter storage. | ||
| self._installation_provider: SlackInstallationProvider | None = config.installation_provider |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject installation_provider in Socket Mode too.
This new option can put the adapter into multi-workspace resolution without client_id/client_secret, but create_slack_adapter() still only blocks the OAuth fields. A mode="socket" + installation_provider config now slips through even though the factory/docstring say Socket Mode is single-workspace-only.
💡 Proposed fix (outside this hunk)
def create_slack_adapter(config: SlackAdapterConfig | None = None) -> SlackAdapter:
"""Create a new SlackAdapter instance.
@@
- if config is not None and (config.mode or "webhook") == "socket" and (config.client_id or config.client_secret):
+ if config is not None and (config.mode or "webhook") == "socket" and (
+ config.client_id is not None
+ or config.client_secret is not None
+ or config.installation_provider is not None
+ ):
raise ValidationError(
"slack",
"Multi-workspace (clientId/clientSecret) is not supported in socket mode.",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # External installation provider (e.g. Vercel Connect). When set, | |
| # per-installation token lookups bypass internal StateAdapter storage. | |
| self._installation_provider: SlackInstallationProvider | None = config.installation_provider | |
| def create_slack_adapter(config: SlackAdapterConfig | None = None) -> SlackAdapter: | |
| """Create a new SlackAdapter instance. | |
| Args: | |
| config: Optional configuration for the Slack adapter. | |
| Returns: | |
| A configured SlackAdapter instance. | |
| Raises: | |
| ValidationError: If Socket Mode is configured with multi-workspace options. | |
| """ | |
| if config is not None and (config.mode or "webhook") == "socket" and ( | |
| config.client_id is not None | |
| or config.client_secret is not None | |
| or config.installation_provider is not None | |
| ): | |
| raise ValidationError( | |
| "slack", | |
| "Multi-workspace (clientId/clientSecret) is not supported in socket mode.", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/adapters/slack/adapter.py` around lines 505 - 507, The factory
and adapter must reject using an external installation provider in Socket Mode:
update create_slack_adapter() to validate that if config.mode == "socket" (or
mode parameter equals "socket") then config.installation_provider (and any
passed SlackInstallationProvider) is not accepted—raise a clear error (or
ValueError) when installation_provider is set for socket mode; also ensure the
adapter's attribute self._installation_provider (and any logic that assigns
config.installation_provider) is only set when mode is not "socket" so Socket
Mode remains single-workspace-only.
Comment on lines
+994
to
1039
| async def _resolve_token_for_team( | ||
| self, installation_id: str, is_enterprise_install: bool = False | ||
| ) -> RequestContext | None: | ||
| """Resolve the bot token for an installation. | ||
|
|
||
| Checks the external installation provider first (e.g. Vercel | ||
| Connect); when no provider is configured, falls back to the | ||
| internal state adapter. | ||
|
|
||
| ``installation_id`` is the ``team_id`` -- or the ``enterprise_id`` | ||
| for Enterprise Grid org-wide installs (``is_enterprise_install``). | ||
| """ | ||
| try: | ||
| installation = await self.get_installation(team_id) | ||
| # Check external installation provider first (e.g. Vercel Connect) | ||
| if self._installation_provider is not None: | ||
| installation = await self._installation_provider.get_installation( | ||
| installation_id, is_enterprise_install | ||
| ) | ||
| if installation: | ||
| return RequestContext( | ||
| token=installation.bot_token, | ||
| bot_user_id=installation.bot_user_id, | ||
| ) | ||
| self._logger.warn( | ||
| "No installation found from provider", | ||
| {"installationId": installation_id, "isEnterpriseInstall": is_enterprise_install}, | ||
| ) | ||
| return None | ||
| # Fall back to internal state adapter | ||
| installation = await self.get_installation(installation_id) | ||
| if installation: | ||
| return RequestContext( | ||
| token=installation.bot_token, | ||
| bot_user_id=installation.bot_user_id, | ||
| ) | ||
| self._logger.warn("No installation found for team", {"teamId": team_id}) | ||
| self._logger.warn( | ||
| "No installation found for team", | ||
| {"installationId": installation_id, "isEnterpriseInstall": is_enterprise_install}, | ||
| ) | ||
| return None | ||
| except Exception as exc: | ||
| self._logger.error("Failed to resolve token for team", {"teamId": team_id, "error": exc}) | ||
| self._logger.error( | ||
| "Failed to resolve token for team", | ||
| {"installationId": installation_id, "isEnterpriseInstall": is_enterprise_install, "error": exc}, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't collapse installation-provider failures into normal cache misses.
The new installation_provider path catches every exception and returns None. Callers treat None as “installation not found” and often ack/continue, so a transient provider outage can silently drop event callbacks or interactive requests with no retry signal.
💡 Proposed fix
except Exception as exc:
self._logger.error(
"Failed to resolve token for team",
{"installationId": installation_id, "isEnterpriseInstall": is_enterprise_install, "error": exc},
)
- return None
+ raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/adapters/slack/adapter.py` around lines 994 - 1039, The current
try/except in _resolve_token_for_team swallows all exceptions (including from
self._installation_provider.get_installation) and returns None, which callers
interpret as "not found"; change the error handling so provider failures are not
collapsed into cache misses: call
self._installation_provider.get_installation(...) inside a small try/except that
logs the exception (use self._logger.error with the
installationId/isEnterpriseInstall and the exception) and then re-raise the
exception (or return a distinct failure signal upstream) instead of returning
None; keep the original fallback to await self.get_installation(installation_id)
only when the provider call succeeds or explicitly returns no installation, and
ensure RequestContext construction remains unchanged.
Comment on lines
+29
to
+32
| # Match bare @mentions (e.g. "@george") to rewrite as Slack's `<@george>`. | ||
| # The lookbehind excludes `<` (already-formatted mentions like `<@U123>`) and | ||
| # any word character, so email addresses like `user@example.com` are left alone. | ||
| BARE_MENTION_REGEX = re.compile(r"(?<![<\w])@(\w+)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🌐 Web query:
In Slack message payloads, does <@...>require a Slack user ID, and are@channel/@herespecial cases that should not be rewritten as<@name>?
💡 Result:
In Slack message payloads, the <@...> syntax specifically requires a Slack user ID (e.g., <@u12345678>) to correctly trigger a mention [1][2]. When using this syntax, the ID is essential for the Slack client to resolve the user and notify them properly [2]. Special mentions like @channel, @here, @everyone, and @group are not user mentions and should not be formatted as <@name>. Instead, they must be represented using the special command syntax <!foo>, where foo is the command [3]. Specifically: - @channel: <!channel> [3] - @here: <!here> [3] - @everyone: <!everyone> [3] - @group: <!group> [3] These <!...> sequences are distinct from user mentions and tell the Slack client to trigger the appropriate notification behavior [3]. For developers using Block Kit, you should use the designated API objects rather than raw strings whenever possible, but when using mrkdwn text objects, these specific formatting syntaxes are required [1][2][4].
Citations:
- 1: https://stackoverflow.com/questions/71300516/how-do-we-mention-or-tag-someone-in-using-slacks-block-kit-builder
- 2: https://docs.slack.dev/messaging/formatting-message-text
- 3: https://github.com/slackhq/slack-api-docs/blob/master/page_formatting.md
- 4: https://docs.slack.dev/reference/block-kit/composition-objects/text-object.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant slack adapter mention resolution + formatting converter code
files=(
"src/chat_sdk/adapters/slack/format_converter.py"
"src/chat_sdk/adapters/slack/adapter.py"
)
for f in "${files[@]}"; do
echo "==== $f ===="
if [ -f "$f" ]; then
wc -l "$f"
sed -n '1,260p' "$f" | nl -ba | sed -n '1,260p'
else
echo "MISSING: $f"
fi
done
# Also search for mention/regex usage to ensure we capture all call sites
echo "==== rg mention related ===="
rg -n "BARE_MENTION_REGEX|to_slack_payload|to_response_url_text|_finalize|_node_to_mrkdwn|_resolve_outgoing_mentions|mention" src/chat_sdk/adapters/slack -SRepository: Chinchill-AI/chat-sdk-python
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
"src/chat_sdk/adapters/slack/format_converter.py"
"src/chat_sdk/adapters/slack/adapter.py"
)
for f in "${files[@]}"; do
echo "==== $f ===="
if [ -f "$f" ]; then
wc -l "$f"
# Print lines 1-260 with line numbers
cat -n "$f" | sed -n '1,260p'
else
echo "MISSING: $f"
fi
done
echo "==== rg mention related ===="
rg -n "BARE_MENTION_REGEX|_finalize|to_slack_payload|to_response_url_text|_node_to_mrkdwn|_resolve_outgoing_mentions|<!@|<!channel|<!here|<!everyone|<!group|`@channel`|`@here`|`@everyone`|`@group`" src/chat_sdk/adapters/slack -SRepository: Chinchill-AI/chat-sdk-python
Length of output: 25844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print targeted sections with context
sed -n '2820,3010p' src/chat_sdk/adapters/slack/adapter.py
echo "----"
sed -n '3460,3700p' src/chat_sdk/adapters/slack/adapter.py
echo "----"
sed -n '3880,4080p' src/chat_sdk/adapters/slack/adapter.py
echo "----"
sed -n '4520,4688p' src/chat_sdk/adapters/slack/adapter.pyRepository: Chinchill-AI/chat-sdk-python
Length of output: 34066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "<!channel|<!here|<!everyone|<!group" src/chat_sdk/adapters/slack -S || true
rg -n "@(channel|here|everyone|group)\b" src/chat_sdk/adapters/slack -S || true
rg -n "slack:user-by-name:|user-by-name" src/chat_sdk/adapters/slack/adapter.py -SRepository: Chinchill-AI/chat-sdk-python
Length of output: 226
🌐 Web query:
Slack does plain text "@channel" trigger mention or must it use "<!channel>"? Slack mrkdwn documentation
💡 Result:
When sending messages via the Slack API, you must use the <!channel> syntax to trigger an @channel mention [1][2][3]. While users typing in the standard Slack client UI can simply type "@channel" to trigger a notification, the Slack platform's mrkdwn formatting requires specific escape sequences for programmatic mentions [4][3]. In the mrkdwn syntax used by the Slack API, special system mentions are represented using the <!command> format [3]. The specific special commands for notifications are: - <!channel>: Notifies all members of a channel [4][1][2]. - <!here>: Notifies only the active members of a channel [4][1][2]. - <!everyone>: Notifies every non-guest member of a workspace (typically restricted to the #general channel) [4][1][2]. If you send the plain text string "@channel" via the API, it will generally render as literal text rather than triggering a mention, unless your integration or the destination channel has specific auto-parsing (link_names) enabled, which is not recommended for reliable programmatic behavior [5][3]. To ensure a mention is triggered, always use the bracketed <!channel> syntax [3].
Citations:
- 1: https://allmarkdowntools.com/slack-markdown
- 2: https://github.com/majiayu000/claude-skill-registry/blob/main/skills/documents/slack-markdown-formatter/SKILL.md
- 3: https://github.com/slackhq/slack-api-docs/blob/master/page_formatting.md
- 4: https://docs.slack.dev/messaging/formatting-message-text
- 5: https://github.com/ccheney/robust-skills/blob/main/skills/slack-mrkdwn/SKILL.md
Fix Slack mention formatting: don’t rewrite unresolved @name (or @channel/@here) into <@...>
SlackFormatConverter._finalize()and_node_to_mrkdwn()applyBARE_MENTION_REGEXto convert any leftover@(\w+)into<@\1>(29-32, 172-178, 191-194), even whenadapter.py::_resolve_outgoing_mentions()couldn’t resolve the name and already left it literal.- Slack expects
<@U...>for user mentions, and@channel/@here/@everyone/@groupmust use<!channel>/<!here>/<!everyone>/<!group>—not<@name>. - Restrict the bare-mention rewrite to Slack user-id-shaped tokens only (e.g.
@U...), and handle the special commands separately (or exclude them entirely).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/adapters/slack/format_converter.py` around lines 29 - 32,
BARE_MENTION_REGEX is too permissive and rewrites unresolved literals like
"`@alice`" or "`@channel`" into `<`@alice`>`; change it to only match Slack
user-ID-shaped tokens and/or exclude special commands: update BARE_MENTION_REGEX
to something like r"(?<![<\w])@([UW][A-Z0-9]+)" (match Slack IDs that start with
U or W and contain caps/numbers) and ensure the transformation code in
SlackFormatConverter._finalize() and SlackFormatConverter._node_to_mrkdwn() only
applies this regex; separately handle or explicitly exclude the special words
"channel", "here", "everyone", and "group" (or convert them to
`<!channel>`/`<!here>`/`<!everyone>`/`<!group>` where intended) and do not apply
the rewrite when adapter.py::_resolve_outgoing_mentions() left a literal
unresolved.
| self._name: str = "telegram" | ||
| self._lock_scope: LockScope = "channel" | ||
| self._persist_message_history: bool = True | ||
| self._persist_thread_history: bool = True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Root cause: protocol migration is incomplete across src/chat_sdk/adapters/telegram/adapter.py and src/chat_sdk/adapters/whatsapp/adapter.py.
Both adapters switched to persist_thread_history, but the active Adapter protocol still requires persist_message_history. Add a temporary alias property in both files (or atomically migrate the protocol and all call sites in the same PR) to restore type and contract compatibility.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/adapters/telegram/adapter.py` at line 597, The Adapter protocol
still expects persist_message_history but this adapter only defines
_persist_thread_history/persist_thread_history; add a temporary alias property
named persist_message_history on the Telegram adapter class that delegates to
the existing self._persist_thread_history (getter and setter) so the class
satisfies the old protocol; do the same change in the WhatsApp adapter (add a
persist_message_history property that reads/writes the existing
_persist_thread_history/persist_thread_history) or alternatively migrate the
protocol and all call sites together—ensure the alias uses the same backing
field (self._persist_thread_history) to maintain identical behavior.
Source: Pipeline failures
Comment on lines
+184
to
+197
| async def _fetch(url: str, *, method: str, headers: dict[str, str], body: str) -> tuple[int, str]: | ||
| """POST ``body`` to ``url`` and return ``(status, text)``. | ||
|
|
||
| Thin seam over aiohttp so tests can stub the network the way upstream | ||
| stubs global ``fetch``. aiohttp is an optional dependency, so it is | ||
| imported lazily (hazard #10); only http(s) URLs are supported, matching | ||
| the WHATWG ``fetch`` upstream relies on. | ||
| """ | ||
| import aiohttp | ||
|
|
||
| async with ( | ||
| aiohttp.ClientSession() as session, | ||
| session.request(method, url, data=body.encode("utf-8"), headers=headers) as response, | ||
| ): |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate callback URLs before issuing the request.
_fetch() sends whatever string was stored in callback_url; the only enforcement today is a docstring comment. That leaves this path open to SSRF against loopback/private targets or credential-bearing URLs whenever a callback destination is misconfigured or user-influenced. Reject unsafe URLs before calling aiohttp rather than relying on downstream failures.
As per coding guidelines, "Validate external URLs before making requests to prevent SSRF attacks."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/callback_url.py` around lines 184 - 197, The _fetch function
currently forwards any string URL to aiohttp, creating an SSRF risk; before
opening aiohttp.ClientSession/session.request validate the URL: parse the URL in
_fetch, ensure the scheme is http or https, reject URLs containing credentials
(username/password in netloc), resolve the host (via socket.getaddrinfo) and
verify none of the resolved IPs are loopback, private, link-local, multicast, or
otherwise non-public using the ipaddress module; if validation fails raise an
exception (e.g., ValueError) and do not call
aiohttp.ClientSession/session.request. Ensure these checks live at the top of
_fetch so unsafe URLs are rejected before any network call.
Source: Coding guidelines
Comment on lines
470
to
+475
| # Both `_state_adapter_instance` (old chat's state backend) | ||
| # and `_message_history` (old chat's cache) would route to | ||
| # and `_thread_history` (old chat's cache) would route to | ||
| # the previous context otherwise. | ||
| if adapter is not None or chat is not None: | ||
| channel._state_adapter_instance = None | ||
| channel._message_history = None | ||
| channel._thread_history = None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Rebind the history cache when rebinding the chat context.
This path clears _thread_history, but the later chat rebind only restores _state_adapter_instance. A ChannelImpl.from_json(..., chat=...) therefore loses cached messages() fallback and append behavior even when ChatConfig.thread_history is enabled.
Also applies to: 503-514
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/channel.py` around lines 470 - 475, When rebinding chat context
in ChannelImpl.from_json the code clears channel._thread_history but only
restores _state_adapter_instance when chat is provided, which loses the
messages() cache/fallback; change the logic so that when chat is passed in you
also rebind the history cache instead of unconditionally clearing it — e.g. if
chat is not None set channel._thread_history from chat.thread_history or
recreate it according to ChatConfig.thread_history (and mirror the same change
in the other identical block affecting lines around the second rebind) so
messages() behavior and append fallback are preserved.
Comment on lines
+1108
to
+1115
| self._logger.error( | ||
| "Modal callbackUrl POST failed", | ||
| {"callback_url": callback_url, "error": str(post_result.error)}, | ||
| ) | ||
| except Exception as error: # mirrors upstream's trailing .catch | ||
| self._logger.error( | ||
| "Modal callbackUrl POST failed", | ||
| {"callback_url": callback_url, "error": str(error)}, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact callback URLs in failure logs.
These log lines emit the full callback_url on transient POST failures. Callback URLs often carry tenant identifiers, shared secrets, or signed query params, so this turns a network blip into a log-based secret leak. Log a redacted form instead.
Also applies to: 1462-1468
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/chat.py` around lines 1108 - 1115, The log statements calling
self._logger.error that include the full callback_url should be changed to log a
redacted version of callback_url instead of the raw value; implement or call a
small sanitizer (e.g., redact_callback_url or similar) that strips or masks
query params and sensitive path segments and use that sanitized value in both
the Modal callback POST error branches shown (the block with
str(post_result.error) and the except Exception as error branch), and make the
same replacement for the analogous logger calls around lines 1462-1468 so no raw
callback_url is emitted in logs.
Comment on lines
+4362
to
+4367
| # _store_modal_context persists via a background task | ||
| await asyncio.sleep(0.02) | ||
|
|
||
| modal_context_keys = [k for k in state.cache if k.startswith("modal-context:")] | ||
| assert len(modal_context_keys) == 1 | ||
| context_id = modal_context_keys[0].split(":")[-1] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Replace fixed sleeps with deterministic async waits in new modal callback tests.
Line 4363 and Line 4496 rely on hardcoded sleeps to synchronize background work. These are prone to CI flakiness; prefer polling for the expected state/task completion with a timeout.
Also applies to: 4495-4497
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_chat_faithful.py` around lines 4362 - 4367, Replace the hardcoded
asyncio.sleep calls used to wait for background persistence of modal context
with a deterministic polling wait with a timeout: repeatedly check state.cache
for keys starting with "modal-context:" (as done in the current assertion) until
the expected count appears or a short timeout elapses, and use asyncio.wait_for
or an equivalent awaitable to fail deterministically on timeout; apply the same
polling pattern for both occurrences (the block around _store_modal_context and
the later block around the modal callback at the other location) so tests wait
reliably for the background task to complete.
…er Protocol CI's Lint & Type Check job failed on pyrefly (43 errors) while every other step was green: - persist_thread_history / persist_message_history were declared on the structural Adapter Protocol, making them REQUIRED members and breaking conformance for every adapter that satisfies the Protocol without extending BaseAdapter (the exact trap documented in PR #131). Moved to BaseAdapter only, with a NOTE on the Protocol; the SDK reads both flags via getattr. - MessengerAdapter (recovered #124 — the original cause of the failure on the recovery push) now defines get_user, mirroring BaseAdapter's ChatNotImplementedError contract; the real Graph-backed lookup remains issue #132. Also: _map_attachment_type returns the Attachment.type Literal, and the user-profile cache write is typed. - transcripts: retention_ms coerced to int (append_to_list ttl_ms contract; fractional milliseconds are meaningless). uv run pyrefly check: 0 errors (matches the origin/main baseline). https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64
Collaborator
Author
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e1cb88286
ℹ️ 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".
Comment on lines
+639
to
+642
| result = await thread.adapter.fetch_messages( | ||
| args["threadId"], | ||
| FetchOptions(limit=limit, cursor=cursor, direction=direction), | ||
| ) |
There was a problem hiding this comment.
For adapters that intentionally have no server-side history, this bypasses the SDK fallback that ThreadImpl.messages() provides. For example, WhatsApp's adapter returns an empty FetchResult and relies on persisted thread history; calling thread.adapter.fetch_messages() directly means an agent's fetchMessages tool sees an empty conversation even after the SDK has stored messages. Please route through the thread iterator/fallback or duplicate that fallback here.
Useful? React with 👍 / 👎.
Comment on lines
+561
to
+564
| "message": { | ||
| "mid": result["message_id"], | ||
| "is_echo": True, | ||
| }, |
There was a problem hiding this comment.
When sending a Messenger card/template, this synthetic echo only contains mid and is_echo, so _parse_messenger_message() caches it as a Message with empty text. Because fetch_messages() reads from this adapter cache first, a card the bot just sent is later read back as a blank message instead of its fallback/title text. Include the card fallback text or attachment payload in the cached event before parsing it.
Useful? React with 👍 / 👎.
Comment on lines
+109
to
+110
| if getattr(message, "markdown", None) is not None: | ||
| return {"markdown_text": self._finalize(message.markdown)} |
There was a problem hiding this comment.
This sends every PostableMarkdown through markdown_text unconditionally, but Slack documents markdown_text as limited to 12,000 characters and mutually exclusive with text (Slack chat.postMessage docs). A markdown response between 12k and the normal text limit now fails where the previous text/mrkdwn path would post, so please validate/split or fall back before returning markdown_text.
Useful? React with 👍 / 👎.
`_store_modal_context` was synchronous and scheduled the state write as a fire-and-forget task before `open_modal` was awaited. With a remote (Redis/Postgres) state backend a fast modal submit could race ahead of the write and miss the stored callbackUrl/channel, silently breaking modal callback POSTs and context restoration. Make `_store_modal_context` `async` and `await` it at both modal-opening call sites (slash + action paths) before `adapter.open_modal`, matching upstream where `openModal` awaits `storeModalContext` (chat.ts :1280/:1342/:1554). Remove the now-unnecessary `asyncio.sleep(0.02)` workaround in the modal-callback test; the context is persisted deterministically by the time `open_modal` returns. https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64
Convert the `config.X or <default/env>` truthiness patterns to `config.X if config.X is not None else <default/env>` for api_version (adapter) and the app_secret / page_access_token / verify_token env fallbacks (config helpers), per the TS->Python truthiness port rule. Mirrors upstream's `??` null-coalescing (index.ts:92/931). Behaviour is unchanged for real configs; an explicit value now wins over the fallback even if it is falsy. https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64
The `ChatInstance` Protocol annotated `process_message` as `-> None`, but the concrete `Chat.process_message` returns `asyncio.Task[None] | None` (it hands back the handler task so streaming callers can await it). Widen the Protocol annotation to match the implementation and upstream's interface-equals-impl typing. No runtime impact. https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64
| await self._invoke_handler(pat.handler, full_event) | ||
|
|
||
| if callback_url_task is not None: | ||
| await callback_url_task |
| message: Message | Callable[[], Awaitable[Message]], | ||
| options: WebhookOptions | None = None, | ||
| ) -> None: ... | ||
| ) -> asyncio.Task[None] | None: ... |
| # ``transcripts`` is not configured on the Chat instance — callers should | ||
| # check ``ChatConfig.transcripts`` if they need a no-raise guard. | ||
| @property | ||
| def transcripts(self) -> TranscriptsApi: ... |
| No-op if the Message has no ``user_key`` (resolver returned None). | ||
| - For AppendInput: ``options.user_key`` is required. | ||
| """ | ||
| ... |
|
|
||
| async def count(self, query: CountQuery) -> int: | ||
| """Total stored count for a user key.""" | ||
| ... |
|
|
||
| async def delete(self, target: DeleteTarget) -> DeleteResult: | ||
| """GDPR / DSR delete — wipes every stored message under the user key.""" | ||
| ... |
| raise ``max_per_user``; to fetch a different slice, narrow with | ||
| ``thread_id`` / ``platforms`` / ``roles``. | ||
| """ | ||
| ... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
The 0.4.29 completion train: stranded-PR recovery + the remaining 4.29 ports, finishing with the release cut. One logical item per commit.
Commits
main; cherry-picked verbatim, cf. previously-reviewed head1405271).fb8c50a).0.4.29, consolidated CHANGELOG (+ backfilled 0.4.27.1), README,lint.ymlre-pinned tochat@4.29.0, deferral rows (Teams Migrate Teams adapter to microsoft-teams-apps (official MS Python SDK) #93 → 0.4.30, tests-kit #470, GitHub/Linear client getters),examples/ai_tools_example.py.msg-history:storage prefix preserved).Plus a final commit: fidelity MAPPING extended to the new 4.29 core test files, baseline regenerated at
chat@4.29.0, and Known Non-Parity rows for the wave's divergences/deferrals.Validation (full gauntlet at
44c44fd)(6 skips = 3 pre-existing documented + 3
skipif-gated on #131.) GitHub CI green on44c44fd.What this completes
Every item in the 0.4.29 sync scope (#98 / PR #108) is ported or documented as deferred, the version is cut, and CI is strict-green at the
chat@4.29.0pin. The 4.30 wave is scoped in #135.https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64