From ae9dd02babedc3a981fbeaa420daa135d328ab1b Mon Sep 17 00:00:00 2001 From: Daria Date: Mon, 24 Aug 2026 13:29:41 +0000 Subject: [PATCH] feat(core): IAI - Drop invalid node groups before save (#36812) --- .../skills/workflow-builder/SKILL.md | 5 +- .../tools/__tests__/workflows.tool.test.ts | 177 +++++++++++++++++- .../instance-ai/src/tools/workflows.tool.ts | 26 ++- .../__tests__/build-workflow.tool.test.ts | 65 ++++++- .../tools/workflows/build-workflow.tool.ts | 26 ++- .../workflows/workflow-build-telemetry.ts | 2 + .../workflows/workflow-validation-warnings.ts | 35 ++++ .../sdk-reference/sdk-language.test.ts | 6 + .../src/prompts/sdk-reference/sdk-language.ts | 5 +- 9 files changed, 331 insertions(+), 16 deletions(-) diff --git a/packages/@n8n/instance-ai/skills/workflow-builder/SKILL.md b/packages/@n8n/instance-ai/skills/workflow-builder/SKILL.md index f7c2bdeb70b..ef7b133970b 100644 --- a/packages/@n8n/instance-ai/skills/workflow-builder/SKILL.md +++ b/packages/@n8n/instance-ai/skills/workflow-builder/SKILL.md @@ -603,8 +603,9 @@ first. `.group(name, members, { description })` on the workflow builder; members are the node handles. Read `knowledge-base/reference/node-groups.md` for the exact rules (trigger nodes excluded, -one connected section, AI sub-nodes stay with their Agent) before creating groups — an invalid -group is rejected on save. When editing an existing workflow, keep existing `.group(...)` calls +one connected section, AI sub-nodes stay with their Agent) before creating groups. Agent save +tools drop an invalid group from the saved workflow and report a warning, so fix the source +instead of re-emitting it. When editing an existing workflow, keep existing `.group(...)` calls and their descriptions intact unless the change is about grouping. ## Workflow Rules diff --git a/packages/@n8n/instance-ai/src/tools/__tests__/workflows.tool.test.ts b/packages/@n8n/instance-ai/src/tools/__tests__/workflows.tool.test.ts index 5088cb98a14..94f15ff4e58 100644 --- a/packages/@n8n/instance-ai/src/tools/__tests__/workflows.tool.test.ts +++ b/packages/@n8n/instance-ai/src/tools/__tests__/workflows.tool.test.ts @@ -29,10 +29,14 @@ vi.mock('../workflows/setup-workflow.service', () => ({ buildCompletedReport: vi.fn().mockReturnValue([]), })); -// Mock the dynamic import of @n8n/workflow-sdk used by get-as-code -vi.mock('@n8n/workflow-sdk', () => ({ - generateWorkflowCode: vi.fn().mockReturnValue('// generated code'), -})); +// Mock code generation used by get-as-code while keeping shared SDK helpers real. +vi.mock('@n8n/workflow-sdk', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + generateWorkflowCode: vi.fn().mockReturnValue('// generated code'), + }; +}); const emptyList = { workflows: [], total: 0, totalInScope: 0 }; @@ -1234,6 +1238,171 @@ describe('workflows tool', () => { }); }); + it('drops invalid node groups before saving and reports coded warnings', async () => { + const context = createMockContext({ permissions: { updateWorkflow: 'always_allow' } }); + (context.workflowService.updateFromWorkflowJSON as Mock).mockResolvedValue({ + id: 'wf1', + versionId: 'v2', + checksum: 'checksum-saved', + }); + const workflow = { + name: 'Updated WF', + nodes: [ + { + id: 'node-1', + name: 'Set', + type: 'n8n-nodes-base.set', + typeVersion: 1, + position: [0, 0], + parameters: {}, + }, + ], + connections: {}, + nodeGroups: [ + { + id: 'group-1', + name: 'Broken group', + nodeIds: ['missing-node', 'another-missing-node'], + }, + ], + }; + + const result = await executeTool<{ + success: boolean; + workflowId?: string; + warnings?: string[]; + }>( + createWorkflowsTool(context, 'full'), + { action: 'update', workflowId: 'wf1', workflow }, + {} as never, + ); + + expect(result).toMatchObject({ success: true, workflowId: 'wf1' }); + expect(result.warnings).toHaveLength(1); + expect(result.warnings?.join('\n')).toContain('[NODE_GROUP_DROPPED]'); + expect(result.warnings?.join('\n')).toContain('Broken group'); + expect(result.warnings?.join('\n')).toContain('missing-node'); + expect(result.warnings?.join('\n')).toContain('another-missing-node'); + expect(context.workflowService.updateFromWorkflowJSON).toHaveBeenCalledWith( + 'wf1', + expect.objectContaining({ nodeGroups: [] }), + ); + }); + + it('saves valid node groups unchanged and returns no node-group warnings', async () => { + const context = createMockContext({ permissions: { updateWorkflow: 'always_allow' } }); + (context.workflowService.updateFromWorkflowJSON as Mock).mockResolvedValue({ + id: 'wf1', + versionId: 'v2', + checksum: 'checksum-saved', + }); + const workflow = { + name: 'Updated WF', + nodes: [ + { + id: 'a', + name: 'A', + type: 'n8n-nodes-base.set', + typeVersion: 1, + position: [0, 0], + parameters: {}, + }, + { + id: 'b', + name: 'B', + type: 'n8n-nodes-base.set', + typeVersion: 1, + position: [100, 0], + parameters: {}, + }, + ], + connections: { A: { main: [[{ node: 'B', type: 'main', index: 0 }]] } }, + nodeGroups: [{ id: 'group-1', name: 'Valid group', nodeIds: ['a', 'b'] }], + }; + + const result = await executeTool<{ + success: boolean; + workflowId?: string; + warnings?: string[]; + }>( + createWorkflowsTool(context, 'full'), + { action: 'update', workflowId: 'wf1', workflow }, + {} as never, + ); + + expect(result).toMatchObject({ success: true, workflowId: 'wf1' }); + expect(result.warnings).toBeUndefined(); + expect(context.workflowService.updateFromWorkflowJSON).toHaveBeenCalledWith('wf1', workflow); + }); + + it('normalizes duplicate and blank node IDs before node-group validation', async () => { + const context = createMockContext({ permissions: { updateWorkflow: 'always_allow' } }); + (context.workflowService.updateFromWorkflowJSON as Mock).mockResolvedValue({ + id: 'wf1', + versionId: 'v2', + checksum: 'checksum-saved', + }); + const workflow = { + name: 'Updated WF', + nodes: [ + { + id: '', + name: 'A', + type: 'n8n-nodes-base.set', + typeVersion: 1, + position: [0, 0], + parameters: {}, + }, + { + id: '', + name: 'B', + type: 'n8n-nodes-base.set', + typeVersion: 1, + position: [100, 0], + parameters: {}, + }, + ], + connections: { A: { main: [[{ node: 'B', type: 'main', index: 0 }]] } }, + nodeGroups: [{ id: 'group-1', name: 'Healed group', nodeIds: ['', ''] }], + }; + + const result = await executeTool<{ + success: boolean; + workflowId?: string; + warnings?: string[]; + }>( + createWorkflowsTool(context, 'full'), + { action: 'update', workflowId: 'wf1', workflow }, + {} as never, + ); + + const savedWorkflow = (context.workflowService.updateFromWorkflowJSON as Mock).mock + .calls[0][1] as typeof workflow; + const nodeIds = savedWorkflow.nodes.map((node) => node.id); + expect(result).toMatchObject({ success: true, workflowId: 'wf1' }); + expect(result.warnings).toBeUndefined(); + expect(nodeIds.every(Boolean)).toBe(true); + expect(new Set(nodeIds)).toHaveProperty('size', 2); + expect(savedWorkflow.nodeGroups).toEqual([{ id: 'group-1', name: 'Healed group', nodeIds }]); + }); + + it('returns a tool error when raw update ID normalization cannot read nodes', async () => { + const context = createMockContext({ permissions: { updateWorkflow: 'always_allow' } }); + + const result = await executeTool( + createWorkflowsTool(context, 'full'), + { + action: 'update', + workflowId: 'wf1', + workflow: { name: 'Updated WF', nodes: [null], connections: {} }, + }, + {} as never, + ); + + expect(result).toMatchObject({ success: false }); + expect(context.workflowService.updateFromWorkflowJSON).not.toHaveBeenCalled(); + }); + it('tells the agent what to do when a user is editing the workflow in the editor', async () => { const context = createMockContext({ permissions: { updateWorkflow: 'always_allow' } }); (context.workflowService.updateFromWorkflowJSON as Mock).mockRejectedValue( diff --git a/packages/@n8n/instance-ai/src/tools/workflows.tool.ts b/packages/@n8n/instance-ai/src/tools/workflows.tool.ts index 5e3afa7f506..12c63333580 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows.tool.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows.tool.ts @@ -5,7 +5,8 @@ */ import { Tool } from '@n8n/agents'; import { isRecord } from '@n8n/utils/is-record'; -import type { WorkflowJSON } from '@n8n/workflow-sdk'; +import { dropInvalidWorkflowJsonGroups, type WorkflowJSON } from '@n8n/workflow-sdk'; +import { makeGetNodeTypeForGrouping } from 'n8n-workflow'; import { nanoid } from 'nanoid'; import { z } from 'zod'; @@ -52,9 +53,11 @@ import { validateWorkflowConfig } from './workflows/validate-workflow.service'; import { grantSessionWorkflowUpdate, canSkipWorkflowUpdateHitl, + formatWarning, } from './workflows/workflow-build-context'; import { refreshWorkflowSourceFileBindingFromWorkflow } from './workflows/workflow-file-bindings'; -import { getReferencedWorkflowIds } from './workflows/workflow-json-utils'; +import { ensureUniqueNodeIds, getReferencedWorkflowIds } from './workflows/workflow-json-utils'; +import { nodeGroupDroppedWarnings } from './workflows/workflow-validation-warnings'; // ── Action schemas ────────────────────────────────────────────────────────── @@ -1320,6 +1323,13 @@ async function handleUpdate( const expectedChecksum = await getObservedWorkflowChecksum(context, input.workflowId); try { + ensureUniqueNodeIds(input.workflow); + const droppedGroupWarnings = nodeGroupDroppedWarnings( + dropInvalidWorkflowJsonGroups( + input.workflow, + context.nodeTypesProvider ? makeGetNodeTypeForGrouping(context.nodeTypesProvider) : null, + ), + ); const saved = expectedChecksum ? await context.workflowService.updateFromWorkflowJSON(input.workflowId, input.workflow, { expectedChecksum, @@ -1331,7 +1341,17 @@ async function handleUpdate( if (saved.checksum) { await rememberObservedWorkflowChecksum(context, input.workflowId, saved.checksum); } - return { success: true, workflowId: input.workflowId }; + return { + success: true, + workflowId: input.workflowId, + ...(droppedGroupWarnings.length > 0 + ? { + warnings: droppedGroupWarnings.map((warning) => + formatWarning(warning.code, warning.message), + ), + } + : {}), + }; } catch (error) { if (error instanceof WorkflowSaveConflictError) { return { diff --git a/packages/@n8n/instance-ai/src/tools/workflows/__tests__/build-workflow.tool.test.ts b/packages/@n8n/instance-ai/src/tools/workflows/__tests__/build-workflow.tool.test.ts index 0fba80a0dd2..a124fe7bb4d 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows/__tests__/build-workflow.tool.test.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows/__tests__/build-workflow.tool.test.ts @@ -28,9 +28,15 @@ vi.mock('../../../tracing/langsmith-tracing', async () => { }; }); -vi.mock('../workflow-validation-warnings', () => ({ - partitionWarnings: vi.fn((warnings: unknown[]) => ({ blocking: [], informational: warnings })), -})); +vi.mock('../workflow-validation-warnings', async () => { + const actual = await vi.importActual( + '../workflow-validation-warnings', + ); + return { + ...actual, + partitionWarnings: vi.fn((warnings: unknown[]) => ({ blocking: [], informational: warnings })), + }; +}); const generatedWorkflow = { name: 'Generated workflow', @@ -320,6 +326,59 @@ describe('createBuildWorkflowTool', () => { expect(result.postBuildFlow?.instructions).not.toContain('# Post-Build Flow'); }); + it('drops invalid node groups before saving and reports the drop', async () => { + const source = 'workflow source from workspace'; + const { context, filePath, trackTelemetry } = makeContext({ source }); + vi.mocked(compileWorkflowSource).mockResolvedValueOnce({ + success: true, + workflow: { + name: 'Grouped workflow', + nodes: [ + { + id: 'node-1', + name: 'Set', + type: 'n8n-nodes-base.set', + typeVersion: 1, + position: [0, 0], + parameters: {}, + }, + ], + connections: {}, + nodeGroups: [ + { + id: 'group-1', + name: 'Broken group', + nodeIds: ['missing-node', 'another-missing-node'], + }, + ], + }, + warnings: [], + compiler: 'sandbox-tsx', + }); + + const result = await executeTool(createBuildWorkflowTool(context), { + filePath, + }); + + expect(result.success).toBe(true); + expect(result.warnings).toHaveLength(1); + expect(result.warnings?.join('\n')).toContain('[NODE_GROUP_DROPPED]'); + expect(result.warnings?.join('\n')).toContain('Broken group'); + expect(result.warnings?.join('\n')).toContain('missing-node'); + expect(result.warnings?.join('\n')).toContain('another-missing-node'); + + const savedWorkflow = vi.mocked(context.workflowService.createFromWorkflowJSON).mock + .calls[0]?.[0]; + expect(savedWorkflow?.nodeGroups).toEqual([]); + expect(trackTelemetry).toHaveBeenCalledWith( + 'instance_ai_workflow_source_build', + expect.objectContaining({ + dropped_group_count: 1, + warning_count: 1, + }), + ); + }); + it('falls back to the post-build-flow handoff for a triggerless one-off build', async () => { const source = 'workflow source from workspace'; const { context, filePath } = makeContext({ source }); diff --git a/packages/@n8n/instance-ai/src/tools/workflows/build-workflow.tool.ts b/packages/@n8n/instance-ai/src/tools/workflows/build-workflow.tool.ts index 12c85075e6f..ad5431bf74e 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows/build-workflow.tool.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows/build-workflow.tool.ts @@ -4,7 +4,12 @@ import { instanceAiConfirmationSeveritySchema, } from '@n8n/api-types'; import { hasPlaceholderDeep } from '@n8n/utils/placeholder'; -import { SDK_IMPORTABLE_FUNCTIONS, type WorkflowJSON } from '@n8n/workflow-sdk'; +import { + dropInvalidWorkflowJsonGroups, + SDK_IMPORTABLE_FUNCTIONS, + type WorkflowJSON, +} from '@n8n/workflow-sdk'; +import { makeGetNodeTypeForGrouping } from 'n8n-workflow'; import { nanoid } from 'nanoid'; import { readFileSync } from 'node:fs'; import { join } from 'node:path'; @@ -71,7 +76,11 @@ import { } from './workflow-json-utils'; import { computeChangedNodeNames, downgradeUnchangedNodeBlockers } from './workflow-node-diff'; import { compileWorkflowSource } from './workflow-source-compiler'; -import { partitionWarnings, type ValidationWarning } from './workflow-validation-warnings'; +import { + nodeGroupDroppedWarnings, + partitionWarnings, + type ValidationWarning, +} from './workflow-validation-warnings'; import { WorkflowSaveConflictError } from '../../errors/workflow-save-conflict.error'; import { INSTANCE_AI_SKILLS_DIR } from '../../skills/runtime-skills'; import { emitTraceOnlyChildRun } from '../../tracing/langsmith-tracing'; @@ -1012,6 +1021,7 @@ export function createBuildWorkflowTool(context: InstanceAiContext) { await stripStaleCredentialsFromWorkflow(context, json); try { + let droppedGroupCount = 0; // Runs first: the passes below key off node ids, so they must be unique. ensureUniqueNodeIds(json); // Recovers the saved id of a surviving node whose source declared none — layered @@ -1021,6 +1031,17 @@ export function createBuildWorkflowTool(context: InstanceAiContext) { await ensureWebhookIds(json, targetWorkflowId, context); await preserveExistingNodeGroupIds(json, targetWorkflowId, context); await preserveExistingNodePositions(json, targetWorkflowId, context); + const groupCountBeforeDrop = json.nodeGroups?.length ?? 0; + const droppedGroupWarnings = nodeGroupDroppedWarnings( + dropInvalidWorkflowJsonGroups( + json, + context.nodeTypesProvider + ? makeGetNodeTypeForGrouping(context.nodeTypesProvider) + : null, + ), + ); + droppedGroupCount = groupCountBeforeDrop - (json.nodeGroups?.length ?? 0); + informational.push(...droppedGroupWarnings); if (await hasLostAllSavedNodeIds(json, targetWorkflowId, context)) { context.logger.debug('Build kept none of the saved node ids', { @@ -1202,6 +1223,7 @@ export function createBuildWorkflowTool(context: InstanceAiContext) { isSupportingWorkflow, isAuxiliarySupportingWorkflow, warningCount: informational.length, + droppedGroupCount, }); return { diff --git a/packages/@n8n/instance-ai/src/tools/workflows/workflow-build-telemetry.ts b/packages/@n8n/instance-ai/src/tools/workflows/workflow-build-telemetry.ts index a65f88dbb5e..4aea1988f0f 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows/workflow-build-telemetry.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows/workflow-build-telemetry.ts @@ -27,6 +27,7 @@ export function trackWorkflowSourceBuild( remediation?: RemediationMetadata; errorCount?: number; warningCount?: number; + droppedGroupCount?: number; }, ): void { const buildContext = context.workflowBuildContext; @@ -44,6 +45,7 @@ export function trackWorkflowSourceBuild( is_auxiliary_supporting_workflow: input.isAuxiliarySupportingWorkflow === true, error_count: input.errorCount ?? 0, warning_count: input.warningCount ?? 0, + dropped_group_count: input.droppedGroupCount ?? 0, ...(input.targetWorkflowId ? { target_workflow_id: input.targetWorkflowId } : {}), ...(input.savedWorkflowId ? { workflow_id: input.savedWorkflowId } : {}), ...(input.binding.sourceHash ? { source_hash: input.binding.sourceHash } : {}), diff --git a/packages/@n8n/instance-ai/src/tools/workflows/workflow-validation-warnings.ts b/packages/@n8n/instance-ai/src/tools/workflows/workflow-validation-warnings.ts index 3cc62228228..f763f184caa 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows/workflow-validation-warnings.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows/workflow-validation-warnings.ts @@ -1,4 +1,7 @@ import { partitionValidationIssues, type IssueSeverity } from '@n8n/workflow-sdk'; +import type { WorkflowGroupViolation } from 'n8n-workflow'; + +export const NODE_GROUP_DROPPED_CODE = 'NODE_GROUP_DROPPED'; export interface ValidationWarning { code: string; @@ -36,3 +39,35 @@ export function partitionWarnings(warnings: ValidationWarning[]): { // {@link partitionValidationIssues}. return partitionValidationIssues(warnings); } + +export function nodeGroupDroppedWarnings( + violations: WorkflowGroupViolation[], +): ValidationWarning[] { + const violationsByGroup = new Map(); + for (const violation of violations) { + const key = JSON.stringify([violation.groupId, violation.groupName]); + const groupViolations = violationsByGroup.get(key); + if (groupViolations) { + groupViolations.push(violation); + } else { + violationsByGroup.set(key, [violation]); + } + } + + const warnings: ValidationWarning[] = []; + for (const groupViolations of violationsByGroup.values()) { + const firstViolation = groupViolations[0]; + if (!firstViolation) continue; + const messages = groupViolations.map(({ message }) => message); + warnings.push(formatNodeGroupDroppedWarning(firstViolation.groupName, messages)); + } + return warnings; +} + +function formatNodeGroupDroppedWarning(groupName: string, messages: string[]): ValidationWarning { + return { + code: NODE_GROUP_DROPPED_CODE, + severity: 'informational', + message: `Node group "${groupName}" was removed from the saved workflow: ${messages.join(' ')}`, + }; +} diff --git a/packages/@n8n/workflow-sdk/src/prompts/sdk-reference/sdk-language.test.ts b/packages/@n8n/workflow-sdk/src/prompts/sdk-reference/sdk-language.test.ts index b7de5260dd5..a026f027860 100644 --- a/packages/@n8n/workflow-sdk/src/prompts/sdk-reference/sdk-language.test.ts +++ b/packages/@n8n/workflow-sdk/src/prompts/sdk-reference/sdk-language.test.ts @@ -114,6 +114,12 @@ describe('NODE_GROUPS_REFERENCE', () => { expect(NODE_GROUPS_REFERENCE).toMatch(/keep the .+ and their descriptions\s+intact/is); }); + it('tells agents invalid groups are dropped with warnings and source should be fixed', () => { + expect(NODE_GROUPS_REFERENCE).toMatch(/drop an invalid group.+report a warning/is); + expect(NODE_GROUPS_REFERENCE).toMatch(/fix the source.+not re-emitted/is); + expect(NODE_GROUPS_REFERENCE).not.toContain('rejected on save'); + }); + it('states the single entry/exit boundary rule that grouping enforces', () => { // reason: 'invalid-subgraph' — grouping rejects a group with more than one // incoming or outgoing main connection (single entry/exit *boundary*). diff --git a/packages/@n8n/workflow-sdk/src/prompts/sdk-reference/sdk-language.ts b/packages/@n8n/workflow-sdk/src/prompts/sdk-reference/sdk-language.ts index efad8e49256..762581fc304 100644 --- a/packages/@n8n/workflow-sdk/src/prompts/sdk-reference/sdk-language.ts +++ b/packages/@n8n/workflow-sdk/src/prompts/sdk-reference/sdk-language.ts @@ -106,8 +106,9 @@ sentence — anything past ${GROUP_DESCRIPTION_MAX_LENGTH} characters is cut off When editing an existing workflow, **keep the \`.group(...)\` calls and their descriptions intact** unless the change is specifically about grouping. -An invalid group is rejected on save, so these following rules MUST be followed when -creating or editing groups. +Agent save tools drop an invalid group from the saved workflow and report a warning. +Fix the source so the invalid group is not re-emitted. These rules MUST be followed +when creating or editing groups. Rules: ${renderRulesLines()}