Conversation
xl-openai
force-pushed
the
xl/plugins
branch
2 times, most recently
from
5bc3961 to
3f18602
Compare
codex-rs/core/src/codex.rs
Outdated
| err.message | ||
| ); | ||
| } | ||
| for plugin in loaded_plugins |
There was a problem hiding this comment.
can this be logged in plugins_manager.plugins_for_config ?
|
|
||
| impl Error for SkillParseError {} | ||
|
|
||
| pub fn load_skills(config: &Config) -> SkillLoadOutcome { |
There was a problem hiding this comment.
Cleanup of functions not longer in use.
xl-openai
force-pushed
the
xl/plugins
branch
5 times, most recently
from
0842dc7 to
b68896a
Compare
sayan-oai
left a comment
sayan-oai
left a comment
There was a problem hiding this comment.
one main question about different config sources for loading plugins
codex-rs/core/src/plugins.rs
Outdated
| return PluginLoadOutcome::default(); | ||
| } | ||
|
|
||
| if !force_reload && let Some(outcome) = self.cached_outcome() { |
There was a problem hiding this comment.
plugins() is reading the global config stack reloaded from disk, but plugins_for_config() above reads from the session's resolved config_layer_stack.
yet both write to/read from the same cache slot.
so skills_for_cwd() can reuse plugin roots from a different config stack
There was a problem hiding this comment.
configured_plugins_from_stack currently reads only from the user layer, so the behavior is consistent for now. We should decide whether we actually need per-cwd plugin loading at all.
There was a problem hiding this comment.
Added a comment to the function.
There was a problem hiding this comment.
hmm, i dont see a need for per-cwd plugin loading right now
There was a problem hiding this comment.
Actually maybe it would be useful. Let me add it back.
sayan-oai
left a comment
sayan-oai
left a comment
There was a problem hiding this comment.
moving back to per-cwd cache, other than that lgtm
xl-openai
force-pushed
the
xl/plugins
branch
from
ec1be01 to
8da5d5c
Compare
|
@codex review again |
![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: 08033c8c0e
ℹ️ 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".
| if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(cwd) { | ||
| return outcome; |
There was a problem hiding this comment.
plugins_for_layer_stack reuses cached plugin outcomes based only on cwd. Since plugins_for_config always calls it with force_reload = false, edits to [plugins] (or plugin paths) for the same cwd can keep returning stale skill roots/MCP servers, including during app-server refresh flows, until some separate clear_cache happens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yes but there will be a force_reload endpoint later.
| Features::from_config(&config_toml, &config_profile, FeatureOverrides::default()); | ||
| features.enabled(Feature::Plugins) |
There was a problem hiding this comment.
Plugin gating is recomputed from raw layer-stack TOML with FeatureOverrides::default() instead of the already-resolved Config.features. If runtime profile/override selection differs from config_toml.profile, plugin loading can be incorrectly disabled/enabled for the session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/core/src/mcp/skill_dependencies.rs
Lines 151 to 152 in 08033c8
Check MCP dependencies against effective server set
maybe_prompt_and_install_mcp_dependencies uses config.mcp_servers to decide what is already installed, but plugin servers are merged only via McpManager. As a result, plugin-provided dependencies are treated as missing, users get incorrect install prompts, and maybe_install_mcp_dependencies can write duplicate MCP entries into global config.
ℹ️ 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".
3 participants
Support loading plugins.
Plugins can now be enabled via [plugins.] in config.toml. They are loaded as first-class entities through PluginsManager, and their default skills/ and .mcp.json contributions are integrated into the existing skills and MCP flows.