diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 9e524c1c85..e60653dfe0 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -451,9 +451,11 @@ Re-adding the same workflow or step catalog URL with the same name succeeds with ```bash specify workflow catalog remove +specify workflow step catalog remove ``` -Removes a catalog by its index in the catalog list. +Removes a project catalog by its index in the corresponding priority-ordered `catalog list`. +Sources supplied by `SPECKIT_WORKFLOW_CATALOG_URL` or `SPECKIT_STEP_CATALOG_URL` cannot be removed this way; unset the variable to manage project sources. ### Catalog Resolution Order diff --git a/src/specify_cli/workflows/catalog/_domain.py b/src/specify_cli/workflows/catalog/_domain.py index 352618fee3..c5b0604a7b 100644 --- a/src/specify_cli/workflows/catalog/_domain.py +++ b/src/specify_cli/workflows/catalog/_domain.py @@ -349,6 +349,15 @@ def _load_catalog_config( raise WorkflowValidationError( f"Failed to read catalog config {config_path}: {exc}" ) from exc + indexed_entries = self._parse_catalog_config(data, config_path) + if indexed_entries is None: + return None + return [entry for _, entry in indexed_entries] + + def _parse_catalog_config( + self, data: Any, config_path: Path + ) -> list[tuple[int, WorkflowCatalogEntry]] | None: + """Validate and order sources, retaining their original YAML positions.""" # An empty document (or explicit ``null``) parses to None -> this config # layer contributes nothing, so ``get_active_catalogs`` moves on to the # next layer (this loader serves both the project and user configs; @@ -384,7 +393,7 @@ def _load_catalog_config( # is valid — fall back to built-in defaults. return None - entries: list[WorkflowCatalogEntry] = [] + entries: list[tuple[int, WorkflowCatalogEntry]] = [] for idx, item in enumerate(catalogs_data): if not isinstance(item, dict): raise WorkflowValidationError( @@ -424,15 +433,18 @@ def _load_catalog_config( else: install_allowed = bool(raw_install) entries.append( - WorkflowCatalogEntry( - url=url, - name=str(item.get("name", f"catalog-{idx + 1}")), - priority=priority, - install_allowed=install_allowed, - description=str(item.get("description", "")), + ( + idx, + WorkflowCatalogEntry( + url=url, + name=str(item.get("name", f"catalog-{idx + 1}")), + priority=priority, + install_allowed=install_allowed, + description=str(item.get("description", "")), + ), ) ) - entries.sort(key=lambda e: e.priority) + entries.sort(key=lambda item: item[1].priority) if not entries: raise WorkflowValidationError( f"Catalog config {config_path} contains {len(catalogs_data)} " @@ -821,7 +833,12 @@ def _coerce_priority(value: Any) -> int: return "added" def remove_catalog(self, index: int) -> str: - """Remove a catalog source by index (0-based). Returns the removed name.""" + """Remove a project source by its index in the active catalog list.""" + if os.environ.get("SPECKIT_WORKFLOW_CATALOG_URL", "").strip(): + raise WorkflowValidationError( + "The active catalog comes from SPECKIT_WORKFLOW_CATALOG_URL. " + "Unset that variable to remove a project catalog source." + ) config_path = self.project_root / ".specify" / "workflow-catalogs.yml" if not config_path.exists(): raise WorkflowValidationError("No catalog config file found.") @@ -832,25 +849,15 @@ def remove_catalog(self, index: int) -> str: raise WorkflowValidationError( f"Catalog config file is unreadable or malformed: {exc}" ) from exc - if data is None: - data = {} - elif not isinstance(data, dict): - raise WorkflowValidationError( - "Catalog config file is corrupted (expected a mapping)." - ) - catalogs = data.get("catalogs", []) - if not isinstance(catalogs, list): - raise WorkflowValidationError( - "Catalog config 'catalogs' must be a list." - ) + entries = self._parse_catalog_config(data, config_path) or [] - if index < 0 or index >= len(catalogs): + if index < 0 or index >= len(entries): raise WorkflowValidationError( - f"Catalog index {index} out of range (0-{len(catalogs) - 1})." + f"Catalog index {index} out of range (0-{len(entries) - 1})." ) - removed = catalogs.pop(index) - data["catalogs"] = catalogs + source_index, entry = entries[index] + data["catalogs"].pop(source_index) try: with open(config_path, "w", encoding="utf-8") as f: @@ -860,6 +867,4 @@ def remove_catalog(self, index: int) -> str: f"Failed to write catalog config {config_path}: {exc}" ) from exc - if isinstance(removed, dict): - return removed.get("name", f"catalog-{index + 1}") - return f"catalog-{index + 1}" + return entry.name diff --git a/src/specify_cli/workflows/step/catalog/_domain.py b/src/specify_cli/workflows/step/catalog/_domain.py index d1f9ecf75c..06192eef48 100644 --- a/src/specify_cli/workflows/step/catalog/_domain.py +++ b/src/specify_cli/workflows/step/catalog/_domain.py @@ -324,6 +324,15 @@ def _load_catalog_config( raise StepValidationError( f"Failed to read catalog config {config_path}: {exc}" ) from exc + indexed_entries = self._parse_catalog_config(data, config_path) + if indexed_entries is None: + return None + return [entry for _, entry in indexed_entries] + + def _parse_catalog_config( + self, data: Any, config_path: Path + ) -> list[tuple[int, StepCatalogEntry]] | None: + """Validate and order sources, retaining their original YAML positions.""" # Same two guards as WorkflowCatalog._load_catalog_config above, kept in # lockstep: this is the step-catalog twin of that loader and read the # same way. Dropping ``or {}`` stops a falsy non-mapping top level from @@ -347,7 +356,7 @@ def _load_catalog_config( if not catalogs_data: return None - entries: list[StepCatalogEntry] = [] + entries: list[tuple[int, StepCatalogEntry]] = [] for idx, item in enumerate(catalogs_data): if not isinstance(item, dict): raise StepValidationError( @@ -386,15 +395,18 @@ def _load_catalog_config( else: install_allowed = bool(raw_install) entries.append( - StepCatalogEntry( - url=url, - name=str(item.get("name", f"catalog-{idx + 1}")), - priority=priority, - install_allowed=install_allowed, - description=str(item.get("description", "")), + ( + idx, + StepCatalogEntry( + url=url, + name=str(item.get("name", f"catalog-{idx + 1}")), + priority=priority, + install_allowed=install_allowed, + description=str(item.get("description", "")), + ), ) ) - entries.sort(key=lambda e: e.priority) + entries.sort(key=lambda item: item[1].priority) if not entries: raise StepValidationError( f"Catalog config {config_path} contains {len(catalogs_data)} " @@ -770,7 +782,12 @@ def _coerce_priority(value: Any) -> int: return "added" def remove_catalog(self, index: int) -> str: - """Remove a catalog source by index (0-based). Returns the removed name.""" + """Remove a project source by its index in the active catalog list.""" + if os.environ.get("SPECKIT_STEP_CATALOG_URL", "").strip(): + raise StepValidationError( + "The active catalog comes from SPECKIT_STEP_CATALOG_URL. " + "Unset that variable to remove a project catalog source." + ) config_path = self.project_root / ".specify" / "step-catalogs.yml" if not config_path.exists(): raise StepValidationError("No step catalog config file found.") @@ -781,25 +798,15 @@ def remove_catalog(self, index: int) -> str: raise StepValidationError( f"Catalog config file is unreadable or malformed: {exc}" ) from exc - if data is None: - data = {} - elif not isinstance(data, dict): - raise StepValidationError( - "Catalog config file is corrupted (expected a mapping)." - ) - catalogs = data.get("catalogs", []) - if not isinstance(catalogs, list): - raise StepValidationError( - "Catalog config 'catalogs' must be a list." - ) + entries = self._parse_catalog_config(data, config_path) or [] - if index < 0 or index >= len(catalogs): + if index < 0 or index >= len(entries): raise StepValidationError( - f"Catalog index {index} out of range (0-{len(catalogs) - 1})." + f"Catalog index {index} out of range (0-{len(entries) - 1})." ) - removed = catalogs.pop(index) - data["catalogs"] = catalogs + source_index, entry = entries[index] + data["catalogs"].pop(source_index) try: with open(config_path, "w", encoding="utf-8") as f: @@ -811,6 +818,4 @@ def remove_catalog(self, index: int) -> str: f"Failed to write catalog config {config_path}: {exc}" ) from exc - if isinstance(removed, dict): - return removed.get("name", f"catalog-{index + 1}") - return f"catalog-{index + 1}" + return entry.name diff --git a/tests/specify_cli/workflows/catalog/test_command_remove.py b/tests/specify_cli/workflows/catalog/test_command_remove.py index 22eaa3891a..bff564cda2 100644 --- a/tests/specify_cli/workflows/catalog/test_command_remove.py +++ b/tests/specify_cli/workflows/catalog/test_command_remove.py @@ -1,6 +1,7 @@ """Tests for ``specify workflow catalog remove``.""" from typer.testing import CliRunner +import yaml from specify_cli import app from specify_cli.workflows.catalog import WorkflowCatalog @@ -17,3 +18,47 @@ def test_workflow_catalog_remove_deletes_selected_source(project_dir, monkeypatc assert all( config["name"] != "local" for config in catalog.get_catalog_configs() ) + + +def test_workflow_catalog_remove_uses_listed_index(project_dir, monkeypatch): + monkeypatch.chdir(project_dir) + monkeypatch.delenv("SPECKIT_WORKFLOW_CATALOG_URL", raising=False) + config = project_dir / ".specify" / "workflow-catalogs.yml" + data = {"notes": "keep", "catalogs": [ + {"name": "team", "url": "https://example.com/team.json", "priority": 10}, + {"name": "review", "url": "https://example.com/review.json", "priority": 1}, + ]} + config.write_text(yaml.safe_dump(data, sort_keys=False), encoding="utf-8") + runner = CliRunner() + listed = runner.invoke(app, ["workflow", "catalog", "list"]) + assert listed.exit_code == 0, listed.output + assert "[0] review" in listed.output + + result = runner.invoke(app, ["workflow", "catalog", "remove", "0"]) + + assert result.exit_code == 0, result.output + assert "'review' removed" in result.output + data["catalogs"].pop(1) + assert yaml.safe_load(config.read_text(encoding="utf-8")) == data + + +def test_workflow_catalog_remove_refuses_environment_source(project_dir, monkeypatch): + monkeypatch.chdir(project_dir) + config = project_dir / ".specify" / "workflow-catalogs.yml" + config.write_text( + "# keep\ncatalogs:\n - name: project\n" + " url: https://example.com/project.json\n", encoding="utf-8", + ) + before = config.read_bytes() + monkeypatch.setenv("SPECKIT_WORKFLOW_CATALOG_URL", "https://example.com/override.json") + runner = CliRunner() + listed = runner.invoke(app, ["workflow", "catalog", "list"]) + assert listed.exit_code == 0, listed.output + assert "[0] env-override" in listed.output + + result = runner.invoke(app, ["workflow", "catalog", "remove", "0"]) + + assert result.exit_code == 1, result.output + assert "SPECKIT_WORKFLOW_CATALOG_URL" in result.output + assert "Unset" in result.output + assert config.read_bytes() == before diff --git a/tests/specify_cli/workflows/step/catalog/test_command_remove.py b/tests/specify_cli/workflows/step/catalog/test_command_remove.py index 71ebf937c2..d2dd50b6bc 100644 --- a/tests/specify_cli/workflows/step/catalog/test_command_remove.py +++ b/tests/specify_cli/workflows/step/catalog/test_command_remove.py @@ -1,6 +1,7 @@ """Tests for ``specify workflow step catalog remove``.""" from typer.testing import CliRunner +import yaml from specify_cli import app from specify_cli.workflows.step.catalog import StepCatalog @@ -21,3 +22,47 @@ def test_workflow_step_catalog_remove_deletes_selected_source( assert all( config["name"] != "local" for config in catalog.get_catalog_configs() ) + + +def test_workflow_step_catalog_remove_uses_listed_index(project_dir, monkeypatch): + monkeypatch.chdir(project_dir) + monkeypatch.delenv("SPECKIT_STEP_CATALOG_URL", raising=False) + config = project_dir / ".specify" / "step-catalogs.yml" + data = {"notes": "keep", "catalogs": [ + {"name": "team", "url": "https://example.com/team.json", "priority": 10}, + {"name": "review", "url": "https://example.com/review.json", "priority": 1}, + ]} + config.write_text(yaml.safe_dump(data, sort_keys=False), encoding="utf-8") + runner = CliRunner() + listed = runner.invoke(app, ["workflow", "step", "catalog", "list"]) + assert listed.exit_code == 0, listed.output + assert "[0] review" in listed.output + + result = runner.invoke(app, ["workflow", "step", "catalog", "remove", "0"]) + + assert result.exit_code == 0, result.output + assert "'review' removed" in result.output + data["catalogs"].pop(1) + assert yaml.safe_load(config.read_text(encoding="utf-8")) == data + + +def test_workflow_step_catalog_remove_refuses_environment_source(project_dir, monkeypatch): + monkeypatch.chdir(project_dir) + config = project_dir / ".specify" / "step-catalogs.yml" + config.write_text( + "# keep\ncatalogs:\n - name: project\n" + " url: https://example.com/project.json\n", encoding="utf-8", + ) + before = config.read_bytes() + monkeypatch.setenv("SPECKIT_STEP_CATALOG_URL", "https://example.com/override.json") + runner = CliRunner() + listed = runner.invoke(app, ["workflow", "step", "catalog", "list"]) + assert listed.exit_code == 0, listed.output + assert "[0] env-override" in listed.output + + result = runner.invoke(app, ["workflow", "step", "catalog", "remove", "0"]) + + assert result.exit_code == 1, result.output + assert "SPECKIT_STEP_CATALOG_URL" in result.output + assert "Unset" in result.output + assert config.read_bytes() == before diff --git a/tests/test_workflows.py b/tests/test_workflows.py index bb83f3eda3..21cb2c684e 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -8981,6 +8981,86 @@ def test_load_symlinked_workflows_dir_fails_closed_not_silently_empty( class TestWorkflowCatalog: """Test WorkflowCatalog catalog resolution.""" + @pytest.fixture(params=["workflow", "step"]) + def removal_catalog(self, request, project_dir, monkeypatch): + from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError + from specify_cli.workflows.step.catalog import StepCatalog, StepValidationError + + kind = request.param + catalog_cls, error = ( + (WorkflowCatalog, WorkflowValidationError) + if kind == "workflow" else (StepCatalog, StepValidationError) + ) + env_key = f"SPECKIT_{kind.upper()}_CATALOG_URL" + monkeypatch.delenv(env_key, raising=False) + config = project_dir / ".specify" / f"{kind}-catalogs.yml" + return catalog_cls(project_dir), error, config, env_key + + @pytest.mark.parametrize("index,original_index", [(0, 2), (1, 4), (2, 3), (3, 1)]) + def test_remove_catalog_uses_listed_position( + self, removal_catalog, index, original_index + ): + catalog, _, config, _ = removal_catalog + data = {"notes": "keep", "catalogs": [ + {"name": "skipped", "url": " "}, + {"name": "duplicate", "url": "https://example.com/low.json", "priority": 10}, + {"name": "duplicate", "url": "https://example.com/first.json", "priority": "2"}, + {"url": "https://example.com/unnamed.json"}, + {"name": "duplicate", "url": "https://example.com/tied.json", "priority": 2}, + ]} + config.write_text(yaml.safe_dump(data, sort_keys=False), encoding="utf-8") + selected = catalog.get_catalog_configs()[index] + assert selected["url"] == data["catalogs"][original_index]["url"] + + assert catalog.remove_catalog(index) == selected["name"] + + data["catalogs"].pop(original_index) + assert yaml.safe_load(config.read_text(encoding="utf-8")) == data + + @pytest.mark.parametrize("index", [-1, 1]) + def test_remove_catalog_bounds_use_visible_entries(self, removal_catalog, index): + catalog, error, config, _ = removal_catalog + config.write_text( + "# keep this comment\ncatalogs:\n - name: skipped\n" + " - url: https://example.com/only.json\n", encoding="utf-8", + ) + before = config.read_bytes() + with pytest.raises(error, match="out of range"): + catalog.remove_catalog(index) + assert config.read_bytes() == before + + def test_remove_catalog_refuses_environment_source(self, removal_catalog, monkeypatch): + catalog, error, config, env_key = removal_catalog + catalog.add_catalog("https://example.com/project.json", "project") + before = config.read_bytes() + monkeypatch.setenv(env_key, " https://example.com/override.json ") + assert catalog.get_catalog_configs()[0]["name"] == "env-override" + + with pytest.raises(error, match=env_key) as exc: + catalog.remove_catalog(0) + assert "Unset" in str(exc.value) + assert config.read_bytes() == before + + def test_remove_catalog_ignores_blank_environment_override(self, removal_catalog, monkeypatch): + catalog, _, config, env_key = removal_catalog + catalog.add_catalog("https://example.com/project.json", "project") + monkeypatch.setenv(env_key, " \t ") + assert catalog.remove_catalog(0) == "project" + assert yaml.safe_load(config.read_text(encoding="utf-8"))["catalogs"] == [] + + @pytest.mark.parametrize("entry", ["invalid", {"name": "no-url"}, { + "url": "https://example.com/invalid.json", "priority": "invalid", + }]) + def test_remove_catalog_rejects_unlistable_config(self, removal_catalog, entry): + catalog, error, config, _ = removal_catalog + config.write_text(yaml.safe_dump({"catalogs": [entry]}), encoding="utf-8") + before = config.read_bytes() + with pytest.raises(error): + catalog.get_catalog_configs() + with pytest.raises(error): + catalog.remove_catalog(0) + assert config.read_bytes() == before + @pytest.mark.parametrize("catalog_type", ["workflow", "step"]) def test_non_mapping_cache_metadata_is_invalid( self, project_dir, catalog_type diff --git a/workflows/README.md b/workflows/README.md index ccca55aee9..0e26487d5d 100644 --- a/workflows/README.md +++ b/workflows/README.md @@ -522,10 +522,12 @@ specify workflow catalog list # Add a custom catalog specify workflow catalog add https://example.com/catalog.json --name my-org -# Remove a catalog +# Remove a project catalog by its index in the priority-ordered list specify workflow catalog remove ``` +Environment-provided sources cannot be removed this way; unset `SPECKIT_WORKFLOW_CATALOG_URL` to manage project sources. + ## Creating a Workflow 1. Create a `workflow.yml` following the schema above