refactor(ai): convert chat_sdk.ai module to package for chat/ai subpath#116
Conversation
…th (vercel/chat#492) Mirror upstream PR vercel/chat#492 which split ai.ts into ai/messages.ts to make room for ai/tools.ts. Convert the chat_sdk.ai module into a package: - src/chat_sdk/ai/messages.py: to_ai_messages + supporting types, moved verbatim from the former ai.py (docstring path updated only). - src/chat_sdk/ai/__init__.py: re-export shim exposing every symbol the old module exported, so from chat_sdk.ai import to_ai_messages keeps working. - tests/test_ai.py renamed to tests/test_ai_messages.py (logic unchanged). No public import path changed. PR 1 of 3 (module move only); the tool factory (create_chat_tools) lands in PR 2. Design issue #109, tracking #98. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
|
Warning Review limit reached
More reviews will be available in 55 minutes. 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 (4)
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 |
There was a problem hiding this comment.
Code Review
This pull request restructures the AI SDK integration by introducing a new 'src/chat_sdk/ai/init.py' module to re-export components from 'chat_sdk.ai.messages', ensuring backward compatibility. Additionally, a docstring reference in 'src/chat_sdk/ai/messages.py' was updated. There are no review comments to address, and the changes are clean and appropriate. I have no further feedback to provide.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5ff616c7f
ℹ️ 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".
| from chat_sdk.ai.messages import ( | ||
| AiAssistantMessage, | ||
| AiFilePart, | ||
| AiImagePart, | ||
| AiMessage, | ||
| AiMessagePart, | ||
| AiTextPart, | ||
| AiUserMessage, | ||
| ToAiMessagesOptions, | ||
| to_ai_messages, | ||
| ) |
There was a problem hiding this comment.
Re-export the existing MIME prefix constant
For consumers that imported the non-underscored TEXT_MIME_PREFIXES from the old chat_sdk.ai module (for example to inspect or reuse the text-file attachment policy), this package shim now raises ImportError because it only imports the message types and to_ai_messages. Since this refactor is intended to preserve the old chat_sdk.ai import surface, please re-export the constant from chat_sdk.ai.messages as well.
Useful? React with 👍 / 👎.
Two CI / review follow-ups on PR #116: 1. Fidelity-mapping fix went too far in the previous commit. The script's parity pin is `chat@4.26.0` (see `lint.yml:63`), where the upstream AI test file is still the flat `packages/chat/src/ai.test.ts` — the move to `ai/messages.test.ts` happened in 4.29.0. Pointing the mapping at the 4.29-era path made `verify_test_fidelity.py --strict` fail with "upstream checkout missing — mapped TS file not found". Revert just the TS half of the mapping to `ai.test.ts`; keep the Python half at `tests/test_ai_messages.py` (the file this PR actually renamed). When the parity pin moves to 4.29.0 the TS path can advance too. Locally verified against a fresh `chat@4.26.0` clone: TOTAL: 564/564 matched (100%), 0 missing All TS tests have Python equivalents. 2. The package shim was missing `TEXT_MIME_PREFIXES` (flagged by Codex P2). The PR description promises to "re-export everything the former `chat_sdk.ai` module exposed" — add the constant to both the import block and `__all__` so the contract holds. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
|
@codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
* feat(ai): add create_chat_tools tool factory (PR 2 of 3) (vercel/chat#492) Port of `packages/chat/src/ai/tools.ts` (plus the supporting `tools/{channels,messages,reactions,threads,users}.ts` and `types.ts`) introduced by vercel/chat#492. Builds on top of #116, which moved `chat_sdk.ai` from a single module to a package so `tools.py` has a home. `create_chat_tools(chat, preset=, require_approval=, overrides=)` returns a mapping of tool-name -> `ChatTool` dataclass holding a description, JSON-Schema-shaped `input_schema`, an async `execute` callable, and the `needs_approval` flag. Three presets (`reader` / `messenger` / `moderator`) and per-tool overrides match the upstream API surface verbatim. Individual factories (`post_message`, `add_reaction`, ...) are also exported so callers can cherry-pick. PR 3 will wire these tools into the existing handler paths; this PR adds the surface only. Validation: uv run ruff check src/ tests/ -> clean uv run ruff format --check src/ tests/ -> clean uv run python scripts/audit_test_quality.py -> 0 hard failures uv run pytest tests/ -q -> 4081 passed, 3 skipped Refs #98, #109. * fix(ai/tools): wrap ChatNotImplementedError + tighten preset check (gemini review) Four small follow-ups to PR 2 of the chat-ai port from Gemini's review: - Import `ChatNotImplementedError` alongside `ChatError`. - Wrap `fetch_channel_messages` invocation in `try/except ChatNotImplementedError` and re-raise as `ChatError` (preserves cause). The early `getattr(adapter, "fetch_channel_messages", None) is None` branch only catches missing attributes; `BaseAdapter` exposes the attribute as a stub that itself raises `ChatNotImplementedError`, so without this wrap the tool surfaces a different exception type than callers expect. Matches the `Chat.get_user` pattern. - Same wrap for `list_threads`. - `_resolve_preset_tools` now distinguishes a single `str` preset from any iterable of presets via `isinstance(preset, str)` rather than `isinstance(preset, list)`, so tuples/sets/Sequences work correctly. Two new regression tests (`test_fetch_channel_messages_wraps_not_implemented`, `test_list_threads_wraps_not_implemented`) drive an `AsyncMock` whose `side_effect` is `ChatNotImplementedError`, assert the surfaced exception is `ChatError` with the expected message, and pin the cause chain (`__cause__ is ChatNotImplementedError`). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj * fix(ai/tools): isolate per-tool input schemas to prevent mutation bleed (review) * fix(ai/tools): pass list_threads options as keyword (codex P2) `MockAdapter.list_threads` is declared as `list_threads(self, channel_id, **kwargs)`, so the tool's positional `list_method(channel_id, ListThreadsOptions(...))` raised `TypeError` for any consumer wiring `create_chat_tools` into MockAdapter (the SDK's own mock). Production adapters accept the kwarg form just as readily as the positional form, so the change is universally safe. The existing `test_list_threads_projects_summaries` used `AsyncMock(return_value=...)` which masked the TypeError by replacing the real `MockAdapter.list_threads` entirely. New `test_list_threads_uses_keyword_options` exercises the real MockAdapter and asserts the default empty result — fails on the old positional call with `TypeError`, passes after. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj * style(ai/tools): apply is-not-None idiom + hoist core import (audit nits) --------- Co-authored-by: Claude <noreply@anthropic.com>
PR 1 of 3 (module move only)
First PR of the
chat/aisubpath port. Design issue #109, tracking #98.Mirrors upstream vercel/chat#492, which split
ai.tsintoai/messages.tsto make room forai/tools.ts. This PR does only the structural move so the larger tool-factory PR (PR 2) builds on a clean base.What changed
src/chat_sdk/ai.py(module) ->src/chat_sdk/ai/(package)src/chat_sdk/ai/messages.py-to_ai_messages+ supporting types, moved verbatim from the oldai.py(only the docstring path reference updated).src/chat_sdk/ai/__init__.py- Re-exports every name intended for public use from the formerchat_sdk.aimodule (the typed message classes,to_ai_messages, andTEXT_MIME_PREFIXES). Stdlib/typing names that the old module form incidentally leaked (e.g.base64,logging,Any) are intentionally not re-exported — they were never part of the intended public surface.tests/test_ai.py->tests/test_ai_messages.py(test logic unchanged).Backward compatibility
No public import path changed. Verified at runtime that
from chat_sdk.ai import to_ai_messages,from chat_sdk import to_ai_messages, andfrom chat_sdk.ai.messages import to_ai_messagesall resolve to the same object. The top-levelsrc/chat_sdk/__init__.pyre-export needed no changes (thechat_sdk.aipackage still exportsto_ai_messages).Out of scope
No
create_chat_toolsor tool factories — that is PR 2.Validation
https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code