From 37ea908d5c7fb3be6a88ea869ec9cdf2e9933019 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Mon, 24 Aug 2026 15:53:23 +0700 Subject: [PATCH] fix(workflows): reject a switch expression that is never evaluated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SwitchStep.validate` checked only that `expression` is present. It goes through the same `evaluate_expression` as a condition, so one written without braces comes back as its own source text: expression: inputs.mode -> expression_value: "inputs.mode" matched_case: "__default__" status: COMPLETED It 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. That is the "silent empty result + COMPLETED" wiring bug this file's own `cases:` guard was written to prevent, on the field one line above it. `if`, `while` and `do-while` already run these two predicates on their `condition`. This reuses them rather than writing a third scan. Only those two apply. A switch matches on strings, so 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 — there is a test pinning that, and a literal `true` and the empty string stay accepted as ordinary case keys for the same reason. --- .../workflows/steps/switch/__init__.py | 31 ++++++++++- tests/test_workflows.py | 55 +++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/steps/switch/__init__.py b/src/specify_cli/workflows/steps/switch/__init__.py index 8a2e4b343e..ff597f9069 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, + condition_is_never_evaluated, + evaluate_expression, +) class SwitchStep(StepBase): @@ -107,6 +111,31 @@ 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. + # + # Only these two checks apply. A switch matches on strings, so 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 condition_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 d599f3c6a4..c8325b4eed 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -3403,6 +3403,61 @@ 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_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."""