Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ea5996f33
ℹ️ 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 PluginInstallParams { | ||
| marketplace_name, | ||
| marketplace_path, | ||
| plugin_name, | ||
| cwd, | ||
| } = params; | ||
|
|
||
| let plugins_manager = self.thread_manager.plugins_manager(); | ||
| let request = PluginInstallRequest { | ||
| plugin_name, | ||
| marketplace_name, | ||
| cwd: cwd.unwrap_or_else(|| self.config.cwd.clone()), | ||
| marketplace_path, |
There was a problem hiding this comment.
plugin_install trusts marketplacePath from the request and forwards it directly into install resolution without verifying it came from plugin/list discovery. This lets a client install from any readable marketplace.json on disk, bypassing the intended home/repo marketplace boundary described for discovered entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think this is OK.
codex-rs/core/src/plugins/manager.rs
Outdated
| // Enabled state is keyed by `<plugin>@<marketplace>`, so duplicate | ||
| // marketplace files with the same declared name intentionally share | ||
| // the same config toggle. | ||
| enabled: configured_plugins | ||
| .get(&format!("{}@{marketplace_name}", plugin.name)) |
There was a problem hiding this comment.
like you say here, our downstream logic keys by plugin@marketplace, yet we return duplicate plugin@marketplace entries from plugin/list. Why? if we are effectively treating them as a single resolved plugin, shouldn't we enforce at most one resolved plugin per plugin@markeplace, and return that from plugin/list?
There was a problem hiding this comment.
Update to keep the first one.
| pub struct PluginSummary { | ||
| pub name: String, | ||
| pub source: PluginSource, | ||
| pub enabled: bool, |
There was a problem hiding this comment.
like you mentioned, we should probably include installed status in an enum here (something like: uninstalled, disabled, enabled).
There was a problem hiding this comment.
Will do it as a follow up PR.
| MarketplaceError::InvalidMarketplaceFile { .. } | ||
| | MarketplaceError::PluginNotFound { .. } | ||
| | MarketplaceError::DuplicatePlugin { .. } | ||
| | MarketplaceError::InvalidPlugin(_) => { | ||
| self.send_invalid_request_error(request_id, err.to_string()) |
There was a problem hiding this comment.
i think in plugin/install we treat an invalid/missing marketplace file as an internal error and not an invalid request. we should unify.
xl-openai
force-pushed
the
xl/plugins
branch
2 times, most recently
from
53c6dd5 to
058a35b
Compare
sayan-oai
left a comment
sayan-oai
left a comment
There was a problem hiding this comment.
one piece of feedback that should be addressed, but approving to unblock
| .into_iter() | ||
| .filter_map(|plugin| { | ||
| let plugin_key = format!("{}@{marketplace_name}", plugin.name); | ||
| if !seen_plugin_keys.insert(plugin_key.clone()) { |
There was a problem hiding this comment.
we silently drop here based on first seen, but in plugin/install we error on duplicates. so i think if there are duplicate plugins in a single marketplace file, plugin/list will return one, but plugin/install will fail on the same marketplace file.
is there a reason to keep the behavior different or should we unify on one (silent drop or error)?
There was a problem hiding this comment.
This is a pretty annoying edge case, but I agree with you — I’ll handle both scenarios and just pick one.
I just don’t want to block plugin listing because of a minor issue on a marketplace.json file.
There was a problem hiding this comment.
yea good to merge without this, but we should track + address soon.
2 participants
Introduce a plugin/list which reads from local marketplace.json.
Also update the signature for plugin/install.