fix(compaction): keep folded user turns out of summarizer judgement - #8419
Merged
Conversation
A constraint stated mid-session ("do not change the public API") existed
nowhere but the transcript, yet only the first user turn was pinned and the
rest reached the fold region — so whether it survived was one summarizer
judgement call. The code it constrains stays re-derivable from the workspace;
the constraint does not. That asymmetry of loss, not the token count, is what
decides here.
Every user turn in the fold region now stays verbatim within a budget, on the
existing kept channel rather than a new one. Three properties make it hold:
- It is structural, not a keep-policy bit. A stated constraint is a fact about
the task, so it is not something a policy flag can switch off.
- It ignores policyStart. The keep policy is scoped to messages after the
latest digest so it cannot grow forever; user turns are bounded by budget
instead, which is what lets protection survive repeated compaction.
- It measures with a fixed estimate. A threshold tracking the last turn's
calibrated ratio would keep a turn at one checkpoint and fold it at the next.
Retention is bounded, not unconditional: hoisting user turns without a budget
is what padded an earlier revision's candidates past the acceptance ceiling,
failing compaction outright instead of degrading it. One turn may spend 1500
tokens and all of them min(8192, window*5%), oldest first — the recent tail
already covers the newest turns. Past those bounds a turn folds like any other
content, so KeepUserMarked joins the default policy and [[keep]] becomes the
documented way past the size budget.
Two tests had fixed the defect as correct behavior.
TestCompactKeepsMidSessionUserTurns asserted the mid-session turn must fold and
then relied on a fake summarizer echoing the fact back; its reply now drops the
fact entirely, so the turn has to survive on its own. The partition test that
asserted all user turns fold is rewritten with budget, total-budget and
across-digest guards. TestKeepUserMarkedRequiresUserPrefixMarker moves down to
isUserMarked, which is the only level that can still observe the marker match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Only the first user turn was pinned verbatim through a fold
(
pinnedPrefixLen). Every later one entered the fold region, where its survivalcame down to a single summarizer judgement call — with two fallbacks that rarely
apply: landing inside the recent tail, or the user having manually typed
[[keep]]under a policy bit that was off by default.So a constraint stated at turn 8 of a 40-turn session ("do not change the public
API", "Go 1.24 compatible", "no new dependencies") could disappear from the
projection while the digest still read as perfectly reasonable. That is the
failure mode where a summary looks right and has quietly dropped a requirement.
The asymmetry is the point: the code a constraint governs stays re-derivable
from the workspace, while the constraint itself exists nowhere but the
transcript. Token count is the wrong thing to rank on; cost of loss is the right
one.
Change
Every user turn in the fold region now stays verbatim within a budget, carried
on the existing
keptchannel (keepIndexes→partitionFoldForProjection)rather than a new mechanism.
pinnedPrefixLenis deliberately untouched — it isthe cache prefix boundary (
msgs[:head]opens the projection) and must stay acontiguous prefix.
Three properties make the protection hold:
task, not content a policy flag should be able to switch off. It sits at the
same level as
isCompactionSummaryin the partition.policyStart. The keep policy is scoped to messages after thelatest digest so it cannot grow without bound; user turns are bounded by
budget instead, which is what lets protection survive repeated compaction
rather than lapsing at the next checkpoint.
ratio would keep a turn at one checkpoint and fold it at the next. Merged with
fixedPinnableUserTurn's duplicate expression intofixedTokenEstimate.Retention is bounded, not unconditional.
compact_partition_test.gorecordedthat an earlier revision hoisted early user turns and it padded candidates past
the acceptance ceiling — which fails compaction outright instead of degrading it.
One turn may spend 1500 tokens, all of them
min(8192, window x 5%), oldestfirst (the recent tail already covers the newest turns, and an old turn has
survived more folds than a new one). Past those bounds a turn folds like any
other content, so
KeepUserMarkedjoins the default policy and[[keep]]is nowthe documented escape hatch for an oversize turn.
Tests that had fixed the defect as correct behavior
TestCompactKeepsMidSessionUserTurnsread like a guard for this behavior butasserted the opposite: the mid-session turn must fold, and the fact then had
to come back via a fake summarizer echoing it. Its reply is now
"Standing facts: none"— dropping the fact entirely — so the turn has tosurvive on its own. It runs the real
a.compact()and asserts onvisibleContext(a), i.e. the provider-visible projection.TestPartitionFoldsAllUserTurnsByDefaultbecomesTestPartitionKeepsSmallUserTurnsVerbatim, joined by guards for the per-turnceiling, the total budget, and retention across a prior digest.
TestKeepUserMarkedRequiresUserPrefixMarkermoves down toisUserMarked: atkeepIndexeslevel every small user turn is kept regardless, which would havehidden a broken marker match.
Verification
repolint's two remaining findings (
desktop/frontend/wailsjs/go/models.ts,repo file-size total) are pre-existing on main-v2. The baseline was not
widened: a 6-line comment and a +2-line growth in
boot.gothat this changeintroduced were fixed in the code instead.
Scope
This closes the "summarizer dropped it" failure mode only. It does not track
constraint lifecycle — if turn 8 says "do not change the public API" and turn
30 says "actually, go ahead", both now survive verbatim and the model reads the
resolution itself. Status tracking (active / satisfied / superseded) is a
separate concern and not attempted here.
Cache-impact: low - the stable system-prompt prefix (base prompt + tools + memory) is byte-identical; the only change is which messages compose the post-checkpoint projection, and compaction is already a declared cache reset point. Kept user turns are capped at 5% of the window, well inside the 50% acceptance ceiling.
Cache-guard: internal/agent/cachehit_e2e_test.go (unchanged, passing) plus TestCompactKeepsMidSessionUserTurns, which drives the real compact() and asserts on the provider-visible projection.
System-prompt-review: esengine - internal/boot/boot.go changes one default policy value (KeepUserMarked added to the nil-config default); no system-prompt text, tool schema, or memory content is touched.
Documentation-impact: updated - docs/SPEC.md "What survives a fold" rewritten (it still described the removed early-user-turn hoist), with the matching section added to docs/SPEC.zh-CN.md.