Cocoapods build path fix #167
Conversation
Co-authored-by: web <web@cameroncooke.com>
|
Cursor Agent can help with this pull request. Just |
commit: |
WalkthroughThis pull request refactors the executor interface and related test infrastructure to support passing working directory (cwd) information to build commands. Test files across the device and macOS build tools are updated to replace timeout parameters with an opts object containing an optional cwd field. The mock executor in test utilities is enhanced with an onExecute callback mechanism to capture and verify executor invocations. Production code in build-utils.ts is modified to pass the derived project directory as cwd when executing xcodebuild. New test cases are added to verify correct cwd handling for both workspace and project build scenarios, including preservation of existing environment variables. Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/mcp/tools/macos/__tests__/build_run_macos.test.ts (3)
137-152: Inconsistent test migration: timeout vs opts.Line 151 expects
timeout: undefined, but the first executor call (line 134) correctly usesopts: { cwd: '/path/to' }. The second executor call (for build settings) doesn't receive acwdparameter in the production code, so the expectation should either beopts: undefinedor the field should be omitted entirely for clarity.🔎 Suggested fix
expect(executorCalls[1]).toEqual({ command: [ 'xcodebuild', '-showBuildSettings', '-project', '/path/to/project.xcodeproj', '-scheme', 'MyApp', '-configuration', 'Debug', ], description: 'Get Build Settings for Launch', logOutput: true, - timeout: undefined, + opts: undefined, });
233-248: Inconsistent test migration: timeout vs opts.Line 247 has the same issue as line 151 - it expects
timeout: undefinedwhilst the executor signature has been updated to useopts. For consistency with the migrated tests, this should be updated to match the new parameter structure.🔎 Suggested fix
expect(executorCalls[1]).toEqual({ command: [ 'xcodebuild', '-showBuildSettings', '-workspace', '/path/to/workspace.xcworkspace', '-scheme', 'MyApp', '-configuration', 'Debug', ], description: 'Get Build Settings for Launch', logOutput: true, - timeout: undefined, + opts: undefined, });
294-320: Incomplete test migration to opts parameter.These test cases still use
timeout?: numberin their mock executor signatures (lines 301, 357, 418) whilst other tests in this file have been migrated to useopts?: { cwd?: string }. For consistency and to align with the broader changes in this PR, these should be updated to use the newoptsparameter structure.🔎 Example fix for one test case (lines 294-320)
const mockExecutor = ( command: string[], description: string, logOutput: boolean, - timeout?: number, + opts?: { cwd?: string }, ) => { callCount++; if (callCount === 1) { // First call for build succeeds return Promise.resolve({ success: true, output: 'BUILD SUCCEEDED', error: '', }); } else if (callCount === 2) { // Second call for build settings fails return Promise.resolve({ success: false, output: '', error: 'error: Failed to get settings', }); } return Promise.resolve({ success: true, output: '', error: '' }); };Apply similar changes to the other test cases at lines 350-383 and 413-438.
Also applies to: 350-383, 413-438
🧹 Nitpick comments (2)
src/utils/build-utils.ts (1)
102-109: Consider validating projectDir before use.The
projectDiris derived usingpath.dirname()but isn't validated before being passed ascwd. IfworkspacePathorprojectPathare at the root level or malformed,projectDircould be an empty string or unexpected value, potentially causing the executor to fail silently or use an unintended directory.🔎 Suggested validation
let projectDir = ''; if (params.workspacePath) { projectDir = path.dirname(params.workspacePath); command.push('-workspace', params.workspacePath); } else if (params.projectPath) { projectDir = path.dirname(params.projectPath); command.push('-project', params.projectPath); } + +if (!projectDir || projectDir === '/') { + log('warning', `Derived project directory is invalid: '${projectDir}'. Using current directory.`); + projectDir = '.'; +}src/utils/__tests__/build-utils.test.ts (1)
264-347: Excellent test coverage for cwd handling, with a minor type safety suggestion.The three test cases comprehensively verify:
- Workspace builds receive the correct
cwd- Project builds receive the correct
cwd- Existing
execOpts.envis preserved when merging withcwdThe use of the
onExecutecallback for observation aligns with the DI testing philosophy from learnings.💡 Optional: Improve type safety
Consider replacing
anywith a proper type forcapturedOptions:-let capturedOptions: any; +let capturedOptions: { cwd?: string; env?: Record<string, string> } | undefined;This would provide better type checking whilst maintaining test clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/mcp/tools/device/__tests__/build_device.test.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.tssrc/test-utils/mock-executors.tssrc/utils/__tests__/build-utils.test.tssrc/utils/build-utils.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript file extensions (.ts) for all relative imports to ensure compatibility with native TypeScript runtimes
Never use .js extensions for internal file imports; always use .ts extensions
Use .js extensions only for external package imports (e.g., @modelcontextprotocol/sdk/server/mcp.js)
Files:
src/utils/__tests__/build-utils.test.tssrc/utils/build-utils.tssrc/mcp/tools/device/__tests__/build_device.test.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.tssrc/test-utils/mock-executors.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx}: Enforce strict Dependency Injection (DI) testing philosophy - completely ban the use of Vitest mocking utilities (vi.mock(), vi.fn(), vi.spyOn(), etc.)
Use injectable executors (CommandExecutor and FileSystemExecutor) for all external interactions instead of mocking
Import core logic functions from tool files and pass mock executors to simulate different test outcomes
Files:
src/utils/__tests__/build-utils.test.tssrc/mcp/tools/device/__tests__/build_device.test.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
When Claude Code is detected, automatically consolidate multiple content blocks into a single text response separated by --- dividers to work around MCP specification violation
Files:
src/utils/__tests__/build-utils.test.tssrc/utils/build-utils.tssrc/mcp/tools/device/__tests__/build_device.test.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.tssrc/test-utils/mock-executors.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:47:54.969Z
Learning: Applies to **/*.test.{ts,tsx} : Use injectable executors (CommandExecutor and FileSystemExecutor) for all external interactions instead of mocking
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:47:54.969Z
Learning: Applies to **/*.test.{ts,tsx} : Import core logic functions from tool files and pass mock executors to simulate different test outcomes
📚 Learning: 2026-01-05T22:47:54.969Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:47:54.969Z
Learning: Applies to **/*.test.{ts,tsx} : Import core logic functions from tool files and pass mock executors to simulate different test outcomes
Applied to files:
src/utils/__tests__/build-utils.test.tssrc/mcp/tools/device/__tests__/build_device.test.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.tssrc/test-utils/mock-executors.ts
📚 Learning: 2026-01-05T22:47:54.969Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:47:54.969Z
Learning: Applies to **/*.test.{ts,tsx} : Use injectable executors (CommandExecutor and FileSystemExecutor) for all external interactions instead of mocking
Applied to files:
src/utils/__tests__/build-utils.test.tssrc/utils/build-utils.tssrc/mcp/tools/device/__tests__/build_device.test.tssrc/mcp/tools/macos/__tests__/build_run_macos.test.tssrc/test-utils/mock-executors.ts
📚 Learning: 2026-01-05T22:47:54.969Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T22:47:54.969Z
Learning: Applies to src/tools/**/*.ts : Organize tools into directories by their functionality and use automatic loading via plugin-based MCP architecture
Applied to files:
src/mcp/tools/device/__tests__/build_device.test.ts
🧬 Code graph analysis (1)
src/utils/__tests__/build-utils.test.ts (2)
src/test-utils/mock-executors.ts (1)
createMockExecutor(27-89)src/utils/build-utils.ts (1)
executeXcodeBuildCommand(45-373)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
src/test-utils/mock-executors.ts (1)
36-88: LGTM! Clean test observability enhancement.The
onExecutecallback addition enables tests to capture and verify executor invocations without violating the DI testing philosophy. The implementation is backward compatible and properly propagates the newoptsparameter (includingcwdandenv) to test observers.Based on learnings, this aligns with the injectable executor pattern for test infrastructure.
src/utils/build-utils.ts (1)
230-234: LGTM! Core fix correctly implements the PR objective.The code properly passes the derived
projectDirascwdto the executor, ensuring CocoaPods relative paths resolve correctly. The spread operator correctly preserves any existingexecOptswhilst setting the working directory.src/mcp/tools/device/__tests__/build_device.test.ts (2)
130-180: LGTM! Comprehensive test coverage for workspace cwd handling.The test correctly verifies that the executor receives
cwd: '/path/to'when building with a workspace at/path/to/MyProject.xcworkspace. The stub executor signature properly captures the newoptsparameter.
182-232: LGTM! Comprehensive test coverage for project cwd handling.The test correctly verifies that the executor receives
cwd: '/path/to'when building with a project at/path/to/MyProject.xcodeproj. Consistent with the workspace test pattern.src/mcp/tools/macos/__tests__/build_run_macos.test.ts (1)
76-171: LGTM! Consistent test updates for cwd handling.These test cases correctly verify that the first executor call (for the build) receives
opts: { cwd: '/path/to' }, ensuring the working directory is properly set for CocoaPods relative path resolution. The test coverage includes both project and workspace scenarios.Also applies to: 173-267
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| description: 'macOS Build', | ||
| logOutput: true, | ||
| timeout: undefined, | ||
| opts: { cwd: '/path/to' }, |
There was a problem hiding this comment.
Test expectations use timeout but stub tracks opts
Medium Severity
The mock executor stub was refactored to track opts instead of timeout (line 88 pushes { command, description, logOutput, opts }), but the test expectations for executorCalls[1] at lines 151 and 247 still assert timeout: undefined. This mismatch causes the toEqual assertion to fail because the actual object has an opts property while the expected object has a timeout property. These should be changed to opts: undefined to match the stub's tracking structure.
Fix CocoaPods build path resolution by passing the project directory as the current working directory (
cwd) to thexcodebuildcommand executor.CocoaPods projects use relative paths in
.xcfilelistfiles. Previously,xcodebuildwas not always executed from the project's root directory, causing these relative paths to fail and leading to build errors like "Unable to load contents of file list." This change ensuresxcodebuildruns in the correct context, resolving these path issues.Note
Ensures CocoaPods-relative paths resolve by running
xcodebuildfrom the project directory.executeXcodeBuildCommandto mergeexecOptswithcwdset to the project dir before calling the executorcreateMockExecutorto accept full executor signature and optionalonExecutecallback; propagateoptscwdhandling tests inbuild-utils.test.ts(workspace, project, andexecOptsmerge)opts: { cwd: '/path/to' }instead oftimeoutand validate command generationWritten by Cursor Bugbot for commit 80f29cc. This will update automatically on new commits. Configure here.