diff --git a/.changeset/flow-node-list-non-record.md b/.changeset/flow-node-list-non-record.md new file mode 100644 index 0000000000..046f9e1402 --- /dev/null +++ b/.changeset/flow-node-list-non-record.md @@ -0,0 +1,14 @@ +--- +"@objectstack/lint": patch +--- + +`validateStackExpressions` no longer throws on a non-record entry of a flow's `nodes` list. + +An empty item in a YAML `nodes:` list deserialises to `null`, so this is an authorable shape — the same one #15552, #15636 and #15742 closed for stack collections and for `objects[].fields`. Here it crashed the linter instead of producing a finding: `flow.nodes: [null, ...]` threw `Cannot read properties of null (reading 'type')`, which presents to an author as a broken tool rather than as a problem with their metadata. + +Both of the file's inline casts now read through `recordsOf`, the one home of this coercion, instead of asserting that `Array.isArray` proves anything about a list's MEMBERS: + +- The flow walk reads `flow.nodes` through `recordsOf`, and — the half that actually removes the crash — hands that coerced array to `collectFlowGraphs` rather than the raw flow. `collectFlowGraphs` declares its input as already-parsed `FlowNodeParsed[]` and is transparent about members, so passing raw authored metadata was calling it out of contract; coercing only the local variable relocated the throw into `@objectstack/spec` instead of ending it. The producer's contract is unchanged, deliberately: widening it to tolerate malformed members is the wrong direction. +- The per-graph walk reads `graph.nodes` through `recordsOf` in place of an `as unknown as` double cast. A nested region's node list is only `Array.isArray`-checked before it becomes a graph, so that list carries the producer's word about its members and not a check. + +A non-record member is dropped whole and in silence, exactly as the file's sibling field readers already did; a flow standing beside the junk entry is still judged, and a `nodes` list holding a plain string still reports exactly what it reported before. diff --git a/packages/lint/src/non-record-object-entry.test.ts b/packages/lint/src/non-record-object-entry.test.ts index 70c7a5dfd9..f6e9edc766 100644 --- a/packages/lint/src/non-record-object-entry.test.ts +++ b/packages/lint/src/non-record-object-entry.test.ts @@ -293,6 +293,60 @@ const underObject = (key: string, valid?: AnyRec): SweptCollection => ({ valid, }); +/** + * A judgeable flow node: the `start` node the flow readers resolve the + * record-change target against, so a control built from it is not merely empty. + */ +const VALID_NODE: AnyRec = { id: 'start', type: 'start', config: { objectName: 'crm_account' } }; + +/** + * A flow's own node list — `stack.flows[].nodes` (#15793). + * + * ## Why this needed a THIRD addressing mode rather than one more row + * + * The two above address a collection by NAME: a top-level stack key, or one + * sub-collection key on an object. A flow's node list is neither. It is the + * `nodes` of a member of `stack.flows`, and #15793's whole diagnosis was that + * this sweep *could not express* it — which is why #15552, #15636 and #15742 + * closed this defect class three times without ever reaching the two casts + * #15793 repaired. The blind spot was in the ADDRESSING, not in the rule table. + * + * The constructor is a three-line sibling of `underObject` because + * `SweptCollection.stack` was already an arbitrary builder; what was missing + * was only the will to write a second shape of one. That is worth saying + * plainly: the gap looked structural and was not. + */ +const underFlow = (key: string, valid?: AnyRec): SweptCollection => ({ + label: `flows[].${key}`, + stack: (members) => ({ objects: [VALID_OBJECT], flows: [{ name: 'crm_flow', edges: [], [key]: members }] }), + valid, +}); + +/** + * A NESTED region's node list — `flows[].nodes[].config.body.nodes` (#15793). + * + * The graph-shaped half proper, and a different reachability question from + * `underFlow`. An ADR-0031 container keeps a whole sub-graph in its `config`, + * and `collectFlowGraphs` turns each into its own `FlowGraph` after checking + * only `Array.isArray` on the inner list — so a non-record member here is one + * the PRODUCER picked up, not one a caller passed in, and no coercion at the + * call site can reach it. Kept in the sweep with its throw recorded below + * rather than left unexpressed: an unaddressable shape is exactly what let this + * class survive three closures. + */ +const underNestedRegion = (valid?: AnyRec): SweptCollection => ({ + label: 'flows[].nodes[].config.body.nodes', + stack: (members) => ({ + objects: [VALID_OBJECT], + flows: [{ + name: 'crm_flow', + edges: [], + nodes: [VALID_NODE, { id: 'lp', type: 'loop', config: { collection: 'x', body: { nodes: members, edges: [] } } }], + }], + }), + valid, +}); + const SWEPT_COLLECTIONS: readonly SweptCollection[] = [ // Top-level, read through `recordsOf(stack.X)` by the re-pointed readers. topLevel('objects', VALID_OBJECT), @@ -320,6 +374,11 @@ const SWEPT_COLLECTIONS: readonly SweptCollection[] = [ topLevel('mappings'), // Top-level, read by `validateSecurityPosture` through `recordsOf`. topLevel('positions'), + // A flow's inner graph — the shape no addressing mode could reach until + // #15793 added one. `underFlow` is the flow's own list; `underNestedRegion` + // is a container's sub-graph, which only the producer can hand out. + underFlow('nodes', VALID_NODE), + underNestedRegion(VALID_NODE), // Per-object sub-collections the same readers walk. underObject('fields', VALID_FIELD), underObject('actions'), @@ -340,11 +399,9 @@ const SWEPT_COLLECTIONS: readonly SweptCollection[] = [ * "nothing throws" would have had to be deleted or weakened on the day it was * written, and would then never have caught the next one. * - * It is EMPTY today, and that is a measurement, not an aspiration: no rule in - * the table throws on a non-record member of any collection swept here. Two - * rows have come out since it was written, each because the sweep went red - * demanding a throw that no longer happens — which is the both-directions half - * earning its keep, since neither removal started with anyone going looking: + * Three rows have come out since it was written, each because the sweep went + * red demanding a throw that no longer happens — which is the both-directions + * half earning its keep, since no removal started with anyone going looking: * * - `stack.datasets` — `indexDatasets` in `validate-chart-bindings.ts`, * re-pointed by #15741. @@ -354,8 +411,39 @@ const SWEPT_COLLECTIONS: readonly SweptCollection[] = [ * the list through `recordsOf` (#15742), which drops a non-record member of * the array shape whole and in silence, exactly as the file's two sibling * field readers already did. + * - `flows[].nodes` / `validateStackExpressions` — the two casts #15793 + * repaired, and the reason the two graph-shaped arms below exist at all. + * + * ## The rows it holds today, both found by the arms that added them + * + * It went from empty to two the moment a flow's inner node list became + * addressable, which is the point #15793 was filed to make: this class was + * closed three times over collections while the same defect stood untouched one + * addressing mode away. + * + * - `flows[].nodes` / `lintFlowPatterns` (#16751) — `lint-flow-patterns.ts` + * holds the SAME two spellings #15793 removed from `validate-expressions.ts` + * (`:1426` inline-casts `flow.nodes`, then `:1430` reads `.type` off each + * member; `:456` and `:1522` double-cast `graph.nodes`). Shallowly + * reachable — an ordinary flow with an empty YAML list item. + * - `flows[].nodes[].config.body.nodes` / `validateStackExpressions` + + * `lintFlowPatterns` (#16752) — neither rule's own reader is at fault here: + * both throw from INSIDE `collectFlowGraphs`, whose region walk reads + * `node.config` off a member of an inner list it checked only with + * `Array.isArray`. No coercion at either call site reaches that list, which + * is why #15793 stopped and filed the fork instead of widening a + * `packages/spec` contract to tolerate malformed members. */ -const RESIDUAL_THROWS: Readonly> = {}; +const RESIDUAL_THROWS: Readonly> = { + // 2026-09-08 — #16751. Removed when `lint-flow-patterns.ts` reads its node + // lists through `recordsOf`, as `validate-expressions.ts` now does. + 'flows[].nodes · null': ['lintFlowPatterns'], + 'flows[].nodes · undefined': ['lintFlowPatterns'], + // 2026-09-08 — #16752. Both entries are ONE defect in `collectFlowGraphs`, + // surfacing through the two rules that call it. Removed together. + 'flows[].nodes[].config.body.nodes · null': ['lintFlowPatterns', 'validateStackExpressions'], + 'flows[].nodes[].config.body.nodes · undefined': ['lintFlowPatterns', 'validateStackExpressions'], +}; /** * Where a junk member still draws a finding no author's file justifies — the diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 1d33f5a995..153d7a6524 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -1124,7 +1124,11 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // ── Flows ────────────────────────────────────────────────────────── for (const flow of recordsOf(stack.flows)) { const flowName = typeof flow.name === 'string' ? flow.name : '(unnamed flow)'; - const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; + // `Array.isArray` proves the LIST, never its MEMBERS — the sentence #15742 + // removed from one reader below in this same file. A YAML `nodes:` item + // left empty deserialises to `null`, and `nodes.find(n => n.type === …)` on + // the very next line dereferenced it (#15793). + const nodes = recordsOf(flow.nodes); // The record-change target object — `record.*` refs resolve against it. const startNode = nodes.find(n => n.type === 'start'); const startCfg = (startNode?.config ?? {}) as AnyRec; @@ -1137,7 +1141,16 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // `objectstack validate` and shipped. This is the author-time half of the // same traversal the engine's registration pass now does; `scope` names the // region so the located message still points at one edge. - const graphs = collectFlowGraphs(flow as { nodes?: FlowNodeParsed[] }); + // + // Handed the COERCED `nodes`, never `flow` raw (#15793). `collectFlowGraphs` + // declares its input as `FlowNodeParsed[]` — already-parsed nodes — and is + // transparent about members: it forwards the caller's array and re-exposes + // that same object. So it neither admits nor rejects a non-record member; + // passing raw authored metadata is calling it OUT OF CONTRACT, and it then + // dereferences `node.config` in its own region walk. Coercing only the + // local `nodes` above does not fix the crash, it relocates it into + // `packages/spec` — measured. Contract-first the caller is what changes. + const graphs = collectFlowGraphs({ ...flow, nodes } as { nodes?: FlowNodeParsed[] }); // [#14089] The flattened-scope shadowing pass needs the flow's COMPLETE // variable set before any condition is judged, so it is a separate walk over @@ -1202,7 +1215,13 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { for (const graph of graphs) { const at = graph.scope ? `flow '${flowName}' · ${graph.scope}` : `flow '${flowName}'`; - for (const node of graph.nodes as unknown as AnyRec[]) { + // `recordsOf`, not `as unknown as AnyRec[]` (#15793). A NESTED region's + // node list is only `Array.isArray`-checked by `collectFlowGraphs` before + // it becomes a graph, so this list carries the producer's word about its + // members and not a check. The top-level graph is clean by the coercion + // at the call site above; this is the same decision made once more where + // that guarantee stops, through the file's one home for it. + for (const node of recordsOf(graph.nodes)) { const cfg = (node.config ?? {}) as AnyRec; const nodeCondWhere = `${at} · node '${node.id}' (${node.type}) condition`; if (!checkStructuralCondition(nodeCondWhere, cfg.condition).refused) {