fix(compaction): preserve thinking blocks during overflow

[lavaman131]

Summary

Prevents critical-overflow context compaction from deleting, partially filtering, or instructing removal of assistant thinking/redacted_thinking content blocks. Also repairs unsafe persisted deletion filters during session rebuild so existing affected sessions recover without mutating Anthropic thinking-bearing assistant turns.

Closes #1386.

Changes

New: thinking-blocks.ts

Extracted shared detection utilities:

• isAssistantThinkingBlockType(type) — type guard for "thinking" / "redacted_thinking"
• contentArrayHasAssistantThinkingBlock(content) — scans a content array
• messageHasAssistantThinkingContentBlock(message) — checks a full message

Core compaction (context-compaction.ts)

• Adds assistantEntryHasThinkingContentBlock helper to detect thinking-bearing CompactableTranscriptEntry values
• Adds assertNoAssistantThinkingDeletionTargets — throws if any reconciled deletion target touches an assistant entry containing thinking blocks (whole-entry or per-block); runs post-reconciliation in standard mode
• Adds assertNoLatestRetainedThinkingAssistantContentBlockDeletionTargets — throws during critical_overflow if a content-block deletion targets the latest retained thinking-bearing assistant entry
• Updates validateContextDeletionRequest to call both assertions after tool-dependency reconciliation
• Updates createContextDeletionTool grep path to skip (rather than match) assistant entries and blocks belonging to thinking-bearing entries, emitting "assistant_thinking_entry" / "assistant_thinking_block" skip reasons
• Adds has_assistant_thinking_blocks, role, and type fields to StoredTranscriptEntry, StoredContentBlock, and their SQL row types
• Rewrites contextCompactionModePrompt for critical_overflow: removes the instruction to evict reasoning traces first; adds an explicit invariant prohibiting deletion of thinking-bearing assistant entries or their sibling blocks
• Switches prepareContextCompaction to use buildEffectiveContextDeletionFilters instead of raw buildContextDeletionFilters

Session rebuild (session-manager.ts)

• Exports buildEffectiveContextDeletionFilters — replays all compaction entries and skips any deletion target whose assistant message carries a thinking block
• When a content-block target for the latest retained thinking-bearing assistant is skipped, collects paired toolCall IDs and finds their toolResult entries so active context has no dangling tool calls
• Later valid content-block deletion filters for restored multi-block tool-result entries are still applied (provenance-aware repair)
• Replaces the default argument of buildContextDeletionFilteredPath and the call in branch-summarization.ts with buildEffectiveContextDeletionFilters so every session rebuild is safe

Tests

• context-compaction-deletion-tool.test.ts — grep-path skips on thinking entries and blocks, skip reason assertions
• context-compaction.test.ts — validateContextDeletionRequest rejection of thinking-block targets (whole-entry and per-block), critical-overflow prompt wording invariant
• build-context.test.ts — persisted-filter repair: stale whole-entry deletion, stale block deletion, paired tool-result restoration, provenance-aware preservation of later valid block filters on restored tool results

Docs & changelog

• docs/compaction.md — extends the critical-overflow description to document the thinking-block verbatim-preservation invariant and the session-rebuild repair behavior
• CHANGELOG.md — ### Fixed entry under ## [Unreleased] referencing Verbatim Compaction can corrupt the latest assistant message's thinking blocks #1386

Breaking Changes

None. All changes are additive guard-rails and internal behavior fixes; no public API surfaces changed.

Validation

bun test packages/coding-agent/test/session-manager/build-context.test.ts \
  packages/coding-agent/test/context-compaction.test.ts \
  packages/coding-agent/test/context-compaction-deletion-tool.test.ts
# Result: 73 pass, 0 fail

bun run typecheck
# Result: tsc --noEmit — passed

Pre-push/pre-commit hooks ran successfully (bun run lint, bun run test:unit).

[claude]

Code Review — PR #1393 (fix: preserve thinking blocks during overflow)

Reviewed against CLAUDE.md. Overall this is a careful, well-targeted fix for a high-severity issue (#1386): it addresses both prevention (the validation guard) and recovery (the rebuild guard for already-corrupted sessions), and the test coverage is genuinely thorough — whole-entry skips, sibling-block preservation, paired tool-result restoration, redacted-thinking parity, and the later-valid-filter provenance case are all exercised. Nice work. A few points below, mostly minor, with one design question worth a deliberate decision.

Design question (worth confirming)

Whole-entry deletion of thinking-bearing assistants is now rejected — stricter than the issue proposed, and may reduce overflow-recovery headroom. Issue #1386 explicitly said "Whole-entry deletion of such assistant messages can remain allowed … since that removes the message entirely rather than mutating its thinking blocks in place." The Anthropic invariant is about mutating the latest assistant turn in place; deleting an entire turn (latest or historical) does not violate it. This PR instead forbids whole-entry deletion of any thinking-bearing assistant entry (assertNoAssistantThinkingDeletionTargets, the in-loop entry-kind check, and the grep assistant_thinking_entry skip).

Consequence: in a long interleaved-thinking session, every assistant turn carries thinking blocks, so during critical_overflow none of them are evictable at all. That removes a large, legitimately-reclaimable chunk of context from the LRU pass and could leave a genuine overflow unresolved (the very thing critical-overflow mode exists to fix). If the goal is only to guard the replay invariant, allowing whole-entry deletion while keeping the partial/content-block rejection would preserve more headroom. If the stricter behavior is intentional (the docs/changelog do state it), a one-line code comment explaining why whole-entry deletion is also forbidden would help future readers, since it contradicts the issue own analysis.

Bugs / correctness

• Redundant branch in buildEffectiveContextDeletionFilters (session-manager.ts ~463-472): the entry-kind branch and the fall-through both run the identical loop over collectToolCallContentBlockIds(message.content). The whole if/else collapses to a single loop — target.kind is irrelevant here. Worth simplifying so it does not imply a distinction that does not exist.

Style / maintainability

• let block + ??= in validateContextDeletionRequest: block is declared undefined and only ever assigned via the ??= operator, equivalent to a plain assignment. A const block scoped inside the content_block branch reads more clearly; the current form looks like a refactor artifact.
• Redundant in-loop thinking checks vs. the post-reconcile assert. The per-deletion checks overlap with assertNoAssistantThinkingDeletionTargets, which re-runs on the reconciled targets. Harmless (and the in-loop versions give nicer per-target errors), but a brief comment noting the post-reconcile assert is authoritative — because reconcileToolDependencies can add targets — would clarify intent.
• Duplicated thinking-block predicates across modules. isAssistantThinkingBlockType / hasAssistantThinkingContentBlock now exist in both context-compaction.ts and session-manager.ts with near-identical bodies. Acceptable given the module split, but a shared helper would avoid drift if the set of thinking types ever changes.
• The filters parameter of buildContextDeletionFilteredPath is now effectively ignored when any context_compaction exists — buildEffectiveContextDeletionFilters rebuilds from path rather than honoring the passed filters. All current callers pass the canonical buildContextDeletionFilters(path), so behavior is unchanged in practice, but the parameter contract is now misleading. Consider dropping the param or documenting that it is only consulted when no compaction entries are present.

Scope notes (not blockers)

• The paired tool-result restoration is per-compaction-entry: restoration and a later valid block-filter must live in separate compaction entries (as the test sets up). A valid content_block deletion on a restored tool-result that sits in the same compaction as the skipped whole-entry deletion is dropped. Defensible (treat the stale plan as a unit, let later valid plans trim), but it deserves a code comment so it is not later mistaken for a bug.
• redacted_thinking blocks fall through textFromContentBlock to JSON.stringify(record), so their data payload now appears in grep-searchable/manifest text. Since these entries are unconditionally protected from deletion this is low-risk, but worth being aware of (opaque payload surfaced to the planner).

Tests

Coverage is strong and tracks the suggested matrix closely. Two small additions would round it out:

1. A test asserting overflow recovery still succeeds (some context remains evictable) in a transcript where most assistant turns carry thinking — to pin the design decision above.
2. The cross-vs-same-compaction restoration limitation as an explicit test, so the chosen semantics are locked in.

Conventions

Bun-only workflow, raw-TS (no build step), .js import specifiers, changelog under Unreleased/Fixed, and user-facing docs in docs/compaction.md are all correctly followed. The changelog/doc entries are accurate but very dense single run-on sentences — consider splitting for readability, though they satisfy the comprehensive-not-lazy rule.

Solid, defensive fix overall. The main thing I would like confirmed is the deliberate choice to forbid whole-entry deletion of thinking-bearing assistants vs. the issue recommendation to allow it.

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
bastani	🟢 Ready	View Preview	Jun 15, 2026, 11:49 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

[lavaman131]

Updated per review/user clarification: critical_overflow may now evict old/non-latest thinking-bearing context, while deterministic validation/rebuild safeguards protect the latest retained thinking-bearing assistant message from content-block mutation. I also added a clear context_delete self-correction error for non-deletable latest thinking blocks.\n\nValidation rerun:\n- bun test packages/coding-agent/test/session-manager/build-context.test.ts packages/coding-agent/test/context-compaction.test.ts packages/coding-agent/test/context-compaction-deletion-tool.test.ts — 75 pass, 0 fail\n- bun run typecheck — passed\n- git diff --check — passed

[claude]

Review: fix(compaction): preserve thinking blocks during overflow

Thorough fix with strong regression coverage. The layered approach — prompt guidance, deterministic validation, grep-tool skips, and a rebuild-time repair (buildEffectiveContextDeletionFilters) — is well structured, and the changelog/docs updates are appropriately detailed. A few things worth a closer look before merge.

Potential bug / behavioral inconsistency

1. Content-block eviction of old (non-latest) thinking blocks is silently a no-op on rebuild.

The PR's stated goal is that critical overflow may evict old non-latest thinking blocks. Validation now allows this (context-compaction.ts gates the thinking checks behind mode !== "critical_overflow"), and the grep tool only skips assistant thinking blocks in non-critical mode. So the model can produce a content_block deletion targeting an old thinking block, and validation will accept and persist it.

But on rebuild, filterMessageContentBlocks preserves any thinking-bearing assistant verbatim, not just the latest:

// session-manager.ts:526
case "assistant": {
    if (hasAssistantThinkingContentBlock(message.content)) return message; // verbatim, ignores deletedBlocks
    ...
}

Meanwhile buildEffectiveContextDeletionFilters only skips content-block targets for the latest thinking assistant — for an older thinking assistant it adds the content-block target to the effective filters, which then hit the guard above and are ignored. Net effect: a critical-overflow content-block deletion of an old thinking block validates and reports token reduction, but the block reappears on rebuild, so no headroom is actually reclaimed (the retry can still overflow). Only whole-entry eviction of old thinking assistants actually takes effect.

If only whole-entry eviction is intended, then validation/grep should reject content-block deletions of thinking blocks even in critical overflow (or the rebuild guard should be scoped to the latest thinking assistant). Right now the validation surface and the rebuild surface disagree. The new tests cover whole-entry old-thinking deletion and validation acceptance, but none assert that an old thinking block is actually removed from the rebuilt content — so this gap is untested.

2. Two different definitions of "latest retained assistant".

• context-compaction.ts (assertNoLatestRetainedThinkingAssistantContentBlockDeletionTargets): takes the absolute latest retained assistant, then only protects it if that one has thinking. A newer non-thinking assistant turns off protection for an older thinking assistant.
• session-manager.ts (findLatestRetainedAssistantWithThinking): skips non-thinking assistants and protects the latest thinking-bearing one.

These can pick different entries when a non-thinking assistant follows a thinking assistant. They happen to converge because the rebuild guard in #1 over-preserves, but the divergence is fragile. Worth aligning on one definition tied to the actual provider replay invariant.

Edge case in the rebuild repair

restoredToolResultEntryIds in buildEffectiveContextDeletionFilters is computed per compaction entry. If compaction entry A partially filtered the latest thinking assistant (so its paired tool-call block is restored verbatim) and a later compaction entry B whole-entry-deletes that paired tool result, B's iteration has an empty restoredToolResultEntryIds, so the result deletion is applied — leaving a dangling tool call against a verbatim-preserved assistant turn. buildSessionContext does no tool-dependency reconciliation after buildContextDeletionFilteredPath (unlike validateContextDeletionRequest), so that dangling call would be sent to the provider. The "later valid filters" test only exercises a content-block deletion of the result (which keeps the entry alive), not a whole-entry deletion. This requires stale persisted data — exactly the scenario this PR repairs — so worth either handling cross-compaction-entry restoration or documenting the limitation.

Minor / nits

• validateContextDeletionRequest: let block is hoisted and assigned via block ??= but is only used within the content_block branch — it can stay a local const inside that branch.
• contentBlocksForEntry: the block type is now computed before the existingDeletedBlocks?.has(blockIndex) early-return, so it's computed for blocks about to be discarded. Trivial, but the original order avoided that.
• build-context.test.ts imports from vitest; CLAUDE.md prescribes bun:test + node:assert/strict. Matches the pre-existing file convention, so not introduced here, but flagging for consistency.

Things that look good

• Validation correctly runs the standard-mode assertion after reconcileToolDependencies (nice comment explaining why) so tool-reconciliation can't sneak in a thinking-block deletion.
• redacted_thinking handled consistently alongside thinking everywhere.
• The self-correction error message is clear and actionable for the model.
• Strong regression coverage for prompt wording, the self-correction path, paired tool-result restoration, and persisted-filter repair.

Overall solid; the main item to resolve is the validation-vs-rebuild mismatch in #1 (decide whether partial old-thinking-block eviction is supported, and make both layers agree), plus a quick look at the cross-compaction-entry restoration edge case.

🤖 Generated with Claude Code

[lavaman131]

Addressed the latest review feedback.\n\nChanges in the pushed update:\n- Rebuild now applies accepted old/non-latest thinking content-block deletions, so critical_overflow actually reclaims that context instead of no-oping.\n- The latest-retained-assistant definition is aligned between validation and rebuild: only the absolute latest retained assistant is protected when it contains thinking/redacted_thinking.\n- Paired tool results restored because of a skipped latest-assistant partial filter are protected from later stale whole-entry deletion, preventing dangling retained tool calls. Later valid content-block trimming of those result entries still works.\n- Added regression tests for old thinking block rebuild eviction and later stale whole-result deletion.\n\nValidation rerun:\n- bun test packages/coding-agent/test/session-manager/build-context.test.ts packages/coding-agent/test/context-compaction.test.ts packages/coding-agent/test/context-compaction-deletion-tool.test.ts — 77 pass, 0 fail\n- bun run typecheck — passed\n- git diff --check — passed

[claude]

Code Review — fix(compaction): preserve thinking blocks during overflow

Thanks for the careful, well-documented change. The invariant being protected (don't break provider replay by editing/reindexing the latest thinking/redacted_thinking-bearing assistant turn, while still allowing old thinking to be evicted under emergency overflow) is correctly identified, and the test coverage is genuinely thorough — paired tool-result restoration, later-stale whole-entry deletion, later-valid block filters, and the prompt-wording regression are all exercised. The CHANGELOG/docs updates follow repo conventions. Issues below, roughly in priority order.

🟠 Redundant double-computation of effective filters (perf + API smell)

buildEffectiveContextDeletionFilters is computed twice on the same path during compaction prep:

• prepareContextCompaction calls it at context-compaction.ts:554 and passes the result into buildContextDeletionFilteredPath at :555.
• But buildContextDeletionFilteredPath itself calls buildEffectiveContextDeletionFilters(path, filters) again at session-manager.ts:572.

Crucially, when the path contains any context_compaction entry, buildEffectiveContextDeletionFilters ignores its filters argument entirely and recomputes everything from path (lines 457–518). So passing the already-effective filters in doesn't short-circuit anything — it recomputes the identical result a second time. Correctness is preserved (it's idempotent here), but:

1. It's wasted work on a hot path — buildContextDeletionFilteredPath is also called from buildSessionContext (session-manager.ts:660), which runs on every context rebuild. The path.some(... === "context_compaction") early-return guards the no-compaction case, but once a session has any compaction entry, the full multi-pass scan runs on every rebuild.
2. The filters parameter of buildEffectiveContextDeletionFilters is now effectively vestigial — honored only in the early-return (no-compaction) branch and silently discarded otherwise. A caller that constructs custom filters and passes them would have them silently dropped — a footgun worth either documenting loudly or removing.

Suggestion: have prepareContextCompaction derive filteredPathEntries from the already-computed effectiveDeletionFilters without re-deriving (e.g. a variant of buildContextDeletionFilteredPath that takes pre-computed effective filters and skips the internal recompute), and/or rename the param to make the "recomputes from path" behavior explicit.

🟡 Dead let block + ??= in validateContextDeletionRequest

In the refactor (context-compaction.ts ~line 1035), block is hoisted to let block: CompactableContentBlock | undefined; before the protected check, but it's only ever assigned inside the content_block branch via block ??= entry.contentBlocks.find(...). Since block is always undefined there, the ??= is equivalent to a plain =. This reads as if the hoist enables some earlier use that never materializes. Recommend reverting to a local const block = ... inside the branch unless a follow-up use is planned.

🟡 Repeated full-path scans in buildEffectiveContextDeletionFilters

The path/compaction list is walked several times: the path.some guard, the rawDeletedEntryIds loop, buildToolResultEntryIdsByCallId, the allRestoredToolResultEntryIds loop, and the final emission loop — each O(entries × targets). Fine for typical sessions, but it compounds with the double-call above. If context rebuild ever shows up in a profile, the restored-tool-result set and the emission loop could share a single iteration with a restoresLatestThinkingAssistant pre-pass.

🟢 Minor: type computed for already-deleted blocks

In contentBlocksForEntry the refactor now computes type before the existingDeletedBlocks?.has(blockIndex) early-return, so the work happens even for blocks that are immediately discarded. Negligible, but the original "bail first" ordering was slightly cheaper.

🟢 Minor: redundant guard ordering in grep skip logic

In the content-block grep loop, the isAssistantThinkingBlockType(block.type) check (assistant_thinking_block) and the block.has_assistant_thinking_blocks === 1 check (assistant_thinking_entry) are sequential and both mode !== "critical_overflow" gated. A thinking block in a thinking-bearing entry always hits the first branch, so the two reasons are mutually exclusive by construction — correct, but worth a one-line comment so a future reader doesn't assume the second branch is unreachable.

Things confirmed / double-checked

• latestRetainedThinkingAssistant?.id comparisons: when it's undefined, ?.id is undefined and a string target.entryId never matches, so those branches are correctly skipped. Safe, though a named boolean would make intent clearer.
• Tests use vitest: consistent with the rest of packages/coding-agent/test, which mirrors upstream pi's layout — the bun:test guidance in CLAUDE.md targets the companion raw-TS packages, so this is correct here. No action needed.

Summary

No blocking correctness bugs found — the invariant logic and tool-result restoration appear sound and are well-tested. The main actionable item is the double computation / vestigial filters parameter (🟠), a small perf cost on the rebuild hot path and a maintainability hazard. The rest are minor cleanups. Nice work on the regression coverage.

Automated review — please weigh suggestions against your own judgment.

[lavaman131]

Addressed the latest review feedback.\n\nUpdates pushed:\n- Removed the redundant effective-filter recomputation in prepareContextCompaction: it now computes effective filters once and passes them to buildContextDeletionFilteredPath, whose optional parameter is documented as precomputed effective filters.\n- Updated branch summarization to use buildContextDeletionFilteredPath(entries) directly so persisted filter repair is consistently applied.\n- Simplified the hoisted let block / ??= into a local const block in the content-block branch.\n- Restored the cheaper early-return ordering in contentBlocksForEntry.\n- Added a comment clarifying why standard-mode grep distinguishes thinking blocks from non-thinking sibling blocks in thinking-bearing assistant messages.\n\nValidation rerun:\n- bun test packages/coding-agent/test/session-manager/build-context.test.ts packages/coding-agent/test/context-compaction.test.ts packages/coding-agent/test/context-compaction-deletion-tool.test.ts — 77 pass, 0 fail\n- bun run typecheck — passed\n- git diff --check — passed\n- Commit/push hooks also passed, including bun run lint and bun run test:unit.

[claude]

Review: fix(compaction): preserve thinking blocks during overflow

Thanks for this — the fix is well-targeted, the layered defense (prompt → validation → grep-tool → rebuild repair) is thoughtful, and the test coverage is genuinely strong (validation rejection both ways, critical-overflow asymmetry, tool reconciliation, and the four persisted-filter repair scenarios). A few things worth addressing, none blocking.

Correctness / robustness

1. PR description claims a filterMessageContentBlocks guard that isn’t in the code. The description says it "Guards filterMessageContentBlocks for assistant messages: if the content array contains thinking blocks, returns the message unchanged." But filterMessageContentBlocks (session-manager.ts:538-542) still filters assistant content unconditionally — and it must, otherwise the "applies old thinking content-block deletion filters when a newer assistant exists" test would fail. So protection of the latest thinking assistant rests entirely on buildEffectiveContextDeletionFilters never emitting those filters; there is no defense-in-depth at the apply layer. That’s a defensible design, but please correct the description so future readers don’t assume a safety net that isn’t there.

2. buildContextDeletionFilters stays exported and is now a foot-gun. Every production caller routes through buildEffectiveContextDeletionFilters (verified: context-compaction.ts:552, branch-summarization.ts:197, and the buildContextDeletionFilteredPath default). But the raw buildContextDeletionFilters is still exported, and feeding its result directly to buildContextDeletionFilteredPath(path, rawFilters) would reflow the latest thinking assistant’s blocks and reintroduce #1386. Consider a doc-comment warning on buildContextDeletionFilters pointing callers to the effective variant, or un-exporting it if nothing external needs it.

Maintainability

3. Thinking-detection logic is duplicated across two modules. isAssistantThinkingBlockType / assistantEntryHasThinkingContentBlock (context-compaction.ts) and isAssistantThinkingContentBlock / hasAssistantThinkingContentBlock (session-manager.ts) implement the same type === "thinking" || "redacted_thinking" predicate. Since this string pair is now load-bearing for an Anthropic API invariant, extracting one shared helper avoids the two copies drifting apart later.

4. Skip-reason naming. In the grep content-block path, a non-thinking sibling block is reported with reason: "assistant_thinking_entry". A block-level skip carrying an entry-level reason is a little surprising for anything parsing skipped[]. Intentional, but a distinct reason or a one-line comment would read better.

Performance

5. buildContextDeletionFilteredPath’s default now runs buildEffectiveContextDeletionFilters on every buildSessionContext call (a hot path during rebuild/navigation), which is O(compactions × targets) and iterates the compaction set ~3×. The early if (!path.some(... "context_compaction")) short-circuit keeps compaction-free sessions free, so this is fine in practice — just flagging it for compaction-heavy sessions.

Style / conventions

6. The new helpers lean on unknown + as { type?: unknown } casts, which CLAUDE.md discourages — reasonable here as runtime guards at a deserialization boundary, but a shared typed guard (see #3) would shrink the cast surface.

7. The touched test files import from vitest rather than the bun:test runner CLAUDE.md mandates. This is pre-existing in all three files (not introduced here), so out of scope for this PR — noting only so it’s on the radar.

Minor

• assertNoLatestRetainedThinkingAssistantContentBlockDeletionTargets re-checks unsafeTarget.kind !== "content_block" after already filtering target.kind === "content_block" in the .find() — redundant (harmless).
• No explicit test asserts that whole-entry deletion of the latest thinking assistant is permitted under critical_overflow (the asymmetry vs. partial deletion). Worth a case to lock in the intended behavior.

Overall this is a solid, well-tested fix for #1386. Items 1 and 2 are the ones I'd most want addressed before merge.

— Reviewed with Claude Opus 4.8

[lavaman131]

Addressed the latest review feedback.\n\nUpdates pushed:\n- Corrected the PR description to state that latest-assistant protection is implemented by effective deletion-filter repair, not a filterMessageContentBlocks no-op guard.\n- Added a warning doc comment to buildContextDeletionFilters explaining it returns raw/unrepaired filters and production rebuilds should use buildEffectiveContextDeletionFilters or buildContextDeletionFilteredPath(path).\n- Extracted thinking/redacted-thinking detection into shared src/core/thinking-blocks.ts and wired both compaction validation and session rebuild through it.\n- Added a regression test that whole-entry deletion of the latest thinking-bearing assistant is allowed during critical_overflow, while partial latest content-block deletion remains rejected.\n\nValidation rerun:\n- bun test packages/coding-agent/test/session-manager/build-context.test.ts packages/coding-agent/test/context-compaction.test.ts packages/coding-agent/test/context-compaction-deletion-tool.test.ts — 78 pass, 0 fail\n- bun run typecheck — passed\n- git diff --check — passed\n- Commit/push hooks also passed, including bun run lint and bun run test:unit.

[claude]

Code Review — PR #1393: fix(compaction): preserve thinking blocks during overflow

Thorough, well-tested fix. The provider replay invariant (thinking / redacted_thinking blocks on the latest retained assistant must survive verbatim) is genuinely subtle, and the PR threads the needle of "protect the latest thinking turn while still allowing old thinking eviction" carefully. Validation and rebuild now agree, the shared thinking-blocks.ts helper kills drift, docs/changelog are updated, and the regression suite (17 new cases) covers the important branches: whole-entry deletion of the latest thinking assistant, partial-block rejection with self-correction, persisted-filter repair, paired tool-result restoration, later stale whole-entry result deletion, and later-valid result-block filters. Nice work.

A few non-blocking observations:

1. Misleading skipped.reason label for non-thinking sibling blocks (minor)

context-compaction.ts, grep path — when skipping a non-thinking sibling block inside a thinking-bearing assistant, the reason is set to "assistant_thinking_entry", even though it is a content-block skip that carries a blockIndex:

if (mode !== "critical_overflow" && block.role === "assistant" && block.has_assistant_thinking_blocks === 1) {
    skipped.push({ entryId: block.entry_id, target, blockIndex: block.block_index, reason: "assistant_thinking_entry", text: block.text });
    continue;
}

The actual-thinking-block branch directly above uses "assistant_thinking_block". So a block-level skip is reported with an _entry reason while the more entry-like case uses _block — easy to misread in diagnostics. Consider a dedicated reason (e.g. assistant_thinking_sibling_block) or reusing assistant_thinking_block. Reporting-only, no functional impact.

2. buildEffectiveContextDeletionFilters is now on the hot rebuild path (perf, minor)

It is now the default arg for buildContextDeletionFilteredPath, i.e. it runs on essentially every context rebuild. Two small things:

• session-manager.ts:452 computes const filters = buildContextDeletionFilters(path) unconditionally, but filters is only used by the no-compaction early return at line 453 — it is discarded when compactions exist. Reorder so the raw build only happens on that early-return branch (if (!path.some(...)) return buildContextDeletionFilters(path);).
• The repair does several O(compactions × targets) passes plus a re-scan of the latest assistant content per matching target. For typical sessions (few compaction entries) this is negligible, but for long-lived sessions with many persisted compactions it is worth keeping an eye on. Fine to defer.

3. contentArrayHasAssistantThinkingBlock called without an array guard (robustness, minor)

In buildEffectiveContextDeletionFilters, latestRetainedAssistantContent is passed straight into contentArrayHasAssistantThinkingBlock, which calls .some(...). This relies on assistant content always being an array. TypeScript enforces it and the existing filterMessageContentBlocks assistant branch makes the same assumption, so this is safe today — but unlike messageHasAssistantThinkingContentBlock (which guards with Array.isArray), this path has no defensive check and runs on every rebuild against potentially legacy/persisted sessions. A cheap Array.isArray guard would make the two helpers consistent and rebuild-failure-proof.

4. buildContextDeletionFilters is now effectively internal (cleanup, nit)

After this PR no production rebuild path calls it directly — it is only reached via buildEffectiveContextDeletionFilters. The doc comment flags it as diagnostics-only, which is good. Just confirm no external/extension consumer depended on the old buildContextDeletionFilteredPath(entries, buildContextDeletionFilters(entries)) shape (branch-summarization was correctly updated to drop the second arg).

Things that look correct

• The "no latest thinking assistant" path produces filters equivalent to the previous raw behavior — good backward compatibility.
• Post-reconcile assertions (assertNoLatestRetainedThinkingAssistantContentBlockDeletionTargets for critical_overflow, assertNoAssistantThinkingDeletionTargets for standard) correctly run after reconcileToolDependencies, so reconciliation-injected targets cannot smuggle a partial deletion onto the protected turn.
• Moving normalizeRawTarget earlier in the validation loop is behavior-preserving.
• Changelog correctly appends under ## [Unreleased] -> ### Fixed per CLAUDE.md rules.

Overall: solid, defensible change with strong test coverage. Only nits above — none blocking.

[claude]

Review: fix(compaction): preserve thinking blocks during overflow

Thorough, well-tested fix for #1386. The core invariant — never let critical-overflow compaction partially mutate the latest retained thinking-bearing assistant turn, while still allowing eviction of older reasoning — is implemented consistently across the prompt, validation, grep tool, and session-rebuild repair. The extraction of thinking-blocks.ts is clean and the test coverage is genuinely strong (grep skips, per-block + whole-entry validation rejection, tool-reconciliation rejection, critical-overflow allowances, persisted-filter repair, paired tool-result restoration). Nice work.

A few points worth a look before merge:

1. Standard mode now forbids whole-entry deletion of any thinking-bearing assistant — is that intended? (design question)

In validateContextDeletionRequest (context-compaction.ts ~L197):

```ts
if (mode !== "critical_overflow" && deletion.kind === "entry" && assistantEntryHasThinkingContentBlock(entry)) {
throw new Error(`Cannot delete assistant entry ${deletion.entryId} because it contains thinking/redacted_thinking content blocks`);
}
```

The verbatim-preservation rationale (avoiding block reindexing / breaking Anthropic signature integrity) really only applies to partial content-block deletion. Deleting an entire old assistant turn doesn't reindex any surviving turn's blocks, and tool-dependency reconciliation already cleans up orphaned tool results. With extended thinking enabled, most assistant turns carry a thinking block, so this makes them all undeletable in standard /compact and threshold auto-compaction — which could materially reduce how much space standard compaction can reclaim. Critical overflow correctly allows whole-entry deletion (see the "allows whole-entry deletion of the latest thinking-bearing assistant" test), so the asymmetry is deliberate-looking, but it's worth confirming this is the intended product trade-off rather than over-restriction. If intended, a one-line comment explaining why standard mode is stricter than critical overflow here would help future readers.

2. Wasted work in buildEffectiveContextDeletionFilters (minor)

session-manager.ts L452 computes const filters = buildContextDeletionFilters(path); then only returns it on the no-compaction early path. When compaction entries exist, filters is discarded and effectiveFilters is rebuilt from scratch. Cheap to fix — do the path.some(...) check first and only call buildContextDeletionFilters in the early-return branch.

3. This heavier function is now the default on the rebuild hot path (perf note)

buildContextDeletionFilteredPath's default param changed from buildContextDeletionFilters to buildEffectiveContextDeletionFilters, which does several extra O(n) passes plus map construction on every session rebuild. It short-circuits when there are no context_compaction entries, so the common case is fine, but for long sessions with compaction history this runs the full repair on each rebuild. prepareContextCompaction correctly avoids the double-compute by passing pre-built filters; just flagging the added cost on the general rebuild path. Probably acceptable.

4. Reuse already-computed content (nitpick)

At L483 you re-cast (latestRetainedThinkingAssistant.message as { content: readonly unknown[] }).content. latestRetainedAssistantContent was already narrowed/computed above and is the same array — reusing it avoids the extra cast.

5. buildEffectiveContextDeletionFilters is intricate (maintainability)

The two-pass structure with restoresLatestThinkingAssistant + allRestoredToolResultEntryIds and the two skip conditions at L512–513 is subtle (whole-entry deletions of paired results are skipped universally, but later content-block trims of restored multi-block results are allowed). The inline comment helps, but a short summary comment at the function top describing the three cases it handles would aid future readers. Not blocking.

Things that are good / verified

• assistantEntryHasThinkingContentBlock checks both the derived contentBlocks and the raw message — good defense against thinking blocks filtered out of contentBlocks via existingDeletedBlocks.
• ContextDeletionMemoryStore is purely in-memory, so the new role / type / has_assistant_thinking_blocks "row" fields need no SQLite schema migration.
• When the latest retained assistant has no thinking block, buildEffectiveContextDeletionFilters reduces to the same target set as the old raw filters — behavior is preserved for the common case.
• CHANGELOG entry is under ## [Unreleased] / ### Fixed and docs (compaction.md) are updated, per CLAUDE.md.

Note: I did not re-run bun run typecheck / the test suite locally (sandbox restriction); relying on the PR's stated 73 pass, 0 fail + green typecheck.

Overall: solid, focused fix. The only thing I'd genuinely want answered before merge is #1 — whether blocking whole-entry deletion of thinking-bearing assistants in standard mode is intended.