diff --git a/CLAUDE.md b/CLAUDE.md index 6ad3701..bc6247e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -180,6 +180,9 @@ The MCP server exposes tools in several categories: - Sub-agents are not allowed to spawn further sub-agents - When you use sub-agents, do not allow them to commit and push. That should be done by you +### Development Best Practices +- Run typecheck and lint after every code change + # important-instruction-reminders Do what has been asked; nothing more, nothing less. NEVER create files unless they're absolutely necessary for achieving your goal. diff --git a/data/nodes.db b/data/nodes.db index 77d1425..bdd9a1d 100644 Binary files a/data/nodes.db and b/data/nodes.db differ diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index b3b7a2f..f67a214 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -7,6 +7,51 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [2.10.3] - 2025-08-07 + +### Fixed +- **Validation System Robustness**: Fixed multiple critical validation issues affecting AI agents and workflow validation (fixes #58, #68, #70, #73) + - **Issue #73**: Fixed `validate_node_minimal` crash when config is undefined + - Added safe property access with optional chaining (`config?.resource`) + - Tool now handles undefined, null, and malformed configs gracefully + - **Issue #58**: Fixed `validate_node_operation` crash on invalid nodeType + - Added type checking before calling string methods + - Prevents "Cannot read properties of undefined (reading 'replace')" error + - **Issue #70**: Fixed validation profile settings being ignored + - Extended profile parameter to all validation phases (nodes, connections, expressions) + - Added Sticky Notes filtering to reduce false positives + - Enhanced cycle detection to allow legitimate loops (SplitInBatches) + - **Issue #68**: Added error recovery suggestions for AI agents + - New `addErrorRecoverySuggestions()` method provides actionable recovery steps + - Categorizes errors and suggests specific fixes for each type + - Helps AI agents self-correct when validation fails + +### Added +- **Input Validation System**: Comprehensive validation for all MCP tool inputs + - Created `validation-schemas.ts` with custom validation utilities + - No external dependencies - pure TypeScript implementation + - Tool-specific validation schemas for all MCP tools + - Clear error messages with field-level details +- **Enhanced Cycle Detection**: Improved detection of legitimate loops vs actual cycles + - Recognizes SplitInBatches loop patterns as valid + - Reduces false positive cycle warnings +- **Comprehensive Test Suite**: Added 16 tests covering all validation fixes + - Tests for crash prevention with malformed inputs + - Tests for profile behavior across validation phases + - Tests for error recovery suggestions + - Tests for legitimate loop patterns + +### Enhanced +- **Validation Profiles**: Now consistently applied across all validation phases + - `minimal`: Reduces warnings for basic validation + - `runtime`: Standard validation for production workflows + - `ai-friendly`: Optimized for AI agent workflow creation + - `strict`: Maximum validation for critical workflows +- **Error Messages**: More helpful and actionable for both humans and AI agents + - Specific recovery suggestions for common errors + - Clear guidance on fixing validation issues + - Examples of correct configurations + ## [2.10.2] - 2025-08-05 ### Updated diff --git a/package.json b/package.json index 69e5208..30215b9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "n8n-mcp", - "version": "2.10.2", + "version": "2.10.3", "description": "Integration between n8n workflow automation and Model Context Protocol (MCP)", "main": "dist/index.js", "bin": { diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 321ff6f..7ce3b25 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -28,6 +28,7 @@ import { handleUpdatePartialWorkflow } from './handlers-workflow-diff'; import { getToolDocumentation, getToolsOverview } from './tools-documentation'; import { PROJECT_VERSION } from '../utils/version'; import { normalizeNodeType, getNodeTypeAlternatives, getWorkflowNodeType } from '../utils/node-utils'; +import { ToolValidation, Validator, ValidationError } from '../utils/validation-schemas'; import { negotiateProtocolVersion, logProtocolNegotiation, @@ -460,9 +461,77 @@ export class N8NDocumentationMCPServer { } /** - * Validate required parameters for tool execution + * Enhanced parameter validation using schemas */ - private validateToolParams(toolName: string, args: any, requiredParams: string[]): void { + private validateToolParams(toolName: string, args: any, legacyRequiredParams?: string[]): void { + try { + // If legacy required params are provided, use the new validation but fall back to basic if needed + let validationResult; + + switch (toolName) { + case 'validate_node_operation': + validationResult = ToolValidation.validateNodeOperation(args); + break; + case 'validate_node_minimal': + validationResult = ToolValidation.validateNodeMinimal(args); + break; + case 'validate_workflow': + case 'validate_workflow_connections': + case 'validate_workflow_expressions': + validationResult = ToolValidation.validateWorkflow(args); + break; + case 'search_nodes': + validationResult = ToolValidation.validateSearchNodes(args); + break; + case 'list_node_templates': + validationResult = ToolValidation.validateListNodeTemplates(args); + break; + case 'n8n_create_workflow': + validationResult = ToolValidation.validateCreateWorkflow(args); + break; + case 'n8n_get_workflow': + case 'n8n_get_workflow_details': + case 'n8n_get_workflow_structure': + case 'n8n_get_workflow_minimal': + case 'n8n_update_full_workflow': + case 'n8n_delete_workflow': + case 'n8n_validate_workflow': + case 'n8n_get_execution': + case 'n8n_delete_execution': + validationResult = ToolValidation.validateWorkflowId(args); + break; + default: + // For tools not yet migrated to schema validation, use basic validation + return this.validateToolParamsBasic(toolName, args, legacyRequiredParams || []); + } + + if (!validationResult.valid) { + const errorMessage = Validator.formatErrors(validationResult, toolName); + logger.error(`Parameter validation failed for ${toolName}:`, errorMessage); + throw new ValidationError(errorMessage); + } + } catch (error) { + // Handle validation errors properly + if (error instanceof ValidationError) { + throw error; // Re-throw validation errors as-is + } + + // Handle unexpected errors from validation system + logger.error(`Validation system error for ${toolName}:`, error); + + // Provide a user-friendly error message + const errorMessage = error instanceof Error + ? `Internal validation error: ${error.message}` + : `Internal validation error while processing ${toolName}`; + + throw new Error(errorMessage); + } + } + + /** + * Legacy parameter validation (fallback) + */ + private validateToolParamsBasic(toolName: string, args: any, requiredParams: string[]): void { const missing: string[] = []; for (const param of requiredParams) { @@ -619,12 +688,17 @@ export class N8NDocumentationMCPServer { fix: 'Provide config as an object with node properties' }], warnings: [], - suggestions: [], + suggestions: [ + '🔧 RECOVERY: Invalid config detected. Fix with:', + ' • Ensure config is an object: { "resource": "...", "operation": "..." }', + ' • Use get_node_essentials to see required fields for this node type', + ' • Check if the node type is correct before configuring it' + ], summary: { hasErrors: true, errorCount: 1, warningCount: 0, - suggestionCount: 0 + suggestionCount: 3 } }; } @@ -638,7 +712,10 @@ export class N8NDocumentationMCPServer { nodeType: args.nodeType || 'unknown', displayName: 'Unknown Node', valid: false, - missingRequiredFields: ['Invalid config format - expected object'] + missingRequiredFields: [ + 'Invalid config format - expected object', + '🔧 RECOVERY: Use format { "resource": "...", "operation": "..." } or {} for empty config' + ] }; } return this.validateNodeMinimal(args.nodeType, args.config); @@ -2141,12 +2218,12 @@ Full documentation is being prepared. For now, use get_node_essentials for confi // Get properties const properties = node.properties || []; - // Extract operation context + // Extract operation context (safely handle undefined config properties) const operationContext = { - resource: config.resource, - operation: config.operation, - action: config.action, - mode: config.mode + resource: config?.resource, + operation: config?.operation, + action: config?.action, + mode: config?.mode }; // Find missing required fields @@ -2163,7 +2240,7 @@ Full documentation is being prepared. For now, use get_node_essentials for confi // Check show conditions if (prop.displayOptions.show) { for (const [key, values] of Object.entries(prop.displayOptions.show)) { - const configValue = config[key]; + const configValue = config?.[key]; const expectedValues = Array.isArray(values) ? values : [values]; if (!expectedValues.includes(configValue)) { @@ -2176,7 +2253,7 @@ Full documentation is being prepared. For now, use get_node_essentials for confi // Check hide conditions if (isVisible && prop.displayOptions.hide) { for (const [key, values] of Object.entries(prop.displayOptions.hide)) { - const configValue = config[key]; + const configValue = config?.[key]; const expectedValues = Array.isArray(values) ? values : [values]; if (expectedValues.includes(configValue)) { @@ -2189,8 +2266,8 @@ Full documentation is being prepared. For now, use get_node_essentials for confi if (!isVisible) continue; } - // Check if field is missing - if (!(prop.name in config)) { + // Check if field is missing (safely handle null/undefined config) + if (!config || !(prop.name in config)) { missingFields.push(prop.displayName || prop.name); } } diff --git a/src/services/enhanced-config-validator.ts b/src/services/enhanced-config-validator.ts index 0e66629..fb372fe 100644 --- a/src/services/enhanced-config-validator.ts +++ b/src/services/enhanced-config-validator.ts @@ -45,6 +45,19 @@ export class EnhancedConfigValidator extends ConfigValidator { mode: ValidationMode = 'operation', profile: ValidationProfile = 'ai-friendly' ): EnhancedValidationResult { + // Input validation - ensure parameters are valid + if (typeof nodeType !== 'string') { + throw new Error(`Invalid nodeType: expected string, got ${typeof nodeType}`); + } + + if (!config || typeof config !== 'object') { + throw new Error(`Invalid config: expected object, got ${typeof config}`); + } + + if (!Array.isArray(properties)) { + throw new Error(`Invalid properties: expected array, got ${typeof properties}`); + } + // Extract operation context from config const operationContext = this.extractOperationContext(config); @@ -190,6 +203,17 @@ export class EnhancedConfigValidator extends ConfigValidator { config: Record, result: EnhancedValidationResult ): void { + // Type safety check - this should never happen with proper validation + if (typeof nodeType !== 'string') { + result.errors.push({ + type: 'invalid_type', + property: 'nodeType', + message: `Invalid nodeType: expected string, got ${typeof nodeType}`, + fix: 'Provide a valid node type string (e.g., "nodes-base.webhook")' + }); + return; + } + // First, validate fixedCollection properties for known problematic nodes this.validateFixedCollectionStructures(nodeType, config, result); diff --git a/src/services/workflow-validator.ts b/src/services/workflow-validator.ts index bc3127e..7003839 100644 --- a/src/services/workflow-validator.ts +++ b/src/services/workflow-validator.ts @@ -79,6 +79,18 @@ export class WorkflowValidator { private nodeValidator: typeof EnhancedConfigValidator ) {} + /** + * Check if a node is a Sticky Note or other non-executable node + */ + private isStickyNote(node: WorkflowNode): boolean { + const stickyNoteTypes = [ + 'n8n-nodes-base.stickyNote', + 'nodes-base.stickyNote', + '@n8n/n8n-nodes-base.stickyNote' + ]; + return stickyNoteTypes.includes(node.type); + } + /** * Validate a complete workflow */ @@ -127,9 +139,10 @@ export class WorkflowValidator { return result; } - // Update statistics after null check - result.statistics.totalNodes = Array.isArray(workflow.nodes) ? workflow.nodes.length : 0; - result.statistics.enabledNodes = Array.isArray(workflow.nodes) ? workflow.nodes.filter(n => !n.disabled).length : 0; + // Update statistics after null check (exclude sticky notes from counts) + const executableNodes = Array.isArray(workflow.nodes) ? workflow.nodes.filter(n => !this.isStickyNote(n)) : []; + result.statistics.totalNodes = executableNodes.length; + result.statistics.enabledNodes = executableNodes.filter(n => !n.disabled).length; // Basic workflow structure validation this.validateWorkflowStructure(workflow, result); @@ -143,21 +156,26 @@ export class WorkflowValidator { // Validate connections if requested if (validateConnections) { - this.validateConnections(workflow, result); + this.validateConnections(workflow, result, profile); } // Validate expressions if requested if (validateExpressions && workflow.nodes.length > 0) { - this.validateExpressions(workflow, result); + this.validateExpressions(workflow, result, profile); } // Check workflow patterns and best practices if (workflow.nodes.length > 0) { - this.checkWorkflowPatterns(workflow, result); + this.checkWorkflowPatterns(workflow, result, profile); } // Add suggestions based on findings this.generateSuggestions(workflow, result); + + // Add AI-specific recovery suggestions if there are errors + if (result.errors.length > 0) { + this.addErrorRecoverySuggestions(result); + } } } catch (error) { @@ -308,7 +326,7 @@ export class WorkflowValidator { profile: string ): Promise { for (const node of workflow.nodes) { - if (node.disabled) continue; + if (node.disabled || this.isStickyNote(node)) continue; try { // Validate node name length @@ -500,7 +518,8 @@ export class WorkflowValidator { */ private validateConnections( workflow: WorkflowJson, - result: WorkflowValidationResult + result: WorkflowValidationResult, + profile: string = 'runtime' ): void { const nodeMap = new Map(workflow.nodes.map(n => [n.name, n])); const nodeIdMap = new Map(workflow.nodes.map(n => [n.id, n])); @@ -591,9 +610,9 @@ export class WorkflowValidator { } }); - // Check for orphaned nodes + // Check for orphaned nodes (exclude sticky notes) for (const node of workflow.nodes) { - if (node.disabled) continue; + if (node.disabled || this.isStickyNote(node)) continue; const normalizedType = node.type.replace('n8n-nodes-base.', 'nodes-base.'); const isTrigger = normalizedType.toLowerCase().includes('trigger') || @@ -612,8 +631,8 @@ export class WorkflowValidator { } } - // Check for cycles - if (this.hasCycle(workflow)) { + // Check for cycles (skip in minimal profile to reduce false positives) + if (profile !== 'minimal' && this.hasCycle(workflow)) { result.errors.push({ type: 'error', message: 'Workflow contains a cycle (infinite loop)' @@ -757,10 +776,22 @@ export class WorkflowValidator { const recursionStack = new Set(); const nodeTypeMap = new Map(); - // Build node type map + // Build node type map (exclude sticky notes) workflow.nodes.forEach(node => { - nodeTypeMap.set(node.name, node.type); + if (!this.isStickyNote(node)) { + nodeTypeMap.set(node.name, node.type); + } }); + + // Known legitimate loop node types + const loopNodeTypes = [ + 'n8n-nodes-base.splitInBatches', + 'nodes-base.splitInBatches', + 'n8n-nodes-base.itemLists', + 'nodes-base.itemLists', + 'n8n-nodes-base.loop', + 'nodes-base.loop' + ]; const hasCycleDFS = (nodeName: string, pathFromLoopNode: boolean = false): boolean => { visited.add(nodeName); @@ -789,18 +820,18 @@ export class WorkflowValidator { } const currentNodeType = nodeTypeMap.get(nodeName); - const isLoopNode = currentNodeType === 'n8n-nodes-base.splitInBatches'; + const isLoopNode = loopNodeTypes.includes(currentNodeType || ''); for (const target of allTargets) { if (!visited.has(target)) { if (hasCycleDFS(target, pathFromLoopNode || isLoopNode)) return true; } else if (recursionStack.has(target)) { - // Allow cycles that involve loop nodes like SplitInBatches + // Allow cycles that involve legitimate loop nodes const targetNodeType = nodeTypeMap.get(target); - const isTargetLoopNode = targetNodeType === 'n8n-nodes-base.splitInBatches'; + const isTargetLoopNode = loopNodeTypes.includes(targetNodeType || ''); // If this cycle involves a loop node, it's legitimate - if (isTargetLoopNode || pathFromLoopNode) { + if (isTargetLoopNode || pathFromLoopNode || isLoopNode) { continue; // Allow this cycle } @@ -813,9 +844,9 @@ export class WorkflowValidator { return false; }; - // Check from all nodes + // Check from all executable nodes (exclude sticky notes) for (const node of workflow.nodes) { - if (!visited.has(node.name)) { + if (!this.isStickyNote(node) && !visited.has(node.name)) { if (hasCycleDFS(node.name)) return true; } } @@ -828,12 +859,13 @@ export class WorkflowValidator { */ private validateExpressions( workflow: WorkflowJson, - result: WorkflowValidationResult + result: WorkflowValidationResult, + profile: string = 'runtime' ): void { const nodeNames = workflow.nodes.map(n => n.name); for (const node of workflow.nodes) { - if (node.disabled) continue; + if (node.disabled || this.isStickyNote(node)) continue; // Create expression context const context = { @@ -922,23 +954,27 @@ export class WorkflowValidator { */ private checkWorkflowPatterns( workflow: WorkflowJson, - result: WorkflowValidationResult + result: WorkflowValidationResult, + profile: string = 'runtime' ): void { // Check for error handling const hasErrorHandling = Object.values(workflow.connections).some( outputs => outputs.error && outputs.error.length > 0 ); - if (!hasErrorHandling && workflow.nodes.length > 3) { + // Only suggest error handling in stricter profiles + if (!hasErrorHandling && workflow.nodes.length > 3 && profile !== 'minimal') { result.warnings.push({ type: 'warning', message: 'Consider adding error handling to your workflow' }); } - // Check node-level error handling properties for ALL nodes + // Check node-level error handling properties for ALL executable nodes for (const node of workflow.nodes) { - this.checkNodeErrorHandling(node, workflow, result); + if (!this.isStickyNote(node)) { + this.checkNodeErrorHandling(node, workflow, result); + } } // Check for very long linear workflows @@ -1641,4 +1677,75 @@ export class WorkflowValidator { return false; } + + /** + * Add AI-specific error recovery suggestions + */ + private addErrorRecoverySuggestions(result: WorkflowValidationResult): void { + // Categorize errors and provide specific recovery actions + const errorTypes = { + nodeType: result.errors.filter(e => e.message.includes('node type') || e.message.includes('Node type')), + connection: result.errors.filter(e => e.message.includes('connection') || e.message.includes('Connection')), + structure: result.errors.filter(e => e.message.includes('structure') || e.message.includes('nodes must be')), + configuration: result.errors.filter(e => e.message.includes('property') || e.message.includes('field')), + typeVersion: result.errors.filter(e => e.message.includes('typeVersion')) + }; + + // Add recovery suggestions based on error types + if (errorTypes.nodeType.length > 0) { + result.suggestions.unshift( + '🔧 RECOVERY: Invalid node types detected. Use these patterns:', + ' • For core nodes: "n8n-nodes-base.nodeName" (e.g., "n8n-nodes-base.webhook")', + ' • For AI nodes: "@n8n/n8n-nodes-langchain.nodeName"', + ' • Never use just the node name without package prefix' + ); + } + + if (errorTypes.connection.length > 0) { + result.suggestions.unshift( + '🔧 RECOVERY: Connection errors detected. Fix with:', + ' • Use node NAMES in connections, not IDs or types', + ' • Structure: { "Source Node Name": { "main": [[{ "node": "Target Node Name", "type": "main", "index": 0 }]] } }', + ' • Ensure all referenced nodes exist in the workflow' + ); + } + + if (errorTypes.structure.length > 0) { + result.suggestions.unshift( + '🔧 RECOVERY: Workflow structure errors. Fix with:', + ' • Ensure "nodes" is an array: "nodes": [...]', + ' • Ensure "connections" is an object: "connections": {...}', + ' • Add at least one node to create a valid workflow' + ); + } + + if (errorTypes.configuration.length > 0) { + result.suggestions.unshift( + '🔧 RECOVERY: Node configuration errors. Fix with:', + ' • Check required fields using validate_node_minimal first', + ' • Use get_node_essentials to see what fields are needed', + ' • Ensure operation-specific fields match the node\'s requirements' + ); + } + + if (errorTypes.typeVersion.length > 0) { + result.suggestions.unshift( + '🔧 RECOVERY: TypeVersion errors. Fix with:', + ' • Add "typeVersion": 1 (or latest version) to each node', + ' • Use get_node_info to check the correct version for each node type' + ); + } + + // Add general recovery workflow + if (result.errors.length > 3) { + result.suggestions.push( + '📋 SUGGESTED WORKFLOW: Too many errors detected. Try this approach:', + ' 1. Fix structural issues first (nodes array, connections object)', + ' 2. Validate node types and fix invalid ones', + ' 3. Add required typeVersion to all nodes', + ' 4. Test connections step by step', + ' 5. Use validate_node_minimal on individual nodes to verify configuration' + ); + } + } } \ No newline at end of file diff --git a/src/utils/validation-schemas.ts b/src/utils/validation-schemas.ts new file mode 100644 index 0000000..fe7d1a3 --- /dev/null +++ b/src/utils/validation-schemas.ts @@ -0,0 +1,312 @@ +/** + * Zod validation schemas for MCP tool parameters + * Provides robust input validation with detailed error messages + */ + +// Simple validation without zod for now, since it's not installed +// We can use TypeScript's built-in validation with better error messages + +export class ValidationError extends Error { + constructor(message: string, public field?: string, public value?: any) { + super(message); + this.name = 'ValidationError'; + } +} + +export interface ValidationResult { + valid: boolean; + errors: Array<{ + field: string; + message: string; + value?: any; + }>; +} + +/** + * Basic validation utilities + */ +export class Validator { + /** + * Validate that a value is a non-empty string + */ + static validateString(value: any, fieldName: string, required: boolean = true): ValidationResult { + const errors: Array<{field: string, message: string, value?: any}> = []; + + if (required && (value === undefined || value === null)) { + errors.push({ + field: fieldName, + message: `${fieldName} is required`, + value + }); + } else if (value !== undefined && value !== null && typeof value !== 'string') { + errors.push({ + field: fieldName, + message: `${fieldName} must be a string, got ${typeof value}`, + value + }); + } else if (required && typeof value === 'string' && value.trim().length === 0) { + errors.push({ + field: fieldName, + message: `${fieldName} cannot be empty`, + value + }); + } + + return { + valid: errors.length === 0, + errors + }; + } + + /** + * Validate that a value is a valid object (not null, not array) + */ + static validateObject(value: any, fieldName: string, required: boolean = true): ValidationResult { + const errors: Array<{field: string, message: string, value?: any}> = []; + + if (required && (value === undefined || value === null)) { + errors.push({ + field: fieldName, + message: `${fieldName} is required`, + value + }); + } else if (value !== undefined && value !== null) { + if (typeof value !== 'object') { + errors.push({ + field: fieldName, + message: `${fieldName} must be an object, got ${typeof value}`, + value + }); + } else if (Array.isArray(value)) { + errors.push({ + field: fieldName, + message: `${fieldName} must be an object, not an array`, + value + }); + } + } + + return { + valid: errors.length === 0, + errors + }; + } + + /** + * Validate that a value is an array + */ + static validateArray(value: any, fieldName: string, required: boolean = true): ValidationResult { + const errors: Array<{field: string, message: string, value?: any}> = []; + + if (required && (value === undefined || value === null)) { + errors.push({ + field: fieldName, + message: `${fieldName} is required`, + value + }); + } else if (value !== undefined && value !== null && !Array.isArray(value)) { + errors.push({ + field: fieldName, + message: `${fieldName} must be an array, got ${typeof value}`, + value + }); + } + + return { + valid: errors.length === 0, + errors + }; + } + + /** + * Validate that a value is a number + */ + static validateNumber(value: any, fieldName: string, required: boolean = true, min?: number, max?: number): ValidationResult { + const errors: Array<{field: string, message: string, value?: any}> = []; + + if (required && (value === undefined || value === null)) { + errors.push({ + field: fieldName, + message: `${fieldName} is required`, + value + }); + } else if (value !== undefined && value !== null) { + if (typeof value !== 'number' || isNaN(value)) { + errors.push({ + field: fieldName, + message: `${fieldName} must be a number, got ${typeof value}`, + value + }); + } else { + if (min !== undefined && value < min) { + errors.push({ + field: fieldName, + message: `${fieldName} must be at least ${min}, got ${value}`, + value + }); + } + if (max !== undefined && value > max) { + errors.push({ + field: fieldName, + message: `${fieldName} must be at most ${max}, got ${value}`, + value + }); + } + } + } + + return { + valid: errors.length === 0, + errors + }; + } + + /** + * Validate that a value is one of allowed values + */ + static validateEnum(value: any, fieldName: string, allowedValues: T[], required: boolean = true): ValidationResult { + const errors: Array<{field: string, message: string, value?: any}> = []; + + if (required && (value === undefined || value === null)) { + errors.push({ + field: fieldName, + message: `${fieldName} is required`, + value + }); + } else if (value !== undefined && value !== null && !allowedValues.includes(value)) { + errors.push({ + field: fieldName, + message: `${fieldName} must be one of: ${allowedValues.join(', ')}, got "${value}"`, + value + }); + } + + return { + valid: errors.length === 0, + errors + }; + } + + /** + * Combine multiple validation results + */ + static combineResults(...results: ValidationResult[]): ValidationResult { + const allErrors = results.flatMap(r => r.errors); + return { + valid: allErrors.length === 0, + errors: allErrors + }; + } + + /** + * Create a detailed error message from validation result + */ + static formatErrors(result: ValidationResult, toolName?: string): string { + if (result.valid) return ''; + + const prefix = toolName ? `${toolName}: ` : ''; + const errors = result.errors.map(e => ` • ${e.field}: ${e.message}`).join('\n'); + + return `${prefix}Validation failed:\n${errors}`; + } +} + +/** + * Tool-specific validation schemas + */ +export class ToolValidation { + /** + * Validate parameters for validate_node_operation tool + */ + static validateNodeOperation(args: any): ValidationResult { + const nodeTypeResult = Validator.validateString(args.nodeType, 'nodeType'); + const configResult = Validator.validateObject(args.config, 'config'); + const profileResult = Validator.validateEnum( + args.profile, + 'profile', + ['minimal', 'runtime', 'ai-friendly', 'strict'], + false // optional + ); + + return Validator.combineResults(nodeTypeResult, configResult, profileResult); + } + + /** + * Validate parameters for validate_node_minimal tool + */ + static validateNodeMinimal(args: any): ValidationResult { + const nodeTypeResult = Validator.validateString(args.nodeType, 'nodeType'); + const configResult = Validator.validateObject(args.config, 'config'); + + return Validator.combineResults(nodeTypeResult, configResult); + } + + /** + * Validate parameters for validate_workflow tool + */ + static validateWorkflow(args: any): ValidationResult { + const workflowResult = Validator.validateObject(args.workflow, 'workflow'); + + // Validate workflow structure if it's an object + let nodesResult: ValidationResult = { valid: true, errors: [] }; + let connectionsResult: ValidationResult = { valid: true, errors: [] }; + + if (workflowResult.valid && args.workflow) { + nodesResult = Validator.validateArray(args.workflow.nodes, 'workflow.nodes'); + connectionsResult = Validator.validateObject(args.workflow.connections, 'workflow.connections'); + } + + const optionsResult = args.options ? + Validator.validateObject(args.options, 'options', false) : + { valid: true, errors: [] }; + + return Validator.combineResults(workflowResult, nodesResult, connectionsResult, optionsResult); + } + + /** + * Validate parameters for search_nodes tool + */ + static validateSearchNodes(args: any): ValidationResult { + const queryResult = Validator.validateString(args.query, 'query'); + const limitResult = Validator.validateNumber(args.limit, 'limit', false, 1, 200); + const modeResult = Validator.validateEnum( + args.mode, + 'mode', + ['OR', 'AND', 'FUZZY'], + false + ); + + return Validator.combineResults(queryResult, limitResult, modeResult); + } + + /** + * Validate parameters for list_node_templates tool + */ + static validateListNodeTemplates(args: any): ValidationResult { + const nodeTypesResult = Validator.validateArray(args.nodeTypes, 'nodeTypes'); + const limitResult = Validator.validateNumber(args.limit, 'limit', false, 1, 50); + + return Validator.combineResults(nodeTypesResult, limitResult); + } + + /** + * Validate parameters for n8n workflow operations + */ + static validateWorkflowId(args: any): ValidationResult { + return Validator.validateString(args.id, 'id'); + } + + /** + * Validate parameters for n8n_create_workflow tool + */ + static validateCreateWorkflow(args: any): ValidationResult { + const nameResult = Validator.validateString(args.name, 'name'); + const nodesResult = Validator.validateArray(args.nodes, 'nodes'); + const connectionsResult = Validator.validateObject(args.connections, 'connections'); + const settingsResult = args.settings ? + Validator.validateObject(args.settings, 'settings', false) : + { valid: true, errors: [] }; + + return Validator.combineResults(nameResult, nodesResult, connectionsResult, settingsResult); + } +} \ No newline at end of file diff --git a/tests/unit/services/workflow-validator-comprehensive.test.ts b/tests/unit/services/workflow-validator-comprehensive.test.ts index f61afd1..8b9e536 100644 --- a/tests/unit/services/workflow-validator-comprehensive.test.ts +++ b/tests/unit/services/workflow-validator-comprehensive.test.ts @@ -223,7 +223,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { it('should error when nodes array is missing', async () => { const workflow = { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(false); expect(result.errors.some(e => e.message === 'Workflow must have a nodes array')).toBe(true); @@ -232,7 +232,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { it('should error when connections object is missing', async () => { const workflow = { nodes: [] } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(false); expect(result.errors.some(e => e.message === 'Workflow must have a connections object')).toBe(true); @@ -241,7 +241,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { it('should warn when workflow has no nodes', async () => { const workflow = { nodes: [], connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(true); // Empty workflows are valid but get a warning expect(result.warnings).toHaveLength(1); @@ -260,7 +260,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(false); expect(result.errors.some(e => e.message.includes('Single-node workflows are only valid for webhook endpoints'))).toBe(true); @@ -279,7 +279,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(true); expect(result.warnings.some(w => w.message.includes('Webhook node has no connections'))).toBe(true); @@ -306,7 +306,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(false); expect(result.errors.some(e => e.message.includes('Multi-node workflow has no connections'))).toBe(true); @@ -333,7 +333,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Duplicate node name: "Webhook"'))).toBe(true); }); @@ -359,7 +359,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Duplicate node ID: "1"'))).toBe(true); }); @@ -392,7 +392,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.statistics.triggerNodes).toBe(3); }); @@ -422,7 +422,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('Workflow has no trigger nodes'))).toBe(true); }); @@ -449,7 +449,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.statistics.totalNodes).toBe(2); expect(result.statistics.enabledNodes).toBe(1); @@ -472,7 +472,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(mockNodeRepository.getNode).not.toHaveBeenCalled(); }); @@ -491,7 +491,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(false); expect(result.errors.some(e => e.message.includes('Invalid node type: "nodes-base.webhook"'))).toBe(true); @@ -512,7 +512,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(false); expect(result.errors.some(e => e.message.includes('Unknown node type: "httpRequest"'))).toBe(true); @@ -533,7 +533,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(mockNodeRepository.getNode).toHaveBeenCalledWith('n8n-nodes-base.webhook'); expect(mockNodeRepository.getNode).toHaveBeenCalledWith('nodes-base.webhook'); @@ -553,7 +553,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(mockNodeRepository.getNode).toHaveBeenCalledWith('@n8n/n8n-nodes-langchain.agent'); expect(mockNodeRepository.getNode).toHaveBeenCalledWith('nodes-langchain.agent'); @@ -574,7 +574,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Missing required property \'typeVersion\''))).toBe(true); }); @@ -594,7 +594,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Invalid typeVersion: invalid'))).toBe(true); }); @@ -614,7 +614,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('Outdated typeVersion: 1. Latest is 2'))).toBe(true); }); @@ -634,7 +634,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('typeVersion 10 exceeds maximum supported version 2'))).toBe(true); }); @@ -664,7 +664,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Missing required field: url'))).toBe(true); expect(result.warnings.some(w => w.message.includes('Consider using HTTPS'))).toBe(true); @@ -689,7 +689,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Failed to validate node: Validation error'))).toBe(true); }); @@ -721,7 +721,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.statistics.validConnections).toBe(1); expect(result.statistics.invalidConnections).toBe(0); @@ -745,7 +745,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Connection from non-existent node: "NonExistent"'))).toBe(true); expect(result.statistics.invalidConnections).toBe(1); @@ -776,7 +776,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Connection uses node ID \'webhook-id\' instead of node name \'Webhook\''))).toBe(true); }); @@ -799,7 +799,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Connection to non-existent node: "NonExistent"'))).toBe(true); expect(result.statistics.invalidConnections).toBe(1); @@ -830,7 +830,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Connection target uses node ID \'set-id\' instead of node name \'Set\''))).toBe(true); }); @@ -861,7 +861,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('Connection to disabled node: "Set"'))).toBe(true); }); @@ -891,7 +891,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.statistics.validConnections).toBe(1); }); @@ -921,7 +921,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.statistics.validConnections).toBe(1); }); @@ -953,7 +953,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('Community node "CustomTool" is being used as an AI tool'))).toBe(true); }); @@ -990,7 +990,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('Node is not connected to any other nodes') && w.nodeName === 'Orphaned')).toBe(true); }); @@ -1033,7 +1033,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Workflow contains a cycle'))).toBe(true); }); @@ -1068,7 +1068,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.statistics.validConnections).toBe(1); expect(result.valid).toBe(true); @@ -1110,7 +1110,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(ExpressionValidator.validateNodeExpressions).toHaveBeenCalledWith( expect.objectContaining({ values: expect.any(Object) }), @@ -1146,7 +1146,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Expression error: Invalid expression syntax'))).toBe(true); expect(result.warnings.some(w => w.message.includes('Expression warning: Deprecated variable usage'))).toBe(true); @@ -1170,7 +1170,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(ExpressionValidator.validateNodeExpressions).not.toHaveBeenCalled(); }); @@ -1187,7 +1187,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { const workflow = builder.build() as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('Consider adding error handling'))).toBe(true); }); @@ -1208,7 +1208,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { const workflow = builder.build() as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('Long linear chain detected'))).toBe(true); }); @@ -1230,7 +1230,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('Missing credentials configuration for slackApi'))).toBe(true); }); @@ -1249,7 +1249,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('AI Agent has no tools connected'))).toBe(true); }); @@ -1279,7 +1279,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.suggestions.some(s => s.includes('N8N_COMMUNITY_PACKAGES_ALLOW_TOOL_USAGE'))).toBe(true); }); @@ -1306,7 +1306,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Node-level properties onError, retryOnFail, credentials are in the wrong location'))).toBe(true); expect(result.errors.some(e => e.details?.fix?.includes('Move these properties from node.parameters to the node level'))).toBe(true); @@ -1327,7 +1327,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Invalid onError value: "invalidValue"'))).toBe(true); }); @@ -1347,7 +1347,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('Using deprecated "continueOnFail: true"'))).toBe(true); }); @@ -1368,7 +1368,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Cannot use both "continueOnFail" and "onError" properties'))).toBe(true); }); @@ -1390,7 +1390,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('maxTries must be a positive number'))).toBe(true); expect(result.errors.some(e => e.message.includes('waitBetweenTries must be a non-negative number'))).toBe(true); @@ -1413,7 +1413,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('maxTries is set to 15'))).toBe(true); expect(result.warnings.some(w => w.message.includes('waitBetweenTries is set to 400000ms'))).toBe(true); @@ -1434,7 +1434,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('retryOnFail is enabled but maxTries is not specified'))).toBe(true); }); @@ -1459,7 +1459,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('alwaysOutputData must be a boolean'))).toBe(true); @@ -1484,7 +1484,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('executeOnce is enabled'))).toBe(true); }); @@ -1512,7 +1512,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes(nodeInfo.message) && w.message.includes('without error handling'))).toBe(true); } @@ -1534,7 +1534,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('Both continueOnFail and retryOnFail are enabled'))).toBe(true); }); @@ -1554,7 +1554,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.suggestions.some(s => s.includes('Consider enabling alwaysOutputData'))).toBe(true); }); @@ -1569,7 +1569,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { const workflow = builder.build() as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.suggestions.some(s => s.includes('Most nodes lack error handling'))).toBe(true); }); @@ -1589,7 +1589,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.suggestions.some(s => s.includes('Replace "continueOnFail: true" with "onError:'))).toBe(true); }); @@ -1610,7 +1610,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.suggestions.some(s => s.includes('Add a trigger node'))).toBe(true); }); @@ -1636,7 +1636,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} // Missing connections } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.suggestions.some(s => s.includes('Example connection structure'))).toBe(true); expect(result.suggestions.some(s => s.includes('Use node NAMES (not IDs) in connections'))).toBe(true); @@ -1667,7 +1667,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.suggestions.some(s => s.includes('Add error handling'))).toBe(true); }); @@ -1682,7 +1682,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { const workflow = builder.build() as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.suggestions.some(s => s.includes('Consider breaking this workflow into smaller sub-workflows'))).toBe(true); }); @@ -1708,7 +1708,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.suggestions.some(s => s.includes('Consider using a Code node for complex data transformations'))).toBe(true); }); @@ -1727,7 +1727,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.suggestions.some(s => s.includes('A minimal workflow needs'))).toBe(true); }); @@ -1756,7 +1756,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { connections: {} } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes(`Did you mean`) && e.message.includes(testCase.suggestion))).toBe(true); } @@ -1848,7 +1848,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); // Should have multiple errors expect(result.valid).toBe(false); @@ -1940,7 +1940,7 @@ describe('WorkflowValidator - Comprehensive Tests', () => { } } as any; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(true); expect(result.errors).toHaveLength(0); diff --git a/tests/unit/services/workflow-validator-edge-cases.test.ts b/tests/unit/services/workflow-validator-edge-cases.test.ts index 17befa7..c1547d8 100644 --- a/tests/unit/services/workflow-validator-edge-cases.test.ts +++ b/tests/unit/services/workflow-validator-edge-cases.test.ts @@ -157,7 +157,7 @@ describe('WorkflowValidator - Edge Cases', () => { nodes: [], connections: {} }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(true); expect(result.warnings.some(w => w.message.includes('empty'))).toBe(true); }); @@ -181,7 +181,7 @@ describe('WorkflowValidator - Edge Cases', () => { const workflow = { nodes, connections }; const start = Date.now(); - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); const duration = Date.now() - start; expect(result).toBeDefined(); @@ -207,7 +207,7 @@ describe('WorkflowValidator - Edge Cases', () => { } }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.statistics.invalidConnections).toBe(0); }); @@ -228,7 +228,7 @@ describe('WorkflowValidator - Edge Cases', () => { } }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(true); }); }); @@ -264,7 +264,7 @@ describe('WorkflowValidator - Edge Cases', () => { connections: {} }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.length).toBeGreaterThan(0); }); @@ -292,7 +292,7 @@ describe('WorkflowValidator - Edge Cases', () => { } }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('self-referencing'))).toBe(true); }); @@ -308,7 +308,7 @@ describe('WorkflowValidator - Edge Cases', () => { } }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('non-existent'))).toBe(true); }); @@ -324,7 +324,7 @@ describe('WorkflowValidator - Edge Cases', () => { } }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.length).toBeGreaterThan(0); }); @@ -341,7 +341,7 @@ describe('WorkflowValidator - Edge Cases', () => { } as any }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); // Should still work as type and index can have defaults expect(result.statistics.validConnections).toBeGreaterThan(0); }); @@ -359,7 +359,7 @@ describe('WorkflowValidator - Edge Cases', () => { } }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.some(e => e.message.includes('Invalid'))).toBe(true); }); }); @@ -382,7 +382,7 @@ describe('WorkflowValidator - Edge Cases', () => { } }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.valid).toBe(true); }); @@ -395,7 +395,7 @@ describe('WorkflowValidator - Edge Cases', () => { connections: {} }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.warnings.some(w => w.message.includes('very long'))).toBe(true); }); }); @@ -479,7 +479,7 @@ describe('WorkflowValidator - Edge Cases', () => { } }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.statistics.validConnections).toBeGreaterThan(0); }); }); @@ -499,7 +499,7 @@ describe('WorkflowValidator - Edge Cases', () => { } }; - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); expect(result.errors.length).toBeGreaterThan(0); expect(result.statistics.validConnections).toBeGreaterThan(0); }); diff --git a/tests/unit/services/workflow-validator-with-mocks.test.ts b/tests/unit/services/workflow-validator-with-mocks.test.ts index e2d2a98..74f53c2 100644 --- a/tests/unit/services/workflow-validator-with-mocks.test.ts +++ b/tests/unit/services/workflow-validator-with-mocks.test.ts @@ -77,7 +77,7 @@ describe('WorkflowValidator - Simple Unit Tests', () => { }; // Act - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); // Assert expect(result.valid).toBe(true); @@ -113,7 +113,7 @@ describe('WorkflowValidator - Simple Unit Tests', () => { }; // Act - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); // Assert expect(result.valid).toBe(false); @@ -154,7 +154,7 @@ describe('WorkflowValidator - Simple Unit Tests', () => { }; // Act - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); // Assert expect(result.valid).toBe(false); @@ -229,7 +229,7 @@ describe('WorkflowValidator - Simple Unit Tests', () => { }; // Act - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); // Assert expect(result.valid).toBe(true); @@ -297,7 +297,7 @@ describe('WorkflowValidator - Simple Unit Tests', () => { }; // Act - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); // Assert expect(result.valid).toBe(false); @@ -386,7 +386,7 @@ describe('WorkflowValidator - Simple Unit Tests', () => { }; // Act - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); // Assert expect(result.valid).toBe(false); @@ -438,7 +438,7 @@ describe('WorkflowValidator - Simple Unit Tests', () => { }; // Act - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); // Assert expect(result.warnings.some(w => w.message.includes('Outdated typeVersion'))).toBe(true); @@ -471,7 +471,7 @@ describe('WorkflowValidator - Simple Unit Tests', () => { }; // Act - const result = await validator.validateWorkflow(workflow); + const result = await validator.validateWorkflow(workflow as any); // Assert expect(result.valid).toBe(false); diff --git a/tests/unit/validation-fixes.test.ts b/tests/unit/validation-fixes.test.ts new file mode 100644 index 0000000..f1e95fb --- /dev/null +++ b/tests/unit/validation-fixes.test.ts @@ -0,0 +1,410 @@ +/** + * Test suite for validation system fixes + * Covers issues #58, #68, #70, #73 + */ + +import { describe, test, expect, beforeAll, afterAll } from 'vitest'; +import { WorkflowValidator } from '../../src/services/workflow-validator'; +import { EnhancedConfigValidator } from '../../src/services/enhanced-config-validator'; +import { ToolValidation, Validator, ValidationError } from '../../src/utils/validation-schemas'; + +describe('Validation System Fixes', () => { + let workflowValidator: WorkflowValidator; + let mockNodeRepository: any; + + beforeAll(async () => { + // Initialize test environment + process.env.NODE_ENV = 'test'; + + // Mock repository for testing + mockNodeRepository = { + getNode: (nodeType: string) => { + if (nodeType === 'nodes-base.webhook' || nodeType === 'n8n-nodes-base.webhook') { + return { + nodeType: 'nodes-base.webhook', + displayName: 'Webhook', + properties: [ + { name: 'path', required: true, displayName: 'Path' }, + { name: 'httpMethod', required: true, displayName: 'HTTP Method' } + ] + }; + } + if (nodeType === 'nodes-base.set' || nodeType === 'n8n-nodes-base.set') { + return { + nodeType: 'nodes-base.set', + displayName: 'Set', + properties: [ + { name: 'values', required: false, displayName: 'Values' } + ] + }; + } + return null; + } + } as any; + + workflowValidator = new WorkflowValidator(mockNodeRepository, EnhancedConfigValidator); + }); + + afterAll(() => { + delete process.env.NODE_ENV; + }); + + describe('Issue #73: validate_node_minimal crashes without input validation', () => { + test('should handle empty config in validation schemas', () => { + // Test the validation schema handles empty config + const result = ToolValidation.validateNodeMinimal({ + nodeType: 'nodes-base.webhook', + config: undefined + }); + + expect(result).toBeDefined(); + expect(result.valid).toBe(false); + expect(result.errors.length).toBeGreaterThan(0); + expect(result.errors[0].field).toBe('config'); + }); + + test('should handle null config in validation schemas', () => { + const result = ToolValidation.validateNodeMinimal({ + nodeType: 'nodes-base.webhook', + config: null + }); + + expect(result).toBeDefined(); + expect(result.valid).toBe(false); + expect(result.errors.length).toBeGreaterThan(0); + expect(result.errors[0].field).toBe('config'); + }); + + test('should accept valid config object', () => { + const result = ToolValidation.validateNodeMinimal({ + nodeType: 'nodes-base.webhook', + config: { path: '/webhook', httpMethod: 'POST' } + }); + + expect(result).toBeDefined(); + expect(result.valid).toBe(true); + expect(result.errors).toHaveLength(0); + }); + }); + + describe('Issue #58: validate_node_operation crashes on nested input', () => { + test('should handle invalid nodeType gracefully', () => { + expect(() => { + EnhancedConfigValidator.validateWithMode( + undefined as any, + { resource: 'channel', operation: 'create' }, + [], + 'operation', + 'ai-friendly' + ); + }).toThrow(Error); + }); + + test('should handle null nodeType gracefully', () => { + expect(() => { + EnhancedConfigValidator.validateWithMode( + null as any, + { resource: 'channel', operation: 'create' }, + [], + 'operation', + 'ai-friendly' + ); + }).toThrow(Error); + }); + + test('should handle non-string nodeType gracefully', () => { + expect(() => { + EnhancedConfigValidator.validateWithMode( + { type: 'nodes-base.slack' } as any, + { resource: 'channel', operation: 'create' }, + [], + 'operation', + 'ai-friendly' + ); + }).toThrow(Error); + }); + + test('should handle valid nodeType properly', () => { + const result = EnhancedConfigValidator.validateWithMode( + 'nodes-base.set', + { values: {} }, + [], + 'operation', + 'ai-friendly' + ); + + expect(result).toBeDefined(); + expect(typeof result.valid).toBe('boolean'); + }); + }); + + describe('Issue #70: Profile settings not respected', () => { + test('should pass profile parameter to all validation phases', async () => { + const workflow = { + nodes: [ + { + id: '1', + name: 'Webhook', + type: 'n8n-nodes-base.webhook', + position: [100, 200], + parameters: { path: '/test', httpMethod: 'POST' }, + typeVersion: 1 + }, + { + id: '2', + name: 'Set', + type: 'n8n-nodes-base.set', + position: [300, 200], + parameters: { values: {} }, + typeVersion: 1 + } + ], + connections: { + 'Webhook': { + main: [[{ node: 'Set', type: 'main', index: 0 }]] + } + } + }; + + const result = await workflowValidator.validateWorkflow(workflow, { + validateNodes: true, + validateConnections: true, + validateExpressions: true, + profile: 'minimal' + }); + + expect(result).toBeDefined(); + expect(result.valid).toBe(true); + // In minimal profile, should have fewer warnings/errors - just check it's reasonable + expect(result.warnings.length).toBeLessThanOrEqual(5); + }); + + test('should filter out sticky notes from validation', async () => { + const workflow = { + nodes: [ + { + id: '1', + name: 'Webhook', + type: 'n8n-nodes-base.webhook', + position: [100, 200], + parameters: { path: '/test', httpMethod: 'POST' }, + typeVersion: 1 + }, + { + id: '2', + name: 'Sticky Note', + type: 'n8n-nodes-base.stickyNote', + position: [300, 100], + parameters: { content: 'This is a note' }, + typeVersion: 1 + } + ], + connections: {} + }; + + const result = await workflowValidator.validateWorkflow(workflow); + + expect(result).toBeDefined(); + expect(result.statistics.totalNodes).toBe(1); // Only webhook, sticky note excluded + expect(result.statistics.enabledNodes).toBe(1); + }); + + test('should allow legitimate loops in cycle detection', async () => { + const workflow = { + nodes: [ + { + id: '1', + name: 'Manual Trigger', + type: 'n8n-nodes-base.manualTrigger', + position: [100, 200], + parameters: {}, + typeVersion: 1 + }, + { + id: '2', + name: 'SplitInBatches', + type: 'n8n-nodes-base.splitInBatches', + position: [300, 200], + parameters: { batchSize: 1 }, + typeVersion: 1 + }, + { + id: '3', + name: 'Set', + type: 'n8n-nodes-base.set', + position: [500, 200], + parameters: { values: {} }, + typeVersion: 1 + } + ], + connections: { + 'Manual Trigger': { + main: [[{ node: 'SplitInBatches', type: 'main', index: 0 }]] + }, + 'SplitInBatches': { + main: [ + [{ node: 'Set', type: 'main', index: 0 }], // Done output + [{ node: 'Set', type: 'main', index: 0 }] // Loop output + ] + }, + 'Set': { + main: [[{ node: 'SplitInBatches', type: 'main', index: 0 }]] // Loop back + } + } + }; + + const result = await workflowValidator.validateWorkflow(workflow); + + expect(result).toBeDefined(); + // Should not report cycle error for legitimate SplitInBatches loop + const cycleErrors = result.errors.filter(e => e.message.includes('cycle')); + expect(cycleErrors).toHaveLength(0); + }); + }); + + describe('Issue #68: Better error recovery suggestions', () => { + test('should provide recovery suggestions for invalid node types', async () => { + const workflow = { + nodes: [ + { + id: '1', + name: 'Invalid Node', + type: 'invalid-node-type', + position: [100, 200], + parameters: {}, + typeVersion: 1 + } + ], + connections: {} + }; + + const result = await workflowValidator.validateWorkflow(workflow); + + expect(result).toBeDefined(); + expect(result.valid).toBe(false); + expect(result.suggestions.length).toBeGreaterThan(0); + + // Should contain recovery suggestions + const recoveryStarted = result.suggestions.some(s => s.includes('🔧 RECOVERY')); + expect(recoveryStarted).toBe(true); + }); + + test('should provide recovery suggestions for connection errors', async () => { + const workflow = { + nodes: [ + { + id: '1', + name: 'Webhook', + type: 'n8n-nodes-base.webhook', + position: [100, 200], + parameters: { path: '/test', httpMethod: 'POST' }, + typeVersion: 1 + } + ], + connections: { + 'Webhook': { + main: [[{ node: 'NonExistentNode', type: 'main', index: 0 }]] + } + } + }; + + const result = await workflowValidator.validateWorkflow(workflow); + + expect(result).toBeDefined(); + expect(result.valid).toBe(false); + expect(result.suggestions.length).toBeGreaterThan(0); + + // Should contain connection recovery suggestions + const connectionRecovery = result.suggestions.some(s => + s.includes('Connection errors detected') || s.includes('connection') + ); + expect(connectionRecovery).toBe(true); + }); + + test('should provide workflow for multiple errors', async () => { + const workflow = { + nodes: [ + { + id: '1', + name: 'Invalid Node 1', + type: 'invalid-type-1', + position: [100, 200], + parameters: {} + // Missing typeVersion + }, + { + id: '2', + name: 'Invalid Node 2', + type: 'invalid-type-2', + position: [300, 200], + parameters: {} + // Missing typeVersion + }, + { + id: '3', + name: 'Invalid Node 3', + type: 'invalid-type-3', + position: [500, 200], + parameters: {} + // Missing typeVersion + } + ], + connections: { + 'Invalid Node 1': { + main: [[{ node: 'NonExistent', type: 'main', index: 0 }]] + } + } + }; + + const result = await workflowValidator.validateWorkflow(workflow); + + expect(result).toBeDefined(); + expect(result.valid).toBe(false); + expect(result.errors.length).toBeGreaterThan(3); + + // Should provide step-by-step recovery workflow + const workflowSuggestion = result.suggestions.some(s => + s.includes('SUGGESTED WORKFLOW') && s.includes('Too many errors detected') + ); + expect(workflowSuggestion).toBe(true); + }); + }); + + describe('Enhanced Input Validation', () => { + test('should validate tool parameters with schemas', () => { + // Test validate_node_operation parameters + const validationResult = ToolValidation.validateNodeOperation({ + nodeType: 'nodes-base.webhook', + config: { path: '/test' }, + profile: 'ai-friendly' + }); + + expect(validationResult.valid).toBe(true); + expect(validationResult.errors).toHaveLength(0); + }); + + test('should reject invalid parameters', () => { + const validationResult = ToolValidation.validateNodeOperation({ + nodeType: 123, // Invalid type + config: 'not an object', // Invalid type + profile: 'invalid-profile' // Invalid enum value + }); + + expect(validationResult.valid).toBe(false); + expect(validationResult.errors.length).toBeGreaterThan(0); + }); + + test('should format validation errors properly', () => { + const validationResult = ToolValidation.validateNodeOperation({ + nodeType: null, + config: null + }); + + const errorMessage = Validator.formatErrors(validationResult, 'validate_node_operation'); + + expect(errorMessage).toContain('validate_node_operation: Validation failed:'); + expect(errorMessage).toContain('nodeType'); + expect(errorMessage).toContain('config'); + }); + }); +}); \ No newline at end of file