fix(workflows): reject a switch expression that is never evaluated

src/specify_cli/workflows/expressions.py

@@ -9,6 +9,7 @@
 
 import json
 import re
+from collections.abc import Iterable
 from contextvars import ContextVar
 from typing import Any
 
@@ -892,6 +893,43 @@ def condition_is_never_evaluated(condition: Any) -> bool:
     return _first_unclosable_block(stripped) == "verbatim"
 
 
+def switch_expression_is_never_evaluated(
+    expression: Any, case_keys: Iterable[Any] = ()
+) -> bool:
+    """True when a switch *expression* is a reference written without its braces.
+
+    ``condition_is_never_evaluated`` cannot be reused here as it stands. It flags
+    every braceless string, because a condition is coerced by ``bool()`` and any
+    non-empty text is therefore always true. A switch instead matches its resolved
+    value against case keys, and a case key is a literal: ``expression: review``
+    resolves to ``"review"`` and dispatches the ``review:`` case, and whitespace
+    strips to the ``""`` key. Both are valid, if constant, switches.
+
+    What is never evaluated is text plainly meant as an expression -- one that opens
+    by walking into a root ``_build_namespace`` supplies, such as ``inputs.mode``.
+    It is matched against the case keys as its own source text, so it falls through
+    to ``default`` on every run. An opening ``{{`` the interpolator emits verbatim
+    is flagged for the same reason, exactly as it is for a condition.
+
+    *case_keys* are the keys the switch declares, and they decide that last point
+    rather than the expression text alone. Text reading like a reference is still a
+    literal the author may have meant: ``expression: inputs.mode`` against a declared
+    ``inputs.mode:`` case dispatches it on every run, because ``SwitchStep.execute``
+    compares the resolved value -- for a braceless expression, this very text,
+    stripped -- against ``str(case_key)``. Such a switch is constant, not unevaluated,
+    so it is left alone. Only a reference matching no declared key can do nothing but
+    fall through, which is the authoring mistake this guards.
+    """
+    if not isinstance(expression, str):
+        return False
+    stripped = expression.strip()
+    if "{{" in stripped:
+        return _first_unclosable_block(stripped) == "verbatim"
+    if _NAMESPACE_REFERENCE.match(stripped) is None:
+        return False
+    return all(str(key) != stripped for key in case_keys)
+
+
 def condition_is_interpolated_to_text(condition: Any) -> bool:
     """True when *condition* holds ``{{ }}`` blocks but is spliced into text, not evaluated.
 
@@ -1121,6 +1159,14 @@ def _has_incomplete_operand(text: str) -> bool:
 # None, so a correction built on one turns a truthy condition false.
 _NAMESPACE_ROOTS = ("inputs", "steps", "item", "fan_in", "context")
 
+# Text that opens by walking into one of those roots: `inputs.mode`, `item[0]`.
+# Matching this is necessary but not sufficient to call a switch expression
+# unevaluated: the same text declared as a case key is a literal the switch really
+# does dispatch, so `switch_expression_is_never_evaluated` checks the keys too.
+_NAMESPACE_REFERENCE = re.compile(
+    r"(?:%s)(?:\.[\w-]|\[\d)" % "|".join(_NAMESPACE_ROOTS)
+)
+
 def _is_path_segment(segment: str) -> bool:
     """Whether _resolve_dot_path can walk *segment*: a name, or a name it indexes."""
     return bool(_PLAIN_SEGMENT.match(segment) or _INDEXED_SEGMENT.match(segment))

src/specify_cli/workflows/step/switch/__init__.py

@@ -5,7 +5,11 @@
 from typing import Any
 
 from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus
-from specify_cli.workflows.expressions import evaluate_expression
+from specify_cli.workflows.expressions import (
+    condition_has_malformed_expression_block,
+    evaluate_expression,
+    switch_expression_is_never_evaluated,
+)
 
 
 class SwitchStep(StepBase):
@@ -107,6 +111,44 @@ def validate(self, config: dict[str, Any]) -> list[str]:
                 f"Switch step {config.get('id', '?')!r} is missing "
                 f"'expression' field."
             )
+        # Presence is not enough. `expression` goes through the same
+        # `evaluate_expression` as a condition, so one written without braces comes
+        # back as its own source text: `expression: inputs.mode` matches no case key,
+        # falls through to `default` on every run -- or dispatches nothing at all when
+        # there is no default -- and still reports COMPLETED. `if`, `while` and
+        # `do-while` already reject that shape on their `condition`; this is the same
+        # fault on the same evaluator, one step type over.
+        #
+        # A switch matches on strings, which moves both boundaries a condition has. A
+        # braceless literal is a valid case key -- `expression: review` dispatches the
+        # `review:` case -- so only text that opens with a namespace reference is
+        # flagged, not every string without braces. And a composite key such as
+        # `{{ inputs.a }}-{{ inputs.b }}` is legitimate here even though the same
+        # shape would be a fault in a boolean condition.
+        #
+        # The declared keys settle the remaining ambiguity, which the expression text
+        # cannot: `inputs.mode` reads like a reference, but against a declared
+        # `inputs.mode:` case it is a literal that dispatches that case on every run,
+        # so it is passed through. Only a reference matching no declared key is left
+        # with nowhere to go but `default`.
+        elif switch_expression_is_never_evaluated(
+            config["expression"],
+            config["cases"].keys() if isinstance(config.get("cases"), dict) else (),
+        ):
+            errors.append(
+                f"Switch step {config.get('id', '?')!r}: 'expression' "
+                f"{config['expression']!r} has no usable '{{ }}' block, so it is "
+                "never evaluated: the literal text is matched against the case keys, "
+                "none of which declares it, so the switch falls through to 'default' "
+                "on every run."
+            )
+        elif condition_has_malformed_expression_block(config["expression"]):
+            errors.append(
+                f"Switch step {config.get('id', '?')!r}: 'expression' "
+                f"{config['expression']!r} opens a '{{' the interpolator cannot "
+                "close, so it falls back to the first raw '}}' and matches on a "
+                "truncated expression instead of the one written."
+            )
         # Every other control-flow step requires its branch payload: ``if``
         # requires ``then``, ``fan-out`` requires ``items`` and ``step``,
         # ``fan-in`` a non-empty ``wait_for``, ``gate`` a ``message``. Without

tests/test_workflows.py

@@ -3722,6 +3722,176 @@ def test_validate_invalid_cases_and_default(self):
         assert any("case 'a' must be a list" in e for e in errors)
         assert any("'default' must be a list" in e for e in errors)
 
+    def test_expression_without_a_block_is_rejected(self):
+        """`expression: inputs.mode` matches its own source text, not the input.
+
+        `evaluate_expression` only substitutes `{{ ... }}`, so the braceless form comes
+        back unchanged, matches no case key, and falls through to `default` on every
+        run while still reporting COMPLETED.
+        """
+        from specify_cli.workflows.step.switch import SwitchStep
+        from specify_cli.workflows.base import StepContext, StepStatus
+
+        config = {
+            "id": "route",
+            "expression": "inputs.mode",
+            "cases": {"review": [{"id": "r", "type": "command", "command": "echo"}]},
+            "default": [{"id": "d", "type": "command", "command": "echo"}],
+        }
+
+        # Ground truth first: this is what the step does with it today.
+        result = SwitchStep().execute(config, StepContext(inputs={"mode": "review"}))
+        assert result.status == StepStatus.COMPLETED
+        assert result.output["matched_case"] == "__default__"
+        assert result.output["expression_value"] == "inputs.mode"
+
+        errors = [e for e in SwitchStep().validate(config) if "'expression'" in e]
+        assert len(errors) == 1
+        assert "never evaluated" in errors[0]
+
+    def test_every_namespace_root_written_without_a_block_is_rejected(self):
+        """Each root `_build_namespace` supplies, walked into without braces."""
+        from specify_cli.workflows.step.switch import SwitchStep
+
+        cases = {"review": [{"id": "r", "type": "command", "command": "echo"}]}
+        for expression in (
+            "inputs.mode",
+            "steps.check.output.stdout",
+            "item.name",
+            "item[0]",
+            "fan_in.results",
+            "context.run_id",
+            "inputs.mode | default('review')",
+            "inputs.mode == 'review'",
+            "  inputs.mode  ",
+        ):
+            config = {"id": "route", "expression": expression, "cases": cases}
+            errors = [
+                e for e in SwitchStep().validate(config) if "'expression'" in e
+            ]
+            assert len(errors) == 1, expression
+            assert "never evaluated" in errors[0], expression
+
+    def test_a_literal_expression_stays_accepted(self):
+        """A switch matches on strings, and a case key is a literal.
+
+        So a braceless literal is a valid -- if constant -- switch, not a fault:
+        `expression: review` dispatches the `review:` case, and whitespace strips to
+        the `""` key. Only text that walks into a namespace root is flagged.
+        """
+        from specify_cli.workflows.step.switch import SwitchStep
+        from specify_cli.workflows.base import StepContext, StepStatus
+
+        review = [{"id": "r", "type": "command", "command": "echo"}]
+        blank = [{"id": "b", "type": "command", "command": "echo"}]
+        cases = {"review": review, "": blank, "approve me": review, "inputs": review}
+
+        # Ground truth first: each of these really does dispatch a declared case.
+        for expression, matched in (
+            ("review", "review"),
+            ("   ", ""),
+            ("approve me", "approve me"),
+            ("inputs", "inputs"),
+        ):
+            config = {"id": "route", "expression": expression, "cases": cases}
+            result = SwitchStep().execute(config, StepContext(inputs={}))
+            assert result.status == StepStatus.COMPLETED, expression
+            assert result.output["matched_case"] == matched, expression
+            assert [
+                e for e in SwitchStep().validate(config) if "'expression'" in e
+            ] == [], expression
+
+        # A name that merely starts like a root is not a reference into it.
+        config = {"id": "route", "expression": "inputsX.mode", "cases": cases}
+        assert [e for e in SwitchStep().validate(config) if "'expression'" in e] == []
+
+    def test_expression_with_an_unclosable_block_is_rejected(self):
+        """Different fault, different message: the block is evaluated, but truncated."""
+        from specify_cli.workflows.step.switch import SwitchStep
+
+        cases = {"review": [{"id": "r", "type": "command", "command": "echo"}]}
+        for expression in ("{{ inputs.x", "{{ inputs.missing | default('oops }}"):
+            config = {"id": "route", "expression": expression, "cases": cases}
+            errors = [
+                e for e in SwitchStep().validate(config) if "'expression'" in e
+            ]
+            assert len(errors) == 1, expression
+
+    def test_a_composite_key_expression_stays_accepted(self):
+        """A switch matches on strings, so more than one block is legitimate here.
+
+        This is the boundary that keeps the two condition predicates safe to reuse on
+        a non-boolean field: `{{ a }}-{{ b }}` is a composite case key, not a fault.
+        A literal `true` and the empty string are likewise ordinary case keys.
+        """
+        from specify_cli.workflows.step.switch import SwitchStep
+
+        cases = {"a-b": [{"id": "r", "type": "command", "command": "echo"}]}
+        for expression in ("{{ inputs.a }}-{{ inputs.b }}", "{{ inputs.mode }}", "true", ""):
+            config = {"id": "route", "expression": expression, "cases": cases}
+            assert [
+                e for e in SwitchStep().validate(config) if "'expression'" in e
+            ] == [], expression
+
+    def test_a_reference_declared_as_a_case_key_stays_accepted(self):
+        """The declared keys decide this, not the shape of the expression text.
+
+        `expression: inputs.mode` reads like a reference written without its braces,
+        but with an `inputs.mode:` case declared it is a literal the switch really
+        dispatches, on every run: `execute` matches the resolved value -- for a
+        braceless expression, this very text, stripped -- against `str(case_key)`.
+        That switch is constant, not unevaluated, so the guard leaves it alone.
+        """
+        from specify_cli.workflows.step.switch import SwitchStep
+        from specify_cli.workflows.base import StepContext, StepStatus
+
+        branch = [{"id": "r", "type": "command", "command": "echo"}]
+        for expression, declared in (
+            ("inputs.mode", "inputs.mode"),
+            ("item[0]", "item[0]"),
+            ("context.run_id", "context.run_id"),
+            # `execute` strips the resolved value before matching, so the padded
+            # form dispatches the unpadded key and must be accepted with it.
+            ("  inputs.mode  ", "inputs.mode"),
+        ):
+            cases = {declared: branch, "review": branch}
+            config = {"id": "route", "expression": expression, "cases": cases}
+
+            # Ground truth first: it really does dispatch the declared case.
+            result = SwitchStep().execute(config, StepContext(inputs={"mode": "review"}))
+            assert result.status == StepStatus.COMPLETED, expression
+            assert result.output["matched_case"] == declared, expression
+            assert [
+                e for e in SwitchStep().validate(config) if "'expression'" in e
+            ] == [], expression
+
+            # Withdraw just that key and the same text has nowhere to go but
+            # `default`, so the guard must fire again. This is what stops the
+            # exemption above from disarming it.
+            without = {"id": "route", "expression": expression, "cases": {"review": branch}}
+            fallthrough = SwitchStep().execute(without, StepContext(inputs={"mode": "review"}))
+            assert fallthrough.output["matched_case"] == "__default__", expression
+            errors = [e for e in SwitchStep().validate(without) if "'expression'" in e]
+            assert len(errors) == 1, expression
+            assert "never evaluated" in errors[0], expression
+
+    def test_a_reference_is_still_rejected_when_cases_are_unusable(self):
+        """A malformed or absent `cases` cannot exempt anything.
+
+        The exemption reads the declared keys, and a non-mapping `cases` has none --
+        it is itself an error `validate` reports separately. The expression must keep
+        its own error rather than fall silent because the keys could not be read.
+        """
+        from specify_cli.workflows.step.switch import SwitchStep
+
+        for cases in ({}, None, [], "review", 3):
+            config = {"id": "route", "expression": "inputs.mode"}
+            if cases is not None:
+                config["cases"] = cases
+            errors = [e for e in SwitchStep().validate(config) if "'expression'" in e]
+            assert len(errors) == 1, cases
+            assert "never evaluated" in errors[0], cases
+
 
 class TestWhileStep:
     """Test the while loop step type."""