mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(workflows): reject non-list input 'enum' instead of crashing (#3601)
A workflow input whose `enum` is a scalar or string (e.g. `enum: 5`,
`enum: "abc"`) previously slipped past `validate_workflow` and crashed
at run time. The `value not in enum_values` membership test in
`_coerce_input` raises a raw `TypeError` ("argument of type 'int' is
not iterable") for a scalar, and a bare string turns enum membership
into a silent substring test. The `TypeError` also escapes
`validate_workflow`'s `except ValueError`, breaking its documented
"return a list of errors, never raise" contract.
This is the same unvalidated-`execute()` crash class as the fan-in
`wait_for` (#3482) and fan-out step-template (#3537) fixes: `validate()`
should reject the value, but the value can still reach the engine via
`execute()`, which accepts unvalidated definitions.
Fix:
- `_coerce_input` requires a list `enum` (or `None`), raising a clean
ValueError for any other shape — so both `validate_workflow` and
runtime `_resolve_inputs` fail fast with a clear message.
- `validate_workflow` checks `enum` shape directly (not only via the
default-coercion path, which is reached only when a `default` exists),
and strips a malformed `enum` before coercing the default so the
wrong-typed-default error is not duplicated as an enum-shape error.
- The `integration: auto` sentinel only strips a *list* `enum`; a
non-list `enum` stays in the definition so it is rejected rather than
silently exempted by the `auto` membership skip.
Tests cover all three layers: `_coerce_input` directly, authoring-time
`validate_workflow` (with no default present), and runtime
`_resolve_inputs`, plus the `integration: auto` interaction.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -201,6 +201,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
|
||||
@@ -209,13 +223,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()
|
||||
@@ -1400,11 +1429,18 @@ class WorkflowEngine:
|
||||
# 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
|
||||
@@ -1450,6 +1486,22 @@ class WorkflowEngine:
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user