refactor: use Tommy TOML library directly for config file manipulation

[msanatan]

Before discovering the Tommy library I tried to parse TOML files directly. Some of the methods lingered from that time, which were messing up configuration on Windows.

Now we use Tommy more, it should work. Also consolodiated some code with fresh eyes

Summary by CodeRabbit

• Refactor
  • Improved internal configuration handling for MCP server setup with enhanced validation and more robust processing logic to ensure more reliable configuration management.

[coderabbitai]

Walkthrough

Refactors TOML configuration handling in CodexConfigHelper from manual string assembly to structured TOML table manipulation using the Tommy library. Updates the UpsertCodexServerBlock method signature to accept additional parameters (uvPath and serverSrc), consolidating the previous two-step process into a single call. Removes obsolete string formatting helpers and updates the dependent caller in McpConfigurationHelper.

Changes

Cohort / File(s)	Change Summary
TOML Table-Based Refactor MCPForUnity/Editor/Helpers/CodexConfigHelper.cs	Replaces string-assembly TOML manipulation with structured TOML table approach; introduces CreateUnityMcpTable and TryParseToml helpers; updates BuildCodexServerBlock to construct TOML tables; refactors UpsertCodexServerBlock to accept uvPath and serverSrc parameters and handle root/mcp_servers/unityMCP table hierarchy; removes FormatTomlStringArray and EscapeTomlString utilities; removes obsolete using directives.
Caller Update MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs	Updates call to UpsertCodexServerBlock to use new three-parameter signature (adds uvPath and serverSrc), eliminating the intermediate BuildCodexServerBlock call and local codexBlock variable.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The refactoring involves logic changes to TOML table manipulation and a breaking API change to UpsertCodexServerBlock, requiring careful verification of parsing/serialization correctness and tracing the impact across callers. However, the scope is contained to two files with a clear structural pattern.

Possibly related PRs

• #325: Modifies the same CodexConfigHelper functions (BuildCodexServerBlock and UpsertCodexServerBlock) for Codex TOML server block construction/insertion.
• #288: Updates CodexConfigHelper and related TOML parsing/upsert logic for Codex configuration handling.

Suggested reviewers

• dsarno
• justinpbarnett
• Scriptwonder

Poem

🐰 From strings we weave to tables tall,
TOML structures heed the call,
Tommy's helpers now take the stage,
Refactored code—a cleaner page!
Three params dance where two once stood,
Your config's now understood! 📋✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 37.50% 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 PR title "refactor: use Tommy TOML library directly for config file manipulation" accurately captures the primary change in the changeset. The summary shows that the main work is refactoring TOML manipulation logic from ad-hoc string assembly and line-by-line parsing to a structured TOML table approach using the Tommy library directly. The title is specific and clear—it identifies both the type of change (refactoring) and the core objective (using Tommy TOML library directly). It's concise without being vague, and a teammate scanning the commit history would immediately understand that this PR modernizes configuration file handling through the Tommy library.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (2)

42-50: Good move to structured TOML; align key naming and factor constants.

Using Tommy tables eliminates fragile string ops. To avoid key typos and keep naming consistent with UpsertCodexServerBlock, extract constants for "mcpServers", "mcp_servers", and "unityMCP". Also consider building via UpsertCodexServerBlock(string.Empty, uvPath, serverSrc) to keep logic centralized.

---

125-141: Validate inputs to avoid null TOML values.

If serverSrc (or uvPath) is null/empty, TomlString.Value may serialize unexpectedly or throw under some lib versions. Add guards or fail fast.

Example:

 private static TomlTable CreateUnityMcpTable(string uvPath, string serverSrc)
 {
+    if (string.IsNullOrWhiteSpace(uvPath)) throw new ArgumentException("uvPath is required", nameof(uvPath));
+    if (string.IsNullOrWhiteSpace(serverSrc)) throw new ArgumentException("serverSrc is required", nameof(serverSrc));
     var unityMCP = new TomlTable();
     unityMCP["command"] = new TomlString { Value = uvPath };
     var argsArray = new TomlArray();
     argsArray.Add(new TomlString { Value = "run" });
     argsArray.Add(new TomlString { Value = "--directory" });
     argsArray.Add(new TomlString { Value = serverSrc });
     argsArray.Add(new TomlString { Value = "server.py" });
     unityMCP["args"] = argsArray;
     return unityMCP;
 }

Double‑check that McpConfigFileHelper.ResolveServerDirectory(...) always returns a non‑empty path across platforms. If not guaranteed, we should compute a safe default upstream and avoid throwing here.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dbdaa76 and ac143f7.

📒 Files selected for processing (2)

• MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (1 hunks)
• MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1)

MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (2)

• CodexConfigHelper (14-210)
• UpsertCodexServerBlock (53-72)

🔇 Additional comments (4)

MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (3)

74-97: Robust parser: good coverage of both key styles.

Parsing checks both "mcp_servers" and "mcpServers" and safely handles arrays/singletons for args. LGTM.

---

99-123: Safe TOML parse path.

Gracefully returns null on parse errors without throwing. Solid.

---

53-72: Use canonical mcp_servers key consistently; respect existing keys to avoid config pollution.

Per Codex CLI documentation, the canonical key is mcp_servers (not mcpServers). The current code is correct in using that key, but the risk is real: if a user's config already contains a legacy [mcpServers] section, UpsertCodexServerBlock will leave both keys present, causing config pollution.

The codebase already handles reading from both keys (lines 82–83), which is defensive. However, UpsertCodexServerBlock should either:

• Option 1: Detect and update the existing key (prefer mcp_servers if both exist), as in the suggested diff.
• Option 2: Normalize to the canonical mcp_servers and optionally remove legacy mcpServers entries on update.

Option 1 aligns with the preference-respecting logic in UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (lines 134–145). If adopting that pattern, apply the suggested diff; otherwise, add a comment explaining the normalization strategy.

MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1)

208-208: Code asymmetry present but practical risk is lower; Codex CLI consumes only mcp_servers.

The Codex CLI expects the top-level key mcp_servers (snake_case) in the config file. The current UpsertCodexServerBlock implementation reads from both "mcp_servers" and "mcpServers" (lines 82–83) but only writes to "mcp_servers". While this is asymmetric, there's no evidence the CLI accepts the camelCase variant, so the practical risk of configs being ignored is mitigated. However, if both keys exist in a TOML file, the "mcpServers" entry will not be updated, leaving stale data. Consider adopting the casing-preference logic found in UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (lines 135–145) to detect and preserve the existing key style, avoiding duplication.