Skip to content

feat(messenger): adapter — webhook, Graph API, send/stream (PR 2 of 2)#124

Merged
patrick-chinchill merged 7 commits into
claude/port-messenger-adapter-scaffoldingfrom
claude/port-messenger-adapter
May 30, 2026
Merged

feat(messenger): adapter — webhook, Graph API, send/stream (PR 2 of 2)#124
patrick-chinchill merged 7 commits into
claude/port-messenger-adapter-scaffoldingfrom
claude/port-messenger-adapter

Conversation

@patrick-chinchill
Copy link
Copy Markdown
Collaborator

@patrick-chinchill patrick-chinchill commented May 29, 2026

Summary

Port of packages/adapter-messenger/src/index.ts from
vercel/chat#461PR 2 of 2 of the
Messenger (Meta) adapter port.

Stacked on #118 (claude/port-messenger-adapter-scaffolding), which added
the scaffolding (types.py, format_converter.py, cards.py). Base this PR
off #118; please merge #118 first.

Tracking: #98 (4.29 sync). Design: #110.

What's ported

  • src/chat_sdk/adapters/messenger/adapter.pyMessengerAdapter +
    create_messenger_adapter factory.
    • Webhook routing: GET subscription verification challenge, POST event
      notifications.
    • X-Hub-Signature-256 verification (HMAC-SHA256, App Secret).
    • Graph API client (aiohttp, shared session, function-local import so the
      dependency stays lazy).
    • Send paths: text, Generic Template card, Button Template card, text
      fallback for unsupported cards.
    • Buffered stream() (Messenger can't edit, so chunks accumulate and ship
      as one message).
    • Inbound event handlers: messages, echoes, postbacks (with chat:
      callback-data decoding), reactions, delivery & read confirmations.
    • Attachment extraction with lazy fetch_data download closure.
    • Local message cache (Messenger has no history API) backing
      fetch_messages / fetch_message, including upstream's
      (timestamp, sequence-suffix) ordering.
    • User-profile cache; fetch_thread / fetch_channel_info hydrate
      display names from the Graph API.
    • Typed Graph-API error mapping → AdapterRateLimitError /
      AuthenticationError / ResourceNotFoundError / ValidationError /
      NetworkError.
  • src/chat_sdk/adapters/messenger/__init__.py — re-exports
    MessengerAdapter, create_messenger_adapter, and the API constants
    (GRAPH_API_BASE, DEFAULT_API_VERSION, MESSENGER_MESSAGE_LIMIT).
  • tests/test_messenger_webhook.py (69 tests).
  • tests/test_messenger_api.py (67 tests).

Design open questions

#110 lists three open questions. Q2 (callback-data passthrough) was already
locked in by PR 1's decode_messenger_callback_data tests.

Q1 — init-failure behavior

Resolved by improving on the sibling WhatsAppAdapter: missing required
credentials raise ValidationError from both the create_messenger_adapter
factory and the MessengerAdapter constructor (the constructor goes
through the same resolved_* env-fallback chain on MessengerAdapterConfig).
WhatsAppAdapter's constructor does no validation (direct misconstruction
raises TypeError, not ValidationError), so Messenger is strictly stricter
while staying consistent with the Meta-family's factory-level checks, and
surfaces config errors loudly at startup rather than at first webhook call.
Pinned by TestFactory::test_constructor_also_raises_on_missing_credentials
and the three test_missing_*_raises cases.

Q3 — signature verification

Resolved by pinning upstream's contract exactly: HMAC-SHA256 over the
raw request body with the App Secret as the key, header parsed as
sha256=. No swappable verifier hook was introduced — a Slack-style
webhook_verifier indirection would diverge from Meta's wire protocol
(single secret, single algo) without offsetting benefit. Flagged in the
adapter docstring as a possible future divergence if multi-tenant or
custom-verifier use cases land. Pinned by TestVerifySignature and the
TestWebhookPostSignatureGate suite (valid / invalid / missing / wrong
algo / missing hash / bad hex / wrong secret / replay-with-modified-body).

Divergences / concerns

None significant. Two minor implementation choices to flag:

  1. The constructor enforces credentials presence (improving on
    WhatsAppAdapter, which does not) in addition to the factory. Upstream's
    TS adapter constructor takes the resolved credentials as plain strings;
    we accept the config dataclass and apply the same env fallbacks, so the
    check lives on both paths.
  2. The User-Profile fetch uses the existing aiohttp session with the
    access_token query param (matching upstream). No SSRF allowlist is
    needed for fetchMessage-style attachment downloads in this PR because
    the attachment URLs come straight from Meta's webhook payload — upstream
    has no such allowlist either. (WhatsAppAdapter has one for download_media
    because the URL is a separately-fetched redirect; Messenger inlines the URL.)

Validation

uv run ruff check src/ tests/         All checks passed!
uv run ruff format --check src/ tests/  204 files already formatted
uv run python scripts/audit_test_quality.py  Hard failures: 0
uv run pytest tests/ -q                4235 passed, 3 skipped

136 new tests (69 webhook + 67 API); no regressions vs. #118 baseline.

Test plan

  • Verify CI passes on Python 3.12.
  • Smoke test: instantiate create_messenger_adapter with explicit
    creds and confirm it constructs without raising.
  • Confirm # Divergence-candidate (see #110) breadcrumbs in PR 1 still
    resolve correctly when this stack lands.

https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj


Generated by Claude Code

…at#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.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 951c98fd-3925-44fc-8ddb-6fcf5806bc93

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the Messenger (Meta) adapter for the chat SDK, including webhook routing, signature verification, message sending/streaming, and local caching, along with comprehensive unit tests. The reviewer's feedback focuses on enhancing robustness and correctness: casting URLs to strings before parsing, performing signature verification on raw request bytes instead of decoded strings, validating that the fetched user profile is a dictionary before caching, and updating the request body extractor to return raw bytes to prevent encoding issues.

Comment on lines +289 to +290
url = getattr(request, "url", "")
parsed = urlparse(url)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If request.url is a non-string object (such as Starlette's URL or yarl's URL object), passing it directly to urlparse can raise a TypeError or AttributeError. Casting url to str before parsing ensures compatibility across different web frameworks.

Suggested change
url = getattr(request, "url", "")
parsed = urlparse(url)
url = getattr(request, "url", "")
parsed = urlparse(str(url))

Comment on lines +304 to +337
def _verify_signature(self, body: str, signature: str | None) -> bool:
"""Verify the ``X-Hub-Signature-256`` header (HMAC-SHA256, App Secret).

Format: ``sha256=<hex>``. Returns ``False`` on any malformed input.
Q3 (see #110): we keep upstream's hard-wired ``app_secret``-based
HMAC scheme. A swappable verifier (the pattern used by the Slack
adapter) would diverge from upstream's contract and isn't justified
for a single-secret Meta integration; flagged as a possible future
divergence but not introduced here.
"""
if not signature:
return False

# Header is ``algo=hash``. Reject anything that isn't sha256 hex.
parts = signature.split("=", 1)
if len(parts) != 2:
return False
algo, hash_hex = parts
if algo != "sha256" or not hash_hex:
return False

try:
computed_hex = hmac.new(
self._app_secret.encode("utf-8"),
body.encode("utf-8"),
hashlib.sha256,
).hexdigest()
# Compare hex strings directly. The signature is presented as a
# lowercase hex string by Meta; ``hexdigest()`` is lowercase.
return hmac.compare_digest(hash_hex, computed_hex)
except Exception:
self._logger.warn("Failed to verify Messenger webhook signature")
return False

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The signature verification is performed on body.encode('utf-8'). However, body was already decoded to a string in _get_request_body. If the original request body contained invalid UTF-8 characters or used a different encoding, the decoding and subsequent re-encoding can alter the raw bytes (e.g., replacing invalid bytes with \ufffd), leading to signature verification failures. For cryptographic correctness and robustness, signature verification should be performed on the raw, unmodified request bytes.

    def _verify_signature(self, body: bytes, signature: str | None) -> bool:
        """Verify the 'X-Hub-Signature-256' header (HMAC-SHA256, App Secret).

        Format: 'sha256=<hex>'. Returns False on any malformed input.
        Q3 (see #110): we keep upstream's hard-wired 'app_secret'-based
        HMAC scheme. A swappable verifier (the pattern used by the Slack
        adapter) would diverge from upstream's contract and isn't justified
        for a single-secret Meta integration; flagged as a possible future
        divergence but not introduced here.
        """
        if not signature:
            return False

        # Header is 'algo=hash'. Reject anything that isn't sha256 hex.
        parts = signature.split("=", 1)
        if len(parts) != 2:
            return False
        algo, hash_hex = parts
        if algo != "sha256" or not hash_hex:
            return False

        try:
            computed_hex = hmac.new(
                self._app_secret.encode("utf-8"),
                body,
                hashlib.sha256,
            ).hexdigest()
            # Compare hex strings directly. The signature is presented as a
            # lowercase hex string by Meta; 'hexdigest()' is lowercase.
            return hmac.compare_digest(hash_hex, computed_hex)
        except Exception:
            self._logger.warn("Failed to verify Messenger webhook signature")
            return False

Comment on lines +851 to +862
try:
profile = await self._graph_api_fetch(
user_id,
method="GET",
query_params={"fields": "first_name,last_name,profile_pic"},
)
self._user_profile_cache[user_id] = profile
return cast(MessengerUserProfile, profile)
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}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If _graph_api_fetch succeeds but returns a non-dictionary response (such as None or an empty list), calling profile.get in _profile_display_name will raise an AttributeError. We should validate that profile is indeed a dictionary before caching and returning it, otherwise fall back to the minimal profile. Additionally, when using cast to narrow a Mapping[str, Any] into a TypedDict like MessengerUserProfile, ensure that the TypedDict is defined as a superset (total=False) to admit all possible keys.

Suggested change
try:
profile = await self._graph_api_fetch(
user_id,
method="GET",
query_params={"fields": "first_name,last_name,profile_pic"},
)
self._user_profile_cache[user_id] = profile
return cast(MessengerUserProfile, profile)
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}
try:
profile = await self._graph_api_fetch(
user_id,
method="GET",
query_params={"fields": "first_name,last_name,profile_pic"},
)
if isinstance(profile, dict):
self._user_profile_cache[user_id] = profile
return cast(MessengerUserProfile, profile)
raise ValidationError("messenger", "Invalid profile response")
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}
References
  1. When using cast to narrow a Mapping[str, Any] into a TypedDict, ensure that the TypedDict is a superset (total=False) that admits all possible keys, especially when performing duck-typing with .get() for mode detection.

Comment on lines +1036 to +1053
async def _get_request_body(request: Any) -> str:
"""Extract body text from a request object (sync or async)."""
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
return text_attr.decode("utf-8") if isinstance(text_attr, (bytes, bytearray)) else str(text_attr)
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 body.decode("utf-8")
return str(body)
return ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To support cryptographically correct signature verification on raw request bytes, _get_request_body should return bytes instead of str. This avoids any encoding/decoding issues (such as replacement characters for invalid UTF-8) that can cause legitimate webhooks to fail signature verification. Ensure that we check for optional values that can be falsy but valid using is not None checks.

    @staticmethod
    async def _get_request_body(request: Any) -> bytes:
        """Extract raw body bytes from a request object (sync or async)."""
        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""
References
  1. When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use is not None instead of a truthiness check to avoid silently ignoring them.

claude added 2 commits May 29, 2026 21:05
…ni 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
@patrick-chinchill patrick-chinchill marked this pull request as ready for review May 30, 2026 00:11
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c976de38ba

ℹ️ 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 +792 to +795
Attachment(
type=self._map_attachment_type(attachment.get("type", "")),
url=url,
fetch_data=self._make_attachment_downloader(url),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve attachment fetchers across queued delivery

When a Messenger message with an attachment is processed under queue, debounce, or burst concurrency, Chat may enqueue the Message; serialization drops the fetch_data callable, and the rehydrate path only restores it if the adapter provides rehydrate_attachment. Because these attachments only keep the URL/fetch closure here and Messenger does not add a rehydrate hook, handlers that receive the dequeued message see attachment.fetch_data is None and can no longer download the file. Store download metadata here and implement rehydrate_attachment to rebuild the closure from it.

Useful? React with 👍 / 👎.

claude added 2 commits May 30, 2026 00:22
…ety (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)
…gression 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).
Copy link
Copy Markdown
Collaborator Author

@codex review


Generated by Claude Code

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 956872a9ca

ℹ️ 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".

def parse_message(self, raw: MessengerRawMessage) -> Message:
"""Parse a raw messaging event into a normalized ``Message``."""
sender = raw.get("sender") or {}
thread_id = self.encode_thread_id(MessengerThreadId(recipient_id=sender.get("id", "")))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use the recipient as the thread for echo events

When parse_message() is given a Messenger echo event, sender.id is the Page ID while recipient.id is the user's PSID—the same case _handle_echo() handles separately. With the current line, parsing or replaying a raw echoed message caches it under messenger:<page id>, so fetch_messages('messenger:<user PSID>') misses the bot message and the normalized message points at the wrong DM. Branch on message.is_echo here before encoding the thread ID.

Useful? React with 👍 / 👎.

… (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
Copy link
Copy Markdown
Collaborator Author

@codex review


Generated by Claude Code

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cdb38809b7

ℹ️ 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".


async def _get_http_session(self) -> Any:
"""Return the shared aiohttp session, creating it lazily on first use."""
import aiohttp
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Declare aiohttp for the Messenger adapter

This lazy import is on the normal runtime path for initialize, sending, profile lookup, and attachment downloads, but the package has dependencies = [] and the optional-dependencies list contains extras for the other aiohttp-based adapters without a messenger extra. In a base install that enables only the new Messenger adapter, these methods will raise ModuleNotFoundError: aiohttp as soon as the adapter talks to Meta; please add a Messenger runtime extra such as messenger = ["aiohttp>=3.9"] or otherwise declare the dependency.

Useful? React with 👍 / 👎.

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
Copy link
Copy Markdown
Collaborator Author

@codex review


Generated by Claude Code

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 14052713c1

ℹ️ 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 +663 to +667
async def fetch_thread(self, thread_id: str) -> ThreadInfo:
"""Fetch thread info, hydrated with the user profile when available."""
recipient_id = self._resolve_thread_id(thread_id).recipient_id
profile = await self._fetch_user_profile(recipient_id)
display_name = self._profile_display_name(profile)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Implement Messenger get_user

This profile lookup is only exposed through fetch_thread/fetch_channel_info; MessengerAdapter never implements the required Adapter.get_user API, unlike the other adapters. When Messenger callers try the common user-lookup path with a PSID (for example await adapter.get_user(psid)), they get AttributeError/unsupported behavior even though _fetch_user_profile already has the Graph API data needed to return UserInfo with name/avatar fields.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Acknowledging this finding but declining for this PR — with a follow-up.

Correction on the specific claim: adapter.get_user(psid) does not raise AttributeError. BaseAdapter.get_user (src/chat_sdk/types.py:1416) raises ChatNotImplementedError, which Chat.get_user translates to a ChatError("does not support get_user"). So callers see a clean "unsupported" surface, not a crash.

Scope rationale: #124 ports vercel/chat's adapter-messenger/src/index.ts (PR #461). Upstream's TS adapter doesn't implement getUser either — grep -n "getUser" /tmp/vercel-chat/packages/adapter-messenger/src/index.ts finds only the unrelated chat.getUserName() call. Adding get_user here would be a deliberate Python-side divergence from upstream, exceeding this PR's brief.

The observation is real though: all 8 sibling Python adapters (slack/telegram/whatsapp/teams/google_chat/discord/github/linear) implement get_user, making Messenger the lone holdout. _fetch_user_profile already has the Graph API data. So this is worth doing — just not on this PR.

Follow-up: I'll open an issue to add MessengerAdapter.get_user (wrapping _fetch_user_profileUserInfo) as a deliberate Python-port enhancement, in the same category as the fetch_subject follow-ups already flagged on #131 (GitHub/Linear native client exposure).


Generated by Claude Code

@patrick-chinchill patrick-chinchill merged commit 3ad373a into claude/port-messenger-adapter-scaffolding May 30, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants