Merge 4491b564d3 into dfac3e524e

web/app/components/workflow/hooks/use-checklist.ts

@@ -70,6 +70,7 @@ import {
   getToolCheckParams,
   getValidTreeNodes,
 } from '../utils'
+import { getConflictingEndOutputVariables } from '../utils/end-output-conflict'
 import { extractPluginId } from '../utils/plugin'
 import { isNodePluginMissing } from '../utils/plugin-install-check'
 import { getTriggerCheckParams } from '../utils/trigger'
@@ -115,33 +116,21 @@ const START_NODE_TYPES: BlockEnum[] = [
 
 const getDuplicateEndOutputMessages = (
   nodes: Node[],
+  edges: Edge[],
   t: ReturnType<typeof useTranslation>['t'],
 ) => {
-  const variableOccurrences = new Map<string, string[]>()
-
-  nodes.forEach((node) => {
-    if (node.type !== CUSTOM_NODE || node.data.type !== BlockEnum.End) return
-
-    const outputs = (node.data as { outputs?: Array<{ variable?: string }> }).outputs || []
-    outputs.forEach((output) => {
-      const variable = output.variable?.trim()
-      if (!variable) return
-
-      const occurrences = variableOccurrences.get(variable) || []
-      occurrences.push(node.id)
-      variableOccurrences.set(variable, occurrences)
-    })
-  })
+  // Only flag Output(End) nodes that reuse a variable name while being able to
+  // run in the same execution. Output nodes on mutually exclusive branches may
+  // safely share names, so branch topology is taken into account here.
+  const conflictingVariablesByNode = getConflictingEndOutputVariables(nodes, edges)
 
   const nodeMessages = new Map<string, string[]>()
-  variableOccurrences.forEach((nodeIds, variable) => {
-    if (nodeIds.length <= 1) return
-
-    Array.from(new Set(nodeIds)).forEach((nodeId) => {
-      const messages = nodeMessages.get(nodeId) || []
+  conflictingVariablesByNode.forEach((variables, nodeId) => {
+    const messages = nodeMessages.get(nodeId) || []
+    variables.forEach((variable) => {
       messages.push(t(($) => $['errorMsg.duplicateOutputVariable'], { ns: 'workflow', variable }))
-      nodeMessages.set(nodeId, messages)
     })
+    nodeMessages.set(nodeId, messages)
   })
 
   return nodeMessages
@@ -325,7 +314,7 @@ export const useChecklist = (nodes: Node[], edges: Edge[], options?: { flowType?
   const needWarningNodes = useMemo<ChecklistItem[]>(() => {
     const list: ChecklistItem[] = []
     const filteredNodes = nodes.filter((node) => node.type === CUSTOM_NODE)
-    const duplicateEndOutputMessages = getDuplicateEndOutputMessages(filteredNodes, t)
+    const duplicateEndOutputMessages = getDuplicateEndOutputMessages(filteredNodes, edges, t)
     const { validNodes } = getValidTreeNodes(filteredNodes, edges)
     const installedPluginIds = new Set(modelProviders.map((p) => extractPluginId(p.provider)))
 
@@ -644,7 +633,7 @@ export const useChecklistBeforePublish = () => {
     const { dataSourceList, environmentVariables = [] } = workflowStore.getState()
     const nodes = getNodes()
     const filteredNodes = nodes.filter((node) => node.type === CUSTOM_NODE)
-    const duplicateEndOutputMessages = getDuplicateEndOutputMessages(filteredNodes, t)
+    const duplicateEndOutputMessages = getDuplicateEndOutputMessages(filteredNodes, edges, t)
     const { validNodes, maxDepth } = getValidTreeNodes(filteredNodes, edges)
 
     if (maxDepth > MAX_TREE_DEPTH) {

web/app/components/workflow/utils/__tests__/end-output-conflict.spec.ts

@@ -0,0 +1,184 @@
+import type { Edge, Node } from '../../types'
+import { createEdge, createNode, resetFixtureCounters } from '../../__tests__/fixtures'
+import { BlockEnum } from '../../types'
+import { getConflictingEndOutputVariables } from '../end-output-conflict'
+
+beforeEach(() => {
+  resetFixtureCounters()
+})
+
+const startNode = (id = 'start') => createNode({ id, data: { type: BlockEnum.Start, title: id } })
+
+const triggerNode = (id: string) =>
+  createNode({ id, data: { type: BlockEnum.TriggerWebhook, title: id } })
+
+const endNode = (id: string, variables: string[]) =>
+  createNode({
+    id,
+    data: {
+      type: BlockEnum.End,
+      title: id,
+      outputs: variables.map((variable) => ({ variable, value_selector: ['sys', variable] })),
+    },
+  })
+
+const branchNode = (id: string, type: BlockEnum) => createNode({ id, data: { type, title: id } })
+
+const edge = (source: string, target: string, sourceHandle?: string): Edge =>
+  createEdge({ source, target, sourceHandle })
+
+const conflictsFor = (nodes: Node[], edges: Edge[]) => {
+  const map = getConflictingEndOutputVariables(nodes, edges)
+  return {
+    has: (nodeId: string, variable: string) => map.get(nodeId)?.has(variable) ?? false,
+    size: map.size,
+  }
+}
+
+describe('getConflictingEndOutputVariables', () => {
+  it('flags identical variables on two parallel end nodes (same handle fork)', () => {
+    const nodes = [startNode(), endNode('end-1', ['answer']), endNode('end-2', ['answer'])]
+    const edges = [edge('start', 'end-1'), edge('start', 'end-2')]
+
+    const { has } = conflictsFor(nodes, edges)
+    expect(has('end-1', 'answer')).toBe(true)
+    expect(has('end-2', 'answer')).toBe(true)
+  })
+
+  it('does NOT flag identical variables on mutually exclusive IfElse branches', () => {
+    const nodes = [
+      startNode(),
+      branchNode('if', BlockEnum.IfElse),
+      endNode('end-true', ['answer']),
+      endNode('end-false', ['answer']),
+    ]
+    const edges = [
+      edge('start', 'if'),
+      edge('if', 'end-true', 'true'),
+      edge('if', 'end-false', 'false'),
+    ]
+
+    expect(conflictsFor(nodes, edges).size).toBe(0)
+  })
+
+  it('does NOT flag identical variables on distinct QuestionClassifier classes', () => {
+    const nodes = [
+      startNode(),
+      branchNode('qc', BlockEnum.QuestionClassifier),
+      endNode('end-a', ['answer']),
+      endNode('end-b', ['answer']),
+    ]
+    const edges = [
+      edge('start', 'qc'),
+      edge('qc', 'end-a', 'class-1'),
+      edge('qc', 'end-b', 'class-2'),
+    ]
+
+    expect(conflictsFor(nodes, edges).size).toBe(0)
+  })
+
+  it('does NOT flag success vs fail-branch of the same node', () => {
+    const nodes = [
+      startNode(),
+      branchNode('llm', BlockEnum.LLM),
+      endNode('end-ok', ['answer']),
+      endNode('end-err', ['answer']),
+    ]
+    const edges = [
+      edge('start', 'llm'),
+      edge('llm', 'end-ok'), // default 'source' handle
+      edge('llm', 'end-err', 'fail-branch'),
+    ]
+
+    expect(conflictsFor(nodes, edges).size).toBe(0)
+  })
+
+  it('flags ends that re-merge after an IfElse before splitting again', () => {
+    // if(true|false) -> merge -> end-1 AND end-2 (parallel after the merge)
+    const nodes = [
+      startNode(),
+      branchNode('if', BlockEnum.IfElse),
+      branchNode('merge', BlockEnum.Code),
+      endNode('end-1', ['answer']),
+      endNode('end-2', ['answer']),
+    ]
+    const edges = [
+      edge('start', 'if'),
+      edge('if', 'merge', 'true'),
+      edge('if', 'merge', 'false'),
+      edge('merge', 'end-1'),
+      edge('merge', 'end-2'),
+    ]
+
+    const { has } = conflictsFor(nodes, edges)
+    expect(has('end-1', 'answer')).toBe(true)
+    expect(has('end-2', 'answer')).toBe(true)
+  })
+
+  it('does NOT flag ends behind disjoint triggers (separate runs)', () => {
+    const nodes = [
+      startNode('start-a'),
+      triggerNode('start-b'),
+      endNode('end-a', ['answer']),
+      endNode('end-b', ['answer']),
+    ]
+    const edges = [edge('start-a', 'end-a'), edge('start-b', 'end-b')]
+
+    expect(conflictsFor(nodes, edges).size).toBe(0)
+  })
+
+  it('flags a variable repeated within a single end node', () => {
+    const nodes = [startNode(), endNode('end-1', ['answer', 'answer'])]
+    const edges = [edge('start', 'end-1')]
+
+    expect(conflictsFor(nodes, edges).has('end-1', 'answer')).toBe(true)
+  })
+
+  it('handles more than two ends: exclusive cases pass, parallel pair fails', () => {
+    // if.true -> end-1 ; if.false -> (parallel) end-2 + end-3
+    const nodes = [
+      startNode(),
+      branchNode('if', BlockEnum.IfElse),
+      branchNode('fork', BlockEnum.Code),
+      endNode('end-1', ['answer']),
+      endNode('end-2', ['answer']),
+      endNode('end-3', ['answer']),
+    ]
+    const edges = [
+      edge('start', 'if'),
+      edge('if', 'end-1', 'true'),
+      edge('if', 'fork', 'false'),
+      edge('fork', 'end-2'),
+      edge('fork', 'end-3'),
+    ]
+
+    const { has } = conflictsFor(nodes, edges)
+    // end-1 is exclusive with both others -> never flagged
+    expect(has('end-1', 'answer')).toBe(false)
+    // end-2 and end-3 run together under the false branch -> flagged
+    expect(has('end-2', 'answer')).toBe(true)
+    expect(has('end-3', 'answer')).toBe(true)
+  })
+
+  it('does not flag different variable names', () => {
+    const nodes = [startNode(), endNode('end-1', ['a']), endNode('end-2', ['b'])]
+    const edges = [edge('start', 'end-1'), edge('start', 'end-2')]
+
+    expect(conflictsFor(nodes, edges).size).toBe(0)
+  })
+
+  it('only flags the shared variable when ends mix shared and unique names', () => {
+    const nodes = [
+      startNode(),
+      endNode('end-1', ['shared', 'only1']),
+      endNode('end-2', ['shared', 'only2']),
+    ]
+    const edges = [edge('start', 'end-1'), edge('start', 'end-2')]
+
+    const { has } = conflictsFor(nodes, edges)
+    expect(has('end-1', 'shared')).toBe(true)
+    expect(has('end-2', 'shared')).toBe(true)
+    expect(has('end-1', 'only1')).toBe(false)
+    expect(has('end-2', 'only2')).toBe(false)
+  })
+})

web/app/components/workflow/utils/end-output-conflict.ts

@@ -0,0 +1,274 @@
+import type { Edge, Node } from '../types'
+import { CUSTOM_NODE } from '../constants'
+import { BlockEnum } from '../types'
+
+/**
+ * Detects Output(End) nodes that declare the *same* output variable name while
+ * being able to run within the *same* workflow execution.
+ *
+ * Because every Output node writes into one shared `outputs` object of the run,
+ * two Output nodes sharing a variable name only collide when both of them can
+ * actually execute in a single run. When they sit on mutually exclusive branches
+ * (only one of them can ever run), reusing the name is safe and must not be
+ * flagged.
+ *
+ * The analysis is intentionally *sound*: it only clears a pair when mutual
+ * exclusivity can be proven, and flags everything else (including cases it can't
+ * prove). This favours false positives over false negatives, so a real overwrite
+ * is never silently allowed.
+ */
+
+// Handle id emitted by the error-handling "fail branch" of error-capable nodes.
+// Kept as a literal to avoid importing the error-handle types barrel here.
+const FAIL_BRANCH_HANDLE = 'fail-branch'
+const DEFAULT_HANDLE = 'source'
+
+const START_TYPES = new Set<BlockEnum>([
+  BlockEnum.Start,
+  BlockEnum.TriggerSchedule,
+  BlockEnum.TriggerWebhook,
+  BlockEnum.TriggerPlugin,
+])
+
+type EndOutput = { variable?: string }
+
+type DecisionKind = 'exclusiveHandles' | 'errorBranch'
+
+type Graph = {
+  nodeById: Map<string, Node>
+  outgoing: Map<string, Edge[]>
+}
+
+const isEndNode = (node: Node): boolean =>
+  node.type === CUSTOM_NODE && node.data.type === BlockEnum.End
+
+const getEndOutputs = (node: Node): EndOutput[] =>
+  (node.data as { outputs?: EndOutput[] }).outputs || []
+
+const handleOf = (edge: Edge): string => edge.sourceHandle || DEFAULT_HANDLE
+
+const buildGraph = (nodes: Node[], edges: Edge[]): Graph => {
+  const nodeById = new Map<string, Node>()
+  const outgoing = new Map<string, Edge[]>()
+
+  nodes.forEach((node) => {
+    nodeById.set(node.id, node)
+  })
+
+  edges.forEach((edge) => {
+    if (!nodeById.has(edge.source) || !nodeById.has(edge.target)) return
+    const list = outgoing.get(edge.source)
+    if (list) list.push(edge)
+    else outgoing.set(edge.source, [edge])
+  })
+
+  return { nodeById, outgoing }
+}
+
+const setsDisjoint = (a: Set<string>, b: Set<string>): boolean => {
+  for (const value of a) {
+    if (b.has(value)) return false
+  }
+  return true
+}
+
+/**
+ * Node ids reachable from `sources`, optionally without ever passing through
+ * `blocked`. Cycle-safe (Loop nodes create back edges).
+ */
+const reachableFrom = (graph: Graph, sources: string[], blocked?: string): Set<string> => {
+  const seen = new Set<string>()
+  const stack = [...sources]
+
+  while (stack.length) {
+    const id = stack.pop()!
+    if (id === blocked || seen.has(id)) continue
+    seen.add(id)
+
+    const outs = graph.outgoing.get(id)
+    if (!outs) continue
+    for (const edge of outs) {
+      if (edge.target === blocked || seen.has(edge.target)) continue
+      stack.push(edge.target)
+    }
+  }
+
+  return seen
+}
+
+/**
+ * What kind of exclusive routing (if any) a node performs across its outgoing
+ * handles.
+ * - IfElse / QuestionClassifier: every distinct handle is mutually exclusive
+ *   with every other one (exactly one handle is taken per run).
+ * - Any node exposing a `fail-branch` handle: the success handle(s) and the
+ *   fail-branch handle are mutually exclusive.
+ */
+const getDecisionKind = (node: Node, outs: Edge[]): DecisionKind | null => {
+  const type = node.data.type
+  if (type === BlockEnum.IfElse || type === BlockEnum.QuestionClassifier) return 'exclusiveHandles'
+  if (outs.some((edge) => handleOf(edge) === FAIL_BRANCH_HANDLE)) return 'errorBranch'
+  return null
+}
+
+const areHandleSetsExclusive = (
+  kind: DecisionKind,
+  handlesToA: Set<string>,
+  handlesToB: Set<string>,
+): boolean => {
+  if (!handlesToA.size || !handlesToB.size) return false
+
+  if (kind === 'exclusiveHandles') return setsDisjoint(handlesToA, handlesToB)
+
+  // errorBranch: success side vs fail-branch side, each side homogeneous.
+  const allFail = (set: Set<string>) => Array.from(set).every((h) => h === FAIL_BRANCH_HANDLE)
+  const noneFail = (set: Set<string>) => Array.from(set).every((h) => h !== FAIL_BRANCH_HANDLE)
+  return (
+    (allFail(handlesToA) && noneFail(handlesToB)) || (noneFail(handlesToA) && allFail(handlesToB))
+  )
+}
+
+export const getConflictingEndOutputVariables = (
+  nodes: Node[],
+  edges: Edge[],
+): Map<string, Set<string>> => {
+  const graph = buildGraph(nodes, edges)
+  const startIds = nodes.filter((node) => START_TYPES.has(node.data.type)).map((node) => node.id)
+  const endNodes = nodes.filter(isEndNode)
+
+  const result = new Map<string, Set<string>>()
+  const addConflict = (nodeId: string, variable: string) => {
+    const set = result.get(nodeId)
+    if (set) set.add(variable)
+    else result.set(nodeId, new Set([variable]))
+  }
+
+  // Reachability per start node (multi-trigger workflows: a single run enters
+  // through exactly one trigger, so ends reached from disjoint trigger sets can
+  // never collide).
+  const reachPerStart = new Map<string, Set<string>>()
+  startIds.forEach((startId) => reachPerStart.set(startId, reachableFrom(graph, [startId])))
+  const reachableFromAnyStart = new Set<string>()
+  reachPerStart.forEach((set) => set.forEach((id) => reachableFromAnyStart.add(id)))
+
+  const startsReaching = (nodeId: string): Set<string> => {
+    const res = new Set<string>()
+    reachPerStart.forEach((set, startId) => {
+      if (set.has(nodeId)) res.add(startId)
+    })
+    return res
+  }
+
+  // --- caches (keyed by node ids) -----------------------------------------
+  const dominanceCache = new Map<string, boolean>()
+  // D dominates E: E is unreachable from every start once D is removed.
+  const dominates = (decisionId: string, endId: string): boolean => {
+    if (decisionId === endId) return false
+    const key = `${decisionId}|${endId}`
+    const cached = dominanceCache.get(key)
+    if (cached !== undefined) return cached
+    const reach = reachableFrom(graph, startIds, decisionId)
+    const value = !reach.has(endId)
+    dominanceCache.set(key, value)
+    return value
+  }
+
+  const handlesReachingCache = new Map<string, Set<string>>()
+  // Handles of D through which E is reachable (never routing back through D).
+  const handlesReaching = (decisionId: string, endId: string): Set<string> => {
+    const key = `${decisionId}|${endId}`
+    const cached = handlesReachingCache.get(key)
+    if (cached) return cached
+
+    const handles = new Set<string>()
+    const outs = graph.outgoing.get(decisionId) || []
+    const perHandleReach = new Map<string, Set<string>>()
+    for (const edge of outs) {
+      const handle = handleOf(edge)
+      let reach = perHandleReach.get(handle)
+      if (!reach) {
+        reach = new Set<string>()
+        perHandleReach.set(handle, reach)
+      }
+      reachableFrom(graph, [edge.target], decisionId).forEach((id) => reach!.add(id))
+    }
+    perHandleReach.forEach((reach, handle) => {
+      if (reach.has(endId)) handles.add(handle)
+    })
+
+    handlesReachingCache.set(key, handles)
+    return handles
+  }
+
+  // Decision nodes, precomputed once.
+  const decisionNodes: Array<{ id: string; kind: DecisionKind }> = []
+  graph.nodeById.forEach((node, id) => {
+    const kind = getDecisionKind(node, graph.outgoing.get(id) || [])
+    if (kind) decisionNodes.push({ id, kind })
+  })
+
+  const existsSeparatingDecision = (endA: string, endB: string): boolean => {
+    for (const { id, kind } of decisionNodes) {
+      if (id === endA || id === endB) continue
+      if (!dominates(id, endA) || !dominates(id, endB)) continue
+      if (areHandleSetsExclusive(kind, handlesReaching(id, endA), handlesReaching(id, endB)))
+        return true
+    }
+    return false
+  }
+
+  const canRunTogether = (endA: string, endB: string): boolean => {
+    // No start/trigger: can't reason about the graph, fall back to treating
+    // same-named outputs as a conflict (matches the previous flat behaviour).
+    if (startIds.length === 0) return true
+
+    // Unreachable ends never run; ends behind disjoint triggers never share a run.
+    if (!reachableFromAnyStart.has(endA) || !reachableFromAnyStart.has(endB)) return false
+    if (setsDisjoint(startsReaching(endA), startsReaching(endB))) return false
+
+    return !existsSeparatingDecision(endA, endB)
+  }
+
+  // Group Output nodes by variable name, and flag same-node repeats immediately
+  // (writing the same key twice within one node always overwrites).
+  const ownersByVariable = new Map<string, string[]>()
+  endNodes.forEach((node) => {
+    const seenInNode = new Set<string>()
+    getEndOutputs(node).forEach((output) => {
+      const variable = output.variable?.trim()
+      if (!variable) return
+      if (seenInNode.has(variable)) {
+        addConflict(node.id, variable)
+        return
+      }
+      seenInNode.add(variable)
+    })
+    seenInNode.forEach((variable) => {
+      const owners = ownersByVariable.get(variable)
+      if (owners) owners.push(node.id)
+      else ownersByVariable.set(variable, [node.id])
+    })
+  })
+
+  ownersByVariable.forEach((ownerIds, variable) => {
+    const uniqueOwners = Array.from(new Set(ownerIds))
+    if (uniqueOwners.length <= 1) return
+
+    for (let i = 0; i < uniqueOwners.length; i++) {
+      const ownerA = uniqueOwners[i]
+      if (!ownerA) continue
+
+      for (let j = i + 1; j < uniqueOwners.length; j++) {
+        const ownerB = uniqueOwners[j]
+        if (!ownerB) continue
+
+        if (canRunTogether(ownerA, ownerB)) {
+          addConflict(ownerA, variable)
+          addConflict(ownerB, variable)
+        }
+      }
+    }
+  })
+
+  return result
+}