From 97d8ffc55e3e3232c7cd084824a6f5efe2d8a58c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 21:02:50 +0000 Subject: [PATCH] fix(lint): descend into loop bodies and every nested region in the flow rule family (#5383) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, 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 none were reported. The identical key on a top-level decision fired immediately — same key, same node type, only the nesting depth differed. `lintFlowPatterns` now iterates `collectFlowGraphs` — the same traversal the engine's registration pass uses, and the one `validate-expressions.ts` already uses on the author side — and prefixes each finding's `where` with the region scope, so a message still points at exactly one node. Findings on a flow's own graph are unchanged byte for byte, since the top-level graph's scope is empty. Two properties of the walk are load-bearing rather than incidental: - Nodes and edges stay PAIRED per region. The branch-routing rules reason about a node together with its out-edges, and a region is a self-contained sub-graph. Flattening into one node bag plus one edge bag would break them both ways: a nested decision's out-edges would be absent from the top-level list so it would read as having none and be skipped, 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. A regression test pins the second case. - A container's config is read region-STRIPPED for the recursive scans. `collectTemplateStrings` walks config to its string leaves and a container's config physically contains its descendants', so a nested double-brace hit was already visible before this change — but attributed to the enclosing `loop`, the same failure mode #4380 fixed for `validate-flow-template-paths`. Descending without stripping would have made it a double report. It now names the node carrying the string, still exactly once. `stripRegions` is exported from `flow-walk.ts` rather than copied, so there is one definition of that view. `flow-runas-unscoped` deliberately keeps its top-level-only data-node search: widening a build-gating rule is its own change with its own blast radius, filed as #5633. Verified the repo's own example apps (`app-showcase` / `app-crm` / `app-todo`, 34 flows) report zero findings before and after, and that the descent does reach their real loop bodies — an inert condition injected into showcase's `loop_tasks` body is caught and scoped to it. --- .changeset/flow-lint-loop-body-descent.md | 52 ++++ packages/lint/src/flow-walk.ts | 14 +- packages/lint/src/lint-flow-patterns.test.ts | 253 +++++++++++++++++++ packages/lint/src/lint-flow-patterns.ts | 226 ++++++++++++----- 4 files changed, 473 insertions(+), 72 deletions(-) create mode 100644 .changeset/flow-lint-loop-body-descent.md 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; }