Skip to content

Honor an explicit MinimumPreserved=0 in compaction strategies instead of collapsing to the default - #686

Open
PratikDhanave wants to merge 1 commit into
microsoft:mainfrom
PratikDhanaveFork:honor-explicit-minimum-preserved-zero
Open

Honor an explicit MinimumPreserved=0 in compaction strategies instead of collapsing to the default#686
PratikDhanave wants to merge 1 commit into
microsoft:mainfrom
PratikDhanaveFork:honor-explicit-minimum-preserved-zero

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

What

The four compaction strategies (TruncationStrategy, SlidingWindowStrategy, SummarizationStrategy, ToolResultStrategy) computed their minimum-preserved floor with:

minimumPreserved := cmp.Or(max(strategy.MinimumPreserved*, 0), default)

Because max(0, 0) == 0 and cmp.Or(0, default) == default, this folds three distinct inputs into the type default (truncation 32, sliding window 1, summarization 8, tool result 16):

  • unset (the intended default case) — correct
  • an explicit 0 — silently overridden by the default
  • a negative value — silently overridden by the default

So a caller who set MinimumPreservedGroups: 0 to preserve nothing got the default instead, and the aggressive preserve-none configuration was inexpressible. Only positive values worked.

This changes each MinimumPreserved* field to *int: a nil config uses the default, while a non-nil value is honored as-is with a negative clamped to 0:

minimumPreserved := default
if strategy.MinimumPreserved* != nil {
    minimumPreserved = max(*strategy.MinimumPreserved*, 0)
}

Why

The .NET port treats this as a nullable int and applies EnsureNonNegative(value) => Math.Max(0, value), so an explicit 0 disables the floor and negatives clamp to 0. The Go port silently discarded the value instead, leaving no equivalent for the preserve-none case. This restores cross-SDK parity.

Tests

  • Added TestTruncationStrategy_ExplicitZeroPreservesNone: an index with 5 non-system groups, nil Trigger/Target, and MinimumPreservedGroups: ptr(0) now compacts and excludes all removable non-system groups rather than preserving the default 32.
  • Added TestSlidingWindowStrategy_NegativeMinimumClampsToZero: a negative floor clamps to 0 (excluding all turns) instead of using the default 1.
  • Updated existing call sites (contextwindow.go, the step18 example) and existing tests to the pointer field type.

go build ./..., go vet ./agent/compaction/..., and go test ./agent/compaction/... all pass.

@PratikDhanave
PratikDhanave requested a review from a team as a code owner July 24, 2026 01:30
Copilot AI review requested due to automatic review settings July 24, 2026 01:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates the compaction strategies’ “minimum preserved” floors so callers can explicitly set 0 (preserve none) and have it honored, while keeping defaults when the value is unset. This aligns the Go SDK behavior with the .NET port by making the minimum-preserved fields nullable and clamping negative values to 0.

Changes:

  • Changed MinimumPreservedGroups / MinimumPreservedTurns fields from int to *int across compaction strategies to distinguish “unset” vs “explicit 0”.
  • Updated compaction logic to use defaults when the pointer is nil, otherwise clamp the provided value to >= 0.
  • Updated examples and tests; added new tests covering explicit 0 and negative clamping behavior.

Reviewed changes

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

Show a summary per file
File Description
examples/02-agents/agents/step18_compaction_pipeline/main.go Updates example strategy config to use pointer-valued minimum-preserved fields and adds a local ptr helper.
agent/compaction/truncation.go Makes MinimumPreservedGroups nullable and honors explicit 0 / clamps negatives.
agent/compaction/toolresult.go Makes MinimumPreservedGroups nullable and honors explicit 0 / clamps negatives.
agent/compaction/summarization.go Makes MinimumPreservedGroups nullable and honors explicit 0 / clamps negatives while keeping existing prompt/default behavior.
agent/compaction/slidingwindow.go Makes MinimumPreservedTurns nullable and honors explicit 0 / clamps negatives.
agent/compaction/index_test.go Updates tests to use pointer-valued MinimumPreservedGroups.
agent/compaction/contextwindow.go Updates internal strategy wiring to provide pointer-valued minimum-preserved config.
agent/compaction/compaction_test.go Updates tests, adds new coverage for explicit 0 and negative clamping, and adds a ptr helper for tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@PratikDhanave
PratikDhanave force-pushed the honor-explicit-minimum-preserved-zero branch from 68767cd to 1ce19e4 Compare July 24, 2026 01:40
@github-actions github-actions Bot added the public-api-change Pull Request changes public APIs label Jul 24, 2026
@github-actions

This comment has been minimized.

The four compaction strategies computed their preserve floor with
cmp.Or(max(field, 0), default), which folds unset, explicit 0, and
negative values all into the type default (truncation 32, sliding
window 1, summarization 8, tool result 16). A caller who set
MinimumPreserved* to 0 to preserve nothing silently got the default
instead, and there was no way to express the preserve-none config.

Change each MinimumPreserved* field to *int so an unset (nil) config
uses the default while an explicit value is honored as-is, clamping a
negative to 0. This mirrors the .NET port, which uses a nullable int
and EnsureNonNegative(value) => Math.Max(0, value).
@PratikDhanave
PratikDhanave force-pushed the honor-explicit-minimum-preserved-zero branch from 1ce19e4 to 083c194 Compare July 24, 2026 09:34
@github-actions github-actions Bot added the parity-approved Go API consistency review found no parity issues label Jul 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Cross-SDK Parity Review ✅

Verdict: parity-approved — this change restores alignment with the upstream .NET implementation.

What changed

All four compaction strategy structs (TruncationStrategy, SlidingWindowStrategy, SummarizationStrategy, ToolResultStrategy) change their MinimumPreserved* field type from int to *int, so an explicit 0 can be distinguished from the zero value and honored as "preserve nothing".

Upstream .NET parity

The canonical .NET equivalents live in dotnet/src/Microsoft.Agents.AI/Compaction/:

  • TruncationCompactionStrategy — constructor: int minimumPreservedGroups = DefaultMinimumPreserved, then EnsureNonNegative(value) (= Math.Max(0, value)). An explicit 0 is stored as-is; negative values clamp to 0.
  • SlidingWindowCompactionStrategy — same pattern with minimumPreservedTurns = DefaultMinimumPreserved.

The old Go behavior (cmp.Or(max(v, 0), default)) diverged from this: because max(0, 0) == 0 and cmp.Or(0, default) == default, an explicit 0 was silently promoted to the type default (32 / 1 / 8 / 16 depending on strategy). The .NET EnsureNonNegative approach preserves 0 — matching the intent of this PR.

Go idiom alignment

Using *int (nil = unset → use default; non-nil → honor the caller) is the idiomatic Go equivalent of .NET's constructor default-parameter pattern. The semantic contract is identical.

No new divergence introduced

No upstream Python compaction implementation was found; the .NET reference is the primary source of truth for this subsystem and the PR is consistent with it.

Labels

  • public-api-change — already present and correct; struct field types are part of the exported API surface.
  • parity-approved — added by this review.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

Generated by Go API Consistency Review Agent · 36.7 AIC · ⌖ 5.67 AIC · ⊞ 5.9K ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parity-approved Go API consistency review found no parity issues public-api-change Pull Request changes public APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants