diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index fe8098f321..f305cb042d 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -857,6 +857,30 @@ def condition_is_never_evaluated(condition: Any) -> bool: return _first_unclosable_block(stripped) == "verbatim" +def switch_expression_is_never_evaluated(expression: 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. + """ + if not isinstance(expression, str): + return False + stripped = expression.strip() + if "{{" in stripped: + return _first_unclosable_block(stripped) == "verbatim" + return _NAMESPACE_REFERENCE.match(stripped) is not None + + def condition_is_interpolated_to_text(condition: Any) -> bool: """True when *condition* holds ``{{ }}`` blocks but is spliced into text, not evaluated. @@ -1086,6 +1110,11 @@ 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]`. +_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/steps/switch/__init__.py b/src/specify_cli/workflows/steps/switch/__init__.py index 8a2e4b343e..36892d111b 100644 --- a/src/specify_cli/workflows/steps/switch/__init__.py +++ b/src/specify_cli/workflows/steps/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,34 @@ 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. + elif switch_expression_is_never_evaluated(config["expression"]): + 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, " + "which 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 2c7141e954..9567c8bbac 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -3665,6 +3665,117 @@ 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.steps.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.steps.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.steps.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.steps.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.steps.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 + class TestWhileStep: """Test the while loop step type."""