Skip to content

Python: Strip reserved kwargs in AgentExecutor to prevent duplicate-argument TypeError#4298

Open
moonbox3 wants to merge 2 commits intomicrosoft:mainfrom
moonbox3:agent/fix-4295-4
Open

Python: Strip reserved kwargs in AgentExecutor to prevent duplicate-argument TypeError#4298
moonbox3 wants to merge 2 commits intomicrosoft:mainfrom
moonbox3:agent/fix-4295-4

Conversation

@moonbox3
Copy link
Contributor

Motivation and Context

When users pass session=, stream=, or messages= as keyword arguments through workflow.run(), those kwargs are forwarded into AgentExecutor which also passes them explicitly to agent.run(), causing a TypeError: got multiple values for keyword argument. This makes the session parameter on workflow.run() unusable despite being a natural thing for callers to provide.

Fixes #4295

Description

The root cause is that _prepare_agent_run_args did not filter out kwargs that AgentExecutor already supplies positionally/explicitly when calling agent.run() (namely session, stream, and messages). The fix strips these reserved parameters from the forwarded run_kwargs dict before they reach agent.run(), and emits a warning so callers know the value was ignored. Tests verify that passing session= or stream= through workflow.run() no longer raises, and that non-reserved kwargs continue to pass through unchanged.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • The PR follows the Contribution Guidelines
  • All unit tests pass, and I have added new tests where possible
  • Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

Note: PR autogenerated by moonbox3's agent

…icrosoft#4295)

workflow.run(session=...) passed 'session' through to agent.run() via
**run_kwargs while AgentExecutor also passes session=self._session
explicitly, causing TypeError: got multiple values for keyword argument.

_prepare_agent_run_args now strips reserved params (session, stream,
messages) from run_kwargs and logs a warning when they are present.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings February 26, 2026 07:44
Copy link
Contributor Author

@moonbox3 moonbox3 left a comment

Choose a reason for hiding this comment

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

Automated Code Review

Reviewers: 3 | Confidence: 87%

✓ Correctness

The change strips reserved keyword arguments (session, stream, messages) from run_kwargs before they are passed to agent.run(), preventing duplicate-keyword TypeErrors. The logic is correct and the tests adequately cover the new behavior. The only notable inconsistency is that _RESERVED_RUN_PARAMS is defined as a class-level frozenset containing all five reserved names but is never referenced anywhere in the code—the stripping loop hardcodes a subset of three names in a separate tuple literal, meaning the frozenset could drift out of sync with the actual stripping logic without any compile-time or runtime indication.

✓ Security Reliability

This diff adds defensive stripping of reserved keyword arguments to prevent duplicate-keyword TypeErrors in agent.run(). The change is straightforward and low-risk. The main reliability concern is that _RESERVED_RUN_PARAMS is defined as a class-level frozenset with 5 entries but is never referenced in the actual stripping logic, which hardcodes only 3 of them in a separate tuple literal. This creates a maintenance hazard where a future developer adds a new reserved param to the frozenset expecting it to be automatically stripped, but it won't be.

✓ Test Coverage

The new tests cover the core behaviour well: integration tests for session and stream kwargs, a parametrized unit test for all three stripped reserved keywords, and a pass-through test for non-reserved kwargs. However, there are a few notable gaps: the _RESERVED_RUN_PARAMS frozenset is declared but never actually referenced in the stripping loop (which hardcodes a tuple), and no test verifies consistency between the two; there is no test for passing multiple reserved kwargs simultaneously; the options return value is not asserted in the stripping tests; and there is no integration-level test for the messages= reserved kwarg.

Suggestions

  • _RESERVED_RUN_PARAMS is defined but never referenced. Consider using it in the stripping loop (e.g., iterating over _RESERVED_RUN_PARAMS - {"options", "additional_function_arguments"}) or removing the constant to avoid a maintenance trap where the frozenset and the loop diverge silently.
  • The _RESERVED_RUN_PARAMS frozenset (line 420) is defined but never referenced in code. The stripping loop on line 437 hardcodes ("session", "stream", "messages") instead of using the constant. Consider either using the constant in the loop (filtering out options and additional_function_arguments which are handled separately), or removing the constant to avoid misleading future maintainers.
  • Consider using run_kwargs.pop(key, None) inside the if key in run_kwargs block to be consistent, though functionally equivalent since the in check guards it.
  • Add a test that passes multiple reserved kwargs at once (e.g., {"session": "x", "stream": True, "messages": [], "custom": 1}) and asserts all reserved keys are stripped while custom keys survive, and that multiple warnings are emitted.
  • In test_prepare_agent_run_args_strips_reserved_kwargs, assert the options return value (expected None here) to fully validate the return tuple.
  • The _RESERVED_RUN_PARAMS frozenset (line 418) is never referenced by the stripping loop (which hardcodes ("session", "stream", "messages")). Consider either using the constant in the loop or adding a test that verifies the constant matches the actually-stripped keys, to prevent the two from drifting apart.
  • Consider adding an integration test for messages= passed through workflow.run(), similar to the existing session= and stream= integration tests.

Automated review by moonbox3's agents

@github-actions github-actions bot changed the title Strip reserved kwargs in AgentExecutor to prevent duplicate-argument TypeError Python: Strip reserved kwargs in AgentExecutor to prevent duplicate-argument TypeError Feb 26, 2026
@markwallace-microsoft
Copy link
Member

markwallace-microsoft commented Feb 26, 2026

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
packages/core/agent_framework/_workflows
   _agent_executor.py2052687%97, 113, 168–169, 221–222, 224–225, 255–257, 265–267, 275–277, 279, 283, 287, 291–292, 391–392, 457, 475
TOTAL22179276287% 

Python Unit Test Overview

Tests Skipped Failures Errors Time
4680 247 💤 0 ❌ 0 🔥 1m 16s ⏱️

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a workflow→agent invocation kwarg collision in the Python Agent Framework, preventing TypeError: got multiple values for keyword argument when workflow-level kwargs overlap with AgentExecutor’s explicit agent.run() parameters.

Changes:

  • Strip reserved kwargs (session, stream, messages) from workflow forwarded kwargs in AgentExecutor._prepare_agent_run_args, emitting a warning when they’re provided.
  • Add unit tests validating workflow.run(session=...) no longer raises and that reserved kwargs are stripped with a warning.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
python/packages/core/agent_framework/_workflows/_agent_executor.py Filters reserved workflow kwargs before spreading into agent.run() to avoid duplicate-argument collisions.
python/packages/core/tests/workflow/test_agent_executor.py Adds regression tests for the collision and for reserved-kwarg stripping + warning behavior.

- Use _RESERVED_RUN_PARAMS constant in stripping loop instead of
  hardcoded tuple to maintain single source of truth
- Trim frozenset to only stripped keys (session, stream, messages);
  options and additional_function_arguments have separate merge logic
- Fix caplog type annotation to use TYPE_CHECKING pattern
- Assert options return value in reserved-kwarg stripping test
- Add test for multiple reserved kwargs supplied simultaneously
- Add integration test for messages= kwarg via workflow.run()

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@moonbox3 moonbox3 enabled auto-merge February 26, 2026 09:04
@moonbox3 moonbox3 self-assigned this Feb 26, 2026
@moonbox3 moonbox3 moved this to In Review in Agent Framework Feb 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

Python: workflow.run(session=...) collides with AgentExecutor session argument

3 participants