Skip to content

Commit f69d12b

Browse files
committed
fix(copilot): mint ids for nested blocks instead of storing the model's handle
workflow_blocks.id is a global primary key, but normalizeBlockIdsInOperations only minted UUIDs for an operation's own block_id. Children arrive keyed by the model's handle under params.nestedNodes and were persisted verbatim, so a workflow adding a child named "waitPoll" collided with whichever workflow stored that name first and the whole save failed with 23505. The pre-insert delete is scoped to the workflow's own rows, so it cannot clear the conflicting row and every retry fails identically. - claim non-UUID nestedNodes keys recursively, at any container depth - rewrite child connections and nested containers through the same mapping - log the Postgres cause on a failed save; Drizzle's message is only "Failed query: <sql> params: <...>" and the SQLSTATE lives on error.cause
1 parent 74212ef commit f69d12b

3 files changed

Lines changed: 165 additions & 36 deletions

File tree

apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.test.ts

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
/**
22
* @vitest-environment node
33
*/
4+
import { isValidUuid } from '@sim/utils/id'
45
import { describe, expect, it, vi } from 'vitest'
56
import {
67
applyTriggerConfigToBlockSubblocks,
78
createBlockFromParams,
89
filterDisallowedTools,
10+
normalizeBlockIdsInOperations,
911
normalizeSubblockValue,
1012
} from '@/lib/copilot/tools/server/workflow/edit-workflow/builders'
1113

@@ -245,3 +247,80 @@ describe('applyTriggerConfigToBlockSubblocks', () => {
245247
})
246248
})
247249
})
250+
251+
describe('normalizeBlockIdsInOperations', () => {
252+
it('mints UUIDs for nested child ids so a model handle never becomes the global block primary key', () => {
253+
const { normalizedOperations, idMapping } = normalizeBlockIdsInOperations([
254+
{
255+
operation_type: 'add',
256+
block_id: 'pollLoop',
257+
params: {
258+
type: 'loop',
259+
nestedNodes: {
260+
waitPoll: { type: 'wait' },
261+
setStatus: { type: 'variables' },
262+
},
263+
},
264+
},
265+
] as any)
266+
267+
const nestedNodes = (normalizedOperations[0] as any).params.nestedNodes
268+
const childIds = Object.keys(nestedNodes)
269+
270+
expect(childIds).toHaveLength(2)
271+
for (const childId of childIds) {
272+
expect(isValidUuid(childId)).toBe(true)
273+
}
274+
expect(childIds).not.toContain('waitPoll')
275+
expect(childIds).not.toContain('setStatus')
276+
expect(idMapping.get('waitPoll')).toBe(childIds[0])
277+
expect(idMapping.get('setStatus')).toBe(childIds[1])
278+
})
279+
280+
it('remaps children of nested containers and the sibling references they carry', () => {
281+
const { normalizedOperations, idMapping } = normalizeBlockIdsInOperations([
282+
{
283+
operation_type: 'add',
284+
block_id: 'outerLoop',
285+
params: {
286+
type: 'loop',
287+
nestedNodes: {
288+
innerLoop: {
289+
type: 'parallel',
290+
nestedNodes: {
291+
deepChild: { type: 'agent' },
292+
},
293+
},
294+
sibling: {
295+
type: 'function',
296+
connections: { success: 'deepChild' },
297+
},
298+
},
299+
},
300+
},
301+
] as any)
302+
303+
const outer = (normalizedOperations[0] as any).params.nestedNodes
304+
const innerId = idMapping.get('innerLoop') as string
305+
const deepId = idMapping.get('deepChild') as string
306+
const siblingId = idMapping.get('sibling') as string
307+
308+
expect(isValidUuid(deepId)).toBe(true)
309+
expect(Object.keys(outer[innerId].nestedNodes)).toEqual([deepId])
310+
expect(outer[siblingId].connections.success).toBe(deepId)
311+
})
312+
313+
it('leaves ids that are already UUIDs untouched', () => {
314+
const existing = '11111111-2222-4333-8444-555555555555'
315+
const { normalizedOperations, idMapping } = normalizeBlockIdsInOperations([
316+
{
317+
operation_type: 'add',
318+
block_id: existing,
319+
params: { type: 'loop', nestedNodes: {} },
320+
},
321+
] as any)
322+
323+
expect(idMapping.size).toBe(0)
324+
expect((normalizedOperations[0] as any).block_id).toBe(existing)
325+
})
326+
})

apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts

Lines changed: 81 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -705,6 +705,9 @@ export function filterDisallowedTools(
705705
* The LLM may generate human-readable IDs like "web_search" or "research_agent"
706706
* which need to be converted to proper UUIDs for database compatibility.
707707
*
708+
* Runs in two passes: every id is claimed before any reference is rewritten, so a
709+
* reference can point at an id claimed by a later operation.
710+
*
708711
* Returns the normalized operations and a mapping from old IDs to new UUIDs.
709712
*/
710713
export function normalizeBlockIdsInOperations(operations: EditWorkflowOperation[]): {
@@ -714,14 +717,38 @@ export function normalizeBlockIdsInOperations(operations: EditWorkflowOperation[
714717
const logger = createLogger('EditWorkflowServerTool')
715718
const idMapping = new Map<string, string>()
716719

717-
// First pass: collect all non-UUID block_ids from add/insert operations
720+
const claimId = (id: string | undefined) => {
721+
if (!id || isValidUuid(id) || idMapping.has(id)) return
722+
const newId = generateId()
723+
idMapping.set(id, newId)
724+
logger.debug('Normalizing block ID', { oldId: id, newId })
725+
}
726+
727+
/**
728+
* Children arrive keyed by the model's own handle under `params.nestedNodes`,
729+
* and containers nest arbitrarily deep. Every level must be claimed here or the
730+
* handle is persisted verbatim as `workflow_blocks.id`, which is a global
731+
* primary key — a name another workflow already stored collides on insert.
732+
*
733+
* Only reached for `add`/`insert_into_subflow`. `edit` also creates children via
734+
* `mergeNestedNodesForParent`, but there a child matching an existing block by
735+
* *name* keeps that block's id, so claiming its handle would repoint sibling
736+
* references at an id no block was created under. That path needs the id minted
737+
* at creation with an alias recorded for reference resolution, not a wider gate
738+
* here.
739+
*/
740+
const claimNestedNodeIds = (nestedNodes: Record<string, any> | undefined) => {
741+
if (!nestedNodes) return
742+
for (const [childId, childBlock] of Object.entries(nestedNodes)) {
743+
claimId(childId)
744+
claimNestedNodeIds(childBlock?.nestedNodes)
745+
}
746+
}
747+
718748
for (const op of operations) {
719749
if (op.operation_type === 'add' || op.operation_type === 'insert_into_subflow') {
720-
if (op.block_id && !isValidUuid(op.block_id)) {
721-
const newId = generateId()
722-
idMapping.set(op.block_id, newId)
723-
logger.debug('Normalizing block ID', { oldId: op.block_id, newId })
724-
}
750+
claimId(op.block_id)
751+
claimNestedNodeIds(op.params?.nestedNodes)
725752
}
726753
}
727754

@@ -740,7 +767,52 @@ export function normalizeBlockIdsInOperations(operations: EditWorkflowOperation[
740767
return idMapping.get(id) ?? id
741768
}
742769

743-
// Second pass: update all references to use new UUIDs
770+
const normalizeConnections = (connections: Record<string, any>): Record<string, any> => {
771+
const normalizedConnections: Record<string, any> = {}
772+
for (const [handle, targets] of Object.entries<any>(connections)) {
773+
if (typeof targets === 'string') {
774+
normalizedConnections[handle] = replaceId(targets)
775+
} else if (Array.isArray(targets)) {
776+
normalizedConnections[handle] = targets.map((t) => {
777+
if (typeof t === 'string') return replaceId(t)
778+
if (t && typeof t === 'object' && t.block) {
779+
return { ...t, block: replaceId(t.block) }
780+
}
781+
return t
782+
})
783+
} else if (targets && typeof targets === 'object' && targets.block) {
784+
normalizedConnections[handle] = { ...targets, block: replaceId(targets.block) }
785+
} else {
786+
normalizedConnections[handle] = targets
787+
}
788+
}
789+
return normalizedConnections
790+
}
791+
792+
/**
793+
* A child's `connections` may name sibling children, not just top-level blocks,
794+
* so nested references resolve through the same flat id mapping.
795+
*/
796+
const normalizeNestedNodes = (nestedNodes: Record<string, any>): Record<string, any> => {
797+
const normalizedNestedNodes: Record<string, any> = {}
798+
for (const [childId, childBlock] of Object.entries<any>(nestedNodes)) {
799+
const newChildId = replaceId(childId) ?? childId
800+
normalizedNestedNodes[newChildId] =
801+
childBlock && typeof childBlock === 'object'
802+
? {
803+
...childBlock,
804+
...(childBlock.connections && {
805+
connections: normalizeConnections(childBlock.connections),
806+
}),
807+
...(childBlock.nestedNodes && {
808+
nestedNodes: normalizeNestedNodes(childBlock.nestedNodes),
809+
}),
810+
}
811+
: childBlock
812+
}
813+
return normalizedNestedNodes
814+
}
815+
744816
const normalizedOperations = operations.map((op) => {
745817
const normalized: EditWorkflowOperation = {
746818
...op,
@@ -755,37 +827,12 @@ export function normalizeBlockIdsInOperations(operations: EditWorkflowOperation[
755827
normalized.params.subflowId = replaceId(normalized.params.subflowId)
756828
}
757829

758-
// Update connection references
759830
if (normalized.params.connections) {
760-
const normalizedConnections: Record<string, any> = {}
761-
for (const [handle, targets] of Object.entries(normalized.params.connections)) {
762-
if (typeof targets === 'string') {
763-
normalizedConnections[handle] = replaceId(targets)
764-
} else if (Array.isArray(targets)) {
765-
normalizedConnections[handle] = targets.map((t) => {
766-
if (typeof t === 'string') return replaceId(t)
767-
if (t && typeof t === 'object' && t.block) {
768-
return { ...t, block: replaceId(t.block) }
769-
}
770-
return t
771-
})
772-
} else if (targets && typeof targets === 'object' && (targets as any).block) {
773-
normalizedConnections[handle] = { ...targets, block: replaceId((targets as any).block) }
774-
} else {
775-
normalizedConnections[handle] = targets
776-
}
777-
}
778-
normalized.params.connections = normalizedConnections
831+
normalized.params.connections = normalizeConnections(normalized.params.connections)
779832
}
780833

781-
// Update nestedNodes block IDs
782834
if (normalized.params.nestedNodes) {
783-
const normalizedNestedNodes: Record<string, any> = {}
784-
for (const [childId, childBlock] of Object.entries(normalized.params.nestedNodes)) {
785-
const newChildId = replaceId(childId) ?? childId
786-
normalizedNestedNodes[newChildId] = childBlock
787-
}
788-
normalized.params.nestedNodes = normalizedNestedNodes
835+
normalized.params.nestedNodes = normalizeNestedNodes(normalized.params.nestedNodes)
789836
}
790837
}
791838

apps/sim/lib/workflows/persistence/utils.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import {
88
import { credential } from '@sim/db/schema'
99
import { createLogger } from '@sim/logger'
1010
import { getActiveWorkflowContext } from '@sim/platform-authz/workflow'
11-
import { getErrorMessage } from '@sim/utils/errors'
11+
import { describeError, getErrorMessage, getPostgresConstraintName } from '@sim/utils/errors'
1212
import { generateId } from '@sim/utils/id'
1313
import {
1414
loadWorkflowFromNormalizedTablesRaw,
@@ -616,7 +616,10 @@ export async function saveWorkflowToNormalizedTables(
616616
})
617617
} catch (error) {
618618
const message = getErrorMessage(error, 'Failed to save workflow state')
619-
logger.error(`Error saving workflow ${workflowId} to normalized tables:`, error)
619+
logger.error(`Error saving workflow ${workflowId} to normalized tables:`, error, {
620+
cause: describeError(error),
621+
constraint: getPostgresConstraintName(error),
622+
})
620623
return { success: false, error: message }
621624
}
622625
}

0 commit comments

Comments
 (0)