From a1c5d0cb0d5ab091098b61855d48acaff782d7d6 Mon Sep 17 00:00:00 2001 From: Kacper Date: Tue, 23 Dec 2025 00:41:59 +0100 Subject: [PATCH] refactor: replace console.log with createLogger in services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Update feature-loader.ts to use logger.info instead of console.log - Update dev-server-service.ts to use createLogger for all logging - Update terminal-service.ts to use createLogger for all logging - Update agent-service.ts to use createLogger for all logging - Fix agent-service.test.ts mock to preserve actual createLogger 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- apps/server/src/services/agent-service.ts | 15 ++++---- .../server/src/services/dev-server-service.ts | 33 +++++++++-------- apps/server/src/services/feature-loader.ts | 6 ++-- apps/server/src/services/terminal-service.ts | 35 ++++++++++--------- .../tests/unit/services/agent-service.test.ts | 10 +++++- 5 files changed, 55 insertions(+), 44 deletions(-) diff --git a/apps/server/src/services/agent-service.ts b/apps/server/src/services/agent-service.ts index e13ec9bc..edbb219d 100644 --- a/apps/server/src/services/agent-service.ts +++ b/apps/server/src/services/agent-service.ts @@ -11,11 +11,14 @@ import { buildPromptWithImages, isAbortError, loadContextFiles, + createLogger, } from '@automaker/utils'; import { ProviderFactory } from '../providers/provider-factory.js'; import { createChatOptions, validateWorkingDirectory } from '../lib/sdk-options.js'; import { PathNotAllowedError, secureFs } from '@automaker/platform'; +const logger = createLogger('AgentService'); + interface Message { id: string; role: 'user' | 'assistant'; @@ -149,7 +152,7 @@ export class AgentService { filename: imageData.filename, }); } catch (error) { - console.error(`[AgentService] Failed to load image ${imagePath}:`, error); + logger.error(`Failed to load image ${imagePath}:`, error); } } } @@ -214,9 +217,7 @@ export class AgentService { // Get provider for this model const provider = ProviderFactory.getProviderForModel(effectiveModel); - console.log( - `[AgentService] Using provider "${provider.getName()}" for model "${effectiveModel}"` - ); + logger.info(`Using provider "${provider.getName()}" for model "${effectiveModel}"`); // Build options for provider const options: ExecuteOptions = { @@ -253,7 +254,7 @@ export class AgentService { // Capture SDK session ID from any message and persist it if (msg.session_id && !session.sdkSessionId) { session.sdkSessionId = msg.session_id; - console.log(`[AgentService] Captured SDK session ID: ${msg.session_id}`); + logger.info(`Captured SDK session ID: ${msg.session_id}`); // Persist the SDK session ID to ensure conversation continuity across server restarts await this.updateSession(sessionId, { sdkSessionId: msg.session_id }); } @@ -329,7 +330,7 @@ export class AgentService { return { success: false, aborted: true }; } - console.error('[AgentService] Error:', error); + logger.error('Error:', error); session.isRunning = false; session.abortController = null; @@ -423,7 +424,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); + logger.error('Failed to save session:', error); } } diff --git a/apps/server/src/services/dev-server-service.ts b/apps/server/src/services/dev-server-service.ts index f695203b..8f80a9dc 100644 --- a/apps/server/src/services/dev-server-service.ts +++ b/apps/server/src/services/dev-server-service.ts @@ -9,9 +9,12 @@ import { spawn, execSync, type ChildProcess } from 'child_process'; import { secureFs } from '@automaker/platform'; +import { createLogger } from '@automaker/utils'; import path from 'path'; import net from 'net'; +const logger = createLogger('DevServerService'); + export interface DevServerInfo { worktreePath: string; port: number; @@ -69,7 +72,7 @@ class DevServerService { for (const pid of pids) { try { execSync(`taskkill /F /PID ${pid}`, { stdio: 'ignore' }); - console.log(`[DevServerService] Killed process ${pid} on port ${port}`); + logger.info(`Killed process ${pid} on port ${port}`); } catch { // Process may have already exited } @@ -82,7 +85,7 @@ class DevServerService { for (const pid of pids) { try { execSync(`kill -9 ${pid}`, { stdio: 'ignore' }); - console.log(`[DevServerService] Killed process ${pid} on port ${port}`); + logger.info(`Killed process ${pid} on port ${port}`); } catch { // Process may have already exited } @@ -93,7 +96,7 @@ class DevServerService { } } catch (error) { // Ignore errors - port might not have any process - console.log(`[DevServerService] No process to kill on port ${port}`); + logger.info(`No process to kill on port ${port}`); } } @@ -251,11 +254,9 @@ class DevServerService { // Small delay to ensure related ports are freed await new Promise((resolve) => setTimeout(resolve, 100)); - console.log(`[DevServerService] Starting dev server on port ${port}`); - console.log(`[DevServerService] Working directory (cwd): ${worktreePath}`); - console.log( - `[DevServerService] Command: ${devCommand.cmd} ${devCommand.args.join(' ')} with PORT=${port}` - ); + logger.info(`Starting dev server on port ${port}`); + logger.info(`Working directory (cwd): ${worktreePath}`); + logger.info(`Command: ${devCommand.cmd} ${devCommand.args.join(' ')} with PORT=${port}`); // Spawn the dev process with PORT environment variable const env = { @@ -276,26 +277,26 @@ class DevServerService { // Log output for debugging if (devProcess.stdout) { devProcess.stdout.on('data', (data: Buffer) => { - console.log(`[DevServer:${port}] ${data.toString().trim()}`); + logger.info(`[DevServer:${port}] ${data.toString().trim()}`); }); } if (devProcess.stderr) { devProcess.stderr.on('data', (data: Buffer) => { const msg = data.toString().trim(); - console.error(`[DevServer:${port}] ${msg}`); + logger.error(`[DevServer:${port}] ${msg}`); }); } devProcess.on('error', (error) => { - console.error(`[DevServerService] Process error:`, error); + logger.error(`Process error:`, error); status.error = error.message; this.allocatedPorts.delete(port); this.runningServers.delete(worktreePath); }); devProcess.on('exit', (code) => { - console.log(`[DevServerService] Process for ${worktreePath} exited with code ${code}`); + logger.info(`Process for ${worktreePath} exited with code ${code}`); status.exited = true; this.allocatedPorts.delete(port); this.runningServers.delete(worktreePath); @@ -352,9 +353,7 @@ class DevServerService { // If we don't have a record of this server, it may have crashed/exited on its own // Return success so the frontend can clear its state if (!server) { - console.log( - `[DevServerService] No server record for ${worktreePath}, may have already stopped` - ); + logger.info(`No server record for ${worktreePath}, may have already stopped`); return { success: true, result: { @@ -364,7 +363,7 @@ class DevServerService { }; } - console.log(`[DevServerService] Stopping dev server for ${worktreePath}`); + logger.info(`Stopping dev server for ${worktreePath}`); // Kill the process if (server.process && !server.process.killed) { @@ -434,7 +433,7 @@ class DevServerService { * Stop all running dev servers (for cleanup) */ async stopAll(): Promise { - console.log(`[DevServerService] Stopping all ${this.runningServers.size} dev servers`); + logger.info(`Stopping all ${this.runningServers.size} dev servers`); for (const [worktreePath] of this.runningServers) { await this.stopDevServer(worktreePath); diff --git a/apps/server/src/services/feature-loader.ts b/apps/server/src/services/feature-loader.ts index 9967018a..21b61b01 100644 --- a/apps/server/src/services/feature-loader.ts +++ b/apps/server/src/services/feature-loader.ts @@ -57,7 +57,7 @@ export class FeatureLoader { try { // Paths are now absolute await secureFs.unlink(oldPath); - console.log(`[FeatureLoader] Deleted orphaned image: ${oldPath}`); + logger.info(`Deleted orphaned image: ${oldPath}`); } catch (error) { // Ignore errors when deleting (file may already be gone) logger.warn(`[FeatureLoader] Failed to delete image: ${oldPath}`, error); @@ -112,7 +112,7 @@ export class FeatureLoader { // Copy the file await secureFs.copyFile(fullOriginalPath, newPath); - console.log(`[FeatureLoader] Copied image: ${originalPath} -> ${newPath}`); + logger.info(`Copied image: ${originalPath} -> ${newPath}`); // Try to delete the original temp file try { @@ -333,7 +333,7 @@ export class FeatureLoader { try { const featureDir = this.getFeatureDir(projectPath, featureId); await secureFs.rm(featureDir, { recursive: true, force: true }); - console.log(`[FeatureLoader] Deleted feature ${featureId}`); + logger.info(`Deleted feature ${featureId}`); return true; } catch (error) { logger.error(`[FeatureLoader] Failed to delete feature ${featureId}:`, error); diff --git a/apps/server/src/services/terminal-service.ts b/apps/server/src/services/terminal-service.ts index 7d59633e..563803fc 100644 --- a/apps/server/src/services/terminal-service.ts +++ b/apps/server/src/services/terminal-service.ts @@ -10,6 +10,9 @@ import { EventEmitter } from 'events'; import * as os from 'os'; import * as fs from 'fs'; import * as path from 'path'; +import { createLogger } from '@automaker/utils'; + +const logger = createLogger('Terminal'); // Maximum scrollback buffer size (characters) const MAX_SCROLLBACK_SIZE = 50000; // ~50KB per terminal @@ -171,7 +174,7 @@ export class TerminalService extends EventEmitter { // Reject paths with null bytes (could bypass path checks) if (cwd.includes('\0')) { - console.warn(`[Terminal] Rejecting path with null byte: ${cwd.replace(/\0/g, '\\0')}`); + logger.warn(`Rejecting path with null byte: ${cwd.replace(/\0/g, '\\0')}`); return homeDir; } @@ -192,10 +195,10 @@ export class TerminalService extends EventEmitter { if (stat.isDirectory()) { return cwd; } - console.warn(`[Terminal] Path exists but is not a directory: ${cwd}, falling back to home`); + logger.warn(`Path exists but is not a directory: ${cwd}, falling back to home`); return homeDir; } catch { - console.warn(`[Terminal] Working directory does not exist: ${cwd}, falling back to home`); + logger.warn(`Working directory does not exist: ${cwd}, falling back to home`); return homeDir; } } @@ -220,7 +223,7 @@ export class TerminalService extends EventEmitter { setMaxSessions(limit: number): void { if (limit >= MIN_MAX_SESSIONS && limit <= MAX_MAX_SESSIONS) { maxSessions = limit; - console.log(`[Terminal] Max sessions limit updated to ${limit}`); + logger.info(`Max sessions limit updated to ${limit}`); } } @@ -231,7 +234,7 @@ export class TerminalService extends EventEmitter { createSession(options: TerminalOptions = {}): TerminalSession | null { // Check session limit if (this.sessions.size >= maxSessions) { - console.error(`[Terminal] Max sessions (${maxSessions}) reached, refusing new session`); + logger.error(`Max sessions (${maxSessions}) reached, refusing new session`); return null; } @@ -256,7 +259,7 @@ export class TerminalService extends EventEmitter { ...options.env, }; - console.log(`[Terminal] Creating session ${id} with shell: ${shell} in ${cwd}`); + logger.info(`Creating session ${id} with shell: ${shell} in ${cwd}`); const ptyProcess = pty.spawn(shell, shellArgs, { name: 'xterm-256color', @@ -328,13 +331,13 @@ export class TerminalService extends EventEmitter { // Handle exit ptyProcess.onExit(({ exitCode }) => { - console.log(`[Terminal] Session ${id} exited with code ${exitCode}`); + logger.info(`Session ${id} exited with code ${exitCode}`); this.sessions.delete(id); this.exitCallbacks.forEach((cb) => cb(id, exitCode)); this.emit('exit', id, exitCode); }); - console.log(`[Terminal] Session ${id} created successfully`); + logger.info(`Session ${id} created successfully`); return session; } @@ -344,7 +347,7 @@ export class TerminalService extends EventEmitter { write(sessionId: string, data: string): boolean { const session = this.sessions.get(sessionId); if (!session) { - console.warn(`[Terminal] Session ${sessionId} not found`); + logger.warn(`Session ${sessionId} not found`); return false; } session.pty.write(data); @@ -359,7 +362,7 @@ export class TerminalService extends EventEmitter { resize(sessionId: string, cols: number, rows: number, suppressOutput: boolean = true): boolean { const session = this.sessions.get(sessionId); if (!session) { - console.warn(`[Terminal] Session ${sessionId} not found for resize`); + logger.warn(`Session ${sessionId} not found for resize`); return false; } try { @@ -385,7 +388,7 @@ export class TerminalService extends EventEmitter { return true; } catch (error) { - console.error(`[Terminal] Error resizing session ${sessionId}:`, error); + logger.error(`Error resizing session ${sessionId}:`, error); session.resizeInProgress = false; // Clear flag on error return false; } @@ -413,14 +416,14 @@ export class TerminalService extends EventEmitter { } // First try graceful SIGTERM to allow process cleanup - console.log(`[Terminal] Session ${sessionId} sending SIGTERM`); + logger.info(`Session ${sessionId} sending SIGTERM`); session.pty.kill('SIGTERM'); // Schedule SIGKILL fallback if process doesn't exit gracefully // The onExit handler will remove session from map when it actually exits setTimeout(() => { if (this.sessions.has(sessionId)) { - console.log(`[Terminal] Session ${sessionId} still alive after SIGTERM, sending SIGKILL`); + logger.info(`Session ${sessionId} still alive after SIGTERM, sending SIGKILL`); try { session.pty.kill('SIGKILL'); } catch { @@ -431,10 +434,10 @@ export class TerminalService extends EventEmitter { } }, 1000); - console.log(`[Terminal] Session ${sessionId} kill initiated`); + logger.info(`Session ${sessionId} kill initiated`); return true; } catch (error) { - console.error(`[Terminal] Error killing session ${sessionId}:`, error); + logger.error(`Error killing session ${sessionId}:`, error); // Still try to remove from map even if kill fails this.sessions.delete(sessionId); return false; @@ -517,7 +520,7 @@ export class TerminalService extends EventEmitter { * Clean up all sessions */ cleanup(): void { - console.log(`[Terminal] Cleaning up ${this.sessions.size} sessions`); + logger.info(`Cleaning up ${this.sessions.size} sessions`); this.sessions.forEach((session, id) => { try { // Clean up flush timeout diff --git a/apps/server/tests/unit/services/agent-service.test.ts b/apps/server/tests/unit/services/agent-service.test.ts index ef2a5e0d..d78df8dc 100644 --- a/apps/server/tests/unit/services/agent-service.test.ts +++ b/apps/server/tests/unit/services/agent-service.test.ts @@ -9,7 +9,15 @@ import { collectAsyncGenerator } from '../../utils/helpers.js'; vi.mock('fs/promises'); vi.mock('@/providers/provider-factory.js'); -vi.mock('@automaker/utils'); +vi.mock('@automaker/utils', async () => { + const actual = await vi.importActual('@automaker/utils'); + return { + ...actual, + readImageAsBase64: vi.fn(), + buildPromptWithImages: vi.fn(), + loadContextFiles: vi.fn(), + }; +}); describe('agent-service.ts', () => { let service: AgentService;