improvement(subblocks): trust block registry over stored subblock type (#5832)

apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts

@@ -451,7 +451,12 @@ export async function copyWorkflowStateIntoTarget(
       clearUnmapped: true,
       canonicalModes: activeCanonicalModes,
     })
-    subBlocks = remapConditionIdsInSubBlocks(subBlocks, oldBlockId, newBlockId) as SubBlockRecord
+    subBlocks = remapConditionIdsInSubBlocks(
+      subBlocks,
+      block.type,
+      oldBlockId,
+      newBlockId
+    ) as SubBlockRecord
 
     // Apply the stored dependent values for this block (the modal's mapping). The reference
     // transform already cleared the source's dependent values when their parent was remapped,

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

@@ -3,6 +3,7 @@
  */
 import { describe, expect, it, vi } from 'vitest'
 import {
+  applyTriggerConfigToBlockSubblocks,
   createBlockFromParams,
   normalizeSubblockValue,
 } from '@/lib/copilot/tools/server/workflow/edit-workflow/builders'
@@ -33,14 +34,27 @@ const knowledgeBlockConfig = {
   ],
 }
 
+const slackBlockConfig = {
+  type: 'slack',
+  name: 'Slack',
+  outputs: {},
+  subBlocks: [{ id: 'channel', type: 'channel-selector' }],
+}
+
 const blocksByType: Record<string, unknown> = {
   agent: agentBlockConfig,
   condition: conditionBlockConfig,
   knowledge: knowledgeBlockConfig,
+  slack: slackBlockConfig,
 }
 
 vi.mock('@/blocks/registry', () => ({
-  getAllBlocks: () => [agentBlockConfig, conditionBlockConfig, knowledgeBlockConfig],
+  getAllBlocks: () => [
+    agentBlockConfig,
+    conditionBlockConfig,
+    knowledgeBlockConfig,
+    slackBlockConfig,
+  ],
   getBlock: (type: string) => blocksByType[type],
 }))
 
@@ -168,3 +182,40 @@ describe('normalizeSubblockValue', () => {
     expect(JSON.parse(result as string)[0].id).not.toBe('filter-1')
   })
 })
+
+describe('applyTriggerConfigToBlockSubblocks', () => {
+  it('uses the registry type for declared keys and short-input only for undeclared keys', () => {
+    const block = { id: 'b1', type: 'slack', subBlocks: {} as Record<string, unknown> }
+
+    applyTriggerConfigToBlockSubblocks(block, { channel: 'C123', customField: 'x' })
+
+    expect(block.subBlocks.channel).toEqual({
+      id: 'channel',
+      type: 'channel-selector',
+      value: 'C123',
+    })
+    expect(block.subBlocks.customField).toEqual({
+      id: 'customField',
+      type: 'short-input',
+      value: 'x',
+    })
+  })
+
+  it('keeps the existing entry metadata when the key already exists', () => {
+    const block = {
+      id: 'b1',
+      type: 'slack',
+      subBlocks: {
+        channel: { id: 'channel', type: 'channel-selector', value: 'C-old' },
+      } as Record<string, { id: string; type: string; value: unknown }>,
+    }
+
+    applyTriggerConfigToBlockSubblocks(block, { channel: 'C-new' })
+
+    expect(block.subBlocks.channel).toEqual({
+      id: 'channel',
+      type: 'channel-selector',
+      value: 'C-new',
+    })
+  })
+})

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

@@ -9,7 +9,7 @@ import {
   isCanonicalPair,
 } from '@/lib/workflows/subblocks/visibility'
 import { hasTriggerCapability } from '@/lib/workflows/triggers/trigger-utils'
-import { getAllBlocks } from '@/blocks/registry'
+import { getAllBlocks, getBlock } from '@/blocks/registry'
 import type { BlockConfig } from '@/blocks/types'
 import { TRIGGER_RUNTIME_SUBBLOCK_IDS } from '@/triggers/constants'
 import type { EditWorkflowOperation, SkippedItem, ValidationError } from './types'
@@ -624,9 +624,14 @@ export function applyTriggerConfigToBlockSubblocks(block: any, triggerConfig: Re
         value: configValue,
       }
     } else {
+      // The registry type is authoritative for declared keys; `short-input` is
+      // only the keep-alive default for dynamic trigger-config keys the block
+      // config does not declare (an `unknown` type would be dropped on the next
+      // sanitize pass, losing the value).
+      const subBlockDef = getBlock(block.type)?.subBlocks.find((sb) => sb.id === configKey)
       block.subBlocks[configKey] = {
         id: configKey,
-        type: 'short-input',
+        type: subBlockDef?.type || 'short-input',
         value: configValue,
       }
     }

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

@@ -326,6 +326,7 @@ export async function duplicateWorkflow(
         if (updatedSubBlocks && typeof updatedSubBlocks === 'object') {
           updatedSubBlocks = remapConditionIdsInSubBlocks(
             updatedSubBlocks as Record<string, any>,
+            block.type,
             block.id,
             newBlockId
           )

apps/sim/lib/workflows/persistence/remap-internal-ids.test.ts

@@ -2,8 +2,10 @@
  * @vitest-environment node
  */
 import { describe, expect, it } from 'vitest'
+import { remapConditionEdgeHandle } from '@/lib/workflows/condition-ids'
 import {
   coerceObjectArray,
+  remapConditionIdsInSubBlocks,
   remapWorkflowReferencesInSubBlocks,
   type SubBlockRecord,
 } from '@/lib/workflows/persistence/remap-internal-ids'
@@ -378,6 +380,77 @@ describe('remapWorkflowReferencesInSubBlocks', () => {
   })
 })
 
+describe('remapConditionIdsInSubBlocks', () => {
+  const OLD_ID = 'old-block'
+  const NEW_ID = 'new-block'
+  const conditionsValue = JSON.stringify([
+    { id: `${OLD_ID}-if`, title: 'if', value: '<a.b> > 1' },
+    { id: `${OLD_ID}-else`, title: 'else', value: '' },
+  ])
+
+  it('remaps condition row ids on a condition block', () => {
+    const subBlocks: SubBlockRecord = {
+      conditions: { id: 'conditions', type: 'condition-input', value: conditionsValue },
+    }
+    const result = remapConditionIdsInSubBlocks(subBlocks, 'condition', OLD_ID, NEW_ID)
+    const rows = JSON.parse(result.conditions.value as string)
+    expect(rows.map((row: { id: string }) => row.id)).toEqual([`${NEW_ID}-if`, `${NEW_ID}-else`])
+  })
+
+  /**
+   * Regression: a fallback writer stamped the conditions subblock `short-input`.
+   * The remap must key on block type + subblock key, not the drifted stored type,
+   * so the row ids and the edge handle move together (previously the ids stayed
+   * stale while the handle remapped, orphaning every edge out of the block).
+   */
+  it('remaps condition row ids even when the stored subblock type drifted', () => {
+    const subBlocks: SubBlockRecord = {
+      conditions: { id: 'conditions', type: 'short-input', value: conditionsValue },
+    }
+    const result = remapConditionIdsInSubBlocks(subBlocks, 'condition', OLD_ID, NEW_ID)
+    const rows = JSON.parse(result.conditions.value as string)
+    const handle = remapConditionEdgeHandle(`condition-${OLD_ID}-else`, OLD_ID, NEW_ID)
+    expect(handle).toBe(`condition-${NEW_ID}-else`)
+    expect(rows.map((row: { id: string }) => row.id)).toContain(`${NEW_ID}-else`)
+  })
+
+  it('remaps route ids on a router_v2 block', () => {
+    const subBlocks: SubBlockRecord = {
+      routes: {
+        id: 'routes',
+        type: 'router-input',
+        value: JSON.stringify([{ id: `${OLD_ID}-route1`, title: 'Route 1', value: 'desc' }]),
+      },
+    }
+    const result = remapConditionIdsInSubBlocks(subBlocks, 'router_v2', OLD_ID, NEW_ID)
+    const rows = JSON.parse(result.routes.value as string)
+    expect(rows[0].id).toBe(`${NEW_ID}-route1`)
+  })
+
+  it('leaves rows with a foreign block-id prefix untouched (matches the edge-handle remap)', () => {
+    const subBlocks: SubBlockRecord = {
+      conditions: {
+        id: 'conditions',
+        type: 'condition-input',
+        value: JSON.stringify([{ id: 'foreign-block-if', title: 'if', value: '' }]),
+      },
+    }
+    const result = remapConditionIdsInSubBlocks(subBlocks, 'condition', OLD_ID, NEW_ID)
+    expect(result.conditions).toBe(subBlocks.conditions)
+    expect(remapConditionEdgeHandle('condition-foreign-block-if', OLD_ID, NEW_ID)).toBe(
+      'condition-foreign-block-if'
+    )
+  })
+
+  it('does not touch subblocks on non-dynamic-handle block types', () => {
+    const subBlocks: SubBlockRecord = {
+      conditions: { id: 'conditions', type: 'condition-input', value: conditionsValue },
+    }
+    const result = remapConditionIdsInSubBlocks(subBlocks, 'function', OLD_ID, NEW_ID)
+    expect(result.conditions).toBe(subBlocks.conditions)
+  })
+})
+
 describe('coerceObjectArray', () => {
   it('returns arrays directly', () => {
     expect(coerceObjectArray([{ a: 1 }])).toEqual({ array: [{ a: 1 }], wasString: false })

apps/sim/lib/workflows/persistence/remap-internal-ids.ts

@@ -1,5 +1,6 @@
 import { createLogger } from '@sim/logger'
 import { remapConditionBlockIds } from '@/lib/workflows/condition-ids'
+import { isDynamicHandleSubblock } from '@/lib/workflows/dynamic-handle-topology'
 import {
   type CanonicalModeOverrides,
   resolveCanonicalMode,
@@ -321,9 +322,16 @@ function remapWorkflowInputTools(
 /**
  * Remap condition/router block IDs within subBlocks when a block is copied with
  * a new ID. Returns a new object without mutating the input.
+ *
+ * Gated on the BLOCK type + canonical subblock key (`conditions`/`routes`), not
+ * the stored subblock `type`: edge handles are remapped by string prefix with no
+ * type gate, so keying this side on mutable stored metadata lets a drifted type
+ * (e.g. a `conditions` entry stamped `short-input` by a fallback writer) skip the
+ * id remap while the handles still move — orphaning every edge out of the block.
  */
 export function remapConditionIdsInSubBlocks(
   subBlocks: SubBlockRecord,
+  blockType: string | undefined,
   oldBlockId: string,
   newBlockId: string
 ): SubBlockRecord {
@@ -332,7 +340,7 @@ export function remapConditionIdsInSubBlocks(
     if (
       subBlock &&
       typeof subBlock === 'object' &&
-      (subBlock.type === 'condition-input' || subBlock.type === 'router-input') &&
+      isDynamicHandleSubblock(blockType, key) &&
       typeof subBlock.value === 'string'
     ) {
       try {

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

@@ -23,6 +23,7 @@ import { LRUCache } from 'lru-cache'
 import type { Edge } from 'reactflow'
 import { releaseWebhookPathClaims } from '@/lib/webhooks/path-claims'
 import { remapConditionBlockIds, remapConditionEdgeHandle } from '@/lib/workflows/condition-ids'
+import { isDynamicHandleSubblock } from '@/lib/workflows/dynamic-handle-topology'
 import {
   backfillCanonicalModes,
   migrateSubblockIds,
@@ -717,7 +718,7 @@ export function regenerateWorkflowStateIds(state: RegenerateStateInput): Regener
         }
 
         if (
-          (updatedSubBlock.type === 'condition-input' || updatedSubBlock.type === 'router-input') &&
+          isDynamicHandleSubblock(block.type, subId) &&
           typeof updatedSubBlock.value === 'string'
         ) {
           try {

apps/sim/lib/workflows/sanitization/subblocks.test.ts

@@ -133,5 +133,59 @@ describe('sanitizeMalformedSubBlocks', () => {
       expect(changed).toBe(true)
       expect(subBlocks.code).toEqual({ id: 'code', type: 'code', value: 'return 1' })
     })
+
+    it('repairs a stored type that contradicts the configured type', () => {
+      const { subBlocks, changed } = sanitizeMalformedSubBlocks({
+        id: 'block-1',
+        type: 'function',
+        subBlocks: {
+          code: { id: 'code', type: 'short-input', value: 'return 1' },
+        },
+      })
+
+      expect(changed).toBe(true)
+      expect(subBlocks.code).toEqual({ id: 'code', type: 'code', value: 'return 1' })
+    })
+
+    /**
+     * Regression: a fallback writer stamped a condition block's `conditions`
+     * subblock `short-input`. Copy-time id remapping used to gate on that stored
+     * type, skipping the conditions array while edge handles still remapped —
+     * orphaning every edge out of the block on fork/duplicate/import.
+     */
+    it('repairs a condition block conditions subblock stamped short-input by a fallback writer', () => {
+      const conditionsValue = JSON.stringify([
+        { id: 'block-1-if', title: 'if', value: '<a.b>' },
+        { id: 'block-1-else', title: 'else', value: '' },
+      ])
+      const { subBlocks, changed } = sanitizeMalformedSubBlocks({
+        id: 'block-1',
+        type: 'condition',
+        subBlocks: {
+          conditions: { id: 'conditions', type: 'short-input', value: conditionsValue },
+        },
+      })
+
+      expect(changed).toBe(true)
+      expect(subBlocks.conditions).toEqual({
+        id: 'conditions',
+        type: 'condition-input',
+        value: conditionsValue,
+      })
+    })
+
+    it('preserves a valid stored type for keys the config does not declare (runtime values)', () => {
+      const input = {
+        webhookId: { id: 'webhookId', type: 'short-input', value: 'wh-1' },
+      }
+      const { subBlocks, changed } = sanitizeMalformedSubBlocks({
+        id: 'block-1',
+        type: 'function',
+        subBlocks: input,
+      })
+
+      expect(changed).toBe(false)
+      expect(subBlocks).toBe(input)
+    })
   })
 })

apps/sim/lib/workflows/sanitization/subblocks.ts

@@ -21,6 +21,17 @@ interface SanitizableBlock {
  * Repairs legacy subBlock metadata when the map key identifies a real field,
  * and drops entries that cannot be associated with a stable subBlock.
  *
+ * For keys the block registry declares, the CONFIGURED type is authoritative
+ * and overwrites a contradicting stored type. Stored types drift in two ways:
+ * fallback writers stamp a plausible-but-wrong default (`short-input`) when
+ * they synthesize a missing structure entry, and block configs evolve their
+ * declared types over time while persisted rows keep the old one. A wrong
+ * stored type silently disables type-gated logic downstream — most damaging
+ * for `condition-input`/`router-input`, where copy-time id remapping skips
+ * the conditions array while edge handles still remap, orphaning the edges.
+ * Draft loads persist this repair via `persistMigratedBlocks`, so stored
+ * state converges back to the registry.
+ *
  * Custom blocks are schema-agnostic here: their server-side config never
  * declares the per-field input sub-blocks (the execution overlay passes bare
  * wiring rows, and this may run with no overlay at all), so "not in config"
@@ -95,10 +106,11 @@ export function sanitizeMalformedSubBlocks(
       continue
     }
 
-    const type =
+    const storedType =
       typeof subBlock.type === 'string' && subBlock.type.length > 0 && subBlock.type !== 'unknown'
         ? subBlock.type
-        : typeFromConfig || DEFAULT_SUBBLOCK_TYPE
+        : null
+    const type = typeFromConfig ?? storedType ?? DEFAULT_SUBBLOCK_TYPE
     const hasValue = Object.hasOwn(subBlock, 'value')
     const value =
       options.convertEmptyStringToNull && subBlock.value === ''
@@ -111,7 +123,12 @@ export function sanitizeMalformedSubBlocks(
     const normalizedValue = hasValue && value !== subBlock.value
 
     if (repairedMetadata) {
-      logger.warn('Repairing malformed subBlock metadata', { blockId: block.id, subBlockId })
+      logger.warn('Repairing malformed subBlock metadata', {
+        blockId: block.id,
+        subBlockId,
+        storedType: subBlock.type,
+        repairedType: type,
+      })
       changed = true
     } else if (normalizedValue) {
       logger.warn('Normalizing malformed subBlock value', { blockId: block.id, subBlockId })

apps/sim/stores/workflows/utils.test.ts

@@ -5,6 +5,7 @@ import {
   createLoopBlock,
   createStarterBlock,
 } from '@sim/testing'
+import type { Edge } from 'reactflow'
 import { describe, expect, it } from 'vitest'
 import { normalizeName } from '@/executor/constants'
 import { getUniqueBlockName, regenerateBlockIds } from './utils'
@@ -438,6 +439,67 @@ describe('regenerateBlockIds', () => {
     expect(duplicatedBlock.data?.parentId).toBe(loopId)
   })
 
+  /**
+   * Regression: a fallback writer stamped a condition block's `conditions`
+   * subblock `short-input`. The id remap must key on block type + subblock key
+   * (not the drifted stored type) so the condition row ids and the outgoing
+   * edge's sourceHandle move together — previously the handle remapped while
+   * the row ids stayed stale, orphaning the edge.
+   */
+  it('keeps condition row ids and edge handles consistent when the stored subblock type drifted', () => {
+    const conditionId = 'condition-1'
+    const targetId = 'target-1'
+
+    const blocksToCopy = {
+      [conditionId]: createBlock({
+        id: conditionId,
+        type: 'condition',
+        name: 'botFilter',
+        subBlocks: {
+          conditions: {
+            id: 'conditions',
+            type: 'short-input',
+            value: JSON.stringify([
+              { id: `${conditionId}-if`, title: 'if', value: '<a.b>' },
+              { id: `${conditionId}-else`, title: 'else', value: '' },
+            ]),
+          },
+        },
+      }),
+      [targetId]: createAgentBlock({ id: targetId, name: 'Agent 1' }),
+    }
+
+    const edges = [
+      {
+        id: 'edge-1',
+        source: conditionId,
+        sourceHandle: `condition-${conditionId}-else`,
+        target: targetId,
+        targetHandle: 'target',
+      },
+    ] as Edge[]
+
+    const result = regenerateBlockIds(
+      blocksToCopy,
+      edges,
+      {},
+      {},
+      {},
+      positionOffset,
+      {},
+      getUniqueBlockName
+    )
+
+    const newCondition = Object.values(result.blocks).find((b) => b.type === 'condition')!
+    const newEdge = result.edges[0]
+    const rowIds = JSON.parse(newCondition.subBlocks.conditions.value as string).map(
+      (row: { id: string }) => row.id
+    )
+
+    expect(rowIds).toEqual([`${newCondition.id}-if`, `${newCondition.id}-else`])
+    expect(newEdge.sourceHandle).toBe(`condition-${newCondition.id}-else`)
+  })
+
   it('should unlock pasted block when source is locked', () => {
     const blockId = 'block-1'
 

apps/sim/stores/workflows/utils.ts

@@ -5,6 +5,7 @@ import type { Edge } from 'reactflow'
 import { DEFAULT_DUPLICATE_OFFSET } from '@/lib/workflows/autolayout/constants'
 import { getEffectiveBlockOutputs } from '@/lib/workflows/blocks/block-outputs'
 import { remapConditionBlockIds, remapConditionEdgeHandle } from '@/lib/workflows/condition-ids'
+import { isDynamicHandleSubblock } from '@/lib/workflows/dynamic-handle-topology'
 import { createDefaultInputFormatField } from '@/lib/workflows/input-format'
 import { buildDefaultCanonicalModes } from '@/lib/workflows/subblocks/visibility'
 import { hasTriggerCapability } from '@/lib/workflows/triggers/trigger-utils'
@@ -276,7 +277,7 @@ export function regenerateWorkflowIds(
     const oldNormalizedName = normalizeName(block.name)
     nameMap.set(oldNormalizedName, oldNormalizedName)
     const newBlock = { ...block, id: newId, subBlocks: structuredClone(block.subBlocks) }
-    remapConditionIds(newBlock.subBlocks, {}, oldId, newId)
+    remapConditionIds(newBlock.subBlocks, {}, block.type, oldId, newId)
     newBlocks[newId] = newBlock
   })
 
@@ -350,6 +351,11 @@ export function regenerateWorkflowIds(
  * Remaps condition/router block IDs within subBlock values when a block is duplicated.
  * Mutates both `subBlocks` and `subBlockValues` in place (callers must pass cloned data).
  *
+ * Gated on the BLOCK type + canonical subblock key (`conditions`/`routes`), not the
+ * stored subblock `type`: edge handles remap by string prefix with no type gate, so a
+ * drifted stored type would skip the id remap here while the handles still move,
+ * orphaning every edge out of the block.
+ *
  * The `subBlockValues[id] ?? subBlock.value` fallback is safe here despite the
  * structure copy being generally stale: condition/router subblocks are
  * dynamic-handle types, which dual-write the structure on every edit
@@ -358,11 +364,12 @@ export function regenerateWorkflowIds(
 export function remapConditionIds(
   subBlocks: Record<string, SubBlockState>,
   subBlockValues: Record<string, unknown>,
+  blockType: string | undefined,
   oldBlockId: string,
   newBlockId: string
 ): void {
   for (const [subBlockId, subBlock] of Object.entries(subBlocks)) {
-    if (subBlock.type !== 'condition-input' && subBlock.type !== 'router-input') continue
+    if (!isDynamicHandleSubblock(blockType, subBlockId)) continue
 
     const value = subBlockValues[subBlockId] ?? subBlock.value
     if (typeof value !== 'string') continue
@@ -466,7 +473,7 @@ export function regenerateBlockIds(
     }
 
     // Remap condition/router IDs in the duplicated block
-    remapConditionIds(newBlock.subBlocks, newSubBlockValues[newId] || {}, oldId, newId)
+    remapConditionIds(newBlock.subBlocks, newSubBlockValues[newId] || {}, block.type, oldId, newId)
   })
 
   // Second pass: update parentId references for nested blocks

apps/sim/stores/workflows/workflow/store.ts

@@ -595,6 +595,7 @@ export const useWorkflowStore = create<WorkflowStore>()(
         remapConditionIds(
           newSubBlocks as Record<string, SubBlockState>,
           clonedSubBlockValues,
+          block.type,
           id,
           newId
         )

apps/sim/tools/github/create_pr_review.test.ts

@@ -130,4 +130,22 @@ describe('createPRReviewV2Tool response', () => {
       createPRReviewV2Tool.transformResponse!(Response.json(reviewPayload({ html_url: '' })))
     ).rejects.toThrow('GitHub review response.html_url must be a non-empty string')
   })
+
+  it('surfaces the GitHub error message on a non-ok response', async () => {
+    const result = await createPRReviewV2Tool.transformResponse!(
+      Response.json({ message: 'Validation Failed' }, { status: 422 })
+    )
+
+    expect(result.success).toBe(false)
+    expect(result.error).toBe('Validation Failed')
+  })
+
+  it('falls back to a status-based error when the error body has no message', async () => {
+    const result = await createPRReviewV2Tool.transformResponse!(
+      new Response('not json', { status: 500 })
+    )
+
+    expect(result.success).toBe(false)
+    expect(result.error).toBe('Failed to submit PR review (HTTP 500)')
+  })
 })

apps/sim/tools/github/create_pr_review.ts

@@ -258,10 +258,9 @@ export const createPRReviewV2Tool: ToolConfig<CreatePRReviewParams, CreatePRRevi
     if (!response.ok) {
       return {
         success: false,
-        error: await responseErrorMessage(
-          response,
-          `Failed to submit PR review (HTTP ${response.status})`
-        ),
+        error:
+          (await readGitHubErrorMessage(response)) ??
+          `Failed to submit PR review (HTTP ${response.status})`,
         output: {
           id: 0,
           user: null,