Skip to content

Commit 60cfdc3

Browse files
authored
fix: Harden scaffold and utility edge cases (#476)
* fix: Address verified Warden findings Correct scaffold settings, isolate LLDB command output, and make device lookup asynchronous. Also harden xcuserstate parsing, template extraction, and xcodemake installation. Fixes #459 * fix(debugger): Report running state after resume * fix(device): Preserve names after refresh failure * fix(debugger): Clear running state after termination
1 parent 46b2cf6 commit 60cfdc3

14 files changed

Lines changed: 679 additions & 116 deletions

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
- Fixed malformed simulator discovery responses, project discovery path-boundary checks, and compiler diagnostic filenames containing glob metacharacters ([#424](https://github.com/getsentry/XcodeBuildMCP/issues/424)).
1313
- 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)).
14+
- 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)).
1415

1516
## [2.6.2]
1617

@@ -694,4 +695,3 @@ Please note that the UI automation features are an early preview and currently i
694695
- Initial release of XcodeBuildMCP
695696
- Basic support for building iOS and macOS applications
696697

697-
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import { describe, expect, it } from 'vitest';
2+
import { deviceFamiliesToNumeric, orientationToIOSConstant } from '../ios-scaffold-settings.ts';
3+
4+
describe('iOS scaffold settings', () => {
5+
it.each([
6+
['portrait', 'UIInterfaceOrientationPortrait'],
7+
['portrait-upside-down', 'UIInterfaceOrientationPortraitUpsideDown'],
8+
['landscape-left', 'UIInterfaceOrientationLandscapeLeft'],
9+
['landscape-right', 'UIInterfaceOrientationLandscapeRight'],
10+
] as const)('maps %s to its Info.plist constant', (orientation, expected) => {
11+
expect(orientationToIOSConstant(orientation)).toBe(expected);
12+
});
13+
14+
it.each([
15+
[['iphone'], '1'],
16+
[['ipad'], '2'],
17+
[['iphone', 'ipad'], '1,2'],
18+
[['universal'], '1,2'],
19+
] as const)('maps device families %j to %s', (families, expected) => {
20+
expect(deviceFamiliesToNumeric([...families])).toBe(expected);
21+
});
22+
});

src/mcp/tools/project-scaffolding/__tests__/scaffold_ios_project.test.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,12 +129,16 @@ describe('scaffold_ios_project plugin', () => {
129129
await initConfigStoreForTest({ iosTemplatePath: '' });
130130

131131
let capturedCommands: string[][] = [];
132+
let unzipOptions: unknown;
132133
const trackingCommandExecutor = createMockExecutor({
133134
success: true,
134135
output: 'Command executed successfully',
135136
});
136137
const capturingExecutor = async (command: string[], ...args: any[]) => {
137138
capturedCommands.push(command);
139+
if (command[0] === 'unzip') {
140+
unzipOptions = args[2];
141+
}
138142
return trackingCommandExecutor(command, ...args);
139143
};
140144

@@ -162,6 +166,9 @@ describe('scaffold_ios_project plugin', () => {
162166
/https:\/\/github\.com\/getsentry\/XcodeBuildMCP-iOS-Template\/releases\/download\/v\d+\.\d+\.\d+\/XcodeBuildMCP-iOS-Template-\d+\.\d+\.\d+\.zip/,
163167
),
164168
]);
169+
expect(unzipOptions).toEqual({
170+
cwd: expect.stringMatching(/xcodebuild-mcp-template-/),
171+
});
165172

166173
await initConfigStoreForTest({ iosTemplatePath: '/mock/template/path' });
167174
});
@@ -242,6 +249,22 @@ describe('scaffold_ios_project plugin', () => {
242249
});
243250

244251
it('should return success response with all optional parameters', async () => {
252+
let writtenXCConfig: string | undefined;
253+
const xcconfigFileSystem = createMockFileSystemExecutor({
254+
existsSync: (path) => path.includes('/mock/template/path'),
255+
readdir: async () => [
256+
{ name: 'Project.xcconfig', isDirectory: () => false, isFile: () => true } as any,
257+
],
258+
readFile: async () =>
259+
[
260+
'TARGETED_DEVICE_FAMILY = old',
261+
'INFOPLIST_KEY_UISupportedInterfaceOrientations = old',
262+
'INFOPLIST_KEY_UISupportedInterfaceOrientations_iPad = old',
263+
].join('\n'),
264+
writeFile: async (_path, content) => {
265+
writtenXCConfig = content;
266+
},
267+
});
245268
const result = await runLogic(() =>
246269
scaffold_ios_projectLogic(
247270
{
@@ -258,13 +281,20 @@ describe('scaffold_ios_project plugin', () => {
258281
supportedOrientationsIpad: ['portrait', 'landscape-left'],
259282
},
260283
mockCommandExecutor,
261-
mockFileSystemExecutor,
284+
xcconfigFileSystem,
262285
),
263286
);
264287

265288
expect(result.isError).toBeFalsy();
266289
const text = allText(result);
267290
expect(text).toContain('Project scaffolded successfully');
291+
expect(writtenXCConfig).toContain('TARGETED_DEVICE_FAMILY = 1');
292+
expect(writtenXCConfig).toContain(
293+
'INFOPLIST_KEY_UISupportedInterfaceOrientations = UIInterfaceOrientationPortrait',
294+
);
295+
expect(writtenXCConfig).toContain(
296+
'INFOPLIST_KEY_UISupportedInterfaceOrientations_iPad = UIInterfaceOrientationPortrait UIInterfaceOrientationLandscapeLeft',
297+
);
268298
expect(result.nextStepParams).toEqual({
269299
build_sim: {
270300
workspacePath: '/tmp/test-projects/TestIOSApp.xcworkspace',
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
export type IOSDeviceFamily = 'iphone' | 'ipad' | 'universal';
2+
3+
export type IOSOrientation =
4+
| 'portrait'
5+
| 'landscape-left'
6+
| 'landscape-right'
7+
| 'portrait-upside-down';
8+
9+
const ORIENTATION_CONSTANTS: Record<IOSOrientation, string> = {
10+
portrait: 'UIInterfaceOrientationPortrait',
11+
'portrait-upside-down': 'UIInterfaceOrientationPortraitUpsideDown',
12+
'landscape-left': 'UIInterfaceOrientationLandscapeLeft',
13+
'landscape-right': 'UIInterfaceOrientationLandscapeRight',
14+
};
15+
16+
/** Converts a scaffold orientation token to its Info.plist build-setting constant. */
17+
export function orientationToIOSConstant(orientation: IOSOrientation): string {
18+
return ORIENTATION_CONSTANTS[orientation];
19+
}
20+
21+
/** Converts scaffold device-family tokens to Xcode's numeric build-setting value. */
22+
export function deviceFamiliesToNumeric(families: IOSDeviceFamily[]): string {
23+
if (families.includes('universal')) {
24+
return '1,2';
25+
}
26+
27+
const numericFamilies = new Set<string>();
28+
if (families.includes('iphone')) {
29+
numericFamilies.add('1');
30+
}
31+
if (families.includes('ipad')) {
32+
numericFamilies.add('2');
33+
}
34+
return [...numericFamilies].join(',');
35+
}

src/mcp/tools/project-scaffolding/scaffold_ios_project.ts

Lines changed: 13 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,12 @@ import {
1414
getHandlerContext,
1515
} from '../../../utils/typed-tool-factory.ts';
1616
import { createScaffoldDomainResult, setScaffoldStructuredOutput } from './domain-result.ts';
17+
import {
18+
deviceFamiliesToNumeric,
19+
orientationToIOSConstant,
20+
type IOSDeviceFamily,
21+
type IOSOrientation,
22+
} from './ios-scaffold-settings.ts';
1723

1824
const BaseScaffoldSchema = z.object({
1925
projectName: z.string().min(1),
@@ -36,40 +42,6 @@ const ScaffoldiOSProjectSchema = BaseScaffoldSchema.extend({
3642
.optional(),
3743
});
3844

39-
/**
40-
* Convert orientation enum to iOS constant
41-
*/
42-
function orientationToIOSConstant(orientation: string): string {
43-
switch (orientation) {
44-
case 'Portrait':
45-
return 'UIInterfaceOrientationPortrait';
46-
case 'PortraitUpsideDown':
47-
return 'UIInterfaceOrientationPortraitUpsideDown';
48-
case 'LandscapeLeft':
49-
return 'UIInterfaceOrientationLandscapeLeft';
50-
case 'LandscapeRight':
51-
return 'UIInterfaceOrientationLandscapeRight';
52-
default:
53-
return orientation;
54-
}
55-
}
56-
57-
/**
58-
* Convert device family enum to numeric value
59-
*/
60-
function deviceFamilyToNumeric(family: string): string {
61-
switch (family) {
62-
case 'iPhone':
63-
return '1';
64-
case 'iPad':
65-
return '2';
66-
case 'iPhone+iPad':
67-
return '1,2';
68-
default:
69-
return '1,2';
70-
}
71-
}
72-
7345
/**
7446
* Update Package.swift file with deployment target
7547
*/
@@ -114,9 +86,11 @@ function updateXCConfigFile(content: string, params: Record<string, unknown>): s
11486
const currentProjectVersion = params.currentProjectVersion as string | undefined;
11587
const platform = params.platform as string;
11688
const deploymentTarget = params.deploymentTarget as string | undefined;
117-
const targetedDeviceFamily = params.targetedDeviceFamily as string | undefined;
118-
const supportedOrientations = params.supportedOrientations as string[] | undefined;
119-
const supportedOrientationsIpad = params.supportedOrientationsIpad as string[] | undefined;
89+
const targetedDeviceFamily = params.targetedDeviceFamily as IOSDeviceFamily[] | undefined;
90+
const supportedOrientations = params.supportedOrientations as IOSOrientation[] | undefined;
91+
const supportedOrientationsIpad = params.supportedOrientationsIpad as
92+
| IOSOrientation[]
93+
| undefined;
12094

12195
// Update project identity settings
12296
result = result.replace(/PRODUCT_NAME = .+/g, `PRODUCT_NAME = ${projectName}`);
@@ -148,8 +122,8 @@ function updateXCConfigFile(content: string, params: Record<string, unknown>): s
148122
}
149123

150124
// Device family
151-
if (targetedDeviceFamily) {
152-
const deviceFamilyValue = deviceFamilyToNumeric(targetedDeviceFamily);
125+
if (targetedDeviceFamily && targetedDeviceFamily.length > 0) {
126+
const deviceFamilyValue = deviceFamiliesToNumeric(targetedDeviceFamily);
153127
result = result.replace(
154128
/TARGETED_DEVICE_FAMILY = .+/g,
155129
`TARGETED_DEVICE_FAMILY = ${deviceFamilyValue}`,
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
import { beforeEach, describe, expect, it, vi } from 'vitest';
2+
import {
3+
__resetDeviceNameCacheForTests,
4+
formatDeviceId,
5+
resolveDeviceName,
6+
type DeviceNameResolverDependencies,
7+
} from '../device-name-resolver.ts';
8+
9+
const deviceId = 'device-identifier';
10+
const udid = '00008110-0012345678901234';
11+
12+
function createDependencies(
13+
overrides: Partial<DeviceNameResolverDependencies> = {},
14+
): DeviceNameResolverDependencies {
15+
return {
16+
runDevicectl: vi.fn().mockResolvedValue(undefined),
17+
readOutput: vi.fn().mockResolvedValue(
18+
JSON.stringify({
19+
result: {
20+
devices: [
21+
{
22+
identifier: deviceId,
23+
deviceProperties: { name: 'Cam’s iPhone' },
24+
hardwareProperties: { udid },
25+
},
26+
],
27+
},
28+
}),
29+
),
30+
removeOutput: vi.fn().mockResolvedValue(undefined),
31+
createOutputPath: () => '/tmp/devices.json',
32+
now: () => 1_000,
33+
...overrides,
34+
};
35+
}
36+
37+
describe('device name resolver', () => {
38+
beforeEach(() => {
39+
__resetDeviceNameCacheForTests();
40+
});
41+
42+
it('loads device names asynchronously and resolves identifiers and UDIDs', async () => {
43+
const deps = createDependencies();
44+
45+
await expect(resolveDeviceName(deviceId, deps)).resolves.toBe('Cam’s iPhone');
46+
await expect(resolveDeviceName(udid, deps)).resolves.toBe('Cam’s iPhone');
47+
48+
expect(deps.runDevicectl).toHaveBeenCalledOnce();
49+
expect(deps.runDevicectl).toHaveBeenCalledWith('/tmp/devices.json');
50+
expect(deps.removeOutput).toHaveBeenCalledWith('/tmp/devices.json');
51+
});
52+
53+
it('returns immediately while an asynchronous cache refresh is running', async () => {
54+
let finishRefresh: (() => void) | undefined;
55+
const runDevicectl = vi.fn(
56+
() =>
57+
new Promise<void>((resolve) => {
58+
finishRefresh = resolve;
59+
}),
60+
);
61+
const deps = createDependencies({ runDevicectl });
62+
63+
expect(formatDeviceId(deviceId, deps)).toBe(deviceId);
64+
expect(runDevicectl).toHaveBeenCalledOnce();
65+
66+
finishRefresh?.();
67+
await expect(resolveDeviceName(deviceId, deps)).resolves.toBe('Cam’s iPhone');
68+
expect(formatDeviceId(deviceId, deps)).toBe(`Cam’s iPhone (${deviceId})`);
69+
});
70+
71+
it('falls back to the device ID when devicectl fails', async () => {
72+
const deps = createDependencies({
73+
runDevicectl: vi.fn().mockRejectedValue(new Error('unavailable')),
74+
});
75+
76+
await expect(resolveDeviceName(deviceId, deps)).resolves.toBeUndefined();
77+
expect(formatDeviceId(deviceId, deps)).toBe(deviceId);
78+
expect(deps.removeOutput).toHaveBeenCalledWith('/tmp/devices.json');
79+
});
80+
81+
it('retains stale device names when a background refresh fails', async () => {
82+
let now = 1_000;
83+
const deps = createDependencies({ now: () => now });
84+
85+
await expect(resolveDeviceName(deviceId, deps)).resolves.toBe('Cam’s iPhone');
86+
now = 31_001;
87+
vi.mocked(deps.runDevicectl).mockRejectedValueOnce(new Error('unavailable'));
88+
89+
expect(formatDeviceId(deviceId, deps)).toBe(`Cam’s iPhone (${deviceId})`);
90+
await expect(resolveDeviceName(deviceId, deps)).resolves.toBe('Cam’s iPhone');
91+
await expect(resolveDeviceName(deviceId, deps)).resolves.toBe('Cam’s iPhone');
92+
expect(formatDeviceId(deviceId, deps)).toBe(`Cam’s iPhone (${deviceId})`);
93+
expect(deps.runDevicectl).toHaveBeenCalledTimes(3);
94+
});
95+
});

src/utils/__tests__/nskeyedarchiver-parser.test.ts

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,9 @@
1-
import { describe, it, expect } from 'vitest';
1+
import { beforeEach, describe, it, expect, vi } from 'vitest';
2+
import { parseBuffer as bplistParseBuffer } from 'bplist-parser';
3+
4+
vi.mock('bplist-parser', () => ({
5+
parseBuffer: vi.fn(),
6+
}));
27
import {
38
parseXcuserstate,
49
parseXcuserstateBuffer,
@@ -8,6 +13,12 @@ import {
813
} from '../nskeyedarchiver-parser.ts';
914

1015
describe('NSKeyedArchiver Parser', () => {
16+
beforeEach(() => {
17+
vi.mocked(bplistParseBuffer).mockImplementation(() => {
18+
throw new Error('invalid plist');
19+
});
20+
});
21+
1122
describe('parseXcuserstate (file path)', () => {
1223
it('returns empty result for non-existent file', () => {
1324
const result = parseXcuserstate('/non/existent/file.xcuserstate');
@@ -97,11 +108,27 @@ describe('NSKeyedArchiver Parser', () => {
97108
});
98109

99110
describe('edge cases', () => {
100-
it('handles xcuserstate without ActiveScheme', () => {
101-
// This would require a specially crafted test fixture
102-
// For now, we just verify the function doesn't crash
103-
const result = parseXcuserstateBuffer(Buffer.from('bplist00'));
104-
expect(result).toEqual({});
111+
it('extracts ActiveRunDestination without ActiveScheme', () => {
112+
const simulatorId = '12345678-1234-1234-1234-123456789ABC';
113+
vi.mocked(bplistParseBuffer).mockReturnValue([
114+
{
115+
$archiver: 'NSKeyedArchiver',
116+
$objects: [
117+
'$null',
118+
'ActiveRunDestination',
119+
'targetDeviceLocation',
120+
{ 'NS.keys': [{ UID: 1 }], 'NS.objects': [{ UID: 4 }] },
121+
{ 'NS.keys': [{ UID: 2 }], 'NS.objects': [{ UID: 5 }] },
122+
`dvtdevice-iphonesimulator:${simulatorId}`,
123+
],
124+
},
125+
]);
126+
127+
expect(parseXcuserstateBuffer(Buffer.from('plist'))).toEqual({
128+
deviceLocation: `dvtdevice-iphonesimulator:${simulatorId}`,
129+
simulatorId,
130+
simulatorPlatform: 'iphonesimulator',
131+
});
105132
});
106133

107134
it('handles scheme object without IDENameString', () => {

0 commit comments

Comments
 (0)