feat: align Codex MCP setup with native CLI#1110
Open
JMartinezRuiz wants to merge 3 commits intoCoplayDev:betafrom
Open
feat: align Codex MCP setup with native CLI#1110JMartinezRuiz wants to merge 3 commits intoCoplayDev:betafrom
JMartinezRuiz wants to merge 3 commits intoCoplayDev:betafrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds CLI-first Codex registration with fallback to direct ~/.codex/config.toml writes, verifies remote-hosted X-API-Key in TOML during status checks (treating missing/stale header as VersionMismatch), introduces Register/Unregister flows and CLI/path helpers, and updates tests and docs for Codex setup. ChangesCodex client configurator, TOML helpers, tests & docs
Sequence DiagramsequenceDiagram
participant User as User / Editor
participant Config as Codex Configurator
participant Helper as CodexConfigHelper
participant TOML as ~/.codex/config.toml
participant CLI as Codex CLI
participant Prefs as EditorPrefs
rect rgba(100, 200, 150, 0.5)
Note over Config,CLI: Register Flow
User->>Config: Call Register()
Config->>Config: ResolveCodexCliPath()
alt CLI Found
Config->>CLI: codex mcp add unityMCP ...
alt Success
Config->>Config: Set Configured + transport
else Failure
Config->>Helper: RegisterWithToml()
end
else CLI Not Found
Config->>Helper: RegisterWithToml()
end
Helper->>Prefs: Read ApiKey (if remote)
alt Remote + ApiKey
Helper->>Helper: AddRemoteAuthHeaderIfNeeded()
end
Helper->>TOML: Write/patch unityMCP server block
Helper-->>Config: Done
end
rect rgba(150, 100, 200, 0.5)
Note over Config,TOML: Status Check Flow
User->>Config: Call CheckStatus()
Config->>Config: Validate remote URL match
alt Remote HTTP Mode
Config->>Helper: HasCodexHttpHeader(toml, ApiKeyHeader, expectedValue)
Helper->>TOML: Parse and read http_headers
Helper-->>Config: Header present & valid?
alt Header Missing/Stale
Config->>Config: Force VersionMismatch
end
end
end
rect rgba(200, 150, 100, 0.5)
Note over Config,TOML: Unregister Flow
User->>Config: Call Unregister()
Config->>CLI: codex mcp remove unityMCP
alt Success or CLI Missing
Config->>Helper: RemoveCodexServerBlock(toml)
Helper->>TOML: Parse and remove unityMCP entry
Helper->>TOML: Delete empty server tables
Helper-->>Config: Updated TOML
end
Config->>Config: Unregistration complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
docs/guides/CODEX_HELP.md (1)
57-63: 💤 Low valueWindows
SystemRootsnippet: consider quoting the path for shell safety.
SystemRoot=C:\Windowsworks in bash because\Wis a no-op escape, and on cmd.exe backslashes are literal. It is fine for the default value, but if a user copies the pattern with a path containing spaces (C:\Program Files\...) or other shell metachars, the unquoted form will silently break. A small quoting tweak makes the snippet robust:-codex mcp add --env SystemRoot=C:\Windows unityMCP -- uvx --from mcpforunityserver mcp-for-unity --transport stdio +codex mcp add --env "SystemRoot=C:\Windows" unityMCP -- uvx --from mcpforunityserver mcp-for-unity --transport stdio🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/CODEX_HELP.md` around lines 57 - 63, The snippet in docs/guides/CODEX_HELP.md uses an unquoted env assignment (SystemRoot=C:\Windows) which will break for paths with spaces or special chars; update the example command (the codex mcp add ... --env SystemRoot=... invocation) to quote the assigned path (e.g., wrap the value in double quotes) and mention quoting when explaining using an absolute uvx path so the command is robust across shells and Windows paths.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
90-104: ⚡ Quick winClear
HttpRemoteBaseUrlinSetUpfor full test isolation.
HttpRemoteBaseUrlis captured/restored inOneTimeSetUp/OneTimeTearDown, but unlikeHttpTransportScopeandApiKeyit is not deleted inSetUp. AfterBuildCodexServerBlock_RemoteHttpModeWithApiKey_IncludesHttpHeaders(Line 563) sets it to"https://unity-mcp.example", that value leaks into subsequent tests. It happens to be harmless today becauseHttpTransportScopeis cleared (soIsRemoteScope()returns false), but adding the deletion now guards future remote-mode tests from picking up an unintended base URL.🧹 Proposed SetUp change
EditorPrefs.DeleteKey(EditorPrefKeys.HttpTransportScope); + EditorPrefs.DeleteKey(EditorPrefKeys.HttpRemoteBaseUrl); EditorPrefs.DeleteKey(EditorPrefKeys.ApiKey);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs` around lines 90 - 104, In SetUp(), also delete the EditorPrefs key for EditorPrefKeys.HttpRemoteBaseUrl so per-test state is isolated; update the SetUp method (the one that currently deletes EditorPrefKeys.GitUrlOverride, HttpTransportScope, ApiKey and sets DevModeForceServerRefresh) to call EditorPrefs.DeleteKey(EditorPrefKeys.HttpRemoteBaseUrl) before calling EditorConfigurationCache.Instance.Refresh(); this mirrors the cleanup done in OneTimeSetUp/OneTimeTearDown and prevents HttpRemoteBaseUrl set by tests like BuildCodexServerBlock_RemoteHttpModeWithApiKey_IncludesHttpHeaders from leaking into later tests.MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (1)
27-86: ⚡ Quick winConsider deduplicating
BuildCodexServerBlockandCreateUnityMcpTable.Both methods now construct an identical
unityMCPserver table (HTTP url + remote auth header, or stdio command/args/env/timeout). After this PR they both callAddRemoteAuthHeaderIfNeeded, so the two HTTP branches are effectively the same code. HavingBuildCodexServerBlockcallCreateUnityMcpTable(uvPath)and then add features/serialize would reduce drift risk for future changes (e.g., header logic, env handling).♻️ Sketch of the consolidation
public static string BuildCodexServerBlock(string uvPath) { var table = new TomlTable(); var mcpServers = new TomlTable(); - var unityMCP = new TomlTable(); - - // Check transport preference - bool useHttpTransport = EditorPrefs.GetBool(MCPForUnity.Editor.Constants.EditorPrefKeys.UseHttpTransport, true); - - if (useHttpTransport) - { - // ... duplicated HTTP setup ... - EnsureRmcpClientFeature(table); - } - else - { - // ... duplicated stdio setup ... - } + var unityMCP = CreateUnityMcpTable(uvPath); + if (EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, true)) + { + EnsureRmcpClientFeature(table); + } mcpServers["unityMCP"] = unityMCP; table["mcp_servers"] = mcpServers; ... }Also applies to: 227-274
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Helpers/CodexConfigHelper.cs` around lines 27 - 86, Refactor BuildCodexServerBlock to reuse CreateUnityMcpTable(uvPath) instead of duplicating unityMCP construction: call CreateUnityMcpTable(uvPath) to obtain the unityMCP TomlTable, then attach it to mcpServers/table and serialize as before; ensure CreateUnityMcpTable contains the transport branching (use HttpEndpointUtility.GetMcpRpcUrl, AddRemoteAuthHeaderIfNeeded, and EnsureRmcpClientFeature for HTTP; AssetPathUtility.GetUvxCommandParts, AddUvxModeFlags, AssetPathUtility.GetBetaServerFromArgsList, Windows env via MCPServiceLocator.Platform.IsWindows/GetSystemRoot, and startup_timeout_sec for stdio) so behavior is identical, and update any related callers to preserve existing features and side-effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs`:
- Around line 638-643: The remote-scope-without-key case is silently falling
through to the CLI registration path; update the registration flow so when
EditorConfigurationCache.Instance.UseHttpTransport &&
HttpEndpointUtility.IsRemoteScope() is true but
EditorPrefs.GetString(EditorPrefKeys.ApiKey, string.Empty) is empty, the code
does not call the CLI registration in Register() and instead surfaces a clear
warning/error and aborts registration. Implement a guard (either in
ShouldUseTomlForRemoteAuth or at the start of Register()) that checks for
IsRemoteScope() + empty API key and then logs/alerts the user (e.g.,
EditorUtility.DisplayDialog or Debug.LogWarning) and returns early rather than
proceeding to call the CLI add --url path.
- Around line 621-636: BuildCodexAddArgs currently places envArg before the
server name for the stdio transport path; change the returned stdio command
string in BuildCodexAddArgs so envArg appears after {CodexServerName} and before
the "--" token (i.e., format as "mcp add {CodexServerName}{envArg} --
{QuoteCliArg(uvxPath)} ... --transport stdio"), leaving all other variables
(uvxPath, devFlags, fromArgs, packageName) unchanged.
In `@MCPForUnity/Editor/Helpers/CodexConfigHelper.cs`:
- Around line 88-99: When TryParseToml returns null in RemoveCodexServerBlock,
do not silently return the original TOML; instead emit a visible warning so
callers (e.g., McpClientConfiguratorBase.RemoveCodexTomlRegistration) know the
file was not modified and manual cleanup of the unityMCP block in
~/.codex/config.toml may be required. Add a warning log right before the "return
existingToml ?? string.Empty;" in RemoveCodexServerBlock (use the project’s
logging facility or UnityEngine.Debug.LogWarning) that includes context about
the parse failure and the path/section (unityMCP) so the user can take manual
action.
---
Nitpick comments:
In `@docs/guides/CODEX_HELP.md`:
- Around line 57-63: The snippet in docs/guides/CODEX_HELP.md uses an unquoted
env assignment (SystemRoot=C:\Windows) which will break for paths with spaces or
special chars; update the example command (the codex mcp add ... --env
SystemRoot=... invocation) to quote the assigned path (e.g., wrap the value in
double quotes) and mention quoting when explaining using an absolute uvx path so
the command is robust across shells and Windows paths.
In `@MCPForUnity/Editor/Helpers/CodexConfigHelper.cs`:
- Around line 27-86: Refactor BuildCodexServerBlock to reuse
CreateUnityMcpTable(uvPath) instead of duplicating unityMCP construction: call
CreateUnityMcpTable(uvPath) to obtain the unityMCP TomlTable, then attach it to
mcpServers/table and serialize as before; ensure CreateUnityMcpTable contains
the transport branching (use HttpEndpointUtility.GetMcpRpcUrl,
AddRemoteAuthHeaderIfNeeded, and EnsureRmcpClientFeature for HTTP;
AssetPathUtility.GetUvxCommandParts, AddUvxModeFlags,
AssetPathUtility.GetBetaServerFromArgsList, Windows env via
MCPServiceLocator.Platform.IsWindows/GetSystemRoot, and startup_timeout_sec for
stdio) so behavior is identical, and update any related callers to preserve
existing features and side-effects.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs`:
- Around line 90-104: In SetUp(), also delete the EditorPrefs key for
EditorPrefKeys.HttpRemoteBaseUrl so per-test state is isolated; update the SetUp
method (the one that currently deletes EditorPrefKeys.GitUrlOverride,
HttpTransportScope, ApiKey and sets DevModeForceServerRefresh) to call
EditorPrefs.DeleteKey(EditorPrefKeys.HttpRemoteBaseUrl) before calling
EditorConfigurationCache.Instance.Refresh(); this mirrors the cleanup done in
OneTimeSetUp/OneTimeTearDown and prevents HttpRemoteBaseUrl set by tests like
BuildCodexServerBlock_RemoteHttpModeWithApiKey_IncludesHttpHeaders from leaking
into later tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77d4188e-ce86-4595-be2a-fa103e77961a
📒 Files selected for processing (8)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Helpers/CodexConfigHelper.csMCPForUnity/README.mdREADME.mdTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.csdocs/guides/CODEX_HELP.mddocs/guides/MCP_CLIENT_CONFIGURATORS.mddocs/i18n/README-zh.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/guides/CODEX_HELP.md (1)
65-75: 💤 Low valueLGTM! Clear remote auth documentation.
The TOML configuration example is syntactically correct and clearly explains when direct TOML editing is necessary. The note about the CLI's current limitation regarding arbitrary HTTP headers appropriately sets expectations.
Optional security consideration: While storing API keys in
~/.codex/config.tomlis a standard pattern for CLI tools, you might consider adding a brief note about file permissions or referencing Codex's documentation on secure credential management, if applicable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/CODEX_HELP.md` around lines 65 - 75, Add a brief security note after the Remote-hosted auth TOML example advising users to restrict permissions on their config file (e.g., set restrictive file permissions like 600) and/or use secure credential management; reference the config file path (~/.codex/config.toml) and the section/header name ([mcp_servers.unityMCP] / http_headers) so readers know exactly where the secret is stored and add a pointer to Codex docs on secure credential management if available.MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
682-690: 💤 Low valueSkip the file write when nothing changed to avoid unnecessary I/O on parse failure.
When
RemoveCodexServerBlockhits the parse-failure path (warning logged, original content returned),WriteAtomicFilestill rewrites the file with the same bytes. This is harmless but wasteful, and slightly muddies the "file unchanged on parse failure" guarantee that the warning message implies. A simpleif (!ReferenceEquals(updatedToml, existingToml))check (or comparing strings) avoids the no-op write.♻️ Proposed refactor
private void RemoveCodexTomlRegistration() { string path = GetConfigPath(); if (!File.Exists(path)) return; string existingToml = File.ReadAllText(path); string updatedToml = CodexConfigHelper.RemoveCodexServerBlock(existingToml); - McpConfigurationHelper.WriteAtomicFile(path, updatedToml); + if (!string.Equals(existingToml, updatedToml, StringComparison.Ordinal)) + { + McpConfigurationHelper.WriteAtomicFile(path, updatedToml); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs` around lines 682 - 690, RemoveCodexTomlRegistration currently always calls McpConfigurationHelper.WriteAtomicFile even when CodexConfigHelper.RemoveCodexServerBlock returned the original content (parse-failure), causing unnecessary rewrites; update RemoveCodexTomlRegistration (use GetConfigPath to read the file, call RemoveCodexServerBlock to get updatedToml) to compare updatedToml with existingToml (ReferenceEquals or string equality) and only call McpConfigurationHelper.WriteAtomicFile(path, updatedToml) when they differ, skipping the write when unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs`:
- Around line 515-521: The branch guarded by ShouldUseTomlForRemoteAuth() must
not call GetUvxPathOrError() because in HTTP remote-auth mode uvx may be absent;
remove that call and instead invoke CodexConfigHelper.BuildCodexServerBlock
without fetching uvx (pass null/empty or use the overload that does not require
uvx) so the TOML snippet for HTTP auth is returned without throwing; update the
code in McpClientConfiguratorBase around the ShouldUseTomlForRemoteAuth() block
to stop calling GetUvxPathOrError() and to call BuildCodexServerBlock in a way
that does not depend on uvx.
- Around line 523-545: The stdio MCP registration string wrongly includes the
explicit "--transport stdio" flag; update the stdio branch in the method that
builds the return string (references: CodexServerName, GetCodexStdioEnvArg(),
QuoteCliArg(), AssetPathUtility.GetUvxCommandParts(),
AssetPathUtility.GetUvxDevFlags(), AssetPathUtility.GetBetaServerFromArgs()) to
remove the " --transport stdio" suffix so the stdio registration uses the form
"codex mcp add {CodexServerName}{envArg} -- {QuoteCliArg(uvxPath)}
{devFlags}{fromArgs} {packageName}" followed by the unregister and list lines.
---
Nitpick comments:
In `@docs/guides/CODEX_HELP.md`:
- Around line 65-75: Add a brief security note after the Remote-hosted auth TOML
example advising users to restrict permissions on their config file (e.g., set
restrictive file permissions like 600) and/or use secure credential management;
reference the config file path (~/.codex/config.toml) and the section/header
name ([mcp_servers.unityMCP] / http_headers) so readers know exactly where the
secret is stored and add a pointer to Codex docs on secure credential management
if available.
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs`:
- Around line 682-690: RemoveCodexTomlRegistration currently always calls
McpConfigurationHelper.WriteAtomicFile even when
CodexConfigHelper.RemoveCodexServerBlock returned the original content
(parse-failure), causing unnecessary rewrites; update
RemoveCodexTomlRegistration (use GetConfigPath to read the file, call
RemoveCodexServerBlock to get updatedToml) to compare updatedToml with
existingToml (ReferenceEquals or string equality) and only call
McpConfigurationHelper.WriteAtomicFile(path, updatedToml) when they differ,
skipping the write when unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04e184aa-3fd4-4897-baf1-210d2de9e36c
📒 Files selected for processing (4)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Helpers/CodexConfigHelper.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.csdocs/guides/CODEX_HELP.md
🚧 Files skipped from review as they are similar to previous changes (1)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs
Author
|
Follow-up pushed in This addresses the remote-auth manual snippet so it no longer depends on I intentionally kept |
nathanandrewturner
approved these changes
May 5, 2026
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.
Description
Codex already had a TOML configurator, but it was not shown as a first-class client in the user docs and the setup flow still presented manual TOML editing as the main path.
This updates Codex setup to use Codex's native MCP CLI for local HTTP and stdio registration, while keeping the existing TOML path for fallback and for remote-hosted auth cases that need
X-API-Key.Type of Change
Changes Made
codex mcp add/codex mcp removefor Codex local HTTP and stdio setup when the Codex CLI is available.~/.codex/config.tomlwrites as a fallback when the CLI is missing.X-API-Key.X-API-Keyheaders during status checks so remote auth config can be refreshed.Testing/Screenshots/Recordings
git diff --checkcodex mcp add unityMCP-test-http --url http://127.0.0.1:8080/mcpcodex mcp get unityMCP-test-http --jsoncodex mcp remove unityMCP-test-httpcodex mcp add unityMCP-test-stdio-order --env "SystemRoot=C:\Windows" -- uvx --from mcpforunityserver mcp-for-unity --transport stdiocodex mcp add unityMCP-test-stdio-transport --env "SystemRoot=C:\Windows" -- uvx --from mcpforunityserver mcp-for-unity --transport stdiocodex mcp get ... --jsoncodex mcp remove ...TestProjects/UnityMCPTestscompleted successfully withScriptAssembliesbuild exit code 0.Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Fixes #933
Additional Notes
The command-line Unity test runner did not emit a test result XML in my local environment, so I am not counting it as a passing test run. The project did compile cleanly in batchmode, and the Codex CLI registration paths were checked directly.
Summary by CodeRabbit
New Features
Documentation
Tests