fix(workflow-edges): enforce edge/block validation server-side, not just client-side by waleedlatif1 · Pull Request #5571 · simstudioai/sim

waleedlatif1 · 2026-07-10T20:44:46Z

Summary

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 workflow_edges, so it reappeared after refresh
Root cause: cycle detection (wouldCreateCycle) — and, on audit, several other edge/block validation rules (scope-boundary crossing, annotation-only-block edges, duplicate edges, trigger-block targets, reserved/duplicate block names) — only ever ran in the client Zustand store, never in apps/realtime's DB write path, which is the actual source of truth on reload
Moved these into @sim/workflow-types (importable by both apps/sim and apps/realtime) so the client store, the collaborative queueing layer, and realtime's persistence layer all share one implementation instead of independently (and silently) drifting
Wired the shared checks into apps/realtime/src/database/operations.ts's edge-add (single + batch) and block-rename handlers as the authoritative gate
Client-side call sites, error messages, and rule ordering are unchanged — this is a server-side enforcement gap fix, not a UX change

Type of Change

Bug fix

Testing

apps/sim/stores/workflows/workflow/store.test.ts, edge-validation.test.ts (new), utils.test.ts — 139+ tests covering cycle rejection (single + same-batch), duplicate edges, scope-boundary edges, annotation-only edges, trigger-target edges, and reserved/duplicate block names
Full executor test suite (1029 tests) passes, confirming the shared normalizeName/RESERVED_BLOCK_NAMES delegation is behavior-identical
tsc --noEmit clean on apps/sim and apps/realtime; bun run scripts/check-monorepo-boundaries.ts and check-realtime-prune-graph.ts both pass
Tested manually in the workflow editor: dragging Table → Agent to close a cycle no longer persists after refresh

Checklist

Code follows project style guidelines
Self-reviewed my changes
Tests added/updated and passing
No new warnings introduced
I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…ust 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

vercel · 2026-07-10T20:44:52Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		Jul 10, 2026 10:09pm

cursor · 2026-07-10T20:44:56Z

PR Summary

Medium Risk
Changes the authoritative persistence path for workflow_edges and block names; behavior should match the client but any remaining client/server rule gaps (e.g. registry-only trigger blocks on the server) could still allow edge cases.

Overview
Fixes invalid workflow graph state surviving refresh by enforcing the same edge and block-rename rules on the realtime DB write path that the editor already applied only in the client.

@sim/workflow-types now owns cycle detection (wouldCreateCycle, filterAcyclicEdges), duplicate/self-loop edge filtering (filterUniqueWorkflowEdges), loop/parallel scope checks, annotation-only and trigger-target rules, and block rename conflicts (getWorkflowBlockNameConflict, normalizeWorkflowBlockName, reserved names). The sim store, collaborative hook, and executor constants delegate to these helpers instead of duplicating logic.

apps/realtime adds filterEdgesForPersist and uses it for single/batch edge adds and batch block adds (after inserts in-tx), plus name conflict checks on UPDATE_NAME. Invalid edges are dropped with structured logging rather than inserted blindly.

Client alignment: collaborativeBatchAddEdges filters cycles before queuing; batchAddEdges uses shared acyclic/unique filters; validateEdges uses shared scope/annotation helpers. New/expanded tests cover edge validation and store behavior.

Note: Server-side trigger detection remains the portable subset (triggerMode, legacy starter); registry-based trigger blocks stay client-enforced only, as documented in code.

^{Reviewed by Cursor Bugbot for commit f75cd5e. Configure here.}

greptile-apps · 2026-07-10T20:48:37Z

Greptile Summary

This PR moves workflow edge and block-name validation into shared code used by the client and realtime persistence paths. The main changes are:

Shared helpers for cycle, duplicate, scope, trigger, annotation-only, and block-name checks.
Realtime validation before single-edge, batch-edge, and block-batch edge inserts.
Client store and collaborative queueing updates to use the same edge filtering logic.
Tests for duplicate edges, empty handles, cycles, scope rules, trigger targets, and reserved names.

Confidence Score: 5/5

This looks safe to merge.

No blocking issues found in the changed code.

Important Files Changed

Filename	Overview
apps/realtime/src/database/operations.ts	Adds server-side validation before workflow edge persistence and documents the transaction serialization point.
packages/workflow-types/src/workflow.ts	Adds shared workflow validation helpers for edge filtering and block-name conflicts.
apps/sim/hooks/use-collaborative-workflow.ts	Filters queued collaborative edge payloads with the shared acyclic-edge logic.
apps/sim/stores/workflows/workflow/store.ts	Updates store-side edge and rename validation to delegate to shared helpers.
apps/sim/stores/workflows/workflow/edge-validation.ts	Uses shared scope and annotation-only checks while keeping client trigger detection in place.

_{Reviews (6): Last reviewed commit: "fix(workflow-edges): validate edges in B..." | Re-trigger Greptile}

- 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)

waleedlatif1 · 2026-07-10T20:53:05Z

@greptile

waleedlatif1 · 2026-07-10T20:53:06Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 878b027. Configure here.}

… 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)

waleedlatif1 · 2026-07-10T21:01:13Z

@greptile

waleedlatif1 · 2026-07-10T21:01:15Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit b959202. Configure here.}

…e 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).

waleedlatif1 · 2026-07-10T21:32:54Z

Ran an independent /simplify pass (4 parallel review agents: reuse, simplification, efficiency, altitude) before treating this as done, on top of the already-clean Greptile 5/5 / Cursor Bugbot review:

Efficiency: clean, no changes. Verified the sequential blocksById-then-edges query ordering in the realtime handlers is a deliberate cheap-work-first short-circuit (not a missed Promise.all), and the DFS cycle check / batch dedup are correctly bounded for this domain's actual scale (tens–hundreds of blocks/edges).
Simplification + altitude (real finding, fixed in c1ce0a0): handleEdgeOperationTx (single ADD) and handleEdgesOperationTx (BATCH_ADD_EDGES) in operations.ts hand-inlined the identical 6-step validation pipeline and each re-fetched blocksById/existingEdgesForCycleCheck independently — two copies of one rule in the same file, which is exactly the client/server drift class this PR closes everywhere else. Extracted filterEdgesForPersist so both call one path. Net -63 lines. This also fixed a real bug it surfaced: droppedCounts was keyed by the scope-boundary check's free-text, block-id-bearing message, so it could never aggregate across edges — now keyed by a stable 'scope boundary' reason.
Reuse (real finding, fixed in c1ce0a0): use-collaborative-workflow.ts's collaborativeUpdateBlockName still hand-rolled the empty/reserved/duplicate name-conflict checks this PR centralized as getWorkflowBlockNameConflict (already adopted by store.ts). Switched it over — which also fixed a latent check-order mismatch (this pre-check ran reserved-before-duplicate; store.ts's real gate runs duplicate-before-reserved after this PR, so the two could disagree on which toast a name that was both reserved and duplicate would show).
Altitude, no code change, flagging for the record: the trigger-target check on the realtime path (isKnownWorkflowTriggerBlock) only catches the legacy triggerMode/starter case, not modern category-based trigger blocks — that's a documented, deliberate architectural boundary (apps/realtime can't import apps/sim/blocks), not a bandaid. Worth a one-line caveat in the PR description that server-side enforcement closes the gap for legacy trigger targets specifically, not full parity with client-side trigger detection.

Re-verified after the fix: type-check, lint, monorepo-boundaries check, and all 112 apps/realtime tests + 91 apps/sim workflow-store/edge-validation tests pass.

waleedlatif1 · 2026-07-10T21:53:46Z

@greptile

waleedlatif1 · 2026-07-10T21:53:50Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit c1ce0a0. Configure here.}

…nvariant 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.

…ting 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).

waleedlatif1 · 2026-07-10T22:09:24Z

Two more things surfaced while tracing every workflowEdges insert site in operations.ts after Cursor's summary flagged BATCH_ADD_BLOCKS — one fixed (f75cd5e), one deliberately not fixed here:

Fixed: BATCH_ADD_BLOCKS (block paste/duplicate/import) inserted its edges with zero validation — confirmed by reading the handler, not just from the summary. Routed through filterEdgesForPersist, same as the other two edge-add paths.

Found, not fixed — WORKFLOW_OPERATIONS.REPLACE_STATE: this operation (full graph replace, sent via enqueueReplaceWorkflowState in apps/sim/lib/workflows/operations/socket-operations.ts) also bulk-inserts edges unvalidated. It's real and client-reachable, not dead code — the client does call normalizeWorkflowState before enqueuing, but that only runs validateEdges (missing-block/protected/annotation/scope), not the cycle or duplicate checks, and nothing stops a client from emitting the raw socket operation directly, bypassing that anyway (the entire premise of this PR). I'm not fixing it in this PR: this operation deletes and replaces the entire workflow atomically, and I don't have enough certainty about its callers' failure-mode expectations (undo/redo restore, version revert, diff-apply) to know whether silently dropping invalid edges — the pattern used everywhere else in this file — is correct here, versus rejecting the whole replace, versus something else. Guessing at that and shipping it would be worse than leaving it open. Flagging clearly so it gets tracked and fixed deliberately rather than silently patched.

Noted, not touched — insertAutoConnectEdge (line ~271): also unvalidated, but it's genuinely dead code — no callers anywhere in apps/realtime or apps/sim. Not a live gap, but worth deleting in a follow-up so it doesn't get wired up later without anyone noticing it lacks validation.

Re-verified after the BATCH_ADD_BLOCKS fix: type-check, lint, monorepo-boundaries, and all 112 apps/realtime tests pass.

waleedlatif1 · 2026-07-10T22:17:02Z

@greptile

waleedlatif1 · 2026-07-10T22:17:07Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit f75cd5e. Configure here.}

waleedlatif1 · 2026-07-10T22:21:44Z

@greptile