From 363c32058f0e2e91fd422fdafcca3fc605b22534 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Tue, 21 Jul 2026 17:54:20 -0700 Subject: [PATCH] improvement(subblocks): trust block registry over stored subblock type --- .../lib/copy/copy-workflows.ts | 7 +- .../workflow/edit-workflow/builders.test.ts | 53 +++++++++++++- .../server/workflow/edit-workflow/builders.ts | 9 ++- .../lib/workflows/persistence/duplicate.ts | 1 + .../persistence/remap-internal-ids.test.ts | 73 +++++++++++++++++++ .../persistence/remap-internal-ids.ts | 10 ++- apps/sim/lib/workflows/persistence/utils.ts | 3 +- .../workflows/sanitization/subblocks.test.ts | 54 ++++++++++++++ .../lib/workflows/sanitization/subblocks.ts | 23 +++++- apps/sim/stores/workflows/utils.test.ts | 62 ++++++++++++++++ apps/sim/stores/workflows/utils.ts | 13 +++- apps/sim/stores/workflows/workflow/store.ts | 1 + .../sim/tools/github/create_pr_review.test.ts | 18 +++++ apps/sim/tools/github/create_pr_review.ts | 7 +- 14 files changed, 318 insertions(+), 16 deletions(-) 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 2fede080ea6..62ee9de38c2 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts +++ b/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, diff --git a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.test.ts b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.test.ts index 77fa6ae5323..e49d0279423 100644 --- a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.test.ts +++ b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.test.ts @@ -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 = { 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 } + + 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, + } + + applyTriggerConfigToBlockSubblocks(block, { channel: 'C-new' }) + + expect(block.subBlocks.channel).toEqual({ + id: 'channel', + type: 'channel-selector', + value: 'C-new', + }) + }) +}) diff --git a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts index 286dc576b1d..f669a7719d5 100644 --- a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts +++ b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts @@ -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, } } diff --git a/apps/sim/lib/workflows/persistence/duplicate.ts b/apps/sim/lib/workflows/persistence/duplicate.ts index ab075a8be5f..9895a90fb7f 100644 --- a/apps/sim/lib/workflows/persistence/duplicate.ts +++ b/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, + block.type, block.id, newBlockId ) diff --git a/apps/sim/lib/workflows/persistence/remap-internal-ids.test.ts b/apps/sim/lib/workflows/persistence/remap-internal-ids.test.ts index 7cdc22f39b2..b0069c858bd 100644 --- a/apps/sim/lib/workflows/persistence/remap-internal-ids.test.ts +++ b/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: ' > 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 }) diff --git a/apps/sim/lib/workflows/persistence/remap-internal-ids.ts b/apps/sim/lib/workflows/persistence/remap-internal-ids.ts index f9a9634cd99..09217823190 100644 --- a/apps/sim/lib/workflows/persistence/remap-internal-ids.ts +++ b/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 { diff --git a/apps/sim/lib/workflows/persistence/utils.ts b/apps/sim/lib/workflows/persistence/utils.ts index f762108e528..0fff85f8fd7 100644 --- a/apps/sim/lib/workflows/persistence/utils.ts +++ b/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 { diff --git a/apps/sim/lib/workflows/sanitization/subblocks.test.ts b/apps/sim/lib/workflows/sanitization/subblocks.test.ts index a9a714a400a..5dd15da3801 100644 --- a/apps/sim/lib/workflows/sanitization/subblocks.test.ts +++ b/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: '' }, + { 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) + }) }) }) diff --git a/apps/sim/lib/workflows/sanitization/subblocks.ts b/apps/sim/lib/workflows/sanitization/subblocks.ts index 1f0d222ce2a..07a2f11cb9b 100644 --- a/apps/sim/lib/workflows/sanitization/subblocks.ts +++ b/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 }) diff --git a/apps/sim/stores/workflows/utils.test.ts b/apps/sim/stores/workflows/utils.test.ts index 00127c138ed..68966905752 100644 --- a/apps/sim/stores/workflows/utils.test.ts +++ b/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: '' }, + { 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' diff --git a/apps/sim/stores/workflows/utils.ts b/apps/sim/stores/workflows/utils.ts index 356ba9a274d..feaf871af1b 100644 --- a/apps/sim/stores/workflows/utils.ts +++ b/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, subBlockValues: Record, + 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 diff --git a/apps/sim/stores/workflows/workflow/store.ts b/apps/sim/stores/workflows/workflow/store.ts index 02d9a58761e..11008b1658f 100644 --- a/apps/sim/stores/workflows/workflow/store.ts +++ b/apps/sim/stores/workflows/workflow/store.ts @@ -595,6 +595,7 @@ export const useWorkflowStore = create()( remapConditionIds( newSubBlocks as Record, clonedSubBlockValues, + block.type, id, newId ) diff --git a/apps/sim/tools/github/create_pr_review.test.ts b/apps/sim/tools/github/create_pr_review.test.ts index f96b18fc70f..4a77b679d81 100644 --- a/apps/sim/tools/github/create_pr_review.test.ts +++ b/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)') + }) }) diff --git a/apps/sim/tools/github/create_pr_review.ts b/apps/sim/tools/github/create_pr_review.ts index ea99eea649c..b0ace3d7636 100644 --- a/apps/sim/tools/github/create_pr_review.ts +++ b/apps/sim/tools/github/create_pr_review.ts @@ -258,10 +258,9 @@ export const createPRReviewV2Tool: ToolConfig