From d5bc7892fc34fa53ec5057ebe8757e2ab752fdf9 Mon Sep 17 00:00:00 2001
From: Hilary Oliver <hilary.j.oliver@gmail.com>
Date: Wed, 5 Apr 2023 13:52:21 +1200
Subject: [PATCH 01/15] Xtrigger function arg validation.

---
 cylc/flow/config.py               | 18 ++++++++++++++----
 cylc/flow/subprocpool.py          | 24 ++++++++++++++++--------
 cylc/flow/xtrigger_mgr.py         |  3 ++-
 cylc/flow/xtriggers/wall_clock.py | 31 +++++++++++++++++++++++++++++++
 tests/unit/test_subprocpool.py    | 21 +++++++++++----------
 5 files changed, 74 insertions(+), 23 deletions(-)

diff --git a/cylc/flow/config.py b/cylc/flow/config.py
index d80456266bf..877cdb1b6da 100644
--- a/cylc/flow/config.py
+++ b/cylc/flow/config.py
@@ -57,6 +57,7 @@
 from cylc.flow.id import Tokens
 from cylc.flow.cycling.integer import IntegerInterval
 from cylc.flow.cycling.iso8601 import ingest_time, ISO8601Interval
+
 from cylc.flow.exceptions import (
     CylcError,
     InputError,
@@ -82,6 +83,7 @@
 from cylc.flow.print_tree import print_tree
 from cylc.flow.simulation import configure_sim_modes
 from cylc.flow.subprocctx import SubFuncContext
+from cylc.flow.subprocpool import get_func
 from cylc.flow.task_events_mgr import (
     EventData,
     get_event_handler_data
@@ -1711,10 +1713,8 @@ def generate_triggers(self, lexpression, left_nodes, right, seq,
                 if label != 'wall_clock':
                     raise WorkflowConfigError(f"xtrigger not defined: {label}")
                 else:
-                    # Allow "@wall_clock" in the graph as an undeclared
-                    # zero-offset clock xtrigger.
-                    xtrig = SubFuncContext(
-                        'wall_clock', 'wall_clock', [], {})
+                    # Allow "@wall_clock" in graph as implicit zero-offset.
+                    xtrig = SubFuncContext('wall_clock', 'wall_clock', [], {})
 
             if xtrig.func_name == 'wall_clock':
                 if self.cycling_type == INTEGER_CYCLING_TYPE:
@@ -1729,10 +1729,20 @@ def generate_triggers(self, lexpression, left_nodes, right, seq,
                         with suppress(IndexError):
                             xtrig.func_kwargs["offset"] = xtrig.func_args[0]
 
+            # Call the xtrigger's validate_config function if it has one.
+            with suppress(AttributeError, ImportError):
+                get_func(xtrig.func_name, "validate_config", self.fdir)(
+                    xtrig.func_args,
+                    xtrig.func_kwargs,
+                    xtrig.get_signature()
+                )
+
             if self.xtrigger_mgr is None:
+                # Validation only.
                 XtriggerManager.validate_xtrigger(label, xtrig, self.fdir)
             else:
                 self.xtrigger_mgr.add_trig(label, xtrig, self.fdir)
+
             self.taskdefs[right].add_xtrig_label(label, seq)
 
     def get_actual_first_point(self, start_point):
diff --git a/cylc/flow/subprocpool.py b/cylc/flow/subprocpool.py
index 9ac3845bb54..42782e10b1b 100644
--- a/cylc/flow/subprocpool.py
+++ b/cylc/flow/subprocpool.py
@@ -66,8 +66,10 @@ def _killpg(proc, signal):
     return True
 
 
-def get_func(func_name, src_dir):
-    """Find and return an xtrigger function from a module of the same name.
+def get_func(mod_name, func_name, src_dir):
+    """Find and return a named function from a named module.
+
+    Can be in <src_dir>/lib/python, cylc.flow.xtriggers, or in Python path.
 
     These locations are checked in this order:
     - <src_dir>/lib/python/
@@ -78,13 +80,17 @@ def get_func(func_name, src_dir):
     Workflow source directory passed in as this is executed in an independent
     process in the command pool and therefore doesn't know about the workflow.
 
+    Raises:
+        ImportError, if the module is not found
+        AttributeError, if the function is not found in the module
+
     """
-    if func_name in _XTRIG_FUNCS:
-        return _XTRIG_FUNCS[func_name]
+    if (mod_name, func_name) in _XTRIG_FUNCS:
+        # Found and cached already.
+        return _XTRIG_FUNCS[(mod_name, func_name)]
 
     # First look in <src-dir>/lib/python.
     sys.path.insert(0, os.path.join(src_dir, 'lib', 'python'))
-    mod_name = func_name
     try:
         mod_by_name = __import__(mod_name, fromlist=[mod_name])
     except ImportError:
@@ -101,11 +107,11 @@ def get_func(func_name, src_dir):
         raise
 
     try:
-        _XTRIG_FUNCS[func_name] = getattr(mod_by_name, func_name)
+        _XTRIG_FUNCS[(mod_name, func_name)] = getattr(mod_by_name, func_name)
     except AttributeError:
         # Module func_name has no function func_name, nor an entry_point entry.
         raise
-    return _XTRIG_FUNCS[func_name]
+    return _XTRIG_FUNCS[(mod_name, func_name)]
 
 
 def run_function(func_name, json_args, json_kwargs, src_dir):
@@ -113,6 +119,8 @@ def run_function(func_name, json_args, json_kwargs, src_dir):
 
     func_name(*func_args, **func_kwargs)
 
+    The function is presumed to be in a module of the same name.
+
     Redirect any function stdout to stderr (and workflow log in debug mode).
     Return value printed to stdout as a JSON string - allows use of the
     existing process pool machinery as-is. src_dir is for local modules.
@@ -121,7 +129,7 @@ def run_function(func_name, json_args, json_kwargs, src_dir):
     func_args = json.loads(json_args)
     func_kwargs = json.loads(json_kwargs)
     # Find and import then function.
-    func = get_func(func_name, src_dir)
+    func = get_func(func_name, func_name, src_dir)
     # Redirect stdout to stderr.
     orig_stdout = sys.stdout
     sys.stdout = sys.stderr
diff --git a/cylc/flow/xtrigger_mgr.py b/cylc/flow/xtrigger_mgr.py
index 6ced0023a56..db6a3453e70 100644
--- a/cylc/flow/xtrigger_mgr.py
+++ b/cylc/flow/xtrigger_mgr.py
@@ -262,8 +262,9 @@ def validate_xtrigger(
 
         """
         fname: str = fctx.func_name
+
         try:
-            func = get_func(fname, fdir)
+            func = get_func(fname, fname, fdir)
         except ImportError:
             raise XtriggerConfigError(
                 label,
diff --git a/cylc/flow/xtriggers/wall_clock.py b/cylc/flow/xtriggers/wall_clock.py
index 3c50d956f0a..e12a17ce8be 100644
--- a/cylc/flow/xtriggers/wall_clock.py
+++ b/cylc/flow/xtriggers/wall_clock.py
@@ -17,6 +17,8 @@
 """xtrigger function to trigger off of a wall clock time."""
 
 from time import time
+from cylc.flow.cycling.iso8601 import interval_parse
+from cylc.flow.exceptions import WorkflowConfigError
 
 
 def wall_clock(trigger_time=None):
@@ -27,3 +29,32 @@ def wall_clock(trigger_time=None):
             Trigger time as seconds since Unix epoch.
     """
     return time() > trigger_time
+
+
+def validate_config(f_args, f_kwargs, f_signature):
+    """Validate and manipulate args parsed from the workflow config.
+
+    wall_clock()  # zero offset
+    wall_clock(PT1H)
+    wall_clock(offset=PT1H)
+
+    The offset must be a valid ISO 8601 interval.
+
+    If f_args used, convert to f_kwargs for clarity.
+
+    """
+    n_args = len(f_args)
+    n_kwargs = len(f_kwargs)
+
+    if n_args + n_kwargs > 1:
+        raise WorkflowConfigError(f"xtrigger: too many args: {f_signature}")
+
+    if n_args:
+        f_kwargs["offset"] = f_args[0]
+    elif not n_kwargs:
+        f_kwargs["offset"] = "P0Y"
+
+    try:
+        interval_parse(f_kwargs["offset"])
+    except ValueError:
+        raise WorkflowConfigError(f"xtrigger: invalid offset: {f_signature}")
diff --git a/tests/unit/test_subprocpool.py b/tests/unit/test_subprocpool.py
index 981ca8e75f9..02ee540fd64 100644
--- a/tests/unit/test_subprocpool.py
+++ b/tests/unit/test_subprocpool.py
@@ -155,7 +155,8 @@ def test_xfunction(self):
             with the_answer_file.open(mode="w") as f:
                 f.write("""the_answer = lambda: 42""")
                 f.flush()
-            fn = get_func("the_answer", temp_dir)
+                f_name = "the_answer"
+            fn = get_func(f_name, f_name, temp_dir)
             result = fn()
             self.assertEqual(42, result)
 
@@ -166,19 +167,18 @@ def test_xfunction_cache(self):
             python_dir.mkdir(parents=True)
             amandita_file = python_dir / "amandita.py"
             with amandita_file.open(mode="w") as f:
-                f.write("""amandita = lambda: 'chocolate'""")
+                f.write("""choco = lambda: 'chocolate'""")
                 f.flush()
-            fn = get_func("amandita", temp_dir)
+            m_name = "amandita"  # module
+            f_name =  "choco"  # function
+            fn = get_func(m_name, f_name, temp_dir)
             result = fn()
             self.assertEqual('chocolate', result)
 
             # is in the cache
-            self.assertTrue('amandita' in _XTRIG_FUNCS)
+            self.assertTrue((m_name, f_name) in _XTRIG_FUNCS)
             # returned from cache
-            self.assertEqual(fn, get_func("amandita", temp_dir))
-            del _XTRIG_FUNCS['amandita']
-            # is not in the cache
-            self.assertFalse('amandita' in _XTRIG_FUNCS)
+            self.assertEqual(fn, get_func(m_name, f_name, temp_dir))
 
     def test_xfunction_import_error(self):
         """Test for error on importing a xtrigger function.
@@ -189,7 +189,7 @@ def test_xfunction_import_error(self):
         """
         with TemporaryDirectory() as temp_dir:
             with self.assertRaises(ModuleNotFoundError):
-                get_func("invalid-module-name", temp_dir)
+                get_func("invalid-module-name", "func-name", temp_dir)
 
     def test_xfunction_attribute_error(self):
         """Test for error on looking for an attribute in a xtrigger script."""
@@ -200,8 +200,9 @@ def test_xfunction_attribute_error(self):
             with the_answer_file.open(mode="w") as f:
                 f.write("""the_droid = lambda: 'excalibur'""")
                 f.flush()
+            f_name = "the_sword"
             with self.assertRaises(AttributeError):
-                get_func("the_sword", temp_dir)
+                get_func(f_name, f_name, temp_dir)
 
 
 @pytest.fixture

From d410f4163d5858917c22070799240cdd188d580c Mon Sep 17 00:00:00 2001
From: Hilary James Oliver <hilary.j.oliver@gmail.com>
Date: Mon, 29 Jan 2024 01:02:58 +1300
Subject: [PATCH 02/15] Update change log.

---
 changes.d/5452.md | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changes.d/5452.md

diff --git a/changes.d/5452.md b/changes.d/5452.md
new file mode 100644
index 00000000000..c9fc9b4d879
--- /dev/null
+++ b/changes.d/5452.md
@@ -0,0 +1 @@
+Support xtrigger argument validation.

From dc12e0b928f9e052e9ae609bc4485077253320db Mon Sep 17 00:00:00 2001
From: Hilary James Oliver <hilary.j.oliver@gmail.com>
Date: Mon, 29 Jan 2024 20:24:02 +1300
Subject: [PATCH 03/15] Arg validation for entry point xtriggers.

---
 changes.d/{5452.md => 5452.feat.md}      |  0
 cylc/flow/config.py                      | 37 ++++++-------
 cylc/flow/scripts/function_run.py        | 11 ++--
 cylc/flow/subprocpool.py                 | 68 ++++++++++++++----------
 cylc/flow/xtrigger_mgr.py                | 15 +++---
 cylc/flow/xtriggers/echo.py              | 20 ++++---
 cylc/flow/xtriggers/wall_clock.py        | 24 ++++++---
 setup.cfg                                | 12 +++--
 tests/functional/xtriggers/03-sequence.t |  4 +-
 tests/unit/test_subprocpool.py           | 16 +++---
 tests/unit/test_xtrigger_mgr.py          | 14 ++---
 11 files changed, 129 insertions(+), 92 deletions(-)
 rename changes.d/{5452.md => 5452.feat.md} (100%)

diff --git a/changes.d/5452.md b/changes.d/5452.feat.md
similarity index 100%
rename from changes.d/5452.md
rename to changes.d/5452.feat.md
diff --git a/cylc/flow/config.py b/cylc/flow/config.py
index 877cdb1b6da..b61edfdf771 100644
--- a/cylc/flow/config.py
+++ b/cylc/flow/config.py
@@ -83,7 +83,7 @@
 from cylc.flow.print_tree import print_tree
 from cylc.flow.simulation import configure_sim_modes
 from cylc.flow.subprocctx import SubFuncContext
-from cylc.flow.subprocpool import get_func
+from cylc.flow.subprocpool import get_xtrig_func
 from cylc.flow.task_events_mgr import (
     EventData,
     get_event_handler_data
@@ -1716,31 +1716,28 @@ def generate_triggers(self, lexpression, left_nodes, right, seq,
                     # Allow "@wall_clock" in graph as implicit zero-offset.
                     xtrig = SubFuncContext('wall_clock', 'wall_clock', [], {})
 
-            if xtrig.func_name == 'wall_clock':
-                if self.cycling_type == INTEGER_CYCLING_TYPE:
-                    raise WorkflowConfigError(
-                        "Clock xtriggers require datetime cycling:"
-                        f" {label} = {xtrig.get_signature()}"
-                    )
-                else:
-                    # Convert offset arg to kwarg for certainty later.
-                    if "offset" not in xtrig.func_kwargs:
-                        xtrig.func_kwargs["offset"] = None
-                        with suppress(IndexError):
-                            xtrig.func_kwargs["offset"] = xtrig.func_args[0]
+            if (
+                xtrig.func_name == 'wall_clock'
+                and self.cycling_type == INTEGER_CYCLING_TYPE
+            ):
+                raise WorkflowConfigError(
+                    "Clock xtriggers require datetime cycling:"
+                    f" {label} = {xtrig.get_signature()}"
+                )
+
+            # Generic xtrigger validation.
+            XtriggerManager.check_xtrigger(label, xtrig, self.fdir)
 
-            # Call the xtrigger's validate_config function if it has one.
+            # Specific xtrigger.validate(), if available.
             with suppress(AttributeError, ImportError):
-                get_func(xtrig.func_name, "validate_config", self.fdir)(
+                get_xtrig_func(xtrig.func_name, "validate", self.fdir)(
                     xtrig.func_args,
                     xtrig.func_kwargs,
                     xtrig.get_signature()
                 )
 
-            if self.xtrigger_mgr is None:
-                # Validation only.
-                XtriggerManager.validate_xtrigger(label, xtrig, self.fdir)
-            else:
+            if self.xtrigger_mgr:
+                # (not available during validation)
                 self.xtrigger_mgr.add_trig(label, xtrig, self.fdir)
 
             self.taskdefs[right].add_xtrig_label(label, seq)
@@ -2433,7 +2430,7 @@ def upgrade_clock_triggers(self):
             xtrig = SubFuncContext(label, 'wall_clock', [], {})
             xtrig.func_kwargs["offset"] = offset
             if self.xtrigger_mgr is None:
-                XtriggerManager.validate_xtrigger(label, xtrig, self.fdir)
+                XtriggerManager.check_xtrigger(label, xtrig, self.fdir)
             else:
                 self.xtrigger_mgr.add_trig(label, xtrig, self.fdir)
             # Add it to the task, for each sequence that the task appears in.
diff --git a/cylc/flow/scripts/function_run.py b/cylc/flow/scripts/function_run.py
index 7ada8805de3..63029a97782 100755
--- a/cylc/flow/scripts/function_run.py
+++ b/cylc/flow/scripts/function_run.py
@@ -18,10 +18,13 @@
 
 (This command is for internal use.)
 
-Run a Python function "<name>(*args, **kwargs)" in the process pool. It must be
-defined in a module of the same name. Positional and keyword arguments must be
-passed in as JSON strings. <src-dir> is the workflow source dir, needed to find
-local xtrigger modules.
+Run a Python xtrigger function "<name>(*args, **kwargs)" in the process pool.
+It must be in a module of the same name. Positional and keyword arguments must
+be passed in as JSON strings.
+
+Python entry points are the preferred way to make xtriggers available to the
+scheduler, but local xtriggers can be stored in <src-dir>.
+
 """
 import sys
 
diff --git a/cylc/flow/subprocpool.py b/cylc/flow/subprocpool.py
index 42782e10b1b..84206d4ebdc 100644
--- a/cylc/flow/subprocpool.py
+++ b/cylc/flow/subprocpool.py
@@ -41,7 +41,8 @@
 from cylc.flow.task_proxy import TaskProxy
 from cylc.flow.wallclock import get_current_time_string
 
-_XTRIG_FUNCS: dict = {}
+_XTRIG_MOD_CACHE: dict = {}
+_XTRIG_FUNC_CACHE: dict = {}
 
 
 def _killpg(proc, signal):
@@ -66,52 +67,61 @@ def _killpg(proc, signal):
     return True
 
 
-def get_func(mod_name, func_name, src_dir):
-    """Find and return a named function from a named module.
+def get_xtrig_mod(mod_name, src_dir):
+    """Find, cache, and return a named xtrigger module.
 
-    Can be in <src_dir>/lib/python, cylc.flow.xtriggers, or in Python path.
+    Locations checked in this order:
+    - <src_dir>/lib/python (prepend to sys.path)
+    - $CYLC_PYTHONPATH (already in sys.path)
+    - `cylc.xtriggers` entry point
 
-    These locations are checked in this order:
-    - <src_dir>/lib/python/
-    - `$CYLC_PYTHONPATH`
-    - defined via a `cylc.xtriggers` entry point for an
-      installed Python package.
+    (Check entry point last so users can override with local implementations).
 
-    Workflow source directory passed in as this is executed in an independent
-    process in the command pool and therefore doesn't know about the workflow.
+    Workflow source dir passed in - this executes in an independent subprocess.
 
     Raises:
         ImportError, if the module is not found
-        AttributeError, if the function is not found in the module
 
     """
-    if (mod_name, func_name) in _XTRIG_FUNCS:
+    if mod_name in _XTRIG_MOD_CACHE:
         # Found and cached already.
-        return _XTRIG_FUNCS[(mod_name, func_name)]
+        return _XTRIG_MOD_CACHE[mod_name]
 
     # First look in <src-dir>/lib/python.
     sys.path.insert(0, os.path.join(src_dir, 'lib', 'python'))
     try:
-        mod_by_name = __import__(mod_name, fromlist=[mod_name])
+        _XTRIG_MOD_CACHE[mod_name] = __import__(mod_name, fromlist=[mod_name])
     except ImportError:
-        # Look for xtriggers via entry_points for external sources.
-        # Do this after the lib/python and PYTHONPATH approaches to allow
-        # users to override entry_point definitions with local/custom
-        # implementations.
+        # Then entry point.
         for entry_point in iter_entry_points('cylc.xtriggers'):
-            if func_name == entry_point.name:
-                _XTRIG_FUNCS[func_name] = entry_point.load()
-                return _XTRIG_FUNCS[func_name]
-
+            if mod_name == entry_point.name:
+                _XTRIG_MOD_CACHE[mod_name] = entry_point.load()
+                return _XTRIG_MOD_CACHE[mod_name]
         # Still unable to find anything so abort
         raise
 
+    return _XTRIG_MOD_CACHE[mod_name]
+
+
+def get_xtrig_func(mod_name, func_name, src_dir):
+    """Find, cache, and return a function from an xtrigger module.
+
+    Raises:
+        ImportError, if the module is not found
+        AttributeError, if the function is not found in the module
+
+    """
+    if (mod_name, func_name) in _XTRIG_FUNC_CACHE:
+        return _XTRIG_FUNC_CACHE[(mod_name, func_name)]
+
+    mod = get_xtrig_mod(mod_name, src_dir)
+
     try:
-        _XTRIG_FUNCS[(mod_name, func_name)] = getattr(mod_by_name, func_name)
+        _XTRIG_FUNC_CACHE[(mod_name, func_name)] = getattr(mod, func_name)
     except AttributeError:
-        # Module func_name has no function func_name, nor an entry_point entry.
         raise
-    return _XTRIG_FUNCS[(mod_name, func_name)]
+
+    return _XTRIG_FUNC_CACHE[(mod_name, func_name)]
 
 
 def run_function(func_name, json_args, json_kwargs, src_dir):
@@ -128,14 +138,18 @@ def run_function(func_name, json_args, json_kwargs, src_dir):
     """
     func_args = json.loads(json_args)
     func_kwargs = json.loads(json_kwargs)
+
     # Find and import then function.
-    func = get_func(func_name, func_name, src_dir)
+    func = get_xtrig_func(func_name, func_name, src_dir)
+
     # Redirect stdout to stderr.
     orig_stdout = sys.stdout
     sys.stdout = sys.stderr
     res = func(*func_args, **func_kwargs)
+
     # Restore stdout.
     sys.stdout = orig_stdout
+
     # Write function return value as JSON to stdout.
     sys.stdout.write(json.dumps(res))
 
diff --git a/cylc/flow/xtrigger_mgr.py b/cylc/flow/xtrigger_mgr.py
index db6a3453e70..890918c49dc 100644
--- a/cylc/flow/xtrigger_mgr.py
+++ b/cylc/flow/xtrigger_mgr.py
@@ -26,7 +26,7 @@
 from cylc.flow.exceptions import XtriggerConfigError
 import cylc.flow.flags
 from cylc.flow.hostuserutil import get_user
-from cylc.flow.subprocpool import get_func
+from cylc.flow.subprocpool import get_xtrig_func
 from cylc.flow.xtriggers.wall_clock import wall_clock
 
 if TYPE_CHECKING:
@@ -240,12 +240,14 @@ def __init__(
         self.do_housekeeping = False
 
     @staticmethod
-    def validate_xtrigger(
+    def check_xtrigger(
         label: str,
         fctx: 'SubFuncContext',
         fdir: str,
     ) -> None:
-        """Validate an Xtrigger function.
+        """Generic xtrigger validation: check existence and string templates.
+
+        Xtrigger modules may also supply a specific "validate" function.
 
         Args:
             label: xtrigger label
@@ -264,7 +266,7 @@ def validate_xtrigger(
         fname: str = fctx.func_name
 
         try:
-            func = get_func(fname, fname, fdir)
+            func = get_xtrig_func(fname, fname, fdir)
         except ImportError:
             raise XtriggerConfigError(
                 label,
@@ -319,13 +321,14 @@ def validate_xtrigger(
     def add_trig(self, label: str, fctx: 'SubFuncContext', fdir: str) -> None:
         """Add a new xtrigger function.
 
-        Check the xtrigger function exists here (e.g. during validation).
+        Call check_xtrigger before this, during validation.
+
         Args:
             label: xtrigger label
             fctx: function context
             fdir: function module directory
+
         """
-        self.validate_xtrigger(label, fctx, fdir)
         self.functx_map[label] = fctx
 
     def mutate_trig(self, label, kwargs):
diff --git a/cylc/flow/xtriggers/echo.py b/cylc/flow/xtriggers/echo.py
index e3c5880cab0..cdc7a73e228 100644
--- a/cylc/flow/xtriggers/echo.py
+++ b/cylc/flow/xtriggers/echo.py
@@ -16,12 +16,11 @@
 
 """A Cylc xtrigger function."""
 
-from contextlib import suppress
+from cylc.flow.exceptions import WorkflowConfigError
 
 
 def echo(*args, **kwargs):
-    """Prints args to stdout and return success only if kwargs['succeed']
-    is True.
+    """Print arguments to stdout, return kwargs['succeed'] and kwargs.
 
     This may be a useful aid to understanding how xtriggers work.
 
@@ -31,7 +30,14 @@ def echo(*args, **kwargs):
     """
     print("echo: ARGS:", args)
     print("echo: KWARGS:", kwargs)
-    result = False
-    with suppress(KeyError):
-        result = kwargs["succeed"] is True
-    return result, kwargs
+
+    return kwargs["succeed"], kwargs
+
+
+def validate(f_args, f_kwargs, f_signature):
+    try:
+        assert type(f_kwargs["succeed"]) is bool
+    except (KeyError, AssertionError):
+        raise WorkflowConfigError(
+            f"xtrigger requires 'succeed=True/False': {f_signature}"
+        )
diff --git a/cylc/flow/xtriggers/wall_clock.py b/cylc/flow/xtriggers/wall_clock.py
index e12a17ce8be..5fbbd821b09 100644
--- a/cylc/flow/xtriggers/wall_clock.py
+++ b/cylc/flow/xtriggers/wall_clock.py
@@ -31,10 +31,10 @@ def wall_clock(trigger_time=None):
     return time() > trigger_time
 
 
-def validate_config(f_args, f_kwargs, f_signature):
+def validate(f_args, f_kwargs, f_signature):
     """Validate and manipulate args parsed from the workflow config.
 
-    wall_clock()  # zero offset
+    wall_clock()  # infer zero interval
     wall_clock(PT1H)
     wall_clock(offset=PT1H)
 
@@ -43,18 +43,30 @@ def validate_config(f_args, f_kwargs, f_signature):
     If f_args used, convert to f_kwargs for clarity.
 
     """
+
     n_args = len(f_args)
     n_kwargs = len(f_kwargs)
 
     if n_args + n_kwargs > 1:
-        raise WorkflowConfigError(f"xtrigger: too many args: {f_signature}")
+        raise WorkflowConfigError(f"Too many args: {f_signature}")
+
+    if n_kwargs:
+        # sole kwarg must be "offset"
+        kw = next(iter(f_kwargs))
+        if kw != "offset":
+            raise WorkflowConfigError(f"Illegal arg '{kw}': {f_signature}")
 
-    if n_args:
+    elif n_args:
+        # convert to kwarg
         f_kwargs["offset"] = f_args[0]
-    elif not n_kwargs:
+        del f_args[0]
+
+    else:
+        # no args, infer zero interval
         f_kwargs["offset"] = "P0Y"
 
+    # must be a valid interval
     try:
         interval_parse(f_kwargs["offset"])
     except ValueError:
-        raise WorkflowConfigError(f"xtrigger: invalid offset: {f_signature}")
+        raise WorkflowConfigError(f": Invalid offset: {f_signature}")
diff --git a/setup.cfg b/setup.cfg
index 7db9eb8bf4e..eca9140b181 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -215,12 +215,14 @@ cylc.main_loop =
 cylc.pre_configure =
 cylc.post_install =
     log_vc_info = cylc.flow.install_plugins.log_vc_info:main
-# NOTE: Built-in xtriggers
+# NOTE: Built-in xtrigger modules
+#   - must contain a function (the xtrigger) with the same name as the module
+#   - and may contain a "validate" function to check arguments
 cylc.xtriggers =
-    echo = cylc.flow.xtriggers.echo:echo
-    wall_clock = cylc.flow.xtriggers.wall_clock:wall_clock
-    workflow_state = cylc.flow.xtriggers.workflow_state:workflow_state
-    xrandom = cylc.flow.xtriggers.xrandom:xrandom
+    echo = cylc.flow.xtriggers.echo
+    wall_clock = cylc.flow.xtriggers.wall_clock
+    workflow_state = cylc.flow.xtriggers.workflow_state
+    xrandom = cylc.flow.xtriggers.xrandom
 
 [bdist_rpm]
 requires =
diff --git a/tests/functional/xtriggers/03-sequence.t b/tests/functional/xtriggers/03-sequence.t
index f45af9b1caf..63c360f66c5 100644
--- a/tests/functional/xtriggers/03-sequence.t
+++ b/tests/functional/xtriggers/03-sequence.t
@@ -31,7 +31,7 @@ init_workflow "${TEST_NAME_BASE}" << '__FLOW_CONFIG__'
    final cycle point = +P1Y
    [[xtriggers]]
        e1 = echo(name='bob', succeed=True)
-       e2 = echo(name='alice')
+       e2 = echo(name='alice', succeed=False)
    [[dependencies]]
       [[[R1]]]
           graph = "start"
@@ -55,7 +55,7 @@ cylc show "${WORKFLOW_NAME}//2026/foo" | grep -E '^  - xtrigger' > 2026.foo.log
 
 # 2026/foo should get only xtrigger e2.
 cmp_ok 2026.foo.log - <<__END__
-  - xtrigger "e2 = echo(name=alice)"
+  - xtrigger "e2 = echo(name=alice, succeed=False)"
 __END__
 
 cylc stop --now --max-polls=10 --interval=2 "${WORKFLOW_NAME}"
diff --git a/tests/unit/test_subprocpool.py b/tests/unit/test_subprocpool.py
index 02ee540fd64..9e2d4af212b 100644
--- a/tests/unit/test_subprocpool.py
+++ b/tests/unit/test_subprocpool.py
@@ -29,7 +29,7 @@
 from cylc.flow.cycling.iso8601 import ISO8601Point
 from cylc.flow.task_events_mgr import TaskJobLogsRetrieveContext
 from cylc.flow.subprocctx import SubProcContext
-from cylc.flow.subprocpool import SubProcPool, _XTRIG_FUNCS, get_func
+from cylc.flow.subprocpool import SubProcPool, _XTRIG_FUNC_CACHE, _XTRIG_MOD_CACHE, get_xtrig_func
 from cylc.flow.task_proxy import TaskProxy
 
 
@@ -156,7 +156,7 @@ def test_xfunction(self):
                 f.write("""the_answer = lambda: 42""")
                 f.flush()
                 f_name = "the_answer"
-            fn = get_func(f_name, f_name, temp_dir)
+            fn = get_xtrig_func(f_name, f_name, temp_dir)
             result = fn()
             self.assertEqual(42, result)
 
@@ -170,15 +170,15 @@ def test_xfunction_cache(self):
                 f.write("""choco = lambda: 'chocolate'""")
                 f.flush()
             m_name = "amandita"  # module
-            f_name =  "choco"  # function
-            fn = get_func(m_name, f_name, temp_dir)
+            f_name = "choco"  # function
+            fn = get_xtrig_func(m_name, f_name, temp_dir)
             result = fn()
             self.assertEqual('chocolate', result)
 
             # is in the cache
-            self.assertTrue((m_name, f_name) in _XTRIG_FUNCS)
+            self.assertTrue((m_name, f_name) in _XTRIG_FUNC_CACHE)
             # returned from cache
-            self.assertEqual(fn, get_func(m_name, f_name, temp_dir))
+            self.assertEqual(fn, get_xtrig_func(m_name, f_name, temp_dir))
 
     def test_xfunction_import_error(self):
         """Test for error on importing a xtrigger function.
@@ -189,7 +189,7 @@ def test_xfunction_import_error(self):
         """
         with TemporaryDirectory() as temp_dir:
             with self.assertRaises(ModuleNotFoundError):
-                get_func("invalid-module-name", "func-name", temp_dir)
+                get_xtrig_func("invalid-module-name", "func-name", temp_dir)
 
     def test_xfunction_attribute_error(self):
         """Test for error on looking for an attribute in a xtrigger script."""
@@ -202,7 +202,7 @@ def test_xfunction_attribute_error(self):
                 f.flush()
             f_name = "the_sword"
             with self.assertRaises(AttributeError):
-                get_func(f_name, f_name, temp_dir)
+                get_xtrig_func(f_name, f_name, temp_dir)
 
 
 @pytest.fixture
diff --git a/tests/unit/test_xtrigger_mgr.py b/tests/unit/test_xtrigger_mgr.py
index bba77c1f7f1..9b497d8e5ab 100644
--- a/tests/unit/test_xtrigger_mgr.py
+++ b/tests/unit/test_xtrigger_mgr.py
@@ -44,7 +44,7 @@ def test_extract_templates():
     )
 
 
-def test_add_xtrigger(xtrigger_mgr):
+def test_check_xtrigger(xtrigger_mgr):
     """Test for adding an xtrigger."""
     xtrig = SubFuncContext(
         label="echo",
@@ -68,7 +68,7 @@ def test_add_xtrigger_with_params(xtrigger_mgr):
     assert xtrig == xtrigger_mgr.functx_map["xtrig"]
 
 
-def test_add_xtrigger_with_unknown_params(xtrigger_mgr):
+def test_check_xtrigger_with_unknown_params(xtrigger_mgr):
     """Test for adding an xtrigger with an unknown parameter.
 
     The XTriggerManager contains a list of specific parameters that are
@@ -87,10 +87,10 @@ def test_add_xtrigger_with_unknown_params(xtrigger_mgr):
         func_kwargs={"location": "soweto"}
     )
     with pytest.raises(XtriggerConfigError):
-        xtrigger_mgr.add_trig("xtrig", xtrig, 'fdir')
+        xtrigger_mgr.check_xtrigger("xtrig", xtrig, 'fdir')
 
 
-def test_add_xtrigger_with_deprecated_params(xtrigger_mgr, caplog):
+def test_check_xtrigger_with_deprecated_params(xtrigger_mgr, caplog):
     """It should flag deprecated template variables."""
     xtrig = SubFuncContext(
         label="echo",
@@ -99,7 +99,7 @@ def test_add_xtrigger_with_deprecated_params(xtrigger_mgr, caplog):
         func_kwargs={"location": "soweto"}
     )
     caplog.set_level(logging.WARNING, CYLC_LOG)
-    xtrigger_mgr.add_trig("xtrig", xtrig, 'fdir')
+    xtrigger_mgr.check_xtrigger("xtrig", xtrig, 'fdir')
     assert caplog.messages == [
         'Xtrigger "xtrig" uses deprecated template variables: suite_name'
     ]
@@ -139,7 +139,7 @@ def test_housekeeping_nothing_satisfied(xtrigger_mgr):
 def test_housekeeping_with_xtrigger_satisfied(xtrigger_mgr):
     """The housekeeping method makes sure only satisfied xtrigger function
     are kept."""
-    xtrigger_mgr.validate_xtrigger = lambda *a, **k: True  # Ignore validation
+    xtrigger_mgr.check_xtrigger = lambda *a, **k: True  # Ignore validation
     xtrig = SubFuncContext(
         label="get_name",
         func_name="get_name",
@@ -171,7 +171,7 @@ def test_housekeeping_with_xtrigger_satisfied(xtrigger_mgr):
 
 def test__call_xtriggers_async(xtrigger_mgr):
     """Test _call_xtriggers_async"""
-    xtrigger_mgr.validate_xtrigger = lambda *a, **k: True  # Ignore validation
+    xtrigger_mgr.check_xtrigger = lambda *a, **k: True  # Ignore validation
     # the echo1 xtrig (not satisfied)
     echo1_xtrig = SubFuncContext(
         label="echo1",

From 4a5eac69ae51011770ed8d0015b8e04553dc8fc9 Mon Sep 17 00:00:00 2001
From: Hilary James Oliver <hilary.j.oliver@gmail.com>
Date: Tue, 30 Jan 2024 17:42:30 +1300
Subject: [PATCH 04/15] Add a function docstring.

---
 cylc/flow/xtriggers/echo.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/cylc/flow/xtriggers/echo.py b/cylc/flow/xtriggers/echo.py
index cdc7a73e228..f955afffb31 100644
--- a/cylc/flow/xtriggers/echo.py
+++ b/cylc/flow/xtriggers/echo.py
@@ -35,6 +35,12 @@ def echo(*args, **kwargs):
 
 
 def validate(f_args, f_kwargs, f_signature):
+    """
+    Validate the xtrigger function arguments parsed from the workflow config.
+
+    This is separate from the xtrigger to allow parse-time validation.
+
+    """
     try:
         assert type(f_kwargs["succeed"]) is bool
     except (KeyError, AssertionError):

From da3721406692a330dddfcf7a101defb526c4f7a9 Mon Sep 17 00:00:00 2001
From: Hilary James Oliver <hilary.j.oliver@gmail.com>
Date: Tue, 30 Jan 2024 17:49:57 +1300
Subject: [PATCH 05/15] Code review tweaks.

---
 cylc/flow/xtriggers/echo.py       | 6 ++----
 cylc/flow/xtriggers/wall_clock.py | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/cylc/flow/xtriggers/echo.py b/cylc/flow/xtriggers/echo.py
index f955afffb31..adfb9bc9f83 100644
--- a/cylc/flow/xtriggers/echo.py
+++ b/cylc/flow/xtriggers/echo.py
@@ -41,9 +41,7 @@ def validate(f_args, f_kwargs, f_signature):
     This is separate from the xtrigger to allow parse-time validation.
 
     """
-    try:
-        assert type(f_kwargs["succeed"]) is bool
-    except (KeyError, AssertionError):
+    if "succeed" not in f_kwargs or not type(f_kwargs["succeed"]) is bool:
         raise WorkflowConfigError(
-            f"xtrigger requires 'succeed=True/False': {f_signature}"
+            f"Requires 'succeed=True/False' arg: {f_signature}"
         )
diff --git a/cylc/flow/xtriggers/wall_clock.py b/cylc/flow/xtriggers/wall_clock.py
index 5fbbd821b09..0e0c40c733c 100644
--- a/cylc/flow/xtriggers/wall_clock.py
+++ b/cylc/flow/xtriggers/wall_clock.py
@@ -69,4 +69,4 @@ def validate(f_args, f_kwargs, f_signature):
     try:
         interval_parse(f_kwargs["offset"])
     except ValueError:
-        raise WorkflowConfigError(f": Invalid offset: {f_signature}")
+        raise WorkflowConfigError(f"Invalid offset: {f_signature}")

From 6fea50f1ec2c03b9592ae55c06723903800f0541 Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Wed, 3 Jan 2024 16:24:12 +0000
Subject: [PATCH 06/15] Add integration tests for xtrigger validation, which
 provides simple examples for each of the built in xtriggers. - Xrandom
 validate function - Init test xrandom validate function - Add unit tests for
 validation of built in xtriggers

---
 cylc/flow/xtriggers/echo.py             |   2 +-
 cylc/flow/xtriggers/wall_clock.py       |   2 +-
 cylc/flow/xtriggers/xrandom.py          |  47 +++++++++
 tests/integration/test_config.py        | 122 ++++++++++++++++++++++++
 tests/unit/xtriggers/test_echo.py       |  37 +++++++
 tests/unit/xtriggers/test_wall_clock.py |  51 ++++++++++
 tests/unit/xtriggers/test_xrandom.py    |  42 ++++++++
 7 files changed, 301 insertions(+), 2 deletions(-)
 create mode 100644 tests/unit/xtriggers/test_echo.py
 create mode 100644 tests/unit/xtriggers/test_wall_clock.py
 create mode 100644 tests/unit/xtriggers/test_xrandom.py

diff --git a/cylc/flow/xtriggers/echo.py b/cylc/flow/xtriggers/echo.py
index adfb9bc9f83..a8e7f060db3 100644
--- a/cylc/flow/xtriggers/echo.py
+++ b/cylc/flow/xtriggers/echo.py
@@ -41,7 +41,7 @@ def validate(f_args, f_kwargs, f_signature):
     This is separate from the xtrigger to allow parse-time validation.
 
     """
-    if "succeed" not in f_kwargs or not type(f_kwargs["succeed"]) is bool:
+    if "succeed" not in f_kwargs or not isinstance(f_kwargs["succeed"], bool):
         raise WorkflowConfigError(
             f"Requires 'succeed=True/False' arg: {f_signature}"
         )
diff --git a/cylc/flow/xtriggers/wall_clock.py b/cylc/flow/xtriggers/wall_clock.py
index 0e0c40c733c..c18ea2e5d2b 100644
--- a/cylc/flow/xtriggers/wall_clock.py
+++ b/cylc/flow/xtriggers/wall_clock.py
@@ -68,5 +68,5 @@ def validate(f_args, f_kwargs, f_signature):
     # must be a valid interval
     try:
         interval_parse(f_kwargs["offset"])
-    except ValueError:
+    except (ValueError, AttributeError):
         raise WorkflowConfigError(f"Invalid offset: {f_signature}")
diff --git a/cylc/flow/xtriggers/xrandom.py b/cylc/flow/xtriggers/xrandom.py
index 53d98865ef1..2fe5ebe1d71 100644
--- a/cylc/flow/xtriggers/xrandom.py
+++ b/cylc/flow/xtriggers/xrandom.py
@@ -17,6 +17,8 @@
 from random import random, randint
 from time import sleep
 
+from cylc.flow.exceptions import WorkflowConfigError
+
 
 COLORS = ["red", "orange", "yellow", "green", "blue", "indigo", "violet"]
 SIZES = ["tiny", "small", "medium", "large", "huge", "humongous"]
@@ -89,3 +91,48 @@ def xrandom(percent, secs=0, _=None):
             'SIZE': SIZES[randint(0, len(SIZES) - 1)]  # nosec
         }
     return satisfied, results
+
+
+def validate(f_args, f_kwargs, f_signature):
+    """Validate and manipulate args parsed from the workflow config.
+
+    percent: - 0 ≤ x ≤ 100
+    secs: An int.
+
+    If f_args used, convert to f_kwargs for clarity.
+
+    """
+    n_args = len(f_args)
+    n_kwargs = len(f_kwargs)
+
+    if n_args + n_kwargs > 3:
+        raise WorkflowConfigError(f"Too many args: {f_signature}")
+
+    if n_args != 1:
+        raise WorkflowConfigError(f"Wrong number of args: {f_signature}")
+
+    if n_kwargs:
+        # kwargs must be "secs" and "_"
+        kw = next(iter(f_kwargs))
+        if kw not in ("secs", "_"):
+            raise WorkflowConfigError(f"Illegal arg '{kw}': {f_signature}")
+
+    # convert to kwarg
+    f_kwargs["percent"] = f_args[0]
+    del f_args[0]
+
+    try:
+        percent = f_kwargs['percent']
+        assert isinstance(percent, (float, int))
+        assert percent >= 0
+        assert percent <= 100
+    except AssertionError:
+        raise WorkflowConfigError(
+            f"'percent' should be a float between 0 and 100: {f_signature}")
+
+    try:
+        secs = f_kwargs['secs']
+        assert isinstance(secs, int)
+    except AssertionError:
+        raise WorkflowConfigError(
+            f"'secs' should be an integer: {f_signature}")
diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py
index acf24d17eaf..df93d50f555 100644
--- a/tests/integration/test_config.py
+++ b/tests/integration/test_config.py
@@ -16,12 +16,18 @@
 
 from pathlib import Path
 import sqlite3
+from typing import TYPE_CHECKING
 import pytest
 
 from cylc.flow.exceptions import ServiceFileError, WorkflowConfigError
 from cylc.flow.parsec.exceptions import ListValueError
 from cylc.flow.pathutil import get_workflow_run_pub_db_path
 
+if TYPE_CHECKING:
+    from types import Any
+
+    Fixture = Any
+
 
 @pytest.mark.parametrize(
     'task_name,valid', [
@@ -341,3 +347,119 @@ def test_validate_incompatible_db(one_conf, flow, validate):
     finally:
         conn.close()
     assert tables == ['suite_params']
+
+
+def test_xtrig_validation_wall_clock(
+    flow: 'Fixture',
+    validate: 'Fixture',
+):
+    """If an xtrigger module has a `validate_config` it is called.
+
+    https://github.com/cylc/cylc-flow/issues/5448
+    """
+    id_ = flow({
+        'scheduler': {'allow implicit tasks': True},
+        'scheduling': {
+            'initial cycle point': '1012',
+            'xtriggers': {'myxt': 'wall_clock(offset=PT755MH)'},
+            'graph': {'R1': '@myxt => foo'},
+        }
+    })
+    with pytest.raises(
+        WorkflowConfigError,
+        match=r'Invalid offset: wall_clock\(offset=PT755MH\)'
+    ):
+        validate(id_)
+
+
+def test_xtrig_validation_echo(
+    flow: 'Fixture',
+    validate: 'Fixture',
+):
+    """If an xtrigger module has a `validate_config` it is called.
+
+    https://github.com/cylc/cylc-flow/issues/5448
+    """
+    id_ = flow({
+        'scheduler': {'allow implicit tasks': True},
+        'scheduling': {
+            'initial cycle point': '1012',
+            'xtriggers': {'myxt': 'echo()'},
+            'graph': {'R1': '@myxt => foo'},
+        }
+    })
+    with pytest.raises(
+        WorkflowConfigError,
+        match=r'Requires \'succeed=True/False\' arg: echo()'
+    ):
+        validate(id_)
+
+
+def test_xtrig_validation_xrandom(
+    flow: 'Fixture',
+    validate: 'Fixture',
+):
+    """If an xtrigger module has a `validate_config` it is called.
+
+    https://github.com/cylc/cylc-flow/issues/5448
+    """
+    id_ = flow({
+        'scheduler': {'allow implicit tasks': True},
+        'scheduling': {
+            'initial cycle point': '1012',
+            'xtriggers': {'myxt': 'xrandom()'},
+            'graph': {'R1': '@myxt => foo'},
+        }
+    })
+    with pytest.raises(
+        WorkflowConfigError,
+        match=r'Wrong number of args: xrandom\(\)'
+    ):
+        validate(id_)
+
+
+def test_xtrig_validation_custom(
+    flow: 'Fixture',
+    validate: 'Fixture',
+    monkeypatch: 'Fixture',
+):
+    """If an xtrigger module has a `validate_config`
+    an exception is raised if that validate function fails.
+
+    https://github.com/cylc/cylc-flow/issues/5448
+
+    Rather than create our own xtrigger module on disk
+    and attempt to trigger a validation failure we
+    mock our own exception, xtrigger and xtrigger
+    validation functions and inject these into the
+    appropriate locations:
+    """
+    GreenExc = type('Green', (Exception,), {})
+
+    def kustom_mock(suite):
+        return True, {}
+
+    def kustom_validate(args, kwargs, sig):
+        raise GreenExc('This is only a test.')
+
+    monkeypatch.setattr(
+        'cylc.flow.xtrigger_mgr.get_xtrig_func',
+        lambda *args: kustom_mock,
+    )
+    monkeypatch.setattr(
+        'cylc.flow.config.get_xtrig_func',
+        lambda *args: kustom_validate if "validate" in args else ''
+    )
+
+    id_ = flow({
+        'scheduler': {'allow implicit tasks': True},
+        'scheduling': {
+            'initial cycle point': '1012',
+            'xtriggers': {'myxt': 'kustom_xt(feature=42)'},
+            'graph': {'R1': '@myxt => foo'},
+        }
+    })
+
+    Path(id_)
+    with pytest.raises(GreenExc, match=r'This is only a test.'):
+        validate(id_)
diff --git a/tests/unit/xtriggers/test_echo.py b/tests/unit/xtriggers/test_echo.py
new file mode 100644
index 00000000000..b4bc104adac
--- /dev/null
+++ b/tests/unit/xtriggers/test_echo.py
@@ -0,0 +1,37 @@
+# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
+# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from cylc.flow.exceptions import WorkflowConfigError
+from cylc.flow.xtriggers.echo import validate
+import pytest
+from pytest import param
+
+
+def test_validate_good_path():
+    assert validate(
+        [], {'succeed': False, 'egg': 'fried', 'potato': 'baked'}, 'succeed'
+    ) is None
+
+
+@pytest.mark.parametrize(
+    'args, kwargs', (
+        param([False], {}, id='no-kwarg'),
+        param([], {'spud': 'mashed'}, id='no-succeed-kwarg'),
+    )
+)
+def test_validate_exceptions(args, kwargs):
+    with pytest.raises(WorkflowConfigError, match='^Requires'):
+        validate(args, kwargs, 'blah')
\ No newline at end of file
diff --git a/tests/unit/xtriggers/test_wall_clock.py b/tests/unit/xtriggers/test_wall_clock.py
new file mode 100644
index 00000000000..6dc1cb2b6f8
--- /dev/null
+++ b/tests/unit/xtriggers/test_wall_clock.py
@@ -0,0 +1,51 @@
+# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
+# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from cylc.flow.exceptions import WorkflowConfigError
+from cylc.flow.xtriggers.wall_clock import validate
+from metomi.isodatetime.parsers import DurationParser
+import pytest
+from pytest import param
+
+
+@pytest.fixture
+def monkeypatch_interval_parser(monkeypatch):
+    """Interval parse only works normally if a WorkflowSpecifics
+    object identify the parser to be used.
+    """
+    monkeypatch.setattr(
+        'cylc.flow.xtriggers.wall_clock.interval_parse',
+        DurationParser().parse
+    )
+
+
+def test_validate_good_path(monkeypatch_interval_parser):
+    assert validate([], {}, 'Alles Gut') is None
+
+
+@pytest.mark.parametrize(
+    'args, kwargs, err', (
+        param([1, 2], {}, "^Too", id='too-many-args'),
+        param([], {'egg': 12}, "^Illegal", id='illegal-arg'),
+        param([1], {}, "^Invalid", id='invalid-offset-int'),
+        param([], {'offset': 'Zaphod'}, "^Invalid", id='invalid-offset-str'),
+    )
+)
+def test_validate_exceptions(
+    monkeypatch_interval_parser, args, kwargs, err
+):
+    with pytest.raises(WorkflowConfigError, match=err):
+        validate(args, kwargs, 'Alles Gut')
diff --git a/tests/unit/xtriggers/test_xrandom.py b/tests/unit/xtriggers/test_xrandom.py
new file mode 100644
index 00000000000..50dacfebcc2
--- /dev/null
+++ b/tests/unit/xtriggers/test_xrandom.py
@@ -0,0 +1,42 @@
+# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
+# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import pytest
+from pytest import param
+
+from cylc.flow.xtriggers.xrandom import validate
+from cylc.flow.exceptions import WorkflowConfigError
+
+
+def test_validate_good_path():
+    assert validate([1], {'secs': 0, '_': 'HelloWorld'}, 'good_path') is None
+
+
+@pytest.mark.parametrize(
+    'args, kwargs, err', (
+        param([100], {'f': 1.1, 'b': 1, 'x': 2}, 'Too', id='too-many-args'),
+        param([], {}, 'Wrong number', id='too-few-args'),
+        param(['foo'], {}, '\'percent', id='percent-not-numeric'),
+        param([101], {}, '\'percent', id='percent>100'),
+        param([-1], {}, '\'percent', id='percent<0'),
+        param([100], {'egg': 1}, 'Illegal', id='invalid-kwarg'),
+        param([100], {'secs': 1.1}, "'secs'", id='secs-not-int'),
+    )
+)
+def test_validate_exceptions(args, kwargs, err):
+    """Illegal args and kwargs cause a WorkflowConfigError raised."""
+    with pytest.raises(WorkflowConfigError, match=f'^{err}'):
+        validate(args, kwargs, 'blah')

From ea50b5379355ed2046dd6094021c775b5551e975 Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Wed, 31 Jan 2024 12:49:18 +0000
Subject: [PATCH 07/15] loosen xargs validation checks

---
 cylc/flow/xtriggers/xrandom.py | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/cylc/flow/xtriggers/xrandom.py b/cylc/flow/xtriggers/xrandom.py
index 2fe5ebe1d71..6e952f4ae97 100644
--- a/cylc/flow/xtriggers/xrandom.py
+++ b/cylc/flow/xtriggers/xrandom.py
@@ -108,7 +108,7 @@ def validate(f_args, f_kwargs, f_signature):
     if n_args + n_kwargs > 3:
         raise WorkflowConfigError(f"Too many args: {f_signature}")
 
-    if n_args != 1:
+    if n_args + n_kwargs < 1:
         raise WorkflowConfigError(f"Wrong number of args: {f_signature}")
 
     if n_kwargs:
@@ -123,16 +123,17 @@ def validate(f_args, f_kwargs, f_signature):
 
     try:
         percent = f_kwargs['percent']
+        percent = float(percent)
         assert isinstance(percent, (float, int))
         assert percent >= 0
         assert percent <= 100
-    except AssertionError:
+    except (AssertionError, ValueError):
         raise WorkflowConfigError(
             f"'percent' should be a float between 0 and 100: {f_signature}")
 
     try:
-        secs = f_kwargs['secs']
+        secs = f_kwargs.get('secs', 0)
         assert isinstance(secs, int)
-    except AssertionError:
+    except (AssertionError, ValueError):
         raise WorkflowConfigError(
             f"'secs' should be an integer: {f_signature}")

From b543af705b9193b949dfdb15990df7ac1d6a5a91 Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Wed, 31 Jan 2024 13:30:36 +0000
Subject: [PATCH 08/15] avoid use of assert

---
 cylc/flow/xtriggers/xrandom.py | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/cylc/flow/xtriggers/xrandom.py b/cylc/flow/xtriggers/xrandom.py
index 6e952f4ae97..a015dd531ad 100644
--- a/cylc/flow/xtriggers/xrandom.py
+++ b/cylc/flow/xtriggers/xrandom.py
@@ -121,19 +121,32 @@ def validate(f_args, f_kwargs, f_signature):
     f_kwargs["percent"] = f_args[0]
     del f_args[0]
 
+    should_raise = False
+
     try:
         percent = f_kwargs['percent']
         percent = float(percent)
-        assert isinstance(percent, (float, int))
-        assert percent >= 0
-        assert percent <= 100
-    except (AssertionError, ValueError):
+    except ValueError:
+        should_raise = True
+    else:
+        if (
+            not isinstance(percent, (float, int))
+            or percent < 0
+            or percent > 100
+        ):
+            should_raise = True
+    if should_raise:
         raise WorkflowConfigError(
             f"'percent' should be a float between 0 and 100: {f_signature}")
 
     try:
         secs = f_kwargs.get('secs', 0)
-        assert isinstance(secs, int)
-    except (AssertionError, ValueError):
+    except ValueError:
+        should_raise = True
+    else:
+        if not isinstance(secs, int):
+            should_raise = True
+
+    if should_raise:
         raise WorkflowConfigError(
             f"'secs' should be an integer: {f_signature}")

From 26bc68460a91eeeb55c226b56d2f3d4ef9ebe5a1 Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Thu, 1 Feb 2024 15:55:51 +0000
Subject: [PATCH 09/15] make docstrings play nicely with Napoleon

---
 cylc/flow/xtriggers/echo.py    | 17 +++++++++++++++--
 cylc/flow/xtriggers/xrandom.py | 30 ++++++++++++++++++++----------
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/cylc/flow/xtriggers/echo.py b/cylc/flow/xtriggers/echo.py
index a8e7f060db3..2e964156b83 100644
--- a/cylc/flow/xtriggers/echo.py
+++ b/cylc/flow/xtriggers/echo.py
@@ -18,14 +18,26 @@
 
 from cylc.flow.exceptions import WorkflowConfigError
 
+from typing import Tuple
 
-def echo(*args, **kwargs):
+
+def echo(*args, **kwargs) -> Tuple:
     """Print arguments to stdout, return kwargs['succeed'] and kwargs.
 
     This may be a useful aid to understanding how xtriggers work.
 
+    Args:
+        succeed: Set the succeess of failure of this xtrigger.
+        *args: Print to stdout.
+        **kwargs: Print to stdout, and return as output.
+
+    Examples:
+
+        >>> echo('Breakfast Time', succeed=True, egg='poached')
+        True, {'succeed': True, 'egg': 'poached'}
+
     Returns
-        tuple: (True/False, kwargs)
+        (True/False, kwargs)
 
     """
     print("echo: ARGS:", args)
@@ -35,6 +47,7 @@ def echo(*args, **kwargs):
 
 
 def validate(f_args, f_kwargs, f_signature):
+
     """
     Validate the xtrigger function arguments parsed from the workflow config.
 
diff --git a/cylc/flow/xtriggers/xrandom.py b/cylc/flow/xtriggers/xrandom.py
index a015dd531ad..f48789ce152 100644
--- a/cylc/flow/xtriggers/xrandom.py
+++ b/cylc/flow/xtriggers/xrandom.py
@@ -16,41 +16,50 @@
 
 from random import random, randint
 from time import sleep
+from typing import TYPE_CHECKING
 
 from cylc.flow.exceptions import WorkflowConfigError
 
 
+if TYPE_CHECKING:
+    from typing import Any, Dict, Tuple
+
+
 COLORS = ["red", "orange", "yellow", "green", "blue", "indigo", "violet"]
 SIZES = ["tiny", "small", "medium", "large", "huge", "humongous"]
 
 
-def xrandom(percent, secs=0, _=None):
+def xrandom(
+    percent: float, secs: int = 0, _: 'Any' = None
+) -> 'Tuple[bool, Dict]':
     """Random xtrigger, with configurable sleep and percent success.
 
-    Sleep for ``sec`` seconds, and report satisfied with ~``percent``
+    Sleep for ``sec`` seconds, and report satisfied with ``percent``
     likelihood.
 
     The ``_`` argument is not used in the function code, but can be used to
     specialize the function signature to cycle point or task.
 
     Args:
-        percent (float):
+        percent:
             Percent likelihood of passing.
-        secs (int):
+        secs:
             Seconds to sleep before starting the trigger.
-        _ (object):
+        _:
             Used to allow users to specialize the trigger with extra
             parameters.
 
     Examples:
         If the percent is zero, it returns that the trigger condition was
         not satisfied, and an empty dictionary.
+
         >>> xrandom(0, 0)
         (False, {})
 
         If the percent is not zero, but the random percent success is not met,
         then it also returns that the trigger condition was not satisfied,
         and an empty dictionary.
+
         >>> import sys
         >>> mocked_random = lambda: 0.3
         >>> sys.modules[__name__].random = mocked_random
@@ -60,6 +69,7 @@ def xrandom(percent, secs=0, _=None):
         Finally, if the percent is not zero, and the random percent success is
         met, then it returns that the trigger condition was satisfied, and a
         dictionary containing random color and size as result.
+
         >>> import sys
         >>> mocked_random = lambda: 0.9
         >>> sys.modules[__name__].random = mocked_random
@@ -69,17 +79,17 @@ def xrandom(percent, secs=0, _=None):
         (True, {'COLOR': 'orange', 'SIZE': 'small'})
 
     Returns:
-        tuple: (satisfied, results)
+        Tuple, containing:
 
-        satisfied (bool):
+        satisfied:
             True if ``satisfied`` else ``False``.
-        results (dict):
+        results:
             A dictionary containing the following keys:
 
             ``COLOR``
-                A random color (e.g. red, orange, ...)
+                A random color (e.g. red, orange, ...).
             ``SIZE``
-                A random size (e.g. small, medium, ...) as the result.
+                A random size (e.g. small, medium, ...).
 
     """
     sleep(float(secs))

From 3013d2129f130f5af382e0980a718010b5cc0e02 Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Thu, 1 Feb 2024 16:24:51 +0000
Subject: [PATCH 10/15] fix doctest

---
 cylc/flow/xtriggers/echo.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cylc/flow/xtriggers/echo.py b/cylc/flow/xtriggers/echo.py
index 2e964156b83..ec7aa63f3c4 100644
--- a/cylc/flow/xtriggers/echo.py
+++ b/cylc/flow/xtriggers/echo.py
@@ -34,7 +34,9 @@ def echo(*args, **kwargs) -> Tuple:
     Examples:
 
         >>> echo('Breakfast Time', succeed=True, egg='poached')
-        True, {'succeed': True, 'egg': 'poached'}
+        echo: ARGS: ('Breakfast Time',)
+        echo: KWARGS: {'succeed': True, 'egg': 'poached'}
+        (True, {'succeed': True, 'egg': 'poached'})
 
     Returns
         (True/False, kwargs)

From 690fd5dcf60ba15c479b62d3b5e49e48ad55092f Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Fri, 2 Feb 2024 08:35:30 +0000
Subject: [PATCH 11/15] spelling fixes

---
 cylc/flow/xtriggers/echo.py    | 2 +-
 cylc/flow/xtriggers/xrandom.py | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cylc/flow/xtriggers/echo.py b/cylc/flow/xtriggers/echo.py
index ec7aa63f3c4..0297545e935 100644
--- a/cylc/flow/xtriggers/echo.py
+++ b/cylc/flow/xtriggers/echo.py
@@ -27,7 +27,7 @@ def echo(*args, **kwargs) -> Tuple:
     This may be a useful aid to understanding how xtriggers work.
 
     Args:
-        succeed: Set the succeess of failure of this xtrigger.
+        succeed: Set the success of failure of this xtrigger.
         *args: Print to stdout.
         **kwargs: Print to stdout, and return as output.
 
diff --git a/cylc/flow/xtriggers/xrandom.py b/cylc/flow/xtriggers/xrandom.py
index f48789ce152..56bbbe7668c 100644
--- a/cylc/flow/xtriggers/xrandom.py
+++ b/cylc/flow/xtriggers/xrandom.py
@@ -68,7 +68,7 @@ def xrandom(
 
         Finally, if the percent is not zero, and the random percent success is
         met, then it returns that the trigger condition was satisfied, and a
-        dictionary containing random color and size as result.
+        dictionary containing random colour and size as result.
 
         >>> import sys
         >>> mocked_random = lambda: 0.9
@@ -87,7 +87,7 @@ def xrandom(
             A dictionary containing the following keys:
 
             ``COLOR``
-                A random color (e.g. red, orange, ...).
+                A random colour (e.g. red, orange, ...).
             ``SIZE``
                 A random size (e.g. small, medium, ...).
 

From 72615f27809066d53f9a8994f09038c33d5a9cf7 Mon Sep 17 00:00:00 2001
From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Date: Mon, 5 Feb 2024 09:14:48 +0000
Subject: [PATCH 12/15] Automatically validate xtrigger function signature
 (#59)

* Remove redundant try block.

* Fix changelog

* Automatically validate xtrigger function signature

---------

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
---
 changes.d/{5452.feat.md => 5955.feat.md} |  0
 cylc/flow/subprocpool.py                 |  5 +-
 cylc/flow/xtrigger_mgr.py                | 11 +++++
 cylc/flow/xtriggers/xrandom.py           | 15 ------
 tests/integration/test_config.py         | 63 +++++++++++++++---------
 tests/unit/xtriggers/test_xrandom.py     | 11 ++---
 6 files changed, 57 insertions(+), 48 deletions(-)
 rename changes.d/{5452.feat.md => 5955.feat.md} (100%)

diff --git a/changes.d/5452.feat.md b/changes.d/5955.feat.md
similarity index 100%
rename from changes.d/5452.feat.md
rename to changes.d/5955.feat.md
diff --git a/cylc/flow/subprocpool.py b/cylc/flow/subprocpool.py
index 84206d4ebdc..1a49eed680d 100644
--- a/cylc/flow/subprocpool.py
+++ b/cylc/flow/subprocpool.py
@@ -116,10 +116,7 @@ def get_xtrig_func(mod_name, func_name, src_dir):
 
     mod = get_xtrig_mod(mod_name, src_dir)
 
-    try:
-        _XTRIG_FUNC_CACHE[(mod_name, func_name)] = getattr(mod, func_name)
-    except AttributeError:
-        raise
+    _XTRIG_FUNC_CACHE[(mod_name, func_name)] = getattr(mod, func_name)
 
     return _XTRIG_FUNC_CACHE[(mod_name, func_name)]
 
diff --git a/cylc/flow/xtrigger_mgr.py b/cylc/flow/xtrigger_mgr.py
index 890918c49dc..d42e9a87710 100644
--- a/cylc/flow/xtrigger_mgr.py
+++ b/cylc/flow/xtrigger_mgr.py
@@ -16,6 +16,7 @@
 
 from contextlib import suppress
 from enum import Enum
+from inspect import signature
 import json
 import re
 from copy import deepcopy
@@ -279,12 +280,22 @@ def check_xtrigger(
                 fname,
                 f"'{fname}' not found in xtrigger module '{fname}'",
             )
+
         if not callable(func):
             raise XtriggerConfigError(
                 label,
                 fname,
                 f"'{fname}' not callable in xtrigger module '{fname}'",
             )
+        if func is not wall_clock:
+            # Validate args and kwargs against the function signature
+            # (but not for wall_clock because it's a special case).
+            try:
+                signature(func).bind(*fctx.func_args, **fctx.func_kwargs)
+            except TypeError as exc:
+                raise XtriggerConfigError(
+                    label, fname, f"{fctx.get_signature()}: {exc}"
+                )
 
         # Check any string templates in the function arg values (note this
         # won't catch bad task-specific values - which are added dynamically).
diff --git a/cylc/flow/xtriggers/xrandom.py b/cylc/flow/xtriggers/xrandom.py
index 56bbbe7668c..c304bc0d20a 100644
--- a/cylc/flow/xtriggers/xrandom.py
+++ b/cylc/flow/xtriggers/xrandom.py
@@ -112,21 +112,6 @@ def validate(f_args, f_kwargs, f_signature):
     If f_args used, convert to f_kwargs for clarity.
 
     """
-    n_args = len(f_args)
-    n_kwargs = len(f_kwargs)
-
-    if n_args + n_kwargs > 3:
-        raise WorkflowConfigError(f"Too many args: {f_signature}")
-
-    if n_args + n_kwargs < 1:
-        raise WorkflowConfigError(f"Wrong number of args: {f_signature}")
-
-    if n_kwargs:
-        # kwargs must be "secs" and "_"
-        kw = next(iter(f_kwargs))
-        if kw not in ("secs", "_"):
-            raise WorkflowConfigError(f"Illegal arg '{kw}': {f_signature}")
-
     # convert to kwarg
     f_kwargs["percent"] = f_args[0]
     del f_args[0]
diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py
index df93d50f555..11d7bd483e1 100644
--- a/tests/integration/test_config.py
+++ b/tests/integration/test_config.py
@@ -16,17 +16,18 @@
 
 from pathlib import Path
 import sqlite3
-from typing import TYPE_CHECKING
+from typing import Any
 import pytest
 
-from cylc.flow.exceptions import ServiceFileError, WorkflowConfigError
+from cylc.flow.exceptions import (
+    ServiceFileError,
+    WorkflowConfigError,
+    XtriggerConfigError,
+)
 from cylc.flow.parsec.exceptions import ListValueError
 from cylc.flow.pathutil import get_workflow_run_pub_db_path
 
-if TYPE_CHECKING:
-    from types import Any
-
-    Fixture = Any
+Fixture = Any
 
 
 @pytest.mark.parametrize(
@@ -353,7 +354,7 @@ def test_xtrig_validation_wall_clock(
     flow: 'Fixture',
     validate: 'Fixture',
 ):
-    """If an xtrigger module has a `validate_config` it is called.
+    """If an xtrigger module has a `validate()` function is called.
 
     https://github.com/cylc/cylc-flow/issues/5448
     """
@@ -376,14 +377,13 @@ def test_xtrig_validation_echo(
     flow: 'Fixture',
     validate: 'Fixture',
 ):
-    """If an xtrigger module has a `validate_config` it is called.
+    """If an xtrigger module has a `validate()` function is called.
 
     https://github.com/cylc/cylc-flow/issues/5448
     """
     id_ = flow({
         'scheduler': {'allow implicit tasks': True},
         'scheduling': {
-            'initial cycle point': '1012',
             'xtriggers': {'myxt': 'echo()'},
             'graph': {'R1': '@myxt => foo'},
         }
@@ -399,21 +399,20 @@ def test_xtrig_validation_xrandom(
     flow: 'Fixture',
     validate: 'Fixture',
 ):
-    """If an xtrigger module has a `validate_config` it is called.
+    """If an xtrigger module has a `validate()` function it is called.
 
     https://github.com/cylc/cylc-flow/issues/5448
     """
     id_ = flow({
         'scheduler': {'allow implicit tasks': True},
         'scheduling': {
-            'initial cycle point': '1012',
-            'xtriggers': {'myxt': 'xrandom()'},
+            'xtriggers': {'myxt': 'xrandom(200)'},
             'graph': {'R1': '@myxt => foo'},
         }
     })
     with pytest.raises(
         WorkflowConfigError,
-        match=r'Wrong number of args: xrandom\(\)'
+        match=r"'percent' should be a float between 0 and 100:"
     ):
         validate(id_)
 
@@ -423,29 +422,30 @@ def test_xtrig_validation_custom(
     validate: 'Fixture',
     monkeypatch: 'Fixture',
 ):
-    """If an xtrigger module has a `validate_config`
+    """If an xtrigger module has a `validate()` function
     an exception is raised if that validate function fails.
 
     https://github.com/cylc/cylc-flow/issues/5448
-
-    Rather than create our own xtrigger module on disk
-    and attempt to trigger a validation failure we
-    mock our own exception, xtrigger and xtrigger
-    validation functions and inject these into the
-    appropriate locations:
     """
+    # Rather than create our own xtrigger module on disk
+    # and attempt to trigger a validation failure we
+    # mock our own exception, xtrigger and xtrigger
+    # validation functions and inject these into the
+    # appropriate locations:
     GreenExc = type('Green', (Exception,), {})
 
-    def kustom_mock(suite):
+    def kustom_xt(feature):
         return True, {}
 
     def kustom_validate(args, kwargs, sig):
         raise GreenExc('This is only a test.')
 
+    # Patch xtrigger func
     monkeypatch.setattr(
         'cylc.flow.xtrigger_mgr.get_xtrig_func',
-        lambda *args: kustom_mock,
+        lambda *args: kustom_xt,
     )
+    # Patch xtrigger's validate func
     monkeypatch.setattr(
         'cylc.flow.config.get_xtrig_func',
         lambda *args: kustom_validate if "validate" in args else ''
@@ -463,3 +463,22 @@ def kustom_validate(args, kwargs, sig):
     Path(id_)
     with pytest.raises(GreenExc, match=r'This is only a test.'):
         validate(id_)
+
+
+def test_xtrig_signature_validation(
+    flow: 'Fixture',
+    validate: 'Fixture',
+):
+    """Test automatic xtrigger function signature validation."""
+    id_ = flow({
+        'scheduler': {'allow implicit tasks': True},
+        'scheduling': {
+            'xtriggers': {'myxt': 'xrandom()'},
+            'graph': {'R1': '@myxt => foo'},
+        }
+    })
+    with pytest.raises(
+        XtriggerConfigError,
+        match=r"xrandom\(\): missing a required argument: 'percent'"
+    ):
+        validate(id_)
diff --git a/tests/unit/xtriggers/test_xrandom.py b/tests/unit/xtriggers/test_xrandom.py
index 50dacfebcc2..23fcd1b97fd 100644
--- a/tests/unit/xtriggers/test_xrandom.py
+++ b/tests/unit/xtriggers/test_xrandom.py
@@ -27,13 +27,10 @@ def test_validate_good_path():
 
 @pytest.mark.parametrize(
     'args, kwargs, err', (
-        param([100], {'f': 1.1, 'b': 1, 'x': 2}, 'Too', id='too-many-args'),
-        param([], {}, 'Wrong number', id='too-few-args'),
-        param(['foo'], {}, '\'percent', id='percent-not-numeric'),
-        param([101], {}, '\'percent', id='percent>100'),
-        param([-1], {}, '\'percent', id='percent<0'),
-        param([100], {'egg': 1}, 'Illegal', id='invalid-kwarg'),
-        param([100], {'secs': 1.1}, "'secs'", id='secs-not-int'),
+        param(['foo'], {}, r"'percent", id='percent-not-numeric'),
+        param([101], {}, r"'percent", id='percent>100'),
+        param([-1], {}, r"'percent", id='percent<0'),
+        param([100], {'secs': 1.1}, r"'secs'", id='secs-not-int'),
     )
 )
 def test_validate_exceptions(args, kwargs, err):

From 8b2d9d447013674de8c8618f8336658046fcb128 Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Mon, 5 Feb 2024 12:45:42 +0000
Subject: [PATCH 13/15] Apply suggestions from code review

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
---
 cylc/flow/xtriggers/xrandom.py | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/cylc/flow/xtriggers/xrandom.py b/cylc/flow/xtriggers/xrandom.py
index c304bc0d20a..641f60003fa 100644
--- a/cylc/flow/xtriggers/xrandom.py
+++ b/cylc/flow/xtriggers/xrandom.py
@@ -16,13 +16,9 @@
 
 from random import random, randint
 from time import sleep
-from typing import TYPE_CHECKING
+from typing import Any, Dict, List, Tuple
 
-from cylc.flow.exceptions import WorkflowConfigError
-
-
-if TYPE_CHECKING:
-    from typing import Any, Dict, Tuple
+from cylc.flow.exceptions import WorkflowConfigError 
 
 
 COLORS = ["red", "orange", "yellow", "green", "blue", "indigo", "violet"]
@@ -103,16 +99,14 @@ def xrandom(
     return satisfied, results
 
 
-def validate(f_args, f_kwargs, f_signature):
+def validate(f_args: List[str], f_kwargs: Dict[str, Any], f_signature: str):
     """Validate and manipulate args parsed from the workflow config.
 
-    percent: - 0 ≤ x ≤ 100
-    secs: An int.
-
-    If f_args used, convert to f_kwargs for clarity.
-
+    The rules for args are:
+    * percent: Must be 0 ≤ x ≤ 100
+    * secs: Must be an integer.
     """
-    # convert to kwarg
+    # convert to kwarg for clarity
     f_kwargs["percent"] = f_args[0]
     del f_args[0]
 

From 124666bd2eb72b44f85f335ceabd6a614f44c613 Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Tue, 6 Feb 2024 16:28:25 +0000
Subject: [PATCH 14/15] fix flake8

---
 cylc/flow/xtriggers/xrandom.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cylc/flow/xtriggers/xrandom.py b/cylc/flow/xtriggers/xrandom.py
index 641f60003fa..2ceaed35a9f 100644
--- a/cylc/flow/xtriggers/xrandom.py
+++ b/cylc/flow/xtriggers/xrandom.py
@@ -18,7 +18,7 @@
 from time import sleep
 from typing import Any, Dict, List, Tuple
 
-from cylc.flow.exceptions import WorkflowConfigError 
+from cylc.flow.exceptions import WorkflowConfigError
 
 
 COLORS = ["red", "orange", "yellow", "green", "blue", "indigo", "violet"]

From 933b2730f790b12f2c81d923f46aa4f99188abc5 Mon Sep 17 00:00:00 2001
From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Date: Tue, 20 Feb 2024 08:27:51 +0000
Subject: [PATCH 15/15] Improve xtrigger validation (#60)

* Improve xtrigger validation

* wall_clock: use placeholder function for signature validation & autodocs

* Fix docstring for autodoc [skip ci]
---
 cylc/flow/config.py                           | 13 +--
 cylc/flow/exceptions.py                       |  7 +-
 cylc/flow/xtrigger_mgr.py                     | 88 +++++++++++-------
 cylc/flow/xtriggers/echo.py                   | 18 ++--
 cylc/flow/xtriggers/wall_clock.py             | 61 ++++++-------
 cylc/flow/xtriggers/xrandom.py                | 57 ++++--------
 tests/functional/runahead/04-no-final-cycle.t |  2 +-
 tests/functional/runahead/no_final/flow.cylc  |  6 +-
 .../spawn-on-demand/15-stop-flow-3/flow.cylc  |  4 +-
 .../35-pass-special-tasks-non-word-names.t    | 40 ---------
 .../validate/69-bare-clock-xtrigger.t         | 32 -------
 tests/integration/test_config.py              | 90 +++++++++++++------
 tests/integration/test_xtrigger_mgr.py        |  1 +
 tests/unit/test_xtrigger_mgr.py               | 13 +--
 tests/unit/xtriggers/test_echo.py             | 19 ++--
 tests/unit/xtriggers/test_wall_clock.py       | 34 ++++---
 tests/unit/xtriggers/test_xrandom.py          | 18 ++--
 17 files changed, 236 insertions(+), 267 deletions(-)
 delete mode 100755 tests/functional/validate/35-pass-special-tasks-non-word-names.t
 delete mode 100644 tests/functional/validate/69-bare-clock-xtrigger.t

diff --git a/cylc/flow/config.py b/cylc/flow/config.py
index 1b0da424a42..f871531adeb 100644
--- a/cylc/flow/config.py
+++ b/cylc/flow/config.py
@@ -83,7 +83,6 @@
 from cylc.flow.print_tree import print_tree
 from cylc.flow.simulation import configure_sim_modes
 from cylc.flow.subprocctx import SubFuncContext
-from cylc.flow.subprocpool import get_xtrig_func
 from cylc.flow.task_events_mgr import (
     EventData,
     get_event_handler_data
@@ -1735,14 +1734,6 @@ def generate_triggers(self, lexpression, left_nodes, right, seq,
             # Generic xtrigger validation.
             XtriggerManager.check_xtrigger(label, xtrig, self.fdir)
 
-            # Specific xtrigger.validate(), if available.
-            with suppress(AttributeError, ImportError):
-                get_xtrig_func(xtrig.func_name, "validate", self.fdir)(
-                    xtrig.func_args,
-                    xtrig.func_kwargs,
-                    xtrig.get_signature()
-                )
-
             if self.xtrigger_mgr:
                 # (not available during validation)
                 self.xtrigger_mgr.add_trig(label, xtrig, self.fdir)
@@ -2434,8 +2425,8 @@ def upgrade_clock_triggers(self):
             # Derive an xtrigger label.
             label = '_'.join(('_cylc', 'wall_clock', task_name))
             # Define the xtrigger function.
-            xtrig = SubFuncContext(label, 'wall_clock', [], {})
-            xtrig.func_kwargs["offset"] = offset
+            args = [] if offset is None else [offset]
+            xtrig = SubFuncContext(label, 'wall_clock', args, {})
             if self.xtrigger_mgr is None:
                 XtriggerManager.check_xtrigger(label, xtrig, self.fdir)
             else:
diff --git a/cylc/flow/exceptions.py b/cylc/flow/exceptions.py
index 43591e7be9c..3ad8c093757 100644
--- a/cylc/flow/exceptions.py
+++ b/cylc/flow/exceptions.py
@@ -241,13 +241,12 @@ class XtriggerConfigError(WorkflowConfigError):
 
     """
 
-    def __init__(self, label: str, trigger: str, message: str):
+    def __init__(self, label: str, message: str):
         self.label: str = label
-        self.trigger: str = trigger
         self.message: str = message
 
-    def __str__(self):
-        return f'[{self.label}] {self.message}'
+    def __str__(self) -> str:
+        return f'[@{self.label}] {self.message}'
 
 
 class ClientError(CylcError):
diff --git a/cylc/flow/xtrigger_mgr.py b/cylc/flow/xtrigger_mgr.py
index d42e9a87710..b891165dbd3 100644
--- a/cylc/flow/xtrigger_mgr.py
+++ b/cylc/flow/xtrigger_mgr.py
@@ -28,9 +28,10 @@
 import cylc.flow.flags
 from cylc.flow.hostuserutil import get_user
 from cylc.flow.subprocpool import get_xtrig_func
-from cylc.flow.xtriggers.wall_clock import wall_clock
+from cylc.flow.xtriggers.wall_clock import _wall_clock
 
 if TYPE_CHECKING:
+    from inspect import BoundArguments
     from cylc.flow.broadcast_mgr import BroadcastMgr
     from cylc.flow.data_store_mgr import DataStoreMgr
     from cylc.flow.subprocctx import SubFuncContext
@@ -246,9 +247,11 @@ def check_xtrigger(
         fctx: 'SubFuncContext',
         fdir: str,
     ) -> None:
-        """Generic xtrigger validation: check existence and string templates.
+        """Generic xtrigger validation: check existence, string templates and
+        function signature.
 
-        Xtrigger modules may also supply a specific "validate" function.
+        Xtrigger modules may also supply a specific `validate` function
+        which will be run here.
 
         Args:
             label: xtrigger label
@@ -262,6 +265,7 @@ def check_xtrigger(
                 * If the function is not callable.
                 * If any string template in the function context
                   arguments are not present in the expected template values.
+                * If the arguments do not match the function signature.
 
         """
         fname: str = fctx.func_name
@@ -270,32 +274,30 @@ def check_xtrigger(
             func = get_xtrig_func(fname, fname, fdir)
         except ImportError:
             raise XtriggerConfigError(
-                label,
-                fname,
-                f"xtrigger module '{fname}' not found",
+                label, f"xtrigger module '{fname}' not found",
             )
         except AttributeError:
             raise XtriggerConfigError(
-                label,
-                fname,
-                f"'{fname}' not found in xtrigger module '{fname}'",
+                label, f"'{fname}' not found in xtrigger module '{fname}'",
             )
 
         if not callable(func):
             raise XtriggerConfigError(
-                label,
-                fname,
-                f"'{fname}' not callable in xtrigger module '{fname}'",
+                label, f"'{fname}' not callable in xtrigger module '{fname}'",
             )
-        if func is not wall_clock:
-            # Validate args and kwargs against the function signature
-            # (but not for wall_clock because it's a special case).
-            try:
-                signature(func).bind(*fctx.func_args, **fctx.func_kwargs)
-            except TypeError as exc:
-                raise XtriggerConfigError(
-                    label, fname, f"{fctx.get_signature()}: {exc}"
-                )
+
+        # Validate args and kwargs against the function signature
+        sig_str = fctx.get_signature()
+        try:
+            bound_args = signature(func).bind(
+                *fctx.func_args, **fctx.func_kwargs
+            )
+        except TypeError as exc:
+            raise XtriggerConfigError(label, f"{sig_str}: {exc}")
+        # Specific xtrigger.validate(), if available.
+        XtriggerManager.try_xtrig_validate_func(
+            label, fname, fdir, bound_args, sig_str
+        )
 
         # Check any string templates in the function arg values (note this
         # won't catch bad task-specific values - which are added dynamically).
@@ -311,9 +313,7 @@ def check_xtrigger(
                     template_vars.add(TemplateVariables(match))
                 except ValueError:
                     raise XtriggerConfigError(
-                        label,
-                        fname,
-                        f"Illegal template in xtrigger: {match}",
+                        label, f"Illegal template in xtrigger: {match}",
                     )
 
         # check for deprecated template variables
@@ -329,6 +329,30 @@ def check_xtrigger(
                 f' {", ".join(t.value for t in deprecated_variables)}'
             )
 
+    @staticmethod
+    def try_xtrig_validate_func(
+        label: str,
+        fname: str,
+        fdir: str,
+        bound_args: 'BoundArguments',
+        signature_str: str,
+    ):
+        """Call an xtrigger's `validate()` function if it is implemented.
+
+        Raise XtriggerConfigError if validation fails.
+        """
+        try:
+            xtrig_validate_func = get_xtrig_func(fname, 'validate', fdir)
+        except (AttributeError, ImportError):
+            return
+        bound_args.apply_defaults()
+        try:
+            xtrig_validate_func(bound_args.arguments)
+        except Exception as exc:  # Note: catch all errors
+            raise XtriggerConfigError(
+                label, f"{signature_str} validation failed: {exc}"
+            )
+
     def add_trig(self, label: str, fctx: 'SubFuncContext', fdir: str) -> None:
         """Add a new xtrigger function.
 
@@ -410,20 +434,18 @@ def get_xtrig_ctx(
         args = []
         kwargs = {}
         if ctx.func_name == "wall_clock":
-            if "trigger_time" in ctx.func_kwargs:
+            if "trigger_time" in ctx.func_kwargs:  # noqa: SIM401 (readabilty)
                 # Internal (retry timer): trigger_time already set.
                 kwargs["trigger_time"] = ctx.func_kwargs["trigger_time"]
-            elif "offset" in ctx.func_kwargs:  # noqa: SIM106
+            else:
                 # External (clock xtrigger): convert offset to trigger_time.
                 # Datetime cycling only.
                 kwargs["trigger_time"] = itask.get_clock_trigger_time(
                     itask.point,
-                    ctx.func_kwargs["offset"]
-                )
-            else:
-                # Should not happen!
-                raise ValueError(
-                    "wall_clock function kwargs needs trigger time or offset"
+                    ctx.func_kwargs.get(
+                        "offset",
+                        ctx.func_args[0] if ctx.func_args else None
+                    )
                 )
         else:
             # Other xtrig functions: substitute template values.
@@ -455,7 +477,7 @@ def call_xtriggers_async(self, itask: 'TaskProxy'):
                 if sig in self.sat_xtrig:
                     # Already satisfied, just update the task
                     itask.state.xtriggers[label] = True
-                elif wall_clock(*ctx.func_args, **ctx.func_kwargs):
+                elif _wall_clock(*ctx.func_args, **ctx.func_kwargs):
                     # Newly satisfied
                     itask.state.xtriggers[label] = True
                     self.sat_xtrig[sig] = {}
diff --git a/cylc/flow/xtriggers/echo.py b/cylc/flow/xtriggers/echo.py
index 0297545e935..d200e2389bb 100644
--- a/cylc/flow/xtriggers/echo.py
+++ b/cylc/flow/xtriggers/echo.py
@@ -16,10 +16,9 @@
 
 """A Cylc xtrigger function."""
 
+from typing import Any, Dict, Tuple
 from cylc.flow.exceptions import WorkflowConfigError
 
-from typing import Tuple
-
 
 def echo(*args, **kwargs) -> Tuple:
     """Print arguments to stdout, return kwargs['succeed'] and kwargs.
@@ -48,15 +47,18 @@ def echo(*args, **kwargs) -> Tuple:
     return kwargs["succeed"], kwargs
 
 
-def validate(f_args, f_kwargs, f_signature):
-
+def validate(all_args: Dict[str, Any]):
     """
     Validate the xtrigger function arguments parsed from the workflow config.
 
     This is separate from the xtrigger to allow parse-time validation.
 
     """
-    if "succeed" not in f_kwargs or not isinstance(f_kwargs["succeed"], bool):
-        raise WorkflowConfigError(
-            f"Requires 'succeed=True/False' arg: {f_signature}"
-        )
+    # NOTE: with (*args, **kwargs) pattern, all_args looks like:
+    # {
+    #     'args': (arg1, arg2, ...),
+    #     'kwargs': {kwarg1: val, kwarg2: val, ...}
+    # }
+    succeed = all_args['kwargs'].get("succeed")
+    if not isinstance(succeed, bool):
+        raise WorkflowConfigError("Requires 'succeed=True/False' arg")
diff --git a/cylc/flow/xtriggers/wall_clock.py b/cylc/flow/xtriggers/wall_clock.py
index c18ea2e5d2b..d5d1a009154 100644
--- a/cylc/flow/xtriggers/wall_clock.py
+++ b/cylc/flow/xtriggers/wall_clock.py
@@ -17,56 +17,53 @@
 """xtrigger function to trigger off of a wall clock time."""
 
 from time import time
+from typing import Any, Dict
 from cylc.flow.cycling.iso8601 import interval_parse
 from cylc.flow.exceptions import WorkflowConfigError
 
 
-def wall_clock(trigger_time=None):
-    """Return True after the desired wall clock time, False.
+def wall_clock(offset: str = 'PT0S'):
+    """Trigger at a specific real "wall clock" time relative to the cycle point
+    in the graph.
+
+    Clock triggers, unlike other trigger functions, are executed synchronously
+    in the main process.
+
+    Args:
+        offset:
+            ISO 8601 interval to wait after the cycle point is reached in real
+            time before triggering. May be negative, in which case it will
+            trigger before the real time reaches the cycle point.
+    """
+    # NOTE: This is just a placeholder for the actual implementation.
+    # This is only used for validating the signature and for autodocs.
+    ...
+
+
+def _wall_clock(trigger_time: int) -> bool:
+    """Actual implementation of wall_clock.
+
+    Return True after the desired wall clock time, or False before.
 
     Args:
-        trigger_time (int):
+        trigger_time:
             Trigger time as seconds since Unix epoch.
     """
     return time() > trigger_time
 
 
-def validate(f_args, f_kwargs, f_signature):
+def validate(args: Dict[str, Any]):
     """Validate and manipulate args parsed from the workflow config.
 
+    NOTE: the xtrigger signature is different to the function signature above
+
     wall_clock()  # infer zero interval
     wall_clock(PT1H)
     wall_clock(offset=PT1H)
 
     The offset must be a valid ISO 8601 interval.
-
-    If f_args used, convert to f_kwargs for clarity.
-
     """
-
-    n_args = len(f_args)
-    n_kwargs = len(f_kwargs)
-
-    if n_args + n_kwargs > 1:
-        raise WorkflowConfigError(f"Too many args: {f_signature}")
-
-    if n_kwargs:
-        # sole kwarg must be "offset"
-        kw = next(iter(f_kwargs))
-        if kw != "offset":
-            raise WorkflowConfigError(f"Illegal arg '{kw}': {f_signature}")
-
-    elif n_args:
-        # convert to kwarg
-        f_kwargs["offset"] = f_args[0]
-        del f_args[0]
-
-    else:
-        # no args, infer zero interval
-        f_kwargs["offset"] = "P0Y"
-
-    # must be a valid interval
     try:
-        interval_parse(f_kwargs["offset"])
+        interval_parse(args["offset"])
     except (ValueError, AttributeError):
-        raise WorkflowConfigError(f"Invalid offset: {f_signature}")
+        raise WorkflowConfigError(f"Invalid offset: {args['offset']}")
diff --git a/cylc/flow/xtriggers/xrandom.py b/cylc/flow/xtriggers/xrandom.py
index 2ceaed35a9f..867035d46f8 100644
--- a/cylc/flow/xtriggers/xrandom.py
+++ b/cylc/flow/xtriggers/xrandom.py
@@ -16,7 +16,7 @@
 
 from random import random, randint
 from time import sleep
-from typing import Any, Dict, List, Tuple
+from typing import Any, Dict, Tuple
 
 from cylc.flow.exceptions import WorkflowConfigError
 
@@ -26,8 +26,8 @@
 
 
 def xrandom(
-    percent: float, secs: int = 0, _: 'Any' = None
-) -> 'Tuple[bool, Dict]':
+    percent: float, secs: int = 0, _: Any = None
+) -> Tuple[bool, Dict[str, str]]:
     """Random xtrigger, with configurable sleep and percent success.
 
     Sleep for ``sec`` seconds, and report satisfied with ``percent``
@@ -75,7 +75,7 @@ def xrandom(
         (True, {'COLOR': 'orange', 'SIZE': 'small'})
 
     Returns:
-        Tuple, containing:
+        tuple: (satisfied, results)
 
         satisfied:
             True if ``satisfied`` else ``False``.
@@ -90,52 +90,31 @@ def xrandom(
     """
     sleep(float(secs))
     results = {}
-    satisfied = random() < float(percent) / 100  # nosec
+    satisfied = random() < float(percent) / 100  # nosec: B311
     if satisfied:
         results = {
-            'COLOR': COLORS[randint(0, len(COLORS) - 1)],  # nosec
-            'SIZE': SIZES[randint(0, len(SIZES) - 1)]  # nosec
+            'COLOR': COLORS[randint(0, len(COLORS) - 1)],  # nosec: B311
+            'SIZE': SIZES[randint(0, len(SIZES) - 1)]  # nosec: B311
         }
     return satisfied, results
 
 
-def validate(f_args: List[str], f_kwargs: Dict[str, Any], f_signature: str):
+def validate(args: Dict[str, Any]):
     """Validate and manipulate args parsed from the workflow config.
 
     The rules for args are:
     * percent: Must be 0 ≤ x ≤ 100
     * secs: Must be an integer.
     """
-    # convert to kwarg for clarity
-    f_kwargs["percent"] = f_args[0]
-    del f_args[0]
-
-    should_raise = False
-
-    try:
-        percent = f_kwargs['percent']
-        percent = float(percent)
-    except ValueError:
-        should_raise = True
-    else:
-        if (
-            not isinstance(percent, (float, int))
-            or percent < 0
-            or percent > 100
-        ):
-            should_raise = True
-    if should_raise:
+    percent = args['percent']
+    if (
+        not isinstance(percent, (float, int))
+        or not (0 <= percent <= 100)
+    ):
         raise WorkflowConfigError(
-            f"'percent' should be a float between 0 and 100: {f_signature}")
+            "'percent' should be a float between 0 and 100"
+        )
 
-    try:
-        secs = f_kwargs.get('secs', 0)
-    except ValueError:
-        should_raise = True
-    else:
-        if not isinstance(secs, int):
-            should_raise = True
-
-    if should_raise:
-        raise WorkflowConfigError(
-            f"'secs' should be an integer: {f_signature}")
+    secs = args['secs']
+    if not isinstance(secs, int):
+        raise WorkflowConfigError("'secs' should be an integer")
diff --git a/tests/functional/runahead/04-no-final-cycle.t b/tests/functional/runahead/04-no-final-cycle.t
index 1c9609033df..ae1ecebc500 100644
--- a/tests/functional/runahead/04-no-final-cycle.t
+++ b/tests/functional/runahead/04-no-final-cycle.t
@@ -32,7 +32,7 @@ TEST_NAME=${TEST_NAME_BASE}-check-fail
 DB="$RUN_DIR/${WORKFLOW_NAME}/log/db"
 TASKS=$(sqlite3 "${DB}" 'select count(*) from task_states where status=="failed"')
 # manual comparison for the test
-if ((TASKS==4)); then
+if ((TASKS==2)); then
     ok "${TEST_NAME}"
 else
     fail "${TEST_NAME}"
diff --git a/tests/functional/runahead/no_final/flow.cylc b/tests/functional/runahead/no_final/flow.cylc
index 89e23f7af1c..3dd1d7e9734 100644
--- a/tests/functional/runahead/no_final/flow.cylc
+++ b/tests/functional/runahead/no_final/flow.cylc
@@ -1,11 +1,13 @@
-# Should shutdown stalled with 4 failed tasks.
+# Should shutdown stalled with 2 failed tasks due to P1 runahead limit
 [scheduler]
     cycle point time zone = Z
     [[events]]
+        abort on stall timeout = True
+        stall timeout = PT0S
         abort on inactivity timeout = True
         inactivity timeout = PT20S
 [scheduling]
-    runahead limit = P3
+    runahead limit = P1
     initial cycle point = 20100101T00
     [[xtriggers]]
         never = wall_clock(P100Y)
diff --git a/tests/functional/spawn-on-demand/15-stop-flow-3/flow.cylc b/tests/functional/spawn-on-demand/15-stop-flow-3/flow.cylc
index fb796f784f5..4731fee49ea 100644
--- a/tests/functional/spawn-on-demand/15-stop-flow-3/flow.cylc
+++ b/tests/functional/spawn-on-demand/15-stop-flow-3/flow.cylc
@@ -12,11 +12,11 @@
         abort on inactivity timeout = True
 [scheduling]
     [[xtriggers]]
-        x = xrandom("0", "10")  # never succeeds
+        x = xrandom(0, 10)  # never succeeds
     [[graph]]
         R1 = """
                @x => never
-               c => never_again 
+               c => never_again
              """
 [runtime]
     [[c]]
diff --git a/tests/functional/validate/35-pass-special-tasks-non-word-names.t b/tests/functional/validate/35-pass-special-tasks-non-word-names.t
deleted file mode 100755
index c523ea25b78..00000000000
--- a/tests/functional/validate/35-pass-special-tasks-non-word-names.t
+++ /dev/null
@@ -1,40 +0,0 @@
-#!/usr/bin/env bash
-# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
-# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-#-------------------------------------------------------------------------------
-# Test validation of special tasks names with non-word characters
-. "$(dirname "$0")/test_header"
-set_test_number 1
-cat >'flow.cylc' <<'__FLOW_CONFIG__'
-[scheduling]
-    initial cycle point = 20200202
-    final cycle point = 20300303
-    [[special tasks]]
-        clock-trigger = t-1, t+1, t%1, t@1
-    [[graph]]
-        P1D = """
-            t-1
-            t+1
-            t%1
-            t@1
-        """
-
-[runtime]
-    [[t-1, t+1, t%1, t@1]]
-        script = true
-__FLOW_CONFIG__
-run_ok "${TEST_NAME_BASE}" cylc validate "${PWD}"
-exit
diff --git a/tests/functional/validate/69-bare-clock-xtrigger.t b/tests/functional/validate/69-bare-clock-xtrigger.t
deleted file mode 100644
index 4408a064dba..00000000000
--- a/tests/functional/validate/69-bare-clock-xtrigger.t
+++ /dev/null
@@ -1,32 +0,0 @@
-#!/usr/bin/env bash
-# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
-# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-# Test that undeclared zero-offset clock xtriggers are allowed.
-. "$(dirname "$0")/test_header"
-
-set_test_number 1
-
-cat >'flow.cylc' <<'__FLOW_CONFIG__'
-[scheduler]
-    allow implicit tasks = True
-[scheduling]
-    initial cycle point = now
-    [[graph]]
-        T00 = "@wall_clock => foo"
-__FLOW_CONFIG__
-
-run_ok "${TEST_NAME_BASE}-val" cylc validate .
diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py
index 11d7bd483e1..581ec4d83fb 100644
--- a/tests/integration/test_config.py
+++ b/tests/integration/test_config.py
@@ -362,17 +362,31 @@ def test_xtrig_validation_wall_clock(
         'scheduler': {'allow implicit tasks': True},
         'scheduling': {
             'initial cycle point': '1012',
-            'xtriggers': {'myxt': 'wall_clock(offset=PT755MH)'},
+            'xtriggers': {'myxt': 'wall_clock(offset=PT7MH)'},
             'graph': {'R1': '@myxt => foo'},
         }
     })
-    with pytest.raises(
-        WorkflowConfigError,
-        match=r'Invalid offset: wall_clock\(offset=PT755MH\)'
-    ):
+    with pytest.raises(WorkflowConfigError, match=(
+        r'\[@myxt\] wall_clock\(offset=PT7MH\) validation failed: '
+        r'Invalid offset: PT7MH'
+    )):
         validate(id_)
 
 
+def test_xtrig_implicit_wall_clock(flow: Fixture, validate: Fixture):
+    """Test @wall_clock is allowed in graph without explicit
+    xtrigger definition.
+    """
+    wid = flow({
+        'scheduler': {'allow implicit tasks': True},
+        'scheduling': {
+            'initial cycle point': '2024',
+            'graph': {'R1': '@wall_clock => foo'},
+        }
+    })
+    validate(wid)
+
+
 def test_xtrig_validation_echo(
     flow: 'Fixture',
     validate: 'Fixture',
@@ -390,7 +404,7 @@ def test_xtrig_validation_echo(
     })
     with pytest.raises(
         WorkflowConfigError,
-        match=r'Requires \'succeed=True/False\' arg: echo()'
+        match=r'echo.* Requires \'succeed=True/False\' arg'
     ):
         validate(id_)
 
@@ -411,8 +425,8 @@ def test_xtrig_validation_xrandom(
         }
     })
     with pytest.raises(
-        WorkflowConfigError,
-        match=r"'percent' should be a float between 0 and 100:"
+        XtriggerConfigError,
+        match=r"'percent' should be a float between 0 and 100"
     ):
         validate(id_)
 
@@ -432,23 +446,16 @@ def test_xtrig_validation_custom(
     # mock our own exception, xtrigger and xtrigger
     # validation functions and inject these into the
     # appropriate locations:
-    GreenExc = type('Green', (Exception,), {})
-
     def kustom_xt(feature):
         return True, {}
 
-    def kustom_validate(args, kwargs, sig):
-        raise GreenExc('This is only a test.')
+    def kustom_validate(args):
+        raise Exception('This is only a test.')
 
-    # Patch xtrigger func
+    # Patch xtrigger func & its validate func
     monkeypatch.setattr(
         'cylc.flow.xtrigger_mgr.get_xtrig_func',
-        lambda *args: kustom_xt,
-    )
-    # Patch xtrigger's validate func
-    monkeypatch.setattr(
-        'cylc.flow.config.get_xtrig_func',
-        lambda *args: kustom_validate if "validate" in args else ''
+        lambda *args: kustom_validate if "validate" in args else kustom_xt
     )
 
     id_ = flow({
@@ -461,24 +468,53 @@ def kustom_validate(args, kwargs, sig):
     })
 
     Path(id_)
-    with pytest.raises(GreenExc, match=r'This is only a test.'):
+    with pytest.raises(XtriggerConfigError, match=r'This is only a test.'):
         validate(id_)
 
 
+@pytest.mark.parametrize('xtrig_call, expected_msg', [
+    pytest.param(
+        'xrandom()',
+        r"xrandom.* missing a required argument: 'percent'",
+        id="missing-arg"
+    ),
+    pytest.param(
+        'wall_clock(alan_grant=1)',
+        r"wall_clock.* unexpected keyword argument 'alan_grant'",
+        id="unexpected-arg"
+    ),
+])
 def test_xtrig_signature_validation(
-    flow: 'Fixture',
-    validate: 'Fixture',
+    flow: 'Fixture', validate: 'Fixture',
+    xtrig_call: str, expected_msg: str
 ):
     """Test automatic xtrigger function signature validation."""
     id_ = flow({
         'scheduler': {'allow implicit tasks': True},
         'scheduling': {
-            'xtriggers': {'myxt': 'xrandom()'},
+            'initial cycle point': '2024',
+            'xtriggers': {'myxt': xtrig_call},
             'graph': {'R1': '@myxt => foo'},
         }
     })
-    with pytest.raises(
-        XtriggerConfigError,
-        match=r"xrandom\(\): missing a required argument: 'percent'"
-    ):
+    with pytest.raises(XtriggerConfigError, match=expected_msg):
         validate(id_)
+
+
+def test_special_task_non_word_names(flow: Fixture, validate: Fixture):
+    """Test validation of special tasks names with non-word characters"""
+    wid = flow({
+        'scheduling': {
+            'initial cycle point': '2020',
+            'special tasks': {
+                'clock-trigger': 't-1, t+1, t%1, t@1',
+            },
+            'graph': {
+                'P1D': 't-1 & t+1 & t%1 & t@1',
+            },
+        },
+        'runtime': {
+            't-1, t+1, t%1, t@1': {'script': True},
+        },
+    })
+    validate(wid)
diff --git a/tests/integration/test_xtrigger_mgr.py b/tests/integration/test_xtrigger_mgr.py
index c116e6968a5..612049163cc 100644
--- a/tests/integration/test_xtrigger_mgr.py
+++ b/tests/integration/test_xtrigger_mgr.py
@@ -21,6 +21,7 @@
 
 from cylc.flow.pathutil import get_workflow_run_dir
 
+
 async def test_2_xtriggers(flow, start, scheduler, monkeypatch):
     """Test that if an itask has 2 wall_clock triggers with different
     offsets that xtrigger manager gets both of them.
diff --git a/tests/unit/test_xtrigger_mgr.py b/tests/unit/test_xtrigger_mgr.py
index 9b497d8e5ab..78f8fe6d969 100644
--- a/tests/unit/test_xtrigger_mgr.py
+++ b/tests/unit/test_xtrigger_mgr.py
@@ -44,7 +44,7 @@ def test_extract_templates():
     )
 
 
-def test_check_xtrigger(xtrigger_mgr):
+def test_add_xtrigger(xtrigger_mgr):
     """Test for adding an xtrigger."""
     xtrig = SubFuncContext(
         label="echo",
@@ -84,9 +84,12 @@ def test_check_xtrigger_with_unknown_params(xtrigger_mgr):
         label="echo",
         func_name="echo",
         func_args=[1, "name", "%(what_is_this)s"],
-        func_kwargs={"location": "soweto"}
+        func_kwargs={"succeed": True}
     )
-    with pytest.raises(XtriggerConfigError):
+    with pytest.raises(
+        XtriggerConfigError,
+        match="Illegal template in xtrigger: what_is_this"
+    ):
         xtrigger_mgr.check_xtrigger("xtrig", xtrig, 'fdir')
 
 
@@ -96,7 +99,7 @@ def test_check_xtrigger_with_deprecated_params(xtrigger_mgr, caplog):
         label="echo",
         func_name="echo",
         func_args=[1, "name", "%(suite_name)s"],
-        func_kwargs={"location": "soweto"}
+        func_kwargs={"succeed": True}
     )
     caplog.set_level(logging.WARNING, CYLC_LOG)
     xtrigger_mgr.check_xtrigger("xtrig", xtrig, 'fdir')
@@ -139,7 +142,6 @@ def test_housekeeping_nothing_satisfied(xtrigger_mgr):
 def test_housekeeping_with_xtrigger_satisfied(xtrigger_mgr):
     """The housekeeping method makes sure only satisfied xtrigger function
     are kept."""
-    xtrigger_mgr.check_xtrigger = lambda *a, **k: True  # Ignore validation
     xtrig = SubFuncContext(
         label="get_name",
         func_name="get_name",
@@ -171,7 +173,6 @@ def test_housekeeping_with_xtrigger_satisfied(xtrigger_mgr):
 
 def test__call_xtriggers_async(xtrigger_mgr):
     """Test _call_xtriggers_async"""
-    xtrigger_mgr.check_xtrigger = lambda *a, **k: True  # Ignore validation
     # the echo1 xtrig (not satisfied)
     echo1_xtrig = SubFuncContext(
         label="echo1",
diff --git a/tests/unit/xtriggers/test_echo.py b/tests/unit/xtriggers/test_echo.py
index b4bc104adac..7b1f4d140ee 100644
--- a/tests/unit/xtriggers/test_echo.py
+++ b/tests/unit/xtriggers/test_echo.py
@@ -20,18 +20,19 @@
 from pytest import param
 
 
-def test_validate_good_path():
-    assert validate(
-        [], {'succeed': False, 'egg': 'fried', 'potato': 'baked'}, 'succeed'
-    ) is None
+def test_validate_good():
+    validate({
+        'args': (),
+        'kwargs': {'succeed': False, 'egg': 'fried', 'potato': 'baked'}
+    })
 
 
 @pytest.mark.parametrize(
-    'args, kwargs', (
-        param([False], {}, id='no-kwarg'),
-        param([], {'spud': 'mashed'}, id='no-succeed-kwarg'),
+    'all_args', (
+        param({'args': (False,), 'kwargs': {}}, id='no-kwarg'),
+        param({'args': (), 'kwargs': {'spud': 'mashed'}}, id='no-succeed-kwarg'),
     )
 )
-def test_validate_exceptions(args, kwargs):
+def test_validate_exceptions(all_args):
     with pytest.raises(WorkflowConfigError, match='^Requires'):
-        validate(args, kwargs, 'blah')
\ No newline at end of file
+        validate(all_args)
diff --git a/tests/unit/xtriggers/test_wall_clock.py b/tests/unit/xtriggers/test_wall_clock.py
index 6dc1cb2b6f8..44bba2619fc 100644
--- a/tests/unit/xtriggers/test_wall_clock.py
+++ b/tests/unit/xtriggers/test_wall_clock.py
@@ -14,13 +14,25 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-from cylc.flow.exceptions import WorkflowConfigError
-from cylc.flow.xtriggers.wall_clock import validate
+from cylc.flow.xtriggers.wall_clock import _wall_clock, validate
 from metomi.isodatetime.parsers import DurationParser
 import pytest
 from pytest import param
 
 
+@pytest.mark.parametrize('trigger_time, expected', [
+    (499, True),
+    (500, False),
+])
+def test_wall_clock(
+    monkeypatch: pytest.MonkeyPatch, trigger_time: int, expected: bool
+):
+    monkeypatch.setattr(
+        'cylc.flow.xtriggers.wall_clock.time', lambda: 500
+    )
+    assert _wall_clock(trigger_time) == expected
+
+
 @pytest.fixture
 def monkeypatch_interval_parser(monkeypatch):
     """Interval parse only works normally if a WorkflowSpecifics
@@ -32,20 +44,18 @@ def monkeypatch_interval_parser(monkeypatch):
     )
 
 
-def test_validate_good_path(monkeypatch_interval_parser):
-    assert validate([], {}, 'Alles Gut') is None
+def test_validate_good(monkeypatch_interval_parser):
+    validate({'offset': 'PT1H'})
 
 
 @pytest.mark.parametrize(
-    'args, kwargs, err', (
-        param([1, 2], {}, "^Too", id='too-many-args'),
-        param([], {'egg': 12}, "^Illegal", id='illegal-arg'),
-        param([1], {}, "^Invalid", id='invalid-offset-int'),
-        param([], {'offset': 'Zaphod'}, "^Invalid", id='invalid-offset-str'),
+    'args, err', (
+        param({'offset': 1}, "^Invalid", id='invalid-offset-int'),
+        param({'offset': 'Zaphod'}, "^Invalid", id='invalid-offset-str'),
     )
 )
 def test_validate_exceptions(
-    monkeypatch_interval_parser, args, kwargs, err
+    monkeypatch_interval_parser, args, err
 ):
-    with pytest.raises(WorkflowConfigError, match=err):
-        validate(args, kwargs, 'Alles Gut')
+    with pytest.raises(Exception, match=err):
+        validate(args)
diff --git a/tests/unit/xtriggers/test_xrandom.py b/tests/unit/xtriggers/test_xrandom.py
index 23fcd1b97fd..21a9fdb776d 100644
--- a/tests/unit/xtriggers/test_xrandom.py
+++ b/tests/unit/xtriggers/test_xrandom.py
@@ -21,19 +21,19 @@
 from cylc.flow.exceptions import WorkflowConfigError
 
 
-def test_validate_good_path():
-    assert validate([1], {'secs': 0, '_': 'HelloWorld'}, 'good_path') is None
+def test_validate_good():
+    validate({'percent': 1, 'secs': 0, '_': 'HelloWorld'})
 
 
 @pytest.mark.parametrize(
-    'args, kwargs, err', (
-        param(['foo'], {}, r"'percent", id='percent-not-numeric'),
-        param([101], {}, r"'percent", id='percent>100'),
-        param([-1], {}, r"'percent", id='percent<0'),
-        param([100], {'secs': 1.1}, r"'secs'", id='secs-not-int'),
+    'args, err', (
+        param({'percent': 'foo'}, r"'percent", id='percent-not-numeric'),
+        param({'percent': 101}, r"'percent", id='percent>100'),
+        param({'percent': -1}, r"'percent", id='percent<0'),
+        param({'percent': 100, 'secs': 1.1}, r"'secs'", id='secs-not-int'),
     )
 )
-def test_validate_exceptions(args, kwargs, err):
+def test_validate_exceptions(args, err):
     """Illegal args and kwargs cause a WorkflowConfigError raised."""
     with pytest.raises(WorkflowConfigError, match=f'^{err}'):
-        validate(args, kwargs, 'blah')
+        validate(args)