From 573439b6e7d69a1b1ce57eeb2e54fc2f947f696f Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Tue, 25 Aug 2026 20:07:56 +0500 Subject: [PATCH 1/3] fix(workflows): reject falsy non-mapping step.yml in step add MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `workflow_step_add` parses a fetched `step.yml` with `_yaml.safe_load(...) or {}`, which coerces a FALSY non-mapping top-level document (`[]`, `false`, `0`, `''`) to `{}` before the `isinstance(meta, dict)` shape check runs. The command then proceeds with `meta = {}`, derives `step_meta = {}` and `type_key = ""`, and reports the unrelated "step.yml missing 'step.type_key' field" instead of the real problem: "step.yml must be a YAML mapping". A TRUTHY non-mapping document (a bare string) already reported the correct error — this was an inconsistency. Same falsy-or-coerce shape as the catalog-config bugs fixed elsewhere in workflows/catalog.py, presets/__init__.py, and integrations (#4187) this cycle. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FW9fAYsCBCAgdKWovtSyqt --- src/specify_cli/workflows/_commands.py | 10 +++- tests/test_workflows.py | 68 ++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 5e40569af0..38db89afb5 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3364,12 +3364,18 @@ def _safe_fetch(url: str) -> bytes: try: import yaml as _yaml - meta = _yaml.safe_load(step_yml_content.decode("utf-8")) or {} + meta = _yaml.safe_load(step_yml_content.decode("utf-8")) except Exception as exc: console.print(f"[red]Error:[/red] Invalid step.yml: {exc}") raise typer.Exit(1) - if not isinstance(meta, dict): + # Do NOT coerce with ``or {}`` here: that also turns a FALSY non-mapping + # (top-level ``[]``, ``false``, ``0``, ``''``) into ``{}`` and silently + # bypasses this shape check, surfacing the unrelated "missing + # 'step.type_key'" error below instead of the real problem. + if meta is None: + meta = {} + elif not isinstance(meta, dict): console.print("[red]Error:[/red] step.yml must be a YAML mapping") raise typer.Exit(1) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index d599f3c6a4..2a21886970 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -10451,6 +10451,74 @@ def read(self, size=-1): project_dir / ".specify" / "workflows" / "steps" / "my-step" ).exists() + @pytest.mark.parametrize("step_yml_body", [b"[]", b"false", b"0", b"''"]) + def test_add_rejects_falsy_non_mapping_step_yml( + self, project_dir, monkeypatch, step_yml_body + ): + """A FALSY non-mapping step.yml document ([], false, 0, '') must be + reported as "step.yml must be a YAML mapping", not silently coerced by + ``or {}`` into {} and then misreported as the unrelated "missing + 'step.type_key'" error — matching how a TRUTHY non-mapping document + (e.g. a bare string) already reports the mapping-shape error.""" + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.catalog import StepCatalog + from specify_cli.authentication import http as auth_http + + monkeypatch.chdir(project_dir) + monkeypatch.setattr( + StepCatalog, + "get_step_info", + lambda self, step_id: { + "id": step_id, + "name": "Test Step", + "url": "https://example.com/step.yml", + "init_url": "https://example.com/__init__.py", + "_install_allowed": True, + }, + ) + + class _FakeResponse: + def __init__(self, url): + self.url = url + self.body = step_yml_body if url.endswith("step.yml") else b"" + self.offset = 0 + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def getheader(self, name): + return None + + def geturl(self): + return self.url + + def read(self, size=-1): + if size < 0: + size = len(self.body) - self.offset + chunk = self.body[self.offset : self.offset + size] + self.offset += len(chunk) + return chunk + + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None: _FakeResponse(url), + ) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "my-step"] + ) + + assert result.exit_code != 0 + assert "step.yml must be a YAML mapping" in result.output + assert not ( + project_dir / ".specify" / "workflows" / "steps" / "my-step" + ).exists() + @pytest.mark.parametrize( ("catalog_fields", "expected"), [ From a8e774bcfe61c2da13bbe6ddf6d2c80df350eab8 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Tue, 1 Sep 2026 15:26:56 +0500 Subject: [PATCH 2/3] fix(workflows): distinguish explicit YAML null from empty step.yml An explicit null document (null/~/NULL) parses to the same None as a genuinely empty document, so it was silently coerced to {} and misreported as the unrelated "missing step.type_key" error instead of the mapping-shape error, per Copilot review on PR #4321. Use yaml.compose to tell the two apart, matching the sibling loaders (yamlio.py, integrations/catalog.py, overlays/layer_sources.py). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0147sii7uC56YzAu2Ep9qH94 --- src/specify_cli/workflows/_commands.py | 17 ++++++++++++----- tests/test_workflows.py | 9 +++++++-- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 38db89afb5..e4cbd61674 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3364,16 +3364,23 @@ def _safe_fetch(url: str) -> bytes: try: import yaml as _yaml - meta = _yaml.safe_load(step_yml_content.decode("utf-8")) + step_yml_text = step_yml_content.decode("utf-8") + # ``safe_load`` returns None for BOTH an empty document and an + # explicit null scalar (``null``, ``~``, ``NULL``), so it cannot + # tell them apart on its own. ``compose`` yields no node only for + # a genuinely empty document. + is_empty_document = _yaml.compose(step_yml_text) is None + meta = _yaml.safe_load(step_yml_text) except Exception as exc: console.print(f"[red]Error:[/red] Invalid step.yml: {exc}") raise typer.Exit(1) # Do NOT coerce with ``or {}`` here: that also turns a FALSY non-mapping - # (top-level ``[]``, ``false``, ``0``, ``''``) into ``{}`` and silently - # bypasses this shape check, surfacing the unrelated "missing - # 'step.type_key'" error below instead of the real problem. - if meta is None: + # (top-level ``[]``, ``false``, ``0``, ``''``, or an explicit ``null``) + # into ``{}`` and silently bypasses this shape check, surfacing the + # unrelated "missing 'step.type_key'" error below instead of the real + # problem. Only a genuinely empty document defaults to ``{}``. + if meta is None and is_empty_document: meta = {} elif not isinstance(meta, dict): console.print("[red]Error:[/red] step.yml must be a YAML mapping") diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 2a21886970..fbc91746b8 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -10451,7 +10451,9 @@ def read(self, size=-1): project_dir / ".specify" / "workflows" / "steps" / "my-step" ).exists() - @pytest.mark.parametrize("step_yml_body", [b"[]", b"false", b"0", b"''"]) + @pytest.mark.parametrize( + "step_yml_body", [b"[]", b"false", b"0", b"''", b"null", b"~", b"NULL"] + ) def test_add_rejects_falsy_non_mapping_step_yml( self, project_dir, monkeypatch, step_yml_body ): @@ -10459,7 +10461,10 @@ def test_add_rejects_falsy_non_mapping_step_yml( reported as "step.yml must be a YAML mapping", not silently coerced by ``or {}`` into {} and then misreported as the unrelated "missing 'step.type_key'" error — matching how a TRUTHY non-mapping document - (e.g. a bare string) already reports the mapping-shape error.""" + (e.g. a bare string) already reports the mapping-shape error. An + explicit null scalar (null/~/NULL) parses to the same ``None`` as a + genuinely empty document, so it must be distinguished (via + ``yaml.compose``) and rejected too, rather than defaulting to {}.""" from typer.testing import CliRunner from specify_cli import app from specify_cli.workflows.catalog import StepCatalog From 8e6d0ec64e3a6ead1ae1cb34095bd4b7461615d4 Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Tue, 1 Sep 2026 19:26:08 +0500 Subject: [PATCH 3/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/_commands.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index e4cbd61674..ed450acf38 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3369,8 +3369,14 @@ def _safe_fetch(url: str) -> bytes: # explicit null scalar (``null``, ``~``, ``NULL``), so it cannot # tell them apart on its own. ``compose`` yields no node only for # a genuinely empty document. - is_empty_document = _yaml.compose(step_yml_text) is None + node = _yaml.compose(step_yml_text) meta = _yaml.safe_load(step_yml_text) + is_empty_document = node is None or ( + meta is None + and isinstance(node, _yaml.nodes.ScalarNode) + and node.value == "" + and node.start_mark.index == node.end_mark.index + ) except Exception as exc: console.print(f"[red]Error:[/red] Invalid step.yml: {exc}") raise typer.Exit(1)