fix(bundler): validate a catalog source before persisting it

[jawwad-ali]

Problem

add_source writes the new entry and only then constructs it:

catalogs.append(entry)
_write(project_root, catalogs)
return CatalogSource.from_dict(entry, Scope.PROJECT)   # <-- validation, too late

CatalogSource.from_dict already rejects an empty id — but by the time it runs, the entry is on disk.

Reproduction on current main (c173bf1)

before     : []
add_source -> raised: A catalog source is missing its 'id'.
AFTER      : [{"id": "", "url": "https://example.test/c.json", "priority": 5, "install_policy": "install-allowed"}]

The command reports a failure, so the user reasonably assumes nothing happened.

It wedges the project

Every command that loads the catalog stack now fails on that stray entry:

load_source_stack -> BROKEN: BundlerError A catalog source is missing its 'id'.

So a single mistyped catalog add --id " " leaves the project's catalog config unusable until bundle-catalogs.yml is hand-edited — by hand, for a file the bundle catalog commands exist to manage.

Fix

Construct before writing, reusing the validation that already exists rather than adding a second check that could drift:

source = CatalogSource.from_dict(entry, Scope.PROJECT)
catalogs.append(entry)
_write(project_root, catalogs)
return source

After the fix:

add with blank id -> refused: A catalog source is missing its 'id'.
config after      : []                      <-- nothing persisted
stack still loads : ['default', 'community']
normal add works  : ok https://example.test/ok.json

Verification

• Fail-before / pass-after: 1 new-vs-baseline failure with the source reverted to upstream/main → passing with the fix.
• The test asserts both halves: the config is left empty and load_source_stack still succeeds — the latter is what the stray entry actually broke.
• Scoped regression over tests/unit: 536 passed vs a clean-main baseline of 535 passed, same 2 pre-existing failures, none new.
• uvx ruff@0.15.0 check src tests → clean

No behaviour change for any input that previously succeeded. The reordered call is the same construction, just earlier, and it raises for exactly the input that already raised — the difference is that the failure no longer leaves a file behind.

Note on overlap: this touches commands_impl/catalog_config.py, as does my #4533, but a different function (add_source vs remove_source). Happy to rebase whichever lands second.

---

Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

🤖 Generated with Claude Code

[Copilot]

🟢 Approval recommended

The fix and regression coverage address the persistence issue with no unresolved blockers.

Pull request overview

Prevents invalid catalog sources from being persisted by validating them before writing configuration.

Changes:

• Validates CatalogSource before updating catalog configuration.
• Adds regression coverage for rejected additions.

File summaries

File	Description
tests/unit/test_bundler_catalog_config.py	Adds regression coverage for rejected additions.
src/specify_cli/bundler/commands_impl/catalog_config.py	Reorders validation before persistence.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[mnriem]

Please resolve conflicts

[jawwad-ali]

@mnriem — I went to resolve the conflicts, and I think this PR should be closed as superseded rather than rebased. main's rewrite of add_source (now bundles/catalog_config.py) already fixes it, and in a way my test contradicts.

The bug is structurally fixed on main. The original defect was ordering: _write ran before CatalogSource.from_dict, so a rejected source was persisted anyway. main now builds the source first:

190:    requested_source = CatalogSource.from_dict(
...
222:    _write(project_root, catalogs)

so anything from_dict rejects is rejected before anything is written.

The specific trigger no longer fails at all. On current main, a whitespace-only --id falls back to the derived id:

add with blank id -> ACCEPTED
config after: [{'id': 'example-test-c', 'url': 'https://example.test/c.json', ...}]

That's a deliberate semantic change, and it means my regression test now fails against main with DID NOT RAISE — it asserts the old behaviour. Rebasing would leave a PR with no remaining fix and a test that contradicts intended behaviour, which isn't worth your review time.

Happy to close this myself if you'd prefer — I've left it for you since you closed #4135 in the same situation. Thanks for the rewrite; it addresses the root cause more thoroughly than my reordering did.

[mnriem]

Closing it as per comment above