fix: stop-and-error now means what it says inside a loop, and gains a skip mode

In error mode - the default - this node did nothing at all inside a loop body.
It threw, and a throw there is caught by that loop's Continue On Error, which
defaults to true. So whether a node whose entire purpose is ending the run
worked at all depended on a setting on a different node:

  loop Continue On Error | mode: stop    | mode: error
  true (the default)     | ends the run  | nothing happens
  false                  | ends the run  | ends the run, failed

Two live workflows were in that state: "Tag Missing" in the publisher and "OCR
Timeout" in Email OCR both fired and were ignored. Two others - "No Attachments
- Skip" and "No OCR Attachments - Skip" - wanted the per-item skip the bug gave
them by accident, so simply making error mode work would have broken them,
aborting a whole run on the first attachment-less email. Two intents, both
spelled mode:error, resolved by an unrelated setting.

They are said apart now:

  error  fail the whole run                                    (now effective)
  stop   end the whole run as completed                        (unchanged)
  skip   end this pass, let the loop take the next item;
         outside a loop, the same as stop                      (new)

Every mode asks the engine through a marker rather than throwing, because a
marker is the only way to say "end the run" and be sure of it - both walks act
on it and leave before they reach their failure handling, so no tolerance
setting gets the chance to swallow it. The node's own record still reads failed
with its reason, which the node suite caught me dropping: the execution failed
but the halt node would have rendered as an ordinary completed node, leaving a
failed run with no visible cause.

execution.failed still fires for a marker-driven failure, read off the wire
both inside a loop and outside it, so an error handler still runs.

Also: a deliberate failure sets stop_requested internally - that is what stops
the nodes after a loop - which would have made a failed run report itself as
"stopped". That flag means "ended early on purpose, and nothing went wrong", so
it is now reported only when the run did not fail.

The two Skip nodes have been migrated to skip mode and Email OCR republished.

scripts/characterise-loops.py is the harness this was verified with, added
because the node suite cannot catch this class of change on its own - it
records what eleven loop shapes do, to be run before and after an engine change
and diffed. The eight pre-existing shapes are unchanged by this commit; three
new ones pin the modes above. Node suite 92 passed 0 failed.

nodes/core/stop-and-error.js

@@ -13,9 +13,9 @@ const configSchema = {
         mode: {
             type: 'string',
             title: 'Mode',
-            enum: ['error', 'stop'],
+            enum: ['error', 'stop', 'skip'],
             default: 'error',
-            description: 'error fails the run, which is what the error trigger reacts to and what shows red in the execution list. stop ends it as an ordinary completed run. Use stop for a condition that is expected, such as a scheduled poll finding nothing to do - marking those failed every few minutes buries the real failures'
+            description: 'error fails the whole run, which is what the error trigger reacts to and what shows red in the execution list. stop ends the whole run as an ordinary completed one - use it for an expected condition such as a scheduled poll finding nothing to do, since marking those failed every few minutes buries the real failures. skip ends only the current loop iteration and lets the loop carry on with the next item; outside a loop it behaves like stop. error and stop both end the entire run even when they fire inside a loop body'
         },
         message: {
             type: 'string',
@@ -37,28 +37,43 @@ const inputSchema = {
 const outputSchema = {
     type: 'object',
     properties: {
-        stopped: { type: 'boolean', description: 'True when the run was ended by this node in stop mode' },
+        stopped: { type: 'boolean', description: 'True when this node ended the run or the iteration' },
+        scope: { type: 'string', description: 'iteration when only the current loop pass was ended, otherwise absent' },
         reason: { type: 'string', description: 'The message the run was ended with' }
     }
 };
 
 async function execute(config, input, context) {
     const message = config.message || 'Workflow stopped';
+    const mode = config.mode || 'error';
 
-    // Default to failing. This node has always thrown, and a workflow relying
-    // on the error trigger firing must keep working after this option arrived.
-    if ((config.mode || 'error') !== 'stop') {
-        smartbotic.log.warn('Stop and Error: ' + message);
-        throw new Error(message);
+    // Every mode asks the engine, through this marker, rather than throwing.
+    //
+    // error mode used to throw, and a throw inside a loop body is caught by the
+    // loop's own Continue On Error - which defaults to true. So a node whose
+    // entire purpose is ending the run did nothing at all there, and whether it
+    // worked depended on a setting on a different node. Asking the engine
+    // directly is the only way to say "end the run" and be sure of it.
+    //
+    // The error trigger still fires: fail makes the execution end as Failed
+    // carrying this message, which is what it reacts to.
+    if (mode === 'skip') {
+        smartbotic.log.info('Skip: ' + message);
+        return {
+            _stop: { reason: message, scope: 'iteration' }
+        };
     }
 
-    // Throwing is the only way a node can halt a run by itself, and a throw is
-    // by definition a failure. Ending a run without failing needs the engine's
-    // cooperation, which is what this marker asks for: it stops the walk, skips
-    // the remaining nodes, and still finishes the execution as completed.
-    smartbotic.log.info('Stop: ' + message);
+    if (mode === 'stop') {
+        smartbotic.log.info('Stop: ' + message);
+        return {
+            _stop: { reason: message }
+        };
+    }
+
+    smartbotic.log.warn('Stop and Error: ' + message);
     return {
-        _stop: { reason: message }
+        _stop: { reason: message, fail: true }
     };
 }
 

scripts/characterise-loops.py

@@ -0,0 +1,323 @@
+#!/usr/bin/env python3
+"""Characterisation tests for loop execution.
+
+Runs a set of workflows that exercise the loop walk and writes a normalised
+summary of what each one did. The point is NOT that the recorded behaviour is
+correct - it is to notice when a change to the engine alters any of it, so
+every difference has to be looked at and justified rather than discovered
+later in production.
+
+    scripts/characterise-loops.py before.json     # before your change
+    ... rebuild the runner and restart it ...
+    scripts/characterise-loops.py after.json      # after it
+    diff before.json after.json
+
+This exists because the node suite cannot catch this class of change. When the
+two graph walks were unified (2026-08-13), four behaviours differed between a
+node inside a loop and the same node outside one; three of the four were
+invisible to all 92 node tests, which passed green throughout. What caught them
+was diffing these baselines.
+
+Needs the webserver and a runner up, and logs in as admin/admin. Every workflow
+it creates is named "zz char: ..." and is deleted again, including on failure.
+"""
+import json, sys, time, urllib.request
+
+API = "http://localhost:8090/api/v1"
+
+
+def req(method, path, body=None, token=None):
+    data = json.dumps(body).encode() if body is not None else None
+    r = urllib.request.Request(API + path, data=data, method=method,
+                               headers={"Content-Type": "application/json"})
+    if token:
+        r.add_header("Authorization", "Bearer " + token)
+    return json.loads(urllib.request.urlopen(r).read())
+
+
+TOKEN = req("POST", "/auth/login", {"username": "admin", "password": "admin"})["accessToken"]
+
+
+def conn(src, tgt, src_out="main", tgt_in="data"):
+    return {"sourceNodeId": src, "sourceOutput": src_out,
+            "targetNodeId": tgt, "targetInput": tgt_in}
+
+
+def seed(items, node_id="seed"):
+    return {"id": node_id, "type": "code", "name": node_id, "position": {"x": 0, "y": 0},
+            "config": {"code": "return { items: %s }" % json.dumps(items)}}
+
+
+def loop_node(node_id="loop", field="result.items", cont=True, x=240):
+    return {"id": node_id, "type": "loop", "name": node_id, "position": {"x": x, "y": 0},
+            "config": {"inputField": field, "continueOnError": cont}}
+
+
+def setf(node_id, name, value, x=480, disabled=False):
+    n = {"id": node_id, "type": "set-fields", "name": node_id, "position": {"x": x, "y": 0},
+         "config": {"fields": [{"name": name, "value": value, "type": "string"}]}}
+    if disabled:
+        n["disabled"] = True
+    return n
+
+
+CASES = {}
+
+
+def case(name):
+    def deco(fn):
+        CASES[name] = fn
+        return fn
+    return deco
+
+
+@case("simple_three_items_two_body_nodes")
+def _():
+    return {
+        "nodes": [
+            seed([{"n": "a"}, {"n": "b"}, {"n": "c"}]),
+            loop_node(),
+            setf("first", "seen", "{{ loop.item.n }}"),
+            # The item must still be readable on the SECOND body node.
+            setf("second", "alsoSeen", "{{ loop.item.n }}-{{ loop.index }}", x=720),
+            setf("after", "done", "yes", x=480),
+        ],
+        "connections": [
+            conn("seed", "loop"), conn("loop", "first", "loop"),
+            conn("first", "second"), conn("loop", "after", "done"),
+        ],
+    }
+
+
+@case("nested_loop_two_by_two")
+def _():
+    return {
+        "nodes": [
+            seed([{"n": "x"}, {"n": "y"}]),
+            loop_node("outer"),
+            {"id": "inner_seed", "type": "code", "name": "inner_seed",
+             "position": {"x": 480, "y": 0},
+             "config": {"code": "return { sub: [{m:'1'}, {m:'2'}] }"}},
+            loop_node("inner", "result.sub", x=720),
+            setf("leaf", "pair", "{{ loop.item.m }}", x=960),
+            setf("after", "done", "yes", x=480),
+        ],
+        "connections": [
+            conn("seed", "outer"), conn("outer", "inner_seed", "loop"),
+            conn("inner_seed", "inner"), conn("inner", "leaf", "loop"),
+            conn("outer", "after", "done"),
+        ],
+    }
+
+
+@case("branch_gating_in_body")
+def _():
+    return {
+        "nodes": [
+            seed([{"n": "a"}, {"n": "b"}]),
+            loop_node(),
+            setf("mark", "seen", "{{ loop.item.n }}"),
+            {"id": "gate", "type": "if-condition", "name": "gate", "position": {"x": 720, "y": 0},
+             "config": {"conditions": [{"field": "data.seen", "operator": "equals", "value": "a"}]}},
+            setf("on_true", "took", "true", x=960),
+            setf("on_false", "took", "false", x=960),
+            setf("after", "done", "yes", x=480),
+        ],
+        "connections": [
+            conn("seed", "loop"), conn("loop", "mark", "loop"), conn("mark", "gate"),
+            conn("gate", "on_true", "true"), conn("gate", "on_false", "false"),
+            conn("loop", "after", "done"),
+        ],
+    }
+
+
+@case("back_edge_true_port_only")
+def _():
+    return {
+        "nodes": [
+            seed([{"n": "a"}, {"n": "b"}, {"n": "c"}]),
+            loop_node(),
+            setf("mark", "seen", "{{ loop.item.n }}"),
+            {"id": "gate", "type": "if-condition", "name": "gate", "position": {"x": 720, "y": 0},
+             "config": {"conditions": [{"field": "data.seen", "operator": "not_equals", "value": "a"}]}},
+            setf("after", "done", "yes", x=480),
+        ],
+        "connections": [
+            conn("seed", "loop"), conn("loop", "mark", "loop"), conn("mark", "gate"),
+            conn("gate", "loop", "true"), conn("loop", "after", "done"),
+        ],
+    }
+
+
+@case("disabled_node_in_body_is_transparent")
+def _():
+    return {
+        "nodes": [
+            seed([{"n": "a"}, {"n": "b"}]),
+            loop_node(),
+            setf("mark", "seen", "{{ loop.item.n }}"),
+            setf("off", "ignored", "x", x=720, disabled=True),
+            setf("tail", "reached", "yes", x=960),
+            setf("after", "done", "yes", x=480),
+        ],
+        "connections": [
+            conn("seed", "loop"), conn("loop", "mark", "loop"),
+            conn("mark", "off"), conn("off", "tail"), conn("loop", "after", "done"),
+        ],
+    }
+
+
+@case("body_failure_continue_on_error_true")
+def _():
+    return {
+        "nodes": [
+            seed([{"n": "a"}, {"n": "b"}, {"n": "c"}]),
+            loop_node(cont=True),
+            # NOTE: a code node's body cannot see `loop` - it is available to
+            # expressions, not as a variable in the sandbox - so this throws on
+            # every item, not only on 'b'. Left as it is because a baseline only
+            # has to be stable and this one exercises "every item failed, run
+            # completed anyway"; do not read the case name as a promise that
+            # exactly one item fails.
+            {"id": "boom", "type": "code", "name": "boom", "position": {"x": 480, "y": 0},
+             "config": {"code": "if (loop.item.n === 'b') { throw new Error('planned'); } return { ok: loop.item.n }"}},
+            setf("after", "done", "yes", x=480),
+        ],
+        "connections": [
+            conn("seed", "loop"), conn("loop", "boom", "loop"), conn("loop", "after", "done"),
+        ],
+    }
+
+
+@case("body_failure_continue_on_error_false")
+def _():
+    return {
+        "nodes": [
+            seed([{"n": "a"}, {"n": "b"}, {"n": "c"}]),
+            loop_node(cont=False),
+            {"id": "boom", "type": "code", "name": "boom", "position": {"x": 480, "y": 0},
+             "config": {"code": "if (loop.item.n === 'b') { throw new Error('planned'); } return { ok: loop.item.n }"}},
+            setf("after", "done", "yes", x=480),
+        ],
+        "connections": [
+            conn("seed", "loop"), conn("loop", "boom", "loop"), conn("loop", "after", "done"),
+        ],
+    }
+
+
+def _halt_in_loop(mode):
+    """A loop of three whose middle item trips a stop-and-error."""
+    return {
+        "nodes": [
+            seed([{"n": "a"}, {"n": "b"}, {"n": "c"}]),
+            loop_node(),
+            setf("mark", "seen", "{{ loop.item.n }}"),
+            {"id": "gate", "type": "if-condition", "name": "gate", "position": {"x": 720, "y": 0},
+             "config": {"conditions": [{"field": "data.seen", "operator": "equals", "value": "b"}]}},
+            {"id": "halt", "type": "stop-and-error", "name": "halt",
+             "position": {"x": 960, "y": -80},
+             "config": {"mode": mode, "message": "item b says stop"}},
+            setf("keep", "kept", "yes", x=960),
+            setf("after", "done", "yes", x=480),
+        ],
+        "connections": [
+            conn("seed", "loop"), conn("loop", "mark", "loop"), conn("mark", "gate"),
+            conn("gate", "halt", "true"), conn("gate", "keep", "false"),
+            conn("loop", "after", "done"),
+        ],
+    }
+
+
+# The three ways a body node can end something, which are three different
+# things: fail the whole run, end the whole run cleanly, and drop this one item
+# and carry on. error mode used to do none of them inside a loop - it threw, and
+# the loop's Continue On Error caught it - so all three are pinned here.
+@case("halt_in_loop_error_fails_the_run")
+def _():
+    return _halt_in_loop("error")
+
+
+@case("halt_in_loop_stop_ends_the_run")
+def _():
+    return _halt_in_loop("stop")
+
+
+@case("halt_in_loop_skip_drops_one_item")
+def _():
+    return _halt_in_loop("skip")
+
+
+@case("empty_item_list")
+def _():
+    return {
+        "nodes": [
+            seed([]),
+            loop_node(),
+            setf("mark", "seen", "{{ loop.item.n }}"),
+            setf("after", "done", "yes", x=480),
+        ],
+        "connections": [
+            conn("seed", "loop"), conn("loop", "mark", "loop"), conn("loop", "after", "done"),
+        ],
+    }
+
+
+def run_case(name, spec):
+    wf = req("POST", "/workflows", {"name": "zz char: " + name, "active": False,
+                                    "nodes": spec["nodes"],
+                                    "connections": spec["connections"]}, TOKEN)
+    wf_id = wf.get("_id") or wf.get("id")
+    try:
+        ex = req("POST", f"/workflows/{wf_id}/execute", {}, TOKEN)
+        exec_id = ex.get("executionId")
+        detail = {}
+        for _ in range(40):
+            time.sleep(1.5)
+            detail = req("GET", "/executions/" + exec_id, token=TOKEN)
+            if detail.get("status") not in ("running", "pending"):
+                break
+
+        records = []
+        for rec in detail.get("nodeExecutions", []):
+            out = rec.get("output")
+            # Only the fields a workflow author would see, and drop the engine's
+            # internal loop bookkeeping, which is noisy and not behaviour.
+            if isinstance(out, dict):
+                out = {k: v for k, v in out.items() if not k.startswith("_")}
+            records.append({
+                "node": rec.get("nodeId"),
+                "status": rec.get("status"),
+                "iteration": rec.get("loopIteration"),
+                "loopNode": rec.get("loopNodeId"),
+                "error": (rec.get("error") or "").split("\n")[0][:110],
+                "output": out,
+            })
+        # Sorted so a run-to-run ordering wobble is not mistaken for a change.
+        records.sort(key=lambda r: (r["node"], str(r["iteration"])))
+        return {"status": detail.get("status"),
+                "error": (detail.get("error") or "").split("\n")[0][:110],
+                "stopped": detail.get("stopped"),
+                "stopReason": detail.get("stopReason"),
+                "toleratedErrorCount": detail.get("toleratedErrorCount"),
+                "records": records}
+    finally:
+        urllib.request.urlopen(urllib.request.Request(
+            API + "/workflows/" + wf_id, method="DELETE",
+            headers={"Authorization": "Bearer " + TOKEN}))
+
+
+out = {}
+for name in sorted(CASES):
+    print("running", name, flush=True)
+    try:
+        out[name] = run_case(name, CASES[name]())
+    except Exception as e:
+        out[name] = {"harness_error": repr(e)}
+
+path = sys.argv[1] if len(sys.argv) > 1 else "loop_characterisation.json"
+with open(path, "w") as f:
+    json.dump(out, f, indent=2, sort_keys=True)
+print("\nwrote", path)
+for name in sorted(out):
+    r = out[name]
+    print("  %-42s %s" % (name, r.get("status") or r.get("harness_error")))

src/runner/workflow_engine.cpp

@@ -225,8 +225,15 @@ nlohmann::json ExecutionResult::toJson() const {
     // failed one carries an error. It is the only record of why an execution
     // that completed did not do the rest of its work, so it is stored even
     // though the status is Completed and the error is empty.
-    j["stopped"] = stop_requested;
-    if (stop_requested) {
+    // Only for a run that ended early and did not fail. stop_requested is also
+    // how a deliberate failure ends the walk - it is what stops the nodes after
+    // a loop from running - but reporting that as "stopped" would contradict
+    // what this flag means to a reader, which is precisely "ended early on
+    // purpose, and nothing went wrong". A failed run says so through its status
+    // and error, which carry the same message.
+    const bool stopped_cleanly = stop_requested && status != ExecutionStatus::Failed;
+    j["stopped"] = stopped_cleanly;
+    if (stopped_cleanly) {
         j["stopReason"] = stop_reason;
         j["stoppedNodeId"] = stopped_node_id;
     }
@@ -917,7 +924,19 @@ Result<ExecutionResult> WorkflowEngine::execute(const Workflow& workflow,
                 }
             }
 
-            if (marker_outcome == MarkerOutcome::Stop || marker_outcome == MarkerOutcome::Pause) {
+            // SkipIteration outside a loop has no pass to end, so it ends the
+            // walk, which is what the node's own description promises.
+            if (marker_outcome == MarkerOutcome::SkipIteration) {
+                result.stop_requested = true;
+                result.stopped_node_id = node_id;
+                if (result.stop_reason.empty()) {
+                    result.stop_reason = node_result.output.value("reason", "");
+                }
+            }
+
+            if (marker_outcome == MarkerOutcome::Stop ||
+                marker_outcome == MarkerOutcome::SkipIteration ||
+                marker_outcome == MarkerOutcome::Pause) {
                 break;
             }
 
@@ -2108,10 +2127,27 @@ WorkflowEngine::MarkerOutcome WorkflowEngine::applyNodeMarkers(
     // a strange thing to mean per-item.
     if (node_result.output.contains("_stop")) {
         const auto& stop = node_result.output["_stop"];
+        const std::string reason = stop.value("reason", "");
+
+        // "end this pass, not the run" - the loop carries on with the next item.
+        // Nothing about the execution as a whole changes, so stop_requested
+        // stays clear; the caller decides what an ended pass means, and outside
+        // a loop there is no pass to end, so it ends the walk instead.
+        if (stop.value("scope", "") == "iteration") {
+            auto& stored = result.node_results[result_key];
+            stored.output.erase("_stop");
+            stored.output["stopped"] = true;
+            stored.output["reason"] = reason;
+            stored.output["scope"] = "iteration";
+
+            LOG_INFO("Execution {}: node {}{} ended its iteration: {}", result.execution_id,
+                     node_id, where, reason.empty() ? "no reason given" : reason);
+            return MarkerOutcome::SkipIteration;
+        }
 
         result.stop_requested = true;
         result.stopped_node_id = node_id;
-        result.stop_reason = stop.value("reason", "");
+        result.stop_reason = reason;
 
         // The marker is engine plumbing. What the node reports is that it
         // stopped and why, not the mechanism that carried it.
@@ -2120,6 +2156,38 @@ WorkflowEngine::MarkerOutcome WorkflowEngine::applyNodeMarkers(
         stored.output["stopped"] = true;
         stored.output["reason"] = result.stop_reason;
 
+        // A deliberate failure. This arrives as a marker rather than a thrown
+        // error precisely so a loop cannot swallow it: a throw inside a loop
+        // body is caught by that loop's Continue On Error, which defaults to
+        // true, so stop-and-error in its default mode did nothing at all there
+        // and whether it worked depended on a setting on another node.
+        //
+        // Setting the status here is what ends the run as Failed: the walk
+        // only promotes a still-Running execution to Completed, so this stands.
+        if (stop.value("fail", false)) {
+            result.status = ExecutionStatus::Failed;
+            result.error = reason;
+
+            // The node itself failed, and its record has to say so - it is what
+            // shows red on the canvas and carries the reason to whoever is
+            // reading the run back. Only the mechanism changed, from a thrown
+            // error to this marker; the node did not stop being a failure.
+            //
+            // Safe to set inside a loop even though a failed body node is
+            // subject to that loop's Continue On Error: both walks act on this
+            // Stop and leave the walk before they reach their failure handling,
+            // so the tolerance never gets the chance to swallow it. That
+            // ordering is the whole point of routing this through a marker.
+            node_result.status = NodeStatus::Failed;
+            node_result.error = reason;
+            stored.status = NodeStatus::Failed;
+            stored.error = reason;
+
+            LOG_INFO("Execution {} failed at node {}{}: {}", result.execution_id, node_id, where,
+                     reason.empty() ? "no reason given" : reason);
+            return MarkerOutcome::Stop;
+        }
+
         LOG_INFO("Execution {} stopped at node {}{}: {}", result.execution_id, node_id, where,
                  result.stop_reason.empty() ? "no reason given" : result.stop_reason);
         return MarkerOutcome::Stop;
@@ -3175,6 +3243,10 @@ bool WorkflowEngine::executeLoopBody(
                         break;
                     }
 
+                    if (cached_outcome == MarkerOutcome::SkipIteration) {
+                        break;
+                    }
+
                     continue;
                 }
 
@@ -3345,6 +3417,14 @@ bool WorkflowEngine::executeLoopBody(
                 break;
             }
 
+            // Only this pass. The remaining body nodes do not run for this item
+            // and the loop moves on to the next one - which is what a node
+            // saying "skip this item" has to mean, as against "end the run".
+            if (body_outcome == MarkerOutcome::SkipIteration) {
+                LOG_INFO("Loop {} item {}: skipped by node {}", loop_node_id, i, body_node_id);
+                break;
+            }
+
             if (callback) {
                 nlohmann::json event_data = {
                     {"executionId", result.execution_id},

src/runner/workflow_engine.hpp

@@ -436,9 +436,12 @@ private:
     // What a node's output asked the engine to do, once its markers have been
     // acted on.
     enum class MarkerOutcome {
-        Continue,  // nothing special, carry on
-        Stop,      // end the run, but successfully
-        Pause      // end this pass; a resume picks it up
+        Continue,      // nothing special, carry on
+        Stop,          // end the run - successfully, or failed if the node said so
+        SkipIteration, // end this loop pass only; the loop carries on with the
+                       // next item. Outside a loop there is no iteration to end,
+                       // so the walk treats it as Stop.
+        Pause          // end this pass; a resume picks it up
     };
 
     // Everything a node can ask of the engine through its output: run another