Repository navigation
fix(compaction): context compaction full usability - #1647
Conversation
… page, open gate Four cascading issues fixed plus observability: 1. Re-enable elision when a compaction summary exists (context-assembly.ts) — the critical prod bug: compactionPointer=0 was disabling elideStaleToolOutputs on the tail, leaving 252k tokens of verbatim tool results even after a summary was stored. Fix: always compute chunk-aligned boundary; summary covers HEAD, tail still needs elision. 2. Sanitize NaN token counts before DB write (ai-monitoring.ts) — 400 error responses fire onFinish with no usage metadata; NaN in a Postgres integer column crashes with 'invalid input syntax for type integer: "NaN"'. 3. Add within-message tool result size cap — capToolResultSize(8000) applied after hard truncation in buildModelContext. Handles the minTailMessages=6 floor edge case where 6 messages each contain 40k-token read_page results and whole-message cuts can't help. 4. Widen compaction gate from admin-only to all authenticated users. 5. Admin observability: GET /api/admin/compaction endpoint + /admin/compaction page showing recent compaction state, summary token counts, and 24h usage log entries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 23 minutes and 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR rolls out context compaction to all authenticated users (previously admin-only), fixes the stale-tool-output elision boundary when a compaction summary already exists, adds a ChangesCompaction Engine Improvements
Admin Compaction Monitoring Dashboard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58e60f8dc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (part.type !== 'tool-result') return part; | ||
| if (typeof part.result !== 'string' || part.result.length <= maxChars) return part; |
There was a problem hiding this comment.
In production, prepareHistoryForModel passes persisted UIMessage parts through this function as type: 'tool-read_page' with an output field, while this predicate only recognizes canonical tool-result/result parts. Consequently, the oversized outputs that motivated this safety net remain untouched and can still push the retained minimum tail over the provider context limit. Support both dialects here, as elideStaleToolOutputs already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a6a40f1 — isToolOutputPartForCap now handles both canonical type:'tool-result'/result and SDK UIMessage type:'tool-{name}'/output formats. Pattern mirrors isToolOutputPart/getOutputValue in tool-result-eliding.ts. Two new test cases cover SDK-format truncation and text-part passthrough. Added output?: unknown to CompactionMessagePart so the field is typed.
| const fetchData = useCallback(async () => { | ||
| try { | ||
| setLoading(true); | ||
| const response = await fetchWithAuth("/api/admin/compaction"); |
There was a problem hiding this comment.
This is a same-origin request from the separately deployed apps/admin application, but the new route exists only under apps/web and the admin Next config has no rewrite for it. The compaction page therefore receives a 404 on every fetch instead of displaying data; add an admin API route/proxy like the existing global-prompt proxy, or explicitly request the web-app origin with the required service authentication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1142db7 — added apps/admin/src/app/api/admin/compaction/route.ts so the admin app serves its own handler at the same relative URL. Mirrors the apps/admin/src/app/api/admin/ai-billing/route.ts pattern.
| .from(aiUsageLogs) | ||
| .where(eq(aiUsageLogs.source, 'compaction')) | ||
| .orderBy(desc(aiUsageLogs.timestamp)) | ||
| .limit(50), |
There was a problem hiding this comment.
The query is described and displayed as covering the last 24 hours, but it only filters by source and never uses since24h. When older compaction records exist, the run count, total cost, and table instead represent the latest 50 records across all time, and can show activity even when no compaction occurred in the preceding day.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb39b09 — gte(aiUsageLogs.timestamp, since24h) added to the compaction logs query so it genuinely covers only the last 24h window.
| db | ||
| .select({ count: sql<number>`count(*)` }) | ||
| .from(conversationCompactions) | ||
| .where(gte(conversationCompactions.lastCompactedAt, since7d)), |
There was a problem hiding this comment.
conversation_compactions has one row per (conversationId, source) and subsequent compactions update that row while incrementing summaryVersion. Counting rows whose latest update occurred within seven days therefore reports one compaction for a conversation compacted multiple times during that period, making the dashboard's “Compactions (7d)” metric substantially undercount active conversations; derive run counts from timestamp-filtered usage events instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1142db7 — the 7d metric now queries ai_usage_logs with source='compaction' (actual LLM runs) instead of conversation_compactions state rows. Card label updated to 'Compaction runs (7d)' to make the semantics clear.
- context-assembly.test.ts: flip 'suppresses elision when summary exists' to assert the CORRECT behavior — elision now runs on the tail even when a summary is present (the old test was validating the bug we just fixed) - compaction-gating.test.ts: update to match widened gate (all authenticated users, not admin-only) - context-window.test.ts: add capToolResultSize unit tests (pure function, 6 cases) and an integration test via buildModelContext showing the cap fires on hard-truncation floor edge-case - ai-monitoring.test.ts: add NaN sanitization test — trackAIUsage with NaN token fields must write undefined to the DB, not crash with Postgres integer error Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
since24h was defined but never used in the aiUsageLogs query — the endpoint was returning up to 50 compaction runs regardless of age. Add `and(..., gte(timestamp, since24h))` so `recentLogs` and `compactionLogsSince24h` in the summary are genuinely time-windowed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@apps/admin/src/app/`(admin)/compaction/page.tsx:
- Around line 38-45: The `conversationId` property in the `CompactionLogRow`
interface is currently typed as `string`, but according to the API payload
contract it can be null. Update the `conversationId` property in the
`CompactionLogRow` interface to be `conversationId: string | null;` to properly
reflect the nullable type from the API and maintain strict TypeScript typing.
- Around line 54-59: The usd function is configured with minimumFractionDigits:
3, which causes USD values to display with 3 decimal places (e.g., $0.120 for 12
cents) instead of the standard 2 decimal places. Change the
minimumFractionDigits property in the usd function's NumberFormat configuration
from 3 to 2 to display currency values correctly with 2 decimal digits.
In `@apps/web/src/app/api/admin/compaction/route.ts`:
- Around line 5-6: The compaction logs query is not applying the calculated
`since24h` filter when fetching `compactionLogsSince24h` and calculating
`totalCompactionCostCents`. Update the query for `compactionLogsSince24h` to
include a filter condition using the `gte` operator (already imported) to ensure
logs are filtered where the timestamp is greater than or equal to the `since24h`
value, rather than just limiting to the latest 50 rows without a time
constraint. This same 24-hour filter should be consistently applied wherever
`compactionLogsSince24h` is queried across the compaction route handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59617d14-7642-4354-acc4-9b3ec115a103
📒 Files selected for processing (7)
apps/admin/src/app/(admin)/compaction/page.tsxapps/admin/src/app/AdminLayoutClient.tsxapps/web/src/app/api/admin/compaction/route.tsapps/web/src/lib/ai/core/compaction/compaction-gating.tsapps/web/src/lib/ai/core/context-assembly.tspackages/lib/src/ai/context-window.tspackages/lib/src/monitoring/ai-monitoring.ts
- conversationId in CompactionLogRow type is nullable (matches API payload) - usd() formatter: minimumFractionDigits 3→2, add maximumFractionDigits:4 (compaction costs are sub-cent so 4 decimal places shows real values; standard 2-digit minimum keeps whole-cent amounts clean) - 24h time filter on compaction logs already addressed in previous commit Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-{name}/output)
Messages flowing through buildModelContext are SDK-format UIMessages cast as
CompactionMessage[], so parts have type:'tool-read_page' and an `output` field
rather than the canonical type:'tool-result' / `result` field. The cap was a
no-op for all real production messages.
Mirror the dual-format isToolOutputPart/getOutputValue pattern from elision:
- isToolOutputPartForCap: matches both canonical and SDK-dialect parts
- capToolResultSize: reads from `result` (canonical) or `output` (SDK),
writes capped value back to the same field
- CompactionMessagePart gains `output?: unknown` to match ElisionMessagePart
Tests: two new cases cover SDK-format truncation and text-part passthrough.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… metric
- Add apps/admin/src/app/api/admin/compaction/route.ts — the admin app is
deployed separately from apps/web, so fetchWithAuth('/api/admin/compaction')
on the admin app's pages was 404ing (no same-origin handler). Pattern
follows apps/admin/src/app/api/admin/ai-billing/route.ts.
- Fix totalCompactions7d metric: was counting rows in conversation_compactions
updated in the last 7d (1 per conversation regardless of how many times it
was compacted). Now counts ai_usage_logs rows with source='compaction' in
the 7d window — i.e. actual LLM compaction runs, not mutable state rows.
Applied to both the web-app and admin-app routes. Card label updated to
"Compaction runs (7d)" to match.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The gate change (canUseCompaction: admin-only → all authenticated users) caused three test files to break: - prepare-context.test.ts: the 'admin gate' test expected role:'user' to get exact passthrough (same reference, no DB reads). Now that the gate is open, getState IS called and a new tail-array is returned. Updated assertions to match (toEqual instead of toBe, expect getState called). Renamed describe block to 'authenticated gate'. - credit-gate.test.ts / stream-socket-events.test.ts: these route tests mock @pagespace/lib/monitoring/ai-context-calculator without providing estimateTokens/estimateMessageTokens/getContextWindowSize. Before the gate change those functions were never reached for role:'user'. Now they are, causing the route to throw before createStreamLifecycle/trackUsage could be called. Fix: mock @/lib/ai/core/compaction/prepare-context as a simple passthrough in both files — these tests cover credit gating and stream lifecycle, not compaction internals. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Test fix (commit b170bac): The gate widening to all authenticated users caused 17 test failures across three files:
Fix: Added |
…ide pagination metadata When capToolResultSize slices a large read_page response at 8000 chars, totalLines/totalMessages was getting cut off because it came after the content block. Agents could see they were truncated but not the full document size. Moving the metadata fields first means they're always visible regardless of content length. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@apps/admin/src/app/api/admin/compaction/route.ts`:
- Around line 15-16: The query ordering by
desc(conversationCompactions.lastCompactedAt) followed by limit(20) will include
conversations with NULL lastCompactedAt values ahead of recently compacted ones,
violating the contract of selecting the 20 most recently compacted
conversations. Add a WHERE clause filter before the orderBy to exclude rows
where lastCompactedAt is NULL, ensuring only conversations with actual
compaction timestamps are considered for the final result set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f57425a-ca21-4772-b254-b83e8e556f03
📒 Files selected for processing (12)
apps/admin/src/app/(admin)/compaction/page.tsxapps/admin/src/app/api/admin/compaction/route.tsapps/web/src/app/api/admin/compaction/route.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.tsapps/web/src/lib/ai/core/__tests__/context-assembly.test.tsapps/web/src/lib/ai/core/compaction/__tests__/compaction-gating.test.tsapps/web/src/lib/ai/core/compaction/__tests__/prepare-context.test.tsapps/web/src/lib/ai/tools/page-read-tools.tspackages/lib/src/ai/__tests__/context-window.test.tspackages/lib/src/ai/context-window.tspackages/lib/src/monitoring/__tests__/ai-monitoring.test.ts
✅ Files skipped from review due to trivial changes (2)
- apps/web/src/app/api/ai/global/[id]/messages/tests/credit-gate.test.ts
- apps/web/src/lib/ai/tools/page-read-tools.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/lib/src/ai/context-window.ts
- apps/web/src/app/api/admin/compaction/route.ts
- apps/admin/src/app/(admin)/compaction/page.tsx
| .orderBy(desc(conversationCompactions.lastCompactedAt)) | ||
| .limit(20), |
There was a problem hiding this comment.
Exclude NULL lastCompactedAt rows from “most recent” selection.
Line 15 orders by lastCompactedAt descending, but this column is nullable; NULL rows can be returned ahead of actual recent compactions, which breaks the “20 most recently compacted conversations” contract.
Suggested fix
db
.select()
.from(conversationCompactions)
+ .where(sql`${conversationCompactions.lastCompactedAt} is not null`)
.orderBy(desc(conversationCompactions.lastCompactedAt))
.limit(20),🤖 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 `@apps/admin/src/app/api/admin/compaction/route.ts` around lines 15 - 16, The
query ordering by desc(conversationCompactions.lastCompactedAt) followed by
limit(20) will include conversations with NULL lastCompactedAt values ahead of
recently compacted ones, violating the contract of selecting the 20 most
recently compacted conversations. Add a WHERE clause filter before the orderBy
to exclude rows where lastCompactedAt is NULL, ensuring only conversations with
actual compaction timestamps are considered for the final result set.
…r "recently compacted" contract NULL rows floated to the top under DESC ordering, masking actual recent compactions. Rows with no lastCompactedAt have never been compacted, so they don't belong in this set at all. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Re-enable elision when summary exists (`context-assembly.ts`) — the confirmed production root cause. `compactionPointer = 0` was passed to `computeElisionBoundary` whenever `hasSummary = true`, making `elideStaleToolOutputs` a no-op on the tail. Result: 252k tokens of verbatim tool results in every request even after a 1.6k-token compaction summary was stored. Fix: always compute chunk-aligned boundary from the tail's assistant turn count; the summary covers the HEAD, the tail still needs elision.
Sanitize NaN token counts (`ai-monitoring.ts`) — 400 error responses fire `onFinish` with no usage metadata, producing `NaN` for all three token fields. Passing `NaN` into a Postgres `integer` column crashes `writeAiUsage` with `invalid input syntax for type integer: "NaN"`. Fix: `Number.isFinite()` guard before the DB write.
Within-message tool result size cap (`context-window.ts`) — new `capToolResultSize(8000)` applied after hard truncation in `buildModelContext`. The `minTailMessages = 6` floor means hard truncation is a no-op when those 6 messages each contain 40k-token `read_page` results. This trims within remaining messages as a last-resort safety net. Handles both canonical (`type: 'tool-result'`) and SDK UIMessage (`type: 'tool-{name}'`) formats.
Widen compaction gate (`compaction-gating.ts`) — was `admin-only`. Now enabled for all authenticated users (`user != null`).
Admin observability — new `GET /api/admin/compaction` endpoint (compaction state + 24h usage logs, served from both `apps/web` and `apps/admin`) + `/compaction` tab in the admin app showing summary tokens, compaction age, per-run cost.
Test alignment — updated `prepare-context.test.ts` to reflect the new gate behavior; added `prepare-context` passthrough mock to `credit-gate.test.ts` and `stream-socket-events.test.ts` so those route tests remain focused on their intended scope (credit gating / stream lifecycle, not compaction internals).
Root cause confirmation (production DB)
Queried the Fly prod DB directly. Conversation `n2z2v2qoqv274om0hpuzz6rx` had a stored compaction summary (1,608 tokens) but subsequent requests still showed 253,645 input tokens — because `1,608 (summary) + 252,000 (unelided tail) = 253,608`. Compaction was firing and storing; elision was just disabled on the resulting tail.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes