diff --git a/.changeset/collect-flow-graphs-drops-non-record-members.md b/.changeset/collect-flow-graphs-drops-non-record-members.md new file mode 100644 index 0000000000..e990b91667 --- /dev/null +++ b/.changeset/collect-flow-graphs-drops-non-record-members.md @@ -0,0 +1,11 @@ +--- +"@objectstack/spec": patch +--- + +`collectFlowGraphs` now honours the `readonly FlowNodeParsed[]` it declares: a member of a region's node list that is not a record is dropped from the `FlowGraph` it hands out, instead of being passed through verbatim. + +An ADR-0031 container keeps a whole sub-graph inside `FlowNodeSchema.config`, a deliberately open `z.record`, so `collectFlowGraphs` re-derives those inner node lists at run time and checks them with `Array.isArray` — which proves the LIST and never its MEMBERS. An empty item in a YAML `nodes:` list under a `loop` body deserialises to `null`, and that `null` reached `graph.nodes` on every returned graph, at every depth. No caller could prevent it: this is an array the walk picks up itself, so no coercion at a call site ever holds it. Filed as #16752. + +- **What changed.** The walk filters what it hands out and skips what it descends into, through one predicate. Array identity is preserved when nothing is dropped, so a well-formed flow allocates nothing new. +- **What deliberately did NOT change.** The declared input type is untouched — widening it to tolerate malformed members was refused on the anti-AI-error axis, and this is the opposite move: the producer now keeps the promise it already made. The schema refusal that owns a malformed region still fires, unchanged; this walk runs inside `FlowSchema`'s parse, where a thrown `TypeError` would escape `safeParse`, so the repair is a drop and a skip and never a throw. `FlowGraph.path` still indexes the raw authored list, so a Zod issue stays anchored where the author wrote the node. +- **Visible consequence.** As with the sibling repairs that read their lists through a record filter, a dropped member renumbers the ones behind it *within* `graph.nodes` — a difference in the index, never in whether a node was judged, and only in a list that was already malformed. diff --git a/packages/lint/src/non-record-object-entry.test.ts b/packages/lint/src/non-record-object-entry.test.ts index f6e9edc766..3bc5389388 100644 --- a/packages/lint/src/non-record-object-entry.test.ts +++ b/packages/lint/src/non-record-object-entry.test.ts @@ -330,9 +330,11 @@ const underFlow = (key: string, valid?: AnyRec): SweptCollection => ({ * 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. + * call site can reach it. That is what #16752 repaired, at the producer: the + * walk now drops a non-record member from the graph it hands out, and this arm + * carries no `RESIDUAL_THROWS` row. It stays in the sweep as the pin on that + * repair — an unaddressable shape is exactly what let this class survive three + * closures, so the addressing is the part worth keeping. */ const underNestedRegion = (valid?: AnyRec): SweptCollection => ({ label: 'flows[].nodes[].config.body.nodes', @@ -399,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. * - * Three rows have come out since it was written, each because the sweep went + * Four 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: * @@ -413,36 +415,38 @@ const SWEPT_COLLECTIONS: readonly SweptCollection[] = [ * field readers already did. * - `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`' + * (`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 + * out, which is what #15793 predicted when it refused to widen the + * `packages/spec` contract and filed the fork instead. ⛔ Note what did NOT + * fix it: #16134 had already stopped that walk DEREFERENCING the member, and + * both rows survived it — a guard against reading junk is not a guard + * against passing it on. * - * ## The rows it holds today, both found by the arms that added them + * ## The row it holds 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. + * 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. - * - `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. + * 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. */ 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'], }; /** diff --git a/packages/spec/src/automation/control-flow.zod.ts b/packages/spec/src/automation/control-flow.zod.ts index 7a0539c415..91919189b6 100644 --- a/packages/spec/src/automation/control-flow.zod.ts +++ b/packages/spec/src/automation/control-flow.zod.ts @@ -486,7 +486,12 @@ export function findRegionEntry(region: { nodes: FlowNodeParsed[]; edges?: FlowE // ─── Where the containers keep their regions ───────────────────────── -/** A dict — region-shaped enough to reach its `nodes` / `edges`. */ +/** + * A dict — region-shaped enough to reach its `nodes` / `edges`, and the same + * test a member of a node list must pass to be a node at all. One spelling for + * both, so what {@link collectFlowGraphs} walks cannot drift from what it hands + * out. + */ function isRegionDict(v: unknown): v is Record { return typeof v === 'object' && v !== null && !Array.isArray(v); } @@ -699,6 +704,16 @@ export interface FlowGraph { * `[...path, 'nodes', i, 'id']` — rather than described in prose (#16134). */ readonly path: readonly (string | number)[]; + /** + * Every member is a record. A node list read out of a container's open + * `z.record` config can hold whatever the author typed — an empty YAML list + * item deserialises to `null` — and a region its own schema refused is left + * RAW for {@link validateControlFlow} to name. The walk therefore drops a + * non-record member rather than hand out an array that does not match this + * declared type (#16752). Only this array is narrowed: the schema refusal + * that owns the malformed region still fires, and {@link path} still indexes + * the RAW list, so a finding stays anchored where the author wrote it. + */ readonly nodes: readonly FlowNodeParsed[]; readonly edges: readonly FlowEdgeParsed[]; } @@ -727,18 +742,31 @@ export function collectFlowGraphs( path: readonly (string | number)[], depth: number, ): void => { - graphs.push({ scope, path, nodes, edges }); + // A region its own schema refused is left RAW by `parseFlowNodeRegions` for + // `validateControlFlow` to name, so an element here can be whatever the + // author typed — `null` included. What is HANDED OUT and what is WALKED both + // drop it, through the one predicate above. + // + // Handed out (#16752): `FlowGraph.nodes` is declared `readonly + // FlowNodeParsed[]`, and an array whose members every caller must re-check + // is not that array. This list is one the walk picked up out of an open + // `z.record` config ITSELF — no caller ever held it, so no coercion at a + // call site can reach it. Identity is preserved when nothing is dropped. + // + // Walked (#16134): skip a non-record rather than read `.config` off it — + // this walk runs inside `FlowSchema`'s parse, where a thrown TypeError would + // escape `safeParse`, which is why this is a skip and not a throw. The schema + // refusal that owns the malformed region still fires, reached now where the + // throw used to pre-empt it. + const kept = nodes.filter((node) => isRegionDict(node)); + graphs.push({ scope, path, nodes: kept.length === nodes.length ? nodes : kept, edges }); if (depth >= MAX_REGION_DEPTH) return; + // Indexed over the RAW list, never `kept`: `path` anchors a Zod issue where + // the author wrote the node, so dropping a member must not renumber the + // siblings that outlive it. `Array.isArray` on the inner list below proves + // the LIST, never its MEMBERS — the sentence removed from four lint readers. nodes.forEach((node, index) => { - // A region its own schema refused is left RAW by `parseFlowNodeRegions` - // for `validateControlFlow` to name, so an element here can be whatever - // the author typed — `null` included. Skip what is not a node object - // rather than read `.config` off it: this walk runs inside `FlowSchema`'s - // parse (#16134), where a thrown TypeError would escape `safeParse`. The - // schema refusal that owns the malformed region still fires — reached now, - // where the throw used to pre-empt it. - const raw: unknown = node; - if (raw === null || typeof raw !== 'object') return; + if (!isRegionDict(node)) return; for (const slot of regionSlotsOf(node)) { if (!isRegionDict(slot.raw) || !Array.isArray(slot.raw.nodes)) continue; visit( diff --git a/packages/spec/src/automation/region-normalization.test.ts b/packages/spec/src/automation/region-normalization.test.ts index 2d34ce7a3d..212ced9b1c 100644 --- a/packages/spec/src/automation/region-normalization.test.ts +++ b/packages/spec/src/automation/region-normalization.test.ts @@ -279,4 +279,113 @@ describe('#4347 — collectFlowGraphs', () => { expect(graphs.length).toBeGreaterThan(1); expect(graphs.length).toBeLessThan(64); }); + + /** + * #16752 — what the walk HANDS OUT matches its declared + * `readonly FlowNodeParsed[]`. + * + * The list in question is one `collectFlowGraphs` picks up ITSELF, out of a + * container's open `z.record` config, and casts after an `Array.isArray` that + * proves the LIST and never its MEMBERS — the sentence #15552 / #15636 / + * #15742 / #15793 removed from four lint readers. No caller ever holds this + * array, so no coercion at a call site can reach it; the guard belongs here. + * + * #16134 already stopped the walk DEREFERENCING a non-record member (a + * `TypeError` thrown here escapes `FlowSchema.safeParse` rather than becoming + * an issue). It did not stop the walk HANDING IT OUT: `nodes` was pushed + * verbatim, so every graph over a junk-bearing list carried the junk — at + * every depth, not only at the `MAX_REGION_DEPTH` ceiling the filing found. + * + * ⛔ The repair is a drop, never a looser signature: widening the declared + * type to tolerate malformed members is the direction #15793 refused on the + * anti-AI-error axis. As with the four repairs above, a dropped member + * renumbers the ones behind it in `graph.nodes` — a difference in the index, + * never in whether a node was judged; `graph.path`, which anchors a Zod issue + * where the author wrote it, is pinned below to stay indexed over the RAW + * list. + */ + describe('#16752 — a non-record member never reaches a returned graph', () => { + /** + * The five shapes a raw node list holds that are not a node. `null` is the + * one an author writes by accident (an empty YAML list item deserialises to + * it); `an array` is the one a bare `typeof x === 'object'` test admits, so + * it pins that the drop is a record test and not an object test. + */ + const NON_NODES: readonly (readonly [string, unknown])[] = [ + ['null', null], + ['undefined', undefined], + ['a string', 'x'], + ['a number', 42], + ['an array', []], + ]; + + const isRecord = (v: unknown): boolean => typeof v === 'object' && v !== null && !Array.isArray(v); + + /** + * `depth` nested loop bodies, the innermost holding `junk` beside two real + * nodes. Hand-built rather than parsed on purpose: a region its own schema + * refuses is left RAW by `parseFlowNodeRegions`, and raw is the state this + * walk has to survive. + */ + const nestedJunk = (depth: number, junk: unknown) => { + let region: Record = { + nodes: [junk, { ...gate, id: 'gate_in' }, { ...write, id: 'write_in' }], + edges: [], + }; + for (let i = depth; i > 0; i--) { + region = { nodes: [loopWith(region, `lp${i}`)], edges: [] }; + } + return { nodes: region.nodes as never, edges: [] }; + }; + + describe.each(NON_NODES)('with %s in the innermost body', (_label, junk) => { + // 0 is the flow's own list, 1 the shape the card reproduced, and 32 the + // depth ceiling — where `visit` pushes a graph and returns without ever + // walking its members, so the junk was handed out with nothing having + // looked at it. + it.each([0, 1, 32])('hands out only records at nesting %i', (depth) => { + const graphs = collectFlowGraphs(nestedJunk(depth, junk)); + expect(graphs.flatMap(g => g.nodes).filter(n => !isRecord(n))).toEqual([]); + }); + + it('still hands out the real nodes standing beside it', () => { + // Anti-vacuity: the drop takes what cannot be read, not the list. A + // guard that emptied every graph would pass the assertion above. + const graphs = collectFlowGraphs(nestedJunk(1, junk)); + expect(graphs[graphs.length - 1]!.nodes.map(n => n.id)).toEqual(['gate_in', 'write_in']); + }); + + it('lets `FlowSchema.safeParse` return an envelope rather than throw (#16134)', () => { + // This walk runs inside the parse, so the repair has to stay a drop and + // a skip; a throw here escapes `safeParse` instead of becoming an issue. + const result = FlowSchema.safeParse({ + name: 'repro', label: 'Repro', type: 'schedule', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + loopWith({ nodes: [junk], edges: [] }), + ], + edges: [], + }); + expect(typeof result.success).toBe('boolean'); + }); + }); + + it('leaves `path` indexed over the RAW list, so a finding stays where the author wrote it', () => { + // The container is at authored index 1 whether or not a non-record + // precedes it: dropping a member must not renumber its siblings in the + // key path a Zod issue is anchored on (#16134). + const graphs = collectFlowGraphs({ + nodes: [null, loopWith(gatedRegion())] as never, + edges: [], + }); + expect(graphs.map(g => g.path)).toEqual([[], ['nodes', 1, 'config', 'body']]); + }); + + it('hands back the very same array when there is nothing to drop', () => { + // Copy-on-write, as `parseFlowNodeRegions` is: a well-formed flow pays + // nothing for the guard. + const nodes = [{ ...gate }, { ...write }]; + expect(collectFlowGraphs({ nodes: nodes as never, edges: [] })[0]!.nodes).toBe(nodes); + }); + }); });