fix(workflows): report validation errors instead of crashing on non-string workflow.yml scalars#3421
fix(workflows): report validation errors instead of crashing on non-string workflow.yml scalars#3421marcelsafin wants to merge 3 commits into
Conversation
…kflow validation YAML parses unquoted scalars like version: 1.0 and id: 123 as float/int, which crashed validate_workflow and workflow add with raw tracebacks. Type-check id, name, version and step ids before regex and string operations so these surface as validation errors. Accept an unquoted schema_version: 1.0 instead of printing a self-identical rejection message. Fixes github#3420 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens workflow YAML validation and workflow add so authoring mistakes like unquoted numeric scalars (e.g. version: 1.0, id: 123) produce clean validation errors instead of crashing with a traceback, and it also accepts schema_version: 1.0 when parsed as a YAML float.
Changes:
- Add type-checks in workflow validation for
workflow.id,workflow.name,workflow.version, and stepidbefore regex/substring operations. - Adjust
workflow addlocal-install path to avoid calling.strip()on non-string workflow IDs. - Add unit + CLI-level tests ensuring these cases exit cleanly with validation messages (no traceback).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/specify_cli/workflows/engine.py |
Adds type-aware validation (including schema_version stringification) to prevent crashes on non-string YAML scalars. |
src/specify_cli/workflows/_commands.py |
Guards workflow add local-install ID checks so non-string IDs don’t crash on .strip(). |
tests/test_workflows.py |
Adds coverage for non-string workflow/name/version/step IDs and CLI behavior for workflow add error output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not definition.id: | ||
| errors.append("Workflow is missing 'workflow.id'.") | ||
| elif not isinstance(definition.id, str): | ||
| errors.append( | ||
| f"'workflow.id' must be a string, got " |
There was a problem hiding this comment.
Done — missing-field checks now use is None or == "" so falsey non-strings (id: 0, name: false, version: 0.0) fall through to the typed error. Added a regression test covering all four fields.
| if not definition.name: | ||
| errors.append("Workflow is missing 'workflow.name'.") | ||
| elif not isinstance(definition.name, str): | ||
| errors.append( | ||
| f"'workflow.name' must be a string, got " |
There was a problem hiding this comment.
Done — missing-field checks now use is None or == "" so falsey non-strings fall through to the typed error. Regression test added.
| step_id = step_config.get("id") | ||
| if not step_id: | ||
| errors.append("Step is missing 'id' field.") | ||
| continue | ||
| if not isinstance(step_id, str): | ||
| errors.append( |
There was a problem hiding this comment.
Done — missing-field checks now use is None or == "" so falsey non-strings fall through to the typed error. Regression test added.
| if not definition.id or ( | ||
| isinstance(definition.id, str) and not definition.id.strip() | ||
| ): |
There was a problem hiding this comment.
Done — missing-field checks now use is None or == "" so falsey non-strings fall through to the typed error. Regression test added.
…fields Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| if str(definition.schema_version) not in ("1.0", "1"): | ||
| errors.append( | ||
| f"Unsupported schema_version {definition.schema_version!r}. " | ||
| f"Expected '1.0'." | ||
| ) |
There was a problem hiding this comment.
Fixed in 352fa3b. Dropped "1" from the accepted values; only "1.0" passes now, so the message is accurate. Unquoted YAML 1.0 still works via the str() comparison.
…s accurate The check also accepted "1" while the error said Expected '1.0'. Unquoted YAML 1.0 still works via str(); plain 1 is now rejected with the message that matches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #3420
Root cause
validate_workflow()inworkflows/engine.pyregex-matches and iteratesdefinition.id,definition.versionand stepidvalues without checking they are strings, and_validate_and_install_localcallsdefinition.id.strip(). YAML parses unquoted scalars (version: 1.0,id: 123) as float/int, so a small authoring mistake crashedworkflow add/workflow validatewith a raw traceback. This is the shared validation path, so all entry points were affected.Change
validate_workflow()type-checksworkflow.id,workflow.nameandworkflow.versionbefore pattern matching and reports what type it got, e.g.'workflow.version' must be a string, got float (1.0) — quote it in YAML (version: "1.0.0")._validate_steps()rejects non-string step ids the same way before the":" in step_idcontainment check.definition.id.strip()guard inworkflow addonly applies to strings; non-string ids fall through tovalidate_workflow()for the typed error.schema_version: 1.0(YAML float) is now accepted viastr()comparison — previously it was rejected with the self-identical messageUnsupported schema_version 1.0. Expected '1.0'.Testing
Six unit tests on
validate_workflow(non-string id/name/version/step id, unquoted schema_version accepted) plus three CLI-level tests assertingworkflow addexits 1 with a validation message and no traceback for each crash case.Full suite: 3842 passed, 107 skipped.
ruff checkclean on changed files.