Skip to content

Commit b90adb0

Browse files
committed
improvement(workflows): share one reference scanner and make tokenizing linear
Move the reference-aware list splitter into @sim/utils next to the tokenizer so both consume a single <workflow.reference> candidate scan instead of a copied loop, and keep ENV_REFERENCE_PATTERN private again. findWorkflowReferenceTokens checked each workflow span against every prior token, which is quadratic on reference-dense values (8s at 160k tokens). Environment tokens are disjoint and ordered and spans arrive in order, so a forward cursor gives identical output in linear time (21ms); a 300k-input differential fuzz against the previous implementation found no mismatches. With classification linear, the 10k-character selector entry cap is no longer needed, so oversized literal ids are validated again instead of silently skipped.
1 parent 8bc634a commit b90adb0

7 files changed

Lines changed: 176 additions & 246 deletions

File tree

‎apps/sim/lib/workflows/editing/validation.test.ts‎

Lines changed: 27 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -1267,8 +1267,7 @@ describe('collectUnresolvedReferences', () => {
12671267
})
12681268

12691269
it('validates the literal entries when a multi-select opens AND closes with a template', async () => {
1270-
// `isReference` is unanchored (startsWith '<' && endsWith '>'), so this value reads as one
1271-
// whole reference. Filtering per entry is what keeps `kb_real` validated.
1270+
// The whole string contains references, so only filtering per entry keeps `kb_real` checked.
12721271
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: ['kb_real'] })
12731272
const state = {
12741273
blocks: {
@@ -1304,63 +1303,36 @@ describe('collectUnresolvedReferences', () => {
13041303
expect(refs).toHaveLength(0)
13051304
})
13061305

1307-
it('skips a single oversized entry, which cannot be classified cheaply', async () => {
1308-
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: ['x'] })
1309-
const state = {
1310-
blocks: {
1311-
kb1: {
1312-
type: 'knowledge',
1313-
name: 'KB',
1314-
subBlocks: { knowledgeBaseId: { value: `kb_${'a'.repeat(10_000)}` } },
1315-
},
1316-
},
1317-
}
1318-
const startedAt = performance.now()
1319-
const refs = await collectUnresolvedReferences(state, CTX)
1320-
expect(performance.now() - startedAt).toBeLessThan(1000)
1321-
expect(mockValidateSelectorIds).not.toHaveBeenCalled()
1322-
expect(refs).toHaveLength(0)
1323-
})
1324-
1325-
it('keeps a comma-bearing reference intact even in an oversized list', async () => {
1326-
// Splitting scans reference regions directly and stays linear, so size does not force a
1327-
// fallback that would tear `<start.pick(a,b)>` into fragments validated as ids.
1328-
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: ['kb_missing'] })
1329-
const padding = Array.from({ length: 400 }, (_, index) => `kb_${'x'.repeat(30)}${index}`)
1330-
const value = [...padding, '<start.pick(a,b)>', 'kb_missing'].join(',')
1331-
expect(value.length).toBeGreaterThan(10_000)
1306+
it('still validates a literal id however long it is', async () => {
1307+
const value = `kb_${'a'.repeat(10_000)}`
1308+
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: [value] })
13321309
const state = {
13331310
blocks: {
13341311
kb1: { type: 'knowledge', name: 'KB', subBlocks: { knowledgeBaseId: { value } } },
13351312
},
13361313
}
1337-
await collectUnresolvedReferences(state, CTX)
1338-
1339-
const [, ids] = mockValidateSelectorIds.mock.calls[0]
1340-
expect(ids).toContain('kb_missing')
1341-
expect(ids).not.toContain('<start.pick(a')
1342-
expect(ids).not.toContain('b)>')
1314+
const refs = await collectUnresolvedReferences(state, CTX)
1315+
expect(mockValidateSelectorIds).toHaveBeenCalledWith('knowledge-base-selector', value, CTX)
1316+
expect(refs).toHaveLength(1)
13431317
})
13441318

1345-
it('still validates the literal entries of an oversized list', async () => {
1346-
// The cap gives up reference-aware splitting, not validation: the entries are still short,
1347-
// so each is classified and the literals are still checked.
1319+
it('checks the literal ids around a reference that contains a comma', async () => {
13481320
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: ['kb_missing'] })
1349-
const padding = Array.from({ length: 400 }, (_, index) => `kb_${'x'.repeat(30)}${index}`)
1350-
const value = [...padding, '<start.kbId>', 'kb_missing'].join(',')
1351-
expect(value.length).toBeGreaterThan(10_000)
13521321
const state = {
13531322
blocks: {
1354-
kb1: { type: 'knowledge', name: 'KB', subBlocks: { knowledgeBaseId: { value } } },
1323+
kb1: {
1324+
type: 'knowledge',
1325+
name: 'KB',
1326+
subBlocks: { knowledgeBaseId: { value: 'kb_a,<start.pick(a,b)>,kb_missing' } },
1327+
},
13551328
},
13561329
}
1357-
const startedAt = performance.now()
13581330
const refs = await collectUnresolvedReferences(state, CTX)
1359-
1360-
expect(performance.now() - startedAt).toBeLessThan(1000)
1361-
const [, ids] = mockValidateSelectorIds.mock.calls[0]
1362-
expect(ids).toContain('kb_missing')
1363-
expect(ids).not.toContain('<start.kbId>')
1331+
expect(mockValidateSelectorIds).toHaveBeenCalledWith(
1332+
'knowledge-base-selector',
1333+
['kb_a', 'kb_missing'],
1334+
CTX
1335+
)
13641336
expect(refs).toHaveLength(1)
13651337
})
13661338

@@ -1384,10 +1356,7 @@ describe('collectUnresolvedReferences', () => {
13841356
expect(refs).toHaveLength(1)
13851357
})
13861358

1387-
// `splitOutsideReferences` is what keeps a comma INSIDE a reference from becoming a separator.
1388-
// Every other reference test above would still pass with a naive `.split(',')` (no comma ->
1389-
// never split at all), so these two are the only ones that pin the reference-aware split from
1390-
// the consumer's side: a torn reference reads as plain literals and gets validated as ids.
1359+
// A torn reference reads as plain literals, so these pin the reference-aware split end to end.
13911360
it('does not split a <block.output> reference that contains a comma', async () => {
13921361
const state = {
13931362
blocks: {
@@ -1433,8 +1402,7 @@ describe('collectUnresolvedReferences', () => {
14331402
expect(refs).toHaveLength(0)
14341403
})
14351404

1436-
// A multi-select that already stores a native array never reaches the comma split, so the
1437-
// array filter is entered by a second, independent route.
1405+
// A native array value never reaches the comma split, so it enters the filter independently.
14381406
it('filters templates out of a value that is already an array', async () => {
14391407
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: ['kb_missing'] })
14401408
const state = {
@@ -1470,9 +1438,7 @@ describe('collectUnresolvedReferences', () => {
14701438
expect(refs).toHaveLength(0)
14711439
})
14721440

1473-
// A separator-only string is truthy, so it survives the `!subBlockValue` bail and reaches the
1474-
// split, which returns nothing. Reusing the all-references bail is what stops an empty list
1475-
// from being sent to the database as if it were a set of ids.
1441+
// A separator-only string is truthy, so it passes the `!subBlockValue` bail and splits to nothing.
14761442
it('skips a value that is nothing but separators', async () => {
14771443
const state = {
14781444
blocks: {
@@ -1495,10 +1461,7 @@ describe('collectUnresolvedReferences', () => {
14951461
expect(refs).toHaveLength(0)
14961462
})
14971463

1498-
// The per-entry filter calls `containsReference` on whatever the array holds, so its
1499-
// non-string bail is load-bearing here - without it a numeric entry throws. A non-string can
1500-
// never be a reference, so it must survive untouched, `null` included: the `filter(Boolean)`
1501-
// that would have dropped it lives inside the comma split, which a native array never reaches.
1464+
// A non-string can never be a reference, so it passes through the filter untouched.
15021465
it('does not throw on a non-string entry inside an array value', async () => {
15031466
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: [] })
15041467
const state = {
@@ -1519,8 +1482,7 @@ describe('collectUnresolvedReferences', () => {
15191482
expect(refs).toHaveLength(0)
15201483
})
15211484

1522-
// Both delimiters are required. A lone `<` (or a lone `{{`) is a malformed literal, not a
1523-
// template, and must keep being reported rather than silently waved through.
1485+
// A lone `<` or `{{` is a malformed literal, not a reference, and must still be reported.
15241486
it.each([
15251487
['an unclosed < delimiter', 'kb_<start'],
15261488
['an unopened > delimiter', 'start.kbId>'],
@@ -1557,8 +1519,7 @@ describe('collectUnresolvedReferences', () => {
15571519
expect(refs).toHaveLength(1)
15581520
})
15591521

1560-
// The guard runs AFTER the canonical active-member check, so a template in the active member
1561-
// must be skipped by the guard - and must not push mode resolution onto the empty twin.
1522+
// The guard runs after the canonical active-member check, so it must not flip the active member.
15621523
it('skips a template held by the ACTIVE canonical member', async () => {
15631524
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: ['<start.cred>'] })
15641525
const state = {
@@ -1576,9 +1537,7 @@ describe('collectUnresolvedReferences', () => {
15761537
})
15771538
})
15781539

1579-
// The lint path (collectUnresolvedReferences) and the agent edit path share collectSelectorFields,
1580-
// but only the edit path can REJECT an operation. A dynamically-bound selector must not block an
1581-
// edit - that rejection is the user-visible failure this guard exists to prevent.
1540+
// validateWorkflowSelectorIds shares collectSelectorFields with the lint, so it skips the same values.
15821541
describe('validateWorkflowSelectorIds (reference guard)', () => {
15831542
beforeEach(() => {
15841543
vi.clearAllMocks()
@@ -1589,7 +1548,7 @@ describe('validateWorkflowSelectorIds (reference guard)', () => {
15891548
['a block-output reference', '<start.kbId>'],
15901549
['an env-var reference', '{{KB_ID}}'],
15911550
['a partially templated value', 'kb_<start.suffix>'],
1592-
])('does not reject an edit whose selector holds %s', async (_label, value) => {
1551+
])('reports no error for a selector holding %s', async (_label, value) => {
15931552
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: [value] })
15941553
const state = {
15951554
blocks: {
@@ -1601,7 +1560,7 @@ describe('validateWorkflowSelectorIds (reference guard)', () => {
16011560
expect(errors).toHaveLength(0)
16021561
})
16031562

1604-
it('still rejects an edit whose selector holds a literal id that does not resolve', async () => {
1563+
it('still reports a literal id that does not resolve', async () => {
16051564
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: ['kb_gone'] })
16061565
const state = {
16071566
blocks: {

‎apps/sim/lib/workflows/editing/validation.ts‎

Lines changed: 5 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1082,17 +1082,6 @@ interface SelectorFieldToValidate {
10821082
value: string | string[]
10831083
}
10841084

1085-
/**
1086-
* Longest single selector entry `containsReference` will classify.
1087-
*
1088-
* Classifying one entry tokenizes it, and `findWorkflowReferenceTokens` is superlinear in
1089-
* candidate count, so an entry of unbounded length is expensive on a write path that admits
1090-
* megabytes. Splitting is unaffected - it scans reference regions directly and stays linear - so
1091-
* only an individual oversized ENTRY is skipped, where there is no cheap way to tell a literal
1092-
* from a dynamic binding. A long LIST of ordinary ids still splits and validates normally.
1093-
*/
1094-
const MAX_SELECTOR_ENTRY_LENGTH = 10_000
1095-
10961085
/**
10971086
* Walk a workflow state and collect selector/credential fields to validate.
10981087
* For canonical pairs only the ACTIVE member is collected (an intentionally-empty
@@ -1147,26 +1136,18 @@ function collectSelectorFields(
11471136
const subBlockValue = blockData.subBlocks?.[subBlockConfig.id]?.value
11481137
if (!subBlockValue) continue
11491138

1150-
const isOversized = (entry: unknown) =>
1151-
typeof entry === 'string' && entry.length > MAX_SELECTOR_ENTRY_LENGTH
1152-
11531139
// Handle comma-separated values for multi-select
11541140
let values: string | string[] = subBlockValue
11551141
if (typeof subBlockValue === 'string' && subBlockValue.includes(',')) {
11561142
values = splitOutsideReferences(subBlockValue)
11571143
}
11581144

1159-
// A dynamically bound value only acquires its id at execution time, so a static
1160-
// id-existence check cannot evaluate it. Filtered per entry rather than on the whole
1161-
// string, because a multi-select can mix literal ids with dynamic ones: testing
1162-
// `<a.b>,kb_real,<c.d>` as a whole would drop `kb_real` along with the references.
1145+
// A reference or env var only resolves to an id at execution time, so it cannot be checked
1146+
// here. Filtered per entry so the literal ids of a mixed multi-select are still checked.
11631147
if (Array.isArray(values)) {
1164-
const literalValues = values.filter(
1165-
(entry) => !isOversized(entry) && !containsReference(entry)
1166-
)
1167-
if (literalValues.length === 0) continue
1168-
values = literalValues
1169-
} else if (isOversized(values) || containsReference(values)) {
1148+
values = values.filter((entry) => !containsReference(entry))
1149+
if (values.length === 0) continue
1150+
} else if (containsReference(values)) {
11701151
continue
11711152
}
11721153

‎apps/sim/lib/workflows/sanitization/references.test.ts‎

Lines changed: 0 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import { describe, expect, it } from 'vitest'
22
import {
33
containsReference,
44
isLikelyReferenceSegment,
5-
splitOutsideReferences,
65
splitReferenceSegment,
76
} from '@/lib/workflows/sanitization/references'
87

@@ -91,63 +90,3 @@ describe('containsReference', () => {
9190
expect(containsReference('a<b<c>d')).toBe(false)
9291
})
9392
})
94-
95-
describe('splitOutsideReferences', () => {
96-
it('splits on separator commas', () => {
97-
expect(splitOutsideReferences('kb_a,kb_b')).toEqual(['kb_a', 'kb_b'])
98-
})
99-
100-
it('trims entries and drops empties', () => {
101-
expect(splitOutsideReferences(' kb_a , , kb_b ')).toEqual(['kb_a', 'kb_b'])
102-
})
103-
104-
it('keeps a comma that sits inside a workflow reference', () => {
105-
expect(splitOutsideReferences('<start.pick(a,b)>')).toEqual(['<start.pick(a,b)>'])
106-
})
107-
108-
it('keeps a comma inside a reference while still splitting around it', () => {
109-
expect(splitOutsideReferences('kb_a,<start.pick(x,y)>,kb_b')).toEqual([
110-
'kb_a',
111-
'<start.pick(x,y)>',
112-
'kb_b',
113-
])
114-
})
115-
116-
it('keeps a comma inside an env-var placeholder', () => {
117-
expect(splitOutsideReferences('{{A,B}},kb_a')).toEqual(['{{A,B}}', 'kb_a'])
118-
})
119-
120-
it('returns a single entry when there is no separator', () => {
121-
expect(splitOutsideReferences('kb_a')).toEqual(['kb_a'])
122-
})
123-
124-
it('stays linear on a large value instead of rescanning tokens per comma', () => {
125-
// A per-comma `tokens.some()` is O(commas x tokens) and took ~2.5s on this input.
126-
const value = '{{A}},'.repeat(40000)
127-
const startedAt = performance.now()
128-
const parts = splitOutsideReferences(value)
129-
const elapsedMs = performance.now() - startedAt
130-
131-
expect(parts).toHaveLength(40000)
132-
expect(elapsedMs).toBeLessThan(1000)
133-
})
134-
135-
it('protects a comma inside a reference that nests an env-var placeholder', () => {
136-
// `findWorkflowReferenceTokens` is non-overlapping, so it reports only the inner `{{A}}` and
137-
// drops the outer candidate. The candidate pass is what keeps the outer region protected.
138-
expect(splitOutsideReferences('<start.body.pick({{A}},b)>,kb_literal')).toEqual([
139-
'<start.body.pick({{A}},b)>',
140-
'kb_literal',
141-
])
142-
expect(splitOutsideReferences('<a.{{B}}x,y>,kb_literal')).toEqual([
143-
'<a.{{B}}x,y>',
144-
'kb_literal',
145-
])
146-
})
147-
148-
it('still splits a near-miss that does not read as a reference', () => {
149-
// `<a.b+c,d>` fails `isLikelyReferenceSegment` (the `+`), so it is not a protected region.
150-
// Loud rather than silent: the fragments are reported as ids that do not resolve.
151-
expect(splitOutsideReferences('<a.b+c,d>')).toEqual(['<a.b+c', 'd>'])
152-
})
153-
})

0 commit comments

Comments
 (0)