From 3722415d69bd71944802d512368903d866393cda Mon Sep 17 00:00:00 2001 From: Lou Garczynski Date: Tue, 1 Sep 2026 14:58:10 +0000 Subject: [PATCH 1/2] fix(execute_code): allow using directives at the top of a snippet WrapUserCode splices the snippet into a method body, where a using directive is illegal: the parser reads it as a using-statement and demands '('. In our usage two thirds of the snippets that declared a using failed on that alone. Hoist the leading run into the wrapper header, blanking the lines in place so compiler line numbers still map to what the caller wrote, and derive the line offset from the header instead of a hardcoded 10 that silently drifts whenever the header changes. --- MCPForUnity/Editor/Tools/ExecuteCode.cs | 69 ++++++++++++++----- .../Tests/EditMode/Tools/ExecuteCodeTests.cs | 59 ++++++++++++++++ 2 files changed, 110 insertions(+), 18 deletions(-) diff --git a/MCPForUnity/Editor/Tools/ExecuteCode.cs b/MCPForUnity/Editor/Tools/ExecuteCode.cs index bef08d49a..5a7670cf5 100644 --- a/MCPForUnity/Editor/Tools/ExecuteCode.cs +++ b/MCPForUnity/Editor/Tools/ExecuteCode.cs @@ -5,6 +5,7 @@ using System.Linq; using System.Reflection; using System.Text; +using System.Text.RegularExpressions; using MCPForUnity.Editor.Helpers; using MCPForUnity.Runtime.Helpers; using Microsoft.CSharp; @@ -19,7 +20,6 @@ public static class ExecuteCode private const int MaxCodeLength = 50000; private const int MaxHistoryEntries = 50; private const int MaxHistoryCodePreview = 500; - internal const int WrapperLineOffset = 10; private const string WrapperClassName = "MCPDynamicCode"; private const string WrapperMethodName = "Execute"; @@ -201,7 +201,7 @@ private static object HandleReplay(JObject @params) private static object CompileAndExecute(string code, string compiler) { - string wrappedSource = WrapUserCode(code); + string wrappedSource = WrapUserCode(code, out int lineOffset); string cacheKey = compiler + "\n" + wrappedSource; if (_compiledCache.TryGetValue(cacheKey, out CompiledSnippet cached)) @@ -217,14 +217,14 @@ private static object CompileAndExecute(string code, string compiler) case "roslyn": if (!RoslynCompiler.IsAvailable) return new ErrorResponse("Roslyn (Microsoft.CodeAnalysis) is not available. Install it via NuGet or use compiler='codedom'."); - compiled = RoslynCompiler.Compile(wrappedSource, assemblyPaths, out var roslynErrors); + compiled = RoslynCompiler.Compile(wrappedSource, assemblyPaths, lineOffset, out var roslynErrors); if (compiled == null) return new ErrorResponse("Compilation failed", new { errors = OffsetErrors(roslynErrors), compiler = "roslyn" }); usedCompiler = "roslyn"; break; case "codedom": - compiled = CodeDomCompile(wrappedSource, assemblyPaths, out var codedomErrors); + compiled = CodeDomCompile(wrappedSource, assemblyPaths, lineOffset, out var codedomErrors); if (compiled == null) return new ErrorResponse("Compilation failed", new { errors = OffsetErrors(codedomErrors), compiler = "codedom" }); usedCompiler = "codedom"; @@ -233,14 +233,14 @@ private static object CompileAndExecute(string code, string compiler) default: // "auto" if (RoslynCompiler.IsAvailable) { - compiled = RoslynCompiler.Compile(wrappedSource, assemblyPaths, out var autoErrors); + compiled = RoslynCompiler.Compile(wrappedSource, assemblyPaths, lineOffset, out var autoErrors); if (compiled == null) return new ErrorResponse("Compilation failed", new { errors = OffsetErrors(autoErrors), compiler = "roslyn" }); usedCompiler = "roslyn"; } else { - compiled = CodeDomCompile(wrappedSource, assemblyPaths, out var autoFallbackErrors); + compiled = CodeDomCompile(wrappedSource, assemblyPaths, lineOffset, out var autoFallbackErrors); if (compiled == null) return new ErrorResponse("Compilation failed", new { errors = OffsetErrors(autoFallbackErrors), compiler = "codedom" }); usedCompiler = "codedom"; @@ -302,7 +302,7 @@ private static List OffsetErrors(List errors) // ──────────────────── CodeDom compiler ──────────────────── - private static Assembly CodeDomCompile(string source, string[] assemblyPaths, out List errors) + private static Assembly CodeDomCompile(string source, string[] assemblyPaths, int lineOffset, out List errors) { errors = new List(); @@ -360,7 +360,7 @@ private static Assembly CodeDomCompile(string source, string[] assemblyPaths, ou continue; hasRealErrors = true; - int userLine = Math.Max(1, error.Line - WrapperLineOffset); + int userLine = Math.Max(1, error.Line - lineOffset); errors.Add($"Line {userLine}: {error.ErrorText}"); } @@ -508,22 +508,54 @@ public CodeDomAssemblyCandidate(string path, AssemblyName assemblyName) // ──────────────────── Shared helpers ──────────────────── - private static string WrapUserCode(string code) + private static readonly string[] BuiltinUsings = { + "using System;", + "using System.Collections.Generic;", + "using System.Linq;", + "using System.Reflection;", + "using System.Text;", + "using UnityEngine;", + "using UnityEditor;", + }; + + private static readonly Regex UsingDirectiveLine = new Regex( + @"^\s*using\s+(?:static\s+)?[A-Za-z_]\w*(?:\s*\.\s*[A-Za-z_]\w*)*\s*;\s*$", RegexOptions.Compiled); + + // Must not match the C# 8 `using var x = ...` declaration, which has two tokens before the = + private static readonly Regex UsingAliasLine = new Regex( + @"^\s*using\s+[A-Za-z_]\w*\s*=\s*[A-Za-z_][\w.]*\s*(?:<[^;]*>)?\s*(?:\[\s*\])*\s*;\s*$", RegexOptions.Compiled); + + private static bool IsUsingDirective(string line) + => UsingDirectiveLine.IsMatch(line) || UsingAliasLine.IsMatch(line); + + // A using directive is illegal in a method body, so the parser reads it as a using-statement and wants a '(' + // Hoisted lines are blanked, not removed, so compiler line numbers still map to the caller's. + private static string WrapUserCode(string code, out int lineOffset) + { + var body = code.Replace("\r\n", "\n").Split('\n'); + var hoisted = new List(); + for (int i = 0; i < body.Length; i++) + { + string trimmed = body[i].Trim(); + if (trimmed.Length == 0 || trimmed.StartsWith("//")) continue; + if (!IsUsingDirective(body[i])) break; + if (!BuiltinUsings.Contains(trimmed) && !hoisted.Contains(trimmed)) hoisted.Add(trimmed); + body[i] = string.Empty; + } + var sb = new StringBuilder(); - sb.AppendLine("using System;"); - sb.AppendLine("using System.Collections.Generic;"); - sb.AppendLine("using System.Linq;"); - sb.AppendLine("using System.Reflection;"); - sb.AppendLine("using UnityEngine;"); - sb.AppendLine("using UnityEditor;"); + foreach (string u in BuiltinUsings) sb.AppendLine(u); + foreach (string u in hoisted) sb.AppendLine(u); sb.AppendLine($"public static class {WrapperClassName}"); sb.AppendLine("{"); sb.AppendLine($" public static object {WrapperMethodName}()"); sb.AppendLine(" {"); - sb.AppendLine(code); + sb.AppendLine(string.Join("\n", body)); sb.AppendLine(" }"); sb.AppendLine("}"); + // 4 = the class line, its brace, the method line, its brace + lineOffset = BuiltinUsings.Length + hoisted.Count + 4; return sb.ToString(); } @@ -663,6 +695,7 @@ public static void ResetCache() _isAvailable = null; } + private static bool Initialize() { try @@ -752,7 +785,7 @@ private static bool Initialize() } } - public static Assembly Compile(string source, string[] assemblyPaths, out List errors) + public static Assembly Compile(string source, string[] assemblyPaths, int lineOffset, out List errors) { errors = new List(); @@ -837,7 +870,7 @@ public static Assembly Compile(string source, string[] assemblyPaths, out List("success"), result.ToString()); + Assert.AreEqual("b.txt", result["data"]["result"].Value()); + } + + [Test] + public void Execute_UsingStaticDirective_IsHoisted() + { + var result = Execute("using static System.Math;\nreturn Max(3, 4);"); + + Assert.IsTrue(result.Value("success"), result.ToString()); + Assert.AreEqual(4, result["data"]["result"].Value()); + } + + [Test] + public void Execute_UsingAliasWithGenericTarget_IsHoisted() + { + var result = Execute("using L = System.Collections.Generic.List;\nreturn new L { 1, 2 }.Count;"); + + Assert.IsTrue(result.Value("success"), result.ToString()); + Assert.AreEqual(2, result["data"]["result"].Value()); + } + + [Test] + public void Execute_UsingAfterComment_IsStillHoisted() + { + var result = Execute("// leading comment\nusing System.IO;\nreturn Path.GetFileName(\"/x/y.txt\");"); + + Assert.IsTrue(result.Value("success"), result.ToString()); + Assert.AreEqual("y.txt", result["data"]["result"].Value()); + } + + + [Test] + public void Execute_RedundantBuiltinUsing_DoesNotDuplicate() + { + var result = Execute("using System;\nreturn String.Concat(\"a\", \"b\");"); + + Assert.IsTrue(result.Value("success"), result.ToString()); + Assert.AreEqual("ab", result["data"]["result"].Value()); + } + + [Test] + public void Execute_ErrorLineNumber_CountsFromCallerCodeAfterHoisting() + { + var result = Execute("using System.IO;\nint ok = 1;\nnope();\nreturn ok;"); + + Assert.IsFalse(result.Value("success"), result.ToString()); + var errors = string.Join("\n", (result["data"]["errors"] as JArray).Select(e => e.Value())); + StringAssert.Contains("Line 3", errors); + } + private static JObject Execute(string code) { return ToJObject(ExecuteCode.HandleCommand(new JObject From 2fbb7a1fc9c66c48f5af0c94877d8d8017b8ac7b Mon Sep 17 00:00:00 2001 From: Lou Garczynski Date: Wed, 2 Sep 2026 13:48:12 +0000 Subject: [PATCH 2/2] fix(execute_code): handle comments in directive scanning, and stop hoisting widening safety_checks A trailing '// note' made the directive fail both regexes, and a leading block comment stopped the scan outright; either way the directive was left in the method body, where it cannot compile. Hoisting also opened a hole: 'using System.IO;' puts File.Delete in scope, which no fully-qualified entry in _blockedPatterns matches. The blocklist is now expanded with the short forms each directive creates, covering namespace imports, aliases and using static. --- MCPForUnity/Editor/Tools/ExecuteCode.cs | 85 +++++++++++++++++-- .../Tests/EditMode/Tools/ExecuteCodeTests.cs | 56 ++++++++++++ 2 files changed, 132 insertions(+), 9 deletions(-) diff --git a/MCPForUnity/Editor/Tools/ExecuteCode.cs b/MCPForUnity/Editor/Tools/ExecuteCode.cs index 5a7670cf5..b8e2cd2a8 100644 --- a/MCPForUnity/Editor/Tools/ExecuteCode.cs +++ b/MCPForUnity/Editor/Tools/ExecuteCode.cs @@ -529,20 +529,46 @@ public CodeDomAssemblyCandidate(string path, AssemblyName assemblyName) private static bool IsUsingDirective(string line) => UsingDirectiveLine.IsMatch(line) || UsingAliasLine.IsMatch(line); - // A using directive is illegal in a method body, so the parser reads it as a using-statement and wants a '(' - // Hoisted lines are blanked, not removed, so compiler line numbers still map to the caller's. - private static string WrapUserCode(string code, out int lineOffset) + // A directive can carry comments, but the regexes above must see a bare `using ...;` + private static string StripComments(string line, ref bool inBlock) + { + var sb = new StringBuilder(); + for (int i = 0; i < line.Length; i++) + { + bool pair = i + 1 < line.Length; + if (inBlock) + { + if (pair && line[i] == '*' && line[i + 1] == '/') { inBlock = false; i++; } + continue; + } + if (pair && line[i] == '/' && line[i + 1] == '*') { inBlock = true; i++; continue; } + if (pair && line[i] == '/' && line[i + 1] == '/') break; + sb.Append(line[i]); + } + return sb.ToString(); + } + + // Blanked, not removed, so compiler line numbers still map to the caller's + private static List HoistLeadingUsings(string[] body) { - var body = code.Replace("\r\n", "\n").Split('\n'); var hoisted = new List(); + bool inBlock = false; for (int i = 0; i < body.Length; i++) { - string trimmed = body[i].Trim(); - if (trimmed.Length == 0 || trimmed.StartsWith("//")) continue; - if (!IsUsingDirective(body[i])) break; - if (!BuiltinUsings.Contains(trimmed) && !hoisted.Contains(trimmed)) hoisted.Add(trimmed); + string directive = StripComments(body[i], ref inBlock).Trim(); + if (directive.Length == 0) continue; + if (!IsUsingDirective(directive)) break; + if (!BuiltinUsings.Contains(directive) && !hoisted.Contains(directive)) hoisted.Add(directive); body[i] = string.Empty; } + return hoisted; + } + + // A using directive is illegal in a method body, so the parser reads it as a using-statement and wants a '(' + private static string WrapUserCode(string code, out int lineOffset) + { + var body = code.Replace("\r\n", "\n").Split('\n'); + var hoisted = HoistLeadingUsings(body); var sb = new StringBuilder(); foreach (string u in BuiltinUsings) sb.AppendLine(u); @@ -591,9 +617,50 @@ private static string[] ResolveAssemblyPaths() return result; } + // A hoisted `using System.IO;` puts File.Delete in scope, which no fully-qualified + // pattern in _blockedPatterns matches. Derive the short forms the directives create. + private static IEnumerable ShortFormsFromUsings(IEnumerable directives) + { + foreach (string directive in directives) + { + string target = directive.Substring("using".Length).Trim().TrimEnd(';').Trim(); + bool isStatic = target.StartsWith("static ", StringComparison.Ordinal); + if (isStatic) target = target.Substring("static ".Length).Trim(); + + string alias = null; + int eq = target.IndexOf('='); + if (eq >= 0) + { + alias = target.Substring(0, eq).Trim(); + target = target.Substring(eq + 1).Trim(); + } + + int lastDot = target.LastIndexOf('.'); + string leaf = lastDot >= 0 ? target.Substring(lastDot + 1) : target; + + foreach (string pattern in _blockedPatterns) + { + if (pattern.StartsWith(target + ".", StringComparison.OrdinalIgnoreCase)) + { + string tail = pattern.Substring(target.Length + 1); + yield return alias == null ? tail : alias + "." + tail; + } + // A blocked pattern already written short, like AssetDatabase.DeleteAsset, + // needs the leaf instead: `using static ...Process;` makes Process.Start bare Start + else if (pattern.StartsWith(leaf + ".", StringComparison.OrdinalIgnoreCase)) + { + string tail = pattern.Substring(leaf.Length + 1); + if (isStatic) yield return tail; + else if (alias != null) yield return alias + "." + tail; + } + } + } + } + private static string CheckBlockedPatterns(string code) { - foreach (var pattern in _blockedPatterns) + var directives = HoistLeadingUsings(code.Replace("\r\n", "\n").Split('\n')); + foreach (var pattern in _blockedPatterns.Concat(ShortFormsFromUsings(directives))) { if (code.IndexOf(pattern, StringComparison.OrdinalIgnoreCase) >= 0) return $"Code contains blocked pattern: '{pattern}'. Disable safety checks with safety_checks=false if this is intentional."; diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs index 30e6c4b93..5865d0c37 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs @@ -596,6 +596,62 @@ public void Execute_ErrorLineNumber_CountsFromCallerCodeAfterHoisting() StringAssert.Contains("Line 3", errors); } + [Test] + public void Execute_UsingWithTrailingComment_IsHoisted() + { + var result = Execute("using System.IO; // needed for Path\nreturn Path.GetFileName(\"/a/b.txt\");"); + + Assert.IsTrue(result.Value("success"), result.ToString()); + Assert.AreEqual("b.txt", result["data"]["result"].Value()); + } + + [Test] + public void Execute_UsingAfterBlockComment_IsStillHoisted() + { + var result = Execute("/* lead\n in */\nusing System.IO;\nreturn Path.GetFileName(\"/a/c.txt\");"); + + Assert.IsTrue(result.Value("success"), result.ToString()); + Assert.AreEqual("c.txt", result["data"]["result"].Value()); + } + + [Test] + public void Execute_UsingBehindInlineBlockComment_IsHoisted() + { + var result = Execute("/* why */ using System.IO;\nreturn Path.GetFileName(\"/a/d.txt\");"); + + Assert.IsTrue(result.Value("success"), result.ToString()); + Assert.AreEqual("d.txt", result["data"]["result"].Value()); + } + + // ──────────────────── Execute: hoisting must not widen safety_checks ──────────────────── + + [Test] + public void Execute_HoistedNamespace_DoesNotBypassSafetyChecks() + { + var result = Execute("using System.IO;\nFile.Delete(\"/tmp/mcp-does-not-exist\");\nreturn 1;"); + + Assert.IsFalse(result.Value("success"), result.ToString()); + StringAssert.Contains("Blocked pattern", result.Value("error")); + } + + [Test] + public void Execute_AliasedType_DoesNotBypassSafetyChecks() + { + var result = Execute("using F = System.IO.File;\nF.Delete(\"/tmp/mcp-does-not-exist\");\nreturn 1;"); + + Assert.IsFalse(result.Value("success"), result.ToString()); + StringAssert.Contains("Blocked pattern", result.Value("error")); + } + + [Test] + public void Execute_UsingStatic_DoesNotBypassSafetyChecks() + { + var result = Execute("using static System.Diagnostics.Process;\nStart(\"ls\");\nreturn 1;"); + + Assert.IsFalse(result.Value("success"), result.ToString()); + StringAssert.Contains("Blocked pattern", result.Value("error")); + } + private static JObject Execute(string code) { return ToJObject(ExecuteCode.HandleCommand(new JObject