diff --git a/src/specify_cli/workflows/steps/do_while/__init__.py b/src/specify_cli/workflows/steps/do_while/__init__.py index 024ced55b5..e6e013be96 100644 --- a/src/specify_cli/workflows/steps/do_while/__init__.py +++ b/src/specify_cli/workflows/steps/do_while/__init__.py @@ -98,6 +98,14 @@ def validate(self, config: dict[str, Any]) -> list[str]: f"Do-while step {config.get('id', '?')!r}: " f"'max_iterations' must be an integer >= 1." ) + if "steps" not in config: + # This step's own docstring promises "The first invocation always + # returns the nested steps for execution" -- with no body it + # validated clean and then returned none, so the loop never ran even + # once. See the matching guard in the ``while`` step. + errors.append( + f"Do-while step {config.get('id', '?')!r} is missing 'steps' field." + ) nested = config.get("steps", []) if not isinstance(nested, list): errors.append( diff --git a/src/specify_cli/workflows/steps/while_loop/__init__.py b/src/specify_cli/workflows/steps/while_loop/__init__.py index e80b93d7f2..97b60fdd08 100644 --- a/src/specify_cli/workflows/steps/while_loop/__init__.py +++ b/src/specify_cli/workflows/steps/while_loop/__init__.py @@ -107,6 +107,18 @@ def validate(self, config: dict[str, Any]) -> list[str]: f"While step {config.get('id', '?')!r}: " f"'max_iterations' must be an integer >= 1." ) + if "steps" not in config: + # A loop with no body is never what the author meant, but it used to + # validate clean and then report COMPLETED at run time while + # returning no next_steps -- so the engine's ``if result.next_steps:`` + # block never fired and the loop the workflow is built around never + # ran once. The mistype is easy: fan-out's payload key is the + # singular ``step:``, so writing ``step:`` on a ``while`` produced a + # silent no-op. ``if`` already requires ``then`` and fan-out already + # requires both ``items`` and ``step``; require a body here too. + errors.append( + f"While step {config.get('id', '?')!r} is missing 'steps' field." + ) nested = config.get("steps", []) if not isinstance(nested, list): errors.append( diff --git a/tests/test_workflows.py b/tests/test_workflows.py index afd70adecf..1ba00d9f9c 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -3411,6 +3411,33 @@ def test_validate_missing_fields(self): assert any("missing 'condition'" in e for e in errors) # max_iterations is optional (defaults to 10) + def test_validate_requires_steps_body(self): + """A while loop with no body must be rejected, not silently a no-op. + + Without this, ``step:`` written instead of ``steps:`` -- an easy slip, + since fan-out's payload key really is the singular ``step:`` -- passed + ``specify workflow validate`` with zero errors, and then reported + COMPLETED at run time while returning no ``next_steps``, so the loop + never ran even once. + """ + from specify_cli.workflows.base import StepContext, StepStatus + from specify_cli.workflows.steps.while_loop import WhileStep + + step = WhileStep() + config = { + "id": "retry", + "condition": "true", + # The mistype: singular 'step' instead of 'steps'. + "step": {"id": "x", "type": "command", "command": "echo"}, + } + errors = step.validate(config) + assert errors == ["While step 'retry' is missing 'steps' field."], errors + + # Demonstrates why it matters: execution is a silent no-op. + result = step.execute(config, StepContext()) + assert result.status == StepStatus.COMPLETED + assert result.next_steps == [] + @pytest.mark.parametrize("bad", [["a", "b"], {"k": "v"}, 5, 1.5]) def test_validate_rejects_non_string_non_bool_condition(self, bad): from specify_cli.workflows.steps.while_loop import WhileStep @@ -3543,6 +3570,25 @@ def test_validate_missing_fields(self): assert any("missing 'condition'" in e for e in errors) # max_iterations is optional (defaults to 10) + def test_validate_requires_steps_body(self): + """A do-while with no body must be rejected, not silently a no-op. + + The step's own docstring promises "The first invocation always returns + the nested steps for execution" -- with no body it validated clean and + then returned none, so the loop never ran even once. + """ + from specify_cli.workflows.base import StepContext, StepStatus + from specify_cli.workflows.steps.do_while import DoWhileStep + + step = DoWhileStep() + config = {"id": "refine", "condition": "true", "max_iterations": 3} + errors = step.validate(config) + assert errors == ["Do-while step 'refine' is missing 'steps' field."], errors + + result = step.execute(config, StepContext()) + assert result.status == StepStatus.COMPLETED + assert result.next_steps == [] + @pytest.mark.parametrize("bad", [["a", "b"], {"k": "v"}, 5, 1.5]) def test_validate_rejects_non_string_non_bool_condition(self, bad): from specify_cli.workflows.steps.do_while import DoWhileStep