From 0972f130c9ddfd8a84615e7c82b8c543d9581de8 Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Tue, 29 Sep 2026 12:12:17 +0100 Subject: [PATCH 1/5] fix(tools): carry element types and constraints into tool schemas The schema rebuild for simple tool parameters kept only description, type, enum and default, dropping items, additionalProperties and Field constraints. The model was never told a list's or dict's element type, and validate_tool_arguments, which builds its validator from the same schema, fell back to string elements: list[int] arguments reached the tool as strings, and list[bool], nested lists and lists of models failed validation and were passed through unchecked. Copy items, additionalProperties and the standard JSON Schema constraint keywords onto the rebuilt property, taking them from the non-null anyOf branch for Optional parameters. In the validator, treat an array with a missing or empty items schema as list[Any] rather than list[str], so list[Any], tuples and external schemas without items are no longer coerced to strings. Fixes #1693 Assisted-by: Claude Code Signed-off-by: Nigel Jones --- mellea/backends/tools.py | 44 ++- .../test_tool_collection_params_unit.py | 362 ++++++++++++++++++ 2 files changed, 404 insertions(+), 2 deletions(-) create mode 100644 test/backends/test_tool_collection_params_unit.py diff --git a/mellea/backends/tools.py b/mellea/backends/tools.py index ae461dfb6a..8d29b679c5 100644 --- a/mellea/backends/tools.py +++ b/mellea/backends/tools.py @@ -632,9 +632,13 @@ def _build_pydantic_type_from_schema(schema: dict[str, Any]) -> Any: **nested_fields, ) - # Handle arrays + # Handle arrays. A missing or empty `items` schema leaves the elements + # unconstrained; falling through to the `string` default would coerce + # every numeric element to a string under `coerce_numbers_to_str`. if json_type == "array": - item_schema = schema.get("items", {}) + item_schema = schema.get("items") + if not item_schema: + return list[Any] item_type = _build_pydantic_type_from_schema(item_schema) return list[item_type] # type: ignore @@ -1346,6 +1350,30 @@ def _inline(branch: dict) -> dict: return out +# JSON Schema keywords copied from Pydantic's schema onto a rebuilt simple +# property. Without them the model is never told a list's or dict's element +# type or a field's constraints, and `validate_tool_arguments`, which builds +# its validator from the same schema, loses the element types too. +_CARRIED_SCHEMA_KEYWORDS = ( + "items", + "additionalProperties", + "minItems", + "maxItems", + "uniqueItems", + "minProperties", + "maxProperties", + "minLength", + "maxLength", + "pattern", + "format", + "minimum", + "maximum", + "exclusiveMinimum", + "exclusiveMaximum", + "multipleOf", +) + + # https://github.com/ollama/ollama-python/blob/60e7b2f9ce710eeb57ef2986c46ea612ae7516af/ollama/_utils.py#L56-L90 def convert_function_to_ollama_tool( func: Callable, name: str | None = None @@ -1493,6 +1521,18 @@ def convert_function_to_ollama_tool( # from scratch would otherwise drop it. if "default" in v: simple_prop["default"] = v["default"] + # Carry element types and constraints across too. For an Optional + # parameter Pydantic puts them on the non-null anyOf branch; with + # several non-null branches they belong to one branch each, so + # none are merged onto the flattened property. + if "anyOf" in v: + non_null = [s for s in v["anyOf"] if s.get("type") != "null"] + keyword_source = non_null[0] if len(non_null) == 1 else {} + else: + keyword_source = v + for keyword in _CARRIED_SCHEMA_KEYWORDS: + if keyword in keyword_source: + simple_prop[keyword] = keyword_source[keyword] schema["properties"][k] = simple_prop # Final pass: recursively inline all remaining $refs at any depth. diff --git a/test/backends/test_tool_collection_params_unit.py b/test/backends/test_tool_collection_params_unit.py new file mode 100644 index 0000000000..fac73fa1fc --- /dev/null +++ b/test/backends/test_tool_collection_params_unit.py @@ -0,0 +1,362 @@ +# Copyright IBM Corp. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for tool schemas and validation of list, dict and constrained parameters. + +Regression tests for the bug where the schema rebuild for simple parameters +kept only `description`, `type`, `enum` and `default`, dropping `items`, +`additionalProperties` and constraints. The model was never told a list's +element type, and `validate_tool_arguments` (which builds its validator from +the same schema) fell back to `string` elements, so `list[int]` arguments +reached the tool as strings. The schema is shared by every backend via +`convert_tools_to_json`, and the validator runs on every backend. + +See: https://github.com/generative-computing/mellea/issues/1693 +""" + +from typing import Annotated, Any + +import pytest +from pydantic import BaseModel, Field, ValidationError + +from mellea.backends.tools import MelleaTool, validate_tool_arguments + + +class Point(BaseModel): + x: int + y: int + + +def ints_tool(nums: list[int]) -> int: + """Sum some numbers. + + Args: + nums: the numbers + """ + return sum(nums) + + +def floats_tool(nums: list[float]) -> float: + """Sum some floats. + + Args: + nums: the numbers + """ + return sum(nums) + + +def optional_ints_tool(nums: list[int] | None = None) -> int: + """Sum some numbers, if given. + + Args: + nums: the numbers + """ + return sum(nums or []) + + +def bools_tool(flags: list[bool]) -> int: + """Count true flags. + + Args: + flags: the flags + """ + return sum(flags) + + +def nested_ints_tool(rows: list[list[int]]) -> int: + """Sum a matrix. + + Args: + rows: the rows + """ + return sum(sum(row) for row in rows) + + +def points_tool(pts: list[Point]) -> int: + """Count points. + + Args: + pts: the points + """ + return len(pts) + + +def optional_points_tool(pts: list[Point] | None = None) -> int: + """Count points, if given. + + Args: + pts: the points + """ + return len(pts or []) + + +def counts_tool(counts: dict[str, int]) -> int: + """Total some counts. + + Args: + counts: counts by name + """ + return sum(counts.values()) + + +def bounded_tool(score: Annotated[int, Field(ge=0, le=10)]) -> int: + """Record a score. + + Args: + score: the score + """ + return score + + +def optional_bounded_tool(score: Annotated[int, Field(ge=0)] | None = None) -> int: + """Record a score, if given. + + Args: + score: the score + """ + return score or 0 + + +def non_empty_tool(nums: Annotated[list[int], Field(min_length=1)]) -> int: + """Sum at least one number. + + Args: + nums: the numbers + """ + return sum(nums) + + +def bounded_str_tool( + code: Annotated[str, Field(max_length=5, pattern="^[A-Z]+$")], +) -> str: + """Look up a code. + + Args: + code: the code + """ + return code + + +def any_list_tool(values: list[Any]) -> int: + """Count values. + + Args: + values: the values + """ + return len(values) + + +def strings_tool(words: list[str]) -> int: + """Count words. + + Args: + words: the words + """ + return len(words) + + +def _prop(func, param): + """Return the serialized schema for one parameter of a callable's tool.""" + return MelleaTool.from_callable(func).as_json_tool["function"]["parameters"][ + "properties" + ][param] + + +def _required(func): + """Return the `required` list of a callable's tool schema.""" + params = MelleaTool.from_callable(func).as_json_tool["function"]["parameters"] + return params.get("required", []) + + +# ============================================================================ +# Schema generation: what every backend sends to the model +# ============================================================================ + + +class TestElementTypesInSchema: + """Element types of lists and dicts must reach the model.""" + + def test_list_int_property_shape(self): + assert _prop(ints_tool, "nums") == { + "description": "the numbers", + "type": "array", + "items": {"type": "integer"}, + } + + def test_list_float_items(self): + assert _prop(floats_tool, "nums")["items"] == {"type": "number"} + + def test_list_bool_items(self): + assert _prop(bools_tool, "flags")["items"] == {"type": "boolean"} + + def test_list_str_items(self): + assert _prop(strings_tool, "words")["items"] == {"type": "string"} + + def test_nested_list_items(self): + assert _prop(nested_ints_tool, "rows")["items"] == { + "type": "array", + "items": {"type": "integer"}, + } + + def test_optional_list_items_taken_from_array_branch(self): + prop = _prop(optional_ints_tool, "nums") + assert prop["type"] == "array" + assert prop["items"] == {"type": "integer"} + assert "nums" not in _required(optional_ints_tool) + + def test_list_of_models_items_inlined(self): + items = _prop(points_tool, "pts")["items"] + assert "$ref" not in items + assert items["type"] == "object" + assert set(items["properties"]) == {"x", "y"} + assert items["properties"]["x"]["type"] == "integer" + assert set(items["required"]) == {"x", "y"} + + def test_optional_list_of_models_items_inlined(self): + prop = _prop(optional_points_tool, "pts") + assert prop["type"] == "array" + assert "$ref" not in prop["items"] + assert set(prop["items"]["properties"]) == {"x", "y"} + assert "pts" not in _required(optional_points_tool) + + def test_dict_value_type(self): + prop = _prop(counts_tool, "counts") + assert prop["type"] == "object" + assert prop["additionalProperties"] == {"type": "integer"} + + def test_any_list_keeps_unconstrained_items(self): + assert _prop(any_list_tool, "values")["items"] == {} + + +class TestConstraintsInSchema: + """`Field` constraints must reach the model.""" + + def test_numeric_bounds(self): + prop = _prop(bounded_tool, "score") + assert prop["type"] == "integer" + assert prop["minimum"] == 0 + assert prop["maximum"] == 10 + + def test_optional_numeric_bound_taken_from_branch(self): + prop = _prop(optional_bounded_tool, "score") + assert prop["type"] == "integer" + assert prop["minimum"] == 0 + + def test_list_length(self): + prop = _prop(non_empty_tool, "nums") + assert prop["minItems"] == 1 + assert prop["items"] == {"type": "integer"} + + def test_string_length_and_pattern(self): + prop = _prop(bounded_str_tool, "code") + assert prop["maxLength"] == 5 + assert prop["pattern"] == "^[A-Z]+$" + + def test_title_not_carried(self): + """Pydantic's per-property `title` stays stripped, as before.""" + assert "title" not in _prop(bounded_tool, "score") + assert "title" not in _prop(ints_tool, "nums") + + +# ============================================================================ +# Validation: what the tool actually receives +# ============================================================================ + + +class TestListValidation: + """Correct list arguments must pass validation with their element types intact.""" + + def test_list_int_not_coerced_to_str(self): + tool = MelleaTool.from_callable(ints_tool) + validated = validate_tool_arguments(tool, {"nums": [1, 2]}) + assert validated == {"nums": [1, 2]} + assert all(type(n) is int for n in validated["nums"]) + + def test_list_int_tool_runs(self): + """The reproducer from the issue: the tool must not raise TypeError.""" + tool = MelleaTool.from_callable(ints_tool) + validated = validate_tool_arguments(tool, {"nums": [1, 2]}) + assert tool.run(**validated) == 3 + + def test_list_int_elements_coerced_from_str(self): + tool = MelleaTool.from_callable(ints_tool) + validated = validate_tool_arguments(tool, {"nums": ["1", "2"]}) + assert validated == {"nums": [1, 2]} + + def test_list_float(self): + tool = MelleaTool.from_callable(floats_tool) + validated = validate_tool_arguments(tool, {"nums": [1.5]}) + assert validated == {"nums": [1.5]} + assert type(validated["nums"][0]) is float + + def test_optional_list_int(self): + tool = MelleaTool.from_callable(optional_ints_tool) + validated = validate_tool_arguments(tool, {"nums": [1, 2]}) + assert validated == {"nums": [1, 2]} + assert all(type(n) is int for n in validated["nums"]) + + def test_optional_list_int_none(self): + tool = MelleaTool.from_callable(optional_ints_tool) + assert validate_tool_arguments(tool, {"nums": None}) == {"nums": None} + + @pytest.mark.parametrize( + ("func", "args"), + [ + (bools_tool, {"flags": [True, False]}), + (nested_ints_tool, {"rows": [[1], [2, 3]]}), + (points_tool, {"pts": [{"x": 1, "y": 2}]}), + (optional_points_tool, {"pts": [{"x": 1, "y": 2}]}), + ], + ids=["list_bool", "list_list_int", "list_model", "optional_list_model"], + ) + def test_previously_rejected_types_validate(self, func, args): + """These failed validation and were passed through unchecked. + + `strict=True` raises on a validation failure instead of falling back + to the original arguments, so passing here proves they validated. + """ + tool = MelleaTool.from_callable(func) + assert validate_tool_arguments(tool, args, strict=True) == args + + def test_list_of_models_rejects_missing_field(self): + """Element validation is real: a point missing `y` is rejected.""" + tool = MelleaTool.from_callable(points_tool) + with pytest.raises(ValidationError, match=r"pts\.0\.y"): + validate_tool_arguments(tool, {"pts": [{"x": 1}]}, strict=True) + + def test_list_str_still_coerces_numbers(self): + """Existing `list[str]` coercion of numbers to strings is unchanged.""" + tool = MelleaTool.from_callable(strings_tool) + validated = validate_tool_arguments(tool, {"words": ["a", 1]}) + assert validated == {"words": ["a", "1"]} + + def test_any_list_elements_untouched(self): + tool = MelleaTool.from_callable(any_list_tool) + validated = validate_tool_arguments( + tool, {"values": [1, "a", True]}, strict=True + ) + assert validated == {"values": [1, "a", True]} + assert type(validated["values"][0]) is int + + def test_array_without_items_elements_untouched(self): + """A schema from outside `from_callable` may omit `items` entirely. + + An array with no `items` places no constraint on its elements, so they + must not be coerced to strings. + """ + as_json_tool = { + "type": "function", + "function": { + "name": "external", + "description": "An externally defined tool.", + "parameters": { + "type": "object", + "properties": {"values": {"type": "array"}}, + "required": ["values"], + }, + }, + } + tool = MelleaTool("external", lambda values: values, as_json_tool) + validated = validate_tool_arguments(tool, {"values": [1, 2.5]}) + assert validated == {"values": [1, 2.5]} + assert type(validated["values"][0]) is int From 779532435c45c9e5f66e520d294643f627f28051 Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Tue, 29 Sep 2026 12:36:25 +0100 Subject: [PATCH 2/5] fix(tools): accept an explicit null for optional tool parameters convert_function_to_ollama_tool drops null from a simple Optional type, so `limit: int | None = None` reaches validate_tool_arguments as a non-required integer. The validator built that field as `int` with a None default, so an explicit None failed: strict mode raised, and lenient mode (used by every backend) fell back to the unvalidated arguments, losing every other coercion in the call. Type non-required validator fields as `T | None`. The existing explicit-null tests passed only through the lenient fallback; they now run with strict=True, and a new test checks the other arguments are still coerced alongside the null. Refs #1693 Assisted-by: Claude Code Signed-off-by: Nigel Jones --- mellea/backends/tools.py | 7 ++- .../test_tool_collection_params_unit.py | 3 +- .../test_tool_validation_integration.py | 45 +++++++++++++++++-- 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/mellea/backends/tools.py b/mellea/backends/tools.py index 8d29b679c5..1fc9c8502c 100644 --- a/mellea/backends/tools.py +++ b/mellea/backends/tools.py @@ -723,8 +723,11 @@ def _build_pydantic_type_from_schema(schema: dict[str, Any]) -> Any: # Required parameter field_definitions[param_name] = (param_type, ...) else: - # Optional parameter (default to None) - field_definitions[param_name] = (param_type, None) + # Optional parameter (default to None). Accept an explicit null + # too: the schema drops `null` from a simple Optional type, so + # `limit: int | None = None` would otherwise reject None and a + # lenient call would fall back to the unvalidated arguments. + field_definitions[param_name] = (param_type | None, None) # Configure model for type coercion if requested if coerce_types: diff --git a/test/backends/test_tool_collection_params_unit.py b/test/backends/test_tool_collection_params_unit.py index fac73fa1fc..7ce3735b93 100644 --- a/test/backends/test_tool_collection_params_unit.py +++ b/test/backends/test_tool_collection_params_unit.py @@ -297,7 +297,8 @@ def test_optional_list_int(self): def test_optional_list_int_none(self): tool = MelleaTool.from_callable(optional_ints_tool) - assert validate_tool_arguments(tool, {"nums": None}) == {"nums": None} + validated = validate_tool_arguments(tool, {"nums": None}, strict=True) + assert validated == {"nums": None} @pytest.mark.parametrize( ("func", "args"), diff --git a/test/backends/test_tool_validation_integration.py b/test/backends/test_tool_validation_integration.py index a2dcadf337..a1ae14426a 100644 --- a/test/backends/test_tool_validation_integration.py +++ b/test/backends/test_tool_validation_integration.py @@ -51,6 +51,16 @@ def optional_tool(required: str, optional: str | None = None) -> str: return f"{required}:{optional or 'none'}" +def limit_tool(count: int, limit: int | None = None) -> int: + """Tool with a required int and an optional int. + + Args: + count: How many items to return + limit: An optional upper bound + """ + return count if limit is None else min(count, limit) + + def union_tool(value: str | int) -> str: """Tool with union type parameter. @@ -252,13 +262,42 @@ def test_optional_param_omitted(self): assert "optional" not in validated def test_optional_param_none(self): - """Test validation when optional parameter is explicitly None.""" + """Test validation when optional parameter is explicitly None. + + `strict=True` raises instead of falling back to the original + arguments, so this proves the None validated rather than slipped + through the lenient fallback. + """ args = {"required": "value1", "optional": None} tool = MelleaTool.from_callable(optional_tool) + validated = validate_tool_arguments(tool, args, strict=True) + + assert validated == {"required": "value1", "optional": None} + + def test_optional_int_none_strict(self): + """An explicit None for `int | None` validates. + + The schema drops `null` from a simple Optional type, so the validator + must still accept it for a non-required field. + """ + args = {"count": 3, "limit": None} + tool = MelleaTool.from_callable(limit_tool) + validated = validate_tool_arguments(tool, args, strict=True) + + assert validated == {"count": 3, "limit": None} + + def test_optional_none_keeps_other_coercions(self): + """An explicit None must not make lenient mode drop the whole call. + + A failed validation returns the original arguments, which would leave + `count` as the string "3". + """ + args = {"count": "3", "limit": None} + tool = MelleaTool.from_callable(limit_tool) validated = validate_tool_arguments(tool, args) - assert validated["required"] == "value1" - assert validated["optional"] is None + assert validated == {"count": 3, "limit": None} + assert type(validated["count"]) is int class TestDefaultedParameters: From 0aaa7fc94f37ffd24fc585e2336145dd047876e9 Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Tue, 29 Sep 2026 13:38:47 +0100 Subject: [PATCH 3/5] fix(tools): address review findings on tool schema carry-through Flatten discriminated unions exposed by the carried keywords. Copying items and additionalProperties sent list[Pet], dict[str, Pet] and list[Owner] (where a field is a discriminated union) out with raw oneOf and discriminator, which tool APIs reject and the rest of the pipeline strips. Re-run _recursively_flatten_in_properties after the final ref inlining, which also covers a discriminated union in a nested model's field. Only treat a non-required validator field as nullable when its schema has no non-null default. `limit: int | None = None` still accepts an explicit None; `page_size: int = 10` rejects it again in strict mode. Treat any items schema that names no type (annotation-only, or a non-object form such as `true`) as list[Any], not just a missing or empty one. Run the correct-input validation tests with strict=True so they cannot pass through the lenient fallback. Refs #1693 Assisted-by: Claude Code Signed-off-by: Nigel Jones --- mellea/backends/tools.py | 30 +++- .../test_tool_collection_params_unit.py | 162 ++++++++++++++++-- .../test_tool_validation_integration.py | 21 +++ 3 files changed, 193 insertions(+), 20 deletions(-) diff --git a/mellea/backends/tools.py b/mellea/backends/tools.py index 1fc9c8502c..481f6078df 100644 --- a/mellea/backends/tools.py +++ b/mellea/backends/tools.py @@ -632,12 +632,16 @@ def _build_pydantic_type_from_schema(schema: dict[str, Any]) -> Any: **nested_fields, ) - # Handle arrays. A missing or empty `items` schema leaves the elements - # unconstrained; falling through to the `string` default would coerce - # every numeric element to a string under `coerce_numbers_to_str`. + # Handle arrays. An `items` schema that names no type (missing, `{}`, + # annotation-only such as `{"description": ...}`, or a non-object + # form such as `true`) leaves the elements unconstrained; falling + # through to the `string` default would coerce every numeric element + # to a string under `coerce_numbers_to_str`. if json_type == "array": item_schema = schema.get("items") - if not item_schema: + if not isinstance(item_schema, dict) or not any( + key in item_schema for key in ("type", "anyOf", "enum", "const") + ): return list[Any] item_type = _build_pydantic_type_from_schema(item_schema) return list[item_type] # type: ignore @@ -723,11 +727,15 @@ def _build_pydantic_type_from_schema(schema: dict[str, Any]) -> Any: # Required parameter field_definitions[param_name] = (param_type, ...) else: - # Optional parameter (default to None). Accept an explicit null - # too: the schema drops `null` from a simple Optional type, so - # `limit: int | None = None` would otherwise reject None and a - # lenient call would fall back to the unvalidated arguments. - field_definitions[param_name] = (param_type | None, None) + # Optional parameter (default to None). The schema drops `null` + # from a simple Optional type, so treat a field with no non-null + # default as nullable: `limit: int | None = None` then accepts an + # explicit None instead of failing and sending a lenient call back + # to the unvalidated arguments. A field with a real default + # (`page_size: int = 10`) still rejects None. + if param_schema.get("default") is None: + param_type = param_type | None + field_definitions[param_name] = (param_type, None) # Configure model for type coercion if requested if coerce_types: @@ -1542,6 +1550,10 @@ def convert_function_to_ollama_tool( # This catches dangling references in nested model properties that weren't # caught by the earlier single-level ref-inlining passes. _recursively_inline_refs(schema, defs) + # Inlining exposes discriminated unions the pre-pass never saw: inside a + # carried `items` or `additionalProperties` (`list[Pet]`, + # `dict[str, Pet]`) or a nested model's field. Flatten those too. + _recursively_flatten_in_properties(schema, defs) tool = OllamaTool( type="function", diff --git a/test/backends/test_tool_collection_params_unit.py b/test/backends/test_tool_collection_params_unit.py index 7ce3735b93..dd008ac8bf 100644 --- a/test/backends/test_tool_collection_params_unit.py +++ b/test/backends/test_tool_collection_params_unit.py @@ -14,7 +14,8 @@ See: https://github.com/generative-computing/mellea/issues/1693 """ -from typing import Annotated, Any +import json +from typing import Annotated, Any, Literal import pytest from pydantic import BaseModel, Field, ValidationError @@ -27,6 +28,69 @@ class Point(BaseModel): y: int +class Cat(BaseModel): + kind: Literal["cat"] + meow: int + + +class Dog(BaseModel): + kind: Literal["dog"] + bark: int + + +Pet = Annotated[Cat | Dog, Field(discriminator="kind")] + + +class Owner(BaseModel): + name: str + pet: Pet + + +def pets_tool(pets: list[Pet]) -> int: + """Count pets. + + Args: + pets: the pets + """ + return len(pets) + + +def optional_pets_tool(pets: list[Pet] | None = None) -> int: + """Count pets, if given. + + Args: + pets: the pets + """ + return len(pets or []) + + +def named_pets_tool(pets: dict[str, Pet]) -> int: + """Count named pets. + + Args: + pets: pets by name + """ + return len(pets) + + +def owners_tool(owners: list[Owner]) -> int: + """Count owners. + + Args: + owners: the owners + """ + return len(owners) + + +def owner_tool(owner: Owner) -> str: + """Name an owner. + + Args: + owner: the owner + """ + return owner.name + + def ints_tool(nums: list[int]) -> int: """Sum some numbers. @@ -146,6 +210,17 @@ def any_list_tool(values: list[Any]) -> int: return len(values) +def described_any_list_tool( + values: list[Annotated[Any, Field(description="a value")]], +) -> int: + """Count values. + + Args: + values: the values + """ + return len(values) + + def strings_tool(words: list[str]) -> int: """Count words. @@ -228,6 +303,40 @@ def test_any_list_keeps_unconstrained_items(self): assert _prop(any_list_tool, "values")["items"] == {} +class TestDiscriminatedUnionsInCollections: + """Discriminated unions exposed by carried keywords must be flattened. + + Tool APIs reject `oneOf` and `discriminator`, and the rest of the + schema pipeline strips them (see `_flatten_discriminated_union`). + """ + + @pytest.mark.parametrize( + ("func", "param"), + [ + (pets_tool, "pets"), + (optional_pets_tool, "pets"), + (named_pets_tool, "pets"), + (owners_tool, "owners"), + (owner_tool, "owner"), + ], + ids=["list", "optional_list", "dict", "list_of_models", "model_field"], + ) + def test_no_oneof_or_discriminator(self, func, param): + rendered = json.dumps(_prop(func, param)) + assert "oneOf" not in rendered + assert "discriminator" not in rendered + assert "$ref" not in rendered + + def test_list_items_become_inlined_anyof(self): + items = _prop(pets_tool, "pets")["items"] + kinds = {b["properties"]["kind"]["const"] for b in items["anyOf"]} + assert kinds == {"cat", "dog"} + + def test_dict_values_become_inlined_anyof(self): + values = _prop(named_pets_tool, "pets")["additionalProperties"] + assert len(values["anyOf"]) == 2 + + class TestConstraintsInSchema: """`Field` constraints must reach the model.""" @@ -268,14 +377,14 @@ class TestListValidation: def test_list_int_not_coerced_to_str(self): tool = MelleaTool.from_callable(ints_tool) - validated = validate_tool_arguments(tool, {"nums": [1, 2]}) + validated = validate_tool_arguments(tool, {"nums": [1, 2]}, strict=True) assert validated == {"nums": [1, 2]} assert all(type(n) is int for n in validated["nums"]) def test_list_int_tool_runs(self): """The reproducer from the issue: the tool must not raise TypeError.""" tool = MelleaTool.from_callable(ints_tool) - validated = validate_tool_arguments(tool, {"nums": [1, 2]}) + validated = validate_tool_arguments(tool, {"nums": [1, 2]}, strict=True) assert tool.run(**validated) == 3 def test_list_int_elements_coerced_from_str(self): @@ -285,13 +394,13 @@ def test_list_int_elements_coerced_from_str(self): def test_list_float(self): tool = MelleaTool.from_callable(floats_tool) - validated = validate_tool_arguments(tool, {"nums": [1.5]}) + validated = validate_tool_arguments(tool, {"nums": [1.5]}, strict=True) assert validated == {"nums": [1.5]} assert type(validated["nums"][0]) is float def test_optional_list_int(self): tool = MelleaTool.from_callable(optional_ints_tool) - validated = validate_tool_arguments(tool, {"nums": [1, 2]}) + validated = validate_tool_arguments(tool, {"nums": [1, 2]}, strict=True) assert validated == {"nums": [1, 2]} assert all(type(n) is int for n in validated["nums"]) @@ -339,11 +448,42 @@ def test_any_list_elements_untouched(self): assert validated == {"values": [1, "a", True]} assert type(validated["values"][0]) is int - def test_array_without_items_elements_untouched(self): - """A schema from outside `from_callable` may omit `items` entirely. + def test_list_of_discriminated_union_validates(self): + """Elements are validated against the branch the tag selects.""" + tool = MelleaTool.from_callable(pets_tool) + validated = validate_tool_arguments( + tool, {"pets": [{"kind": "cat", "meow": "1"}]}, strict=True + ) + assert validated == {"pets": [{"kind": "cat", "meow": 1}]} + + def test_list_of_discriminated_union_rejects_wrong_branch_fields(self): + tool = MelleaTool.from_callable(pets_tool) + with pytest.raises(ValidationError, match=r"pets\.0\.cat\.meow"): + validate_tool_arguments( + tool, {"pets": [{"kind": "cat", "bark": 1}]}, strict=True + ) + + def test_described_any_list_elements_untouched(self): + """An annotation-only items schema constrains nothing either.""" + tool = MelleaTool.from_callable(described_any_list_tool) + validated = validate_tool_arguments(tool, {"values": [1, 2]}, strict=True) + assert validated == {"values": [1, 2]} + assert type(validated["values"][0]) is int + + @pytest.mark.parametrize( + "array_schema", + [ + {"type": "array"}, + {"type": "array", "items": True}, + {"type": "array", "items": {"description": "a value"}}, + ], + ids=["no_items", "items_true", "annotation_only_items"], + ) + def test_external_unconstrained_array_elements_untouched(self, array_schema): + """A schema from outside `from_callable` may leave `items` unconstrained. - An array with no `items` places no constraint on its elements, so they - must not be coerced to strings. + No `items`, `items: true`, and an items schema naming no type all place + no constraint on the elements, so they must not be coerced to strings. """ as_json_tool = { "type": "function", @@ -352,12 +492,12 @@ def test_array_without_items_elements_untouched(self): "description": "An externally defined tool.", "parameters": { "type": "object", - "properties": {"values": {"type": "array"}}, + "properties": {"values": array_schema}, "required": ["values"], }, }, } tool = MelleaTool("external", lambda values: values, as_json_tool) - validated = validate_tool_arguments(tool, {"values": [1, 2.5]}) + validated = validate_tool_arguments(tool, {"values": [1, 2.5]}, strict=True) assert validated == {"values": [1, 2.5]} assert type(validated["values"][0]) is int diff --git a/test/backends/test_tool_validation_integration.py b/test/backends/test_tool_validation_integration.py index a1ae14426a..6ee7d69545 100644 --- a/test/backends/test_tool_validation_integration.py +++ b/test/backends/test_tool_validation_integration.py @@ -61,6 +61,16 @@ def limit_tool(count: int, limit: int | None = None) -> int: return count if limit is None else min(count, limit) +def paged_tool(query: str, page_size: int = 10) -> str: + """Tool with a non-nullable defaulted parameter. + + Args: + query: The search query + page_size: Results per page + """ + return f"{query}:{page_size}" + + def union_tool(value: str | int) -> str: """Tool with union type parameter. @@ -286,6 +296,17 @@ def test_optional_int_none_strict(self): assert validated == {"count": 3, "limit": None} + def test_null_for_non_nullable_default_rejected_strict(self): + """Only nullable parameters accept None. + + `page_size: int = 10` has a real default, so None is not a valid value + for it and strict mode must still raise. + """ + args = {"query": "q", "page_size": None} + tool = MelleaTool.from_callable(paged_tool) + with pytest.raises(ValidationError, match="page_size"): + validate_tool_arguments(tool, args, strict=True) + def test_optional_none_keeps_other_coercions(self): """An explicit None must not make lenient mode drop the whole call. From 2b4614673f43085e2b4edde8302148ba383dee1e Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Tue, 29 Sep 2026 13:44:31 +0100 Subject: [PATCH 4/5] refactor(tools): tighten comments on schema carry-through Comment-only change; no behaviour change. Refs #1693 Assisted-by: Claude Code Signed-off-by: Nigel Jones --- mellea/backends/tools.py | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/mellea/backends/tools.py b/mellea/backends/tools.py index 481f6078df..897c860abc 100644 --- a/mellea/backends/tools.py +++ b/mellea/backends/tools.py @@ -632,11 +632,9 @@ def _build_pydantic_type_from_schema(schema: dict[str, Any]) -> Any: **nested_fields, ) - # Handle arrays. An `items` schema that names no type (missing, `{}`, - # annotation-only such as `{"description": ...}`, or a non-object - # form such as `true`) leaves the elements unconstrained; falling - # through to the `string` default would coerce every numeric element - # to a string under `coerce_numbers_to_str`. + # Handle arrays. An `items` schema naming no type (missing, `{}`, + # description-only, or `true`) leaves elements unconstrained; the + # `string` default would coerce numbers to strings. if json_type == "array": item_schema = schema.get("items") if not isinstance(item_schema, dict) or not any( @@ -728,11 +726,8 @@ def _build_pydantic_type_from_schema(schema: dict[str, Any]) -> Any: field_definitions[param_name] = (param_type, ...) else: # Optional parameter (default to None). The schema drops `null` - # from a simple Optional type, so treat a field with no non-null - # default as nullable: `limit: int | None = None` then accepts an - # explicit None instead of failing and sending a lenient call back - # to the unvalidated arguments. A field with a real default - # (`page_size: int = 10`) still rejects None. + # from simple Optional types, so a field with no non-null default + # is taken as nullable; `page_size: int = 10` still rejects None. if param_schema.get("default") is None: param_type = param_type | None field_definitions[param_name] = (param_type, None) @@ -1361,10 +1356,8 @@ def _inline(branch: dict) -> dict: return out -# JSON Schema keywords copied from Pydantic's schema onto a rebuilt simple -# property. Without them the model is never told a list's or dict's element -# type or a field's constraints, and `validate_tool_arguments`, which builds -# its validator from the same schema, loses the element types too. +# JSON Schema keywords copied onto a rebuilt simple property, so the model and +# `validate_tool_arguments` both see element types and constraints. _CARRIED_SCHEMA_KEYWORDS = ( "items", "additionalProperties", @@ -1532,10 +1525,9 @@ def convert_function_to_ollama_tool( # from scratch would otherwise drop it. if "default" in v: simple_prop["default"] = v["default"] - # Carry element types and constraints across too. For an Optional - # parameter Pydantic puts them on the non-null anyOf branch; with - # several non-null branches they belong to one branch each, so - # none are merged onto the flattened property. + # Carry element types and constraints. For Optional, Pydantic puts + # them on the single non-null anyOf branch; with several branches + # they can't be merged onto one property. if "anyOf" in v: non_null = [s for s in v["anyOf"] if s.get("type") != "null"] keyword_source = non_null[0] if len(non_null) == 1 else {} @@ -1550,9 +1542,8 @@ def convert_function_to_ollama_tool( # This catches dangling references in nested model properties that weren't # caught by the earlier single-level ref-inlining passes. _recursively_inline_refs(schema, defs) - # Inlining exposes discriminated unions the pre-pass never saw: inside a - # carried `items` or `additionalProperties` (`list[Pet]`, - # `dict[str, Pet]`) or a nested model's field. Flatten those too. + # Inlining can expose discriminated unions the pre-pass missed + # (`list[Pet]`, a nested model's field); flatten those too. _recursively_flatten_in_properties(schema, defs) tool = OllamaTool( From e00701a3fb0a59326619d6b4087a7f6c77d9a27d Mon Sep 17 00:00:00 2001 From: Nigel Jones Date: Tue, 29 Sep 2026 14:02:02 +0100 Subject: [PATCH 5/5] fix(tools): keep lenient validation from raising on unbuildable schemas validate_tool_arguments built its validator model outside the try, so a schema it could not build (boolean sub-schemas such as properties: {"a": true}, valid JSON Schema from MCP servers) raised TypeError or AttributeError even with strict=False. The HF path does not catch it, so generation failed; the OpenAI-compatible path dropped the tool call as malformed on TypeError and failed on AttributeError. Build the model inside the existing try, so lenient mode logs and returns the original arguments and strict mode still raises. Also trim test docstrings to one line. Refs #1693 Assisted-by: Claude Code Signed-off-by: Nigel Jones --- mellea/backends/tools.py | 69 ++++++++++--------- .../test_tool_collection_params_unit.py | 18 +---- .../test_tool_validation_integration.py | 64 +++++++++++------ 3 files changed, 81 insertions(+), 70 deletions(-) diff --git a/mellea/backends/tools.py b/mellea/backends/tools.py index 897c860abc..c3936e09f9 100644 --- a/mellea/backends/tools.py +++ b/mellea/backends/tools.py @@ -714,45 +714,46 @@ def _build_pydantic_type_from_schema(schema: dict[str, Any]) -> Any: # Simple type mapping return JSON_TYPE_TO_PYTHON.get(json_type, Any) - # Build Pydantic model from JSON schema - field_definitions: dict[str, Any] = {} + # Build inside the try so an unbuildable schema takes the lenient fallback. + try: + # Build Pydantic model from JSON schema + field_definitions: dict[str, Any] = {} - for param_name, param_schema in properties.items(): - param_type = _build_pydantic_type_from_schema(param_schema) + for param_name, param_schema in properties.items(): + param_type = _build_pydantic_type_from_schema(param_schema) - # Determine if parameter is required - if param_name in required_fields: - # Required parameter - field_definitions[param_name] = (param_type, ...) + # Determine if parameter is required + if param_name in required_fields: + # Required parameter + field_definitions[param_name] = (param_type, ...) + else: + # Optional parameter (default to None). The schema drops `null` + # from simple Optional types, so a field with no non-null default + # is taken as nullable; `page_size: int = 10` still rejects None. + if param_schema.get("default") is None: + param_type = param_type | None + field_definitions[param_name] = (param_type, None) + + # Configure model for type coercion if requested + if coerce_types: + model_config = ConfigDict( + str_strip_whitespace=True, + strict=False, # Allow type coercion + extra="forbid" if strict else "allow", # Handle extra fields + # Enable coercion modes for common LLM output issues + coerce_numbers_to_str=True, # Allow int/float -> str + ) else: - # Optional parameter (default to None). The schema drops `null` - # from simple Optional types, so a field with no non-null default - # is taken as nullable; `page_size: int = 10` still rejects None. - if param_schema.get("default") is None: - param_type = param_type | None - field_definitions[param_name] = (param_type, None) - - # Configure model for type coercion if requested - if coerce_types: - model_config = ConfigDict( - str_strip_whitespace=True, - strict=False, # Allow type coercion - extra="forbid" if strict else "allow", # Handle extra fields - # Enable coercion modes for common LLM output issues - coerce_numbers_to_str=True, # Allow int/float -> str - ) - else: - model_config = ConfigDict( - strict=True, # No coercion - extra="forbid" if strict else "allow", - ) + model_config = ConfigDict( + strict=True, # No coercion + extra="forbid" if strict else "allow", + ) - # Create dynamic Pydantic model for validation - ValidatorModel = create_model( - f"{tool_name}_Validator", __config__=model_config, **field_definitions - ) + # Create dynamic Pydantic model for validation + ValidatorModel = create_model( + f"{tool_name}_Validator", __config__=model_config, **field_definitions + ) - try: # Validate using Pydantic validated_model = ValidatorModel(**args) # Only emit fields the model actually sent. A bare model_dump() would diff --git a/test/backends/test_tool_collection_params_unit.py b/test/backends/test_tool_collection_params_unit.py index dd008ac8bf..b28416d177 100644 --- a/test/backends/test_tool_collection_params_unit.py +++ b/test/backends/test_tool_collection_params_unit.py @@ -304,11 +304,7 @@ def test_any_list_keeps_unconstrained_items(self): class TestDiscriminatedUnionsInCollections: - """Discriminated unions exposed by carried keywords must be flattened. - - Tool APIs reject `oneOf` and `discriminator`, and the rest of the - schema pipeline strips them (see `_flatten_discriminated_union`). - """ + """Discriminated unions in carried keywords are flattened for tool APIs.""" @pytest.mark.parametrize( ("func", "param"), @@ -420,11 +416,7 @@ def test_optional_list_int_none(self): ids=["list_bool", "list_list_int", "list_model", "optional_list_model"], ) def test_previously_rejected_types_validate(self, func, args): - """These failed validation and were passed through unchecked. - - `strict=True` raises on a validation failure instead of falling back - to the original arguments, so passing here proves they validated. - """ + """These used to skip validation; `strict=True` proves they now pass it.""" tool = MelleaTool.from_callable(func) assert validate_tool_arguments(tool, args, strict=True) == args @@ -480,11 +472,7 @@ def test_described_any_list_elements_untouched(self): ids=["no_items", "items_true", "annotation_only_items"], ) def test_external_unconstrained_array_elements_untouched(self, array_schema): - """A schema from outside `from_callable` may leave `items` unconstrained. - - No `items`, `items: true`, and an items schema naming no type all place - no constraint on the elements, so they must not be coerced to strings. - """ + """Unconstrained external `items` must not coerce elements to strings.""" as_json_tool = { "type": "function", "function": { diff --git a/test/backends/test_tool_validation_integration.py b/test/backends/test_tool_validation_integration.py index 6ee7d69545..609788ac32 100644 --- a/test/backends/test_tool_validation_integration.py +++ b/test/backends/test_tool_validation_integration.py @@ -272,12 +272,7 @@ def test_optional_param_omitted(self): assert "optional" not in validated def test_optional_param_none(self): - """Test validation when optional parameter is explicitly None. - - `strict=True` raises instead of falling back to the original - arguments, so this proves the None validated rather than slipped - through the lenient fallback. - """ + """Explicit None for `str | None` validates (strict, so no fallback).""" args = {"required": "value1", "optional": None} tool = MelleaTool.from_callable(optional_tool) validated = validate_tool_arguments(tool, args, strict=True) @@ -285,11 +280,7 @@ def test_optional_param_none(self): assert validated == {"required": "value1", "optional": None} def test_optional_int_none_strict(self): - """An explicit None for `int | None` validates. - - The schema drops `null` from a simple Optional type, so the validator - must still accept it for a non-required field. - """ + """An explicit None for `int | None` validates.""" args = {"count": 3, "limit": None} tool = MelleaTool.from_callable(limit_tool) validated = validate_tool_arguments(tool, args, strict=True) @@ -297,22 +288,14 @@ def test_optional_int_none_strict(self): assert validated == {"count": 3, "limit": None} def test_null_for_non_nullable_default_rejected_strict(self): - """Only nullable parameters accept None. - - `page_size: int = 10` has a real default, so None is not a valid value - for it and strict mode must still raise. - """ + """A real default (`page_size: int = 10`) still rejects None.""" args = {"query": "q", "page_size": None} tool = MelleaTool.from_callable(paged_tool) with pytest.raises(ValidationError, match="page_size"): validate_tool_arguments(tool, args, strict=True) def test_optional_none_keeps_other_coercions(self): - """An explicit None must not make lenient mode drop the whole call. - - A failed validation returns the original arguments, which would leave - `count` as the string "3". - """ + """A None must not make lenient mode drop the other coercions.""" args = {"count": "3", "limit": None} tool = MelleaTool.from_callable(limit_tool) validated = validate_tool_arguments(tool, args) @@ -435,9 +418,48 @@ def test_union_with_string_number(self): assert validated["value"] in ["42", 42] +def _external_tool(value_schema: Any) -> MelleaTool: + """Build a tool whose single `value` parameter uses a raw JSON schema.""" + as_json_tool = { + "type": "function", + "function": { + "name": "external", + "description": "An externally defined tool.", + "parameters": { + "type": "object", + "properties": {"value": value_schema}, + "required": ["value"], + }, + }, + } + return MelleaTool("external", lambda value: value, as_json_tool) + + +# Boolean sub-schemas are valid JSON Schema (e.g. from MCP) but broke the validator. +BOOLEAN_SUBSCHEMAS = [ + pytest.param(True, 1, id="property_true"), + pytest.param({"type": "object", "properties": {"a": True}}, {"a": 1}, id="nested"), + pytest.param({"anyOf": [True, {"type": "null"}]}, 1, id="anyof"), +] + + class TestEdgeCases: """Test edge cases.""" + @pytest.mark.parametrize(("value_schema", "value"), BOOLEAN_SUBSCHEMAS) + def test_unsupported_schema_lenient_returns_original_args( + self, value_schema, value + ): + """An unbuildable schema falls back to the original args, not a raise.""" + tool = _external_tool(value_schema) + assert validate_tool_arguments(tool, {"value": value}) == {"value": value} + + @pytest.mark.parametrize(("value_schema", "value"), BOOLEAN_SUBSCHEMAS) + def test_unsupported_schema_strict_raises(self, value_schema, value): + tool = _external_tool(value_schema) + with pytest.raises((TypeError, AttributeError)): + validate_tool_arguments(tool, {"value": value}, strict=True) + def test_no_parameters_tool(self): """Test validation with no-parameter tool.""" args = {}