feat(conversation-memory): complete Phase 4 - observation system documentation and deprecation - #452
Conversation
…ation Implements the foundation of the observation system inspired by claude-mem: - DB schema: observations, session_summaries, pending_events, vec_observations tables - TypeScript interfaces: Observation, SessionSummary, CompactObservation, PendingEvent - Observer core logic with SQLite queue polling and 30min idle timeout - Observation prompts for init, per-tool execution, and session summary - Observation CRUD with embedding generation - Session summary generation and storage - PostToolUse hook integration (<5ms response, no LLM calls) - Observer background process with PID file management - Observer CLI commands: start, stop, status - Hooks.json updates for PostToolUse and Stop events - CLI routing for observe and observer commands - Comprehensive tests for all new functionality Key architectural differences from claude-mem: - SQLite queue instead of Express HTTP for IPC - One Gemini conversation per session (prompt caching benefits) - Low-value tools skipped at code level (no LLM filter needed) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…observer shutdown Implements SessionStart hook context injection and observer shutdown: - Inject CLI for SessionStart hook (displays recent observations as compact table) - Inject context formatter with stats and MCP tool usage hints - Progressive disclosure Layer 1 format (~30 tokens per observation) - Hooks.json updates for SessionStart (inject, sync, observer start) - CLI routing for inject command - Observer process shutdown handling after summarize event - Graceful PID file cleanup and database connection closure Key features: - SessionStart inject shows last 7 days of observations (max 30) - Observations grouped by date in markdown table format - Includes stats (observation count, token estimate) - MCP tool hints: search(), get_observations(), read() - Observer shuts down gracefully after generating session summary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…mentation and deprecation Complete the observation system implementation with documentation updates and deprecation notices for legacy exchange-based search. Changes: - Update all documentation for observation-based progressive disclosure (3 layers) - Add deprecation notice to searchMultipleConcepts (v6.0 → v7.0 removal) - Fix observer CLI bugs (import and subcommand routing) - Add project field to PendingEvent schema and processing Phase 4 complete: All 4 phases of the observation system design are now implemented, documented, and ready for use. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe conversation-memory plugin was reworked into an observation-driven, progressive-disclosure system: new DB tables and types for observations and events, an observer pipeline (CLI + background process) that creates observations from tool events, a three-layer search/get_observations/read flow, CLI hooks, MCP tool changes, and updated docs/tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Agent/Client
participant Hook as Hook (SessionStart / PostToolUse)
participant CLI as Inject/Observe CLI
participant DB as Database
participant Observer as Observer Process
participant LLM as LLM
Client->>Hook: trigger (SessionStart / PostToolUse / Stop)
Hook->>CLI: run inject / observe (--summarize?)
CLI->>DB: query or insert pending_event
DB-->>CLI: results / ack
Observer->>DB: poll pending_events
DB-->>Observer: pending_event data
Observer->>LLM: buildObservationPrompt / buildSummaryPrompt
LLM-->>Observer: XML observation / summary
Observer->>DB: insertObservation / insertSessionSummary
DB-->>Observer: persisted
sequenceDiagram
participant Agent as Search Agent
participant MCP as MCP Server
participant Search as searchObservations
participant Vec as vec_observations (vector)
participant DB as Database
participant Agent2 as Agent (follow-up)
Agent->>MCP: search(query, options)
MCP->>Search: searchObservations(query, options)
Search->>Vec: vector similarity query
Vec-->>Search: candidate ids + scores
Search->>DB: text filters & metadata joins
DB-->>Search: compact observations
Search-->>MCP: Layer 1 results (compact)
Agent->>MCP: get_observations(ids)
MCP->>DB: getObservationsByIds(ids)
DB-->>MCP: full observation details (Layer 2)
Agent2->>MCP: read(path) [opt]
MCP->>DB: read raw transcript (Layer 3)
DB-->>MCP: raw conversation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugins/conversation-memory/src/cli/index-cli.ts (1)
113-116:⚠️ Potential issue | 🟡 MinorComments claim dependency check is bypassed, but
ensureDependencies()runs for all commands.Lines 120 and 127 say "handle directly without dependency check for speed", but
ensureDependencies()at line 116 executes before the switch statement, so the check still runs. Either move the fast-path commands before theensureDependencies()call, or update the comments.Proposed fix — move fast-path commands before dependency check
async function main() { try { + // Fast-path commands that bypass dependency check + if (command === 'inject') { + await import('./inject-cli.js'); + return; + } + if (command === 'observe') { + await import('./observe-cli.js'); + return; + } + if (command === 'observer' || command === 'observer-run') { + await import('./observer-cli.js'); + return; + } + // Ensure dependencies are installed await ensureDependencies(); switch (command) { - case 'inject': - // Inject command - handle directly without dependency check for speed - await import('./inject-cli.js'); - break; - - case 'observe': - case 'observer': - case 'observer-run': - // Observer commands - handle directly without dependency check for speed - await import('./observer-cli.js'); - break; - case 'index-session':plugins/conversation-memory/skills/remembering-conversations/MCP-TOOLS.md (1)
144-196:⚠️ Potential issue | 🟡 MinorDocumentation inconsistency: Response Format and Error Handling sections still reference conversations instead of observations.
The "Response Format" section (lines 148-179) and the "Error Handling" section (lines 183-186) still show the old conversation-based output format (paths, summaries, "No conversations found"). However, single-concept search now returns observations, as documented earlier in lines 50-78. This creates confusion for readers—the same
searchtool appears to return two different formats.Either:
- Clarify that this Response Format section applies only to multi-concept (legacy) searches, or
- Update it to show the observation-based response format for single-concept queries.
Similarly, line 185 says "No conversations found" but the error table at line 470 says "No observations found".
plugins/conversation-memory/src/core/db.ts (1)
24-34:⚠️ Potential issue | 🔴 CriticalBug: Migration for
pending_events.projectwill fail due to redundant column definition.The migration at line 24 targets the
pending_eventstable, but the migration system (line 9) only checks columns from theexchangestable. This meanscolumnNames.has('project_pending_events')is alwaysfalse, causingALTER TABLE pending_events ADD COLUMN project TEXTto execute on everyinitDatabase()call.However, the
CREATE TABLE IF NOT EXISTS pending_eventsat lines 144–159 already definesproject TEXTat line 154. SQLite will throw a "duplicate column name: project" error on execution since there is no error handling.Fix: Remove the redundant migration entry since the column is already included in the
CREATE TABLEstatement.{ name: 'thinking_triggers', sql: 'ALTER TABLE exchanges ADD COLUMN thinking_triggers TEXT' }, { name: 'compressed_tool_summary', sql: 'ALTER TABLE exchanges ADD COLUMN compressed_tool_summary TEXT' }, - { name: 'project_pending_events', sql: 'ALTER TABLE pending_events ADD COLUMN project TEXT' }, ];
🤖 Fix all issues with AI agents
In `@plugins/conversation-memory/hooks/hooks.json`:
- Around line 31-41: The PostToolUse/Stop hooks are invoking the "observe"
command but index-cli.ts routes "observe" to observer-cli.js which has no case
for "observe", so observe-cli.js (and its handlePostToolUse/--summarize Stop
logic) is never executed; fix by adding an "observe" branch in the command
switch in index-cli.ts that imports/dispatches to the module that implements the
observe handlers (the code in observe-cli.js / functions like handlePostToolUse
and summarize stop logic) or alternately change hooks.json to call the existing
subcommands (e.g., a direct "node .../dist/observe-cli.js observe" or the
correct subcommand name); ensure the new branch invokes the same entrypoint used
by observe-cli.js so PostToolUse and Stop events are queued to the DB.
In `@plugins/conversation-memory/src/cli/index-cli.ts`:
- Around line 119-129: The switch handling for commands incorrectly imports
observer-cli for the 'observe' case; update the case branch so that 'observe'
(used by PostToolUse/Stop hooks and event queuing) imports './observe-cli.js'
while leaving 'observer' and 'observer-run' importing './observer-cli.js'
unchanged; modify the import statement in the switch block that currently
references './observer-cli.js' for the 'observe' case to './observe-cli.js'
(look for the switch handling the 'inject'/'observe'/'observer'/'observer-run'
cases in index-cli.ts).
In `@plugins/conversation-memory/src/cli/observer-cli.ts`:
- Around line 16-17: The bug stems from calculating subcommandIndex by grouping
'observer' and 'observer-run' together; change the ternary so only 'observer'
expects a subcommand (e.g., subcommandIndex = process.argv[2] === 'observer' ? 3
: 2) so that when argv[2] is 'observer-run' the command variable picks
'observer-run' instead of defaulting to 'status'; additionally, wrap the
'observer-run' switch case body in braces and keep the const declarations scoped
inside that block to satisfy Biome's noSwitchDeclarations rule and avoid
block-scope issues for the code that handles the 'observer-run' case.
In `@plugins/conversation-memory/src/core/inject.ts`:
- Around line 69-72: The JSON.parse calls for row.facts, row.concepts,
row.filesRead, and row.filesModified in the observation construction within
plugins/conversation-memory/src/core/inject.ts are unguarded and will throw on
null/empty/malformed values; replace them with a safe parse helper (e.g.,
safeParseJson) or wrap each parse in try/catch returning a sensible default
(empty array or object) so malformed DB data doesn't crash injection; update the
code paths that build the observation objects (the location creating facts,
concepts, filesRead, filesModified) to use this safe parsing helper and ensure
defaults are explicit.
In `@plugins/conversation-memory/src/core/observation-prompt.ts`:
- Around line 63-71: The XML template in observation-prompt.ts interpolates
toolName, cwd, project, and JSON.stringify(toolInput) without escaping which can
break XML; update the template to pass each interpolated value through the
existing escapeXml function (i.e., replace direct ${toolName}, ${cwd},
${project}, and ${JSON.stringify(toolInput, null, 2)} with ${escapeXml(...)} )
so that toolName, cwd, project and the JSON stringified toolInput are
XML-escaped before being inserted into the <tool_event> tags, keeping the
already-escaped ${escapeXml(toolResponse)} as-is.
- Around line 96-101: The buildSummaryPrompt return currently injects raw
sessionContext and project into XML; update buildSummaryPrompt to XML-escape
those values (escape &, <, >, " and ') before interpolation to avoid malformed
XML—either call an existing xmlEscape/escapeXml helper or add a small utility
(e.g., escapeXml) and use it when inserting sessionContext and project into the
template string in observation-prompt.ts.
In `@plugins/conversation-memory/src/core/observer.ts`:
- Around line 280-313: processSummarizeEvent currently calls shutdownObserver
which invokes process.exit(0) and kills the process before
pollPendingEvents.markEventProcessed runs, causing re-processing of the
summarize event; to fix, ensure the event is marked processed before exiting by
calling the existing markEventProcessed for the summarize event (from
pollPendingEvents) prior to calling shutdownObserver, or refactor
shutdownObserver to remove process.exit and instead return control to the caller
(processSummarizeEvent) so the caller can call markEventProcessed and then exit;
update references in processSummarizeEvent, shutdownObserver, and
pollPendingEvents (symbols: processSummarizeEvent, shutdownObserver,
markEventProcessed, pollPendingEvents) accordingly.
- Around line 248-253: context.conversationHistory is unbounded and is passed
wholesale to llmProvider.complete which risks exceeding context limits and loses
role info; limit growth by trimming or using a sliding window before calling
llmProvider.complete (e.g., implement a helper like
trimConversationHistory(maxMessagesOrTokens) and call it after every push) and
change the payload generation to include role markers when joining (e.g., map
msg => `${msg.role}: ${msg.content}`) so the model can distinguish user vs
system/model messages; ensure trimming runs both when appending in the tool-use
path and right before calling llmProvider.complete.
- Around line 122-132: The setInterval-based polling creates overlapping async
executions of pollPendingEvents; replace the setInterval logic (the pollInterval
variable and its setInterval invocation) with a single async loop that awaits
pollPendingEvents before sleeping, using POLL_INTERVAL_MS for the delay and
honoring the shouldShutdown return to break the loop; ensure any existing
clearInterval usage or pidPath shutdown signaling is handled (stop referring to
pollInterval inside pollPendingEvents if it expects an interval id) so only one
pollPendingEvents runs at a time and errors are still caught and logged.
In `@plugins/conversation-memory/src/core/search.ts`:
- Around line 515-542: The text search SQL in search.ts constructs
`${whereClause}` which can be empty, then blindly appends `AND (...)`, causing
invalid SQL when there is no preceding WHERE; update the SQL assembly in the
text search block (the prepared statement that creates textStmt) to
conditionally insert either `WHERE` or `AND` before the `(...)` depending on
whether `whereClause` is non-empty (e.g. use `whereClause ? \`${whereClause} AND
(...)\` : \`WHERE (...)\``) so the query is valid when `whereClauses` is empty;
ensure the parameters passed to `textStmt.all(...whereParams, likeQuery,
likeQuery, likeQuery, limit * 2)` still align with the resulting SQL.
- Around line 427-433: The timestamp comparisons in the if (after) and if
(before) blocks use ISO date strings but observations.timestamp is stored as an
INTEGER epoch; parse the provided date strings into a Unix timestamp matching
the DB unit before pushing to whereParams (e.g., const ts = new
Date(after).getTime() / 1000 if the DB stores seconds, or .getTime() for
milliseconds), then push ts into whereParams for the whereClauses; verify the
exact epoch unit in db.ts and use that conversion when updating the if (after)
and if (before) blocks referencing whereClauses and whereParams.
- Line 419: The call to initDatabase() in searchObservations opens a DB
connection that is never closed; wrap the database usage in a try/finally so
db.close() is always invoked (move the existing logic that uses db into the try
block and put db.close() in finally), preserving return values and rethrowing
errors if necessary; apply the same try/finally pattern to the other location
that also calls initDatabase() later in the file so every code path ensures
db.close() is executed (refer to initDatabase(), db.close(), and
searchObservations to find the spots to change).
- Around line 465-512: The vector-query is missing the sqlite-vec
nearest-neighbor MATCH clause so v.distance is meaningless; update the prepared
SQL (used where stmt is built in the vector branch) to include the vec table's
MATCH condition (e.g., "AND v.embedding MATCH ? AND k = ?"/the same pattern used
in searchConversations) and pass the generated queryEmbedding (and k if
required) into whereParams before executing stmt.all(...whereParams, ...). Keep
existing calls to initEmbeddings(), generateEmbedding(), and
applyRecencyBoost(), and ensure JSON.parse usage for facts/concepts/files*
remains unchanged.
In `@plugins/conversation-memory/src/mcp/server.ts`:
- Around line 262-272: SearchInputSchema is strict but missing the types,
concepts, and files fields that clients send, so SearchInputSchema.parse(args)
will reject requests; update SearchInputSchema to include types:
z.array(z.string()).optional(), concepts: z.array(z.string()).optional(), and
files: z.array(z.string()).optional() (or the correct types per JSON tool
schema), then remove the (args as any) casts in the ObservationSearchOptions
construction and use args.types, args.concepts, and args.files directly so
ObservationSearchOptions (used in the single-concept search block and passed to
ObservationSearchOptions) receives the parsed values.
🟡 Minor comments (8)
plugins/conversation-memory/README.md-13-13 (1)
13-13:⚠️ Potential issue | 🟡 MinorInconsistency: "SessionEnd hook" vs "SessionStart Hook" on Line 176.
Line 13 states "SessionEnd hook syncs conversations automatically" but the "How It Works" section at line 176 is titled "Automatic Indexing (SessionStart Hook)" and describes behavior triggered "When each Claude Code session starts." These are contradictory. Based on the PR objectives referencing "SessionStart injection," line 13 appears stale.
Proposed fix
-- **Automatic Indexing**: SessionEnd hook syncs conversations automatically +- **Automatic Indexing**: SessionStart hook syncs conversations automaticallyplugins/conversation-memory/src/cli/observer-cli.ts-71-75 (1)
71-75:⚠️ Potential issue | 🟡 MinorWrap
constdeclaration in a block to prevent scope leakage across switch clauses.As flagged by Biome: the
constdeclaration on line 73 is accessible from other switch clauses, which can lead to accidental reference before initialization (TDZ error).Proposed fix
- case 'observer-run': - // This is the internal command that actually runs the observer - const { startObserver: run } = await import('../core/observer.js'); - await run(); - break; + case 'observer-run': { + // This is the internal command that actually runs the observer + const { startObserver: run } = await import('../core/observer.js'); + await run(); + break; + }plugins/conversation-memory/src/core/observations.test.ts-399-421 (1)
399-421:⚠️ Potential issue | 🟡 MinorTest for JSON-stringified arrays doesn't verify the parsed result.
The test only asserts
toBeDefined()but never checks that the stringified arrays (facts,concepts) were actually parsed back into proper arrays. This test would pass even ifparseObservationreturned them as raw strings.Proposed fix — add value assertions
const observation = parseObservation(data); // Should handle stringified arrays expect(observation).toBeDefined(); + expect(observation?.facts).toEqual(['fact1', 'fact2']); + expect(observation?.concepts).toEqual(['concept1']); });plugins/conversation-memory/src/mcp/server.ts-327-331 (1)
327-331:⚠️ Potential issue | 🟡 Minor
forEachcallback should not return a value.Per the Biome hint, the
forEachcallback at line 329 returns the result ofoutput += .... Use afor...ofloop instead:Proposed fix
if (obs.facts.length > 0) { output += `**Facts:**\n`; - obs.facts.forEach((f: string) => output += `- ${f}\n`); + for (const f of obs.facts) { + output += `- ${f}\n`; + } output += `\n`; }plugins/conversation-memory/skills/remembering-conversations/MCP-TOOLS.md-106-112 (1)
106-112:⚠️ Potential issue | 🟡 MinorMissing blank line before fenced code block.
Per markdownlint MD031, fenced code blocks should be surrounded by blank lines. Line 107 starts a code block immediately after line 106 text.
**Consider using single-concept search with filters instead:** + ```json {plugins/conversation-memory/src/core/observations.ts-66-68 (1)
66-68:⚠️ Potential issue | 🟡 Minor
if (limit)is falsy forlimit = 0, and LIMIT is string-interpolated instead of parameterized.Two issues here:
if (limit)treats0as falsy, so passinglimit = 0silently returns all rows instead of none. Useif (limit !== undefined)(orlimit != null).- The LIMIT value is interpolated directly into the SQL string. While TypeScript types it as
number, this is inconsistent withgetCompactObservations(Line 173) which correctly parameterizes the limit via?. Prefer parameterized queries consistently.Proposed fix (showing getObservationsByProject; apply same pattern to getObservationsByType)
- if (limit) { - sql += ` LIMIT ${limit}`; - } - - const stmt = db.prepare(sql); - const rows = stmt.all(project) as any[]; + const params: (string | number)[] = [project]; + if (limit != null) { + sql += ` LIMIT ?`; + params.push(limit); + } + + const stmt = db.prepare(sql); + const rows = stmt.all(...params) as any[];Also applies to: 100-102
plugins/conversation-memory/src/core/observations.ts-218-244 (1)
218-244:⚠️ Potential issue | 🟡 Minor
parseObservationalso doesn't guard againstNaNfortimestampandcreatedAt.Same
parseIntissue applies totimestamp(Line 225) andcreatedAt(Line 236). If the input is non-numeric, these becomeNaNandvalidateObservationdoesn't check them, leading toNaNvalues stored in the database.plugins/conversation-memory/src/core/observations.ts-191-213 (1)
191-213:⚠️ Potential issue | 🟡 Minor
validateObservationdoes not catchNaNforpromptNumber.
NaN < 0evaluates tofalse, so aNaNpromptNumber passes validation. This matters becauseparseObservation(Line 224) usesparseIntwhich returnsNaNfor non-numeric input, and the result flows throughvalidateObservation.Proposed fix
- if (observation.promptNumber < 0) { + if (!Number.isFinite(observation.promptNumber) || observation.promptNumber < 0) { return false; }
🧹 Nitpick comments (15)
plugins/conversation-memory/hooks/hooks.json (1)
5-13: Synchronous inject hook with 10s timeout — verify this is intentional.The inject hook runs synchronously (no
"async": true), which means it blocks session startup until completion or the 10s timeout. Given the comment ininject-cli.tssays it should complete in ~100ms, this is likely fine — but if the database is slow or locked, it could delay startup noticeably.Consider whether a shorter timeout (e.g., 3-5s) would be safer, or if this is acceptable.
plugins/conversation-memory/src/cli/observe-cli.ts (1)
19-73: Stdin reading may hang if no data is piped and stdin is a TTY.
for await (const chunk of process.stdin)will block indefinitely if stdin is a TTY (no data piped). If this CLI is ever invoked without piped input (e.g., during debugging or a misconfigured hook), it will hang rather than timing out. The hook timeout (async) would eventually kill the process, but this is worth noting.Consider adding a brief check or timeout guard, or at minimum documenting that this CLI must always receive piped input.
plugins/conversation-memory/agents/search-conversation.md (1)
119-128: Consider adding a deprecation timeline for multi-concept search.The PR objectives mention deprecation of
searchMultipleConcepts(planned removal v7.0, deprecation starting v6.0). This section notes it's "legacy" but doesn't mention the deprecation timeline, which would help agent users understand the migration urgency.plugins/conversation-memory/README.md (1)
311-343: Project structure section is outdated and doesn't reflect new modules.The tree listing omits all the new core files added in this PR (e.g.,
observations.ts,observer.ts,inject.ts,session-summary.ts,observation-prompt.ts,paths.ts,types.ts) and the new CLI files (observe-cli.ts,observer-cli.ts,inject-cli.ts). Since this PR significantly expands the architecture, the project structure should be updated to avoid misleading new contributors.plugins/conversation-memory/src/core/observation-prompt.test.ts (2)
169-175: Marginally flaky timestamp assertion ingenerateIdtest.
generateId()capturesDate.now()internally, and line 171 callsDate.now()separately after the fact. If the millisecond counter advances between the two calls,timestamp.substring(0, 10)could differ. In practice this is extremely unlikely (10-second granularity), but a more robust approach would capture the timestamp before and verify the ID falls within the range.Proposed fix
it('should generate IDs with timestamp prefix', () => { + const before = Date.now().toString(); const id = generateId(); - const timestamp = Date.now().toString(); + const after = Date.now().toString(); expect(id).toMatch(/^\d+-/); - expect(id.startsWith(timestamp.substring(0, 10))).toBe(true); + // Verify timestamp prefix is within the expected range + const idTimestamp = id.split('-')[0]; + expect(Number(idTimestamp)).toBeGreaterThanOrEqual(Number(before)); + expect(Number(idTimestamp)).toBeLessThanOrEqual(Number(after)); });
11-11: Unused type imports.
ObservationandSessionSummaryare imported but never referenced in the test file.Proposed fix
-import type { Observation, SessionSummary } from './types.js';plugins/conversation-memory/src/cli/observer-cli.ts (1)
22-38:startObserverdoesn't verify the spawned process started successfully.The function logs "Observer process started" immediately after
spawn()without checking for spawn errors. IfcliPathdoesn't exist ornodecan't be found, the user gets a false success message. Consider listening for the'error'event before printing confirmation.Proposed fix
async function startObserver(): Promise<void> { const pluginRoot = process.env.CLAUDE_PLUGIN_ROOT || process.cwd(); const cliPath = path.join(pluginRoot, 'dist', 'cli.mjs'); const observer = spawn('node', [cliPath, 'observer-run'], { detached: true, stdio: 'ignore', env: { ...process.env, CLAUDE_PLUGIN_ROOT: pluginRoot } }); - observer.unref(); - - console.log('Observer process started'); + observer.on('error', (err) => { + console.error('Failed to start observer process:', err.message); + process.exit(1); + }); + + observer.unref(); + + console.log(`Observer process started (pid: ${observer.pid})`); }plugins/conversation-memory/src/core/inject.ts (1)
5-9:sessionIdsoption is declared but never used in the query.
InjectOptions.sessionIdsis defined butgetObservationsForInjectionnever references it for filtering. Either implement session-based filtering or remove the field to avoid confusing callers.plugins/conversation-memory/src/core/observations.test.ts (1)
35-46: Consider usingfsimport instead of inlinerequire.The
require('fs')on line 39 inside theafterEachis a CommonJS-style import in what appears to be an ESM module (given the.jsextension imports elsewhere). While it works in practice, using a top-levelimport fs from 'fs'would be more consistent with the rest of the file.plugins/conversation-memory/src/core/session-summary.ts (2)
64-90: Duplicated row-to-SessionSummarymapping logic withdb.ts.The SQL column selection, aliasing, and
JSON.parsemapping at lines 68-89 is nearly identical togetSessionSummaryindb.ts(lines 440-466 in the relevant snippet). If theSessionSummaryshape changes, both locations must be updated in lockstep.Consider extracting a shared
mapRowToSessionSummary(row: any): SessionSummaryhelper (or adding agetSessionSummariesByProjectfunction indb.ts) to keep the mapping in one place.
9-24: Thin wrappers add indirection without added logic.
getLatestSessionSummaryandsaveSessionSummaryare 1:1 pass-throughs togetSessionSummaryandinsertSessionSummaryfromdb.ts. They don't add validation, logging, or transformation. Callers could use thedb.tsfunctions directly. This is fine if the intent is to establish a facade for future extension, but worth noting to avoid confusion.plugins/conversation-memory/src/core/db.ts (1)
383-405:getPendingEventsuses spread then override — works but is fragile.Line 401 spreads the entire raw row object (
...row) and then overridestoolInputandprocessed. This works because the SQL aliases match thePendingEventinterface property names. However, if a column alias ever drifts from the interface shape, the spread will silently produce incorrect data. A safer approach is explicit field mapping (likegetSessionSummarydoes).plugins/conversation-memory/src/core/observer.ts (1)
194-221: RedundantpromptCountassignment fortool_useevents.Line 212 sets
context.promptCount = promptNumberinside thetool_usebranch, and line 219 sets it again unconditionally. Fortool_use, this is redundant. For unknown event types, it silently increments the prompt counter without doing useful work.Proposed cleanup
if (event.eventType === 'tool_use') { await processToolUseEvent(db, llmProvider, event, context, promptNumber); - context.promptCount = promptNumber; return false; } else if (event.eventType === 'summarize') { await processSummarizeEvent(db, llmProvider, event, context, pollInterval, pidPath); return true; // Signal shutdown after summarize } context.promptCount = promptNumber; return false;plugins/conversation-memory/src/core/observations.ts (1)
39-45: Extract a shared row-mapping helper to reduce duplication.The JSON-parse mapping for
facts,concepts,filesRead,filesModifiedis duplicated six times. A single helper (e.g.,mapRowToObservation) would improve maintainability and provide a single place to add error handling for corrupt JSON.Example helper
function mapRowToObservation(row: any): Observation { return { ...row, facts: JSON.parse(row.facts), concepts: JSON.parse(row.concepts), filesRead: JSON.parse(row.filesRead), filesModified: JSON.parse(row.filesModified), }; }Also applies to: 73-78, 107-112, 136-141, 179-184, 270-275
plugins/conversation-memory/src/core/observation-prompt.ts (1)
265-270:generateIdusesDate.now()+Math.random()— adequate for observation IDs but not collision-proof under high concurrency.If the observer processes multiple events within the same millisecond,
Date.now()is identical andMath.random()provides ~48 bits of entropy. For this use case (local observation store, not a distributed system), this is acceptable. Consider switching tocrypto.randomUUID()if you want stronger guarantees in the future.
| "PostToolUse": [ | ||
| { | ||
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "node ${CLAUDE_PLUGIN_ROOT}/dist/cli.mjs observe", | ||
| "async": true | ||
| } | ||
| ] | ||
| } | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the routing of 'observe' command in index-cli.ts
echo "=== index-cli.ts observe/observer routing ==="
rg -n "case 'observe'" plugins/conversation-memory/src/cli/index-cli.ts -A 5
echo ""
echo "=== What does observer-cli.ts export/handle? ==="
head -50 plugins/conversation-memory/src/cli/observer-cli.ts 2>/dev/null || echo "File not found"
echo ""
echo "=== What does observe-cli.ts export/handle? ==="
head -50 plugins/conversation-memory/src/cli/observe-cli.ts 2>/dev/null || echo "File not found"
echo ""
echo "=== Check for handlePostToolUse in observe-cli.ts ==="
rg -n "handlePostToolUse\|handleStop" plugins/conversation-memory/src/cli/observe-cli.ts 2>/dev/null || echo "Not found"
echo ""
echo "=== Check index-cli.ts for all command cases ==="
rg -n "^\s*case " plugins/conversation-memory/src/cli/index-cli.tsRepository: baleen37/claude-plugins
Length of output: 3642
🏁 Script executed:
#!/bin/bash
# Verify if observe-cli.ts is imported/routed anywhere in index-cli.ts
echo "=== Is observe-cli.ts imported in index-cli.ts? ==="
rg "observe-cli" plugins/conversation-memory/src/cli/index-cli.ts
echo ""
echo "=== Check if observe-cli.ts is called as a direct module ==="
rg "observe-cli" plugins/conversation-memory/src/cli/ -l
echo ""
echo "=== Full routing in index-cli.ts for observe/observer cases ==="
sed -n '124,130p' plugins/conversation-memory/src/cli/index-cli.tsRepository: baleen37/claude-plugins
Length of output: 458
🏁 Script executed:
#!/bin/bash
# Final verification: check what observer-cli.js actually exports/does with 'observe' as subcommand
echo "=== What happens when observer-cli.js receives 'observe' as subcommand? ==="
sed -n '1,80p' plugins/conversation-memory/src/cli/observer-cli.ts | tail -40
echo ""
echo "=== Check observer-cli.ts for 'observe' case handling ==="
rg -n "case.*observe\|if.*observe" plugins/conversation-memory/src/cli/observer-cli.tsRepository: baleen37/claude-plugins
Length of output: 1094
observe command routes to observer-cli.js which has no handler for it — PostToolUse and Stop hooks will fail.
In index-cli.ts (lines 124-129), the observe command is routed to observer-cli.js, but observer-cli.js only implements handlers for start, stop, status, and observer-run subcommands. The observe subcommand has no case in the switch statement and falls through to the error handler. Meanwhile, observe-cli.js — which contains handlePostToolUse() and the --summarize Stop event logic — is never imported or reached.
This means the PostToolUse hook in hooks.json (line 36) and the Stop hook (line 41) will both fail to queue events to the database, leaving the observer process with nothing to process.
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/hooks/hooks.json` around lines 31 - 41, The
PostToolUse/Stop hooks are invoking the "observe" command but index-cli.ts
routes "observe" to observer-cli.js which has no case for "observe", so
observe-cli.js (and its handlePostToolUse/--summarize Stop logic) is never
executed; fix by adding an "observe" branch in the command switch in
index-cli.ts that imports/dispatches to the module that implements the observe
handlers (the code in observe-cli.js / functions like handlePostToolUse and
summarize stop logic) or alternately change hooks.json to call the existing
subcommands (e.g., a direct "node .../dist/observe-cli.js observe" or the
correct subcommand name); ensure the new branch invokes the same entrypoint used
by observe-cli.js so PostToolUse and Stop events are queued to the DB.
| case 'inject': | ||
| // Inject command - handle directly without dependency check for speed | ||
| await import('./inject-cli.js'); | ||
| break; | ||
|
|
||
| case 'observe': | ||
| case 'observer': | ||
| case 'observer-run': | ||
| // Observer commands - handle directly without dependency check for speed | ||
| await import('./observer-cli.js'); | ||
| break; |
There was a problem hiding this comment.
observe command incorrectly routed to observer-cli.js — should be observe-cli.js.
The observe command (used by PostToolUse and Stop hooks) needs to go to observe-cli.js which handles event queuing. Currently it's routed to observer-cli.js (observer lifecycle management: start/stop/status), meaning PostToolUse events will never be queued.
observer and observer-run correctly route to observer-cli.js.
🐛 Proposed fix
case 'inject':
// Inject command - handle directly without dependency check for speed
await import('./inject-cli.js');
break;
case 'observe':
- case 'observer':
- case 'observer-run':
- // Observer commands - handle directly without dependency check for speed
+ // Observe command (PostToolUse/Stop hooks) - queues events
+ await import('./observe-cli.js');
+ break;
+
+ case 'observer':
+ case 'observer-run':
+ // Observer lifecycle commands (start/stop/status)
await import('./observer-cli.js');
break;🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/cli/index-cli.ts` around lines 119 - 129, The
switch handling for commands incorrectly imports observer-cli for the 'observe'
case; update the case branch so that 'observe' (used by PostToolUse/Stop hooks
and event queuing) imports './observe-cli.js' while leaving 'observer' and
'observer-run' importing './observer-cli.js' unchanged; modify the import
statement in the switch block that currently references './observer-cli.js' for
the 'observe' case to './observe-cli.js' (look for the switch handling the
'inject'/'observe'/'observer'/'observer-run' cases in index-cli.ts).
| const subcommandIndex = process.argv[2] === 'observer' || process.argv[2] === 'observer-run' ? 3 : 2; | ||
| const command = process.argv[subcommandIndex] || 'status'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'index-cli\.ts' -t fRepository: baleen37/claude-plugins
Length of output: 115
🏁 Script executed:
fd 'observer-cli\.ts' -t fRepository: baleen37/claude-plugins
Length of output: 118
🏁 Script executed:
# Read observer-cli.ts to see the actual implementation
wc -l $(fd 'observer-cli\.ts' -t f)Repository: baleen37/claude-plugins
Length of output: 122
🏁 Script executed:
cat -n plugins/conversation-memory/src/cli/observer-cli.tsRepository: baleen37/claude-plugins
Length of output: 3235
🏁 Script executed:
cat -n plugins/conversation-memory/src/cli/index-cli.tsRepository: baleen37/claude-plugins
Length of output: 10267
Critical bug: observer-run command is unreachable — spawned observer defaults to status check instead of running.
When startObserver spawns node dist/cli.mjs observer-run (line 26), the observer-cli module receives process.argv = ['node', 'dist/cli.mjs', 'observer-run']. The condition on line 16 treats 'observer-run' as a prefix expecting a subcommand at argv[3]. Since none exists, command defaults to 'status' (line 17), making the 'observer-run' case at line 71 unreachable. The spawned background process will only check status and exit.
Additionally, line 73 has a Biome noSwitchDeclarations violation: the const statement inside the switch case requires block scope.
Proposed fix
-const subcommandIndex = process.argv[2] === 'observer' || process.argv[2] === 'observer-run' ? 3 : 2;
-const command = process.argv[subcommandIndex] || 'status';
+let command: string;
+if (process.argv[2] === 'observer-run') {
+ command = 'observer-run';
+} else {
+ const subcommandIndex = process.argv[2] === 'observer' ? 3 : 2;
+ command = process.argv[subcommandIndex] || 'status';
+}Also wrap the 'observer-run' case in braces:
case 'observer-run':
+ {
const { startObserver: run } = await import('../core/observer.js');
await run();
break;
+ }🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/cli/observer-cli.ts` around lines 16 - 17,
The bug stems from calculating subcommandIndex by grouping 'observer' and
'observer-run' together; change the ternary so only 'observer' expects a
subcommand (e.g., subcommandIndex = process.argv[2] === 'observer' ? 3 : 2) so
that when argv[2] is 'observer-run' the command variable picks 'observer-run'
instead of defaulting to 'status'; additionally, wrap the 'observer-run' switch
case body in braces and keep the const declarations scoped inside that block to
satisfy Biome's noSwitchDeclarations rule and avoid block-scope issues for the
code that handles the 'observer-run' case.
| facts: JSON.parse(row.facts), | ||
| concepts: JSON.parse(row.concepts), | ||
| filesRead: JSON.parse(row.filesRead), | ||
| filesModified: JSON.parse(row.filesModified), |
There was a problem hiding this comment.
JSON.parse calls on DB fields lack error handling — corrupted data will crash injection.
If any observation row has null, empty string, or malformed JSON in facts, concepts, filesRead, or filesModified, the unguarded JSON.parse will throw and abort the entire injection context generation. This is a production resilience concern since DB data could become inconsistent.
Proposed fix — safe parse with fallback
+ const safeParseArray = (val: string | null): string[] => {
+ if (!val) return [];
+ try { return JSON.parse(val); } catch { return []; }
+ };
+
observationsByDate.get(date)!.push({
id: row.id,
sessionId: row.sessionId,
project: row.project,
timestamp: row.timestamp,
type: row.type,
title: row.title,
subtitle: row.subtitle,
- facts: JSON.parse(row.facts),
- concepts: JSON.parse(row.concepts),
- filesRead: JSON.parse(row.filesRead),
- filesModified: JSON.parse(row.filesModified),
+ facts: safeParseArray(row.facts),
+ concepts: safeParseArray(row.concepts),
+ filesRead: safeParseArray(row.filesRead),
+ filesModified: safeParseArray(row.filesModified),
time
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| facts: JSON.parse(row.facts), | |
| concepts: JSON.parse(row.concepts), | |
| filesRead: JSON.parse(row.filesRead), | |
| filesModified: JSON.parse(row.filesModified), | |
| const safeParseArray = (val: string | null): string[] => { | |
| if (!val) return []; | |
| try { return JSON.parse(val); } catch { return []; } | |
| }; | |
| observationsByDate.get(date)!.push({ | |
| id: row.id, | |
| sessionId: row.sessionId, | |
| project: row.project, | |
| timestamp: row.timestamp, | |
| type: row.type, | |
| title: row.title, | |
| subtitle: row.subtitle, | |
| facts: safeParseArray(row.facts), | |
| concepts: safeParseArray(row.concepts), | |
| filesRead: safeParseArray(row.filesRead), | |
| filesModified: safeParseArray(row.filesModified), | |
| time | |
| }); |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/inject.ts` around lines 69 - 72, The
JSON.parse calls for row.facts, row.concepts, row.filesRead, and
row.filesModified in the observation construction within
plugins/conversation-memory/src/core/inject.ts are unguarded and will throw on
null/empty/malformed values; replace them with a safe parse helper (e.g.,
safeParseJson) or wrap each parse in try/catch returning a sensible default
(empty array or object) so malformed DB data doesn't crash injection; update the
code paths that build the observation objects (the location creating facts,
concepts, filesRead, filesModified) to use this safe parsing helper and ensure
defaults are explicit.
| return `${context} | ||
|
|
||
| <tool_event> | ||
| <tool_name>${toolName}</tool_name> | ||
| <cwd>${cwd}</cwd> | ||
| <project>${project}</project> | ||
| <tool_input>${JSON.stringify(toolInput, null, 2)}</tool_input> | ||
| <tool_response>${escapeXml(toolResponse)}</tool_response> | ||
| </tool_event> |
There was a problem hiding this comment.
toolName, cwd, project, and toolInput are not XML-escaped, unlike toolResponse.
Line 70 correctly escapes toolResponse via escapeXml(), but toolName (Line 66), cwd (Line 67), project (Line 68), and the JSON.stringify(toolInput) output (Line 69) are interpolated raw. If any of these contain characters like <, >, or &, the XML structure sent to the LLM will be malformed, potentially causing observation parsing failures.
Proposed fix
<tool_event>
- <tool_name>${toolName}</tool_name>
- <cwd>${cwd}</cwd>
- <project>${project}</project>
- <tool_input>${JSON.stringify(toolInput, null, 2)}</tool_input>
+ <tool_name>${escapeXml(toolName)}</tool_name>
+ <cwd>${escapeXml(cwd)}</cwd>
+ <project>${escapeXml(project)}</project>
+ <tool_input>${escapeXml(JSON.stringify(toolInput, null, 2))}</tool_input>
<tool_response>${escapeXml(toolResponse)}</tool_response>
</tool_event>🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/observation-prompt.ts` around lines 63 -
71, The XML template in observation-prompt.ts interpolates toolName, cwd,
project, and JSON.stringify(toolInput) without escaping which can break XML;
update the template to pass each interpolated value through the existing
escapeXml function (i.e., replace direct ${toolName}, ${cwd}, ${project}, and
${JSON.stringify(toolInput, null, 2)} with ${escapeXml(...)} ) so that toolName,
cwd, project and the JSON stringified toolInput are XML-escaped before being
inserted into the <tool_event> tags, keeping the already-escaped
${escapeXml(toolResponse)} as-is.
| if (after) validateISODate(after, '--after'); | ||
| if (before) validateISODate(before, '--before'); | ||
|
|
||
| const db = initDatabase(); |
There was a problem hiding this comment.
Database connection is never closed in searchObservations.
initDatabase() is called at line 419 but db.close() is never called. Compare with searchConversations which correctly calls db.close() at line 170. This leaks a database connection on every search call.
Proposed fix: wrap in try/finally
const db = initDatabase();
+ try {
let results: CompactObservationResult[] = [];
// ... existing logic ...
// Apply limit
- return results.slice(0, limit);
+ return results.slice(0, limit);
+ } finally {
+ db.close();
+ }
}Also applies to: 583-584
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/search.ts` at line 419, The call to
initDatabase() in searchObservations opens a DB connection that is never closed;
wrap the database usage in a try/finally so db.close() is always invoked (move
the existing logic that uses db into the try block and put db.close() in
finally), preserving return values and rethrowing errors if necessary; apply the
same try/finally pattern to the other location that also calls initDatabase()
later in the file so every code path ensures db.close() is executed (refer to
initDatabase(), db.close(), and searchObservations to find the spots to change).
| if (after) { | ||
| whereClauses.push('o.timestamp >= ?'); | ||
| whereParams.push(after); | ||
| } | ||
| if (before) { | ||
| whereClauses.push('o.timestamp <= ?'); | ||
| whereParams.push(before); |
There was a problem hiding this comment.
Timestamp type mismatch: date string compared against INTEGER column.
The observations.timestamp column is INTEGER NOT NULL (Unix epoch, per db.ts line 113), but the date filter compares it with an ISO date string like "2025-01-01". SQLite will coerce the comparison, but the semantics are unreliable — an integer like 1735689600 won't meaningfully compare with "2025-01-01".
You need to convert the ISO date to a Unix timestamp for the comparison, e.g.:
if (after) {
whereClauses.push('o.timestamp >= ?');
whereParams.push(new Date(after).getTime());
}Alternatively, use SQLite's date functions, but converting in TypeScript is simpler and more explicit.
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/search.ts` around lines 427 - 433, The
timestamp comparisons in the if (after) and if (before) blocks use ISO date
strings but observations.timestamp is stored as an INTEGER epoch; parse the
provided date strings into a Unix timestamp matching the DB unit before pushing
to whereParams (e.g., const ts = new Date(after).getTime() / 1000 if the DB
stores seconds, or .getTime() for milliseconds), then push ts into whereParams
for the whereClauses; verify the exact epoch unit in db.ts and use that
conversion when updating the if (after) and if (before) blocks referencing
whereClauses and whereParams.
| if (mode === 'vector' || mode === 'both') { | ||
| // Vector similarity search using vec_observations | ||
| await initEmbeddings(); | ||
| const queryEmbedding = await generateEmbedding(query); | ||
|
|
||
| const stmt = db.prepare(` | ||
| SELECT | ||
| o.id, | ||
| o.session_id as sessionId, | ||
| o.project, | ||
| o.timestamp, | ||
| o.type, | ||
| o.title, | ||
| o.subtitle, | ||
| o.facts, | ||
| o.concepts, | ||
| o.files_read as filesRead, | ||
| o.files_modified as filesModified, | ||
| v.distance | ||
| FROM observations o | ||
| INNER JOIN vec_observations v ON o.id = v.id | ||
| ${whereClause} | ||
| ORDER BY v.distance | ||
| LIMIT ? | ||
| `); | ||
|
|
||
| const vectorResults = stmt.all(...whereParams, limit * 2) as any[]; | ||
|
|
||
| for (const row of vectorResults) { | ||
| // Convert distance to similarity (1 - distance for cosine distance) | ||
| const similarity = Math.max(0, 1 - row.distance); | ||
| const boostedSimilarity = applyRecencyBoost(similarity, row.timestamp); | ||
|
|
||
| results.push({ | ||
| id: row.id, | ||
| sessionId: row.sessionId, | ||
| project: row.project, | ||
| timestamp: row.timestamp, | ||
| type: row.type, | ||
| title: row.title, | ||
| subtitle: row.subtitle, | ||
| facts: JSON.parse(row.facts), | ||
| concepts: JSON.parse(row.concepts), | ||
| filesRead: JSON.parse(row.filesRead), | ||
| filesModified: JSON.parse(row.filesModified), | ||
| similarity: boostedSimilarity | ||
| }); | ||
| } |
There was a problem hiding this comment.
Critical: Vector search is missing the MATCH clause — no similarity comparison is performed.
The vec_observations virtual table (sqlite-vec) requires a WHERE v.embedding MATCH ? AND k = ? clause to perform nearest-neighbor search, as done correctly in searchConversations (lines 96-114). Without it, v.distance is undefined/meaningless and the query will likely error or return arbitrary rows.
Proposed fix
const stmt = db.prepare(`
SELECT
o.id,
o.session_id as sessionId,
o.project,
o.timestamp,
o.type,
o.title,
o.subtitle,
o.facts,
o.concepts,
o.files_read as filesRead,
o.files_modified as filesModified,
v.distance
- FROM observations o
- INNER JOIN vec_observations v ON o.id = v.id
- ${whereClause}
- ORDER BY v.distance
- LIMIT ?
+ FROM vec_observations v
+ JOIN observations o ON v.id = o.id
+ WHERE v.embedding MATCH ?
+ AND k = ?
+ ${whereClauses.length > 0 ? 'AND ' + whereClauses.join(' AND ') : ''}
+ ORDER BY v.distance ASC
`);
- const vectorResults = stmt.all(...whereParams, limit * 2) as any[];
+ const vectorResults = stmt.all(
+ Buffer.from(new Float32Array(queryEmbedding).buffer),
+ limit * 2,
+ ...whereParams
+ ) as any[];🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/search.ts` around lines 465 - 512, The
vector-query is missing the sqlite-vec nearest-neighbor MATCH clause so
v.distance is meaningless; update the prepared SQL (used where stmt is built in
the vector branch) to include the vec table's MATCH condition (e.g., "AND
v.embedding MATCH ? AND k = ?"/the same pattern used in searchConversations) and
pass the generated queryEmbedding (and k if required) into whereParams before
executing stmt.all(...whereParams, ...). Keep existing calls to
initEmbeddings(), generateEmbedding(), and applyRecencyBoost(), and ensure
JSON.parse usage for facts/concepts/files* remains unchanged.
| if (mode === 'text' || mode === 'both') { | ||
| // Text-based search using LIKE | ||
| const textStmt = db.prepare(` | ||
| SELECT | ||
| o.id, | ||
| o.session_id as sessionId, | ||
| o.project, | ||
| o.timestamp, | ||
| o.type, | ||
| o.title, | ||
| o.subtitle, | ||
| o.facts, | ||
| o.concepts, | ||
| o.files_read as filesRead, | ||
| o.files_modified as filesModified | ||
| FROM observations o | ||
| ${whereClause} | ||
| AND ( | ||
| o.title LIKE ? OR | ||
| o.subtitle LIKE ? OR | ||
| o.narrative LIKE ? | ||
| ) | ||
| ORDER BY o.timestamp DESC | ||
| LIMIT ? | ||
| `); | ||
|
|
||
| const likeQuery = `%${query}%`; | ||
| const textResults = textStmt.all(...whereParams, likeQuery, likeQuery, likeQuery, limit * 2) as any[]; |
There was a problem hiding this comment.
Critical: Invalid SQL when no filter clauses are active in text search.
When whereClauses is empty, whereClause is '' (line 463). The text query at lines 531-536 becomes:
FROM observations o
AND (
o.title LIKE ? OR ...This is a syntax error — AND without a preceding WHERE.
Proposed fix
const textStmt = db.prepare(`
SELECT
o.id,
o.session_id as sessionId,
o.project,
o.timestamp,
o.type,
o.title,
o.subtitle,
o.facts,
o.concepts,
o.files_read as filesRead,
o.files_modified as filesModified
FROM observations o
- ${whereClause}
- AND (
+ WHERE ${whereClauses.length > 0 ? whereClauses.join(' AND ') + ' AND' : ''} (
o.title LIKE ? OR
o.subtitle LIKE ? OR
o.narrative LIKE ?
)
ORDER BY o.timestamp DESC
LIMIT ?
`);🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/search.ts` around lines 515 - 542, The
text search SQL in search.ts constructs `${whereClause}` which can be empty,
then blindly appends `AND (...)`, causing invalid SQL when there is no preceding
WHERE; update the SQL assembly in the text search block (the prepared statement
that creates textStmt) to conditionally insert either `WHERE` or `AND` before
the `(...)` depending on whether `whereClause` is non-empty (e.g. use
`whereClause ? \`${whereClause} AND (...)\` : \`WHERE (...)\``) so the query is
valid when `whereClauses` is empty; ensure the parameters passed to
`textStmt.all(...whereParams, likeQuery, likeQuery, likeQuery, limit * 2)` still
align with the resulting SQL.
| // Single-concept search (use observations) | ||
| const options: ObservationSearchOptions = { | ||
| mode: params.mode, | ||
| limit: params.limit, | ||
| after: params.after, | ||
| before: params.before, | ||
| projects: params.projects, | ||
| types: (args as any).types, | ||
| concepts: (args as any).concepts, | ||
| files: (args as any).files, | ||
| }; |
There was a problem hiding this comment.
Critical: types, concepts, files params will be rejected by the strict Zod schema.
SearchInputSchema (lines 32-73) uses .strict() and does not include types, concepts, or files properties. When a client sends these parameters, SearchInputSchema.parse(args) at line 232 will throw a Zod error for unrecognized keys — they never reach lines 269-271.
The Zod schema must be updated to match the JSON Schema exposed in the tool definition (lines 160-162):
Proposed fix: add missing fields to SearchInputSchema
const SearchInputSchema = z
.object({
query: z
.union([
z.string().min(2, 'Query must be at least 2 characters'),
z
.array(z.string().min(2))
.min(2, 'Must provide at least 2 concepts for multi-concept search')
.max(5, 'Cannot search more than 5 concepts at once'),
])
.describe(
'Search query - string for single concept, array of strings for multi-concept AND search'
),
mode: SearchModeEnum.default('both').describe(
'Search mode: "vector" for semantic similarity, "text" for exact matching, "both" for combined (default: "both"). Only used for single-concept searches.'
),
limit: z
.number()
.int()
.min(1)
.max(50)
.default(10)
.describe('Maximum number of results to return (default: 10)'),
after: z
.string()
.regex(/^\d{4}-\d{2}-\d{2}$/, 'Date must be in YYYY-MM-DD format')
.optional()
.describe('Only return conversations after this date (YYYY-MM-DD format)'),
before: z
.string()
.regex(/^\d{4}-\d{2}-\d{2}$/, 'Date must be in YYYY-MM-DD format')
.optional()
.describe('Only return conversations before this date (YYYY-MM-DD format)'),
projects: z
.array(z.string().min(1))
.optional()
.describe('Filter results to specific project names'),
+ types: z
+ .array(z.string().min(1))
+ .optional()
+ .describe('Filter by observation types (single-concept only)'),
+ concepts: z
+ .array(z.string().min(1))
+ .optional()
+ .describe('Filter by tagged concepts (single-concept only)'),
+ files: z
+ .array(z.string().min(1))
+ .optional()
+ .describe('Filter by files mentioned or modified (single-concept only)'),
response_format: ResponseFormatEnum.default('markdown').describe(
'Output format: "markdown" for human-readable or "json" for machine-readable (default: "markdown")'
),
})
.strict();Then replace the (args as any) casts:
- types: (args as any).types,
- concepts: (args as any).concepts,
- files: (args as any).files,
+ types: params.types,
+ concepts: params.concepts,
+ files: params.files,🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/mcp/server.ts` around lines 262 - 272,
SearchInputSchema is strict but missing the types, concepts, and files fields
that clients send, so SearchInputSchema.parse(args) will reject requests; update
SearchInputSchema to include types: z.array(z.string()).optional(), concepts:
z.array(z.string()).optional(), and files: z.array(z.string()).optional() (or
the correct types per JSON tool schema), then remove the (args as any) casts in
the ObservationSearchOptions construction and use args.types, args.concepts, and
args.files directly so ObservationSearchOptions (used in the single-concept
search block and passed to ObservationSearchOptions) receives the parsed values.
Fix test failure by adding required "matcher" field to PostToolUse and Stop hook entries. The hooks.json schema requires all hook entries to have both "matcher" and "hooks" fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Complete Phase 4 of the observation system implementation - documentation updates and deprecation notices for the legacy exchange-based search.
Changes
searchMultipleConcepts(v6.0 → v7.0 removal)Phases Complete
✅ Phase 1: DB schema + pending_events queue + observer process + PostToolUse hook
✅ Phase 2: Stop hook (session summary) + SessionStart inject hook
✅ Phase 3: search/get_observations MCP tools + Progressive Disclosure
✅ Phase 4: Agent/skill updates + exchange search deprecation
All 4 phases of the observation system design are now implemented, documented, and ready for use.
Files Modified
Testing
🤖 Generated with Claude Code
Summary by CodeRabbit