From f69d12be731eb2963ddf76c9396e08577fae5da0 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 12 Aug 2026 09:40:25 -0700 Subject: [PATCH 1/2] 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: params: <...>" and the SQLSTATE lives on error.cause --- .../workflow/edit-workflow/builders.test.ts | 79 ++++++++++++ .../server/workflow/edit-workflow/builders.ts | 115 ++++++++++++------ apps/sim/lib/workflows/persistence/utils.ts | 7 +- 3 files changed, 165 insertions(+), 36 deletions(-) diff --git a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.test.ts b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.test.ts index baf77b90ea3..30e375477cc 100644 --- a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.test.ts +++ b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.test.ts @@ -1,11 +1,13 @@ /** * @vitest-environment node */ +import { isValidUuid } from '@sim/utils/id' import { describe, expect, it, vi } from 'vitest' import { applyTriggerConfigToBlockSubblocks, createBlockFromParams, filterDisallowedTools, + normalizeBlockIdsInOperations, normalizeSubblockValue, } from '@/lib/copilot/tools/server/workflow/edit-workflow/builders' @@ -245,3 +247,80 @@ describe('applyTriggerConfigToBlockSubblocks', () => { }) }) }) + +describe('normalizeBlockIdsInOperations', () => { + it('mints UUIDs for nested child ids so a model handle never becomes the global block primary key', () => { + const { normalizedOperations, idMapping } = normalizeBlockIdsInOperations([ + { + operation_type: 'add', + block_id: 'pollLoop', + params: { + type: 'loop', + nestedNodes: { + waitPoll: { type: 'wait' }, + setStatus: { type: 'variables' }, + }, + }, + }, + ] as any) + + const nestedNodes = (normalizedOperations[0] as any).params.nestedNodes + const childIds = Object.keys(nestedNodes) + + expect(childIds).toHaveLength(2) + for (const childId of childIds) { + expect(isValidUuid(childId)).toBe(true) + } + expect(childIds).not.toContain('waitPoll') + expect(childIds).not.toContain('setStatus') + expect(idMapping.get('waitPoll')).toBe(childIds[0]) + expect(idMapping.get('setStatus')).toBe(childIds[1]) + }) + + it('remaps children of nested containers and the sibling references they carry', () => { + const { normalizedOperations, idMapping } = normalizeBlockIdsInOperations([ + { + operation_type: 'add', + block_id: 'outerLoop', + params: { + type: 'loop', + nestedNodes: { + innerLoop: { + type: 'parallel', + nestedNodes: { + deepChild: { type: 'agent' }, + }, + }, + sibling: { + type: 'function', + connections: { success: 'deepChild' }, + }, + }, + }, + }, + ] as any) + + const outer = (normalizedOperations[0] as any).params.nestedNodes + const innerId = idMapping.get('innerLoop') as string + const deepId = idMapping.get('deepChild') as string + const siblingId = idMapping.get('sibling') as string + + expect(isValidUuid(deepId)).toBe(true) + expect(Object.keys(outer[innerId].nestedNodes)).toEqual([deepId]) + expect(outer[siblingId].connections.success).toBe(deepId) + }) + + it('leaves ids that are already UUIDs untouched', () => { + const existing = '11111111-2222-4333-8444-555555555555' + const { normalizedOperations, idMapping } = normalizeBlockIdsInOperations([ + { + operation_type: 'add', + block_id: existing, + params: { type: 'loop', nestedNodes: {} }, + }, + ] as any) + + expect(idMapping.size).toBe(0) + expect((normalizedOperations[0] as any).block_id).toBe(existing) + }) +}) diff --git a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts index 524567f7f96..d24ce6b54ef 100644 --- a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts +++ b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts @@ -705,6 +705,9 @@ export function filterDisallowedTools( * The LLM may generate human-readable IDs like "web_search" or "research_agent" * which need to be converted to proper UUIDs for database compatibility. * + * Runs in two passes: every id is claimed before any reference is rewritten, so a + * reference can point at an id claimed by a later operation. + * * Returns the normalized operations and a mapping from old IDs to new UUIDs. */ export function normalizeBlockIdsInOperations(operations: EditWorkflowOperation[]): { @@ -714,14 +717,38 @@ export function normalizeBlockIdsInOperations(operations: EditWorkflowOperation[ const logger = createLogger('EditWorkflowServerTool') const idMapping = new Map() - // First pass: collect all non-UUID block_ids from add/insert operations + const claimId = (id: string | undefined) => { + if (!id || isValidUuid(id) || idMapping.has(id)) return + const newId = generateId() + idMapping.set(id, newId) + logger.debug('Normalizing block ID', { oldId: id, newId }) + } + + /** + * Children arrive keyed by the model's own handle under `params.nestedNodes`, + * and containers nest arbitrarily deep. Every level must be claimed here or the + * handle is persisted verbatim as `workflow_blocks.id`, which is a global + * primary key — a name another workflow already stored collides on insert. + * + * Only reached for `add`/`insert_into_subflow`. `edit` also creates children via + * `mergeNestedNodesForParent`, but there a child matching an existing block by + * *name* keeps that block's id, so claiming its handle would repoint sibling + * references at an id no block was created under. That path needs the id minted + * at creation with an alias recorded for reference resolution, not a wider gate + * here. + */ + const claimNestedNodeIds = (nestedNodes: Record | undefined) => { + if (!nestedNodes) return + for (const [childId, childBlock] of Object.entries(nestedNodes)) { + claimId(childId) + claimNestedNodeIds(childBlock?.nestedNodes) + } + } + for (const op of operations) { if (op.operation_type === 'add' || op.operation_type === 'insert_into_subflow') { - if (op.block_id && !isValidUuid(op.block_id)) { - const newId = generateId() - idMapping.set(op.block_id, newId) - logger.debug('Normalizing block ID', { oldId: op.block_id, newId }) - } + claimId(op.block_id) + claimNestedNodeIds(op.params?.nestedNodes) } } @@ -740,7 +767,52 @@ export function normalizeBlockIdsInOperations(operations: EditWorkflowOperation[ return idMapping.get(id) ?? id } - // Second pass: update all references to use new UUIDs + const normalizeConnections = (connections: Record): Record => { + const normalizedConnections: Record = {} + for (const [handle, targets] of Object.entries(connections)) { + if (typeof targets === 'string') { + normalizedConnections[handle] = replaceId(targets) + } else if (Array.isArray(targets)) { + normalizedConnections[handle] = targets.map((t) => { + if (typeof t === 'string') return replaceId(t) + if (t && typeof t === 'object' && t.block) { + return { ...t, block: replaceId(t.block) } + } + return t + }) + } else if (targets && typeof targets === 'object' && targets.block) { + normalizedConnections[handle] = { ...targets, block: replaceId(targets.block) } + } else { + normalizedConnections[handle] = targets + } + } + return normalizedConnections + } + + /** + * A child's `connections` may name sibling children, not just top-level blocks, + * so nested references resolve through the same flat id mapping. + */ + const normalizeNestedNodes = (nestedNodes: Record): Record => { + const normalizedNestedNodes: Record = {} + for (const [childId, childBlock] of Object.entries(nestedNodes)) { + const newChildId = replaceId(childId) ?? childId + normalizedNestedNodes[newChildId] = + childBlock && typeof childBlock === 'object' + ? { + ...childBlock, + ...(childBlock.connections && { + connections: normalizeConnections(childBlock.connections), + }), + ...(childBlock.nestedNodes && { + nestedNodes: normalizeNestedNodes(childBlock.nestedNodes), + }), + } + : childBlock + } + return normalizedNestedNodes + } + const normalizedOperations = operations.map((op) => { const normalized: EditWorkflowOperation = { ...op, @@ -755,37 +827,12 @@ export function normalizeBlockIdsInOperations(operations: EditWorkflowOperation[ normalized.params.subflowId = replaceId(normalized.params.subflowId) } - // Update connection references if (normalized.params.connections) { - const normalizedConnections: Record = {} - for (const [handle, targets] of Object.entries(normalized.params.connections)) { - if (typeof targets === 'string') { - normalizedConnections[handle] = replaceId(targets) - } else if (Array.isArray(targets)) { - normalizedConnections[handle] = targets.map((t) => { - if (typeof t === 'string') return replaceId(t) - if (t && typeof t === 'object' && t.block) { - return { ...t, block: replaceId(t.block) } - } - return t - }) - } else if (targets && typeof targets === 'object' && (targets as any).block) { - normalizedConnections[handle] = { ...targets, block: replaceId((targets as any).block) } - } else { - normalizedConnections[handle] = targets - } - } - normalized.params.connections = normalizedConnections + normalized.params.connections = normalizeConnections(normalized.params.connections) } - // Update nestedNodes block IDs if (normalized.params.nestedNodes) { - const normalizedNestedNodes: Record = {} - for (const [childId, childBlock] of Object.entries(normalized.params.nestedNodes)) { - const newChildId = replaceId(childId) ?? childId - normalizedNestedNodes[newChildId] = childBlock - } - normalized.params.nestedNodes = normalizedNestedNodes + normalized.params.nestedNodes = normalizeNestedNodes(normalized.params.nestedNodes) } } diff --git a/apps/sim/lib/workflows/persistence/utils.ts b/apps/sim/lib/workflows/persistence/utils.ts index c9313ef325d..0e91fba50b0 100644 --- a/apps/sim/lib/workflows/persistence/utils.ts +++ b/apps/sim/lib/workflows/persistence/utils.ts @@ -8,7 +8,7 @@ import { import { credential } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getActiveWorkflowContext } from '@sim/platform-authz/workflow' -import { getErrorMessage } from '@sim/utils/errors' +import { describeError, getErrorMessage, getPostgresConstraintName } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' import { loadWorkflowFromNormalizedTablesRaw, @@ -616,7 +616,10 @@ export async function saveWorkflowToNormalizedTables( }) } catch (error) { const message = getErrorMessage(error, 'Failed to save workflow state') - logger.error(`Error saving workflow ${workflowId} to normalized tables:`, error) + logger.error(`Error saving workflow ${workflowId} to normalized tables:`, error, { + cause: describeError(error), + constraint: getPostgresConstraintName(error), + }) return { success: false, error: message } } } From 7894c0ba7a17b74e0ae9c40d9ec97a0cbb814531 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 12 Aug 2026 09:46:52 -0700 Subject: [PATCH 2/2] fix(copilot): warn when an edit batch reuses one block handle Two declarations sharing a handle collapse to a single block, because references naming that handle are ambiguous and the flat id mapping has no way to express a per-declaration-site id. Behavior is unchanged; the warn makes the case visible in production instead of silent. --- .../tools/server/workflow/edit-workflow/builders.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts index d24ce6b54ef..ce61e8fd784 100644 --- a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts +++ b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts @@ -718,7 +718,17 @@ export function normalizeBlockIdsInOperations(operations: EditWorkflowOperation[ const idMapping = new Map() const claimId = (id: string | undefined) => { - if (!id || isValidUuid(id) || idMapping.has(id)) return + if (!id || isValidUuid(id)) return + if (idMapping.has(id)) { + /* + * Two declarations share one handle. References naming it are already + * ambiguous, so both resolve to whichever block is written last. Warned + * rather than disambiguated because splitting them needs ids minted per + * declaration site, which the flat reference mapping cannot express. + */ + logger.warn('Duplicate block handle in edit batch; declarations will collapse', { id }) + return + } const newId = generateId() idMapping.set(id, newId) logger.debug('Normalizing block ID', { oldId: id, newId })