fix(compaction): stabilize deletion planner

[lavaman131]

Summary

Fixes `/compact` and auto-compaction regressions (#1310): removes the native `better-sqlite3` dependency from the transcript deletion store by replacing it with a pure TypeScript in-memory implementation, ensures the session's configured reasoning level is honored (not silently dropped) during context compaction, and removes an artificial 4096-token output cap on the compaction model.

Changes

• Replaced `ContextDeletionSqliteStore` with `ContextDeletionMemoryStore` — the SQLite-backed store required `better-sqlite3` / `bun:sqlite`, which couldn't be reliably loaded across runtimes (Node vs. Bun). The new in-memory store is a drop-in replacement with identical transactional rollback semantics: mutations within a `transaction()` call are rolled back on error via snapshot/restore.
• Removed `better-sqlite3` and `@types/better-sqlite3` from `packages/coding-agent` and refreshed `bun.lock`, eliminating all prebuilt native `.node` binary transitive dependencies.
• Fixed thinking level pass-through for compaction — `AgentSession` was passing `undefined` to `contextCompact`, causing the planner to silently fall back to the lowest supported reasoning level. It now passes `this.thinkingLevel` so the user's selected level (e.g. `"high"`) is honored.
• Removed `getLowestContextCompactionThinkingLevel` — the helper that always downgraded thinking to the minimum supported level is no longer needed now that the caller's level is passed through directly.
• Removed artificial 4096-token output cap — `runContextDeletionAssistant` was capping `maxTokens` at 4096 regardless of the model's actual limit. It now uses the model's configured output cap directly, allowing the compaction planner to produce longer responses when the model supports it.
• Updated tests — the auto-compaction queue test asserts the thinking level argument is forwarded to `contextCompact`; deletion-tool tests updated to assert the selected level passes through rather than being downgraded.
• Changelog — entry added under `[Unreleased] → Fixed` in `packages/coding-agent/CHANGELOG.md`.

Notes

• No API or behavior changes for callers — `ContextDeletionMemoryStore` is a drop-in for `ContextDeletionSqliteStore`; the rollback contract is identical.
• Thinking level behavior is now correct: compaction uses the caller-supplied level rather than always downgrading to the minimum. Callers passing `undefined` still default to `"off"`.

[claude]

Review: fix(compaction): stabilize deletion planner

Thanks for this — replacing the native better-sqlite3 dependency with an in-memory store is a solid simplification that removes a real source of install/runtime fragility (native dlopen, prebuild-install, the bun:sqlite vs better-sqlite3 branch). The diff is clean: no dangling references to the removed SqliteAdapter/SqliteRow/createRequire machinery remain, better-sqlite3 was only used here (web-access uses the node:sqlite builtin), and the removed getLowestContextCompactionThinkingLevel export has no other importers. Good that you kept the row-shaped return types so the call sites did not have to change.

A few things worth a look before merge.

1. Thinking level vs. the hard-coded maxTokens = 4096 cap (worth verifying). runContextDeletionAssistant caps output at Math.min(4096, model.maxTokens ...). Previously the planner always ran at the lowest supported level, so essentially the whole 4096 budget went to tool calls. Now it runs at the user-selected level, which can be high/xhigh. On reasoning models the thinking budget counts against output tokens, so a high level plus a 4096 cap could starve the deletion plan (or, if the provider requires maxTokens > thinkingBudget, error outright) — exactly during the critical-overflow path where compaction most needs to succeed. Have you exercised this against a real high/xhigh reasoning model end-to-end (not just the faux provider)? If the cap and a high thinking budget can collide, consider clamping the effective compaction level (e.g. cap at medium) or raising maxTokens when thinking is high. This is the change most likely to reintroduce a regression, so I would want it confirmed rather than assumed.

2. Default thinkingLevel = "off" removes the model-supported guarantee. The deleted getLowestContextCompactionThinkingLevel guaranteed a level the model actually supports. The new default param is a bare "off". The AgentSession path is safe because this.thinkingLevel is already clamped to a supported level — but any direct caller (or future one) hitting the default "off" on a model that does not support off now relies entirely on the provider clamping it. Worth confirming the provider clamps silently rather than rejecting; otherwise a small getSupportedThinkingLevels clamp inside contextCompact would restore the old safety net cheaply.

3. Minor performance nits in ContextDeletionMemoryStore. getContentBlockForRead does a linear this.contentBlocks.find(...) on every read tool call. You already track blockKeys/contentBlockCountByEntryId during construction — a Map keyed by entryId:blockIndex would make this O(1) and mirror the old SQLite primary key. Also, listContentBlocksForGrep builds a copy and .sort()s on every call, but contentBlocks is already in (entryPosition, blockIndex) order from the flatMap construction — the sort is redundant each invocation. Neither is a correctness issue; transcripts are bounded, so low priority.

4. Rollback semantics — correct, but untested. The snapshot/restore approach correctly narrows rollback to the only mutable state (deletionTargets, callCount, lastError) since the transcript tables are now immutable, and it even tolerates nesting (which BEGIN IMMEDIATE would have rejected). However, the PR advertises "keeps rollback semantics" and there is no direct unit test for it — in practice the tool callbacks catch their own errors inside the transaction, so the rollback branch is rarely hit. Consider adding a test that throws inside store.transaction(...) and asserts the mutable state is restored, plus tests for the new constructor guards (Duplicate transcript entry id, does not belong to entry). These are cheap and lock in the behavior the refactor depends on.

Tests / changelog. Coverage for the behavioral change is good: both the AgentSession auto-compaction path (calls[0]?.[5] === "high") and the deletion-tool path assert the selected level is preserved, and the renamed tests read clearly. Changelog entry is correctly placed under [Unreleased] / Fixed with the issue link. The bun.lock version-field churn (0.8.26-alpha.11 to 0.8.26) looks like a benign re-sync against the already-bumped package.json; just confirm it was produced by bun install and not hand-edited.

Overall a good, well-scoped fix. My only real blocker-candidate is item 1 — please confirm the high/xhigh plus 4096-token interaction on a live reasoning model, since that is the scenario the original lowest-level behavior was protecting against.

— Reviewed with Claude Code

[claude]

Review: fix(compaction): stabilize deletion planner

Solid, well-scoped fix. Dropping the native better-sqlite3 dependency in favor of a plain in-memory store is the right call — it removes a runtime-fragile native .node binary from the dependency graph and is what was breaking /compact and auto-compaction across runtimes. The rollback parity (snapshot/restore) and the thinking-level pass-through both look correct. A few things worth confirming before merge.

Correctness / parity — looks good

• Transactional rollback is faithfully preserved: only the mutable fields (deletionTargets, callCount, lastError) are captured in snapshot() and reset in restore(); entries/contentBlocks are immutable after construction, so they don't need snapshotting.
• The constructor's duplicate-id / mismatched-block validation reproduces the old UNIQUE / FOREIGN KEY constraints with clearer error messages.
• The auto-compaction test asserts mock.calls[0]?.[5] — index 5 correctly maps to the thinkingLevel positional arg in contextCompact(preparation, model, apiKey, headers, signal, thinkingLevel, mode).

Things to confirm

1. Behavior change, not just a bug fix: compaction now uses the session's reasoning level.
Removing getLowestContextCompactionThinkingLevel changes compaction from always using the lowest supported level to using whatever the session selected. The PR frames this purely as "the level was silently dropped," but it's also a cost/latency change: a user running high/xhigh will now spend that reasoning budget on every compaction pass, which is a mechanical task that previously ran cheap. Is the cost trade-off intended, or should compaction still cap at a low level (e.g. min(selected, low))?

2. Possible regression in the standalone default thinkingLevel = "off".
The removed helper guaranteed a model-supported level — it returned "minimal" when a reasoning model didn't support "off" (there was a dedicated test for exactly this, now changed). The new default of "off" drops that safety net. In the AgentSession path this is fine because this.thinkingLevel is kept valid for the current model by _getThinkingLevelForModelSwitch. But a direct contextCompact(...) caller that omits the level now sends "off" to models that may not support it. Worth confirming the pi-ai Agent layer normalizes an unsupported "off" rather than erroring — otherwise the old guard was load-bearing.

3. Version vs. CHANGELOG.
package.json is at 0.8.26, which already has a released, dated section (## [0.8.26] - 2026-06-08). The new fix sits under [Unreleased] with no version to ship under, and bun.lock moved 0.8.26-alpha.11 -> 0.8.26. Per the repo's release flow this fix needs a bump (e.g. 0.8.27 or a 0.8.26-alpha.12 prerelease) before it can publish — otherwise it's stranded under [Unreleased]. Please confirm the versioning is intentional.

Performance (minor)

4. getContentBlockForRead is O(n) per call.

const block = this.contentBlocks.find((candidate) => candidate.entryId === entryId && candidate.blockIndex === blockIndex);

SQLite had a PRIMARY KEY (entry_id, block_index) index (O(log n)); the read tool can be invoked many times during a deletion run, so on a large transcript this becomes O(n·reads). You already iterate all blocks once to build contentBlockCountByEntryId — cheap to also populate a Map keyed by ${entryId}:${blockIndex} (mirroring entriesById) and look up in O(1).

5. listContentBlocksForGrep re-sorts a fresh copy on every call.
contentBlocks is already built in (entryPosition, blockIndex) order (flatMap over ordered entries, blocks in array order), so the per-call .sort(...) is likely redundant — or precompute the sorted array once in the constructor.

Nits

6. Vestigial SQL-shaped row types. The in-memory store still returns SQL-flavored rows (is_protected: 1|0, snake_case entry_id). Fine for a minimal diff, but a follow-up could convert the consumers to real booleans/camelCase and drop the EntryTextRow/ContentBlockReadRow shapes entirely.

7. Removed export. getLowestContextCompactionThinkingLevel was exported and is now gone — technically a public-surface removal. No internal callers remain (confirmed), so almost certainly fine; just noting it.

Tests

Coverage is appropriate: the new auto-compaction test pins the forwarded thinking level, and the two deletion-tool tests were updated to reflect pass-through semantics. If you keep the standalone "off" default (point 2), consider re-adding a test that a reasoning-only model doesn't choke on an unsupported "off" — that's the scenario the deleted test used to guard.

Overall: good fix that removes a real source of fragility. Main asks are confirming the reasoning-level cost trade-off (1), the unsupported-"off" safety (2), and the version bump (3).

_{🤖 Generated with Claude Code}

[claude]

Code Review — fix(compaction): stabilize deletion planner (#1311)

Thanks for this — dropping the native better-sqlite3 dependency in favor of a pure-TS in-memory store is a solid portability win (no more .node prebuilds, no Node-vs-Bun dlopen divergence), and the thinking-level fix is a real correctness improvement. The ContextDeletionMemoryStore reads cleanly as a drop-in. A few things worth a closer look before merge.

🔴 Undocumented behavior change: the 4096 maxTokens cap was removed

// before
const maxTokens = Math.min(4096, model.maxTokens > 0 ? model.maxTokens : Number.POSITIVE_INFINITY);
// after
const maxTokens = model.maxTokens > 0 ? model.maxTokens : Number.POSITIVE_INFINITY;

This isn't mentioned in the PR summary or the changelog, but it's a meaningful change: compaction agent responses are no longer bounded at 4096 output tokens and can now use the model's full output budget (often 8k–64k+). I suspect this is intentional and necessary — once you honor a "high"/"xhigh" thinking level, the thinking tokens count against maxTokens, so a 4096 cap would truncate the thinking budget plus the deletion tool call. If so, great, but:

• Please confirm it was deliberate, and consider a bounded ceiling (e.g. Math.min(reasonableCap, model.maxTokens)) rather than uncapping entirely. The deletion planner only emits tool calls + thinking, so an unbounded output budget mostly translates into cost/latency exposure on a mechanical task.
• Worth a line in the changelog since it affects token spend.

🟡 Design question: honoring the full session thinking level for compaction

The old getLowestContextCompactionThinkingLevel deliberately downgraded compaction to the lowest supported level — that reads like an intentional cost optimization for a mechanical task, not a bug. This PR now passes the user's full level (this.thinkingLevel) straight through. Combined with the maxTokens change above, every compaction now burns the user's full reasoning budget at full output width.

If #1310 explicitly wants the selected level respected, this is correct — just flagging that the two changes compound on cost. (Also worth a sanity check: since compaction reuses this.model, the level is guaranteed supported, so dropping the getSupportedThinkingLevels validation is safe here — but it does remove the only guard if a caller ever passes a level the model doesn't support.)

🟡 Performance: O(n) lookups in the new store

The transcript being compacted is by definition large, and these run inside the deletion loop:

• getContentBlockForRead does a linear this.contentBlocks.find(...) per read (context-compaction.ts:1345, called per target at :1723). For a model that reads many targets, that's O(reads × blocks).
• listContentBlocksForGrep re-sorts a fresh copy on every call (:1320, called at :1517 and :1654), though block order never changes after construction.

Suggestions, both cheap:

• Add a contentBlocksByKey: Map<string, StoredContentBlock> keyed by \${entryId}:${blockIndex}`in the constructor (symmetric with the existingentriesById) and use it in getContentBlockForRead`.
• Sort contentBlocks once in the constructor so listContentBlocksForGrep can skip the per-call .sort().

The SQLite version had these as indexed lookups, so this is the one spot where the rewrite is a latency regression rather than a wash.

🟡 Test coverage gaps for the new store

The thinking-level pass-through test (agent-session-auto-compaction-queue.test.ts) and the updated deletion-tool assertions are good. But the rewritten store added new behavior that isn't directly exercised:

• The constructor's new validation throws — duplicate entryId, duplicate entryId:blockIndex, and the block/entry mismatch guard.
• The snapshot() / restore() rollback path in transaction() (the core invariant the SQLite ROLLBACK previously guaranteed). A focused unit test that mutates targets inside a transaction() that throws, then asserts readTargets()/getCallCount() are unchanged, would lock in the contract.

🟢 Looks good

• Snapshot/restore correctness: only deletionTargets, callCount, and lastError are mutable; entries/contentBlocks are never mutated post-construction, so snapshotting just those three is sound, and copyDeletionTarget gives the deep copy needed to avoid aliasing.
• No dangling references to the removed symbols (getLowestContextCompactionThinkingLevel, ContextDeletionSqliteStore, SqliteAdapter, store.close()), so typecheck stays green.
• Changelog entry is correctly placed under ## [Unreleased] → ### Fixed with issue attribution, per CLAUDE.md.

Nit

• getGrepScanTextLength now uses JS String.length (UTF-16 code units) where SQLite LENGTH(text) counted Unicode characters — a tiny discrepancy for non-ASCII text, but it only feeds a heuristic scan-size, so negligible.

Overall this is a good fix; the main thing I'd want resolved before merge is confirming the maxTokens uncap is intentional (and ideally bounded + noted in the changelog), plus the cheap content-block Map for the read path.