From 1c186a5fac8a86918f97ee69c7637f9b5c0eff3b Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 24 Jul 2026 13:39:08 +0500 Subject: [PATCH 1/3] fix(workflows): reject falsy non-mapping workflow-catalogs.yml top level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WorkflowCatalog._load_catalog_config parsed the config with `yaml.safe_load(...) or {}`, then checked `isinstance(data, dict)`. The `or {}` coerces a FALSY non-mapping top level (`[]`, `false`, `0`, `''`) to `{}` *before* the guard runs, so those are silently swallowed as "empty config" and fall back to the built-in defaults -- while a TRUTHY non-mapping (`5`, a bare list) correctly raises. Same silent-swallow inconsistency the bundler catalog reader fixed for its own config. Drop the `or {}` and branch on `None` (empty document / explicit `null`) explicitly: `None` stays a valid no-op, every non-mapping (falsy or truthy) now raises the same actionable error. Correct configs are unaffected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/workflows/catalog.py | 10 +++++++++- tests/test_workflows.py | 26 ++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 63ddc76395..3b50c6ecac 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -332,11 +332,19 @@ def _load_catalog_config( if not config_path.exists(): return None try: - data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {} + data = yaml.safe_load(config_path.read_text(encoding="utf-8")) except (yaml.YAMLError, OSError, UnicodeError) as exc: raise WorkflowValidationError( f"Failed to read catalog config {config_path}: {exc}" ) from exc + # An empty document (or explicit ``null``) parses to None -> no project + # catalogs, fall back to the built-in defaults. Do NOT coerce with + # ``or {}`` here: that also turns a FALSY non-mapping (top-level ``[]``, + # ``false``, ``0``, ``''``) into ``{}`` and silently swallows it, while a + # TRUTHY non-mapping (``5``, a bare list) correctly raises below -- an + # inconsistency. Only None means "no document". + if data is None: + return None if not isinstance(data, dict): raise WorkflowValidationError( f"Invalid catalog config: expected a mapping, " diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 1c29ab56e6..32fccc1ecd 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -6317,6 +6317,32 @@ def test_project_level_config(self, project_dir): assert len(entries) == 1 assert entries[0].name == "custom" + @pytest.mark.parametrize("body", ["[]\n", "false\n", "0\n", "''\n"]) + def test_falsy_non_mapping_config_rejected(self, project_dir, body): + """A FALSY non-mapping top-level config ([], false, 0, '') must raise, + like a truthy non-mapping (5, a bare list) already does. The previous + ``yaml.safe_load(...) or {}`` coerced these to {} and silently swallowed + them, diverging from the truthy case.""" + from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError + + config_path = project_dir / ".specify" / "workflow-catalogs.yml" + config_path.write_text(body, encoding="utf-8") + catalog = WorkflowCatalog(project_dir) + with pytest.raises(WorkflowValidationError, match="expected a mapping"): + catalog._load_catalog_config(config_path) + + @pytest.mark.parametrize("body", ["", "# only a comment\n", "null\n", "~\n"]) + def test_empty_or_null_config_is_noop(self, project_dir, body): + """An empty document, comment-only file, or explicit top-level null is a + valid no-op (no project catalogs -> built-in defaults), and must NOT be + confused with a falsy non-mapping.""" + from specify_cli.workflows.catalog import WorkflowCatalog + + config_path = project_dir / ".specify" / "workflow-catalogs.yml" + config_path.write_text(body, encoding="utf-8") + catalog = WorkflowCatalog(project_dir) + assert catalog._load_catalog_config(config_path) is None + @pytest.mark.parametrize("bad_priority", [True, False, float("inf")]) def test_config_priority_bool_or_inf_rejected(self, project_dir, bad_priority): """`priority: true` must not be silently coerced to 1, and `priority: .inf` From 58d22e38586feb3f34a8cd67bb6585f3a34f16c6 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 25 Jul 2026 00:21:29 +0500 Subject: [PATCH 2/3] docs(workflows): describe the catalog-config fallthrough accurately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment said a None return means "no project catalogs, fall back to the built-in defaults". Both halves were imprecise: _load_catalog_config serves the project AND user configs, and get_active_catalogs falls through env -> project -> user -> built-in, so a None from the project layer moves on to the USER config; the built-in defaults apply only once every layer returned None. Reword the loader comment and the mirror test docstring. Comments only -- no behaviour change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/workflows/catalog.py | 14 ++++++++------ tests/test_workflows.py | 5 +++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 3b50c6ecac..964a769791 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -337,12 +337,14 @@ def _load_catalog_config( raise WorkflowValidationError( f"Failed to read catalog config {config_path}: {exc}" ) from exc - # An empty document (or explicit ``null``) parses to None -> no project - # catalogs, fall back to the built-in defaults. Do NOT coerce with - # ``or {}`` here: that also turns a FALSY non-mapping (top-level ``[]``, - # ``false``, ``0``, ``''``) into ``{}`` and silently swallows it, while a - # TRUTHY non-mapping (``5``, a bare list) correctly raises below -- an - # inconsistency. Only None means "no document". + # An empty document (or explicit ``null``) parses to None -> this config + # layer contributes nothing, so ``get_active_catalogs`` moves on to the + # next layer (this loader serves both the project and user configs; + # the built-in defaults apply only once every layer has returned None). + # Do NOT coerce with ``or {}`` here: that also turns a FALSY non-mapping + # (top-level ``[]``, ``false``, ``0``, ``''``) into ``{}`` and silently + # swallows it, while a TRUTHY non-mapping (``5``, a bare list) correctly + # raises below -- an inconsistency. Only None means "no document". if data is None: return None if not isinstance(data, dict): diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 32fccc1ecd..eab8a2def3 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -6334,8 +6334,9 @@ def test_falsy_non_mapping_config_rejected(self, project_dir, body): @pytest.mark.parametrize("body", ["", "# only a comment\n", "null\n", "~\n"]) def test_empty_or_null_config_is_noop(self, project_dir, body): """An empty document, comment-only file, or explicit top-level null is a - valid no-op (no project catalogs -> built-in defaults), and must NOT be - confused with a falsy non-mapping.""" + valid no-op: the loader returns None so that config layer is skipped and + get_active_catalogs falls through to the next one. It must NOT be + confused with a falsy non-mapping, which raises.""" from specify_cli.workflows.catalog import WorkflowCatalog config_path = project_dir / ".specify" / "workflow-catalogs.yml" From f9f8206c498c0a5ab8a07153dec05559b660fc9d Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 25 Jul 2026 10:23:59 +0500 Subject: [PATCH 3/3] fix(workflows): close the same falsy-mask gap in 'catalogs' and StepCatalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review follow-up: the top-level fix left the identical asymmetry live five lines below, and again in this file's twin loader. 1. WorkflowCatalog._load_catalog_config: the ``catalogs`` shape check sat behind an emptiness check, so a FALSY non-list (``catalogs: {}``/``''``/``0``/ ``false``) was silently swallowed as "no catalogs" while ``catalogs: 5`` raised. Verified before this commit: ``catalogs: {}`` -> None (no error). Shape now checked first; absent/explicit-null and empty-list stay no-ops (matching the bundler's reader). 2. StepCatalog._load_catalog_config -- the step-catalog twin, read the same way -- still had ``yaml.safe_load(...) or {}``, so falsy non-mappings bypassed its isinstance guard (``[]`` -> None while ``5`` raised). Same two guards applied, keeping the two loaders in lockstep. Eight new parametrized cases, all failing before this commit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/workflows/catalog.py | 32 ++++++++++++++----- tests/test_workflows.py | 47 ++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 7 deletions(-) diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 964a769791..779cde58d2 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -352,16 +352,25 @@ def _load_catalog_config( f"Invalid catalog config: expected a mapping, " f"got {type(data).__name__}" ) - catalogs_data = data.get("catalogs", []) - if not catalogs_data: - # Empty catalogs list (e.g. after removing last entry) - # is valid — fall back to built-in defaults. + # Same asymmetry as the top level above, one nesting level down: the + # shape check has to run BEFORE the emptiness check, or a FALSY non-list + # (``catalogs: {}``/``''``/``0``/``false``) is silently swallowed as + # "no catalogs" while a TRUTHY non-list (``catalogs: 5``) correctly + # raises. An absent key or an explicit ``catalogs:`` null means "nothing + # configured here" (matching the bundler's reader), and an empty list + # stays valid too. + catalogs_data = data.get("catalogs") + if catalogs_data is None: return None if not isinstance(catalogs_data, list): raise WorkflowValidationError( f"Invalid catalog config: 'catalogs' must be a list, " f"got {type(catalogs_data).__name__}" ) + if not catalogs_data: + # Empty catalogs list (e.g. after removing last entry) + # is valid — fall back to built-in defaults. + return None entries: list[WorkflowCatalogEntry] = [] for idx, item in enumerate(catalogs_data): @@ -1016,24 +1025,33 @@ def _load_catalog_config( if not config_path.exists(): return None try: - data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {} + data = yaml.safe_load(config_path.read_text(encoding="utf-8")) except (yaml.YAMLError, OSError, UnicodeError) as exc: raise StepValidationError( f"Failed to read catalog config {config_path}: {exc}" ) from exc + # Same two guards as WorkflowCatalog._load_catalog_config above, kept in + # lockstep: this is the step-catalog twin of that loader and read the + # same way. Dropping ``or {}`` stops a falsy non-mapping top level from + # being coerced past the isinstance check, and the ``catalogs`` shape + # check runs before the emptiness check for the same reason. + if data is None: + return None if not isinstance(data, dict): raise StepValidationError( f"Invalid catalog config: expected a mapping, " f"got {type(data).__name__}" ) - catalogs_data = data.get("catalogs", []) - if not catalogs_data: + catalogs_data = data.get("catalogs") + if catalogs_data is None: return None if not isinstance(catalogs_data, list): raise StepValidationError( f"Invalid catalog config: 'catalogs' must be a list, " f"got {type(catalogs_data).__name__}" ) + if not catalogs_data: + return None entries: list[StepCatalogEntry] = [] for idx, item in enumerate(catalogs_data): diff --git a/tests/test_workflows.py b/tests/test_workflows.py index eab8a2def3..a99d0ce186 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -6331,6 +6331,53 @@ def test_falsy_non_mapping_config_rejected(self, project_dir, body): with pytest.raises(WorkflowValidationError, match="expected a mapping"): catalog._load_catalog_config(config_path) + @pytest.mark.parametrize("body", ["catalogs: {}\n", "catalogs: ''\n", "catalogs: 0\n", "catalogs: false\n"]) + def test_falsy_non_list_catalogs_rejected(self, project_dir, body): + """A FALSY non-list ``catalogs:`` value must raise, like a truthy one + (``catalogs: 5``) already does. The shape check sat behind the emptiness + check, so these were silently swallowed as "no catalogs".""" + from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError + + config_path = project_dir / ".specify" / "workflow-catalogs.yml" + config_path.write_text(body, encoding="utf-8") + catalog = WorkflowCatalog(project_dir) + with pytest.raises(WorkflowValidationError, match="'catalogs' must be a list"): + catalog._load_catalog_config(config_path) + + @pytest.mark.parametrize("body", ["catalogs:\n", "catalogs: []\n"]) + def test_absent_or_empty_catalogs_is_noop(self, project_dir, body): + """An explicit ``catalogs:`` null or an empty list stays a valid no-op — + the layer contributes nothing and resolution falls through.""" + from specify_cli.workflows.catalog import WorkflowCatalog + + config_path = project_dir / ".specify" / "workflow-catalogs.yml" + config_path.write_text(body, encoding="utf-8") + catalog = WorkflowCatalog(project_dir) + assert catalog._load_catalog_config(config_path) is None + + @pytest.mark.parametrize("body", ["[]\n", "false\n", "0\n", "''\n"]) + def test_step_catalog_falsy_non_mapping_rejected(self, project_dir, body): + """StepCatalog._load_catalog_config is the twin of the workflow loader and + had the same ``or {}`` coercion, so falsy non-mappings bypassed its + isinstance guard. It must reject them like a truthy non-mapping.""" + from specify_cli.workflows.catalog import StepCatalog, StepValidationError + + config_path = project_dir / ".specify" / "step-catalogs.yml" + config_path.write_text(body, encoding="utf-8") + catalog = StepCatalog(project_dir) + with pytest.raises(StepValidationError, match="expected a mapping"): + catalog._load_catalog_config(config_path) + + @pytest.mark.parametrize("body", ["", "# only a comment\n", "null\n", "~\n"]) + def test_step_catalog_empty_or_null_is_noop(self, project_dir, body): + """...while an empty document or explicit null stays a valid no-op.""" + from specify_cli.workflows.catalog import StepCatalog + + config_path = project_dir / ".specify" / "step-catalogs.yml" + config_path.write_text(body, encoding="utf-8") + catalog = StepCatalog(project_dir) + assert catalog._load_catalog_config(config_path) is None + @pytest.mark.parametrize("body", ["", "# only a comment\n", "null\n", "~\n"]) def test_empty_or_null_config_is_noop(self, project_dir, body): """An empty document, comment-only file, or explicit top-level null is a