fix(compaction): previous summary is duplicated when a later split degrades#109828
Merged
steipete merged 2 commits intoJul 17, 2026
Conversation
clawsweeper
Bot
added
proof: sufficient
Contributor
|
Codex review: needs maintainer review before merge. Reviewed July 17, 2026, 12:46 PM ET / 16:46 UTC. Summary PR surface: Source +4, Tests +22. Total +26 across 3 files. Reproducibility: yes. The supplied composed current-main run reproduces two copies of the prior summary when the oldest split succeeds and a later split degrades, and the added regression case models the same reachable sequence. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused classification change after exact-head checks confirm both invariants: restore the prior summary when the oldest split degrades, and do not restore it again when the oldest split survives but a later split degrades. Do we have a high-confidence way to reproduce the issue? Yes. The supplied composed current-main run reproduces two copies of the prior summary when the oldest split succeeds and a later split degrades, and the added regression case models the same reachable sequence. Is this the best way to solve the issue? Yes. Narrowing the result kind to whether the oldest split degraded answers the safeguard consumer's actual restoration question, removes the competing coarse flag, and preserves the existing oldest-split outage behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5366b6de8171. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +4, Tests +22. Total +26 across 3 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (4 earlier review cycles)
|
steipete
force-pushed
the
fix/compaction-duplicate-summary
branch
from
July 17, 2026 16:40
b761716 to
3c3cec0
Compare
…plit degrades summarizeInStages stamped a merged result as generic-fallback whenever ANY split degraded. compaction-safeguard reads that kind to decide whether to restore the previous summary, on the documented assumption that "a generic fallback means redistillation never happened". That assumption only holds for chunk 0. summarizeViaLLM prepends the previous summary as the first message, so it always lands in the oldest chunk. When that chunk summarizes fine but a later one degrades, the merge already carries the redistilled summary — and the safeguard prepends it a second time. Every subsequent compaction that has any degraded split re-adds it again, so a PR that exists to stop token burn compounds it instead. The kind now reports whether the oldest split degraded, which is exactly the question the only consumer asks. The coarse "any chunk" flag is gone rather than kept alongside the precise one, so there is a single source of truth. Behavior for a degraded oldest split is unchanged: the previous summary is genuinely lost there and restoring it is still correct.
steipete
force-pushed
the
fix/compaction-duplicate-summary
branch
from
July 17, 2026 16:41
3c3cec0 to
8ad8cfd
Compare
Contributor
|
Maintainer review complete. This is the best fix for the compaction boundary: only loss of the oldest split requires prior-summary restoration; later fallback placeholders remain represented in the successful merge. Proof:
Ready for maintainer prepare/merge. |
clawsweeper
Bot
added
rating: 🦞 diamond lobster
Contributor
|
Merged via squash.
|
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Jul 18, 2026
…grades (openclaw#109828) * fix(compaction): stop duplicating the previous summary when a later split degrades summarizeInStages stamped a merged result as generic-fallback whenever ANY split degraded. compaction-safeguard reads that kind to decide whether to restore the previous summary, on the documented assumption that "a generic fallback means redistillation never happened". That assumption only holds for chunk 0. summarizeViaLLM prepends the previous summary as the first message, so it always lands in the oldest chunk. When that chunk summarizes fine but a later one degrades, the merge already carries the redistilled summary — and the safeguard prepends it a second time. Every subsequent compaction that has any degraded split re-adds it again, so a PR that exists to stop token burn compounds it instead. The kind now reports whether the oldest split degraded, which is exactly the question the only consumer asks. The coarse "any chunk" flag is gone rather than kept alongside the precise one, so there is a single source of truth. Behavior for a degraded oldest split is unchanged: the previous summary is genuinely lost there and restoring it is still correct. * test(compaction): pin previous-summary restoration boundary --------- Co-authored-by: Peter Steinberger <[email protected]>
github-project-automation
Bot
moved this from Todo
to Done
in Upstream OSS contributions
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
2 participants
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.
What Problem This Solves
When staged compaction summarizes a long session and any split degrades, the previous summary is added
to the new summary twice. It then compounds: every later compaction cycle that has a degraded split
re-adds it again, so the summary grows a copy of itself each round and burns tokens on every subsequent
turn.
Since #86900 landed the staged circuit breaker to stop token burn when the summarizer is unavailable,
this is the same failure mode arriving through the merge path.
Why This Change Was Made
summarizeInStages(src/agents/compaction.ts:492) stamped the merged result as a fallback if anysplit degraded:
The only consumer of that kind is
summarizeViaLLM(src/agents/agent-hooks/compaction-safeguard.ts:413-420),which reads it to decide whether to restore the previous summary:
That comment states an invariant the code does not hold. The same function prepends the previous
summary as the first message (
:396-399→prependPreviousSummaryForRedistill,:107), so it alwayslands in chunk 0. If chunk 0 summarizes fine and a later chunk degrades, redistillation demonstrably
did happen — the merge already carries the summary — but the coarse flag stamps the result as a fallback
anyway, and the previous summary gets prepended on top of a merge that already contains it.
Reachable on the normal path:
MAX_CONSECUTIVE_GENERIC_FALLBACKS(:48) only opens the circuit onconsecutive degradations, so a single failed split completes the merge, and
compaction-safeguard.ts:1375passes
previousSummaryon every repeat compaction.The fix reports what the consumer actually asks — did the oldest split degrade — instead of "did anything
degrade". The coarse flag is removed rather than kept beside the precise one, so there is one source of
truth and the two cannot drift apart. With this, the existing comment becomes true.
Nothing outside these two modules reads the kind, so no other surface is affected.
compaction-safeguard.tsneeds no change: its logic was already correct given a correct kind.
Evidence
End-to-end, composing the real
summarizeInStageswith the verbatim decision fromsummarizeViaLLM:413-420, with chunk 0 succeeding and a later chunk degrading:Before (
origin/main):After (this branch):
Tests
The added case (
oldest summary→ degraded →newest summary→ merge) fails onorigin/mainwithexpected { kind: 'generic-fallback' } to deeply equal { kind: 'summary' }, and passes here.The existing
remains degradedcase is unmodified and still green — its oldest split is the one thatrejects, so the previous summary really is lost there and the fallback stamp is still correct. That is the
behavior split this change is built around.
compaction-planning-worker.test.tsfails identically on unmodifiedorigin/mainin this environment(
Cannot find module compaction-planning.js— the worker wants built output), so it is unrelated to thischange.
Merge Risk
Low. One flag in one function, +14/-4 in source, no signature or type change, no config surface.
The only behavior that moves is the case this fixes: a merged summary whose oldest split survived is now
reported as
summaryinstead ofgeneric-fallback. The degraded-oldest-split path — the one the existingtest pins — is untouched, so the outage protection that #86900 added still holds exactly where it matters.
AI-assisted.