Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Design / storage summary for this PR: Filesystem layout
Flow
After that, normal marketplace discovery combines the registered user-added marketplace roots with the existing curated checkout. Plugin listing can see plugins from the newly added marketplace, but already-installed plugin artifacts are left untouched. |
faaff79 to
f4909a7
Compare
| } | ||
| } | ||
|
|
||
| pub fn marketplace_install_root(codex_home: &Path) -> PathBuf { |
There was a problem hiding this comment.
maybe have a marketplace.rs or something as this file becomes too big.
|
@codex review |
f4909a7 to
f9b23d5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9b23d58b9
ℹ️ 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
| if registry_path.is_file() { | ||
| return installed_marketplace_roots_from_registry(®istry_path); |
There was a problem hiding this comment.
Fall back to disk scan when registry parsing fails
installed_marketplace_roots immediately trusts known_marketplaces.json when it exists. If that file is malformed/truncated, installed_marketplace_roots_from_registry returns an empty list and no filesystem fallback runs, so valid marketplaces under .tmp/marketplaces vanish from discovery. A single bad registry file effectively disables all installed marketplaces.
Useful? React with 👍 / 👎.
codex-rs/cli/src/marketplace_cmd.rs
Outdated
| let backup = if destination.exists() { | ||
| let parent = destination | ||
| .parent() | ||
| .context("marketplace destination has no parent")?; | ||
| let staging_root = marketplace_staging_root(parent); | ||
| fs::create_dir_all(&staging_root)?; | ||
| let backup = tempfile::Builder::new() | ||
| .prefix("marketplace-backup-") | ||
| .tempdir_in(&staging_root)?; | ||
| let backup_root = backup.path().join("previous"); | ||
| fs::rename(destination, &backup_root)?; | ||
| Some((backup, backup_root)) |
There was a problem hiding this comment.
Prevent concurrent add from overwriting existing marketplace
run_add rejects existing destinations, but replace_marketplace_root still replaces any destination that appears later. If another marketplace add creates the same directory between the check and this rename, this process moves the existing install aside and overwrites it. That TOCTOU race causes lost updates instead of the expected conflict error.
Useful? React with 👍 / 👎.
| fn marketplace_registry_path(codex_home: &Path) -> PathBuf { | ||
| codex_home.join(".tmp").join(KNOWN_MARKETPLACES_FILE) | ||
| } |
There was a problem hiding this comment.
why do we need known_marketplaces.json? if marketplaces added via this CLI command are installed to a static location, can't we just walk that dir to find installed marketplaces?
There was a problem hiding this comment.
chatted offline, its to track metadata about plugin source.
codex-rs/cli/src/marketplace_cmd.rs
Outdated
| /// Sparse-checkout paths to use while cloning git sources. | ||
| #[arg(long = "sparse", value_name = "PATH", num_args = 1..)] | ||
| sparse_paths: Vec<String>, | ||
| } |
There was a problem hiding this comment.
i dont think this works (--sparse swallows source):
codex marketplace add --sparse plugins/foo owner/repo
what about something like:
codex marketplace add owner/repo --sparse plugins/foo?
codex-rs/cli/src/marketplace_cmd.rs
Outdated
| let marketplace_name = validate_marketplace_root(&staged_root) | ||
| .with_context(|| format!("failed to validate marketplace from {}", source.display()))?; |
There was a problem hiding this comment.
i think plugins require marketplace names to be only ASCII alnum / _ / -, should we enforce that here as well?
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 972d3a5495
ℹ️ 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.display() | ||
| ); | ||
| println!("Installed marketplace root: {}", existing_root.display()); | ||
| return Ok(()); |
There was a problem hiding this comment.
Re-record registry on idempotent source adds
When marketplace add finds an already-installed source, it returns immediately without calling record_installed_marketplace_root. If known_marketplaces.json exists but is missing that entry, discovery trusts the registry and skips disk scan, so the marketplace stays invisible even though the CLI reports it is already added.
Useful? React with 👍 / 👎.
| if !metadata_path.is_file() { | ||
| continue; | ||
| } | ||
| let metadata = read_marketplace_source_metadata(&root)?; |
There was a problem hiding this comment.
Skip bad metadata entries while scanning installs
A single malformed .codex-marketplace-source file aborts installed_marketplace_root_for_source because parse errors are propagated. That prevents adding unrelated marketplaces whenever one existing install has corrupted metadata, turning one bad entry into a global failure.
Useful? React with 👍 / 👎.
sayan-oai
left a comment
There was a problem hiding this comment.
thanks for cleaning up! one nit
| source.starts_with("git@") && source.contains(':') | ||
| } | ||
|
|
||
| fn is_http_git_url(source: &str) -> bool { |
There was a problem hiding this comment.
nit-ish: feels like theres a lot of local FS operations/git operations alongside the main CLI command parsing.
maybe worth breaking up those ops into another file, or moving stuff to plugins module?
will leave it up to you
e8fa02e to
972d3a5
Compare
Added a new top-level
codex marketplace addcommand for installing plugin marketplaces into Codex’s local marketplace cache.This change adds source parsing for local directories, GitHub shorthand, and git URLs, supports optional
--refand git-only--sparsecheckout paths, stages the source in a temp directory, validates the marketplace manifest, and installs it under$CODEX_HOME/marketplaces/<marketplace-name>Included tests cover local install behavior in the CLI and marketplace discovery from installed roots in core. Scoped formatting and fix passes were run, and targeted CLI/core tests passed.