diff --git a/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/parse-validate-handler.ts b/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/parse-validate-handler.ts index 4cd975502a8..8a3bf5466a4 100644 --- a/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/parse-validate-handler.ts +++ b/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/parse-validate-handler.ts @@ -7,8 +7,14 @@ */ import type { Logger } from '@n8n/backend-common'; -import { parseWorkflowCodeToBuilder, validateWorkflow, workflow } from '@n8n/workflow-sdk'; -import type { WorkflowJSON } from '@n8n/workflow-sdk'; +import { + connectRequiredSubnodeInputs, + describeAddedSubnodeConnection, + parseWorkflowCodeToBuilder, + validateWorkflow, + workflow, +} from '@n8n/workflow-sdk'; +import type { AddedSubnodeConnection, WorkflowJSON } from '@n8n/workflow-sdk'; import type { INodeTypes } from 'n8n-workflow'; import type { ParseAndValidateResult, ValidationWarning } from '../types'; @@ -67,6 +73,16 @@ export class ParseValidateHandler { this.nodeTypesProvider = config.nodeTypesProvider; } + /** + * Complete required AI subnode connections the generated code left unwired, + * e.g. an output parser with `autoFix: true` and no model. Needs node types, + * so a handler built without a provider leaves the workflow untouched. + */ + private connectRequiredInputs(json: WorkflowJSON): AddedSubnodeConnection[] { + if (!this.nodeTypesProvider) return []; + return connectRequiredSubnodeInputs(json, this.nodeTypesProvider); + } + /** * Options for the graph validation pass. * @@ -252,6 +268,7 @@ export class ParseValidateHandler { // Convert to JSON for JSON-based validation const json = builder.toJSON(); + this.connectRequiredInputs(json); // Run JSON-based validation for additional checks const validationResult = validateWorkflow(json, { @@ -292,6 +309,10 @@ export class ParseValidateHandler { // Convert to JSON with Dagre layout matching the FE's tidy-up const workflowJson: WorkflowJSON = builder.toJSON({ tidyUp: true, existingGroupIdsByName }); + // A fresh serialization, so the links added above have to be re-added. + for (const link of this.connectRequiredInputs(workflowJson)) { + allWarnings.push(describeAddedSubnodeConnection(link)); + } this.logger?.debug('Parsed workflow', { id: workflowJson.id, diff --git a/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler.test.ts b/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler.test.ts index 206a7d2f843..ad1a6f6caf7 100644 --- a/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler.test.ts +++ b/packages/@n8n/ai-workflow-builder.ee/src/code-builder/handlers/test/parse-validate-handler.test.ts @@ -3,7 +3,13 @@ */ import type { WorkflowJSON } from '@n8n/workflow-sdk'; -import { parseWorkflowCodeToBuilder, validateWorkflow, workflow } from '@n8n/workflow-sdk'; +import { + connectRequiredSubnodeInputs, + describeAddedSubnodeConnection, + parseWorkflowCodeToBuilder, + validateWorkflow, + workflow, +} from '@n8n/workflow-sdk'; import type { INodeTypes } from 'n8n-workflow'; import type { Mock } from 'vitest'; @@ -15,12 +21,16 @@ vi.mock('@n8n/workflow-sdk', () => ({ validateWorkflow: vi.fn(), workflow: { fromJSON: vi.fn() }, stripImportStatements: vi.fn((code: string) => code), + connectRequiredSubnodeInputs: vi.fn(() => []), + describeAddedSubnodeConnection: vi.fn(), })); // Typed mock references const mockParseWorkflowCodeToBuilder = parseWorkflowCodeToBuilder as Mock; const mockValidateWorkflow = validateWorkflow as Mock; const mockFromJSON = workflow.fromJSON as Mock; +const mockConnectRequiredSubnodeInputs = connectRequiredSubnodeInputs as Mock; +const mockDescribeAddedSubnodeConnection = describeAddedSubnodeConnection as Mock; describe('ParseValidateHandler', () => { let handler: ParseValidateHandler; @@ -58,6 +68,63 @@ describe('ParseValidateHandler', () => { expect(mockBuilder.validate).toHaveBeenCalled(); }); + it('completes required subnode inputs on the returned workflow and reports them', async () => { + // The helper itself is covered in required-subnode-connections.test.ts. + // What is untested here is the handler around it: that it runs against + // the serialized workflow with the provider, and surfaces what it added. + const nodeTypesProvider = { getByNameAndVersion: vi.fn() } as unknown as INodeTypes; + const mockWorkflow = { + id: 'test', + name: 'Test', + nodes: [{ id: 'parser', name: 'Parser', type: 'parser' }], + connections: {}, + }; + const mockBuilder = { + regenerateNodeIds: vi.fn(), + validate: vi.fn().mockReturnValue({ valid: true, errors: [], warnings: [] }), + generatePinData: vi.fn(), + toJSON: vi.fn().mockReturnValue(mockWorkflow), + }; + mockParseWorkflowCodeToBuilder.mockReturnValue(mockBuilder); + mockValidateWorkflow.mockReturnValue({ valid: true, errors: [], warnings: [] }); + mockConnectRequiredSubnodeInputs.mockReturnValue([ + { nodeName: 'Parser', connectionType: 'ai_languageModel', sourceNode: 'Model' }, + ]); + mockDescribeAddedSubnodeConnection.mockReturnValue({ + code: 'REQUIRED_SUBNODE_CONNECTED', + message: 'Connected Model to Parser', + severity: 'info', + }); + + const result = await new ParseValidateHandler({ nodeTypesProvider }).parseAndValidate( + 'const workflow = {}', + ); + + expect(mockConnectRequiredSubnodeInputs).toHaveBeenCalledWith( + mockWorkflow, + nodeTypesProvider, + ); + expect(result.workflow).toBe(mockWorkflow); + expect(result.warnings.map((warning) => warning.code)).toContain( + 'REQUIRED_SUBNODE_CONNECTED', + ); + }); + + it('leaves the workflow alone when no node-type provider is configured', async () => { + const mockBuilder = { + regenerateNodeIds: vi.fn(), + validate: vi.fn().mockReturnValue({ valid: true, errors: [], warnings: [] }), + generatePinData: vi.fn(), + toJSON: vi.fn().mockReturnValue({ id: 'test', name: 'Test', nodes: [], connections: {} }), + }; + mockParseWorkflowCodeToBuilder.mockReturnValue(mockBuilder); + mockValidateWorkflow.mockReturnValue({ valid: true, errors: [], warnings: [] }); + + await handler.parseAndValidate('const workflow = {}'); + + expect(mockConnectRequiredSubnodeInputs).not.toHaveBeenCalled(); + }); + it('skips structural checks in JSON validation that the graph pass already covers', async () => { const mockBuilder = { regenerateNodeIds: vi.fn(), diff --git a/packages/@n8n/workflow-sdk/src/codegen/emit-instance-ai.test.ts b/packages/@n8n/workflow-sdk/src/codegen/emit-instance-ai.test.ts index af964e8c974..5118c0cefd6 100644 --- a/packages/@n8n/workflow-sdk/src/codegen/emit-instance-ai.test.ts +++ b/packages/@n8n/workflow-sdk/src/codegen/emit-instance-ai.test.ts @@ -252,6 +252,8 @@ describe('emit-instance-ai', () => { 'hasProperty', 'isPlainObject', // Validation + 'connectRequiredSubnodeInputs', + 'describeAddedSubnodeConnection', 'containsExpression', 'isSensitiveHeader', 'isCredentialFieldName', diff --git a/packages/@n8n/workflow-sdk/src/index.ts b/packages/@n8n/workflow-sdk/src/index.ts index 11fa28468c9..8111a723ecd 100644 --- a/packages/@n8n/workflow-sdk/src/index.ts +++ b/packages/@n8n/workflow-sdk/src/index.ts @@ -176,6 +176,11 @@ export { type ValidateWorkflowBuilderOptions, type ValidateWorkflowBuilderResult, type CollectedValidationIssue, + connectRequiredSubnodeInputs, + describeAddedSubnodeConnection, + type AddedSubnodeConnection, + type ClearedSubnodeInput, + type WorkflowForSubnodeWiring, } from './validation'; // Code-node source lint — the host re-runs the Python rules with the executing diff --git a/packages/@n8n/workflow-sdk/src/validation.ts b/packages/@n8n/workflow-sdk/src/validation.ts index 7aafc0aae40..db3a527de41 100644 --- a/packages/@n8n/workflow-sdk/src/validation.ts +++ b/packages/@n8n/workflow-sdk/src/validation.ts @@ -16,6 +16,11 @@ export { type ValidateWorkflowBuilderOptions, type ValidateWorkflowBuilderResult, type CollectedValidationIssue, + connectRequiredSubnodeInputs, + describeAddedSubnodeConnection, + type AddedSubnodeConnection, + type ClearedSubnodeInput, + type WorkflowForSubnodeWiring, } from './validation/index'; export { diff --git a/packages/@n8n/workflow-sdk/src/validation/index.ts b/packages/@n8n/workflow-sdk/src/validation/index.ts index 5a3939d9922..86e8b807ff8 100644 --- a/packages/@n8n/workflow-sdk/src/validation/index.ts +++ b/packages/@n8n/workflow-sdk/src/validation/index.ts @@ -43,5 +43,13 @@ export { type DisplayOptionsContext, } from './display-options'; +export { + connectRequiredSubnodeInputs, + describeAddedSubnodeConnection, + type AddedSubnodeConnection, + type ClearedSubnodeInput, + type WorkflowForSubnodeWiring, +} from './required-subnode-connections'; + export { resolveMainInputCount } from './node-port-resolvers/resolve-main-input-count'; export { resolveMainOutputCount } from './node-port-resolvers/resolve-main-output-count'; diff --git a/packages/@n8n/workflow-sdk/src/validation/required-subnode-connections.test.ts b/packages/@n8n/workflow-sdk/src/validation/required-subnode-connections.test.ts new file mode 100644 index 00000000000..eb16d8d823e --- /dev/null +++ b/packages/@n8n/workflow-sdk/src/validation/required-subnode-connections.test.ts @@ -0,0 +1,251 @@ +import type { INodeTypes } from 'n8n-workflow'; +import { describe, expect, it } from 'vitest'; + +import { + connectRequiredSubnodeInputs, + type WorkflowForSubnodeWiring, +} from './required-subnode-connections'; + +const AGENT = '@n8n/n8n-nodes-langchain.agent'; +const PARSER = '@n8n/n8n-nodes-langchain.outputParserStructured'; +const LOADER = '@n8n/n8n-nodes-langchain.documentDefaultDataLoader'; +const MODEL = '@n8n/n8n-nodes-langchain.lmChatOpenAi'; +const AUTOFIX = '@n8n/n8n-nodes-langchain.outputParserAutofixing'; + +const BUILDER_HINTS: Record = { + [AGENT]: { + ai_languageModel: { required: true }, + ai_outputParser: { required: false, displayOptions: { show: { hasOutputParser: [true] } } }, + }, + [PARSER]: { + ai_languageModel: { required: true, displayOptions: { show: { autoFix: [true] } } }, + }, + [AUTOFIX]: { + ai_languageModel: { required: true }, + }, + [LOADER]: { + ai_textSplitter: { + required: true, + displayOptions: { show: { textSplittingMode: ['custom'] } }, + }, + }, +}; + +const nodeTypes = { + getByNameAndVersion: (type: string) => { + const inputs = BUILDER_HINTS[type]; + if (!inputs) return undefined; + return { description: { builderHint: { inputs } } }; + }, +} as unknown as INodeTypes; + +/** Agent with a model and a parser whose capability needs a model of its own. */ +function agentWithParser( + parserParameters: Record, + options: { models?: string[] } = {}, +): WorkflowForSubnodeWiring { + const models = options.models ?? ['OpenAI Chat Model']; + const connections: WorkflowForSubnodeWiring['connections'] = { + 'Output Parser': { + ai_outputParser: [[{ node: 'Feature Release Agent', type: 'ai_outputParser', index: 0 }]], + }, + }; + for (const model of models) { + connections[model] = { + ai_languageModel: [[{ node: 'Feature Release Agent', type: 'ai_languageModel', index: 0 }]], + }; + } + + return { + nodes: [ + { + name: 'Feature Release Agent', + type: AGENT, + typeVersion: 3.1, + parameters: { hasOutputParser: true }, + }, + { name: 'Output Parser', type: PARSER, typeVersion: 1.3, parameters: parserParameters }, + ...models.map((name) => ({ name, type: MODEL, typeVersion: 1.3, parameters: {} })), + ], + connections, + }; +} + +describe('connectRequiredSubnodeInputs', () => { + it("wires the agent's model into an autoFix parser that has none", () => { + const workflow = agentWithParser({ schemaType: 'fromJson', autoFix: true }); + + const result = connectRequiredSubnodeInputs(workflow, nodeTypes); + + expect(result).toEqual([ + { + sourceNode: 'OpenAI Chat Model', + targetNode: 'Output Parser', + connectionType: 'ai_languageModel', + viaParent: 'Feature Release Agent', + }, + ]); + expect(workflow.connections['OpenAI Chat Model']?.ai_languageModel?.[0]).toEqual([ + { node: 'Feature Release Agent', type: 'ai_languageModel', index: 0 }, + { node: 'Output Parser', type: 'ai_languageModel', index: 0 }, + ]); + }); + + it('leaves a parser alone when autoFix is off, since it needs no model', () => { + const workflow = agentWithParser({ schemaType: 'fromJson' }); + + const result = connectRequiredSubnodeInputs(workflow, nodeTypes); + + expect(result).toEqual([]); + expect(workflow.connections['OpenAI Chat Model']?.ai_languageModel?.[0]).toHaveLength(1); + }); + + it('leaves an already-wired required input alone', () => { + const workflow = agentWithParser({ autoFix: true }); + workflow.connections['OpenAI Chat Model']!.ai_languageModel![0]!.push({ + node: 'Output Parser', + type: 'ai_languageModel', + index: 0, + }); + + const result = connectRequiredSubnodeInputs(workflow, nodeTypes); + + expect(result).toEqual([]); + }); + + it('does not repair an input the caller just cleared', () => { + const workflow = agentWithParser({ autoFix: true }); + + const result = connectRequiredSubnodeInputs(workflow, nodeTypes, { + clearedInputs: [{ nodeName: 'Output Parser', connectionType: 'ai_languageModel' }], + }); + + expect(result).toEqual([]); + expect(workflow.connections['OpenAI Chat Model']?.ai_languageModel?.[0]).toHaveLength(1); + }); + + it('still repairs inputs the caller did not clear', () => { + const workflow = agentWithParser({ autoFix: true }); + + const result = connectRequiredSubnodeInputs(workflow, nodeTypes, { + clearedInputs: [{ nodeName: 'Some Other Node', connectionType: 'ai_languageModel' }], + }); + + expect(result).toHaveLength(1); + }); + + it('wires nothing when the parent has no model to take', () => { + const workflow = agentWithParser({ autoFix: true }, { models: [] }); + + expect(connectRequiredSubnodeInputs(workflow, nodeTypes)).toEqual([]); + }); + + it('wires nothing when the parent has two models, rather than picking one', () => { + const workflow = agentWithParser({ autoFix: true }, { models: ['Primary', 'Fallback'] }); + + expect(connectRequiredSubnodeInputs(workflow, nodeTypes)).toEqual([]); + expect(workflow.connections.Primary?.ai_languageModel?.[0]).toHaveLength(1); + expect(workflow.connections.Fallback?.ai_languageModel?.[0]).toHaveLength(1); + }); + + it('does not treat an ungated required input as missing', () => { + // The agent's own ai_languageModel is ungated and already connected. + const workflow = agentWithParser({ autoFix: true }); + + const result = connectRequiredSubnodeInputs(workflow, nodeTypes); + + expect(result.map((a) => a.targetNode)).toEqual(['Output Parser']); + }); + + it('handles a non-boolean gate on a different connection type', () => { + const workflow: WorkflowForSubnodeWiring = { + nodes: [ + { name: 'Agent', type: AGENT, typeVersion: 3.1, parameters: {} }, + { + name: 'Loader', + type: LOADER, + typeVersion: 1, + parameters: { textSplittingMode: 'custom' }, + }, + { name: 'Splitter', type: 'ts', typeVersion: 1, parameters: {} }, + ], + connections: { + Loader: { ai_document: [[{ node: 'Agent', type: 'ai_document', index: 0 }]] }, + Splitter: { ai_textSplitter: [[{ node: 'Agent', type: 'ai_textSplitter', index: 0 }]] }, + }, + }; + + const result = connectRequiredSubnodeInputs(workflow, nodeTypes); + + expect(result).toEqual([ + { + sourceNode: 'Splitter', + targetNode: 'Loader', + connectionType: 'ai_textSplitter', + viaParent: 'Agent', + }, + ]); + }); + + it('ignores node types it cannot resolve', () => { + const workflow: WorkflowForSubnodeWiring = { + nodes: [{ name: 'Mystery', type: 'community.unknown', typeVersion: 1, parameters: {} }], + connections: {}, + }; + + expect(connectRequiredSubnodeInputs(workflow, nodeTypes)).toEqual([]); + }); + + it('survives a provider that throws on an unknown version', () => { + const throwingNodeTypes = { + getByNameAndVersion: () => { + throw new Error('Node version not found'); + }, + } as unknown as INodeTypes; + + const workflow = agentWithParser({ autoFix: true }); + + expect(connectRequiredSubnodeInputs(workflow, throwingNodeTypes)).toEqual([]); + }); + + // An autofixing parser both consumes and produces `ai_outputParser`, so two + // of them chain. Repairing the outer one is what makes the inner one's source + // findable, and `workflow.nodes` carries no ordering guarantee. + describe.each([ + ['outer parser first', ['Autofix Outer', 'Autofix Inner']], + ['inner parser first', ['Autofix Inner', 'Autofix Outer']], + ])('with a repair that depends on another repair (%s)', (_label, order) => { + it('wires both, whatever order the nodes arrive in', () => { + const workflow: WorkflowForSubnodeWiring = { + nodes: [ + { name: 'Agent', type: AGENT, typeVersion: 3.1, parameters: {} }, + { name: 'OpenAI Chat Model', type: MODEL, typeVersion: 1.3, parameters: {} }, + ...order.map((name) => ({ name, type: AUTOFIX, typeVersion: 1, parameters: {} })), + ], + connections: { + 'OpenAI Chat Model': { + ai_languageModel: [[{ node: 'Agent', type: 'ai_languageModel', index: 0 }]], + }, + 'Autofix Outer': { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + 'Autofix Inner': { + ai_outputParser: [[{ node: 'Autofix Outer', type: 'ai_outputParser', index: 0 }]], + }, + }, + }; + + const added = connectRequiredSubnodeInputs(workflow, nodeTypes); + + expect(added.map((link) => link.targetNode).sort()).toEqual([ + 'Autofix Inner', + 'Autofix Outer', + ]); + // The inner one takes its model from the parent that was just repaired. + expect(added.find((link) => link.targetNode === 'Autofix Inner')).toMatchObject({ + sourceNode: 'OpenAI Chat Model', + viaParent: 'Autofix Outer', + }); + }); + }); +}); diff --git a/packages/@n8n/workflow-sdk/src/validation/required-subnode-connections.ts b/packages/@n8n/workflow-sdk/src/validation/required-subnode-connections.ts new file mode 100644 index 00000000000..9a1a23af157 --- /dev/null +++ b/packages/@n8n/workflow-sdk/src/validation/required-subnode-connections.ts @@ -0,0 +1,228 @@ +/** + * Completes AI subnode connections that a node's own parameters made required. + * + * Turning on a capability can add a required input one level below the node the + * author was thinking about (`autoFix` on an output parser needs a model), and + * generated code routinely sets the parameter without wiring it. + */ + +import type { INodeTypes } from 'n8n-workflow'; + +import { matchesDisplayOptions } from './display-options'; +import type { DisplayOptions, DisplayOptionsContext } from './display-options'; + +/** + * Structural rather than `WorkflowJSON` so both callers fit: the SDK and + * `n8n-workflow` declare separate node and connection types. + */ +export interface WorkflowForSubnodeWiring { + nodes: Array<{ + name?: string; + type: string; + typeVersion?: number | string; + parameters?: Record; + }>; + connections: { + [sourceNode: string]: + | { + [connectionType: string]: + | Array | null> + | undefined; + } + | undefined; + }; +} + +/** A connection this pass added. */ +export interface AddedSubnodeConnection { + sourceNode: string; + targetNode: string; + connectionType: string; + /** Parent both nodes hang off, which is where the source came from. */ + viaParent: string; +} + +/** Kept next to the producer so both callers share one wording. */ +export function describeAddedSubnodeConnection(link: AddedSubnodeConnection): { + code: string; + message: string; + nodeName: string; +} { + return { + code: 'REQUIRED_SUBNODE_CONNECTED', + message: `Connected '${link.sourceNode}' to the ${link.connectionType} input of '${link.targetNode}', which its own parameters made required. The source was taken from '${link.viaParent}'. Wire this explicitly in future edits.`, + nodeName: link.targetNode, + }; +} + +/** An input the caller cleared on purpose, which must not be repaired. */ +export interface ClearedSubnodeInput { + nodeName: string; + connectionType: string; +} + +/** node -> connection type -> source nodes feeding it. */ +type IncomingIndex = Map>>; + +function buildIncomingIndex(workflow: WorkflowForSubnodeWiring): IncomingIndex { + const incoming: IncomingIndex = new Map(); + + for (const [sourceNode, byType] of Object.entries(workflow.connections ?? {})) { + for (const [connectionType, outputs] of Object.entries(byType ?? {})) { + if (!Array.isArray(outputs)) continue; + for (const targets of outputs) { + for (const target of targets ?? []) { + if (!target?.node) continue; + const type = target.type ?? connectionType; + const byTargetType = incoming.get(target.node) ?? new Map>(); + const sources = byTargetType.get(type) ?? new Set(); + sources.add(sourceNode); + byTargetType.set(type, sources); + incoming.set(target.node, byTargetType); + } + } + } + } + + return incoming; +} + +/** Every node this subnode feeds over an `ai_*` connection. */ +function findParents(workflow: WorkflowForSubnodeWiring, nodeName: string): string[] { + const parents = new Set(); + const byType = workflow.connections?.[nodeName]; + + for (const [connectionType, outputs] of Object.entries(byType ?? {})) { + if (!connectionType.startsWith('ai_') || !Array.isArray(outputs)) continue; + for (const targets of outputs) { + for (const target of targets ?? []) { + if (target?.node) parents.add(target.node); + } + } + } + + return [...parents]; +} + +function addConnection( + workflow: WorkflowForSubnodeWiring, + incoming: IncomingIndex, + sourceNode: string, + targetNode: string, + connectionType: string, +): void { + const byType = (workflow.connections[sourceNode] ??= {}); + const outputs = (byType[connectionType] ??= []); + outputs[0] = [...(outputs[0] ?? []), { node: targetNode, type: connectionType, index: 0 }]; + + // Keep the index in step with the graph. A repair one level down reads the + // sources of its own parent, and that parent may have been repaired earlier + // in this same pass, so a stale index would lose the chain. + const byTargetType = incoming.get(targetNode) ?? new Map>(); + const sources = byTargetType.get(connectionType) ?? new Set(); + sources.add(sourceNode); + byTargetType.set(connectionType, sources); + incoming.set(targetNode, byTargetType); +} + +/** + * Mutates `workflow.connections` and returns the links added. Anything it cannot + * satisfy is left for `validateWorkflow` to report as `MISSING_REQUIRED_INPUT`. + * + * `clearedInputs` are inputs the caller just disconnected; repairing one would + * undo their own removal. + */ +export function connectRequiredSubnodeInputs( + workflow: WorkflowForSubnodeWiring, + nodeTypesProvider: INodeTypes, + options: { clearedInputs?: readonly ClearedSubnodeInput[] } = {}, +): AddedSubnodeConnection[] { + const cleared = new Set( + (options.clearedInputs ?? []).map((input) => `${input.nodeName}\u0000${input.connectionType}`), + ); + const added: AddedSubnodeConnection[] = []; + const incoming = buildIncomingIndex(workflow); + + function onePass(): number { + let count = 0; + + for (const node of workflow.nodes) { + if (!node.name) continue; + + // validate-workflow.ts does the same: the wire can carry a string. + const version = + typeof node.typeVersion === 'string' + ? parseFloat(node.typeVersion) + : (node.typeVersion ?? 1); + + let builderHintInputs; + try { + builderHintInputs = nodeTypesProvider.getByNameAndVersion(node.type, version)?.description + ?.builderHint?.inputs; + } catch { + continue; // unknown type or version + } + if (!builderHintInputs) continue; + + const parameters = node.parameters ?? {}; + + for (const [connectionType, inputConfig] of Object.entries(builderHintInputs)) { + if (!connectionType.startsWith('ai_')) continue; + if (!inputConfig?.required) continue; + if (cleared.has(`${node.name}\u0000${connectionType}`)) continue; + + // A gated input does not exist until its condition holds. + const displayOptions = inputConfig.displayOptions as DisplayOptions | undefined; + if (displayOptions) { + const context: DisplayOptionsContext = { + parameters, + nodeVersion: version, + rootParameters: parameters, + }; + if (!matchesDisplayOptions(context, displayOptions)) continue; + } + + // buildIncomingIndex only stores non-empty sets. + if (incoming.get(node.name)?.get(connectionType)) continue; + + // Take the source from the parent the subnode hangs off, not the whole graph. + const parentBySource = new Map(); + for (const parent of findParents(workflow, node.name)) { + for (const source of incoming.get(parent)?.get(connectionType) ?? []) { + if (source !== node.name) parentBySource.set(source, parent); + } + } + + // Wire only when the source is unambiguous. + const candidates = [...parentBySource.keys()]; + if (candidates.length !== 1) continue; + + const [sourceNode] = candidates; + addConnection(workflow, incoming, sourceNode, node.name, connectionType); + added.push({ + sourceNode, + targetNode: node.name, + connectionType, + viaParent: parentBySource.get(sourceNode) as string, + }); + count++; + } + } + + return count; + } + + // One repair can supply the parent another repair reads from, and + // `workflow.nodes` is in no particular order, so a single pass would make the + // result depend on that order. Sweep until a pass changes nothing. + // + // A pass only fills an input that had no source, and `addConnection` records + // it, so each input is filled at most once and each chain needs at most one + // pass per link. The cap is belt and braces: it holds even if a later edit + // stops the index from being updated, which would otherwise spin here. + for (let pass = 0; pass <= workflow.nodes.length; pass++) { + if (onePass() === 0) break; + } + + return added; +} diff --git a/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.migration.service.test.ts b/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.migration.service.test.ts index 0f3c5590c23..8a87760528d 100644 --- a/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.migration.service.test.ts +++ b/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.migration.service.test.ts @@ -187,7 +187,7 @@ describe('BreakingChangeMigrationService', () => { const workflow = buildWorkflow([aiNode], { versionId: 'v1', activeVersionId: 'v1' }); workflowFinderService.findWorkflowForUser.mockResolvedValue(workflow); workflowService.update.mockResolvedValue(mock({ versionId: 'new-version' })); - workflowValidationService.validateForActivation.mockReturnValue({ isValid: true }); + workflowValidationService.validateForActivation.mockResolvedValue({ isValid: true }); const result = await service.migrateWorkflow(RULE_ID, 'wf-1', user); @@ -230,7 +230,7 @@ describe('BreakingChangeMigrationService', () => { const workflow = buildWorkflow([aiNode], { versionId: 'v2', activeVersionId: 'v1' }); workflowFinderService.findWorkflowForUser.mockResolvedValue(workflow); workflowService.update.mockResolvedValue(mock({ versionId: 'new-version' })); - workflowValidationService.validateForActivation.mockReturnValue({ isValid: true }); + workflowValidationService.validateForActivation.mockResolvedValue({ isValid: true }); const result = await service.migrateWorkflow(RULE_ID, 'wf-1', user); @@ -245,7 +245,7 @@ describe('BreakingChangeMigrationService', () => { workflowFinderService.findWorkflowForUser.mockResolvedValue(workflow); workflowService.update.mockResolvedValue(mock({ versionId: 'new-version' })); // e.g. only a manual trigger, so activation validation fails. - workflowValidationService.validateForActivation.mockReturnValue({ + workflowValidationService.validateForActivation.mockResolvedValue({ isValid: false, error: 'no trigger', }); @@ -285,7 +285,7 @@ describe('BreakingChangeMigrationService', () => { workflowService.update.mockResolvedValue( mock({ versionId: 'new-version' }), ); - workflowValidationService.validateForActivation.mockReturnValue({ isValid: true }); + workflowValidationService.validateForActivation.mockResolvedValue({ isValid: true }); const result = await service.migrateWorkflow(EACH_RULE_ID, 'wf-1', user); diff --git a/packages/cli/src/modules/breaking-changes/breaking-changes.migration.service.ts b/packages/cli/src/modules/breaking-changes/breaking-changes.migration.service.ts index 57804559a17..ed067ad5190 100644 --- a/packages/cli/src/modules/breaking-changes/breaking-changes.migration.service.ts +++ b/packages/cli/src/modules/breaking-changes/breaking-changes.migration.service.ts @@ -94,10 +94,12 @@ export class BreakingChangeMigrationService { unmapped.length === 0 && notes.length === 0 && wasPublishedVersion && - this.workflowValidationService.validateForActivation( - Object.fromEntries(nodes.map((node) => [node.name, node])), - connections, - this.nodeTypes, + ( + await this.workflowValidationService.validateForActivation( + Object.fromEntries(nodes.map((node) => [node.name, node])), + connections, + this.nodeTypes, + ) ).isValid; // Checksum of the workflow as fetched, so a concurrent edit landing between 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 4a8266b7637..96455fb0192 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 @@ -3114,6 +3114,296 @@ describe('update-workflow MCP tool', () => { expect(response.validationWarnings).toEqual([]); }); + test('does not re-add a required subnode link that this batch removed', async () => { + nodeTypes.getByNameAndVersion.mockImplementation(((type: string) => { + if (type === '@n8n/n8n-nodes-langchain.outputParserStructured') { + return { + description: { + group: ['transform'], + builderHint: { + inputs: { + ai_languageModel: { + required: true, + displayOptions: { show: { autoFix: [true] } }, + }, + }, + }, + }, + }; + } + return { description: { group: ['transform'] } }; + }) as unknown as typeof nodeTypes.getByNameAndVersion); + + findWorkflowMock.mockResolvedValue( + Object.assign(new WorkflowEntity(), { + id: 'wf-1', + name: 'Existing', + settings: { availableInMCP: true }, + nodes: [ + makeNode({ + id: 'agent', + name: 'AI Agent', + type: '@n8n/n8n-nodes-langchain.agent', + typeVersion: 3.1, + }), + makeNode({ + id: 'parser', + name: 'Parser', + type: '@n8n/n8n-nodes-langchain.outputParserStructured', + typeVersion: 1.3, + parameters: { autoFix: true }, + }), + makeNode({ + id: 'model', + name: 'Model', + type: '@n8n/n8n-nodes-langchain.lmChatOpenAi', + typeVersion: 1.3, + }), + ], + connections: { + Parser: { + ai_outputParser: [[{ node: 'AI Agent', type: 'ai_outputParser', index: 0 }]], + }, + Model: { + ai_languageModel: [ + [ + { node: 'AI Agent', type: 'ai_languageModel', index: 0 }, + { node: 'Parser', type: 'ai_languageModel', index: 0 }, + ], + ], + }, + } as IConnections, + }), + ); + + const result = await callHandler({ + workflowId: 'wf-1', + operations: [ + { + type: 'removeConnection', + source: 'Model', + target: 'Parser', + connectionType: 'ai_languageModel', + }, + ], + }); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + const targets = saved.connections.Model.ai_languageModel?.[0]?.map((c) => c.node); + expect(targets).toEqual(['AI Agent']); + }); + + test('keeps the removal when the batch also renames the target node', async () => { + nodeTypes.getByNameAndVersion.mockImplementation(((type: string) => { + if (type === '@n8n/n8n-nodes-langchain.outputParserStructured') { + return { + description: { + group: ['transform'], + builderHint: { + inputs: { + ai_languageModel: { + required: true, + displayOptions: { show: { autoFix: [true] } }, + }, + }, + }, + }, + }; + } + return { description: { group: ['transform'] } }; + }) as unknown as typeof nodeTypes.getByNameAndVersion); + + findWorkflowMock.mockResolvedValue( + Object.assign(new WorkflowEntity(), { + id: 'wf-1', + name: 'Existing', + settings: { availableInMCP: true }, + nodes: [ + makeNode({ + id: 'agent', + name: 'AI Agent', + type: '@n8n/n8n-nodes-langchain.agent', + typeVersion: 3.1, + }), + makeNode({ + id: 'parser', + name: 'Parser', + type: '@n8n/n8n-nodes-langchain.outputParserStructured', + typeVersion: 1.3, + parameters: { autoFix: true }, + }), + makeNode({ + id: 'model', + name: 'Model', + type: '@n8n/n8n-nodes-langchain.lmChatOpenAi', + typeVersion: 1.3, + }), + ], + connections: { + Parser: { + ai_outputParser: [[{ node: 'AI Agent', type: 'ai_outputParser', index: 0 }]], + }, + Model: { + ai_languageModel: [ + [ + { node: 'AI Agent', type: 'ai_languageModel', index: 0 }, + { node: 'Parser', type: 'ai_languageModel', index: 0 }, + ], + ], + }, + } as IConnections, + }), + ); + + const result = await callHandler({ + workflowId: 'wf-1', + operations: [ + { + type: 'removeConnection', + source: 'Model', + target: 'Parser', + connectionType: 'ai_languageModel', + }, + { type: 'renameNode', oldName: 'Parser', newName: 'Schema' }, + ], + }); + + expect(result.isError).toBeUndefined(); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + const targets = saved.connections.Model.ai_languageModel?.[0]?.map((c) => c.node); + expect(targets).toEqual(['AI Agent']); + }); + + /** + * A parser whose required model input is unsatisfied, with the only model + * wired to the agent instead. A repair pass therefore has an unambiguous + * source to borrow, so any auto-wiring shows up in the saved graph. + */ + const seedParserMissingItsModel = () => { + nodeTypes.getByNameAndVersion.mockImplementation(((type: string) => { + if (type === '@n8n/n8n-nodes-langchain.outputParserStructured') { + return { + description: { + group: ['transform'], + builderHint: { + inputs: { + ai_languageModel: { + required: true, + displayOptions: { show: { autoFix: [true] } }, + }, + }, + }, + }, + }; + } + return { description: { group: ['transform'] } }; + }) as unknown as typeof nodeTypes.getByNameAndVersion); + + findWorkflowMock.mockResolvedValue( + Object.assign(new WorkflowEntity(), { + id: 'wf-1', + name: 'Existing', + settings: { availableInMCP: true }, + nodes: [ + makeNode({ + id: 'agent', + name: 'AI Agent', + type: '@n8n/n8n-nodes-langchain.agent', + typeVersion: 3.1, + }), + makeNode({ + id: 'parser', + name: 'Parser', + type: '@n8n/n8n-nodes-langchain.outputParserStructured', + typeVersion: 1.3, + parameters: { autoFix: true }, + }), + makeNode({ + id: 'model', + name: 'Model', + type: '@n8n/n8n-nodes-langchain.lmChatOpenAi', + typeVersion: 1.3, + }), + ], + connections: { + Parser: { + ai_outputParser: [[{ node: 'AI Agent', type: 'ai_outputParser', index: 0 }]], + }, + Model: { + ai_languageModel: [[{ node: 'AI Agent', type: 'ai_languageModel', index: 0 }]], + }, + } as IConnections, + }), + ); + }; + + test('does not auto-wire anything for a layout-only update', async () => { + seedParserMissingItsModel(); + const result = await callHandler({ + workflowId: 'wf-1', + operations: [{ type: 'setNodePosition', nodeName: 'Parser', position: [10, 20] }], + }); + + expect(result.isError).toBeUndefined(); + + const response = parseResult(result); + const warnings = (response.validationWarnings ?? []) as Array<{ code: string }>; + expect(warnings.some((w) => w.code === 'REQUIRED_SUBNODE_CONNECTED')).toBe(false); + + // The warning is derived from the links the repair added, so assert the + // graph too: dropping the warning must not be enough to pass this. + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + const targets = saved.connections.Model.ai_languageModel?.[0]?.map((c) => c.node); + expect(targets).toEqual(['AI Agent']); + }); + + test('does not auto-wire anything for a settings-only update', async () => { + seedParserMissingItsModel(); + const result = await callHandler({ + workflowId: 'wf-1', + operations: [ + { type: 'setNodeSettings', nodeName: 'Parser', settings: { retryOnFail: true } }, + ], + }); + + expect(result.isError).toBeUndefined(); + + const response = parseResult(result); + const warnings = (response.validationWarnings ?? []) as Array<{ code: string }>; + expect(warnings.some((w) => w.code === 'REQUIRED_SUBNODE_CONNECTED')).toBe(false); + + // The model stays wired to the agent alone; the repair pass must not + // have borrowed it for the parser's required input. + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + const targets = saved.connections.Model.ai_languageModel?.[0]?.map((c) => c.node); + expect(targets).toEqual(['AI Agent']); + }); + + test('does not auto-wire anything for a workflow-level-only update', async () => { + seedParserMissingItsModel(); + + const result = await callHandler({ + workflowId: 'wf-1', + operations: [ + { type: 'setWorkflowMetadata', description: 'Renamed by an agent' }, + { type: 'setWorkflowSettings', settings: { executionOrder: 'v1' } }, + ], + }); + + expect(result.isError).toBeUndefined(); + + const response = parseResult(result); + const warnings = (response.validationWarnings ?? []) as Array<{ code: string }>; + expect(warnings.some((w) => w.code === 'REQUIRED_SUBNODE_CONNECTED')).toBe(false); + + const saved = updateMock.mock.calls[0][1] as WorkflowEntity; + const targets = saved.connections.Model.ai_languageModel?.[0]?.map((c) => c.node); + expect(targets).toEqual(['AI Agent']); + }); + test('refuses to save when an addConnection wires an agent as a tool to another agent', async () => { findWorkflowMock.mockResolvedValue( Object.assign(new WorkflowEntity(), { 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 5243cabae6a..163673e54ce 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 @@ -3,8 +3,13 @@ import type { Logger } from '@n8n/backend-common'; import type { GlobalConfig } from '@n8n/config'; import { type User, type SharedWorkflowRepository, WorkflowEntity } from '@n8n/db'; import { hasGlobalScope } from '@n8n/permissions'; +import { + connectRequiredSubnodeInputs, + describeAddedSubnodeConnection, + type ClearedSubnodeInput, +} from '@n8n/workflow-sdk'; import isEqual from 'lodash/isEqual'; -import type { IWorkflowSettings } from 'n8n-workflow'; +import { NodeConnectionTypes, type IWorkflowSettings } from 'n8n-workflow'; import { z } from 'zod'; import type { CollaborationService } from '@/collaboration/collaboration.service'; @@ -729,6 +734,26 @@ const isTagOperation = (op: PartialUpdateOperation) => const isSettingsOperation = (op: PartialUpdateOperation) => op.type === 'setWorkflowSettings'; +/** + * Operations that cannot leave a required subnode input unsatisfied: they touch + * neither connections nor the parameters a conditional `inputs` expression reads. + * Node-level execution settings qualify, since `inputs` only sees `$parameter`. + */ +const GRAPH_NEUTRAL_OPERATIONS = new Set([ + 'addTags', + 'removeTags', + 'setNodeSettings', + 'setWorkflowSettings', + 'setWorkflowMetadata', + 'addNodeGroup', + 'removeNodeGroup', + 'updateNodeGroup', + 'setNodeGroups', + 'setNodePosition', +]); + +const touchesGraph = (op: PartialUpdateOperation) => !GRAPH_NEUTRAL_OPERATIONS.has(op.type); + /** * Rejects operations this instance cannot serve, before anything is loaded or * applied. @@ -1160,6 +1185,29 @@ export const createUpdateWorkflowTool = ( throw new Error(result.error); } + // Setting a parameter can make a subnode input required without wiring it. + // Skip inputs this batch disconnected, or removeConnection is a no-op. + // Names are carried through later renames, since the pass runs against + // the final graph. + const clearedInputs: ClearedSubnodeInput[] = []; + for (const op of strictOperations) { + if (op.type === 'removeConnection') { + clearedInputs.push({ + nodeName: op.target, + connectionType: op.connectionType ?? NodeConnectionTypes.Main, + }); + } else if (op.type === 'renameNode') { + for (const cleared of clearedInputs) { + if (cleared.nodeName === op.oldName) cleared.nodeName = op.newName; + } + } + } + // A tag, settings or layout edit cannot create an unsatisfied input, and + // wiring one off the back of such an update would be a surprise. + const addedSubnodeLinks = strictOperations.some(touchesGraph) + ? connectRequiredSubnodeInputs(result.workflow, nodeTypes, { clearedInputs }) + : []; + const { skippedOperations, removedGroups, nodeGroupsNeedPersisting } = resolveNodeGroupViolations(result, nodeTypes); @@ -1248,6 +1296,10 @@ export const createUpdateWorkflowTool = ( nodeTypes, ); + for (const link of addedSubnodeLinks) { + validationWarnings.push(describeAddedSubnodeConnection(link)); + } + validationWarnings.push( ...(await buildUninstalledNodeWarnings( workflowUpdateData.nodes.filter((node) => result.addedNodeNames.includes(node.name)), diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/support/workflow-review-test-data.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/support/workflow-review-test-data.ts index f5484d94f19..079805f3cc4 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/support/workflow-review-test-data.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/support/workflow-review-test-data.ts @@ -54,7 +54,7 @@ export const REVIEW_TABLES = [ export function stubWorkflowValidation( workflowValidationService: MockProxy, ): void { - workflowValidationService.validateForActivation.mockReturnValue({ isValid: true }); + workflowValidationService.validateForActivation.mockResolvedValue({ isValid: true }); workflowValidationService.validateDynamicCredentials.mockResolvedValue({ isValid: true }); workflowValidationService.validatePublisherCredentialAccess.mockResolvedValue({ isValid: true }); workflowValidationService.validateSubWorkflowReferences.mockResolvedValue({ isValid: true }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-publish-guard.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-publish-guard.integration.test.ts index e3716b3012f..1d4c48fc16f 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-publish-guard.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-publish-guard.integration.test.ts @@ -138,7 +138,7 @@ describe('publishing a workflow under review', () => { const request = await createOpenReview(workflow.id, versionId); // Activation fails once — after the approval has already committed. - workflowValidationService.validateForActivation.mockReturnValueOnce({ + workflowValidationService.validateForActivation.mockResolvedValueOnce({ isValid: false, error: 'The workflow has issues', }); diff --git a/packages/cli/src/workflows/__tests__/workflow-validation.service.test.ts b/packages/cli/src/workflows/__tests__/workflow-validation.service.test.ts index fdfeac0f4e4..02dcef8f38a 100644 --- a/packages/cli/src/workflows/__tests__/workflow-validation.service.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow-validation.service.test.ts @@ -6,6 +6,7 @@ import type { INodeTypeDescription, ICredentialType, } from 'n8n-workflow'; +import { deepCopy, NodeConnectionTypes } from 'n8n-workflow'; import { mock } from 'vitest-mock-extended'; import type { CredentialTypes } from '@/credential-types'; @@ -327,7 +328,7 @@ describe('WorkflowValidationService', () => { return nodeType; }; - it('should return valid for workflow with no connected nodes', () => { + it('should return valid for workflow with no connected nodes', async () => { const nodes = { Webhook: createNode('Webhook', 'n8n-nodes-base.webhook'), }; @@ -335,12 +336,12 @@ describe('WorkflowValidationService', () => { mockNodeTypes.getByNameAndVersion.mockReturnValue(createMockNodeType([], [], true)); - const result = service.validateForActivation(nodes, connections, mockNodeTypes); + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); expect(result.isValid).toBe(true); }); - it('should return valid for workflow with all valid connected nodes', () => { + it('should return valid for workflow with all valid connected nodes', async () => { const nodes = { Webhook: createNode('Webhook', 'n8n-nodes-base.webhook'), 'HTTP Request': createNode('HTTP Request', 'n8n-nodes-base.httpRequest', { @@ -365,12 +366,12 @@ describe('WorkflowValidationService', () => { return undefined; }) as any); - const result = service.validateForActivation(nodes, connections, mockNodeTypes); + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); expect(result.isValid).toBe(true); }); - it('should return invalid when connected node is missing required credential', () => { + it('should return invalid when connected node is missing required credential', async () => { const nodes = { Webhook: createNode('Webhook', 'n8n-nodes-base.webhook'), Agent: createNode('Agent', 'n8n-nodes-base.agent', { @@ -394,7 +395,7 @@ describe('WorkflowValidationService', () => { return undefined; }) as any); - const result = service.validateForActivation(nodes, connections, mockNodeTypes); + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); expect(result.isValid).toBe(false); expect(result.error).toContain('Cannot publish workflow'); @@ -403,7 +404,7 @@ describe('WorkflowValidationService', () => { expect(result.error).toContain('Missing required credential: OpenAI API'); }); - it('should return invalid when connected node has credential without ID', () => { + it('should return invalid when connected node has credential without ID', async () => { const nodes = { Webhook: createNode('Webhook', 'n8n-nodes-base.webhook'), Agent: createNode('Agent', 'n8n-nodes-base.agent', { @@ -428,13 +429,13 @@ describe('WorkflowValidationService', () => { return undefined; }) as any); - const result = service.validateForActivation(nodes, connections, mockNodeTypes); + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); expect(result.isValid).toBe(false); expect(result.error).toContain('Credential not configured: OpenAI API'); }); - it('should skip validation for disabled nodes', () => { + it('should skip validation for disabled nodes', async () => { const nodes = { Webhook: createNode('Webhook', 'n8n-nodes-base.webhook'), Agent: createNode('Agent', 'n8n-nodes-base.agent', { @@ -459,12 +460,12 @@ describe('WorkflowValidationService', () => { return undefined; }) as any); - const result = service.validateForActivation(nodes, connections, mockNodeTypes); + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); expect(result.isValid).toBe(true); }); - it('should skip validation for disconnected nodes', () => { + it('should skip validation for disconnected nodes', async () => { const nodes = { Webhook: createNode('Webhook', 'n8n-nodes-base.webhook'), Agent: createNode('Agent', 'n8n-nodes-base.agent', { @@ -489,12 +490,43 @@ describe('WorkflowValidationService', () => { return undefined; }) as any); - const result = service.validateForActivation(nodes, connections, mockNodeTypes); + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); expect(result.isValid).toBe(true); }); - it('should validate multiple nodes with issues', () => { + it('should skip validation for an island with no path from a trigger', async () => { + const nodes = { + Webhook: createNode('Webhook', 'n8n-nodes-base.webhook'), + Agent: createNode('Agent', 'n8n-nodes-base.agent', { parameters: {} }), + Helper: createNode('Helper', 'n8n-nodes-base.agent', { parameters: {} }), + }; + // Wired to each other but never reached by the webhook, so neither runs. + const connections: IConnections = { + Helper: { main: [[{ node: 'Agent', type: NodeConnectionTypes.Main, index: 0 }]] }, + }; + + mockNodeTypes.getByNameAndVersion.mockImplementation((( + type: string, + ): INodeType | undefined => { + if (type === 'n8n-nodes-base.webhook') { + return createMockNodeType([], [], true); + } + if (type === 'n8n-nodes-base.agent') { + return createMockNodeType( + [{ name: 'openAiApi', displayName: 'OpenAI API', required: true }], + [], + ); + } + return undefined; + }) as any); + + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); + + expect(result.isValid).toBe(true); + }); + + it('should validate multiple nodes with issues', async () => { const nodes = { Webhook: createNode('Webhook', 'n8n-nodes-base.webhook'), Agent1: createNode('Agent1', 'n8n-nodes-base.agent', { @@ -524,7 +556,7 @@ describe('WorkflowValidationService', () => { return undefined; }) as any); - const result = service.validateForActivation(nodes, connections, mockNodeTypes); + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); expect(result.isValid).toBe(false); expect(result.error).toContain('2 nodes have configuration issues'); @@ -532,7 +564,7 @@ describe('WorkflowValidationService', () => { expect(result.error).toContain('Node "Agent2"'); }); - it('should return invalid when node type is not found', () => { + it('should return invalid when node type is not found', async () => { const nodes = { Webhook: createNode('Webhook', 'n8n-nodes-base.webhook'), Unknown: createNode('Unknown', 'n8n-nodes-base.unknownNode'), @@ -548,14 +580,14 @@ describe('WorkflowValidationService', () => { return undefined; }) as any); - const result = service.validateForActivation(nodes, connections, mockNodeTypes); + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); expect(result.isValid).toBe(false); expect(result.error).toContain('Node "Unknown"'); expect(result.error).toContain('Node type not found'); }); - it('should return invalid when workflow has no trigger node', () => { + it('should return invalid when workflow has no trigger node', async () => { const nodes = { Set: createNode('Set', 'n8n-nodes-base.set'), }; @@ -563,13 +595,13 @@ describe('WorkflowValidationService', () => { mockNodeTypes.getByNameAndVersion.mockReturnValue(createMockNodeType([], [])); - const result = service.validateForActivation(nodes, connections, mockNodeTypes); + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); expect(result.isValid).toBe(false); expect(result.error).toContain('no trigger node'); }); - it('should respect displayOptions when validating credentials', () => { + it('should respect displayOptions when validating credentials', async () => { // Simulates a Webhook node with authentication parameter set to 'none' const nodes = { Webhook: createNode('Webhook', 'n8n-nodes-base.webhook', { @@ -605,13 +637,13 @@ describe('WorkflowValidationService', () => { mockNodeTypes.getByNameAndVersion.mockReturnValue(nodeType); - const result = service.validateForActivation(nodes, connections, mockNodeTypes); + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); // Should be valid because authentication='none', so no credentials are required expect(result.isValid).toBe(true); }); - it('should validate credentials when displayOptions match', () => { + it('should validate credentials when displayOptions match', async () => { // Simulates a Webhook node with authentication='basicAuth' but missing credential const nodes = { Webhook: createNode('Webhook', 'n8n-nodes-base.webhook', { @@ -637,7 +669,7 @@ describe('WorkflowValidationService', () => { mockNodeTypes.getByNameAndVersion.mockReturnValue(nodeType); - const result = service.validateForActivation(nodes, connections, mockNodeTypes); + const result = await service.validateForActivation(nodes, connections, mockNodeTypes); // Should be invalid because authentication='basicAuth' but no credential is set expect(result.isValid).toBe(false); @@ -1742,4 +1774,704 @@ describe('WorkflowValidationService', () => { expect(result.error).toContain('"Webhook"'); }); }); + describe('validateRequiredInputsConnected', () => { + const nodeTypes = mock(); + + const node = (name: string, type: string, disabled = false): INode => ({ + name, + type, + id: `node-${name}`, + typeVersion: 1, + position: [0, 0], + parameters: {}, + ...(disabled ? { disabled } : {}), + }); + + /** Parser-like node whose `Model` input is declared required. */ + const parserType = { + description: { + displayName: 'Parser', + name: 'parser', + group: ['transform'], + version: 1, + description: '', + defaults: { name: 'Parser' }, + inputs: [{ displayName: 'Model', type: 'ai_languageModel', required: true }], + outputs: ['ai_outputParser'], + properties: [], + } as unknown as INodeTypeDescription, + } as INodeType; + + /** A parent with no required gated inputs of its own. */ + const agentType = { + description: { + displayName: 'Agent', + name: 'agent', + group: ['transform'], + version: 1, + description: '', + defaults: { name: 'Agent' }, + inputs: ['main'], + outputs: ['main'], + properties: [], + } as unknown as INodeTypeDescription, + } as INodeType; + + const modelType = { + description: { + displayName: 'Model', + name: 'model', + group: ['transform'], + version: 1, + description: '', + defaults: { name: 'Model' }, + inputs: [], + outputs: ['ai_languageModel'], + properties: [], + } as unknown as INodeTypeDescription, + } as INodeType; + + /** An entry point, so nodes wired behind it are reachable by a run. */ + const triggerType = { + description: { + displayName: 'Trigger', + name: 'trigger', + group: ['trigger'], + version: 1, + description: '', + defaults: { name: 'Trigger' }, + inputs: [], + outputs: ['main'], + properties: [], + } as unknown as INodeTypeDescription, + trigger: async () => undefined, + } as unknown as INodeType; + + /** Wires the trigger into `target`, the way an activatable workflow would. */ + const startedAt = (target: string) => ({ + Trigger: { main: [[{ node: target, type: 'main', index: 0 }]] }, + }); + + beforeEach(() => { + nodeTypes.getByNameAndVersion.mockImplementation((type: string) => { + if (type === 'trigger') return triggerType; + if (type === 'parser') return parserType; + if (type === 'agent') return agentType; + return modelType; + }); + }); + + it('rejects activation when a required input has nothing connected', async () => { + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'trigger'), node('Parser', 'parser'), node('Agent', 'agent')], + { + ...startedAt('Agent'), + Parser: { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + expect(result.error).toContain( + "'Parser' has no node connected to its required 'Model' input", + ); + }); + + it('ignores an island of wired nodes with no path from a trigger', async () => { + // The parser is connected, so it is not a floating node, but nothing + // reaches its agent. The island never runs, so it must not block + // publishing. + const result = await service.validateRequiredInputsConnected( + [ + node('Trigger', 'trigger'), + node('Tail', 'agent'), + node('Parser', 'parser'), + node('Island', 'agent'), + ], + { + ...startedAt('Tail'), + Parser: { + ai_outputParser: [[{ node: 'Island', type: 'ai_outputParser', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result).toEqual({ isValid: true }); + }); + + it('ignores a chain behind a disabled trigger', async () => { + // A disabled trigger starts nothing, so nothing downstream of it runs. + nodeTypes.getByNameAndVersion.mockImplementation((type: string) => { + if (type === 'trigger') return triggerType; + if (type === 'mainParser') { + return { + description: { + ...parserType.description, + outputs: ['main'], + } as unknown as INodeTypeDescription, + } as INodeType; + } + return modelType; + }); + + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'trigger', true), node('Parser', 'mainParser')], + startedAt('Parser') as unknown as IConnections, + nodeTypes, + ); + + expect(result).toEqual({ isValid: true }); + }); + + it('still checks a node behind a disabled node on the main path', async () => { + // A disabled node passes its input through, so what follows it still runs. + nodeTypes.getByNameAndVersion.mockImplementation((type: string) => { + if (type === 'trigger') return triggerType; + if (type === 'mainParser') { + return { + description: { + ...parserType.description, + outputs: ['main'], + } as unknown as INodeTypeDescription, + } as INodeType; + } + if (type === 'agent') return agentType; + return modelType; + }); + + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'trigger'), node('Skipped', 'agent', true), node('Parser', 'mainParser')], + { + ...startedAt('Skipped'), + Skipped: { main: [[{ node: 'Parser', type: 'main', index: 0 }]] }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + expect(result.error).toContain("'Parser'"); + }); + + it('ignores a node that is wired to nothing', async () => { + // A floating node cannot break a run, so it must not block publishing. + const result = await service.validateRequiredInputsConnected( + [node('Parser', 'parser')], + {}, + nodeTypes, + ); + + expect(result).toEqual({ isValid: true }); + }); + + it('ignores a subnode whose only consumer is disabled', async () => { + // The trigger reaches the disabled agent, so the parser counts as + // executable. A disabled agent never resolves it though, so only the + // disabled-consumer exemption keeps this workflow publishable. + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'trigger'), node('Parser', 'parser'), node('Agent', 'agent', true)], + { + ...startedAt('Agent'), + Parser: { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result).toEqual({ isValid: true }); + }); + + it('ignores a subnode nested behind a disabled consumer', async () => { + // 'Inner' feeds 'Outer', which is enabled, so looking only one hop out + // reads it as live. 'Outer' only feeds the disabled agent though, so + // nothing ever resolves either of them. + const result = await service.validateRequiredInputsConnected( + [ + node('Trigger', 'trigger'), + node('Inner', 'parser'), + node('Outer', 'parser'), + node('Agent', 'agent', true), + ], + { + ...startedAt('Agent'), + Outer: { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + Inner: { + ai_outputParser: [[{ node: 'Outer', type: 'ai_outputParser', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result).toEqual({ isValid: true }); + }); + + it('still checks a subnode nested behind an enabled consumer', async () => { + // Same shape with the agent enabled, so the whole chain resolves. + const result = await service.validateRequiredInputsConnected( + [ + node('Trigger', 'trigger'), + node('Inner', 'parser'), + node('Outer', 'parser'), + node('Agent', 'agent'), + ], + { + ...startedAt('Agent'), + Outer: { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + Inner: { + ai_outputParser: [[{ node: 'Outer', type: 'ai_outputParser', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + expect(result.error).toContain("'Inner'"); + expect(result.error).toContain("'Outer'"); + }); + + it('ignores a subnode whose consumer chain loops back on itself', async () => { + // A cycle between two supply nodes must not hang the consumer walk. + const result = await service.validateRequiredInputsConnected( + [ + node('Trigger', 'trigger'), + node('Left', 'parser'), + node('Right', 'parser'), + node('Agent', 'agent', true), + ], + { + ...startedAt('Agent'), + Left: { + ai_outputParser: [ + [ + { node: 'Right', type: 'ai_outputParser', index: 0 }, + { node: 'Agent', type: 'ai_outputParser', index: 0 }, + ], + ], + }, + Right: { + ai_outputParser: [[{ node: 'Left', type: 'ai_outputParser', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result).toEqual({ isValid: true }); + }); + + it('still checks a subnode that also supplies an enabled consumer', async () => { + const result = await service.validateRequiredInputsConnected( + [ + node('Trigger', 'trigger'), + node('Parser', 'parser'), + node('Off', 'agent', true), + node('On', 'agent'), + ], + { + ...startedAt('On'), + Parser: { + ai_outputParser: [ + [ + { node: 'Off', type: 'ai_outputParser', index: 0 }, + { node: 'On', type: 'ai_outputParser', index: 0 }, + ], + ], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + }); + + it('still checks a node that can output main but has only a disabled subnode consumer', async () => { + // Its main output happens to be unwired, so the connection record shows + // only the ai_ edge. Declared outputs are what decide, not the wiring. + nodeTypes.getByNameAndVersion.mockImplementation((type: string) => { + if (type === 'trigger') return triggerType; + if (type === 'dualOutput') { + return { + description: { + ...parserType.description, + outputs: ['main', 'ai_tool'], + } as unknown as INodeTypeDescription, + } as INodeType; + } + return agentType; + }); + + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'trigger'), node('Store', 'dualOutput'), node('Agent', 'agent', true)], + { + ...startedAt('Store'), + Store: { ai_tool: [[{ node: 'Agent', type: 'ai_tool', index: 0 }]] }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + expect(result.error).toContain("'Store'"); + }); + + it('still checks a node on the main path when its consumer is disabled', async () => { + // Unlike a subnode, a node with a main output runs whatever happens + // downstream, so its own unmet input still blocks publishing. + nodeTypes.getByNameAndVersion.mockImplementation((type: string) => { + if (type === 'trigger') return triggerType; + if (type === 'mainParser') { + return { + description: { + ...parserType.description, + outputs: ['main'], + } as unknown as INodeTypeDescription, + } as INodeType; + } + return agentType; + }); + + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'trigger'), node('Parser', 'mainParser'), node('Next', 'agent', true)], + { + ...startedAt('Parser'), + Parser: { main: [[{ node: 'Next', type: 'main', index: 0 }]] }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + }); + + describe('with a fallback model declared on a second input of the same type', () => { + const agentWithFallbackType = { + description: { + displayName: 'Agent', + name: 'agentWithFallback', + group: ['transform'], + version: 1, + description: '', + defaults: { name: 'Agent' }, + inputs: [ + 'main', + { displayName: 'Chat Model', type: 'ai_languageModel', required: true }, + { displayName: 'Fallback Model', type: 'ai_languageModel', required: true }, + ], + outputs: ['main'], + properties: [], + } as unknown as INodeTypeDescription, + } as INodeType; + + beforeEach(() => { + nodeTypes.getByNameAndVersion.mockImplementation((type: string) => { + if (type === 'trigger') return triggerType; + return type === 'agentWithFallback' ? agentWithFallbackType : modelType; + }); + }); + + it('rejects activation when only the primary model is connected', async () => { + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'trigger'), node('Agent', 'agentWithFallback'), node('Model', 'model')], + { + ...startedAt('Agent'), + Model: { + ai_languageModel: [[{ node: 'Agent', type: 'ai_languageModel', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + expect(result.error).toContain("'Fallback Model'"); + }); + + it('allows activation once each model sits on its own input', async () => { + const result = await service.validateRequiredInputsConnected( + [ + node('Trigger', 'trigger'), + node('Agent', 'agentWithFallback'), + node('Primary', 'model'), + node('Fallback', 'model'), + ], + { + ...startedAt('Agent'), + Primary: { + ai_languageModel: [[{ node: 'Agent', type: 'ai_languageModel', index: 0 }]], + }, + Fallback: { + ai_languageModel: [[{ node: 'Agent', type: 'ai_languageModel', index: 1 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result).toEqual({ isValid: true }); + }); + + it('treats a disabled fallback as not connected', async () => { + const result = await service.validateRequiredInputsConnected( + [ + node('Trigger', 'trigger'), + node('Agent', 'agentWithFallback'), + node('Primary', 'model'), + node('Fallback', 'model', true), + ], + { + ...startedAt('Agent'), + Primary: { + ai_languageModel: [[{ node: 'Agent', type: 'ai_languageModel', index: 0 }]], + }, + Fallback: { + ai_languageModel: [[{ node: 'Agent', type: 'ai_languageModel', index: 1 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + expect(result.error).toContain("'Fallback Model'"); + }); + }); + + it('still checks a trigger that is wired to nothing', async () => { + // A trigger starts the run whether or not anything feeds it, so the + // floating-node exemption must not cover it. + nodeTypes.getByNameAndVersion.mockImplementation((type: string) => { + if (type === 'trigger') return triggerType; + if (type === 'triggerParser') { + return { ...parserType, trigger: async () => undefined } as unknown as INodeType; + } + return modelType; + }); + + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'triggerParser')], + {}, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + expect(result.error).toContain("'Trigger'"); + }); + + it('does not rewrite the parameters of the nodes it was given', async () => { + // These are the nodes about to be persisted as the active version. + const parser = node('Parser', 'parser'); + const before = parser.parameters; + + await service.validateRequiredInputsConnected( + [parser, node('Model', 'model')], + { + Model: { + ai_languageModel: [[{ node: 'Parser', type: 'ai_languageModel', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(parser.parameters).toBe(before); + }); + + it('allows activation once the required input is connected', async () => { + const result = await service.validateRequiredInputsConnected( + [node('Parser', 'parser'), node('Model', 'model')], + { + Model: { + ai_languageModel: [[{ node: 'Parser', type: 'ai_languageModel', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result).toEqual({ isValid: true }); + }); + + it('treats a disabled source as not connected', async () => { + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'trigger'), node('Parser', 'parser'), node('Model', 'model', true)], + { + ...startedAt('Parser'), + Model: { + ai_languageModel: [[{ node: 'Parser', type: 'ai_languageModel', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + }); + + it('ignores disabled nodes with unmet requirements', async () => { + const result = await service.validateRequiredInputsConnected( + [node('Parser', 'parser', true), node('Agent', 'agent')], + { + Parser: { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result).toEqual({ isValid: true }); + }); + + it('reports every unmet required input at once', async () => { + const result = await service.validateRequiredInputsConnected( + [ + node('Trigger', 'trigger'), + node('Parser A', 'parser'), + node('Parser B', 'parser'), + node('Agent', 'agent'), + ], + { + ...startedAt('Agent'), + 'Parser A': { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + 'Parser B': { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.error).toContain("'Parser A'"); + expect(result.error).toContain("'Parser B'"); + }); + + describe('with a dynamic inputs expression', () => { + /** `inputs` is an expression, so it resolves against the node's parameters. */ + const gatedParserType = { + description: { + displayName: 'Parser', + name: 'gatedParser', + group: ['transform'], + version: 1, + description: '', + defaults: { name: 'Parser' }, + inputs: + '={{ $parameter.autoFix ? [{ displayName: "Model", type: "ai_languageModel", required: true }] : [] }}', + outputs: ['ai_outputParser'], + properties: [ + { name: 'autoFix', displayName: 'Auto-fix', type: 'boolean', default: true }, + ], + } as unknown as INodeTypeDescription, + } as INodeType; + + /** + * Same shape, but the expression hits a runtime error. Mirrors the + * LangChain Code node, which maps over its `Inputs` collection — empty + * by default, so the map throws on `undefined`. + */ + const brokenParserType = { + description: { + ...gatedParserType.description, + name: 'brokenParser', + inputs: '={{ $parameter.inputs.input.map((i) => ({ type: i.type })) }}', + } as unknown as INodeTypeDescription, + } as INodeType; + + /** Same shape, but the expression cannot be parsed at all. */ + const malformedParserType = { + description: { + ...gatedParserType.description, + name: 'malformedParser', + inputs: '={{ ( }}', + } as unknown as INodeTypeDescription, + } as INodeType; + + beforeEach(() => { + nodeTypes.getByNameAndVersion.mockImplementation((type: string) => { + if (type === 'trigger') return triggerType; + if (type === 'gatedParser') return gatedParserType; + if (type === 'brokenParser') return brokenParserType; + if (type === 'malformedParser') return malformedParserType; + if (type === 'agent') return agentType; + return modelType; + }); + }); + + it('resolves the expression against parameter defaults the caller never set', async () => { + // The node omits `autoFix`; only the type default makes the input + // required. The transient workflow fills defaults in, so the + // expression must see the filled-in value, not the caller's bare node. + const parser = node('Parser', 'gatedParser'); + expect(parser.parameters).toEqual({}); + + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'trigger'), parser, node('Agent', 'agent')], + { + ...startedAt('Agent'), + Parser: { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + expect(result.error).toContain("'Parser'"); + // Filling defaults must not leak back onto the version being saved. + expect(parser.parameters).toEqual({}); + }); + + it('reads a runtime error in the expression the way the engine does', async () => { + // The engine swallows it and runs the node with no inputs, so refusing + // to publish here would block a workflow the runtime is happy with. + // The real case is the LangChain Code node, which maps over its + // `Inputs` collection — empty by default. + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'trigger'), node('Parser', 'brokenParser'), node('Agent', 'agent')], + { + ...startedAt('Agent'), + Parser: { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(true); + }); + + it('reports an input it could not determine instead of treating it as absent', async () => { + // An unparseable expression is a bug in the node type, not a state a + // user can configure, so it is worth surfacing. + const result = await service.validateRequiredInputsConnected( + [node('Trigger', 'trigger'), node('Parser', 'malformedParser'), node('Agent', 'agent')], + { + ...startedAt('Agent'), + Parser: { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + } as unknown as IConnections, + nodeTypes, + ); + + expect(result.isValid).toBe(false); + expect(result.error).toContain('could not be determined'); + }); + }); + + it('does not mutate the connections it was given', async () => { + const connections = { + Model: { + ai_languageModel: [[{ node: 'Parser', type: 'ai_languageModel', index: 0 }]], + }, + } as unknown as IConnections; + const snapshot = deepCopy(connections); + + await service.validateRequiredInputsConnected( + [node('Parser', 'parser'), node('Model', 'model')], + connections, + nodeTypes, + ); + + expect(connections).toEqual(snapshot); + }); + }); }); diff --git a/packages/cli/src/workflows/__tests__/workflow.service.test.ts b/packages/cli/src/workflows/__tests__/workflow.service.test.ts index caaa4df83d3..5f4b26a9a12 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow.service.test.ts @@ -1363,14 +1363,14 @@ describe('WorkflowService', () => { // Bypass validation internals const internals = workflowService as unknown as { _detectWebhookConflicts: () => Promise; - _validateNodes: () => void; + _validateNodes: () => Promise; _validateDynamicCredentials: () => Promise; _validatePublisherCredentialAccess: () => Promise; _validateSubWorkflowReferences: () => Promise; _validateTriggerNodeIds: () => void; }; vi.spyOn(internals, '_detectWebhookConflicts').mockResolvedValue(undefined); - vi.spyOn(internals, '_validateNodes').mockReturnValue(undefined); + vi.spyOn(internals, '_validateNodes').mockResolvedValue(undefined); vi.spyOn(internals, '_validateDynamicCredentials').mockResolvedValue(undefined); vi.spyOn(internals, '_validatePublisherCredentialAccess').mockResolvedValue(undefined); vi.spyOn(internals, '_validateSubWorkflowReferences').mockResolvedValue(undefined); diff --git a/packages/cli/src/workflows/workflow-validation.service.ts b/packages/cli/src/workflows/workflow-validation.service.ts index 062c73021a6..cf1fbb8b0ef 100644 --- a/packages/cli/src/workflows/workflow-validation.service.ts +++ b/packages/cli/src/workflows/workflow-validation.service.ts @@ -7,12 +7,16 @@ import { FULL_ACCESS_NODE_TYPES } from 'n8n-core'; import { validateWorkflowHasTriggerLikeNode, NodeHelpers, + Workflow, mapConnectionsByDestination, validateNodeCredentials, - isNodeConnected, + getUnconnectedRequiredInputs, + getReachableNodeNames, + onlySuppliesDisabledNodes, isTriggerLikeNode, isTriggerNode, classifyTriggerIdentity, + NodeConnectionTypes, } from 'n8n-workflow'; import type { INode, @@ -28,6 +32,7 @@ import { CredentialTypes } from '@/credential-types'; import { DynamicCredentialsProxy } from '@/credentials/dynamic-credentials-proxy'; import { CredentialsPermissionChecker } from '@/executions/pre-execution-checks/credentials-permission-checker'; import type { NodeTypes } from '@/node-types'; +import { withExpressionIsolate } from '@/utils'; export interface WorkflowValidationResult { isValid: boolean; @@ -64,8 +69,32 @@ export class WorkflowValidationService { ) {} /** - * Validates node configuration (credentials, parameters) for connected and enabled nodes. - * Trigger-like nodes are always validated even without connections. + * Names of every node a run can reach. Publishing must not fail over a node + * that never executes, and an edge alone does not make a node reachable: an + * island of wired-up nodes with no path from a trigger never runs. + * + * Every enabled trigger is a start node, a disconnected one included, since + * it still starts the workflow. + */ + private executableNodeNames( + nodes: INode[], + connections: IConnections, + nodeTypes: NodeTypes, + ): Set { + return getReachableNodeNames( + nodes, + connections, + mapConnectionsByDestination(connections), + (node) => { + const nodeType = nodeTypes.getByNameAndVersion(node.type, node.typeVersion); + return !!nodeType && isTriggerLikeNode(nodeType); + }, + ); + } + + /** + * Validates node configuration (credentials, parameters) for every node a run + * can reach. Enabled nodes only. */ private validateNodeConfiguration( nodes: INode[], @@ -73,7 +102,7 @@ export class WorkflowValidationService { nodeTypes: NodeTypes, ): WorkflowValidationResult { try { - const connectionsByDestination = mapConnectionsByDestination(connections); + const executable = this.executableNodeNames(nodes, connections, nodeTypes); const issuesFound: Array<{ nodeName: string; issues: string[] }> = []; for (const node of nodes) { @@ -90,11 +119,10 @@ export class WorkflowValidationService { continue; } - const isNodeTriggerLike = isTriggerLikeNode(nodeType); - - const isConnected = isNodeConnected(node.name, connections, connectionsByDestination); - - if (!isConnected && !isNodeTriggerLike) continue; + // A node no run can reach cannot fail one, so it must not block + // publishing. Being wired to something is not enough: an island of + // connected nodes with no path from a trigger never executes. + if (!executable.has(node.name)) continue; const nodeIssues: string[] = []; const credentialIssues = validateNodeCredentials(node, nodeType); @@ -314,11 +342,11 @@ export class WorkflowValidationService { return []; } - validateForActivation( + async validateForActivation( nodes: INodes, connections: IConnections, nodeTypes: NodeTypes, - ): WorkflowValidationResult { + ): Promise { // Validate workflow entry points: active, poll, webhook, or schedule triggers. const triggerValidation = validateWorkflowHasTriggerLikeNode(nodes, nodeTypes, STARTING_NODES); @@ -339,6 +367,103 @@ export class WorkflowValidationService { return configValidation; } + return await this.validateRequiredInputsConnected(nodesArray, connections, nodeTypes); + } + + /** + * Refuses activation when a required input has nothing connected. The editor + * draws this warning already but nothing enforced it, so such a workflow could + * publish and then throw on every execution. + * + * Called by `validateForActivation`; public so it can be exercised directly. + */ + async validateRequiredInputsConnected( + nodes: INode[], + connections: IConnections, + nodeTypes: NodeTypes, + ): Promise { + // Transient, so dynamic `inputs` expressions can be evaluated. Built over + // shallow node copies because the constructor reassigns `node.parameters` + // with defaults filled in, and these nodes are the version about to be saved. + const workflow = new Workflow({ + nodes: nodes.map((node) => ({ ...node })), + connections, + active: false, + nodeTypes, + }); + const executable = this.executableNodeNames(nodes, connections, nodeTypes); + const issues: string[] = []; + + // Memoized because the consumer walk revisits nodes, and resolving outputs + // can mean evaluating an expression. + const canOutputMainByName = new Map(); + const canOutputMain = (candidate: INode): boolean => { + const cached = canOutputMainByName.get(candidate.name); + if (cached !== undefined) return cached; + + const candidateType = nodeTypes.getByNameAndVersion(candidate.type, candidate.typeVersion); + // Unknown type: assume it belongs to the flow, so nothing opts out quietly. + const result = candidateType?.description + ? NodeHelpers.getNodeOutputs(workflow, candidate, candidateType.description).some( + (output) => + (typeof output === 'string' ? output : output.type) === NodeConnectionTypes.Main, + ) + : true; + + canOutputMainByName.set(candidate.name, result); + return result; + }; + + // Those expressions need an isolate under the VM engine, or they throw. + await withExpressionIsolate(workflow, async () => { + for (const node of nodes) { + if (node.disabled) continue; + + const nodeType = nodeTypes.getByNameAndVersion(node.type, node.typeVersion); + if (!nodeType?.description) continue; + + // Same rule as validateNodeConfiguration: only nodes a run can reach. + if (!executable.has(node.name)) continue; + + // Same reasoning further out: a subnode is only ever resolved by the + // node it supplies, so if every chain of consumers out of it is + // disabled it cannot break a run either. Scoped to nodes with no main + // output, since a node on the main path runs whatever its consumers do. + if ( + !isTriggerLikeNode(nodeType) && + onlySuppliesDisabledNodes(node, connections, nodes, canOutputMain) + ) { + continue; + } + + // Strictly: a swallowed expression error would read as "requires nothing". + let required: ReturnType; + try { + required = getUnconnectedRequiredInputs(workflow, node, nodeType.description, { + throwOnExpressionError: true, + }); + } catch (error) { + issues.push( + `the inputs of '${node.name}' could not be determined (${ensureError(error).message})`, + ); + continue; + } + + for (const input of required) { + issues.push( + `'${node.name}' has no node connected to its required '${input.displayName ?? input.type}' input`, + ); + } + } + }); + + if (issues.length > 0) { + return { + isValid: false, + error: `Workflow cannot be activated because required inputs are not connected: ${issues.join('; ')}.`, + }; + } + return { isValid: true }; } diff --git a/packages/cli/src/workflows/workflow.service.ts b/packages/cli/src/workflows/workflow.service.ts index eb10426e4d3..20e6215f21a 100644 --- a/packages/cli/src/workflows/workflow.service.ts +++ b/packages/cli/src/workflows/workflow.service.ts @@ -1075,7 +1075,7 @@ export class WorkflowService { await this._detectWebhookConflicts(workflow, versionToActivate); - this._validateNodes(workflowId, versionToActivate.nodes, versionToActivate.connections); + await this._validateNodes(workflowId, versionToActivate.nodes, versionToActivate.connections); await this._validateDynamicCredentials(workflowId, versionToActivate.nodes, workflow.settings); if (versionIdToActivate !== previousActiveVersionId) { await this._validatePublisherCredentialAccess(workflowId, user, versionToActivate.nodes); @@ -1858,13 +1858,13 @@ export class WorkflowService { } } - _validateNodes(workflowId: string, nodes: INode[], connections: IConnections) { + async _validateNodes(workflowId: string, nodes: INode[], connections: IConnections) { const nodesToValidate = nodes.reduce((acc, node) => { acc[node.name] = node; return acc; }, {}); - const validation = this.workflowValidationService.validateForActivation( + const validation = await this.workflowValidationService.validateForActivation( nodesToValidate, connections, this.nodeTypes, diff --git a/packages/cli/test/integration/workflows/workflow.service.test.ts b/packages/cli/test/integration/workflows/workflow.service.test.ts index e1f9d6f10f8..5b12fc2ce91 100644 --- a/packages/cli/test/integration/workflows/workflow.service.test.ts +++ b/packages/cli/test/integration/workflows/workflow.service.test.ts @@ -144,7 +144,7 @@ beforeEach(() => { nodeTypes.getByNameAndVersion.mockReset(); workflowValidationService.validateTriggerNodeIds.mockReset(); workflowValidationService.validateTriggerNodeIds.mockReturnValue({ isValid: true }); - workflowValidationService.validateForActivation.mockReturnValue({ isValid: true }); + workflowValidationService.validateForActivation.mockResolvedValue({ isValid: true }); workflowValidationService.validateDynamicCredentials.mockResolvedValue({ isValid: true }); workflowValidationService.validatePublisherCredentialAccess.mockResolvedValue({ isValid: true }); workflowValidationService.validateSubWorkflowReferences.mockResolvedValue({ isValid: true }); @@ -617,7 +617,7 @@ describe('activateWorkflow()', () => { await createWorkflowHistoryItem(workflow.id, { versionId: newVersionId }); // Mock validation to fail - workflowValidationService.validateForActivation.mockReturnValue({ + workflowValidationService.validateForActivation.mockResolvedValue({ isValid: false, error: 'Workflow cannot be activated because it has no trigger node.', }); diff --git a/packages/cli/test/integration/workflows/workflows.controller.test.ts b/packages/cli/test/integration/workflows/workflows.controller.test.ts index ada13dab4b7..f855c894ac3 100644 --- a/packages/cli/test/integration/workflows/workflows.controller.test.ts +++ b/packages/cli/test/integration/workflows/workflows.controller.test.ts @@ -142,7 +142,7 @@ beforeEach(async () => { authMemberAgent = testServer.authAgentFor(member); anotherMember = await createMember(); - workflowValidationService.validateForActivation.mockReturnValue({ isValid: true }); + workflowValidationService.validateForActivation.mockResolvedValue({ isValid: true }); workflowValidationService.validateDynamicCredentials.mockResolvedValue({ isValid: true }); workflowValidationService.validatePublisherCredentialAccess.mockResolvedValue({ isValid: true }); workflowValidationService.validateSubWorkflowReferences.mockResolvedValue({ isValid: true }); diff --git a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue index 50b52ce37d0..4480f1084c8 100644 --- a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue +++ b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue @@ -172,9 +172,9 @@ const containsTrigger = computed((): boolean => { return foundTriggers.value.length > 0; }); -const nodesWithValidationIssues = computed( - () => workflowDocumentStore.value.nodesWithValidationIssues, -); +// The nodes that actually block publishing, so the count in the message matches +// why the button is disabled. +const nodesWithValidationIssues = computed(() => workflowDocumentStore.value.publishBlockingNodes); const hasNodeIssues = computed(() => workflowDocumentStore.value.hasPublishBlockingIssues); diff --git a/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.test.ts b/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.test.ts index f5f71a0c97f..628639a4afb 100644 --- a/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.test.ts +++ b/packages/frontend/editor-ui/src/app/composables/useCanvasOperations.test.ts @@ -276,6 +276,7 @@ describe('useCanvasOperations', () => { vi.mocked(workflowDocumentStoreInstance.getWorkflowObjectAccessorSnapshot).mockReturnValue({ id: workflowDocumentStoreInstance.workflowId, connectionsBySourceNode: workflowDocumentStoreInstance.connectionsBySourceNode, + connectionsByDestinationNode: workflowDocumentStoreInstance.connectionsByDestinationNode, pinData: workflowDocumentStoreInstance.pinnedDataByNodeName as IPinData, expression: workflowDocumentStoreInstance.getExpressionHandler(), getNode: workflowDocumentStoreInstance.getNodeByName, diff --git a/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.test.ts b/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.test.ts index 4ecbfb8d73b..bdad5ca10ef 100644 --- a/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.test.ts +++ b/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.test.ts @@ -17,6 +17,7 @@ import { CUSTOM_API_CALL_KEY, EnterpriseEditionFeature } from '@/app/constants'; import { mockedStore } from '@/__tests__/utils'; import { mock } from 'vitest-mock-extended'; import { faker } from '@faker-js/faker'; +import type { WorkflowObjectAccessors } from '@/app/types/workflow'; import type { INodeUi } from '@/Interface'; import type { IUsedCredential, @@ -49,6 +50,7 @@ const mockDocumentStore = { updateNodeProperties: vi.fn(), getExpressionHandler: vi.fn(() => ({})), getPinDataSnapshot: vi.fn().mockReturnValue({}), + getWorkflowObjectAccessorSnapshot: vi.fn(() => ({}) as unknown as WorkflowObjectAccessors), }; vi.mock('@/app/stores/workflowDocument.store', async () => { @@ -712,6 +714,154 @@ describe('useNodeHelpers()', () => { }); }); + describe('input issues in getNodeIssues', () => { + const nodeTypeWithRequiredInput: INodeTypeDescription = { + displayName: 'Agent', + name: 'agent', + group: ['transform'], + version: 1, + description: 'Agent node', + defaults: { name: 'Agent' }, + inputs: [ + NodeConnectionTypes.Main, + { type: NodeConnectionTypes.AiLanguageModel, displayName: 'Model', required: true }, + ], + outputs: [NodeConnectionTypes.Main], + properties: [], + }; + + it('reports a required input with nothing connected', () => { + const node = createTestNode({ name: 'Agent', type: 'agent' }); + const workflow = { + getNode: () => node, + connectionsByDestinationNode: {}, + } as unknown as Workflow; + + const { getNodeIssues } = useNodeHelpers(); + const result = getNodeIssues(nodeTypeWithRequiredInput, node, workflow, [ + 'typeUnknown', + 'parameters', + 'credentials', + 'execution', + ]); + + expect(result?.input?.[NodeConnectionTypes.AiLanguageModel]).toBeDefined(); + }); + + it('reports both unconnected inputs when they share a connection type', () => { + // An agent with a fallback declares Chat Model and Fallback Model, both + // `ai_languageModel`. Keyed by type, so one must not hide the other. + const node = createTestNode({ name: 'Agent', type: 'agent' }); + const workflow = { + getNode: () => node, + connectionsByDestinationNode: {}, + } as unknown as Workflow; + + vi.spyOn(NodeHelpers, 'getNodeInputs').mockReturnValue([ + { type: NodeConnectionTypes.AiLanguageModel, displayName: 'Chat Model', required: true }, + { + type: NodeConnectionTypes.AiLanguageModel, + displayName: 'Fallback Model', + required: true, + }, + ]); + + const { getNodeIssues } = useNodeHelpers(); + const result = getNodeIssues(nodeTypeWithRequiredInput, node, workflow, [ + 'typeUnknown', + 'parameters', + 'credentials', + 'execution', + ]); + + expect(result?.input?.[NodeConnectionTypes.AiLanguageModel]).toHaveLength(2); + }); + + it('records the input issue when a parameter change makes an input required', () => { + // The NDV calls this after a parameter change, so a newly required input + // shows its warning without a workflow reload. + const node = createTestNode({ name: 'Agent', type: 'agent' }); + const gatedType: INodeTypeDescription = { + ...nodeTypeWithRequiredInput, + inputs: `={{ $parameter.hasOutputParser ? [{ type: "${NodeConnectionTypes.AiLanguageModel}", displayName: "Model", required: true }] : [] }}`, + }; + + mockDocumentStore.getNodeByName = vi.fn().mockReturnValue(node); + mockDocumentStore.setNodeIssue = vi.fn(); + mockDocumentStore.getWorkflowObjectAccessorSnapshot = vi.fn().mockReturnValue({ + getNode: () => node, + connectionsByDestinationNode: {}, + }); + mockedStore(useNodeTypesStore).getNodeType = vi.fn().mockReturnValue(gatedType); + vi.spyOn(NodeHelpers, 'getNodeInputs').mockReturnValue([ + { type: NodeConnectionTypes.AiLanguageModel, displayName: 'Model', required: true }, + ]); + + const { updateNodeInputIssuesByName } = useNodeHelpers(); + updateNodeInputIssuesByName('Agent'); + + expect(mockDocumentStore.setNodeIssue).toHaveBeenCalledWith({ + node: 'Agent', + type: 'input', + value: expect.objectContaining({ + [NodeConnectionTypes.AiLanguageModel]: expect.anything(), + }), + }); + }); + + it('clears the input issue once the required input is satisfied', () => { + const node = createTestNode({ name: 'Agent', type: 'agent' }); + + mockDocumentStore.getNodeByName = vi.fn().mockReturnValue(node); + mockDocumentStore.setNodeIssue = vi.fn(); + mockDocumentStore.getWorkflowObjectAccessorSnapshot = vi.fn().mockReturnValue({ + getNode: (name: string) => + name === 'Model' ? createTestNode({ name: 'Model', type: 'model' }) : node, + connectionsByDestinationNode: { + Agent: { + [NodeConnectionTypes.AiLanguageModel]: [ + [{ node: 'Model', type: NodeConnectionTypes.AiLanguageModel, index: 0 }], + ], + }, + }, + }); + mockedStore(useNodeTypesStore).getNodeType = vi + .fn() + .mockReturnValue(nodeTypeWithRequiredInput); + vi.spyOn(NodeHelpers, 'getNodeInputs').mockReturnValue([ + { type: NodeConnectionTypes.AiLanguageModel, displayName: 'Model', required: true }, + ]); + + const { updateNodeInputIssuesByName } = useNodeHelpers(); + updateNodeInputIssuesByName('Agent'); + + expect(mockDocumentStore.setNodeIssue).toHaveBeenCalledWith({ + node: 'Agent', + type: 'input', + value: null, + }); + }); + + it('skips the input check when input issues are ignored', () => { + // The tool-config panel passes an accessor built from getNode alone and + // opts out of input issues. The shared check reads the connection map, + // so it must not run against such a partial accessor. + const node = createTestNode({ name: 'Agent', type: 'agent' }); + const partialAccessor = { getNode: () => node } as unknown as Workflow; + + const { getNodeIssues } = useNodeHelpers(); + const result = getNodeIssues(nodeTypeWithRequiredInput, node, partialAccessor, [ + 'typeUnknown', + 'parameters', + 'credentials', + 'execution', + 'input', + ]); + + expect(result?.input).toBeUndefined(); + }); + }); + describe('credential issues with AI Gateway', () => { const nodeTypeWithCreds: INodeTypeDescription = { displayName: 'Google AI', diff --git a/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.ts b/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.ts index 814f72c15f8..1891639ddd7 100644 --- a/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.ts +++ b/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.ts @@ -10,6 +10,7 @@ import { getChildNodes, getParentNodes, nodeIssuesToString, + getUnconnectedRequiredInputs, } from 'n8n-workflow'; import type { INodeProperties, @@ -18,7 +19,6 @@ import type { INodeIssues, ICredentialType, INodeIssueObjectProperty, - INodeInputConfiguration, INodeExecutionData, ITaskDataConnections, IBinaryKeyData, @@ -227,11 +227,16 @@ export function useNodeHelpers() { } } - const nodeInputIssues = getNodeInputIssues(workflow, node, nodeType); - if (nodeIssues === null) { - nodeIssues = nodeInputIssues; - } else { - NodeHelpers.mergeIssues(nodeIssues, nodeInputIssues); + // Honoured like the other kinds: the tool-config panel passes a partial + // accessor (getNode only) and opts out of input issues, so the shared + // check must not run against it. + if (!ignoreIssues.includes('input')) { + const nodeInputIssues = getNodeInputIssues(workflow, node, nodeType); + if (nodeIssues === null) { + nodeIssues = nodeInputIssues; + } else { + NodeHelpers.mergeIssues(nodeIssues, nodeInputIssues); + } } } @@ -297,6 +302,14 @@ export function useNodeHelpers() { }); } + function updateNodeInputIssuesByName(name: string): void { + const node = workflowDocumentStore.value.getNodeByName(name) ?? null; + + if (node) { + updateNodeInputIssues(node); + } + } + function updateNodesInputIssues() { const nodes = workflowDocumentStore.value.allNodes; @@ -391,25 +404,22 @@ export function useNodeHelpers() { const foundIssues: INodeIssueObjectProperty = {}; const workflowNode = workflow.getNode(node.name); - let inputs: Array = []; - if (nodeType && workflowNode) { - inputs = NodeHelpers.getNodeInputs(workflow, workflowNode, nodeType); - } + // Detection is shared with the backend publish check; only wording is local. + const unconnected = + nodeType && workflowNode + ? getUnconnectedRequiredInputs(workflow, workflowNode, nodeType) + : []; - inputs.forEach((input) => { - if (typeof input === 'string' || input.required !== true) { - return; - } - - const parentNodes = workflow.getParentNodes(node.name, input.type, 1); - - if (parentNodes.length === 0) { - foundIssues[input.type] = [ - i18n.baseText('nodeIssues.input.missing', { - interpolate: { inputName: input.displayName || input.type }, - }), - ]; - } + unconnected.forEach((input) => { + // Accumulate: a node can leave two inputs of one type unconnected, e.g. + // an agent's Chat Model and Fallback Model are both `ai_languageModel`. + // Assigning would drop all but the last. + foundIssues[input.type] = [ + ...(foundIssues[input.type] ?? []), + i18n.baseText('nodeIssues.input.missing', { + interpolate: { inputName: input.displayName || input.type }, + }), + ]; }); if (Object.keys(foundIssues).length) { @@ -1096,6 +1106,7 @@ export function useNodeHelpers() { updateNodesExecutionIssues, updateNodesParameterIssues, updateNodeInputIssues, + updateNodeInputIssuesByName, updateNodeCredentialIssuesByName, updateNodeCredentialIssues, updateNodeParameterIssuesByName, diff --git a/packages/frontend/editor-ui/src/app/composables/useRunWorkflow.test.ts b/packages/frontend/editor-ui/src/app/composables/useRunWorkflow.test.ts index 44eb861a47e..7b6058b81ce 100644 --- a/packages/frontend/editor-ui/src/app/composables/useRunWorkflow.test.ts +++ b/packages/frontend/editor-ui/src/app/composables/useRunWorkflow.test.ts @@ -72,7 +72,6 @@ const { mockDocumentStore } = vi.hoisted(() => { getPinnedDataLastUpdate: vi.fn(), getPinnedDataLastRemovedAt: vi.fn(), getWorkflowObjectAccessorSnapshot: vi.fn(), - hasNodeValidationIssues: false, nodeValidationIssues: [], serialize: vi.fn(), } as Partial>>> as Mocked< @@ -363,15 +362,14 @@ describe('useRunWorkflow({ router })', () => { it('should not prevent running a webhook-based workflow that has issues', async () => { const { runWorkflowApi } = useRunWorkflow({ router }); - mockDocumentStore.hasNodeValidationIssues = true; + // Nothing here gates a run on the document store's issue flags, so there is + // no flag to set. The point stands: a webhook-based run still resolves. vi.mocked(workflowsStore).runWorkflow.mockResolvedValue({ executionId: '123', waitingForWebhook: true, }); await expect(runWorkflowApi({} as IStartRunData)).resolves.not.toThrow(); - - mockDocumentStore.hasNodeValidationIssues = false; }); it('should handle workflow run failure', async () => { @@ -415,7 +413,6 @@ describe('useRunWorkflow({ router })', () => { vi.mocked(uiStore).activeActions = ['']; vi.mocked(workflowsStore).runWorkflow.mockResolvedValue(mockExecutionResponse); - mockDocumentStore.hasNodeValidationIssues = true; mockDocumentStore.serialize.mockReturnValue({ id: 'workflowId', nodes: [], @@ -432,7 +429,6 @@ describe('useRunWorkflow({ router })', () => { vi.mocked(pushConnectionStore).isConnected = true; vi.mocked(workflowsStore).runWorkflow.mockResolvedValue(mockExecutionResponse); - mockDocumentStore.hasNodeValidationIssues = false; mockDocumentStore.serialize.mockReturnValue({ id: 'workflowId', nodes: [], @@ -679,7 +675,6 @@ describe('useRunWorkflow({ router })', () => { vi.mocked(pushConnectionStore).isConnected = true; vi.mocked(workflowsStore).runWorkflow.mockResolvedValue(mockExecutionResponse); - mockDocumentStore.hasNodeValidationIssues = false; mockDocumentStore.serialize.mockReturnValue({ id: 'workflowId', nodes: [], @@ -919,7 +914,6 @@ describe('useRunWorkflow({ router })', () => { vi.mocked(pushConnectionStore).isConnected = true; vi.mocked(workflowsStore).runWorkflow.mockResolvedValue(mockExecutionResponse); - mockDocumentStore.hasNodeValidationIssues = false; mockDocumentStore.serialize.mockReturnValue(workflowData); seedActiveRunData(mockRunData); vi.mocked(agentRequestStore).getAgentRequest.mockReturnValue(agentRequest); @@ -980,7 +974,6 @@ describe('useRunWorkflow({ router })', () => { vi.mocked(pushConnectionStore).isConnected = true; vi.mocked(workflowsStore).runWorkflow.mockResolvedValue({ executionId: '123' }); - mockDocumentStore.hasNodeValidationIssues = false; mockDocumentStore.serialize.mockReturnValue({ id: 'workflowId', nodes: [createTestNode({ id: currentNodeId, name: 'Test node' })], @@ -1024,7 +1017,6 @@ describe('useRunWorkflow({ router })', () => { vi.mocked(pushConnectionStore).isConnected = true; vi.mocked(workflowsStore).runWorkflow.mockResolvedValue(mockExecutionResponse); - mockDocumentStore.hasNodeValidationIssues = false; mockDocumentStore.serialize.mockReturnValue( mock({ id: 'workflowId', nodes: [] }), ); @@ -1058,7 +1050,6 @@ describe('useRunWorkflow({ router })', () => { vi.mocked(pushConnectionStore).isConnected = true; vi.mocked(workflowsStore).runWorkflow.mockResolvedValue(mockExecutionResponse); - mockDocumentStore.hasNodeValidationIssues = false; mockDocumentStore.serialize.mockReturnValue( mock({ id: 'workflowId', nodes: [] }), ); @@ -1113,7 +1104,6 @@ describe('useRunWorkflow({ router })', () => { beforeEach(() => { vi.mocked(pushConnectionStore).isConnected = true; vi.mocked(workflowsStore).runWorkflow.mockResolvedValue({ executionId: 'exec-123' }); - mockDocumentStore.hasNodeValidationIssues = false; mockDocumentStore.checkIfNodeHasChatParent.mockReturnValue(false); mockDocumentStore.checkIfToolNodeHasChatParent.mockReturnValue(false); }); @@ -1173,7 +1163,6 @@ describe('useRunWorkflow({ router })', () => { beforeEach(() => { vi.mocked(pushConnectionStore).isConnected = true; vi.mocked(workflowsStore).runWorkflow.mockResolvedValue(mockExecutionResponse); - mockDocumentStore.hasNodeValidationIssues = false; seedActiveRunData({ NodeName: [] }); }); diff --git a/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.test.ts b/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.test.ts index cdd4b9f1ef6..a987f841e44 100644 --- a/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.test.ts +++ b/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.test.ts @@ -28,13 +28,17 @@ import type { INodeUi, IWorkflowDb } from '@/Interface'; import type { ProjectSharingData } from '@/features/collaboration/projects/projects.types'; import type { IUsedCredential } from '@/features/credentials/credentials.types'; -const { getNodeTypeMock } = vi.hoisted(() => ({ +const { getNodeTypeMock, isTriggerNodeMock } = vi.hoisted(() => ({ getNodeTypeMock: vi.fn().mockReturnValue(null), + // The publish gate asks which nodes start a run. Anything named like a trigger + // counts, which keeps these tests readable without a node-type registry. + isTriggerNodeMock: vi.fn((type: string) => type.toLowerCase().includes('trigger')), })); vi.mock('@/app/stores/nodeTypes.store', () => ({ useNodeTypesStore: vi.fn(() => ({ getNodeType: getNodeTypeMock, + isTriggerNode: isTriggerNodeMock, communityNodeType: vi.fn().mockReturnValue(null), getAllNodeTypes: vi.fn().mockReturnValue({ nodeTypes: {}, @@ -205,8 +209,8 @@ describe('workflowDocument.store orchestration', () => { }); }); - describe('hasNodeValidationIssues', () => { - it('should return true when a node has issues and connected', () => { + describe('nodeValidationIssues', () => { + it('should report a node that has issues and is connected', () => { const workflowDocumentStore = useWorkflowDocumentStore(createWorkflowDocumentId('test-wf')); workflowDocumentStore.setNodes([ @@ -218,11 +222,10 @@ describe('workflowDocument.store orchestration', () => { Node1: { main: [[{ node: 'Node2', type: NodeConnectionTypes.Main, index: 0 }]] }, }); - const hasIssues = workflowDocumentStore.hasNodeValidationIssues; - expect(hasIssues).toBe(true); + expect(workflowDocumentStore.nodeValidationIssues.length).toBeGreaterThan(0); }); - it('should return false when node has issues but it is not connected', () => { + it('should report nothing when the node with issues is not connected', () => { const workflowDocumentStore = useWorkflowDocumentStore(createWorkflowDocumentId('test-wf')); workflowDocumentStore.setNodes([ @@ -230,11 +233,10 @@ describe('workflowDocument.store orchestration', () => { createNode({ name: 'Node2' }), ]); - const hasIssues = workflowDocumentStore.hasNodeValidationIssues; - expect(hasIssues).toBe(false); + expect(workflowDocumentStore.nodeValidationIssues).toEqual([]); }); - it('should return false when no nodes have issues', () => { + it('should report nothing when no nodes have issues', () => { const workflowDocumentStore = useWorkflowDocumentStore(createWorkflowDocumentId('test-wf')); workflowDocumentStore.setNodes([ @@ -245,17 +247,50 @@ describe('workflowDocument.store orchestration', () => { Node1: { main: [[{ node: 'Node2', type: NodeConnectionTypes.Main, index: 0 }]] }, }); - const hasIssues = workflowDocumentStore.hasNodeValidationIssues; - expect(hasIssues).toBe(false); + expect(workflowDocumentStore.nodeValidationIssues).toEqual([]); }); - it('should return false when there are no nodes', () => { + it('should report nothing when there are no nodes', () => { const workflowDocumentStore = useWorkflowDocumentStore(createWorkflowDocumentId('test-wf')); workflowDocumentStore.setNodes([]); - const hasIssues = workflowDocumentStore.hasNodeValidationIssues; - expect(hasIssues).toBe(false); + expect(workflowDocumentStore.nodeValidationIssues).toEqual([]); + }); + }); + + describe('hasPublishBlockingIssues', () => { + it('should block when a trigger reaches the node with issues', () => { + const workflowDocumentStore = useWorkflowDocumentStore(createWorkflowDocumentId('test-wf')); + + workflowDocumentStore.setNodes([ + createNode({ name: 'My trigger', type: 'n8n-nodes-base.scheduleTrigger' }), + createNode({ name: 'Node1', issues: { parameters: { field: ['Error message'] } } }), + ]); + workflowDocumentStore.setConnections({ + 'My trigger': { main: [[{ node: 'Node1', type: NodeConnectionTypes.Main, index: 0 }]] }, + }); + + expect(workflowDocumentStore.hasPublishBlockingIssues).toBe(true); + }); + + it('should not block when the node with issues sits on an island', () => { + const workflowDocumentStore = useWorkflowDocumentStore(createWorkflowDocumentId('test-wf')); + + // Node1 and Node2 are wired to each other, so connectedness would call + // Node1 live. No trigger reaches them, so no run can involve them. + workflowDocumentStore.setNodes([ + createNode({ name: 'My trigger', type: 'n8n-nodes-base.scheduleTrigger' }), + createNode({ name: 'Reached' }), + createNode({ name: 'Node1', issues: { parameters: { field: ['Error message'] } } }), + createNode({ name: 'Node2' }), + ]); + workflowDocumentStore.setConnections({ + 'My trigger': { main: [[{ node: 'Reached', type: NodeConnectionTypes.Main, index: 0 }]] }, + Node1: { main: [[{ node: 'Node2', type: NodeConnectionTypes.Main, index: 0 }]] }, + }); + + expect(workflowDocumentStore.hasPublishBlockingIssues).toBe(false); }); }); diff --git a/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.ts b/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.ts index 05bb33e75d2..f5aa0e73b22 100644 --- a/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.ts +++ b/packages/frontend/editor-ui/src/app/stores/workflowDocument.store.ts @@ -39,7 +39,7 @@ import { assignNodeId, serializeNode } from '@/app/utils/nodes/nodeTransforms'; import type { WorkflowObjectAccessors } from '../types'; import type { IWorkflowDb } from '@/Interface'; import type { INode, ProjectSharingData } from 'n8n-workflow'; -import { deepCopy, nodeIssuesToString } from 'n8n-workflow'; +import { deepCopy, nodeIssuesToString, NodeConnectionTypes, NodeHelpers } from 'n8n-workflow'; import type { WorkflowData } from '@n8n/rest-api-client/api/workflows'; import type { Scope } from '@n8n/permissions'; import type { IUsedCredential } from '@/features/credentials/credentials.types'; @@ -251,6 +251,32 @@ export function useWorkflowDocumentStore(id: WorkflowDocumentId) { allNodes: workflowDocumentNodes.allNodes, outgoingConnectionsByNodeName: workflowDocumentConnections.outgoingConnectionsByNodeName, incomingConnectionsByNodeName: workflowDocumentConnections.incomingConnectionsByNodeName, + connectionsBySourceNode: workflowDocumentConnections.connectionsBySourceNode, + connectionsByDestinationNode: workflowDocumentConnections.connectionsByDestinationNode, + // The editor only has the node description, so it reads the `trigger` group + // where the server reads the type's trigger/webhook/poll implementation. + isTriggerLike: (node) => nodeTypesStore.isTriggerNode(node.type), + canOutputMain: (node) => { + const nodeType = nodeTypesStore.getNodeType(node.type, node.typeVersion); + // Unknown type answers "yes", so a half-built graph is never written off + // as unable to affect a run. + if (!nodeType) return true; + try { + // Evaluated, not read off the description: several vector stores build + // `outputs` from an expression, and reading the raw expression would + // call them main-path nodes. The server evaluates it the same way. + return NodeHelpers.getNodeOutputs( + { expression: workflowDocumentExpression.getExpressionHandler() }, + node, + nodeType, + ).some( + (output) => + (typeof output === 'string' ? output : output.type) === NodeConnectionTypes.Main, + ); + } catch { + return true; + } + }, nodesById: workflowDocumentNodes.nodesById, onNodesChange: workflowDocumentNodes.onNodesChange, nodeIssuesToString, @@ -418,6 +444,8 @@ export function useWorkflowDocumentStore(id: WorkflowDocumentId) { return { id: workflowId, connectionsBySourceNode: workflowDocumentConnections.connectionsBySourceNode.value, + connectionsByDestinationNode: + workflowDocumentConnections.connectionsByDestinationNode.value, pinData: workflowDocumentPinData.getPinDataSnapshot(), expression: workflowDocumentExpression.getExpressionHandler(), getNode: workflowDocumentNodes.getNodeByName, diff --git a/packages/frontend/editor-ui/src/app/stores/workflowDocument/useWorkflowDocumentNodesIssues.test.ts b/packages/frontend/editor-ui/src/app/stores/workflowDocument/useWorkflowDocumentNodesIssues.test.ts index 9a83d97303c..a861145ba47 100644 --- a/packages/frontend/editor-ui/src/app/stores/workflowDocument/useWorkflowDocumentNodesIssues.test.ts +++ b/packages/frontend/editor-ui/src/app/stores/workflowDocument/useWorkflowDocumentNodesIssues.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect } from 'vitest'; import { computed, shallowRef } from 'vue'; import { createTestNode } from '@/__tests__/mocks'; import type { INodeUi } from '@/Interface'; -import type { INodeConnections } from 'n8n-workflow'; +import type { IConnections, INodeConnections } from 'n8n-workflow'; import { useWorkflowDocumentNodesIssues, type WorkflowDocumentNodesIssuesDeps, @@ -11,38 +11,100 @@ import { const connectedNode = (overrides: Partial = {}): INodeUi => createTestNode({ name: 'Node', ...overrides }) as INodeUi; +/** `createTestNode` takes a `Partial`, which has no `issues`. */ +const nodeWithIssues = (name: string, issues: INodeUi['issues']): INodeUi => + createTestNode({ name, ...{ issues } }) as INodeUi; + const hasConnections: () => INodeConnections = () => ({ main: [[{ node: 'Other', type: 'main' as const, index: 0 }]], }); const noConnections: () => INodeConnections = () => ({}); +/** What the canvas leaves behind after the last link of a type is deleted. */ +const emptyConnectionEntry: () => INodeConnections = () => ({ ai_outputParser: [[]] }); +/** And the same with the output slot nulled out rather than emptied. */ +const nulledConnectionEntry: () => INodeConnections = () => ({ ai_outputParser: [null] }); -function createDeps(nodes: INodeUi[], connected = true): WorkflowDocumentNodesIssuesDeps { +type DepOverrides = Partial; + +function createDeps( + nodes: INodeUi[], + connected = true, + overrides: DepOverrides = {}, +): WorkflowDocumentNodesIssuesDeps { return { allNodes: computed(() => nodes), outgoingConnectionsByNodeName: connected ? hasConnections : noConnections, incomingConnectionsByNodeName: noConnections, + connectionsBySourceNode: computed(() => ({})), + connectionsByDestinationNode: computed(() => ({})), + // Default: everything is a trigger, so every node is reachable and the + // per-issue-kind rules are what the assertions actually exercise. + isTriggerLike: () => true, + canOutputMain: () => true, nodesById: shallowRef(new Map(nodes.map((n) => [n.id, n]))), onNodesChange: () => {}, nodeIssuesToString: () => [], + ...overrides, + }; +} + +/** Trigger -> Agent over `main`, with `subNode` supplying the agent over `ai_outputParser`. */ +function subNodeGraph({ agentDisabled = false } = {}) { + const trigger = createTestNode({ name: 'Trigger', type: 'n8n-nodes-base.scheduleTrigger' }); + const agent = createTestNode({ + name: 'Agent', + type: '@n8n/n8n-nodes-langchain.agent', + disabled: agentDisabled, + }); + const subNode = nodeWithIssues('Parser', { + input: { ai_languageModel: ['Model input is not connected'] }, + }); + + const connectionsBySourceNode: IConnections = { + Trigger: { main: [[{ node: 'Agent', type: 'main', index: 0 }]] }, + Parser: { ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]] }, + }; + const connectionsByDestinationNode: IConnections = { + Agent: { + main: [[{ node: 'Trigger', type: 'main', index: 0 }]], + ai_outputParser: [[{ node: 'Parser', type: 'ai_outputParser', index: 0 }]], + }, + }; + + return { + nodes: [trigger, agent, subNode] as INodeUi[], + overrides: { + connectionsBySourceNode: computed(() => connectionsBySourceNode), + connectionsByDestinationNode: computed(() => connectionsByDestinationNode), + isTriggerLike: (node: INodeUi) => node.name === 'Trigger', + // Only the agent sits on the main path; the parser just supplies it. + canOutputMain: (node: INodeUi) => node.name !== 'Parser', + } satisfies DepOverrides, }; } describe('useWorkflowDocumentNodesIssues', () => { - describe('hasNodeValidationIssues', () => { - it('includes execution issues', () => { - const node = connectedNode({ issues: { execution: true } }); - const { hasNodeValidationIssues } = useWorkflowDocumentNodesIssues(createDeps([node])); + describe('nodeValidationIssues', () => { + // Feeds the assistant's build todos, which still ask whether a node is wired + // to anything rather than whether a run can reach it. + it('includes a connected node with parameter issues', () => { + const node = connectedNode({ issues: { parameters: { param1: ['Missing value'] } } }); + const { nodeValidationIssues } = useWorkflowDocumentNodesIssues(createDeps([node])); - expect(hasNodeValidationIssues.value).toBe(true); + expect(nodeValidationIssues.value.length).toBeGreaterThan(0); }); - it('includes parameter issues', () => { - const node = connectedNode({ - issues: { parameters: { param1: ['Missing value'] } }, + it.each([ + ['an output slot with no endpoints', emptyConnectionEntry], + ['an output slot that is null', nulledConnectionEntry], + ])('skips a node left with %s', (_label, connections) => { + const node = connectedNode({ issues: { parameters: { param1: ['Missing value'] } } }); + const { nodeValidationIssues } = useWorkflowDocumentNodesIssues({ + ...createDeps([node]), + outgoingConnectionsByNodeName: connections, }); - const { hasNodeValidationIssues } = useWorkflowDocumentNodesIssues(createDeps([node])); - expect(hasNodeValidationIssues.value).toBe(true); + expect(nodeValidationIssues.value).toEqual([]); }); }); @@ -80,5 +142,81 @@ describe('useWorkflowDocumentNodesIssues', () => { expect(hasPublishBlockingIssues.value).toBe(true); }); + + it('does not block on a node no trigger can reach', () => { + // Wired to another node, so connectedness would call it live, but there is + // no trigger anywhere in the graph. + const island = nodeWithIssues('Island Parser', { + input: { ai_languageModel: ['Model input is not connected'] }, + }); + const neighbour = createTestNode({ name: 'Island Agent' }) as INodeUi; + + const connections: IConnections = { + 'Island Parser': { + ai_outputParser: [[{ node: 'Island Agent', type: 'ai_outputParser', index: 0 }]], + }, + }; + + const { hasPublishBlockingIssues } = useWorkflowDocumentNodesIssues( + createDeps([island, neighbour], true, { + connectionsBySourceNode: computed(() => connections), + connectionsByDestinationNode: computed(() => ({ + 'Island Agent': { + ai_outputParser: [[{ node: 'Island Parser', type: 'ai_outputParser', index: 0 }]], + }, + })), + isTriggerLike: () => false, + }), + ); + + expect(hasPublishBlockingIssues.value).toBe(false); + }); + + it('blocks on an unmet input when the node it supplies runs', () => { + const { nodes, overrides } = subNodeGraph(); + const { hasPublishBlockingIssues } = useWorkflowDocumentNodesIssues( + createDeps(nodes, true, overrides), + ); + + expect(hasPublishBlockingIssues.value).toBe(true); + }); + + it('does not block on an unmet input when the only node it supplies is disabled', () => { + const { nodes, overrides } = subNodeGraph({ agentDisabled: true }); + const { hasPublishBlockingIssues } = useWorkflowDocumentNodesIssues( + createDeps(nodes, true, overrides), + ); + + expect(hasPublishBlockingIssues.value).toBe(false); + }); + + it('still blocks on a credential issue behind a disabled consumer', () => { + // The inert-subnode exemption is for unmet inputs only. A credential is the + // node's own problem, and the server blocks on it, so the button must too. + const { nodes, overrides } = subNodeGraph({ agentDisabled: true }); + const parser = nodes.find((node) => node.name === 'Parser') as INodeUi; + parser.issues = { credentials: { cred1: ['Not set'] } }; + + const { hasPublishBlockingIssues } = useWorkflowDocumentNodesIssues( + createDeps(nodes, true, overrides), + ); + + expect(hasPublishBlockingIssues.value).toBe(true); + }); + }); + + describe('publishBlockingNodes', () => { + it('lists only the nodes that block, so the count matches the button', () => { + const { nodes, overrides } = subNodeGraph(); + const unreachable = nodeWithIssues('Lone Parser', { + input: { ai_languageModel: ['Model input is not connected'] }, + }); + + const { publishBlockingNodes } = useWorkflowDocumentNodesIssues( + createDeps([...nodes, unreachable], true, overrides), + ); + + expect(publishBlockingNodes.value.map((node) => node.name)).toEqual(['Parser']); + }); }); }); diff --git a/packages/frontend/editor-ui/src/app/stores/workflowDocument/useWorkflowDocumentNodesIssues.ts b/packages/frontend/editor-ui/src/app/stores/workflowDocument/useWorkflowDocumentNodesIssues.ts index 9aa9ff9cdfb..7a3589b5203 100644 --- a/packages/frontend/editor-ui/src/app/stores/workflowDocument/useWorkflowDocumentNodesIssues.ts +++ b/packages/frontend/editor-ui/src/app/stores/workflowDocument/useWorkflowDocumentNodesIssues.ts @@ -2,7 +2,8 @@ import { computed, effectScope, shallowReactive, type ComputedRef, type ShallowR import { structuralComputed } from '@n8n/composables/structuralComputed'; import isEqual from 'lodash/isEqual'; import type { INodeUi, WorkflowValidationIssue } from '@/Interface'; -import type { INodeConnections, INodeIssues, INode } from 'n8n-workflow'; +import type { IConnections, INodeConnections, INodeIssues, INode } from 'n8n-workflow'; +import { getReachableNodeNames, onlySuppliesDisabledNodes } from 'n8n-workflow'; import { CHANGE_ACTION } from './types'; import type { NodeAddedPayload, @@ -15,41 +16,81 @@ export type WorkflowDocumentNodesIssuesDeps = { allNodes: ComputedRef; outgoingConnectionsByNodeName: (nodeName: string) => INodeConnections; incomingConnectionsByNodeName: (nodeName: string) => INodeConnections; + connectionsBySourceNode: ComputedRef; + connectionsByDestinationNode: ComputedRef; + /** Whether this node starts a run. */ + isTriggerLike: (node: INodeUi) => boolean; + /** Whether this node can produce a `main` output. Answers `true` when unsure. */ + canOutputMain: (node: INodeUi) => boolean; nodesById: ShallowRef>; onNodesChange: (cb: (event: NodesChangeEvent) => void) => void; nodeIssuesToString: (issues: INodeIssues, node?: INode) => string[]; }; +/** + * Whether anything is actually wired to this node. + * + * A connection type can outlive its links: deleting the last connection leaves + * `{ ai_outputParser: [[]] }` behind, so counting the type keys reports a node + * wired to nothing as connected. Count the endpoints instead. + */ +function hasAnyConnection(connections: INodeConnections): boolean { + return Object.values(connections).some((outputs) => + (outputs ?? []).some((targets) => (targets ?? []).length > 0), + ); +} + export function useWorkflowDocumentNodesIssues(deps: WorkflowDocumentNodesIssuesDeps) { - const nodesWithValidationIssues = computed(() => - deps.allNodes.value.filter((node) => { - const nodeHasIssues = Object.keys(node.issues ?? {}).length > 0; - const isConnected = - Object.keys(deps.outgoingConnectionsByNodeName(node.name)).length > 0 || - Object.keys(deps.incomingConnectionsByNodeName(node.name)).length > 0; + /** + * Nodes that stop the workflow being published, which is also what the + * blocked-publish message counts, so the button and its reason always agree. + * + * Mirrors the server (`workflow-validation.service.ts`): a node only blocks if + * a trigger can reach it, and an unmet input only blocks if something that + * runs will ask for it. A node no run can reach cannot break one, so it must + * not stop publishing. It keeps its own warning on the canvas either way, + * which is what tells the user it is still unfinished. + */ + const publishBlockingNodes = computed(() => { + const nodes = deps.allNodes.value; + const reachable = getReachableNodeNames( + nodes, + deps.connectionsBySourceNode.value, + deps.connectionsByDestinationNode.value, + deps.isTriggerLike, + ); - return !node.disabled && isConnected && nodeHasIssues; - }), - ); + return nodes.filter((node) => { + if (node.disabled) return false; - const nodesWithValidationIssuesCount = computed(() => nodesWithValidationIssues.value.length); - - const hasNodeValidationIssues = computed(() => nodesWithValidationIssuesCount.value > 0); - - /** Whether any connected node has issues that should block publishing. - * Execution issues are excluded — they are runtime errors, not configuration problems. */ - const hasPublishBlockingIssues = computed(() => - deps.allNodes.value.some((node) => { + // Execution issues are runtime errors, not configuration problems. const { execution: _, ...configIssues } = node.issues ?? {}; - if (Object.keys(configIssues).length === 0) return false; + const issueKinds = Object.keys(configIssues); + if (issueKinds.length === 0) return false; - const isConnected = - Object.keys(deps.outgoingConnectionsByNodeName(node.name)).length > 0 || - Object.keys(deps.incomingConnectionsByNodeName(node.name)).length > 0; + if (!reachable.has(node.name)) return false; - return !node.disabled && isConnected; - }), - ); + // An unmet input only matters once something asks for it. Credential and + // parameter issues belong to the node itself, so those block as soon as a + // run can reach it. + if ( + issueKinds.every((kind) => kind === 'input') && + !deps.isTriggerLike(node) && + onlySuppliesDisabledNodes( + node, + deps.connectionsBySourceNode.value, + nodes, + deps.canOutputMain, + ) + ) { + return false; + } + + return true; + }); + }); + + const hasPublishBlockingIssues = computed(() => publishBlockingNodes.value.length > 0); const nodeValidationIssues = computed(() => { const issues: WorkflowValidationIssue[] = []; @@ -61,8 +102,8 @@ export function useWorkflowDocumentNodesIssues(deps: WorkflowDocumentNodesIssues if (!node.issues || node.disabled) return; const isConnected = - Object.keys(deps.outgoingConnectionsByNodeName(node.name)).length > 0 || - Object.keys(deps.incomingConnectionsByNodeName(node.name)).length > 0; + hasAnyConnection(deps.outgoingConnectionsByNodeName(node.name)) || + hasAnyConnection(deps.incomingConnectionsByNodeName(node.name)); if (!isConnected) return; @@ -164,9 +205,7 @@ export function useWorkflowDocumentNodesIssues(deps: WorkflowDocumentNodesIssues applyReconcileEntries(Array.from(deps.nodesById.value.keys())); return { - nodesWithValidationIssues, - nodesWithValidationIssuesCount, - hasNodeValidationIssues, + publishBlockingNodes, hasPublishBlockingIssues, nodeValidationIssues, validationErrorsByNodeId, diff --git a/packages/frontend/editor-ui/src/app/types/workflow.ts b/packages/frontend/editor-ui/src/app/types/workflow.ts index f98539b4ed3..74b00459918 100644 --- a/packages/frontend/editor-ui/src/app/types/workflow.ts +++ b/packages/frontend/editor-ui/src/app/types/workflow.ts @@ -8,6 +8,7 @@ export type WorkflowObjectAccessors = Pick< Workflow, | 'id' | 'connectionsBySourceNode' + | 'connectionsByDestinationNode' | 'expression' | 'pinData' | 'getNode' diff --git a/packages/frontend/editor-ui/src/features/ndv/settings/components/NodeSettings.vue b/packages/frontend/editor-ui/src/features/ndv/settings/components/NodeSettings.vue index 107c6a343d7..7f7b0d04e68 100644 --- a/packages/frontend/editor-ui/src/features/ndv/settings/components/NodeSettings.vue +++ b/packages/frontend/editor-ui/src/features/ndv/settings/components/NodeSettings.vue @@ -447,6 +447,7 @@ const valueChanged = (parameterData: IUpdateInformation) => { nodeHelpers.updateNodeParameterIssuesByName(_node.name); nodeHelpers.updateNodeCredentialIssuesByName(_node.name); + nodeHelpers.updateNodeInputIssuesByName(_node.name); } } else if (nameIsParameter(parameterData)) { // A node parameter changed diff --git a/packages/frontend/editor-ui/src/features/ndv/settings/composables/useNodeSettingsParameters.ts b/packages/frontend/editor-ui/src/features/ndv/settings/composables/useNodeSettingsParameters.ts index d9912df2e70..8c45b542dd5 100644 --- a/packages/frontend/editor-ui/src/features/ndv/settings/composables/useNodeSettingsParameters.ts +++ b/packages/frontend/editor-ui/src/features/ndv/settings/composables/useNodeSettingsParameters.ts @@ -171,6 +171,8 @@ export function useNodeSettingsParameters() { nodeHelpers.updateNodeParameterIssuesByName(node.name); nodeHelpers.updateNodeCredentialIssuesByName(node.name); + // A parameter can change which inputs the node exposes, e.g. `autoFix`. + nodeHelpers.updateNodeInputIssuesByName(node.name); telemetry.trackNodeParametersValuesChange(nodeTypeDescription.name, parameterData); } diff --git a/packages/frontend/editor-ui/src/features/workflows/components/WorkflowPublishModal.vue b/packages/frontend/editor-ui/src/features/workflows/components/WorkflowPublishModal.vue index 7727c8e6caf..92fcd5dd607 100644 --- a/packages/frontend/editor-ui/src/features/workflows/components/WorkflowPublishModal.vue +++ b/packages/frontend/editor-ui/src/features/workflows/components/WorkflowPublishModal.vue @@ -63,9 +63,9 @@ const isReattempt = computed( workflowDocumentStore.value.publicationStatus === 'failed', ); -const nodesWithValidationIssues = computed( - () => workflowDocumentStore.value.nodesWithValidationIssues, -); +// The nodes that actually block publishing, so this list and the Publish button +// cannot disagree about what needs fixing. +const nodesWithValidationIssues = computed(() => workflowDocumentStore.value.publishBlockingNodes); const hasNodeIssues = computed(() => workflowDocumentStore.value.hasPublishBlockingIssues); diff --git a/packages/quality/testing/playwright/tests/e2e/api/publish-unreachable-node.spec.ts b/packages/quality/testing/playwright/tests/e2e/api/publish-unreachable-node.spec.ts new file mode 100644 index 00000000000..ef6d3f38217 --- /dev/null +++ b/packages/quality/testing/playwright/tests/e2e/api/publish-unreachable-node.spec.ts @@ -0,0 +1,338 @@ +import type { IWorkflowBase } from 'n8n-workflow'; +import { nanoid } from 'nanoid'; + +import { test, expect } from '../../../fixtures/base'; + +const AGENT = '@n8n/n8n-nodes-langchain.agent'; +const PARSER = '@n8n/n8n-nodes-langchain.outputParserAutofixing'; +const LANGCHAIN_CODE = '@n8n/n8n-nodes-langchain.code'; +const VECTOR_STORE_TOOL = '@n8n/n8n-nodes-langchain.toolVectorStore'; +const IN_MEMORY_VECTOR_STORE = '@n8n/n8n-nodes-langchain.vectorStoreInMemory'; +const STRUCTURED_PARSER = '@n8n/n8n-nodes-langchain.outputParserStructured'; + +/** + * A schedule trigger feeding a No-Op, plus an agent with an autofixing parser + * attached. Neither the agent nor the parser has a language model, so both + * declare an unmet required input. + * + * `reachable` decides whether the trigger feeds the agent. When it does not, + * the agent and parser form an island: wired to each other, reached by nothing. + */ +function buildWorkflow(reachable: boolean): Partial { + const nodes = [ + { + id: nanoid(), + name: 'Schedule Trigger', + type: 'n8n-nodes-base.scheduleTrigger', + typeVersion: 1.2, + position: [0, 0] as [number, number], + parameters: { rule: { interval: [{ field: 'days' }] } }, + }, + { + id: nanoid(), + name: 'No Op', + type: 'n8n-nodes-base.noOp', + typeVersion: 1, + position: [220, 0] as [number, number], + parameters: {}, + }, + { + id: nanoid(), + name: 'Agent', + type: AGENT, + typeVersion: 2.2, + position: [220, 260] as [number, number], + parameters: { promptType: 'define', text: 'hello', hasOutputParser: true }, + }, + { + id: nanoid(), + name: 'Parser', + type: PARSER, + typeVersion: 1, + position: [420, 420] as [number, number], + parameters: {}, + }, + ]; + + const connections: IWorkflowBase['connections'] = { + 'Schedule Trigger': { + main: [[{ node: reachable ? 'Agent' : 'No Op', type: 'main', index: 0 }]], + }, + Parser: { + ai_outputParser: [[{ node: 'Agent', type: 'ai_outputParser', index: 0 }]], + }, + }; + + return { + name: `required input reachability ${nanoid()}`, + nodes, + connections, + settings: { executionOrder: 'v1' }, + }; +} + +test.describe( + 'Publishing a workflow with unmet required inputs', + { + annotation: [{ type: 'owner', description: 'Catalysts' }], + }, + () => { + const cleanupWorkflowIds: string[] = []; + const cleanupCredentialIds: string[] = []; + + test.afterEach(async ({ api }) => { + // The published cases leave a daily schedule active, which would keep + // firing on a persistent stack. + while (cleanupWorkflowIds.length > 0) { + const workflowId = cleanupWorkflowIds.pop() as string; + + // A refusal case never activated, so this is expected to fail there. + try { + await api.workflows.deactivate(workflowId); + } catch { + // not active + } + + // Archive first. The API refuses to delete a workflow that is not + // archived, so deleting straight away leaves the row behind. + await api.workflows.archive(workflowId); + await api.workflows.delete(workflowId); + } + + while (cleanupCredentialIds.length > 0) { + await api.credentials.deleteCredential(cleanupCredentialIds.pop() as string); + } + }); + + test('publishes when the nodes with unmet inputs cannot be reached', async ({ api }) => { + const created = await api.workflows.createWorkflow(buildWorkflow(false)); + cleanupWorkflowIds.push(created.id); + + const response = await api.workflows.activateRaw(created.id, created.versionId); + + expect( + response.ok(), + `publish was refused over nodes no run can reach: ${await response.text()}`, + ).toBe(true); + + // Activation lands through the publication outbox. Poll for it before the + // teardown deactivates, so cleanup cannot race the publish. + await expect + .poll(async () => (await api.workflows.getPublicationStatus(created.id)).status, { + timeout: 15_000, + }) + .toBe('published'); + }); + + test('publishes a node whose inputs expression cannot be evaluated', async ({ api }) => { + // The LangChain Code node builds its inputs by mapping over its `Inputs` + // collection, which is empty here, so the expression throws. The engine + // swallows that and runs the node with no inputs, so publishing must not + // be refused. Deleting the last `Inputs` entry after wiring the node up + // reaches this state from the editor. + const created = await api.workflows.createWorkflow({ + name: `unresolvable inputs ${nanoid()}`, + nodes: [ + { + id: nanoid(), + name: 'Schedule Trigger', + type: 'n8n-nodes-base.scheduleTrigger', + typeVersion: 1.2, + position: [0, 0], + parameters: { rule: { interval: [{ field: 'days' }] } }, + }, + { + id: nanoid(), + name: 'Custom Tool', + type: LANGCHAIN_CODE, + typeVersion: 1, + position: [220, 0], + parameters: { inputs: {}, outputs: { output: [{ type: 'main' }] } }, + }, + ], + connections: { + 'Schedule Trigger': { + main: [[{ node: 'Custom Tool', type: 'main', index: 0 }]], + }, + }, + settings: { executionOrder: 'v1' }, + }); + cleanupWorkflowIds.push(created.id); + + const response = await api.workflows.activateRaw(created.id, created.versionId); + + expect( + response.ok(), + `publish was refused over an inputs expression the engine ignores: ${await response.text()}`, + ).toBe(true); + + await expect + .poll(async () => (await api.workflows.getPublicationStatus(created.id)).status, { + timeout: 15_000, + }) + .toBe('published'); + }); + + test('publishes a subnode nested behind a disabled parent', async ({ api }) => { + // The tool only feeds the disabled agent, and the vector store only feeds + // the tool, so nothing ever asks the store for its Embedding input. + // Stopping at the first consumer reads the store as live, because the tool + // between them is enabled. + const created = await api.workflows.createWorkflow({ + name: `nested behind a disabled parent ${nanoid()}`, + nodes: [ + { + id: nanoid(), + name: 'Schedule Trigger', + type: 'n8n-nodes-base.scheduleTrigger', + typeVersion: 1.2, + position: [0, 0], + parameters: { rule: { interval: [{ field: 'days' }] } }, + }, + { + id: nanoid(), + name: 'Agent', + type: AGENT, + typeVersion: 2.2, + position: [220, 0], + parameters: { promptType: 'define', text: 'hello' }, + disabled: true, + }, + { + id: nanoid(), + name: 'Store Tool', + type: VECTOR_STORE_TOOL, + typeVersion: 1, + position: [220, 260], + // Set so the parameter validator passes and the required-input + // check is what this test actually exercises. + parameters: { name: 'store_tool', description: 'company docs' }, + }, + { + id: nanoid(), + name: 'Store', + type: IN_MEMORY_VECTOR_STORE, + typeVersion: 1.3, + position: [420, 420], + parameters: {}, + }, + ], + connections: { + 'Schedule Trigger': { main: [[{ node: 'Agent', type: 'main', index: 0 }]] }, + 'Store Tool': { ai_tool: [[{ node: 'Agent', type: 'ai_tool', index: 0 }]] }, + Store: { + ai_vectorStore: [[{ node: 'Store Tool', type: 'ai_vectorStore', index: 0 }]], + }, + }, + settings: { executionOrder: 'v1' }, + }); + cleanupWorkflowIds.push(created.id); + + const response = await api.workflows.activateRaw(created.id, created.versionId); + + expect( + response.ok(), + `publish was refused over a subnode nothing can reach: ${await response.text()}`, + ).toBe(true); + + await expect + .poll(async () => (await api.workflows.getPublicationStatus(created.id)).status, { + timeout: 15_000, + }) + .toBe('published'); + }); + + test('publishes a subnode left on its own beside a healthy agent', async ({ api }) => { + // The chat model is reachable, so the pre-existing credential check applies to + // it. Give it one, or the refusal under test is masked by a credential error. + const credential = await api.credentials.createCredential({ + name: `anthropic ${nanoid()}`, + type: 'anthropicApi', + data: { apiKey: 'sk-ant-not-a-real-key' }, + }); + cleanupCredentialIds.push(credential.id); + + // A Structured Output Parser dragged onto the canvas with Auto-Fix Format on + // and never wired to anything. Its own `Model` input is required and unmet, + // but no run reaches it, so it must not hold up the agent beside it. The + // agent's own Output Parser input is exposed and empty, which is allowed: + // only Chat Model and Fallback Model are required on the agent. + const created = await api.workflows.createWorkflow({ + name: `lone subnode ${nanoid()}`, + nodes: [ + { + id: nanoid(), + name: 'Schedule Trigger', + type: 'n8n-nodes-base.scheduleTrigger', + typeVersion: 1.2, + position: [0, 0], + parameters: { rule: { interval: [{ field: 'days' }] } }, + }, + { + id: nanoid(), + name: 'Sentiment Analyzer', + type: AGENT, + typeVersion: 2.2, + position: [260, 0], + parameters: { promptType: 'define', text: 'hello', hasOutputParser: true }, + }, + { + id: nanoid(), + name: 'Chat Model', + type: '@n8n/n8n-nodes-langchain.lmChatAnthropic', + typeVersion: 1.3, + position: [200, 300], + parameters: { + model: { __rl: true, mode: 'list', value: 'claude-sonnet-4-5' }, + options: {}, + }, + credentials: { anthropicApi: { id: credential.id, name: credential.name } }, + }, + { + id: nanoid(), + name: 'Structured Output Parser', + type: STRUCTURED_PARSER, + typeVersion: 1.3, + position: [560, 300], + parameters: { jsonSchemaExample: '{ "sentiment": "positive" }', autoFix: true }, + }, + ], + connections: { + 'Schedule Trigger': { + main: [[{ node: 'Sentiment Analyzer', type: 'main', index: 0 }]], + }, + 'Chat Model': { + ai_languageModel: [ + [{ node: 'Sentiment Analyzer', type: 'ai_languageModel', index: 0 }], + ], + }, + }, + settings: { executionOrder: 'v1' }, + }); + cleanupWorkflowIds.push(created.id); + + const response = await api.workflows.activateRaw(created.id, created.versionId); + + expect( + response.ok(), + `publish was refused over a subnode wired to nothing: ${await response.text()}`, + ).toBe(true); + + await expect + .poll(async () => (await api.workflows.getPublicationStatus(created.id)).status, { + timeout: 15_000, + }) + .toBe('published'); + }); + + test('refuses to publish when the same nodes are reachable', async ({ api }) => { + const created = await api.workflows.createWorkflow(buildWorkflow(true)); + cleanupWorkflowIds.push(created.id); + + const response = await api.workflows.activateRaw(created.id, created.versionId); + + expect(response.ok()).toBe(false); + expect(await response.text()).toContain('has no node connected to its required'); + }); + }, +); diff --git a/packages/workflow/src/common/get-runnable-nodes.ts b/packages/workflow/src/common/get-runnable-nodes.ts new file mode 100644 index 00000000000..6e66d248911 --- /dev/null +++ b/packages/workflow/src/common/get-runnable-nodes.ts @@ -0,0 +1,114 @@ +import { getExecutableNodeNames } from './get-executable-nodes'; +import type { IConnections, INode } from '../interfaces'; + +/** + * Which nodes a run can actually involve. + * + * Publishing is gated on this in two places that must agree: the server refuses + * activation over a node that would break a run, and the editor greys out the + * Publish button for the same reason. A node no run can reach cannot break one, + * so neither surface may block on it. + */ + +/** Nodes a node feeds directly, by name, in graph order. */ +function consumerNames(nodeName: string, connections: IConnections): string[] { + const names: string[] = []; + + for (const targetsByOutput of Object.values(connections[nodeName] ?? {})) { + for (const targets of targetsByOutput ?? []) { + for (const target of targets ?? []) { + names.push(target.node); + } + } + } + + return names; +} + +/** + * Names of every node some trigger can reach, following the same rules the + * engine uses: the forward `main` closure of each enabled trigger, plus the + * sub-nodes hanging off anything in it. + * + * Disabled nodes are walked through rather than stopped at, because a disabled + * node passes its items downstream, so what follows one still runs. + * + * @param isTriggerLike decides what starts a run. The server reads the node + * type's implementation, the editor only has the description, so each caller + * supplies its own. + */ +export function getReachableNodeNames( + nodes: INode[], + connectionsBySourceNode: IConnections, + connectionsByDestinationNode: IConnections, + isTriggerLike: (node: INode) => boolean, +): Set { + const reachable = new Set(); + + for (const node of nodes) { + if (node.disabled) continue; + if (!isTriggerLike(node)) continue; + + for (const name of getExecutableNodeNames( + connectionsBySourceNode, + connectionsByDestinationNode, + node.name, + )) { + reachable.add(name); + } + } + + return reachable; +} + +/** + * Whether nothing will ever ask this node for its inputs, because every chain + * of consumers out of it is disabled. Only ever true for supply-type nodes (LLM + * models, parsers, memory, tools): anything that can produce a `main` output + * belongs to the flow and runs regardless of what its consumers do. + * + * Consumers are followed transitively. A model feeding a parser feeding a + * disabled agent is as inert as one feeding the disabled agent directly, and + * stopping at the first hop would refuse to publish over it. + * + * `canOutputMain` reads the type's declared outputs rather than the + * connections, since an unwired `main` output is still a main-path node. It must + * answer `true` when unsure, so a half-built graph does not silently opt out of + * the check. + */ +export function onlySuppliesDisabledNodes( + node: INode, + connections: IConnections, + nodes: INode[], + canOutputMain: (node: INode) => boolean, +): boolean { + if (canOutputMain(node)) return false; + + const nodeByName = new Map(nodes.map((n) => [n.name, n])); + // Doubles as cycle protection and keeps this linear over the sub-graph. + const visited = new Set(); + + /** Whether anything that actually runs asks `nodeName` for its output. */ + function isAsked(nodeName: string): boolean { + if (visited.has(nodeName)) return false; + visited.add(nodeName); + + for (const consumerName of consumerNames(nodeName, connections)) { + const consumer = nodeByName.get(consumerName); + // An unknown target is treated as live. + if (!consumer) return true; + if (consumer.disabled) continue; + // A consumer on the main path runs, so it will ask. A supply consumer is + // itself only asked by its own consumers. + if (canOutputMain(consumer) || isAsked(consumer.name)) return true; + } + + return false; + } + + // A supply node wired to nothing is left to the reachability check, which + // skips it anyway, rather than exempted here. + if (consumerNames(node.name, connections).length === 0) return false; + + return !isAsked(node.name); +} diff --git a/packages/workflow/src/common/index.ts b/packages/workflow/src/common/index.ts index 90c4454e3e5..5346ee24d46 100644 --- a/packages/workflow/src/common/index.ts +++ b/packages/workflow/src/common/index.ts @@ -4,5 +4,6 @@ export * from './get-connected-nodes'; export * from './get-executable-nodes'; export * from './get-node-by-name'; export * from './get-parent-nodes'; +export * from './get-runnable-nodes'; export * from './map-connections-by-destination'; export * from './resolve-variables'; diff --git a/packages/workflow/src/node-helpers.ts b/packages/workflow/src/node-helpers.ts index 485f2697d74..f3d7a334433 100644 --- a/packages/workflow/src/node-helpers.ts +++ b/packages/workflow/src/node-helpers.ts @@ -1244,6 +1244,19 @@ export function getNodeInputs( workflow: WorkflowForNodeHelpers, node: INode, nodeTypeData: INodeTypeDescription, + options: { + /** + * Surface a broken `inputs` expression instead of reporting no inputs. + * Swallowing suits rendering; a caller judging validity wants the error. + * + * Only a thrown error counts. An expression that evaluates but yields a + * non-list is left alone: that is almost always an unconfigured node + * (the LangChain Code node maps over an empty `Inputs` collection), and + * the engine reads the same case as "no inputs" and runs. Failing here + * would block publishing a workflow the runtime is happy with. + */ + throwOnExpressionError?: boolean; + } = {}, ): Array { if (Array.isArray(nodeTypeData?.inputs)) { return nodeTypeData.inputs; @@ -1258,6 +1271,7 @@ export function getNodeInputs( {}, ) || []) as NodeConnectionType[]; } catch (e) { + if (options.throwOnExpressionError) throw e; console.warn('Could not calculate inputs dynamically for node: ', node.name); return []; } diff --git a/packages/workflow/src/node-validation.ts b/packages/workflow/src/node-validation.ts index 7b0ae2f3e4e..bd5635426e3 100644 --- a/packages/workflow/src/node-validation.ts +++ b/packages/workflow/src/node-validation.ts @@ -1,5 +1,6 @@ -import type { INode, INodeType, IConnections } from './interfaces'; -import { displayParameter } from './node-helpers'; +import type { INode, INodeType, INodeInputConfiguration, INodeTypeDescription } from './interfaces'; +import { displayParameter, getNodeInputs } from './node-helpers'; +import type { Workflow } from './workflow'; export interface NodeValidationIssue { credential?: string; @@ -12,6 +13,60 @@ export interface NodeCredentialIssue { credentialName: string; } +/** A `Pick` so both the editor's snapshot accessor and the engine's `Workflow` fit. */ +export type WorkflowForInputValidation = Pick< + Workflow, + 'expression' | 'getNode' | 'connectionsByDestinationNode' +>; + +/** + * Inputs a node declares as required with nothing enabled connected to them. + * + * Inputs can be conditional, so they are resolved against the node's current + * parameters first. A disabled source counts as absent, matching the runtime + * ("must be connected and enabled"). + * + * Shared so the editor warning and the publish check agree; callers format their + * own message. + */ +export function getUnconnectedRequiredInputs( + workflow: WorkflowForInputValidation, + node: INode, + nodeTypeDescription: INodeTypeDescription, + options: { throwOnExpressionError?: boolean } = {}, +): INodeInputConfiguration[] { + const unconnected: INodeInputConfiguration[] = []; + // Required by the type on purpose. A missing map is not an empty graph: it + // would read every required input as unconnected. Better a loud TypeError + // here than a silent over-report from an accessor cast past the type. + const arrivals = workflow.connectionsByDestinationNode[node.name]; + + // A node can declare several inputs of one type — an agent's Chat Model and + // Fallback Model are both `ai_languageModel` — and each is satisfied on its + // own index. Counting position per type mirrors how the engine resolves them + // (`validateInputConfiguration` filters by type, then indexes into the + // type's connections), so one connected model cannot satisfy both. + const indexByType = new Map(); + + for (const input of getNodeInputs(workflow, node, nodeTypeDescription, options)) { + const type = typeof input === 'string' ? input : input.type; + const inputIndex = indexByType.get(type) ?? 0; + indexByType.set(type, inputIndex + 1); + + if (typeof input === 'string' || input.required !== true) continue; + + const sources = arrivals?.[type]?.[inputIndex] ?? []; + const hasEnabledSource = sources.some((source) => { + const parent = workflow.getNode(source.node); + return parent ? !parent.disabled : false; + }); + + if (!hasEnabledSource) unconnected.push(input); + } + + return unconnected; +} + /** * Validates that all required credentials are set for a node. * Respects displayOptions to only validate credentials that should be shown. @@ -53,30 +108,6 @@ export function validateNodeCredentials(node: INode, nodeType: INodeType): NodeC return issues; } -/** - * Checks if a node has any incoming or outgoing connections. - */ -export function isNodeConnected( - nodeName: string, - connections: IConnections, - connectionsByDestination: IConnections, -): boolean { - // Check outgoing connections - if (connections[nodeName] && Object.keys(connections[nodeName]).length > 0) { - return true; - } - - // Check incoming connections - if ( - connectionsByDestination[nodeName] && - Object.keys(connectionsByDestination[nodeName]).length > 0 - ) { - return true; - } - - return false; -} - /** * Checks if a node type is a trigger-like node (trigger, webhook, or poll). * These nodes are workflow entry points and should always be validated. diff --git a/packages/workflow/test/node-validation.test.ts b/packages/workflow/test/node-validation.test.ts index 9415bcfa58b..8cc46444ef5 100644 --- a/packages/workflow/test/node-validation.test.ts +++ b/packages/workflow/test/node-validation.test.ts @@ -1,9 +1,10 @@ -import type { INode, INodeType, IConnections, INodeTypeDescription } from '../src/interfaces'; +import type { INode, INodeType, INodeTypeDescription } from '../src/interfaces'; import { validateNodeCredentials, - isNodeConnected, + getUnconnectedRequiredInputs, isTriggerLikeNode, type NodeCredentialIssue, + type WorkflowForInputValidation, } from '../src/node-validation'; describe('node-validation', () => { @@ -198,77 +199,6 @@ describe('node-validation', () => { }); }); - describe('isNodeConnected', () => { - it('should return true when node has outgoing connections', () => { - const connections: IConnections = { - 'Node A': { - main: [[{ node: 'Node B', type: 'main', index: 0 }]], - }, - }; - const connectionsByDestination: IConnections = {}; - - const result = isNodeConnected('Node A', connections, connectionsByDestination); - - expect(result).toBe(true); - }); - - it('should return true when node has incoming connections', () => { - const connections: IConnections = {}; - const connectionsByDestination: IConnections = { - 'Node B': { - main: [[{ node: 'Node A', type: 'main', index: 0 }]], - }, - }; - - const result = isNodeConnected('Node B', connections, connectionsByDestination); - - expect(result).toBe(true); - }); - - it('should return true when node has both incoming and outgoing connections', () => { - const connections: IConnections = { - 'Node B': { - main: [[{ node: 'Node C', type: 'main', index: 0 }]], - }, - }; - const connectionsByDestination: IConnections = { - 'Node B': { - main: [[{ node: 'Node A', type: 'main', index: 0 }]], - }, - }; - - const result = isNodeConnected('Node B', connections, connectionsByDestination); - - expect(result).toBe(true); - }); - - it('should return false when node has no connections', () => { - const connections: IConnections = { - 'Node A': { - main: [[{ node: 'Node B', type: 'main', index: 0 }]], - }, - }; - const connectionsByDestination: IConnections = {}; - - const result = isNodeConnected('Node C', connections, connectionsByDestination); - - expect(result).toBe(false); - }); - - it('should return false when node exists but has empty connections', () => { - const connections: IConnections = { - 'Node A': {}, - }; - const connectionsByDestination: IConnections = { - 'Node A': {}, - }; - - const result = isNodeConnected('Node A', connections, connectionsByDestination); - - expect(result).toBe(false); - }); - }); - describe('isTriggerLikeNode', () => { it('should return true for node with trigger function', () => { const nodeType: INodeType = { @@ -368,4 +298,219 @@ describe('node-validation', () => { expect(isTriggerLikeNode(nodeType)).toBe(false); }); }); + describe('getUnconnectedRequiredInputs', () => { + const parser: INode = { + name: 'Parser', + type: 'parser', + id: 'parser-1', + typeVersion: 1, + position: [0, 0], + parameters: { autoFix: true }, + }; + + const model: INode = { + name: 'Model', + type: 'model', + id: 'model-1', + typeVersion: 1, + position: [0, 0], + parameters: {}, + }; + + /** An expression, so inputs depend on the node's parameters. */ + const description = { + inputs: + '={{ $parameter.autoFix ? [{ displayName: "Model", type: "ai_languageModel", required: true }] : [] }}', + } as unknown as INodeTypeDescription; + + /** + * `sourcesByIndex` lists the nodes arriving at each `ai_languageModel` + * input, by input index — the same shape the engine indexes into, so a + * source on index 0 cannot stand in for the input on index 1. + */ + const makeWorkflow = ( + sourcesByIndex: string[][], + nodes: Record, + ): WorkflowForInputValidation => + ({ + expression: { + // Mirrors the engine: an undefined `inputs` resolves to undefined + // rather than to a list. + getSimpleParameterValue: (node: INode, value: unknown) => + value === undefined + ? undefined + : (node.parameters as { autoFix?: boolean }).autoFix + ? [{ displayName: 'Model', type: 'ai_languageModel', required: true }] + : [], + }, + getNode: (name: string) => nodes[name] ?? null, + connectionsByDestinationNode: { + Parser: { + ai_languageModel: sourcesByIndex.map((names) => + names.map((name) => ({ node: name, type: 'ai_languageModel', index: 0 })), + ), + }, + }, + }) as unknown as WorkflowForInputValidation; + + it('reports a required input with nothing connected', () => { + const result = getUnconnectedRequiredInputs( + makeWorkflow([], { Parser: parser }), + parser, + description, + ); + + expect(result).toHaveLength(1); + expect(result[0].type).toBe('ai_languageModel'); + }); + + it('reports nothing once an enabled node is connected', () => { + const result = getUnconnectedRequiredInputs( + makeWorkflow([['Model']], { Parser: parser, Model: model }), + parser, + description, + ); + + expect(result).toEqual([]); + }); + + it('treats a disabled source as not connected', () => { + const result = getUnconnectedRequiredInputs( + makeWorkflow([['Model']], { Parser: parser, Model: { ...model, disabled: true } }), + parser, + description, + ); + + expect(result).toHaveLength(1); + }); + + it('reports nothing when a failed inputs expression is treated as empty', () => { + const throwing = { + expression: { + getSimpleParameterValue: () => { + throw new Error('boom'); + }, + }, + getNode: () => parser, + connectionsByDestinationNode: {}, + } as unknown as WorkflowForInputValidation; + + expect(getUnconnectedRequiredInputs(throwing, parser, description)).toEqual([]); + }); + + it('surfaces a failed inputs expression when asked to', () => { + const throwing = { + expression: { + getSimpleParameterValue: () => { + throw new Error('boom'); + }, + }, + getNode: () => parser, + connectionsByDestinationNode: {}, + } as unknown as WorkflowForInputValidation; + + expect(() => + getUnconnectedRequiredInputs(throwing, parser, description, { + throwOnExpressionError: true, + }), + ).toThrow('boom'); + }); + + // An expression that evaluates to a non-list is the normal state of an + // unconfigured node: the LangChain Code node maps over its `Inputs` + // collection, which is empty by default, and the engine swallows the + // resulting TypeError into `null`. The engine then reads that as "no + // inputs" and runs, so neither caller may treat it as an error — doing + // so blocks publishing a workflow the runtime is happy with. + describe('when the expression resolves to no list at all', () => { + const unresolved = { + expression: { getSimpleParameterValue: () => null }, + getNode: () => parser, + connectionsByDestinationNode: {}, + } as unknown as WorkflowForInputValidation; + + it('reports nothing even when asked to surface expression errors', () => { + expect( + getUnconnectedRequiredInputs(unresolved, parser, description, { + throwOnExpressionError: true, + }), + ).toEqual([]); + }); + + it('reports nothing when swallowing suits the caller', () => { + expect(getUnconnectedRequiredInputs(unresolved, parser, description)).toEqual([]); + }); + }); + + it('reports nothing for a type that declares no inputs at all', () => { + // Nothing to resolve, so this is "requires nothing", not "unknown". + const noInputs = { properties: [] } as unknown as INodeTypeDescription; + + expect( + getUnconnectedRequiredInputs(makeWorkflow([], { Parser: parser }), parser, noInputs, { + throwOnExpressionError: true, + }), + ).toEqual([]); + }); + + // An agent with a fallback declares two required `ai_languageModel` inputs; + // the engine satisfies each on its own index. + describe('with two required inputs of the same type', () => { + const twoModels = { + inputs: [ + { displayName: 'Chat Model', type: 'ai_languageModel', required: true }, + { displayName: 'Fallback Model', type: 'ai_languageModel', required: true }, + ], + } as unknown as INodeTypeDescription; + + it('does not let a source on the first index satisfy the second', () => { + const result = getUnconnectedRequiredInputs( + makeWorkflow([['Model']], { Parser: parser, Model: model }), + parser, + twoModels, + ); + + expect(result.map((input) => input.displayName)).toEqual(['Fallback Model']); + }); + + it('reports nothing once each index has its own source', () => { + const result = getUnconnectedRequiredInputs( + makeWorkflow([['Primary'], ['Fallback']], { + Parser: parser, + Primary: model, + Fallback: { ...model, name: 'Fallback' }, + }), + parser, + twoModels, + ); + + expect(result).toEqual([]); + }); + + it('treats a disabled source on the second index as absent', () => { + const result = getUnconnectedRequiredInputs( + makeWorkflow([['Primary'], ['Fallback']], { + Parser: parser, + Primary: model, + Fallback: { ...model, name: 'Fallback', disabled: true }, + }), + parser, + twoModels, + ); + + expect(result.map((input) => input.displayName)).toEqual(['Fallback Model']); + }); + }); + + it('reports nothing when the parameters do not make the input required', () => { + const off = { ...parser, parameters: { autoFix: false } }; + const result = getUnconnectedRequiredInputs( + makeWorkflow([], { Parser: off }), + off, + description, + ); + + expect(result).toEqual([]); + }); + }); });