From 0778c55d85d5f45d8331addaceb5e48c4727af78 Mon Sep 17 00:00:00 2001 From: czlonkowski <56956555+czlonkowski@users.noreply.github.com> Date: Fri, 24 Oct 2025 14:17:30 +0200 Subject: [PATCH] fix: add warnings for If/Switch node connection parameters (issue #360) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implemented a warning system to guide users toward using smart parameters (branch="true"/"false" for If nodes, case=N for Switch nodes) instead of sourceIndex, which can lead to incorrect branch routing. Changes: - Added warnings property to WorkflowDiffResult interface - Warnings generated when sourceIndex used with If/Switch nodes - Enhanced tool documentation with CRITICAL pitfalls - Added regression tests reproducing issue #360 - Version bump to 2.22.1 The branch parameter functionality works correctly - this fix adds helpful warnings to prevent users from accidentally using the less intuitive sourceIndex parameter. Conceived by Romuald Członkowski - https://www.aiadvisors.pl/en 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- CHANGELOG.md | 49 ++++++++ package.json | 2 +- .../n8n-update-partial-workflow.ts | 2 + src/services/workflow-diff-engine.ts | 28 ++++- src/types/workflow-diff.ts | 1 + .../services/workflow-diff-engine.test.ts | 107 ++++++++++++++++++ 6 files changed, 186 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b1a653..bc4783a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,55 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### 🐛 Bug Fixes + +**Issue #360: Enhanced Warnings for If/Switch Node Connection Parameters** + +Fixed issue where users could unintentionally place multiple If node connections on the same branch (TRUE/FALSE) when using `sourceIndex` parameter instead of the recommended `branch` parameter. The system now provides helpful warnings to guide users toward better practices. + +#### What Was Fixed + +1. **New Warning System**: + - Warns when using `sourceIndex` with If nodes - suggests `branch="true"` or `branch="false"` instead + - Warns when using `sourceIndex` with Switch nodes - suggests `case=N` instead + - Explains the correct branch structure: `main[0]=TRUE branch, main[1]=FALSE branch` + +2. **Enhanced Documentation**: + - Added **CRITICAL** pitfalls to `n8n_update_partial_workflow` tool documentation + - Clear guidance that using `sourceIndex=0` for multiple connections puts them ALL on the TRUE branch + - Examples showing correct vs. incorrect usage + +3. **Type System Improvements**: + - Added `warnings` field to `WorkflowDiffResult` interface + - Warnings are non-blocking (operations still succeed) + - Differentiated from errors for better UX + +#### Behavior + +The existing `branch` parameter works correctly and has comprehensive test coverage: +- `branch="true"` → routes to `main[0]` (TRUE path) +- `branch="false"` → routes to `main[1]` (FALSE path) + +The issue was that users who didn't know about the `branch` parameter would naturally use `sourceIndex`, which led to incorrect branch routing. + +#### Example Warning + +``` +Connection to If node "Check Condition" uses sourceIndex=0. +Consider using branch="true" or branch="false" for better clarity. +If node outputs: main[0]=TRUE branch, main[1]=FALSE branch. +``` + +#### Test Coverage + +- Added regression tests that reproduce the exact issue from #360 +- Verify warnings are generated for If and Switch nodes +- Confirm existing smart parameter tests still pass + +**Conceived by Romuald Członkowski - https://www.aiadvisors.pl/en** + +--- + ### ✨ New Features **Auto-Update Node Versions with Smart Migration** diff --git a/package.json b/package.json index ba3bdb6..4a608cf 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "n8n-mcp", - "version": "2.22.0", + "version": "2.22.1", "description": "Integration between n8n workflow automation and Model Context Protocol (MCP)", "main": "dist/index.js", "types": "dist/index.d.ts", diff --git a/src/mcp/tool-docs/workflow_management/n8n-update-partial-workflow.ts b/src/mcp/tool-docs/workflow_management/n8n-update-partial-workflow.ts index 0acfbe5..c9822a0 100644 --- a/src/mcp/tool-docs/workflow_management/n8n-update-partial-workflow.ts +++ b/src/mcp/tool-docs/workflow_management/n8n-update-partial-workflow.ts @@ -272,6 +272,8 @@ Please choose a different name. 'Use "updates" property for updateNode operations: {type: "updateNode", updates: {...}}', 'Smart parameters (branch, case) only work with IF and Switch nodes - ignored for other node types', 'Explicit sourceIndex overrides smart parameters (branch, case) if both provided', + '**CRITICAL**: For If nodes, ALWAYS use branch="true"/"false" instead of sourceIndex. Using sourceIndex=0 for multiple connections will put them ALL on the TRUE branch (main[0]), breaking your workflow logic!', + '**CRITICAL**: For Switch nodes, ALWAYS use case=N instead of sourceIndex. Using same sourceIndex for multiple connections will put them on the same case output.', 'cleanStaleConnections removes ALL broken connections - cannot be selective', 'replaceConnections overwrites entire connections object - all previous connections lost', '**Auto-sanitization behavior**: Binary operators (equals, contains) automatically have singleValue removed; unary operators (isEmpty, isNotEmpty) automatically get singleValue:true added', diff --git a/src/services/workflow-diff-engine.ts b/src/services/workflow-diff-engine.ts index 6b0ca4b..ab5baa9 100644 --- a/src/services/workflow-diff-engine.ts +++ b/src/services/workflow-diff-engine.ts @@ -38,6 +38,8 @@ const logger = new Logger({ prefix: '[WorkflowDiffEngine]' }); export class WorkflowDiffEngine { // Track node name changes during operations for connection reference updates private renameMap: Map = new Map(); + // Track warnings during operation processing + private warnings: WorkflowDiffValidationError[] = []; /** * Apply diff operations to a workflow @@ -47,8 +49,9 @@ export class WorkflowDiffEngine { request: WorkflowDiffRequest ): Promise { try { - // Reset rename tracking for this diff operation + // Reset tracking for this diff operation this.renameMap.clear(); + this.warnings = []; // Clone workflow to avoid modifying original const workflowCopy = JSON.parse(JSON.stringify(workflow)); @@ -114,6 +117,7 @@ export class WorkflowDiffEngine { ? 'Validation successful. All operations are valid.' : `Validation completed with ${errors.length} errors.`, errors: errors.length > 0 ? errors : undefined, + warnings: this.warnings.length > 0 ? this.warnings : undefined, applied: appliedIndices, failed: failedIndices }; @@ -126,6 +130,7 @@ export class WorkflowDiffEngine { operationsApplied: appliedIndices.length, message: `Applied ${appliedIndices.length} operations, ${failedIndices.length} failed (continueOnError mode)`, errors: errors.length > 0 ? errors : undefined, + warnings: this.warnings.length > 0 ? this.warnings : undefined, applied: appliedIndices, failed: failedIndices }; @@ -213,7 +218,8 @@ export class WorkflowDiffEngine { success: true, workflow: workflowCopy, operationsApplied, - message: `Successfully applied ${operationsApplied} operations (${nodeOperations.length} node ops, ${otherOperations.length} other ops)` + message: `Successfully applied ${operationsApplied} operations (${nodeOperations.length} node ops, ${otherOperations.length} other ops)`, + warnings: this.warnings.length > 0 ? this.warnings : undefined }; } } catch (error) { @@ -685,6 +691,24 @@ export class WorkflowDiffEngine { sourceIndex = operation.case; } + // Validation: Warn if using sourceIndex with If/Switch nodes without smart parameters + if (sourceNode && operation.sourceIndex !== undefined && operation.branch === undefined && operation.case === undefined) { + if (sourceNode.type === 'n8n-nodes-base.if') { + this.warnings.push({ + operation: -1, // Not tied to specific operation index in request + message: `Connection to If node "${operation.source}" uses sourceIndex=${operation.sourceIndex}. ` + + `Consider using branch="true" or branch="false" for better clarity. ` + + `If node outputs: main[0]=TRUE branch, main[1]=FALSE branch.` + }); + } else if (sourceNode.type === 'n8n-nodes-base.switch') { + this.warnings.push({ + operation: -1, // Not tied to specific operation index in request + message: `Connection to Switch node "${operation.source}" uses sourceIndex=${operation.sourceIndex}. ` + + `Consider using case=N for better clarity (case=0 for first output, case=1 for second, etc.).` + }); + } + } + return { sourceOutput, sourceIndex }; } diff --git a/src/types/workflow-diff.ts b/src/types/workflow-diff.ts index e76b757..e35169d 100644 --- a/src/types/workflow-diff.ts +++ b/src/types/workflow-diff.ts @@ -170,6 +170,7 @@ export interface WorkflowDiffResult { success: boolean; workflow?: any; // Updated workflow if successful errors?: WorkflowDiffValidationError[]; + warnings?: WorkflowDiffValidationError[]; // Non-blocking warnings (e.g., parameter suggestions) operationsApplied?: number; message?: string; applied?: number[]; // Indices of successfully applied operations (when continueOnError is true) diff --git a/tests/unit/services/workflow-diff-engine.test.ts b/tests/unit/services/workflow-diff-engine.test.ts index df4cb47..42dddb1 100644 --- a/tests/unit/services/workflow-diff-engine.test.ts +++ b/tests/unit/services/workflow-diff-engine.test.ts @@ -1418,6 +1418,113 @@ describe('WorkflowDiffEngine', () => { expect(result.workflow!.connections['Switch']['main'][2][0].node).toBe('Handler'); expect(result.workflow!.connections['Switch']['main'][1]).toEqual([]); }); + + it('should warn when using sourceIndex with If node (issue #360)', async () => { + const addIF: any = { + type: 'addNode', + node: { + name: 'Check Condition', + type: 'n8n-nodes-base.if', + position: [400, 300] + } + }; + + const addSuccess: any = { + type: 'addNode', + node: { + name: 'Success Handler', + type: 'n8n-nodes-base.set', + position: [600, 200] + } + }; + + const addError: any = { + type: 'addNode', + node: { + name: 'Error Handler', + type: 'n8n-nodes-base.set', + position: [600, 400] + } + }; + + // BAD: Using sourceIndex with If node (reproduces issue #360) + const connectSuccess: any = { + type: 'addConnection', + source: 'Check Condition', + target: 'Success Handler', + sourceIndex: 0 // Should use branch="true" instead + }; + + const connectError: any = { + type: 'addConnection', + source: 'Check Condition', + target: 'Error Handler', + sourceIndex: 0 // Should use branch="false" instead - both will end up in main[0]! + }; + + const request: WorkflowDiffRequest = { + id: 'test-workflow', + operations: [addIF, addSuccess, addError, connectSuccess, connectError] + }; + + const result = await diffEngine.applyDiff(baseWorkflow, request); + + expect(result.success).toBe(true); + + // Should produce warnings + expect(result.warnings).toBeDefined(); + expect(result.warnings!.length).toBe(2); + expect(result.warnings![0].message).toContain('Consider using branch="true" or branch="false"'); + expect(result.warnings![0].message).toContain('If node outputs: main[0]=TRUE branch, main[1]=FALSE branch'); + expect(result.warnings![1].message).toContain('Consider using branch="true" or branch="false"'); + + // Both connections end up in main[0] (the bug behavior) + expect(result.workflow!.connections['Check Condition']['main'][0].length).toBe(2); + expect(result.workflow!.connections['Check Condition']['main'][0][0].node).toBe('Success Handler'); + expect(result.workflow!.connections['Check Condition']['main'][0][1].node).toBe('Error Handler'); + }); + + it('should warn when using sourceIndex with Switch node', async () => { + const addSwitch: any = { + type: 'addNode', + node: { + name: 'Switch', + type: 'n8n-nodes-base.switch', + position: [400, 300] + } + }; + + const addHandler: any = { + type: 'addNode', + node: { + name: 'Handler', + type: 'n8n-nodes-base.set', + position: [600, 300] + } + }; + + // BAD: Using sourceIndex with Switch node + const connect: any = { + type: 'addConnection', + source: 'Switch', + target: 'Handler', + sourceIndex: 1 // Should use case=1 instead + }; + + const request: WorkflowDiffRequest = { + id: 'test-workflow', + operations: [addSwitch, addHandler, connect] + }; + + const result = await diffEngine.applyDiff(baseWorkflow, request); + + expect(result.success).toBe(true); + + // Should produce warning + expect(result.warnings).toBeDefined(); + expect(result.warnings!.length).toBe(1); + expect(result.warnings![0].message).toContain('Consider using case=N for better clarity'); + }); }); describe('AddConnection with sourceIndex (Phase 0 Fix)', () => {