From b53af69f16bde590f1dd469a5e902e6a46da9738 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Wed, 15 Jul 2026 22:30:10 +0500 Subject: [PATCH 1/2] fix(workflows): reject non-list input 'enum' instead of crashing A workflow input `enum` was never type-checked, so a scalar value (`enum: 5`, `enum: true`) made the `value not in enum_values` membership test raise a raw `TypeError` ("argument of type 'int' is not ... iterable"). This escaped `validate_workflow`'s `except ValueError`, breaking its documented "return errors, never raise" contract, and crashed `_resolve_inputs` outright at run time (`execute` does not auto-validate the definition first). A bare-string `enum` was silently worse: `value in "abc"` is a substring/character test, not enum membership. Require `enum` to be a list in both `_coerce_input` (the single runtime choke point) and the authoring-time inputs loop (so a bad `enum` on an input with no `default`, or the `integration: auto` case that strips `enum` before coercion, is still caught). Suppress the redundant "invalid default" report when `enum` is already flagged, so the operator sees one clear error. Same unvalidated-config crash class as the switch/fan-in/fan-out non-list guards. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/workflows/engine.py | 38 ++++++++++- tests/test_workflows.py | 100 ++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index ed52710bed..e88b6d9ad0 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,7 +215,13 @@ 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). - if "default" in input_def: + # A malformed (non-list) ``enum`` is already reported above; skip + # the default check when it is, so ``_coerce_input`` doesn't re-raise + # the same enum error re-framed as an "invalid default" (a confusing + # duplicate). The type/coercion of the default is still meaningfully + # checkable, but reporting one clear enum error is better than two. + enum_is_valid = enum_values is None or isinstance(enum_values, list) + if "default" in input_def and enum_is_valid: default_value = input_def["default"] is_auto_integration = ( input_name == "integration" and default_value == "auto" @@ -1418,6 +1438,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 8fefad740e..743ecccb21 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -3950,6 +3950,106 @@ def test_validate_workflow_rejects_non_string_default_for_string_type(self): errors = validate_workflow(definition) assert any("invalid default" in e for e in errors), errors + def test_validate_workflow_rejects_scalar_enum(self): + """A non-list ``enum`` (``enum: 5``) must be reported as a validation + error, not crash ``validate_workflow`` with a raw ``TypeError`` from the + ``value not in enum_values`` membership test.""" + from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow + + definition = WorkflowDefinition.from_string(""" +schema_version: "1.0" +workflow: + id: "scalar-enum" + name: "Scalar 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'" in e and "must be a list" in e for e in errors), errors + + def test_validate_workflow_rejects_string_enum(self): + """A bare-string ``enum`` (``enum: abc``) must be rejected too — otherwise + ``value in "abc"`` is a silent substring/character test, not enum + membership.""" + from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow + + definition = WorkflowDefinition.from_string(""" +schema_version: "1.0" +workflow: + id: "string-enum" + name: "String Enum" + version: "1.0.0" +inputs: + scope: + type: string + default: "a" + enum: "abc" +steps: + - id: noop + type: gate + message: "noop" + options: [approve] +""") + errors = validate_workflow(definition) + assert any("invalid 'enum'" in e and "must be a list" in e for e in errors), errors + # The malformed enum is reported once — not re-framed a second time as an + # "invalid default" by the default-coercion path. + assert sum("invalid 'enum'" in e for e in errors) == 1, errors + + def test_resolve_inputs_rejects_scalar_enum_at_runtime(self, project_dir): + """A non-list ``enum`` must raise a clean ``ValueError`` (not a raw + ``TypeError``) when a provided value is coerced at run time, since + ``execute`` does not auto-validate the definition first.""" + from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition + + definition = WorkflowDefinition.from_string(""" +schema_version: "1.0" +workflow: + id: "runtime-scalar-enum" + name: "Runtime Scalar Enum" + version: "1.0.0" +inputs: + scope: + type: string + enum: 5 +""") + engine = WorkflowEngine(project_dir) + with pytest.raises(ValueError, match="invalid 'enum'"): + engine._resolve_inputs(definition, {"scope": "bar"}) + + def test_validate_workflow_accepts_list_enum(self): + """A well-formed list ``enum`` must still validate cleanly and continue to + check the default against enum membership.""" + from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow + + definition = WorkflowDefinition.from_string(""" +schema_version: "1.0" +workflow: + id: "list-enum" + name: "List Enum" + version: "1.0.0" +inputs: + scope: + type: string + default: "full" + enum: ["full", "backend-only"] +steps: + - id: noop + type: gate + message: "noop" + options: [approve] +""") + errors = validate_workflow(definition) + assert not any("enum" in e for e in errors), errors + def test_while_loop_condition_reads_latest_iteration(self, project_dir): """Regression: while-loop condition must see updated step output from the most recent iteration, not stale iteration-0 data. From a6ed0665980dea9d099e0b80b19587007c30333f Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Sun, 19 Jul 2026 21:01:41 +0500 Subject: [PATCH 2/2] fix(workflows): close two enum-guard gaps flagged in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review on #3552 found two holes in the non-list `enum` guard: 1. Runtime `integration: auto` path bypassed the guard. `_resolve_inputs` stripped `enum` before calling `_coerce_input` whenever `"enum" in input_def`, so a malformed `enum: 5` on the auto-integration input was removed and never reached the shape check — it resolved successfully instead of raising the promised clean `ValueError`. Now only a *list* `enum` is stripped; a scalar/string `enum` stays in so `_coerce_input` rejects it. 2. Validation hid independent default-type errors. When `enum` was malformed, `validate_workflow` skipped the default coercion entirely, so `type: string` + `default: 5` + `enum: 5` reported only the enum error and silently accepted the wrong-typed default. Now the enum is stripped from the coercion input (suppressing the duplicate membership error) but coercion still runs, so the default's type is validated and both errors surface. Adds regression tests for both cases; both fail on the pre-fix code (test-the-test verified). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/workflows/engine.py | 32 +++++++++++---- tests/test_workflows.py | 61 +++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 8 deletions(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index e88b6d9ad0..edcb52d770 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -215,19 +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). - # A malformed (non-list) ``enum`` is already reported above; skip - # the default check when it is, so ``_coerce_input`` doesn't re-raise - # the same enum error re-framed as an "invalid default" (a confusing - # duplicate). The type/coercion of the default is still meaningfully - # checkable, but reporting one clear enum error is better than two. enum_is_valid = enum_values is None or isinstance(enum_values, list) - if "default" in input_def and enum_is_valid: + 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() @@ -1388,11 +1397,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 diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 743ecccb21..ed73090c7e 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -4050,6 +4050,67 @@ def test_validate_workflow_accepts_list_enum(self): errors = validate_workflow(definition) assert not any("enum" in e for e in errors), errors + def test_validate_workflow_scalar_enum_still_reports_bad_default_type(self): + """A malformed ``enum`` must not mask an independent default type error. + + With ``type: string`` and ``default: 5`` alongside ``enum: 5``, the + malformed enum is reported once *and* the wrong-typed default is still + surfaced — stripping only ``enum`` from the coercion input preserves the + default's type validation instead of skipping it entirely.""" + from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow + + definition = WorkflowDefinition.from_string(""" +schema_version: "1.0" +workflow: + id: "scalar-enum-bad-default" + name: "Scalar Enum Bad Default" + version: "1.0.0" +inputs: + scope: + type: string + default: 5 + enum: 5 +steps: + - id: noop + type: gate + message: "noop" + options: [approve] +""") + errors = validate_workflow(definition) + # The enum shape is reported exactly once... + assert sum("invalid 'enum'" in e for e in errors) == 1, errors + # ...and the independent wrong-typed default is still surfaced. + assert any( + "invalid default" in e and "expected a string" in e for e in errors + ), errors + + def test_resolve_inputs_auto_integration_rejects_scalar_enum(self, project_dir): + """The ``integration: auto`` runtime exemption must not swallow a + malformed ``enum``. + + ``_resolve_inputs`` strips ``enum`` before coercing the ``auto`` + sentinel so a workflow listing specific integrations doesn't crash on + ``"auto"``. But that strip must apply only to a *list* ``enum`` — a + scalar ``enum: 5`` must still reach ``_coerce_input`` and raise a clean + ``ValueError``, not be silently exempted and resolve successfully.""" + from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition + + definition = WorkflowDefinition.from_string(""" +schema_version: "1.0" +workflow: + id: "auto-integration-scalar-enum" + name: "Auto Integration Scalar Enum" + version: "1.0.0" +inputs: + integration: + type: string + default: "auto" + enum: 5 +""") + engine = WorkflowEngine(project_dir) + with pytest.raises(ValueError, match="invalid 'enum'"): + engine._resolve_inputs(definition, {}) + def test_while_loop_condition_reads_latest_iteration(self, project_dir): """Regression: while-loop condition must see updated step output from the most recent iteration, not stale iteration-0 data.