fix(lint): descend into loop bodies and every nested region in the flow rule family (#5383) (#5635)

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.

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

.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.

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)) {

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'");
+  });
+});