feat(ai): add create_chat_tools tool factory (PR 2 of 3)#122
Conversation
…#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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 ports the TypeScript Chat SDK tool factory to Python, introducing the ChatTool class and the create_chat_tools orchestrator to expose chat operations to AI agents, supported by a comprehensive test suite. The review feedback suggests improving error handling by catching ChatNotImplementedError when invoking optional adapter methods (such as fetch_channel_messages and list_threads) and raising a clean ChatError. Additionally, it recommends a more robust type check for the preset parameter by checking if it is a string rather than a list.
| from typing import Any, Literal | ||
|
|
||
| from chat_sdk.chat import Chat | ||
| from chat_sdk.errors import ChatError |
There was a problem hiding this comment.
Import ChatNotImplementedError from chat_sdk.errors so that we can catch it when calling optional adapter methods directly.
| from chat_sdk.errors import ChatError | |
| from chat_sdk.errors import ChatError, ChatNotImplementedError |
| cursor = args.get("cursor") | ||
| direction = args.get("direction", "backward") | ||
| result = await fetch_method( | ||
| channel_id, |
There was a problem hiding this comment.
Since fetch_channel_messages is an optional adapter method defined on BaseAdapter (which raises ChatNotImplementedError by default), calling it directly will raise ChatNotImplementedError instead of the expected ChatError if the adapter does not implement it. Wrap the call in a try...except ChatNotImplementedError block to raise ChatError with a clean message, matching the pattern used in Chat.get_user. Ensure that the await statement is placed inside the try block, and always accompany the fix with a regression test that verifies the exception is correctly logged and propagated.
try:
result = await fetch_method(
channel_id,
FetchOptions(limit=limit, cursor=cursor, direction=direction),
)
except ChatNotImplementedError as exc:
raise ChatError(f'Adapter "{adapter_name}" does not support fetching channel messages') from excReferences
- When handling exceptions for asynchronous operations, ensure that the
awaitstatement is placed inside thetryblock so that exceptions are caught during execution, and always accompany the fix with a regression test that verifies the exception is correctly logged and propagated.
|
|
||
| limit = args.get("limit", 20) | ||
| cursor = args.get("cursor") | ||
| result = await list_method(channel_id, ListThreadsOptions(limit=limit, cursor=cursor)) |
There was a problem hiding this comment.
Since list_threads is an optional adapter method defined on BaseAdapter (which raises ChatNotImplementedError by default), calling it directly will raise ChatNotImplementedError instead of the expected ChatError if the adapter does not implement it. Wrap the call in a try...except ChatNotImplementedError block to raise ChatError with a clean message, matching the pattern used in Chat.get_user. Ensure that the await statement is placed inside the try block, and always accompany the fix with a regression test that verifies the exception is correctly logged and propagated.
try:
result = await list_method(channel_id, ListThreadsOptions(limit=limit, cursor=cursor))
except ChatNotImplementedError as exc:
raise ChatError(f'Adapter "{adapter_name}" does not support listing threads') from excReferences
- When handling exceptions for asynchronous operations, ensure that the
awaitstatement is placed inside thetryblock so that exceptions are caught during execution, and always accompany the fix with a regression test that verifies the exception is correctly logged and propagated.
|
|
||
|
|
||
| def _resolve_preset_tools(preset: ChatToolPreset | list[ChatToolPreset]) -> set[str]: | ||
| presets: list[str] = list(preset) if isinstance(preset, list) else [preset] |
There was a problem hiding this comment.
Using isinstance(preset, list) to check if preset is a collection of presets is fragile. If a user passes a tuple or set of presets, it will fail or behave unexpectedly because it will treat the entire collection as a single preset name. Checking isinstance(preset, str) to distinguish a single string from any other iterable collection is much more robust and idiomatic in Python.
presets: list[str] = [preset] if isinstance(preset, str) else list(preset)…emini 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
![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: 2a20605432
ℹ️ 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".
| limit = args.get("limit", 20) | ||
| cursor = args.get("cursor") | ||
| try: | ||
| result = await list_method(channel_id, ListThreadsOptions(limit=limit, cursor=cursor)) |
There was a problem hiding this comment.
When the tool is used with the built-in MockAdapter (a common downstream test adapter), listThreads raises TypeError before returning anything because MockAdapter.list_threads is defined as list_threads(self, channel_id, **kwargs), so this second positional argument is rejected. Passing the options as options=... (or otherwise accommodating keyword-style adapters) keeps the production adapters working while making the new tool usable with the SDK’s own mock adapter.
Useful? React with 👍 / 👎.
`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
|
@codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
patrick-chinchill
merged commit bf66edb
into
claude/port-chat-ai-module-move
2 participants
PR 2 of 3 —
chat/aisubpath port (vercel/chat#492)Stacked on top of #116 (
claude/port-chat-ai-module-move), which did the structural module-to-package move only. This PR adds thecreate_chat_toolstool factory in the newchat_sdk.aipackage. PR 3 will wire these tools into the existing handler / consumer paths.GitHub will automatically retarget this PR to
mainonce #116 merges.Refs tracking issue #98 (overall 4.29 sync) and design issue #109 (chat-ai port design).
What's ported from vercel/chat#492
packages/chat/src/ai/tools.ts(createChatTools orchestrator)packages/chat/src/ai/types.ts(ChatBinding,ToolOptions,ToolOverrides)packages/chat/src/ai/tools/channels.ts—get_channel_infopackages/chat/src/ai/tools/messages.ts—post_message,post_channel_message,send_direct_message,edit_message,delete_messagepackages/chat/src/ai/tools/reactions.ts—add_reaction,remove_reactionpackages/chat/src/ai/tools/threads.ts—fetch_messages,fetch_channel_messages,fetch_thread,list_threads,get_thread_participants,subscribe_thread,unsubscribe_thread,start_typingpackages/chat/src/ai/tools/users.ts—get_userpackages/chat/src/ai/index.test.tsPublic API
Each tool is a
ChatTooldataclass:Individual factories (
post_message,add_reaction, ...) are also exported so callers can cherry-pick — matching upstream's per-symbol exports.Python idiom notes / divergences
ai)tool()helper andzodschemas. Python has no canonical agent runtime; rather than add a third-party schema validator as a hard dependency, each tool factory returns a plainChatTooldataclass holding a JSON-Schema-shapedinput_schemadict. Consumers that bind these into an actual agent runtime translate that dict to their schema layer. No new runtime deps added.postMessage,fetchChannelMessages, ...) so consumers that mix this with the JS SDK across language boundaries see the same tool ids.input_schema+inputSchema, etc.) so callers porting upstreamoverridesdicts get the same protection.ToolOverridesis a plaindict[str, Any]rather than aTypedDict; the upstream type is aPartial<Pick<Tool, ...>>over fields from the Vercel AI SDK package that don't have direct Python equivalents.Out of scope (PR 3)
create_chat_toolsinto existing handlers / consumer code paths.apps/docs/content/docs/ai/*).Files
src/chat_sdk/ai/tools.py(new)src/chat_sdk/ai/__init__.py(re-exports added)tests/test_ai_tools.py(new, 38 tests)Validation
Baseline (from #116) was 4043 passed, 3 skipped — this PR adds exactly the 38 new tests.
Load-bearing verification: each test was confirmed to fail when its corresponding production code is reverted (spot-checked by breaking
_resolve_approvaland watchingTestRequireApproval::test_per_tool_approval_overridesfail).🤖 Generated with Claude Code via claude-code-sdk-python
Generated by Claude Code