diff --git a/.env.eval.example b/.env.eval.example index cf2849c4ad7..9c3cbf2998e 100644 --- a/.env.eval.example +++ b/.env.eval.example @@ -15,11 +15,6 @@ N8N_LOG_LEVEL=debug N8N_AI_ENABLED=true N8N_ENABLED_MODULES=instance-ai -# Uncomment when calibrating MCP grouping cases (--build-via-mcp): canvas-group -# support in the MCP tools is PostHog-gated and PostHog is unreachable locally -# (fails closed), so the env var is the only lever. Mirrors test-evals-mcp.yml. -# N8N_MCP_CANVAS_GROUPS_ENABLED=true - # Model key for the builder and the eval helper (mock generation, verifier, # user-proxy, expectation judge). When the instance runs without the proxy, the # model client reads ANTHROPIC_API_KEY directly, so export that too if you go diff --git a/.github/workflows/test-evals-mcp.yml b/.github/workflows/test-evals-mcp.yml index 0acd4dcc2d1..a622277f3d7 100644 --- a/.github/workflows/test-evals-mcp.yml +++ b/.github/workflows/test-evals-mcp.yml @@ -192,14 +192,11 @@ jobs: # agents registers the agent-builder MCP tools so the suite measures # workflow building with the production tool surface, catching # tool-choice regressions the extra tools could introduce. - # MCP canvas-group support is PostHog-gated and PostHog is unreachable - # here (fails closed), so the env var force-enables it; for i in "${!PORTS[@]}"; do port="${PORTS[$i]}" docker run -d --name "n8n-eval-mcp-$((i + 1))" \ -e E2E_TESTS=true \ -e N8N_ENABLED_MODULES=instance-ai,agents \ - -e N8N_MCP_CANVAS_GROUPS_ENABLED=true \ -e N8N_AI_ENABLED=true \ -e N8N_INSTANCE_AI_MODEL_API_KEY="$EVALS_ANTHROPIC_KEY" \ -e N8N_AI_ASSISTANT_BASE_URL="" \ diff --git a/packages/@n8n/api-types/src/index.ts b/packages/@n8n/api-types/src/index.ts index 05bd57e5b0e..6bb83495c46 100644 --- a/packages/@n8n/api-types/src/index.ts +++ b/packages/@n8n/api-types/src/index.ts @@ -688,7 +688,6 @@ export { MCP_APPS_FLAG, MCP_APPS_VARIANT_CONTROL, MCP_APPS_VARIANT_ENABLED, - MCP_CANVAS_GROUPS_FLAG, MCP_INSTANCE_CONTEXT_FLAG, MCP_AGENT_SCOPES, MCP_INSTANCE_SCOPES, diff --git a/packages/@n8n/api-types/src/schemas/mcp.schema.ts b/packages/@n8n/api-types/src/schemas/mcp.schema.ts index 573826d14f6..3c336512893 100644 --- a/packages/@n8n/api-types/src/schemas/mcp.schema.ts +++ b/packages/@n8n/api-types/src/schemas/mcp.schema.ts @@ -10,12 +10,6 @@ export const MCP_APPS_FLAG = '087_mcp_apps'; export const MCP_APPS_VARIANT_CONTROL = 'control'; export const MCP_APPS_VARIANT_ENABLED = 'variant'; -// PostHog boolean rollout flag id gating Canvas node-group support in the MCP -// workflow-builder tools (group docs in the SDK reference, group handling in -// the create/validate/update tools). `true` enables; anything else keeps the -// current behaviour. -export const MCP_CANVAS_GROUPS_FLAG = '102_mcp_canvas_groups'; - // Rollout flag for the instance-context read surface on the MCP server: the // activity tools and the node-usage tool. Separate from the Instance AI read // flag so the two surfaces roll out independently against the same data. diff --git a/packages/@n8n/config/src/configs/endpoints.config.ts b/packages/@n8n/config/src/configs/endpoints.config.ts index a4e4b594d73..66a4ff813b8 100644 --- a/packages/@n8n/config/src/configs/endpoints.config.ts +++ b/packages/@n8n/config/src/configs/endpoints.config.ts @@ -206,14 +206,6 @@ export class EndpointsConfig { @Env('N8N_MCP_APPS_ENABLED') mcpAppsEnabled: boolean = false; - /** - * Force-enable Canvas node-group support in the MCP workflow-builder tools. - * Acts as an operator-level override of the PostHog rollout flag. - * Cannot force-disable: setting this to `false` falls back to PostHog. - */ - @Env('N8N_MCP_CANVAS_GROUPS_ENABLED') - mcpCanvasGroupsEnabled: boolean = false; - /** * Force-enable the instance-context read surface on the MCP server: the * activity tools and the node-usage tool. diff --git a/packages/@n8n/config/test/config.test.ts b/packages/@n8n/config/test/config.test.ts index 3d5d95d53b6..0a8acf293e6 100644 --- a/packages/@n8n/config/test/config.test.ts +++ b/packages/@n8n/config/test/config.test.ts @@ -295,7 +295,6 @@ describe('GlobalConfig', () => { mcp: 'mcp', mcpAppsEnabled: false, mcpBuilderEnabled: true, - mcpCanvasGroupsEnabled: false, mcpInstanceContextEnabled: false, mcpMaxRegisteredClients: 5000, mcpTest: 'mcp-test', 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 ddd05175be7..fb7caeb0f9b 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 @@ -1,11 +1,6 @@ import { GROUP_DESCRIPTION_MAX_LENGTH, NODE_GROUPING_RULES } from 'n8n-workflow'; -import { - GROUPING_GUIDANCE, - NODE_GROUPS_REFERENCE, - SDK_LANGUAGE_REFERENCE, - buildSdkLanguageReference, -} from './sdk-language'; +import { GROUPING_GUIDANCE, NODE_GROUPS_REFERENCE, SDK_LANGUAGE_REFERENCE } from './sdk-language'; import { SDK_METHODS, FORBIDDEN_NODE_TYPES, @@ -107,23 +102,6 @@ describe('SDK_LANGUAGE_REFERENCE rendering', () => { }); }); -describe('buildSdkLanguageReference', () => { - it('includes the groups docs by default', () => { - expect(buildSdkLanguageReference()).toBe(buildSdkLanguageReference({ includeGroups: true })); - expect(buildSdkLanguageReference()).toContain(NODE_GROUPS_REFERENCE); - }); - - it('omits only the groups docs when includeGroups is false', () => { - const withoutGroups = buildSdkLanguageReference({ includeGroups: false }); - - expect(withoutGroups).not.toContain('## Node groups'); - // The rest of the reference is intact. - expect(withoutGroups).toContain('restricted subset of TypeScript'); - expect(withoutGroups).toContain('## Forbidden constructs'); - expect(withoutGroups).toContain('## Where to put runtime logic'); - }); -}); - describe('NODE_GROUPS_REFERENCE', () => { it('explains what a group is and that it still matters', () => { // A build called grouping "a finishing touch" and skipped it, echoing the word 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 fdf4800f36f..503c798f739 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 @@ -116,9 +116,8 @@ ${renderRulesLines()} /** * Grouping judgement guidance: *when* to group — the rules that make a group - * valid live in `NODE_GROUPS_REFERENCE`. MCP appends it to the technique list - * only when the canvas-groups flag is on; Instance AI always materializes it - * into the knowledge base. + * valid live in `NODE_GROUPS_REFERENCE`. MCP appends it to the technique list; + * Instance AI materializes it into the knowledge base. */ export const GROUPING_GUIDANCE = `## Grouping @@ -152,14 +151,8 @@ Examples: Read the node groups reference for the exact rules before creating groups.`; -/** - * Render the full language reference. The node-groups section is included by - * default (Instance AI's knowledge base); the MCP SDK reference passes its - * `canvasGroupsEnabled` flag state as `includeGroups`. - */ -export function buildSdkLanguageReference(options: { includeGroups?: boolean } = {}): string { - const { includeGroups = true } = options; - +/** Render the full language reference, node-groups section included. */ +export function buildSdkLanguageReference(): string { return `# Workflow SDK language reference SDK builder code is a **restricted subset of TypeScript**, not a Code node and @@ -173,7 +166,9 @@ ${renderMethodLines()} ${SAFE_METHODS_SENTENCE} -${includeGroups ? `${NODE_GROUPS_REFERENCE}\n\n` : ''}## Forbidden constructs +${NODE_GROUPS_REFERENCE} + +## Forbidden constructs ${renderForbiddenLines()} @@ -214,7 +209,6 @@ node access, or a step that would otherwise need three or more native nodes. /** * Full reference including groups docs. Materialized into Instance AI's - * knowledge base for on-demand reading; the MCP SDK reference embeds the - * groups-gated variant via `buildSdkLanguageReference` instead. + * knowledge base for on-demand reading and embedded in the MCP SDK reference. */ export const SDK_LANGUAGE_REFERENCE = buildSdkLanguageReference(); diff --git a/packages/cli/src/modules/mcp/__tests__/create-workflow-from-code.tool.test.ts b/packages/cli/src/modules/mcp/__tests__/create-workflow-from-code.tool.test.ts index c8dddc5ee5d..ba1444eaf8d 100644 --- a/packages/cli/src/modules/mcp/__tests__/create-workflow-from-code.tool.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/create-workflow-from-code.tool.test.ts @@ -7,10 +7,7 @@ import { mock } from 'vitest-mock-extended'; import { z } from 'zod'; import { McpPostSaveMetricsService } from '../mcp-post-save-metrics.service'; -import { - createCreateWorkflowFromCodeTool, - type CreateWorkflowFromCodeToolOptions, -} from '../tools/workflow-builder/create-workflow-from-code.tool'; +import { createCreateWorkflowFromCodeTool } from '../tools/workflow-builder/create-workflow-from-code.tool'; import { CredentialsService } from '@/credentials/credentials.service'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; @@ -179,7 +176,7 @@ describe('create-workflow-from-code MCP tool', () => { incrementPostSaveFailure: vi.fn(), }); - const createTool = (options?: CreateWorkflowFromCodeToolOptions) => + const createTool = () => createCreateWorkflowFromCodeTool( user, workflowCreationService, @@ -191,7 +188,6 @@ describe('create-workflow-from-code MCP tool', () => { projectRepository, dataTableOps as never, aiGatewayService, - options, logger, postSaveMetrics, ); @@ -1123,7 +1119,7 @@ describe('create-workflow-from-code MCP tool', () => { }); }); - describe('canvas groups (102_mcp_canvas_groups)', () => { + describe('canvas groups', () => { const nodeGroups = [{ id: 'g1', name: 'Ingestion', nodeIds: ['node-1', 'node-2'] }]; /** results.data of the last tracked telemetry event */ @@ -1134,22 +1130,7 @@ describe('create-workflow-from-code MCP tool', () => { return payload.results?.data; }; - test('flag off: groups from the code are dropped and telemetry is unchanged', async () => { - mockParseAndValidate.mockResolvedValue({ - workflow: { ...mockWorkflowJson, nodeGroups }, - warnings: [], - }); - - const result = await callHandler({ code: 'const wf = ...' }); - - expect(parseResult(result).workflowId).toBe('wf-saved-1'); - const passedWorkflow = createWorkflowMock.mock.calls[0][1] as WorkflowEntity; - expect(passedWorkflow).not.toHaveProperty('nodeGroups'); - // Telemetry payload is byte-identical to the pre-flag shape. - expect(trackedData()).toEqual({ workflowId: 'wf-saved-1', nodeCount: 2 }); - }); - - test('flag on: groups from the code are persisted on the created workflow', async () => { + test('groups from the code are persisted on the created workflow', async () => { mockParseAndValidate.mockResolvedValue({ workflow: { ...mockWorkflowJson, @@ -1161,10 +1142,7 @@ describe('create-workflow-from-code MCP tool', () => { warnings: [], }); - const result = await callHandler( - { code: 'const wf = ...' }, - createTool({ canvasGroupsEnabled: true }), - ); + const result = await callHandler({ code: 'const wf = ...' }, createTool()); expect(parseResult(result).workflowId).toBe('wf-saved-1'); const passedWorkflow = createWorkflowMock.mock.calls[0][1] as WorkflowEntity; @@ -1172,10 +1150,10 @@ describe('create-workflow-from-code MCP tool', () => { expect(trackedData()).toEqual({ workflowId: 'wf-saved-1', nodeCount: 2, groupCount: 1 }); }); - test('flag on: code without groups persists an empty group list', async () => { + test('code without groups persists an empty group list', async () => { mockParseAndValidate.mockResolvedValue({ workflow: mockWorkflowJson, warnings: [] }); - await callHandler({ code: 'const wf = ...' }, createTool({ canvasGroupsEnabled: true })); + await callHandler({ code: 'const wf = ...' }, createTool()); const passedWorkflow = createWorkflowMock.mock.calls[0][1] as WorkflowEntity; expect(passedWorkflow.nodeGroups).toEqual([]); @@ -1183,11 +1161,8 @@ describe('create-workflow-from-code MCP tool', () => { }); // Structural group rules (no triggers, single connected subgraph, no non-main connection - // crossing the group boundary) aren't checked before `workflowCreationService.createWorkflow` - // yet — that service's own `validateWorkflowNodeGroups` (a shared safety net used by the public - // API, editor controller, importer, etc.) still rejects the whole creation for an invalid group. - // These tests describe the target behavior for that follow-up: with canvasGroupsEnabled on, an - // invalid group should be dropped and reported in `skippedGroups` instead, while the rest of the + // crossing the group boundary) are checked before `workflowCreationService.createWorkflow`, + // so an invalid group is dropped and reported in `skippedGroups` while the rest of the // workflow is still created. describe('structural validation', () => { beforeEach(() => { @@ -1214,10 +1189,7 @@ describe('create-workflow-from-code MCP tool', () => { warnings: [], }); - const result = await callHandler( - { code: 'const wf = ...' }, - createTool({ canvasGroupsEnabled: true }), - ); + const result = await callHandler({ code: 'const wf = ...' }, createTool()); expect(result.isError).toBeUndefined(); expect(parseResult(result).workflowId).toBe('wf-saved-1'); @@ -1269,10 +1241,7 @@ describe('create-workflow-from-code MCP tool', () => { warnings: [], }); - const result = await callHandler( - { code: 'const wf = ...' }, - createTool({ canvasGroupsEnabled: true }), - ); + const result = await callHandler({ code: 'const wf = ...' }, createTool()); expect(result.isError).toBeUndefined(); @@ -1319,10 +1288,7 @@ describe('create-workflow-from-code MCP tool', () => { warnings: [], }); - const result = await callHandler( - { code: 'const wf = ...' }, - createTool({ canvasGroupsEnabled: true }), - ); + const result = await callHandler({ code: 'const wf = ...' }, createTool()); expect(result.isError).toBeUndefined(); @@ -1380,10 +1346,7 @@ describe('create-workflow-from-code MCP tool', () => { warnings: [], }); - const result = await callHandler( - { code: 'const wf = ...' }, - createTool({ canvasGroupsEnabled: true }), - ); + const result = await callHandler({ code: 'const wf = ...' }, createTool()); expect(result.isError).toBeUndefined(); @@ -1408,10 +1371,7 @@ describe('create-workflow-from-code MCP tool', () => { warnings: [], }); - const result = await callHandler( - { code: 'const wf = ...' }, - createTool({ canvasGroupsEnabled: true }), - ); + const result = await callHandler({ code: 'const wf = ...' }, createTool()); expect(result.isError).toBeUndefined(); @@ -1430,10 +1390,7 @@ describe('create-workflow-from-code MCP tool', () => { warnings: [], }); - const result = await callHandler( - { code: 'const wf = ...' }, - createTool({ canvasGroupsEnabled: true }), - ); + const result = await callHandler({ code: 'const wf = ...' }, createTool()); const response = parseResult(result); expect(response.skippedGroups).toEqual([ @@ -1455,24 +1412,7 @@ describe('create-workflow-from-code MCP tool', () => { parameters: {}, })); - test('flag on: a saved canvas over the ceiling with no groups gets a warning', async () => { - mockParseAndValidate.mockResolvedValue({ - workflow: { ...mockWorkflowJson, nodes: wideNodes }, - warnings: [], - }); - - const result = await callHandler( - { code: 'const wf = ...' }, - createTool({ canvasGroupsEnabled: true }), - ); - - const response = parseResult(result); - expect(response.warnings).toEqual([ - expect.objectContaining({ code: 'TOP_LEVEL_ITEMS_OVER_CEILING' }), - ]); - }); - - test('flag off: no ceiling warning, groups cannot be kept anyway', async () => { + test('a saved canvas over the ceiling with no groups gets a warning', async () => { mockParseAndValidate.mockResolvedValue({ workflow: { ...mockWorkflowJson, nodes: wideNodes }, warnings: [], @@ -1480,7 +1420,10 @@ describe('create-workflow-from-code MCP tool', () => { const result = await callHandler({ code: 'const wf = ...' }, createTool()); - expect(parseResult(result)).not.toHaveProperty('warnings'); + const response = parseResult(result); + expect(response.warnings).toEqual([ + expect.objectContaining({ code: 'TOP_LEVEL_ITEMS_OVER_CEILING' }), + ]); }); }); }); diff --git a/packages/cli/src/modules/mcp/__tests__/get-workflow-best-practices.tool.test.ts b/packages/cli/src/modules/mcp/__tests__/get-workflow-best-practices.tool.test.ts index 33fd5bca08b..eb4e9c045a2 100644 --- a/packages/cli/src/modules/mcp/__tests__/get-workflow-best-practices.tool.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/get-workflow-best-practices.tool.test.ts @@ -32,8 +32,7 @@ describe('get-workflow-best-practices MCP tool', () => { telemetry = mock(); }); - const createTool = (canvasGroupsEnabled = false) => - createGetWorkflowBestPracticesTool(user, telemetry, { canvasGroupsEnabled }); + const createTool = () => createGetWorkflowBestPracticesTool(user, telemetry); const textOf = (result: { content: Array<{ type: string; text?: string }> }) => result.content.map((c) => c.text ?? '').join('\n'); @@ -140,30 +139,13 @@ describe('get-workflow-best-practices MCP tool', () => { }); describe('node grouping guidance', () => { - const listText = async (canvasGroupsEnabled: boolean) => { - const result = await createTool(canvasGroupsEnabled).handler( - { technique: 'list' }, - {} as never, - ); - return textOf(result); - }; + test('appends a grouping guidance section to the technique list', async () => { + const result = await createTool().handler({ technique: 'list' }, {} as never); + const text = textOf(result); - describe('when canvasGroupsEnabled is true', () => { - test('appends a grouping guidance section to the technique list', async () => { - const text = await listText(true); - - expect(text).toContain('## Grouping'); - expect(text).toMatch(/sub-workflow/i); // groups vs sub-workflows - expect(text).toMatch(/collapsed/i); // created collapsed by default - }); - }); - - describe('when canvasGroupsEnabled is false', () => { - test('does not mention grouping in the technique list', async () => { - const text = await listText(false); - - expect(text).not.toContain('## Grouping'); - }); + expect(text).toContain('## Grouping'); + expect(text).toMatch(/sub-workflow/i); // groups vs sub-workflows + expect(text).toMatch(/collapsed/i); // created collapsed by default }); }); }); diff --git a/packages/cli/src/modules/mcp/__tests__/get-workflow-sdk-reference.tool.test.ts b/packages/cli/src/modules/mcp/__tests__/get-workflow-sdk-reference.tool.test.ts index 9d83bd5e5f8..aedf9fb8d9d 100644 --- a/packages/cli/src/modules/mcp/__tests__/get-workflow-sdk-reference.tool.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/get-workflow-sdk-reference.tool.test.ts @@ -66,17 +66,13 @@ describe('get-workflow-sdk-reference MCP tool', () => { expect(content).toContain('## Where to put runtime logic'); }); - test('embeds the groups docs exactly once when the flag is on, never when off', () => { - const withGroups = getSdkReferenceContent(undefined, { includeGroups: true }); - expect(withGroups.split('## Node groups')).toHaveLength(2); - - const withoutGroups = getSdkReferenceContent(undefined, { includeGroups: false }); - expect(withoutGroups).not.toContain('## Node groups'); + test('embeds the groups docs exactly once', () => { + expect(getSdkReferenceContent().split('## Node groups')).toHaveLength(2); }); }); test('accepts patterns_detailed as a tool section', async () => { - const tool = createGetWorkflowSdkReferenceTool(user, telemetry, { canvasGroupsEnabled: false }); + const tool = createGetWorkflowSdkReferenceTool(user, telemetry); const sectionSchema = tool.config.inputSchema?.section; expect(tool.config.description).toContain('Required reference'); @@ -93,75 +89,34 @@ describe('get-workflow-sdk-reference MCP tool', () => { }); }); - describe('node groups (canvasGroupsEnabled)', () => { + describe('node groups', () => { describe('getSdkReferenceContent', () => { - test('embeds verbatim the exact shared contents with the IAI in the full reference when enabled', () => { - expect(getSdkReferenceContent(undefined, { includeGroups: true })).toContain( - NODE_GROUPS_REFERENCE, - ); + test('embeds verbatim the exact shared contents with the IAI in the full reference', () => { + expect(getSdkReferenceContent()).toContain(NODE_GROUPS_REFERENCE); }); - test('returns only the group section for section="groups" when enabled', () => { - const content = getSdkReferenceContent('groups', { includeGroups: true }); + test('returns only the group section for section="groups"', () => { + const content = getSdkReferenceContent('groups'); expect(content).toContain(NODE_GROUPS_REFERENCE); // Just the group section — not the rest of the reference. expect(content).not.toContain('## Workflow Patterns'); expect(content).not.toContain(''); }); - - test('omits the groups description when disabled, leaving today’s output unchanged', () => { - const withFlagOff = getSdkReferenceContent(undefined, { includeGroups: false }); - - expect(withFlagOff).not.toContain(NODE_GROUPS_REFERENCE); - - expect(withFlagOff).toBe(getSdkReferenceContent()); - }); - - test('section="groups" yields no group content when disabled', () => { - expect(getSdkReferenceContent('groups', { includeGroups: false })).not.toContain( - NODE_GROUPS_REFERENCE, - ); - }); }); describe('createGetWorkflowSdkReferenceTool', () => { - describe('When canvasGroupsEnabled is true', () => { - test('the tool accepts section="groups"', () => { - const enabled = createGetWorkflowSdkReferenceTool(user, telemetry, { - canvasGroupsEnabled: true, - }); + test('the tool accepts section="groups"', () => { + const tool = createGetWorkflowSdkReferenceTool(user, telemetry); - expect(enabled.config.inputSchema?.section.safeParse('groups').success).toBe(true); - }); - - test('the tool handler serves the groups reference', async () => { - const enabled = createGetWorkflowSdkReferenceTool(user, telemetry, { - canvasGroupsEnabled: true, - }); - - const enabledResult = await enabled.handler({ section: 'groups' }, {} as never); - expect(structuredOf(enabledResult)?.reference).toContain(NODE_GROUPS_REFERENCE); - }); + expect(tool.config.inputSchema?.section.safeParse('groups').success).toBe(true); }); - describe('When canvasGroupsEnabled is false', () => { - test('the tool does not accept section="groups"', () => { - const disabled = createGetWorkflowSdkReferenceTool(user, telemetry, { - canvasGroupsEnabled: false, - }); + test('the tool handler serves the groups reference', async () => { + const tool = createGetWorkflowSdkReferenceTool(user, telemetry); - expect(disabled.config.inputSchema?.section.safeParse('groups').success).toBe(false); - }); - - test('the tool handler does not serve the groups reference', async () => { - const disabled = createGetWorkflowSdkReferenceTool(user, telemetry, { - canvasGroupsEnabled: false, - }); - - const disabledResult = await disabled.handler({ section: undefined }, {} as never); - expect(structuredOf(disabledResult)?.reference).not.toContain(NODE_GROUPS_REFERENCE); - }); + const result = await tool.handler({ section: 'groups' }, {} as never); + expect(structuredOf(result)?.reference).toContain(NODE_GROUPS_REFERENCE); }); }); }); diff --git a/packages/cli/src/modules/mcp/__tests__/mcp-instructions.test.ts b/packages/cli/src/modules/mcp/__tests__/mcp-instructions.test.ts index 54e4648b0a5..cf0efc26ab0 100644 --- a/packages/cli/src/modules/mcp/__tests__/mcp-instructions.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/mcp-instructions.test.ts @@ -35,52 +35,25 @@ describe('getMcpInstructions', () => { }); describe('node groups pointer', () => { - describe('when canvasGroupsEnabled is true', () => { - test('points the client to the groups reference', () => { - const instructions = getMcpInstructions({ - isBuilderEnabled: true, - isN8nConnectAvailable: true, - canvasGroupsEnabled: true, - }); - - expect(instructions).toMatch(/group/i); - // Points at the on-demand groups section of the SDK reference. - expect(instructions).toContain('"groups"'); + test('points the client to the groups reference', () => { + const instructions = getMcpInstructions({ + isBuilderEnabled: true, + isN8nConnectAvailable: true, }); - test('stays intro-only when the builder is disabled', () => { - const instructions = getMcpInstructions({ - isBuilderEnabled: false, - isN8nConnectAvailable: false, - canvasGroupsEnabled: true, - }); - - expect(instructions).toContain('official MCP server for n8n'); - expect(instructions).not.toContain('"groups"'); - }); + expect(instructions).toMatch(/group/i); + // Points at the on-demand groups section of the SDK reference. + expect(instructions).toContain('"groups"'); }); - describe('when canvasGroupsEnabled is false', () => { - test('does not mention the groups reference', () => { - const instructions = getMcpInstructions({ - isBuilderEnabled: true, - isN8nConnectAvailable: true, - canvasGroupsEnabled: false, - }); - - expect(instructions).not.toContain('"groups"'); - expect(instructions).not.toContain('does not fail the whole update'); - expect(instructions).not.toContain('skippedOperations'); + test('stays intro-only when the builder is disabled', () => { + const instructions = getMcpInstructions({ + isBuilderEnabled: false, + isN8nConnectAvailable: false, }); - test('omits the groups pointer by default', () => { - const instructions = getMcpInstructions({ - isBuilderEnabled: true, - isN8nConnectAvailable: true, - }); - - expect(instructions).not.toContain('"groups"'); - }); + expect(instructions).toContain('official MCP server for n8n'); + expect(instructions).not.toContain('"groups"'); }); }); @@ -132,7 +105,6 @@ describe('getMcpInstructions', () => { const instructions = getMcpInstructions({ isBuilderEnabled: true, isN8nConnectAvailable: true, - canvasGroupsEnabled: true, isAgentsEnabled: true, isUserPreferencesEnabled: true, }); diff --git a/packages/cli/src/modules/mcp/__tests__/mcp-scopes.test.ts b/packages/cli/src/modules/mcp/__tests__/mcp-scopes.test.ts index c704d20946e..91d47eb7475 100644 --- a/packages/cli/src/modules/mcp/__tests__/mcp-scopes.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/mcp-scopes.test.ts @@ -61,7 +61,6 @@ const ALL_MAPPED_TOOLS = new Set(Object.values(TOOLS_BY_SCOPE).flat()); const mcpFeatureFlags = (overrides: Partial = {}): McpFeatureFlags => ({ mcpApps: { enabled: false, variant: 'unassigned' }, - canvasGroupsEnabled: false, instanceContextEnabled: false, // On by default so the drift guards below cover `get_user_preferences`. Its own // registration tests set it explicitly either way. diff --git a/packages/cli/src/modules/mcp/__tests__/mcp.controller.test.ts b/packages/cli/src/modules/mcp/__tests__/mcp.controller.test.ts index 14157472d31..25d99f5faef 100644 --- a/packages/cli/src/modules/mcp/__tests__/mcp.controller.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/mcp.controller.test.ts @@ -118,7 +118,6 @@ describe('McpController', () => { // when a flag matters. (mcpService.resolveFeatureFlags as Mock).mockResolvedValue({ mcpApps: { enabled: false, variant: 'unassigned' }, - canvasGroupsEnabled: false, }); Container.set(Logger, logger); @@ -171,7 +170,6 @@ describe('McpController', () => { }); (mcpService.resolveFeatureFlags as Mock).mockResolvedValue({ mcpApps: { enabled: true, variant: 'variant' }, - canvasGroupsEnabled: true, }); const res = createRes(); @@ -195,7 +193,6 @@ describe('McpController', () => { mcp_connection_status: 'success', mcp_apps_enabled: true, mcp_apps_variant: 'variant', - mcp_canvas_groups_enabled: true, }); }); @@ -207,7 +204,6 @@ describe('McpController', () => { }); (mcpService.resolveFeatureFlags as Mock).mockResolvedValue({ mcpApps: { enabled: false, variant: 'unassigned' }, - canvasGroupsEnabled: false, }); const res = createRes(); @@ -240,7 +236,6 @@ describe('McpController', () => { mcp_connection_status: 'success', mcp_apps_enabled: false, mcp_apps_variant: 'unassigned', - mcp_canvas_groups_enabled: false, }); }); @@ -357,7 +352,6 @@ describe('McpController', () => { }); (mcpService.resolveFeatureFlags as Mock).mockResolvedValue({ mcpApps: { enabled: true, variant: 'env_override' }, - canvasGroupsEnabled: false, }); const res = createRes(); @@ -389,7 +383,6 @@ describe('McpController', () => { }); (mcpService.resolveFeatureFlags as Mock).mockResolvedValue({ mcpApps: { enabled: true, variant: 'variant' }, - canvasGroupsEnabled: false, }); const res = createRes(); @@ -407,7 +400,7 @@ describe('McpController', () => { expect(mcpService.resolveFeatureFlags as Mock).toHaveBeenCalledTimes(1); expect(mcpService.getServer as unknown as Mock).toHaveBeenCalledWith( expect.objectContaining({ id: 'user-1' }), - { mcpApps: { enabled: true, variant: 'variant' }, canvasGroupsEnabled: false }, + { mcpApps: { enabled: true, variant: 'variant' } }, { name: 'Claude', version: '1.0.0' }, { caller: undefined, grantedScopes: undefined }, ); @@ -424,7 +417,6 @@ describe('McpController', () => { }); (mcpService.resolveFeatureFlags as Mock).mockResolvedValue({ mcpApps: { enabled: false, variant: 'unassigned' }, - canvasGroupsEnabled: false, aiPreferencesEnabled: true, }); const res = createRes(); @@ -462,7 +454,6 @@ describe('McpController', () => { }); (mcpService.resolveFeatureFlags as Mock).mockResolvedValue({ mcpApps: { enabled: false, variant: 'control' }, - canvasGroupsEnabled: false, }); const res = createRes(); @@ -481,7 +472,7 @@ describe('McpController', () => { expect(mcpService.resolveFeatureFlags as Mock).toHaveBeenCalledTimes(1); expect(mcpService.getServer as unknown as Mock).toHaveBeenCalledWith( expect.objectContaining({ id: 'user-1' }), - { mcpApps: { enabled: false, variant: 'control' }, canvasGroupsEnabled: false }, + { mcpApps: { enabled: false, variant: 'control' } }, undefined, { caller: undefined, grantedScopes: undefined }, ); diff --git a/packages/cli/src/modules/mcp/__tests__/mcp.service.test.ts b/packages/cli/src/modules/mcp/__tests__/mcp.service.test.ts index 0c9583b1fb6..e17dd67cce2 100644 --- a/packages/cli/src/modules/mcp/__tests__/mcp.service.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/mcp.service.test.ts @@ -3,7 +3,6 @@ import { MCP_APPS_FLAG, MCP_APPS_VARIANT_CONTROL, MCP_APPS_VARIANT_ENABLED, - MCP_CANVAS_GROUPS_FLAG, CONTEXT_PREFERENCES_CONTROL_VARIANT, CONTEXT_PREFERENCES_ENABLED_VARIANT, CONTEXT_PREFERENCES_FLAG, @@ -74,7 +73,6 @@ const mockAiGatewayService = () => const mcpFeatureFlags = (overrides: Partial = {}): McpFeatureFlags => ({ mcpApps: { enabled: false, variant: 'unassigned' }, - canvasGroupsEnabled: false, instanceContextEnabled: false, aiPreferencesEnabled: false, ...overrides, @@ -360,7 +358,6 @@ describe('McpService', () => { const buildResolutionService = (opts: { postHogClient: Mocked; mcpAppsEnabled?: boolean; - mcpCanvasGroupsEnabled?: boolean; mcpInstanceContextEnabled?: boolean; }) => new McpService( @@ -377,7 +374,6 @@ describe('McpService', () => { webhook: '/webhook', webhookTest: '/webhook-test', mcpAppsEnabled: opts.mcpAppsEnabled ?? false, - mcpCanvasGroupsEnabled: opts.mcpCanvasGroupsEnabled ?? false, mcpInstanceContextEnabled: opts.mcpInstanceContextEnabled ?? false, }, }), @@ -418,13 +414,11 @@ describe('McpService', () => { const postHogClient = mockInstance(PostHogClient); postHogClient.getFeatureFlags.mockResolvedValue({ [MCP_APPS_FLAG]: MCP_APPS_VARIANT_ENABLED, - [MCP_CANVAS_GROUPS_FLAG]: true, }); const service = buildResolutionService({ postHogClient }); await expect(service.resolveFeatureFlags(user)).resolves.toEqual({ mcpApps: { enabled: true, variant: 'variant' }, - canvasGroupsEnabled: true, instanceContextEnabled: false, aiPreferencesEnabled: false, }); @@ -550,56 +544,13 @@ describe('McpService', () => { }); }); - describe('canvas groups', () => { - it('enables canvas groups for users with the boolean flag set', async () => { - const postHogClient = mockInstance(PostHogClient); - postHogClient.getFeatureFlags.mockResolvedValue({ [MCP_CANVAS_GROUPS_FLAG]: true }); - const service = buildResolutionService({ postHogClient }); - - await expect(service.resolveFeatureFlags(user)).resolves.toMatchObject({ - canvasGroupsEnabled: true, - }); - }); - - it('keeps canvas groups disabled when the flag is missing', async () => { - const postHogClient = mockInstance(PostHogClient); - postHogClient.getFeatureFlags.mockResolvedValue({}); - const service = buildResolutionService({ postHogClient }); - - await expect(service.resolveFeatureFlags(user)).resolves.toMatchObject({ - canvasGroupsEnabled: false, - }); - }); - - it('treats non-boolean flag values as disabled', async () => { - const postHogClient = mockInstance(PostHogClient); - postHogClient.getFeatureFlags.mockResolvedValue({ [MCP_CANVAS_GROUPS_FLAG]: 'variant' }); - const service = buildResolutionService({ postHogClient }); - - await expect(service.resolveFeatureFlags(user)).resolves.toMatchObject({ - canvasGroupsEnabled: false, - }); - }); - - it('enables canvas groups when the operator force-enables them', async () => { - const postHogClient = mockInstance(PostHogClient); - postHogClient.getFeatureFlags.mockResolvedValue({}); - const service = buildResolutionService({ postHogClient, mcpCanvasGroupsEnabled: true }); - - await expect(service.resolveFeatureFlags(user)).resolves.toMatchObject({ - canvasGroupsEnabled: true, - }); - }); - }); - it('still queries PostHog when only some features are env-overridden', async () => { const postHogClient = mockInstance(PostHogClient); - postHogClient.getFeatureFlags.mockResolvedValue({ [MCP_CANVAS_GROUPS_FLAG]: true }); + postHogClient.getFeatureFlags.mockResolvedValue({}); const service = buildResolutionService({ postHogClient, mcpAppsEnabled: true }); await expect(service.resolveFeatureFlags(user)).resolves.toEqual({ mcpApps: { enabled: true, variant: 'env_override' }, - canvasGroupsEnabled: true, instanceContextEnabled: false, aiPreferencesEnabled: false, }); @@ -615,13 +566,11 @@ describe('McpService', () => { const service = buildResolutionService({ postHogClient, mcpAppsEnabled: true, - mcpCanvasGroupsEnabled: true, mcpInstanceContextEnabled: true, }); await expect(service.resolveFeatureFlags(user)).resolves.toEqual({ mcpApps: { enabled: true, variant: 'env_override' }, - canvasGroupsEnabled: true, instanceContextEnabled: true, aiPreferencesEnabled: true, }); diff --git a/packages/cli/src/modules/mcp/__tests__/update-workflow.tool.test.ts b/packages/cli/src/modules/mcp/__tests__/update-workflow.tool.test.ts index 543295297d6..68a3dd3feec 100644 --- a/packages/cli/src/modules/mcp/__tests__/update-workflow.tool.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/update-workflow.tool.test.ts @@ -201,7 +201,7 @@ describe('update-workflow MCP tool', () => { incrementPostSaveFailure: vi.fn(), }); - const createTool = (options?: { canvasGroupsEnabled?: boolean }) => + const createTool = () => createUpdateWorkflowTool( user, workflowFinderService, @@ -218,7 +218,6 @@ describe('update-workflow MCP tool', () => { subworkflowPolicyChecker, workflowPublishedDataService, aiGatewayService, - options, logger, postSaveMetrics, ); @@ -266,31 +265,22 @@ describe('update-workflow MCP tool', () => { // `$ref: "#/properties/..."` pointer, which strict MCP clients cannot // resolve — they only follow refs into `$defs` — leaving this tool's // arguments unvalidated client-side. - test.each([{ canvasGroupsEnabled: false }, { canvasGroupsEnabled: true }])( - 'served input schema is self-contained — no JSON pointer refs (canvasGroupsEnabled: $canvasGroupsEnabled)', - ({ canvasGroupsEnabled }) => { - const tool = createTool({ canvasGroupsEnabled }); - const served = shapeToStandardSchema(tool.config.inputSchema!)[ - '~standard' - ].jsonSchema.input({ target: 'draft-2020-12' }); + test('served input schema is self-contained — no JSON pointer refs', () => { + const tool = createTool(); + const served = shapeToStandardSchema(tool.config.inputSchema!)['~standard'].jsonSchema.input({ + target: 'draft-2020-12', + }); - expect(JSON.stringify(served)).not.toContain('$ref'); - }, - ); + expect(JSON.stringify(served)).not.toContain('$ref'); + }); }); - describe('docs mention the non-fatal group exception only when canvasGroupsEnabled', () => { + describe('docs mention the non-fatal group exception', () => { const operationsDescription = (tool: ReturnType) => (tool.config.inputSchema!.operations as z.ZodTypeAny).description; - test('flag off: neither the tool description nor the operations field mention the exception', () => { + test('both the tool description and the operations field name the non-fatal operation types', () => { const tool = createTool(); - expect(tool.config.description).not.toContain('one exception to "atomically"'); - expect(operationsDescription(tool)).not.toContain('except node-group operations'); - }); - - test('flag on: both the tool description and the operations field name the non-fatal operation types', () => { - const tool = createTool({ canvasGroupsEnabled: true }); const nonFatalTypesList = [...NON_FATAL_OPERATION_TYPES].join(', '); expect(tool.config.description).toContain( `Node-group operations (${nonFatalTypesList}) are the one exception to "atomically"`, @@ -405,31 +395,12 @@ describe('update-workflow MCP tool', () => { operations: [{ type: 'addNodeGroup', name: 'Group', nodeNames: ['A', 'B'] }], }; - test('published schema rejects gated group ops when the flag is off', () => { + test('published schema accepts group ops', () => { const parsed = buildPublishedInputSchema(createTool()).safeParse(addGroupInput); - expect(parsed.success).toBe(false); - }); - - test('published schema accepts gated group ops when the flag is on', () => { - const parsed = buildPublishedInputSchema(createTool({ canvasGroupsEnabled: true })).safeParse( - addGroupInput, - ); expect(parsed.success).toBe(true); }); - test('handler rejects gated group ops when the flag is off', async () => { - const result = await callHandler({ - workflowId: 'wf-1', - operations: [{ type: 'addNodeGroup', name: 'Group', nodeNames: ['A'] }], - }); - - expect(result.isError).toBe(true); - const response = parseResult(result); - expect(response.error).toContain('not available on this instance'); - expect(updateMock).not.toHaveBeenCalled(); - }); - - test('setNodeGroups works with the flag off', async () => { + test('setNodeGroups replaces the groups', async () => { const result = await callHandler({ workflowId: 'wf-1', operations: [ @@ -445,13 +416,13 @@ describe('update-workflow MCP tool', () => { expect(saved.nodeGroups).toEqual([{ id: 'g1', name: 'Group', nodeIds: ['a', 'b'] }]); }); - test('applies addNodeGroup end-to-end when the flag is on', async () => { + test('applies addNodeGroup end-to-end', async () => { const result = await callHandler( { workflowId: 'wf-1', operations: [{ type: 'addNodeGroup', id: 'g1', name: 'Group', nodeNames: ['A', 'B'] }], }, - createTool({ canvasGroupsEnabled: true }), + createTool(), ); expect(result.isError).toBeUndefined(); @@ -459,7 +430,7 @@ describe('update-workflow MCP tool', () => { expect(saved.nodeGroups).toEqual([{ id: 'g1', name: 'Group', nodeIds: ['a', 'b'] }]); }); - test('applies updateNodeGroup and removeNodeGroup against existing groups when the flag is on', async () => { + test('applies updateNodeGroup and removeNodeGroup against existing groups', async () => { findWorkflowMock.mockResolvedValue( Object.assign(buildExistingWorkflow(), { nodeGroups: [ @@ -482,7 +453,7 @@ describe('update-workflow MCP tool', () => { { type: 'removeNodeGroup', groupName: 'Second' }, ], }, - createTool({ canvasGroupsEnabled: true }), + createTool(), ); expect(result.isError).toBeUndefined(); @@ -492,7 +463,7 @@ describe('update-workflow MCP tool', () => { ]); }); - test('removeNode prunes the node from groups and persists them, regardless of the flag', async () => { + test('removeNode prunes the node from groups and persists them', async () => { findWorkflowMock.mockResolvedValue( Object.assign(buildExistingWorkflow(), { nodeGroups: [{ id: 'g1', name: 'Group', nodeIds: ['a', 'b'] }], @@ -562,14 +533,600 @@ describe('update-workflow MCP tool', () => { }) as typeof nodeTypes.getByNameAndVersion); }); - describe('canvasGroupsEnabled off', () => { - test('a structurally invalid group is not pre-checked; a persistence-layer rejection still surfaces as isError', async () => { - findWorkflowMock.mockResolvedValue(buildWorkflowWithTrigger()); - updateMock.mockRejectedValueOnce( - new Error('Node group "Group" cannot contain trigger nodes: Trigger.'), + test('a group with a trigger inside is skipped while the rest of the update saves', async () => { + findWorkflowMock.mockResolvedValue(buildWorkflowWithTrigger()); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [ + { type: 'updateNodeParameters', nodeName: 'B', parameters: { url: 'https://new' } }, + { + type: 'setNodeGroups', + nodeGroups: [{ id: 'g1', name: 'Group', nodeNames: ['Trigger', 'A'] }], + }, + ], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodes.find((n) => n.name === 'B')!.parameters).toEqual({ + url: 'https://new', + }); + expect(saved.nodeGroups ?? []).toEqual([]); + + const response = parseResult(result); + expect(response.skippedOperations).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + reason: expect.stringContaining('cannot contain trigger nodes') as string, + }), + ]), + ); + }); + + test('a non-group operation that disconnects an existing group is caught: the group is removed and reported in removedGroups, the rest of the update still saves', async () => { + // A removeConnection never touches nodeGroups directly, so + // nodeGroupsChanged alone would miss this — the structural check must + // run whenever the workflow HAS groups, not only when a group op ran. + findWorkflowMock.mockResolvedValue( + Object.assign(new WorkflowEntity(), { + id: 'wf-1', + name: 'Existing', + settings: { availableInMCP: true }, + nodes: [makeNode({ id: 'a', name: 'A' }), makeNode({ id: 'b', name: 'B' })], + connections: { + A: { main: [[{ node: 'B', type: 'main', index: 0 }]] }, + } as IConnections, + nodeGroups: [{ id: 'g1', name: 'Group', nodeIds: ['a', 'b'] }], + }), + ); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [{ type: 'removeConnection', source: 'A', target: 'B' }], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + // The connection removal itself still applied... + expect(saved.connections.A?.main?.[0] ?? []).toEqual([]); + // ...and the now-disconnected group was dropped, not silently re-sent as-is. + expect(saved.nodeGroups).toEqual([]); + + const response = parseResult(result); + // No operation was skipped: the caller asked for a removeConnection and got + // it. The group is collateral damage, so it belongs in removedGroups. + expect(response.skippedOperations ?? []).toEqual([]); + expect(response.removedGroups).toEqual([ + { + groupName: 'Group', + reason: expect.stringContaining('single connected subgraph') as string, + }, + ]); + }); + + describe('a submitted group overlapping an existing one', () => { + const buildWorkflowWithGroup = () => + Object.assign(buildWorkflowWithTrigger(), { + nodeGroups: [{ id: 'g1', name: 'Existing group', nodeIds: ['a', 'b'] }], + }); + + test('is skipped without taking the existing group down with it', async () => { + // The validator flags both sides of an overlap; only the submitted one + // may go, since the operation that caused it was rejected. + findWorkflowMock.mockResolvedValue(buildWorkflowWithGroup()); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [ + { type: 'addNodeGroup', name: 'Overlapping', nodeNames: ['Trigger', 'A'] }, + ], + }, + createTool(), ); - const result = await callHandler({ + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodeGroups).toEqual([ + { id: 'g1', name: 'Existing group', nodeIds: ['a', 'b'] }, + ]); + + const response = parseResult(result); + expect(response.skippedOperations).toEqual([ + { + opIndex: 0, + type: 'addNodeGroup', + reason: expect.stringContaining('belongs to multiple groups') as string, + }, + ]); + expect(response.removedGroups).toBeUndefined(); + }); + + test('does not stop the other operations in the same batch from saving', async () => { + findWorkflowMock.mockResolvedValue(buildWorkflowWithGroup()); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [ + { type: 'renameNode', oldName: 'B', newName: 'B renamed' }, + { type: 'addNodeGroup', name: 'Overlapping', nodeNames: ['Trigger', 'A'] }, + ], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodes.map((n) => n.name)).toContain('B renamed'); + expect(saved.nodeGroups).toHaveLength(1); + + const response = parseResult(result); + expect(response.appliedOperations).toBe(1); + expect(response.skippedOperations).toEqual([ + expect.objectContaining({ opIndex: 1, type: 'addNodeGroup' }), + ]); + expect(response.removedGroups).toBeUndefined(); + }); + + test('drops both when the same setNodeGroups submitted both of them', async () => { + // Neither group has priority here: the caller wrote both in one + // operation, so both belong in skippedOperations, not removedGroups. + findWorkflowMock.mockResolvedValue(buildWorkflowWithGroup()); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [ + { + type: 'setNodeGroups', + nodeGroups: [ + { id: 'g1', name: 'First', nodeNames: ['A', 'B'] }, + { id: 'g2', name: 'Second', nodeNames: ['A'] }, + ], + }, + ], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodeGroups).toEqual([]); + + const response = parseResult(result); + expect(response.removedGroups).toBeUndefined(); + expect(response.skippedOperations).toHaveLength(2); + }); + }); + + test('a group that splits an AI sub-node from its Agent is skipped', async () => { + findWorkflowMock.mockResolvedValue( + Object.assign(new WorkflowEntity(), { + id: 'wf-1', + name: 'Existing', + settings: { availableInMCP: true }, + nodes: [ + makeNode({ id: 'agent', name: 'Agent', type: '@n8n/n8n-nodes-langchain.agent' }), + makeNode({ + id: 'model', + name: 'Model', + type: '@n8n/n8n-nodes-langchain.agentTool', + position: [200, 0], + }), + ], + connections: { + Model: { + ai_languageModel: [[{ node: 'Agent', type: 'ai_languageModel', index: 0 }]], + }, + } as IConnections, + }), + ); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [{ type: 'addNodeGroup', name: 'Group', nodeNames: ['Agent'] }], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodeGroups ?? []).toEqual([]); + + const response = parseResult(result); + // The op that actually created this group was addNodeGroup, not setNodeGroups — + // the reported type must reflect that, not a hardcoded guess. + expect(response.skippedOperations).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + type: 'addNodeGroup', + reason: expect.stringContaining('cannot cross the') as string, + }), + ]), + ); + }); + + test('a group whose nodes form a disconnected subgraph is skipped', async () => { + findWorkflowMock.mockResolvedValue( + Object.assign(new WorkflowEntity(), { + id: 'wf-1', + name: 'Existing', + settings: { availableInMCP: true }, + nodes: [ + makeNode({ id: 'a', name: 'A' }), + makeNode({ id: 'b', name: 'B', position: [400, 0] }), + ], + connections: {} as IConnections, + }), + ); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [{ type: 'addNodeGroup', name: 'Group', nodeNames: ['A', 'B'] }], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodeGroups ?? []).toEqual([]); + + const response = parseResult(result); + expect(response.skippedOperations).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + reason: expect.stringContaining('single connected subgraph') as string, + }), + ]), + ); + }); + + test('one invalid group among valid ones in the same setNodeGroups only drops the invalid one', async () => { + findWorkflowMock.mockResolvedValue(buildWorkflowWithTrigger()); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [ + { + type: 'setNodeGroups', + nodeGroups: [ + { id: 'g1', name: 'Bad', nodeNames: ['Trigger'] }, + { id: 'g2', name: 'Good', nodeNames: ['A', 'B'] }, + ], + }, + ], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodeGroups).toEqual([{ id: 'g2', name: 'Good', nodeIds: ['a', 'b'] }]); + + const response = parseResult(result); + expect(response.skippedOperations).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + type: 'setNodeGroups', + reason: expect.stringContaining('cannot contain trigger nodes') as string, + }), + ]), + ); + // "Good" was persisted, so the operation did apply — just not in full. + expect(response.appliedOperations).toBe(1); + }); + + test('a setNodeGroups whose every group is dropped is discounted once, not per group', async () => { + // Trigger -> A -> B plus a detached C: one group fails on the trigger and + // the other on connectivity, with no member overlap between them. + findWorkflowMock.mockResolvedValue( + Object.assign(buildWorkflowWithTrigger(), { + nodes: [ + makeNode({ id: 'trigger', name: 'Trigger', type: 'n8n-nodes-base.manualTrigger' }), + makeNode({ id: 'a', name: 'A', position: [200, 0] }), + makeNode({ id: 'b', name: 'B', position: [400, 0] }), + makeNode({ id: 'c', name: 'C', position: [600, 200] }), + ], + }), + ); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [ + { + type: 'setNodeGroups', + nodeGroups: [ + { id: 'g1', name: 'Bad', nodeNames: ['Trigger'] }, + { id: 'g2', name: 'AlsoBad', nodeNames: ['A', 'C'] }, + ], + }, + ], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const response = parseResult(result); + // Two violations, one operation: the count must not go negative. + expect(response.skippedOperations).toHaveLength(2); + expect(response.appliedOperations).toBe(0); + }); + + test('a group made invalid via updateNodeGroup reports updateNodeGroup, not a hardcoded type', async () => { + findWorkflowMock.mockResolvedValue( + Object.assign(buildWorkflowWithTrigger(), { + nodeGroups: [{ id: 'g1', name: 'Group', nodeIds: ['a', 'b'] }], + }), + ); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [ + { type: 'updateNodeGroup', groupName: 'Group', nodeNames: ['Trigger', 'A', 'B'] }, + ], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodeGroups ?? []).toEqual([]); + + const response = parseResult(result); + expect(response.skippedOperations).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + type: 'updateNodeGroup', + reason: expect.stringContaining('cannot contain trigger nodes') as string, + }), + ]), + ); + }); + + test('when addNodeGroup then updateNodeGroup touch the same group in one batch, the later op wins', async () => { + findWorkflowMock.mockResolvedValue(buildWorkflowWithTrigger()); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [ + { type: 'addNodeGroup', name: 'Group', nodeNames: ['A', 'B'] }, + { type: 'updateNodeGroup', groupName: 'Group', nodeNames: ['Trigger', 'A', 'B'] }, + ], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodeGroups ?? []).toEqual([]); + + const response = parseResult(result); + // Created by addNodeGroup, then modified by updateNodeGroup — the reported + // type must reflect the LAST op that touched it, not the one that created it. + expect(response.skippedOperations).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + type: 'updateNodeGroup', + reason: expect.stringContaining('cannot contain trigger nodes') as string, + }), + ]), + ); + }); + + test('a group made invalid by removeNode pruning its bridge node is reported in removedGroups, not as a skipped removeNode', async () => { + // Trigger -> A -> B -> C, group {A, B, C}. Removing the bridge node B + // prunes it from the group, leaving {A, C} with no path between them. + findWorkflowMock.mockResolvedValue( + Object.assign(new WorkflowEntity(), { + id: 'wf-1', + name: 'Existing', + settings: { availableInMCP: true }, + nodes: [ + makeNode({ id: 'trigger', name: 'Trigger', type: 'n8n-nodes-base.manualTrigger' }), + makeNode({ id: 'a', name: 'A', position: [200, 0] }), + makeNode({ id: 'b', name: 'B', position: [400, 0] }), + makeNode({ id: 'c', name: 'C', position: [600, 0] }), + ], + connections: { + Trigger: { main: [[{ node: 'A', type: 'main', index: 0 }]] }, + A: { main: [[{ node: 'B', type: 'main', index: 0 }]] }, + B: { main: [[{ node: 'C', type: 'main', index: 0 }]] }, + } as IConnections, + nodeGroups: [{ id: 'g1', name: 'Group', nodeIds: ['a', 'b', 'c'] }], + }), + ); + + const result = await callHandler( + { workflowId: 'wf-1', operations: [{ type: 'removeNode', nodeName: 'B' }] }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodeGroups ?? []).toEqual([]); + + const response = parseResult(result); + // The node is gone: the removeNode applied in full. Pruning the group was + // a side effect, so the group's loss is not a skipped operation. + expect(response.skippedOperations ?? []).toEqual([]); + expect(response.appliedOperations).toBe(1); + expect(response.removedGroups).toEqual([ + { + groupName: 'Group', + reason: expect.stringContaining('single connected subgraph') as string, + }, + ]); + }); + + test('adding a node that branches out of an existing group removes the group and reports it as collateral', async () => { + // Trigger -> A -> B, group {A, B}. Branching a new node off A gives the + // group two outgoing boundary connections, breaking single-entry/exit. + findWorkflowMock.mockResolvedValue( + Object.assign(buildWorkflowWithTrigger(), { + nodeGroups: [{ id: 'g1', name: 'Chain', nodeIds: ['a', 'b'] }], + }), + ); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [ + { type: 'addNode', node: { name: 'C', type: 'n8n-nodes-base.set', typeVersion: 1 } }, + { type: 'addConnection', source: 'A', target: 'C' }, + ], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + // Both requested operations landed... + expect(saved.nodes.map((n) => n.name)).toContain('C'); + expect(saved.connections.A?.main?.[0]).toEqual( + expect.arrayContaining([expect.objectContaining({ node: 'C' })]), + ); + // ...and only the group was lost. + expect(saved.nodeGroups).toEqual([]); + + const response = parseResult(result); + expect(response.appliedOperations).toBe(2); + expect(response.skippedOperations ?? []).toEqual([]); + expect(response.removedGroups).toEqual([ + { + groupName: 'Chain', + reason: expect.stringContaining('single connected subgraph') as string, + }, + ]); + }); + + test('a group already invalid before this batch is removed and reported, whatever the operations were', async () => { + // Legacy data: basic-only validation (e.g. a git import) lets a group + // with a trigger through, so any later update has to clean it up — + // the same removal the canvas performs on load. + findWorkflowMock.mockResolvedValue( + Object.assign(buildWorkflowWithTrigger(), { + nodeGroups: [{ id: 'g1', name: 'Legacy', nodeIds: ['trigger', 'a'] }], + }), + ); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [{ type: 'setNodePosition', nodeName: 'B', position: [50, 50] }], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodeGroups).toEqual([]); + + const response = parseResult(result); + expect(response.appliedOperations).toBe(1); + expect(response.skippedOperations ?? []).toEqual([]); + expect(response.removedGroups).toEqual([ + { + groupName: 'Legacy', + reason: expect.stringContaining('cannot contain trigger nodes') as string, + }, + ]); + }); + + test('every skippedOperations entry carries the index of the operation it belongs to', async () => { + findWorkflowMock.mockResolvedValue(buildWorkflowWithTrigger()); + + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [ + { type: 'setNodePosition', nodeName: 'A', position: [10, 10] }, + // Fails the basic checks: unknown member. + { type: 'addNodeGroup', name: 'Missing', nodeNames: ['Nope'] }, + // Passes them, then fails the structural check. + { type: 'addNodeGroup', name: 'WithTrigger', nodeNames: ['Trigger', 'A'] }, + ], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const response = parseResult(result); + expect(response.skippedOperations).toEqual([ + expect.objectContaining({ opIndex: 1, type: 'addNodeGroup' }), + expect.objectContaining({ opIndex: 2, type: 'addNodeGroup' }), + ]); + expect(response.appliedOperations).toBe(1); + }); + + test('all groups valid: no skipped operations are reported', async () => { + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [{ type: 'addNodeGroup', name: 'Group', nodeNames: ['A', 'B'] }], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect(saved.nodeGroups).toEqual([ + { id: expect.any(String) as string, name: 'Group', nodeIds: ['a', 'b'] }, + ]); + + const response = parseResult(result); + expect(response.skippedOperations ?? []).toEqual([]); + }); + + test('no group operation in the batch: structural validation does not run', async () => { + const result = await callHandler( + { + workflowId: 'wf-1', + operations: [{ type: 'setNodePosition', nodeName: 'A', position: [50, 50] }], + }, + createTool(), + ); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + expect('nodeGroups' in saved).toBe(false); + }); + + test('the response reports skippedOperations with a human-readable reason', async () => { + findWorkflowMock.mockResolvedValue(buildWorkflowWithTrigger()); + + const result = await callHandler( + { workflowId: 'wf-1', operations: [ { @@ -577,357 +1134,55 @@ describe('update-workflow MCP tool', () => { nodeGroups: [{ id: 'g1', name: 'Group', nodeNames: ['Trigger', 'A'] }], }, ], - }); + }, + createTool(), + ); - expect(result.isError).toBe(true); - const response = parseResult(result); - expect(response.error).toContain('cannot contain trigger nodes'); - }); - - test('a non-group operation that disconnects an existing group is not pre-checked either', async () => { - findWorkflowMock.mockResolvedValue( - Object.assign(new WorkflowEntity(), { - id: 'wf-1', - name: 'Existing', - settings: { availableInMCP: true }, - nodes: [makeNode({ id: 'a', name: 'A' }), makeNode({ id: 'b', name: 'B' })], - connections: { - A: { main: [[{ node: 'B', type: 'main', index: 0 }]] }, - } as IConnections, - nodeGroups: [{ id: 'g1', name: 'Group', nodeIds: ['a', 'b'] }], + const response = parseResult(result); + expect(response.skippedOperations).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + reason: expect.stringContaining('cannot contain trigger nodes') as string, }), - ); - - const result = await callHandler({ - workflowId: 'wf-1', - operations: [{ type: 'removeConnection', source: 'A', target: 'B' }], - }); - - expect(result.isError).toBeUndefined(); - // nodeGroups isn't touched or re-checked with the flag off — omitted from - // the persisted payload exactly as before this fix (preserve-on-omit). - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect('nodeGroups' in saved).toBe(false); - }); + ]), + ); }); - describe('canvasGroupsEnabled on', () => { - const createOnTool = () => createTool({ canvasGroupsEnabled: true }); + describe('top-level ceiling warning', () => { + const looseNodes = (count: number) => + Array.from({ length: count }, (_, i) => + makeNode({ id: `n${i}`, name: `Step ${i}`, position: [i * 200, 0] }), + ); + const addNodeOps = (from: number, to: number) => + Array.from({ length: to - from }, (_, i) => ({ + type: 'addNode', + node: makeNode({ id: `n${from + i}`, name: `Step ${from + i}` }), + })); - test('a group with a trigger inside is skipped while the rest of the update saves', async () => { - findWorkflowMock.mockResolvedValue(buildWorkflowWithTrigger()); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [ - { type: 'updateNodeParameters', nodeName: 'B', parameters: { url: 'https://new' } }, - { - type: 'setNodeGroups', - nodeGroups: [{ id: 'g1', name: 'Group', nodeNames: ['Trigger', 'A'] }], - }, - ], - }, - createOnTool(), + test('an update that pushes the canvas over the ceiling gets a warning', async () => { + findWorkflowMock.mockResolvedValue( + Object.assign(buildExistingWorkflow(), { nodes: looseNodes(6), connections: {} }), ); - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodes.find((n) => n.name === 'B')!.parameters).toEqual({ - url: 'https://new', - }); - expect(saved.nodeGroups ?? []).toEqual([]); + const result = await callHandler( + { workflowId: 'wf-1', operations: addNodeOps(6, 8) }, + createTool(), + ); const response = parseResult(result); - expect(response.skippedOperations).toEqual( + expect(response.validationWarnings).toEqual( expect.arrayContaining([ - expect.objectContaining({ - reason: expect.stringContaining('cannot contain trigger nodes') as string, - }), + expect.objectContaining({ code: 'TOP_LEVEL_ITEMS_OVER_CEILING' }), ]), ); + expect(response.validationWarnings).not.toEqual( + expect.arrayContaining([expect.objectContaining({ preExisting: true })]), + ); }); - test('a non-group operation that disconnects an existing group is caught: the group is removed and reported in removedGroups, the rest of the update still saves', async () => { - // A removeConnection never touches nodeGroups directly, so - // nodeGroupsChanged alone would miss this — the structural check must - // run whenever the workflow HAS groups, not only when a group op ran. + test('a canvas already over the ceiling, with no box added, gets the warning marked pre-existing', async () => { findWorkflowMock.mockResolvedValue( - Object.assign(new WorkflowEntity(), { - id: 'wf-1', - name: 'Existing', - settings: { availableInMCP: true }, - nodes: [makeNode({ id: 'a', name: 'A' }), makeNode({ id: 'b', name: 'B' })], - connections: { - A: { main: [[{ node: 'B', type: 'main', index: 0 }]] }, - } as IConnections, - nodeGroups: [{ id: 'g1', name: 'Group', nodeIds: ['a', 'b'] }], - }), - ); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [{ type: 'removeConnection', source: 'A', target: 'B' }], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - // The connection removal itself still applied... - expect(saved.connections.A?.main?.[0] ?? []).toEqual([]); - // ...and the now-disconnected group was dropped, not silently re-sent as-is. - expect(saved.nodeGroups).toEqual([]); - - const response = parseResult(result); - // No operation was skipped: the caller asked for a removeConnection and got - // it. The group is collateral damage, so it belongs in removedGroups. - expect(response.skippedOperations ?? []).toEqual([]); - expect(response.removedGroups).toEqual([ - { - groupName: 'Group', - reason: expect.stringContaining('single connected subgraph') as string, - }, - ]); - }); - - describe('a submitted group overlapping an existing one', () => { - const buildWorkflowWithGroup = () => - Object.assign(buildWorkflowWithTrigger(), { - nodeGroups: [{ id: 'g1', name: 'Existing group', nodeIds: ['a', 'b'] }], - }); - - test('is skipped without taking the existing group down with it', async () => { - // The validator flags both sides of an overlap; only the submitted one - // may go, since the operation that caused it was rejected. - findWorkflowMock.mockResolvedValue(buildWorkflowWithGroup()); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [ - { type: 'addNodeGroup', name: 'Overlapping', nodeNames: ['Trigger', 'A'] }, - ], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodeGroups).toEqual([ - { id: 'g1', name: 'Existing group', nodeIds: ['a', 'b'] }, - ]); - - const response = parseResult(result); - expect(response.skippedOperations).toEqual([ - { - opIndex: 0, - type: 'addNodeGroup', - reason: expect.stringContaining('belongs to multiple groups') as string, - }, - ]); - expect(response.removedGroups).toBeUndefined(); - }); - - test('does not stop the other operations in the same batch from saving', async () => { - findWorkflowMock.mockResolvedValue(buildWorkflowWithGroup()); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [ - { type: 'renameNode', oldName: 'B', newName: 'B renamed' }, - { type: 'addNodeGroup', name: 'Overlapping', nodeNames: ['Trigger', 'A'] }, - ], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodes.map((n) => n.name)).toContain('B renamed'); - expect(saved.nodeGroups).toHaveLength(1); - - const response = parseResult(result); - expect(response.appliedOperations).toBe(1); - expect(response.skippedOperations).toEqual([ - expect.objectContaining({ opIndex: 1, type: 'addNodeGroup' }), - ]); - expect(response.removedGroups).toBeUndefined(); - }); - - test('drops both when the same setNodeGroups submitted both of them', async () => { - // Neither group has priority here: the caller wrote both in one - // operation, so both belong in skippedOperations, not removedGroups. - findWorkflowMock.mockResolvedValue(buildWorkflowWithGroup()); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [ - { - type: 'setNodeGroups', - nodeGroups: [ - { id: 'g1', name: 'First', nodeNames: ['A', 'B'] }, - { id: 'g2', name: 'Second', nodeNames: ['A'] }, - ], - }, - ], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodeGroups).toEqual([]); - - const response = parseResult(result); - expect(response.removedGroups).toBeUndefined(); - expect(response.skippedOperations).toHaveLength(2); - }); - }); - - test('a group that splits an AI sub-node from its Agent is skipped', async () => { - findWorkflowMock.mockResolvedValue( - Object.assign(new WorkflowEntity(), { - id: 'wf-1', - name: 'Existing', - settings: { availableInMCP: true }, - nodes: [ - makeNode({ id: 'agent', name: 'Agent', type: '@n8n/n8n-nodes-langchain.agent' }), - makeNode({ - id: 'model', - name: 'Model', - type: '@n8n/n8n-nodes-langchain.agentTool', - position: [200, 0], - }), - ], - connections: { - Model: { - ai_languageModel: [[{ node: 'Agent', type: 'ai_languageModel', index: 0 }]], - }, - } as IConnections, - }), - ); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [{ type: 'addNodeGroup', name: 'Group', nodeNames: ['Agent'] }], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodeGroups ?? []).toEqual([]); - - const response = parseResult(result); - // The op that actually created this group was addNodeGroup, not setNodeGroups — - // the reported type must reflect that, not a hardcoded guess. - expect(response.skippedOperations).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - type: 'addNodeGroup', - reason: expect.stringContaining('cannot cross the') as string, - }), - ]), - ); - }); - - test('a group whose nodes form a disconnected subgraph is skipped', async () => { - findWorkflowMock.mockResolvedValue( - Object.assign(new WorkflowEntity(), { - id: 'wf-1', - name: 'Existing', - settings: { availableInMCP: true }, - nodes: [ - makeNode({ id: 'a', name: 'A' }), - makeNode({ id: 'b', name: 'B', position: [400, 0] }), - ], - connections: {} as IConnections, - }), - ); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [{ type: 'addNodeGroup', name: 'Group', nodeNames: ['A', 'B'] }], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodeGroups ?? []).toEqual([]); - - const response = parseResult(result); - expect(response.skippedOperations).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - reason: expect.stringContaining('single connected subgraph') as string, - }), - ]), - ); - }); - - test('one invalid group among valid ones in the same setNodeGroups only drops the invalid one', async () => { - findWorkflowMock.mockResolvedValue(buildWorkflowWithTrigger()); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [ - { - type: 'setNodeGroups', - nodeGroups: [ - { id: 'g1', name: 'Bad', nodeNames: ['Trigger'] }, - { id: 'g2', name: 'Good', nodeNames: ['A', 'B'] }, - ], - }, - ], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodeGroups).toEqual([{ id: 'g2', name: 'Good', nodeIds: ['a', 'b'] }]); - - const response = parseResult(result); - expect(response.skippedOperations).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - type: 'setNodeGroups', - reason: expect.stringContaining('cannot contain trigger nodes') as string, - }), - ]), - ); - // "Good" was persisted, so the operation did apply — just not in full. - expect(response.appliedOperations).toBe(1); - }); - - test('a setNodeGroups whose every group is dropped is discounted once, not per group', async () => { - // Trigger -> A -> B plus a detached C: one group fails on the trigger and - // the other on connectivity, with no member overlap between them. - findWorkflowMock.mockResolvedValue( - Object.assign(buildWorkflowWithTrigger(), { - nodes: [ - makeNode({ id: 'trigger', name: 'Trigger', type: 'n8n-nodes-base.manualTrigger' }), - makeNode({ id: 'a', name: 'A', position: [200, 0] }), - makeNode({ id: 'b', name: 'B', position: [400, 0] }), - makeNode({ id: 'c', name: 'C', position: [600, 200] }), - ], - }), + Object.assign(buildExistingWorkflow(), { nodes: looseNodes(9), connections: {} }), ); const result = await callHandler( @@ -935,407 +1190,70 @@ describe('update-workflow MCP tool', () => { workflowId: 'wf-1', operations: [ { - type: 'setNodeGroups', - nodeGroups: [ - { id: 'g1', name: 'Bad', nodeNames: ['Trigger'] }, - { id: 'g2', name: 'AlsoBad', nodeNames: ['A', 'C'] }, - ], + type: 'updateNodeParameters', + nodeName: 'Step 1', + parameters: { url: 'https://new' }, }, ], }, - createOnTool(), + createTool(), ); - expect(result.isError).toBeUndefined(); - const response = parseResult(result); - // Two violations, one operation: the count must not go negative. - expect(response.skippedOperations).toHaveLength(2); - expect(response.appliedOperations).toBe(0); - }); - - test('a group made invalid via updateNodeGroup reports updateNodeGroup, not a hardcoded type', async () => { - findWorkflowMock.mockResolvedValue( - Object.assign(buildWorkflowWithTrigger(), { - nodeGroups: [{ id: 'g1', name: 'Group', nodeIds: ['a', 'b'] }], - }), - ); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [ - { type: 'updateNodeGroup', groupName: 'Group', nodeNames: ['Trigger', 'A', 'B'] }, - ], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodeGroups ?? []).toEqual([]); - - const response = parseResult(result); - expect(response.skippedOperations).toEqual( + expect(response.validationWarnings).toEqual( expect.arrayContaining([ expect.objectContaining({ - type: 'updateNodeGroup', - reason: expect.stringContaining('cannot contain trigger nodes') as string, + code: 'TOP_LEVEL_ITEMS_OVER_CEILING', + preExisting: true, + message: expect.stringContaining('[pre-existing]') as string, }), ]), ); }); - test('when addNodeGroup then updateNodeGroup touch the same group in one batch, the later op wins', async () => { - findWorkflowMock.mockResolvedValue(buildWorkflowWithTrigger()); + test('swapping one loose node for a new one keeps the box count but is not pre-existing', async () => { + findWorkflowMock.mockResolvedValue( + Object.assign(buildExistingWorkflow(), { nodes: looseNodes(9), connections: {} }), + ); const result = await callHandler( { workflowId: 'wf-1', - operations: [ - { type: 'addNodeGroup', name: 'Group', nodeNames: ['A', 'B'] }, - { type: 'updateNodeGroup', groupName: 'Group', nodeNames: ['Trigger', 'A', 'B'] }, - ], + operations: [{ type: 'removeNode', nodeName: 'Step 8' }, ...addNodeOps(9, 10)], }, - createOnTool(), + createTool(), ); - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodeGroups ?? []).toEqual([]); - const response = parseResult(result); - // Created by addNodeGroup, then modified by updateNodeGroup — the reported - // type must reflect the LAST op that touched it, not the one that created it. - expect(response.skippedOperations).toEqual( + expect(response.validationWarnings).toEqual( expect.arrayContaining([ - expect.objectContaining({ - type: 'updateNodeGroup', - reason: expect.stringContaining('cannot contain trigger nodes') as string, - }), + expect.objectContaining({ code: 'TOP_LEVEL_ITEMS_OVER_CEILING' }), ]), ); + expect(response.validationWarnings).not.toEqual( + expect.arrayContaining([expect.objectContaining({ preExisting: true })]), + ); }); - test('a group made invalid by removeNode pruning its bridge node is reported in removedGroups, not as a skipped removeNode', async () => { - // Trigger -> A -> B -> C, group {A, B, C}. Removing the bridge node B - // prunes it from the group, leaving {A, C} with no path between them. + test('a canvas already over the ceiling that this update adds loose nodes to is not marked pre-existing', async () => { findWorkflowMock.mockResolvedValue( - Object.assign(new WorkflowEntity(), { - id: 'wf-1', - name: 'Existing', - settings: { availableInMCP: true }, - nodes: [ - makeNode({ id: 'trigger', name: 'Trigger', type: 'n8n-nodes-base.manualTrigger' }), - makeNode({ id: 'a', name: 'A', position: [200, 0] }), - makeNode({ id: 'b', name: 'B', position: [400, 0] }), - makeNode({ id: 'c', name: 'C', position: [600, 0] }), - ], - connections: { - Trigger: { main: [[{ node: 'A', type: 'main', index: 0 }]] }, - A: { main: [[{ node: 'B', type: 'main', index: 0 }]] }, - B: { main: [[{ node: 'C', type: 'main', index: 0 }]] }, - } as IConnections, - nodeGroups: [{ id: 'g1', name: 'Group', nodeIds: ['a', 'b', 'c'] }], - }), + Object.assign(buildExistingWorkflow(), { nodes: looseNodes(9), connections: {} }), ); const result = await callHandler( - { workflowId: 'wf-1', operations: [{ type: 'removeNode', nodeName: 'B' }] }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodeGroups ?? []).toEqual([]); - - const response = parseResult(result); - // The node is gone: the removeNode applied in full. Pruning the group was - // a side effect, so the group's loss is not a skipped operation. - expect(response.skippedOperations ?? []).toEqual([]); - expect(response.appliedOperations).toBe(1); - expect(response.removedGroups).toEqual([ - { - groupName: 'Group', - reason: expect.stringContaining('single connected subgraph') as string, - }, - ]); - }); - - test('adding a node that branches out of an existing group removes the group and reports it as collateral', async () => { - // Trigger -> A -> B, group {A, B}. Branching a new node off A gives the - // group two outgoing boundary connections, breaking single-entry/exit. - findWorkflowMock.mockResolvedValue( - Object.assign(buildWorkflowWithTrigger(), { - nodeGroups: [{ id: 'g1', name: 'Chain', nodeIds: ['a', 'b'] }], - }), - ); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [ - { type: 'addNode', node: { name: 'C', type: 'n8n-nodes-base.set', typeVersion: 1 } }, - { type: 'addConnection', source: 'A', target: 'C' }, - ], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - // Both requested operations landed... - expect(saved.nodes.map((n) => n.name)).toContain('C'); - expect(saved.connections.A?.main?.[0]).toEqual( - expect.arrayContaining([expect.objectContaining({ node: 'C' })]), - ); - // ...and only the group was lost. - expect(saved.nodeGroups).toEqual([]); - - const response = parseResult(result); - expect(response.appliedOperations).toBe(2); - expect(response.skippedOperations ?? []).toEqual([]); - expect(response.removedGroups).toEqual([ - { - groupName: 'Chain', - reason: expect.stringContaining('single connected subgraph') as string, - }, - ]); - }); - - test('a group already invalid before this batch is removed and reported, whatever the operations were', async () => { - // Legacy data: basic-only validation (e.g. a git import) lets a group - // with a trigger through, so any later update has to clean it up — - // the same removal the canvas performs on load. - findWorkflowMock.mockResolvedValue( - Object.assign(buildWorkflowWithTrigger(), { - nodeGroups: [{ id: 'g1', name: 'Legacy', nodeIds: ['trigger', 'a'] }], - }), - ); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [{ type: 'setNodePosition', nodeName: 'B', position: [50, 50] }], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodeGroups).toEqual([]); - - const response = parseResult(result); - expect(response.appliedOperations).toBe(1); - expect(response.skippedOperations ?? []).toEqual([]); - expect(response.removedGroups).toEqual([ - { - groupName: 'Legacy', - reason: expect.stringContaining('cannot contain trigger nodes') as string, - }, - ]); - }); - - test('every skippedOperations entry carries the index of the operation it belongs to', async () => { - findWorkflowMock.mockResolvedValue(buildWorkflowWithTrigger()); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [ - { type: 'setNodePosition', nodeName: 'A', position: [10, 10] }, - // Fails the basic checks: unknown member. - { type: 'addNodeGroup', name: 'Missing', nodeNames: ['Nope'] }, - // Passes them, then fails the structural check. - { type: 'addNodeGroup', name: 'WithTrigger', nodeNames: ['Trigger', 'A'] }, - ], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const response = parseResult(result); - expect(response.skippedOperations).toEqual([ - expect.objectContaining({ opIndex: 1, type: 'addNodeGroup' }), - expect.objectContaining({ opIndex: 2, type: 'addNodeGroup' }), - ]); - expect(response.appliedOperations).toBe(1); - }); - - test('all groups valid: no skipped operations are reported', async () => { - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [{ type: 'addNodeGroup', name: 'Group', nodeNames: ['A', 'B'] }], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect(saved.nodeGroups).toEqual([ - { id: expect.any(String) as string, name: 'Group', nodeIds: ['a', 'b'] }, - ]); - - const response = parseResult(result); - expect(response.skippedOperations ?? []).toEqual([]); - }); - - test('no group operation in the batch: structural validation does not run', async () => { - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [{ type: 'setNodePosition', nodeName: 'A', position: [50, 50] }], - }, - createOnTool(), - ); - - expect(result.isError).toBeUndefined(); - - const saved = updateMock.mock.calls[0][1] as WorkflowEntity; - expect('nodeGroups' in saved).toBe(false); - }); - - test('the response reports skippedOperations with a human-readable reason', async () => { - findWorkflowMock.mockResolvedValue(buildWorkflowWithTrigger()); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [ - { - type: 'setNodeGroups', - nodeGroups: [{ id: 'g1', name: 'Group', nodeNames: ['Trigger', 'A'] }], - }, - ], - }, - createOnTool(), + { workflowId: 'wf-1', operations: addNodeOps(9, 11) }, + createTool(), ); const response = parseResult(result); - expect(response.skippedOperations).toEqual( + expect(response.validationWarnings).toEqual( expect.arrayContaining([ - expect.objectContaining({ - reason: expect.stringContaining('cannot contain trigger nodes') as string, - }), + expect.objectContaining({ code: 'TOP_LEVEL_ITEMS_OVER_CEILING' }), ]), ); - }); - - describe('top-level ceiling warning', () => { - const looseNodes = (count: number) => - Array.from({ length: count }, (_, i) => - makeNode({ id: `n${i}`, name: `Step ${i}`, position: [i * 200, 0] }), - ); - const addNodeOps = (from: number, to: number) => - Array.from({ length: to - from }, (_, i) => ({ - type: 'addNode', - node: makeNode({ id: `n${from + i}`, name: `Step ${from + i}` }), - })); - - test('an update that pushes the canvas over the ceiling gets a warning', async () => { - findWorkflowMock.mockResolvedValue( - Object.assign(buildExistingWorkflow(), { nodes: looseNodes(6), connections: {} }), - ); - - const result = await callHandler( - { workflowId: 'wf-1', operations: addNodeOps(6, 8) }, - createOnTool(), - ); - - const response = parseResult(result); - expect(response.validationWarnings).toEqual( - expect.arrayContaining([ - expect.objectContaining({ code: 'TOP_LEVEL_ITEMS_OVER_CEILING' }), - ]), - ); - expect(response.validationWarnings).not.toEqual( - expect.arrayContaining([expect.objectContaining({ preExisting: true })]), - ); - }); - - test('a canvas already over the ceiling, with no box added, gets the warning marked pre-existing', async () => { - findWorkflowMock.mockResolvedValue( - Object.assign(buildExistingWorkflow(), { nodes: looseNodes(9), connections: {} }), - ); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [ - { - type: 'updateNodeParameters', - nodeName: 'Step 1', - parameters: { url: 'https://new' }, - }, - ], - }, - createOnTool(), - ); - - const response = parseResult(result); - expect(response.validationWarnings).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - code: 'TOP_LEVEL_ITEMS_OVER_CEILING', - preExisting: true, - message: expect.stringContaining('[pre-existing]') as string, - }), - ]), - ); - }); - - test('swapping one loose node for a new one keeps the box count but is not pre-existing', async () => { - findWorkflowMock.mockResolvedValue( - Object.assign(buildExistingWorkflow(), { nodes: looseNodes(9), connections: {} }), - ); - - const result = await callHandler( - { - workflowId: 'wf-1', - operations: [{ type: 'removeNode', nodeName: 'Step 8' }, ...addNodeOps(9, 10)], - }, - createOnTool(), - ); - - const response = parseResult(result); - expect(response.validationWarnings).toEqual( - expect.arrayContaining([ - expect.objectContaining({ code: 'TOP_LEVEL_ITEMS_OVER_CEILING' }), - ]), - ); - expect(response.validationWarnings).not.toEqual( - expect.arrayContaining([expect.objectContaining({ preExisting: true })]), - ); - }); - - test('a canvas already over the ceiling that this update adds loose nodes to is not marked pre-existing', async () => { - findWorkflowMock.mockResolvedValue( - Object.assign(buildExistingWorkflow(), { nodes: looseNodes(9), connections: {} }), - ); - - const result = await callHandler( - { workflowId: 'wf-1', operations: addNodeOps(9, 11) }, - createOnTool(), - ); - - const response = parseResult(result); - expect(response.validationWarnings).toEqual( - expect.arrayContaining([ - expect.objectContaining({ code: 'TOP_LEVEL_ITEMS_OVER_CEILING' }), - ]), - ); - expect(response.validationWarnings).not.toEqual( - expect.arrayContaining([expect.objectContaining({ preExisting: true })]), - ); - }); + expect(response.validationWarnings).not.toEqual( + expect.arrayContaining([expect.objectContaining({ preExisting: true })]), + ); }); }); }); @@ -1434,7 +1352,7 @@ describe('update-workflow MCP tool', () => { { type: 'addNodeGroup', name: 'Group', nodeNames: ['Missing'] }, ], }, - createTool({ canvasGroupsEnabled: true }), + createTool(), ); expect(result.isError).toBeUndefined(); @@ -1464,7 +1382,7 @@ describe('update-workflow MCP tool', () => { workflowId: 'wf-1', operations: [{ type: 'removeConnection', source: 'A', target: 'B' }], }, - createTool({ canvasGroupsEnabled: true }), + createTool(), ); expect(result.isError).toBeUndefined(); @@ -1492,7 +1410,7 @@ describe('update-workflow MCP tool', () => { workflowId: 'wf-1', operations: [{ type: 'addNodeGroup', name: 'Group', nodeNames: ['A', 'B'] }], }, - createTool({ canvasGroupsEnabled: true }), + createTool(), ); expect(result.isError).toBeUndefined(); @@ -1789,7 +1707,7 @@ describe('update-workflow MCP tool', () => { workflowId: 'wf-1', operations: [{ type: 'addNodeGroup', id: 'g1', name: 'Group', nodeNames: ['A', 'B'] }], }, - createTool({ canvasGroupsEnabled: true }), + createTool(), ); const response = parseResult(result); @@ -2571,7 +2489,6 @@ describe('update-workflow MCP tool', () => { subworkflowPolicyChecker, workflowPublishedDataService, aiGatewayService, - {}, logger, postSaveMetrics, ); @@ -3933,7 +3850,6 @@ describe('update-workflow MCP tool', () => { subworkflowPolicyChecker, workflowPublishedDataService, aiGatewayService, - {}, logger, postSaveMetrics, ); @@ -3973,7 +3889,6 @@ describe('update-workflow MCP tool', () => { subworkflowPolicyChecker, workflowPublishedDataService, aiGatewayService, - {}, logger, postSaveMetrics, ); @@ -4013,7 +3928,6 @@ describe('update-workflow MCP tool', () => { subworkflowPolicyChecker, workflowPublishedDataService, aiGatewayService, - {}, logger, postSaveMetrics, ); diff --git a/packages/cli/src/modules/mcp/__tests__/validate-workflow-code.tool.test.ts b/packages/cli/src/modules/mcp/__tests__/validate-workflow-code.tool.test.ts index 531d1c86920..8e57e8b5e06 100644 --- a/packages/cli/src/modules/mcp/__tests__/validate-workflow-code.tool.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/validate-workflow-code.tool.test.ts @@ -5,10 +5,7 @@ import { NodeConnectionTypes } from 'n8n-workflow'; import { NodeTypes } from '@/node-types'; import { Telemetry } from '@/telemetry'; -import { - createValidateWorkflowCodeTool, - type ValidateWorkflowCodeToolOptions, -} from '../tools/workflow-builder/validate-workflow-code.tool'; +import { createValidateWorkflowCodeTool } from '../tools/workflow-builder/validate-workflow-code.tool'; // Mocks referenced inside vi.mock factories must come from vi.hoisted. const { mockParseAndValidate, mockStripImportStatements } = vi.hoisted(() => ({ @@ -64,8 +61,7 @@ describe('validate-workflow-code MCP tool', () => { }) as typeof nodeTypes.getByNameAndVersion); }); - const createTool = (options?: ValidateWorkflowCodeToolOptions) => - createValidateWorkflowCodeTool(user, telemetry, nodeTypes, options); + const createTool = () => createValidateWorkflowCodeTool(user, telemetry, nodeTypes); describe('smoke tests', () => { test('creates tool with correct name and readOnlyHint=true', () => { @@ -274,7 +270,7 @@ describe('validate-workflow-code MCP tool', () => { }); }); - describe('canvas groups (102_mcp_canvas_groups)', () => { + describe('canvas groups', () => { const makeGroupedWorkflow = ( nodeGroups: Array<{ id: string; name: string; nodeIds: string[] }>, ) => ({ @@ -333,29 +329,13 @@ describe('validate-workflow-code MCP tool', () => { }) as typeof nodeTypes.getByNameAndVersion); }); - test('flag off: groups are not validated and output/telemetry are unchanged', async () => { - mockParseAndValidate.mockResolvedValue({ - workflow: makeGroupedWorkflow([{ id: 'g1', name: 'Group', nodeIds: ['trigger', 'a'] }]), - warnings: [], - }); - - const tool = createTool(); - const result = await tool.handler({ code: 'const wf = ...' }, {} as never); - - const response = parseResult(result); - expect(response.valid).toBe(true); - expect(response).not.toHaveProperty('warnings'); - // Telemetry payload is byte-identical to the pre-flag shape. - expect(trackedData()).toEqual({ nodeCount: 3, warningCount: 0 }); - }); - - test('flag on: a valid group produces no errors and is counted in telemetry', async () => { + test('a valid group produces no errors and is counted in telemetry', async () => { mockParseAndValidate.mockResolvedValue({ workflow: makeGroupedWorkflow([{ id: 'g1', name: 'Group', nodeIds: ['a', 'b'] }]), warnings: [], }); - const tool = createTool({ canvasGroupsEnabled: true }); + const tool = createTool(); const result = await tool.handler({ code: 'const wf = ...' }, {} as never); const response = parseResult(result); @@ -368,13 +348,13 @@ describe('validate-workflow-code MCP tool', () => { }); }); - test('flag on: group violations fail validation with the save-path message', async () => { + test('group violations fail validation with the save-path message', async () => { mockParseAndValidate.mockResolvedValue({ workflow: makeGroupedWorkflow([{ id: 'g1', name: 'Group', nodeIds: ['trigger', 'a'] }]), warnings: [], }); - const tool = createTool({ canvasGroupsEnabled: true }); + const tool = createTool(); const result = await tool.handler({ code: 'const wf = ...' }, {} as never); const response = parseResult(result); @@ -394,7 +374,7 @@ describe('validate-workflow-code MCP tool', () => { }); }); - test('flag on: all group violations are reported as errors, one entry each', async () => { + test('all group violations are reported as errors, one entry each', async () => { const sdkWarning = { code: 'deprecated', message: 'Node X is deprecated' }; mockParseAndValidate.mockResolvedValue({ workflow: makeGroupedWorkflow([ @@ -404,7 +384,7 @@ describe('validate-workflow-code MCP tool', () => { warnings: [sdkWarning], }); - const tool = createTool({ canvasGroupsEnabled: true }); + const tool = createTool(); const result = await tool.handler({ code: 'const wf = ...' }, {} as never); const response = parseResult(result); @@ -429,7 +409,7 @@ describe('validate-workflow-code MCP tool', () => { }); }); - test('flag on: connections under unsafe object keys are skipped, not assigned', async () => { + test('connections under unsafe object keys are skipped, not assigned', async () => { // Built via JSON.parse: an object literal with a "__proto__" key would // invoke the prototype setter instead of creating an own property. // Both entries would be boundary-crossing ai_languageModel connections @@ -448,7 +428,7 @@ describe('validate-workflow-code MCP tool', () => { warnings: [], }); - const tool = createTool({ canvasGroupsEnabled: true }); + const tool = createTool(); const result = await tool.handler({ code: 'const wf = ...' }, {} as never); const response = parseResult(result); @@ -462,13 +442,13 @@ describe('validate-workflow-code MCP tool', () => { }); }); - test('flag on: workflows without groups report groupCount 0', async () => { + test('workflows without groups report groupCount 0', async () => { mockParseAndValidate.mockResolvedValue({ workflow: makeGroupedWorkflow([]), warnings: [], }); - const tool = createTool({ canvasGroupsEnabled: true }); + const tool = createTool(); const result = await tool.handler({ code: 'const wf = ...' }, {} as never); const response = parseResult(result); diff --git a/packages/cli/src/modules/mcp/__tests__/workflow-operations.test.ts b/packages/cli/src/modules/mcp/__tests__/workflow-operations.test.ts index c4f0149e698..b5e950b0cbb 100644 --- a/packages/cli/src/modules/mcp/__tests__/workflow-operations.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/workflow-operations.test.ts @@ -1112,13 +1112,17 @@ describe('applyOperations', () => { expect(result.workflow.nodeGroups).toEqual([]); }); - test('fails when a node name does not exist', () => { + test('is skipped when a node name does not exist', () => { const result = applyOperations(baseWorkflow(), [ { type: 'setNodeGroups', nodeGroups: [{ name: 'Group', nodeNames: ['A', 'Missing'] }] }, ]); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.error).toContain("node 'Missing' in group 'Group' not found"); + expect(result.success).toBe(true); + if (!result.success) { + return; + } + expect(result.skippedOperations[0]?.reason).toContain( + "node 'Missing' in group 'Group' not found", + ); }); test('resolves nodes added earlier in the same operation batch', () => { @@ -1236,7 +1240,7 @@ describe('applyOperations', () => { ]); }); - test('fails when a group with the same name already exists', () => { + test('is skipped when a group with the same name already exists', () => { const wf = { ...baseWorkflow(), nodeGroups: [{ id: 'g1', name: 'Group', nodeIds: ['a'] }], @@ -1244,12 +1248,16 @@ describe('applyOperations', () => { const result = applyOperations(wf, [ { type: 'addNodeGroup', name: 'Group', nodeNames: ['B'] }, ]); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.error).toContain("a node group named 'Group' already exists"); + expect(result.success).toBe(true); + if (!result.success) { + return; + } + expect(result.skippedOperations[0]?.reason).toContain( + "a node group named 'Group' already exists", + ); }); - test('fails when a group with the same id already exists', () => { + test('is skipped when a group with the same id already exists', () => { const wf = { ...baseWorkflow(), nodeGroups: [{ id: 'g1', name: 'Existing', nodeIds: ['a'] }], @@ -1257,18 +1265,26 @@ describe('applyOperations', () => { const result = applyOperations(wf, [ { type: 'addNodeGroup', id: 'g1', name: 'New', nodeNames: ['B'] }, ]); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.error).toContain("a node group with id 'g1' already exists"); + expect(result.success).toBe(true); + if (!result.success) { + return; + } + expect(result.skippedOperations[0]?.reason).toContain( + "a node group with id 'g1' already exists", + ); }); - test('fails when a node name does not exist', () => { + test('is skipped when a node name does not exist', () => { const result = applyOperations(baseWorkflow(), [ { type: 'addNodeGroup', name: 'Group', nodeNames: ['Missing'] }, ]); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.error).toContain("node 'Missing' in group 'Group' not found"); + expect(result.success).toBe(true); + if (!result.success) { + return; + } + expect(result.skippedOperations[0]?.reason).toContain( + "node 'Missing' in group 'Group' not found", + ); }); test('dedupes duplicate node names', () => { @@ -1339,13 +1355,15 @@ describe('applyOperations', () => { expect(result.workflow.nodes).toHaveLength(2); }); - test('fails when the group does not exist', () => { + test('is skipped when the group does not exist', () => { const result = applyOperations(baseWorkflow(), [ { type: 'removeNodeGroup', groupName: 'Missing' }, ]); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.error).toContain("node group 'Missing' not found"); + expect(result.success).toBe(true); + if (!result.success) { + return; + } + expect(result.skippedOperations[0]?.reason).toContain("node group 'Missing' not found"); }); test('can remove a group added earlier in the same batch', () => { @@ -1385,13 +1403,17 @@ describe('applyOperations', () => { expect(result.success).toBe(true); }); - test('fails when the new name collides with another group', () => { + test('is skipped when the new name collides with another group', () => { const result = applyOperations(groupedWorkflow(), [ { type: 'updateNodeGroup', groupName: 'Group', newName: 'Other' }, ]); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.error).toContain("a node group named 'Other' already exists"); + expect(result.success).toBe(true); + if (!result.success) { + return; + } + expect(result.skippedOperations[0]?.reason).toContain( + "a node group named 'Other' already exists", + ); }); test('replaces membership resolving and deduping node names', () => { @@ -1403,13 +1425,17 @@ describe('applyOperations', () => { expect(result.workflow.nodeGroups![1].nodeIds).toEqual(['a']); }); - test('fails when a member node name does not exist', () => { + test('is skipped when a member node name does not exist', () => { const result = applyOperations(groupedWorkflow(), [ { type: 'updateNodeGroup', groupName: 'Group', nodeNames: ['Missing'] }, ]); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.error).toContain("node 'Missing' in group 'Group' not found"); + expect(result.success).toBe(true); + if (!result.success) { + return; + } + expect(result.skippedOperations[0]?.reason).toContain( + "node 'Missing' in group 'Group' not found", + ); }); test('sets a new description', () => { @@ -1439,22 +1465,26 @@ describe('applyOperations', () => { expect(result.workflow.nodeGroups![0].description).toBe('Old description'); }); - test('fails when the group does not exist', () => { + test('is skipped when the group does not exist', () => { const result = applyOperations(baseWorkflow(), [ { type: 'updateNodeGroup', groupName: 'Missing', newName: 'X' }, ]); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.error).toContain("node group 'Missing' not found"); + expect(result.success).toBe(true); + if (!result.success) { + return; + } + expect(result.skippedOperations[0]?.reason).toContain("node group 'Missing' not found"); }); - test('fails when no change is specified', () => { + test('is skipped when no change is specified', () => { const result = applyOperations(groupedWorkflow(), [ { type: 'updateNodeGroup', groupName: 'Group' }, ]); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.error).toContain( + expect(result.success).toBe(true); + if (!result.success) { + return; + } + expect(result.skippedOperations[0]?.reason).toContain( 'updateNodeGroup must specify at least one of newName, nodeNames, or description', ); }); @@ -1469,11 +1499,9 @@ describe('applyOperations', () => { // Valid nodeNames combined with a colliding newName: since a failing group // op is skipped rather than fatal, applying the membership before checking // the name would persist a change the skip report says never happened. - const result = applyOperations( - groupedWorkflow(), - [{ type: 'updateNodeGroup', groupName: 'Group', nodeNames: ['A', 'B'], newName: 'Other' }], - { canvasGroupsEnabled: true }, - ); + const result = applyOperations(groupedWorkflow(), [ + { type: 'updateNodeGroup', groupName: 'Group', nodeNames: ['A', 'B'], newName: 'Other' }, + ]); expect(result.success).toBe(true); if (!result.success) return; expect(result.skippedOperations).toEqual([ @@ -1595,142 +1623,121 @@ describe('applyOperations', () => { }); describe('non-fatal operation types', () => { - describe('canvasGroupsEnabled off', () => { - test('a normal batch with no failures succeeds with no skipped operations', () => { - const result = applyOperations(baseWorkflow(), [ - { type: 'updateNodeParameters', nodeName: 'B', parameters: { url: 'https://new' } }, - ]); - expect(result.success).toBe(true); - if (!result.success) return; - expect(result.skippedOperations ?? []).toEqual([]); - }); - - test('a failing group operation still aborts the whole batch', () => { - const result = applyOperations(baseWorkflow(), [ - { type: 'addNodeGroup', name: 'Group', nodeNames: ['Missing'] }, - ]); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.error).toContain("node 'Missing' in group 'Group' not found"); - expect(result.opIndex).toBe(0); - }); - - test('a failing non-group operation still aborts the whole batch', () => { - const result = applyOperations(baseWorkflow(), [ - { type: 'updateNodeParameters', nodeName: 'B', parameters: { url: 'https://new' } }, - { type: 'removeNode', nodeName: 'Missing' }, - ]); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.opIndex).toBe(1); - }); + test('a normal batch with no failures succeeds with no skipped operations', () => { + const result = applyOperations(baseWorkflow(), [ + { type: 'updateNodeParameters', nodeName: 'B', parameters: { url: 'https://new' } }, + ]); + expect(result.success).toBe(true); + if (!result.success) { + return; + } + expect(result.skippedOperations ?? []).toEqual([]); }); - describe('canvasGroupsEnabled on', () => { - test('a failing addNodeGroup is skipped while surrounding operations still apply', () => { - const wf = baseWorkflow(); - const ops: PartialUpdateOperation[] = [ - { type: 'addNode', node: { name: 'C', type: 'n8n-nodes-base.set', typeVersion: 1 } }, - { type: 'addNodeGroup', name: 'Group', nodeNames: ['Missing'] }, - { type: 'addConnection', source: 'A', target: 'C' }, - ]; - const result = applyOperations(wf, ops, { canvasGroupsEnabled: true }); - expect(result.success).toBe(true); - if (!result.success) return; - expect(result.workflow.nodes.some((n) => n.name === 'C')).toBe(true); - expect(result.workflow.connections.A?.main?.[0]).toEqual( - expect.arrayContaining([expect.objectContaining({ node: 'C' })]), - ); - expect(result.workflow.nodeGroups ?? []).toEqual([]); - expect(result.skippedOperations).toEqual([ - { - opIndex: 1, - type: 'addNodeGroup', - reason: "node 'Missing' in group 'Group' not found", - }, - ]); - }); + test('a failing non-group operation still aborts the whole batch', () => { + const result = applyOperations(baseWorkflow(), [ + { type: 'updateNodeParameters', nodeName: 'B', parameters: { url: 'https://new' } }, + { type: 'removeNode', nodeName: 'Missing' }, + ]); + expect(result.success).toBe(false); + if (result.success) { + return; + } + expect(result.opIndex).toBe(1); + }); - test('a failing non-group operation still aborts the whole batch even with the flag on', () => { - const result = applyOperations( - baseWorkflow(), - [{ type: 'removeNode', nodeName: 'Missing' }], - { canvasGroupsEnabled: true }, - ); - expect(result.success).toBe(false); - if (result.success) return; - expect(result.opIndex).toBe(0); - }); + test('a failing addNodeGroup is skipped while surrounding operations still apply', () => { + const wf = baseWorkflow(); + const ops: PartialUpdateOperation[] = [ + { type: 'addNode', node: { name: 'C', type: 'n8n-nodes-base.set', typeVersion: 1 } }, + { type: 'addNodeGroup', name: 'Group', nodeNames: ['Missing'] }, + { type: 'addConnection', source: 'A', target: 'C' }, + ]; + const result = applyOperations(wf, ops); + expect(result.success).toBe(true); + if (!result.success) return; + expect(result.workflow.nodes.some((n) => n.name === 'C')).toBe(true); + expect(result.workflow.connections.A?.main?.[0]).toEqual( + expect.arrayContaining([expect.objectContaining({ node: 'C' })]), + ); + expect(result.workflow.nodeGroups ?? []).toEqual([]); + expect(result.skippedOperations).toEqual([ + { + opIndex: 1, + type: 'addNodeGroup', + reason: "node 'Missing' in group 'Group' not found", + }, + ]); + }); - test('two non-fatal failures in the same batch are both skipped, in order', () => { - const wf = { - ...baseWorkflow(), - nodeGroups: [{ id: 'g1', name: 'Group', nodeIds: ['a'] }], - }; - const ops: PartialUpdateOperation[] = [ - { type: 'addNodeGroup', name: 'Group', nodeNames: ['B'] }, - { type: 'updateNodeGroup', groupName: 'Missing', newName: 'X' }, - ]; - const result = applyOperations(wf, ops, { canvasGroupsEnabled: true }); - expect(result.success).toBe(true); - if (!result.success) return; - expect(result.workflow.nodeGroups).toEqual([{ id: 'g1', name: 'Group', nodeIds: ['a'] }]); - expect(result.skippedOperations).toEqual([ - { - opIndex: 0, - type: 'addNodeGroup', - reason: "a node group named 'Group' already exists", - }, - { - opIndex: 1, - type: 'updateNodeGroup', - reason: "node group 'Missing' not found", - }, - ]); - }); + test('two non-fatal failures in the same batch are both skipped, in order', () => { + const wf = { + ...baseWorkflow(), + nodeGroups: [{ id: 'g1', name: 'Group', nodeIds: ['a'] }], + }; + const ops: PartialUpdateOperation[] = [ + { type: 'addNodeGroup', name: 'Group', nodeNames: ['B'] }, + { type: 'updateNodeGroup', groupName: 'Missing', newName: 'X' }, + ]; + const result = applyOperations(wf, ops); + expect(result.success).toBe(true); + if (!result.success) return; + expect(result.workflow.nodeGroups).toEqual([{ id: 'g1', name: 'Group', nodeIds: ['a'] }]); + expect(result.skippedOperations).toEqual([ + { + opIndex: 0, + type: 'addNodeGroup', + reason: "a node group named 'Group' already exists", + }, + { + opIndex: 1, + type: 'updateNodeGroup', + reason: "node group 'Missing' not found", + }, + ]); + }); - test('a batch where every operation is a failing group op still succeeds with no groups persisted', () => { - const ops: PartialUpdateOperation[] = [ - { type: 'addNodeGroup', name: 'Group', nodeNames: ['Missing'] }, - { type: 'removeNodeGroup', groupName: 'Missing' }, - ]; - const result = applyOperations(baseWorkflow(), ops, { canvasGroupsEnabled: true }); - expect(result.success).toBe(true); - if (!result.success) return; - expect(result.workflow.nodeGroups ?? []).toEqual([]); - expect(result.nodeGroupsChanged).toBe(false); - expect(result.skippedOperations).toHaveLength(2); - }); + test('a batch where every operation is a failing group op still succeeds with no groups persisted', () => { + const ops: PartialUpdateOperation[] = [ + { type: 'addNodeGroup', name: 'Group', nodeNames: ['Missing'] }, + { type: 'removeNodeGroup', groupName: 'Missing' }, + ]; + const result = applyOperations(baseWorkflow(), ops); + expect(result.success).toBe(true); + if (!result.success) return; + expect(result.workflow.nodeGroups ?? []).toEqual([]); + expect(result.nodeGroupsChanged).toBe(false); + expect(result.skippedOperations).toHaveLength(2); + }); - test('skipping a non-fatal operation does not corrupt state for later operations', () => { - const ops: PartialUpdateOperation[] = [ - { type: 'addNode', node: { name: 'C', type: 'n8n-nodes-base.set', typeVersion: 1 } }, - { type: 'addNodeGroup', name: 'Group', nodeNames: ['Missing'] }, - { type: 'setNodeParameter', nodeName: 'C', path: '/foo', value: 'bar' }, - ]; - const result = applyOperations(baseWorkflow(), ops, { canvasGroupsEnabled: true }); - expect(result.success).toBe(true); - if (!result.success) return; - expect(result.workflow.nodes.find((n) => n.name === 'C')!.parameters).toEqual({ - foo: 'bar', - }); - expect(result.skippedOperations).toHaveLength(1); + test('skipping a non-fatal operation does not corrupt state for later operations', () => { + const ops: PartialUpdateOperation[] = [ + { type: 'addNode', node: { name: 'C', type: 'n8n-nodes-base.set', typeVersion: 1 } }, + { type: 'addNodeGroup', name: 'Group', nodeNames: ['Missing'] }, + { type: 'setNodeParameter', nodeName: 'C', path: '/foo', value: 'bar' }, + ]; + const result = applyOperations(baseWorkflow(), ops); + expect(result.success).toBe(true); + if (!result.success) return; + expect(result.workflow.nodes.find((n) => n.name === 'C')!.parameters).toEqual({ + foo: 'bar', }); + expect(result.skippedOperations).toHaveLength(1); + }); - test('opIndex in skippedOperations reflects the original position in the input array', () => { - const ops: PartialUpdateOperation[] = [ - { type: 'updateNodeParameters', nodeName: 'B', parameters: { url: 'https://new' } }, - { type: 'addNode', node: { name: 'C', type: 'n8n-nodes-base.set', typeVersion: 1 } }, - { type: 'addNodeGroup', name: 'Group', nodeNames: ['Missing'] }, - { type: 'setNodeParameter', nodeName: 'C', path: '/foo', value: 'bar' }, - ]; - const result = applyOperations(baseWorkflow(), ops, { canvasGroupsEnabled: true }); - expect(result.success).toBe(true); - if (!result.success) return; - expect(result.skippedOperations).toEqual([ - { opIndex: 2, type: 'addNodeGroup', reason: expect.any(String) as string }, - ]); - }); + test('opIndex in skippedOperations reflects the original position in the input array', () => { + const ops: PartialUpdateOperation[] = [ + { type: 'updateNodeParameters', nodeName: 'B', parameters: { url: 'https://new' } }, + { type: 'addNode', node: { name: 'C', type: 'n8n-nodes-base.set', typeVersion: 1 } }, + { type: 'addNodeGroup', name: 'Group', nodeNames: ['Missing'] }, + { type: 'setNodeParameter', nodeName: 'C', path: '/foo', value: 'bar' }, + ]; + const result = applyOperations(baseWorkflow(), ops); + expect(result.success).toBe(true); + if (!result.success) return; + expect(result.skippedOperations).toEqual([ + { opIndex: 2, type: 'addNodeGroup', reason: expect.any(String) as string }, + ]); }); }); diff --git a/packages/cli/src/modules/mcp/mcp.controller.ts b/packages/cli/src/modules/mcp/mcp.controller.ts index c8610819b69..04f17fdceff 100644 --- a/packages/cli/src/modules/mcp/mcp.controller.ts +++ b/packages/cli/src/modules/mcp/mcp.controller.ts @@ -145,7 +145,6 @@ export class McpController { auth_type: (req as McpAuthenticatedRequest).mcpCaller?.authType, mcp_apps_enabled: featureFlags.mcpApps.enabled, mcp_apps_variant: featureFlags.mcpApps.variant, - mcp_canvas_groups_enabled: featureFlags.canvasGroupsEnabled, }; // In stateless mode, create a new instance of transport and server for each request diff --git a/packages/cli/src/modules/mcp/mcp.service.ts b/packages/cli/src/modules/mcp/mcp.service.ts index 56b58fe7416..cfbc93e84e3 100644 --- a/packages/cli/src/modules/mcp/mcp.service.ts +++ b/packages/cli/src/modules/mcp/mcp.service.ts @@ -3,7 +3,6 @@ import { MCP_APPS_FLAG, MCP_APPS_VARIANT_CONTROL, MCP_APPS_VARIANT_ENABLED, - MCP_CANVAS_GROUPS_FLAG, MCP_INSTANCE_CONTEXT_FLAG, CONTEXT_PREFERENCES_ENABLED_VARIANT, CONTEXT_PREFERENCES_FLAG, @@ -137,8 +136,6 @@ export type McpAppsResolution = { /** Per-user resolution of every PostHog-gated MCP feature. */ export type McpFeatureFlags = { mcpApps: McpAppsResolution; - /** Canvas node-group support in the workflow-builder tools. */ - canvasGroupsEnabled: boolean; /** The instance-context read surface: the activity tools and node-usage. */ instanceContextEnabled: boolean; /** The `get_user_preferences` tool. */ @@ -253,8 +250,7 @@ export class McpService { * PostHog. */ async resolveFeatureFlags(user: User): Promise { - const { mcpAppsEnabled, mcpCanvasGroupsEnabled, mcpInstanceContextEnabled } = - this.globalConfig.endpoints; + const { mcpAppsEnabled, mcpInstanceContextEnabled } = this.globalConfig.endpoints; // `PostHogClient.getFeatureFlags` swallows PostHog errors internally and // returns `{}`, so a transient outage fails closed (feature off, MCP Apps @@ -263,7 +259,6 @@ export class McpService { return { mcpApps: this.resolveMcpApps(mcpAppsEnabled, flags), - canvasGroupsEnabled: mcpCanvasGroupsEnabled || flags[MCP_CANVAS_GROUPS_FLAG] === true, instanceContextEnabled: mcpInstanceContextEnabled || flags[MCP_INSTANCE_CONTEXT_FLAG] === true, // Multivariate flag: only the `variant` arm enables the feature. @@ -454,7 +449,6 @@ export class McpService { instructions: getMcpInstructions({ isBuilderEnabled: builderInstructionsEnabled, isN8nConnectAvailable: n8nConnectAvailable, - canvasGroupsEnabled: featureFlags.canvasGroupsEnabled, isAgentsEnabled: agentInstructionsEnabled, isUserPreferencesEnabled: userPreferencesInstructionsEnabled, }), @@ -749,9 +743,7 @@ export class McpService { ); registerIfAllowed(getNodeTypesTool); - const bestPracticesTool = createGetWorkflowBestPracticesTool(user, this.telemetry, { - canvasGroupsEnabled: featureFlags.canvasGroupsEnabled, - }); + const bestPracticesTool = createGetWorkflowBestPracticesTool(user, this.telemetry); registerIfAllowed(bestPracticesTool); const exploreNodeResourcesTool = createExploreNodeResourcesTool( @@ -761,9 +753,7 @@ export class McpService { ); registerIfAllowed(exploreNodeResourcesTool); - const validateTool = createValidateWorkflowCodeTool(user, this.telemetry, this.nodeTypes, { - canvasGroupsEnabled: featureFlags.canvasGroupsEnabled, - }); + const validateTool = createValidateWorkflowCodeTool(user, this.telemetry, this.nodeTypes); registerIfAllowed(validateTool); const validateNodeTool = createValidateNodeTool(user, this.telemetry); @@ -780,7 +770,6 @@ export class McpService { this.projectRepository, dataTableOps, this.aiGatewayService, - { canvasGroupsEnabled: featureFlags.canvasGroupsEnabled }, this.logger, this.postSaveMetrics, ); @@ -885,7 +874,6 @@ export class McpService { this.subworkflowPolicyChecker, this.workflowPublishedDataService, this.aiGatewayService, - { canvasGroupsEnabled: featureFlags.canvasGroupsEnabled }, this.logger, this.postSaveMetrics, ); @@ -914,9 +902,7 @@ export class McpService { { uri: 'n8n://workflow-sdk/reference', mimeType: 'text/plain', - text: getSdkReferenceContent(undefined, { - includeGroups: featureFlags.canvasGroupsEnabled, - }), + text: getSdkReferenceContent(), }, ], }), @@ -924,9 +910,7 @@ export class McpService { // SDK reference tool — always registered alongside the MCP resource above, // so all clients can access the SDK reference regardless of resource support. - const sdkRefTool = createGetWorkflowSdkReferenceTool(user, this.telemetry, { - canvasGroupsEnabled: featureFlags.canvasGroupsEnabled, - }); + const sdkRefTool = createGetWorkflowSdkReferenceTool(user, this.telemetry); registerIfAllowed(sdkRefTool); } diff --git a/packages/cli/src/modules/mcp/mcp.types.ts b/packages/cli/src/modules/mcp/mcp.types.ts index f154d299be7..0e64fa6dce4 100644 --- a/packages/cli/src/modules/mcp/mcp.types.ts +++ b/packages/cli/src/modules/mcp/mcp.types.ts @@ -181,7 +181,6 @@ export type UserConnectedToMCPEventPayload = { http_status?: number; mcp_apps_enabled?: boolean; mcp_apps_variant?: McpAppsTelemetryVariant; - mcp_canvas_groups_enabled?: boolean; error?: string; }; diff --git a/packages/cli/src/modules/mcp/tools/workflow-builder/create-workflow-from-code.tool.ts b/packages/cli/src/modules/mcp/tools/workflow-builder/create-workflow-from-code.tool.ts index a39b8f70775..0a92ab44a5c 100644 --- a/packages/cli/src/modules/mcp/tools/workflow-builder/create-workflow-from-code.tool.ts +++ b/packages/cli/src/modules/mcp/tools/workflow-builder/create-workflow-from-code.tool.ts @@ -53,20 +53,6 @@ import { const MAX_WORKFLOW_DESCRIPTION_LENGTH = 255; -export type CreateWorkflowFromCodeToolOptions = { - /** - * `102_mcp_canvas_groups` rollout flag: when true, node groups authored in the - * SDK code (`.group(...)`) are persisted on the created workflow. Off by - * default — groups are then dropped at the entity assembly, exactly like - * before groups were supported. With the flag on, an invalid group does not - * fail the creation: it is dropped and reported in `skippedGroups` instead, - * while the rest of the workflow is still created. This tool pre-validates - * with the same rules `WorkflowCreationService.createWorkflow` enforces, so - * that shared service's own (fatal) group check never actually triggers here. - */ - canvasGroupsEnabled?: boolean; -}; - function normalizeWorkflowDescription(description?: string) { if (!description) return { description: undefined, truncated: false }; if (description.length <= MAX_WORKFLOW_DESCRIPTION_LENGTH) { @@ -325,7 +311,6 @@ export const createCreateWorkflowFromCodeTool = ( projectRepository: ProjectRepository, dataTableOps: DataTableUserOperations, aiGatewayService: AiGatewayService, - options: CreateWorkflowFromCodeToolOptions = {}, logger: Logger, postSaveMetrics: McpPostSaveMetricsService, ): ToolDefinition => ({ @@ -424,8 +409,7 @@ export const createCreateWorkflowFromCodeTool = ( ...(workflowDescription ? { description: workflowDescription } : {}), nodes: workflowJson.nodes, connections: workflowJson.connections, - // Flag off: groups keep being dropped here, exactly like before. - ...(options.canvasGroupsEnabled ? { nodeGroups: workflowJson.nodeGroups ?? [] } : {}), + nodeGroups: workflowJson.nodeGroups ?? [], settings: { ...workflowJson.settings, executionOrder: 'v1', availableInMCP: true }, pinData: workflowJson.pinData, meta: { ...workflowJson.meta, aiBuilderAssisted: true, builderVariant: 'mcp' }, @@ -440,11 +424,10 @@ export const createCreateWorkflowFromCodeTool = ( // parser above. Validate them here, before the shared persistence layer's // own (fatal) check, so an invalid group is dropped and reported instead // of aborting the whole creation. - const skippedGroups = options.canvasGroupsEnabled - ? dropInvalidWorkflowGroups(newWorkflow, makeGetNodeTypeForGrouping(nodeTypes)).map( - (violation) => ({ groupName: violation.groupName, reason: violation.message }), - ) - : []; + const skippedGroups = dropInvalidWorkflowGroups( + newWorkflow, + makeGetNodeTypeForGrouping(nodeTypes), + ).map((violation) => ({ groupName: violation.groupName, reason: violation.message })); landingProject = projectId ? await projectRepository.findOneBy({ id: projectId }) @@ -539,10 +522,7 @@ export const createCreateWorkflowFromCodeTool = ( skippedGroups: skippedGroups.length > 0 ? skippedGroups : undefined, }; - // Groups are dropped on save when the flag is off, so only warn when they can be kept. - const ceilingWarning = options.canvasGroupsEnabled - ? topLevelItemsWarning(savedWorkflow) - : undefined; + const ceilingWarning = topLevelItemsWarning(savedWorkflow); const warnings = ceilingWarning ? [...result.warnings, ceilingWarning] : result.warnings; const output = warnings.length > 0 ? { ...baseOutput, warnings } : baseOutput; @@ -566,11 +546,7 @@ export const createCreateWorkflowFromCodeTool = ( data: { workflowId: savedWorkflow.id, nodeCount: savedWorkflow.nodes.length, - // Rollout monitoring for `102_mcp_canvas_groups`; absent when the - // flag is off so the payload stays identical across cohorts. - ...(options.canvasGroupsEnabled - ? { groupCount: workflowJson.nodeGroups?.length ?? 0 } - : {}), + groupCount: workflowJson.nodeGroups?.length ?? 0, }, }; telemetry.track(USER_CALLED_MCP_TOOL_EVENT, telemetryPayload); diff --git a/packages/cli/src/modules/mcp/tools/workflow-builder/get-workflow-best-practices.tool.ts b/packages/cli/src/modules/mcp/tools/workflow-builder/get-workflow-best-practices.tool.ts index 8d3dc06ad13..b67f804f453 100644 --- a/packages/cli/src/modules/mcp/tools/workflow-builder/get-workflow-best-practices.tool.ts +++ b/packages/cli/src/modules/mcp/tools/workflow-builder/get-workflow-best-practices.tool.ts @@ -49,7 +49,7 @@ const outputSchema = { .describe('All available techniques, returned when "list" was requested.'), } satisfies z.ZodRawShape; -function buildListResponse(canvasGroupsEnabled: boolean) { +function buildListResponse() { const availableTechniques = Object.entries(TechniqueDescription).map(([key, description]) => ({ technique: key, description, @@ -66,9 +66,8 @@ function buildListResponse(canvasGroupsEnabled: boolean) { (t) => `- ${t.technique}${t.hasDocumentation ? '' : ' (no detailed documentation yet)'} — ${t.description}`, ), - // Grouping guidance is flag-gated and appended last, so flag-off output is - // unchanged. - ...(canvasGroupsEnabled ? ['', GROUPING_GUIDANCE] : []), + '', + GROUPING_GUIDANCE, ].join('\n'); return { @@ -110,7 +109,6 @@ function buildTechniqueResponse(technique: WorkflowTechniqueType) { export const createGetWorkflowBestPracticesTool = ( user: User, telemetry: Telemetry, - { canvasGroupsEnabled }: { canvasGroupsEnabled: boolean }, ): ToolDefinition => ({ name: MCP_GET_WORKFLOW_BEST_PRACTICES_TOOL.toolName, config: { @@ -135,9 +133,7 @@ export const createGetWorkflowBestPracticesTool = ( try { const response = - technique === LIST_SENTINEL - ? buildListResponse(canvasGroupsEnabled) - : buildTechniqueResponse(technique); + technique === LIST_SENTINEL ? buildListResponse() : buildTechniqueResponse(technique); telemetryPayload.results = { success: true, diff --git a/packages/cli/src/modules/mcp/tools/workflow-builder/get-workflow-sdk-reference.tool.ts b/packages/cli/src/modules/mcp/tools/workflow-builder/get-workflow-sdk-reference.tool.ts index b57753e4f48..5145b37cf7b 100644 --- a/packages/cli/src/modules/mcp/tools/workflow-builder/get-workflow-sdk-reference.tool.ts +++ b/packages/cli/src/modules/mcp/tools/workflow-builder/get-workflow-sdk-reference.tool.ts @@ -8,7 +8,7 @@ import { getSdkReferenceContent, type SdkReferenceSection } from './sdk-referenc import { USER_CALLED_MCP_TOOL_EVENT } from '../../mcp.constants'; import type { ToolDefinition, UserCalledMCPToolEventPayload } from '../../mcp.types'; -const BASE_SECTIONS: SdkReferenceSection[] = [ +const VALID_SECTIONS: SdkReferenceSection[] = [ 'patterns', 'patterns_detailed', 'expressions', @@ -17,28 +17,18 @@ const BASE_SECTIONS: SdkReferenceSection[] = [ 'import', 'guidelines', 'design', + 'groups', 'all', ]; -// `'groups'` is only advertised (and accepted) when the canvas-groups flag is on, -// so with the flag off the accepted `section` values are exactly what they were -// before groups existed. -const buildInputSchema = (canvasGroupsEnabled: boolean) => { - const validSections: SdkReferenceSection[] = canvasGroupsEnabled - ? [...BASE_SECTIONS, 'groups'] - : BASE_SECTIONS; - - return { - section: z - .enum(validSections as [string, ...string[]]) - .optional() - .describe( - 'Optional section to retrieve. Omit this for the full reference, or use a section for targeted lookup.', - ), - } satisfies z.ZodRawShape; -}; - -type SdkReferenceInputSchema = ReturnType; +const inputSchema = { + section: z + .enum(VALID_SECTIONS as [string, ...string[]]) + .optional() + .describe( + 'Optional section to retrieve. Omit this for the full reference, or use a section for targeted lookup.', + ), +} satisfies z.ZodRawShape; const outputSchema = { reference: z.string().describe('SDK reference documentation content for the requested section'), @@ -51,13 +41,12 @@ const outputSchema = { export const createGetWorkflowSdkReferenceTool = ( user: User, telemetry: Telemetry, - { canvasGroupsEnabled }: { canvasGroupsEnabled: boolean }, -): ToolDefinition => ({ +): ToolDefinition => ({ name: MCP_GET_SDK_REFERENCE_TOOL.toolName, config: { description: 'Required reference when building a workflow, and only then. Call this BEFORE writing workflow code to learn workflow(), trigger()/node(), .add()/.to(), expr(), and credential patterns.', - inputSchema: buildInputSchema(canvasGroupsEnabled), + inputSchema, outputSchema, annotations: { title: MCP_GET_SDK_REFERENCE_TOOL.displayTitle, @@ -74,9 +63,7 @@ export const createGetWorkflowSdkReferenceTool = ( parameters: { section }, }; - const content = getSdkReferenceContent(section as SdkReferenceSection | undefined, { - includeGroups: canvasGroupsEnabled, - }); + const content = getSdkReferenceContent(section as SdkReferenceSection | undefined); telemetryPayload.results = { success: true }; telemetry.track(USER_CALLED_MCP_TOOL_EVENT, telemetryPayload); diff --git a/packages/cli/src/modules/mcp/tools/workflow-builder/mcp-instructions.ts b/packages/cli/src/modules/mcp/tools/workflow-builder/mcp-instructions.ts index b8a1ec2836b..bf2df447b6e 100644 --- a/packages/cli/src/modules/mcp/tools/workflow-builder/mcp-instructions.ts +++ b/packages/cli/src/modules/mcp/tools/workflow-builder/mcp-instructions.ts @@ -37,12 +37,6 @@ export type McpInstructionsOptions = { */ isN8nConnectAvailable?: boolean; - /** - * Whether canvas node groups are enabled for this user. - * If true, the builder instructions point the client at the groups docs. - */ - canvasGroupsEnabled?: boolean; - /** * Whether first-class Agent tools are enabled on this MCP server. * If true, the instructions include Agent build guidance and artifact routing. @@ -60,7 +54,6 @@ export function getMcpInstructions(options: McpInstructionsOptions): string { const { isBuilderEnabled, isN8nConnectAvailable = false, - canvasGroupsEnabled = false, isAgentsEnabled = false, isUserPreferencesEnabled = false, } = options; @@ -72,12 +65,9 @@ export function getMcpInstructions(options: McpInstructionsOptions): string { ? `Before ${MCP_USER_PREFERENCES_TRIGGER_CLAUSE} call ${MCP_GET_USER_PREFERENCES_TOOL_NAME} first and apply what it returns for the remainder of the task.` : ''; - // Only appended when the flag is on; keeps the paid-per-session string short. - const GROUPS_HINT = canvasGroupsEnabled - ? ` + const GROUPS_HINT = ` -Node groups: when a workflow has several distinct stages, organise it into named groups so it is readable on the canvas. Before creating groups, call ${MCP_GET_SDK_REFERENCE_TOOL.toolName} with section "groups" for the rules, and ${MCP_GET_WORKFLOW_BEST_PRACTICES_TOOL.toolName} (technique "list") for when to group. The save never fails because of groups, so read its result: when it reports TOP_LEVEL_ITEMS_OVER_CEILING, skippedGroups or removedGroups, repair the groups with ${MCP_UPDATE_WORKFLOW_TOOL.toolName} before you tell the user the workflow is done. A warning marked [pre-existing] describes a canvas that was already like that before your update; you do not need to repair it before you report done.` - : ''; +Node groups: when a workflow has several distinct stages, organise it into named groups so it is readable on the canvas. Before creating groups, call ${MCP_GET_SDK_REFERENCE_TOOL.toolName} with section "groups" for the rules, and ${MCP_GET_WORKFLOW_BEST_PRACTICES_TOOL.toolName} (technique "list") for when to group. The save never fails because of groups, so read its result: when it reports TOP_LEVEL_ITEMS_OVER_CEILING, skippedGroups or removedGroups, repair the groups with ${MCP_UPDATE_WORKFLOW_TOOL.toolName} before you tell the user the workflow is done. A warning marked [pre-existing] describes a canvas that was already like that before your update; you do not need to repair it before you report done.`; const N8N_CONNECT_HINT = isN8nConnectAvailable ? ` @@ -118,7 +108,7 @@ To build n8n workflows${WORKFLOWS_ONLY_CLAUSE}, follow these steps in order: 9. Create: Call ${MCP_CREATE_WORKFLOW_FROM_CODE_TOOL.toolName} with the validated code to save the workflow to n8n. Include a short \`description\` (1-2 sentences, max 255 chars) summarizing what the workflow does — this helps users find and understand their workflows. -10. Update: Call ${MCP_UPDATE_WORKFLOW_TOOL.toolName} with the workflow ID and a list of operations (addNode, removeNode, updateNodeParameters, setNodeParameter, renameNode, addConnection, removeConnection, setNodeCredential, setNodePosition, setNodeDisabled, setNodeSettings, setWorkflowMetadata, setWorkflowSettings). The whole batch is atomic: if any op fails the workflow is unchanged. To modify an existing node's configuration, use updateNodeParameters or setNodeParameter — do NOT use removeNode followed by addNode for the same node, as this disconnects any attached sub-nodes (LLM models, memory, tools) and they will not be re-attached automatically. Use setNodeSettings to change a node's execution behavior (onError, retryOnFail, maxTries, waitBetweenTries, alwaysOutputData, executeOnce); for sub-nodes (LLM model, memory, tools) this is the only way to set onError, because the canvas UI does not expose that setting for them. +10. Update: Call ${MCP_UPDATE_WORKFLOW_TOOL.toolName} with the workflow ID and a list of operations (addNode, removeNode, updateNodeParameters, setNodeParameter, renameNode, addConnection, removeConnection, setNodeCredential, setNodePosition, setNodeDisabled, setNodeSettings, setWorkflowMetadata, setWorkflowSettings, setNodeGroups, addNodeGroup, removeNodeGroup, updateNodeGroup). The whole batch is atomic: if any op fails the workflow is unchanged, except node-group operations, which are skipped and reported in skippedOperations. To modify an existing node's configuration, use updateNodeParameters or setNodeParameter — do NOT use removeNode followed by addNode for the same node, as this disconnects any attached sub-nodes (LLM models, memory, tools) and they will not be re-attached automatically. Use setNodeSettings to change a node's execution behavior (onError, retryOnFail, maxTries, waitBetweenTries, alwaysOutputData, executeOnce); for sub-nodes (LLM model, memory, tools) this is the only way to set onError, because the canvas UI does not expose that setting for them. 11. Archive: Call ${MCP_ARCHIVE_WORKFLOW_TOOL.toolName} with the workflow ID. diff --git a/packages/cli/src/modules/mcp/tools/workflow-builder/sdk-reference-content.ts b/packages/cli/src/modules/mcp/tools/workflow-builder/sdk-reference-content.ts index e42b64a7ee5..6dbaf9fdcfb 100644 --- a/packages/cli/src/modules/mcp/tools/workflow-builder/sdk-reference-content.ts +++ b/packages/cli/src/modules/mcp/tools/workflow-builder/sdk-reference-content.ts @@ -14,7 +14,7 @@ import { ADDITIONAL_FUNCTIONS, WORKFLOW_RULES, NODE_GROUPS_REFERENCE, - buildSdkLanguageReference, + SDK_LANGUAGE_REFERENCE, } from '@n8n/workflow-sdk/prompts/sdk-reference'; // NOTE: CODING_GUIDELINES and DESIGN_GUIDANCE are MCP-only constants defined @@ -90,23 +90,8 @@ const SECTIONS: Record, string> = { /** * Get the full SDK reference content or a filtered section. - * - * Node-group docs are gated behind `includeGroups` (fed from the - * `canvasGroupsEnabled` feature flag): when false, no group content is served - * anywhere in the output. */ -export function getSdkReferenceContent( - section?: SdkReferenceSection, - options: { includeGroups?: boolean } = {}, -): string { - const { includeGroups = false } = options; - - // The groups section only exists when the flag is on; otherwise even an - // explicit request yields nothing. - if (section === 'groups') { - return includeGroups ? SECTIONS.groups : ''; - } - +export function getSdkReferenceContent(section?: SdkReferenceSection): string { if (section && section !== 'all' && section in SECTIONS) { return SECTIONS[section]; } @@ -117,9 +102,9 @@ export function getSdkReferenceContent( SECTIONS.import, '', // Language rules for the restricted SDK subset (what the AST interpreter - // accepts). The flag-gated node-groups docs ride inside it, so this is the - // single place group content enters the full reference. - buildSdkLanguageReference({ includeGroups }), + // accepts). The node-groups docs ride inside it, so this is the single + // place group content enters the full reference. + SDK_LANGUAGE_REFERENCE, '', SECTIONS.patterns, '', diff --git a/packages/cli/src/modules/mcp/tools/workflow-builder/update-workflow.tool.ts b/packages/cli/src/modules/mcp/tools/workflow-builder/update-workflow.tool.ts index 8ea580ba43d..d65257cb78a 100644 --- a/packages/cli/src/modules/mcp/tools/workflow-builder/update-workflow.tool.ts +++ b/packages/cli/src/modules/mcp/tools/workflow-builder/update-workflow.tool.ts @@ -66,7 +66,7 @@ const MAX_OPERATIONS_PER_CALL = 100; // which drops `undefined` properties. `deepCopy` preserves them, breaking recovery. // eslint-disable-next-line n8n-local-rules/no-json-parse-json-stringify const normalize = (value: unknown) => JSON.parse(JSON.stringify(value ?? null)); -const baseOperationTypes = [ +const operationTypes = [ 'updateNodeParameters', 'setNodeParameter', 'addNode', @@ -83,21 +83,14 @@ const baseOperationTypes = [ 'addTags', 'removeTags', 'setNodeGroups', -] as const satisfies ReadonlyArray; -// Granular group ops roll out behind the `102_mcp_canvas_groups` flag; -// `setNodeGroups` predates the flag and stays ungated. -const gatedGroupOperationTypes = [ 'addNodeGroup', 'removeNodeGroup', 'updateNodeGroup', ] as const satisfies ReadonlyArray; -// The `satisfies` on both tuples above is what catches a renamed operation type -// in workflow-operations.ts at compile time, instead of silently leaving the gate -// unmatched or an implemented operation unreachable. The set element type is -// narrowed to match so `.has()` only accepts a real operation type. -const GATED_GROUP_OP_TYPES: ReadonlySet = new Set( - gatedGroupOperationTypes, -); +// The `satisfies` above is what catches a renamed operation type in +// workflow-operations.ts at compile time, instead of silently leaving an +// implemented operation unreachable. +const operationTypeSchema = z.enum(operationTypes); const GRAPH_OPERATION_TYPES: ReadonlySet = new Set([ 'addNode', 'removeNode', @@ -115,10 +108,6 @@ const GRAPH_OPERATION_TYPES: ReadonlySet = new S 'updateNodeGroup', 'setNodeSettings', ]); -const buildOperationTypeSchema = (canvasGroupsEnabled: boolean) => - canvasGroupsEnabled - ? z.enum([...baseOperationTypes, ...gatedGroupOperationTypes]) - : z.enum(baseOperationTypes); // A factory, not a shared instance: reusing one Zod instance across two // properties makes the JSON Schema generator dedupe the second occurrence into // a `$ref` to a `#/properties/...` path, which strict MCP clients cannot @@ -165,102 +154,83 @@ const combinedSettingsInputSchema = z .describe( 'Settings to write. For setNodeSettings use the node-level keys (onError, retryOnFail, maxTries, waitBetweenTries, alwaysOutputData, executeOnce). For setWorkflowSettings use the workflow-level keys (errorWorkflow, timezone, executionOrder, saveExecutionProgress, saveManualExecutions, saveDataErrorExecution, saveDataSuccessExecution, executionTimeout, timeSavedPerExecution, callerPolicy, callerIds). Provide only the keys for the operation you are running.', ); -const buildOperationInputSchema = (canvasGroupsEnabled: boolean) => - z - .object({ - type: buildOperationTypeSchema(canvasGroupsEnabled).describe('Operation type.'), - nodeName: z.string().optional().describe('For node-targeted ops.'), - node: nodeInputSchema.optional().describe('For addNode.'), - parameters: z - .record(z.string(), z.unknown()) - .optional() - .describe('For updateNodeParameters.'), - replace: z.boolean().optional().describe('For updateNodeParameters; default false.'), - path: z.string().min(2).optional().describe('For setNodeParameter; JSON Pointer path.'), - value: z.unknown().optional().describe('For setNodeParameter.'), - oldName: z.string().optional().describe('For renameNode.'), - newName: z - .string() - .optional() - .describe(canvasGroupsEnabled ? 'For renameNode or updateNodeGroup.' : 'For renameNode.'), - source: z.string().optional().describe('For connection ops.'), - target: z.string().optional().describe('For connection ops.'), - sourceIndex: z - .number() - .int() - .nonnegative() - .optional() - .describe( - 'For connection ops; which output of the source node the connection starts from. Default 0, the first output. Use it to wire a branch: on an If node the false branch is index 1, and onError "continueErrorOutput" appends an error output after the regular ones (index 1 on a single-output node such as HTTP Request, index 2 on an If node). This is the only field that selects an output.', - ), - targetIndex: z - .number() - .int() - .nonnegative() - .optional() - .describe( - 'For connection ops; which input of the target node the connection ends at. Default 0.', - ), - connectionType: z.string().optional().describe('For connection ops; default "main".'), - credentialKey: z.string().optional().describe('For setNodeCredential.'), - credentialId: z.string().optional().describe('For setNodeCredential.'), - credentialName: z.string().optional().describe('For setNodeCredential.'), - position: positionInputSchema().optional().describe('For setNodePosition.'), - disabled: z.boolean().optional().describe('For setNodeDisabled.'), - settings: combinedSettingsInputSchema - .optional() - .describe('For setNodeSettings or setWorkflowSettings.'), - name: z - .string() - .max(128) - .optional() - .describe( - canvasGroupsEnabled - ? 'For setWorkflowMetadata (workflow name) or addNodeGroup (group name).' - : 'Only used for setWorkflowMetadata.', - ), - description: z - .string() - .max(255) - .optional() - .describe( - canvasGroupsEnabled - ? 'For setWorkflowMetadata, addNodeGroup, or updateNodeGroup.' - : 'Only used for setWorkflowMetadata.', - ), - names: z.array(z.string()).optional().describe('For addTags / removeTags.'), - nodeGroups: z - .array( - z.object({ - id: z.string().optional(), - name: z.string(), - nodeNames: z.array(z.string()), - description: z.string().optional(), - }), - ) - .optional() - .describe( - 'For setNodeGroups. Replaces all node groups; pass [] to clear. Group members are node names, not ids.', - ), - ...(canvasGroupsEnabled - ? { - groupName: z.string().optional().describe('For removeNodeGroup / updateNodeGroup.'), - nodeNames: z - .array(z.string()) - .optional() - .describe('For addNodeGroup / updateNodeGroup; group member node names.'), - id: z.string().optional().describe('For addNodeGroup; group id, generated if omitted.'), - } - : {}), - }) - // Strict, so a field this schema does not declare fails the call instead of - // being stripped. Stripping made the tool report success for an operation it - // never ran: a guessed output-index field (e.g. sourceOutput) vanished and - // the connection was wired from output 0. - .strict() - .describe('Workflow update operation. Provide fields matching type.'); +const operationInputSchema = z + .object({ + type: operationTypeSchema.describe('Operation type.'), + nodeName: z.string().optional().describe('For node-targeted ops.'), + node: nodeInputSchema.optional().describe('For addNode.'), + parameters: z.record(z.string(), z.unknown()).optional().describe('For updateNodeParameters.'), + replace: z.boolean().optional().describe('For updateNodeParameters; default false.'), + path: z.string().min(2).optional().describe('For setNodeParameter; JSON Pointer path.'), + value: z.unknown().optional().describe('For setNodeParameter.'), + oldName: z.string().optional().describe('For renameNode.'), + newName: z.string().optional().describe('For renameNode or updateNodeGroup.'), + source: z.string().optional().describe('For connection ops.'), + target: z.string().optional().describe('For connection ops.'), + sourceIndex: z + .number() + .int() + .nonnegative() + .optional() + .describe( + 'For connection ops; which output of the source node the connection starts from. Default 0, the first output. Use it to wire a branch: on an If node the false branch is index 1, and onError "continueErrorOutput" appends an error output after the regular ones (index 1 on a single-output node such as HTTP Request, index 2 on an If node). This is the only field that selects an output.', + ), + targetIndex: z + .number() + .int() + .nonnegative() + .optional() + .describe( + 'For connection ops; which input of the target node the connection ends at. Default 0.', + ), + connectionType: z.string().optional().describe('For connection ops; default "main".'), + credentialKey: z.string().optional().describe('For setNodeCredential.'), + credentialId: z.string().optional().describe('For setNodeCredential.'), + credentialName: z.string().optional().describe('For setNodeCredential.'), + position: positionInputSchema().optional().describe('For setNodePosition.'), + disabled: z.boolean().optional().describe('For setNodeDisabled.'), + settings: combinedSettingsInputSchema + .optional() + .describe('For setNodeSettings or setWorkflowSettings.'), + name: z + .string() + .max(128) + .optional() + .describe('For setWorkflowMetadata (workflow name) or addNodeGroup (group name).'), + description: z + .string() + .max(255) + .optional() + .describe('For setWorkflowMetadata, addNodeGroup, or updateNodeGroup.'), + names: z.array(z.string()).optional().describe('For addTags / removeTags.'), + nodeGroups: z + .array( + z.object({ + id: z.string().optional(), + name: z.string(), + nodeNames: z.array(z.string()), + description: z.string().optional(), + }), + ) + .optional() + .describe( + 'For setNodeGroups. Replaces all node groups; pass [] to clear. Group members are node names, not ids.', + ), + groupName: z.string().optional().describe('For removeNodeGroup / updateNodeGroup.'), + nodeNames: z + .array(z.string()) + .optional() + .describe('For addNodeGroup / updateNodeGroup; group member node names.'), + id: z.string().optional().describe('For addNodeGroup; group id, generated if omitted.'), + }) + // Strict, so a field this schema does not declare fails the call instead of + // being stripped. Stripping made the tool report success for an operation it + // never ran: a guessed output-index field (e.g. sourceOutput) vanished and + // the connection was wired from output 0. + .strict() + .describe('Workflow update operation. Provide fields matching type.'); type OperationInput = { - type: (typeof baseOperationTypes)[number] | (typeof gatedGroupOperationTypes)[number]; + type: (typeof operationTypes)[number]; [key: string]: unknown; }; const strictOperationsSchema = z.array(partialUpdateOperationSchema); @@ -282,36 +252,24 @@ function parseStrictOperations(operations: OperationInput[]): PartialUpdateOpera } const NON_FATAL_OPERATION_TYPES_LIST = [...NON_FATAL_OPERATION_TYPES].join(', '); -const buildToolDescription = (canvasGroupsEnabled: boolean) => { - const base = - 'Atomically update an existing workflow with operation objects. Edits nodes/connections and also workflow-level settings via setWorkflowSettings — including the error workflow that runs automatically on failure to send alerts (e.g. when a user asks to "add error handling" or "notify me if this breaks"). Pass skillsUsed if n8n skills were used.'; - return canvasGroupsEnabled - ? `${base} Node-group operations (${NON_FATAL_OPERATION_TYPES_LIST}) are the one exception to "atomically": an invalid one is skipped and reported in skippedOperations instead of aborting the whole update. Separately, if other edits in the batch make an existing group invalid, that group is removed and reported in removedGroups.` - : base; -}; -// The concrete return type (not a widened z.ZodRawShape) keeps the tool's -// generic coupled to the real schema shape, so the handler's argument -// annotation is compile-checked against it via ToolHandler's parameter types. -const buildInputSchema = (canvasGroupsEnabled: boolean) => - ({ - workflowId: z.string().describe('The ID of the workflow to update.'), - skillsUsed: z.array(z.string()).optional().describe(SKILLS_USED_PARAM_DESCRIPTION), - operations: z - .array(buildOperationInputSchema(canvasGroupsEnabled)) - .min(1) - .max(MAX_OPERATIONS_PER_CALL) - .describe( - canvasGroupsEnabled - ? `Ordered operations to apply atomically (max ${MAX_OPERATIONS_PER_CALL}). If any op fails, nothing is saved — except node-group operations (${NON_FATAL_OPERATION_TYPES_LIST}): an invalid one is skipped and reported in skippedOperations, while the rest of the batch still saves. An existing group that these ops leave invalid is removed and reported in removedGroups.` - : `Ordered operations to apply atomically (max ${MAX_OPERATIONS_PER_CALL}). If any op fails, nothing is saved.`, - ), - versionName: versionNameInputSchema.describe( - 'Short summary of what this update changes, shown in the workflow\'s version history (e.g. "Added Slack notification after HTTP request"). Always provide it.', +const TOOL_DESCRIPTION = `Atomically update an existing workflow with operation objects. Edits nodes/connections and also workflow-level settings via setWorkflowSettings — including the error workflow that runs automatically on failure to send alerts (e.g. when a user asks to "add error handling" or "notify me if this breaks"). Pass skillsUsed if n8n skills were used. Node-group operations (${NON_FATAL_OPERATION_TYPES_LIST}) are the one exception to "atomically": an invalid one is skipped and reported in skippedOperations instead of aborting the whole update. Separately, if other edits in the batch make an existing group invalid, that group is removed and reported in removedGroups.`; +const inputSchema = { + workflowId: z.string().describe('The ID of the workflow to update.'), + skillsUsed: z.array(z.string()).optional().describe(SKILLS_USED_PARAM_DESCRIPTION), + operations: z + .array(operationInputSchema) + .min(1) + .max(MAX_OPERATIONS_PER_CALL) + .describe( + `Ordered operations to apply atomically (max ${MAX_OPERATIONS_PER_CALL}). If any op fails, nothing is saved — except node-group operations (${NON_FATAL_OPERATION_TYPES_LIST}): an invalid one is skipped and reported in skippedOperations, while the rest of the batch still saves. An existing group that these ops leave invalid is removed and reported in removedGroups.`, ), - versionDescription: versionDescriptionInputSchema.describe( - 'Longer description of what changed and why, shown in the version history alongside the version name.', - ), - }) satisfies z.ZodRawShape; + versionName: versionNameInputSchema.describe( + 'Short summary of what this update changes, shown in the workflow\'s version history (e.g. "Added Slack notification after HTTP request"). Always provide it.', + ), + versionDescription: versionDescriptionInputSchema.describe( + 'Longer description of what changed and why, shown in the version history alongside the version name.', + ), +} satisfies z.ZodRawShape; // The MCP SDK publishes this schema with `additionalProperties: false` and // validates `structuredContent` against it on every response. Success returns // the full payload below; the error path returns only `{ error }`. To keep @@ -824,23 +782,12 @@ const isSettingsOperation = (op: PartialUpdateOperation) => op.type === 'setWork /** * Rejects operations this instance cannot serve, before anything is loaded or - * applied. Throw order is part of the contract: gated group ops first, then - * tag ops. + * applied. */ function assertOperationsSupported( strictOperations: PartialUpdateOperation[], - { canvasGroupsEnabled, tagsDisabled }: { canvasGroupsEnabled: boolean; tagsDisabled: boolean }, + { tagsDisabled }: { tagsDisabled: boolean }, ): void { - // Defense in depth: with the flag off, the published schema already - // rejects these op types at the enum level; this guards against the - // loose and strict schemas drifting apart. Flag first so the scan only - // runs on instances where it can actually reject something. - if (!canvasGroupsEnabled && strictOperations.some((op) => GATED_GROUP_OP_TYPES.has(op.type))) { - throw new Error( - 'Node group operations (addNodeGroup, removeNodeGroup, updateNodeGroup) are not available on this instance.', - ); - } - if (tagsDisabled && strictOperations.some(isTagOperation)) { throw new Error('Tag operations are not supported on this instance because tags are disabled.'); } @@ -858,7 +805,6 @@ function assertOperationsSupported( */ function resolveNodeGroupViolations( result: ApplyOperationsSuccess, - canvasGroupsEnabled: boolean, nodeTypes: NodeTypes, ): { skippedOperations: SkippedOperation[]; @@ -877,16 +823,14 @@ function resolveNodeGroupViolations( // groups this batch touched, not which one caused a given violation — two group // ops that collide take each other down. const getNodeType = makeGetNodeTypeForGrouping(nodeTypes); - const violations = canvasGroupsEnabled - ? [ - ...dropInvalidWorkflowGroups( - result.workflow, - getNodeType, - (violation) => result.groupOperations[violation.groupId] !== undefined, - ), - ...dropInvalidWorkflowGroups(result.workflow, getNodeType), - ] - : []; + const violations = [ + ...dropInvalidWorkflowGroups( + result.workflow, + getNodeType, + (violation) => result.groupOperations[violation.groupId] !== undefined, + ), + ...dropInvalidWorkflowGroups(result.workflow, getNodeType), + ]; for (const violation of violations) { const requestedBy = result.groupOperations[violation.groupId]; @@ -1168,20 +1112,9 @@ export const createUpdateWorkflowTool = ( subworkflowPolicyChecker: SubworkflowPolicyChecker, workflowPublishedDataService: WorkflowPublishedDataService, aiGatewayService: AiGatewayService, - options: { - /** - * `102_mcp_canvas_groups` rollout flag: when true, the granular node-group - * operations (addNodeGroup, removeNodeGroup, updateNodeGroup) are published - * in the tool schema and accepted by the handler. `setNodeGroups` predates - * the flag and is always available. - */ - canvasGroupsEnabled?: boolean; - } = {}, logger: Logger, postSaveMetrics: McpPostSaveMetricsService, -): ToolDefinition> => { - const canvasGroupsEnabled = options.canvasGroupsEnabled === true; - +): ToolDefinition => { // Bound once: these never vary per call, so the call sites below show only // what is being validated. const settingsGuardDependencies: WorkflowSettingsGuardDependencies = { @@ -1196,8 +1129,8 @@ export const createUpdateWorkflowTool = ( return { name: MCP_UPDATE_WORKFLOW_TOOL.toolName, config: { - description: buildToolDescription(canvasGroupsEnabled), - inputSchema: buildInputSchema(canvasGroupsEnabled), + description: TOOL_DESCRIPTION, + inputSchema, outputSchema, annotations: { title: MCP_UPDATE_WORKFLOW_TOOL.displayTitle, @@ -1249,7 +1182,6 @@ export const createUpdateWorkflowTool = ( hasGraphOps = strictOperations.some((op) => GRAPH_OPERATION_TYPES.has(op.type)); assertOperationsSupported(strictOperations, { - canvasGroupsEnabled, tagsDisabled: globalConfig.tags.disabled, }); @@ -1266,7 +1198,6 @@ export const createUpdateWorkflowTool = ( const result = applyOperations( toWorkflowSlice(existingWorkflow, { includeTags: hasTagOperations }), strictOperations, - { canvasGroupsEnabled }, ); if (!result.success) { @@ -1274,7 +1205,7 @@ export const createUpdateWorkflowTool = ( } const { skippedOperations, removedGroups, nodeGroupsNeedPersisting } = - resolveNodeGroupViolations(result, canvasGroupsEnabled, nodeTypes); + resolveNodeGroupViolations(result, nodeTypes); const credentialCheck = await validateCredentialReferences( strictOperations, @@ -1405,12 +1336,9 @@ export const createUpdateWorkflowTool = ( result.groupOperations, ); - // Groups are dropped on save when the flag is off, so only warn when they can be kept. // A canvas that was already this wide before the update is marked pre-existing, // so the agent does not rework a layout it did not make. - const ceilingWarning = canvasGroupsEnabled - ? topLevelItemsWarning(updatedWorkflow) - : undefined; + const ceilingWarning = topLevelItemsWarning(updatedWorkflow); if (ceilingWarning) { const preExistingUngroupedNodeNames = new Set( diff --git a/packages/cli/src/modules/mcp/tools/workflow-builder/validate-workflow-code.tool.ts b/packages/cli/src/modules/mcp/tools/workflow-builder/validate-workflow-code.tool.ts index 130c129fa38..059fd55038d 100644 --- a/packages/cli/src/modules/mcp/tools/workflow-builder/validate-workflow-code.tool.ts +++ b/packages/cli/src/modules/mcp/tools/workflow-builder/validate-workflow-code.tool.ts @@ -13,16 +13,6 @@ import { getSdkReferenceHint } from '../workflow-validation.utils'; import { buildInvalidAiToolSourceErrorResponse } from './connection-structure-check'; import { CODE_BUILDER_VALIDATE_TOOL, MAX_WORKFLOW_CODE_LENGTH } from './constants'; -export type ValidateWorkflowCodeToolOptions = { - /** - * `102_mcp_canvas_groups` rollout flag: when true, node-group rule violations - * are reported as validation errors (`valid: false`) — they hard-block the - * save path — and traced in telemetry. Off by default — the tool then - * behaves exactly as before groups were validated. - */ - canvasGroupsEnabled?: boolean; -}; - const inputSchema = { code: z .string() @@ -66,7 +56,6 @@ export const createValidateWorkflowCodeTool = ( user: User, telemetry: Telemetry, nodeTypes: NodeTypes, - options: ValidateWorkflowCodeToolOptions = {}, ): ToolDefinition => ({ name: CODE_BUILDER_VALIDATE_TOOL.toolName, config: { @@ -109,11 +98,10 @@ export const createValidateWorkflowCodeTool = ( ); if (invalidToolSourceResponse) return invalidToolSourceResponse; - // `102_mcp_canvas_groups` rollout: report node-group rule violations as - // validation errors, with the same messages the save path rejects with — - // like the ai_tool-source check above, they hard-block saving. Flag off: - // output and telemetry are identical to before groups existed. - if (options.canvasGroupsEnabled && (result.workflow.nodeGroups?.length ?? 0) > 0) { + // Report node-group rule violations as validation errors, with the same + // messages the save path rejects with — like the ai_tool-source check + // above, they hard-block saving. + if ((result.workflow.nodeGroups?.length ?? 0) > 0) { const groupsResult = validateWorkflowGroups({ nodes: toGroupValidationNodes(result.workflow.nodes), connectionsBySourceNode: toEngineConnections(result.workflow.connections), @@ -150,11 +138,7 @@ export const createValidateWorkflowCodeTool = ( data: { nodeCount: result.workflow.nodes.length, warningCount: result.warnings.length, - // Rollout monitoring for `102_mcp_canvas_groups`; absent when the - // flag is off so the payload stays identical across cohorts. - ...(options.canvasGroupsEnabled - ? { groupCount: result.workflow.nodeGroups?.length ?? 0 } - : {}), + groupCount: result.workflow.nodeGroups?.length ?? 0, }, }; telemetry.track(USER_CALLED_MCP_TOOL_EVENT, telemetryPayload); diff --git a/packages/cli/src/modules/mcp/tools/workflow-builder/workflow-operations.ts b/packages/cli/src/modules/mcp/tools/workflow-builder/workflow-operations.ts index a092238c310..e7ffbf21980 100644 --- a/packages/cli/src/modules/mcp/tools/workflow-builder/workflow-operations.ts +++ b/packages/cli/src/modules/mcp/tools/workflow-builder/workflow-operations.ts @@ -1234,16 +1234,13 @@ const OPERATION_HANDLERS: { [K in PartialUpdateOperation['type']]: OpHandler * * The function never mutates the input. * - * With `{ canvasGroupsEnabled: true }`, a failing operation of a type in - * `NON_FATAL_OPERATION_TYPES` does not abort the batch — it is skipped and recorded - * in the result's `skippedOperations` instead, and the remaining operations still - * apply. With the flag off (or omitted), every operation is fatal, matching the - * historical behavior exactly. + * A failing operation of a type in `NON_FATAL_OPERATION_TYPES` does not abort + * the batch — it is skipped and recorded in the result's `skippedOperations` + * instead, and the remaining operations still apply. */ export function applyOperations( input: WorkflowSlice, operations: PartialUpdateOperation[], - options: { canvasGroupsEnabled?: boolean } = {}, ): ApplyOperationsResult { const skippedOperations: SkippedOperation[] = []; @@ -1268,7 +1265,7 @@ export function applyOperations( const error = handler(op, ctx, i); if (error) { - if (options.canvasGroupsEnabled && NON_FATAL_OPERATION_TYPES.has(op.type)) { + if (NON_FATAL_OPERATION_TYPES.has(op.type)) { skippedOperations.push({ opIndex: i, type: op.type, reason: error }); continue; }