fix(codexcli): translate the MCP keys Codex reads under different names - #2608
Merged
Conversation
The codex MCP adapter translated only enabledTools/disabledTools/envVars/ experimentalEnvironment; every other canonical field reached config.toml under its canonical name. Verified against Codex rust-v0.146.1 RawMcpServerConfig: - tools: Codex declares it as a per-tool approval table, so the canonical string array is a serde type error that fails the whole server entry. It is now dropped with a warning on generate, and Codex's approval table is no longer lifted into the canonical model on import. - headers is written as http_headers (and imported back), so authenticated remote servers actually authenticate. On a stdio server Codex rejects http_headers outright, so it is dropped with a warning there. - timeout and networkTimeout are converted into tool_timeout_sec and startup_timeout_sec (milliseconds to seconds), both directions. startup_timeout_ms is imported verbatim unless the seconds spelling is also set, matching Codex's own precedence. - type/transport/alwaysAllow/trust and the Kiro lists are dropped silently. Part of #2496.
…rt on import Addresses review findings on PR #2608: - A negative timeout is now dropped with a warning in both directions. Codex builds a Duration out of tool_timeout_sec / startup_timeout_sec, and Duration::try_from_secs_f64 errors on a negative value, which fails the whole config.toml — the same failure class this PR fixes for tools. - http_headers is shape-checked on import (a table of string values) and passed through omitPrototypePollutionKeys, matching the generate direction, so a hand-written value cannot produce a .rulesync/mcp.jsonc the schema rejects. - Dropping the canonical type left an imported Codex server with no transport at all, since Codex states none of its own. A url server with no command now gets type: "http" restated on import, which keeps the round-trip stable for the adapters that branch on it. Adds warn assertions, malformed-value cases, and a full canonical -> codex -> canonical round-trip test.
This was referenced Aug 7, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the four MCP key-translation gaps recorded in the 2026-08-06 re-check comment on #2496.
src/features/mcp/codexcli-mcp.tstranslated onlyenabledTools/disabledTools/envVars/experimentalEnvironment; every other canonical field fell through the generic passthrough and landed inconfig.tomlunder the canonical name rather than the name Codex reads. Verified againstRawMcpServerConfigat rust-v0.146.1.tools(the bug). Codex declarestools: Option<HashMap<String, McpServerToolConfig>>, so the canonicalstring[]is a hard type error that fails the whole server entry rather than degrading. It is now stripped on generate with a warning pointing atenabledTools/disabledTools. Notools.<name>.approval_modeauthoring surface was invented — that is a design call, and the canonicaltoolsis a reservedstring[]. The same table is no longer lifted into the canonical model on import either: it is CLI-written approval state that the generate path already preserves in place, and its shape would not survive the canonical schema.headers→http_headerson generate and back on import, so an authenticated remote server actually authenticates. Codex errors with "http_headers is not supported for stdio", so on a stdio server the headers are dropped with a warning instead of failing the entry.timeout⇄tool_timeout_secandnetworkTimeout⇄startup_timeout_sec, converting milliseconds to seconds (a sub-second remainder stays fractional; Codex reads both asf64). Codex also acceptsstartup_timeout_msand prefersstartup_timeout_secwhen both are set, so import mirrors that precedence.type/transport(Codex infers the transport fromcommandversusurl),alwaysAllow,trust, and the Kiro authoring lists are dropped silently — they appear on nearly every canonical server, so warning on them would fire on every generate.Still open on #2496
This is a slice. The parent issue keeps its design items and stays open: the
[agents]/[[skills.config]]gateway-ownership question, plugin bundles, and a canonical spelling forbearer_token_env_var(which passes through under its own name today, as doesenv_http_headers).Testing
pnpm cicheck(full: code + content)npx vitest run --config vitest.e2e.config.ts src/e2e/e2e-mcp.spec.tsstartup_timeout_msprecedence. Two existing tests were updated to the new intent: the canonicaltoolsarray no longer reaches the file, andtypeis no longer written for remote servers.Part of #2496
🤖 Generated with Claude Code