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: 2c36d0e447
ℹ️ 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/store.rs
Outdated
| if plugin_name.is_empty() || marketplace_name.is_empty() { | ||
| return Err(PluginStoreError::InvalidPluginKey(format!( | ||
| "invalid plugin key `{plugin_key}`; expected <plugin>@<marketplace>" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
parse_plugin_key only checks for empty strings, but plugin_root/install later join these values into filesystem paths. Keys like ../../etc@test (or manifest names with //absolute roots) can escape plugins/cache, letting plugin load/install read, overwrite, or delete paths outside codex home via remove_existing_target and recursive copy.
Useful? React with 👍 / 👎.
xl-openai
force-pushed
the
xl/plugins
branch
3 times, most recently
from
068231b to
abab47d
Compare
codex-rs/core/src/plugins/store.rs
Outdated
| Ok((plugin_name, marketplace_name)) | ||
| } | ||
|
|
||
| fn validate_plugin_key_segment( |
There was a problem hiding this comment.
optional nit: maybe just inline the two calls in parse_plugin_key? there's already a lot of standalone functions in this file
codex-rs/core/src/plugins/manager.rs
Outdated
|
|
||
| ConfigService::new_with_defaults(self.codex_home.clone()) | ||
| .write_value(ConfigValueWriteParams { | ||
| key_path: format!("plugins.{}", result.plugin_key), |
There was a problem hiding this comment.
we separate by . here but ConfigService::parse_key_path splits on ., so this would cause weird key splitting I think.
codex-rs/core/src/plugins/store.rs
Outdated
| .map(|_| plugin_name) | ||
| } | ||
|
|
||
| fn parse_plugin_key(plugin_key: &str) -> Result<(&str, &str), PluginStoreError> { |
There was a problem hiding this comment.
should we just make this a type rather than a specifically-formatted string? like PluginId {name, marketplace}, or something?
2 participants
Update config.toml plugin entries to use <plugin_name>@<marketplace_name> as the key.
Plugin now stays in [plugins/cache/marketplace-name/plugin-name/$version/]
Clean up the plugin code structure.
Add plugin install functionality (not used yet).