fix(workflows): reject a condition that has no {{ }} block

src/specify_cli/workflows/expressions.py

@@ -690,3 +690,63 @@ def evaluate_condition(condition: str, context: Any) -> bool:
         if lower == "true":
             return True
     return bool(result)
+
+
+def condition_is_never_evaluated(condition: Any) -> bool:
+    """True when a string *condition* is silently treated as always-true text.
+
+    ``evaluate_condition`` resolves its argument through
+    ``evaluate_expression``, which only substitutes ``{{ ... }}`` blocks. A
+    string with no such block comes back unchanged, and — unless it reads
+    ``true``/``false`` — is then coerced by ``bool()``. So an expression
+    authored without the braces, e.g. ``condition: inputs.count > 100``, is
+    never evaluated at all: it is a non-empty string, so the ``if`` step always
+    takes ``then`` and a ``while``/``do-while`` step always runs to
+    ``max_iterations``.
+
+    That is the same silent-truthiness authoring mistake the step validators
+    already reject for a list/dict/number condition, and it is easy to write:
+    GitHub Actions accepts a bare expression in ``if:``.
+
+    An empty/whitespace string is excluded — it coerces to ``False``, which is
+    a definite answer rather than a silent always-true.
+    """
+    if not isinstance(condition, str):
+        return False
+    stripped = condition.strip()
+    if not stripped or stripped.lower() in ("true", "false"):
+        return False
+    open_at = stripped.find("{{")
+    if open_at == -1:
+        return True
+    # An opening ``{{`` with no ``}}`` anywhere after it is never substituted
+    # either: ``_interpolate_expressions`` takes its ``raw_close == -1`` branch
+    # and appends the tail verbatim. So ``{{ inputs.count > 100`` -- and the
+    # reversed ``}} inputs.count > 100 {{``, whose only ``{{`` is last -- come
+    # back unchanged and are just as silently true as a brace-less string.
+    return stripped.find("}}", open_at + 2) == -1
+
+
+def format_condition_correction(condition: Any) -> str:
+    """Render *condition* wrapped in ``{{ }}`` as a quoted, paste-ready YAML scalar.
+
+    The validators hand this back as the corrected form, so it has to survive a
+    round trip through a YAML parser. A plain ``"{{ ... }}"`` does not: a
+    condition holding a double quote (``inputs.name == "zzz"``) closes the
+    scalar early and the workflow file no longer loads. Quoting is therefore
+    chosen from the content -- double by default, single when the expression
+    itself contains a double quote, and double with backslash escapes when it
+    contains both.
+
+    A stray delimiter is dropped rather than nested: ``{{ inputs.count > 100``
+    corrects to ``"{{ inputs.count > 100 }}"``, not to a doubled ``{{ {{ ... }} }}``.
+    """
+    core = str(condition).strip()
+    core = re.sub(r"^\s*(\{\{|\}\})\s*", "", core)
+    core = re.sub(r"\s*(\{\{|\}\})\s*$", "", core).strip()
+    wrapped = "{{ " + core + " }}"
+    if '"' not in wrapped and "\\" not in wrapped:
+        return '"' + wrapped + '"'
+    if "'" not in wrapped:
+        return "'" + wrapped + "'"
+    return '"' + wrapped.replace("\\", "\\\\").replace('"', '\\"') + '"'

src/specify_cli/workflows/steps/do_while/__init__.py

@@ -5,6 +5,10 @@
 from typing import Any
 
 from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus
+from specify_cli.workflows.expressions import (
+    condition_is_never_evaluated,
+    format_condition_correction,
+)
 
 
 class DoWhileStep(StepBase):
@@ -88,6 +92,20 @@ def validate(self, config: dict[str, Any]) -> list[str]:
                 f"Do-while step {config.get('id', '?')!r}: 'condition' must be a "
                 f"string or boolean, got {type(config['condition']).__name__}."
             )
+        elif condition_is_never_evaluated(config["condition"]):
+            # A string condition with no ``{{ }}`` block is never evaluated:
+            # evaluate_expression() returns it unchanged and bool() then makes
+            # any non-empty text true. `condition: inputs.count > 100` reads as
+            # a real comparison but always takes every iteration. This is the same
+            # silent-truthiness mistake the list/dict branch above rejects, and
+            # GitHub Actions accepts a bare expression in `if:`, so it is easy
+            # to write by habit.
+            errors.append(
+                f"Do-while step {config.get('id', '?')!r}: 'condition' "
+                f"{config['condition']!r} has no complete '{{{{ }}}}' block, so it is "
+                "never evaluated and is always true. Wrap the expression: "
+                + format_condition_correction(config["condition"]) + "."
+            )
         max_iter = config.get("max_iterations")
         if max_iter is not None:
             # bool is a subclass of int, so isinstance(True, int) is True and

src/specify_cli/workflows/steps/if_then/__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_condition
+from specify_cli.workflows.expressions import (
+    condition_is_never_evaluated,
+    format_condition_correction,
+    evaluate_condition,
+)
 
 
 class IfThenStep(StepBase):
@@ -79,6 +83,20 @@ def validate(self, config: dict[str, Any]) -> list[str]:
                 f"If step {config.get('id', '?')!r}: 'condition' must be a "
                 f"string or boolean, got {type(config['condition']).__name__}."
             )
+        elif condition_is_never_evaluated(config["condition"]):
+            # A string condition with no ``{{ }}`` block is never evaluated:
+            # evaluate_expression() returns it unchanged and bool() then makes
+            # any non-empty text true. `condition: inputs.count > 100` reads as
+            # a real comparison but always takes ``then``. This is the same
+            # silent-truthiness mistake the list/dict branch above rejects, and
+            # GitHub Actions accepts a bare expression in `if:`, so it is easy
+            # to write by habit.
+            errors.append(
+                f"If step {config.get('id', '?')!r}: 'condition' "
+                f"{config['condition']!r} has no complete '{{{{ }}}}' block, so it is "
+                "never evaluated and is always true. Wrap the expression: "
+                + format_condition_correction(config["condition"]) + "."
+            )
         if "then" not in config:
             errors.append(
                 f"If step {config.get('id', '?')!r} is missing 'then' field."

src/specify_cli/workflows/steps/while_loop/__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_condition
+from specify_cli.workflows.expressions import (
+    condition_is_never_evaluated,
+    format_condition_correction,
+    evaluate_condition,
+)
 
 
 class WhileStep(StepBase):
@@ -97,6 +101,20 @@ def validate(self, config: dict[str, Any]) -> list[str]:
                 f"While step {config.get('id', '?')!r}: 'condition' must be a "
                 f"string or boolean, got {type(config['condition']).__name__}."
             )
+        elif condition_is_never_evaluated(config["condition"]):
+            # A string condition with no ``{{ }}`` block is never evaluated:
+            # evaluate_expression() returns it unchanged and bool() then makes
+            # any non-empty text true. `condition: inputs.count > 100` reads as
+            # a real comparison but always takes every iteration. This is the same
+            # silent-truthiness mistake the list/dict branch above rejects, and
+            # GitHub Actions accepts a bare expression in `if:`, so it is easy
+            # to write by habit.
+            errors.append(
+                f"While step {config.get('id', '?')!r}: 'condition' "
+                f"{config['condition']!r} has no complete '{{{{ }}}}' block, so it is "
+                "never evaluated and is always true. Wrap the expression: "
+                + format_condition_correction(config["condition"]) + "."
+            )
         max_iter = config.get("max_iterations")
         if max_iter is not None:
             # bool is a subclass of int, so isinstance(True, int) is True and

tests/unit/test_condition_expression_block.py

@@ -0,0 +1,150 @@
+"""A string condition with no ``{{ }}`` block is never evaluated (always true)."""
+
+import pytest
+import yaml
+
+from specify_cli.workflows.base import StepContext
+from specify_cli.workflows.expressions import (
+    condition_is_never_evaluated,
+    evaluate_condition,
+    format_condition_correction,
+)
+from specify_cli.workflows.steps.do_while import DoWhileStep
+from specify_cli.workflows.steps.if_then import IfThenStep
+from specify_cli.workflows.steps.while_loop import WhileStep
+
+STEP_CLASSES = [IfThenStep, WhileStep, DoWhileStep]
+
+
+@pytest.mark.parametrize(
+    "condition",
+    ["inputs.count > 100", "inputs.name == 'zzz'", "inputs.count < 3"],
+)
+def test_brace_less_condition_is_always_true_at_runtime(condition):
+    """The behaviour the validator now warns about, pinned so it cannot drift."""
+    ctx = StepContext(inputs={"count": 5, "name": "abc"})
+    # Same expression with braces resolves to its real (false) value...
+    assert evaluate_condition("{{ " + condition + " }}", ctx) is False
+    # ...without them it is only non-empty text, so bool() makes it true.
+    assert evaluate_condition(condition, ctx) is True
+
+
+@pytest.mark.parametrize("step_cls", STEP_CLASSES)
+def test_validator_rejects_condition_without_expression_block(step_cls):
+    config = {"id": "s1", "condition": "inputs.count > 100", "then": [], "steps": []}
+    errors = [e for e in step_cls().validate(config) if "never evaluated" in e]
+    assert len(errors) == 1
+    assert "inputs.count > 100" in errors[0]
+    # The message hands back the corrected form.
+    assert '"{{ inputs.count > 100 }}"' in errors[0]
+
+
+@pytest.mark.parametrize("step_cls", STEP_CLASSES)
+@pytest.mark.parametrize(
+    "condition",
+    ["{{ inputs.count > 100 }}", "true", "false", "TRUE", True, False, "", "   "],
+)
+def test_validator_accepts_evaluated_and_literal_conditions(step_cls, condition):
+    """No false positives: braces, boolean literals and bools stay valid."""
+    config = {"id": "s1", "condition": condition, "then": [], "steps": []}
+    assert not [e for e in step_cls().validate(config) if "never evaluated" in e]
+
+
+@pytest.mark.parametrize(
+    ("value", "expected"),
+    [
+        ("inputs.count > 100", True),
+        ("{{ inputs.count > 100 }}", False),
+        ("prefix {{ inputs.a }} suffix", False),
+        ("true", False),
+        ("False", False),
+        ("", False),
+        ("   ", False),
+        (True, False),
+        (["a"], False),
+        (3, False),
+    ],
+)
+def test_condition_is_never_evaluated(value, expected):
+    assert condition_is_never_evaluated(value) is expected
+
+
+# --- An unterminated ``{{`` is the same defect, not a different one -----------
+#
+# ``_interpolate_expressions`` substitutes nothing when no ``}}`` follows the
+# opening ``{{`` (its ``raw_close == -1`` branch appends the tail verbatim), so
+# ``{{ inputs.count > 100`` is returned unchanged and coerced to true exactly
+# like a brace-less string.
+
+BACKSLASH = chr(92)
+
+NEVER_EVALUATED = [
+    "inputs.count > 100",        # no delimiter at all
+    "{{ inputs.count > 100",     # opened, never closed
+    "}} inputs.count > 100 {{",  # reversed: the only '{{' is last
+]
+
+
+@pytest.mark.parametrize("condition", NEVER_EVALUATED)
+def test_incomplete_block_is_silently_true_and_is_flagged(condition):
+    ctx = StepContext(inputs={"count": 5, "name": "abc"})
+    assert evaluate_condition(condition, ctx) is True
+    assert condition_is_never_evaluated(condition) is True
+
+
+@pytest.mark.parametrize(
+    "condition",
+    [
+        "{{ inputs.count > 100 }}",
+        "{{ inputs.a }} and {{ inputs.b }}",
+        "{{ inputs.text | default('}}') }}",  # literal '}}' inside an argument
+    ],
+)
+def test_complete_block_is_not_flagged(condition):
+    assert condition_is_never_evaluated(condition) is False
+
+
+# --- The suggested correction has to survive a YAML round trip ---------------
+
+TRICKY_CONDITIONS = [
+    "inputs.count > 100",
+    'inputs.name == "zzz"',                                   # double quote
+    "inputs.name == 'zzz'",                                   # single quote
+    'inputs.a == "x" and inputs.b == \'y\'',                  # both
+    "inputs.path == 'C:" + BACKSLASH + "tmp'",                # backslash
+    'inputs.path == "C:' + BACKSLASH + 'tmp"',                # backslash + quote
+    '{{ inputs.name == "zzz"',                                # incomplete + quote
+    "}} inputs.count > 100 {{",
+]
+
+
+@pytest.mark.parametrize("condition", TRICKY_CONDITIONS)
+def test_correction_is_valid_yaml_and_round_trips(condition):
+    """A correction the author cannot paste into their workflow is no correction."""
+    loaded = yaml.safe_load("condition: " + format_condition_correction(condition))
+    stripped = condition.strip().lstrip("{}").rstrip("{}").strip()
+    assert loaded["condition"] == "{{ " + stripped + " }}"
+
+
+@pytest.mark.parametrize("condition", TRICKY_CONDITIONS)
+def test_correction_does_not_trip_the_validator_again(condition):
+    loaded = yaml.safe_load("condition: " + format_condition_correction(condition))
+    assert condition_is_never_evaluated(loaded["condition"]) is False
+
+
+@pytest.mark.parametrize("condition", ["{{ inputs.count > 100", "}} a > 1 {{"])
+def test_correction_replaces_a_stray_delimiter_instead_of_nesting_one(condition):
+    corrected = format_condition_correction(condition)
+    assert "{{ {{" not in corrected and "}} }}" not in corrected
+    assert corrected.count("{{") == 1 and corrected.count("}}") == 1
+
+
+@pytest.mark.parametrize("step_cls", STEP_CLASSES)
+@pytest.mark.parametrize("condition", ['inputs.name == "zzz"', "{{ inputs.count > 100"])
+def test_validator_correction_is_yaml_safe(step_cls, condition):
+    config = {"id": "s1", "condition": condition, "then": [], "steps": []}
+    errors = [e for e in step_cls().validate(config) if "never evaluated" in e]
+    assert len(errors) == 1
+    suggested = errors[0].split("Wrap the expression: ", 1)[1].rstrip(".")
+    loaded = yaml.safe_load("condition: " + suggested)
+    assert condition_is_never_evaluated(loaded["condition"]) is False