fix(workflows): skip dynamically bound selectors in id validation - #7799
Open
mzxchandra wants to merge 5 commits into
Open
fix(workflows): skip dynamically bound selectors in id validation#7799mzxchandra wants to merge 5 commits into
mzxchandra wants to merge 5 commits into
Conversation
A `<block.path>` or `{{ENV_VAR}}` token may legitimately contain a comma
(`<start.pick(a,b)>`), and its fragments read as plain literals once split, so a
naive `.split(',')` turns one dynamically bound value into several bogus ids.
Adds `splitOutsideReferences`, which treats only commas outside every reference
token as separators. Token spans are marked once into a lookup rather than
rescanned per comma: a per-comma `tokens.some()` is O(commas x tokens) and took
~2.5s on a 240KB value of repeated `{{A}},`, which is reachable on the 10MB
graph-write paths.
Known limitation, unchanged from the `.split(',')` this replaces and covered by a
characterization test: the tokenizer suppresses a workflow span that overlaps an
environment token, so `<start.body.pick({{A}},b)>` still splits.
Tier-2 selector validation is a static id-existence check against the workspace,
so it cannot evaluate a value whose id only arrives at execution time. A
`<block.output>` or `{{ENV_VAR}}` binding written into a selector field was
therefore reported as a resource that does not exist, on every graph write.
`collectSelectorFields` now skips those values via the existing
`containsReference`, and splits multi-select values with
`splitOutsideReferences` so a reference containing a comma is not torn into
fragments that each get validated as an id.
Filtering is per entry rather than on the whole string: a multi-select can mix
literal ids with dynamic ones, and testing `<a.b>,kb_real,<c.d>` as a whole would
drop `kb_real` along with the references.
Verified end to end against a local dev server: the three reference forms drop
from one unresolved-reference finding each to zero, while a literal id that does
not resolve still reports one.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Contributor
Author
|
@cubic review |
Contributor
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
Review round 1.
`findWorkflowReferenceTokens` is contractually non-overlapping, so for
`<a.pick({{B}},c)>` it reports only the inner `{{B}}` and discards the outer
candidate. That is correct for a tokenizer and wrong for a splitter, which needs
the union of protected regions rather than a disjoint set, so the comma was
unprotected and `c)>` was validated as a literal id.
Adds a candidate pass built from the tokenizer's own exported predicates, leaving
the shared package's non-overlapping contract untouched. It runs only when an
environment token is present, since overlap with one is the only reason a
workflow candidate is dropped.
Also caps the value length before tokenizing. Reference detection parses the
whole string and the tokenizer is superlinear in candidate count (795ms for a
240KB value of repeated `<a.b>,`), which this change newly puts on a write path
that admits megabytes. Past the cap the field is skipped rather than parsed; the
lint is advisory, so declining to check is the safe direction.
Adds `as const` to the test context object per the repo's TypeScript conventions.
Contributor
Author
Contributor
Author
|
@cubic-dev-ai review this PR |
Contributor
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
…tor value Review round 2. The length cap skipped the whole field, so an oversized list of plain literal ids lost validation it previously had. Literals never needed tokenization, so the cap was broader than the cost it was there to bound. It now gives up only the reference-aware split: past the cap the value is split plainly, and its entries are classified and validated as usual, since they are short enough that the tokenizer's per-candidate cost does not apply (1MB of literal ids across 30000 entries measures ~9ms). Only an individual entry past the cap is skipped, where there is no cheap way to tell a literal from a dynamic binding.
Contributor
Author
Review round 3.
An oversized value fell back to plain splitting, which tore a comma-bearing
reference into fragments that were then validated as literal ids. The fallback
existed to avoid `findWorkflowReferenceTokens`, which is superlinear in candidate
count.
That pass was never needed here. It returns contractually NON-overlapping tokens,
and the O(tokens^2) overlap check is the cost of producing that partition. A
splitter only needs to know whether an index sits inside SOME reference, so
scanning environment placeholders and `<...>` candidates independently gives the
union directly - cheaper and more accurate, since nothing is suppressed.
Splitting is now linear and the size fallback is gone, so a comma-bearing
reference survives at any length:
240KB of `<a.b>,` 682ms -> 15ms
240KB of `<a.p({{X}},y)>,` 177ms -> 12ms
1MB of literal ids 1ms
The length cap now applies to a single ENTRY rather than the whole field, which
is all it was ever needed for: classifying one entry tokenizes it, and there is
no cheap way to tell a literal from a dynamic binding past that size.
Exports `ENV_REFERENCE_PATTERN` from `@sim/utils` rather than duplicating the
pattern, so the two stay in step.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tier-2 selector validation (
collectSelectorFields→validateSelectorIds) is a static id-existence check against the workspace. It cannot evaluate a value whose id only arrives at execution time, so a<block.output>or{{ENV_VAR}}binding written into a selector field was reported as a resource that does not exist — on every graph write.Two commits:
fix(workflows): split multi-select values without tearing referencesA reference token may legitimately contain a comma (
<start.pick(a,b)>), and its fragments read as plain literals once split, so the existing.split(',')turned one dynamic value into several bogus ids. AddssplitOutsideReferences, which treats only commas outside every reference token as separators.fix(workflows): skip dynamically bound selectors in id validationcollectSelectorFieldsnow skips dynamically bound values via the existingcontainsReference, and splits withsplitOutsideReferences. Filtering is per entry, not on the whole string: a multi-select can mix literal ids with dynamic ones, and testing<a.b>,kb_real,<c.d>as a whole would dropkb_realalong with the references.Behaviour is unchanged for literal ids — one that does not resolve is still reported.
Performance
Token spans are marked once into a lookup rather than rescanned per comma. A per-comma
tokens.some()is O(commas × tokens):This runs synchronously on the graph-write paths, which admit bodies up to
MAX_IMPORT_BODY_BYTES(10MB), so the quadratic form was worth avoiding. A regression test asserts the 240KB case stays under 1s.Test Coverage
Legend: ★★★ behaviour + edge + error · ★★ happy path
245 tests pass across
lib/workflows/editing/andlib/workflows/sanitization/. Fullapps/simsuite green.Known limitations (characterized by tests, not fixed here)
findWorkflowReferenceTokenscollects{{...}}first and suppresses any workflow token overlapping one, so the outer<...>span is never recorded and its commas are unprotected:<start.body.pick({{A}},b)>→['<start.body.pick({{A}}', 'b)>']. This is byte-identical to the.split(',')it replaces — not a regression — and the root cause is the overlap rule in the shared@sim/utils/workflow-referencestokenizer, so fixing it belongs there.isLikelyWorkflowReferenceSegment(<a.b+c,d>, unbalanced<a.b,kb_x) gets no span and splits. Loud rather than silent.buildWorkflowLintReportiterates its collectors as a homogeneous pair, so surfacing a skip count means changing both signatures and that loop. Left out of this PR deliberately.Review
Reviewed against the repo checklist plus testing, maintainability, and security passes, and two independent adversarial passes.
lint-report.ts: "Findings never block a write"), and credentials are re-authorized at execution time byauthorizeCredentialUseForAuthindependently of anything the editor recorded, so a more lenient editor check cannot grant access.containsReferencewith a substring test (which wrongly matchedvalue <limit && value>max), used an unanchored whole-value check that let<start.a>,kb_real,<start.b>skipkb_real, and scanned tokens per comma.Test plan
bun run testinapps/sim— full suite greenbun run type-checkcleanbiome checkclean on all changed filesbun run check:api-validationpasses