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

[jawwad-ali]

Problem

CatalogSource.from_dict and CatalogEntry.from_dict coerce fields with str(data.get(key, "")).

That default only covers a missing key. A key that is present but null — how YAML spells an empty field (author: with nothing after it), and what a generator emits for an absent value in JSON — yields None, and str(None) is the literal "None".

Reproduction on current main (c173bf1)

CatalogEntry.from_dict({"id": "demo", "name": None, "version": None, ...})

name              = 'None'   <-- literal 'None'
version           = 'None'   <-- literal 'None'
role              = 'None'   <-- literal 'None'
description       = 'None'   <-- literal 'None'
author            = 'None'   <-- literal 'None'
license           = 'None'   <-- literal 'None'
download_url      = 'None'   <-- literal 'None'
sha256            = None     (already guarded)
repository        = None     (already guarded)

The same constructor already guards sha256 and repository against exactly this, so the treatment was internally inconsistent.

For CatalogSource it is more than cosmetic

"None" is truthy, so it defeats the required-field guards and the malformed source is accepted:

CatalogSource.from_dict({"id": None, "url": None, "priority": 5, "install_policy": "install-allowed"}, Scope.PROJECT)
# -> id = 'None', url = 'None'

if not source_id:
    raise BundlerError("A catalog source is missing its 'id'.")   # never fires

A user whose config has an empty id: silently gets a catalog source registered under the name "None", pointing at a URL "None".

Fix

Route both constructors through the existing manifest._text helper, whose docstring already describes this precise failure mode for bundle manifests:

A .get(key, "") default only covers a missing key. A key that is present but null … yields None, and str(None) is the literal "None".

After the fix:

fields still literal 'None': NONE  <-- fixed
null id : refused -> A catalog source is missing its 'id'.
null url: refused -> Catalog source 'ok' is missing its 'url'.
normal source still works: ok https://example.test/catalog.json

Verification

• Fail-before / pass-after: 9 new-vs-baseline failures with the source reverted to upstream/main → 54 passed with the fix.
• Parametrized across all seven affected CatalogEntry fields, plus dedicated tests for the null id and null url sources.
• Scoped regression over tests/contract: 170 passed, up from a clean-main baseline of 161 passed, with zero pre-existing or new failures.
• No circular import introduced by the models.manifest import (verified by importing models.catalog directly).
• uvx ruff@0.15.0 check src tests → clean

Behaviour change, disclosed: only inputs that previously produced the literal "None" change. A source with a null id/url now raises the accurate "missing its 'id'/'url'" error instead of being accepted; download_url: null now reads as empty, so it reports "has no download_url" rather than failing later as a non-HTTP(S) URL.

---

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

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Catalog entry IDs remain incorrectly normalized, and the nested version-field change lacks regression coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Normalizes explicit null catalog values to empty strings using _text.

Changes:

• Fixes null source and catalog-entry parsing.
• Adds regression tests for affected fields and required source values.
• Reported verification was reviewed but not rerun.

File summaries

File	Description
src/specify_cli/bundler/models/catalog.py	Applies null-safe text normalization.
tests/contract/test_catalog_schema.py	Adds null-value regression coverage.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Balanced

---

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

[mnriem]

Please address Copilot feedback and resolve conflicts

[mnriem]

Please resolve conflicts

[jawwad-ali]

@mnriem Conflicts resolved (f8f6682). The fix moved to src/specify_cli/bundles/catalogs.py, with imports adapted to the new layout (from . import BundlerError, from .manifest import _text). Every hunk (source_id, url, entry_id, name, download_url, requires_speckit_version through _text) is confirmed present, and the tests import from specify_cli.bundles.catalogs.

Verified: 11 new cases fail with the source reverted to main; 57 pass with the fix.

Rebased as a single commit on current main; uvx ruff@0.15.0 check src tests is clean. Any remaining local failures are the pre-existing Windows symlink-privilege tests, which fail identically on unmodified main.

[Copilot]

Copilot review overview

🟢 Approval recommended

The focused fix is consistent with existing normalization behavior and has comprehensive regression coverage.

Review effort: Balanced
Findings: None

Resolved since last review (2)

• CatalogEntry.from_dict still computes entry_id with str(data.get("id", "")), so an explicit…
• The implementation also changes requires.speckit_version to use _text, but this regression test…