diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 426f71af9b..981eec058d 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/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)) diff --git a/src/specify_cli/workflows/step/switch/__init__.py b/src/specify_cli/workflows/step/switch/__init__.py index 8a2e4b343e..b02af542fb 100644 --- a/src/specify_cli/workflows/step/switch/__init__.py +++ b/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 diff --git a/tests/test_workflows.py b/tests/test_workflows.py index d8abcc0f55..845fb46436 100644 --- a/tests/test_workflows.py +++ b/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."""