Skip to content

Commit 33dfef2

Browse files
authored
fix(workflows): lint unquoted string references in JSON fields and surface lint on deploy (#8803)
* fix(workflows): lint unquoted string references in JSON fields and surface lint on deploy * fix(workflows): lint only the JSON fields the operation sends, and lint deploys as the acting user * fix(workflows): check JSON editors sent under a canonical param
1 parent 999483f commit 33dfef2

9 files changed

Lines changed: 584 additions & 20 deletions

File tree

‎apps/docs/openapi-v2-workflows.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7048,7 +7048,7 @@
70487048
"required": ["blockId", "blockName", "blockType", "field", "value", "kind", "reason"],
70497049
"additionalProperties": false
70507050
},
7051-
"description": "Credential, resource, tool, and skill references that do not resolve. These values are still persisted; they are reported, not dropped."
7051+
"description": "Credential, resource, tool, and skill references that do not resolve, and `block-output` references that will not work as written: a block that does not exist, an output field the block does not have (reason `unknown-field`), or a text output written unquoted in a JSON field (reason `unquoted-json-string`). These values are still persisted; they are reported, not dropped."
70527052
},
70537053
"tableFieldIssues": {
70547054
"type": "array",

‎apps/sim/lib/api/contracts/v2/workflows.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2917,7 +2917,7 @@ const v2WorkflowLintSchema = z
29172917
})
29182918
)
29192919
.describe(
2920-
'Credential, resource, tool, and skill references that do not resolve. These values are still persisted; they are reported, not dropped.'
2920+
'Credential, resource, tool, and skill references that do not resolve, and `block-output` references that will not work as written: a block that does not exist, an output field the block does not have (reason `unknown-field`), or a text output written unquoted in a JSON field (reason `unquoted-json-string`). These values are still persisted; they are reported, not dropped.'
29212921
),
29222922
tableFieldIssues: z
29232923
.array(

‎apps/sim/lib/workflows/application/apply-workflow-operations.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ vi.mock('@/lib/workflows/editing/validation', () => ({
8080
vi.mock('@/lib/workflows/editing/lint', () => ({
8181
collectWorkflowFieldIssues: () => [],
8282
collectDanglingBlockOutputReferences: () => [],
83+
collectUnquotedJsonStringReferences: () => [],
8384
lintEditedWorkflowState: hoisted.lintGraph,
8485
}))
8586
vi.mock('@/lib/billing/core/subscription', () => billingSubscriptionMock)

‎apps/sim/lib/workflows/application/deployments.ts‎

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,25 @@
11
import { AuditAction, AuditResourceType } from '@sim/audit'
2-
import { resolvePrincipalAttribution, toPrincipalActor } from '@sim/auth/principal'
2+
import {
3+
resolvePrincipalAttribution,
4+
resolvePrincipalSubjectUserId,
5+
toPrincipalActor,
6+
} from '@sim/auth/principal'
7+
import { createLogger } from '@sim/logger'
38
import { assertWorkflowMutable, WorkflowLockedError } from '@sim/platform-authz/workflow'
9+
import { getErrorMessage } from '@sim/utils/errors'
410
import { OrchestrationError, type OrchestrationErrorCode } from '@/lib/core/orchestration/types'
511
import { listLiveWorkflowMcpToolsForWorkflow } from '@/lib/mcp/queries'
612
import { notifyWorkflowReverted } from '@/lib/realtime/notify'
713
import { listDeployedWebhookUrls } from '@/lib/webhooks/deployed-urls'
814
import { requireWorkflowExecutionUserId } from '@/lib/workflows/application/authorization'
915
import { defineAuthorizedWorkflowUseCase } from '@/lib/workflows/application/authorized-workflow-use-case'
16+
import type { ActiveWorkflowApplicationContext } from '@/lib/workflows/application/context'
1017
import { workflowOperations } from '@/lib/workflows/application/operations'
1118
import { resolvePrincipalWorkflowContext } from '@/lib/workflows/application/principal-scope'
19+
import { withWorkflowBlockScope } from '@/lib/workflows/application/workflow-block-scope'
1220
import { checkNeedsRedeployment } from '@/lib/workflows/deployment-status'
21+
import { formatWorkflowLintMessage, hasWorkflowLintIssues } from '@/lib/workflows/editing/lint'
22+
import { buildWorkflowLintReport } from '@/lib/workflows/editing/lint-report'
1323
import {
1424
getWorkflowDeploymentSummary,
1525
performActivateVersion,
@@ -19,9 +29,12 @@ import {
1929
} from '@/lib/workflows/orchestration'
2030
import {
2131
findPreviousDeploymentVersion,
32+
loadWorkflowDeploymentVersionState,
2233
updateDeploymentVersionMetadata,
2334
} from '@/lib/workflows/persistence/utils'
2435

36+
const logger = createLogger('WorkflowDeployments')
37+
2538
export interface DeployWorkflowInput {
2639
workflowId: string
2740
assertedWorkspaceId?: string
@@ -88,6 +101,44 @@ async function requireMutableWorkflow(workflowId: string): Promise<void> {
88101
}
89102
}
90103

104+
/**
105+
* The lint findings of the version a deploy admitted, as one warning. That
106+
* version serves callers once activation completes, so it is linted even while
107+
* activation is still pending.
108+
*
109+
* Deploy does not refuse on lint: findings are advisory, and some depend on the
110+
* identity that runs the workflow. But a caller that deployed without linting
111+
* would otherwise first learn of a block that cannot run from a failed live
112+
* execution. Linting is best-effort and never fails the deploy that preceded it.
113+
*/
114+
async function deployedVersionLintWarning(
115+
context: ActiveWorkflowApplicationContext,
116+
deploymentVersionId: string | undefined,
117+
subjectUserId: string | null
118+
): Promise<string | undefined> {
119+
if (!deploymentVersionId) return undefined
120+
try {
121+
const report = await withWorkflowBlockScope(context, async () =>
122+
buildWorkflowLintReport(
123+
await loadWorkflowDeploymentVersionState(
124+
context.workflowId,
125+
deploymentVersionId,
126+
context.workspaceId
127+
),
128+
{ workflowId: context.workflowId, workspaceId: context.workspaceId, subjectUserId }
129+
)
130+
)
131+
if (!hasWorkflowLintIssues(report)) return undefined
132+
return `The version this deploy publishes has lint findings and may fail when it runs. ${formatWorkflowLintMessage(report)}`
133+
} catch (error) {
134+
logger.warn('Deployed version lint failed', {
135+
workflowId: context.workflowId,
136+
error: getErrorMessage(error),
137+
})
138+
return undefined
139+
}
140+
}
141+
91142
export const deployWorkflow = defineAuthorizedWorkflowUseCase({
92143
operation: workflowOperations.deploy,
93144
resolveContext: resolvePrincipalWorkflowContext<DeployWorkflowInput>,
@@ -108,8 +159,14 @@ export const deployWorkflow = defineAuthorizedWorkflowUseCase({
108159
idempotencyKey: input.idempotencyKey,
109160
})
110161
if (!result.success) throwDeploymentFailure(result, 'Failed to deploy workflow')
162+
const lintWarning = await deployedVersionLintWarning(
163+
context,
164+
result.deploymentVersionId,
165+
resolvePrincipalSubjectUserId(principal) ?? null
166+
)
111167
return {
112168
...result,
169+
warnings: lintWarning ? [...(result.warnings ?? []), lintWarning] : result.warnings,
113170
workflowId: context.workflowId,
114171
workspaceId: context.workspaceId,
115172
}

‎apps/sim/lib/workflows/application/workflow-deployments.test.ts‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
2626

2727
const mocks = vi.hoisted(() => ({
2828
listMcpTools: vi.fn(),
29+
buildWorkflowLintReport: vi.fn(),
2930
}))
3031

3132
vi.mock('@sim/audit', () => auditMock)
@@ -46,6 +47,10 @@ vi.mock('@/lib/mcp/queries', () => ({
4647

4748
vi.mock('@/lib/workflows/deployment-status', () => workflowDeploymentStatusMock)
4849

50+
vi.mock('@/lib/workflows/editing/lint-report', () => ({
51+
buildWorkflowLintReport: mocks.buildWorkflowLintReport,
52+
}))
53+
4954
import {
5055
activateWorkflowVersion,
5156
deployWorkflow,
@@ -78,6 +83,19 @@ const context = {
7883
billedAccountUserId: 'billing-owner-1',
7984
}
8085

86+
const cleanLint = {
87+
sources: [],
88+
sinks: [],
89+
orphanBlocks: [],
90+
emptyOutgoingPorts: [],
91+
invalidBranchPorts: [],
92+
invalidConnectionTargets: [],
93+
fieldIssues: [],
94+
unresolvedReferences: [],
95+
tableFieldIssues: [],
96+
notes: [],
97+
}
98+
8199
const adminPrincipals: Array<{ principal: Principal; actorUserId: string }> = [
82100
{
83101
principal: createSessionPrincipal({ userId: 'session-user' }),
@@ -110,6 +128,7 @@ describe('workflow deployment application use cases', () => {
110128
success: true,
111129
deployedAt: new Date('2026-08-08T00:00:00Z'),
112130
version: 4,
131+
deploymentVersionId: 'version-4',
113132
activeDeployment: null,
114133
latestDeploymentAttempt: null,
115134
warnings: [],
@@ -128,6 +147,11 @@ describe('workflow deployment application use cases', () => {
128147
version: 3,
129148
})
130149
mockRevert.mockResolvedValue({ success: true, lastSaved: 12345 })
150+
workflowsPersistenceUtilsMockFns.mockLoadWorkflowDeploymentVersionState.mockResolvedValue({
151+
blocks: {},
152+
edges: [],
153+
})
154+
mocks.buildWorkflowLintReport.mockResolvedValue(cleanLint)
131155
})
132156

133157
it.each(adminPrincipals)(
@@ -297,4 +321,69 @@ describe('workflow deployment application use cases', () => {
297321
})
298322
).rejects.toThrow('Failed to deploy workflow')
299323
})
324+
325+
/**
326+
* A draft whose Table block could never parse its row JSON deployed with an
327+
* empty `warnings`, so the caller first learned of it from a failed live run.
328+
*/
329+
describe('lint findings on the deployed graph', () => {
330+
const unquotedRowJson = {
331+
...cleanLint,
332+
unresolvedReferences: [
333+
{
334+
blockId: 'insert',
335+
blockName: 'Insert Order',
336+
field: 'data',
337+
value: ['<start.order_id>'],
338+
kind: 'block-output' as const,
339+
reason: 'unquoted-json-string: quote it',
340+
},
341+
],
342+
}
343+
344+
it('reports them as a deploy warning, linted as the acting user', async () => {
345+
mockDeploy.mockResolvedValueOnce({
346+
success: true,
347+
version: 4,
348+
deploymentVersionId: 'version-4',
349+
warnings: [
350+
'Deployment activation completed, and post-activation notifications are queued.',
351+
],
352+
})
353+
mocks.buildWorkflowLintReport.mockImplementation(
354+
async (_graph: unknown, scope: { subjectUserId: string | null }) =>
355+
scope.subjectUserId === 'session-user' ? unquotedRowJson : cleanLint
356+
)
357+
358+
const result = await deployWorkflow.execute({
359+
principal: createSessionPrincipal({ userId: 'session-user' }),
360+
input: { workflowId: 'workflow-1', requestId: 'request-10' },
361+
})
362+
363+
expect(result.warnings).toHaveLength(2)
364+
expect(result.warnings?.[1]).toContain('"Insert Order".data <start.order_id>')
365+
})
366+
367+
it('adds nothing for a clean graph', async () => {
368+
const result = await deployWorkflow.execute({
369+
principal: createSessionPrincipal(),
370+
input: { workflowId: 'workflow-1', requestId: 'request-11' },
371+
})
372+
373+
expect(result.warnings).toEqual([])
374+
})
375+
376+
it('never fails or blocks a deploy when lint cannot run', async () => {
377+
workflowsPersistenceUtilsMockFns.mockLoadWorkflowDeploymentVersionState.mockRejectedValueOnce(
378+
new Error('version read failed')
379+
)
380+
381+
const result = await deployWorkflow.execute({
382+
principal: createSessionPrincipal(),
383+
input: { workflowId: 'workflow-1', requestId: 'request-12' },
384+
})
385+
386+
expect(result).toMatchObject({ success: true, version: 4, warnings: [] })
387+
})
388+
})
300389
})

0 commit comments

Comments
 (0)