-
Notifications
You must be signed in to change notification settings - Fork 532
Enable the rmcp_client feature so it works with Codex CLI
#395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis PR adds a new helper method Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (1)
77-92: LGTM! Proper conditional logic for HTTP mode.The code correctly reads the transport preference and conditionally sets the
rmcp_clientfeature flag when HTTP transport is enabled. The ordering is appropriate since the feature flag operates at the root level.Minor consideration: When a user switches from HTTP to stdio mode, the
features.rmcp_clientflag will remain in the TOML. This is likely harmless (stdio mode would ignore it), but consider whether it should be explicitly removed for cleaner configuration files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs(4 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (2)
TomlTable(143-164)TomlTable(170-211)MCPForUnity/Editor/External/Tommy.cs (1)
TomlBoolean(242-252)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (1)
MCPForUnity/Editor/External/Tommy.cs (1)
TomlBoolean(242-252)
🔇 Additional comments (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
465-472: LGTM! Thorough validation of the rmcp_client feature flag.The test properly validates that the
features.rmcp_clientflag is present at the root level, is a boolean type, and is set to true for HTTP transport mode. The assertions follow a clear pattern: existence check → type check → value check.
547-553: LGTM! Consistent validation for the upsert scenario.The test correctly validates that
features.rmcp_clientis properly set when upserting an existing TOML configuration in HTTP mode. The assertions mirror the build scenario test, ensuring consistent behavior across both code paths.MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (2)
32-34: LGTM! Clean integration of the feature flag.The call to
EnsureRmcpClientFeatureis appropriately placed within the HTTP transport branch, and the comment clearly explains why the flag is needed for Codex's Rust MCP client.
213-227: I'm unable to clone the repository to verify the claims, but I can analyze the review comment based on the information provided.Looking at the review comment:
- What was verified: The code implementation itself shows proper null safety and correct TOML table handling
- What was requested for verification: The reviewer asked to confirm that
features.rmcp_clientshould exist at the root level (not nested withinmcp_servers.unityMCP)- What the code shows: The implementation clearly places the
featurestable at the root level of the TOML config:root["features"] = features;andfeatures["rmcp_client"] = new TomlBoolean { Value = true };The code implementation is consistent with the reviewer's assumption that
features.rmcp_clientbelongs at the root level. Since:
- The implementation has been approved as well-written
- The code structure matches the reviewer's stated expectation
- The reviewer's main verification request cannot be completed without access to external Codex CLI documentation (which is typically proprietary or external)
- No actual issues were identified with the code
Implementation is correct with proper null safety and root-level features table placement.
The
EnsureRmcpClientFeaturemethod correctly creates or retrieves thefeaturestable at the root level and sets thermcp_clientboolean flag. The null check is appropriate and the TomlTable/TomlBoolean usage is safe.
Interesting nitpick, but at least in my testing, it doesn't impact stdio mode |
* 'main' of https://github.com/CoplayDev/unity-mcp: Harden PlayMode test runs (CoplayDev#396) Enable the `rmcp_client` feature so it works with Codex CLI (CoplayDev#395) chore: bump version to 8.0.0 HTTP Server, uvx, C# only custom tools (CoplayDev#375) [CUSTOM TOOLS] Roslyn Runtime Compilation Feature (CoplayDev#371)
Awesome fix @dsarno
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.