fix(workflows): scope the canonical sub-block index to the active surface - #6990
Conversation
…face A block that is both an action and a trigger holds one `subBlocks` array — its own fields plus its trigger's, spread in after them. The two sets routinely share a `canonicalParamId` under different ids, so indexing them together collapses a trigger field into an action pair whose `basicId` it can never be. Every group-relative question about that field then answers for the dormant surface. The serializer was never affected: `shouldSerializeSubBlock` drops the inactive surface before the canonical collapse reads it, so it resolves against a value map the dormant surface cannot appear in. Every other caller resolves against the block's full value map, so the scoping has to live in the index. - add `getCanonicalSubBlocksForSurface` / `buildCanonicalIndexForSurface`, and move the three sites that already had the filter inline onto them - `getCardSubBlocks` derives its own index instead of accepting one; it already took `triggerMode`, and accepting an index is what let all three callers pass one built for the other surface - scope the remaining consumers that resolve against a full value map: the canvas card, autolayout, both preview surfaces, the dependsOn gate, the canonical value hook, reactive conditions, and the copilot selector lint - keep a canonical group with no advanced member out of the legacy `advancedMode` path, which deleted its basic member and republished nothing - merge legacy type-scoped tool modes as a baseline under index-scoped ones, so the first re-toggle stops reverting the ids the user has not touched
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Adds Also: Reviewed by Cursor Bugbot for commit 4a083b9. Configure here. |
Greptile SummaryThis PR scopes canonical sub-block indexing to the active action or trigger surface so UI, fork/sync, preview, and validation paths resolve overlapping canonical fields consistently.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/workflows/subblocks/visibility.ts | Introduces the central active-surface projection and canonical-index helper used throughout the change. |
| apps/sim/ee/workspace-forking/lib/remap/remap-references.ts | Scopes canonical mode gates during fork remapping while retaining the full index for dormant-member classification. |
| apps/sim/lib/workflows/blocks/canvas-card-fields.ts | Builds the canonical index from the requested card surface to prevent action fields from suppressing trigger fields. |
| apps/sim/serializer/index.ts | Retains deliberate filter-before-collapse serializer behavior and fixes canonical groups without an advanced member. |
| scripts/check-canonical-index-surface.ts | Adds a static audit guarding surface-sensitive canonical-index call sites. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Block[Block configuration] --> Surface{Active surface}
Surface -->|Action| Action[Action sub-blocks]
Surface -->|Trigger| Trigger[Trigger sub-blocks]
Action --> Index[Canonical index]
Trigger --> Index
Index --> Consumers[Editor, canvas, preview, fork/sync, validation]
Reviews (3): Last reviewed commit: "fix(audits): stop the canonical-index gu..." | Re-trigger Greptile
…ve surface `createCanonicalModeGates` indexed a block's whole `subBlocks` array, so on a mixed action/trigger block a trigger field sharing a `canonicalParamId` with an action pair was read as a member of THAT pair. Being neither its `basicId` nor in its `advancedIds`, `isDormantMember` answered true the moment the shared mode resolved to advanced — and a fork acts on that by clearing the value, so a configured trigger field was silently wiped on fork/sync. Reachable without any explicit toggle: a block configured as an action with a manual id and then switched to trigger mode leaves the pair's value heuristic resolving to advanced on its own. - `createCanonicalModeGates` takes the surface and scopes its index - thread `triggerMode` through `RemapForkContext`, `SubBlockTransform`, `clearDependentsOnRemap`, `collectClearedDependents`, the reference scanners, and the promote cleared-ref collectors - nested tool params and the dependent scan are unchanged: a tool is always the action surface, and the dependent scan already narrows its configs
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
… before scoping Surface scoping decides canonical membership for the ACTIVE surface. Applying it to every key also re-classified the dormant surface's own values: they stopped being dormant members, which meant the remap no longer cleared them AND started detecting them as references — turning a stale action selector on a trigger-mode block into a mapping requirement that can block promote/sync. The gates now pick the index per key: the scoped one for anything the active surface defines (the fix — a trigger field gets its own group instead of being read as a stranded member of an action pair), the whole array for everything else, which is byte-for-byte the pre-scoping behavior. Also adds `check:canonical-index`, an audit that fails any call building a canonical index off a config's whole `subBlocks`, or calling the fork gates without a surface, unless annotated with why. This defect shipped three times in three subsystems; the 14 sites that legitimately mean one fixed surface now say so at the call.
|
Good catch on the inactive-surface refs — that was a real regression in the fork commit, fixed in 2d53b4a. Scoping the gates was meant to stop a live trigger field being misread as a stranded member of an action pair (which made The gates now pick the index per key: the scoped one for anything the active surface defines, the whole array for everything else. That leaves the dormant surface byte-for-byte as it was before scoping existed. Worth noting the blast radius was narrower than "High" implies: Two tests pin both directions, and I verified each fails against the previous commit: one asserts the dormant action members classify identically to the unscoped gates, and one drives |
The audit holds `buildCanonicalIndex(` and `createCanonicalModeGates(` as string literals to search for, and its own regex matched them — the arg-count rule then fired on the literal. It passed locally only because the file was still untracked when it ran, so `git ls-files` did not list it; committing it made the audit scan itself and fail CI on the first run. Exempts the audit's own source alongside the module that defines the primitives. Verified the guard still fails on both regression shapes after the exemption.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 4a083b9. Configure here.
1 participant
Summary
subBlocksarray — its own fields plus its trigger's, spread in after them. The two sets routinely share acanonicalParamIdunder different ids (siteSelector/manualSiteIdvstriggerSiteId, allsiteId), so indexing them together collapses the trigger field into an action pair whosebasicIdit can never be. Every group-relative question about that field then answers for the dormant surface.getCanonicalSubBlocksForSurface/buildCanonicalIndexForSurfaceand moved the three sites that already had the filter inline onto them.createCanonicalModeGatesindexed the whole array, soisDormantMemberanswered true for a trigger field the moment the shared mode key resolved to advanced — and the documented policy for a dormant member is to clear its value outright. Reachable with no explicit toggle: configure a block as an action with a manual id, switch it to trigger mode, and the pair's value heuristic resolves to advanced on its own. ThreadedtriggerModethroughRemapForkContext,SubBlockTransform,clearDependentsOnRemap,collectClearedDependents, the reference scanners, and the promote cleared-ref collectors.getCardSubBlocksnow derives its own index instead of accepting one; it already tooktriggerMode, and accepting an index is what let all three callers pass one built for the other surface. 12 blocks were affected on the card and in the autolayout height estimate — Airtable's Base ID and Table ID (both required), Webflow's Site and Collection, and eight blocks' trigger Credentials.dependsOngate, the canonical value hook, reactive conditions, and the copilot selector lint.advancedModepath, which forced'advanced', resolved nothing, and still deleted the basic member — so the block serialized without a field the user had filled and then reported it as missing and required (posthog.apiKey,hubspot.objectType,managed_agent.environmentType, and 6 more).The serializer keeps the unscoped index deliberately, and that is documented:
shouldSerializeSubBlockdrops the inactive surface before the canonical collapse reads it, so it resolves against a value map the dormant surface cannot appear in. That filter-then-resolve ordering is why execution has always been correct here. Trigger config never travels this path anyway —buildProviderConfigindexestriggerDef.subBlocks, projected by construction. Nested tool params and the fork dependent scan are also unchanged: a tool is always the action surface, and the dependent scan already narrows its configs.No API, schema, persisted-field, or generated-artifact changes.
Type of Change
Testing
remapForkSubBlocks, asserting a live trigger field survives a remap under an advanced shared mode, plus a no-regression guard that the action surface still gates normally. Both new assertions confirmed failing before the fix.advancedModecase, including that the surviving value is no longer reported as a missing required field. Both confirmed failing before the fix.ee,lib/workflows,lib/copilot,hooks,serializer; 11,087 acrossee,stores,triggers,tools,providers,executor,blocks; 5,680 acrosslib/workflows,app/workspace,hooks.turbo run type-check26/26,bun run lintclean, all 32 audits incheck:auditspass,check-block-registryagainst staging passes.Checklist