fix: resolve multiple n8n_update_partial_workflow bugs (#635)

* fix: use correct MCP SDK API for server capabilities in test getServerVersion() returns Implementation (name/version only), not the full init result. Use client.getServerCapabilities() instead to access server capabilities, fixing the CI typecheck failure. Concieved by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: resolve multiple n8n_update_partial_workflow bugs (#592, #599, #610, #623, #624, #625, #629, #630, #633) Phase 1 - Data loss prevention: - Add missing unary operators (empty, notEmpty, exists, notExists) to sanitizer (#592) - Preserve positional empty arrays in connections during removeNode/cleanStale (#610) - Scope sanitization to modified nodes only, preventing unrelated node corruption - Add empty body {} to activate/deactivate POST calls to fix 415 errors (#633) Phase 2 - Error handling & response clarity: - Serialize Zod errors to readable "path: message" strings (#630) - Add saved:true/false field to all response paths (#625) - Improve updateNode error hint with correct structure example (#623) - Track removed node names for better removeConnection errors (#624) Phase 3 - Connection & type fixes: - Coerce sourceOutput/targetInput to String() consistently (#629) - Accept numeric sourceOutput/targetInput at Zod schema level via transform Phase 4 - Tag operations via dedicated API (#599): - Track tags as tagsToAdd/tagsToRemove instead of mutating workflow.tags - Orchestrate tag creation and association via listTags/createTag/updateWorkflowTags - Reconcile conflicting add/remove for same tag (last operation wins) - Tag failures produce warnings, not hard errors Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add v2.37.0 changelog entry Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: resolve pre-existing integration test failures in CI - Create new MCP Server instance per connection in test helpers (SDK 1.27+ requires separate Protocol instance per connection) - Normalize database paths with path.resolve() in shared-database singleton to prevent path mismatch errors across test files - Add no-op catch handler to deferred initialization promise in server.ts to prevent unhandled rejection warnings - Properly call mcpServer.shutdown() in test helper close() to release shared database references Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-22 02:13:09 +00:00 · 2026-03-14 17:27:33 +01:00
parent 248f859c49
commit 9590f751d2
15 changed files with 504 additions and 225 deletions
--- 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();
+      });
+    });
  });
 });
--- 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);
    });
--- 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);
    });