Skip to content

fix(build): Handle unexpected throws in build-only tools#378

Closed
cameroncooke wants to merge 1 commit intomainfrom
cam/issue-334-build-only-with-error-handling
Closed

fix(build): Handle unexpected throws in build-only tools#378
cameroncooke wants to merge 1 commit intomainfrom
cam/issue-334-build-only-with-error-handling

Conversation

@cameroncooke
Copy link
Copy Markdown
Collaborator

Add defensive error handling for build-only tool execution paths.

Issue #334 called out that unexpected throws from the build execution layer could escape build-only tools. This change wraps executeXcodeBuildCommand calls in the three build-only executors (build_device, build_sim, build_macos) and converts those throws into failed build domain results, so callers get a structured failure instead of an unhandled exception.

I considered adding a broader framework-level catch in createSessionAwareTool, but that would affect every tool and increase regression risk. Keeping this change local to the three build-only executors is the smallest consistent fix for the reported gap.

Also adds focused regression tests for each tool and updates CHANGELOG.md under Unreleased.

Fixes #334

Wrap build-only executor xcodebuild invocations in try/catch so unexpected
execution exceptions are converted into failed build domain results.

Add focused regression tests for device, simulator, and macOS build-only
tools to confirm thrown execution errors no longer escape the handler.

Fixes #334
Co-Authored-By: OpenAI Codex <codex@openai.com>
Comment on lines +314 to +316
it('should return error result when executeXcodeBuildCommand throws unexpectedly', async () => {
const executeSpy = vi
.spyOn(buildUtils, 'executeXcodeBuildCommand')
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test uses vi.spyOn on imported module instead of injected dependency

This new test uses vi.spyOn(buildUtils, 'executeXcodeBuildCommand') to force a throw, rather than injecting a mock dependency through the executor parameter. The skill's guardrail requires unit tests to inject command/filesystem/external dependencies and prefer using existing mock executor helpers. Module-level spying couples the test to internal import structure and bypasses the dependency injection contract used elsewhere in this file (e.g., createMockExecutor, createSpyExecutor).

Verification

Reviewed the hunk and surrounding tests in the same describe block, which consistently inject createMockExecutor/createSpyExecutor per the skill's mock-executor helper guidance. The new test instead patches the buildUtils module export, which conflicts with the 'inject command/filesystem/external dependencies' and 'use existing mock executor helpers' guardrails. Could not verify whether buildDeviceLogic exposes a seam to inject executeXcodeBuildCommand directly without reading the source, so confidence is medium.

Identified by Warden xcodebuildmcp-test-boundary-review · DZ7-24F

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Apr 28, 2026

Open in StackBlitz

npm i https://pkg.pr.new/xcodebuildmcp@378

commit: f09579c

@cameroncooke cameroncooke marked this pull request as ready for review April 29, 2026 08:19
@cameroncooke
Copy link
Copy Markdown
Collaborator Author

Closing this after re-auditing the actual xcodebuild execution path.

The reported failure mode is already handled at the shared helper layer. executeXcodeBuildCommand catches executor throws and returns { isError: true }, and that behavior is already covered in src/utils/__tests__/build-utils.test.ts with thrown spawn errors such as ENOENT, EACCES, and EPERM.

Because build, test, and build-run tools all route through the same helper with different xcodebuild actions, adding per-tool try/catch wrappers would duplicate the shared error boundary and diverge from the existing pattern used by the build_run_* tools.

The new tests in this PR forced an artificial failure mode by mocking executeXcodeBuildCommand itself to reject. That bypasses the production seam (CommandExecutor) and made a normally non-throwing helper appear to need per-tool catch blocks.

No code change is needed here. If a real escaping path is found later, the fix should be proven and handled in executeXcodeBuildCommand, not individually in build-only tools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add withErrorHandling to build-only tool logic functions

1 participant