fix(lint): a flow's edges list drops a non-record member instead of throwing

.changeset/16910-flow-edges-recordsof.md

@@ -0,0 +1,21 @@
+---
+"@objectstack/lint": patch
+---
+
+A flow's `edges` list no longer takes the whole authoring gate down when one of its members is not a record — the sibling list #16751's repair did not reach (#16910).
+
+`lintFlowPatterns` read `.label` off each member of `flow.edges` behind nothing but an `Array.isArray` check, which proves the LIST and never its MEMBERS. A YAML `edges:` list item left empty deserialises to `null`, so hand-written metadata turned `objectstack validate` into an uncaught `TypeError` out of a function contractually typed `(stack) => FlowLintFinding[]`:
+
+```
+edges:[null, valid]        threw=YES  TypeError: Cannot read properties of null (reading 'label')
+edges:[undefined, valid]   threw=YES  TypeError: Cannot read properties of undefined (reading 'label')
+```
+
+A linter that throws instead of reporting fails hardest on exactly the documents it is most needed for, and the author gets a stack trace where a diagnostic belongs.
+
+- **The junk member is DROPPED, silently**, through `recordsOf` — the same coercion, from the same one home (`object-graph.ts`), that #16751 chose for the seven flow-NODE-list readers, so two sibling lists on one flow member cannot disagree about what a malformed member means.
+- ⭐ **The valid edge beside it is still JUDGED.** "No longer throws" is half a contract: a guard that abandoned the list would satisfy it and would have traded the crash for silence. Measured against a control holding the same flow without the junk member, the surviving finding is identical in rule and location, and no finding is invented about an entry no author wrote.
+- **Two rules, not one.** `os validate` runs the rule TABLE, so one throwing reader takes every other rule's verdict down with it: once `lintFlowPatterns` stopped throwing, the identical defect surfaced one file over in `validateStackExpressions`, which read the same list through the same double cast. Both are repaired here; repairing only the filed one would have left the gate down on the same document.
+- ⭐ **Which reader actually carried the crash, measured by ablation** — both edge walks read `graph.edges`, not the flow's own list, because `collectFlowGraphs` re-exposes whatever array it is handed. Reverting `graph.edges` alone in either file reds the new cases (10 failures in `lintFlowPatterns`, 6 in `validateStackExpressions`); reverting either `flow.edges` coercion alone leaves them green. The two `flow.edges` coercions are therefore **defence in depth, not the load-bearing fix**, and are kept deliberately: they hand the COERCED array to `collectFlowGraphs` rather than the raw one, which is the discipline the node lists already follow, and they keep two sibling lists on one flow member reading the same way. ⛔ Read them as belt and braces, not as one repair written twice.
+- **The producer's edge side is still member-blind.** `collectFlowGraphs` filters the nodes it hands out and forwards edges untouched, so `FlowGraph.edges` is declared `FlowEdgeParsed[]` and can contain a non-record. It does not dereference them today, which is why the consumer coercion is sufficient; that asymmetry is filed separately rather than widened here.
+- **No new finding id and no new diagnostic.** On every well-formed document the output is byte-identical; the only behaviour that changes is on input that previously crashed.

packages/lint/src/lint-flow-patterns.test.ts

@@ -2484,3 +2484,124 @@ describe('#16405 — an `http` node payload is not a region, and both #1315 rule
     expect(top).toHaveLength(3);
   });
 });
+
+/**
+ * #16910 — a non-record member of a flow's `edges` list must not throw, and the
+ * VALID edge beside it must still be judged.
+ *
+ * ## Why both halves are asserted, and why the second one is the real one
+ *
+ * A linter that throws instead of reporting takes the whole gate down on
+ * malformed input — and malformed input is exactly what a linter exists to
+ * catch, so the tool failed hardest on the documents it is most needed for and
+ * the author got a stack trace where a diagnostic belongs. That is the defect.
+ * But "no longer throws" is half a contract: a guard that abandoned the whole
+ * edge list would satisfy it and would have traded the crash for silence, which
+ * is strictly worse than the crash because nothing reports it. So every case
+ * below pins the surviving finding by IDENTITY — rule id and `where` — against
+ * a control holding the same flow without the junk member, never by "did not
+ * throw" alone.
+ *
+ * ## The semantics, and why it matches #16751
+ *
+ * The junk member is DROPPED, silently, by `recordsOf`'s `isRec` filter — the
+ * same coercion and the same one home (`object-graph.ts`) #16751 chose for the
+ * seven flow-NODE-list readers, so the two sibling lists on the same flow
+ * member cannot disagree about what a malformed member means. Dropping is not
+ * reporting: a rule that INVENTED a finding about an entry no author wrote is
+ * the phantom half of this same defect class, which is why the control
+ * comparison below is an equality and not a `toBeGreaterThan`.
+ *
+ * ## Both addressing depths
+ *
+ * The flow's own list and a nested region's list are separate readers reached by
+ * separate routes — the second arrives through `collectFlowGraphs`, out of a
+ * container's open `z.record` config, behind nothing but `Array.isArray`, so no
+ * coercion at the call site can reach it. The top-level repair alone left it
+ * throwing; measured, not assumed.
+ */
+describe('a non-record member of a flow `edges` list (#16910)', () => {
+  /** Labelled like an error path, left at the default type: the #3863 shape. */
+  const VALID_EDGE = { source: 'act', target: 'done', label: 'error' };
+  const ENDPOINTS = [
+    { id: 'start', type: 'start', config: {} },
+    { id: 'act', type: 'create_record', config: {} },
+    { id: 'done', type: 'end', config: {} },
+  ];
+
+  /** The four shapes a raw `edges` list can hold that are not a record. */
+  const JUNK: readonly (readonly [string, unknown])[] = [
+    ['null', null],
+    ['undefined', undefined],
+    ['a string', 'x'],
+    ['a number', 42],
+  ];
+
+  const topLevel = (edges: readonly unknown[]) => ({
+    objects: [{ name: 'crm_account', fields: [{ name: 'name', type: 'text' }] }],
+    flows: [{ name: 'crm_flow', nodes: ENDPOINTS, edges }],
+  });
+
+  const nestedRegion = (edges: readonly unknown[]) => ({
+    objects: [{ name: 'crm_account', fields: [{ name: 'name', type: 'text' }] }],
+    flows: [{
+      name: 'crm_flow',
+      nodes: [
+        { id: 'start', type: 'start', config: {} },
+        { id: 'lp', type: 'loop', config: { collection: 'x', body: { nodes: ENDPOINTS, edges } } },
+      ],
+      edges: [],
+    }],
+  });
+
+  const errorLabelFindings = (stack: unknown) =>
+    lintFlowPatterns(stack as Record<string, unknown>)
+      .filter((f) => f.rule === FLOW_ERROR_LABEL_NOT_FAULT);
+
+  describe.each([
+    ["the flow's own list", topLevel, "flow 'crm_flow' · edge 'act' → 'done'"],
+    ["a nested region's list", nestedRegion, "flow 'crm_flow' · loop 'lp' body · edge 'act' → 'done'"],
+  ])('%s', (_label, build, expectedWhere) => {
+    it('judges the valid edge with no junk member beside it (the control)', () => {
+      const fnds = errorLabelFindings(build([VALID_EDGE]));
+      expect(fnds).toHaveLength(1);
+      expect(fnds[0].where).toBe(expectedWhere);
+    });
+
+    it.each(JUNK)('drops %s and still judges the valid edge beside it', (_shape, junk) => {
+      // Half one: the rule returns rather than throwing. `lintFlowPatterns` is
+      // typed `(stack) => FlowLintFinding[]` and its docblock promises it never
+      // throws; before this repair a `null` here broke that promise outright.
+      expect(() => lintFlowPatterns(build([junk, VALID_EDGE]) as Record<string, unknown>)).not.toThrow();
+
+      // Half two — the one that matters: the surviving edge is still JUDGED,
+      // by identity against the control, so a guard that abandoned the list
+      // reds here instead of passing half the assertion.
+      const fnds = errorLabelFindings(build([junk, VALID_EDGE]));
+      expect(fnds).toHaveLength(1);
+      expect(fnds[0].where).toBe(expectedWhere);
+    });
+
+    it.each(JUNK)('invents no finding about %s, which no author wrote', (_shape, junk) => {
+      // The phantom half of the same defect class: dropping a member must be
+      // invisible to the rule table in BOTH directions, so this is an equality
+      // against the control and not a lower bound.
+      expect(lintFlowPatterns(build([junk, VALID_EDGE]) as Record<string, unknown>))
+        .toEqual(lintFlowPatterns(build([VALID_EDGE]) as Record<string, unknown>));
+    });
+  });
+
+  /**
+   * The whole gate, not one rule: `os validate` runs the rule TABLE, so a single
+   * throwing reader takes every other rule's verdict down with it. Measured on
+   * this input, `validateStackExpressions` threw from its own `graph.edges`
+   * reader after `lintFlowPatterns` stopped — the same defect one file over,
+   * repaired in the same change for the same reason.
+   */
+  it.each(JUNK)('takes no rule in the authoring table down with %s', (_shape, junk) => {
+    for (const rule of AUTHORING_RULES) {
+      expect(() => rule.run(topLevel([junk, VALID_EDGE]) as Record<string, unknown>, {}), rule.name).not.toThrow();
+      expect(() => rule.run(nestedRegion([junk, VALID_EDGE]) as Record<string, unknown>, {}), rule.name).not.toThrow();
+    }
+  });
+});

packages/lint/src/lint-flow-patterns.ts

@@ -1438,8 +1438,17 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] {
     // array and re-exposes the same objects), so coercing only for the local
     // read relocates the crash into `packages/spec` instead of removing it —
     // measured on #15793, and measured again here.
+    //
+    // `edges` is the SAME reader one list over, and #16751 reached only the
+    // node lists: `scanErrorLabelledEdges` read `.label` off each member and a
+    // `null` edge threw out of a function contractually typed
+    // `(stack) => Finding[]` (#16910). A linter that throws instead of
+    // reporting takes the gate down on exactly the malformed document it
+    // exists to catch, so the author gets a stack trace where a diagnostic
+    // belongs. Same coercion, same home, and the same onward-handoff
+    // condition: the COERCED array is what goes to `collectFlowGraphs` below.
     const nodes = recordsOf(flow.nodes);
-    const edges = Array.isArray(flow.edges) ? (flow.edges as AnyRec[]) : [];
+    const edges = recordsOf(flow.edges);
 
     // (a) #1874 — date-equality time condition on a record-change start node.
     const start = nodes.find((n) => n.type === 'start');
@@ -1539,7 +1548,16 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] {
       // at the call site above, and a nested region's node list arrives here
       // with only `Array.isArray` behind it.
       const graphNodes = recordsOf(graph.nodes);
-      const graphEdges = graph.edges as unknown as AnyRec[];
+      // `graph.edges` needed the identical repair, and it is NOT covered by the
+      // coercion at the call site above: a nested region's edge list is read
+      // out of a container's open `z.record` config by `collectFlowGraphs`
+      // behind nothing but `Array.isArray` (`control-flow.zod.ts`, where the
+      // node side is member-filtered since #16752 and the edge side is not), so
+      // it reaches here carrying the producer's word about its members rather
+      // than a check. Measured on this tree: a nested `body.edges` holding a
+      // `null` threw from the same `scanErrorLabelledEdges` frame as the
+      // top-level list did (#16910). Belt and braces, ⛔ not one repair twice.
+      const graphEdges = recordsOf(graph.edges);
 
       // (b) #1315 — wrong interpolation syntax in any node's template values. Flow
       //     node values use SINGLE braces; double-brace `{{ }}` and bare `$ref.x`

packages/lint/src/non-record-object-entry.test.ts

@@ -322,6 +322,88 @@ const underFlow = (key: string, valid?: AnyRec): SweptCollection => ({
   valid,
 });
 
+/**
+ * A judgeable flow EDGE: labelled like an error path, left at the default type,
+ * out of a node whose executor does not select by label — the `#3863` shape, so
+ * a control built from it draws exactly one finding
+ * (`flow-error-label-not-fault`) and the population pin below has something to
+ * count on the edge side too.
+ */
+const VALID_EDGE: AnyRec = { source: 'act', target: 'done', label: 'error' };
+
+/** The nodes {@link VALID_EDGE} runs between, so the edge is judged in context. */
+const EDGE_ENDPOINT_NODES: readonly AnyRec[] = [
+  VALID_NODE,
+  { id: 'act', type: 'create_record', config: { objectName: 'crm_account' } },
+  { id: 'done', type: 'end', config: {} },
+];
+
+/**
+ * A flow's own EDGE list — `stack.flows[].edges` (#16910).
+ *
+ * ## Why this is a row of its own and not covered by `underFlow('nodes')`
+ *
+ * {@link underFlow} addresses ONE key of a flow member, and every existing row
+ * built with it addresses `nodes`. `edges` is a second list on the same member,
+ * read by the same function, four lines from the same repair — and #16751
+ * re-pointed the seven NODE-list readers without reaching it. That is precisely
+ * why this sweep is written over a table: the reader was fixed, the sibling
+ * list beside it was not, and nothing in the suite could say so because no arm
+ * addressed it. `scanErrorLabelledEdges` read `.label` off each member, so a
+ * YAML `edges:` item left empty (`null`) threw a `TypeError` out of a function
+ * contractually typed `(stack) => Finding[]`.
+ *
+ * The constructor supplies real endpoint nodes rather than reusing
+ * {@link underFlow}: the point of the population pin here is that the VALID
+ * edge beside the junk one is still JUDGED, and an edge judged against an empty
+ * node table is a weaker control than one judged in context.
+ */
+const underFlowEdges = (valid?: AnyRec): SweptCollection => ({
+  label: 'flows[].edges',
+  stack: (members) => ({
+    objects: [VALID_OBJECT],
+    flows: [{ name: 'crm_flow', nodes: [...EDGE_ENDPOINT_NODES], edges: members }],
+  }),
+  valid,
+});
+
+/**
+ * A NESTED region's EDGE list — `flows[].nodes[].config.body.edges` (#16910).
+ *
+ * The eighth reader, and the one the card that filed `flows[].edges` predicted
+ * would exist ("an eighth is likelier than not"). It is NOT reachable through
+ * {@link underFlowEdges}: this list is read out of a container's open
+ * `z.record` config by `collectFlowGraphs` behind nothing but `Array.isArray`
+ * — the node side of that same producer walk has been member-filtered since
+ * #16752, the edge side has not — so it arrives at `lint-flow-patterns.ts`'
+ * `graph.edges` reader carrying the producer's word about its members rather
+ * than a check, exactly as `graph.nodes` did before #16751. Measured on the
+ * tree that carried the top-level repair alone: a `null` here still threw, from
+ * the identical `scanErrorLabelledEdges` frame.
+ *
+ * ⛔ The consumer coercion at the call site cannot reach it, which is the whole
+ * reason this is a separate arm rather than a second shape of the one above.
+ */
+const underNestedRegionEdges = (valid?: AnyRec): SweptCollection => ({
+  label: 'flows[].nodes[].config.body.edges',
+  stack: (members) => ({
+    objects: [VALID_OBJECT],
+    flows: [{
+      name: 'crm_flow',
+      nodes: [
+        VALID_NODE,
+        {
+          id: 'lp',
+          type: 'loop',
+          config: { collection: 'x', body: { nodes: [...EDGE_ENDPOINT_NODES], edges: members } },
+        },
+      ],
+      edges: [],
+    }],
+  }),
+  valid,
+});
+
 /**
  * A NESTED region's node list — `flows[].nodes[].config.body.nodes` (#15793).
  *
@@ -381,6 +463,10 @@ const SWEPT_COLLECTIONS: readonly SweptCollection[] = [
   // is a container's sub-graph, which only the producer can hand out.
   underFlow('nodes', VALID_NODE),
   underNestedRegion(VALID_NODE),
+  // The EDGE lists beside them (#16910) — the same reader, the sibling list
+  // #16751 did not reach, at both addressing depths.
+  underFlowEdges(VALID_EDGE),
+  underNestedRegionEdges(VALID_EDGE),
   // Per-object sub-collections the same readers walk.
   underObject('fields', VALID_FIELD),
   underObject('actions'),

packages/lint/src/validate-expressions.test.ts

@@ -2793,8 +2793,15 @@ const READ_SURFACES: Array<{ receiver: string; expected: string[]; declaredBy: s
     keys: () => shapeKeysOf(ObjectStackSchema.shape.hooks),
   },
   {
+    // [#16910] `edges` joined `nodes` here when the flow's EDGE list stopped
+    // being read out of the `{ ...flow }` spread and became a literal
+    // `flow.edges` read through `recordsOf`. Adding it is the deliberate visit
+    // this table exists to force, and the read is literal for exactly the
+    // reason the `f.options` note below gives: the spread handed the raw array
+    // to `collectFlowGraphs` while hiding the surface from this scan, which is
+    // how a `null` edge member reached the edge walk and threw.
     receiver: 'flow',
-    expected: ['name', 'nodes'],
+    expected: ['edges', 'name', 'nodes'],
     declaredBy: 'ObjectStackSchema.flows[]',
     keys: () => shapeKeysOf(ObjectStackSchema.shape.flows),
   },