fix(compression): materialize nested compression summaries - #10
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (25)
📝 WalkthroughWalkthroughCompression summaries now use templates with runtime child placeholders. Dynamic compression materializes child summaries before commit, token estimation excludes covered content, and storage and protocol validation reject unresolved summaries and duplicate child references. ChangesCompression Summary Flow
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/agent-core/src/compression/types.ts (1)
55-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a nominal marker to separate templates from materialized summaries.
CompressionSummaryTemplateandCompressionSummaryare structurally identical. TypeScript therefore accepts a template anywhere a materialized summary is required, so the whole contract depends on runtime checks. A branded field would make the distinction compile-time enforceable.♻️ Proposed brand
export interface CompressionSummaryTemplate { + readonly __materialized?: never; readonly sections: CompressionSummarySections; } export interface CompressionSummary { + readonly __materialized: true; readonly sections: CompressionSummarySections; }Note that adding a runtime brand field would change the persisted shape, so
exact(summary, ["sections"])inpackages/protocol/src/guards.tswould need the same update. A type-only phantom marker avoids that.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/compression/types.ts` around lines 55 - 61, Add a type-only nominal brand to CompressionSummaryTemplate and CompressionSummary so they are no longer structurally interchangeable, without adding any runtime or persisted field. Update the relevant construction and type-narrowing sites to satisfy the phantom marker while keeping the serialized shape limited to sections; do not alter guards unless required by runtime changes.packages/agent-core/src/compression/summary.test.ts (1)
96-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the two throwing paths of materialization.
The suite covers the success path only. Two error paths stay untested:
summary.tsline 80 throwsCompressionSummaryValidationErrorwhen the template fails validation.summary.tsline 85 throwsRequired child block ${ref} does not existwhenblocksByRefomits a required ref.The second path is reachable from
prepareDynamicRangeCompression, which converts the throw into a rejection result. A test pins that contract.💚 Proposed tests
+ test("rejects a required child block that is absent from state", () => { + const parentTemplate = template({ sections: sections("Before (b1) after") }); + + expect(() => materializeCompressionSummaryTemplate(parentTemplate, ["b1"], {})) + .toThrow("Required child block b1 does not exist"); + }); + + test("rejects a template whose placeholder is not required", () => { + const parentTemplate = template({ sections: sections("Unexpected (b9)") }); + + expect(() => materializeCompressionSummaryTemplate(parentTemplate, [], {})) + .toThrow("Placeholder (b9) is not a required child block ref"); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/compression/summary.test.ts` around lines 96 - 109, Add tests alongside the existing “compression summary materialization” coverage for both throwing paths: assert that invalid input to materializeCompressionSummaryTemplate raises CompressionSummaryValidationError, and assert that a missing required ref in blocksByRef raises the expected “Required child block ${ref} does not exist” error. Also cover prepareDynamicRangeCompression with an omitted required block and verify it returns the established rejection result rather than propagating the exception.packages/agent-core/src/compression/summary.ts (1)
90-95: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFlatten child summaries before parent materialization.
renderCompressionSummary(child.summary)inserts ten##markers into one parent section. This creates nested or inline headings, such asBefore ## Current Objective. Each nesting level also repeats the complete child rendering. The token estimate can therefore reportsavedTokens === 0, while the commit path still accepts the block. Use a header-free child representation and add a depth test that bounds rendered headers and confirms positive savings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/compression/summary.ts` around lines 90 - 95, Update the child-summary substitution in the section materialization flow around COMPRESSION_SUMMARY_SECTION_NAMES to use a header-free, flattened representation instead of renderCompressionSummary(child.summary), preventing nested or inline ## headings and repeated rendering at each depth. Add a depth-focused test that bounds the rendered header count and verifies positive token savings while preserving successful block commitment.packages/agent-core/src/store/helpers.ts (1)
844-847: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: use Zod 4's unified
errorparam.
.refine(fn, "Compression child block refs must be unique")uses the plain-string message form. Zod 4 keeps this working but deprecates it in favor of the unifiederrorparam.♻️ Optional modernization
const CompressionChildBlockRefsSchema = z.array(BlockRefSchema).refine( (refs) => new Set(refs).size === refs.length, - "Compression child block refs must be unique", + { error: "Compression child block refs must be unique" }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/store/helpers.ts` around lines 844 - 847, Modernize CompressionChildBlockRefsSchema by replacing the deprecated plain-string refine message with Zod 4’s unified error parameter, while preserving the existing uniqueness validation and message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/agent-core/src/compression/summary.test.ts`:
- Around line 96-109: Add tests alongside the existing “compression summary
materialization” coverage for both throwing paths: assert that invalid input to
materializeCompressionSummaryTemplate raises CompressionSummaryValidationError,
and assert that a missing required ref in blocksByRef raises the expected
“Required child block ${ref} does not exist” error. Also cover
prepareDynamicRangeCompression with an omitted required block and verify it
returns the established rejection result rather than propagating the exception.
In `@packages/agent-core/src/compression/summary.ts`:
- Around line 90-95: Update the child-summary substitution in the section
materialization flow around COMPRESSION_SUMMARY_SECTION_NAMES to use a
header-free, flattened representation instead of
renderCompressionSummary(child.summary), preventing nested or inline ## headings
and repeated rendering at each depth. Add a depth-focused test that bounds the
rendered header count and verifies positive token savings while preserving
successful block commitment.
In `@packages/agent-core/src/compression/types.ts`:
- Around line 55-61: Add a type-only nominal brand to CompressionSummaryTemplate
and CompressionSummary so they are no longer structurally interchangeable,
without adding any runtime or persisted field. Update the relevant construction
and type-narrowing sites to satisfy the phantom marker while keeping the
serialized shape limited to sections; do not alter guards unless required by
runtime changes.
In `@packages/agent-core/src/store/helpers.ts`:
- Around line 844-847: Modernize CompressionChildBlockRefsSchema by replacing
the deprecated plain-string refine message with Zod 4’s unified error parameter,
while preserving the existing uniqueness validation and message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d80078cf-5d32-4ac5-b909-845a1dee4cc0
📒 Files selected for processing (24)
packages/agent-core/src/agents/query/hooks/auto-compact.test.tspackages/agent-core/src/agents/query/hooks/hybrid-compression.test.tspackages/agent-core/src/agents/query/hooks/hybrid-compression.tspackages/agent-core/src/compression/constants.tspackages/agent-core/src/compression/dcp-parity.test.tspackages/agent-core/src/compression/dynamic-range.test.tspackages/agent-core/src/compression/dynamic-range.tspackages/agent-core/src/compression/original-range.test.tspackages/agent-core/src/compression/state.test.tspackages/agent-core/src/compression/state.tspackages/agent-core/src/compression/summary.test.tspackages/agent-core/src/compression/summary.tspackages/agent-core/src/compression/types.tspackages/agent-core/src/compression/validation.tspackages/agent-core/src/store/helpers.test.tspackages/agent-core/src/store/helpers.tspackages/agent-core/src/store/projection.test.tspackages/agent-core/src/store/reduce.tspackages/agent-core/src/store/session-read-projection.test.tspackages/agent-core/src/tools/builtins/compress.test.tspackages/agent-core/src/tools/builtins/compress.tspackages/protocol/src/compression.tspackages/protocol/src/guards.test.tspackages/protocol/src/guards.ts
💤 Files with no reviewable changes (3)
- packages/agent-core/src/store/session-read-projection.test.ts
- packages/agent-core/src/agents/query/hooks/auto-compact.test.ts
- packages/agent-core/src/tools/builtins/compress.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 24 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
BREAKING CHANGE: reject compression summaries that carry child refs or unresolved block placeholders.
80c770b to
5bb74c5
Compare
|
Review triage for current head
Local validation on this head: full |
1 participant
Summary
CompressionBlock.childBlockRefsthe single lineage authority and require exact-once placeholder expansion.Why
Nested compression previously retained references/placeholders without guaranteeing that the parent summary contained the full child content. That made a later compression block depend on earlier block state instead of being a self-contained compressed representation.
Impact
Parent compression blocks now preserve complete nested context. Compression summaries carrying child refs, unresolved
(bN)placeholders, duplicate lineage, or invalid nesting are rejected.This is an intentional breaking refactor. Old compression state is not migrated and no fallback compatibility path is retained.
Validation
bun run test— 8/8 Turborepo tasks passedbun run buildgit diff --check origin/main...HEADSummary by CodeRabbit