⚠️ Potential issue | 🟡 Minor

Consider adding a foreign key constraint on conversation_id.

The lcm_rollups table lacks a foreign key to conversations(conversation_id). This means:

• Rollups won't be cascade-deleted when conversations are deleted.
• Orphaned rollup data may accumulate over time.

If this is intentional for FTS5-independence or to avoid schema changes to existing tables, consider adding a cleanup mechanism or periodic GC for orphaned rollups.

Verify each finding against the current code and only fix it if needed.

In `@src/db/migration.ts` around lines 888 - 910, The lcm_rollups table is missing
a foreign key to conversations(conversation_id); add a foreign key constraint on
conversation_id (e.g., FOREIGN KEY(conversation_id) REFERENCES
conversations(conversation_id) ON DELETE CASCADE) in the CREATE TABLE for
lcm_rollups to ensure rollups are removed when a conversation is deleted; if you
cannot alter the existing table in-place (SQLite limitation), implement the
change by creating a new table with the FK, copying data, dropping the old table
and renaming the new one, or alternatively add a periodic GC routine to delete
orphaned rows from lcm_rollups by joining against conversations.

⚠️ Potential issue | 🔴 Critical

Non-awaited replaceRollupSources call inside transaction.

Line 215-222 calls this.store.replaceRollupSources(...) without await. Due to the bug in replaceRollupSources using void withDatabaseTransaction(...), the source replacement may not complete before the outer transaction commits.

This ties to the critical issue in rollup-store.ts. Once that's fixed to return a Promise, this call must be awaited:

-      this.store.replaceRollupSources(
+      await this.store.replaceRollupSources(
         rollupId,
         summaries.map((summary, index) => ({
           type: "summary",
           id: summary.summaryId,
           ordinal: index,
         })),
       );

Verify each finding against the current code and only fix it if needed.

In `@src/rollup-builder.ts` around lines 182 - 254, The call to
this.store.replaceRollupSources(...) inside the withDatabaseTransaction block is
not awaited, so the replacement may not finish before the transaction commits;
update the call in rollup-builder.ts to await
this.store.replaceRollupSources(...) (after rollup-store.ts has been fixed to
return a Promise) so the source-replace runs inside and completes within the
surrounding withDatabaseTransaction; ensure you await replaceRollupSources in
the same transaction before calling upsertRollup/upsertState to preserve
transactional guarantees.

🧹 Nitpick | 🔵 Trivial

Token estimation is a rough heuristic.

estimateTokens uses ceil(length / 4) which assumes ~4 chars/token (typical for English with GPT-style tokenizers). This underestimates for CJK text (~1-2 chars/token) and varies for code.

Since it's used consistently for both budget checking and comparison, the relative accuracy is acceptable. Consider documenting the limitation or using an actual tokenizer if precision matters for production rollups.

Verify each finding against the current code and only fix it if needed.

In `@src/rollup-builder.ts` around lines 507 - 513, estimateTokens currently uses
a simple heuristic (Math.ceil(text.length / 4)) which misestimates for CJK and
code; update the implementation or surrounding docs: either replace
estimateTokens with a real tokenizer call (e.g., use the project's tokenizer
utility or an external tokenizer) and use that in all places that call
estimateTokens, or add an inline comment / JSDoc on the estimateTokens function
explaining the ~4 chars/token heuristic and its limitations for CJK and code so
callers know the inaccuracy; reference the estimateTokens function to locate and
change the behavior or add the documentation.

⚠️ Potential issue | 🔴 Critical

Critical: Transaction not awaited — race condition and silent failures.

void withDatabaseTransaction(...) discards the Promise, making this fire-and-forget. The DELETE/INSERT operations may not complete before the outer transaction in buildDayRollup commits, causing:

• Race condition: sources may not be persisted
• Silent failures: errors are swallowed

-  replaceRollupSources(rollupId: string, sources: RollupSourceInput[]): void {
-    void withDatabaseTransaction(this.db, "BEGIN", () => {
+  async replaceRollupSources(rollupId: string, sources: RollupSourceInput[]): Promise<void> {
+    await withDatabaseTransaction(this.db, "BEGIN", () => {
       this.db.prepare(`DELETE FROM lcm_rollup_sources WHERE rollup_id = ?`).run(rollupId);

       if (sources.length === 0) {
         return;
       }

       const insert = this.db.prepare(
         `INSERT INTO lcm_rollup_sources (rollup_id, source_type, source_id, ordinal)
          VALUES (?, ?, ?, ?)`,
       );

       for (const source of sources) {
         insert.run(rollupId, source.type, source.id, source.ordinal);
       }
     });
   }

-      this.store.replaceRollupSources(
+      await this.store.replaceRollupSources(

Verify each finding against the current code and only fix it if needed.

In `@src/store/rollup-store.ts` around lines 274 - 291, The replaceRollupSources
function fires a transaction with void withDatabaseTransaction(...) which
discards the Promise causing race conditions and silent failures; make
replaceRollupSources return a Promise and await the transaction (change
signature to async and remove the void) so the DELETE/INSERT complete before
callers proceed, and then update call sites such as buildDayRollup in
src/rollup-builder.ts to await replaceRollupSources to ensure the transaction is
completed before the outer transaction commits.

🧹 Nitpick | 🔵 Trivial

Type-safe database accessor uses internal property access.

The getLcmDatabase function accesses lcm.db through a type assertion. This works but couples to internal implementation. If the engine's database access changes, this will break silently.

Consider exposing a proper getter on LcmContextEngine or documenting this coupling.

Verify each finding against the current code and only fix it if needed.

In `@src/tools/lcm-recent-tool.ts` around lines 99 - 105, getLcmDatabase currently
reaches into an internal property via a type assertion (candidate.db) which
couples callers to LcmContextEngine's implementation; add a public accessor on
LcmContextEngine (e.g., getDatabase(): DatabaseSync | undefined or throw) and
update getLcmDatabase to call that accessor (handle undefined by throwing the
same error) so callers use the stable API instead of candidate.db, or
alternatively document this coupling on LcmContextEngine if adding a getter is
not possible.

Do not skip rebuild checks when pending_rebuild is 0

This early return prevents any fingerprint comparison or day-level rebuild once a state row exists with pending_rebuild = 0. In this same commit, buildDayRollup always writes pending_rebuild: 0, and there is no corresponding path that flips it back to 1 when new leaf summaries arrive, so subsequent buildDailyRollups() calls will skip all days and leave rollups stale.

Useful? React with 👍 / 👎.

Avoid empty grep query in all-conversations fallback

Passing query: "" into retrieval.grep makes this fallback effectively return nothing for most backends (FTS query sanitization yields an empty phrase and the LIKE fallback drops empty-term queries), so lcm_recent(allConversations=true) can incorrectly report no activity even when messages/summaries exist in the requested time window.

Useful? React with 👍 / 👎.

Stop parsing date keys via fixed noon UTC timestamps

Using T12:00:00.000Z to represent a calendar day shifts to the next local date in high positive-offset zones (for example UTC+13/UTC+14), so downstream calls like getLocalDayBounds(parseDateKey(dateKey), tz) can compute bounds for the wrong day and misfile rollups by one date.

Useful? React with 👍 / 👎.

upsertRollup updates rollup_id on conflict (rollup_id = excluded.rollup_id). Because lcm_rollup_sources.rollup_id is a foreign key without ON UPDATE CASCADE, changing the primary key can either fail with a foreign key constraint error or orphan existing lcm_rollup_sources rows. Consider keeping the existing rollup_id on conflict (don’t update the PK), or add ON UPDATE CASCADE to the foreign key if PK updates are required.

replaceRollupSources calls the async withDatabaseTransaction(...) but does not await it (uses void ...). This can allow the caller to continue before the delete/insert work is committed/released, and any errors become unhandled promise rejections. Make this method async and await the transaction (and propagate errors).

The all-conversations fallback calls retrieval.grep with query: "" and mode: "full_text". With the current FTS5 sanitization this likely becomes a match-all query, which can be extremely expensive because it forces the FTS table to return/score a huge result set before created_at ordering + LIMIT can be applied. Prefer a dedicated "recent items" query path (e.g., fetch messages/summaries by created_at within the bounds) instead of FTS when there is no search term.

parseIsoTimestampParam(p, "since"|"before") is invoked but its return values are unused and any parsing errors are swallowed. Since since/before aren’t part of this tool’s schema, this code is effectively a no-op and can hide invalid caller input. Either remove this block entirely, or add since/before to the schema and actually apply them (with proper validation/error reporting).

This tool is registered, but there’s no wiring that actually builds/populates lcm_rollups (e.g., RollupBuilder isn’t invoked anywhere in the runtime). As a result lcm_recent will almost always hit fallback behavior, which doesn’t match the PR description of adding a daily/weekly/monthly rollup layer. Either add a scheduled/background build step (or hook into existing compaction/summarization flows) or adjust the PR description/tool description to reflect that rollups must be built externally.

The tool description says it "Falls back to time-bounded lcm_grep when no rollup exists", but the single-conversation fallback path instead runs a direct SQL query over summaries (leaf only) and does not include message matches. If the intended fallback is lcm_grep-style behavior, consider routing through retrieval.grep (with appropriate query semantics) or update the tool/PR description to accurately describe the implemented fallback behavior.

No automated tests were added for lcm_recent period resolution, rollup selection (single-period vs multi-day combine), and fallback formatting. The repo already has Vitest coverage for other LCM tools (e.g. test/lcm-tools.test.ts), so adding similar tests here would help prevent regressions in date/timezone logic and output structure.

lcm_rollup_sources.rollup_id references lcm_rollups(rollup_id) with ON DELETE CASCADE but no ON UPDATE action. Since RollupStore.upsertRollup currently attempts to update rollup_id on conflict, this schema will either block the update (FK constraint failure) or leave sources inconsistent. Either avoid updating rollup_id in the upsert, or define the FK with ON UPDATE CASCADE (and migrate existing DBs accordingly).

🟡 Missing changeset file for new user-facing features (lcm_recent tool, rollup tables)

Per AGENTS.md: "Before merging a PR, check whether it changes user-facing behavior or should appear in npm release notes. If yes, make sure a maintainer adds a .changeset/*.md file before merge or immediately after in a follow-up PR." and "Treat a PR as not release-ready until the changeset question has been answered."

This PR adds significant new user-facing features — the lcm_recent tool, rollup database tables, RollupStore, and RollupBuilder — but contains no corresponding changeset file. The existing changesets (wise-bears-doubt.md and async-bootstrap-sync-io.md) are for unrelated features. A minor bump changeset should be added for this new feature.

Was this helpful? React with 👍 or 👎 to provide feedback.

📝 Info: markStale call is redundant before upsertRollup in same transaction

At src/rollup-builder.ts:178-184, markStale(existing.rollup_id) sets the row's status to 'stale'. Immediately after, upsertRollup at line 186 overwrites the same row (same conversation_id, period_kind, period_key) with status 'ready' via ON CONFLICT ... DO UPDATE. The markStale write is immediately overwritten and has no observable effect since both operations are in the same transaction. If the intent was to leave an audit trail of the stale transition, it's lost. Not a bug but unnecessary work.

Was this helpful? React with 👍 or 👎 to provide feedback.

🚩 getLcmDatabase accesses private field via unsafe cast — fragile coupling

In src/tools/lcm-recent-tool.ts:99-105, getLcmDatabase casts LcmContextEngine through unknown to access the private readonly db field (src/engine.ts:1201). This works at runtime because TypeScript's private is not enforced in JavaScript, but it creates fragile coupling — renaming or making db a #private field would silently break this. No other tool in the codebase uses this pattern (confirmed via search). Consider adding a public accessor like getDatabase() on LcmContextEngine if tools need DB access.

Was this helpful? React with 👍 or 👎 to provide feedback.

📝 Info: localDateTimeToUtc uses single-pass offset correction — may drift near DST transitions

In src/rollup-builder.ts:546-551, localDateTimeToUtc computes a UTC date by guessing a UTC time, calculating the timezone offset at that guess, and subtracting. This single-pass approach can produce incorrect results near DST transitions where the offset at the guessed UTC time differs from the offset at the corrected UTC time. However, this function is only called from getLocalDayBounds with 00:00:00 time, and DST transitions almost universally happen at 2am+, so midnight conversions are safe in practice. Worth documenting this limitation if the function is ever reused for arbitrary times.

Was this helpful? React with 👍 or 👎 to provide feedback.

🧹 Nitpick | 🔵 Trivial

Rollup tables: Schema is sound, orphan cleanup needed.

The tables are well-structured with appropriate constraints:

• lcm_rollups: unique on (conversation_id, period_kind, period_key)
• lcm_rollup_sources: cascades on rollup deletion

As noted in a previous review, lcm_rollups and lcm_rollup_state lack FKs to conversations. This appears intentional for FTS5-independence but means orphaned rollups won't auto-delete when conversations are removed. The Phase 2 TODO in index.ts (line 1880-1881) should include periodic GC for orphaned rollup data.

Verify each finding against the current code and only fix it if needed.

In `@src/db/migration.ts` around lines 887 - 931, Add a periodic GC job (Phase 2
TODO in index.ts) that removes orphaned rollup data left when conversations are
deleted: run a transaction that DELETEs from lcm_rollups where conversation_id
NOT IN (SELECT id FROM conversations) (this will cascade-remove
lcm_rollup_sources), and DELETEs from lcm_rollup_state where conversation_id NOT
IN (SELECT id FROM conversations); schedule it to run regularly (daily or
configurable), wrap in a safe transaction with logging and error handling, and
expose a way to trigger it manually for backfill/recovery.

🧹 Nitpick | 🔵 Trivial

Redundant leaf filter.

getLeafSummariesForDay already filters to kind = 'leaf' in SQL (line 392 of rollup-store.ts). The .filter((summary) => summary.kind === "leaf") is defensive but redundant.

-      const leafSummaries = summaries
-        .filter((summary) => summary.kind === "leaf")
-        .sort(compareSummariesChronologically);
+      const leafSummaries = summaries.sort(compareSummariesChronologically);

Verify each finding against the current code and only fix it if needed.

In `@src/rollup-builder.ts` around lines 106 - 108, The .filter((summary) =>
summary.kind === "leaf") is redundant because getLeafSummariesForDay already
returns only leaf summaries; remove that filter and directly sort the summaries
with compareSummariesChronologically (i.e., replace const leafSummaries =
summaries.filter(...).sort(...) with const leafSummaries =
summaries.sort(compareSummariesChronologically)), keeping the variable name and
usage intact; reference getLeafSummariesForDay in rollup-store.ts and
compareSummariesChronologically to locate the related logic.

🧹 Nitpick | 🔵 Trivial

Clarify kind semantics for multi-day ranges.

For "7d" and "30d", the returned kind: "day" is semantically misleading since these represent ranges, not single day rollups. The code handles this correctly (no periodKey triggers list-based lookup at line 422), but a comment explaining this behavior would improve maintainability.

   if (normalized === "7d") {
     const startDay = addDays(today, -6);
     return {
       label: "last 7 days",
-      kind: "day",
+      kind: "day", // Multi-day range; no periodKey triggers list-based rollup lookup
       start: getUtcDateForZonedMidnight(startDay, timezone),
       end: getUtcDateForZonedMidnight(addDays(today, 1), timezone),
     };
   }

Verify each finding against the current code and only fix it if needed.

In `@src/tools/lcm-recent-tool.ts` around lines 200 - 218, The returned objects
for normalized values "7d" and "30d" use kind: "day" which is misleading because
these are multi-day ranges; add a brief clarifying comment next to the blocks
handling normalized === "7d" and normalized === "30d" explaining that kind:
"day" denotes the rollup/unit (daily granularity) while the start/end define a
multi-day range and that periodKey (see usage around periodKey at line ~422)
drives list-based lookups rather than single-day behavior; keep
getUtcDateForZonedMidnight and the existing start/end logic intact and only add
the comment to improve maintainability.

🧹 Nitpick | 🔵 Trivial

Consider extracting rollup row mapping.

The mapping from RollupRow to RollupRecord (lines 425-446) is verbose but correct. If reused elsewhere, consider extracting to a shared helper.

function toRollupRecord(row: RollupRow): RollupRecord {
  return {
    rollupId: row.rollup_id,
    // ... rest of mapping
  };
}

Verify each finding against the current code and only fix it if needed.

In `@src/tools/lcm-recent-tool.ts` around lines 422 - 454, Extract the inline
mapping from RollupRow to RollupRecord into a shared helper (e.g.,
toRollupRecord) and use it where rollups are transformed (the current mapping
inside the resolution.kind branch that produces usableRollups); keep the same
field conversions (new Date for
period_start/period_end/coverage_start/coverage_end/built_at/invalidated_at,
parseJsonStringArray for source_summary_ids, and direct mappings for other
fields), then replace the .map(...) call with
rollups.filter(...).map(toRollupRecord) so combineRollups and other consumers
(e.g., combineRollups, rollupStore.listRollups usage) can reuse the conversion.