Skip to content

Commit aacfb66

Browse files
marcelsafinCopilot
andcommitted
fix(workflows): treat falsey non-strings as type errors, not missing fields
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 3bf28c3 commit aacfb66

3 files changed

Lines changed: 34 additions & 10 deletions

File tree

src/specify_cli/workflows/_commands.py

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -601,11 +601,14 @@ def _validate_and_install_local(yaml_path: Path, source_label: str) -> None:
601601
except (ValueError, yaml.YAMLError) as exc:
602602
console.print(f"[red]Error:[/red] Invalid workflow YAML: {exc}")
603603
raise typer.Exit(1)
604-
# Non-string ids (e.g. unquoted ``id: 123``) fall through to
605-
# validate_workflow below, which reports a typed error instead of
606-
# crashing on ``.strip()`` here.
607-
if not definition.id or (
608-
isinstance(definition.id, str) and not definition.id.strip()
604+
# Non-string ids (e.g. unquoted ``id: 123`` or ``id: 0``) fall through
605+
# to validate_workflow below, which reports a typed error instead of
606+
# crashing on ``.strip()`` here. Only None/empty/whitespace-only ids
607+
# are rejected as missing.
608+
if (
609+
definition.id is None
610+
or definition.id == ""
611+
or (isinstance(definition.id, str) and not definition.id.strip())
609612
):
610613
console.print("[red]Error:[/red] Workflow definition has an empty or missing 'id'")
611614
raise typer.Exit(1)

src/specify_cli/workflows/engine.py

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,10 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]:
140140
# -- Top-level fields -------------------------------------------------
141141
# YAML parses unquoted scalars like ``id: 123`` or ``version: 1.0`` as
142142
# int/float; check types before regex/string operations so authoring
143-
# mistakes surface as validation errors instead of tracebacks.
144-
if not definition.id:
143+
# mistakes surface as validation errors instead of tracebacks. Only
144+
# ``None``/empty-string count as missing so falsey non-strings
145+
# (``id: 0``, ``name: false``) still get the typed error.
146+
if definition.id is None or definition.id == "":
145147
errors.append("Workflow is missing 'workflow.id'.")
146148
elif not isinstance(definition.id, str):
147149
errors.append(
@@ -154,15 +156,15 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]:
154156
f"with hyphens."
155157
)
156158

157-
if not definition.name:
159+
if definition.name is None or definition.name == "":
158160
errors.append("Workflow is missing 'workflow.name'.")
159161
elif not isinstance(definition.name, str):
160162
errors.append(
161163
f"'workflow.name' must be a string, got "
162164
f"{type(definition.name).__name__} ({definition.name!r})."
163165
)
164166

165-
if not definition.version:
167+
if definition.version is None or definition.version == "":
166168
errors.append("Workflow is missing 'workflow.version'.")
167169
elif not isinstance(definition.version, str):
168170
errors.append(
@@ -277,7 +279,7 @@ def _validate_steps(
277279
continue
278280

279281
step_id = step_config.get("id")
280-
if not step_id:
282+
if step_id is None or step_id == "":
281283
errors.append("Step is missing 'id' field.")
282284
continue
283285
if not isinstance(step_id, str):

tests/test_workflows.py

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2687,6 +2687,25 @@ def test_non_string_step_id_reports_error(self):
26872687
errors = validate_workflow(definition)
26882688
assert any("Step ID" in e and "string" in e for e in errors)
26892689

2690+
def test_falsey_non_string_scalars_report_typed_errors(self):
2691+
from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow
2692+
2693+
definition = WorkflowDefinition.from_string("""
2694+
workflow:
2695+
id: 0
2696+
name: false
2697+
version: 0.0
2698+
steps:
2699+
- id: 0
2700+
command: speckit.specify
2701+
""")
2702+
errors = validate_workflow(definition)
2703+
assert any("'workflow.id' must be a string" in e for e in errors)
2704+
assert any("'workflow.name' must be a string" in e for e in errors)
2705+
assert any("'workflow.version' must be a string" in e for e in errors)
2706+
assert any("Step ID must be a string" in e for e in errors)
2707+
assert not any("missing" in e for e in errors)
2708+
26902709
def test_unquoted_schema_version_accepted(self):
26912710
from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow
26922711

0 commit comments

Comments
 (0)