From 0f90dd8b205ed651593083a59ad3003db410122c Mon Sep 17 00:00:00 2001 From: Manjunath Janardhan Date: Sat, 12 Sep 2026 11:03:03 +0530 Subject: [PATCH] Python: keep agent compaction configuration when HandoffBuilder clones participants `_clone_chat_agent` rebuilds each participant through `Agent(...)` to inject the handoff tools and middleware, listing constructor arguments by hand. `compaction_strategy` and `tokenizer` live outside `default_options` and were not in that list, so the clone silently got `None` for both and the participant ran with no compaction at all. Nothing raises: a long handoff conversation simply grows unbounded until it trips the model's context limit, after showing up as cost and latency first. Both are forwarded by reference rather than deep-copied, matching `context_providers` and `middleware`. They hold immutable configuration the clone never mutates, and a tokenizer can carry a vocabulary that is expensive or unsafe to copy. Rebuilding through the constructor is deliberate and stays: `Agent.__init__` re-separates MCP tools from regular ones, which is why the method recombines `agent.mcp_tools` into the tools list first. Copying the instance instead would skip that. The second test is the one that matters beyond this bug. Every parameter added to `Agent.__init__` has to be added to this call or it is silently dropped, and the `test_handoff_clone_preserves_*` tests above were each written after that already happened. The guard reads the keyword names off the `Agent(...)` call with `ast` and asserts every constructor parameter is either forwarded or named as deliberately handled elsewhere, so the next missing field fails here rather than in a user's workflow. Reading the AST rather than substring-matching the source means a commented-out argument cannot satisfy it. --- .../_handoff.py | 5 + .../orchestrations/tests/test_handoff.py | 97 +++++++++++++++++++ 2 files changed, 102 insertions(+) diff --git a/python/packages/orchestrations/agent_framework_orchestrations/_handoff.py b/python/packages/orchestrations/agent_framework_orchestrations/_handoff.py index 9403a4ec2f9..948f780641b 100644 --- a/python/packages/orchestrations/agent_framework_orchestrations/_handoff.py +++ b/python/packages/orchestrations/agent_framework_orchestrations/_handoff.py @@ -299,6 +299,11 @@ def _clone_chat_agent(self, agent: Agent[Any]) -> Agent[Any]: context_providers=agent.context_providers, middleware=agent.middleware, require_per_service_call_history_persistence=agent.require_per_service_call_history_persistence, + # Shared by reference rather than deep-copied, like `context_providers` and + # `middleware` above: both hold immutable configuration the clone never mutates, + # and a tokenizer can carry a vocabulary that is expensive or unsafe to copy. + compaction_strategy=agent.compaction_strategy, + tokenizer=agent.tokenizer, default_options=cloned_options, # type: ignore[assignment] additional_properties=deepcopy(agent.additional_properties), ) diff --git a/python/packages/orchestrations/tests/test_handoff.py b/python/packages/orchestrations/tests/test_handoff.py index 97b2a42e4fe..a6fa08148a8 100644 --- a/python/packages/orchestrations/tests/test_handoff.py +++ b/python/packages/orchestrations/tests/test_handoff.py @@ -1,7 +1,10 @@ # Copyright (c) Microsoft. All rights reserved. +import ast +import inspect import os import re +import textwrap from collections.abc import AsyncIterable, Awaitable, Mapping, Sequence from typing import Annotated, Any, cast from unittest.mock import AsyncMock, MagicMock @@ -12,6 +15,7 @@ AgentContext, AgentResponse, AgentResponseUpdate, + CharacterEstimatorTokenizer, ChatOptions, ChatResponse, ChatResponseUpdate, @@ -20,6 +24,7 @@ InMemoryHistoryProvider, Message, ResponseStream, + ToolResultCompactionStrategy, WorkflowEvent, WorkflowRunState, agent_middleware, @@ -985,6 +990,98 @@ async def observe_properties(context: AgentContext, call_next): assert cloned_additional_properties is not coordinator.additional_properties +async def test_handoff_clone_preserves_compaction_strategy_and_tokenizer() -> None: + """Handoff clones must keep agent-level compaction configuration (#8320). + + Both live outside ``default_options``, so rebuilding the agent through its constructor + drops them unless they are forwarded explicitly. The clone then silently falls back to + no compaction, and a long handoff conversation grows unbounded until it trips the + model's context limit -- a failure that shows up as cost and latency long before it + shows up as an error. + """ + strategy = ToolResultCompactionStrategy() + tokenizer = CharacterEstimatorTokenizer() + + coordinator = Agent( + id="coordinator", + name="coordinator", + client=MockChatClient(name="coordinator"), + compaction_strategy=strategy, + tokenizer=tokenizer, + require_per_service_call_history_persistence=True, + ) + specialist = Agent( + id="specialist", + name="specialist", + client=MockChatClient(name="specialist"), + require_per_service_call_history_persistence=True, + ) + + workflow = ( + HandoffBuilder( + participants=_as_handoff_agents(coordinator, specialist), + termination_condition=lambda conversation: any(msg.role == "assistant" for msg in conversation), + ) + .with_start_agent(_as_handoff_agent(coordinator)) + .build() + ) + + await _drain(workflow.run("hello", stream=True)) + + executor = workflow.executors[resolve_agent_id(coordinator)] + assert isinstance(executor, HandoffAgentExecutor) + cloned = cast(Agent, executor.agent) + + # Shared by reference, like context_providers and middleware: these hold immutable + # configuration, and a tokenizer may carry a vocabulary that is costly to copy. + assert cloned.compaction_strategy is strategy + assert cloned.tokenizer is tokenizer + + +def test_handoff_clone_forwards_every_agent_constructor_field() -> None: + """Guard against the next field being dropped the way #8320 dropped two. + + ``_clone_chat_agent`` rebuilds the agent by listing constructor arguments by hand, so + every parameter added to ``Agent.__init__`` has to be added here too or it is silently + lost. That has already happened repeatedly -- the ``test_handoff_clone_preserves_*`` + tests above were each written after a field went missing. This asserts the inverse: + every constructor parameter is either forwarded or named below as deliberately handled + another way, so a new parameter fails here instead of in a user's workflow. + """ + handled_elsewhere = { + # Recombined with `agent.mcp_tools` and passed through `default_options["tools"]`, + # because the constructor re-separates MCP tools from regular ones. + "tools", + # Carried inside `default_options` rather than as its own argument. + "instructions", + } + + parameters = { + name + for name, param in inspect.signature(Agent.__init__).parameters.items() + if name != "self" and param.kind not in (param.VAR_POSITIONAL, param.VAR_KEYWORD) + } + + # Read the keyword names off the `Agent(...)` call itself rather than substring-matching + # the source: a commented-out argument would satisfy a substring check, and renaming the + # local would break every match at once. + tree = ast.parse(textwrap.dedent(inspect.getsource(HandoffAgentExecutor._clone_chat_agent))) + forwarded = { + keyword.arg + for node in ast.walk(tree) + if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == "Agent" + for keyword in node.keywords + if keyword.arg is not None + } + assert forwarded, "could not find the Agent(...) call in _clone_chat_agent; this guard needs updating" + + missing = parameters - forwarded - handled_elsewhere + assert not missing, ( + f"_clone_chat_agent does not forward {sorted(missing)}; add them to the Agent(...) " + f"call, or to `handled_elsewhere` with a comment saying why." + ) + + def test_clean_conversation_for_handoff_keeps_text_only_history() -> None: """Tool-control messages must be excluded from persisted handoff history.""" function_call = Content.from_function_call(