fix: export/import scripts now use API instead of direct DB access #332
Conversation
Export script fix: - Add format=json parameter to SearchManager for raw data output - Add getSdkSessionsBySessionIds method to SessionStore - Add POST /api/sdk-sessions/batch endpoint to DataRoutes - Refactor export-memories.ts to use HTTP API Import script fix: - Add import methods to SessionStore with duplicate detection - Add POST /api/import endpoint to DataRoutes - Refactor import-memories.ts to use HTTP API 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
@claude review it |
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
@claude review |
|
Claude finished @thedotmack's task in 1m 41s —— View job Code Review CompleteI've reviewed the PR and have several observations and suggestions: Overall AssessmentStrengths:
Concerns:
Detailed Findings1. Export Script (scripts/export-memories.ts)Issues: Critical: Hybrid Approach Inconsistency (lines 131-158) // Uses API for observations/summaries/prompts
const searchResponse = await fetch(`${baseUrl}/api/search?${params.toString()}`);
// But then switches to direct DB access for sdk_sessions
const Database = (await import('better-sqlite3')).default;
const db = new Database(dbPath, { readonly: true });Why this is problematic:
Recommendation: // After getting sdkSessionIds from search results
const sessionsResponse = await fetch(`${baseUrl}/api/sdk-sessions/batch`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ ids: Array.from(sdkSessionIds) })
});
const sessions = await sessionsResponse.json();Minor: Dynamic import location (line 136) const Database = (await import('better-sqlite3')).default;This import is inside the function body. While it works, it's unusual and makes the dependency less obvious. 2. Import Script (scripts/import-memories.ts)Issues: Critical: Removed API-based import (entire file) Expected pattern: async function importMemories(inputFile: string) {
const exportData = JSON.parse(readFileSync(inputFile, 'utf-8'));
const response = await fetch(`${baseUrl}/api/import`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(exportData)
});
const result = await response.json();
// Display stats
}Actual implementation:
Regression: Worker dependency
Recommendation:
Minor: Error messages (lines 33-35) if (!existsSync(dbPath)) {
console.error(`❌ Database not found at: ${dbPath}`);
process.exit(1);
}Missing the helpful hint from export script: console.error('💡 Has claude-mem been initialized? Try running a session first.');3. SearchManager Format Parameter (src/services/worker/SearchManager.ts)Issues: Undocumented Feature (lines 97-101) const params = new URLSearchParams({
query,
format: 'json', // ← This parameter is not validated or documented
limit: '999999'
});The
Question: Where is Looking at the search method (lines 86-318), it always returns formatted text, not JSON: return {
content: [{
type: 'text' as const,
text: lines.join('\n') // Always formatted text
}]
};This looks like dead code or incomplete implementation. Recommendation:
async search(args: any): Promise<any> {
const normalized = this.normalizeParams(args);
const { query, type, format = 'text', ...options } = normalized;
// ... existing search logic ...
if (format === 'json') {
return {
observations,
sessions,
prompts,
totalResults: observations.length + sessions.length + prompts.length
};
}
// ... existing formatted text output ...
}
4. SessionStore Changes (src/services/sqlite/SessionStore.ts)No issues found in the diff - this file only shows additions for batch API support which look good. The new methods for batch fetching are well-implemented:
5. DataRoutes.ts (src/services/worker/http/routes/DataRoutes.ts)Missing Endpoint:
But this endpoint is not implemented in DataRoutes.ts. The file shows:
Recommendation: /**
* Get SDK sessions by array of IDs
* POST /api/sdk-sessions/batch
* Body: { ids: string[] }
*/
private handleGetSessionsByIds = this.wrapHandler((req: Request, res: Response): void => {
const { ids } = req.body;
if (!ids || !Array.isArray(ids)) {
this.badRequest(res, 'ids must be an array of strings');
return;
}
if (ids.length === 0) {
res.json([]);
return;
}
const store = this.dbManager.getSessionStore();
const placeholders = ids.map(() => '?').join(',');
const sessions = store.db.prepare(`
SELECT * FROM sdk_sessions
WHERE sdk_session_id IN (${placeholders})
ORDER BY started_at_epoch DESC
`).all(...ids);
res.json(sessions);
});And register it: app.post('/api/sdk-sessions/batch', this.handleGetSessionsByIds.bind(this));Missing
This is also not implemented. Should it be added to DataRoutes.ts? Testing ConcernsFrom the test plan:
Questions:
Recommendations SummaryMust Fix (blocking issues):
Should Fix (code quality):
Nice to Have:
Architecture QuestionThis PR highlights an important architectural question: When should scripts use the worker API vs. direct database access? Current state:
Recommendation:
This should be documented in the codebase to guide future development. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the export/import memory scripts to use the worker HTTP API instead of direct database access, fixing issues with project filtering and providing a more consistent architecture.
Key Changes:
- Added JSON format support to SearchManager for raw data output
- Created new API endpoints: batch SDK sessions retrieval and bulk import with duplicate detection
- Refactored both scripts to use HTTP API calls instead of direct
better-sqlite3access
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/worker/http/routes/DataRoutes.ts | Added two new endpoints: POST /api/sdk-sessions/batch for retrieving session metadata and POST /api/import for importing memories with duplicate detection |
| src/services/worker/SearchManager.ts | Added JSON format option to return raw data structures instead of formatted text for programmatic access |
| src/services/sqlite/SessionStore.ts | Added getSdkSessionsBySessionIds method and four import methods with duplicate checking logic |
| scripts/import-memories.ts | Refactored to use worker API instead of direct database access, significantly simplifying the code |
| scripts/export-memories.ts | Replaced direct database queries with API calls to the new batch SDK sessions endpoint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.badRequest(res, 'sdkSessionIds must be an array'); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
The sdkSessionIds array should be validated to ensure all elements are strings. Currently, if the array contains non-string values, the database query could fail or produce unexpected results.
| // Validate all sdkSessionIds are strings | |
| if (!sdkSessionIds.every(id => typeof id === 'string')) { | |
| this.badRequest(res, 'All sdkSessionIds must be strings'); | |
| return; | |
| } |
| private handleImport = this.wrapHandler((req: Request, res: Response): void => { | ||
| const { sessions, summaries, observations, prompts } = req.body; | ||
|
|
||
| const stats = { | ||
| sessionsImported: 0, | ||
| sessionsSkipped: 0, | ||
| summariesImported: 0, | ||
| summariesSkipped: 0, | ||
| observationsImported: 0, | ||
| observationsSkipped: 0, | ||
| promptsImported: 0, | ||
| promptsSkipped: 0 | ||
| }; |
There was a problem hiding this comment.
The import endpoint lacks input validation. All four parameters (sessions, summaries, observations, prompts) should be validated to ensure they are arrays before being passed to the import methods. Without validation, non-array values could cause runtime errors in the database operations.
| WHERE sdk_session_id = ? AND title = ? AND created_at_epoch = ? | ||
| `).get(obs.sdk_session_id, obs.title, obs.created_at_epoch) as { id: number } | undefined; |
There was a problem hiding this comment.
The duplicate detection logic is incorrect when title is null. If multiple observations have the same sdk_session_id and created_at_epoch but null titles, they would be incorrectly identified as duplicates. Consider using COALESCE or IS NULL checks in the WHERE clause to handle this edge case properly.
| WHERE sdk_session_id = ? AND title = ? AND created_at_epoch = ? | |
| `).get(obs.sdk_session_id, obs.title, obs.created_at_epoch) as { id: number } | undefined; | |
| WHERE sdk_session_id = ? | |
| AND ((title = ?) OR (title IS NULL AND ? IS NULL)) | |
| AND created_at_epoch = ? | |
| `).get(obs.sdk_session_id, obs.title, obs.title, obs.created_at_epoch) as { id: number } | undefined; |
| } | ||
|
|
||
| // Read and parse export file | ||
| const exportData = JSON.parse(readFileSync(inputFile, 'utf-8')); |
There was a problem hiding this comment.
JSON.parse should be wrapped in a try-catch block to handle malformed JSON files gracefully. Currently, if the file contains invalid JSON, the script will crash with an unhelpful error message instead of providing clear feedback to the user.
| const exportData = JSON.parse(readFileSync(inputFile, 'utf-8')); | |
| let exportData; | |
| try { | |
| exportData = JSON.parse(readFileSync(inputFile, 'utf-8')); | |
| } catch (err) { | |
| console.error(`❌ Failed to parse JSON in input file: ${inputFile}`); | |
| console.error(' Please ensure the file contains valid JSON.'); | |
| process.exit(1); | |
| } |
| private handleImport = this.wrapHandler((req: Request, res: Response): void => { | ||
| const { sessions, summaries, observations, prompts } = req.body; | ||
|
|
||
| const stats = { | ||
| sessionsImported: 0, | ||
| sessionsSkipped: 0, | ||
| summariesImported: 0, | ||
| summariesSkipped: 0, | ||
| observationsImported: 0, | ||
| observationsSkipped: 0, | ||
| promptsImported: 0, | ||
| promptsSkipped: 0 | ||
| }; | ||
|
|
||
| const store = this.dbManager.getSessionStore(); | ||
|
|
||
| // Import sessions first (dependency for everything else) | ||
| if (Array.isArray(sessions)) { | ||
| for (const session of sessions) { | ||
| const result = store.importSdkSession(session); | ||
| if (result.imported) { | ||
| stats.sessionsImported++; | ||
| } else { | ||
| stats.sessionsSkipped++; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Import summaries (depends on sessions) | ||
| if (Array.isArray(summaries)) { | ||
| for (const summary of summaries) { | ||
| const result = store.importSessionSummary(summary); | ||
| if (result.imported) { | ||
| stats.summariesImported++; | ||
| } else { | ||
| stats.summariesSkipped++; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Import observations (depends on sessions) | ||
| if (Array.isArray(observations)) { | ||
| for (const obs of observations) { | ||
| const result = store.importObservation(obs); | ||
| if (result.imported) { | ||
| stats.observationsImported++; | ||
| } else { | ||
| stats.observationsSkipped++; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Import prompts (depends on sessions) | ||
| if (Array.isArray(prompts)) { | ||
| for (const prompt of prompts) { | ||
| const result = store.importUserPrompt(prompt); | ||
| if (result.imported) { | ||
| stats.promptsImported++; | ||
| } else { | ||
| stats.promptsSkipped++; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| res.json({ | ||
| success: true, | ||
| stats | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The import endpoint lacks test coverage. Given the complexity of this endpoint (handling four different types of imports with duplicate detection), comprehensive tests are needed to verify correct behavior including empty arrays, invalid data, duplicate detection, and proper error handling.
| // Import sessions first (dependency for everything else) | ||
| if (Array.isArray(sessions)) { | ||
| for (const session of sessions) { | ||
| const result = store.importSdkSession(session); | ||
| if (result.imported) { | ||
| stats.sessionsImported++; | ||
| } else { | ||
| stats.sessionsSkipped++; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Import summaries (depends on sessions) | ||
| if (Array.isArray(summaries)) { | ||
| for (const summary of summaries) { | ||
| const result = store.importSessionSummary(summary); | ||
| if (result.imported) { | ||
| stats.summariesImported++; | ||
| } else { | ||
| stats.summariesSkipped++; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Import observations (depends on sessions) | ||
| if (Array.isArray(observations)) { | ||
| for (const obs of observations) { | ||
| const result = store.importObservation(obs); | ||
| if (result.imported) { | ||
| stats.observationsImported++; | ||
| } else { | ||
| stats.observationsSkipped++; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Import prompts (depends on sessions) | ||
| if (Array.isArray(prompts)) { | ||
| for (const prompt of prompts) { | ||
| const result = store.importUserPrompt(prompt); | ||
| if (result.imported) { | ||
| stats.promptsImported++; | ||
| } else { | ||
| stats.promptsSkipped++; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Performance concern: Each import operation performs individual database queries for duplicate checking and insertion. For large imports, this could be slow. Consider batching the operations or using prepared statements with batch execution to improve performance, especially when importing thousands of records.
| this.badRequest(res, 'sdkSessionIds must be an array'); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Empty array validation should be added. When sdkSessionIds is an empty array, the function should return an empty array immediately without making a database call, for consistency with getSdkSessionsBySessionIds behavior.
| if (sdkSessionIds.length === 0) { | |
| res.json([]); | |
| return; | |
| } |
| private handleGetSdkSessionsByIds = this.wrapHandler((req: Request, res: Response): void => { | ||
| const { sdkSessionIds } = req.body; | ||
|
|
||
| if (!Array.isArray(sdkSessionIds)) { | ||
| this.badRequest(res, 'sdkSessionIds must be an array'); | ||
| return; | ||
| } | ||
|
|
||
| const store = this.dbManager.getSessionStore(); | ||
| const sessions = store.getSdkSessionsBySessionIds(sdkSessionIds); | ||
| res.json(sessions); | ||
| }); |
There was a problem hiding this comment.
The new batch SDK sessions endpoint lacks test coverage. Other batch endpoints in this codebase have corresponding tests, and this new endpoint should follow the same pattern to ensure input validation and edge cases (empty arrays, invalid values) are properly tested.
| files_edited: string | null; | ||
| notes: string | null; | ||
| prompt_number: number | null; | ||
| discovery_tokens: number; |
There was a problem hiding this comment.
Type signature inconsistency: discovery_tokens is typed as number but the code uses a fallback (discovery_tokens || 0), suggesting it could be null or undefined. The type should be number | null to match the actual data and the export script's interface definitions.
| discovery_tokens: number; | |
| discovery_tokens: number | null; |
| files_read: string | null; | ||
| files_modified: string | null; | ||
| prompt_number: number | null; | ||
| discovery_tokens: number; |
There was a problem hiding this comment.
Type signature inconsistency: discovery_tokens is typed as number but the code uses a fallback (discovery_tokens || 0), suggesting it could be null or undefined. The type should be number | null to match the actual data and the export script's interface definitions.
| discovery_tokens: number; | |
| discovery_tokens: number | null; |
2 participants
Summary
export-memories.ts- was failing to export memories with project filterimport-memories.ts- refactored to use API for consistencyPOST /api/sdk-sessions/batch)POST /api/import) with duplicate detectionProblem
The export/import scripts were using direct database access via
better-sqlite3, which caused issues:Solution
Refactored both scripts to use the worker HTTP API, which:
Test plan
🤖 Generated with Claude Code