diff --git a/.github/workflows/test-build.yml b/.github/workflows/test-build.yml index 456472576fb..7c802be4e4c 100644 --- a/.github/workflows/test-build.yml +++ b/.github/workflows/test-build.yml @@ -90,6 +90,16 @@ jobs: echo "✅ All feature flags are properly configured" + - name: Check subblock ID stability + run: | + if [ "${{ github.event_name }}" = "pull_request" ]; then + BASE_REF="origin/${{ github.base_ref }}" + git fetch --depth=1 origin "${{ github.base_ref }}" 2>/dev/null || true + else + BASE_REF="HEAD~1" + fi + bun run apps/sim/scripts/check-subblock-id-stability.ts "$BASE_REF" + - name: Lint code run: bun run lint:check diff --git a/apps/sim/lib/workflows/migrations/subblock-migrations.test.ts b/apps/sim/lib/workflows/migrations/subblock-migrations.test.ts new file mode 100644 index 00000000000..952e9ee3e60 --- /dev/null +++ b/apps/sim/lib/workflows/migrations/subblock-migrations.test.ts @@ -0,0 +1,183 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import type { BlockState } from '@/stores/workflows/workflow/types' +import { migrateSubblockIds } from './subblock-migrations' + +function makeBlock(overrides: Partial & { type: string }): BlockState { + return { + id: 'block-1', + name: 'Test', + position: { x: 0, y: 0 }, + subBlocks: {}, + outputs: {}, + enabled: true, + ...overrides, + } as BlockState +} + +describe('migrateSubblockIds', () => { + describe('knowledge block', () => { + it('should rename knowledgeBaseId to knowledgeBaseSelector', () => { + const input: Record = { + b1: makeBlock({ + type: 'knowledge', + subBlocks: { + operation: { id: 'operation', type: 'dropdown', value: 'search' }, + knowledgeBaseId: { + id: 'knowledgeBaseId', + type: 'knowledge-base-selector', + value: 'kb-uuid-123', + }, + }, + }), + } + + const { blocks, migrated } = migrateSubblockIds(input) + + expect(migrated).toBe(true) + expect(blocks['b1'].subBlocks['knowledgeBaseSelector']).toEqual({ + id: 'knowledgeBaseSelector', + type: 'knowledge-base-selector', + value: 'kb-uuid-123', + }) + expect(blocks['b1'].subBlocks['knowledgeBaseId']).toBeUndefined() + expect(blocks['b1'].subBlocks['operation'].value).toBe('search') + }) + + it('should prefer new key when both old and new exist', () => { + const input: Record = { + b1: makeBlock({ + type: 'knowledge', + subBlocks: { + knowledgeBaseId: { + id: 'knowledgeBaseId', + type: 'knowledge-base-selector', + value: 'stale-kb', + }, + knowledgeBaseSelector: { + id: 'knowledgeBaseSelector', + type: 'knowledge-base-selector', + value: 'fresh-kb', + }, + }, + }), + } + + const { blocks, migrated } = migrateSubblockIds(input) + + expect(migrated).toBe(true) + expect(blocks['b1'].subBlocks['knowledgeBaseSelector'].value).toBe('fresh-kb') + expect(blocks['b1'].subBlocks['knowledgeBaseId']).toBeUndefined() + }) + + it('should not touch blocks that already use the new key', () => { + const input: Record = { + b1: makeBlock({ + type: 'knowledge', + subBlocks: { + knowledgeBaseSelector: { + id: 'knowledgeBaseSelector', + type: 'knowledge-base-selector', + value: 'kb-uuid', + }, + }, + }), + } + + const { blocks, migrated } = migrateSubblockIds(input) + + expect(migrated).toBe(false) + expect(blocks['b1'].subBlocks['knowledgeBaseSelector'].value).toBe('kb-uuid') + }) + }) + + it('should not mutate the input blocks', () => { + const input: Record = { + b1: makeBlock({ + type: 'knowledge', + subBlocks: { + knowledgeBaseId: { + id: 'knowledgeBaseId', + type: 'knowledge-base-selector', + value: 'kb-uuid', + }, + }, + }), + } + + const { blocks } = migrateSubblockIds(input) + + expect(input['b1'].subBlocks['knowledgeBaseId']).toBeDefined() + expect(blocks['b1'].subBlocks['knowledgeBaseSelector']).toBeDefined() + expect(blocks).not.toBe(input) + }) + + it('should skip blocks with no registered migrations', () => { + const input: Record = { + b1: makeBlock({ + type: 'function', + subBlocks: { + code: { id: 'code', type: 'code', value: 'console.log("hi")' }, + }, + }), + } + + const { blocks, migrated } = migrateSubblockIds(input) + + expect(migrated).toBe(false) + expect(blocks['b1'].subBlocks['code'].value).toBe('console.log("hi")') + }) + + it('should migrate multiple blocks in one pass', () => { + const input: Record = { + b1: makeBlock({ + id: 'b1', + type: 'knowledge', + subBlocks: { + knowledgeBaseId: { + id: 'knowledgeBaseId', + type: 'knowledge-base-selector', + value: 'kb-1', + }, + }, + }), + b2: makeBlock({ + id: 'b2', + type: 'knowledge', + subBlocks: { + knowledgeBaseId: { + id: 'knowledgeBaseId', + type: 'knowledge-base-selector', + value: 'kb-2', + }, + }, + }), + b3: makeBlock({ + id: 'b3', + type: 'function', + subBlocks: { + code: { id: 'code', type: 'code', value: '' }, + }, + }), + } + + const { blocks, migrated } = migrateSubblockIds(input) + + expect(migrated).toBe(true) + expect(blocks['b1'].subBlocks['knowledgeBaseSelector'].value).toBe('kb-1') + expect(blocks['b2'].subBlocks['knowledgeBaseSelector'].value).toBe('kb-2') + expect(blocks['b3'].subBlocks['code']).toBeDefined() + }) + + it('should handle blocks with empty subBlocks', () => { + const input: Record = { + b1: makeBlock({ type: 'knowledge', subBlocks: {} }), + } + + const { migrated } = migrateSubblockIds(input) + + expect(migrated).toBe(false) + }) +}) diff --git a/apps/sim/lib/workflows/migrations/subblock-migrations.ts b/apps/sim/lib/workflows/migrations/subblock-migrations.ts new file mode 100644 index 00000000000..fffdfdc9b60 --- /dev/null +++ b/apps/sim/lib/workflows/migrations/subblock-migrations.ts @@ -0,0 +1,90 @@ +import { createLogger } from '@sim/logger' +import type { BlockState } from '@/stores/workflows/workflow/types' + +const logger = createLogger('SubblockMigrations') + +/** + * Maps old subblock IDs to their current equivalents per block type. + * + * When a subblock is renamed in a block definition, old deployed/saved states + * still carry the value under the previous key. Without this mapping the + * serializer silently drops the value, breaking execution. + * + * Format: { blockType: { oldSubblockId: newSubblockId } } + */ +export const SUBBLOCK_ID_MIGRATIONS: Record> = { + knowledge: { + knowledgeBaseId: 'knowledgeBaseSelector', + }, +} + +/** + * Migrates legacy subblock IDs inside a single block's subBlocks map. + * Returns a new subBlocks record if anything changed, or the original if not. + */ +function migrateBlockSubblockIds( + subBlocks: Record, + renames: Record +): { subBlocks: Record; migrated: boolean } { + let migrated = false + + for (const oldId of Object.keys(renames)) { + if (oldId in subBlocks) { + migrated = true + break + } + } + + if (!migrated) return { subBlocks, migrated: false } + + const result = { ...subBlocks } + + for (const [oldId, newId] of Object.entries(renames)) { + if (!(oldId in result)) continue + + if (newId in result) { + delete result[oldId] + continue + } + + const oldEntry = result[oldId] + result[newId] = { ...oldEntry, id: newId } + delete result[oldId] + } + + return { subBlocks: result, migrated: true } +} + +/** + * Applies subblock-ID migrations to every block in a workflow. + * Returns a new blocks record with migrated subBlocks where needed. + */ +export function migrateSubblockIds(blocks: Record): { + blocks: Record + migrated: boolean +} { + let anyMigrated = false + const result: Record = {} + + for (const [blockId, block] of Object.entries(blocks)) { + const renames = SUBBLOCK_ID_MIGRATIONS[block.type] + if (!renames || !block.subBlocks) { + result[blockId] = block + continue + } + + const { subBlocks, migrated } = migrateBlockSubblockIds(block.subBlocks, renames) + if (migrated) { + logger.info('Migrated legacy subblock IDs', { + blockId: block.id, + blockType: block.type, + }) + anyMigrated = true + result[blockId] = { ...block, subBlocks } + } else { + result[blockId] = block + } + } + + return { blocks: result, migrated: anyMigrated } +} diff --git a/apps/sim/lib/workflows/persistence/utils.ts b/apps/sim/lib/workflows/persistence/utils.ts index b9d70021d9c..c94883d6539 100644 --- a/apps/sim/lib/workflows/persistence/utils.ts +++ b/apps/sim/lib/workflows/persistence/utils.ts @@ -14,6 +14,7 @@ import { and, desc, eq, inArray, sql } from 'drizzle-orm' import type { Edge } from 'reactflow' import { v4 as uuidv4 } from 'uuid' import type { DbOrTx } from '@/lib/db/types' +import { migrateSubblockIds } from '@/lib/workflows/migrations/subblock-migrations' import { sanitizeAgentToolsInBlocks } from '@/lib/workflows/sanitization/validation' import type { BlockState, Loop, Parallel, WorkflowState } from '@/stores/workflows/workflow/types' import { SUBFLOW_TYPES } from '@/stores/workflows/workflow/types' @@ -113,10 +114,10 @@ export async function loadDeployedWorkflowState( resolvedWorkspaceId = wfRow?.workspaceId ?? undefined } - const resolvedBlocks = state.blocks || {} - const { blocks: migratedBlocks } = resolvedWorkspaceId - ? await migrateCredentialIds(resolvedBlocks, resolvedWorkspaceId) - : { blocks: resolvedBlocks } + const { blocks: migratedBlocks } = await applyBlockMigrations( + state.blocks || {}, + resolvedWorkspaceId + ) return { blocks: migratedBlocks, @@ -133,6 +134,50 @@ export async function loadDeployedWorkflowState( } } +interface MigrationContext { + blocks: Record + workspaceId?: string + migrated: boolean +} + +type BlockMigration = (ctx: MigrationContext) => MigrationContext | Promise + +function createMigrationPipeline(migrations: BlockMigration[]) { + return async ( + blocks: Record, + workspaceId?: string + ): Promise<{ blocks: Record; migrated: boolean }> => { + let ctx: MigrationContext = { blocks, workspaceId, migrated: false } + for (const migration of migrations) { + ctx = await migration(ctx) + } + return { blocks: ctx.blocks, migrated: ctx.migrated } + } +} + +const applyBlockMigrations = createMigrationPipeline([ + (ctx) => { + const { blocks } = sanitizeAgentToolsInBlocks(ctx.blocks) + return { ...ctx, blocks } + }, + + (ctx) => ({ + ...ctx, + blocks: migrateAgentBlocksToMessagesFormat(ctx.blocks), + }), + + async (ctx) => { + if (!ctx.workspaceId) return ctx + const { blocks, migrated } = await migrateCredentialIds(ctx.blocks, ctx.workspaceId) + return { ...ctx, blocks, migrated: ctx.migrated || migrated } + }, + + (ctx) => { + const { blocks, migrated } = migrateSubblockIds(ctx.blocks) + return { ...ctx, blocks, migrated: ctx.migrated || migrated } + }, +]) + /** * Migrates agent blocks from old format (systemPrompt/userPrompt) to new format (messages array) * This ensures backward compatibility for workflows created before the messages-input refactor. @@ -356,22 +401,16 @@ export async function loadWorkflowFromNormalizedTables( blocksMap[block.id] = assembled }) - // Sanitize any invalid custom tools in agent blocks to prevent client crashes - const { blocks: sanitizedBlocks } = sanitizeAgentToolsInBlocks(blocksMap) - - // Migrate old agent block format (systemPrompt/userPrompt) to new messages array format - const migratedBlocks = migrateAgentBlocksToMessagesFormat(sanitizedBlocks) - - // Migrate legacy account.id → credential.id in OAuth subblocks - const { blocks: credMigratedBlocks, migrated: credentialsMigrated } = workflowRow?.workspaceId - ? await migrateCredentialIds(migratedBlocks, workflowRow.workspaceId) - : { blocks: migratedBlocks, migrated: false } + const { blocks: finalBlocks, migrated } = await applyBlockMigrations( + blocksMap, + workflowRow?.workspaceId ?? undefined + ) - if (credentialsMigrated) { + if (migrated) { Promise.resolve().then(async () => { try { - for (const [blockId, block] of Object.entries(credMigratedBlocks)) { - if (block.subBlocks !== migratedBlocks[blockId]?.subBlocks) { + for (const [blockId, block] of Object.entries(finalBlocks)) { + if (block.subBlocks !== blocksMap[blockId]?.subBlocks) { await db .update(workflowBlocks) .set({ subBlocks: block.subBlocks, updatedAt: new Date() }) @@ -381,7 +420,7 @@ export async function loadWorkflowFromNormalizedTables( } } } catch (err) { - logger.warn('Failed to persist credential ID migration', { workflowId, error: err }) + logger.warn('Failed to persist block migrations', { workflowId, error: err }) } }) } @@ -422,13 +461,13 @@ export async function loadWorkflowFromNormalizedTables( forEachItems: (config as Loop).forEachItems ?? '', whileCondition: (config as Loop).whileCondition ?? '', doWhileCondition: (config as Loop).doWhileCondition ?? '', - enabled: credMigratedBlocks[subflow.id]?.enabled ?? true, + enabled: finalBlocks[subflow.id]?.enabled ?? true, } loops[subflow.id] = loop - if (credMigratedBlocks[subflow.id]) { - const block = credMigratedBlocks[subflow.id] - credMigratedBlocks[subflow.id] = { + if (finalBlocks[subflow.id]) { + const block = finalBlocks[subflow.id] + finalBlocks[subflow.id] = { ...block, data: { ...block.data, @@ -449,7 +488,7 @@ export async function loadWorkflowFromNormalizedTables( (config as Parallel).parallelType === 'collection' ? (config as Parallel).parallelType : 'count', - enabled: credMigratedBlocks[subflow.id]?.enabled ?? true, + enabled: finalBlocks[subflow.id]?.enabled ?? true, } parallels[subflow.id] = parallel } else { @@ -458,7 +497,7 @@ export async function loadWorkflowFromNormalizedTables( }) return { - blocks: credMigratedBlocks, + blocks: finalBlocks, edges: edgesArray, loops, parallels, diff --git a/apps/sim/scripts/check-subblock-id-stability.ts b/apps/sim/scripts/check-subblock-id-stability.ts new file mode 100644 index 00000000000..87e70f90d35 --- /dev/null +++ b/apps/sim/scripts/check-subblock-id-stability.ts @@ -0,0 +1,170 @@ +#!/usr/bin/env bun + +/** + * CI check: detect subblock ID renames that would break deployed workflows. + * + * Compares the current block registry against the parent commit. + * If any subblock ID was removed from a block, it must have a corresponding + * entry in SUBBLOCK_ID_MIGRATIONS — otherwise this script exits non-zero. + * + * Usage: + * bun run apps/sim/scripts/check-subblock-id-stability.ts [base-ref] + * + * base-ref defaults to HEAD~1. In a PR CI pipeline, pass the merge base: + * bun run apps/sim/scripts/check-subblock-id-stability.ts origin/main + */ + +import { execSync } from 'child_process' +import { SUBBLOCK_ID_MIGRATIONS } from '@/lib/workflows/migrations/subblock-migrations' +import { getAllBlocks } from '@/blocks/registry' + +const baseRef = process.argv[2] || 'HEAD~1' + +const gitRoot = execSync('git rev-parse --show-toplevel', { encoding: 'utf-8' }).trim() +const gitOpts = { encoding: 'utf-8' as const, cwd: gitRoot } + +type IdMap = Record> + +/** + * Extracts subblock IDs from the `subBlocks: [ ... ]` section of a block + * definition. Only grabs the top-level `id:` of each subblock object — + * ignores nested IDs inside `options`, `columns`, etc. + */ +function extractSubBlockIds(source: string): string[] { + const startIdx = source.indexOf('subBlocks:') + if (startIdx === -1) return [] + + const bracketStart = source.indexOf('[', startIdx) + if (bracketStart === -1) return [] + + const ids: string[] = [] + let braceDepth = 0 + let bracketDepth = 0 + let i = bracketStart + 1 + bracketDepth = 1 + + while (i < source.length && bracketDepth > 0) { + const ch = source[i] + + if (ch === '[') bracketDepth++ + else if (ch === ']') { + bracketDepth-- + if (bracketDepth === 0) break + } else if (ch === '{') { + braceDepth++ + if (braceDepth === 1) { + const ahead = source.slice(i, i + 200) + const idMatch = ahead.match(/{\s*(?:\/\/[^\n]*\n\s*)*id:\s*['"]([^'"]+)['"]/) + if (idMatch) { + ids.push(idMatch[1]) + } + } + } else if (ch === '}') { + braceDepth-- + } + + i++ + } + + return ids +} + +function getCurrentIds(): IdMap { + const map: IdMap = {} + for (const block of getAllBlocks()) { + map[block.type] = new Set(block.subBlocks.map((sb) => sb.id)) + } + return map +} + +function getPreviousIds(): IdMap { + const registryPath = 'apps/sim/blocks/registry.ts' + const blocksDir = 'apps/sim/blocks/blocks' + + let hasChanges = false + try { + const diff = execSync( + `git diff --name-only ${baseRef} HEAD -- ${registryPath} ${blocksDir}`, + gitOpts + ).trim() + hasChanges = diff.length > 0 + } catch { + console.log('⚠ Could not diff against base ref — skipping check') + process.exit(0) + } + + if (!hasChanges) { + console.log('✓ No block definition changes detected — nothing to check') + process.exit(0) + } + + const map: IdMap = {} + + try { + const blockFiles = execSync(`git ls-tree -r --name-only ${baseRef} -- ${blocksDir}`, gitOpts) + .trim() + .split('\n') + .filter((f) => f.endsWith('.ts') && !f.endsWith('.test.ts')) + + for (const filePath of blockFiles) { + let content: string + try { + content = execSync(`git show ${baseRef}:${filePath}`, gitOpts) + } catch { + continue + } + + const typeMatch = content.match(/BlockConfig\s*=\s*\{[\s\S]*?type:\s*['"]([^'"]+)['"]/) + if (!typeMatch) continue + const blockType = typeMatch[1] + + const ids = extractSubBlockIds(content) + if (ids.length === 0) continue + + map[blockType] = new Set(ids) + } + } catch (err) { + console.log(`⚠ Could not read previous block files from ${baseRef} — skipping check`, err) + process.exit(0) + } + + return map +} + +const previous = getPreviousIds() +const current = getCurrentIds() +const errors: string[] = [] + +for (const [blockType, prevIds] of Object.entries(previous)) { + const currIds = current[blockType] + if (!currIds) continue + + const migrations = SUBBLOCK_ID_MIGRATIONS[blockType] ?? {} + + for (const oldId of prevIds) { + if (currIds.has(oldId)) continue + + if (oldId in migrations) continue + + errors.push( + `Block "${blockType}": subblock ID "${oldId}" was removed.\n` + + ` → Add a migration in SUBBLOCK_ID_MIGRATIONS (lib/workflows/migrations/subblock-migrations.ts)\n` + + ` mapping "${oldId}" to its replacement ID.` + ) + } +} + +if (errors.length > 0) { + console.error('✗ Subblock ID stability check FAILED\n') + console.error( + 'Removing subblock IDs breaks deployed workflows.\n' + + 'Either revert the rename or add a migration entry.\n' + ) + for (const err of errors) { + console.error(` ${err}\n`) + } + process.exit(1) +} else { + console.log('✓ Subblock ID stability check passed') + process.exit(0) +}