refactor: migrate AgentService to use centralized logger

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 <noreply@anthropic.com>
This commit is contained in:
Stephan Rieche
2025-12-30 01:43:27 +01:00
parent d5aea8355b
commit 504d9aa9d7
3 changed files with 26 additions and 20 deletions

View File

@@ -12,6 +12,7 @@ import {
buildPromptWithImages, buildPromptWithImages,
isAbortError, isAbortError,
loadContextFiles, loadContextFiles,
createLogger,
} from '@automaker/utils'; } from '@automaker/utils';
import { ProviderFactory } from '../providers/provider-factory.js'; import { ProviderFactory } from '../providers/provider-factory.js';
import { createChatOptions, validateWorkingDirectory } from '../lib/sdk-options.js'; import { createChatOptions, validateWorkingDirectory } from '../lib/sdk-options.js';
@@ -76,6 +77,7 @@ export class AgentService {
private metadataFile: string; private metadataFile: string;
private events: EventEmitter; private events: EventEmitter;
private settingsService: SettingsService | null = null; private settingsService: SettingsService | null = null;
private logger = createLogger('AgentService');
constructor(dataDir: string, events: EventEmitter, settingsService?: SettingsService) { constructor(dataDir: string, events: EventEmitter, settingsService?: SettingsService) {
this.stateDir = path.join(dataDir, 'agent-sessions'); this.stateDir = path.join(dataDir, 'agent-sessions');
@@ -149,12 +151,12 @@ export class AgentService {
}) { }) {
const session = this.sessions.get(sessionId); const session = this.sessions.get(sessionId);
if (!session) { 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`); throw new Error(`Session ${sessionId} not found`);
} }
if (session.isRunning) { 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'); throw new Error('Agent is already processing a message');
} }
@@ -176,7 +178,7 @@ export class AgentService {
filename: imageData.filename, filename: imageData.filename,
}); });
} catch (error) { } 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 }; return { success: false, aborted: true };
} }
console.error('[AgentService] Error:', error); this.logger.error('Error:', error);
session.isRunning = false; session.isRunning = false;
session.abortController = null; session.abortController = null;
@@ -486,7 +488,7 @@ export class AgentService {
await secureFs.writeFile(sessionFile, JSON.stringify(messages, null, 2), 'utf-8'); await secureFs.writeFile(sessionFile, JSON.stringify(messages, null, 2), 'utf-8');
await this.updateSessionTimestamp(sessionId); await this.updateSessionTimestamp(sessionId);
} catch (error) { } 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 { try {
await secureFs.writeFile(queueFile, JSON.stringify(queue, null, 2), 'utf-8'); await secureFs.writeFile(queueFile, JSON.stringify(queue, null, 2), 'utf-8');
} catch (error) { } 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, model: nextPrompt.model,
}); });
} catch (error) { } catch (error) {
console.error('[AgentService] Failed to process queued prompt:', error); this.logger.error('Failed to process queued prompt:', error);
this.emitAgentEvent(sessionId, { this.emitAgentEvent(sessionId, {
type: 'queue_error', type: 'queue_error',
error: (error as Error).message, error: (error as Error).message,

View File

@@ -5,14 +5,15 @@ import type { SettingsService } from '@/services/settings-service.js';
// Mock the logger // Mock the logger
vi.mock('@automaker/utils', async () => { vi.mock('@automaker/utils', async () => {
const actual = await vi.importActual('@automaker/utils'); const actual = await vi.importActual('@automaker/utils');
const mockLogger = {
info: vi.fn(),
error: vi.fn(),
warn: vi.fn(),
debug: vi.fn(),
};
return { return {
...actual, ...actual,
createLogger: () => ({ createLogger: () => mockLogger,
info: vi.fn(),
error: vi.fn(),
warn: vi.fn(),
debug: vi.fn(),
}),
}; };
}); });

View File

@@ -7,6 +7,14 @@ import * as promptBuilder from '@automaker/utils';
import * as contextLoader from '@automaker/utils'; import * as contextLoader from '@automaker/utils';
import { collectAsyncGenerator } from '../../utils/helpers.js'; 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('fs/promises');
vi.mock('@/providers/provider-factory.js'); vi.mock('@/providers/provider-factory.js');
vi.mock('@automaker/utils', async () => { vi.mock('@automaker/utils', async () => {
@@ -16,12 +24,7 @@ vi.mock('@automaker/utils', async () => {
loadContextFiles: vi.fn(), loadContextFiles: vi.fn(),
buildPromptWithImages: vi.fn(), buildPromptWithImages: vi.fn(),
readImageAsBase64: vi.fn(), readImageAsBase64: vi.fn(),
createLogger: vi.fn(() => ({ createLogger: vi.fn(() => mockLogger),
info: vi.fn(),
error: vi.fn(),
warn: vi.fn(),
debug: vi.fn(),
})),
}; };
}); });
@@ -244,7 +247,7 @@ describe('agent-service.ts', () => {
imagePaths: ['/path/test.png'], 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 () => { it('should use custom model if provided', async () => {