Skip to content

spec: collectFlowGraphs dereferences a non-record member of a NESTED region's node list — its own walk, not the caller's #16752

Description

@baozhoutao

Found while implementing #15793. Filed as a finding only, not claimed. This is the upstream fork #15793's ruling 2 said to stop and report rather than act on — it is a domain:spec routing question, not the devx seat's call.

What #15793 settled, and the one shape that reading does not cover

#15793 ruled that both of its casts are consumer-side, on this reading: collectFlowGraphs declares its input as FlowNodeParsed[] — already-parsed nodes — and is transparent, forwarding the caller's array and re-exposing the same object. So a null reaching graph.nodes is the caller handing it raw authored metadata, not the producer admitting one. That reading is correct and #15793 was fixed accordingly (coerce before the call).

It covers the array the caller passes in. It does not cover the arrays collectFlowGraphs picks up by itself.

Measured

On 7c12e475, with the top-level flow.nodes already coerced to records by the caller:

collectFlowGraphs({
  nodes: [ { id: 'start', type: 'start', config: {} },
           { id: 'lp', type: 'loop',
             config: { collection: 'x', body: { nodes: [null, { id: 'inner', type: 'noop', config: {} }], edges: [] } } } ],
  edges: [],
});
THREW  TypeError: Cannot read properties of null (reading 'config')
    at regionSlotsOf        control-flow.zod.ts:515:20
    at visit                control-flow.zod.ts:714:26
    at visit                control-flow.zod.ts:716:9
    at collectFlowGraphs    control-flow.zod.ts:726:3

The caller cannot coerce this list: it lives inside node.config, and collectFlowGraphs is what decides to descend into it. In visit:

if (!isRegionDict(slot.raw) || !Array.isArray(slot.raw.nodes)) continue;
visit(slot.raw.nodes as FlowNodeParsed[], ...)

Array.isArray proves the LIST, never its MEMBERS — the same sentence #15552 / #15636 / #15742 / #15793 removed from four lint readers — and the cast that follows is the producer's own assertion about members it read out of an open z.record config, not the caller's.

Why this is a genuine contract question and not just one more guard

Two readings, and they route differently:

  1. The recursive-contract reading. FlowNodeParsed.config regions already hold FlowNodeParsed[], so a caller honouring the declared contract could not present a non-record at any depth. Then this is still caller-side, the declared input is simply recursive, and the fix is that no lint reader may hand collectFlowGraphs raw metadata at all — a bigger consumer-side change than lint: validateStackExpressions throws on a non-record entry of a flow's nodes list — two inline casts no collection sweep can reach #15793 made.
  2. The producer-walk reading. collectFlowGraphs re-derives these inner arrays from an open z.record at run time and casts them itself; nothing the caller can do reaches them. Then the guard belongs here.

⭐ Deciding between these is the routing call. #15793 deliberately did not make it.

A second, independent defect in the same function — the depth ceiling leaks a non-record

visit pushes the graph before the depth guard returns:

graphs.push({ scope, nodes, edges });
if (depth >= MAX_REGION_DEPTH) return;
for (const node of nodes) { ... }

So at exactly MAX_REGION_DEPTH (32) the members are never walked, never dereferenced — and a graph whose nodes hold a null is returned to the caller successfully:

nesting 30 : THREW Cannot read properties of null (reading 'config')
nesting 31 : THREW Cannot read properties of null (reading 'config')
nesting 32 : OK, 33 graph(s); graphs whose nodes hold a NON-RECORD: 1
nesting 33 : OK, 33 graph(s); graphs whose nodes hold a NON-RECORD: 0

This is why the graph.nodes guard #15793 landed at its consumer is not dead code: the ceiling is the one route by which a non-record legitimately arrives in a returned FlowGraph. Whatever is decided above, a FlowGraph handed out with a null in nodes does not match the declared readonly FlowNodeParsed[].

Notes

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions