diff --git a/CHANGELOG.md b/CHANGELOG.md index ce689a600..eb9d3090a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ - Fixed `suppressWarnings` being ignored in settled build, build-run, and test output. The flag was honored only while streaming, so warnings still reached the final MCP tool response ([#447](https://github.com/getsentry/XcodeBuildMCP/issues/447)). - Fixed iOS scaffold orientation and device-family settings, LLDB command isolation and argument escaping, run-destination parsing without an active scheme, concurrent working-directory mutations, blocking physical-device name lookup, and unverified `xcodemake` downloads ([#459](https://github.com/getsentry/XcodeBuildMCP/issues/459)). - Fixed simulator UI launching and keyboard controls to prefer Xcode 27's Device Hub when available, with Simulator.app as the legacy fallback. +- Fixed `stop_mac_app` app-name targeting so it no longer terminates unrelated processes whose command lines contain the app name, and reject unsafe process IDs before execution ([#306](https://github.com/getsentry/XcodeBuildMCP/issues/306)). - Fixed incremental `xcodemake` builds when DerivedData uses an absolute path by updating the pinned wrapper and delegating Makefile reuse to it so its argument and freshness checks are always applied ([#466](https://github.com/getsentry/XcodeBuildMCP/issues/466)). ## [2.6.2] diff --git a/src/mcp/tools/macos/__tests__/stop_mac_app.test.ts b/src/mcp/tools/macos/__tests__/stop_mac_app.test.ts index bb93564c2..fc43d486c 100644 --- a/src/mcp/tools/macos/__tests__/stop_mac_app.test.ts +++ b/src/mcp/tools/macos/__tests__/stop_mac_app.test.ts @@ -1,6 +1,11 @@ import { describe, it, expect } from 'vitest'; import { schema, handler, stop_mac_appLogic } from '../stop_mac_app.ts'; -import { allText, runLogic } from '../../../../test-utils/test-helpers.ts'; +import { + allText, + createMockToolHandlerContext, + runLogic, +} from '../../../../test-utils/test-helpers.ts'; +import { createMockExecutor, createNoopExecutor } from '../../../../test-utils/mock-executors.ts'; describe('stop_mac_app plugin', () => { describe('Export Field Validation (Literal)', () => { @@ -17,28 +22,53 @@ describe('stop_mac_app plugin', () => { // Test invalid inputs expect(schema.appName.safeParse(null).success).toBe(false); + expect(schema.appName.safeParse('').success).toBe(false); expect(schema.processId.safeParse('not-number').success).toBe(false); expect(schema.processId.safeParse(null).success).toBe(false); + expect(schema.processId.safeParse(0).success).toBe(false); + expect(schema.processId.safeParse(-1).success).toBe(false); + expect(schema.processId.safeParse(1.5).success).toBe(false); + expect(schema.processId.safeParse(Number.NaN).success).toBe(false); + expect(schema.processId.safeParse(Number.MAX_SAFE_INTEGER + 1).success).toBe(false); }); }); describe('Input Validation', () => { it('should return exact validation error for missing parameters', async () => { - const mockExecutor = async () => ({ success: true, output: '', process: {} as any }); - const result = await runLogic(() => stop_mac_appLogic({}, mockExecutor)); + const result = await runLogic(() => stop_mac_appLogic({}, createNoopExecutor())); expect(result.isError).toBe(true); expect(allText(result)).toContain('appName or processId'); }); + + it.each([0, -1, 1.5, Number.NaN, Number.MAX_SAFE_INTEGER + 1])( + 'should reject unsafe process ID %s at the execution boundary', + async (processId) => { + const calls: string[][] = []; + const executor = createMockExecutor({ onExecute: (command) => calls.push(command) }); + const { ctx, result, run } = createMockToolHandlerContext(); + + await run(() => stop_mac_appLogic({ processId }, executor)); + + expect(result.isError()).toBe(true); + expect(result.text()).toContain('processId must be a positive safe integer'); + const structuredResult = ctx.structuredOutput?.result; + expect(structuredResult?.kind).toBe('stop-result'); + if (structuredResult?.kind !== 'stop-result') { + throw new Error('Expected stop-result structured output.'); + } + expect(structuredResult.artifacts).toEqual({ appName: '' }); + expect(calls).toHaveLength(0); + }, + ); }); describe('Command Generation', () => { it('should generate correct command for process ID', async () => { - const calls: any[] = []; - const mockExecutor = async (command: string[]) => { - calls.push({ command }); - return { success: true, output: '', process: {} as any }; - }; + const calls: string[][] = []; + const mockExecutor = createMockExecutor({ + onExecute: (command) => calls.push(command), + }); await runLogic(() => stop_mac_appLogic( @@ -50,35 +80,69 @@ describe('stop_mac_app plugin', () => { ); expect(calls).toHaveLength(1); - expect(calls[0].command).toEqual(['kill', '1234']); + expect(calls[0]).toEqual(['kill', '1234']); }); - it('should generate correct command for app name', async () => { - const calls: any[] = []; - const mockExecutor = async (command: string[]) => { - calls.push({ command }); - return { success: true, output: '', process: {} as any }; - }; + it('should target app names by literal process name', async () => { + const calls: string[][] = []; + const mockExecutor = createMockExecutor({ + onExecute: (command) => calls.push(command), + }); await runLogic(() => stop_mac_appLogic( { - appName: 'Calculator', + appName: 'Brimday', }, mockExecutor, ), ); expect(calls).toHaveLength(1); - expect(calls[0].command).toEqual(['pkill', '-f', 'Calculator']); + expect(calls[0]).toEqual(['killall', '--', 'Brimday']); + }); + + it('should pass long app executable names to killall', async () => { + const calls: string[][] = []; + const mockExecutor = createMockExecutor({ + onExecute: (command) => calls.push(command), + }); + + await runLogic(() => + stop_mac_appLogic( + { + appName: 'ThisIsAVeryLongApplicationName', + }, + mockExecutor, + ), + ); + + expect(calls[0]).toEqual(['killall', '--', 'ThisIsAVeryLongApplicationName']); + }); + + it('should treat app names as literal process names', async () => { + const calls: string[][] = []; + const mockExecutor = createMockExecutor({ + onExecute: (command) => calls.push(command), + }); + + await runLogic(() => + stop_mac_appLogic( + { + appName: '-Example.*[Test]', + }, + mockExecutor, + ), + ); + + expect(calls[0]).toEqual(['killall', '--', '-Example.*[Test]']); }); it('should prioritize processId over appName', async () => { - const calls: any[] = []; - const mockExecutor = async (command: string[]) => { - calls.push({ command }); - return { success: true, output: '', process: {} as any }; - }; + const calls: string[][] = []; + const mockExecutor = createMockExecutor({ + onExecute: (command) => calls.push(command), + }); await runLogic(() => stop_mac_appLogic( @@ -91,13 +155,13 @@ describe('stop_mac_app plugin', () => { ); expect(calls).toHaveLength(1); - expect(calls[0].command).toEqual(['kill', '1234']); + expect(calls[0]).toEqual(['kill', '1234']); }); }); describe('Response Processing', () => { it('should return exact successful stop response by app name', async () => { - const mockExecutor = async () => ({ success: true, output: '', process: {} as any }); + const mockExecutor = createMockExecutor({}); const result = await runLogic(() => stop_mac_appLogic( @@ -112,7 +176,7 @@ describe('stop_mac_app plugin', () => { }); it('should return exact successful stop response with both parameters (processId takes precedence)', async () => { - const mockExecutor = async () => ({ success: true, output: '', process: {} as any }); + const mockExecutor = createMockExecutor({}); const result = await runLogic(() => stop_mac_appLogic( @@ -128,9 +192,7 @@ describe('stop_mac_app plugin', () => { }); it('should handle execution errors', async () => { - const mockExecutor = async () => { - throw new Error('Process not found'); - }; + const mockExecutor = createMockExecutor(new Error('Process not found')); const result = await runLogic(() => stop_mac_appLogic( diff --git a/src/mcp/tools/macos/stop_mac_app.ts b/src/mcp/tools/macos/stop_mac_app.ts index 2f4cc2d49..9b70c263b 100644 --- a/src/mcp/tools/macos/stop_mac_app.ts +++ b/src/mcp/tools/macos/stop_mac_app.ts @@ -14,8 +14,13 @@ import { } from '../../../utils/app-lifecycle-results.ts'; const stopMacAppSchema = z.object({ - appName: z.string().optional(), - processId: z.number().optional(), + appName: z.string().min(1).optional(), + processId: z + .number() + .int() + .positive() + .refine(Number.isSafeInteger, 'processId must be a positive safe integer.') + .optional(), }); type StopMacAppParams = z.infer; @@ -54,20 +59,29 @@ export function createStopMacAppExecutor( executor: CommandExecutor, ): NonStreamingExecutor { return async (params) => { - const artifacts = createStopMacAppArtifacts(params); - if (!params.appName && params.processId === undefined) { - return buildStopFailure(artifacts, 'Either appName or processId must be provided.'); + return buildStopFailure({ appName: '' }, 'Either appName or processId must be provided.'); } - const target = params.processId ? `PID ${params.processId}` : params.appName!; + if ( + params.processId !== undefined && + (!Number.isSafeInteger(params.processId) || params.processId <= 0) + ) { + return buildStopFailure( + { appName: params.appName ?? '' }, + 'processId must be a positive safe integer.', + ); + } + + const artifacts = createStopMacAppArtifacts(params); + const target = params.processId !== undefined ? `PID ${params.processId}` : params.appName!; log('info', `Stopping macOS app: ${target}`); try { const command = params.processId !== undefined ? ['kill', String(params.processId)] - : ['pkill', '-f', params.appName!]; + : ['killall', '--', params.appName!]; const result = await executor(command, 'Stop macOS App'); if (!result.success) { diff --git a/src/smoke-tests/__tests__/e2e-mcp-device-macos.test.ts b/src/smoke-tests/__tests__/e2e-mcp-device-macos.test.ts index fa048fc47..f4a395de5 100644 --- a/src/smoke-tests/__tests__/e2e-mcp-device-macos.test.ts +++ b/src/smoke-tests/__tests__/e2e-mcp-device-macos.test.ts @@ -20,7 +20,7 @@ beforeAll(async () => { 'xctrace list devices': { success: true, output: 'No devices found.' }, open: { success: true, output: '' }, kill: { success: true, output: '' }, - pkill: { success: true, output: '' }, + killall: { success: true, output: '' }, 'defaults read': { success: true, output: 'io.sentry.MyApp' }, PlistBuddy: { success: true, output: 'io.sentry.MyApp' }, xcresulttool: { success: true, output: '{}' }, @@ -328,7 +328,7 @@ describe('MCP Device and macOS Tool Invocation (e2e)', () => { expect(commandStrs.some((c) => c.includes('kill') && c.includes('54321'))).toBe(true); }); - it('stop_mac_app captures pkill command with appName', async () => { + it('stop_mac_app captures killall command with appName', async () => { harness.resetCapturedCommands(); const result = await harness.client.callTool({ name: 'stop_mac_app', @@ -338,7 +338,7 @@ describe('MCP Device and macOS Tool Invocation (e2e)', () => { expectContent(result); const commandStrs = harness.capturedCommands.map((c) => c.command.join(' ')); - expect(commandStrs.some((c) => c.includes('MyMacApp'))).toBe(true); + expect(commandStrs.some((c) => c === 'killall -- MyMacApp')).toBe(true); }); it('get_mac_app_path captures xcodebuild showBuildSettings command', async () => {