Python: [BREAKING] Fix harness before-strategy compaction under per-service-call persistence#7055
Python: [BREAKING] Fix harness before-strategy compaction under per-service-call persistence#7055westey-m wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing harness bug where “before-strategy” (per-model-call) compaction never executed under require_per_service_call_history_persistence=True, causing long persisted histories to be sent to the model without token-budget compaction. It does so by moving the before-phase compaction into the per-model-call chat pipeline (via the agent’s compaction_strategy option) while keeping after-phase compaction as a post-turn CompactionProvider.
Changes:
- Refactors harness compaction assembly to return
(before_strategy, after_provider)and wires the before-strategy throughAgent(compaction_strategy=...)so it runs insideBaseChatClient.get_responseon the fully loaded per-call history. - Changes the default after-phase strategy (when token params are provided) to
ContextWindowCompactionStrategy(matching the before-phase), instead ofToolResultCompactionStrategy. - Adds regression/integration tests proving before-compaction truncates loaded history under per-service-call persistence, preserves multi-turn coherence, and enforces an expected chat-client middleware execution order.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/packages/core/agent_framework/_harness/_agent.py | Rewires harness compaction so before-phase runs per model call via agent compaction_strategy, and aligns default before/after strategies under token params. |
| python/packages/core/tests/core/test_harness_agent.py | Adds tests covering split-phase defaults, regression for per-service-call persistence, multi-turn coherence, and middleware execution order. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 90%
✓ Correctness
The PR correctly separates before-strategy compaction from the CompactionProvider and wires it as the agent's compaction_strategy option, which runs inside BaseChatClient.get_response per model call. The tokenizer flows correctly through the system: ContextWindowCompactionStrategy stores its own tokenizer internally (with CharacterEstimatorTokenizer default), so even when the external tokenizer is None, compaction works properly via TokenBudgetComposedStrategy's annotate_token_counts call. The shared default_strategy instance is safe to reuse across both phases since it is stateless (no mutable instance state across calls). The
oroperator usage for resolving strategies is equivalent tois Nonechecks for CompactionStrategy objects which are always truthy. All tests verify the correct behavior.
✓ Security Reliability
The PR correctly rewires before-strategy compaction from a CompactionProvider.before_run hook (which was a no-op under per-service-call persistence) to the agent's compaction_strategy option that runs per model call inside BaseChatClient.get_response. The shared ContextWindowCompactionStrategy instance is functionally stateless between calls (its internal TokenBudgetComposedStrategy instances annotate fresh each invocation), so reuse across both phases is safe. No security vulnerabilities, resource leaks, injection risks, or unhandled failure modes found. Input validation for token params is performed upstream in create_harness_agent before _assemble_compaction is called.
✓ Test Coverage
The test coverage for this PR is thorough and well-structured. The PR adds four substantial new tests covering: split-phase wiring, regression for the core bug (#7011), multi-turn history coherence, and middleware execution order. Existing tests were appropriately updated to reflect the new agent.compaction_strategy attribute. The only minor gap is that the pre-existing
test_create_harness_agent_custom_after_strategy_enables_compaction_without_tokenstest was not updated to also assertagent.compaction_strategy is None, which would symmetrically verify that providing only an after-strategy doesn't accidentally wire anything as the agent-level compaction_strategy.
✓ Failure Modes
No operational failure modes found. The refactoring correctly separates before-strategy (per model call via agent compaction_strategy) from after-strategy (post-turn via CompactionProvider). The shared ContextWindowCompactionStrategy instance is stateless, the token annotation is self-contained within TokenBudgetComposedStrategy, and exclusion flag leakage between phases is intentional and safe under skip_excluded=False semantics.
✓ Design Approach
The new before-compaction placement is correct for local-history mode, but it still does not solve the stated harness bug for clients/runs that keep history server-side. In that path the per-service middleware never reloads provider history into
messages, so wiringbefore_compactionas the agent-levelcompaction_strategystill leaves historical turns uncompacted.
Automated review by westey-m's agents
Motivation & Context
create_harness_agentenables per-service-call history persistence(
require_per_service_call_history_persistence=True). With that setting the agentskips
HistoryProvider.before_run, so the before-phase compaction — which was wiredas a
CompactionProvider.before_runhook — only ever saw an empty context and neverran. As a result the harness had no working
before_strategy(per-model-call)compaction, so long histories were sent to the model uncompacted.
This PR makes before-strategy compaction actually run, on the full persisted history,
per model call.
Description & Review Guide
What are the major changes?
compaction_strategychat option insteadof a
CompactionProvider.before_runhook. It now runs insideBaseChatClient.get_response— per model call, inner ofPerServiceCallHistoryPersistingMiddleware(which has already loaded the fullhistory into the outgoing messages) and outer of the leaf client.
_assemble_compactionnow returns a(before_strategy, after_provider)tuple; theafter-strategy stays on a
CompactionProvider(itsafter_runcompacts persistedhistory in place).
create_harness_agentpasses the before-strategy and tokenizerto the
Agent.ContextWindowCompactionStrategywhen token params are provided (previously the after phase defaulted to
ToolResultCompactionStrategy). Caller-suppliedbefore_compaction_strategy/after_compaction_strategystill win per phase.truncates the loaded history under per-service-call persistence, a multi-turn
coherence check, and a test asserting the exact chat-client middleware execution
order: Function Invocation Loop → MessageInjection → PerServiceCall → Compaction →
Leaf ChatClient.
What is the impact of these changes?
call, keeping requests within the context budget inside the function-calling loop.
tool-result eviction (
ToolResultCompactionStrategy) to full context-windowcompaction (
ContextWindowCompactionStrategy). Callers who relied on the previousdefault can pass an explicit
after_compaction_strategy.What do you want reviewers to focus on?
create_harness_agent/_assemble_compactionand theguaranteed middleware ordering.
Related Issue
Closes #7011
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.