fix(workflows): reject non-list input 'enum' instead of crashing#3552
fix(workflows): reject non-list input 'enum' instead of crashing#3552Noor-ul-ain001 wants to merge 2 commits into
Conversation
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds workflow enum type validation to prevent raw runtime errors.
Changes:
- Rejects non-list enums during validation and coercion.
- Adds regression tests for scalar, string, runtime, and valid-list enums.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/engine.py |
Adds enum shape validation and error handling. |
tests/test_workflows.py |
Covers malformed and valid enums. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
| 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) |
| enum_is_valid = enum_values is None or isinstance(enum_values, list) | ||
| if "default" in input_def and enum_is_valid: |
mnriem
left a comment
There was a problem hiding this comment.
Address Copilot feedback
Copilot review on github#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) <noreply@anthropic.com>
|
@ 1.
2. Skipping default coercion hid independent default errors (comment on When Tests
Both fail on the pre-fix code (test-the-test verified). Full |
Problem
A workflow input
enumis never type-checked. A scalar value crashes the validator and the runtime input resolver with a rawTypeErrorinstead of producing a clean error:The crash comes from the
value not in enum_valuesmembership test inWorkflowEngine._coerce_input. Two paths are affected:validate_workflow()documents a "return errors, never raise" contract, but theTypeErrorescapes itsexcept ValueErrorand propagates.execute()does not auto-validate the definition (seeload_workflow's docstring), so_resolve_inputshits the same crash when a provided value is coerced.A bare-string
enum(enum: "abc") is silently worse:value in "abc"becomes a substring/character test rather than enum membership, so it passes validation while doing the wrong thing at runtime.Fix
Require
enumto be a list in both places:_coerce_input(the single runtime choke point) raises a cleanValueErroron a non-listenum.validate_workflowreports it as a validation error — needed independently because_coerce_inputis only reached from validation when adefaultis present, and theintegration: autocase stripsenumbefore coercing.The redundant "invalid default" report is suppressed when
enumis already flagged, so the operator sees one clear error rather than the same problem re-framed twice.This is the same unvalidated-config crash class as the existing switch / fan-in / fan-out non-list guards.
Tests
Added to
tests/test_workflows.py:enum(no default) → clean validation error, no crashenum→ rejected + reported exactly once (no duplicate)enumat run time via_resolve_inputs→ cleanValueError, notTypeErrorenum→ still validates and still checks the default against membershipFull
test_workflows.pypasses except the pre-existing Windows-without-elevation symlink-guard tests (unrelated to this change).🤖 Generated with Claude Code