Python: Add function_loop_max_iterations to create_harness_agent#7346
Python: Add function_loop_max_iterations to create_harness_agent#7346westey-m wants to merge 1 commit into
Conversation
agent-framework-automation
Bot
added
documentation
There was a problem hiding this comment.
Pull request overview
This PR adds a supported, harness-level way to cap the per-request function/tool-calling loop (LLM round-trips) when using Python create_harness_agent, bringing it in line with the .NET harness option and reducing the need for callers to mutate chat clients directly.
Changes:
- Added a new keyword-only
function_loop_max_iterations: int | None = Noneparameter tocreate_harness_agent, validating>= 1, applying it toclient.function_invocation_configuration["max_iterations"]when supported, and logging a warning otherwise. - Updated type stubs and documentation to clarify the distinction between the agent re-run loop (
loop_*) and the per-request tool-calling loop (function_loop_max_iterations). - Added unit tests covering the new behavior (set, default no-op, invalid value, unsupported client warning).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| python/packages/core/agent_framework/_harness/_agent.py | Adds function_loop_max_iterations parameter, validation, and application to the client’s function invocation config. |
| python/packages/core/agent_framework/_harness/_agent.pyi | Mirrors the new parameter in the public type stub. |
| python/packages/core/tests/core/test_harness_agent.py | Adds focused tests for the new harness parameter and its warning path. |
| python/packages/core/AGENTS.md | Documents the new parameter and clarifies how it differs from loop_* harness looping. |
| python/packages/core/tests/core/test_function_invocation_logic.py | Minor formatting change (blank line). |
| logger.warning( | ||
| "function_loop_max_iterations was set but client %r does not expose " | ||
| "function_invocation_configuration; the setting will be ignored.", | ||
| type(client).__name__, | ||
| ) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 86%
✓ Correctness
This PR adds a
function_loop_max_iterationsparameter tocreate_harness_agentto cap the per-request function/tool-calling loop on the chat client, bringing the Python harness to parity with the .NET harness'sMaximumIterationsPerRequest. The implementation is clean and correct: validation rejects values < 1,getattrsafely handles clients withoutfunction_invocation_configuration, the dict mutation pattern matches existing usage in_tools.py, the.pyistub is updated, docstrings are accurate with clear cross-references, and tests cover all branches (set, default no-op, invalid value, unsupported client warning). No correctness issues found.
✓ Security Reliability
The PR adds a new
function_loop_max_iterationsparameter tocreate_harness_agentwith proper input validation (≥ 1), a graceful warning when the client lacksfunction_invocation_configuration, and correct mutation of the client's config dict. The validation at the harness level is important sincenormalize_function_invocation_configurationonly runs at client construction time — post-construction mutations to the dict bypass that check. Thegetattr-based duck-typing approach is appropriate for clients that may or may not mix inFunctionInvocationLayer. Tests cover the happy path, default no-op, validation rejection, and unsupported-client warning. No security or reliability issues found.
✓ Test Coverage
The test coverage for the new
function_loop_max_iterationsparameter is comprehensive. All four production code branches are exercised: (1) happy path sets the value on the client, (2) defaultNoneleaves configuration untouched, (3) value < 1 raisesValueError, (4) client withoutfunction_invocation_configurationlogs a warning. Assertions are meaningful (checking specific config values, error messages, log content) and mocks are appropriate (MockBaseChatClientinheritsFunctionInvocationLayerproviding the attribute,_FakeChatClientdoes not). No gaps or issues found.
✓ Failure Modes
The PR adds a clean, well-validated parameter to configure the per-request function/tool-calling loop cap. The validation occurs before use, the getattr pattern handles clients that don't expose function_invocation_configuration gracefully with a warning, and all code paths are covered by tests. No silent failures, lost errors, or operational failure modes were found in the changed code.
✗ Design Approach
I found one design issue: the new option mutates caller-owned client state before harness assembly has completed, so a later assembly error can leave
client.function_invocation_configuration["max_iterations"]changed even though no agent was created. Other than that, the added parameter, validation, and docs look consistent with the existing function-invocation model.
Flagged Issues
-
create_harness_agentwritesclient.function_invocation_configuration["max_iterations"]before later assembly steps that can still raise (e.g.,_assemble_shellraisesTypeErrorfor an invalidshell_executor). A failed call likecreate_harness_agent(..., function_loop_max_iterations=5, shell_executor=bad_value)silently leaves the caller's client reconfigured even though no agent was returned.
Automated review by westey-m's agents
|
Flagged issue
Source: automated DevFlow PR review |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 92%
✓ Correctness
The implementation is correct and well-tested. The
function_loop_max_iterationsparameter is properly validated (≥ 1), safely applied viagetattrwith a fallback, and the mutation of the client'sfunction_invocation_configurationdict is sound since it's a plain instance attribute (not a property). Tests comprehensively cover the happy path, default behavior, validation error, and the warning path for unsupported clients. No correctness issues found.
✓ Security Reliability
The implementation is clean from a security/reliability perspective. Input validation is done before mutation, the getattr pattern gracefully handles clients without function_invocation_configuration, and the TypedDict assignment is type-safe. The only issue (using %r for a string type name in the warning log) is already tracked in the existing review thread. No injection risks, resource leaks, or unhandled failure modes.
✓ Test Coverage
Test coverage for the new
function_loop_max_iterationsparameter is thorough. Four tests cover the happy path, default/no-op behavior, validation rejection, and the warning for unsupported clients. Assertions are meaningful (exact value checks, error message matching, log record inspection), and the choice of mocks is appropriate—MockBaseChatClient(from conftest) hasfunction_invocation_configurationwhile_FakeChatClientdoes not. No significant test coverage gaps identified.
✓ Failure Modes
The PR adds a well-guarded, additive parameter with proper validation (ValueError for < 1), a graceful fallback (warning + skip when the attribute is missing), and correct mutation of the expected TypedDict. No silent failures, lost errors, or partial-write issues were found. The only notable issue (using %r instead of %s for the type name in the warning message) is already tracked by an existing unresolved review comment.
✓ Design Approach
I found one design-level issue: the new API is being specified and tested as a mutation of the caller-owned chat client, which makes the setting shared across every harness agent built from that client instead of agent-local like the .NET harness counterpart.
Automated review by westey-m's agents
| disable_file_memory=True, | ||
| function_loop_max_iterations=7, | ||
| ) | ||
| assert client.function_invocation_configuration["max_iterations"] == 7 |
There was a problem hiding this comment.
This assertion locks in mutating the caller-owned client as the contract, but create_harness_agent passes that same client object straight into Agent(...) and Agent stores it by reference. That means create_harness_agent(shared_client, function_loop_max_iterations=7) followed by create_harness_agent(shared_client, function_loop_max_iterations=3) will silently change the first agent's cap to 3 as well. The .NET harness avoids this by configuring function invocation on a harness-owned wrapper instead of mutating the caller's client. Consider not cementing the shared-mutation behavior in tests; add coverage for reusing one client across two agents and keep the cap agent-local.
Motivation & Context
create_harness_agenthad no way to configure the per-request function/tool-calling loop cap. The underlying knob —client.function_invocation_configuration["max_iterations"](default 40) — could only be tuned by reaching into the chat client directly. The .NET harness already exposes this viaHarnessAgentOptions.MaximumIterationsPerRequest, so this brings the Python harness to parity and gives callers a supported way to bound tool-calling round-trips per request.Description & Review Guide
What are the major changes?
function_loop_max_iterations: int | None = Nonetocreate_harness_agent. When set, it is applied to the passed-in client'sfunction_invocation_configuration["max_iterations"]; whenNone(default) the client's config is left untouched.< 1raisesValueError) and warns/skips when the client does not exposefunction_invocation_configuration.loop_should_continue/loop_next_message/loop_max_iterationsdocstrings to make explicit that they govern re-running the whole agent (AgentLoopMiddleware), which is distinct from the per-request function-calling loop, with cross-references between the two..pyistub, adds tests, and updatesAGENTS.mdand the changelog.What is the impact of these changes?
Noneleaves the client configuration as-is). The name is deliberately distinct from the pre-existingloop_max_iterationsto avoid confusion between the agent re-run loop and the function-calling loop.What do you want reviewers to focus on?
function_invocation_configurationis the preferred approach (consistent with how function invocation is configured elsewhere), and the chosen parameter namefunction_loop_max_iterations.Related Issue
Fixes #6449
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.