Add Codex to autoconfig options #288
Conversation
WalkthroughAdds Codex support and related plumbing: new Codex client entry and enum values, Codex TOML helpers, a TOML parser, centralized config/server resolution and atomic file-write utilities, editor window integration and tests, plus accompanying Unity .meta files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Editor as MCPForUnityEditorWindow
participant Codex as CodexConfigHelper
participant Cfg as McpConfigFileHelper
participant FS as Filesystem
User->>Editor: Open / Trigger setup
Editor->>Codex: IsCodexConfigured(pythonDir)
alt codex configured
Codex-->>Editor: true
Editor-->>User: skip configuration
else not configured
Codex-->>Editor: false
Editor->>Cfg: ResolveServerDirectory(pythonDir, existingArgs)
Cfg-->>Editor: serverSrc
Editor->>Codex: BuildCodexServerBlock(uvPath, serverSrc)
Codex-->>Editor: tomlBlock
Editor->>Codex: UpsertCodexServerBlock(existingToml, tomlBlock)
Codex-->>Editor: updatedToml
Editor->>Cfg: WriteAtomicFile(configPath, updatedToml)
Cfg->>FS: tmp/write/backup/replace
FS-->>Cfg: OK / Error
Cfg-->>Editor: OK / throw
Editor-->>User: configured / failed
end
sequenceDiagram
autonumber
participant Editor
participant Cfg as McpConfigFileHelper
participant FS as Filesystem
Editor->>Cfg: WriteAtomicFile(path, contents)
note right of Cfg: create temp → write → backup original → replace → cleanup
Cfg->>FS: perform filesystem ops
alt success
FS-->>Cfg: OK
Cfg-->>Editor: OK
else failure
FS-->>Cfg: Error
Cfg-->>Editor: throw (backup preserved)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (3)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
UnityMcpBridge/Editor/Windows/ManualConfigEditorWindow.cs (1)
234-240: Align copy button label with TOML instructionsOn Line 236 we tell Codex users to paste TOML, yet the button still says “Copy JSON”. Update the label so the UI stays consistent.
EditorGUILayout.BeginHorizontal(); GUILayout.FlexibleSpace(); + string copyButtonLabel = mcpClient?.mcpType == McpTypes.Codex ? "Copy TOML" : "Copy JSON"; if ( GUILayout.Button( - "Copy JSON", + copyButtonLabel, copyButtonStyle, GUILayout.Height(25), GUILayout.Width(100) ) )UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
1716-1776: Critical: Don’t write JSON into Codex TOML on auto-rewriteWhen Codex config exists but points to a different directory, the code calls
WriteToConfig(...), which writes JSON. For Codex, you must update TOML viaConfigureCodexClient(...).Apply:
- string rewriteResult = WriteToConfig(pythonDir, configPath, mcpClient); + string rewriteResult = (mcpClient.mcpType == McpTypes.Codex) + ? ConfigureCodexClient(pythonDir, configPath, mcpClient) + : WriteToConfig(pythonDir, configPath, mcpClient);
🧹 Nitpick comments (8)
UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs (4)
16-27: Handle--directory=PATHform as wellCurrently only
--directory <PATH>is supported. Add support for the--directory=<PATH>style to be robust across clients.Apply:
- public static string ExtractDirectoryArg(string[] args) + public static string ExtractDirectoryArg(string[] args) { if (args == null) return null; - for (int i = 0; i < args.Length - 1; i++) + for (int i = 0; i < args.Length; i++) { - if (string.Equals(args[i], "--directory", StringComparison.OrdinalIgnoreCase)) - { - return args[i + 1]; - } + var cur = args[i] ?? string.Empty; + if (string.Equals(cur, "--directory", StringComparison.OrdinalIgnoreCase) && i < args.Length - 1) + return args[i + 1]; + if (cur.StartsWith("--directory=", StringComparison.OrdinalIgnoreCase)) + return cur.Substring("--directory=".Length); } return null; }
29-46: Normalize trailing separators for stable path equalityTrailing slashes/backslashes can cause false negatives. Trim them before comparing.
- string na = Path.GetFullPath(a.Trim()); - string nb = Path.GetFullPath(b.Trim()); + string na = Path.GetFullPath(a.Trim()) + .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + string nb = Path.GetFullPath(b.Trim()) + .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
89-97: Avoid Unity PackageCache path on all OS, not just WindowsThe PackageCache path is ephemeral on macOS/Linux too. Generalize the check.
- if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) - && !string.IsNullOrEmpty(serverSrc) - && serverSrc.IndexOf(@"\Library\PackageCache\", StringComparison.OrdinalIgnoreCase) >= 0 - && !EditorPrefs.GetBool("MCPForUnity.UseEmbeddedServer", false)) - { - serverSrc = ServerInstaller.GetServerPath(); - } + string normSrc = (serverSrc ?? string.Empty).Replace('/', '\\'); + if (!string.IsNullOrEmpty(normSrc) + && normSrc.IndexOf(@"\Library\PackageCache\", StringComparison.OrdinalIgnoreCase) >= 0 + && !EditorPrefs.GetBool("MCPForUnity.UseEmbeddedServer", false)) + { + serverSrc = ServerInstaller.GetServerPath(); + }
100-107: Create parent directory before atomic writePrevents failures when the config directory doesn’t exist.
public static void WriteAtomicFile(string path, string contents) { string tmp = path + ".tmp"; string backup = path + ".backup"; bool writeDone = false; try { + var dir = Path.GetDirectoryName(path); + if (!string.IsNullOrEmpty(dir)) Directory.CreateDirectory(dir); File.WriteAllText(tmp, contents, new UTF8Encoding(false));UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (3)
1152-1198: Remove duplicate, unused WriteAtomicFile (use shared helper instead)This private copy is now redundant; the class already uses
McpConfigFileHelper.WriteAtomicFile. Remove to avoid drift.- private static void WriteAtomicFile(string path, string contents) - { - string tmp = path + ".tmp"; - string backup = path + ".backup"; - bool writeDone = false; - try - { - File.WriteAllText(tmp, contents, new UTF8Encoding(false)); - try - { - File.Replace(tmp, path, backup); - writeDone = true; - } - catch (FileNotFoundException) - { - File.Move(tmp, path); - writeDone = true; - } - catch (PlatformNotSupportedException) - { - if (File.Exists(path)) - { - try { if (File.Exists(backup)) File.Delete(backup); } catch { } - File.Move(path, backup); - } - File.Move(tmp, path); - writeDone = true; - } - } - catch (Exception ex) - { - try - { - if (!writeDone && File.Exists(backup)) - { - try { File.Copy(backup, path, true); } catch { } - } - } - catch { } - throw new Exception($"Failed to write config file '{path}': {ex.Message}", ex); - } - finally - { - try { if (File.Exists(tmp)) File.Delete(tmp); } catch { } - try { if (writeDone && File.Exists(backup)) File.Delete(backup); } catch { } - } - } +
706-733: Simplify Cursor config path resolutionBoth branches build the same path. Collapse into one assignment.
- try - { - string configPath = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) - ? Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - ".cursor", "mcp.json") - : Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - ".cursor", "mcp.json"); + try + { + string configPath = Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), + ".cursor", "mcp.json");
1324-1339: Improve Codex manual block with existing args (if present)Reuse existing Codex args when building the manual TOML so the suggested directory matches current config.
- string manualConfig = mcpClient?.mcpType == McpTypes.Codex - ? CodexConfigHelper.BuildCodexServerBlock(uvPathForManual, McpConfigFileHelper.ResolveServerDirectory(pythonDir, null)).TrimEnd() + Environment.NewLine - : ConfigJsonBuilder.BuildManualConfigJson(uvPathForManual, pythonDir, mcpClient); + string manualConfig; + if (mcpClient?.mcpType == McpTypes.Codex) + { + string[] existingArgs = null; + try + { + if (File.Exists(configPath)) + { + var toml = File.ReadAllText(configPath); + if (CodexConfigHelper.TryParseCodexServer(toml, out _, out var parsed)) + existingArgs = parsed; + } + } + catch { } + string serverDir = McpConfigFileHelper.ResolveServerDirectory(pythonDir, existingArgs); + manualConfig = CodexConfigHelper.BuildCodexServerBlock(uvPathForManual, serverDir).TrimEnd() + Environment.NewLine; + } + else + { + manualConfig = ConfigJsonBuilder.BuildManualConfigJson(uvPathForManual, pythonDir, mcpClient); + }UnityMcpBridge/Editor/Data/McpClients.cs (1)
162-183: Clarify Codex listing numberThe comment label jumps back to “// 4)” even though earlier entries already use that number. Consider renumbering this and the nearby “// 3) Kiro” comment (or drop the numbering altogether) so the list stays easy to follow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
UnityMcpBridge/Editor/Data/McpClients.cs(1 hunks)UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs(1 hunks)UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs.meta(1 hunks)UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs(1 hunks)UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs.meta(1 hunks)UnityMcpBridge/Editor/Models/McpTypes.cs(1 hunks)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(15 hunks)UnityMcpBridge/Editor/Windows/ManualConfigEditorWindow.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs.meta
🧬 Code graph analysis (3)
UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs (4)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs (1)
Exists(78-94)UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (3)
ServerInstaller(12-743)GetServerPath(112-115)EnsureServerInstalled(22-110)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
WriteAtomicFile(1152-1198)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
ServerPathResolver(8-148)TryFindEmbeddedServerSource(15-100)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (2)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (5)
CodexConfigHelper(14-274)IsCodexConfigured(16-38)BuildCodexServerBlock(40-46)TryParseCodexServer(102-146)UpsertCodexServerBlock(48-100)UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs (6)
McpConfigFileHelper(14-185)ExtractDirectoryArg(16-27)PathsEqual(29-46)WriteAtomicFile(100-150)ResolveServerDirectory(52-98)ResolveServerSource(152-184)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (1)
UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs (3)
McpConfigFileHelper(14-185)ExtractDirectoryArg(16-27)PathsEqual(29-46)
🔇 Additional comments (4)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (2)
571-576: LGTM: Per‑client “already configured” checks now include CodexGood use of
CodexConfigHelper.IsCodexConfigured(pythonDir)alongside existing checks. This avoids redundant rewrites.Also applies to: 589-593, 669-674, 687-691
1276-1301: LGTM: Centralized resolution and atomic writesUsing
McpConfigFileHelper.ResolveServerDirectory(...)andMcpConfigFileHelper.WriteAtomicFile(...)improves consistency and safety.Also applies to: 1556-1575
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs.meta (1)
1-11: Meta asset looks goodStandard Unity .meta stub with default settings—no issues spotted.
UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs.meta (1)
1-11: Meta asset looks goodMatches the usual Unity defaults; nothing to change.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs.meta(1 hunks)UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers.meta
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs.meta
🧰 Additional context used
🧬 Code graph analysis (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (2)
CodexConfigHelper(15-372)TryParseCodexServer(103-148)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (1)
UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs (3)
McpConfigFileHelper(14-185)ExtractDirectoryArg(16-27)PathsEqual(29-46)
🔇 Additional comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
8-67: Solid coverage for Codex TOML parsing scenarios.Great to see the parser exercised across single-line, multi-line, and comment-heavy cases; this gives confidence the helper can tolerate the real Codex configs we’ve seen in the wild.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
47-67: Add a test for single-quoted arrays.
ParseTomlStringArrayinCodexConfigHelpercontains bespoke fallback logic to handle TOML arrays written with single quotes. None of the current tests exercise that branch, so a regression there would slip through. Please add a fifth test (e.g.,args = ['run', '--directory', '/abs/path', 'server.py']) to make sure the single-quote path stays green. Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs(1 hunks)UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (2)
CodexConfigHelper(15-373)TryParseCodexServer(103-149)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (1)
UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs (3)
McpConfigFileHelper(14-185)ExtractDirectoryArg(16-27)PathsEqual(29-46)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (1)
UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs (3)
McpConfigFileHelper(14-185)ExtractDirectoryArg(16-27)PathsEqual(29-46)
It's a single file and OSS, easy to integrate into Unity
… inside literal strings are treated as embedded apostrophes instead of prematurely ending the value
Seeing random JSON in my codex config was pretty annoying
|
Recent changes still work well 
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
706-733: Avoid invalid cast when reading Cursor args; use JArray.ToObject<string[]>().Casting dynamic JArray to IEnumerable can throw; prefer strongly-typed conversion.
Apply this diff:
- var args = unity.args; + var args = unity.args; if (args == null) return false; - // Prefer exact extraction of the --directory value and compare normalized paths - string[] strArgs = ((System.Collections.Generic.IEnumerable<object>)args) - .Select(x => x?.ToString() ?? string.Empty) - .ToArray(); - string dir = McpConfigFileHelper.ExtractDirectoryArg(strArgs); + // Prefer exact extraction of the --directory value and compare normalized paths + var jArr = args as JArray; + string[] strArgs = jArr?.ToObject<string[]>(); + if (strArgs == null) return false; + string dir = McpConfigFileHelper.ExtractDirectoryArg(strArgs); if (string.IsNullOrEmpty(dir)) return false; return McpConfigFileHelper.PathsEqual(dir, pythonDir);🧹 Nitpick comments (11)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs (1)
49-60: Preserve the existing AutoRegister preference during tests.Hard-setting the key to
falseand deleting it afterward wipes whatever value the developer had configured (and can flip it back to the default). Cache the existing value in SetUp and restore it in TearDown instead of unconditionally deleting the key.@@ - private string _serverSrcDir; + private string _serverSrcDir; + private bool? _previousAutoRegisterEnabled; @@ - // Disable auto-registration to avoid hitting user configs during tests - EditorPrefs.SetBool("MCPForUnity.AutoRegisterEnabled", false); + const string autoRegisterKey = "MCPForUnity.AutoRegisterEnabled"; + _previousAutoRegisterEnabled = EditorPrefs.HasKey(autoRegisterKey) + ? EditorPrefs.GetBool(autoRegisterKey) + : (bool?)null; + EditorPrefs.SetBool(autoRegisterKey, false); @@ - EditorPrefs.DeleteKey("MCPForUnity.AutoRegisterEnabled"); + const string autoRegisterKey = "MCPForUnity.AutoRegisterEnabled"; + if (_previousAutoRegisterEnabled.HasValue) + { + EditorPrefs.SetBool(autoRegisterKey, _previousAutoRegisterEnabled.Value); + } + else + { + EditorPrefs.DeleteKey(autoRegisterKey); + }UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (3)
589-593: Make “Connected ✓” reflect any configured client, not just Cursor/Codex/Claude.Using specific helpers can show false states. Prefer the client statuses already computed.
Apply this diff:
- lastClientRegisteredOk = anyRegistered - || IsCursorConfigured(pythonDir) - || CodexConfigHelper.IsCodexConfigured(pythonDir) - || IsClaudeConfigured(); + lastClientRegisteredOk = anyRegistered + || mcpClients.clients.Any(c => c.status == McpStatus.Configured);
687-691: Unify success flag computation with client statuses.Avoid hard‑coding specific clients; trust statuses post‑configuration.
Apply this diff:
- lastClientRegisteredOk = anyRegistered - || IsCursorConfigured(pythonDir) - || CodexConfigHelper.IsCodexConfigured(pythonDir) - || IsClaudeConfigured(); + lastClientRegisteredOk = anyRegistered + || mcpClients.clients.Any(c => c.status == McpStatus.Configured);
1512-1585: Lock preference name is Cursor‑specific; broaden or support both keys.This gate is now used for Codex too. Consider a generic key while honoring the old one for back‑compat.
Apply this diff:
- try { if (EditorPrefs.GetBool("MCPForUnity.LockCursorConfig", false)) return "Skipped (locked)"; } catch { } + try + { + bool locked = EditorPrefs.GetBool("MCPForUnity.LockMcpConfig", false) + || EditorPrefs.GetBool("MCPForUnity.LockCursorConfig", false); // back-compat + if (locked) return "Skipped (locked)"; + } + catch { }UnityMcpBridge/Editor/External/Tommy.cs (7)
492-493: Unity/C# compatibility: replace ‘or’ type patterns (C# 9) with C# 8-safe logicLogical patterns (or) in type patterns require C# 9.
Apply this diff:
- var hasRealValues = !collapsedItems.All(n => n.Value is TomlTable {IsInline: false} or TomlArray {IsTableArray: true}); + var hasRealValues = !collapsedItems.All(n => + { + var v = n.Value; + return (v is TomlTable t && t.IsInline == false) || (v is TomlArray a && a.IsTableArray); + });
514-520: Unity/C# compatibility: replace ‘or’ pattern in conditionSame C# 9 logical pattern. Use classic boolean expressions.
Apply this diff:
- if (collapsedItem.Value is TomlArray {IsTableArray: true} or TomlTable {IsInline: false}) + if ((collapsedItem.Value is TomlArray a && a.IsTableArray) || + (collapsedItem.Value is TomlTable t && t.IsInline == false)) {
794-801: Unity/C# compatibility: char ‘or’ patternReplace with classic || to avoid C# 9 requirement.
- if (c is TomlSyntax.COMMENT_SYMBOL or TomlSyntax.NEWLINE_CHARACTER) + if (c == TomlSyntax.COMMENT_SYMBOL || c == TomlSyntax.NEWLINE_CHARACTER) {
1937-1949: Unity/C# compatibility: replace C# 9 relational/logical patterns in helpersThese helpers use C# 9 pattern syntax (is/or/and). Convert to C# 8-friendly expressions.
Apply this diff:
- public static bool IsQuoted(char c) => c is BASIC_STRING_SYMBOL or LITERAL_STRING_SYMBOL; + public static bool IsQuoted(char c) => c == BASIC_STRING_SYMBOL || c == LITERAL_STRING_SYMBOL; - public static bool IsWhiteSpace(char c) => c is ' ' or '\t'; + public static bool IsWhiteSpace(char c) => c == ' ' || c == '\t'; - public static bool IsNewLine(char c) => c is NEWLINE_CHARACTER or NEWLINE_CARRIAGE_RETURN_CHARACTER; + public static bool IsNewLine(char c) => c == NEWLINE_CHARACTER || c == NEWLINE_CARRIAGE_RETURN_CHARACTER; - public static bool IsBareKey(char c) => - c is >= 'A' and <= 'Z' or >= 'a' and <= 'z' or >= '0' and <= '9' or '_' or '-'; + public static bool IsBareKey(char c) => + (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || c == '_' || c == '-'; - public static bool IsValueSeparator(char c) => - c is ITEM_SEPARATOR or ARRAY_END_SYMBOL or INLINE_TABLE_END_SYMBOL; + public static bool IsValueSeparator(char c) => + c == ITEM_SEPARATOR || c == ARRAY_END_SYMBOL || c == INLINE_TABLE_END_SYMBOL;Also applies to: 1958-1960
616-618: TOMLParser.ForceASCII appears unused (redundant with static TOML.ForceASCII)Parser.ForceASCII is set but never read; escape logic reads TOML.ForceASCII. Either wire the instance setting into escape paths or remove the parser‑level property to avoid confusion.
Also applies to: 1752-1756
421-422: Avoid exception on Delete when node not foundRawTable.First(...) will throw if the node isn’t present. Consider a safe remove.
Example:
- public override void Delete(TomlNode node) => RawTable.Remove(RawTable.First(kv => kv.Value == node).Key); + public override void Delete(TomlNode node) + { + var kv = RawTable.FirstOrDefault(kv => kv.Value == node); + if (!EqualityComparer<string>.Default.Equals(kv.Key, null)) + RawTable.Remove(kv.Key); + }
350-383: Array‑of‑tables comment emission repeats header comment for each tableThis writes the same Comment before every [[name]] section. Confirm if that’s intended; if not, emit once or prefer per‑table comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/WriteToConfigTests.cs(2 hunks)UnityMcpBridge/Editor/External.meta(1 hunks)UnityMcpBridge/Editor/External/Tommy.cs(1 hunks)UnityMcpBridge/Editor/External/Tommy.cs.meta(1 hunks)UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs(1 hunks)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(16 hunks)✅ Files skipped from review due to trivial changes (2)
- UnityMcpBridge/Editor/External/Tommy.cs.meta
- UnityMcpBridge/Editor/External.meta
🚧 Files skipped from review as they are similar to previous changes (2)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs
- UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (4)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (5)
CodexConfigHelper(17-242)IsCodexConfigured(19-41)BuildCodexServerBlock(43-49)TryParseCodexServer(105-145)UpsertCodexServerBlock(51-103)UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs (6)
McpConfigFileHelper(14-185)ExtractDirectoryArg(16-27)PathsEqual(29-46)WriteAtomicFile(100-150)ResolveServerDirectory(52-98)ResolveServerSource(152-184)UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (2)
ConfigJsonBuilder(7-128)BuildManualConfigJson(9-29)UnityMcpBridge/Editor/Windows/ManualConfigEditorWindow.cs (2)
ManualConfigEditorWindow(9-295)ShowWindow(19-27)🔇 Additional comments (7)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (7)
1276-1277: LGTM: centralizing server directory resolution.Using McpConfigFileHelper.ResolveServerDirectory avoids drift and improves cross‑platform behavior.
1301-1301: LGTM: atomic write via shared helper.Good move to McpConfigFileHelper.WriteAtomicFile.
1324-1339: LGTM: manual config flow handles Codex TOML vs JSON.Correctly generates TOML block for Codex and JSON for others.
1341-1396: LGTM: robust server source resolution.Combines installed/dev/embedded resolution with macOS path normalization and debug hints.
1427-1511: LGTM: configure per‑OS path, Codex path, and status update.Clear split between Codex TOML and JSON clients with proper status handling.
1716-1753: LGTM: client‑specific config parsing, including VSCode and Codex.Properly handles VSCode’s top‑level servers and Codex TOML via CodexConfigHelper.
1758-1772: LGTM: unified path extraction/compare and auto‑rewrite.Using McpConfigFileHelper.ExtractDirectoryArg/PathsEqual keeps checks consistent; auto‑rewrite path correctly switches to Codex when needed.
| using System; | ||
| using System.Collections; | ||
| using System.Collections.Generic; | ||
| using System.Globalization; | ||
| using System.IO; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Text.RegularExpressions; | ||
|
|
||
| namespace MCPForUnity.External.Tommy |
There was a problem hiding this comment.
🧩 Analysis chain
Third‑party code attribution/license
This file appears to embed a TOML parser (“Tommy”). Ensure the repository includes appropriate third‑party attribution and license files per the original project’s terms.
Run to locate licenses/attributions:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Top-level licenses:"
fd -HI --max-depth 2 'LICENSE*' | xargs -I{} sh -c 'echo "== {} =="; head -n 20 "{}"; echo'
echo "Mentions of Tommy/TOML parser:"
rg -n -C2 -i 'tommy|TOML parser|license|third[- ]party|attribution' || trueLength of output: 6022
Add Tommy parser license and attribution
This file embeds the Tommy TOML parser but its original license and copyright notice aren’t included.
Add Tommy’s license (e.g. include its LICENSE file or header notice in UnityMcpBridge/Editor/External/Tommy.cs) and update your top-level LICENSE accordingly.
| public override string ToInlineToml() => | ||
| IntegerBase != Base.Decimal | ||
| ? $"0{TomlSyntax.BaseIdentifiers[(int) IntegerBase]}{Convert.ToString(Value, (int) IntegerBase)}" | ||
| : Value.ToString(CultureInfo.InvariantCulture); | ||
| } |
There was a problem hiding this comment.
Correct sign placement for non‑decimal integers (TOML requires sign before 0x/0o/0b)
Negative non-decimal integers currently emit as 0x-a which is invalid. The sign must precede the base prefix, e.g., -0xA.
Apply this diff:
- public override string ToInlineToml() =>
- IntegerBase != Base.Decimal
- ? $"0{TomlSyntax.BaseIdentifiers[(int) IntegerBase]}{Convert.ToString(Value, (int) IntegerBase)}"
- : Value.ToString(CultureInfo.InvariantCulture);
+ public override string ToInlineToml()
+ {
+ if (IntegerBase == Base.Decimal)
+ return Value.ToString(CultureInfo.InvariantCulture);
+
+ var raw = Convert.ToString(Value, (int) IntegerBase);
+ var sign = raw.StartsWith("-", StringComparison.Ordinal) ? "-" : "";
+ var digits = sign.Length == 1 ? raw.Substring(1) : raw;
+ return $"{sign}0{TomlSyntax.BaseIdentifiers[(int) IntegerBase]}{digits}";
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public override string ToInlineToml() => | |
| IntegerBase != Base.Decimal | |
| ? $"0{TomlSyntax.BaseIdentifiers[(int) IntegerBase]}{Convert.ToString(Value, (int) IntegerBase)}" | |
| : Value.ToString(CultureInfo.InvariantCulture); | |
| } | |
| public override string ToInlineToml() | |
| { | |
| if (IntegerBase == Base.Decimal) | |
| return Value.ToString(CultureInfo.InvariantCulture); | |
| var raw = Convert.ToString(Value, (int) IntegerBase); | |
| var sign = raw.StartsWith("-", StringComparison.Ordinal) ? "-" : ""; | |
| var digits = sign.Length == 1 ? raw.Substring(1) : raw; | |
| return $"{sign}0{TomlSyntax.BaseIdentifiers[(int) IntegerBase]}{digits}"; | |
| } |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/External/Tommy.cs around lines 185 to 189, the current
ToInlineToml emits the sign after the base prefix (e.g., 0x-a) which is invalid
TOML; change the logic so if Value is negative you capture a leading "-" and use
the absolute value for Convert.ToString, then return sign + "0" +
TomlSyntax.BaseIdentifiers[(int)IntegerBase] + convertedAbsoluteValue; keep the
existing decimal branch unchanged.
There was a problem hiding this comment.
Ignoring, we have tests to confirm the TOML parsing and saving works
| // Consume the quote character and read the key name | ||
| col++; | ||
| buffer.Append(ReadQuotedValueSingleLine((char) reader.Read())); | ||
| quoted = true; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Fix quoted-key parsing: wrong argument passed to ReadQuotedValueSingleLine
The code consumes the opening quote but passes the next character as the quote delimiter. This breaks parsing of quoted keys. Use the actual quote char for the delimiter and pass the first payload char as initial data.
Apply this diff:
- // Consume the quote character and read the key name
- col++;
- buffer.Append(ReadQuotedValueSingleLine((char) reader.Read()));
- quoted = true;
- continue;
+ // Consume the quote character and read the key name
+ ConsumeChar();
+ var initial = (char) reader.Read();
+ var key = ReadQuotedValueSingleLine(c, initial);
+ if (key is null) return AddError("Invalid quoted key.");
+ buffer.Append(key);
+ quoted = true;
+ continue;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Consume the quote character and read the key name | |
| col++; | |
| buffer.Append(ReadQuotedValueSingleLine((char) reader.Read())); | |
| quoted = true; | |
| continue; | |
| } | |
| // Consume the quote character and read the key name | |
| ConsumeChar(); | |
| var initial = (char) reader.Read(); | |
| var key = ReadQuotedValueSingleLine(c, initial); | |
| if (key is null) return AddError("Invalid quoted key."); | |
| buffer.Append(key); | |
| quoted = true; | |
| continue; |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/External/Tommy.cs around lines 1035-1040, the code
currently consumes the opening quote then calls ReadQuotedValueSingleLine with
the next character as the quote delimiter; instead, read and store the actual
quote character as the delimiter and pass the first payload character as initial
data to ReadQuotedValueSingleLine. Concretely: after incrementing col, read the
quote char into a variable and then call ReadQuotedValueSingleLine using that
quote char as the delimiter and the next reader.Read() result as the initial
data to append.
There was a problem hiding this comment.
Ignoring, we have tests to confirm the TOML parsing and saving works
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (3)
585-589: Broaden success condition; avoid hard‑coding specific clients.lastClientRegisteredOk should reflect any client being configured, not just Cursor/Codex/Claude.
Apply:
- lastClientRegisteredOk = anyRegistered - || IsCursorConfigured(pythonDir) - || CodexConfigHelper.IsCodexConfigured(pythonDir) - || IsClaudeConfigured(); + lastClientRegisteredOk = anyRegistered + || mcpClients.clients.Any(c => c.status == McpStatus.Configured);
698-725: Harden Cursor args parsing; prefer JArray.ToObject to dynamic enumeration.Reduces fragility when Json.NET returns JToken/JArray.
Apply:
- var args = unity.args; + var args = unity.args; if (args == null) return false; // Prefer exact extraction of the --directory value and compare normalized paths - string[] strArgs = ((System.Collections.Generic.IEnumerable<object>)args) - .Select(x => x?.ToString() ?? string.Empty) - .ToArray(); - string dir = McpConfigFileHelper.ExtractDirectoryArg(strArgs); + var jarr = args as JArray; + string[] strArgs = jarr != null + ? jarr.ToObject<string[]>() + : ((IEnumerable<object>)args).Select(x => x?.ToString() ?? string.Empty).ToArray(); + string dir = McpConfigFileHelper.ExtractDirectoryArg(strArgs); if (string.IsNullOrEmpty(dir)) return false; return McpConfigFileHelper.PathsEqual(dir, pythonDir);
1456-1529: Codex configuration path is well‑factored.Parses existing TOML, respects existing uv, reuses ResolveServerDirectory, atomic write, prefs update.
Consider renaming the lock key “MCPForUnity.LockCursorConfig” to a generic “MCPForUnity.LockMcpConfig” in a follow‑up for clarity (touches multiple files).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (6)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (5)
CodexConfigHelper(17-242)IsCodexConfigured(19-41)BuildCodexServerBlock(43-49)TryParseCodexServer(105-145)UpsertCodexServerBlock(51-103)UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs (6)
McpConfigFileHelper(14-185)ExtractDirectoryArg(16-27)PathsEqual(29-46)ResolveServerDirectory(52-98)WriteAtomicFile(100-150)ResolveServerSource(152-184)UnityMcpBridge/Editor/Models/McpClient.cs (2)
McpClient(3-46)SetStatus(33-45)UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
FindUvPath(529-717)ServerInstaller(12-743)UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (2)
ConfigJsonBuilder(7-128)BuildManualConfigJson(9-29)UnityMcpBridge/Editor/Windows/ManualConfigEditorWindow.cs (2)
ManualConfigEditorWindow(9-295)ShowWindow(19-27)
🔇 Additional comments (9)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (9)
571-574: Per‑client “alreadyConfigured” fix looks good.Switching to CheckMcpConfiguration + status check resolves the prior gating bug.
665-667: RunSetupNow: per‑client “alreadyConfigured” fix looks good.Using client.status avoids Cursor‑only gating.
679-683: Mirror the broader success condition here too.Same improvement as above to avoid special‑casing.
Apply:
- lastClientRegisteredOk = anyRegistered - || IsCursorConfigured(pythonDir) - || CodexConfigHelper.IsCodexConfigured(pythonDir) - || IsClaudeConfigured(); + lastClientRegisteredOk = anyRegistered + || mcpClients.clients.Any(c => c.status == McpStatus.Configured);
1245-1245: Nice: centralized atomic writes via McpConfigFileHelper.Removes duplicated WriteAtomicFile implementation.
1267-1283: Good Codex TOML branch for manual config.Selecting TOML for Codex and JSON for others is correct.
1285-1288: Consistent server source resolution.Using McpConfigFileHelper.ResolveServerSource keeps behavior uniform across flows.
1371-1421: ConfigureMcpClient flow looks solid.
- OS‑specific config path selection
- Codex routed to ConfigureCodexClient; others to JSON writer
- Status update on success
1660-1697: Per‑client config detection improvements LGTM.
- VSCode supports new top‑level servers and legacy schema
- Codex uses TOML parsing helper
- Others use standard JSON model
1702-1716: Auto‑rewrite on path drift is appropriate.Reuses per‑client writers and preserves user experience.
Probably a good habit to keep all 3rd party licenses and copyrights, even if they're also MIT licenses
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
UnityMcpBridge/Editor/External/Tommy.cs (2)
514-520: Replace C# 9 “or” pattern in condition for C# 8 compatibility (if needed).Same “or” pattern here; use classic type checks.
Apply this diff:
- if (collapsedItem.Value is TomlArray {IsTableArray: true} or TomlTable {IsInline: false}) + { + var val = collapsedItem.Value; + if ((val is TomlArray arr && arr.IsTableArray) || (val is TomlTable tbl && tbl.IsInline == false)) { if (!first) tw.WriteLine(); first = false; collapsedItem.Value.WriteTo(tw, $"{namePrefix}{key}"); continue; } + }
1937-1961: Avoid C# 9 relational/“or” patterns in character helpers (if targeting ≤ C# 8).These helpers use C# 9 patterns. Safer to use classic comparisons for broader Unity compatibility.
Apply this diff:
- public static bool IsQuoted(char c) => c is BASIC_STRING_SYMBOL or LITERAL_STRING_SYMBOL; - - public static bool IsWhiteSpace(char c) => c is ' ' or '\t'; - - public static bool IsNewLine(char c) => c is NEWLINE_CHARACTER or NEWLINE_CARRIAGE_RETURN_CHARACTER; - - public static bool IsLineBreak(char c) => c == NEWLINE_CHARACTER; - - public static bool IsEmptySpace(char c) => IsWhiteSpace(c) || IsNewLine(c); + public static bool IsQuoted(char c) => c == BASIC_STRING_SYMBOL || c == LITERAL_STRING_SYMBOL; + + public static bool IsWhiteSpace(char c) => c == ' ' || c == '\t'; + + public static bool IsNewLine(char c) => c == NEWLINE_CHARACTER || c == NEWLINE_CARRIAGE_RETURN_CHARACTER; + + public static bool IsLineBreak(char c) => c == NEWLINE_CHARACTER; + + public static bool IsEmptySpace(char c) => IsWhiteSpace(c) || IsNewLine(c); @@ - public static bool IsValueSeparator(char c) => - c is ITEM_SEPARATOR or ARRAY_END_SYMBOL or INLINE_TABLE_END_SYMBOL; + public static bool IsValueSeparator(char c) => + c == ITEM_SEPARATOR || c == ARRAY_END_SYMBOL || c == INLINE_TABLE_END_SYMBOL;If the repo targets C# 9+, you can keep the current form. Please confirm with the script in my other comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
UnityMcpBridge/Editor/External/Tommy.cs(1 hunks)
🔇 Additional comments (2)
UnityMcpBridge/Editor/External/Tommy.cs (2)
1-10: Third‑party license/attribution required for embedded TOML parser (Tommy).This appears to vend a third‑party TOML parser. Ensure the original license and attribution are included in-repo (e.g., LICENSE file in External/, header notice in this file) per upstream terms.
Do you want me to draft the attribution header and add a LICENSE file entry for Tommy?
185-189: Fix non‑decimal integer emission: sign must precede base prefix (-0xA, not 0x-a).Current ToInlineToml emits invalid TOML for negative hex/oct/bin. Emit the sign before the base prefix.
Apply this diff:
- public override string ToInlineToml() => - IntegerBase != Base.Decimal - ? $"0{TomlSyntax.BaseIdentifiers[(int) IntegerBase]}{Convert.ToString(Value, (int) IntegerBase)}" - : Value.ToString(CultureInfo.InvariantCulture); + public override string ToInlineToml() + { + if (IntegerBase == Base.Decimal) + return Value.ToString(CultureInfo.InvariantCulture); + + var raw = Convert.ToString(Value, (int) IntegerBase); + var sign = raw.Length > 0 && raw[0] == '-' ? "-" : ""; + var digits = sign.Length == 1 ? raw.Substring(1) : raw; + return $"{sign}0{TomlSyntax.BaseIdentifiers[(int) IntegerBase]}{digits}"; + }
| var hasRealValues = !collapsedItems.All(n => n.Value is TomlTable {IsInline: false} or TomlArray {IsTableArray: true}); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Avoid C# 9 pattern “or” for Unity/C# 8 compatibility (if targeting ≤ C# 8).
This uses C# 9 “or” pattern. If the project targets C# 8 (common on Unity 2021 LTS), rewrite to classic checks.
Apply this diff:
- var hasRealValues = !collapsedItems.All(n => n.Value is TomlTable {IsInline: false} or TomlArray {IsTableArray: true});
+ var hasRealValues = !collapsedItems.All(n =>
+ {
+ var v = n.Value;
+ return (v is TomlTable tbl && tbl.IsInline == false) || (v is TomlArray arr && arr.IsTableArray);
+ });To verify language level, run:
🏁 Script executed:
#!/bin/bash
# Inspect Unity version and any LangVersion hints
fd -H ProjectVersion.txt | xargs -I{} sh -c 'echo "== {} =="; cat "{}"'
rg -n -C2 -i 'LangVersion|csc\.rsp|mcs|Roslyn|ApiCompatibilityLevel|ScriptingRuntimeVersion' || trueLength of output: 24064
Replace C# 9 or pattern with classic checks
Unity 2021.3 LTS uses C# ≤8, which doesn’t support pattern combinators.
- var hasRealValues = !collapsedItems.All(n => n.Value is TomlTable {IsInline: false} or TomlArray {IsTableArray: true});
+ var hasRealValues = !collapsedItems.All(n =>
+ {
+ var v = n.Value;
+ return (v is TomlTable tbl && tbl.IsInline == false) || (v is TomlArray arr && arr.IsTableArray);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var hasRealValues = !collapsedItems.All(n => n.Value is TomlTable {IsInline: false} or TomlArray {IsTableArray: true}); | |
| var hasRealValues = !collapsedItems.All(n => | |
| { | |
| var v = n.Value; | |
| return (v is TomlTable tbl && tbl.IsInline == false) | |
| || (v is TomlArray arr && arr.IsTableArray); | |
| }); |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/External/Tommy.cs around lines 492-493, the use of the
C#9 pattern combinator "or" in the lambda (n.Value is TomlTable {IsInline:
false} or TomlArray {IsTableArray: true}) is unsupported in Unity 2021.3 (C#
≤8); replace it with classic checks using explicit || and separate type tests,
e.g. test n.Value is TomlTable tt && tt.IsInline == false OR n.Value is
TomlArray ta && ta.IsTableArray == true, preserving the outer negation and
behavior.
There was a problem hiding this comment.
It seems to work OK with Unity 2021, will revisit if we get user errors
| var v when TomlSyntax.IsIntegerWithBase(v, out var numberBase) => new TomlInteger | ||
| { | ||
| Value = Convert.ToInt64(value.Substring(2).RemoveAll(TomlSyntax.INT_NUMBER_SEPARATOR), numberBase), | ||
| IntegerBase = (TomlInteger.Base) numberBase | ||
| }, | ||
| var _ => null |
There was a problem hiding this comment.
Parse sign for non‑decimal integers (current code ignores leading +/-).
When parsing 0x/0o/0b values, the sign is dropped. Honor the sign and adjust the digit offset.
Apply this diff:
- var v when TomlSyntax.IsIntegerWithBase(v, out var numberBase) => new TomlInteger
- {
- Value = Convert.ToInt64(value.Substring(2).RemoveAll(TomlSyntax.INT_NUMBER_SEPARATOR), numberBase),
- IntegerBase = (TomlInteger.Base) numberBase
- },
+ var v when TomlSyntax.IsIntegerWithBase(v, out var numberBase) => new TomlInteger
+ {
+ Value =
+ (value.Length > 0 && value[0] == '-' ? -1 : 1) *
+ Convert.ToInt64(
+ value.Substring(value.Length > 0 && (value[0] == '+' || value[0] == '-') ? 3 : 2)
+ .RemoveAll(TomlSyntax.INT_NUMBER_SEPARATOR),
+ numberBase),
+ IntegerBase = (TomlInteger.Base) numberBase
+ },🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/External/Tommy.cs around lines 1116 to 1121, the parser
strips the leading sign when converting non‑decimal integer literals (0x/0o/0b);
detect a leading '+' or '-' and set the digit start index to 3 when a sign is
present (2 otherwise), take value.Substring(digitStart).RemoveAll(...), convert
using the detected base, and if the original value had a '-' multiply the
resulting Int64 by -1 before assigning to TomlInteger.Value; keep IntegerBase
assignment the same.
There was a problem hiding this comment.
Ignoring, we have tests to confirm the TOML parsing and saving works
| public static readonly Regex BasedIntegerPattern = | ||
| new(@"^0(?<base>x|b|o)(?!_)(_?[0-9A-F])*$", RegexOptions.Compiled | RegexOptions.IgnoreCase); | ||
|
|
There was a problem hiding this comment.
Allow optional +/- for non‑decimal integers (parser currently rejects -0xA, +0b101).
TOML permits a leading sign for based integers. Update the regex to accept an optional sign.
Apply this diff:
- public static readonly Regex BasedIntegerPattern =
- new(@"^0(?<base>x|b|o)(?!_)(_?[0-9A-F])*$", RegexOptions.Compiled | RegexOptions.IgnoreCase);
+ public static readonly Regex BasedIntegerPattern =
+ new(@"^(\+|-)?0(?<base>x|b|o)(?!_)(_?[0-9A-F])*$", RegexOptions.Compiled | RegexOptions.IgnoreCase);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static readonly Regex BasedIntegerPattern = | |
| new(@"^0(?<base>x|b|o)(?!_)(_?[0-9A-F])*$", RegexOptions.Compiled | RegexOptions.IgnoreCase); | |
| public static readonly Regex BasedIntegerPattern = | |
| new(@"^(\+|-)?0(?<base>x|b|o)(?!_)(_?[0-9A-F])*$", RegexOptions.Compiled | RegexOptions.IgnoreCase); |
🤖 Prompt for AI Agents
In UnityMcpBridge/Editor/External/Tommy.cs around lines 1843 to 1845, the
BasedIntegerPattern regex currently rejects signed based integers; update it to
allow an optional leading + or - by adding an optional sign class before the 0
anchor (e.g. change start from ^0 to ^[+-]?0) while keeping the rest of the
pattern and RegexOptions unchanged so values like -0xA and +0b101 are accepted.
There was a problem hiding this comment.
Ignoring, we have tests to confirm the TOML parsing and saving works
* feat: add Codex CLI client support with config.toml handling * feat: add config helpers for managing Codex and MCP server configurations * feat: add TOML array parsing support for multi-line and trailing comma formats * fix: handle TOML inline comments in section headers during parsing * fix: strip TOML comments before processing section headers * fix: improve JSON parsing to handle escaped single quotes in config strings * Use Tommy for TOML parsing It's a single file and OSS, easy to integrate into Unity * fix: patched Tommy’s literal-string handling so doubled single quotes inside literal strings are treated as embedded apostrophes instead of prematurely ending the value * Don't overwrite MCP configs while testing Seeing random JSON in my codex config was pretty annoying * PR Feedback * Keep Tommy compatible with Unity 2021 * Re-include Tommy's license Probably a good habit to keep all 3rd party licenses and copyrights, even if they're also MIT licenses
1 participant
Closes #286, note that Codex uses TOML instead of JSON
Summary by CodeRabbit
New Features
Improvements
Tests