triggers: valid trigger is reported as a WorkflowConfigError

[oliver-sanders]

There appears to be some inconsistency over what is and isn't a valid task trigger.

The task name a+ is valid, and some triggers written with this task name are valid, but some aren't?!

Reproducible Example

This example is valid:

[scheduler]
    allow implicit tasks = True

[scheduling]
    [[graph]]
        R1 = a+ => c

This example is not valid:

[scheduler]
    allow implicit tasks = True

[scheduling]
    [[graph]]
        R1 = a+ | b => c  # WorkflowConfigError: ERROR: bad trigger

Yes this example is valid:

[scheduler]
    allow implicit tasks = True

[scheduling]
    [[graph]]
        R1 = a+:succeeded | b => c

Expected Behaviour

Since a+ is a valid task name, any valid trigger that uses this task name should be valid too.

If the trailing + character is not something that we can support in task triggers, then a task name ending in + should not be valid.

Please also check other characters.

[hjoliver]

(Should fix, but fortunately for us, trailing + seems an unlikely choice in task names)

[wxtim]

Note to self: After much investigation I've found that the code around line 620 of graph_parser.py expand a|b to a:succeeded|b:succeeded, but that the same is not tru of a+|b which expands to a+|b:succeeded.

[wxtim]

I've created the following diff, which should give further work on this a head start, but can't prioritize right now:

diff --git a/cylc/flow/graph_parser.py b/cylc/flow/graph_parser.py
index 075f914d0..057b635de 100644
--- a/cylc/flow/graph_parser.py
+++ b/cylc/flow/graph_parser.py
@@ -620,15 +620,8 @@ class GraphParser:
                 else:
                     # Make success triggers explicit.
                     n_trig = TASK_OUTPUT_SUCCEEDED
-                    if offset:
-                        this = r'\b%s\b%s(?!:)' % (
-                            re.escape(name),
-                            re.escape(offset)
-                        )
-                    else:
-                        this = r'\b%s\b(?![\[:])' % re.escape(name)
-                    that = f"{name}{offset}:{n_trig}"
-                    expr = re.sub(this, that, expr)
+                    expr = self._expand_implicit_success(
+                        expr, name, n_trig, offset)
 
                 n_info.append((name, offset, n_trig, opt))
 
@@ -661,6 +654,28 @@ class GraphParser:
             expr = re.sub(self.__class__._RE_OPT, '', expr)
             self._families_all_to_all(expr, rights, info, family_trig_map)
 
+    @staticmethod
+    def _expand_implicit_success(
+        expr: str, name: str, n_trig: str, offset: str
+    ) -> str:
+        """Expand implicit success triggers to explicit ones.
+
+        Examples:
+            >>> this = GraphParser._expand_implicit_success
+            >>> this('a|b', 'a', 'succeeded', '')
+            'a:succeeded|b'
+            >>> this('a+|b', 'a+', 'succeeded', '')
+            'a+:succeeded|b'
+        """
+        if offset:
+            find = r'\b%s\b%s(?!:)' % (re.escape(name), re.escape(offset))
+        else:
+            find = r'%s(?![\[:])' % re.escape(name)
+
+        replace = f"{name}{offset}:{n_trig}"
+
+        return re.sub(find, replace, expr)
+
     def _families_all_to_all(
         self,
         expr: str,

A direct fix is easy (remove \b) from the second regex, but this leaves too many questions to put this up as a PR:

• What happens if there is an offset.
• What other characters might cause similar problems?
• Is there a reason why the \b is used? What problems will removing it cause?
• Although it's currently legal, might it be saner to be more restrictive on task names to avoid too many corner cases?