Skip to content

feat: add catalog discovery CLI commands#2360

Open
Adr1an04 wants to merge 23 commits intogithub:mainfrom
Adr1an04:feat/2344-integration-catalog-cli
Open

feat: add catalog discovery CLI commands#2360
Adr1an04 wants to merge 23 commits intogithub:mainfrom
Adr1an04:feat/2344-integration-catalog-cli

Conversation

@Adr1an04
Copy link
Copy Markdown
Contributor

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 list
  • specify 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, and upgrade commands.

What changed

  • Added integration catalog search and info commands.
  • Added project-level integration catalog source management.
  • Added IntegrationCatalog.get_catalog_configs(), add_catalog(), and remove_catalog().
  • Added CLI coverage for search, info, catalog list/add/remove, and project guards.
  • Added unit coverage for catalog source management.

Final-entry removal behavior

A review pass caught that removing the final project catalog entry could leave this file behind:

catalogs: []

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:

.specify/integration-catalogs.yml

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

  • No specify integration add, remove, enable, disable, or set-priority commands were added.
  • Existing specify integration list, install, uninstall, switch, and upgrade behavior was not refactored.
  • _load_catalog_config() validation was not relaxed.
  • Workflow, extension, and preset catalog behavior was not changed.
  • catalog remove <index> remains 0-based, matching the issue request and the existing workflow catalog command shape.
  • catalog add writes project-level config only. It does not add user/global catalog management.

Test selection reasoning

Changed file Affects Test Why
src/specify_cli/__init__.py New integration CLI commands T1, T2, T4 Registers and handles integration search, info, and catalog list/add/remove
src/specify_cli/integrations/catalog.py Integration catalog source management T1, T2, T4 Adds catalog config list/add/remove behavior and final-entry deletion
tests/integrations/test_cli.py CLI regression coverage T1, T2, T4 Covers the new Typer commands and project guard behavior
tests/integrations/test_integration_catalog.py Library regression coverage T1, T2, T4 Covers catalog source management, validation, and built-in fallback behavior

Required tests

  • T1: focused integration catalog library and CLI regression tests
  • T2: full integration test suite
  • T3: agent config consistency test
  • T4: full pytest suite
  • T5: whitespace diff check

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.

git branch --show-current
# feat/2344-integration-catalog-cli

git status --short
# no output

git log --oneline -3
# 049ab8e feat: add catalog discovery CLI commands
# aad7b16 Add Spec Orchestrator extension to community catalog (#2350)
# 6cec171 chore: release 0.8.1, begin 0.8.2.dev0 development (#2356)

python -m pytest tests/integrations/test_integration_catalog.py tests/integrations/test_cli.py -v
# 90 passed in 4.61s

python -m pytest tests/integrations -q
# 941 passed in 52.14s

python -m pytest tests/test_agent_config_consistency.py -q
# 24 passed in 0.14s

python -m pytest -q
# 1629 passed, 125 skipped, 18 warnings in 60.71s

git diff --check
# no output

Command-level validation covered by tests

The automated CLI tests cover:

  • Running specify integration search in a spec-kit project.
  • Filtering search results by --tag.
  • Filtering search results by --author.
  • Showing a friendly no-results hint.
  • Marking discovery-only catalog entries as not directly installable.
  • Running specify integration info for catalog entries.
  • Falling back to built-in integration info when a built-in integration is not present in the catalog.
  • Listing built-in default catalog sources.
  • Adding a project-level catalog source.
  • Rejecting invalid non-HTTPS catalog URLs.
  • Rejecting duplicate catalog URLs.
  • Removing a catalog source by 0-based index.
  • Rejecting out-of-range catalog indexes.
  • Rejecting removal when no project catalog config exists.
  • Removing the final catalog entry and restoring built-in defaults.

Final-entry regression validation

The focused tests include both library-level and CLI-level coverage for the final-entry removal bug:

  • Unit coverage verifies remove_catalog(0) deletes .specify/integration-catalogs.yml when removing the only catalog entry.
  • Unit coverage verifies subsequent get_active_catalogs() returns the built-in default and community catalogs.
  • CLI coverage verifies catalog add followed by catalog remove 0 deletes the config file.
  • CLI coverage verifies a follow-up integration catalog list succeeds 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.

Copilot AI review requested due to automatic review settings April 24, 2026 21:07
@Adr1an04 Adr1an04 requested a review from mnriem as a code owner April 24, 2026 21:07
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 search and specify integration info for catalog-based discovery.
  • Adds specify integration catalog list/add/remove for 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.

Comment thread src/specify_cli/integrations/catalog.py Outdated
Comment thread src/specify_cli/integrations/catalog.py
Comment thread tests/integrations/test_cli.py
Comment thread src/specify_cli/integrations/catalog.py
@Adr1an04 Adr1an04 force-pushed the feat/2344-integration-catalog-cli branch from 049ab8e to 522acb8 Compare April 24, 2026 21:44
@Adr1an04 Adr1an04 requested a review from Copilot April 24, 2026 21:44
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/specify_cli/integrations/catalog.py Outdated
Comment on lines +523 to +527
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)."
)
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +495 to +499
max_priority = max(existing_priorities, default=0)
catalogs.append(
{
"name": name or f"catalog-{len(catalogs) + 1}",
"url": url,
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +447 to +453
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
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/specify_cli/integrations/catalog.py Outdated
# 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()
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
config_path.unlink()
try:
config_path.unlink()
except OSError as exc:
raise IntegrationValidationError(
f"Failed to delete catalog config {config_path}: {exc}"
) from exc

Copilot uses AI. Check for mistakes.
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.
"""
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
"""
"""
url = url.strip()

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/specify_cli/integrations/catalog.py Outdated
Comment on lines +546 to +566
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)
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread src/specify_cli/__init__.py Outdated
Comment on lines +2740 to +2749

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}")
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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}")

Copilot uses AI. Check for mistakes.
@Adr1an04 Adr1an04 force-pushed the feat/2344-integration-catalog-cli branch from ab52d2d to 6d86483 Compare April 24, 2026 23:06
@Adr1an04 Adr1an04 requested a review from Copilot April 24, 2026 23:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +2581 to +2584
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)
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread src/specify_cli/__init__.py Outdated

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.")
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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."
)

Copilot uses AI. Check for mistakes.
class IntegrationValidationError(IntegrationCatalogError):
"""Validation error for catalog config or catalog management operations."""


Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Suggested change
class IntegrationConfigError(IntegrationValidationError):
"""Raised when local catalog configuration or YAML content is invalid."""

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 156 to 162
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}"
)
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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.

Copilot uses AI. Check for mistakes.
Comment thread src/specify_cli/__init__.py Outdated
Comment on lines +2760 to +2763
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."""
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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.
"""

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/specify_cli/integrations/catalog.py Outdated
Comment on lines +516 to +522
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)
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +601 to +619
# 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]
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +471 to +482
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
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
@Adr1an04 Adr1an04 force-pushed the feat/2344-integration-catalog-cli branch from da57bb2 to 9a3bf32 Compare April 29, 2026 01:23
@Adr1an04 Adr1an04 requested a review from Copilot April 29, 2026 01:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/specify_cli/integrations/catalog.py Outdated
Comment on lines +704 to +710
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
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the removed entry has a missing/blank name, remove_catalog() falls back to catalog-{target_yaml_idx + 1}. Since removal is explicitly based on display order (sorted by priority), target_yaml_idx may not match the index the user passed/saw in catalog list, which can make the removal confirmation confusing in hand-edited configs. Consider deriving the fallback label from the display index (index + 1) and/or using the URL as the fallback identifier so the confirmation always matches the selected entry.

Suggested change
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
fallback_name = f"catalog-{index + 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
removed_url = removed.get("url")
if removed_url is not None:
normalized_url = str(removed_url).strip()
if normalized_url:
return normalized_url

Copilot uses AI. Check for mistakes.
Comment thread src/specify_cli/__init__.py Outdated
Comment on lines +2788 to +2791
if env_override or project_configs is None:
console.print(f" - [bold]{cfg.get('name', 'catalog')}[/bold] — {install_status}")
else:
console.print(f" [{i}] [bold]{cfg.get('name', f'catalog-{i + 1}')}[/bold] — {install_status}")
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

integration catalog list prints the raw name value from the catalog config. If a user hand-edits integration-catalogs.yml and sets name: to null/None or whitespace, the output will show None/blank and can disagree with the name echoed by catalog remove (which normalizes blanks). Consider normalizing the display name here (e.g., str(...).strip() with a catalog-{i+1} fallback) so list/remove are consistent and the index labels remain understandable.

Suggested change
if env_override or project_configs is None:
console.print(f" - [bold]{cfg.get('name', 'catalog')}[/bold] — {install_status}")
else:
console.print(f" [{i}] [bold]{cfg.get('name', f'catalog-{i + 1}')}[/bold] — {install_status}")
raw_name = cfg.get("name")
display_name = str(raw_name).strip() if raw_name is not None else ""
if not display_name:
display_name = f"catalog-{i + 1}"
if env_override or project_configs is None:
console.print(f" - [bold]{display_name}[/bold] — {install_status}")
else:
console.print(f" [{i}] [bold]{display_name}[/bold] — {install_status}")

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +566 to +572
max_priority = max(existing_priorities, default=0)
normalized_name = str(name).strip() if name is not None else ""
catalogs.append(
{
"name": normalized_name or f"catalog-{len(catalogs) + 1}",
"url": url,
"priority": max_priority + 1,
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When auto-generating a catalog name in add_catalog(), the code uses f"catalog-{len(catalogs) + 1}". Since catalogs can include blank-URL entries that are later skipped by _load_catalog_config(), this can produce surprising names like catalog-2 for the first actual active catalog. Consider deriving the default name from the count of valid/removable existing entries (or from the next display index) to keep generated names intuitive and consistent with list/remove behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +178 to 184
raw_name = item.get("name")
name = str(raw_name).strip() if raw_name is not None else ""
entries.append(
IntegrationCatalogEntry(
url=url,
name=str(item.get("name", f"catalog-{idx + 1}")),
name=name,
priority=priority,
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_load_catalog_config() currently strips name and allows it to become an empty string. That blank entry.name then propagates into _get_merged_integrations() as _catalog_name and into warning messages (e.g., "Could not fetch catalog ''"), so catalog provenance may disappear in integration search/info output. Consider synthesizing a non-empty default name for blank/None names (e.g., assign a generated name after sorting, or fall back to a stable value derived from URL/position) so entry.name is always meaningful.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Adr1an04
Copy link
Copy Markdown
Contributor Author

I rebased this branch onto the latest from main to keep it current and easier to review and merge @mnriem. I also fixed the latest Copilot comments that came about catalog source display and fallback names, and the newest.

For validation, I ran the integration catalog tests, CLI tests, focused catalog and CLI combined tests, the full integration suite, tests/test_agent_config_consistency.py -q, and the full pytest suite. I also manually tested and looked for any additional edge cases. Everything passed, let me know if you want me to update anything else.

I'll be on the lookout if copilot makes more comments on your run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(integrations): add catalog discovery CLI commands (search, info, catalog list/add/remove)

3 participants