Skip to content

Commit 8ad1a78

Browse files
authored
refactor(4/12): extract build/test utilities, platform steps, and xcodebuild pipeline (#322)
## Summary This is **PR 4 of 12** in a stacked PR series that decouples the rendering pipeline from MCP transport. Depends on PR 3 (rendering engine). Extracts shared build and test logic from monolithic utility files into focused, single-responsibility modules. This is prep work for the tool handler migrations (PRs 6-9) -- the extracted modules provide the building blocks that tool handlers will call instead of duplicating logic inline. ### What was extracted and why **Build preflight** (`build-preflight.ts`): Validates build parameters (project path, scheme, destination) before invoking xcodebuild. Previously scattered across individual tool handlers with subtle inconsistencies. **Test preflight** (`test-preflight.ts`): Validates test parameters, resolves test plans, and checks for common misconfiguration. Extracted from the ~500-line test tool handlers where each platform (sim/device/macOS) had its own copy. **Platform step modules**: Each platform's build/test/run workflow was a monolithic function. Now decomposed into composable steps: - `simulator-steps.ts`: Boot simulator, install app, launch app, capture logs - `device-steps.ts`: Prepare device, install, launch - `macos-steps.ts`: Build, launch, capture logs - `app-path-resolver.ts`: Locate built .app bundles in DerivedData (shared across all platforms) - `device-name-resolver.ts`: Resolve device UDID to display name **Xcodebuild pipeline** (`xcodebuild-pipeline.ts`): The core build/test execution engine. Accepts an `emit` callback from the handler context, streams parsed events during execution, and returns structured state. This replaces the old pattern where the pipeline owned renderers directly. **Xcodebuild output** (`xcodebuild-output.ts`): Constructs the final event sequence from pipeline state (summary, detail trees, diagnostics). Separated from the pipeline itself so the same output logic works for both real builds and test replay. **Supporting extractions**: `derived-data-path.ts`, `log-paths.ts`, `xcodebuild-log-capture.ts`, `swift-test-discovery.ts`, `xcresult-test-failures.ts`, `simulator-test-execution.ts`, `tool-error-handling.ts`. ### Deleted modules - `capabilities.ts`, `validation/index.ts`, `test-result-content.ts`, `workflow-selection.ts` -- functionality moved into the extracted modules or no longer needed with the new architecture. ### Changes to existing modules - `build-utils.ts`: Slimmed down significantly. Warning suppression, content consolidation, and build-specific logic moved to dedicated modules. - `test-common.ts`: Simplified to a thin coordination layer that delegates to `test-preflight.ts` and `swift-test-discovery.ts`. ## Stack navigation - PR 1-3/12: Foundation (logging removal, event types, rendering engine) - **PR 4/12** (this PR): Build/test utility extraction, platform steps, xcodebuild pipeline - PR 5/12: Runtime handler contract and tool invoker - PR 6-9/12: Tool migrations (these use the extracted modules) - PR 10-12/12: Boundaries, config, tests ## Test plan - [ ] `npx vitest run` passes -- tests for each extracted module - [ ] Build preflight correctly rejects invalid schemes/destinations - [ ] Pipeline streams events through emit callback during execution - [ ] Xcodebuild output constructs correct event sequences from pipeline state
1 parent 200ac9c commit 8ad1a78

42 files changed

Lines changed: 4187 additions & 1094 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.cursor/BUGBOT.md

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ For full details see [README.md](README.md) and [docs/ARCHITECTURE.md](docs/ARCH
1212
## 1. Security Checklist — Critical
1313

1414
* No hard-coded secrets, tokens or DSNs.
15-
* All shell commands must flow through `CommandExecutor` with validated arguments (no direct `child_process` calls).
15+
* MCP tool logic functions that orchestrate long-running processes with sub-processes (e.g., `xcodebuild`) must flow through `CommandExecutor` with validated arguments. Standalone utility modules that invoke simple, short-lived commands may use direct `child_process`/`fs` imports and standard vitest mocking.
1616
* Paths must be sanitised via helpers in `src/utils/validation.ts`.
1717
* Sentry breadcrumbs / logs must **NOT** include user PII.
1818

@@ -22,7 +22,7 @@ For full details see [README.md](README.md) and [docs/ARCHITECTURE.md](docs/ARCH
2222

2323
| Rule | Quick diff heuristic |
2424
|------|----------------------|
25-
| Dependency injection only | New `child_process` \| `fs` import ⇒ **critical** |
25+
| Dependency injection for tool logic | New `child_process` \| `fs` import in MCP tool logic **warning** (check if process is complex/long-running) |
2626
| Handler / Logic split | `handler` > 20 LOC or contains branching ⇒ **critical** |
2727
| Plugin auto-registration | Manual `registerTool(...)` / `registerResource(...)`**critical** |
2828

@@ -45,7 +45,8 @@ export const handler = (p: FooBarParams) => fooBarLogic(p);
4545

4646
## 3. Testing Checklist
4747

48-
* **External-boundary rule**: Use `createMockExecutor` / `createMockFileSystemExecutor` for command execution and filesystem side effects.
48+
* **External-boundary rule for tool logic**: Use `createMockExecutor` / `createMockFileSystemExecutor` for complex process orchestration (xcodebuild, multi-step pipelines) in MCP tool logic functions.
49+
* **Simple utilities**: Standalone utility modules with simple command calls can use direct imports and standard vitest mocking.
4950
* **Internal mocking is allowed**: `vi.mock`, `vi.fn`, `vi.spyOn`, and `.mock*` are acceptable for internal modules/collaborators.
5051
* Each tool must have tests covering happy-path **and** at least one failure path.
5152
* Avoid the `any` type unless justified with an inline comment.
@@ -66,15 +67,15 @@ export const handler = (p: FooBarParams) => fooBarLogic(p);
6667
|--------------|--------------------|
6768
| Complex logic in `handler` | Move to `*Logic` function |
6869
| Re-implementing logging | Use `src/utils/logger.ts` |
69-
| Direct `fs` / `child_process` usage | Inject `FileSystemExecutor` / `CommandExecutor` |
70+
| Direct `fs` / `child_process` in tool logic orchestrating complex processes | Inject `FileSystemExecutor` / `CommandExecutor` |
7071
| Chained re-exports | Export directly from source |
7172

7273
---
7374

7475
### How Bugbot Can Verify Rules
7576

7677
1. **External-boundary violations**: confirm tests use injected executors/filesystem for external side effects.
77-
2. **DI compliance**: search for direct `child_process` / `fs` imports outside approved patterns.
78+
2. **DI compliance**: check direct `child_process` / `fs` imports in MCP tool logic; standalone utilities with simple commands are acceptable.
7879
3. **Docs accuracy**: compare `docs/TOOLS.md` against `src/mcp/tools/**`.
7980
4. **Style**: ensure ESLint and Prettier pass (`npm run lint`, `npm run format:check`).
8081

docs/dev/ARCHITECTURE.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -418,13 +418,13 @@ Not all parts are required for every tool. For example, `swift_package_build` ha
418418

419419
### Testing Principles
420420

421-
XcodeBuildMCP uses a **Dependency Injection (DI)** pattern for testing external boundaries (command execution, filesystem, and other side effects). Vitest mocking libraries (`vi.mock`, `vi.fn`, etc.) are acceptable for internal collaborators when needed. This keeps tests robust while preserving deterministic behavior at external boundaries.
421+
XcodeBuildMCP uses a **Dependency Injection (DI)** pattern for MCP tool logic functions that orchestrate complex, long-running processes with sub-processes (e.g., `xcodebuild`), where standard vitest mocking produces race conditions. Standalone utility modules with simple commands may use direct `child_process`/`fs` imports and standard vitest mocking. Vitest mocking libraries (`vi.mock`, `vi.fn`, etc.) are also acceptable for internal collaborators.
422422

423423
For detailed guidelines, see the [Testing Guide](TESTING.md).
424424

425425
### Test Structure Example
426426

427-
Tests inject mock "executors" for external interactions like command-line execution or file system access. This allows for deterministic testing of tool logic without mocking the implementation itself. The project provides helper functions like `createMockExecutor` and `createMockFileSystemExecutor` in `src/test-utils/mock-executors.ts` to facilitate this pattern.
427+
Tests for MCP tool logic inject mock "executors" for complex process orchestration (e.g., xcodebuild). This allows for deterministic testing without race conditions from non-deterministic sub-process ordering. The project provides helper functions like `createMockExecutor` and `createMockFileSystemExecutor` in `src/test-utils/mock-executors.ts`. Standalone utility modules with simple commands use standard vitest mocking.
428428

429429
```typescript
430430
import { describe, it, expect } from 'vitest';

docs/dev/CODE_QUALITY.md

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -53,17 +53,18 @@ XcodeBuildMCP enforces several architectural patterns that cannot be expressed t
5353

5454
### 1. Dependency Injection Pattern
5555

56-
**Rule**: All tools must use dependency injection for external interactions.
56+
**Rule**: MCP tool logic functions that orchestrate complex, long-running processes with sub-processes (e.g., `xcodebuild`) must use dependency injection for external interactions. This is because standard vitest mocking produces race conditions when sub-process ordering is non-deterministic.
57+
58+
Standalone utility modules that invoke simple, short-lived commands (e.g., `xcrun devicectl list`, `xcrun xcresulttool get`) may use direct `child_process`/`fs` imports and be tested with standard vitest mocking.
5759

5860
**Allowed**:
59-
- `createMockExecutor()` for command execution mocking
60-
- `createMockFileSystemExecutor()` for file system mocking
61-
- Logic functions accepting `executor?: CommandExecutor` parameter
61+
- `createMockExecutor()` / `createMockFileSystemExecutor()` for complex process orchestration in tool logic
62+
- Logic functions accepting `executor?: CommandExecutor` parameter for xcodebuild and similar pipelines
63+
- Direct `child_process`/`fs` imports in standalone utility modules with simple commands, tested via vitest mocking
6264

6365
**Forbidden**:
64-
- Direct calls to `execSync`, `spawn`, or `exec` in production code
6566
- Testing handler functions directly
66-
- Real external side effects in unit tests (xcodebuild/xcrun/filesystem writes outside mocks)
67+
- Real external side effects in unit tests (real `xcodebuild`, `xcrun`, AXe, filesystem writes/reads outside test harness)
6768

6869
### 2. Handler Signature Compliance
6970

docs/dev/CONTRIBUTING.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ Before making changes, please familiarize yourself with:
270270
All contributions must adhere to the testing standards outlined in the [**XcodeBuildMCP Plugin Testing Guidelines (TESTING.md)**](TESTING.md). This is the canonical source of truth for all testing practices.
271271

272272
**Key Principles (Summary):**
273-
- **Dependency Injection for External Boundaries**: All external dependencies (command execution, file system access) must be injected into tool logic functions using the `CommandExecutor` and `FileSystemExecutor` patterns.
273+
- **Dependency Injection for Complex Processes**: MCP tool logic functions that orchestrate complex, long-running processes with sub-processes (e.g., `xcodebuild`) must use injected `CommandExecutor` and `FileSystemExecutor` patterns. Standalone utility modules with simple commands may use direct imports and standard vitest mocking.
274274
- **Internal Mocking Is Allowed**: Vitest mocking (`vi.mock`, `vi.fn`, `vi.spyOn`, etc.) is acceptable for internal modules/collaborators.
275275
- **Test Production Code**: Tests must import and execute the actual tool logic, not mock implementations.
276276
- **Comprehensive Coverage**: Tests must cover input validation, command generation, and output processing.

docs/dev/TESTING.md

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,28 +20,32 @@ This document provides comprehensive testing guidelines for XcodeBuildMCP plugin
2020

2121
### 🚨 CRITICAL: External Dependency Mocking Rules
2222

23-
### ABSOLUTE RULE: External side effects must use dependency injection utilities
23+
### When to use dependency-injection executors
2424

25-
### Use dependency-injection mocks for EXTERNAL dependencies:
26-
- `createMockExecutor()` / `createNoopExecutor()` for command execution (`xcrun`, `xcodebuild`, AXe, etc.)
27-
- `createMockFileSystemExecutor()` / `createNoopFileSystemExecutor()` for file system interactions
25+
`CommandExecutor` / `FileSystemExecutor` DI is required for **MCP tool logic functions** that orchestrate complex, long-running processes with sub-processes (e.g., `xcodebuild`, multi-step build pipelines). Standard vitest mocking produces race conditions with these because sub-process ordering is non-deterministic.
26+
27+
- `createMockExecutor()` / `createNoopExecutor()` for command execution in tool logic
28+
- `createMockFileSystemExecutor()` / `createNoopFileSystemExecutor()` for file system interactions in tool logic
29+
30+
### When standard vitest mocking is fine
31+
32+
Standalone utility modules that invoke simple, short-lived commands (e.g., `xcrun devicectl list`, `xcrun xcresulttool get`) may use direct `child_process`/`fs` imports and be tested with standard vitest mocking (`vi.fn`, `vi.mock`, `vi.spyOn`, etc.). This is simpler and perfectly adequate for deterministic, single-command utilities.
2833

2934
### Internal mocking guidance:
3035
- Vitest mocking (`vi.fn`, `vi.mock`, `vi.spyOn`, `.mockResolvedValue`, etc.) is allowed for internal modules and in-memory collaborators
3136
- Prefer straightforward, readable test doubles over over-engineered mocks
3237

3338
### Still forbidden:
3439
- Hitting real external systems in unit tests (real `xcodebuild`, `xcrun`, AXe, filesystem writes/reads outside test harness)
35-
- Bypassing dependency injection for external effects
3640

3741
### OUR CORE PRINCIPLE
3842

39-
**Simple Rule**: Use dependency-injection mock executors for external boundaries; use Vitest mocking only for internal behavior.
43+
**Simple Rule**: Use dependency-injection mock executors for complex process orchestration in tool logic; use standard vitest mocking for simple utility modules and internal behavior.
4044

4145
**Why This Rule Exists**:
42-
1. **Reliability**: External side effects stay deterministic and hermetic
43-
2. **Clarity**: Internal collaboration assertions remain concise and readable
44-
3. **Architectural Enforcement**: External boundaries are explicit in tool logic signatures
46+
1. **Reliability**: Complex multi-process orchestration stays deterministic and hermetic via DI executors
47+
2. **Simplicity**: Simple utilities use standard vitest mocking without unnecessary abstraction
48+
3. **Architectural Enforcement**: External boundaries for complex processes are explicit in tool logic signatures
4549
4. **Maintainability**: Tests fail for behavior regressions, not incidental environment differences
4650

4751
### Integration Testing with Dependency Injection
@@ -111,7 +115,7 @@ Test → Plugin Handler → utilities → [DEPENDENCY INJECTION] createMockExecu
111115

112116
### Handler Requirements
113117

114-
All plugin handlers must support dependency injection:
118+
MCP tool logic functions that orchestrate complex processes must support dependency injection:
115119

116120
```typescript
117121
export function tool_nameLogic(
@@ -134,12 +138,9 @@ export default {
134138
};
135139
```
136140

137-
**Important**: The dependency injection pattern applies to ALL handlers, including:
138-
- Tool handlers
139-
- Resource handlers
140-
- Any future handler types (prompts, etc.)
141+
**Important**: The dependency injection pattern applies to tool and resource handler logic that orchestrates complex, long-running processes (e.g., `xcodebuild`). Standalone utility modules with simple commands may use direct imports and standard vitest mocking.
141142

142-
Always use default parameter values (e.g., `= getDefaultCommandExecutor()`) to ensure production code works without explicit executor injection, while tests can override with mock executors.
143+
Always use default parameter values (e.g., `= getDefaultCommandExecutor()`) in tool logic to ensure production code works without explicit executor injection, while tests can override with mock executors.
143144

144145
### Test Requirements
145146

src/test-utils/test-helpers.ts

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
/**
2+
* Shared test helpers for extracting text content from tool responses.
3+
*/
4+
5+
import { expect } from 'vitest';
6+
import type { ToolHandlerContext, ImageAttachment } from '../rendering/types.ts';
7+
import type { PipelineEvent } from '../types/pipeline-events.ts';
8+
import type { ToolResponse, NextStepParamsMap } from '../types/common.ts';
9+
import type { ToolHandler } from '../utils/typed-tool-factory.ts';
10+
import { renderEvents } from '../rendering/render.ts';
11+
import { createRenderSession } from '../rendering/render.ts';
12+
import { handlerContextStorage } from '../utils/typed-tool-factory.ts';
13+
14+
/**
15+
* Extract and join all text content items from a tool response.
16+
*/
17+
export function allText(result: {
18+
content: ReadonlyArray<{ type: string; text?: string; [key: string]: unknown }>;
19+
}): string {
20+
return result.content
21+
.filter(
22+
(c): c is { type: 'text'; text: string } => c.type === 'text' && typeof c.text === 'string',
23+
)
24+
.map((c) => c.text)
25+
.join('\n');
26+
}
27+
28+
/**
29+
* Assert that a tool response represents a pending xcodebuild result
30+
* with an optional next-step tool reference.
31+
*/
32+
export interface MockToolHandlerResult {
33+
events: PipelineEvent[];
34+
attachments: ImageAttachment[];
35+
nextStepParams?: NextStepParamsMap;
36+
text(): string;
37+
isError(): boolean;
38+
}
39+
40+
export function createMockToolHandlerContext(): {
41+
ctx: ToolHandlerContext;
42+
result: MockToolHandlerResult;
43+
run: <T>(fn: () => Promise<T>) => Promise<T>;
44+
} {
45+
const events: PipelineEvent[] = [];
46+
const attachments: ImageAttachment[] = [];
47+
const ctx: ToolHandlerContext = {
48+
emit: (event) => {
49+
events.push(event);
50+
},
51+
attach: (image) => {
52+
attachments.push(image);
53+
},
54+
};
55+
const resultObj: MockToolHandlerResult = {
56+
events,
57+
attachments,
58+
get nextStepParams() {
59+
return ctx.nextStepParams;
60+
},
61+
text() {
62+
return renderEvents(events, 'text');
63+
},
64+
isError() {
65+
return events.some(
66+
(e) =>
67+
(e.type === 'status-line' && e.level === 'error') ||
68+
(e.type === 'summary' && e.status === 'FAILED'),
69+
);
70+
},
71+
};
72+
return {
73+
ctx,
74+
result: resultObj,
75+
run: async <T>(fn: () => Promise<T>): Promise<T> => {
76+
return handlerContextStorage.run(ctx, fn);
77+
},
78+
};
79+
}
80+
81+
export async function runToolLogic<T>(logic: () => Promise<T>): Promise<{
82+
response: T;
83+
result: MockToolHandlerResult;
84+
}> {
85+
const { result, run } = createMockToolHandlerContext();
86+
const response = await run(logic);
87+
return { response, result };
88+
}
89+
90+
export interface RunLogicResult {
91+
content: Array<{ type: string; text?: string; data?: string; mimeType?: string }>;
92+
isError?: boolean;
93+
nextStepParams?: NextStepParamsMap;
94+
attachments?: ImageAttachment[];
95+
text?: string;
96+
}
97+
98+
/**
99+
* Run a tool's logic function in a mock handler context and return a
100+
* ToolResponse-shaped result for backward-compatible test assertions.
101+
*/
102+
export async function runLogic(logic: () => Promise<unknown>): Promise<RunLogicResult> {
103+
const { result, run } = createMockToolHandlerContext();
104+
const response = await run(logic);
105+
106+
if (
107+
response &&
108+
typeof response === 'object' &&
109+
'content' in (response as Record<string, unknown>)
110+
) {
111+
return response as RunLogicResult;
112+
}
113+
114+
const text = result.text();
115+
const textContent = text.length > 0 ? [{ type: 'text' as const, text }] : [];
116+
const imageContent = result.attachments.map((attachment) => ({
117+
type: 'image' as const,
118+
data: attachment.data,
119+
mimeType: attachment.mimeType,
120+
}));
121+
122+
return {
123+
content: [...textContent, ...imageContent],
124+
isError: result.isError() ? true : undefined,
125+
nextStepParams: result.nextStepParams,
126+
attachments: result.attachments,
127+
text,
128+
};
129+
}
130+
131+
export interface CallHandlerResult {
132+
content: Array<{ type: 'text'; text: string }>;
133+
isError?: boolean;
134+
nextStepParams?: NextStepParamsMap;
135+
}
136+
137+
/**
138+
* Call a tool handler in test mode, providing a session context and
139+
* returning a ToolResponse-shaped result for backward-compatible assertions.
140+
*/
141+
export async function callHandler(
142+
handler:
143+
| ToolHandler
144+
| ((args: Record<string, unknown>, ctx?: ToolHandlerContext) => Promise<void>),
145+
args: Record<string, unknown>,
146+
): Promise<CallHandlerResult> {
147+
const session = createRenderSession('text');
148+
const ctx: ToolHandlerContext = {
149+
emit: (event) => session.emit(event),
150+
attach: (image) => session.attach(image),
151+
};
152+
await handler(args, ctx);
153+
const text = session.finalize();
154+
return {
155+
content: text ? [{ type: 'text' as const, text }] : [],
156+
isError: session.isError() || undefined,
157+
nextStepParams: ctx.nextStepParams,
158+
};
159+
}
160+
161+
function isMockToolHandlerResult(
162+
result: ToolResponse | MockToolHandlerResult,
163+
): result is MockToolHandlerResult {
164+
return 'events' in result && Array.isArray(result.events) && typeof result.text === 'function';
165+
}
166+
167+
export function expectPendingBuildResponse(
168+
result: ToolResponse | MockToolHandlerResult,
169+
nextStepToolId?: string,
170+
): void {
171+
if (isMockToolHandlerResult(result)) {
172+
expect(result.events.some((event) => event.type === 'summary')).toBe(true);
173+
174+
if (nextStepToolId) {
175+
expect(result.nextStepParams).toEqual(
176+
expect.objectContaining({
177+
[nextStepToolId]: expect.any(Object),
178+
}),
179+
);
180+
} else {
181+
expect(result.nextStepParams).toBeUndefined();
182+
}
183+
return;
184+
}
185+
186+
expect(result.content).toEqual([]);
187+
expect(result._meta).toEqual(
188+
expect.objectContaining({
189+
pendingXcodebuild: expect.objectContaining({
190+
kind: 'pending-xcodebuild',
191+
}),
192+
}),
193+
);
194+
195+
if (nextStepToolId) {
196+
expect(result.nextStepParams).toEqual(
197+
expect.objectContaining({
198+
[nextStepToolId]: expect.any(Object),
199+
}),
200+
);
201+
} else {
202+
expect(result.nextStepParams).toBeUndefined();
203+
}
204+
}

src/types/common.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ export interface PlatformBuildOptions {
137137
simulatorId?: string;
138138
deviceId?: string;
139139
useLatestOS?: boolean;
140+
packageCachePath?: string;
140141
arch?: string;
141142
logPrefix: string;
142143
}

0 commit comments

Comments
 (0)