fix: tool call arguments by akihikokuroda · Pull Request #896 · generative-computing/mellea

akihikokuroda · 2026-04-22T00:40:37Z

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 · 2026-04-22T00:40:57Z

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

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

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?

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.
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.
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.

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:

def f(email: Email) — required BaseModel param; confirms $ref is resolved and validate_tool_arguments accepts {"to": "a@b.com", "subject": "hi", "body": "..."}
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
def f(person: Person) where Person has address: Address — confirms two-level nesting works end-to-end

ajbozarth · 2026-04-22T23:32:10Z

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

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

ajbozarth

Both reviews addressed, two nits below.

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

ajbozarth · 2026-04-23T01:46:56Z

+        reason="Nested model resolution not yet implemented. "
+        "This test documents the expected behavior once recursive $ref resolution is added. "
+        "Currently fails because Address remains as a dangling $ref inside Person's schema. "
+        "https://github.com/generative-computing/mellea/issues/404 for implementation details."


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

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

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.

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.

Looking at the current state: the URL has been updated from #404 (the issue this PR closes) to #911 (the tracking issue for the nested-model follow-up), so the substantive concern here is addressed — the skip reason now points to an issue that'll remain open after merge.

One small cosmetic thing left over from the earlier iterations: the phrasing "NESTED_MODEL_RESOLUTION_ISSUE.md in https://github.com/generative-computing/mellea/issues/911" reads a bit oddly since that .md file isn't in the repo. Could be tightened to just "See #911 for implementation details." before merge. Not a blocker.

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

planetf1 · 2026-04-23T10:41:20Z

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?

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

jakelorocco · 2026-04-27T19:55:56Z

+    out, _ = await react(
+        goal="Write an email about the Mellea python library to Jake with the subject 'cool library'.",
+        context=ChatContext(),
+        backend=m.backend,
+        tools=[search_tool],
+        format=Email,
+        loop_budget=20,
+    )
+    print(out)


I think this should remain commented out. It's just showing that there's another way of using the function; unless you explicitly want to run both and compare the output.

jakelorocco · 2026-04-27T19:57:02Z

        An ``OllamaTool`` instance representing the function as an OpenAI-compatible
        tool schema.
    """
+    import copy


Is there a reason this isn't a top level import?

No, I'll move it.

jakelorocco · 2026-04-27T19:57:59Z

+    def resolve_ref(ref_path: str, defs: dict) -> dict:
+        """Resolve a $ref path like '#/$defs/Email' to the actual schema."""
+        if ref_path.startswith("#/$defs/"):
+            def_name = ref_path.split("/")[-1]
+            return defs.get(def_name, {})
+        elif ref_path.startswith("#/definitions/"):
+            def_name = ref_path.split("/")[-1]
+            return defs.get(def_name, {})
+        return {}
+
+    # Helper to check if anyOf represents a complex union (not just Optional[primitive])
+    def _is_complex_anyof(v: dict) -> bool:
+        """Check if anyOf contains complex types (refs or nested objects)."""
+        any_of_schemas = v.get("anyOf", [])
+        for sub_schema in any_of_schemas:
+            # Skip null types - they just indicate optionality
+            if sub_schema.get("type") == "null":
+                continue
+            # Check for references or nested properties
+            if "$ref" in sub_schema or "properties" in sub_schema:
+                return True
+        return False


Should these be defined within this function? Is there a reason they aren't top level declarations?

That way we can have unit tests for these functions as well.

I'll move it out and add unit tests for it.

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

planetf1 · 2026-05-01T10:23:07Z

+        # For nested objects, we expect either:
+        # 1. type: "object" with nested properties
+        # 2. A reference to the Email schema
+        if email_schema["type"] == "object":


Minor: now that the fix guarantees email_schema["type"] == "object" (the whole point of the PR is that the $ref is inlined), this if is dead code — and if a future change regressed the type, the inner assertions would be silently skipped and the test would still pass. Same pattern a few lines below in test_nested_basemodel_schema at line 164 (if person_schema["type"] == "object":) and line 173 (if address_schema.get("type") == "object":).

Suggest making all three unconditional assert so the tests actually fail on regression. Happy for this to land as-is and file a follow-up if you'd prefer — just flagging since the current shape won't catch what I think it's meant to catch.

(Unrelated to this test but for reference while you're in the area: the discriminator-union gap I mentioned on the allOf thread is tracked in #989.)

planetf1

A few minor comments but overall LGTM.
Did open #989 to address another hole I found in tool parms

akihikokuroda requested a review from a team as a code owner April 22, 2026 00:40

akihikokuroda requested review from ajbozarth and nrfulton April 22, 2026 00:40

akihikokuroda changed the title ~~bug: fix tool call arguments~~ fix: fix tool call arguments Apr 22, 2026

github-actions Bot added the bug Something isn't working label Apr 22, 2026

akihikokuroda changed the title ~~fix: fix tool call arguments~~ fix: tool call arguments Apr 22, 2026

fix tool calling argument

e1bec38

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

akihikokuroda force-pushed the toolparams branch from 0a6b036 to e1bec38 Compare April 22, 2026 16:09

akihikokuroda requested a review from jakelorocco April 22, 2026 18:17

planetf1 requested changes Apr 22, 2026

View reviewed changes

Comment thread mellea/backends/tools.py Outdated

Comment thread mellea/backends/tools.py Outdated

ajbozarth requested changes Apr 22, 2026

View reviewed changes

Comment thread mellea/backends/tools.py Outdated

Comment thread mellea/backends/tools.py

Comment thread mellea/backends/tools.py Outdated

Comment thread mellea/backends/tools.py Outdated

review comments

8cdef61

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

ajbozarth approved these changes Apr 23, 2026

View reviewed changes

Comment thread test/backends/test_pydantic_tool_parameters.py Outdated

Comment thread test/backends/test_pydantic_tool_parameters.py Outdated

Assisted-by: IBM Bob

eb7c4a6

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

ajbozarth reviewed Apr 23, 2026

View reviewed changes

Assisted-by: IBM Bob

0fb1b6e

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

akihikokuroda requested a review from planetf1 April 23, 2026 16:31

add tracking issue number

c2963fa

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

jakelorocco reviewed Apr 27, 2026

View reviewed changes

review comments

1e78c81

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

akihikokuroda requested a review from jakelorocco April 27, 2026 22:13

planetf1 reviewed Apr 28, 2026

View reviewed changes

Comment thread mellea/backends/tools.py

planetf1 reviewed Apr 28, 2026

View reviewed changes

Comment thread mellea/backends/tools.py

planetf1 reviewed Apr 28, 2026

View reviewed changes

Comment thread mellea/backends/tools.py Outdated

planetf1 reviewed Apr 28, 2026

View reviewed changes

Comment thread mellea/backends/tools.py

planetf1 reviewed Apr 28, 2026

View reviewed changes

Comment thread test/backends/test_pydantic_tool_parameters.py

review comment

0831bcc

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

review comment

891b72a

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>

akihikokuroda requested a review from planetf1 April 28, 2026 18:10

planetf1 mentioned this pull request May 1, 2026

Discriminated-union tool parameters lose their schema and fail validation #989

Open

planetf1 reviewed May 1, 2026

View reviewed changes

planetf1 approved these changes May 1, 2026

View reviewed changes

akihikokuroda added this pull request to the merge queue May 1, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 1, 2026

Conversation

akihikokuroda commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Misc PR

Type of PR

Description

Testing

Attribution

Uh oh!

github-actions Bot commented Apr 22, 2026

Uh oh!

planetf1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajbozarth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajbozarth commented Apr 22, 2026

Uh oh!

ajbozarth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

planetf1 commented Apr 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

planetf1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

akihikokuroda commented Apr 22, 2026 •

edited

Loading