Fix: destination parameter bug#98
Conversation
…to latest 1.25.1 which depends on zod v4 which has breaking changes.
…ssue and produces streaming output. Which caused xc-mcp to not return anything at all. Enhanced xcodebuild output with immediate error/warning visibility and streaming command execution.
…ning on macOS platform.
There was a problem hiding this comment.
Pull request overview
This pull request fixes a critical bug where iOS simulator destinations were incorrectly applied to macOS builds, preventing macOS builds from running. The fix introduces explicit parameter tracking, platform-compatible destination resolution, and enhanced build output visibility.
Changes:
- Introduced explicit parameter tracking to distinguish caller-provided vs. cached values, preventing stale destinations from overriding explicit parameters
- Implemented platform-aware destination resolution that filters simulator suggestions by platform and avoids simulator carry-over for macOS/device builds
- Enhanced build output with top-level errors/warnings arrays (first 10 each) and added fatal pattern detection with early termination
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/tools/xcodebuild/build.ts |
Core fix: parameter tracking, destination resolution logic, platform-compatible caching, streaming command execution with fatal pattern detection |
src/utils/command.ts |
Added executeCommandStreaming function with timeout and fatal pattern detection capabilities |
src/utils/response-cache.ts |
Enhanced extractBuildSummary to include first 10 errors and warnings arrays for immediate visibility |
tests/__tests__/tools/xcodebuild-build.test.ts |
New comprehensive test suite validating destination resolution, parameter precedence, and platform matching |
tests/__tests__/utils/response-cache.test.ts |
Added tests for errors/warnings arrays including 10-item limit validation |
README.md |
Updated documentation to reflect immediate error/warning visibility without secondary calls |
package.json, package-lock.json, src/index.ts |
Version updates and dependency pinning |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| summary: { errorCount: 0, warningCount: 1, duration: 5000 } | ||
| } | ||
| // Only use xcodebuild-get-details for full logs or >10 errors/warnings | ||
| \`\`\` |
There was a problem hiding this comment.
The code block uses escaped backticks (\\\\`) instead of regular markdown code fence backticks. This will render incorrectly in markdown - the backslashes should be removed so it displays as a proper code block.
| const provided = { | ||
| configuration: Object.prototype.hasOwnProperty.call(rawArgs, 'configuration'), | ||
| destination: Object.prototype.hasOwnProperty.call(rawArgs, 'destination'), | ||
| sdk: Object.prototype.hasOwnProperty.call(rawArgs, 'sdk'), | ||
| derivedDataPath: Object.prototype.hasOwnProperty.call(rawArgs, 'derivedDataPath'), | ||
| }; |
There was a problem hiding this comment.
Using Object.prototype.hasOwnProperty.call() is the correct safe approach to check property existence. However, the check only verifies if the property exists on the object, not if it's explicitly defined by the caller. For example, if destructuring provides a default value (e.g., 'configuration = "Debug"' on line 78), the property would still exist in rawArgs. Consider documenting this behavior or ensuring that defaults are not applied during destructuring if you want to distinguish between explicitly-provided and defaulted parameters.
| { | ||
| buildId: "build-abc123", | ||
| success: true, | ||
| warnings: ["...warning: unused variable 'foo'..."], // first 10 warnings |
There was a problem hiding this comment.
The code comment says "first 10 warnings" but the actual implementation in response-cache.ts slices both errors and warnings to the first 10. The comment should be updated to mention both errors and warnings for consistency.
| warnings: ["...warning: unused variable 'foo'..."], // first 10 warnings | |
| warnings: ["...warning: unused variable 'foo'..."], // first 10 warnings (errors similarly truncated) |
| const augmentedErrors = [...summary.errors]; | ||
| if (result.fatalMatch) { | ||
| augmentedErrors.unshift(`Detected fatal xcodebuild output: ${result.fatalMatch}`); | ||
| } | ||
| if (result.timedOut) { | ||
| augmentedErrors.unshift(`Build aborted after ${timeoutMs}ms (timeout)`); | ||
| } |
There was a problem hiding this comment.
The error augmentation logic uses unshift to add fatal errors at the beginning, but since errors array is already sliced to 10 items in extractBuildSummary, this could result in more than 10 errors being returned. Consider slicing augmentedErrors after all additions to maintain the 10-item limit mentioned in the documentation.
| export async function executeCommandStreaming( | ||
| command: string, | ||
| options: StreamingOptions = {} | ||
| ): Promise<StreamingCommandResult> { | ||
| const { | ||
| timeout = 60000, | ||
| maxBuffer = 10 * 1024 * 1024, | ||
| fatalPatterns = [], | ||
| onFatalMatch, | ||
| } = options; | ||
|
|
||
| return new Promise<StreamingCommandResult>((resolve, reject) => { | ||
| const child = spawn(command, { shell: true, timeout }); | ||
|
|
||
| let stdout = ''; | ||
| let stderr = ''; | ||
| let timedOut = false; | ||
| let fatalMatch: string | undefined; | ||
|
|
||
| const checkPatterns = (chunk: string) => { | ||
| if (fatalMatch) return; | ||
| for (const pattern of fatalPatterns) { | ||
| const match = chunk.match(pattern); | ||
| if (match) { | ||
| fatalMatch = match[0]; | ||
| onFatalMatch?.(match[0]); | ||
| child.kill(); | ||
| break; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const timeoutId = setTimeout(() => { | ||
| timedOut = true; | ||
| child.kill(); | ||
| }, timeout); | ||
|
|
||
| child.stdout?.on('data', data => { | ||
| const text = data.toString(); | ||
| stdout += text; | ||
| checkPatterns(text); | ||
| if (stdout.length > maxBuffer) { | ||
| child.kill(); | ||
| clearTimeout(timeoutId); | ||
| reject( | ||
| new McpError( | ||
| ErrorCode.InternalError, | ||
| `Command output exceeded max buffer size of ${maxBuffer} bytes` | ||
| ) | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| child.stderr?.on('data', data => { | ||
| const text = data.toString(); | ||
| stderr += text; | ||
| checkPatterns(text); | ||
| }); | ||
|
|
||
| child.on('close', code => { | ||
| clearTimeout(timeoutId); | ||
| resolve({ | ||
| stdout: stdout.trim(), | ||
| stderr: stderr.trim(), | ||
| code: code ?? 0, | ||
| timedOut, | ||
| fatalMatch, | ||
| }); | ||
| }); | ||
|
|
||
| child.on('error', error => { | ||
| clearTimeout(timeoutId); | ||
| reject(new McpError(ErrorCode.InternalError, `Failed to execute command: ${error.message}`)); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The new executeCommandStreaming function introduced in this PR lacks unit test coverage. Since tests/tests/utils/command.test.ts exists and tests other command utilities, consider adding comprehensive tests for executeCommandStreaming including timeout scenarios, fatal pattern detection, and buffer overflow handling.
|
|
||
| ## Example: Optimal Login Flow | ||
|
|
||
| \`\`\`typescript |
There was a problem hiding this comment.
The code block uses escaped backticks (\\\\`) instead of regular markdown code fence backticks. This will render incorrectly in markdown - the backslashes should be removed so it displays as a proper code block.
| } | ||
|
|
||
| // Destructure errors/warnings from summary for top-level placement | ||
| const { errors: _ignoredErrors, warnings, ...summaryRest } = adjustedSummary; |
There was a problem hiding this comment.
The destructuring operation extracts 'errors' from adjustedSummary and renames it to '_ignoredErrors', but then uses the separate 'augmentedErrors' array for the top-level errors field. This is correct but the naming could be clearer. Consider renaming '_ignoredErrors' to '_summaryErrors' to better communicate that these are the summary's error list (which is superseded by augmentedErrors).
| const { errors: _ignoredErrors, warnings, ...summaryRest } = adjustedSummary; | |
| const { errors: _summaryErrors, warnings, ...summaryRest } = adjustedSummary; |
| { | ||
| name: 'xc-mcp', | ||
| version: '3.2.0', | ||
| version: '3.2.1', |
There was a problem hiding this comment.
Version mismatch detected: package.json declares version 3.2.2, but src/index.ts declares version 3.2.1. These versions should be synchronized to avoid confusion.
| version: '3.2.1', | |
| version: '3.2.2', |
| describe('xcodebuild-build parameter resolution', () => { | ||
| const projectPath = 'SpaceTime.xcodeproj'; | ||
| const scheme = 'SpaceTime'; | ||
|
|
||
| const successResult = { | ||
| stdout: '', | ||
| stderr: '', | ||
| code: 0, | ||
| timedOut: false, | ||
| }; | ||
|
|
||
| beforeEach(() => { | ||
| jest.resetModules(); | ||
| jest.clearAllMocks(); | ||
|
|
||
| mockBuildXcodebuildCommand.mockImplementation((action: string, path: string, opts: any) => | ||
| JSON.stringify({ action, path, opts }) | ||
| ); | ||
| mockExecuteCommandStreaming.mockResolvedValue(successResult); | ||
| mockExtractBuildSummary.mockReturnValue({ | ||
| success: true, | ||
| errors: [], | ||
| warnings: [], | ||
| errorCount: 0, | ||
| warningCount: 0, | ||
| buildSizeBytes: 0, | ||
| }); | ||
| mockResponseCacheStore.mockReturnValue('build-id'); | ||
| mockCreateConfigManager.mockReturnValue({ | ||
| recordSuccessfulBuild: jest.fn(), | ||
| }); | ||
| }); | ||
|
|
||
| async function loadTool() { | ||
| const mod = await import('../../../src/tools/xcodebuild/build.js'); | ||
| return mod.xcodebuildBuildTool; | ||
| } | ||
|
|
||
| function responseFrom(result: any) { | ||
| return JSON.parse(result.content[0].text); | ||
| } | ||
|
|
||
| it('uses explicit destination and does not override it', async () => { | ||
| mockGetPreferredBuildConfig.mockResolvedValue({ | ||
| scheme, | ||
| configuration: 'Release', | ||
| destination: 'platform=iOS Simulator,id=CACHED', | ||
| }); | ||
|
|
||
| const xcodebuildBuildTool = await loadTool(); | ||
|
|
||
| const result = await xcodebuildBuildTool({ | ||
| projectPath, | ||
| scheme, | ||
| destination: 'generic/platform=macOS,name=Any Mac', | ||
| sdk: 'macosx', | ||
| }); | ||
|
|
||
| const lastCall = mockBuildXcodebuildCommand.mock.calls.at(-1)![2] as any; | ||
| expect(lastCall.destination).toBe('generic/platform=macOS,name=Any Mac'); | ||
| expect(mockGetPreferredSimulator).not.toHaveBeenCalled(); | ||
|
|
||
| const response = responseFrom(result); | ||
| expect(response.intelligence.usedSmartDestination).toBe(false); | ||
| }); | ||
|
|
||
| it('skips cached simulator destination when building for macOS', async () => { | ||
| mockGetPreferredBuildConfig.mockResolvedValue({ | ||
| scheme, | ||
| configuration: 'Debug', | ||
| destination: 'platform=iOS Simulator,id=CACHED', | ||
| }); | ||
|
|
||
| const xcodebuildBuildTool = await loadTool(); | ||
|
|
||
| const result = await xcodebuildBuildTool({ | ||
| projectPath, | ||
| scheme, | ||
| sdk: 'macosx', | ||
| }); | ||
|
|
||
| const lastCall = mockBuildXcodebuildCommand.mock.calls.at(-1)![2] as any; | ||
| expect(lastCall.destination).toBeUndefined(); | ||
|
|
||
| const response = responseFrom(result); | ||
| expect(response.intelligence.usedSmartDestination).toBe(false); | ||
| }); | ||
|
|
||
| it('reuses cached destination when platform matches sdk', async () => { | ||
| mockGetPreferredBuildConfig.mockResolvedValue({ | ||
| scheme, | ||
| configuration: 'Debug', | ||
| destination: 'platform=tvOS Simulator,id=TV-1', | ||
| }); | ||
|
|
||
| const xcodebuildBuildTool = await loadTool(); | ||
|
|
||
| const result = await xcodebuildBuildTool({ | ||
| projectPath, | ||
| scheme, | ||
| sdk: 'tvossimulator', | ||
| }); | ||
|
|
||
| const lastCall = mockBuildXcodebuildCommand.mock.calls.at(-1)![2] as any; | ||
| expect(lastCall.destination).toBe('platform=tvOS Simulator,id=TV-1'); | ||
|
|
||
| const response = responseFrom(result); | ||
| expect(response.intelligence.usedSmartDestination).toBe(true); | ||
| }); | ||
|
|
||
| it('selects a platform-matched simulator when no cached destination', async () => { | ||
| mockGetPreferredBuildConfig.mockResolvedValue({ | ||
| scheme, | ||
| configuration: 'Debug', | ||
| }); | ||
|
|
||
| mockGetPreferredSimulator.mockResolvedValue(null); | ||
| mockGetSimulatorList.mockResolvedValue({ | ||
| devices: { | ||
| 'com.apple.CoreSimulator.SimRuntime.tvOS-18-0': [ | ||
| { | ||
| udid: 'TV-UDID', | ||
| name: 'Apple TV', | ||
| deviceTypeIdentifier: 'com.apple.CoreSimulator.SimDeviceType.Apple-TV-4K', | ||
| state: 'Shutdown', | ||
| isAvailable: true, | ||
| availability: 'available', | ||
| bootHistory: [], | ||
| }, | ||
| ], | ||
| 'com.apple.CoreSimulator.SimRuntime.iOS-18-0': [ | ||
| { | ||
| udid: 'PHONE', | ||
| name: 'iPhone', | ||
| deviceTypeIdentifier: 'com.apple.CoreSimulator.SimDeviceType.iPhone-16', | ||
| state: 'Shutdown', | ||
| isAvailable: true, | ||
| availability: 'available', | ||
| bootHistory: [], | ||
| }, | ||
| ], | ||
| }, | ||
| runtimes: [], | ||
| devicetypes: [], | ||
| lastUpdated: new Date(), | ||
| preferredByProject: new Map(), | ||
| }); | ||
|
|
||
| const xcodebuildBuildTool = await loadTool(); | ||
|
|
||
| const result = await xcodebuildBuildTool({ | ||
| projectPath, | ||
| scheme, | ||
| sdk: 'tvossimulator', | ||
| }); | ||
|
|
||
| const lastCall = mockBuildXcodebuildCommand.mock.calls.at(-1)![2] as any; | ||
| expect(lastCall.destination).toBe('platform=tvOS Simulator,id=TV-UDID'); | ||
|
|
||
| const response = responseFrom(result); | ||
| expect(response.intelligence.usedSmartDestination).toBe(true); | ||
| }); | ||
|
|
||
| it('prioritizes caller-provided configuration over cached', async () => { | ||
| mockGetPreferredBuildConfig.mockResolvedValue({ | ||
| scheme, | ||
| configuration: 'Debug', | ||
| }); | ||
|
|
||
| const xcodebuildBuildTool = await loadTool(); | ||
|
|
||
| await xcodebuildBuildTool({ | ||
| projectPath, | ||
| scheme, | ||
| configuration: 'Release', | ||
| }); | ||
|
|
||
| const lastCall = mockBuildXcodebuildCommand.mock.calls.at(-1)![2] as any; | ||
| expect(lastCall.configuration).toBe('Release'); | ||
| }); | ||
|
|
||
| it('uses cached configuration when caller does not provide one and marks smart usage', async () => { | ||
| mockGetPreferredBuildConfig.mockResolvedValue({ | ||
| scheme, | ||
| configuration: 'Release', | ||
| }); | ||
|
|
||
| const xcodebuildBuildTool = await loadTool(); | ||
|
|
||
| const result = await xcodebuildBuildTool({ | ||
| projectPath, | ||
| scheme, | ||
| }); | ||
|
|
||
| const lastCall = mockBuildXcodebuildCommand.mock.calls.at(-1)![2] as any; | ||
| expect(lastCall.configuration).toBe('Release'); | ||
|
|
||
| const response = responseFrom(result); | ||
| expect(response.intelligence.usedSmartConfiguration).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The new fatal pattern detection and timeout handling features introduced in executeCommandStreaming and build.ts lack test coverage. Consider adding test cases for scenarios where timeout occurs (timedOut: true) or when a fatal pattern is matched (fatalMatch is set).
| const baseName = lastSegment.split('-')[0]; | ||
| if (!baseName) return undefined; |
There was a problem hiding this comment.
The platformLabelFromRuntime function assumes the runtime format has a segment with a dash separator (e.g., 'tvOS-18-0'), but if the runtime string doesn't contain a dash, baseName will be the entire lastSegment. Consider adding defensive handling for runtimes that don't follow the expected format to avoid malformed platform labels like 'com Simulator' or 'SimRuntime Simulator'.
| const baseName = lastSegment.split('-')[0]; | |
| if (!baseName) return undefined; | |
| if (!lastSegment) return undefined; | |
| const hasDash = lastSegment.includes('-'); | |
| const baseName = hasDash ? lastSegment.split('-')[0] : lastSegment; | |
| if (!baseName) return undefined; | |
| // If there is no dash in the last segment, make sure the base name looks like a platform | |
| // token before constructing a label. This avoids labels like "com Simulator" or | |
| // "SimRuntime Simulator" for unexpected runtime formats. | |
| if (!hasDash) { | |
| const lower = baseName.toLowerCase(); | |
| const looksLikePlatform = | |
| lower.includes('mac') || | |
| lower.includes('iphone') || | |
| lower.includes('ios') || | |
| lower.includes('tvos') || | |
| lower.includes('appletv') || | |
| lower.includes('watch') || | |
| lower.includes('vision'); | |
| if (!looksLikePlatform) { | |
| return undefined; | |
| } | |
| } |
Code Review — PR #98: Fix destination parameter bugOverviewThis PR fixes a real bug where cached iOS simulator destinations were being injected into macOS builds, making macOS targets unbuildable. It also adds streaming command execution with early-exit on fatal patterns, and promotes errors/warnings to the top-level build response. Good motivation, but there are significant concerns. Issues🔴 55-second build timeout will break large real-world projectsFile: const timeoutMs = 55_000; // Stay under MCP transport limitsThe old timeout was 10 minutes. This change caps all builds at 55 seconds. In practice, even medium-sized iOS projects regularly take 2–5 minutes on a clean build, and anything with many dependencies or a cold cache can exceed 10 minutes. This will cause The comment "Stay under MCP transport limits" doesn't hold up — MCP transport has no 55-second constraint. If there's a specific transport issue, please document it. Otherwise, consider restoring a longer timeout (5–10 minutes) and keeping early-exit only for the fatal patterns. A better approach: const timeoutMs = 600_000; // 10 minutes for builds...with early-exit still triggered by 🔴 Version inconsistency in
|
| Status | |
|---|---|
| 55s build timeout | 🔴 Will break large projects — restore 10 min, keep fatal-exit |
Version mismatch 3.2.1 vs 3.2.2 |
🔴 Fix before merge |
| SDK exact-version pin | 🟡 Prefer ^1.17.1 unless there's a regression reason |
resolveDestination bug fix |
✅ Correct and well-structured |
executeCommandStreaming |
✅ Sound design (minor: spawn timeout no-op) |
| Errors/warnings at top level | ✅ Good UX improvement |
provided tracking |
🟡 Works, could simplify |
Code Review — PR #98: Fix destination parameter bugOverall: 🔶 Needs changes (critical timeout issue + minor items) What this does
🚨 Critical — 55-second build timeout will break real-world iOS buildsconst timeoutMs = 55_000; // Stay under MCP transport limitsReplacing a 10-minute timeout with 55 seconds will cause routine builds to fail. Clean iOS simulator builds regularly take 2–5 minutes; clean builds of mid-size projects exceed that. The comment says "stay under MCP transport limits" but MCP stdio transport has no timeout on individual tool calls — this constraint doesn't apply here. Please either restore the previous 600 000 ms default or make the timeout configurable (e.g. an optional Bug — double timeout on
|
This prevents not being able to run macOS builds at all because "iOS" was injected as destination regardless of destination parameter passed.
Fixes: prevent stale iOS simulator destinations from being applied, avoid hidden overrides when the caller provided a destination.