Skip to content

fix: Agent clone and serialization with a tools-less chat generator - #12365

Open
lets-order-some-fries wants to merge 1 commit into
deepset-ai:mainfrom
lets-order-some-fries:fix/agent-empty-tools-list-roundtrip
Open

fix: Agent clone and serialization with a tools-less chat generator#12365
lets-order-some-fries wants to merge 1 commit into
deepset-ai:mainfrom
lets-order-some-fries:fix/agent-empty-tools-list-roundtrip

Conversation

@lets-order-some-fries

Copy link
Copy Markdown

Related Issues

  • No existing issue; found while reading Agent's init/serialization paths. Happy to open one if you'd prefer the bug tracked separately.

Proposed Changes:

An Agent built on a chat generator whose run() has no tools parameter is a supported configuration — the suite already covers constructing and running one in TestAgentRun::test_no_tools_with_chat_generator_without_tools_support. But such an Agent cannot be cloned or serialized: both raise TypeError.

The cause is a value that changes shape between the two passes:

  1. __init__ guards with tools is not None (agent.py:441). tools=None passes.
  2. __init__ then normalizes it away: self.tools = tools if tools is not None else [] (agent.py:469).
  3. clone() rebuilds from getattr(self, name) for each init param, so it passes tools=[]. to_dict() emits serialize_tools_or_toolset(self.tools)[], which from_dict hands back to __init__.
  4. On that second pass [] is not None is True, so the guard fires and raises — for an Agent that has no tools.

The same rule is already implemented a second time for the run path, at agent.py:734:

if flat_tools and not self._chat_generator_supports_tools:

That one uses truthiness, so it correctly treats an empty list as "no tools". The two guards disagree, and the run() one is right — an empty list carries no tools. This change makes __init__ agree with it.

The is not None form was deliberate, per the comment it replaces: truthiness on a Toolset calls __len__, which for SearchableToolset would iterate and prematurely warm it up at init. That property is preserved — only a list is ever measured:

tools_provided = tools is not None and (not isinstance(tools, list) or len(tools) > 0)

isinstance(tools, list) short-circuits before any __len__ call, so a Toolset is still never measured here. A non-empty tools value still raises exactly as before, and passing tools=[] explicitly is now accepted for the same reason tools=None is.

How did you test it?

Three unit tests, one per affected entry point, added to the classes that already own those behaviours. Each fails on main and passes with the change — verified by reverting only agent.py and re-running:

# with agent.py reverted, tests present:
FAILED TestAgentInit::test_empty_tools_list_with_chat_generator_without_tools_support
FAILED TestAgentSerialization::test_to_dict_from_dict_without_tools
FAILED TestAgentClone::test_clone_without_tools
3 failed, 1 passed        # the 1 passing is the existing test_no_tools_... run-path case

# with the change:
4 passed

Full runs at this head (36706ae):

  • hatch run test:unit test/components/agents/test_agent.py104 passed
  • hatch run test:unit test/components/agents/ test/tools/ test/core/2359 passed, 173 deselected
  • hatch run test:typesSuccess: no issues found in 432 source files
  • hatch run fmtAll checks passed!
  • pre-commit run --files <the three changed files> → all hooks Passed

test_chat_generator_must_support_tools (non-empty tools + tools-less generator still raises) is unchanged and still passes.

Notes for the reviewer

One pre-existing failure, unrelated to this PR, that I did not tick as green. A full hatch run test:unit at this head reports 2 failed, 6116 passed. Both failures are in test/components/generators/chat/test_openai.py:

TestChatCompletionChunkConversion::test_convert_chat_completion_chunk_to_streaming_chunk
TestChatCompletionChunkConversion::test_handle_stream_response

They reproduce identically on a clean checkout of main (915888bb) with none of my changes applied, so they are not caused by this PR. For what it's worth, they look like fallout from chore: unpin openai (#12348): with openai>=2.6.0 now resolving to openai 3.1.0, the usage payload gained fields the fixtures don't expect — the diff is confined to meta, e.g. prompt_tokens_details now carrying cache_write_tokens / image_tokens / text_tokens. Flagging it because a fresh install currently hits it; happy to open a separate issue or PR if that's useful.

One test-scaffolding change. MockChatGeneratorWithoutTools.to_dict() returned {"type": "MockChatGeneratorWithoutTools", "data": {}} — a bare class name and a data key, neither of which round-trips through component_from_dict. It is now a resolvable dotted path with init_parameters, which is what the serialization test needs. Nothing referenced the old shape.

I did not touch the :raises TypeError: docstring, since it remains accurate — a generator that doesn't support tools still raises when tools are actually provided.

Alternatives I considered and rejected: leaving self.tools un-normalized (wide blast radius — plenty of code assumes a list), or emitting None from clone()/to_dict() when empty (fixes the round trip but leaves an explicit tools=[] still raising, and leaves the two guards disagreeing).

AI assistance disclosure: this PR was written with an AI assistant. I reviewed the change, and ran the tests and quality checks reported above.

Checklist

An Agent built on a chat generator whose run() has no `tools` parameter is a
supported configuration, but it could not be cloned or round-tripped.

`__init__` normalizes `tools` to `[]`, and both `clone()` and `to_dict()` read
that normalized value back into the constructor. The tool-support guard used
`tools is not None`, so the empty list was treated as "tools were provided" and
raised TypeError on the second pass. The equivalent guard in `run()` already
uses truthiness and treats an empty list as no tools; this makes `__init__`
agree with it.

Only a list is measured, so a Toolset is still never tested for truthiness at
init - the reason the `is not None` form was used in the first place.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lets-order-some-fries
lets-order-some-fries requested a review from a team as a code owner August 16, 2026 11:42
@lets-order-some-fries
lets-order-some-fries requested review from julian-risch and removed request for a team August 16, 2026 11:42
@vercel

vercel Bot commented Aug 16, 2026

Copy link
Copy Markdown

@lets-order-some-fries is attempting to deploy a commit to the deepset Team on Vercel.

A member of the Team first needs to authorize it.

@CLAassistant

CLAassistant commented Aug 16, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@HaystackBot

Copy link
Copy Markdown
Contributor

Hi @lets-order-some-fries, thanks a lot for your contribution! 🙏

We noticed that the Contributor License Agreement (CLA) check (license/cla) hasn't passed yet, so we've temporarily moved this PR to draft and paused the review assignment.

To get your PR reviewed, please sign the CLA via the link in the license/cla check below (or in the CLA bot comment). As soon as the check turns green, this PR will automatically be marked ready for review again and a reviewer will be re-assigned.

@HaystackBot
HaystackBot removed the request for review from julian-risch August 16, 2026 13:04
@HaystackBot HaystackBot added the cla-pending PR is in draft until the contributor signs the CLA label Aug 16, 2026
@HaystackBot
HaystackBot marked this pull request as draft August 16, 2026 13:04
@HaystackBot
HaystackBot marked this pull request as ready for review August 16, 2026 13:29
@HaystackBot HaystackBot removed the cla-pending PR is in draft until the contributor signs the CLA label Aug 16, 2026
@HaystackBot

Copy link
Copy Markdown
Contributor

Thanks for signing the CLA, @lets-order-some-fries! 🎉 This PR is now ready for review again and the reviewer has been re-assigned.

@lets-order-some-fries

Copy link
Copy Markdown
Author

Gentle nudge on this one, with the current state so it's quick to action:

  • The bug still reproduces on main today — haystack/components/agents/agent.py:440 is still if tools is not None and not self._chat_generator_supports_tools:, so an Agent built on a tools-less chat generator still can't be cloned or round-tripped through to_dict/from_dict, even though TestAgentRun::test_no_tools_with_chat_generator_without_tools_support covers constructing and running exactly that.
  • The CLA is signed and green, and the branch is still effectively current (mergeable: true, one commit behind).
  • The only thing standing in the way is that the heavyweight workflows have never run: this head has just guard, sync and triage, and Tests / Check Release Notes sit at action_required awaiting the first-time-contributor "Approve and run". That's why mergeable_state reads blocked — not a failure, just nothing dispatched.

No rush from my side, and I realise the queue is long. Happy to rebase onto current main if that's easier, or to split the one test-scaffolding change (MockChatGeneratorWithoutTools.to_dict returning a resolvable dotted path, which the round-trip test needs) into its own PR if you'd rather review the guard fix in isolation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants