Python: HarnessAgent: Disable compaction when max tokens not provided#6410
Conversation
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 89%
✓ Correctness
The PR correctly makes max_context_window_tokens and max_output_tokens optional in create_harness_agent, gracefully disabling compaction when they're not provided. The logic in _assemble_compaction_provider is sound: it returns None when disable_compaction is True, when no before_strategy can be resolved, or when token params are missing and no custom strategy is given. Validation properly guards against None. The remaining changes are cosmetic reformatting. No correctness issues found.
✓ Security Reliability
This PR makes max_context_window_tokens and max_output_tokens optional in create_harness_agent, disabling compaction gracefully when they are not provided. The validation logic correctly guards against invalid values when provided, the compaction assembly safely returns None when a before-strategy cannot be resolved, and the default_opts handling correctly skips max_tokens when max_output_tokens is None. The remaining changes are formatting-only (test files, _skills.py). No security or reliability issues identified.
✓ Test Coverage
The new tests cover the primary scenarios well (both params omitted, custom before-strategy override, disable_compaction precedence). However, there is a coverage gap for the 'partial token params' case where only one of max_context_window_tokens or max_output_tokens is provided — compaction is silently disabled with no test documenting this behavior, which represents a likely user misconfiguration path.
✓ Failure Modes
The PR cleanly makes token params optional and disables compaction when both are omitted. The main concern is an operational silent-failure path: when a user provides only ONE of the two token params (e.g., max_context_window_tokens without max_output_tokens), compaction is silently disabled with no log warning, making a likely misconfiguration invisible.
✗ Design Approach
The optional-token change is mostly aligned with the stated goal, but it accidentally makes one documented customization path unreachable: a caller can no longer supply an
after_compaction_strategywithout also supplying a before strategy or token budget. That means the new design silently drops a valid post-run-only compaction configuration.
Flagged Issues
-
_assemble_compaction_providernow returnsNonewhenever no before-strategy can be resolved, so a caller-providedafter_compaction_strategyis silently ignored (python/packages/core/agent_framework/_harness/_agent.py:94-95). This conflicts with theCompactionProvidercontract that either strategy may beNoneto skip that phase, meaningafter_strategyalone is a valid configuration (python/packages/core/agent_framework/_compaction.py:1194,1232-1236).
Automated review by westey-m's agents
There was a problem hiding this comment.
Pull request overview
This PR aligns the Python HarnessAgent behavior with the .NET version by making token budget parameters optional and automatically disabling compaction when they aren’t supplied, while updating docs and tests to reflect the new default behavior.
Changes:
- Made
max_context_window_tokens/max_output_tokensoptional increate_harness_agentand disabled default compaction when they’re omitted. - Updated harness sample README to document “minimal setup” vs “with compaction”.
- Added unit tests covering compaction behavior when token params are omitted and when custom strategies/flags are provided; applied minor formatting-only updates in MCP test files.
Reviewed changes
Copilot reviewed 6 out of 6 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 | Makes token budgets optional and changes compaction/default options assembly accordingly. |
| python/packages/core/tests/core/test_harness_agent.py | Adds coverage for the new “no token params disables compaction” behavior and override precedence. |
| python/samples/02-agents/harness/README.md | Updates sample docs to reflect optional token parameters and compaction being opt-in. |
| python/packages/core/agent_framework/_skills.py | Minor formatting-only changes. |
| python/packages/core/tests/core/test_mcp_skills.py | Minor formatting-only changes. |
| python/packages/core/tests/core/test_mcp_observability.py | Minor formatting-only changes. |
Reject max_output_tokens=0 (must be positive), mirroring max_context_window_tokens. Addresses PR review feedback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Motivation and Context
Aligning the .net and python versions by having the max token params optional and where it just gracefully disable compaction if not provided.
#6333
Description
Contribution Checklist