Conversation
Coverage Report
Per-file coverage (top 40 by missed lines)
|
There was a problem hiding this comment.
Pull request overview
This PR removes hardcoded/default registry configuration and central registry-check compatibility logic, shifting “unsupported checks” to be declared by each registry via RegistryDefinition, and updates MCP/CLI surfaces to reflect supported registries dynamically.
Changes:
- Add
excluded_checkstosafe_pkgs_core::RegistryDefinitionand move registry check-support policy into each registry crate’sregistry_definition(). - Remove the binary-level
app_registry_check_support()and update registry catalog support computation to useexcluded_checks. - Update MCP JSON schema metadata and CLI defaults to derive supported/default registries from the registered definitions (instead of hardcoded
"npm").
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/registries/mod.rs |
Uses excluded_checks for support matrix; removes hardcoded fallback defaults in favor of “must be registered” behavior. |
src/mcp/server.rs |
Builds dynamic schema enum/default/description from registry catalog; updates server instructions to derive lockfile keywords dynamically. |
src/main.rs |
CLI --registry default now uses default_lockfile_registry_key(); removes centralized check-support policy and updates tests accordingly. |
crates/registry/pypi/src/lib.rs |
Declares excluded_checks for PyPI. |
crates/registry/npm/src/lib.rs |
Declares empty excluded_checks for npm. |
crates/registry/cargo/src/lib.rs |
Declares excluded_checks for Cargo. |
crates/core/src/lib.rs |
Adds excluded_checks field to RegistryDefinition. |
crates/checks/README.md |
Updates wiring documentation to reflect excluded_checks ownership. |
CONTRIBUTING.md |
Updates contributor guidance to declare unsupported checks in the registry crate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| serde_json::json!(format!( | ||
| "Package registry. Supported: {}. Defaults to \"{}\".", | ||
| crate::registries::supported_package_registry_keys().join("\", \""), | ||
| crate::registries::default_package_registry_key(), | ||
| )), |
There was a problem hiding this comment.
The generated description string is missing the opening quote around the first registry key because it uses join("\", \"") without adding a leading ". This currently renders like Supported: npm", "cargo"... instead of Supported: "npm", "cargo".... Consider joining with ", " and wrapping the whole list, or mapping keys to already-quoted strings before joining.
| serde_json::json!(format!( | ||
| "Registry for parsing and checks. Supported: {}. Defaults to \"{}\".", | ||
| crate::registries::supported_lockfile_registry_keys().join("\", \""), | ||
| crate::registries::default_lockfile_registry_key(), | ||
| )), |
There was a problem hiding this comment.
Same quoting issue as package_registry_schema: supported_lockfile_registry_keys().join("\", \"") produces a list missing the opening quote for the first element, so the description renders oddly. Build the list as properly quoted items (or avoid embedding quotes in the separator).
| let registry_files = crate::registries::supported_lockfile_registry_keys() | ||
| .into_iter() | ||
| .filter_map(|key| { | ||
| crate::registries::supported_lockfile_files_for_registry(key) | ||
| .map(|files| format!("{key}: {}", files.join("/"))) | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join(", "); |
There was a problem hiding this comment.
lockfile_path_schema calls supported_lockfile_files_for_registry once per registry key; that helper rebuilds the registry catalog on each call (register_default_catalog()), which is unnecessary work during schema generation. Consider fetching the catalog once (or adding a cached map of registry->supported_files in registries) and reusing it here.
| let lockfile_keywords = crate::registries::supported_lockfile_registry_keys() | ||
| .into_iter() | ||
| .filter_map(|key| crate::registries::supported_lockfile_files_for_registry(key)) | ||
| .flatten() | ||
| .map(|f| format!("`{f}`")) | ||
| .collect::<Vec<_>>() | ||
| .join(", "); |
There was a problem hiding this comment.
get_info builds lockfile_keywords by repeatedly calling supported_lockfile_files_for_registry, which reconstructs the registry catalog each time. Since get_info can be invoked multiple times by clients, consider caching these supported file keywords (or computing them once from a single catalog instance) to avoid repeated catalog builds.
| pub fn default_package_registry_key() -> &'static str { | ||
| registry_definitions() | ||
| .first() | ||
| .map(|def| def.key) | ||
| .unwrap_or("npm") | ||
| .expect("at least one registry must be registered") | ||
| } | ||
|
|
||
| /// Returns the default lockfile registry key. | ||
| pub fn default_lockfile_registry_key() -> &'static str { | ||
| registry_definitions() | ||
| .iter() | ||
| .find(|def| def.create_lockfile_parser.is_some()) | ||
| .map(|def| def.key) | ||
| .unwrap_or("npm") | ||
| .expect("at least one lockfile-capable registry must be registered") | ||
| } |
There was a problem hiding this comment.
default_package_registry_key/default_lockfile_registry_key now expect(...) instead of providing a fallback. This will panic if app_registry_definitions() is ever empty (or if no lockfile-capable registries are registered), including during CLI default evaluation and MCP schema generation. Prefer returning Option<&'static str>/Result<_,_> and surfacing a user-facing error, or keep a non-panicking fallback consistent with the schema enum.
No description provided.