Skip to content

Align script management responses and validation #13

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.

Sign up for GitHub

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

Merged
dsarno merged 1 commit into from
Aug 17, 2025
Merged

Align script management responses and validation #13

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 100 additions & 18 deletions UnityMcpBridge/Editor/Tools/ManageScript.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ public static object HandleCommand(JObject @params)
}
case "validate":
{
string level = @params["level"]?.ToString()?.ToLowerInvariant() ?? "basic";
string level = @params["level"]?.ToString()?.ToLowerInvariant() ?? "standard";
var chosen = level switch
{
"basic" => ValidationLevel.Basic,
Expand Down Expand Up @@ -343,8 +343,10 @@ private static object ReadScript(string fullPath, string relativePath)

// Return both normal and encoded contents for larger files
bool isLarge = contents.Length > 10000; // If content is large, include encoded version
var uri = $"unity://path/{relativePath}";
var responseData = new
{
uri,
path = relativePath,
contents = contents,
// For large files, also include base64-encoded version
Expand Down Expand Up @@ -406,23 +408,36 @@ string contents
try
{
File.Replace(tempPath, fullPath, backupPath);
// Clean up backup to avoid stray assets inside the project
try
{
if (File.Exists(backupPath))
File.Delete(backupPath);
}
catch
{
// ignore failures deleting the backup
}
}
catch (PlatformNotSupportedException)
{
File.Copy(tempPath, fullPath, true);
try { File.Delete(tempPath); } catch { }
try { if (File.Exists(backupPath)) File.Delete(backupPath); } catch { }
}
catch (IOException)
{
// Cross-volume moves can throw IOException; fallback to copy
File.Copy(tempPath, fullPath, true);
try { File.Delete(tempPath); } catch { }
try { if (File.Exists(backupPath)) File.Delete(backupPath); } catch { }
}

// Prepare success response BEFORE any operation that can trigger a domain reload
var uri = $"unity://path/{relativePath}";
var ok = Response.Success(
$"Script '{name}.cs' updated successfully at '{relativePath}'.",
new { path = relativePath, scheduledRefresh = true }
new { uri, path = relativePath, scheduledRefresh = true }
);

// Schedule a debounced import/compile on next editor tick to avoid stalling the reply
Expand Down Expand Up @@ -450,6 +465,17 @@ private static object ApplyTextEdits(
{
if (!File.Exists(fullPath))
return Response.Error($"Script not found at '{relativePath}'.");
// Refuse edits if the target is a symlink
try
{
var attrs = File.GetAttributes(fullPath);
if ((attrs & FileAttributes.ReparsePoint) != 0)
return Response.Error("Refusing to edit a symlinked script path.");
}
catch
{
// If checking attributes fails, proceed without the symlink guard
}
if (edits == null || edits.Count == 0)
return Response.Error("No edits provided.");

Expand Down Expand Up @@ -555,9 +581,23 @@ private static object ApplyTextEdits(
var tmp = fullPath + ".tmp";
File.WriteAllText(tmp, working, enc);
string backup = fullPath + ".bak";
try { File.Replace(tmp, fullPath, backup); }
catch (PlatformNotSupportedException) { File.Copy(tmp, fullPath, true); try { File.Delete(tmp); } catch { } }
catch (IOException) { File.Copy(tmp, fullPath, true); try { File.Delete(tmp); } catch { } }
try
{
File.Replace(tmp, fullPath, backup);
try { if (File.Exists(backup)) File.Delete(backup); } catch { /* ignore */ }
}
catch (PlatformNotSupportedException)
{
File.Copy(tmp, fullPath, true);
try { File.Delete(tmp); } catch { }
try { if (File.Exists(backup)) File.Delete(backup); } catch { }
}
catch (IOException)
{
File.Copy(tmp, fullPath, true);
try { File.Delete(tmp); } catch { }
try { if (File.Exists(backup)) File.Delete(backup); } catch { }
}

ManageScriptRefreshHelpers.ScheduleScriptRefresh(relativePath);
return Response.Success(
Expand Down Expand Up @@ -738,6 +778,17 @@ private static object EditScript(
{
if (!File.Exists(fullPath))
return Response.Error($"Script not found at '{relativePath}'.");
// Refuse edits if the target is a symlink
try
{
var attrs = File.GetAttributes(fullPath);
if ((attrs & FileAttributes.ReparsePoint) != 0)
return Response.Error("Refusing to edit a symlinked script path.");
}
catch
{
// ignore failures checking attributes and proceed
}
if (edits == null || edits.Count == 0)
return Response.Error("No edits provided.");

Expand Down Expand Up @@ -986,9 +1037,23 @@ private static object EditScript(
var tmp = fullPath + ".tmp";
File.WriteAllText(tmp, working, enc);
string backup = fullPath + ".bak";
try { File.Replace(tmp, fullPath, backup); }
catch (PlatformNotSupportedException) { File.Copy(tmp, fullPath, true); try { File.Delete(tmp); } catch { } }
catch (IOException) { File.Copy(tmp, fullPath, true); try { File.Delete(tmp); } catch { } }
try
{
File.Replace(tmp, fullPath, backup);
try { if (File.Exists(backup)) File.Delete(backup); } catch { /* ignore */ }
}
catch (PlatformNotSupportedException)
{
File.Copy(tmp, fullPath, true);
try { File.Delete(tmp); } catch { }
try { if (File.Exists(backup)) File.Delete(backup); } catch { }
}
catch (IOException)
{
File.Copy(tmp, fullPath, true);
try { File.Delete(tmp); } catch { }
try { if (File.Exists(backup)) File.Delete(backup); } catch { }
}

// Decide refresh behavior
string refreshMode = options?["refresh"]?.ToString()?.ToLowerInvariant();
Expand All @@ -1001,11 +1066,17 @@ private static object EditScript(

if (immediate)
{
// Force an immediate import/compile on the main thread
AssetDatabase.ImportAsset(relativePath, ImportAssetOptions.ForceSynchronousImport | ImportAssetOptions.ForceUpdate);
// Force on main thread
EditorApplication.delayCall += () =>
{
AssetDatabase.ImportAsset(
relativePath,
ImportAssetOptions.ForceSynchronousImport | ImportAssetOptions.ForceUpdate
);
#if UNITY_EDITOR
UnityEditor.Compilation.CompilationPipeline.RequestScriptCompilation();
UnityEditor.Compilation.CompilationPipeline.RequestScriptCompilation();
#endif
};
}
else
{
Expand Down Expand Up @@ -1523,11 +1594,14 @@ private static bool ValidateScriptSyntax(string contents, ValidationLevel level,
}

#if USE_ROSLYN
// Advanced Roslyn-based validation
if (!ValidateScriptSyntaxRoslyn(contents, level, errorList))
// Advanced Roslyn-based validation: only run for Standard+; fail on Roslyn errors
if (level >= ValidationLevel.Standard)
{
errors = errorList.ToArray();
return level != ValidationLevel.Standard; //TODO: Allow standard to run roslyn right now, might formalize it in the future
if (!ValidateScriptSyntaxRoslyn(contents, level, errorList))
{
errors = errorList.ToArray();
return false;
}
}
#endif

Expand Down Expand Up @@ -2105,20 +2179,28 @@ static class RefreshDebounce
{
private static int _pending;
private static DateTime _last;
private static readonly object _lock = new object();
private static readonly HashSet<string> _paths = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
private static bool _scheduled;

public static void Schedule(string relPath, TimeSpan window)
{
Interlocked.Exchange(ref _pending, 1);
lock (_lock) { _paths.Add(relPath); }
var now = DateTime.UtcNow;
if ((now - _last) < window) return;
if (_scheduled && (now - _last) < window) return;
_last = now;
_scheduled = true;
Copy link

@cursor cursor bot Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Debouncing Logic Fails to Prevent Race Conditions

The RefreshDebounce.Schedule method's debouncing logic is flawed. The _scheduled flag isn't synchronized, which can lead to race conditions and duplicate scheduling. Also, the _scheduled && (now - _last) < window condition allows new refreshes to be scheduled immediately after a debounced call completes, bypassing the time window and defeating the debouncing.

Fix in Cursor Fix in Web


EditorApplication.delayCall += () =>
{
_scheduled = false;
if (Interlocked.Exchange(ref _pending, 0) == 1)
{
// Prefer targeted import and script compile over full refresh
AssetDatabase.ImportAsset(relPath, ImportAssetOptions.ForceUpdate);
string[] toImport;
lock (_lock) { toImport = _paths.ToArray(); _paths.Clear(); }
foreach (var p in toImport)
AssetDatabase.ImportAsset(p, ImportAssetOptions.ForceUpdate);
#if UNITY_EDITOR
UnityEditor.Compilation.CompilationPipeline.RequestScriptCompilation();
#endif
Expand Down