From 089ed70a62d4e0ae091d8c22363801b26b741116 Mon Sep 17 00:00:00 2001 From: hugo Date: Sun, 1 Mar 2026 13:07:50 +0100 Subject: [PATCH] fix: eliminate non-null assertions, fix broken test, add error-path coverage --- src/main/ipc/chatHandlers.ts | 21 +++++-- tests/engine/SecureKeyStore.test.ts | 1 - tests/ipc/chatHandlersKeychain.test.ts | 86 +++++++++++++++++++++----- 3 files changed, 86 insertions(+), 22 deletions(-) diff --git a/src/main/ipc/chatHandlers.ts b/src/main/ipc/chatHandlers.ts index 928a512..46c8d13 100644 --- a/src/main/ipc/chatHandlers.ts +++ b/src/main/ipc/chatHandlers.ts @@ -16,6 +16,16 @@ let openCodeManagerInitPromise: Promise | null = null; let mainWindowGetter: (() => BrowserWindow | null) | null = null; let engineBundle: EngineBundle | null = null; +/** + * Get or create the SecureKeyStore instance. + */ +function getSecureKeyStore(): SecureKeyStore { + if (!secureKeyStore) { + secureKeyStore = new SecureKeyStore(getChatEngine()); + } + return secureKeyStore; +} + /** * Initialize chat handlers with the main window reference */ @@ -49,16 +59,15 @@ async function getOpenCodeManager(): Promise { () => mainWindowGetter?.() || null ); - // Initialize secure key store and load API key - const engine = getChatEngine(); - secureKeyStore = new SecureKeyStore(engine); + // Load API key from encrypted storage + const keyStore = getSecureKeyStore(); openCodeManagerInitPromise = (async () => { try { // Clean up old plain-text key from settings (pre-keychain storage) - await secureKeyStore!.cleanupPlainTextKey('opencode_api_key'); + await keyStore.cleanupPlainTextKey('opencode_api_key'); // Load API key from encrypted storage - const key = await secureKeyStore!.retrieve('opencode_api_key'); + const key = await keyStore.retrieve('opencode_api_key'); if (key) { openCodeManager!.setApiKey(key); } @@ -117,7 +126,7 @@ export function registerChatHandlers(): void { manager.setApiKey(apiKey); // Persist to encrypted storage - await secureKeyStore!.store('opencode_api_key', apiKey); + await getSecureKeyStore().store('opencode_api_key', apiKey); return { success: true }; } catch (error) { diff --git a/tests/engine/SecureKeyStore.test.ts b/tests/engine/SecureKeyStore.test.ts index d241472..42a6902 100644 --- a/tests/engine/SecureKeyStore.test.ts +++ b/tests/engine/SecureKeyStore.test.ts @@ -9,7 +9,6 @@ import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; // Track mock state let safeStorageAvailable = true; -const encryptedBuffers = new Map(); // Mock Electron's safeStorage vi.mock('electron', () => ({ diff --git a/tests/ipc/chatHandlersKeychain.test.ts b/tests/ipc/chatHandlersKeychain.test.ts index 5daa4c9..27a868e 100644 --- a/tests/ipc/chatHandlersKeychain.test.ts +++ b/tests/ipc/chatHandlersKeychain.test.ts @@ -20,6 +20,12 @@ const chatEngineInstances: Array> = []; const openCodeManagerInstances: Array> = []; const secureKeyStoreInstances: Array> = []; +// Per-test overrides for SecureKeyStore mock behavior +let secureKeyStoreRetrieveResult: string | null = 'encrypted-stored-key'; +let secureKeyStoreStoreError: Error | null = null; +let secureKeyStoreRetrieveError: Error | null = null; +let secureKeyStoreCleanupError: Error | null = null; + vi.mock('electron', () => ({ BrowserWindow: { fromWebContents: vi.fn(), @@ -64,10 +70,17 @@ vi.mock('../../src/main/engine/SecureKeyStore', () => ({ constructor() { const instance = { isAvailable: vi.fn(() => true), - store: vi.fn(async () => undefined), - retrieve: vi.fn(async () => 'encrypted-stored-key'), + store: vi.fn(async (_key: string, _value: string) => { + if (secureKeyStoreStoreError) throw secureKeyStoreStoreError; + }), + retrieve: vi.fn(async () => { + if (secureKeyStoreRetrieveError) throw secureKeyStoreRetrieveError; + return secureKeyStoreRetrieveResult; + }), remove: vi.fn(async () => undefined), - cleanupPlainTextKey: vi.fn(async () => undefined), + cleanupPlainTextKey: vi.fn(async () => { + if (secureKeyStoreCleanupError) throw secureKeyStoreCleanupError; + }), }; secureKeyStoreInstances.push(instance); return instance; @@ -103,6 +116,10 @@ describe('chatHandlers keychain integration', () => { chatEngineInstances.length = 0; openCodeManagerInstances.length = 0; secureKeyStoreInstances.length = 0; + secureKeyStoreRetrieveResult = 'encrypted-stored-key'; + secureKeyStoreStoreError = null; + secureKeyStoreRetrieveError = null; + secureKeyStoreCleanupError = null; vi.resetModules(); }); @@ -190,26 +207,65 @@ describe('chatHandlers keychain integration', () => { }); it('handles missing key gracefully on init (no key stored)', async () => { - // Override retrieve to return null - vi.resetModules(); - const secureKeyStoreModuleMock = await import('../../src/main/engine/SecureKeyStore'); - // The mock class already returns 'encrypted-stored-key', but we need null for this test - // We'll handle this differently - mock retrieve to return null + secureKeyStoreRetrieveResult = null; const mod = await import('../../src/main/ipc/chatHandlers'); const mockBundle = { postEngine: {}, mediaEngine: {}, postMediaEngine: {} }; mod.initializeChatHandlers(() => mainWindowMock as never, mockBundle as any); mod.registerChatHandlers(); - // Override the retrieve mock before triggering init - // Since we can't easily change the mock after construction, we verify - // that when retrieve returns null, setApiKey is not called with null const handler = registeredHandlers.get('chat:checkReady'); - - // Make retrieve return null for this test - secureKeyStoreInstances.length = 0; - const result = await handler!(undefined); expect(result.ready).toBe(true); + + const manager = openCodeManagerInstances[0]; + // setApiKey should NOT have been called since there's no stored key + expect(manager.setApiKey).not.toHaveBeenCalled(); + }); + + it('still initializes when retrieve() throws on init', async () => { + secureKeyStoreRetrieveError = new Error('decryption failed'); + + const mod = await import('../../src/main/ipc/chatHandlers'); + const mockBundle = { postEngine: {}, mediaEngine: {}, postMediaEngine: {} }; + mod.initializeChatHandlers(() => mainWindowMock as never, mockBundle as any); + mod.registerChatHandlers(); + + const handler = registeredHandlers.get('chat:checkReady'); + const result = await handler!(undefined); + // Init should complete even if key retrieval fails + expect(result.ready).toBe(true); + + const manager = openCodeManagerInstances[0]; + expect(manager.setApiKey).not.toHaveBeenCalled(); + }); + + it('still initializes when cleanupPlainTextKey() throws on init', async () => { + secureKeyStoreCleanupError = new Error('delete failed'); + + const mod = await import('../../src/main/ipc/chatHandlers'); + const mockBundle = { postEngine: {}, mediaEngine: {}, postMediaEngine: {} }; + mod.initializeChatHandlers(() => mainWindowMock as never, mockBundle as any); + mod.registerChatHandlers(); + + const handler = registeredHandlers.get('chat:checkReady'); + const result = await handler!(undefined); + // Init should complete even if cleanup fails + expect(result.ready).toBe(true); + }); + + it('returns error when store() throws on chat:setApiKey', async () => { + secureKeyStoreStoreError = new Error('encryption unavailable'); + + const mod = await import('../../src/main/ipc/chatHandlers'); + const mockBundle = { postEngine: {}, mediaEngine: {}, postMediaEngine: {} }; + mod.initializeChatHandlers(() => mainWindowMock as never, mockBundle as any); + mod.registerChatHandlers(); + + const handler = registeredHandlers.get('chat:setApiKey'); + const result = await handler!(undefined, 'sk-new-key'); + + expect(result.success).toBe(false); + expect(result.error).toContain('encryption unavailable'); }); });