From 4bbbc6d57d45ea04c8ec72c5f1a6b42147fdabec Mon Sep 17 00:00:00 2001 From: Tom Brandenburg Date: Sun, 6 Sep 2026 18:24:35 +0200 Subject: [PATCH] fix: classify wired nodes without editor x/y as regular nodes, not config nodes Node-RED's flow parser treats any node lacking both x and y properties as a global config node, regardless of its type. Hand-authored --flow-json flows (this tool's core use case, per the README's own agent example) commonly omit those editor-only coordinates, so a wired node ends up misclassified as a config node and undergoes Flow.js's circular config node dependency scan instead of normal instantiation. That scan throws "Circular config node dependency detected" the moment one of the node's own property values equals another node's id -- including its own id, e.g. when name equals id (exactly the README's agent example). This aborts the whole flow's instantiation, which previously surfaced as 'target ... is not instantiated' and made the unrelated waitForFlowsSettled() stop/safe race look like the root cause. Assign synthetic x/y coordinates to every unambiguously wired node (has a wires array, or is a link out node) before deploying an in-memory flow, leaving genuine config nodes untouched. Also add a one-tick debounce in waitForFlowsSettled() before resolving on a stop/safe runtime-state event, so a flows:started already scheduled for the same deploy attempt wins the race instead -- defensive hardening per the issue's own suggested refinement, even though current analysis shows stop/safe and flows:started are mutually exclusive outcomes in the installed @node-red/runtime version. Fixes #28 --- src/run-envelope.js | 64 +++++++++++++++++-- .../run-envelope.integration.test.js | 38 +++++++++++ 2 files changed, 98 insertions(+), 4 deletions(-) diff --git a/src/run-envelope.js b/src/run-envelope.js index a18664c..832d6b3 100644 --- a/src/run-envelope.js +++ b/src/run-envelope.js @@ -34,6 +34,43 @@ function stderrLogHandler() { }; } +/** + * Node-RED's flow parser (`@node-red/runtime/lib/flows/util.js`) classifies + * *any* node lacking both `x` and `y` properties as a global config node -- + * regardless of its actual `type` -- since those coordinates are otherwise + * only ever used by the editor canvas. A real, editor-exported flow always + * has them on every wired node, so this never matters there. But hand-authored + * `--flow-json` flows (this tool's own core use case; see the README's + * `agent` example) commonly omit them, since they carry no runtime meaning. + * A wired node misclassified as a config node undergoes `Flow.js`'s + * config-node circular-dependency scan instead of normal instantiation, + * which scans every one of its own property values against other node ids + * and throws "Circular config node dependency detected" the moment any + * property value happens to equal another node's id -- including its own, + * e.g. a node whose `name` equals its own `id` (an extremely natural thing + * to write by hand, and exactly what the README's own agent example does). + * That aborts the whole flow's instantiation, so downstream preflight + * validation reports the target/return nodes as "not instantiated" even + * though the flow is otherwise entirely valid (see issue #28). + * + * Fix: assign synthetic coordinates to every node that is unambiguously a + * regular (wired) node -- i.e. it already declares a `wires` array, or is a + * `link out` node (which routes via `links` instead of `wires`) -- so + * Node-RED's parser classifies it correctly. Nodes without either (real + * config nodes) are left untouched. + */ +function withDeployCoordinates(flow) { + let n = 0; + return flow.map((node) => { + const isWired = Object.prototype.hasOwnProperty.call(node, "wires") || node.type === "link out"; + const hasCoords = + Object.prototype.hasOwnProperty.call(node, "x") && Object.prototype.hasOwnProperty.call(node, "y"); + if (!isWired || hasCoords) return node; + n += 1; + return { ...node, x: n * 100, y: 100 }; + }); +} + /** * Waits for Node-RED to finish attempting to start the deployed flows. * @@ -55,20 +92,39 @@ function stderrLogHandler() { * either way; if the flows never actually started, the target/return nodes * simply won't be instantiated and the existing preflight validation in * `createHostLinkCaller` reports the real, specific error instead. + * + * A `stop`/`safe` `runtime-state` event and a real `flows:started` are + * mutually exclusive outcomes of the same deploy attempt in the installed + * `@node-red/runtime` (each early-return failure path returns before ever + * reaching the code that emits `flows:started`), so this never races in + * practice today. Still, resolving on `stop`/`safe` is deferred by one + * macrotask (`setImmediate`) rather than immediately, so that if a + * `flows:started` for the same attempt is already scheduled to fire right + * after, it wins instead -- cheap insurance against exactly the kind of + * premature-resolution regression reported in issue #28, without delaying + * genuine failures beyond a single negligible tick. + * + * (Uses `setTimeout(fn, 0)` rather than `setImmediate` purely because the + * latter isn't part of this project's configured ESLint globals; both defer + * to the next macrotask.) */ function waitForFlowsSettled(RED) { return new Promise((resolve) => { - const onStarted = () => { + let settled = false; + const finish = () => { + if (settled) return; + settled = true; + RED.events.removeListener("flows:started", onStarted); RED.events.removeListener("runtime-event", onRuntimeEvent); resolve(); }; + const onStarted = () => finish(); const onRuntimeEvent = (event) => { if ( event?.id === "runtime-state" && (event.payload?.state === "stop" || event.payload?.state === "safe") ) { - RED.events.removeListener("flows:started", onStarted); - resolve(); + setTimeout(finish, 0); } }; RED.events.once("flows:started", onStarted); @@ -113,7 +169,7 @@ async function runFlowInvocation({ flow, flowFile, msg, options }) { let caller; RED.init({ - ...(flow ? { storageModule: createMemoryStorageModule(flow) } : { flowFile }), + ...(flow ? { storageModule: createMemoryStorageModule(withDeployCoordinates(flow)) } : { flowFile }), userDir, httpAdminRoot: false, httpNodeRoot: false, diff --git a/test/integration/run-envelope.integration.test.js b/test/integration/run-envelope.integration.test.js index ab64fa1..dad5e97 100644 --- a/test/integration/run-envelope.integration.test.js +++ b/test/integration/run-envelope.integration.test.js @@ -49,3 +49,41 @@ test("integration: runFlowInvocation rejects with preflight error for an unregis fs.rmSync(userDir, { recursive: true, force: true }); } }); + +/** + * Regression coverage for issue #28: a v0.2.14 report that flows using the + * `agent` node (node-red-agents) deterministically fail with "Circular + * config node dependency" / "not instantiated", even though the flow is + * otherwise valid and deployed fine on v0.2.12/v0.2.13. + * + * Root cause (confirmed against the real `@tbrandenburg/node-red-agents` + * package, and reproducible with only core node types as below): Node-RED's + * flow parser (`@node-red/runtime/lib/flows/util.js`) classifies *any* node + * lacking both `x` and `y` as a global config node, regardless of its + * actual type -- and the README's own hand-authored `--flow-json` `agent` + * example (like this fixture) omits those editor-only coordinates. A wired + * node misclassified as a config node undergoes `Flow.js`'s config-node + * circular-dependency scan, which throws "Circular config node dependency + * detected" the moment one of its own property values happens to equal + * another node's id -- including its own id, e.g. a node whose `name` + * equals its own `id` (exactly what this fixture, and the README example, + * both do). That aborts the whole flow's instantiation, which is what + * previously made the *unrelated* `waitForFlowsSettled` race a prime + * suspect: the target/return nodes end up "not instantiated" either way. + * This flow is otherwise entirely valid and must deploy and succeed exactly + * as it did before v0.2.14. + */ +const SELF_NAMED_LINK_FLOW = [ + { id: "tab", type: "tab", label: "t" }, + { id: "ask", type: "link in", z: "tab", name: "ask", wires: [["return"]] }, + { id: "return", type: "link out", z: "tab", name: "return", mode: "return" } +]; + +test("integration: runFlowInvocation deploys and calls a flow whose nodes omit editor x/y coordinates (issue #28)", async () => { + const result = await runFlowInvocation({ + flow: SELF_NAMED_LINK_FLOW, + msg: { payload: "hi" }, + options: { target: "ask", timeoutMs: 5000, format: "json" } + }); + assert.deepEqual(JSON.parse(result.output).payload, "hi"); +});