Skip to content

perf(agent): cheap compaction — dedicated summarizer model, input cap…#532

Open
kevincodex1 wants to merge 1 commit into
mainfrom
feat/cheap-compaction
Open

perf(agent): cheap compaction — dedicated summarizer model, input cap…#532
kevincodex1 wants to merge 1 commit into
mainfrom
feat/cheap-compaction

Conversation

@kevincodex1

@kevincodex1 kevincodex1 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Compaction summarization was the single most expensive recurring event in a
long run: the full elided middle — verbatim tool bodies included — went to
the session's main model at full price. Near a 200k window that is a
~100k-token input call, recursively multiplied when the middle itself
overflows the summarizer. This PR makes compaction cheap on four independent
axes, each safe on its own.

1. Dedicated summarizer model

Compaction summaries route to a cheap model instead of the session's main
model. Resolution order (providers.CompactionModelID):

  1. ZERO_COMPACTION_MODEL env
  2. preferences.compactionModel in config
  3. Curated default — official endpoints only: Anthropic → Claude Haiku
    4.5, Google → Gemini 2.5 Flash-Lite, OpenAI → GPT-4.1 mini. Custom /
    *-compatible endpoints get no default (their catalogs are
    unknowable; a guaranteed failed call per run is worse than main-model
    prices), so they opt in via config only.

The literal value main at any level forces the old behavior. The provider
is built lazily on the first paid compaction (chat-only sessions never
build it), and any failure — factory or call — permanently falls back to
the main provider for the rest of the run
, so a misconfigured summarizer
can never break or repeatedly delay compaction. When the session already
runs on the curated cheap model, no dedicated summarizer is created.

Wired in exec (internal/cli/summarizer.go) and the TUI (resolved against
the active profile at run start, so it tracks mid-session model
switches).

2. Summarizer input caps

renderTranscript now clamps each tool-result body to 2,000 chars and each
tool-call argument blob (a write_file call carries the whole file) to 500,
head+tail with a rune-safe split and an explicit omission marker. The
summary needs what was done and decided, not raw payloads — the model that
acted on the full output already turned it into decisions the surrounding
messages record. Cuts summarizer input 5–10× on tool-heavy sessions, and
compounds with (1): cheap model × small input.

3. Free reactive prune

recover() (the context-limit-error path) now runs the same
prune-stale-tool-output stage the proactive path already had: the first
context-limit error is answered by pruning old tool bodies at zero cost,
without consuming the one-shot reactive budget. The paid summarizer only
runs when a later error finds nothing left to prune.

4. Context-window re-derivation on model switch

New Options.ContextWindowFor (wired to the model registry in exec and the
TUI) lets the loop re-derive the compaction threshold after an
escalate_model switch — closing the documented KNOWN LIMITATION where a
switch to a smaller-window model could overflow the target and a larger one
over-compacted. The low-water mark resets on window change so a smaller
window can't be suppressed by stale bookkeeping.

Deliberately not in this PR

  • Trigger raise (0.7 → 0.8) and background/anticipatory compaction with a
    visible "compacting…" notice
    — next PR; they belong together (the
    background path is the validation the compactionTriggerRatio comment
    asks for before raising).
  • Summarizer output-token cap — needs CompletionRequest.MaxTokens
    plumbed through all three providers; follow-up.
  • exec spec-draft wiring — draft runs are short planning sessions; the
    main provider is fine there.

Checklist

  • The linked issue already has the issue-approved label.
    No linked issue — core-team change, same waiver as feat: agent quality, caching, retry, and tooling upgrades #506/perf: universal tool-output ceiling with spill + async post-edit diagnostics #518.
  • go build ./..., go vet ./..., and go test ./... pass locally.
    Build and vet clean. Full suite passes except the 6 pre-existing
    internal/cli failures from the local opengateway provider profile
    ("openai provider opengateway requires official baseURL") —
    byte-identical failure set on main, unrelated to this change.
  • gofmt clean.
  • Tests added/updated for the change (and run under -race where relevant).
    New: summarizer routing (cheap provider gets the call, main gets none,
    factory lazy+memoized), sticky fallback on call failure AND factory
    failure, transcript clamping (bodies + args, head/tail survival),
    reactive prune-before-pay with budget semantics across two recovers,
    updateWindow unit test, end-to-end escalation window re-derivation
    observed via OnContext, and CompactionModelID resolution-order /
    curated-default / already-cheap tests. internal/agent and
    internal/providers pass under -race.
  • UI changes include screenshots or a short recording where possible.
    N/A — no visual changes (a config preference and wiring only).

Summary by CodeRabbit

  • New Features

    • Added an option to use a separate compaction/summarization model, with automatic fallback to the main model if it fails.
    • Compaction now adjusts more accurately when the active model changes mid-run.
  • Bug Fixes

    • Improved recovery from context-limit situations by pruning excess tool output before summarizing.
    • Reduced summarization payload size to help avoid failures on large tool calls and results.

…s, free reactive prune, window re-derive

Compaction summarization was the single most expensive recurring event
in a long run: the full elided middle — verbatim tool bodies included —
went to the session's main model at full price (a ~100k-token call near
a 200k window, recursively multiplied on overflow).

Four changes, each independently safe:

- Dedicated summarizer model. Compaction summaries route to a cheap
  model resolved as ZERO_COMPACTION_MODEL > preferences.compactionModel
  > a curated default on official endpoints (Haiku 4.5 / Gemini
  Flash-Lite / gpt-4.1-mini; custom-endpoint catalogs are unknowable so
  they get none). Built lazily on the first paid compaction; ANY
  failure permanently falls back to the main provider for the run, so a
  misconfigured summarizer can never break compaction. "main" at any
  level forces the old behavior.

- Summarizer input caps. renderTranscript clamps tool-result bodies to
  2,000 chars and tool-call arguments to 500 (head+tail, rune-safe) —
  the summary needs what was done and decided, not raw payloads. Cuts
  summarizer input 5-10x on tool-heavy sessions.

- Free reactive prune. recover() now runs the same prune-first stage as
  the proactive path: a context-limit error is first answered by
  pruning stale tool bodies at zero cost, without consuming the
  one-shot reactive budget; the paid summarizer only runs when a later
  error finds nothing left to prune.

- Window re-derivation on model switch. Options.ContextWindowFor
  (wired to the model registry in exec and the TUI) lets the loop
  re-derive the compaction threshold after escalate_model, closing the
  documented KNOWN LIMITATION where a switch to a smaller-window model
  could overflow and a larger one over-compacted.
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Zero automated PR review

Verdict: No blockers found

Blockers

  • None found.

Validation

  • [pass] Diff hygiene: git diff --check
  • [pass] Tests: go test ./...
  • [pass] Build: go run ./cmd/zero-release build
  • [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: fed37e58b861
Changed files (12): internal/agent/compaction.go, internal/agent/compaction_cheap_test.go, internal/agent/loop.go, internal/agent/types.go, internal/cli/app.go, internal/cli/exec.go, internal/cli/summarizer.go, internal/config/types.go, internal/providers/compaction_model.go, internal/providers/compaction_model_test.go, internal/tui/model.go, internal/tui/options.go

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds support for routing agent compaction summarization to a dedicated cheaper provider model, with automatic fallback to the main provider on failure, transcript payload clamping to shrink summarizer input, and mid-run context-window recalibration after model escalation, plus configuration and CLI/TUI wiring for a CompactionModel preference.

Changes

Cheap Summarizer Compaction Feature

Layer / File(s) Summary
Options and config contracts
internal/agent/types.go, internal/config/types.go, internal/tui/options.go
agent.Options gains Summarizer and ContextWindowFor callbacks; PreferencesConfig and tui.Options gain CompactionModel string fields.
Compaction summarizer fallback and transcript clamping
internal/agent/compaction.go
compactionState memoizes a dedicated summarizer, permanently disables it after failure, falls back to main provider, adds updateWindow for mid-run recalibration, prunes stale tool output before paid summarization in recover, and clamps tool-call args/results in renderTranscript.
Loop context window recalibration
internal/agent/loop.go
Run recomputes context window via ContextWindowFor and calls compactor.updateWindow after model escalation.
Agent compaction tests
internal/agent/compaction_cheap_test.go
New tests cover dedicated summarizer usage, failure fallback/stickiness, factory errors, transcript clamping, prune-before-summarize recovery, window updates, and end-to-end escalation.
Compaction model selection
internal/providers/compaction_model.go, internal/providers/compaction_model_test.go
New CompactionModelID resolves env override, preference, curated per-provider defaults, and skips when main model is already cheap; tests cover precedence and defaults.
CLI and TUI wiring
internal/cli/summarizer.go, internal/cli/exec.go, internal/cli/app.go, internal/tui/model.go
New summarizerFactory builds a provider for the compaction model; wired into agent.Options in exec.go; CompactionModel passed into TUI options in app.go; TUI model overrides Summarizer/ContextWindowFor using providers.CompactionModelID.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Run as agent.Run
  participant State as compactionState
  participant Cheap as Dedicated Summarizer
  participant Main as Main Provider

  Run->>State: maybeCompact / recover(context-limit error)
  State->>State: pruneStaleToolOutput (recover only)
  alt still over threshold
    State->>State: summarizeClosureFor
    State->>Cheap: build & call summarizer
    alt cheap succeeds
      Cheap-->>State: summary
    else cheap fails
      State->>State: mark summarizerBroken
      State->>Main: call main provider
      Main-->>State: summary
    end
  end
  Run->>State: updateWindow(new context window on escalation)
Loading

Possibly related PRs

  • Gitlawb/zero#135: Both PRs modify internal/agent/compaction.go's summarization closure and fallback mechanism.
  • Gitlawb/zero#332: Both PRs modify how context window is derived per-model for compaction sizing.
  • Gitlawb/zero#484: Both PRs modify compactionState and the proactive/reactive compaction paths (maybeCompact/recover).

Suggested reviewers: gnanam1990, Vasanthdev2004


🐇 A cheap little brain now does the trimming,
Falls back to the boss when its focus is dimming,
Tool blobs get clipped at head and at tail,
Windows resize when the models don't fail,
Compaction hops on with a lighter-weight whim.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main change: cheaper compaction via a dedicated summarizer and transcript input capping.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/cheap-compaction

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (5)
internal/agent/compaction_cheap_test.go (1)

178-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for a partial/insufficient prune reclaim.

Once the recover ordering/fit-check fix (see compaction.go review) lands, add a case here where pruneStaleToolOutput reclaims a small amount that still leaves the request over threshold — asserting the call falls through to the paid summarizer within the same recover invocation rather than returning a still-overflowing retry.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/agent/compaction_cheap_test.go` around lines 178 - 230, The current
test only covers a full free-prune retry and a later paid summarize retry; add
coverage for the partial-prune path in TestRecoverPrunesBeforeSummarizing. After
the recover ordering/fit-check change, create a case where pruneStaleToolOutput
removes only a small amount but the history is still over the context limit, and
assert recover falls through to the summarizer within the same invocation
instead of returning a retry that still overflows. Use the existing recover,
pruneStaleToolOutput, and state.reactiveAttempted/provider.requests checks to
verify the paid summarizer is reached immediately when pruning is insufficient.
internal/providers/compaction_model_test.go (1)

35-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No test exercises the OpenAI curated-default branch.

defaultCompactionModel's config.ProviderKindOpenAI case (defaultOpenAICompactionModel) is never hit by any test here — coverage stops at Anthropic, Google, and openai-compatible (no-default). Consider adding an OpenAI case to TestCompactionModelIDCuratedDefaults to close the gap on this switch statement.

✅ Suggested addition
 	// Custom/compatible endpoints: catalog unknowable, no default.
 	compatible := config.ProviderProfile{
 		Name:         "opengateway",
 		ProviderKind: config.ProviderKindOpenAICompatible,
 		BaseURL:      "https://opengateway.gitlawb.com/v1",
 		Model:        "some-model",
 	}
 	if got := CompactionModelID(compatible, ""); got != "" {
 		t.Fatalf("openai-compatible must have no default, got %q", got)
 	}
+
+	openai := config.ProviderProfile{Name: "openai", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-5"}
+	if got := CompactionModelID(openai, ""); got != defaultOpenAICompactionModel {
+		t.Fatalf("openai default = %q, want %q", got, defaultOpenAICompactionModel)
+	}
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/providers/compaction_model_test.go` around lines 35 - 70, Add an
OpenAI provider case to TestCompactionModelIDCuratedDefaults so the
config.ProviderKindOpenAI branch in CompactionModelID/defaultCompactionModel is
exercised. Use a config.ProviderProfile with Name/ProviderKindOpenAI and a
non-curated model, then assert it returns defaultOpenAICompactionModel,
alongside the existing Anthropic, Google, and openai-compatible checks.
internal/providers/compaction_model.go (1)

34-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Update the Google compaction default internal/providers/compaction_model.go:34-38defaultGoogleCompactionModel still points at gemini-2.5-flash-lite, while the current budget-tier Flash-Lite model is gemini-3.1-flash-lite. This hardcoded catalog entry will drift unless someone revisits it periodically.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/providers/compaction_model.go` around lines 34 - 38, Update the
hardcoded Google compaction default in the compaction model constants so
defaultGoogleCompactionModel points to the current Flash-Lite model instead of
the older gemini-2.5-flash-lite value; keep the change limited to the constants
block in compaction_model.go and verify any provider selection logic that reads
defaultGoogleCompactionModel continues to use the updated catalog entry.
internal/cli/summarizer.go (1)

17-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a unit test for summarizerFactory.

This is a small, pure adapter with three distinct branches (no model → nil, no newProvider → nil, resolved model → lazy closure with the model swapped in). Worth a direct test in internal/cli given it's the wiring point for a cost-sensitive feature, and the existing test coverage (compaction_cheap_test.go, compaction_model_test.go) is one layer removed from this file.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/summarizer.go` around lines 17 - 27, Add a unit test for
summarizerFactory in internal/cli that covers its three branches: returning nil
when providers.CompactionModelID yields an empty model ID, returning nil when
newProvider is nil, and returning a lazy closure when a model is resolved. Use
summarizerFactory and its returned func(context.Context) (agent.Provider, error)
to verify the closure swaps profile.Model to the resolved compaction model while
preserving the rest of the provider profile.
internal/tui/model.go (1)

4445-4463: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extracting the summarizer-construction logic into providers to avoid duplicating internal/cli/summarizer.go's logic.

The CompactionModelID → mutate profile.Model → wrap newProvider sequence here duplicates summarizerFactory in internal/cli/summarizer.go. Since tui likely can't import cli (the dependency runs the other way via deps.runTUI), a shared helper in internal/providers (e.g. providers.SummarizerFactory(profile, preference, newProvider)) would let both call sites stay in sync as the resolution rules evolve.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/tui/model.go` around lines 4445 - 4463, The summarizer setup in the
compaction path duplicates the same model-selection and provider-wrapping flow
used by summarizerFactory, so extract that construction into a shared helper in
providers. Move the CompactionModelID lookup, profile.Model mutation, and
newProvider wrapper into a reusable providers helper such as SummarizerFactory,
then have both internal/tui/model.go and internal/cli/summarizer.go call it to
keep the resolution rules in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/agent/compaction.go`:
- Around line 519-531: The cheap-first pruning path in recover is currently
blocked by the reactiveAttempted budget guard and returns too early on any
reclaimed bytes, so it can miss a recoverable overflow. Move the free-prune
check so it runs before the one-shot reactive budget gate, mirroring
maybeCompact, and after pruneStaleToolOutput validate that the pruned request
actually fits the limit before returning retried=true. Use the existing symbols
recover, maybeCompact, pruneStaleToolOutput, and the token-fit logic around
calibratedTokens/estimateTokens/estimateToolDefTokens to keep the free retry and
paid summarization fallback behavior correct.

---

Nitpick comments:
In `@internal/agent/compaction_cheap_test.go`:
- Around line 178-230: The current test only covers a full free-prune retry and
a later paid summarize retry; add coverage for the partial-prune path in
TestRecoverPrunesBeforeSummarizing. After the recover ordering/fit-check change,
create a case where pruneStaleToolOutput removes only a small amount but the
history is still over the context limit, and assert recover falls through to the
summarizer within the same invocation instead of returning a retry that still
overflows. Use the existing recover, pruneStaleToolOutput, and
state.reactiveAttempted/provider.requests checks to verify the paid summarizer
is reached immediately when pruning is insufficient.

In `@internal/cli/summarizer.go`:
- Around line 17-27: Add a unit test for summarizerFactory in internal/cli that
covers its three branches: returning nil when providers.CompactionModelID yields
an empty model ID, returning nil when newProvider is nil, and returning a lazy
closure when a model is resolved. Use summarizerFactory and its returned
func(context.Context) (agent.Provider, error) to verify the closure swaps
profile.Model to the resolved compaction model while preserving the rest of the
provider profile.

In `@internal/providers/compaction_model_test.go`:
- Around line 35-70: Add an OpenAI provider case to
TestCompactionModelIDCuratedDefaults so the config.ProviderKindOpenAI branch in
CompactionModelID/defaultCompactionModel is exercised. Use a
config.ProviderProfile with Name/ProviderKindOpenAI and a non-curated model,
then assert it returns defaultOpenAICompactionModel, alongside the existing
Anthropic, Google, and openai-compatible checks.

In `@internal/providers/compaction_model.go`:
- Around line 34-38: Update the hardcoded Google compaction default in the
compaction model constants so defaultGoogleCompactionModel points to the current
Flash-Lite model instead of the older gemini-2.5-flash-lite value; keep the
change limited to the constants block in compaction_model.go and verify any
provider selection logic that reads defaultGoogleCompactionModel continues to
use the updated catalog entry.

In `@internal/tui/model.go`:
- Around line 4445-4463: The summarizer setup in the compaction path duplicates
the same model-selection and provider-wrapping flow used by summarizerFactory,
so extract that construction into a shared helper in providers. Move the
CompactionModelID lookup, profile.Model mutation, and newProvider wrapper into a
reusable providers helper such as SummarizerFactory, then have both
internal/tui/model.go and internal/cli/summarizer.go call it to keep the
resolution rules in sync.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: db672f43-3e2b-4da5-98fd-1a78c661f0d7

📥 Commits

Reviewing files that changed from the base of the PR and between d185f3e and fed37e5.

📒 Files selected for processing (12)
  • internal/agent/compaction.go
  • internal/agent/compaction_cheap_test.go
  • internal/agent/loop.go
  • internal/agent/types.go
  • internal/cli/app.go
  • internal/cli/exec.go
  • internal/cli/summarizer.go
  • internal/config/types.go
  • internal/providers/compaction_model.go
  • internal/providers/compaction_model_test.go
  • internal/tui/model.go
  • internal/tui/options.go

Comment on lines +519 to +531
// CHEAP FIRST STAGE (mirrors maybeCompact): a context-limit failure can
// often be fixed for free by pruning old tool-result bodies. A free retry
// does NOT consume the one-shot reactive budget — if the pruned request
// still overflows, the next recover finds nothing left to prune and pays
// for summarization then.
if pruned, reclaimed := pruneStaleToolOutput(messages, state.preserveLast); reclaimed > 0 {
state.lowWaterMark = state.calibratedTokens(estimateTokens(pruned) + estimateToolDefTokens(tools))
return pruned, true, nil
}

result, compactErr := Compact(messages, CompactionOptions{
PreserveLast: state.preserveLast,
Summarize: summarizeClosure(ctx, provider, state.onUsage),
Summarize: state.summarizeClosureFor(ctx, provider),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Free-prune stage is gated behind the one-shot budget and skips the fit-check — can strand a recoverable overflow.

Two issues with the new cheap-first-stage in recover:

  1. The state.reactiveAttempted guard (just above, unchanged) runs before this free-prune stage. Once any paid compaction has fired once this run, every later context-limit error skips free pruning entirely — contradicting the stated intent that pruning "does NOT consume the one-shot reactive budget."
  2. Unlike maybeCompact's mirrored stage (which checks size <= state.threshold before short-circuiting, line 469), this branch returns retried=true on any reclaimed > 0 without checking whether the pruned result actually fits. If the reclaim is insufficient (e.g., an oversized tool result inside the preserveLast protection window), the retried request overflows again — and since the loop only invokes recover once per triggering site, the paid-summarizer fallback inside this same call is never reached. The run fails even though summarization could have saved it.

TestRecoverPrunesBeforeSummarizing only covers the "reclaim is definitely enough" case, so this gap isn't caught by tests.

🔧 Suggested fix: reorder the guard and validate the reclaim before short-circuiting
-	if state.reactiveAttempted {
-		return messages, false, nil
-	}
 	if !isContextLimitError(errorMessage) {
 		return messages, false, nil
 	}
 
-	// CHEAP FIRST STAGE (mirrors maybeCompact): a context-limit failure can
-	// often be fixed for free by pruning old tool-result bodies. A free retry
-	// does NOT consume the one-shot reactive budget — if the pruned request
-	// still overflows, the next recover finds nothing left to prune and pays
-	// for summarization then.
-	if pruned, reclaimed := pruneStaleToolOutput(messages, state.preserveLast); reclaimed > 0 {
-		state.lowWaterMark = state.calibratedTokens(estimateTokens(pruned) + estimateToolDefTokens(tools))
-		return pruned, true, nil
-	}
+	// CHEAP FIRST STAGE (mirrors maybeCompact): attempted regardless of the
+	// one-shot reactive budget, since it's free. Only short-circuit when the
+	// pruned history actually fits — otherwise fall through to the paid
+	// summarizer below instead of wasting a retry that fails again.
+	if pruned, reclaimed := pruneStaleToolOutput(messages, state.preserveLast); reclaimed > 0 {
+		size := state.calibratedTokens(estimateTokens(pruned) + estimateToolDefTokens(tools))
+		state.lowWaterMark = size
+		if size <= state.threshold {
+			return pruned, true, nil
+		}
+		messages = pruned
+	}
+
+	if state.reactiveAttempted {
+		return messages, false, nil
+	}
 
 	result, compactErr := Compact(messages, CompactionOptions{
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// CHEAP FIRST STAGE (mirrors maybeCompact): a context-limit failure can
// often be fixed for free by pruning old tool-result bodies. A free retry
// does NOT consume the one-shot reactive budget — if the pruned request
// still overflows, the next recover finds nothing left to prune and pays
// for summarization then.
if pruned, reclaimed := pruneStaleToolOutput(messages, state.preserveLast); reclaimed > 0 {
state.lowWaterMark = state.calibratedTokens(estimateTokens(pruned) + estimateToolDefTokens(tools))
return pruned, true, nil
}
result, compactErr := Compact(messages, CompactionOptions{
PreserveLast: state.preserveLast,
Summarize: summarizeClosure(ctx, provider, state.onUsage),
Summarize: state.summarizeClosureFor(ctx, provider),
if !isContextLimitError(errorMessage) {
return messages, false, nil
}
// CHEAP FIRST STAGE (mirrors maybeCompact): attempted regardless of the
// one-shot reactive budget, since it's free. Only short-circuit when the
// pruned history actually fits — otherwise fall through to the paid
// summarizer below instead of wasting a retry that fails again.
if pruned, reclaimed := pruneStaleToolOutput(messages, state.preserveLast); reclaimed > 0 {
size := state.calibratedTokens(estimateTokens(pruned) + estimateToolDefTokens(tools))
state.lowWaterMark = size
if size <= state.threshold {
return pruned, true, nil
}
messages = pruned
}
if state.reactiveAttempted {
return messages, false, nil
}
result, compactErr := Compact(messages, CompactionOptions{
PreserveLast: state.preserveLast,
Summarize: state.summarizeClosureFor(ctx, provider),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/agent/compaction.go` around lines 519 - 531, The cheap-first pruning
path in recover is currently blocked by the reactiveAttempted budget guard and
returns too early on any reclaimed bytes, so it can miss a recoverable overflow.
Move the free-prune check so it runs before the one-shot reactive budget gate,
mirroring maybeCompact, and after pruneStaleToolOutput validate that the pruned
request actually fits the limit before returning retried=true. Use the existing
symbols recover, maybeCompact, pruneStaleToolOutput, and the token-fit logic
around calibratedTokens/estimateTokens/estimateToolDefTokens to keep the free
retry and paid summarization fallback behavior correct.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I found issues that need to be addressed before this is ready.

Findings

  • [P1] Complete CodeRabbit's request to keep insufficient prune from consuming the retry
    internal/agent/compaction.go:524
    CodeRabbit's unresolved review item is still valid: recover returns retried=true as soon as pruneStaleToolOutput reclaims any bytes, without checking whether the pruned request is now under the compaction threshold. The caller then retries that same turn once; if the reclaim was too small, the retry hits the context limit again and the paid Compact fallback in this recover call is never reached, even though summarization could have saved the turn. The same block is also behind the reactiveAttempted guard, so once a paid reactive compaction has happened, later context-limit errors skip the supposedly free prune stage entirely. Please complete the review request by moving the free prune before the paid-budget guard and only short-circuiting when the pruned request actually fits; otherwise fall through to summarization in the same call.

  • [P2] Keep cheap summarization after escalating from a cheap model
    internal/providers/compaction_model.go:75
    When the run starts on the curated cheap model, CompactionModelID returns an empty model ID and summarizerFactory installs no dedicated summarizer. If --allow-escalation then switches the run from claude-haiku-4.5 to its catalog upgrade target claude-sonnet-4.5, agent.Run updates the main provider and context window but never revisits the summarizer factory. Any later compaction in that now-expensive run summarizes on Sonnet instead of staying on Haiku, which loses the core cost-saving behavior exactly in the cheap-start/strong-escalation workflow this PR enables. Please refresh the summarizer choice after a successful model switch, or avoid disabling the dedicated summarizer for runs that can escalate away from the cheap model.

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I read through this and jatmn's two findings both hold up against the code — confirming with specifics so they can be addressed in one pass.

P1 (compaction.go:516): confirmed, with one nuance. The cheap-prune stage short-circuits on reclaimed > 0, not on "pruned request now fits under threshold." So a too-small prune returns retried=true, the caller retries the same turn, the retry overflows again, and only the next recover call falls through to the paid Compact (because the second prune reclaims nothing). Summarization is reached eventually, so "never reached" overstates it — the real cost is one wasted failed model round-trip per too-small prune. jatmn's fix (only short-circuit when the pruned request actually fits, else fall through to Compact in the same call) kills that wasted round and is the right shape. On the reactiveAttempted part: the free prune IS gated behind if state.reactiveAttempted at the top of recover, so once a paid compaction has fired, a later context-limit error skips the free prune entirely. Since prune doesn't consume the budget, running it before that guard (and gating only the paid Compact path behind reactiveAttempted) would give one free retry that can't loop — worth doing alongside the threshold check.

P2 (compaction_model.go + cli/summarizer.go): confirmed exactly. summarizerFactory returns nil when CompactionModelID is "" — which is what happens when the run starts on the cheap model (TestCompactionModelIDSkipsWhenMainIsAlreadyCheap). So options.Summarizer is nil, summarizeProvider falls back to main, and at startup main IS the cheap model — fine. But after escalate_model switches haiku→sonnet, updateWindow (loop.go) re-derives the threshold but leaves summarizerFactory nil, so summarizeProvider now returns the new main (sonnet). The run summarizes on Sonnet from then on, which is the opposite of the PR's point. The cheap model is now strictly cheaper than main, so post-escalation is exactly when a dedicated summarizer should be installed. Either rebuild the factory after a successful switch (CompactionModelID(sonnet) returns haiku, so the builder is now non-nil) or don't disable the summarizer up front for runs that can escalate. The escalation test added here only asserts the window is re-derived to 400000 — it doesn't cover the summarizer path, so P2 ships with no test catching it.

The clamp-for-summary work (summaryToolResultClamp / summaryToolArgsClamp) is sound — it only touches the summarizer transcript, not the live messages, so there's no session data loss. I like the overall direction; the cost regression in P2 is the one I'd want fixed before merge since it hits the exact cheap-start/strong-escalation workflow this PR opens up.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants