feat: Handle alternate plugin manifest paths#18182
Conversation
| let repo_root = TempDir::new()?; | ||
| let valid_plugin_root = repo_root.path().join("plugins/valid-plugin"); | ||
| std::fs::create_dir_all(repo_root.path().join(".git"))?; | ||
| std::fs::create_dir_all(repo_root.path().join(".claude-plugin"))?; |
There was a problem hiding this comment.
Let's use such const for all places: ALTERNATE_PLUGIN_MANIFEST_RELATIVE_PATH?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c3c64a8ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let Some(manifest) = load_plugin_manifest(source_path.as_path()) else { | ||
| warn!( | ||
| path = %path.display(), | ||
| plugin = name, | ||
| source_path = %source_path.display(), | ||
| "skipping marketplace plugin that failed to load" | ||
| ); | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
Preserve invalid-manifest errors for explicit plugin reads
Skipping manifest-unloadable entries in load_marketplace removes them before read_plugin_for_config does name lookup. For a requested plugin with missing/invalid manifest, plugin/read now degrades to PluginNotFound instead of surfacing an invalid-manifest error, hiding the real failure mode for a direct read request.
Useful? React with 👍 / 👎.
| let Some(manifest) = load_plugin_manifest(source_path.as_path()) else { | ||
| warn!( | ||
| path = %path.display(), | ||
| plugin = name, | ||
| source_path = %source_path.display(), | ||
| "skipping marketplace plugin that failed to load" | ||
| ); | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
Return invalid-manifest error for explicit plugin reads
Filtering out unloadable plugins inside load_marketplace causes read_plugin_for_config to miss requested entries and return PluginNotFound instead of the real invalid-manifest failure. That hides actionable diagnostics for plugin/read callers asking for a known plugin with a bad/missing manifest.
Useful? React with 👍 / 👎.
| let Some(manifest) = load_plugin_manifest(source_path.as_path()) else { | ||
| warn!( | ||
| path = %path.display(), | ||
| plugin = name, | ||
| source_path = %source_path.display(), | ||
| "skipping marketplace plugin that failed to load" | ||
| ); | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
Keep remote sync state updates for dropped marketplace entries
sync_remote_plugins iterates curated_marketplace.plugins; after this change, manifest-broken entries are removed upstream by load_marketplace. Those plugins then never reach sync reconciliation, so remote disable/uninstall state can be ignored and stale local enable/install config can persist.
Useful? React with 👍 / 👎.
201598a to
f52a20c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d80dfc031
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let Some(source_path) = resolve_supported_plugin_source_path(marketplace_path, &name, source) | ||
| else { | ||
| return Ok(None); | ||
| }; |
There was a problem hiding this comment.
Enforce availability policy before skipping bad sources
find_installable_marketplace_plugin relies on find_marketplace_plugin, but matching entries are discarded when source resolution fails (Ok(None)). Policy checks (NOT_AVAILABLE/product restrictions) happen only after a surviving entry is returned, so a restricted first entry can be bypassed by a later duplicate with the same name. This can incorrectly allow installation instead of returning PluginNotAvailable.
Useful? React with 👍 / 👎.
| let Some(source_path) = resolve_supported_plugin_source_path(marketplace_path, &name, source) | ||
| else { | ||
| return Ok(None); | ||
| }; |
There was a problem hiding this comment.
Check plugin policy before discarding unresolved sources
find_installable_marketplace_plugin enforces availability only after find_marketplace_plugin returns a resolved source. In resolve_marketplace_plugin_entry, invalid/unsupported sources return Ok(None) first, so a disallowed first duplicate entry can be skipped and a later installable duplicate accepted. This can bypass marketplace install/product restrictions for the same plugin name.
Useful? React with 👍 / 👎.
| @@ -214,6 +185,31 @@ pub fn resolve_marketplace_plugin( | |||
| }) | |||
There was a problem hiding this comment.
Preserve not-available errors for unresolved plugin entries
Matching entries with invalid/unsupported sources are silently skipped, and the function falls through to PluginNotFound. For install flows this misclassifies plugins that are present-but-not-installable as “not found,” changing behavior and confusing clients that rely on PluginNotAvailable to distinguish policy from typo/missing-plugin cases.
Useful? React with 👍 / 👎.
| pub use codex_utils_plugins::mention_syntax; | ||
| pub use codex_utils_plugins::plugin_namespace_for_skill_path; |
There was a problem hiding this comment.
Keep PLUGIN_MANIFEST_PATH in codex-plugin public API
This change removes the codex_plugin::PLUGIN_MANIFEST_PATH re-export, which is unrelated to alternate manifest discovery and introduces a breaking API change for downstream users. Crates importing that symbol will fail to compile after this commit even if they do not use the new alternate-path behavior.
Useful? React with 👍 / 👎.
| let Some(source_path) = resolve_supported_plugin_source_path(marketplace_path, &name, source) | ||
| else { | ||
| return Ok(None); | ||
| }; |
There was a problem hiding this comment.
Check plugin policy before discarding unresolved sources
find_installable_marketplace_plugin enforces availability only after find_marketplace_plugin returns a resolved source. In resolve_marketplace_plugin_entry, invalid/unsupported sources return Ok(None), so a restricted first duplicate entry can be skipped and a later installable duplicate accepted. This can bypass marketplace install/product restrictions for the same plugin name.
Useful? React with 👍 / 👎.
| @@ -214,6 +185,31 @@ pub fn resolve_marketplace_plugin( | |||
| }) | |||
There was a problem hiding this comment.
Return not-available when matched entry is restricted
When a matched plugin entry is present but skipped due source resolution, find_marketplace_plugin falls through to PluginNotFound. For install paths this changes behavior from policy denial to “not found,” even when the plugin exists and is intentionally unavailable. Clients lose the ability to distinguish bad plugin names from policy-restricted plugins.
Useful? React with 👍 / 👎.
| pub use codex_utils_plugins::mention_syntax; | ||
| pub use codex_utils_plugins::plugin_namespace_for_skill_path; |
There was a problem hiding this comment.
Preserve PLUGIN_MANIFEST_PATH re-export for compatibility
This commit removes codex_plugin::PLUGIN_MANIFEST_PATH from the public API, which is a breaking change unrelated to alternate-manifest discovery. Downstream crates importing that symbol will fail to compile after this commit even if they do not use new functionality.
Useful? React with 👍 / 👎.
| Err(MarketplaceError::InvalidPlugin(message)) => { | ||
| warn!( | ||
| path = %path.display(), | ||
| marketplace = %marketplace.name, | ||
| error = %message, | ||
| "skipping invalid marketplace plugin" | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Surface invalid plugin entries as marketplace load errors
load_marketplace now catches InvalidPlugin and silently skips that entry. A malformed marketplace can become empty without any marketplaceLoadErrors, making configuration bugs hard to detect and causing plugins to disappear without actionable errors.
Useful? React with 👍 / 👎.
Load plugin manifests through a shared discoverable-path helper so manifest reads, installs, and skill names all see the same alternate manifest location.