feat: add catalog discovery CLI commands#2360
Conversation
There was a problem hiding this comment.
Pull request overview
Adds first-class integration catalog discovery and project-level catalog source management to the Specify CLI, aligning integrations with the existing catalog UX used by workflows/extensions while keeping integration install/switch behavior unchanged.
Changes:
- Introduces
specify integration searchandspecify integration infofor catalog-based discovery. - Adds
specify integration catalog list/add/removefor project-scoped catalog source management. - Adds integration catalog source management APIs (
get_catalog_configs,add_catalog,remove_catalog) with new unit + CLI tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/specify_cli/__init__.py |
Registers new integration search/info and integration catalog ... Typer commands and project guard. |
src/specify_cli/integrations/catalog.py |
Adds catalog source management API and a validation error type; handles deleting config on final-entry removal. |
tests/integrations/test_cli.py |
Adds end-to-end CLI coverage for search/info/catalog list/add/remove flows and project guard behavior. |
tests/integrations/test_integration_catalog.py |
Adds unit coverage for catalog source stack, add/remove behavior, and final-entry deletion fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adr1an04
force-pushed
the
feat/2344-integration-catalog-cli
branch
from
049ab8e to
522acb8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {} | ||
| if not isinstance(data, dict): | ||
| raise IntegrationValidationError( | ||
| "Catalog config file is corrupted (expected a mapping)." | ||
| ) |
There was a problem hiding this comment.
remove_catalog() reads/parses YAML without any error handling. If integration-catalogs.yml exists but has invalid YAML (or can't be read), yaml.safe_load/read_text will raise and bypass the CLI's IntegrationCatalogError handling.
Match _load_catalog_config by catching yaml.YAMLError, OSError, UnicodeError, etc., and raise IntegrationValidationError with a clear message.
| max_priority = max(existing_priorities, default=0) | ||
| catalogs.append( | ||
| { | ||
| "name": name or f"catalog-{len(catalogs) + 1}", | ||
| "url": url, |
There was a problem hiding this comment.
Priority derivation only considers entries that explicitly include a priority field (existing_priorities). If the config contains valid entries without priority (allowed by _load_catalog_config, which defaults to idx + 1), max_priority can be computed too low and the new catalog may not sort last as intended.
Derive max_priority using the same defaulting logic as _load_catalog_config (treat missing priority as idx + 1) to keep ordering predictable even for hand-edited files.
| if config_path.exists(): | ||
| raw = yaml.safe_load(config_path.read_text(encoding="utf-8")) | ||
| if not isinstance(raw, dict): | ||
| raise IntegrationValidationError( | ||
| "Catalog config file is corrupted (expected a mapping)." | ||
| ) | ||
| data = raw |
There was a problem hiding this comment.
add_catalog() uses yaml.safe_load(...) without catching yaml.YAMLError/I/O/Unicode errors (unlike _load_catalog_config). If the existing config file contains invalid YAML, this will bubble up as an unhandled exception and the CLI will crash instead of returning a clean IntegrationCatalogError/IntegrationValidationError.
Consider wrapping the read/parse in a try/except (same exceptions as _load_catalog_config) and re-raising as IntegrationValidationError with the path in the message.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Deleting the file lets the project fall back to built-in | ||
| # defaults, which matches the behavior before any | ||
| # `catalog add` was ever run. | ||
| config_path.unlink() |
There was a problem hiding this comment.
remove_catalog() deletes the config file with config_path.unlink() but doesn’t handle filesystem errors (permissions, race with another process, etc.). Since the rest of the method wraps IO/YAML errors into IntegrationValidationError, consider wrapping the unlink in a try/except and raising IntegrationValidationError on failure (or use missing_ok=True plus explicit error handling).
| config_path.unlink() | |
| try: | |
| config_path.unlink() | |
| except OSError as exc: | |
| raise IntegrationValidationError( | |
| f"Failed to delete catalog config {config_path}: {exc}" | |
| ) from exc |
| The URL is validated before being written. Duplicate URLs are rejected. | ||
| Priority is derived as ``max(existing) + 1`` so the new entry sorts last | ||
| in the resolution order unless the user edits the file manually. | ||
| """ |
There was a problem hiding this comment.
add_catalog() doesn’t normalize/strip the incoming url before storing it and before duplicate checks. This allows near-duplicates like https://example.com/cat.json vs https://example.com/cat.json to be added as distinct entries, and _load_catalog_config() will later strip them to the same URL (effectively creating duplicate sources). Normalize with url = url.strip() (and use the normalized value for validation, comparisons, and writing).
| """ | |
| """ | |
| url = url.strip() |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| catalogs = data.get("catalogs", []) | ||
| if not isinstance(catalogs, list): | ||
| raise IntegrationValidationError( | ||
| "Catalog config 'catalogs' must be a list." | ||
| ) | ||
|
|
||
| if not catalogs: | ||
| # An empty list is the kind of state that only happens if the | ||
| # user hand-edited the file; our own `remove_catalog` deletes | ||
| # the file when the last entry is popped. Surface a clear | ||
| # message instead of `out of range (0--1)`. | ||
| raise IntegrationValidationError( | ||
| "Catalog config contains no catalog entries." | ||
| ) | ||
|
|
||
| if index < 0 or index >= len(catalogs): | ||
| raise IntegrationValidationError( | ||
| f"Catalog index {index} out of range (0-{len(catalogs) - 1})." | ||
| ) | ||
|
|
||
| removed = catalogs.pop(index) |
There was a problem hiding this comment.
integration catalog list enumerates catalogs in the active order (sorted by priority via _load_catalog_config()), but remove_catalog() removes by the raw YAML list position. If a user hand-edits priorities (or reorders entries), the index shown by catalog list can end up deleting a different entry than the user selected. Consider removing by the same ordering you display (e.g., sort entries by derived priority, map the chosen displayed index back to the original YAML index before popping), or alternatively make catalog list preserve file order for project configs.
|
|
||
| try: | ||
| catalog.add_catalog(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: {url}") |
There was a problem hiding this comment.
add_catalog() normalizes the URL with strip() before writing, but the success message prints the original url argument. If the user includes surrounding whitespace, the CLI will report a different URL than what was actually persisted and what duplicate detection uses. Consider printing the normalized value (or the URL read back from the updated config) so the output matches on-disk state.
| try: | |
| catalog.add_catalog(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: {url}") | |
| normalized_url = url.strip() | |
| try: | |
| 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}") |
Adr1an04
force-pushed
the
feat/2344-integration-catalog-cli
branch
from
ab52d2d to
6d86483
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except IntegrationCatalogError as exc: | ||
| console.print(f"[red]Error:[/red] {exc}") | ||
| console.print("\nTip: The catalog may be temporarily unavailable. Try again later.") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
The error handler prints a network-oriented hint ("catalog may be temporarily unavailable") for every IntegrationCatalogError, but IntegrationCatalogError is also raised for local config/YAML validation failures (e.g., invalid .specify/integration-catalogs.yml). This can mislead users into retrying instead of fixing the config. Consider catching IntegrationValidationError separately (or narrowing the hint to fetch/URLError cases) and tailoring the guidance accordingly.
|
|
||
| if catalog_error: | ||
| console.print(f"[red]Error:[/red] Could not query integration catalog: {catalog_error}") | ||
| console.print("\nTry again when online, or use a built-in integration ID directly.") |
There was a problem hiding this comment.
This branch suggests "Try again when online" whenever the catalog lookup failed, but catalog failures can also be caused by a malformed local catalog config file. Consider adjusting the guidance to mention checking .specify/integration-catalogs.yml (or distinguishing validation vs fetch errors) so users get actionable next steps.
| console.print("\nTry again when online, or use a built-in integration ID directly.") | |
| console.print( | |
| "\nCheck your network connection or verify that .specify/integration-catalogs.yml is valid, " | |
| "or use a built-in integration ID directly." | |
| ) |
| class IntegrationValidationError(IntegrationCatalogError): | ||
| """Validation error for catalog config or catalog management operations.""" | ||
|
|
||
|
|
There was a problem hiding this comment.
IntegrationValidationError is introduced, but _load_catalog_config() still raises the broader IntegrationCatalogError for config/YAML validation problems. That makes it hard for callers (especially the CLI) to distinguish “local config is invalid” from “catalog fetch failed” and provide accurate guidance. Consider raising IntegrationValidationError (or a dedicated config error subclass) from config parsing/validation paths in _load_catalog_config().
| class IntegrationConfigError(IntegrationValidationError): | |
| """Raised when local catalog configuration or YAML content is invalid.""" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| priority = int(item.get("priority", idx + 1)) | ||
| except (TypeError, ValueError): | ||
| raise IntegrationCatalogError( | ||
| raise IntegrationValidationError( | ||
| f"Invalid priority for catalog '{item.get('name', idx + 1)}': " | ||
| f"expected integer, got {item.get('priority')!r}" | ||
| ) |
There was a problem hiding this comment.
_load_catalog_config parses priority via int(...), which will silently accept YAML booleans (true/false) as 1/0. add_catalog explicitly rejects boolean priorities, so a config with priority: true would work for catalog list/search but then block catalog add. Consider explicitly rejecting bool here as well (e.g., check isinstance(raw_priority, bool) before casting) so validation is consistent and avoids surprising YAML bool coercion.
| url: str = typer.Argument(..., help="Catalog URL to add (must use HTTPS)"), | ||
| name: Optional[str] = typer.Option(None, "--name", help="Catalog name"), | ||
| ): | ||
| """Add an integration catalog source to the project config.""" |
There was a problem hiding this comment.
The CLI help text says the catalog URL "must use HTTPS", but _validate_catalog_url also allows http:// for localhost. Update the argument help (and/or command help) to mention the localhost exception so users aren’t misled during local testing.
| url: str = typer.Argument(..., help="Catalog URL to add (must use HTTPS)"), | |
| name: Optional[str] = typer.Option(None, "--name", help="Catalog name"), | |
| ): | |
| """Add an integration catalog source to the project config.""" | |
| url: str = typer.Argument( | |
| ..., | |
| help="Catalog URL to add (must use HTTPS, except http://localhost for local testing)", | |
| ), | |
| name: Optional[str] = typer.Option(None, "--name", help="Catalog name"), | |
| ): | |
| """Add an integration catalog source to the project config. | |
| Catalog URLs must use HTTPS, except that ``http://localhost`` is allowed | |
| for local testing. | |
| """ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isinstance(raw_priority, bool) or not isinstance(raw_priority, int): | ||
| raise IntegrationValidationError( | ||
| f"Invalid catalog entry at index {idx}: " | ||
| f"'priority' must be an integer, got " | ||
| f"{type(raw_priority).__name__}." | ||
| ) | ||
| existing_priorities.append(raw_priority) |
There was a problem hiding this comment.
add_catalog() rejects existing entries whose priority is a numeric string (e.g. '10') or other int-coercible values, but _load_catalog_config() accepts those via int(raw_priority). This inconsistency can make a config file that loads fine suddenly block catalog add. Consider normalizing with the same int(...) conversion logic as _load_catalog_config() (still rejecting bool) so add/remove/list behave consistently on hand-edited YAML.
| if isinstance(raw_priority, bool) or not isinstance(raw_priority, int): | |
| raise IntegrationValidationError( | |
| f"Invalid catalog entry at index {idx}: " | |
| f"'priority' must be an integer, got " | |
| f"{type(raw_priority).__name__}." | |
| ) | |
| existing_priorities.append(raw_priority) | |
| if isinstance(raw_priority, bool): | |
| raise IntegrationValidationError( | |
| f"Invalid catalog entry at index {idx}: " | |
| f"'priority' must be an integer, got " | |
| f"{type(raw_priority).__name__}." | |
| ) | |
| try: | |
| normalized_priority = int(raw_priority) | |
| except (TypeError, ValueError): | |
| raise IntegrationValidationError( | |
| f"Invalid catalog entry at index {idx}: " | |
| f"'priority' must be an integer, got " | |
| f"{type(raw_priority).__name__}." | |
| ) from None | |
| existing_priorities.append(normalized_priority) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Map displayed index -> raw YAML index using the same priority | ||
| # defaulting as ``_load_catalog_config``. We deliberately stay | ||
| # tolerant here (no new validation errors) because the goal is | ||
| # only to mirror the order shown by ``catalog list``; entries | ||
| # that ``_load_catalog_config`` would have rejected outright | ||
| # would have failed ``catalog list`` already. | ||
| priority_pairs: List[Tuple[int, int]] = [] | ||
| for yaml_idx, item in enumerate(catalogs): | ||
| if isinstance(item, dict): | ||
| try: | ||
| priority = int(item.get("priority", yaml_idx + 1)) | ||
| except (TypeError, ValueError): | ||
| priority = yaml_idx + 1 | ||
| else: | ||
| priority = yaml_idx + 1 | ||
| priority_pairs.append((priority, yaml_idx)) | ||
| # Stable sort: ties keep their YAML order, matching list-view ordering. | ||
| priority_pairs.sort(key=lambda p: p[0]) | ||
| display_order: List[int] = [yaml_idx for _, yaml_idx in priority_pairs] |
There was a problem hiding this comment.
remove_catalog() builds display_order from every raw YAML entry, but catalog list (via _load_catalog_config) skips entries whose url is missing/empty after stripping. If a config contains a blank-url entry plus valid entries, catalog list indices won’t match remove_catalog indices, so catalog remove 0 can delete a different entry than the one shown at index 0. To keep behavior consistent, mirror _load_catalog_config’s skip rule when building display_order (ignore entries with empty/whitespace-only url), or compute the display list via _load_catalog_config and map back to the YAML entry by URL/name before popping.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if config_path.exists(): | ||
| try: | ||
| raw = yaml.safe_load(config_path.read_text(encoding="utf-8")) | ||
| except (yaml.YAMLError, OSError, UnicodeError) as exc: | ||
| raise IntegrationValidationError( | ||
| f"Failed to read catalog config {config_path}: {exc}" | ||
| ) from exc | ||
| if not isinstance(raw, dict): | ||
| raise IntegrationValidationError( | ||
| "Catalog config file is corrupted (expected a mapping)." | ||
| ) | ||
| data = raw |
There was a problem hiding this comment.
In add_catalog, yaml.safe_load() can return None for an empty config file, which then triggers the "corrupted (expected a mapping)" error. Other catalog config readers in this repo coerce None to {} (e.g., yaml.safe_load(... ) or {}), which would let catalog add recover gracefully from an empty file instead of forcing the user to delete it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raise IntegrationValidationError( | ||
| f"Invalid catalog entry at index {idx}: " | ||
| f"'priority' must be an integer, got " | ||
| f"{type(raw_priority).__name__}." | ||
| ) from None |
There was a problem hiding this comment.
In add_catalog, the non-integer priority validation error only reports the type (e.g. "got str") rather than the actual value. This makes it harder for users to diagnose a bad hand-edited config. Consider including the offending value (repr) in the message, similar to _load_catalog_config’s raw_priority!r formatting.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {} | ||
| except (yaml.YAMLError, OSError, UnicodeError) as exc: | ||
| raise IntegrationCatalogError( | ||
| raise IntegrationValidationError( | ||
| f"Failed to read catalog config {config_path}: {exc}" | ||
| ) | ||
| ) from exc | ||
| if not isinstance(data, dict): | ||
| raise IntegrationCatalogError( | ||
| raise IntegrationValidationError( | ||
| f"Invalid catalog config {config_path}: expected a YAML mapping at the root" | ||
| ) |
There was a problem hiding this comment.
yaml.safe_load(... ) or {} will coerce falsy non-mapping YAML roots (e.g. [], false, 0, '') into {}, which then triggers the “contains no 'catalogs' entries” path instead of correctly reporting an invalid root type. Consider only defaulting to {} when the parsed value is None, preserving other root types so expected a YAML mapping at the root is raised for []/false/0/... configs.
| try: | ||
| data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {} | ||
| except (yaml.YAMLError, OSError, UnicodeError) as exc: | ||
| raise IntegrationValidationError( | ||
| f"Failed to read catalog config {config_path}: {exc}" | ||
| ) from exc | ||
| if not isinstance(data, dict): | ||
| raise IntegrationValidationError( | ||
| "Catalog config file is corrupted (expected a mapping)." | ||
| ) |
There was a problem hiding this comment.
Same issue here: yaml.safe_load(... ) or {} will turn falsy non-dict YAML roots into {}, so a corrupted config like [] won’t be detected as “expected a mapping” and instead falls into the empty-catalog branch. Suggest parsing into a raw variable and only replacing with {} when raw is None before the isinstance(..., dict) check.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except IntegrationValidationError as exc: | ||
| console.print(f"[red]Error:[/red] {exc}") | ||
| console.print( | ||
| "\nTip: Check .specify/integration-catalogs.yml for invalid catalog configuration." |
There was a problem hiding this comment.
The local-config failure tip hardcodes .specify/integration-catalogs.yml, but IntegrationCatalog.get_active_catalogs() can also fail on the user-level config at ~/.specify/integration-catalogs.yml. When that happens, this guidance points users to the wrong file. Consider wording the tip to reference the path in the exception (already included), or mention both project and user config locations.
| "\nTip: Check .specify/integration-catalogs.yml for invalid catalog configuration." | |
| "\nTip: Check the configuration file path shown above for invalid catalog configuration " | |
| "(for example, .specify/integration-catalogs.yml or ~/.specify/integration-catalogs.yml)." |
| console.print(f"[red]Error:[/red] Could not query integration catalog: {catalog_error}") | ||
| if isinstance(catalog_error, IntegrationValidationError): | ||
| console.print( | ||
| "\nCheck .specify/integration-catalogs.yml, " |
There was a problem hiding this comment.
Similar to integration search, this local-config guidance hardcodes the project path .specify/integration-catalogs.yml, but the thrown IntegrationValidationError may be for the user-level config (~/.specify/...). Suggest adjusting the message to reference the failing config path (included in the error) or mention both possible locations.
| "\nCheck .specify/integration-catalogs.yml, " | |
| "\nCheck .specify/integration-catalogs.yml or " | |
| "~/.specify/integration-catalogs.yml, " |
| def integration_catalog_add( | ||
| url: str = typer.Argument( | ||
| ..., | ||
| help="Catalog URL to add (HTTPS required, except http://localhost for local testing)", |
There was a problem hiding this comment.
The add command help text says HTTP is only allowed for http://localhost, but _validate_catalog_url also allows http://127.0.0.1 and http://[::1]. Update the help string to match the actual validation rules so users aren't misled.
| help="Catalog URL to add (HTTPS required, except http://localhost for local testing)", | |
| help="Catalog URL to add (HTTPS required, except http://localhost, http://127.0.0.1, or http://[::1] for local testing)", |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| catalogs_data = data.get("catalogs", []) | ||
| if not isinstance(catalogs_data, list): | ||
| raise IntegrationCatalogError( | ||
| raise IntegrationValidationError( | ||
| f"Invalid catalog config: 'catalogs' must be a list, " | ||
| f"got {type(catalogs_data).__name__}" | ||
| ) |
There was a problem hiding this comment.
_load_catalog_config() raises several IntegrationValidationErrors that don't include config_path in the message (e.g., when 'catalogs' is the wrong type). This makes the CLI tip "check the configuration file path shown above" unreliable because the path may not actually be shown. Include the file path in these validation messages (and ideally for invalid entry errors too) so users can immediately locate the failing config file.
| raise typer.Exit(1) | ||
| except IntegrationCatalogError as exc: | ||
| console.print(f"[red]Error:[/red] {exc}") | ||
| console.print("\nTip: The catalog may be temporarily unavailable. Try again later.") |
There was a problem hiding this comment.
The generic except IntegrationCatalogError branch prints a network-oriented tip ("catalog may be temporarily unavailable"), but an invalid SPECKIT_INTEGRATION_CATALOG_URL currently raises IntegrationCatalogError (not IntegrationValidationError) in IntegrationCatalog.get_active_catalogs(). That means env-var misconfiguration will incorrectly show the network tip instead of local-config guidance. Consider re-wrapping env-var URL validation failures as IntegrationValidationError, or explicitly detecting env-var failures here and showing the local-config/env-var fix tip.
| console.print("\nTip: The catalog may be temporarily unavailable. Try again later.") | |
| if os.environ.get("SPECKIT_INTEGRATION_CATALOG_URL"): | |
| console.print( | |
| "\nTip: Check the SPECKIT_INTEGRATION_CATALOG_URL environment variable for an invalid " | |
| "catalog URL, or unset it to use the configured catalog files " | |
| "(.specify/integration-catalogs.yml or ~/.specify/integration-catalogs.yml)." | |
| ) | |
| else: | |
| console.print("\nTip: The catalog may be temporarily unavailable. Try again later.") |
| if isinstance(removed, dict): | ||
| return removed.get("name", f"catalog-{target_yaml_idx + 1}") | ||
| return f"catalog-{target_yaml_idx + 1}" |
There was a problem hiding this comment.
remove_catalog() can return a non-string (or an empty/None name) because it directly returns removed.get('name', ...) without normalization. That can lead to user-facing output like Catalog source 'None' removed. Consider coercing to str and falling back when the value is missing/blank, so the CLI always gets a sensible display name.
| if isinstance(removed, dict): | |
| return removed.get("name", f"catalog-{target_yaml_idx + 1}") | |
| return f"catalog-{target_yaml_idx + 1}" | |
| fallback_name = f"catalog-{target_yaml_idx + 1}" | |
| if isinstance(removed, dict): | |
| removed_name = removed.get("name") | |
| if removed_name is not None: | |
| normalized_name = str(removed_name).strip() | |
| if normalized_name: | |
| return normalized_name | |
| return fallback_name |
| @integration_catalog_app.command("list") | ||
| def integration_catalog_list(): | ||
| """List configured integration catalog sources.""" | ||
| from .integrations.catalog import IntegrationCatalog, IntegrationCatalogError | ||
|
|
||
| project_root = _require_specify_project() | ||
| catalog = IntegrationCatalog(project_root) | ||
|
|
||
| try: | ||
| configs = catalog.get_catalog_configs() | ||
| except IntegrationCatalogError as exc: |
There was a problem hiding this comment.
integration catalog list currently renders the active catalog stack via get_active_catalogs() (env override → project config → user config → built-ins), but catalog add/remove only operate on the project-level file. If a user has a user-level config (or an env override), catalog list will show indexes that catalog remove <index> cannot act on (it will error with “No catalog config file found.”). Consider either (a) making catalog list show only the project-level config entries (plus a separate built-in section), or (b) explicitly labeling each entry’s source (env/project/user/built-in) and restricting/remapping indexes so remove applies only to removable project entries.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| existing_url = cat.get("url") | ||
| if not isinstance(existing_url, str) or not existing_url.strip(): | ||
| raise IntegrationValidationError( | ||
| f"Invalid catalog entry at index {idx}: " | ||
| f"'url' must be a non-empty string." | ||
| ) |
There was a problem hiding this comment.
IntegrationCatalog.add_catalog() currently rejects any existing config entry whose url is blank/whitespace-only. However _load_catalog_config() explicitly skips blank-URL entries (treating them as non-active), and remove_catalog() also treats them as non-removable/skipped. This makes catalog add fail on configs that the rest of the integration catalog stack considers valid. Consider treating blank-URL entries the same way as _load_catalog_config() (skip them for validation/dup-check/priority derivation) instead of raising.
| existing_url = existing_url.strip() | ||
| # Re-run the same URL validation used when loading, so a corrupt | ||
| # entry surfaces here instead of at the next `integration` call. | ||
| self._validate_catalog_url(existing_url) |
There was a problem hiding this comment.
When validating existing entries in add_catalog(), _validate_catalog_url(existing_url) can raise IntegrationCatalogError without indicating which on-disk config entry caused it (no file path / index context). Since this is a local-config problem, consider catching that exception and re-raising IntegrationValidationError that includes config_path and the entry index (similar to _load_catalog_config()), so users can quickly locate and fix the offending entry.
| self._validate_catalog_url(existing_url) | |
| try: | |
| self._validate_catalog_url(existing_url) | |
| except IntegrationCatalogError as exc: | |
| raise IntegrationValidationError( | |
| f"Invalid catalog entry at index {idx} in " | |
| f"{config_path}: {exc}" | |
| ) from exc |
| try: | ||
| project_configs = catalog.get_project_catalog_configs() | ||
| configs = project_configs if project_configs is not None else catalog.get_catalog_configs() | ||
| except IntegrationCatalogError as exc: |
There was a problem hiding this comment.
integration catalog list prefers get_project_catalog_configs() whenever a project config file exists, which means it will ignore the higher-precedence SPECKIT_INTEGRATION_CATALOG_URL override in that case (even though IntegrationCatalog.get_active_catalogs() will use the env var). This can make the command output disagree with the actual catalog sources used by integration search/info. Consider always showing the active stack (or at least explicitly detecting the env override and noting that it supersedes project config).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
While it is quite a big PR this adds catalog based integration discovery and project level catalog source management to the Specify CLI. In practice, that means users can now search for integrations, view integration details, list configured catalogs, and add or remove project-specific catalog sources. The main goal is to make integrations easier to discover through catalogs without changing the existing install, switch, or uninstall behavior. Those commands still work the same way, and integrations remain single-active as before. I addressed the Copilot feedback with focused catalog fixes: clearer config/YAML errors, explicit source precedence, corrected catalog list behavior, matching remove-by-index handling, and cleanup for invalid URLs, priorities, empty configs, and final-entry removal. Scope remains limited to discovery and catalog source management. Broader catalog-backed install behavior is out of scope so I didn't touch it. I validated with focused catalog/CLI tests, integrations, agent config consistency, full pytest, targeted runs, and whitespace checks. Thank you and please let me know if I should change anything!! |
This PR Closes #2344.
Summary
This PR adds integration catalog discovery and catalog-source management commands for the Specify CLI.
It introduces:
specify integration search [query] [--tag T] [--author A]specify integration info <id>specify integration catalog listspecify integration catalog add <url> [--name N]specify integration catalog remove <index>The implementation follows the existing workflow catalog CLI shape while keeping integration behavior separate from workflows/extensions/presets. Integrations remain single-active through the existing
install,uninstall,switch, andupgradecommands.What changed
IntegrationCatalog.get_catalog_configs(),add_catalog(), andremove_catalog().Final-entry removal behavior
A review pass caught that removing the final project catalog entry could leave this file behind:
That would cause later integration commands to fail, because
_load_catalog_config()rejects an existing config file with no catalog entries.This PR now handles that case by deleting:
If catalog entries remain after removal, the file is written normally. If no entries remain, the file is removed and the project falls back to the built-in default catalogs.
What did not change
specify integration add,remove,enable,disable, orset-prioritycommands were added.specify integration list,install,uninstall,switch, andupgradebehavior was not refactored._load_catalog_config()validation was not relaxed.catalog remove <index>remains 0-based, matching the issue request and the existing workflow catalog command shape.catalog addwrites project-level config only. It does not add user/global catalog management.Test selection reasoning
src/specify_cli/__init__.pyintegration search,info, andcatalog list/add/removesrc/specify_cli/integrations/catalog.pytests/integrations/test_cli.pytests/integrations/test_integration_catalog.pyRequired tests
Testing
I ran the focused catalog/CLI regression tests, the full integration suite, agent config consistency, the full pytest suite, and a whitespace diff check from native PowerShell on Windows.
Command-level validation covered by tests
The automated CLI tests cover:
Final-entry regression validation
The focused tests include both library-level and CLI-level coverage for the final-entry removal bug:
remove_catalog(0)deletes.specify/integration-catalogs.ymlwhen removing the only catalog entry.get_active_catalogs()returns the built-indefaultandcommunitycatalogs.catalog addfollowed bycatalog remove 0deletes the config file.integration catalog listsucceeds and shows the built-in defaults.AI disclosure
I used ChatGPT/Cursor as a coding and review assistant while working on this PR. I implemented and reviewed the final changes myself, and used the tools to help investigate the issue, draft parts of the tests, and polish the PR description. I also ran the validation commands and verified the final diff and test results before submitting this PR.