fix(workflow-edges): enforce edge/block validation server-side, not just client-side (#5571)

* fix(workflow-edges): enforce edge/block validation server-side, not just client-side

Dragging a connection that creates a cycle correctly refused to render
client-side, but the cyclic edge was still queued for realtime persistence
and written to the DB, so it reappeared after refresh. Root cause: cycle
detection (and several other edge/block rules) only lived in the client
Zustand store and was never enforced by the realtime persistence layer,
which is the actual source of truth on reload.

- Move wouldCreateCycle, edge scope-boundary, annotation-only-block,
  duplicate-edge, and block-name-conflict checks into @sim/workflow-types
  so the client store, the collaborative queueing layer, and
  apps/realtime's DB write path all share one implementation
- Wire these into apps/realtime/database/operations.ts's edge-add and
  block-rename handlers as the authoritative gate
- Client-side behavior is unchanged (same call sites, same error messages,
  same rule ordering) — verified via existing + new test coverage

* fix(workflow-edges): address review findings on realtime edge validation

- Select triggerMode when fetching blocks for edge-add validation —
  isKnownWorkflowTriggerBlock checked block.triggerMode but the column
  was never fetched from the DB, so trigger-mode blocks could still
  receive an incoming edge (Cursor Bugbot)
- Make filterUniqueWorkflowEdges incremental, so two duplicate edges
  within the same BATCH_ADD_EDGES payload are also deduped instead of
  both surviving (Greptile)

* fix(workflow-edges): normalize empty-string handles in duplicate-edge check

filterUniqueWorkflowEdges compared handles with ??, so a `sourceHandle: ''`
edge wasn't recognized as a duplicate of an existing null-handle edge —
even though both get persisted as the same null value at insert time
(edge.sourceHandle || null). Falsy-coalesce in the comparison so '' and
null/undefined are treated as the same "no handle" state everywhere.
(Greptile)

* improvement(workflow-edges): dedup realtime edge-add validation, reuse block-name-conflict helper

/simplify pass on the already-merged-quality PR before sign-off:

- Extract filterEdgesForPersist in apps/realtime/src/database/operations.ts:
  the single-edge ADD and batch BATCH_ADD_EDGES handlers hand-inlined the
  same six-step validation pipeline (missing block, protected target,
  annotation-only, trigger-target, scope boundary, duplicate, cycle) and
  each independently re-fetched blocksById/existingEdgesForCycleCheck. Two
  copies of one rule in the same file was exactly the drift risk this PR
  otherwise closes across client/server. One shared helper now backs both.
  Net -63 lines despite the new shared function.
- Fix a real bug this surfaced: droppedCounts was keyed by the free-text,
  block-id-bearing scope-boundary message, so it could never aggregate
  across edges/runs. Now keyed by a stable 'scope boundary' reason.
- use-collaborative-workflow.ts's collaborativeUpdateBlockName still
  hand-rolled the empty/reserved/duplicate block-name-conflict checks this
  PR centralized as getWorkflowBlockNameConflict (already adopted by
  store.ts). Switched it to the shared helper, which also fixes a latent
  check-order mismatch between the two (this pre-check ran reserved before
  duplicate; the store's real gate — after this PR's own store.ts change —
  runs duplicate before reserved, so they could disagree on which toast a
  name that was both reserved and duplicate would surface).

* docs(workflow-edges): document the per-workflow write-serialization invariant

No behavior change. Address Greptile P1 on filterEdgesForPersist ('concurrent
duplicate writes can persist without a per-workflow write guard') by
documenting, at the actual mechanism, why the concern doesn't apply here:
persistWorkflowOperation's leading 'UPDATE workflow SET updatedAt ... WHERE
id = workflowId' already takes a row lock that serializes every operation
(including edge adds) for a given workflowId — a second concurrent call
blocks on that UPDATE until the first transaction commits or rolls back, so
the validate-then-insert sequence in filterEdgesForPersist can never
interleave across two writers on the same workflow.

Verified empirically, not just by reading: ran two concurrent transactions
against a throwaway local Postgres against the exact statement shape used
here (UPDATE the parent row, sleep to simulate the read/validate window,
insert, commit). The second transaction's UPDATE blocked for the full
duration of the first's transaction and only proceeded once the first
committed — confirming the row lock, not any application-level guard,
already provides the serialization Greptile flagged as missing.

Added a comment at the lock site (not a second, redundant advisory lock)
so a future change can't silently break this invariant by making the
UPDATE conditional/skippable as a perceived no-op optimization.

* fix(workflow-edges): validate edges in BATCH_ADD_BLOCKS before persisting

Real gap Cursor's PR summary flagged ('BATCH_ADD_BLOCKS edge inserts... not
fully covered by the new server pipeline'), confirmed by reading the code:
this handler bulk-inserted the edges from a block-paste/duplicate/import
payload directly into workflowEdges with zero validation — no missing-block,
protected-target, annotation-only, trigger-target, scope-boundary,
duplicate, or cycle check. A client sending edges through this operation
instead of BATCH_ADD_EDGES could bypass every rule this PR otherwise
enforces server-side, exactly the class of gap the PR exists to close.

Routes it through the same filterEdgesForPersist used by the other two
edge-add handlers. Runs after the block insert in this same handler, so the
shared helper's blocksById lookup also sees the blocks this same batch just
inserted (a transaction observes its own prior writes).

Author: waleedlatif1

apps/realtime/src/database/operations.ts

@@ -1,3 +1,7 @@
+import {
+  normalizeWorkflowBlockName,
+  RESERVED_WORKFLOW_BLOCK_NAMES,
+} from '@sim/workflow-types/workflow'
 import { getMaxExecutionTimeout } from '@/lib/core/execution-limits'
 import type { LoopType, ParallelType } from '@/lib/workflows/types'
 
@@ -151,11 +155,12 @@ export const SPECIAL_REFERENCE_PREFIXES = [
   REFERENCE.PREFIX.VARIABLE,
 ] as const
 
-export const RESERVED_BLOCK_NAMES = [
-  REFERENCE.PREFIX.LOOP,
-  REFERENCE.PREFIX.PARALLEL,
-  REFERENCE.PREFIX.VARIABLE,
-] as const
+/**
+ * Delegates to the shared implementation in `@sim/workflow-types` so the
+ * client store and the realtime persistence layer agree on the same reserved
+ * names. Values intentionally mirror REFERENCE.PREFIX.{LOOP,PARALLEL,VARIABLE} above.
+ */
+export const RESERVED_BLOCK_NAMES = RESERVED_WORKFLOW_BLOCK_NAMES
 
 export const LOOP_REFERENCE = {
   ITERATION: 'iteration',
@@ -478,12 +483,10 @@ export function escapeRegExp(value: string): string {
  * spaces and dots. Used for both block names and variable names to ensure
  * consistent matching.
  *
- * Dots are stripped because `.` is the reference path delimiter — a name like
- * "Trigger.dev 1" must normalize to "triggerdev1" so the reference
- * `<triggerdev1.output>` parses unambiguously. Dotted names could never be
- * referenced before (the first path segment cut the name at the dot), so
- * stripping dots cannot break any previously working reference.
+ * Delegates to the shared implementation in `@sim/workflow-types` so the
+ * client store and the realtime persistence layer normalize block names
+ * identically when checking for reserved/duplicate names.
  */
 export function normalizeName(name: string): string {
-  return name.toLowerCase().replace(/\s+/g, '').replace(/\./g, '')
+  return normalizeWorkflowBlockName(name)
 }

apps/sim/executor/constants.ts

@@ -12,6 +12,7 @@ import {
   WORKFLOW_OPERATIONS,
 } from '@sim/realtime-protocol/constants'
 import { generateId } from '@sim/utils/id'
+import { getWorkflowBlockNameConflict } from '@sim/workflow-types/workflow'
 import { useQueryClient } from '@tanstack/react-query'
 import { isEqual } from 'es-toolkit'
 import type { Edge } from 'reactflow'
@@ -27,7 +28,6 @@ import { isSyntheticToolSubBlockId } from '@/lib/workflows/tool-input/synthetic-
 import { useSocket } from '@/app/workspace/providers/socket-provider'
 import { getBlock } from '@/blocks'
 import { getSubBlocksDependingOnChange } from '@/blocks/utils'
-import { normalizeName, RESERVED_BLOCK_NAMES } from '@/executor/constants'
 import { invalidateDeploymentQueries } from '@/hooks/queries/deployments'
 import { useUndoRedo } from '@/hooks/use-undo-redo'
 import {
@@ -54,7 +54,11 @@ import type {
   Position,
   WorkflowState,
 } from '@/stores/workflows/workflow/types'
-import { findAllDescendantNodes, isBlockProtected } from '@/stores/workflows/workflow/utils'
+import {
+  filterAcyclicEdges,
+  findAllDescendantNodes,
+  isBlockProtected,
+} from '@/stores/workflows/workflow/utils'
 
 const logger = createLogger('CollaborativeWorkflow')
 
@@ -1028,27 +1032,26 @@ export function useCollaborativeWorkflow() {
       }
 
       const trimmedName = name.trim()
-      const normalizedNewName = normalizeName(trimmedName)
+      const currentBlocks = useWorkflowStore.getState().blocks
+      const siblingNamesById = Object.fromEntries(
+        Object.entries(currentBlocks).map(([blockId, b]) => [blockId, b.name])
+      )
+      const conflict = getWorkflowBlockNameConflict(id, trimmedName, siblingNamesById)
 
-      if (!normalizedNewName) {
+      if (conflict?.reason === 'empty') {
         logger.error('Cannot rename block to empty name')
         toast.error('Block name cannot be empty')
         return { success: false, error: 'Block name cannot be empty' }
       }
 
-      if ((RESERVED_BLOCK_NAMES as readonly string[]).includes(normalizedNewName)) {
+      if (conflict?.reason === 'reserved') {
         logger.error(`Cannot rename block to reserved name: "${trimmedName}"`)
         toast.error(`"${trimmedName}" is a reserved name and cannot be used`)
         return { success: false, error: `"${trimmedName}" is a reserved name` }
       }
 
-      const currentBlocks = useWorkflowStore.getState().blocks
-      const conflictingBlock = Object.entries(currentBlocks).find(
-        ([blockId, block]) => blockId !== id && normalizeName(block.name) === normalizedNewName
-      )
-
-      if (conflictingBlock) {
-        const conflictName = conflictingBlock[1].name
+      if (conflict?.reason === 'duplicate') {
+        const conflictName = currentBlocks[conflict.conflictingBlockId as string].name
         logger.error(`Cannot rename block to "${trimmedName}" - conflicts with "${conflictName}"`)
         toast.error(`Block name "${trimmedName}" already exists`)
         return { success: false, error: `Block name "${trimmedName}" already exists` }
@@ -1423,7 +1426,12 @@ export function useCollaborativeWorkflow() {
       const currentEdges = useWorkflowStore.getState().edges
       const validEdges = filterValidEdges(edges, blocks)
       const newEdges = filterNewEdges(validEdges, currentEdges)
-      if (newEdges.length === 0) return false
+      // Reject cyclic edges here, before they are queued for realtime/DB
+      // persistence — the local store also runs this check, but only after
+      // an unfiltered payload would already be enqueued. Filtering once
+      // here keeps the queued payload and the local store in agreement.
+      const acyclicEdges = filterAcyclicEdges(newEdges, currentEdges)
+      if (acyclicEdges.length === 0) return false
 
       const operationId = generateId()
 
@@ -1432,16 +1440,16 @@ export function useCollaborativeWorkflow() {
         operation: {
           operation: EDGES_OPERATIONS.BATCH_ADD_EDGES,
           target: OPERATION_TARGETS.EDGES,
-          payload: { edges: newEdges },
+          payload: { edges: acyclicEdges },
         },
         workflowId: activeWorkflowId || '',
         userId: session?.user?.id || 'unknown',
       })
 
-      useWorkflowStore.getState().batchAddEdges(newEdges, { skipValidation: true })
+      useWorkflowStore.getState().batchAddEdges(acyclicEdges, { skipValidation: true })
 
       if (!options?.skipUndoRedo) {
-        newEdges.forEach((edge) => undoRedo.recordAddEdge(edge.id))
+        acyclicEdges.forEach((edge) => undoRedo.recordAddEdge(edge.id))
       }
 
       return true

apps/sim/hooks/use-collaborative-workflow.ts

@@ -1,5 +1,6 @@
 import { generateId } from '@sim/utils/id'
 import { mergeSubblockStateWithValues } from '@sim/workflow-persistence/subblocks'
+import { filterUniqueWorkflowEdges } from '@sim/workflow-types/workflow'
 import type { Edge } from 'reactflow'
 import { DEFAULT_DUPLICATE_OFFSET } from '@/lib/workflows/autolayout/constants'
 import { getEffectiveBlockOutputs } from '@/lib/workflows/blocks/block-outputs'
@@ -32,16 +33,7 @@ export function filterValidEdges(edges: Edge[], blocks: Record<string, BlockStat
 }
 
 export function filterNewEdges(edgesToAdd: Edge[], currentEdges: Edge[]): Edge[] {
-  return edgesToAdd.filter((edge) => {
-    if (edge.source === edge.target) return false
-    return !currentEdges.some(
-      (e) =>
-        e.source === edge.source &&
-        e.sourceHandle === edge.sourceHandle &&
-        e.target === edge.target &&
-        e.targetHandle === edge.targetHandle
-    )
-  })
+  return filterUniqueWorkflowEdges(edgesToAdd, currentEdges)
 }
 
 export interface RegeneratedState {

apps/sim/stores/workflows/utils.ts

@@ -0,0 +1,99 @@
+import type { Edge } from 'reactflow'
+import { describe, expect, it } from 'vitest'
+import { validateEdges } from '@/stores/workflows/workflow/edge-validation'
+import type { BlockState } from '@/stores/workflows/workflow/types'
+
+function makeBlock(id: string, type: string, overrides?: Partial<BlockState>): BlockState {
+  return {
+    id,
+    type,
+    name: id,
+    position: { x: 0, y: 0 },
+    subBlocks: {},
+    outputs: {},
+    enabled: true,
+    ...overrides,
+  }
+}
+
+function makeEdge(id: string, source: string, target: string): Edge {
+  return { id, source, target, type: 'default' }
+}
+
+describe('validateEdges', () => {
+  it('accepts an edge between two root-scope blocks', () => {
+    const blocks = {
+      a: makeBlock('a', 'starter'),
+      b: makeBlock('b', 'function'),
+    }
+    const result = validateEdges([makeEdge('e1', 'a', 'b')], blocks)
+    expect(result.valid).toHaveLength(1)
+    expect(result.dropped).toHaveLength(0)
+  })
+
+  it('drops an edge referencing a missing block', () => {
+    const blocks = { a: makeBlock('a', 'starter') }
+    const result = validateEdges([makeEdge('e1', 'a', 'missing')], blocks)
+    expect(result.valid).toHaveLength(0)
+    expect(result.dropped[0].reason).toBe('edge references a missing block')
+  })
+
+  it('drops an edge touching an annotation-only (note) block', () => {
+    const blocks = {
+      a: makeBlock('a', 'note'),
+      b: makeBlock('b', 'function'),
+    }
+    const result = validateEdges([makeEdge('e1', 'a', 'b')], blocks)
+    expect(result.valid).toHaveLength(0)
+    expect(result.dropped[0].reason).toBe('edge references an annotation-only block')
+  })
+
+  it('drops an edge targeting a trigger block', () => {
+    const blocks = {
+      a: makeBlock('a', 'function'),
+      b: makeBlock('b', 'function', { triggerMode: true }),
+    }
+    const result = validateEdges([makeEdge('e1', 'a', 'b')], blocks)
+    expect(result.valid).toHaveLength(0)
+    expect(result.dropped[0].reason).toBe('trigger blocks cannot be edge targets')
+  })
+
+  it('drops an edge crossing loop scope boundaries', () => {
+    const blocks = {
+      loop: makeBlock('loop', 'loop'),
+      inner: makeBlock('inner', 'function', { data: { parentId: 'loop', extent: 'parent' } }),
+      outer: makeBlock('outer', 'function'),
+    }
+    const result = validateEdges([makeEdge('e1', 'inner', 'outer')], blocks)
+    expect(result.valid).toHaveLength(0)
+    expect(result.dropped[0].reason).toContain('different scopes')
+  })
+
+  it('accepts an edge from a loop container into its own child', () => {
+    const blocks = {
+      loop: makeBlock('loop', 'loop'),
+      inner: makeBlock('inner', 'function', { data: { parentId: 'loop', extent: 'parent' } }),
+    }
+    const result = validateEdges([makeEdge('e1', 'loop', 'inner')], blocks)
+    expect(result.valid).toHaveLength(1)
+  })
+
+  it('accepts an edge from a loop child back out to its own container', () => {
+    const blocks = {
+      loop: makeBlock('loop', 'loop'),
+      inner: makeBlock('inner', 'function', { data: { parentId: 'loop', extent: 'parent' } }),
+    }
+    const result = validateEdges([makeEdge('e1', 'inner', 'loop')], blocks)
+    expect(result.valid).toHaveLength(1)
+  })
+
+  it('accepts edges between two siblings inside the same loop', () => {
+    const blocks = {
+      loop: makeBlock('loop', 'loop'),
+      a: makeBlock('a', 'function', { data: { parentId: 'loop', extent: 'parent' } }),
+      b: makeBlock('b', 'function', { data: { parentId: 'loop', extent: 'parent' } }),
+    }
+    const result = validateEdges([makeEdge('e1', 'a', 'b')], blocks)
+    expect(result.valid).toHaveLength(1)
+  })
+})

apps/sim/stores/workflows/workflow/edge-validation.test.ts

@@ -1,6 +1,9 @@
+import {
+  getWorkflowEdgeScopeDropReason,
+  isWorkflowAnnotationOnlyBlockType,
+} from '@sim/workflow-types/workflow'
 import type { Edge } from 'reactflow'
 import { TriggerUtils } from '@/lib/workflows/triggers/triggers'
-import { isAnnotationOnlyBlock } from '@/executor/constants'
 import type { BlockState } from '@/stores/workflows/workflow/types'
 
 interface DroppedEdge {
@@ -13,40 +16,6 @@ export interface EdgeValidationResult {
   dropped: DroppedEdge[]
 }
 
-function isContainerBlock(block: BlockState | undefined): boolean {
-  return block?.type === 'loop' || block?.type === 'parallel'
-}
-
-function getParentId(block: BlockState | undefined): string | null {
-  return block?.data?.parentId ?? null
-}
-
-function getScopeDropReason(edge: Edge, blocks: Record<string, BlockState>): string | null {
-  const sourceBlock = blocks[edge.source]
-  const targetBlock = blocks[edge.target]
-
-  if (!sourceBlock || !targetBlock) {
-    return 'edge references a missing block'
-  }
-
-  const sourceParent = getParentId(sourceBlock)
-  const targetParent = getParentId(targetBlock)
-
-  if (sourceParent === targetParent) {
-    return null
-  }
-
-  if (targetParent === edge.source && isContainerBlock(sourceBlock)) {
-    return null
-  }
-
-  if (sourceParent === edge.target && isContainerBlock(targetBlock)) {
-    return null
-  }
-
-  return `blocks are in different scopes (${sourceParent ?? 'root'} -> ${targetParent ?? 'root'})`
-}
-
 export function validateEdges(
   edges: Edge[],
   blocks: Record<string, BlockState>
@@ -63,7 +32,10 @@ export function validateEdges(
       continue
     }
 
-    if (isAnnotationOnlyBlock(sourceBlock.type) || isAnnotationOnlyBlock(targetBlock.type)) {
+    if (
+      isWorkflowAnnotationOnlyBlockType(sourceBlock.type) ||
+      isWorkflowAnnotationOnlyBlockType(targetBlock.type)
+    ) {
       dropped.push({ edge, reason: 'edge references an annotation-only block' })
       continue
     }
@@ -73,7 +45,7 @@ export function validateEdges(
       continue
     }
 
-    const scopeDropReason = getScopeDropReason(edge, blocks)
+    const scopeDropReason = getWorkflowEdgeScopeDropReason(edge, blocks)
     if (scopeDropReason) {
       dropped.push({ edge, reason: scopeDropReason })
       continue