From 37a8955494b88991f19874f9d335282727a26577 Mon Sep 17 00:00:00 2001 From: Ralph Khreish <35776126+Crunchyman-ralph@users.noreply.github.com> Date: Thu, 4 Sep 2025 23:07:18 +0200 Subject: [PATCH] chore: implement requested changes --- package.json | 8 +-- .../tm-core/src/auth/auth-manager.test.ts | 71 +++++++++++++++++-- packages/tm-core/src/auth/auth-manager.ts | 8 +-- .../tm-core/src/storage/storage-factory.ts | 8 ++- 4 files changed, 78 insertions(+), 17 deletions(-) diff --git a/package.json b/package.json index 5535e0f7..be808a92 100644 --- a/package.json +++ b/package.json @@ -19,10 +19,10 @@ "build:packages": "npm run build:core && npm run build:cli", "build:core": "cd packages/tm-core && npm run build", "build:cli": "cd apps/cli && npm run build", - "test": "node --experimental-vm-modules node_modules/.bin/jest", - "test:fails": "node --experimental-vm-modules node_modules/.bin/jest --onlyFailures", - "test:watch": "node --experimental-vm-modules node_modules/.bin/jest --watch", - "test:coverage": "node --experimental-vm-modules node_modules/.bin/jest --coverage", + "test": "vitest run", + "test:fails": "vitest run --changed", + "test:watch": "vitest", + "test:coverage": "vitest run --coverage", "test:e2e": "./tests/e2e/run_e2e.sh", "test:e2e-report": "./tests/e2e/run_e2e.sh --analyze-log", "postpack": "chmod +x dist/task-master.js dist/mcp-server.js", diff --git a/packages/tm-core/src/auth/auth-manager.test.ts b/packages/tm-core/src/auth/auth-manager.test.ts index 10ef85ef..54ec05ee 100644 --- a/packages/tm-core/src/auth/auth-manager.test.ts +++ b/packages/tm-core/src/auth/auth-manager.test.ts @@ -3,9 +3,8 @@ */ import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { AuthManager } from './auth-manager.js'; -// Mock the logger to verify warnings +// Mock the logger to verify warnings (must be hoisted before SUT import) const mockLogger = { warn: vi.fn(), info: vi.fn(), @@ -17,11 +16,66 @@ vi.mock('../logger/index.js', () => ({ getLogger: () => mockLogger })); +// Spy on CredentialStore constructor to verify config propagation +const CredentialStoreSpy = vi.fn(); +vi.mock('./credential-store.js', () => { + return { + CredentialStore: class { + constructor(config: any) { + CredentialStoreSpy(config); + this.getCredentials = vi.fn(() => null); + } + getCredentials() { + return null; + } + saveCredentials() {} + clearCredentials() {} + hasValidCredentials() { + return false; + } + } + }; +}); + +// Mock OAuthService to avoid side effects +vi.mock('./oauth-service.js', () => { + return { + OAuthService: class { + constructor() {} + authenticate() { + return Promise.resolve({}); + } + getAuthorizationUrl() { + return null; + } + } + }; +}); + +// Mock SupabaseAuthClient to avoid side effects +vi.mock('../clients/supabase-client.js', () => { + return { + SupabaseAuthClient: class { + constructor() {} + refreshSession() { + return Promise.resolve({}); + } + signOut() { + return Promise.resolve(); + } + } + }; +}); + +// Import SUT after mocks +import { AuthManager } from './auth-manager.js'; + describe('AuthManager Singleton', () => { beforeEach(() => { // Reset singleton before each test AuthManager.resetInstance(); vi.clearAllMocks(); + CredentialStoreSpy.mockClear(); }); it('should return the same instance on multiple calls', () => { @@ -41,11 +95,14 @@ describe('AuthManager Singleton', () => { const instance = AuthManager.getInstance(config); expect(instance).toBeDefined(); - // Verify the config is passed to internal components - // This would be observable when attempting operations that use the config - // For example, getCredentials would look in the configured file path + // Assert that CredentialStore was constructed with the provided config + expect(CredentialStoreSpy).toHaveBeenCalledTimes(1); + expect(CredentialStoreSpy).toHaveBeenCalledWith(config); + + // Verify the config is passed to internal components through observable behavior + // getCredentials would look in the configured file path const credentials = instance.getCredentials(); - expect(credentials).toBeNull(); // File doesn't exist, but it should check the right path + expect(credentials).toBeNull(); // File doesn't exist, but config was propagated correctly }); it('should warn when config is provided after initialization', () => { @@ -60,7 +117,7 @@ describe('AuthManager Singleton', () => { // Verify warning was logged expect(mockLogger.warn).toHaveBeenCalledWith( - 'getInstance called with config after initialization; config is ignored.' + expect.stringMatching(/config.*after initialization.*ignored/i) ); }); diff --git a/packages/tm-core/src/auth/auth-manager.ts b/packages/tm-core/src/auth/auth-manager.ts index 1d678906..35ede566 100644 --- a/packages/tm-core/src/auth/auth-manager.ts +++ b/packages/tm-core/src/auth/auth-manager.ts @@ -17,7 +17,7 @@ import { getLogger } from '../logger/index.js'; * Authentication manager class */ export class AuthManager { - private static instance: AuthManager; + private static instance: AuthManager | null = null; private credentialStore: CredentialStore; private oauthService: OAuthService; private supabaseClient: SupabaseAuthClient; @@ -41,14 +41,14 @@ export class AuthManager { 'getInstance called with config after initialization; config is ignored.' ); } - return AuthManager.instance!; + return AuthManager.instance; } /** * Reset the singleton instance (useful for testing) */ static resetInstance(): void { - AuthManager.instance = null as any; + AuthManager.instance = null; } /** @@ -120,7 +120,7 @@ export class AuthManager { await this.supabaseClient.signOut(); } catch (error) { // Log but don't throw - we still want to clear local credentials - console.warn('Failed to sign out from Supabase:', error); + getLogger('AuthManager').warn('Failed to sign out from Supabase:', error); } // Always clear local credentials (removes auth.json file) diff --git a/packages/tm-core/src/storage/storage-factory.ts b/packages/tm-core/src/storage/storage-factory.ts index 873ff0f6..9e445316 100644 --- a/packages/tm-core/src/storage/storage-factory.ts +++ b/packages/tm-core/src/storage/storage-factory.ts @@ -57,13 +57,17 @@ export class StorageFactory { case 'api': if (!StorageFactory.isHamsterAvailable(config)) { + const missing: string[] = []; + if (!config.storage?.apiEndpoint) missing.push('apiEndpoint'); + if (!config.storage?.apiAccessToken) missing.push('apiAccessToken'); + // Check if authenticated via AuthManager const authManager = AuthManager.getInstance(); if (!authManager.isAuthenticated()) { throw new TaskMasterError( - 'API storage configured but not authenticated. Run: tm auth login', + `API storage not fully configured (${missing.join(', ') || 'credentials missing'}). Run: tm auth login, or set the missing field(s).`, ERROR_CODES.MISSING_CONFIGURATION, - { storageType: 'api' } + { storageType: 'api', missing } ); } // Use auth token from AuthManager