Skip to content

feat(cli): add plugin catalog core#618

Open
johnnygreco wants to merge 10 commits intomainfrom
johnny/feat/617/plugin-catalog-cli-core
Open

feat(cli): add plugin catalog core#618
johnnygreco wants to merge 10 commits intomainfrom
johnny/feat/617/plugin-catalog-cli-core

Conversation

@johnnygreco
Copy link
Copy Markdown
Contributor

📋 Summary

This PR adds the Data Designer core CLI support for plugin taps so users can discover, inspect, and install plugins from catalog sources while preserving the existing command-controller-service-repository CLI pattern. It also aligns the core implementation with the schema v2 tap contract from NVIDIA-NeMo/DataDesignerPlugins#36, including strict catalog validation, install-plan rendering, compatibility checks, and post-install entry point verification.

🔗 Related Issue

Closes #617

🔄 Changes

✨ Added

  • Added data-designer plugins commands for list, search, info, install, installed, and taps list/add/remove.
  • Added typed plugin catalog, tap config, compatibility, install plan, and installed entry point models.
  • Added plugin tap persistence and URL-keyed catalog caching under ~/.data-designer/.
  • Added install plan generation for PyPI, Git PEP 508 direct references, and local path sources.
  • Added CLI developer README coverage for plugin tap files, cache behavior, and plugin command examples.

🔧 Changed

  • Wired the plugins command group through the existing lazy Typer CLI registration in main.py.
  • Added packaging>=24,<27 for version, requirement, specifier, marker, and package-name handling.
  • Made the built-in NVIDIA tap URL overridable via DATA_DESIGNER_DEFAULT_PLUGIN_TAP_URL for QA/staging.
  • Made tap aliases case-insensitive and made plugins installed list entry point metadata without importing plugin modules.

🐛 Fixed

  • Strictly validate schema v2 catalog payloads to match the DataDesignerPlugins tap contract, including source union shapes, docs URLs, compatibility metadata, package paths, and duplicate runtime plugin names.
  • Prefer newest compatible catalog entries before falling back to forced incompatible installs.
  • Invalidate import caches before post-install runtime verification.
  • Report invalid tap aliases as concise CLI errors instead of raw Pydantic validation output.
  • Avoid stale cache reuse when an alias is repointed to a different URL.

🧪 Tests

  • Added command, controller, repository, catalog service, and install service coverage for tap management, schema validation, compatibility selection, install planning, post-install verification, and CLI delegation.

🔍 Attention Areas

⚠️ Reviewers: Please pay special attention to the following:

  • plugin_catalog.py — strict schema v2 validation is the main compatibility boundary with the plugin tap repository.
  • plugin_tap_repository.py — handles URL normalization, remote/local catalog loading, fallback cache behavior, and URL-keyed cache files.
  • plugin_install_service.py — constructs and runs package-manager commands, including the exact Git/path install targets.
  • pyproject.toml and uv.lock — add and resolve the new packaging runtime dependency for the CLI package.

🧪 Testing

  • make test passes — not run; scoped CLI coverage below passed
  • Unit tests added/updated
  • E2E tests added/updated: N/A — no dedicated E2E harness, but a live PR36 tap smoke test was run

Additional verification run locally:

  • uv run ruff check packages/data-designer/src/data_designer/cli packages/data-designer/tests/cli
  • uv run --package data-designer pytest packages/data-designer/tests/cli
  • git diff --check
  • Loaded schema v2 catalog from NVIDIA-NeMo/DataDesignerPlugins PR fix: pulling default provider name from default config file and use it #36 raw catalog SHA 0c378546ad1d3bc7e24f0673d558f91a72ae9ce5

✅ Checklist

  • Follows commit message conventions
  • Commits are signed off (DCO) — pending author confirmation/signoff
  • Architecture docs updated (if applicable) — CLI developer README updated

@johnnygreco johnnygreco requested a review from a team as a code owner May 7, 2026 20:58
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Code Review: PR #618 — feat(cli): add plugin catalog core

Summary

Adds a data-designer plugins command group that lets users browse, search, inspect, and install plugins from "tap" catalogs (built-in NVIDIA tap plus user-added aliases), plus a strict schema-v2 validator that matches the DataDesignerPlugins PR #36 contract. The implementation follows the repo's existing command → controller → service → repository CLI layering, adds packaging>=24,<27 as a runtime dep, and ships ~850 lines of test coverage across the four new layers.

Overall the PR is well-structured, strictly validated, and thoughtfully tested. Comments below are mostly minor; one high-value item (entry point group constant usage) and a few smaller polish items.

Findings

Correctness

  • plugin_catalog.py:900 and plugin_catalog_service.py:1796 hard-code "data_designer.plugins" instead of using the PLUGIN_ENTRY_POINT_GROUP constant defined in plugin_catalog.py:839. Three call-sites with the same literal invite drift — swap the string for the constant (including the PluginEntryPointInfo.group default and importlib.metadata.entry_points(group=...) call).
  • PluginTapRepository.load() (repositories/plugin_tap_repository.py:1359) swallows every exception and returns None. A corrupted plugin_taps.yaml silently disappears from the user's config — they get the built-in tap back and no error. Consider narrowing the catch (at least to OSError | yaml.YAMLError | ValidationError) and surfacing a warning so the user knows their file didn't parse.
  • _load_cached_catalog (repositories/plugin_tap_repository.py:1473) also catches bare Exception, which hides JSON corruption / partial writes. A print_warning on fall-through would help debuggability without changing behavior.
  • PluginCatalogController exception handling is broad. run_search, _list_entries_or_exit, _get_entry_or_exit, run_taps_remove, etc. catch bare Exceptionprint_error(str(e)). Prefer catching the known types (PluginCatalogError, ValueError, URLError, OSError) so genuine bugs (e.g. AttributeError during refactors) don't turn into vague user-facing messages.
  • run_taps_add double-catches ValidationError and then Exception. Since the repository only raises ValueError (duplicate) and Pydantic only raises ValidationError, the bare Exception branch is dead — remove it or narrow it.
  • plugins_callback's dummy _ = tap is load-bearing but unobvious. A one-line comment ("# read via ctx.parent.params in subcommands") would save the next reader a trip to Click's internals. (Feedback instruction says "no comments unless WHY is non-obvious" — this qualifies.)
  • _resolve_path_source (services/plugin_install_service.py:2029) assumes the tap URL points to <root>/catalog/plugins.json for path sources. This matches normalize_tap_location, but a user who passes a local catalog JSON named something else gets Path.parent which may or may not be what they want. Catalog-level _validate_package_path already blocks .. traversal, so this is a UX rather than a security issue — worth a test for the non-canonical tap location case.

Schema validation

  • Strict key sets (CATALOG_PLUGIN_KEYS etc.) require exact equality. Good for the v2 freeze, but note: the Pydantic models carry ConfigDict(extra="allow") which becomes unreachable once the strict validator runs first. Either drop extra="allow" (defensive; strict is already enforced) or keep the asymmetry deliberately and document why in the module docstring. Current code works but the intent is ambiguous.
  • _catalog_data_designer_compatibility enforces specifier == str(requirement.specifier) exactly. Catalog authors who write data-designer (>=0.5.7) in requirement and >=0.5.7 in specifier will hit spurious mismatches if Requirement's canonical form differs from what they wrote. Likely fine in practice (tap schema v2 controls the format), but worth noting to the tap-producer side.
  • Catalog size limit is exactly 1 * 1024 * 1024. A 1 MB cap for a JSON catalog is reasonable but quite tight if the NVIDIA tap grows. Consider 4–8 MB, or document the cap in the tap schema so producers know.

Security

  • urlopen(request, timeout=10) with 1 MB read cap is solid: no shell exec, size-capped read, http/https scheme guard, explicit timeout. ✓
  • Subprocess install uses list-form subprocess.run(command, check=False) — no shell, no injection. ✓
  • PEP 508 direct references (git install target) are built from validated fields (url scheme-checked, subdirectory passed through _validate_package_path). ✓
  • Untrusted-tap install warning is printed but does not block. That matches the design (force confirmation via confirm_action(default=False)), but reviewers should note that --yes on an untrusted tap skips the prompt with only a yellow warning. Consider requiring an explicit --allow-untrusted flag when combining --yes with a non-trusted tap.
  • Remote fetch follows redirects implicitly via urlopen's default HTTPRedirectHandler, which restricts to http/https/ftp — no file:// escape. ✓

Tests

  • Coverage is broad: command delegation, controller branching (dry-run, incompatible, force, trusted/untrusted), repository (cache keying, zero-TTL refresh, schema rejection, case-insensitive aliases, duplicate runtime names), service (compatibility evaluation, grouping, installed entry points), install service (pypi/git/path, uv vs pip, verification flow).
  • Missing: no test exercises _fetch_remote_catalog error paths (HTTP 4xx/5xx, oversized body, decode failure). A single patch("urllib.request.urlopen") test per branch would round this out.
  • Missing: no test for the "cache fallback when fresh fetch fails" branch in load_catalog. That's the main reason to have a cache — worth one direct test.

Style / minor

  • Several raise typer.Exit(code=1) blocks follow except ... as e: but don't chain (from e). The project convention appears to be bare raise typer.Exit(...) for CLI errors, so this is consistent, but the lost traceback makes debug logs less useful. Consider a debug-mode env var that re-raises.
  • plugin_catalog.py is 490 lines with 20+ module-level constants and helper _-prefixed validators. Consider splitting the validator helpers into plugin_catalog/_validation.py in a follow-up — not blocking.
  • InstalledPluginInfo, InstallPlan, CompatibilityResult are @dataclass(frozen=True) while the catalog models are Pydantic. Fine, but an out-of-band explanation ("internal DTOs vs serialized catalog schema") in the module docstring would help.
  • _fetch_catalog_payload return type is dict; under the project's modern-typing rule it should be dict[str, object] (already done elsewhere in this PR).

Verdict

Approve with minor requests. The architecture is sound, the schema validation is appropriately strict for a v2 contract freeze, and the test coverage is solid. Action items in priority order:

  1. Replace three "data_designer.plugins" string literals with PLUGIN_ENTRY_POINT_GROUP.
  2. Narrow the bare except Exception catches in the controller and repository; drop the dead Exception branch in run_taps_add.
  3. Surface (don't swallow) load errors for plugin_taps.yaml and cache files.
  4. Add a couple of tests for remote-fetch error paths and cache fallback.
  5. Consider --allow-untrusted as a stronger guard when --yes + untrusted tap combine.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 7, 2026

Greptile Summary

This PR introduces the full plugin catalog CLI infrastructure — tap management, schema v2 validation, install planning (PyPI, Git, local path), and post-install entry-point verification — wired into the existing lazy Typer CLI pattern. All three issues raised in the previous review round have been addressed in subsequent commits.

  • Tap management (plugin_tap_repository.py): URL-keyed cache files, case-insensitive alias matching, GitHub blob/tree URL normalization with subdirectory preservation, and stale-alias cache cleanup via an alias-keyed scan.
  • Schema validation (plugin_catalog.py): Strict exact-key matching for every catalog object, duplicate runtime name detection keyed on entry_point.name, PEP 508 specifier/marker cross-validation against the requirement string, and package path segment safety checks.
  • Install service (plugin_install_service.py): Correct entry.entry_point.name lookup in verify_entry_point, importlib.invalidate_caches() before the check, and path source resolution relative to the tap root with the catalog/ directory handled specially.

Confidence Score: 5/5

Safe to merge — all three issues from the previous review round are addressed and no new correctness problems were found.

The three previously flagged problems (unreachable compatibility guard, duplicate-name check keyed on the wrong field, GitHub tree URL path loss) are all fixed and verified by the author. The strict schema validator, install plan builder, post-install verification, and tap cache logic are all logically correct. The verify_entry_point lookup now uses entry.entry_point.name (the entry-point registry key) rather than the catalog name, which was the remaining open concern. Test coverage spans commands, controller, repository, and both services.

No files require special attention.

Important Files Changed

Filename Overview
packages/data-designer/src/data_designer/cli/plugin_catalog.py Strict schema v2 validator with exact key matching, PEP 508 cross-validation, duplicate entry_point.name detection, and package-path segment safety checks. Logic is correct throughout.
packages/data-designer/src/data_designer/cli/repositories/plugin_tap_repository.py URL-keyed cache with TTL, GitHub blob/tree normalization with subdirectory support, stale-alias cache cleanup, and safe fallback to stale cache on network failure. All logic reviewed and correct.
packages/data-designer/src/data_designer/cli/services/plugin_install_service.py Correct entry.entry_point.name used in verify_entry_point, importlib.invalidate_caches() called before discovery, path resolution handles the catalog/plugins.json tap root convention correctly.
packages/data-designer/src/data_designer/cli/services/plugin_catalog_service.py Compatibility evaluation correctly uses full version for specifier checks and python_version major.minor for marker evaluation. Compatible entries are preferred before falling back to newest incompatible.
packages/data-designer/src/data_designer/cli/controllers/plugin_catalog_controller.py Previously-flagged unreachable compatibility guard is now fixed: run_install fetches with include_incompatible=True then applies the controller-level --force gate before building the plan.
packages/data-designer/src/data_designer/cli/commands/plugins.py Clean Typer command wrappers with --tap inheritance from parent context and appropriate warnings when parent --tap is ignored by tap-management subcommands.
packages/data-designer/src/data_designer/cli/main.py Lazy Typer registration for plugins and taps subgroups wired correctly; plugins_callback stores --tap on the context for downstream resolution.

Reviews (6): Last reviewed commit: "fix(cli): verify plugin entry point name..." | Re-trigger Greptile

Comment thread packages/data-designer/src/data_designer/cli/plugin_catalog.py Outdated
Add typed catalog and tap models, persistent tap storage, cached
catalog loading, compatibility evaluation, install plan generation,
and runtime plugin discovery helpers.

Refs #617
Wire list, search, info, install, installed, and tap management
commands through the existing command-controller CLI pattern.

Refs #617
Add regression coverage for tap caching, catalog compatibility,
installer command generation, local path resolution, and Typer command
delegation.

Refs #617
Validate tap catalogs against the schema v2 contract used by
NVIDIA-NeMo/DataDesignerPlugins#36, including source union fields,
docs URLs, package paths, compatibility metadata, and unique runtime
plugin names.

Derive Git install targets as package-qualified PEP 508 direct
references so git tap entries install the package described by the
catalog source metadata.

Refs #617
- Invalidate import caches before post-install entry point verification
- Make tap aliases case-insensitive and cache catalogs by alias plus URL
- Prefer compatible catalog entries before falling back to forced installs
- Clarify unused --tap behavior and list installed entry points without imports
- Add direct controller coverage and update CLI plugin documentation

Refs #617
Fetch install targets before compatibility filtering so the controller
owns the final --force decision and the incompatible install guard stays
reachable.

Refs #617
Apply ruff formatting to the plugin command and tap repository tests so
CI format checks pass on the PR merge commit.

Refs #617
Key catalog duplicate detection by entry_point.name so distinct catalog
entries cannot register the same runtime plugin name.

Refs #617
@johnnygreco johnnygreco force-pushed the johnny/feat/617/plugin-catalog-cli-core branch from 4549bd7 to 3916648 Compare May 8, 2026 02:43
Comment thread packages/data-designer/src/data_designer/cli/services/plugin_install_service.py Outdated
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.

Build Data Designer plugin catalog CLI core

1 participant