Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 82 additions & 8 deletions evalbench/generators/models/codex_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ def __init__(self, querygenerator_config):

self.setup_config = querygenerator_config.get("setup", {})
self.config_path = os.path.join(self.codex_config_dir, "config.toml")
self.inline_mcp_servers = {}
self.enabled_plugins = {}
self._setup()

@staticmethod
Expand Down Expand Up @@ -248,9 +250,10 @@ def _setup(self):
either be a repo with a `skills/` child or a single skill folder.
"""
mcp_servers_config = self.setup_config.get("mcp_servers", {})
extra_config = dict(self._DEFAULT_TOP_LEVEL_CONFIG)
extra_config.update(self.setup_config.get("config", {}))
self._write_config_toml(mcp_servers_config, extra_config)
if isinstance(mcp_servers_config, list):
self._install_mcp_servers_from_repo(mcp_servers_config)
elif isinstance(mcp_servers_config, dict):
self.inline_mcp_servers.update(mcp_servers_config)

skills_config = self.setup_config.get("skills", [])
if skills_config:
Expand All @@ -260,6 +263,8 @@ def _setup(self):
if skills_dir_path:
self._setup_skills_from_dir(skills_dir_path)

self._write_config_toml()

def _setup_skills(self, skills: list):
"""Installs Codex skills from repo or local path configs."""
setup_env = os.environ.copy()
Expand All @@ -284,6 +289,15 @@ def _setup_skills(self, skills: list):
repo_dir = self._clone_extension_repo(url, plugins_dir, setup_env)
if repo_dir:
self._register_codex_plugin(repo_dir, skill_config)
plugin_name = skill_config.get("plugin_name") or skill_config.get("plugin") or self._read_codex_plugin_name(repo_dir)
if not plugin_name:
plugin_name = os.path.basename(os.path.abspath(repo_dir))
marketplace_name = skill_config.get("marketplace_name", "evalbench-local-marketplace")
plugin_id = f"{plugin_name}@{marketplace_name}"
# Enable the plugin in config.toml. Even for skills-only plugins,
# this is safe and ensures any plugin-specific configuration is passed
# to the plugin context, and matches the behavior of _install_mcp_servers_from_repo.
self.enabled_plugins.setdefault(plugin_id, {}).update(skill_config.get("config", {}) or {})

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should fix before merge — or at minimum verify. This is a silent behavior change for every existing skills: config that uses action: install_from_repo.

Before this PR, cloning a skill repo only registered it in the marketplace and copied SKILL folders — nothing was ever added to config.toml. After this PR, every such repo also gets a [plugins."<name>@evalbench-local-marketplace"] section with enabled = true written to config.toml (via self.enabled_plugins).

Whether that breaks anything depends on how Codex CLI handles an enabled = true on a plugin with no runtime component. Two ways forward:

  1. Confirm against an existing skills-only config that Codex is tolerant, and add a one-line comment here explaining why the skills and MCP paths deliberately share the enable-in-config behavior.
  2. If it's not safe / not intended, scope the enabled_plugins.setdefault(...) call to _install_mcp_servers_from_repo only and leave _setup_skills doing what it did before.

Either is fine; the current state (silent change with no comment) is the risky option.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verified. Codex CLI tolerates this behavior correctly and it is the correct way to pass configuration options for skills-only plugins. Added a comment explaining the design decision.

self._install_skills_from_source(repo_dir, skill_config)
elif action in ("copy", "link", "install") and path:
# Materialize the skill instead of symlinking so Codex sees a
Expand Down Expand Up @@ -346,7 +360,7 @@ def _clone_extension_repo(

def _register_codex_plugin(self, repo_dir: str, skill_config: dict):
"""Registers a local Codex plugin marketplace entry for cloned repos."""
plugin_name = skill_config.get("plugin_name") or self._read_codex_plugin_name(repo_dir)
plugin_name = skill_config.get("plugin_name") or skill_config.get("plugin") or self._read_codex_plugin_name(repo_dir)
if not plugin_name:
plugin_name = os.path.basename(os.path.abspath(repo_dir))

Expand All @@ -355,7 +369,7 @@ def _register_codex_plugin(self, repo_dir: str, skill_config: dict):
display_name = skill_config.get(
"marketplace_display_name", "EvalBench Local Skills")

plugins_dir = os.path.join(self.codex_config_dir, "plugins")
plugins_dir = os.path.join(self.fake_home, ".agents", "plugins")
os.makedirs(plugins_dir, exist_ok=True)
marketplace_path = os.path.join(plugins_dir, "marketplace.json")

Expand All @@ -375,11 +389,16 @@ def _register_codex_plugin(self, repo_dir: str, skill_config: dict):
logging.warning(
f"Failed to read Codex marketplace at {marketplace_path}: {e}")

# Compute path relative to fake_home so that it works as a local marketplace source
rel_path = os.path.relpath(os.path.abspath(repo_dir), self.fake_home)
if not (rel_path.startswith("./") or rel_path.startswith("../") or rel_path.startswith("/")):
rel_path = f"./{rel_path}"

entry = {
"name": plugin_name,
"source": {
"source": "local",
"path": os.path.abspath(repo_dir),
"path": rel_path,
},
"policy": {
"installation": "AVAILABLE",
Expand All @@ -400,6 +419,19 @@ def _register_codex_plugin(self, repo_dir: str, skill_config: dict):
logging.info(
f"Registered Codex plugin '{plugin_name}' in {marketplace_path}")

# Install the plugin via Codex CLI so it changes status from 'not installed' to 'installed, enabled'
cmd = ["npm", "exec", "--yes", self.codex_cli_version, "--", "plugin", "add", f"{plugin_name}@{marketplace_name}"]
try:
setup_env = os.environ.copy()
setup_env.update(self.env)
result = subprocess.run(cmd, env=setup_env, check=False, capture_output=True, text=True)
if result.returncode == 0:
logging.info(f"Successfully installed Codex plugin '{plugin_name}@{marketplace_name}'")
else:
logging.error(f"Failed to install Codex plugin '{plugin_name}@{marketplace_name}': {result.stderr.strip()}")
except Exception as e:
logging.error(f"Error executing plugin installation command: {e}")

@staticmethod
def _read_codex_plugin_name(repo_dir: str) -> str:
plugin_json_path = os.path.join(repo_dir, ".codex-plugin", "plugin.json")
Expand Down Expand Up @@ -468,7 +500,39 @@ def _find_skill_dirs(source_dir: str) -> list[str]:
if os.path.exists(os.path.join(source_dir, entry, "SKILL.md"))
]

def _write_config_toml(self, mcp_servers_config: dict, extra_config: dict):
def _install_mcp_servers_from_repo(self, mcp_servers: list):
"""Clones plugin repositories that bundle MCP servers and enables them."""
setup_env = os.environ.copy()
setup_env.update(self.env)

plugins_dir = os.path.join(self.codex_config_dir, "plugins")
os.makedirs(plugins_dir, exist_ok=True)

for mcp_config in mcp_servers:
if not isinstance(mcp_config, dict):
logging.warning(f"Unsupported MCP server config: {mcp_config}")
continue
action = mcp_config.get("action")
url = mcp_config.get("url")
if action != "install_from_repo" or not url:
logging.warning(
f"Unsupported MCP server config: {mcp_config}. When "
"'mcp_servers' is a list, each entry must use "
"'action: install_from_repo' with 'url'.")
continue
repo_dir = self._clone_extension_repo(url, plugins_dir, setup_env)
if repo_dir:
plugin_name = mcp_config.get("plugin") or self._read_codex_plugin_name(repo_dir)
if not plugin_name:
plugin_name = os.path.basename(os.path.abspath(repo_dir))
self._register_codex_plugin(repo_dir, mcp_config)
self._install_skills_from_source(repo_dir, mcp_config)

marketplace_name = mcp_config.get("marketplace_name", "evalbench-local-marketplace")
plugin_id = f"{plugin_name}@{marketplace_name}"
self.enabled_plugins.setdefault(plugin_id, {}).update(mcp_config.get("config", {}) or {})

def _write_config_toml(self):
"""Writes Codex CLI's `config.toml` with MCP server declarations.

Accepts the same Gemini-style MCP shape the rest of evalbench uses
Expand All @@ -486,18 +550,28 @@ def _write_config_toml(self, mcp_servers_config: dict, extra_config: dict):
"""
lines: list[str] = []

extra_config = dict(self._DEFAULT_TOP_LEVEL_CONFIG)
extra_config.update(self.setup_config.get("config", {}))

for key, value in extra_config.items():
lines.append(f"{key} = {self._toml_value(value)}")
if extra_config:
lines.append("")

for server_name, config in mcp_servers_config.items():
for server_name, config in self.inline_mcp_servers.items():
translated = self._translate_mcp_config(server_name, dict(config))
lines.append(f"[mcp_servers.{self._toml_key(server_name)}]")
for key, value in translated.items():
lines.append(f"{key} = {self._toml_value(value)}")
lines.append("")

for plugin_id, options in self.enabled_plugins.items():
lines.append(f"[plugins.{self._toml_key(plugin_id)}]")
lines.append("enabled = true")
if options:
lines.append(f"options = {self._toml_value(options)}")
lines.append("")

with open(self.config_path, "w") as f:
f.write("\n".join(lines).rstrip() + "\n")

Expand Down
51 changes: 51 additions & 0 deletions evalbench/test/codex_cli_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,54 @@ def test_execute_cli_command_default_cwd(mock_popen, monkeypatch):
mock_popen.assert_called_once()
kwargs = mock_popen.call_args.kwargs
assert kwargs.get("cwd") == generator.fake_home


@patch('generators.models.codex_cli.subprocess.run')
def test_register_codex_plugin_uses_merged_env(mock_run, monkeypatch):
monkeypatch.setenv("HOME", "/fake/real_home")
monkeypatch.setenv("PARENT_VAR", "parent_value")

with (
patch('generators.models.codex_cli.os.makedirs'),
patch('generators.models.codex_cli.open', create=True),
):
generator = CodexCliGenerator({"model": "gpt-4", "env": {"GENERATOR_VAR": "gen_value"}})

with (
patch('generators.models.codex_cli.os.path.exists', return_value=False),
patch('generators.models.codex_cli.open', create=True),
patch('generators.models.codex_cli.json.dump'),
):
generator._register_codex_plugin("/fake/repo_dir", {"plugin_name": "my-plugin"})

mock_run.assert_called_once()
kwargs = mock_run.call_args.kwargs
passed_env = kwargs.get("env")
assert passed_env is not None
assert passed_env.get("PARENT_VAR") == "parent_value"
assert passed_env.get("GENERATOR_VAR") == "gen_value"
assert passed_env.get("HOME") == generator.fake_home


def test_write_config_toml_escapes_plugin_id(monkeypatch, tmp_path):
monkeypatch.setenv("HOME", "/fake/real_home")

with (
patch('generators.models.codex_cli.os.makedirs'),
patch('generators.models.codex_cli.open', create=True),
):
generator = CodexCliGenerator({"model": "gpt-4"})

generator.enabled_plugins = {
"dak@evalbench-local-marketplace": {"opt1": "val1"},
"clean_plugin": {}
}

config_file = tmp_path / "config.toml"
generator.config_path = str(config_file)

generator._write_config_toml()

content = config_file.read_text()
assert '[plugins."dak@evalbench-local-marketplace"]' in content
assert '[plugins.clean_plugin]' in content
Loading