Skip to content

feat: support product-scoped plugins.#15041

Merged
xl-openai merged 2 commits intomainfrom
xl/plugins3
Mar 19, 2026
Merged

feat: support product-scoped plugins.#15041
xl-openai merged 2 commits intomainfrom
xl/plugins3

Conversation

@xl-openai
Copy link
Collaborator

@xl-openai xl-openai commented Mar 18, 2026

  1. Added SessionSource::Custom(String) and --session-source.
  2. Enforced plugin and skill products by session_source.
  3. Applied the same filtering to curated background refresh.

Copy link
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +1259 to +1266
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remove disallowed curated plugins from active cache

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 👍 / 👎.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK.

@xl-openai
Copy link
Collaborator Author

@codex review

Copy link
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

plugins_manager
.install_plugin_with_remote_sync(&config, auth.as_ref(), request)
.await
} else {
plugins_manager.install_plugin(request).await
};

P1 Badge 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".

Comment on lines +2358 to +2359
| SessionSource::SubAgent(_)
| SessionSource::Unknown => Some(Product::Codex),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve parent product for sub-agent sessions

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 👍 / 👎.

@xl-openai xl-openai force-pushed the xl/plugins3 branch 2 times, most recently from 70861d7 to 1955609 Compare March 18, 2026 11:12
sayan-oai

This comment was marked as spam.

@sayan-oai sayan-oai dismissed their stale review March 18, 2026 17:54

Posted in error by Codex; user requested a local-only review instead.

};
if cwds.is_empty() {
cwds = vec![cwd];
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +189 to +192
let outcome = crate::skills::filter_skill_load_outcome_for_product(
finalize_skill_outcome(load_skills_from_roots(roots), &config_layer_stack),
self.restriction_product,
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to check for product restrictions here too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call add some logic to the actual sync time.

let plugin = marketplace
.plugins
.into_iter()
.find(|plugin| plugin.name == request.plugin_name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this also could go either way, but do we want to enforce product restrictions on plugin/read?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call will add PluginNotFound

"exec" => SessionSource::Exec,
"mcp" | "appserver" | "app-server" | "app_server" => SessionSource::Mcp,
"unknown" => SessionSource::Unknown,
_ => SessionSource::Custom(trimmed.to_string()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@sayan-oai sayan-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@xl-openai xl-openai force-pushed the xl/plugins3 branch 4 times, most recently from cac3148 to 78e19a7 Compare March 19, 2026 04:56
| SessionSource::Exec
| SessionSource::Mcp
| SessionSource::Unknown => Some(Product::Codex),
SessionSource::SubAgent(_) => None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confirming, we are ok with subagents not inheriting the restriction product of parent thread? IIUC this means subagents cant use product-scoped plugins like their parents can

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is ok the ThreadManager is only inited once and the product should be set already.

@xl-openai xl-openai merged commit db5781a into main Mar 19, 2026
54 of 56 checks passed
@xl-openai xl-openai deleted the xl/plugins3 branch March 19, 2026 07:46
@github-actions github-actions bot locked and limited conversation to collaborators Mar 19, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants