diff --git a/python/packages/orchestrations/agent_framework_orchestrations/_handoff.py b/python/packages/orchestrations/agent_framework_orchestrations/_handoff.py index 9403a4ec2f..948f780641 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 97b2a42e4f..a6fa08148a 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(