Conversation
| @@ -0,0 +1,640 @@ | |||
| use super::*; | |||
|
|
|||
| impl CodexMessageProcessor { | |||
There was a problem hiding this comment.
Try move plugin API out from codex_message_processor.rs as the file is too large now.
| @@ -0,0 +1,313 @@ | |||
| use crate::remote::RemotePluginServiceConfig; | |||
There was a problem hiding this comment.
Git doesn’t detect it as a rename, but remote_legacy.rs is the old remote.rs moved over intact. It remains wired to the legacy plugin API and is currently only used for the initial sync path. The new implementation now lives in remote.rs.
| let remote_plugin_service_config = RemotePluginServiceConfig { | ||
| chatgpt_base_url: config.chatgpt_base_url.clone(), | ||
| }; | ||
| let remote_plugin_id = format!("{plugin_name}@{remote_marketplace_name}"); |
There was a problem hiding this comment.
[P2] plugin_name is request-controlled here, so concatenating it into "{plugin_name}@{remote_marketplace_name}" and then later interpolating that value into "{base_url}/ps/plugins/{plugin_id}" creates a path/query injection primitive against the authenticated ChatGPT plugin API. A crafted plugin name containing /, ?, or # can rewrite the request target before fetch_remote_plugin_detail() does the GET. We should either carry the raw plugin_name/marketplace separately and build the URL with path-segment escaping (for example via Url::path_segments_mut()), or percent-encode plugin_id before appending it to the path.
There was a problem hiding this comment.
Will add a safeguard but app-server is not a public facing service the caller should just be codex app/TUI.
Co-authored-by: Codex <noreply@openai.com>
578a2af to
a10067c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc88185633
ℹ️ 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".
| | RemotePluginCatalogError::UnexpectedStatus { .. } | ||
| | RemotePluginCatalogError::Decode { .. } => JSONRPCErrorError { | ||
| code: INTERNAL_ERROR_CODE, |
There was a problem hiding this comment.
Map missing remote plugins to invalid requests
Treating every UnexpectedStatus as INTERNAL_ERROR makes normal user mistakes (e.g. unknown plugin id returning HTTP 404 from /ps/plugins/{id}) look like server failures. plugin/read should classify not-found remote plugins as an invalid request, consistent with local plugin lookup behavior.
Useful? React with 👍 / 👎.
| let directory_plugins = fetch_directory_plugins_for_scope(config, auth, scope).await?; | ||
| let installed_plugins = fetch_installed_plugins_for_scope(config, auth, scope).await?; |
There was a problem hiding this comment.
Continue listing healthy scopes when one scope fails
fetch_remote_marketplaces aborts on the first scope error (?), so a WORKSPACE failure (e.g. permission or transient 5xx) drops GLOBAL results too. This causes plugin/list to return no remote marketplaces even when one scope is still available.
Useful? React with 👍 / 👎.
| let installed_plugin = fetch_installed_plugins_for_scope(config, auth, scope) | ||
| .await? |
There was a problem hiding this comment.
Fallback when installed-state fetch fails in plugin/read
fetch_remote_plugin_detail fetches plugin metadata first, but then hard-fails on any /ps/plugins/installed error via ?. A transient installed-list failure turns a readable plugin into an error response. This should degrade to installed=false/default skill states instead of failing the whole read.
Useful? React with 👍 / 👎.
| Ok(remote_marketplaces) => data.extend( | ||
| remote_marketplaces | ||
| .into_iter() | ||
| .map(remote_marketplace_to_info), | ||
| ), |
There was a problem hiding this comment.
Deduplicate name collisions when merging remote marketplaces
plugin_list blindly appends remote marketplaces to local ones. Because marketplace names are user-defined locally, a local chatgpt-global can collide with the remote chatgpt-global, yielding duplicate marketplace names and conflicting <plugin>@<marketplace> identities in client flows.
Useful? React with 👍 / 👎.
| } | ||
| }; | ||
|
|
||
| if config.features.enabled(Feature::RemotePlugin) { |
There was a problem hiding this comment.
Enforce plugins gate before exposing remote catalogs
Remote branches in plugin/list and plugin/read are gated only by remote_plugin. If plugins = false but remote_plugin = true, remote marketplaces/details are still served, bypassing the main plugins feature-disable behavior used by local paths. The remote path should also require Feature::Plugins.
Useful? React with 👍 / 👎.
| for scope in RemotePluginScope::all() { | ||
| let directory_plugins = fetch_directory_plugins_for_scope(config, auth, scope).await?; | ||
| let installed_plugins = fetch_installed_plugins_for_scope(config, auth, scope).await?; |
There was a problem hiding this comment.
Fetch remote scopes concurrently to avoid long list stalls
Remote catalog loading is fully serial: for each scope it waits for directory then installed calls, each with a 30s timeout. In degraded networks this can add up to ~120s before returning plugin/list, causing avoidable latency spikes. Scope/page fetches should be parallelized or budgeted.
Useful? React with 👍 / 👎.
| interface: Some(MarketplaceInterface { | ||
| display_name: Some(OPENAI_CURATED_MARKETPLACE_DISPLAY_NAME.to_string()), | ||
| }), |
There was a problem hiding this comment.
does this mean we no longer have the formatted OpenAI Curated in product surfaces and they see openai-curated instead?
There was a problem hiding this comment.
I don’t understand the original purpose of this code — our marketplace already defines its own marketplace display name.
Also, in the Codex App we actually override the value to “Built by OpenAI,” but the lookup uses the marketplace name rather than the display name.
sayan-oai
left a comment
There was a problem hiding this comment.
@xl-openai sorry to dump, but codex findings below.
1 and 2 can be deferred if we are not doing TUI changes in this PR like you said.
3 seems small, worth checking.
4 seems like it's worth checking, but if we are maintaining all the remote plugins we can probably be more lax.
5 is a nit.
Findings
-
Remote plugin details are implemented in app-server, but the TUI never calls them.
In plugins.rs (line 1098), the TUI decides details are viewable only when marketplace.path.is_some(). Remote marketplaces intentionally have path: None, so plugins.rs (line 1134) builds no Enter action. -
Installed remote plugins get a toggle that only writes local config, so it will not actually change remote state.
-
Remote pagination can loop forever.
remote.rs (line 519) and remote.rs (line 538) keep requesting pages until next_page_token is None, with no max page count and no repeated-token detection. -
Remote default prompts bypass the documented caps.
The protocol says plugin default prompts are capped at 3 entries and 128 chars in v2.rs (line 3881). Local manifests enforce this in manifest.rs (line 244). Remote mapping just copies the prompt into a one-item vec in remote.rs (line 437).
5.: The app-server docs are stale for this API change.
README.md (line 189) still describes only local plugin list/read. It does not mention remote marketplaces, remoteMarketplaceName, or nullable marketplacePath / skill path. Example: a client following the README would still assume plugin/read requires marketplacePath.
| plugins_manager.maybe_start_non_curated_plugin_cache_refresh(&roots); | ||
|
|
||
| let config = match self.load_latest_config(/*fallback_cwd*/ None).await { | ||
| Ok(config) => config, | ||
| Err(err) => { | ||
| self.outgoing.send_error(request_id, err).await; | ||
| return; | ||
| } | ||
| }; |
There was a problem hiding this comment.
saw that later in this function we check if Feature::Plugins is enabled; im surprised we dont already know that by that point. why not just swap the config load and cache refresh and early return if the feature is disabled?
Add a temporary internal remote_plugin feature flag that merges remote marketplaces into plugin/list and routes plugin/read through the remote APIs when needed, while keeping pure local marketplaces working as before.