Skip to content

Commit 8c0dfee

Browse files
committed
chore(embeddings): scope this branch to the multi-provider block
Two changes made while building the Embeddings block are not part of it and ship separately, so their files are restored to staging here: - copilot edit-workflow validation resolving same-id conditional subblock variants. The embeddings block surfaced it, but it is a platform fix affecting ~20 blocks that declare a field id more than once, and it narrows what programmatic edits accept — that deserves its own review. - the sync-engine test de-flake, which is unrelated test hygiene. Both are preserved in full on feat/embeddings-full-snapshot. Note this restores the reported bug where a programmatic edit to an embeddings block validates model/dimensions against the last-declared provider variant. The block is unaffected in the editor and at runtime.
1 parent baf0067 commit 8c0dfee

4 files changed

Lines changed: 96 additions & 279 deletions

File tree

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

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

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

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

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

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

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

Lines changed: 0 additions & 161 deletions
Original file line numberDiff line numberDiff line change
@@ -202,83 +202,7 @@ 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-
// Catch-all declared first: it matches everything, so it must not shadow
261-
// the conditioned variant below purely by declaration order.
262-
{
263-
id: 'mode',
264-
type: 'dropdown',
265-
options: [{ label: 'default', id: 'default' }],
266-
},
267-
{
268-
id: 'mode',
269-
type: 'dropdown',
270-
options: [
271-
{ label: 'fast', id: 'fast' },
272-
{ label: 'slow', id: 'slow' },
273-
],
274-
condition: { field: 'provider', value: 'beta' },
275-
},
276-
],
277-
tools: { access: ['multi_variant_tool'], config: { tool: () => 'multi_variant_tool' } },
278-
}
279-
280205
const blockConfigsByType: Record<string, unknown> = {
281-
multi_variant_block: multiVariantBlockConfig,
282206
condition: conditionBlockConfig,
283207
slack: oauthBlockConfig,
284208
router_v2: routerBlockConfig,
@@ -355,91 +279,6 @@ describe('validateInputsForBlock', () => {
355279
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: [] })
356280
})
357281

358-
/**
359-
* A block may declare one field id several times, each variant conditioned on
360-
* another field. Keying a map by id alone kept whichever variant was declared
361-
* last, so a value valid for the selected provider was checked against an
362-
* unrelated provider's options and rejected.
363-
*/
364-
describe('same-id conditional field variants', () => {
365-
it('validates against the variant selected in the same mutation', () => {
366-
// 'a1' belongs to the first variant; the last-declared one offers b1/b2.
367-
const result = validateInputsForBlock(
368-
'multi_variant_block',
369-
{ provider: 'alpha', model: 'a1' },
370-
'mv-1'
371-
)
372-
373-
expect(result.errors).toHaveLength(0)
374-
expect(result.validInputs.model).toBe('a1')
375-
})
376-
377-
it('accepts a value absent from the last-declared variant', () => {
378-
// 100 exists only on the alpha `size` variant.
379-
const result = validateInputsForBlock(
380-
'multi_variant_block',
381-
{ provider: 'alpha', model: 'a1', size: '100' },
382-
'mv-2'
383-
)
384-
385-
expect(result.errors).toHaveLength(0)
386-
expect(result.validInputs.size).toBe('100')
387-
})
388-
389-
it('resolves against saved values when the mutation is partial', () => {
390-
const result = validateInputsForBlock('multi_variant_block', { size: '100' }, 'mv-3', {
391-
provider: 'alpha',
392-
model: 'a1',
393-
})
394-
395-
expect(result.errors).toHaveLength(0)
396-
expect(result.validInputs.size).toBe('100')
397-
})
398-
399-
it('rejects a value belonging to a different variant', () => {
400-
// 25 is beta-only, so it must not pass while alpha is selected.
401-
const result = validateInputsForBlock(
402-
'multi_variant_block',
403-
{ provider: 'alpha', model: 'a1', size: '25' },
404-
'mv-4'
405-
)
406-
407-
expect(result.errors).toHaveLength(1)
408-
expect(result.errors[0].field).toBe('size')
409-
})
410-
411-
it('rejects a value no variant offers', () => {
412-
const result = validateInputsForBlock(
413-
'multi_variant_block',
414-
{ provider: 'alpha', model: 'nope' },
415-
'mv-5'
416-
)
417-
418-
expect(result.errors).toHaveLength(1)
419-
expect(result.errors[0].field).toBe('model')
420-
})
421-
422-
it('prefers a conditioned variant over an unconditioned catch-all', () => {
423-
// `mode` declares the catch-all first; it must not shadow the beta variant.
424-
const result = validateInputsForBlock(
425-
'multi_variant_block',
426-
{ provider: 'beta', mode: 'fast' },
427-
'mv-7'
428-
)
429-
430-
expect(result.errors).toHaveLength(0)
431-
expect(result.validInputs.mode).toBe('fast')
432-
})
433-
434-
it('falls back to the union when no variant condition matches', () => {
435-
// Without a provider nothing resolves, so widen rather than guess.
436-
const result = validateInputsForBlock('multi_variant_block', { model: 'b1' }, 'mv-6')
437-
438-
expect(result.errors).toHaveLength(0)
439-
expect(result.validInputs.model).toBe('b1')
440-
})
441-
})
442-
443282
it('accepts condition-input arrays with arbitrary item ids', () => {
444283
const result = validateInputsForBlock(
445284
'condition',

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

Lines changed: 13 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import { getSkillById } from '@/lib/workflows/skills/operations'
1111
import {
1212
buildCanonicalIndex,
1313
buildSubBlockValues,
14-
evaluateSubBlockCondition,
1514
isCanonicalPair,
1615
resolveCanonicalMode,
1716
} from '@/lib/workflows/subblocks/visibility'
@@ -58,13 +57,7 @@ export function findBlockWithDuplicateNormalizedName(
5857
export function validateInputsForBlock(
5958
blockType: string,
6059
inputs: Record<string, any>,
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>
60+
blockId: string
6861
): ValidationResult {
6962
const errors: ValidationError[] = []
7063

@@ -91,32 +84,20 @@ export function validateInputsForBlock(
9184
}
9285

9386
const validatedInputs: Record<string, any> = {}
87+
const subBlockMap = new Map<string, SubBlockConfig>()
9488

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[]>()
89+
// Build map of subBlock id -> config
10190
for (const subBlock of blockConfig.subBlocks) {
102-
const existing = subBlockCandidates.get(subBlock.id)
103-
if (existing) existing.push(subBlock)
104-
else subBlockCandidates.set(subBlock.id, [subBlock])
91+
subBlockMap.set(subBlock.id, subBlock)
10592
}
10693

107-
/** Incoming values win over saved ones so a single mutation resolves itself. */
108-
const effectiveValues: Record<string, unknown> = { ...existingValues, ...inputs }
109-
11094
for (const [key, value] of Object.entries(inputs)) {
11195
// Skip runtime subblock IDs
11296
if (TRIGGER_RUNTIME_SUBBLOCK_IDS.includes(key)) {
11397
continue
11498
}
11599

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

121102
// If subBlock doesn't exist in config, skip it (unless it's a known dynamic field)
122103
if (!subBlockConfig) {
@@ -160,10 +141,10 @@ export function validateInputsForBlock(
160141
continue
161142
}
162143

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.
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.
167148

168149
// Validate value based on subBlock type
169150
const validationResult = validateValueForSubBlockType(
@@ -294,69 +275,6 @@ function validateAgentSkillEntry(item: any, index: number): string | null {
294275
return null
295276
}
296277

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-
if (active.length === 0) return null
330-
/**
331-
* An unconditioned variant matches everything, so it would shadow a genuinely
332-
* selected one purely by being declared earlier. Prefer a variant that
333-
* actually asserted something about the current values.
334-
*/
335-
return active.find((candidate) => candidate.condition) ?? active[0]
336-
}
337-
338-
/**
339-
* Collapses same-id variants into one definition whose options are the union of
340-
* every variant's. Used only when the active variant cannot be resolved, so an
341-
* unresolvable write is never rejected for a value that is legal somewhere.
342-
*/
343-
function unionSubBlock(candidates: SubBlockConfig[]): SubBlockConfig {
344-
const seen = new Set<string>()
345-
const merged: Array<{ id: string; label?: string }> = []
346-
for (const candidate of candidates) {
347-
const options = readOptions(candidate)
348-
if (!Array.isArray(options)) continue
349-
for (const option of options) {
350-
if (seen.has(option.id)) continue
351-
seen.add(option.id)
352-
merged.push(option)
353-
}
354-
}
355-
return merged.length > 0
356-
? ({ ...candidates[0], options: merged } as SubBlockConfig)
357-
: candidates[0]
358-
}
359-
360278
/**
361279
* Validates a value against its expected subBlock type
362280
* Returns validation result with the value or an error
@@ -378,7 +296,10 @@ export function validateValueForSubBlockType(
378296
switch (type) {
379297
case 'dropdown': {
380298
// Validate against allowed options
381-
const options = readOptions(subBlockConfig)
299+
const options =
300+
typeof subBlockConfig.options === 'function'
301+
? subBlockConfig.options()
302+
: subBlockConfig.options
382303
if (options && Array.isArray(options)) {
383304
const validIds = options.map((opt) => opt.id)
384305
if (!validIds.includes(value)) {

0 commit comments

Comments
 (0)