diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f1f18f..8e317e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [2.37.0] - 2026-03-14 + +### Fixed + +- **Unary operator sanitization** (Issue #592): Added missing `empty`, `notEmpty`, `exists`, `notExists` operators to the sanitizer's unary operator list, preventing IF/Switch node corruption during partial updates +- **Positional connection array preservation** (Issue #610): `removeNode` and `cleanStaleConnections` now trim only trailing empty arrays, preserving intermediate positional indices for IF/Switch multi-output nodes +- **Scoped sanitization**: Auto-sanitization now only runs on nodes that were actually added or updated, preventing unrelated nodes (e.g., HTTP Request parameters) from being silently modified +- **Activate/deactivate 415 errors** (Issue #633): Added empty body `{}` to POST calls for workflow activation/deactivation endpoints +- **Zod error readability** (Issue #630): Validation errors now return human-readable `"path: message"` strings instead of raw Zod error objects +- **updateNode error hints** (Issue #623): Improved error message when `updates` parameter is missing, showing correct structure with `nodeId`/`nodeName` and `updates` fields +- **removeConnection after removeNode** (Issue #624): When a node was already removed by a prior `removeNode` operation, the error message now explains that connections were automatically cleaned up +- **Connection type coercion** (Issue #629): `sourceOutput` and `targetInput` are now coerced to strings, handling numeric values (0, 1) passed by MCP clients + +### Added + +- **`saved` field in responses** (Issue #625): All `n8n_update_partial_workflow` responses now include `saved: true/false` to distinguish whether the workflow was persisted to n8n +- **Tag operations via dedicated API** (Issue #599): `addTag`/`removeTag` now use the n8n tag API (`PUT /workflows/{id}/tags`) instead of embedding tags in the workflow body, fixing silent tag failures. Includes automatic tag creation, case-insensitive name resolution, and last-operation-wins reconciliation for conflicting add/remove +- **`updateWorkflowTags` API client method**: New method on `N8nApiClient` for managing workflow tag associations via the dedicated endpoint +- **`operationsApplied` in top-level response**: Promoted from nested `details` to top-level for easier consumption by MCP clients + +Conceived by Romuald Czlonkowski - https://www.aiadvisors.pl/en + ## [2.36.2] - 2026-03-14 ### Changed diff --git a/src/database/shared-database.ts b/src/database/shared-database.ts index b55c01d..a169c79 100644 --- a/src/database/shared-database.ts +++ b/src/database/shared-database.ts @@ -11,6 +11,7 @@ * Issue: https://github.com/czlonkowski/n8n-mcp/issues/XXX */ +import path from 'path'; import { DatabaseAdapter, createDatabaseAdapter } from './database-adapter'; import { NodeRepository } from './node-repository'; import { TemplateService } from '../templates/template-service'; @@ -43,21 +44,26 @@ let initializationPromise: Promise | null = null; * @returns Shared database state with connection and services */ export async function getSharedDatabase(dbPath: string): Promise { + // Normalize to a canonical absolute path so that callers using different + // relative or join-based paths (e.g. "./data/nodes.db" vs an absolute path) + // resolve to the same string and do not trigger a false "different path" error. + const normalizedPath = dbPath === ':memory:' ? dbPath : path.resolve(dbPath); + // If already initialized with the same path, increment ref count and return - if (sharedState && sharedState.initialized && sharedState.dbPath === dbPath) { + if (sharedState && sharedState.initialized && sharedState.dbPath === normalizedPath) { sharedState.refCount++; logger.debug('Reusing shared database connection', { refCount: sharedState.refCount, - dbPath + dbPath: normalizedPath }); return sharedState; } // If already initialized with a DIFFERENT path, this is a configuration error - if (sharedState && sharedState.initialized && sharedState.dbPath !== dbPath) { + if (sharedState && sharedState.initialized && sharedState.dbPath !== normalizedPath) { logger.error('Attempted to initialize shared database with different path', { existingPath: sharedState.dbPath, - requestedPath: dbPath + requestedPath: normalizedPath }); throw new Error(`Shared database already initialized with different path: ${sharedState.dbPath}`); } @@ -69,7 +75,7 @@ export async function getSharedDatabase(dbPath: string): Promise = Array.isArray(updatedWorkflow.tags) + ? updatedWorkflow.tags.map((t: any) => typeof t === 'object' ? { id: t.id, name: t.name } : { id: '', name: t }) + : []; + + // Resolve tag names to IDs + const allTags = await client.listTags(); + const tagMap = new Map(); + for (const t of allTags.data) { + if (t.id) tagMap.set(t.name.toLowerCase(), t.id); + } + + // Create any tags that don't exist yet + for (const tagName of (diffResult.tagsToAdd || [])) { + if (!tagMap.has(tagName.toLowerCase())) { + try { + const newTag = await client.createTag({ name: tagName }); + if (newTag.id) tagMap.set(tagName.toLowerCase(), newTag.id); + } catch (createErr) { + tagWarnings.push(`Failed to create tag "${tagName}": ${createErr instanceof Error ? createErr.message : 'Unknown error'}`); + } + } + } + + // Compute final tag set — resolve string-type tags via tagMap + const currentTagIds = new Set(); + for (const et of existingTags) { + if (et.id) { + currentTagIds.add(et.id); + } else { + const resolved = tagMap.get(et.name.toLowerCase()); + if (resolved) currentTagIds.add(resolved); + } + } + + for (const tagName of (diffResult.tagsToAdd || [])) { + const tagId = tagMap.get(tagName.toLowerCase()); + if (tagId) currentTagIds.add(tagId); + } + + for (const tagName of (diffResult.tagsToRemove || [])) { + const tagId = tagMap.get(tagName.toLowerCase()); + if (tagId) currentTagIds.delete(tagId); + } + + // Update workflow tags via dedicated API + await client.updateWorkflowTags(input.id, Array.from(currentTagIds)); + } catch (tagError) { + tagWarnings.push(`Tag update failed: ${tagError instanceof Error ? tagError.message : 'Unknown error'}`); + logger.warn('Tag operations failed (non-blocking)', tagError); + } + } + // Handle activation/deactivation if requested let finalWorkflow = updatedWorkflow; let activationMessage = ''; @@ -319,6 +378,7 @@ export async function handleUpdatePartialWorkflow( logger.error('Failed to activate workflow after update', activationError); return { success: false, + saved: true, error: 'Workflow updated successfully but activation failed', details: { workflowUpdated: true, @@ -334,6 +394,7 @@ export async function handleUpdatePartialWorkflow( logger.error('Failed to deactivate workflow after update', deactivationError); return { success: false, + saved: true, error: 'Workflow updated successfully but deactivation failed', details: { workflowUpdated: true, @@ -363,6 +424,7 @@ export async function handleUpdatePartialWorkflow( return { success: true, + saved: true, data: { id: finalWorkflow.id, name: finalWorkflow.name, @@ -375,7 +437,7 @@ export async function handleUpdatePartialWorkflow( applied: diffResult.applied, failed: diffResult.failed, errors: diffResult.errors, - warnings: diffResult.warnings + warnings: mergeWarnings(diffResult.warnings, tagWarnings) } }; } catch (error) { @@ -413,7 +475,9 @@ export async function handleUpdatePartialWorkflow( return { success: false, error: 'Invalid input', - details: { errors: error.errors } + details: { + errors: error.errors.map(e => `${e.path.join('.')}: ${e.message}`) + } }; } @@ -425,6 +489,21 @@ export async function handleUpdatePartialWorkflow( } } +/** + * Merge diff engine warnings with tag operation warnings into a single array. + * Returns undefined when there are no warnings to keep the response clean. + */ +function mergeWarnings( + diffWarnings: WorkflowDiffValidationError[] | undefined, + tagWarnings: string[] +): WorkflowDiffValidationError[] | undefined { + const merged: WorkflowDiffValidationError[] = [ + ...(diffWarnings || []), + ...tagWarnings.map(w => ({ operation: -1, message: w })) + ]; + return merged.length > 0 ? merged : undefined; +} + /** * Infer intent from operations when not explicitly provided */ diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 08e6832..b6993b1 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -210,6 +210,13 @@ export class N8NDocumentationMCPServer { } }); + // Attach a no-op catch handler to prevent Node.js from flagging this as an + // unhandled rejection in the interval between construction and the first + // await of this.initialized (via ensureInitialized). This does NOT suppress + // the error: the original this.initialized promise still rejects, and + // ensureInitialized() will re-throw it when awaited. + this.initialized.catch(() => {}); + logger.info('Initializing n8n Documentation MCP server'); this.server = new Server( diff --git a/src/services/n8n-api-client.ts b/src/services/n8n-api-client.ts index cf16768..d8adcb0 100644 --- a/src/services/n8n-api-client.ts +++ b/src/services/n8n-api-client.ts @@ -254,7 +254,7 @@ export class N8nApiClient { async activateWorkflow(id: string): Promise { try { - const response = await this.client.post(`/workflows/${id}/activate`); + const response = await this.client.post(`/workflows/${id}/activate`, {}); return response.data; } catch (error) { throw handleN8nApiError(error); @@ -263,7 +263,7 @@ export class N8nApiClient { async deactivateWorkflow(id: string): Promise { try { - const response = await this.client.post(`/workflows/${id}/deactivate`); + const response = await this.client.post(`/workflows/${id}/deactivate`, {}); return response.data; } catch (error) { throw handleN8nApiError(error); @@ -493,6 +493,15 @@ export class N8nApiClient { } } + async updateWorkflowTags(workflowId: string, tagIds: string[]): Promise { + try { + const response = await this.client.put(`/workflows/${workflowId}/tags`, tagIds.filter(id => id).map(id => ({ id }))); + return response.data; + } catch (error) { + throw handleN8nApiError(error); + } + } + // Source Control Management (Enterprise feature) async getSourceControlStatus(): Promise { try { diff --git a/src/services/node-sanitizer.ts b/src/services/node-sanitizer.ts index 4361542..9e653c3 100644 --- a/src/services/node-sanitizer.ts +++ b/src/services/node-sanitizer.ts @@ -41,7 +41,7 @@ export function sanitizeWorkflowNodes(workflow: any): any { return { ...workflow, - nodes: workflow.nodes.map((node: any) => sanitizeNode(node)) + nodes: workflow.nodes.map(sanitizeNode) }; } @@ -121,9 +121,7 @@ function sanitizeFilterConditions(conditions: any): any { // Sanitize conditions array if (sanitized.conditions && Array.isArray(sanitized.conditions)) { - sanitized.conditions = sanitized.conditions.map((condition: any) => - sanitizeCondition(condition) - ); + sanitized.conditions = sanitized.conditions.map(sanitizeCondition); } return sanitized; @@ -214,18 +212,25 @@ function inferDataType(operation: string): string { return 'boolean'; } - // Number operations + // Number operations (partial match to catch variants like "greaterThan" containing "gt") const numberOps = ['isNumeric', 'gt', 'gte', 'lt', 'lte']; if (numberOps.some(op => operation.includes(op))) { return 'number'; } - // Date operations + // Date operations (partial match to catch variants like "isAfter" containing "after") const dateOps = ['after', 'before', 'afterDate', 'beforeDate']; if (dateOps.some(op => operation.includes(op))) { return 'dateTime'; } + // Object operations: empty/notEmpty/exists/notExists are generic object-level checks + // (distinct from isEmpty/isNotEmpty which are boolean-typed operations) + const objectOps = ['empty', 'notEmpty', 'exists', 'notExists']; + if (objectOps.includes(operation)) { + return 'object'; + } + // Default to string return 'string'; } @@ -239,7 +244,11 @@ function isUnaryOperator(operation: string): boolean { 'isNotEmpty', 'true', 'false', - 'isNumeric' + 'isNumeric', + 'empty', + 'notEmpty', + 'exists', + 'notExists' ]; return unaryOps.includes(operation); } diff --git a/src/services/workflow-diff-engine.ts b/src/services/workflow-diff-engine.ts index 6c330c6..d06da2a 100644 --- a/src/services/workflow-diff-engine.ts +++ b/src/services/workflow-diff-engine.ts @@ -38,11 +38,22 @@ import { isActivatableTrigger } from '../utils/node-type-utils'; const logger = new Logger({ prefix: '[WorkflowDiffEngine]' }); +/** + * Not safe for concurrent use — create a new instance per request. + * Instance state is reset at the start of each applyDiff() call. + */ 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[] = []; + // Track which nodes were added/updated so sanitization only runs on them + private modifiedNodeIds = new Set(); + // Track removed node names for better error messages + private removedNodeNames = new Set(); + // Track tag operations for dedicated API calls + private tagsToAdd: string[] = []; + private tagsToRemove: string[] = []; /** * Apply diff operations to a workflow @@ -55,6 +66,10 @@ export class WorkflowDiffEngine { // Reset tracking for this diff operation this.renameMap.clear(); this.warnings = []; + this.modifiedNodeIds.clear(); + this.removedNodeNames.clear(); + this.tagsToAdd = []; + this.tagsToRemove = []; // Clone workflow to avoid modifying original const workflowCopy = JSON.parse(JSON.stringify(workflow)); @@ -135,7 +150,9 @@ export class WorkflowDiffEngine { errors: errors.length > 0 ? errors : undefined, warnings: this.warnings.length > 0 ? this.warnings : undefined, applied: appliedIndices, - failed: failedIndices + failed: failedIndices, + tagsToAdd: this.tagsToAdd.length > 0 ? this.tagsToAdd : undefined, + tagsToRemove: this.tagsToRemove.length > 0 ? this.tagsToRemove : undefined }; } else { // Atomic mode: all operations must succeed @@ -201,12 +218,16 @@ export class WorkflowDiffEngine { } } - // Sanitize ALL nodes in the workflow after operations are applied - // This ensures existing invalid nodes (e.g., binary operators with singleValue: true) - // are fixed automatically when any update is made to the workflow - workflowCopy.nodes = workflowCopy.nodes.map((node: WorkflowNode) => sanitizeNode(node)); - - logger.debug('Applied full-workflow sanitization to all nodes'); + // Sanitize only modified nodes to avoid breaking unrelated nodes (#592) + if (this.modifiedNodeIds.size > 0) { + workflowCopy.nodes = workflowCopy.nodes.map((node: WorkflowNode) => { + if (this.modifiedNodeIds.has(node.id)) { + return sanitizeNode(node); + } + return node; + }); + logger.debug(`Sanitized ${this.modifiedNodeIds.size} modified nodes`); + } // If validateOnly flag is set, return success without applying if (request.validateOnly) { @@ -233,7 +254,9 @@ export class WorkflowDiffEngine { message: `Successfully applied ${operationsApplied} operations (${nodeOperations.length} node ops, ${otherOperations.length} other ops)`, warnings: this.warnings.length > 0 ? this.warnings : undefined, shouldActivate: shouldActivate || undefined, - shouldDeactivate: shouldDeactivate || undefined + shouldDeactivate: shouldDeactivate || undefined, + tagsToAdd: this.tagsToAdd.length > 0 ? this.tagsToAdd : undefined, + tagsToRemove: this.tagsToRemove.length > 0 ? this.tagsToRemove : undefined }; } } catch (error) { @@ -248,7 +271,6 @@ export class WorkflowDiffEngine { } } - /** * Validate a single operation */ @@ -405,7 +427,7 @@ export class WorkflowDiffEngine { // Check for missing required parameter if (!operation.updates) { - return `Missing required parameter 'updates'. The updateNode operation requires an 'updates' object containing properties to modify. Example: {type: "updateNode", nodeId: "abc", updates: {name: "New Name"}}`; + return `Missing required parameter 'updates'. The updateNode operation requires an 'updates' object. Correct structure: {type: "updateNode", nodeId: "abc-123" OR nodeName: "My Node", updates: {name: "New Name", "parameters.url": "https://example.com"}}`; } const node = this.findNode(workflow, operation.nodeId, operation.nodeName); @@ -510,12 +532,18 @@ export class WorkflowDiffEngine { const targetNode = this.findNode(workflow, operation.target, operation.target); if (!sourceNode) { + if (this.removedNodeNames.has(operation.source)) { + return `Source node "${operation.source}" was already removed by a prior removeNode operation. Its connections were automatically cleaned up — no separate removeConnection needed.`; + } const availableNodes = workflow.nodes .map(n => `"${n.name}" (id: ${n.id.substring(0, 8)}...)`) .join(', '); return `Source node not found: "${operation.source}". Available nodes: ${availableNodes}. Tip: Use node ID for names with special characters.`; } if (!targetNode) { + if (this.removedNodeNames.has(operation.target)) { + return `Target node "${operation.target}" was already removed by a prior removeNode operation. Its connections were automatically cleaned up — no separate removeConnection needed.`; + } const availableNodes = workflow.nodes .map(n => `"${n.name}" (id: ${n.id.substring(0, 8)}...)`) .join(', '); @@ -614,13 +642,16 @@ export class WorkflowDiffEngine { // Sanitize node to ensure complete metadata (filter options, operator structure, etc.) const sanitizedNode = sanitizeNode(newNode); + this.modifiedNodeIds.add(sanitizedNode.id); workflow.nodes.push(sanitizedNode); } private applyRemoveNode(workflow: Workflow, operation: RemoveNodeOperation): void { const node = this.findNode(workflow, operation.nodeId, operation.nodeName); if (!node) return; - + + this.removedNodeNames.add(node.name); + // Remove node from array const index = workflow.nodes.findIndex(n => n.id === node.id); if (index !== -1) { @@ -631,30 +662,36 @@ export class WorkflowDiffEngine { delete workflow.connections[node.name]; // Remove all connections to this node - Object.keys(workflow.connections).forEach(sourceName => { - const sourceConnections = workflow.connections[sourceName]; - Object.keys(sourceConnections).forEach(outputName => { - sourceConnections[outputName] = sourceConnections[outputName].map(connections => + for (const [sourceName, sourceConnections] of Object.entries(workflow.connections)) { + for (const [outputName, outputConns] of Object.entries(sourceConnections)) { + sourceConnections[outputName] = outputConns.map(connections => connections.filter(conn => conn.node !== node.name) - ).filter(connections => connections.length > 0); - - // Clean up empty arrays - if (sourceConnections[outputName].length === 0) { + ); + + // Trim trailing empty arrays only (preserve intermediate empty arrays for positional indices) + const trimmed = sourceConnections[outputName]; + while (trimmed.length > 0 && trimmed[trimmed.length - 1].length === 0) { + trimmed.pop(); + } + + if (trimmed.length === 0) { delete sourceConnections[outputName]; } - }); - + } + // Clean up empty connection objects if (Object.keys(sourceConnections).length === 0) { delete workflow.connections[sourceName]; } - }); + } } private applyUpdateNode(workflow: Workflow, operation: UpdateNodeOperation): void { const node = this.findNode(workflow, operation.nodeId, operation.nodeName); if (!node) return; + this.modifiedNodeIds.add(node.id); + // Track node renames for connection reference updates if (operation.updates.name && operation.updates.name !== node.name) { const oldName = node.name; @@ -706,8 +743,8 @@ export class WorkflowDiffEngine { ): { sourceOutput: string; sourceIndex: number } { const sourceNode = this.findNode(workflow, operation.source, operation.source); - // Start with explicit values or defaults - let sourceOutput = operation.sourceOutput ?? 'main'; + // Start with explicit values or defaults, coercing to correct types + let sourceOutput = String(operation.sourceOutput ?? 'main'); let sourceIndex = operation.sourceIndex ?? 0; // Smart parameter: branch (for IF nodes) @@ -758,7 +795,8 @@ export class WorkflowDiffEngine { // Use nullish coalescing to properly handle explicit 0 values // Default targetInput to sourceOutput to preserve connection type for AI connections (ai_tool, ai_memory, etc.) - const targetInput = operation.targetInput ?? sourceOutput; + // Coerce to string to handle numeric values passed as sourceOutput/targetInput + const targetInput = String(operation.targetInput ?? sourceOutput); const targetIndex = operation.targetIndex ?? 0; // Initialize source node connections object @@ -795,18 +833,14 @@ export class WorkflowDiffEngine { private applyRemoveConnection(workflow: Workflow, operation: RemoveConnectionOperation): void { const sourceNode = this.findNode(workflow, operation.source, operation.source); const targetNode = this.findNode(workflow, operation.target, operation.target); - // If ignoreErrors is true, silently succeed even if nodes don't exist if (!sourceNode || !targetNode) { - if (operation.ignoreErrors) { - return; // Gracefully handle missing nodes - } - return; // Should never reach here if validation passed, but safety check + return; } - const sourceOutput = operation.sourceOutput || 'main'; + const sourceOutput = String(operation.sourceOutput ?? 'main'); const connections = workflow.connections[sourceNode.name]?.[sourceOutput]; if (!connections) return; - + // Remove connection from all indices workflow.connections[sourceNode.name][sourceOutput] = connections.map(conns => conns.filter(conn => conn.node !== targetNode.name) @@ -877,20 +911,26 @@ export class WorkflowDiffEngine { } private applyAddTag(workflow: Workflow, operation: AddTagOperation): void { - if (!workflow.tags) { - workflow.tags = []; + // Track for dedicated API call instead of modifying workflow.tags directly + // Reconcile: if previously marked for removal, cancel the removal instead + const removeIdx = this.tagsToRemove.indexOf(operation.tag); + if (removeIdx !== -1) { + this.tagsToRemove.splice(removeIdx, 1); } - if (!workflow.tags.includes(operation.tag)) { - workflow.tags.push(operation.tag); + if (!this.tagsToAdd.includes(operation.tag)) { + this.tagsToAdd.push(operation.tag); } } private applyRemoveTag(workflow: Workflow, operation: RemoveTagOperation): void { - if (!workflow.tags) return; - - const index = workflow.tags.indexOf(operation.tag); - if (index !== -1) { - workflow.tags.splice(index, 1); + // Track for dedicated API call instead of modifying workflow.tags directly + // Reconcile: if previously marked for addition, cancel the addition instead + const addIdx = this.tagsToAdd.indexOf(operation.tag); + if (addIdx !== -1) { + this.tagsToAdd.splice(addIdx, 1); + } + if (!this.tagsToRemove.includes(operation.tag)) { + this.tagsToRemove.push(operation.tag); } } @@ -1015,7 +1055,12 @@ export class WorkflowDiffEngine { } return true; }) - ).filter(conns => conns.length > 0); + ); + + // Trim trailing empty arrays only (preserve intermediate for positional indices) + while (filteredConnections.length > 0 && filteredConnections[filteredConnections.length - 1].length === 0) { + filteredConnections.pop(); + } if (filteredConnections.length === 0) { delete outputs[outputName]; diff --git a/src/types/n8n-api.ts b/src/types/n8n-api.ts index e281dd4..e840741 100644 --- a/src/types/n8n-api.ts +++ b/src/types/n8n-api.ts @@ -311,6 +311,7 @@ export interface WebhookRequest { // MCP Tool Response Type export interface McpToolResponse { success: boolean; + saved?: boolean; data?: unknown; error?: string; message?: string; @@ -318,6 +319,7 @@ export interface McpToolResponse { details?: Record; executionId?: string; workflowId?: string; + operationsApplied?: number; } // Execution Filtering Types diff --git a/src/types/workflow-diff.ts b/src/types/workflow-diff.ts index 06173e6..c4a33fd 100644 --- a/src/types/workflow-diff.ts +++ b/src/types/workflow-diff.ts @@ -190,6 +190,8 @@ export interface WorkflowDiffResult { staleConnectionsRemoved?: Array<{ from: string; to: string }>; // For cleanStaleConnections operation shouldActivate?: boolean; // Flag to activate workflow after update (for activateWorkflow operation) shouldDeactivate?: boolean; // Flag to deactivate workflow after update (for deactivateWorkflow operation) + tagsToAdd?: string[]; + tagsToRemove?: string[]; } // Helper type for node reference (supports both ID and name) diff --git a/tests/integration/mcp-protocol/protocol-compliance.test.ts b/tests/integration/mcp-protocol/protocol-compliance.test.ts index 7e34f60..76a7767 100644 --- a/tests/integration/mcp-protocol/protocol-compliance.test.ts +++ b/tests/integration/mcp-protocol/protocol-compliance.test.ts @@ -105,21 +105,14 @@ describe('MCP Protocol Compliance', () => { describe('Message Format Validation', () => { it('should reject messages without method', async () => { - // Test by sending raw message through transport - const [serverTransport, clientTransport] = InMemoryTransport.createLinkedPair(); - const testClient = new Client({ name: 'test', version: '1.0.0' }, {}); - - await mcpServer.connectToTransport(serverTransport); - await testClient.connect(clientTransport); - + // MCP SDK 1.27+ enforces single-connection per Server instance, + // so use the existing client from beforeEach instead of a new one. try { // This should fail as MCP SDK validates method - await (testClient as any).request({ method: '', params: {} }); + await (client as any).request({ method: '', params: {} }); expect.fail('Should have thrown an error'); } catch (error) { expect(error).toBeDefined(); - } finally { - await testClient.close(); } }); @@ -250,10 +243,15 @@ describe('MCP Protocol Compliance', () => { describe('Transport Layer', () => { it('should handle transport disconnection gracefully', async () => { - const [serverTransport, clientTransport] = InMemoryTransport.createLinkedPair(); - const testClient = new Client({ name: 'test', version: '1.0.0' }, {}); + // Use a dedicated server instance so we don't conflict with the + // shared mcpServer that beforeEach already connected a transport to. + const dedicatedServer = new TestableN8NMCPServer(); + await dedicatedServer.initialize(); - await mcpServer.connectToTransport(serverTransport); + const [serverTransport, clientTransport] = InMemoryTransport.createLinkedPair(); + await dedicatedServer.connectToTransport(serverTransport); + + const testClient = new Client({ name: 'test', version: '1.0.0' }, {}); await testClient.connect(clientTransport); // Make a request @@ -270,6 +268,8 @@ describe('MCP Protocol Compliance', () => { } catch (error) { expect(error).toBeDefined(); } + + await dedicatedServer.close(); }); it('should handle multiple sequential connections', async () => { diff --git a/tests/integration/mcp-protocol/session-management.test.ts b/tests/integration/mcp-protocol/session-management.test.ts index 2f18a1a..2ad5d09 100644 --- a/tests/integration/mcp-protocol/session-management.test.ts +++ b/tests/integration/mcp-protocol/session-management.test.ts @@ -73,10 +73,11 @@ describe('MCP Session Management', { timeout: 15000 }, () => { const serverInfo = await client.getServerVersion(); expect(serverInfo).toBeDefined(); expect(serverInfo?.name).toBe('n8n-documentation-mcp'); - - // Check capabilities if they exist - if (serverInfo?.capabilities) { - expect(serverInfo.capabilities).toHaveProperty('tools'); + + // Check capabilities via the dedicated method + const capabilities = client.getServerCapabilities(); + if (capabilities) { + expect(capabilities).toHaveProperty('tools'); } // Clean up - ensure proper order @@ -340,9 +341,9 @@ describe('MCP Session Management', { timeout: 15000 }, () => { it('should handle different client versions', async () => { const mcpServer = new TestableN8NMCPServer(); await mcpServer.initialize(); - - const clients = []; + // MCP SDK 1.27+ enforces single-connection per Server instance, + // so we test each version sequentially rather than concurrently. for (const version of ['1.0.0', '1.1.0', '2.0.0']) { const [serverTransport, clientTransport] = InMemoryTransport.createLinkedPair(); await mcpServer.connectToTransport(serverTransport); @@ -353,21 +354,14 @@ describe('MCP Session Management', { timeout: 15000 }, () => { }, {}); await client.connect(clientTransport); - clients.push(client); + + const info = await client.getServerVersion(); + expect(info!.name).toBe('n8n-documentation-mcp'); + + await client.close(); + await new Promise(resolve => setTimeout(resolve, 50)); } - // All versions should work - const responses = await Promise.all( - clients.map(client => client.getServerVersion()) - ); - - responses.forEach(info => { - expect(info!.name).toBe('n8n-documentation-mcp'); - }); - - // Clean up - await Promise.all(clients.map(client => client.close())); - await new Promise(resolve => setTimeout(resolve, 100)); // Give time for all clients to fully close await mcpServer.close(); }); }); diff --git a/tests/integration/mcp-protocol/test-helpers.ts b/tests/integration/mcp-protocol/test-helpers.ts index 10c1dfe..8dc4d89 100644 --- a/tests/integration/mcp-protocol/test-helpers.ts +++ b/tests/integration/mcp-protocol/test-helpers.ts @@ -1,7 +1,7 @@ import { Server } from '@modelcontextprotocol/sdk/server/index.js'; import { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; -import { - CallToolRequestSchema, +import { + CallToolRequestSchema, ListToolsRequestSchema, InitializeRequestSchema, } from '@modelcontextprotocol/sdk/types.js'; @@ -14,18 +14,30 @@ export class TestableN8NMCPServer { private mcpServer: N8NDocumentationMCPServer; private server: Server; private transports = new Set(); - private connections = new Set(); private static instanceCount = 0; private testDbPath: string; constructor() { - // Use a unique test database for each instance to avoid conflicts - // This prevents concurrent test issues with database locking - const instanceId = TestableN8NMCPServer.instanceCount++; - this.testDbPath = `/tmp/n8n-mcp-test-${process.pid}-${instanceId}.db`; + // Use path.resolve to produce a canonical absolute path so the shared + // database singleton always sees the exact same string, preventing + // "Shared database already initialized with different path" errors. + const path = require('path'); + this.testDbPath = path.resolve(process.cwd(), 'data', 'nodes.db'); process.env.NODE_DB_PATH = this.testDbPath; - - this.server = new Server({ + + this.server = this.createServer(); + + this.mcpServer = new N8NDocumentationMCPServer(); + this.setupHandlers(this.server); + } + + /** + * Create a fresh MCP SDK Server instance. + * MCP SDK 1.27+ enforces single-connection per Protocol instance, + * so we create a new one each time we need to connect to a transport. + */ + private createServer(): Server { + return new Server({ name: 'n8n-documentation-mcp', version: '1.0.0' }, { @@ -33,14 +45,11 @@ export class TestableN8NMCPServer { tools: {} } }); - - this.mcpServer = new N8NDocumentationMCPServer(); - this.setupHandlers(); } - private setupHandlers() { + private setupHandlers(server: Server) { // Initialize handler - this.server.setRequestHandler(InitializeRequestSchema, async () => { + server.setRequestHandler(InitializeRequestSchema, async () => { return { protocolVersion: '2024-11-05', capabilities: { @@ -54,27 +63,27 @@ export class TestableN8NMCPServer { }); // List tools handler - this.server.setRequestHandler(ListToolsRequestSchema, async () => { + server.setRequestHandler(ListToolsRequestSchema, async () => { // Import the tools directly from the tools module const { n8nDocumentationToolsFinal } = await import('../../../src/mcp/tools'); const { n8nManagementTools } = await import('../../../src/mcp/tools-n8n-manager'); const { isN8nApiConfigured } = await import('../../../src/config/n8n-api'); - + // Combine documentation tools with management tools if API is configured const tools = [...n8nDocumentationToolsFinal]; if (isN8nApiConfigured()) { tools.push(...n8nManagementTools); } - + return { tools }; }); // Call tool handler - this.server.setRequestHandler(CallToolRequestSchema, async (request) => { + server.setRequestHandler(CallToolRequestSchema, async (request) => { try { // The mcpServer.executeTool returns raw data, we need to wrap it in the MCP response format const result = await this.mcpServer.executeTool(request.params.name, request.params.arguments || {}); - + return { content: [ { @@ -98,21 +107,8 @@ export class TestableN8NMCPServer { } async initialize(): Promise { - // Copy production database to test location for realistic testing - try { - const fs = await import('fs'); - const path = await import('path'); - const prodDbPath = path.join(process.cwd(), 'data', 'nodes.db'); - - if (await fs.promises.access(prodDbPath).then(() => true).catch(() => false)) { - await fs.promises.copyFile(prodDbPath, this.testDbPath); - } - } catch (error) { - // Ignore copy errors, database will be created fresh - } - - // The MCP server initializes its database lazily - // We can trigger initialization by calling executeTool + // The MCP server initializes its database lazily via the shared + // database singleton. Trigger initialization by calling executeTool. try { await this.mcpServer.executeTool('tools_documentation', {}); } catch (error) { @@ -125,33 +121,26 @@ export class TestableN8NMCPServer { if (!transport || typeof transport !== 'object') { throw new Error('Invalid transport provided'); } - - // Set up any missing transport handlers to prevent "Cannot set properties of undefined" errors - if (transport && typeof transport === 'object') { - const transportAny = transport as any; - if (transportAny.serverTransport && !transportAny.serverTransport.onclose) { - transportAny.serverTransport.onclose = () => {}; - } + + // MCP SDK 1.27+ enforces single-connection per Protocol instance. + // Close the current server and create a fresh one so that _transport + // is guaranteed to be undefined. Reusing the same Server after close() + // is unreliable because _transport is cleared asynchronously via the + // transport onclose callback chain, which can fail in CI. + try { + await this.server.close(); + } catch { + // Ignore errors during cleanup of previous transport } - - // MCP SDK 1.27+ enforces single-connection per Server instance. - // Close existing connections before connecting a new transport. - for (const conn of this.connections) { - try { - if (conn && typeof conn.close === 'function') { - await conn.close(); - } - } catch { - // Ignore errors during cleanup - } - } - this.connections.clear(); + + // Create a brand-new Server instance for this connection + this.server = this.createServer(); + this.setupHandlers(this.server); // Track this transport for cleanup this.transports.add(transport); - const connection = await this.server.connect(transport); - this.connections.add(connection); + await this.server.connect(transport); } async close(): Promise { @@ -164,78 +153,47 @@ export class TestableN8NMCPServer { }); const performClose = async () => { - // Close all connections first with timeout protection - const connectionPromises = Array.from(this.connections).map(async (connection) => { - const connTimeout = new Promise((resolve) => setTimeout(resolve, 500)); - - try { - if (connection && typeof connection.close === 'function') { - await Promise.race([connection.close(), connTimeout]); - } - } catch (error) { - // Ignore errors during connection cleanup - } - }); - - await Promise.allSettled(connectionPromises); - this.connections.clear(); - + // Close the MCP SDK Server (resets _transport via _onclose) + try { + await this.server.close(); + } catch { + // Ignore errors during server close + } + + // Shut down the inner N8NDocumentationMCPServer to release the + // shared database reference and prevent resource leaks. + try { + await this.mcpServer.shutdown(); + } catch { + // Ignore errors during inner server shutdown + } + // Close all tracked transports with timeout protection const transportPromises: Promise[] = []; - + for (const transport of this.transports) { const transportTimeout = new Promise((resolve) => setTimeout(resolve, 500)); - + try { - // Force close all transports const transportAny = transport as any; - - // Try different close methods if (transportAny.close && typeof transportAny.close === 'function') { transportPromises.push( Promise.race([transportAny.close(), transportTimeout]) ); } - if (transportAny.serverTransport?.close) { - transportPromises.push( - Promise.race([transportAny.serverTransport.close(), transportTimeout]) - ); - } - if (transportAny.clientTransport?.close) { - transportPromises.push( - Promise.race([transportAny.clientTransport.close(), transportTimeout]) - ); - } - } catch (error) { + } catch { // Ignore errors during transport cleanup } } - - // Wait for all transports to close with timeout + await Promise.allSettled(transportPromises); - - // Clear the transports set this.transports.clear(); - - // Don't shut down the shared MCP server instance }; // Race between actual close and timeout await Promise.race([performClose(), closeTimeout]); - - // Clean up test database - if (this.testDbPath) { - try { - const fs = await import('fs'); - await fs.promises.unlink(this.testDbPath).catch(() => {}); - await fs.promises.unlink(`${this.testDbPath}-shm`).catch(() => {}); - await fs.promises.unlink(`${this.testDbPath}-wal`).catch(() => {}); - } catch (error) { - // Ignore cleanup errors - } - } } - + static async shutdownShared(): Promise { if (sharedMcpServer) { await sharedMcpServer.shutdown(); diff --git a/tests/unit/mcp/handlers-workflow-diff.test.ts b/tests/unit/mcp/handlers-workflow-diff.test.ts index ba184bd..1c9126a 100644 --- a/tests/unit/mcp/handlers-workflow-diff.test.ts +++ b/tests/unit/mcp/handlers-workflow-diff.test.ts @@ -73,6 +73,9 @@ describe('handlers-workflow-diff', () => { mockApiClient = { getWorkflow: vi.fn(), updateWorkflow: vi.fn(), + listTags: vi.fn().mockResolvedValue({ data: [] }), + createTag: vi.fn(), + updateWorkflowTags: vi.fn().mockResolvedValue([]), }; // Setup mock diff engine @@ -150,6 +153,7 @@ describe('handlers-workflow-diff', () => { expect(result).toEqual({ success: true, + saved: true, data: { id: 'test-workflow-id', name: 'Test Workflow', @@ -309,10 +313,12 @@ describe('handlers-workflow-diff', () => { expect(result).toEqual({ success: false, + saved: false, + operationsApplied: 0, error: 'Failed to apply diff operations', details: { errors: ['Node "non-existent-node" not found'], - operationsApplied: 0, + warnings: undefined, applied: [], failed: [0], }, @@ -630,10 +636,14 @@ describe('handlers-workflow-diff', () => { expect(result).toEqual({ success: false, + saved: false, + operationsApplied: 1, error: 'Failed to apply diff operations', details: { errors: ['Operation 2 failed: Node "invalid-node" not found'], - operationsApplied: 1, + warnings: undefined, + applied: undefined, + failed: undefined, }, }); }); @@ -855,5 +865,141 @@ describe('handlers-workflow-diff', () => { }); }); }); + + describe('Tag Operations via Dedicated API', () => { + it('should create a new tag and associate it with the workflow', async () => { + const testWorkflow = createTestWorkflow(); + const updatedWorkflow = { ...testWorkflow }; + + mockApiClient.getWorkflow.mockResolvedValue(testWorkflow); + mockDiffEngine.applyDiff.mockResolvedValue({ + success: true, + workflow: updatedWorkflow, + operationsApplied: 1, + message: 'Success', + errors: [], + tagsToAdd: ['new-tag'], + }); + mockApiClient.updateWorkflow.mockResolvedValue(updatedWorkflow); + mockApiClient.listTags.mockResolvedValue({ data: [] }); + mockApiClient.createTag.mockResolvedValue({ id: 'tag-123', name: 'new-tag' }); + + const result = await handleUpdatePartialWorkflow({ + id: 'test-workflow-id', + operations: [{ type: 'addTag', tag: 'new-tag' }], + }, mockRepository); + + expect(result.success).toBe(true); + expect(mockApiClient.createTag).toHaveBeenCalledWith({ name: 'new-tag' }); + expect(mockApiClient.updateWorkflowTags).toHaveBeenCalledWith('test-workflow-id', ['tag-123']); + }); + + it('should use existing tag ID when tag already exists', async () => { + const testWorkflow = createTestWorkflow(); + const updatedWorkflow = { ...testWorkflow }; + + mockApiClient.getWorkflow.mockResolvedValue(testWorkflow); + mockDiffEngine.applyDiff.mockResolvedValue({ + success: true, + workflow: updatedWorkflow, + operationsApplied: 1, + message: 'Success', + errors: [], + tagsToAdd: ['existing-tag'], + }); + mockApiClient.updateWorkflow.mockResolvedValue(updatedWorkflow); + mockApiClient.listTags.mockResolvedValue({ data: [{ id: 'tag-456', name: 'existing-tag' }] }); + + const result = await handleUpdatePartialWorkflow({ + id: 'test-workflow-id', + operations: [{ type: 'addTag', tag: 'existing-tag' }], + }, mockRepository); + + expect(result.success).toBe(true); + expect(mockApiClient.createTag).not.toHaveBeenCalled(); + expect(mockApiClient.updateWorkflowTags).toHaveBeenCalledWith('test-workflow-id', ['tag-456']); + }); + + it('should remove a tag from the workflow', async () => { + const testWorkflow = createTestWorkflow({ + tags: [{ id: 'tag-789', name: 'old-tag' }], + }); + const updatedWorkflow = { ...testWorkflow }; + + mockApiClient.getWorkflow.mockResolvedValue(testWorkflow); + mockDiffEngine.applyDiff.mockResolvedValue({ + success: true, + workflow: updatedWorkflow, + operationsApplied: 1, + message: 'Success', + errors: [], + tagsToRemove: ['old-tag'], + }); + mockApiClient.updateWorkflow.mockResolvedValue(updatedWorkflow); + mockApiClient.listTags.mockResolvedValue({ data: [{ id: 'tag-789', name: 'old-tag' }] }); + + const result = await handleUpdatePartialWorkflow({ + id: 'test-workflow-id', + operations: [{ type: 'removeTag', tag: 'old-tag' }], + }, mockRepository); + + expect(result.success).toBe(true); + expect(mockApiClient.updateWorkflowTags).toHaveBeenCalledWith('test-workflow-id', []); + }); + + it('should produce warning on tag creation failure without failing the operation', async () => { + const testWorkflow = createTestWorkflow(); + const updatedWorkflow = { ...testWorkflow }; + + mockApiClient.getWorkflow.mockResolvedValue(testWorkflow); + mockDiffEngine.applyDiff.mockResolvedValue({ + success: true, + workflow: updatedWorkflow, + operationsApplied: 1, + message: 'Success', + errors: [], + tagsToAdd: ['fail-tag'], + }); + mockApiClient.updateWorkflow.mockResolvedValue(updatedWorkflow); + mockApiClient.listTags.mockResolvedValue({ data: [] }); + mockApiClient.createTag.mockRejectedValue(new Error('Tag creation failed')); + + const result = await handleUpdatePartialWorkflow({ + id: 'test-workflow-id', + operations: [{ type: 'addTag', tag: 'fail-tag' }], + }, mockRepository); + + expect(result.success).toBe(true); + expect(result.saved).toBe(true); + // Tag creation failure should produce a warning, not block the update + const warnings = (result.details as any)?.warnings; + expect(warnings).toBeDefined(); + expect(warnings.some((w: any) => w.message.includes('Failed to create tag'))).toBe(true); + }); + + it('should not call tag APIs when no tag operations are present', async () => { + const testWorkflow = createTestWorkflow(); + const updatedWorkflow = { ...testWorkflow }; + + mockApiClient.getWorkflow.mockResolvedValue(testWorkflow); + mockDiffEngine.applyDiff.mockResolvedValue({ + success: true, + workflow: updatedWorkflow, + operationsApplied: 1, + message: 'Success', + errors: [], + }); + mockApiClient.updateWorkflow.mockResolvedValue(updatedWorkflow); + + await handleUpdatePartialWorkflow({ + id: 'test-workflow-id', + operations: [{ type: 'updateName', name: 'New Name' }], + }, mockRepository); + + expect(mockApiClient.listTags).not.toHaveBeenCalled(); + expect(mockApiClient.createTag).not.toHaveBeenCalled(); + expect(mockApiClient.updateWorkflowTags).not.toHaveBeenCalled(); + }); + }); }); }); \ No newline at end of file diff --git a/tests/unit/services/n8n-api-client.test.ts b/tests/unit/services/n8n-api-client.test.ts index 6ff1bf3..6a71dd6 100644 --- a/tests/unit/services/n8n-api-client.test.ts +++ b/tests/unit/services/n8n-api-client.test.ts @@ -398,7 +398,7 @@ describe('N8nApiClient', () => { const result = await client.activateWorkflow('123'); - expect(mockAxiosInstance.post).toHaveBeenCalledWith('/workflows/123/activate'); + expect(mockAxiosInstance.post).toHaveBeenCalledWith('/workflows/123/activate', {}); expect(result).toEqual(activatedWorkflow); expect(result.active).toBe(true); }); @@ -484,7 +484,7 @@ describe('N8nApiClient', () => { const result = await client.deactivateWorkflow('123'); - expect(mockAxiosInstance.post).toHaveBeenCalledWith('/workflows/123/deactivate'); + expect(mockAxiosInstance.post).toHaveBeenCalledWith('/workflows/123/deactivate', {}); expect(result).toEqual(deactivatedWorkflow); expect(result.active).toBe(false); }); diff --git a/tests/unit/services/workflow-diff-engine.test.ts b/tests/unit/services/workflow-diff-engine.test.ts index 3082279..0c1281e 100644 --- a/tests/unit/services/workflow-diff-engine.test.ts +++ b/tests/unit/services/workflow-diff-engine.test.ts @@ -424,7 +424,7 @@ describe('WorkflowDiffEngine', () => { expect(result.success).toBe(false); expect(result.errors![0].message).toContain('Missing required parameter \'updates\''); - expect(result.errors![0].message).toContain('Example:'); + expect(result.errors![0].message).toContain('Correct structure:'); }); }); @@ -1898,16 +1898,15 @@ describe('WorkflowDiffEngine', () => { }; const result = await diffEngine.applyDiff(baseWorkflow, request); - + expect(result.success).toBe(true); - expect(result.workflow!.tags).toContain('production'); - expect(result.workflow!.tags).toHaveLength(3); + expect(result.tagsToAdd).toContain('production'); }); it('should not add duplicate tags', async () => { const operation: AddTagOperation = { type: 'addTag', - tag: 'test' // Already exists + tag: 'test' // Already exists in workflow but tagsToAdd tracks it for API }; const request: WorkflowDiffRequest = { @@ -1916,9 +1915,10 @@ describe('WorkflowDiffEngine', () => { }; const result = await diffEngine.applyDiff(baseWorkflow, request); - + expect(result.success).toBe(true); - expect(result.workflow!.tags).toHaveLength(2); // No change + // Tags are now tracked for dedicated API call, not modified on workflow + expect(result.tagsToAdd).toEqual(['test']); }); it('should create tags array if not exists', async () => { @@ -1935,10 +1935,9 @@ describe('WorkflowDiffEngine', () => { }; const result = await diffEngine.applyDiff(baseWorkflow, request); - + expect(result.success).toBe(true); - expect(result.workflow!.tags).toBeDefined(); - expect(result.workflow!.tags).toEqual(['new-tag']); + expect(result.tagsToAdd).toEqual(['new-tag']); }); it('should remove an existing tag', async () => { @@ -1953,10 +1952,9 @@ describe('WorkflowDiffEngine', () => { }; const result = await diffEngine.applyDiff(baseWorkflow, request); - + expect(result.success).toBe(true); - expect(result.workflow!.tags).not.toContain('test'); - expect(result.workflow!.tags).toHaveLength(1); + expect(result.tagsToRemove).toContain('test'); }); it('should handle removing non-existent tag gracefully', async () => { @@ -1971,9 +1969,11 @@ describe('WorkflowDiffEngine', () => { }; const result = await diffEngine.applyDiff(baseWorkflow, request); - + expect(result.success).toBe(true); - expect(result.workflow!.tags).toHaveLength(2); // No change + expect(result.tagsToRemove).toEqual(['non-existent']); + // workflow.tags unchanged since tags are now handled via dedicated API + expect(result.workflow!.tags).toHaveLength(2); }); }); @@ -2509,7 +2509,7 @@ describe('WorkflowDiffEngine', () => { expect(result.failed).toEqual([1]); // Operation 1 failed expect(result.errors).toHaveLength(1); expect(result.workflow.name).toBe('New Workflow Name'); - expect(result.workflow.tags).toContain('production'); + expect(result.tagsToAdd).toContain('production'); }); it('should return success false if all operations fail in continueOnError mode', async () => { @@ -3356,7 +3356,7 @@ describe('WorkflowDiffEngine', () => { expect(result.failed).toContain(1); // replaceConnections with invalid node expect(result.applied).toContain(2); // removeConnection with ignoreErrors expect(result.applied).toContain(3); // addTag - expect(result.workflow.tags).toContain('final-tag'); + expect(result.tagsToAdd).toContain('final-tag'); }); }); @@ -4610,7 +4610,7 @@ describe('WorkflowDiffEngine', () => { expect(result.success).toBe(true); expect(result.operationsApplied).toBe(3); expect(result.workflow!.name).toBe('Updated Workflow Name'); - expect(result.workflow!.tags).toContain('production'); + expect(result.tagsToAdd).toContain('production'); expect(result.shouldActivate).toBe(true); });