chore(types): pyrefly 213 → 0 errors + fix 3 latent runtime bugs#49
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughType and runtime-safety changes: lock_scope typed to LockScope, request-body helpers made duck-typed and awaitable-aware, stream posting uses typed Postable wrappers, explicit casts replace type-ignores, handler dispatch supports sync/async, and pyrefly/CI baseline usage removed. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 updates the SDK's type definitions and adapter implementations to improve type safety and consistency. Key changes include introducing the LockScope type, making thread and channel optional in event types, and refactoring the WhatsApp inbound message structure to support the reserved from keyword. Additionally, static analysis baselines and import configurations were updated. Feedback was provided regarding the use of type casting in the GitHub adapter, suggesting a more robust approach to avoid potential runtime issues.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/chat_sdk/adapters/slack/adapter.py (1)
679-679: Use explicitNonefallback for header selection.Line 679 uses
orchaining. Insrc/, prefer explicitis not Nonefallback to avoid truthiness-driven behavior differences.♻️ Suggested refactor
- content_type = headers.get("content-type") or headers.get("Content-Type") or "" + lower_ct = headers.get("content-type") + upper_ct = headers.get("Content-Type") if lower_ct is None else None + content_type = lower_ct if lower_ct is not None else (upper_ct if upper_ct is not None else "")As per coding guidelines "
src/**/*.py: Usex if x is not None else defaultinstead ofx or defaultto avoid truthiness traps when porting from TypeScript."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chat_sdk/adapters/slack/adapter.py` at line 679, The code uses truthiness chaining to pick a header value (content_type = headers.get("content-type") or headers.get("Content-Type") or "") which can mis-handle falsy but valid header values; update the selection to use explicit None checks: retrieve headers.get("content-type") and if that is not None use it, else use headers.get("Content-Type") if that is not None, otherwise fall back to ""—apply this change where content_type and headers are used (e.g., in the Slack adapter request handling function/method that contains content_type).tests/test_coverage_gaps.py (1)
432-432: AssertPostableRawdirectly in these stream tests.Checking only
.rawstill lets any ad-hoc object with that attribute pass. Comparing againstPostableRaw(raw=...)would lock in the regression this PR is fixing.✅ Stronger assertion
from chat_sdk.types import ( Author, FetchOptions, Message, MessageMetadata, + PostableRaw, RawMessage, ) @@ - assert call_args.args[1].raw == "hello world" + assert call_args.args[1] == PostableRaw(raw="hello world") @@ - assert adapter.post_message.call_args.args[1].raw == "chunk" + assert adapter.post_message.call_args.args[1] == PostableRaw(raw="chunk")As per coding guidelines,
tests/**/*.py: Every test must fail when the code is wrong.Also applies to: 466-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_coverage_gaps.py` at line 432, Replace the loose attribute check with a direct PostableRaw object equality: instead of asserting call_args.args[1].raw == "hello world", assert call_args.args[1] == PostableRaw(raw="hello world") (and add/import PostableRaw if missing); do the same replacement for the similar assertion at the other occurrence (line ~466) so the tests assert the full PostableRaw value rather than any object with a .raw attribute.src/chat_sdk/adapters/linear/adapter.py (1)
354-361: Use explicitNonechecks for alias fallback fields.These
a or bchains treat""as missing and silently switch to the alias/default. In this file the repo rule is to preserve falsy-but-valid values withis not Nonechecks.♻️ Example pattern
- issue_id = cast("str | None", data.get("issueId") or data.get("issue_id")) + issue_id = cast("str | None", data.get("issueId")) + if issue_id is None: + issue_id = cast("str | None", data.get("issue_id"))Please apply the same pattern to the other changed alias lookups here (
userId/user_id,createdAt/created_at,updatedAt/updated_at). As per coding guidelines,src/**/*.py: Usex if x is not None else defaultinstead ofx or defaultto avoid truthiness traps when porting from TypeScript.Also applies to: 410-423, 857-861
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chat_sdk/adapters/linear/adapter.py` around lines 354 - 361, The alias fallbacks use "a or b" which treats falsy-but-valid values (like empty string) as missing; update all alias lookups (e.g., issue_id, parent_id/root_comment_id, user_id, created_at, updated_at) to use explicit None checks with the pattern "x if x is not None else y" instead of "x or y" so falsy values are preserved—locate the occurrences in src/chat_sdk/adapters/linear/adapter.py around the blocks that assign issue_id, parent_id/root_comment_id and the other alias pairs (userId/user_id, createdAt/created_at, updatedAt/updated_at) and replace each fallback expression accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chat_sdk/adapters/github/adapter.py`:
- Around line 1092-1098: The current body-handling branch treats request.body as
a raw value and will return the string representation of a callable/coroutine
instead of invoking it; mirror the same callable/awaitable guard used for
text_attr: check body = getattr(request, "body", None), if callable(body) call
it, then if inspect.isawaitable(result) await it, then decode bytes or cast to
str and return; use the same inspect.isawaitable logic and decoding behavior as
used for text_attr to ensure coroutines are awaited and bytes are decoded.
In `@src/chat_sdk/adapters/google_chat/adapter.py`:
- Around line 773-779: The fallback that builds body from raw_body currently
turns a callable request.body into its string representation, causing json.loads
to fail; update the logic around raw_body in the adapter so that if raw_body is
callable you call it (and if the result is awaitable await it) before treating
it as bytes or str—then decode bytes to UTF-8 as done elsewhere (mirror the
text() path behavior) and assign to body; operate on the same variables
(raw_body, body, request) and ensure awaitable results are awaited to avoid
unawaited coroutine bugs.
In `@src/chat_sdk/adapters/linear/adapter.py`:
- Around line 986-994: The code currently calls request.body when it's a
callable but doesn't await async callables, so async callables return coroutine
objects; change the callable handling in the block that starts with body =
getattr(request, "body", None) to call body() and if the result is
awaitable/coroutine await it before further processing; then proceed with the
existing logic that checks for .read (raw_read) and uses
inspect.iscoroutinefunction/raw_read to await reads as needed. Specifically
update the callable branch for request.body to handle awaiting the result (use
inspect.isawaitable or inspect.iscoroutine) so functions like request.body() and
async read paths yield actual bytes/str for signature verification and JSON
parsing.
In `@src/chat_sdk/adapters/slack/adapter.py`:
- Around line 660-667: The webhook body handling is missing a callable check for
request.body causing bound method objects to be stringified; update the
raw/request.body branch in the slack adapter so it mirrors the text_attr logic:
if getattr(request, "body", None) is callable, call it before awaiting, then if
the result is a coroutine or future await it, and finally decode bytes/bytearray
to UTF-8 or str(raw) into the body variable; ensure you modify the raw / body
handling around the existing raw = getattr(request, "body", None) code path so
signature verification receives the actual payload.
In `@src/chat_sdk/adapters/teams/adapter.py`:
- Around line 1773-1783: _in _get_request_body_, the code calls request.body()
and body.read() without awaiting their returned coroutine objects, causing
unawaited coroutine returns; change the logic to detect and await awaitable
results (use inspect.isawaitable()) when calling request.body() and when
invoking body.read(), and preserve the existing branches for bytes/str handling
and callable detection so that any coroutine returned by request.body() or
body.read() is awaited before decoding or casting.
In `@src/chat_sdk/chat.py`:
- Around line 1798-1800: Handlers are still being awaited unconditionally in
several places (e.g. _handle_slash_command_event, _handle_action_event,
_handle_reaction_event, process_modal_submit and the assistant/app-home/member
handler loops), so synchronous handlers will raise; add a helper like async def
_maybe_await(self, result: Any) -> Any that awaits only if
inspect.isawaitable(result) and returns the result otherwise, then replace
direct awaits on handler calls throughout this file with awaiting the helper
(i.e., call _maybe_await(handler(...)) whenever you currently do await
handler(...) or do result = handler(...); await result) to ensure all handler
invocations consistently support sync and async handlers.
---
Nitpick comments:
In `@src/chat_sdk/adapters/linear/adapter.py`:
- Around line 354-361: The alias fallbacks use "a or b" which treats
falsy-but-valid values (like empty string) as missing; update all alias lookups
(e.g., issue_id, parent_id/root_comment_id, user_id, created_at, updated_at) to
use explicit None checks with the pattern "x if x is not None else y" instead of
"x or y" so falsy values are preserved—locate the occurrences in
src/chat_sdk/adapters/linear/adapter.py around the blocks that assign issue_id,
parent_id/root_comment_id and the other alias pairs (userId/user_id,
createdAt/created_at, updatedAt/updated_at) and replace each fallback expression
accordingly.
In `@src/chat_sdk/adapters/slack/adapter.py`:
- Line 679: The code uses truthiness chaining to pick a header value
(content_type = headers.get("content-type") or headers.get("Content-Type") or
"") which can mis-handle falsy but valid header values; update the selection to
use explicit None checks: retrieve headers.get("content-type") and if that is
not None use it, else use headers.get("Content-Type") if that is not None,
otherwise fall back to ""—apply this change where content_type and headers are
used (e.g., in the Slack adapter request handling function/method that contains
content_type).
In `@tests/test_coverage_gaps.py`:
- Line 432: Replace the loose attribute check with a direct PostableRaw object
equality: instead of asserting call_args.args[1].raw == "hello world", assert
call_args.args[1] == PostableRaw(raw="hello world") (and add/import PostableRaw
if missing); do the same replacement for the similar assertion at the other
occurrence (line ~466) so the tests assert the full PostableRaw value rather
than any object with a .raw attribute.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 07619c82-0bba-4200-a3ff-a8d86cbc2f69
📒 Files selected for processing (21)
.pyrefly-baseline.jsonsrc/chat_sdk/adapters/discord/adapter.pysrc/chat_sdk/adapters/discord/cards.pysrc/chat_sdk/adapters/github/adapter.pysrc/chat_sdk/adapters/github/cards.pysrc/chat_sdk/adapters/google_chat/adapter.pysrc/chat_sdk/adapters/google_chat/cards.pysrc/chat_sdk/adapters/google_chat/workspace_events.pysrc/chat_sdk/adapters/linear/adapter.pysrc/chat_sdk/adapters/linear/cards.pysrc/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/adapters/teams/adapter.pysrc/chat_sdk/adapters/telegram/adapter.pysrc/chat_sdk/adapters/whatsapp/adapter.pysrc/chat_sdk/adapters/whatsapp/types.pysrc/chat_sdk/channel.pysrc/chat_sdk/chat.pysrc/chat_sdk/thread.pysrc/chat_sdk/types.pytests/test_coverage_gaps.pytests/test_critical_fixes.py
✅ Files skipped from review due to trivial changes (3)
- tests/test_critical_fixes.py
- src/chat_sdk/adapters/linear/cards.py
- src/chat_sdk/channel.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/chat_sdk/adapters/whatsapp/types.py
- src/chat_sdk/types.py
- src/chat_sdk/adapters/whatsapp/adapter.py
- src/chat_sdk/adapters/discord/adapter.py
- src/chat_sdk/adapters/telegram/adapter.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/chat_sdk/adapters/slack/adapter.py (1)
660-664:⚠️ Potential issue | 🔴 CriticalCritical: invoke/await callable
request.bodybefore decoding.Current fallback can stringify a bound method instead of reading payload bytes, breaking webhook parsing/signature flow in frameworks exposing
body()methods.Suggested fix
- raw = getattr(request, "body", None) - if raw is not None: - if asyncio.iscoroutine(raw) or asyncio.isfuture(raw): - raw = await raw - body = raw.decode("utf-8") if isinstance(raw, (bytes, bytearray)) else str(raw) + raw_attr = getattr(request, "body", None) + if raw_attr is not None: + raw = raw_attr() if callable(raw_attr) else raw_attr + if inspect.isawaitable(raw): + raw = await raw + body = raw.decode("utf-8") if isinstance(raw, (bytes, bytearray)) else str(raw)#!/bin/bash # Verify body branch shape and reproduce callable-body behavior. rg -n "raw = getattr\(request, \"body\", None\)|iscoroutine\(raw\)|isfuture\(raw\)" src/chat_sdk/adapters/slack/adapter.py -A6 -B2 python - <<'PY' import inspect, asyncio class Req: async def body(self): return b'payload' r = Req() raw = getattr(r, "body", None) print("callable:", callable(raw)) print("stringified:", str(raw)) coro = raw() print("awaitable_after_call:", inspect.isawaitable(coro)) print("resolved:", asyncio.run(coro).decode()) PYAs per coding guidelines, "Every async call must be awaited. Unawaited coroutines silently return truthy objects. Use AsyncMock (not MagicMock) in tests to surface these issues."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chat_sdk/adapters/slack/adapter.py` around lines 660 - 664, The current body-extraction logic (in the Slack adapter code around the raw/body handling where raw = getattr(request, "body", None) and subsequent asyncio.iscoroutine/isfuture checks) fails to call request.body when it's a callable, causing bound methods to be stringified instead of reading payload bytes; update the flow to first check if raw is callable and if so call it (raw = raw()), then if the result is awaitable await it (or if asyncio.isfuture), and finally decode bytes to a UTF-8 string into body; ensure this change touches the same variables (raw, body) and preserves existing coroutine/future checks so webhook signature parsing receives the actual payload.src/chat_sdk/adapters/google_chat/adapter.py (1)
773-776:⚠️ Potential issue | 🟠 MajorHandle callable
request.bodyin webhook fallback path.If
request.bodyis a method/coroutine, this branch stringifies the callable instead of reading payload bytes, which can breakjson.loads().Suggested fix
- raw_body = getattr(request, "body", None) - if raw_body is not None: - body = raw_body.decode("utf-8") if isinstance(raw_body, bytes) else str(raw_body) + raw_body_attr = getattr(request, "body", None) + if raw_body_attr is not None: + raw_body = raw_body_attr() if callable(raw_body_attr) else raw_body_attr + if inspect.isawaitable(raw_body): + raw_body = await raw_body + body = raw_body.decode("utf-8") if isinstance(raw_body, (bytes, bytearray)) else str(raw_body)#!/bin/bash # Verify current webhook body branch and reproduce callable-body behavior. rg -n "raw_body = getattr\(request, \"body\", None\)|body = raw_body\.decode" src/chat_sdk/adapters/google_chat/adapter.py -A4 -B2 python - <<'PY' import inspect, asyncio class Req: async def body(self): return b'{"ok": true}' r = Req() raw = getattr(r, "body", None) print("callable:", callable(raw)) print("stringified:", str(raw)) coro = raw() print("awaitable_after_call:", inspect.isawaitable(coro)) print("decoded_payload:", asyncio.run(coro).decode("utf-8")) PYAs per coding guidelines, "Every async call must be awaited. Unawaited coroutines silently return truthy objects. Use AsyncMock (not MagicMock) in tests to surface these issues."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chat_sdk/adapters/google_chat/adapter.py` around lines 773 - 776, The current fallback reads raw_body = getattr(request, "body", None) and treats it as bytes/str, but if request.body is a callable (sync or async) it stringifies the callable instead of reading payload bytes; update the logic around raw_body (the getattr check and the branch that sets body) to: detect if raw_body is callable, call it, await the result if it is an awaitable, then decode bytes or str as before (use inspect.isawaitable/asyncio.iscoroutine to detect awaitables); ensure this happens before the isinstance(raw_body, bytes) check so json.loads() receives the actual JSON payload rather than a stringified function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/lint.yml:
- Around line 60-63: Add a CI step to run verify_test_fidelity.py alongside the
existing validation steps (e.g., the audit_test_quality.py step and the Pyrefly
step) so every upstream TypeScript test is checked for a matching Python test;
locate the lint.yml workflow where the "Pyrefly type check" (id: pyrefly) and
the audit_test_quality step are defined and insert a new job/step named like
"Verify test fidelity" that executes verify_test_fidelity.py (ensure it follows
the same runner/context as the other validation checks and respects any
continue-on-error or failure semantics used by audit_test_quality.py).
In `@pyproject.toml`:
- Around line 83-84: Update the stale baseline wording in the pyproject.toml
comment block that references the checker target so it matches the new "zero
errors" policy: replace any mention of baseline-based behavior or legacy
baseline instructions with a concise note that the target is "zero errors" and
keep the example command (uv run pyrefly check) for running the check; edit the
comment text near the existing "Target: zero errors. Run: uv run pyrefly check"
header to remove ambiguity and reflect the zero-error policy.
In `@src/chat_sdk/types.py`:
- Around line 1344-1352: The implementation Chat.handle_incoming_message
currently has a mismatched signature vs the protocol
ChatInstance.handle_incoming_message; update the Chat.handle_incoming_message
method to accept the same parameters (adapter: Adapter, thread_id: str, message:
Message, options: WebhookOptions | None = None, *, is_subscribed: bool = False)
or add a catch-all **kwargs that preserves the protocol defaults so it conforms
to ChatInstance; ensure the method accepts (and if unused, explicitly ignores)
options and is_subscribed to restore protocol conformance.
---
Duplicate comments:
In `@src/chat_sdk/adapters/google_chat/adapter.py`:
- Around line 773-776: The current fallback reads raw_body = getattr(request,
"body", None) and treats it as bytes/str, but if request.body is a callable
(sync or async) it stringifies the callable instead of reading payload bytes;
update the logic around raw_body (the getattr check and the branch that sets
body) to: detect if raw_body is callable, call it, await the result if it is an
awaitable, then decode bytes or str as before (use
inspect.isawaitable/asyncio.iscoroutine to detect awaitables); ensure this
happens before the isinstance(raw_body, bytes) check so json.loads() receives
the actual JSON payload rather than a stringified function.
In `@src/chat_sdk/adapters/slack/adapter.py`:
- Around line 660-664: The current body-extraction logic (in the Slack adapter
code around the raw/body handling where raw = getattr(request, "body", None) and
subsequent asyncio.iscoroutine/isfuture checks) fails to call request.body when
it's a callable, causing bound methods to be stringified instead of reading
payload bytes; update the flow to first check if raw is callable and if so call
it (raw = raw()), then if the result is awaitable await it (or if
asyncio.isfuture), and finally decode bytes to a UTF-8 string into body; ensure
this change touches the same variables (raw, body) and preserves existing
coroutine/future checks so webhook signature parsing receives the actual
payload.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 134a727c-de65-4813-8c1d-1f15f8c24495
📒 Files selected for processing (17)
.github/workflows/lint.yml.pyrefly-baseline.jsonpyproject.tomlsrc/chat_sdk/adapters/discord/adapter.pysrc/chat_sdk/adapters/google_chat/adapter.pysrc/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/adapters/teams/adapter.pysrc/chat_sdk/adapters/teams/cards.pysrc/chat_sdk/adapters/telegram/adapter.pysrc/chat_sdk/adapters/whatsapp/adapter.pysrc/chat_sdk/adapters/whatsapp/cards.pysrc/chat_sdk/chat.pysrc/chat_sdk/shared/base_format_converter.pysrc/chat_sdk/shared/card_utils.pysrc/chat_sdk/shared/mock_adapter.pysrc/chat_sdk/thread.pysrc/chat_sdk/types.py
✅ Files skipped from review due to trivial changes (1)
- src/chat_sdk/adapters/whatsapp/cards.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/chat_sdk/chat.py
- src/chat_sdk/adapters/discord/adapter.py
- src/chat_sdk/adapters/whatsapp/adapter.py
- src/chat_sdk/adapters/teams/adapter.py
5 tasks
patrick-chinchill
added a commit
that referenced
this pull request
Apr 23, 2026
Addresses CodeRabbit critical/major findings + github-code-quality nits:
**Critical: `request.body` duck-typing symmetry** (5 adapters).
Previously only `request.text` got the `getattr + isawaitable` treatment;
`request.body` was still vulnerable to the same async-method case (Flask,
FastAPI body-as-coroutine, etc.). Would return `<coroutine object ...>`
stringified, breaking JSON parsing and HMAC verification.
Applied to:
- src/chat_sdk/adapters/linear/adapter.py
- src/chat_sdk/adapters/discord/adapter.py
- src/chat_sdk/adapters/teams/adapter.py
- src/chat_sdk/adapters/slack/adapter.py
- src/chat_sdk/adapters/github/adapter.py
- src/chat_sdk/adapters/google_chat/adapter.py
- src/chat_sdk/adapters/whatsapp/adapter.py
(Telegram already had the correct pattern.)
**Major: sync-handler support was partial.** Previously `_run_handlers`,
`_direct_message_handlers`, and pattern handlers used `inspect.isawaitable`
guards, but `_handle_action_event`, `_handle_slash_command_event`,
`_handle_reaction_event`, `process_modal_submit`, `process_modal_close`,
and the assistant/app-home/member-joined handler loops still `await`ed
unconditionally. A sync handler on any of those paths would crash with
`TypeError`.
Fix: added `Chat._invoke_handler(handler, *args)` static helper that
calls the handler, narrows with `inspect.isawaitable`, and returns the
result (so modal-submit handlers that return a `ModalResponse` still
work). Routed all 11 remaining handler-invocation sites through it.
**Minor: Unused import false positives** (CodeQL). `ModalElement` in
slack/adapter.py and `WhatsAppCardResultInteractive` /
`WhatsAppCardResultText` in whatsapp/adapter.py were used only in
string-literal `cast("ModalElement", ...)` forms. Switched to direct
`cast(ModalElement, ...)` so the import is syntactically used — pyrefly
still resolves the cast and CodeQL no longer flags.
**Minor: Protocol signature**: dropped extra `options` / `is_subscribed`
params from `ChatInstance.handle_incoming_message` protocol stub —
matches `Chat.handle_incoming_message` implementation exactly.
Validation:
- `uv run pyrefly check` — 0 errors
- `uv run ruff check src/ tests/ scripts/` — clean
- `uv run pytest tests/ --tb=short -q` — 3469 passed, 2 skipped
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/chat_sdk/adapters/teams/adapter.py (1)
1773-1786:⚠️ Potential issue | 🟠 MajorAwait the result of
body.read(), not just coroutine functions.
inspect.iscoroutinefunction(raw_read)still misses syncread()methods that return awaitables, so this path can hand a coroutine object back to the caller and break body parsing.🔧 Suggested fix
if hasattr(body, "read"): raw_read = body.read - raw = await raw_read() if inspect.iscoroutinefunction(raw_read) else raw_read() + raw = raw_read() + if inspect.isawaitable(raw): + raw = await raw return raw.decode("utf-8") if isinstance(raw, bytes) else rawRun this to verify why the current gate is still too narrow:
#!/bin/bash sed -n '1773,1786p' src/chat_sdk/adapters/teams/adapter.py python - <<'PY' import inspect import asyncio class Body: def read(self): async def inner(): return b'{"ok": true}' return inner() body = Body() raw_read = body.read print("iscoroutinefunction(raw_read) =", inspect.iscoroutinefunction(raw_read)) raw = raw_read() print("isawaitable(raw) =", inspect.isawaitable(raw)) print("returned_type =", type(raw).__name__) async def main(): print("awaited_value =", await raw) asyncio.run(main()) PYAs per coding guidelines, "Every async call must be awaited. Unawaited coroutines silently return truthy objects. Use AsyncMock (not MagicMock) in tests to surface these issues."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chat_sdk/adapters/teams/adapter.py` around lines 1773 - 1786, The current logic checks inspect.iscoroutinefunction(raw_read) before deciding to await body.read(), which misses sync read() methods that return awaitables; update the body-reading branch (around getattr(request, "body", None) and the raw_read handling) to always call raw_read() into raw and then if inspect.isawaitable(raw) await raw (instead of testing raw_read with iscoroutinefunction); then proceed to decode raw into text as before.
🧹 Nitpick comments (1)
src/chat_sdk/adapters/discord/adapter.py (1)
963-975: Consider documenting the unusedstatusparameter.The
statusparameter is part of the interface signature but Discord's typing endpoint doesn't support custom status messages. A brief docstring note would clarify this for future maintainers.📝 Suggested documentation
async def start_typing(self, thread_id: str, status: str | None = None) -> None: - """Start typing indicator in a Discord channel or thread.""" + """Start typing indicator in a Discord channel or thread. + + Args: + thread_id: The thread or channel to show typing in. + status: Ignored; Discord's typing indicator doesn't support custom status. + """ decoded = self.decode_thread_id(thread_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chat_sdk/adapters/discord/adapter.py` around lines 963 - 975, The start_typing method's status parameter is unused because Discord's typing endpoint doesn't accept a custom message; update the start_typing(docstring) to note that status is intentionally ignored/unsupported and mention that the parameter exists to satisfy the interface. Reference the start_typing method and the status parameter in the docstring so future maintainers know why status is unused and not a bug.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chat_sdk/adapters/google_chat/adapter.py`:
- Around line 764-785: The request-body parsing in adapter.py currently only
uses request.text when it's callable, skipping populated non-callable text
attributes; update the logic around text_attr/request.text so that when
text_attr is not None you first check if callable and call/await it, otherwise
if text_attr is bytes decode to UTF-8 or convert to str and assign to body
(mirroring how raw_body is handled), then fall back to
raw_body/dict/str(request) only if text_attr is None; adjust the branches that
reference text_attr, raw_body, body, getattr(request, "text", None) and
inspect.isawaitable to implement this behavior.
---
Duplicate comments:
In `@src/chat_sdk/adapters/teams/adapter.py`:
- Around line 1773-1786: The current logic checks
inspect.iscoroutinefunction(raw_read) before deciding to await body.read(),
which misses sync read() methods that return awaitables; update the body-reading
branch (around getattr(request, "body", None) and the raw_read handling) to
always call raw_read() into raw and then if inspect.isawaitable(raw) await raw
(instead of testing raw_read with iscoroutinefunction); then proceed to decode
raw into text as before.
---
Nitpick comments:
In `@src/chat_sdk/adapters/discord/adapter.py`:
- Around line 963-975: The start_typing method's status parameter is unused
because Discord's typing endpoint doesn't accept a custom message; update the
start_typing(docstring) to note that status is intentionally ignored/unsupported
and mention that the parameter exists to satisfy the interface. Reference the
start_typing method and the status parameter in the docstring so future
maintainers know why status is unused and not a bug.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a69a241-36d3-4223-afd8-5862723d1f32
📒 Files selected for processing (9)
src/chat_sdk/adapters/discord/adapter.pysrc/chat_sdk/adapters/github/adapter.pysrc/chat_sdk/adapters/google_chat/adapter.pysrc/chat_sdk/adapters/linear/adapter.pysrc/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/adapters/teams/adapter.pysrc/chat_sdk/adapters/whatsapp/adapter.pysrc/chat_sdk/chat.pysrc/chat_sdk/types.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/chat_sdk/adapters/github/adapter.py
- src/chat_sdk/adapters/linear/adapter.py
patrick-chinchill
added a commit
that referenced
this pull request
Apr 23, 2026
…line PR #48 is based off main, which doesn't yet have PR #49's replace-imports-with-any submodule wildcards (slack_sdk.*). The TYPE_CHECKING import of AsyncWebClient from slack_sdk.web.async_client therefore tripped missing-import on this branch. Reverted to 'current_client -> Any' with a docstring capturing the actual runtime type. Once PR #49 merges, we can promote to a typed forward ref in a follow-up (or in the next release).
patrick-chinchill
added a commit
that referenced
this pull request
Apr 23, 2026
CodeRabbit MAJOR (google_chat/adapter.py:785): non-callable
`request.text` was skipped by the `and callable(text_attr)` gate,
falling through to the body/str(request) path and turning valid
string-text attributes into unparseable output. The bug affects 5
adapters: github, google_chat, whatsapp, slack, telegram. Discord,
Linear, and Teams already had the correct pattern.
Fix: move the callable check INSIDE the `if text_attr is not None`
branch so populated string/bytes attributes are consumed.
```python
# Before (buggy):
if text_attr is not None and callable(text_attr):
result = text_attr()
body = str(await result if inspect.isawaitable(result) else result)
else:
# Fall through to body — wrong if text_attr was a string!
...
# After (correct):
if text_attr is not None:
if callable(text_attr):
result = text_attr()
text_attr = await result if inspect.isawaitable(result) else result
body = text_attr.decode("utf-8") if isinstance(text_attr, bytes) else str(text_attr)
else:
# body fallback only when text is actually absent
...
```
Also addresses github-code-quality "Statement has no effect" (types.py:1349):
collapse the multi-line `) -> None: ...` stub for
`ChatInstance.handle_incoming_message` to single-line to match the
other protocol stubs and silence CodeQL's "naked ellipsis" false
positive.
Validation:
- 3469 tests pass
- ruff check + format clean
- pyrefly 0 errors
patrick-chinchill
added a commit
that referenced
this pull request
Apr 23, 2026
Self-review findings on PR #49: 1. **bytearray inconsistency**: github / google_chat / whatsapp / telegram text-attr path used `isinstance(text_attr, bytes)`, missing bytearray. Slack's already had `(bytes, bytearray)`. Unified all four to match. 2. **Zero regression coverage** for the non-callable `request.text` string-attribute path — the fix's raison d'être shipped untested. Added `tests/test_request_body_extraction.py` with 7 cases × 3 static-method adapters (github, telegram, whatsapp) = 21 tests. Cases covered per adapter: - `text` as plain string attribute (Django-style) - `text` as bytes attribute (decoded UTF-8) - `text` as bytearray attribute (bytes/bytearray symmetry) - `text` as sync method (Flask-style) - `text` as async method (aiohttp/FastAPI) - `body` as async method (FastAPI `async def body(self)`) - `body` as bytes property (Django-style) Discord/Teams/Slack/Linear/Google Chat have instance methods (not static) — their extractor requires adapter construction which needs real platform config. Their paths are exercised via existing integration tests; the 3 static adapters cover the same patterns with less setup overhead and catch the specific historical bugs (non-callable text gate, sync text() crash, async body() coroutine stringification, bytes/bytearray asymmetry). Validation: - 3490 tests pass (21 new) - ruff check + format clean - pyrefly 0 errors
patrick-chinchill
changed the title
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/chat_sdk/adapters/slack/adapter.py (1)
690-691: Lower-caseContent-Typeand avoid theorfallback here.Header-name lookup is case-insensitive now, but the media type token is too. A value like
Application/X-WWW-Form-Urlencodedstill falls into the JSON path, and theorchain also reintroduces the truthiness trap the repo guideline calls out.🩹 Suggested fix
- content_type = headers.get("content-type") or headers.get("Content-Type") or "" + content_type_header = headers.get("content-type") + if content_type_header is None: + content_type_header = headers.get("Content-Type") + content_type = str(content_type_header).lower() if content_type_header is not None else "" if "application/x-www-form-urlencoded" in content_type:As per coding guidelines, "Use
x if x is not None else defaultinstead ofx or defaultto avoid truthiness traps when porting from TypeScript."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chat_sdk/adapters/slack/adapter.py` around lines 690 - 691, Replace the current headers lookup and comparison so you perform a case-insensitive header-name lookup and a case-insensitive media-type check without using the truthy "or" fallback: locate where you assign content_type from headers (the variable content_type and the headers dict) and instead fetch the content-type value via a case-normalized lookup (e.g., find the header key by lowercasing keys) then set media = content_type if content_type is not None else "" and compare using media.lower() (e.g., check "application/x-www-form-urlencoded" in media.lower()) so mixed-case header names and media types are handled correctly and no "x or default" truthiness trap is used.src/chat_sdk/adapters/linear/adapter.py (1)
345-362: Normalize Linear webhook keys once at the boundary.This keeps carrying both
issueId/issue_id,parentId/parent_id,userId/user_id, etc. through internal code paths. Please normalize the webhook payload to snake_case once and keep the rest of the adapter snake_case-only; otherwise mixed-key lookups will keep leaking into internal logic and types.As per coding guidelines, "Convert camelCase dispatch keys to snake_case internally, but maintain camelCase at serialization boundaries."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chat_sdk/adapters/linear/adapter.py` around lines 345 - 362, Normalize the incoming Linear webhook payload to snake_case at the adapter boundary before any internal processing: add a small helper (e.g. normalize_keys_to_snake_case) and call it on payload, then set data = cast("LinearCommentData", normalized_payload.get("data", {})) and actor = cast("LinearWebhookActor", normalized_payload.get("actor", {})) so all subsequent code (the issue_id/parent_id lookups, thread_id computation, self.encode_thread_id, and logging) uses snake_case keys only; update any references in this method to read user_id, issue_id, parent_id, etc., not camelCase variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chat_sdk/adapters/discord/adapter.py`:
- Around line 1406-1433: The _get_request_body method incompletely handles
byte-backed attributes and incorrectly decides when to await read(); update it
so all branches (body, text, data) consistently decode bytes and bytearray to
UTF-8 (including callable text() results and callable data() if present), and
change the read handling to call raw = raw_read() and await it only if
inspect.isawaitable(raw) (not based on inspect.iscoroutinefunction(raw_read));
also ensure callable text/data results are awaited when awaitable before
decoding or str()-ifying so downstream code that expects a str won't receive a
byte representation.
In `@src/chat_sdk/adapters/linear/adapter.py`:
- Around line 981-1008: The helper _get_request_body mishandles bytes: ensure
callable/text paths decode bytes and bytearray (not stringify), treat
non-callable text as possibly bytes-like and decode, include bytearray in data
handling, and always await any awaitable return values (use inspect.isawaitable
on the call result, not inspect.iscoroutinefunction on the method).
Specifically, in _get_request_body update the body.read handling to call
raw_read() then await if inspect.isawaitable(raw) before decoding, change
callable text() path to await the result if awaitable and decode
bytes/bytearray, decode non-callable text when it's bytes-like, and handle
bytearray in the data branch; reference symbols: _get_request_body, body,
raw_read/read, text/text_attr, data.
In `@tests/test_request_body_extraction.py`:
- Around line 199-224: Add a new parametrized test that exercises the sync
callable body branch by calling each extractor with
_SyncCallableBodyRequest(b'{"ok": true}') and asserting the returned string
equals '{"ok": true}'; mirror the structure of test_handles_async_callable_body
but use _SyncCallableBodyRequest to ensure the callable-but-non-awaitable body
path is covered and will fail if regressions occur (reference
_SyncCallableBodyRequest and the extractor parameter from _adapters()).
- Around line 133-154: The try/except blocks around importing adapters are too
broad and swallow genuine import errors inside adapter modules; instead, probe
the specific third-party dependency for each adapter before importing the
adapter and only skip when that dependency import fails. For each adapter case
(GitHubAdapter._get_request_body, TelegramAdapter._get_request_body,
WhatsAppAdapter._get_request_body) first attempt to import the external package
the adapter requires (e.g., the github client, telegram lib, whatsapp/twilio
lib) and only if that import raises ImportError then skip appending to result;
otherwise import the adapter normally so internal adapter import errors surface
and fail the test. Ensure you still append to result on success and keep the
same result list usage.
---
Nitpick comments:
In `@src/chat_sdk/adapters/linear/adapter.py`:
- Around line 345-362: Normalize the incoming Linear webhook payload to
snake_case at the adapter boundary before any internal processing: add a small
helper (e.g. normalize_keys_to_snake_case) and call it on payload, then set data
= cast("LinearCommentData", normalized_payload.get("data", {})) and actor =
cast("LinearWebhookActor", normalized_payload.get("actor", {})) so all
subsequent code (the issue_id/parent_id lookups, thread_id computation,
self.encode_thread_id, and logging) uses snake_case keys only; update any
references in this method to read user_id, issue_id, parent_id, etc., not
camelCase variants.
In `@src/chat_sdk/adapters/slack/adapter.py`:
- Around line 690-691: Replace the current headers lookup and comparison so you
perform a case-insensitive header-name lookup and a case-insensitive media-type
check without using the truthy "or" fallback: locate where you assign
content_type from headers (the variable content_type and the headers dict) and
instead fetch the content-type value via a case-normalized lookup (e.g., find
the header key by lowercasing keys) then set media = content_type if
content_type is not None else "" and compare using media.lower() (e.g., check
"application/x-www-form-urlencoded" in media.lower()) so mixed-case header names
and media types are handled correctly and no "x or default" truthiness trap is
used.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d79f49c-dbab-49ce-9140-71b9f5e85260
📒 Files selected for processing (11)
pyproject.tomlsrc/chat_sdk/adapters/discord/adapter.pysrc/chat_sdk/adapters/github/adapter.pysrc/chat_sdk/adapters/google_chat/adapter.pysrc/chat_sdk/adapters/linear/adapter.pysrc/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/adapters/teams/adapter.pysrc/chat_sdk/adapters/telegram/adapter.pysrc/chat_sdk/adapters/whatsapp/adapter.pysrc/chat_sdk/types.pytests/test_request_body_extraction.py
🚧 Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- src/chat_sdk/adapters/teams/adapter.py
- src/chat_sdk/adapters/telegram/adapter.py
Collaborator
Author
patrick-chinchill
left a comment
patrick-chinchill
left a comment
There was a problem hiding this comment.
Re-reviewed current head a242e19. This looks good to merge. Focused verification passed: uv run pytest tests/test_request_body_extraction.py tests/test_coverage_gaps.py tests/test_critical_fixes.py -q (54 passed). I couldn't submit an approval because GitHub does not allow approving your own pull request.
Tackles the largest clusters of real type issues without resorting to
`# type: ignore`:
**Protocol conformance (lock_scope)** — all 8 adapters typed `lock_scope`
as `str | None` instead of `LockScope | None` (`Literal['channel',
'thread'] | None`), so every adapter failed the `Adapter` protocol,
cascading through every `adapter=self` call site (ActionEvent,
ReactionEvent, etc.). Fixed at the source + propagated to internal
`_lock_scope` fields on stateful adapters.
**Protocol conformance (parameter names)** — protocols compare parameter
names, not just types. Several adapters named params `_thread_id` /
`_status` / `_emoji` to signal "unused", which broke structural typing
against `Adapter`. Renamed to match the protocol.
**Optional dep submodule imports** — `replace-imports-with-any` only
matched top-level (`slack_sdk`, `nacl`, `redis`), so submodule imports
(`slack_sdk.web.async_client`, `nacl.signing`, `redis.asyncio`) still
flagged. Added `.*` wildcards for each optional dep.
**TypedDict keys that are Python keywords** — `WhatsAppInboundMessage`
had `from_` but the runtime JSON key is `"from"`, so every
`message["from"]` access flagged bad-typed-dict-key. Converted to
functional-form TypedDict so the Python source can spell `"from"` as
a string key.
**TypedDict union duck-typing** — GitHub adapter's init detects auth
mode via `config.get("token")` / `.get("app_id")`, but the public
config type is a union of 4 auth-specific TypedDicts where no single
variant has all keys. Added an internal superset TypedDict
(`_GitHubAdapterConfigInternal`) cast at the init boundary so the
duck-typing stays type-safe without fragmenting the public API.
**Event types accepting None** — `ReactionEvent.thread` and
`SlashCommandEvent.channel` were typed as required, but adapters
legitimately don't always have a Thread/Channel at dispatch time (e.g.
Slack reactions on channel-root messages, Discord DM slash commands).
Relaxed to `Thread | None` / `Channel | None` with a docstring note
for consumers.
Baseline regenerated (`uv run pyrefly check --baseline ...
--update-baseline`) — reduced from 213 → 130 entries, all remaining
entries are now on the local repo paths (previous baseline had
`/home/user/...` artifacts from the cloud agent env that never
matched locally).
Validation:
- `uv run pyrefly check --baseline=.pyrefly-baseline.json` → 0 new
- `uv run ruff check src/ tests/ scripts/` → clean
- `uv run pytest tests/` → 3359 passed, 11 skipped
… of 38%)
Continues the clean-up on fix/pyrefly-zero-errors. Combined with the
prior commit, the total reduction this branch is 205 → 21 errors (90%).
**NoReturn annotations for error re-raisers** (−22 errors): Slack's
`_handle_slack_error` and Google Chat's `_handle_google_chat_error`
always raise but were typed `-> None`/`-> Any`, so every `try/except`
that propagated through them showed as "missing return". Annotated
both as `NoReturn` to let pyrefly skip the downstream paths.
**Framework-agnostic `request` duck-typing** (−14 not-async errors):
`hasattr(request, "text") and callable(request.text)` narrows `Any`
to `object`, making `await request.text()` fail type-checking. Swapped
to `getattr(request, "text", None)` (preserves `Any`) plus
`inspect.isawaitable` for the sync-vs-async branch — same runtime
behavior, but properly typed. Applied across all 7 adapters that
duck-type aiohttp / FastAPI / Flask requests. Bonus: also fixes the
latent runtime bug where a sync `request.text` (Flask) would have
raised `TypeError: object is not awaitable`.
**Handler sync/async union** (−3 not-async errors): handler types are
`Callable[..., Awaitable[None] | None]` but `_run_handlers`,
`_direct_message_handlers`, and pattern handlers all `await`
unconditionally. Switched to `inspect.isawaitable` guards — same
runtime fix, same pragmatism about the TS port's `Promise<void> |
void` convention.
**Protocol conformance**:
- `_ChatSingleton` now `Protocol` (was plain class), so `Chat`
structurally satisfies it without an import cycle
- `ThreadImpl.channel` returns `Channel` (protocol) not `ChannelImpl`
so `ThreadImpl` passes the `Thread` protocol
- `set_state` param renamed `new_state` → `state` to match protocol
- `_ChannelImplConfigForChat` removed (duplicate of
`_ChannelImplConfigWithAdapter`)
- `ChatInstance` protocol now declares `handle_incoming_message`
(Discord adapter was calling it but the protocol didn't advertise it)
**TypedDict unions duck-typed via `cast`**: many adapters `.get()`
against TypedDict unions where the key only exists on some variants.
Added narrow `cast(...)` calls where the code has already
runtime-verified the variant via a `type` check.
**Card-element helpers**: helpers typed `dict[str, Any]` but callers
passed `CardChild` (union of TypedDicts). Added `cast("dict[str, Any]",
child)` at the `_render_child` / `_convert_child_to_widgets` branches
— the variant is already narrowed via `child.get("type")`.
**Misc**:
- `WhatsAppRawMessage.message` tightened `dict[str, Any]` → `WhatsAppInboundMessage`
- `GitHubAdapter._parse_issue_comment` / `_parse_review_comment`
param typed `GitHubRepository` instead of `dict[str, Any]`
- `_get_attachment_type` returns `Literal["audio","file","image","video"]`
- `is_encrypted_token_data` isn't a TypeGuard, so added an `assert`
to collapse the `Optional` before the attribute access
Baseline regenerated: 213 → 21 entries (90% of original errors gone).
`pyrefly check --baseline=.pyrefly-baseline.json` shows 0 new errors.
Validation:
- `uv run pytest tests/ -q` → 3359 passed, 11 skipped
- `uv run ruff check src/ tests/ scripts/` → clean
- `uv run ruff format --check src/ tests/ scripts/` → clean
- `uv run pyrefly check --baseline=.pyrefly-baseline.json` → 0 new
…oved)
**Upstream check**: verified each change against vercel/chat TS source.
All changes in this PR are now either upstream-aligned or Python-specific
internals (request duck-typing, Optional protocol declarations, etc.).
**chinchill-api impact**: zero — chinchill-api doesn't import chat_sdk
yet (uses slack-bolt directly). No consumer-facing breaking changes.
Fixes in this commit:
- Batch-cast at TypedDict-union access boundaries (card_utils.py,
linear/cards.py, discord/cards.py, base_format_converter.py)
- `Attachment.type` Literal typing for Teams / Discord
`_get_attachment_type` helpers
- Telegram `TelegramReactionType.get(...)` casts (reaction_key +
reaction_to_emoji_value)
- Telegram `from_user` cast after `.get("from_user") or .get("from")`
- WhatsApp `WhatsAppCardResult` union discrimination via cast
- WhatsApp adapter / Linear adapter / Google Chat adapter: replaced
`{"markdown": ...}` dict literals with `PostableMarkdown(...)` to match
the `AdapterPostableMessage` union
- GitHub `_dispatch_event`: cast at variant narrowing boundaries
- Mock adapter: `post_channel_message` returns channel_id as thread_id
(was None, violating RawMessage.thread_id: str)
- `_ChannelImplConfigForChat` removed (duplicate of
`_ChannelImplConfigWithAdapter`) — matches upstream TS which only has
`ChannelImplConfigWithAdapter | ChannelImplConfigLazy`
- `thread.py`: `getattr(chunk, "text", "")` for MarkdownTextChunk
narrowing (upstream handles via TS literal-union narrowing)
**Reverted prior divergences after upstream check:**
- `ReactionEvent.thread: Thread` (was `Thread | None`) — upstream has it
required. Adapters pass `thread=None` at construction with a
`# pyrefly: ignore[bad-argument-type]` + comment; Chat re-wraps with
a real Thread before dispatching to handlers (same pattern as upstream
TS `Omit<ReactionEvent, "thread">`).
- `SlashCommandEvent.channel: Channel` (was `Channel | None`) — same
pattern, upstream-aligned.
- `set_state(new_state=...)` (was renamed to `state`) — upstream is
`setState(newState: ...)`, so Python param stays `new_state`.
Protocol signature also reverted to `new_state`.
Baseline file removed entirely — CI now enforces zero type errors.
`.github/workflows/lint.yml` updated to drop baseline references.
Validation:
- `uv run pyrefly check` → **0 errors**
- `uv run ruff check src/ tests/ scripts/` → clean
- `uv run ruff format --check src/ tests/ scripts/` → clean
- `uv run pytest tests/ --tb=short -q` → 3469 passed, 2 skipped
Addresses CodeRabbit critical/major findings + github-code-quality nits:
**Critical: `request.body` duck-typing symmetry** (5 adapters).
Previously only `request.text` got the `getattr + isawaitable` treatment;
`request.body` was still vulnerable to the same async-method case (Flask,
FastAPI body-as-coroutine, etc.). Would return `<coroutine object ...>`
stringified, breaking JSON parsing and HMAC verification.
Applied to:
- src/chat_sdk/adapters/linear/adapter.py
- src/chat_sdk/adapters/discord/adapter.py
- src/chat_sdk/adapters/teams/adapter.py
- src/chat_sdk/adapters/slack/adapter.py
- src/chat_sdk/adapters/github/adapter.py
- src/chat_sdk/adapters/google_chat/adapter.py
- src/chat_sdk/adapters/whatsapp/adapter.py
(Telegram already had the correct pattern.)
**Major: sync-handler support was partial.** Previously `_run_handlers`,
`_direct_message_handlers`, and pattern handlers used `inspect.isawaitable`
guards, but `_handle_action_event`, `_handle_slash_command_event`,
`_handle_reaction_event`, `process_modal_submit`, `process_modal_close`,
and the assistant/app-home/member-joined handler loops still `await`ed
unconditionally. A sync handler on any of those paths would crash with
`TypeError`.
Fix: added `Chat._invoke_handler(handler, *args)` static helper that
calls the handler, narrows with `inspect.isawaitable`, and returns the
result (so modal-submit handlers that return a `ModalResponse` still
work). Routed all 11 remaining handler-invocation sites through it.
**Minor: Unused import false positives** (CodeQL). `ModalElement` in
slack/adapter.py and `WhatsAppCardResultInteractive` /
`WhatsAppCardResultText` in whatsapp/adapter.py were used only in
string-literal `cast("ModalElement", ...)` forms. Switched to direct
`cast(ModalElement, ...)` so the import is syntactically used — pyrefly
still resolves the cast and CodeQL no longer flags.
**Minor: Protocol signature**: dropped extra `options` / `is_subscribed`
params from `ChatInstance.handle_incoming_message` protocol stub —
matches `Chat.handle_incoming_message` implementation exactly.
Validation:
- `uv run pyrefly check` — 0 errors
- `uv run ruff check src/ tests/ scripts/` — clean
- `uv run pytest tests/ --tb=short -q` — 3469 passed, 2 skipped
CodeRabbit MAJOR (google_chat/adapter.py:785): non-callable
`request.text` was skipped by the `and callable(text_attr)` gate,
falling through to the body/str(request) path and turning valid
string-text attributes into unparseable output. The bug affects 5
adapters: github, google_chat, whatsapp, slack, telegram. Discord,
Linear, and Teams already had the correct pattern.
Fix: move the callable check INSIDE the `if text_attr is not None`
branch so populated string/bytes attributes are consumed.
```python
# Before (buggy):
if text_attr is not None and callable(text_attr):
result = text_attr()
body = str(await result if inspect.isawaitable(result) else result)
else:
# Fall through to body — wrong if text_attr was a string!
...
# After (correct):
if text_attr is not None:
if callable(text_attr):
result = text_attr()
text_attr = await result if inspect.isawaitable(result) else result
body = text_attr.decode("utf-8") if isinstance(text_attr, bytes) else str(text_attr)
else:
# body fallback only when text is actually absent
...
```
Also addresses github-code-quality "Statement has no effect" (types.py:1349):
collapse the multi-line `) -> None: ...` stub for
`ChatInstance.handle_incoming_message` to single-line to match the
other protocol stubs and silence CodeQL's "naked ellipsis" false
positive.
Validation:
- 3469 tests pass
- ruff check + format clean
- pyrefly 0 errors
Self-review findings on PR #49: 1. **bytearray inconsistency**: github / google_chat / whatsapp / telegram text-attr path used `isinstance(text_attr, bytes)`, missing bytearray. Slack's already had `(bytes, bytearray)`. Unified all four to match. 2. **Zero regression coverage** for the non-callable `request.text` string-attribute path — the fix's raison d'être shipped untested. Added `tests/test_request_body_extraction.py` with 7 cases × 3 static-method adapters (github, telegram, whatsapp) = 21 tests. Cases covered per adapter: - `text` as plain string attribute (Django-style) - `text` as bytes attribute (decoded UTF-8) - `text` as bytearray attribute (bytes/bytearray symmetry) - `text` as sync method (Flask-style) - `text` as async method (aiohttp/FastAPI) - `body` as async method (FastAPI `async def body(self)`) - `body` as bytes property (Django-style) Discord/Teams/Slack/Linear/Google Chat have instance methods (not static) — their extractor requires adapter construction which needs real platform config. Their paths are exercised via existing integration tests; the 3 static adapters cover the same patterns with less setup overhead and catch the specific historical bugs (non-callable text gate, sync text() crash, async body() coroutine stringification, bytes/bytearray asymmetry). Validation: - 3490 tests pass (21 new) - ruff check + format clean - pyrefly 0 errors
Self-review caught the asymmetry: previous commit unified bytearray handling on the text-path only. All 7 adapters still used `isinstance(body, bytes)` on the body-path — a bytearray body would fall through to `str(bytearray(...))` producing the repr string `"bytearray(b'...')"` instead of the decoded content. Applied `(bytes, bytearray)` symmetrically across all 7 adapters (discord, linear, google_chat, github, whatsapp, telegram, teams). Also added a `test_handles_bytearray_body_attribute` regression test covering the new path — parameterized across the 3 static-method adapters that the test matrix reaches.
- pyproject.toml: drop stale baseline wording; state zero-error CI policy directly (coderabbit). - test_request_body_extraction: annotate `except ImportError: pass` as skipping adapters whose optional deps aren't installed (github-code- quality empty-except flag). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…g pattern CodeRabbit flagged three real bugs in the discord/linear/teams `_get_request_body` implementations that the github/telegram/whatsapp fix didn't cover: - `str(await result if ... else result)` wrong-stringifies bytes returned from a callable `request.text()` (e.g. `str(b"...") -> "b'...'"`). - Non-callable `request.text` holding `bytes`/`bytearray` returns raw bytes to a `-> str` signature, typecheck lie + caller breaks. - `data` fallback missed `bytearray` (matches the earlier text/body bytes-vs-bytearray fix). - `read()` was only awaited via `iscoroutinefunction(raw_read)`, which misses wrapped/decorated methods that return an awaitable. Switched to `inspect.isawaitable(raw_result)` on the call result. Restructured all three to match the text-first shape used by the static adapters (github/telegram/whatsapp) and marked them `@staticmethod` — `self` was unused and consistency avoids future regressions. Tests: extended `test_request_body_extraction.py` matrix from 3 adapters to 6 and added `test_handles_sync_callable_body` for the previously unexercised sync body() branch. Dropped the broad `except ImportError: pass` guard — every adapter lazy-imports platform deps inside methods, so direct imports work and a broken adapter module now fails the suite instead of silently skipping (matches the "every test must fail when the code is wrong" rule). 54 parametrized cases (9 shapes × 6 adapters) — all passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
patrick-chinchill
force-pushed
the
fix/pyrefly-zero-errors
branch
from
6ccd363 to
a422389
Compare
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
1 participant
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.
Zero-error type-safety for the Python SDK — no
Anywidening, 10 targeted# pyrefly: ignorelines (at adapter event-construction sites matching upstream'sOmit<>partial-event pattern), fixes three latent runtime bugs as side effects.Numbers
Upstream check
Verified all changes against
vercel/chatTS source andchinchill-apiusage. All changes are upstream-aligned. Reverted initial divergences:ReactionEvent.thread: Thread | Nonethread: Thread(required)# pyrefly: ignoreat 7 adapter construction sites with comment explaining Chat re-wrapsSlashCommandEvent.channel: Channel | Nonechannel: Channel(required)set_state(state=...)setState(newState: ...)new_stateto match TSchinchill-api impact: zero — chinchill-api doesn't import
chat_sdkyet (usesslack-boltdirectly).Latent runtime bugs fixed as side effects
Sync handler support:
await handler(...)unconditionally crashed on sync handlers returningNone(the handler types declareAwaitable[None] | None). NewChat._invoke_handlerhelper narrows withinspect.isawaitable. Applied at all 11 handler invocation sites (message, reaction, action, slash, modal, assistant, home, member-joined).request.bodyasync-method support: on FastAPI/Starlette whererequest.bodyis an async callable, the old code stringified the coroutine (producing"<coroutine object...>") and webhook signature verification failed. Fixed across all 7 adapters withcallable + inspect.isawaitabledance.Non-callable
request.textstring attribute: the old gatecallable(text_attr)silently skipped Django-style string attributes, falling through to body. Fixed across 5 adapters.bytes/bytearraysymmetry: adapters mixedisinstance(x, bytes)with(bytes, bytearray)— bytearray inputs fell tostr(bytearray(...))producing"bytearray(b'...')". Unified across all 7 adapters on both text and body paths.Root causes fixed
Protocol conformance (~70 errors)
lock_scope: LockScope | None(wasstr | None) — all 8 adapters now match theAdapterprotocol; cascaded across everyadapter=selfcall site._thread_id/_status/_emoji/new_statenormalized._ChatSingleton→Protocol— was a plain class;Chatnow structurally satisfies it.ThreadImpl.channel -> Channel(was-> ChannelImpl) — matches upstreamget channel(): Channel<TState>.ChatInstanceprotocol advertiseshandle_incoming_message— upstream TS also declares it._ChannelImplConfigForChatremoved — duplicate of_ChannelImplConfigWithAdapter.Error-handler annotations (−22)
_handle_slack_error,_handle_google_chat_error→NoReturn.Framework-agnostic request duck-typing (−14)
hasattr(request, "text") and callable(request.text)narrowsAny→object, makingawait request.text()fail type-check (and also crash at runtime on sync Flask).getattr(request, "text", None)+inspect.isawaitable.Handler sync/async union (−3)
Callable[..., Awaitable[None] | None]. Old codeawait handler(...)would crash on sync handlers. Fixed withinspect.isawaitableguards viaChat._invoke_handler.TypedDict unions and functional TypedDicts
WhatsAppInboundMessage["from"]— functional TypedDict form ("from"is a Python keyword).GitHubAdapterConfigunion — internal superset TypedDict cast at init boundary._dispatch_event, Discord interactiondata, WhatsApp message parsing — narrowcast(...)at runtime-narrowed sites.Card-element helpers
cast("dict[str, Any]", child)in_render_child/_convert_child_to_widgetsbranches where the variant is already narrowed via.get("type").Optional-dep submodule imports (−7)
replace-imports-with-anynow wildcards submodules (slack_sdk.*,nacl.*,redis.*, etc.).Test additions
tests/test_request_body_extraction.pycovering 7 request shapes × 3 static-method adapters (github, telegram, whatsapp): string/bytes/bytearray text-attr, sync/async text(), async body() and bytes/bytearray body-attr. Catches all four runtime bugs above.Baseline removed
CI now enforces zero type errors — no regression room. The 21-entry baseline file is gone.
Test plan
uv run pyrefly check→ 0 errorsuv run ruff check src/ tests/ scripts/→ cleanuv run ruff format --check src/ tests/ scripts/→ cleanuv run pytest tests/ --tb=short -q→ 3,493 passed, 2 skippedReady for review.
Summary by CodeRabbit
Chores
Tests