Fix:#96 fail to process xcodebuild error ooutput.#97
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.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #96 where xcodebuild failed to properly detect and handle simulator-locked errors and other fatal conditions during streaming output. The solution introduces streaming command execution with early termination on fatal patterns, adds immediate error/warning visibility to build responses, and pins the MCP SDK version for stability.
Key Changes:
- Added
executeCommandStreamingfunction with fatal pattern detection and early process termination - Enhanced build responses to include first 10 errors/warnings at top level for immediate visibility
- Reduced build timeout from 10 minutes to 55 seconds to stay under MCP transport limits
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/command.ts | Adds new streaming command execution with fatal pattern detection and timeout handling |
| src/utils/response-cache.ts | Extends build summary to include arrays of first 10 errors and warnings |
| src/tools/xcodebuild/build.ts | Integrates streaming execution, processes fatal matches and timeouts, surfaces errors/warnings at top level |
| tests/tests/utils/response-cache.test.ts | Updates tests to validate new errors/warnings arrays and 10-item limit |
| package.json, package-lock.json | Pins @modelcontextprotocol/sdk to exact version 1.17.1 |
| README.md | Updates documentation to reflect immediate error/warning visibility in build responses |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 lacks test coverage. While other functions in this module have comprehensive tests in command.test.ts, there are no tests for the streaming functionality, fatal pattern detection, timeout behavior, or buffer overflow handling. Consider adding tests for this new function to match the existing test coverage pattern.
| \`\`\`typescript | ||
| // Build returns errors/warnings at top level - no second call needed | ||
| xcodebuild-build({ projectPath: "MyApp.xcodeproj", scheme: "MyApp" }) | ||
| // Returns: | ||
| { | ||
| buildId: "abc123", | ||
| success: true, | ||
| warnings: ["...warning: unused variable..."], // immediate visibility | ||
| 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 backticks in the code block are escaped with backslashes, which will render literally in the markdown instead of creating a proper code block. Remove the backslash escapes before the triple backticks.
| } = options; | ||
|
|
||
| return new Promise<StreamingCommandResult>((resolve, reject) => { | ||
| const child = spawn(command, { shell: true, timeout }); | ||
|
|
There was a problem hiding this comment.
The cwd option from CommandOptions is not extracted from the options parameter and not passed to the spawn call. This means the working directory cannot be configured for streaming commands, creating an inconsistency with the existing executeCommand function which does support cwd. Consider extracting cwd from options and passing it to spawn.
| } = options; | |
| return new Promise<StreamingCommandResult>((resolve, reject) => { | |
| const child = spawn(command, { shell: true, timeout }); | |
| cwd, | |
| } = options; | |
| return new Promise<StreamingCommandResult>((resolve, reject) => { | |
| const child = spawn(command, { shell: true, timeout, cwd }); |
| child.stderr?.on('data', data => { | ||
| const text = data.toString(); | ||
| stderr += text; | ||
| checkPatterns(text); |
There was a problem hiding this comment.
The stderr buffer size is not checked against maxBuffer, only stdout is checked. This could lead to unbounded memory growth if stderr produces large amounts of output. Consider adding a similar check for stderr length as is done for stdout at line 113.
| checkPatterns(text); | |
| checkPatterns(text); | |
| if (stderr.length > maxBuffer) { | |
| child.kill(); | |
| clearTimeout(timeoutId); | |
| reject( | |
| new McpError( | |
| ErrorCode.InternalError, | |
| `Command error output exceeded max buffer size of ${maxBuffer} bytes` | |
| ) | |
| ); | |
| } |
Code Review — PR #97: Fix xcodebuild error output (streaming + early visibility)OverviewThis PR adds streaming command execution to Relationship to PR #98This PR and PR #98 are from the same author and share most of their changed code:
PR #98 is a superset of this PR (it adds the Issues (shared with PR #98)🔴 55-second build timeout will break large real-world projectsFile: const timeoutMs = 55_000; // Stay under MCP transport limitsThis drops the timeout from 10 minutes to 55 seconds. Medium-to-large iOS projects routinely exceed 55 seconds on a clean build, and this will cause builds to report The fatal pattern early-exit and the timeout are independent mechanisms — early-exit on detected fatal patterns can stay at 55s or even 0 (immediate), while the overall timeout should remain at 5–10 minutes for legitimate builds. 🟡 SDK pinned to exact version
|
Code Review — PR #97: Fix xcodebuild error output processingOverall: 🔶 Needs changes (shares critical issues with PR #98, plus merge conflict) What this does
|
Failed to detect when xcodebuild encounters "simulator locked" issue 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.