[codex] Add cross-repo plugin sources to marketplace manifests#18017
Conversation
xli-oai
added
the
oai
ChatGPT Codex Connector
![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: 03cb3e141f
ℹ️ 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".
| run_git( | ||
| &["clone", url, destination.to_string_lossy().as_ref()], | ||
| None, | ||
| )?; |
There was a problem hiding this comment.
normalize_git_plugin_source_url allows ./ and ../ git URLs, but clone_git_plugin_source executes git clone with cwd set to None. That makes relative URLs resolve from the process working directory instead of the marketplace file's root, so installs/detail reads can clone the wrong repo or fail whenever cwd differs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Relative git source URLs are now normalized from the marketplace root during parsing, so later materialization no longer depends on the process cwd.
|
Could be a follow up PR but we need to deal with the actual loading of those plugins. |
Yes! 👍 |
xli-oai
force-pushed
the
xli-codex/cross-repo-marketplace-support
branch
from
21c13ec to
2cb73c8
Compare
|
@xli-oai this seem to regress in marketplace plugin read behavior - with this change, it means that means an untrusted marketplace manifest can trigger git clone side effects during metadata reads, before the user explicitly approves installation. does that sound correct? IMO plugin/read should stay read-only. We should only fetch or clone plugin sources during the actual install flow, where we can apply the right approval and source checks. |
xli-oai
force-pushed
the
xli-codex/cross-repo-marketplace-support
branch
from
63b356c to
4225ed0
Compare
xli-oai
force-pushed
the
xli-codex/cross-repo-marketplace-support
branch
from
47c4626 to
cb711d8
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/app-server/src/codex_message_processor.rs
Lines 6454 to 6458 in cb711d8
Expose install-required reason in plugin/read responses
Core now sets details_unavailable_reason for uninstalled git sources, but plugin/read drops that signal and returns only empty description/skills/apps. Clients cannot distinguish “install required for remote source” from genuinely empty plugin metadata.
ℹ️ 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 review |
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/app-server-protocol/src/protocol/v2.rs
Lines 3605 to 3613 in da33743
Expose remote-detail unavailability reason over app-server API
Core now sets details_unavailable_reason for uninstalled git-sourced plugins, but the v2 PluginDetail response type has no field for that signal. Clients receive empty details without a machine-readable reason, making the new core behavior effectively unusable from the API.
ℹ️ 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".
| MarketplacePluginSource::Git { .. } => { | ||
| warn!( | ||
| plugin = plugin_name, | ||
| marketplace = OPENAI_CURATED_MARKETPLACE_NAME, | ||
| "skipping remote curated plugin source during cache refresh" | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
refresh_curated_plugin_cache drops every curated plugin whose source is Git. A configured curated plugin backed by a git source will never be installed/updated during curated sync, so users can stay on missing/stale code even after marketplace updates.
Useful? React with 👍 / 👎.
| MarketplacePluginSource::Git { .. } => { | ||
| warn!( | ||
| plugin = plugin_name, | ||
| marketplace = %marketplace_name, | ||
| "skipping remote plugin source during remote sync" | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
sync_plugins_from_remote skips curated entries with MarketplacePluginSource::Git, so they never enter local_plugins. Remote-enabled plugins with those names are then not installed/uninstalled locally, causing persistent divergence between backend plugin state and local cache/config.
Useful? React with 👍 / 👎.
| let materialized = | ||
| materialize_marketplace_plugin_source(codex_home, &source).map_err(|err| { | ||
| format!("failed to materialize plugin source for {plugin_key}: {err}") | ||
| })?; | ||
| let source_path = materialized.path.clone(); | ||
| let plugin_version = plugin_version_for_source(source_path.as_path()) | ||
| .map_err(|err| format!("failed to read plugin version for {plugin_key}: {err}"))?; | ||
|
|
||
| if mode == NonCuratedCacheRefreshMode::IfVersionChanged | ||
| && store.active_plugin_version(&plugin_id).as_deref() == Some(plugin_version.as_str()) |
There was a problem hiding this comment.
In IfVersionChanged mode, non-curated refresh materializes git sources and reads manifest versions before checking whether the installed version already matches. This forces a full clone per configured git plugin even when nothing changed, adding avoidable network/disk cost to periodic refresh.
Useful? React with 👍 / 👎.
| if mode == NonCuratedCacheRefreshMode::IfVersionChanged | ||
| && store.active_plugin_version(&plugin_id).as_deref() == Some(plugin_version.as_str()) |
There was a problem hiding this comment.
Cache refresh gates reinstall on plugin version equality. For git plugins without an explicit manifest version, plugin_version_for_source resolves to "local", so upstream commits on the tracked ref are ignored and refresh wrongly skips reinstall.
Useful? React with 👍 / 👎.
| ".." => { | ||
| normalized.pop(); | ||
| } |
There was a problem hiding this comment.
normalize_relative_git_plugin_source_url accepts .. segments and repeatedly pop()s the marketplace root, allowing manifests to resolve relative git URLs outside the marketplace tree. Third-party marketplace files can therefore point installs at arbitrary local repositories/paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48562472d. Relative git URLs now reject any .. path segment directly, including slash and backslash variants, so they cannot escape the marketplace root.
|
@viyatb-oai Fixed. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20417c883c
ℹ️ 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".
| MarketplacePluginSource::Git { .. } => { | ||
| warn!( | ||
| plugin = plugin_name, | ||
| marketplace = %marketplace_name, | ||
| "skipping remote plugin source during remote sync" | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
sync_plugins_from_remote records git-sourced plugins in local_plugin_names but then continues, so they never enter local_plugins. The later install/enable reconciliation iterates only local_plugins, which means remotely enabled curated git plugins are silently ignored and local state cannot converge to backend state.
Useful? React with 👍 / 👎.
| let source_path = match plugin.source { | ||
| MarketplacePluginSource::Local { path } => path, | ||
| MarketplacePluginSource::Git { .. } => { | ||
| warn!( | ||
| plugin = plugin_name, | ||
| marketplace = OPENAI_CURATED_MARKETPLACE_NAME, | ||
| "skipping remote curated plugin source during cache refresh" | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
refresh_curated_plugin_cache drops every curated plugin whose source is Git. For configured curated plugins that moved to git-backed sources, startup cache refresh cannot reinstall/update them when curated version changes, leaving stale or missing local installs despite valid configuration.
Useful? React with 👍 / 👎.
3 participants
Summary
main, including alternate manifest locations and string local sourcesDetails
This teaches the marketplace parser to accept all of the following:
"source": "./plugins/foo"{"source":"local","path":"./plugins/foo"}{"source":"url","url":"https://github.com/org/repo.git"}{"source":"git-subdir","url":"owner/repo","path":"plugins/foo","ref":"main","sha":"..."}It also preserves the newer tolerant behavior from
main: invalid or unsupported plugin entries are skipped instead of breaking the whole marketplace.Validation
cargo test -p codex-core plugins::marketplace::testsjust fix -p codex-corejust fmtNotes
cargo test -p codex-corerun still hit unrelated existing failures in agent and multi-agent tests during this session; the marketplace-focused suite passed after the rebase resolution.