Update to support Codex CLI #325
Conversation
Fix the JSON installation on Codex in Wizard; Adding the lines to support Codex CLI on windows temporarily, thanks to openai/codex#4180
WalkthroughConfiguration for the Codex MCP server is now generated as TOML with Windows-specific environment paths exposed via a dedicated block. The setup flow routes Codex clients to a specialized configuration handler, while the TOML builder supports both camelCase and snake_case section naming variants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The TOML building logic introduces case normalization and environment variable gathering requiring careful verification, while the setup routing is straightforward. Mixed complexity across two files with one containing denser logic. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (3)
90-91: Consider validating hardcoded PowerShell paths before adding to config.The hardcoded paths for PowerShell and pwsh may not exist on all Windows systems (e.g., older Windows versions without PowerShell 7, or systems with different installation paths). Writing non-existent paths to the configuration could cause issues.
Consider adding existence checks:
string powershell = Path.Combine(Environment.SystemDirectory ?? (systemRoot + "\\System32"), "WindowsPowerShell\\v1.0\\powershell.exe"); string pwsh = Path.Combine(programFiles, "PowerShell\\7\\pwsh.exe"); + + // Only include paths that actually exist + bool powershellExists = File.Exists(powershell); + bool pwshExists = File.Exists(pwsh);Then conditionally add these to the TOML output:
sb.AppendLine($"LOCALAPPDATA = \"{EscapeTomlString(localAppData)}\""); - sb.AppendLine($"POWERSHELL = \"{EscapeTomlString(powershell)}\""); + if (powershellExists) + sb.AppendLine($"POWERSHELL = \"{EscapeTomlString(powershell)}\""); sb.AppendLine($"PROGRAMDATA = \"{EscapeTomlString(programData)}\""); sb.AppendLine($"PROGRAMFILES = \"{EscapeTomlString(programFiles)}\""); - sb.AppendLine($"PWSH = \"{EscapeTomlString(pwsh)}\""); + if (pwshExists) + sb.AppendLine($"PWSH = \"{EscapeTomlString(pwsh)}\"");
114-114: Add logging to catch block for debugging.The empty catch block with only a comment makes it impossible to diagnose failures in the Windows environment block construction.
Add minimal logging:
} - catch { /* best effort */ } + catch (Exception ex) + { + // Best effort - log but don't fail + UnityEngine.Debug.LogWarning($"UnityMCP: Failed to build Windows environment block: {ex.Message}"); + }
74-88: Clarify HOMEPATH derivation logic.The logic for deriving
HOMEPATHfromUSERPROFILEis correct but confusing. Line 84 usesroot.Length - 1to "keep leading backslash," but the calculation is not immediately obvious.Consider adding a comment to clarify the substring logic:
try { + // For "C:\\Users\\name" with root "C:\\", skip 2 chars to get "\\Users\\name" var root = Path.GetPathRoot(userProfile) ?? string.Empty; // e.g., C:\\ - homePath = userProfile.Substring(root.Length - 1); // keep leading backslash + homePath = userProfile.Substring(root.Length - 1); } catch { homePath = "\\"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs(3 hunks)UnityMcpBridge/Editor/Setup/SetupWizardWindow.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Setup/SetupWizardWindow.cs (1)
UnityMcpBridge/Editor/Helpers/McpConfigurationHelper.cs (3)
McpConfigurationHelper(19-296)ConfigureCodexClient(147-221)WriteMcpConfiguration(27-142)
🔇 Additional comments (6)
UnityMcpBridge/Editor/Setup/SetupWizardWindow.cs (2)
629-637: LGTM! Clean routing logic for Codex configuration.The implementation correctly routes Codex clients to the specialized TOML configuration handler while maintaining the JSON-based configuration for other clients. The null check on the client parameter ensures safe access to
mcpType.
653-655: Consistent configuration routing in manual setup flow.The routing logic mirrors
PerformClientConfiguration, ensuring consistent behavior between automated and manual configuration paths.UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (4)
46-52: LGTM! StringBuilder refactor and timeout configuration.The refactor to
StringBuilderimproves maintainability, and the addition ofstartup_timeout_sec = 30provides a reasonable timeout for the Codex CLI startup process.
133-145: Well-designed casing detection and preference logic.The implementation correctly detects existing casing conventions and preserves them when upserting the Codex server block. The fallback to checking for a parent
[mcpServers]section (line 139) is a reasonable heuristic for inferring preferred casing when the unityMCP section doesn't exist yet.
147-166: Clean helper functions for TOML section handling.The helper functions provide clear abstractions:
IsSectioncorrectly identifies section headers while excluding array-of-tables syntaxSectionNameextracts the section name safelyTargetOrChildhandles case-insensitive matching for both camelCase and snake_case variants
168-209: Robust section replacement logic.The parsing and replacement logic correctly:
- Identifies target sections and their nested tables (e.g.,
.env)- Replaces the entire target region with the new block
- Preserves the chosen casing convention
- Appends the block if no existing section is found
1 participant
Fix the JSON installation on Codex in Wizard;
Adding the lines to support Codex CLI on windows temporarily, thanks to openai/codex#4180
Summary by CodeRabbit