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: 78f820f76b
ℹ️ 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".
codex-rs/core/src/plugins/manager.rs
Outdated
| for plugin_id in uninstalls { | ||
| store.uninstall(&plugin_id)?; | ||
| } | ||
| for (source_path, plugin_id) in installs { | ||
| store.install(source_path, plugin_id)?; |
There was a problem hiding this comment.
sync_plugins_from_remote removes existing plugins before adding new ones. If a later store.install(...) fails (e.g., bad/missing manifest in curated source), the method returns early and skips config edits, but already-uninstalled plugins stay deleted. This creates a partial, inconsistent state after a failed sync. Make the update transactional or install first, then uninstall.
Useful? React with 👍 / 👎.
xl-openai
force-pushed
the
xl/plugins
branch
2 times, most recently
from
0bc4212 to
546f2ed
Compare
sayan-oai
left a comment
sayan-oai
left a comment
There was a problem hiding this comment.
approving to unblock, a few small questions
| let curated_marketplace_path = AbsolutePathBuf::try_from( | ||
| curated_marketplace_root.join(".agents/plugins/marketplace.json"), | ||
| ) | ||
| .map_err(|_| PluginRemoteSyncError::LocalMarketplaceNotFound)?; |
There was a problem hiding this comment.
can this race with the async curated marketplace fetch on startup? probably not a big issue
There was a problem hiding this comment.
Good call. I think some sort of client retry is needed.
codex-rs/core/src/plugins/manager.rs
Outdated
| Err(err) => { | ||
| warn!(error = %err, "skipping remote plugin sync because remote state could not be fetched"); | ||
| return Ok(RemotePluginSyncResult::default()); |
There was a problem hiding this comment.
question: callers are ok with a no-op success response on sync errors?
There was a problem hiding this comment.
I’ve considered this, but I think it’s more responsible to let users use whatever is already on disk rather than blocking them. I’ll add a response message that indicates whether the sync succeeded.
| if is_installed { | ||
| uninstalls.push(plugin_id); | ||
| } | ||
| if is_installed || current_enabled.is_some() { | ||
| result.uninstalled_plugin_ids.push(plugin_key.clone()); | ||
| } | ||
| if current_enabled.is_some() { | ||
| config_edits.push(ConfigEdit::ClearPath { | ||
| segments: vec!["plugins".to_string(), plugin_key], | ||
| }); | ||
| } |
There was a problem hiding this comment.
is the requirement that force sync makes the two plugin lists exactly the same? explicitly uninstalling local plugins that weren't in the remote list seems harsh. can we leave them alone and just have the if case?
There was a problem hiding this comment.
If a plugin is disabled remotely, I think it should also be disabled locally. Otherwise, things could get messy quickly. Also, if we go with a remote-only model, there’s currently no way to remove it from local once it’s been installed if we go this way.
|
@codex review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cda167e006
ℹ️ 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".
| for (source_path, plugin_id) in installs { | ||
| store.install(source_path, plugin_id)?; | ||
| } | ||
| for plugin_id in uninstalls { | ||
| store.uninstall(&plugin_id)?; | ||
| } |
There was a problem hiding this comment.
sync_plugins_from_remote executes installs/uninstalls sequentially and returns on first error. If an early operation succeeds and a later one fails, the cache is partially mutated but config edits are not applied, leaving cache/config inconsistent while the request is treated as sync failure. This can silently change local plugin state after a failed forceRemoteSync call.
Useful? React with 👍 / 👎.
codex-rs/core/src/plugins/manager.rs
Outdated
| for plugin in curated_marketplace.plugins { | ||
| let plugin_id = PluginId::new(plugin.name.clone(), marketplace_name.clone())?; | ||
| let plugin_key = plugin_id.as_key(); | ||
| let source_path = match plugin.source { | ||
| MarketplacePluginSourceSummary::Local { path } => path, | ||
| }; | ||
| let current_enabled = configured_plugins | ||
| .get(&plugin_key) | ||
| .map(|plugin| plugin.enabled); | ||
| let is_installed = self.store.is_installed(&plugin_id); | ||
| local_plugin_names.insert(plugin.name.clone()); | ||
| local_plugins.push(( | ||
| plugin.name, |
There was a problem hiding this comment.
The sync planner appends every curated marketplace entry to local_plugins without deduping by plugin key. If marketplace.json has duplicate plugin names, the same plugin is planned multiple times, so repeated installs/uninstalls can occur and later duplicate sources can overwrite earlier ones. This conflicts with listing behavior that intentionally keeps first-seen entries.
Useful? React with 👍 / 👎.
2 participants
Add forceRemoteSync to plugin/list.
When it is set to True, we will sync the local plugin status with the remote one (backend-api/plugins/list).