feat(compaction)!: remove summary compaction in favor of verbatim - #1313
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Review: feat(compaction)! — remove summary compaction in favor of verbatim (#1305) Thanks for this — a large, well-structured removal. The new deletion-shaped hook contract is clean, the security thinking around the extension-facing preparation snapshot is excellent, and test coverage for the new hooks (cancel / deletion request / empty-request rejection / immutable-transcript validation / offline deletion) is genuinely thorough. Comments below, roughly ordered by importance. 🔴 1. Legacy session reload resurrects the full pre-compaction transcript This is the one I would most like confirmed before merge. In buildSessionContext() (session-manager.ts) the old code honored a legacy compaction boundary — it emitted the summary, then only messages from firstKeptEntryId onward. The new code drops that branch entirely and iterates the whole filteredPath with The compaction entry itself is now inert (good — the summary is no longer injected), but the message entries before firstKeptEntryId are still ancestors on the path, so they now get re-appended into active context. For any session previously summary-compacted, resuming it will restore the entire pre-compaction transcript that summarization had removed AND drop the summary that replaced it. That is not data loss (the JSONL is append-only), but it can cause a large, surprising context jump — potentially an immediate overflow on load — for existing sessions. The changelog says historical type:compaction lines are inert and not injected, which is true for the summary, but does not capture that the messages it replaced come back. Suggestions: confirm this is intended; consider a migration (migrateSessionEntries) that converts a legacy compaction boundary into an equivalent context_compaction deletion of the pre-firstKeptEntryId entries (or keep honoring firstKeptEntryId as a deletion boundary in buildSessionContext); and add a test that loads a session containing a legacy compaction entry and asserts the resulting active context (currently uncovered). 🟡 2. isLegacyCompactionSummaryMessage obfuscation chat-message-renderer.ts uses 🟡 3. Lost exhaustiveness checks Several never-based exhaustive switches were replaced with silent fall-throughs to handle legacy runtime roles: convertToLlm (default return undefined), messageText (trailing return ""), and addMessageToChat (default break). These are needed because legacy compactionSummary messages can still appear at runtime, but they also silently swallow any genuinely new role added later. Consider keeping the exhaustive never check while branching the known-legacy role explicitly first, so future additions still fail at compile time. 🟡 4. Duplicated planner path in _applyContextVerbatimCompaction In agent-session.ts, the "extension handlers present but no deletionRequest" branch and the "no extension handlers" branch run identical resolvePlannerAuth() + runContextCompact(...) logic (the same call copied verbatim). Worth extracting a small local runPlanner() helper to dedupe. 🟢 5. deepFreeze(structuredClone(preparation)) cost Cloning + deep-freezing the whole transcript on every compaction is fine in the common case (only when a session_before_compact handler is registered, and compaction already pays an LLM round-trip). Minor notes: it is O(transcript size) for very large sessions, and structuredClone throws if any message field is ever non-cloneable. Low risk, just flagging. 🟢 6. Dropped coverage: extension throws fallback to planner The old "continue with default compaction if extension throws" test was removed. The behavior is still preserved by runner.emit() per-handler try/catch (it logs via emitError and leaves result undefined, so the planner runs). Since that is a meaningful guarantee, consider re-adding a small test asserting a throwing session_before_compact handler falls through to planner compaction. 🟢 7. Broadened hasTaskBearingContext context-compaction.ts — the "would leave no user task" guard now accepts custom, branchSummary, and entryType branch_summary in addition to user. Reasonable given compactionSummary is gone, but it means a session whose only surviving task-bearing context is a branch summary now passes. Confirm that is intended. Overall a solid, careful change, and the direction (verbatim-only, auditable deletion) is the right one. Item 1 is the only thing I would consider blocking pending confirmation/testing; the rest are cleanups. 🤖 Generated with Claude Code |
PR Review:
|
Route compaction documentation, APIs, tests, and runtime paths toward verbatim-only context compaction for #1305. BREAKING CHANGE: Legacy summary compaction APIs and hook shapes are removed in favor of verbatim context compaction. Assistant-model: GPT-5.5
…1305+1309) sections
Addresses review comments on the verbatim-only compaction change: - agent-session: isolate session_compact observer errors from the committed compaction. The hook fires after backup/persist/rebuild, so a throwing observer is now routed to the non-fatal extension-error channel instead of rejecting a successful, already-persisted compaction. - agent-session: extract a single runPlanner() closure, removing the triplicated resolve-auth/run-planner fallback blocks. - messages/context-compaction: restore compile-time exhaustiveness (never) guards in convertToLlm and messageText. This surfaced that the upstream pi-agent-core AgentMessage union still includes the legacy compactionSummary role, now handled explicitly (inert: excluded from LLM context) instead of silently falling through. - context-compaction: extract a documented isTaskBearingEntry() predicate shared by the critical-overflow protection and the task-bearing guard; a surviving branch summary intentionally satisfies the guard even when every user message is evicted under critical overflow. - tests: add critical-overflow task-bearing assertions; run the credential-less deletion-shaped compaction tests unconditionally and gate only the planner fallback behind ANTHROPIC_API_KEY. Assistant-model: Claude Opus 4.8
05c715c to
ad9046f
Compare
|
Code Review — PR #1313 (verbatim-only compaction + ctx.ui.custom) Reviewed the runtime, extension hooks, session-manager, and the workflows custom-UI path against CLAUDE.md conventions. This is a large, well-structured removal — the dead summary path is excised cleanly and the new exhaustiveness guards are a real improvement. Findings are ordered by impact; nothing here is a blocker. Strengths
Suggestions (1) Perf — build extensionPreparation only when a handler exists. In (2) Robustness — structuredClone can throw DataCloneError. Related to (1): if any entry in (3) Please confirm intent — broadened task-bearing validation. (4) Minor — type-only dep added to the dependency-light authoring contract. (5) Nit — duplicated no-op fallback. The two (6) Nit — hil-answer-notifications now scans all runs x stages per store notification (footprint-based) instead of diffing previous vs. current snapshots, relying entirely on emitOnce dedup. Correct and simpler, just O(runs x stages) per notify() — fine at current scale. Conventions Overall: solid, well-tested change. (1) and (2) are the two I would act on before merge; the rest are confirm/optional. Generated with Claude Code |
… dedupe fallback Addresses the second-round review on PR #1313 (_applyContextVerbatimCompaction): - Build the deep-frozen extension snapshot only when a session_before_compact handler exists, instead of on every compaction. Compaction fires when the transcript is largest, so the common no-extension path no longer deep-clones and freezes the whole transcript for nothing. - Guard structuredClone with try/catch: a non-cloneable entry would otherwise raise a raw DataCloneError and turn a viable compaction into a hard failure. It now surfaces a clear error. Transcript entries are plain data, so this is a latent-invariant guard on the (now hot) path. - Collapse the two byte-identical runPlanner() fallback blocks into a single post-hook `if (!validated)` branch. Reviewer items 3/4/6 were confirm/notes only: the broadened task-bearing predicate is the previously-confirmed intended behavior, so no code change. Assistant-model: Claude Opus 4.8
Claude Code Review — PR #1313 (verbatim-only compaction +
|
) * feat(compaction)!: remove summary compaction Route compaction documentation, APIs, tests, and runtime paths toward verbatim-only context compaction for #1305. BREAKING CHANGE: Legacy summary compaction APIs and hook shapes are removed in favor of verbatim context compaction. Assistant-model: GPT-5.5 * chore(release): prepare 0.8.28-alpha.1 * feat(workflows): ctx.ui.custom HIL prompts (#1309) — workflows-only, drift removed * docs(changelog): organize 0.8.27-alpha.1 (#1311) and 0.8.28-alpha.1 (1305+1309) sections * refactor(compaction): address PR #1313 review feedback Addresses review comments on the verbatim-only compaction change: - agent-session: isolate session_compact observer errors from the committed compaction. The hook fires after backup/persist/rebuild, so a throwing observer is now routed to the non-fatal extension-error channel instead of rejecting a successful, already-persisted compaction. - agent-session: extract a single runPlanner() closure, removing the triplicated resolve-auth/run-planner fallback blocks. - messages/context-compaction: restore compile-time exhaustiveness (never) guards in convertToLlm and messageText. This surfaced that the upstream pi-agent-core AgentMessage union still includes the legacy compactionSummary role, now handled explicitly (inert: excluded from LLM context) instead of silently falling through. - context-compaction: extract a documented isTaskBearingEntry() predicate shared by the critical-overflow protection and the task-bearing guard; a surviving branch summary intentionally satisfies the guard even when every user message is evicted under critical overflow. - tests: add critical-overflow task-bearing assertions; run the credential-less deletion-shaped compaction tests unconditionally and gate only the planner fallback behind ANTHROPIC_API_KEY. Assistant-model: Claude Opus 4.8 * refactor(compaction): lazy extension snapshot, guard structuredClone, dedupe fallback Addresses the second-round review on PR #1313 (_applyContextVerbatimCompaction): - Build the deep-frozen extension snapshot only when a session_before_compact handler exists, instead of on every compaction. Compaction fires when the transcript is largest, so the common no-extension path no longer deep-clones and freezes the whole transcript for nothing. - Guard structuredClone with try/catch: a non-cloneable entry would otherwise raise a raw DataCloneError and turn a viable compaction into a hard failure. It now surfaces a clear error. Transcript entries are plain data, so this is a latent-invariant guard on the (now hot) path. - Collapse the two byte-identical runPlanner() fallback blocks into a single post-hook `if (!validated)` branch. Reviewer items 3/4/6 were confirm/notes only: the broadened task-bearing predicate is the previously-confirmed intended behavior, so no code change. Assistant-model: Claude Opus 4.8
Summary
Removes the legacy summary compaction path so all compaction — manual `/compact`, auto threshold/overflow, SDK, RPC, and extension-triggered — flows exclusively through verbatim (deletion-only) compaction via `contextCompact()`. Also ships `ctx.ui.custom()` for graph-visible custom TUI human-in-the-loop prompts in workflows.
Closes #1305. Closes #1309. Rebased on top of the P0 fix from #1311 (`fix(compaction): stabilize deletion planner`), which is already in `main`.
Breaking Changes
Historical `type:"compaction"` JSONL lines on disk are inert — they are never re-injected into active LLM context.
Key Changes
Compaction (breaking)
Workflows: `ctx.ui.custom()` HIL prompts
Validation
Review feedback addressed (2026-06-08)
Rebased onto the latest `main` (now past the `0.8.27` release plus workflow fixes #1316/#1317); changelog/version sections reconciled and `main`'s unreleased entries folded into `0.8.28-alpha.1`. The branch is now conflict-free.
Scope — intentional stacking of #1309. This PR deliberately bundles the compaction removal (#1305) and the workflows `ctx.ui.custom()` HIL feature (#1309). The two changes touch disjoint areas (`packages/coding-agent/src/core/compaction/` vs `packages/workflows/`) and are individually revertible by commit.
Addressed items: