From 504d9aa9d78f0d1b5bf6f1265fbb0e2f71765b78 Mon Sep 17 00:00:00 2001 From: Stephan Rieche Date: Tue, 30 Dec 2025 01:43:27 +0100 Subject: [PATCH] refactor: migrate AgentService to use centralized logger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace console.error calls with createLogger for consistent logging across the AgentService. This improves debuggability and makes logger calls testable. Changes: - Add createLogger import from @automaker/utils - Add private logger instance initialized with 'AgentService' prefix - Replace all 7 console.error calls with this.logger.error - Update test mocks to use vi.hoisted() for proper mock access - Update settings-helpers test to create mockLogger inside vi.mock() Test Impact: - All 774 tests passing - Logger error calls are now verifiable in tests - Mock logger properly accessible via vi.hoisted() pattern Resolves Gemini Code Assist suggestions: - "Make logger mockable for test assertions" - "Use logger instead of console.error in AgentService" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- apps/server/src/services/agent-service.ts | 16 +++++++++------- .../tests/unit/lib/settings-helpers.test.ts | 13 +++++++------ .../tests/unit/services/agent-service.test.ts | 17 ++++++++++------- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/apps/server/src/services/agent-service.ts b/apps/server/src/services/agent-service.ts index 77dbb98c..c507d81b 100644 --- a/apps/server/src/services/agent-service.ts +++ b/apps/server/src/services/agent-service.ts @@ -12,6 +12,7 @@ import { buildPromptWithImages, isAbortError, loadContextFiles, + createLogger, } from '@automaker/utils'; import { ProviderFactory } from '../providers/provider-factory.js'; import { createChatOptions, validateWorkingDirectory } from '../lib/sdk-options.js'; @@ -76,6 +77,7 @@ export class AgentService { private metadataFile: string; private events: EventEmitter; private settingsService: SettingsService | null = null; + private logger = createLogger('AgentService'); constructor(dataDir: string, events: EventEmitter, settingsService?: SettingsService) { this.stateDir = path.join(dataDir, 'agent-sessions'); @@ -149,12 +151,12 @@ export class AgentService { }) { const session = this.sessions.get(sessionId); if (!session) { - console.error('[AgentService] ERROR: Session not found:', sessionId); + this.logger.error('ERROR: Session not found:', sessionId); throw new Error(`Session ${sessionId} not found`); } if (session.isRunning) { - console.error('[AgentService] ERROR: Agent already running for session:', sessionId); + this.logger.error('ERROR: Agent already running for session:', sessionId); throw new Error('Agent is already processing a message'); } @@ -176,7 +178,7 @@ export class AgentService { filename: imageData.filename, }); } catch (error) { - console.error(`[AgentService] Failed to load image ${imagePath}:`, error); + this.logger.error(`Failed to load image ${imagePath}:`, error); } } } @@ -392,7 +394,7 @@ export class AgentService { return { success: false, aborted: true }; } - console.error('[AgentService] Error:', error); + this.logger.error('Error:', error); session.isRunning = false; session.abortController = null; @@ -486,7 +488,7 @@ export class AgentService { await secureFs.writeFile(sessionFile, JSON.stringify(messages, null, 2), 'utf-8'); await this.updateSessionTimestamp(sessionId); } catch (error) { - console.error('[AgentService] Failed to save session:', error); + this.logger.error('Failed to save session:', error); } } @@ -720,7 +722,7 @@ export class AgentService { try { await secureFs.writeFile(queueFile, JSON.stringify(queue, null, 2), 'utf-8'); } catch (error) { - console.error('[AgentService] Failed to save queue state:', error); + this.logger.error('Failed to save queue state:', error); } } @@ -769,7 +771,7 @@ export class AgentService { model: nextPrompt.model, }); } catch (error) { - console.error('[AgentService] Failed to process queued prompt:', error); + this.logger.error('Failed to process queued prompt:', error); this.emitAgentEvent(sessionId, { type: 'queue_error', error: (error as Error).message, diff --git a/apps/server/tests/unit/lib/settings-helpers.test.ts b/apps/server/tests/unit/lib/settings-helpers.test.ts index aa778e26..8af48580 100644 --- a/apps/server/tests/unit/lib/settings-helpers.test.ts +++ b/apps/server/tests/unit/lib/settings-helpers.test.ts @@ -5,14 +5,15 @@ import type { SettingsService } from '@/services/settings-service.js'; // Mock the logger vi.mock('@automaker/utils', async () => { const actual = await vi.importActual('@automaker/utils'); + const mockLogger = { + info: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + debug: vi.fn(), + }; return { ...actual, - createLogger: () => ({ - info: vi.fn(), - error: vi.fn(), - warn: vi.fn(), - debug: vi.fn(), - }), + createLogger: () => mockLogger, }; }); diff --git a/apps/server/tests/unit/services/agent-service.test.ts b/apps/server/tests/unit/services/agent-service.test.ts index 302dcfa1..8405c918 100644 --- a/apps/server/tests/unit/services/agent-service.test.ts +++ b/apps/server/tests/unit/services/agent-service.test.ts @@ -7,6 +7,14 @@ import * as promptBuilder from '@automaker/utils'; import * as contextLoader from '@automaker/utils'; import { collectAsyncGenerator } from '../../utils/helpers.js'; +// Create a shared mock logger instance for assertions using vi.hoisted +const mockLogger = vi.hoisted(() => ({ + info: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + debug: vi.fn(), +})); + vi.mock('fs/promises'); vi.mock('@/providers/provider-factory.js'); vi.mock('@automaker/utils', async () => { @@ -16,12 +24,7 @@ vi.mock('@automaker/utils', async () => { loadContextFiles: vi.fn(), buildPromptWithImages: vi.fn(), readImageAsBase64: vi.fn(), - createLogger: vi.fn(() => ({ - info: vi.fn(), - error: vi.fn(), - warn: vi.fn(), - debug: vi.fn(), - })), + createLogger: vi.fn(() => mockLogger), }; }); @@ -244,7 +247,7 @@ describe('agent-service.ts', () => { imagePaths: ['/path/test.png'], }); - // Logger will be called with error, but we don't need to assert it + expect(mockLogger.error).toHaveBeenCalled(); }); it('should use custom model if provided', async () => {