diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 91a9ac2ad5..4eb20ef837 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -193,6 +193,20 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]: f"Must be 'string', 'number', or 'boolean'." ) + # ``enum`` must be a list. Checked here — not only via the + # ``_coerce_input`` call below — because that call is reached only + # when a ``default`` is present, and the ``integration: auto`` case + # strips ``enum`` before coercing; a scalar/string ``enum`` on an + # input with no default (or the auto-integration default) would + # otherwise slip through here and then crash ``_resolve_inputs`` with + # a raw ``TypeError`` at run time. ``None`` means "no enum". + enum_values = input_def.get("enum") + if enum_values is not None and not isinstance(enum_values, list): + errors.append( + f"Input {input_name!r} has invalid 'enum': must be a list, " + f"got {type(enum_values).__name__}." + ) + # Validate the default eagerly so authoring mistakes (e.g. a # default not in the declared enum, or a non-numeric default for # a number input) surface at install/validation time instead of @@ -201,13 +215,28 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]: # enum-membership check is exempted for that exact case — the # declared type is still enforced (e.g. ``type: number`` paired # with ``default: "auto"`` is still rejected). + enum_is_valid = enum_values is None or isinstance(enum_values, list) if "default" in input_def: default_value = input_def["default"] is_auto_integration = ( input_name == "integration" and default_value == "auto" ) + # Strip ``enum`` from the definition handed to ``_coerce_input`` + # when either: + # * this is the auto-integration sentinel (enum-membership is + # a runtime concern, exempted for ``"auto"``), or + # * the ``enum`` is malformed (non-list) and already reported + # above — leaving it in would make ``_coerce_input`` re-raise + # the same enum-shape error re-framed as an "invalid default" + # (a confusing duplicate). + # Removing *only* ``enum`` (rather than skipping the check + # entirely) preserves the default's type validation: a + # ``type: string`` input with ``default: 5, enum: 5`` still + # reports the wrong-typed default alongside the enum error, + # instead of hiding it. + strip_enum = is_auto_integration or not enum_is_valid validation_input_def: dict[str, Any] = input_def - if is_auto_integration and "enum" in input_def: + if strip_enum and "enum" in input_def: validation_input_def = { key: value for key, value in input_def.items() @@ -1381,11 +1410,18 @@ def _resolve_inputs( # definition (``string`` rejects non-strings, ``number`` rejects # bools and uncoercible values, ``boolean`` rejects non-bools), # so ill-typed values still fail fast here. + # + # ``execute()`` accepts unvalidated definitions, so a malformed + # (non-list) ``enum`` can reach here. Only strip a *list* ``enum``: + # a scalar/string ``enum`` must stay in the definition so + # ``_coerce_input`` raises the clean shape ``ValueError`` instead of + # being silently exempted by the ``auto`` membership skip (which + # would otherwise let ``enum: 5`` resolve successfully). coerce_input_def = input_def if ( name == "integration" and value == "auto" - and "enum" in input_def + and isinstance(input_def.get("enum"), list) ): coerce_input_def = { key: val @@ -1431,6 +1467,22 @@ def _coerce_input( input_type = input_def.get("type", "string") enum_values = input_def.get("enum") + # ``enum`` must be a list. A scalar (``enum: 5``, ``enum: true``) makes + # the ``value not in enum_values`` membership test below raise a raw + # ``TypeError`` ("argument of type 'int' is not ... iterable"), which + # escapes ``validate_workflow``'s ``except ValueError`` and breaks its + # "return errors, never raise" contract — and crashes ``_resolve_inputs`` + # outright at run time. A bare string is just as wrong: ``value in "abc"`` + # is a silent substring/character test, not enum membership. Require a + # list so both forms fail fast with a clear message. ``None`` means "no + # enum" and is left alone. + if enum_values is not None and not isinstance(enum_values, list): + msg = ( + f"Input {name!r} has invalid 'enum': must be a list, got " + f"{type(enum_values).__name__}." + ) + raise ValueError(msg) + if input_type == "number": # Reject bools explicitly: ``bool`` is a subclass of ``int`` so # ``float(True)`` succeeds and would silently coerce a YAML diff --git a/tests/test_workflows.py b/tests/test_workflows.py index b1d9841535..2369e88d87 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -4248,6 +4248,108 @@ def test_coerce_number_input_rejects_infinity_cleanly(self): assert WorkflowEngine._coerce_input("count", 5.0, {"type": "number"}) == 5 assert WorkflowEngine._coerce_input("count", 3.5, {"type": "number"}) == 3.5 + def test_coerce_input_rejects_non_list_enum_cleanly(self): + """A non-list ``enum`` (scalar or string) must raise a clean ValueError, + not the raw ``TypeError`` from the ``value not in enum`` membership test. + + A scalar (``enum: 5``) makes ``value not in 5`` raise + ``TypeError: argument of type 'int' is not iterable``. A bare string + (``enum: "abc"``) is silently wrong instead — ``value in "abc"`` is a + substring test, not enum membership — so it must be rejected too. + """ + from specify_cli.workflows.engine import WorkflowEngine + + for bad_enum in (5, True, "abc", {"a": 1}): + with pytest.raises(ValueError, match="invalid 'enum': must be a list"): + WorkflowEngine._coerce_input( + "scope", "x", {"type": "string", "enum": bad_enum} + ) + # A valid list ``enum`` still works, and ``None`` means "no enum". + assert ( + WorkflowEngine._coerce_input( + "scope", "a", {"type": "string", "enum": ["a", "b"]} + ) + == "a" + ) + assert ( + WorkflowEngine._coerce_input("scope", "x", {"type": "string"}) == "x" + ) + + def test_validate_workflow_rejects_non_list_enum(self): + """A non-list ``enum`` must be reported as an error, not crash + ``validate_workflow``. The membership test would raise ``TypeError``, + which escapes its ``except ValueError`` and breaks the "return a list of + errors, never raise" contract. This must surface even with no ``default`` + present (the coercion path that would otherwise catch it is only reached + when a default exists). + """ + from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow + + definition = WorkflowDefinition.from_string(""" +schema_version: "1.0" +workflow: + id: "bad-enum" + name: "Bad Enum" + version: "1.0.0" +inputs: + scope: + type: string + enum: 5 +steps: + - id: noop + type: gate + message: "noop" + options: [approve] +""") + errors = validate_workflow(definition) + assert any("invalid 'enum': must be a list" in e for e in errors), errors + + def test_resolve_inputs_rejects_non_list_enum_at_runtime(self, project_dir): + """``execute()`` accepts unvalidated definitions, so a non-list ``enum`` + can reach ``_resolve_inputs`` at run time. It must fail with a clean + ValueError rather than the raw ``TypeError`` from the membership test. + """ + from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition + + definition = WorkflowDefinition.from_string(""" +schema_version: "1.0" +workflow: + id: "runtime-bad-enum" + name: "Runtime Bad Enum" + version: "1.0.0" +inputs: + scope: + type: string + enum: 5 +""") + engine = WorkflowEngine(project_dir) + with pytest.raises(ValueError, match="invalid 'enum': must be a list"): + engine._resolve_inputs(definition, {"scope": "x"}) + + def test_non_list_enum_on_integration_auto_still_rejected(self, project_dir): + """The ``integration: auto`` sentinel strips a *list* ``enum`` before + coercion (enum-membership is a runtime concern for ``auto``). A non-list + ``enum`` must NOT be silently stripped by that path — it is still an + authoring error and must fail with the clean shape ValueError. + """ + from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition + + definition = WorkflowDefinition.from_string(""" +schema_version: "1.0" +workflow: + id: "auto-bad-enum" + name: "Auto Bad Enum" + version: "1.0.0" +inputs: + integration: + type: string + default: "auto" + enum: 5 +""") + engine = WorkflowEngine(project_dir) + with pytest.raises(ValueError, match="invalid 'enum': must be a list"): + engine._resolve_inputs(definition, {}) + def test_validate_workflow_rejects_infinite_default_for_number_type(self): """``type: number`` with an infinite default (YAML ``.inf``) must be reported as an error, not raise. ``int(inf)`` raises OverflowError during