Support detect and import MCP, Subagents, hooks, commands from external#19949
Support detect and import MCP, Subagents, hooks, commands from external#19949alexsong-oai merged 15 commits intomainfrom
Conversation
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0fb610fff
ℹ️ 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".
| fn append_convertible_hook_groups(settings: &JsonValue, migration: &mut HookMigration) { | ||
| let Some(hooks_config) = settings.get("hooks").and_then(JsonValue::as_object) else { | ||
| return; |
There was a problem hiding this comment.
Honor disableAllHooks when importing hooks
Hook migration ignores the source disableAllHooks flag and still imports active hook handlers from hooks. That changes behavior: users who explicitly disabled hooks in external config will get hooks enabled after migration. Detection/import should skip hook migration (or preserve disabled state) when this flag is true.
Useful? React with 👍 / 👎.
| fn command_skill_name(source_commands: &Path, source_file: &Path) -> String { | ||
| slugify_name(&format!( | ||
| "{COMMAND_SKILL_PREFIX}-{}", | ||
| command_source_name(source_commands, source_file) | ||
| )) |
There was a problem hiding this comment.
Disambiguate colliding command skill slugs
command_skill_name normalizes paths through slugify_name, so different commands can collapse to the same skill directory (e.g. foo_bar.md and foo-bar.md). import_commands treats target_dir.exists() as already migrated and silently skips later collisions, losing commands. Use a collision-safe naming scheme or fail with an explicit error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Commands with name collisions will be ignored.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/app-server/src/config/external_agent_config.rs
Lines 236 to 240 in 161389c
MCP migration loads only .claude/settings.json for enabledMcpjsonServers/disabledMcpjsonServers. Those are hierarchical settings, and .claude/settings.local.json can override them. Ignoring local overrides produces wrong enabled=false decisions and incorrect detect/import results.
ℹ️ 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".
# Conflicts: # codex-rs/app-server-protocol/schema/json/ClientRequest.json # codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json # codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json # codex-rs/app-server-protocol/schema/json/v2/ExternalAgentConfigDetectResponse.json # codex-rs/app-server-protocol/schema/json/v2/ExternalAgentConfigImportParams.json # codex-rs/app-server-protocol/schema/typescript/v2/ExternalAgentConfigMigrationItemType.ts # codex-rs/app-server-protocol/src/protocol/v2.rs # codex-rs/app-server/src/config/external_agent_config.rs # codex-rs/app-server/src/external_agent_config_api.rs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0508e55d0
ℹ️ 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 review |
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/app-server/src/message_processor.rs
Lines 1135 to 1146 in 66755e8
handle_external_agent_config_import() only treats Plugins as config-mutating. New import types (notably McpServerConfig and Hooks) update .codex/config.toml / hooks.json, but handle_config_mutation() is skipped, so running sessions can keep stale configuration until restart.
ℹ️ 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 review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cab4cfa4a
ℹ️ 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 review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0762f5fcff
ℹ️ 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".
| JsonValue::Bool(value) => Some(value.to_string()), | ||
| JsonValue::Number(value) => Some(value.to_string()), |
There was a problem hiding this comment.
Reject non-string MCP command/url values
json_string coerces booleans and numbers into strings, and mcp_server_toml_table accepts that output for MCP command/url. Invalid source config like { "command": true } is migrated as command = "true" instead of being skipped, creating unusable MCP entries and violating the commit’s conservative-import behavior.
Useful? React with 👍 / 👎.
|
|
||
| let enabled_servers = settings | ||
| .and_then(|settings| settings.get("enabledMcpjsonServers")) | ||
| .map(json_string_vec) |
There was a problem hiding this comment.
Ignore non-string MCP allow/deny list entries
enabledMcpjsonServers and disabledMcpjsonServers are parsed through json_string_vec, which stringifies booleans/numbers. A malformed value like enabledMcpjsonServers: true creates a non-empty allow-list ("true"), causing mcp_server_is_disabled to filter out every real server. Non-string entries should be ignored, not treated as server names.
Useful? React with 👍 / 👎.
Why
This PR expands the migration path so Codex can detect and import MCP server config, hooks, commands, and subagents configs in a Codex-native shape.
What changed
codex-external-agent-migrationcrate that owns conversion logic for external-agent MCP servers, hooks, commands, and subagents.Migration strategy
The migration is intentionally conservative: Codex only imports external-agent config that can be represented safely in Codex today. Unsupported or ambiguous config is skipped instead of being partially translated into behavior that may not match the source system.
MCP servers: import supported stdio and HTTP MCP server definitions into
mcp_servers. Disabled servers and servers filtered out by sourceenabledMcpjsonServers/disabledMcpjsonServersare skipped. Project-scoped MCP entries from.claude.jsonare included when they match the repo path.Hooks: import only supported command hooks into
.codex/hooks.json. Unsupported hook features such as conditional groups, async handlers, prompt/http hooks, or unknown fields are skipped. Referenced hook scripts are copied into.codex/hooks/, preserving any existing target scripts.Commands: import supported external commands as Codex skills under
.agents/skills/source-command-*. Commands that rely on source runtime expansion such as$ARGUMENTS,$1,@filereferences, shell interpolation, or colliding generated names are skipped.Subagents: import valid subagent Markdown files into
.codex/agents/*.tomlwhen they have the minimum Codex agent fields. Source model names are not migrated, so imported agents keep the user’s Codex default model; compatible reasoning effort and sandbox mode are migrated when present.Skills and project guidance: copy missing skill directories into
.agents/skillsand migrateCLAUDE.mdguidance intoAGENTS.md, rewriting source-agent terminology to Codex terminology where appropriate.Detection details: detected migration items include lightweight details for UI preview, such as MCP server names, hook event names, generated command skill names, and subagent names. Import still recomputes from disk instead of trusting details as the source of truth.
Adds focused coverage for the new migration behavior and app-server import flow.
Verification
cargo test -p codex-external-agent-migrationcargo test -p codex-hookscargo test -p codex-app-server external_agent_configjust bazel-lock-check