From ae962f3bd53c37c421554fe80ce2418c3b6e7fb9 Mon Sep 17 00:00:00 2001
From: Tymur <48928509+kartsan03@users.noreply.github.com>
Date: Mon, 12 Oct 2026 11:52:39 +0000
Subject: [PATCH] fix(workflows): skip the right side of and/or when the left
 side decides

`and` and `or` evaluated both operands before combining them, so a guard
could not protect the operand after it. With an empty optional input,
`inputs.payload != '' and (inputs.payload | from_json | length)` still ran
`from_json` on the empty string and failed the run with "invalid JSON".

Return as soon as the left operand decides the result. The validation
probes behind condition corrections (`_evaluator_rejects`,
`_collect_leaves`) now pass a `_ProbeNamespace` as the outer namespace and
keep evaluating both operands, so an unknown name or misused filter in a
skipped operand is still reported.

Assisted-by: Codex CLI (model: gpt-6-astra, autonomous)
Assisted-by: Claude Code (model: claude-opus-5-5, autonomous)
---
 docs/reference/workflows.md                   |  2 +
 src/specify_cli/workflows/expressions.py      | 18 +++++++--
 .../specify_cli/workflows/test_command_run.py | 39 +++++++++++++++++++
 tests/test_workflows.py                       | 29 ++++++++++++++
 tests/unit/test_condition_expression_block.py | 15 ++++---
 5 files changed, 94 insertions(+), 9 deletions(-)

diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md
index 93377f4bfb..07f3cb3bf2 100644
--- a/docs/reference/workflows.md
+++ b/docs/reference/workflows.md
@@ -1052,6 +1052,8 @@ Notes on the newer filters:
   condition: "{{ inputs.items | length }}"   # 0 is False, any non-zero count is True
   ```
 
+`and` and `or` return `true` or `false` and evaluate their right side only when the left side does not decide the result: `and` skips it when the left side is false, and `or` skips it when the left side is true. A condition can therefore guard a filter that would fail on an empty value. `{{ inputs.payload != '' and (inputs.payload | from_json | length) }}` is false for an empty optional `payload` instead of failing the run on invalid JSON. When the right side is evaluated, its errors still fail the run.
+
 Example:
 
 ```yaml
diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py
index 6ac4ab18c5..3cbce53185 100644
--- a/src/specify_cli/workflows/expressions.py
+++ b/src/specify_cli/workflows/expressions.py
@@ -884,12 +884,17 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any:
     or_idx = _find_top_level(expr, " or ")
     if or_idx != -1:
         left = _evaluate_simple_expression(expr[:or_idx].strip(), namespace)
+        # Validation probes must still visit operands that runtime can skip.
+        if bool(left) and not isinstance(namespace, _ProbeNamespace):
+            return True
         right = _evaluate_simple_expression(expr[or_idx + 4:].strip(), namespace)
         return bool(left) or bool(right)
 
     and_idx = _find_top_level(expr, " and ")
     if and_idx != -1:
         left = _evaluate_simple_expression(expr[:and_idx].strip(), namespace)
+        if not bool(left) and not isinstance(namespace, _ProbeNamespace):
+            return False
         right = _evaluate_simple_expression(expr[and_idx + 5:].strip(), namespace)
         return bool(left) and bool(right)
 
@@ -1392,6 +1397,11 @@ class _ProbeNamespace(dict):
     real inputs. Deliberately *not* resolving leaves to a sentinel value: a probe
     that answers every lookup also answers ``inputs.count+1``, which is the
     malformed shape the probe is meant to expose.
+
+    The probes also pass one as the outer namespace, which tells
+    ``_evaluate_simple_expression`` to evaluate both operands of ``and``/``or``
+    instead of short-circuiting, so a leaf or filter in a skipped operand is
+    still checked.
     """
 
     def __missing__(self, key: str) -> "_ProbeNamespace":  # noqa: UP037  # pragma: no cover
@@ -1414,7 +1424,7 @@ def _evaluator_rejects(text: str) -> str | None:
     """
     try:
         _evaluate_simple_expression(
-            text, {root: _ProbeNamespace() for root in _NAMESPACE_ROOTS}
+            text, _ProbeNamespace({root: _ProbeNamespace() for root in _NAMESPACE_ROOTS})
         )
     except ValueError as exc:
         message = str(exc)
@@ -1436,8 +1446,8 @@ def _collect_leaves(text: str) -> list[str]:
 
     Runs the same probe ``_evaluator_rejects`` uses, with the leaf sink armed.
     Literals never reach the dot-path resolution, and operands, filter arguments
-    and list elements all do -- ``_evaluate_simple_expression`` evaluates both
-    sides of ``or``/``and`` eagerly rather than short-circuiting, so a leaf is
+    and list elements all do -- probe evaluation visits both sides of
+    ``or``/``and`` eagerly rather than short-circuiting, so a leaf is
     recorded whatever the other side is worth.
 
     A probe run can still raise on its own placeholder values, which is what
@@ -1450,7 +1460,7 @@ def _collect_leaves(text: str) -> list[str]:
     token = _leaf_sink.set(leaves)
     try:
         _evaluate_simple_expression(
-            text, {root: _ProbeNamespace() for root in _NAMESPACE_ROOTS}
+            text, _ProbeNamespace({root: _ProbeNamespace() for root in _NAMESPACE_ROOTS})
         )
     except Exception:  # noqa: BLE001 - probe values, reported by _evaluator_rejects
         pass
diff --git a/tests/specify_cli/workflows/test_command_run.py b/tests/specify_cli/workflows/test_command_run.py
index 432ed6a48a..d43b76627f 100644
--- a/tests/specify_cli/workflows/test_command_run.py
+++ b/tests/specify_cli/workflows/test_command_run.py
@@ -1392,3 +1392,42 @@ def test_run_json_invalid_input_not_on_stdout(self, tmp_path):
         assert result.exit_code == 1
         assert "Invalid input format" not in result.stdout
         assert "Invalid input format" in result.stderr
+
+
+@pytest.mark.parametrize(("condition", "empty_branch"), [
+    ("inputs.payload != '' and (inputs.payload | from_json | length)", "absent"),
+    ("inputs.payload == '' or (inputs.payload | from_json | length)", "provided"),
+])
+@pytest.mark.parametrize("payload", ["", "[1]", "broken"])
+def test_run_short_circuits_optional_json(tmp_path, monkeypatch, condition, empty_branch, payload):
+    from typer.testing import CliRunner
+    from specify_cli import app
+
+    workflow = tmp_path / "optional.yml"
+    workflow.write_text(yaml.safe_dump({
+        "schema_version": "1.0",
+        "workflow": {"id": "optional", "name": "Optional payload", "version": "1.0.0"},
+        "inputs": {"payload": {"type": "string", "default": ""}},
+        "steps": [{
+            "id": "check", "type": "if", "condition": "{{ " + condition + " }}",
+            "then": [{"id": "provided", "type": "shell", "run": "echo provided"}],
+            "else": [{"id": "absent", "type": "shell", "run": "echo absent"}],
+        }],
+    }), encoding="utf-8")
+    monkeypatch.chdir(tmp_path)
+    result = CliRunner().invoke(app, [
+        "workflow", "run", str(workflow), "--json", "--input", f"payload={payload}",
+    ])
+    if payload == "broken":
+        assert result.exit_code == 1
+        assert "from_json: invalid JSON" in result.output
+        return
+    assert result.exit_code == 0, result.output
+    outcome = json.loads(result.stdout)
+    assert outcome["status"] == "completed"
+    state = json.loads((
+        tmp_path / ".specify" / "workflows" / "runs" / outcome["run_id"] / "state.json"
+    ).read_text(encoding="utf-8"))
+    branch = "provided" if payload else empty_branch
+    assert state["step_results"][branch]["output"]["stdout"].strip() == branch
+    assert set(state["step_results"]) == {"check", branch}
diff --git a/tests/test_workflows.py b/tests/test_workflows.py
index a4ea5d1786..82228a6ae9 100644
--- a/tests/test_workflows.py
+++ b/tests/test_workflows.py
@@ -387,6 +387,35 @@ def test_boolean_or(self):
         ctx = StepContext(inputs={"a": False, "b": True})
         assert evaluate_expression("{{ inputs.a or inputs.b }}", ctx) is True
 
+    @pytest.mark.parametrize(("expression", "expected"), [
+        ("inputs.payload != '' and (inputs.payload | from_json | length)", False),
+        ("inputs.payload == '' or (inputs.payload | from_json | length)", True),
+        ("false and true or true", True),
+        ("true or (false and (inputs.payload | from_json))", True),
+    ])
+    def test_boolean_short_circuits(self, expression, expected):
+        from specify_cli.workflows.expressions import evaluate_expression
+        from specify_cli.workflows.base import StepContext
+
+        assert evaluate_expression(
+            "{{ " + expression + " }}", StepContext(inputs={"payload": ""})
+        ) is expected
+
+    @pytest.mark.parametrize("expression", [
+        "true and (inputs.payload | from_json)",
+        "false or (inputs.payload | from_json)",
+        "(inputs.payload | from_json) and false",
+        "(inputs.payload | from_json) or true",
+    ])
+    def test_boolean_required_operand_still_raises(self, expression):
+        from specify_cli.workflows.expressions import evaluate_expression
+        from specify_cli.workflows.base import StepContext
+
+        with pytest.raises(ValueError, match="from_json: invalid JSON"):
+            evaluate_expression(
+                "{{ " + expression + " }}", StepContext(inputs={"payload": ""})
+            )
+
     def test_list_literal_preserves_quoted_commas(self):
         from specify_cli.workflows.expressions import evaluate_expression
         from specify_cli.workflows.base import StepContext
diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py
index 23c80ae9e2..421b15e99b 100644
--- a/tests/unit/test_condition_expression_block.py
+++ b/tests/unit/test_condition_expression_block.py
@@ -888,16 +888,21 @@ def test_the_sink_is_cleared_even_when_the_probe_raises():
 
 
 def test_both_sides_of_a_boolean_are_reported():
-    """The load-bearing property: `_evaluate_simple_expression` evaluates both
-    operands of `and`/`or` and only then combines them. If it ever
-    short-circuits, the gate would stop seeing the right-hand operand and go
-    quietly blind -- so assert it here rather than rely on it silently.
-    """
+    """Probes must visit both operands even when runtime short-circuits."""
     assert _collect_leaves("inputs.a or bogus") == ["inputs.a", "bogus"]
+    assert _collect_leaves("true or bogus") == ["bogus"]
     assert _collect_leaves("false and bogus") == ["bogus"]
     assert _unresolvable_term("false and bogus") is not None
 
 
+@pytest.mark.parametrize("template", ["true or ({})", "false and ({})", "true or (false and ({}))"])
+@pytest.mark.parametrize("filtered", ["inputs.x | nosuchfilter", "inputs.x | join", "inputs.x | length(1)"])
+def test_filter_probe_checks_short_circuited_operands(template, filtered):
+    expression = template.format(filtered)
+    assert _evaluator_rejects(expression) is not None
+    assert "evaluator rejects" in format_condition_remediation(expression)
+
+
 def test_leaves_seen_before_a_probe_error_are_kept():
     """The probe hands `join` a placeholder and it raises. The leaves reached
     before that are real, so discarding them would lose `bogus` -- the filter