Python: Strip reserved kwargs in AgentExecutor to prevent duplicate-argument TypeError#4298
Python: Strip reserved kwargs in AgentExecutor to prevent duplicate-argument TypeError#4298moonbox3 wants to merge 2 commits intomicrosoft:mainfrom
Conversation
…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>
moonbox3
left a comment
There was a problem hiding this comment.
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_PARAMSis 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_PARAMSis 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_PARAMSfrozenset 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; theoptionsreturn value is not asserted in the stripping tests; and there is no integration-level test for themessages=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_PARAMSfrozenset (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 outoptionsandadditional_function_argumentswhich are handled separately), or removing the constant to avoid misleading future maintainers. - Consider using
run_kwargs.pop(key, None)inside theif key in run_kwargsblock to be consistent, though functionally equivalent since theincheck 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 theoptionsreturn value (expectedNonehere) to fully validate the return tuple. - The
_RESERVED_RUN_PARAMSfrozenset (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 throughworkflow.run(), similar to the existingsession=andstream=integration tests.
Automated review by moonbox3's agents
python/packages/core/agent_framework/_workflows/_agent_executor.py
Outdated
Show resolved
Hide resolved
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
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 inAgentExecutor._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. |
python/packages/core/agent_framework/_workflows/_agent_executor.py
Outdated
Show resolved
Hide resolved
- 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>
Motivation and Context
When users pass
session=,stream=, ormessages=as keyword arguments throughworkflow.run(), those kwargs are forwarded intoAgentExecutorwhich also passes them explicitly toagent.run(), causing aTypeError: got multiple values for keyword argument. This makes thesessionparameter onworkflow.run()unusable despite being a natural thing for callers to provide.Fixes #4295
Description
The root cause is that
_prepare_agent_run_argsdid not filter out kwargs thatAgentExecutoralready supplies positionally/explicitly when callingagent.run()(namelysession,stream, andmessages). The fix strips these reserved parameters from the forwardedrun_kwargsdict before they reachagent.run(), and emits a warning so callers know the value was ignored. Tests verify that passingsession=orstream=throughworkflow.run()no longer raises, and that non-reserved kwargs continue to pass through unchanged.Contribution Checklist