fix(bundler): treat an explicitly null catalog field as empty, not "None"

src/specify_cli/bundler/models/catalog.py

@@ -13,6 +13,7 @@
 
 from .. import BundlerError
 from ..lib.yamlio import ensure_within, load_yaml
+from .manifest import _text
 
 CONFIG_FILENAME = "bundle-catalogs.yml"
 # Supported bundle-catalogs.yml schema (major version). Both readers of the
@@ -70,8 +71,14 @@ def install_allowed(self) -> bool:
     def from_dict(cls, data: Any, scope: Scope) -> "CatalogSource":
         if not isinstance(data, dict):
             raise BundlerError("Each catalog source must be a mapping.")
-        source_id = str(data.get("id", "")).strip()
-        url = str(data.get("url", "")).strip()
+        # ``_text`` rather than ``str(...get(k, ""))``: the default only covers a
+        # *missing* key. A key present but null -- how YAML spells an empty field
+        # (``id:`` with nothing after it) -- yields ``None``, and ``str(None)``
+        # is the literal ``"None"``, which is truthy and so sailed straight past
+        # the required-field guards below: a source with ``id: null`` was
+        # accepted and registered under the name ``"None"``.
+        source_id = _text(data.get("id"))
+        url = _text(data.get("url"))
         if not source_id:
             raise BundlerError("A catalog source is missing its 'id'.")
         if not url:
@@ -163,7 +170,11 @@ class CatalogEntry:
     def from_dict(cls, data: Any) -> "CatalogEntry":
         if not isinstance(data, dict):
             raise BundlerError("Each catalog entry must be a mapping.")
-        entry_id = str(data.get("id", "")).strip()
+        # ``_text`` here too: an ``id: null`` otherwise became the literal
+        # "None", which is truthy, so ``load_catalog_payload`` reported it as an
+        # id MISMATCH against the mapping key rather than the accurate
+        # missing-id error.
+        entry_id = _text(data.get("id"))
         # `or {}` would coerce a FALSY non-mapping (0, '', False, []) to {} before
         # the isinstance guard, silently accepting a corrupt catalog entry; only
         # an absent/None value means "not present".
@@ -185,14 +196,16 @@ def from_dict(cls, data: Any) -> "CatalogEntry":
             )
         return cls(
             id=entry_id,
-            name=str(data.get("name", "")).strip(),
-            version=str(data.get("version", "")).strip(),
-            role=str(data.get("role", "")).strip(),
-            description=str(data.get("description", "")).strip(),
-            author=str(data.get("author", "")).strip(),
-            license=str(data.get("license", "")).strip(),
-            download_url=str(data.get("download_url", "")).strip(),
-            requires_speckit_version=str(requires.get("speckit_version", "")).strip(),
+            # See the note in ``CatalogSource.from_dict``: an explicitly null
+            # field must read as empty, not as the literal string "None".
+            name=_text(data.get("name")),
+            version=_text(data.get("version")),
+            role=_text(data.get("role")),
+            description=_text(data.get("description")),
+            author=_text(data.get("author")),
+            license=_text(data.get("license")),
+            download_url=_text(data.get("download_url")),
+            requires_speckit_version=_text(requires.get("speckit_version")),
             sha256=(
                 None
                 if data.get("sha256") is None

tests/contract/test_catalog_schema.py

@@ -256,6 +256,72 @@ def test_catalog_entry_rejects_non_boolean_verified():
         CatalogEntry.from_dict(data)
 
 
+@pytest.mark.parametrize(
+    "field",
+    [
+        "name",
+        "version",
+        "role",
+        "description",
+        "author",
+        "license",
+        "download_url",
+    ],
+)
+def test_catalog_entry_explicit_null_field_reads_as_empty(field: str):
+    """An explicitly null field must read as "", not the literal "None".
+
+    `str(data.get(key, ""))` only defaults for a *missing* key. A key present
+    but null — how YAML spells an empty field (`author:` with nothing after
+    it) — yields `None`, and `str(None)` is the truthy string `"None"`. The
+    same constructor already guards `sha256` and `repository` against exactly
+    this.
+    """
+    from specify_cli.bundler.models.catalog import CatalogEntry
+
+    data = catalog_entry_dict("demo")
+    data[field] = None
+
+    entry = CatalogEntry.from_dict(data)
+
+    assert getattr(entry, field) == ""
+
+
+def test_catalog_source_rejects_an_explicitly_null_id():
+    """`id: null` must be refused, not registered as a source named "None".
+
+    The `if not source_id` guard was defeated by the truthy literal, so the
+    source was accepted and carried the name `"None"` into the stack.
+    """
+    from specify_cli.bundler.models.catalog import CatalogSource, Scope
+
+    with pytest.raises(BundlerError, match="missing its 'id'"):
+        CatalogSource.from_dict(
+            {
+                "id": None,
+                "url": "https://example.test/catalog.json",
+                "priority": 5,
+                "install_policy": "install-allowed",
+            },
+            Scope.PROJECT,
+        )
+
+
+def test_catalog_source_rejects_an_explicitly_null_url():
+    from specify_cli.bundler.models.catalog import CatalogSource, Scope
+
+    with pytest.raises(BundlerError, match="missing its 'url'"):
+        CatalogSource.from_dict(
+            {
+                "id": "demo",
+                "url": None,
+                "priority": 5,
+                "install_policy": "install-allowed",
+            },
+            Scope.PROJECT,
+        )
+
+
 def test_catalog_entry_preserves_sha256_through_provenance():
     digest = "a" * 64
     payload = catalog_payload(
@@ -338,3 +404,37 @@ def test_catalog_entry_rejects_falsy_non_mapping(field, bad):
     data[field] = bad
     with pytest.raises(BundlerError, match=f"'{field}' must be a mapping"):
         CatalogEntry.from_dict(data)
+
+
+def test_catalog_entry_explicit_null_id_reports_missing_id():
+    """`id: null` must surface as the missing-id error, not an id mismatch.
+
+    `entry_id` was still computed with `str(data.get("id", ""))`, so an explicit
+    null became the literal "None". That is truthy, so `load_catalog_payload`
+    compared it against the mapping key and reported
+    "id mismatch: key 'demo' != entry id 'None'" instead of the accurate
+    missing-id error.
+    """
+    from specify_cli.bundler.models.catalog import CatalogEntry
+
+    data = catalog_entry_dict("demo")
+    data["id"] = None
+    assert CatalogEntry.from_dict(data).id == ""
+
+    with pytest.raises(BundlerError, match="missing its 'id' field"):
+        load_catalog_payload(catalog_payload({"demo": data}))
+
+
+def test_catalog_entry_explicit_null_requires_speckit_version_reads_as_empty():
+    """The nested `requires.speckit_version` branch is covered too.
+
+    It was switched to `_text` alongside the top-level fields, but only the
+    top-level attributes were exercised — so this branch could regress while
+    the suite still passed.
+    """
+    from specify_cli.bundler.models.catalog import CatalogEntry
+
+    data = catalog_entry_dict("demo")
+    data["requires"] = {"speckit_version": None}
+
+    assert CatalogEntry.from_dict(data).requires_speckit_version == ""