fix(workflows): validate deployment structure#4863
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Invalid block/edge snapshots are rejected with New orchestration tests assert structure failures short-circuit before schedule/webhook validation and before Reviewed by Cursor Bugbot for commit 1945237. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryAdds a shared
Confidence Score: 4/5The change is straightforward and the core guard logic is correct; the only gaps are test coverage and a small leftover check that won't cause runtime misbehavior. The shared validation helper is wired in correctly and the rejection behavior is tested. The new tests only cover error paths, so a regression that causes deploy.test.ts — both new describe blocks lack a happy-path case; deploy.ts — the pre-validation blocks check in Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| describe('performFullDeploy', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| vi.stubGlobal('fetch', vi.fn().mockResolvedValue(new Response(null, { status: 200 }))) | ||
| mockLimit.mockResolvedValue([ | ||
| { | ||
| id: 'workflow-1', | ||
| name: 'Workflow', | ||
| workspaceId: 'workspace-1', | ||
| }, | ||
| ]) | ||
| mockValidateWorkflowState.mockReturnValue({ valid: true, errors: [], warnings: [] }) | ||
| mockValidateWorkflowSchedules.mockReturnValue({ isValid: true }) | ||
| mockValidateTriggerWebhookConfigForDeploy.mockResolvedValue({ success: true }) | ||
| }) | ||
|
|
||
| it('rejects structurally invalid workflows before schedule or webhook deployment checks', async () => { | ||
| mockValidateWorkflowState.mockReturnValue({ | ||
| valid: false, | ||
| errors: ["Edge references non-existent source block 'missing-source'"], | ||
| warnings: [], | ||
| }) | ||
| mockDeployWorkflow.mockImplementation(async ({ validateWorkflowState }) => { | ||
| return validateWorkflowState({ | ||
| ...VALID_WORKFLOW_STATE, | ||
| edges: [{ id: 'edge-1', source: 'missing-source', target: 'missing-target' }], | ||
| }) | ||
| }) | ||
|
|
||
| const result = await performFullDeploy({ | ||
| workflowId: 'workflow-1', | ||
| userId: 'user-1', | ||
| workflowName: 'Workflow', | ||
| }) | ||
|
|
||
| expect(result).toEqual({ | ||
| success: false, | ||
| error: | ||
| "Invalid workflow structure: Edge references non-existent source block 'missing-source'", | ||
| errorCode: 'validation', | ||
| }) | ||
| expect(mockValidateWorkflowSchedules).not.toHaveBeenCalled() | ||
| expect(mockValidateTriggerWebhookConfigForDeploy).not.toHaveBeenCalled() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Both performFullDeploy and performActivateVersion test suites only exercise the rejection path. There are no cases where mockDeployWorkflow / mockActivateWorkflowVersion return a success payload, so there is no test that confirms a structurally valid workflow actually completes the deploy/activate flow after the new validation passes. If validateWorkflowForDeployment were accidentally changed to always return { success: false }, the existing suite would still pass. Consider adding at least one success-path case per describe block to guard the integration end-to-end.
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 by running Sim's full workflow structure validation before deployment paths persist or activate workflow deployment versions.
This adds a shared deploy-time validation helper that checks, in order:
validateWorkflowStatevalidateWorkflowSchedulesvalidateTriggerWebhookConfigForDeployThe same guard is used for:
performFullDeployperformActivateVersionWhy
Before this, deploy orchestration only validated schedules and trigger webhooks. A workflow snapshot with invalid block/edge structure could still reach deployment persistence if it passed those narrower checks. For agent workflows, that creates a reliability problem: broken canvases can become callable/deployed artifacts and fail later in harder-to-debug execution paths.
Tests
Added orchestration tests for:
Local validation:
bunx biome check apps/sim/lib/workflows/orchestration/deploy.ts apps/sim/lib/workflows/orchestration/deploy.test.tsNODE_OPTIONS='--max-old-space-size=8192' bunx tsc --noEmit --project apps/sim/tsconfig.json --pretty falseI also attempted the focused Vitest file, but this macOS environment cannot load Rollup's native optional binary:
bun run test lib/workflows/orchestration/deploy.test.ts@rollup/rollup-darwin-arm64code-signature loader failure before tests start