Conversation
|
Caution Review failedThe pull request is closed. WalkthroughProject version bumped in pyproject.toml from 0.84.5 to 0.84.6. ModelToolSpecification migrated from a TypedDict/dict shape to an attribute-based class across many call sites (openai, openai.realtime, anthropic, bedrock, gemini, mcp, mistral, ollama, vllm, etc.), and FunctionTool now includes a meta field in its published spec. parameters/specification.py adds ParameterAlternativesSpecification plus compression logic to emit compact "type" arrays for unions of primitive types. parameters/schema.py gained handling for list-typed "type" in simplified schema rendering. An Ollama integration test file was removed and some tests updated to expect the new compact union schema. Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Pre-merge checks and finishing touches and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
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 |
ad07ae3 to
50fe6b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/draive/models/tools/function.py (1)
118-129: Storespecificationas a State instance, not a dict; also use the finalized meta.The repo now expects attribute access (tool.name, …). Keeping a dict here breaks MCP/Toolbox and causes CI type validation noise. Build ModelToolSpecification with the same updated Meta you assign to
self.meta.Apply:
- self.specification: ModelToolSpecification - object.__setattr__( - self, - "specification", - { - "name": name, - "description": description, - "parameters": parameters, - "additionalProperties": False, - "meta": meta, - }, - ) + # Build finalized meta first (mirrors self.meta below) and use it for the spec too + effective_meta: Meta = ( + meta.updated(kind="tool", name=name, description=description) + if description + else meta.updated(kind="tool", name=name) + ) + self.specification: ModelToolSpecification + object.__setattr__( + self, + "specification", + ModelToolSpecification( + name=name, + description=description, + parameters=parameters, + meta=effective_meta, + ), + )Also update the
self.metaassignment below to re-useeffective_meta:- self.meta: Meta - if description: - object.__setattr__( - self, - "meta", - meta.updated( - kind="tool", - name=name, - description=description, - ), - ) - - else: - object.__setattr__( - self, - "meta", - meta.updated( - kind="tool", - name=name, - ), - ) + self.meta: Meta + object.__setattr__(self, "meta", effective_meta)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
pyproject.toml(1 hunks)src/draive/anthropic/messages.py(3 hunks)src/draive/bedrock/converse.py(2 hunks)src/draive/gemini/generating.py(3 hunks)src/draive/mcp/server.py(1 hunks)src/draive/mistral/completions.py(2 hunks)src/draive/models/tools/function.py(1 hunks)src/draive/models/tools/toolbox.py(1 hunks)src/draive/models/types.py(1 hunks)src/draive/ollama/chat.py(2 hunks)src/draive/openai/realtime.py(2 hunks)src/draive/openai/responses.py(3 hunks)src/draive/parameters/specification.py(3 hunks)src/draive/vllm/messages.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports from the draive package (e.g., from draive import ...), not relative imports
Follow Ruff import ordering: standard library, third-party, then local imports
Use Python 3.12+ typing features (e.g., | unions, PEP 695 generics)
Prefer abstract/base collection types (Sequence, Mapping, Set, Iterable) over concrete list, dict, set in type hints
Define and raise custom exception types for specific error cases
Use NumPy docstring convention for all functions, classes, and methods
Skip module-level docstrings unless explicitly requested
Docstrings should include Parameters, Returns, Raises, and Notes (if needed) sectionsFormat all Python code with Ruff formatter (run via
make format); do not use any other formatter
Files:
src/draive/models/tools/function.pysrc/draive/ollama/chat.pysrc/draive/mcp/server.pysrc/draive/openai/realtime.pysrc/draive/bedrock/converse.pysrc/draive/models/tools/toolbox.pysrc/draive/mistral/completions.pysrc/draive/openai/responses.pysrc/draive/anthropic/messages.pysrc/draive/models/types.pysrc/draive/gemini/generating.pysrc/draive/vllm/messages.pysrc/draive/parameters/specification.py
src/draive/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/draive/**/*.py: Import Haiway symbols directly asfrom haiway import State, ctx
Usectx.scope(...)to bind activeStateinstances; avoid global state
Route all logging throughctx.log_*and never use
Enforce strict typing on public APIs; avoid untyped definitions andAnyexcept at verified third‑party boundaries
Prefer explicit attribute access with static types; avoid dynamicgetattrexcept at narrow boundaries
Use abstract immutable protocols (Mapping,Sequence,Iterable) instead of concretedict/list/setin public types
Usefinalwhere applicable; prefer composition over complex inheritance
Use precise unions (|), narrow withmatch/isinstance, and avoidcastunless provably safe and localized
Model immutable configuration/data ashaiway.State; avoid in‑place mutation and preferState.updated(...)/functional builders
Access active state viahaiway.ctxwithin async scopes (ctx.scope(...))
Public state methods that dispatch on the active instance must use@statemethod
Add structured, concise logs around generation calls, tool dispatch, and provider requests/responses (without secrets); add metrics viactx.recordwhere applicable
All I/O is async; keep boundaries async, usectx.spawnfor detached tasks, and avoid custom threading
Translate provider/SDK errors into typed exceptions; never raise bareException; preserve context
Compose multimodal content withMultimodalContent.of(...); useResourceContent/Referencefor blobs; wrap custom data inArtifactContent(use hidden when needed)
Public symbols require NumPy-style docstrings with Parameters/Returns/Raises and rationale when non-obvious
Avoid docstrings for internal helpers; prefer self-explanatory names; skip module-level docstrings
Never log secrets or full request bodies containing keys/tokens
Read credentials from environment variables; resolve via helpers likegetenv_str
Files:
src/draive/models/tools/function.pysrc/draive/ollama/chat.pysrc/draive/mcp/server.pysrc/draive/openai/realtime.pysrc/draive/bedrock/converse.pysrc/draive/models/tools/toolbox.pysrc/draive/mistral/completions.pysrc/draive/openai/responses.pysrc/draive/anthropic/messages.pysrc/draive/models/types.pysrc/draive/gemini/generating.pysrc/draive/vllm/messages.pysrc/draive/parameters/specification.py
🧠 Learnings (1)
📚 Learning: 2025-09-11T15:22:15.967Z
Learnt from: CR
PR: miquido/draive#0
File: AGENTS.md:0-0
Timestamp: 2025-09-11T15:22:15.967Z
Learning: Applies to src/draive/**/*.py : Model immutable configuration/data as `haiway.State`; avoid in‑place mutation and prefer `State.updated(...)`/functional builders
Applied to files:
src/draive/models/types.py
🧬 Code graph analysis (9)
src/draive/ollama/chat.py (1)
src/draive/models/tools/function.py (4)
tool(346-365)tool(369-418)tool(422-467)tool(470-500)
src/draive/mcp/server.py (1)
src/draive/models/tools/function.py (4)
tool(346-365)tool(369-418)tool(422-467)tool(470-500)
src/draive/openai/realtime.py (1)
src/draive/models/tools/function.py (4)
tool(346-365)tool(369-418)tool(422-467)tool(470-500)
src/draive/mistral/completions.py (1)
src/draive/models/tools/function.py (4)
tool(346-365)tool(369-418)tool(422-467)tool(470-500)
src/draive/openai/responses.py (1)
src/draive/models/tools/function.py (4)
tool(346-365)tool(369-418)tool(422-467)tool(470-500)
src/draive/anthropic/messages.py (1)
src/draive/models/tools/function.py (4)
tool(346-365)tool(369-418)tool(422-467)tool(470-500)
src/draive/models/types.py (1)
src/draive/parameters/specification.py (1)
ToolParametersSpecification(193-204)
src/draive/gemini/generating.py (1)
src/draive/models/tools/function.py (4)
tool(346-365)tool(369-418)tool(422-467)tool(470-500)
src/draive/vllm/messages.py (1)
src/draive/models/tools/function.py (4)
tool(346-365)tool(369-418)tool(422-467)tool(470-500)
🪛 GitHub Actions: CI
src/draive/models/types.py
[error] 273-273: Validation of .specifications[0].meta failed: 'MISSING' is not matching expected type of 'haiway.utils.metadata.Meta'. Command: 'uv run pytest --rootdir= ./tests --doctest-modules --junitxml=junit/test-results.xml'.
[error] 273-273: Validation of .specifications[0].meta failed: 'MISSING' is not matching expected type of 'haiway.utils.metadata.Meta'. Command: 'uv run pytest --rootdir= ./tests --doctest-modules --junitxml=junit/test-results.xml'.
🔇 Additional comments (22)
pyproject.toml (1)
8-8: Approve version bump; verify other version declarations & update release notes.
- pyproject.toml (line 8) contains version = "0.84.6" — tag v0.84.6 and update CHANGELOG/release notes.
- The rg check for version returned "No files were searched". Confirm there are no other version declarations (e.g., package init.py, version.py, setup.cfg) or re-run: rg -nP "version\s*=\s*['"][0-9]+.[0-9]+.[0-9]+['"]" -S || true
src/draive/models/tools/toolbox.py (1)
212-213: Attribute access is correct — no dict-style accesses remain.Search found 0 occurrences of tool['name']/tool["name"] and the codebase consistently uses attribute access (tool.name) — tools_selection = tool_suggestion.name is appropriate.
src/draive/anthropic/messages.py (3)
150-150: LGTM! Attribute access pattern correctly implemented.The change from dictionary-style access to attribute access for tool specifications is consistent and correct.
675-676: LGTM! Consistent use of attribute access.The change properly migrates from dictionary-style to attribute access for the tool parameters.
688-689: LGTM! Attribute access migration completed.All tool specification properties are now correctly accessed via attributes.
src/draive/bedrock/converse.py (2)
128-128: LGTM! Consistent attribute access pattern.The change properly migrates tool specifications to use attribute access.
433-435: LGTM! Tool conversion properly uses attributes.The function correctly accesses tool properties via attributes.
src/draive/vllm/messages.py (3)
130-130: LGTM! Consistent attribute access pattern.The tool specifications properly use attribute access for extracting tool names.
276-276: LGTM! Streaming path correctly updated.The streaming completion path consistently uses attribute access for tool specifications.
593-596: LGTM! Tool parameter conversion properly uses attributes.The function correctly accesses tool properties via attributes, maintaining consistency with the migration pattern.
src/draive/gemini/generating.py (3)
138-138: LGTM! Consistent attribute access in logging.The tool specifications properly use attribute access for extracting tool names.
282-282: LGTM! Streaming path consistently updated.The streaming completion path properly uses attribute access for tool specifications.
469-471: LGTM! Tool declaration properly uses attributes.The FunctionDeclarationDict creation correctly accesses tool properties via attributes.
src/draive/mistral/completions.py (2)
290-290: LGTM! Consistent attribute access in logging.The tool specifications properly use attribute access for extracting tool names.
574-580: Improved default schema with stricter validation.The tool specification correctly uses attribute access and improves the default schema by adding
"additionalProperties": Falsefor stricter validation.src/draive/ollama/chat.py (2)
119-119: LGTM! Consistent attribute access pattern.The tool specifications properly use attribute access for extracting tool names.
347-355: Improved default schema with stricter validation.The tool specification correctly uses attribute access and improves the default schema by adding
"additionalProperties": Falsefor stricter validation, consistent with other providers.src/draive/parameters/specification.py (1)
35-39: LGTM! Well-designed type for compressed union representations.The
ParameterAlternativesSpecificationprovides an efficient way to represent unions of primitive types as a compact array.src/draive/openai/responses.py (3)
185-185: LGTM: attribute access for tool names in logsSwitch to
tool.namealigns with the new object-based spec.
425-425: LGTM: attribute access for tool names in stream logsConsistent with non-stream path.
730-742: Omit injected restrictive empty parameter schema; preserve explicit {} and align with realtimeWhen tool.parameters is unspecified, emit None (omit parameters) instead of injecting {"type":"object","properties":{},"additionalProperties": False}; preserve {} when explicitly provided.
cast( ToolParam, FunctionToolParam( type="function", - name=tool.name, - description=tool.description or None, - parameters=cast(dict[str, object] | None, tool.parameters) - or { - "type": "object", - "properties": {}, - "additionalProperties": False, - }, - strict=tool.meta.get_bool( - "strict_parameters", - default=False, - ), + name=tool.name, + description=tool.description or None, + # Omit when unspecified; preserve {} if explicitly provided. + parameters=cast(dict[str, object] | None, tool.parameters) + if tool.parameters is not None + else None, + strict=tool.meta.get_bool("strict_parameters", default=False), ), )Repository search shows many existing uses of "additionalProperties": False (e.g. src/draive/openai/responses.py:734-737, src/draive/parameters/specification.py:687). Verify callers that intentionally rely on restrictive schemas before applying this change.
src/draive/openai/realtime.py (1)
104-104: LGTM: attribute access for tool names in logsMatches the responses path.
50fe6b6 to
c479960
Compare
c479960 to
a615220
Compare
No description provided.