Skip to content

Commit 4ad9363

Browse files
cameroncookecameroncooke
committed
refactor: remove duplicate manual validation across all MCP tools
This commit eliminates redundant manual parameter validation that was duplicating Zod schema validation across 55 tool files in the MCP server. ## Changes Made ### Tool Files Modified (55 total): - Removed validateRequiredParam imports and calls - Preserved domain-specific validation (file existence checks, etc.) - All tools now rely solely on Zod schema validation via createTypedTool ### Test Files Updated: - Updated tests to expect Zod validation error messages - Changed tests to call handlers (with validation) instead of logic functions for parameter validation testing - Preserved all functional test coverage ### Key Improvements: - Eliminated 2,545 lines of redundant validation code - Consistent validation architecture using Zod schemas - Better error messages from Zod validation - Improved maintainability with single source of truth for parameter validation - Preserved all domain-specific business logic validation ### Quality Assurance: - All 1,557 tests passing - TypeScript compilation clean - ESLint validation passed - Build successful This refactoring addresses the critical Cursor Bot review comments from PR #86 by removing the duplicate validation layer while maintaining full functionality and type safety through the established createTypedTool pattern.
1 parent 199a397 commit 4ad9363

117 files changed

Lines changed: 919 additions & 2545 deletions

File tree

Some content is hidden

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

‎src/mcp/tools/device-project/__tests__/build_dev_proj.test.ts‎

Lines changed: 42 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -47,45 +47,58 @@ describe('build_dev_proj plugin', () => {
4747
});
4848
});
4949

50-
describe('Handler Behavior (Complete Literal Returns)', () => {
51-
it('should return exact validation failure response for missing projectPath', async () => {
52-
const result = await build_dev_projLogic(
53-
{
54-
projectPath: null,
55-
scheme: 'MyScheme',
56-
},
57-
createNoopExecutor(),
58-
);
50+
describe('Parameter Validation (via Handler)', () => {
51+
it('should return Zod validation error for missing projectPath', async () => {
52+
const result = await buildDevProj.handler({
53+
scheme: 'MyScheme',
54+
});
5955

60-
expect(result).toEqual({
61-
content: [
62-
{
63-
type: 'text',
64-
text: "Required parameter 'projectPath' is missing. Please provide a value for this parameter.",
65-
},
66-
],
67-
isError: true,
56+
expect(result.isError).toBe(true);
57+
expect(result.content[0].text).toContain('Parameter validation failed');
58+
expect(result.content[0].text).toContain('projectPath');
59+
expect(result.content[0].text).toContain('Required');
60+
});
61+
62+
it('should return Zod validation error for missing scheme', async () => {
63+
const result = await buildDevProj.handler({
64+
projectPath: '/path/to/MyProject.xcodeproj',
65+
});
66+
67+
expect(result.isError).toBe(true);
68+
expect(result.content[0].text).toContain('Parameter validation failed');
69+
expect(result.content[0].text).toContain('scheme');
70+
expect(result.content[0].text).toContain('Required');
71+
});
72+
73+
it('should return Zod validation error for invalid parameter types', async () => {
74+
const result = await buildDevProj.handler({
75+
projectPath: 123, // Should be string
76+
scheme: 'MyScheme',
6877
});
78+
79+
expect(result.isError).toBe(true);
80+
expect(result.content[0].text).toContain('Parameter validation failed');
6981
});
82+
});
83+
84+
describe('Handler Behavior (Complete Literal Returns)', () => {
85+
it('should pass validation and execute successfully with valid parameters', async () => {
86+
const mockExecutor = createMockExecutor({
87+
success: true,
88+
output: 'Build succeeded',
89+
});
7090

71-
it('should return exact validation failure response for missing scheme', async () => {
7291
const result = await build_dev_projLogic(
7392
{
7493
projectPath: '/path/to/MyProject.xcodeproj',
75-
scheme: null,
94+
scheme: 'MyScheme',
7695
},
77-
createNoopExecutor(),
96+
mockExecutor,
7897
);
7998

80-
expect(result).toEqual({
81-
content: [
82-
{
83-
type: 'text',
84-
text: "Required parameter 'scheme' is missing. Please provide a value for this parameter.",
85-
},
86-
],
87-
isError: true,
88-
});
99+
expect(result.isError).toBeUndefined();
100+
expect(result.content).toHaveLength(2);
101+
expect(result.content[0].text).toContain('✅ iOS Device Build build succeeded');
89102
});
90103

91104
it('should verify command generation with mock executor', async () => {

‎src/mcp/tools/device-project/__tests__/get_device_app_path_proj.test.ts‎

Lines changed: 2 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -48,45 +48,8 @@ describe('get_device_app_path_proj plugin', () => {
4848
});
4949

5050
describe('Handler Behavior (Complete Literal Returns)', () => {
51-
it('should return exact validation failure response for missing projectPath', async () => {
52-
const result = await get_device_app_path_projLogic(
53-
{
54-
projectPath: null,
55-
scheme: 'MyScheme',
56-
},
57-
createMockExecutor({ success: true }),
58-
);
59-
60-
expect(result).toEqual({
61-
content: [
62-
{
63-
type: 'text',
64-
text: "Required parameter 'projectPath' is missing. Please provide a value for this parameter.",
65-
},
66-
],
67-
isError: true,
68-
});
69-
});
70-
71-
it('should return exact validation failure response for missing scheme', async () => {
72-
const result = await get_device_app_path_projLogic(
73-
{
74-
projectPath: '/path/to/project.xcodeproj',
75-
scheme: null,
76-
},
77-
createMockExecutor({ success: true }),
78-
);
79-
80-
expect(result).toEqual({
81-
content: [
82-
{
83-
type: 'text',
84-
text: "Required parameter 'scheme' is missing. Please provide a value for this parameter.",
85-
},
86-
],
87-
isError: true,
88-
});
89-
});
51+
// Note: Parameter validation is now handled by Zod schema validation in createTypedTool,
52+
// so invalid parameters never reach the logic function. Schema validation is tested above.
9053

9154
it('should generate correct xcodebuild command for iOS', async () => {
9255
const calls: Array<{

‎src/mcp/tools/device-project/build_dev_proj.ts‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77

88
import { z } from 'zod';
99
import { ToolResponse, XcodePlatform } from '../../../types/common.js';
10-
import { validateRequiredParam } from '../../../utils/index.js';
1110
import { executeXcodeBuildCommand } from '../../../utils/index.js';
1211
import { CommandExecutor, getDefaultCommandExecutor } from '../../../utils/command.js';
1312
import { createTypedTool } from '../../../utils/typed-tool-factory.js';
@@ -32,12 +31,6 @@ export async function build_dev_projLogic(
3231
params: BuildDevProjParams,
3332
executor: CommandExecutor,
3433
): Promise<ToolResponse> {
35-
const projectValidation = validateRequiredParam('projectPath', params.projectPath);
36-
if (!projectValidation.isValid) return projectValidation.errorResponse!;
37-
38-
const schemeValidation = validateRequiredParam('scheme', params.scheme);
39-
if (!schemeValidation.isValid) return schemeValidation.errorResponse!;
40-
4134
const processedParams = {
4235
...params,
4336
configuration: params.configuration ?? 'Debug', // Default config

‎src/mcp/tools/device-project/get_device_app_path_proj.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import { z } from 'zod';
99
import { ToolResponse } from '../../../types/common.js';
1010
import { log } from '../../../utils/index.js';
11-
import { validateRequiredParam, createTextResponse } from '../../../utils/index.js';
11+
import { createTextResponse } from '../../../utils/index.js';
1212
import { CommandExecutor, getDefaultCommandExecutor } from '../../../utils/index.js';
1313
import { createTypedTool } from '../../../utils/typed-tool-factory.js';
1414

@@ -42,12 +42,6 @@ export async function get_device_app_path_projLogic(
4242
params: GetDeviceAppPathProjParams,
4343
executor: CommandExecutor,
4444
): Promise<ToolResponse> {
45-
const projectValidation = validateRequiredParam('projectPath', params.projectPath);
46-
if (!projectValidation.isValid) return projectValidation.errorResponse!;
47-
48-
const schemeValidation = validateRequiredParam('scheme', params.scheme);
49-
if (!schemeValidation.isValid) return schemeValidation.errorResponse!;
50-
5145
const platformMap = {
5246
iOS: XcodePlatform.iOS,
5347
watchOS: XcodePlatform.watchOS,

‎src/mcp/tools/device-workspace/__tests__/build_dev_ws.test.ts‎

Lines changed: 14 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -46,41 +46,25 @@ describe('build_dev_ws plugin', () => {
4646

4747
describe('Handler Behavior (Complete Literal Returns)', () => {
4848
it('should return exact validation error response for workspacePath', async () => {
49-
const result = await build_dev_wsLogic(
50-
{
51-
scheme: 'MyScheme',
52-
},
53-
createNoopExecutor(),
54-
);
55-
56-
expect(result).toEqual({
57-
content: [
58-
{
59-
type: 'text',
60-
text: "Required parameter 'workspacePath' is missing. Please provide a value for this parameter.",
61-
},
62-
],
63-
isError: true,
49+
const result = await buildDevWs.handler({
50+
scheme: 'MyScheme',
6451
});
52+
53+
expect(result.isError).toBe(true);
54+
expect(result.content[0].text).toContain('Parameter validation failed');
55+
expect(result.content[0].text).toContain('workspacePath');
56+
expect(result.content[0].text).toContain('Required');
6557
});
6658

6759
it('should return exact validation error response for scheme', async () => {
68-
const result = await build_dev_wsLogic(
69-
{
70-
workspacePath: '/path/to/workspace.xcworkspace',
71-
},
72-
createNoopExecutor(),
73-
);
74-
75-
expect(result).toEqual({
76-
content: [
77-
{
78-
type: 'text',
79-
text: "Required parameter 'scheme' is missing. Please provide a value for this parameter.",
80-
},
81-
],
82-
isError: true,
60+
const result = await buildDevWs.handler({
61+
workspacePath: '/path/to/workspace.xcworkspace',
8362
});
63+
64+
expect(result.isError).toBe(true);
65+
expect(result.content[0].text).toContain('Parameter validation failed');
66+
expect(result.content[0].text).toContain('scheme');
67+
expect(result.content[0].text).toContain('Required');
8468
});
8569

8670
it('should generate correct xcodebuild command for workspace', async () => {

‎src/mcp/tools/device-workspace/__tests__/get_device_app_path_ws.test.ts‎

Lines changed: 4 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -43,43 +43,11 @@ describe('get_device_app_path_ws plugin', () => {
4343
});
4444

4545
describe('Handler Behavior (Complete Literal Returns)', () => {
46-
it('should return exact validation error response for workspacePath', async () => {
47-
const result = await get_device_app_path_wsLogic(
48-
{
49-
scheme: 'MyScheme',
50-
},
51-
createNoopExecutor(),
52-
);
53-
54-
expect(result).toEqual({
55-
content: [
56-
{
57-
type: 'text',
58-
text: "Required parameter 'workspacePath' is missing. Please provide a value for this parameter.",
59-
},
60-
],
61-
isError: true,
62-
});
63-
});
46+
// Note: workspacePath validation is now handled by Zod schema in createTypedTool wrapper
47+
// The logic function expects valid parameters that have passed Zod validation
6448

65-
it('should return exact validation error response for scheme', async () => {
66-
const result = await get_device_app_path_wsLogic(
67-
{
68-
workspacePath: '/path/to/workspace.xcworkspace',
69-
},
70-
createNoopExecutor(),
71-
);
72-
73-
expect(result).toEqual({
74-
content: [
75-
{
76-
type: 'text',
77-
text: "Required parameter 'scheme' is missing. Please provide a value for this parameter.",
78-
},
79-
],
80-
isError: true,
81-
});
82-
});
49+
// Note: scheme validation is now handled by Zod schema in createTypedTool wrapper
50+
// The logic function expects valid parameters that have passed Zod validation
8351

8452
it('should generate correct xcodebuild command for getting build settings', async () => {
8553
const calls: any[] = [];

‎src/mcp/tools/device-workspace/build_dev_ws.ts‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77

88
import { z } from 'zod';
99
import { ToolResponse, XcodePlatform } from '../../../types/common.js';
10-
import { validateRequiredParam } from '../../../utils/index.js';
1110
import { executeXcodeBuildCommand } from '../../../utils/index.js';
1211
import { CommandExecutor, getDefaultCommandExecutor } from '../../../utils/command.js';
1312
import { createTypedTool } from '../../../utils/typed-tool-factory.js';
@@ -29,14 +28,8 @@ export async function build_dev_wsLogic(
2928
params: BuildDevWsParams,
3029
executor: CommandExecutor,
3130
): Promise<ToolResponse> {
32-
// No casting needed! Parameters are guaranteed valid by schema validation
33-
// Direct validation using typed parameters
34-
const workspaceValidation = validateRequiredParam('workspacePath', params.workspacePath);
35-
if (!workspaceValidation.isValid) return workspaceValidation.errorResponse!;
36-
37-
const schemeValidation = validateRequiredParam('scheme', params.scheme);
38-
if (!schemeValidation.isValid) return schemeValidation.errorResponse!;
39-
31+
// Parameters are guaranteed valid by Zod schema validation in createTypedTool
32+
// No manual validation needed for required parameters
4033
return executeXcodeBuildCommand(
4134
{
4235
...params,

‎src/mcp/tools/device-workspace/get_device_app_path_ws.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import { z } from 'zod';
99
import { ToolResponse } from '../../../types/common.js';
1010
import { log } from '../../../utils/index.js';
11-
import { validateRequiredParam, createTextResponse } from '../../../utils/index.js';
11+
import { createTextResponse } from '../../../utils/index.js';
1212
import { CommandExecutor, getDefaultCommandExecutor } from '../../../utils/index.js';
1313
import { createTypedTool } from '../../../utils/typed-tool-factory.js';
1414

@@ -42,12 +42,6 @@ export async function get_device_app_path_wsLogic(
4242
params: GetDeviceAppPathWsParams,
4343
executor: CommandExecutor,
4444
): Promise<ToolResponse> {
45-
const workspaceValidation = validateRequiredParam('workspacePath', params.workspacePath);
46-
if (!workspaceValidation.isValid) return workspaceValidation.errorResponse!;
47-
48-
const schemeValidation = validateRequiredParam('scheme', params.scheme);
49-
if (!schemeValidation.isValid) return schemeValidation.errorResponse!;
50-
5145
const platformMap = {
5246
iOS: XcodePlatform.iOS,
5347
watchOS: XcodePlatform.watchOS,

‎src/mcp/tools/logging/__tests__/start_sim_log_cap.test.ts‎

Lines changed: 2 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -82,49 +82,8 @@ describe('start_sim_log_cap plugin', () => {
8282
});
8383

8484
describe('Handler Behavior (Complete Literal Returns)', () => {
85-
it('should return error for missing simulatorUuid', async () => {
86-
const mockExecutor = createMockExecutor({ success: true, output: '' });
87-
const result = await start_sim_log_capLogic(
88-
{ bundleId: 'com.example.app' } as any,
89-
mockExecutor,
90-
);
91-
92-
expect(result).toEqual({
93-
content: [
94-
{
95-
type: 'text',
96-
text: "Required parameter 'simulatorUuid' is missing. Please provide a value for this parameter.",
97-
},
98-
],
99-
isError: true,
100-
});
101-
});
102-
103-
it('should handle null bundleId parameter', async () => {
104-
const mockExecutor = createMockExecutor({ success: true, output: '' });
105-
const logCaptureStub = (params: any, executor: any) => {
106-
return Promise.resolve({
107-
sessionId: 'test-uuid-123',
108-
logFilePath: '/tmp/test.log',
109-
processes: [],
110-
error: undefined,
111-
});
112-
};
113-
114-
const result = await start_sim_log_capLogic(
115-
{
116-
simulatorUuid: 'test-uuid',
117-
bundleId: null,
118-
} as any,
119-
mockExecutor,
120-
logCaptureStub,
121-
);
122-
123-
expect(result.isError).toBeUndefined();
124-
expect(result.content[0].text).toBe(
125-
"Log capture started successfully. Session ID: test-uuid-123.\n\nNote: Only structured logs are being captured.\n\nNext Steps:\n1. Interact with your simulator and app.\n2. Use 'stop_sim_log_cap' with session ID 'test-uuid-123' to stop capture and retrieve logs.",
126-
);
127-
});
85+
// Note: Parameter validation is now handled by createTypedTool wrapper
86+
// Invalid parameters will not reach the logic function, so we test valid scenarios
12887

12988
it('should return error when log capture fails', async () => {
13089
const mockExecutor = createMockExecutor({ success: true, output: '' });

0 commit comments

Comments
 (0)