Skip to content

fix(sdk): context-compact no longer repeats a compaction that changed nothing - #65

Merged
matej21 merged 1 commit into
mainfrom
fix/compaction-preamble-floor
Nov 6, 2026
Merged

matej21 merged 1 commit into
mainfrom
fix/compaction-preamble-floor

Conversation

@matej21

@matej21 matej21 commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

Problem

contextCompactPlugin decides whether to compact from the provider's prompt size of the previous inference (lastInferenceMetrics.promptTokens). Two paths make an agent compact again without effect, and limitsGuardPlugin counts each one toward maxCompactions:

  1. Stale prompt size. context_compacted replaces the history but leaves lastInferenceMetrics unchanged. If the turn after a compaction is paused, or its inference fails and is resumed, the next attempt sees the pre-compaction prompt size and compacts again. It summarises its own summary and counts another compaction.
  2. Empty compaction. When every message is still inside keepRecentMessages, there is nothing to compact. Yet compactIfNeeded returned an empty result and the plugin emitted context_compacted for it. An agent whose system prompt and tools are already over maxTokens does this on every early turn.

We saw a related case in production: an agent whose immutable preamble alone was larger than maxTokens, so every prompt stayed over the threshold, every turn compacted, and the agent was paused at maxCompactions. History compaction cannot fix that, and the cause belongs with whoever builds the preamble. What it needs from the SDK is a clear signal.

Change

  • The plugin keeps a per-agent state slice (contextCompaction, rebuilt from the event log) and skips compaction until an inference has completed since the last one.
  • ContextCompactor.compactIfNeeded returns null when nothing was compacted, so no context_compacted is emitted for it. compact() itself is unchanged.
  • A warning is logged when the first inference after a compaction is still over maxTokens. It names the configured threshold and the measured prompt size, because every following turn will compact again. This is diagnostic only; the trigger is unchanged.

An earlier version of this PR raised the threshold after an ineffective compaction. Review showed that this could push the prompt past the model's context window, which the plugin does not know, so it was dropped.

Tests

context-compact.integration.test.ts, new repeated compaction block. All three tests fail on main:

  • No compaction is emitted while every message is in the kept tail (one event on main).
  • A turn paused right after its compaction does not compact again on resume (two on main).
  • A turn whose inference fails after a compaction and is resumed after the error backoff does not compact again (three on main).

The existing test "after compaction, subsequent inference sees fewer messages" passed only because of an empty compaction. It now drives a real one and asserts that the request got shorter.

The warning is not asserted, because the test harness uses a silent logger.

bun test packages/sdk/src: 1872 pass, 0 fail. bun run ts:build and bun run lint pass.

Status

🤖 Generated with Claude Code

@matej21

matej21 commented Nov 6, 2026

Copy link
Copy Markdown
Member Author

Review round 1: changes

Reviewer: Claude agent (general-purpose subagent, Opus 5.5), independent of the author session. Commit reviewed: 583cd4d.

# Severity Finding
1 blocker Raising the threshold to "post-compaction prompt + maxTokens" assumes the excess is in what compaction cannot remove. Often it is not: a large message in the first turn after compaction, or large kept messages. Reproduced with maxTokens 500, keepRecentMessages 2 and a provider limit of 1000. The base recovered (829 → 229 tokens). This PR let the prompt grow to 1037, which the provider rejected with context_length. The message was lost, and compaction never fired again because the last accepted prompt was below the raised threshold. When maxTokens is a large fraction of the context window, the raised threshold is always above the window.
2 major Ignoring messagesRemoved: 0 applies only to this plugin's state. The plugin still emits context_compacted when nothing was compacted, and limits-guard and session-stats count it. A fresh agent whose system prompt and tools exceed maxTokens emits one no-op per turn until its history is longer than keepRecentMessages.
3 minor No test for an inference that fails after compaction, an inference_retried, or a threshold that exceeds the provider limit.
4 nit A handler-skipped inference records promptTokensAfter: 0, so it is not treated as an inference for the floor.

Checked with no issue found: skipping compaction until an inference completes after the previous one (failure, context_length, retry, pause and resume, reload), plugin builder usage, and the module-private helpers.

@matej21
matej21 force-pushed the fix/compaction-preamble-floor branch from 583cd4d to 52298c9 Compare November 6, 2026 13:32
@matej21 matej21 changed the title fix(sdk): context-compact stops compacting every turn when compaction cannot get the prompt under maxTokens fix(sdk): context-compact no longer repeats a compaction that changed nothing Nov 6, 2026
@matej21

matej21 commented Nov 6, 2026

Copy link
Copy Markdown
Member Author

Response to review round 1

The design was narrowed, and the PR now holds a single commit, 52298c9.

# Outcome
1 Fixed by removal. The threshold raise is gone; the trigger is maxTokens again. A warning is logged when the first inference after a compaction is still over maxTokens, and behaviour is unchanged.
2 Fixed in 52298c9. compactIfNeeded returns null when nothing was compacted, so no context_compacted is emitted and limits-guard and session-stats no longer count it. The reducer guard is removed.
3 Fixed in 52298c9. New test: an inference fails after a compaction and the turn is resumed after the error backoff. The threshold test is gone along with the rule.
4 No change needed. A handler skip records promptTokens: 0, so the next decision sees no stale size, and with no threshold raise there is nothing to miss.

… nothing

Two paths made an agent compact again without effect, each counted toward limits-guard's
maxCompactions:

- context_compacted replaces the history but leaves lastInferenceMetrics alone. When the turn
  after a compaction was paused or its inference failed, the next attempt compacted again on
  the prompt size measured before the first compaction, summarising the summary. The plugin
  now keeps a per-agent state slice and skips compaction until an inference has completed
  since the last one.
- With every message inside keepRecentMessages there is nothing to compact, yet
  compactIfNeeded ran compact(), which returned an empty result the plugin emitted as
  context_compacted. compactIfNeeded now returns null in that case without calling compact().

A warning is logged when the first inference after a compaction is still over maxTokens. That
means either large recent messages the next compaction will remove, or a system prompt, tools
and immutable preamble too large on their own, in which case every following turn compacts.

The existing test "after compaction, subsequent inference sees fewer messages" passed only on
such an empty compaction; it now drives a real one and checks the request got shorter.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@matej21

matej21 commented Nov 6, 2026

Copy link
Copy Markdown
Member Author

Review round 2: approve

Reviewer: same Claude agent as round 1 (general-purpose subagent, Opus 5.5). Commit reviewed: 52298c9.

Verified:

  • compactIfNeeded returning null has one production caller, and every event consumer only counts or displays the event.
  • The skip-until-an-inference-completes rule cannot block a due compaction, because the history cannot grow before an inference_completed. Checked for context_length, inference_retried, a handler skip, reload and an aborted inference.
  • The warning fires at most once per compaction.
  • All new tests fail on main, and the paused and failed cases fail even with only the compactor change applied.
# Severity Finding
1 minor "after compaction, subsequent inference sees fewer messages" never checks the message count. messagesInThirdCall is written but never read.
2 nit On main, the failed-turn test fails only by timeout, because each repeated compaction re-arms the mock's failures.
3 nit The warning comment and the commit message claim a prompt over maxTokens after compaction means the uncompactable part is too large. It can also be large recent messages that the next compaction removes.
4 nit While every message is kept, compact() still logs "Starting context compaction" and "No messages to compact" on every turn, although no event is emitted.

@matej21
matej21 force-pushed the fix/compaction-preamble-floor branch from 52298c9 to f685f79 Compare November 6, 2026 13:38
@matej21

matej21 commented Nov 6, 2026

Copy link
Copy Markdown
Member Author

Response to review round 2

All fixes are folded into the single commit f685f79.

# Outcome
1 Fixed in f685f79. The test records the message count of each regular request and asserts that the compacted third request carries fewer than the second one plus the turn's two messages. The unused variable is removed.
2 Fixed in f685f79. Failures are armed only on the first summary request. On main the test now fails on its assertion (3 compactions instead of 1).
3 Fixed in f685f79. The comment and the commit message name both causes.
4 Fixed in f685f79. compactIfNeeded checks the kept-message count first and returns null without calling compact(), so no misleading logs. compact() keeps its own guard for direct callers.

Only minor items and nits, so no further round. bun test packages/sdk/src: 1872 pass. ts:build and lint pass.

@matej21
matej21 marked this pull request as ready for review November 6, 2026 13:38
@matej21
matej21 merged commit 565bcf7 into main Nov 6, 2026
1 check passed
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.

1 participant