## Summary
An If/Else workflow step branch whose `filterGroupId` doesn't resolve to
any entry in
`stepFilterGroups` was matching unconditionally — before any of the
step's real conditions
were evaluated — instead of being rejected. This let a single stale or
mistyped
`filterGroupId` silently hijack the routing of an entire If/Else step.
Fixes#23754
## Problem
Reproduced with a standalone unit test against `findMatchingBranch`:
```ts
const branches = [
{ id: 'branch-A', filterGroupId: 'group-id-that-does-not-exist', nextStepIds: ['wrong-step'] },
{ id: 'branch-B', filterGroupId: 'real-group', nextStepIds: ['correct-step'] },
];
const stepFilterGroups = [{ id: 'real-group', logicalOperator: 'AND' }];
const resolvedFilters = [{ /* branch-B's real filter, evaluates to false */ }];
findMatchingBranch({ branches, stepFilterGroups, resolvedFilters }).id;
// => 'branch-A' (its condition was never evaluated at all)
```
`branch-A` wins even though its `filterGroupId` doesn't exist and
`branch-B`'s actual
(non-matching) filter was correctly evaluated to `false`.
## Root cause
`find-matching-branch.util.ts` builds `branchFilterGroups` via
`collectAllDescendantGroups(branch.filterGroupId, stepFilterGroups)`,
which silently
returns an empty `Set` when the root id isn't found. The resulting empty
`branchFilterGroups`/`branchFilters` are passed to
`evaluateFilterConditions`, which treats
"both empty" as vacuously `true` — a rule that's correct for the real
trailing else-branch
(no `filterGroupId` at all, by design) but indistinguishable, at this
call site, from "the
referenced group doesn't exist." Since `Array.prototype.find` returns
the first match, this
branch wins over any later branch whose condition was actually
evaluated.
There was also no validation path that would catch this before
execution:
`validateBranchingStep` (`validate-workflow-graph.util.ts`) already
checks If/Else branch
count and `nextStepIds` connectivity, but had no check for
`filterGroupId` referential
integrity.
## Fix
1. `find-matching-branch.util.ts` — throw
`WorkflowStepExecutorException`
(`INVALID_STEP_INPUT`) when a branch's `filterGroupId` doesn't resolve
to any group,
instead of silently falling through to
`evaluateFilterConditions({filterGroups: [],
filters: []})`. This mirrors the sibling guard clauses already in this
action for other
malformed-input cases.
2. `validate-workflow-graph.util.ts` — extended the existing `IF_ELSE`
branch checks in
`validateBranchingStep` with the same check, surfaced as a new
`IF_ELSE_BRANCH_FILTER_GROUP_NOT_FOUND` issue code, so
`validate_workflow` catches this
before a workflow ever runs.
**Alternative considered:** fixing only at validation time. Rejected —
validation can be
skipped (e.g. the AI workflow-editing tool's `validate: false` option)
or bypassed entirely
by a direct API write, so the execution-time guard is the actual fix;
the validation check
is defense in depth, not a substitute.
**Alternative considered:** silently skipping the malformed branch
instead of throwing.
Rejected — throwing immediately gives a specific, actionable error
pointing at the exact
misconfiguration, matching this file's existing error granularity
(distinct messages for
"not an if-else step", "no branches", "missing filter groups/filters",
"no matching
branch").
## Tests
- `find-matching-branch.util.spec.ts` (new) — real-condition match,
else-branch fallback
match, throws on a dangling `filterGroupId` (fails on `main`, passes
here), throws when no
branch matches and there's no else branch.
- `validate-workflow-graph.util.test.ts` (+2) — flags
`IF_ELSE_BRANCH_FILTER_GROUP_NOT_FOUND`
for a dangling reference; does not false-positive on a
correctly-configured branch.
- Full module suites: `npx nx test twenty-server` scoped to
`src/modules/workflow` →
68 suites / 626 tests passed. `npx jest
packages/twenty-shared/src/workflow` → 30 suites /
248 tests passed.
- `npx nx lint twenty-server twenty-shared` and `npx nx typecheck
twenty-server
twenty-shared` → clean.
## Compatibility / risk
Internal-only change to workflow execution and validation logic — no
GraphQL schema
change, no public API signature change, no migration. A workflow that
today relies
(accidentally) on the silent "dangling group = always match" behavior
would start throwing
at execution time, but that was never intentional or documented
behavior.
## Out of scope
- Branch **ordering** invariants (e.g. asserting the group-less else
branch is always
last) — not needed for this fix; the defect reproduces purely from a
dangling
`filterGroupId`, independent of order.
<!-- This is an auto-generated description by cubic. -->
<a
href="https://cubic.dev/pr/twentyhq/twenty/pull/23758?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="Review in cubic"
src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->
---------
Co-authored-by: Thomas Trompette <thomas.trompette@sfr.fr>
blockgroot
0f35b5895a
