diff --git a/.changeset/lint-flow-node-list-recordsof.md b/.changeset/lint-flow-node-list-recordsof.md new file mode 100644 index 0000000000..0d7b2067f1 --- /dev/null +++ b/.changeset/lint-flow-node-list-recordsof.md @@ -0,0 +1,20 @@ +--- +"@objectstack/lint": patch +--- + +Flow-node-list readers no longer throw on a non-record member — `lintFlowPatterns`, `collectFlowVariableNames` and the three record-change template-path readers now coerce through `recordsOf`. + +`lintFlowPatterns` crashed on an ordinary flow. A YAML `nodes:` list item left empty deserialises to `null`, and the rule read `nodes.find(n => n.type === 'start')` off a list it had only `Array.isArray`-checked, so an author's own metadata turned `objectstack validate` into an uncaught `TypeError` out of a function contractually typed `(stack) => Finding[]`: + +``` +TypeError: Cannot read properties of null (reading 'type') + at lint-flow-patterns.ts:1430 +``` + +`Array.isArray` proves the LIST, never its MEMBERS — the same sentence removed from `validate-expressions.ts` one file over. All seven readers now go through `recordsOf` (`object-graph.ts`), which stays the single home for this coercion; no new copy of the predicate is declared. + +- **`lintFlowPatterns`** — `flow.nodes` is coerced once, and that coerced array is what is handed on to `collectFlowGraphs`. That second half is the load-bearing one: `collectFlowGraphs` is transparent about members (it forwards the caller's array and re-exposes the same objects), so coercing only for the local read would have moved the crash into `packages/spec` rather than removing it. Its two `graph.nodes` readers are coerced as well, because a nested region's node list reaches them with only an `Array.isArray` behind it. +- **`collectFlowVariableNames`** — the `graph.nodes` walk had no member guard while the `flow.variables` walk seven lines above it did. Reachable today only at a region nest of exactly `MAX_REGION_DEPTH`; it now cannot throw at any depth. +- **The three record-change template-path readers** (`boundObjectOf`, `declaredExpandOf` and the per-flow start lookup in `validateFlowTemplatePaths`) were **not** throwing. They survived on an optional chain in the `.find` predicate — one character's difference from the reader that did throw, maintained by nothing and looking redundant next to the `Array.isArray` above it. They are coerced for the same reason and the optional chain goes with it. This half is a hardening, not a bug fix. + +A malformed member is dropped, in silence, exactly as `recordsOf` drops one everywhere else; the valid nodes standing beside it are still judged and the findings a flow draws are unchanged. diff --git a/packages/lint/src/flow-variable-scope.ts b/packages/lint/src/flow-variable-scope.ts index 2202963845..948eaee28f 100644 --- a/packages/lint/src/flow-variable-scope.ts +++ b/packages/lint/src/flow-variable-scope.ts @@ -69,6 +69,8 @@ import { firstUndeclaredReference } from '@objectstack/formula'; +import { recordsOf } from './object-graph.js'; + type AnyRec = Record; /** The node shape this module reads: `collectFlowGraphs`' element type, loosened. */ @@ -219,8 +221,13 @@ export function collectFlowVariableNames( } for (const graph of graphs) { - for (const item of graph.nodes) { - const flowNode = item as AnyRec; + // `recordsOf`, not a bare cast (#16751). Row 1 above guards each + // `flow.variables` member with `if (!item || typeof item !== 'object')` + // seven lines up; this loop did not, so a non-record region node made + // `flowNode.id` throw out of a collector that is contractually total. + // `collectFlowGraphs` only `Array.isArray`-checks a nested region's list, + // so the members reaching here carry no promise from the producer either. + for (const flowNode of recordsOf(graph.nodes)) { // Row 8 — the node id itself. if (typeof flowNode.id === 'string' && flowNode.id) names.add(flowNode.id); const rawConfig = flowNode.config; diff --git a/packages/lint/src/lint-flow-patterns.ts b/packages/lint/src/lint-flow-patterns.ts index f73cc7ef94..81e48eadb1 100644 --- a/packages/lint/src/lint-flow-patterns.ts +++ b/packages/lint/src/lint-flow-patterns.ts @@ -447,13 +447,19 @@ function findDataNodeAnywhere( edges: AnyRec[], ): { readonly node: AnyRec; readonly scope: string } | null { // A cast, not a parse — same contract as the main per-graph walk below: the - // walk touches only `type` / `config`, and the guarded arrays are passed so a - // malformed region cannot make this throw (this module never throws). + // walk touches only `type` / `config`, and the arrays handed in are the ones + // the caller already coerced through `recordsOf`, so a malformed member + // cannot make this throw (this module never throws). for (const graph of collectFlowGraphs({ nodes: nodes as unknown as FlowNodeParsed[], edges: edges as unknown as FlowEdgeParsed[], })) { - for (const node of graph.nodes as unknown as AnyRec[]) { + // `recordsOf`, not `as unknown as AnyRec[]` (#16751). The top-level list is + // clean by the caller's coercion, but a NESTED region's node list is only + // `Array.isArray`-checked by `collectFlowGraphs` before it becomes a graph + // — it carries the producer's word about its members, not a check. Same + // decision made once more where that guarantee stops, through the one home. + for (const node of recordsOf(graph.nodes)) { if (DATA_NODE_TYPES.has(typeof node.type === 'string' ? (node.type as string) : '')) { return { node, scope: graph.scope }; } @@ -1423,7 +1429,16 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] { const findings: FlowLintFinding[] = []; 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. A YAML `nodes:` item + // left empty deserialises to `null`, and `nodes.find(n => n.type === …)` + // four lines down dereferenced it (#16751). Read through `recordsOf` — the + // one home for this coercion (`object-graph.ts`) — and note that THIS array + // is also what goes to `collectFlowGraphs` below, never `flow.nodes` raw: + // that producer is transparent about members (it forwards the caller's + // 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. + const nodes = recordsOf(flow.nodes); const edges = Array.isArray(flow.edges) ? (flow.edges as AnyRec[]) : []; // (a) #1874 — date-equality time condition on a record-change start node. @@ -1519,7 +1534,11 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] { edges: edges as unknown as FlowEdgeParsed[], })) { const at = graph.scope ? `flow '${flowName}' · ${graph.scope}` : `flow '${flowName}'`; - const graphNodes = graph.nodes as unknown as AnyRec[]; + // `recordsOf`, not `as unknown as AnyRec[]` (#16751) — the same reason as + // in `findDataNodeAnywhere`: the top-level graph is clean by the coercion + // 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[]; // (b) #1315 — wrong interpolation syntax in any node's template values. Flow diff --git a/packages/lint/src/non-record-object-entry.test.ts b/packages/lint/src/non-record-object-entry.test.ts index 3bc5389388..6b4dc4d8b7 100644 --- a/packages/lint/src/non-record-object-entry.test.ts +++ b/packages/lint/src/non-record-object-entry.test.ts @@ -401,7 +401,7 @@ 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. * - * Four rows have come out since it was written, each because the sweep went + * Five 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: * @@ -416,8 +416,7 @@ const SWEPT_COLLECTIONS: readonly SweptCollection[] = [ * - `flows[].nodes` / `validateStackExpressions` — the two casts #15793 * repaired, and the reason the two graph-shaped arms below exist at all. * - `flows[].nodes[].config.body.nodes` / `validateStackExpressions` + - * `lintFlowPatterns` (#16752) — neither rule's own reader was ever at fault - * here, and neither was repaired: the throw was `collectFlowGraphs`' + * `lintFlowPatterns` (#16752) — the throw was `collectFlowGraphs`' * (`packages/spec`), which handed out a `FlowGraph` whose `nodes` held the * junk member it had picked up out of a container's open `z.record` config. * Both rules stopped throwing the moment the PRODUCER stopped handing it @@ -427,27 +426,44 @@ const SWEPT_COLLECTIONS: readonly SweptCollection[] = [ * both rows survived it — a guard against reading junk is not a guard * against passing it on. * - * ## The row it holds today + * ⭐ These two arms are now green for TWO independent reasons, and both were + * measured. #16751 landed alongside and re-pointed the two CONSUMER readers + * this shape reached — `lint-flow-patterns.ts`' own `graph.nodes` reader and + * `collectFlowVariableNames`' unguarded `graph.nodes` walk — and on the tree + * before either fix those were the frames the throws carried, one in each + * rule, neither inside `packages/spec`. So "neither rule's own reader was at + * fault" is the wrong reading of this pair in one direction only: the + * producer's declared `FlowNodeParsed[]` really did lie about what it + * returned, AND the consumers really did dereference without a guard. + * Neither repair makes the other unnecessary — remove the producer fix and + * the junk is handed out again to every other consumer; remove the consumer + * coercion and these two readers are back to trusting a declared element + * type. Read the pair as belt and braces, ⛔ not as one change that turned + * out to be redundant. + * - `flows[].nodes` / `lintFlowPatterns` (#16751) — the SAME two spellings one + * file over. `lint-flow-patterns.ts` inline-cast `flow.nodes` and then read + * `.type` off each member, and double-cast `graph.nodes` at two further + * readers; `flow-variable-scope.ts` walked `graph.nodes` with no member + * guard while guarding `flow.variables` seven lines up. All re-pointed at + * `recordsOf`, with the COERCED array — never `flow.nodes` raw — handed on + * to `collectFlowGraphs`, measured: hand the raw one on and the crash + * RELOCATES to the graph-node reader instead of going away. This is the + * shallow half, and the producer repair above never covered it: `flow.nodes` + * is a list the RULE reads itself, before any producer sees it. + * + * ## It holds no row today * * 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. The graph-shaped pair is gone; the shallow one is not. - * - * - `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. The two - * `graph.nodes` casts are covered from the producer side since #16752, so - * what is left to repair here is the `flow.nodes` read the rule does itself. + * addressing mode away. The graph-shaped pair went out when the producer + * stopped handing a junk member out, the shallow one when the last flow-node + * list reader stopped trusting `Array.isArray` about members, and the table is + * empty again. Empty is this ratchet's resting state, not its retirement: it + * stays exact in both directions, so a rule that starts throwing on any swept + * collection reds here because it is not listed. */ -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'], -}; +const RESIDUAL_THROWS: Readonly> = {}; /** * Where a junk member still draws a finding no author's file justifies — the diff --git a/packages/lint/src/validate-flow-template-paths.ts b/packages/lint/src/validate-flow-template-paths.ts index e1a9575766..9540d7638b 100644 --- a/packages/lint/src/validate-flow-template-paths.ts +++ b/packages/lint/src/validate-flow-template-paths.ts @@ -253,8 +253,14 @@ function isRecordTriggered(flow: AnyRec, startConfig: AnyRec): boolean { /** Resolve the object a record-change flow binds to, from its start node. */ function boundObjectOf(flow: AnyRec): string | undefined { - const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; - const start = nodes.find((n) => n?.type === 'start'); + // `recordsOf`, not `Array.isArray` + cast (#16751). This site and the two + // below never threw — but only because each `.find` predicate happens to be + // spelled `n?.type`, one character away from the reader that did throw in + // `lint-flow-patterns.ts`. Nothing maintained that difference, and the `?.` + // reads as redundant beside an `Array.isArray`, so the coercion is made where + // it has a home and the optional chain goes with it. + const nodes = recordsOf(flow.nodes); + const start = nodes.find((n) => n.type === 'start'); if (!start) return undefined; const config = (start.config ?? {}) as AnyRec; const typed = (start.start ?? {}) as AnyRec; @@ -271,8 +277,8 @@ function boundObjectOf(flow: AnyRec): string | undefined { * or `string[]`; anything else yields the empty set. */ function declaredExpandOf(flow: AnyRec): Set { - const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; - const start = nodes.find((n) => n?.type === 'start'); + const nodes = recordsOf(flow.nodes); + const start = nodes.find((n) => n.type === 'start'); const raw = ((start?.config ?? {}) as AnyRec).expand; if (typeof raw === 'string') return new Set(raw ? [raw] : []); if (Array.isArray(raw)) return new Set(raw.filter((r): r is string => typeof r === 'string' && r.length > 0)); @@ -295,8 +301,8 @@ export function validateFlowTemplatePaths(stack: AnyRec): FlowTemplatePathFindin flows.forEach((flow, flowIndex) => { const flowName = typeof flow.name === 'string' ? flow.name : `#${flowIndex}`; - const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; - const start = (nodes.find((n) => n?.type === 'start')?.config ?? {}) as AnyRec; + const nodes = recordsOf(flow.nodes); + const start = (nodes.find((n) => n.type === 'start')?.config ?? {}) as AnyRec; if (!isRecordTriggered(flow, start)) return; const objectName = boundObjectOf(flow);