Bug fix and log feature update#919
Merged
Scriptwonder merged 2 commits intoCoplayDev:betafrom Mar 11, 2026
Merged
Conversation
1. Add McpLog that could log mcp calls and errors under Asset/ 2. Solve issues CoplayDev#816 via incluging str in annotation, extracted AssignedObjectreference() helper that verify assignments and resolve component types from gameobjects.
Contributor
Reviewer's GuideIntroduces structured MCP execution logging to asset files and refactors Unity object assignment to support string IDs and component filtering while relaxing several Python tool parameter annotations to also accept plain strings. Sequence diagram for MCP command execution loggingsequenceDiagram
actor EditorUser
participant McpAdvancedSection
participant TransportCommandDispatcher
participant CommandRegistry
participant PendingCommand
participant McpLogRecord
EditorUser->>McpAdvancedSection: Toggle logRecordToggle
McpAdvancedSection->>McpLogRecord: Set IsEnabled
EditorUser->>TransportCommandDispatcher: Execute MCP command
TransportCommandDispatcher->>CommandRegistry: ExecuteCommand(type, parameters, completionSource)
alt Async command (result is null)
TransportCommandDispatcher->>TransportCommandDispatcher: Start Stopwatch (sw)
CommandRegistry-->>PendingCommand: completionSource.Task
PendingCommand-->>TransportCommandDispatcher: Task completion callback
TransportCommandDispatcher->>TransportCommandDispatcher: Stop sw
TransportCommandDispatcher->>McpLogRecord: Log(commandType, parameters, logType, status, durationMs, error)
else Sync command (result not null)
TransportCommandDispatcher->>TransportCommandDispatcher: Start Stopwatch (sw)
CommandRegistry-->>TransportCommandDispatcher: result
TransportCommandDispatcher->>TransportCommandDispatcher: Stop sw
TransportCommandDispatcher->>McpLogRecord: Log(commandType, parameters, logType, SUCCESS, durationMs)
end
Class diagram for McpLogRecord and ComponentOps updatesclassDiagram
class McpLogRecord {
- string LogPath
- string ErrorLogPath
- long MaxLogSizeBytes
- bool _sessionStarted
+ bool IsEnabled
+ void Log(string commandType, JObject parameters, string type, string status, long durationMs, string error)
- void AppendLine(string path, string line)
- void RotateIfNeeded(string path)
}
class EditorPrefKeys {
<<static>>
+ string LogRecordEnabled
}
class TransportCommandDispatcher {
- void ProcessCommand(string id, PendingCommand pending)
}
class ComponentOps {
- bool SetObjectReference(SerializedProperty prop, JToken value, string error)
- bool AssignObjectReference(SerializedProperty prop, UnityEngineObject resolved, string componentFilter, string error)
- bool ResolveSceneObjectByName(SerializedProperty prop, string name, string error)
}
class McpAdvancedSection {
- Toggle debugLogsToggle
- Toggle logRecordToggle
+ void CacheUIElements()
+ void InitializeUI()
+ void RegisterCallbacks()
+ void UpdatePathOverrides()
}
class SerializedProperty {
+ UnityEngineObject objectReferenceValue
}
class GameObject {
+ string name
+ Component[] GetComponents()
}
class Component {
}
class GameObjectLookup {
+ GameObject ResolveInstanceID(int id)
+ GameObject ResolveSceneObjectByName(string name)
}
class AssetDatabase {
+ UnityEngineObject LoadAssetAtPath(string path)
}
class AssetPathUtility {
+ string SanitizeAssetPath(string path)
}
class McpLog {
+ void Warn(string message)
+ void SetDebugLoggingEnabled(bool enabled)
}
McpLogRecord ..> EditorPrefKeys : uses
McpLogRecord ..> McpLog : logsWarnings
McpAdvancedSection ..> McpLogRecord : togglesLogging
TransportCommandDispatcher ..> McpLogRecord : recordsExecution
ComponentOps ..> GameObjectLookup : resolvesGameObjects
ComponentOps ..> AssetDatabase : loadsAssets
ComponentOps ..> AssetPathUtility : sanitizesPaths
ComponentOps --> SerializedProperty : assignsReferences
ComponentOps --> GameObject : inspectsComponents
GameObject --> Component : contains
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds editor-side structured logging (mcp.log) with a UI toggle, introduces a helper for component-aware SerializedProperty assignments, and widens several server tool function parameters to accept either dicts or JSON strings. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor Process
participant Dispatcher as TransportCommandDispatcher
participant Logger as McpLogRecord
participant FS as FileSystem
Editor->>Dispatcher: Execute command (sync/async)
Dispatcher->>Dispatcher: Start stopwatch
alt sync
Dispatcher->>Dispatcher: Get result
Dispatcher->>Logger: Log SUCCESS (type, params, ms)
else async
Dispatcher->>Dispatcher: Capture info, await task
Dispatcher->>Logger: Log SUCCESS/ERROR (type, params, ms, status, error?)
end
Logger->>Logger: If IsEnabled?
alt enabled
Logger->>Logger: Build JSON entry (ts, tool, type, status, ms, ...)
alt first session log
Logger->>FS: Write session_start entry to mcp.log
end
Logger->>FS: Append entry to mcp.log
Logger->>Logger: RotateIfNeeded (>1MB)
alt status == ERROR
Logger->>FS: Append to mcpError.log
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
AssignObjectReference, thecomponentFilteris only applied whenresolvedis aGameObject; if the resolver ever returns aComponentdirectly, the filter will be silently ignored—consider either validating the filter in that case or enforcing thatcomponentFilteris only used withGameObjectresolutions. - The new
McpLogRecordlogger writes to the same files from async continuations and potentially the main thread without any locking; adding a simple synchronization mechanism (e.g., alockaroundAppendLine/RotateIfNeeded) would reduce the risk of interleaved or corrupted log writes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AssignObjectReference`, the `componentFilter` is only applied when `resolved` is a `GameObject`; if the resolver ever returns a `Component` directly, the filter will be silently ignored—consider either validating the filter in that case or enforcing that `componentFilter` is only used with `GameObject` resolutions.
- The new `McpLogRecord` logger writes to the same files from async continuations and potentially the main thread without any locking; adding a simple synchronization mechanism (e.g., a `lock` around `AppendLine`/`RotateIfNeeded`) would reduce the risk of interleaved or corrupted log writes.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs" line_range="374-377" />
<code_context>
+ var capturedType = command.type;
+ var capturedParams = parameters;
+ var capturedLogType = logType;
+ pending.CompletionSource.Task.ContinueWith(t =>
{
+ sw?.Stop();
+ McpLogRecord.Log(capturedType, capturedParams, capturedLogType,
+ t.IsFaulted ? "ERROR" : "SUCCESS",
+ sw?.ElapsedMilliseconds ?? 0,
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid calling McpLogRecord (and Unity APIs transitively) from a background thread in the async continuation.
Because this continuation uses `TaskScheduler.Default`, it will typically run on a thread‑pool thread. `McpLogRecord.Log` reads Unity state (`Application.unityVersion`, `EditorPrefs` via `IsEnabled`, and possibly `McpLog.Warn`), which is generally only safe from the main thread in the editor and may lead to subtle bugs or crashes when called off‑thread.
Please either:
- Marshal the logging work to the main thread (e.g., via `EditorApplication.delayCall`), or
- Capture only plain data here and forward it to a main‑thread logger.
Also ensure the synchronous `.Result` path uses the same main‑thread logging model for consistency.
</issue_to_address>
### Comment 2
<location path="MCPForUnity/Editor/Helpers/McpLogRecord.cs" line_range="65-68" />
<code_context>
+ var line = entry.ToString(Formatting.None);
+ AppendLine(LogPath, line);
+
+ if (status == "ERROR")
+ {
+ RotateIfNeeded(ErrorLogPath);
+ AppendLine(ErrorLogPath, line);
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider synchronizing log writes/rotation to avoid races when multiple commands log concurrently.
`Log` may now be invoked concurrently from multiple async continuations. `RotateIfNeeded` rewrites the file while `AppendLine` appends, but these aren’t synchronized. Under load, that can corrupt logs or throw IO exceptions. To harden this, consider serializing access per file (e.g., a lock around `RotateIfNeeded` + `AppendLine`, or a dedicated logging queue).
Suggested implementation:
```csharp
if (parameters != null)
entry["params"] = parameters;
if (error != null)
entry["error"] = error;
var line = entry.ToString(Formatting.None);
// Serialize writes to the main log file to avoid races between concurrent log calls.
lock (_logFileLock)
{
AppendLine(LogPath, line);
}
if (status == "ERROR")
{
// Serialize rotation + append for the error log to avoid interleaving of rewrites and appends.
lock (_errorLogFileLock)
{
RotateIfNeeded(ErrorLogPath);
AppendLine(ErrorLogPath, line);
}
}
```
```csharp
// Synchronization objects to serialize access to log files across concurrent log invocations.
private static readonly object _logFileLock = new object();
private static readonly object _errorLogFileLock = new object();
internal static void Log(string commandType, JObject parameters, string type, string status, long durationMs, string error = null)
{
if (!IsEnabled) return;
try
{
if (!_sessionStarted)
{
_sessionStarted = true;
```
1. If `AppendLine` or `RotateIfNeeded` are called on `LogPath` / `ErrorLogPath` from other methods in this class, those call sites should also be wrapped in the corresponding `lock` to ensure all access to a given file is serialized.
2. If there are additional log files beyond `LogPath` and `ErrorLogPath`, consider introducing corresponding lock objects and following the same pattern.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+374
to
+377
| pending.CompletionSource.Task.ContinueWith(t => | ||
| { | ||
| sw?.Stop(); | ||
| McpLogRecord.Log(capturedType, capturedParams, capturedLogType, |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Avoid calling McpLogRecord (and Unity APIs transitively) from a background thread in the async continuation.
Because this continuation uses TaskScheduler.Default, it will typically run on a thread‑pool thread. McpLogRecord.Log reads Unity state (Application.unityVersion, EditorPrefs via IsEnabled, and possibly McpLog.Warn), which is generally only safe from the main thread in the editor and may lead to subtle bugs or crashes when called off‑thread.
Please either:
- Marshal the logging work to the main thread (e.g., via
EditorApplication.delayCall), or - Capture only plain data here and forward it to a main‑thread logger.
Also ensure the synchronous .Result path uses the same main‑thread logging model for consistency.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in MCP execution file logger in the Unity Editor and improves server/tool parameter handling + Unity-side reference assignment to address #816 (setting Component/ObjectReference properties via manage_components).
Changes:
- Broadened several Python tool schemas to accept JSON strings for
properties/component_propertiespayloads. - Added Unity Editor “Log Record” toggle and
McpLogRecordto write per-command execution entries (including duration/status). - Refactored Unity ObjectReference assignment to centralize validation and improve instanceID/string handling + component resolution.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Server/src/services/tools/manage_material.py | Accept properties as dict or JSON string in tool schema. |
| Server/src/services/tools/manage_gameobject.py | Accept component_properties as dict or JSON string in tool schema. |
| Server/src/services/tools/manage_components.py | Accept properties as dict or JSON string in tool schema. |
| Server/src/services/tools/manage_asset.py | Accept properties as dict or JSON string in tool schema. |
| MCPForUnity/Editor/Windows/Components/Advanced/McpAdvancedSection.uxml | Adds UI toggle for enabling file-based execution logging. |
| MCPForUnity/Editor/Windows/Components/Advanced/McpAdvancedSection.cs | Wires the new toggle to EditorPrefs via McpLogRecord.IsEnabled. |
| MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs | Measures/logs tool/resource executions and async completion results. |
| MCPForUnity/Editor/Helpers/McpLogRecord.cs | New helper that appends JSONL log entries and rotates logs at 1MB. |
| MCPForUnity/Editor/Helpers/ComponentOps.cs | Adds AssignObjectReference helper with component fallback/filter and improved string handling. |
| MCPForUnity/Editor/Constants/EditorPrefKeys.cs | Adds LogRecordEnabled preference key. |
Comments suppressed due to low confidence (1)
MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs:398
- Exceptions caught in
ProcessCommand(sync execution errors) are not written toMcpLogRecord, so the file log will miss exactly the failures it’s meant to capture. Consider callingMcpLogRecord.Log(..., status="ERROR", ...)inside thecatchblock as well (including a useful error message/stack info as appropriate).
sw?.Stop();
McpLogRecord.Log(command.type, parameters, logType, "SUCCESS", sw?.ElapsedMilliseconds ?? 0);
var response = new { status = "success", result };
pending.TrySetResult(JsonConvert.SerializeObject(response));
RemovePending(id, pending);
}
catch (Exception ex)
{
McpLog.Error($"Error processing command: {ex.Message}\n{ex.StackTrace}");
pending.TrySetResult(SerializeError(ex.Message, "Unknown (error during processing)", ex.StackTrace));
RemovePending(id, pending);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+13
to
+15
| private static readonly string LogPath = Path.Combine(Application.dataPath, "mcp.log"); | ||
| private static readonly string ErrorLogPath = Path.Combine(Application.dataPath, "mcpError.log"); | ||
| private const long MaxLogSizeBytes = 1024 * 1024; // 1 MB |
There was a problem hiding this comment.
Log files are written under Application.dataPath (Assets/), which will trigger frequent asset-database refresh/import when enabled and can significantly slow the editor + create noisy VCS changes. Consider writing to Library/ (or Application.persistentDataPath/Application.temporaryCachePath) and/or ensuring these files are excluded from version control.
Comment on lines
+56
to
+60
| if (parameters != null) | ||
| entry["params"] = parameters; | ||
|
|
||
| if (error != null) | ||
| entry["error"] = error; |
There was a problem hiding this comment.
This logs the full parameters object into the file. Tool params can include very large blobs (e.g., base64 file contents/previews) and potentially sensitive values, so this can quickly bloat logs and leak data into plain-text files under the project. Consider logging only a whitelist of safe keys (or only param keys), and truncating large values.
Comment on lines
+77
to
+93
| private static void AppendLine(string path, string line) | ||
| { | ||
| File.AppendAllText(path, line + Environment.NewLine); | ||
| } | ||
|
|
||
| private static void RotateIfNeeded(string path) | ||
| { | ||
| try | ||
| { | ||
| if (!File.Exists(path)) return; | ||
| var info = new FileInfo(path); | ||
| if (info.Length <= MaxLogSizeBytes) return; | ||
|
|
||
| var lines = File.ReadAllLines(path); | ||
| var half = lines.Length / 2; | ||
| File.WriteAllLines(path, lines[half..]); | ||
| } |
There was a problem hiding this comment.
AppendLine/RotateIfNeeded are not synchronized. If multiple commands complete concurrently, log writes and rotations can interleave (e.g., one thread rotating while another appends), causing lost/corrupted lines or IO exceptions. Consider guarding file operations with a lock, or using a single-producer queue/background writer so rotation and appends are serialized.
Comment on lines
+377
to
+380
| McpLogRecord.Log(capturedType, capturedParams, capturedLogType, | ||
| t.IsFaulted ? "ERROR" : "SUCCESS", | ||
| sw?.ElapsedMilliseconds ?? 0, | ||
| t.IsFaulted ? t.Exception?.InnerException?.Message : null); |
There was a problem hiding this comment.
In the async-command path, canceled tasks will currently be logged as SUCCESS because the status only checks t.IsFaulted. Consider handling t.IsCanceled explicitly (e.g., log CANCELED/ERROR) so logs don’t incorrectly report cancellations as successes.
Suggested change
| McpLogRecord.Log(capturedType, capturedParams, capturedLogType, | |
| t.IsFaulted ? "ERROR" : "SUCCESS", | |
| sw?.ElapsedMilliseconds ?? 0, | |
| t.IsFaulted ? t.Exception?.InnerException?.Message : null); | |
| var status = t.IsCanceled | |
| ? "CANCELED" | |
| : t.IsFaulted | |
| ? "ERROR" | |
| : "SUCCESS"; | |
| var errorMessage = t.IsFaulted | |
| ? t.Exception?.InnerException?.Message | |
| : t.IsCanceled | |
| ? "Task was canceled." | |
| : null; | |
| McpLogRecord.Log(capturedType, capturedParams, capturedLogType, | |
| status, | |
| sw?.ElapsedMilliseconds ?? 0, | |
| errorMessage); |
Comment on lines
+749
to
+760
| foreach (var comp in components) | ||
| { | ||
| if (comp == null) continue; | ||
| if (string.Equals(comp.GetType().Name, componentFilter, StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(comp.GetType().FullName, componentFilter, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| prop.objectReferenceValue = comp; | ||
| if (prop.objectReferenceValue != null) | ||
| return true; | ||
| } | ||
| } | ||
| error = $"Component '{componentFilter}' not found on GameObject '{filterGo.name}'."; |
There was a problem hiding this comment.
When a componentFilter is provided and the target property does not accept Components (e.g., the field is a GameObject), this branch will fail and return Component '<name>' not found even if the component exists—because assignment is rejected by the property type. Consider improving the failure message to distinguish “component not found” vs “component found but incompatible”, and/or falling back to assigning the GameObject if it’s compatible.
Suggested change
| foreach (var comp in components) | |
| { | |
| if (comp == null) continue; | |
| if (string.Equals(comp.GetType().Name, componentFilter, StringComparison.OrdinalIgnoreCase) || | |
| string.Equals(comp.GetType().FullName, componentFilter, StringComparison.OrdinalIgnoreCase)) | |
| { | |
| prop.objectReferenceValue = comp; | |
| if (prop.objectReferenceValue != null) | |
| return true; | |
| } | |
| } | |
| error = $"Component '{componentFilter}' not found on GameObject '{filterGo.name}'."; | |
| var foundMatchingComponent = false; | |
| foreach (var comp in components) | |
| { | |
| if (comp == null) continue; | |
| if (string.Equals(comp.GetType().Name, componentFilter, StringComparison.OrdinalIgnoreCase) || | |
| string.Equals(comp.GetType().FullName, componentFilter, StringComparison.OrdinalIgnoreCase)) | |
| { | |
| foundMatchingComponent = true; | |
| // First, try assigning the component itself. | |
| prop.objectReferenceValue = comp; | |
| if (prop.objectReferenceValue != null) | |
| return true; | |
| // If the property does not accept this component type, try assigning | |
| // the GameObject itself in case the property expects a GameObject. | |
| prop.objectReferenceValue = filterGo; | |
| if (prop.objectReferenceValue != null) | |
| return true; | |
| } | |
| } | |
| if (foundMatchingComponent) | |
| { | |
| error = $"Component '{componentFilter}' found on GameObject '{filterGo.name}' but it is not compatible with the property type."; | |
| } | |
| else | |
| { | |
| error = $"Component '{componentFilter}' not found on GameObject '{filterGo.name}'."; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Server/src/services/tools/manage_gameobject.py (1)
14-38: Consider validating the nested dict structure.The
_normalize_component_propertiesfunction accepts any dict without validating that the values are themselves dicts. According to the annotation on lines 92-97, the expected structure isdict[str, dict[str, Any]]. Input like{"MyScript": "not_a_dict"}would pass validation but may cause errors on the Unity side.🛡️ Optional: Add nested structure validation
def _normalize_component_properties(value: Any) -> tuple[dict[str, dict[str, Any]] | None, str | None]: """ Robustly normalize component_properties to a dict. Returns (parsed_dict, error_message). If error_message is set, parsed_dict is None. """ if value is None: return None, None - # Already a dict - validate structure + # Already a dict - validate nested structure if isinstance(value, dict): + for key, val in value.items(): + if not isinstance(val, dict): + return None, f"component_properties['{key}'] must be a dict, got {type(val).__name__}" return value, None # Try parsing as JSON string if isinstance(value, str): # Check for obviously invalid values if value in ("[object Object]", "undefined", "null", ""): return None, f"component_properties received invalid value: '{value}'. Expected a JSON object like {{\"ComponentName\": {{\"property\": value}}}}" parsed = parse_json_payload(value) if isinstance(parsed, dict): + for key, val in parsed.items(): + if not isinstance(val, dict): + return None, f"component_properties['{key}'] must be a dict, got {type(val).__name__}" return parsed, None return None, f"component_properties must be a JSON object (dict), got string that parsed to {type(parsed).__name__}" return None, f"component_properties must be a dict or JSON string, got {type(value).__name__}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_gameobject.py` around lines 14 - 38, The function _normalize_component_properties should enforce that when a dict (or a parsed JSON dict) is accepted, each value is itself a dict[str, Any]; update _normalize_component_properties to iterate over the top-level dict (after parse_json_payload when value is a string) and validate that every value is a dict, returning (None, error_message) if any entry has a non-dict value (include the offending key and its type in the message) and otherwise return the validated dict; preserve existing special-string checks and error formats so callers still receive the same return types.MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs (1)
371-377: Snapshot the request params before deferring the log.Line 372 captures the same mutable params object that gets passed into
CommandRegistry.ExecuteCommand(). If a handler normalizes or rewrites that object before completion, the recorded entry no longer reflects the incoming request. Capture a snapshot before execution and log that snapshot in both branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs` around lines 371 - 377, The code currently captures the mutable parameters object (parameters) into capturedParams and defers logging until pending.CompletionSource.Task finishes, which can record a mutated request; create an immutable snapshot of the request parameters (e.g., capturedParamsSnapshot) immediately before calling CommandRegistry.ExecuteCommand (or before creating pending) and use that snapshot in the ContinueWith callback and in both success/failure log branches when calling McpLogRecord.Log(capturedType, capturedParamsSnapshot, capturedLogType,...). Ensure the snapshot is a deep copy/JSON clone so later handler mutations don't affect the logged data and replace all uses of capturedParams in the deferred logging with the snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs`:
- Around line 600-601: The component filter parsed into the variable
componentFilter is not being forwarded when the payload uses the { "name": ... }
branch, and ResolveSceneObjectByName(...) is being called with a hardcoded null
component; update the name-branch code path to pass componentFilter through to
the same resolution logic (i.e. call ResolveSceneObjectByName(name,
componentFilter) or otherwise forward componentFilter) so that
ResolveSceneObjectByName receives the requested component type instead of null;
ensure all analogous spots noted (the blocks around the { "name": ... } branch
and the call sites that currently pass null) are updated to use the
componentFilter variable.
In `@MCPForUnity/Editor/Helpers/McpLogRecord.cs`:
- Around line 30-40: The log rotation is only invoked when starting a session
(_sessionStarted check), so subsequent AppendLine calls (AppendLine(LogPath,
...)) never recheck size and mcp.log can grow past the limit; fix by ensuring
RotateIfNeeded(LogPath) is called before every AppendLine call (i.e., move or
add RotateIfNeeded(LogPath) so it runs both inside the session start path and
the normal append path or incorporate rotation into AppendLine itself),
referencing the RotateIfNeeded, _sessionStarted, AppendLine and LogPath symbols
when you update the code.
- Around line 24-69: Concurrent calls to Log() can race on _sessionStarted,
RotateIfNeeded(), and AppendLine(), causing duplicate session_start records or
lost lines; protect the entire critical section by introducing and using a
single private static lock object (e.g., _logLock) and wrap the code paths that
read/modify _sessionStarted, call RotateIfNeeded(LogPath/ErrorLogPath) and call
AppendLine(...) in a single lock statement inside Log(string, JObject, ...).
Apply the same locking to the other logging method that touches the same state
(the other Log overload referenced in the comment) so session rotation and
appends are serialized.
In `@MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs`:
- Around line 364-387: The async logging currently only checks t.IsFaulted and
logs SUCCESS for handlers that return a JSON payload with status="error"; update
the continuation that runs on pending.CompletionSource (and the synchronous path
after CommandRegistry.ExecuteCommand returns non-null, plus any early returns
such as SerializeError) to derive the outcome from the completed payload instead
of only IsFaulted — inspect the Task result
(pending.CompletionSource.Task.Result or the object passed via TrySetResult) to
read the response status field and map it to "SUCCESS"/"ERROR", include any
error message when status indicates failure, and call McpLogRecord.Log with that
derived status and message; alternatively, change the call sites where handlers
complete to pass an explicit outcome into the continuation and ensure
RemovePending is still scheduled after logging. Ensure all failure flows (sync
exceptions, SerializeError returns, and async responses with status="error")
invoke McpLogRecord.Log using the same mapping logic.
---
Nitpick comments:
In `@MCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.cs`:
- Around line 371-377: The code currently captures the mutable parameters object
(parameters) into capturedParams and defers logging until
pending.CompletionSource.Task finishes, which can record a mutated request;
create an immutable snapshot of the request parameters (e.g.,
capturedParamsSnapshot) immediately before calling
CommandRegistry.ExecuteCommand (or before creating pending) and use that
snapshot in the ContinueWith callback and in both success/failure log branches
when calling McpLogRecord.Log(capturedType, capturedParamsSnapshot,
capturedLogType,...). Ensure the snapshot is a deep copy/JSON clone so later
handler mutations don't affect the logged data and replace all uses of
capturedParams in the deferred logging with the snapshot.
In `@Server/src/services/tools/manage_gameobject.py`:
- Around line 14-38: The function _normalize_component_properties should enforce
that when a dict (or a parsed JSON dict) is accepted, each value is itself a
dict[str, Any]; update _normalize_component_properties to iterate over the
top-level dict (after parse_json_payload when value is a string) and validate
that every value is a dict, returning (None, error_message) if any entry has a
non-dict value (include the offending key and its type in the message) and
otherwise return the validated dict; preserve existing special-string checks and
error formats so callers still receive the same return types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd2e6757-eb3d-4085-b54b-d8c3d19123a1
📒 Files selected for processing (11)
MCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/McpLogRecord.csMCPForUnity/Editor/Helpers/McpLogRecord.cs.metaMCPForUnity/Editor/Services/Transport/TransportCommandDispatcher.csMCPForUnity/Editor/Windows/Components/Advanced/McpAdvancedSection.csMCPForUnity/Editor/Windows/Components/Advanced/McpAdvancedSection.uxmlServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_components.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_material.py
Comment on lines
+600
to
+601
| // Optional component type filter — e.g. {"instanceID": 123, "component": "Button"} | ||
| string componentFilter = jObj["component"]?.ToString(); |
Contributor
There was a problem hiding this comment.
Pass componentFilter through the { "name": ... } path.
componentFilter is parsed at Lines 600-601, but the { "name": ... } branch at Line 689 drops it and ResolveSceneObjectByName() hardcodes null at Line 819. A payload like { "name": "Canvas", "component": "Button" } will therefore ignore the requested component or fall back to the first compatible one.
🧩 Proposed fix
- return ResolveSceneObjectByName(prop, nameToken.ToString(), out error);
+ return ResolveSceneObjectByName(prop, nameToken.ToString(), componentFilter, out error);
...
- private static bool ResolveSceneObjectByName(SerializedProperty prop, string name, out string error)
+ private static bool ResolveSceneObjectByName(SerializedProperty prop, string name, string componentFilter, out string error)
{
error = null;
if (string.IsNullOrWhiteSpace(name))
{
error = "Cannot resolve object reference from empty name.";
@@
- return AssignObjectReference(prop, go, null, out error);
+ return AssignObjectReference(prop, go, componentFilter, out error);
}Also applies to: 686-690, 794-819
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 600 - 601, The
component filter parsed into the variable componentFilter is not being forwarded
when the payload uses the { "name": ... } branch, and
ResolveSceneObjectByName(...) is being called with a hardcoded null component;
update the name-branch code path to pass componentFilter through to the same
resolution logic (i.e. call ResolveSceneObjectByName(name, componentFilter) or
otherwise forward componentFilter) so that ResolveSceneObjectByName receives the
requested component type instead of null; ensure all analogous spots noted (the
blocks around the { "name": ... } branch and the call sites that currently pass
null) are updated to use the componentFilter variable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issues
#816
Additional Notes
Summary by Sourcery
Add configurable MCP execution logging and improve component/property handling and annotations across Unity and server tools.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Refactor