From d2eb89d27fe089d127050650eda07729dd5bff85 Mon Sep 17 00:00:00 2001 From: Alex Newman Date: Sun, 19 Apr 2026 20:17:57 -0700 Subject: [PATCH] fix: resolve all CodeRabbit review comments Critical fixes: - GeminiCliHooksInstaller: wrap install/uninstall prep in try/catch to maintain numeric return contract - McpIntegrations: move mkdirSync inside try block for Goose installer Major fixes: - import-xml-observations: guard invalid dates before toISOString() - runtime.ts: guard response.json() parsing - WorktreeAdoption: delay adoptedSqliteIds mutation until SQL succeeds - CursorHooksInstaller: move mkdirSync inside try block - McpIntegrations: throw on failed claude-mem block replacement - OpenClawInstaller: propagate parse failure instead of returning {} - OpenClawInstaller: move mkdirSync inside try block - WindsurfHooksInstaller: validate hooks.json shape with optional chaining - timeline/queries: pass normalized Error directly to logger - ChromaSync: use composite dedup key (entityType:id) to prevent cross-type collisions - EnvManager: wrap preflight directory/file ops in try/catch with ENV logging Co-Authored-By: Claude Opus 4.6 (1M context) --- src/bin/import-xml-observations.ts | 3 + src/npx-cli/commands/runtime.ts | 9 +- .../infrastructure/WorktreeAdoption.ts | 2 +- .../integrations/CursorHooksInstaller.ts | 5 +- .../integrations/GeminiCliHooksInstaller.ts | 82 +++++++++---------- src/services/integrations/McpIntegrations.ts | 7 +- .../integrations/OpenClawInstaller.ts | 14 ++-- .../integrations/WindsurfHooksInstaller.ts | 19 ++--- src/services/sqlite/timeline/queries.ts | 6 +- src/services/sync/ChromaSync.ts | 12 ++- src/shared/EnvManager.ts | 29 ++++--- 11 files changed, 104 insertions(+), 84 deletions(-) diff --git a/src/bin/import-xml-observations.ts b/src/bin/import-xml-observations.ts index 98c5dbec..8b4b9cdf 100644 --- a/src/bin/import-xml-observations.ts +++ b/src/bin/import-xml-observations.ts @@ -78,6 +78,9 @@ function buildTimestampMap(): TimestampMapping { if (timestamp && sessionId) { // Round timestamp to second for matching with XML timestamps const roundedTimestamp = new Date(timestamp); + if (Number.isNaN(roundedTimestamp.getTime())) { + continue; + } roundedTimestamp.setMilliseconds(0); const key = roundedTimestamp.toISOString(); diff --git a/src/npx-cli/commands/runtime.ts b/src/npx-cli/commands/runtime.ts index e6293551..da3560eb 100644 --- a/src/npx-cli/commands/runtime.ts +++ b/src/npx-cli/commands/runtime.ts @@ -179,7 +179,14 @@ export async function runSearchCommand(queryParts: string[]): Promise { process.exit(1); } - const data = await response.json(); + let data: unknown; + try { + data = await response.json(); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error); + console.error(pc.red(`Search failed: invalid JSON response (${message})`)); + process.exit(1); + } if (typeof data === 'object' && data !== null) { console.log(JSON.stringify(data, null, 2)); diff --git a/src/services/infrastructure/WorktreeAdoption.ts b/src/services/infrastructure/WorktreeAdoption.ts index b6d82939..96143f86 100644 --- a/src/services/infrastructure/WorktreeAdoption.ts +++ b/src/services/infrastructure/WorktreeAdoption.ts @@ -254,10 +254,10 @@ export async function adoptMergedWorktrees(opts: { worktreeProject, parentProject ) as Array<{ id: number }>; - for (const r of rows) adoptedSqliteIds.push(r.id); const obsChanges = updateObs.run(parentProject, worktreeProject).changes; const sumChanges = updateSum.run(parentProject, worktreeProject).changes; + for (const r of rows) adoptedSqliteIds.push(r.id); result.adoptedObservations += obsChanges; result.adoptedSummaries += sumChanges; }; diff --git a/src/services/integrations/CursorHooksInstaller.ts b/src/services/integrations/CursorHooksInstaller.ts index 62f8b78e..2192c8ca 100644 --- a/src/services/integrations/CursorHooksInstaller.ts +++ b/src/services/integrations/CursorHooksInstaller.ts @@ -316,9 +316,6 @@ export async function installCursorHooks(target: CursorInstallTarget): Promise { console.log(` Using Bun runtime: ${bunPath}`); console.log(` Worker service: ${workerServicePath}`); - // Build hook commands for all mapped events - const hooksConfig: GeminiHooksConfig = {}; - for (const geminiEvent of Object.keys(GEMINI_EVENT_TO_INTERNAL_EVENT)) { - const command = buildHookCommand(bunPath, workerServicePath, geminiEvent); - hooksConfig[geminiEvent] = [createHookGroup(command)]; - } - - // Read existing settings and merge - const existingSettings = readGeminiSettings(); - const mergedSettings = mergeHooksIntoSettings(existingSettings, hooksConfig); - try { + // Build hook commands for all mapped events + const hooksConfig: GeminiHooksConfig = {}; + for (const geminiEvent of Object.keys(GEMINI_EVENT_TO_INTERNAL_EVENT)) { + const command = buildHookCommand(bunPath, workerServicePath, geminiEvent); + hooksConfig[geminiEvent] = [createHookGroup(command)]; + } + + // Read existing settings and merge + const existingSettings = readGeminiSettings(); + const mergedSettings = mergeHooksIntoSettings(existingSettings, hooksConfig); + writeGeminiHooksAndSetupContext(mergedSettings); return 0; } catch (error) { @@ -358,37 +358,37 @@ export function uninstallGeminiCliHooks(): number { return 0; } - const settings = readGeminiSettings(); - if (!settings.hooks) { - console.log(' No hooks found in Gemini CLI settings — nothing to uninstall.'); - return 0; - } - - let removedCount = 0; - - // Remove claude-mem hooks from within each group, preserving other hooks - for (const [eventName, groups] of Object.entries(settings.hooks)) { - const filteredGroups = groups - .map(group => { - const remainingHooks = group.hooks.filter(hook => hook.name !== HOOK_NAME); - removedCount += group.hooks.length - remainingHooks.length; - return { ...group, hooks: remainingHooks }; - }) - .filter(group => group.hooks.length > 0); - - if (filteredGroups.length > 0) { - settings.hooks[eventName] = filteredGroups; - } else { - delete settings.hooks[eventName]; - } - } - - // Clean up empty hooks object - if (Object.keys(settings.hooks).length === 0) { - delete settings.hooks; - } - try { + const settings = readGeminiSettings(); + if (!settings.hooks) { + console.log(' No hooks found in Gemini CLI settings — nothing to uninstall.'); + return 0; + } + + let removedCount = 0; + + // Remove claude-mem hooks from within each group, preserving other hooks + for (const [eventName, groups] of Object.entries(settings.hooks)) { + const filteredGroups = groups + .map(group => { + const remainingHooks = group.hooks.filter(hook => hook.name !== HOOK_NAME); + removedCount += group.hooks.length - remainingHooks.length; + return { ...group, hooks: remainingHooks }; + }) + .filter(group => group.hooks.length > 0); + + if (filteredGroups.length > 0) { + settings.hooks[eventName] = filteredGroups; + } else { + delete settings.hooks[eventName]; + } + } + + // Clean up empty hooks object + if (Object.keys(settings.hooks).length === 0) { + delete settings.hooks; + } + writeSettingsAndCleanupGeminiContext(settings, removedCount); return 0; } catch (error) { diff --git a/src/services/integrations/McpIntegrations.ts b/src/services/integrations/McpIntegrations.ts index 4dba7a57..86d26fb8 100644 --- a/src/services/integrations/McpIntegrations.ts +++ b/src/services/integrations/McpIntegrations.ts @@ -288,9 +288,9 @@ export async function installGooseMcpIntegration(): Promise { const configPath = getGooseConfigPath(); const configDirectory = path.dirname(configPath); - mkdirSync(configDirectory, { recursive: true }); try { + mkdirSync(configDirectory, { recursive: true }); mergeGooseYamlConfig(configPath, mcpServerPath); return 0; } catch (error) { @@ -308,9 +308,10 @@ function mergeGooseYamlConfig(configPath: string, mcpServerPath: string): void { const claudeMemPattern = /( {2}claude-mem:\n(?:.*\n)*?(?= {2}\S|\n\n|^\S|$))/m; const newEntry = buildGooseClaudeMemEntryYaml(mcpServerPath) + '\n'; - if (claudeMemPattern.test(yamlContent)) { - yamlContent = yamlContent.replace(claudeMemPattern, newEntry); + if (!claudeMemPattern.test(yamlContent)) { + throw new Error('Found mcpServers/claude-mem markers but could not locate a replaceable claude-mem block'); } + yamlContent = yamlContent.replace(claudeMemPattern, newEntry); writeFileSync(configPath, yamlContent); console.log(` Updated existing claude-mem entry in: ${configPath}`); } else if (yamlContent.includes('mcpServers:')) { diff --git a/src/services/integrations/OpenClawInstaller.ts b/src/services/integrations/OpenClawInstaller.ts index 7dca3f32..53c491e0 100644 --- a/src/services/integrations/OpenClawInstaller.ts +++ b/src/services/integrations/OpenClawInstaller.ts @@ -147,12 +147,9 @@ function readOpenClawConfig(): Record { try { return JSON.parse(readFileSync(configFilePath, 'utf-8')); } catch (error) { - if (error instanceof Error) { - logger.error('WORKER', 'Failed to parse openclaw.json, using empty config', { path: configFilePath }, error); - } else { - logger.error('WORKER', 'Failed to parse openclaw.json, using empty config', { path: configFilePath }, new Error(String(error))); - } - return {}; + const normalizedError = error instanceof Error ? error : new Error(String(error)); + logger.error('WORKER', 'Failed to parse openclaw.json', { path: configFilePath }, normalizedError); + throw normalizedError; } } @@ -255,9 +252,6 @@ export function installOpenClawPlugin(): number { const extensionDirectory = getOpenClawClaudeMemExtensionDirectory(); const destinationDistDirectory = path.join(extensionDirectory, 'dist'); - // Create the extension directory structure - mkdirSync(destinationDistDirectory, { recursive: true }); - // Locate optional assets before entering the try block const manifestPath = findPluginManifestPath(); const skillsDirectory = findPluginSkillsDirectory(); @@ -271,6 +265,8 @@ export function installOpenClawPlugin(): number { }; try { + // Create the extension directory structure inside try to catch EACCES/ENOSPC + mkdirSync(destinationDistDirectory, { recursive: true }); copyPluginFilesAndRegister(preBuiltDistDirectory, destinationDistDirectory, extensionDirectory, manifestPath, skillsDirectory, extensionPackageJson); return 0; } catch (error) { diff --git a/src/services/integrations/WindsurfHooksInstaller.ts b/src/services/integrations/WindsurfHooksInstaller.ts index 3251012d..b709e335 100644 --- a/src/services/integrations/WindsurfHooksInstaller.ts +++ b/src/services/integrations/WindsurfHooksInstaller.ts @@ -441,11 +441,13 @@ export function uninstallWindsurfHooks(): number { } function removeClaudeMemHookEntries(): void { - const config: WindsurfHooksJson = JSON.parse(readFileSync(WINDSURF_HOOKS_JSON_PATH, 'utf-8')); + const parsed = JSON.parse(readFileSync(WINDSURF_HOOKS_JSON_PATH, 'utf-8')) as Partial; + const config: WindsurfHooksJson = { hooks: parsed.hooks ?? {} }; for (const eventName of WINDSURF_HOOK_EVENTS) { - if (config.hooks[eventName]) { - config.hooks[eventName] = config.hooks[eventName].filter( + const eventHooks = config.hooks[eventName] ?? []; + if (eventHooks.length > 0) { + config.hooks[eventName] = eventHooks.filter( (hook) => !hook.command.includes('worker-service') || !hook.command.includes('windsurf'), ); if (config.hooks[eventName].length === 0) { @@ -487,21 +489,18 @@ export function checkWindsurfHooksStatus(): number { console.log(`User-level: Installed`); console.log(` Config: ${WINDSURF_HOOKS_JSON_PATH}`); - let parsedConfig: WindsurfHooksJson | null = null; + let parsedConfig: Partial | null = null; try { parsedConfig = JSON.parse(readFileSync(WINDSURF_HOOKS_JSON_PATH, 'utf-8')); } catch (error) { - if (error instanceof Error) { - logger.error('WORKER', 'Unable to parse hooks.json', { path: WINDSURF_HOOKS_JSON_PATH }, error); - } else { - logger.error('WORKER', 'Unable to parse hooks.json', { path: WINDSURF_HOOKS_JSON_PATH }, new Error(String(error))); - } + const normalizedError = error instanceof Error ? error : new Error(String(error)); + logger.error('WORKER', 'Unable to parse hooks.json', { path: WINDSURF_HOOKS_JSON_PATH }, normalizedError); console.log(` Mode: Unable to parse hooks.json`); } if (parsedConfig) { const registeredEvents = WINDSURF_HOOK_EVENTS.filter( - (event) => parsedConfig!.hooks[event]?.some( + (event) => (parsedConfig?.hooks?.[event] ?? []).some( (hook) => hook.command.includes('worker-service') && hook.command.includes('windsurf') ) ); diff --git a/src/services/sqlite/timeline/queries.ts b/src/services/sqlite/timeline/queries.ts index 6d59064f..6e4676cf 100644 --- a/src/services/sqlite/timeline/queries.ts +++ b/src/services/sqlite/timeline/queries.ts @@ -112,7 +112,8 @@ export function getTimelineAroundObservation( startEpoch = beforeRecords.length > 0 ? beforeRecords[beforeRecords.length - 1].created_at_epoch : anchorEpoch; endEpoch = afterRecords.length > 0 ? afterRecords[afterRecords.length - 1].created_at_epoch : anchorEpoch; } catch (err) { - logger.error('DB', 'Error getting boundary observations', undefined, { error: err instanceof Error ? err : new Error(String(err)), project }); + const normalizedError = err instanceof Error ? err : new Error(String(err)); + logger.error('DB', 'Error getting boundary observations', { project }, normalizedError); return { observations: [], sessions: [], prompts: [] }; } } else { @@ -144,7 +145,8 @@ export function getTimelineAroundObservation( startEpoch = beforeRecords.length > 0 ? beforeRecords[beforeRecords.length - 1].created_at_epoch : anchorEpoch; endEpoch = afterRecords.length > 0 ? afterRecords[afterRecords.length - 1].created_at_epoch : anchorEpoch; } catch (err) { - logger.error('DB', 'Error getting boundary timestamps', undefined, { error: err instanceof Error ? err : new Error(String(err)), project }); + const normalizedError = err instanceof Error ? err : new Error(String(err)); + logger.error('DB', 'Error getting boundary timestamps', { project }, normalizedError); return { observations: [], sessions: [], prompts: [] }; } } diff --git a/src/services/sync/ChromaSync.ts b/src/services/sync/ChromaSync.ts index 8c7fa961..397e7790 100644 --- a/src/services/sync/ChromaSync.ts +++ b/src/services/sync/ChromaSync.ts @@ -809,7 +809,7 @@ export class ChromaSync { // chroma_query_documents returns nested arrays (one per query text) // We always pass a single query text, so we access [0] const ids: number[] = []; - const seen = new Set(); + const seen = new Set(); const docIds = results?.ids?.[0] || []; const rawMetadatas = results?.metadatas?.[0] || []; const rawDistances = results?.distances?.[0] || []; @@ -828,16 +828,22 @@ export class ChromaSync { const promptMatch = docId.match(/prompt_(\d+)/); let sqliteId: number | null = null; + let entityType: string | null = null; if (obsMatch) { sqliteId = parseInt(obsMatch[1], 10); + entityType = 'observation'; } else if (summaryMatch) { sqliteId = parseInt(summaryMatch[1], 10); + entityType = 'session_summary'; } else if (promptMatch) { sqliteId = parseInt(promptMatch[1], 10); + entityType = 'user_prompt'; } - if (sqliteId !== null && !seen.has(sqliteId)) { - seen.add(sqliteId); + if (sqliteId !== null && entityType) { + const dedupeKey = `${entityType}:${sqliteId}`; + if (seen.has(dedupeKey)) continue; + seen.add(dedupeKey); ids.push(sqliteId); metadatas.push(rawMetadatas[i] ?? null); distances.push(rawDistances[i] ?? 0); diff --git a/src/shared/EnvManager.ts b/src/shared/EnvManager.ts index 9a8c4639..6ad62731 100644 --- a/src/shared/EnvManager.ts +++ b/src/shared/EnvManager.ts @@ -131,18 +131,25 @@ export function loadClaudeMemEnv(): ClaudeMemEnv { * Save credentials to ~/.claude-mem/.env */ export function saveClaudeMemEnv(env: ClaudeMemEnv): void { - // Ensure directory exists with restricted permissions (owner only) - if (!existsSync(DATA_DIR)) { - mkdirSync(DATA_DIR, { recursive: true, mode: 0o700 }); - } - // Fix permissions on pre-existing directories (mode: is only applied on creation) - // Note: On Windows, chmod has no effect — permissions are controlled via ACLs. - chmodSync(DATA_DIR, 0o700); + let existing: Record = {}; + try { + // Ensure directory exists with restricted permissions (owner only) + if (!existsSync(DATA_DIR)) { + mkdirSync(DATA_DIR, { recursive: true, mode: 0o700 }); + } + // Fix permissions on pre-existing directories (mode: is only applied on creation) + // Note: On Windows, chmod has no effect — permissions are controlled via ACLs. + chmodSync(DATA_DIR, 0o700); - // Load existing to preserve any extra keys - const existing = existsSync(ENV_FILE_PATH) - ? parseEnvFile(readFileSync(ENV_FILE_PATH, 'utf-8')) - : {}; + // Load existing to preserve any extra keys + existing = existsSync(ENV_FILE_PATH) + ? parseEnvFile(readFileSync(ENV_FILE_PATH, 'utf-8')) + : {}; + } catch (error) { + const normalizedError = error instanceof Error ? error : new Error(String(error)); + logger.error('ENV', 'Failed to set up env directory or read existing env', {}, normalizedError); + throw normalizedError; + } // Update with new values const updated: Record = { ...existing };