Browse Source

fix: expressions can see the loop variable the author named

An OCR reply was addressed to nobody and the send failed with "At least one
recipient is required". The recipient came from {{email.from}}, which evaluated
to empty - as did {{email.subject}} in the same node, so the subject read
"Re:  - OCR Result" with a gap where the subject should be. Two empty fields,
one broken lookup.

Two causes, both older than they look. Neither had ever run: the attachment
extraction fixed in 5756a79 failed first on every previous attempt, so nothing
downstream of it had executed.

First, a nested loop built its iteration context from scratch, holding only its
own item. build_email sits in a loop over one email's attachments, nested in a
loop over emails, so the outer loop's `email` was simply not there. The inner
loop now inherits the enclosing iteration's context, and its own values win on
a clash, so a nested loop reusing the same variable name still means its own
item.

Second, and the larger of the two: evaluateJavaScriptExpression declares a
FIXED list of names - item, index, currentItem, currentIndex, totalItems,
isFirst, isLast, loop, data. A loop's item variable is configurable, so naming
it "email" makes {{email.from}} an undefined identifier and the expression
yields nothing rather than failing. Every workflow that referenced a loop item
by the name it chose has been quietly getting empty strings. Each input key
that is a usable identifier is now bound, skipping reserved words - binding
`const return = ...` would take the whole expression down with it.

A code node saw `email` correctly throughout, which is why this hid: only
expression evaluation was affected, and the data was always present.

The test pins that a node inside a nested loop can reference both loops' items
at once, with the inner loop deliberately using a different variable name.
fszontagh 3 tuần trước cách đây
mục cha
commit
8061e0f5dc

+ 52 - 2
src/runner/workflow_engine.cpp

@@ -3324,7 +3324,11 @@ bool WorkflowEngine::executeLoopBody(
         std::unordered_map<std::string, NodeExecutionResult> iteration_results;
 
         // Create input for first body node with current item
-        nlohmann::json loop_input;
+        // Starts from the enclosing iteration's context, if there is one, so an
+        // outer loop's variables stay addressable inside this one. Everything
+        // below overwrites, so the inner loop's own values always win.
+        nlohmann::json loop_input = ctx.inherited.is_object() ? ctx.inherited
+                                                              : nlohmann::json::object();
         loop_input[ctx.item_variable] = ctx.items[i];
         loop_input[ctx.index_variable] = i;
         loop_input["totalItems"] = ctx.items.size();
@@ -3631,6 +3635,9 @@ bool WorkflowEngine::executeLoopBody(
                 inner_ctx.item_variable = body_result.output.value("_itemVariable", "item");
                 inner_ctx.index_variable = body_result.output.value("_indexVariable", "index");
                 inner_ctx.continue_on_error = body_result.output.value("_continueOnError", true);
+                // What this iteration can see is what the nested loop's body
+                // should see too, minus anything the inner loop redefines.
+                inner_ctx.inherited = loop_input;
 
                 const std::string inner_key_prefix =
                     key_prefix + body_node_id + "_iter_" + std::to_string(i) + "_";
@@ -4100,6 +4107,49 @@ nlohmann::json WorkflowEngine::evaluateJavaScriptExpression(
 
     // Build JavaScript code to evaluate the expression
     // Wrap in module.exports.execute format expected by ScriptEngine
+    // A loop may name its item whatever it likes - itemVariableName is part of
+    // the loop node's configuration - and the names above are only the fixed
+    // ones. Without this, a loop over emails whose item is called "email" makes
+    // {{email.from}} an undefined identifier, and the expression yields nothing
+    // at all: an OCR reply came out addressed to "" and the send failed with
+    // "At least one recipient is required".
+    //
+    // Every remaining input key that is a usable identifier is bound, so both
+    // the canonical loop.item form and the author's chosen name work.
+    std::string custom_bindings;
+    if (input.is_object()) {
+        static const std::set<std::string> already_bound = {
+            "item", "index", "currentItem", "currentIndex", "totalItems",
+            "isFirst", "isLast", "loop", "data"
+        };
+        // Binding one of these would be a syntax error and take the whole
+        // expression down with it.
+        static const std::set<std::string> reserved = {
+            "break", "case", "catch", "class", "const", "continue", "debugger",
+            "default", "delete", "do", "else", "export", "extends", "finally",
+            "for", "function", "if", "import", "in", "instanceof", "new",
+            "return", "super", "switch", "this", "throw", "try", "typeof",
+            "var", "void", "while", "with", "yield", "let", "static", "enum",
+            "await", "implements", "package", "protected", "interface",
+            "private", "public", "null", "true", "false", "undefined",
+            "arguments", "eval", "input", "config", "context", "module",
+            "exports", "execute"
+        };
+        for (const auto& [key, value] : input.items()) {
+            if (already_bound.count(key) || reserved.count(key)) continue;
+            if (key.empty()) continue;
+            const bool valid_start = std::isalpha(static_cast<unsigned char>(key[0])) ||
+                                     key[0] == '_' || key[0] == '$';
+            if (!valid_start) continue;
+            const bool valid = std::all_of(key.begin(), key.end(), [](unsigned char c) {
+                return std::isalnum(c) || c == '_' || c == '$';
+            });
+            if (!valid) continue;
+            custom_bindings += "            const " + key + " = input[" +
+                               nlohmann::json(key).dump() + "];\n";
+        }
+    }
+
     std::string js_code = R"(
         const $node = )" + node_outputs.dump() + R"(;
 
@@ -4144,7 +4194,7 @@ nlohmann::json WorkflowEngine::evaluateJavaScriptExpression(
             // still runs first and keeps producing the bare "item" form for
             // expressions written before this existed.
             const loop = input.loop || {};
-
+)" + custom_bindings + R"(
             return )" + safe_expression + R"(;
         }
 

+ 13 - 0
src/runner/workflow_engine.hpp

@@ -464,6 +464,19 @@ private:
         std::vector<nlohmann::json> results;
         size_t current_index = 0;
 
+        // The enclosing iteration's context, when this loop is nested inside
+        // another. Body nodes of the inner loop see these keys too, so a
+        // reference to the OUTER loop's item still resolves - `{{email.from}}`
+        // inside a loop over that email's attachments, say. Without it the
+        // inner loop builds a context containing only its own variables and
+        // every outer reference silently resolves to empty, which is how an
+        // OCR reply was addressed to nobody: `to` came out "" and the send
+        // failed with "At least one recipient is required".
+        //
+        // Inner values win on a clash, so a nested loop that reuses the same
+        // item variable name still means its own item.
+        nlohmann::json inherited;
+
         // How many body passes ran, and how many of those failed. Filled in by
         // executeLoopBody on the way out.
         //

+ 137 - 0
tests/nodes/nested-loop-sees-outer-item.json

@@ -0,0 +1,137 @@
+{
+  "name": "nested-loop-sees-outer-item",
+  "description": "A node inside a nested loop must still be able to reference the OUTER loop's item. Reproduces the Email OCR failure of 2026-09-04: build_email sits in a loop over one email's attachments, nested in a loop over emails, and {{email.from}} resolved to empty - so the reply was addressed to nobody and the send failed with \"At least one recipient is required\". The inner loop deliberately uses a different item variable name, and the assertion checks both are visible at once.",
+  "nodes": [
+    {
+      "id": "n1",
+      "name": "Trigger",
+      "type": "click-trigger",
+      "position": {
+        "x": 0,
+        "y": 0
+      },
+      "config": {}
+    },
+    {
+      "id": "seed",
+      "name": "Seed",
+      "type": "code",
+      "position": {
+        "x": 0,
+        "y": 100
+      },
+      "config": {
+        "code": "return { mails: [{ from: 'a@example.com', parts: ['p1', 'p2'] }] };"
+      }
+    },
+    {
+      "id": "outer",
+      "name": "Outer",
+      "type": "loop",
+      "position": {
+        "x": 0,
+        "y": 200
+      },
+      "config": {
+        "inputField": "result.mails",
+        "itemVariableName": "email",
+        "continueOnError": false
+      }
+    },
+    {
+      "id": "inner",
+      "name": "Inner",
+      "type": "loop",
+      "position": {
+        "x": 240,
+        "y": 200
+      },
+      "config": {
+        "inputField": "data.parts",
+        "itemVariableName": "part",
+        "continueOnError": false
+      }
+    },
+    {
+      "id": "build",
+      "name": "Build",
+      "type": "set-fields",
+      "position": {
+        "x": 480,
+        "y": 200
+      },
+      "config": {
+        "mode": "only-set",
+        "fields": [
+          {
+            "name": "outerFrom",
+            "value": "{{email.from}}"
+          },
+          {
+            "name": "innerPart",
+            "value": "{{part}}"
+          }
+        ]
+      }
+    },
+    {
+      "id": "after",
+      "name": "After",
+      "type": "set-fields",
+      "position": {
+        "x": 240,
+        "y": 340
+      },
+      "config": {
+        "mode": "only-set",
+        "fields": [
+          {
+            "name": "done",
+            "value": "yes"
+          }
+        ]
+      }
+    }
+  ],
+  "connections": [
+    {
+      "sourceNodeId": "n1",
+      "sourceOutput": "main",
+      "targetNodeId": "seed",
+      "targetInput": "data"
+    },
+    {
+      "sourceNodeId": "seed",
+      "sourceOutput": "main",
+      "targetNodeId": "outer",
+      "targetInput": "data"
+    },
+    {
+      "sourceNodeId": "outer",
+      "sourceOutput": "loop",
+      "targetNodeId": "inner",
+      "targetInput": "data"
+    },
+    {
+      "sourceNodeId": "inner",
+      "sourceOutput": "loop",
+      "targetNodeId": "build",
+      "targetInput": "data"
+    },
+    {
+      "sourceNodeId": "outer",
+      "sourceOutput": "done",
+      "targetNodeId": "after",
+      "targetInput": "data"
+    }
+  ],
+  "expect": {
+    "build": {
+      "status": "completed",
+      "output": {
+        "outerFrom": "a@example.com",
+        "innerPart": "p2"
+      }
+    }
+  }
+}