From cadcd2da5106f0749de21177f6ad3b01dc941b5b Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 11 Sep 2026 21:17:42 +0500 Subject: [PATCH] fix(bundler): validate a catalog source before persisting it `add_source` wrote the new entry and only then constructed it: catalogs.append(entry) _write(project_root, catalogs) return CatalogSource.from_dict(entry, Scope.PROJECT) `CatalogSource.from_dict` already rejects an empty id, but by the time it ran the entry was on disk. A whitespace-only `--id` (or a url from which no id can be derived) therefore reported an error while leaving a broken entry behind: add_source -> raised: A catalog source is missing its 'id'. AFTER : [{"id": "", "url": "https://example.test/c.json", ...}] The user sees a failure and reasonably assumes nothing happened. In fact the project's catalog config is now unusable -- every command that loads the stack fails on that entry: load_source_stack -> BROKEN: BundlerError A catalog source is missing its 'id'. so the project stays wedged until bundle-catalogs.yml is hand-edited. Constructing before writing reuses the validation that already exists; nothing is persisted when it fails. Co-Authored-By: Claude Opus 5 (1M context) --- .../bundler/commands_impl/catalog_config.py | 10 ++++++- tests/unit/test_bundler_catalog_config.py | 28 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index f763a21c65..afd0357e66 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -198,9 +198,17 @@ def add_source( "priority": int(priority), "install_policy": install_policy.value, } + # Construct BEFORE writing. ``CatalogSource.from_dict`` already rejects an + # empty id, but it used to run after ``_write`` had persisted the entry, so + # a whitespace-only ``--id`` (or a url from which no id can be derived) + # reported "A catalog source is missing its 'id'" while leaving + # ``{"id": "", ...}`` behind in bundle-catalogs.yml. Every later command + # that loads the stack then failed with that same error, wedging the + # project's catalog config until the file was hand-edited. + source = CatalogSource.from_dict(entry, Scope.PROJECT) catalogs.append(entry) _write(project_root, catalogs) - return CatalogSource.from_dict(entry, Scope.PROJECT) + return source def remove_source(project_root: Path, id_or_url: str) -> str: diff --git a/tests/unit/test_bundler_catalog_config.py b/tests/unit/test_bundler_catalog_config.py index 46c333700a..0ff17e6370 100644 --- a/tests/unit/test_bundler_catalog_config.py +++ b/tests/unit/test_bundler_catalog_config.py @@ -89,6 +89,34 @@ def test_remove_source_accepts_relative_local_path(tmp_path: Path, monkeypatch): cc.remove_source(project, "sub/cat.json") +def test_failed_add_does_not_persist_a_broken_entry(tmp_path: Path): + """A rejected `catalog add` must leave the config untouched. + + `CatalogSource.from_dict` already rejects an empty id, but it ran *after* + `_write` had persisted the entry, so a whitespace-only `--id` reported + "A catalog source is missing its 'id'" while leaving `{"id": "", ...}` + behind — and every later command that loads the stack then failed with that + same error, wedging the project's catalog config. + """ + from specify_cli.bundler.services.catalog_stack import load_source_stack + + project = tmp_path / "proj" + (project / ".specify").mkdir(parents=True) + + with pytest.raises(BundlerError, match="missing its 'id'"): + cc.add_source( + project, + "https://example.test/c.json", + source_id=" ", + policy="install-allowed", + priority=5, + ) + + assert cc._read(project) == [] + # The stack must still load — this is what the stray entry used to break. + assert [source.id for source in load_source_stack(project)] + + def test_remove_by_id_does_not_also_delete_canonical_url_match(tmp_path: Path, monkeypatch): """`remove ` must remove only the exact-id source, not also a different source whose url happens to equal the id's canonicalized path. (_canonicalize_url