diff --git a/src/test-utils/mock-executors.ts b/src/test-utils/mock-executors.ts index f74e7777..bbe20112 100644 --- a/src/test-utils/mock-executors.ts +++ b/src/test-utils/mock-executors.ts @@ -31,6 +31,8 @@ export function createMockExecutor( output?: string; error?: string; process?: unknown; + exitCode?: number; + shouldThrow?: Error; } | Error | string, @@ -42,6 +44,13 @@ export function createMockExecutor( }; } + // If shouldThrow is specified, return executor that rejects with that error + if (result.shouldThrow) { + return async () => { + throw result.shouldThrow; + }; + } + const mockProcess = { pid: 12345, stdout: null, @@ -50,7 +59,7 @@ export function createMockExecutor( stdio: [null, null, null], killed: false, connected: false, - exitCode: result.success === false ? 1 : 0, + exitCode: result.exitCode ?? (result.success === false ? 1 : 0), signalCode: null, spawnargs: [], spawnfile: 'sh', @@ -61,6 +70,7 @@ export function createMockExecutor( output: result.output ?? '', error: result.error, process: (result.process ?? mockProcess) as ChildProcess, + exitCode: result.exitCode ?? (result.success === false ? 1 : 0), }); } @@ -104,6 +114,7 @@ export function createCommandMatchingMockExecutor( output?: string; error?: string; process?: unknown; + exitCode?: number; } >, ): CommandExecutor { @@ -132,7 +143,7 @@ export function createCommandMatchingMockExecutor( stdio: [null, null, null], killed: false, connected: false, - exitCode: result.success === false ? 1 : 0, + exitCode: result.exitCode ?? (result.success === false ? 1 : 0), signalCode: null, spawnargs: [], spawnfile: 'sh', @@ -143,6 +154,7 @@ export function createCommandMatchingMockExecutor( output: result.output ?? '', error: result.error, process: (result.process ?? mockProcess) as ChildProcess, + exitCode: result.exitCode ?? (result.success === false ? 1 : 0), }; }; } diff --git a/src/utils/CommandExecutor.ts b/src/utils/CommandExecutor.ts index ff85881b..f06ac19f 100644 --- a/src/utils/CommandExecutor.ts +++ b/src/utils/CommandExecutor.ts @@ -19,4 +19,5 @@ export interface CommandResponse { output: string; error?: string; process: ChildProcess; + exitCode?: number; } diff --git a/src/utils/__tests__/build-utils.test.ts b/src/utils/__tests__/build-utils.test.ts new file mode 100644 index 00000000..9364e76a --- /dev/null +++ b/src/utils/__tests__/build-utils.test.ts @@ -0,0 +1,263 @@ +/** + * Tests for build-utils Sentry classification logic + */ + +import { describe, it, expect } from 'vitest'; +import { createMockExecutor } from '../../test-utils/mock-executors.ts'; +import { executeXcodeBuildCommand } from '../build-utils.ts'; +import { XcodePlatform } from '../xcode.ts'; + +describe('build-utils Sentry Classification', () => { + const mockPlatformOptions = { + platform: XcodePlatform.macOS, + logPrefix: 'Test Build', + }; + + const mockParams = { + scheme: 'TestScheme', + configuration: 'Debug', + projectPath: '/path/to/project.xcodeproj', + }; + + describe('Exit Code 64 Classification (MCP Error)', () => { + it('should trigger Sentry logging for exit code 64 (invalid arguments)', async () => { + const mockExecutor = createMockExecutor({ + success: false, + error: 'xcodebuild: error: invalid option', + exitCode: 64, + }); + + const result = await executeXcodeBuildCommand( + mockParams, + mockPlatformOptions, + false, + 'build', + mockExecutor, + ); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('❌ [stderr] xcodebuild: error: invalid option'); + expect(result.content[1].text).toContain('❌ Test Build build failed for scheme TestScheme'); + }); + }); + + describe('Other Exit Codes Classification (User Error)', () => { + it('should not trigger Sentry logging for exit code 65 (user error)', async () => { + const mockExecutor = createMockExecutor({ + success: false, + error: 'Scheme TestScheme was not found', + exitCode: 65, + }); + + const result = await executeXcodeBuildCommand( + mockParams, + mockPlatformOptions, + false, + 'build', + mockExecutor, + ); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('❌ [stderr] Scheme TestScheme was not found'); + expect(result.content[1].text).toContain('❌ Test Build build failed for scheme TestScheme'); + }); + + it('should not trigger Sentry logging for exit code 66 (file not found)', async () => { + const mockExecutor = createMockExecutor({ + success: false, + error: 'project.xcodeproj cannot be opened', + exitCode: 66, + }); + + const result = await executeXcodeBuildCommand( + mockParams, + mockPlatformOptions, + false, + 'build', + mockExecutor, + ); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('❌ [stderr] project.xcodeproj cannot be opened'); + }); + + it('should not trigger Sentry logging for exit code 70 (destination error)', async () => { + const mockExecutor = createMockExecutor({ + success: false, + error: 'Unable to find a destination matching the provided destination specifier', + exitCode: 70, + }); + + const result = await executeXcodeBuildCommand( + mockParams, + mockPlatformOptions, + false, + 'build', + mockExecutor, + ); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('❌ [stderr] Unable to find a destination matching'); + }); + + it('should not trigger Sentry logging for exit code 1 (general build failure)', async () => { + const mockExecutor = createMockExecutor({ + success: false, + error: 'Build failed with errors', + exitCode: 1, + }); + + const result = await executeXcodeBuildCommand( + mockParams, + mockPlatformOptions, + false, + 'build', + mockExecutor, + ); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('❌ [stderr] Build failed with errors'); + }); + }); + + describe('Spawn Error Classification (Environment Error)', () => { + it('should not trigger Sentry logging for ENOENT spawn error', async () => { + const spawnError = new Error('spawn xcodebuild ENOENT') as NodeJS.ErrnoException; + spawnError.code = 'ENOENT'; + + const mockExecutor = createMockExecutor({ + success: false, + error: '', + shouldThrow: spawnError, + }); + + const result = await executeXcodeBuildCommand( + mockParams, + mockPlatformOptions, + false, + 'build', + mockExecutor, + ); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain( + 'Error during Test Build build: spawn xcodebuild ENOENT', + ); + }); + + it('should not trigger Sentry logging for EACCES spawn error', async () => { + const spawnError = new Error('spawn xcodebuild EACCES') as NodeJS.ErrnoException; + spawnError.code = 'EACCES'; + + const mockExecutor = createMockExecutor({ + success: false, + error: '', + shouldThrow: spawnError, + }); + + const result = await executeXcodeBuildCommand( + mockParams, + mockPlatformOptions, + false, + 'build', + mockExecutor, + ); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain( + 'Error during Test Build build: spawn xcodebuild EACCES', + ); + }); + + it('should not trigger Sentry logging for EPERM spawn error', async () => { + const spawnError = new Error('spawn xcodebuild EPERM') as NodeJS.ErrnoException; + spawnError.code = 'EPERM'; + + const mockExecutor = createMockExecutor({ + success: false, + error: '', + shouldThrow: spawnError, + }); + + const result = await executeXcodeBuildCommand( + mockParams, + mockPlatformOptions, + false, + 'build', + mockExecutor, + ); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain( + 'Error during Test Build build: spawn xcodebuild EPERM', + ); + }); + + it('should trigger Sentry logging for non-spawn exceptions', async () => { + const otherError = new Error('Unexpected internal error'); + + const mockExecutor = createMockExecutor({ + success: false, + error: '', + shouldThrow: otherError, + }); + + const result = await executeXcodeBuildCommand( + mockParams, + mockPlatformOptions, + false, + 'build', + mockExecutor, + ); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain( + 'Error during Test Build build: Unexpected internal error', + ); + }); + }); + + describe('Success Case (No Sentry Logging)', () => { + it('should not trigger any error logging for successful builds', async () => { + const mockExecutor = createMockExecutor({ + success: true, + output: 'BUILD SUCCEEDED', + exitCode: 0, + }); + + const result = await executeXcodeBuildCommand( + mockParams, + mockPlatformOptions, + false, + 'build', + mockExecutor, + ); + + expect(result.isError).toBeFalsy(); + expect(result.content[0].text).toContain( + '✅ Test Build build succeeded for scheme TestScheme', + ); + }); + }); + + describe('Exit Code Undefined Cases', () => { + it('should not trigger Sentry logging when exitCode is undefined', async () => { + const mockExecutor = createMockExecutor({ + success: false, + error: 'Some error without exit code', + exitCode: undefined, + }); + + const result = await executeXcodeBuildCommand( + mockParams, + mockPlatformOptions, + false, + 'build', + mockExecutor, + ); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('❌ [stderr] Some error without exit code'); + }); + }); +}); diff --git a/src/utils/build-utils.ts b/src/utils/build-utils.ts index 545923cf..be6aa317 100644 --- a/src/utils/build-utils.ts +++ b/src/utils/build-utils.ts @@ -247,9 +247,13 @@ export async function executeXcodeBuildCommand( } if (!result.success) { - log('error', `${platformOptions.logPrefix} ${buildAction} failed: ${result.error}`); + const isMcpError = result.exitCode === 64; - // Create concise error response with warnings/errors included + log( + isMcpError ? 'error' : 'warning', + `${platformOptions.logPrefix} ${buildAction} failed: ${result.error}`, + { sentry: isMcpError }, + ); const errorResponse = createTextResponse( `❌ ${platformOptions.logPrefix} ${buildAction} failed for scheme ${params.scheme}.`, true, @@ -339,7 +343,16 @@ Future builds will use the generated Makefile for improved performance. return consolidateContentForClaudeCode(successResponse); } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); - log('error', `Error during ${platformOptions.logPrefix} ${buildAction}: ${errorMessage}`); + + const isSpawnError = + error instanceof Error && + 'code' in error && + ['ENOENT', 'EACCES', 'EPERM'].includes((error as NodeJS.ErrnoException).code ?? ''); + + log('error', `Error during ${platformOptions.logPrefix} ${buildAction}: ${errorMessage}`, { + sentry: !isSpawnError, + }); + return consolidateContentForClaudeCode( createTextResponse( `Error during ${platformOptions.logPrefix} ${buildAction}: ${errorMessage}`, diff --git a/src/utils/command.ts b/src/utils/command.ts index 05ecf63e..ce73a33b 100644 --- a/src/utils/command.ts +++ b/src/utils/command.ts @@ -126,6 +126,7 @@ async function defaultExecutor( output: stdout, error: success ? undefined : stderr, process: childProcess, + exitCode: code ?? undefined, }; resolve(response); diff --git a/src/utils/logger.ts b/src/utils/logger.ts index 58cf6185..14dcc905 100644 --- a/src/utils/logger.ts +++ b/src/utils/logger.ts @@ -37,6 +37,13 @@ const LOG_LEVELS = { export type LogLevel = keyof typeof LOG_LEVELS; +/** + * Optional context for logging to control Sentry capture + */ +export interface LogContext { + sentry?: boolean; +} + // Client-requested log level (null means no filtering) let clientLogLevel: LogLevel | null = null; @@ -130,8 +137,9 @@ function shouldLog(level: string): boolean { * Log a message with the specified level * @param level The log level (emergency, alert, critical, error, warning, notice, info, debug) * @param message The message to log + * @param context Optional context to control Sentry capture and other behavior */ -export function log(level: string, message: string): void { +export function log(level: string, message: string, context?: LogContext): void { // Check if we should log this level if (!shouldLog(level)) { return; @@ -140,7 +148,11 @@ export function log(level: string, message: string): void { const timestamp = new Date().toISOString(); const logMessage = `[${timestamp}] [${level.toUpperCase()}] ${message}`; - if (level === 'error' && SENTRY_ENABLED) { + // Default: error level goes to Sentry + // But respect explicit override from context + const captureToSentry = SENTRY_ENABLED && (context?.sentry ?? level === 'error'); + + if (captureToSentry) { withSentry((s) => s.captureMessage(logMessage)); }