diff --git a/src/mcp/tools/device/test_device.ts b/src/mcp/tools/device/test_device.ts index 1e78e62c..81634454 100644 --- a/src/mcp/tools/device/test_device.ts +++ b/src/mcp/tools/device/test_device.ts @@ -26,6 +26,7 @@ import { getSessionAwareToolSchemaShape, } from '../../../utils/typed-tool-factory.ts'; import { nullifyEmptyStrings } from '../../../utils/schema-helpers.ts'; +import { validateXcodebuildExtraArgs } from '../../../utils/xcodebuild-validation.ts'; // Unified schema: XOR between projectPath and workspacePath const baseSchemaObject = z.object({ @@ -183,6 +184,13 @@ export async function testDeviceLogic( executor: CommandExecutor = getDefaultCommandExecutor(), fileSystemExecutor: FileSystemExecutor = getDefaultFileSystemExecutor(), ): Promise { + // Validate extraArgs for invalid xcodebuild options + const validation = validateXcodebuildExtraArgs(params.extraArgs); + if (!validation.isValid) { + log('error', `Invalid extraArgs: ${validation.error}`); + return createTextResponse(validation.error!, true); + } + log( 'info', `Starting test run for scheme ${params.scheme} on platform ${params.platform ?? 'iOS'} (internal)`, diff --git a/src/mcp/tools/macos/test_macos.ts b/src/mcp/tools/macos/test_macos.ts index 238aacea..5cbe27d7 100644 --- a/src/mcp/tools/macos/test_macos.ts +++ b/src/mcp/tools/macos/test_macos.ts @@ -26,6 +26,7 @@ import { getSessionAwareToolSchemaShape, } from '../../../utils/typed-tool-factory.ts'; import { nullifyEmptyStrings } from '../../../utils/schema-helpers.ts'; +import { validateXcodebuildExtraArgs } from '../../../utils/xcodebuild-validation.ts'; // Unified schema: XOR between projectPath and workspacePath const baseSchemaObject = z.object({ @@ -239,6 +240,13 @@ export async function testMacosLogic( executor: CommandExecutor = getDefaultCommandExecutor(), fileSystemExecutor: FileSystemExecutor = getDefaultFileSystemExecutor(), ): Promise { + // Validate extraArgs for invalid xcodebuild options + const validation = validateXcodebuildExtraArgs(params.extraArgs); + if (!validation.isValid) { + log('error', `Invalid extraArgs: ${validation.error}`); + return createTextResponse(validation.error!, true); + } + log('info', `Starting test run for scheme ${params.scheme} on platform macOS (internal)`); try { diff --git a/src/mcp/tools/simulator/__tests__/test_sim.test.ts b/src/mcp/tools/simulator/__tests__/test_sim.test.ts index 69bf3ab8..7880fad0 100644 --- a/src/mcp/tools/simulator/__tests__/test_sim.test.ts +++ b/src/mcp/tools/simulator/__tests__/test_sim.test.ts @@ -6,7 +6,8 @@ import { describe, it, expect, beforeEach } from 'vitest'; import * as z from 'zod'; import { sessionStore } from '../../../../utils/session-store.ts'; -import testSim from '../test_sim.ts'; +import testSim, { test_simLogic } from '../test_sim.ts'; +import { createMockExecutor } from '../../../../test-utils/mock-executors.ts'; describe('test_sim tool', () => { beforeEach(() => { @@ -97,4 +98,47 @@ describe('test_sim tool', () => { expect(result.content[0].text).toContain('simulatorName'); }); }); + + describe('Validation', () => { + it('should reject invalid xcodebuild options in extraArgs', async () => { + const mockExecutor = createMockExecutor({ success: true, output: 'mock output' }); + + const result = await test_simLogic( + { + projectPath: '/path/to/project.xcodeproj', + scheme: 'MyScheme', + simulatorName: 'iPhone 16', + configuration: 'Debug', + extraArgs: ['-test-arg', '--snapshot-record'], + }, + mockExecutor, + ); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('-test-arg'); + expect(result.content[0].text).toContain('not a recognized xcodebuild argument'); + expect(result.content[0].text).toContain('testRunnerEnv'); + }); + + it('should accept valid extraArgs', async () => { + const mockExecutor = createMockExecutor({ success: true, output: 'mock output' }); + + const result = await test_simLogic( + { + projectPath: '/path/to/project.xcodeproj', + scheme: 'MyScheme', + simulatorName: 'iPhone 16', + configuration: 'Debug', + extraArgs: ['-only-testing:MyTests/MyTestClass'], + }, + mockExecutor, + ); + + // Should not fail due to validation (but might fail for other reasons in mock) + // The key is that it doesn't fail with the validation error message + if (result.isError) { + expect(result.content[0].text).not.toContain('not a recognized xcodebuild argument'); + } + }); + }); }); diff --git a/src/mcp/tools/simulator/test_sim.ts b/src/mcp/tools/simulator/test_sim.ts index 66d40a3d..003f0557 100644 --- a/src/mcp/tools/simulator/test_sim.ts +++ b/src/mcp/tools/simulator/test_sim.ts @@ -18,6 +18,8 @@ import { createSessionAwareTool, getSessionAwareToolSchemaShape, } from '../../../utils/typed-tool-factory.ts'; +import { validateXcodebuildExtraArgs } from '../../../utils/xcodebuild-validation.ts'; +import { createTextResponse } from '../../../utils/validation.ts'; // Define base schema object with all fields const baseSchemaObject = z.object({ @@ -91,6 +93,13 @@ export async function test_simLogic( params: TestSimulatorParams, executor: CommandExecutor, ): Promise { + // Validate extraArgs for invalid xcodebuild options + const validation = validateXcodebuildExtraArgs(params.extraArgs); + if (!validation.isValid) { + log('error', `Invalid extraArgs: ${validation.error}`); + return createTextResponse(validation.error!, true); + } + // Log warning if useLatestOS is provided with simulatorId if (params.simulatorId && params.useLatestOS !== undefined) { log( diff --git a/src/utils/__tests__/xcodebuild-validation.test.ts b/src/utils/__tests__/xcodebuild-validation.test.ts new file mode 100644 index 00000000..977b6d65 --- /dev/null +++ b/src/utils/__tests__/xcodebuild-validation.test.ts @@ -0,0 +1,119 @@ +/** + * Tests for xcodebuild validation utilities + */ + +import { describe, it, expect } from 'vitest'; +import { validateXcodebuildExtraArgs } from '../xcodebuild-validation.ts'; + +describe('validateXcodebuildExtraArgs', () => { + describe('valid extraArgs', () => { + it('should return valid for undefined extraArgs', () => { + const result = validateXcodebuildExtraArgs(undefined); + expect(result.isValid).toBe(true); + expect(result.error).toBeUndefined(); + }); + + it('should return valid for empty extraArgs array', () => { + const result = validateXcodebuildExtraArgs([]); + expect(result.isValid).toBe(true); + expect(result.error).toBeUndefined(); + }); + + it('should return valid for legitimate xcodebuild options', () => { + const result = validateXcodebuildExtraArgs([ + '-only-testing:MyTests/MyTestClass', + '-verbose', + '-dry-run', + ]); + expect(result.isValid).toBe(true); + expect(result.error).toBeUndefined(); + }); + + it('should return valid for result bundle path option', () => { + const result = validateXcodebuildExtraArgs([ + '-resultBundlePath', + '/path/to/results.xcresult', + ]); + expect(result.isValid).toBe(true); + expect(result.error).toBeUndefined(); + }); + + it('should return valid for multiple legitimate options', () => { + const result = validateXcodebuildExtraArgs([ + '-only-testing:MyTests/MyTestClass', + '-configuration', + 'Debug', + '-sdk', + 'iphonesimulator', + ]); + expect(result.isValid).toBe(true); + expect(result.error).toBeUndefined(); + }); + }); + + describe('invalid extraArgs', () => { + it('should reject -test-arg option', () => { + const result = validateXcodebuildExtraArgs(['-test-arg', '--snapshot-record']); + expect(result.isValid).toBe(false); + expect(result.error).toContain('-test-arg'); + expect(result.error).toContain('not a recognized xcodebuild argument'); + expect(result.error).toContain('testRunnerEnv'); + }); + + it('should reject --test-arg option (double dash)', () => { + const result = validateXcodebuildExtraArgs(['--test-arg', 'value']); + expect(result.isValid).toBe(false); + expect(result.error).toContain('--test-arg'); + }); + + it('should reject -testArg option (camelCase)', () => { + const result = validateXcodebuildExtraArgs(['-testArg', 'value']); + expect(result.isValid).toBe(false); + expect(result.error).toContain('-testArg'); + }); + + it('should reject --testArg option (camelCase with double dash)', () => { + const result = validateXcodebuildExtraArgs(['--testArg', 'value']); + expect(result.isValid).toBe(false); + expect(result.error).toContain('--testArg'); + }); + + it('should reject when invalid option is mixed with valid options', () => { + const result = validateXcodebuildExtraArgs([ + '-only-testing:MyTests/MyTestClass', + '-test-arg', + '--snapshot-record', + '-verbose', + ]); + expect(result.isValid).toBe(false); + expect(result.error).toContain('-test-arg'); + }); + + it('should provide helpful error message with testRunnerEnv suggestion', () => { + const result = validateXcodebuildExtraArgs(['-test-arg', 'value']); + expect(result.error).toContain('testRunnerEnv'); + expect(result.error).toContain('{ "testRunnerEnv": { "MY_VAR": "value" } }'); + }); + }); + + describe('edge cases', () => { + it('should handle array with only invalid option', () => { + const result = validateXcodebuildExtraArgs(['-test-arg']); + expect(result.isValid).toBe(false); + expect(result.error).toContain('-test-arg'); + }); + + it('should detect first invalid option in sequence', () => { + const result = validateXcodebuildExtraArgs(['-test-arg', '--test-arg', '-testArg']); + expect(result.isValid).toBe(false); + // Should return error for first invalid option encountered + expect(result.error).toBeDefined(); + }); + + it('should handle options that look similar but are valid', () => { + // -test is a valid xcodebuild action, not in our invalid list + const result = validateXcodebuildExtraArgs(['-test', '-verbose']); + expect(result.isValid).toBe(true); + }); + }); +}); diff --git a/src/utils/xcodebuild-validation.ts b/src/utils/xcodebuild-validation.ts new file mode 100644 index 00000000..6d6169ac --- /dev/null +++ b/src/utils/xcodebuild-validation.ts @@ -0,0 +1,49 @@ +/** + * XcodeBuild Validation Utilities + * + * This module provides validation for xcodebuild command parameters, + * particularly for detecting invalid or unsupported options. + */ + +/** + * List of invalid or commonly misused xcodebuild options. + * These options are not recognized by xcodebuild and will cause it to fail. + */ +const INVALID_XCODEBUILD_OPTIONS = new Set([ + '-test-arg', // Not a valid xcodebuild option; use testRunnerEnv instead + '--test-arg', + '-testArg', + '--testArg', +]); + +/** + * Validates extraArgs for invalid xcodebuild options. + * + * @param extraArgs - Array of extra arguments to validate + * @returns Object with isValid flag and error message if invalid + */ +export function validateXcodebuildExtraArgs(extraArgs?: string[]): { + isValid: boolean; + error?: string; +} { + if (!extraArgs || extraArgs.length === 0) { + return { isValid: true }; + } + + // Check for invalid options + for (const arg of extraArgs) { + if (INVALID_XCODEBUILD_OPTIONS.has(arg)) { + return { + isValid: false, + error: `Invalid xcodebuild option: '${arg}'. This is not a recognized xcodebuild argument. + +If you're trying to pass arguments to the test runner, use the 'testRunnerEnv' parameter instead. +Example: { "testRunnerEnv": { "MY_VAR": "value" } } + +For more information, see the xcodebuild man page or documentation.`, + }; + } + } + + return { isValid: true }; +}