fix: enforce workflow validation before deploy and run#3505
fix: enforce workflow validation before deploy and run#3505elliotllliu wants to merge 1 commit intosimstudioai:mainfrom
Conversation
Previously, hasValidationErrors was hardcoded to false in panel.tsx, and the deploy API only validated schedule data. This allowed users to deploy completely broken/unconfigured workflows. Frontend (panel.tsx): - Replace hardcoded `false` with actual validation using validateWorkflowBlocks() that checks required sub-block fields and block connectivity - Deploy/Run buttons are now disabled when validation errors exist Backend (deploy/route.ts): - Call validateWorkflowBlocks() before deploying - Return 400 with detailed error messages when validation fails New file (lib/workflows/validation.ts): - validateWorkflowBlocks(): checks that all required sub-block fields are filled and non-trigger blocks have incoming connections - Supports conditional required fields (field-dependent validation) - Skips disabled blocks and container nodes (loops, parallels) Fixes simstudioai#3444
PR SummaryMedium Risk Overview A new Written by Cursor Bugbot for commit 9f05a1f. This will update automatically on new commits. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| blockName: blockState.name, | ||
| message: 'Block is not connected to any input', | ||
| }) | ||
| } |
There was a problem hiding this comment.
Blocks inside containers always fail connection validation
High Severity
The "incoming connection" check doesn't account for blocks that are children of loop/parallel containers. Blocks inside a container have blockState.data?.parentId set, but the first block inside any container won't have an incoming edge — it's the loop/parallel entry point. The validation incorrectly flags these as "not connected to any input," producing false errors that prevent deploying or running any workflow using loops or parallels. Blocks with a parentId need to be excluded from the incoming-edge requirement.
Additional Locations (1)
Greptile SummaryThis PR fixes a long-standing gap where broken or unconfigured workflows could still be deployed and run: it introduces a shared Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([User clicks Run / Deploy]) --> B{Button disabled?}
B -- "Run: checks hasValidationErrors\n(via useMemo in panel.tsx)" --> C{Has errors?}
B -- "Deploy: only checks isEmpty / canAdmin\n(validation NOT checked)" --> D[Deploy proceeds]
C -- Yes --> E[Button disabled — action blocked]
C -- No --> F[Action triggers]
F --> G[API: POST /api/workflows/id/deploy]
G --> H[loadWorkflowFromNormalizedTables]
H --> I[validateWorkflowBlocks\nblocks + edges]
I -- errors found --> J[Return 400\nwith error details]
I -- no errors --> K[validateWorkflowSchedules]
K --> L[deployWorkflow]
L --> M[200 Success]
D --> G
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const req = subBlockConfig.required | ||
| if (req === undefined || req === false) return false | ||
| if (req === true) return true |
There was a problem hiding this comment.
Function variant of required silently mis-handled
The SubBlockConfig.required type includes a third variant — a callable: ((values?: Record<string, unknown>) => { field; value; not?; and? }). The current isSubBlockRequired only handles boolean and the object variant. When required is a function:
- It is not
undefined,false, ortrue, so it falls through to the conditional-check branch. req.fieldon a function isundefined, soblockState.subBlocks[undefined as any]resolves toundefined.req.valueis alsoundefined, sofieldValue === req.value→true.- The function returns
true, treating the field as always required.
This produces false-positive validation errors for any sub-block that conditionally declares required via a function — blocking deployment of valid workflows once such blocks are introduced.
The function should be called with the current block's sub-block values:
if (typeof req === 'function') {
const values = Object.fromEntries(
Object.entries(blockState.subBlocks).map(([k, v]) => [k, v?.value])
)
const resolved = req(values)
// treat resolved like the object variant
...
}| // Check required sub-block fields | ||
| for (const subBlockConfig of blockConfig.subBlocks) { | ||
| if (!isSubBlockRequired(subBlockConfig, blockState)) continue | ||
|
|
||
| const subBlockState = blockState.subBlocks[subBlockConfig.id] | ||
| const value = subBlockState?.value | ||
| const isEmpty = | ||
| value === null || | ||
| value === undefined || | ||
| value === '' || | ||
| (Array.isArray(value) && value.length === 0) | ||
|
|
||
| if (isEmpty) { | ||
| errors.push({ | ||
| blockId, | ||
| blockName: blockState.name, | ||
| message: `Missing required field: ${subBlockConfig.title || subBlockConfig.id}`, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Multiple errors per block for the same missing field
The loop over blockConfig.subBlocks pushes one error per required-but-empty field. For a block missing three fields this generates three separate error objects. That is fine for the backend response message, but on the frontend validationErrors.length > 0 only tells the user something is wrong — there is no UI showing which blocks or fields need attention.
Consider deduplicating to at most one error per block (the first failing field), or grouping errors by blockId, so future UI work has a clean structure to display targeted feedback.
This is a style/UX suggestion; the current behavior is not broken.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| import { getWorkflowWithValues } from '@/stores/workflows' | ||
| import { useWorkflowRegistry } from '@/stores/workflows/registry/store' | ||
| import { useWorkflowStore } from '@/stores/workflows/workflow/store' | ||
| import { validateWorkflowBlocks } from '@/lib/workflows/validation' |
There was a problem hiding this comment.
Import ordering violates project alias conventions
The import of validateWorkflowBlocks on line 58 appears after the store imports (@/stores/…), breaking the established grouping order where @/lib/… utilities come before @/stores/…. Per the project's import-path rules, library utilities should be in a distinct group above store imports.
| import { validateWorkflowBlocks } from '@/lib/workflows/validation' | |
| import { validateWorkflowBlocks } from '@/lib/workflows/validation' | |
| import { useChatStore } from '@/stores/chat/store' |
(Move the @/lib/workflows/validation import up into the @/lib/… import group, before the @/stores/… group.)
Rule Used: Import patterns for the Sim application (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Fixes #3444 — Broken/unconfigured workflows can no longer be deployed or run.
Problem
panel.tsxhadhasValidationErrorshardcoded tofalse, so Deploy/Run buttons were never disabled/api/workflows/[id]/deploy) only validated schedule data, never checking if blocks were properly configured or connectedSolution
New validation module (
lib/workflows/validation.ts):validateWorkflowBlocks(blocks, edges)checks:requiredconfigs)Frontend (
panel.tsx):falsewith live validation viauseMemo+validateWorkflowBlocksBackend (
deploy/route.ts):validateWorkflowBlocks()after loading workflow state400with detailed error messages listing each validation failureChanges
apps/sim/lib/workflows/validation.tsapps/sim/app/workspace/.../panel.tsxfalseapps/sim/app/api/workflows/[id]/deploy/route.tsTest plan