feat(conversation-memory): restore conversation-memory plugin - #425
Conversation
Restore the full conversation-memory plugin that was previously migrated to the me plugin. This restores local semantic search capabilities using Transformers.js, SQLite with vector embeddings, and the complete MCP server implementation. Changes: - Restore full plugins/conversation-memory/ directory from commit 6f6f91e - Add conversation-memory back to marketplace.json - Restore TypeScript source code, MCP server, CLI tools, hooks, and tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a new "conversation-memory" plugin and marketplace entry: plugin manifests, MCP server (search/read tools), CLI and build/runtime wrappers, embedding-backed SQLite vector index, parsing/indexing/summarization/sync pipelines, logging/paths utilities, extensive docs, and many tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Agent
participant CLI as CLI (sync/index)
participant Archive as Archive Dir
participant Parser as Parser
participant Embeddings as Embedding Model
participant DB as SQLite+Vec
participant MCP as MCP Server
participant Search as Search Module
rect rgba(100, 150, 200, 0.5)
Note over User,DB: Sync & Index Flow
User->>CLI: run sync/index command
CLI->>Archive: copy newer conversation files
Archive-->>CLI: file paths
CLI->>Parser: parse JSONL -> exchanges
Parser-->>CLI: exchanges + metadata
CLI->>Embeddings: generate embeddings per exchange
Embeddings-->>CLI: vectors
CLI->>DB: insert exchanges + vectors
DB-->>DB: store rows & vec entries
end
rect rgba(150, 100, 200, 0.5)
Note over User,Search: Search & Retrieval Flow
User->>MCP: send search request
MCP->>Search: validate + invoke searchConversations
Search->>Embeddings: generate embedding for query
Embeddings-->>Search: query vector
Search->>DB: vector/text query -> ranked rows
DB-->>Search: results + snippets
Search-->>MCP: formatted results
MCP-->>User: return search/read response
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 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)
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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@plugins/conversation-memory/src/core/db.ts`:
- Around line 132-199: The insertExchange function performs multiple writes
without a transaction and declares an unused parameter toolNames; fix by
removing the toolNames parameter from insertExchange and its callers (sync.ts,
verify.ts, indexer.ts locations mentioned) and wrap all DB operations in a
single transaction using db.transaction or db.exec("BEGIN"/"COMMIT"/"ROLLBACK")
around the sequence: the stmt.run(...) upsert into exchanges, the
delStmt.run(...) + vecStmt.run(...) for vec_exchanges, and the tool_calls
inserts (toolStmt.run(...) loop) so either all succeed or all roll back; ensure
you convert boolean flags the same way (1/0) and still use Buffer.from(new
Float32Array(embedding).buffer) for the embedding.
In `@plugins/conversation-memory/src/core/embeddings.ts`:
- Around line 5-21: initEmbeddings can be raced by concurrent callers because
they all check embeddingPipeline and may start pipeline(...) in parallel; fix by
adding a shared loading promise (e.g., embeddingPipelineLoadingPromise) that is
set before awaiting pipeline and awaited by subsequent callers so only the first
caller performs the load; update initEmbeddings to set
embeddingPipelineLoadingPromise when starting the load, await it to assign
embeddingPipeline on success (and clear the promise on failure), and ensure
generateEmbedding awaits initEmbeddings or embeddingPipelineLoadingPromise
before using embeddingPipeline.
In `@plugins/conversation-memory/src/core/indexer.ts`:
- Around line 257-365: indexUnprocessed currently leaks the DB on exceptions and
builds an in-memory list of all exchanges (unprocessed[]) which can blow memory;
wrap DB usage in a try/finally so db.close() is always called (move
initDatabase() result into a variable and ensure db.close() in finally), and
stop accumulating full ConversationExchange[] arrays by processing each
conversation immediately after parsing: after parseConversation(project,
sourcePath, archivePath) handle summary (use processBatch or per-conv
summarizeConversation) and then call generateExchangeEmbedding + insertExchange
per exchange right away instead of pushing into unprocessed; keep processBatch
for concurrent summaries but change it to accept single-conversation work so you
never retain all exchanges in memory.
- Around line 185-255: indexSession creates the DB via initDatabase() but calls
db.close() only at the end, so if generateExchangeEmbedding or insertExchange
throws the DB is leaked; wrap the DB lifetime in a try/finally (immediately
after const db = initDatabase()) and move db.close() into finally to ensure
closure even on errors, keeping the rest of the logic (summarizeConversation,
generateExchangeEmbedding, insertExchange) inside the try block; reference
functions/variables: indexSession, initDatabase, db.close,
generateExchangeEmbedding, insertExchange.
- Around line 41-183: The indexConversations function can leak the DB handle if
generateExchangeEmbedding or insertExchange throws; wrap the main processing
logic (after const db = initDatabase()) in a try/finally so db.close() is always
called. Specifically, enclose the loop that iterates projects/toProcess and the
summarization/embedding steps (where generateExchangeEmbedding and
insertExchange are called) in a try block and call db.close() in the finally
block; remove or keep the existing db.close() calls inside early-return paths
but ensure they are not duplicating cleanup (or only let finally handle close).
Ensure the finally runs even on exceptions so the DB handle opened in
indexConversations is always closed.
In `@plugins/conversation-memory/src/core/search.ts`:
- Around line 42-74: The vector-path SQL currently injects
`${after}`/`${before}` and attempts to use `e.timestamp` in the KNN query (via
`timeClause`), which is unsafe and ineffective; update the `mode === 'vector'`
branch (functions/vars: initEmbeddings, generateEmbedding, stmt, vec_exchanges,
exchanges, timeClause) to: (1) remove the `${after}/${before}` interpolation
from the vec KNN SQL and use only parameter placeholders for the KNN inputs
(embedding buffer and `limit`/`k`), (2) run the KNN query without time
constraints, then perform timestamp filtering in application code on the
returned rows (use the existing validateISODate to parse/verify `after`/`before`
and compare against `e.timestamp`), and (3) only after filtering apply the
result `limit`; alternatively, if you prefer server-side filtering for
performance, add a denormalized timestamp metadata column to `vec_exchanges` and
constrain that column in the KNN SQL using parameterized placeholders instead of
interpolated strings.
In `@plugins/conversation-memory/src/core/show.ts`:
- Around line 278-288: The toolUseMap populated in the top-level loop (variable
toolUseMap, iterating messages and assistant message content) is never read
later, so remove the dead-code: delete the Map creation and the for-loop that
sets toolUseMap, or alternatively refactor the downstream lookup logic (the code
handling tool result lookup in the Markdown formatter / the block between lines
~407-441) to consult toolUseMap instead of re-scanning messages; update any
references to rely on the single source (either the removed map or the new
map-based lookup) and ensure no unused variables remain (look for toolUseMap,
msg/block iteration and the lookup code in the Markdown formatter to reconcile).
- Around line 360-375: The assistant message rendering injects raw HTML from
renderMarkdownSafely (which calls marked.parse) without sanitization; update
renderMarkdownSafely to sanitize the HTML output before returning it (e.g.,
import and use isomorphic-dompurify/DOMPurify.sanitize on the string returned by
marked.parse or enable marked's sanitizer hook), and ensure both branches that
call renderMarkdownSafely (the Array block handling and the string content
branch if applicable) use the sanitized output; add the DOMPurify import and
typings as needed and keep escapeHtml for plain-text branches to avoid
double-escaping.
In `@plugins/conversation-memory/src/core/summarizer.ts`:
- Around line 174-201: The isTrivialConversation function miscounts CJK text
because the split regex treats CJK ideographs as delimiters; update the logic to
split only on whitespace for Latin scripts and separately count CJK characters
as words: compute wordCount = number of non-empty tokens from splitting on /\s+/
plus the count of CJK characters matched with a regex like
/[\u4E00-\u9FFF\u3400-\u4DBF\uF900-\uFAFF\uAC00-\uD7AF\u3040-\u309F\u30A0-\u30FF]/g,
then use that combined count for the "< 15" trivial check in
isTrivialConversation so substantive CJK messages are not misclassified.
In `@plugins/conversation-memory/src/core/sync.ts`:
- Around line 162-200: initEmbeddings() can throw after db is created via
initDatabase(), leaking the DB handle; wrap the work that uses db in a
try/finally so db.close() always runs. Specifically, after calling const db =
initDatabase() move the rest of the indexing logic (including await
initEmbeddings(), the for loop over filesToIndex, and result updates) into a try
block and call db.close() inside a finally block; ensure insertExchange(db, ...)
and any early continues still result in db.close() by keeping those inside the
try.
- Around line 13-21: The function shouldSkipConversation currently reads entire
files with fs.readFileSync and uses an exclusion marker that never appears in
files; update it to read only the first chunk (e.g., fs.createReadStream with a
small highWaterMark or fs.open + read a fixed byte range) and search that chunk
for markers instead of loading the whole file, remove SUMMARIZER_CONTEXT_MARKER
from EXCLUSION_MARKERS so only actual file markers remain, and eliminate the
duplicated sumTokenUsage by moving the implementation from sync.ts and
summarizer.ts into a shared utility module (export/import the single
sumTokenUsage utility wherever needed).
In `@plugins/conversation-memory/src/core/verify.ts`:
- Line 50: The verifyIndex code currently builds files with const files =
fs.readdirSync(projectPath).filter(...) but only filters by .jsonl, causing
agent-*.jsonl files to be falsely flagged; update the filter used in verifyIndex
(the files variable) to match sync.ts by excluding files that start with
"agent-" (i.e., filter to f => f.endsWith('.jsonl') && !f.startsWith('agent-'))
so verifyIndex and sync.ts use the same selection logic.
In `@plugins/conversation-memory/src/mcp/server.ts`:
- Around line 250-274: The read handler currently reads params.path directly
(ShowConversationInputSchema, params.path, formatConversationAsMarkdown)
allowing path traversal; fix by resolving the absolute path and ensuring it is
inside the allowed archive directory before reading: compute a resolvedPath via
path.resolve(archiveDir, params.path) (or use path.resolve and path.relative)
and check that the resolvedPath is within archiveDir (reject if it escapes with
.. or if path.relative indicates traversal or the prefix doesn't match), keep
the fs.existsSync and subsequent read only after this validation, and throw a
clear error when the path is outside the archive.
🟡 Minor comments (17)
plugins/conversation-memory/README.md-13-13 (1)
13-13:⚠️ Potential issue | 🟡 MinorInconsistency: "SessionEnd" vs "SessionStart" hook.
Line 13 says "Automatic Indexing: SessionEnd hook syncs conversations automatically" but Line 134's section heading says "Automatic Indexing (SessionStart Hook)" and Line 136 says "When each Claude Code session starts (startup or resume)." These contradict each other — one of them needs to be corrected to match the actual
hooks.jsonconfiguration.plugins/conversation-memory/README.md-117-124 (1)
117-124:⚠️ Potential issue | 🟡 MinorIncorrect directory path in installation instructions.
Line 119 says
cd plugins/memorybut the actual plugin directory isplugins/conversation-memory.📝 Proposed fix
# Install dependencies -cd plugins/memory +cd plugins/conversation-memory npm installplugins/conversation-memory/src/core/stats.ts-109-113 (1)
109-113:⚠️ Potential issue | 🟡 MinorDivision by zero if
totalConversationsis 0.
formatStatsis a public function. If called withtotalConversations: 0andconversationsWithoutSummaries > 0(e.g., from a manually constructedIndexStats), this division producesNaN. Guard against it or gate the condition ontotalConversations > 0as well.🛡️ Proposed fix
- if (stats.conversationsWithoutSummaries > 0) { + if (stats.conversationsWithoutSummaries > 0 && stats.totalConversations > 0) { const percentage = ((stats.conversationsWithoutSummaries / stats.totalConversations) * 100).toFixed(1);plugins/conversation-memory/src/core/search.ts-77-110 (1)
77-110:⚠️ Potential issue | 🟡 MinorText search LIKE pattern doesn't escape wildcards in user input.
On Line 97,
%${query}%is passed to aLIKEclause. If the user's query contains%or_characters, the LIKE pattern will produce unexpected results (e.g.,%matches any sequence). This is a minor correctness gap.plugins/conversation-memory/src/core/db.ts-217-223 (1)
217-223:⚠️ Potential issue | 🟡 Minor
deleteExchangedoes not delete associatedtool_calls, leaving orphan rows.The function deletes from
vec_exchangesandexchangesbut not fromtool_calls. Sincetool_calls.exchange_idreferencesexchanges.idvia a foreign key, and SQLite doesn't enforceON DELETE CASCADEby default (andPRAGMA foreign_keysis not enabled), orphan tool_call rows will accumulate.🛠️ Proposed fix
export function deleteExchange(db: Database.Database, id: string): void { + // Delete tool calls first (child rows) + db.prepare(`DELETE FROM tool_calls WHERE exchange_id = ?`).run(id); + // Delete from vector table db.prepare(`DELETE FROM vec_exchanges WHERE id = ?`).run(id); // Delete from main table db.prepare(`DELETE FROM exchanges WHERE id = ?`).run(id); }plugins/conversation-memory/src/core/search.ts-114-143 (1)
114-143:⚠️ Potential issue | 🟡 Minor
similarityis set toundefinedfor text-mode results, violatingSearchResult.similarity: number.Line 139 sets
similarity: mode === 'text' ? undefined : 1 - row.distance, but theSearchResultinterface (fromtypes.tsLine 41) declaressimilarity: number. Theas SearchResultcast masks this type error at compile time, but consumers may encounterundefinedat runtime when they expect a number.🛠️ Suggested fix
Either update the
SearchResultinterface to allowsimilarity?: number, or provide a fallback:- similarity: mode === 'text' ? undefined : 1 - row.distance, + similarity: mode === 'text' ? 0 : 1 - row.distance,plugins/conversation-memory/src/cli/search-cli.ts-53-62 (1)
53-62:⚠️ Potential issue | 🟡 MinorMissing bounds checks for option arguments —
--after,--before,--limitwithout a value will silently misbehave.When
--after,--before, or--limitis the last argument (or followed by another flag),args[++i]evaluates toundefined. This causes:
--after/--before:undefinedpassed as date → confusing error fromvalidateISODate--limit:parseInt(undefined)→NaN→ SQLLIMIT NaNwhich may return no results or error🛠️ Proposed fix — validate option values exist
} else if (arg === '--after') { - after = args[++i]; + if (i + 1 >= args.length) { console.error('--after requires a YYYY-MM-DD value'); process.exit(1); } + after = args[++i]; } else if (arg === '--before') { - before = args[++i]; + if (i + 1 >= args.length) { console.error('--before requires a YYYY-MM-DD value'); process.exit(1); } + before = args[++i]; } else if (arg === '--limit') { - limit = parseInt(args[++i]); + if (i + 1 >= args.length) { console.error('--limit requires a number'); process.exit(1); } + limit = parseInt(args[++i], 10); + if (isNaN(limit)) { console.error('--limit must be a valid number'); process.exit(1); }plugins/conversation-memory/src/cli/search-cli.ts-43-43 (1)
43-43:⚠️ Potential issue | 🟡 Minor
--bothflag is shown in help but not handled in arg parsing — it becomes a query term.Line 43 shows
conversation-memory search --both "React Router data loading"as an example, but the argument parser has no case for--both. Since'both'is the default mode, the flag is silently added toqueries[], corrupting the search.🛠️ Proposed fix
} else if (arg === '--text') { mode = 'text'; + } else if (arg === '--both') { + mode = 'both'; } else if (arg === '--after') {plugins/conversation-memory/tests/mcp-server.bats-4-9 (1)
4-9:⚠️ Potential issue | 🟡 Minor
cli.mjstests aren't guarded by the setup skip — they'll fail instead of skipping when cli.mjs is missing.The
setup()only checks fordist/mcp-server.mjs. If the build partially succeeds (mcp-server built, cli not), thecli.mjstests at lines 28-35 will fail rather than gracefully skip. Consider adding a guard forcli.mjsas well, or generalizing the check.Proposed fix
setup() { # Ensure build artifacts exist if [ ! -f "${BATS_TEST_DIRNAME}/../dist/mcp-server.mjs" ]; then skip "MCP server not built - run 'npm run build' first" fi + if [ ! -f "${BATS_TEST_DIRNAME}/../dist/cli.mjs" ]; then + skip "CLI not built - run 'npm run build' first" + fi }Also applies to: 28-30
plugins/conversation-memory/tests/cli/ensure-dependencies.bats-34-59 (1)
34-59:⚠️ Potential issue | 🟡 MinorThis test is a no-op — it verifies that doing nothing changes nothing.
Lines 48-49 explicitly state "We simulate this by doing nothing." The test creates a directory, sleeps, then confirms the timestamp hasn't changed — without ever invoking
cli.mjsorensureDependencies. This will always pass regardless of the actual implementation.Either invoke
ensureDependencies(e.g., vianode dist/cli.mjswith appropriate args/env) to confirm it skipsnpm installwhennode_modulesexists, or remove this test to avoid false confidence.plugins/conversation-memory/.claude-plugin/plugin.json-1-8 (1)
1-8:⚠️ Potential issue | 🟡 MinorVersion mismatch: plugin.json declares
5.1.1but marketplace.json declares5.1.2.The plugin manifest here says
"version": "5.1.1"while the marketplace entry at.claude-plugin/marketplace.json(line 135) lists"version": "5.1.2". These should be consistent — pick one and update the other.plugins/conversation-memory/src/cli/show-cli.ts-13-14 (1)
13-14:⚠️ Potential issue | 🟡 MinorMissing validation for
--formatvalue — invalid or missing values are silently treated as markdown.If
--formatis the last arg,args[++i]isundefined, and theascast hides the problem. If the user passes--format pdf, it's silently accepted and defaults to markdown. Consider validating:Proposed fix
if (arg === '--format' || arg === '-f') { - format = args[++i] as 'markdown' | 'html'; + const value = args[++i]; + if (value !== 'markdown' && value !== 'html') { + console.error(`Error: Invalid format '${value ?? ''}'. Must be 'markdown' or 'html'.`); + process.exit(1); + } + format = value; } else if (arg === '--help' || arg === '-h') {plugins/conversation-memory/src/core/show.ts-37-37 (1)
37-37:⚠️ Potential issue | 🟡 Minor
JSON.parseper line without error handling — single malformed line crashes formatting.Both
formatConversationAsMarkdown(line 37) andformatConversationAsHTML(line 225) parse all lines withlines.map(line => JSON.parse(line)). A single malformed JSON line will throw and abort the entire operation, even if the MCPreadtool catches the outer error.Wrap in try/catch to skip bad lines gracefully.
🔧 Proposed fix for both functions
- const allMessages: ConversationMessage[] = lines.map(line => JSON.parse(line)); + const allMessages: ConversationMessage[] = []; + for (const line of lines) { + try { + allMessages.push(JSON.parse(line)); + } catch { + // Skip malformed JSONL lines + } + }Also applies to: 225-225
plugins/conversation-memory/src/cli/index-cli.ts-51-51 (1)
51-51:⚠️ Potential issue | 🟡 MinorNon-English comment.
Line 51 has a Korean comment (
모든 명령 실행 전). Replace with English for consistency.📝 Suggested fix
- // Ensure dependencies are installed (모든 명령 실행 전) + // Ensure dependencies are installed (before all commands)plugins/conversation-memory/commands/search-conversation.md-78-81 (1)
78-81:⚠️ Potential issue | 🟡 MinorCapitalize "Markdown" as a proper noun.
Line 78 and 81 use lowercase
markdownwhen referring to the Markdown formatting language. The format enum valuemarkdownis fine in the code context (line 78), but the prose on line 81 should capitalize it.📝 Suggested fix
-Always use markdown format unless the user specifically requests JSON. +Always use Markdown format unless the user specifically requests JSON.plugins/conversation-memory/src/cli/index-cli.ts-133-136 (1)
133-136:⚠️ Potential issue | 🟡 MinorDefault case silently runs
index-allfor unknown commands.If a user passes an unrecognized command (e.g., a typo like
vreify), it will silently index all conversations instead of showing an error. Consider printing usage info for unrecognized commands.🔧 Proposed fix
case 'index-all': - default: await indexConversations(undefined, undefined, concurrency, noSummaries); break; + + default: + console.error(`Unknown command: ${command}`); + console.error('Available commands: index-session, index-cleanup, verify, repair, rebuild, index-all'); + process.exit(1); }plugins/conversation-memory/src/cli/index-cli.ts-54-136 (1)
54-136:⚠️ Potential issue | 🟡 MinorWrap
switchcase bodies in blocks to prevent variable leakage.Biome correctly flags
const/letdeclarations incaseclauses without block scoping. Variables likesessionId,issues,repairIssues,dbPath, andarchiveDirare technically accessible from other cases.🔧 Proposed fix (showing two cases; apply the same pattern to all)
switch (command) { - case 'index-session': - const sessionId = process.argv[3]; + case 'index-session': { + const sessionId = process.argv[3]; if (!sessionId) { console.error('Usage: index-cli index-session <session-id>'); process.exit(1); } await indexSession(sessionId, concurrency, noSummaries); break; + } - case 'verify': - console.log('Verifying conversation index...'); - const issues = await verifyIndex(); + case 'verify': { + console.log('Verifying conversation index...'); + const issues = await verifyIndex(); // ... rest of case break; + }
🧹 Nitpick comments (24)
plugins/conversation-memory/src/core/stats.ts (1)
22-23: Duplicate dynamic import offs.
fsis dynamically imported at Line 22 and again at Line 55 inside thetryblock. The innerconst fsshadows the outer one. The second import is unnecessary — just remove it and let thetryblock use the outerfs.♻️ Proposed fix
// Check for summaries (these are files, not DB fields) - const fs = await import('fs'); const conversationPaths = db.prepare('SELECT DISTINCT archive_path FROM exchanges').all() as Array<{ archive_path: string }>;Also applies to: 55-55
plugins/conversation-memory/src/core/embeddings.ts (1)
9-10: Relative cache dir is CWD-dependent.
env.cacheDir = './.cache'resolves relative to the working directory at runtime. If the CLI or MCP server is invoked from different directories, model artifacts will be downloaded to separate.cachefolders. Consider using an absolute path derived from yourpaths.tsutilities (e.g., undergetSuperpowersDir()) so the cache is shared across invocations.plugins/conversation-memory/src/core/summarizer.test.ts (1)
4-12: Test exercises a local copy, not the actualchunkExchangesimplementation.The test duplicates
chunkExchangesat lines 6-12 instead of importing it fromsummarizer.tswhere it's defined at line 162. SincechunkExchangesis not exported, these tests document current behavior but provide zero regression coverage—if the real implementation drifts, these tests will still pass. ExportchunkExchangesfrom the summarizer module and import it in the test for meaningful coverage.plugins/conversation-memory/src/core/logger.ts (1)
70-85:logErrorsilently records{ error: undefined }when called without an error argument.When
logError('msg')is called without theerrorparameter,errorDataisundefinedand the log entry'sdatabecomes{ error: undefined }, which serializes to{}in JSON. This is harmless but could be confusing when reading logs — no error info is recorded yet the field exists. Consider guarding:♻️ Proposed refinement
- const entry: LogEntry = { - timestamp: new Date().toISOString(), - level: LogLevel.ERROR, - message, - data: data ? { ...data, error: errorData } : { error: errorData } - }; + const entry: LogEntry = { + timestamp: new Date().toISOString(), + level: LogLevel.ERROR, + message, + data: data || errorData ? { ...data, ...(errorData != null && { error: errorData }) } : undefined + };plugins/conversation-memory/src/core/search.ts (1)
227-287:searchMultipleConceptsopens/closes the database N times (once per concept).Each call to
searchConversations(Line 239) internally callsinitDatabase()anddb.close(). For multi-concept searches, this opens and closes the database for every concept. While functionally correct, it's wasteful. Consider accepting a shareddbhandle or deferring this to a future optimization.plugins/conversation-memory/src/core/summarizer.ts (2)
112-124:as anycast on SDK options hides potential type mismatches.Line 124 casts the entire options object to
any, suppressing type checking for fields likeenv,resume, andsystemPrompt. If the SDK's API changes, this will silently break at runtime.Consider narrowing the cast or extending the SDK types if additional options are needed, rather than casting the entire object.
7-19: Global mutable token tracking is fragile under concurrent use.
currentRunTokenUsagesis module-level mutable state shared across all callers. IfsummarizeConversationis ever called concurrently (e.g., parallel indexing), token accounting will be corrupted. Currently safe if the hook runs serially, but worth noting as a maintainability concern.plugins/conversation-memory/tests/mcp-server.bats (1)
15-20: Shebang test only assertsheadexits successfully — it doesn't validate content.
head -1on any non-empty file returns status 0, so this test will pass regardless of whether a shebang is present. If the intent is to verify a Node.js shebang, check the output content:Proposed fix
`@test` "mcp-server.mjs: has valid Node.js shebang or can be executed with node" { - # Check if file starts with shebang or is valid JS run head -1 "${BATS_TEST_DIRNAME}/../dist/mcp-server.mjs" - # Either has shebang or is valid JS (we'll test execution next) [ "$status" -eq 0 ] + [[ "$output" == "#!/usr/bin/env node" ]] || [[ "$output" == "#!/usr/bin/node" ]] }plugins/conversation-memory/tests/cli/ensure-dependencies.bats (1)
23-32: Test doesn't exercisecli.mjs— it only testsmkdir -pand[ -d ... ].Despite the test name referencing
cli.mjs: ensureDependencies, no CLI code is invoked. The test just creates a directory and verifies it exists. Consider invoking the actualensureDependencieslogic to provide meaningful coverage.plugins/conversation-memory/src/core/indexer.test.ts (1)
1-68: Extract the repeated file-filtering logic as a named export to avoid duplication and enable proper testing.This test file duplicates the file filter logic inline across all test cases rather than importing and testing the actual filtering implementation from
indexer.ts. The same pattern appears multiple times throughout the production code as well (indexer.ts lines 81, 201, 288 and sync.ts line 128), making this an opportunity to improve both test isolation and code maintainability.Extract the filter predicate as a reusable function (e.g.,
isConversationFile) in the core module and export it, then import and use it in both the production code and tests. This ensures tests validate the actual implementation and eliminates the repeated logic pattern.plugins/conversation-memory/scripts/mcp-server-wrapper.mjs (2)
19-129: Consider extracting shared spawn-and-diagnose logic into a helper.
runNpmInstallandrunBuildfollow the exact same pattern: spawn npm, collect stderr, resolve/reject on exit, diagnose errors (EACCES/ENOSPC/network), and handle spawn errors. The only differences are the npm arguments and a couple of log messages.A single
runNpmCommand(args, label)helper would eliminate the duplication.♻️ Sketch of a unified helper
-function runNpmInstall() { - return new Promise((resolve, reject) => { - const isWindows = process.platform === 'win32'; - const npmCommand = isWindows ? 'npm.cmd' : 'npm'; - console.error('[conversation-memory] Installing dependencies (first run only)...'); - ... - }); -} - -function runBuild() { - return new Promise((resolve, reject) => { - const isWindows = process.platform === 'win32'; - const npmCommand = isWindows ? 'npm.cmd' : 'npm'; - console.error('[conversation-memory] Building plugin...'); - ... - }); -} +function runNpm(args, { startMsg, successMsg, failMsg, fallbackCmd }) { + return new Promise((resolve, reject) => { + const isWindows = process.platform === 'win32'; + const npmCommand = isWindows ? 'npm.cmd' : 'npm'; + console.error(`[conversation-memory] ${startMsg}`); + + let stderrOutput = ''; + const child = spawn(npmCommand, args, { + cwd: PLUGIN_ROOT, + stdio: ['ignore', 'pipe', 'pipe'], + shell: isWindows, + }); + + child.stdout.on('data', (data) => process.stderr.write(data)); + child.stderr.on('data', (data) => { stderrOutput += data.toString(); process.stderr.write(data); }); + + child.on('exit', (code) => { + if (code === 0) { console.error(`[conversation-memory] ${successMsg}`); resolve(); } + else { diagnoseError(stderrOutput, failMsg, fallbackCmd); reject(new Error(`${fallbackCmd} failed (${code})`)); } + }); + child.on('error', (err) => { console.error(`[conversation-memory] ERROR: ${err.message}`); reject(err); }); + }); +}
131-158: Build staleness check only considerspackage.jsonmtime.Source file changes (under
src/) won't trigger a rebuild—onlypackage.jsonupdates will. This is intentional per the comment ("version update"), but worth confirming it matches the expected developer workflow. If the wrapper is also the primary entry-point during development, stale builds may confuse contributors.plugins/conversation-memory/src/cli/sync-cli.ts (1)
44-59: Background mode silently swallows failures.With
stdio: 'ignore', if the background sync process fails (dependency install failure, embedding error, etc.), there's zero user-visible feedback. Consider redirecting stdout/stderr to a log file so failures are at least diagnosable after the fact.♻️ Example: redirect to a log file
+import { openSync } from 'fs'; +import { getArchiveDir } from '../core/paths.js'; + // Spawn a detached process +const logPath = path.join(getArchiveDir(), 'sync.log'); +const logFd = openSync(logPath, 'a'); const child = spawn(process.execPath, [ process.argv[1], // This script ...filteredArgs ], { detached: true, - stdio: 'ignore' + stdio: ['ignore', logFd, logFd] });plugins/conversation-memory/src/mcp/server.ts (1)
30-67: Dual schema definitions will drift — consider generating one from the other.The Zod
SearchInputSchema(lines 30-67) and the JSON Schema in the tool listing (lines 126-142) encode the same constraints. If one is updated without the other, validation will silently diverge. Libraries likezod-to-json-schemacan derive the JSON Schema from Zod at runtime.Also applies to: 119-149
plugins/conversation-memory/src/cli/index-cli.ts (1)
12-29: Inconsistentnpm installflags across the plugin.This file uses
--legacy-peer-deps --silent(line 19), whilemcp-server-wrapper.mjsuses--prefer-offline --no-audit --no-fund(line 29). Different flags can produce different dependency trees. Consider aligning them, or extracting shared install logic.plugins/conversation-memory/src/core/verify.ts (1)
64-68:continueon missing summary skips corruption check.When a summary file is missing, the
continueskips both the outdated check and the corruption parse. A file could be both missing its summary and corrupted, in which case the repair would attempt to re-index a corrupted file and fail. Detecting corruption upfront would be more robust.♻️ Suggested approach: don't skip corruption check
// Check for missing summary if (!fs.existsSync(summaryPath)) { result.missing.push({ path: conversationPath, reason: 'No summary file' }); - continue; + // Don't continue — still check for corruption below }plugins/conversation-memory/src/core/show.ts (1)
26-221: Significant code duplication between Markdown and HTML formatters.Message filtering (lines 40-52 / 228-248), sidechain handling, tool-result forward scanning, and token usage rendering are nearly identical between the two functions. Consider extracting shared logic (e.g., a
prepareMessageshelper for filtering, afindToolResulthelper for the forward scan).Also applies to: 223-755
plugins/conversation-memory/src/core/parser.ts (3)
27-37: File stream is not cleaned up on unexpected errors.If the file doesn't exist or an I/O error occurs during streaming, the
fileStreamwon't be explicitly closed. Whilereadline'sfor awaithandles the happy path, wrapping the stream lifecycle in atry/finallywould be safer against resource leaks.♻️ Proposed fix to ensure stream cleanup
const fileStream = fs.createReadStream(filePath); - const rl = readline.createInterface({ - input: fileStream, - crlfDelay: Infinity - }); + const rl = readline.createInterface({ + input: fileStream, + crlfDelay: Infinity + }); let lineNumber = 0; ... - for await (const line of rl) { - lineNumber++; - ... - } + try { + for await (const line of rl) { + lineNumber++; + ... + } + } finally { + rl.close(); + fileStream.destroy(); + }
140-149: TODO stub: tool result association is incomplete.User messages consisting solely of
tool_resultblocks (no text) will be skipped at Line 153 becausetoolCallsis only populated for assistanttool_useblocks. This means tool execution results are silently dropped, and subsequent assistant responses may get appended to the prior exchange rather than a new one triggered by the tool-result turn.If this is a known deferral, consider at minimum tracking tool result content (e.g.,
isErrorstatus) on the preceding exchange'sToolCallentries so error context is preserved.Would you like me to open an issue to track the
tool_result→tool_useassociation?
222-229: Usepathutilities instead of hardcoded'/'separator.
filePath.split('/')breaks on Windows where the separator is\. Sincepathis not imported in this file but is available via Node.js, usepath.basename/path.dirnamefor reliable cross-platform behavior.♻️ Proposed fix
Add
import path from 'path';at the top of the file, then:- // Extract project name from path (directory name before the .jsonl file) - const pathParts = filePath.split('/'); - let project = 'unknown'; - - // Find the parent directory name (second to last part) - if (pathParts.length >= 2) { - project = pathParts[pathParts.length - 2]; - } + // Extract project name from the parent directory of the .jsonl file + const project = path.basename(path.dirname(filePath)) || 'unknown';plugins/conversation-memory/src/core/indexer.ts (3)
12-17: Global side effects at module import time.Setting
process.env.CLAUDE_CODE_MAX_OUTPUT_TOKENSandEventEmitter.defaultMaxListenersat the top level means they take effect as soon as any code imports this module — even if none of the exported functions are called. This can cause surprising behavior in tests or when this module is imported transitively.Consider moving these into the functions that need them (e.g., at the start of
indexConversations), or into a dedicatedinitfunction.♻️ Proposed fix — move side effects into function scope
-// Set max output tokens for Claude SDK (used by summarizer) -process.env.CLAUDE_CODE_MAX_OUTPUT_TOKENS = '20000'; - -// Increase max listeners for concurrent API calls -import { EventEmitter } from 'events'; -EventEmitter.defaultMaxListeners = 20;Then inside each exported function that needs it (e.g.,
indexConversations):function applyRuntimeDefaults() { process.env.CLAUDE_CODE_MAX_OUTPUT_TOKENS ??= '20000'; EventEmitter.defaultMaxListeners = Math.max(EventEmitter.defaultMaxListeners, 20); }
25-39:processBatchpropagates individual item failures to the entire batch.
Promise.allrejects on the first error, discarding results from other items in the same batch. Current callers mitigate this by catching inside the processor, but this is a fragile contract. ConsiderPromise.allSettledto make the function resilient by default.♻️ Proposed safer alternative
async function processBatch<T, R>( items: T[], processor: (item: T) => Promise<R>, concurrency: number -): Promise<R[]> { - const results: R[] = []; +): Promise<PromiseSettledResult<R>[]> { + const results: PromiseSettledResult<R>[] = []; for (let i = 0; i < items.length; i += concurrency) { const batch = items.slice(i, i + concurrency); - const batchResults = await Promise.all(batch.map(processor)); + const batchResults = await Promise.allSettled(batch.map(processor)); results.push(...batchResults); } return results; }
296-300: Raw SQL query bypasses thedb.tsabstraction.The inline
db.prepare('SELECT COUNT(*) as count FROM exchanges WHERE archive_path = ?')couples this module directly to the DB schema. If the table or column name changes indb.ts, this query silently breaks. Consider adding a helper likeisExchangeIndexed(db, archivePath)indb.ts.plugins/conversation-memory/src/core/sync.ts (1)
203-238: Files indexed in the same sync run are re-parsed for summarization.Newly copied files that pass through the indexing block (Lines 171-198) and also need summaries will be fully parsed a second time at Line 220. For large files this doubles the I/O. Consider caching parsed exchanges (e.g., in a
Map<string, ConversationExchange[]>) and reusing them in the summary phase.
| export function insertExchange( | ||
| db: Database.Database, | ||
| exchange: ConversationExchange, | ||
| embedding: number[], | ||
| toolNames?: string[] | ||
| ): void { | ||
| const now = Date.now(); | ||
|
|
||
| const stmt = db.prepare(` | ||
| INSERT OR REPLACE INTO exchanges | ||
| (id, project, timestamp, user_message, assistant_message, archive_path, line_start, line_end, last_indexed, | ||
| parent_uuid, is_sidechain, session_id, cwd, git_branch, claude_version, | ||
| thinking_level, thinking_disabled, thinking_triggers) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
| `); | ||
|
|
||
| stmt.run( | ||
| exchange.id, | ||
| exchange.project, | ||
| exchange.timestamp, | ||
| exchange.userMessage, | ||
| exchange.assistantMessage, | ||
| exchange.archivePath, | ||
| exchange.lineStart, | ||
| exchange.lineEnd, | ||
| now, | ||
| exchange.parentUuid || null, | ||
| exchange.isSidechain ? 1 : 0, | ||
| exchange.sessionId || null, | ||
| exchange.cwd || null, | ||
| exchange.gitBranch || null, | ||
| exchange.claudeVersion || null, | ||
| exchange.thinkingLevel || null, | ||
| exchange.thinkingDisabled ? 1 : 0, | ||
| exchange.thinkingTriggers || null | ||
| ); | ||
|
|
||
| // Insert into vector table (delete first since virtual tables don't support REPLACE) | ||
| const delStmt = db.prepare(`DELETE FROM vec_exchanges WHERE id = ?`); | ||
| delStmt.run(exchange.id); | ||
|
|
||
| const vecStmt = db.prepare(` | ||
| INSERT INTO vec_exchanges (id, embedding) | ||
| VALUES (?, ?) | ||
| `); | ||
|
|
||
| vecStmt.run(exchange.id, Buffer.from(new Float32Array(embedding).buffer)); | ||
|
|
||
| // Insert tool calls if present | ||
| if (exchange.toolCalls && exchange.toolCalls.length > 0) { | ||
| const toolStmt = db.prepare(` | ||
| INSERT OR REPLACE INTO tool_calls | ||
| (id, exchange_id, tool_name, tool_input, tool_result, is_error, timestamp) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?) | ||
| `); | ||
|
|
||
| for (const toolCall of exchange.toolCalls) { | ||
| toolStmt.run( | ||
| toolCall.id, | ||
| toolCall.exchangeId, | ||
| toolCall.toolName, | ||
| toolCall.toolInput ? JSON.stringify(toolCall.toolInput) : null, | ||
| toolCall.toolResult || null, | ||
| toolCall.isError ? 1 : 0, | ||
| toolCall.timestamp | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'insertExchange' --type=ts -C 3Repository: baleen37/claude-plugins
Length of output: 5334
🏁 Script executed:
sed -n '132,199p' plugins/conversation-memory/src/core/db.tsRepository: baleen37/claude-plugins
Length of output: 2149
🏁 Script executed:
rg -n 'db\.transaction\(' --type=ts -B 2 -A 5Repository: baleen37/claude-plugins
Length of output: 49
🏁 Script executed:
rg -n 'deleteExchange' --type=ts -A 10Repository: baleen37/claude-plugins
Length of output: 2574
🏁 Script executed:
# Check if better-sqlite3 transaction API is used anywhere in the codebase
rg -n '\.transaction\s*\(' --type=tsRepository: baleen37/claude-plugins
Length of output: 49
🏁 Script executed:
# Check the ConversationExchange type to understand the structure better
rg -n 'interface ConversationExchange|type ConversationExchange' --type=ts -A 15Repository: baleen37/claude-plugins
Length of output: 1179
🏁 Script executed:
sed -n '11,50p' plugins/conversation-memory/src/core/types.tsRepository: baleen37/claude-plugins
Length of output: 914
🏁 Script executed:
# Check how the database is initialized and what methods are available
sed -n '1,50p' plugins/conversation-memory/src/core/db.tsRepository: baleen37/claude-plugins
Length of output: 2113
Wrap database writes in transaction and remove unused toolNames parameter.
The function performs three separate write operations (exchange upsert, vec_exchanges delete+insert, tool_calls inserts) without a transaction wrapper. If the process crashes between operations, the database will be left in an inconsistent state (e.g., exchange exists but no vector embedding).
Additionally, the toolNames parameter is declared but never used—tool calls are sourced from exchange.toolCalls instead.
🛠️ Proposed fix
Wrap all database operations in a transaction and remove the unused parameter:
export function insertExchange(
db: Database.Database,
exchange: ConversationExchange,
embedding: number[],
- toolNames?: string[]
): void {
+ const txn = db.transaction(() => {
const now = Date.now();
const stmt = db.prepare(`
INSERT OR REPLACE INTO exchanges
(id, project, timestamp, user_message, assistant_message, archive_path, line_start, line_end, last_indexed,
parent_uuid, is_sidechain, session_id, cwd, git_branch, claude_version,
thinking_level, thinking_disabled, thinking_triggers)
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
`);
stmt.run(
exchange.id,
exchange.project,
exchange.timestamp,
exchange.userMessage,
exchange.assistantMessage,
exchange.archivePath,
exchange.lineStart,
exchange.lineEnd,
now,
exchange.parentUuid || null,
exchange.isSidechain ? 1 : 0,
exchange.sessionId || null,
exchange.cwd || null,
exchange.gitBranch || null,
exchange.claudeVersion || null,
exchange.thinkingLevel || null,
exchange.thinkingDisabled ? 1 : 0,
exchange.thinkingTriggers || null
);
const delStmt = db.prepare(`DELETE FROM vec_exchanges WHERE id = ?`);
delStmt.run(exchange.id);
const vecStmt = db.prepare(`
INSERT INTO vec_exchanges (id, embedding)
VALUES (?, ?)
`);
vecStmt.run(exchange.id, Buffer.from(new Float32Array(embedding).buffer));
if (exchange.toolCalls && exchange.toolCalls.length > 0) {
const toolStmt = db.prepare(`
INSERT OR REPLACE INTO tool_calls
(id, exchange_id, tool_name, tool_input, tool_result, is_error, timestamp)
VALUES (?, ?, ?, ?, ?, ?, ?)
`);
for (const toolCall of exchange.toolCalls) {
toolStmt.run(
toolCall.id,
toolCall.exchangeId,
toolCall.toolName,
toolCall.toolInput ? JSON.stringify(toolCall.toolInput) : null,
toolCall.toolResult || null,
toolCall.isError ? 1 : 0,
toolCall.timestamp
);
}
}
+ });
+ txn();
}Note: Also remove the toolNames argument from all four callers (sync.ts:188, verify.ts:167, indexer.ts:165, indexer.ts:241, indexer.ts:359).
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/db.ts` around lines 132 - 199, The
insertExchange function performs multiple writes without a transaction and
declares an unused parameter toolNames; fix by removing the toolNames parameter
from insertExchange and its callers (sync.ts, verify.ts, indexer.ts locations
mentioned) and wrap all DB operations in a single transaction using
db.transaction or db.exec("BEGIN"/"COMMIT"/"ROLLBACK") around the sequence: the
stmt.run(...) upsert into exchanges, the delStmt.run(...) + vecStmt.run(...) for
vec_exchanges, and the tool_calls inserts (toolStmt.run(...) loop) so either all
succeed or all roll back; ensure you convert boolean flags the same way (1/0)
and still use Buffer.from(new Float32Array(embedding).buffer) for the embedding.
| export async function initEmbeddings(): Promise<void> { | ||
| if (!embeddingPipeline) { | ||
| console.log('Loading embedding model (first run may take time)...'); | ||
|
|
||
| // Set cache directory | ||
| env.cacheDir = './.cache'; | ||
|
|
||
| // Load EmbeddingGemma with Q4 quantization | ||
| embeddingPipeline = await pipeline( | ||
| 'feature-extraction', | ||
| 'onnx-community/embeddinggemma-300m-ONNX', | ||
| { dtype: 'q4' } as any | ||
| ); | ||
|
|
||
| console.log('Embedding model loaded'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Concurrent callers can trigger duplicate model loads.
If two async callers invoke initEmbeddings (or generateEmbedding) before the first await pipeline(...) resolves, both will see embeddingPipeline === null and start loading the model in parallel. The second load is wasted work and doubles memory/time during startup.
Guard with a loading promise:
🔒 Proposed fix
let embeddingPipeline: FeatureExtractionPipeline | null = null;
+let initPromise: Promise<void> | null = null;
export async function initEmbeddings(): Promise<void> {
- if (!embeddingPipeline) {
+ if (embeddingPipeline) return;
+ if (initPromise) return initPromise;
+
+ initPromise = (async () => {
console.log('Loading embedding model (first run may take time)...');
// Set cache directory
env.cacheDir = './.cache';
// Load EmbeddingGemma with Q4 quantization
embeddingPipeline = await pipeline(
'feature-extraction',
'onnx-community/embeddinggemma-300m-ONNX',
{ dtype: 'q4' } as any
);
console.log('Embedding model loaded');
- }
+ })();
+
+ return initPromise;
}🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/embeddings.ts` around lines 5 - 21,
initEmbeddings can be raced by concurrent callers because they all check
embeddingPipeline and may start pipeline(...) in parallel; fix by adding a
shared loading promise (e.g., embeddingPipelineLoadingPromise) that is set
before awaiting pipeline and awaited by subsequent callers so only the first
caller performs the load; update initEmbeddings to set
embeddingPipelineLoadingPromise when starting the load, await it to assign
embeddingPipeline on success (and clear the promise on failure), and ensure
generateEmbedding awaits initEmbeddings or embeddingPipelineLoadingPromise
before using embeddingPipeline.
| export async function indexConversations( | ||
| limitToProject?: string, | ||
| maxConversations?: number, | ||
| concurrency: number = 1, | ||
| noSummaries: boolean = false | ||
| ): Promise<void> { | ||
| console.log('Initializing database...'); | ||
| const db = initDatabase(); | ||
|
|
||
| console.log('Loading embedding model...'); | ||
| await initEmbeddings(); | ||
|
|
||
| if (noSummaries) { | ||
| console.log('⚠️ Running in no-summaries mode (skipping AI summaries)'); | ||
| } | ||
|
|
||
| console.log('Scanning for conversation files...'); | ||
| const PROJECTS_DIR = getProjectsDir(); | ||
| const ARCHIVE_DIR = getArchiveDir(); // Now uses paths.ts | ||
| const projects = fs.readdirSync(PROJECTS_DIR); | ||
|
|
||
| let totalExchanges = 0; | ||
| let conversationsProcessed = 0; | ||
|
|
||
| const excludedProjects = getExcludedProjects(); | ||
|
|
||
| for (const project of projects) { | ||
| // Skip excluded projects | ||
| if (excludedProjects.includes(project)) { | ||
| console.log(`\nSkipping excluded project: ${project}`); | ||
| continue; | ||
| } | ||
|
|
||
| // Skip if limiting to specific project | ||
| if (limitToProject && project !== limitToProject) continue; | ||
| const projectPath = path.join(PROJECTS_DIR, project); | ||
| const stat = fs.statSync(projectPath); | ||
|
|
||
| if (!stat.isDirectory()) continue; | ||
|
|
||
| const files = fs.readdirSync(projectPath).filter(f => f.endsWith('.jsonl') && !f.startsWith('agent-')); | ||
|
|
||
| if (files.length === 0) continue; | ||
|
|
||
| console.log(`\nProcessing project: ${project} (${files.length} conversations)`); | ||
| if (concurrency > 1) console.log(` Concurrency: ${concurrency}`); | ||
|
|
||
| // Create archive directory for this project | ||
| const projectArchive = path.join(ARCHIVE_DIR, project); | ||
| fs.mkdirSync(projectArchive, { recursive: true }); | ||
|
|
||
| // Prepare all conversations first | ||
| type ConvToProcess = { | ||
| file: string; | ||
| sourcePath: string; | ||
| archivePath: string; | ||
| summaryPath: string; | ||
| exchanges: ConversationExchange[]; | ||
| }; | ||
|
|
||
| const toProcess: ConvToProcess[] = []; | ||
|
|
||
| for (const file of files) { | ||
| const sourcePath = path.join(projectPath, file); | ||
| const archivePath = path.join(projectArchive, file); | ||
|
|
||
| // Copy to archive | ||
| if (!fs.existsSync(archivePath)) { | ||
| fs.copyFileSync(sourcePath, archivePath); | ||
| console.log(` Archived: ${file}`); | ||
| } | ||
|
|
||
| // Parse conversation | ||
| const exchanges = await parseConversation(sourcePath, project, archivePath); | ||
|
|
||
| if (exchanges.length === 0) { | ||
| console.log(` Skipped ${file} (no exchanges)`); | ||
| continue; | ||
| } | ||
|
|
||
| toProcess.push({ | ||
| file, | ||
| sourcePath, | ||
| archivePath, | ||
| summaryPath: archivePath.replace('.jsonl', '-summary.txt'), | ||
| exchanges | ||
| }); | ||
| } | ||
|
|
||
| // Batch summarize conversations in parallel (unless --no-summaries) | ||
| if (!noSummaries) { | ||
| const needsSummary = toProcess.filter(c => !fs.existsSync(c.summaryPath)); | ||
|
|
||
| if (needsSummary.length > 0) { | ||
| console.log(` Generating ${needsSummary.length} summaries (concurrency: ${concurrency})...`); | ||
|
|
||
| await processBatch(needsSummary, async (conv) => { | ||
| try { | ||
| const summary = await summarizeConversation(conv.exchanges, undefined, conv.file); | ||
| fs.writeFileSync(conv.summaryPath, summary, 'utf-8'); | ||
| const wordCount = summary.split(/\s+/).length; | ||
| console.log(` ✓ ${conv.file}: ${wordCount} words`); | ||
| return summary; | ||
| } catch (error) { | ||
| console.log(` ✗ ${conv.file}: ${error}`); | ||
| logError(`Summary failed for ${conv.file}`, error); | ||
| return null; | ||
| } | ||
| }, concurrency); | ||
| } | ||
| } else { | ||
| console.log(` Skipping ${toProcess.length} summaries (--no-summaries mode)`); | ||
| } | ||
|
|
||
| // Now process embeddings and DB inserts (fast, sequential is fine) | ||
| for (const conv of toProcess) { | ||
| for (const exchange of conv.exchanges) { | ||
| const toolNames = exchange.toolCalls?.map(tc => tc.toolName); | ||
| const embedding = await generateExchangeEmbedding( | ||
| exchange.userMessage, | ||
| exchange.assistantMessage, | ||
| toolNames | ||
| ); | ||
|
|
||
| insertExchange(db, exchange, embedding, toolNames); | ||
| } | ||
|
|
||
| totalExchanges += conv.exchanges.length; | ||
| conversationsProcessed++; | ||
|
|
||
| // Check if we hit the limit | ||
| if (maxConversations && conversationsProcessed >= maxConversations) { | ||
| console.log(`\nReached limit of ${maxConversations} conversations`); | ||
| db.close(); | ||
| console.log(`✅ Indexing complete! Conversations: ${conversationsProcessed}, Exchanges: ${totalExchanges}`); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| db.close(); | ||
| console.log(`\n✅ Indexing complete! Conversations: ${conversationsProcessed}, Exchanges: ${totalExchanges}`); | ||
| } |
There was a problem hiding this comment.
Database handle leaks on unhandled exceptions.
If generateExchangeEmbedding or insertExchange throws (Lines 159-165), the function exits without calling db.close(). The early-return path (Line 174) and normal exit (Line 181) both close the DB, but the exception path does not.
Wrap the main body in try/finally to guarantee cleanup.
🐛 Proposed fix
export async function indexConversations(
...
): Promise<void> {
console.log('Initializing database...');
const db = initDatabase();
-
- console.log('Loading embedding model...');
- await initEmbeddings();
- ...
- db.close();
- console.log(`\n✅ Indexing complete! ...`);
-}
+ try {
+ console.log('Loading embedding model...');
+ await initEmbeddings();
+ ...
+ } finally {
+ db.close();
+ }
+ console.log(`\n✅ Indexing complete! ...`);
+}🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/indexer.ts` around lines 41 - 183, The
indexConversations function can leak the DB handle if generateExchangeEmbedding
or insertExchange throws; wrap the main processing logic (after const db =
initDatabase()) in a try/finally so db.close() is always called. Specifically,
enclose the loop that iterates projects/toProcess and the
summarization/embedding steps (where generateExchangeEmbedding and
insertExchange are called) in a try block and call db.close() in the finally
block; remove or keep the existing db.close() calls inside early-return paths
but ensure they are not duplicating cleanup (or only let finally handle close).
Ensure the finally runs even on exceptions so the DB handle opened in
indexConversations is always closed.
| export async function indexSession(sessionId: string, concurrency: number = 1, noSummaries: boolean = false): Promise<void> { | ||
| console.log(`Indexing session: ${sessionId}`); | ||
|
|
||
| // Find the conversation file for this session | ||
| const PROJECTS_DIR = getProjectsDir(); | ||
| const ARCHIVE_DIR = getArchiveDir(); // Now uses paths.ts | ||
| const projects = fs.readdirSync(PROJECTS_DIR); | ||
| const excludedProjects = getExcludedProjects(); | ||
| let found = false; | ||
|
|
||
| for (const project of projects) { | ||
| if (excludedProjects.includes(project)) continue; | ||
|
|
||
| const projectPath = path.join(PROJECTS_DIR, project); | ||
| if (!fs.statSync(projectPath).isDirectory()) continue; | ||
|
|
||
| const files = fs.readdirSync(projectPath).filter(f => f.includes(sessionId) && f.endsWith('.jsonl') && !f.startsWith('agent-')); | ||
|
|
||
| if (files.length > 0) { | ||
| found = true; | ||
| const file = files[0]; | ||
| const sourcePath = path.join(projectPath, file); | ||
|
|
||
| const db = initDatabase(); | ||
| await initEmbeddings(); | ||
|
|
||
| const projectArchive = path.join(ARCHIVE_DIR, project); | ||
| fs.mkdirSync(projectArchive, { recursive: true }); | ||
|
|
||
| const archivePath = path.join(projectArchive, file); | ||
|
|
||
| // Archive | ||
| if (!fs.existsSync(archivePath)) { | ||
| fs.copyFileSync(sourcePath, archivePath); | ||
| } | ||
|
|
||
| // Parse and summarize | ||
| const exchanges = await parseConversation(sourcePath, project, archivePath); | ||
|
|
||
| if (exchanges.length > 0) { | ||
| // Generate summary (unless --no-summaries) | ||
| const summaryPath = archivePath.replace('.jsonl', '-summary.txt'); | ||
| if (!noSummaries && !fs.existsSync(summaryPath)) { | ||
| const summary = await summarizeConversation(exchanges, undefined, file); | ||
| fs.writeFileSync(summaryPath, summary, 'utf-8'); | ||
| console.log(`Summary: ${summary.split(/\s+/).length} words`); | ||
| } | ||
|
|
||
| // Index | ||
| for (const exchange of exchanges) { | ||
| const toolNames = exchange.toolCalls?.map(tc => tc.toolName); | ||
| const embedding = await generateExchangeEmbedding( | ||
| exchange.userMessage, | ||
| exchange.assistantMessage, | ||
| toolNames | ||
| ); | ||
| insertExchange(db, exchange, embedding, toolNames); | ||
| } | ||
|
|
||
| console.log(`✅ Indexed session ${sessionId}: ${exchanges.length} exchanges`); | ||
| } | ||
|
|
||
| db.close(); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!found) { | ||
| console.log(`Session ${sessionId} not found`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same DB leak pattern — db.close() skipped on exception.
Lines 208–247 follow the same pattern as indexConversations: if generateExchangeEmbedding or insertExchange throws, db.close() on Line 247 is never reached. Apply the same try/finally fix here.
🐛 Proposed fix
const db = initDatabase();
await initEmbeddings();
...
+ try {
const exchanges = await parseConversation(sourcePath, project, archivePath);
...
- db.close();
+ } finally {
+ db.close();
+ }
break;🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/indexer.ts` around lines 185 - 255,
indexSession creates the DB via initDatabase() but calls db.close() only at the
end, so if generateExchangeEmbedding or insertExchange throws the DB is leaked;
wrap the DB lifetime in a try/finally (immediately after const db =
initDatabase()) and move db.close() into finally to ensure closure even on
errors, keeping the rest of the logic (summarizeConversation,
generateExchangeEmbedding, insertExchange) inside the try block; reference
functions/variables: indexSession, initDatabase, db.close,
generateExchangeEmbedding, insertExchange.
| export async function indexUnprocessed(concurrency: number = 1, noSummaries: boolean = false): Promise<void> { | ||
| console.log('Finding unprocessed conversations...'); | ||
| if (concurrency > 1) console.log(`Concurrency: ${concurrency}`); | ||
| if (noSummaries) console.log('⚠️ Running in no-summaries mode (skipping AI summaries)'); | ||
|
|
||
| const db = initDatabase(); | ||
| await initEmbeddings(); | ||
|
|
||
| const PROJECTS_DIR = getProjectsDir(); | ||
| const ARCHIVE_DIR = getArchiveDir(); // Now uses paths.ts | ||
| const projects = fs.readdirSync(PROJECTS_DIR); | ||
| const excludedProjects = getExcludedProjects(); | ||
|
|
||
| type UnprocessedConv = { | ||
| project: string; | ||
| file: string; | ||
| sourcePath: string; | ||
| archivePath: string; | ||
| summaryPath: string; | ||
| exchanges: ConversationExchange[]; | ||
| }; | ||
|
|
||
| const unprocessed: UnprocessedConv[] = []; | ||
|
|
||
| // Collect all unprocessed conversations | ||
| for (const project of projects) { | ||
| if (excludedProjects.includes(project)) continue; | ||
|
|
||
| const projectPath = path.join(PROJECTS_DIR, project); | ||
| if (!fs.statSync(projectPath).isDirectory()) continue; | ||
|
|
||
| const files = fs.readdirSync(projectPath).filter(f => f.endsWith('.jsonl') && !f.startsWith('agent-')); | ||
|
|
||
| for (const file of files) { | ||
| const sourcePath = path.join(projectPath, file); | ||
| const projectArchive = path.join(ARCHIVE_DIR, project); | ||
| const archivePath = path.join(projectArchive, file); | ||
| const summaryPath = archivePath.replace('.jsonl', '-summary.txt'); | ||
|
|
||
| // Check if already indexed in database | ||
| const alreadyIndexed = db.prepare('SELECT COUNT(*) as count FROM exchanges WHERE archive_path = ?') | ||
| .get(archivePath) as { count: number }; | ||
|
|
||
| if (alreadyIndexed.count > 0) continue; | ||
|
|
||
| fs.mkdirSync(projectArchive, { recursive: true }); | ||
|
|
||
| // Archive if needed | ||
| if (!fs.existsSync(archivePath)) { | ||
| fs.copyFileSync(sourcePath, archivePath); | ||
| } | ||
|
|
||
| // Parse and check | ||
| const exchanges = await parseConversation(sourcePath, project, archivePath); | ||
| if (exchanges.length === 0) continue; | ||
|
|
||
| unprocessed.push({ project, file, sourcePath, archivePath, summaryPath, exchanges }); | ||
| } | ||
| } | ||
|
|
||
| if (unprocessed.length === 0) { | ||
| console.log('✅ All conversations are already processed!'); | ||
| db.close(); | ||
| return; | ||
| } | ||
|
|
||
| console.log(`Found ${unprocessed.length} unprocessed conversations`); | ||
|
|
||
| // Batch process summaries (unless --no-summaries) | ||
| if (!noSummaries) { | ||
| const needsSummary = unprocessed.filter(c => !fs.existsSync(c.summaryPath)); | ||
| if (needsSummary.length > 0) { | ||
| console.log(`Generating ${needsSummary.length} summaries (concurrency: ${concurrency})...\n`); | ||
|
|
||
| await processBatch(needsSummary, async (conv) => { | ||
| try { | ||
| const summary = await summarizeConversation(conv.exchanges, undefined, `${conv.project}/${conv.file}`); | ||
| fs.writeFileSync(conv.summaryPath, summary, 'utf-8'); | ||
| const wordCount = summary.split(/\s+/).length; | ||
| console.log(` ✓ ${conv.project}/${conv.file}: ${wordCount} words`); | ||
| return summary; | ||
| } catch (error) { | ||
| console.log(` ✗ ${conv.project}/${conv.file}: ${error}`); | ||
| logError(`Summary failed for ${conv.project}/${conv.file}`, error); | ||
| return null; | ||
| } | ||
| }, concurrency); | ||
| } | ||
| } else { | ||
| console.log(`Skipping summaries for ${unprocessed.length} conversations (--no-summaries mode)\n`); | ||
| } | ||
|
|
||
| // Now index embeddings | ||
| console.log(`\nIndexing embeddings...`); | ||
| for (const conv of unprocessed) { | ||
| for (const exchange of conv.exchanges) { | ||
| const toolNames = exchange.toolCalls?.map(tc => tc.toolName); | ||
| const embedding = await generateExchangeEmbedding( | ||
| exchange.userMessage, | ||
| exchange.assistantMessage, | ||
| toolNames | ||
| ); | ||
| insertExchange(db, exchange, embedding, toolNames); | ||
| } | ||
| } | ||
|
|
||
| db.close(); | ||
| console.log(`\n✅ Processed ${unprocessed.length} conversations`); | ||
| } |
There was a problem hiding this comment.
Same db.close() leak on exceptions; additionally, all unprocessed exchanges are held in memory.
-
Same
try/finallyissue —db.close()on Line 363 is skipped if an exception propagates from the embedding/insert loop (Lines 351-360). -
Lines 290-314 parse all unprocessed files upfront, storing every
ConversationExchange[]in theunprocessedarray before indexing begins. For a large backlog this could cause significant memory pressure. Consider processing each conversation's embeddings immediately after parsing, or streaming in smaller batches.
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/indexer.ts` around lines 257 - 365,
indexUnprocessed currently leaks the DB on exceptions and builds an in-memory
list of all exchanges (unprocessed[]) which can blow memory; wrap DB usage in a
try/finally so db.close() is always called (move initDatabase() result into a
variable and ensure db.close() in finally), and stop accumulating full
ConversationExchange[] arrays by processing each conversation immediately after
parsing: after parseConversation(project, sourcePath, archivePath) handle
summary (use processBatch or per-conv summarizeConversation) and then call
generateExchangeEmbedding + insertExchange per exchange right away instead of
pushing into unprocessed; keep processBatch for concurrent summaries but change
it to accept single-conversation work so you never retain all exchanges in
memory.
| function isTrivialConversation(exchanges: ConversationExchange[]): boolean { | ||
| if (exchanges.length === 0) { | ||
| return true; | ||
| } | ||
|
|
||
| if (exchanges.length === 1) { | ||
| const userMsg = exchanges[0].userMessage.trim(); | ||
| const assistantMsg = exchanges[0].assistantMessage.trim(); | ||
|
|
||
| // Check for exit command (only /exit is the official command) | ||
| if (userMsg === '/exit') { | ||
| return true; | ||
| } | ||
|
|
||
| // Use word count instead of character count for language-agnostic detection | ||
| // Split on whitespace and CJK characters | ||
| const wordCount = (userMsg + ' ' + assistantMsg) | ||
| .split(/[\s\u3000-\u303F\u4E00-\u9FFF\uAC00-\uD7AF\u3040-\u309F\u30A0-\u30FF]+/) | ||
| .filter(word => word.length > 0).length; | ||
|
|
||
| // Less than 15 words total is considered trivial | ||
| if (wordCount < 15) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
CJK word-count heuristic will misclassify substantive CJK conversations as trivial.
The regex on Line 191 splits on CJK character ranges (\u4E00-\u9FFF, etc.), treating each ideograph as a delimiter rather than a word. For a message written entirely in Chinese/Japanese/Korean, the split produces mostly empty strings, yielding a near-zero wordCount. A substantive 2-sentence CJK conversation would be incorrectly classified as trivial and skipped.
🛠️ Suggested approach — count CJK characters as words
- const wordCount = (userMsg + ' ' + assistantMsg)
- .split(/[\s\u3000-\u303F\u4E00-\u9FFF\uAC00-\uD7AF\u3040-\u309F\u30A0-\u30FF]+/)
- .filter(word => word.length > 0).length;
+ const text = userMsg + ' ' + assistantMsg;
+ // Count whitespace-delimited words + individual CJK characters
+ const latinWords = text.split(/\s+/).filter(w => w.length > 0).length;
+ const cjkChars = (text.match(/[\u4E00-\u9FFF\uAC00-\uD7AF\u3040-\u309F\u30A0-\u30FF]/g) || []).length;
+ const wordCount = latinWords + cjkChars;This way, each CJK character contributes to the count, and Latin words are counted normally.
📝 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.
| function isTrivialConversation(exchanges: ConversationExchange[]): boolean { | |
| if (exchanges.length === 0) { | |
| return true; | |
| } | |
| if (exchanges.length === 1) { | |
| const userMsg = exchanges[0].userMessage.trim(); | |
| const assistantMsg = exchanges[0].assistantMessage.trim(); | |
| // Check for exit command (only /exit is the official command) | |
| if (userMsg === '/exit') { | |
| return true; | |
| } | |
| // Use word count instead of character count for language-agnostic detection | |
| // Split on whitespace and CJK characters | |
| const wordCount = (userMsg + ' ' + assistantMsg) | |
| .split(/[\s\u3000-\u303F\u4E00-\u9FFF\uAC00-\uD7AF\u3040-\u309F\u30A0-\u30FF]+/) | |
| .filter(word => word.length > 0).length; | |
| // Less than 15 words total is considered trivial | |
| if (wordCount < 15) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| function isTrivialConversation(exchanges: ConversationExchange[]): boolean { | |
| if (exchanges.length === 0) { | |
| return true; | |
| } | |
| if (exchanges.length === 1) { | |
| const userMsg = exchanges[0].userMessage.trim(); | |
| const assistantMsg = exchanges[0].assistantMessage.trim(); | |
| // Check for exit command (only /exit is the official command) | |
| if (userMsg === '/exit') { | |
| return true; | |
| } | |
| // Use word count instead of character count for language-agnostic detection | |
| // Split on whitespace and CJK characters | |
| const text = userMsg + ' ' + assistantMsg; | |
| // Count whitespace-delimited words + individual CJK characters | |
| const latinWords = text.split(/\s+/).filter(w => w.length > 0).length; | |
| const cjkChars = (text.match(/[\u4E00-\u9FFF\uAC00-\uD7AF\u3040-\u309F\u30A0-\u30FF]/g) || []).length; | |
| const wordCount = latinWords + cjkChars; | |
| // Less than 15 words total is considered trivial | |
| if (wordCount < 15) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/summarizer.ts` around lines 174 - 201,
The isTrivialConversation function miscounts CJK text because the split regex
treats CJK ideographs as delimiters; update the logic to split only on
whitespace for Latin scripts and separately count CJK characters as words:
compute wordCount = number of non-empty tokens from splitting on /\s+/ plus the
count of CJK characters matched with a regex like
/[\u4E00-\u9FFF\u3400-\u4DBF\uF900-\uFAFF\uAC00-\uD7AF\u3040-\u309F\u30A0-\u30FF]/g,
then use that combined count for the "< 15" trivial check in
isTrivialConversation so substantive CJK messages are not misclassified.
| function shouldSkipConversation(filePath: string): boolean { | ||
| try { | ||
| const content = fs.readFileSync(filePath, 'utf-8'); | ||
| return EXCLUSION_MARKERS.some(marker => content.includes(marker)); | ||
| } catch (error) { | ||
| // If we can't read the file, don't skip it | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find where EXCLUSION_MARKERS is defined
rg -n "EXCLUSION_MARKERS" plugins/conversation-memory/src/core/sync.ts -A 3 -B 3Repository: baleen37/claude-plugins
Length of output: 673
🏁 Script executed:
# Search for where these markers are used and defined throughout the codebase
rg -n "DO NOT INDEX THIS CHAT|NO_INSIGHTS_FOUND|SUMMARIZER_CONTEXT_MARKER" --type=tsRepository: baleen37/claude-plugins
Length of output: 999
🏁 Script executed:
# Look at test files to see marker placement in example files
find plugins/conversation-memory -name "*.test.ts" -o -name "*.spec.ts" | head -5Repository: baleen37/claude-plugins
Length of output: 175
🏁 Script executed:
# Check if there are fixture files with conversation data
find plugins/conversation-memory -name "*.json" -o -name "*.jsonl" | head -10Repository: baleen37/claude-plugins
Length of output: 333
🏁 Script executed:
# Search for sumTokenUsage across the codebase to check for duplication
rg -n "export.*sumTokenUsage|function sumTokenUsage" --type=tsRepository: baleen37/claude-plugins
Length of output: 286
🏁 Script executed:
# Check the SUMMARIZER_CONTEXT_MARKER value in constants.ts
rg -n "export const SUMMARIZER_CONTEXT_MARKER" plugins/conversation-memory/src/core/constants.ts -A 2Repository: baleen37/claude-plugins
Length of output: 227
🏁 Script executed:
# Check test files to see how exclusion markers are used
rg -n "EXCLUSION_MARKERS|DO NOT INDEX|NO_INSIGHTS" plugins/conversation-memory/src/core/summarizer.test.ts -B 3 -A 3Repository: baleen37/claude-plugins
Length of output: 49
🏁 Script executed:
# Look at the conversation parsing logic
rg -n "parseConversation|readFileSync" plugins/conversation-memory/src/core/sync.ts -B 2 -A 5Repository: baleen37/claude-plugins
Length of output: 1870
🏁 Script executed:
# Check if markers are typically written at file start by looking at how conversations are saved/parsed
rg -n "writeFileSync|appendFileSync" plugins/conversation-memory/src/core/ --type=ts -B 2 -A 2Repository: baleen37/claude-plugins
Length of output: 3074
🏁 Script executed:
# Check how the marker is used - is it added to prompts or to conversation files?
rg -n "SUMMARIZER_CONTEXT_MARKER" plugins/conversation-memory/src/core/ --type=ts -B 1 -A 1 | head -30Repository: baleen37/claude-plugins
Length of output: 1972
🏁 Script executed:
# Check the parser to understand conversation file structure
rg -n "parseConversation" plugins/conversation-memory/src/core/parser.ts -A 20 | head -40Repository: baleen37/claude-plugins
Length of output: 1307
🏁 Script executed:
# Look for how markers are added to conversations
rg -n "DO NOT INDEX THIS CHAT|NO_INSIGHTS_FOUND" plugins/conversation-memory/src/core/ --type=tsRepository: baleen37/claude-plugins
Length of output: 287
🏁 Script executed:
# Check if there's documentation about marker placement
find plugins/conversation-memory -name "README*" -o -name "*.md" | xargs ls -la 2>/dev/nullRepository: baleen37/claude-plugins
Length of output: 616
🏁 Script executed:
# See the full definition of EXCLUSION_MARKERS context
cat -n plugins/conversation-memory/src/core/sync.ts | sed -n '1,15p'Repository: baleen37/claude-plugins
Length of output: 692
🏁 Script executed:
# Check the sumTokenUsage implementations to see if they're identical
cat -n plugins/conversation-memory/src/core/summarizer.ts | sed -n '80,90p'Repository: baleen37/claude-plugins
Length of output: 719
🏁 Script executed:
cat -n plugins/conversation-memory/src/core/sync.ts | sed -n '38,48p'Repository: baleen37/claude-plugins
Length of output: 839
🏁 Script executed:
# Check if sync.ts exports sumTokenUsage
rg -n "export.*sumTokenUsage" plugins/conversation-memory/src/core/sync.tsRepository: baleen37/claude-plugins
Length of output: 49
🏁 Script executed:
# Check if sync.ts imports sumTokenUsage from summarizer instead
rg -n "import.*sumTokenUsage" plugins/conversation-memory/src/core/Repository: baleen37/claude-plugins
Length of output: 49
🏁 Script executed:
# Check if SUMMARIZER_CONTEXT_MARKER ever appears in conversation files or test data
rg "Context: This summary will be shown" plugins/conversation-memory/ --type=ts --type=jsonRepository: baleen37/claude-plugins
Length of output: 232
🏁 Script executed:
# Look at the actual conversatione file format/examples
find plugins/conversation-memory -name "*.jsonl" -o -name "*conversation*" -type f | grep -E "\.(jsonl|json)$"Repository: baleen37/claude-plugins
Length of output: 49
🏁 Script executed:
# Check how these markers are meant to be used in conversations
rg -n "DO NOT INDEX THIS CHAT" plugins/ -B 5 -A 5 --type=tsRepository: baleen37/claude-plugins
Length of output: 998
🏁 Script executed:
# Check if SUMMARIZER_CONTEXT_MARKER should actually be in EXCLUSION_MARKERS
# by looking at where it's set and how it's used
rg -n "SUMMARIZER_CONTEXT_MARKER" plugins/conversation-memory/src/core/ -B 2 -A 2 | head -50Repository: baleen37/claude-plugins
Length of output: 3734
🏁 Script executed:
# Check sync.ts to see how shouldSkipConversation is actually used in context
cat -n plugins/conversation-memory/src/core/sync.ts | sed -n '140,180p'Repository: baleen37/claude-plugins
Length of output: 1780
🏁 Script executed:
# Check if SUMMARIZER_CONTEXT_MARKER ever gets written to conversation files
# by searching for where it might be appended/written
rg -n "SUMMARIZER_CONTEXT_MARKER" plugins/conversation-memory/src/core/ -B 3 -A 3 | grep -E "(write|append|file|conversation)" -iRepository: baleen37/claude-plugins
Length of output: 4660
🏁 Script executed:
# Look at how conversations are created/parsed to understand the file structure
cat -n plugins/conversation-memory/src/core/parser.ts | sed -n '27,80p'Repository: baleen37/claude-plugins
Length of output: 2124
shouldSkipConversation reads entire files inefficiently, and includes a questionable marker in exclusion list.
The function loads JSONL conversation files entirely into memory via fs.readFileSync() just to check for marker strings. Since this is called twice per file (lines 146 and 174), large files incur repeated overhead. Reading only the first chunk (where metadata markers typically appear) would be more efficient.
However, there's a logic issue: SUMMARIZER_CONTEXT_MARKER (a prompt prefix string from summarizer.ts) is included in EXCLUSION_MARKERS, but this constant is only used in LLM prompts and is never written to conversation files. It should be removed from the exclusion list.
Additionally, sumTokenUsage is duplicated between summarizer.ts and sync.ts with nearly identical implementations—extract to a shared utility module.
♻️ Proposed changes
-
Remove SUMMARIZER_CONTEXT_MARKER from EXCLUSION_MARKERS (line 10) since it's never written to conversation files.
-
Optimize shouldSkipConversation to read only the first chunk:
function shouldSkipConversation(filePath: string): boolean {
try {
- const content = fs.readFileSync(filePath, 'utf-8');
- return EXCLUSION_MARKERS.some(marker => content.includes(marker));
+ const fd = fs.openSync(filePath, 'r');
+ const buf = Buffer.alloc(8192);
+ const bytesRead = fs.readSync(fd, buf, 0, buf.length, 0);
+ fs.closeSync(fd);
+ const head = buf.toString('utf-8', 0, bytesRead);
+ return EXCLUSION_MARKERS.some(marker => head.includes(marker));
} catch (error) {
return false;
}
}- Consolidate sumTokenUsage into a shared utility file.
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/sync.ts` around lines 13 - 21, The
function shouldSkipConversation currently reads entire files with
fs.readFileSync and uses an exclusion marker that never appears in files; update
it to read only the first chunk (e.g., fs.createReadStream with a small
highWaterMark or fs.open + read a fixed byte range) and search that chunk for
markers instead of loading the whole file, remove SUMMARIZER_CONTEXT_MARKER from
EXCLUSION_MARKERS so only actual file markers remain, and eliminate the
duplicated sumTokenUsage by moving the implementation from sync.ts and
summarizer.ts into a shared utility module (export/import the single
sumTokenUsage utility wherever needed).
| // Index copied files (unless skipIndex is set) | ||
| if (!options.skipIndex && filesToIndex.length > 0) { | ||
| const { initDatabase, insertExchange } = await import('./db.js'); | ||
| const { initEmbeddings, generateExchangeEmbedding } = await import('./embeddings.js'); | ||
| const { parseConversation } = await import('./parser.js'); | ||
|
|
||
| const db = initDatabase(); | ||
| await initEmbeddings(); | ||
|
|
||
| for (const file of filesToIndex) { | ||
| try { | ||
| // Check for DO NOT INDEX marker | ||
| if (shouldSkipConversation(file)) { | ||
| continue; // Skip indexing but file is already copied | ||
| } | ||
|
|
||
| const project = path.basename(path.dirname(file)); | ||
| const exchanges = await parseConversation(file, project, file); | ||
|
|
||
| for (const exchange of exchanges) { | ||
| const toolNames = exchange.toolCalls?.map(tc => tc.toolName); | ||
| const embedding = await generateExchangeEmbedding( | ||
| exchange.userMessage, | ||
| exchange.assistantMessage, | ||
| toolNames | ||
| ); | ||
| insertExchange(db, exchange, embedding, toolNames); | ||
| } | ||
|
|
||
| result.indexed++; | ||
| } catch (error) { | ||
| result.errors.push({ | ||
| file, | ||
| error: error instanceof Error ? error.message : String(error) | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| db.close(); |
There was a problem hiding this comment.
DB handle leaks if initEmbeddings() throws.
If initEmbeddings() on Line 169 fails, db (initialized on Line 168) is never closed. Wrap in try/finally:
🐛 Proposed fix
if (!options.skipIndex && filesToIndex.length > 0) {
const { initDatabase, insertExchange } = await import('./db.js');
const { initEmbeddings, generateExchangeEmbedding } = await import('./embeddings.js');
const { parseConversation } = await import('./parser.js');
const db = initDatabase();
- await initEmbeddings();
-
- for (const file of filesToIndex) {
- ...
+ try {
+ await initEmbeddings();
+
+ for (const file of filesToIndex) {
+ ...
+ }
+ } finally {
+ db.close();
}
-
- db.close();
}📝 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.
| // Index copied files (unless skipIndex is set) | |
| if (!options.skipIndex && filesToIndex.length > 0) { | |
| const { initDatabase, insertExchange } = await import('./db.js'); | |
| const { initEmbeddings, generateExchangeEmbedding } = await import('./embeddings.js'); | |
| const { parseConversation } = await import('./parser.js'); | |
| const db = initDatabase(); | |
| await initEmbeddings(); | |
| for (const file of filesToIndex) { | |
| try { | |
| // Check for DO NOT INDEX marker | |
| if (shouldSkipConversation(file)) { | |
| continue; // Skip indexing but file is already copied | |
| } | |
| const project = path.basename(path.dirname(file)); | |
| const exchanges = await parseConversation(file, project, file); | |
| for (const exchange of exchanges) { | |
| const toolNames = exchange.toolCalls?.map(tc => tc.toolName); | |
| const embedding = await generateExchangeEmbedding( | |
| exchange.userMessage, | |
| exchange.assistantMessage, | |
| toolNames | |
| ); | |
| insertExchange(db, exchange, embedding, toolNames); | |
| } | |
| result.indexed++; | |
| } catch (error) { | |
| result.errors.push({ | |
| file, | |
| error: error instanceof Error ? error.message : String(error) | |
| }); | |
| } | |
| } | |
| db.close(); | |
| // Index copied files (unless skipIndex is set) | |
| if (!options.skipIndex && filesToIndex.length > 0) { | |
| const { initDatabase, insertExchange } = await import('./db.js'); | |
| const { initEmbeddings, generateExchangeEmbedding } = await import('./embeddings.js'); | |
| const { parseConversation } = await import('./parser.js'); | |
| const db = initDatabase(); | |
| try { | |
| await initEmbeddings(); | |
| for (const file of filesToIndex) { | |
| try { | |
| // Check for DO NOT INDEX marker | |
| if (shouldSkipConversation(file)) { | |
| continue; // Skip indexing but file is already copied | |
| } | |
| const project = path.basename(path.dirname(file)); | |
| const exchanges = await parseConversation(file, project, file); | |
| for (const exchange of exchanges) { | |
| const toolNames = exchange.toolCalls?.map(tc => tc.toolName); | |
| const embedding = await generateExchangeEmbedding( | |
| exchange.userMessage, | |
| exchange.assistantMessage, | |
| toolNames | |
| ); | |
| insertExchange(db, exchange, embedding, toolNames); | |
| } | |
| result.indexed++; | |
| } catch (error) { | |
| result.errors.push({ | |
| file, | |
| error: error instanceof Error ? error.message : String(error) | |
| }); | |
| } | |
| } | |
| } finally { | |
| db.close(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/sync.ts` around lines 162 - 200,
initEmbeddings() can throw after db is created via initDatabase(), leaking the
DB handle; wrap the work that uses db in a try/finally so db.close() always
runs. Specifically, after calling const db = initDatabase() move the rest of the
indexing logic (including await initEmbeddings(), the for loop over
filesToIndex, and result updates) into a try block and call db.close() inside a
finally block; ensure insertExchange(db, ...) and any early continues still
result in db.close() by keeping those inside the try.
|
|
||
| if (!stat.isDirectory()) continue; | ||
|
|
||
| const files = fs.readdirSync(projectPath).filter(f => f.endsWith('.jsonl')); |
There was a problem hiding this comment.
Missing agent- file exclusion that sync.ts applies.
sync.ts (line 126 in the relevant snippets) filters out files starting with agent-:
.filter(f => f.endsWith('.jsonl') && !f.startsWith('agent-'))
Here, verifyIndex only filters by .jsonl extension, so agent- files will be checked and flagged as "missing summaries" even though they were never intended to be indexed. This produces false positives.
🔧 Proposed fix
- const files = fs.readdirSync(projectPath).filter(f => f.endsWith('.jsonl'));
+ const files = fs.readdirSync(projectPath).filter(f => f.endsWith('.jsonl') && !f.startsWith('agent-'));📝 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.
| const files = fs.readdirSync(projectPath).filter(f => f.endsWith('.jsonl')); | |
| const files = fs.readdirSync(projectPath).filter(f => f.endsWith('.jsonl') && !f.startsWith('agent-')); |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/verify.ts` at line 50, The verifyIndex
code currently builds files with const files =
fs.readdirSync(projectPath).filter(...) but only filters by .jsonl, causing
agent-*.jsonl files to be falsely flagged; update the filter used in verifyIndex
(the files variable) to match sync.ts by excluding files that start with
"agent-" (i.e., filter to f => f.endsWith('.jsonl') && !f.startsWith('agent-'))
so verifyIndex and sync.ts use the same selection logic.
| if (name === 'read') { | ||
| const params = ShowConversationInputSchema.parse(args); | ||
|
|
||
| // Verify file exists | ||
| if (!fs.existsSync(params.path)) { | ||
| throw new Error(`File not found: ${params.path}`); | ||
| } | ||
|
|
||
| // Read and format conversation with optional line range | ||
| const jsonlContent = fs.readFileSync(params.path, 'utf-8'); | ||
| const markdownContent = formatConversationAsMarkdown( | ||
| jsonlContent, | ||
| params.startLine, | ||
| params.endLine | ||
| ); | ||
|
|
||
| return { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: markdownContent, | ||
| }, | ||
| ], | ||
| }; | ||
| } |
There was a problem hiding this comment.
Path traversal: read tool allows reading arbitrary files.
The path parameter accepts any non-empty string and the handler directly calls fs.readFileSync on it. An MCP client could request /etc/passwd, ~/.ssh/id_rsa, or any other file the process can access.
Add a path validation check to ensure the resolved path is within the expected archive directory.
🔒 Proposed path validation
+import { getArchiveDir } from '../core/paths.js';
+import pathModule from 'path';
if (name === 'read') {
const params = ShowConversationInputSchema.parse(args);
+ // Ensure the path is within the archive directory
+ const archiveDir = getArchiveDir();
+ const resolvedPath = pathModule.resolve(params.path);
+ if (!resolvedPath.startsWith(archiveDir)) {
+ throw new Error(`Access denied: path must be within the conversation archive directory`);
+ }
+
// Verify file exists
if (!fs.existsSync(params.path)) {
throw new Error(`File not found: ${params.path}`);
}🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/mcp/server.ts` around lines 250 - 274, The
read handler currently reads params.path directly (ShowConversationInputSchema,
params.path, formatConversationAsMarkdown) allowing path traversal; fix by
resolving the absolute path and ensuring it is inside the allowed archive
directory before reading: compute a resolvedPath via path.resolve(archiveDir,
params.path) (or use path.resolve and path.relative) and check that the
resolvedPath is within archiveDir (reject if it escapes with .. or if
path.relative indicates traversal or the prefix doesn't match), keep the
fs.existsSync and subsequent read only after this validation, and throw a clear
error when the path is outside the archive.
- Add $schema reference to hooks.json - Remove non-standard async property from hooks - Add sync command to CLI (integrate syncConversations) - Add version field to remembering-conversations skill - Fix README installation path (memory -> conversation-memory) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@plugins/conversation-memory/src/cli/index-cli.ts`:
- Around line 101-103: The console message that suggests "Run with --repair" is
misleading because the CLI uses the repair subcommand; update the log line that
checks issues (the block referencing issues.missing, issues.orphaned,
issues.outdated, issues.corrupted and the console.log call) to instruct users to
run the repair subcommand instead (for example: "Run 'index-cli repair' to fix
these issues." or "Run the 'repair' subcommand (e.g., index-cli repair).") so
the message matches the actual CLI invocation.
- Around line 120-149: The rebuild branch currently deletes only the main DB
file (dbPath) but leaves SQLite WAL/SHM sidecars; update the case 'rebuild'
logic to also remove the WAL and SHM files derived from getDbPath() (e.g.,
`${dbPath}-wal` and `${dbPath}-shm`) if they exist, using the same
fs.existsSync/fs.unlinkSync pattern; keep the existing behavior for deleting
summaries and then call indexConversations(...) as before so the fresh database
is created without stale journal files.
- Around line 56-154: The switch cases in the CLI (cases 'index-session',
'index-cleanup', 'sync', 'verify', 'repair', 'rebuild', 'index-all') declare
const/let variables (e.g., sessionId, syncSourceDir, issues, repairIssues,
dbPath, archiveDir) which leak across switch scope; wrap the body of each case
in its own block { ... } so those declarations are block-scoped and cannot
collide or trigger TDZ issues—apply this to the blocks that call indexSession,
indexUnprocessed, syncConversations, verifyIndex, repairIndex,
getDbPath/getArchiveDir cleanup, and indexConversations.
- Around line 109-118: In the 'repair' switch case, the condition currently
checks only repairIssues.missing, .orphaned, and .outdated before deciding to
call repairIndex; include repairIssues.corrupted in that check so corrupted-only
issues trigger repairIndex; update the conditional in the case 'repair' block
(where verifyIndex() is called and repairIndex(repairIssues) is invoked) to sum
or otherwise test corrupted.length alongside the other arrays.
| switch (command) { | ||
| case 'index-session': | ||
| const sessionId = process.argv[3]; | ||
| if (!sessionId) { | ||
| console.error('Usage: index-cli index-session <session-id>'); | ||
| process.exit(1); | ||
| } | ||
| await indexSession(sessionId, concurrency, noSummaries); | ||
| break; | ||
|
|
||
| case 'index-cleanup': | ||
| await indexUnprocessed(concurrency, noSummaries); | ||
| break; | ||
|
|
||
| case 'sync': | ||
| const syncSourceDir = path.join(os.homedir(), '.claude', 'projects'); | ||
| const syncDestDir = getArchiveDir(); | ||
| console.log('Syncing conversations...'); | ||
| const syncResult = await syncConversations(syncSourceDir, syncDestDir, { skipSummaries: noSummaries }); | ||
| console.log(`\nSync complete!`); | ||
| console.log(` Copied: ${syncResult.copied}`); | ||
| console.log(` Skipped: ${syncResult.skipped}`); | ||
| console.log(` Indexed: ${syncResult.indexed}`); | ||
| console.log(` Summarized: ${syncResult.summarized}`); | ||
| if (syncResult.errors.length > 0) { | ||
| console.log(`\nErrors: ${syncResult.errors.length}`); | ||
| syncResult.errors.forEach(err => console.log(` ${err.file}: ${err.error}`)); | ||
| } | ||
| break; | ||
|
|
||
| case 'verify': | ||
| console.log('Verifying conversation index...'); | ||
| const issues = await verifyIndex(); | ||
|
|
||
| console.log('\n=== Verification Results ==='); | ||
| console.log(`Missing summaries: ${issues.missing.length}`); | ||
| console.log(`Orphaned entries: ${issues.orphaned.length}`); | ||
| console.log(`Outdated files: ${issues.outdated.length}`); | ||
| console.log(`Corrupted files: ${issues.corrupted.length}`); | ||
|
|
||
| if (issues.missing.length > 0) { | ||
| console.log('\nMissing summaries:'); | ||
| issues.missing.forEach(m => console.log(` ${m.path}`)); | ||
| } | ||
|
|
||
| if (issues.missing.length + issues.orphaned.length + issues.outdated.length + issues.corrupted.length > 0) { | ||
| console.log('\nRun with --repair to fix these issues.'); | ||
| process.exit(1); | ||
| } else { | ||
| console.log('\n✅ Index is healthy!'); | ||
| } | ||
| break; | ||
|
|
||
| case 'repair': | ||
| console.log('Verifying conversation index...'); | ||
| const repairIssues = await verifyIndex(); | ||
|
|
||
| if (repairIssues.missing.length + repairIssues.orphaned.length + repairIssues.outdated.length > 0) { | ||
| await repairIndex(repairIssues); | ||
| } else { | ||
| console.log('✅ No issues to repair!'); | ||
| } | ||
| break; | ||
|
|
||
| case 'rebuild': | ||
| console.log('Rebuilding entire index...'); | ||
|
|
||
| // Delete database | ||
| const dbPath = getDbPath(); | ||
| if (fs.existsSync(dbPath)) { | ||
| fs.unlinkSync(dbPath); | ||
| console.log('Deleted existing database'); | ||
| } | ||
|
|
||
| // Delete all summary files | ||
| const archiveDir = getArchiveDir(); | ||
| if (fs.existsSync(archiveDir)) { | ||
| const projects = fs.readdirSync(archiveDir); | ||
| for (const project of projects) { | ||
| const projectPath = path.join(archiveDir, project); | ||
| if (!fs.statSync(projectPath).isDirectory()) continue; | ||
|
|
||
| const summaries = fs.readdirSync(projectPath).filter(f => f.endsWith('-summary.txt')); | ||
| for (const summary of summaries) { | ||
| fs.unlinkSync(path.join(projectPath, summary)); | ||
| } | ||
| } | ||
| console.log('Deleted all summary files'); | ||
| } | ||
|
|
||
| // Re-index everything | ||
| console.log('Re-indexing all conversations...'); | ||
| await indexConversations(undefined, undefined, concurrency, noSummaries); | ||
| break; | ||
|
|
||
| case 'index-all': | ||
| default: | ||
| await indexConversations(undefined, undefined, concurrency, noSummaries); | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Wrap each case body in braces to avoid switch-scope variable leakage.
All const/let declarations inside switch cases are scoped to the entire switch block in JS/TS, which can lead to TDZ errors or accidental access from other cases. Biome flagged this as well. Wrap each case body in { }.
♻️ Example fix for two cases (apply the same pattern to all)
case 'index-session':
+ {
const sessionId = process.argv[3];
if (!sessionId) {
console.error('Usage: index-cli index-session <session-id>');
process.exit(1);
}
await indexSession(sessionId, concurrency, noSummaries);
break;
+ }
- case 'sync':
+ case 'sync': {
const syncSourceDir = path.join(os.homedir(), '.claude', 'projects');
...
break;
+ }🧰 Tools
🪛 Biome (2.3.13)
[error] 58-59: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 71-72: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 72-73: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 74-75: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 82-82: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
[error] 88-90: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 98-98: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
[error] 111-113: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 124-125: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 131-132: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/cli/index-cli.ts` around lines 56 - 154, The
switch cases in the CLI (cases 'index-session', 'index-cleanup', 'sync',
'verify', 'repair', 'rebuild', 'index-all') declare const/let variables (e.g.,
sessionId, syncSourceDir, issues, repairIssues, dbPath, archiveDir) which leak
across switch scope; wrap the body of each case in its own block { ... } so
those declarations are block-scoped and cannot collide or trigger TDZ
issues—apply this to the blocks that call indexSession, indexUnprocessed,
syncConversations, verifyIndex, repairIndex, getDbPath/getArchiveDir cleanup,
and indexConversations.
| if (issues.missing.length + issues.orphaned.length + issues.outdated.length + issues.corrupted.length > 0) { | ||
| console.log('\nRun with --repair to fix these issues.'); | ||
| process.exit(1); |
There was a problem hiding this comment.
Misleading hint: --repair is not a flag — the command is repair.
Line 102 tells the user to "Run with --repair", but looking at the switch structure, the actual CLI invocation is the repair subcommand (e.g., index-cli repair), not a --repair flag. This will confuse users.
🐛 Proposed fix
- console.log('\nRun with --repair to fix these issues.');
+ console.log('\nRun the "repair" command to fix these issues.');📝 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.
| if (issues.missing.length + issues.orphaned.length + issues.outdated.length + issues.corrupted.length > 0) { | |
| console.log('\nRun with --repair to fix these issues.'); | |
| process.exit(1); | |
| if (issues.missing.length + issues.orphaned.length + issues.outdated.length + issues.corrupted.length > 0) { | |
| console.log('\nRun the "repair" command to fix these issues.'); | |
| process.exit(1); |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/cli/index-cli.ts` around lines 101 - 103, The
console message that suggests "Run with --repair" is misleading because the CLI
uses the repair subcommand; update the log line that checks issues (the block
referencing issues.missing, issues.orphaned, issues.outdated, issues.corrupted
and the console.log call) to instruct users to run the repair subcommand instead
(for example: "Run 'index-cli repair' to fix these issues." or "Run the 'repair'
subcommand (e.g., index-cli repair).") so the message matches the actual CLI
invocation.
| case 'repair': | ||
| console.log('Verifying conversation index...'); | ||
| const repairIssues = await verifyIndex(); | ||
|
|
||
| if (repairIssues.missing.length + repairIssues.orphaned.length + repairIssues.outdated.length > 0) { | ||
| await repairIndex(repairIssues); | ||
| } else { | ||
| console.log('✅ No issues to repair!'); | ||
| } | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for the repairIndex function definition
ast-grep --pattern $'async function repairIndex($_) {
$$$
}'Repository: baleen37/claude-plugins
Length of output: 5030
🏁 Script executed:
# Search for corrupted references in the codebase
rg -n 'corrupted' --type=ts -C 3Repository: baleen37/claude-plugins
Length of output: 4003
🏁 Script executed:
# Check the verifyIndex function to see what fields it returns
ast-grep --pattern $'async function verifyIndex($_) {
$$$
}'Repository: baleen37/claude-plugins
Length of output: 49
🏁 Script executed:
# Look for type definitions related to verifyIndex or repairIssues
rg -n 'interface|type.*[Ii]ssues' --type=ts -C 2Repository: baleen37/claude-plugins
Length of output: 9356
🏁 Script executed:
sed -n '109,118p' plugins/conversation-memory/src/cli/index-cli.tsRepository: baleen37/claude-plugins
Length of output: 437
🏁 Script executed:
# Also get more context around the repair case
sed -n '105,125p' plugins/conversation-memory/src/cli/index-cli.tsRepository: baleen37/claude-plugins
Length of output: 686
repair case omits corrupted from the condition and will skip repair if only corrupted files exist.
The condition on line 113 checks missing.length + orphaned.length + outdated.length > 0 but excludes corrupted.length. When the only issues are corrupted files, repairIndex is never called and users see a false "✅ No issues to repair!" message. This is inconsistent with the verify case, which properly includes corrupted entries in its condition.
Fix
- if (repairIssues.missing.length + repairIssues.orphaned.length + repairIssues.outdated.length > 0) {
+ if (repairIssues.missing.length + repairIssues.orphaned.length + repairIssues.outdated.length + repairIssues.corrupted.length > 0) {📝 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.
| case 'repair': | |
| console.log('Verifying conversation index...'); | |
| const repairIssues = await verifyIndex(); | |
| if (repairIssues.missing.length + repairIssues.orphaned.length + repairIssues.outdated.length > 0) { | |
| await repairIndex(repairIssues); | |
| } else { | |
| console.log('✅ No issues to repair!'); | |
| } | |
| break; | |
| case 'repair': | |
| console.log('Verifying conversation index...'); | |
| const repairIssues = await verifyIndex(); | |
| if (repairIssues.missing.length + repairIssues.orphaned.length + repairIssues.outdated.length + repairIssues.corrupted.length > 0) { | |
| await repairIndex(repairIssues); | |
| } else { | |
| console.log('✅ No issues to repair!'); | |
| } | |
| break; |
🧰 Tools
🪛 Biome (2.3.13)
[error] 111-113: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/cli/index-cli.ts` around lines 109 - 118, In
the 'repair' switch case, the condition currently checks only
repairIssues.missing, .orphaned, and .outdated before deciding to call
repairIndex; include repairIssues.corrupted in that check so corrupted-only
issues trigger repairIndex; update the conditional in the case 'repair' block
(where verifyIndex() is called and repairIndex(repairIssues) is invoked) to sum
or otherwise test corrupted.length alongside the other arrays.
| case 'rebuild': | ||
| console.log('Rebuilding entire index...'); | ||
|
|
||
| // Delete database | ||
| const dbPath = getDbPath(); | ||
| if (fs.existsSync(dbPath)) { | ||
| fs.unlinkSync(dbPath); | ||
| console.log('Deleted existing database'); | ||
| } | ||
|
|
||
| // Delete all summary files | ||
| const archiveDir = getArchiveDir(); | ||
| if (fs.existsSync(archiveDir)) { | ||
| const projects = fs.readdirSync(archiveDir); | ||
| for (const project of projects) { | ||
| const projectPath = path.join(archiveDir, project); | ||
| if (!fs.statSync(projectPath).isDirectory()) continue; | ||
|
|
||
| const summaries = fs.readdirSync(projectPath).filter(f => f.endsWith('-summary.txt')); | ||
| for (const summary of summaries) { | ||
| fs.unlinkSync(path.join(projectPath, summary)); | ||
| } | ||
| } | ||
| console.log('Deleted all summary files'); | ||
| } | ||
|
|
||
| // Re-index everything | ||
| console.log('Re-indexing all conversations...'); | ||
| await indexConversations(undefined, undefined, concurrency, noSummaries); | ||
| break; |
There was a problem hiding this comment.
rebuild doesn't remove SQLite WAL/SHM journal files.
When SQLite runs in WAL mode, it creates <db>-wal and <db>-shm sidecar files. If only the main DB file is deleted, stale journal files may interfere with the freshly created database.
🛡️ Proposed fix
const dbPath = getDbPath();
if (fs.existsSync(dbPath)) {
fs.unlinkSync(dbPath);
+ // Clean up WAL/SHM journal files if present
+ for (const suffix of ['-wal', '-shm']) {
+ const journalPath = dbPath + suffix;
+ if (fs.existsSync(journalPath)) {
+ fs.unlinkSync(journalPath);
+ }
+ }
console.log('Deleted existing database');
}📝 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.
| case 'rebuild': | |
| console.log('Rebuilding entire index...'); | |
| // Delete database | |
| const dbPath = getDbPath(); | |
| if (fs.existsSync(dbPath)) { | |
| fs.unlinkSync(dbPath); | |
| console.log('Deleted existing database'); | |
| } | |
| // Delete all summary files | |
| const archiveDir = getArchiveDir(); | |
| if (fs.existsSync(archiveDir)) { | |
| const projects = fs.readdirSync(archiveDir); | |
| for (const project of projects) { | |
| const projectPath = path.join(archiveDir, project); | |
| if (!fs.statSync(projectPath).isDirectory()) continue; | |
| const summaries = fs.readdirSync(projectPath).filter(f => f.endsWith('-summary.txt')); | |
| for (const summary of summaries) { | |
| fs.unlinkSync(path.join(projectPath, summary)); | |
| } | |
| } | |
| console.log('Deleted all summary files'); | |
| } | |
| // Re-index everything | |
| console.log('Re-indexing all conversations...'); | |
| await indexConversations(undefined, undefined, concurrency, noSummaries); | |
| break; | |
| case 'rebuild': | |
| console.log('Rebuilding entire index...'); | |
| // Delete database | |
| const dbPath = getDbPath(); | |
| if (fs.existsSync(dbPath)) { | |
| fs.unlinkSync(dbPath); | |
| // Clean up WAL/SHM journal files if present | |
| for (const suffix of ['-wal', '-shm']) { | |
| const journalPath = dbPath + suffix; | |
| if (fs.existsSync(journalPath)) { | |
| fs.unlinkSync(journalPath); | |
| } | |
| } | |
| console.log('Deleted existing database'); | |
| } | |
| // Delete all summary files | |
| const archiveDir = getArchiveDir(); | |
| if (fs.existsSync(archiveDir)) { | |
| const projects = fs.readdirSync(archiveDir); | |
| for (const project of projects) { | |
| const projectPath = path.join(archiveDir, project); | |
| if (!fs.statSync(projectPath).isDirectory()) continue; | |
| const summaries = fs.readdirSync(projectPath).filter(f => f.endsWith('-summary.txt')); | |
| for (const summary of summaries) { | |
| fs.unlinkSync(path.join(projectPath, summary)); | |
| } | |
| } | |
| console.log('Deleted all summary files'); | |
| } | |
| // Re-index everything | |
| console.log('Re-indexing all conversations...'); | |
| await indexConversations(undefined, undefined, concurrency, noSummaries); | |
| break; |
🧰 Tools
🪛 Biome (2.3.13)
[error] 124-125: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 131-132: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/cli/index-cli.ts` around lines 120 - 149, The
rebuild branch currently deletes only the main DB file (dbPath) but leaves
SQLite WAL/SHM sidecars; update the case 'rebuild' logic to also remove the WAL
and SHM files derived from getDbPath() (e.g., `${dbPath}-wal` and
`${dbPath}-shm`) if they exist, using the same fs.existsSync/fs.unlinkSync
pattern; keep the existing behavior for deleting summaries and then call
indexConversations(...) as before so the fresh database is created without stale
journal files.
…migration Improvements based on episodic-memory comparison: 1. Inline Content Exclusion Markers - Add "DO NOT INDEX THIS CHAT" marker detection - Support Korean markers: "이 대화는 인덱싱하지 마세요" - ParseResult type with exclusion status - Skip excluded conversations during indexing 2. Project Filter in Search - Add `projects` parameter to MCP search tool - Add `--project` flag to CLI search command - Support multiple project filtering - Update SQL queries with project filter clause 3. CLI Wrapper Improvements - Add comprehensive help text with examples - Add bin entry point in package.json - Improve error messages - Add environment variable documentation 4. Bun Runtime Migration - Migrate build script to use Bun bundler - Update package.json scripts for Bun - Update hooks to use `bun` instead of `node` - Keep Node.js 18+ compatibility 5. Documentation Updates - Update README with new features - Add inline exclusion marker documentation - Add CLI usage examples - Update dependencies section Also includes: - verify/repair commands (already implemented) - index-session CLI command (already implemented) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The sdd command now explicitly instructs to load the @superpowers:subagent-driven-development skill first before proceeding with any implementation work. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@plugins/conversation-memory/README.md`:
- Line 13: The README currently claims "SessionEnd hook" for automatic indexing
but the implementation and later docs refer to the "SessionStart hook" that runs
on "startup|resume", causing a timing inconsistency; update the README text that
reads "SessionEnd hook" to "SessionStart hook (runs on startup|resume)" or
otherwise align the wording so the description of automatic indexing matches the
documented/implemented hook name and timing referenced elsewhere (search for
"SessionStart", "startup|resume", and the "SessionEnd hook" phrase to locate the
lines to update).
- Line 342: Add the missing "index-all" CLI command to the README's CLI Usage
section: locate the CLI Usage list in plugins/conversation-memory/README.md (the
block around the existing commands) and insert the "index-all" entry with the
same brief description used elsewhere ("Re-index all conversations (slow, use
with caution)"), ensuring formatting matches the other listed commands so the
documented commands are consistent with the upgrade guide.
- Around line 279-283: Update the README's core directory listing to match the
actual source files: replace the incorrect filenames `searcher.ts` ->
`search.ts` and `storage.ts` -> `db.ts`, and either expand the list to include
all actual files in the core directory or condense it to only the primary
modules (e.g., `indexer.ts`, `search.ts`, `db.ts`, `types.ts`) to avoid
misleading partial counts; verify the real filenames in the `core/` folder and
update the README text accordingly so the file references and counts match the
implementation.
In `@plugins/conversation-memory/src/core/indexer.ts`:
- Line 239: The call to summarizeConversation uses an undefined variable
exchanges; replace that with the parsed exchanges available as
parseResult.exchanges. Update the call in indexer.ts (where
summarizeConversation is invoked) to pass parseResult.exchanges instead of
exchanges, keeping the same other arguments (e.g., file). Ensure any variable
named exchanges in that scope is removed or replaced so the function receives
the correct data.
In `@plugins/conversation-memory/src/core/search.ts`:
- Around line 158-163: The returned object sets similarity to undefined when
mode === 'text', but SearchResult.similarity in types.ts is declared as number;
update either the type or the value: either make similarity optional/nullable in
the SearchResult interface (change similarity: number to similarity?: number |
null) or ensure the returned object always supplies a numeric value (e.g., set
similarity: mode === 'text' ? 0 : 1 - row.distance) in the function that returns
{ exchange, similarity, snippet, summary } in search.ts; pick one approach and
adjust all usages of SearchResult accordingly.
In `@plugins/conversation-memory/src/core/verify.ts`:
- Line 164: The call to summarizeConversation uses an undefined identifier
exchanges; replace it with the parsed result field parseResult.exchanges so the
correct variable is passed (update the call in verify.ts from
summarizeConversation(exchanges) to
summarizeConversation(parseResult.exchanges)); ensure any other references in
the surrounding function (e.g., where parseResult is defined) use
parseResult.exchanges consistently to avoid ReferenceError.
- Around line 126-185: The DB handle returned by initDatabase() can leak if any
async work (generateExchangeEmbedding, insertExchange, summarizeConversation,
parseConversationWithResult, etc.) throws; wrap the entire processing block that
deletes orphaned entries and re-indexes conversations in a try/finally (or use
try/catch/finally) so that db.close() is always called in the finally block,
while preserving the existing per-conversation try/catch for logging errors;
ensure deleteExchange and the re-index loop remain inside the try so failures
still get handled but db.close executes regardless.
In `@plugins/conversation-memory/src/mcp/server.ts`:
- Around line 192-214: The multi-concept branch builds an options object without
including the projects filter, so searchMultipleConcepts(params.query, options)
ignores params.projects; update the options object created in the
Array.isArray(params.query) branch to include projects: params.projects (same
key used in the single-concept path) so that searchMultipleConcepts receives the
project filter; ensure any downstream calls like formatMultiConceptResults and
the JSON response continue to use params.query and the results unchanged.
🧹 Nitpick comments (4)
plugins/me/commands/sdd.md (1)
9-11: Consider a more specific language identifier for the code block.The code block uses
textas the language identifier. While acceptable, consider usingbashorshif this is meant to be executed as a shell command, or omit the language identifier entirely if it's purely illustrative.📝 Alternative options
Option 1 - Shell command:
-```text +```bash `@superpowers`:subagent-driven-developmentOption 2 - No language identifier: ```diff -```text +``` `@superpowers`:subagent-driven-development</details> </blockquote></details> <details> <summary>plugins/conversation-memory/src/core/parser.ts (2)</summary><blockquote> `38-41`: **`hasExclusionMarker` is defined but never called — dead code.** The actual exclusion check is performed inline at lines 246-254. This function can be removed, or the inline check refactored to use it. --- `273-280`: **Hardcoded `/` separator won't split paths correctly on Windows.** `filePath.split('/')` assumes Unix paths. If cross-platform support matters, use `path.sep` or `path.parse`/`path.dirname`. <details> <summary>♻️ Proposed fix</summary> ```diff +import path from 'path'; ... export async function parseConversationFile(filePath: string): Promise<{ ... }> { - const pathParts = filePath.split('/'); - let project = 'unknown'; - - if (pathParts.length >= 2) { - project = pathParts[pathParts.length - 2]; - } + const project = path.basename(path.dirname(filePath)) || 'unknown';plugins/conversation-memory/README.md (1)
22-22: Consider simplifying "CLI Interface" to "CLI".The phrase "CLI Interface" is redundant since CLI already stands for "Command Line Interface". Consider using just "CLI" for cleaner documentation.
✏️ Suggested improvements
Line 22:
-- **CLI Interface**: Direct CLI access for manual operations +- **CLI**: Direct CLI access for manual operationsLine 244:
-The plugin provides a CLI interface for manual operations: +The plugin provides a CLI for manual operations:As per coding guidelines identified by static analysis tools.
Also applies to: 244-244
|
|
||
| ## Features | ||
|
|
||
| - **Automatic Indexing**: SessionEnd hook syncs conversations automatically |
There was a problem hiding this comment.
Fix the hook timing inconsistency.
Line 13 states "SessionEnd hook" but line 142 and line 277 indicate this is actually a "SessionStart hook" that runs on "startup|resume". This inconsistency could confuse users about when conversations are indexed.
📝 Proposed fix
-- **Automatic Indexing**: SessionEnd hook syncs conversations automatically
+- **Automatic Indexing**: SessionStart hook syncs conversations automatically📝 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.
| - **Automatic Indexing**: SessionEnd hook syncs conversations automatically | |
| - **Automatic Indexing**: SessionStart hook syncs conversations automatically |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/README.md` at line 13, The README currently
claims "SessionEnd hook" for automatic indexing but the implementation and later
docs refer to the "SessionStart hook" that runs on "startup|resume", causing a
timing inconsistency; update the README text that reads "SessionEnd hook" to
"SessionStart hook (runs on startup|resume)" or otherwise align the wording so
the description of automatic indexing matches the documented/implemented hook
name and timing referenced elsewhere (search for "SessionStart",
"startup|resume", and the "SessionEnd hook" phrase to locate the lines to
update).
| │ ├── core/ # Core library (from @obra/episodic-memory) | ||
| │ │ ├── indexer.ts # Conversation indexing | ||
| │ │ ├── searcher.ts # Semantic + text search | ||
| │ │ ├── storage.ts # SQLite + embeddings | ||
| │ │ └── types.ts # Type definitions |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List files in the core directory
echo "=== Files in plugins/conversation-memory/src/core/ ==="
fd -e ts -d 1 . plugins/conversation-memory/src/core/ 2>/dev/null || echo "Directory not found or no TypeScript files"
# Also check if the directory exists
echo -e "\n=== Directory structure check ==="
ls -la plugins/conversation-memory/src/core/ 2>/dev/null || echo "Directory does not exist"
# Check the overall structure
echo -e "\n=== Overall src structure ==="
ls -la plugins/conversation-memory/src/ 2>/dev/null || echo "src directory does not exist"Repository: baleen37/claude-plugins
Length of output: 2317
Update the core directory file references in the project structure documentation.
The documentation lists incorrect file names:
searcher.tsshould besearch.tsstorage.tsshould bedb.ts
Additionally, the documentation shows only 4 of 16 files in the core directory. Consider updating the structure to reflect the actual implementation or consolidating the list to show only the main files.
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/README.md` around lines 279 - 283, Update the
README's core directory listing to match the actual source files: replace the
incorrect filenames `searcher.ts` -> `search.ts` and `storage.ts` -> `db.ts`,
and either expand the list to include all actual files in the core directory or
condense it to only the primary modules (e.g., `indexer.ts`, `search.ts`,
`db.ts`, `types.ts`) to avoid misleading partial counts; verify the real
filenames in the `core/` folder and update the README text accordingly so the
file references and counts match the implementation.
| npm run build | ||
|
|
||
| # 5. Reindex all conversations (downloads ~197MB model on first run) | ||
| node dist/cli.mjs index-all |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for index-all command in CLI source and dist
rg -n "index-all" plugins/conversation-memory/Repository: baleen37/claude-plugins
Length of output: 408
Add index-all to the CLI Usage documentation.
The index-all command is correctly used in the upgrade guide (line 342) and is a valid, implemented command ("Re-index all conversations (slow, use with caution)"). However, it's not documented in the CLI Usage section (lines 246-267), creating an inconsistency in the README. Add it to the list of documented commands for completeness.
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/README.md` at line 342, Add the missing
"index-all" CLI command to the README's CLI Usage section: locate the CLI Usage
list in plugins/conversation-memory/README.md (the block around the existing
commands) and insert the "index-all" entry with the same brief description used
elsewhere ("Re-index all conversations (slow, use with caution)"), ensuring
formatting matches the other listed commands so the documented commands are
consistent with the upgrade guide.
| // Generate summary (unless --no-summaries) | ||
| const summaryPath = archivePath.replace('.jsonl', '-summary.txt'); | ||
| if (!noSummaries && !fs.existsSync(summaryPath)) { | ||
| const summary = await summarizeConversation(exchanges, undefined, file); |
There was a problem hiding this comment.
exchanges is undefined — same bug as in verify.ts.
summarizeConversation(exchanges, ...) references an undeclared variable. The exchanges are available as parseResult.exchanges from line 227.
🐛 Proposed fix
- const summary = await summarizeConversation(exchanges, undefined, file);
+ const summary = await summarizeConversation(parseResult.exchanges, undefined, file);📝 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.
| const summary = await summarizeConversation(exchanges, undefined, file); | |
| const summary = await summarizeConversation(parseResult.exchanges, undefined, file); |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/indexer.ts` at line 239, The call to
summarizeConversation uses an undefined variable exchanges; replace that with
the parsed exchanges available as parseResult.exchanges. Update the call in
indexer.ts (where summarizeConversation is invoked) to pass
parseResult.exchanges instead of exchanges, keeping the same other arguments
(e.g., file). Ensure any variable named exchanges in that scope is removed or
replaced so the function receives the correct data.
| return { | ||
| exchange, | ||
| similarity: mode === 'text' ? undefined : 1 - row.distance, | ||
| snippet, | ||
| summary | ||
| } as SearchResult & { summary?: string }; |
There was a problem hiding this comment.
similarity is undefined for text-mode results, but SearchResult.similarity is typed as number.
Line 160 assigns undefined when mode === 'text', but the SearchResult interface (from types.ts) defines similarity: number — not optional. The as cast on line 163 masks this. Consider making similarity optional in the interface, or assigning a default value (e.g., 0) for text results.
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/search.ts` around lines 158 - 163, The
returned object sets similarity to undefined when mode === 'text', but
SearchResult.similarity in types.ts is declared as number; update either the
type or the value: either make similarity optional/nullable in the SearchResult
interface (change similarity: number to similarity?: number | null) or ensure
the returned object always supplies a numeric value (e.g., set similarity: mode
=== 'text' ? 0 : 1 - row.distance) in the function that returns { exchange,
similarity, snippet, summary } in search.ts; pick one approach and adjust all
usages of SearchResult accordingly.
| const db = initDatabase(); | ||
| await initEmbeddings(); | ||
|
|
||
| // Remove orphaned entries first | ||
| for (const orphan of issues.orphaned) { | ||
| console.log(`Removing orphaned entry: ${orphan.uuid}`); | ||
| deleteExchange(db, orphan.uuid); | ||
| } | ||
|
|
||
| // Re-index missing and outdated conversations | ||
| const toReindex = [ | ||
| ...issues.missing.map(m => m.path), | ||
| ...issues.outdated.map(o => o.path) | ||
| ]; | ||
|
|
||
| for (const conversationPath of toReindex) { | ||
| console.log(`Re-indexing: ${conversationPath}`); | ||
| try { | ||
| // Extract project name from path | ||
| const archiveDir = getArchiveDir(); | ||
| const relativePath = conversationPath.replace(archiveDir + path.sep, ''); | ||
| const project = relativePath.split(path.sep)[0]; | ||
|
|
||
| // Parse conversation | ||
| const parseResult = await parseConversationWithResult(conversationPath, project, conversationPath); | ||
|
|
||
| if (parseResult.isExcluded) { | ||
| console.log(` Skipped (excluded: ${parseResult.exclusionReason})`); | ||
| continue; | ||
| } | ||
|
|
||
| if (parseResult.exchanges.length === 0) { | ||
| console.log(` Skipped (no exchanges)`); | ||
| continue; | ||
| } | ||
|
|
||
| // Generate/update summary | ||
| const summaryPath = conversationPath.replace('.jsonl', '-summary.txt'); | ||
| const summary = await summarizeConversation(exchanges); | ||
| fs.writeFileSync(summaryPath, summary, 'utf-8'); | ||
| console.log(` Created summary: ${summary.split(/\s+/).length} words`); | ||
|
|
||
| // Index exchanges | ||
| for (const exchange of parseResult.exchanges) { | ||
| const toolNames = exchange.toolCalls?.map(tc => tc.toolName); | ||
| const embedding = await generateExchangeEmbedding( | ||
| exchange.userMessage, | ||
| exchange.assistantMessage, | ||
| toolNames | ||
| ); | ||
| insertExchange(db, exchange, embedding, toolNames); | ||
| } | ||
|
|
||
| console.log(` Indexed ${parseResult.exchanges.length} exchanges`); | ||
| } catch (error) { | ||
| console.error(`Failed to re-index ${conversationPath}:`, error); | ||
| } | ||
| } | ||
|
|
||
| db.close(); |
There was a problem hiding this comment.
Database handle leaks if the re-indexing loop throws.
If generateExchangeEmbedding, insertExchange, or summarizeConversation throws, db.close() on line 185 is never reached. Wrap in try/finally.
🐛 Proposed fix
const db = initDatabase();
await initEmbeddings();
+ try {
// Remove orphaned entries first
...
// (all processing logic)
...
- db.close();
+ } finally {
+ db.close();
+ }🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/verify.ts` around lines 126 - 185, The
DB handle returned by initDatabase() can leak if any async work
(generateExchangeEmbedding, insertExchange, summarizeConversation,
parseConversationWithResult, etc.) throws; wrap the entire processing block that
deletes orphaned entries and re-indexes conversations in a try/finally (or use
try/catch/finally) so that db.close() is always called in the finally block,
while preserving the existing per-conversation try/catch for logging errors;
ensure deleteExchange and the re-index loop remain inside the try so failures
still get handled but db.close executes regardless.
|
|
||
| // Generate/update summary | ||
| const summaryPath = conversationPath.replace('.jsonl', '-summary.txt'); | ||
| const summary = await summarizeConversation(exchanges); |
There was a problem hiding this comment.
exchanges is undefined — this will throw a ReferenceError at runtime.
Line 164 calls summarizeConversation(exchanges), but exchanges is not declared in this scope. It should be parseResult.exchanges. The variable is declared on line 150 as parseResult.
🐛 Proposed fix
- const summary = await summarizeConversation(exchanges);
+ const summary = await summarizeConversation(parseResult.exchanges);📝 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.
| const summary = await summarizeConversation(exchanges); | |
| const summary = await summarizeConversation(parseResult.exchanges); |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/verify.ts` at line 164, The call to
summarizeConversation uses an undefined identifier exchanges; replace it with
the parsed result field parseResult.exchanges so the correct variable is passed
(update the call in verify.ts from summarizeConversation(exchanges) to
summarizeConversation(parseResult.exchanges)); ensure any other references in
the surrounding function (e.g., where parseResult is defined) use
parseResult.exchanges consistently to avoid ReferenceError.
| if (Array.isArray(params.query)) { | ||
| // Multi-concept search | ||
| const options = { | ||
| limit: params.limit, | ||
| after: params.after, | ||
| before: params.before, | ||
| }; | ||
|
|
||
| const results = await searchMultipleConcepts(params.query, options); | ||
|
|
||
| if (params.response_format === 'json') { | ||
| resultText = JSON.stringify( | ||
| { | ||
| results: results, | ||
| count: results.length, | ||
| concepts: params.query, | ||
| }, | ||
| null, | ||
| 2 | ||
| ); | ||
| } else { | ||
| resultText = await formatMultiConceptResults(results, params.query); | ||
| } |
There was a problem hiding this comment.
Multi-concept search silently drops the projects filter.
The single-concept path (line 222) correctly passes projects: params.projects, but the multi-concept path constructs options without projects. Users filtering by project will get unfiltered multi-concept results.
🐛 Proposed fix
const options = {
limit: params.limit,
after: params.after,
before: params.before,
+ projects: params.projects,
};📝 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.
| if (Array.isArray(params.query)) { | |
| // Multi-concept search | |
| const options = { | |
| limit: params.limit, | |
| after: params.after, | |
| before: params.before, | |
| }; | |
| const results = await searchMultipleConcepts(params.query, options); | |
| if (params.response_format === 'json') { | |
| resultText = JSON.stringify( | |
| { | |
| results: results, | |
| count: results.length, | |
| concepts: params.query, | |
| }, | |
| null, | |
| 2 | |
| ); | |
| } else { | |
| resultText = await formatMultiConceptResults(results, params.query); | |
| } | |
| if (Array.isArray(params.query)) { | |
| // Multi-concept search | |
| const options = { | |
| limit: params.limit, | |
| after: params.after, | |
| before: params.before, | |
| projects: params.projects, | |
| }; | |
| const results = await searchMultipleConcepts(params.query, options); | |
| if (params.response_format === 'json') { | |
| resultText = JSON.stringify( | |
| { | |
| results: results, | |
| count: results.length, | |
| concepts: params.query, | |
| }, | |
| null, | |
| 2 | |
| ); | |
| } else { | |
| resultText = await formatMultiConceptResults(results, params.query); | |
| } |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/mcp/server.ts` around lines 192 - 214, The
multi-concept branch builds an options object without including the projects
filter, so searchMultipleConcepts(params.query, options) ignores
params.projects; update the options object created in the
Array.isArray(params.query) branch to include projects: params.projects (same
key used in the single-concept path) so that searchMultipleConcepts receives the
project filter; ensure any downstream calls like formatMultiConceptResults and
the JSON response continue to use params.query and the results unchanged.
…ules Add 500+ tests across 15 test files covering: - Core modules: db, parser, embeddings, paths, search, sync, stats, show - MCP server: tool validation and error handling - CLI commands: sync, search, show, stats - Test utilities: helpers and data factories Infrastructure: - test-all.sh: unified test runner for Bun and Vitest - Export show.ts functions and MCP schemas for testing - Add test:all, test:bun, test:vitest npm scripts - Fix ShellCheck warnings in test-all.sh All tests pass: 502 pass, 5 skip Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@plugins/conversation-memory/package.json`:
- Around line 19-27: The dependency range for `@modelcontextprotocol/sdk` in
package.json is too broad and may conflict with the zod peer dependency; update
package.json so either pin `@modelcontextprotocol/sdk` to the tested version
(e.g., "1.0.4" or "~1.0.4") or upgrade both `@modelcontextprotocol/sdk` and zod
together (e.g., set `@modelcontextprotocol/sdk` to a specific recent version like
"1.26.0" or a caret if desired, and set zod to ">=3.25.0" such as "^3.25.0") to
satisfy the SDK's peer-dep requirement and avoid resolution to zod 3.22.x.
In `@plugins/conversation-memory/src/cli/stats-cli.test.ts`:
- Around line 16-45: Tests in stats-cli.test.ts are vacuous because they only
assert JavaScript Array behavior instead of exercising the CLI's argument
parsing; replace or remove them and add real tests that call the CLI parser or
run the binary. Update the test suite to import and call the actual argument
parser (e.g., a parseArgs or parseStatsCliArgs function) or spawn the CLI entry
point (e.g., the stats-cli module) with process-like argv values, then assert
outcomes for --help, -h, no args, and unexpected positional args; remove the
current fixture tests that only use local arrays (the describe blocks testing
includes/length) so the tests cover real code paths in the stats-cli parsing
logic.
- Around line 352-392: The current tests in the "stats-cli help output" suite
only assert against hard-coded inline strings and never invoke the real CLI
help, so replace them to call the actual help output: run the CLI with "--help"
(e.g., use child_process.execSync or spawnSync to invoke the built command
`conversation-memory stats --help`) or, if the module exports a help function,
call that exported help/getHelpText function (look for exported symbols in the
stats CLI module). Capture the stdout returned and assert that it contains the
real usage lines such as "Usage:", "conversation-memory stats", "EXAMPLES:", and
the specific sections ("Total conversations and exchanges", "Conversations
with/without AI summaries", etc.) instead of comparing to an inline string
literal.
- Around line 334-350: Replace the vacuous tests in stats-cli.test.ts with real
error-path tests: remove the inline-literal assertions and either (A) invoke the
CLI entrypoint (e.g., run the stats CLI via child_process.exec or spawn) with an
invalid DB path to cause getIndexStats to throw, then assert the process exits
with code 1 and stderr contains "Error getting stats:"; or (B) unit-test by
mocking the getIndexStats function to throw and spyOn process.exit and
console/error output (or capture stderr) to assert the error message and exit
code; target the getIndexStats symbol and the CLI entrypoint/process.exit
behavior when replacing the tests.
- Around line 1-14: The test file only imports core helpers (getIndexStats,
formatStats, IndexStats) and never exercises the CLI wiring; update the test to
import and invoke the actual CLI module (stats-cli.ts) or its exported handler
(e.g., runStatsCommand, main, or handleStatsArgs) so the integration is
validated: mock/spy getIndexStats and formatStats, simulate argv values, capture
stdout/stderr, and stub process.exit to assert exit codes and printed output;
alternatively if the CLI has no testable export, add a small export (runStatsCli
or handleStatsArgs) in stats-cli.ts and call that from this test instead of
duplicating core tests, or remove this file if it only duplicates coverage from
stats.test.ts. Ensure references to getIndexStats, formatStats, IndexStats and
the CLI export name (stats-cli.ts / runStatsCommand / main) are used so the
correct modules are located.
- Around line 239-240: The test assumes US date formatting ('1/1/2025' /
'1/31/2025') which breaks in other locales; update either the production
formatter or the test: either (A) change formatStats to call toLocaleDateString
with a fixed locale (e.g., toLocaleDateString('en-US')) so output is
deterministic, or (B) make the test locale-agnostic by generating the expected
strings using the same formatting used in production (call the same
toLocaleDateString on your test Date objects or derive a regex that matches
day/month/year numeric patterns) and then assert output contains those computed
values; reference formatStats and the assertions in stats-cli.test.ts when
applying the change.
In `@plugins/conversation-memory/src/cli/sync-cli.test.ts`:
- Line 47: The test assertion using expect(Array.isArray(result.errors)) is
missing a matcher and does nothing; update the test in sync-cli.test.ts (the
expect call that checks result.errors) to use a proper matcher such as asserting
truthiness or array type (for example, use toBe(true) on
Array.isArray(result.errors) or use toBeInstanceOf(Array) /
toEqual(expect.any(Array)) on result.errors) so the assertion actually validates
that result.errors is an array.
- Around line 212-223: The test builds tokenOutput using toLocaleString()
causing locale-dependent formatting; update usages of
totalInput.toLocaleString(), totalOutput.toLocaleString(), and
cacheCreate.toLocaleString() in the tokenOutput assembly (or the expectations)
to use an explicit locale (e.g., toLocaleString('en-US')) or compare against raw
numeric values instead (e.g., assert totalInput === 1100 etc.); change the
assertions (expect(tokenOutput).toContain(...)) accordingly so they no longer
depend on the environment locale and reference the tokenOutput variable and the
totalInput/totalOutput/cacheCreate symbols when making the adjustment.
In `@plugins/conversation-memory/src/core/show.test.ts`:
- Around line 207-211: The test reveals renderMarkdownSafely currently allows
raw HTML; update the production implementation in show.ts (renderMarkdownSafely
used by formatConversationAsHTML) to sanitize output—use a sanitizer like
DOMPurify on the HTML returned by marked (or enable marked's sanitize option
and/or a post-process with DOMPurify) so any <script> tags or unsafe attributes
are stripped; then update this test in show.test.ts to assert the script
tag/content is removed (e.g.,
expect(result).not.toContain('<script>alert("xss")</script>') and expect allowed
markdown to still render).
In `@plugins/conversation-memory/src/core/show.ts`:
- Line 37: Replace the direct JSON.parse mapping that builds allMessages in both
formatConversationAsMarkdown and formatConversationAsHTML with a safe-parse
loop: iterate over lines, try JSON.parse for each line inside a try/catch, push
the parsed ConversationMessage into allMessages only on success, and silently
skip (or optionally log) any malformed lines to match parser.ts behavior so a
single bad JSONL line won't throw and abort processing.
In `@plugins/conversation-memory/src/mcp/server.test.ts`:
- Around line 43-109: Remove the duplicated local schema definitions
(SearchInputSchema and ShowConversationInputSchema) and import those exact
exported symbols from server.ts instead; replace the block that redefines
SearchModeEnum/ResponseFormatEnum/SearchInputSchema/ShowConversationInputSchema
with import statements pulling SearchInputSchema and ShowConversationInputSchema
from server.ts, update any references in this test file to use the imported
names, and run tests to ensure no build/import issues remain.
In `@plugins/conversation-memory/test-all.sh`:
- Around line 42-50: The print_error and print_warning functions currently write
to stdout; change them to emit their messages to stderr by redirecting the echo
output within print_error and print_warning (reference the functions print_error
and print_warning) so that error and warning messages are sent to stderr (e.g.,
use shell redirection to >&2 inside each function) while keeping the same
message formatting and variables (RED, YELLOW, NC).
🟡 Minor comments (8)
plugins/conversation-memory/src/cli/stats-cli.test.ts-294-297 (1)
294-297:⚠️ Potential issue | 🟡 MinorComment says "right-padded" but the code uses
padStart(left-padding).Line 294:
// Counts should be right-padded to 4 characters—padStartleft-pads (right-aligns) the number. The comment should say "left-padded" or "right-aligned".plugins/conversation-memory/src/cli/stats-cli.test.ts-394-466 (1)
394-466:⚠️ Potential issue | 🟡 MinorCalculation and formatting tests exercise JavaScript built-ins, not application code.
This entire block tests
Number.toLocaleString(),String.padStart(), and basic arithmetic. These are language-level guarantees, not application behavior.Additionally:
- Line 436:
expect(formatted).toBe('1,234,567')is locale-dependent — same issue as the date formatting concern above.- Line 460: Test name says "should truncate 5-digit numbers when padding" but the assertion verifies the opposite — that
padStartdoes not truncate. The name is misleading.Fix the misleading test name
- test('should truncate 5-digit numbers when padding', () => { + test('should not truncate numbers longer than pad width', () => {plugins/conversation-memory/src/cli/search-cli.test.ts-362-363 (1)
362-363:⚠️ Potential issue | 🟡 MinorHardcoded
/tmp/paths risk collisions in parallel test runs.
/tmp/test-archive.jsonland/tmp/test-multi.jsonlare fixed paths. If tests run concurrently, they could collide. Usefs.mkdtempSync(as done in other test files in this PR) to create unique temp directories.Suggested approach
- const archivePath = '/tmp/test-archive.jsonl'; - fs.writeFileSync(archivePath, 'line1\nline2\n'); + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'search-test-')); + const archivePath = path.join(tempDir, 'test-archive.jsonl'); + fs.writeFileSync(archivePath, 'line1\nline2\n');Apply similarly for
test-multi.jsonl. Clean up withfs.rmSync(tempDir, { recursive: true })in thefinallyblock.Also applies to: 400-401
plugins/conversation-memory/src/mcp/server.test.ts-112-166 (1)
112-166:⚠️ Potential issue | 🟡 Minor
paramsis assigned but never used inmockToolCall.On line 115,
const params = SearchInputSchema.parse(args)parses and validates the input, butparamsis never referenced — line 122 checksargs.response_format(the raw, unparsed input) instead. Similarly on line 132,const params = ShowConversationInputSchema.parse(args)is unused (line 136 reads fromparams.pathbut only in a no-op conditional).If you intend to validate via Zod (which you do), consider using
paramsfor subsequent logic so defaults are respected:Suggested fix
if (toolName === 'search') { const params = SearchInputSchema.parse(args); // Return success response (mocked) return { content: [ { type: 'text', - text: args.response_format === 'json' + text: params.response_format === 'json' ? JSON.stringify({ results: [], count: 0 }, null, 2) : 'No results found.' } ], isError: false }; }plugins/conversation-memory/src/core/sync.test.ts-121-121 (1)
121-121:⚠️ Potential issue | 🟡 MinorTypo in test description:
MEMODYshould beMEMORY.The closing tag in the test name reads
</INSTRUCTIONS-TO-EPISODIC-MEMODY>instead of</INSTRUCTIONS-TO-EPISODIC-MEMORY>. The actual test content on line 130 uses the correct spelling.- test('detects <INSTRUCTIONS-TO-EPISODIC-MEMORY>DO NOT INDEX THIS CHAT</INSTRUCTIONS-TO-EPISODIC-MEMODY> marker', async () => { + test('detects <INSTRUCTIONS-TO-EPISODIC-MEMORY>DO NOT INDEX THIS CHAT</INSTRUCTIONS-TO-EPISODIC-MEMORY> marker', async () => {plugins/conversation-memory/src/mcp/server.ts-75-94 (1)
75-94:⚠️ Potential issue | 🟡 MinorNo cross-field validation that
startLine ≤ endLine.Both fields are individually validated (
min(1)) and optional, but when both are provided there's no.refine()ensuringstartLine <= endLine. PassingstartLine: 10, endLine: 2would silently produce unexpected output depending onformatConversationAsMarkdown's behavior.🛡️ Suggested refinement
.strict() + .refine( + (data) => !data.startLine || !data.endLine || data.startLine <= data.endLine, + { message: 'startLine must be less than or equal to endLine' } + )plugins/conversation-memory/src/core/stats.test.ts-120-126 (1)
120-126:⚠️ Potential issue | 🟡 MinorThis assertion always passes —
expect(async fn).not.toThrow()doesn't catch async rejections.
expect(fn).not.toThrow()callsfnsynchronously and checks for a synchronous throw. Sincefnis async, it returns a Promise (never throws synchronously), so the assertion always passes regardless of whether the Promise rejects.🐛 Proposed fix
test('should not throw error for missing database', async () => { const nonExistentPath = path.join(tmpdir(), `non-existent-${Date.now()}.db`); - - expect(async () => { - await getIndexStats(nonExistentPath); - }).not.toThrow(); + // Simply await — if it rejects, the test fails + const stats = await getIndexStats(nonExistentPath); + expect(stats).toBeDefined(); });plugins/conversation-memory/src/core/search.test.ts-20-60 (1)
20-60:⚠️ Potential issue | 🟡 MinorDatabase handle leaked in
validateISODatetests.
createTestDatabase()sets the module-leveltestDbPathas a side effect but only returns theDatabase— it never assignstestDb. ThevalidateISODatedescribe block (lines 112-259) captures the return into a localconst dbbut never setstestDb. WhencleanupTestDatabase()runs, theif (testDb)guard at line 54 is falsy, sodb.close()is never called, leaking the SQLite file handle.Either assign
testDbinsidecreateTestDatabase()or explicitly close the localdbin each test'sfinallyblock.Proposed fix — assign testDb inside createTestDatabase
function createTestDatabase(): Database { testDbPath = path.join(tmpdir(), `test-search-${Date.now()}.db`); const db = new Database(testDbPath); + testDb = db; // Create test schema ...
🧹 Nitpick comments (20)
plugins/conversation-memory/src/cli/stats-cli.test.ts (1)
100-136: Tests only assert on locally constructed objects — no code under test.Lines 100–135 construct
IndexStatsobjects inline and then assert the same values that were just assigned. These are effectively compile-time type checks (verifying the shape of the interface), not behavioral tests. They provide zero regression protection.If the goal is to verify the type shape, a simple
satisfies IndexStatsassertion at compile time would suffice. Otherwise, remove these in favor of tests that exercise actual code paths.plugins/conversation-memory/src/cli/sync-cli.test.ts (1)
82-148: Tests verify inline arithmetic and built-in JS methods, not actual production code.The token-usage tests (lines 84–148) assert that
1000 + 200 === 1200, and the argument-parsing tests (lines 243–274) assert thatArray.includesandArray.filterwork correctly. Neither exercises any function exported from the CLI or core modules, so they cannot catch regressions.As acknowledged in the file header (lines 7–9), the CLI needs to export its formatting and argument-parsing helpers for meaningful test coverage. Consider extracting those helpers and testing them directly, or removing these placeholder tests to avoid giving a false sense of coverage.
Also applies to: 241-275
plugins/conversation-memory/package.json (1)
33-35: Node 18 is past end-of-life; consider bumping the engine requirement.Node.js 18 reached EOL in April 2025. The
@types/nodedev dependency already targets^20.0.0, which suggests the codebase is realistically targeting Node 20+. Aligning theenginesfield avoids silently running on an unsupported runtime.🔧 Proposed fix
"engines": { - "node": ">=18.0.0" + "node": ">=20.0.0" }plugins/conversation-memory/src/cli/search-cli.test.ts (2)
14-20: Unused imports:searchConversationsandsearchMultipleConceptsare imported but never called.These two functions are imported on lines 15-16 but no test in this file actually invokes them. The "execution paths" tests (lines 284-325) only construct local variables. Either remove these imports or write tests that actually call through the production search functions.
23-265: Tests re-implement argument parsing inline instead of testing the actual CLI parser.Every test in this block duplicates a
forloop that manually parsesargs, rather than importing and invoking the real parsing logic fromsearch-cli.ts. These tests will pass regardless of whether the actual CLI has a bug in its argument parsing. Consider extracting the CLI's argument-parsing logic into a testable function (e.g.,parseSearchArgs(args: string[])) and testing that instead.plugins/conversation-memory/src/core/embeddings.test.ts (2)
219-297: Tautological tests provide no coverage.Tests in "embedding vector specifications" (lines 219-251), "embedding model configuration" (lines 253-273), and "embedding generation options" (lines 275-297) create local literals and assert they equal themselves. For example:
const dimension = 768; expect(dimension).toBe(768);These will never fail and don't exercise any production code. They only document intended values. Consider either:
- Importing and asserting against actual exported constants/config from
embeddings.ts, or- Removing these tests and keeping the values documented in code comments or a constants file.
81-181: Text formatting tests mirror production logic but don't call it.These tests reconstruct the formatting logic inline (e.g., template literals for
User:/Assistant:/Tools:sections). If the productiongenerateExchangeEmbeddingchanges its format, these tests won't catch the drift. Consider exporting a helper (e.g.,formatExchangeText) fromembeddings.tsand testing it directly.plugins/conversation-memory/src/core/paths.test.ts (1)
114-138: Side effect: tests create~/.config/conversation-memoryon the host filesystem.When
CONVERSATION_MEMORY_CONFIG_DIRis unset,getSuperpowersDir()falls back to~/.config/conversation-memoryand creates it. This means running these tests on a developer's machine will create that directory as a side effect. The comment on lines 132-133 acknowledges this. If this is undesirable, these two tests could setCONVERSATION_MEMORY_CONFIG_DIRto a temp path instead (though that would defeat the purpose of testing the default).Worth noting but likely acceptable for an integration-style test.
plugins/conversation-memory/src/core/parser.test.ts (1)
527-543: Weak assertion for malformed JSON test.Line 542 uses
toBeGreaterThanOrEqual(1)which is overly permissive. The test data has two valid user→assistant pairs (lines 1+3, 5+6, with malformed lines 2 and 4 skipped). A more precise assertion would better document the expected behavior and catch regressions.Suggested fix
- // Should parse the valid exchanges and skip invalid JSON lines - expect(exchanges.length).toBeGreaterThanOrEqual(1); + // Should parse the 2 valid user-assistant exchange pairs, skipping invalid JSON lines + expect(exchanges.length).toBe(2);plugins/conversation-memory/src/core/show.ts (2)
163-193: Near-identical tool result lookup logic is duplicated between the two formatters.The forward-scan for
tool_resultblocks (lines 163-193 in Markdown, 407-441 in HTML) follows the same pattern: search up to 5 subsequent messages for a matchingtool_use_id. Consider extracting this into a shared helper to reduce duplication and ensure consistency.Similarly, the message filtering logic (lines 40-52 vs 228-248) is effectively identical.
Also applies to: 407-441
170-172: Variablecontentshadows the outercontentdeclaration.Line 172 declares
const content = (resultBlock as any).contentwhich shadows thecontentfrom line 138 (const content = msg.message.content). Same pattern occurs at line 419 in the HTML formatter. Not a bug due to block scoping, but rename to e.g.resultContentfor clarity.plugins/conversation-memory/src/cli/show-cli.test.ts (2)
19-155: Same pattern as search-cli.test.ts: argument parsing duplicated inline.These tests re-implement the format/file-path parsing loop locally instead of testing the actual CLI's exported parsing function. They won't detect real regressions in
show-cli.ts. Same recommendation applies: extract the CLI's arg parsing into a testable function and import it.
492-562: Error handling and help tests are tautological — they test hardcoded local strings.Lines 492-562 construct error/help message strings locally and assert their contents. These never exercise any production code path from
show-cli.ts. Consider either removing them or testing the actual CLI's error/help output (e.g., by invoking the CLI'smainorshowHelpfunction).plugins/conversation-memory/src/core/db.test.ts (2)
708-711: Busy-wait spin loop wastes CPU; prefer a shortsleep-based delay.The
while (Date.now() - start < 2)loop spins the CPU. In a test environment this works but is wasteful. Consider using a promise-based delay or accept thatDate.now()granularity might not differ in 2ms.♻️ Suggested fix
- // Small delay to ensure different timestamps - const start = Date.now(); - while (Date.now() - start < 2) { - // Wait at least 2ms - } + // Small delay to ensure different timestamps + await new Promise(resolve => setTimeout(resolve, 5));Note: This would require making the test
async. Alternatively, sinceinsertExchangeusesDate.now()internally and sequential calls will produce different (or equal) timestamps, you could simply accept this by inserting at least one statement between the two calls to give the clock a chance to tick.
217-222: Consider addingafterEachto close the DB instance.The in-memory database opened in
beforeEachis never explicitly closed. While GC handles in-memory DBs, explicit cleanup prevents resource leaks if the test count grows and avoids potential issues with better-sqlite3's native bindings.♻️ Suggested fix
beforeEach(() => { db = initTestDatabase(); }); + + afterEach(() => { + db.close(); + });You'll also need to add
afterEachto the import:-import { describe, test, expect, beforeEach } from 'vitest'; +import { describe, test, expect, beforeEach, afterEach } from 'vitest';plugins/conversation-memory/tests/test-utils.ts (1)
427-438:createTempConversationFileleaks the temp directory path.
createTempDir()is called internally but only the file path is returned. The caller cannot clean up the parent directory. The docstring example suggestsfs.unlinkSync(filePath), which leaves the directory behind.Consider returning both paths or accepting an existing directory as a parameter:
♻️ Option: accept an optional directory parameter
export function createTempConversationFile( content: string, - filename?: string + filename?: string, + dir?: string ): string { - const tempDir = createTempDir(); + const tempDir = dir ?? createTempDir(); const actualFilename = filename || `test-${Date.now()}.jsonl`; const filePath = path.join(tempDir, actualFilename);This lets callers that manage their own temp directories (e.g., via
createTempDir()+cleanupTempDir()inafterEach) avoid orphaned directories.plugins/conversation-memory/test-all.sh (1)
119-119: UnnecessaryexportonBUN_TESTS_FAILEDandVITEST_TESTS_FAILED.These variables are only used within this script.
exportmakes them available to child processes, which is unneeded and inconsistent withBUN_TESTS_PASSED/VITEST_TESTS_PASSED(which are not exported).- export BUN_TESTS_FAILED=1 + BUN_TESTS_FAILED=1- export VITEST_TESTS_FAILED=1 + VITEST_TESTS_FAILED=1Also applies to: 184-184
plugins/conversation-memory/src/core/search.test.ts (3)
109-260: Reduce boilerplate — extract shared mock setup into a helper orbeforeEach.Every test in this block repeats the same ~10-line mock-setup / teardown pattern. A shared helper (or a nested
beforeEach/afterEach) would eliminate ~50 lines of duplication and make it easier to update mock wiring later.Additionally,
expect(true).toBe(false)(e.g., lines 150, 176, 203, 228) is a weak "should-not-reach" guard — if it fires, the failure message is just"expected true to be false". Considerthrow new Error("Expected searchConversations to throw")for a clearer failure message, or use Bun'sexpect(fn).toThrow()pattern instead of try/catch.
490-491: Hardcoded/tmp/paths risk collisions in parallel test runs.Throughout the
formatResultsand later test blocks, archive files are created at fixed paths like/tmp/test-archive.jsonl. If tests run in parallel (or other test files use the same names), they'll clobber each other. The DB helper already usestmpdir()+Date.now()for uniqueness — apply the same pattern here.Example fix
- const archivePath = '/tmp/test-archive.jsonl'; + const archivePath = path.join(tmpdir(), `test-archive-${Date.now()}-${Math.random().toString(36).slice(2)}.jsonl`);Then update the
mockExchange.archivePathto match. The same treatment is needed for all/tmp/test-*.jsonlpaths in this file.Also applies to: 515-516, 565-566, 599-600, 632-633, 681-682, 711-712
93-102:insertTestEmbeddingis unused in this file.This helper is defined but never called. If it's not needed here, consider removing it to avoid dead code. If it's intended for future vector-search tests, a comment noting that (or moving it to the shared
test-utils.ts) would clarify intent.
| "dependencies": { | ||
| "@anthropic-ai/claude-agent-sdk": "^0.1.9", | ||
| "@huggingface/transformers": "^3.8.1", | ||
| "@modelcontextprotocol/sdk": "^1.0.4", | ||
| "better-sqlite3": "^9.6.0", | ||
| "marked": "^17.0.1", | ||
| "sqlite-vec": "^0.1.6", | ||
| "zod": "^3.22.4" | ||
| }, |
There was a problem hiding this comment.
@modelcontextprotocol/sdk version range is very broad; potential zod peer-dep incompatibility.
@modelcontextprotocol/sdk latest version is 1.26.0. The ^1.0.4 range allows resolution anywhere from 1.0.4 to 1.26.0, which spans significant API evolution. More critically, the SDK has a required peer dependency on zod, and "maintains backwards compatibility with projects using Zod v3.25 or later". This means newer resolved versions of the SDK require zod >= 3.25, but the current zod: "^3.22.4" constraint could resolve to 3.22.x–3.24.x on a fresh install, causing peer-dependency warnings or runtime failures.
Consider:
- Pinning
@modelcontextprotocol/sdkto the exact version you've tested against (e.g.,"~1.0.4"or"1.0.4"), or bumping both to current versions. - If bumping to a recent SDK version, also bump
zodto"^3.25.0"to satisfy the peer dependency.
🔧 Option A: Pin to tested version
- "@modelcontextprotocol/sdk": "^1.0.4",
+ "@modelcontextprotocol/sdk": "~1.0.4",🔧 Option B: Bump both to current
- "@modelcontextprotocol/sdk": "^1.0.4",
+ "@modelcontextprotocol/sdk": "^1.26.0",
- "zod": "^3.22.4"
+ "zod": "^3.25.0"📝 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.
| "dependencies": { | |
| "@anthropic-ai/claude-agent-sdk": "^0.1.9", | |
| "@huggingface/transformers": "^3.8.1", | |
| "@modelcontextprotocol/sdk": "^1.0.4", | |
| "better-sqlite3": "^9.6.0", | |
| "marked": "^17.0.1", | |
| "sqlite-vec": "^0.1.6", | |
| "zod": "^3.22.4" | |
| }, | |
| "dependencies": { | |
| "@anthropic-ai/claude-agent-sdk": "^0.1.9", | |
| "@huggingface/transformers": "^3.8.1", | |
| "@modelcontextprotocol/sdk": "~1.0.4", | |
| "better-sqlite3": "^9.6.0", | |
| "marked": "^17.0.1", | |
| "sqlite-vec": "^0.1.6", | |
| "zod": "^3.22.4" | |
| }, |
| "dependencies": { | |
| "@anthropic-ai/claude-agent-sdk": "^0.1.9", | |
| "@huggingface/transformers": "^3.8.1", | |
| "@modelcontextprotocol/sdk": "^1.0.4", | |
| "better-sqlite3": "^9.6.0", | |
| "marked": "^17.0.1", | |
| "sqlite-vec": "^0.1.6", | |
| "zod": "^3.22.4" | |
| }, | |
| "dependencies": { | |
| "@anthropic-ai/claude-agent-sdk": "^0.1.9", | |
| "@huggingface/transformers": "^3.8.1", | |
| "@modelcontextprotocol/sdk": "^1.26.0", | |
| "better-sqlite3": "^9.6.0", | |
| "marked": "^17.0.1", | |
| "sqlite-vec": "^0.1.6", | |
| "zod": "^3.25.0" | |
| }, |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/package.json` around lines 19 - 27, The
dependency range for `@modelcontextprotocol/sdk` in package.json is too broad and
may conflict with the zod peer dependency; update package.json so either pin
`@modelcontextprotocol/sdk` to the tested version (e.g., "1.0.4" or "~1.0.4") or
upgrade both `@modelcontextprotocol/sdk` and zod together (e.g., set
`@modelcontextprotocol/sdk` to a specific recent version like "1.26.0" or a caret
if desired, and set zod to ">=3.25.0" such as "^3.25.0") to satisfy the SDK's
peer-dep requirement and avoid resolution to zod 3.22.x.
| /** | ||
| * Tests for stats-cli.ts | ||
| * | ||
| * Tests the CLI command for displaying index statistics. | ||
| * Covers argument parsing, getIndexStats call, and stats output formatting. | ||
| */ | ||
|
|
||
| import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import os from 'os'; | ||
|
|
||
| // Import the core functions that the CLI uses | ||
| import { getIndexStats, formatStats, type IndexStats } from '../core/stats.js'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
This test file never imports or exercises any CLI module.
The file is named stats-cli.test.ts but only imports getIndexStats, formatStats, and IndexStats from ../core/stats.js. No actual CLI entry point, argument parser, or CLI-specific logic is tested. The real CLI module (e.g., stats-cli.ts) presumably orchestrates argument parsing, calls getIndexStats, pipes output through formatStats, handles process.exit, etc.—none of that is covered here.
Given that stats.test.ts already exists for the core module, most tests in this file either duplicate that coverage or test nothing at all. Consider importing and testing the actual CLI module (or its exported functions/handlers) to validate the integration wiring.
#!/bin/bash
# Check if there's a stats-cli.ts module that exports testable functions
fd 'stats-cli.ts' --exclude '*.test.*' --exec cat {}#!/bin/bash
# Check what existing stats.test.ts covers to identify overlap
fd 'stats.test.ts' --exclude 'stats-cli*' --exec head -80 {}🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/cli/stats-cli.test.ts` around lines 1 - 14,
The test file only imports core helpers (getIndexStats, formatStats, IndexStats)
and never exercises the CLI wiring; update the test to import and invoke the
actual CLI module (stats-cli.ts) or its exported handler (e.g., runStatsCommand,
main, or handleStatsArgs) so the integration is validated: mock/spy
getIndexStats and formatStats, simulate argv values, capture stdout/stderr, and
stub process.exit to assert exit codes and printed output; alternatively if the
CLI has no testable export, add a small export (runStatsCli or handleStatsArgs)
in stats-cli.ts and call that from this test instead of duplicating core tests,
or remove this file if it only duplicates coverage from stats.test.ts. Ensure
references to getIndexStats, formatStats, IndexStats and the CLI export name
(stats-cli.ts / runStatsCommand / main) are used so the correct modules are
located.
| describe('stats-cli argument parsing', () => { | ||
| describe('help flag', () => { | ||
| test('should detect --help flag', () => { | ||
| const args = ['--help']; | ||
| const hasHelp = args.includes('--help') || args.includes('-h'); | ||
|
|
||
| expect(hasHelp).toBe(true); | ||
| }); | ||
|
|
||
| test('should detect -h flag', () => { | ||
| const args = ['-h']; | ||
| const hasHelp = args.includes('--help') || args.includes('-h'); | ||
|
|
||
| expect(hasHelp).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('no arguments', () => { | ||
| test('should accept no arguments', () => { | ||
| const args: string[] = []; | ||
| expect(args.length).toBe(0); | ||
| }); | ||
|
|
||
| test('should ignore extra arguments', () => { | ||
| const args = ['extra', 'args']; | ||
| // stats-cli doesn't use any positional arguments | ||
| expect(args.length).toBeGreaterThan(0); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Vacuous tests — argument parsing block tests JavaScript built-ins, not CLI code.
These tests verify that Array.includes and Array.length work correctly on locally constructed arrays. They don't invoke any argument parsing logic from the CLI module. They'll always pass regardless of whether the CLI code is correct, broken, or deleted entirely.
Either remove these or replace them with tests that exercise the real CLI argument parsing (e.g., by importing a parseArgs function or spawning the CLI process).
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/cli/stats-cli.test.ts` around lines 16 - 45,
Tests in stats-cli.test.ts are vacuous because they only assert JavaScript Array
behavior instead of exercising the CLI's argument parsing; replace or remove
them and add real tests that call the CLI parser or run the binary. Update the
test suite to import and call the actual argument parser (e.g., a parseArgs or
parseStatsCliArgs function) or spawn the CLI entry point (e.g., the stats-cli
module) with process-like argv values, then assert outcomes for --help, -h, no
args, and unexpected positional args; remove the current fixture tests that only
use local arrays (the describe blocks testing includes/length) so the tests
cover real code paths in the stats-cli parsing logic.
| expect(output).toContain('1/1/2025'); | ||
| expect(output).toContain('1/31/2025'); |
There was a problem hiding this comment.
Locale-dependent date assertions will break in non-en-US environments.
toLocaleDateString() output depends on the runtime locale. '1/1/2025' and '1/31/2025' are US English formats — CI runners with a different locale (e.g., en-GB → 01/01/2025, de-DE → 1.1.2025) will fail.
The root issue is in the production code (formatStats uses toLocaleDateString() without specifying a locale), but at minimum the test should accommodate this:
Option A: match the production code's locale-aware output
- expect(output).toContain('1/1/2025');
- expect(output).toContain('1/31/2025');
+ expect(output).toContain(new Date('2025-01-01T00:00:00Z').toLocaleDateString());
+ expect(output).toContain(new Date('2025-01-31T23:59:59Z').toLocaleDateString());Alternatively, pin the locale in formatStats itself (e.g., toLocaleDateString('en-US')) for deterministic output.
📝 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.
| expect(output).toContain('1/1/2025'); | |
| expect(output).toContain('1/31/2025'); | |
| expect(output).toContain(new Date('2025-01-01T00:00:00Z').toLocaleDateString()); | |
| expect(output).toContain(new Date('2025-01-31T23:59:59Z').toLocaleDateString()); |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/cli/stats-cli.test.ts` around lines 239 -
240, The test assumes US date formatting ('1/1/2025' / '1/31/2025') which breaks
in other locales; update either the production formatter or the test: either (A)
change formatStats to call toLocaleDateString with a fixed locale (e.g.,
toLocaleDateString('en-US')) so output is deterministic, or (B) make the test
locale-agnostic by generating the expected strings using the same formatting
used in production (call the same toLocaleDateString on your test Date objects
or derive a regex that matches day/month/year numeric patterns) and then assert
output contains those computed values; reference formatStats and the assertions
in stats-cli.test.ts when applying the change.
| describe('stats-cli error handling', () => { | ||
| describe('error messages', () => { | ||
| test('should handle getIndexStats errors', async () => { | ||
| // This test verifies error handling structure | ||
| const errorMessage = 'Error getting stats: Database error'; | ||
|
|
||
| expect(errorMessage).toContain('Error getting stats:'); | ||
| }); | ||
|
|
||
| test('should exit with error code on failure', () => { | ||
| // The CLI exits with code 1 on error | ||
| const exitCode = 1; | ||
|
|
||
| expect(exitCode).toBe(1); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Vacuous error-handling tests — asserting on inline literals.
These tests construct a string 'Error getting stats: Database error' and a number 1, then assert on those same literals. No actual error path of the CLI is exercised. They will always pass even if error handling is completely broken.
Remove these or replace with tests that trigger real error conditions (e.g., passing an invalid DB path that causes getIndexStats to throw, then verifying stderr output and exit code via a subprocess or by mocking process.exit).
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/cli/stats-cli.test.ts` around lines 334 -
350, Replace the vacuous tests in stats-cli.test.ts with real error-path tests:
remove the inline-literal assertions and either (A) invoke the CLI entrypoint
(e.g., run the stats CLI via child_process.exec or spawn) with an invalid DB
path to cause getIndexStats to throw, then assert the process exits with code 1
and stderr contains "Error getting stats:"; or (B) unit-test by mocking the
getIndexStats function to throw and spyOn process.exit and console/error output
(or capture stderr) to assert the error message and exit code; target the
getIndexStats symbol and the CLI entrypoint/process.exit behavior when replacing
the tests.
| test('renders markdown but does NOT escape HTML (potential XSS)', () => { | ||
| const result = renderMarkdownSafely('<script>alert("xss")</script>'); | ||
| // NOTE: marked library does NOT sanitize by default | ||
| // This is a known security issue that should be addressed | ||
| expect(result).toContain('<script>alert("xss")</script>'); |
There was a problem hiding this comment.
The test documents a real XSS vulnerability in renderMarkdownSafely — consider addressing it.
This test explicitly acknowledges that marked does not sanitize HTML by default. Since renderMarkdownSafely is used by formatConversationAsHTML (line 808-829 of this file confirms markdown-content rendering), any assistant message detected as markdown could inject arbitrary HTML/JS when viewed in a browser.
Consider adding DOMPurify or marked's built-in sanitize option in the production code (show.ts). The test assertion would then need to flip to verify the script tag is removed.
Would you like me to open an issue to track adding HTML sanitization to renderMarkdownSafely?
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/show.test.ts` around lines 207 - 211,
The test reveals renderMarkdownSafely currently allows raw HTML; update the
production implementation in show.ts (renderMarkdownSafely used by
formatConversationAsHTML) to sanitize output—use a sanitizer like DOMPurify on
the HTML returned by marked (or enable marked's sanitize option and/or a
post-process with DOMPurify) so any <script> tags or unsafe attributes are
stripped; then update this test in show.test.ts to assert the script tag/content
is removed (e.g., expect(result).not.toContain('<script>alert("xss")</script>')
and expect allowed markdown to still render).
| ) | ||
| : allLines; | ||
|
|
||
| const allMessages: ConversationMessage[] = lines.map(line => JSON.parse(line)); |
There was a problem hiding this comment.
JSON.parse without error handling will crash on any malformed JSONL line.
Line 37 (and identically line 225) maps every line through JSON.parse with no try/catch. A single malformed line in the JSONL input will throw and abort the entire formatting operation. The parser module (parser.ts) handles this gracefully by skipping bad lines — show.ts should do the same.
Suggested fix
- const allMessages: ConversationMessage[] = lines.map(line => JSON.parse(line));
+ const allMessages: ConversationMessage[] = [];
+ for (const line of lines) {
+ try {
+ allMessages.push(JSON.parse(line));
+ } catch {
+ // Skip malformed JSON lines
+ }
+ }Apply to both formatConversationAsMarkdown (line 37) and formatConversationAsHTML (line 225).
📝 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.
| const allMessages: ConversationMessage[] = lines.map(line => JSON.parse(line)); | |
| const allMessages: ConversationMessage[] = []; | |
| for (const line of lines) { | |
| try { | |
| allMessages.push(JSON.parse(line)); | |
| } catch { | |
| // Skip malformed JSON lines | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/core/show.ts` at line 37, Replace the direct
JSON.parse mapping that builds allMessages in both formatConversationAsMarkdown
and formatConversationAsHTML with a safe-parse loop: iterate over lines, try
JSON.parse for each line inside a try/catch, push the parsed ConversationMessage
into allMessages only on success, and silently skip (or optionally log) any
malformed lines to match parser.ts behavior so a single bad JSONL line won't
throw and abort processing.
| // Re-define schemas for testing (in production these would be imported from server.ts) | ||
| const SearchModeEnum = z.enum(['vector', 'text', 'both']); | ||
| const ResponseFormatEnum = z.enum(['markdown', 'json']); | ||
|
|
||
| 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 (e.g., ["my-project", "another-project"])'), | ||
| response_format: ResponseFormatEnum.default('markdown').describe( | ||
| 'Output format: "markdown" for human-readable or "json" for machine-readable (default: "markdown")' | ||
| ), | ||
| }) | ||
| .strict(); | ||
|
|
||
| const ShowConversationInputSchema = z | ||
| .object({ | ||
| path: z | ||
| .string() | ||
| .min(1, 'Path is required') | ||
| .describe('Absolute path to the JSONL conversation file to display'), | ||
| startLine: z | ||
| .number() | ||
| .int() | ||
| .min(1) | ||
| .optional() | ||
| .describe('Starting line number (1-indexed, inclusive). Omit to start from beginning.'), | ||
| endLine: z | ||
| .number() | ||
| .int() | ||
| .min(1) | ||
| .optional() | ||
| .describe('Ending line number (1-indexed, inclusive). Omit to read to end.'), | ||
| }) | ||
| .strict(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Search for schema exports in server.ts
rg -n 'export.*SearchInputSchema|export.*ShowConversationInputSchema' plugins/conversation-memory/src/mcp/ --type=tsRepository: baleen37/claude-plugins
Length of output: 414
🏁 Script executed:
# Check the structure of server.ts to understand what's exported
fd -e ts -p 'server.ts$' plugins/conversation-memory/src/mcp/ | head -5Repository: baleen37/claude-plugins
Length of output: 112
🏁 Script executed:
# Get file sizes to understand scope
wc -l plugins/conversation-memory/src/mcp/server.ts plugins/conversation-memory/src/mcp/server.test.tsRepository: baleen37/claude-plugins
Length of output: 187
🏁 Script executed:
# Read the comment explaining why schemas aren't imported (around line 36-37)
sed -n '30,45p' plugins/conversation-memory/src/mcp/server.test.tsRepository: baleen37/claude-plugins
Length of output: 744
🏁 Script executed:
# Read the mockToolCall function to verify the params usage issue (lines 112-140)
sed -n '112,140p' plugins/conversation-memory/src/mcp/server.test.tsRepository: baleen37/claude-plugins
Length of output: 904
🏁 Script executed:
# Compare the actual schema definitions in both files
echo "=== server.ts SearchInputSchema ===" && sed -n '48,71p' plugins/conversation-memory/src/mcp/server.ts
echo -e "\n=== server.test.ts SearchInputSchema ===" && sed -n '46,70p' plugins/conversation-memory/src/mcp/server.test.tsRepository: baleen37/claude-plugins
Length of output: 1900
🏁 Script executed:
# Check imports in server.test.ts
head -30 plugins/conversation-memory/src/mcp/server.test.tsRepository: baleen37/claude-plugins
Length of output: 988
🏁 Script executed:
# Check imports in server.ts to see if importing from test would create circular dependency
head -40 plugins/conversation-memory/src/mcp/server.tsRepository: baleen37/claude-plugins
Length of output: 1300
🏁 Script executed:
# Verify the params variable is truly unused - check all uses of params in mockToolCall
sed -n '112,166p' plugins/conversation-memory/src/mcp/server.test.tsRepository: baleen37/claude-plugins
Length of output: 1394
Import schemas directly from server.ts to eliminate duplication and drift risk.
The SearchInputSchema and ShowConversationInputSchema are already exported from server.ts (line 99). Redefining them locally here creates maintenance burden—if production schemas change, tests will silently pass against stale definitions. The comment cites "build dependencies" as the reason, but no such dependency exists; importing is feasible and eliminates this risk.
Replace the local schema definitions (lines 46–109) with direct imports from server.ts.
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/src/mcp/server.test.ts` around lines 43 - 109,
Remove the duplicated local schema definitions (SearchInputSchema and
ShowConversationInputSchema) and import those exact exported symbols from
server.ts instead; replace the block that redefines
SearchModeEnum/ResponseFormatEnum/SearchInputSchema/ShowConversationInputSchema
with import statements pulling SearchInputSchema and ShowConversationInputSchema
from server.ts, update any references in this test file to use the imported
names, and run tests to ensure no build/import issues remain.
| # Print error message | ||
| print_error() { | ||
| echo -e "${RED}✗ $1${NC}" | ||
| } | ||
|
|
||
| # Print warning message | ||
| print_warning() { | ||
| echo -e "${YELLOW}⚠ $1${NC}" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Error and warning messages should go to stderr.
print_error and print_warning write to stdout. As per coding guidelines, error messages in shell scripts should be directed to stderr.
♻️ Proposed fix
print_error() {
- echo -e "${RED}✗ $1${NC}"
+ echo -e "${RED}✗ $1${NC}" >&2
}
print_warning() {
- echo -e "${YELLOW}⚠ $1${NC}"
+ echo -e "${YELLOW}⚠ $1${NC}" >&2
}As per coding guidelines, "Direct error messages to stderr in hook scripts".
📝 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.
| # Print error message | |
| print_error() { | |
| echo -e "${RED}✗ $1${NC}" | |
| } | |
| # Print warning message | |
| print_warning() { | |
| echo -e "${YELLOW}⚠ $1${NC}" | |
| } | |
| # Print error message | |
| print_error() { | |
| echo -e "${RED}✗ $1${NC}" >&2 | |
| } | |
| # Print warning message | |
| print_warning() { | |
| echo -e "${YELLOW}⚠ $1${NC}" >&2 | |
| } |
🤖 Prompt for AI Agents
In `@plugins/conversation-memory/test-all.sh` around lines 42 - 50, The
print_error and print_warning functions currently write to stdout; change them
to emit their messages to stderr by redirecting the echo output within
print_error and print_warning (reference the functions print_error and
print_warning) so that error and warning messages are sent to stderr (e.g., use
shell redirection to >&2 inside each function) while keeping the same message
formatting and variables (RED, YELLOW, NC).
| run_bun_tests() { | ||
| print_header "Running Bun Tests" | ||
|
|
||
| if ! command_exists bun; then | ||
| print_error "bun is not installed or not in PATH" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
set -e causes the script to abort if run_bun_tests returns 1, skipping vitest tests entirely.
If bun is not installed (line 63: return 1), set -e on line 11 terminates the script immediately. In all mode, run_vitest_tests and print_summary are never reached. The same applies if run_vitest_tests returns 1 at line 157.
🐛 Proposed fix — prevent early exit in `all` mode
all|"")
print_header "Running All Tests"
- run_bun_tests
- run_vitest_tests
+ run_bun_tests || true
+ run_vitest_tests || true
print_summary
;;The TOTAL_EXIT_CODE variable already tracks failures, so the final exit $TOTAL_EXIT_CODE will still return the correct exit code.
Also applies to: 222-248
1 participant
Summary
Restore the full conversation-memory plugin that was previously migrated to the me plugin in commit 7a41004.
Changes
plugins/conversation-memory/directory from commit 6f6f91eFeatures Restored
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests