[codex] Add managed MCP server matchers - #29648
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 251056df68
ℹ️ 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".
c70065c to
4b838e8
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| let mut merged_toml = TomlValue::Table(toml::map::Map::new()); | ||
| for layer in &layers { | ||
| merge_toml_values(&mut merged_toml, &layer.regular_toml); | ||
| replace_mcp_server_requirements(&mut merged_toml, &layer.regular_toml); |
There was a problem hiding this comment.
Could we reuse/extend the existing identity shape for matcher-based requirements (e.g. identity.command = { match = ..., value = ... }) so all MCP requirement variants stay under the same key and we don’t need this extra MCP-specific replacement pass?
There was a problem hiding this comment.
The replacement behavior is not only needed because matchers currently sit outside identity. There is already a latent edge case with the existing identity contract:
# Lower layer
[mcp_servers.proxy.identity]
command = "old-command"
# Higher layer
[mcp_servers.proxy.identity]
url = "https://example.com/mcp"The generic recursive merge produces:
[mcp_servers.proxy.identity]
command = "old-command"
url = "https://example.com/mcp"McpServerIdentity is an untagged enum. Serde tries the command variant first, and unknown fields are tolerated, so this silently deserializes as the lower command identity and ignores the higher-priority URL.
Therefore, even if matcher forms were nested under identity, each same-name MCP requirement would still need to be replaced atomically across layers. I’ve moved that generic atomic-override mechanism into merge.rs and added documentation and coverage.
4b838e8 to
ef28c60
Compare
d5a13af to
39cb801
Compare
39cb801 to
f42f626
Compare
Summary
This PR extends the existing managed
mcp_serversidentity requirement so that one name-qualified rule can use either:Matcher-based rules stay under the released
identitykey and use the sameMcpServerRequirementabstraction andmcp_servers.<server_name>namespace.Behavior
Policy activation and name qualification are unchanged:
mcp_serversis absent, ordinary configured MCP servers remain unrestricted.mcp_serversis present, a server needs a matching same-name requirement.mcp_servers = {}continues to deny every configured MCP server.Plugin-bundled MCP servers use the same requirement shapes under
plugins.<plugin_name>.mcp_servers.<server_name>. Top-level non-empty rules continue to govern only ordinary configured servers; plugin rules remain explicitly plugin-scoped. The existing globally emptymcp_servers = {}plugin kill switch is preserved.Requirements layers continue to use the existing regular TOML merge behavior. Atomic replacement of named MCP requirements is intentionally out of scope here and is tracked independently in #30118.
Requirement contract
The released exact identity contract remains valid:
Command identities continue to check only
command; they do not inspect arguments,cwd,env, orenv_vars.A command matcher uses an exact executable plus an exact-length, ordered argument list. Each argument position supports
exact,prefix, or full-valueregexmatching:Direct streamable HTTP MCP definitions can use the same value matcher types through
identity.url:Plugin-bundled MCP matchers use the same contract inside the plugin-qualified allowlist:
Regexes are validated while managed requirements are loaded, and regex matching must cover the complete value. Command matchers constrain only the executable and arguments.
Why
Enterprise administrators need to allow MCP servers by executable and positional-argument shape, including fixed arguments plus constrained values such as internal MCP URLs passed to a proxy.
Validation
just fmtgit diff --checkjust test -p codex-config(198 passed)just test -p codex-core mcp_servers_by_matchers --lib(2 passed)