feat: support remote_sync for plugin install/uninstall.#14878
Conversation
![chatgpt-codex-connector[bot]](https://cdn.statically.io/img/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: 25175a669d
ℹ️ 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".
| source, | ||
| })?; | ||
| let expected_enabled = action == "enable"; | ||
| if parsed.id != plugin_id { |
There was a problem hiding this comment.
post_remote_plugin_mutation rejects any successful response whose id differs from the requested plugin_id. But this API family already uses opaque IDs in responses (e.g., remote list fixtures use values like "1"), so a valid mutation response can be misclassified as UnexpectedPluginId. That makes forceRemoteSync installs/uninstalls fail even after backend success.
Useful? React with 👍 / 👎.
| if !status.is_success() { | ||
| return Err(RemotePluginMutationError::UnexpectedStatus { url, status, body }); | ||
| } |
There was a problem hiding this comment.
codex says the backend uninstall endpoint returns 404 when the plugin isn't installed remotely. so if you have the plugin installed locally but not remotely and call uninstall with forceRemoteSync=true, it will fail here before local removal
There was a problem hiding this comment.
which is OK? ideally plugins/list will re-sync?
| { | ||
| // For now, sync treats remote `enabled = false` as uninstall rather than a distinct | ||
| // disabled state. | ||
| // TODO: Switch sync to `plugins/installed` so install and enable states stay distinct. |
There was a problem hiding this comment.
clarifying, this means new app-server endpoint?
There was a problem hiding this comment.
no it is a chat endpoint. we will move from calling chat plugins/list to just plugins/installed.
sayan-oai
left a comment
sayan-oai
left a comment
There was a problem hiding this comment.
approving to unblock, but left a comment with idempotency concerns.
| self.uninstall_plugin_id(plugin_id).await | ||
| } | ||
|
|
||
| pub async fn uninstall_plugin_with_remote_sync( |
There was a problem hiding this comment.
optional nit: maybe these methods should have curated in the name, or something to clearly convey this is only with the curated plugins (we dont support remote-syncing other marketplaces, or local plugins)?
There was a problem hiding this comment.
Actually not really—we also have private plugins (with remote sync). The real question is whether desktop can support purely local plugins, which I don’t have an answer for yet.
2 participants
will eventually migrate to plugin/installed for more precise state handling.