feat(chat): message.subject + fetch_subject adapter hook#131
feat(chat): message.subject + fetch_subject adapter hook#131patrick-chinchill wants to merge 8 commits into
Conversation
Port the core message.subject feature from upstream eb5f94a (PR #459, "feat(chat): message.subject + adapter client access"). Tracks #98. Core type system + chat binding only: - MessageSubject dataclass (snake_case, ~10 fields) + MessageSubjectParty for the assignee/author { id, name } sub-objects, mirroring the upstream TS MessageSubject interface in packages/chat/src/types.ts. - Optional fetch_subject(raw) -> MessageSubject | None hook on BaseAdapter (default returns None). Declared on BaseAdapter (not the Adapter Protocol) to match how every other optional adapter hook is declared here, so adapters that don't implement it still satisfy Adapter for type-checking. - Message.subject async accessor backed by an identity-keyed, weakly-scoped adapter registry + cached resolution future (mirrors upstream's adapterMap WeakMap and _subjectPromise). Resolved subject is cached so a second `await message.subject` does not re-call fetch_subject; raising hooks resolve to None (mirrors upstream .catch(() => null)). - Chat registers the owning adapter at the single dispatch bind site (_dispatch_to_handlers), through which every dispatched message flows. WeakMap hashability decision: Message is a plain @DataClass (eq=True) and therefore unhashable, so weakref.WeakKeyDictionary[Message, Adapter] raises TypeError. Rather than change Message's equality contract (eq=False/frozen), we key a plain dict by id(message) (object identity, matching WeakMap semantics) and register a weakref.finalize callback per message that pops the entry on GC. weakref.ref works on a plain dataclass even though hash() does not, and the finalizer closes the id() reuse hole. Out of scope (follow-ups): GitHub + Linear adapter implementations of fetch_subject, which depend on those adapters exposing their native client (.octokit / .linear_client) — not yet present. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
|
Warning Review limit reached
More reviews will be available in 59 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the MessageSubject and MessageSubjectParty dataclasses to represent external subjects associated with messages, along with a weakly-scoped registry mapping Message objects to their owning Adapter to lazily resolve and cache these subjects. Feedback on the changes suggests preventing duplicate finalizer registrations when registering a message adapter multiple times, and reordering the adapter checks to ensure adapter is verified as not None before attempting to retrieve its fetch_subject attribute.
| key = id(message) | ||
| _message_adapter_map[key] = adapter | ||
|
|
||
| # Drop the entry when the message is GC'd. A zero-arg closure (rather than | ||
| # ``weakref.finalize(message, dict.pop, key, None)``) captures ``key`` and | ||
| # keeps the finalizer callable's type unambiguous for the type-checker. | ||
| # ``pop(key, None)`` is a no-op if the entry was already removed. | ||
| def _cleanup() -> None: | ||
| _message_adapter_map.pop(key, None) | ||
|
|
||
| weakref.finalize(message, _cleanup) |
There was a problem hiding this comment.
If set_message_adapter is called multiple times for the same message object, a new finalizer is registered on each call. This can lead to an accumulation of duplicate finalizer objects and closures, causing unnecessary memory overhead. Checking if the message is already registered before creating a new finalizer prevents this duplicate registration.
| key = id(message) | |
| _message_adapter_map[key] = adapter | |
| # Drop the entry when the message is GC'd. A zero-arg closure (rather than | |
| # ``weakref.finalize(message, dict.pop, key, None)``) captures ``key`` and | |
| # keeps the finalizer callable's type unambiguous for the type-checker. | |
| # ``pop(key, None)`` is a no-op if the entry was already removed. | |
| def _cleanup() -> None: | |
| _message_adapter_map.pop(key, None) | |
| weakref.finalize(message, _cleanup) | |
| key = id(message) | |
| already_registered = key in _message_adapter_map | |
| _message_adapter_map[key] = adapter | |
| if not already_registered: | |
| # Drop the entry when the message is GC'd. A zero-arg closure (rather than | |
| # weakref.finalize(message, dict.pop, key, None)) captures key and | |
| # keeps the finalizer callable's type unambiguous for the type-checker. | |
| # pop(key, None) is a no-op if the entry was already removed. | |
| def _cleanup() -> None: | |
| _message_adapter_map.pop(key, None) | |
| weakref.finalize(message, _cleanup) |
| adapter = _get_message_adapter(self) | ||
| fetch_subject = getattr(adapter, "fetch_subject", None) | ||
| if adapter is None or fetch_subject is None: | ||
| return None |
There was a problem hiding this comment.
When adapter is None, calling getattr(adapter, "fetch_subject", None) is unnecessary and can be confusing, even though getattr(None, ...) technically returns the default value in Python. It is cleaner, safer, and more idiomatic to perform the adapter is None check first before attempting to retrieve any attributes from it. Additionally, when checking for optional values that can be falsy but valid, ensure you use explicit is not None (or is None) checks instead of truthiness checks.
adapter = _get_message_adapter(self)
if adapter is None:
return None
fetch_subject = getattr(adapter, "fetch_subject", None)
if fetch_subject is None:
return NoneReferences
- 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.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…emini review) Address two Gemini review findings on PR #131: - set_message_adapter: register the weakref.finalize cleanup only on first registration for a given message identity. Re-registering the same live message now overwrites only the adapter value instead of accumulating redundant finalizers (re-dispatch / rehydrate / multiple handler passes). The id()-reuse hole stays closed: a GC'd predecessor's finalizer has already popped the entry, so a fresh finalizer is registered for the new object. - Message._resolve_subject: check adapter is None and return early before calling getattr(adapter, "fetch_subject", None) for clarity/robustness (behavior unchanged). Extends TestSetMessageAdapterWeakref: no-duplicate-finalizer (load-bearing, counts live weakref.finalize callbacks), re-registration adapter overwrite, and clean single GC cleanup after re-registration. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
CodeQL flagged test_types.py importing chat_sdk.types both as `import chat_sdk.types as types_module` and via `from chat_sdk.types import`. Its suggested "minimal fix" (delete the aliased import) would have broken the build — `types_module` is referenced in 8 places to reach the module internals `_message_adapter_map` and `_get_message_adapter`. Correct consolidation: import those two names in the existing `from` block and drop the alias, replacing the 8 `types_module.X` references with `X`. Pure import-style change, no behavior change; test_types.py still 40 passed. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
|
@codex review Generated by Claude Code |
![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: fd9293122f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
| if self._subject_future is None: | ||
| self._subject_future = asyncio.ensure_future(self._resolve_subject()) | ||
| return await self._subject_future |
There was a problem hiding this comment.
When a caller awaits message.subject through a timeout/cancelled handler, cancellation propagates into the cached _subject_future because it is awaited directly. For example, await asyncio.wait_for(message.subject, ...) on a slow fetch_subject cancels the shared task; every later await message.subject then raises CancelledError instead of retrying or resolving to None, so one timed-out handler permanently poisons that Message's subject accessor. Consider shielding the shared task or clearing the cache when it is cancelled.
Useful? React with 👍 / 👎.
…dex review) The cached `_subject_future` in `Message._subject` was awaited directly. A caller cancellation (e.g. `asyncio.wait_for(msg.subject, timeout=...)` firing) propagated into the shared future, marking it as cancelled and permanently poisoning the cache so every subsequent `await msg.subject` raised CancelledError. Wrap the cached-future await with `asyncio.shield(...)` so caller cancellation propagates to the caller while the inner task keeps running and the cache stays usable. Add a regression test that exercises the bug end-to-end (slow adapter + tight wait_for timeout + second awaiter).
|
@GHCodex 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: 66405461a0
ℹ️ 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".
| # Mirrors upstream's ``setMessageAdapter`` call at the dispatch bind | ||
| # site (packages/chat/src/chat.ts). Every dispatched message flows | ||
| # through here, so this is the single registration point. | ||
| set_message_adapter(message, adapter) |
There was a problem hiding this comment.
When queue/burst drains collapse multiple messages, only the latest message passed to _dispatch_to_handlers is registered here; the earlier messages are delivered to handlers as context.skipped without ever calling set_message_adapter (they were only enqueued while the lock was busy). In that scenario, await context.skipped[0].subject always resolves to None even when the adapter implements fetch_subject, so handlers cannot use the new subject accessor on skipped messages preserved by the concurrency context. Register the skipped messages in the context with the same adapter before invoking handlers.
Useful? React with 👍 / 👎.
Codex P2 finding on PR #131: when concurrency strategies collapse multiple messages (burst window, queue drain), only the latest ``message`` was bound to its owning adapter via ``set_message_adapter``. The earlier arrivals surfaced to handlers through ``context.skipped`` had no adapter registered, so ``await context.skipped[i].subject`` always resolved to ``None`` — even when the adapter implemented ``fetch_subject``. ``_dispatch_to_handlers`` is the single registration point for adapter binding; extend it to bind every message in ``context.skipped`` before invoking the handler chain. This is the only call site that ever passes a ``MessageContext``, so no other dispatch path needs touching. Test: end-to-end burst test that drives msg1 → msg2 → msg3 through ``burst`` strategy, attaches a ``fetch_subject`` hook to the mock adapter, and asserts ``await message.subject`` *and* both ``await context.skipped[i].subject`` calls resolve to their adapter- fetched ``MessageSubject`` (not ``None``). Reverting the fix reproduces the bug exactly. Full suite green (4067 passed). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Fixes the Lint & Type Check CI failure on PR #131 — ruff's line-length heuristic kept the two ``handle_incoming_message`` calls on a single line each (they fit). My audit-fix commit unnecessarily wrapped them. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
| await asyncio.sleep(0.005) | ||
| await chat.handle_incoming_message(adapter, "slack:C123:1234.5678", _mk("msg-burst-2", "Hey @slack-bot second")) | ||
| await chat.handle_incoming_message(adapter, "slack:C123:1234.5678", _mk("msg-burst-3", "Hey @slack-bot third")) | ||
| await task |
This reverts commit 583643a.
Fixes the Lint & Type Check CI failure on PR #131 — ruff's line-length heuristic kept the two ``handle_incoming_message`` calls on a single line each (they fit). My audit-fix commit unnecessarily wrapped them. Re-applied after revert 66ef156 (which had to undo the stray pickup of unrelated messenger adapter files that bled in from a checkout to ``origin/main`` during local mypy investigation). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
2 participants
Ports the core
message.subjectfeature from upstreameb5f94a(vercel/chat PR #459,feat(chat): message.subject + adapter client access,chat@4.29.0).Worklist item P0-1. Tracks #98.
What's ported (CORE type system + chat binding only)
MessageSubjectdataclass insrc/chat_sdk/types.py— snake_case mirror of upstream's TSMessageSubjectinterface (id,type,raw,assignee,author,description,labels,status,title,url). Plus a smallMessageSubjectPartydataclass for the{ id, name }shape upstream uses inline forassignee/author. Typed dataclass, not a raw dict.fetch_subject(raw) -> MessageSubject | Noneadapter hook onBaseAdapter(default returnsNone). See the Protocol-placement note below.Message.subjectasync accessor backed by upstream's WeakMap +_subjectPromisepattern:fetch_subject;Nonewhen no adapter is registered, the adapter has no hook, the hook returnsNone, or the hook raises (failures swallowed, mirroring upstream.catch(() => null));await message.subjectdoes NOT re-callfetch_subject; concurrent awaits share one in-flight resolution.Chat._dispatch_to_handlersregisters the owning adapter for every dispatched message viaset_message_adapter. This is the single convergence point all dispatch paths (subscribed / mention / DM / pattern / queue / debounce / burst / rehydrated) flow through — upstream callssetMessageAdapterin 3 scattered spots (handleIncomingMessage+ tworehydrateMessagebranches), all of which reach this method here.WeakMap hashability decision (the load-bearing call)
Upstream uses
const adapterMap = new WeakMap<Message, Adapter>()— weak on the Message key, strong on the Adapter value, keyed by object identity.Messagein this repo is a plain@dataclass(eq=True), which makes instances unhashable (verified:hash(msg)raisesTypeError, andweakref.WeakKeyDictionary[Message, Adapter]raisesTypeError: unhashable type: 'Message').weakref.ref(msg)does work on a plain dataclass.Chosen approach: a plain
dict[int, Adapter]keyed byid(message)+ a per-messageweakref.finalizecallback that pops the entry on GC.id()), matchingWeakMap.id()-reuse hole — the entry is removed before CPython can recycle the id.Messagetoeq=False/frozen=True, which would have restored an identity hash but silently changedMessage's public equality contract (andWeakValueDictionaryis the wrong shape — it would weakly hold the long-lived Adapter, not the Message).The cache (
_subject_future) is a dataclass field withinit=False, compare=False, repr=False, so it stays out of__init__, equality, andrepr. The accessor returns a fresh coroutine each access (soawait msg.subjectworks repeatedly andasyncio.run(msg.subject)is valid) while sharing a singleensure_future-scheduled resolution underneath.Protocol placement note
fetch_subjectis declared onBaseAdapteronly, NOT on theAdapterProtocol. Adding it to the structuralProtocolmakes it a required attribute and breaks every adapter that doesn't define it (pyrefly flagged 36 such errors). This matches how every other optional hook here (stream,open_dm,rehydrate_attachment,get_channel_visibility, ...) is declared — onBaseAdapter, not the Protocol.Message.subjectreads the hook viagetattr(adapter, "fetch_subject", None), so presence is fully optional regardless of base class.Follow-ups (out of scope)
fetch_subject— depends on the adapter exposing its native.octokitclient (doesn't exist yet).fetch_subject— depends on the adapter exposing its native.linear_client(doesn't exist yet).Both are the "adapter client access" half of upstream #459 and are deferred until those adapters expose their native clients.
Tests
tests/test_types.py:TestMessageSubjectDataclass— minimal required fields, all fields incl. nestedMessageSubjectParty.TestMessageSubject(mirrors upstreammessage.test.tsdescribe("subject")): None when no adapter; None when adapter has nofetch_subject; returns adapter result; awaited twice -> called once; null result cached; concurrent access shares one call; raising hook -> None; raw payload passed through.TestSetMessageAdapterWeakref— registration doesn't crash on unhashable Message; entry removed on GC; distinct messages map to distinct adapters.tests/test_chat_faithful.py:TestSubjectBinding— handler resolvesmessage.subjectvia the adapter hook after real dispatch;Nonewhen the adapter has no hook.Validation
uv run ruff check src/ tests/— All checks passeduv run ruff format --check src/ tests/— 195 files already formatteduv run python scripts/audit_test_quality.py— Hard failures: 0uv run pytest tests/ -q— 4062 passed, 3 skipped (no regressions)uv run pyrefly check— 0 errors (matchesorigin/mainbaseline; no new errors)scripts/verify_test_fidelity.py --strictis not runnable in this environment (the upstreamchat@4.29.0checkout movedai.test.tsintoai/, so the script's 4.26.0-pinned path resolution fails identically on untouchedorigin/main);message.test.tsis not in its MAPPING scope. CHANGELOG /pyproject.tomlintentionally untouched (cut PR handles versioning).https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code