feat(compaction): agent-loop planner with transcript-bound deletion tools

[lavaman131]

Summary

Replaces the context compaction planner's single completeSimple call with a multi-turn Agent loop. Deletion decisions are now bound to four executable transcript tools that validate and accumulate targets incrementally, eliminating final-response parsing as a failure mode and enabling multiple independently guarded deletion calls per turn.

Closes #1300.

Changes

New tools and controller

• context_delete (reworked from context_deletion_plan): transcript-bound tool that validates and accumulates deletion targets incrementally; routes through the same validation pipeline as grep-delete.
• context_grep_delete (new): bulk-deletes transcript entries or content blocks matching a literal string or regex, with embedded guardrails — protected-entry/block skipping, maxMatches safety cap, and optional expectedMatchCount assertion.
• context_search_transcript (new): read-only search over the full transcript working copy, returning match snippets without mutating deletion state; lets the planner find candidate entry IDs before committing deletions.
• context_read_entry (new): reads a small slice of one transcript entry or content block by ID; keeps per-read cost bounded via maxChars and offset pagination.
• createContextDeletionTool (new): factory that creates all four tools plus a controller (getDeletionRequest, getValidatedResult, getLastError, getCallCount) backed by a shared SQLite working copy.
• CONTEXT_COMPACTION_MAX_TURNS = 50: hard turn cap on the agent loop.

Planning loop

• Switches from completeSimple to Agent + streamSimple with AbortSignal correctly propagated via event listener.
• Planner is now required to call at least one tool; throws if callCount === 0.
• System prompt and fixed prompt updated to describe all four tools and the multi-call workflow.
• Transcript is written to a temporary JSONL file and passed as a file path in the prompt (manifest provides a bounded preview of up to 80 entries); the file is cleaned up after the agent loop completes.
• Thinking level is resolved to the lowest supported level for the model (getLowestContextCompactionThinkingLevel).

Validation hardening

• reconcileToolDependencies (new): bounded fixpoint repair pass that auto-reconciles tool-call/tool-result pairing violations before the strict validateToolDependencies assertion — handles promoted entry deletions, orphaned results, and protected-entry constraints.
• mergeContextDeletionTargets (new): merges incremental target lists while keeping entry-level deletions canonical over per-block targets.
• validateContextDeletionPlan now runs reconcileToolDependencies before overlap and dependency checks, turning previously thrown errors into repaired outputs.
• contextCompact uses the pre-validated plan from tool execution (skips redundant post-hoc validateContextDeletionPlan call).

Planner state persistence

• ContextDeletionSqliteStore: transient SQLite working copy holding transcript entries, content blocks, accumulated deletion targets, call count, and last error — enables parallel-safe tool execution and efficient grep/search scans.

Transcript manifest

• contextCompactionTranscriptManifest: generates a bounded manifest (protected entries + highest-token unprotected entries, up to 80) as a structured preview in the prompt, with the full JSONL file path for deeper reads.
• transcriptEntryFilePayload: serializes full entry text and content blocks to JSONL for the transcript file.

Tests

• Rewrites the planner tool integration test (renamed context-compaction-deletion-tool.test.ts) to exercise multi-tool-call turns and verify tool-result pairing in continuation context.
• Adds unit tests for the terminate: false continuation on both tools, grep guardrail behavior (protected skip, maxMatches, match reporting), and the context_grep_delete path through the planner agent.
• Adds six new validateContextDeletionPlan tests covering tool-call/result reconciliation, block-index promotion, combined sibling deletions, and multi-tool assistant entry promotion.

Migration notes

No breaking changes to the public contextCompact / planContextDeletions API surface. Internal parseContextDeletionPlanResponse is renamed to parseContextDeletionResponse; RawContextDeletionPlan is renamed to ContextDeletionRequest; ValidatedContextDeletionPlan is renamed to ValidatedContextDeletionResult. Existing callers of validateContextDeletionPlan receive reconciled rather than raw targets in the returned deletedTargets.

Validation

bun test packages/coding-agent/test/context-compaction-deletion-tool.test.ts
bun test packages/coding-agent/test/context-compaction.test.ts packages/coding-agent/test/context-compaction-deletion-tool.test.ts
bun test test/unit/persistence-compaction-policy.test.ts packages/coding-agent/test/interactive-mode-compaction.test.ts packages/coding-agent/test/context-compaction.test.ts packages/coding-agent/test/context-compaction-deletion-tool.test.ts
bun run typecheck

[claude]

Review: feat(compaction): bind deletion planning to transcript tools

Nice refactor — moving from "parse the final transcript text" to transcript-bound executable tools that validate each deletion as it's recorded is a real robustness win, and the new tool-call/result reconciliation cases (reconcileToolDependencies, the content-block promotion tests) are well thought through. A few things worth a look before merge.

Potential bugs / correctness

1. Abort can still apply a partial plan. In planContextDeletions (context-compaction.ts:1191-1208), after await agent.prompt(...) the only failure check is agent.state.errorMessage. On cancellation the listener calls agent.abort(), but if abort resolves the prompt without setting errorMessage, the function falls through and getCallCount() > 0 returns whatever partial set was recorded — which contextCompact will then apply. The previous completeSimple(..., { signal }) path surfaced an error stop reason instead. Recommend re-checking signal?.aborted after the prompt and throwing, so a user-cancelled compaction never deletes anything:

```ts
if (signal?.aborted) throw new Error("Context compaction planning failed: Request was aborted");
```

2. Tool execute() throwing on validation — confirm it becomes a tool error, not a run abort. Both tools validate the merged target set (applyValidatedTargets → validateContextDeletionPlan), which throws on conditions like "would remove all context entries" / "would leave no user task". Across accumulating calls (or a broad context_grep_delete), a merged set can trip those even when each increment looked fine. This design only works if the Agent converts execute() exceptions into error tool-results the model can react to. If the framework instead propagates the throw, an over-aggressive plan aborts the whole compaction — a regression from the old single-shot "validate then fail gracefully" behavior. Please confirm; if it propagates, wrap each execute body and return a terminate: false error result instead.

Security

3. ReDoS via model-supplied regex. createGrepMatcher (context-compaction.ts:908-914) compiles new RegExp(pattern, ...) when regex: true and runs matcher.test() across every entry/block of a potentially large transcript. A pathological pattern (catastrophic backtracking) can hang the compaction step indefinitely — and try/catch won't help with a hang, only with the separate problem of an invalid pattern throwing synchronously (also currently uncaught). Consider guarding transcript size, a match timeout, or validating/restricting the pattern; at minimum wrap new RegExp so an invalid pattern returns a graceful tool error rather than bubbling up.

Performance / cost

4. No turn cap on the planner agent. This replaced a single completeSimple call with a full agent loop (new Agent(...).prompt(...)) with no visible max-steps bound. The model can call context_deletion_plan / context_grep_delete an unbounded number of times, each up to maxTokens, for what is supposed to be a cheap maintenance pass. Worth an explicit iteration/step cap so a misbehaving planner can't run up latency and token cost.

5. Redundant double validation. contextCompact calls validateContextDeletionPlan(plan, ...) again on the plan returned by planContextDeletions, but that plan is already the fully reconciled/validated deletedTargets (re-running reconcileToolDependencies on an already-reconciled set). The controller already exposes getValidatedPlan() — consider plumbing that through to avoid recomputing.

Code quality (minor)

• getValidatedPlan() is exposed on the controller but unused by any caller — either use it (see Flora131/feat/refine readme #5) or drop it.
• reconcileToolDependencies fixpoint loop is bounded by entries.length * 2 passes with a changed flag. If it ever fails to converge within the bound it silently returns a possibly non-reconciled set (which would then fail validateToolDependencies). A debug log/assertion when passes are exhausted while changed is still true would surface non-convergence rather than hide it.
• Grep telemetry inconsistency: when a single-block content_block match is promoted to an entry candidate (context-compaction.ts:1030-1033), the already_deleted skip still records target: "content_block" + blockIndex even though the effective target is the entry. Cosmetic, but can confuse the details consumer.

Test coverage

Good additions on pairing, content-block promotion, combined call IDs, and protected skipping. Gaps given the new surface area:

• Abort / cancellation behavior (add agent instructions #1).
• Invalid regex pattern and regex: true matching (update readme and mcp servers #3).
• target: "content_block" grep path.
• maxMatches exceeded and expectedMatchCount mismatch skip reasons (no test exercises these guardrails end-to-end).
• A merged plan that trips a validateContextDeletionPlan invariant, asserting it surfaces as a graceful tool error rather than aborting the run (updates to readme and instructions #2).

Overall direction is solid; #1 (abort) and #3 (ReDoS) are the two I'd treat as blocking.

🤖 Generated with Claude Code

[claude]

PR Review — feat(compaction): replace single-shot planner with transcript-bound multi-call tool controller

Thanks for this — the move from final-response parsing to executable, immediately-validated tools is a solid architectural improvement, and the test coverage is genuinely impressive (multi-call turns, grep guardrails, regex/invalid-regex, promotion, turn cap, abort). Feedback is grouped by area; nothing here is a blocker. The highest-value items are the performance note and the ReDoS note.

Strengths

• Both tools set executionMode: "sequential" and the agent uses toolExecution: "sequential" — correct and important, since both tools mutate the shared deletedTargets list; parallel execution would race. Good that this is explicit.
• Validation is layered well: per-call validateContextDeletionPlan plus reconcileToolDependencies as a repair pass before the strict assertion. The non-terminating tool-error pattern (return terminate: false with error in details) lets the model self-correct mid-turn rather than aborting the whole compaction.
• Guardrails on context_grep_delete (protected skip, maxMatches, expectedMatchCount, regex pattern/scan caps) are thoughtful and well-tested.

Performance

• reconcileToolDependencies is on the hot path and is roughly O(N^3) worst case. It runs up to transcript.entries.length * 2 passes; each pass iterates every callEntry, and inside that loop it recomputes getDeletedEntryIds(targets) and getDeletedContentBlocks(targets) (each O(targets)) twice per call id. Since validateContextDeletionPlan runs on every tool call — and twice per context_deletion_plan call (once for incomingPlan, once for the merged set) — this can get expensive on large transcripts. The changed flag usually short-circuits after a pass or two, so typical cost is fine, but consider hoisting the deleted-id/deleted-block computation to once per pass rather than per-call-id, and documenting the expected bound.

Potential issues / correctness

• ReDoS surface in context_grep_delete. assertSafeRegexScan caps scanned characters at 250k, but a pathological model-supplied pattern (e.g. (a+)+$) within the 512-char limit can still cause catastrophic backtracking on a much smaller input with no timeout. Input is model-generated in a controlled loop so risk is low, but worth noting the limitation, or guarding with a linear-time matcher / time budget.
• console.warn for non-convergence (~line 725) is the only direct console.* in the compaction module. On a hot path a non-converging reconcile could log repeatedly. Prefer the project logger if available, and include entry/target counts to make it actionable.
• Masked root-cause when the only tool call fails validation. If the planner only ever produced an errored tool call, validatedPlan stays undefined with callCount > 0, so contextCompact throws the generic "No safe context deletions proposed" instead of surfacing the underlying validation error already captured in details.error. Consider threading the last tool error into that final throw.
• thinkingLevel plumbing changed shape. Previously reasoning: thinkingLevel went to completeSimple; now effectiveThinkingLevel is set in Agent initialState.thinkingLevel. Assuming Agent translates that into the per-request reasoning option this is equivalent — worth confirming reasoning still reaches the provider, since no test asserts it.

Minor / nits

• planContextDeletions now spins up the full Agent loop just to return .plan (discarding validatedPlan), while contextCompact runs the loop again independently. Not wrong (they are not called together), but a short doc comment noting it re-runs the planner would prevent accidental double-planning by future callers.
• maxMatches is compared against candidates.length (post-skip), but the schema description says "If more matches are found, no deletions are applied." Matches vs. applyable-candidates is a subtle distinction — tightening the wording would avoid confusion.
• canonicalizeEntryTargets only promotes fully-deleted content blocks to entry deletions for entries in entriesWithToolCalls. The "would remove every content block" test relies on this (non-tool entries are rejected rather than promoted). Defensible, but subtle enough to deserve a code comment.

Tests
Coverage is strong. Two gaps worth considering: (1) an explicit assertion that reasoning/thinkingLevel reaches the stream when model.reasoning is true; (2) the CONTEXT_GREP_DELETE_MAX_REGEX_SCAN_CHARS / pattern-length guard branches do not appear to be exercised.

Overall a well-structured, well-tested change that meaningfully hardens the compaction planner. The performance bound on reconcileToolDependencies is the main thing I would want a second look at before merge.

Reviewed against CLAUDE.md conventions (Bun-only, raw-TS, no build step) — no violations spotted in the changed files.

[claude]

Review: agent-loop planner with transcript-bound deletion tools

Thorough, well-tested change. The move from a single parse-the-final-response completeSimple call to an Agent loop with executable, self-validating tools is a real robustness win — eliminating prose-parsing as a failure mode and letting each deletion call be guarded independently is the right direction. Abort propagation, the explicit terminate: false, errors-surfaced-as-non-terminating-tool-results, and the turn cap are all handled cleanly, and the test suite is genuinely comprehensive (grep guardrails, regex safety, content-block promotion, reconciliation scenarios). Nice work.

A few things worth considering before merge:

1. Performance of reconcileToolDependencies on large transcripts (medium)

This runs a bounded fixpoint of up to entries.length * 2 passes, and inside recordChange it rebuilds deletedEntryIds/deletedContentBlocks from the full targets array on every mutation, while the addTarget/deleteEntryTarget helpers are themselves O(targets) (targets.some(... targetKey ...)). Compounding this, applyValidatedTargets re-runs the entire validateContextDeletionPlan (which re-reconciles the whole accumulated set) on every tool call. The combined worst case is comfortably super-linear in transcript size — and compaction is exactly the moment when the transcript is at its largest. It's bounded by CONTEXT_COMPACTION_PLANNER_MAX_TURNS, so not unbounded, but it'd be worth benchmarking a realistic full-context transcript (hundreds of entries, many tool calls) to confirm it stays well under a sensible budget. Caching the deleted-id sets and mutating them incrementally instead of full rebuilds would cut the constant factor a lot.

2. Module-level mutable warnedReconciliationNonConvergence (low–medium)

This makes reconcileToolDependencies impure, fires at most once per process (so a second unrelated session that hits non-convergence is silent), and persists across tests — a latent source of order-dependent test behavior if anything ever asserts on it. Prefer routing through the package's structured logger with its own throttling rather than a global boolean + raw console.warn (the rest of the codebase generally avoids bare console.* in library paths).

3. Hitting the turn cap discards all accumulated work (design)

When plannerTurnCount > CONTEXT_COMPACTION_PLANNER_MAX_TURNS, the run throws and every validated deletion target accumulated over the prior 8 turns is dropped, failing compaction entirely. Since the tool already owns a fully-validated plan at that point, consider falling back to getValidatedPlan() (if non-empty) instead of a hard failure — a chatty-but-productive planner shouldn't waste the whole compaction. At minimum, worth a comment documenting that the throw-away is intentional.

4. Behavior change in validateContextDeletionPlan — flag for downstream callers

The function shifts from rejecting orphaned tool-call/result pairings to silently repairing and expanding the deletion set (deleting a tool call now auto-deletes its result and vice-versa). This is reasonable and the PR documents it, and the tests are updated accordingly — but it's a meaningful semantics change for any external caller that previously relied on the throw as a validation signal. The returned deletedTargets can now contain more entries than the caller requested. Make sure that expansion is acceptable everywhere validateContextDeletionPlan is consumed, not just on the compaction path.

5. ReDoS guard is best-effort (security — mostly positive, one note)

The heuristic rejection of nested quantified groups / quantified alternation / backreferences plus the 250k scan cap is good defense-in-depth, and always compiling with u + escaping literals is correct. Two notes: (a) the heuristic won't catch every catastrophic shape (e.g. long a*a*a*…$ quantifier chains), so the real backstop is genuinely the scan-size cap — worth a one-line comment saying so; (b) matcher.test() is called without the g flag, which is correct (no lastIndex statefulness) — good, just keep it that way if anyone refactors to capture matches.

6. Minor

• executionMode: "parallel" is slightly misleading since stateMutationQueue serializes all mutations anyway — correctness is preserved (each call is atomic) and a test documents it, but a short comment on the tool would save a future reader the double-take.
• expectedMatchCount is compared against candidates.length (post-filter: unprotected and not-already-deleted), not raw text matches. That's defensible, but a planner reasoning about "how many times does X appear" may expect raw-match semantics; tightening the param description would help.

None of these are blockers. (1) and (3) are the two I'd most want addressed — the first for safety at scale, the second so an over-talkative planner doesn't silently nuke a compaction's worth of valid work.

🤖 Generated with Claude Code

[claude]

Review: agent-loop planner with transcript-bound deletion tools

Thanks for this — the move from final-response parsing to executable, immediately-validated tools is a genuine improvement, and the reconcileToolDependencies + mergeContextDeletionTargets logic is well thought through. Test coverage is excellent: guardrail skips, maxMatches/expectedMatchCount, the turn cap, abort, thinking-level passthrough, and the six new reconciliation cases all read clearly. Abort handling (pre-check, once listener, finally cleanup, post-check) is correct.

My main concern is one architectural decision; the rest are minor.

Major — the SQLite working copy looks like over-engineering

This PR adds better-sqlite3 (a native runtime dependency, with prebuild-install + ~12 transitive packages now in bun.lock) to dependencies, purely to hold ephemeral, in-memory, single-threaded planner state — a small list of deletion targets plus a call counter and a last-error string.

Concerns:

1. Install/build cost & reliability. better-sqlite3 runs a prebuild-install postinstall that downloads a platform-specific prebuilt binary (or falls back to node-gyp compilation). For a published CLI (@bastani/atomic, dist/index.js run under Node), every consumer install now carries that native-binary download as a new failure mode. The Bun-compiled binary path side-steps this via the process.versions.bun → bun:sqlite branch, but the npm package does not.
2. No actual benefit. The data is tiny, fully in memory (:memory:), and the whole transcript is already materialized in memory as CompactableTranscript. The SQLite copy duplicates it for zero memory or persistence win, then full-table-scans it for grep (including a correlated sibling-count subquery per block) — strictly more overhead than iterating the arrays you already hold.
3. The "parallel-safe" justification doesn't hold. tool/grepTool use executionMode: "parallel", but each execute body is fully synchronous inside store.transaction() (no await between BEGIN IMMEDIATE and COMMIT). On JS's single thread, two Promise.all-dispatched calls already run to completion one-after-another regardless of SQLite — the BEGIN IMMEDIATE serialization is redundant. A plain array mutated synchronously behind the same controller interface would be equally safe (the allows parallel tool execution while serializing shared planner state test would still pass).

The ContextDeletionPlannerToolController interface is a clean seam — I'd suggest backing it with an in-memory structure (array of targets + counters) instead of ContextDeletionSqliteStore/SqliteAdapter. That drops ~250 lines, the native dependency, the dual-runtime require shim, and the install-time binary download, with no behavior change. If there's a concrete requirement driving SQLite (durability across a crash, transcripts too large for memory, future on-disk spill), it's worth calling out explicitly in the PR — as written the state is transient and the motivation isn't visible.

Minor

• Module-level mutable global warnedReconciliationNonConvergence (context-compaction.ts) is process-wide and never reset, so the non-convergence warning fires at most once for the entire process lifetime. For a long-running session that can hide later genuinely-different non-convergence events. Consider a per-run flag, or at least a comment that "once per process" is intentional.
• ReDoS heuristic is best-effort. assertSafeRegexPattern catches common nested-quantifier/alternation/backreference shapes but cannot catch everything (JS RegExp has no timeout). The MAX_REGEX_SCAN_CHARS cap is the real backstop — worth a one-line comment that the heuristic is defense-in-depth, not a guarantee, so a future reader does not over-trust it.
• planContextDeletions now drives a full model loop. The docstring correctly warns that calling it and contextCompact independently re-runs planning. Since it now does real model work (not just parsing), double-check no caller invokes both in sequence.
• Stale-error edge: in contextCompact, if validatedPlan exists with zero targets while lastToolError is set from an earlier recovered tool call, the thrown message appends a tool error that was not actually the cause. Low impact, but the wording could mislead.
• Exact pin better-sqlite3: 12.10.0 / @types/better-sqlite3: 7.6.13 diverges from the ^-range convention used by neighbors — fine for a native module, just flagging for consistency. (Moot if the dependency is dropped per the above.)

Nice work overall — the core redesign and tests are solid; the SQLite layer is the one thing I would push back on before merge.

🤖 Generated with Claude Code

[claude]

Review: agent-loop planner with transcript-bound deletion tools

Thorough, well-tested change — the incremental tool-bound deletion model is a real improvement over parsing a final JSON blob, and the reconciliation/validation hardening (with six new targeted tests) is the strongest part of the PR. The ReDoS guard, scan-size caps, and maxMatches/expectedMatchCount guardrails on context_grep_delete are nice defensive touches. Comments below, roughly in priority order.

🔴 Adding better-sqlite3 (native dep) for an in-memory transient store

This is the headline concern. The PR pulls in better-sqlite3@12.10.0 (plus a tail of transitive native-build deps: prebuild-install, tar-fs, node-abi, etc.) into @bastani/atomic, a package that ships as a bundled CLI and is cross-compiled to single binaries.

• The store is purely an in-memory :memory: working copy (createTransientSqliteDatabase, context-compaction.ts:1158) duplicating data already fully resident in CompactableTranscript. The grep/search/read tools just run SELECT ... ORDER BY position (listEntriesForGrep, listContentBlocksForGrep) — i.e. a linear scan equivalent to iterating the in-memory arrays you already have. SQLite buys nothing here over a Map<entryId, entry> + arrays.
• The "parallel-safe" justification doesn't hold. Every tool execute body is fully synchronous inside store.transaction() (BEGIN IMMEDIATE → sync work → COMMIT), so they serialize on the JS event loop regardless of executionMode: "parallel". The transaction provides no real concurrency protection — and if any body ever became async, the nested BEGIN IMMEDIATE would throw "cannot start a transaction within a transaction". So the design is load-bearing on the work being synchronous, which a plain JS structure gives you for free.
• Native-dep risks for a bundled/cross-compiled CLI: bun build --compile --target=bun-<platform> (see scripts/build-binaries.sh:100) statically analyzes the string-literal moduleRequire("better-sqlite3") call. Even though the Bun runtime path returns early via bun:sqlite, the bundler may still try to resolve/embed the native .node module — at minimum please confirm bun run build:binary and a cross-target build still succeed. For Node (npm) consumers, better-sqlite3 adds an install-time prebuild-install/compile step that fails offline or on unsupported platforms — a meaningful fragility increase for a CLI.

Recommendation: drop SQLite and back the store with plain in-memory structures (the data is already in transcript.entries). It removes the native dependency, the dual-runtime bun:sqlite/better-sqlite3 branch, the SQL schema, and the row-mapping adapters — net simplification with no behavior loss. If there's a concrete reason SQLite is needed (e.g. a planned on-disk persistence story), it'd help to state it explicitly, since today everything is :memory: and torn down per planning run.

🟠 writePlannerTranscriptFile appears to be vestigial / writes transcript to disk for no consumer

runContextDeletionPlanner writes the full transcript to a temp JSONL (context-compaction.ts:1894) and embeds the path in the prompt, but the planner Agent is given only [tool, grepTool, searchTool, readEntryTool] — none of which read that file (they all read from the SQLite store). The prompt (context-compaction.ts:339) even tells the model the file exists but to use context_search_transcript/context_read_entry instead. So the write is dead I/O that also spills potentially sensitive transcript content to a tmp file, and the prompt misleads the model into thinking it can open a file it has no tool for. Either wire up a file-backed read path or remove the file write and the <transcript-file> prompt reference.

🟠 Turn-cap discards already-accumulated valid deletions

When plannerTurnCount > CONTEXT_COMPACTION_PLANNER_MAX_TURNS, streamFn returns an error stream (context-compaction.ts:2026) which sets agent.state.errorMessage, causing runContextDeletionPlanner to throw (:2053). But the planner may have already validated and accumulated useful deletion targets across earlier turns. Hitting the cap throws all of that away and fails the whole compaction. Consider treating the cap as a graceful stop that returns plannerTool.getValidatedPlan() (if non-empty) rather than an error.

🟡 Minor

• Module-global mutable state: warnedReconciliationNonConvergence (:769) is process-global, so the "did not converge" warning fires at most once per process and leaks across test runs. A per-run flag (or threading it through) would be cleaner and test-isolated.
• Prompt typo: "Unresolved bugss" (:323).
• Redundant validation: context_deletion_plan validates incomingPlan and then re-validates the merged set inside applyValidatedTargets (:1484–:1485). Harmless (idempotent) but worth a comment noting the double pass is intentional for early per-call error reporting.
• Legacy exports: parseContextDeletionPlanResponse / parseContextDeletionPlan / isContextDeletionPlanToolCall are now off the hot path (tool owns the plan). They're still exercised by context-compaction.test.ts, so fine to keep, but flagging as prune candidates once the old parse path is fully retired.

✅ Tests

Coverage is good: the new reconciliation/promotion unit tests, the grep guardrail tests (protected skip, maxMatches, scan-size, regex rejection), the terminate: false continuation, the turn-cap stop, and the abort path are all exercised. Two gaps worth adding: (1) a test asserting behavior when the planner only inspects (search/read) but never deletes — confirming contextCompact surfaces "No safe context deletions proposed" with lastToolError; and (2) the Node/better-sqlite3 runtime branch of createTransientSqliteDatabase (current tests run under Bun, so the better-sqlite3 path is never executed in CI — meaning a broken native install wouldn't be caught by tests).

Overall: solid logic and validation work. The main ask is reconsidering whether SQLite + a native dependency is warranted for an in-memory, per-run working copy, and removing the unused transcript-file write.

— review by Claude (Opus 4.8)

[claude]

Review: agent-loop planner with transcript-bound deletion tools

Thorough, well-structured PR. The shift from parsing a final completeSimple response to executable, transcript-bound tools that validate-on-invocation is a genuine robustness win — it removes final-response JSON parsing as a failure mode, and the validation/reconciliation logic is carefully built. Test coverage is excellent (24 tool tests + 21 compaction tests covering guardrails, regex-safety, critical-overflow, turn-cap, overflow recovery, and the serialized-parallel path). Findings below, roughly by impact.

🔴 Worth resolving before merge

1. better-sqlite3 as a hard dependency for a Bun-compiled CLI.
packages/coding-agent/package.json adds better-sqlite3@12.10.0 to dependencies (not optionalDependencies), pulling in a native module chain (prebuild-install, node-abi, tar-fs, …). But createTransientSqliteDatabase() only uses better-sqlite3 on the non-Bun path — and this package ships as bun --compile binaries (6 cross-compiled targets) that run under Bun and use the builtin bun:sqlite. For npm consumers the native module either fetches a prebuilt binary or compiles from source (node-gyp/python/C++) at install time — real install friction for a path the primary distribution never executes. Please either (a) move it to optionalDependencies with a graceful guard, or (b) reconsider the next point.

2. Is a SQLite engine warranted here at all?
ContextDeletionSqliteStore is a transient :memory: DB holding a few dozen transcript entries for the duration of one synchronous planning loop. The grep/search tools SELECT ... ORDER BY position to pull all rows into JS and run the regex in JS — SQLite isn't doing the matching, just acting as an in-memory list. The reconciliation/merge logic (reconcileToolDependencies, mergeContextDeletionTargets) already operates on plain arrays/Maps. The stated justification ("parallel-safe tool execution") isn't load-bearing — see point 3. A plain in-memory structure would drop ~250 lines (adapter + schema + SQL + the dual-runtime fork) and the native dependency. If there's a forward-looking reason for SQLite (cross-run persistence, very large transcripts), a comment stating it would help; otherwise this reads as over-engineering for an ephemeral working copy.

🟡 Should address

3. "Parallel" execution safety relies on fully-synchronous tool bodies.
Tools are declared executionMode: \"parallel\" with toolExecution: \"parallel\", share one SQLite connection, and each execute wraps work in store.transaction() → BEGIN IMMEDIATE ... COMMIT. This is safe today only because every execute body is synchronous (no await inside the transaction), so Promise.all([...]) runs each to completion before the next starts. Correct, but fragile: introducing any await inside a transaction would throw "cannot start a transaction within a transaction" on the shared connection. Worth a comment at the transaction() boundary documenting this invariant (the "serializes shared deletion state" test is good but doesn't enforce the no-await rule).

4. PR description is stale vs. the merged code. The description references context_deletion_plan, createContextDeletionPlannerTool, getPlan/getValidatedPlan, and CONTEXT_COMPACTION_PLANNER_MAX_TURNS = 8. The code actually uses context_delete, createContextDeletionTool, getDeletionRequest/getValidatedResult, and CONTEXT_COMPACTION_MAX_TURNS = 50. Please reconcile — relevant to the next point too.

5. CONTEXT_COMPACTION_MAX_TURNS = 50 is a lot for an automatic operation. With maxTokens capped at 4096 per turn, this permits up to ~50 model round-trips per compaction, which runs automatically (including overflow recovery). That's significant latency/cost for a background task. The description's "8" feels more defensible — confirm 50 is intentional, and consider a tighter cap (or a token budget across turns).

🟢 Minor / nits

• Plaintext transcript in tmpdir(): writeContextCompactionTranscriptFile writes the full (potentially sensitive) conversation to os.tmpdir() in plaintext. mkdtempSync yields a 0700 dir on most platforms and cleanup is correctly in a finally, so low-risk — just flagging.
• warnedReconciliationNonConvergence is module-global, so the non-convergence warning fires at most once per process (and persists across tests). Intentional de-spam, but worth a note since the warning becomes effectively un-observable after the first occurrence.
• thinkingLevel + 4096 cap: with thinking enabled at the lowest supported level, the thinking budget shares the 4096-token output ceiling and could truncate on complex turns. Probably fine for a planner; flagging.
• A few as number / as casts around blockIndex (normalizeRawTarget, the SQLite row mapping) — acceptable given the schema, but CLAUDE.md's "avoid casts/unknown" guidance suggests narrowing where cheap.

Tests

Strong coverage; test names map cleanly to the guardrails (protected-skip, maxMatches, regex length/backtracking/scan-size, content-block promotion, critical-overflow boundaries, turn-cap and overflow graceful degradation). One gap: no test exercises the non-Bun (better-sqlite3) branch of createTransientSqliteDatabase — understandable since the suite runs under Bun, but that path ships untested.

Overall: solid, careful work. The main asks are (1)/(2) — justify or shed the SQLite + native dependency — and (4)/(5) — sync the description and confirm the 50-turn cap.

🤖 Generated with Claude Code