diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d22b0d3f..709cd7663 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ - Added `toggle_connect_hardware_keyboard` tool to toggle the iOS Simulator hardware keyboard connection ([#346](https://github.com/getsentry/XcodeBuildMCP/issues/346)). - Fixed `xcode_tools_bridge_disconnect` immediately re-syncing proxied tools after a manual disconnect ([#343](https://github.com/getsentry/XcodeBuildMCP/issues/343)). - Stopped suggesting an unsupported `--device-id`/`deviceId` argument in the `device list` next-step hint for `device build`/`build_device`; device targeting flows through session defaults ([#350](https://github.com/getsentry/XcodeBuildMCP/pull/350) by [@MukundaKatta](https://github.com/MukundaKatta)). +- Added error handling around build-only tool execution paths (`build_device`, `build_sim`, `build_macos`) so unexpected execution throws are reported as failed build results instead of escaping the handler ([#334](https://github.com/getsentry/XcodeBuildMCP/issues/334)). ## [2.3.2] diff --git a/src/mcp/tools/device/__tests__/build_device.test.ts b/src/mcp/tools/device/__tests__/build_device.test.ts index 4e4d01757..1976cb8e3 100644 --- a/src/mcp/tools/device/__tests__/build_device.test.ts +++ b/src/mcp/tools/device/__tests__/build_device.test.ts @@ -1,10 +1,11 @@ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; import { computeScopedDerivedDataPath } from '../../../../utils/derived-data-path.ts'; import * as z from 'zod'; import { createMockExecutor } from '../../../../test-utils/mock-executors.ts'; import { expectPendingBuildResponse, runToolLogic } from '../../../../test-utils/test-helpers.ts'; import { schema, handler, buildDeviceLogic } from '../build_device.ts'; import { sessionStore } from '../../../../utils/session-store.ts'; +import * as buildUtils from '../../../../utils/build/index.ts'; import type { CommandExecutor } from '../../../../utils/execution/index.ts'; const runHandlerWithExecutor = handler as unknown as ( @@ -30,6 +31,7 @@ function createSpyExecutor(): { describe('build_device plugin', () => { beforeEach(() => { sessionStore.clear(); + vi.restoreAllMocks(); }); describe('Export Field Validation (Literal)', () => { @@ -309,6 +311,26 @@ describe('build_device plugin', () => { expectPendingBuildResponse(result); }); + it('should return error result when executeXcodeBuildCommand throws unexpectedly', async () => { + const executeSpy = vi + .spyOn(buildUtils, 'executeXcodeBuildCommand') + .mockRejectedValueOnce(new Error('Unexpected build error')); + + const { result } = await runToolLogic(() => + buildDeviceLogic( + { + projectPath: '/path/to/MyProject.xcodeproj', + scheme: 'MyScheme', + }, + createMockExecutor({ success: true, output: 'Build succeeded' }), + ), + ); + + expect(executeSpy).toHaveBeenCalledOnce(); + expect(result.isError()).toBe(true); + expectPendingBuildResponse(result); + }); + it('should include optional parameters in command', async () => { const spy = createSpyExecutor(); diff --git a/src/mcp/tools/device/build_device.ts b/src/mcp/tools/device/build_device.ts index f5787f61b..f6604d0c1 100644 --- a/src/mcp/tools/device/build_device.ts +++ b/src/mcp/tools/device/build_device.ts @@ -81,29 +81,43 @@ export function createBuildDeviceExecutor( }; const started = createDomainStreamingPipeline('build_device', 'BUILD', ctx, 'build-result'); - const buildResult = await executeXcodeBuildCommand( - processedParams, - { - platform, - logPrefix: `${platform} Device Build`, - }, - params.preferXcodebuild ?? false, - 'build', - executor, - undefined, - started.pipeline, - ); + try { + const buildResult = await executeXcodeBuildCommand( + processedParams, + { + platform, + logPrefix: `${platform} Device Build`, + }, + params.preferXcodebuild ?? false, + 'build', + executor, + undefined, + started.pipeline, + ); - return createBuildDomainResult({ - started, - succeeded: !buildResult.isError, - target: 'device', - artifacts: { - buildLogPath: started.pipeline.logPath, - }, - fallbackErrorMessages: collectFallbackErrorMessages(started, [], buildResult.content), - request: createBuildDeviceRequest(params), - }); + return createBuildDomainResult({ + started, + succeeded: !buildResult.isError, + target: 'device', + artifacts: { + buildLogPath: started.pipeline.logPath, + }, + fallbackErrorMessages: collectFallbackErrorMessages(started, [], buildResult.content), + request: createBuildDeviceRequest(params), + }); + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + return createBuildDomainResult({ + started, + succeeded: false, + target: 'device', + artifacts: { + buildLogPath: started.pipeline.logPath, + }, + fallbackErrorMessages: collectFallbackErrorMessages(started, [errorMessage]), + request: createBuildDeviceRequest(params), + }); + } }; } diff --git a/src/mcp/tools/macos/__tests__/build_macos.test.ts b/src/mcp/tools/macos/__tests__/build_macos.test.ts index 2b98adf40..a9a102588 100644 --- a/src/mcp/tools/macos/__tests__/build_macos.test.ts +++ b/src/mcp/tools/macos/__tests__/build_macos.test.ts @@ -1,10 +1,11 @@ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; import { computeScopedDerivedDataPath } from '../../../../utils/derived-data-path.ts'; import * as z from 'zod'; import { createMockExecutor } from '../../../../test-utils/mock-executors.ts'; import { expectPendingBuildResponse, runToolLogic } from '../../../../test-utils/test-helpers.ts'; import { sessionStore } from '../../../../utils/session-store.ts'; import { schema, handler, buildMacOSLogic } from '../build_macos.ts'; +import * as buildUtils from '../../../../utils/build/index.ts'; const runBuildMacOS = ( params: Parameters[0], @@ -29,6 +30,7 @@ function createSpyExecutor(): { describe('build_macos plugin', () => { beforeEach(() => { sessionStore.clear(); + vi.restoreAllMocks(); }); describe('Export Field Validation (Literal)', () => { @@ -150,6 +152,24 @@ describe('build_macos plugin', () => { }); }); + it('should return error result when executeXcodeBuildCommand throws unexpectedly', async () => { + const executeSpy = vi + .spyOn(buildUtils, 'executeXcodeBuildCommand') + .mockRejectedValueOnce(new Error('Unexpected macOS build error')); + + const { result } = await runBuildMacOS( + { + projectPath: '/path/to/MyProject.xcodeproj', + scheme: 'MyScheme', + }, + createMockExecutor({ success: true, output: 'BUILD SUCCEEDED' }), + ); + + expect(executeSpy).toHaveBeenCalledOnce(); + expect(result.isError()).toBe(true); + expectPendingBuildResponse(result); + }); + it('should return exact exception handling response', async () => { const mockExecutor = async () => { throw new Error('Network error'); diff --git a/src/mcp/tools/macos/build_macos.ts b/src/mcp/tools/macos/build_macos.ts index 7eeb86713..f543b6bf0 100644 --- a/src/mcp/tools/macos/build_macos.ts +++ b/src/mcp/tools/macos/build_macos.ts @@ -76,19 +76,34 @@ export function createBuildMacOSExecutor( return async (params, ctx) => { const configuration = params.configuration ?? 'Debug'; const started = createDomainStreamingPipeline('build_macos', 'BUILD', ctx, 'build-result'); - const buildResult = await executeXcodeBuildCommand( - { ...params, configuration }, - { - platform: XcodePlatform.macOS, - arch: params.arch, - logPrefix: 'macOS Build', - }, - params.preferXcodebuild ?? false, - 'build', - executor, - undefined, - started.pipeline, - ); + let buildResult; + try { + buildResult = await executeXcodeBuildCommand( + { ...params, configuration }, + { + platform: XcodePlatform.macOS, + arch: params.arch, + logPrefix: 'macOS Build', + }, + params.preferXcodebuild ?? false, + 'build', + executor, + undefined, + started.pipeline, + ); + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + return createBuildDomainResult({ + started, + succeeded: false, + target: 'macos', + artifacts: { + buildLogPath: started.pipeline.logPath, + }, + fallbackErrorMessages: collectFallbackErrorMessages(started, [errorMessage]), + request: createBuildMacOSRequest(params), + }); + } let bundleId: string | undefined; if (!buildResult.isError) { diff --git a/src/mcp/tools/simulator/__tests__/build_sim.test.ts b/src/mcp/tools/simulator/__tests__/build_sim.test.ts index 684011d1d..7cb95a287 100644 --- a/src/mcp/tools/simulator/__tests__/build_sim.test.ts +++ b/src/mcp/tools/simulator/__tests__/build_sim.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; import { computeScopedDerivedDataPath } from '../../../../utils/derived-data-path.ts'; import * as z from 'zod'; import { @@ -7,6 +7,7 @@ import { } from '../../../../test-utils/mock-executors.ts'; import { expectPendingBuildResponse, runToolLogic } from '../../../../test-utils/test-helpers.ts'; import { sessionStore } from '../../../../utils/session-store.ts'; +import * as buildUtils from '../../../../utils/build/index.ts'; import { schema, handler, build_simLogic } from '../build_sim.ts'; @@ -18,6 +19,7 @@ const runBuildSimLogic = ( describe('build_sim tool', () => { beforeEach(() => { sessionStore.clear(); + vi.restoreAllMocks(); }); describe('Export Field Validation (Literal)', () => { @@ -462,6 +464,25 @@ describe('build_sim tool', () => { expectPendingBuildResponse(result); }); + it('should return error result when executeXcodeBuildCommand throws unexpectedly', async () => { + const executeSpy = vi + .spyOn(buildUtils, 'executeXcodeBuildCommand') + .mockRejectedValueOnce(new Error('Unexpected simulator build error')); + + const { result } = await runBuildSimLogic( + { + workspacePath: '/path/to/workspace', + scheme: 'MyScheme', + simulatorName: 'iPhone 17', + }, + createMockExecutor({ success: true, output: 'BUILD SUCCEEDED' }), + ); + + expect(executeSpy).toHaveBeenCalledOnce(); + expect(result.isError()).toBe(true); + expectPendingBuildResponse(result); + }); + it('should handle build warnings', async () => { const mockExecutor = createMockExecutor({ success: true, diff --git a/src/mcp/tools/simulator/build_sim.ts b/src/mcp/tools/simulator/build_sim.ts index 5a6a02f04..9bb3854da 100644 --- a/src/mcp/tools/simulator/build_sim.ts +++ b/src/mcp/tools/simulator/build_sim.ts @@ -173,26 +173,41 @@ export function createBuildSimExecutor( } const started = createDomainStreamingPipeline('build_sim', 'BUILD', ctx, 'build-result'); - const buildResult = await executeXcodeBuildCommand( - resolved.sharedBuildParams, - resolved.platformOptions, - params.preferXcodebuild ?? false, - 'build', - executor, - undefined, - started.pipeline, - ); - - return createBuildDomainResult({ - started, - succeeded: !buildResult.isError, - target: 'simulator', - artifacts: { - buildLogPath: started.pipeline.logPath, - }, - fallbackErrorMessages: collectFallbackErrorMessages(started, [], buildResult.content), - request: resolved.invocationRequest, - }); + + try { + const buildResult = await executeXcodeBuildCommand( + resolved.sharedBuildParams, + resolved.platformOptions, + params.preferXcodebuild ?? false, + 'build', + executor, + undefined, + started.pipeline, + ); + + return createBuildDomainResult({ + started, + succeeded: !buildResult.isError, + target: 'simulator', + artifacts: { + buildLogPath: started.pipeline.logPath, + }, + fallbackErrorMessages: collectFallbackErrorMessages(started, [], buildResult.content), + request: resolved.invocationRequest, + }); + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + return createBuildDomainResult({ + started, + succeeded: false, + target: 'simulator', + artifacts: { + buildLogPath: started.pipeline.logPath, + }, + fallbackErrorMessages: collectFallbackErrorMessages(started, [errorMessage]), + request: resolved.invocationRequest, + }); + } }; }