diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 63ddc76395..779cde58d2 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -332,26 +332,45 @@ 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 -> 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): raise WorkflowValidationError( 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): @@ -1006,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 1c29ab56e6..a99d0ce186 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -6317,6 +6317,80 @@ 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", ["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 + 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" + 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`