Skip to content

Commit a748d44

Browse files
cameroncookeclaude
andcommitted
fix(runtime): Merge dynamic next-step params into templates
Apply dynamic next-step params even when manifest templates already define\nparams so scaffold follow-up actions resolve placeholders correctly.\n\nAlign testing docs and pattern checks with the canonical policy:\nuse executor/filesystem DI for external boundaries and allow Vitest\nmocking for internal collaborators. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 01a55e9 commit a748d44

8 files changed

Lines changed: 124 additions & 183 deletions

File tree

docs/dev/ARCHITECTURE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,7 @@ Not all parts are required for every tool. For example, `swift_package_build` ha
418418

419419
### Testing Principles
420420

421-
XcodeBuildMCP uses a strict **Dependency Injection (DI)** pattern for testing, which completely bans the use of traditional mocking libraries like Vitest's `vi.mock` or `vi.fn`. This ensures that tests are robust, maintainable, and verify the actual integration between components.
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.
422422

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

docs/dev/CODE_QUALITY.md

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,9 @@ XcodeBuildMCP enforces several architectural patterns that cannot be expressed t
6161
- Logic functions accepting `executor?: CommandExecutor` parameter
6262

6363
**Forbidden**:
64-
- Direct use of `vi.mock()`, `vi.fn()`, or any Vitest mocking
6564
- Direct calls to `execSync`, `spawn`, or `exec` in production code
6665
- Testing handler functions directly
66+
- Real external side effects in unit tests (xcodebuild/xcrun/filesystem writes outside mocks)
6767

6868
### 2. Handler Signature Compliance
6969

@@ -145,7 +145,7 @@ The pattern checker enforces XcodeBuildMCP-specific architectural rules:
145145
node scripts/check-code-patterns.js
146146

147147
# Check specific pattern type
148-
node scripts/check-code-patterns.js --pattern=vitest
148+
node scripts/check-code-patterns.js
149149
node scripts/check-code-patterns.js --pattern=execsync
150150
node scripts/check-code-patterns.js --pattern=handler
151151
node scripts/check-code-patterns.js --pattern=handler-testing
@@ -172,12 +172,11 @@ npm run tools:all
172172

173173
The pattern checker identifies the following violations:
174174

175-
### 1. Vitest Mocking Violations
176-
177-
**What**: Any use of Vitest mocking functions
178-
**Why**: Breaks dependency injection architecture
179-
**Fix**: Use `createMockExecutor()` instead
175+
### 1. External-Boundary Mocking Violations
180176

177+
**What**: Tests that mock external side effects without injected executors/filesystem dependencies
178+
**Why**: Breaks deterministic external-boundary testing
179+
**Fix**: Use `createMockExecutor()` / `createMockFileSystemExecutor()` for external dependencies
181180
### 2. ExecSync Violations
182181

183182
**What**: Direct use of Node.js child_process functions in production code
@@ -251,7 +250,7 @@ node scripts/check-code-patterns.js # Check architectural compliance
251250
### 3. Writing Tests
252251

253252
1. Import the logic function, not the default export
254-
2. Use `createMockExecutor()` for mocking
253+
2. Use `createMockExecutor()` / `createMockFileSystemExecutor()` for external side effects
255254
3. Test three dimensions: validation, command generation, output processing
256255
4. Never test handlers directly
257256

docs/dev/CONTRIBUTING.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -270,8 +270,8 @@ 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-
- **No Vitest Mocking**: All forms of `vi.mock`, `vi.fn`, `vi.spyOn`, etc., are strictly forbidden.
274-
- **Dependency Injection**: 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 External Boundaries**: All external dependencies (command execution, file system access) must be injected into tool logic functions using the `CommandExecutor` and `FileSystemExecutor` patterns.
274+
- **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.
277277

docs/dev/TESTING.md

Lines changed: 32 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -18,53 +18,40 @@ This document provides comprehensive testing guidelines for XcodeBuildMCP plugin
1818

1919
## Testing Philosophy
2020

21-
### 🚨 CRITICAL: No Vitest Mocking Allowed
22-
23-
### ABSOLUTE RULE: ALL VITEST MOCKING IS COMPLETELY BANNED
24-
25-
### FORBIDDEN PATTERNS (will cause immediate test failure):
26-
27-
#### Vitest Mocking (COMPLETELY BANNED):
28-
- `vi.mock()` - BANNED
29-
- `vi.fn()` - BANNED
30-
- `vi.mocked()` - BANNED
31-
- `vi.spyOn()` - BANNED
32-
- `.mockResolvedValue()` - BANNED
33-
- `.mockRejectedValue()` - BANNED
34-
- `.mockReturnValue()` - BANNED
35-
- `.mockImplementation()` - BANNED
36-
- `.toHaveBeenCalled()` - BANNED
37-
- `.toHaveBeenCalledWith()` - BANNED
38-
- `MockedFunction` type - BANNED
39-
40-
#### Manual Mock Implementations (BANNED - use our utilities instead):
41-
- `const mockExecutor = async (...) => { ... }` - Use `createMockExecutor()` instead
42-
- `const mockFsDeps = { readFile: async () => ... }` - Use `createMockFileSystemExecutor()` instead
43-
- `const mockServer = { ... }` - Refactor to use dependency injection pattern
44-
- Any manual async function implementations for mocking behavior
45-
46-
### ONLY ALLOWED MOCKING:
47-
- `createMockExecutor({ success: true, output: 'result' })` - command execution
48-
- `createMockFileSystemExecutor({ readFile: async () => 'content' })` - file system operations
21+
### 🚨 CRITICAL: External Dependency Mocking Rules
22+
23+
### ABSOLUTE RULE: External side effects must use dependency injection utilities
24+
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
28+
29+
### Internal mocking guidance:
30+
- Vitest mocking (`vi.fn`, `vi.mock`, `vi.spyOn`, `.mockResolvedValue`, etc.) is allowed for internal modules and in-memory collaborators
31+
- Prefer straightforward, readable test doubles over over-engineered mocks
32+
33+
### Still forbidden:
34+
- Hitting real external systems in unit tests (real `xcodebuild`, `xcrun`, AXe, filesystem writes/reads outside test harness)
35+
- Bypassing dependency injection for external effects
4936

5037
### OUR CORE PRINCIPLE
5138

52-
**Simple Rule**: No mocking other than `createMockExecutor()` and `createMockFileSystemExecutor()` (and their noop variants).
39+
**Simple Rule**: Use dependency-injection mock executors for external boundaries; use Vitest mocking only for internal behavior.
5340

5441
**Why This Rule Exists**:
55-
1. **Consistency**: All tests use the same mocking utilities, making them predictable and maintainable
56-
2. **Reliability**: Our utilities are thoroughly tested and handle edge cases properly
57-
3. **Architectural Enforcement**: Prevents bypassing our dependency injection patterns
58-
4. **Simplicity**: One clear rule instead of complex guidelines about what mocking is acceptable
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
45+
4. **Maintainability**: Tests fail for behavior regressions, not incidental environment differences
5946

6047
### Integration Testing with Dependency Injection
6148

62-
XcodeBuildMCP follows a **pure dependency injection** testing philosophy that eliminates vitest mocking:
49+
XcodeBuildMCP follows a dependency-injection testing philosophy for external boundaries:
6350

6451
-**Test plugin interfaces** (public API contracts)
6552
-**Test integration flows** (plugin → utilities → external tools)
66-
-**Use dependency injection** with createMockExecutor()
67-
- **Never mock vitest functions** (vi.mock, vi.fn, etc.)
53+
-**Use dependency injection** with createMockExecutor()/createMockFileSystemExecutor() for external dependencies
54+
- **Use Vitest mocking when needed** for internal modules and collaborators
6855

6956
### Benefits
7057

@@ -76,7 +63,7 @@ XcodeBuildMCP follows a **pure dependency injection** testing philosophy that el
7663

7764
### Automated Violation Checking
7865

79-
To enforce the no-mocking policy, the project includes a script that automatically checks for banned testing patterns.
66+
To enforce external-boundary testing policy, the project includes a script that checks for architectural test-pattern violations.
8067

8168
```bash
8269
# Run the script to check for violations
@@ -91,7 +78,7 @@ This script is part of the standard development workflow and should be run befor
9178
- Manual mock executors: `const mockExecutor = async (...) => { ... }`
9279
- Manual filesystem mocks: `const mockFsDeps = { readFile: async () => ... }`
9380
- Manual server mocks: `const mockServer = { ... }`
94-
- Vitest mocking patterns: `vi.mock()`, `vi.fn()`, etc.
81+
- External side-effect patterns that bypass injected executors/filesystem dependencies
9582

9683
#### ❌ FALSE POSITIVES (should NOT be flagged):
9784
- Test data tracking: `commandCalls.push({ ... })` - This is just collecting test data, not mocking behavior
@@ -118,7 +105,7 @@ Test → Plugin Handler → utilities → [DEPENDENCY INJECTION] createMockExecu
118105
### What Gets Mocked
119106
- Command execution via `createMockExecutor()`
120107
- File system operations via `createMockFileSystemExecutor()`
121-
- Nothing else - all vitest mocking is banned
108+
- Internal modules can use Vitest mocks where appropriate
122109

123110
## Dependency Injection Strategy
124111

@@ -359,8 +346,8 @@ describe('simulator-project re-exports', () => {
359346
import { vi, describe, it, expect, beforeEach } from 'vitest';
360347
import { z } from 'zod';
361348

362-
// CRITICAL: NO VITEST MOCKING ALLOWED
363-
// Import ONLY what you need - no mock setup
349+
// Use dependency-injection mocks for external boundaries.
350+
// Vitest mocks are acceptable for internal collaborators when needed.
364351

365352
import tool from '../tool_name.ts';
366353
import { createMockExecutor } from '../../utils/command.js';
@@ -1266,8 +1253,8 @@ npm run test:coverage -- src/plugins/simulator-workspace/
12661253
### Validation Scripts
12671254

12681255
```bash
1269-
# Check for vitest mocking violations
1270-
node scripts/check-code-patterns.js --pattern=vitest
1256+
# Check for architectural pattern violations
1257+
node scripts/check-code-patterns.js
12711258

12721259
# Check dependency injection compliance
12731260
node scripts/audit-dependency-container.js
@@ -1278,12 +1265,12 @@ node scripts/audit-dependency-container.js
12781265
## Best Practices Summary
12791266

12801267
1. **Dependency injection**: Always use createMockExecutor() and createMockFileSystemExecutor()
1281-
2. **No vitest mocking**: All vi.mock, vi.fn, etc. patterns are banned
1268+
2. **External boundaries via DI**: mock command execution/filesystem with injected executors
12821269
3. **Three dimensions**: Test input validation, command execution, and output processing
12831270
4. **Literal expectations**: Use exact strings in assertions to catch regressions
12841271
5. **Performance**: Ensure fast execution through proper mocking
12851272
6. **Coverage**: Aim for 95%+ with focus on error paths
12861273
7. **Consistency**: Follow standard patterns across all plugin tests
12871274
8. **Test safety**: Default executors prevent accidental real system calls
12881275

1289-
This testing strategy ensures robust, maintainable tests that provide confidence in plugin functionality while remaining resilient to implementation changes and completely eliminating vitest mocking dependencies.
1276+
This testing strategy ensures robust, maintainable tests that provide confidence in plugin functionality while remaining resilient to implementation changes and keeping external boundaries deterministic.

scripts/check-code-patterns.js

Lines changed: 15 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,10 @@
1111
* node scripts/check-code-patterns.js --help
1212
*
1313
* ARCHITECTURAL RULES ENFORCED:
14-
* 1. NO vitest mocking patterns (vi.mock, vi.fn, .mockResolvedValue, etc.)
14+
* 1. External boundaries in tests should use dependency-injection utilities
1515
* 2. NO execSync usage in production code (use CommandExecutor instead)
16-
* 3. ONLY dependency injection with createMockExecutor() and createMockFileSystemExecutor()
17-
* 4. NO handler signature violations (handlers must have exact MCP SDK signatures)
18-
* 5. NO handler testing violations (test logic functions, not handlers directly)
16+
* 3. NO handler signature violations (handlers must have exact MCP SDK signatures)
17+
* 4. NO handler testing violations (test logic functions, not handlers directly)
1918
*
2019
* For comprehensive code quality documentation, see docs/dev/CODE_QUALITY.md
2120
*
@@ -49,7 +48,7 @@ OPTIONS:
4948
--help, -h Show this help message
5049
5150
PATTERN TYPES:
52-
vitest Check only vitest mocking violations (vi.mock, vi.fn, etc.)
51+
vitest Check only custom vitest policy violations (currently none)
5352
execsync Check only execSync usage in production code
5453
handler Check only handler signature violations
5554
handler-testing Check only handler testing violations (testing handlers instead of logic functions)
@@ -77,28 +76,9 @@ const EXECSYNC_PATTERNS = [
7776
/^import\s+{[^}]*(?:exec|spawn|execSync)[^}]*}\s+from\s+['"](?:node:)?child_process['"]/m, // Named imports of functions
7877
];
7978

80-
// CRITICAL: ALL VITEST MOCKING PATTERNS ARE COMPLETELY FORBIDDEN
81-
// ONLY dependency injection with approved mock utilities is allowed
82-
const VITEST_GENERIC_PATTERNS = [
83-
/vi\.mock\s*\(/, // vi.mock() - BANNED
84-
/vi\.fn\s*\(/, // vi.fn() - BANNED
85-
/vi\.mocked\s*\(/, // vi.mocked() - BANNED
86-
/vi\.spyOn\s*\(/, // vi.spyOn() - BANNED
87-
/vi\.clearAllMocks\s*\(/, // vi.clearAllMocks() - BANNED
88-
/\.mockResolvedValue/, // .mockResolvedValue - BANNED
89-
/\.mockRejectedValue/, // .mockRejectedValue - BANNED
90-
/\.mockReturnValue/, // .mockReturnValue - BANNED
91-
/\.mockImplementation/, // .mockImplementation - BANNED
92-
/\.mockClear/, // .mockClear - BANNED
93-
/\.mockReset/, // .mockReset - BANNED
94-
/\.mockRestore/, // .mockRestore - BANNED
95-
/\.toHaveBeenCalled/, // .toHaveBeenCalled - BANNED
96-
/\.toHaveBeenCalledWith/, // .toHaveBeenCalledWith - BANNED
97-
/MockedFunction/, // MockedFunction type - BANNED
98-
/as MockedFunction/, // Type casting to MockedFunction - BANNED
99-
/\bexecSync\b/, // execSync usage - BANNED (use executeCommand instead)
100-
/\bexecSyncFn\b/, // execSyncFn usage - BANNED (use executeCommand instead)
101-
];
79+
// Vitest mocking is allowed for internal collaborators.
80+
// Keep this list empty unless a specific project policy requires certain vitest patterns to be blocked.
81+
const VITEST_GENERIC_PATTERNS = [];
10282

10383
// APPROVED mock utilities - ONLY these are allowed
10484
const APPROVED_MOCK_PATTERNS = [
@@ -110,14 +90,8 @@ const APPROVED_MOCK_PATTERNS = [
11090
/\bcreateMockEnvironmentDetector\b/,
11191
];
11292

113-
// REFINED PATTERNS - Only flag ACTUAL vitest violations, not approved dependency injection patterns
114-
// Manual executors and mock objects are APPROVED when used for dependency injection
115-
const UNAPPROVED_MOCK_PATTERNS = [
116-
// ONLY ACTUAL VITEST PATTERNS (vi.* usage) - Everything else is approved
117-
/\bmock[A-Z][a-zA-Z0-9]*\s*=\s*vi\./, // mockSomething = vi.fn() - vitest assignments only
118-
119-
// No other patterns - manual executors and mock objects are approved for dependency injection
120-
];
93+
// Custom vitest restrictions can be added here if needed.
94+
const UNAPPROVED_MOCK_PATTERNS = [];
12195

12296
// Function to check if a line contains unapproved mock patterns
12397
function hasUnapprovedMockPattern(line) {
@@ -615,8 +589,8 @@ function main() {
615589
console.log('🔍 XcodeBuildMCP Code Pattern Violations Checker\n');
616590
console.log(`🎯 Checking pattern type: ${patternFilter.toUpperCase()}\n`);
617591
console.log('CODE GUIDELINES ENFORCED:');
618-
console.log('✅ ONLY ALLOWED: createMockExecutor() and createMockFileSystemExecutor()');
619-
console.log('❌ BANNED: vitest mocking patterns (vi.mock, vi.fn, .mockResolvedValue, etc.)');
592+
console.log('✅ External boundaries in tests should use createMockExecutor()/createMockFileSystemExecutor()');
593+
console.log('✅ Vitest mocking is allowed for internal collaborators');
620594
console.log('❌ BANNED: execSync usage in production code (use CommandExecutor instead)');
621595
console.log('ℹ️ TypeScript patterns: Handled by ESLint with proper test exceptions');
622596
console.log(
@@ -898,12 +872,11 @@ function main() {
898872
if (needsConversion.length > 0) {
899873
console.log(`🚨 CRITICAL ACTION REQUIRED (TEST FILES):`);
900874
console.log(`=======================================`);
901-
console.log(`1. IMMEDIATELY remove ALL vitest mocking from ${needsConversion.length} files`);
902-
console.log(`2. BANNED: vi.mock(), vi.fn(), .mockResolvedValue(), .toHaveBeenCalled(), etc.`);
875+
console.log(`1. Fix architectural violations in ${needsConversion.length} files`);
903876
console.log(
904-
`3. BANNED: Testing handlers directly (.handler()) - test logic functions with dependency injection`,
877+
`2. BANNED: Testing handlers directly (.handler()) - test logic functions with dependency injection`,
905878
);
906-
console.log(`4. ONLY ALLOWED: createMockExecutor() and createMockFileSystemExecutor()`);
879+
console.log(`3. Use injected executors/filesystem mocks for external side effects`);
907880
console.log(`4. Update plugin implementations to accept executor?: CommandExecutor parameter`);
908881
console.log(`5. Run this script again after each fix to track progress`);
909882
console.log('');
@@ -974,7 +947,7 @@ function main() {
974947
if (!hasViolations && mixed.length === 0) {
975948
console.log(`🎉 ALL FILES COMPLY WITH PROJECT STANDARDS!`);
976949
console.log(`==========================================`);
977-
console.log(`✅ All files use ONLY createMockExecutor() and createMockFileSystemExecutor()`);
950+
console.log(`✅ External boundary tests use injected executors/filesystem dependencies`);
978951
console.log(`✅ All files follow TypeScript best practices (no unsafe casts)`);
979952
console.log(`✅ All handler signatures comply with MCP SDK requirements`);
980953
console.log(`✅ All utilities properly use CommandExecutor dependency injection`);

0 commit comments

Comments
 (0)