Skip to content

Comments

Switch to non-strict OpenAI tools#412

Merged
KaQuMiQ merged 1 commit intomainfrom
feature/strict
Sep 15, 2025
Merged

Switch to non-strict OpenAI tools#412
KaQuMiQ merged 1 commit intomainfrom
feature/strict

Conversation

@KaQuMiQ
Copy link
Collaborator

@KaQuMiQ KaQuMiQ commented Sep 15, 2025

No description provided.

@coderabbitai
Copy link

coderabbitai bot commented Sep 15, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Project 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)
Check name Status Explanation Resolution
Description Check ❓ Inconclusive No PR description was provided, so there is no author-written summary to confirm intent or highlight motivations; the diff shows multiple non-trivial changes (per-tool strictness defaulting, ModelToolFunctionSpecification -> class/state with meta, widespread dict->attribute access updates, parameter-spec compression, and deleted integration tests), making it impossible to fully assess intent or risk from the title alone. Therefore this check is inconclusive. Ask the author to add a concise description that lists the primary behavioral and API-shape changes (per-tool strictness default and rationale, the class-based tool-spec migration and meta field, dict->attribute access migration, parameter schema changes such as type-array unions, and the removed tests) plus any migration/compatibility notes and targeted files or tests for reviewers to focus on.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Switch to non-strict OpenAI tools" correctly identifies a real change in the PR—the move to per-tool non-strict parameter validation (defaulting to non-strict) for OpenAI-related code—but it only captures one aspect of a broader refactor that migrates tool specifications from TypedDicts to class/state objects, adds a meta field, converts many consumers from dict->attribute access, and changes parameter schema generation and tests; thus the title is related but not comprehensive.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c479960 and a615220.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (18)
  • 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/schema.py (1 hunks)
  • src/draive/parameters/specification.py (3 hunks)
  • src/draive/vllm/messages.py (3 hunks)
  • tests/test_ollama_integration.py (0 hunks)
  • tests/test_schema.py (1 hunks)
  • tests/test_specification.py (1 hunks)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: Store specification as 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.meta assignment below to re-use effective_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

📥 Commits

Reviewing files that changed from the base of the PR and between ad07ae3 and 50fe6b6.

⛔ Files ignored due to path filters (1)
  • uv.lock is 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) sections

Format all Python code with Ruff formatter (run via make format); do not use any other formatter

Files:

  • src/draive/models/tools/function.py
  • src/draive/ollama/chat.py
  • src/draive/mcp/server.py
  • src/draive/openai/realtime.py
  • src/draive/bedrock/converse.py
  • src/draive/models/tools/toolbox.py
  • src/draive/mistral/completions.py
  • src/draive/openai/responses.py
  • src/draive/anthropic/messages.py
  • src/draive/models/types.py
  • src/draive/gemini/generating.py
  • src/draive/vllm/messages.py
  • src/draive/parameters/specification.py
src/draive/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

src/draive/**/*.py: Import Haiway symbols directly as from haiway import State, ctx
Use ctx.scope(...) to bind active State instances; avoid global state
Route all logging through ctx.log_* and never use print
Enforce strict typing on public APIs; avoid untyped definitions and Any except at verified third‑party boundaries
Prefer explicit attribute access with static types; avoid dynamic getattr except at narrow boundaries
Use abstract immutable protocols (Mapping, Sequence, Iterable) instead of concrete dict/list/set in public types
Use final where applicable; prefer composition over complex inheritance
Use precise unions (|), narrow with match/isinstance, and avoid cast unless provably safe and localized
Model immutable configuration/data as haiway.State; avoid in‑place mutation and prefer State.updated(...)/functional builders
Access active state via haiway.ctx within 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 via ctx.record where applicable
All I/O is async; keep boundaries async, use ctx.spawn for detached tasks, and avoid custom threading
Translate provider/SDK errors into typed exceptions; never raise bare Exception; preserve context
Compose multimodal content with MultimodalContent.of(...); use ResourceContent/Reference for blobs; wrap custom data in ArtifactContent (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 like getenv_str

Files:

  • src/draive/models/tools/function.py
  • src/draive/ollama/chat.py
  • src/draive/mcp/server.py
  • src/draive/openai/realtime.py
  • src/draive/bedrock/converse.py
  • src/draive/models/tools/toolbox.py
  • src/draive/mistral/completions.py
  • src/draive/openai/responses.py
  • src/draive/anthropic/messages.py
  • src/draive/models/types.py
  • src/draive/gemini/generating.py
  • src/draive/vllm/messages.py
  • src/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": False for 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": False for stricter validation, consistent with other providers.

src/draive/parameters/specification.py (1)

35-39: LGTM! Well-designed type for compressed union representations.

The ParameterAlternativesSpecification provides 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 logs

Switch to tool.name aligns with the new object-based spec.


425-425: LGTM: attribute access for tool names in stream logs

Consistent with non-stream path.


730-742: Omit injected restrictive empty parameter schema; preserve explicit {} and align with realtime

When 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 logs

Matches the responses path.

@KaQuMiQ KaQuMiQ merged commit f493a34 into main Sep 15, 2025
4 of 5 checks passed
@KaQuMiQ KaQuMiQ deleted the feature/strict branch September 15, 2025 14:12
This was referenced Sep 16, 2025
This was referenced Sep 26, 2025
@coderabbitai coderabbitai bot mentioned this pull request Nov 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant