Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
24 changes: 23 additions & 1 deletion src/mcp/tools/device/__tests__/build_device.test.ts
Original file line number Diff line number Diff line change
@@ -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 (
Expand All @@ -30,6 +31,7 @@
describe('build_device plugin', () => {
beforeEach(() => {
sessionStore.clear();
vi.restoreAllMocks();
});

describe('Export Field Validation (Literal)', () => {
Expand Down Expand Up @@ -309,6 +311,26 @@
expectPendingBuildResponse(result);
});

it('should return error result when executeXcodeBuildCommand throws unexpectedly', async () => {
const executeSpy = vi
.spyOn(buildUtils, 'executeXcodeBuildCommand')

Check warning on line 316 in src/mcp/tools/device/__tests__/build_device.test.ts

View check run for this annotation

@sentry/warden / warden: xcodebuildmcp-test-boundary-review

Test uses vi.spyOn on imported module instead of injected dependency

This new test uses `vi.spyOn(buildUtils, 'executeXcodeBuildCommand')` to force a throw, rather than injecting a mock dependency through the executor parameter. The skill's guardrail requires unit tests to inject command/filesystem/external dependencies and prefer using existing mock executor helpers. Module-level spying couples the test to internal import structure and bypasses the dependency injection contract used elsewhere in this file (e.g., `createMockExecutor`, `createSpyExecutor`).

Check warning on line 316 in src/mcp/tools/device/__tests__/build_device.test.ts

View workflow job for this annotation

GitHub Actions / warden: xcodebuildmcp-test-boundary-review

Test uses vi.spyOn on imported module instead of injected dependency

This new test relies on `vi.spyOn(buildUtils, 'executeXcodeBuildCommand')` to simulate an unexpected throw, rather than injecting a fake executor or dependency through the function signature. The skill's guardrails state that unit tests should inject command/filesystem/external dependencies and prefer testing logic via injected dependencies. Spying on a module-level import couples the test to the module's internal structure and bypasses the dependency-injection pattern used elsewhere in this file (which threads `mockExecutor` into `buildDeviceLogic`). Consider exposing `executeXcodeBuildCommand` as an injectable parameter so the throw can be simulated without module spying.
Comment on lines +314 to +316

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test uses vi.spyOn on imported module instead of injected dependency

This new test uses vi.spyOn(buildUtils, 'executeXcodeBuildCommand') to force a throw, rather than injecting a mock dependency through the executor parameter. The skill's guardrail requires unit tests to inject command/filesystem/external dependencies and prefer using existing mock executor helpers. Module-level spying couples the test to internal import structure and bypasses the dependency injection contract used elsewhere in this file (e.g., createMockExecutor, createSpyExecutor).

Verification

Reviewed the hunk and surrounding tests in the same describe block, which consistently inject createMockExecutor/createSpyExecutor per the skill's mock-executor helper guidance. The new test instead patches the buildUtils module export, which conflicts with the 'inject command/filesystem/external dependencies' and 'use existing mock executor helpers' guardrails. Could not verify whether buildDeviceLogic exposes a seam to inject executeXcodeBuildCommand directly without reading the source, so confidence is medium.

Identified by Warden xcodebuildmcp-test-boundary-review · DZ7-24F

.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();

Expand Down
58 changes: 36 additions & 22 deletions src/mcp/tools/device/build_device.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
});
}
};
}

Expand Down
22 changes: 21 additions & 1 deletion src/mcp/tools/macos/__tests__/build_macos.test.ts
Original file line number Diff line number Diff line change
@@ -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<typeof buildMacOSLogic>[0],
Expand All @@ -29,6 +30,7 @@ function createSpyExecutor(): {
describe('build_macos plugin', () => {
beforeEach(() => {
sessionStore.clear();
vi.restoreAllMocks();
});

describe('Export Field Validation (Literal)', () => {
Expand Down Expand Up @@ -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');
Expand Down
41 changes: 28 additions & 13 deletions src/mcp/tools/macos/build_macos.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
23 changes: 22 additions & 1 deletion src/mcp/tools/simulator/__tests__/build_sim.test.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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';

Expand All @@ -18,6 +19,7 @@ const runBuildSimLogic = (
describe('build_sim tool', () => {
beforeEach(() => {
sessionStore.clear();
vi.restoreAllMocks();
});

describe('Export Field Validation (Literal)', () => {
Expand Down Expand Up @@ -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,
Expand Down
55 changes: 35 additions & 20 deletions src/mcp/tools/simulator/build_sim.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
};
}

Expand Down
Loading