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: dca59a99d4
ℹ️ 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 product_restricted_plugin_names.contains(&plugin_id.plugin_name) { | ||
| info!( | ||
| plugin = plugin_id.plugin_name, | ||
| marketplace = OPENAI_CURATED_MARKETPLACE_NAME, | ||
| session_source = %session_source, | ||
| "skipping curated plugin cache refresh for product-restricted plugin" | ||
| ); | ||
| continue; |
There was a problem hiding this comment.
refresh_curated_plugin_cache skips product-restricted configured plugins with continue, but never uninstalls or clears an already-active cached version. load_plugin still resolves active_plugin_root for configured entries, so switching session_source can leave previously installed disallowed plugins active, defeating product scoping.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/app-server/src/codex_message_processor.rs
Lines 5617 to 5622 in d78c7c7
Enforce product policy in plugin/install
plugin/list now filters by policy.products, but plugin_install still installs any named plugin without checking session_source. Because install_plugin* receives no source restriction, a client can call plugin/install directly with a disallowed plugin name/path and bypass product gating.
ℹ️ 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/protocol/src/protocol.rs
Outdated
| | SessionSource::SubAgent(_) | ||
| | SessionSource::Unknown => Some(Product::Codex), |
There was a problem hiding this comment.
Do not hard-map all SubAgent sources to Product::Codex. Sub-agents spawned from a custom source (e.g. chatgpt) will be evaluated as Codex, so product-scoped skill/plugin filtering can switch products mid-thread and expose the wrong gated content.
Useful? React with 👍 / 👎.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
sayan-oai
dismissed
their stale review
Posted in error by Codex; user requested a local-only review instead.
| }; | ||
| if cwds.is_empty() { | ||
| cwds = vec![cwd]; | ||
| } |
There was a problem hiding this comment.
why do we need this change? to make cwds mut? this also grabs the session state lock regardless of whether cwds is empty, which seems mildly worse.
| let outcome = crate::skills::filter_skill_load_outcome_for_product( | ||
| finalize_skill_outcome(load_skills_from_roots(roots), &config_layer_stack), | ||
| self.restriction_product, | ||
| ); |
There was a problem hiding this comment.
nit: it seems a bit tedious to have this pattern of calling 3 helpers to get the outcome. you have to remember too much as the user. is it possible to clean up, or make one wrapper?
| Option<String>, | ||
| )>::new(); | ||
| let mut local_plugin_names = HashSet::new(); | ||
| for plugin in curated_marketplace.plugins { |
There was a problem hiding this comment.
do we need to check for product restrictions here too?
| let plugin = marketplace | ||
| .plugins | ||
| .into_iter() | ||
| .find(|plugin| plugin.name == request.plugin_name); |
There was a problem hiding this comment.
this also could go either way, but do we want to enforce product restrictions on plugin/read?
| "exec" => SessionSource::Exec, | ||
| "mcp" | "appserver" | "app-server" | "app_server" => SessionSource::Mcp, | ||
| "unknown" => SessionSource::Unknown, | ||
| _ => SessionSource::Custom(trimmed.to_string()), |
There was a problem hiding this comment.
do we want to coerce to lowercase? the values we match against in rollout/mod.rs is lowercase
| { | ||
| plugin_sources.insert(plugin_name, source_path); | ||
| } else { | ||
| product_restricted_plugin_names.insert(plugin_name); |
There was a problem hiding this comment.
product_restricted_plugin_names is a bit confusing IMO; maybe disallowed_plugin_names_for_product? because restriction_product means the product for this session and we are reusing restricted in a confusing way.
sayan-oai
left a comment
sayan-oai
left a comment
There was a problem hiding this comment.
small clarity nit:
maybe for readers it'd be nice to include a doc comment somewhere explaining that for skills:
- if the plugin doesnt match the requested product, its filtered out
- if the plugin matches but some bundled skills dont match, theyre filtered out
- separately, if there are individual skills that dont match, theyre filtered out
you have to bounce around a few files to figure that out right now.
2 participants
Uh oh!
There was an error while loading. Please reload this page.