fix(workflows): raise a clear error, not a cryptic crash, on non-string filter args (#3522)

The `map`, `join`, and `contains` expression filters assumed their
argument was a string. A non-string argument — an authoring mistake such
as `| map(5)`, `| join(5)`, or `| contains(5)` — reached an operation
that only strings support and raised a cryptic exception that escaped the
evaluator entirely:

  * `map(5)`      -> `attr.split(".")`  -> AttributeError
  * `join(5)`     -> `separator.join(...)` -> AttributeError
  * `contains(5)` on a string value -> `x in str` -> TypeError

The engine wraps neither expression evaluation nor `step_impl.execute()`
in a try/except, so each of these took down the whole run with a message
that names none of the real problem.

Validate the argument type up front and raise a `ValueError` naming the
filter and the offending type instead, mirroring the strict argument
handling already in `from_json`. `contains` guards only the string-value
branch: for a list value, membership of any element type is legitimate
(`5 in [1, 2, 5]`), so that branch is intentionally left unguarded.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: Noor-ul-ain001

src/specify_cli/workflows/expressions.py

@@ -35,14 +35,38 @@ def _filter_default(value: Any, default_value: Any = "") -> Any:
 
 
 def _filter_join(value: Any, separator: str = ", ") -> str:
-    """Join a list into a string with *separator*."""
+    """Join a list into a string with *separator*.
+
+    Raises ``ValueError`` when *separator* is not a string. Without the guard a
+    non-string separator (an authoring mistake like ``| join(5)``) reaches
+    ``str.join`` and raises a cryptic ``AttributeError: 'int' object has no
+    attribute 'join'`` that escapes the evaluator and crashes the whole run,
+    since the engine wraps neither expression evaluation nor ``execute`` in a
+    try/except. Mirrors the strict argument handling in ``from_json``.
+    """
+    if not isinstance(separator, str):
+        raise ValueError(
+            f"join: expected a string separator, got {type(separator).__name__}"
+        )
     if isinstance(value, list):
         return separator.join(str(v) for v in value)
     return str(value)
 
 
 def _filter_map(value: Any, attr: str) -> list[Any]:
-    """Map a list of dicts to a specific attribute."""
+    """Map a list of dicts to a specific attribute.
+
+    Raises ``ValueError`` when *attr* is not a string. Without the guard a
+    non-string attribute (an authoring mistake like ``| map(5)``) reaches
+    ``attr.split(".")`` and raises a cryptic ``AttributeError: 'int' object has
+    no attribute 'split'`` that escapes the evaluator and crashes the whole run,
+    since the engine wraps neither expression evaluation nor ``execute`` in a
+    try/except. Mirrors the strict argument handling in ``from_json``.
+    """
+    if not isinstance(attr, str):
+        raise ValueError(
+            f"map: expected a string attribute name, got {type(attr).__name__}"
+        )
     if isinstance(value, list):
         result = []
         for item in value:
@@ -63,9 +87,25 @@ def _filter_map(value: Any, attr: str) -> list[Any]:
     return []
 
 
-def _filter_contains(value: Any, substring: str) -> bool:
-    """Check if a string or list contains *substring*."""
+def _filter_contains(value: Any, substring: Any) -> bool:
+    """Check if a string or list contains *substring*.
+
+    For a string *value*, *substring* must itself be a string: ``x in y`` on a
+    string requires a string left operand, so a non-string argument (an
+    authoring mistake like ``| contains(5)``) would otherwise raise a cryptic
+    ``TypeError`` that escapes the evaluator and crashes the whole run, since
+    the engine wraps neither expression evaluation nor ``execute`` in a
+    try/except. Raise a ``ValueError`` naming the problem instead, mirroring the
+    strict argument handling in ``from_json``. For a list *value*, membership of
+    any element type is legitimate (``5 in [1, 2, 5]``), so that branch is left
+    unguarded.
+    """
     if isinstance(value, str):
+        if not isinstance(substring, str):
+            raise ValueError(
+                "contains: expected a string argument when the value is a "
+                f"string, got {type(substring).__name__}"
+            )
         return substring in value
     if isinstance(value, list):
         return substring in value

tests/test_workflows.py

@@ -586,6 +586,51 @@ def test_filter_unknown_name_with_args_raises(self):
         with pytest.raises(ValueError, match="unknown filter 'upper'"):
             evaluate_expression("{{ inputs.text | upper('x') }}", ctx)
 
+    def test_filter_map_non_string_attr_raises(self):
+        # A non-string attribute (authoring mistake like `map(5)`) must raise a
+        # ValueError naming the problem, not leak the cryptic AttributeError
+        # from attr.split() that would escape the evaluator and crash the run.
+        import pytest
+        from specify_cli.workflows.expressions import evaluate_expression
+        from specify_cli.workflows.base import StepContext
+
+        ctx = StepContext(inputs={"rows": [{"id": "a"}, {"id": "b"}]})
+        with pytest.raises(ValueError, match="map: expected a string attribute name"):
+            evaluate_expression("{{ inputs.rows | map(5) }}", ctx)
+
+    def test_filter_join_non_string_separator_raises(self):
+        # A non-string separator (authoring mistake like `join(5)`) must raise a
+        # ValueError, not leak the cryptic AttributeError from str.join.
+        import pytest
+        from specify_cli.workflows.expressions import evaluate_expression
+        from specify_cli.workflows.base import StepContext
+
+        ctx = StepContext(inputs={"tags": ["a", "b"]})
+        with pytest.raises(ValueError, match="join: expected a string separator"):
+            evaluate_expression("{{ inputs.tags | join(5) }}", ctx)
+
+    def test_filter_contains_non_string_arg_on_string_raises(self):
+        # For a string value, `contains` requires a string argument: `x in y` on
+        # a string needs a string left operand. A non-string argument must raise
+        # a ValueError, not leak the cryptic TypeError that would crash the run.
+        import pytest
+        from specify_cli.workflows.expressions import evaluate_expression
+        from specify_cli.workflows.base import StepContext
+
+        ctx = StepContext(inputs={"text": "hello"})
+        with pytest.raises(ValueError, match="contains: expected a string argument"):
+            evaluate_expression("{{ inputs.text | contains(5) }}", ctx)
+
+    def test_filter_contains_non_string_arg_on_list_ok(self):
+        # For a list value, membership of any element type is legitimate, so a
+        # non-string argument stays valid and is not rejected.
+        from specify_cli.workflows.expressions import evaluate_expression
+        from specify_cli.workflows.base import StepContext
+
+        ctx = StepContext(inputs={"nums": [1, 2, 5]})
+        assert evaluate_expression("{{ inputs.nums | contains(5) }}", ctx) is True
+        assert evaluate_expression("{{ inputs.nums | contains(9) }}", ctx) is False
+
     def test_registered_filters_unaffected(self):
         # Regression: all five registered filters keep working unchanged.
         from specify_cli.workflows.expressions import evaluate_expression