From e197014d822e1e95dec3fcde023f81832f5c5f43 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 8 Jul 2026 23:35:44 -0700 Subject: [PATCH] fix(agent): scope nested tool canonical-mode overrides by instance, not type Two tool entries of the same type inside an Agent block's tool-input array (e.g. two Table tools) shared a single canonical-mode override keyed by ${toolType}:${canonicalId}, so switching basic/advanced mode on one field silently switched it on every other instance of the same tool type - including at execution time, where the wrong basic/advanced value could be resolved for the second tool. Rescope the override key to the tool's position in its tool-input array (${toolIndex}:${canonicalId}) instead of its type, and thread that index through every consumer: the editor (read + write), execution (agent-handler/providers), search-index, and fork/promote remapping. --- apps/realtime/src/database/operations.ts | 33 ++++ apps/realtime/src/middleware/permissions.ts | 1 + .../components/tool-input/tool-input.tsx | 50 ++++-- .../lib/copy/copy-workflows.test.ts | 61 +++++++ .../lib/copy/copy-workflows.ts | 32 ++-- .../lib/mapping/dependent-reconfigs.test.ts | 93 ++++++++++- .../lib/mapping/dependent-reconfigs.ts | 6 +- .../lib/remap/fork-bootstrap.ts | 18 +-- .../lib/remap/remap-references.test.ts | 37 +++++ .../lib/remap/remap-references.ts | 149 ++++++++++++----- .../executor/handlers/agent/agent-handler.ts | 42 +++-- apps/sim/hooks/use-collaborative-workflow.ts | 39 +++++ .../persistence/remap-internal-ids.ts | 7 +- .../lib/workflows/search-replace/indexer.ts | 10 +- .../workflows/subblocks/visibility.test.ts | 150 +++++++++++++++++- .../sim/lib/workflows/subblocks/visibility.ts | 109 +++++++++++-- apps/sim/providers/utils.test.ts | 31 +++- apps/sim/providers/utils.ts | 23 ++- .../stores/workflows/workflow/store.test.ts | 71 +++++++++ apps/sim/stores/workflows/workflow/store.ts | 28 ++++ apps/sim/stores/workflows/workflow/types.ts | 7 + apps/sim/tools/params-resolver.ts | 2 + packages/realtime-protocol/src/constants.ts | 1 + packages/realtime-protocol/src/schemas.ts | 1 + 24 files changed, 872 insertions(+), 129 deletions(-) diff --git a/apps/realtime/src/database/operations.ts b/apps/realtime/src/database/operations.ts index 4c2735a2087..7f514bda77f 100644 --- a/apps/realtime/src/database/operations.ts +++ b/apps/realtime/src/database/operations.ts @@ -576,6 +576,39 @@ async function handleBlockOperationTx( break } + case BLOCK_OPERATIONS.REPLACE_CANONICAL_MODES: { + if (!payload.id || !payload.data?.canonicalModes) { + throw new Error('Missing required fields for replace canonical modes operation') + } + + const existingBlock = await tx + .select({ data: workflowBlocks.data }) + .from(workflowBlocks) + .where(and(eq(workflowBlocks.id, payload.id), eq(workflowBlocks.workflowId, workflowId))) + .limit(1) + + const currentData = (existingBlock?.[0]?.data as Record) || {} + + const updateResult = await tx + .update(workflowBlocks) + .set({ + data: { + ...currentData, + canonicalModes: payload.data.canonicalModes, + }, + updatedAt: new Date(), + }) + .where(and(eq(workflowBlocks.id, payload.id), eq(workflowBlocks.workflowId, workflowId))) + .returning({ id: workflowBlocks.id }) + + if (updateResult.length === 0) { + throw new Error(`Block ${payload.id} not found in workflow ${workflowId}`) + } + + logger.debug(`Replaced block canonical modes: ${payload.id}`) + break + } + case BLOCK_OPERATIONS.TOGGLE_HANDLES: { if (!payload.id || payload.horizontalHandles === undefined) { throw new Error('Missing required fields for toggle handles operation') diff --git a/apps/realtime/src/middleware/permissions.ts b/apps/realtime/src/middleware/permissions.ts index 00fc5c9580f..4f4a4296c16 100644 --- a/apps/realtime/src/middleware/permissions.ts +++ b/apps/realtime/src/middleware/permissions.ts @@ -28,6 +28,7 @@ const WRITE_OPERATIONS: string[] = [ BLOCK_OPERATIONS.UPDATE_PARENT, BLOCK_OPERATIONS.UPDATE_ADVANCED_MODE, BLOCK_OPERATIONS.UPDATE_CANONICAL_MODE, + BLOCK_OPERATIONS.REPLACE_CANONICAL_MODES, BLOCK_OPERATIONS.TOGGLE_HANDLES, // Batch block operations BLOCKS_OPERATIONS.BATCH_UPDATE_POSITIONS, diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx index 6511dde22ef..a64d37b25c7 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx @@ -106,6 +106,7 @@ import { type CanonicalModeOverrides, evaluateSubBlockCondition, isCanonicalPair, + reindexToolCanonicalModes, resolveCanonicalMode, resolveDependencyValue, type SubBlockCondition, @@ -488,7 +489,15 @@ export const ToolInput = memo(function ToolInput({ [blockId] ) ) - const { collaborativeSetBlockCanonicalMode } = useCollaborativeWorkflow() + const { collaborativeSetBlockCanonicalMode, collaborativeSetBlockCanonicalModes } = + useCollaborativeWorkflow() + const reindexCanonicalModesOnMutate = useCallback( + (oldTools: StoredTool[], newTools: StoredTool[]) => { + const next = reindexToolCanonicalModes(oldTools, newTools, canonicalModeOverrides) + if (next) collaborativeSetBlockCanonicalModes(blockId, next) + }, + [canonicalModeOverrides, collaborativeSetBlockCanonicalModes, blockId] + ) const value = isPreview ? previewValue : storeValue @@ -514,11 +523,15 @@ export const ToolInput = memo(function ToolInput({ // Uses canonical resolution so the active field (basic vs advanced) is respected. const toolCredentialId = useMemo(() => { const allBlocks = getAllBlocks() - for (const tool of selectedTools) { + for (const [toolIndex, tool] of selectedTools.entries()) { const blockConfig = allBlocks.find((b: { type: string }) => b.type === tool.type) if (!blockConfig?.subBlocks) continue const toolCanonical = buildCanonicalIndex(blockConfig.subBlocks) - const scopedOverrides = scopeCanonicalModesForTool(canonicalModeOverrides, tool.type) + const scopedOverrides = scopeCanonicalModesForTool( + canonicalModeOverrides, + toolIndex, + tool.type + ) const reactiveSubBlock = blockConfig.subBlocks.find( (sb: { reactiveCondition?: unknown }) => sb.reactiveCondition ) @@ -909,19 +922,23 @@ export const ToolInput = memo(function ToolInput({ const handleRemoveTool = useCallback( (toolIndex: number) => { if (isPreview || disabled) return - setStoreValue(selectedTools.filter((_, index) => index !== toolIndex)) + const updatedTools = selectedTools.filter((_, index) => index !== toolIndex) + reindexCanonicalModesOnMutate(selectedTools, updatedTools) + setStoreValue(updatedTools) }, - [isPreview, disabled, selectedTools, setStoreValue] + [isPreview, disabled, selectedTools, reindexCanonicalModesOnMutate, setStoreValue] ) const handleRemoveAllFromServer = useCallback( (serverId: string | undefined) => { if (isPreview || disabled || !serverId) return - setStoreValue( - selectedTools.filter((t) => !(t.type === 'mcp' && t.params?.serverId === serverId)) + const updatedTools = selectedTools.filter( + (t) => !(t.type === 'mcp' && t.params?.serverId === serverId) ) + reindexCanonicalModesOnMutate(selectedTools, updatedTools) + setStoreValue(updatedTools) }, - [isPreview, disabled, selectedTools, setStoreValue] + [isPreview, disabled, selectedTools, reindexCanonicalModesOnMutate, setStoreValue] ) const handleDeleteTool = useCallback( @@ -949,10 +966,11 @@ export const ToolInput = memo(function ToolInput({ }) if (updatedTools.length !== selectedTools.length) { + reindexCanonicalModesOnMutate(selectedTools, updatedTools) setStoreValue(updatedTools) } }, - [selectedTools, customTools, setStoreValue] + [selectedTools, customTools, reindexCanonicalModesOnMutate, setStoreValue] ) const handleParamChange = useCallback( @@ -1121,6 +1139,7 @@ export const ToolInput = memo(function ToolInput({ newTools.splice(adjustedDropIndex, 0, draggedTool) } + reindexCanonicalModesOnMutate(selectedTools, newTools) setStoreValue(newTools) setDraggedIndex(null) setDragOverIndex(null) @@ -1420,6 +1439,10 @@ export const ToolInput = memo(function ToolInput({ description: mcpTool.description, }, })) + // Diff against `filteredTools` (pre-spread, same refs as `selectedTools`) - the + // spread copy below preserves the same relative order, so this correctly reflects + // each surviving tool's new position. + reindexCanonicalModesOnMutate(selectedTools, filteredTools) setStoreValue([...filteredTools.map((t) => ({ ...t, isExpanded: false })), ...newTools]) setMcpServerDrilldown(null) setOpen(false) @@ -1650,6 +1673,7 @@ export const ToolInput = memo(function ToolInput({ customUnsupported, availableWorkflows, isToolAlreadySelected, + reindexCanonicalModesOnMutate, ]) return ( @@ -1692,7 +1716,11 @@ export const ToolInput = memo(function ToolInput({ }) : null - const toolScopedOverrides = scopeCanonicalModesForTool(canonicalModeOverrides, tool.type) + const toolScopedOverrides = scopeCanonicalModesForTool( + canonicalModeOverrides, + toolIndex, + tool.type + ) const subBlocksResult: SubBlocksForToolInput | null = !isCustomTool && !isMcpTool && currentToolId @@ -2086,7 +2114,7 @@ export const ToolInput = memo(function ToolInput({ const nextMode = canonicalMode === 'advanced' ? 'basic' : 'advanced' collaborativeSetBlockCanonicalMode( blockId, - `${tool.type}:${canonicalId}`, + `${toolIndex}:${canonicalId}`, nextMode ) }, diff --git a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts index 88d22a908bf..330fdb2a025 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts +++ b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.test.ts @@ -293,3 +293,64 @@ describe('copyWorkflowStateIntoTarget folder fallback', () => { expect(result.name).toBe('Orphaned placement') }) }) + +describe('copyWorkflowStateIntoTarget canonicalModes reindex propagation', () => { + it( + "persists a transform's reindexed canonicalModes on the copied block, and uses that " + + "SAME reindexed value (not the source's stale one) for every subsequent remap step", + async () => { + mockSaveWorkflowToNormalizedTables.mockResolvedValue({ success: true }) + const seenCanonicalModes: Array | undefined> = [] + const tx = { + insert: () => ({ values: () => Promise.resolve() }), + } as unknown as DbOrTx + + await copyWorkflowStateIntoTarget({ + tx, + targetWorkflowId: 'wf-child', + targetWorkspaceId: 'ws-target', + userId: 'target-user', + mode: 'create', + now: new Date('2026-07-01'), + sourceState: { + blocks: { + block1: { + id: 'block1', + type: 'agent', + name: 'Agent', + subBlocks: {}, + // The source's ORIGINAL (pre-drop) canonicalModes - every step after the + // transform must see the REINDEXED value below instead, not this one. + data: { canonicalModes: { '1:credential': 'advanced' } }, + }, + }, + edges: [], + loops: {}, + parallels: {}, + variables: {}, + }, + sourceMeta: { name: 'Reindex test', description: null, folderId: null, sortOrder: 0 }, + workflowIdMap: new Map(), + folderIdMap: new Map(), + nameRegistry: buildWorkflowNameRegistry([]), + // Simulates a `tool-input` drop shifting tool 1 -> 0: returns subBlocks unchanged but + // reports the reindexed canonicalModes via the callback, exactly like + // `createForkBootstrapTransform`/`createForkSubBlockTransform` do. + transformSubBlocks: (subBlocks, _blockType, canonicalModes, onCanonicalModesChanged) => { + seenCanonicalModes.push(canonicalModes) + onCanonicalModesChanged?.({ '0:credential': 'advanced' }) + return subBlocks + }, + }) + + const [, remappedState] = mockSaveWorkflowToNormalizedTables.mock.calls.at(-1)! + const persistedBlock = Object.values(remappedState.blocks)[0] as { + data?: { canonicalModes?: Record } + } + // The transform received the source's original value... + expect(seenCanonicalModes).toEqual([{ '1:credential': 'advanced' }]) + // ...and the PERSISTED block carries the reindexed one, not the stale source value. + expect(persistedBlock.data?.canonicalModes).toEqual({ '0:credential': 'advanced' }) + } + ) +}) diff --git a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts index ca6f476c85c..2fede080ea6 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts +++ b/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts @@ -12,6 +12,7 @@ import { sanitizeSubBlocksForDuplicate, } from '@/lib/workflows/persistence/remap-internal-ids' import { saveWorkflowToNormalizedTables } from '@/lib/workflows/persistence/utils' +import type { CanonicalModeOverrides } from '@/lib/workflows/subblocks/visibility' import { deriveForkBlockId, type ForkBlockIdResolver, @@ -20,6 +21,7 @@ import { applyDependentOverrides, collectClearedDependents, type NeedsConfigurationField, + type SubBlockTransform, } from '@/ee/workspace-forking/lib/remap/remap-references' import type { BlockData, @@ -33,12 +35,6 @@ import type { const logger = createLogger('WorkspaceForkCopyWorkflows') -type SubBlockTransform = ( - subBlocks: SubBlockRecord, - blockType: string, - canonicalModes?: Record -) => SubBlockRecord - interface ResolveForkFolderMappingParams { tx: DbOrTx sourceWorkspaceId: string @@ -433,13 +429,18 @@ export async function copyWorkflowStateIntoTarget( const sourceSubBlocks = (block.subBlocks ?? {}) as unknown as SubBlockRecord const sanitizedSource = sanitizeSubBlocksForDuplicate(sourceSubBlocks) let subBlocks: SubBlockRecord = sanitizedSource + // Tracks the block's live `canonicalModes` through this pass, so a `tool-input` reindex + // (a dropped custom-tool/MCP entry shifts later tools' array positions) is visible to every + // later step below that resolves a nested tool's basic/advanced mode - not just the final + // persisted `updatedData`. Starts as the source value; `transformSubBlocks` may replace it. + let activeCanonicalModes: CanonicalModeOverrides | undefined = ( + block.data as { canonicalModes?: Record } | undefined + )?.canonicalModes if (transformSubBlocks) { - subBlocks = transformSubBlocks( - subBlocks, - block.type, - (block.data as { canonicalModes?: Record } | undefined) - ?.canonicalModes - ) + subBlocks = transformSubBlocks(subBlocks, block.type, activeCanonicalModes, (next) => { + activeCanonicalModes = next + updatedData = { ...updatedData, canonicalModes: next } as BlockData + }) } if (varIdMapping.size > 0) { subBlocks = remapVariableIdsInSubBlocks(subBlocks, varIdMapping) @@ -448,9 +449,7 @@ export async function copyWorkflowStateIntoTarget( // rather than leave them pointing at the source workspace. subBlocks = remapWorkflowReferencesInSubBlocks(subBlocks, workflowIdMap, { clearUnmapped: true, - canonicalModes: ( - block.data as { canonicalModes?: Record } | undefined - )?.canonicalModes, + canonicalModes: activeCanonicalModes, }) subBlocks = remapConditionIdsInSubBlocks(subBlocks, oldBlockId, newBlockId) as SubBlockRecord @@ -477,8 +476,7 @@ export async function copyWorkflowStateIntoTarget( block.name, targetCurrent, subBlocks, - (block.data as { canonicalModes?: Record } | undefined) - ?.canonicalModes + activeCanonicalModes ) ) } diff --git a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.test.ts b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.test.ts index 47edd6330e4..50accd71868 100644 --- a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.test.ts +++ b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.test.ts @@ -411,7 +411,7 @@ describe('collectForkDependentReconfigs', () => { return undefined as unknown as BlockConfig }) // Agent block with a nested gmail tool; the dormant basic credential holds a stale id while the - // tool-scoped `gmail:credential` override (when present) marks advanced as active. + // tool-scoped `0:credential` override (when present) marks advanced as active. const agentState = (canonicalModes?: Record) => ({ blocks: { @@ -447,7 +447,7 @@ describe('collectForkDependentReconfigs', () => { // verbatim on sync, so its dependents are never cleared and no re-pick is offered. const withOverride = collectForkDependentReconfigs( [replaceItem], - new Map([['wf-src', agentState({ 'gmail:credential': 'advanced' })]]), + new Map([['wf-src', agentState({ '0:credential': 'advanced' })]]), resolve ) expect(withOverride).toEqual([]) @@ -465,6 +465,95 @@ describe('collectForkDependentReconfigs', () => { }) }) + it('regression: two same-type nested tools resolve their canonical-mode override independently', () => { + vi.mocked(getBlock).mockImplementation((type) => { + if (type === 'agent') return blockWith([{ id: 'tools', title: 'Tools', type: 'tool-input' }]) + if (type === 'gmail') + return blockWith([ + { + id: 'credential', + title: 'Credential', + type: 'oauth-input', + canonicalParamId: 'credential', + mode: 'basic', + }, + { + id: 'manualCredential', + title: 'Credential ID', + type: 'short-input', + canonicalParamId: 'credential', + mode: 'advanced', + }, + { + id: 'folder', + title: 'Label', + type: 'folder-selector', + dependsOn: ['credential'], + selectorKey: 'gmail.labels', + required: true, + }, + ]) + return undefined as unknown as BlockConfig + }) + const states = new Map([ + [ + 'wf-src', + { + blocks: { + 'block-1': { + id: 'block-1', + type: 'agent', + name: 'Block', + // Tool #0 is scoped to advanced (manual mode passes through - no re-pick, per the + // test above); tool #1 is scoped to basic (re-pick offered on its own value). Both + // are the SAME tool type ("gmail") and the SAME canonicalId ("credential"), so only + // the per-instance index can tell them apart - before the fix both shared the single + // `gmail:credential` key, which would have made tool #1 advanced-active too (and + // therefore ALSO skipped, dropping the result to zero entries instead of one). + data: { canonicalModes: { '0:credential': 'advanced', '1:credential': 'basic' } }, + subBlocks: { + tools: { + value: [ + { + type: 'gmail', + title: 'Gmail 1', + params: { + credential: 'cred-0-stale', + manualCredential: 'cred-0-active', + folder: 'INBOX', + }, + }, + { + type: 'gmail', + title: 'Gmail 2', + params: { + credential: 'cred-1-basic', + manualCredential: 'cred-1-stale', + folder: 'SENT', + }, + }, + ], + }, + }, + }, + }, + edges: [], + loops: {}, + parallels: {}, + variables: {}, + } as unknown as WorkflowState, + ], + ]) + + const result = collectForkDependentReconfigs([replaceItem], states, resolve) + + expect(result).toHaveLength(1) + expect(result[0]).toMatchObject({ + parentSourceId: 'cred-1-basic', + subBlockKey: 'tools[1].folder', + }) + }) + it('offers a nested tool selector even when the source left it empty', () => { vi.mocked(getBlock).mockImplementation((type) => { if (type === 'agent') return blockWith([{ id: 'tools', title: 'Tools', type: 'tool-input' }]) diff --git a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts index b2489c0ed86..f5684cd9b26 100644 --- a/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts +++ b/apps/sim/ee/workspace-forking/lib/mapping/dependent-reconfigs.ts @@ -319,7 +319,11 @@ export function collectForkDependentReconfigs( contextSubBlocks: toolContextSubBlocks, blockName: block.name, targetWorkflowId: item.targetWorkflowId, - canonicalModes: scopeCanonicalModesForTool(block.data?.canonicalModes, tool.type), + canonicalModes: scopeCanonicalModesForTool( + block.data?.canonicalModes, + toolIndex, + tool.type + ), resolveTargetBlockId: resolveBlockId, makeSubBlockKey: (id) => `${toolInputKey}[${toolIndex}].${id}`, makeTitle: (dependent) => dependent.title ?? dependent.id ?? '', diff --git a/apps/sim/ee/workspace-forking/lib/remap/fork-bootstrap.ts b/apps/sim/ee/workspace-forking/lib/remap/fork-bootstrap.ts index 34224a38b48..51860636a71 100644 --- a/apps/sim/ee/workspace-forking/lib/remap/fork-bootstrap.ts +++ b/apps/sim/ee/workspace-forking/lib/remap/fork-bootstrap.ts @@ -1,9 +1,8 @@ -import type { SubBlockRecord } from '@/lib/workflows/persistence/remap-internal-ids' -import type { CanonicalModeOverrides } from '@/lib/workflows/subblocks/visibility' +import type { ForkRemapKind } from '@/ee/workspace-forking/lib/remap/remap-references' import { clearDependentsOnRemap, - type ForkRemapKind, remapForkSubBlocks, + type SubBlockTransform, } from '@/ee/workspace-forking/lib/remap/remap-references' /** @@ -21,14 +20,8 @@ export type ForkCopyResolver = (kind: ForkRemapKind, sourceId: string) => string * empty; env-var `{{KEY}}` references are preserved (name-based, they resolve once * the child defines the key). */ -export function createForkBootstrapTransform( - resolveCopied: ForkCopyResolver -): ( - subBlocks: SubBlockRecord, - blockType: string, - canonicalModes?: CanonicalModeOverrides -) => SubBlockRecord { - return (subBlocks, blockType, canonicalModes) => { +export function createForkBootstrapTransform(resolveCopied: ForkCopyResolver): SubBlockTransform { + return (subBlocks, blockType, canonicalModes, onCanonicalModesChanged) => { // Every resolution at fork-create IS a copy (the resolver is the copy id map), so all // remapped keys carry copy provenance - copy-faithful dependents (column picks) survive. // `blockType`/`canonicalModes` activate the mode policy: active basic remaps, active @@ -38,11 +31,12 @@ export function createForkBootstrapTransform( canonicalModes, isCopiedTarget: (kind, sourceId) => resolveCopied(kind, sourceId) != null, }) + if (result.canonicalModes) onCanonicalModesChanged?.(result.canonicalModes) return clearDependentsOnRemap( result.subBlocks, blockType, result.remappedKeys, - canonicalModes, + result.canonicalModes ?? canonicalModes, result.copyRemappedKeys ) } diff --git a/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts b/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts index a5be6bc02da..dbc1a95ac2a 100644 --- a/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts +++ b/apps/sim/ee/workspace-forking/lib/remap/remap-references.test.ts @@ -473,6 +473,43 @@ describe('MCP block server remap follows the tool selection (optimistic verbatim expect(tool.toolId).toBe('mcp-tgt9-search_docs') }) + it( + 'regression: dropping an unresolved custom-tool reindexes the surviving tool ' + + "canonicalModes so it doesn't inherit the dropped tool's old-index mode", + () => { + vi.mocked(getBlock).mockImplementation((type) => { + if (type === 'agent') + return blockWith([{ id: 'tools', title: 'Tools', type: 'tool-input' }]) + if (type === 'table') return blockWith([]) + return undefined as unknown as BlockConfig + }) + const transform = createForkBootstrapTransform(() => null) + const onCanonicalModesChanged = vi.fn() + const result = transform( + { + tools: { + id: 'tools', + type: 'tool-input', + value: [ + // Index 0: unresolved custom-tool - fork-create always clears unresolved, so + // this entry is dropped, shifting every later tool down by one. + { type: 'custom-tool', title: 'Missing', customToolId: 'missing-tool' }, + // Index 1 -> 0 after the drop. + { type: 'table', operation: 'query_rows', params: {} }, + ], + }, + }, + 'agent', + { '1:tableId': 'advanced' }, + onCanonicalModesChanged + ) + const tools = result.tools.value as Array<{ type: string }> + expect(tools).toHaveLength(1) + expect(tools[0].type).toBe('table') + expect(onCanonicalModesChanged).toHaveBeenCalledWith({ '0:tableId': 'advanced' }) + } + ) + it('remap layer: the tool follow-rewrite is not registered as a remapped parent key', () => { // Only `server` may drive dependent clears; the followed tool must not (its own // dependent - arguments - is preserved with it). diff --git a/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts b/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts index aae84339baa..60b2663f57a 100644 --- a/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts +++ b/apps/sim/ee/workspace-forking/lib/remap/remap-references.ts @@ -25,6 +25,7 @@ import { evaluateSubBlockCondition, isCanonicalPair, isNonEmptyValue, + reindexCanonicalModesByPosition, resolveActiveCanonicalValue, resolveCanonicalMode, scopeCanonicalModesForTool, @@ -183,8 +184,29 @@ export interface RemapSubBlocksResult { * selection) can be preserved instead of cleared. Empty when no provenance was supplied. */ copyRemappedKeys: Set + /** + * The block's `canonicalModes`, reindexed to match the shifted array positions when a nested + * `tool-input` subblock dropped an unresolved custom-tool/MCP entry (see + * {@link reindexCanonicalModesByPosition}). `undefined` when nothing needed to change - the + * caller should keep the block's existing `canonicalModes` in that case. + */ + canonicalModes?: CanonicalModeOverrides } +/** + * A `copyWorkflowStateIntoTarget` subBlock transform. Returns the rewritten subBlocks (unchanged + * shape, for backward compatibility with existing callers/tests); a nested `tool-input` reindex + * (see {@link RemapSubBlocksResult.canonicalModes}) is surfaced separately via the optional + * `onCanonicalModesChanged` callback rather than the return value, so a caller that doesn't + * persist `canonicalModes` (most don't need to) can ignore it entirely. + */ +export type SubBlockTransform = ( + subBlocks: SubBlockRecord, + blockType: string, + canonicalModes?: CanonicalModeOverrides, + onCanonicalModesChanged?: (next: CanonicalModeOverrides) => void +) => SubBlockRecord + /** * The canonical-pair mode questions every fork/promote surface asks of a subblock key. * A pair is two SUBBLOCKS with different ids sharing one `canonicalParamId`; the block's @@ -349,10 +371,14 @@ interface ToolBlockRemapOptions { /** Copy provenance for a resolved target (see {@link RemapForkContext.isCopiedTarget}). */ isCopiedTarget?: (kind: ForkRemapKind, sourceId: string) => boolean /** - * The owning BLOCK's `data.canonicalModes` (keys scoped `${toolType}:${canonicalId}` for - * nested tools), so the active canonical member per pair matches the tool-input UI. + * The owning BLOCK's `data.canonicalModes` (keys scoped `${toolIndex}:${canonicalId}` for + * nested tools - by the tool's position in its tool-input array, not its type, so two tools + * of the same type don't share an override), so the active canonical member per pair matches + * the tool-input UI. */ parentCanonicalModes?: CanonicalModeOverrides + /** This tool's position in its parent's `tool-input` array - see {@link parentCanonicalModes}. */ + toolIndex?: number } /** @@ -387,7 +413,11 @@ export function remapToolBlockResources( // tool-input UI does: the block-level overrides scoped to this tool, then the value heuristic. const toolValues: Record = typeof tool.operation === 'string' ? { operation: tool.operation, ...params } : { ...params } - const scopedModes = scopeCanonicalModesForTool(opts.parentCanonicalModes, tool.type) + const scopedModes = scopeCanonicalModesForTool( + opts.parentCanonicalModes, + opts.toolIndex, + tool.type + ) const toolBlockSubBlocks = (opts.blockConfigs?.[tool.type] ?? getBlock(tool.type))?.subBlocks const gates = createCanonicalModeGates(toolBlockSubBlocks, toolValues, scopedModes) @@ -434,6 +464,7 @@ export function remapToolBlockResources( try { configs = getToolInputParamConfigs({ tool: toolView, + toolIndex: opts.toolIndex, parentCanonicalModes: opts.parentCanonicalModes, blockConfigs: opts.blockConfigs, }) @@ -612,7 +643,7 @@ interface ForkToolInputOptions { resolveMcpServerMeta?: ForkMcpServerMetaResolver /** Copy provenance for a resolved target (see {@link RemapForkContext.isCopiedTarget}). */ isCopiedTarget?: (kind: ForkRemapKind, sourceId: string) => boolean - /** Block-level canonical-mode overrides (`${toolType}:`-scoped for nested tools). */ + /** Block-level canonical-mode overrides (`${toolIndex}:`-scoped for nested tools). */ parentCanonicalModes?: CanonicalModeOverrides } @@ -623,32 +654,52 @@ interface ForkToolInputOptions { * a block tool's `params` and rewritten via {@link remapToolBlockResources}. The * MCP entry's derived `toolId` is rebuilt when the server id changes. On fork an * unresolved custom-tool/MCP tool is dropped; on promote it's kept and recorded. + * + * A dropped entry shifts every later tool's array position, so the owning block's + * `canonicalModes` (index-scoped, see {@link reindexCanonicalModesByPosition}) must be + * reindexed to match - tracked here by old/new POSITION rather than object identity, since a + * kept-but-rewritten entry (a remapped `customToolId`/`serverId`) is a clone, not the same + * reference as its source. */ function remapForkToolInputValue( value: unknown, resolve: ForkReferenceResolver, opts: ForkToolInputOptions -): unknown { +): { value: unknown; canonicalModes?: Record } { const { array, wasString } = coerceObjectArray(value) - if (!array) return value + if (!array) return { value } let changed = false - const next = array.flatMap((tool) => { - if (!isRecord(tool) || typeof tool.type !== 'string') return [tool] + const next: unknown[] = [] + const newIndexByOldIndex = new Map() + + array.forEach((tool, toolIndex) => { + const keep = (nextTool: unknown) => { + newIndexByOldIndex.set(toolIndex, next.length) + next.push(nextTool) + } + + if (!isRecord(tool) || typeof tool.type !== 'string') { + keep(tool) + return + } if (tool.type === 'custom-tool' && typeof tool.customToolId === 'string') { const target = resolve('custom-tool', tool.customToolId) opts.record?.('custom-tool', tool.customToolId, target != null) if (target != null) { if (target !== tool.customToolId) { changed = true - return [{ ...tool, customToolId: target }] + keep({ ...tool, customToolId: target }) + return } - return [tool] + keep(tool) + return } if (opts.clearUnresolved) { changed = true - return [] + return // Dropped - later tools shift down. } - return [tool] + keep(tool) + return } if (tool.type === 'mcp' && isRecord(tool.params) && typeof tool.params.serverId === 'string') { const serverId = tool.params.serverId @@ -671,21 +722,22 @@ function remapForkToolInputValue( nextParams = { ...omit(nextParams, ['serverUrl']), serverName: meta.name } if (meta.url) nextParams.serverUrl = meta.url } - return [ - { - ...tool, - params: nextParams, - toolId: toolName ? createMcpToolId(target, toolName) : tool.toolId, - }, - ] + keep({ + ...tool, + params: nextParams, + toolId: toolName ? createMcpToolId(target, toolName) : tool.toolId, + }) + return } - return [tool] + keep(tool) + return } if (opts.clearUnresolved) { changed = true - return [] + return // Dropped - later tools shift down. } - return [tool] + keep(tool) + return } const remapped = remapToolBlockResources(tool, { resolve, @@ -694,12 +746,18 @@ function remapForkToolInputValue( clearUnresolved: opts.clearUnresolved, isCopiedTarget: opts.isCopiedTarget, parentCanonicalModes: opts.parentCanonicalModes, + toolIndex, }) if (remapped !== tool) changed = true - return [remapped] + keep(remapped) }) - if (!changed) return value - return wasString ? JSON.stringify(next) : next + + const canonicalModes = reindexCanonicalModesByPosition( + newIndexByOldIndex, + opts.parentCanonicalModes + ) + if (!changed) return { value, canonicalModes } + return { value: wasString ? JSON.stringify(next) : next, canonicalModes } } /** @@ -758,6 +816,8 @@ export function remapForkSubBlocks( const copyRemappedKeys = new Set() /** MCP server ids remapped to a DIFFERENT mapped target this pass (source id -> target id). */ const mcpServerRemaps = new Map() + /** Set when a `tool-input` subblock dropped an entry, shifting later tools' positions. */ + let reindexedCanonicalModes: CanonicalModeOverrides | undefined const recordReference = (key: string, reference: ForkReference, mapped: boolean) => { if (mode !== 'promote') return @@ -884,16 +944,21 @@ export function remapForkSubBlocks( mapped ) } - value = - subBlockType === 'tool-input' - ? remapForkToolInputValue(value, resolve, { - clearUnresolved, - record, - resolveMcpServerMeta: context?.resolveMcpServerMeta, - isCopiedTarget: context?.isCopiedTarget, - parentCanonicalModes: context?.canonicalModes, - }) - : remapForkSkillInputValue(value, resolve, { clearUnresolved, record }) + if (subBlockType === 'tool-input') { + const toolInputResult = remapForkToolInputValue(value, resolve, { + clearUnresolved, + record, + resolveMcpServerMeta: context?.resolveMcpServerMeta, + isCopiedTarget: context?.isCopiedTarget, + // Build on any reindex from an earlier `tool-input` subblock on this same block + // (rare - most blocks have one), so multiple fields don't clobber each other. + parentCanonicalModes: reindexedCanonicalModes ?? context?.canonicalModes, + }) + value = toolInputResult.value + if (toolInputResult.canonicalModes) reindexedCanonicalModes = toolInputResult.canonicalModes + } else { + value = remapForkSkillInputValue(value, resolve, { clearUnresolved, record }) + } } if (value !== valueBeforeResource) remappedKeys.add(subBlockKey) @@ -954,6 +1019,7 @@ export function remapForkSubBlocks( unmapped: Array.from(unmapped.values()), remappedKeys, copyRemappedKeys, + canonicalModes: reindexedCanonicalModes, } } @@ -1156,7 +1222,7 @@ function collectClearedToolParamDependents( const gates = createCanonicalModeGates( toolConfig.subBlocks, mergedValues, - scopeCanonicalModesForTool(parentCanonicalModes, tool.type) + scopeCanonicalModesForTool(parentCanonicalModes, index, tool.type) ) const toolLabel = typeof tool.title === 'string' && tool.title ? tool.title : toolConfig.name for (const cfg of toolConfig.subBlocks) { @@ -1413,23 +1479,20 @@ export function createForkSubBlockTransform( /** Copy provenance (promote's copy-selection overlay), keeping copy-faithful dependents. */ isCopiedTarget?: (kind: ForkRemapKind, sourceId: string) => boolean } -): ( - subBlocks: SubBlockRecord, - blockType: string, - canonicalModes?: CanonicalModeOverrides -) => SubBlockRecord { - return (subBlocks, blockType, canonicalModes) => { +): SubBlockTransform { + return (subBlocks, blockType, canonicalModes, onCanonicalModesChanged) => { const result = remapSubBlocks(subBlocks, resolve, { blockType, canonicalModes, resolveMcpServerMeta: options?.resolveMcpServerMeta, isCopiedTarget: options?.isCopiedTarget, }) + if (result.canonicalModes) onCanonicalModesChanged?.(result.canonicalModes) return clearDependentsOnRemap( result.subBlocks, blockType, result.remappedKeys, - canonicalModes, + result.canonicalModes ?? canonicalModes, result.copyRemappedKeys ) } diff --git a/apps/sim/executor/handlers/agent/agent-handler.ts b/apps/sim/executor/handlers/agent/agent-handler.ts index 50e183de22f..2373b8e6527 100644 --- a/apps/sim/executor/handlers/agent/agent-handler.ts +++ b/apps/sim/executor/handlers/agent/agent-handler.ts @@ -66,6 +66,10 @@ export class AgentBlockHandler implements BlockHandler { block: SerializedBlock, inputs: AgentInputs ): Promise { + const toolIndexByRef = new Map( + (inputs.tools || []).map((tool, index) => [tool, index] as const) + ) + const filteredTools = await this.filterUnavailableMcpTools(ctx, inputs.tools || []) const filteredInputs = { ...inputs, tools: filteredTools } @@ -80,7 +84,8 @@ export class AgentBlockHandler implements BlockHandler { const formattedTools = await this.formatTools( ctx, filteredInputs.tools || [], - block.canonicalModes + block.canonicalModes, + toolIndexByRef ) const skillInputs = filteredInputs.skills ?? [] @@ -205,31 +210,38 @@ export class AgentBlockHandler implements BlockHandler { }) } + /** + * `canonicalModes` overrides are keyed by each tool's position in the ORIGINAL, unfiltered + * tools array (matching what the editor wrote), not by `tool.type` - so two tool entries of + * the same type (e.g. two Table tools) resolve independently. `toolIndexByRef` preserves that + * original position across the mcp-availability filter and the mcp/other split below, both of + * which would otherwise renumber tools by their post-filter position. + */ private async formatTools( ctx: ExecutionContext, inputTools: ToolInput[], - canonicalModes?: Record + canonicalModes?: Record, + toolIndexByRef?: Map ): Promise { if (!Array.isArray(inputTools)) return [] - const filtered = inputTools.filter((tool) => { - const usageControl = tool.usageControl || 'auto' - return usageControl !== 'none' - }) + const filtered = inputTools + .map((tool, localIndex) => ({ tool, toolIndex: toolIndexByRef?.get(tool) ?? localIndex })) + .filter(({ tool }) => (tool.usageControl || 'auto') !== 'none') const mcpTools: ToolInput[] = [] - const otherTools: ToolInput[] = [] + const otherTools: Array<{ tool: ToolInput; toolIndex: number }> = [] - for (const tool of filtered) { - if (tool.type === 'mcp') { - mcpTools.push(tool) + for (const entry of filtered) { + if (entry.tool.type === 'mcp') { + mcpTools.push(entry.tool) } else { - otherTools.push(tool) + otherTools.push(entry) } } const otherResults = await Promise.all( - otherTools.map(async (tool) => { + otherTools.map(async ({ tool, toolIndex }) => { try { if (tool.type && tool.type !== 'custom-tool') { await validateBlockType(ctx.userId, ctx.workspaceId, tool.type, ctx) @@ -237,7 +249,7 @@ export class AgentBlockHandler implements BlockHandler { if (tool.type === 'custom-tool' && (tool.schema || tool.customToolId)) { return await this.createCustomTool(ctx, tool) } - return this.transformBlockTool(ctx, tool, canonicalModes) + return this.transformBlockTool(ctx, tool, canonicalModes, toolIndex) } catch (error) { logger.error(`[AgentHandler] Error creating tool:`, { tool, error }) return null @@ -554,7 +566,8 @@ export class AgentBlockHandler implements BlockHandler { private async transformBlockTool( ctx: ExecutionContext, tool: ToolInput, - canonicalModes?: Record + canonicalModes?: Record, + toolIndex?: number ) { const transformedTool = await transformBlockTool(tool, { selectedOperation: tool.operation, @@ -567,6 +580,7 @@ export class AgentBlockHandler implements BlockHandler { }), getTool, canonicalModes, + toolIndex, }) if (transformedTool) { diff --git a/apps/sim/hooks/use-collaborative-workflow.ts b/apps/sim/hooks/use-collaborative-workflow.ts index 0bfdecbfda1..f24ed617827 100644 --- a/apps/sim/hooks/use-collaborative-workflow.ts +++ b/apps/sim/hooks/use-collaborative-workflow.ts @@ -232,6 +232,11 @@ export function useCollaborativeWorkflow() { .getState() .setBlockCanonicalMode(payload.id, payload.canonicalId, payload.canonicalMode) break + case BLOCK_OPERATIONS.REPLACE_CANONICAL_MODES: + useWorkflowStore + .getState() + .setBlockCanonicalModes(payload.id, payload.data?.canonicalModes ?? {}) + break } } else if (target === OPERATION_TARGETS.BLOCKS) { switch (operation) { @@ -1277,6 +1282,39 @@ export function useCollaborativeWorkflow() { [isBaselineDiffView, activeWorkflowId, addToQueue, session?.user?.id] ) + /** + * Wholesale-replaces `block.data.canonicalModes`, rather than merging one key like + * {@link collaborativeSetBlockCanonicalMode}. Needed to reindex nested tool-input overrides on + * reorder/removal: a merge can't atomically drop a now-stale index key, and sequential + * per-key sets can clobber each other when two tools swap positions. + */ + const collaborativeSetBlockCanonicalModes = useCallback( + (id: string, canonicalModes: Record) => { + if (isBaselineDiffView) { + return + } + + useWorkflowStore.getState().setBlockCanonicalModes(id, canonicalModes) + + if (!activeWorkflowId) { + return + } + + const operationId = generateId() + addToQueue({ + id: operationId, + operation: { + operation: BLOCK_OPERATIONS.REPLACE_CANONICAL_MODES, + target: OPERATION_TARGETS.BLOCK, + payload: { id, data: { canonicalModes } }, + }, + workflowId: activeWorkflowId, + userId: session?.user?.id || 'unknown', + }) + }, + [isBaselineDiffView, activeWorkflowId, addToQueue, session?.user?.id] + ) + const collaborativeBatchToggleBlockHandles = useCallback( (ids: string[]) => { if (isBaselineDiffView) { @@ -2166,6 +2204,7 @@ export function useCollaborativeWorkflow() { collaborativeBatchUpdateParent, collaborativeToggleBlockAdvancedMode, collaborativeSetBlockCanonicalMode, + collaborativeSetBlockCanonicalModes, collaborativeBatchToggleBlockHandles, collaborativeBatchToggleLocked, collaborativeBatchAddBlocks, diff --git a/apps/sim/lib/workflows/persistence/remap-internal-ids.ts b/apps/sim/lib/workflows/persistence/remap-internal-ids.ts index 5a42bcd4fce..f9a9634cd99 100644 --- a/apps/sim/lib/workflows/persistence/remap-internal-ids.ts +++ b/apps/sim/lib/workflows/persistence/remap-internal-ids.ts @@ -1,6 +1,9 @@ import { createLogger } from '@sim/logger' import { remapConditionBlockIds } from '@/lib/workflows/condition-ids' -import { resolveCanonicalMode } from '@/lib/workflows/subblocks/visibility' +import { + type CanonicalModeOverrides, + resolveCanonicalMode, +} from '@/lib/workflows/subblocks/visibility' import { SYSTEM_SUBBLOCK_IDS, TRIGGER_RUNTIME_SUBBLOCK_IDS } from '@/triggers/constants' const logger = createLogger('WorkflowRemapInternalIds') @@ -148,7 +151,7 @@ export function remapVariableIdsInSubBlocks( export function remapWorkflowReferencesInSubBlocks( subBlocks: SubBlockRecord, workflowIdMap: Map | undefined, - options?: { clearUnmapped?: boolean; canonicalModes?: Record } + options?: { clearUnmapped?: boolean; canonicalModes?: CanonicalModeOverrides } ): SubBlockRecord { if (!workflowIdMap?.size) return subBlocks const clearUnmapped = options?.clearUnmapped ?? false diff --git a/apps/sim/lib/workflows/search-replace/indexer.ts b/apps/sim/lib/workflows/search-replace/indexer.ts index 359dc8e7c01..bd7ac997b09 100644 --- a/apps/sim/lib/workflows/search-replace/indexer.ts +++ b/apps/sim/lib/workflows/search-replace/indexer.ts @@ -676,11 +676,14 @@ function isVisibleToolParameter(param: ToolParameterConfig, values: Record blockConfigs?: WorkflowSearchIndexerOptions['blockConfigs'] @@ -724,7 +727,11 @@ export function getToolInputParamConfigs({ if (!toolId) return genericFallback() - const scopedCanonicalModes = scopeCanonicalModesForTool(parentCanonicalModes, tool.type) + const scopedCanonicalModes = scopeCanonicalModesForTool( + parentCanonicalModes, + toolIndex, + tool.type + ) const blockConfig = tool.type !== 'custom-tool' && tool.type !== 'mcp' ? (blockConfigs?.[tool.type] ?? getBlock(tool.type)) @@ -989,6 +996,7 @@ function addToolInputMatches({ const params = getToolInputParamConfigs({ tool, + toolIndex, parentCanonicalModes, credentialTypeById, blockConfigs, diff --git a/apps/sim/lib/workflows/subblocks/visibility.test.ts b/apps/sim/lib/workflows/subblocks/visibility.test.ts index b55bfad5f54..8d6a87224b1 100644 --- a/apps/sim/lib/workflows/subblocks/visibility.test.ts +++ b/apps/sim/lib/workflows/subblocks/visibility.test.ts @@ -2,7 +2,11 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' -import { evaluateSubBlockCondition } from './visibility' +import { + evaluateSubBlockCondition, + reindexToolCanonicalModes, + scopeCanonicalModesForTool, +} from './visibility' describe('evaluateSubBlockCondition', () => { describe('simple value matching', () => { @@ -178,3 +182,147 @@ describe('evaluateSubBlockCondition', () => { }) }) }) + +describe('scopeCanonicalModesForTool', () => { + it.concurrent('returns undefined when there are no overrides', () => { + expect(scopeCanonicalModesForTool(undefined, 0)).toBeUndefined() + }) + + it.concurrent('returns undefined when toolIndex is undefined', () => { + expect(scopeCanonicalModesForTool({ '0:tableId': 'advanced' }, undefined)).toBeUndefined() + }) + + it.concurrent('strips the toolIndex prefix for the matching tool instance', () => { + const overrides = { '0:tableId': 'advanced', '1:tableId': 'basic' } + expect(scopeCanonicalModesForTool(overrides, 0)).toEqual({ tableId: 'advanced' }) + expect(scopeCanonicalModesForTool(overrides, 1)).toEqual({ tableId: 'basic' }) + }) + + it.concurrent( + 'keeps two same-type tool instances independent (regression: two Table tools on one Agent block used to share a mode)', + () => { + const overrides = { '0:tableId': 'advanced', '1:tableId': 'basic' } + // Both tools have type "table" and canonicalId "tableId" - only toolIndex disambiguates them. + expect(scopeCanonicalModesForTool(overrides, 0)).toEqual({ tableId: 'advanced' }) + expect(scopeCanonicalModesForTool(overrides, 1)).toEqual({ tableId: 'basic' }) + } + ) + + it.concurrent('returns undefined when no keys match the given toolIndex prefix', () => { + expect(scopeCanonicalModesForTool({ '1:tableId': 'advanced' }, 0)).toBeUndefined() + }) + + it.concurrent('ignores falsy override values', () => { + expect( + scopeCanonicalModesForTool({ '0:tableId': undefined as unknown as 'advanced' }, 0) + ).toBeUndefined() + }) + + it.concurrent( + 'falls back to the legacy toolType-scoped prefix when no index-scoped key matches', + () => { + // Saved before per-instance scoping shipped - must not be silently dropped. + const legacyOverrides = { 'table:tableId': 'advanced' as const } + expect(scopeCanonicalModesForTool(legacyOverrides, 0, 'table')).toEqual({ + tableId: 'advanced', + }) + expect(scopeCanonicalModesForTool(legacyOverrides, 3, 'table')).toEqual({ + tableId: 'advanced', + }) + } + ) + + it.concurrent('prefers an index-scoped key over the legacy type-scoped fallback', () => { + const overrides = { 'table:tableId': 'advanced' as const, '0:tableId': 'basic' as const } + expect(scopeCanonicalModesForTool(overrides, 0, 'table')).toEqual({ tableId: 'basic' }) + }) + + it.concurrent('does not fall back when no legacyToolType is given', () => { + expect(scopeCanonicalModesForTool({ 'table:tableId': 'advanced' }, 0)).toBeUndefined() + }) +}) + +describe('reindexToolCanonicalModes', () => { + // Generic over T - only object identity matters, so a plain marker object stands in for a + // real StoredTool/fork-parsed-tool. + const tool = (label: string) => ({ label }) + + it.concurrent('returns undefined when there are no overrides', () => { + expect(reindexToolCanonicalModes([tool('a')], [tool('a')], undefined)).toBeUndefined() + }) + + it.concurrent('returns undefined when every tool keeps its index', () => { + const a = tool('a') + const b = tool('b') + expect(reindexToolCanonicalModes([a, b], [a, b], { '0:tableId': 'advanced' })).toBeUndefined() + }) + + it.concurrent('re-keys a surviving tool overrides to its new index after a removal', () => { + const a = tool('a') + const b = tool('b') + const c = tool('c') + // Remove `a` (index 0): b shifts 1->0, c shifts 2->1. + const result = reindexToolCanonicalModes([a, b, c], [b, c], { + '1:tableId': 'advanced', + '2:tableId': 'basic', + }) + expect(result).toEqual({ '0:tableId': 'advanced', '1:tableId': 'basic' }) + }) + + it.concurrent('re-keys overrides after a drag reorder (swap)', () => { + const a = tool('a') + const b = tool('b') + // Swap a and b: a moves 0->1, b moves 1->0. A naive sequential re-key would have one + // write clobber the other since both use the same canonicalId; this must resolve both + // from the ORIGINAL snapshot into one atomic result. + const result = reindexToolCanonicalModes([a, b], [b, a], { + '0:tableId': 'advanced', + '1:tableId': 'basic', + }) + expect(result).toEqual({ '1:tableId': 'advanced', '0:tableId': 'basic' }) + }) + + it.concurrent( + 'regression: drops a removed tool old key so a later tool cannot inherit it', + () => { + const a = tool('a') + const b = tool('b') + // Remove `b` (index 1): nothing survives at index 1 in the result, so a future tool + // appended back into that slot won't silently inherit `b`'s old advanced mode. + const result = reindexToolCanonicalModes([a, b], [a], { '1:tableId': 'advanced' }) + expect(result).toEqual({}) + } + ) + + it.concurrent('drops a stale index key with no corresponding old-array position', () => { + // Simulates leftover pollution from before this fix (or an earlier missed clear): + // index 5 doesn't correspond to any tool in `oldTools` at all. + const a = tool('a') + const result = reindexToolCanonicalModes([a], [a], { + '5:tableId': 'advanced', + '0:tableId': 'basic', + }) + expect(result).toEqual({ '0:tableId': 'basic' }) + }) + + it.concurrent('carries a legacy (non-index-scoped) key through unchanged', () => { + const a = tool('a') + const b = tool('b') + // `table:tableId` isn't tied to any array position - removing/reordering tools must not + // touch it. + const result = reindexToolCanonicalModes([a, b], [b], { + '0:tableId': 'advanced', + 'table:tableId': 'basic', + }) + expect(result).toEqual({ 'table:tableId': 'basic' }) + }) + + it.concurrent('ignores falsy override values', () => { + const a = tool('a') + const b = tool('b') + const result = reindexToolCanonicalModes([a, b], [b, a], { + '0:tableId': undefined as unknown as 'advanced', + }) + expect(result).toBeUndefined() + }) +}) diff --git a/apps/sim/lib/workflows/subblocks/visibility.ts b/apps/sim/lib/workflows/subblocks/visibility.ts index 7cce5d9575e..13042c803b8 100644 --- a/apps/sim/lib/workflows/subblocks/visibility.ts +++ b/apps/sim/lib/workflows/subblocks/visibility.ts @@ -259,19 +259,11 @@ export function resolveActiveCanonicalValue( return mode === 'advanced' ? advancedValue : basicValue } -/** - * Strip the `${toolType}:` prefix from canonical-mode override keys, returning the overrides for a - * nested tool keyed by bare `canonicalId`. An agent block stores its nested tools' modes scoped as - * `${toolType}:${canonicalId}` (to avoid cross-tool collisions when tools share a `canonicalParamId`), - * so this is the canonical un-scoping primitive. Returns `undefined` when there are no overrides, no - * `toolType`, or no matching keys. - */ -export function scopeCanonicalModesForTool( - overrides: CanonicalModeOverrides | undefined, - toolType: string | undefined +/** Extract override entries matching a `${prefix}` key into a bare-`canonicalId`-keyed object. */ +function extractPrefixedModes( + overrides: CanonicalModeOverrides, + prefix: string ): CanonicalModeOverrides | undefined { - if (!overrides || !toolType) return undefined - const prefix = `${toolType}:` let scoped: CanonicalModeOverrides | undefined for (const [key, value] of Object.entries(overrides)) { if (key.startsWith(prefix) && value) { @@ -282,6 +274,99 @@ export function scopeCanonicalModesForTool( return scoped } +/** + * Strip the `${toolIndex}:` prefix from canonical-mode override keys, returning the overrides for a + * nested tool keyed by bare `canonicalId`. An agent block stores its nested tools' modes scoped as + * `${toolIndex}:${canonicalId}` — keyed by the tool's position in the `tool-input` array, not its + * `type` — so that two tool entries of the SAME type (e.g. two Table tools on one Agent block) get + * independent canonical modes instead of colliding on a shared `${toolType}:${canonicalId}` key. + * + * Falls back to the legacy `${legacyToolType}:` prefix (the pre-instance-scoping format) when no + * index-scoped key matches, so an override saved before this scoping change isn't silently dropped - + * it keeps applying (type-shared, the old behavior) until the user re-toggles it explicitly, at which + * point it's rewritten under the new index-scoped key. + * + * Returns `undefined` when there are no overrides, no `toolIndex`, and no legacy match. + */ +export function scopeCanonicalModesForTool( + overrides: CanonicalModeOverrides | undefined, + toolIndex: number | undefined, + legacyToolType?: string +): CanonicalModeOverrides | undefined { + if (!overrides) return undefined + const scoped = + toolIndex !== undefined ? extractPrefixedModes(overrides, `${toolIndex}:`) : undefined + if (scoped) return scoped + return legacyToolType ? extractPrefixedModes(overrides, `${legacyToolType}:`) : undefined +} + +const INDEX_SCOPED_KEY = /^(\d+):(.+)$/ + +/** + * Canonical-mode overrides are keyed by a tool's position in its `tool-input` array + * (`${toolIndex}:${canonicalId}`), so anything that reorders or removes tools - the editor + * (drag-reorder, remove, delete), fork/promote copy (dropping an unresolved custom-tool/MCP + * entry) - must carry each surviving tool's overrides to its new position and DROP the + * vacated index. Otherwise a saved basic/advanced choice can attach to whichever DIFFERENT + * tool later lands on that old index (e.g. a newly-added tool, always appended at the end, + * can refill a slot a removal just freed). + * + * Returns the full replacement `canonicalModes` object (for an atomic whole-map write - a + * per-key merge can't drop a key, and sequential per-key writes can clobber each other when + * two tools swap positions), or `undefined` when nothing needs to change. A legacy, + * non-index-scoped key (the `${toolType}:` fallback format) isn't tied to any array position, + * so it's carried over unchanged. + */ +export function reindexCanonicalModesByPosition( + newIndexByOldIndex: ReadonlyMap, + overrides: CanonicalModeOverrides | undefined +): Record | undefined { + if (!overrides) return undefined + + let changed = false + const result: Record = {} + for (const [key, mode] of Object.entries(overrides)) { + if (!mode) continue + const match = INDEX_SCOPED_KEY.exec(key) + if (!match) { + result[key] = mode + continue + } + const newIndex = newIndexByOldIndex.get(Number(match[1])) + if (newIndex === undefined) { + changed = true // Tool removed (or an already-stale index) - drop the key. + continue + } + if (newIndex !== Number(match[1])) changed = true + result[`${newIndex}:${match[2]}`] = mode + } + return changed ? result : undefined +} + +/** + * {@link reindexCanonicalModesByPosition}, diffing `oldTools` against `newTools` by OBJECT + * IDENTITY to derive the old-index -> new-index map. Callers must not clone the tool objects + * they keep (only filter/splice/reorder the array itself) - a kept-but-cloned tool (e.g. a + * `{ ...tool, someField: x }` spread) won't match its old reference and will be treated as + * removed. Use {@link reindexCanonicalModesByPosition} directly when a caller's remap can + * clone kept entries (fork/promote does, to rewrite a remapped id) and already knows each + * surviving old index's new position by other means (e.g. tracking it during the same pass + * that builds the new array, rather than post-hoc identity comparison). + */ +export function reindexToolCanonicalModes( + oldTools: readonly T[], + newTools: readonly T[], + overrides: CanonicalModeOverrides | undefined +): Record | undefined { + const newIndexByRef = new Map(newTools.map((tool, index) => [tool, index])) + const newIndexByOldIndex = new Map() + oldTools.forEach((tool, oldIndex) => { + const newIndex = newIndexByRef.get(tool) + if (newIndex !== undefined) newIndexByOldIndex.set(oldIndex, newIndex) + }) + return reindexCanonicalModesByPosition(newIndexByOldIndex, overrides) +} + /** * Check if a block has any standalone advanced-only fields (not part of canonical pairs). * These require the block-level advanced mode toggle to be visible. diff --git a/apps/sim/providers/utils.test.ts b/apps/sim/providers/utils.test.ts index 41f76b6c3f6..0182491436e 100644 --- a/apps/sim/providers/utils.test.ts +++ b/apps/sim/providers/utils.test.ts @@ -1564,11 +1564,12 @@ describe('transformBlockTool multi-instance unique IDs', () => { const transformTable = ( params: Record, - canonicalModes?: Record + canonicalModes?: Record, + toolIndex?: number ) => transformBlockTool( { type: 'table', operation: 'query_rows', params }, - { selectedOperation: 'query_rows', getAllBlocks, getTool, canonicalModes } + { selectedOperation: 'query_rows', getAllBlocks, getTool, canonicalModes, toolIndex } ) it('appends the table id when stored under the basic selector subblock key', async () => { @@ -1579,7 +1580,8 @@ describe('transformBlockTool multi-instance unique IDs', () => { it('appends the table id resolved from the advanced manual input', async () => { const result = await transformTable( { manualTableId: 'tbl_xyz' }, - { 'table:tableId': 'advanced' } + { '0:tableId': 'advanced' }, + 0 ) expect(result?.id).toBe('table_query_rows_tbl_xyz') }) @@ -1600,6 +1602,21 @@ describe('transformBlockTool multi-instance unique IDs', () => { const result = await transformTable({}) expect(result?.id).toBe('table_query_rows') }) + + it('regression: two Table tool instances on one Agent block resolve their canonical mode independently', async () => { + // Both tools are type "table" with canonicalId "tableId" and BOTH basic + advanced values + // populated, so only the explicit per-instance mode determines which one wins. Before the fix, + // canonicalModes was keyed by `${toolType}:${canonicalId}` (shared across every "table" tool), + // so toggling tool #0 to advanced also flipped tool #1's resolved value. + const sharedParams = { tableSelector: 'tbl_basic', manualTableId: 'tbl_advanced' } + const canonicalModes = { '0:tableId': 'advanced', '1:tableId': 'basic' } + + const first = await transformTable(sharedParams, canonicalModes, 0) + const second = await transformTable(sharedParams, canonicalModes, 1) + + expect(first?.id).toBe('table_query_rows_tbl_advanced') + expect(second?.id).toBe('table_query_rows_tbl_basic') + }) }) describe('transformBlockTool knowledge-base multi-instance unique IDs', () => { @@ -1637,11 +1654,12 @@ describe('transformBlockTool knowledge-base multi-instance unique IDs', () => { const transformKb = ( params: Record, - canonicalModes?: Record + canonicalModes?: Record, + toolIndex?: number ) => transformBlockTool( { type: 'knowledge', operation: 'search', params }, - { selectedOperation: 'search', getAllBlocks, getTool, canonicalModes } + { selectedOperation: 'search', getAllBlocks, getTool, canonicalModes, toolIndex } ) it('appends the knowledge base id when stored under the basic selector subblock key', async () => { @@ -1652,7 +1670,8 @@ describe('transformBlockTool knowledge-base multi-instance unique IDs', () => { it('appends the knowledge base id resolved from the advanced manual input', async () => { const result = await transformKb( { manualKnowledgeBaseId: 'kb_xyz' }, - { 'knowledge:knowledgeBaseId': 'advanced' } + { '0:knowledgeBaseId': 'advanced' }, + 0 ) expect(result?.id).toBe('knowledge_search_kb_xyz') }) diff --git a/apps/sim/providers/utils.ts b/apps/sim/providers/utils.ts index 487313e6564..4f7864f6024 100644 --- a/apps/sim/providers/utils.ts +++ b/apps/sim/providers/utils.ts @@ -15,8 +15,10 @@ import { import { buildCanonicalIndex, type CanonicalGroup, + type CanonicalModeOverrides, isCanonicalPair, resolveActiveCanonicalValue, + scopeCanonicalModesForTool, } from '@/lib/workflows/subblocks/visibility' import { isCustomTool } from '@/executor/constants' import { @@ -491,8 +493,7 @@ export function extractAndParseJSON(content: string): any { function resolveCanonicalResourceParams( params: Record, canonicalGroups: CanonicalGroup[], - blockType: string, - canonicalModes?: Record + scopedCanonicalModes?: CanonicalModeOverrides ): Record { if (canonicalGroups.length === 0) return params const resolved = { ...params } @@ -501,7 +502,7 @@ function resolveCanonicalResourceParams( if (existing !== undefined && existing !== null && existing !== '') continue // Route through the canonical SOT: an explicit scoped override wins, else the value heuristic - // no `?? 'basic'` (which ignored an advanced-only value when basic was empty). - const explicitMode = canonicalModes?.[`${blockType}:${group.canonicalId}`] + const explicitMode = scopedCanonicalModes?.[group.canonicalId] const chosen = resolveActiveCanonicalValue( group, params, @@ -527,9 +528,18 @@ export async function transformBlockTool( getTool: (toolId: string) => any getToolAsync?: (toolId: string) => Promise canonicalModes?: Record + /** + * Position of this tool within its parent agent block's `tool-input` array. Canonical-mode + * overrides are stored scoped by this index (`${toolIndex}:${canonicalId}`) rather than by + * `block.type`, so that two tool entries of the same type (e.g. two Table tools) don't share + * a canonical-mode override. Omit for tools with no such array position (e.g. Pi local tools). + */ + toolIndex?: number } ): Promise { - const { selectedOperation, getAllBlocks, getTool, getToolAsync, canonicalModes } = options + const { selectedOperation, getAllBlocks, getTool, getToolAsync, canonicalModes, toolIndex } = + options + const scopedCanonicalModes = scopeCanonicalModesForTool(canonicalModes, toolIndex, block.type) const blockDef = getAllBlocks().find((b: any) => b.type === block.type) if (!blockDef) { @@ -595,8 +605,7 @@ export async function transformBlockTool( const resolvedResourceParams = resolveCanonicalResourceParams( userProvidedParams, canonicalGroups, - block.type, - canonicalModes + scopedCanonicalModes ) let uniqueToolId = toolConfig.id @@ -635,7 +644,7 @@ export async function transformBlockTool( for (const group of canonicalGroups) { // Route through the canonical SOT: an explicit scoped override wins, else the value // heuristic - no `?? 'basic'` (which dropped an advanced-only value when basic was empty). - const explicitMode = canonicalModes?.[`${block.type}:${group.canonicalId}`] + const explicitMode = scopedCanonicalModes?.[group.canonicalId] const chosen = resolveActiveCanonicalValue( group, result, diff --git a/apps/sim/stores/workflows/workflow/store.test.ts b/apps/sim/stores/workflows/workflow/store.test.ts index 720fee128b8..860418bd7ee 100644 --- a/apps/sim/stores/workflows/workflow/store.test.ts +++ b/apps/sim/stores/workflows/workflow/store.test.ts @@ -769,6 +769,77 @@ describe('workflow store', () => { }) }) + describe('setBlockCanonicalMode / setBlockCanonicalModes', () => { + it('should merge a single canonical mode into an empty map', () => { + const { setBlockCanonicalMode } = useWorkflowStore.getState() + addBlock('agent1', 'agent', 'Test Agent', { x: 0, y: 0 }) + + setBlockCanonicalMode('agent1', 'credential', 'advanced') + + expect(useWorkflowStore.getState().blocks.agent1?.data?.canonicalModes).toEqual({ + credential: 'advanced', + }) + }) + + it('should merge without clobbering existing keys', () => { + const { setBlockCanonicalMode } = useWorkflowStore.getState() + addBlock('agent1', 'agent', 'Test Agent', { x: 0, y: 0 }) + + setBlockCanonicalMode('agent1', '0:tableId', 'advanced') + setBlockCanonicalMode('agent1', '1:tableId', 'basic') + + expect(useWorkflowStore.getState().blocks.agent1?.data?.canonicalModes).toEqual({ + '0:tableId': 'advanced', + '1:tableId': 'basic', + }) + }) + + it('should not throw when merging into a non-existent block', () => { + const { setBlockCanonicalMode } = useWorkflowStore.getState() + expect(() => setBlockCanonicalMode('non-existent', 'credential', 'advanced')).not.toThrow() + }) + + it('should wholesale-replace canonicalModes, dropping keys absent from the new map', () => { + const { setBlockCanonicalMode, setBlockCanonicalModes } = useWorkflowStore.getState() + addBlock('agent1', 'agent', 'Test Agent', { x: 0, y: 0 }) + setBlockCanonicalMode('agent1', '0:tableId', 'advanced') + setBlockCanonicalMode('agent1', '1:tableId', 'basic') + + // Reindex after removing tool 0: only tool 1's (now re-keyed) entry survives. + setBlockCanonicalModes('agent1', { '0:tableId': 'basic' }) + + expect(useWorkflowStore.getState().blocks.agent1?.data?.canonicalModes).toEqual({ + '0:tableId': 'basic', + }) + }) + + it('should preserve sibling data fields when replacing canonicalModes', () => { + const { setBlockCanonicalMode, setBlockCanonicalModes } = useWorkflowStore.getState() + addBlock('agent1', 'agent', 'Test Agent', { x: 0, y: 0 }) + setBlockCanonicalMode('agent1', '0:tableId', 'advanced') + useWorkflowStore.setState((state) => ({ + blocks: { + ...state.blocks, + agent1: { + ...state.blocks.agent1, + data: { ...state.blocks.agent1.data, someOtherField: 'keep-me' }, + }, + }, + })) + + setBlockCanonicalModes('agent1', {}) + + const data = useWorkflowStore.getState().blocks.agent1?.data + expect(data?.canonicalModes).toEqual({}) + expect(data?.someOtherField).toBe('keep-me') + }) + + it('should not throw when replacing canonicalModes on a non-existent block', () => { + const { setBlockCanonicalModes } = useWorkflowStore.getState() + expect(() => setBlockCanonicalModes('non-existent', { credential: 'advanced' })).not.toThrow() + }) + }) + describe('workflow state management', () => { it('should work with WorkflowBuilder for complex setups', () => { const workflowState = WorkflowBuilder.linear(3).build() diff --git a/apps/sim/stores/workflows/workflow/store.ts b/apps/sim/stores/workflows/workflow/store.ts index d7bc200a80f..75b4953a2bc 100644 --- a/apps/sim/stores/workflows/workflow/store.ts +++ b/apps/sim/stores/workflows/workflow/store.ts @@ -828,6 +828,34 @@ export const useWorkflowStore = create()( get().updateLastSaved() }, + setBlockCanonicalModes: ( + id: string, + canonicalModes: Record + ) => { + set((state) => { + const block = state.blocks[id] + if (!block) { + return state + } + + return { + blocks: { + ...state.blocks, + [id]: { + ...block, + data: { + ...block.data, + canonicalModes, + }, + }, + }, + edges: [...state.edges], + loops: { ...state.loops }, + } + }) + get().updateLastSaved() + }, + syncDynamicHandleSubblockValue: (blockId: string, subblockId: string, value: unknown) => { set((state) => { const block = state.blocks[blockId] diff --git a/apps/sim/stores/workflows/workflow/types.ts b/apps/sim/stores/workflows/workflow/types.ts index 1f32f318764..1afca6311a1 100644 --- a/apps/sim/stores/workflows/workflow/types.ts +++ b/apps/sim/stores/workflows/workflow/types.ts @@ -73,6 +73,13 @@ export interface WorkflowActions { } setBlockAdvancedMode: (id: string, advancedMode: boolean) => void setBlockCanonicalMode: (id: string, canonicalId: string, mode: 'basic' | 'advanced') => void + /** + * Wholesale-replaces `block.data.canonicalModes`, rather than merging one key like + * {@link setBlockCanonicalMode}. Needed when reindexing nested tool-input overrides on + * reorder/removal: a merge can't atomically drop a now-stale index key, and sequential + * per-key sets can clobber each other when two tools swap positions. + */ + setBlockCanonicalModes: (id: string, canonicalModes: Record) => void syncDynamicHandleSubblockValue: (blockId: string, subblockId: string, value: unknown) => void setBlockTriggerMode: (id: string, triggerMode: boolean) => void updateBlockLayoutMetrics: (id: string, dimensions: { width: number; height: number }) => void diff --git a/apps/sim/tools/params-resolver.ts b/apps/sim/tools/params-resolver.ts index 70d31b3d898..d0bd4e83097 100644 --- a/apps/sim/tools/params-resolver.ts +++ b/apps/sim/tools/params-resolver.ts @@ -5,6 +5,7 @@ import { evaluateSubBlockCondition, getCanonicalValues, isCanonicalPair, + reindexToolCanonicalModes, resolveCanonicalMode, resolveDependencyValue, type SubBlockCondition, @@ -18,6 +19,7 @@ export { type CanonicalModeOverrides, evaluateSubBlockCondition, isCanonicalPair, + reindexToolCanonicalModes, resolveCanonicalMode, resolveDependencyValue, scopeCanonicalModesForTool, diff --git a/packages/realtime-protocol/src/constants.ts b/packages/realtime-protocol/src/constants.ts index 7ba3751a34f..37074c5a807 100644 --- a/packages/realtime-protocol/src/constants.ts +++ b/packages/realtime-protocol/src/constants.ts @@ -5,6 +5,7 @@ export const BLOCK_OPERATIONS = { UPDATE_PARENT: 'update-parent', UPDATE_ADVANCED_MODE: 'update-advanced-mode', UPDATE_CANONICAL_MODE: 'update-canonical-mode', + REPLACE_CANONICAL_MODES: 'replace-canonical-modes', TOGGLE_HANDLES: 'toggle-handles', } as const diff --git a/packages/realtime-protocol/src/schemas.ts b/packages/realtime-protocol/src/schemas.ts index 030155a402d..3dd8da3bcc0 100644 --- a/packages/realtime-protocol/src/schemas.ts +++ b/packages/realtime-protocol/src/schemas.ts @@ -33,6 +33,7 @@ export const BlockOperationSchema = z.object({ BLOCK_OPERATIONS.UPDATE_PARENT, BLOCK_OPERATIONS.UPDATE_ADVANCED_MODE, BLOCK_OPERATIONS.UPDATE_CANONICAL_MODE, + BLOCK_OPERATIONS.REPLACE_CANONICAL_MODES, BLOCK_OPERATIONS.TOGGLE_HANDLES, ]), target: z.literal(OPERATION_TARGETS.BLOCK),