Skip to content

Python: keep agent compaction configuration when HandoffBuilder clones participants - #8329

Open
Manjunath Janardhan (manjunathshiva) wants to merge 1 commit into
microsoft:mainfrom
manjunathshiva:python-handoff-clone-keeps-compaction-8320
Open

Python: keep agent compaction configuration when HandoffBuilder clones participants#8329
Manjunath Janardhan (manjunathshiva) wants to merge 1 commit into
microsoft:mainfrom
manjunathshiva:python-handoff-clone-keeps-compaction-8320

Conversation

@manjunathshiva

Copy link
Copy Markdown
Contributor

Motivation & Context

HandoffAgentExecutor._clone_chat_agent rebuilds each participant through Agent(...) to inject
handoff tools and middleware, listing constructor arguments by hand. compaction_strategy and
tokenizer live outside default_options and were missing from that list, so the clone got None
for both:

compaction_strategy   original=ToolResultCompactionStrategy   clone=None   *** DROPPED ***
tokenizer             original=CharacterEstimatorTokenizer    clone=None   *** DROPPED ***
instructions          KEPT          name  KEPT

Nothing raises. The participant simply runs with no compaction, so a long handoff conversation grows
unbounded until it trips the model's context limit — surfacing as cost and latency well before it
surfaces as an error.

One note on the issue text: it says "the cloned executor agent gets None for both" and "forward
both fields", but only ever names compaction_strategy. The second field is tokenizer. A fix
written from the title alone would close half the bug.

Description & Review Guide

  • What are the major changes? Two arguments forwarded in _clone_chat_agent, plus two tests.
    No API change, no change to compaction behaviour itself.

  • Why by reference and not deepcopy. Both are forwarded as-is, matching context_providers
    and middleware on the lines above. I checked that the strategies hold only immutable
    configuration — ToolResultCompactionStrategy stores one threshold, ContextWindowCompactionStrategy
    ten — and CharacterEstimatorTokenizer is stateless, so clones cannot interfere through them.
    additional_properties stays deep-copied because it is a mutable mapping the clone does own.

  • Why not replace the hand-rolled constructor with a copy. Agent inherits __copy__ and
    __deepcopy__ from SerializationMixin, which copy every instance field and would have fixed this
    class of bug permanently — so I checked whether they could be used here. They cannot:
    Agent.__init__ re-separates MCP tools from regular tools, which is exactly why this method
    recombines agent.mcp_tools into the tools list before calling it. Copying the instance would skip
    that separation, and copy would additionally share default_options with the original, so
    mutating allow_multiple_tool_calls on the clone would reach back into the caller's agent.
    Rebuilding through the constructor is deliberate and stays.

  • The second test is the point. 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 directly above were
    each written after that already happened — this is at least the fourth field to go missing.
    test_handoff_clone_forwards_every_agent_constructor_field reads the keyword names off the
    Agent(...) call with ast and asserts every constructor parameter is either forwarded or named in
    handled_elsewhere with a reason. The next new parameter fails there instead of in a user's
    workflow. Reading the AST rather than substring-matching the source is deliberate: I verified that a
    commented-out argument still fails the guard, which a substring check would have passed.

  • What is the impact of these changes? Compaction configured on a handoff participant now takes
    effect. That is a behaviour change in the sense that previously-ignored configuration starts
    working, so a long-running handoff conversation will begin compacting where it did not before. I
    have not marked it a breaking change because it makes documented configuration behave as
    documented, but say if you would rather label it.

  • What do you want reviewers to focus on? Whether by-reference is the sharing you want for these
    two, and whether the drift guard is welcome — it is a little unusual for this suite, and I would
    rather drop it than have it be a maintenance burden. _clone_chat_agent is the only
    clone-by-reconstruct site in the repo, so there is no sibling instance to fix.

    Validation: orchestrations 197 passing with poe check -P orchestrations clean across all five
    type checkers, and core 5136 passing unchanged. Both new tests fail against the unfixed tree.

Related Issue

Fixes #8320

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused fix correctly preserves both omitted fields and includes appropriate regression protection.

Pull request overview

Preserves agent-level compaction settings when handoff participants are cloned.

Changes:

  • Forwards compaction_strategy and tokenizer to cloned agents.
  • Adds regression and constructor-field drift tests.
File summaries
File Description
python/packages/orchestrations/agent_framework_orchestrations/_handoff.py Preserves compaction configuration during cloning.
python/packages/orchestrations/tests/test_handoff.py Tests configuration retention and future constructor coverage.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

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

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: HandoffBuilder drops agent-level compaction_strategy when cloning participants

2 participants