Skip to content

Conversation

@msanatan
Copy link
Contributor

@msanatan msanatan commented Nov 25, 2025

Awesome fix @dsarno

Summary by CodeRabbit

  • New Features

    • Configuration system now automatically enables protocol support settings when HTTP transport is selected, ensuring proper setup for HTTP/SSE-based communications.
  • Tests

    • Test suite enhanced to verify protocol support configuration is correctly applied and enabled in HTTP transport scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@msanatan msanatan requested a review from dsarno November 25, 2025 19:41
@msanatan msanatan self-assigned this Nov 25, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 25, 2025

Walkthrough

This PR adds a new helper method EnsureRmcpClientFeature to the CodexConfigHelper class and integrates it into the HTTP transport code paths. When HTTP transport is selected in BuildCodexServerBlock and UpsertCodexServerBlock, the helper method ensures the TOML configuration includes a features.rmcp_client flag set to true. Corresponding tests validate this new behavior.

Changes

Cohort / File(s) Summary
HTTP Transport RMCP Client Flag Implementation
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs
Added EnsureRmcpClientFeature(TomlTable root) private helper method to set features.rmcp_client to true. Modified BuildCodexServerBlock and UpsertCodexServerBlock to invoke this method when HTTP transport is selected.
HTTP Transport Test Validation
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs
Enhanced BuildCodexServerBlock_HttpMode_GeneratesUrlField and UpsertCodexServerBlock_HttpMode_GeneratesUrlField tests to verify that features.rmcp_client is present as a boolean and set to true.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • The new EnsureRmcpClientFeature method is straightforward and self-contained
  • Integration points are limited to two well-defined locations in existing methods
  • Test additions follow existing patterns and verify straightforward assertions
  • No complex logic or unexpected control flow changes

Possibly related PRs

Poem

🐰 A flag hops into place,
rmcp_client, bold and true,
HTTP transport takes its race,
Features shine with toml glue,
Config flows smooth as morning dew!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: enabling the rmcp_client feature for Codex CLI compatibility, which aligns with the code changes that add EnsureRmcpClientFeature method and invoke it for HTTP transport.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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_client feature 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_client flag 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

📥 Commits

Reviewing files that changed from the base of the PR and between dbe5f33 and 3cf516b.

📒 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_client flag 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_client is 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 EnsureRmcpClientFeature is 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:

  1. What was verified: The code implementation itself shows proper null safety and correct TOML table handling
  2. What was requested for verification: The reviewer asked to confirm that features.rmcp_client should exist at the root level (not nested within mcp_servers.unityMCP)
  3. What the code shows: The implementation clearly places the features table at the root level of the TOML config: root["features"] = features; and features["rmcp_client"] = new TomlBoolean { Value = true };

The code implementation is consistent with the reviewer's assumption that features.rmcp_client belongs 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 EnsureRmcpClientFeature method correctly creates or retrieves the features table at the root level and sets the rmcp_client boolean flag. The null check is appropriate and the TomlTable/TomlBoolean usage is safe.

@msanatan
Copy link
Contributor Author

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_client feature 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_client flag 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

Interesting nitpick, but at least in my testing, it doesn't impact stdio mode

@msanatan msanatan merged commit be6c387 into CoplayDev:main Nov 25, 2025
1 check passed
@msanatan msanatan deleted the fix-codex-cli branch November 25, 2025 21:08
dsarno added a commit to dsarno/unity-mcp that referenced this pull request Nov 26, 2025
* '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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant