diff --git a/.changeset/flow-lint-loop-body-descent.md b/.changeset/flow-lint-loop-body-descent.md new file mode 100644 index 0000000000..1ae73e870d --- /dev/null +++ b/.changeset/flow-lint-loop-body-descent.md @@ -0,0 +1,52 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): the flow rule family now descends into `loop` bodies and every other nested region (#5383) + +The flow anti-pattern rules read a flow's `nodes` / `edges` **flat off the top +level**, so every rule in the family was blind to anything authored inside an +ADR-0031 container — a `loop` body, a `parallel` branch, a `try_catch` +try/catch. Loop bodies are where a lot of real branching lives (a per-item gate +inside a sweep is the standard shape for a scheduled flow), so this was a large +share of authorable flow metadata that no flow rule inspected. + +Measured in a real app: 8 `decision` nodes carried the inert singular +`config.condition` that `flow-inert-node-condition` exists to catch, all 8 +inside a `loop` body, and `pnpm lint` reported none of them. The identical key +on a **top-level** decision in the same repo fired immediately — same key, same +node type, only the nesting depth differed. The blind spot also explains its own +survival: the gate visibly worked where it could see, so the top-level copies +got cleaned up while the nested ones read as approved. + +Rules now reported at every depth: `flow-inert-node-condition`, +`flow-decision-unconditional-branch`, `flow-branch-label-unmatched`, +`flow-default-edge-with-condition`, `flow-multiple-default-edges`, +`flow-double-brace-interpolation`, `flow-bare-dollar-reference`, +`flow-date-equality-filter`, `flow-phantom-aggregation`, +`flow-error-label-not-fault`, and the `flow-approval-revise-*` family. Note the +severity asymmetry this closes: `flow-default-edge-with-condition` is a +build-stopping `error` that until now could not see a contradiction authored one +level down. + +A finding inside a region carries the region scope in its `where`, so the +message still points at exactly one node — `flow 'x' · loop 'sweep' body · +node 'y' (decision)`, matching the scope vocabulary the engine's registration +pass already uses. Findings on a flow's own graph are unchanged, byte for byte. + +Two details worth knowing if you consume these findings: + +- Each region is scanned against **its own** `edges`. The branch-routing rules + reason about a node together with its out-edges, and a region is a + self-contained sub-graph, so a nested decision's out-edges live in the + region's own edge list. +- `flow-double-brace-interpolation` / `flow-bare-dollar-reference` scan a node's + config recursively, and a container's config physically contains its + descendants'. A nested hit was therefore already *visible* before this change + — but attributed to the enclosing `loop` rather than the node carrying the + string. Such a finding now names the right node, and is still reported exactly + once. + +`flow-runas-unscoped` deliberately keeps looking at top-level nodes only: +widening a build-gating rule is its own change with its own blast radius, and is +tracked separately. diff --git a/packages/lint/src/flow-walk.ts b/packages/lint/src/flow-walk.ts index a534cb53f4..b8b8be6ae8 100644 --- a/packages/lint/src/flow-walk.ts +++ b/packages/lint/src/flow-walk.ts @@ -118,8 +118,18 @@ export function flowNodeLabel(node: AnyRec, index: number): string { return strName(node.label) ?? strName(node.id) ?? `#${index}`; } -/** `config` minus the region slots, or `undefined` when there is no config. */ -function stripRegions(config: unknown): AnyRec | undefined { +/** + * `config` minus the region slots, or `undefined` when there is no config. + * + * Copy-on-write: a config with no region key comes back by reference. + * + * Exported since #5383 because {@link WalkedFlowNode.localConfig} is not the only + * consumer that needs this view. `lint-flow-patterns.ts` walks graphs rather than + * nodes (it needs each region's `edges` too, which this walk does not carry), but + * its recursive config scans hit the identical double-count trap described above — + * so it reads the same region-stripped view, from this one definition. + */ +export function stripRegions(config: unknown): AnyRec | undefined { if (!isRec(config)) return undefined; let out: AnyRec | undefined; for (const key of Object.keys(config)) { diff --git a/packages/lint/src/lint-flow-patterns.test.ts b/packages/lint/src/lint-flow-patterns.test.ts index dfdab74a4b..4722ff91a9 100644 --- a/packages/lint/src/lint-flow-patterns.test.ts +++ b/packages/lint/src/lint-flow-patterns.test.ts @@ -742,3 +742,256 @@ describe('flow-inert-node-condition (#4414)', () => { expect(lintFlowPatterns(conditionNodeFlow('acme_custom_step', { condition: 'a == b' }))).toHaveLength(0); }); }); + +/** + * #5383 — the rule family used to read `flow.nodes` / `flow.edges` FLAT, so every + * rule in it was blind to anything authored inside an ADR-0031 container. + * + * Measured in a real app (HotCRM): 8 `decision` nodes carried the inert singular + * `config.condition` that `flow-inert-node-condition` exists to catch, all 8 + * inside a `loop` body, and `pnpm lint` reported none. The identical key on a + * TOP-LEVEL decision in the same repo fired immediately — same key, same node + * type, only the nesting depth differed. There was no loop-body fixture anywhere + * in this file, which is consistent with the gap going unnoticed for that long. + * + * Every case below pins the nested finding against its top-level twin, so a + * future flattening of the walk fails here instead of going quiet again. + */ + +/** A scheduled sweep: `loop` over leads, with `body` holding the per-item graph. */ +function loopBodyFlow(body: { nodes: unknown[]; edges: unknown[] }) { + return { + flows: [{ + name: 'campaign_enrollment', + runAs: 'system', + nodes: [ + { id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 9 * * *' } }, + { + id: 'loop_leads', type: 'loop', label: 'Loop Leads', + config: { collection: '{vars.leads}', itemVar: 'lead', body }, + }, + { id: 'end', type: 'end' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'loop_leads' }, + { id: 'e2', source: 'loop_leads', target: 'end' }, + ], + }], + }; +} + +describe('#5383 — flow-inert-node-condition descends into a loop body', () => { + // The shipped shape, reduced: a per-item gate inside a sweep, whose predicate + // was written on the node instead of its out-edges. + const nested = () => loopBodyFlow({ + nodes: [ + { id: 'check_not_enrolled', type: 'decision', config: { condition: 'lead.enrolled == false' } }, + { id: 'enroll', type: 'create_record', config: { objectName: 'campaign_member' } }, + ], + edges: [{ id: 'b1', source: 'check_not_enrolled', target: 'enroll' }], + }); + + it('flags it, scoped to the region so the message still names exactly one node', () => { + const fnds = lintFlowPatterns(nested()); + // Exactly one finding overall: no collateral from the descent, and no second + // copy reported against the enclosing `loop`. + expect(fnds).toHaveLength(1); + expect(fnds[0].rule).toBe(FLOW_INERT_NODE_CONDITION); + expect(fnds[0].where).toBe( + "flow 'campaign_enrollment' · loop 'loop_leads' body · node 'check_not_enrolled' (decision)", + ); + expect(fnds[0].message).toContain('nothing reads it'); + // The decision-specific hint still applies one level down. + expect(fnds[0].hint).toContain('isDefault'); + expect(fnds[0].severity).toBeUndefined(); + }); + + it('still reports the TOP-LEVEL twin with no region breadcrumb (the A/B)', () => { + const fnds = lintFlowPatterns({ + flows: [{ + name: 'campaign_enrollment', + runAs: 'system', + nodes: [ + { id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 9 * * *' } }, + { id: 'check_not_enrolled', type: 'decision', config: { condition: 'lead.enrolled == false' } }, + ], + edges: [{ id: 'e1', source: 'start', target: 'check_not_enrolled' }], + }], + }); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toBe("flow 'campaign_enrollment' · node 'check_not_enrolled' (decision)"); + expect(fnds[0].where).not.toContain('loop'); + }); + + it('descends a loop nested inside a loop — same depth semantics as the engine', () => { + const fnds = lintFlowPatterns(loopBodyFlow({ + nodes: [{ + id: 'loop_touchpoints', type: 'loop', + config: { + collection: '{lead.touchpoints}', itemVar: 'tp', + body: { + nodes: [{ id: 'check_recent', type: 'decision', config: { condition: 'tp.age_days < 7' } }], + edges: [], + }, + }, + }], + edges: [], + })).filter((f) => f.rule === FLOW_INERT_NODE_CONDITION); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toBe( + "flow 'campaign_enrollment' · loop 'loop_leads' body → loop 'loop_touchpoints' body · " + + "node 'check_recent' (decision)", + ); + }); + + it('descends a parallel branch too — the scope names the branch index', () => { + const fnds = lintFlowPatterns({ + flows: [{ + name: 'fan_out', + runAs: 'system', + nodes: [ + { id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 9 * * *' } }, + { + id: 'fan', type: 'parallel', + config: { + branches: [ + { name: 'owner', nodes: [{ id: 'gate', type: 'decision', config: { condition: 'a == b' } }], edges: [] }, + { name: 'watchers', nodes: [{ id: 'ping', type: 'notify', config: { title: 'Hi {record.name}' } }], edges: [] }, + ], + }, + }, + ], + edges: [{ id: 'e1', source: 'start', target: 'fan' }], + }], + }).filter((f) => f.rule === FLOW_INERT_NODE_CONDITION); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toBe("flow 'fan_out' · parallel 'fan' branch 0 · node 'gate' (decision)"); + }); +}); + +/** + * #5383 — the branch-routing family reasons about a node together with its + * OUT-EDGES, so the walk has to hand each region its own `edges` array. These + * cases are unreachable from the top-level edge list by construction: nothing at + * the top level has `gate` or `push` as a source, so a walk that descended into + * region NODES while still reading top-level EDGES would see zero out-edges and + * skip every one of them. + */ +describe('#5383 — the branch-routing family reads the region’s own edges', () => { + it('flags a nested edge that is both the default and conditional (GATING)', () => { + const fnds = lintFlowPatterns(loopBodyFlow({ + nodes: [ + { id: 'gate', type: 'decision' }, + { id: 'nudge', type: 'notify', config: { title: 'Nudge {lead.name}' } }, + { id: 'skip', type: 'end' }, + ], + edges: [ + { id: 'b1', source: 'gate', target: 'nudge', condition: 'lead.score > 50' }, + { id: 'b2', source: 'gate', target: 'skip', isDefault: true, condition: 'lead.score <= 50' }, + ], + })); + expect(fnds).toHaveLength(1); + expect(fnds[0].rule).toBe(FLOW_DEFAULT_EDGE_WITH_CONDITION); + // The severity asymmetry the issue called out: a build-stopping rule that + // could not see a contradiction authored one level down. + expect(fnds[0].severity).toBe('error'); + expect(fnds[0].where).toBe( + "flow 'campaign_enrollment' · loop 'loop_leads' body · edge 'gate' → 'skip'", + ); + expect(fnds[0].message).toContain('contradictory'); + }); + + it('flags a nested unconditional out-edge alongside a guarded sibling', () => { + const fnds = lintFlowPatterns(loopBodyFlow({ + nodes: [ + { id: 'gate', type: 'decision' }, + { id: 'nudge', type: 'notify', config: { title: 'Nudge {lead.name}' } }, + { id: 'log', type: 'create_record', config: { objectName: 'touch_log' } }, + ], + edges: [ + { id: 'b1', source: 'gate', target: 'nudge', condition: 'lead.score > 50' }, + { id: 'b2', source: 'gate', target: 'log' }, + ], + })).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toBe("flow 'campaign_enrollment' · loop 'loop_leads' body · decision 'gate'"); + expect(fnds[0].message).toContain("'log'"); + }); + + it('flags a nested error-labelled edge left at the default type', () => { + const fnds = lintFlowPatterns(loopBodyFlow({ + nodes: [ + { id: 'push', type: 'http', config: { url: 'https://example.test/hook' } }, + { id: 'handle', type: 'create_record', config: { objectName: 'sync_error' } }, + ], + edges: [{ id: 'b1', source: 'push', target: 'handle', label: 'error' }], + })).filter((f) => f.rule === FLOW_ERROR_LABEL_NOT_FAULT); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toBe( + "flow 'campaign_enrollment' · loop 'loop_leads' body · edge 'push' → 'handle'", + ); + }); + + it('does NOT merge two regions into one bag — a shared node id is not a fan-out', () => { + // `gate` exists in BOTH branches, each with exactly ONE default out-edge. + // Node ids are unique per graph, not per flow, so flattening every region + // into one node bag + one edge bag would see two `isDefault` edges out of + // "gate" and raise flow-multiple-default-edges — a finding neither region + // contains. Pairing each region with its own edges is what keeps this quiet. + const branch = (cond: string) => ({ + nodes: [ + { id: 'gate', type: 'decision' }, + { id: 'x', type: 'end' }, + { id: 'y', type: 'end' }, + ], + edges: [ + { id: 'g1', source: 'gate', target: 'x', condition: cond }, + { id: 'g2', source: 'gate', target: 'y', isDefault: true }, + ], + }); + const fnds = lintFlowPatterns({ + flows: [{ + name: 'twin_regions', + runAs: 'system', + nodes: [ + { id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 9 * * *' } }, + { + id: 'fan', type: 'parallel', + config: { + branches: [ + { name: 'a', ...branch('lead.score > 50') }, + { name: 'b', ...branch('lead.score > 90') }, + ], + }, + }, + ], + edges: [{ id: 'e1', source: 'start', target: 'fan' }], + }], + }); + expect(fnds.filter((f) => f.rule === FLOW_MULTIPLE_DEFAULT_EDGES)).toHaveLength(0); + expect(fnds).toHaveLength(0); + }); +}); + +describe('#5383 — a recursive config scan does not double-report the container', () => { + it('moves a nested double-brace finding onto the node carrying it, still exactly once', () => { + const fnds = lintFlowPatterns(loopBodyFlow({ + nodes: [{ id: 'send_reminder', type: 'notify', config: { title: 'Reminder: {{lead.name}}' } }], + edges: [], + })); + // Exactly one. The `loop`'s own config physically CONTAINS `body`, and + // `collectTemplateStrings` is recursive, so descending without stripping the + // region slots would report this a SECOND time against 'loop_leads'. + expect(fnds).toHaveLength(1); + expect(fnds[0].rule).toBe(FLOW_DOUBLE_BRACE_INTERP); + expect(fnds[0].where).toBe( + "flow 'campaign_enrollment' · loop 'loop_leads' body · node 'send_reminder' (notify)", + ); + // Before #5383 the COUNT was already 1 here — the string was found by + // recursing through the container's config and attributed to the `loop`. + // That is the `validate-flow-template-paths` failure mode (#4380): visible, + // but judged against a node that does not carry the string. So for this rule + // the fix is re-attribution, not new visibility. + expect(fnds[0].where).not.toContain("node 'loop_leads'"); + }); +}); diff --git a/packages/lint/src/lint-flow-patterns.ts b/packages/lint/src/lint-flow-patterns.ts index 58b10ffcd0..91d7331120 100644 --- a/packages/lint/src/lint-flow-patterns.ts +++ b/packages/lint/src/lint-flow-patterns.ts @@ -43,9 +43,65 @@ * shape is a daily SCHEDULE trigger + a range query. We flag the equality form * specifically (range operators `>=`/`<=` are not flagged — they're the building * block of the correct pattern), keeping false positives near zero. + * + * ## Every graph in the flow, not just the top-level one (#5383) + * + * These rules used to read `flow.nodes` / `flow.edges` flat, so every one of + * them was blind to anything authored inside an ADR-0031 container: a `loop` + * body, a `parallel` branch, a `try_catch` try/catch. That is not a corner — + * a per-item gate inside a sweep is the standard shape for a scheduled flow, and + * it is exactly where the rules were needed. Measured in a real app (HotCRM): + * 8 `decision` nodes carried the inert singular `config.condition` that + * {@link FLOW_INERT_NODE_CONDITION} exists to catch, all 8 inside a `loop` body, + * and `pnpm lint` reported none of them. The identical key on a TOP-LEVEL + * decision in the same repo fired immediately — same key, same node type, only + * the nesting depth differed. The blind spot also explains its own survival: the + * gate visibly worked where it could see, so the top-level copies got cleaned up + * and the nested ones read as approved. + * + * The fix is to iterate {@link collectFlowGraphs} — the same traversal the + * engine's registration pass uses (`validateNodeConfigKeys`, + * `validateFlowExpressions`) and that `validate-expressions.ts` already uses on + * the author side — and to prefix each finding's `where` with + * {@link FlowGraph.scope}, so a message still points at exactly one node + * (`flow 'x' · loop 'sweep' body · node 'y' (decision)`). + * + * Two things about that walk are load-bearing here, not incidental: + * + * - **nodes and edges stay PAIRED per region.** A region is a self-contained + * sub-graph: its edges join its own nodes, and no top-level edge reaches into + * it. Flattening every region into one node bag plus one edge bag would break + * the branch-routing family in both directions — a nested `decision`'s + * out-edges would be absent from the top-level edge list, so it would read as + * having none and be skipped outright (`outs.length === 0`), while two nodes + * in *different* regions sharing an id (ids are unique per graph, not per + * flow) would have their out-edges merged into one phantom fan-out. Each + * graph is therefore scanned against its own `edges`. + * - **a container's own config is read region-STRIPPED for the recursive + * scans.** {@link collectTemplateStrings} walks a node's config to its string + * leaves, and a container's config physically CONTAINS every descendant's. + * Before this change that produced a *mis-attributed* finding rather than a + * missing one: a `{{ }}` inside a loop body was reported against the `loop` + * node, the same failure mode `validate-flow-template-paths` had (#4380) — + * visible, but judged against the wrong node. Descending without stripping + * would have turned that into a DOUBLE report (once at the container, once at + * the node). Stripping the region slots moves each such finding onto the node + * that actually carries the string, and the count stays 1. + * + * Deliberately still flow-level, i.e. read off the top-level nodes only: the + * start-node trigger rules (a trigger is a property of the flow, and a region + * has an entry node, not a `start`) and {@link FLOW_RUNAS_UNSCOPED}, whose + * data-node search is left alone here on purpose — widening a build-GATING rule + * is its own change with its own blast radius, filed separately. */ -import { APPROVAL_NODE_TYPE, APPROVAL_REVISE_NODE_TYPE } from '@objectstack/spec/automation'; +import { + APPROVAL_NODE_TYPE, + APPROVAL_REVISE_NODE_TYPE, + collectFlowGraphs, +} from '@objectstack/spec/automation'; +import type { FlowNodeParsed, FlowEdgeParsed } from '@objectstack/spec/automation'; +import { stripRegions } from './flow-walk.js'; export interface FlowLintFinding { where: string; @@ -323,7 +379,7 @@ function edgeLabelOf(e: AnyRec): string { * See {@link ERROR_LABELS} for why this is a footgun and what is excluded. */ function scanErrorLabelledEdges( - flowName: string, + at: string, nodes: AnyRec[], edges: AnyRec[], findings: FlowLintFinding[], @@ -343,7 +399,7 @@ function scanErrorLabelledEdges( if (BRANCH_LABEL_NODE_TYPES.has(typeById.get(src) ?? '')) continue; findings.push({ - where: `flow '${flowName}' · edge '${src}' → '${String(e.target)}'`, + where: `${at} · edge '${src}' → '${String(e.target)}'`, message: `edge is labelled '${String(e.label)}' but its type is '${String(e.type ?? 'default')}', not 'fault' — ` + `so it is an ORDINARY out-edge. Unconditional out-edges all run in parallel, so '${String(e.target)}' ` + @@ -398,7 +454,7 @@ function scanErrorLabelledEdges( * this rule existed still reaches run time. */ function scanBranchRouting( - flowName: string, + at: string, nodes: AnyRec[], edges: AnyRec[], findings: FlowLintFinding[], @@ -418,7 +474,7 @@ function scanBranchRouting( for (const e of outs) { if (e.isDefault === true && e.condition) { findings.push({ - where: `flow '${flowName}' · edge '${src}' → '${String(e.target)}'`, + where: `${at} · edge '${src}' → '${String(e.target)}'`, message: `edge sets \`isDefault: true\` AND a \`condition\` — contradictory. \`isDefault\` means ` + `"take this edge when NO sibling condition matched"; a condition makes it an ordinary ` + @@ -436,7 +492,7 @@ function scanBranchRouting( const defaults = outs.filter((e) => e.isDefault === true && !e.condition); if (defaults.length > 1) { findings.push({ - where: `flow '${flowName}' · node '${src}'`, + where: `${at} · node '${src}'`, message: `${defaults.length} out-edges are marked \`isDefault: true\` (${defaults .map((e) => `'${String(e.target)}'`) @@ -470,7 +526,7 @@ function scanBranchRouting( const cfg = (node.config ?? {}) as AnyRec; if (cfg.condition == null || conditionSource(cfg.condition).trim() === '') continue; findings.push({ - where: `flow '${flowName}' · node '${String(node.id)}' (${nodeType})`, + where: `${at} · node '${String(node.id)}' (${nodeType})`, message: `\`config.condition\` is set but nothing reads it — the key is the trigger gate on a \`start\` ` + `node and is ignored on every other node type, so this predicate never gates anything. ` + @@ -508,7 +564,7 @@ function scanBranchRouting( const unclaimed = [...declaredLabels].filter((l) => !edgeLabels.has(l)); if (unclaimed.length > 0) { findings.push({ - where: `flow '${flowName}' · decision '${nid}'`, + where: `${at} · decision '${nid}'`, message: `declares branch label(s) ${unclaimed.map((l) => `'${l}'`).join(', ')} that no out-edge ` + `carries — out-edge labels are [${[...edgeLabels].map((l) => `'${l}'`).join(', ') || 'none'}]. ` + @@ -534,7 +590,7 @@ function scanBranchRouting( ); if (ungated.length > 0) { findings.push({ - where: `flow '${flowName}' · decision '${nid}'`, + where: `${at} · decision '${nid}'`, message: `has guarded out-edge(s) alongside unconditional one(s) ` + `(${ungated.map((e) => `'${String(e.target)}'`).join(', ')}) — an unconditional out-edge is ` + @@ -551,7 +607,7 @@ function scanBranchRouting( } function scanApprovalReviseLoops( - flowName: string, + at: string, nodes: AnyRec[], edges: AnyRec[], findings: FlowLintFinding[], @@ -580,7 +636,7 @@ function scanApprovalReviseLoops( .map((e) => (typeof e.target === 'string' ? e.target : '')) .filter((t) => t && nodeIds.has(t)); if (reviseTargets.length === 0) continue; // only approvals that declare a revise branch - const where = `flow '${flowName}' \u00b7 approval '${aid}'`; + const where = `${at} \u00b7 approval '${aid}'`; // #3823 / amended ADR-0044 \u2014 the revise window must be the service-owned // pause. `error`, under this module's stated bar ("the runtime refuses"): @@ -671,8 +727,10 @@ function scanApprovalReviseLoops( } /** - * Lint every flow's start node for known authoring anti-patterns. Returns a - * (possibly empty) list of advisory findings — never throws, never fails a build. + * Lint every flow for known authoring anti-patterns — its own graph AND every + * nested ADR-0031 region (#5383). Returns a (possibly empty) list of findings; + * never throws. A finding marked `severity: 'error'` fails the build, and since + * #5383 it can be raised by a node inside a `loop` body too. */ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] { const findings: FlowLintFinding[] = []; @@ -737,70 +795,98 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] { } } - // (b) #1315 — wrong interpolation syntax in any node's template values. Flow - // node values use SINGLE braces; double-brace `{{ }}` and bare `$ref.x` - // are carried over from the formula template dialect / other platforms. - for (const node of nodes) { - const nodeWhere = `flow '${flowName}' · node '${node.id}' (${node.type})`; - - // (a2) #1874 — date-EQUALITY (`==`/`$eq`/`$in`) against a time value in a - // query filter. A scheduled flow that filters this way silently matches - // nothing; the robust shape is a `$gte`/`$lt` day window. - const cfg = (node.config ?? {}) as AnyRec; - if (cfg.filter) scanFilterForDateEquality(cfg.filter, `${nodeWhere} filter`, findings); - - // (a3) #1870 — a node-config key naming a non-existent capability (there is - // no aggregate node) is silently ignored at runtime, so the node - // computes nothing. Point the author at the data-layer equivalent. - for (const key of Object.keys(cfg)) { - if (PHANTOM_AGG_KEYS.has(key)) { - findings.push({ - where: nodeWhere, - message: - `node config has \`${key}\` — the automation engine has no aggregate node, so \`${key}\` is ` + - `silently ignored and this node computes nothing at runtime.`, - hint: - `Aggregation belongs in the data layer: use \`Field.summary\` for a cross-object rollup ` + - `(sum/count of children), or \`Field.formula\` for a per-record computed value. (#1870)`, - rule: FLOW_PHANTOM_AGGREGATION, - }); + // (b)–(e) #5383 — every graph in the flow, not just the top-level one: its + // own `nodes`/`edges` plus each nested ADR-0031 region, each scanned + // against ITS OWN edge list. `scope` is empty for the flow's own graph, + // so `at` is byte-identical to the old prefix there and only a nested + // finding gains the region breadcrumb. See the module header for why the + // per-region pairing and the region-strip below are load-bearing. + for (const graph of collectFlowGraphs({ + // A cast, not a parse. `FlowNodeSchema.config` is an open `z.record`, so a + // region's contents arrive as raw authored records even in a parsed stack — + // a nested edge `condition` may still be a bare string where a top-level + // one is an Expression envelope. Every rule below reads both + // (`conditionSource`), and the walk itself only touches `type` / `config`. + // The already-guarded arrays are passed rather than `flow` itself so a + // non-array `nodes` still cannot throw: this function promises it never does. + nodes: nodes as unknown as FlowNodeParsed[], + edges: edges as unknown as FlowEdgeParsed[], + })) { + const at = graph.scope ? `flow '${flowName}' · ${graph.scope}` : `flow '${flowName}'`; + const graphNodes = graph.nodes as unknown as AnyRec[]; + const graphEdges = graph.edges as unknown as AnyRec[]; + + // (b) #1315 — wrong interpolation syntax in any node's template values. Flow + // node values use SINGLE braces; double-brace `{{ }}` and bare `$ref.x` + // are carried over from the formula template dialect / other platforms. + for (const node of graphNodes) { + const nodeWhere = `${at} · node '${node.id}' (${node.type})`; + + // (a2) #1874 — date-EQUALITY (`==`/`$eq`/`$in`) against a time value in a + // query filter. A scheduled flow that filters this way silently matches + // nothing; the robust shape is a `$gte`/`$lt` day window. + const cfg = (node.config ?? {}) as AnyRec; + if (cfg.filter) scanFilterForDateEquality(cfg.filter, `${nodeWhere} filter`, findings); + + // (a3) #1870 — a node-config key naming a non-existent capability (there is + // no aggregate node) is silently ignored at runtime, so the node + // computes nothing. Point the author at the data-layer equivalent. + for (const key of Object.keys(cfg)) { + if (PHANTOM_AGG_KEYS.has(key)) { + findings.push({ + where: nodeWhere, + message: + `node config has \`${key}\` — the automation engine has no aggregate node, so \`${key}\` is ` + + `silently ignored and this node computes nothing at runtime.`, + hint: + `Aggregation belongs in the data layer: use \`Field.summary\` for a cross-object rollup ` + + `(sum/count of children), or \`Field.formula\` for a per-record computed value. (#1870)`, + rule: FLOW_PHANTOM_AGGREGATION, + }); + } } - } - const strings: string[] = []; - collectTemplateStrings(node.config, undefined, strings); - for (const str of strings) { - if (DOUBLE_BRACE.test(str)) { - findings.push({ - where: nodeWhere, - message: `double-brace interpolation \`${str.trim().slice(0, 80)}\` — flow node values use SINGLE braces.`, - hint: `Use \`{var}\` (e.g. \`{record.title}\`). Double-brace \`{{ }}\` is the formula/template-field dialect, not flow node values. (#1315)`, - rule: FLOW_DOUBLE_BRACE_INTERP, - }); - } - if (BARE_DOLLAR_REF.test(str)) { - findings.push({ - where: nodeWhere, - message: `\`${str.trim().slice(0, 80)}\` looks like a reference written as a literal — a bare \`$ref.field\` is NOT interpolated.`, - hint: `Wrap it and bind a variable: \`{source.id}\` (or \`{$User.Id}\` for the current user). (#1315)`, - rule: FLOW_BARE_DOLLAR_REF, - }); + // Region-STRIPPED: this scan is recursive and a container's config + // physically contains every descendant's, which the walk above already + // visits in its own right. Without the strip a `{{ }}` in a loop body + // would be reported twice — once here against the `loop`, once against the + // node that carries it. With it, the count stays 1 and the finding lands + // on the right node (before #5383 it landed only on the container). + const strings: string[] = []; + collectTemplateStrings(stripRegions(node.config), undefined, strings); + for (const str of strings) { + if (DOUBLE_BRACE.test(str)) { + findings.push({ + where: nodeWhere, + message: `double-brace interpolation \`${str.trim().slice(0, 80)}\` — flow node values use SINGLE braces.`, + hint: `Use \`{var}\` (e.g. \`{record.title}\`). Double-brace \`{{ }}\` is the formula/template-field dialect, not flow node values. (#1315)`, + rule: FLOW_DOUBLE_BRACE_INTERP, + }); + } + if (BARE_DOLLAR_REF.test(str)) { + findings.push({ + where: nodeWhere, + message: `\`${str.trim().slice(0, 80)}\` looks like a reference written as a literal — a bare \`$ref.field\` is NOT interpolated.`, + hint: `Wrap it and bind a variable: \`{source.id}\` (or \`{$User.Id}\` for the current user). (#1315)`, + rule: FLOW_BARE_DOLLAR_REF, + }); + } } } - } - // (c) ADR-0044 — approval send-back-for-revision loop footguns. - scanApprovalReviseLoops(flowName, nodes, edges, findings); + // (c) ADR-0044 — approval send-back-for-revision loop footguns. + scanApprovalReviseLoops(at, graphNodes, graphEdges, findings); - // (d) #3863 — an edge labelled like an error path but typed 'default' is an - // unconditional out-edge: the handler runs on every SUCCESS, in parallel - // with the real path, and never on a failure. - scanErrorLabelledEdges(flowName, nodes, edges, findings); + // (d) #3863 — an edge labelled like an error path but typed 'default' is an + // unconditional out-edge: the handler runs on every SUCCESS, in parallel + // with the real path, and never on a failure. + scanErrorLabelledEdges(at, graphNodes, graphEdges, findings); - // (e) #4414 — a decision that declares a branch it cannot route: an - // unclaimable branch label, an unconditional sibling that runs anyway, - // or a self-contradictory / duplicated `isDefault` marker. - scanBranchRouting(flowName, nodes, edges, findings); + // (e) #4414 — a decision that declares a branch it cannot route: an + // unclaimable branch label, an unconditional sibling that runs anyway, + // or a self-contradictory / duplicated `isDefault` marker. + scanBranchRouting(at, graphNodes, graphEdges, findings); + } } return findings; }