Skip to content

Commit 5f1492b

Browse files
committed
fix(copilot): resolve same-id subblock variants before validating
A block may declare one field id several times, each variant conditioned on another field — the embeddings block declares model, dimensions, and taskType once per provider, and the image and video generators do the same. Validation keyed a map by id alone, so whichever variant was declared last silently became the validator for every write to that field. Programmatic edits to an embeddings block were therefore checked against Mistral's option lists whatever the saved provider: `text-embedding-3-small` was rejected as not one of mistral-embed/codestral-embed, and dimensions valid only elsewhere (3072, 768) could not be set at all. Values that happened to overlap the last variant passed, so automation saw partial success rather than a clean failure. Keep every candidate per id and pick the one whose condition holds, evaluating against the mutation's inputs merged over the block's saved values so a partial write still resolves. When no condition matches, fall back to the union of all variants' options rather than guessing. Conditions still never gate whether a field may be written — that was a deliberate choice and a hidden field stays writable. They only select which definition describes the field, and an unresolved condition widens the accepted set instead of narrowing it.
1 parent 9f976b6 commit 5f1492b

3 files changed

Lines changed: 234 additions & 16 deletions

File tree

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

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { createLogger } from '@sim/logger'
22
import { isValidKey } from '@/lib/workflows/sanitization/key-validation'
3+
import { buildSubBlockValues } from '@/lib/workflows/subblocks/visibility'
34
import { TriggerUtils } from '@/lib/workflows/triggers/triggers'
45
import { getBlock } from '@/blocks/registry'
56
import { normalizeName, RESERVED_BLOCK_NAMES } from '@/executor/constants'
@@ -201,7 +202,8 @@ function mergeNestedNodesForParent(
201202
const childValidation = validateInputsForBlock(
202203
existingBlock.type,
203204
childBlock.inputs,
204-
existingId
205+
existingId,
206+
buildSubBlockValues(existingBlock.subBlocks)
205207
)
206208
validationErrors.push(...childValidation.errors)
207209

@@ -426,7 +428,12 @@ export function handleEditOperation(op: EditWorkflowOperation, ctx: OperationCon
426428
if (!block.subBlocks) block.subBlocks = {}
427429

428430
// Validate inputs against block configuration
429-
const validationResult = validateInputsForBlock(block.type, params.inputs, block_id)
431+
const validationResult = validateInputsForBlock(
432+
block.type,
433+
params.inputs,
434+
block_id,
435+
buildSubBlockValues(block.subBlocks ?? {})
436+
)
430437
validationErrors.push(...validationResult.errors)
431438

432439
Object.entries(validationResult.validInputs).forEach(([inputKey, value]) => {
@@ -898,7 +905,12 @@ export function handleInsertIntoSubflowOperation(
898905
// Update inputs if provided (with validation)
899906
if (params.inputs) {
900907
// Validate inputs against block configuration
901-
const validationResult = validateInputsForBlock(existingBlock.type, params.inputs, block_id)
908+
const validationResult = validateInputsForBlock(
909+
existingBlock.type,
910+
params.inputs,
911+
block_id,
912+
buildSubBlockValues(existingBlock.subBlocks ?? {})
913+
)
902914
validationErrors.push(...validationResult.errors)
903915

904916
Object.entries(validationResult.validInputs).forEach(([key, value]) => {

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

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,67 @@ const toolsByIdMock: Record<string, unknown> = {
202202
},
203203
}
204204

205+
/**
206+
* Declares one field id several times, each variant conditioned on another
207+
* field — the shape used by the embeddings, image-generator, and
208+
* video-generator blocks. `size` deliberately overlaps on 50 so a test can
209+
* distinguish "resolved the right variant" from "happened to overlap".
210+
*/
211+
const multiVariantBlockConfig = {
212+
type: 'multi_variant_block',
213+
name: 'Multi Variant Block',
214+
outputs: {},
215+
subBlocks: [
216+
{
217+
id: 'provider',
218+
type: 'dropdown',
219+
options: [
220+
{ label: 'Alpha', id: 'alpha' },
221+
{ label: 'Beta', id: 'beta' },
222+
],
223+
},
224+
{
225+
id: 'model',
226+
type: 'dropdown',
227+
options: [
228+
{ label: 'a1', id: 'a1' },
229+
{ label: 'a2', id: 'a2' },
230+
],
231+
condition: { field: 'provider', value: 'alpha' },
232+
},
233+
{
234+
id: 'model',
235+
type: 'dropdown',
236+
options: [
237+
{ label: 'b1', id: 'b1' },
238+
{ label: 'b2', id: 'b2' },
239+
],
240+
condition: { field: 'provider', value: 'beta' },
241+
},
242+
{
243+
id: 'size',
244+
type: 'dropdown',
245+
options: [
246+
{ label: '100', id: '100' },
247+
{ label: '50', id: '50' },
248+
],
249+
condition: { field: 'provider', value: 'alpha', and: { field: 'model', value: 'a1' } },
250+
},
251+
{
252+
id: 'size',
253+
type: 'dropdown',
254+
options: [
255+
{ label: '50', id: '50' },
256+
{ label: '25', id: '25' },
257+
],
258+
condition: { field: 'provider', value: 'beta' },
259+
},
260+
],
261+
tools: { access: ['multi_variant_tool'], config: { tool: () => 'multi_variant_tool' } },
262+
}
263+
205264
const blockConfigsByType: Record<string, unknown> = {
265+
multi_variant_block: multiVariantBlockConfig,
206266
condition: conditionBlockConfig,
207267
slack: oauthBlockConfig,
208268
router_v2: routerBlockConfig,
@@ -279,6 +339,79 @@ describe('validateInputsForBlock', () => {
279339
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: [] })
280340
})
281341

342+
/**
343+
* A block may declare one field id several times, each variant conditioned on
344+
* another field. Keying a map by id alone kept whichever variant was declared
345+
* last, so a value valid for the selected provider was checked against an
346+
* unrelated provider's options and rejected.
347+
*/
348+
describe('same-id conditional field variants', () => {
349+
it('validates against the variant selected in the same mutation', () => {
350+
// 'a1' belongs to the first variant; the last-declared one offers b1/b2.
351+
const result = validateInputsForBlock(
352+
'multi_variant_block',
353+
{ provider: 'alpha', model: 'a1' },
354+
'mv-1'
355+
)
356+
357+
expect(result.errors).toHaveLength(0)
358+
expect(result.validInputs.model).toBe('a1')
359+
})
360+
361+
it('accepts a value absent from the last-declared variant', () => {
362+
// 100 exists only on the alpha `size` variant.
363+
const result = validateInputsForBlock(
364+
'multi_variant_block',
365+
{ provider: 'alpha', model: 'a1', size: '100' },
366+
'mv-2'
367+
)
368+
369+
expect(result.errors).toHaveLength(0)
370+
expect(result.validInputs.size).toBe('100')
371+
})
372+
373+
it('resolves against saved values when the mutation is partial', () => {
374+
const result = validateInputsForBlock('multi_variant_block', { size: '100' }, 'mv-3', {
375+
provider: 'alpha',
376+
model: 'a1',
377+
})
378+
379+
expect(result.errors).toHaveLength(0)
380+
expect(result.validInputs.size).toBe('100')
381+
})
382+
383+
it('rejects a value belonging to a different variant', () => {
384+
// 25 is beta-only, so it must not pass while alpha is selected.
385+
const result = validateInputsForBlock(
386+
'multi_variant_block',
387+
{ provider: 'alpha', model: 'a1', size: '25' },
388+
'mv-4'
389+
)
390+
391+
expect(result.errors).toHaveLength(1)
392+
expect(result.errors[0].field).toBe('size')
393+
})
394+
395+
it('rejects a value no variant offers', () => {
396+
const result = validateInputsForBlock(
397+
'multi_variant_block',
398+
{ provider: 'alpha', model: 'nope' },
399+
'mv-5'
400+
)
401+
402+
expect(result.errors).toHaveLength(1)
403+
expect(result.errors[0].field).toBe('model')
404+
})
405+
406+
it('falls back to the union when no variant condition matches', () => {
407+
// Without a provider nothing resolves, so widen rather than guess.
408+
const result = validateInputsForBlock('multi_variant_block', { model: 'b1' }, 'mv-6')
409+
410+
expect(result.errors).toHaveLength(0)
411+
expect(result.validInputs.model).toBe('b1')
412+
})
413+
})
414+
282415
it('accepts condition-input arrays with arbitrary item ids', () => {
283416
const result = validateInputsForBlock(
284417
'condition',

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

Lines changed: 86 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { getSkillById } from '@/lib/workflows/skills/operations'
1111
import {
1212
buildCanonicalIndex,
1313
buildSubBlockValues,
14+
evaluateSubBlockCondition,
1415
isCanonicalPair,
1516
resolveCanonicalMode,
1617
} from '@/lib/workflows/subblocks/visibility'
@@ -57,7 +58,13 @@ export function findBlockWithDuplicateNormalizedName(
5758
export function validateInputsForBlock(
5859
blockType: string,
5960
inputs: Record<string, any>,
60-
blockId: string
61+
blockId: string,
62+
/**
63+
* The block's already-saved subblock values, when editing an existing block.
64+
* Lets a partial mutation resolve conditional field variants against state it
65+
* is not itself rewriting.
66+
*/
67+
existingValues?: Record<string, unknown>
6168
): ValidationResult {
6269
const errors: ValidationError[] = []
6370

@@ -84,20 +91,32 @@ export function validateInputsForBlock(
8491
}
8592

8693
const validatedInputs: Record<string, any> = {}
87-
const subBlockMap = new Map<string, SubBlockConfig>()
8894

89-
// Build map of subBlock id -> config
95+
/**
96+
* A field id can be declared more than once, each variant conditioned on
97+
* another field, so every candidate is kept and the active one is resolved
98+
* per field below.
99+
*/
100+
const subBlockCandidates = new Map<string, SubBlockConfig[]>()
90101
for (const subBlock of blockConfig.subBlocks) {
91-
subBlockMap.set(subBlock.id, subBlock)
102+
const existing = subBlockCandidates.get(subBlock.id)
103+
if (existing) existing.push(subBlock)
104+
else subBlockCandidates.set(subBlock.id, [subBlock])
92105
}
93106

107+
/** Incoming values win over saved ones so a single mutation resolves itself. */
108+
const effectiveValues: Record<string, unknown> = { ...existingValues, ...inputs }
109+
94110
for (const [key, value] of Object.entries(inputs)) {
95111
// Skip runtime subblock IDs
96112
if (TRIGGER_RUNTIME_SUBBLOCK_IDS.includes(key)) {
97113
continue
98114
}
99115

100-
const subBlockConfig = subBlockMap.get(key)
116+
const candidates = subBlockCandidates.get(key)
117+
const subBlockConfig = candidates
118+
? (resolveActiveSubBlock(candidates, effectiveValues) ?? unionSubBlock(candidates))
119+
: undefined
101120

102121
// If subBlock doesn't exist in config, skip it (unless it's a known dynamic field)
103122
if (!subBlockConfig) {
@@ -141,10 +160,10 @@ export function validateInputsForBlock(
141160
continue
142161
}
143162

144-
// Note: We do NOT check subBlockConfig.condition here.
145-
// Conditions are for UI display logic (show/hide fields in the editor).
146-
// For API/Copilot, any valid field in the block schema should be accepted.
147-
// The runtime will use the relevant fields based on the actual operation.
163+
// A field is never rejected for being conditionally hidden — conditions are
164+
// UI display logic, and any field in the block schema stays writable. They
165+
// are consulted only to pick which same-id variant defines this field, and
166+
// an unresolved condition widens the accepted set rather than narrowing it.
148167

149168
// Validate value based on subBlock type
150169
const validationResult = validateValueForSubBlockType(
@@ -275,6 +294,63 @@ function validateAgentSkillEntry(item: any, index: number): string | null {
275294
return null
276295
}
277296

297+
/** Reads a subblock's options list, which may be declared as a thunk. */
298+
function readOptions(subBlockConfig: SubBlockConfig) {
299+
return typeof subBlockConfig.options === 'function'
300+
? subBlockConfig.options()
301+
: subBlockConfig.options
302+
}
303+
304+
/**
305+
* Picks which of several same-id subblock definitions applies.
306+
*
307+
* A block may declare one field id several times, each variant conditioned on
308+
* another field — the embeddings block declares `model`, `dimensions`, and
309+
* `taskType` once per provider, and the image/video generators do the same.
310+
* Keying a map by id alone silently keeps whichever variant happens to be
311+
* declared last, so a value valid for the selected provider is checked against
312+
* an unrelated provider's option list.
313+
*
314+
* Conditions are evaluated against the mutation's inputs merged over the
315+
* block's saved values, so a write that sets only `model` still resolves
316+
* against an already-persisted `provider`.
317+
*
318+
* Returns null when no variant's condition matches, which leaves the caller to
319+
* fall back to accepting anything valid for any variant rather than guessing.
320+
*/
321+
function resolveActiveSubBlock(
322+
candidates: SubBlockConfig[],
323+
effectiveValues: Record<string, unknown>
324+
): SubBlockConfig | null {
325+
if (candidates.length === 1) return candidates[0]
326+
const active = candidates.filter((candidate) =>
327+
evaluateSubBlockCondition(candidate.condition, effectiveValues)
328+
)
329+
return active.length > 0 ? active[0] : null
330+
}
331+
332+
/**
333+
* Collapses same-id variants into one definition whose options are the union of
334+
* every variant's. Used only when the active variant cannot be resolved, so an
335+
* unresolvable write is never rejected for a value that is legal somewhere.
336+
*/
337+
function unionSubBlock(candidates: SubBlockConfig[]): SubBlockConfig {
338+
const seen = new Set<string>()
339+
const merged: Array<{ id: string; label?: string }> = []
340+
for (const candidate of candidates) {
341+
const options = readOptions(candidate)
342+
if (!Array.isArray(options)) continue
343+
for (const option of options) {
344+
if (seen.has(option.id)) continue
345+
seen.add(option.id)
346+
merged.push(option)
347+
}
348+
}
349+
return merged.length > 0
350+
? ({ ...candidates[0], options: merged } as SubBlockConfig)
351+
: candidates[0]
352+
}
353+
278354
/**
279355
* Validates a value against its expected subBlock type
280356
* Returns validation result with the value or an error
@@ -296,10 +372,7 @@ export function validateValueForSubBlockType(
296372
switch (type) {
297373
case 'dropdown': {
298374
// Validate against allowed options
299-
const options =
300-
typeof subBlockConfig.options === 'function'
301-
? subBlockConfig.options()
302-
: subBlockConfig.options
375+
const options = readOptions(subBlockConfig)
303376
if (options && Array.isArray(options)) {
304377
const validIds = options.map((opt) => opt.id)
305378
if (!validIds.includes(value)) {

0 commit comments

Comments
 (0)