feat(core): implement unified KeychainService and migrate token storage (#21344)

packages/core/src/index.ts

@@ -123,6 +123,8 @@ export * from './services/sessionSummaryUtils.js';
 export * from './services/contextManager.js';
 export * from './services/trackerService.js';
 export * from './services/trackerTypes.js';
+export * from './services/keychainService.js';
+export * from './services/keychainTypes.js';
 export * from './skills/skillManager.js';
 export * from './skills/skillLoader.js';
 

packages/core/src/mcp/token-storage/keychain-token-storage.test.ts

@@ -5,54 +5,41 @@
  */
 
 import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
-import type { KeychainTokenStorage } from './keychain-token-storage.js';
+import { KeychainTokenStorage } from './keychain-token-storage.js';
 import type { OAuthCredentials } from './types.js';
+import { KeychainService } from '../../services/keychainService.js';
 import { coreEvents } from '../../utils/events.js';
-
-// Hoist the mock to be available in the vi.mock factory
-const mockKeytar = vi.hoisted(() => ({
-  getPassword: vi.fn(),
-  setPassword: vi.fn(),
-  deletePassword: vi.fn(),
-  findCredentials: vi.fn(),
-}));
-
-const mockServiceName = 'service-name';
-const mockCryptoRandomBytesString = 'random-string';
-
-// Mock the dynamic import of 'keytar'
-vi.mock('keytar', () => ({
-  default: mockKeytar,
-}));
-
-vi.mock('node:crypto', async (importOriginal) => {
-  const actual = await importOriginal<typeof import('node:crypto')>();
-  return {
-    ...actual,
-    randomBytes: vi.fn(() => ({
-      toString: vi.fn(() => mockCryptoRandomBytesString),
-    })),
-  };
-});
-
-vi.mock('../../utils/events.js', () => ({
-  coreEvents: {
-    emitFeedback: vi.fn(),
-    emitTelemetryKeychainAvailability: vi.fn(),
-  },
-}));
+import { KEYCHAIN_TEST_PREFIX } from '../../services/keychainTypes.js';
 
 describe('KeychainTokenStorage', () => {
   let storage: KeychainTokenStorage;
+  const mockServiceName = 'service-name';
+  let storageState: Map<string, string>;
 
-  beforeEach(async () => {
-    vi.resetAllMocks();
-    // Reset the internal state of the keychain-token-storage module
-    vi.resetModules();
-    const { KeychainTokenStorage } = await import(
-      './keychain-token-storage.js'
-    );
+  beforeEach(() => {
+    vi.clearAllMocks();
     storage = new KeychainTokenStorage(mockServiceName);
+    storageState = new Map();
+
+    // Use stateful spies to verify logic behaviorally
+    vi.spyOn(KeychainService.prototype, 'getPassword').mockImplementation(
+      async (account) => storageState.get(account) ?? null,
+    );
+    vi.spyOn(KeychainService.prototype, 'setPassword').mockImplementation(
+      async (account, value) => {
+        storageState.set(account, value);
+      },
+    );
+    vi.spyOn(KeychainService.prototype, 'deletePassword').mockImplementation(
+      async (account) => storageState.delete(account),
+    );
+    vi.spyOn(KeychainService.prototype, 'findCredentials').mockImplementation(
+      async () =>
+        Array.from(storageState.entries()).map(([account, password]) => ({
+          account,
+          password,
+        })),
+    );
   });
 
   afterEach(() => {
@@ -70,375 +57,149 @@ describe('KeychainTokenStorage', () => {
     updatedAt: Date.now(),
   } as OAuthCredentials;
 
-  describe('checkKeychainAvailability', () => {
-    it('should return true if keytar is available and functional', async () => {
-      mockKeytar.setPassword.mockResolvedValue(undefined);
-      mockKeytar.getPassword.mockResolvedValue('test');
-      mockKeytar.deletePassword.mockResolvedValue(true);
-
-      const isAvailable = await storage.checkKeychainAvailability();
-      expect(isAvailable).toBe(true);
-      expect(mockKeytar.setPassword).toHaveBeenCalledWith(
-        mockServiceName,
-        `__keychain_test__${mockCryptoRandomBytesString}`,
-        'test',
-      );
-      expect(mockKeytar.getPassword).toHaveBeenCalledWith(
-        mockServiceName,
-        `__keychain_test__${mockCryptoRandomBytesString}`,
-      );
-      expect(mockKeytar.deletePassword).toHaveBeenCalledWith(
-        mockServiceName,
-        `__keychain_test__${mockCryptoRandomBytesString}`,
-      );
-    });
-
-    it('should return false if keytar fails to set password', async () => {
-      const error = new Error('write error');
-      mockKeytar.setPassword.mockRejectedValue(error);
-      const isAvailable = await storage.checkKeychainAvailability();
-      expect(isAvailable).toBe(false);
-    });
-
-    it('should return false if retrieved password does not match', async () => {
-      mockKeytar.setPassword.mockResolvedValue(undefined);
-      mockKeytar.getPassword.mockResolvedValue('wrong-password');
-      mockKeytar.deletePassword.mockResolvedValue(true);
-      const isAvailable = await storage.checkKeychainAvailability();
-      expect(isAvailable).toBe(false);
-    });
-
-    it('should cache the availability result', async () => {
-      mockKeytar.setPassword.mockResolvedValue(undefined);
-      mockKeytar.getPassword.mockResolvedValue('test');
-      mockKeytar.deletePassword.mockResolvedValue(true);
-
-      await storage.checkKeychainAvailability();
-      await storage.checkKeychainAvailability();
-
-      expect(mockKeytar.setPassword).toHaveBeenCalledTimes(1);
-    });
-  });
-
-  describe('with keychain unavailable', () => {
-    beforeEach(async () => {
-      // Force keychain to be unavailable
-      mockKeytar.setPassword.mockRejectedValue(new Error('keychain error'));
-      await storage.checkKeychainAvailability();
-    });
-
-    it('getCredentials should throw', async () => {
-      await expect(storage.getCredentials('server')).rejects.toThrow(
-        'Keychain is not available',
-      );
-    });
-
-    it('setCredentials should throw', async () => {
-      await expect(storage.setCredentials(validCredentials)).rejects.toThrow(
-        'Keychain is not available',
-      );
-    });
-
-    it('deleteCredentials should throw', async () => {
-      await expect(storage.deleteCredentials('server')).rejects.toThrow(
-        'Keychain is not available',
-      );
-    });
-
-    it('listServers should throw', async () => {
-      await expect(storage.listServers()).rejects.toThrow(
-        'Keychain is not available',
-      );
-    });
-
-    it('getAllCredentials should throw', async () => {
-      await expect(storage.getAllCredentials()).rejects.toThrow(
-        'Keychain is not available',
-      );
-    });
-  });
-
   describe('with keychain available', () => {
-    beforeEach(async () => {
-      mockKeytar.setPassword.mockResolvedValue(undefined);
-      mockKeytar.getPassword.mockResolvedValue('test');
-      mockKeytar.deletePassword.mockResolvedValue(true);
-      await storage.checkKeychainAvailability();
-      // Reset mocks after availability check
-      vi.resetAllMocks();
+    beforeEach(() => {
+      vi.spyOn(KeychainService.prototype, 'isAvailable').mockResolvedValue(
+        true,
+      );
     });
 
-    describe('getCredentials', () => {
-      it('should return null if no credentials are found', async () => {
-        mockKeytar.getPassword.mockResolvedValue(null);
-        const result = await storage.getCredentials('test-server');
-        expect(result).toBeNull();
-        expect(mockKeytar.getPassword).toHaveBeenCalledWith(
-          mockServiceName,
-          'test-server',
-        );
-      });
+    it('should store and retrieve credentials correctly', async () => {
+      await storage.setCredentials(validCredentials);
+      const retrieved = await storage.getCredentials('test-server');
 
-      it('should return credentials if found and not expired', async () => {
-        mockKeytar.getPassword.mockResolvedValue(
-          JSON.stringify(validCredentials),
-        );
-        const result = await storage.getCredentials('test-server');
-        expect(result).toEqual(validCredentials);
-      });
-
-      it('should return null if credentials have expired', async () => {
-        const expiredCreds = {
-          ...validCredentials,
-          token: { ...validCredentials.token, expiresAt: Date.now() - 1000 },
-        };
-        mockKeytar.getPassword.mockResolvedValue(JSON.stringify(expiredCreds));
-        const result = await storage.getCredentials('test-server');
-        expect(result).toBeNull();
-      });
-
-      it('should throw if stored data is corrupted JSON', async () => {
-        mockKeytar.getPassword.mockResolvedValue('not-json');
-        await expect(storage.getCredentials('test-server')).rejects.toThrow(
-          'Failed to parse stored credentials for test-server',
-        );
-      });
+      expect(retrieved?.token.accessToken).toBe('access-token');
+      expect(retrieved?.serverName).toBe('test-server');
     });
 
-    describe('setCredentials', () => {
-      it('should save credentials to keychain', async () => {
-        vi.useFakeTimers();
-        mockKeytar.setPassword.mockResolvedValue(undefined);
-        await storage.setCredentials(validCredentials);
-        expect(mockKeytar.setPassword).toHaveBeenCalledWith(
-          mockServiceName,
-          'test-server',
-          JSON.stringify({ ...validCredentials, updatedAt: Date.now() }),
-        );
-      });
+    it('should return null if no credentials are found or they are expired', async () => {
+      expect(await storage.getCredentials('missing')).toBeNull();
 
-      it('should throw if saving to keychain fails', async () => {
-        mockKeytar.setPassword.mockRejectedValue(
-          new Error('keychain write error'),
-        );
-        await expect(storage.setCredentials(validCredentials)).rejects.toThrow(
-          'keychain write error',
-        );
-      });
+      const expiredCreds = {
+        ...validCredentials,
+        token: { ...validCredentials.token, expiresAt: Date.now() - 1000 },
+      };
+      await storage.setCredentials(expiredCreds);
+      expect(await storage.getCredentials('test-server')).toBeNull();
     });
 
-    describe('deleteCredentials', () => {
-      it('should delete credentials from keychain', async () => {
-        mockKeytar.deletePassword.mockResolvedValue(true);
-        await storage.deleteCredentials('test-server');
-        expect(mockKeytar.deletePassword).toHaveBeenCalledWith(
-          mockServiceName,
-          'test-server',
-        );
-      });
-
-      it('should throw if no credentials were found to delete', async () => {
-        mockKeytar.deletePassword.mockResolvedValue(false);
-        await expect(storage.deleteCredentials('test-server')).rejects.toThrow(
-          'No credentials found for test-server',
-        );
-      });
-
-      it('should throw if deleting from keychain fails', async () => {
-        mockKeytar.deletePassword.mockRejectedValue(
-          new Error('keychain delete error'),
-        );
-        await expect(storage.deleteCredentials('test-server')).rejects.toThrow(
-          'keychain delete error',
-        );
-      });
+    it('should throw if stored data is corrupted JSON', async () => {
+      storageState.set('bad-server', 'not-json');
+      await expect(storage.getCredentials('bad-server')).rejects.toThrow(
+        /Failed to parse/,
+      );
     });
 
-    describe('listServers', () => {
-      it('should return a list of server names', async () => {
-        mockKeytar.findCredentials.mockResolvedValue([
-          { account: 'server1', password: '' },
-          { account: 'server2', password: '' },
-        ]);
-        const result = await storage.listServers();
-        expect(result).toEqual(['server1', 'server2']);
+    it('should list servers and filter internal keys', async () => {
+      await storage.setCredentials(validCredentials);
+      await storage.setCredentials({
+        ...validCredentials,
+        serverName: 'server2',
       });
+      storageState.set(`${KEYCHAIN_TEST_PREFIX}internal`, '...');
+      storageState.set('__secret__key', '...');
 
-      it('should not include internal test keys in the server list', async () => {
-        mockKeytar.findCredentials.mockResolvedValue([
-          { account: 'server1', password: '' },
-          {
-            account: `__keychain_test__${mockCryptoRandomBytesString}`,
-            password: '',
-          },
-          { account: 'server2', password: '' },
-        ]);
-        const result = await storage.listServers();
-        expect(result).toEqual(['server1', 'server2']);
-      });
-
-      it('should return an empty array on error', async () => {
-        const error = new Error('find error');
-        mockKeytar.findCredentials.mockRejectedValue(error);
-        const result = await storage.listServers();
-        expect(result).toEqual([]);
-        expect(coreEvents.emitFeedback).toHaveBeenCalledWith(
-          'error',
-          'Failed to list servers from keychain',
-          error,
-        );
-      });
+      const servers = await storage.listServers();
+      expect(servers).toEqual(['test-server', 'server2']);
     });
 
-    describe('getAllCredentials', () => {
-      it('should return a map of all valid credentials and emit feedback for invalid ones', async () => {
-        const creds2 = {
-          ...validCredentials,
-          serverName: 'server2',
-        };
-        const expiredCreds = {
-          ...validCredentials,
-          serverName: 'expired-server',
-          token: { ...validCredentials.token, expiresAt: Date.now() - 1000 },
-        };
-        const structurallyInvalidCreds = {
-          serverName: 'invalid-server',
-        };
+    it('should handle getAllCredentials with individual parse errors', async () => {
+      await storage.setCredentials(validCredentials);
+      storageState.set('bad', 'not-json');
+      const emitFeedbackSpy = vi.spyOn(coreEvents, 'emitFeedback');
 
-        mockKeytar.findCredentials.mockResolvedValue([
-          {
-            account: 'test-server',
-            password: JSON.stringify(validCredentials),
-          },
-          { account: 'server2', password: JSON.stringify(creds2) },
-          {
-            account: 'expired-server',
-            password: JSON.stringify(expiredCreds),
-          },
-          { account: 'bad-server', password: 'not-json' },
-          {
-            account: 'invalid-server',
-            password: JSON.stringify(structurallyInvalidCreds),
-          },
-        ]);
-
-        const result = await storage.getAllCredentials();
-        expect(result.size).toBe(2);
-        expect(result.get('test-server')).toEqual(validCredentials);
-        expect(result.get('server2')).toEqual(creds2);
-        expect(result.has('expired-server')).toBe(false);
-        expect(result.has('bad-server')).toBe(false);
-        expect(result.has('invalid-server')).toBe(false);
-
-        expect(coreEvents.emitFeedback).toHaveBeenCalledWith(
-          'error',
-          'Failed to parse credentials for bad-server',
-          expect.any(SyntaxError),
-        );
-        expect(coreEvents.emitFeedback).toHaveBeenCalledWith(
-          'error',
-          'Failed to parse credentials for invalid-server',
-          expect.any(Error),
-        );
-      });
-
-      it('should emit feedback and return empty map if findCredentials fails', async () => {
-        const error = new Error('find all error');
-        mockKeytar.findCredentials.mockRejectedValue(error);
-
-        const result = await storage.getAllCredentials();
-        expect(result.size).toBe(0);
-        expect(coreEvents.emitFeedback).toHaveBeenCalledWith(
-          'error',
-          'Failed to get all credentials from keychain',
-          error,
-        );
-      });
+      const result = await storage.getAllCredentials();
+      expect(result.size).toBe(1);
+      expect(emitFeedbackSpy).toHaveBeenCalled();
     });
 
-    describe('clearAll', () => {
-      it('should delete all credentials for the service', async () => {
-        mockKeytar.findCredentials.mockResolvedValue([
-          { account: 'server1', password: '' },
-          { account: 'server2', password: '' },
-        ]);
-        mockKeytar.deletePassword.mockResolvedValue(true);
+    it('should aggregate errors in clearAll', async () => {
+      storageState.set('s1', '...');
+      storageState.set('s2', '...');
 
-        await storage.clearAll();
+      // Aggregating a system error (rejection)
+      vi.spyOn(KeychainService.prototype, 'deletePassword')
+        .mockResolvedValueOnce(true)
+        .mockRejectedValueOnce(new Error('system fail'));
 
-        expect(mockKeytar.deletePassword).toHaveBeenCalledTimes(2);
-        expect(mockKeytar.deletePassword).toHaveBeenCalledWith(
-          mockServiceName,
-          'server1',
-        );
-        expect(mockKeytar.deletePassword).toHaveBeenCalledWith(
-          mockServiceName,
-          'server2',
-        );
-      });
+      await expect(storage.clearAll()).rejects.toThrow(
+        /Failed to clear some credentials: system fail/,
+      );
 
-      it('should throw an aggregated error if deletions fail', async () => {
-        mockKeytar.findCredentials.mockResolvedValue([
-          { account: 'server1', password: '' },
-          { account: 'server2', password: '' },
-        ]);
-        mockKeytar.deletePassword
-          .mockResolvedValueOnce(true)
-          .mockRejectedValueOnce(new Error('delete failed'));
+      // Aggregating a 'not found' error (returns false)
+      vi.spyOn(KeychainService.prototype, 'deletePassword')
+        .mockResolvedValueOnce(true)
+        .mockResolvedValueOnce(false);
 
-        await expect(storage.clearAll()).rejects.toThrow(
-          'Failed to clear some credentials: delete failed',
-        );
-      });
+      await expect(storage.clearAll()).rejects.toThrow(
+        /Failed to clear some credentials: No credentials found/,
+      );
     });
 
-    describe('Secrets', () => {
-      it('should set and get a secret', async () => {
-        mockKeytar.setPassword.mockResolvedValue(undefined);
-        mockKeytar.getPassword.mockResolvedValue('secret-value');
+    it('should manage secrets with prefix independently', async () => {
+      await storage.setSecret('key1', 'val1');
+      await storage.setCredentials(validCredentials);
 
-        await storage.setSecret('secret-key', 'secret-value');
-        const value = await storage.getSecret('secret-key');
+      expect(await storage.getSecret('key1')).toBe('val1');
+      expect(await storage.listSecrets()).toEqual(['key1']);
+      expect(await storage.listServers()).not.toContain('key1');
+    });
+  });
 
-        expect(mockKeytar.setPassword).toHaveBeenCalledWith(
-          mockServiceName,
-          '__secret__secret-key',
-          'secret-value',
-        );
-        expect(mockKeytar.getPassword).toHaveBeenCalledWith(
-          mockServiceName,
-          '__secret__secret-key',
-        );
-        expect(value).toBe('secret-value');
-      });
+  describe('unavailability handling', () => {
+    beforeEach(() => {
+      vi.spyOn(KeychainService.prototype, 'isAvailable').mockResolvedValue(
+        false,
+      );
+      vi.spyOn(KeychainService.prototype, 'getPassword').mockRejectedValue(
+        new Error('Keychain is not available'),
+      );
+      vi.spyOn(KeychainService.prototype, 'setPassword').mockRejectedValue(
+        new Error('Keychain is not available'),
+      );
+      vi.spyOn(KeychainService.prototype, 'deletePassword').mockRejectedValue(
+        new Error('Keychain is not available'),
+      );
+      vi.spyOn(KeychainService.prototype, 'findCredentials').mockRejectedValue(
+        new Error('Keychain is not available'),
+      );
+    });
 
-      it('should delete a secret', async () => {
-        mockKeytar.deletePassword.mockResolvedValue(true);
-        await storage.deleteSecret('secret-key');
-        expect(mockKeytar.deletePassword).toHaveBeenCalledWith(
-          mockServiceName,
-          '__secret__secret-key',
-        );
-      });
+    it.each([
+      { method: 'getCredentials', args: ['s'] },
+      { method: 'setCredentials', args: [validCredentials] },
+      { method: 'deleteCredentials', args: ['s'] },
+      { method: 'clearAll', args: [] },
+    ])(
+      '$method should propagate unavailability error',
+      async ({ method, args }) => {
+        await expect(
+          (
+            storage as unknown as Record<
+              string,
+              (...args: unknown[]) => Promise<unknown>
+            >
+          )[method](...args),
+        ).rejects.toThrow('Keychain is not available');
+      },
+    );
 
-      it('should list secrets', async () => {
-        mockKeytar.findCredentials.mockResolvedValue([
-          { account: '__secret__secret1', password: '' },
-          { account: '__secret__secret2', password: '' },
-          { account: 'server1', password: '' },
-        ]);
-        const secrets = await storage.listSecrets();
-        expect(secrets).toEqual(['secret1', 'secret2']);
-      });
-
-      it('should not list secrets in listServers', async () => {
-        mockKeytar.findCredentials.mockResolvedValue([
-          { account: '__secret__secret1', password: '' },
-          { account: 'server1', password: '' },
-        ]);
-        const servers = await storage.listServers();
-        expect(servers).toEqual(['server1']);
-      });
+    it.each([
+      { method: 'listServers' },
+      { method: 'getAllCredentials' },
+      { method: 'listSecrets' },
+    ])('$method should emit feedback and return empty', async ({ method }) => {
+      const emitFeedbackSpy = vi.spyOn(coreEvents, 'emitFeedback');
+      expect(
+        await (storage as unknown as Record<string, () => Promise<unknown>>)[
+          method
+        ](),
+      ).toEqual(method === 'getAllCredentials' ? new Map() : []);
+      expect(emitFeedbackSpy).toHaveBeenCalledWith(
+        'error',
+        expect.any(String),
+        expect.any(Error),
+      );
     });
   });
 });

packages/core/src/mcp/token-storage/keychain-token-storage.ts

@@ -4,70 +4,30 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import * as crypto from 'node:crypto';
 import { BaseTokenStorage } from './base-token-storage.js';
 import type { OAuthCredentials, SecretStorage } from './types.js';
 import { coreEvents } from '../../utils/events.js';
-import { KeychainAvailabilityEvent } from '../../telemetry/types.js';
-
-interface Keytar {
-  getPassword(service: string, account: string): Promise<string | null>;
-  setPassword(
-    service: string,
-    account: string,
-    password: string,
-  ): Promise<void>;
-  deletePassword(service: string, account: string): Promise<boolean>;
-  findCredentials(
-    service: string,
-  ): Promise<Array<{ account: string; password: string }>>;
-}
-
-const KEYCHAIN_TEST_PREFIX = '__keychain_test__';
-const SECRET_PREFIX = '__secret__';
+import { KeychainService } from '../../services/keychainService.js';
+import {
+  KEYCHAIN_TEST_PREFIX,
+  SECRET_PREFIX,
+} from '../../services/keychainTypes.js';
 
 export class KeychainTokenStorage
   extends BaseTokenStorage
   implements SecretStorage
 {
-  private keychainAvailable: boolean | null = null;
-  private keytarModule: Keytar | null = null;
-  private keytarLoadAttempted = false;
+  private readonly keychainService: KeychainService;
 
-  async getKeytar(): Promise<Keytar | null> {
-    // If we've already tried loading (successfully or not), return the result
-    if (this.keytarLoadAttempted) {
-      return this.keytarModule;
-    }
-
-    this.keytarLoadAttempted = true;
-
-    try {
-      // Try to import keytar without any timeout - let the OS handle it
-      const moduleName = 'keytar';
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-      const module = await import(moduleName);
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-      this.keytarModule = module.default || module;
-    } catch (_) {
-      //Keytar is optional so we shouldn't raise an error of log anything.
-    }
-    return this.keytarModule;
+  constructor(serviceName: string) {
+    super(serviceName);
+    this.keychainService = new KeychainService(serviceName);
   }
 
   async getCredentials(serverName: string): Promise<OAuthCredentials | null> {
-    if (!(await this.checkKeychainAvailability())) {
-      throw new Error('Keychain is not available');
-    }
-
-    const keytar = await this.getKeytar();
-    if (!keytar) {
-      throw new Error('Keytar module not available');
-    }
-
     try {
       const sanitizedName = this.sanitizeServerName(serverName);
-      const data = await keytar.getPassword(this.serviceName, sanitizedName);
+      const data = await this.keychainService.getPassword(sanitizedName);
 
       if (!data) {
         return null;
@@ -90,15 +50,6 @@ export class KeychainTokenStorage
   }
 
   async setCredentials(credentials: OAuthCredentials): Promise<void> {
-    if (!(await this.checkKeychainAvailability())) {
-      throw new Error('Keychain is not available');
-    }
-
-    const keytar = await this.getKeytar();
-    if (!keytar) {
-      throw new Error('Keytar module not available');
-    }
-
     this.validateCredentials(credentials);
 
     const sanitizedName = this.sanitizeServerName(credentials.serverName);
@@ -108,24 +59,12 @@ export class KeychainTokenStorage
     };
 
     const data = JSON.stringify(updatedCredentials);
-    await keytar.setPassword(this.serviceName, sanitizedName, data);
+    await this.keychainService.setPassword(sanitizedName, data);
   }
 
   async deleteCredentials(serverName: string): Promise<void> {
-    if (!(await this.checkKeychainAvailability())) {
-      throw new Error('Keychain is not available');
-    }
-
-    const keytar = await this.getKeytar();
-    if (!keytar) {
-      throw new Error('Keytar module not available');
-    }
-
     const sanitizedName = this.sanitizeServerName(serverName);
-    const deleted = await keytar.deletePassword(
-      this.serviceName,
-      sanitizedName,
-    );
+    const deleted = await this.keychainService.deletePassword(sanitizedName);
 
     if (!deleted) {
       throw new Error(`No credentials found for ${serverName}`);
@@ -133,17 +72,8 @@ export class KeychainTokenStorage
   }
 
   async listServers(): Promise<string[]> {
-    if (!(await this.checkKeychainAvailability())) {
-      throw new Error('Keychain is not available');
-    }
-
-    const keytar = await this.getKeytar();
-    if (!keytar) {
-      throw new Error('Keytar module not available');
-    }
-
     try {
-      const credentials = await keytar.findCredentials(this.serviceName);
+      const credentials = await this.keychainService.findCredentials();
       return credentials
         .filter(
           (cred) =>
@@ -162,20 +92,9 @@ export class KeychainTokenStorage
   }
 
   async getAllCredentials(): Promise<Map<string, OAuthCredentials>> {
-    if (!(await this.checkKeychainAvailability())) {
-      throw new Error('Keychain is not available');
-    }
-
-    const keytar = await this.getKeytar();
-    if (!keytar) {
-      throw new Error('Keytar module not available');
-    }
-
     const result = new Map<string, OAuthCredentials>();
     try {
-      const credentials = (
-        await keytar.findCredentials(this.serviceName)
-      ).filter(
+      const credentials = (await this.keychainService.findCredentials()).filter(
         (c) =>
           !c.account.startsWith(KEYCHAIN_TEST_PREFIX) &&
           !c.account.startsWith(SECRET_PREFIX),
@@ -208,119 +127,48 @@ export class KeychainTokenStorage
   }
 
   async clearAll(): Promise<void> {
-    if (!(await this.checkKeychainAvailability())) {
-      throw new Error('Keychain is not available');
-    }
-
-    const servers = this.keytarModule
-      ? await this.keytarModule
-          .findCredentials(this.serviceName)
-          .then((creds) => creds.map((c) => c.account))
-          .catch((error: Error) => {
-            throw new Error(
-              `Failed to list servers for clearing: ${error.message}`,
-            );
-          })
-      : [];
-    const errors: Error[] = [];
-
-    for (const server of servers) {
-      try {
-        await this.deleteCredentials(server);
-      } catch (error) {
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
-        errors.push(error as Error);
-      }
-    }
-
-    if (errors.length > 0) {
-      throw new Error(
-        `Failed to clear some credentials: ${errors.map((e) => e.message).join(', ')}`,
-      );
-    }
-  }
-
-  // Checks whether or not a set-get-delete cycle with the keychain works.
-  // Returns false if any operation fails.
-  async checkKeychainAvailability(): Promise<boolean> {
-    if (this.keychainAvailable !== null) {
-      return this.keychainAvailable;
-    }
-
     try {
-      const keytar = await this.getKeytar();
-      if (!keytar) {
-        this.keychainAvailable = false;
-        return false;
+      const credentials = await this.keychainService.findCredentials();
+      const errors: Error[] = [];
+
+      for (const cred of credentials) {
+        try {
+          await this.deleteCredentials(cred.account);
+        } catch (error) {
+          // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
+          errors.push(error as Error);
+        }
       }
 
-      const testAccount = `${KEYCHAIN_TEST_PREFIX}${crypto.randomBytes(8).toString('hex')}`;
-      const testPassword = 'test';
-
-      await keytar.setPassword(this.serviceName, testAccount, testPassword);
-      const retrieved = await keytar.getPassword(this.serviceName, testAccount);
-      const deleted = await keytar.deletePassword(
-        this.serviceName,
-        testAccount,
+      if (errors.length > 0) {
+        throw new Error(
+          `Failed to clear some credentials: ${errors.map((e) => e.message).join(', ')}`,
+        );
+      }
+    } catch (error) {
+      coreEvents.emitFeedback(
+        'error',
+        'Failed to clear credentials from keychain',
+        error,
       );
-
-      const success = deleted && retrieved === testPassword;
-      this.keychainAvailable = success;
-
-      coreEvents.emitTelemetryKeychainAvailability(
-        new KeychainAvailabilityEvent(success),
-      );
-
-      return success;
-    } catch (_error) {
-      this.keychainAvailable = false;
-
-      // Do not log the raw error message to avoid potential PII leaks
-      // (e.g. from OS-level error messages containing file paths)
-      coreEvents.emitTelemetryKeychainAvailability(
-        new KeychainAvailabilityEvent(false),
-      );
-
-      return false;
+      throw error;
     }
   }
 
   async isAvailable(): Promise<boolean> {
-    return this.checkKeychainAvailability();
+    return this.keychainService.isAvailable();
   }
 
   async setSecret(key: string, value: string): Promise<void> {
-    if (!(await this.checkKeychainAvailability())) {
-      throw new Error('Keychain is not available');
-    }
-    const keytar = await this.getKeytar();
-    if (!keytar) {
-      throw new Error('Keytar module not available');
-    }
-    await keytar.setPassword(this.serviceName, `${SECRET_PREFIX}${key}`, value);
+    await this.keychainService.setPassword(`${SECRET_PREFIX}${key}`, value);
   }
 
   async getSecret(key: string): Promise<string | null> {
-    if (!(await this.checkKeychainAvailability())) {
-      throw new Error('Keychain is not available');
-    }
-    const keytar = await this.getKeytar();
-    if (!keytar) {
-      throw new Error('Keytar module not available');
-    }
-    return keytar.getPassword(this.serviceName, `${SECRET_PREFIX}${key}`);
+    return this.keychainService.getPassword(`${SECRET_PREFIX}${key}`);
   }
 
   async deleteSecret(key: string): Promise<void> {
-    if (!(await this.checkKeychainAvailability())) {
-      throw new Error('Keychain is not available');
-    }
-    const keytar = await this.getKeytar();
-    if (!keytar) {
-      throw new Error('Keytar module not available');
-    }
-    const deleted = await keytar.deletePassword(
-      this.serviceName,
+    const deleted = await this.keychainService.deletePassword(
       `${SECRET_PREFIX}${key}`,
     );
     if (!deleted) {
@@ -329,15 +177,8 @@ export class KeychainTokenStorage
   }
 
   async listSecrets(): Promise<string[]> {
-    if (!(await this.checkKeychainAvailability())) {
-      throw new Error('Keychain is not available');
-    }
-    const keytar = await this.getKeytar();
-    if (!keytar) {
-      throw new Error('Keytar module not available');
-    }
     try {
-      const credentials = await keytar.findCredentials(this.serviceName);
+      const credentials = await this.keychainService.findCredentials();
       return credentials
         .filter((cred) => cred.account.startsWith(SECRET_PREFIX))
         .map((cred) => cred.account.substring(SECRET_PREFIX.length));

packages/core/src/services/keychainService.test.ts

@@ -0,0 +1,183 @@
+/**
+ * @license
+ * Copyright 2026 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest';
+import { KeychainService } from './keychainService.js';
+import { coreEvents } from '../utils/events.js';
+import { debugLogger } from '../utils/debugLogger.js';
+
+type MockKeychain = {
+  getPassword: Mock | undefined;
+  setPassword: Mock | undefined;
+  deletePassword: Mock | undefined;
+  findCredentials: Mock | undefined;
+};
+
+const mockKeytar: MockKeychain = {
+  getPassword: vi.fn(),
+  setPassword: vi.fn(),
+  deletePassword: vi.fn(),
+  findCredentials: vi.fn(),
+};
+
+vi.mock('keytar', () => ({ default: mockKeytar }));
+
+vi.mock('../utils/events.js', () => ({
+  coreEvents: { emitTelemetryKeychainAvailability: vi.fn() },
+}));
+
+vi.mock('../utils/debugLogger.js', () => ({
+  debugLogger: { log: vi.fn() },
+}));
+
+describe('KeychainService', () => {
+  let service: KeychainService;
+  const SERVICE_NAME = 'test-service';
+  let passwords: Record<string, string> = {};
+
+  beforeEach(() => {
+    vi.clearAllMocks();
+    service = new KeychainService(SERVICE_NAME);
+    passwords = {};
+
+    // Stateful mock implementation to verify behavioral correctness
+    mockKeytar.setPassword?.mockImplementation((_svc, acc, val) => {
+      passwords[acc] = val;
+      return Promise.resolve();
+    });
+    mockKeytar.getPassword?.mockImplementation((_svc, acc) =>
+      Promise.resolve(passwords[acc] ?? null),
+    );
+    mockKeytar.deletePassword?.mockImplementation((_svc, acc) => {
+      const exists = !!passwords[acc];
+      delete passwords[acc];
+      return Promise.resolve(exists);
+    });
+    mockKeytar.findCredentials?.mockImplementation(() =>
+      Promise.resolve(
+        Object.entries(passwords).map(([account, password]) => ({
+          account,
+          password,
+        })),
+      ),
+    );
+  });
+
+  describe('isAvailable', () => {
+    it('should return true and emit telemetry on successful functional test', async () => {
+      const available = await service.isAvailable();
+
+      expect(available).toBe(true);
+      expect(mockKeytar.setPassword).toHaveBeenCalled();
+      expect(coreEvents.emitTelemetryKeychainAvailability).toHaveBeenCalledWith(
+        expect.objectContaining({ available: true }),
+      );
+    });
+
+    it('should return false, log error, and emit telemetry on failed functional test', async () => {
+      mockKeytar.setPassword?.mockRejectedValue(new Error('locked'));
+
+      const available = await service.isAvailable();
+
+      expect(available).toBe(false);
+      expect(debugLogger.log).toHaveBeenCalledWith(
+        expect.stringContaining('encountered an error'),
+        'locked',
+      );
+      expect(coreEvents.emitTelemetryKeychainAvailability).toHaveBeenCalledWith(
+        expect.objectContaining({ available: false }),
+      );
+    });
+
+    it('should return false, log validation error, and emit telemetry on module load failure', async () => {
+      const originalMock = mockKeytar.getPassword;
+      mockKeytar.getPassword = undefined; // Break schema
+
+      const available = await service.isAvailable();
+
+      expect(available).toBe(false);
+      expect(debugLogger.log).toHaveBeenCalledWith(
+        expect.stringContaining('failed structural validation'),
+        expect.objectContaining({ getPassword: expect.any(Array) }),
+      );
+      expect(coreEvents.emitTelemetryKeychainAvailability).toHaveBeenCalledWith(
+        expect.objectContaining({ available: false }),
+      );
+
+      mockKeytar.getPassword = originalMock;
+    });
+
+    it('should log failure if functional test cycle returns false', async () => {
+      mockKeytar.getPassword?.mockResolvedValue('wrong-password');
+
+      const available = await service.isAvailable();
+
+      expect(available).toBe(false);
+      expect(debugLogger.log).toHaveBeenCalledWith(
+        expect.stringContaining('functional verification failed'),
+      );
+    });
+
+    it('should cache the result and handle concurrent initialization attempts once', async () => {
+      await Promise.all([
+        service.isAvailable(),
+        service.isAvailable(),
+        service.isAvailable(),
+      ]);
+
+      expect(mockKeytar.setPassword).toHaveBeenCalledTimes(1);
+    });
+  });
+
+  describe('Password Operations', () => {
+    beforeEach(async () => {
+      await service.isAvailable();
+      vi.clearAllMocks();
+    });
+
+    it('should store, retrieve, and delete passwords correctly', async () => {
+      await service.setPassword('acc1', 'secret1');
+      await service.setPassword('acc2', 'secret2');
+
+      expect(await service.getPassword('acc1')).toBe('secret1');
+      expect(await service.getPassword('acc2')).toBe('secret2');
+
+      const creds = await service.findCredentials();
+      expect(creds).toHaveLength(2);
+      expect(creds).toContainEqual({ account: 'acc1', password: 'secret1' });
+
+      expect(await service.deletePassword('acc1')).toBe(true);
+      expect(await service.getPassword('acc1')).toBeNull();
+      expect(await service.findCredentials()).toHaveLength(1);
+    });
+
+    it('getPassword should return null if key is missing', async () => {
+      expect(await service.getPassword('missing')).toBeNull();
+    });
+  });
+
+  describe('When Unavailable', () => {
+    beforeEach(() => {
+      mockKeytar.setPassword?.mockRejectedValue(new Error('Unavailable'));
+    });
+
+    it.each([
+      { method: 'getPassword', args: ['acc'] },
+      { method: 'setPassword', args: ['acc', 'val'] },
+      { method: 'deletePassword', args: ['acc'] },
+      { method: 'findCredentials', args: [] },
+    ])('$method should throw a consistent error', async ({ method, args }) => {
+      await expect(
+        (
+          service as unknown as Record<
+            string,
+            (...args: unknown[]) => Promise<unknown>
+          >
+        )[method](...args),
+      ).rejects.toThrow('Keychain is not available');
+    });
+  });
+});

packages/core/src/services/keychainService.ts

@@ -0,0 +1,147 @@
+/**
+ * @license
+ * Copyright 2026 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import * as crypto from 'node:crypto';
+import { coreEvents } from '../utils/events.js';
+import { KeychainAvailabilityEvent } from '../telemetry/types.js';
+import { debugLogger } from '../utils/debugLogger.js';
+import {
+  type Keychain,
+  KeychainSchema,
+  KEYCHAIN_TEST_PREFIX,
+} from './keychainTypes.js';
+
+/**
+ * Service for interacting with OS-level secure storage (e.g. keytar).
+ */
+export class KeychainService {
+  // Track an ongoing initialization attempt to avoid race conditions.
+  private initializationPromise?: Promise<Keychain | null>;
+
+  /**
+   * @param serviceName Unique identifier for the app in the OS keychain.
+   */
+  constructor(private readonly serviceName: string) {}
+
+  async isAvailable(): Promise<boolean> {
+    return (await this.getKeychain()) !== null;
+  }
+
+  /**
+   * Retrieves a secret for the given account.
+   * @throws Error if the keychain is unavailable.
+   */
+  async getPassword(account: string): Promise<string | null> {
+    const keychain = await this.getKeychainOrThrow();
+    return keychain.getPassword(this.serviceName, account);
+  }
+
+  /**
+   * Securely stores a secret.
+   * @throws Error if the keychain is unavailable.
+   */
+  async setPassword(account: string, value: string): Promise<void> {
+    const keychain = await this.getKeychainOrThrow();
+    await keychain.setPassword(this.serviceName, account, value);
+  }
+
+  /**
+   * Removes a secret from the keychain.
+   * @returns true if the secret was deleted, false otherwise.
+   * @throws Error if the keychain is unavailable.
+   */
+  async deletePassword(account: string): Promise<boolean> {
+    const keychain = await this.getKeychainOrThrow();
+    return keychain.deletePassword(this.serviceName, account);
+  }
+
+  /**
+   * Lists all account/secret pairs stored under this service.
+   * @throws Error if the keychain is unavailable.
+   */
+  async findCredentials(): Promise<
+    Array<{ account: string; password: string }>
+  > {
+    const keychain = await this.getKeychainOrThrow();
+    return keychain.findCredentials(this.serviceName);
+  }
+
+  private async getKeychainOrThrow(): Promise<Keychain> {
+    const keychain = await this.getKeychain();
+    if (!keychain) {
+      throw new Error('Keychain is not available');
+    }
+    return keychain;
+  }
+
+  private getKeychain(): Promise<Keychain | null> {
+    return (this.initializationPromise ??= this.initializeKeychain());
+  }
+
+  // High-level orchestration of the loading and testing cycle.
+  private async initializeKeychain(): Promise<Keychain | null> {
+    let resultKeychain: Keychain | null = null;
+
+    try {
+      const keychainModule = await this.loadKeychainModule();
+      if (keychainModule) {
+        if (await this.isKeychainFunctional(keychainModule)) {
+          resultKeychain = keychainModule;
+        } else {
+          debugLogger.log('Keychain functional verification failed');
+        }
+      }
+    } catch (error) {
+      // Avoid logging full error objects to prevent PII exposure.
+      const message = error instanceof Error ? error.message : String(error);
+      debugLogger.log('Keychain initialization encountered an error:', message);
+    }
+
+    coreEvents.emitTelemetryKeychainAvailability(
+      new KeychainAvailabilityEvent(resultKeychain !== null),
+    );
+
+    return resultKeychain;
+  }
+
+  // Low-level dynamic loading and structural validation.
+  private async loadKeychainModule(): Promise<Keychain | null> {
+    const moduleName = 'keytar';
+    const module: unknown = await import(moduleName);
+    const potential = (this.isRecord(module) && module['default']) || module;
+
+    const result = KeychainSchema.safeParse(potential);
+    if (result.success) {
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
+      return potential as Keychain;
+    }
+
+    debugLogger.log(
+      'Keychain module failed structural validation:',
+      result.error.flatten().fieldErrors,
+    );
+    return null;
+  }
+
+  private isRecord(obj: unknown): obj is Record<string, unknown> {
+    return typeof obj === 'object' && obj !== null;
+  }
+
+  // Performs a set-get-delete cycle to verify keychain functionality.
+  private async isKeychainFunctional(keychain: Keychain): Promise<boolean> {
+    const testAccount = `${KEYCHAIN_TEST_PREFIX}${crypto.randomBytes(8).toString('hex')}`;
+    const testPassword = 'test';
+
+    await keychain.setPassword(this.serviceName, testAccount, testPassword);
+    const retrieved = await keychain.getPassword(this.serviceName, testAccount);
+    const deleted = await keychain.deletePassword(
+      this.serviceName,
+      testAccount,
+    );
+
+    return deleted && retrieved === testPassword;
+  }
+}

packages/core/src/services/keychainTypes.ts

@@ -0,0 +1,38 @@
+/**
+ * @license
+ * Copyright 2026 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { z } from 'zod';
+
+/**
+ * Interface for OS-level secure storage operations.
+ * Note: Method names must match the underlying library (e.g. keytar)
+ * to support correct dynamic loading and schema validation.
+ */
+export interface Keychain {
+  getPassword(service: string, account: string): Promise<string | null>;
+  setPassword(
+    service: string,
+    account: string,
+    password: string,
+  ): Promise<void>;
+  deletePassword(service: string, account: string): Promise<boolean>;
+  findCredentials(
+    service: string,
+  ): Promise<Array<{ account: string; password: string }>>;
+}
+
+/**
+ * Zod schema to validate that a module satisfies the Keychain interface.
+ */
+export const KeychainSchema = z.object({
+  getPassword: z.function(),
+  setPassword: z.function(),
+  deletePassword: z.function(),
+  findCredentials: z.function(),
+});
+
+export const KEYCHAIN_TEST_PREFIX = '__keychain_test__';
+export const SECRET_PREFIX = '__secret__';