From 118ec791bc772463cb4f342ceaa39ab205889f34 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 11 Sep 2026 13:12:12 -0500 Subject: [PATCH 01/11] feat: make catalog add idempotent across all catalog families (#4505) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rerunning `catalog add` for an already-configured entry now succeeds as a no-op instead of failing, so the command is safe inside re-runnable workflows. This implements the assessment's recommended Option A ("idempotent no-op for equivalent entries"): an identical rerun exits 0, while a request that matches an existing entry's identity but supplies different settings is rejected as a conflict rather than silently overwriting priority/install permissions. Applied consistently to all six families: - extension / preset — identity = catalog name - integration / workflow / step — identity = catalog URL - bundle — identity = source id or url The URL-identity class methods now return "added"/"unchanged" so the CLI can report the no-op, and bundle add_source returns (source, status). Updates duplicate-add tests to the new semantics, adds no-op + conflict coverage per family, and documents the behavior in the reference docs. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/reference/bundles.md | 2 + docs/reference/extensions.md | 2 + docs/reference/integrations.md | 2 + docs/reference/presets.md | 2 + docs/reference/workflows.md | 2 + .../bundler/commands_impl/catalog_config.py | 31 ++++++++---- src/specify_cli/commands/bundle/__init__.py | 16 ++++-- src/specify_cli/extensions/_commands.py | 22 +++++++- .../integrations/_query_commands.py | 9 +++- src/specify_cli/integrations/catalog.py | 26 +++++++--- src/specify_cli/presets/_commands.py | 22 +++++++- src/specify_cli/workflows/_commands.py | 14 ++++-- src/specify_cli/workflows/catalog.py | 45 ++++++++++++++--- tests/contract/test_bundle_cli.py | 34 +++++++++++++ tests/integrations/test_cli.py | 17 ++++++- .../integrations/test_integration_catalog.py | 29 ++++++++--- tests/test_extensions.py | 50 +++++++++++++++++++ tests/test_presets.py | 38 ++++++++++++++ tests/test_workflows.py | 36 ++++++++++--- tests/unit/test_bundler_catalog_config.py | 7 +-- 20 files changed, 347 insertions(+), 59 deletions(-) diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index bb2a6aa7c7..a047678e6b 100644 --- a/docs/reference/bundles.md +++ b/docs/reference/bundles.md @@ -175,6 +175,8 @@ specify bundle catalog add Registers a project-scoped catalog source and persists it. +Adding a source is idempotent (identity is the source **id or url**): re-running `catalog add` with the same id/url and identical `--policy`/`--priority` is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding a matching id/url with *different* settings is rejected as a conflict rather than silently overwriting the existing source — remove it first to change it. + ### Remove a Catalog Source ```bash diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 22357ccea0..a3712aea04 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -156,6 +156,8 @@ specify extension catalog add Adds a catalog to the project's `.specify/extension-catalogs.yml`. +Adding a catalog is idempotent (identity is the catalog **name**): re-running `catalog add` with the same name and identical settings is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same name with *different* settings is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. + ### Remove a Catalog ```bash diff --git a/docs/reference/integrations.md b/docs/reference/integrations.md index 32310cf81f..87ebea3a22 100644 --- a/docs/reference/integrations.md +++ b/docs/reference/integrations.md @@ -233,6 +233,8 @@ specify integration catalog add Adds a custom catalog URL to the project's `.specify/integration-catalogs.yml`. The URL must use HTTPS (except `http://localhost`, `http://127.0.0.1`, or `http://[::1]` for local testing). +Adding a catalog is idempotent (identity is the catalog **URL**): re-running `catalog add` with the same URL and the same (or no) `--name` is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same URL with a *different* `--name` is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. + ### Remove a Catalog ```bash diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 0723200d67..64824daf2f 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -123,6 +123,8 @@ specify preset catalog add Adds a catalog to the project's `.specify/preset-catalogs.yml`. +Adding a catalog is idempotent (identity is the catalog **name**): re-running `catalog add` with the same name and identical settings is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same name with *different* settings is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. + ### Remove a Catalog ```bash diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index a547a10e42..abef928228 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -405,6 +405,8 @@ specify workflow catalog add Adds a custom catalog URL to the project's `.specify/workflow-catalogs.yml`. +Adding a catalog is idempotent (identity is the catalog **URL**): re-running `catalog add` with the same URL and the same (or no) `--name` is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same URL with a *different* `--name` is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. + ### Remove a Catalog ```bash diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index f763a21c65..7bc669d093 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -139,7 +139,7 @@ def add_source( policy: str, priority: int, source_id: str | None = None, -) -> CatalogSource: +) -> tuple[CatalogSource, str]: url = url.strip() if not url: raise BundlerError("A catalog url is required.") @@ -186,21 +186,32 @@ def add_source( resolved_id = (source_id or _derive_id(url)).strip() catalogs = _read(project_root) - for existing in catalogs: - if existing.get("id") == resolved_id or existing.get("url") == url: - raise BundlerError( - f"Catalog source '{resolved_id}' (or url) already exists in this project." - ) - - entry = { + desired = { "id": resolved_id, "url": url, "priority": int(priority), "install_policy": install_policy.value, } - catalogs.append(entry) + for existing in catalogs: + if existing.get("id") == resolved_id or existing.get("url") == url: + # Idempotent add (#4505): identity is the source id or url. A rerun + # requesting the same settings is a successful no-op; differing + # settings are a conflict rather than a silent overwrite. + if ( + existing.get("id") == resolved_id + and existing.get("url") == url + and int(existing.get("priority", 0)) == desired["priority"] + and str(existing.get("install_policy", "")) == desired["install_policy"] + ): + return CatalogSource.from_dict(dict(existing), Scope.PROJECT), "unchanged" + raise BundlerError( + f"Catalog source '{resolved_id}' (or url) already exists in this " + "project with different settings. Remove it first to change it." + ) + + catalogs.append(desired) _write(project_root, catalogs) - return CatalogSource.from_dict(entry, Scope.PROJECT) + return CatalogSource.from_dict(desired, Scope.PROJECT), "added" def remove_source(project_root: Path, id_or_url: str) -> str: diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index b809afba80..f37b23b605 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -672,15 +672,21 @@ def catalog_add( project_root = require_project_root() from ...bundler.commands_impl.catalog_config import add_source - source = add_source(project_root, url, policy=policy, priority=priority, source_id=source_id) + source, status = add_source(project_root, url, policy=policy, priority=priority, source_id=source_id) except BundlerError as exc: _fail(str(exc)) return - console.print( - f"[green]✓[/green] Added catalog '{_escape_markup(str(source.id))}' " - f"(priority {source.priority}, {source.install_policy.value})." - ) + if status == "unchanged": + console.print( + f"[green]✓[/green] Catalog '{_escape_markup(str(source.id))}' is already " + f"configured (priority {source.priority}, {source.install_policy.value})." + ) + else: + console.print( + f"[green]✓[/green] Added catalog '{_escape_markup(str(source.id))}' " + f"(priority {source.priority}, {source.install_policy.value})." + ) @bundle_catalog_app.command("remove") diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 454482d054..6550e0e177 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -640,10 +640,28 @@ def catalog_add( safe_name = _escape_markup(name) safe_url = _escape_markup(url) - # Check for duplicate name + # Idempotent add (#4505): a rerun that requests an identical entry is a + # successful no-op so the same `catalog add` can live in a re-runnable + # workflow without failing. A same-name entry whose settings differ is + # still a conflict — we refuse to silently change priority/install + # permissions and ask the user to remove it first. for existing in catalogs: if isinstance(existing, dict) and existing.get("name") == name: - console.print(f"[yellow]Warning:[/yellow] A catalog named '{safe_name}' already exists.") + if ( + str(existing.get("url", "")) == url + and existing.get("priority") == priority + and bool(existing.get("install_allowed", False)) == install_allowed + and str(existing.get("description", "")) == description + ): + console.print( + f"[green]✓[/green] Catalog '[bold]{safe_name}[/bold]' is already " + "configured with these settings; nothing to do." + ) + return + console.print( + f"[red]Error:[/red] A catalog named '{safe_name}' already exists with " + "different settings." + ) console.print("Use 'specify extension catalog remove' first, or choose a different name.") raise typer.Exit(1) diff --git a/src/specify_cli/integrations/_query_commands.py b/src/specify_cli/integrations/_query_commands.py index 0cd254879a..5525822923 100644 --- a/src/specify_cli/integrations/_query_commands.py +++ b/src/specify_cli/integrations/_query_commands.py @@ -543,14 +543,19 @@ def integration_catalog_add( normalized_url = url.strip() try: - catalog.add_catalog(normalized_url, name) + status = catalog.add_catalog(normalized_url, name) except IntegrationCatalogError as exc: # Covers both URL validation (base class) and config-file validation # (IntegrationValidationError subclass). console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) - console.print(f"[green]✓[/green] Catalog source added: {normalized_url}") + if status == "unchanged": + console.print( + f"[green]✓[/green] Catalog source already configured: {normalized_url}" + ) + else: + console.print(f"[green]✓[/green] Catalog source added: {normalized_url}") @integration_catalog_app.command("remove") diff --git a/src/specify_cli/integrations/catalog.py b/src/specify_cli/integrations/catalog.py index b8d76cb9c6..70dcb9caac 100644 --- a/src/specify_cli/integrations/catalog.py +++ b/src/specify_cli/integrations/catalog.py @@ -390,14 +390,20 @@ def get_project_catalog_configs(self) -> Optional[List[Dict[str, Any]]]: for e in entries ] - def add_catalog(self, url: str, name: Optional[str] = None) -> None: + def add_catalog(self, url: str, name: Optional[str] = None) -> str: """Add a catalog source to the project-level config file. The URL is normalized (whitespace stripped) and validated before being - written. Duplicate URLs are rejected, including near-duplicates that - differ only by surrounding whitespace. Priority is derived as - ``max(existing) + 1`` so the new entry sorts last in the resolution - order unless the user edits the file manually. + written. Identity for an integration catalog is the (normalized) URL. + Adding a URL that is already configured is idempotent (#4505): a rerun + that requests the same name (or no explicit name) is a successful + no-op, while a rerun that requests a *different* name is rejected as a + conflict rather than silently overwriting the stored entry. Priority is + derived as ``max(existing) + 1`` so a newly added entry sorts last in + the resolution order unless the user edits the file manually. + + Returns ``"added"`` when a new entry is written, or ``"unchanged"`` + when an equivalent entry already existed. """ url = url.strip() if not url: @@ -432,6 +438,7 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> None: # Validate each existing entry before mutating anything. Fail fast so # we don't silently preserve a corrupt sibling entry or derive a new # priority from a bogus value. + requested_name = str(name).strip() if name is not None else "" existing_priorities: List[int] = [] valid_catalog_count = 0 for idx, cat in enumerate(catalogs): @@ -452,8 +459,14 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> None: f"Invalid catalog entry at index {idx} in {config_path}: {exc}" ) from exc if existing_url == url: + # Idempotent add (#4505): same URL already configured. + existing_name = str(cat.get("name", "")).strip() + if not requested_name or requested_name == existing_name: + return "unchanged" raise IntegrationValidationError( - f"Catalog URL already configured: {url}" + f"Catalog URL already configured with a different name " + f"('{existing_name}'): {url}. Remove it first or pass " + f"--name '{existing_name}'." ) valid_catalog_count += 1 if "priority" in cat: @@ -502,6 +515,7 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> None: sort_keys=False, allow_unicode=True, ) + return "added" def remove_catalog(self, index: int) -> str: """Remove a catalog source by 0-based index. diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index c5dbf0ddca..863415b91c 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -928,10 +928,28 @@ def preset_catalog_add( safe_name = _escape_markup(str(name)) safe_url = _escape_markup(str(url)) - # Check for duplicate name + # Idempotent add (#4505): a rerun that requests an identical entry is a + # successful no-op so the same `catalog add` can live in a re-runnable + # workflow without failing. A same-name entry whose settings differ is + # still a conflict — we refuse to silently change priority/install + # permissions and ask the user to remove it first. for existing in catalogs: if isinstance(existing, dict) and existing.get("name") == name: - console.print(f"[yellow]Warning:[/yellow] A catalog named '{safe_name}' already exists.") + if ( + str(existing.get("url", "")) == url + and existing.get("priority") == priority + and bool(existing.get("install_allowed", False)) == install_allowed + and str(existing.get("description", "")) == description + ): + console.print( + f"[green]✓[/green] Catalog '[bold]{safe_name}[/bold]' is already " + "configured with these settings; nothing to do." + ) + return + console.print( + f"[red]Error:[/red] A catalog named '{safe_name}' already exists with " + "different settings." + ) console.print("Use 'specify preset catalog remove' first, or choose a different name.") raise typer.Exit(1) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 7691066714..db648a6f57 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3025,12 +3025,15 @@ def workflow_catalog_add( project_root = _require_specify_project() catalog = WorkflowCatalog(project_root) try: - catalog.add_catalog(url, name) + status = catalog.add_catalog(url, name) except WorkflowValidationError as exc: console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) - console.print(f"[green]✓[/green] Catalog source added: {url}") + if status == "unchanged": + console.print(f"[green]✓[/green] Catalog source already configured: {url}") + else: + console.print(f"[green]✓[/green] Catalog source added: {url}") @workflow_catalog_app.command("remove") @@ -3771,12 +3774,15 @@ def workflow_step_catalog_add( catalog = StepCatalog(project_root) try: - catalog.add_catalog(url, name) + status = catalog.add_catalog(url, name) except StepValidationError as exc: console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) - console.print(f"[green]✓[/green] Step catalog source added: {url}") + if status == "unchanged": + console.print(f"[green]✓[/green] Step catalog source already configured: {url}") + else: + console.print(f"[green]✓[/green] Step catalog source added: {url}") @workflow_step_catalog_app.command("remove") diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 5fffa4b45f..72b3431e0d 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -705,8 +705,15 @@ def get_catalog_configs(self) -> list[dict[str, Any]]: for e in entries ] - def add_catalog(self, url: str, name: str | None = None) -> None: - """Add a catalog source to the project-level config.""" + def add_catalog(self, url: str, name: str | None = None) -> str: + """Add a catalog source to the project-level config. + + Identity is the URL: adding a URL that is already configured is + idempotent (#4505). A rerun requesting the same name (or no explicit + name) is a no-op that returns ``"unchanged"``; a rerun requesting a + different name is rejected as a conflict. Returns ``"added"`` when a + new entry is written. + """ self._validate_catalog_url(url) config_path = self.project_root / ".specify" / "workflow-catalogs.yml" @@ -731,11 +738,18 @@ def add_catalog(self, url: str, name: str | None = None) -> None: raise WorkflowValidationError( "Catalog config 'catalogs' must be a list." ) - # Check for duplicate URL (guard against non-dict entries) + # Idempotent add (#4505): identity is the URL. A rerun requesting the + # same name (or no explicit name) is a no-op; a different name conflicts. + requested_name = str(name).strip() if name is not None else "" for cat in catalogs: if isinstance(cat, dict) and cat.get("url") == url: + existing_name = str(cat.get("name", "")).strip() + if not requested_name or requested_name == existing_name: + return "unchanged" raise WorkflowValidationError( - f"Catalog URL already configured: {url}" + f"Catalog URL already configured with a different name " + f"('{existing_name}'): {url}. Remove it first or pass " + f"--name '{existing_name}'." ) # Derive priority from the highest existing priority + 1. @@ -776,6 +790,7 @@ def _coerce_priority(value: Any) -> int: raise WorkflowValidationError( f"Failed to write catalog config {config_path}: {exc}" ) from exc + return "added" def remove_catalog(self, index: int) -> str: """Remove a catalog source by index (0-based). Returns the removed name.""" @@ -1388,8 +1403,15 @@ def get_catalog_configs(self) -> list[dict[str, Any]]: for e in entries ] - def add_catalog(self, url: str, name: str | None = None) -> None: - """Add a catalog source to the project-level config.""" + def add_catalog(self, url: str, name: str | None = None) -> str: + """Add a catalog source to the project-level config. + + Identity is the URL: adding a URL that is already configured is + idempotent (#4505). A rerun requesting the same name (or no explicit + name) is a no-op that returns ``"unchanged"``; a rerun requesting a + different name is rejected as a conflict. Returns ``"added"`` when a + new entry is written. + """ self._validate_catalog_url(url) config_path = self.project_root / ".specify" / "step-catalogs.yml" @@ -1414,10 +1436,18 @@ def add_catalog(self, url: str, name: str | None = None) -> None: raise StepValidationError( "Catalog config 'catalogs' must be a list." ) + # Idempotent add (#4505): identity is the URL. A rerun requesting the + # same name (or no explicit name) is a no-op; a different name conflicts. + requested_name = str(name).strip() if name is not None else "" for cat in catalogs: if isinstance(cat, dict) and cat.get("url") == url: + existing_name = str(cat.get("name", "")).strip() + if not requested_name or requested_name == existing_name: + return "unchanged" raise StepValidationError( - f"Catalog URL already configured: {url}" + f"Catalog URL already configured with a different name " + f"('{existing_name}'): {url}. Remove it first or pass " + f"--name '{existing_name}'." ) # Coerce existing priorities to int with a safe fallback so a user-edited @@ -1459,6 +1489,7 @@ def _coerce_priority(value: Any) -> int: raise StepValidationError( f"Failed to write catalog config {config_path}: {exc}" ) from exc + return "added" def remove_catalog(self, index: int) -> str: """Remove a catalog source by index (0-based). Returns the removed name.""" diff --git a/tests/contract/test_bundle_cli.py b/tests/contract/test_bundle_cli.py index 6db4dab769..3029064e2a 100644 --- a/tests/contract/test_bundle_cli.py +++ b/tests/contract/test_bundle_cli.py @@ -211,6 +211,40 @@ def test_catalog_add_and_remove(project: Path): assert removed.exit_code == 0 +def test_catalog_add_duplicate_is_idempotent(project: Path): + catalog = project / "local-catalog.json" + write_catalog_file(catalog, {"demo": catalog_entry_dict("demo")}) + + first = runner.invoke( + app, + ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "10"], + ) + assert first.exit_code == 0, first.output + second = runner.invoke( + app, + ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "10"], + ) + assert second.exit_code == 0, second.output + assert "already" in second.output + + +def test_catalog_add_duplicate_different_settings_conflicts(project: Path): + catalog = project / "local-catalog.json" + write_catalog_file(catalog, {"demo": catalog_entry_dict("demo")}) + + first = runner.invoke( + app, + ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "10"], + ) + assert first.exit_code == 0, first.output + second = runner.invoke( + app, + ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "20"], + ) + assert second.exit_code == 1 + assert "different settings" in second.output + + def test_catalog_remove_builtin_is_refused(project: Path): result = runner.invoke(app, ["bundle", "catalog", "remove", "default"]) assert result.exit_code == 1 diff --git a/tests/integrations/test_cli.py b/tests/integrations/test_cli.py index 2beb411a2a..6ffe023da9 100644 --- a/tests/integrations/test_cli.py +++ b/tests/integrations/test_cli.py @@ -2539,7 +2539,7 @@ def test_catalog_add_rejects_invalid_url(self, tmp_path, monkeypatch): assert result.exit_code == 1 assert "HTTPS" in result.output - def test_catalog_add_rejects_duplicate(self, tmp_path, monkeypatch): + def test_catalog_add_duplicate_is_idempotent(self, tmp_path, monkeypatch): project = self._make_project(tmp_path) url = "https://dup.example.com/catalog.json" first = self._invoke( @@ -2549,9 +2549,22 @@ def test_catalog_add_rejects_duplicate(self, tmp_path, monkeypatch): second = self._invoke( ["integration", "catalog", "add", url], project ) - assert second.exit_code == 1 + assert second.exit_code == 0, second.output assert "already configured" in second.output + def test_catalog_add_duplicate_different_name_conflicts(self, tmp_path, monkeypatch): + project = self._make_project(tmp_path) + url = "https://dup.example.com/catalog.json" + first = self._invoke( + ["integration", "catalog", "add", url, "--name", "first"], project + ) + assert first.exit_code == 0, first.output + second = self._invoke( + ["integration", "catalog", "add", url, "--name", "second"], project + ) + assert second.exit_code == 1 + assert "different name" in second.output + def test_catalog_remove_out_of_range(self, tmp_path, monkeypatch): project = self._make_project(tmp_path) # Need a config file for remove to attempt an index lookup diff --git a/tests/integrations/test_integration_catalog.py b/tests/integrations/test_integration_catalog.py index c414c3d8ea..f413640c37 100644 --- a/tests/integrations/test_integration_catalog.py +++ b/tests/integrations/test_integration_catalog.py @@ -1168,12 +1168,23 @@ def test_add_catalog_normalizes_name(self, tmp_path, monkeypatch): entries = data["catalogs"] assert [e["name"] for e in entries] == ["mine", "catalog-2"] - def test_add_catalog_rejects_duplicate_url(self, tmp_path, monkeypatch): + def test_add_catalog_duplicate_url_is_idempotent_noop(self, tmp_path, monkeypatch): + """Re-adding the same URL (no explicit name) is a successful no-op (#4505).""" self._isolate(tmp_path, monkeypatch) cat = IntegrationCatalog(tmp_path) - cat.add_catalog("https://dup.example.com/catalog.json") - with pytest.raises(IntegrationValidationError, match="already configured"): - cat.add_catalog("https://dup.example.com/catalog.json") + assert cat.add_catalog("https://dup.example.com/catalog.json") == "added" + assert cat.add_catalog("https://dup.example.com/catalog.json") == "unchanged" + cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" + data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert len(data["catalogs"]) == 1 + + def test_add_catalog_duplicate_url_different_name_conflicts(self, tmp_path, monkeypatch): + """Re-adding the same URL with a different name is rejected as a conflict (#4505).""" + self._isolate(tmp_path, monkeypatch) + cat = IntegrationCatalog(tmp_path) + cat.add_catalog("https://dup.example.com/catalog.json", name="first") + with pytest.raises(IntegrationValidationError, match="different name"): + cat.add_catalog("https://dup.example.com/catalog.json", name="second") def test_add_catalog_rejects_invalid_url(self, tmp_path, monkeypatch): self._isolate(tmp_path, monkeypatch) @@ -1502,13 +1513,15 @@ def test_add_catalog_strips_whitespace_in_url(self, tmp_path, monkeypatch): data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) assert data["catalogs"][0]["url"] == "https://a.example.com/catalog.json" - def test_add_catalog_rejects_whitespace_only_duplicate(self, tmp_path, monkeypatch): - """A second add with only whitespace differences must be rejected as a duplicate.""" + def test_add_catalog_whitespace_only_duplicate_is_noop(self, tmp_path, monkeypatch): + """A second add differing only by whitespace (no new name) is an idempotent no-op.""" self._isolate(tmp_path, monkeypatch) cat = IntegrationCatalog(tmp_path) cat.add_catalog("https://a.example.com/catalog.json", name="a") - with pytest.raises(IntegrationValidationError, match="already configured"): - cat.add_catalog(" https://a.example.com/catalog.json ") + assert cat.add_catalog(" https://a.example.com/catalog.json ") == "unchanged" + cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" + data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert len(data["catalogs"]) == 1 def test_remove_catalog_wraps_unlink_oserror(self, tmp_path, monkeypatch): """An OSError from `Path.unlink` surfaces as IntegrationValidationError.""" diff --git a/tests/test_extensions.py b/tests/test_extensions.py index fb6da1803e..039a767e28 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7598,6 +7598,56 @@ def test_catalog_add_escapes_url_markup(self, tmp_path): assert result.exit_code == 0, result.output assert f"URL: {url}" in result.output + def test_catalog_add_duplicate_is_idempotent(self, tmp_path): + """Re-adding an identical catalog is a successful no-op (#4505).""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + args = [ + "extension", "catalog", "add", + "https://example.com/catalog.json", "--name", "community", + ] + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + first = runner.invoke(app, args, catch_exceptions=True) + second = runner.invoke(app, args, catch_exceptions=True) + + assert first.exit_code == 0, first.output + assert second.exit_code == 0, second.output + assert "nothing to do" in second.output + + def test_catalog_add_duplicate_different_settings_conflicts(self, tmp_path): + """Re-adding a same-named catalog with different settings errors (#4505).""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + first = runner.invoke(app, [ + "extension", "catalog", "add", + "https://example.com/catalog.json", "--name", "community", + "--priority", "10", + ], catch_exceptions=True) + second = runner.invoke(app, [ + "extension", "catalog", "add", + "https://example.com/catalog.json", "--name", "community", + "--priority", "20", + ], catch_exceptions=True) + + assert first.exit_code == 0, first.output + assert second.exit_code == 1 + assert "different settings" in second.output + def test_catalog_add_escapes_config_saved_path_markup(self, tmp_path): """Catalog add's saved-path label should render literally under Rich.""" from typer.testing import CliRunner diff --git a/tests/test_presets.py b/tests/test_presets.py index 17aef20dce..fee6f5001a 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3915,6 +3915,44 @@ def test_catalog_add_escapes_rich_markup(self, project_dir): assert config["catalogs"][0]["name"] == name assert config["catalogs"][0]["url"] == url + def test_catalog_add_duplicate_is_idempotent(self, project_dir): + """Re-adding an identical preset catalog is a successful no-op (#4505).""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + args = [ + "preset", "catalog", "add", + "https://example.com/c.json", "--name", "mine", + ] + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + first = runner.invoke(app, args) + second = runner.invoke(app, args) + assert first.exit_code == 0, first.output + assert second.exit_code == 0, second.output + assert "nothing to do" in second.output + + def test_catalog_add_duplicate_different_settings_conflicts(self, project_dir): + """Re-adding a same-named preset catalog with different settings errors (#4505).""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + first = runner.invoke(app, [ + "preset", "catalog", "add", + "https://example.com/c.json", "--name", "mine", "--priority", "10", + ]) + second = runner.invoke(app, [ + "preset", "catalog", "add", + "https://example.com/c.json", "--name", "mine", "--priority", "20", + ]) + assert first.exit_code == 0, first.output + assert second.exit_code == 1 + assert "different settings" in second.output + def test_catalog_remove_escapes_rich_markup(self, project_dir): """`preset catalog remove` must not parse the name as Rich markup.""" from typer.testing import CliRunner diff --git a/tests/test_workflows.py b/tests/test_workflows.py index ac9f47b405..c134afcc15 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -8786,14 +8786,24 @@ def test_add_catalog_with_existing_inf_priority(self, project_dir): new = next(c for c in data["catalogs"] if c["url"] == "https://b.example.com/c.json") assert new["priority"] == 1 # max(inf coerced to 0) + 1 - def test_add_catalog_duplicate_rejected(self, project_dir): + def test_add_catalog_duplicate_is_idempotent(self, project_dir): from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError catalog = WorkflowCatalog(project_dir) - catalog.add_catalog("https://example.com/catalog.json") + assert catalog.add_catalog("https://example.com/catalog.json") == "added" + assert catalog.add_catalog("https://example.com/catalog.json") == "unchanged" - with pytest.raises(WorkflowValidationError, match="already configured"): - catalog.add_catalog("https://example.com/catalog.json") + cfg = project_dir / ".specify" / "workflow-catalogs.yml" + data = yaml.safe_load(cfg.read_text(encoding="utf-8")) + assert len(data["catalogs"]) == 1 + + def test_add_catalog_duplicate_different_name_conflicts(self, project_dir): + from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError + + catalog = WorkflowCatalog(project_dir) + catalog.add_catalog("https://example.com/catalog.json", "first") + with pytest.raises(WorkflowValidationError, match="different name"): + catalog.add_catalog("https://example.com/catalog.json", "second") def test_remove_catalog(self, project_dir): from specify_cli.workflows.catalog import WorkflowCatalog @@ -9528,14 +9538,24 @@ def test_add_catalog_rejects_falsy_non_mapping_config( assert config_path.read_text(encoding="utf-8") == original - def test_add_catalog_duplicate_rejected(self, project_dir): + def test_add_catalog_duplicate_is_idempotent(self, project_dir): from specify_cli.workflows.catalog import StepCatalog, StepValidationError catalog = StepCatalog(project_dir) - catalog.add_catalog("https://example.com/steps.json") + assert catalog.add_catalog("https://example.com/steps.json") == "added" + assert catalog.add_catalog("https://example.com/steps.json") == "unchanged" - with pytest.raises(StepValidationError, match="already configured"): - catalog.add_catalog("https://example.com/steps.json") + cfg = project_dir / ".specify" / "step-catalogs.yml" + data = yaml.safe_load(cfg.read_text(encoding="utf-8")) + assert len(data["catalogs"]) == 1 + + def test_add_catalog_duplicate_different_name_conflicts(self, project_dir): + from specify_cli.workflows.catalog import StepCatalog, StepValidationError + + catalog = StepCatalog(project_dir) + catalog.add_catalog("https://example.com/steps.json", "first") + with pytest.raises(StepValidationError, match="different name"): + catalog.add_catalog("https://example.com/steps.json", "second") def test_remove_catalog(self, project_dir): from specify_cli.workflows.catalog import StepCatalog diff --git a/tests/unit/test_bundler_catalog_config.py b/tests/unit/test_bundler_catalog_config.py index 46c333700a..26bdcbc95c 100644 --- a/tests/unit/test_bundler_catalog_config.py +++ b/tests/unit/test_bundler_catalog_config.py @@ -63,8 +63,9 @@ def test_add_source_persists_absolute_local_path(tmp_path: Path, monkeypatch): catalog.write_text("{}", encoding="utf-8") monkeypatch.chdir(project) - source = cc.add_source(project, "sub/cat.json", policy="install-allowed", priority=50) + source, status = cc.add_source(project, "sub/cat.json", policy="install-allowed", priority=50) + assert status == "added" assert Path(source.url).is_absolute() assert Path(source.url) == catalog.resolve() @@ -234,7 +235,7 @@ def test_add_source_allows_local_path_with_colon(tmp_path: Path, monkeypatch): (project / ".specify").mkdir(parents=True) monkeypatch.chdir(project) # A relative path containing ':' but no '://' is still a local path. - source = cc.add_source(project, "weird:name.json", policy="install-allowed", priority=50) + source, _ = cc.add_source(project, "weird:name.json", policy="install-allowed", priority=50) assert source.url.endswith("weird:name.json") or "weird" in source.url @@ -248,7 +249,7 @@ def test_add_source_rejects_plain_http_for_non_localhost(tmp_path: Path): def test_add_source_allows_http_for_localhost(tmp_path: Path): project = tmp_path / "proj" (project / ".specify").mkdir(parents=True) - source = cc.add_source(project, "http://localhost:8080/c.json", policy="install-allowed", priority=50) + source, _ = cc.add_source(project, "http://localhost:8080/c.json", policy="install-allowed", priority=50) assert source.url == "http://localhost:8080/c.json" From 27dbf5fb8a8ea57728618af02403fe588f64d99c Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 15 Sep 2026 08:07:23 -0500 Subject: [PATCH 02/11] Address PR #4543 review: normalize catalog settings and harden bundle add Resolve Copilot review findings on the idempotent catalog-add work (#4505): - Bundle: parse a matching existing entry through CatalogSource.from_dict before comparing, so a hand-edited non-integer priority surfaces as a clean BundlerError instead of leaking int()'s ValueError/OverflowError past the CLI's `except BundlerError`; string priorities now compare equal too. - Extensions/presets: normalize a stored entry's priority (numeric strings) and install_allowed (string booleans) with the same rules as the catalog reader, so a valid equivalent rerun is a no-op instead of a false conflict. - Tests: add same-explicit-name no-op coverage for integration and workflow (and step) catalogs, string-representation idempotency for extensions and presets, and bundle coverage for the malformed/string stored priority paths. - Remove unused imports flagged by ruff. Assisted-by: GitHub Copilot (model: claude-opus-4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../bundler/commands_impl/catalog_config.py | 19 ++++++-- src/specify_cli/extensions/_commands.py | 36 ++++++++++++++- src/specify_cli/presets/_commands.py | 37 ++++++++++++++- .../integrations/test_integration_catalog.py | 10 ++++ tests/test_extensions.py | 34 ++++++++++++++ tests/test_presets.py | 31 +++++++++++++ tests/test_workflows.py | 26 ++++++++++- tests/unit/test_bundler_catalog_config.py | 46 +++++++++++++++++++ 8 files changed, 228 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index 7bc669d093..7e45d4925b 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -197,13 +197,22 @@ def add_source( # Idempotent add (#4505): identity is the source id or url. A rerun # requesting the same settings is a successful no-op; differing # settings are a conflict rather than a silent overwrite. + # + # Parse the matching entry through CatalogSource.from_dict first: + # _read() only checks that entries are mappings, so a hand-edited + # entry may carry a non-integer priority. Normalizing here surfaces + # that as a clean BundlerError (matching catalog parsing) instead of + # leaking int()'s ValueError/OverflowError past the CLI's + # `except BundlerError`, and lets supported representations (e.g. a + # string priority) compare equal to the requested defaults. + existing_source = CatalogSource.from_dict(dict(existing), Scope.PROJECT) if ( - existing.get("id") == resolved_id - and existing.get("url") == url - and int(existing.get("priority", 0)) == desired["priority"] - and str(existing.get("install_policy", "")) == desired["install_policy"] + existing_source.id == resolved_id + and existing_source.url == url + and existing_source.priority == desired["priority"] + and existing_source.install_policy.value == desired["install_policy"] ): - return CatalogSource.from_dict(dict(existing), Scope.PROJECT), "unchanged" + return existing_source, "unchanged" raise BundlerError( f"Catalog source '{resolved_id}' (or url) already exists in this " "project with different settings. Remove it first to change it." diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 6550e0e177..a48c321530 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -327,6 +327,36 @@ def install_extension_from_url( pass +def _normalize_catalog_priority(value: object) -> object: + """Normalize a stored catalog priority the way the catalog reader does. + + The reader (``specify_cli/catalogs.py``) accepts integer-string priorities + like ``"10"`` but rejects bools. Mirror that here so an equivalent rerun + whose persisted priority is a supported string representation is still a + no-op rather than a false conflict (#4505). A value that cannot be + normalized is returned unchanged so it simply fails to compare equal. + """ + if isinstance(value, bool): + return value + try: + return int(value) + except (TypeError, ValueError, OverflowError): + return value + + +def _normalize_catalog_install_allowed(value: object) -> bool: + """Normalize a stored ``install_allowed`` the way the catalog reader does. + + The reader treats the strings ``"true"``/``"yes"``/``"1"`` (case- and + whitespace-insensitive) as truthy; everything else falls back to ``bool``. + Comparing raw values instead would report ``install_allowed: "false"`` as a + conflict because ``bool("false")`` is ``True``. + """ + if isinstance(value, str): + return value.strip().lower() in ("true", "yes", "1") + return bool(value) + + def _load_catalog_command_config(project_root: Path, config_path: Path) -> dict: """Load extension catalog CLI config with user-facing shape errors.""" try: @@ -649,8 +679,10 @@ def catalog_add( if isinstance(existing, dict) and existing.get("name") == name: if ( str(existing.get("url", "")) == url - and existing.get("priority") == priority - and bool(existing.get("install_allowed", False)) == install_allowed + and _normalize_catalog_priority(existing.get("priority")) == priority + and _normalize_catalog_install_allowed( + existing.get("install_allowed", False) + ) == install_allowed and str(existing.get("description", "")) == description ): console.print( diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 863415b91c..018b27384b 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -47,6 +47,37 @@ preset_app.add_typer(preset_catalog_app, name="catalog") +def _normalize_catalog_priority(value: object) -> object: + """Normalize a stored catalog priority the way the preset reader does. + + The preset reader (``specify_cli/presets/__init__.py``) accepts + integer-string priorities like ``"10"`` but rejects bools. Mirror that here + so an equivalent rerun whose persisted priority is a supported string + representation is still a no-op rather than a false conflict (#4505). A + value that cannot be normalized is returned unchanged so it simply fails to + compare equal. + """ + if isinstance(value, bool): + return value + try: + return int(value) + except (TypeError, ValueError, OverflowError): + return value + + +def _normalize_catalog_install_allowed(value: object) -> bool: + """Normalize a stored ``install_allowed`` the way the preset reader does. + + The reader treats the strings ``"true"``/``"yes"``/``"1"`` (case- and + whitespace-insensitive) as truthy; everything else falls back to ``bool``. + Comparing raw values instead would report ``install_allowed: "false"`` as a + conflict because ``bool("false")`` is ``True``. + """ + if isinstance(value, str): + return value.strip().lower() in ("true", "yes", "1") + return bool(value) + + def _warn_unmet_extension_dependencies(manager, manifest) -> None: """Warn when a preset's declared extension dependencies are unsatisfied. @@ -937,8 +968,10 @@ def preset_catalog_add( if isinstance(existing, dict) and existing.get("name") == name: if ( str(existing.get("url", "")) == url - and existing.get("priority") == priority - and bool(existing.get("install_allowed", False)) == install_allowed + and _normalize_catalog_priority(existing.get("priority")) == priority + and _normalize_catalog_install_allowed( + existing.get("install_allowed", False) + ) == install_allowed and str(existing.get("description", "")) == description ): console.print( diff --git a/tests/integrations/test_integration_catalog.py b/tests/integrations/test_integration_catalog.py index f413640c37..b8f2714c85 100644 --- a/tests/integrations/test_integration_catalog.py +++ b/tests/integrations/test_integration_catalog.py @@ -1186,6 +1186,16 @@ def test_add_catalog_duplicate_url_different_name_conflicts(self, tmp_path, monk with pytest.raises(IntegrationValidationError, match="different name"): cat.add_catalog("https://dup.example.com/catalog.json", name="second") + def test_add_catalog_duplicate_url_same_name_is_idempotent_noop(self, tmp_path, monkeypatch): + """Re-adding the same URL with the *same* explicit name is a no-op (#4505).""" + self._isolate(tmp_path, monkeypatch) + cat = IntegrationCatalog(tmp_path) + assert cat.add_catalog("https://dup.example.com/catalog.json", name="mine") == "added" + assert cat.add_catalog("https://dup.example.com/catalog.json", name="mine") == "unchanged" + cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" + data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert len(data["catalogs"]) == 1 + def test_add_catalog_rejects_invalid_url(self, tmp_path, monkeypatch): self._isolate(tmp_path, monkeypatch) cat = IntegrationCatalog(tmp_path) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 039a767e28..e3bdfde0c0 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7648,6 +7648,40 @@ def test_catalog_add_duplicate_different_settings_conflicts(self, tmp_path): assert second.exit_code == 1 assert "different settings" in second.output + def test_catalog_add_string_representations_are_idempotent(self, tmp_path): + """A stored entry using supported string representations (a numeric-string + priority and a string boolean) is equivalent to the requested defaults, so + a rerun is a no-op rather than a false conflict (#4505).""" + import yaml as _yaml + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + # Hand-written config: priority as a string, install_allowed as "false". + (project_dir / ".specify" / "extension-catalogs.yml").write_text( + _yaml.safe_dump({"catalogs": [{ + "name": "community", + "url": "https://example.com/catalog.json", + "priority": "10", + "install_allowed": "false", + "description": "", + }]}), + encoding="utf-8", + ) + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, [ + "extension", "catalog", "add", + "https://example.com/catalog.json", "--name", "community", + ], catch_exceptions=True) + + assert result.exit_code == 0, result.output + assert "nothing to do" in result.output + def test_catalog_add_escapes_config_saved_path_markup(self, tmp_path): """Catalog add's saved-path label should render literally under Rich.""" from typer.testing import CliRunner diff --git a/tests/test_presets.py b/tests/test_presets.py index fee6f5001a..138b6290d9 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3953,6 +3953,37 @@ def test_catalog_add_duplicate_different_settings_conflicts(self, project_dir): assert second.exit_code == 1 assert "different settings" in second.output + def test_catalog_add_string_representations_are_idempotent(self, project_dir): + """A stored preset catalog using supported string representations (a + numeric-string priority and a string boolean) is equivalent to the + requested defaults, so a rerun is a no-op rather than a false conflict + (#4505).""" + import yaml as _yaml + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + (project_dir / ".specify" / "preset-catalogs.yml").write_text( + _yaml.safe_dump({"catalogs": [{ + "name": "mine", + "url": "https://example.com/c.json", + "priority": "10", + "install_allowed": "false", + "description": "", + }]}), + encoding="utf-8", + ) + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, [ + "preset", "catalog", "add", + "https://example.com/c.json", "--name", "mine", + ]) + + assert result.exit_code == 0, result.output + assert "nothing to do" in result.output + def test_catalog_remove_escapes_rich_markup(self, project_dir): """`preset catalog remove` must not parse the name as Rich markup.""" from typer.testing import CliRunner diff --git a/tests/test_workflows.py b/tests/test_workflows.py index c134afcc15..a0bd9091a4 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -8787,7 +8787,7 @@ def test_add_catalog_with_existing_inf_priority(self, project_dir): assert new["priority"] == 1 # max(inf coerced to 0) + 1 def test_add_catalog_duplicate_is_idempotent(self, project_dir): - from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError + from specify_cli.workflows.catalog import WorkflowCatalog catalog = WorkflowCatalog(project_dir) assert catalog.add_catalog("https://example.com/catalog.json") == "added" @@ -8797,6 +8797,17 @@ def test_add_catalog_duplicate_is_idempotent(self, project_dir): data = yaml.safe_load(cfg.read_text(encoding="utf-8")) assert len(data["catalogs"]) == 1 + def test_add_catalog_duplicate_same_name_is_idempotent(self, project_dir): + from specify_cli.workflows.catalog import WorkflowCatalog + + catalog = WorkflowCatalog(project_dir) + assert catalog.add_catalog("https://example.com/catalog.json", "mine") == "added" + assert catalog.add_catalog("https://example.com/catalog.json", "mine") == "unchanged" + + cfg = project_dir / ".specify" / "workflow-catalogs.yml" + data = yaml.safe_load(cfg.read_text(encoding="utf-8")) + assert len(data["catalogs"]) == 1 + def test_add_catalog_duplicate_different_name_conflicts(self, project_dir): from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError @@ -9539,7 +9550,7 @@ def test_add_catalog_rejects_falsy_non_mapping_config( assert config_path.read_text(encoding="utf-8") == original def test_add_catalog_duplicate_is_idempotent(self, project_dir): - from specify_cli.workflows.catalog import StepCatalog, StepValidationError + from specify_cli.workflows.catalog import StepCatalog catalog = StepCatalog(project_dir) assert catalog.add_catalog("https://example.com/steps.json") == "added" @@ -9549,6 +9560,17 @@ def test_add_catalog_duplicate_is_idempotent(self, project_dir): data = yaml.safe_load(cfg.read_text(encoding="utf-8")) assert len(data["catalogs"]) == 1 + def test_add_catalog_duplicate_same_name_is_idempotent(self, project_dir): + from specify_cli.workflows.catalog import StepCatalog + + catalog = StepCatalog(project_dir) + assert catalog.add_catalog("https://example.com/steps.json", "mine") == "added" + assert catalog.add_catalog("https://example.com/steps.json", "mine") == "unchanged" + + cfg = project_dir / ".specify" / "step-catalogs.yml" + data = yaml.safe_load(cfg.read_text(encoding="utf-8")) + assert len(data["catalogs"]) == 1 + def test_add_catalog_duplicate_different_name_conflicts(self, project_dir): from specify_cli.workflows.catalog import StepCatalog, StepValidationError diff --git a/tests/unit/test_bundler_catalog_config.py b/tests/unit/test_bundler_catalog_config.py index 26bdcbc95c..1851ebda8c 100644 --- a/tests/unit/test_bundler_catalog_config.py +++ b/tests/unit/test_bundler_catalog_config.py @@ -124,6 +124,52 @@ def test_add_source_refuses_symlinked_specify_escape(tmp_path: Path): cc.add_source(project, "https://example.com/c.json", policy="install-allowed", priority=50) +def test_add_source_rerun_surfaces_bad_stored_priority_as_bundlererror(tmp_path: Path): + """A hand-edited matching entry with a non-integer priority must surface a + clean BundlerError during an idempotent-add comparison rather than leaking + int()'s ValueError past the CLI's `except BundlerError` (#4505).""" + project = tmp_path / "proj" + (project / ".specify").mkdir(parents=True) + cc._config_path(project).write_text( + "schema_version: '1.0'\n" + "catalogs:\n" + " - id: mine\n" + " url: https://example.com/c.json\n" + " priority: not-a-number\n" + " install_policy: install-allowed\n", + encoding="utf-8", + ) + + with pytest.raises(BundlerError, match="non-integer priority"): + cc.add_source( + project, "https://example.com/c.json", source_id="mine", + policy="install-allowed", priority=10, + ) + + +def test_add_source_rerun_with_string_priority_is_unchanged(tmp_path: Path): + """A stored priority written as a numeric string is normalized like catalog + parsing, so an otherwise-identical rerun is a no-op, not a false conflict.""" + project = tmp_path / "proj" + (project / ".specify").mkdir(parents=True) + cc._config_path(project).write_text( + "schema_version: '1.0'\n" + "catalogs:\n" + " - id: mine\n" + " url: https://example.com/c.json\n" + " priority: '10'\n" + " install_policy: install-allowed\n", + encoding="utf-8", + ) + + source, status = cc.add_source( + project, "https://example.com/c.json", source_id="mine", + policy="install-allowed", priority=10, + ) + assert status == "unchanged" + assert source.priority == 10 + + def test_read_rejects_non_list_catalogs(tmp_path: Path): project = tmp_path / "proj" (project / ".specify").mkdir(parents=True) From 7da46584848ea8019f86f1d616c8291a6ad22171 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 16 Sep 2026 15:44:52 -0500 Subject: [PATCH 03/11] fix: normalize catalog identities for idempotent adds Align extension names and catalog URLs with reader normalization. Cover workflow and step CLI no-op and conflict outcomes, preserve existing configuration, and document step catalogs. Assisted-by: GitHub Copilot (model: gpt-6-astra, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/reference/extensions.md | 2 + docs/reference/presets.md | 2 + docs/reference/workflows.md | 16 +++++ src/specify_cli/extensions/_commands.py | 6 +- src/specify_cli/presets/_commands.py | 5 +- src/specify_cli/workflows/catalog.py | 18 ++--- tests/test_extensions.py | 53 ++++++++++++++- tests/test_presets.py | 45 ++++++++++++- tests/test_workflows.py | 87 +++++++++++++++++++++++++ 9 files changed, 216 insertions(+), 18 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index a3712aea04..7fbb7fa382 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -158,6 +158,8 @@ Adds a catalog to the project's `.specify/extension-catalogs.yml`. Adding a catalog is idempotent (identity is the catalog **name**): re-running `catalog add` with the same name and identical settings is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same name with *different* settings is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. +Surrounding whitespace in catalog names and URLs is ignored when comparing entries and stripped from newly added entries. A no-op leaves the existing configuration unchanged. + ### Remove a Catalog ```bash diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 64824daf2f..d5547f1b58 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -125,6 +125,8 @@ Adds a catalog to the project's `.specify/preset-catalogs.yml`. Adding a catalog is idempotent (identity is the catalog **name**): re-running `catalog add` with the same name and identical settings is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same name with *different* settings is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. +Surrounding whitespace in catalog URLs is ignored when comparing entries and stripped from newly added entries. A no-op leaves the existing configuration unchanged. + ### Remove a Catalog ```bash diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index abef928228..2961cb7ca9 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -407,6 +407,8 @@ Adds a custom catalog URL to the project's `.specify/workflow-catalogs.yml`. Adding a catalog is idempotent (identity is the catalog **URL**): re-running `catalog add` with the same URL and the same (or no) `--name` is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same URL with a *different* `--name` is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. +Surrounding whitespace in catalog URLs is ignored when comparing entries and stripped from newly added entries. A no-op leaves the existing configuration unchanged. + ### Remove a Catalog ```bash @@ -424,6 +426,20 @@ Catalogs are resolved in this order (first match wins): 3. **User config** — `~/.specify/workflow-catalogs.yml` 4. **Built-in defaults** — official catalog + community catalog +### Step Catalogs + +Custom step types have a separate catalog stack: + +```bash +specify workflow step catalog list +specify workflow step catalog add [--name ] +specify workflow step catalog remove +``` + +`step catalog add` writes to `.specify/step-catalogs.yml`. Like workflow catalogs, step catalogs use the **URL** as their identity, ignoring surrounding whitespace. Adding the same URL with the same (or no) `--name` is a successful no-op (exit code 0) that leaves the configuration unchanged. A different `--name` for that URL is a conflict (exit code 1); remove the existing entry first to change it. New entries store the URL without surrounding whitespace. + +`step catalog list` shows the active sources, and `step catalog remove` removes a project entry by its index. Step catalog resolution uses `SPECKIT_STEP_CATALOG_URL`, then the project config, then `~/.specify/step-catalogs.yml`, then built-in defaults. + ## Workflow Definition Workflows are defined in YAML files. Here is the built-in **Full SDD Cycle** workflow that ships with Spec Kit: diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index a48c321530..d57230f6d8 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -645,6 +645,8 @@ def catalog_add( project_root = _require_specify_project() specify_dir = project_root / ".specify" + url = url.strip() + name = name.strip() # Validate URL tmp_catalog = ExtensionCatalog(project_root) @@ -676,9 +678,9 @@ def catalog_add( # still a conflict — we refuse to silently change priority/install # permissions and ask the user to remove it first. for existing in catalogs: - if isinstance(existing, dict) and existing.get("name") == name: + if isinstance(existing, dict) and str(existing.get("name", "")).strip() == name: if ( - str(existing.get("url", "")) == url + str(existing.get("url", "")).strip() == url and _normalize_catalog_priority(existing.get("priority")) == priority and _normalize_catalog_install_allowed( existing.get("install_allowed", False) diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 018b27384b..f5909c2ddc 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -922,6 +922,7 @@ def preset_catalog_add( project_root = _require_specify_project() specify_dir = project_root / ".specify" + url = url.strip() # Validate URL tmp_catalog = PresetCatalog(project_root) @@ -954,7 +955,7 @@ def preset_catalog_add( console.print("[red]Error:[/red] Invalid catalog config: 'catalogs' must be a list.") raise typer.Exit(1) - # Only rendering is escaped — the raw values are what get persisted and + # Only rendering is escaped — the unescaped values get persisted and # compared below, so a name containing markup still round-trips exactly. safe_name = _escape_markup(str(name)) safe_url = _escape_markup(str(url)) @@ -967,7 +968,7 @@ def preset_catalog_add( for existing in catalogs: if isinstance(existing, dict) and existing.get("name") == name: if ( - str(existing.get("url", "")) == url + str(existing.get("url", "")).strip() == url and _normalize_catalog_priority(existing.get("priority")) == priority and _normalize_catalog_install_allowed( existing.get("install_allowed", False) diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 72b3431e0d..4451f3cebc 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -708,12 +708,13 @@ def get_catalog_configs(self) -> list[dict[str, Any]]: def add_catalog(self, url: str, name: str | None = None) -> str: """Add a catalog source to the project-level config. - Identity is the URL: adding a URL that is already configured is - idempotent (#4505). A rerun requesting the same name (or no explicit - name) is a no-op that returns ``"unchanged"``; a rerun requesting a + Identity is the URL with surrounding whitespace stripped. Adding an + existing URL is idempotent (#4505): requesting the same name (or no + explicit name) returns ``"unchanged"``; a rerun requesting a different name is rejected as a conflict. Returns ``"added"`` when a new entry is written. """ + url = url.strip() self._validate_catalog_url(url) config_path = self.project_root / ".specify" / "workflow-catalogs.yml" @@ -742,7 +743,7 @@ def add_catalog(self, url: str, name: str | None = None) -> str: # same name (or no explicit name) is a no-op; a different name conflicts. requested_name = str(name).strip() if name is not None else "" for cat in catalogs: - if isinstance(cat, dict) and cat.get("url") == url: + if isinstance(cat, dict) and str(cat.get("url", "")).strip() == url: existing_name = str(cat.get("name", "")).strip() if not requested_name or requested_name == existing_name: return "unchanged" @@ -1406,12 +1407,13 @@ def get_catalog_configs(self) -> list[dict[str, Any]]: def add_catalog(self, url: str, name: str | None = None) -> str: """Add a catalog source to the project-level config. - Identity is the URL: adding a URL that is already configured is - idempotent (#4505). A rerun requesting the same name (or no explicit - name) is a no-op that returns ``"unchanged"``; a rerun requesting a + Identity is the URL with surrounding whitespace stripped. Adding an + existing URL is idempotent (#4505): requesting the same name (or no + explicit name) returns ``"unchanged"``; a rerun requesting a different name is rejected as a conflict. Returns ``"added"`` when a new entry is written. """ + url = url.strip() self._validate_catalog_url(url) config_path = self.project_root / ".specify" / "step-catalogs.yml" @@ -1440,7 +1442,7 @@ def add_catalog(self, url: str, name: str | None = None) -> str: # same name (or no explicit name) is a no-op; a different name conflicts. requested_name = str(name).strip() if name is not None else "" for cat in catalogs: - if isinstance(cat, dict) and cat.get("url") == url: + if isinstance(cat, dict) and str(cat.get("url", "")).strip() == url: existing_name = str(cat.get("name", "")).strip() if not requested_name or requested_name == existing_name: return "unchanged" diff --git a/tests/test_extensions.py b/tests/test_extensions.py index e3bdfde0c0..226e47d588 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7598,7 +7598,9 @@ def test_catalog_add_escapes_url_markup(self, tmp_path): assert result.exit_code == 0, result.output assert f"URL: {url}" in result.output - def test_catalog_add_duplicate_is_idempotent(self, tmp_path): + @pytest.mark.parametrize("name", ["community", " community "]) + @pytest.mark.parametrize("padding", ["", " \t"]) + def test_catalog_add_duplicate_is_idempotent(self, tmp_path, name, padding): """Re-adding an identical catalog is a successful no-op (#4505).""" from typer.testing import CliRunner from unittest.mock import patch @@ -7614,12 +7616,57 @@ def test_catalog_add_duplicate_is_idempotent(self, tmp_path): ] runner = CliRunner() with patch.object(Path, "cwd", return_value=project_dir): - first = runner.invoke(app, args, catch_exceptions=True) + first = runner.invoke(app, [ + "extension", "catalog", "add", + f"{padding}https://example.com/catalog.json{padding}", "--name", name, + ], catch_exceptions=True) + assert first.exit_code == 0, first.output + config_path = project_dir / ".specify" / "extension-catalogs.yml" + original = config_path.read_bytes() second = runner.invoke(app, args, catch_exceptions=True) - assert first.exit_code == 0, first.output assert second.exit_code == 0, second.output assert "nothing to do" in second.output + assert config_path.read_bytes() == original + entries = yaml.safe_load(original)["catalogs"] + assert len(entries) == 1 + assert entries[0]["name"] == "community" + assert entries[0]["url"] == "https://example.com/catalog.json" + + @pytest.mark.parametrize("stored_name,name", [ + (" community ", "community"), + ("community", " community "), + (" community ", "\tcommunity\t"), + ]) + @pytest.mark.parametrize("stored_padding,padding", [(" \t", ""), ("", " \t"), (" ", "\t")]) + @pytest.mark.parametrize("priority", [10, 20]) + def test_catalog_add_normalizes_existing_identity( + self, project_dir, monkeypatch, stored_name, name, stored_padding, padding, priority + ): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + config_path = project_dir / ".specify" / "extension-catalogs.yml" + config_path.write_text(yaml.safe_dump({"catalogs": [{ + "name": stored_name, + "url": f"{stored_padding}https://example.com/catalog.json{stored_padding}", + "priority": 10, + "install_allowed": False, + }]}), encoding="utf-8") + original = config_path.read_bytes() + modified_at = config_path.stat().st_mtime_ns + + result = CliRunner().invoke(app, [ + "extension", "catalog", "add", + f"{padding}https://example.com/catalog.json{padding}", + "--name", name, "--priority", str(priority), + ], catch_exceptions=False) + + assert result.exit_code == (0 if priority == 10 else 1), result.output + assert ("nothing to do" if priority == 10 else "different settings") in result.output + assert config_path.read_bytes() == original + assert config_path.stat().st_mtime_ns == modified_at def test_catalog_add_duplicate_different_settings_conflicts(self, tmp_path): """Re-adding a same-named catalog with different settings errors (#4505).""" diff --git a/tests/test_presets.py b/tests/test_presets.py index 138b6290d9..efa3be2812 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3915,7 +3915,8 @@ def test_catalog_add_escapes_rich_markup(self, project_dir): assert config["catalogs"][0]["name"] == name assert config["catalogs"][0]["url"] == url - def test_catalog_add_duplicate_is_idempotent(self, project_dir): + @pytest.mark.parametrize("padding", ["", " \t"]) + def test_catalog_add_duplicate_is_idempotent(self, project_dir, padding): """Re-adding an identical preset catalog is a successful no-op (#4505).""" from typer.testing import CliRunner from unittest.mock import patch @@ -3927,11 +3928,49 @@ def test_catalog_add_duplicate_is_idempotent(self, project_dir): ] runner = CliRunner() with patch.object(Path, "cwd", return_value=project_dir): - first = runner.invoke(app, args) + first = runner.invoke(app, [ + "preset", "catalog", "add", + f"{padding}https://example.com/c.json{padding}", "--name", "mine", + ]) + assert first.exit_code == 0, first.output + config_path = project_dir / ".specify" / "preset-catalogs.yml" + original = config_path.read_bytes() second = runner.invoke(app, args) - assert first.exit_code == 0, first.output assert second.exit_code == 0, second.output assert "nothing to do" in second.output + assert config_path.read_bytes() == original + entries = yaml.safe_load(original)["catalogs"] + assert len(entries) == 1 + assert entries[0]["url"] == "https://example.com/c.json" + + @pytest.mark.parametrize("stored_padding,padding", [(" \t", ""), ("", " \t"), (" ", "\t")]) + @pytest.mark.parametrize("priority", [10, 20]) + def test_catalog_add_normalizes_existing_url( + self, project_dir, monkeypatch, stored_padding, padding, priority + ): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + config_path = project_dir / ".specify" / "preset-catalogs.yml" + config_path.write_text(yaml.safe_dump({"catalogs": [{ + "name": "mine", + "url": f"{stored_padding}https://example.com/c.json{stored_padding}", + "priority": 10, + "install_allowed": False, + }]}), encoding="utf-8") + original = config_path.read_bytes() + modified_at = config_path.stat().st_mtime_ns + + result = CliRunner().invoke(app, [ + "preset", "catalog", "add", f"{padding}https://example.com/c.json{padding}", + "--name", "mine", "--priority", str(priority), + ], catch_exceptions=False) + + assert result.exit_code == (0 if priority == 10 else 1), result.output + assert ("nothing to do" if priority == 10 else "different settings") in result.output + assert config_path.read_bytes() == original + assert config_path.stat().st_mtime_ns == modified_at def test_catalog_add_duplicate_different_settings_conflicts(self, project_dir): """Re-adding a same-named preset catalog with different settings errors (#4505).""" diff --git a/tests/test_workflows.py b/tests/test_workflows.py index a0bd9091a4..32e4d4a650 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -9981,6 +9981,93 @@ def make_module_name(type_key: str) -> str: assert name_a != name_b, "Module names for 'a-b' and 'a_b' must differ" +@pytest.mark.parametrize("command,config_filename", [ + (["workflow", "catalog", "add"], "workflow-catalogs.yml"), + (["workflow", "step", "catalog", "add"], "step-catalogs.yml"), +]) +class TestWorkflowCatalogAddCLI: + @pytest.mark.parametrize("name", [None, "mine"]) + @pytest.mark.parametrize("padding", ["", " \t"]) + def test_add_catalog_duplicate_outcomes( + self, project_dir, monkeypatch, command, config_filename, name, padding + ): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + url = "https://example.com/catalog.json" + name_args = ["--name", name] if name is not None else [] + runner = CliRunner() + first = runner.invoke( + app, [*command, f"{padding}{url}{padding}", *name_args], catch_exceptions=False + ) + assert first.exit_code == 0, first.output + assert "source added" in first.output + config_path = project_dir / ".specify" / config_filename + original = config_path.read_bytes() + modified_at = config_path.stat().st_mtime_ns + + second = runner.invoke(app, [*command, url, *name_args], catch_exceptions=False) + + assert second.exit_code == 0, second.output + assert "already configured" in second.output + assert "source added" not in second.output + assert config_path.read_bytes() == original + assert config_path.stat().st_mtime_ns == modified_at + entries = yaml.safe_load(original)["catalogs"] + assert len(entries) == 1 + assert entries[0]["url"] == url + assert entries[0]["name"] == (name or "catalog-1") + + conflict = runner.invoke( + app, [*command, url, "--name", "different"], catch_exceptions=False + ) + assert conflict.exit_code == 1, conflict.output + assert "different name" in conflict.output + assert "source added" not in conflict.output + assert config_path.read_bytes() == original + assert config_path.stat().st_mtime_ns == modified_at + + @pytest.mark.parametrize("stored_padding,padding", [ + ("", ""), (" \t", ""), ("", " \t"), (" ", "\t"), + ]) + @pytest.mark.parametrize("name", [None, "mine", "different"]) + def test_add_catalog_existing_url_outcomes( + self, project_dir, monkeypatch, command, config_filename, stored_padding, padding, name + ): + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + url = "https://example.com/catalog.json" + config_path = project_dir / ".specify" / config_filename + config_path.write_text(yaml.safe_dump({"catalogs": [{ + "name": "mine", + "url": f"{stored_padding}{url}{stored_padding}", + "priority": 7, + "install_allowed": False, + "description": "Keep this entry unchanged.", + }]}), encoding="utf-8") + original = config_path.read_bytes() + modified_at = config_path.stat().st_mtime_ns + name_args = ["--name", name] if name is not None else [] + + result = CliRunner().invoke( + app, [*command, f"{padding}{url}{padding}", *name_args], catch_exceptions=False + ) + + if name == "different": + assert result.exit_code == 1, result.output + assert "different name" in result.output + assert "already configured:" not in result.output + else: + assert result.exit_code == 0, result.output + assert "already configured" in result.output + assert "source added" not in result.output + assert config_path.read_bytes() == original + assert config_path.stat().st_mtime_ns == modified_at + + # ===== CLI Step Remove Tests ===== class TestWorkflowStepRemoveCLI: From f17ed43753c8ce6cb73777cdce332f095f2417f9 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 17 Sep 2026 07:28:15 -0500 Subject: [PATCH 04/11] fix: reject invalid catalog equivalence Reject boolean priorities during extension and preset equivalence checks and normalize stored bundle identities before duplicate matching. Add regression coverage and document these rules. Scope CLI test working-directory changes so Windows can clean up temporary projects before fixture teardown. Assisted-by: GitHub Copilot (model: gpt-6-astra, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/reference/bundles.md | 2 + docs/reference/extensions.md | 2 + docs/reference/presets.md | 2 + .../bundler/commands_impl/catalog_config.py | 5 +- src/specify_cli/extensions/_commands.py | 8 +-- src/specify_cli/presets/_commands.py | 10 ++-- tests/contract/test_bundle_cli.py | 26 +++++++++ tests/test_extensions.py | 43 ++++++++++++-- tests/test_presets.py | 41 +++++++++++-- tests/test_workflows.py | 30 ++++++---- tests/unit/test_bundler_catalog_config.py | 57 ++++++++++++++++++- 11 files changed, 190 insertions(+), 36 deletions(-) diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index a047678e6b..f1a727f577 100644 --- a/docs/reference/bundles.md +++ b/docs/reference/bundles.md @@ -177,6 +177,8 @@ Registers a project-scoped catalog source and persists it. Adding a source is idempotent (identity is the source **id or url**): re-running `catalog add` with the same id/url and identical `--policy`/`--priority` is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding a matching id/url with *different* settings is rejected as a conflict rather than silently overwriting the existing source — remove it first to change it. +Surrounding whitespace in source ids and URLs is ignored when matching identities and comparing settings. No-ops and conflicts leave the existing configuration unchanged; they do not rewrite stored values to normalize them. + ### Remove a Catalog Source ```bash diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 7fbb7fa382..f5c024a99c 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -160,6 +160,8 @@ Adding a catalog is idempotent (identity is the catalog **name**): re-running `c Surrounding whitespace in catalog names and URLs is ignored when comparing entries and stripped from newly added entries. A no-op leaves the existing configuration unchanged. +Stored priorities may use numeric strings, but YAML booleans (`true`/`false`) are invalid and are never equivalent to integer priorities (`1`/`0`). Re-adding a matching catalog with an invalid stored priority reports a conflict. + ### Remove a Catalog ```bash diff --git a/docs/reference/presets.md b/docs/reference/presets.md index d5547f1b58..d96bc3ee85 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -127,6 +127,8 @@ Adding a catalog is idempotent (identity is the catalog **name**): re-running `c Surrounding whitespace in catalog URLs is ignored when comparing entries and stripped from newly added entries. A no-op leaves the existing configuration unchanged. +Stored priorities may use numeric strings, but YAML booleans (`true`/`false`) are invalid and are never equivalent to integer priorities (`1`/`0`). Re-adding a matching catalog with an invalid stored priority reports a conflict. + ### Remove a Catalog ```bash diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index 7e45d4925b..c8f17407ce 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -193,7 +193,10 @@ def add_source( "install_policy": install_policy.value, } for existing in catalogs: - if existing.get("id") == resolved_id or existing.get("url") == url: + if ( + str(existing.get("id", "")).strip() == resolved_id + or str(existing.get("url", "")).strip() == url + ): # Idempotent add (#4505): identity is the source id or url. A rerun # requesting the same settings is a successful no-op; differing # settings are a conflict rather than a silent overwrite. diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index d57230f6d8..28375faa93 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -327,21 +327,21 @@ def install_extension_from_url( pass -def _normalize_catalog_priority(value: object) -> object: +def _normalize_catalog_priority(value: object) -> int | None: """Normalize a stored catalog priority the way the catalog reader does. The reader (``specify_cli/catalogs.py``) accepts integer-string priorities like ``"10"`` but rejects bools. Mirror that here so an equivalent rerun whose persisted priority is a supported string representation is still a no-op rather than a false conflict (#4505). A value that cannot be - normalized is returned unchanged so it simply fails to compare equal. + normalized returns ``None`` so it cannot compare equal to an integer. """ if isinstance(value, bool): - return value + return None try: return int(value) except (TypeError, ValueError, OverflowError): - return value + return None def _normalize_catalog_install_allowed(value: object) -> bool: diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index f5909c2ddc..40c505cb59 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -47,22 +47,22 @@ preset_app.add_typer(preset_catalog_app, name="catalog") -def _normalize_catalog_priority(value: object) -> object: +def _normalize_catalog_priority(value: object) -> int | None: """Normalize a stored catalog priority the way the preset reader does. The preset reader (``specify_cli/presets/__init__.py``) accepts integer-string priorities like ``"10"`` but rejects bools. Mirror that here so an equivalent rerun whose persisted priority is a supported string representation is still a no-op rather than a false conflict (#4505). A - value that cannot be normalized is returned unchanged so it simply fails to - compare equal. + value that cannot be normalized returns ``None`` so it cannot compare equal + to an integer. """ if isinstance(value, bool): - return value + return None try: return int(value) except (TypeError, ValueError, OverflowError): - return value + return None def _normalize_catalog_install_allowed(value: object) -> bool: diff --git a/tests/contract/test_bundle_cli.py b/tests/contract/test_bundle_cli.py index 3029064e2a..6752c9161d 100644 --- a/tests/contract/test_bundle_cli.py +++ b/tests/contract/test_bundle_cli.py @@ -228,6 +228,32 @@ def test_catalog_add_duplicate_is_idempotent(project: Path): assert "already" in second.output +@pytest.mark.parametrize("priority,exit_code", [(10, 0), (20, 1)]) +def test_catalog_add_normalizes_stored_identity(project: Path, priority, exit_code): + config_path = project / ".specify" / "bundle-catalogs.yml" + config_path.write_text(yaml.safe_dump({ + "schema_version": "1.0", + "catalogs": [{ + "id": " \tlocal\t ", + "url": " \thttps://example.com/catalog.json\t ", + "priority": 10, + "install_policy": "install-allowed", + }], + }), encoding="utf-8") + original = config_path.read_bytes() + modified_at = config_path.stat().st_mtime_ns + + result = runner.invoke(app, [ + "bundle", "catalog", "add", "https://example.com/catalog.json", + "--id", "local", "--policy", "install-allowed", "--priority", str(priority), + ], catch_exceptions=False) + + assert result.exit_code == exit_code, result.output + assert ("already" if exit_code == 0 else "different settings") in result.output + assert config_path.read_bytes() == original + assert config_path.stat().st_mtime_ns == modified_at + + def test_catalog_add_duplicate_different_settings_conflicts(project: Path): catalog = project / "local-catalog.json" write_catalog_file(catalog, {"demo": catalog_entry_dict("demo")}) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 226e47d588..38be66a269 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7646,7 +7646,6 @@ def test_catalog_add_normalizes_existing_identity( from typer.testing import CliRunner from specify_cli import app - monkeypatch.chdir(project_dir) config_path = project_dir / ".specify" / "extension-catalogs.yml" config_path.write_text(yaml.safe_dump({"catalogs": [{ "name": stored_name, @@ -7657,11 +7656,13 @@ def test_catalog_add_normalizes_existing_identity( original = config_path.read_bytes() modified_at = config_path.stat().st_mtime_ns - result = CliRunner().invoke(app, [ - "extension", "catalog", "add", - f"{padding}https://example.com/catalog.json{padding}", - "--name", name, "--priority", str(priority), - ], catch_exceptions=False) + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke(app, [ + "extension", "catalog", "add", + f"{padding}https://example.com/catalog.json{padding}", + "--name", name, "--priority", str(priority), + ], catch_exceptions=False) assert result.exit_code == (0 if priority == 10 else 1), result.output assert ("nothing to do" if priority == 10 else "different settings") in result.output @@ -7729,6 +7730,36 @@ def test_catalog_add_string_representations_are_idempotent(self, tmp_path): assert result.exit_code == 0, result.output assert "nothing to do" in result.output + @pytest.mark.parametrize("stored_priority,priority", [ + (True, 1), (False, 0), (1, 1), (0, 0), ("1", 1), ("0", 0), + ]) + def test_catalog_add_priority_equivalence(self, project_dir, monkeypatch, stored_priority, priority): + from typer.testing import CliRunner + from specify_cli import app + + config_path = project_dir / ".specify" / "extension-catalogs.yml" + config_path.write_text(yaml.safe_dump({"catalogs": [{ + "name": "mine", + "url": "https://example.com/catalog.json", + "priority": stored_priority, + "install_allowed": False, + }]}), encoding="utf-8") + original = config_path.read_bytes() + modified_at = config_path.stat().st_mtime_ns + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke(app, [ + "extension", "catalog", "add", "https://example.com/catalog.json", + "--name", "mine", "--priority", str(priority), + ], catch_exceptions=False) + + invalid = isinstance(stored_priority, bool) + assert result.exit_code == (1 if invalid else 0), result.output + assert ("different settings" if invalid else "nothing to do") in result.output + assert config_path.read_bytes() == original + assert config_path.stat().st_mtime_ns == modified_at + def test_catalog_add_escapes_config_saved_path_markup(self, tmp_path): """Catalog add's saved-path label should render literally under Rich.""" from typer.testing import CliRunner diff --git a/tests/test_presets.py b/tests/test_presets.py index efa3be2812..abeedf9c83 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3951,7 +3951,6 @@ def test_catalog_add_normalizes_existing_url( from typer.testing import CliRunner from specify_cli import app - monkeypatch.chdir(project_dir) config_path = project_dir / ".specify" / "preset-catalogs.yml" config_path.write_text(yaml.safe_dump({"catalogs": [{ "name": "mine", @@ -3962,10 +3961,12 @@ def test_catalog_add_normalizes_existing_url( original = config_path.read_bytes() modified_at = config_path.stat().st_mtime_ns - result = CliRunner().invoke(app, [ - "preset", "catalog", "add", f"{padding}https://example.com/c.json{padding}", - "--name", "mine", "--priority", str(priority), - ], catch_exceptions=False) + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke(app, [ + "preset", "catalog", "add", f"{padding}https://example.com/c.json{padding}", + "--name", "mine", "--priority", str(priority), + ], catch_exceptions=False) assert result.exit_code == (0 if priority == 10 else 1), result.output assert ("nothing to do" if priority == 10 else "different settings") in result.output @@ -4023,6 +4024,36 @@ def test_catalog_add_string_representations_are_idempotent(self, project_dir): assert result.exit_code == 0, result.output assert "nothing to do" in result.output + @pytest.mark.parametrize("stored_priority,priority", [ + (True, 1), (False, 0), (1, 1), (0, 0), ("1", 1), ("0", 0), + ]) + def test_catalog_add_priority_equivalence(self, project_dir, monkeypatch, stored_priority, priority): + from typer.testing import CliRunner + from specify_cli import app + + config_path = project_dir / ".specify" / "preset-catalogs.yml" + config_path.write_text(yaml.safe_dump({"catalogs": [{ + "name": "mine", + "url": "https://example.com/catalog.json", + "priority": stored_priority, + "install_allowed": False, + }]}), encoding="utf-8") + original = config_path.read_bytes() + modified_at = config_path.stat().st_mtime_ns + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke(app, [ + "preset", "catalog", "add", "https://example.com/catalog.json", + "--name", "mine", "--priority", str(priority), + ], catch_exceptions=False) + + invalid = isinstance(stored_priority, bool) + assert result.exit_code == (1 if invalid else 0), result.output + assert ("different settings" if invalid else "nothing to do") in result.output + assert config_path.read_bytes() == original + assert config_path.stat().st_mtime_ns == modified_at + def test_catalog_remove_escapes_rich_markup(self, project_dir): """`preset catalog remove` must not parse the name as Rich markup.""" from typer.testing import CliRunner diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 32e4d4a650..ac87c7733a 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -9994,20 +9994,23 @@ def test_add_catalog_duplicate_outcomes( from typer.testing import CliRunner from specify_cli import app - monkeypatch.chdir(project_dir) url = "https://example.com/catalog.json" name_args = ["--name", name] if name is not None else [] runner = CliRunner() - first = runner.invoke( - app, [*command, f"{padding}{url}{padding}", *name_args], catch_exceptions=False - ) + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + first = runner.invoke( + app, [*command, f"{padding}{url}{padding}", *name_args], catch_exceptions=False + ) assert first.exit_code == 0, first.output assert "source added" in first.output config_path = project_dir / ".specify" / config_filename original = config_path.read_bytes() modified_at = config_path.stat().st_mtime_ns - second = runner.invoke(app, [*command, url, *name_args], catch_exceptions=False) + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + second = runner.invoke(app, [*command, url, *name_args], catch_exceptions=False) assert second.exit_code == 0, second.output assert "already configured" in second.output @@ -10019,9 +10022,11 @@ def test_add_catalog_duplicate_outcomes( assert entries[0]["url"] == url assert entries[0]["name"] == (name or "catalog-1") - conflict = runner.invoke( - app, [*command, url, "--name", "different"], catch_exceptions=False - ) + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + conflict = runner.invoke( + app, [*command, url, "--name", "different"], catch_exceptions=False + ) assert conflict.exit_code == 1, conflict.output assert "different name" in conflict.output assert "source added" not in conflict.output @@ -10038,7 +10043,6 @@ def test_add_catalog_existing_url_outcomes( from typer.testing import CliRunner from specify_cli import app - monkeypatch.chdir(project_dir) url = "https://example.com/catalog.json" config_path = project_dir / ".specify" / config_filename config_path.write_text(yaml.safe_dump({"catalogs": [{ @@ -10052,9 +10056,11 @@ def test_add_catalog_existing_url_outcomes( modified_at = config_path.stat().st_mtime_ns name_args = ["--name", name] if name is not None else [] - result = CliRunner().invoke( - app, [*command, f"{padding}{url}{padding}", *name_args], catch_exceptions=False - ) + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, [*command, f"{padding}{url}{padding}", *name_args], catch_exceptions=False + ) if name == "different": assert result.exit_code == 1, result.output diff --git a/tests/unit/test_bundler_catalog_config.py b/tests/unit/test_bundler_catalog_config.py index 1851ebda8c..b08d90df62 100644 --- a/tests/unit/test_bundler_catalog_config.py +++ b/tests/unit/test_bundler_catalog_config.py @@ -124,7 +124,8 @@ def test_add_source_refuses_symlinked_specify_escape(tmp_path: Path): cc.add_source(project, "https://example.com/c.json", policy="install-allowed", priority=50) -def test_add_source_rerun_surfaces_bad_stored_priority_as_bundlererror(tmp_path: Path): +@pytest.mark.parametrize("padding", ["", " \t"]) +def test_add_source_rerun_surfaces_bad_stored_priority_as_bundlererror(tmp_path: Path, padding): """A hand-edited matching entry with a non-integer priority must surface a clean BundlerError during an idempotent-add comparison rather than leaking int()'s ValueError past the CLI's `except BundlerError` (#4505).""" @@ -133,18 +134,23 @@ def test_add_source_rerun_surfaces_bad_stored_priority_as_bundlererror(tmp_path: cc._config_path(project).write_text( "schema_version: '1.0'\n" "catalogs:\n" - " - id: mine\n" - " url: https://example.com/c.json\n" + f" - id: '{padding}mine{padding}'\n" + f" url: '{padding}https://example.com/c.json{padding}'\n" " priority: not-a-number\n" " install_policy: install-allowed\n", encoding="utf-8", ) + config_path = cc._config_path(project) + original = config_path.read_bytes() + modified_at = config_path.stat().st_mtime_ns with pytest.raises(BundlerError, match="non-integer priority"): cc.add_source( project, "https://example.com/c.json", source_id="mine", policy="install-allowed", priority=10, ) + assert config_path.read_bytes() == original + assert config_path.stat().st_mtime_ns == modified_at def test_add_source_rerun_with_string_priority_is_unchanged(tmp_path: Path): @@ -170,6 +176,51 @@ def test_add_source_rerun_with_string_priority_is_unchanged(tmp_path: Path): assert source.priority == 10 +@pytest.mark.parametrize("id_padding", ["", " \t"]) +@pytest.mark.parametrize("url_padding", ["", " \t"]) +@pytest.mark.parametrize("source_id,url,outcome", [ + ("local", "https://example.com/c.json", "unchanged"), + ("local", "https://example.com/other.json", "conflict"), + ("other", "https://example.com/c.json", "conflict"), + ("other", "https://example.com/other.json", "added"), +]) +def test_add_source_normalizes_stored_identity( + tmp_path: Path, id_padding, url_padding, source_id, url, outcome +): + project = tmp_path / "proj" + (project / ".specify").mkdir(parents=True) + existing = { + "id": f"{id_padding}local{id_padding}", + "url": f"{url_padding}https://example.com/c.json{url_padding}", + "priority": 10, + "install_policy": "install-allowed", + } + cc._write(project, [existing]) + config_path = cc._config_path(project) + original = config_path.read_bytes() + modified_at = config_path.stat().st_mtime_ns + + if outcome == "conflict": + with pytest.raises(BundlerError, match="different settings"): + cc.add_source(project, url, source_id=source_id, policy="install-allowed", priority=10) + else: + source, status = cc.add_source( + project, url, source_id=source_id, policy="install-allowed", priority=10, + ) + assert status == outcome + assert source.id == source_id + assert source.url == url + assert source.priority == 10 + assert source.install_allowed + + entries = cc._read(project) + assert entries[0] == existing + assert len(entries) == (2 if outcome == "added" else 1) + if outcome != "added": + assert config_path.read_bytes() == original + assert config_path.stat().st_mtime_ns == modified_at + + def test_read_rejects_non_list_catalogs(tmp_path: Path): project = tmp_path / "proj" (project / ".specify").mkdir(parents=True) From 229eaaaae5248f0d752daa3d5929381e6dabf9c5 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 18 Sep 2026 14:44:26 -0500 Subject: [PATCH 05/11] fix: normalize catalog and artifact resolution Normalize installed extension aliases before collision checks, resolve project-installed scripts from .specify/scripts, and mirror reader defaults for extension and preset catalog identity comparisons. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/artifacts/catalog.py | 8 +- src/specify_cli/extensions/__init__.py | 13 ++- src/specify_cli/extensions/_commands.py | 6 +- src/specify_cli/presets/_commands.py | 12 ++- tests/test_artifact_command.py | 47 +++++++++- tests/test_extensions.py | 117 ++++++++++++++++++++++++ tests/test_presets.py | 88 ++++++++++++++++++ 7 files changed, 278 insertions(+), 13 deletions(-) diff --git a/src/specify_cli/artifacts/catalog.py b/src/specify_cli/artifacts/catalog.py index d56a3b0a59..8dd96108d0 100644 --- a/src/specify_cli/artifacts/catalog.py +++ b/src/specify_cli/artifacts/catalog.py @@ -99,11 +99,15 @@ def _project_core_asset_root(project_root: Path | None, subdir: str) -> Path | N return None if subdir not in {"commands", "scripts", "templates"}: return None # pragma: no cover — internal misuse + if subdir == "scripts": + candidate = project_root / ".specify" / "scripts" + return candidate if candidate.is_dir() else None + from ..presets import PresetResolver # lazy: avoids circular import candidate = PresetResolver(project_root).templates_dir - if subdir != "templates": - candidate = candidate / subdir + if subdir == "commands": + candidate = candidate / "commands" return candidate if candidate.is_dir() else None diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 882379d2f1..4ee351f887 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1216,17 +1216,24 @@ def _validate_install_conflicts(self, manifest: ExtensionManifest) -> None: installed_names = self._get_installed_command_name_map( exclude_extension_id=manifest.id ) + installed_shadow_names: Dict[str, str] = {} + for installed_name, extension_id in installed_names.items(): + installed_shadow_names.setdefault( + self._normalize_shadow_name(installed_name), extension_id + ) core_shadow_names = { self._normalize_shadow_name(f"speckit.{name}") for name in CORE_COMMAND_NAMES } collisions = [] for name in sorted(declared_names): - if name in installed_names: + normalized_name = self._normalize_shadow_name(name) + if normalized_name in installed_shadow_names: collisions.append( - f"{name} (already provided by extension '{installed_names[name]}')" + f"{name} (already provided by extension " + f"'{installed_shadow_names[normalized_name]}')" ) - elif self._normalize_shadow_name(name) in core_shadow_names: + elif normalized_name in core_shadow_names: collisions.append(f"{name} (conflicts with core command)") if collisions: diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 28375faa93..fc5747603b 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -677,11 +677,13 @@ def catalog_add( # workflow without failing. A same-name entry whose settings differ is # still a conflict — we refuse to silently change priority/install # permissions and ask the user to remove it first. - for existing in catalogs: + for idx, existing in enumerate(catalogs): if isinstance(existing, dict) and str(existing.get("name", "")).strip() == name: if ( str(existing.get("url", "")).strip() == url - and _normalize_catalog_priority(existing.get("priority")) == priority + and _normalize_catalog_priority( + existing.get("priority", idx + 1) + ) == priority and _normalize_catalog_install_allowed( existing.get("install_allowed", False) ) == install_allowed diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 40c505cb59..19658cfd05 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -923,6 +923,7 @@ def preset_catalog_add( project_root = _require_specify_project() specify_dir = project_root / ".specify" url = url.strip() + name = name.strip() # Validate URL tmp_catalog = PresetCatalog(project_root) @@ -965,11 +966,16 @@ def preset_catalog_add( # workflow without failing. A same-name entry whose settings differ is # still a conflict — we refuse to silently change priority/install # permissions and ask the user to remove it first. - for existing in catalogs: - if isinstance(existing, dict) and existing.get("name") == name: + for idx, existing in enumerate(catalogs): + if ( + isinstance(existing, dict) + and str(existing.get("name", "")).strip() == name + ): if ( str(existing.get("url", "")).strip() == url - and _normalize_catalog_priority(existing.get("priority")) == priority + and _normalize_catalog_priority( + existing.get("priority", idx + 1) + ) == priority and _normalize_catalog_install_allowed( existing.get("install_allowed", False) ) == install_allowed diff --git a/tests/test_artifact_command.py b/tests/test_artifact_command.py index b574dad7b9..c75282458f 100644 --- a/tests/test_artifact_command.py +++ b/tests/test_artifact_command.py @@ -183,6 +183,38 @@ def test_core_scripts_reuse_existing_runtime_fallback( "demo": script } + def test_core_scripts_use_project_install_without_shared_asset_fallback( + self, spec_kit_project: Path, monkeypatch: pytest.MonkeyPatch + ): + commands_dir = spec_kit_project / ".specify" / "templates" / "commands" + commands_dir.mkdir() + scripts_dir = spec_kit_project / ".specify" / "scripts" / "bash" + scripts_dir.mkdir(parents=True) + (commands_dir / "demo.md").write_text( + "---\n" + "scripts:\n" + " sh: scripts/bash/demo.sh\n" + "---\n", + encoding="utf-8", + ) + script = scripts_dir / "demo.sh" + script.write_text("#!/bin/sh\n", encoding="utf-8") + (spec_kit_project / ".specify" / "init-options.json").write_text( + json.dumps({"script": "sh"}), + encoding="utf-8", + ) + monkeypatch.setattr( + "specify_cli.artifacts.catalog._locate_shared_asset_dir", + lambda _subdir: None, + ) + + catalog = ArtifactCatalog(spec_kit_project) + + assert catalog._selected_core_script_paths() == {"demo": script} + assert "script:demo" in { + artifact.id for artifact in catalog.list_artifacts() + } + @pytest.mark.parametrize( "reference_kind", [ @@ -483,13 +515,22 @@ def test_includes_project_local_core_assets(self, spec_kit_project: Path): commands_dir = templates_dir / "commands" commands_dir.mkdir() (commands_dir / "local-command.md").write_text( - "---\ndescription: Local command\n---\n", encoding="utf-8" + "---\n" + "description: Local command\n" + "scripts:\n" + " sh: scripts/bash/legacy-script.sh\n" + "---\n", + encoding="utf-8", ) - scripts_dir = templates_dir / "scripts" - scripts_dir.mkdir() + scripts_dir = spec_kit_project / ".specify" / "scripts" / "bash" + scripts_dir.mkdir(parents=True) (scripts_dir / "legacy-script.sh").write_text( "# Local script\n", encoding="utf-8" ) + (spec_kit_project / ".specify" / "init-options.json").write_text( + json.dumps({"script": "sh"}), + encoding="utf-8", + ) catalog = ArtifactCatalog(spec_kit_project) artifacts = {artifact.id: artifact for artifact in catalog.list_artifacts()} diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 38be66a269..2608674a0e 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3126,6 +3126,79 @@ def test_install_rejects_command_collision_with_installed_extension(self, temp_d with pytest.raises(ValidationError, match="already provided by extension 'ext-one'"): manager.install_from_directory(second_dir, "0.1.0", register_commands=False) + def test_install_rejects_equivalent_alias_collision_with_installed_extension( + self, temp_dir, project_dir + ): + """Equivalent alias spellings must not overwrite installed output.""" + import yaml + + first_dir = temp_dir / "ext-one" + first_dir.mkdir() + (first_dir / "commands").mkdir() + first_manifest = { + "schema_version": "1.0", + "extension": { + "id": "ext-one", + "name": "Extension One", + "version": "1.0.0", + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + { + "name": "speckit.ext-one.sync", + "file": "commands/cmd.md", + "aliases": ["shared-sync"], + } + ] + }, + } + (first_dir / "extension.yml").write_text(yaml.dump(first_manifest)) + (first_dir / "commands" / "cmd.md").write_text( + "---\ndescription: Test\n---\n\nBody" + ) + installed_ext_dir = project_dir / ".specify" / "extensions" / "ext-one" + installed_ext_dir.parent.mkdir(parents=True, exist_ok=True) + shutil.copytree(first_dir, installed_ext_dir) + + second_dir = temp_dir / "ext-two" + second_dir.mkdir() + (second_dir / "commands").mkdir() + second_manifest = { + "schema_version": "1.0", + "extension": { + "id": "ext-two", + "name": "Extension Two", + "version": "1.0.0", + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + { + "name": "speckit.ext-two.sync", + "file": "commands/cmd.md", + "aliases": ["speckit.shared.sync"], + } + ] + }, + } + (second_dir / "extension.yml").write_text(yaml.dump(second_manifest)) + (second_dir / "commands" / "cmd.md").write_text( + "---\ndescription: Test\n---\n\nBody" + ) + + manager = ExtensionManager(project_dir) + manager.registry.add("ext-one", {"version": "1.0.0", "source": "local"}) + + with pytest.raises( + ValidationError, match="already provided by extension 'ext-one'" + ): + manager.install_from_directory( + second_dir, "0.1.0", register_commands=False + ) + def test_install_rejects_alias_shadowing_core_command(self, temp_dir, project_dir): """An alias equal to a core command's qualified name must not install. @@ -7760,6 +7833,50 @@ def test_catalog_add_priority_equivalence(self, project_dir, monkeypatch, stored assert config_path.read_bytes() == original assert config_path.stat().st_mtime_ns == modified_at + def test_catalog_add_omitted_priority_uses_reader_default( + self, project_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + config_path = project_dir / ".specify" / "extension-catalogs.yml" + config_path.write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "name": "mine", + "url": "https://example.com/catalog.json", + "install_allowed": False, + } + ] + } + ), + encoding="utf-8", + ) + original = config_path.read_bytes() + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "extension", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + "mine", + "--priority", + "1", + ], + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert "nothing to do" in result.output + assert config_path.read_bytes() == original + def test_catalog_add_escapes_config_saved_path_markup(self, tmp_path): """Catalog add's saved-path label should render literally under Rich.""" from typer.testing import CliRunner diff --git a/tests/test_presets.py b/tests/test_presets.py index abeedf9c83..3ef53a8554 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3973,6 +3973,94 @@ def test_catalog_add_normalizes_existing_url( assert config_path.read_bytes() == original assert config_path.stat().st_mtime_ns == modified_at + @pytest.mark.parametrize("requested_name", ["mine", " mine "]) + def test_catalog_add_normalizes_existing_name( + self, project_dir, monkeypatch, requested_name + ): + from typer.testing import CliRunner + from specify_cli import app + + config_path = project_dir / ".specify" / "preset-catalogs.yml" + config_path.write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "name": " mine ", + "url": "https://example.com/catalog.json", + "priority": 10, + "install_allowed": False, + } + ] + } + ), + encoding="utf-8", + ) + original = config_path.read_bytes() + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "preset", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + requested_name, + ], + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert "nothing to do" in result.output + assert config_path.read_bytes() == original + + def test_catalog_add_omitted_priority_uses_reader_default( + self, project_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + config_path = project_dir / ".specify" / "preset-catalogs.yml" + config_path.write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "name": "mine", + "url": "https://example.com/catalog.json", + "install_allowed": False, + } + ] + } + ), + encoding="utf-8", + ) + original = config_path.read_bytes() + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "preset", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + "mine", + "--priority", + "1", + ], + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert "nothing to do" in result.output + assert config_path.read_bytes() == original + def test_catalog_add_duplicate_different_settings_conflicts(self, project_dir): """Re-adding a same-named preset catalog with different settings errors (#4505).""" from typer.testing import CliRunner From 055c5c20e29e30a68953ce3e557960879d5bd58a Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 18 Sep 2026 15:11:12 -0500 Subject: [PATCH 06/11] fix: align catalog idempotency normalization Mirror catalog reader defaults for generated names, validate matching integration entries before no-op returns, and preserve custom bundle source IDs when a URL rerun omits --id. Remove unrelated artifact and extension-collision follow-ups from this PR after rebasing. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/artifacts/catalog.py | 8 +- .../bundler/commands_impl/catalog_config.py | 8 +- src/specify_cli/extensions/__init__.py | 13 +- src/specify_cli/extensions/_commands.py | 19 ++- src/specify_cli/integrations/catalog.py | 31 +++-- src/specify_cli/presets/_commands.py | 22 +++- .../integrations/test_integration_catalog.py | 29 +++++ tests/test_artifact_command.py | 47 +------- tests/test_extensions.py | 113 +++++++----------- tests/test_presets.py | 40 +++++++ tests/unit/test_bundler_catalog_config.py | 24 ++++ 11 files changed, 196 insertions(+), 158 deletions(-) diff --git a/src/specify_cli/artifacts/catalog.py b/src/specify_cli/artifacts/catalog.py index 8dd96108d0..d56a3b0a59 100644 --- a/src/specify_cli/artifacts/catalog.py +++ b/src/specify_cli/artifacts/catalog.py @@ -99,15 +99,11 @@ def _project_core_asset_root(project_root: Path | None, subdir: str) -> Path | N return None if subdir not in {"commands", "scripts", "templates"}: return None # pragma: no cover — internal misuse - if subdir == "scripts": - candidate = project_root / ".specify" / "scripts" - return candidate if candidate.is_dir() else None - from ..presets import PresetResolver # lazy: avoids circular import candidate = PresetResolver(project_root).templates_dir - if subdir == "commands": - candidate = candidate / "commands" + if subdir != "templates": + candidate = candidate / subdir return candidate if candidate.is_dir() else None diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index c8f17407ce..53a2852695 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -183,7 +183,8 @@ def add_source( url = _canonicalize_url(url) install_policy = InstallPolicy.parse(policy) - resolved_id = (source_id or _derive_id(url)).strip() + requested_id = source_id.strip() if source_id is not None else "" + resolved_id = requested_id or _derive_id(url) catalogs = _read(project_root) desired = { @@ -210,7 +211,10 @@ def add_source( # string priority) compare equal to the requested defaults. existing_source = CatalogSource.from_dict(dict(existing), Scope.PROJECT) if ( - existing_source.id == resolved_id + ( + existing_source.id == resolved_id + or (not requested_id and existing_source.url == url) + ) and existing_source.url == url and existing_source.priority == desired["priority"] and existing_source.install_policy.value == desired["install_policy"] diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 4ee351f887..882379d2f1 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1216,24 +1216,17 @@ def _validate_install_conflicts(self, manifest: ExtensionManifest) -> None: installed_names = self._get_installed_command_name_map( exclude_extension_id=manifest.id ) - installed_shadow_names: Dict[str, str] = {} - for installed_name, extension_id in installed_names.items(): - installed_shadow_names.setdefault( - self._normalize_shadow_name(installed_name), extension_id - ) core_shadow_names = { self._normalize_shadow_name(f"speckit.{name}") for name in CORE_COMMAND_NAMES } collisions = [] for name in sorted(declared_names): - normalized_name = self._normalize_shadow_name(name) - if normalized_name in installed_shadow_names: + if name in installed_names: collisions.append( - f"{name} (already provided by extension " - f"'{installed_shadow_names[normalized_name]}')" + f"{name} (already provided by extension '{installed_names[name]}')" ) - elif normalized_name in core_shadow_names: + elif self._normalize_shadow_name(name) in core_shadow_names: collisions.append(f"{name} (conflicts with core command)") if collisions: diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index fc5747603b..44895f38e8 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -677,10 +677,25 @@ def catalog_add( # workflow without failing. A same-name entry whose settings differ is # still a conflict — we refuse to silently change priority/install # permissions and ask the user to remove it first. + valid_catalog_count = 0 for idx, existing in enumerate(catalogs): - if isinstance(existing, dict) and str(existing.get("name", "")).strip() == name: + if not isinstance(existing, dict): + continue + existing_url = str(existing.get("url", "")).strip() + if not existing_url: + continue + valid_catalog_count += 1 + raw_existing_name = existing.get("name") + existing_name = ( + str(raw_existing_name).strip() + if raw_existing_name is not None + else "" + ) + if not existing_name: + existing_name = f"catalog-{valid_catalog_count}" + if existing_name == name: if ( - str(existing.get("url", "")).strip() == url + existing_url == url and _normalize_catalog_priority( existing.get("priority", idx + 1) ) == priority diff --git a/src/specify_cli/integrations/catalog.py b/src/specify_cli/integrations/catalog.py index 70dcb9caac..4abe423c9d 100644 --- a/src/specify_cli/integrations/catalog.py +++ b/src/specify_cli/integrations/catalog.py @@ -458,17 +458,6 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> str: raise IntegrationValidationError( f"Invalid catalog entry at index {idx} in {config_path}: {exc}" ) from exc - if existing_url == url: - # Idempotent add (#4505): same URL already configured. - existing_name = str(cat.get("name", "")).strip() - if not requested_name or requested_name == existing_name: - return "unchanged" - raise IntegrationValidationError( - f"Catalog URL already configured with a different name " - f"('{existing_name}'): {url}. Remove it first or pass " - f"--name '{existing_name}'." - ) - valid_catalog_count += 1 if "priority" in cat: raw_priority = cat.get("priority") if isinstance(raw_priority, bool): @@ -480,17 +469,27 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> str: try: normalized_priority = int(raw_priority) except (TypeError, ValueError, OverflowError): - # OverflowError: int(float("inf")) — a ``priority: .inf``. raise IntegrationValidationError( f"Invalid catalog entry at index {idx} in {config_path}: " f"'priority' must be an integer, got " f"{raw_priority!r}." ) from None - existing_priorities.append(normalized_priority) else: - # Match `_load_catalog_config()`'s defaulting rule so the new - # entry still sorts after implicit-priority siblings. - existing_priorities.append(idx + 1) + # Match `_load_catalog_config()`'s defaulting rule. + normalized_priority = idx + 1 + existing_priorities.append(normalized_priority) + + if existing_url == url: + # Idempotent add (#4505): same URL already configured. + existing_name = str(cat.get("name", "")).strip() + if not requested_name or requested_name == existing_name: + return "unchanged" + raise IntegrationValidationError( + f"Catalog URL already configured with a different name " + f"('{existing_name}'): {url}. Remove it first or pass " + f"--name '{existing_name}'." + ) + valid_catalog_count += 1 max_priority = max(existing_priorities, default=0) normalized_name = str(name).strip() if name is not None else "" diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 19658cfd05..96842e6173 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -966,13 +966,25 @@ def preset_catalog_add( # workflow without failing. A same-name entry whose settings differ is # still a conflict — we refuse to silently change priority/install # permissions and ask the user to remove it first. + valid_catalog_count = 0 for idx, existing in enumerate(catalogs): - if ( - isinstance(existing, dict) - and str(existing.get("name", "")).strip() == name - ): + if not isinstance(existing, dict): + continue + existing_url = str(existing.get("url", "")).strip() + if not existing_url: + continue + valid_catalog_count += 1 + raw_existing_name = existing.get("name") + existing_name = ( + str(raw_existing_name).strip() + if raw_existing_name is not None + else "" + ) + if not existing_name: + existing_name = f"catalog-{valid_catalog_count}" + if existing_name == name: if ( - str(existing.get("url", "")).strip() == url + existing_url == url and _normalize_catalog_priority( existing.get("priority", idx + 1) ) == priority diff --git a/tests/integrations/test_integration_catalog.py b/tests/integrations/test_integration_catalog.py index b8f2714c85..d50f1787ef 100644 --- a/tests/integrations/test_integration_catalog.py +++ b/tests/integrations/test_integration_catalog.py @@ -1196,6 +1196,35 @@ def test_add_catalog_duplicate_url_same_name_is_idempotent_noop(self, tmp_path, data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) assert len(data["catalogs"]) == 1 + def test_add_catalog_duplicate_url_validates_stored_priority( + self, tmp_path, monkeypatch + ): + self._isolate(tmp_path, monkeypatch) + cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" + cfg_path.parent.mkdir(parents=True, exist_ok=True) + cfg_path.write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "name": "mine", + "url": "https://dup.example.com/catalog.json", + "priority": "first", + } + ] + } + ), + encoding="utf-8", + ) + + with pytest.raises( + IntegrationValidationError, match="'priority' must be an integer" + ): + IntegrationCatalog(tmp_path).add_catalog( + "https://dup.example.com/catalog.json", + name="mine", + ) + def test_add_catalog_rejects_invalid_url(self, tmp_path, monkeypatch): self._isolate(tmp_path, monkeypatch) cat = IntegrationCatalog(tmp_path) diff --git a/tests/test_artifact_command.py b/tests/test_artifact_command.py index c75282458f..b574dad7b9 100644 --- a/tests/test_artifact_command.py +++ b/tests/test_artifact_command.py @@ -183,38 +183,6 @@ def test_core_scripts_reuse_existing_runtime_fallback( "demo": script } - def test_core_scripts_use_project_install_without_shared_asset_fallback( - self, spec_kit_project: Path, monkeypatch: pytest.MonkeyPatch - ): - commands_dir = spec_kit_project / ".specify" / "templates" / "commands" - commands_dir.mkdir() - scripts_dir = spec_kit_project / ".specify" / "scripts" / "bash" - scripts_dir.mkdir(parents=True) - (commands_dir / "demo.md").write_text( - "---\n" - "scripts:\n" - " sh: scripts/bash/demo.sh\n" - "---\n", - encoding="utf-8", - ) - script = scripts_dir / "demo.sh" - script.write_text("#!/bin/sh\n", encoding="utf-8") - (spec_kit_project / ".specify" / "init-options.json").write_text( - json.dumps({"script": "sh"}), - encoding="utf-8", - ) - monkeypatch.setattr( - "specify_cli.artifacts.catalog._locate_shared_asset_dir", - lambda _subdir: None, - ) - - catalog = ArtifactCatalog(spec_kit_project) - - assert catalog._selected_core_script_paths() == {"demo": script} - assert "script:demo" in { - artifact.id for artifact in catalog.list_artifacts() - } - @pytest.mark.parametrize( "reference_kind", [ @@ -515,22 +483,13 @@ def test_includes_project_local_core_assets(self, spec_kit_project: Path): commands_dir = templates_dir / "commands" commands_dir.mkdir() (commands_dir / "local-command.md").write_text( - "---\n" - "description: Local command\n" - "scripts:\n" - " sh: scripts/bash/legacy-script.sh\n" - "---\n", - encoding="utf-8", + "---\ndescription: Local command\n---\n", encoding="utf-8" ) - scripts_dir = spec_kit_project / ".specify" / "scripts" / "bash" - scripts_dir.mkdir(parents=True) + scripts_dir = templates_dir / "scripts" + scripts_dir.mkdir() (scripts_dir / "legacy-script.sh").write_text( "# Local script\n", encoding="utf-8" ) - (spec_kit_project / ".specify" / "init-options.json").write_text( - json.dumps({"script": "sh"}), - encoding="utf-8", - ) catalog = ArtifactCatalog(spec_kit_project) artifacts = {artifact.id: artifact for artifact in catalog.list_artifacts()} diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 2608674a0e..c7c4810edf 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3126,79 +3126,6 @@ def test_install_rejects_command_collision_with_installed_extension(self, temp_d with pytest.raises(ValidationError, match="already provided by extension 'ext-one'"): manager.install_from_directory(second_dir, "0.1.0", register_commands=False) - def test_install_rejects_equivalent_alias_collision_with_installed_extension( - self, temp_dir, project_dir - ): - """Equivalent alias spellings must not overwrite installed output.""" - import yaml - - first_dir = temp_dir / "ext-one" - first_dir.mkdir() - (first_dir / "commands").mkdir() - first_manifest = { - "schema_version": "1.0", - "extension": { - "id": "ext-one", - "name": "Extension One", - "version": "1.0.0", - "description": "Test", - }, - "requires": {"speckit_version": ">=0.1.0"}, - "provides": { - "commands": [ - { - "name": "speckit.ext-one.sync", - "file": "commands/cmd.md", - "aliases": ["shared-sync"], - } - ] - }, - } - (first_dir / "extension.yml").write_text(yaml.dump(first_manifest)) - (first_dir / "commands" / "cmd.md").write_text( - "---\ndescription: Test\n---\n\nBody" - ) - installed_ext_dir = project_dir / ".specify" / "extensions" / "ext-one" - installed_ext_dir.parent.mkdir(parents=True, exist_ok=True) - shutil.copytree(first_dir, installed_ext_dir) - - second_dir = temp_dir / "ext-two" - second_dir.mkdir() - (second_dir / "commands").mkdir() - second_manifest = { - "schema_version": "1.0", - "extension": { - "id": "ext-two", - "name": "Extension Two", - "version": "1.0.0", - "description": "Test", - }, - "requires": {"speckit_version": ">=0.1.0"}, - "provides": { - "commands": [ - { - "name": "speckit.ext-two.sync", - "file": "commands/cmd.md", - "aliases": ["speckit.shared.sync"], - } - ] - }, - } - (second_dir / "extension.yml").write_text(yaml.dump(second_manifest)) - (second_dir / "commands" / "cmd.md").write_text( - "---\ndescription: Test\n---\n\nBody" - ) - - manager = ExtensionManager(project_dir) - manager.registry.add("ext-one", {"version": "1.0.0", "source": "local"}) - - with pytest.raises( - ValidationError, match="already provided by extension 'ext-one'" - ): - manager.install_from_directory( - second_dir, "0.1.0", register_commands=False - ) - def test_install_rejects_alias_shadowing_core_command(self, temp_dir, project_dir): """An alias equal to a core command's qualified name must not install. @@ -7877,6 +7804,46 @@ def test_catalog_add_omitted_priority_uses_reader_default( assert "nothing to do" in result.output assert config_path.read_bytes() == original + @pytest.mark.parametrize("stored_name", ["missing", None, "", " \t"]) + def test_catalog_add_blank_name_uses_reader_default( + self, project_dir, monkeypatch, stored_name + ): + from typer.testing import CliRunner + from specify_cli import app + + config_path = project_dir / ".specify" / "extension-catalogs.yml" + entry = { + "url": "https://example.com/catalog.json", + "priority": 10, + "install_allowed": False, + } + if stored_name != "missing": + entry["name"] = stored_name + config_path.write_text( + yaml.safe_dump({"catalogs": [entry]}), + encoding="utf-8", + ) + original = config_path.read_bytes() + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "extension", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + "catalog-1", + ], + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert "nothing to do" in result.output + assert config_path.read_bytes() == original + def test_catalog_add_escapes_config_saved_path_markup(self, tmp_path): """Catalog add's saved-path label should render literally under Rich.""" from typer.testing import CliRunner diff --git a/tests/test_presets.py b/tests/test_presets.py index 3ef53a8554..a583944e74 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -4061,6 +4061,46 @@ def test_catalog_add_omitted_priority_uses_reader_default( assert "nothing to do" in result.output assert config_path.read_bytes() == original + @pytest.mark.parametrize("stored_name", ["missing", None, "", " \t"]) + def test_catalog_add_blank_name_uses_reader_default( + self, project_dir, monkeypatch, stored_name + ): + from typer.testing import CliRunner + from specify_cli import app + + config_path = project_dir / ".specify" / "preset-catalogs.yml" + entry = { + "url": "https://example.com/catalog.json", + "priority": 10, + "install_allowed": False, + } + if stored_name != "missing": + entry["name"] = stored_name + config_path.write_text( + yaml.safe_dump({"catalogs": [entry]}), + encoding="utf-8", + ) + original = config_path.read_bytes() + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "preset", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + "catalog-1", + ], + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert "nothing to do" in result.output + assert config_path.read_bytes() == original + def test_catalog_add_duplicate_different_settings_conflicts(self, project_dir): """Re-adding a same-named preset catalog with different settings errors (#4505).""" from typer.testing import CliRunner diff --git a/tests/unit/test_bundler_catalog_config.py b/tests/unit/test_bundler_catalog_config.py index b08d90df62..94794b7ce6 100644 --- a/tests/unit/test_bundler_catalog_config.py +++ b/tests/unit/test_bundler_catalog_config.py @@ -176,6 +176,30 @@ def test_add_source_rerun_with_string_priority_is_unchanged(tmp_path: Path): assert source.priority == 10 +def test_add_source_same_url_without_id_preserves_custom_id(tmp_path: Path): + project = tmp_path / "proj" + (project / ".specify").mkdir(parents=True) + original_source, first_status = cc.add_source( + project, + "https://example.com/c.json", + source_id="custom", + policy="install-allowed", + priority=10, + ) + + source, status = cc.add_source( + project, + "https://example.com/c.json", + policy="install-allowed", + priority=10, + ) + + assert first_status == "added" + assert status == "unchanged" + assert source.id == original_source.id == "custom" + assert len(cc._read(project)) == 1 + + @pytest.mark.parametrize("id_padding", ["", " \t"]) @pytest.mark.parametrize("url_padding", ["", " \t"]) @pytest.mark.parametrize("source_id,url,outcome", [ From a2c71d08bd2086c403746961af93d7f47786dc48 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 18 Sep 2026 15:28:32 -0500 Subject: [PATCH 07/11] fix: harden catalog identity edge cases Reject blank catalog names, preserve same-name conflicts for blank-URL entries, escape Rich success output, normalize loader fallback names, and validate all integration catalog entries before returning unchanged. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/extensions/_commands.py | 10 ++- .../integrations/_query_commands.py | 7 +- src/specify_cli/integrations/catalog.py | 34 ++++++--- src/specify_cli/presets/_commands.py | 10 ++- src/specify_cli/workflows/_commands.py | 16 ++-- src/specify_cli/workflows/catalog.py | 12 ++- tests/integrations/test_cli.py | 12 +++ .../integrations/test_integration_catalog.py | 75 +++++++++++++++++++ tests/test_extensions.py | 68 +++++++++++++++++ tests/test_presets.py | 70 +++++++++++++++++ tests/test_workflows.py | 60 +++++++++++++++ 11 files changed, 343 insertions(+), 31 deletions(-) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 44895f38e8..2283f3c0cb 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -647,6 +647,9 @@ def catalog_add( specify_dir = project_root / ".specify" url = url.strip() name = name.strip() + if not name: + console.print("[red]Error:[/red] Catalog name must be non-empty.") + raise typer.Exit(1) # Validate URL tmp_catalog = ExtensionCatalog(project_root) @@ -682,16 +685,15 @@ def catalog_add( if not isinstance(existing, dict): continue existing_url = str(existing.get("url", "")).strip() - if not existing_url: - continue - valid_catalog_count += 1 raw_existing_name = existing.get("name") existing_name = ( str(raw_existing_name).strip() if raw_existing_name is not None else "" ) - if not existing_name: + if existing_url: + valid_catalog_count += 1 + if not existing_name and existing_url: existing_name = f"catalog-{valid_catalog_count}" if existing_name == name: if ( diff --git a/src/specify_cli/integrations/_query_commands.py b/src/specify_cli/integrations/_query_commands.py index 5525822923..0426606a2f 100644 --- a/src/specify_cli/integrations/_query_commands.py +++ b/src/specify_cli/integrations/_query_commands.py @@ -547,15 +547,16 @@ def integration_catalog_add( except IntegrationCatalogError as exc: # Covers both URL validation (base class) and config-file validation # (IntegrationValidationError subclass). - console.print(f"[red]Error:[/red] {exc}") + console.print(f"[red]Error:[/red] {_rich_escape(str(exc))}") raise typer.Exit(1) + safe_url = _rich_escape(normalized_url) if status == "unchanged": console.print( - f"[green]✓[/green] Catalog source already configured: {normalized_url}" + f"[green]✓[/green] Catalog source already configured: {safe_url}" ) else: - console.print(f"[green]✓[/green] Catalog source added: {normalized_url}") + console.print(f"[green]✓[/green] Catalog source added: {safe_url}") @integration_catalog_app.command("remove") diff --git a/src/specify_cli/integrations/catalog.py b/src/specify_cli/integrations/catalog.py index 4abe423c9d..f787461586 100644 --- a/src/specify_cli/integrations/catalog.py +++ b/src/specify_cli/integrations/catalog.py @@ -441,6 +441,7 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> str: requested_name = str(name).strip() if name is not None else "" existing_priorities: List[int] = [] valid_catalog_count = 0 + matching_catalog: tuple[bool, str] | None = None for idx, cat in enumerate(catalogs): if not isinstance(cat, dict): raise IntegrationValidationError( @@ -478,18 +479,31 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> str: # Match `_load_catalog_config()`'s defaulting rule. normalized_priority = idx + 1 existing_priorities.append(normalized_priority) + valid_catalog_count += 1 - if existing_url == url: - # Idempotent add (#4505): same URL already configured. - existing_name = str(cat.get("name", "")).strip() - if not requested_name or requested_name == existing_name: - return "unchanged" - raise IntegrationValidationError( - f"Catalog URL already configured with a different name " - f"('{existing_name}'): {url}. Remove it first or pass " - f"--name '{existing_name}'." + raw_existing_name = cat.get("name") + existing_name = ( + str(raw_existing_name).strip() + if raw_existing_name is not None + else "" + ) + if not existing_name: + existing_name = f"catalog-{valid_catalog_count}" + if existing_url == url and matching_catalog is None: + matching_catalog = ( + not requested_name or requested_name == existing_name, + existing_name, ) - valid_catalog_count += 1 + + if matching_catalog is not None: + names_match, existing_name = matching_catalog + if names_match: + return "unchanged" + raise IntegrationValidationError( + f"Catalog URL already configured with a different name " + f"('{existing_name}'): {url}. Remove it first or pass " + f"--name '{existing_name}'." + ) max_priority = max(existing_priorities, default=0) normalized_name = str(name).strip() if name is not None else "" diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 96842e6173..1bd5728a2a 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -924,6 +924,9 @@ def preset_catalog_add( specify_dir = project_root / ".specify" url = url.strip() name = name.strip() + if not name: + console.print("[red]Error:[/red] Catalog name must be non-empty.") + raise typer.Exit(1) # Validate URL tmp_catalog = PresetCatalog(project_root) @@ -971,16 +974,15 @@ def preset_catalog_add( if not isinstance(existing, dict): continue existing_url = str(existing.get("url", "")).strip() - if not existing_url: - continue - valid_catalog_count += 1 raw_existing_name = existing.get("name") existing_name = ( str(raw_existing_name).strip() if raw_existing_name is not None else "" ) - if not existing_name: + if existing_url: + valid_catalog_count += 1 + if not existing_name and existing_url: existing_name = f"catalog-{valid_catalog_count}" if existing_name == name: if ( diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index db648a6f57..2ff4f914a0 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3027,13 +3027,14 @@ def workflow_catalog_add( try: status = catalog.add_catalog(url, name) except WorkflowValidationError as exc: - console.print(f"[red]Error:[/red] {exc}") + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") raise typer.Exit(1) + safe_url = _escape_markup(url) if status == "unchanged": - console.print(f"[green]✓[/green] Catalog source already configured: {url}") + console.print(f"[green]✓[/green] Catalog source already configured: {safe_url}") else: - console.print(f"[green]✓[/green] Catalog source added: {url}") + console.print(f"[green]✓[/green] Catalog source added: {safe_url}") @workflow_catalog_app.command("remove") @@ -3776,13 +3777,16 @@ def workflow_step_catalog_add( try: status = catalog.add_catalog(url, name) except StepValidationError as exc: - console.print(f"[red]Error:[/red] {exc}") + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") raise typer.Exit(1) + safe_url = _escape_markup(url) if status == "unchanged": - console.print(f"[green]✓[/green] Step catalog source already configured: {url}") + console.print( + f"[green]✓[/green] Step catalog source already configured: {safe_url}" + ) else: - console.print(f"[green]✓[/green] Step catalog source added: {url}") + console.print(f"[green]✓[/green] Step catalog source added: {safe_url}") @workflow_step_catalog_app.command("remove") diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 4451f3cebc..8e3a2f6f98 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -742,9 +742,11 @@ def add_catalog(self, url: str, name: str | None = None) -> str: # Idempotent add (#4505): identity is the URL. A rerun requesting the # same name (or no explicit name) is a no-op; a different name conflicts. requested_name = str(name).strip() if name is not None else "" - for cat in catalogs: + for idx, cat in enumerate(catalogs): if isinstance(cat, dict) and str(cat.get("url", "")).strip() == url: - existing_name = str(cat.get("name", "")).strip() + existing_name = str( + cat.get("name", f"catalog-{idx + 1}") + ).strip() if not requested_name or requested_name == existing_name: return "unchanged" raise WorkflowValidationError( @@ -1441,9 +1443,11 @@ def add_catalog(self, url: str, name: str | None = None) -> str: # Idempotent add (#4505): identity is the URL. A rerun requesting the # same name (or no explicit name) is a no-op; a different name conflicts. requested_name = str(name).strip() if name is not None else "" - for cat in catalogs: + for idx, cat in enumerate(catalogs): if isinstance(cat, dict) and str(cat.get("url", "")).strip() == url: - existing_name = str(cat.get("name", "")).strip() + existing_name = str( + cat.get("name", f"catalog-{idx + 1}") + ).strip() if not requested_name or requested_name == existing_name: return "unchanged" raise StepValidationError( diff --git a/tests/integrations/test_cli.py b/tests/integrations/test_cli.py index 6ffe023da9..e2aa7f448c 100644 --- a/tests/integrations/test_cli.py +++ b/tests/integrations/test_cli.py @@ -2565,6 +2565,18 @@ def test_catalog_add_duplicate_different_name_conflicts(self, tmp_path, monkeypa assert second.exit_code == 1 assert "different name" in second.output + def test_catalog_add_escapes_markup_in_success_output(self, tmp_path, monkeypatch): + project = self._make_project(tmp_path) + url = "https://dup.example.com/[/red]/catalog.json" + + first = self._invoke(["integration", "catalog", "add", url], project) + assert first.exit_code == 0, first.output + assert url in first.output + + second = self._invoke(["integration", "catalog", "add", url], project) + assert second.exit_code == 0, second.output + assert url in second.output + def test_catalog_remove_out_of_range(self, tmp_path, monkeypatch): project = self._make_project(tmp_path) # Need a config file for remove to attempt an index lookup diff --git a/tests/integrations/test_integration_catalog.py b/tests/integrations/test_integration_catalog.py index d50f1787ef..e5fe0c785c 100644 --- a/tests/integrations/test_integration_catalog.py +++ b/tests/integrations/test_integration_catalog.py @@ -1225,6 +1225,81 @@ def test_add_catalog_duplicate_url_validates_stored_priority( name="mine", ) + @pytest.mark.parametrize("stored_name", ["missing", None, "", " \t"]) + def test_add_catalog_duplicate_url_uses_reader_name_fallback( + self, tmp_path, monkeypatch, stored_name + ): + self._isolate(tmp_path, monkeypatch) + cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" + cfg_path.parent.mkdir(parents=True, exist_ok=True) + entry = { + "url": "https://dup.example.com/catalog.json", + "priority": 1, + } + if stored_name != "missing": + entry["name"] = stored_name + cfg_path.write_text( + yaml.safe_dump({"catalogs": [entry]}), + encoding="utf-8", + ) + + assert ( + IntegrationCatalog(tmp_path).add_catalog( + "https://dup.example.com/catalog.json", + name="catalog-1", + ) + == "unchanged" + ) + + @pytest.mark.parametrize( + ("invalid_entry", "match"), + [ + ( + { + "name": "bad-url", + "url": "http://example.com/catalog.json", + "priority": 2, + }, + "HTTPS", + ), + ( + { + "name": "bad-priority", + "url": "https://other.example.com/catalog.json", + "priority": "first", + }, + "'priority' must be an integer", + ), + ], + ) + def test_add_catalog_duplicate_url_validates_remaining_entries( + self, tmp_path, monkeypatch, invalid_entry, match + ): + self._isolate(tmp_path, monkeypatch) + cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" + cfg_path.parent.mkdir(parents=True, exist_ok=True) + cfg_path.write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "name": "mine", + "url": "https://dup.example.com/catalog.json", + "priority": 1, + }, + invalid_entry, + ] + } + ), + encoding="utf-8", + ) + + with pytest.raises(IntegrationValidationError, match=match): + IntegrationCatalog(tmp_path).add_catalog( + "https://dup.example.com/catalog.json", + name="mine", + ) + def test_add_catalog_rejects_invalid_url(self, tmp_path, monkeypatch): self._isolate(tmp_path, monkeypatch) cat = IntegrationCatalog(tmp_path) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index c7c4810edf..564db2bde4 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7568,6 +7568,31 @@ def test_extensionignore_negation_pattern(self, temp_dir, valid_manifest_data): class TestExtensionAddCLI: """CLI integration tests for extension add command.""" + def test_catalog_add_rejects_empty_normalized_name(self, project_dir, monkeypatch): + from typer.testing import CliRunner + from specify_cli import app + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "extension", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + " \t", + ], + catch_exceptions=False, + ) + + assert result.exit_code == 1 + assert "must be non-empty" in result.output + assert not ( + project_dir / ".specify" / "extension-catalogs.yml" + ).exists() + def test_catalog_add_escapes_url_markup(self, tmp_path): """Catalog add should render user-supplied URLs literally.""" from typer.testing import CliRunner @@ -7844,6 +7869,49 @@ def test_catalog_add_blank_name_uses_reader_default( assert "nothing to do" in result.output assert config_path.read_bytes() == original + def test_catalog_add_same_name_with_blank_url_conflicts( + self, project_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + config_path = project_dir / ".specify" / "extension-catalogs.yml" + config_path.write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "name": "mine", + "url": "", + "priority": 10, + "install_allowed": False, + } + ] + } + ), + encoding="utf-8", + ) + original = config_path.read_bytes() + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "extension", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + "mine", + ], + catch_exceptions=False, + ) + + assert result.exit_code == 1 + assert "different settings" in result.output + assert config_path.read_bytes() == original + def test_catalog_add_escapes_config_saved_path_markup(self, tmp_path): """Catalog add's saved-path label should render literally under Rich.""" from typer.testing import CliRunner diff --git a/tests/test_presets.py b/tests/test_presets.py index a583944e74..19c1132768 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3848,6 +3848,33 @@ def test_default_description(self): class TestPresetCatalogMultiCatalog: + def test_catalog_add_rejects_empty_normalized_name( + self, project_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "preset", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + " \t", + ], + catch_exceptions=False, + ) + + assert result.exit_code == 1 + assert "must be non-empty" in result.output + assert not ( + project_dir / ".specify" / "preset-catalogs.yml" + ).exists() + """Test multi-catalog support in PresetCatalog.""" def test_default_active_catalogs(self, project_dir): @@ -4101,6 +4128,49 @@ def test_catalog_add_blank_name_uses_reader_default( assert "nothing to do" in result.output assert config_path.read_bytes() == original + def test_catalog_add_same_name_with_blank_url_conflicts( + self, project_dir, monkeypatch + ): + from typer.testing import CliRunner + from specify_cli import app + + config_path = project_dir / ".specify" / "preset-catalogs.yml" + config_path.write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "name": "mine", + "url": "", + "priority": 10, + "install_allowed": False, + } + ] + } + ), + encoding="utf-8", + ) + original = config_path.read_bytes() + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + "preset", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + "mine", + ], + catch_exceptions=False, + ) + + assert result.exit_code == 1 + assert "different settings" in result.output + assert config_path.read_bytes() == original + def test_catalog_add_duplicate_different_settings_conflicts(self, project_dir): """Re-adding a same-named preset catalog with different settings errors (#4505).""" from typer.testing import CliRunner diff --git a/tests/test_workflows.py b/tests/test_workflows.py index ac87c7733a..0a2043209b 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -9986,6 +9986,66 @@ def make_module_name(type_key: str) -> str: (["workflow", "step", "catalog", "add"], "step-catalogs.yml"), ]) class TestWorkflowCatalogAddCLI: + def test_add_catalog_escapes_markup_in_success_output( + self, project_dir, monkeypatch, command, config_filename + ): + from typer.testing import CliRunner + from specify_cli import app + + url = "https://example.com/[/red]/catalog.json" + runner = CliRunner() + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + first = runner.invoke( + app, [*command, url], catch_exceptions=False + ) + assert first.exit_code == 0, first.output + assert url in first.output + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + second = runner.invoke( + app, [*command, url], catch_exceptions=False + ) + assert second.exit_code == 0, second.output + assert url in second.output + + def test_add_catalog_missing_name_uses_loader_fallback( + self, project_dir, monkeypatch, command, config_filename + ): + from typer.testing import CliRunner + from specify_cli import app + + url = "https://example.com/catalog.json" + config_path = project_dir / ".specify" / config_filename + config_path.write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "url": url, + "priority": 1, + "install_allowed": True, + } + ] + } + ), + encoding="utf-8", + ) + original = config_path.read_bytes() + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [*command, url, "--name", "catalog-1"], + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert "already configured" in result.output + assert config_path.read_bytes() == original + @pytest.mark.parametrize("name", [None, "mine"]) @pytest.mark.parametrize("padding", ["", " \t"]) def test_add_catalog_duplicate_outcomes( From 0904615595957888e29a6034108efe9bebc0b7b8 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 18 Sep 2026 15:41:07 -0500 Subject: [PATCH 08/11] fix: normalize catalog removal names Trim requested and stored extension and preset catalog names during removal so the recovery path uses the same identity rules as idempotent add. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/extensions/_commands.py | 8 +++++- src/specify_cli/presets/_commands.py | 8 +++++- tests/test_extensions.py | 38 +++++++++++++++++++++++++ tests/test_presets.py | 38 +++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 2283f3c0cb..74c40a162c 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -744,6 +744,7 @@ def catalog_remove( """Remove a catalog from .specify/extension-catalogs.yml.""" project_root = _require_specify_project() specify_dir = project_root / ".specify" + name = name.strip() config_path = specify_dir / "extension-catalogs.yml" if not config_path.exists(): @@ -758,7 +759,12 @@ def catalog_remove( raise typer.Exit(1) safe_name = _escape_markup(name) original_count = len(catalogs) - catalogs = [c for c in catalogs if isinstance(c, dict) and c.get("name") != name] + catalogs = [ + c + for c in catalogs + if isinstance(c, dict) + and str(c.get("name", "")).strip() != name + ] if len(catalogs) == original_count: console.print(f"[red]Error:[/red] Catalog '{safe_name}' not found.") diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 1bd5728a2a..8aa96475d7 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -1035,6 +1035,7 @@ def preset_catalog_remove( project_root = _require_specify_project() specify_dir = project_root / ".specify" + name = name.strip() config_path = specify_dir / "preset-catalogs.yml" if not config_path.exists(): @@ -1060,7 +1061,12 @@ def preset_catalog_remove( safe_name = _escape_markup(str(name)) original_count = len(catalogs) - catalogs = [c for c in catalogs if isinstance(c, dict) and c.get("name") != name] + catalogs = [ + c + for c in catalogs + if isinstance(c, dict) + and str(c.get("name", "")).strip() != name + ] if len(catalogs) == original_count: console.print(f"[red]Error:[/red] Catalog '{safe_name}' not found.") diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 564db2bde4..16e8bbd939 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7912,6 +7912,44 @@ def test_catalog_add_same_name_with_blank_url_conflicts( assert "different settings" in result.output assert config_path.read_bytes() == original + @pytest.mark.parametrize("requested_name", ["mine", " mine "]) + def test_catalog_remove_normalizes_name( + self, project_dir, monkeypatch, requested_name + ): + from typer.testing import CliRunner + from specify_cli import app + + config_path = project_dir / ".specify" / "extension-catalogs.yml" + config_path.write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "name": " mine ", + "url": "https://example.com/catalog.json", + "priority": 10, + "install_allowed": False, + } + ] + } + ), + encoding="utf-8", + ) + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + ["extension", "catalog", "remove", requested_name], + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert "Removed catalog 'mine'" in result.output + assert yaml.safe_load(config_path.read_text(encoding="utf-8"))[ + "catalogs" + ] == [] + def test_catalog_add_escapes_config_saved_path_markup(self, tmp_path): """Catalog add's saved-path label should render literally under Rich.""" from typer.testing import CliRunner diff --git a/tests/test_presets.py b/tests/test_presets.py index 19c1132768..e85fffae7a 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -4278,6 +4278,44 @@ def test_catalog_remove_escapes_rich_markup(self, project_dir): assert result.exit_code == 0, result.output assert name in result.output + @pytest.mark.parametrize("requested_name", ["mine", " mine "]) + def test_catalog_remove_normalizes_name( + self, project_dir, monkeypatch, requested_name + ): + from typer.testing import CliRunner + from specify_cli import app + + config_path = project_dir / ".specify" / "preset-catalogs.yml" + config_path.write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "name": " mine ", + "url": "https://example.com/catalog.json", + "priority": 10, + "install_allowed": False, + } + ] + } + ), + encoding="utf-8", + ) + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + ["preset", "catalog", "remove", requested_name], + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert "Removed catalog 'mine'" in result.output + assert yaml.safe_load(config_path.read_text(encoding="utf-8"))[ + "catalogs" + ] == [] + def test_catalog_remove_escapes_markup_in_not_found_error(self, project_dir): """The not-found error path renders the name too.""" from typer.testing import CliRunner From 4d2b90414670301fc348073b67d257f7e13c7927 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 18 Sep 2026 15:57:24 -0500 Subject: [PATCH 09/11] fix: validate idempotent workflow catalog adds Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/catalog.py | 10 +++- tests/test_workflows.py | 71 ++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 8e3a2f6f98..6b49342aef 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -744,6 +744,9 @@ def add_catalog(self, url: str, name: str | None = None) -> str: requested_name = str(name).strip() if name is not None else "" for idx, cat in enumerate(catalogs): if isinstance(cat, dict) and str(cat.get("url", "")).strip() == url: + # A no-op must not bless a project config that the normal + # catalog-loading path would reject. + self._load_catalog_config(config_path) existing_name = str( cat.get("name", f"catalog-{idx + 1}") ).strip() @@ -776,7 +779,7 @@ def _coerce_priority(value: Any) -> int: ) catalogs.append( { - "name": name or f"catalog-{len(catalogs) + 1}", + "name": requested_name or f"catalog-{len(catalogs) + 1}", "url": url, "priority": max_priority + 1, "install_allowed": True, @@ -1445,6 +1448,9 @@ def add_catalog(self, url: str, name: str | None = None) -> str: requested_name = str(name).strip() if name is not None else "" for idx, cat in enumerate(catalogs): if isinstance(cat, dict) and str(cat.get("url", "")).strip() == url: + # A no-op must not bless a project config that the normal + # catalog-loading path would reject. + self._load_catalog_config(config_path) existing_name = str( cat.get("name", f"catalog-{idx + 1}") ).strip() @@ -1476,7 +1482,7 @@ def _coerce_priority(value: Any) -> int: ) catalogs.append( { - "name": name or f"catalog-{len(catalogs) + 1}", + "name": requested_name or f"catalog-{len(catalogs) + 1}", "url": url, "priority": max_priority + 1, "install_allowed": True, diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 0a2043209b..95c898d5bf 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -9986,6 +9986,77 @@ def make_module_name(type_key: str) -> str: (["workflow", "step", "catalog", "add"], "step-catalogs.yml"), ]) class TestWorkflowCatalogAddCLI: + @pytest.mark.parametrize( + ("requested_name", "stored_name"), + [(" mine ", "mine"), (" ", "catalog-1")], + ) + def test_add_catalog_normalizes_name( + self, + project_dir, + monkeypatch, + command, + config_filename, + requested_name, + stored_name, + ): + from typer.testing import CliRunner + from specify_cli import app + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [ + *command, + "https://example.com/catalog.json", + "--name", + requested_name, + ], + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + config_path = project_dir / ".specify" / config_filename + data = yaml.safe_load(config_path.read_text(encoding="utf-8")) + assert data["catalogs"][0]["name"] == stored_name + + @pytest.mark.parametrize("priority", ["not-a-number", True]) + def test_add_catalog_duplicate_validates_existing_config( + self, project_dir, monkeypatch, command, config_filename, priority + ): + from typer.testing import CliRunner + from specify_cli import app + + url = "https://example.com/catalog.json" + config_path = project_dir / ".specify" / config_filename + config_path.write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "name": "mine", + "url": url, + "priority": priority, + } + ] + } + ), + encoding="utf-8", + ) + original = config_path.read_bytes() + + with monkeypatch.context() as scoped: + scoped.chdir(project_dir) + result = CliRunner().invoke( + app, + [*command, url, "--name", "mine"], + catch_exceptions=False, + ) + + assert result.exit_code == 1, result.output + assert "Invalid priority" in result.output + assert config_path.read_bytes() == original + def test_add_catalog_escapes_markup_in_success_output( self, project_dir, monkeypatch, command, config_filename ): From 66f84629caae655dcba834fb7ad7cc55559d4700 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 18 Sep 2026 16:09:33 -0500 Subject: [PATCH 10/11] fix: minimize catalog add idempotency Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/reference/bundles.md | 4 - docs/reference/extensions.md | 6 - docs/reference/integrations.md | 2 - docs/reference/presets.md | 6 - docs/reference/workflows.md | 18 - .../bundler/commands_impl/catalog_config.py | 43 +- src/specify_cli/commands/bundle/__init__.py | 16 +- src/specify_cli/extensions/_commands.py | 101 +---- .../integrations/_query_commands.py | 12 +- src/specify_cli/integrations/catalog.py | 67 +-- src/specify_cli/presets/_commands.py | 104 +---- src/specify_cli/workflows/_commands.py | 22 +- src/specify_cli/workflows/catalog.py | 75 +--- tests/contract/test_bundle_cli.py | 60 --- tests/integrations/test_cli.py | 29 +- .../integrations/test_integration_catalog.py | 144 +------ tests/test_extensions.py | 366 +--------------- tests/test_presets.py | 393 +----------------- tests/test_workflows.py | 298 +------------ tests/unit/test_bundler_catalog_config.py | 153 ++----- 20 files changed, 200 insertions(+), 1719 deletions(-) diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index f1a727f577..bb2a6aa7c7 100644 --- a/docs/reference/bundles.md +++ b/docs/reference/bundles.md @@ -175,10 +175,6 @@ specify bundle catalog add Registers a project-scoped catalog source and persists it. -Adding a source is idempotent (identity is the source **id or url**): re-running `catalog add` with the same id/url and identical `--policy`/`--priority` is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding a matching id/url with *different* settings is rejected as a conflict rather than silently overwriting the existing source — remove it first to change it. - -Surrounding whitespace in source ids and URLs is ignored when matching identities and comparing settings. No-ops and conflicts leave the existing configuration unchanged; they do not rewrite stored values to normalize them. - ### Remove a Catalog Source ```bash diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index f5c024a99c..22357ccea0 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -156,12 +156,6 @@ specify extension catalog add Adds a catalog to the project's `.specify/extension-catalogs.yml`. -Adding a catalog is idempotent (identity is the catalog **name**): re-running `catalog add` with the same name and identical settings is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same name with *different* settings is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. - -Surrounding whitespace in catalog names and URLs is ignored when comparing entries and stripped from newly added entries. A no-op leaves the existing configuration unchanged. - -Stored priorities may use numeric strings, but YAML booleans (`true`/`false`) are invalid and are never equivalent to integer priorities (`1`/`0`). Re-adding a matching catalog with an invalid stored priority reports a conflict. - ### Remove a Catalog ```bash diff --git a/docs/reference/integrations.md b/docs/reference/integrations.md index 87ebea3a22..32310cf81f 100644 --- a/docs/reference/integrations.md +++ b/docs/reference/integrations.md @@ -233,8 +233,6 @@ specify integration catalog add Adds a custom catalog URL to the project's `.specify/integration-catalogs.yml`. The URL must use HTTPS (except `http://localhost`, `http://127.0.0.1`, or `http://[::1]` for local testing). -Adding a catalog is idempotent (identity is the catalog **URL**): re-running `catalog add` with the same URL and the same (or no) `--name` is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same URL with a *different* `--name` is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. - ### Remove a Catalog ```bash diff --git a/docs/reference/presets.md b/docs/reference/presets.md index d96bc3ee85..0723200d67 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -123,12 +123,6 @@ specify preset catalog add Adds a catalog to the project's `.specify/preset-catalogs.yml`. -Adding a catalog is idempotent (identity is the catalog **name**): re-running `catalog add` with the same name and identical settings is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same name with *different* settings is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. - -Surrounding whitespace in catalog URLs is ignored when comparing entries and stripped from newly added entries. A no-op leaves the existing configuration unchanged. - -Stored priorities may use numeric strings, but YAML booleans (`true`/`false`) are invalid and are never equivalent to integer priorities (`1`/`0`). Re-adding a matching catalog with an invalid stored priority reports a conflict. - ### Remove a Catalog ```bash diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 2961cb7ca9..a547a10e42 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -405,10 +405,6 @@ specify workflow catalog add Adds a custom catalog URL to the project's `.specify/workflow-catalogs.yml`. -Adding a catalog is idempotent (identity is the catalog **URL**): re-running `catalog add` with the same URL and the same (or no) `--name` is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same URL with a *different* `--name` is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. - -Surrounding whitespace in catalog URLs is ignored when comparing entries and stripped from newly added entries. A no-op leaves the existing configuration unchanged. - ### Remove a Catalog ```bash @@ -426,20 +422,6 @@ Catalogs are resolved in this order (first match wins): 3. **User config** — `~/.specify/workflow-catalogs.yml` 4. **Built-in defaults** — official catalog + community catalog -### Step Catalogs - -Custom step types have a separate catalog stack: - -```bash -specify workflow step catalog list -specify workflow step catalog add [--name ] -specify workflow step catalog remove -``` - -`step catalog add` writes to `.specify/step-catalogs.yml`. Like workflow catalogs, step catalogs use the **URL** as their identity, ignoring surrounding whitespace. Adding the same URL with the same (or no) `--name` is a successful no-op (exit code 0) that leaves the configuration unchanged. A different `--name` for that URL is a conflict (exit code 1); remove the existing entry first to change it. New entries store the URL without surrounding whitespace. - -`step catalog list` shows the active sources, and `step catalog remove` removes a project entry by its index. Step catalog resolution uses `SPECKIT_STEP_CATALOG_URL`, then the project config, then `~/.specify/step-catalogs.yml`, then built-in defaults. - ## Workflow Definition Workflows are defined in YAML files. Here is the built-in **Full SDD Cycle** workflow that ships with Spec Kit: diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index 53a2852695..50a13ae5dc 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -139,7 +139,7 @@ def add_source( policy: str, priority: int, source_id: str | None = None, -) -> tuple[CatalogSource, str]: +) -> CatalogSource: url = url.strip() if not url: raise BundlerError("A catalog url is required.") @@ -183,51 +183,26 @@ def add_source( url = _canonicalize_url(url) install_policy = InstallPolicy.parse(policy) - requested_id = source_id.strip() if source_id is not None else "" - resolved_id = requested_id or _derive_id(url) + resolved_id = (source_id or _derive_id(url)).strip() catalogs = _read(project_root) - desired = { + entry = { "id": resolved_id, "url": url, "priority": int(priority), "install_policy": install_policy.value, } for existing in catalogs: - if ( - str(existing.get("id", "")).strip() == resolved_id - or str(existing.get("url", "")).strip() == url - ): - # Idempotent add (#4505): identity is the source id or url. A rerun - # requesting the same settings is a successful no-op; differing - # settings are a conflict rather than a silent overwrite. - # - # Parse the matching entry through CatalogSource.from_dict first: - # _read() only checks that entries are mappings, so a hand-edited - # entry may carry a non-integer priority. Normalizing here surfaces - # that as a clean BundlerError (matching catalog parsing) instead of - # leaking int()'s ValueError/OverflowError past the CLI's - # `except BundlerError`, and lets supported representations (e.g. a - # string priority) compare equal to the requested defaults. - existing_source = CatalogSource.from_dict(dict(existing), Scope.PROJECT) - if ( - ( - existing_source.id == resolved_id - or (not requested_id and existing_source.url == url) - ) - and existing_source.url == url - and existing_source.priority == desired["priority"] - and existing_source.install_policy.value == desired["install_policy"] - ): - return existing_source, "unchanged" + if existing.get("id") == resolved_id or existing.get("url") == url: + if existing == entry: + return CatalogSource.from_dict(existing, Scope.PROJECT) raise BundlerError( - f"Catalog source '{resolved_id}' (or url) already exists in this " - "project with different settings. Remove it first to change it." + f"Catalog source '{resolved_id}' (or url) already exists in this project." ) - catalogs.append(desired) + catalogs.append(entry) _write(project_root, catalogs) - return CatalogSource.from_dict(desired, Scope.PROJECT), "added" + return CatalogSource.from_dict(entry, Scope.PROJECT) def remove_source(project_root: Path, id_or_url: str) -> str: diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index f37b23b605..b809afba80 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -672,21 +672,15 @@ def catalog_add( project_root = require_project_root() from ...bundler.commands_impl.catalog_config import add_source - source, status = add_source(project_root, url, policy=policy, priority=priority, source_id=source_id) + source = add_source(project_root, url, policy=policy, priority=priority, source_id=source_id) except BundlerError as exc: _fail(str(exc)) return - if status == "unchanged": - console.print( - f"[green]✓[/green] Catalog '{_escape_markup(str(source.id))}' is already " - f"configured (priority {source.priority}, {source.install_policy.value})." - ) - else: - console.print( - f"[green]✓[/green] Added catalog '{_escape_markup(str(source.id))}' " - f"(priority {source.priority}, {source.install_policy.value})." - ) + console.print( + f"[green]✓[/green] Added catalog '{_escape_markup(str(source.id))}' " + f"(priority {source.priority}, {source.install_policy.value})." + ) @bundle_catalog_app.command("remove") diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 74c40a162c..97a9f56761 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -327,36 +327,6 @@ def install_extension_from_url( pass -def _normalize_catalog_priority(value: object) -> int | None: - """Normalize a stored catalog priority the way the catalog reader does. - - The reader (``specify_cli/catalogs.py``) accepts integer-string priorities - like ``"10"`` but rejects bools. Mirror that here so an equivalent rerun - whose persisted priority is a supported string representation is still a - no-op rather than a false conflict (#4505). A value that cannot be - normalized returns ``None`` so it cannot compare equal to an integer. - """ - if isinstance(value, bool): - return None - try: - return int(value) - except (TypeError, ValueError, OverflowError): - return None - - -def _normalize_catalog_install_allowed(value: object) -> bool: - """Normalize a stored ``install_allowed`` the way the catalog reader does. - - The reader treats the strings ``"true"``/``"yes"``/``"1"`` (case- and - whitespace-insensitive) as truthy; everything else falls back to ``bool``. - Comparing raw values instead would report ``install_allowed: "false"`` as a - conflict because ``bool("false")`` is ``True``. - """ - if isinstance(value, str): - return value.strip().lower() in ("true", "yes", "1") - return bool(value) - - def _load_catalog_command_config(project_root: Path, config_path: Path) -> dict: """Load extension catalog CLI config with user-facing shape errors.""" try: @@ -645,11 +615,6 @@ def catalog_add( project_root = _require_specify_project() specify_dir = project_root / ".specify" - url = url.strip() - name = name.strip() - if not name: - console.print("[red]Error:[/red] Catalog name must be non-empty.") - raise typer.Exit(1) # Validate URL tmp_catalog = ExtensionCatalog(project_root) @@ -675,56 +640,24 @@ def catalog_add( safe_name = _escape_markup(name) safe_url = _escape_markup(url) - # Idempotent add (#4505): a rerun that requests an identical entry is a - # successful no-op so the same `catalog add` can live in a re-runnable - # workflow without failing. A same-name entry whose settings differ is - # still a conflict — we refuse to silently change priority/install - # permissions and ask the user to remove it first. - valid_catalog_count = 0 - for idx, existing in enumerate(catalogs): - if not isinstance(existing, dict): - continue - existing_url = str(existing.get("url", "")).strip() - raw_existing_name = existing.get("name") - existing_name = ( - str(raw_existing_name).strip() - if raw_existing_name is not None - else "" - ) - if existing_url: - valid_catalog_count += 1 - if not existing_name and existing_url: - existing_name = f"catalog-{valid_catalog_count}" - if existing_name == name: - if ( - existing_url == url - and _normalize_catalog_priority( - existing.get("priority", idx + 1) - ) == priority - and _normalize_catalog_install_allowed( - existing.get("install_allowed", False) - ) == install_allowed - and str(existing.get("description", "")) == description - ): - console.print( - f"[green]✓[/green] Catalog '[bold]{safe_name}[/bold]' is already " - "configured with these settings; nothing to do." - ) - return - console.print( - f"[red]Error:[/red] A catalog named '{safe_name}' already exists with " - "different settings." - ) - console.print("Use 'specify extension catalog remove' first, or choose a different name.") - raise typer.Exit(1) - - catalogs.append({ + entry = { "name": name, "url": url, "priority": priority, "install_allowed": install_allowed, "description": description, - }) + } + + # Check for duplicate name + for existing in catalogs: + if isinstance(existing, dict) and existing.get("name") == name: + if existing == entry: + return + console.print(f"[yellow]Warning:[/yellow] A catalog named '{safe_name}' already exists.") + console.print("Use 'specify extension catalog remove' first, or choose a different name.") + raise typer.Exit(1) + + catalogs.append(entry) config["catalogs"] = catalogs config_path.write_text(yaml.safe_dump(config, default_flow_style=False, sort_keys=False, allow_unicode=True), encoding="utf-8") @@ -744,7 +677,6 @@ def catalog_remove( """Remove a catalog from .specify/extension-catalogs.yml.""" project_root = _require_specify_project() specify_dir = project_root / ".specify" - name = name.strip() config_path = specify_dir / "extension-catalogs.yml" if not config_path.exists(): @@ -759,12 +691,7 @@ def catalog_remove( raise typer.Exit(1) safe_name = _escape_markup(name) original_count = len(catalogs) - catalogs = [ - c - for c in catalogs - if isinstance(c, dict) - and str(c.get("name", "")).strip() != name - ] + catalogs = [c for c in catalogs if isinstance(c, dict) and c.get("name") != name] if len(catalogs) == original_count: console.print(f"[red]Error:[/red] Catalog '{safe_name}' not found.") diff --git a/src/specify_cli/integrations/_query_commands.py b/src/specify_cli/integrations/_query_commands.py index 0426606a2f..0cd254879a 100644 --- a/src/specify_cli/integrations/_query_commands.py +++ b/src/specify_cli/integrations/_query_commands.py @@ -543,20 +543,14 @@ def integration_catalog_add( normalized_url = url.strip() try: - status = catalog.add_catalog(normalized_url, name) + catalog.add_catalog(normalized_url, name) except IntegrationCatalogError as exc: # Covers both URL validation (base class) and config-file validation # (IntegrationValidationError subclass). - console.print(f"[red]Error:[/red] {_rich_escape(str(exc))}") + console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) - safe_url = _rich_escape(normalized_url) - if status == "unchanged": - console.print( - f"[green]✓[/green] Catalog source already configured: {safe_url}" - ) - else: - console.print(f"[green]✓[/green] Catalog source added: {safe_url}") + console.print(f"[green]✓[/green] Catalog source added: {normalized_url}") @integration_catalog_app.command("remove") diff --git a/src/specify_cli/integrations/catalog.py b/src/specify_cli/integrations/catalog.py index f787461586..6a49409f56 100644 --- a/src/specify_cli/integrations/catalog.py +++ b/src/specify_cli/integrations/catalog.py @@ -390,20 +390,14 @@ def get_project_catalog_configs(self) -> Optional[List[Dict[str, Any]]]: for e in entries ] - def add_catalog(self, url: str, name: Optional[str] = None) -> str: + def add_catalog(self, url: str, name: Optional[str] = None) -> None: """Add a catalog source to the project-level config file. The URL is normalized (whitespace stripped) and validated before being - written. Identity for an integration catalog is the (normalized) URL. - Adding a URL that is already configured is idempotent (#4505): a rerun - that requests the same name (or no explicit name) is a successful - no-op, while a rerun that requests a *different* name is rejected as a - conflict rather than silently overwriting the stored entry. Priority is - derived as ``max(existing) + 1`` so a newly added entry sorts last in - the resolution order unless the user edits the file manually. - - Returns ``"added"`` when a new entry is written, or ``"unchanged"`` - when an equivalent entry already existed. + written. Duplicate URLs are rejected, including near-duplicates that + differ only by surrounding whitespace. Priority is derived as + ``max(existing) + 1`` so the new entry sorts last in the resolution + order unless the user edits the file manually. """ url = url.strip() if not url: @@ -438,10 +432,9 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> str: # Validate each existing entry before mutating anything. Fail fast so # we don't silently preserve a corrupt sibling entry or derive a new # priority from a bogus value. - requested_name = str(name).strip() if name is not None else "" + normalized_name = str(name).strip() if name is not None else "" existing_priorities: List[int] = [] valid_catalog_count = 0 - matching_catalog: tuple[bool, str] | None = None for idx, cat in enumerate(catalogs): if not isinstance(cat, dict): raise IntegrationValidationError( @@ -459,6 +452,17 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> str: raise IntegrationValidationError( f"Invalid catalog entry at index {idx} in {config_path}: {exc}" ) from exc + if existing_url == url: + generated_name = f"catalog-{valid_catalog_count + 1}" + requested_name = normalized_name or generated_name + existing_name = str(cat.get("name", generated_name)).strip() + if existing_name == requested_name: + self._load_catalog_config(config_path) + return + raise IntegrationValidationError( + f"Catalog URL already configured: {url}" + ) + valid_catalog_count += 1 if "priority" in cat: raw_priority = cat.get("priority") if isinstance(raw_priority, bool): @@ -470,43 +474,19 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> str: try: normalized_priority = int(raw_priority) except (TypeError, ValueError, OverflowError): + # OverflowError: int(float("inf")) — a ``priority: .inf``. raise IntegrationValidationError( f"Invalid catalog entry at index {idx} in {config_path}: " f"'priority' must be an integer, got " f"{raw_priority!r}." ) from None + existing_priorities.append(normalized_priority) else: - # Match `_load_catalog_config()`'s defaulting rule. - normalized_priority = idx + 1 - existing_priorities.append(normalized_priority) - valid_catalog_count += 1 - - raw_existing_name = cat.get("name") - existing_name = ( - str(raw_existing_name).strip() - if raw_existing_name is not None - else "" - ) - if not existing_name: - existing_name = f"catalog-{valid_catalog_count}" - if existing_url == url and matching_catalog is None: - matching_catalog = ( - not requested_name or requested_name == existing_name, - existing_name, - ) - - if matching_catalog is not None: - names_match, existing_name = matching_catalog - if names_match: - return "unchanged" - raise IntegrationValidationError( - f"Catalog URL already configured with a different name " - f"('{existing_name}'): {url}. Remove it first or pass " - f"--name '{existing_name}'." - ) - + # Match `_load_catalog_config()`'s defaulting rule so the new + # entry still sorts after implicit-priority siblings. + existing_priorities.append(idx + 1) + max_priority = max(existing_priorities, default=0) max_priority = max(existing_priorities, default=0) - normalized_name = str(name).strip() if name is not None else "" generated_name = f"catalog-{valid_catalog_count + 1}" catalogs.append( { @@ -528,7 +508,6 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> str: sort_keys=False, allow_unicode=True, ) - return "added" def remove_catalog(self, index: int) -> str: """Remove a catalog source by 0-based index. diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 8aa96475d7..beeed45000 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -47,37 +47,6 @@ preset_app.add_typer(preset_catalog_app, name="catalog") -def _normalize_catalog_priority(value: object) -> int | None: - """Normalize a stored catalog priority the way the preset reader does. - - The preset reader (``specify_cli/presets/__init__.py``) accepts - integer-string priorities like ``"10"`` but rejects bools. Mirror that here - so an equivalent rerun whose persisted priority is a supported string - representation is still a no-op rather than a false conflict (#4505). A - value that cannot be normalized returns ``None`` so it cannot compare equal - to an integer. - """ - if isinstance(value, bool): - return None - try: - return int(value) - except (TypeError, ValueError, OverflowError): - return None - - -def _normalize_catalog_install_allowed(value: object) -> bool: - """Normalize a stored ``install_allowed`` the way the preset reader does. - - The reader treats the strings ``"true"``/``"yes"``/``"1"`` (case- and - whitespace-insensitive) as truthy; everything else falls back to ``bool``. - Comparing raw values instead would report ``install_allowed: "false"`` as a - conflict because ``bool("false")`` is ``True``. - """ - if isinstance(value, str): - return value.strip().lower() in ("true", "yes", "1") - return bool(value) - - def _warn_unmet_extension_dependencies(manager, manifest) -> None: """Warn when a preset's declared extension dependencies are unsatisfied. @@ -922,11 +891,6 @@ def preset_catalog_add( project_root = _require_specify_project() specify_dir = project_root / ".specify" - url = url.strip() - name = name.strip() - if not name: - console.print("[red]Error:[/red] Catalog name must be non-empty.") - raise typer.Exit(1) # Validate URL tmp_catalog = PresetCatalog(project_root) @@ -959,61 +923,29 @@ def preset_catalog_add( console.print("[red]Error:[/red] Invalid catalog config: 'catalogs' must be a list.") raise typer.Exit(1) - # Only rendering is escaped — the unescaped values get persisted and + # Only rendering is escaped — the raw values are what get persisted and # compared below, so a name containing markup still round-trips exactly. safe_name = _escape_markup(str(name)) safe_url = _escape_markup(str(url)) - # Idempotent add (#4505): a rerun that requests an identical entry is a - # successful no-op so the same `catalog add` can live in a re-runnable - # workflow without failing. A same-name entry whose settings differ is - # still a conflict — we refuse to silently change priority/install - # permissions and ask the user to remove it first. - valid_catalog_count = 0 - for idx, existing in enumerate(catalogs): - if not isinstance(existing, dict): - continue - existing_url = str(existing.get("url", "")).strip() - raw_existing_name = existing.get("name") - existing_name = ( - str(raw_existing_name).strip() - if raw_existing_name is not None - else "" - ) - if existing_url: - valid_catalog_count += 1 - if not existing_name and existing_url: - existing_name = f"catalog-{valid_catalog_count}" - if existing_name == name: - if ( - existing_url == url - and _normalize_catalog_priority( - existing.get("priority", idx + 1) - ) == priority - and _normalize_catalog_install_allowed( - existing.get("install_allowed", False) - ) == install_allowed - and str(existing.get("description", "")) == description - ): - console.print( - f"[green]✓[/green] Catalog '[bold]{safe_name}[/bold]' is already " - "configured with these settings; nothing to do." - ) - return - console.print( - f"[red]Error:[/red] A catalog named '{safe_name}' already exists with " - "different settings." - ) - console.print("Use 'specify preset catalog remove' first, or choose a different name.") - raise typer.Exit(1) - - catalogs.append({ + entry = { "name": name, "url": url, "priority": priority, "install_allowed": install_allowed, "description": description, - }) + } + + # Check for duplicate name + for existing in catalogs: + if isinstance(existing, dict) and existing.get("name") == name: + if existing == entry: + return + console.print(f"[yellow]Warning:[/yellow] A catalog named '{safe_name}' already exists.") + console.print("Use 'specify preset catalog remove' first, or choose a different name.") + raise typer.Exit(1) + + catalogs.append(entry) config["catalogs"] = catalogs config_path.write_text(yaml.safe_dump(config, default_flow_style=False, sort_keys=False, allow_unicode=True), encoding="utf-8") @@ -1035,7 +967,6 @@ def preset_catalog_remove( project_root = _require_specify_project() specify_dir = project_root / ".specify" - name = name.strip() config_path = specify_dir / "preset-catalogs.yml" if not config_path.exists(): @@ -1061,12 +992,7 @@ def preset_catalog_remove( safe_name = _escape_markup(str(name)) original_count = len(catalogs) - catalogs = [ - c - for c in catalogs - if isinstance(c, dict) - and str(c.get("name", "")).strip() != name - ] + catalogs = [c for c in catalogs if isinstance(c, dict) and c.get("name") != name] if len(catalogs) == original_count: console.print(f"[red]Error:[/red] Catalog '{safe_name}' not found.") diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 2ff4f914a0..7691066714 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3025,16 +3025,12 @@ def workflow_catalog_add( project_root = _require_specify_project() catalog = WorkflowCatalog(project_root) try: - status = catalog.add_catalog(url, name) + catalog.add_catalog(url, name) except WorkflowValidationError as exc: - console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) - safe_url = _escape_markup(url) - if status == "unchanged": - console.print(f"[green]✓[/green] Catalog source already configured: {safe_url}") - else: - console.print(f"[green]✓[/green] Catalog source added: {safe_url}") + console.print(f"[green]✓[/green] Catalog source added: {url}") @workflow_catalog_app.command("remove") @@ -3775,18 +3771,12 @@ def workflow_step_catalog_add( catalog = StepCatalog(project_root) try: - status = catalog.add_catalog(url, name) + catalog.add_catalog(url, name) except StepValidationError as exc: - console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) - safe_url = _escape_markup(url) - if status == "unchanged": - console.print( - f"[green]✓[/green] Step catalog source already configured: {safe_url}" - ) - else: - console.print(f"[green]✓[/green] Step catalog source added: {safe_url}") + console.print(f"[green]✓[/green] Step catalog source added: {url}") @workflow_step_catalog_app.command("remove") diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 6b49342aef..ca56d93e8e 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -705,16 +705,8 @@ def get_catalog_configs(self) -> list[dict[str, Any]]: for e in entries ] - def add_catalog(self, url: str, name: str | None = None) -> str: - """Add a catalog source to the project-level config. - - Identity is the URL with surrounding whitespace stripped. Adding an - existing URL is idempotent (#4505): requesting the same name (or no - explicit name) returns ``"unchanged"``; a rerun requesting a - different name is rejected as a conflict. Returns ``"added"`` when a - new entry is written. - """ - url = url.strip() + def add_catalog(self, url: str, name: str | None = None) -> None: + """Add a catalog source to the project-level config.""" self._validate_catalog_url(url) config_path = self.project_root / ".specify" / "workflow-catalogs.yml" @@ -739,23 +731,16 @@ def add_catalog(self, url: str, name: str | None = None) -> str: raise WorkflowValidationError( "Catalog config 'catalogs' must be a list." ) - # Idempotent add (#4505): identity is the URL. A rerun requesting the - # same name (or no explicit name) is a no-op; a different name conflicts. - requested_name = str(name).strip() if name is not None else "" + # Check for duplicate URL (guard against non-dict entries) for idx, cat in enumerate(catalogs): - if isinstance(cat, dict) and str(cat.get("url", "")).strip() == url: - # A no-op must not bless a project config that the normal - # catalog-loading path would reject. - self._load_catalog_config(config_path) - existing_name = str( - cat.get("name", f"catalog-{idx + 1}") - ).strip() - if not requested_name or requested_name == existing_name: - return "unchanged" + if isinstance(cat, dict) and cat.get("url") == url: + generated_name = f"catalog-{idx + 1}" + requested_name = name or generated_name + if cat.get("name", generated_name) == requested_name: + self._load_catalog_config(config_path) + return raise WorkflowValidationError( - f"Catalog URL already configured with a different name " - f"('{existing_name}'): {url}. Remove it first or pass " - f"--name '{existing_name}'." + f"Catalog URL already configured: {url}" ) # Derive priority from the highest existing priority + 1. @@ -779,7 +764,7 @@ def _coerce_priority(value: Any) -> int: ) catalogs.append( { - "name": requested_name or f"catalog-{len(catalogs) + 1}", + "name": name or f"catalog-{len(catalogs) + 1}", "url": url, "priority": max_priority + 1, "install_allowed": True, @@ -796,7 +781,6 @@ def _coerce_priority(value: Any) -> int: raise WorkflowValidationError( f"Failed to write catalog config {config_path}: {exc}" ) from exc - return "added" def remove_catalog(self, index: int) -> str: """Remove a catalog source by index (0-based). Returns the removed name.""" @@ -1409,16 +1393,8 @@ def get_catalog_configs(self) -> list[dict[str, Any]]: for e in entries ] - def add_catalog(self, url: str, name: str | None = None) -> str: - """Add a catalog source to the project-level config. - - Identity is the URL with surrounding whitespace stripped. Adding an - existing URL is idempotent (#4505): requesting the same name (or no - explicit name) returns ``"unchanged"``; a rerun requesting a - different name is rejected as a conflict. Returns ``"added"`` when a - new entry is written. - """ - url = url.strip() + def add_catalog(self, url: str, name: str | None = None) -> None: + """Add a catalog source to the project-level config.""" self._validate_catalog_url(url) config_path = self.project_root / ".specify" / "step-catalogs.yml" @@ -1443,23 +1419,15 @@ def add_catalog(self, url: str, name: str | None = None) -> str: raise StepValidationError( "Catalog config 'catalogs' must be a list." ) - # Idempotent add (#4505): identity is the URL. A rerun requesting the - # same name (or no explicit name) is a no-op; a different name conflicts. - requested_name = str(name).strip() if name is not None else "" for idx, cat in enumerate(catalogs): - if isinstance(cat, dict) and str(cat.get("url", "")).strip() == url: - # A no-op must not bless a project config that the normal - # catalog-loading path would reject. - self._load_catalog_config(config_path) - existing_name = str( - cat.get("name", f"catalog-{idx + 1}") - ).strip() - if not requested_name or requested_name == existing_name: - return "unchanged" + if isinstance(cat, dict) and cat.get("url") == url: + generated_name = f"catalog-{idx + 1}" + requested_name = name or generated_name + if cat.get("name", generated_name) == requested_name: + self._load_catalog_config(config_path) + return raise StepValidationError( - f"Catalog URL already configured with a different name " - f"('{existing_name}'): {url}. Remove it first or pass " - f"--name '{existing_name}'." + f"Catalog URL already configured: {url}" ) # Coerce existing priorities to int with a safe fallback so a user-edited @@ -1482,7 +1450,7 @@ def _coerce_priority(value: Any) -> int: ) catalogs.append( { - "name": requested_name or f"catalog-{len(catalogs) + 1}", + "name": name or f"catalog-{len(catalogs) + 1}", "url": url, "priority": max_priority + 1, "install_allowed": True, @@ -1501,7 +1469,6 @@ def _coerce_priority(value: Any) -> int: raise StepValidationError( f"Failed to write catalog config {config_path}: {exc}" ) from exc - return "added" def remove_catalog(self, index: int) -> str: """Remove a catalog source by index (0-based). Returns the removed name.""" diff --git a/tests/contract/test_bundle_cli.py b/tests/contract/test_bundle_cli.py index 6752c9161d..6db4dab769 100644 --- a/tests/contract/test_bundle_cli.py +++ b/tests/contract/test_bundle_cli.py @@ -211,66 +211,6 @@ def test_catalog_add_and_remove(project: Path): assert removed.exit_code == 0 -def test_catalog_add_duplicate_is_idempotent(project: Path): - catalog = project / "local-catalog.json" - write_catalog_file(catalog, {"demo": catalog_entry_dict("demo")}) - - first = runner.invoke( - app, - ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "10"], - ) - assert first.exit_code == 0, first.output - second = runner.invoke( - app, - ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "10"], - ) - assert second.exit_code == 0, second.output - assert "already" in second.output - - -@pytest.mark.parametrize("priority,exit_code", [(10, 0), (20, 1)]) -def test_catalog_add_normalizes_stored_identity(project: Path, priority, exit_code): - config_path = project / ".specify" / "bundle-catalogs.yml" - config_path.write_text(yaml.safe_dump({ - "schema_version": "1.0", - "catalogs": [{ - "id": " \tlocal\t ", - "url": " \thttps://example.com/catalog.json\t ", - "priority": 10, - "install_policy": "install-allowed", - }], - }), encoding="utf-8") - original = config_path.read_bytes() - modified_at = config_path.stat().st_mtime_ns - - result = runner.invoke(app, [ - "bundle", "catalog", "add", "https://example.com/catalog.json", - "--id", "local", "--policy", "install-allowed", "--priority", str(priority), - ], catch_exceptions=False) - - assert result.exit_code == exit_code, result.output - assert ("already" if exit_code == 0 else "different settings") in result.output - assert config_path.read_bytes() == original - assert config_path.stat().st_mtime_ns == modified_at - - -def test_catalog_add_duplicate_different_settings_conflicts(project: Path): - catalog = project / "local-catalog.json" - write_catalog_file(catalog, {"demo": catalog_entry_dict("demo")}) - - first = runner.invoke( - app, - ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "10"], - ) - assert first.exit_code == 0, first.output - second = runner.invoke( - app, - ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "20"], - ) - assert second.exit_code == 1 - assert "different settings" in second.output - - def test_catalog_remove_builtin_is_refused(project: Path): result = runner.invoke(app, ["bundle", "catalog", "remove", "default"]) assert result.exit_code == 1 diff --git a/tests/integrations/test_cli.py b/tests/integrations/test_cli.py index e2aa7f448c..2beb411a2a 100644 --- a/tests/integrations/test_cli.py +++ b/tests/integrations/test_cli.py @@ -2539,7 +2539,7 @@ def test_catalog_add_rejects_invalid_url(self, tmp_path, monkeypatch): assert result.exit_code == 1 assert "HTTPS" in result.output - def test_catalog_add_duplicate_is_idempotent(self, tmp_path, monkeypatch): + def test_catalog_add_rejects_duplicate(self, tmp_path, monkeypatch): project = self._make_project(tmp_path) url = "https://dup.example.com/catalog.json" first = self._invoke( @@ -2549,33 +2549,8 @@ def test_catalog_add_duplicate_is_idempotent(self, tmp_path, monkeypatch): second = self._invoke( ["integration", "catalog", "add", url], project ) - assert second.exit_code == 0, second.output - assert "already configured" in second.output - - def test_catalog_add_duplicate_different_name_conflicts(self, tmp_path, monkeypatch): - project = self._make_project(tmp_path) - url = "https://dup.example.com/catalog.json" - first = self._invoke( - ["integration", "catalog", "add", url, "--name", "first"], project - ) - assert first.exit_code == 0, first.output - second = self._invoke( - ["integration", "catalog", "add", url, "--name", "second"], project - ) assert second.exit_code == 1 - assert "different name" in second.output - - def test_catalog_add_escapes_markup_in_success_output(self, tmp_path, monkeypatch): - project = self._make_project(tmp_path) - url = "https://dup.example.com/[/red]/catalog.json" - - first = self._invoke(["integration", "catalog", "add", url], project) - assert first.exit_code == 0, first.output - assert url in first.output - - second = self._invoke(["integration", "catalog", "add", url], project) - assert second.exit_code == 0, second.output - assert url in second.output + assert "already configured" in second.output def test_catalog_remove_out_of_range(self, tmp_path, monkeypatch): project = self._make_project(tmp_path) diff --git a/tests/integrations/test_integration_catalog.py b/tests/integrations/test_integration_catalog.py index e5fe0c785c..5222eb95dc 100644 --- a/tests/integrations/test_integration_catalog.py +++ b/tests/integrations/test_integration_catalog.py @@ -1168,137 +1168,19 @@ def test_add_catalog_normalizes_name(self, tmp_path, monkeypatch): entries = data["catalogs"] assert [e["name"] for e in entries] == ["mine", "catalog-2"] - def test_add_catalog_duplicate_url_is_idempotent_noop(self, tmp_path, monkeypatch): - """Re-adding the same URL (no explicit name) is a successful no-op (#4505).""" - self._isolate(tmp_path, monkeypatch) - cat = IntegrationCatalog(tmp_path) - assert cat.add_catalog("https://dup.example.com/catalog.json") == "added" - assert cat.add_catalog("https://dup.example.com/catalog.json") == "unchanged" - cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" - data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) - assert len(data["catalogs"]) == 1 - - def test_add_catalog_duplicate_url_different_name_conflicts(self, tmp_path, monkeypatch): - """Re-adding the same URL with a different name is rejected as a conflict (#4505).""" - self._isolate(tmp_path, monkeypatch) - cat = IntegrationCatalog(tmp_path) - cat.add_catalog("https://dup.example.com/catalog.json", name="first") - with pytest.raises(IntegrationValidationError, match="different name"): - cat.add_catalog("https://dup.example.com/catalog.json", name="second") - - def test_add_catalog_duplicate_url_same_name_is_idempotent_noop(self, tmp_path, monkeypatch): - """Re-adding the same URL with the *same* explicit name is a no-op (#4505).""" - self._isolate(tmp_path, monkeypatch) - cat = IntegrationCatalog(tmp_path) - assert cat.add_catalog("https://dup.example.com/catalog.json", name="mine") == "added" - assert cat.add_catalog("https://dup.example.com/catalog.json", name="mine") == "unchanged" - cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" - data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) - assert len(data["catalogs"]) == 1 - - def test_add_catalog_duplicate_url_validates_stored_priority( + def test_add_catalog_is_idempotent_for_identical_url_and_name( self, tmp_path, monkeypatch ): self._isolate(tmp_path, monkeypatch) + cat = IntegrationCatalog(tmp_path) + cat.add_catalog("https://dup.example.com/catalog.json") cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" - cfg_path.parent.mkdir(parents=True, exist_ok=True) - cfg_path.write_text( - yaml.safe_dump( - { - "catalogs": [ - { - "name": "mine", - "url": "https://dup.example.com/catalog.json", - "priority": "first", - } - ] - } - ), - encoding="utf-8", - ) - - with pytest.raises( - IntegrationValidationError, match="'priority' must be an integer" - ): - IntegrationCatalog(tmp_path).add_catalog( - "https://dup.example.com/catalog.json", - name="mine", - ) - - @pytest.mark.parametrize("stored_name", ["missing", None, "", " \t"]) - def test_add_catalog_duplicate_url_uses_reader_name_fallback( - self, tmp_path, monkeypatch, stored_name - ): - self._isolate(tmp_path, monkeypatch) - cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" - cfg_path.parent.mkdir(parents=True, exist_ok=True) - entry = { - "url": "https://dup.example.com/catalog.json", - "priority": 1, - } - if stored_name != "missing": - entry["name"] = stored_name - cfg_path.write_text( - yaml.safe_dump({"catalogs": [entry]}), - encoding="utf-8", - ) - - assert ( - IntegrationCatalog(tmp_path).add_catalog( - "https://dup.example.com/catalog.json", - name="catalog-1", - ) - == "unchanged" - ) - - @pytest.mark.parametrize( - ("invalid_entry", "match"), - [ - ( - { - "name": "bad-url", - "url": "http://example.com/catalog.json", - "priority": 2, - }, - "HTTPS", - ), - ( - { - "name": "bad-priority", - "url": "https://other.example.com/catalog.json", - "priority": "first", - }, - "'priority' must be an integer", - ), - ], - ) - def test_add_catalog_duplicate_url_validates_remaining_entries( - self, tmp_path, monkeypatch, invalid_entry, match - ): - self._isolate(tmp_path, monkeypatch) - cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" - cfg_path.parent.mkdir(parents=True, exist_ok=True) - cfg_path.write_text( - yaml.safe_dump( - { - "catalogs": [ - { - "name": "mine", - "url": "https://dup.example.com/catalog.json", - "priority": 1, - }, - invalid_entry, - ] - } - ), - encoding="utf-8", - ) + original = cfg_path.read_bytes() + cat.add_catalog("https://dup.example.com/catalog.json") + assert cfg_path.read_bytes() == original - with pytest.raises(IntegrationValidationError, match=match): - IntegrationCatalog(tmp_path).add_catalog( - "https://dup.example.com/catalog.json", - name="mine", - ) + with pytest.raises(IntegrationValidationError, match="already configured"): + cat.add_catalog("https://dup.example.com/catalog.json", name="different") def test_add_catalog_rejects_invalid_url(self, tmp_path, monkeypatch): self._isolate(tmp_path, monkeypatch) @@ -1627,15 +1509,13 @@ def test_add_catalog_strips_whitespace_in_url(self, tmp_path, monkeypatch): data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) assert data["catalogs"][0]["url"] == "https://a.example.com/catalog.json" - def test_add_catalog_whitespace_only_duplicate_is_noop(self, tmp_path, monkeypatch): - """A second add differing only by whitespace (no new name) is an idempotent no-op.""" + def test_add_catalog_rejects_whitespace_only_duplicate(self, tmp_path, monkeypatch): + """A second add with only whitespace differences must be rejected as a duplicate.""" self._isolate(tmp_path, monkeypatch) cat = IntegrationCatalog(tmp_path) cat.add_catalog("https://a.example.com/catalog.json", name="a") - assert cat.add_catalog(" https://a.example.com/catalog.json ") == "unchanged" - cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" - data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) - assert len(data["catalogs"]) == 1 + with pytest.raises(IntegrationValidationError, match="already configured"): + cat.add_catalog(" https://a.example.com/catalog.json ") def test_remove_catalog_wraps_unlink_oserror(self, tmp_path, monkeypatch): """An OSError from `Path.unlink` surfaces as IntegrationValidationError.""" diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 16e8bbd939..7d44c11d61 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7568,65 +7568,7 @@ def test_extensionignore_negation_pattern(self, temp_dir, valid_manifest_data): class TestExtensionAddCLI: """CLI integration tests for extension add command.""" - def test_catalog_add_rejects_empty_normalized_name(self, project_dir, monkeypatch): - from typer.testing import CliRunner - from specify_cli import app - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - [ - "extension", - "catalog", - "add", - "https://example.com/catalog.json", - "--name", - " \t", - ], - catch_exceptions=False, - ) - - assert result.exit_code == 1 - assert "must be non-empty" in result.output - assert not ( - project_dir / ".specify" / "extension-catalogs.yml" - ).exists() - - def test_catalog_add_escapes_url_markup(self, tmp_path): - """Catalog add should render user-supplied URLs literally.""" - from typer.testing import CliRunner - from unittest.mock import patch - from specify_cli import app - - project_dir = tmp_path / "test-project" - project_dir.mkdir() - (project_dir / ".specify").mkdir() - - url = "https://example.com/[red]catalog[/red].json" - - runner = CliRunner() - with patch.object(Path, "cwd", return_value=project_dir): - result = runner.invoke( - app, - [ - "extension", - "catalog", - "add", - url, - "--name", - "community", - ], - catch_exceptions=True, - ) - - assert result.exit_code == 0, result.output - assert f"URL: {url}" in result.output - - @pytest.mark.parametrize("name", ["community", " community "]) - @pytest.mark.parametrize("padding", ["", " \t"]) - def test_catalog_add_duplicate_is_idempotent(self, tmp_path, name, padding): - """Re-adding an identical catalog is a successful no-op (#4505).""" + def test_catalog_add_is_idempotent_for_identical_entry(self, tmp_path): from typer.testing import CliRunner from unittest.mock import patch from specify_cli import app @@ -7634,68 +7576,26 @@ def test_catalog_add_duplicate_is_idempotent(self, tmp_path, name, padding): project_dir = tmp_path / "test-project" project_dir.mkdir() (project_dir / ".specify").mkdir() - args = [ - "extension", "catalog", "add", - "https://example.com/catalog.json", "--name", "community", + "extension", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + "community", ] + runner = CliRunner() with patch.object(Path, "cwd", return_value=project_dir): - first = runner.invoke(app, [ - "extension", "catalog", "add", - f"{padding}https://example.com/catalog.json{padding}", "--name", name, - ], catch_exceptions=True) - assert first.exit_code == 0, first.output + assert runner.invoke(app, args).exit_code == 0 config_path = project_dir / ".specify" / "extension-catalogs.yml" original = config_path.read_bytes() - second = runner.invoke(app, args, catch_exceptions=True) - - assert second.exit_code == 0, second.output - assert "nothing to do" in second.output - assert config_path.read_bytes() == original - entries = yaml.safe_load(original)["catalogs"] - assert len(entries) == 1 - assert entries[0]["name"] == "community" - assert entries[0]["url"] == "https://example.com/catalog.json" - - @pytest.mark.parametrize("stored_name,name", [ - (" community ", "community"), - ("community", " community "), - (" community ", "\tcommunity\t"), - ]) - @pytest.mark.parametrize("stored_padding,padding", [(" \t", ""), ("", " \t"), (" ", "\t")]) - @pytest.mark.parametrize("priority", [10, 20]) - def test_catalog_add_normalizes_existing_identity( - self, project_dir, monkeypatch, stored_name, name, stored_padding, padding, priority - ): - from typer.testing import CliRunner - from specify_cli import app + assert runner.invoke(app, args).exit_code == 0 + assert config_path.read_bytes() == original + assert runner.invoke(app, [*args, "--priority", "11"]).exit_code == 1 - config_path = project_dir / ".specify" / "extension-catalogs.yml" - config_path.write_text(yaml.safe_dump({"catalogs": [{ - "name": stored_name, - "url": f"{stored_padding}https://example.com/catalog.json{stored_padding}", - "priority": 10, - "install_allowed": False, - }]}), encoding="utf-8") - original = config_path.read_bytes() - modified_at = config_path.stat().st_mtime_ns - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke(app, [ - "extension", "catalog", "add", - f"{padding}https://example.com/catalog.json{padding}", - "--name", name, "--priority", str(priority), - ], catch_exceptions=False) - - assert result.exit_code == (0 if priority == 10 else 1), result.output - assert ("nothing to do" if priority == 10 else "different settings") in result.output - assert config_path.read_bytes() == original - assert config_path.stat().st_mtime_ns == modified_at - - def test_catalog_add_duplicate_different_settings_conflicts(self, tmp_path): - """Re-adding a same-named catalog with different settings errors (#4505).""" + def test_catalog_add_escapes_url_markup(self, tmp_path): + """Catalog add should render user-supplied URLs literally.""" from typer.testing import CliRunner from unittest.mock import patch from specify_cli import app @@ -7704,251 +7604,25 @@ def test_catalog_add_duplicate_different_settings_conflicts(self, tmp_path): project_dir.mkdir() (project_dir / ".specify").mkdir() - runner = CliRunner() - with patch.object(Path, "cwd", return_value=project_dir): - first = runner.invoke(app, [ - "extension", "catalog", "add", - "https://example.com/catalog.json", "--name", "community", - "--priority", "10", - ], catch_exceptions=True) - second = runner.invoke(app, [ - "extension", "catalog", "add", - "https://example.com/catalog.json", "--name", "community", - "--priority", "20", - ], catch_exceptions=True) - - assert first.exit_code == 0, first.output - assert second.exit_code == 1 - assert "different settings" in second.output - - def test_catalog_add_string_representations_are_idempotent(self, tmp_path): - """A stored entry using supported string representations (a numeric-string - priority and a string boolean) is equivalent to the requested defaults, so - a rerun is a no-op rather than a false conflict (#4505).""" - import yaml as _yaml - from typer.testing import CliRunner - from unittest.mock import patch - from specify_cli import app - - project_dir = tmp_path / "test-project" - project_dir.mkdir() - (project_dir / ".specify").mkdir() - # Hand-written config: priority as a string, install_allowed as "false". - (project_dir / ".specify" / "extension-catalogs.yml").write_text( - _yaml.safe_dump({"catalogs": [{ - "name": "community", - "url": "https://example.com/catalog.json", - "priority": "10", - "install_allowed": "false", - "description": "", - }]}), - encoding="utf-8", - ) + url = "https://example.com/[red]catalog[/red].json" runner = CliRunner() with patch.object(Path, "cwd", return_value=project_dir): - result = runner.invoke(app, [ - "extension", "catalog", "add", - "https://example.com/catalog.json", "--name", "community", - ], catch_exceptions=True) - - assert result.exit_code == 0, result.output - assert "nothing to do" in result.output - - @pytest.mark.parametrize("stored_priority,priority", [ - (True, 1), (False, 0), (1, 1), (0, 0), ("1", 1), ("0", 0), - ]) - def test_catalog_add_priority_equivalence(self, project_dir, monkeypatch, stored_priority, priority): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "extension-catalogs.yml" - config_path.write_text(yaml.safe_dump({"catalogs": [{ - "name": "mine", - "url": "https://example.com/catalog.json", - "priority": stored_priority, - "install_allowed": False, - }]}), encoding="utf-8") - original = config_path.read_bytes() - modified_at = config_path.stat().st_mtime_ns - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke(app, [ - "extension", "catalog", "add", "https://example.com/catalog.json", - "--name", "mine", "--priority", str(priority), - ], catch_exceptions=False) - - invalid = isinstance(stored_priority, bool) - assert result.exit_code == (1 if invalid else 0), result.output - assert ("different settings" if invalid else "nothing to do") in result.output - assert config_path.read_bytes() == original - assert config_path.stat().st_mtime_ns == modified_at - - def test_catalog_add_omitted_priority_uses_reader_default( - self, project_dir, monkeypatch - ): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "extension-catalogs.yml" - config_path.write_text( - yaml.safe_dump( - { - "catalogs": [ - { - "name": "mine", - "url": "https://example.com/catalog.json", - "install_allowed": False, - } - ] - } - ), - encoding="utf-8", - ) - original = config_path.read_bytes() - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - [ - "extension", - "catalog", - "add", - "https://example.com/catalog.json", - "--name", - "mine", - "--priority", - "1", - ], - catch_exceptions=False, - ) - - assert result.exit_code == 0, result.output - assert "nothing to do" in result.output - assert config_path.read_bytes() == original - - @pytest.mark.parametrize("stored_name", ["missing", None, "", " \t"]) - def test_catalog_add_blank_name_uses_reader_default( - self, project_dir, monkeypatch, stored_name - ): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "extension-catalogs.yml" - entry = { - "url": "https://example.com/catalog.json", - "priority": 10, - "install_allowed": False, - } - if stored_name != "missing": - entry["name"] = stored_name - config_path.write_text( - yaml.safe_dump({"catalogs": [entry]}), - encoding="utf-8", - ) - original = config_path.read_bytes() - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - [ - "extension", - "catalog", - "add", - "https://example.com/catalog.json", - "--name", - "catalog-1", - ], - catch_exceptions=False, - ) - - assert result.exit_code == 0, result.output - assert "nothing to do" in result.output - assert config_path.read_bytes() == original - - def test_catalog_add_same_name_with_blank_url_conflicts( - self, project_dir, monkeypatch - ): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "extension-catalogs.yml" - config_path.write_text( - yaml.safe_dump( - { - "catalogs": [ - { - "name": "mine", - "url": "", - "priority": 10, - "install_allowed": False, - } - ] - } - ), - encoding="utf-8", - ) - original = config_path.read_bytes() - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( + result = runner.invoke( app, [ "extension", "catalog", "add", - "https://example.com/catalog.json", + url, "--name", - "mine", + "community", ], - catch_exceptions=False, - ) - - assert result.exit_code == 1 - assert "different settings" in result.output - assert config_path.read_bytes() == original - - @pytest.mark.parametrize("requested_name", ["mine", " mine "]) - def test_catalog_remove_normalizes_name( - self, project_dir, monkeypatch, requested_name - ): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "extension-catalogs.yml" - config_path.write_text( - yaml.safe_dump( - { - "catalogs": [ - { - "name": " mine ", - "url": "https://example.com/catalog.json", - "priority": 10, - "install_allowed": False, - } - ] - } - ), - encoding="utf-8", - ) - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - ["extension", "catalog", "remove", requested_name], - catch_exceptions=False, + catch_exceptions=True, ) assert result.exit_code == 0, result.output - assert "Removed catalog 'mine'" in result.output - assert yaml.safe_load(config_path.read_text(encoding="utf-8"))[ - "catalogs" - ] == [] + assert f"URL: {url}" in result.output def test_catalog_add_escapes_config_saved_path_markup(self, tmp_path): """Catalog add's saved-path label should render literally under Rich.""" diff --git a/tests/test_presets.py b/tests/test_presets.py index e85fffae7a..b154c24986 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3848,34 +3848,29 @@ def test_default_description(self): class TestPresetCatalogMultiCatalog: - def test_catalog_add_rejects_empty_normalized_name( - self, project_dir, monkeypatch - ): + """Test multi-catalog support in PresetCatalog.""" + + def test_catalog_add_is_idempotent_for_identical_entry(self, project_dir): from typer.testing import CliRunner + from unittest.mock import patch from specify_cli import app - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - [ - "preset", - "catalog", - "add", - "https://example.com/catalog.json", - "--name", - " \t", - ], - catch_exceptions=False, - ) - - assert result.exit_code == 1 - assert "must be non-empty" in result.output - assert not ( - project_dir / ".specify" / "preset-catalogs.yml" - ).exists() - - """Test multi-catalog support in PresetCatalog.""" + args = [ + "preset", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + "community", + ] + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + assert runner.invoke(app, args).exit_code == 0 + config_path = project_dir / ".specify" / "preset-catalogs.yml" + original = config_path.read_bytes() + assert runner.invoke(app, args).exit_code == 0 + assert config_path.read_bytes() == original + assert runner.invoke(app, [*args, "--priority", "11"]).exit_code == 1 def test_default_active_catalogs(self, project_dir): """Test that default catalogs are returned when no config exists.""" @@ -3942,316 +3937,6 @@ def test_catalog_add_escapes_rich_markup(self, project_dir): assert config["catalogs"][0]["name"] == name assert config["catalogs"][0]["url"] == url - @pytest.mark.parametrize("padding", ["", " \t"]) - def test_catalog_add_duplicate_is_idempotent(self, project_dir, padding): - """Re-adding an identical preset catalog is a successful no-op (#4505).""" - from typer.testing import CliRunner - from unittest.mock import patch - from specify_cli import app - - args = [ - "preset", "catalog", "add", - "https://example.com/c.json", "--name", "mine", - ] - runner = CliRunner() - with patch.object(Path, "cwd", return_value=project_dir): - first = runner.invoke(app, [ - "preset", "catalog", "add", - f"{padding}https://example.com/c.json{padding}", "--name", "mine", - ]) - assert first.exit_code == 0, first.output - config_path = project_dir / ".specify" / "preset-catalogs.yml" - original = config_path.read_bytes() - second = runner.invoke(app, args) - assert second.exit_code == 0, second.output - assert "nothing to do" in second.output - assert config_path.read_bytes() == original - entries = yaml.safe_load(original)["catalogs"] - assert len(entries) == 1 - assert entries[0]["url"] == "https://example.com/c.json" - - @pytest.mark.parametrize("stored_padding,padding", [(" \t", ""), ("", " \t"), (" ", "\t")]) - @pytest.mark.parametrize("priority", [10, 20]) - def test_catalog_add_normalizes_existing_url( - self, project_dir, monkeypatch, stored_padding, padding, priority - ): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "preset-catalogs.yml" - config_path.write_text(yaml.safe_dump({"catalogs": [{ - "name": "mine", - "url": f"{stored_padding}https://example.com/c.json{stored_padding}", - "priority": 10, - "install_allowed": False, - }]}), encoding="utf-8") - original = config_path.read_bytes() - modified_at = config_path.stat().st_mtime_ns - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke(app, [ - "preset", "catalog", "add", f"{padding}https://example.com/c.json{padding}", - "--name", "mine", "--priority", str(priority), - ], catch_exceptions=False) - - assert result.exit_code == (0 if priority == 10 else 1), result.output - assert ("nothing to do" if priority == 10 else "different settings") in result.output - assert config_path.read_bytes() == original - assert config_path.stat().st_mtime_ns == modified_at - - @pytest.mark.parametrize("requested_name", ["mine", " mine "]) - def test_catalog_add_normalizes_existing_name( - self, project_dir, monkeypatch, requested_name - ): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "preset-catalogs.yml" - config_path.write_text( - yaml.safe_dump( - { - "catalogs": [ - { - "name": " mine ", - "url": "https://example.com/catalog.json", - "priority": 10, - "install_allowed": False, - } - ] - } - ), - encoding="utf-8", - ) - original = config_path.read_bytes() - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - [ - "preset", - "catalog", - "add", - "https://example.com/catalog.json", - "--name", - requested_name, - ], - catch_exceptions=False, - ) - - assert result.exit_code == 0, result.output - assert "nothing to do" in result.output - assert config_path.read_bytes() == original - - def test_catalog_add_omitted_priority_uses_reader_default( - self, project_dir, monkeypatch - ): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "preset-catalogs.yml" - config_path.write_text( - yaml.safe_dump( - { - "catalogs": [ - { - "name": "mine", - "url": "https://example.com/catalog.json", - "install_allowed": False, - } - ] - } - ), - encoding="utf-8", - ) - original = config_path.read_bytes() - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - [ - "preset", - "catalog", - "add", - "https://example.com/catalog.json", - "--name", - "mine", - "--priority", - "1", - ], - catch_exceptions=False, - ) - - assert result.exit_code == 0, result.output - assert "nothing to do" in result.output - assert config_path.read_bytes() == original - - @pytest.mark.parametrize("stored_name", ["missing", None, "", " \t"]) - def test_catalog_add_blank_name_uses_reader_default( - self, project_dir, monkeypatch, stored_name - ): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "preset-catalogs.yml" - entry = { - "url": "https://example.com/catalog.json", - "priority": 10, - "install_allowed": False, - } - if stored_name != "missing": - entry["name"] = stored_name - config_path.write_text( - yaml.safe_dump({"catalogs": [entry]}), - encoding="utf-8", - ) - original = config_path.read_bytes() - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - [ - "preset", - "catalog", - "add", - "https://example.com/catalog.json", - "--name", - "catalog-1", - ], - catch_exceptions=False, - ) - - assert result.exit_code == 0, result.output - assert "nothing to do" in result.output - assert config_path.read_bytes() == original - - def test_catalog_add_same_name_with_blank_url_conflicts( - self, project_dir, monkeypatch - ): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "preset-catalogs.yml" - config_path.write_text( - yaml.safe_dump( - { - "catalogs": [ - { - "name": "mine", - "url": "", - "priority": 10, - "install_allowed": False, - } - ] - } - ), - encoding="utf-8", - ) - original = config_path.read_bytes() - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - [ - "preset", - "catalog", - "add", - "https://example.com/catalog.json", - "--name", - "mine", - ], - catch_exceptions=False, - ) - - assert result.exit_code == 1 - assert "different settings" in result.output - assert config_path.read_bytes() == original - - def test_catalog_add_duplicate_different_settings_conflicts(self, project_dir): - """Re-adding a same-named preset catalog with different settings errors (#4505).""" - from typer.testing import CliRunner - from unittest.mock import patch - from specify_cli import app - - runner = CliRunner() - with patch.object(Path, "cwd", return_value=project_dir): - first = runner.invoke(app, [ - "preset", "catalog", "add", - "https://example.com/c.json", "--name", "mine", "--priority", "10", - ]) - second = runner.invoke(app, [ - "preset", "catalog", "add", - "https://example.com/c.json", "--name", "mine", "--priority", "20", - ]) - assert first.exit_code == 0, first.output - assert second.exit_code == 1 - assert "different settings" in second.output - - def test_catalog_add_string_representations_are_idempotent(self, project_dir): - """A stored preset catalog using supported string representations (a - numeric-string priority and a string boolean) is equivalent to the - requested defaults, so a rerun is a no-op rather than a false conflict - (#4505).""" - import yaml as _yaml - from typer.testing import CliRunner - from unittest.mock import patch - from specify_cli import app - - (project_dir / ".specify" / "preset-catalogs.yml").write_text( - _yaml.safe_dump({"catalogs": [{ - "name": "mine", - "url": "https://example.com/c.json", - "priority": "10", - "install_allowed": "false", - "description": "", - }]}), - encoding="utf-8", - ) - - runner = CliRunner() - with patch.object(Path, "cwd", return_value=project_dir): - result = runner.invoke(app, [ - "preset", "catalog", "add", - "https://example.com/c.json", "--name", "mine", - ]) - - assert result.exit_code == 0, result.output - assert "nothing to do" in result.output - - @pytest.mark.parametrize("stored_priority,priority", [ - (True, 1), (False, 0), (1, 1), (0, 0), ("1", 1), ("0", 0), - ]) - def test_catalog_add_priority_equivalence(self, project_dir, monkeypatch, stored_priority, priority): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "preset-catalogs.yml" - config_path.write_text(yaml.safe_dump({"catalogs": [{ - "name": "mine", - "url": "https://example.com/catalog.json", - "priority": stored_priority, - "install_allowed": False, - }]}), encoding="utf-8") - original = config_path.read_bytes() - modified_at = config_path.stat().st_mtime_ns - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke(app, [ - "preset", "catalog", "add", "https://example.com/catalog.json", - "--name", "mine", "--priority", str(priority), - ], catch_exceptions=False) - - invalid = isinstance(stored_priority, bool) - assert result.exit_code == (1 if invalid else 0), result.output - assert ("different settings" if invalid else "nothing to do") in result.output - assert config_path.read_bytes() == original - assert config_path.stat().st_mtime_ns == modified_at - def test_catalog_remove_escapes_rich_markup(self, project_dir): """`preset catalog remove` must not parse the name as Rich markup.""" from typer.testing import CliRunner @@ -4278,44 +3963,6 @@ def test_catalog_remove_escapes_rich_markup(self, project_dir): assert result.exit_code == 0, result.output assert name in result.output - @pytest.mark.parametrize("requested_name", ["mine", " mine "]) - def test_catalog_remove_normalizes_name( - self, project_dir, monkeypatch, requested_name - ): - from typer.testing import CliRunner - from specify_cli import app - - config_path = project_dir / ".specify" / "preset-catalogs.yml" - config_path.write_text( - yaml.safe_dump( - { - "catalogs": [ - { - "name": " mine ", - "url": "https://example.com/catalog.json", - "priority": 10, - "install_allowed": False, - } - ] - } - ), - encoding="utf-8", - ) - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - ["preset", "catalog", "remove", requested_name], - catch_exceptions=False, - ) - - assert result.exit_code == 0, result.output - assert "Removed catalog 'mine'" in result.output - assert yaml.safe_load(config_path.read_text(encoding="utf-8"))[ - "catalogs" - ] == [] - def test_catalog_remove_escapes_markup_in_not_found_error(self, project_dir): """The not-found error path renders the name too.""" from typer.testing import CliRunner diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 95c898d5bf..c7df1bbc3c 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -8786,35 +8786,20 @@ def test_add_catalog_with_existing_inf_priority(self, project_dir): new = next(c for c in data["catalogs"] if c["url"] == "https://b.example.com/c.json") assert new["priority"] == 1 # max(inf coerced to 0) + 1 - def test_add_catalog_duplicate_is_idempotent(self, project_dir): - from specify_cli.workflows.catalog import WorkflowCatalog - - catalog = WorkflowCatalog(project_dir) - assert catalog.add_catalog("https://example.com/catalog.json") == "added" - assert catalog.add_catalog("https://example.com/catalog.json") == "unchanged" - - cfg = project_dir / ".specify" / "workflow-catalogs.yml" - data = yaml.safe_load(cfg.read_text(encoding="utf-8")) - assert len(data["catalogs"]) == 1 - - def test_add_catalog_duplicate_same_name_is_idempotent(self, project_dir): - from specify_cli.workflows.catalog import WorkflowCatalog - - catalog = WorkflowCatalog(project_dir) - assert catalog.add_catalog("https://example.com/catalog.json", "mine") == "added" - assert catalog.add_catalog("https://example.com/catalog.json", "mine") == "unchanged" - - cfg = project_dir / ".specify" / "workflow-catalogs.yml" - data = yaml.safe_load(cfg.read_text(encoding="utf-8")) - assert len(data["catalogs"]) == 1 - - def test_add_catalog_duplicate_different_name_conflicts(self, project_dir): + def test_add_catalog_is_idempotent_for_identical_url_and_name( + self, project_dir + ): from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError catalog = WorkflowCatalog(project_dir) - catalog.add_catalog("https://example.com/catalog.json", "first") - with pytest.raises(WorkflowValidationError, match="different name"): - catalog.add_catalog("https://example.com/catalog.json", "second") + catalog.add_catalog("https://example.com/catalog.json") + config_path = project_dir / ".specify" / "workflow-catalogs.yml" + original = config_path.read_bytes() + catalog.add_catalog("https://example.com/catalog.json") + assert config_path.read_bytes() == original + + with pytest.raises(WorkflowValidationError, match="already configured"): + catalog.add_catalog("https://example.com/catalog.json", "different") def test_remove_catalog(self, project_dir): from specify_cli.workflows.catalog import WorkflowCatalog @@ -9549,35 +9534,20 @@ def test_add_catalog_rejects_falsy_non_mapping_config( assert config_path.read_text(encoding="utf-8") == original - def test_add_catalog_duplicate_is_idempotent(self, project_dir): - from specify_cli.workflows.catalog import StepCatalog - - catalog = StepCatalog(project_dir) - assert catalog.add_catalog("https://example.com/steps.json") == "added" - assert catalog.add_catalog("https://example.com/steps.json") == "unchanged" - - cfg = project_dir / ".specify" / "step-catalogs.yml" - data = yaml.safe_load(cfg.read_text(encoding="utf-8")) - assert len(data["catalogs"]) == 1 - - def test_add_catalog_duplicate_same_name_is_idempotent(self, project_dir): - from specify_cli.workflows.catalog import StepCatalog - - catalog = StepCatalog(project_dir) - assert catalog.add_catalog("https://example.com/steps.json", "mine") == "added" - assert catalog.add_catalog("https://example.com/steps.json", "mine") == "unchanged" - - cfg = project_dir / ".specify" / "step-catalogs.yml" - data = yaml.safe_load(cfg.read_text(encoding="utf-8")) - assert len(data["catalogs"]) == 1 - - def test_add_catalog_duplicate_different_name_conflicts(self, project_dir): + def test_add_catalog_is_idempotent_for_identical_url_and_name( + self, project_dir + ): from specify_cli.workflows.catalog import StepCatalog, StepValidationError catalog = StepCatalog(project_dir) - catalog.add_catalog("https://example.com/steps.json", "first") - with pytest.raises(StepValidationError, match="different name"): - catalog.add_catalog("https://example.com/steps.json", "second") + catalog.add_catalog("https://example.com/steps.json") + config_path = project_dir / ".specify" / "step-catalogs.yml" + original = config_path.read_bytes() + catalog.add_catalog("https://example.com/steps.json") + assert config_path.read_bytes() == original + + with pytest.raises(StepValidationError, match="already configured"): + catalog.add_catalog("https://example.com/steps.json", "different") def test_remove_catalog(self, project_dir): from specify_cli.workflows.catalog import StepCatalog @@ -9981,230 +9951,6 @@ def make_module_name(type_key: str) -> str: assert name_a != name_b, "Module names for 'a-b' and 'a_b' must differ" -@pytest.mark.parametrize("command,config_filename", [ - (["workflow", "catalog", "add"], "workflow-catalogs.yml"), - (["workflow", "step", "catalog", "add"], "step-catalogs.yml"), -]) -class TestWorkflowCatalogAddCLI: - @pytest.mark.parametrize( - ("requested_name", "stored_name"), - [(" mine ", "mine"), (" ", "catalog-1")], - ) - def test_add_catalog_normalizes_name( - self, - project_dir, - monkeypatch, - command, - config_filename, - requested_name, - stored_name, - ): - from typer.testing import CliRunner - from specify_cli import app - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - [ - *command, - "https://example.com/catalog.json", - "--name", - requested_name, - ], - catch_exceptions=False, - ) - - assert result.exit_code == 0, result.output - config_path = project_dir / ".specify" / config_filename - data = yaml.safe_load(config_path.read_text(encoding="utf-8")) - assert data["catalogs"][0]["name"] == stored_name - - @pytest.mark.parametrize("priority", ["not-a-number", True]) - def test_add_catalog_duplicate_validates_existing_config( - self, project_dir, monkeypatch, command, config_filename, priority - ): - from typer.testing import CliRunner - from specify_cli import app - - url = "https://example.com/catalog.json" - config_path = project_dir / ".specify" / config_filename - config_path.write_text( - yaml.safe_dump( - { - "catalogs": [ - { - "name": "mine", - "url": url, - "priority": priority, - } - ] - } - ), - encoding="utf-8", - ) - original = config_path.read_bytes() - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - [*command, url, "--name", "mine"], - catch_exceptions=False, - ) - - assert result.exit_code == 1, result.output - assert "Invalid priority" in result.output - assert config_path.read_bytes() == original - - def test_add_catalog_escapes_markup_in_success_output( - self, project_dir, monkeypatch, command, config_filename - ): - from typer.testing import CliRunner - from specify_cli import app - - url = "https://example.com/[/red]/catalog.json" - runner = CliRunner() - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - first = runner.invoke( - app, [*command, url], catch_exceptions=False - ) - assert first.exit_code == 0, first.output - assert url in first.output - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - second = runner.invoke( - app, [*command, url], catch_exceptions=False - ) - assert second.exit_code == 0, second.output - assert url in second.output - - def test_add_catalog_missing_name_uses_loader_fallback( - self, project_dir, monkeypatch, command, config_filename - ): - from typer.testing import CliRunner - from specify_cli import app - - url = "https://example.com/catalog.json" - config_path = project_dir / ".specify" / config_filename - config_path.write_text( - yaml.safe_dump( - { - "catalogs": [ - { - "url": url, - "priority": 1, - "install_allowed": True, - } - ] - } - ), - encoding="utf-8", - ) - original = config_path.read_bytes() - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, - [*command, url, "--name", "catalog-1"], - catch_exceptions=False, - ) - - assert result.exit_code == 0, result.output - assert "already configured" in result.output - assert config_path.read_bytes() == original - - @pytest.mark.parametrize("name", [None, "mine"]) - @pytest.mark.parametrize("padding", ["", " \t"]) - def test_add_catalog_duplicate_outcomes( - self, project_dir, monkeypatch, command, config_filename, name, padding - ): - from typer.testing import CliRunner - from specify_cli import app - - url = "https://example.com/catalog.json" - name_args = ["--name", name] if name is not None else [] - runner = CliRunner() - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - first = runner.invoke( - app, [*command, f"{padding}{url}{padding}", *name_args], catch_exceptions=False - ) - assert first.exit_code == 0, first.output - assert "source added" in first.output - config_path = project_dir / ".specify" / config_filename - original = config_path.read_bytes() - modified_at = config_path.stat().st_mtime_ns - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - second = runner.invoke(app, [*command, url, *name_args], catch_exceptions=False) - - assert second.exit_code == 0, second.output - assert "already configured" in second.output - assert "source added" not in second.output - assert config_path.read_bytes() == original - assert config_path.stat().st_mtime_ns == modified_at - entries = yaml.safe_load(original)["catalogs"] - assert len(entries) == 1 - assert entries[0]["url"] == url - assert entries[0]["name"] == (name or "catalog-1") - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - conflict = runner.invoke( - app, [*command, url, "--name", "different"], catch_exceptions=False - ) - assert conflict.exit_code == 1, conflict.output - assert "different name" in conflict.output - assert "source added" not in conflict.output - assert config_path.read_bytes() == original - assert config_path.stat().st_mtime_ns == modified_at - - @pytest.mark.parametrize("stored_padding,padding", [ - ("", ""), (" \t", ""), ("", " \t"), (" ", "\t"), - ]) - @pytest.mark.parametrize("name", [None, "mine", "different"]) - def test_add_catalog_existing_url_outcomes( - self, project_dir, monkeypatch, command, config_filename, stored_padding, padding, name - ): - from typer.testing import CliRunner - from specify_cli import app - - url = "https://example.com/catalog.json" - config_path = project_dir / ".specify" / config_filename - config_path.write_text(yaml.safe_dump({"catalogs": [{ - "name": "mine", - "url": f"{stored_padding}{url}{stored_padding}", - "priority": 7, - "install_allowed": False, - "description": "Keep this entry unchanged.", - }]}), encoding="utf-8") - original = config_path.read_bytes() - modified_at = config_path.stat().st_mtime_ns - name_args = ["--name", name] if name is not None else [] - - with monkeypatch.context() as scoped: - scoped.chdir(project_dir) - result = CliRunner().invoke( - app, [*command, f"{padding}{url}{padding}", *name_args], catch_exceptions=False - ) - - if name == "different": - assert result.exit_code == 1, result.output - assert "different name" in result.output - assert "already configured:" not in result.output - else: - assert result.exit_code == 0, result.output - assert "already configured" in result.output - assert "source added" not in result.output - assert config_path.read_bytes() == original - assert config_path.stat().st_mtime_ns == modified_at - - # ===== CLI Step Remove Tests ===== class TestWorkflowStepRemoveCLI: diff --git a/tests/unit/test_bundler_catalog_config.py b/tests/unit/test_bundler_catalog_config.py index 94794b7ce6..62be668b5b 100644 --- a/tests/unit/test_bundler_catalog_config.py +++ b/tests/unit/test_bundler_catalog_config.py @@ -63,13 +63,37 @@ def test_add_source_persists_absolute_local_path(tmp_path: Path, monkeypatch): catalog.write_text("{}", encoding="utf-8") monkeypatch.chdir(project) - source, status = cc.add_source(project, "sub/cat.json", policy="install-allowed", priority=50) + source = cc.add_source(project, "sub/cat.json", policy="install-allowed", priority=50) - assert status == "added" assert Path(source.url).is_absolute() assert Path(source.url) == catalog.resolve() +def test_add_source_is_idempotent_for_identical_entry(tmp_path: Path): + project = tmp_path / "proj" + (project / ".specify").mkdir(parents=True) + args = { + "policy": "install-allowed", + "priority": 50, + "source_id": "example", + } + + first = cc.add_source(project, "https://example.com/catalog.json", **args) + original = cc._config_path(project).read_bytes() + second = cc.add_source(project, "https://example.com/catalog.json", **args) + + assert second == first + assert cc._config_path(project).read_bytes() == original + with pytest.raises(BundlerError, match="already exists"): + cc.add_source( + project, + "https://example.com/catalog.json", + policy="install-allowed", + priority=51, + source_id="example", + ) + + def test_remove_source_accepts_relative_local_path(tmp_path: Path, monkeypatch): """add_source stores a local path as an absolute url, so remove_source must accept the same relative path the caller added; otherwise `remove ./cat.json` @@ -124,127 +148,6 @@ def test_add_source_refuses_symlinked_specify_escape(tmp_path: Path): cc.add_source(project, "https://example.com/c.json", policy="install-allowed", priority=50) -@pytest.mark.parametrize("padding", ["", " \t"]) -def test_add_source_rerun_surfaces_bad_stored_priority_as_bundlererror(tmp_path: Path, padding): - """A hand-edited matching entry with a non-integer priority must surface a - clean BundlerError during an idempotent-add comparison rather than leaking - int()'s ValueError past the CLI's `except BundlerError` (#4505).""" - project = tmp_path / "proj" - (project / ".specify").mkdir(parents=True) - cc._config_path(project).write_text( - "schema_version: '1.0'\n" - "catalogs:\n" - f" - id: '{padding}mine{padding}'\n" - f" url: '{padding}https://example.com/c.json{padding}'\n" - " priority: not-a-number\n" - " install_policy: install-allowed\n", - encoding="utf-8", - ) - - config_path = cc._config_path(project) - original = config_path.read_bytes() - modified_at = config_path.stat().st_mtime_ns - with pytest.raises(BundlerError, match="non-integer priority"): - cc.add_source( - project, "https://example.com/c.json", source_id="mine", - policy="install-allowed", priority=10, - ) - assert config_path.read_bytes() == original - assert config_path.stat().st_mtime_ns == modified_at - - -def test_add_source_rerun_with_string_priority_is_unchanged(tmp_path: Path): - """A stored priority written as a numeric string is normalized like catalog - parsing, so an otherwise-identical rerun is a no-op, not a false conflict.""" - project = tmp_path / "proj" - (project / ".specify").mkdir(parents=True) - cc._config_path(project).write_text( - "schema_version: '1.0'\n" - "catalogs:\n" - " - id: mine\n" - " url: https://example.com/c.json\n" - " priority: '10'\n" - " install_policy: install-allowed\n", - encoding="utf-8", - ) - - source, status = cc.add_source( - project, "https://example.com/c.json", source_id="mine", - policy="install-allowed", priority=10, - ) - assert status == "unchanged" - assert source.priority == 10 - - -def test_add_source_same_url_without_id_preserves_custom_id(tmp_path: Path): - project = tmp_path / "proj" - (project / ".specify").mkdir(parents=True) - original_source, first_status = cc.add_source( - project, - "https://example.com/c.json", - source_id="custom", - policy="install-allowed", - priority=10, - ) - - source, status = cc.add_source( - project, - "https://example.com/c.json", - policy="install-allowed", - priority=10, - ) - - assert first_status == "added" - assert status == "unchanged" - assert source.id == original_source.id == "custom" - assert len(cc._read(project)) == 1 - - -@pytest.mark.parametrize("id_padding", ["", " \t"]) -@pytest.mark.parametrize("url_padding", ["", " \t"]) -@pytest.mark.parametrize("source_id,url,outcome", [ - ("local", "https://example.com/c.json", "unchanged"), - ("local", "https://example.com/other.json", "conflict"), - ("other", "https://example.com/c.json", "conflict"), - ("other", "https://example.com/other.json", "added"), -]) -def test_add_source_normalizes_stored_identity( - tmp_path: Path, id_padding, url_padding, source_id, url, outcome -): - project = tmp_path / "proj" - (project / ".specify").mkdir(parents=True) - existing = { - "id": f"{id_padding}local{id_padding}", - "url": f"{url_padding}https://example.com/c.json{url_padding}", - "priority": 10, - "install_policy": "install-allowed", - } - cc._write(project, [existing]) - config_path = cc._config_path(project) - original = config_path.read_bytes() - modified_at = config_path.stat().st_mtime_ns - - if outcome == "conflict": - with pytest.raises(BundlerError, match="different settings"): - cc.add_source(project, url, source_id=source_id, policy="install-allowed", priority=10) - else: - source, status = cc.add_source( - project, url, source_id=source_id, policy="install-allowed", priority=10, - ) - assert status == outcome - assert source.id == source_id - assert source.url == url - assert source.priority == 10 - assert source.install_allowed - - entries = cc._read(project) - assert entries[0] == existing - assert len(entries) == (2 if outcome == "added" else 1) - if outcome != "added": - assert config_path.read_bytes() == original - assert config_path.stat().st_mtime_ns == modified_at - - def test_read_rejects_non_list_catalogs(tmp_path: Path): project = tmp_path / "proj" (project / ".specify").mkdir(parents=True) @@ -356,7 +259,7 @@ def test_add_source_allows_local_path_with_colon(tmp_path: Path, monkeypatch): (project / ".specify").mkdir(parents=True) monkeypatch.chdir(project) # A relative path containing ':' but no '://' is still a local path. - source, _ = cc.add_source(project, "weird:name.json", policy="install-allowed", priority=50) + source = cc.add_source(project, "weird:name.json", policy="install-allowed", priority=50) assert source.url.endswith("weird:name.json") or "weird" in source.url @@ -370,7 +273,7 @@ def test_add_source_rejects_plain_http_for_non_localhost(tmp_path: Path): def test_add_source_allows_http_for_localhost(tmp_path: Path): project = tmp_path / "proj" (project / ".specify").mkdir(parents=True) - source, _ = cc.add_source(project, "http://localhost:8080/c.json", policy="install-allowed", priority=50) + source = cc.add_source(project, "http://localhost:8080/c.json", policy="install-allowed", priority=50) assert source.url == "http://localhost:8080/c.json" From 8ade9a3e6a55fabb173732c3f27cb39ddcd7e42a Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 18 Sep 2026 16:25:51 -0500 Subject: [PATCH 11/11] test: align integration catalog idempotency Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/integrations/test_cli.py | 12 +++++++++--- tests/integrations/test_integration_catalog.py | 11 ++--------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/tests/integrations/test_cli.py b/tests/integrations/test_cli.py index 2beb411a2a..5b1754a015 100644 --- a/tests/integrations/test_cli.py +++ b/tests/integrations/test_cli.py @@ -2539,7 +2539,7 @@ def test_catalog_add_rejects_invalid_url(self, tmp_path, monkeypatch): assert result.exit_code == 1 assert "HTTPS" in result.output - def test_catalog_add_rejects_duplicate(self, tmp_path, monkeypatch): + def test_catalog_add_accepts_identical_duplicate(self, tmp_path, monkeypatch): project = self._make_project(tmp_path) url = "https://dup.example.com/catalog.json" first = self._invoke( @@ -2549,8 +2549,14 @@ def test_catalog_add_rejects_duplicate(self, tmp_path, monkeypatch): second = self._invoke( ["integration", "catalog", "add", url], project ) - assert second.exit_code == 1 - assert "already configured" in second.output + assert second.exit_code == 0, second.output + + conflict = self._invoke( + ["integration", "catalog", "add", url, "--name", "different"], + project, + ) + assert conflict.exit_code == 1 + assert "already configured" in conflict.output def test_catalog_remove_out_of_range(self, tmp_path, monkeypatch): project = self._make_project(tmp_path) diff --git a/tests/integrations/test_integration_catalog.py b/tests/integrations/test_integration_catalog.py index 5222eb95dc..c414c3d8ea 100644 --- a/tests/integrations/test_integration_catalog.py +++ b/tests/integrations/test_integration_catalog.py @@ -1168,19 +1168,12 @@ def test_add_catalog_normalizes_name(self, tmp_path, monkeypatch): entries = data["catalogs"] assert [e["name"] for e in entries] == ["mine", "catalog-2"] - def test_add_catalog_is_idempotent_for_identical_url_and_name( - self, tmp_path, monkeypatch - ): + def test_add_catalog_rejects_duplicate_url(self, tmp_path, monkeypatch): self._isolate(tmp_path, monkeypatch) cat = IntegrationCatalog(tmp_path) cat.add_catalog("https://dup.example.com/catalog.json") - cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" - original = cfg_path.read_bytes() - cat.add_catalog("https://dup.example.com/catalog.json") - assert cfg_path.read_bytes() == original - with pytest.raises(IntegrationValidationError, match="already configured"): - cat.add_catalog("https://dup.example.com/catalog.json", name="different") + cat.add_catalog("https://dup.example.com/catalog.json") def test_add_catalog_rejects_invalid_url(self, tmp_path, monkeypatch): self._isolate(tmp_path, monkeypatch)