fix: tool call arguments

[akihikokuroda]

Misc PR

Type of PR

• Bug Fix
• New Feature
• Documentation
• Other

Description

• Link to Issue: Fix: fix: tool requests with pydantic models as parameters appear to be broken #404

Testing

• Tests added to the respective file if code was changed
• New code has 100% coverage if code as added
• Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Attribution

• AI coding assistants used

[github-actions]

The PR description has been updated. Please fill out the template for your PR to be reviewed.

[planetf1]

I think the Optional fix is needed - the nested issue isn't a regresion, but worth tracking in a new issue at least

Can we add test cases for the function to prevent regressions?

1. def f(email: Email) — required BaseModel param
   Confirms the core fix works: email is inlined in the schema (no $ref in the output), and validate_tool_arguments accepts {"to": "a@b.com", "subject": "hi"} without error.
2. def f(x: str, y: str | None = None) — Optional scalar
   Confirms Optional params still work: y must be absent from required and the schema type must be "string", not a raw anyOf structure.
3. def f(person: Person) where Person has address: Address
   Confirms nested models work end-to-end: both Person and Address fully inlined in the schema, and validate_tool_arguments accepts a nested dict without a ValidationError.

[planetf1]

There seems to be an issue here in that the 'anyOf' in v wll catch Optional parms and needs narrowing since they need to be removed - and I expect Optional parms are common in real tools

--

  elif "properties" in v or "allOf" in v or "anyOf" in v:
      if parsed_docstring.get(k):
          v["description"] = parsed_docstring[k]
      schema["properties"][k] = v          # preserved as-is

Pydantic v2 serialises str | None as {"anyOf": [{"type": "string"}, {"type": "null"}]}. That hits "anyOf" in v — so this branch fires for every Optional parameter, not just complex nested types.

The else branch (line 968) that previously handled this is now unreachable for any anyOf schema. Specifically, lines 975–978 — which remove nullable params from required — are dead code for this case.

Concrete result

  For def f(x: str, y: str | None = None):

• Before: y removed from required, schema flattened to "type": "string"
• After: y stays in required, raw anyOf preserved in the schema

Ollama receives required: ["x", "y"] for a parameter that was optional. Every existing tool with any Optional or X | None param is silently broken.

Fix

Narrow the elif to complex anyOf only — those where sub-schemas contain $ref or nested properties:

  def _is_complex_anyof(v: dict) -> bool:
      return any("$ref" in s or "properties" in s for s in v.get("anyOf", []))

  elif "properties" in v or "allOf" in v or ("anyOf" in v and _is_complex_anyof(v)):

This lets Optional[str] fall through to the else branch as before, while Optional[SomeModel] is correctly preserved.

[planetf1]

Not a regression, but an issue we have (could move to new issue?) - not a blocker - more subtle found during code review

..the fix doesn't extend to models that contain other models.

When Pydantic generates a schema for a nested type like Person (which has an address: Address field), resolve_ref inlines Person at the top level but leaves "address": {"$ref": "#/$defs/Address"} unresolved inside it. Ollama receives a dangling $ref it can't follow.

The validation side has the same gap — _build_pydantic_type_from_schema has no $ref case, so it defaults json_type to "string" for any sub-schema that's a reference. Passing a nested dict then fails with a Pydantic ValidationError.

Two things needed:

• resolve_ref (or a wrapper) should recursively walk the inlined schema and resolve any further $refs it finds — with a visited set to guard against cycles.
• _build_pydantic_type_from_schema needs a $ref branch that looks up the definition in defs and recurses. defs is currently only in scope inside convert_function_to_ollama_tool; it'll need to be passed in or closed over.

[ajbozarth]

Fixes the $ref inlining problem and extends validate_tool_arguments to handle the resulting nested schemas — the approach makes sense. Main blocker before merge is missing tests — the PR template has "Tests added" unchecked. The fix touches a pure function that's straightforward to unit test. Three cases would give good coverage:

1. def f(email: Email) — required BaseModel param; confirms $ref is resolved and validate_tool_arguments accepts {"to": "a@b.com", "subject": "hi", "body": "..."}
2. def f(x: str, y: str | None = None) — Optional scalar regression; y must be absent from required and schema type must be "string", not raw anyOf
3. def f(person: Person) where Person has address: Address — confirms two-level nesting works end-to-end

[ajbozarth]

My Claude review hit a lot of the same points as @planetf1 's review did, but overall this is looking good outside the missing tests

[ajbozarth]

Both reviews addressed, two nits below.

[ajbozarth]

isn't this url for the issue this will close?

[akihikokuroda]

The "NESTED_MODEL_RESOLUTION_ISSUE.md" is attached to the issue.

[ajbozarth]

why not open a GitHub issue for that? having a md plan file linked in an issue is pretty sub-par. Or if keeping that md file is important for some reason putting it in the repo's dev docs in this PR would still be better.

[planetf1]

I too think we should raise an issue to track this - I expect as mellea usage increases having nested models is going to be more relevant.

[planetf1]

Thanks for the updates @akihikokuroda looking good - I think we need to track the nested model aspect, and also the commits still refer to 'Assisted-By Bob' - that should be in the commit message but it seems odd as the first line. That could be fixed when merging via the squash commit, but it's probably safer to update the commits before?